feat: add session snapshot persistence and restoration#7494
Open
chirag-bruno wants to merge 1 commit intousebruno:mainfrom
Open
feat: add session snapshot persistence and restoration#7494chirag-bruno wants to merge 1 commit intousebruno:mainfrom
chirag-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
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:
WalkthroughAdds snapshot persistence and restoration across renderer and main: debounced snapshot middleware, IPC handlers and SnapshotManager, snapshot (de)serialization utilities, extended tab shape (pathname, exampleName), tab restore/sync reducers, collection/workspace restore wiring, and loading UI for restored/missing tabs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Renderer App (Redux)
participant Middleware as Snapshot Middleware
participant ipcR as ipcRenderer
participant Main as Main Process
participant Service as SnapshotManager
participant FS as File System
App->>Middleware: Dispatch action in SAVE_TRIGGERS
Middleware->>Middleware: Debounce (1000ms) then serialize snapshot
Middleware->>ipcR: invoke('renderer:save-snapshot', snapshot)
ipcR->>Main: IPC request -> save-snapshot
Main->>Service: SnapshotManager.saveSnapshot(data)
Service->>Service: Validate schema
Service->>FS: Write snapshot file
FS-->>Service: success/error
Service-->>Main: result
Main-->>ipcR: IPC reply
ipcR-->>Middleware: resolution
sequenceDiagram
participant App as Renderer App
participant Action as switchWorkspace / mountCollection
participant ipcR as ipcRenderer
participant Main as Main Process
participant Service as SnapshotManager
participant Store as Redux Store
App->>Action: switchWorkspace / mountCollection
Action->>ipcR: invoke('renderer:get-workspace-snapshot' / 'renderer:get-collection-snapshot')
ipcR->>Main: request snapshot
Main->>Service: getWorkspace/getCollection
Service-->>Main: snapshot data
Main-->>ipcR: reply with snapshot
ipcR-->>Action: snapshot data
Action->>Store: dispatch(restoreTabs(collectionUid, tabs, activeTab))
Store-->>App: tabs restored (matched by UID, pathname, or exampleName)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
📝 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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-app/src/components/GlobalSearchModal/index.js (1)
247-266:⚠️ Potential issue | 🟠 MajorPass
pathnamefor request search results too.The folder branch now carries it, but the request branch above still omits
pathname. Request tabs opened from Global Search will bypass the new pathname-based restore flow, so they won't behave like tabs opened from the sidebar after a restart.🧭 Proposed fix
dispatch(addTab({ uid: result.item.uid, collectionUid: result.collectionUid, requestPaneTab: getDefaultRequestPaneTab(result.item), - type: 'request' + type: 'request', + pathname: result.item.pathname }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/GlobalSearchModal/index.js` around lines 247 - 266, When handling SEARCH_TYPES.REQUEST in the GlobalSearchModal, include the pathname so request tabs restore via the pathname-based flow: when dispatching addTab inside the SEARCH_TYPES.REQUEST branch (where result, result.item.uid, result.collectionUid, getDefaultRequestPaneTab(result.item) and addTab are used), add pathname: result.item.pathname to the dispatched payload so request tabs behave like sidebar-opened tabs after restart.packages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.js (1)
22-30:⚠️ Potential issue | 🟡 MinorAdd
display: blockto maketext-overflow: ellipsiswork on the span element.
.tab-nameis rendered as a<span>, andtext-overflow: ellipsisonly works on block-level elements. Even withoverflow: hiddenand constrained width from the parent flex container, the inline span won't display the ellipsis.💄 Proposed fix
.tab-name { position: relative; + display: block; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; font-size: 0.8125rem;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.js` around lines 22 - 30, The .tab-name span is inline so text-overflow: ellipsis doesn't take effect; update the .tab-name rule in StyledWrapper.js to set a block-level display (e.g., display: block or display: inline-block) while keeping overflow: hidden/white-space: nowrap/text-overflow: ellipsis so the ellipsis appears for truncated tab names.
🧹 Nitpick comments (1)
packages/bruno-electron/src/services/snapshot/index.js (1)
99-99: Prefer atomic snapshot writes to reduce corruption risk.Writing directly to
app-snapshot.jsoncan leave a broken file on interruption. Consider temp-write then rename in the same directory.Proposed refactor
- fs.writeFileSync(this.getSnapshotPath(), JSON.stringify(data, null, 2), 'utf8'); + const snapshotPath = this.getSnapshotPath(); + const tempPath = `${snapshotPath}.tmp`; + fs.writeFileSync(tempPath, JSON.stringify(data, null, 2), 'utf8'); + fs.renameSync(tempPath, snapshotPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/services/snapshot/index.js` at line 99, The current direct write in writeSnapshot (fs.writeFileSync(this.getSnapshotPath(), ...)) risks corruption; change it to perform an atomic write by writing JSON.stringify(data, null, 2) to a temp file in the same directory (e.g., original name + .tmp or a unique tmp name), fsyncing/closing if necessary, then fs.renameSync the temp file to this.getSnapshotPath() to atomically replace the snapshot; handle and propagate errors and ensure the temp file is cleaned up on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/RequestTabPanel/index.js`:
- Around line 251-260: The current check in RequestTabPanel that renders
RequestTabPanelLoading when focusedTab.name is truthy can hang indefinitely for
deleted/missing items; update the conditional around RequestTabPanelLoading
(where focusedTab.name, focusedTab.pathname and activeTabUid are used) to only
show the loading UI while the tab is actually pending resolution (e.g., a
dedicated pending/loading flag or while the collection/folder lookup is
in-flight) and add a fallback path that renders the not-found/cleared-tab state
when the lookup fails or the target folder/item is confirmed missing, rather
than relying solely on focusedTab.name.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js`:
- Around line 205-207: activeTabInCollection selection for response examples is
not unique; change how activeTab is set and restored by using a composite key
combining pathname and exampleName (e.g., `${pathname}::${exampleName}`) instead
of exampleName alone when assigning activeTab.value in the logic that sets
activeTab (refer to activeTabInCollection and activeTab), and update the
matching logic in restoreTabs to look for that same composite key so examples
are uniquely identified across requests.
In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js`:
- Around line 297-305: The uid assignment for restored tabs uses
snapshotTab.pathname which can collide for multiple tabs of the same path;
update the uid generation in tabs.js (the code that sets const uid =
snapshotTab.pathname || ...) to compose a unique key incorporating
snapshotTab.type and a per-tab identifier (e.g., snapshotTab.exampleName or
snapshotTab.exampleId when present) and fall back to the existing
`temp-${Date.now()}-${Math.random()}` pattern; ensure the object pushed into
state.tabs still uses the new uid while preserving existing
pathname/name/exampleName fields.
- Around line 287-324: After removing tabs for the collection (state.tabs =
state.tabs.filter(...)), ensure state.activeTabUid is cleared if it points to a
now-removed or different-collection tab so it doesn't remain stale;
specifically, check state.activeTabUid against the new state.tabs (and/or its
collectionUid) and set state.activeTabUid = null when the uid is not found or
belongs to another collection, then let the existing snapshotTabs loop /
fallback logic assign a proper active tab (references: state.tabs,
state.activeTabUid, isActiveTab, snapshotTabs).
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 347-352: findTabByAccessor currently only handles 'pathname' and
treats all other accessors as 'type', so tabs with accessor 'exampleName' won't
be matched; update the function (findTabByAccessor) to explicitly handle
accessor === 'exampleName' by returning tabs.find(t => t.exampleName ===
activeTab.value), keep the existing branch for 'pathname' and the fallback for
'type', and ensure the early null return for missing activeTab remains.
- Around line 524-536: Normalize and canonicalize the workspace paths before
comparing by resolving and realpath-ing both values: call
fs.promises.realpath(path.resolve(...)) (handle errors) for
snapshot.activeWorkspacePath and for workspacePath, then compare the resulting
canonicalized paths when setting shouldSwitch in the block that reads snapshot
via ipcRenderer.invoke('renderer:get-snapshot'); keep the existing fallback
logic (using workspaceConfig.type and activeWorkspaceUid) in the catch path if
realpath/resolve fails.
- Around line 388-407: The code only calls
ipcRenderer.invoke('renderer:get-collection-snapshot',
activeCollection.pathname) when needsMount is true, so restoredActiveTab remains
null for collections already mounted; change the flow in the block where
activeCollection is handled (the activeCollection / needsMount logic around
dispatch(mountCollection)) to always call ipcRenderer.invoke(...) regardless of
needsMount, assign its result to restoredActiveTab =
collectionSnapshot?.activeTab, and keep the existing mountCollection dispatch
when needsMount is true; also preserve the current .catch(() => null) error
handling so a failed snapshot fetch doesn't throw.
In `@packages/bruno-electron/src/services/snapshot/index.js`:
- Around line 87-94: getSnapshot() currently returns any parseable JSON which
can be an invalid snapshot; after reading and JSON.parse in getSnapshot(),
validate the parsed object against the expected snapshot shape (e.g., required
keys and types) and if validation fails return this._getEmptySnapshot(); add or
reuse a validator function (e.g., validateSnapshot(obj) or SnapshotValidator)
and call it from getSnapshot() so only correctly-shaped snapshots are returned
while parse errors still fall back to _getEmptySnapshot().
- Around line 96-104: The saveSnapshot function currently catches all errors
from snapshotSchema.validateSync and fs.writeFileSync and returns false, which
swallows failures; change it to let errors propagate by either removing the
try/catch or rethrowing after logging: keep the console.error('Failed to save
snapshot:', err.message) if desired but throw err (or rethrow a new Error with
context) so callers invoking saveSnapshot receive a rejected result; target the
saveSnapshot method and the validation/write calls (snapshotSchema.validateSync
and fs.writeFileSync) when making this change.
---
Outside diff comments:
In `@packages/bruno-app/src/components/GlobalSearchModal/index.js`:
- Around line 247-266: When handling SEARCH_TYPES.REQUEST in the
GlobalSearchModal, include the pathname so request tabs restore via the
pathname-based flow: when dispatching addTab inside the SEARCH_TYPES.REQUEST
branch (where result, result.item.uid, result.collectionUid,
getDefaultRequestPaneTab(result.item) and addTab are used), add pathname:
result.item.pathname to the dispatched payload so request tabs behave like
sidebar-opened tabs after restart.
In `@packages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.js`:
- Around line 22-30: The .tab-name span is inline so text-overflow: ellipsis
doesn't take effect; update the .tab-name rule in StyledWrapper.js to set a
block-level display (e.g., display: block or display: inline-block) while
keeping overflow: hidden/white-space: nowrap/text-overflow: ellipsis so the
ellipsis appears for truncated tab names.
---
Nitpick comments:
In `@packages/bruno-electron/src/services/snapshot/index.js`:
- Line 99: The current direct write in writeSnapshot
(fs.writeFileSync(this.getSnapshotPath(), ...)) risks corruption; change it to
perform an atomic write by writing JSON.stringify(data, null, 2) to a temp file
in the same directory (e.g., original name + .tmp or a unique tmp name),
fsyncing/closing if necessary, then fs.renameSync the temp file to
this.getSnapshotPath() to atomically replace the snapshot; handle and propagate
errors and ensure the temp file is cleaned up on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3691574-a0eb-4a2e-9087-42e0fe331f49
📒 Files selected for processing (19)
packages/bruno-app/src/components/GlobalSearchModal/index.jspackages/bruno-app/src/components/RequestTabPanel/RequestNotFound/index.jspackages/bruno-app/src/components/RequestTabPanel/RequestTabPanelLoading/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/RequestTabs/ExampleTab/index.jspackages/bruno-app/src/components/RequestTabs/RequestTab/RequestTabLoading.jspackages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.jspackages/bruno-app/src/components/RequestTabs/RequestTab/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/ExampleItem/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-electron/src/index.jspackages/bruno-electron/src/ipc/snapshot.jspackages/bruno-electron/src/services/snapshot/index.js
Comment on lines
+205
to
+207
| if (activeTabInCollection.type === 'response-example' && activeTabInCollection.exampleName) { | ||
| activeTab = { accessor: 'exampleName', value: activeTabInCollection.exampleName }; | ||
| } else if (activeTabInCollection.pathname) { |
Contributor
There was a problem hiding this comment.
Active response-example tab selector is not unique.
On Line 205, using exampleName alone can select the wrong tab when multiple requests have examples with the same name.
💡 Suggested direction
Use a composite key for response examples (e.g., pathname::exampleName) when writing activeTab.value, and match against that same composite in restoreTabs.
- if (activeTabInCollection.type === 'response-example' && activeTabInCollection.exampleName) {
- activeTab = { accessor: 'exampleName', value: activeTabInCollection.exampleName };
+ if (activeTabInCollection.type === 'response-example' && activeTabInCollection.exampleName) {
+ const exampleKey = `${activeTabInCollection.pathname || ''}::${activeTabInCollection.exampleName}`;
+ activeTab = { accessor: 'exampleName', value: exampleKey };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js`
around lines 205 - 207, activeTabInCollection selection for response examples is
not unique; change how activeTab is set and restored by using a composite key
combining pathname and exampleName (e.g., `${pathname}::${exampleName}`) instead
of exampleName alone when assigning activeTab.value in the logic that sets
activeTab (refer to activeTabInCollection and activeTab), and update the
matching logic in restoreTabs to look for that same composite key so examples
are uniquely identified across requests.
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Outdated
Show resolved
Hide resolved
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Show resolved
Hide resolved
Comment on lines
+524
to
+536
| let shouldSwitch = false; | ||
| try { | ||
| const snapshot = await ipcRenderer.invoke('renderer:get-snapshot'); | ||
| const activeWorkspacePath = snapshot?.activeWorkspacePath; | ||
|
|
||
| if (activeWorkspacePath) { | ||
| shouldSwitch = workspacePath === activeWorkspacePath; | ||
| } else { | ||
| shouldSwitch = !activeWorkspaceUid || workspaceConfig.type === 'default'; | ||
| } | ||
| } catch (err) { | ||
| shouldSwitch = !activeWorkspaceUid || workspaceConfig.type === 'default'; | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize workspace paths before comparing them.
This raw string equality is not stable across Windows/macOS/Linux path casing and separator differences, so the app can skip switching to the snapshot's active workspace even when both paths point to the same directory. As per coding guidelines, Bruno is a cross-platform Electron desktop app that runs on macOS, Windows, and Linux and we should Never assume case-sensitive or case-insensitive filesystems.
🪟 Proposed fix
const activeWorkspacePath = snapshot?.activeWorkspacePath;
if (activeWorkspacePath) {
- shouldSwitch = workspacePath === activeWorkspacePath;
+ shouldSwitch = normalizePath(workspacePath) === normalizePath(activeWorkspacePath);
} else {
shouldSwitch = !activeWorkspaceUid || workspaceConfig.type === 'default';
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`
around lines 524 - 536, Normalize and canonicalize the workspace paths before
comparing by resolving and realpath-ing both values: call
fs.promises.realpath(path.resolve(...)) (handle errors) for
snapshot.activeWorkspacePath and for workspacePath, then compare the resulting
canonicalized paths when setting shouldSwitch in the block that reads snapshot
via ipcRenderer.invoke('renderer:get-snapshot'); keep the existing fallback
logic (using workspaceConfig.type and activeWorkspaceUid) in the catch path if
realpath/resolve fails.
Comment on lines
+87
to
+94
| getSnapshot() { | ||
| try { | ||
| const content = fs.readFileSync(this.getSnapshotPath(), 'utf8'); | ||
| return JSON.parse(content); | ||
| } catch (err) { | ||
| return this._getEmptySnapshot(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate snapshot shape on read, not only on write.
getSnapshot() currently accepts any parseable JSON. Invalid-but-parseable files can break restore paths instead of falling back cleanly.
Proposed fix
getSnapshot() {
try {
const content = fs.readFileSync(this.getSnapshotPath(), 'utf8');
- return JSON.parse(content);
+ const parsed = JSON.parse(content);
+ snapshotSchema.validateSync(parsed, { strict: true });
+ return parsed;
} catch (err) {
return this._getEmptySnapshot();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/services/snapshot/index.js` around lines 87 - 94,
getSnapshot() currently returns any parseable JSON which can be an invalid
snapshot; after reading and JSON.parse in getSnapshot(), validate the parsed
object against the expected snapshot shape (e.g., required keys and types) and
if validation fails return this._getEmptySnapshot(); add or reuse a validator
function (e.g., validateSnapshot(obj) or SnapshotValidator) and call it from
getSnapshot() so only correctly-shaped snapshots are returned while parse errors
still fall back to _getEmptySnapshot().
Comment on lines
+96
to
+104
| saveSnapshot(data) { | ||
| try { | ||
| snapshotSchema.validateSync(data, { strict: true }); | ||
| fs.writeFileSync(this.getSnapshotPath(), JSON.stringify(data, null, 2), 'utf8'); | ||
| return true; | ||
| } catch (err) { | ||
| console.error('Failed to save snapshot:', err.message); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Do not swallow snapshot save failures.
Returning false here is effectively silent for current renderer callers; they only handle rejected invokes. Throw so failures propagate predictably.
Proposed fix
saveSnapshot(data) {
try {
snapshotSchema.validateSync(data, { strict: true });
fs.writeFileSync(this.getSnapshotPath(), JSON.stringify(data, null, 2), 'utf8');
return true;
} catch (err) {
- console.error('Failed to save snapshot:', err.message);
- return false;
+ console.error('Failed to save snapshot:', err.message);
+ throw err;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/services/snapshot/index.js` around lines 96 -
104, The saveSnapshot function currently catches all errors from
snapshotSchema.validateSync and fs.writeFileSync and returns false, which
swallows failures; change it to let errors propagate by either removing the
try/catch or rethrowing after logging: keep the console.error('Failed to save
snapshot:', err.message) if desired but throw err (or rethrow a new Error with
context) so callers invoking saveSnapshot receive a rejected result; target the
saveSnapshot method and the validation/write calls (snapshotSchema.validateSync
and fs.writeFileSync) when making this change.
6 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (2)
388-401:⚠️ Potential issue | 🟠 MajorSnapshot lookup is gated on
needsMount, leavingrestoredActiveTabnull for already-mounted collections.When a user switches back to a workspace whose active collection is already mounted,
needsMountis false, the snapshot fetch is skipped, andrestoredActiveTabstays null. The UI then falls back to the workspace overview instead of restoring the last active tab.Proposed fix
if (activeCollection) { dispatch(expandCollection(activeCollection.uid)); const needsMount = activeCollection.mountStatus !== 'mounted' && activeCollection.mountStatus !== 'mounting'; if (needsMount) { await dispatch(mountCollection({ collectionUid: activeCollection.uid, collectionPathname: activeCollection.pathname, brunoConfig: activeCollection.brunoConfig })).catch((err) => console.error('Failed to mount active collection:', err)); - - const collectionSnapshot = await ipcRenderer.invoke('renderer:get-collection-snapshot', activeCollection.pathname).catch(() => null); - restoredActiveTab = collectionSnapshot?.activeTab; } + + const collectionSnapshot = await ipcRenderer.invoke('renderer:get-collection-snapshot', activeCollection.pathname).catch(() => null); + restoredActiveTab = collectionSnapshot?.activeTab; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 388 - 401, The snapshot lookup should run regardless of whether a mount was needed so restoredActiveTab is populated for already-mounted collections; modify the logic around activeCollection so that you still compute needsMount and await dispatch(mountCollection(...)) when true (using the existing .catch error handling), but move the ipcRenderer.invoke('renderer:get-collection-snapshot', activeCollection.pathname) call (and assignment to restoredActiveTab) outside/after the needsMount conditional so it always executes for an activeCollection; keep existing error handling for the snapshot fetch and preserve use of dispatch(expandCollection(activeCollection.uid)) and the mountCollection call.
518-530:⚠️ Potential issue | 🟠 MajorRaw string equality for workspace paths is not cross-platform safe.
workspacePath === activeWorkspacePathcan fail on Windows due to case differences or mixed separators. Normalize both paths before comparing.Proposed fix
if (activeWorkspacePath) { - shouldSwitch = workspacePath === activeWorkspacePath; + shouldSwitch = normalizePath(workspacePath) === normalizePath(activeWorkspacePath); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 518 - 530, The equality check using workspacePath === activeWorkspacePath is not cross-platform safe; normalize both paths before comparing (e.g., use Node's path utilities to normalize/resolve and normalize separators and case) where the snapshot is read (around ipcRenderer.invoke('renderer:get-snapshot') and the shouldSwitch assignment) so that workspacePath and activeWorkspacePath are compared in a canonical form; keep the fallback logic using activeWorkspaceUid and workspaceConfig.type unchanged.packages/bruno-app/src/utils/snapshot/index.js (1)
213-232:⚠️ Potential issue | 🔴 CriticalUID collision for
response-exampletabs sharing the same pathname.
deserializeTabusessnapshotTab.pathnameas the UID. Multiple response-example tabs under the same request will get identical UIDs, breaking focus and close behavior.Proposed fix
export const deserializeTab = (snapshotTab, collectionUid) => { - const uid = snapshotTab.pathname || `temp-${Date.now()}-${Math.random()}`; + let uid = snapshotTab.pathname || `temp-${Date.now()}-${Math.random()}`; + if (snapshotTab.type === 'response-example' && snapshotTab.exampleName) { + uid = `${snapshotTab.pathname || 'response-example'}::${snapshotTab.exampleName}`; + } return { uid,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/snapshot/index.js` around lines 213 - 232, deserializeTab currently uses snapshotTab.pathname as the uid which causes collisions for multiple response-example tabs under the same request; update deserializeTab so that when snapshotTab.type === 'response-example' it composes a unique uid (e.g., combine snapshotTab.pathname with a stable differentiator such as snapshotTab.exampleName or fallback to a timestamp/random suffix) instead of relying on pathname alone, keeping the existing temp- fallback behavior for other types; change the uid assignment in deserializeTab to incorporate this logic so response-example tabs get distinct identifiers.packages/bruno-app/src/providers/ReduxStore/slices/tabs.js (1)
286-302:⚠️ Potential issue | 🔴 Critical
activeTabUidcan remain dangling afterrestoreTabs.The filter at line 288 removes all tabs for the collection, but
state.activeTabUidmay still reference one of those removed tabs. The fallback at line 300 only triggers when!state.activeTabUid, not when it points to a stale UID.Proposed fix
restoreTabs: (state, action) => { const { collectionUid, tabs: snapshotTabs, activeTab } = action.payload; + const activeTabBelongedToCollection = state.tabs.some( + (t) => t.uid === state.activeTabUid && t.collectionUid === collectionUid + ); state.tabs = state.tabs.filter((t) => t.collectionUid !== collectionUid); + if (activeTabBelongedToCollection) { + state.activeTabUid = null; + } (snapshotTabs || []).forEach((snapshotTab) => { const tab = deserializeTab(snapshotTab, collectionUid); state.tabs.push(tab); if (checkIsActiveTab(snapshotTab, activeTab)) { state.activeTabUid = tab.uid; } }); - if (!state.activeTabUid) { + const activeStillExists = state.tabs.some((t) => t.uid === state.activeTabUid); + if (!activeStillExists) { state.activeTabUid = state.tabs.find((t) => t.collectionUid === collectionUid)?.uid || null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js` around lines 286 - 302, The restoreTabs reducer can leave state.activeTabUid pointing at a removed tab from the same collection; update restoreTabs so that after removing existing tabs for collectionUid you also clear state.activeTabUid if it references a tab that was removed, then proceed to deserializeTab and restore tabs and set activeTabUid from checkIsActiveTab as currently done; alternatively, after the restore loop validate that state.activeTabUid refers to a tab whose collectionUid === collectionUid and if not set it to null (so the existing fallback will pick a valid tab).packages/bruno-app/src/components/RequestTabPanel/index.js (1)
251-260:⚠️ Potential issue | 🟠 MajorLoading state can persist indefinitely for deleted/missing items.
Both folder and item branches render
RequestTabPanelLoadingwhenfocusedTab.nameis truthy, but there's no escape hatch if the item genuinely doesn't exist. Once the collection finishes mounting, the spinner remains forever.Proposed fix
if (!folder) { - if (focusedTab.name) { + if (focusedTab.name && collection?.mountStatus === 'mounting') { return ( <RequestTabPanelLoadingif (!item || !item.uid) { - if (focusedTab.name) { + if (focusedTab.name && collection?.mountStatus === 'mounting') { return ( <RequestTabPanelLoadingAlso applies to: 281-290
🧹 Nitpick comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
350-350: RedundantipcRendererdeclaration.
ipcRendereris already destructured at module scope (line 21). This inner declaration shadows it unnecessarily.export const switchWorkspace = (workspaceUid) => { return async (dispatch, getState) => { - const { ipcRenderer } = window; dispatch(setActiveWorkspace(workspaceUid));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` at line 350, Remove the redundant local destructuring "const { ipcRenderer } = window;" inside the function so it doesn't shadow the module-scope ipcRenderer; update any references in the surrounding function to use the existing module-level ipcRenderer (destructured earlier at top of the file) and ensure no other local variable named ipcRenderer remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 99-104: The path comparison in shouldExcludeTab can fail on
Windows due to mixed separators and casing; update shouldExcludeTab to normalize
both tab.pathname and transientDirectory (e.g., use path.resolve/path.normalize
and replace backslashes with forward slashes or use path.sep-aware
normalization) and perform a case-insensitive compare (toLowerCase()) before
using startsWith; ensure you still guard for undefined pathname and return true
when the normalized pathname startsWith the normalized transientDirectory.
- Around line 44-82: The SAVE_TRIGGERS set is missing the 'tabs/restoreTabs'
action so restored tab state isn't persisted; update the SAVE_TRIGGERS constant
to include 'tabs/restoreTabs' alongside the other tab lifecycle entries (refer
to the SAVE_TRIGGERS export and add the string 'tabs/restoreTabs' within that
Set so restore actions trigger saves).
---
Duplicate comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/tabs.js`:
- Around line 286-302: The restoreTabs reducer can leave state.activeTabUid
pointing at a removed tab from the same collection; update restoreTabs so that
after removing existing tabs for collectionUid you also clear state.activeTabUid
if it references a tab that was removed, then proceed to deserializeTab and
restore tabs and set activeTabUid from checkIsActiveTab as currently done;
alternatively, after the restore loop validate that state.activeTabUid refers to
a tab whose collectionUid === collectionUid and if not set it to null (so the
existing fallback will pick a valid tab).
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 388-401: The snapshot lookup should run regardless of whether a
mount was needed so restoredActiveTab is populated for already-mounted
collections; modify the logic around activeCollection so that you still compute
needsMount and await dispatch(mountCollection(...)) when true (using the
existing .catch error handling), but move the
ipcRenderer.invoke('renderer:get-collection-snapshot',
activeCollection.pathname) call (and assignment to restoredActiveTab)
outside/after the needsMount conditional so it always executes for an
activeCollection; keep existing error handling for the snapshot fetch and
preserve use of dispatch(expandCollection(activeCollection.uid)) and the
mountCollection call.
- Around line 518-530: The equality check using workspacePath ===
activeWorkspacePath is not cross-platform safe; normalize both paths before
comparing (e.g., use Node's path utilities to normalize/resolve and normalize
separators and case) where the snapshot is read (around
ipcRenderer.invoke('renderer:get-snapshot') and the shouldSwitch assignment) so
that workspacePath and activeWorkspacePath are compared in a canonical form;
keep the fallback logic using activeWorkspaceUid and workspaceConfig.type
unchanged.
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 213-232: deserializeTab currently uses snapshotTab.pathname as the
uid which causes collisions for multiple response-example tabs under the same
request; update deserializeTab so that when snapshotTab.type ===
'response-example' it composes a unique uid (e.g., combine snapshotTab.pathname
with a stable differentiator such as snapshotTab.exampleName or fallback to a
timestamp/random suffix) instead of relying on pathname alone, keeping the
existing temp- fallback behavior for other types; change the uid assignment in
deserializeTab to incorporate this logic so response-example tabs get distinct
identifiers.
---
Nitpick comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Line 350: Remove the redundant local destructuring "const { ipcRenderer } =
window;" inside the function so it doesn't shadow the module-scope ipcRenderer;
update any references in the surrounding function to use the existing
module-level ipcRenderer (destructured earlier at top of the file) and ensure no
other local variable named ipcRenderer remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c54eedf-b84b-4202-afa0-1bf4d8f713d5
📒 Files selected for processing (6)
packages/bruno-app/src/components/RequestTabPanel/RequestNotFound/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-app/src/utils/snapshot/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js
- packages/bruno-app/src/components/RequestTabPanel/RequestNotFound/index.js
Comment on lines
+44
to
+82
| export const SAVE_TRIGGERS = new Set([ | ||
| // Tab actions - lifecycle | ||
| 'tabs/addTab', | ||
| 'tabs/closeTabs', | ||
| 'tabs/focusTab', | ||
| 'tabs/closeAllCollectionTabs', | ||
| 'tabs/reorderTabs', | ||
| 'tabs/makeTabPermanent', | ||
| 'tabs/syncTabUid', | ||
|
|
||
| // Tab actions - request pane state | ||
| 'tabs/updateRequestPaneTab', | ||
| 'tabs/updateRequestPaneTabWidth', | ||
| 'tabs/updateRequestPaneTabHeight', | ||
|
|
||
| // Tab actions - response pane state | ||
| 'tabs/updateResponsePaneTab', | ||
| 'tabs/updateResponsePaneScrollPosition', | ||
| 'tabs/updateResponseFormat', | ||
| 'tabs/updateResponseViewTab', | ||
|
|
||
| // Tab actions - other pane state | ||
| 'tabs/updateScriptPaneTab', | ||
| 'tabs/updateRequestBodyScrollPosition', | ||
|
|
||
| // Workspace actions | ||
| 'workspaces/setActiveWorkspace', | ||
|
|
||
| // Collection actions | ||
| 'collections/selectEnvironment', | ||
| 'collections/updateCollectionMountStatus', | ||
| 'collections/toggleCollection', | ||
| 'collections/expandCollection', | ||
|
|
||
| // Console/devtools actions | ||
| 'logs/openConsole', | ||
| 'logs/closeConsole', | ||
| 'logs/setActiveTab' | ||
| ]); |
Contributor
There was a problem hiding this comment.
tabs/restoreTabs is missing from SAVE_TRIGGERS.
After restoring tabs, the new state won't be persisted because restoreTabs isn't in the trigger set. If the app crashes before another triggering action, the restored tabs are lost.
// Tab actions - lifecycle
'tabs/addTab',
'tabs/closeTabs',
'tabs/focusTab',
'tabs/closeAllCollectionTabs',
'tabs/reorderTabs',
'tabs/makeTabPermanent',
'tabs/syncTabUid',
+ 'tabs/restoreTabs',📝 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.
Suggested change
| export const SAVE_TRIGGERS = new Set([ | |
| // Tab actions - lifecycle | |
| 'tabs/addTab', | |
| 'tabs/closeTabs', | |
| 'tabs/focusTab', | |
| 'tabs/closeAllCollectionTabs', | |
| 'tabs/reorderTabs', | |
| 'tabs/makeTabPermanent', | |
| 'tabs/syncTabUid', | |
| // Tab actions - request pane state | |
| 'tabs/updateRequestPaneTab', | |
| 'tabs/updateRequestPaneTabWidth', | |
| 'tabs/updateRequestPaneTabHeight', | |
| // Tab actions - response pane state | |
| 'tabs/updateResponsePaneTab', | |
| 'tabs/updateResponsePaneScrollPosition', | |
| 'tabs/updateResponseFormat', | |
| 'tabs/updateResponseViewTab', | |
| // Tab actions - other pane state | |
| 'tabs/updateScriptPaneTab', | |
| 'tabs/updateRequestBodyScrollPosition', | |
| // Workspace actions | |
| 'workspaces/setActiveWorkspace', | |
| // Collection actions | |
| 'collections/selectEnvironment', | |
| 'collections/updateCollectionMountStatus', | |
| 'collections/toggleCollection', | |
| 'collections/expandCollection', | |
| // Console/devtools actions | |
| 'logs/openConsole', | |
| 'logs/closeConsole', | |
| 'logs/setActiveTab' | |
| ]); | |
| export const SAVE_TRIGGERS = new Set([ | |
| // Tab actions - lifecycle | |
| 'tabs/addTab', | |
| 'tabs/closeTabs', | |
| 'tabs/focusTab', | |
| 'tabs/closeAllCollectionTabs', | |
| 'tabs/reorderTabs', | |
| 'tabs/makeTabPermanent', | |
| 'tabs/syncTabUid', | |
| 'tabs/restoreTabs', | |
| // Tab actions - request pane state | |
| 'tabs/updateRequestPaneTab', | |
| 'tabs/updateRequestPaneTabWidth', | |
| 'tabs/updateRequestPaneTabHeight', | |
| // Tab actions - response pane state | |
| 'tabs/updateResponsePaneTab', | |
| 'tabs/updateResponsePaneScrollPosition', | |
| 'tabs/updateResponseFormat', | |
| 'tabs/updateResponseViewTab', | |
| // Tab actions - other pane state | |
| 'tabs/updateScriptPaneTab', | |
| 'tabs/updateRequestBodyScrollPosition', | |
| // Workspace actions | |
| 'workspaces/setActiveWorkspace', | |
| // Collection actions | |
| 'collections/selectEnvironment', | |
| 'collections/updateCollectionMountStatus', | |
| 'collections/toggleCollection', | |
| 'collections/expandCollection', | |
| // Console/devtools actions | |
| 'logs/openConsole', | |
| 'logs/closeConsole', | |
| 'logs/setActiveTab' | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/snapshot/index.js` around lines 44 - 82, The
SAVE_TRIGGERS set is missing the 'tabs/restoreTabs' action so restored tab state
isn't persisted; update the SAVE_TRIGGERS constant to include 'tabs/restoreTabs'
alongside the other tab lifecycle entries (refer to the SAVE_TRIGGERS export and
add the string 'tabs/restoreTabs' within that Set so restore actions trigger
saves).
Comment on lines
+99
to
+104
| export const shouldExcludeTab = (tab, transientDirectory) => { | ||
| if (transientDirectory && tab.pathname?.startsWith(transientDirectory)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }; |
Contributor
There was a problem hiding this comment.
Path comparison with startsWith may fail on Windows.
Mixed path separators or casing differences could cause transient tabs to slip through. Consider normalizing both paths before comparison.
Proposed fix
+import { normalizePath } from 'utils/common/path';
+
export const shouldExcludeTab = (tab, transientDirectory) => {
- if (transientDirectory && tab.pathname?.startsWith(transientDirectory)) {
+ if (transientDirectory && tab.pathname) {
+ const normalizedTabPath = normalizePath(tab.pathname);
+ const normalizedTransient = normalizePath(transientDirectory);
+ if (normalizedTabPath.startsWith(normalizedTransient)) {
+ return true;
+ }
- return true;
}
return false;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/snapshot/index.js` around lines 99 - 104, The
path comparison in shouldExcludeTab can fail on Windows due to mixed separators
and casing; update shouldExcludeTab to normalize both tab.pathname and
transientDirectory (e.g., use path.resolve/path.normalize and replace
backslashes with forward slashes or use path.sep-aware normalization) and
perform a case-insensitive compare (toLowerCase()) before using startsWith;
ensure you still guard for undefined pathname and return true when the
normalized pathname startsWith the normalized transientDirectory.
cf13875 to
77d8af9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
351-351: Remove redundantipcRendererdeclaration.
ipcRendereris already destructured fromwindowat line 22 (module scope). This inner re-declaration is unnecessary.🧹 Proposed fix
export const switchWorkspace = (workspaceUid) => { return async (dispatch, getState) => { - const { ipcRenderer } = window; dispatch(setActiveWorkspace(workspaceUid));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` at line 351, Remove the redundant inner declaration of ipcRenderer (const { ipcRenderer } = window;) inside the function — the module already destructures ipcRenderer at module scope, so delete the inner declaration and use the existing ipcRenderer symbol (e.g., in the actions file where ipcRenderer is re-declared) to avoid shadowing; run lint/format after the removal to ensure no unused-variable warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/components/RequestTabs/ExampleTab/index.js`:
- Line 35: hasChanges can return false during mount because example?.uid is
briefly undefined; update the useMemo call so hasExampleChanges is invoked with
a stable fallback UID (e.g., the parsed pathname or existing requestUid
variable) instead of possibly undefined, and include that fallback in the
dependency array. Specifically, modify the useMemo around hasChanges (currently
using hasExampleChanges(item, example?.uid)) to call hasExampleChanges(item,
exampleUidFallback) where exampleUidFallback is derived from the pathname or
other stable source, and add exampleUidFallback to the dependency list so
unsaved changes are detected during mount.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js`:
- Around line 56-63: The active collection membership check uses raw path
strings and can misclassify on differing path formats; import normalizePath (as
suggested) and normalize each workspace collection path when building
workspaceCollectionPaths and also normalize activeCollection.pathname before
comparing, ensuring you still filter falsy values and preserve
activeCollectionUid logic (look for workspaceCollectionPaths, activeTab,
activeCollectionUid, and activeCollection.pathname in this file to update).
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 2965-2973: mountCollection currently re-dispatches restoreTabs
even though hydrateTabs already restored tabs during workspace hydration; modify
the mountCollection flow around the
ipcRenderer.invoke('renderer:get-collection-snapshot', collectionPathname)
result so you only call dispatch(restoreTabs({ collectionUid, ... })) when tabs
have not already been hydrated for this collection — e.g., check existing tabs
in Redux state for collectionUid (or a workspaceHydrated flag) and skip calling
restoreTabs if tabs exist or hydration completed; keep the existing condition on
collectionSnapshot?.tabs?.length > 0 and add the dup-check before dispatching
restoreTabs.
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 407-414: The code is passing a tab UID as the pathname to addTab
because getActiveTabFromSnapshot currently returns only tab.uid; change this by
having getActiveTabFromSnapshot return the full tab object (or at least { uid,
pathname }) and then use the returned object's pathname when dispatching addTab
(replace activeTabUid usage with the returnedTab.pathname and returnedTab.uid),
or alternatively update the dispatch to request the correct pathname field from
whatever structure getActiveTabFromSnapshot is changed to return; ensure
references to getActiveTabFromSnapshot, activeTabUid, and addTab are updated
consistently.
---
Nitpick comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Line 351: Remove the redundant inner declaration of ipcRenderer (const {
ipcRenderer } = window;) inside the function — the module already destructures
ipcRenderer at module scope, so delete the inner declaration and use the
existing ipcRenderer symbol (e.g., in the actions file where ipcRenderer is
re-declared) to avoid shadowing; run lint/format after the removal to ensure no
unused-variable warnings remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85e3b6f8-bf9d-497a-80d2-e6e1de8a32bb
📒 Files selected for processing (21)
packages/bruno-app/src/components/GlobalSearchModal/index.jspackages/bruno-app/src/components/RequestTabPanel/RequestNotFound/index.jspackages/bruno-app/src/components/RequestTabPanel/RequestTabPanelLoading/index.jspackages/bruno-app/src/components/RequestTabPanel/index.jspackages/bruno-app/src/components/RequestTabs/ExampleTab/index.jspackages/bruno-app/src/components/RequestTabs/RequestTab/RequestTabLoading.jspackages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.jspackages/bruno-app/src/components/RequestTabs/RequestTab/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/ExampleItem/index.jspackages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.jspackages/bruno-app/src/providers/ReduxStore/slices/app.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/providers/ReduxStore/slices/tabs.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-app/src/utils/snapshot/index.jspackages/bruno-electron/src/index.jspackages/bruno-electron/src/ipc/snapshot.jspackages/bruno-electron/src/services/snapshot/index.js
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
- packages/bruno-app/src/components/RequestTabs/RequestTab/RequestTabLoading.js
- packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js
- packages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.js
- packages/bruno-app/src/utils/snapshot/index.js
- packages/bruno-app/src/components/GlobalSearchModal/index.js
- packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/ExampleItem/index.js
- packages/bruno-electron/src/services/snapshot/index.js
packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js
Outdated
Show resolved
Hide resolved
Comment on lines
+2965
to
+2973
| try { | ||
| const collectionSnapshot = await ipcRenderer.invoke('renderer:get-collection-snapshot', collectionPathname); | ||
| if (collectionSnapshot?.tabs?.length > 0) { | ||
| dispatch(restoreTabs({ | ||
| collectionUid, | ||
| tabs: collectionSnapshot.tabs, | ||
| activeTab: collectionSnapshot.activeTab | ||
| })); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid double-restoring tabs during collection mount.
mountCollection now restores tabs here, but hydrateTabs already restores them during workspace hydration (packages/bruno-app/src/utils/snapshot/index.js, Lines 142-157). Because restoreTabs clears existing tabs first, the second restore can overwrite the first pass and reset tab state.
💡 Suggested guard to prevent duplicate restore pass
try {
const collectionSnapshot = await ipcRenderer.invoke('renderer:get-collection-snapshot', collectionPathname);
- if (collectionSnapshot?.tabs?.length > 0) {
+ const hasTabsForCollection = getState().tabs.tabs.some((t) => t.collectionUid === collectionUid);
+ if (!hasTabsForCollection && collectionSnapshot?.tabs?.length > 0) {
dispatch(restoreTabs({
collectionUid,
tabs: collectionSnapshot.tabs,
activeTab: collectionSnapshot.activeTab
}));
}
} catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`
around lines 2965 - 2973, mountCollection currently re-dispatches restoreTabs
even though hydrateTabs already restored tabs during workspace hydration; modify
the mountCollection flow around the
ipcRenderer.invoke('renderer:get-collection-snapshot', collectionPathname)
result so you only call dispatch(restoreTabs({ collectionUid, ... })) when tabs
have not already been hydrated for this collection — e.g., check existing tabs
in Redux state for collectionUid (or a workspaceHydrated flag) and skip calling
restoreTabs if tabs exist or hydration completed; keep the existing condition on
collectionSnapshot?.tabs?.length > 0 and add the dup-check before dispatching
restoreTabs.
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
528-540:⚠️ Potential issue | 🟠 MajorNormalize workspace paths before comparison.
Raw string equality at line 534 can fail across platforms with differing path separators or casing.
Proposed fix
try { const snapshot = await ipcRenderer.invoke('renderer:get-snapshot'); const activeWorkspacePath = snapshot?.activeWorkspacePath; if (activeWorkspacePath) { - shouldSwitch = workspacePath === activeWorkspacePath; + shouldSwitch = normalizePath(workspacePath) === normalizePath(activeWorkspacePath); } else { shouldSwitch = !activeWorkspaceUid || workspaceConfig.type === 'default'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 528 - 540, The comparison between workspacePath and snapshot.activeWorkspacePath should normalize file paths before comparing to avoid mismatches from separators or case differences; update the block around shouldSwitch/ipcRenderer.invoke('renderer:get-snapshot') to normalize both values (e.g., using path.normalize and, on case-insensitive platforms, toLowerCase) before checking equality with activeWorkspacePath, and use the same normalized form in the else/catch fallback logic that inspects workspaceConfig.type and active workspace UID (workspacePath, activeWorkspacePath, shouldSwitch, workspaceConfig, activeWorkspaceUid).packages/bruno-app/src/utils/snapshot/index.js (2)
40-42:⚠️ Potential issue | 🟡 MinorPath comparison with
startsWithmay fail on Windows.Mixed separators or casing differences could let transient tabs slip through.
Proposed fix
+import { normalizePath } from 'utils/common/path'; + export const shouldExcludeTab = (tab, transientDirectory) => { - return transientDirectory && tab.pathname?.startsWith(transientDirectory); + if (!transientDirectory || !tab.pathname) return false; + return normalizePath(tab.pathname).startsWith(normalizePath(transientDirectory)); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/snapshot/index.js` around lines 40 - 42, The current startsWith check in shouldExcludeTab can miss matches on Windows due to backslash vs forward-slash and casing differences; update shouldExcludeTab to normalize both transientDirectory and tab.pathname (e.g., use path.normalize or replace backslashes with forward slashes), ensure both have a consistent trailing separator, and on Windows perform a case-insensitive comparison (lowercase both strings) before using startsWith so transientDirectory reliably matches tab.pathname; keep references to shouldExcludeTab, transientDirectory, and tab.pathname when making the change.
10-36:⚠️ Potential issue | 🟠 Major
tabs/restoreTabsis missing fromSAVE_TRIGGERS.Restored tabs won't trigger a snapshot save. If the app crashes before another triggering action, the restored state is lost.
['tabs/makeTabPermanent', null], ['tabs/syncTabUid', null], + ['tabs/restoreTabs', null], ['tabs/updateRequestPaneTab', null],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/snapshot/index.js` around lines 10 - 36, SAVE_TRIGGERS is missing the action key for restored tabs so calling the "tabs/restoreTabs" action won't cause a snapshot save; add an entry for ['tabs/restoreTabs', null] to the Map initialization in SAVE_TRIGGERS (alongside the other 'tabs/...' keys) so that restoreTabs triggers snapshot saving, and ensure the key name matches the action dispatched elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 83-99: serializeActiveTab currently returns only exampleName for
'response-example' tabs which is ambiguous; change serializeActiveTab to return
a composite identifier for response examples (e.g., include both the
tab.exampleName and the request context such as tab.pathname or tab.requestId)
instead of just {accessor: 'exampleName', value: tab.exampleName}; then update
isActiveTab to recognize and compare that composite identifier (compare both
exampleName and pathname/requestId when accessor indicates an example) so
example selection is unique across requests. Ensure you reference
serializeActiveTab and isActiveTab when making the changes.
---
Duplicate comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 528-540: The comparison between workspacePath and
snapshot.activeWorkspacePath should normalize file paths before comparing to
avoid mismatches from separators or case differences; update the block around
shouldSwitch/ipcRenderer.invoke('renderer:get-snapshot') to normalize both
values (e.g., using path.normalize and, on case-insensitive platforms,
toLowerCase) before checking equality with activeWorkspacePath, and use the same
normalized form in the else/catch fallback logic that inspects
workspaceConfig.type and active workspace UID (workspacePath,
activeWorkspacePath, shouldSwitch, workspaceConfig, activeWorkspaceUid).
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 40-42: The current startsWith check in shouldExcludeTab can miss
matches on Windows due to backslash vs forward-slash and casing differences;
update shouldExcludeTab to normalize both transientDirectory and tab.pathname
(e.g., use path.normalize or replace backslashes with forward slashes), ensure
both have a consistent trailing separator, and on Windows perform a
case-insensitive comparison (lowercase both strings) before using startsWith so
transientDirectory reliably matches tab.pathname; keep references to
shouldExcludeTab, transientDirectory, and tab.pathname when making the change.
- Around line 10-36: SAVE_TRIGGERS is missing the action key for restored tabs
so calling the "tabs/restoreTabs" action won't cause a snapshot save; add an
entry for ['tabs/restoreTabs', null] to the Map initialization in SAVE_TRIGGERS
(alongside the other 'tabs/...' keys) so that restoreTabs triggers snapshot
saving, and ensure the key name matches the action dispatched elsewhere in the
codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d62a1ee-603f-40a9-934d-736cf6a52ce0
📒 Files selected for processing (3)
packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-app/src/utils/snapshot/index.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
547-549:⚠️ Potential issue | 🟠 MajorUse normalized path comparison when deciding
shouldSwitch.Line 548 compares raw path strings. Path separator/casing differences can prevent restoring the intended active workspace on different OS/filesystems.
🧭 Proposed fix
if (activeWorkspacePath) { - shouldSwitch = workspacePath === activeWorkspacePath; + const normalizedWorkspacePath = normalizePath(workspacePath); + const normalizedActiveWorkspacePath = normalizePath(activeWorkspacePath); + shouldSwitch = process.platform === 'win32' + ? normalizedWorkspacePath.toLowerCase() === normalizedActiveWorkspacePath.toLowerCase() + : normalizedWorkspacePath === normalizedActiveWorkspacePath; } else { shouldSwitch = !activeWorkspaceUid || workspaceConfig.type === 'default'; }As per coding guidelines, "Bruno is a cross-platform Electron desktop app ... Never assume case-sensitive or case-insensitive filesystems."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 547 - 549, The comparison setting shouldSwitch currently compares raw strings (workspacePath vs activeWorkspacePath); instead normalize and resolve both paths before comparing to handle separator/casing differences: import/require the path module, compute normalized/resolved forms for activeWorkspacePath and workspacePath (e.g., via path.resolve and path.normalize) and, for case-insensitive platforms (check process.platform === 'win32' or use os.platform()), compare their lowercased forms; update the shouldSwitch assignment in the block that references activeWorkspacePath/workspacePath so it uses these normalized comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 352-356: The code calls dispatch(setActiveWorkspace(workspaceUid))
before confirming the workspace exists, which can leave state pointing to a
non-existent workspace; update the logic in the action to first obtain the
workspace via getState().workspaces.workspaces.find((w) => w.uid ===
workspaceUid) and return early if not found, and only then call
dispatch(setActiveWorkspace(workspaceUid)); ensure you reference the same
workspaceUid variable and preserve any subsequent logic that depended on the
dispatched active workspace.
---
Duplicate comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 547-549: The comparison setting shouldSwitch currently compares
raw strings (workspacePath vs activeWorkspacePath); instead normalize and
resolve both paths before comparing to handle separator/casing differences:
import/require the path module, compute normalized/resolved forms for
activeWorkspacePath and workspacePath (e.g., via path.resolve and
path.normalize) and, for case-insensitive platforms (check process.platform ===
'win32' or use os.platform()), compare their lowercased forms; update the
shouldSwitch assignment in the block that references
activeWorkspacePath/workspacePath so it uses these normalized comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ca7032a-a49b-4051-a4a5-6ecc13d52065
📒 Files selected for processing (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
Comment on lines
352
to
+356
| dispatch(setActiveWorkspace(workspaceUid)); | ||
|
|
||
| const workspace = getState().workspaces.workspaces.find((w) => w.uid === workspaceUid); | ||
| if (!workspace) return; | ||
|
|
Contributor
There was a problem hiding this comment.
Validate workspace before mutating active workspace state.
Line 352 sets active workspace before the existence check on Line 355. If workspaceUid is stale, state can point to a non-existent workspace while the function exits early.
🔧 Proposed fix
export const switchWorkspace = (workspaceUid) => {
return async (dispatch, getState) => {
- dispatch(setActiveWorkspace(workspaceUid));
-
const workspace = getState().workspaces.workspaces.find((w) => w.uid === workspaceUid);
- if (!workspace) return;
+ if (!workspace) {
+ return;
+ }
+
+ dispatch(setActiveWorkspace(workspaceUid));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`
around lines 352 - 356, The code calls
dispatch(setActiveWorkspace(workspaceUid)) before confirming the workspace
exists, which can leave state pointing to a non-existent workspace; update the
logic in the action to first obtain the workspace via
getState().workspaces.workspaces.find((w) => w.uid === workspaceUid) and return
early if not found, and only then call
dispatch(setActiveWorkspace(workspaceUid)); ensure you reference the same
workspaceUid variable and preserve any subsequent logic that depended on the
dispatched active workspace.
- Add snapshot middleware to persist UI state (tabs, workspaces, environments) - Add SnapshotManager service in electron for atomic snapshot storage - Add accessor-based tab serialization using pathname for reliable restoration - Add loading states for tabs while collections are mounting - Add hydrateTabs to restore tabs from snapshots on app load - Add devTools state persistence (console open/height/tab)
chirag-bruno
force-pushed
the
feature/app-snapshot-manager
branch
from
d919776 to
e968184
Compare
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.
Description
Automatically saves and restores UI state across app restarts:
Key implementation details:
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Improvements