refactor: workspaces preference#6343
Conversation
…space config from storage and improving path handling
WalkthroughRefactored workspace storage mechanism to replace object-based entries (containing pathname, config, and UID) with simple path strings. Updated IPC handlers and the storage layer to operate on paths, removing UID-based deduplication and maximum workspace limits. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/bruno-electron/src/store/last-opened-workspaces.js (2)
15-25: Consider normalizing paths before storage.The
includes()check uses strict string equality, so paths like/workspaceand/workspace/would be treated as different entries. Consider normalizing paths (e.g., removing trailing slashes) to prevent accidental duplicates.add(workspacePath) { + const normalizedPath = workspacePath.replace(/\/+$/, ''); const workspaces = this.getAll(); - if (workspaces.includes(workspacePath)) { + if (workspaces.includes(normalizedPath)) { return workspaces; } - workspaces.unshift(workspacePath); + workspaces.unshift(normalizedPath); this.store.set('workspaces.lastOpenedWorkspaces', workspaces); return workspaces; }
22-24: No upper bound on stored workspaces.The previous
MAX_WORKSPACESlimit was removed. While unlikely to cause issues for typical usage, the list could grow indefinitely. Consider adding a reasonable cap (e.g., 50 entries) if this is a concern.packages/bruno-electron/src/ipc/workspace.js (1)
391-423: Good error isolation per workspace.The per-workspace try/catch prevents one malformed workspace from breaking the entire lookup.
Consider using
readWorkspaceConfig(workspacePath)(from../utils/workspace-config.js) instead of inlineyaml.loadfor consistency with other handlers, but this is optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-electron/src/ipc/workspace.js(7 hunks)packages/bruno-electron/src/store/last-opened-workspaces.js(1 hunks)
🧰 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-electron/src/store/last-opened-workspaces.jspackages/bruno-electron/src/ipc/workspace.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/ipc/workspace.js (1)
packages/bruno-electron/src/utils/workspace-config.js (6)
path(2-2)fs(1-1)workspaceConfig(80-80)yaml(3-3)readWorkspaceConfig(72-87)validateWorkspaceConfig(98-112)
⏰ 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: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (7)
packages/bruno-electron/src/store/last-opened-workspaces.js (1)
27-32: LGTM!Clean implementation. If you adopt path normalization in
add(), apply the same normalization here for consistency.packages/bruno-electron/src/ipc/workspace.js (6)
57-57: LGTM!Correctly updated to the simplified path-based API.
84-84: LGTM!Consistent with the new
add(workspacePath)signature.
122-122: LGTM!Matches the updated API.
190-214: LGTM with optional consideration.The validation logic correctly filters out paths where
workspace.ymldoesn't exist. For deeper validation (malformed YAML, invalid workspace type), you could reusereadWorkspaceConfigandvalidateWorkspaceConfigas done inmain:renderer-ready. However, the current approach is reasonable for a quick existence check.
225-237: LGTM!Correct usage of the updated
remove(workspacePath)API.
478-506: LGTM!Solid implementation with proper validation using
readWorkspaceConfigandvalidateWorkspaceConfig. Good error isolation per workspace, and invalid paths are correctly cleaned up from storage.
Description
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
Chores
✏️ Tip: You can customize this high-level summary in your review settings.