fix(workspace): heal legacy object entries in lastOpenedWorkspaces#8769
fix(workspace): heal legacy object entries in lastOpenedWorkspaces#8769vijayh-bruno wants to merge 1 commit into
Conversation
WalkthroughElectron workspace handling now normalizes persisted workspace entries, migrates legacy values, de-duplicates paths, and rejects invalid startup path inputs before filesystem checks. Tests cover non-string inputs with and without configuration validation. ChangesWorkspace handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-electron/src/store/last-opened-workspaces.js (1)
10-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused coverage for the persistence migration.
The supplied tests cover startup input validation but not legacy
pathnamerecovery, invalid-entry removal, duplicate collapse, or rewrite-on-change behavior. Add behavior-level tests for these cases before merging.As per coding guidelines, meaningful code changes must add or update deterministic behavior-driven tests.
Also applies to: 45-55
🤖 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-electron/src/store/last-opened-workspaces.js` around lines 10 - 28, Add deterministic behavior-driven tests for normalizeWorkspaceEntry and normalizeWorkspaceEntries covering legacy object pathname recovery, removal of invalid entries, collapsing duplicate string/pathname values, and persistence rewriting when normalized data differs from stored data. Keep existing startup validation coverage intact and assert observable persisted output rather than implementation details.Source: Coding guidelines
🤖 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-electron/tests/utils/workspace-startup.spec.js`:
- Around line 123-124: Update both isValidWorkspacePathOnDisk expectations in
the workspace startup test to use an OS-neutral temporary path built with
path.join(os.tmpdir(), 'ws') instead of the hardcoded '/tmp/ws', adding the
required path and os imports if absent.
---
Nitpick comments:
In `@packages/bruno-electron/src/store/last-opened-workspaces.js`:
- Around line 10-28: Add deterministic behavior-driven tests for
normalizeWorkspaceEntry and normalizeWorkspaceEntries covering legacy object
pathname recovery, removal of invalid entries, collapsing duplicate
string/pathname values, and persistence rewriting when normalized data differs
from stored data. Keep existing startup validation coverage intact and assert
observable persisted output rather than implementation details.
🪄 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 Plus
Run ID: fdfe67c8-62b6-45c9-bd4f-db66edd9733c
📒 Files selected for processing (3)
packages/bruno-electron/src/store/last-opened-workspaces.jspackages/bruno-electron/src/utils/workspace-startup.jspackages/bruno-electron/tests/utils/workspace-startup.spec.js
| expect(isValidWorkspacePathOnDisk({ pathname: '/tmp/ws' })).toBe(false); | ||
| expect(isValidWorkspacePathOnDisk({ pathname: '/tmp/ws' }, { validateConfig: true })).toBe(false); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an OS-neutral temporary path in the test.
The new fixtures hardcode /tmp/ws. Although the guard returns before filesystem access today, this embeds Unix-only path syntax in a cross-platform test suite. Use path.join(os.tmpdir(), 'ws') instead.
As per path instructions, use os.tmpdir() instead of hardcoded /tmp and build paths with path.join() or path.resolve().
🤖 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-electron/tests/utils/workspace-startup.spec.js` around lines
123 - 124, Update both isValidWorkspacePathOnDisk expectations in the workspace
startup test to use an OS-neutral temporary path built with
path.join(os.tmpdir(), 'ws') instead of the hardcoded '/tmp/ws', adding the
required path and os imports if absent.
Source: Path instructions
1 participant
Description
Problem:
Workspaces disappearing after upgrade when
preferences.json→workspaces.lastOpenedWorkspacescontains legacy object entries mixed with string paths. Startup code assumed every entry was a string and calledpath.join(entry, 'workspace.yml'). On an object entry, this threw aTypeError, which aborted workspace resolution and made the handler return an empty list. So one bad entry wiped out every workspace, leaving only the default "My Workspace".Fix:
Normalise entries in the store: keep strings, recover the path from an
object's
pathname, drop anything unusable, and de-duplicate. So callersonly ever see a string-only list.
migrate()runs once on load and rewritespreferences.jsonto the clean string-only array, scrubbing the legacy/experimental build-related objects from disk.Guard
isValidWorkspacePathOnDiskagainst non-string input as defense.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
Bug Fixes
Tests