fix(collection-watcher): prevent crash when deleting collections#7470
Merged
bijin-bruno merged 6 commits intousebruno:mainfrom Mar 13, 2026
Merged
Conversation
… deletion When deleting an OpenAPI-synced collection, saveBrunoConfig() writes to bruno.json which creates buffered chokidar events (80ms stabilityThreshold). If the collection directory is removed before those events fire, getCollectionFormat() throws "No collection configuration found" for each .bru file in the collection. Add fs.existsSync(collectionPath) guards in the change, unlink, and unlinkDir handlers to bail out early when the collection root no longer exists.
abhishek-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds defensive try/catch guards and existence checks to collection watcher unlink/unlinkDir handlers, plus explicit error logging; introduces an end-to-end Playwright test for collection deletion and a test helper to perform the deletion from the Workspace Overview. Changes
Sequence Diagram(s)(Skipped — changes are defensive handlers and a test; no new multi-component control flow requiring diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Line 424: The TOCTOU gap occurs because getCollectionFormat(collectionPath) is
called after an initial fs.existsSync check in change, unlink, and unlinkDir; if
the directory vanishes in between it throws an unhandled "No collection
configuration found" error. Immediately after each existsSync(collectionPath)
guard in the change, unlink, and unlinkDir code paths, call
getCollectionFormat(collectionPath) inside a try-catch (before any other
dependent operations) and handle the error by logging or returning early (same
behavior as when the existsSync guard fails) so the functions don't proceed when
the collection config is gone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5109df1a-a2c8-48be-b4f9-95ea3abadb9c
📒 Files selected for processing (1)
packages/bruno-electron/src/app/collection-watcher.js
| }; | ||
|
|
||
| const change = async (win, pathname, collectionUid, collectionPath) => { | ||
| if (!fs.existsSync(collectionPath)) return; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/bruno-electron/src/app && wc -l collection-watcher.jsRepository: usebruno/bruno
Length of output: 83
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '420,430p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 518
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '550,570p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 701
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '580,595p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 498
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '424,530p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 2999
🏁 Script executed:
cd packages/bruno-electron/src/app && rg -A 5 'const isFolderRootFile|function isFolderRootFile' collection-watcher.jsRepository: usebruno/bruno
Length of output: 277
🏁 Script executed:
cd packages/bruno-electron/src/app && rg -B 3 -A 3 'getCollectionFormat' collection-watcher.js | head -80Repository: usebruno/bruno
Length of output: 1946
🏁 Script executed:
cd packages/bruno-electron/src && rg -A 15 'const getCollectionFormat|function getCollectionFormat' utils/filesystem.jsRepository: usebruno/bruno
Length of output: 570
🏁 Script executed:
cd packages/bruno-electron/src && rg -B 2 -A 2 'No collection configuration found' .Repository: usebruno/bruno
Length of output: 254
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '506,520p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 493
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '560,575p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 520
🏁 Script executed:
cd packages/bruno-electron/src/app && sed -n '588,605p' collection-watcher.jsRepository: usebruno/bruno
Length of output: 532
Harden against TOCTOU after the new existsSync guards.
The guards at lines 424, 556, and 584 help, but getCollectionFormat(collectionPath) is still called later without protection in all three functions. In unlink (line 563) and unlinkDir (line 591), the call is entirely outside any try-catch. In change (line 527), there's an uncaught call to getCollectionFormat. If the collection folder disappears between the initial existsSync check and the getCollectionFormat call, an unhandled "No collection configuration found" error will be thrown.
Wrap getCollectionFormat(collectionPath) in a try-catch block immediately after each existsSync check, before any dependent operations, to close the TOCTOU window.
Suggested hardening patch
const change = async (win, pathname, collectionUid, collectionPath) => {
if (!fs.existsSync(collectionPath)) return;
+ let format;
+ try {
+ format = getCollectionFormat(collectionPath);
+ } catch (err) {
+ return;
+ }
@@
- if (isFolderRootFile(pathname, collectionPath)) {
+ if (path.basename(pathname) === `folder.${format}`) {
@@
- let format = getCollectionFormat(collectionPath);
let content = fs.readFileSync(pathname, 'utf8');
file.data = await parseFolder(content, { format });
@@
- const format = getCollectionFormat(collectionPath);
if (hasRequestExtension(pathname, format)) {
@@
const unlink = (win, pathname, collectionUid, collectionPath) => {
if (!fs.existsSync(collectionPath)) return;
+ let format;
+ try {
+ format = getCollectionFormat(collectionPath);
+ } catch (err) {
+ return;
+ }
@@
- const format = getCollectionFormat(collectionPath);
if (hasRequestExtension(pathname, format)) {
@@
const unlinkDir = async (win, pathname, collectionUid, collectionPath) => {
if (!fs.existsSync(collectionPath)) return;
+ let format;
+ try {
+ format = getCollectionFormat(collectionPath);
+ } catch (err) {
+ return;
+ }
@@
- const format = getCollectionFormat(collectionPath);
const folderFilePath = path.join(pathname, `folder.${format}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-electron/src/app/collection-watcher.js` at line 424, The
TOCTOU gap occurs because getCollectionFormat(collectionPath) is called after an
initial fs.existsSync check in change, unlink, and unlinkDir; if the directory
vanishes in between it throws an unhandled "No collection configuration found"
error. Immediately after each existsSync(collectionPath) guard in the change,
unlink, and unlinkDir code paths, call getCollectionFormat(collectionPath)
inside a try-catch (before any other dependent operations) and handle the error
by logging or returning early (same behavior as when the existsSync guard fails)
so the functions don't proceed when the collection config is gone.
Added logic to remove the collection from the watcher when deleting files, preventing chokidar from firing events on a directory that is being removed. This change enhances stability during collection deletions by ensuring the watcher is properly managed.
abhishek-bruno
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`:
- Around line 161-167: The current flow calls
ipcRenderer.invoke('renderer:remove-collection', ...) which mutates
workspace.yml before you stop the watcher, causing removedCollection to become
null and preventing deleteFiles from removing the folder; change the sequence so
you first tear down the watcher without mutating the workspace (invoke the
watcher-stop IPC — use 'renderer:remove-collection-from-workspace' or a
dedicated watcher-stop IPC) using collection.pathname/collection.uid/workspaceId
to stop chokidar, then call ipcRenderer.invoke('renderer:remove-collection',
collection.pathname, collection.uid, workspaceId) to mutate workspace.yml and
finally run the deleteFiles branch (or rely on the returned removedCollection)
so the directory removal actually executes.
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 555-582: The unlink handler currently logs all exceptions as
errors even when the collection root is missing during normal delete races;
update the logic in the try/catch around getCollectionFormat/hasRequestExtension
and the overall unlink block (the function handling unlink events with variables
pathname, collectionPath, collectionUid, and calls to isEnvironmentsFolder and
unlinkEnvironmentFile) to treat a missing collection root as an expected flow:
either pre-check that the collection root exists (fs.existsSync(collectionPath)
or equivalent) before proceeding, or in the catch inspect the error (e.g.,
err.code === 'ENOENT' or similar) and return silently instead of calling
console.error; preserve error logging for unexpected errors only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aba4e126-650a-4107-a24f-b833d6577a9c
📒 Files selected for processing (2)
packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.jspackages/bruno-electron/src/app/collection-watcher.js
Comment on lines
+161
to
+167
| // Remove collection from the watcher | ||
| // When deleting files, stop the collection watcher first to prevent | ||
| // chokidar from firing events on a directory that's being removed | ||
| if (deleteFiles && collection) { | ||
| const workspaceId = workspace.pathname || workspace.uid || 'default'; | ||
| await ipcRenderer.invoke('renderer:remove-collection', collection.pathname, collection.uid, workspaceId); | ||
| } |
Contributor
There was a problem hiding this comment.
Pre-removal IPC can prevent deleteFiles from deleting the collection directory.
renderer:remove-collection already removes the collection from workspace.yml. When Line 169 then calls renderer:remove-collection-from-workspace, removedCollection can be null, so the deleteFiles branch may never remove the folder.
Suggested fix (keep watcher teardown, avoid first workspace mutation)
- if (deleteFiles && collection) {
- const workspaceId = workspace.pathname || workspace.uid || 'default';
- await ipcRenderer.invoke('renderer:remove-collection', collection.pathname, collection.uid, workspaceId);
- }
+ if (deleteFiles && collection) {
+ // teardown watcher/runtime state only; let remove-collection-from-workspace
+ // own workspace.yml mutation + file deletion
+ await ipcRenderer.invoke('renderer:remove-collection', collection.pathname, collection.uid);
+ }Also applies to: 169-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/providers/ReduxStore/slices/workspaces/actions.js`
around lines 161 - 167, The current flow calls
ipcRenderer.invoke('renderer:remove-collection', ...) which mutates
workspace.yml before you stop the watcher, causing removedCollection to become
null and preventing deleteFiles from removing the folder; change the sequence so
you first tear down the watcher without mutating the workspace (invoke the
watcher-stop IPC — use 'renderer:remove-collection-from-workspace' or a
dedicated watcher-stop IPC) using collection.pathname/collection.uid/workspaceId
to stop chokidar, then call ipcRenderer.invoke('renderer:remove-collection',
collection.pathname, collection.uid, workspaceId) to mutate workspace.yml and
finally run the deleteFiles branch (or rely on the returned removedCollection)
so the directory removal actually executes.
…g deletion Eliminated the logic for stopping the collection watcher before deletion, streamlining the action for removing collections from workspaces. This change simplifies the code and maintains functionality without unnecessary complexity.
…onality Introduced a new test suite to verify the deletion of collections from the workspace overview. The test ensures that collections are properly removed from both the UI and the file system, confirming the absence of any uncaught errors during the deletion process. This addition enhances the test coverage for collection management features.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/collection/delete/delete-collection.spec.ts (2)
23-36: Extract locators to variables for improved maintainability.Per guidelines, locators should be stored in variables. This improves readability and makes updates easier if selectors change.
♻️ Suggested locator extraction
+ // Define locators + const homeButton = page.locator('.home-button'); + const overviewTab = page.locator('.request-tab').filter({ hasText: 'Overview' }); + const collectionCard = page.locator('.collection-card').filter({ hasText: collectionName }); + const deleteDropdownItem = page.locator('.dropdown-item').filter({ hasText: 'Delete' }); + const deleteModal = page.locator('.bruno-modal').filter({ hasText: 'Delete Collection' }); + // Navigate to Workspace - await page.locator('.home-button').click(); - - // Navigate to workspace overview - const overviewTab = page.locator('.request-tab').filter({ hasText: 'Overview' }); + await homeButton.click(); await overviewTab.click(); - // Find the collection card and open its menu - const collectionCard = page.locator('.collection-card').filter({ hasText: collectionName }); await collectionCard.waitFor({ state: 'visible', timeout: 5000 }); await collectionCard.locator('.collection-menu').click(); - // Click Delete from the dropdown - await page.locator('.dropdown-item').filter({ hasText: 'Delete' }).click(); - - // Wait for delete confirmation modal - const deleteModal = page.locator('.bruno-modal').filter({ hasText: 'Delete Collection' }); + await deleteDropdownItem.click(); await deleteModal.waitFor({ state: 'visible', timeout: 5000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/delete/delete-collection.spec.ts` around lines 23 - 36, Extract the inline locators into clearly named variables to improve maintainability: create variables like homeButtonLocator for page.locator('.home-button'), overviewTabLocator for page.locator('.request-tab').filter({ hasText: 'Overview' }), collectionCardLocator for page.locator('.collection-card').filter({ hasText: collectionName }), collectionMenuLocator from collectionCardLocator.locator('.collection-menu'), and deleteDropdownItemLocator for page.locator('.dropdown-item').filter({ hasText: 'Delete' }); then replace the inline uses in the sequence (clicks and waitFor) with these variables so selectors are centralized and easier to update.
7-66: Wrap logical sections intest.stepfor improved report readability.The test covers the deletion flow well, but per coding guidelines, using
test.stepmakes generated reports easier to read. Consider grouping the logical phases: setup, navigation, deletion, and verification.♻️ Suggested refactor with test.step
test('Delete collection from workspace overview removes files from disk', async ({ page, createTmpDir }) => { const collectionName = 'delete-test-collection'; const tmpDir = await createTmpDir(collectionName); const collectionPath = path.join(tmpDir, collectionName); - // Create a collection with a request - await createCollection(page, collectionName, tmpDir); - await createRequest(page, 'ping', collectionName, { url: 'http://localhost:8081/ping' }); - - // Verify collection directory exists on disk - expect(fs.existsSync(collectionPath)).toBe(true); + await test.step('Create collection with a request', async () => { + await createCollection(page, collectionName, tmpDir); + await createRequest(page, 'ping', collectionName, { url: 'http://localhost:8081/ping' }); + expect(fs.existsSync(collectionPath)).toBe(true); + }); // Capture any uncaught errors during deletion const pageErrors: Error[] = []; page.on('pageerror', (error) => pageErrors.push(error)); - // Navigate to Workspace - await page.locator('.home-button').click(); - - // Navigate to workspace overview - const overviewTab = page.locator('.request-tab').filter({ hasText: 'Overview' }); - await overviewTab.click(); - - // Find the collection card and open its menu - const collectionCard = page.locator('.collection-card').filter({ hasText: collectionName }); - await collectionCard.waitFor({ state: 'visible', timeout: 5000 }); - await collectionCard.locator('.collection-menu').click(); - - // Click Delete from the dropdown - await page.locator('.dropdown-item').filter({ hasText: 'Delete' }).click(); - - // Wait for delete confirmation modal - const deleteModal = page.locator('.bruno-modal').filter({ hasText: 'Delete Collection' }); - await deleteModal.waitFor({ state: 'visible', timeout: 5000 }); - - // Type 'delete' to confirm - await deleteModal.locator('#delete-confirm-input').fill('delete'); - - // Click the Delete button - await deleteModal.getByRole('button', { name: 'Delete', exact: true }).click(); - - // Wait for modal to close and success toast - await deleteModal.waitFor({ state: 'hidden', timeout: 10000 }); + await test.step('Delete collection via workspace overview', async () => { + await page.locator('.home-button').click(); + const overviewTab = page.locator('.request-tab').filter({ hasText: 'Overview' }); + await overviewTab.click(); + + const collectionCard = page.locator('.collection-card').filter({ hasText: collectionName }); + await collectionCard.waitFor({ state: 'visible', timeout: 5000 }); + await collectionCard.locator('.collection-menu').click(); + + await page.locator('.dropdown-item').filter({ hasText: 'Delete' }).click(); + + const deleteModal = page.locator('.bruno-modal').filter({ hasText: 'Delete Collection' }); + await deleteModal.waitFor({ state: 'visible', timeout: 5000 }); + await deleteModal.locator('#delete-confirm-input').fill('delete'); + await deleteModal.getByRole('button', { name: 'Delete', exact: true }).click(); + await deleteModal.waitFor({ state: 'hidden', timeout: 10000 }); + }); - // Verify collection is removed from workspace overview - await expect( - page.locator('.collection-card').filter({ hasText: collectionName }) - ).not.toBeVisible(); - - // Verify collection is removed from sidebar - await expect( - page.locator('#sidebar-collection-name').filter({ hasText: collectionName }) - ).not.toBeVisible(); - - // Verify collection directory is deleted from disk - expect(fs.existsSync(collectionPath)).toBe(false); - - // Verify no uncaught JS errors occurred during deletion - expect(pageErrors).toHaveLength(0); + await test.step('Verify collection removed from UI and disk', async () => { + await expect( + page.locator('.collection-card').filter({ hasText: collectionName }) + ).not.toBeVisible(); + await expect( + page.locator('#sidebar-collection-name').filter({ hasText: collectionName }) + ).not.toBeVisible(); + expect(fs.existsSync(collectionPath)).toBe(false); + expect(pageErrors).toHaveLength(0); + }); });As per coding guidelines: "Promote the use of
test.stepas much as possible so the generated reports are easier to read."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/collection/delete/delete-collection.spec.ts` around lines 7 - 66, The test "Delete collection from workspace overview removes files from disk" should be split into explicit test.step blocks to improve report readability: wrap the setup phase (tmpDir creation, createCollection, createRequest, and initial fs.existsSync check) in an await test.step('setup', async () => { ... }); wrap navigation actions (clicking .home-button, opening Overview tab, locating collection card) in await test.step('navigation', ...); wrap the deletion interaction (opening menu, clicking Delete, filling '#delete-confirm-input', confirming) in await test.step('deletion', ...); and wrap verifications (collection not visible in overview/sidebar, fs.existsSync check, and pageErrors assertion) in await test.step('verification', ...); update references to the existing symbols collectionName, createTmpDir, createCollection, createRequest, collectionCard, deleteModal, and pageErrors when moving their related code into the respective steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/collection/delete/delete-collection.spec.ts`:
- Around line 23-36: Extract the inline locators into clearly named variables to
improve maintainability: create variables like homeButtonLocator for
page.locator('.home-button'), overviewTabLocator for
page.locator('.request-tab').filter({ hasText: 'Overview' }),
collectionCardLocator for page.locator('.collection-card').filter({ hasText:
collectionName }), collectionMenuLocator from
collectionCardLocator.locator('.collection-menu'), and deleteDropdownItemLocator
for page.locator('.dropdown-item').filter({ hasText: 'Delete' }); then replace
the inline uses in the sequence (clicks and waitFor) with these variables so
selectors are centralized and easier to update.
- Around line 7-66: The test "Delete collection from workspace overview removes
files from disk" should be split into explicit test.step blocks to improve
report readability: wrap the setup phase (tmpDir creation, createCollection,
createRequest, and initial fs.existsSync check) in an await test.step('setup',
async () => { ... }); wrap navigation actions (clicking .home-button, opening
Overview tab, locating collection card) in await test.step('navigation', ...);
wrap the deletion interaction (opening menu, clicking Delete, filling
'#delete-confirm-input', confirming) in await test.step('deletion', ...); and
wrap verifications (collection not visible in overview/sidebar, fs.existsSync
check, and pageErrors assertion) in await test.step('verification', ...); update
references to the existing symbols collectionName, createTmpDir,
createCollection, createRequest, collectionCard, deleteModal, and pageErrors
when moving their related code into the respective steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4504b1b-58cd-4c54-ac02-42db53a32d9b
📒 Files selected for processing (1)
tests/collection/delete/delete-collection.spec.ts
…tion Added a new utility function to delete a collection directly from the workspace overview page. This function encapsulates the steps required to navigate the UI, confirm deletion, and ensure the collection is removed from both the interface and the file system. Updated the corresponding test to utilize this new function, enhancing code reusability and test clarity.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/utils/page/actions.ts (1)
326-360: Add verification that collection is removed after deletion.The function completes without confirming the collection card is no longer visible. Compare with
removeCollection(lines 457-459), which verifies the element is gone. Adding a similar assertion here would catch silent failures and align with Playwright best practices for multiple assertions.Suggested verification
// Wait for modal to close await deleteModal.waitFor({ state: 'hidden', timeout: 10000 }); + + // Verify collection card is removed from overview + await expect(collectionCard).not.toBeVisible(); }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/page/actions.ts` around lines 326 - 360, The function deleteCollectionFromOverview currently does not verify the collection was actually removed; after the existing wait for the deleteModal to be hidden, add a post-delete verification that the collection card for collectionName is gone—either by awaiting collectionCard.waitFor({ state: 'hidden', timeout: 5000 }) or using Playwright's assertion (expect(collectionCard).not.toBeVisible()) to match the pattern used in removeCollection; place this check at the end of deleteCollectionFromOverview (after deleteModal.waitFor) to ensure deletion succeeded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/utils/page/actions.ts`:
- Around line 326-360: The function deleteCollectionFromOverview currently does
not verify the collection was actually removed; after the existing wait for the
deleteModal to be hidden, add a post-delete verification that the collection
card for collectionName is gone—either by awaiting collectionCard.waitFor({
state: 'hidden', timeout: 5000 }) or using Playwright's assertion
(expect(collectionCard).not.toBeVisible()) to match the pattern used in
removeCollection; place this check at the end of deleteCollectionFromOverview
(after deleteModal.waitFor) to ensure deletion succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35494f23-ccdd-4320-a602-414dffd2f531
📒 Files selected for processing (2)
tests/collection/delete/delete-collection.spec.tstests/utils/page/actions.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/collection/delete/delete-collection.spec.ts
… error handling Enhanced the unlink and unlinkDir functions to check for the existence of the collection path before proceeding. Added error handling for the getCollectionFormat function to prevent crashes when the collection format cannot be retrieved. These changes improve stability and robustness during collection deletion operations.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/bruno-electron/src/app/collection-watcher.js (2)
568-570:⚠️ Potential issue | 🟡 MinorTreat missing collection config during delete race as expected flow (silent return).
At Line 569 and Line 609,
getCollectionFormatfailures are logged as errors. In deletion races this is expected and can spam logs; prefer silent return here and reserve error logs for unexpected failures only.Suggested noise-reduction patch
try { format = getCollectionFormat(collectionPath); - } catch (error) { - console.error(`Error getting collection format for: ${collectionPath}`, error); + } catch (_error) { return; } @@ try { format = getCollectionFormat(collectionPath); - } catch (error) { - console.error(`Error getting collection format for: ${collectionPath}`, error); + } catch (_error) { return; }Also applies to: 608-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 568 - 570, The catch blocks around the getCollectionFormat call (referenced as getCollectionFormat and using collectionPath) should treat missing-config/delete-race errors as silent returns: update both catch handlers to detect expected "missing collection config" conditions (e.g. error.code === 'ENOENT' or an error.message that indicates no config) and simply return without logging; for all other unexpected errors, keep the existing console.error call and rethrow or return as before. Ensure you change both occurrences that handle getCollectionFormat failures so delete races do not spam the logs.
555-558:⚠️ Potential issue | 🟠 Major
change()still has an unguarded format lookup path.This hardens
unlink/unlinkDir, but the same failure mode remains inchange:isFolderRootFile(...)at Line 501 andgetCollectionFormat(...)at Line 525 can still throw when config disappears mid-delete, leaving a crash path open.Suggested patch to close the remaining `change()` gap
const change = async (win, pathname, collectionUid, collectionPath) => { + if (!fs.existsSync(collectionPath)) { + return; + } + + let format; + try { + format = getCollectionFormat(collectionPath); + } catch (_error) { + return; + } + if (isBrunoConfigFile(pathname, collectionPath)) { @@ - if (isFolderRootFile(pathname, collectionPath)) { + if (path.basename(pathname) === `folder.${format}`) { @@ - let format = getCollectionFormat(collectionPath); let content = fs.readFileSync(pathname, 'utf8'); file.data = await parseFolder(content, { format }); @@ - const format = getCollectionFormat(collectionPath); if (hasRequestExtension(pathname, format)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/app/collection-watcher.js` around lines 555 - 558, In change(), guard against mid-delete config disappearance by checking that collectionPath exists (fs.existsSync(collectionPath)) before calling isFolderRootFile(...) and getCollectionFormat(...), or wrap those calls in a try/catch that returns early on error; specifically update the change function to either return when the path is missing or catch exceptions from isFolderRootFile and getCollectionFormat and treat them as a benign disappearance (no crash), ensuring the function exits cleanly instead of throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-electron/src/app/collection-watcher.js`:
- Around line 568-570: The catch blocks around the getCollectionFormat call
(referenced as getCollectionFormat and using collectionPath) should treat
missing-config/delete-race errors as silent returns: update both catch handlers
to detect expected "missing collection config" conditions (e.g. error.code ===
'ENOENT' or an error.message that indicates no config) and simply return without
logging; for all other unexpected errors, keep the existing console.error call
and rethrow or return as before. Ensure you change both occurrences that handle
getCollectionFormat failures so delete races do not spam the logs.
- Around line 555-558: In change(), guard against mid-delete config
disappearance by checking that collectionPath exists
(fs.existsSync(collectionPath)) before calling isFolderRootFile(...) and
getCollectionFormat(...), or wrap those calls in a try/catch that returns early
on error; specifically update the change function to either return when the path
is missing or catch exceptions from isFolderRootFile and getCollectionFormat and
treat them as a benign disappearance (no crash), ensuring the function exits
cleanly instead of throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5eb0909c-1952-42b6-a46d-a6e3c0b3b1b3
📒 Files selected for processing (1)
packages/bruno-electron/src/app/collection-watcher.js
6 tasks
lohit-bruno
pushed a commit
to lohit-bruno/bruno
that referenced
this pull request
Mar 18, 2026
…bruno#7470) * fix(collection-watcher): guard against events firing after collection deletion When deleting an OpenAPI-synced collection, saveBrunoConfig() writes to bruno.json which creates buffered chokidar events (80ms stabilityThreshold). If the collection directory is removed before those events fire, getCollectionFormat() throws "No collection configuration found" for each .bru file in the collection. Add fs.existsSync(collectionPath) guards in the change, unlink, and unlinkDir handlers to bail out early when the collection root no longer exists. * fix(workspaces): ensure collection watcher stops before deletion Added logic to remove the collection from the watcher when deleting files, preventing chokidar from firing events on a directory that is being removed. This change enhances stability during collection deletions by ensuring the watcher is properly managed. * refactor(workspaces): remove redundant collection watcher logic during deletion Eliminated the logic for stopping the collection watcher before deletion, streamlining the action for removing collections from workspaces. This change simplifies the code and maintains functionality without unnecessary complexity. * test(collection): add integration test for collection deletion functionality Introduced a new test suite to verify the deletion of collections from the workspace overview. The test ensures that collections are properly removed from both the UI and the file system, confirming the absence of any uncaught errors during the deletion process. This addition enhances the test coverage for collection management features. * feat(collection): implement deleteCollectionFromOverview utility function Added a new utility function to delete a collection directly from the workspace overview page. This function encapsulates the steps required to navigate the UI, confirm deletion, and ensure the collection is removed from both the interface and the file system. Updated the corresponding test to utilize this new function, enhancing code reusability and test clarity. * fix(collection-watcher): add guards for collection path existence and error handling Enhanced the unlink and unlinkDir functions to check for the existence of the collection path before proceeding. Added error handling for the getCollectionFormat function to prevent crashes when the collection format cannot be retrieved. These changes improve stability and robustness during collection deletion operations.
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
2 participants
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
Problem
When deleting an OpenAPI-synced collection, saveBrunoConfig() writes to bruno.json which creates buffered chokidar events (due to awaitWriteFinish with 80ms stability threshold). If the collection directory is removed before those buffered events fire, getCollectionFormat() throws "No collection configuration found" — once per .bru file in the collection.
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