Skip to content

feat(web): Inline upload progress#410

Merged
FSM1 merged 28 commits into
mainfrom
feat/inline-upload-progress
Mar 30, 2026
Merged

feat(web): Inline upload progress#410
FSM1 merged 28 commits into
mainfrom
feat/inline-upload-progress

Conversation

@FSM1

@FSM1 FSM1 commented Mar 30, 2026

Copy link
Copy Markdown
Owner

Summary

Replaces the floating UploadModal popup with inline upload progress rows integrated directly into the file browser list, providing in-context upload feedback.

What changed

  • Upload store refactored to per-file trackingMap<string, PerFileUpload> replaces batch-level state. Each file has independent status, progress, cancel token, and error. Actions: addFile, updateFileProgress, setFileStatus, removeFile, cancelFile, retryFile.
  • New UploadListItem component — inline progress row matching the FileListItem grid layout. Fine-grained Zustand selector (subscribes to single file entry). Progress bar, cancel/retry/dismiss buttons, 1s completion flash, ARIA accessibility.
  • Virtual entry merging in FileList — upload rows appear at their alphabetical position alongside real files. Folder-scoped filtering via targetFolderId. Ref-based equality check on the selector prevents re-renders on progress ticks.
  • Old popup removedUploadModal.tsx, UploadItem.tsx, and all popup CSS deleted.
  • Dead code cleaned up — removed unused useFileUpload hook, redundant setFileComplete action, uploadFiles() batch function. Extracted createUploadId() and isActiveUpload() helpers. Made cancelFile atomic (cancel + delete in single set()).

Files changed (code)

File Change
stores/upload.store.ts Per-file Map, helpers, same-value guard on progress
components/file-browser/UploadListItem.tsx New inline upload row component
components/file-browser/FileList.tsx Virtual entry merging, ref-based selector equality
components/file-browser/FileBrowser.tsx Removed UploadModal, added retry wiring
components/file-browser/UploadModal.tsx Deleted
components/file-browser/UploadItem.tsx Deleted
hooks/useDropUpload.ts Per-file actions, boolean isUploading selector
hooks/useFileUpload.ts Deleted (zero consumers)
services/upload.service.ts Removed dead uploadFiles() batch function
styles/upload.css Inline CSS replaces popup CSS

Test plan

  • TypeScript compiles clean (tsc --noEmit)
  • 23/23 upload store tests pass (error recovery, pending replacements)
  • React Rules of Hooks violation fixed (useCallback before early return)
  • Manual: upload a file → progress row appears inline at alphabetical position
  • Manual: upload completes → green flash for 1s → row disappears, real file appears
  • Manual: cancel mid-upload → row removed immediately
  • Manual: upload fails → red bar with retry/dismiss buttons
  • Manual: retry → re-triggers upload, progress resets
  • Manual: upload to subfolder → rows only appear in that folder's view

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Uploads now show as inline rows inside the file browser (alphabetical position) with a brief completion flash, reduced-motion support, and the floating upload popup removed
    • Per-file progress tracking with individual cancel, retry, and dismiss controls; inline error states with retry
  • Tests

    • Updated upload error-recovery tests for per-file tracking
  • Documentation

    • Roadmap, phase planning, UI spec, and verification docs updated for Phase 36: Inline upload progress

FSM1 and others added 21 commits March 30, 2026 03:13
Entire-Checkpoint: f8637df6d80c
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 058ef38e725f
Move non-standard inherited spacing values (12px, 6px, 3px) out of the
main spacing scale into a read-only inherited exceptions section. Add
typography advisory for 10px/11px proximity. Improve dismiss tooltip
from "Dismiss" to "Dismiss error" for clarity.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: f3ff7d5eb7c5
Entire-Checkpoint: 26eefa9e1c06
Entire-Checkpoint: 82929df927b4
Entire-Checkpoint: 5174187dc048
Entire-Checkpoint: be07c554de11
…d state

- Replace batch-level UploadState fields (status, progress, currentFile, etc.)
  with per-file Map<string, PerFileUpload> tracking
- Add per-file actions: addFile, updateFileProgress, setFileStatus,
  setFileComplete, removeFile, cancelFile, retryFile
- Export PerFileUpload type for downstream component consumption
- Each file gets independent cancel token, progress, and error state
- Preserve PendingReplacement type and actions unchanged
- Rewrite upload-error-recovery tests for per-file API (12 tests passing)
- Stub UploadModal.tsx (returns null) to prevent type errors during transition
- useDropUpload: generate per-file upload IDs, call addFile/updateFileProgress/
  setFileComplete per file, derive isUploading from files Map
- useDropUpload: use setFileStatus for error handling with specific file ID
- upload.service: replace batch startUpload/setEncrypting/setUploading with
  per-file addFile/updateFileProgress/setFileComplete
