Skip to content

feat(dashboard): add settings API contract#363

Closed
powerfulDev-cloud wants to merge 3 commits intoLight-Heart-Labs:mainfrom
powerfulDev-cloud:feat-dashboard-settings-contract
Closed

feat(dashboard): add settings API contract#363
powerfulDev-cloud wants to merge 3 commits intoLight-Heart-Labs:mainfrom
powerfulDev-cloud:feat-dashboard-settings-contract

Conversation

@powerfulDev-cloud
Copy link

No description provided.

Copy link
Collaborator

@Lightheartdevs Lightheartdevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: REQUEST CHANGES — Hard conflict with PR #364

Blocking: Incompatible /api/settings implementations

PR #363 adds GET /api/settings in main.py with a rich payload (version, services, GPU, model, updates, hostname). PR #364 adds GET /api/settings in routers/runtime.py with a slimmer payload (version, tier, uptime, storage). Different response shapes, different locations, different tests. Cannot both merge.

Recommendation: Merge the richer payload from #363 into the routers/runtime.py location from #364. Both PRs also have near-identical _format_uptime helpers.

Blocking: Verify triggerUpdate exists

Settings.jsx imports triggerUpdate from ../hooks/useVersion, but no changes to that hook are in the diff. If this function doesn't exist on the target branch, the build breaks.

Non-blocking:

  • _read_version_state silently returns ("0.0.0", {}) on JSON parse failure — add logger.warning
  • socket.gethostname() exposed in API — unnecessary since frontend already has window.location.hostname

🤖 Reviewed with Claude Code

Copy link
Collaborator

@Lightheartdevs Lightheartdevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — Needs more context in PR description.

+355/-115 across 6 files for a settings API contract, but the PR body is empty beyond the title. Need:

  1. What endpoints are added/changed?
  2. How does this relate to #364 (which also adds settings APIs)?
  3. Are these frontend-only changes or backend too?

Please add a description so reviewers can evaluate without reading 470 lines of diff cold.

Copy link
Collaborator

@Lightheartdevs Lightheartdevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes — Overlaps with #364, needs coordination.

What it does

Adds /api/settings endpoint to main.py (inline, not in a router) that aggregates system status, storage, version info, and service list into one payload. Also refactors _compute_storage into a shared function and adds id to service status objects.

Overlap with #364

PR #364 (@championVisionAI) also adds /api/settings — but as a dedicated routers/runtime.py with a cleaner implementation that includes voice, diagnostics, and settings in one router. These two PRs will directly conflict — both define GET /api/settings with different response shapes.

Comparison

Aspect #363 (this PR) #364 (championVisionAI)
Location Inline in main.py routers/runtime.py
Settings shape Includes services list, GPU, model, updates, storage Version, tier, uptime, storage (simpler)
Voice endpoints None Full voice settings + token minting
Diagnostics None /api/test/{llm,voice,rag,workflows}
Tests Removes some existing tests, doesn't add new ones 10 new tests

Issues

  1. No PR description — just a title. Reviewers can't understand intent without reading the full diff.
  2. Removes existing teststest_api_status_authenticated, test_api_storage_authenticated, test_api_external_links_authenticated, test_api_service_tokens_authenticated are removed without replacements covering the same endpoints.
  3. /api/settings conflicts with #364 — need to pick one implementation or merge them.

Recommendation

#364 is the stronger implementation (dedicated router, input validation, tests, voice + diagnostics). Recommend either:

  1. Close this PR in favor of #364, or
  2. Merge the _compute_storage_payload refactor and service id addition into #364

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants