feat: wire web file upload through the facade write handles - #1071
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughChangesThe PR adds folder-scoped uploads. Files can be dropped or selected, staged in chunks, committed through the engine facade, and tracked through progress, cancellation, retry, failure, and dismissal states. Upload flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant UploadZone
participant UploadPanel
participant useDropUpload
participant EngineFacade
participant UploadListItem
User->>UploadZone: drop or select files
UploadZone->>UploadPanel: forward files
UploadPanel->>useDropUpload: queue files
useDropUpload->>EngineFacade: stage chunks and commit writes
EngineFacade-->>useDropUpload: progress and lifecycle events
useDropUpload-->>UploadListItem: update upload entry
User->>UploadListItem: cancel, retry, or dismiss
UploadListItem->>useDropUpload: invoke lifecycle action
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Drops and file picks now drive beginWrite / pushChunk / commitWrite: each file is sliced a chunk at a time and the buffer is transferred into the engine, so no plaintext copy lands in React state. Per-file rows key on the op id commitWrite returns and render the engine's own opProgress phases; a dead letter settles the row as terminal, where an uploadFailed is one attempt the drain retries. Cancel aborts a staging handle or issues cancelUpload once the op exists, and a refused write keeps the engine's stable code so an over-budget refusal reads apart from a terminal failure. Closes #873
…vertook Review gates on the upload wiring. Security: an engine-driven cancel left a row with no control at all — cancel was gated on the row being active and dismiss on it having failed — so the row and the File handle behind it were stranded for the life of the mount. Every settled row now retires itself and offers dismiss. The retire timer is also cancellable again: a dead letter can follow the blocks landing, and the timer the completed phase scheduled would otherwise sweep the failed row. The unbound-update slot is a map gated on an open commit, so a foreign op's report can neither accumulate nor clobber the update a commit is waiting for. Simplify: the upload surface moves into UploadPanel so a block-confirmed event no longer repaints the whole listing; cancel reads the op id off the job rather than render state; the drop target takes the folder id the snapshot reports instead of re-deriving it from the breadcrumb trail; and the shimmer runs only on the row actually being fed. A stopped attempt and an over-budget refusal now share the warning colour, since neither is the settled red of a row that will never publish.
3ac86fa to
450f789
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ticket requirements check against #873. One scope item from the ticket is not fulfilled in this diff; the rest of the ticket's scope (write-handle drive loop with transferred chunks, per-file progress keyed on the op id, cancel via cancelUpload, the harvested upload surface and stylesheet) is implemented.
Posted via Macroscope — Ticket Requirements
There was a problem hiding this comment.
Two small notes on apps/web/src/hooks/useDropUpload.ts; everything else looks consistent with the surrounding code.
Posted via Macroscope — Language idioms
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/hooks/useDropUpload.ts (1)
239-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
retiredependency.
rundoes not callretire; it reaches retirement throughapply. The extra dependency rebuildsrunwithout cause.- [apply, dropOp, engine, patch, retire] + [apply, dropOp, engine, patch]🤖 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 `@apps/web/src/hooks/useDropUpload.ts` at line 239, Remove the unused retire dependency from the dependency array associated with run in useDropUpload, leaving apply and the other dependencies unchanged since retirement is reached through apply.
🤖 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 `@apps/web/src/components/file-browser/FileBrowser.tsx`:
- Line 26: Move the useDropUpload lifecycle out of the folder-conditional
UploadPanel into a component that remains mounted across navigation, passing the
current folder as its write target. Update the rendering so the drop zone
appears only when folder is non-null while upload rows remain rendered
unconditionally, and reset or scope rows when the active folder changes so
uploads from the previous folder are not shown under the new folder.
In `@apps/web/src/hooks/useDropUpload.ts`:
- Around line 23-30: Run the formatter on useDropUpload.ts and apply Prettier’s
required layout to the UploadPhase union type so the lint gate passes.
- Around line 281-292: Update retry in useDropUpload to reset the row through
apply rather than calling patch directly, ensuring the pending retirement timer
is cleared when transitioning back to staging. Preserve the existing
cancellation, unbinding, job reset, and enqueue behavior, and add a test
covering retrying a cancelled committed row before the timer fires, verifying
the row remains and runs again.
---
Nitpick comments:
In `@apps/web/src/hooks/useDropUpload.ts`:
- Line 239: Remove the unused retire dependency from the dependency array
associated with run in useDropUpload, leaving apply and the other dependencies
unchanged since retirement is reached through apply.
🪄 Autofix
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 Plus
Run ID: 755ca538-4ead-4cdd-bbd0-387583ca69ec
📒 Files selected for processing (11)
apps/web/src/components/file-browser/FileBrowser.tsxapps/web/src/components/file-browser/UploadListItem.test.tsxapps/web/src/components/file-browser/UploadListItem.tsxapps/web/src/components/file-browser/UploadPanel.tsxapps/web/src/components/file-browser/UploadZone.test.tsxapps/web/src/components/file-browser/UploadZone.tsxapps/web/src/hooks/useDropUpload.test.tsxapps/web/src/hooks/useDropUpload.tsapps/web/src/main.tsxapps/web/src/styles/upload.cssapps/web/src/vault/useFolderNavigation.ts
…e retirement timer on retry An upload panel gated on a non-null folder unmounted whenever the routed folder had no snapshot yet, taking the in-flight rows, their cancel and retry controls, and the engine subscription with it. Mount it always and gate only the drop target, on a settled folder — which also stops a malformed node route from accepting drops into the root behind its own "that is not a folder id" error. `retry` moved a settled row back to `staging` through `patch`, leaving the retirement timer that phase had scheduled to delete the row and its job mid-run. Route it through `apply`, which stops the timer for a phase that is not retiring, and drop the stale `retire` dependency from `run`. Entire-Checkpoint: aa776a71af92
There was a problem hiding this comment.
One minor finding; the two items from the previous run (the stale retire dependency and the silent abortWrite catch) are resolved.
Posted via Macroscope — Language idioms
…phase that reports one The progress track only renders while the row is active, and `uploaded` is not an active phase, so its arm of `measured` was unreachable. Entire-Checkpoint: 246c9b26ac95
1 participant
Closes #873.
The upload half of the web content path, on top of the write-handle surface
packages/clientalready exposes. No engine or client command is added — this is UI wiring overbeginWrite/pushChunk/commitWrite/abortWrite/cancelUploadand theopProgressevent stream.What lands
apps/web/src/hooks/useDropUpload.ts— the drive loop. A file is sliced at a 1 MiB boundary and each slice is read and handed topushChunkin one step, so the buffer is detached by the transfer and no plaintext copy ever reaches a React value. Files run one at a time, becausebeginWritereserves the whole version against the staging budget and files started together would otherwise contend for room only one can have.UploadZone.tsx— the drop target, doubling as a file picker. Ignores drags that carry no files, and uses an enter/leave depth counter so crossing a child does not drop the highlight.UploadListItem.tsx— one row per upload, in the same three columns as the listing below it. Determinate bar once the drain reports confirmed blocks, a shimmer only on the row actually being fed.UploadPanel.tsx— owns the rows, so a block-confirmed event repaints them alone rather than the whole folder listing.styles/upload.css— terminal-aesthetic styling carried over from the v1 upload surface, withprefers-reduced-motionhonoured.useFolderNavigationgains one field: the folder id the snapshot reported, which the drop target needs as its write target. It was already computing it to build the breadcrumb trail.Phases
Rows key on the op id
commitWriteresolves with, and every phase past that point is the engine's word:stagingqueuedcommitWritereturned an op iduploadinguploadStarted/uploadProgressuploadeduploadCompleted— blocks on the network, record publishes next; the row retires itselfstalleduploadFailed— one attempt stopped, and the drain retries itcancelleduploadCancelled, or a handle abandoned before commitfaileddeadLetterfor the opuploadFailedis deliberately not terminal: the facade documents anEvent::DeadLetterin the same pass as the thing that says the op will never publish, so the row stays open and offers cancel until a dead letter arrives.Cancel
Cancel before commit sets a flag the run loop reads at its next chunk boundary and aborts the handle; cancel after commit issues
cancelUpload(opId). A cancel that lands mid-commit is picked up as soon as the op id exists. A refusal —tooLateToCancel,notAnUpload— is shown on the row without moving it off the upload it is still doing.Over-budget
A refused write keeps
EngineRequestError.code, so the UI rendersoverBudgetin the warning colour with a retry — a ceiling, not a verdict — apart from the settled red of a row that will never publish. A stopped attempt reads the same way, for the same reason. The engine's own message is what distinguishes the six causes, verbatim. See below.Review gates
/simplifyand/security-reviewboth ran on this diff; the second commit is their output.Security review found three real defects, all fixed and covered by tests:
uploadCancelledhad no control at all and held itsFilefor the life of the mount. Every settled row now retires itself and offers dismiss.uploadCompletedmeans blocks are on the network, not that the record published, so adeadLettercan legitimately follow — and the timer the completed phase scheduled would then sweep the failed row away. Timers are now per row and cleared whenever a row moves off a settled phase.queued. It is now a map gated on an open commit window, so nothing accumulates and nothing clobbers.Simplify's structural findings — the panel extraction, reading the folder id off the snapshot instead of the breadcrumb trail, taking the op id off the job rather than render state, and restricting the shimmer — are in the same commit.
Not done, on purpose
OverBudgetCauseincrates/engine/src/facade.rsdoes separate them, but every variant collapses to the single codeoverBudgetat the wasm boundary incrates/wasm/src/host.rs, so only the prose differs. Splitting them by substring-matching the diagnostic would be exactly the source-text assertion the testing law forbids, and the fix belongs incrates/wasmpluspackages/client, which this PR does not own. Filed as engine: carry the over-budget cause across the wasm boundary #1073, with the rendering split as web: render the drain's over-budget hold on the upload row #1075 which it blocks. web: wire file upload through the facade write handles #873's scope bullet has been struck and pointed at that pair, so closing it here does not drop the requirement.SnapshotDescriptor.blockednames the op the drain is holding, and nothing inapps/webreads it, so a held op sits atqueuedwith no explanation. Wiring it needs the snapshot store and the upload rows to meet somewhere, which is a design choice bigger than this issue. Filed as web: render the drain's over-budget hold on the upload row #1075.packages/client/src/worker/engineHost.ts, tracked separately. Nothing here retains an extra copy.Issue-body corrections
The body is stale in three places, all of them now fixed on
main:OpPhasecarries the fiveUpload*variants and the upload path emitsop_id: Some(op_id).OpPhaseatfacade.rs:464-471with "onlyDownload*variants", andemit_op_progresshardcodingop_id: Noneat:1684-1691. Neither is true any more.Command::Create { content: Option<PlaintextContent> }atfacade.rs:238-247as the shape going away. It is already gone —Command::Createcarries no content.It also asks to harvest the v1 components. They were read for visual and interaction continuity, then rewritten: v1 leaned on
react-dropzone, a Zustand upload store, and axios cancel tokens, none of which this app has, and its cancel was cosmetic once a batch was in flight.Gate
web unit— 31 new tests across the hook and both components; the fullapps/websuite is 141 green.Verified in a real browser against the dev server: the drag highlight survives crossing a child, a text-only drag is ignored, a drop delivers the
File, the determinate bar measures 42% of its track for a 0.42 fraction, only the sealing row animates, the over-budget and retrying lines render in the warning colour while a dead letter renders red, and a cancelled row offers retry and dismiss.Note
Add file upload UI with drag-and-drop, progress tracking, and cancellation to the file browser
UploadZone,UploadPanel, andUploadListItemcomponents to the file browser, providing drag-and-drop and file picker upload entry points with per-upload progress rows.useDropUploadhook to orchestrate the full upload pipeline: queuing, chunking files into 1MB pieces, staging viabeginWrite, committing, tracking progress via engine events, and auto-retiring completed rows.useFolderNavigationto expose the currently settled folder id, used to enable the drop target only after the routed folder has landed.Macroscope summarized 9a0fd7d.
Summary by CodeRabbit