- useFileUpload: derive batch-level fields (status, progress, currentFile,
  completedFiles) from per-file Map for backward compatibility
- Remove all batch-level store action calls (startUpload, setSuccess, etc.)
- Always use getState() for reads inside async callbacks (stale closure prevention)
- Break long ternary in useFileUpload.ts onto multiple lines
- Collapse multiline if condition in upload.store.ts reset()
- SUMMARY.md with task commits, decisions, and self-check
- STATE.md updated with phase 36 position and metrics
- ROADMAP.md updated with plan progress (1/2 complete)
- Create UploadListItem.tsx with fine-grained Zustand selectors per file ID
- Support encrypting, uploading, complete, error, and cancelled states
- Progress bar with aria-valuenow/aria-valuemax, pulse animation for encrypting
- Cancel, retry, dismiss buttons with aria-labels and focus-visible styles
- Completion flash timer (1000ms) with cleanup on unmount
- Delete all popup/modal/upload-item CSS rules from upload.css
- Add inline upload CSS with terminal aesthetic, reduced motion support
…onents

- Merge upload entries into FileList with virtual entry sorting and duplicate filtering
- Wire onRetryUpload from FileBrowser through FileList to UploadListItem via handleFileDrop
- Remove UploadModal import/render from FileBrowser
- Delete UploadModal.tsx and UploadItem.tsx (D-11)
- Update barrel exports in index.ts
- Add 36-02-SUMMARY.md with task commits and self-check
- Update STATE.md progress and metrics
- Update ROADMAP.md plan progress
Fixes React Rules of Hooks violation — hooks were defined after the
`if (!file) return null` guard, causing "Rendered more hooks than
previous render" when a file is removed from the store mid-render.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: eccd0812694c
Phase 36 (inline-upload-progress) verified — 8/8 must-haves passed.
Hooks order gap resolved inline, all tests green.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 73418a451496
All 8/8 truths now VERIFIED after dfeaadf moved useCallback hooks
before the early return guard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 1a879ac749c3
@coderabbitai

coderabbitai Bot commented Mar 30, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@FSM1 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 13 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 13 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 379c397e-4aaf-4937-aa2f-56fcf8a618a9

📥 Commits

Reviewing files that changed from the base of the PR and between 44d2869 and 01197a8.

📒 Files selected for processing (3)
  • apps/web/src/components/file-browser/UploadListItem.tsx
  • apps/web/src/stores/upload.store.ts
  • apps/web/src/styles/upload.css

Walkthrough

Phase 36: refactors web uploads to per-file tracking and inline upload rows. Replaces the floating UploadModal/UploadItem with UploadListItem rendered inside the FileList, updates the upload store/hooks/services/tests to per-file APIs, and adds planning/validation/docs for the change.

Changes

