Settings overhaul: routed sub-tabs, server logs, changelog, about page#294
Merged
Settings overhaul: routed sub-tabs, server logs, changelog, about page#294
Conversation
…elog, reusable setting components - Rename Server tab to Settings with horizontal sub-tab navigation (General, Generation, GPU, Logs, Changelog) - All sub-tabs are proper routes under /settings/* with /server redirect for backwards compat - General: connection settings, link cards (docs + discord), API reference card, app updates - Generation: auto-chunking, crossfade, normalize, autoplay as SettingRow components - GPU: info card with platform-aware icons (Apple logo for MPS), CUDA management, explainer text - Logs: real-time server log viewer piped from Tauri sidecar via event system (Tauri-only) - Changelog: parsed from CHANGELOG.md at build time via Vite virtual module plugin - New reusable SettingRow/SettingSection components for consistent settings layout - New Toggle (switch) UI component replacing checkboxes in settings - Toast viewport now offsets when audio player is open - Sidebar stays active on settings sub-routes (fuzzy matching)
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughReplaces ServerTab with a tabbed SettingsLayout (General, Generation, GPU, Logs, Changelog, About), adds server log streaming and a capped log store, exposes CHANGELOG.md via a Vite virtual plugin and parser, updates routing and Vite configs, and adds multiple settings UI components and backend packaging tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Backend as Backend/Tauri
participant TauriLC as TauriLifecycle
participant App as App.tsx
participant LogStore as useLogStore
participant LogsPage as LogsPage
Backend->>TauriLC: emit "server-log" event {stream,line}
TauriLC->>App: invoke subscribed callback(entry)
App->>LogStore: addEntry(entry)
LogStore-->>LogsPage: updated entries (subscribe)
LogsPage->>LogsPage: render entries, manage auto-scroll
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
backend/voicebox-server.spec (1)
9-9: Consider de-duplicating the hidden import manifest.Line 9 keeps a large inline import manifest that now also overlaps with
backend/build_binary.py; keeping this in one shared source would reduce drift risk between build paths.♻️ Suggested direction
+# backend/pyinstaller_manifest.py +HIDDEN_IMPORTS = [ + "backend", + "backend.main", + ... +] + # backend/voicebox-server.spec -from ... import ... -hiddenimports = ['backend', 'backend.main', ...] +from backend.pyinstaller_manifest import HIDDEN_IMPORTS +hiddenimports = list(HIDDEN_IMPORTS) # backend/build_binary.py -args.extend(["--hidden-import", "backend", ...]) +for mod in HIDDEN_IMPORTS: + args.extend(["--hidden-import", mod])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/voicebox-server.spec` at line 9, The hiddenimports list in voicebox-server.spec duplicates the large import manifest defined in backend.build_binary.py; extract the shared manifest into a single constant (e.g., HIDDEN_IMPORTS or HIDDEN_IMPORT_MANIFEST) in backend.build_binary (or a new module like backend.build_shared) and have voicebox-server.spec import or load that constant instead of inlining the list, then remove the duplicated inline list from voicebox-server.spec and ensure any build scripts use the shared constant so both build paths reference the same source of truth.app/plugins/changelog.ts (1)
16-21: Consider adding error handling for missing changelog file.If
CHANGELOG.mdis missing or unreadable,readFileSyncwill throw with a genericENOENTerror, which may be confusing during build failures. A descriptive error message would improve developer experience.♻️ Proposed enhancement
+import { existsSync, readFileSync } from 'node:fs'; -import { readFileSync } from 'node:fs'; ... load(id) { if (id === resolvedId) { + if (!existsSync(changelogPath)) { + throw new Error(`Changelog not found at ${changelogPath}`); + } const raw = readFileSync(changelogPath, 'utf-8'); return `export default ${JSON.stringify(raw)};`; } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/plugins/changelog.ts` around lines 16 - 21, The load function currently calls readFileSync(changelogPath, 'utf-8') without handling failures; wrap the file read in a try/catch inside load (the branch that checks id === resolvedId) and on failure throw or log a new Error that includes changelogPath and the original error message so build errors show a descriptive "missing/unreadable CHANGELOG.md" style message rather than raw ENOENT; reference load, resolvedId, changelogPath and readFileSync when making the change.app/src/components/ServerTab/SettingRow.tsx (1)
49-54: EnsurehtmlForis paired with matchingidon controls.The
labelelement'shtmlForattribute only creates an accessible association when the target control has a matchingid. Thecursor-pointerclass also sets an expectation that clicking the label will activate something.Consider documenting this requirement or conditionally applying
cursor-pointeronly whenhtmlForis provided:♻️ Conditional cursor styling
<label htmlFor={htmlFor} - className="text-sm font-medium leading-none cursor-pointer select-none" + className={`text-sm font-medium leading-none select-none ${htmlFor ? 'cursor-pointer' : ''}`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerTab/SettingRow.tsx` around lines 49 - 54, The label in SettingRow.tsx uses htmlFor but the associated control may lack a matching id and the cursor-pointer class creates a false affordance; update the SettingRow component to ensure the control receives the same id when an htmlFor prop is provided (or require/prop-drill an id prop to the child control) and/or conditionally apply the "cursor-pointer" class only when htmlFor is truthy. Locate the label usage in SettingRow (the label element using htmlFor and {title}) and either (a) pass the htmlFor value through to the child control as its id or (b) change the className logic to include "cursor-pointer" only if htmlFor is present; also add a short prop comment or JSDoc on SettingRow explaining that htmlFor requires a matching id on the input.app/src/components/ui/toggle.tsx (1)
27-32: Consider adding visible focus styling for keyboard navigation.The toggle lacks explicit focus indicator styles. While browsers provide a default focus ring, it may not be visible against all backgrounds or may have been reset by a global CSS reset. Adding a focus ring improves keyboard accessibility.
♿ Proposed focus styling
className={cn( 'relative inline-flex h-5 w-9 shrink-0 items-center rounded-full transition-colors', + 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2', checked ? 'bg-accent' : 'bg-muted-foreground/25', disabled ? 'opacity-50 cursor-not-allowed' : 'cursor-pointer', className, )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ui/toggle.tsx` around lines 27 - 32, The Toggle component's root element (the className assembled via cn with checked and disabled states) lacks an explicit keyboard focus indicator; update the className list in the Toggle component (the cn call that currently composes 'relative inline-flex h-5 w-9 ...') to include focus-visible ring classes (e.g., focus-visible:ring, focus-visible:ring-offset and appropriate ring color/width) and remove any conflicting outline-none so keyboard users get a clear visible focus ring when the component receives focus; ensure the focus styles apply even when checked or disabled by placing them alongside the existing checked/disabled class entries.web/vite.config.ts (1)
3-8: Minor inconsistency in plugin ordering.The plugin order here is
react(), tailwindcss(), changelogPlugin()whileapp/vite.config.tsusestailwindcss(), react(), changelogPlugin(). This works but is inconsistent across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/vite.config.ts` around lines 3 - 8, The plugins array in defineConfig uses react(), tailwindcss(), changelogPlugin(...) but should match the other config's ordering; update the plugins list so tailwindcss() comes first, then react(), then changelogPlugin(path.resolve(__dirname, '..')), i.e., replace the current plugins order with tailwindcss(), react(), changelogPlugin(...) in the default export; ensure you keep the existing changelogPlugin call and path.resolve argument unchanged.app/src/components/ServerTab/GeneralPage.tsx (2)
157-166: Same state desync concern for network access toggle.The
setModecall should similarly be reverted if the underlying operation conceptually fails (though this one doesn't have a platform call that could fail).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerTab/GeneralPage.tsx` around lines 157 - 166, The network-access toggle currently calls setMode directly inside onCheckedChange which can desync UI if the underlying change fails; capture the previous mode (e.g., const prev = mode), set mode optimistically, then perform the operation that applies the change (or a placeholder async call), and if that operation throws or returns a failure revert by calling setMode(prev). Update the onCheckedChange handler (the function passed to onCheckedChange) to use this pattern and ensure toast is shown only after success (or show an error toast on revert) so setMode and toast stay consistent.
132-143: State may desync if platform call fails.
setKeepServerRunningOnClose(checked)updates the store immediately, thenplatform.lifecycle.setKeepServerRunning(checked)is called. If the platform call fails, the UI shows the new state but the platform has the old state.Consider reverting on failure:
onCheckedChange={(checked: boolean) => { setKeepServerRunningOnClose(checked); platform.lifecycle.setKeepServerRunning(checked).catch((error) => { console.error('Failed to sync setting to Rust:', error); + setKeepServerRunningOnClose(!checked); // Revert on failure + toast({ + title: 'Failed to update setting', + description: 'Could not sync setting to backend.', + variant: 'destructive', + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerTab/GeneralPage.tsx` around lines 132 - 143, The UI currently updates state optimistically by calling setKeepServerRunningOnClose(checked) before the platform call, which can desync if platform.lifecycle.setKeepServerRunning(checked) fails; change this to either perform the platform call first and only call setKeepServerRunningOnClose on success, or keep the optimistic update but revert it in the catch handler (call setKeepServerRunningOnClose(!checked)), and surface the failure via toast or console.error so users know the setting didn't persist; update the onCheckedChange handler accordingly to ensure state and platform remain consistent and reference setKeepServerRunningOnClose and platform.lifecycle.setKeepServerRunning for the fix.app/src/components/ServerTab/GpuPage.tsx (1)
165-167: Consider surfacing EventSource connection errors.The
onerrorhandler silently closes the connection without notifying the user. If the SSE stream fails due to network issues, the UI will show stale progress with no indication of failure.eventSource.onerror = () => { eventSource.close(); + setDownloadProgress(null); + refetchCudaStatus(); };This resets the UI and refreshes the actual status from the server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/ServerTab/GpuPage.tsx` around lines 165 - 167, In GpuPage.tsx update the eventSource.onerror handler (currently calling eventSource.close()) to surface the failure: set a component error state and/or show a user-visible notification, call eventSource.close(), and then trigger the existing status-refresh logic so the UI fetches the actual status from the server (i.e., replace the silent close in eventSource.onerror with code that sets an error flag/notification and invokes the component's status refresh function).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/ServerTab/ChangelogPage.tsx`:
- Around line 51-68: The list mapping in ChangelogPage.tsx uses the item content
as the React key (items.map((item) => ...)), which can collide for duplicate
list entries; change the mapping to use a stable unique key instead (for example
include the parent block index/elements.length and the item index or a generated
unique id) by updating the items.map callback signature to items.map((item, idx)
=> ...) and using a composite key such as `${elements.length}-${idx}` (or
another stable unique scheme) for the <li> elements.
- Around line 99-117: The table uses non-unique content values as React keys
(headers.map key={h}, rows.map key={row.join('|')}, and row.map key={cell})
which can collide; update these to use stable positional keys or composite keys
instead (e.g., use the header index in headers.map like `${headerIndex}-${h}`,
use row index for rows.map like `${rowIndex}-${row.join('|')}` or ideally
`${rowIndex}`, and use `${rowIndex}-${cellIndex}` for cells) by updating the key
props in the header, row, and cell renderers (refer to the headers.map,
rows.map, and row.map calls) so keys are unique and stable across renders.
In `@app/src/components/ServerTab/GpuPage.tsx`:
- Around line 234-258: Both handleSwitchToCpu and handleRestart share a racey
sequence around starting/stopping health polling and calling
platform.lifecycle.restartServer; extract a shared helper (e.g.,
restartServerWithPolling) and move the coordinated logic there: ensure you clear
any existing healthPollRef before initiating restart, call
platform.lifecycle.restartServer and await it, only startHealthPolling after
restart completes (or avoid starting it if you immediately clear it), and always
clear healthPollRef in a finally block; update handleSwitchToCpu and
handleRestart to call this helper and remove their duplicated
polling/restart/clearInterval code to fix the race and centralize behavior.
- Around line 210-232: The handleRestart sequence races because
startHealthPolling() is called before platform.lifecycle.restartServer()
completes; adjust handleRestart (and related healthPollRef logic) to begin
polling only after the restart has been initiated and the server has been
observed to go unhealthy (i.e., call platform.lifecycle.restartServer() first or
wait for the first poll to return unhealthy before treating it as a restart in
progress), then startHealthPolling() to detect when it comes back healthy and
update restartPhase accordingly; ensure you still clear healthPollRef in both
success and catch branches and keep restartPhase transitions (stopping → waiting
→ ready → idle) consistent in functions handleRestart, startHealthPolling, and
any health check callbacks so the phase cannot be set to 'ready' while the
server is still healthy.
In `@app/src/components/ServerTab/LogsPage.tsx`:
- Line 99: The current entries.map((entry, i) => <LogLine
key={`${entry.timestamp}-${i}`} entry={entry} />) uses the array index i which
causes key churn when the capped buffer trims old entries; instead generate and
persist a stable unique id for each log entry in the log store (e.g., add an id
field when creating entries in the store where entries are pushed/trimmed, using
a UUID or incrementing counter) and then change the rendering to use that stable
id as the key (replace `${entry.timestamp}-${i}` with entry.id). Update code
that constructs entries (store functions that append/trim logs, MAX_LOG_ENTRIES
logic) to ensure the id is assigned once and survives buffer shifts so LogLine
components remain stable.
In `@app/src/components/ui/toaster.tsx`:
- Line 28: The toast viewport offset is too small and can overlap the fixed
audio player: update the conditional class on ToastViewport (the JSX using
ToastViewport and isPlayerOpen) to use a larger bottom spacing—e.g., replace the
'sm:bottom-32' value with 'sm:bottom-40' (or another value >=10rem) so toasts
sit above the AudioPlayer’s py-3 + min-h-[80px] footprint; adjust only the class
string in the ToastViewport JSX to avoid overlap.
In `@tauri/src/platform/lifecycle.ts`:
- Around line 90-101: subscribeToServerLogs currently returns a disposer
synchronously while listen(...) resolves asynchronously, causing a race where
the returned disposer can be called before unlisten is assigned; modify
subscribeToServerLogs to introduce a local boolean disposed = false, have the
returned disposer set disposed = true and call unlisten() if it's already
assigned, and in the listen(...).then handler set unlisten = fn and if
(disposed) { unlisten(); unlisten = null; } to ensure cleanup executes even if
disposal happened before registration completes; also attach a .catch handler to
the listen Promise to avoid unhandled rejections and handle cleanup state
consistently.
---
Nitpick comments:
In `@app/plugins/changelog.ts`:
- Around line 16-21: The load function currently calls
readFileSync(changelogPath, 'utf-8') without handling failures; wrap the file
read in a try/catch inside load (the branch that checks id === resolvedId) and
on failure throw or log a new Error that includes changelogPath and the original
error message so build errors show a descriptive "missing/unreadable
CHANGELOG.md" style message rather than raw ENOENT; reference load, resolvedId,
changelogPath and readFileSync when making the change.
In `@app/src/components/ServerTab/GeneralPage.tsx`:
- Around line 157-166: The network-access toggle currently calls setMode
directly inside onCheckedChange which can desync UI if the underlying change
fails; capture the previous mode (e.g., const prev = mode), set mode
optimistically, then perform the operation that applies the change (or a
placeholder async call), and if that operation throws or returns a failure
revert by calling setMode(prev). Update the onCheckedChange handler (the
function passed to onCheckedChange) to use this pattern and ensure toast is
shown only after success (or show an error toast on revert) so setMode and toast
stay consistent.
- Around line 132-143: The UI currently updates state optimistically by calling
setKeepServerRunningOnClose(checked) before the platform call, which can desync
if platform.lifecycle.setKeepServerRunning(checked) fails; change this to either
perform the platform call first and only call setKeepServerRunningOnClose on
success, or keep the optimistic update but revert it in the catch handler (call
setKeepServerRunningOnClose(!checked)), and surface the failure via toast or
console.error so users know the setting didn't persist; update the
onCheckedChange handler accordingly to ensure state and platform remain
consistent and reference setKeepServerRunningOnClose and
platform.lifecycle.setKeepServerRunning for the fix.
In `@app/src/components/ServerTab/GpuPage.tsx`:
- Around line 165-167: In GpuPage.tsx update the eventSource.onerror handler
(currently calling eventSource.close()) to surface the failure: set a component
error state and/or show a user-visible notification, call eventSource.close(),
and then trigger the existing status-refresh logic so the UI fetches the actual
status from the server (i.e., replace the silent close in eventSource.onerror
with code that sets an error flag/notification and invokes the component's
status refresh function).
In `@app/src/components/ServerTab/SettingRow.tsx`:
- Around line 49-54: The label in SettingRow.tsx uses htmlFor but the associated
control may lack a matching id and the cursor-pointer class creates a false
affordance; update the SettingRow component to ensure the control receives the
same id when an htmlFor prop is provided (or require/prop-drill an id prop to
the child control) and/or conditionally apply the "cursor-pointer" class only
when htmlFor is truthy. Locate the label usage in SettingRow (the label element
using htmlFor and {title}) and either (a) pass the htmlFor value through to the
child control as its id or (b) change the className logic to include
"cursor-pointer" only if htmlFor is present; also add a short prop comment or
JSDoc on SettingRow explaining that htmlFor requires a matching id on the input.
In `@app/src/components/ui/toggle.tsx`:
- Around line 27-32: The Toggle component's root element (the className
assembled via cn with checked and disabled states) lacks an explicit keyboard
focus indicator; update the className list in the Toggle component (the cn call
that currently composes 'relative inline-flex h-5 w-9 ...') to include
focus-visible ring classes (e.g., focus-visible:ring, focus-visible:ring-offset
and appropriate ring color/width) and remove any conflicting outline-none so
keyboard users get a clear visible focus ring when the component receives focus;
ensure the focus styles apply even when checked or disabled by placing them
alongside the existing checked/disabled class entries.
In `@backend/voicebox-server.spec`:
- Line 9: The hiddenimports list in voicebox-server.spec duplicates the large
import manifest defined in backend.build_binary.py; extract the shared manifest
into a single constant (e.g., HIDDEN_IMPORTS or HIDDEN_IMPORT_MANIFEST) in
backend.build_binary (or a new module like backend.build_shared) and have
voicebox-server.spec import or load that constant instead of inlining the list,
then remove the duplicated inline list from voicebox-server.spec and ensure any
build scripts use the shared constant so both build paths reference the same
source of truth.
In `@web/vite.config.ts`:
- Around line 3-8: The plugins array in defineConfig uses react(),
tailwindcss(), changelogPlugin(...) but should match the other config's
ordering; update the plugins list so tailwindcss() comes first, then react(),
then changelogPlugin(path.resolve(__dirname, '..')), i.e., replace the current
plugins order with tailwindcss(), react(), changelogPlugin(...) in the default
export; ensure you keep the existing changelogPlugin call and path.resolve
argument unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dea2260f-6bec-4274-922f-7efbd7d95558
📒 Files selected for processing (32)
CHANGELOG.mdapp/plugins/changelog.tsapp/src/App.tsxapp/src/components/History/HistoryTable.tsxapp/src/components/ServerTab/AboutPage.tsxapp/src/components/ServerTab/ChangelogPage.tsxapp/src/components/ServerTab/GeneralPage.tsxapp/src/components/ServerTab/GenerationPage.tsxapp/src/components/ServerTab/GpuPage.tsxapp/src/components/ServerTab/LogsPage.tsxapp/src/components/ServerTab/ServerTab.tsxapp/src/components/ServerTab/SettingRow.tsxapp/src/components/Sidebar.tsxapp/src/components/ui/slider.tsxapp/src/components/ui/toaster.tsxapp/src/components/ui/toggle.tsxapp/src/global.d.tsapp/src/lib/utils/parseChangelog.tsapp/src/platform/types.tsapp/src/router.tsxapp/src/stores/logStore.tsapp/tsconfig.node.jsonapp/vite.config.tsbackend/build_binary.pybackend/routes/health.pybackend/voicebox-server.specdocs/notes/BACKEND_CODE_REVIEW.mdtauri/src-tauri/src/main.rstauri/src/platform/lifecycle.tstauri/vite.config.tsweb/src/platform/lifecycle.tsweb/vite.config.ts
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
1 participant
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.
Summary
SettingRow/SettingSectioncomponents for consistent layout across all settings pagesCHANGELOG.mdat build time via a Vite virtual module plugin/serverredirects to/settingsfor backwards compatSummary by CodeRabbit
New Features
UI Improvements
Chores