fix(receive): rename incoming files on a directory collision - #168
Conversation
The directory-picker save path de-duped names only against the other files in
the same batch, then called getFileHandle(name, { create: true }). A file
already sitting in the chosen folder was overwritten silently, so receiving a
second report.pdf destroyed the first.
uniqueName now probes the folder as well as the batch: the existing file keeps
its name and the incoming one becomes "report (1).pdf". The probe stays cheap,
a single getFileHandle for the exact candidate, never a directory scan.
existsInDir reports three states rather than two. NotFoundError means the slot
is free; any other failure is "unknown", and on that we stop probing and return
the current candidate so the real create:true write surfaces the real error.
Treating an unreadable directory as free would overwrite a file we cannot see,
and treating it as taken would spin forever looking for a free slot.
The dir argument is optional, so the single-file showSaveFilePicker path and
the in-memory tray keep their existing behaviour. In-batch de-duplication is
unchanged: it already worked, and it now has a regression check so it stays
that way.
Verified against the real File System Access API through OPFS: an absent-name
probe throws NotFoundError, and three report.pdf receives into a folder that
already holds one leave four files with the original intact. The pre-fix code
exercised the same way leaves one file with the original's contents replaced.
useWarpTransfer.check.mjs gains collision cases for the on-disk collision, the
three-in-one-batch case, the no-collision case, combined disk and batch
collisions, probe cost, an inconclusive probe, dotfiles, and extensionless
names.
Closes Ishannaik#133
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesFilename collision handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Transfer
participant uniqueName
participant FileSystemDirectoryHandle
Transfer->>uniqueName: resolve candidate filename
uniqueName->>FileSystemDirectoryHandle: probe candidate name
FileSystemDirectoryHandle-->>uniqueName: available, existing, or inconclusive result
uniqueName-->>Transfer: selected filename
Transfer->>FileSystemDirectoryHandle: write selected file
Suggested reviewers: 🚥 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
🤖 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 `@web/src/lib/warp/useWarpTransfer.ts`:
- Around line 793-796: The directory-target write flow around uniqueName must
open each FileSystemWritableFileStream with mode "exclusive" instead of the
default siloed mode. Catch NoModificationAllowedError from createWritable,
generate a new unique name, and retry the write; update the related FsFileHandle
typing and test mocks so createWritable accepts the mode option.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e510328-b408-44bb-8999-12b523204cb4
📒 Files selected for processing (2)
web/src/lib/warp/useWarpTransfer.check.mjsweb/src/lib/warp/useWarpTransfer.ts
| const dir = target.dirHandle; | ||
| // Probes the folder as well as this batch, so an existing file keeps its | ||
| // name and the incoming one becomes "name (1).ext" (issue #133). | ||
| savedName = await uniqueName(usedNames, it.name, dir); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that the local file-handle type accepts createWritable options.
rg -n -C 4 'interface FsFileHandle|createWritable' web/src/lib/warp/peer.ts web/src/lib/warpRepository: Ishannaik/warp
Length of output: 6064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useWarpTransfer.ts relevant sections =="
sed -n '730,805p' web/src/lib/warp/useWarpTransfer.ts
sed -n '805,880p' web/src/lib/warp/useWarpTransfer.ts
echo "== uniqueName definition and dir handling =="
rg -n -C 5 'function uniqueName|const uniqueName|uniqueName\s*=|export.*uniqueName|async function.*dir|dirHandle|createWritable' web/src/lib/warp peer.ts 2>/dev/null | sed -n '1,220p'
echo "== peer.ts target setup relevant section =="
sed -n '180,245p' web/src/lib/warp/peer.ts
echo "== type declarations mentions for FileSystemWritableFileStream mode =="
rg -n 'createWritable|FileSystemWritableFileStream|mode|NoModificationAllowedError' web -g '*.ts' -g '*.tsx' -g '*.d.ts'Repository: Ishannaik/warp
Length of output: 23237
🌐 Web query:
FileSystemFileHandle createWritable mode exclusive default siloed MDN
💡 Result:
In the File System Access API, the createWritable() method of FileSystemFileHandle accepts an optional mode property within its options object to control locking behavior [1][2]. The mode parameter has two possible values: "siloed" (Default) This is the default behavior [1][2]. Multiple FileSystemWritableFileStream writers can be opened simultaneously for the same file handle [2][3]. Each writer operates on its own swap file, and the last writer to close its stream determines the final content of the file [2][3][4]. "exclusive" When set to "exclusive", only one FileSystemWritableFileStream can be open for a given file handle at a time [1][2]. If an attempt is made to open a subsequent writer before the first one is closed, the browser will throw a NoModificationAllowedError [1][2]. This mode is intended to prevent multiple writers from clobbering each other's changes [5][4]. These locking modes were introduced to provide developers with more control over file access, particularly in scenarios involving multiple tabs or windows [3][4][6].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileHandle/createWritable
- 2: https://github.com/mdn/content/blob/main/files/en-us/web/api/filesystemfilehandle/createwritable/index.md
- 3: https://developer.chrome.com/blog/new-dev-trial-for-multiple-readers-and-writers
- 4: https://github.com/whatwg/fs/blob/main/proposals/MultipleReadersWriters.md
- 5: Add new locking modes to file handle primitives whatwg/fs#151
- 6: https://tag-github-bot.w3.org/gh/w3ctag/design-reviews/845
Use an exclusive writable stream for directory targets.
createWritable() defaults to mode: "siloed", so concurrent receives can open writers for the same name (1).ext and the last writer wins. Open the FileSystemWritableFileStream with { mode: "exclusive" }, catch NoModificationAllowedError, retry with a new unique name, and update the FsFileHandle/test mocks to support createWritable({ mode }).
🤖 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 `@web/src/lib/warp/useWarpTransfer.ts` around lines 793 - 796, The
directory-target write flow around uniqueName must open each
FileSystemWritableFileStream with mode "exclusive" instead of the default siloed
mode. Catch NoModificationAllowedError from createWritable, generate a new
unique name, and retry the write; update the related FsFileHandle typing and
test mocks so createWritable accepts the mode option.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return err?.name === "NotFoundError" ? false : "unknown"; | ||
| } | ||
| } | ||
|
|
||
| async function uniqueName(used, name, dir) { | ||
| const dot = name.lastIndexOf("."); | ||
| const stem = dot > 0 ? name.slice(0, dot) : name; | ||
| const ext = dot > 0 ? name.slice(dot) : ""; | ||
|
|
||
| let candidate = name; | ||
| for (let n = 1; ; n += 1) { | ||
| if (!used.has(candidate)) { | ||
| const onDisk = dir ? await existsInDir(dir, candidate) : false; | ||
| if (onDisk !== true) { | ||
| used.add(candidate); | ||
| return candidate; | ||
| } | ||
| } | ||
| candidate = `${stem} (${n})${ext}`; | ||
| } | ||
| } | ||
|
|
||
| /** A fake directory handle over a set of existing names. Records every probe. */ | ||
| function fakeDir(existing = [], opts = {}) { | ||
| const files = new Set(existing); | ||
| return { | ||
| files, | ||
| probes: [], | ||
| created: [], | ||
| async getFileHandle(name, options) { | ||
| if (options?.create) { | ||
| this.created.push(name); | ||
| files.add(name); |
There was a problem hiding this comment.
existsInDir and uniqueName in this file are manually duplicated from useWarpTransfer.ts (the comment even calls this out: "reproduced 1:1"). Because the test is run with bare node and can't import TypeScript, that constraint is real — but it means any future change to the production algorithm that isn't mirrored here will pass all 9 collision checks while silently testing the old logic. The "1:1" comment depends on a human catching the drift, not tooling.
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/lib/warp/useWarpTransfer.check.mjs
Line: 461-493
Comment:
**Test helpers hand-copied from production source**
`existsInDir` and `uniqueName` in this file are manually duplicated from `useWarpTransfer.ts` (the comment even calls this out: "reproduced 1:1"). Because the test is run with bare `node` and can't import TypeScript, that constraint is real — but it means any future change to the production algorithm that isn't mirrored here will pass all 9 collision checks while silently testing the old logic. The "1:1" comment depends on a human catching the drift, not tooling.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Ishannaik
left a comment
Ishannaik
left a comment
There was a problem hiding this comment.
Independent review — APPROVE
Verified locally on pr-168: all 8 engine harnesses pass, including 13 collision assertions covering every acceptance criterion in #133 (disk collision, batch triple, no-collision passthrough, composed disk+batch, probe economy, unknown-probe handling, no-dir path, dotfiles, extensionless). Lint/typecheck/build clean.
Logic review: no off-by-one in the (n) sequence; dot > 0 keeps dotfiles whole; the three-state probe is the right call and the "unknown" handling (defer to the real create:true write) avoids both clobbering unseen files and infinite loops. The auto-resume path reuses durable registry entries, so reconnects do not re-resolve names. Single-file picker, IDB, and memory paths untouched.
On the bot findings:
- CodeRabbit's mode:"exclusive" idea is reasonable hardening but note it is a concurrent-writer lock per MDN, not a file-existence check, so it narrows rather than closes the probe-to-create race. The race needs two simultaneous receives into the same folder, which the current single-pairing flow does not produce. Fine as a follow-up.
- Greptile's 1:1-copied-test-helpers point is valid and inherent to bare-node harnesses; the copies match production exactly today.
Nice, well-scoped fix. Ship it.
2 participants
What & why
The directory-picker save path de-duped filenames only against the other files in the same batch, then called
getFileHandle(name, { create: true }). A file already sitting in the chosen folder was overwritten silently, so receiving a secondreport.pdfdestroyed the first.uniqueNamenow probes the folder as well as the batch: the existing file keeps its name and the incoming one becomesreport (1).pdf. The probe stays cheap — a singlegetFileHandlefor the exact candidate, never a directory scan.existsInDirreports three states rather than two.NotFoundErrormeans the slot is free; any other failure is"unknown", and on that we stop probing and return the current candidate so the realcreate: truewrite surfaces the real error. Treating an unreadable directory as free would overwrite a file we cannot see, and treating it as taken would spin forever looking for a free slot.The
dirargument is optional, so the single-fileshowSaveFilePickerpath and the in-memory tray keep their existing behaviour.One note on the acceptance criteria: in-batch de-duplication already worked before this change — three same-named files in one batch already became
a.txt,a (1).txt,a (2).txt. Only the on-disk case was broken. That criterion now has a regression check so it stays working, but it isn't a behaviour change.Closes #133
Type
Checklist
pnpm lint && pnpm typecheckpass locallypnpm --filter @warp/web buildis green (web changes)web/src/lib/warp/*.check.mjsharness passes (and covers the new behaviour)This is a web-only, two-file engine change, so per the template's "delete lines that genuinely don't apply" I've dropped the server-test and mobile-width rows: the diff contains no
server/files and no UI.Hard constraints
SEND_HIGH_WATERinpeer.tsuntouched —peer.tsis not in this diff at allTesting
useWarpTransfer.check.mjsgains 13 collision cases (50 checks pass in total): the on-disk collision, three-in-one-batch, the no-collision case, combined disk + batch, probe cost, an inconclusive probe, dotfiles, and extensionless names.Because that harness uses a fake directory handle, it proves the algorithm but not the API contract. So the behaviour was also verified against the real File System Access API through OPFS in Chrome:
NotFoundError— the assumption the whole fix rests onreport.pdfreceives into a folder that already holds one leave four files with the original intact.envbecomes.env (1)As a control, the pre-fix code exercised the same way leaves one file with the original's contents replaced — the bug reproduced as real data loss.
Known gaps, deliberately out of scope
DefaultReceiveHostinpeer.tscarries a second copy of the same in-memory-only de-dupe. It is not the app's path —useWarpTransferinjects its ownReceiveHostat bothnew WarpPeersites — but a library consumer who doesn't inject one would still overwrite. Happy to fix in a follow-up if you'd like it in scope.getFileHandlehas no exclusive-create option, andcreateWritable()defaults tomode: "siloed", so the last writer can win.createWritable({ mode: "exclusive" })would instead surface the clash asNoModificationAllowedErrorand could drive a retry under a fresh name, but that needs a widerFsFileHandletype, a change to the write path, and updated mocks, so it is a separate follow-up rather than part of this naming fix. This is pre-existing behaviour and unchanged here.useWarpTransfer.check.mjsisn't wired intopnpm test(webhas notestscript), so it has to be run explicitly, as its own header says.showDirectoryPicker— it needs a user gesture and an OS dialog. OPFS exercises the samegetFileHandleinterface and is the closest substitute, which is why the control above matters.Summary by CodeRabbit
Greptile Summary
This PR fixes a silent data-loss bug (#133) where receiving a file into a folder that already contained a same-named file would overwrite it.
uniqueNameis promoted to async and extended to probe the chosen directory with a single cheapgetFileHandlecall per candidate, so the existing file keeps its name and the incoming file steps aside.existsInDirintroduces a three-state return (true/false/\"unknown\") so an unreadable directory neither causes infinite looping nor silently overwrites an invisible file — an inconclusive probe returns the current candidate immediately, deferring the real error to the subsequentcreate: truewrite.acceptcallback inuseWarpTransfer.tswas already async, so the addition ofawait uniqueName(…, dir)fits naturally into the existing flow.useWarpTransfer.check.mjscover on-disk collision, pure in-batch deduplication, combined disk+batch, the "unknown" probe path, probe cost, dotfiles, and extensionless names.Confidence Score: 4/5
Safe to merge — the overwrite bug is correctly fixed, all edge cases are covered, and the async change fits naturally into an already-async accept callback.
The algorithm is correct and well-documented. The only concern is that the test helpers in the .check.mjs file are manually duplicated from the production TypeScript, meaning a future algorithm change won't automatically be reflected in the tests unless the test file is also updated.
Files Needing Attention: web/src/lib/warp/useWarpTransfer.check.mjs — the hand-copied existsInDir and uniqueName helpers need to stay in sync with their counterparts in useWarpTransfer.ts.
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(receive): rename incoming files on a..." | Re-trigger Greptile