Cohort / File(s) Summary
Planning & Documentation
​.planning/ROADMAP.md, ​.planning/STATE.md, ​.planning/config.json, ​.planning/phases/36-inline-upload-progress/*
Added Phase 36 artifacts: roadmap/state updates, branch template change, and multiple planning/spec/validation/summary/verification documents for inline upload-progress.
Upload Store Refactor
apps/web/src/stores/upload.store.ts, apps/web/src/stores/__tests__/upload-error-recovery.test.ts
Replaced batch-level upload state with files: Map<string, PerFileUpload>, added per-file types/actions (addFile, updateFileProgress, setFileStatus, removeFile, cancelFile), and updated tests to assert per-file states.
Upload Orchestration / Hooks & Service
apps/web/src/hooks/useDropUpload.ts, apps/web/src/hooks/useFileUpload.ts, apps/web/src/hooks/index.ts, apps/web/src/services/upload.service.ts
useDropUpload now creates per-file IDs, registers files, passes per-file cancel tokens, and updates per-file progress/status. Removed useFileUpload and the multi-file uploadFiles() orchestration; uploadFile remains. Barrel export adjusted.
Components: Removal → Inline
apps/web/src/components/file-browser/UploadModal.tsx, apps/web/src/components/file-browser/UploadItem.tsx, apps/web/src/components/file-browser/UploadListItem.tsx
Removed popup UploadModal and UploadItem; added UploadListItem that subscribes to a single file entry, renders inline progress, handles cancel/retry/dismiss, and schedules 1s removal after completion.
File List Integration
apps/web/src/components/file-browser/FileBrowser.tsx, apps/web/src/components/file-browser/FileList.tsx, apps/web/src/components/file-browser/index.ts
Removed modal rendering from FileBrowser, added handleRetryUpload/onRetryUpload plumbing. FileList merges folder-scoped virtual upload entries (_uploading) with real items, deduplicates completing names, sorts alphabetically, and renders UploadListItem. Barrel exports updated.
Styles
apps/web/src/styles/upload.css
Deleted popup/modal CSS and animations; added inline-row styles, progress-track/fill, encrypting pulse, status classes, action/button styles, and reduced-motion handling.
Tests / Validation
apps/web/src/stores/__tests__/upload-error-recovery.test.ts, .planning/...VERIFICATION.md
Updated tests to use per-file APIs and assert per-file states; added verification docs and grep/typecheck/build-based validation steps for removal of popup artifacts.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant FileBrowser
    participant useDropUpload
    participant UploadService
    participant UploadStore
    participant FileList
    participant UploadListItem

    User->>FileBrowser: drop file(s)
    FileBrowser->>useDropUpload: handleFileDrop([file], folderId)

    loop per-file
        useDropUpload->>useDropUpload: createUploadId(filename)
        useDropUpload->>UploadStore: addFile(uploadId, filename, folderId, file)
        useDropUpload->>UploadService: uploadFile(file, cancelToken)
        UploadService->>UploadStore: updateFileProgress(uploadId, percent)
        UploadService->>UploadStore: setFileStatus(uploadId, 'complete'|'error')
    end

    FileList->>UploadStore: subscribe files (scoped)
    UploadStore-->>FileList: files map (filtered)
    FileList->>UploadListItem: render per upload entry
    UploadListItem->>UploadStore: read status/progress
    UploadListItem->>UploadStore: cancelFile/removeFile / call onRetry -> useDropUpload
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(web): Inline upload progress' directly and clearly summarizes the main change—replacing the floating UploadModal popup with inline upload progress rows in the file browser list.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/inline-upload-progress

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Mar 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.25%. Comparing base (bb87d17) to head (01197a8).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   61.28%   61.25%   -0.04%     
==========================================
  Files         133      133              
  Lines        9790     9790              
  Branches      988      986       -2     
==========================================
- Hits         6000     5997       -3     
- Misses       3574     3577       +3     
  Partials      216      216              
Flag Coverage Δ
api 84.31% <ø> (-0.06%) ⬇️
api-client 84.31% <ø> (-0.06%) ⬇️
core 84.31% <ø> (-0.06%) ⬇️
crypto 84.31% <ø> (-0.06%) ⬇️
sdk 84.31% <ø> (-0.06%) ⬇️
sdk-core 84.31% <ø> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Extract createUploadId() and isActiveUpload() helpers to eliminate
  3x ID format duplication and 5x status-check duplication
- Remove redundant setFileComplete (setFileStatus('complete') already
  handles progress: 100)
- Make cancelFile atomic (cancel + remove in single set() call),
  removing two-step race in UploadListItem
- Add same-value guard to updateFileProgress to skip no-op store
  updates on repeated progress ticks
- Add ref-based equality check to FileList's upload selector to
  prevent re-rendering entire file list on every progress callback
- Use stable timestamps (0) for virtual upload entries
- Remove dead useFileUpload hook (zero consumers) and its only caller
  uploadFiles() from upload.service.ts
- Remove 'cancelled' status from PerFileUpload (cancelFile now deletes
  the entry immediately)
- Remove WHAT-not-WHY comments from UploadListItem JSX

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: e095e7612dbd

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Replaces the floating upload popup with inline per-file upload progress rows integrated into the file browser list, backed by a refactored per-file upload store.

Changes:

  • Refactored useUploadStore from batch-level fields to a per-file Map with per-file cancel/retry/progress.
  • Added UploadListItem and wired FileList/FileBrowser to render uploads inline (and removed UploadModal/UploadItem + popup CSS).
  • Updated upload flows (useDropUpload, upload.service, useFileUpload) and adjusted store tests for the new per-file API.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
apps/web/src/styles/upload.css Removes popup styles and adds inline upload row/progress/button styling.
apps/web/src/stores/upload.store.ts Introduces per-file upload tracking (Map) and per-file actions (add/update/cancel/retry/reset).
apps/web/src/stores/tests/upload-error-recovery.test.ts Rewrites tests to assert per-file error transitions and orphan cleanup behavior.
apps/web/src/services/upload.service.ts Migrates sequential upload flow to create/update per-file store entries.
apps/web/src/hooks/useFileUpload.ts Derives legacy/batch-like fields from the per-file Map for compatibility.
apps/web/src/hooks/useDropUpload.ts Migrates SDK + duplicate-upload flows to per-file store actions and inline progress model.
apps/web/src/components/file-browser/index.ts Updates barrel exports to remove popup components and export UploadListItem.
apps/web/src/components/file-browser/UploadModal.tsx Deleted (popup UI removed).
apps/web/src/components/file-browser/UploadListItem.tsx New inline upload progress row with cancel/retry/dismiss actions.
apps/web/src/components/file-browser/UploadItem.tsx Deleted (popup row UI removed).
apps/web/src/components/file-browser/FileList.tsx Merges upload “virtual entries” into sorted list and renders UploadListItem.
apps/web/src/components/file-browser/FileBrowser.tsx Removes popup and wires retry callback to drop-upload handler.
.planning/phases/36-inline-upload-progress/* Adds planning/spec/verification artifacts for Phase 36.
.planning/config.json Updates planning git branch templates.
.planning/STATE.md Updates planning state/progress and notes Phase 36.
.planning/ROADMAP.md Adds Phase 36 entry.
Comments suppressed due to low confidence (1)

apps/web/src/services/upload.service.ts:131

  • uploadFiles() creates a per-file entry in the upload store, but if uploadFile() throws the catch block only logs and rethrows. The corresponding store entry never transitions to 'error', so the UI/store can get stuck showing an in-progress upload with no way to retry/dismiss. Track the current uploadId and call setFileStatus(uploadId, 'error', message) (and/or removeFile) on non-cancel errors.

Comment thread apps/web/src/styles/upload.css
Comment thread apps/web/src/components/file-browser/UploadListItem.tsx Outdated
Comment thread apps/web/src/hooks/useDropUpload.ts
Comment thread apps/web/src/stores/upload.store.ts
Comment thread .planning/STATE.md Outdated
Comment thread apps/web/src/components/file-browser/UploadListItem.tsx Outdated
Comment thread apps/web/src/components/file-browser/FileList.tsx

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
.planning/STATE.md (1)

82-89: Minor table formatting inconsistency (informational only).

Lines 82-89 use a different column format than the preceding rows (combined "Phase 33 P01" naming vs separate Phase/Plan columns). This causes the table to have mismatched column counts.

Based on learnings, .planning/STATE.md metrics are auto-generated during GSD execution and minor inconsistencies are acceptable since they don't affect code behavior.

Optional: Align table format for consistency
-| Phase 33 P01 | 11min | 2 tasks  | 3 files  |
-| Phase 33 P02 | 3min  | 1 tasks  | 3 files  |
-| Phase 34 P01 | 3min  | 2 tasks  | 9 files  |
-| Phase 34 P02 | 4min  | 2 tasks  | 8 files  |
-| Phase 34 P03 | 2min  | 1 tasks  | 1 files  |
-| Phase 35 P02 | 5min  | 4 tasks  | 6 files  |
+| 33           | 01    | 11min    | 2        | 3     |
+| 33           | 02    | 3min     | 1        | 3     |
+| 34           | 01    | 3min     | 2        | 9     |
+| 34           | 02    | 4min     | 2        | 8     |
+| 34           | 03    | 2min     | 1        | 1     |
+| 35           | 02    | 5min     | 4        | 6     |
 | Phase 35 P03 | 8min  | 4 tasks  | 14 files |
-| Phase 35 P05 | 5min  | 3 tasks  | 3 files  |
+| 35           | 03    | 8min     | 4        | 14    |
+| 35           | 05    | 5min     | 3        | 3     |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.planning/STATE.md around lines 82 - 89, The table rows using "Phase 33
P01", "Phase 33 P02", etc. combine Phase and Plan into one column causing a
mismatched column count; update those rows to separate the Phase and Plan into
the same columns as the rest of the table (e.g., split "Phase 33 P01" into
"Phase 33" and "P01") so all rows have consistent column counts and alignment
with the table header.
apps/web/src/components/file-browser/FileList.tsx (2)

113-134: Ref-based equality check is a solid optimization but has a subtle gap.

The custom equality logic at lines 126-130 compares id and status but not filename. While filename changes mid-upload are unlikely, the entries array includes filename (line 122), suggesting it was intended for comparison. If consistency is desired:

🔧 Optional fix to include filename in equality check
       prev.length === entries.length &&
-      prev.every((p, i) => p.id === entries[i].id && p.status === entries[i].status)
+      prev.every((p, i) => p.id === entries[i].id && p.status === entries[i].status && p.filename === entries[i].filename)

That said, this is minor since filename won't change during an upload. The overall pattern correctly prevents re-renders on progress ticks per 36-RESEARCH.md Pitfall 6.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/file-browser/FileList.tsx` around lines 113 - 134,
The equality check in the uploadEntries selector uses prevEntriesRef and
compares only id and status, but the UploadEntry includes filename; update the
comparison used in the useUploadStore selector (the block building
uploadEntries/prevEntriesRef) to also compare filename so changes to filename
are detected and cause updates; ensure you still return the same prev array when
id, status, and filename all match to preserve the ref-based optimization.

136-157: Type cast hides structural mismatch with FolderChild.

UploadVirtualEntry includes the _uploading discriminator field (line 20) which FolderChild (union of FolderEntry | FilePointer) doesn't have. The cast on line 157 bypasses TypeScript's structural checks. This works because sortItems only accesses type and name, but future changes to sortItems could silently break.

Consider a discriminated union or intersection type if this pattern expands:

type SortableItem = FolderChild | UploadVirtualEntry;

For now, the cast is acceptable given the limited scope and clear _uploading check at render time (line 199).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/file-browser/FileList.tsx` around lines 136 - 157,
The code unsafely casts UploadVirtualEntry to FolderChild when calling
sortItems, hiding a structural mismatch because UploadVirtualEntry contains the
_uploading discriminator; change the types so sortItems accepts a union that
includes UploadVirtualEntry (e.g., introduce SortableItem = FolderChild |
UploadVirtualEntry) and update the call site from sortItems(allItems as
FolderChild[]) to sortItems(allItems as SortableItem[]), keeping the existing
runtime _uploading checks in rendering (and adjusting any sortItems signature or
overloads to accept SortableItem).
apps/web/src/components/file-browser/UploadListItem.tsx (1)

4-8: Make the retry contract non-optional.

Error rows always render [R], but onRetry is optional and Line 56 silently degrades the button into a dismiss when the callback is missing. Since the current caller chain already wires retry through, tightening the prop here lets TypeScript catch future regressions.

♻️ Proposed fix
 type UploadListItemProps = {
   /** Upload entry ID in the upload store */
   fileId: string;
   /** Callback to re-trigger the upload for a failed file (passes the original File object) */
-  onRetry?: (file: File) => void;
+  onRetry: (file: File) => void;
 };
@@
   const handleRetry = useCallback(() => {
     const originalFile = file?.file;
     removeFile(fileId);
-    if (originalFile && onRetry) {
+    if (originalFile) {
       onRetry(originalFile);
     }
   }, [fileId, removeFile, file?.file, onRetry]);

Also applies to: 53-59, 117-127

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/file-browser/UploadListItem.tsx` around lines 4 - 8,
The prop onRetry on UploadListItemProps is currently optional but the UI always
renders a retry control; make the contract strict by removing the optional
marker so onRetry: (file: File) => void is required. Update the
UploadListItemProps type and any component signature for UploadListItem to
reflect the non-optional prop, remove any runtime fallback logic that treats
onRetry as missing (e.g., the conditional at the button render around line 56),
and fix all callers to pass the retry callback where UploadListItem is
instantiated so TypeScript enforces the new contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/stores/upload.store.ts`:
- Around line 28-29: createUploadId(filename: string) currently builds keys with
filename + Date.now(), which can collide if two same-name files start within the
same millisecond; update createUploadId to append a collision-resistant
component (e.g., a UUID/random hex string from crypto.randomUUID() or a secure
random bytes and hex encoding, or an incrementing unique counter) so the keys
used by the Map<string, PerFileUpload> are unique even for same filenames
started concurrently; ensure the new id still includes filename for readability
but add the random/UUID suffix and update any code that parses or relies on the
old format (refer to createUploadId and all Map access using those keys).
- Around line 75-82: updateFileProgress currently unconditionally sets status:
'uploading' which can revert terminal entries; modify updateFileProgress(id,
progress) to first retrieve the entry from state.files and return state
immediately if entry.status is a terminal state (e.g., 'complete' or 'error' —
match your code's terminal values), otherwise proceed to check for the no-op
(same progress) and update the Map; reference the updateFileProgress function
and the state.files Map when making this change so you only overwrite status for
non-terminal entries.

---

Nitpick comments:
In @.planning/STATE.md:
- Around line 82-89: The table rows using "Phase 33 P01", "Phase 33 P02", etc.
combine Phase and Plan into one column causing a mismatched column count; update
those rows to separate the Phase and Plan into the same columns as the rest of
the table (e.g., split "Phase 33 P01" into "Phase 33" and "P01") so all rows
have consistent column counts and alignment with the table header.

In `@apps/web/src/components/file-browser/FileList.tsx`:
- Around line 113-134: The equality check in the uploadEntries selector uses
prevEntriesRef and compares only id and status, but the UploadEntry includes
filename; update the comparison used in the useUploadStore selector (the block
building uploadEntries/prevEntriesRef) to also compare filename so changes to
filename are detected and cause updates; ensure you still return the same prev
array when id, status, and filename all match to preserve the ref-based
optimization.
- Around line 136-157: The code unsafely casts UploadVirtualEntry to FolderChild
when calling sortItems, hiding a structural mismatch because UploadVirtualEntry
contains the _uploading discriminator; change the types so sortItems accepts a
union that includes UploadVirtualEntry (e.g., introduce SortableItem =
FolderChild | UploadVirtualEntry) and update the call site from
sortItems(allItems as FolderChild[]) to sortItems(allItems as SortableItem[]),
keeping the existing runtime _uploading checks in rendering (and adjusting any
sortItems signature or overloads to accept SortableItem).

In `@apps/web/src/components/file-browser/UploadListItem.tsx`:
- Around line 4-8: The prop onRetry on UploadListItemProps is currently optional
but the UI always renders a retry control; make the contract strict by removing
the optional marker so onRetry: (file: File) => void is required. Update the
UploadListItemProps type and any component signature for UploadListItem to
reflect the non-optional prop, remove any runtime fallback logic that treats
onRetry as missing (e.g., the conditional at the button render around line 56),
and fix all callers to pass the retry callback where UploadListItem is
instantiated so TypeScript enforces the new contract.
🪄 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: 76e096b4-e392-4194-ae22-7830a952cca3

📥 Commits

Reviewing files that changed from the base of the PR and between bb87d17 and 45eb47a.

📒 Files selected for processing (27)
  • .planning/ROADMAP.md
  • .planning/STATE.md
  • .planning/config.json
  • .planning/phases/36-inline-upload-progress/.gitkeep
  • .planning/phases/36-inline-upload-progress/36-01-PLAN.md
  • .planning/phases/36-inline-upload-progress/36-01-SUMMARY.md
  • .planning/phases/36-inline-upload-progress/36-02-PLAN.md
  • .planning/phases/36-inline-upload-progress/36-02-SUMMARY.md
  • .planning/phases/36-inline-upload-progress/36-CONTEXT.md
  • .planning/phases/36-inline-upload-progress/36-DISCUSSION-LOG.md
  • .planning/phases/36-inline-upload-progress/36-RESEARCH.md
  • .planning/phases/36-inline-upload-progress/36-UI-SPEC.md
  • .planning/phases/36-inline-upload-progress/36-VALIDATION.md
  • .planning/phases/36-inline-upload-progress/36-VERIFICATION.md
  • apps/web/src/components/file-browser/FileBrowser.tsx
  • apps/web/src/components/file-browser/FileList.tsx
  • apps/web/src/components/file-browser/UploadItem.tsx
  • apps/web/src/components/file-browser/UploadListItem.tsx
  • apps/web/src/components/file-browser/UploadModal.tsx
  • apps/web/src/components/file-browser/index.ts
  • apps/web/src/hooks/index.ts
  • apps/web/src/hooks/useDropUpload.ts
  • apps/web/src/hooks/useFileUpload.ts
  • apps/web/src/services/upload.service.ts
  • apps/web/src/stores/__tests__/upload-error-recovery.test.ts
  • apps/web/src/stores/upload.store.ts
  • apps/web/src/styles/upload.css
💤 Files with no reviewable changes (5)
  • apps/web/src/hooks/index.ts
  • apps/web/src/services/upload.service.ts
  • apps/web/src/hooks/useFileUpload.ts
  • apps/web/src/components/file-browser/UploadItem.tsx
  • apps/web/src/components/file-browser/UploadModal.tsx

Comment thread apps/web/src/stores/upload.store.ts Outdated
Comment thread apps/web/src/stores/upload.store.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.

Comment thread apps/web/src/hooks/useDropUpload.ts
Comment thread apps/web/src/hooks/useDropUpload.ts Outdated
Comment thread apps/web/src/components/file-browser/UploadListItem.tsx Outdated
Comment thread .planning/phases/36-inline-upload-progress/36-VERIFICATION.md Outdated
- Make upload ID collision-safe with incrementing sequence suffix
- Guard updateFileProgress against terminal states (complete/error)
- Fix orphan unpin: only collect unregistered duplicate-path CIDs,
  not SDK-registered files that are already in IPNS
- Conditionally render retry button only when onRetry and file.file
  are available
- Update VERIFICATION.md to reflect current code (no setFileComplete,
  no retryFile, atomic cancelFile)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: ca87b369e6a7
@FSM1

FSM1 commented Mar 30, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai please review

@coderabbitai

coderabbitai Bot commented Mar 30, 2026

Copy link
Copy Markdown

@FSM1 Sure! I'll start reviewing PR #410 right away.


🧠 Learnings used
Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: apps/web/src/hooks/useDropUpload.ts:63-74
Timestamp: 2026-03-20T14:24:47.508Z
Learning: In `apps/web/src/hooks/useDropUpload.ts`, the divergent parent-folder resolution between the collision-check path (uses `useFolderStore.getState().folders[folderId]` directly) and the upload path (uses `folderId === 'root' ? getRootFolderState(vault, folders) : folders[folderId]`) is pre-existing code moved during the Phase 19.1 SDK extraction refactor (PR `#296`). Consolidating both into a single resolution call is a known deferred improvement — do not flag this as a new regression introduced by the PR.

Learnt from: CR
Repo: FSM1/cipher-box PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-06T02:23:13.531Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : For file uploads: 1) Generate random file key and IV, 2) encrypt file content with AES-256-GCM, 3) wrap file key with user's public key using ECIES, 4) upload encrypted file to backend → IPFS without plaintext filename, 5) create per-file FileMetadata IPNS record, 6) add FilePointer to folder metadata, 7) republish folder IPNS

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 55
File: apps/web/src/components/file-browser/EmptyState.tsx:18-24
Timestamp: 2026-02-07T01:09:20.001Z
Learning: For multi-file uploads in apps/web, the planned batching strategy is: keep `addFile` for single-file operations, add `addFilesToFolder` (or similar batch method) that collects all file metadata and calls `updateFolderMetadata` once at the end, with a single `createAndPublishIpnsRecord` invocation per batch instead of per file.

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 372
File: apps/web/src/hooks/useFileOperations.ts:460-480
Timestamp: 2026-03-26T22:11:32.283Z
Learning: In `apps/web/src/hooks/useFileOperations.ts` (`handleUpdateFile`, `handleAddFile`, `handleAddFiles`): The `reWrapForRecipients()` call is intentionally fire-and-forget (not awaited before `replaceFileInFolder` / `addFileToFolder`). Awaiting it before publish would add latency to every file save/upload. The brief window where share recipients see stale content after an owner update is an accepted architectural tradeoff documented in the writable-shares PoC (Phase 27). Do not flag this as a race condition or access-gap regression in current or future PRs.

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: apps/web/src/hooks/useFolderMutations.ts:419-444
Timestamp: 2026-03-20T14:25:05.881Z
Learning: In `apps/web/src/hooks/useFolderMutations.ts` `handleDeleteItems()`: The `folderIdsToRemove` cleanup is intentionally applied only after the full deletion loop. SDK events emitted by `client.deleteToBin()` / `client.deleteItem()` on successful deletes drive `useFolderStore` updates via the `subscribeToSdk` event subscription, so per-iteration in-loop store cleanup is redundant for the happy path. The residual edge case—stale nested folder nodes left in `useFolderStore` after a mid-batch partial failure—is a known, pre-existing gap deferred as a follow-up improvement and should not be flagged as a regression in this or future PRs.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @.planning/phases/36-inline-upload-progress/36-VERIFICATION.md:
- Around line 4-6: The front-matter fields (status:, score:, gaps:) at the top
currently claim success but later text declares a BLOCKER; pick a single
authoritative outcome and make the document consistent: either change the top
front-matter to reflect the blocker (e.g., status: blocked/failed, adjust score
from 8/8 to the correct value, and add the blocking item into gaps: with a short
description) or remove/resolve the blocker language in the later sections so
they match a passed state; update all occurrences of the blocker text (the
phrase “BLOCKER” and “one gap blocks full goal achievement”) to match the chosen
authoritative outcome so the front-matter and body are synchronized.
🪄 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: db0a3dad-0b07-492c-b015-11ea755e71aa

