Open
Conversation
- 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)
Make serializeSnapshot async to fetch existing snapshot before saving, ensuring collections not currently loaded in Redux are preserved. Fix activeTab serialization to pass collection object instead of just UID.
… IPC Replace array-based snapshot storage with key-value maps keyed by pathname for O(1) lookups. Separate tabs into their own top-level map, decoupled from collections. - Rewrite SnapshotManager with map-based schema and granular read/write methods (getWorkspace, getTabs, setCollection, removeWorkspace, etc.) - Add 12 granular IPC handlers (renderer:snapshot:*) replacing 4 coarse ones - Update middleware serialization to produce maps; remove activeCollectionUidCache in favor of lastActiveCollectionPathname on workspace objects - Fix mountCollection passing collectionUid instead of collection to restoreTabs - Preserve non-active workspace state from existing snapshot on save
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughThis PR implements a comprehensive UI snapshot persistence system for Bruno. It introduces Redux middleware and utilities to automatically save and restore application state (tabs, workspaces, collections, environments) across Electron app restarts, along with new loading states for asynchronous tab and item resolution via pathname-based lookup. Backend support includes new IPC handlers and a SnapshotManager service. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Action
participant Redux as Redux Store
participant Middleware as Snapshot Middleware
participant IPC as IPC (Renderer)
participant Electron as Electron Main
participant Manager as SnapshotManager
participant File as Disk Storage
User->>Redux: Dispatch action (in SAVE_TRIGGERS)
Redux->>Middleware: Action received
Middleware->>Middleware: Debounce (1s)
rect rgba(100, 150, 200, 0.5)
Note over Middleware,File: Snapshot Save Flow
Middleware->>IPC: renderer:snapshot:get
IPC->>Electron: IPC call
Electron->>Manager: getSnapshot()
Manager->>File: Read existing snapshot
File-->>Manager: Current snapshot
Manager-->>Electron: Snapshot data
end
rect rgba(150, 200, 100, 0.5)
Note over Middleware,File: Serialization & Persistence
Middleware->>Middleware: Serialize workspaces/collections/tabs<br/>(apply filters, path normalization)
Middleware->>IPC: renderer:snapshot:save(payload)
IPC->>Electron: IPC call
Electron->>Manager: saveSnapshot(normalized)
Manager->>Manager: Validate & normalize
Manager->>File: Write ui-state-snapshot.json
File-->>Manager: Success
Manager-->>Electron: Boolean result
end
Electron-->>IPC: Response
IPC-->>Middleware: Resolution
Middleware-->>Redux: Save complete (or error logged)
sequenceDiagram
participant App as App Init
participant Redux as Redux
participant Workspace as Workspace Action
participant IPC as IPC (Renderer)
participant Electron as Electron Main
participant Manager as SnapshotManager
App->>Workspace: switchWorkspace()
Workspace->>Redux: setSnapshotReady(false)
rect rgba(100, 150, 200, 0.5)
Note over Workspace,Manager: Load Snapshot
Workspace->>IPC: renderer:snapshot:get-workspace
IPC->>Electron: IPC call
Electron->>Manager: getWorkspace(id)
Manager-->>Electron: Workspace snapshot
Electron-->>IPC: Response
IPC-->>Workspace: Snapshot data
end
rect rgba(150, 200, 100, 0.5)
Note over Workspace,Redux: Restore UI State
Workspace->>Workspace: Mount collections
Workspace->>Workspace: hydrateTabs (for each collection)
Workspace->>Redux: restoreTabs (per collection)
Workspace->>Redux: mountCollection
Workspace->>Redux: expandCollection
Workspace->>Redux: setActiveTab
end
Workspace->>Redux: setSnapshotReady(true)
Redux-->>App: State restored
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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new UI state snapshot pipeline to persist/restore workspace, collection, tab, and DevTools state across app restarts, including migrations from the legacy collection-array snapshot format.
Changes:
- Introduces an Electron-side
SnapshotManager(electron-store + yup schema) and IPC surface for snapshot read/write. - Adds a renderer snapshot middleware + snapshot (de)serialization helpers to persist/restore tabs, workspace selection, collection state, environments, and DevTools state.
- Updates UI/tab rendering to support pathname/exampleName-based restoration with loading placeholders, and adds Playwright coverage for snapshot persistence.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/workspace/snapshot-persistence.spec.ts | Adds end-to-end coverage for snapshot persistence/restoration scenarios. |
| packages/bruno-electron/src/store/ui-state-snapshot.js | Removes legacy UI snapshot store implementation. |
| packages/bruno-electron/src/services/snapshot/index.js | Adds centralized snapshot manager with schema validation + legacy migration. |
| packages/bruno-electron/src/ipc/snapshot.js | Adds IPC handlers for snapshot CRUD operations. |
| packages/bruno-electron/src/ipc/collection.js | Routes snapshot updates through the new snapshot manager. |
| packages/bruno-electron/src/index.js | Registers snapshot IPC on app startup. |
| packages/bruno-electron/src/app/collection-watcher.js | Hydrates renderer with snapshot-derived collection environment info. |
| packages/bruno-app/src/utils/snapshot/index.js | Adds snapshot serialization/deserialization + hydration helpers. |
| packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js | Restores workspace/collection/tab/devtools state on workspace load/switch; gates saving via snapshotReady. |
| packages/bruno-app/src/providers/ReduxStore/slices/tabs.js | Adds pathname/exampleName support, tab restoration, and example UID syncing. |
| packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js | Adds expandCollection, persists environment pathname, updates snapshot payload shape. |
| packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js | Persists env selection via environment path; hydrates tabs after mount. |
| packages/bruno-app/src/providers/ReduxStore/slices/app.js | Adds snapshotReady flag and setter action. |
| packages/bruno-app/src/providers/ReduxStore/middlewares/tasks/middleware.js | Updates example-tab opening to include pathname/exampleName. |
| packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js | Adds debounced snapshot persistence middleware. |
| packages/bruno-app/src/providers/ReduxStore/index.js | Registers snapshot middleware. |
| packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.js | Ensures opened tabs include type + pathname for restoration. |
| packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/ExampleItem/index.js | Ensures example tabs include pathname + exampleName. |
| packages/bruno-app/src/components/RequestTabs/RequestTab/index.js | Adds pathname fallback lookup and loading placeholder handling. |
| packages/bruno-app/src/components/RequestTabs/RequestTab/StyledWrapper.js | Adds tab label ellipsis styling. |
| packages/bruno-app/src/components/RequestTabs/RequestTab/RequestTabLoading.js | Adds tab placeholder component for loading/mounting collections. |
| packages/bruno-app/src/components/RequestTabs/ExampleTab/index.js | Adds pathname/exampleName fallback, placeholder state, and UID syncing. |
| packages/bruno-app/src/components/RequestTabPanel/index.js | Adds pathname/exampleName fallback rendering and panel loading placeholder. |
| packages/bruno-app/src/components/RequestTabPanel/RequestTabPanelLoading/index.js | Adds panel loading placeholder component. |
| packages/bruno-app/src/components/RequestTabPanel/RequestNotFound/index.js | Fixes delayed error UI timer cleanup. |
| packages/bruno-app/src/components/GlobalSearchModal/index.js | Ensures tabs opened from search include type + pathname. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+461
to
+467
| // electron-store uses dot notation for nested paths, so we need to escape dots in pathnames | ||
| _escapeKey(pathname) { | ||
| if (typeof pathname !== 'string') { | ||
| return ''; | ||
| } | ||
|
|
||
| return pathname.replace(/\./g, '\\.'); |
There was a problem hiding this comment.
_escapeKey only escapes dots, but electron-store (dot-prop) also treats backslashes as escape characters in key paths. If a workspace/collection pathname contains \ (common on Windows), set/get/delete can target the wrong nested key. Escape backslashes as well (e.g., replace \ with \\ before escaping dots), or normalize all path keys to POSIX (/) before storing.
Suggested change
| // electron-store uses dot notation for nested paths, so we need to escape dots in pathnames | |
| _escapeKey(pathname) { | |
| if (typeof pathname !== 'string') { | |
| return ''; | |
| } | |
| return pathname.replace(/\./g, '\\.'); | |
| // electron-store uses dot notation for nested paths, so we need to escape | |
| // backslashes first and then dots in pathnames | |
| _escapeKey(pathname) { | |
| if (typeof pathname !== 'string') { | |
| return ''; | |
| } | |
| return pathname.replace(/\\/g, '\\\\').replace(/\./g, '\\.'); |
Comment on lines
+74
to
+81
| // Build a uid → pathname map for collections (used to resolve activeCollectionUid → pathname) | ||
| const collectionUidToPathname = new Map(); | ||
| (collections.collections || []).forEach((c) => { | ||
| if (c.uid && c.pathname) { | ||
| collectionUidToPathname.set(c.uid, normalizePath(c.pathname)); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
collectionUidToPathname is created but never used. This adds noise and suggests missing logic; please remove it or use it (e.g., for resolving active collection) so it doesn’t become stale.
Suggested change
| // Build a uid → pathname map for collections (used to resolve activeCollectionUid → pathname) | |
| const collectionUidToPathname = new Map(); | |
| (collections.collections || []).forEach((c) => { | |
| if (c.uid && c.pathname) { | |
| collectionUidToPathname.set(c.uid, normalizePath(c.pathname)); | |
| } | |
| }); |
Comment on lines
+108
to
+166
| snapshot.workspaces[workspace.pathname] = { | ||
| lastActiveCollectionPathname, | ||
| sorting: 'az', | ||
| collectionPathnames: workspaceCollectionPaths | ||
| }; | ||
| }); | ||
|
|
||
| (collections.collections || []).forEach((collection) => { | ||
| // Skip scratch collections and collections without pathname | ||
| if (!collection.pathname || scratchCollectionUids.has(collection.uid)) { | ||
| return; | ||
| } | ||
|
|
||
| const normalizedPath = normalizePath(collection.pathname); | ||
|
|
||
| // Persist tab state only for the active workspace's collections. | ||
| // For non-active workspaces, preserve the last persisted snapshot entries. | ||
| if (activeWorkspace && !activeWorkspaceCollectionPaths.has(normalizedPath)) { | ||
| return; | ||
| } | ||
|
|
||
| serializedCollectionPaths.add(normalizedPath); | ||
|
|
||
| // Find which workspace this collection belongs to | ||
| const workspacePathname = Object.keys(snapshot.workspaces).find((wp) => { | ||
| const ws = snapshot.workspaces[wp]; | ||
| return ws.collectionPathnames.some((cp) => normalizePath(cp) === normalizedPath); | ||
| }) || ''; | ||
|
|
||
| snapshot.collections[collection.pathname] = { | ||
| workspacePathname, | ||
| environment: { | ||
| collection: getCollectionEnvironmentPath( | ||
| collection, | ||
| (collection.environments || []).find((env) => env.uid === collection.activeEnvironmentUid), | ||
| '' | ||
| ), | ||
| global: globalEnvironments.activeGlobalEnvironmentUid || '' | ||
| }, | ||
| isOpen: !collection.collapsed, | ||
| isMounted: collection.mountStatus === 'mounted' | ||
| }; | ||
|
|
||
| // Get transient directory for this collection to filter transient tabs | ||
| const transientDirectory = collections.tempDirectories?.[collection.uid]; | ||
|
|
||
| // Filter and serialize tabs, excluding transient requests | ||
| const collectionTabs = (tabs.tabs || []) | ||
| .filter((t) => t.collectionUid === collection.uid && !shouldExcludeTab(t, transientDirectory)) | ||
| .map((t) => serializeTab(t, collection)); | ||
|
|
||
| const activeTabInCollection = (tabs.tabs || []).find( | ||
| (t) => t.collectionUid === collection.uid && t.uid === tabs.activeTabUid && !shouldExcludeTab(t, transientDirectory) | ||
| ); | ||
|
|
||
| snapshot.tabs[collection.pathname] = { | ||
| activeTab: serializeActiveTab(activeTabInCollection, collection), | ||
| tabs: collectionTabs | ||
| }; |
There was a problem hiding this comment.
Snapshot maps are keyed with the raw collection.pathname / workspace.pathname, while comparisons elsewhere normalize paths. On Windows or mixed path separator scenarios this can create duplicate keys (e.g. C:\x vs C:/x) and make hydration/migration inconsistent. Consider using normalizePath(...) for the keys you write into snapshot.workspaces, snapshot.collections, and snapshot.tabs (and keep values normalized as well).
Comment on lines
+199
to
+206
| name: snapshotTab.name || null, | ||
| pathname: pathname || null, | ||
| requestPaneTab: snapshotTab.request?.tab || 'params', | ||
| requestPaneWidth: snapshotTab.request?.width || null, | ||
| requestPaneHeight: snapshotTab.request?.height || null, | ||
| responsePaneTab: snapshotTab.response?.tab || 'response', | ||
| responseFormat: snapshotTab.response?.format || null, | ||
| responseViewTab: snapshotTab.response?.viewTab || null, |
There was a problem hiding this comment.
deserializeTab uses || when defaulting numeric snapshot values (e.g., requestPaneWidth, requestPaneHeight). This will treat 0 as falsy and overwrite it with null. Use nullish coalescing (??) for these fields (and other snapshot fields where empty string/0 are valid) so persisted values round-trip correctly.
Suggested change
| name: snapshotTab.name || null, | |
| pathname: pathname || null, | |
| requestPaneTab: snapshotTab.request?.tab || 'params', | |
| requestPaneWidth: snapshotTab.request?.width || null, | |
| requestPaneHeight: snapshotTab.request?.height || null, | |
| responsePaneTab: snapshotTab.response?.tab || 'response', | |
| responseFormat: snapshotTab.response?.format || null, | |
| responseViewTab: snapshotTab.response?.viewTab || null, | |
| name: snapshotTab.name ?? null, | |
| pathname: pathname ?? null, | |
| requestPaneTab: snapshotTab.request?.tab ?? 'params', | |
| requestPaneWidth: snapshotTab.request?.width ?? null, | |
| requestPaneHeight: snapshotTab.request?.height ?? null, | |
| responsePaneTab: snapshotTab.response?.tab ?? 'response', | |
| responseFormat: snapshotTab.response?.format ?? null, | |
| responseViewTab: snapshotTab.response?.viewTab ?? null, |
| import { findItemInCollection } from 'utils/collections'; | ||
| import { sendRequest } from 'providers/ReduxStore/slices/collections/actions'; | ||
| import { findItemInCollection, findItemInCollectionByPathname, areItemsLoading } from 'utils/collections'; | ||
| import { cancelRequest, sendRequest } from 'providers/ReduxStore/slices/collections/actions'; |
There was a problem hiding this comment.
cancelRequest is imported but not used in this module. If the intention is to add cancel support, wire it up; otherwise remove the import to avoid lint failures/noise.
Suggested change
| import { cancelRequest, sendRequest } from 'providers/ReduxStore/slices/collections/actions'; | |
| import { sendRequest } from 'providers/ReduxStore/slices/collections/actions'; |
![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: 6
🧹 Nitpick comments (9)
packages/bruno-electron/src/ipc/collection.js (1)
1632-1638: Nit: rethrowing loses the original stack.
throw new Error(error.message)strips the original error's stack trace, which makes debugging snapshot failures harder. Consider rethrowingerrordirectly (or usingPromise.reject(error)to stay consistent with the other handlers in this file).♻️ Suggested change
ipcMain.handle('renderer:update-ui-state-snapshot', (event, { type, data }) => { try { snapshotManager.update({ type, data }); } catch (error) { - throw new Error(error.message); + return Promise.reject(error); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/collection.js` around lines 1632 - 1638, The IPC handler for 'renderer:update-ui-state-snapshot' currently catches errors and rethrows a new Error which loses the original stack; change the catch to rethrow the original error (e.g., throw error) or return Promise.reject(error) so the original stack and error details from snapshotManager.update are preserved, updating the handler that wraps snapshotManager.update({ type, data }) accordingly.packages/bruno-app/src/utils/snapshot/index.js (3)
82-98: Matching a normalized path againstuidandnameconflates identifiers.Lines 89 and 93 reuse
normalizedEnvironmentPath(a pathname with trailing-slash stripped and backslashes flipped) as the comparison key for bothenvironment.uidandenvironment.name. A real pathname like/users/foo/envs/dev.bruwill never equal a uid or a name, so these branches only fire when the caller passed a non-path (e.g., a raw name) viaenvironmentPath. That's a semantic smell — either the field is a path or it's an identifier.Either (a) accept a separate
environmentUid/environmentNameinsnapshotDataand match each against its corresponding field, or (b) document thatenvironmentPathis deliberately a "best-effort reference" and rename it accordingly. Today's behavior works but is easy to break on future refactors.🤖 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 82 - 98, The current matcher conflates path vs identifier by comparing normalizedEnvironmentPath (from normalizeSnapshotPathRef(environment?.pathname)) against environment.uid and environment.name; change the logic to accept and check distinct keys: use snapshotData.environmentPath (normalized via normalizeSnapshotPathRef on environment?.pathname) only to compare against environment?.pathname, and add/accept snapshotData.environmentUid and snapshotData.environmentName and compare those against environment?.uid and environment?.name respectively inside the collection.environments.find callback (fall back to selectedEnvironment for legacy name matching if needed); update any callers to pass environmentUid/environmentName when available or keep environmentPath for path-only matching and add a clear comment on the new behavior.
15-40:SAVE_TRIGGERSshould be aSet, not aMap<string, null>.No value is ever read from the map — every entry is
nulland the only access pattern will beSAVE_TRIGGERS.has(action.type). ASet<string>conveys intent, halves the memory footprint, and avoids misleading future readers into thinking thenullslot is meaningful.♻️ Proposed refactor
-export const SAVE_TRIGGERS = new Map([ - ['app/setSnapshotReady', null], - ['tabs/addTab', null], +export const SAVE_TRIGGERS = new Set([ + 'app/setSnapshotReady', + 'tabs/addTab', ... - ['logs/setActiveTab', null] -]); + '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 15 - 40, SAVE_TRIGGERS is defined as a Map filled with string keys and null values but only ever used with SAVE_TRIGGERS.has(action.type); change it to a Set<string> to convey intent and reduce memory: replace the Map construction with a Set containing the same action type strings (keep the same symbol SAVE_TRIGGERS) and ensure all current usages (e.g., SAVE_TRIGGERS.has(...)) continue to work unchanged.
201-209: Default pane values applied unconditionally — dead fields on non-request tabs.
requestPaneTab: 'params',responsePaneTab: 'response', etc. are set for every tab type, including singletons likevariablesorcollection-runnerwhereserializeTabnever writesrequest/responseblocks. Harmless today, but it inflates the tab object and muddies the shape. Consider gating these defaults behindisRequestTab(type):♻️ Proposed refactor
const tab = { collectionUid: collection.uid, type, preview: !snapshotTab.permanent, name: snapshotTab.name || null, - pathname: pathname || null, - requestPaneTab: snapshotTab.request?.tab || 'params', - requestPaneWidth: snapshotTab.request?.width || null, - requestPaneHeight: snapshotTab.request?.height || null, - responsePaneTab: snapshotTab.response?.tab || 'response', - responseFormat: snapshotTab.response?.format || null, - responseViewTab: snapshotTab.response?.viewTab || null, - responsePaneScrollPosition: null, - scriptPaneTab: null + pathname: pathname || null, + ...(isRequestTab(type) ? { + requestPaneTab: snapshotTab.request?.tab || 'params', + requestPaneWidth: snapshotTab.request?.width || null, + requestPaneHeight: snapshotTab.request?.height || null, + responsePaneTab: snapshotTab.response?.tab || 'response', + responseFormat: snapshotTab.response?.format || null, + responseViewTab: snapshotTab.response?.viewTab || null, + responsePaneScrollPosition: null, + scriptPaneTab: 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 201 - 209, The code unconditionally assigns request/response pane defaults (requestPaneTab, requestPaneWidth, requestPaneHeight, responsePaneTab, responseFormat, responseViewTab) from snapshotTab even for non-request tabs, producing dead fields; change the object construction so these properties are only added when the tab is a request-type (use or add an isRequestTab(type) check) or when snapshotTab.request/response exist (refer to snapshotTab, requestPaneTab, responsePaneTab and serializeTab in your change), i.e., wrap or conditionally spread the request/response defaults into the returned tab snapshot only when isRequestTab(type) is true so non-request singletons like variables/collection-runner keep a minimal shape.tests/workspace/snapshot-persistence.spec.ts (5)
179-182: Request-pane active-tab assertion couples to layout classes.
page2.locator('.request-pane > .px-4')(Line 179) and the/active/class regex (Line 182) depend on Tailwind spacing (px-4) and an internal className pattern. A Tailwind v4 migration or any style refactor will silently break this test. Route throughgetByTestId+getByRole('tab', { selected: true })ortoHaveAttribute('aria-selected', 'true')instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workspace/snapshot-persistence.spec.ts` around lines 179 - 182, The test is brittle due to CSS-dependent selectors and class checks: replace the DOM queries that use page2.locator('.request-pane > .px-4') and the class-regex check on headersTab with semantic/test-id and ARIA assertions; locate the request pane via a test id (e.g., getByTestId('request-pane') or similar), find the tab using getByRole('tab', { name: 'Headers' }) and assert selected state via getByRole(..., { selected: true }) or toHaveAttribute('aria-selected', 'true') on the headersTab element instead of checking for the /active/ class.
28-34: Per-test boilerplate duplication — extract alaunchAndWaitForLoadedhelper.The three-liner
launchElectronApp→firstWindow→waitFor('[data-app-state="loaded"]', 30000)shows up 14 times in this file. A single helper (e.g.,await launchLoadedApp({ userDataPath })returning{ app, page }) would cut ~40 lines and make the tests read like specs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workspace/snapshot-persistence.spec.ts` around lines 28 - 34, Extract the repeated three-line startup sequence into a helper (e.g., launchLoadedApp) so tests call something like await launchLoadedApp({ userDataPath }) and get back { app, page }; specifically, factor out the launchElectronApp call, app.firstWindow(), and await page.locator('[data-app-state="loaded"]').waitFor({ timeout: 30000 }) into a single async function (referencing launchElectronApp, firstWindow, and the locator waitFor) and replace the 14 duplicated occurrences in the spec with calls to that helper to reduce boilerplate and improve readability.
61-62: Brittle DOM selectors — preferdata-testid.
.request-tab .tab-label(Line 61),.console-header/.console-tab(Lines 448, 450, 467, 470),.dropdown-item(Lines 220, 278, 392),.request-pane > .px-4(Line 179) are all CSS/Tailwind-class selectors that will break on any styling refactor. Adddata-testidhooks on the corresponding components and usegetByTestIdhere.As per coding guidelines: "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workspace/snapshot-persistence.spec.ts` around lines 61 - 62, Replace brittle CSS/Tailwind selectors with data-testid hooks: add descriptive data-testid attributes to the UI components that render the elements currently targeted by selectors '.request-tab .tab-label', '.console-header', '.console-tab', '.dropdown-item', and '.request-pane > .px-4'; then update the tests (e.g., where you call page2.locator('.request-tab .tab-label') and other occurrences) to use Playwright's getByTestId or locator('[data-testid="..."]') with those test IDs instead of class-based selectors so tests no longer break on styling refactors.
195-205: DuplicateWORKSPACE_YML+ dialog-stub block across 3 tests.The same 10-line YAML literal and the
dialog.showOpenDialogmonkey-patch repeat verbatim at Lines 195–221, 247–279, and 361–393. Extract a shared helper (e.g.,createWorkspaceFixture(createTmpDir, name)andopenWorkspaceViaDialog(app, page, path)) intests/utils/page/to keep these tests focused on the actual assertions.As per coding guidelines: "Use consistent patterns and helper utilities where they improve clarity. Prefer shared test utilities over copy-pasted setup code, but only when it actually reduces complexity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workspace/snapshot-persistence.spec.ts` around lines 195 - 205, The tests duplicate the same WORKSPACE_YML literal and dialog.showOpenDialog monkey-patch across multiple specs; create a shared helper in tests/utils/page/, e.g., add createWorkspaceFixture(createTmpDir, name) that writes WORKSPACE_YML (or builds it from name) to the tmp dir and returns the path, and add openWorkspaceViaDialog(app, page, workspacePath) that applies the dialog.showOpenDialog monkey-patch and triggers the UI open action; then replace the repeated blocks in the spec files with calls to createWorkspaceFixture and openWorkspaceViaDialog so WORKSPACE_YML and the dialog stub live only in the new helpers.
44-48: Debounce flushes rely on fixedwaitForTimeout(2000)— fragile and slow.This pattern repeats 7+ times across the suite (Lines 46, 92, 129, 163, 226, 290, 300, 334, 402, 457, 536). If the debounce window ever grows, every test flakes; if it shrinks, we're wasting CI minutes. Per coding guidelines,
page.waitForTimeoutshould be avoided unless unavoidable.Prefer a deterministic signal — e.g., have the renderer flip a
data-snapshot-savedattribute (or emit an IPC ack) after each flush, thenawait expect(page.locator('[data-snapshot-state="idle"]')).toBeVisible(). Fallback: pollreadSnapshot(userDataPath)for the expected mutation with Playwright'sexpect.poll.As per coding guidelines: "Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/workspace/snapshot-persistence.spec.ts` around lines 44 - 48, The test uses a fragile fixed delay via page.waitForTimeout(2000) before calling closeElectronApp(app); replace this with a deterministic wait for the snapshot-flush signal: have the renderer set a DOM attribute (e.g., data-snapshot-saved or data-snapshot-state="idle") or emit an IPC ack when the debounce flush completes, then in the test await that signal (e.g., await expect(page.locator('[data-snapshot-state="idle"]')).toBeVisible()) instead of waitForTimeout; alternatively use Playwright's expect.poll to poll readSnapshot(userDataPath) for the expected mutation before calling closeElectronApp(app) — update all occurrences of page.waitForTimeout in this spec to use one of these deterministic approaches.
🤖 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/collections/actions.js`:
- Around line 3008-3015: Move the call to hydrateCollectionTabs so tabs are
restored before any snapshot-triggering mount state is saved: call
hydrateCollectionTabs(collection, dispatch, restoreTabs) (using the same
collection detection logic) before dispatching updateCollectionMountStatus and
addTransientDirectory, because updateCollectionMountStatus triggers a snapshot
save and could persist a mounted collection with no tabs if restoreTabs runs
after the debounce window.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js`:
- Around line 2923-2933: The code derives the environment file extension using
collection?.brunoConfig?.version which is unreliable; change the discriminator
to use collection.format or collection?.brunoConfig?.opencollection to determine
whether to use 'bru' or 'yml' (e.g., treat opencollection or format === 'open'
as YAML/open collection -> 'yml', else 'bru'), update the const extension and
ensure the subsequent environmentPath and the getCollectionEnvironmentPath call
use that corrected logic (symbols: extension, environmentPath,
getCollectionEnvironmentPath, collection?.brunoConfig).
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 230-241: The current hydrateCollectionTabs function swallows IPC
errors via .catch(() => null); replace that catch with one that logs the error
and collection context (e.g., capture the thrown error from
ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname) and call
console.error or your app logger with a message like "Failed to get tabs
snapshot for" plus collection.pathname and the error, then return null) so
failures are visible; apply the same pattern to the other similar IPC
invocation(s) in this file (the other .catch(() => null) occurrence) and keep
the existing dispatch(restoreTabs({...})) flow unchanged.
In `@packages/bruno-electron/src/services/snapshot/index.js`:
- Around line 416-458: _getByPathWithNormalizedFallback and
_deleteByPathWithNormalizedFallback only normalize separators but still compare
case-sensitively; update the matching logic to compare normalized paths in a
case-insensitive way by lowercasing both sides (e.g. const normalizedPath =
path.normalize(pathname).toLowerCase(); and compare with
path.normalize(key).toLowerCase()). Keep the rest of the flow the same (use
this._escapeKey for store keys and this.store.get/delete for rootKey and
matchingKey) so lookups and deletes will match entries that only differ by case.
In `@tests/workspace/snapshot-persistence.spec.ts`:
- Around line 505-512: The test currently only checks that workspace-name
renders and may pass even if SnapshotManager didn't rewrite the corrupt file;
after launching the app (launchElectronApp / page.waitFor), call the
readSnapshot helper for the ui-state-snapshot.json (e.g.,
readSnapshot(userDataPath)) and assert it returns a valid object containing the
expected top-level keys used by SnapshotManager (for example the keys present in
the default snapshot structure), ensuring the corrupt file was rewritten rather
than simply ignored; reference SnapshotManager and ui-state-snapshot.json to
locate the related behavior.
---
Nitpick comments:
In `@packages/bruno-app/src/utils/snapshot/index.js`:
- Around line 82-98: The current matcher conflates path vs identifier by
comparing normalizedEnvironmentPath (from
normalizeSnapshotPathRef(environment?.pathname)) against environment.uid and
environment.name; change the logic to accept and check distinct keys: use
snapshotData.environmentPath (normalized via normalizeSnapshotPathRef on
environment?.pathname) only to compare against environment?.pathname, and
add/accept snapshotData.environmentUid and snapshotData.environmentName and
compare those against environment?.uid and environment?.name respectively inside
the collection.environments.find callback (fall back to selectedEnvironment for
legacy name matching if needed); update any callers to pass
environmentUid/environmentName when available or keep environmentPath for
path-only matching and add a clear comment on the new behavior.
- Around line 15-40: SAVE_TRIGGERS is defined as a Map filled with string keys
and null values but only ever used with SAVE_TRIGGERS.has(action.type); change
it to a Set<string> to convey intent and reduce memory: replace the Map
construction with a Set containing the same action type strings (keep the same
symbol SAVE_TRIGGERS) and ensure all current usages (e.g.,
SAVE_TRIGGERS.has(...)) continue to work unchanged.
- Around line 201-209: The code unconditionally assigns request/response pane
defaults (requestPaneTab, requestPaneWidth, requestPaneHeight, responsePaneTab,
responseFormat, responseViewTab) from snapshotTab even for non-request tabs,
producing dead fields; change the object construction so these properties are
only added when the tab is a request-type (use or add an isRequestTab(type)
check) or when snapshotTab.request/response exist (refer to snapshotTab,
requestPaneTab, responsePaneTab and serializeTab in your change), i.e., wrap or
conditionally spread the request/response defaults into the returned tab
snapshot only when isRequestTab(type) is true so non-request singletons like
variables/collection-runner keep a minimal shape.
In `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 1632-1638: The IPC handler for 'renderer:update-ui-state-snapshot'
currently catches errors and rethrows a new Error which loses the original
stack; change the catch to rethrow the original error (e.g., throw error) or
return Promise.reject(error) so the original stack and error details from
snapshotManager.update are preserved, updating the handler that wraps
snapshotManager.update({ type, data }) accordingly.
In `@tests/workspace/snapshot-persistence.spec.ts`:
- Around line 179-182: The test is brittle due to CSS-dependent selectors and
class checks: replace the DOM queries that use page2.locator('.request-pane >
.px-4') and the class-regex check on headersTab with semantic/test-id and ARIA
assertions; locate the request pane via a test id (e.g.,
getByTestId('request-pane') or similar), find the tab using getByRole('tab', {
name: 'Headers' }) and assert selected state via getByRole(..., { selected: true
}) or toHaveAttribute('aria-selected', 'true') on the headersTab element instead
of checking for the /active/ class.
- Around line 28-34: Extract the repeated three-line startup sequence into a
helper (e.g., launchLoadedApp) so tests call something like await
launchLoadedApp({ userDataPath }) and get back { app, page }; specifically,
factor out the launchElectronApp call, app.firstWindow(), and await
page.locator('[data-app-state="loaded"]').waitFor({ timeout: 30000 }) into a
single async function (referencing launchElectronApp, firstWindow, and the
locator waitFor) and replace the 14 duplicated occurrences in the spec with
calls to that helper to reduce boilerplate and improve readability.
- Around line 61-62: Replace brittle CSS/Tailwind selectors with data-testid
hooks: add descriptive data-testid attributes to the UI components that render
the elements currently targeted by selectors '.request-tab .tab-label',
'.console-header', '.console-tab', '.dropdown-item', and '.request-pane >
.px-4'; then update the tests (e.g., where you call page2.locator('.request-tab
.tab-label') and other occurrences) to use Playwright's getByTestId or
locator('[data-testid="..."]') with those test IDs instead of class-based
selectors so tests no longer break on styling refactors.
- Around line 195-205: The tests duplicate the same WORKSPACE_YML literal and
dialog.showOpenDialog monkey-patch across multiple specs; create a shared helper
in tests/utils/page/, e.g., add createWorkspaceFixture(createTmpDir, name) that
writes WORKSPACE_YML (or builds it from name) to the tmp dir and returns the
path, and add openWorkspaceViaDialog(app, page, workspacePath) that applies the
dialog.showOpenDialog monkey-patch and triggers the UI open action; then replace
the repeated blocks in the spec files with calls to createWorkspaceFixture and
openWorkspaceViaDialog so WORKSPACE_YML and the dialog stub live only in the new
helpers.
- Around line 44-48: The test uses a fragile fixed delay via
page.waitForTimeout(2000) before calling closeElectronApp(app); replace this
with a deterministic wait for the snapshot-flush signal: have the renderer set a
DOM attribute (e.g., data-snapshot-saved or data-snapshot-state="idle") or emit
an IPC ack when the debounce flush completes, then in the test await that signal
(e.g., await expect(page.locator('[data-snapshot-state="idle"]')).toBeVisible())
instead of waitForTimeout; alternatively use Playwright's expect.poll to poll
readSnapshot(userDataPath) for the expected mutation before calling
closeElectronApp(app) — update all occurrences of page.waitForTimeout in this
spec to use one of these deterministic approaches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5584c3e-cb27-47a0-87c3-ea4f14142b5b
📒 Files selected for processing (26)
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/middlewares/tasks/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/app/collection-watcher.jspackages/bruno-electron/src/index.jspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/ipc/snapshot.jspackages/bruno-electron/src/services/snapshot/index.jspackages/bruno-electron/src/store/ui-state-snapshot.jstests/workspace/snapshot-persistence.spec.ts
💤 Files with no reviewable changes (1)
- packages/bruno-electron/src/store/ui-state-snapshot.js
Comment on lines
+3008
to
+3015
| .then(async (transientDirPath) => { | ||
| dispatch(updateCollectionMountStatus({ collectionUid, mountStatus: 'mounted' })); | ||
| dispatch(addTransientDirectory({ collectionUid, pathname: transientDirPath })); | ||
|
|
||
| const collection = getState().collections.collections.find((c) => c.uid === collectionUid); | ||
| if (collection?.pathname) { | ||
| await hydrateCollectionTabs(collection, dispatch, restoreTabs); | ||
| } |
Contributor
There was a problem hiding this comment.
Hydrate restored tabs before marking the collection as mounted.
updateCollectionMountStatus({ mountStatus: 'mounted' }) is itself a snapshot save trigger, but hydrateCollectionTabs() runs after it and tabs/restoreTabs is not a save trigger in the provided snapshot trigger list. If the IPC read is slow enough to outlive the debounce, the middleware can persist this collection as “mounted with no tabs” and overwrite the previous tab snapshot.
Suggested direction
callIpc('renderer:mount-collection', { collectionUid, collectionPathname, brunoConfig })
.then(async (transientDirPath) => {
- dispatch(updateCollectionMountStatus({ collectionUid, mountStatus: 'mounted' }));
dispatch(addTransientDirectory({ collectionUid, pathname: transientDirPath }));
const collection = getState().collections.collections.find((c) => c.uid === collectionUid);
if (collection?.pathname) {
await hydrateCollectionTabs(collection, dispatch, restoreTabs);
}
+
+ dispatch(updateCollectionMountStatus({ collectionUid, mountStatus: 'mounted' }));
})🤖 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 3008 - 3015, Move the call to hydrateCollectionTabs so tabs are
restored before any snapshot-triggering mount state is saved: call
hydrateCollectionTabs(collection, dispatch, restoreTabs) (using the same
collection detection logic) before dispatching updateCollectionMountStatus and
addTransientDirectory, because updateCollectionMountStatus triggers a snapshot
save and could persist a mounted collection with no tabs if restoreTabs runs
after the debounce window.
Comment on lines
+2923
to
+2933
| const extension = collection?.brunoConfig?.version === '1' ? 'bru' : 'yml'; | ||
| const environmentPath = environment?.pathname | ||
| || (environment?.name && collection?.pathname | ||
| ? path.join(collection.pathname, 'environments', `${environment.name}.${extension}`) | ||
| : null); | ||
|
|
||
| ipcRenderer.invoke('renderer:update-ui-state-snapshot', { | ||
| type: 'COLLECTION_ENVIRONMENT', | ||
| data: { collectionPath: collection?.pathname, environmentName: environment.name } | ||
| data: { | ||
| collectionPath: collection?.pathname, | ||
| environmentPath: getCollectionEnvironmentPath(collection, environment, environmentPath) |
Contributor
There was a problem hiding this comment.
Don't derive the snapshot env extension from brunoConfig.version.
This can persist a .bru path for YAML/OpenCollection workspaces that also carry version, so the restored environment lookup misses the real file after restart. Use collection.format or brunoConfig.opencollection as the discriminator here.
Suggested fix
- const extension = collection?.brunoConfig?.version === '1' ? 'bru' : 'yml';
+ const extension = collection?.format === 'yml' || collection?.brunoConfig?.opencollection ? 'yml' : 'bru';
const environmentPath = environment?.pathname
|| (environment?.name && collection?.pathname
? path.join(collection.pathname, 'environments', `${environment.name}.${extension}`)
: null);
ipcRenderer.invoke('renderer:update-ui-state-snapshot', {
type: 'COLLECTION_ENVIRONMENT',
data: {
collectionPath: collection?.pathname,
- environmentPath: getCollectionEnvironmentPath(collection, environment, environmentPath)
+ environmentPath
}
});Based on learnings, the repo uses opencollection to determine collection format; version is not reliable for that check.
🤖 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/index.js`
around lines 2923 - 2933, The code derives the environment file extension using
collection?.brunoConfig?.version which is unreliable; change the discriminator
to use collection.format or collection?.brunoConfig?.opencollection to determine
whether to use 'bru' or 'yml' (e.g., treat opencollection or format === 'open'
as YAML/open collection -> 'yml', else 'bru'), update the const extension and
ensure the subsequent environmentPath and the getCollectionEnvironmentPath call
use that corrected logic (symbols: extension, environmentPath,
getCollectionEnvironmentPath, collection?.brunoConfig).
Comment on lines
+230
to
+241
| export const hydrateCollectionTabs = async (collection, dispatch, restoreTabs) => { | ||
| const { ipcRenderer } = window; | ||
|
|
||
| const tabsSnapshot = await ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname).catch(() => null); | ||
| if (tabsSnapshot?.tabs?.length > 0) { | ||
| dispatch(restoreTabs({ | ||
| collection, | ||
| tabs: tabsSnapshot.tabs, | ||
| activeTab: tabsSnapshot.activeTab | ||
| })); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Silent IPC failure — hydration errors disappear into the void.
.catch(() => null) at Line 233 (and Line 252) turns any renderer:snapshot:get-tabs failure into a no-op with zero telemetry. If hydration starts failing in the wild (malformed snapshot, schema drift, IPC not registered), users will see "my tabs are gone" with nothing to correlate in logs. At minimum, log the error — ideally with the collection pathname for context.
🛡️ Proposed fix
- const tabsSnapshot = await ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname).catch(() => null);
+ const tabsSnapshot = await ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname).catch((err) => {
+ console.error(`[snapshot] Failed to hydrate tabs for ${collection.pathname}:`, err);
+ return null;
+ });📝 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 hydrateCollectionTabs = async (collection, dispatch, restoreTabs) => { | |
| const { ipcRenderer } = window; | |
| const tabsSnapshot = await ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname).catch(() => null); | |
| if (tabsSnapshot?.tabs?.length > 0) { | |
| dispatch(restoreTabs({ | |
| collection, | |
| tabs: tabsSnapshot.tabs, | |
| activeTab: tabsSnapshot.activeTab | |
| })); | |
| } | |
| }; | |
| export const hydrateCollectionTabs = async (collection, dispatch, restoreTabs) => { | |
| const { ipcRenderer } = window; | |
| const tabsSnapshot = await ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname).catch((err) => { | |
| console.error(`[snapshot] Failed to hydrate tabs for ${collection.pathname}:`, err); | |
| return null; | |
| }); | |
| if (tabsSnapshot?.tabs?.length > 0) { | |
| dispatch(restoreTabs({ | |
| collection, | |
| tabs: tabsSnapshot.tabs, | |
| activeTab: tabsSnapshot.activeTab | |
| })); | |
| } | |
| }; |
🤖 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 230 - 241, The
current hydrateCollectionTabs function swallows IPC errors via .catch(() =>
null); replace that catch with one that logs the error and collection context
(e.g., capture the thrown error from
ipcRenderer.invoke('renderer:snapshot:get-tabs', collection.pathname) and call
console.error or your app logger with a message like "Failed to get tabs
snapshot for" plus collection.pathname and the error, then return null) so
failures are visible; apply the same pattern to the other similar IPC
invocation(s) in this file (the other .catch(() => null) occurrence) and keep
the existing dispatch(restoreTabs({...})) flow unchanged.
Comment on lines
+283
to
+323
| _normalizeCollectionMap(collections) { | ||
| if (Array.isArray(collections)) { | ||
| return this._migrateLegacyCollectionsArray(collections); | ||
| } | ||
|
|
||
| if (!isObject(collections)) { | ||
| return {}; | ||
| } | ||
|
|
||
| return Object.entries(collections).reduce((acc, [collectionPath, collection]) => { | ||
| if (!collectionPath || !isObject(collection)) { | ||
| return acc; | ||
| } | ||
|
|
||
| acc[collectionPath] = this._normalizeCollectionEntry(collectionPath, collection); | ||
| return acc; | ||
| }, {}); | ||
| } | ||
|
|
||
| _migrateLegacyCollectionsArray(collectionsArray = []) { | ||
| return collectionsArray.reduce((acc, collection) => { | ||
| if (!isObject(collection) || typeof collection.pathname !== 'string') { | ||
| return acc; | ||
| } | ||
|
|
||
| const collectionPath = collection.pathname; | ||
| const environmentRef = collection.environmentPath ?? collection.selectedEnvironment; | ||
|
|
||
| acc[collectionPath] = this._normalizeCollectionEntry(collectionPath, { | ||
| workspacePathname: '', | ||
| environment: { | ||
| collection: environmentRef, | ||
| global: '' | ||
| }, | ||
| isOpen: false, | ||
| isMounted: false | ||
| }); | ||
|
|
||
| return acc; | ||
| }, {}); | ||
| } |
Contributor
There was a problem hiding this comment.
Legacy snapshot migration drops persisted tabs.
_migrateLegacyCollectionsArray() only reconstructs the collections map, while _normalizeTabsMap() only reads the new top-level tabs structure. On first load of an old array-based snapshot, collection metadata survives but the previously persisted tab list is discarded instead of being migrated.
Also applies to: 382-398
Comment on lines
+416
to
+458
| _getByPathWithNormalizedFallback(rootKey, pathname) { | ||
| if (typeof pathname !== 'string' || !pathname) { | ||
| return null; | ||
| } | ||
|
|
||
| const entry = this.store.get(`${rootKey}.${this._escapeKey(pathname)}`, undefined); | ||
| if (entry !== undefined) { | ||
| return entry; | ||
| } | ||
|
|
||
| const rootValue = this.store.get(rootKey, {}); | ||
| if (!isObject(rootValue)) { | ||
| return null; | ||
| } | ||
|
|
||
| const normalizedPath = path.normalize(pathname); | ||
| const matchingKey = Object.keys(rootValue).find((key) => path.normalize(key) === normalizedPath); | ||
|
|
||
| if (!matchingKey) { | ||
| return null; | ||
| } | ||
|
|
||
| return rootValue[matchingKey] ?? null; | ||
| } | ||
|
|
||
| _deleteByPathWithNormalizedFallback(rootKey, pathname) { | ||
| if (typeof pathname !== 'string' || !pathname) { | ||
| return; | ||
| } | ||
|
|
||
| this.store.delete(`${rootKey}.${this._escapeKey(pathname)}`); | ||
|
|
||
| const rootValue = this.store.get(rootKey, {}); | ||
| if (!isObject(rootValue)) { | ||
| return; | ||
| } | ||
|
|
||
| const normalizedPath = path.normalize(pathname); | ||
| const matchingKey = Object.keys(rootValue).find((key) => key !== pathname && path.normalize(key) === normalizedPath); | ||
|
|
||
| if (matchingKey) { | ||
| this.store.delete(`${rootKey}.${this._escapeKey(matchingKey)}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Normalized path fallback is still case-sensitive.
path.normalize(key) === normalizedPath fixes separators, but it still treats paths that differ only by casing as different entries. On Windows that can make snapshot lookups/deletes miss the stored workspace or collection, or leave duplicate entries behind after the same path is reopened with different casing. As per coding guidelines, "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-electron/src/services/snapshot/index.js` around lines 416 -
458, _getByPathWithNormalizedFallback and _deleteByPathWithNormalizedFallback
only normalize separators but still compare case-sensitively; update the
matching logic to compare normalized paths in a case-insensitive way by
lowercasing both sides (e.g. const normalizedPath =
path.normalize(pathname).toLowerCase(); and compare with
path.normalize(key).toLowerCase()). Keep the rest of the flow the same (use
this._escapeKey for store keys and this.store.get/delete for rootKey and
matchingKey) so lookups and deletes will match entries that only differ by case.
Comment on lines
+505
to
+512
| fs.writeFileSync(snapshotPath, '{ invalid json !!!', 'utf-8'); | ||
|
|
||
| const app = await launchElectronApp({ userDataPath }); | ||
| const page = await app.firstWindow(); | ||
| await page.locator('[data-app-state="loaded"]').waitFor({ timeout: 30000 }); | ||
|
|
||
| // App should recover and show default workspace | ||
| await expect(page.getByTestId('workspace-name')).toBeVisible({ timeout: 10000 }); |
Contributor
There was a problem hiding this comment.
Corrupt-snapshot test may not actually exercise the recovery path.
Per SnapshotManager construction (packages/bruno-electron/src/services/snapshot/index.js Lines 22–30), electron-store is initialized with clearInvalidConfig: true, which silently resets malformed JSON on load. Good — that's what we want to test. But the assertion at Line 512 only checks that workspace-name renders, which is also true on a totally empty userData dir (the fresh-launch test above). Consider also asserting that ui-state-snapshot.json was rewritten to a valid default structure (readSnapshot(userDataPath) returns an object with the expected top-level keys) so a regression that throws before rewrite would be caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/workspace/snapshot-persistence.spec.ts` around lines 505 - 512, The
test currently only checks that workspace-name renders and may pass even if
SnapshotManager didn't rewrite the corrupt file; after launching the app
(launchElectronApp / page.waitFor), call the readSnapshot helper for the
ui-state-snapshot.json (e.g., readSnapshot(userDataPath)) and assert it returns
a valid object containing the expected top-level keys used by SnapshotManager
(for example the keys present in the default snapshot structure), ensuring the
corrupt file was rewritten rather than simply ignored; reference SnapshotManager
and ui-state-snapshot.json to locate the related behavior.
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
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
Extends #7494 with a few changes before finalising them in the original PR
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
Release Notes
New Features
Improvements
Tests