-
Notifications
You must be signed in to change notification settings - Fork 0
Restore dashboard, add LICENSE + CI, fix test imports - #1
Restore dashboard, add LICENSE + CI, fix test imports #1devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI Engineer
I'll be helping with this pull request! Here's what you should know:
✅ I will automatically:
- Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
- Look at CI failures and help fix them
Note: I can only respond to comments from users who have write access to this repository.
⚙️ Control Options:
- Disable automatic comment, CI, and merge conflict monitoring
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin Review found 5 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Duplicate MQTT clients reconnect forever
Each route mounts separate page and Layout clients under one clientId. The broker evicts the existing client whenever its duplicate connects. Their automatic reconnects can loop indefinitely, leaving telemetry unstable.
Prompt for agents
The active layout and every active page each call useMQTT, so one route creates at least two mqtt.js clients. Both currently use the same fixed clientId in dashboard/src/hooks/useMQTT.jsx, and reconnectPeriod makes both retry after the broker disconnects the duplicate. Refactor MQTT ownership so the application has one shared connection and distributes status/data through context or a shared store. Alternatively, if multiple connections are intentional, assign a stable unique ID per hook instance and verify cleanup and reconnection behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Remote dashboards target the wrong broker
When users open the dashboard remotely, MQTT_BROKER targets their device instead of the NestShift hub. The connection never reaches the hub broker, so live status and telemetry remain unavailable.
Prompt for agents
The browser resolves ws://localhost:9001 against the viewing machine, but the deployed dashboard is intended to be opened remotely from the NestShift hub. Make the MQTT WebSocket endpoint configurable through Vite environment settings or derive it from window.location, and route it to the hub broker through the deployment proxy. Preserve an explicit localhost default only for local development if needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Live brain data stays mocked
When nestshift/brain/status arrives, handleMessage adds snake_case fields beside the camelCase values rendered by Brain. The heartbeat publishes none of those display fields. Brain keeps showing fabricated startup values while reporting connected.
Prompt for agents
The active Brain page expects the camelCase MOCK_DATA schema, while services/brain/nare.py publishes snake_case heartbeat fields with different names and granularity. Add an explicit adapter in handleMessage for nestshift/brain/status that maps the service contract into the page model, and decide how unavailable metrics such as hourly spikes, latency, manual overrides, and intent distribution are represented rather than retaining mock values. Audit device and energy topic adapters for the same aggregate-versus-event schema mismatch.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟨 MQTT telemetry crosses plaintext WebSockets
The dashboard connects through ws://, exposing home telemetry to network interception and modification. Production transport requires an authenticated wss:// endpoint.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟨 Readiness check assumes an open home API
The browser treats an unauthenticated Home Assistant response as readiness. Secured installations return 401, encouraging an exposed API configuration to complete startup.
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The
Updated workflowcommit (4150bad) deleted all ofdashboard/while leavingdashboard/.vite/deps_temp_*/package.jsonbehind, so the README's quick start (cd dashboard && npm install && npm run dev) pointed at an empty directory. This restores the app from 4150bad^, drops the.vitejunk, and adds the missing guardrails that would have caught the regression.Dashboard — restored as-is except two import bugs that made it unbuildable even before the deletion:
Layout.jsximported'./hooks/useMQTT'(resolves tosrc/components/hooks/...) →'../hooks/useMQTT'.src/hooks/useMQTT.{js,jsx}; Vite resolves.jsfirst, so every page got the wrong hook shape ({connected, messages}instead of{status, data}). The.jsone is a distinct lower-level client +getMockData, renamed touseMQTTClient.js, andapi/client.jsupdated to import from it.npm run buildnow succeeds.Tests —
test_behaviour_model.pyandtest_drift_detector.pyboth didsys.path.insert(...)+from main import ..., so the second one to import got the first service's cachedmainmodule. Addedload_service_module()inconftest.pywhich loads each service'smain.pyunder a uniquesys.modulesname, plus the runtime deps the service modules pull in (aiomqtt,influxdb-client,apscheduler, ...) totests/requirements.txt.Once those two files actually imported, they exposed real bugs in
services/automation-agent/main.py, fixed here rather than in the tests:record_eventassumed an ISO string; adatetime(or a non-ISO MQTTtimestamppayload) crashed the message loop. Now accepts either.RuleEngine.validate_actiondidaction["device_id"]andsystem_state["occupancy"].get("occupied"), raisingKeyError/AttributeErroron any action without a device id or a booleanoccupancy— in a safety-check path. Now tolerant, and the HVAC bound also covers the{"action": "set_temperature", "params": {"temperature": ...}}shape used byservices/api/safety_filter.py.38 tests pass (was 30 passing + 2 collection errors).
Other: added the MIT
LICENSEthe README badge already linked to, a.github/workflows/ci.ymlrunning pytest + the dashboard build on every PR, and.vite//dist/to.gitignore. Also guarded the JWT secret:Not addressed:
RESEARCH.mdstill referencesfigures/, which the same commit deleted.Link to Devin session: https://app.devin.ai/sessions/202689cf4384414db4cbcb092a7ed2e7
Open in Devin Desktop: https://app.devin.ai/desktop/session/202689cf4384414db4cbcb092a7ed2e7?variant=devin
Requested by: @aryan597