📥 Commits

Reviewing files that changed from the base of the PR and between 45eb47a and caaf557.

📒 Files selected for processing (4)
  • .planning/phases/36-inline-upload-progress/36-VERIFICATION.md
  • apps/web/src/components/file-browser/UploadListItem.tsx
  • apps/web/src/hooks/useDropUpload.ts
  • apps/web/src/stores/upload.store.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • apps/web/src/hooks/useDropUpload.ts
  • apps/web/src/components/file-browser/UploadListItem.tsx
  • apps/web/src/stores/upload.store.ts

Comment thread .planning/phases/36-inline-upload-progress/36-VERIFICATION.md
@FSM1 FSM1 requested a review from Copilot March 30, 2026 12:29
Remove stale BLOCKER and gaps language from initial verification —
all anti-patterns and gaps have been resolved in subsequent commits.
Frontmatter (passed, 8/8, gaps: []) now matches body text.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 420fe751454e

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Comment thread apps/web/src/components/file-browser/UploadListItem.tsx Outdated
The date column is hidden on mobile (responsive.css), making
cancel/retry/dismiss inaccessible. Mirror FileListItem's pattern:
render actions in both the desktop date cell and a separate
.file-list-item-mobile-actions cell that shows on mobile.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 1e66510df185
@FSM1

FSM1 commented Mar 30, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai please review

@coderabbitai

coderabbitai Bot commented Mar 30, 2026

Copy link
Copy Markdown

@FSM1 Sure! I'll kick off a fresh review of PR #410 right away.


🧠 Learnings used
Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: apps/web/src/hooks/useDropUpload.ts:63-74
Timestamp: 2026-03-20T14:24:47.508Z
Learning: In `apps/web/src/hooks/useDropUpload.ts`, the divergent parent-folder resolution between the collision-check path (uses `useFolderStore.getState().folders[folderId]` directly) and the upload path (uses `folderId === 'root' ? getRootFolderState(vault, folders) : folders[folderId]`) is pre-existing code moved during the Phase 19.1 SDK extraction refactor (PR `#296`). Consolidating both into a single resolution call is a known deferred improvement — do not flag this as a new regression introduced by the PR.

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: packages/sdk/src/client.ts:286-360
Timestamp: 2026-03-20T23:50:55.576Z
Learning: In `packages/sdk/src/client.ts` (`CipherBoxClient`): Concurrent mutations to the same folder (e.g., `createFolder`, `renameItem`, `moveItem`, `deleteItem`, `deleteToBin`) are intentionally not serialized with a per-folder mutex in the SDK. Concurrent safety is provided at the UI layer — buttons/actions are disabled while an operation is in flight. A keyed async mutex/queue per folder IPNS name (and a separate one for bin state) is a known deferred improvement and should not be re-flagged as a regression in this or future PRs (first flagged in PR `#296`).

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 372
File: apps/web/src/hooks/useFileOperations.ts:460-480
Timestamp: 2026-03-26T22:11:32.283Z
Learning: In `apps/web/src/hooks/useFileOperations.ts` (`handleUpdateFile`, `handleAddFile`, `handleAddFiles`): The `reWrapForRecipients()` call is intentionally fire-and-forget (not awaited before `replaceFileInFolder` / `addFileToFolder`). Awaiting it before publish would add latency to every file save/upload. The brief window where share recipients see stale content after an owner update is an accepted architectural tradeoff documented in the writable-shares PoC (Phase 27). Do not flag this as a race condition or access-gap regression in current or future PRs.

Learnt from: CR
Repo: FSM1/cipher-box PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-03-06T02:23:13.531Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : For file uploads: 1) Generate random file key and IV, 2) encrypt file content with AES-256-GCM, 3) wrap file key with user's public key using ECIES, 4) upload encrypted file to backend → IPFS without plaintext filename, 5) create per-file FileMetadata IPNS record, 6) add FilePointer to folder metadata, 7) republish folder IPNS

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 296
File: apps/web/src/hooks/useFolderMutations.ts:419-444
Timestamp: 2026-03-20T14:25:05.881Z
Learning: In `apps/web/src/hooks/useFolderMutations.ts` `handleDeleteItems()`: The `folderIdsToRemove` cleanup is intentionally applied only after the full deletion loop. SDK events emitted by `client.deleteToBin()` / `client.deleteItem()` on successful deletes drive `useFolderStore` updates via the `subscribeToSdk` event subscription, so per-iteration in-loop store cleanup is redundant for the happy path. The residual edge case—stale nested folder nodes left in `useFolderStore` after a mid-batch partial failure—is a known, pre-existing gap deferred as a follow-up improvement and should not be flagged as a regression in this or future PRs.

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 0
File: :0-0
Timestamp: 2026-03-27T00:04:37.583Z
Learning: In `apps/api/src/ipns/ipns.service.ts` (`upsertFolderIpns`, lines ~182-288): The "shared child IPNS records become recipient-owned" concern is NOT a vulnerability. (1) For existing shared IPNS records: when no owner `FolderIpns` row is found for `(userId, ipnsName)`, `findActiveWriteShare()` is called; if an active write share exists, the owner's row is fetched via `writeShare.sharerId` — sequence/CID updates always land on the owner's row. Post-revocation `findActiveWriteShare()` returns null, blocking recipient publishes. (2) For new child IPNS records created by a write-share recipient (e.g., new subfolders): these are stored under the recipient's `userId` intentionally because the recipient generated and signed that keypair; post-revocation the recipient loses write access to the parent folder metadata, so orphaned child records have no writable pointer and are non-exploitable. (3) `encryptedIpnsPrivateKey`/`keyEpoch` updates and `enrollFolder()` calls are both gated on `existing.userId === userId`, preventing TEE enrollment overwrites. Do NOT re-flag this as a security regression in future PRs.

