Repository navigation
Add Venstar admin routes - #159
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughAdds Venstar thermostat discovery, adapter scan/status methods, types, state/config fields, mocks and tests, and two new routes (/venstar/status, /venstar/setup) with GET/POST handlers and server wiring. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Server
participant VenstarAdapter
participant Discovery
participant Thermostat
User->>Browser: Click "Scan" (/venstar/status POST)
Browser->>Server: POST /venstar/status
Server->>VenstarAdapter: venstar.scan()
VenstarAdapter->>Discovery: scanVenstarThermostats(state, config)
alt JSON discovery
Discovery->>Thermostat: GET discovery JSON endpoint
Thermostat-->>Discovery: discovery JSON (list)
else SSDP discovery
Discovery->>Thermostat: M-SEARCH (UDP)
Thermostat-->>Discovery: SSDP response (LOCATION)
end
Discovery->>Thermostat: GET /query/info (per-device)
Thermostat-->>Discovery: info JSON (temp, humidity, etc.)
Discovery-->>VenstarAdapter: thermostats + diagnostics
VenstarAdapter-->>Server: update state, return scan result
Server->>Browser: Render status page (banner + lists)
Browser-->>User: Display configured/discovered/online/offline
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 13, 2026 03:35
Contributor
|
🔎 Preview deployed: https://kody-pr-159.kentcdodds.workers.dev Worker: Mocks:
|
![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: 4
🧹 Nitpick comments (1)
packages/home-connector/app/handlers.ts (1)
86-89: Consider isolating dashboard render from unexpected Venstar adapter errors.A small
try/catcharound the Venstar status fetch would prevent the entire home page render from failing if an unexpected exception escapes adapter internals.♻️ Suggested defensive fallback
- const venstarStatus = await venstar.listThermostatsWithStatus() + let venstarStatus: Awaited< + ReturnType<ReturnType<typeof createVenstarAdapter>['listThermostatsWithStatus']> + > = [] + try { + venstarStatus = await venstar.listThermostatsWithStatus() + } catch { + venstarStatus = [] + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/app/handlers.ts` around lines 86 - 89, Wrap the call to venstar.listThermostatsWithStatus() in a try/catch inside the handler so adapter exceptions don't break the page render: call venstar.listThermostatsWithStatus() in the try block, compute onlineVenstarCount from the returned array as before, and in catch set a safe fallback (e.g., an empty array or onlineVenstarCount = 0) and log the error (using the existing logger) so the dashboard can still render when venstar fails; update references to venstarStatus and onlineVenstarCount accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/home-connector/app/venstar-handlers.ts`:
- Around line 273-276: The venstar adapter created by createVenstarAdapter()
already closes over state, so remove the extra state argument from all its
calls: change invocations of venstar.scan(state) and venstar.getStatus(state) to
venstar.scan() and venstar.getStatus() (repeat for the other three sites
mentioned). Locate usages of venstar.scan and venstar.getStatus in this file and
eliminate the passed-in state parameter so the calls match the zero-arity
signatures exposed by createVenstarAdapter().
In `@packages/home-connector/src/adapters/venstar/discovery.ts`:
- Around line 109-128: The UDP message handler currently calls
normalizeDeviceLocation(locationHeader) twice and can throw if LOCATION is
malformed; instead, treat the LOCATION header as untrusted, call
normalizeDeviceLocation once inside a try/catch, skip adding/processing the hit
if normalization fails, and only store the normalized location in hits and
locations when successful. Update the socket.on('message') callback to: read
locationHeader, attempt to normalize it with normalizeDeviceLocation(...) inside
a try/catch, if it throws return/continue (skip this message), and then use that
single normalized value when pushing into hits and when adding to the locations
Map (referencing the callback for socket.on('message'), the
normalizeDeviceLocation function, the hits array, and the locations Map).
- Around line 84-90: The fetchJson function used by scanVenstarThermostats has
no timeout and can hang; update fetchJson<T>(url: string) to accept an optional
timeout (e.g., timeoutMs = 5000) and implement an AbortController that aborts
the fetch after the timeout, passing the controller.signal to fetch; ensure you
properly clear the timer, throw a clear timeout Error when abort fires (or
rethrow fetch errors) so callers like scanVenstarThermostats and any SSDP/JSON
feed lookups fail fast instead of hanging indefinitely.
In `@packages/home-connector/src/adapters/venstar/mock-driver.ts`:
- Around line 165-179: The USN generation in listMockVenstarDiscoveryEntries
uses the loop index which causes unstable USNs; change it to derive a stable
identifier from thermostat data instead (e.g., prefer state.info.mac, then
state.info.serial, then fallback to the IP) and use that stableId when composing
the usn string; update references inside listMockVenstarDiscoveryEntries (and
keep using the trimmed name logic from state.info.name) so removal/insertion
order no longer changes the USN for a given device.
---
Nitpick comments:
In `@packages/home-connector/app/handlers.ts`:
- Around line 86-89: Wrap the call to venstar.listThermostatsWithStatus() in a
try/catch inside the handler so adapter exceptions don't break the page render:
call venstar.listThermostatsWithStatus() in the try block, compute
onlineVenstarCount from the returned array as before, and in catch set a safe
fallback (e.g., an empty array or onlineVenstarCount = 0) and log the error
(using the existing logger) so the dashboard can still render when venstar
fails; update references to venstarStatus and onlineVenstarCount accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f0cf652-1ba3-483b-b546-5d896bfdd534
📒 Files selected for processing (20)
packages/home-connector/app/handlers.tspackages/home-connector/app/router.node.test.tspackages/home-connector/app/router.tspackages/home-connector/app/routes.tspackages/home-connector/app/venstar-handlers.tspackages/home-connector/mocks/venstar.tspackages/home-connector/server/dev-server.tspackages/home-connector/server/index.tspackages/home-connector/src/adapters/venstar/discovery.node.test.tspackages/home-connector/src/adapters/venstar/discovery.tspackages/home-connector/src/adapters/venstar/index.node.test.tspackages/home-connector/src/adapters/venstar/index.tspackages/home-connector/src/adapters/venstar/mock-driver.tspackages/home-connector/src/adapters/venstar/types.tspackages/home-connector/src/config.node.test.tspackages/home-connector/src/config.tspackages/home-connector/src/index.tspackages/home-connector/src/mcp/server.node.test.tspackages/home-connector/src/state.tspackages/home-connector/src/storage/index.node.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/home-connector/app/venstar-handlers.ts (1)
169-175: Minor: Inconsistent code formatting forparsedvsrawfields.
lookup.parsedis rendered inline with<code>(line 172), whilelookup.rawusesrenderCodeBlock(line 175). This creates visual inconsistency in the diagnostics UI—both are JSON, but one appears inline and the other as a block.Consider using
renderCodeBlockfor both, or making the distinction intentional based on expected data size.♻️ Suggested consistency fix
${lookup.parsed - ? html`<div> - Parsed: - <code>${formatJson(lookup.parsed)}</code> - </div>` + ? html`<div>Parsed:</div> + ${renderCodeBlock(formatJson(lookup.parsed))}` : ''}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/app/venstar-handlers.ts` around lines 169 - 175, The rendering is inconsistent: lookup.parsed is wrapped in an inline <code> while lookup.raw uses renderCodeBlock; change the UI to render both JSON fields consistently by replacing the inline <code>${formatJson(lookup.parsed)}</code> with renderCodeBlock(formatJson(lookup.parsed)) (or add a size-based conditional that uses renderCodeBlock for larger payloads), referencing lookup.parsed, lookup.raw, renderCodeBlock, and formatJson so both parsed and raw JSON display the same way.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/home-connector/app/venstar-handlers.ts`:
- Around line 169-175: The rendering is inconsistent: lookup.parsed is wrapped
in an inline <code> while lookup.raw uses renderCodeBlock; change the UI to
render both JSON fields consistently by replacing the inline
<code>${formatJson(lookup.parsed)}</code> with
renderCodeBlock(formatJson(lookup.parsed)) (or add a size-based conditional that
uses renderCodeBlock for larger payloads), referencing lookup.parsed,
lookup.raw, renderCodeBlock, and formatJson so both parsed and raw JSON display
the same way.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 833cbb27-5bee-49f1-9765-fef4972eb719
📒 Files selected for processing (2)
packages/home-connector/app/venstar-handlers.tspackages/home-connector/src/adapters/venstar/mock-driver.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/home-connector/src/adapters/venstar/mock-driver.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2043baf. Configure here.
| (thermostat) => | ||
| !configuredIps.has(normalizeThermostatIp(thermostat.ip)), | ||
| ), | ||
| allDiscovered: state.venstarDiscoveredThermostats, |
Contributor
There was a problem hiding this comment.
Unused allDiscovered property is dead code
Low Severity
The allDiscovered property returned by getStatus() is never read anywhere in the codebase. Only discovered (the filtered list), configured, and diagnostics are consumed by callers like venstar-handlers.ts. This is dead code that adds unnecessary data to the return value.
Reviewed by Cursor Bugbot for commit 2043baf. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/home-connector/app/handlers.ts (1)
70-73: Consider moving Venstar probe work off the dashboard request path.Line 70 awaits live thermostat status on every dashboard load. If endpoints are slow/unreachable, admin page latency can spike; a cached snapshot/background refresh model would keep this page snappier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/home-connector/app/handlers.ts` around lines 70 - 73, The dashboard handler currently awaits venstar.listThermostatsWithStatus() on each request (calculating onlineVenstarCount), which blocks the request path; instead implement a background refresh and cached snapshot: create a module-scoped cache (e.g., cachedVenstarStatus and lastUpdated) and a background updater (setInterval or a scheduled job) that calls venstar.listThermostatsWithStatus() and updates the cache, then change the handler to read from cachedVenstarStatus to compute onlineVenstarCount and return the cached value (optionally trigger a non-blocking refresh if data is stale), keeping function names like venstar.listThermostatsWithStatus() and the onlineVenstarCount computation but removing the direct await from the request path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/home-connector/app/handlers.ts`:
- Around line 70-73: The dashboard handler currently awaits
venstar.listThermostatsWithStatus() on each request (calculating
onlineVenstarCount), which blocks the request path; instead implement a
background refresh and cached snapshot: create a module-scoped cache (e.g.,
cachedVenstarStatus and lastUpdated) and a background updater (setInterval or a
scheduled job) that calls venstar.listThermostatsWithStatus() and updates the
cache, then change the handler to read from cachedVenstarStatus to compute
onlineVenstarCount and return the cached value (optionally trigger a
non-blocking refresh if data is stale), keeping function names like
venstar.listThermostatsWithStatus() and the onlineVenstarCount computation but
removing the direct await from the request path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9fb241a5-4afe-4757-8329-fddbde4a0b40
📒 Files selected for processing (19)
docs/contributing/environment-variables.mddocs/use/execute.mdpackages/home-connector/app/bond-handlers.tspackages/home-connector/app/handler-utils.tspackages/home-connector/app/handlers.tspackages/home-connector/app/router.node.test.tspackages/home-connector/app/sonos-handlers.tspackages/home-connector/app/venstar-handlers.tspackages/home-connector/src/adapters/venstar/discovery.tspackages/home-connector/src/adapters/venstar/index.node.test.tspackages/home-connector/src/adapters/venstar/mock-driver.tspackages/home-connector/src/mcp/server.tspackages/worker/src/env-schema.tspackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/scheduler/client.tspackages/worker/src/scheduler/process-due-jobs.tspackages/worker/src/scheduler/schedule.tspackages/worker/src/scheduler/scheduler-do.tspackages/worker/src/scheduler/types.ts
✅ Files skipped from review due to trivial changes (11)
- docs/use/execute.md
- packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts
- packages/worker/src/scheduler/process-due-jobs.ts
- packages/worker/src/scheduler/schedule.ts
- packages/worker/src/scheduler/client.ts
- docs/contributing/environment-variables.md
- packages/worker/src/scheduler/types.ts
- packages/home-connector/src/mcp/server.ts
- packages/worker/src/env-schema.ts
- packages/worker/src/scheduler/scheduler-do.ts
- packages/home-connector/app/handler-utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/home-connector/src/adapters/venstar/index.node.test.ts
- packages/home-connector/app/router.node.test.ts
- packages/home-connector/src/adapters/venstar/mock-driver.ts
- packages/home-connector/src/adapters/venstar/discovery.ts
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
2 participants
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
/venstar/statusand/venstar/setupadmin pages backed by the Venstar adapter and configVENSTAR_DISCOVERY_URLoverride and mock JSON discovery for local dev/testsLOCATIONheaders, making mock discovery USNs stable, and rendering parsed/raw diagnostics consistentlyTesting
npm run validatenpx vitest run --project mcp-e2enpx vitest run --root . --project node-unit "packages/home-connector/src/config.node.test.ts" "packages/home-connector/src/adapters/venstar/index.node.test.ts" "packages/home-connector/src/adapters/venstar/discovery.node.test.ts" "packages/home-connector/app/router.node.test.ts" "packages/home-connector/src/mcp/server.node.test.ts"npx oxlint "packages/home-connector/src/config.ts" "packages/home-connector/src/config.node.test.ts" "packages/home-connector/src/state.ts" "packages/home-connector/src/adapters/venstar/types.ts" "packages/home-connector/src/adapters/venstar/discovery.ts" "packages/home-connector/src/adapters/venstar/discovery.node.test.ts" "packages/home-connector/src/adapters/venstar/index.ts" "packages/home-connector/src/adapters/venstar/index.node.test.ts" "packages/home-connector/src/adapters/venstar/mock-driver.ts" "packages/home-connector/mocks/venstar.ts" "packages/home-connector/server/dev-server.ts" "packages/home-connector/src/index.ts" "packages/home-connector/app/router.ts" "packages/home-connector/app/venstar-handlers.ts" "packages/home-connector/app/router.node.test.ts" "packages/home-connector/src/mcp/server.node.test.ts" "packages/home-connector/src/storage/index.node.test.ts"Scan nowand verifying discovered thermostats plus diagnostics in the running appArtifacts
validate_full_gate_latest.log
mcp-e2e-direct.log
venstar-scan-tests.log
venstar-scan-status-check.log
Summary by CodeRabbit
New Features
Tests
Documentation