Pr 03 dashboard settings update ux#353
Pr 03 dashboard settings update ux#353eva57gr wants to merge 3 commits intoLight-Heart-Labs:mainfrom
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
Blocking: script_path leaked in API response
/api/update/readiness returns the absolute filesystem path of the update script. The frontend never uses this value. Remove it from the response — leaking internal paths is unnecessary information disclosure.
Blocking: _collect_rollback_state has no error handling
Path.glob on a nonexistent backups directory will raise. Since this is an I/O boundary, add a targeted FileNotFoundError catch returning {"available": False, "backup_count": 0}.
Blocking: Coordinate with PR #363
Both PRs substantially modify Settings.jsx, updates.py, README.md, and test files. Need a defined merge order — the second to merge will require a non-trivial rebase.
Non-blocking:
- Version-state spread-merge pattern repeated 3 times with subtly different shapes — consider a normalizer function
- "Refresh this page in a minute" after update — no auto-refresh or polling, users could miss the window
🤖 Reviewed with Claude Code
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review — Needs description and coordination with #363/#364.
PR title is "Pr 03 dashboard settings update ux" — no description, no context. +456/-23 across 7 files touching dashboard frontend and API.
Without a description, I can only infer from the diff that this adds UX for the settings/update flow in the dashboard. But #363 and #364 both add /api/settings backends — which one does this frontend consume?
Recommendation: Add a PR description explaining:
- What UX flows are added/changed
- Which backend PR this depends on (#363 or #364)
- Screenshots or before/after if it's a UI change
Has CHANGES_REQUESTED from a prior reviewer.
|
Closing per maintainer request. The work here is appreciated — please reopen or create a fresh PR if you'd like to continue this effort. |
No description provided.