Learnt from: FSM1
Repo: FSM1/cipher-box PR: 55
File: apps/web/src/components/file-browser/EmptyState.tsx:18-24
Timestamp: 2026-02-07T01:09:20.001Z
Learning: For multi-file uploads in apps/web, the planned batching strategy is: keep `addFile` for single-file operations, add `addFilesToFolder` (or similar batch method) that collects all file metadata and calls `updateFolderMetadata` once at the end, with a single `createAndPublishIpnsRecord` invocation per batch instead of per file.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comment thread apps/web/src/styles/upload.css Outdated
Comment thread apps/web/src/components/file-browser/UploadListItem.tsx Outdated
- Hover background now uses :not() to exclude error/complete rows,
  preserving their state-specific tint on hover
- Completion auto-removal (1s timer) moved from UploadListItem's
  useEffect to setFileStatus in the store, so completed entries are
  always pruned even if the user navigates away from the folder

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: 9277e581df62
@FSM1 FSM1 temporarily deployed to feat/inline-upload-progress - cipher-box PR #410 March 30, 2026 13:05 — with Render Destroyed
@FSM1 FSM1 merged commit 81c9f71 into main Mar 30, 2026
25 checks passed
@FSM1 FSM1 deleted the feat/inline-upload-progress branch March 30, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants