feat: persist and restore UI/audio/model settings#174
Merged
Conversation
Contributor
Reviewer's GuideThis PR introduces a centralized settings persistence layer and wires it into the app to persist and restore UI layout, audio device selection, model selection, and various tuning parameters, while also hardening device refresh logic and adding tests for both settings and device handling behavior. Sequence diagram for app startup, settings hydration, and device refreshsequenceDiagram
autonumber
participant Browser as BrowserWindow
participant App as AppComponent
participant Settings as SettingsStorage
participant Store as AppStore
participant Media as NavigatorMediaDevices
Browser->>App: mount()
activate App
App->>Browser: addEventListener(resize)
App->>Settings: loadSettingsFromStorage()
activate Settings
Settings-->>App: PersistedSettings
deactivate Settings
App->>Store: setSelectedModelId(persistedModelId?)
App->>Store: setSelectedDeviceId(persistedDeviceId?)
App->>Store: setEnergyThreshold(...)
App->>Store: setSileroThreshold(...)
App->>Store: setV4InferenceIntervalMs(...)
App->>Store: setV4SilenceFlushSec(...)
App->>Store: setStreamingWindow(...)
App->>Store: setShowDebugPanel(persistedVisible?)
App->>App: setDebugPanelHeight(clampDebugPanelHeight(persistedHeight))
App->>App: setActiveTranscriptTab(persistedTab?)
App->>App: setMergedSplitRatio(clampMergedSplitRatio(persistedRatio))
App->>App: setWidgetPos(clampWidgetPosition(persistedOrDefaultPosition))
App->>Store: refreshDevices()
activate Store
Store->>Media: enumerateDevices()
activate Media
Media-->>Store: devices
deactivate Media
Store->>Store: filter audioinput devices
alt no audio devices
Store->>Store: setAvailableDevices([])
Store->>Store: setSelectedDeviceId('')
else some devices
Store->>Store: setAvailableDevices(mics)
Store->>Store: compute hasCurrentSelection
alt has current selection
Store->>Store: keep selectedDeviceId
else no valid selection
Store->>Store: setSelectedDeviceId(firstMicId)
end
end
deactivate Store
Store-->>App: refreshDevices() resolved
App->>App: setSettingsHydrated(true)
deactivate App
Sequence diagram for settings snapshot persistence with debouncesequenceDiagram
autonumber
participant App as AppComponent
participant Store as AppStore
participant Settings as SettingsStorage
participant LS as LocalStorage
rect rgb(235, 245, 255)
note over App,Store: User changes settings (UI, audio, model, tuning)
App->>Store: update signals (e.g. setEnergyThreshold, setSelectedModelId)
App->>App: update local signals (widgetPos, debugPanelHeight, mergedSplitRatio)
end
App->>App: reactiveEffect() triggered
alt settingsHydrated is false
App-->>App: return (do not persist)
else settingsHydrated is true
App->>Store: read general, model, audio settings
App->>App: read ui signals (widgetPos, debugPanelHeight, mergedSplitRatio, activeTab)
App->>App: build snapshot : PersistedSettings
alt existing debounce timeout
App->>App: clearTimeout(settingsSaveTimeout)
end
App->>App: setTimeout(SETTINGS_SAVE_DEBOUNCE_MS)
App-->>App: store timeout id in settingsSaveTimeout
App->>App: timeout fires
App->>Settings: saveSettingsToStorage(snapshot)
activate Settings
Settings->>LS: setItem(APP_SETTINGS_STORAGE_KEY, JSON.stringify(snapshot))
LS-->>Settings: success or error
Settings-->>App: return
deactivate Settings
end
Class diagram for centralized settings persistence and consumersclassDiagram
class SettingsStorage {
<<utility>>
+APP_SETTINGS_STORAGE_KEY : string
+MIN_DEBUG_PANEL_HEIGHT : number
+MAX_DEBUG_PANEL_HEIGHT : number
+DEFAULT_DEBUG_PANEL_HEIGHT : number
+MIN_MERGED_SPLIT_RATIO : number
+MAX_MERGED_SPLIT_RATIO : number
+DEFAULT_MERGED_SPLIT_RATIO : number
+clampDebugPanelHeight(height number) number
+clampMergedSplitRatio(ratio number) number
+loadSettingsFromStorage() PersistedSettings
+saveSettingsToStorage(settings PersistedSettings) void
}
class PersistedSettings {
+general : PersistedGeneral
+model : PersistedModel
+audio : PersistedAudio
+ui : PersistedUi
}
class PersistedGeneral {
+energyThreshold : number
+sileroThreshold : number
+v4InferenceIntervalMs : number
+v4SilenceFlushSec : number
+streamingWindow : number
}
class PersistedModel {
+selectedModelId : string
}
class PersistedAudio {
+selectedDeviceId : string
+selectedDeviceLabel : string
}
class PersistedUi {
+widgetPosition : WidgetPosition
+debugPanel : PersistedDebugPanel
+transcript : PersistedTranscript
}
class WidgetPosition {
+x : number
+y : number
}
class PersistedDebugPanel {
+visible : boolean
+height : number
}
class PersistedTranscript {
+activeTab : StoredTranscriptTab
+mergedSplitRatio : number
}
class StoredTranscriptTab {
<<type alias>>
+live
+merged
}
class AppComponent {
+mergedSplitRatio : Signal~number~
+debugPanelHeight : Signal~number~
+settingsHydrated : Signal~boolean~
+widgetPos : Signal~WidgetPosition~
+activeTranscriptTab : Signal~StoredTranscriptTab~
+onMount() void
+createEffect_saveSettingsSnapshot() void
}
class AppStore {
+energyThreshold() number
+sileroThreshold() number
+v4InferenceIntervalMs() number
+v4SilenceFlushSec() number
+streamingWindow() number
+selectedModelId() string
+selectedDeviceId() string
+availableDevices() MediaDeviceInfo[]
+showDebugPanel() boolean
+setEnergyThreshold(value number) void
+setSileroThreshold(value number) void
+setV4InferenceIntervalMs(value number) void
+setV4SilenceFlushSec(value number) void
+setStreamingWindow(value number) void
+setSelectedModelId(id string) void
+setSelectedDeviceId(id string) void
+setShowDebugPanel(visible boolean) void
+refreshDevices() Promise~void~
}
class TranscriptionDisplayProps {
+activeTab : StoredTranscriptTab
+onTabChange(tab StoredTranscriptTab) void
+mergedSplitRatio : number
+onMergedSplitRatioChange(ratio number) void
}
class TranscriptionDisplayComponent {
+internalMergedSplitRatio : Signal~number~
+mergedSplitRatio() number
+setMergedSplitRatio(ratio number) void
+startSplitResize(event MouseEvent) void
}
class DebugPanelProps {
+audioEngine : AudioEngine
+melClient : MelWorkerClient
+height : number
+onHeightChange(height number) void
}
class DebugPanelComponent {
+internalHeight : Signal~number~
+panelHeight() number
+setPanelHeight(height number) void
+handleMouseDown(e MouseEvent) void
+handleMouseMove(e MouseEvent) void
+handleMouseUp() void
}
SettingsStorage ..> PersistedSettings
PersistedSettings o--> PersistedGeneral
PersistedSettings o--> PersistedModel
PersistedSettings o--> PersistedAudio
PersistedSettings o--> PersistedUi
PersistedUi o--> WidgetPosition
PersistedUi o--> PersistedDebugPanel
PersistedUi o--> PersistedTranscript
PersistedTranscript ..> StoredTranscriptTab
AppComponent ..> SettingsStorage : uses
AppComponent ..> AppStore : uses
AppComponent o--> PersistedSettings : builds snapshots
TranscriptionDisplayComponent ..> TranscriptionDisplayProps : reads
TranscriptionDisplayComponent ..> SettingsStorage : uses clampMergedSplitRatio
DebugPanelComponent ..> DebugPanelProps : reads
DebugPanelComponent ..> SettingsStorage : uses clampDebugPanelHeight
AppStore ..> navigator_mediaDevices : uses
class navigator_mediaDevices {
+enumerateDevices() Promise~MediaDeviceInfo[]~
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds a storage-backed PersistedSettings system and integrates it into app startup and lifecycle to hydrate and persist UI and device state; updates component props for controlled UI values and extends device refresh logic and tests. Changes
Sequence DiagramsequenceDiagram
participant App as App.tsx
participant Storage as settingsStorage
participant LocalStorage as Browser Storage
participant AppStore as appStore
participant Components as UI Components
App->>Storage: loadSettingsFromStorage()
Storage->>LocalStorage: read APP_SETTINGS_STORAGE_KEY
Storage-->>App: PersistedSettings (sanitized & clamped, with legacy fallbacks)
App->>AppStore: refreshDevices(options?)
AppStore-->>App: devices + validated selection
App->>Components: set props (widgetPosition, activeTab, mergedSplitRatio, debugPanel.height, model/audio settings)
Note over App,Components: hydration complete
rect rgba(100,150,200,0.5)
Components->>App: onHeightChange / onMergedSplitRatioChange / other UI events
App->>App: update signals, debounce snapshot (150ms)
App->>Storage: saveSettingsToStorage(snapshot)
Storage->>LocalStorage: write APP_SETTINGS_STORAGE_KEY
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/App.tsx:309` </location>
<code_context>
}
});
+ createEffect(() => {
+ if (!settingsHydrated()) return;
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new settings persistence and widget layout logic from `App` into dedicated helpers/hooks to keep `App` focused on wiring and view concerns.
You’ve added valuable persistence, but a lot of it is now cross‑cutting inside `App`. You can keep all behavior while shrinking `App`’s responsibilities by pushing snapshot building + hydration into a small abstraction.
### 1. Extract settings persistence into a helper/hook
Move the `createEffect` snapshot + debounced save and the `onMount` hydration logic into a dedicated module, e.g. `useSettingsPersistence.ts` (or `settingsPersistence.ts`). `App` then just wires signals into it.
**New helper (sketch):**
```ts
// utils/useSettingsPersistence.ts
import {
clampDebugPanelHeight,
clampMergedSplitRatio,
DEFAULT_DEBUG_PANEL_HEIGHT,
DEFAULT_MERGED_SPLIT_RATIO,
loadSettingsFromStorage,
saveSettingsToStorage,
} from './settingsStorage';
const SETTINGS_SAVE_DEBOUNCE_MS = 150;
export function useSettingsPersistence(options: {
appStore: typeof appStore;
widgetPos: Accessor<{ x: number; y: number } | null>;
setWidgetPos: (pos: { x: number; y: number } | null) => void;
activeTranscriptTab: Accessor<TranscriptViewTab>;
setActiveTranscriptTab: (tab: TranscriptViewTab) => void;
debugPanelHeight: Accessor<number>;
setDebugPanelHeight: (h: number) => void;
mergedSplitRatio: Accessor<number>;
setMergedSplitRatio: (r: number) => void;
}) {
const {
appStore,
widgetPos,
setWidgetPos,
activeTranscriptTab,
setActiveTranscriptTab,
debugPanelHeight,
setDebugPanelHeight,
mergedSplitRatio,
setMergedSplitRatio,
} = options;
const [settingsHydrated, setSettingsHydrated] = createSignal(false);
let saveTimeout: number | undefined;
// Hydration
onMount(() => {
const persisted = loadSettingsFromStorage();
const g = persisted.general;
const ui = persisted.ui;
if (persisted.model?.selectedModelId && MODELS.some(m => m.id === persisted.model!.selectedModelId)) {
appStore.setSelectedModelId(persisted.model.selectedModelId);
}
if (persisted.audio?.selectedDeviceId) {
appStore.setSelectedDeviceId(persisted.audio.selectedDeviceId);
}
if (g?.energyThreshold !== undefined) appStore.setEnergyThreshold(g.energyThreshold);
if (g?.sileroThreshold !== undefined) appStore.setSileroThreshold(g.sileroThreshold);
if (g?.v4InferenceIntervalMs !== undefined) appStore.setV4InferenceIntervalMs(g.v4InferenceIntervalMs);
if (g?.v4SilenceFlushSec !== undefined) appStore.setV4SilenceFlushSec(g.v4SilenceFlushSec);
if (g?.streamingWindow !== undefined) appStore.setStreamingWindow(g.streamingWindow);
if (ui?.debugPanel?.visible !== undefined) appStore.setShowDebugPanel(ui.debugPanel.visible);
if (ui?.debugPanel?.height !== undefined) {
setDebugPanelHeight(clampDebugPanelHeight(ui.debugPanel.height));
}
if (ui?.transcript?.activeTab) {
setActiveTranscriptTab(ui.transcript.activeTab);
}
if (ui?.transcript?.mergedSplitRatio !== undefined) {
setMergedSplitRatio(clampMergedSplitRatio(ui.transcript.mergedSplitRatio));
}
if (ui?.widgetPosition) {
setWidgetPos(clampWidgetPosition(ui.widgetPosition));
} else {
setWidgetPos(getDefaultWidgetPosition());
}
setSettingsHydrated(true);
});
// Saving
createEffect(() => {
if (!settingsHydrated()) return;
const selectedDeviceId = appStore.selectedDeviceId();
const selectedDevice = appStore.availableDevices().find(d => d.deviceId === selectedDeviceId);
const snapshot = {
general: {
energyThreshold: appStore.energyThreshold(),
sileroThreshold: appStore.sileroThreshold(),
v4InferenceIntervalMs: appStore.v4InferenceIntervalMs(),
v4SilenceFlushSec: appStore.v4SilenceFlushSec(),
streamingWindow: appStore.streamingWindow(),
},
model: {
selectedModelId: appStore.selectedModelId(),
},
audio: {
selectedDeviceId,
selectedDeviceLabel: selectedDevice?.label,
},
ui: {
widgetPosition: widgetPos() ?? undefined,
debugPanel: {
visible: appStore.showDebugPanel(),
height: debugPanelHeight(),
},
transcript: {
activeTab: activeTranscriptTab(),
mergedSplitRatio: mergedSplitRatio(),
},
},
};
if (saveTimeout) clearTimeout(saveTimeout);
saveTimeout = window.setTimeout(
() => saveSettingsToStorage(snapshot),
SETTINGS_SAVE_DEBOUNCE_MS,
);
});
onCleanup(() => {
if (saveTimeout) clearTimeout(saveTimeout);
});
return { settingsHydrated };
}
```
**Then in `App` you can trim to:**
```ts
const App: Component = () => {
// ...existing signals...
const [mergedSplitRatio, setMergedSplitRatio] = createSignal(DEFAULT_MERGED_SPLIT_RATIO);
const [debugPanelHeight, setDebugPanelHeight] = createSignal(DEFAULT_DEBUG_PANEL_HEIGHT);
const [widgetPos, setWidgetPos] = createSignal<{ x: number; y: number } | null>(null);
const { settingsHydrated } = useSettingsPersistence({
appStore,
widgetPos,
setWidgetPos,
activeTranscriptTab,
setActiveTranscriptTab,
debugPanelHeight,
setDebugPanelHeight,
mergedSplitRatio,
setMergedSplitRatio,
});
onMount(() => {
const onResize = () => setWindowHeight(window.innerHeight);
window.addEventListener('resize', onResize);
workerClient = new TranscriptionWorkerClient();
// ...worker wiring...
void appStore.refreshDevices();
setWorkerReady(true);
return () => window.removeEventListener('resize', onResize);
});
// onCleanup now no longer needs to clear settingsSaveTimeout
};
```
This keeps all current behavior but pulls out the big `createEffect` and the hydration `onMount` logic into a single, named abstraction, making `App` focus on wiring and view concerns.
### 2. Move widget layout helpers out of `App`
Right now `getDefaultWidgetPosition` and `clampWidgetPosition` sit in `App`. They belong with other settings/UI layout logic (e.g. `settingsStorage.ts` or a new `uiLayout.ts`).
For example:
```ts
// utils/uiLayout.ts
export const WIDGET_MAX_W = 672;
export const WIDGET_MIN_H = 80;
export const getDefaultWidgetPosition = (): { x: number; y: number } => {
const width = typeof window !== 'undefined' ? window.innerWidth : 800;
const height = typeof window !== 'undefined' ? window.innerHeight : 600;
return {
x: Math.max(0, (width - WIDGET_MAX_W) / 2),
y: Math.max(0, height - 140),
};
};
export const clampWidgetPosition = (position: { x: number; y: number }) => {
if (typeof window === 'undefined') return position;
return {
x: Math.max(0, Math.min(window.innerWidth - WIDGET_MAX_W, position.x)),
y: Math.max(0, Math.min(window.innerHeight - WIDGET_MIN_H, position.y)),
};
};
```
Then import these into both `App` and the persistence helper as needed:
```ts
import { getDefaultWidgetPosition, clampWidgetPosition } from './utils/uiLayout';
```
This reduces the “layout math” noise inside `App` and keeps all settings/layout logic co‑located and easier to test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/App.tsx
Outdated
| } | ||
| }); | ||
|
|
||
| createEffect(() => { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the new settings persistence and widget layout logic from App into dedicated helpers/hooks to keep App focused on wiring and view concerns.
You’ve added valuable persistence, but a lot of it is now cross‑cutting inside App. You can keep all behavior while shrinking App’s responsibilities by pushing snapshot building + hydration into a small abstraction.
1. Extract settings persistence into a helper/hook
Move the createEffect snapshot + debounced save and the onMount hydration logic into a dedicated module, e.g. useSettingsPersistence.ts (or settingsPersistence.ts). App then just wires signals into it.
New helper (sketch):
// utils/useSettingsPersistence.ts
import {
clampDebugPanelHeight,
clampMergedSplitRatio,
DEFAULT_DEBUG_PANEL_HEIGHT,
DEFAULT_MERGED_SPLIT_RATIO,
loadSettingsFromStorage,
saveSettingsToStorage,
} from './settingsStorage';
const SETTINGS_SAVE_DEBOUNCE_MS = 150;
export function useSettingsPersistence(options: {
appStore: typeof appStore;
widgetPos: Accessor<{ x: number; y: number } | null>;
setWidgetPos: (pos: { x: number; y: number } | null) => void;
activeTranscriptTab: Accessor<TranscriptViewTab>;
setActiveTranscriptTab: (tab: TranscriptViewTab) => void;
debugPanelHeight: Accessor<number>;
setDebugPanelHeight: (h: number) => void;
mergedSplitRatio: Accessor<number>;
setMergedSplitRatio: (r: number) => void;
}) {
const {
appStore,
widgetPos,
setWidgetPos,
activeTranscriptTab,
setActiveTranscriptTab,
debugPanelHeight,
setDebugPanelHeight,
mergedSplitRatio,
setMergedSplitRatio,
} = options;
const [settingsHydrated, setSettingsHydrated] = createSignal(false);
let saveTimeout: number | undefined;
// Hydration
onMount(() => {
const persisted = loadSettingsFromStorage();
const g = persisted.general;
const ui = persisted.ui;
if (persisted.model?.selectedModelId && MODELS.some(m => m.id === persisted.model!.selectedModelId)) {
appStore.setSelectedModelId(persisted.model.selectedModelId);
}
if (persisted.audio?.selectedDeviceId) {
appStore.setSelectedDeviceId(persisted.audio.selectedDeviceId);
}
if (g?.energyThreshold !== undefined) appStore.setEnergyThreshold(g.energyThreshold);
if (g?.sileroThreshold !== undefined) appStore.setSileroThreshold(g.sileroThreshold);
if (g?.v4InferenceIntervalMs !== undefined) appStore.setV4InferenceIntervalMs(g.v4InferenceIntervalMs);
if (g?.v4SilenceFlushSec !== undefined) appStore.setV4SilenceFlushSec(g.v4SilenceFlushSec);
if (g?.streamingWindow !== undefined) appStore.setStreamingWindow(g.streamingWindow);
if (ui?.debugPanel?.visible !== undefined) appStore.setShowDebugPanel(ui.debugPanel.visible);
if (ui?.debugPanel?.height !== undefined) {
setDebugPanelHeight(clampDebugPanelHeight(ui.debugPanel.height));
}
if (ui?.transcript?.activeTab) {
setActiveTranscriptTab(ui.transcript.activeTab);
}
if (ui?.transcript?.mergedSplitRatio !== undefined) {
setMergedSplitRatio(clampMergedSplitRatio(ui.transcript.mergedSplitRatio));
}
if (ui?.widgetPosition) {
setWidgetPos(clampWidgetPosition(ui.widgetPosition));
} else {
setWidgetPos(getDefaultWidgetPosition());
}
setSettingsHydrated(true);
});
// Saving
createEffect(() => {
if (!settingsHydrated()) return;
const selectedDeviceId = appStore.selectedDeviceId();
const selectedDevice = appStore.availableDevices().find(d => d.deviceId === selectedDeviceId);
const snapshot = {
general: {
energyThreshold: appStore.energyThreshold(),
sileroThreshold: appStore.sileroThreshold(),
v4InferenceIntervalMs: appStore.v4InferenceIntervalMs(),
v4SilenceFlushSec: appStore.v4SilenceFlushSec(),
streamingWindow: appStore.streamingWindow(),
},
model: {
selectedModelId: appStore.selectedModelId(),
},
audio: {
selectedDeviceId,
selectedDeviceLabel: selectedDevice?.label,
},
ui: {
widgetPosition: widgetPos() ?? undefined,
debugPanel: {
visible: appStore.showDebugPanel(),
height: debugPanelHeight(),
},
transcript: {
activeTab: activeTranscriptTab(),
mergedSplitRatio: mergedSplitRatio(),
},
},
};
if (saveTimeout) clearTimeout(saveTimeout);
saveTimeout = window.setTimeout(
() => saveSettingsToStorage(snapshot),
SETTINGS_SAVE_DEBOUNCE_MS,
);
});
onCleanup(() => {
if (saveTimeout) clearTimeout(saveTimeout);
});
return { settingsHydrated };
}Then in App you can trim to:
const App: Component = () => {
// ...existing signals...
const [mergedSplitRatio, setMergedSplitRatio] = createSignal(DEFAULT_MERGED_SPLIT_RATIO);
const [debugPanelHeight, setDebugPanelHeight] = createSignal(DEFAULT_DEBUG_PANEL_HEIGHT);
const [widgetPos, setWidgetPos] = createSignal<{ x: number; y: number } | null>(null);
const { settingsHydrated } = useSettingsPersistence({
appStore,
widgetPos,
setWidgetPos,
activeTranscriptTab,
setActiveTranscriptTab,
debugPanelHeight,
setDebugPanelHeight,
mergedSplitRatio,
setMergedSplitRatio,
});
onMount(() => {
const onResize = () => setWindowHeight(window.innerHeight);
window.addEventListener('resize', onResize);
workerClient = new TranscriptionWorkerClient();
// ...worker wiring...
void appStore.refreshDevices();
setWorkerReady(true);
return () => window.removeEventListener('resize', onResize);
});
// onCleanup now no longer needs to clear settingsSaveTimeout
};This keeps all current behavior but pulls out the big createEffect and the hydration onMount logic into a single, named abstraction, making App focus on wiring and view concerns.
2. Move widget layout helpers out of App
Right now getDefaultWidgetPosition and clampWidgetPosition sit in App. They belong with other settings/UI layout logic (e.g. settingsStorage.ts or a new uiLayout.ts).
For example:
// utils/uiLayout.ts
export const WIDGET_MAX_W = 672;
export const WIDGET_MIN_H = 80;
export const getDefaultWidgetPosition = (): { x: number; y: number } => {
const width = typeof window !== 'undefined' ? window.innerWidth : 800;
const height = typeof window !== 'undefined' ? window.innerHeight : 600;
return {
x: Math.max(0, (width - WIDGET_MAX_W) / 2),
y: Math.max(0, height - 140),
};
};
export const clampWidgetPosition = (position: { x: number; y: number }) => {
if (typeof window === 'undefined') return position;
return {
x: Math.max(0, Math.min(window.innerWidth - WIDGET_MAX_W, position.x)),
y: Math.max(0, Math.min(window.innerHeight - WIDGET_MIN_H, position.y)),
};
};Then import these into both App and the persistence helper as needed:
import { getDefaultWidgetPosition, clampWidgetPosition } from './utils/uiLayout';This reduces the “layout math” noise inside App and keeps all settings/layout logic co‑located and easier to test.
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
src/stores/appStore.ts
Outdated
| try { | ||
| if (typeof navigator === 'undefined' || !navigator.mediaDevices?.enumerateDevices) { | ||
| setAvailableDevices([]); | ||
| setSelectedDeviceId(''); |
There was a problem hiding this comment.
WARNING: setSelectedDeviceId('') in the SSR/no-mediaDevices guard clobbers any device ID that was just restored from persisted settings.
In App.tsx, appStore.setSelectedDeviceId(persistedDeviceId) is called synchronously in onMount before refreshDevices(). If navigator.mediaDevices is absent (e.g. in a test environment or a future SSR path), this early-return branch immediately overwrites the restored ID with ''. The finally callback then sets settingsHydrated(true), causing the save effect to persist selectedDeviceId: '' back to storage, silently erasing the user's device preference.
Consider not touching selectedDeviceId in this guard — the device list is simply unavailable, but the previously-selected ID is still valid for when devices become available later:
Suggested change
| setSelectedDeviceId(''); | |
| setAvailableDevices([]); |
| settings.ui.widgetPosition = legacyWidgetPosition; | ||
| } | ||
|
|
||
| if (legacySplitRatio !== undefined && !settings.ui?.transcript?.mergedSplitRatio) { |
There was a problem hiding this comment.
WARNING: Falsy check !settings.ui?.transcript?.mergedSplitRatio will incorrectly skip migration if mergedSplitRatio is 0. While the current minimum is 0.3 (so 0 is impossible after clamping), this pattern is fragile: if the minimum is ever lowered to 0, the legacy value will be silently ignored even when the new key has no stored value.
Prefer an explicit === undefined check for consistency with the rest of the codebase:
Suggested change
| if (legacySplitRatio !== undefined && !settings.ui?.transcript?.mergedSplitRatio) { | |
| if (legacySplitRatio !== undefined && settings.ui?.transcript?.mergedSplitRatio === undefined) { |
| const legacyWidgetPosition = readLegacyWidgetPosition(); | ||
| const legacySplitRatio = readLegacySplitRatio(); | ||
|
|
||
| if (legacyWidgetPosition && !settings.ui?.widgetPosition) { |
There was a problem hiding this comment.
SUGGESTION: For consistency, the widget-position legacy migration check also uses a falsy guard (!settings.ui?.widgetPosition). An object { x: 0, y: 0 } is truthy so this is safe today, but aligning both checks to === undefined would make the intent explicit and guard against future edge cases:
Suggested change
| if (legacyWidgetPosition && !settings.ui?.widgetPosition) { | |
| if (legacyWidgetPosition && settings.ui?.widgetPosition === undefined) { |
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)No issues found in unchanged code. Files Reviewed (5 files)
Key Findings
Settings persistence architecture: The Legacy migration: The falsy guards in No fragile zones touched: This PR does not modify transcription window calculations, cursor/mature-cursor logic, or sliding-window merge/finalization logic. Risk is low. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/settingsStorage.ts (1)
200-227: Consider cleaning up legacy keys after successful migration.Legacy keys (
boncukjs-control-widget-pos,keet-merged-split-ratio) are read and merged into the unified settings but are never removed fromlocalStorage. After a migration cycle (i.e., when the unified key is saved), these orphaned keys persist indefinitely. A one-time cleanup after migration would prevent stale legacy data from lingering.♻️ Optional: remove legacy keys after merging
export const loadSettingsFromStorage = (): PersistedSettings => { if (typeof localStorage === 'undefined') return {}; let parsed: unknown = null; try { const raw = localStorage.getItem(APP_SETTINGS_STORAGE_KEY); if (raw) parsed = JSON.parse(raw); } catch { parsed = null; } const settings = sanitizeSettings(parsed); const legacyWidgetPosition = readLegacyWidgetPosition(); const legacySplitRatio = readLegacySplitRatio(); if (legacyWidgetPosition && !settings.ui?.widgetPosition) { settings.ui = settings.ui ?? {}; settings.ui.widgetPosition = legacyWidgetPosition; + try { localStorage.removeItem('boncukjs-control-widget-pos'); } catch { /* ignore */ } } if (legacySplitRatio !== undefined && !settings.ui?.transcript?.mergedSplitRatio) { settings.ui = settings.ui ?? {}; settings.ui.transcript = settings.ui.transcript ?? {}; settings.ui.transcript.mergedSplitRatio = legacySplitRatio; + try { localStorage.removeItem('keet-merged-split-ratio'); } catch { /* ignore */ } } return settings; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/settingsStorage.ts` around lines 200 - 227, The loadSettingsFromStorage function merges legacy values from readLegacyWidgetPosition and readLegacySplitRatio into the sanitized settings but never removes the old localStorage keys; after detecting and applying a legacy value (i.e., when settings.ui.widgetPosition or settings.ui.transcript.mergedSplitRatio gets set from legacy), call localStorage.removeItem for the legacy keys (boncukjs-control-widget-pos and keet-merged-split-ratio) so they are cleaned up once migration is applied; ensure this cleanup happens only after a successful merge and before returning settings, and keep use of APP_SETTINGS_STORAGE_KEY and sanitizeSettings unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/settingsStorage.ts`:
- Around line 200-227: The loadSettingsFromStorage function merges legacy values
from readLegacyWidgetPosition and readLegacySplitRatio into the sanitized
settings but never removes the old localStorage keys; after detecting and
applying a legacy value (i.e., when settings.ui.widgetPosition or
settings.ui.transcript.mergedSplitRatio gets set from legacy), call
localStorage.removeItem for the legacy keys (boncukjs-control-widget-pos and
keet-merged-split-ratio) so they are cleaned up once migration is applied;
ensure this cleanup happens only after a successful merge and before returning
settings, and keep use of APP_SETTINGS_STORAGE_KEY and sanitizeSettings
unchanged.
Delay settings persistence until audio devices are enumerated to avoid overwriting cached selections with startup defaults. Restore preferred input using label-tolerant matching with ID fallback, and keep selection when device enumeration is temporarily empty. Add tests for empty-device enumeration and label-based restoration behavior.
src/stores/appStore.ts
Outdated
| const label = device.label.trim().toLowerCase(); | ||
| if (!label) return false; | ||
| if (label === preferredDeviceLabel) return true; | ||
| return label.includes(preferredDeviceLabel) || preferredDeviceLabel.includes(label); |
There was a problem hiding this comment.
WARNING: Bidirectional substring match is overly permissive
The second condition preferredDeviceLabel.includes(label) can produce false-positive matches. For example, if a device has a short label like "USB" and the persisted preferred label is "USB Microphone", the condition preferredDeviceLabel.includes(label) is true, so that device would be selected even though it may not be the right one. The intent is to find a device whose label contains the preferred label (forward match), not the reverse.
Consider dropping the reverse direction:
Suggested change
| return label.includes(preferredDeviceLabel) || preferredDeviceLabel.includes(label); | |
| return label.includes(preferredDeviceLabel); |
| mergedSplitValue === undefined ? undefined : clampMergedSplitRatio(mergedSplitValue); | ||
| if (activeTab || mergedSplitRatio !== undefined) { | ||
| nextUi.transcript = {}; | ||
| if (activeTab) nextUi.transcript.activeTab = activeTab; |
There was a problem hiding this comment.
SUGGESTION: Use !== undefined for consistency with the mergedSplitRatio guard on the same line
activeTab is StoredTranscriptTab | undefined. Both 'live' and 'merged' are truthy so the falsy check works in practice, but activeTab !== undefined is more explicit and consistent with the mergedSplitRatio !== undefined pattern used on line 164.
Suggested change
| if (activeTab) nextUi.transcript.activeTab = activeTab; | |
| if (activeTab !== undefined) nextUi.transcript.activeTab = activeTab; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/stores/appStore.test.ts (1)
104-114: Extract thenavigator.mediaDevicesmock setup into a shared helper.The same
Object.defineProperty(navigator, 'mediaDevices', …)/Object.defineProperty(navigator.mediaDevices, 'enumerateDevices', …)block is copy-pasted in every test within "Device Management". A small helper (or a nestedbeforeEach) would eliminate ~60 lines of duplication and make new test cases easier to add.♻️ Example helper
function mockEnumerateDevices(devices: Partial<MediaDeviceInfo>[]) { const mock = vi.fn().mockResolvedValue(devices); if (!navigator.mediaDevices) { Object.defineProperty(navigator, 'mediaDevices', { value: {}, writable: true, }); } Object.defineProperty(navigator.mediaDevices, 'enumerateDevices', { value: mock, writable: true, }); return mock; }Then each test becomes:
it('should keep selected device when it still exists', async () => { mockEnumerateDevices([ { kind: 'audioinput', deviceId: 'mic1', label: 'Microphone 1' }, { kind: 'audioinput', deviceId: 'mic2', label: 'Microphone 2' }, ]); store.setSelectedDeviceId('mic2'); await store.refreshDevices(); expect(store.selectedDeviceId()).toBe('mic2'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/appStore.test.ts` around lines 104 - 114, Extract the repeated navigator.mediaDevices mock setup into a shared helper function (e.g., mockEnumerateDevices) and replace the inline Object.defineProperty blocks in the "Device Management" tests with calls to that helper; the helper should create/define navigator.mediaDevices when missing and set navigator.mediaDevices.enumerateDevices to a vi.fn() that resolves the provided devices, return the mock so tests can assert calls, and update tests that reference enumerateDevicesMock (and use store.refreshDevices / store.setSelectedDeviceId / store.selectedDeviceId) to call the new helper instead.src/stores/appStore.ts (1)
249-261: Bidirectionalincludesfor label matching may be overly permissive.The condition
label.includes(preferredDeviceLabel) || preferredDeviceLabel.includes(label)means a short preferred label like"mic"will match any device whose label contains"mic", and conversely a short device label will match any long preferred label. This could produce false positives when multiple devices share common substrings.Consider scoring candidates (e.g. prefer exact, then longest substring overlap) or at minimum requiring a minimum substring length, so a renamed device doesn't accidentally bind to an unrelated one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/appStore.ts` around lines 249 - 261, The current bidirectional includes check in the preferred-device-selection block (using preferredDeviceLabel, mics, preferredByLabel, and setSelectedDeviceId) is too permissive; replace it with a stricter matching routine: first try exact match (label === preferredDeviceLabel), then collect substring-match candidates and score them by longest common substring or longest contiguous match length (require a minimum match length, e.g. >= 3 characters) to avoid tiny matches like "mic"; if multiple candidates tie, prefer the one with the longest label or the first enumerated; only setSelectedDeviceId when a candidate passes the minimum threshold. Ensure you update the matching logic where device.label is normalized (trim().toLowerCase()) and fall back to previous behavior only if no candidate meets the threshold.src/App.tsx (1)
219-225: Negative clamping edge case inclampWidgetPosition.When
window.innerWidth < WIDGET_MAX_W(e.g. on a narrow mobile viewport),window.innerWidth - WIDGET_MAX_Wis negative, makingMath.min(negative, position.x)always negative, thenMath.max(0, negative)clamps to 0. The widget is pinned to the left edge — functional but worth documenting or explicitly handling (e.g. centering the widget instead).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 219 - 225, clampWidgetPosition currently pins the widget to the left/top when the viewport is smaller than the widget; change it to compute maxX = window.innerWidth - WIDGET_MAX_W and maxY = window.innerHeight - WIDGET_MIN_H and then: if maxX >= 0 clamp x between 0 and maxX, otherwise set x = Math.floor(maxX / 2) to center horizontally (similarly for y using maxY), keeping the function name clampWidgetPosition and constants WIDGET_MAX_W / WIDGET_MIN_H to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 442-456: The cleanup for the event listeners registered inside the
onMount block is currently returned from onMount (ignored); move the removal
calls for window.removeEventListener('resize', onResize),
window.removeEventListener('beforeunload', onBeforeUnload),
window.removeEventListener('pagehide', onPageHide) and
document.removeEventListener('visibilitychange', onVisibilityChange) into the
existing onCleanup callback so those handlers are properly removed; locate the
onMount registration and the onCleanup function and transfer the four
removeEventListener calls into onCleanup (keeping the same handler references:
onResize, onBeforeUnload, onPageHide, onVisibilityChange).
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 219-225: clampWidgetPosition currently pins the widget to the
left/top when the viewport is smaller than the widget; change it to compute maxX
= window.innerWidth - WIDGET_MAX_W and maxY = window.innerHeight - WIDGET_MIN_H
and then: if maxX >= 0 clamp x between 0 and maxX, otherwise set x =
Math.floor(maxX / 2) to center horizontally (similarly for y using maxY),
keeping the function name clampWidgetPosition and constants WIDGET_MAX_W /
WIDGET_MIN_H to locate and update the logic.
In `@src/stores/appStore.test.ts`:
- Around line 104-114: Extract the repeated navigator.mediaDevices mock setup
into a shared helper function (e.g., mockEnumerateDevices) and replace the
inline Object.defineProperty blocks in the "Device Management" tests with calls
to that helper; the helper should create/define navigator.mediaDevices when
missing and set navigator.mediaDevices.enumerateDevices to a vi.fn() that
resolves the provided devices, return the mock so tests can assert calls, and
update tests that reference enumerateDevicesMock (and use store.refreshDevices /
store.setSelectedDeviceId / store.selectedDeviceId) to call the new helper
instead.
In `@src/stores/appStore.ts`:
- Around line 249-261: The current bidirectional includes check in the
preferred-device-selection block (using preferredDeviceLabel, mics,
preferredByLabel, and setSelectedDeviceId) is too permissive; replace it with a
stricter matching routine: first try exact match (label ===
preferredDeviceLabel), then collect substring-match candidates and score them by
longest common substring or longest contiguous match length (require a minimum
match length, e.g. >= 3 characters) to avoid tiny matches like "mic"; if
multiple candidates tie, prefer the one with the longest label or the first
enumerated; only setSelectedDeviceId when a candidate passes the minimum
threshold. Ensure you update the matching logic where device.label is normalized
(trim().toLowerCase()) and fall back to previous behavior only if no candidate
meets the threshold.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/stores/appStore.ts (1)
249-261: Label containment match is unidirectional — consider bidirectional check.
label.includes(preferredDeviceLabel)only matches when the stored label is a substring of the current enumerated label. The reverse scenario — where the stored label is more specific than what the device currently reports (e.g., stored"Blue Yeti X Stereo Microphone"but device now reports"Blue Yeti X") — will silently fall through to the first-device fallback.♻️ Proposed bidirectional containment check
const preferredByLabel = mics.find((device) => { const label = device.label.trim().toLowerCase(); if (!label) return false; if (label === preferredDeviceLabel) return true; - return label.includes(preferredDeviceLabel); + return label.includes(preferredDeviceLabel) || preferredDeviceLabel.includes(label); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/appStore.ts` around lines 249 - 261, The current selection logic in the preferredDeviceLabel block only checks if device.label contains preferredDeviceLabel, which misses cases where the stored preferredDeviceLabel is longer/more specific than the enumerated device.label; update the matching in the mics.find callback (used to produce preferredByLabel) to perform a bidirectional containment check (i.e., check both device.label.includes(preferredDeviceLabel) OR preferredDeviceLabel.includes(device.label)) while still normalizing with .trim().toLowerCase(), then continue to call setSelectedDeviceId(preferredByLabel.deviceId) when a match is found.src/App.tsx (2)
275-275:widgetPosis always non-null — the null branch at line 1135 is dead code.
initialWidgetPosis always a{ x, y }object (fromclampWidgetPositionorgetDefaultWidgetPosition), andsetWidgetPos(null)is never called anywhere. The signal's| nulltype and the conditional class expression at line 1135 are therefore unreachable. Consider removing thenullunion if null-position is no longer a supported state, or document why it's kept for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` at line 275, The widgetPos signal is typed as possibly null but initialWidgetPos (from clampWidgetPosition/getDefaultWidgetPosition) is always an object and setWidgetPos(null) is never used, so remove the unnecessary null union and the dead conditional class logic: change the signal declaration to createSignal<{ x: number; y: number }>(initialWidgetPos) (remove | null), update any call sites or JSX that check for widgetPos being null (the conditional class expression currently guarding at the place using widgetPos) to use widgetPos() directly, and if you want to keep a note about future nullable support, add a short comment by the widgetPos/setWidgetPos declaration instead of keeping the runtime null branch.
398-408: Non-idiomatic debounce cleanup — preferonCleanupinside the effect.The
createEffectmanually clearssettingsSaveTimeouton every re-run and relies on the component-levelonCleanup(viaflushSettingsSave) to cancel any pending timeout on disposal. This works today becausesettingsReadyForPersistis never reset tofalseafter becomingtrue, but if the effect returns early due to that guard changing, the pending timeout is silently left in flight.The idiomatic SolidJS pattern is to register cleanup inside the effect so cancellation is automatic regardless of how the effect re-runs or disposes:
♻️ Proposed idiomatic debounce
createEffect(() => { if (!settingsReadyForPersist()) return; const snapshot = getSettingsSnapshot(); - if (settingsSaveTimeout) { - clearTimeout(settingsSaveTimeout); - } - settingsSaveTimeout = window.setTimeout(() => { - saveSettingsToStorage(snapshot); - settingsSaveTimeout = undefined; - }, SETTINGS_SAVE_DEBOUNCE_MS); + const id = window.setTimeout(() => { + saveSettingsToStorage(snapshot); + }, SETTINGS_SAVE_DEBOUNCE_MS); + onCleanup(() => clearTimeout(id)); });This removes the need for the
settingsSaveTimeoutmodule variable entirely (it would still be needed byflushSettingsSave, so keep it there if you want the flush behaviour, or refactorflushSettingsSaveto take the snapshot eagerly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 398 - 408, The effect should register its own cleanup with onCleanup instead of manually clearing the module-level settingsSaveTimeout at the top; inside createEffect call getSettingsSnapshot and start a timeout (store it in a local variable, and if you still need flushSettingsSave keep assigning that id to the module-level settingsSaveTimeout), then call onCleanup(() => { clearTimeout(localTimeout); settingsSaveTimeout = undefined; }) so the timer is cancelled on every re-run or dispose (and remove the manual clearTimeout(settingsSaveTimeout) at the start of the effect); keep using settingsReadyForPersist, saveSettingsToStorage and getSettingsSnapshot as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/App.tsx`:
- Line 275: The widgetPos signal is typed as possibly null but initialWidgetPos
(from clampWidgetPosition/getDefaultWidgetPosition) is always an object and
setWidgetPos(null) is never used, so remove the unnecessary null union and the
dead conditional class logic: change the signal declaration to createSignal<{ x:
number; y: number }>(initialWidgetPos) (remove | null), update any call sites or
JSX that check for widgetPos being null (the conditional class expression
currently guarding at the place using widgetPos) to use widgetPos() directly,
and if you want to keep a note about future nullable support, add a short
comment by the widgetPos/setWidgetPos declaration instead of keeping the runtime
null branch.
- Around line 398-408: The effect should register its own cleanup with onCleanup
instead of manually clearing the module-level settingsSaveTimeout at the top;
inside createEffect call getSettingsSnapshot and start a timeout (store it in a
local variable, and if you still need flushSettingsSave keep assigning that id
to the module-level settingsSaveTimeout), then call onCleanup(() => {
clearTimeout(localTimeout); settingsSaveTimeout = undefined; }) so the timer is
cancelled on every re-run or dispose (and remove the manual
clearTimeout(settingsSaveTimeout) at the start of the effect); keep using
settingsReadyForPersist, saveSettingsToStorage and getSettingsSnapshot as
before.
In `@src/stores/appStore.ts`:
- Around line 249-261: The current selection logic in the preferredDeviceLabel
block only checks if device.label contains preferredDeviceLabel, which misses
cases where the stored preferredDeviceLabel is longer/more specific than the
enumerated device.label; update the matching in the mics.find callback (used to
produce preferredByLabel) to perform a bidirectional containment check (i.e.,
check both device.label.includes(preferredDeviceLabel) OR
preferredDeviceLabel.includes(device.label)) while still normalizing with
.trim().toLowerCase(), then continue to call
setSelectedDeviceId(preferredByLabel.deviceId) when a match is found.
github-actions bot
pushed a commit
that referenced
this pull request
Feb 21, 2026
feat: persist and restore UI/audio/model settings 23d330f
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
2 participants
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 by Sourcery
Persist application settings for UI layout, audio device selection, and model/configuration options, and restore them on load using a unified settings storage utility.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests