feat(desktop): allow opening additional windows from tray#1953
feat(desktop): allow opening additional windows from tray#1953Kitenite wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a main-process window-manager with IPC and workspace window APIs, threads route Changes
Sequence DiagramsequenceDiagram
actor User
participant Tray as Tray/Menu
participant DB as LocalDB
participant Main as MainProcess
participant WinMgr as WindowManager
participant BW as BrowserWindow
participant Renderer as Renderer
User->>Tray: Click "New Window"
Tray->>DB: read lastActiveWorkspaceId
DB-->>Tray: return workspaceId
Tray->>Main: request openWorkspaceWindow(workspaceId)
Main->>WinMgr: openWorkspaceWindow(input)
WinMgr->>WinMgr: normalize path & query, compute hash
WinMgr->>BW: create BrowserWindow (apply size/theme/preload)
WinMgr->>BW: loadFile with computed hash
BW->>Renderer: initialize renderer for route
Renderer->>Main: register IPC window handler (attach)
BW-->>User: show window
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/window-manager.ts">
<violation number="1" location="apps/desktop/src/main/lib/window-manager.ts:100">
P2: Use `closed` instead of `close` for IPC handler detachment. The `close` event can be cancelled (e.g., by `beforeunload`), which would leave the window visible but with IPC detached. `closed` guarantees the window is actually gone.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| ); | ||
|
|
||
| window.on("close", () => { |
There was a problem hiding this comment.
P2: Use closed instead of close for IPC handler detachment. The close event can be cancelled (e.g., by beforeunload), which would leave the window visible but with IPC detached. closed guarantees the window is actually gone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/window-manager.ts, line 100:
<comment>Use `closed` instead of `close` for IPC handler detachment. The `close` event can be cancelled (e.g., by `beforeunload`), which would leave the window visible but with IPC detached. `closed` guarantees the window is actually gone.</comment>
<file context>
@@ -0,0 +1,124 @@
+ },
+ );
+
+ window.on("close", () => {
+ ipcWindowHandler?.detachWindow(window);
+ });
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/window-loader.ts (1)
35-36: Minor: Outdated comment.The comment "TanStack Router uses hash-based routing, so we always start at
#/" is now outdated since the hash can be any path with query parameters, not just "#/".📝 Suggested comment update
} else { // Production: load from file with hash routing - // TanStack Router uses hash-based routing, so we always start at `#/` + // TanStack Router uses hash-based routing with the computed path and query console.log("[window-loader] Loading file:", props.htmlFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/window-loader.ts` around lines 35 - 36, Update the outdated comment in window-loader.ts that currently reads "TanStack Router uses hash-based routing, so we always start at `#/`" to reflect that the URL hash can contain any path and query parameters (not only "#/"); edit the comment directly in the production load block (the lines near the "Production: load from file with hash routing" comment) to state that TanStack Router uses hash-based routing and the app should preserve and load the full hash portion (path and query) rather than assuming a fixed "#/" start.
🤖 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/lib/window-loader.ts`:
- Around line 35-36: Update the outdated comment in window-loader.ts that
currently reads "TanStack Router uses hash-based routing, so we always start at
`#/`" to reflect that the URL hash can contain any path and query parameters (not
only "#/"); edit the comment directly in the production load block (the lines
near the "Production: load from file with hash routing" comment) to state that
TanStack Router uses hash-based routing and the app should preserve and load the
full hash portion (path and query) rather than assuming a fixed "#/" start.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/lib/electron-app/factories/windows/create.tsapps/desktop/src/lib/trpc/routers/window.tsapps/desktop/src/lib/window-loader.tsapps/desktop/src/main/lib/tray/index.tsapps/desktop/src/main/lib/window-manager.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/workspace/$workspaceId/page.tsxapps/desktop/src/shared/types/electron.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/window-manager.ts`:
- Around line 103-105: Replace the use of the "close" event with the "closed"
event when detaching IPC to avoid detaching when a close is canceled: change the
listener on window (the call using window.on("close", ...)) to use "closed"
instead and call ipcWindowHandler?.detachWindow(window) inside that "closed"
handler so detach only runs after the BrowserWindow has actually closed; keep
the optional chaining and existing ipcWindowHandler.detachWindow symbol intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/main/lib/menu.tsapps/desktop/src/main/lib/tray/index.tsapps/desktop/src/main/lib/window-manager.ts
| window.on("close", () => { | ||
| ipcWindowHandler?.detachWindow(window); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Electron BrowserWindow close vs closed events difference preventDefault cancel
💡 Result:
In Electron BrowserWindow, close and closed happen at different times:
-
'close'event: fired when the window is about to close (before the page’s DOMbeforeunload/unload). This is the event you can cancel by callingevent.preventDefault().
Source: ElectronBrowserWindowdocs note thatevent.preventDefault()cancels the close. [1] -
'closed'event: fired after the window is already closed. At this point you should cleanup (remove references) and you cannot cancel anything because the window is gone.
Source: Electron docs say afterclosedyou should remove the reference and avoid using it. [1] (Also commonly bites people who trypreventDefault()inclosed—it’s too late. [2])
Example pattern (cancel close / hide-to-tray style):
win.on('close', (e) => {
e.preventDefault(); // cancels closing
win.hide(); // or show a confirmation, etc.
});
win.on('closed', () => {
win = null; // cleanup only
});Related nuance: win.destroy() will not emit close, but it will emit closed, so close handlers won’t run in that case. [1]
Sources: [1] Electron BrowserWindow API docs. [2] Stack Overflow: “Only hide the window when closing it [Electron]”.
🏁 Script executed:
# Check the full context of the window-manager.ts file around lines 103-105
cat -n apps/desktop/src/main/lib/window-manager.ts | head -120 | tail -40Repository: superset-sh/superset
Length of output: 1358
🏁 Script executed:
# Search for any preventDefault calls on close events in the desktop app
rg "preventDefault|on\(\"close\"" apps/desktop/src --type ts -B 2 -A 2Repository: superset-sh/superset
Length of output: 50377
Use closed event instead of close for IPC cleanup.
The close event fires when the window is about to close and can be canceled with event.preventDefault(). If the close is canceled, the window remains open but IPC will already be detached, breaking the window-IPC lifecycle synchronization. Use the closed event instead, which fires only after the window has already closed.
Suggested fix
- window.on("close", () => {
+ window.on("closed", () => {
ipcWindowHandler?.detachWindow(window);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| window.on("close", () => { | |
| ipcWindowHandler?.detachWindow(window); | |
| }); | |
| window.on("closed", () => { | |
| ipcWindowHandler?.detachWindow(window); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/window-manager.ts` around lines 103 - 105, Replace
the use of the "close" event with the "closed" event when detaching IPC to avoid
detaching when a close is canceled: change the listener on window (the call
using window.on("close", ...)) to use "closed" instead and call
ipcWindowHandler?.detachWindow(window) inside that "closed" handler so detach
only runs after the BrowserWindow has actually closed; keep the optional
chaining and existing ipcWindowHandler.detachWindow symbol intact.
There was a problem hiding this comment.
2 issues found across 19 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/lib/window-scoped-storage.ts">
<violation number="1" location="apps/desktop/src/renderer/lib/window-scoped-storage.ts:23">
P2: Empty `catch {}` silently swallows sessionStorage errors. Log a warning with context so failures are observable in production.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/stores/markdown-preferences/store.ts">
<violation number="1" location="apps/desktop/src/renderer/stores/markdown-preferences/store.ts:23">
P1: Window-scoped storage key breaks persistence of markdown preferences across app restarts. `getWindowScopedStorageKey` derives a UUID from `sessionStorage`, which resets every session — so the `persist` middleware's localStorage key changes on each launch and the previously saved style preference is never found. Unlike tabs or sidebar state, a markdown style preference is a global user setting that should persist and be shared across windows. Consider keeping the static key `"markdown-preferences"` for this store.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }), | ||
| { | ||
| name: "markdown-preferences", | ||
| name: getWindowScopedStorageKey("markdown-preferences"), |
There was a problem hiding this comment.
P1: Window-scoped storage key breaks persistence of markdown preferences across app restarts. getWindowScopedStorageKey derives a UUID from sessionStorage, which resets every session — so the persist middleware's localStorage key changes on each launch and the previously saved style preference is never found. Unlike tabs or sidebar state, a markdown style preference is a global user setting that should persist and be shared across windows. Consider keeping the static key "markdown-preferences" for this store.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/stores/markdown-preferences/store.ts, line 23:
<comment>Window-scoped storage key breaks persistence of markdown preferences across app restarts. `getWindowScopedStorageKey` derives a UUID from `sessionStorage`, which resets every session — so the `persist` middleware's localStorage key changes on each launch and the previously saved style preference is never found. Unlike tabs or sidebar state, a markdown style preference is a global user setting that should persist and be shared across windows. Consider keeping the static key `"markdown-preferences"` for this store.</comment>
<file context>
@@ -19,7 +20,7 @@ export const useMarkdownPreferencesStore = create<MarkdownPreferencesState>()(
}),
{
- name: "markdown-preferences",
+ name: getWindowScopedStorageKey("markdown-preferences"),
},
),
</file context>
| name: getWindowScopedStorageKey("markdown-preferences"), | |
| name: "markdown-preferences", |
| const created = createScopeId(); | ||
| window.sessionStorage.setItem(WINDOW_SCOPE_ID_KEY, created); | ||
| return created; | ||
| } catch { |
There was a problem hiding this comment.
P2: Empty catch {} silently swallows sessionStorage errors. Log a warning with context so failures are observable in production.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/lib/window-scoped-storage.ts, line 23:
<comment>Empty `catch {}` silently swallows sessionStorage errors. Log a warning with context so failures are observable in production.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,31 @@
+ const created = createScopeId();
+ window.sessionStorage.setItem(WINDOW_SCOPE_ID_KEY, created);
+ return created;
+ } catch {
+ return null;
+ }
</file context>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
1777-1777: Consider one-time fallback from legacytabs-storageto avoid upgrade reset.Using a new scoped key is right for isolation, but this likely drops existing persisted tabs after upgrade. A one-time read/migrate path from the old key would preserve continuity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/tabs/store.ts` at line 1777, Add a one-time migration that checks for the legacy key "tabs-storage" and, if the new getWindowScopedStorageKey("tabs-storage") has no data, reads the legacy value and writes it to the new scoped key (then remove or mark the legacy key migrated). Implement this near the store initialization where the name is set (reference getWindowScopedStorageKey and the literal "tabs-storage") so the migration runs once on startup, avoids clobbering existing scoped data, and preserves users' persisted tabs after upgrades.apps/desktop/src/main/lib/notifications/server.ts (1)
8-8: Use tsconfig alias for the new tabs-state import.Line 8 introduces a relative import; please use the project alias for consistency with repo conventions.
Suggested change
-import { getMergedTabsState } from "../app-state/tabs-state"; +import { getMergedTabsState } from "main/lib/app-state/tabs-state";As per coding guidelines
apps/desktop/**/*.{ts,tsx}: Use alias as defined in tsconfig.json when possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/notifications/server.ts` at line 8, Replace the relative import of getMergedTabsState with the project tsconfig alias for the app-state module (i.e., swap import { getMergedTabsState } from "../app-state/tabs-state" to use the repo's configured alias for the app-state/tabs-state module, matching other imports in the codebase) so the getMergedTabsState import uses the tsconfig path alias rather than a relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/app-state/tabs-state.ts`:
- Around line 17-21: The conditional currently dereferences
appState.data.tabsState.tabs before confirming tabsState exists; change the
check so tabsState is validated first and only then inspect tabs length (e.g.,
ensure appState.data.tabsState is truthy before using
appState.data.tabsState.tabs.length or use optional chaining like
appState.data.tabsState?.tabs?.length) and only push appState.data.tabsState
into states when appState.data.tabsState is defined; locate the logic around the
states array and appState.data.tabsState in this file and reorder/adjust the
guard accordingly.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/notifications/server.ts`:
- Line 8: Replace the relative import of getMergedTabsState with the project
tsconfig alias for the app-state module (i.e., swap import { getMergedTabsState
} from "../app-state/tabs-state" to use the repo's configured alias for the
app-state/tabs-state module, matching other imports in the codebase) so the
getMergedTabsState import uses the tsconfig path alias rather than a relative
path.
In `@apps/desktop/src/renderer/stores/tabs/store.ts`:
- Line 1777: Add a one-time migration that checks for the legacy key
"tabs-storage" and, if the new getWindowScopedStorageKey("tabs-storage") has no
data, reads the legacy value and writes it to the new scoped key (then remove or
mark the legacy key migrated). Implement this near the store initialization
where the name is set (reference getWindowScopedStorageKey and the literal
"tabs-storage") so the migration runs once on startup, avoids clobbering
existing scoped data, and preserves users' persisted tabs after upgrades.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/desktop/src/lib/trpc/index.tsapps/desktop/src/lib/trpc/routers/ui-state/index.tsapps/desktop/src/main/lib/app-state/index.tsapps/desktop/src/main/lib/app-state/schemas.tsapps/desktop/src/main/lib/app-state/tabs-state.tsapps/desktop/src/main/lib/notifications/server.tsapps/desktop/src/main/lib/terminal/daemon/daemon-manager.tsapps/desktop/src/main/lib/terminal/dev-reset.tsapps/desktop/src/main/windows/main.tsapps/desktop/src/renderer/lib/persistent-hash-history/persistent-hash-history.tsapps/desktop/src/renderer/lib/window-scoped-storage.tsapps/desktop/src/renderer/stores/changes/store.tsapps/desktop/src/renderer/stores/file-explorer.tsapps/desktop/src/renderer/stores/markdown-preferences/store.tsapps/desktop/src/renderer/stores/ports/store.tsapps/desktop/src/renderer/stores/search-dialog-state.tsapps/desktop/src/renderer/stores/sidebar-state.tsapps/desktop/src/renderer/stores/tabs/store.tsapps/desktop/src/renderer/stores/workspace-sidebar-state.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/app-state/tabs-state.ts`:
- Around line 12-19: toWindowTabsState is returning references to the original
maps which leads to merge-time mutation; change it to return new shallow copies
instead of aliasing (e.g., replace activeTabIds: state?.activeTabIds ?? {} with
activeTabIds: { ...state?.activeTabIds } ?? {} and do the same for
focusedPaneIds and tabHistoryStacks) so later uses of Object.assign or mutations
(see the places that call toWindowTabsState and the Object.assign merge code)
will not mutate persisted/shared state; also ensure any Object.assign calls
create a new target object (Object.assign({}, existing, updates)) rather than
mutating an input object.
- Around line 96-98: The resetTabsState function currently assigns
appState.data.tabsState = defaultAppState.tabsState by reference which can let
future in-place mutations corrupt the canonical default; replace that assignment
with a deep copy (e.g., structuredClone or an equivalent deep-clone utility) so
appState.data.tabsState becomes an independent object, and keep the existing
clearing of appState.data.tabsStateByWindow; update references in resetTabsState
to use the cloned value rather than the original default object.
- Around line 22-29: The function getTabsStateForWindow reads
appState.data.tabsStateByWindow[key] without verifying tabsStateByWindow exists;
guard access to avoid crashes when older/malformed persisted state lacks that
property by checking appState.data.tabsStateByWindow (or using optional
chaining) before indexing with key, and fall back to
toWindowTabsState(sharedState) when tabsStateByWindow is missing or key lookup
yields undefined; update the windowState assignment in getTabsStateForWindow
accordingly so it references appState.data.tabsStateByWindow only after
confirming that object exists.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/app-state/schemas.tsapps/desktop/src/main/lib/app-state/tabs-state.ts
| function toWindowTabsState( | ||
| state: Partial<TabsState> | undefined, | ||
| ): WindowTabsState { | ||
| return { | ||
| activeTabIds: state?.activeTabIds ?? {}, | ||
| focusedPaneIds: state?.focusedPaneIds ?? {}, | ||
| tabHistoryStacks: state?.tabHistoryStacks ?? {}, | ||
| }; |
There was a problem hiding this comment.
toWindowTabsState currently aliases maps, causing merge-time mutation side effects.
Lines 16-18 return original map references. Then Lines 46-48 mutate those objects via Object.assign, which can unintentionally mutate shared persisted state during read/merge.
💡 Proposed fix
function toWindowTabsState(
state: Partial<TabsState> | undefined,
): WindowTabsState {
return {
- activeTabIds: state?.activeTabIds ?? {},
- focusedPaneIds: state?.focusedPaneIds ?? {},
- tabHistoryStacks: state?.tabHistoryStacks ?? {},
+ activeTabIds: { ...(state?.activeTabIds ?? {}) },
+ focusedPaneIds: { ...(state?.focusedPaneIds ?? {}) },
+ tabHistoryStacks: { ...(state?.tabHistoryStacks ?? {}) },
};
}Also applies to: 40-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/app-state/tabs-state.ts` around lines 12 - 19,
toWindowTabsState is returning references to the original maps which leads to
merge-time mutation; change it to return new shallow copies instead of aliasing
(e.g., replace activeTabIds: state?.activeTabIds ?? {} with activeTabIds: {
...state?.activeTabIds } ?? {} and do the same for focusedPaneIds and
tabHistoryStacks) so later uses of Object.assign or mutations (see the places
that call toWindowTabsState and the Object.assign merge code) will not mutate
persisted/shared state; also ensure any Object.assign calls create a new target
object (Object.assign({}, existing, updates)) rather than mutating an input
object.
| export function getTabsStateForWindow( | ||
| windowId: number | null | undefined, | ||
| ): TabsState { | ||
| const sharedState = appState.data.tabsState ?? defaultAppState.tabsState; | ||
| const key = getWindowKey(windowId); | ||
| const windowState = | ||
| (key ? appState.data.tabsStateByWindow[key] : undefined) ?? | ||
| toWindowTabsState(sharedState); |
There was a problem hiding this comment.
Guard tabsStateByWindow before keyed access to avoid crash.
On Line 28, appState.data.tabsStateByWindow[key] can throw if older/malformed persisted state is loaded without this property.
💡 Proposed fix
export function getTabsStateForWindow(
windowId: number | null | undefined,
): TabsState {
const sharedState = appState.data.tabsState ?? defaultAppState.tabsState;
const key = getWindowKey(windowId);
+ const byWindow = appState.data.tabsStateByWindow ?? {};
const windowState =
- (key ? appState.data.tabsStateByWindow[key] : undefined) ??
+ (key ? byWindow[key] : undefined) ??
toWindowTabsState(sharedState);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function getTabsStateForWindow( | |
| windowId: number | null | undefined, | |
| ): TabsState { | |
| const sharedState = appState.data.tabsState ?? defaultAppState.tabsState; | |
| const key = getWindowKey(windowId); | |
| const windowState = | |
| (key ? appState.data.tabsStateByWindow[key] : undefined) ?? | |
| toWindowTabsState(sharedState); | |
| export function getTabsStateForWindow( | |
| windowId: number | null | undefined, | |
| ): TabsState { | |
| const sharedState = appState.data.tabsState ?? defaultAppState.tabsState; | |
| const key = getWindowKey(windowId); | |
| const byWindow = appState.data.tabsStateByWindow ?? {}; | |
| const windowState = | |
| (key ? byWindow[key] : undefined) ?? | |
| toWindowTabsState(sharedState); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/app-state/tabs-state.ts` around lines 22 - 29, The
function getTabsStateForWindow reads appState.data.tabsStateByWindow[key]
without verifying tabsStateByWindow exists; guard access to avoid crashes when
older/malformed persisted state lacks that property by checking
appState.data.tabsStateByWindow (or using optional chaining) before indexing
with key, and fall back to toWindowTabsState(sharedState) when tabsStateByWindow
is missing or key lookup yields undefined; update the windowState assignment in
getTabsStateForWindow accordingly so it references
appState.data.tabsStateByWindow only after confirming that object exists.
| export function resetTabsState(): void { | ||
| appState.data.tabsState = defaultAppState.tabsState; | ||
| appState.data.tabsStateByWindow = {}; |
There was a problem hiding this comment.
Avoid assigning defaultAppState.tabsState by reference during reset.
Line 97 reuses the same object instance from defaults; later in-place mutations can corrupt the canonical default object.
💡 Proposed fix
export function resetTabsState(): void {
- appState.data.tabsState = defaultAppState.tabsState;
+ appState.data.tabsState = {
+ tabs: [...defaultAppState.tabsState.tabs],
+ panes: { ...defaultAppState.tabsState.panes },
+ activeTabIds: { ...defaultAppState.tabsState.activeTabIds },
+ focusedPaneIds: { ...defaultAppState.tabsState.focusedPaneIds },
+ tabHistoryStacks: { ...defaultAppState.tabsState.tabHistoryStacks },
+ };
appState.data.tabsStateByWindow = {};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/app-state/tabs-state.ts` around lines 96 - 98, The
resetTabsState function currently assigns appState.data.tabsState =
defaultAppState.tabsState by reference which can let future in-place mutations
corrupt the canonical default; replace that assignment with a deep copy (e.g.,
structuredClone or an equivalent deep-clone utility) so appState.data.tabsState
becomes an independent object, and keep the existing clearing of
appState.data.tabsStateByWindow; update references in resetTabsState to use the
cloned value rather than the original default object.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
/workspaceValidation
bun run --cwd apps/desktop typecheckSummary by cubic
Adds a “New Window” action in the tray and Window menu to open another desktop window. New windows go to the last active workspace (or the workspace list), support deep links, and keep isolated view state per window while syncing tabs/panes across windows.
New Features
Stability
Written for commit 5b70038. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements