Skip to content

Navigation Menu

Sign in
Sign up

Restore dashboard, add LICENSE + CI, fix test imports - #1

Open
devin-ai-integration[bot] wants to merge 2 commits into
main from
devin/1788637983-restore-dashboard-ci
Open

Restore dashboard, add LICENSE + CI, fix test imports #1
devin-ai-integration[bot] wants to merge 2 commits into
main from
devin/1788637983-restore-dashboard-ci

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Sep 5, 2026
edited
Loading

Copy link
Copy Markdown

Summary

The Updated workflow commit (4150bad) deleted all of dashboard/ while leaving dashboard/.vite/deps_temp_*/package.json behind, 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 .vite junk, 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.jsx imported './hooks/useMQTT' (resolves to src/components/hooks/...) → '../hooks/useMQTT'.
  • Two different hooks both lived at src/hooks/useMQTT.{js,jsx}; Vite resolves .js first, so every page got the wrong hook shape ({connected, messages} instead of {status, data}). The .js one is a distinct lower-level client + getMockData, renamed to useMQTTClient.js, and api/client.js updated to import from it. npm run build now succeeds.

Teststest_behaviour_model.py and test_drift_detector.py both did sys.path.insert(...) + from main import ..., so the second one to import got the first service's cached main module. Added load_service_module() in conftest.py which loads each service's main.py under a unique sys.modules name, plus the runtime deps the service modules pull in (aiomqtt, influxdb-client, apscheduler, ...) to tests/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_event assumed an ISO string; a datetime (or a non-ISO MQTT timestamp payload) crashed the message loop. Now accepts either.
  • RuleEngine.validate_action did action["device_id"] and system_state["occupancy"].get("occupied"), raising KeyError/AttributeError on any action without a device id or a boolean occupancy — in a safety-check path. Now tolerant, and the HVAC bound also covers the {"action": "set_temperature", "params": {"temperature": ...}} shape used by services/api/safety_filter.py.

38 tests pass (was 30 passing + 2 collection errors).

Other: added the MIT LICENSE the README badge already linked to, a .github/workflows/ci.yml running pytest + the dashboard build on every PR, and .vite//dist/ to .gitignore. Also guarded the JWT secret:

SECRET_KEY = os.getenv("JWT_SECRET")
if not SECRET_KEY:
 if os.getenv("NESTSHIFT_ENV", "development") == "production":
 raise RuntimeError("JWT_SECRET must be set when NESTSHIFT_ENV=production")
 SECRET_KEY = "dev-secret-change-in-prod"

Not addressed: RESEARCH.md still references figures/, 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


Devin Review

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Copy link
Copy Markdown
Author

🤖 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>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Author

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)

Devin Review

Comment on lines +89 to +90
clientId: 'nestshift-dashboard',
reconnectPeriod: 5000,

@devin-ai-integration devin-ai-integration Bot Sep 5, 2026

Copy link
Copy Markdown
Author

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.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

import { useState, useEffect, useCallback } from 'react'
import mqtt from 'mqtt'

const MQTT_BROKER = 'ws://localhost:9001'

@devin-ai-integration devin-ai-integration Bot Sep 5, 2026

Copy link
Copy Markdown
Author

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.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +127 to +131
if (key === 'overview') updated.overview = { ...prev.overview, ...payload }
else if (key === 'brain') updated.brain = { ...prev.brain, ...payload }
else if (key === 'energy') updated.energy = { ...prev.energy, ...payload }
else if (key === 'devices') updated.devices = { ...prev.devices, ...payload }
else if (key === 'safety') updated.safety = { ...prev.safety, ...payload }

@devin-ai-integration devin-ai-integration Bot Sep 5, 2026

Copy link
Copy Markdown
Author

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.
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

import { useState, useEffect, useCallback } from 'react'
import mqtt from 'mqtt'

const MQTT_BROKER = 'ws://localhost:9001'

@devin-ai-integration devin-ai-integration Bot Sep 5, 2026

Copy link
Copy Markdown
Author

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.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +20 to +21
const response = await fetch('http://localhost:8123/api/');
setHaReady(response.ok);

@devin-ai-integration devin-ai-integration Bot Sep 5, 2026

Copy link
Copy Markdown
Author

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.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

AltStyle によって変換されたページ (->オリジナル) /