Repository navigation
fix(bridge): kid/smart toggle reports ok:True + device_pushed, not a false failure - #147
Closed
BrettKinny wants to merge 1 commit into
Closed
BrettKinny wants to merge 1 commit into
BrettKinny wants to merge 1 commit into
Conversation
…false failure
Audit finding. The dashboard kid/smart-mode setters persisted + hot-applied the
flip, then returned {ok:False} when the firmware didn't acknowledge the LED push.
The dashboard rendered "toggle failed", the operator re-toggled, and the state
ping-ponged — even though the bridge state was already correct.
Mirror the /admin/kid-mode shape: setters now return
{ok:True, device_pushed:<bool>, warning:<str|None>}. The dashboard routes pass
device_pushed through; kid_mode_result.html / smart_mode_result.html render the
normal success alert plus a "saved, but the on-device LED may be stale" note
(plain <small>, no new Tailwind class) when device_pushed is False. A genuine
bridge-write exception still surfaces as an error via the route's try/except.
Tests: new tests/test_toggle_shape.py (7 cases) — setter shape for kid+smart
(ack vs no-ack) and route render (no-ack → success+warning, not "failed";
ack → success, no warning).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a dashboard UX/logic bug where kid/smart-mode toggles could persist + hot-apply correctly but still report {ok: False} when the firmware didn’t ACK the LED push, causing operators to re-toggle and “ping-pong” the state. It updates the bridge setter return shape and dashboard rendering so “no ACK” is treated as a successful save with an explicit “LED may be stale” notice.
Changes:
- Update kid/smart dashboard setters to return
{ok: True, device_pushed: bool, warning: str|None}even when firmware doesn’t ACK. - Pass
device_pushedthrough dashboard routes and render a non-error “LED may be stale” note in result templates whendevice_pushedis false. - Add tests validating setter shape and route rendering behavior for ACK vs no-ACK scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
bridge.py |
Changes dashboard setter return shape to avoid false failures on firmware no-ACK. |
bridge/dashboard.py |
Routes now pass device_pushed into the template context on success. |
bridge/templates/kid_mode_result.html |
Shows a success + “LED may be stale” note when device_pushed is false. |
bridge/templates/smart_mode_result.html |
Same “LED may be stale” success note for smart-mode toggles. |
tests/test_toggle_shape.py |
Adds tests for setter result shape and dashboard render outcomes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+638
to
+642
| # Bridge state is already persisted + hot-applied; a firmware no-ack only | ||
| # means the on-device LED pip is stale, not that the toggle failed. | ||
| # Report ok:True with device_pushed:false (mirroring /admin/kid-mode) so | ||
| # the dashboard doesn't show "toggle failed" and the operator doesn't | ||
| # re-toggle into a kid-mode state ping-pong. |
Comment on lines
+127
to
+134
| def test_smart_route_no_ack_renders_success_with_warning(self): | ||
| dash._state["smart_mode_setter"] = AsyncMock( | ||
| return_value={"ok": True, "device_pushed": False, "warning": "stale"} | ||
| ) | ||
| b = _body(asyncio.run(dash.smart_mode_set(_req(), enabled="on"))) | ||
| self.assertIn("Smart Mode", b) | ||
| self.assertNotIn("toggle failed", b) | ||
| self.assertIn("LED may be stale", b) |
Comment on lines
+14
to
+28
| import asyncio | ||
| import importlib.util | ||
| import os | ||
| import sys | ||
| import tempfile | ||
| import unittest | ||
| from contextlib import asynccontextmanager | ||
| from pathlib import Path | ||
| from unittest.mock import AsyncMock | ||
|
|
||
| from starlette.requests import Request | ||
|
|
||
| _state_dir = Path(tempfile.mkdtemp(prefix="dotty-toggle-state-")) | ||
| os.environ.setdefault("DOTTY_KID_MODE_STATE", str(_state_dir / "kid-mode")) | ||
| os.environ.setdefault("DOTTY_SMART_MODE_STATE", str(_state_dir / "smart-mode")) |
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
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.
Audit finding. Stacked on #146 (base is the kid-filter branch).
Bug
The dashboard kid/smart-mode setters persisted + hot-applied the flip, then returned
{ok:False}when the firmware didn't acknowledge the LED push. The dashboard rendered "toggle failed", the operator re-toggled, and the state ping-ponged — even though the bridge state was already correct.Fix
Mirror the
/admin/kid-modeshape: setters return{ok:True, device_pushed:<bool>, warning:<str|None>}. The dashboard routes passdevice_pushedthrough;kid_mode_result.html/smart_mode_result.htmlrender the normal success alert plus a "saved, but the on-device LED may be stale" note (plain<small>, no new Tailwind class → no CSS rebuild) whendevice_pushedis False. A genuine bridge-write exception still surfaces as an error via the route'stry/except.Tests
New
tests/test_toggle_shape.py(7 cases): setter shape for kid+smart (ack vs no-ack), and route render (no-ack → success + warning, not "failed"; ack → success, no warning). Fulltests/suite 62 passed; ruff clean.Deploys via the bridge container.
🤖 Generated with Claude Code