fix(desktop): make addFileViewerPane respect file open mode setting#1565
Conversation
Centralize the file open mode (tab vs split) logic in addFileViewerPane so CMD+P and all other callers automatically respect the user's setting without having to pass openInNewTab explicitly.
📝 WalkthroughWalkthroughThis PR introduces a module-level cache for the file open mode preference and refactors component-level hook usage to use a centralized getter function. The cache is initialized when the workspace page loads, and the tabs store now uses the cached value as a fallback when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/hooks/useFileOpenMode/useFileOpenMode.ts (2)
5-10: Stale cache window at startup — design trade-off to document
cachedFileOpenModeinitializes toDEFAULT_FILE_OPEN_MODEand only updates onceuseFileOpenMode()renders and the tRPC query resolves. AnyaddFileViewerPanecall fired in that window uses the default mode, ignoring the user's setting.Since this goes through IPC (Electron main-process local-db read), the gap is small but non-zero. A user with "New tab" configured who opens a file immediately on cold start (e.g., via a notification or restored session) would momentarily see split-pane behavior.
The cache-warming in
WorkspacePagemitigates this for the typical flow. Consider adding a brief doc comment togetFileOpenModeacknowledging the startup race so future maintainers don't overlook it.📝 Suggested doc addition
-/** Non-React getter, kept in sync by useFileOpenMode(). */ +/** + * Non-React getter for the current file-open mode, kept in sync by useFileOpenMode(). + * + * Returns DEFAULT_FILE_OPEN_MODE until useFileOpenMode() has been called and + * its query has resolved (cache warm-up). Callers should be aware of this + * startup window. + */ export function getFileOpenMode(): FileOpenMode { return cachedFileOpenMode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useFileOpenMode/useFileOpenMode.ts` around lines 5 - 10, Add a short doc comment above cachedFileOpenMode/getFileOpenMode explaining that cachedFileOpenMode initializes to DEFAULT_FILE_OPEN_MODE and may be stale until useFileOpenMode() runs and the tRPC IPC read resolves (causing a small startup race where callers like addFileViewerPane may see the default mode); note that WorkspacePage cache-warming mitigates the common path and call out this trade-off for future maintainers so they don’t inadvertently rely on the getter being fully warm at cold start. Reference cachedFileOpenMode, DEFAULT_FILE_OPEN_MODE, getFileOpenMode(), useFileOpenMode(), and WorkspacePage in the comment.
14-16: Render-phase mutation of module-level state — optionaluseEffectrefactor
cachedFileOpenMode = modeis a side effect inside the render body, which violates React's render-purity expectation. In React 19 Concurrent Mode, renders can be discarded or re-invoked (Strict Mode double-render) before commit.That said, this particular mutation is idempotent (same query state → same
mode), so it won't cause observable issues in practice. Be aware of the real trade-off: moving touseEffectmakes the code more React-correct but delays the cache update until after the commit phase, meaninggetFileOpenMode()can return a stale value for the window between render and effect execution.♻️ React-idiomatic alternative (with timing caveat)
-export function useFileOpenMode(): FileOpenMode { - const { data } = electronTrpc.settings.getFileOpenMode.useQuery(); - const mode = data ?? DEFAULT_FILE_OPEN_MODE; - cachedFileOpenMode = mode; - return mode; -} +export function useFileOpenMode(): FileOpenMode { + const { data } = electronTrpc.settings.getFileOpenMode.useQuery(); + const mode = data ?? DEFAULT_FILE_OPEN_MODE; + useEffect(() => { + cachedFileOpenMode = mode; + }, [mode]); + return mode; +}Note: the synchronous assignment in the current code means
getFileOpenMode()is accurate immediately after the render that produced the newmode, whereasuseEffectwould leave a brief window after render where the cache lags. If synchronous accuracy is required (e.g., the store callsgetFileOpenMode()in the same tick as a file open action), the current approach is actually preferable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/hooks/useFileOpenMode/useFileOpenMode.ts` around lines 14 - 16, The render is mutating module-level cachedFileOpenMode inside useFileOpenMode (the line `cachedFileOpenMode = mode`), which should be moved out of render; update useFileOpenMode to compute const mode = data ?? DEFAULT_FILE_OPEN_MODE and then perform the assignment inside a useEffect that runs when mode changes (e.g., useEffect(() => { cachedFileOpenMode = mode }, [mode])); keep the same default fallback (DEFAULT_FILE_OPEN_MODE) and ensure any callers using getFileOpenMode are aware of the possible micro-timing window where the cache lags after render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/renderer/hooks/useFileOpenMode/useFileOpenMode.ts`:
- Around line 5-10: Add a short doc comment above
cachedFileOpenMode/getFileOpenMode explaining that cachedFileOpenMode
initializes to DEFAULT_FILE_OPEN_MODE and may be stale until useFileOpenMode()
runs and the tRPC IPC read resolves (causing a small startup race where callers
like addFileViewerPane may see the default mode); note that WorkspacePage
cache-warming mitigates the common path and call out this trade-off for future
maintainers so they don’t inadvertently rely on the getter being fully warm at
cold start. Reference cachedFileOpenMode, DEFAULT_FILE_OPEN_MODE,
getFileOpenMode(), useFileOpenMode(), and WorkspacePage in the comment.
- Around line 14-16: The render is mutating module-level cachedFileOpenMode
inside useFileOpenMode (the line `cachedFileOpenMode = mode`), which should be
moved out of render; update useFileOpenMode to compute const mode = data ??
DEFAULT_FILE_OPEN_MODE and then perform the assignment inside a useEffect that
runs when mode changes (e.g., useEffect(() => { cachedFileOpenMode = mode },
[mode])); keep the same default fallback (DEFAULT_FILE_OPEN_MODE) and ensure any
callers using getFileOpenMode are aware of the possible micro-timing window
where the cache lags after render.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
addFileViewerPaneso it reads user config by default whenopenInNewTabis not explicitly passedChanges
useFileOpenMode.ts— AddedgetFileOpenMode()imperative getter backed by a module-level cache, so the Zustand store can read the setting synchronouslytabs/store.ts—addFileViewerPanenow defaultsopenInNewTabfrom user config viagetFileOpenMode()when not explicitly setpage.tsx(workspace) — CallsuseFileOpenMode()to keep the cache warm while a workspace is mounteduseFileOpenMode()+openInNewTabpassing fromFilesView.tsx,useFileLinkClick.ts,RightSidebar/index.tsxTest Plan
openInNewTabstill overrides the defaultSummary by CodeRabbit
Refactor