improve: workspace handling#6495
Conversation
WalkthroughWorkspace operations replaced direct YAML I/O with centralized workspace-config helpers, atomic writes, and per-workspace locking. API spec and IPC handlers now use the new helpers and normalized client-facing workspace payloads; default-workspace naming/type is centralized and migration/initialization flows include validation and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Renderer as Renderer/UI
participant IPC as Main IPC Handler
participant Lock as Workspace Lock
participant Config as Workspace Config Utils
participant FS as File System
rect rgb(220,255,220)
Note over Renderer,FS: New locked & atomic workspace update flow
Client->>Renderer: Trigger add/remove API spec
Renderer->>IPC: IPC call (addApiSpec/removeApiSpec)
IPC->>Lock: withLock(getWorkspaceLockKey(path))
Lock-->>IPC: lock held
IPC->>Config: readWorkspaceConfig(workspacePath)
Config->>FS: read workspace.yml
FS-->>Config: YAML content
Config-->>IPC: parsed config
IPC->>Config: addApiSpecToWorkspace / removeApiSpecFromWorkspace
Config->>FS: writeWorkspaceFileAtomic (temp write + rename)
FS-->>Config: write success
Lock-->>IPC: lock released
IPC->>Renderer: emit updated prepared workspace config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-electron/src/app/apiSpecs.js (1)
93-105: Inlinerequire()calls are inefficient.Using
require('fs'),require('path'), andrequire('js-yaml')inside the function body is redundant since these are already imported at the top of the file. This adds unnecessary overhead on each invocation.🔎 Proposed fix
win.webContents.send('main:apispec-tree-updated', 'addFile', { pathname: apiSpecPath, uid: uid, - raw: require('fs').readFileSync(apiSpecPath, 'utf8'), - name: require('path').basename(apiSpecPath, require('path').extname(apiSpecPath)), - filename: require('path').basename(apiSpecPath), + raw: fs.readFileSync(apiSpecPath, 'utf8'), + name: path.basename(apiSpecPath, path.extname(apiSpecPath)), + filename: path.basename(apiSpecPath), json: (() => { - const ext = require('path').extname(apiSpecPath).toLowerCase(); - const content = require('fs').readFileSync(apiSpecPath, 'utf8'); + const ext = path.extname(apiSpecPath).toLowerCase(); + const content = fs.readFileSync(apiSpecPath, 'utf8'); if (ext === '.yaml' || ext === '.yml') { - return require('js-yaml').load(content); + return yaml.load(content); } else if (ext === '.json') { return JSON.parse(content); } return null; })() });You'll also need to add
yamlto the imports at the top:const yaml = require('js-yaml');packages/bruno-electron/src/ipc/apiSpec.js (1)
22-29: FloatingPromise.resolve()has no effect.Line 25 creates a promise that is never awaited or returned. This appears to be leftover from a previous refactor and should be removed.
🔎 Proposed fix
ipcMain.handle('renderer:save-api-spec', async (event, pathname, content) => { try { await writeFile(pathname, content); - Promise.resolve(); + return; } catch (error) { return Promise.reject(error); } });
🧹 Nitpick comments (7)
packages/bruno-electron/src/app/apiSpecs.js (1)
12-33: Duplicated constants and helpers across files.
DEFAULT_WORKSPACE_NAME,normalizeWorkspaceConfig, andprepareWorkspaceConfigForClientare duplicated inworkspace-watcher.jsandipc/workspace.js. Consider exporting these from a single source (e.g.,workspace-config.js) to maintain consistency and reduce maintenance overhead.packages/bruno-electron/src/app/workspace-watcher.js (2)
14-14: Consistent default workspace naming.The
DEFAULT_WORKSPACE_NAMEconstant and its usage look correct. However, this pattern (lines 51-55) could useprepareWorkspaceConfigForClienthelper for consistency with other files. Currently, this file does an inline spread whileapiSpecs.jsandworkspace.jsuse the helper.Also applies to: 51-55
21-29: DuplicatenormalizeWorkspaceConfigimplementation.This function is identical to the one in
workspace-config.js(lines 187-194). Consider importing it from the centralized utility instead.packages/bruno-electron/src/ipc/workspace.js (2)
29-40: Duplicated constant and helper.
DEFAULT_WORKSPACE_NAMEandprepareWorkspaceConfigForClientare repeated acrossapiSpecs.js,workspace-watcher.js, and here. Extract to a shared module.
174-209: Inconsistent config reading pattern.
renderer:load-workspace-apispecsuses directfs.readFileSyncandyaml.load(lines 186-187) while other handlers usereadWorkspaceConfig. Consider using the centralized utility for consistency.🔎 Proposed refactor
ipcMain.handle('renderer:load-workspace-apispecs', async (event, workspacePath) => { try { if (!workspacePath) { throw new Error('Workspace path is undefined'); } - const workspaceFilePath = path.join(workspacePath, 'workspace.yml'); - - if (!fs.existsSync(workspaceFilePath)) { - throw new Error('Invalid workspace: workspace.yml not found'); - } - - const yamlContent = fs.readFileSync(workspaceFilePath, 'utf8'); - const workspaceConfig = yaml.load(yamlContent); - - if (!workspaceConfig || typeof workspaceConfig !== 'object') { - return []; - } - - const specs = workspaceConfig.specs || []; + validateWorkspacePath(workspacePath); + const workspaceConfig = readWorkspaceConfig(workspacePath); + const specs = workspaceConfig.specs || workspaceConfig.apiSpecs || []; const resolvedSpecs = specs.map((spec) => { // ... rest unchanged }); return resolvedSpecs; } catch (error) { throw error; } });packages/bruno-electron/src/store/workspace-environments.js (1)
133-152:getActiveGlobalEnvironmentUidstill uses direct file access.This method reads
workspace.ymldirectly whilesetActiveGlobalEnvironmentUidnow usesreadWorkspaceConfig. Consider using the centralized utility for consistency, though this is read-only so the impact is minimal.packages/bruno-electron/src/store/default-workspace.js (1)
227-230: Consider reusingpreferencesStoreinstance.The cleanup function creates a new
Store({ name: 'global-environments' })but already has access topreferencesStorefrom the outer scope. While functional, reusing instances is slightly more efficient. The global-environments store is separate though, so this may be intentional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/bruno-electron/src/app/apiSpecs.jspackages/bruno-electron/src/app/workspace-watcher.jspackages/bruno-electron/src/ipc/apiSpec.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/store/default-workspace.jspackages/bruno-electron/src/store/workspace-environments.jspackages/bruno-electron/src/utils/workspace-config.jspackages/bruno-electron/src/utils/workspace-lock.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-electron/src/app/workspace-watcher.jspackages/bruno-electron/src/utils/workspace-lock.jspackages/bruno-electron/src/store/workspace-environments.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/ipc/apiSpec.jspackages/bruno-electron/src/utils/workspace-config.jspackages/bruno-electron/src/store/default-workspace.jspackages/bruno-electron/src/app/apiSpecs.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-electron/src/app/workspace-watcher.jspackages/bruno-electron/src/utils/workspace-lock.jspackages/bruno-electron/src/store/workspace-environments.jspackages/bruno-electron/src/ipc/workspace.jspackages/bruno-electron/src/ipc/apiSpec.jspackages/bruno-electron/src/utils/workspace-config.jspackages/bruno-electron/src/store/default-workspace.jspackages/bruno-electron/src/app/apiSpecs.js
🧬 Code graph analysis (5)
packages/bruno-electron/src/store/workspace-environments.js (2)
packages/bruno-electron/src/utils/workspace-config.js (8)
workspaceFilePath(26-26)workspaceFilePath(161-161)workspaceFilePath(198-198)path(2-2)fs(1-1)readWorkspaceConfig(197-212)generateYamlContent(214-270)writeWorkspaceFileAtomic(25-45)packages/bruno-electron/src/utils/workspace-lock.js (2)
withLock(26-33)getWorkspaceLockKey(35-37)
packages/bruno-electron/src/ipc/apiSpec.js (1)
packages/bruno-electron/src/utils/workspace-config.js (1)
removeApiSpecFromWorkspace(461-495)
packages/bruno-electron/src/utils/workspace-config.js (1)
packages/bruno-electron/src/utils/workspace-lock.js (2)
withLock(26-33)getWorkspaceLockKey(35-37)
packages/bruno-electron/src/store/default-workspace.js (1)
packages/bruno-electron/src/utils/workspace-config.js (2)
path(2-2)isValidCollectionEntry(47-61)
packages/bruno-electron/src/app/apiSpecs.js (2)
packages/bruno-electron/src/app/workspace-watcher.js (10)
fs(2-2)require(6-6)require(7-7)require(8-8)require(10-10)path(3-3)DEFAULT_WORKSPACE_NAME(14-14)workspaceConfig(41-41)isDefault(49-49)normalizeWorkspaceConfig(21-29)packages/bruno-electron/src/utils/workspace-config.js (13)
fs(1-1)require(5-5)require(6-6)require(7-7)require(498-498)path(2-2)workspaceConfig(205-205)readWorkspaceConfig(197-212)specs(240-240)specs(415-415)addApiSpecToWorkspace(428-459)normalizeWorkspaceConfig(187-195)getWorkspaceUid(497-504)
⏰ 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: Unit Tests
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (12)
packages/bruno-electron/src/app/apiSpecs.js (1)
50-85: Logic looks good with the new workspace-config utilities.The refactored flow using
addApiSpecToWorkspace,readWorkspaceConfig, andprepareWorkspaceConfigForClientis cleaner and aligns with the centralized workspace-config approach. The guard for missinga.pathon line 64 is a good defensive addition.packages/bruno-electron/src/ipc/apiSpec.js (1)
44-61: Clean refactor to centralized utility.The switch to
removeApiSpecFromWorkspaceis a solid improvement—delegates removal logic to the workspace-config module with proper locking and atomic writes.packages/bruno-electron/src/utils/workspace-lock.js (1)
26-37:withLockwrapper and key helper look good.The try/finally pattern ensures the lock is always released.
getWorkspaceLockKeyprovides a consistent namespace for workspace locks.packages/bruno-electron/src/ipc/workspace.js (1)
45-90: Consistent application ofprepareWorkspaceConfigForClient.All IPC handlers now correctly use the helper to ensure default workspaces display with
'My Workspace'name and'default'type. This is a clean and maintainable pattern.Also applies to: 92-119, 121-159, 376-399, 497-512, 537-554, 590-610, 612-661
packages/bruno-electron/src/store/workspace-environments.js (1)
154-172: Good refactor to use locking and atomic writes.The
setActiveGlobalEnvironmentUidmethod now properly useswithLockfor concurrency control andwriteWorkspaceFileAtomicfor safe file updates.Minor note: The existence check on line 161 is outside the lock, so there's a theoretical race if the file is deleted between check and lock acquisition. However,
readWorkspaceConfiginside the lock would throw anyway, making this low-risk.packages/bruno-electron/src/utils/workspace-config.js (3)
12-23: YAML quoting helper is safe but may over-quote.The
quoteYamlValuefunction always wraps values in double quotes, which is safe for special characters but produces more verbose YAML. This is acceptable for robustness.
47-119: Good input validation and sanitization.
isValidCollectionEntry,isValidSpecEntry,sanitizeCollections, andsanitizeSpecsprovide solid defensive checks. Logging invalid entries is helpful for debugging.
272-277: Consistent locking across all mutating operations.All workspace config mutations now use
withLockfor concurrency safety. The pattern is consistently applied and the code is well-structured.Also applies to: 297-308, 310-318, 320-353, 355-396, 428-459, 461-495
packages/bruno-electron/src/store/default-workspace.js (4)
115-122: Good safeguard against runaway directory creation.The
MAX_WORKSPACE_CREATION_ATTEMPTScheck prevents an infinite loop or excessive directory creation. Throwing an error is the right behavior here.
139-151: Clean deferred cleanup pattern.Returning a cleanup function ensures migration data isn't deleted until the workspace is successfully written. This prevents data loss if workspace creation fails.
Also applies to: 225-237
167-180: Good validation of migrated collections.Filtering with
isValidCollectionEntryensures only valid collection entries are migrated. The null-check on line 169 handles edge cases well.
204-205: No changes needed.
stringifyEnvironmentreturns a string synchronously, not a Promise. The code is correct as written. Other files in the codebase unnecessarily await this function, but awaiting a non-Promise value is harmless in JavaScript.
| const writeWorkspaceFileAtomic = async (workspacePath, content) => { | ||
| const workspaceFilePath = path.join(workspacePath, 'workspace.yml'); | ||
| const tempFilePath = path.join(os.tmpdir(), `workspace-${Date.now()}-${Math.random().toString(36).slice(2)}.yml`); | ||
|
|
||
| try { | ||
| await writeFile(tempFilePath, content); | ||
|
|
||
| if (fs.existsSync(workspaceFilePath)) { | ||
| fs.unlinkSync(workspaceFilePath); | ||
| } | ||
|
|
||
| fs.renameSync(tempFilePath, workspaceFilePath); | ||
| } catch (error) { | ||
| if (fs.existsSync(tempFilePath)) { | ||
| try { | ||
| fs.unlinkSync(tempFilePath); | ||
| } catch (_) {} | ||
| } | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Atomic write pattern could lose data on failure.
The current pattern (delete existing → rename temp) isn't truly atomic. If the process crashes or renameSync fails after unlinkSync, the workspace.yml is lost. On POSIX systems, renameSync can atomically replace an existing file. On Windows, you may need a different approach.
🔎 Suggested fix for POSIX systems
const writeWorkspaceFileAtomic = async (workspacePath, content) => {
const workspaceFilePath = path.join(workspacePath, 'workspace.yml');
const tempFilePath = path.join(os.tmpdir(), `workspace-${Date.now()}-${Math.random().toString(36).slice(2)}.yml`);
try {
await writeFile(tempFilePath, content);
- if (fs.existsSync(workspaceFilePath)) {
- fs.unlinkSync(workspaceFilePath);
- }
-
- fs.renameSync(tempFilePath, workspaceFilePath);
+ // On POSIX, rename atomically replaces existing file
+ // On Windows, may need to unlink first or use fs.promises.rename
+ try {
+ fs.renameSync(tempFilePath, workspaceFilePath);
+ } catch (renameErr) {
+ if (renameErr.code === 'EXDEV') {
+ // Cross-device move: copy then delete
+ fs.copyFileSync(tempFilePath, workspaceFilePath);
+ fs.unlinkSync(tempFilePath);
+ } else {
+ throw renameErr;
+ }
+ }
} catch (error) {
if (fs.existsSync(tempFilePath)) {
try {
fs.unlinkSync(tempFilePath);
} catch (_) {}
}
throw error;
}
};🤖 Prompt for AI Agents
In packages/bruno-electron/src/utils/workspace-config.js around lines 25–45, the
current sequence deletes the target then renames the temp file, which can lose
workspace.yml if the process crashes or rename fails; instead write the temp
file into the same directory as workspace.yml and perform a single atomic
replace by calling fs.renameSync(tempFilePath, workspaceFilePath) without
unlinking the existing file first; additionally add a Windows-safe fallback: if
renameSync throws (expected on some Windows edge-cases), attempt
fs.copyFileSync(tempFilePath, workspaceFilePath) followed by
fs.unlinkSync(tempFilePath) inside its own try/catch, and preserve the existing
cleanup of the temp file in the outer catch before rethrowing the original
error.
| const acquireLock = async (key, timeout = 10000) => { | ||
| const startTime = Date.now(); | ||
|
|
||
| while (locks.has(key)) { | ||
| if (Date.now() - startTime > timeout) { | ||
| throw new Error(`Lock acquisition timeout for: ${key}`); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); | ||
| } | ||
|
|
||
| let releaseFn; | ||
| const releasePromise = new Promise((resolve) => { | ||
| releaseFn = resolve; | ||
| }); | ||
|
|
||
| locks.set(key, releasePromise); | ||
|
|
||
| return () => { | ||
| locks.delete(key); | ||
| releaseFn(); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Potential race condition in polling-based lock acquisition.
The polling loop checks locks.has(key) then sets the lock, but another caller could pass the same check between the while exit and locks.set. This is a classic TOCTOU (time-of-check to time-of-use) issue.
Consider using a promise-based queue pattern where waiters subscribe to the release promise rather than polling.
🔎 Suggested alternative using a queue-based approach
-const locks = new Map();
+const lockQueues = new Map();
const acquireLock = async (key, timeout = 10000) => {
- const startTime = Date.now();
-
- while (locks.has(key)) {
- if (Date.now() - startTime > timeout) {
- throw new Error(`Lock acquisition timeout for: ${key}`);
- }
- await new Promise((resolve) => setTimeout(resolve, 50));
+ const startTime = Date.now();
+
+ if (!lockQueues.has(key)) {
+ lockQueues.set(key, Promise.resolve());
}
-
- let releaseFn;
- const releasePromise = new Promise((resolve) => {
- releaseFn = resolve;
- });
-
- locks.set(key, releasePromise);
-
- return () => {
- locks.delete(key);
- releaseFn();
- };
+
+ let release;
+ const currentQueue = lockQueues.get(key);
+ const newQueue = new Promise((resolve) => {
+ release = resolve;
+ });
+
+ lockQueues.set(key, newQueue);
+
+ const timeoutPromise = new Promise((_, reject) => {
+ setTimeout(() => reject(new Error(`Lock acquisition timeout for: ${key}`)),
+ Math.max(0, timeout - (Date.now() - startTime)));
+ });
+
+ await Promise.race([currentQueue, timeoutPromise]);
+
+ return release;
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/bruno-electron/src/utils/workspace-lock.js around lines 3 to 24, the
polling-based acquireLock has a TOCTOU race where another caller can pass the
while check before this caller sets the lock; replace the polling with a
queue-based per-key waiter list: maintain a Map from key to an array of resolver
functions, and when acquiring push a resolver and if you are the first entry you
immediately own the lock, otherwise create and await a promise that the resolver
will resolve; on release shift the queue and call the next resolver to wake the
next waiter; implement timeout by storing a timer that rejects/removes the
waiter if it expires, and ensure both successful release and timeout clean up
the map entry when the queue becomes empty.
|
|
||
| const writeWorkspaceFileAtomic = async (workspacePath, content) => { | ||
| const workspaceFilePath = path.join(workspacePath, 'workspace.yml'); | ||
| const tempFilePath = path.join(os.tmpdir(), `workspace-${Date.now()}-${Math.random().toString(36).slice(2)}.yml`); |
There was a problem hiding this comment.
use randomBytes from node:crypto instead
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/bruno-electron/src/utils/workspace-config.js (1)
26-46: Atomic write pattern could lose data on failure.The current sequence (delete existing → rename temp) creates a window where workspace.yml doesn't exist. If the process crashes or
renameSyncfails afterunlinkSync, data is lost. On POSIX systems,renameSynccan atomically replace an existing file without unlinking first.Note: This issue was already flagged in previous review comments. The suggested fix involves calling
renameSyncdirectly without unlinking first, with a fallback for cross-device moves.
🧹 Nitpick comments (3)
packages/bruno-electron/src/store/default-workspace.js (1)
167-180: Consider a more explicit filter pattern.The current approach maps invalid entries to
nullthen filters them out. While functional, it's clearer to filter first or use a more explicit pattern.🔎 Alternative approach
- const collections = lastOpenedCollections - .map((collectionPath) => { - if (!collectionPath || typeof collectionPath !== 'string') { - return null; - } - const absolutePath = path.resolve(collectionPath); - const collectionName = path.basename(absolutePath); - - return { - path: absolutePath, - name: collectionName - }; - }) - .filter((collection) => isValidCollectionEntry(collection)); + const collections = lastOpenedCollections + .filter((collectionPath) => collectionPath && typeof collectionPath === 'string') + .map((collectionPath) => { + const absolutePath = path.resolve(collectionPath); + const collectionName = path.basename(absolutePath); + + return { + path: absolutePath, + name: collectionName + }; + }) + .filter((collection) => isValidCollectionEntry(collection));packages/bruno-electron/src/utils/workspace-config.js (2)
13-24: Consider additional YAML special characters.The current escaping handles backslashes and double quotes, which is appropriate for workspace names, types, and paths. However, if this function is intended for general YAML value quoting, consider handling newlines (
\n), tabs (\t), and carriage returns (\r).For the current use case (names, types, paths), the implementation is sufficient.
263-266: Consider validating activeEnvironmentUid is non-empty.The
typeofcheck ensures it's a string, but an empty string would still pass validation. Consider adding a length check to prevent writing invalid UIDs.- if (config.activeEnvironmentUid && typeof config.activeEnvironmentUid === 'string') { + if (config.activeEnvironmentUid && typeof config.activeEnvironmentUid === 'string' && config.activeEnvironmentUid.trim() !== '') { yamlLines.push(''); yamlLines.push(`activeEnvironmentUid: ${config.activeEnvironmentUid}`); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-electron/src/store/default-workspace.jspackages/bruno-electron/src/utils/workspace-config.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-electron/src/store/default-workspace.jspackages/bruno-electron/src/utils/workspace-config.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-electron/src/store/default-workspace.jspackages/bruno-electron/src/utils/workspace-config.js
🧬 Code graph analysis (1)
packages/bruno-electron/src/store/default-workspace.js (1)
packages/bruno-electron/src/utils/workspace-config.js (2)
path(2-2)isValidCollectionEntry(48-62)
⏰ 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: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - macOS
🔇 Additional comments (14)
packages/bruno-electron/src/store/default-workspace.js (5)
8-13: LGTM: Imports properly structured.The new workspace-config imports are correctly destructured and all are used in the migration and validation flows.
115-122: Good defensive programming.The hard cap prevents potential infinite loops and throws a clear error message when too many workspace directories already exist. The limit of 20 attempts is reasonable.
139-151: LGTM: Deferred cleanup pattern is sound.Capturing the cleanup function and invoking it after workspace.yml creation ensures the workspace is fully initialized before performing destructive cleanup operations. This prevents data loss if workspace creation fails.
193-195: Good defensive validation.Skipping environments with invalid or missing names prevents migration errors and ensures only valid environment entries are processed.
225-237: Verify Store instantiation in cleanup function.The cleanup function creates a new
Storeinstance forglobal-environments. Confirm this is the correct approach versus reusingglobalEnvironmentsStoreor passing the store instance as a closure variable. Creating a fresh instance may be intentional to avoid stale references after migration completes.If creating a new instance is required, consider adding a comment explaining why:
// Create fresh store instance to ensure we're clearing the latest state const globalEnvStore = new Store({ name: 'global-environments' });packages/bruno-electron/src/utils/workspace-config.js (9)
3-8: LGTM: Imports follow Node.js best practices.The use of
node:cryptoprefix is the recommended approach for Node.js built-in modules. This also addresses the previous feedback to userandomBytesfrom the crypto module.
48-78: LGTM: Validation covers essential requirements.The validation correctly checks for valid objects with non-empty name and path strings. This matches the validation logic referenced in the external context snippet.
If stricter validation is needed in the future (e.g., path format validation, name character restrictions), it can be added here centrally.
80-120: Solid sanitization logic with good error visibility.The functions properly validate entries, log errors for debugging, trim strings, and handle optional fields. Returning empty arrays for non-array inputs prevents downstream errors.
274-319: LGTM: Consistent locking pattern across update operations.The use of
withLockfor all config mutations prevents race conditions. The pattern of lock → read → modify → write atomically → unlock is correctly implemented acrosswriteWorkspaceConfig,updateWorkspaceName, andupdateWorkspaceDocs.
321-354: Good defensive validation and locking.The function validates input with
isValidCollectionEntrybefore proceeding, uses locking to prevent races, and correctly updates or adds collections based on path matching.
356-397: Solid removal logic with helpful metadata.The function correctly normalizes paths before comparison and returns metadata (
removedCollection,shouldDeleteFiles) to help callers handle post-removal cleanup. The logic for determiningshouldDeleteFilesbased on remote presence and path type is appropriate.
462-496: LGTM: Removal logic matches collection removal pattern.The function correctly normalizes paths, uses locking, and returns helpful metadata for callers.
525-529: LGTM: New exports properly exposed.The additions of
writeWorkspaceFileAtomic,isValidCollectionEntry, andisValidSpecEntryto the module exports are correct and align with their usage in the default-workspace store and other files.
429-460: The OR matching logic appears intentional for idempotency, but clarify if the update behavior is desired.The caller in
apiSpecs.jsalso uses OR logic when checking for existing specs, so the matching pattern is consistent and intentional. However, the behavior still merits clarification: if someone tries to add a spec with the same name but different path (or vice versa), it will update the existing spec rather than add a new one. Confirm whether this update-on-match behavior is the intended design or if stricter matching—such as by path alone—would be more appropriate.
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
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.