WIP: feat(naming-collisions)/handle naming collision across clone, create, rename and drag & drop#8423
Conversation
…/naming-collisions/02-electron-authoritative-copy-clone
…/02-electron-authoritative-copy-clone feat/electron-filename-copy-paste-clone
…ollisions/02-electron-authoritative-copy-clone Revert "feat/electron-filename-copy-paste-clone"
sachin-thakur-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
sachin-thakur-bruno
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces shared filesystem-safe naming utilities ( ChangesCollision-safe naming overhaul & one-click clone
Sequence Diagram(s)sequenceDiagram
participant UI as CollectionItem / Redux Action
participant IPC as Electron IPC Handler
participant FS as filesystem.js (writeFileUnique / mkdirUnique)
UI->>IPC: renderer:new-request / clone-folder / move-item (name, path)
IPC->>FS: writeFileUnique / mkdirUnique / getUniqueTargetPath
FS-->>IPC: resolved pathname (numeric suffix if collision)
IPC-->>UI: { pathname, filename } / { newPathname }
UI->>UI: enqueue OPEN_REQUEST with result?.pathname
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-electron/src/ipc/collection.js (1)
581-601: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
targetFilenamebefore passing it towriteFileUnique().
targetDirnameis validated, buttargetFilenameis renderer-controlled and becomes the base path segment. A value containing path separators can escape the target directory beforewriteFileUnique()writes the file.Suggested guard
const filename = targetFilename || path.basename(sourcePathname); +if (path.basename(filename) !== filename || path.win32.basename(filename) !== filename) { + throw new Error(`${filename} is not a valid filename`); +} const filenameWithoutExt = filename.replace(/\.(bru|yml)$/, ''); +if (!validateName(filenameWithoutExt)) { + throw new Error(`${filename} is not a valid filename`); +}As per path instructions, path handling must account for platform differences.
Also applies to: 2274-2279
🤖 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/ipc/collection.js` around lines 581 - 601, Validate the renderer-controlled targetFilename before it is used to derive the base name for writeFileUnique() in the collection save flow. Add a path-safety check in the function that computes filename/filenameWithoutExt so targetFilename cannot contain separators or any value that would escape targetDirname, and make the validation platform-aware by using path parsing/normalization rather than assuming one separator style. Keep the guard close to the existing filename resolution logic so the same protection applies wherever writeFileUnique() is called in this code path, including the other referenced save locations.Source: Path instructions
🧹 Nitpick comments (2)
packages/bruno-electron/src/utils/tests/filesystem/index.spec.js (1)
64-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd behavioral coverage for the atomic create helpers.
This only asserts the computed target name. Please add tests for
writeFileUnique()andmkdirUnique()creatingname, thenname1under collision, ideally with parallelPromise.all()calls to exercise theEEXISTretry path. As per coding guidelines, “Add tests for any new functionality or meaningful changes.”🤖 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/utils/tests/filesystem/index.spec.js` around lines 64 - 74, The current filesystem spec only covers getUniqueTargetPath, but it misses behavioral coverage for the atomic create helpers. Update the tests in index.spec.js to exercise writeFileUnique() and mkdirUnique() directly: verify they create the original name on first call and name1 when the target already exists, ideally using parallel Promise.all() calls to trigger the EEXIST retry path. Use the unique symbols getUniqueTargetPath, writeFileUnique, and mkdirUnique to locate the relevant test block and add the new assertions there.Source: Coding guidelines
packages/bruno-electron/src/utils/filesystem.js (1)
366-372: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNormalize and clean up directory lock keys.
Use a resolved key and delete completed entries; otherwise equivalent directory spellings can bypass serialization, and
dirLockChainsgrows for every unique directory touched.🤖 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/utils/filesystem.js` around lines 366 - 372, The withDirLock helper currently uses the raw dirname as the dirLockChains key and never removes finished entries, so equivalent path spellings can bypass serialization and the map can grow unbounded. Update withDirLock to normalize/resolve the directory before looking up or setting the lock chain, and ensure the dirLockChains entry is deleted once the operation settles while still preserving the existing sequential execution behavior around fn().
🤖 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/components/Sidebar/Collections/Collection/CollectionItem/index.js`:
- Around line 603-608: The one-click clone handler in CollectionItem currently
allows every sidebar item to call cloneItem, but that action only supports
folders and requests, so standalone app items can take the wrong path. Update
handleCloneItem to first gate cloning with isItemARequest(item) (or add explicit
app cloning support before dispatching cloneItem), and keep the success/error
toast behavior only for the supported item types.
- Around line 603-608: Add a behavioral test for handleCloneItem in
CollectionItem to cover the one-click clone flow for both folder and request
items, asserting the success toast on a resolved cloneItem dispatch and the
error toast on a rejected dispatch. Use the existing CollectionItem clone action
logic to verify the sidebar behavior, but do not duplicate the filename-suffix
resolution cases already covered in the filesystem helper tests.
In `@packages/bruno-common/src/utils/naming.spec.ts`:
- Around line 39-42: The current Windows reserved-name coverage in
naming.spec.ts only checks bare device names, so extend the existing
validateName and validateNameError tests to cover the real cross-platform edge
cases: reject reserved names with extensions like CON.txt, and confirm COM0/LPT0
remain valid. Update the assertions around validateName and validateNameError so
the reserved-name handling is locked down for both plain and dotted inputs
without introducing OS-specific behavior.
In `@packages/bruno-common/src/utils/naming.ts`:
- Line 12: The Windows reserved-name check in reservedDeviceNames is too narrow
and also rejects invalid COM0/LPT0 cases, so update the matcher in naming.ts to
treat device names as reserved even when followed by a file extension (for
example CON.txt or NUL.bru) while only reserving COM1-9 and LPT1-9. Make the
same adjustment wherever this helper is used in the shared naming validation
path so create/rename behavior stays OS-agnostic across the cross-platform app.
In `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 1598-1655: The IPC move handlers currently trust renderer-provided
paths and only check existence, which leaves copy/delete/write operations
exposed to out-of-scope paths. In ipcMain.handle('renderer:move-item') and
ipcMain.handle('renderer:move-item-cross-format'), add collection-boundary
validation for both sourcePathname and targetDirname before calling
copyPathTo(), writeFileUnique(), or removePath(), and reject if either path is
outside the allowed collection. Keep the checks alongside the existing
fs.existsSync and same-folder logic so the move flow only proceeds after both
paths are verified.
- Line 1044: The rename flow is using the IPC-provided newPath before validating
the requested filename, so adjust the main-process rename handler to derive the
destination path from oldPath plus a validated newFilename first, then pass that
computed path into getUniqueRenamePath(). Keep the validation and path
construction together in the same rename logic so untrusted IPC input is not
used for collision resolution until after it has been sanitized.
In `@packages/bruno-electron/src/utils/filesystem.js`:
- Around line 123-128: The retry loop in
`getSafePathToWrite()`/`nextSuffixedName()` can truncate away the numeric
suffix, causing every `EEXIST` retry to map to the same path. Update the
filename generation flow so `getSafeUniqueFilename()` reserves space for both
the suffix and extension before truncation, and have the write loop use that
safe unique name when building `pathname`.
- Around line 347-356: The copy flow in copyPathTo/getUniqueTargetPath is still
race-prone and can overwrite or recursively copy into itself. Update the target
selection and copy checks to claim the destination exclusively before writing,
and add an OS-agnostic guard using path.resolve()/path.relative() to reject any
directory copy where targetPath resolves inside source. Also ensure
mkdir/copyFile paths fail instead of merging existing directories or overwriting
files after the uniqueness check.
---
Outside diff comments:
In `@packages/bruno-electron/src/ipc/collection.js`:
- Around line 581-601: Validate the renderer-controlled targetFilename before it
is used to derive the base name for writeFileUnique() in the collection save
flow. Add a path-safety check in the function that computes
filename/filenameWithoutExt so targetFilename cannot contain separators or any
value that would escape targetDirname, and make the validation platform-aware by
using path parsing/normalization rather than assuming one separator style. Keep
the guard close to the existing filename resolution logic so the same protection
applies wherever writeFileUnique() is called in this code path, including the
other referenced save locations.
---
Nitpick comments:
In `@packages/bruno-electron/src/utils/filesystem.js`:
- Around line 366-372: The withDirLock helper currently uses the raw dirname as
the dirLockChains key and never removes finished entries, so equivalent path
spellings can bypass serialization and the map can grow unbounded. Update
withDirLock to normalize/resolve the directory before looking up or setting the
lock chain, and ensure the dirLockChains entry is deleted once the operation
settles while still preserving the existing sequential execution behavior around
fn().
In `@packages/bruno-electron/src/utils/tests/filesystem/index.spec.js`:
- Around line 64-74: The current filesystem spec only covers
getUniqueTargetPath, but it misses behavioral coverage for the atomic create
helpers. Update the tests in index.spec.js to exercise writeFileUnique() and
mkdirUnique() directly: verify they create the original name on first call and
name1 when the target already exists, ideally using parallel Promise.all() calls
to trigger the EEXIST retry path. Use the unique symbols getUniqueTargetPath,
writeFileUnique, and mkdirUnique to locate the relevant test block and add the
new assertions there.
🪄 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: 605dc777-69b4-479b-8b40-cb304403441b
📒 Files selected for processing (9)
packages/bruno-app/src/components/Sidebar/Collections/Collection/CollectionItem/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/actions.jspackages/bruno-app/src/utils/common/regex.jspackages/bruno-common/src/utils/index.tspackages/bruno-common/src/utils/naming.spec.tspackages/bruno-common/src/utils/naming.tspackages/bruno-electron/src/ipc/collection.jspackages/bruno-electron/src/utils/filesystem.jspackages/bruno-electron/src/utils/tests/filesystem/index.spec.js
| it('rejects Windows reserved device names', () => { | ||
| expect(validateName('CON')).toBe(false); | ||
| expect(validateName('LPT1')).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the Windows reserved-name edge cases here.
This only covers bare CON/LPT1, so it would miss the real cross-platform regression: CON.txt should be rejected, while COM0/LPT0 should stay valid. Please lock those cases down in both validateName and validateNameError.
As per coding guidelines, Add tests for any new functionality or meaningful changes. As per path instructions, Bruno is a cross-platform Electron desktop app that runs on macOS, Windows, and Linux. Ensure that all code is OS-agnostic.
Also applies to: 59-64
🤖 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-common/src/utils/naming.spec.ts` around lines 39 - 42, The
current Windows reserved-name coverage in naming.spec.ts only checks bare device
names, so extend the existing validateName and validateNameError tests to cover
the real cross-platform edge cases: reject reserved names with extensions like
CON.txt, and confirm COM0/LPT0 remain valid. Update the assertions around
validateName and validateNameError so the reserved-name handling is locked down
for both plain and dotted inputs without introducing OS-specific behavior.
Sources: Coding guidelines, Path instructions
| if (!safeToRename(oldPath, newPath)) { | ||
| throw new Error(`path: ${newPath} already exists`); | ||
| } | ||
| newPath = getUniqueRenamePath(oldPath, newPath); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Validate and derive newPath before collision resolution.
newPath is accepted from IPC and passed into getUniqueRenamePath() before newFilename validation. Derive the destination from oldPath + a validated filename in the main process, then resolve collisions.
🤖 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/ipc/collection.js` at line 1044, The rename flow
is using the IPC-provided newPath before validating the requested filename, so
adjust the main-process rename handler to derive the destination path from
oldPath plus a validated newFilename first, then pass that computed path into
getUniqueRenamePath(). Keep the validation and path construction together in the
same rename logic so untrusted IPC input is not used for collision resolution
until after it has been sanitized.
There was a problem hiding this comment.
Before our work the handler also trusted newPath (it did safeToRename(oldPath, newPath) then threw). We replaced the throw with getUniqueRenamePath(...). So now, instead of rejecting a collision, we resolve and rename to a path derived from the renderer's newPath.
Adding a validation for this.
There was a problem hiding this comment.
@sachin-thakur-bruno Agreed — good clarification. This wasn’t introduced by your change; the handler already trusted the IPC-provided newPath before, and this PR just changed the collision behavior from reject to suffix.
Adding validation/deriving the destination in the main process is still the right hardening step here, so that getUniqueRenamePath() only runs on a validated path built from oldPath + newFilename. That would address my concern.
🧠 Learnings used
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.
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 8387
File: packages/bruno-cli/src/utils/persist-variables.js:330-337
Timestamp: 2026-06-26T21:24:02.726Z
Learning: In the usebruno/bruno repository, the codebase is standardized to LF line endings. During code review, do not flag CRLF-preservation/line-ending change concerns for these text-based source/config files unless the file format or an explicit workflow/tooling requirement requires preserving CRLF (e.g., a Windows-specific file type or a mandated EOL rule).
|
@sachin-thakur-bruno Would love to see a good bunch of e2e tests for this given that this is a bug prone path. |
Yes I will be adding e2e tests for all scenarios. |
2 participants
Description
https://usebruno.atlassian.net/browse/BRU-894
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