Persist workspace state across app restarts#6573
Persist workspace state across app restarts#6573chirag-bruno wants to merge 7 commits intousebruno:mainfrom
Conversation
…reen component - Added redux-persist to manage state persistence across sessions. - Implemented a LoadingScreen component to enhance user experience during data loading. - Updated Main component to utilize PersistGate for handling loading state. - Modified ReduxStore to include persisted reducers for workspaces. - Updated package.json to include redux-persist as a dependency.
…ogic - Updated Main component to ensure renderer is ready after persistence is complete. - Improved LoadingScreen integration during the loading state of the PersistGate. - Refactored ReduxStore to remove persisted reducer for workspaces. - Added ensureActiveWorkspace function to manage active workspace switching after collections are loaded.
- Introduced a new test suite to verify that the active workspace selection persists across application restarts. - Implemented helper functions for creating and selecting workspaces, as well as retrieving the current active workspace name. - Utilized a singleton pattern for workspace setup to optimize test execution and resource management.
- Removed outdated comments in Main.js and actions.js to enhance code clarity. - Streamlined the ensureActiveWorkspace function to focus on workspace activation logic without unnecessary comments. - Ensured that workspace collections are loaded before switching to the active workspace.
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughIntegrates redux-persist for workspace state persistence, adds a LoadingScreen, wraps app initialization with PersistGate to wait for rehydration, moves the renderer-ready IPC handshake to post-bootstrap, centralizes workspace activation logic, and adds Playwright fixtures/tests and workspace YAML fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main.js
participant PG as PersistGate
participant Persistor as Redux Persistor
participant Store as Redux Store
participant Providers as App Providers
participant IPC as IPC Renderer
rect rgba(200,220,240,0.9)
Note over Main,PG: App start — wrap providers with PersistGate
Main->>PG: Render PersistGate with persistor
end
rect rgba(240,220,200,0.9)
Note over PG,Persistor: Rehydration phase
PG->>Persistor: bootstrap()
Persistor->>Store: rehydrate persisted slices (workspaces)
Store-->>Persistor: rehydration complete
end
rect rgba(240,240,220,0.9)
Note over Main,Providers: Loading UI during bootstrap
Main->>Main: Render LoadingScreen until bootstrapped
end
rect rgba(200,240,200,0.9)
Note over PG,IPC: Post-bootstrap initialization
Persistor-->>PG: bootstrapped
PG->>Providers: mount provider tree
Providers->>Main: providers mounted
Main->>IPC: invoke('renderer:ready') %% one-time call
IPC-->>Main: ack
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/bruno-app/src/components/LoadingScreen/index.js (1)
3-17: Consider adding accessibility attributes.The component looks clean. For screen reader users, consider adding
role="status"andaria-live="polite"to announce the loading state, plusaria-hidden="true"on the decorative spinner.🔎 Suggested accessibility enhancement
const LoadingScreen = () => { return ( - <div className="fixed inset-0 z-50 flex items-center justify-center bg-white dark:bg-zinc-900 transition-all duration-300"> + <div className="fixed inset-0 z-50 flex items-center justify-center bg-white dark:bg-zinc-900 transition-all duration-300" role="status" aria-live="polite"> <div className="flex flex-col items-center"> <IconLoader2 className="animate-spin h-12 w-12 mb-4 text-zinc-600 dark:text-zinc-400" strokeWidth={1.5} + aria-hidden="true" /> <h3 className="text-xl font-medium text-zinc-900 dark:text-zinc-50"> Loading Bruno </h3> </div> </div> ); };packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
402-408: Empty catch block swallows errors silently.The catch block on lines 404-405 swallows errors from
loadWorkspaceCollections. Consider at minimum logging the error for debugging.🔎 Proposed fix
try { await dispatch(loadWorkspaceCollections(workspaceUid)); } catch (error) { + console.error('Failed to load workspace collections:', error); }packages/bruno-app/src/pages/Main.js (1)
52-52: Minor: Redundant ipcRenderer check.The
window.ipcRenderercheck here is technically redundant since the early return on line 34 would have already bailed out. Harmless, but could be simplified.🔎 Simplified condition
- if (bootstrapped && window.ipcRenderer && !rendererReadyCalled.current) { + if (bootstrapped && !rendererReadyCalled.current) {tests/workspace/workspace-persistence.spec.ts (3)
62-62: AvoidwaitForTimeout— prefer assertion-based waits.Per coding guidelines, reduce usage of
page.waitForTimeout(). The workspace switch completion might be better detected via a locator assertion.🔎 Alternative approach
- // Wait a bit for workspace to be created and switched to - await page.waitForTimeout(500); - // Verify workspace name appears in title bar await expect(page.locator('.workspace-name')).toContainText(workspaceName); + // The assertion above already waits for the element, additional timeout likely unnecessary
200-203: Multiple hardcoded waits reduce test reliability.Lines 202 and 222 use
waitForTimeout(2000). Consider replacing with condition-based waits where possible — e.g., waiting for a specific element state or network idle.- await page.waitForTimeout(2000); + // If waiting for persistence, consider polling localStorage or waiting for a UI indicator
128-136: Non-null assertion onworkspaceSetupPromise.Lines 129 and 134 use
!non-null assertion. While the fixture dependency chain should guarantee initialization, consider adding a guard for safety.🔎 Defensive check
workspaceNames: async ({ userDataPath }, use) => { - const setup = await workspaceSetupPromise!; + if (!workspaceSetupPromise) { + throw new Error('workspaceSetupPromise not initialized'); + } + const setup = await workspaceSetupPromise; await use(setup.workspaceNames); },packages/bruno-app/src/providers/ReduxStore/index.js (1)
51-56: Add missing redux-persist actions to serializableCheck.The ignored actions list is incomplete. Redux Toolkit and redux-persist documentation recommend ignoring all six persist actions to prevent serialization warnings.
🔎 Proposed fix
middleware: (getDefaultMiddleware) => getDefaultMiddleware({ serializableCheck: { - ignoredActions: ['persist/PERSIST', 'persist/REHYDRATE', 'persist/PURGE'] + ignoredActions: [ + 'persist/PERSIST', + 'persist/REHYDRATE', + 'persist/PURGE', + 'persist/FLUSH', + 'persist/PAUSE', + 'persist/REGISTER' + ] } }).concat(middleware)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
packages/bruno-app/package.jsonpackages/bruno-app/src/components/LoadingScreen/index.jspackages/bruno-app/src/pages/Main.jspackages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jstests/workspace/workspace-persistence.spec.ts
💤 Files with no reviewable changes (1)
- packages/bruno-app/src/providers/App/useIpcEvents.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/LoadingScreen/index.jstests/workspace/workspace-persistence.spec.tspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-app/src/pages/Main.js
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/workspace/workspace-persistence.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/components/LoadingScreen/index.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-app/src/pages/Main.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
tests/workspace/workspace-persistence.spec.ts
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
tests/workspace/workspace-persistence.spec.ts
🧬 Code graph analysis (1)
tests/workspace/workspace-persistence.spec.ts (1)
packages/bruno-electron/src/utils/workspace-config.js (1)
workspaceName(217-217)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (6)
packages/bruno-app/package.json (1)
87-87: LGTM!The redux-persist dependency addition is appropriate for the workspace persistence feature. Version ^6.0.0 is stable and well-maintained.
packages/bruno-app/src/providers/ReduxStore/index.js (1)
28-34: LGTM on persistence configuration.The whitelist approach for
activeWorkspaceUidis correct — only persisting the minimal state needed for workspace restoration.packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
358-392: Solid helper for workspace activation.The logic correctly waits for all workspaces with
pathnameto finish loading before selecting the active one. The fallback chain (current active → default → first) is sensible.One note:
switchWorkspaceis async but called withoutawait. This is likely intentional (fire-and-forget), but worth confirming this won't cause race conditions in edge cases.packages/bruno-app/src/pages/Main.js (1)
50-77: LGTM on PersistGate integration.The render prop pattern with
useRefto ensure single IPC invocation is correct. Therenderer:readysignal is properly gated until persistence bootstrapping completes. Clean implementation.tests/workspace/workspace-persistence.spec.ts (2)
69-74: Singleton pattern note.The singleton
workspaceSetupPromiseis a smart optimization to avoid recreating workspaces for each test. Be aware this introduces test coupling — if the shared state gets corrupted, all subsequent tests fail.
187-234: Good test coverage for persistence.The test properly validates that workspace selection survives multiple app restarts. The iteration approach with random workspace selection provides decent coverage.
…leware configuration - Added role and aria-live attributes to the LoadingScreen for better accessibility. - Removed unnecessary window.ipcRenderer check in Main component to simplify logic. - Expanded ignored actions in Redux middleware to include additional persistence actions. - Added error logging for failed workspace collection loading in actions.js. - Improved test stability by ensuring UI elements are ready before interaction in workspace persistence tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/workspace/workspace-persistence.spec.ts (5)
75-79: Consider removing the waitForTimeout here.The
waitForTimeout(500)on line 76 precedes the assertion on line 79. Sinceexpect(page.locator('.workspace-name')).toContainText(workspaceName)already has built-in auto-waiting, the explicit timeout may be unnecessary.As per coding guidelines, reduce usage of
waitForTimeoutunless absolutely necessary and the locator cannot be found using existingexpect()calls.🔎 Suggested refactor
- // Wait a bit for workspace to be created and switched to - await page.waitForTimeout(500); - // Verify workspace name appears in title bar await expect(page.locator('.workspace-name')).toContainText(workspaceName);
142-149: Non-null assertions onworkspaceSetupPromiseassume fixture ordering.Lines 143 and 148 use
workspaceSetupPromise!which assumesuserDataPathfixture always runs first to initialize the promise. If fixture dependencies change or tests are reorganized, this could cause runtime errors.The current setup works because
userDataPathis a dependency in the fixture chain, but adding a comment explaining this dependency would improve maintainability.
214-217: Unnecessary waitForTimeout after getting workspace name.Line 216 adds a 2000ms delay after
getActiveWorkspaceName, which already awaits the element's visibility. This timeout appears superfluous and slows down test execution.As per coding guidelines, avoid
waitForTimeoutunless absolutely necessary.🔎 Suggested refactor
await page.locator('[data-app-state="loaded"]').waitFor({ timeout: 30000 }); const activeWorkspaceName = await getActiveWorkspaceName(page); - await page.waitForTimeout(2000); expect(activeWorkspaceName.toLowerCase()).toContain(previousWorkspaceName!.toLowerCase());
204-205: InitialpreviousWorkspaceNameassumes 'Workspace5' is the last created workspace.The hardcoded
'Workspace5'relies on the singleton setup creating workspaces in order and ending on Workspace5. If the setup logic changes, this test could fail unexpectedly. Consider adding a comment explaining this assumption or deriving it from the fixture.
234-245: Consider documenting why these delays are necessary for persistence.The 2000ms delays (lines 236 and 244) appear to be necessary for redux-persist to flush state to storage before app close. A brief comment explaining this would help future maintainers understand these aren't arbitrary waits.
// Wait before closing to ensure state is saved + // redux-persist needs time to flush state to localStorage await new Promise((resolve) => setTimeout(resolve, 2000));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/bruno-app/src/components/LoadingScreen/index.jspackages/bruno-app/src/pages/Main.jspackages/bruno-app/src/providers/ReduxStore/index.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jstests/workspace/workspace-persistence.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/LoadingScreen/index.js
- packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/pages/Main.jspackages/bruno-app/src/providers/ReduxStore/index.jstests/workspace/workspace-persistence.spec.ts
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/workspace/workspace-persistence.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/pages/Main.jspackages/bruno-app/src/providers/ReduxStore/index.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
tests/workspace/workspace-persistence.spec.ts
🧬 Code graph analysis (2)
packages/bruno-app/src/pages/Main.js (3)
packages/bruno-app/src/providers/ReduxStore/index.js (2)
persistor(66-66)persistor(66-66)packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
window(19-19)packages/bruno-app/src/components/LoadingScreen/index.js (1)
LoadingScreen(3-18)
tests/workspace/workspace-persistence.spec.ts (3)
packages/bruno-electron/src/utils/workspace-config.js (1)
workspaceName(217-217)packages/bruno-app/src/components/WorkspaceHome/index.js (1)
workspaceNameInput(22-22)tests/workspace/default-workspace/default-workspace.spec.ts (1)
test(5-230)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (11)
tests/workspace/workspace-persistence.spec.ts (4)
1-4: LGTM!Imports are correctly structured with Playwright types and Node.js fs/path modules.
83-88: Module-level singleton may cause issues with parallel test execution.The
workspaceSetupPromisesingleton at module scope works for serial execution but could cause race conditions or unexpected state sharing if Playwright runs tests in parallel across workers. Each worker would share this module state.Consider documenting that this test file must run serially or use Playwright's
test.describe.serialif parallel execution becomes an issue.
171-190: LGTM!The
selectWorkspacehelper follows Playwright best practices: usestest.stepfor reporting, proper locator variables, and waitFor states instead of arbitrary timeouts.
195-199: LGTM!Clean helper function with appropriate visibility assertion.
packages/bruno-app/src/pages/Main.js (3)
1-3: LGTM!Imports are correctly structured with
useReffor the one-time guard andPersistGatefor redux-persist integration.
31-33: LGTM!The
useRefguard is the correct pattern to ensurerenderer:readyis called exactly once, preventing duplicate IPC calls on re-renders.
50-77: LGTM!The
PersistGateimplementation is well-structured:
- Uses render prop pattern correctly for conditional rendering
- One-time IPC guard prevents duplicate
renderer:readycalls- Error handling with
.catch()logs failures appropriately- Clean separation between loading state and bootstrapped state
packages/bruno-app/src/providers/ReduxStore/index.js (4)
1-3: LGTM!Imports are correctly structured for redux-persist integration.
28-34: LGTM!The persist configuration is appropriately scoped:
- Uses
whitelistto persist onlyactiveWorkspaceUid, avoiding bloating storage with unnecessary state- Key is namespaced correctly as
'workspaces'
36-47: LGTM!The
rootReducercorrectly combines all slices, withpersistedWorkspacesReducerwired for the workspaces slice.
49-66: LGTM!Store configuration is correct:
- All redux-persist actions (
PERSIST,REHYDRATE,PURGE,FLUSH,PAUSE,REGISTER) are ignored in serializable checks- Custom middleware is properly concatenated
persistoris exported for use withPersistGate
…tence tests - Introduced a new function, restartAppWithPersistentState, to manage app restarts while preserving user data and workspace state. - Implemented helper functions for workspace selection and retrieval of the current active workspace name. - Created a new test suite to verify that active workspace selections persist across application restarts, enhancing test coverage for workspace management. - Added fixture files for workspace configurations to support the new tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
playwright/index.ts (1)
216-293: Implementation looks solid, with a few minor refinements.The fixture correctly maintains persistent state across restarts by reusing the same userDataPath. Template replacement logic is well-structured with clear helper functions.
Consider these optional improvements:
Line 262:
fs.cpSyncis synchronous. Since the rest of the file uses async patterns, considerfs.promises.cp(src, dest, { recursive: true })for consistency.Lines 247-253: The silent catch on line 252 may hide legitimate errors beyond binary files. Consider logging skipped files or being more specific about which errors to ignore:
- } catch (err) { - // Skip files that can't be read as text (e.g., binary files) - } + } catch (err) { + // Skip binary files that can't be read as UTF-8 + if (err instanceof Error && !err.message.includes('invalid UTF-8')) { + console.warn(`Failed to process ${fullPath}:`, err.message); + } + }Line 244: The
textExtensionslist is hardcoded. If you anticipate needing to process other text-based config formats, consider making it a parameter or constant at the module level.tests/workspace/persistence/workspace-persistence.spec.ts (1)
44-90: Replace arbitrary timeouts with deterministic waiting mechanisms.The test logic is sound, but using
setTimeout(lines 77, 85) introduces non-deterministic waits that can make tests flaky. Per coding guidelines, avoidpage.waitForTimeout()unless absolutely necessary.Recommended alternatives for timeouts
Line 77: Instead of waiting 2 seconds for state to save, consider:
- Listening for a persistence-complete event from the app
- Checking for a specific DOM state or attribute that indicates save completion
- Using IPC to confirm state was written to disk
Example:
- // Wait before closing to ensure state is saved - await new Promise((resolve) => setTimeout(resolve, 2000)); + // Wait for workspace state to persist + await expect(page.locator('[data-persistence-state="synced"]')).toBeVisible({ timeout: 5000 });Line 85: For app close confirmation, consider:
- Relying on the fixture cleanup (which already closes apps properly)
- Or checking that the process has actually terminated
- // Wait a bit for the app to fully close and cleanup - if (iteration < 5) { - await new Promise((resolve) => setTimeout(resolve, 2000)); - } + // Fixture cleanup handles app closure properly + // No additional wait neededIf you must keep the timeouts for now, document why they're necessary with specific reasoning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
playwright/index.tstests/workspace/persistence/fixtures/workspaces/workspace-1/workspace.ymltests/workspace/persistence/fixtures/workspaces/workspace-2/workspace.ymltests/workspace/persistence/fixtures/workspaces/workspace-3/workspace.ymltests/workspace/persistence/fixtures/workspaces/workspace-4/workspace.ymltests/workspace/persistence/fixtures/workspaces/workspace-5/workspace.ymltests/workspace/persistence/init-user-data/preferences.jsontests/workspace/persistence/workspace-persistence.spec.ts
✅ Files skipped from review due to trivial changes (5)
- tests/workspace/persistence/fixtures/workspaces/workspace-5/workspace.yml
- tests/workspace/persistence/fixtures/workspaces/workspace-4/workspace.yml
- tests/workspace/persistence/init-user-data/preferences.json
- tests/workspace/persistence/fixtures/workspaces/workspace-1/workspace.yml
- tests/workspace/persistence/fixtures/workspaces/workspace-3/workspace.yml
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/workspace/persistence/fixtures/workspaces/workspace-2/workspace.ymltests/workspace/persistence/workspace-persistence.spec.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
playwright/index.tstests/workspace/persistence/workspace-persistence.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
tests/workspace/persistence/workspace-persistence.spec.ts
🧬 Code graph analysis (1)
tests/workspace/persistence/workspace-persistence.spec.ts (1)
playwright/index.ts (1)
test(28-327)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
🔇 Additional comments (3)
tests/workspace/persistence/fixtures/workspaces/workspace-2/workspace.yml (1)
1-11: LGTM!The fixture structure is valid and properly nested in the fixtures folder, following the documented pattern for test fixtures.
tests/workspace/persistence/workspace-persistence.spec.ts (2)
8-27: Helper function is well-structured.Good use of
test.stepfor better test reporting, proper use of locators, and multiple assertions.
32-36: LGTM!Helper function correctly waits for visibility before retrieving text content.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
358-392: LGTM! Solid centralization of workspace selection logic.The
ensureActiveWorkspacehelper correctly waits for all workspace collections to load before determining the active workspace, preventing race conditions during app initialization. The fallback logic (active → default → first workspace) handles edge cases like deleted or invalid workspaces gracefully.The synchronous dispatch of
setActiveWorkspaceon line 233 inswitchWorkspaceensures that multiple rapid calls toensureActiveWorkspacewon't trigger duplicate switches, which is a nice touch.Optional: Add clarifying comment about race condition prevention
Consider adding a brief comment explaining that
switchWorkspaceupdatesactiveWorkspaceUidsynchronously, preventing duplicate switches:const ensureActiveWorkspace = (dispatch, getState) => { const state = getState(); const { workspaces, activeWorkspaceUid } = state.workspaces; + // Note: switchWorkspace dispatches setActiveWorkspace synchronously, + // so multiple calls to ensureActiveWorkspace won't cause duplicate switches if (targetWorkspaceUid && targetWorkspaceUid !== activeWorkspaceUid) { dispatch(switchWorkspace(targetWorkspaceUid)); } };This would help future maintainers understand the design intent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js
🧬 Code graph analysis (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/index.js (5)
workspace(20-20)workspace(42-42)workspace(50-50)workspace(63-63)workspace(75-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
402-409: LGTM! Good error handling and delegation.The addition of
console.erroron line 405 improves debugging visibility for workspace loading failures. CallingensureActiveWorkspaceafter the try-catch ensures the app always attempts to establish a valid active workspace, even when collection loading fails—this aligns well with the PR's goal of robust workspace persistence across restarts.
|
@chirag-bruno Why was this closed, this seems like something that would be very useful as constantly needing to open the same folders/requests when switching workspaces is quite annoying. It looks like this would solve that and it was mostly done. |
|
Hey @slootjes, after reviewing the implementation, we realized this feature would be more valuable if it worked across Workspaces → Collections → Tabs. |
2 participants
Description
This PR implements workspace state persistence using redux-persist, ensuring the active workspace selection is maintained across application restarts. The implementation includes enhanced loading state management with a dedicated LoadingScreen component, improved workspace activation logic that waits for collections to load before switching, and comprehensive test coverage for persistence behavior.
Contribution Checklist:
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.