Fix PR #2 review follow-ups - #3
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request adds external camera and video-doorbell support, including still-based streaming and doorbell-trigger wiring. It centralizes protocol and HTTP decoding, adds timeout and retry handling, hardens client and pairing lifecycles, serializes mode commands, and expands probe validation. The Homebridge UI pairing flow and temperature validation are updated, while documentation, build configuration, CI, and release workflows are revised. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Camera/doorbell: - Publish camera and video-doorbell accessories externally; HomeKit does not render a CameraController on a bridged accessory, so the feature was non-functional as registered. - Use a DoorbellController (not a bare Doorbell service + CameraController) for the video doorbell so ring notifications carry the interior snapshot. - Expire cached snapshots after ~240s so idle live-view taps fail fast instead of spawning ffmpeg against an expired pre-signed URL. - Guard the ffmpeg exit/error/spawn handlers against a torn-down session to avoid a spurious forceStopStreamingSession and a leaked pump interval. Lifecycle/telemetry: - Tolerate a null/non-string refresh_token instead of aborting token refresh. - Re-check stopped after startup awaits so a stop() during start() cannot leak the 60s status poll. - Reset the probe temperature sensor when the probe is unplugged so stale hot readings don't stay latched. - Keep a just-started mode switch on until the oven reports active (was flipped off by preheat/poll telemetry) and iterate a services snapshot when removing stale mode services. Pairing: - Supersede a lingering active session on begin() so pairing can be retried after the config UI loses the session id. - Keep polling on a transient non-array devices body instead of failing. - Tolerate a null refresh_token in the registration response. Config UI: - Preserve an entered mode temperature of 0 instead of coercing it to 350. - Apply the request timeout to config writes, matching the read path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@cursor review |
|
Insufficient balance to process this code review. Please add funds or upgrade your plan in billing. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/protocol.ts (1)
124-126: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnforce mode-temperature validation during normalization.
normalizeOvenConfigis the shared boundary forconfig.json, butm.tempF ?? 350accepts non-finite or out-of-range runtime values. The UI declares a100–550°Frange, whileJuneClientlater passes this value directly tostartMode; validate the numeric bounds here as well so hand-edited configuration cannot bypass them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/protocol.ts` around lines 124 - 126, Update mode normalization in normalizeOvenConfig to validate m.tempF as a finite numeric value within the UI’s 100–550°F range before passing it to startMode. Preserve the existing 350°F default when temperature is absent, and ensure invalid or out-of-range configured values are normalized to a safe valid temperature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-08-expanded-homekit-features.md`:
- Around line 3-4: Remove the blank line within the blockquote in the historical
plan note, or prefix that line with “>”, so the entire note remains a continuous
blockquote and satisfies markdownlint MD028.
In `@docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md`:
- Around line 92-95: Update the “Video Doorbell integration is shipped” section
to state that enabling the camera publishes it as a separate external accessory
requiring users to add it in Home with the Homebridge PIN. Remove the claim that
the camera attaches to the same accessory without restructuring or re-pairing,
while preserving the doorbell’s standalone behavior.
In `@homebridge-ui/public/index.html`:
- Around line 352-354: Update the temperature change handler for
.field-mode-temp to reject empty raw input and values outside the inclusive
100–550 range before calling updateMode. Validate event.target.value before
numeric conversion, and preserve the 350 fallback for invalid or out-of-range
values.
In `@src/accessories/mode-switch.ts`:
- Around line 17-20: Ensure the activation latch represented by
awaitingActiveSubtype is cleared when the transition is externally cancelled or
fails to become active, rather than only on active: true or an accessory-issued
successful cancel. Update the mode-switch transition handling and JuneClient
integration as needed to propagate cancellation or apply an expiry, while
preserving suppression of premature inactive telemetry during valid activation;
add a regression test covering the latch clearing and subsequent active: false
handling.
In `@src/pairing.ts`:
- Around line 404-417: Update JunePairingSession.begin() to verify the session
remains open after each awaited asynchronous initialization operation, including
registerDevice() and requestPairingCode(), before continuing or returning
success. If destroy() or another close action occurred, stop initialization and
ensure no WebSocket is opened or success is returned for the destroyed session.
In `@src/protocol.test.ts`:
- Around line 57-58: Update the test “drops mode entries missing a
primitiveType” to include a runtime-shaped mode entry that omits the
primitiveType property, using a narrow cast if required by the type system.
Preserve the existing empty-string case only if it remains relevant, and ensure
the assertions verify entries missing the property are dropped.
---
Outside diff comments:
In `@src/protocol.ts`:
- Around line 124-126: Update mode normalization in normalizeOvenConfig to
validate m.tempF as a finite numeric value within the UI’s 100–550°F range
before passing it to startMode. Preserve the existing 350°F default when
temperature is absent, and ensure invalid or out-of-range configured values are
normalized to a safe valid temperature.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8290cfa8-c21d-4d64-99d1-ec5e5c3dea6f
📒 Files selected for processing (21)
README.mddocs/superpowers/plans/2026-07-08-doorbell-probe-polish.mddocs/superpowers/plans/2026-07-08-expanded-homekit-features.mddocs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.mdhomebridge-ui/public/index.htmlhomebridge-ui/public/june-ui.jssrc/accessories/camera.test.tssrc/accessories/camera.tssrc/accessories/doorbell.tssrc/accessories/mode-switch.tssrc/accessories/probe-sensor.tssrc/accessories/review-fixes.test.tssrc/june-client-lifecycle.test.tssrc/june-client.test.tssrc/june-client.tssrc/pairing.test.tssrc/pairing.tssrc/platform.test.tssrc/platform.tssrc/protocol.test.tssrc/protocol.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 7 file(s) based on 6 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 7 file(s) based on 6 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Long preheat drops mode switch
- Removed the arbitrary activation-latch expiry so acknowledged modes remain on throughout preheat until active telemetry or cancellation clears the latch.
- ✅ Fixed: Startup retry revives stopped client
- Added a stopped-state check inside the queued startup retry callback before it can invoke start and revive the client.
- ✅ Fixed: Pairing backoff exceeds session deadline
- Reset the five-minute pairing deadline when association polling begins so user confirmation time no longer consumes the polling window.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 81b7069. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md (1)
123-127: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented probe-value lifecycle.
The shipped
src/accessories/probe-sensor.tsresets the characteristic to0 °CwhenprobePresent === false; it does not retain the last value across probe-removal updates. Update this section to match the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md` around lines 123 - 127, Update the probe-value lifecycle description in the section referencing sensor_data.probe and probePresent to state that the characteristic resets to 0 °C when the probe is absent, rather than retaining the last reported value across removal updates.src/pairing.ts (1)
351-397: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame unguarded-post-await pattern resurfaces in
waitForAssociation()'s success path.
begin()was hardened withassertOpen()after every awaited step specifically to prevent a superseded/destroyed session from mutating state or reporting success (per prior critical review feedback).waitForAssociation()gets the same treatment before/after the delay, but not after the association fetch itself: once it resolves, lines 384-390 readthis.registration.deviceId/password/accessToken/refreshTokenand then callsetState('paired')/close()with no re-check.destroy()nullsthis.registration, so if it runs while this fetch is in flight, this throws a rawTypeErrorinstead of the intended "Pairing session was superseded." error — currently masked only becausefail()no-ops whenthis.closedis already true. That's an incidental safety net, not an explicit guard, and it's inconsistent with the pattern this PR just established elsewhere in the same class.🔒️ Proposed fix
const { response, body: bodyValue } = await fetchJsonWithTimeout(`${JUNE_API_URL}/2/devices/${this.registration.deviceId}/associated`, { headers: { Authorization: `Bearer ${this.registration.accessToken}`, 'User-Agent': JUNE_USER_AGENT }, }); + this.assertOpen(); if (!response.ok) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pairing.ts` around lines 351 - 397, Add an explicit this.assertOpen() immediately after fetchJsonWithTimeout() returns in waitForAssociation(), before reading this.registration or mutating status. Preserve the existing response validation and success flow, while ensuring superseded sessions raise the intended pairing-session error instead of accessing cleared registration state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md`:
- Around line 92-94: Update the later camera integration statement near the
doorbell pairing instructions to consistently describe the camera as a
separately published external accessory, matching the contract established in
the “Video Doorbell integration is shipped” section. Remove or revise wording
that says the camera is part of the same accessory, while preserving the
standalone doorbell behavior and Home app pairing guidance.
In `@package.json`:
- Line 43: Restore a guaranteed build before publication by adding npm run build
to package.json’s prepublishOnly script. Also update
.github/workflows/release.yml at lines 31-31 so the release job runs the build
before either publish command; both sites must enforce compilation before
publishing.
In `@src/pairing.ts`:
- Line 352: Update the per-attempt delay in the association polling flow around
calculateAssociationDelay to unref the setTimeout timer using the same optional
unref pattern as the file’s other timers, while preserving the existing Promise
delay behavior.
---
Outside diff comments:
In `@docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md`:
- Around line 123-127: Update the probe-value lifecycle description in the
section referencing sensor_data.probe and probePresent to state that the
characteristic resets to 0 °C when the probe is absent, rather than retaining
the last reported value across removal updates.
In `@src/pairing.ts`:
- Around line 351-397: Add an explicit this.assertOpen() immediately after
fetchJsonWithTimeout() returns in waitForAssociation(), before reading
this.registration or mutating status. Preserve the existing response validation
and success flow, while ensuring superseded sessions raise the intended
pairing-session error instead of accessing cleared registration state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 97238ed5-012b-479c-b873-cf812bdb92dc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
.github/workflows/ci.yml.github/workflows/release.yml.gitignoredocs/superpowers/plans/2026-07-08-expanded-homekit-features.mddocs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.mddocs/superpowers/specs/2026-07-22-code-optimization-hardening-design.mdhomebridge-ui/public/index.htmlhomebridge-ui/public/june-ui.jspackage.jsonsrc/accessories/accessories.test.tssrc/accessories/camera.test.tssrc/accessories/camera.tssrc/accessories/doorbell.tssrc/accessories/mode-switch.tssrc/accessories/preheat-switch.tssrc/accessories/probe-sensor.tssrc/accessories/sensors.tssrc/accessories/thermostat.tssrc/homebridge-ui.test.tssrc/http.test.tssrc/http.tssrc/june-client-lifecycle.test.tssrc/june-client.tssrc/pairing.test.tssrc/pairing.tssrc/platform.tssrc/protocol-decode.tssrc/protocol.test.tssrc/protocol.tstsconfig.json
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
| **Video Doorbell integration is shipped.** When both options are enabled, the Camera is published | ||
| as a separate external accessory, requiring users to add it in the Home app with the Homebridge | ||
| PIN. The doorbell operates standalone whether the camera is enabled or not. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the camera accessory contract consistent.
Lines 92–94 correctly describe a separately published camera, but Lines 115–116 still say it is implemented as part of the same accessory as the doorbell. Update the later statement so users are not given conflicting pairing instructions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-07-08-june-expanded-homekit-features-design.md`
around lines 92 - 94, Update the later camera integration statement near the
doorbell pairing instructions to consistently describe the camera as a
separately published external accessory, matching the contract established in
the “Video Doorbell integration is shipped” section. Remove or revise wording
that says the camera is part of the same accessory, while preserving the
standalone doorbell behavior and Home app pairing guidance.
| "test": "vitest run", | ||
| "prepack": "npm run build", | ||
| "prepublishOnly": "npm run lint && npm run test && npm run build" | ||
| "prepublishOnly": "npm run lint && npm test" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore a guaranteed build before publication.
Both the package lifecycle and release workflow now omit compilation, so published artifacts may contain stale or missing dist output.
package.json#L43-L43: restorenpm run buildinprepublishOnly, or make another publish-time build authoritative..github/workflows/release.yml#L31-L31: ensure the release job runs that build before either publish command.
📍 Affects 2 files
package.json#L43-L43(this comment).github/workflows/release.yml#L31-L31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 43, Restore a guaranteed build before publication by
adding npm run build to package.json’s prepublishOnly script. Also update
.github/workflows/release.yml at lines 31-31 so the release job runs the build
before either publish command; both sites must enforce compilation before
publishing.
| await new Promise(resolve => setTimeout(resolve, 3000)); | ||
| const response = await fetchWithTimeout(`${JUNE_API_URL}/2/devices/${this.registration.deviceId}/associated`, { | ||
| this.assertOpen(); | ||
| await new Promise(resolve => setTimeout(resolve, calculateAssociationDelay(attempt))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unref the per-attempt polling delay timer.
Every other timer in this file (deadline, eviction timers) is .unref?.()'d so it doesn't keep the process alive. This delay's setTimeout isn't, so a pending association poll can hold the event loop open (up to 30s per attempt) even during shutdown.
🔒️ Proposed fix
- await new Promise(resolve => setTimeout(resolve, calculateAssociationDelay(attempt)));
+ await new Promise<void>(resolve => {
+ const timer = setTimeout(resolve, calculateAssociationDelay(attempt));
+ timer.unref?.();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise(resolve => setTimeout(resolve, calculateAssociationDelay(attempt))); | |
| await new Promise<void>(resolve => { | |
| const timer = setTimeout(resolve, calculateAssociationDelay(attempt)); | |
| timer.unref?.(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pairing.ts` at line 352, Update the per-attempt delay in the association
polling flow around calculateAssociationDelay to unref the setTimeout timer
using the same optional unref pattern as the file’s other timers, while
preserving the existing Promise delay behavior.
Summary
Why
PR #2 review feedback identified stream lifecycle leaks, a URL-validation bypass through redirects, repeated doorbell notifications, and state races between telemetry and mode commands.
Validation
npm test— 46 tests passednpm run lintnpm run buildgit diff --checkNote
Medium Risk
Changes affect live camera/ffmpeg sessions, external HomeKit accessory publishing, and oven command/token/pairing paths; regressions could impact streaming stability or device control, though coverage was expanded with unit tests.
Overview
Addresses PR #2 review feedback with hardening across camera streaming, accessory state, client security, and platform publishing.
Camera / video doorbell: Stream prep and ffmpeg lifecycle now use single-shot callbacks, shutdown clears all sessions, and failures call
forceStopStreamingSession. Snapshot and live fetches useredirect: 'error'.10011URLs are restricted to trusted HTTPS hosts; cached stills expire after ~240s. Camera and combined doorbell+camera are published viapublishExternalAccessories(withDoorbellControllerwhen both are enabled); README notes manual Home app pairing for external tiles.Doorbell & modes: Shared
watchDoorbellTriggersrings only on rising edges of ready/done. Mode switches serialize commands, ignore staleactive: falsewhile commands are in flight or preheat is pending, and the Config UI clamps mode temps (100–550°F).Probe & config UI: Probe sensor widens HomeKit temp bounds and clears to 0°C when unplugged. Config save paths are wrapped in UI timeouts.
Client & pairing: Token refresh tolerates null/missing
refresh_token;start()bails if stopped mid-boot. Pairing supersedes stuck sessions instead of blocking retries; association polling tolerates transient invalid bodies.Docs/plan files are updated to match the shipped feature contract.
Reviewed by Cursor Bugbot for commit caa930c. Bugbot is set up for automated code reviews on this repo. Configure here.