fix(desktop): single instance, model picker, and theme boot - #43457
fix(desktop): single instance, model picker, and theme boot#43457Nigmat-future wants to merge 2 commits into
Conversation
austinpickett
left a comment
There was a problem hiding this comment.
✅ Approved
Three focused desktop fixes bundled correctly:
-
Single-instance lock:
app.requestSingleInstanceLock()prevents duplicate processes;second-instancehandler focuses the existing window or creates a new one. Standard Electron pattern. -
Theme boot flash fix:
getWindowBackgroundColor()readsrendererTitleBarTheme?.background(already loaded from prior sessions) or falls back to a dark/light default matching the OS theme. Applied to bothcreateWindow()andcreateSessionWindow()so windows never repaint white on dark-mode startup. -
Title bar theme propagation:
applyTitleBarThemeToWindowis a proper helper that guards against destroyed windows before callingsetTitleBarOverlayandsetBackgroundColor. Thehermes:titlebar-themeIPC handler now correctly targets the sender window (not justmainWindow) and propagates to all openBrowserWindowinstances.
Well-scoped trio of fixes, each with good defensive guards.
Reviewed by Hermes Agent
austinpickett
left a comment
There was a problem hiding this comment.
Clean, focused set of fixes — all three concerns (single-instance, model picker stubs, theme flash on cold boot) are well-reasoned and the implementations are solid.
Single instance lock — correct placement before any window creation; the second-instance handler properly falls back to createWindow() when the original window was already destroyed. Guard call of focusWindow(mainWindow) is safe.
Theme boot script — the inline <script> in <head> runs synchronously before any render frame. The three-key localStorage schema (MODE_KEY, PROFILE_MODES_KEY, LAST_PROFILE_KEY) mirrors what ThemeProvider writes, so the boot paint genuinely matches the stored preference. All JSON parsing is guarded; silent catch means a corrupt store never breaks cold start.
Background color / title-bar theme propagation — getWindowBackgroundColor() consulting rendererTitleBarTheme?.background before falling back to nativeTheme.shouldUseDarkColors is the right order of precedence. Changing _event → event and dispatching the overlay update through BrowserWindow.fromWebContents(event.sender) correctly handles multi-window setups (previously only mainWindow received the overlay).
Model picker cache guard — returning prev (not undefined) when !prev?.providers?.length avoids persisting a provider/model-only stub. The explicit read-then-write (getQueryData → setQueryData) instead of the functional-updater form is a small but necessary change since the guard needs the value.
Model visibility empty-set fallback — folding stored.size === 0 into the !stored branch (both yield defaultVisibleKeys) corrects the prior behaviour where a user who had explicitly cleared all visibility choices saw zero models. The new test covers it.
One warning to address before merge:
The package.json diff drops electron/windows-child-process.test.cjs from test:desktop:platforms. It looks unintentional — the line was replaced when inserting single-instance.test.cjs without preserving the trailing entry. Two other open PRs (#43720, #43642) re-add it on top of the original line, so this will surface as a merge conflict. Please restore the dropped test file in the command before this merges.
Overall: approvable once the package.json test-script regression is fixed — the functional changes are correct and well-tested.
Reviewed by Hermes Agent
Code Review SummaryPR #43457 -- fix(desktop): single instance, model picker, and theme boot Warnings
Suggestions
Looks Good
Reviewed by Hermes Agent |
c9a4967 to
64bff1a
Compare
|
Rebased onto latest Changes since last review:
Verification:
Please re-review when you have a moment — the previous approval was dismissed after the rebase. Thanks! |
Prevent a second Electron process from opening a duplicate window, fix empty model picker when visibility cache is an empty set, and apply theme/titlebar before first paint to avoid flash on launch.
Co-authored-by: Cursor <cursoragent@cursor.com>
64bff1a to
dae7131
Compare
|
Thanks again for the earlier review, @austinpickett 👍 I rebased onto the latest How the conflicts resolved:
Behavior is unchanged — just reconciled with main's newer theme-boot work. Happy to adjust if any of the merge calls look off. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Desktop fixes. The model-options cache concern is still relevant, but the branch needs selective salvage rather than a direct port.
Problems
- The empty-set visibility fallback in
apps/desktop/src/store/model-visibility.tsconflicts with current main's intentional contract: an empty non-null stored set means “everything hidden” (apps/desktop/src/store/model-visibility.ts:130-137), covered atapps/desktop/src/store/model-visibility.test.ts:183-193and documented by461fcc096. apps/desktop/electron/main.cjsno longer exists after the TypeScript migration (39d09453f). Currentmain.tsalready implements a stronger single-instance/deep-link flow atapps/desktop/electron/main.ts:9039-9058and themed pre-paint behavior atapps/desktop/electron/main.ts:7043-7046,7250-7254.- The new cache test reimplements the logic in a local helper rather than testing
useModelControls, so it would not catch a regression in the production writer (apps/desktop/src/app/session/hooks/use-model-controls.ts:27-32).
Suggested changes
- Port only the model-options cache guard to the current TypeScript hook and cover that production path. Keep the current null-versus-empty visibility semantics.
Automated hermes-sweeper review.
| providers: readonly ModelOptionProvider[] | ||
| ): Set<string> { | ||
| if (!stored) { | ||
| if (!stored || stored.size === 0) { |
There was a problem hiding this comment.
Please do not conflate an explicit empty stored set with an uncustomized selection. Current main intentionally treats empty non-null storage as “everything hidden” (apps/desktop/src/store/model-visibility.ts:130-137), with regression coverage at apps/desktop/src/store/model-visibility.test.ts:183-193.
| import type { ModelOptionsResponse } from '@/types/hermes' | ||
|
|
||
| function patchModelOptionsCache( | ||
| queryClient: QueryClient, |
There was a problem hiding this comment.
This helper duplicates rather than invokes the production cache writer, so the test can remain green if useModelControls regresses. Extract/import a production pure helper or render the hook and assert its actual cache update.
|
|
||
| test('main process enforces a single app instance on Windows launches', () => { | ||
| const mainSource = fs.readFileSync(path.join(__dirname, 'main.cjs'), 'utf8') | ||
|
|
There was a problem hiding this comment.
This is a source-pattern test, not a behavior contract. The current Desktop test surface is TypeScript, and the Electron entry point has moved to electron/main.ts; replace this with a behavioral test if the relevant lifecycle logic is salvaged.
Summary
Test plan
npm testinapps/desktop(model visibility, use-model-controls, single-instance).Made with Cursor