Merged
Conversation
Mirrors the Shelly NG discovery wizard for Gen 1 devices: scans the local network via the main connector's CoAP/mDNS browser, supports manual hostname + password entry, and adopts selected devices in batch. The wizard shares the proven session-based UX from shelly-ng (30s scan window, 5min finished-session TTL, client-side progress ticking, race-resilient adopt with create→update fallback, opt-in updates for already-registered devices) but speaks the Gen 1 HTTP API (Basic auth with `admin` username, `/shelly` for identification, `/status` for auth validation) instead of NG's RPC. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Align discovery `findDescriptor` with the substring + name fallback used by `device-mapper.service.ts` and `shelly-v1.device.platform.ts`. The wizard must agree with the main connector — strict equality would mark variants whose `type` carries an extra suffix as `unsupported` even though the connector adopts them fine. - Translate the wizard strings the previous commit left in English in `de-DE`, `es-ES`, and `pl-PL` (messages, texts, buttons, statuses). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Arm the finish timer and register the session BEFORE the seed loop so the wall-clock session lifetime matches `expiresAt`. The seed loop's per-device HTTP probe + DB lookup was pushing the real expiry several seconds past `expiresAt`, leaving the frontend's progress bar pinned at 100% while polling still reported `running`. - Drop the unused `passwords` map and `storePassword()` helper. Nothing ever read from them — the frontend tracks per-hostname passwords in its own reactive store and re-sends them with each adopt request. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Run the seed loop concurrently and don't block the start response on
it. Each `handleLibDevice` does a 3s HTTP probe, so awaiting them
sequentially would push the start response past the scan duration with
even a few unreachable devices in the lib's registry. Per-device
snapshots still land in `session.devices` as they finish, and the
frontend's 1Hz polling picks them up. `start()` is now synchronous;
the controller drops its `await` accordingly.
- Fix `@ValidateNested({ each: true })` on the discovery device's
`authentication` property — that's a single nested object, not an
array, so `each: true` was a no-op at best and a bug at worst.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Extract `findDescriptor` to a shared `descriptor.utils.ts` and route the
three (now four) callers through it: `ShellyV1DiscoveryService`,
`DeviceMapperService`, `ShellyV1DevicePlatform`, and `ShellyV1Service`.
All four were doing variants of the same model-substring + name-fallback
match — divergence was the exact failure mode that surfaced once already
in this PR's review thread.
- Switch the discovery endpoints to `toInstance(...)` for parity with the
rest of the V1 controller (`POST /info`, `GET /supported`). The discovery
models now carry `@Expose({ name: 'snake_case' })` / `@ApiProperty({ name:
'snake_case' })` aliases on every multi-word field, so the wire format
matches the snake_case convention the V1 plugin already uses elsewhere.
The frontend's `snakeToCamel` already handles both shapes, so the change
is non-breaking.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`addManualDevice` was caching the user's entered password in `passwordByHostname` whenever the backend returned a session snapshot — no matter what the inspect step actually said about validity. For an `already_registered` device, the DB-hit status takes priority over `needs_password`, so a wrong password against an existing device still produced `status: 'already_registered'` with `authentication.valid: false`. A subsequent Adopt click then read the bad password back out and passed it to `updateRegistered`, overwriting the correct on-disk password with the wrong one. Now we only persist the password when the inspect step didn't actively report invalid credentials. Manual entries against unauthenticated devices and against existing devices with a correct password still flow through; bad-password attempts no longer poison the store. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Stop the discovery poll timer when `refreshDiscovery` rejects. The most likely cause is a 404 after the backend GC'd the session, and the catch block was swallowing the error and letting the 1Hz interval keep firing until the component unmounted. - Tighten the `selectedDevices` filter to reject `undefined`, not just `null`. The map is typed `DevicesModuleDeviceCategory | null` but a missing key reads as `undefined` at runtime, so the previous `!== null` guard could let half-initialized rows through and `adoptSelected` would then cast `undefined` straight to `DevicesModuleDeviceCategory` on the way to the backend. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cdf62c3. Configure here.
Clicking "Scan again" called `start()` a second time, which built a fresh `subscribeToAddedDevice` listener on top of the previous one without canceling the old session. The lib then double-probed every newly-seen device, the orphan finish-timer kept ticking until it fired, and the frontend (which only tracks the latest session id) had no view of the duplicate work. Now `start()` first walks `this.sessions` and finishes anything still `running` — clearing its timer, detaching its `add` listener, and scheduling its 5-minute cleanup — before creating the new session. The finish path is fire-and-forget because its synchronous parts complete before the next loop tick. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
subscribeToAddedDevice()/getKnownDevices()pair onShelliesAdapterService— no parallel network listeners/statusImplementation notes
Backend (
apps/backend/src/plugins/devices-shelly-v1/)services/shelly-v1-discovery.service.ts— session lifecycle (30s scan window, 5min finished-TTL), seed fromgetKnownDevices()+ live subscription, manual lookup probes/shellythen validates auth via/statusservices/shellies-adapter.service.ts— addedgetKnownDevices()andsubscribeToAddedDevice()with cross-restart subscriber retentioncontrollers/shelly-v1-devices.controller.ts— new endpoints:POST /discovery,GET /discovery/{id},POST /discovery/{id}/manualmodels/shelly-v1.model.ts+models/shelly-v1-response.model.ts— discovery device / session models and response wrapper, registered indevices-shelly-v1.openapi.tsshellieslibrary'sdevice.idformat used by auto-discoveryAdmin (
apps/admin/src/plugins/devices-shelly-v1/)composables/useDevicesWizard.ts— session polling (1s), client-side scan-progress ticking that's resilient to clock skew, race-resilient adopt with create→update fallback, opt-in updates for already-registered devicescomponents/shelly-v1-devices-wizard.vue— three-step wizard with sortable categories table, select-all checkbox, and table-format results stepTest plan
pnpm --filter ./apps/backend exec jest src/plugins/devices-shelly-v1— 105/105 pass locallypnpm --filter ./apps/admin run test:unit— 1330/1330 pass locallyDevices → wizard/devices-shelly-v1-pluginwith a Gen 1 device on the LAN and confirm it appears via mDNS, can be adopted with category, and shows up in the devices listneeds_password), then with the right password (expectready)already_registeredwith opt-in update)🤖 Generated with Claude Code
Note
Medium Risk
Adds new backend discovery-session APIs and a new in-memory discovery service wired into the Shelly V1 connector, plus a new admin wizard flow that can create or update devices; mistakes could affect device adoption/update behavior but changes are scoped to the Shelly V1 plugin.
Overview
Adds a Shelly Gen 1 (V1) discovery/adoption wizard in the admin UI (Discovery → Categories → Results), including polling scan progress, manual hostname/password lookups, per-device selection/category/name editing, and a results table.
Introduces new backend support for the wizard via
POST /devices/discovery,GET /devices/discovery/:id, andPOST /devices/discovery/:id/manual, backed by a short-lived discovery session service that reuses the existing Shellies connector event stream (newgetKnownDevices()/subscribeToAddedDevice()), probes devices over HTTP for enrichment/auth validation, and exposes typed OpenAPI models.Refactors Shelly V1 descriptor matching into shared
findShellyV1Descriptor()and updates the platform/mapper/service to use it, and extends admin schemas/transformers/openapi aliases and i18n strings to support the new discovery payloads.Reviewed by Cursor Bugbot for commit e5b4a62. Bugbot is set up for automated code reviews on this repo. Configure here.