fix(snapshot): active workspace restoration from snapshot#8392
Open
sid-bruno wants to merge 2 commits into
Open
fix(snapshot): active workspace restoration from snapshot#8392sid-bruno wants to merge 2 commits into
sid-bruno wants to merge 2 commits into
Conversation
- Added a new action to restore the active workspace from a snapshot. - Integrated workspace restoration into IPC events for improved user experience. - Refactored workspace loading logic to prioritize valid workspaces and handle invalid paths. - Introduced utility functions for workspace path validation and normalization. - Added tests to ensure correct functionality of workspace restoration and validation logic.
sid-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
Contributor
WalkthroughThe PR centralizes workspace startup resolution, emits a workspace-ready IPC event after Electron startup, restores the active workspace from the renderer snapshot, and updates snapshot serialization plus tests to preserve collection environment state and verify restart behavior. ChangesSnapshot-driven workspace restore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
sid-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/snapshots/active-workspace-restore.spec.ts (1)
97-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap actions in
test.stepand extract locators.Per the e2e path instructions, structure each test with
test.step(Arrange/Act/Assert) for readable reports, and assign reused locators (e.g.workspace-name,workspace-menu) to variables. The second test also skips the active-item assertion the first one makes — worth mirroring for parity.As per path instructions: "Promote the use of
test.stepas much as possible" and "Use locator variables for locators".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/snapshots/active-workspace-restore.spec.ts` around lines 97 - 124, Wrap both workspace restore tests in test.step blocks for clear Arrange/Act/Assert reporting, and extract repeated locators such as workspace-name and workspace-menu into local variables before reuse. Update the 5th-position restore test to mirror the active-item assertion used in the 4th-position test so both cases validate the restored active workspace consistently. Use the existing test functions and helper calls in active-workspace-restore.spec.ts to keep the structure aligned.Source: Path instructions
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js (1)
843-881: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: fold the two
renderer:snapshot:getcalls into one.
workspaceOpenedEventinvokesrenderer:snapshot:gettwice per event (Line 844 for devtools, Line 869 for the switch decision). Reusing a single fetch trims an IPC round-trip on every workspace-opened.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js` around lines 843 - 881, `workspaceOpenedEvent` is calling `ipcRenderer.invoke('renderer:snapshot:get')` twice in the same flow, once for devtools state and again for the workspace switch decision. Fetch the snapshot once near the start of the function, reuse that value for both the devtools handling and the `shouldSwitch` calculation, and keep the existing fallback behavior in the `catch` path when the snapshot lookup fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 843-881: `workspaceOpenedEvent` is calling
`ipcRenderer.invoke('renderer:snapshot:get')` twice in the same flow, once for
devtools state and again for the workspace switch decision. Fetch the snapshot
once near the start of the function, reuse that value for both the devtools
handling and the `shouldSwitch` calculation, and keep the existing fallback
behavior in the `catch` path when the snapshot lookup fails.
In `@tests/snapshots/active-workspace-restore.spec.ts`:
- Around line 97-124: Wrap both workspace restore tests in test.step blocks for
clear Arrange/Act/Assert reporting, and extract repeated locators such as
workspace-name and workspace-menu into local variables before reuse. Update the
5th-position restore test to mirror the active-item assertion used in the
4th-position test so both cases validate the restored active workspace
consistently. Use the existing test functions and helper calls in
active-workspace-restore.spec.ts to keep the structure aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a09db041-762d-4e56-9602-39a09ccdc92d
📒 Files selected for processing (6)
packages/bruno-app/src/providers/App/useIpcEvents.jspackages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/utils/workspace-startup.jspackages/bruno-electron/tests/utils/workspace-startup.spec.jstests/snapshots/active-workspace-restore.spec.ts
…reservation logic - Moved snapshot serialization logic to a new file for better organization. - Implemented environment preservation for collections during snapshot serialization. - Added utility functions to handle collection environment checks and serialization. - Introduced tests to validate the new serialization behavior and environment preservation. - Enhanced existing tests to cover edge cases for unmounted collections and environment persistence.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js (1)
57-64: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInvalidate in-flight debounced saves before flushing.
clearTimeout(saveTimer)only stops a callback that has not started yet. If the debounced save is already insideserializeSnapshot()oripcRenderer.invoke(),flushSnapshotNow()can write the fresh snapshot and the older callback can still finish afterwards, overwriting it with stale state. Track a save generation or in-flight promise so older writes bail before persisting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js` around lines 57 - 64, The snapshot middleware’s `handleAction` path for `app/setSnapshotReady` only cancels the pending timeout, so an already-running debounced save can still complete after `flushSnapshotNow(getState)` and overwrite the newer snapshot with stale data. Update the snapshot save flow in the middleware to track an in-flight save generation or promise state around `serializeSnapshot()` and `ipcRenderer.invoke()` so older saves detect they are stale and exit before persisting; use the existing `saveTimer`, `flushSnapshotNow`, and snapshot write logic to gate writes by the latest generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.js`:
- Around line 105-115: `serializeSnapshot` can throw during fresh snapshot
creation because `existingDevTools.tabs` may be undefined when no prior snapshot
exists. Update the snapshot-building logic in `serializeSnapshot` to fall back
to a safe empty object before calling `Object.assign`, and use the resolved
active tab value consistently for both `activeTab` and the `tabs` entry so the
initial snapshot can be saved without crashing.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.spec.js`:
- Around line 128-202: Add coverage for the first-run path in serializeSnapshot
by including a test where getExistingSnapshot returns null, since all current
cases assume an existing snapshot. Update the serializeSnapshot collection
environment preservation spec to verify behavior when bootstrap starts with no
prior snapshot, using the existing serializeSnapshot and makeState helpers, so
regressions in extras.devTools are caught.
In `@tests/snapshots/environment/environment.spec.ts`:
- Around line 132-139: The restart test uses fixed sleeps in the snapshot
persistence flow, which is both slow and flaky. In environment.spec.ts, replace
the two page2.waitForTimeout(2000) calls with an event-driven wait tied to
snapshot persistence, reusing waitForSnapshotCollectionEnvironment() or
expect.poll() around the openCollection/page2 assertions so the test waits for
the expected environment state instead of an arbitrary delay.
In `@tests/utils/snapshot.ts`:
- Around line 65-69: The polling logic in
findSnapshotCollectionEntry/readSnapshot handling currently returns an empty
string for both a missing snapshot entry and a deliberately cleared
selectedEnvironment, so waits for an empty selection can succeed too early.
Update the snapshot polling in snapshot.ts to return a distinct sentinel value
until the collection entry exists, and only return '' once the entry is actually
present and cleared. Keep the change localized around the expect.poll callback
that reads snapshot entries so callers waiting on selectedEnvironment behave
correctly.
---
Outside diff comments:
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.js`:
- Around line 57-64: The snapshot middleware’s `handleAction` path for
`app/setSnapshotReady` only cancels the pending timeout, so an already-running
debounced save can still complete after `flushSnapshotNow(getState)` and
overwrite the newer snapshot with stale data. Update the snapshot save flow in
the middleware to track an in-flight save generation or promise state around
`serializeSnapshot()` and `ipcRenderer.invoke()` so older saves detect they are
stale and exit before persisting; use the existing `saveTimer`,
`flushSnapshotNow`, and snapshot write logic to gate writes by the latest
generation.
🪄 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: b7b61888-8e7e-4d0f-8d87-e73e3cc99874
📒 Files selected for processing (5)
packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/middleware.jspackages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.jspackages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.spec.jstests/snapshots/environment/environment.spec.tstests/utils/snapshot.ts
Comment on lines
+105
to
+115
| const existingDevTools = existingSnapshot?.extras?.devTools ?? {}; | ||
|
|
||
| const snapshot = { | ||
| activeWorkspacePath: activeWorkspace?.pathname || null, | ||
| extras: { | ||
| devTools: { | ||
| open: logs.isConsoleOpen, | ||
| activeTab: logs.activeTab ?? existingDevTools.activeTab ?? 'terminal', | ||
| tabs: Object.assign(existingDevTools.tabs, { | ||
| [logs.activeTab]: {} | ||
| }) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
devTools.tabs can crash fresh snapshot creation.
When there is no existing snapshot yet, existingDevTools falls back to {}, so existingDevTools.tabs is undefined here. Passing that into Object.assign() throws before any snapshot can be saved. Build from a safe empty object and reuse the resolved active tab for both fields.
Possible fix
- const existingDevTools = existingSnapshot?.extras?.devTools ?? {};
+ const existingDevTools = existingSnapshot?.extras?.devTools ?? {};
+ const activeDevToolsTab = logs.activeTab ?? existingDevTools.activeTab ?? 'terminal';
const snapshot = {
activeWorkspacePath: activeWorkspace?.pathname || null,
extras: {
devTools: {
open: logs.isConsoleOpen,
- activeTab: logs.activeTab ?? existingDevTools.activeTab ?? 'terminal',
- tabs: Object.assign(existingDevTools.tabs, {
- [logs.activeTab]: {}
+ activeTab: activeDevToolsTab,
+ tabs: Object.assign({}, existingDevTools.tabs ?? {}, {
+ [activeDevToolsTab]: {}
})
}
},📝 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
| const existingDevTools = existingSnapshot?.extras?.devTools ?? {}; | |
| const snapshot = { | |
| activeWorkspacePath: activeWorkspace?.pathname || null, | |
| extras: { | |
| devTools: { | |
| open: logs.isConsoleOpen, | |
| activeTab: logs.activeTab ?? existingDevTools.activeTab ?? 'terminal', | |
| tabs: Object.assign(existingDevTools.tabs, { | |
| [logs.activeTab]: {} | |
| }) | |
| const existingDevTools = existingSnapshot?.extras?.devTools ?? {}; | |
| const activeDevToolsTab = logs.activeTab ?? existingDevTools.activeTab ?? 'terminal'; | |
| const snapshot = { | |
| activeWorkspacePath: activeWorkspace?.pathname || null, | |
| extras: { | |
| devTools: { | |
| open: logs.isConsoleOpen, | |
| activeTab: activeDevToolsTab, | |
| tabs: Object.assign({}, existingDevTools.tabs ?? {}, { | |
| [activeDevToolsTab]: {} | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.js`
around lines 105 - 115, `serializeSnapshot` can throw during fresh snapshot
creation because `existingDevTools.tabs` may be undefined when no prior snapshot
exists. Update the snapshot-building logic in `serializeSnapshot` to fall back
to a safe empty object before calling `Object.assign`, and use the resolved
active tab value consistently for both `activeTab` and the `tabs` entry so the
initial snapshot can be saved without crashing.
Comment on lines
+128
to
+202
| describe('serializeSnapshot collection environment preservation', () => { | ||
| it('preserves existing environment fields for an unmounted collection without loaded environments', async () => { | ||
| const snapshot = await serializeSnapshot(makeState(), { | ||
| getExistingSnapshot: async () => makeExistingSnapshot() | ||
| }); | ||
|
|
||
| expect(snapshot.collections).toHaveLength(1); | ||
| expect(snapshot.collections[0]).toMatchObject({ | ||
| pathname: COLLECTION_PATH, | ||
| workspacePathname: WORKSPACE_PATH, | ||
| environmentPath: ENVIRONMENT_PATH, | ||
| selectedEnvironment: 'local', | ||
| environment: { | ||
| collection: ENVIRONMENT_PATH, | ||
| global: '' | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| it('preserves existing environment fields for a mounted but unhydrated collection', async () => { | ||
| const snapshot = await serializeSnapshot( | ||
| makeState({ mountStatus: 'mounted', environments: [], activeEnvironmentUid: null }), | ||
| { getExistingSnapshot: async () => makeExistingSnapshot() } | ||
| ); | ||
|
|
||
| expect(snapshot.collections[0]).toMatchObject({ | ||
| environmentPath: ENVIRONMENT_PATH, | ||
| selectedEnvironment: 'local' | ||
| }); | ||
| }); | ||
|
|
||
| it('writes redux environment selection when mounted collection is hydrated', async () => { | ||
| const snapshot = await serializeSnapshot( | ||
| makeState({ | ||
| mountStatus: 'mounted', | ||
| activeEnvironmentUid: 'env-2', | ||
| environments: [ | ||
| { | ||
| uid: 'env-2', | ||
| name: 'staging', | ||
| pathname: '/tmp/workspace/collections/api/environments/staging.yml' | ||
| } | ||
| ] | ||
| }), | ||
| { getExistingSnapshot: async () => makeExistingSnapshot() } | ||
| ); | ||
|
|
||
| expect(snapshot.collections[0]).toMatchObject({ | ||
| environmentPath: '/tmp/workspace/collections/api/environments/staging.yml', | ||
| selectedEnvironment: 'staging' | ||
| }); | ||
| }); | ||
|
|
||
| it('writes empty environment when mounted hydrated collection selection was cleared', async () => { | ||
| const snapshot = await serializeSnapshot( | ||
| makeState({ | ||
| mountStatus: 'mounted', | ||
| activeEnvironmentUid: null, | ||
| environments: [ | ||
| { | ||
| uid: 'env-1', | ||
| name: 'local', | ||
| pathname: ENVIRONMENT_PATH | ||
| } | ||
| ] | ||
| }), | ||
| { getExistingSnapshot: async () => makeExistingSnapshot() } | ||
| ); | ||
|
|
||
| expect(snapshot.collections[0]).toMatchObject({ | ||
| environmentPath: '', | ||
| selectedEnvironment: '' | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the no-existing-snapshot path.
Every integration case injects getExistingSnapshot(), but first-run serialization is a valid production path. A null snapshot case would catch bootstrap regressions in extras.devTools.
Possible test
describe('serializeSnapshot collection environment preservation', () => {
+ it('creates a snapshot when no existing snapshot is present', async () => {
+ const snapshot = await serializeSnapshot(makeState(), {
+ getExistingSnapshot: async () => null
+ });
+
+ expect(snapshot.extras.devTools).toMatchObject({
+ open: false,
+ activeTab: 'terminal',
+ tabs: {
+ terminal: {}
+ }
+ });
+ });
+
it('preserves existing environment fields for an unmounted collection without loaded environments', async () => {📝 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
| describe('serializeSnapshot collection environment preservation', () => { | |
| it('preserves existing environment fields for an unmounted collection without loaded environments', async () => { | |
| const snapshot = await serializeSnapshot(makeState(), { | |
| getExistingSnapshot: async () => makeExistingSnapshot() | |
| }); | |
| expect(snapshot.collections).toHaveLength(1); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| pathname: COLLECTION_PATH, | |
| workspacePathname: WORKSPACE_PATH, | |
| environmentPath: ENVIRONMENT_PATH, | |
| selectedEnvironment: 'local', | |
| environment: { | |
| collection: ENVIRONMENT_PATH, | |
| global: '' | |
| } | |
| }); | |
| }); | |
| it('preserves existing environment fields for a mounted but unhydrated collection', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ mountStatus: 'mounted', environments: [], activeEnvironmentUid: null }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: ENVIRONMENT_PATH, | |
| selectedEnvironment: 'local' | |
| }); | |
| }); | |
| it('writes redux environment selection when mounted collection is hydrated', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ | |
| mountStatus: 'mounted', | |
| activeEnvironmentUid: 'env-2', | |
| environments: [ | |
| { | |
| uid: 'env-2', | |
| name: 'staging', | |
| pathname: '/tmp/workspace/collections/api/environments/staging.yml' | |
| } | |
| ] | |
| }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: '/tmp/workspace/collections/api/environments/staging.yml', | |
| selectedEnvironment: 'staging' | |
| }); | |
| }); | |
| it('writes empty environment when mounted hydrated collection selection was cleared', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ | |
| mountStatus: 'mounted', | |
| activeEnvironmentUid: null, | |
| environments: [ | |
| { | |
| uid: 'env-1', | |
| name: 'local', | |
| pathname: ENVIRONMENT_PATH | |
| } | |
| ] | |
| }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: '', | |
| selectedEnvironment: '' | |
| }); | |
| }); | |
| }); | |
| describe('serializeSnapshot collection environment preservation', () => { | |
| it('creates a snapshot when no existing snapshot is present', async () => { | |
| const snapshot = await serializeSnapshot(makeState(), { | |
| getExistingSnapshot: async () => null | |
| }); | |
| expect(snapshot.extras.devTools).toMatchObject({ | |
| open: false, | |
| activeTab: 'terminal', | |
| tabs: { | |
| terminal: {} | |
| } | |
| }); | |
| }); | |
| it('preserves existing environment fields for an unmounted collection without loaded environments', async () => { | |
| const snapshot = await serializeSnapshot(makeState(), { | |
| getExistingSnapshot: async () => makeExistingSnapshot() | |
| }); | |
| expect(snapshot.collections).toHaveLength(1); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| pathname: COLLECTION_PATH, | |
| workspacePathname: WORKSPACE_PATH, | |
| environmentPath: ENVIRONMENT_PATH, | |
| selectedEnvironment: 'local', | |
| environment: { | |
| collection: ENVIRONMENT_PATH, | |
| global: '' | |
| } | |
| }); | |
| }); | |
| it('preserves existing environment fields for a mounted but unhydrated collection', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ mountStatus: 'mounted', environments: [], activeEnvironmentUid: null }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: ENVIRONMENT_PATH, | |
| selectedEnvironment: 'local' | |
| }); | |
| }); | |
| it('writes redux environment selection when mounted collection is hydrated', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ | |
| mountStatus: 'mounted', | |
| activeEnvironmentUid: 'env-2', | |
| environments: [ | |
| { | |
| uid: 'env-2', | |
| name: 'staging', | |
| pathname: '/tmp/workspace/collections/api/environments/staging.yml' | |
| } | |
| ] | |
| }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: '/tmp/workspace/collections/api/environments/staging.yml', | |
| selectedEnvironment: 'staging' | |
| }); | |
| }); | |
| it('writes empty environment when mounted hydrated collection selection was cleared', async () => { | |
| const snapshot = await serializeSnapshot( | |
| makeState({ | |
| mountStatus: 'mounted', | |
| activeEnvironmentUid: null, | |
| environments: [ | |
| { | |
| uid: 'env-1', | |
| name: 'local', | |
| pathname: ENVIRONMENT_PATH | |
| } | |
| ] | |
| }), | |
| { getExistingSnapshot: async () => makeExistingSnapshot() } | |
| ); | |
| expect(snapshot.collections[0]).toMatchObject({ | |
| environmentPath: '', | |
| selectedEnvironment: '' | |
| }); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/bruno-app/src/providers/ReduxStore/middlewares/snapshot/serializeSnapshot.spec.js`
around lines 128 - 202, Add coverage for the first-run path in serializeSnapshot
by including a test where getExistingSnapshot returns null, since all current
cases assume an existing snapshot. Update the serializeSnapshot collection
environment preservation spec to verify behavior when bootstrap starts with no
prior snapshot, using the existing serializeSnapshot and makeState helpers, so
regressions in extras.devTools are caught.
Source: Coding guidelines
Comment on lines
+132
to
+139
| // Wait for debounced snapshot save to flush to verify the snapshot isn't overwritten by the next save | ||
| await page2.waitForTimeout(2000); | ||
|
|
||
| await openCollection(page2, 'Collection B'); | ||
| await expect(page2.locator('.current-environment')).toContainText('local-b'); | ||
|
|
||
| // Wait for debounced snapshot save to flush to verify the snapshot isn't overwritten by the next save | ||
| await page2.waitForTimeout(2000); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the fixed sleeps with a real persistence signal.
These waitForTimeout(2000) calls make the restart test slower when CI is fast and still flaky when persistence takes longer than 2s. Reuse waitForSnapshotCollectionEnvironment() or expect.poll() here so the step waits on the snapshot state you actually care about. As per path instructions, "Try to reduce usage of page.waitForTimeout(); in code unless absolutely necessary" and "Replace magic timeouts with event-driven waits in E2E tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/snapshots/environment/environment.spec.ts` around lines 132 - 139, The
restart test uses fixed sleeps in the snapshot persistence flow, which is both
slow and flaky. In environment.spec.ts, replace the two
page2.waitForTimeout(2000) calls with an event-driven wait tied to snapshot
persistence, reusing waitForSnapshotCollectionEnvironment() or expect.poll()
around the openCollection/page2 assertions so the test waits for the expected
environment state instead of an arbitrary delay.
Source: Path instructions
Comment on lines
+65
to
+69
| await expect.poll(() => { | ||
| const snapshot = readSnapshot(userDataPath); | ||
| const entry = findSnapshotCollectionEntry(snapshot, collectionPath); | ||
| return entry?.selectedEnvironment || ''; | ||
| }, { timeout }).toBe(selectedEnvironment); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Don't let '' mean both “not written yet” and “cleared”.
If a caller waits for selectedEnvironment === '', this poll passes immediately when the collection entry is still missing, because both cases return ''. Return a sentinel until the entry exists so clear-selection waits only succeed after the snapshot is actually written.
Possible fix
await expect.poll(() => {
const snapshot = readSnapshot(userDataPath);
const entry = findSnapshotCollectionEntry(snapshot, collectionPath);
- return entry?.selectedEnvironment || '';
- }, { timeout }).toBe(selectedEnvironment);
+ return entry
+ ? { found: true, value: entry.selectedEnvironment ?? '' }
+ : { found: false, value: null };
+ }, { timeout }).toEqual({ found: true, value: selectedEnvironment });📝 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
| await expect.poll(() => { | |
| const snapshot = readSnapshot(userDataPath); | |
| const entry = findSnapshotCollectionEntry(snapshot, collectionPath); | |
| return entry?.selectedEnvironment || ''; | |
| }, { timeout }).toBe(selectedEnvironment); | |
| await expect.poll(() => { | |
| const snapshot = readSnapshot(userDataPath); | |
| const entry = findSnapshotCollectionEntry(snapshot, collectionPath); | |
| return entry | |
| ? { found: true, value: entry.selectedEnvironment ?? '' } | |
| : { found: false, value: null }; | |
| }, { timeout }).toEqual({ found: true, value: selectedEnvironment }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/utils/snapshot.ts` around lines 65 - 69, The polling logic in
findSnapshotCollectionEntry/readSnapshot handling currently returns an empty
string for both a missing snapshot entry and a deliberately cleared
selectedEnvironment, so waits for an empty selection can succeed too early.
Update the snapshot polling in snapshot.ts to return a distinct sentinel value
until the collection entry exists, and only return '' once the entry is actually
present and cleared. Keep the change localized around the expect.poll callback
that reads snapshot entries so callers waiting on selectedEnvironment behave
correctly.
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
1 participant
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
JIRA
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Bug Fixes
Tests