Conversation
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
Jessie-QingYu
left a comment
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR adds real multipart upload progress for chat attachments by routing attachment-bearing turns through XMLHttpRequest (which exposes upload.onprogress) while keeping the existing fetch path for text-only turns. The response handling is cleanly refactored into a shared parseTurnResponseText so both transports produce identical PostTurnResult shapes, and the new onUploadProgress option is backward-compatible (all three call sites verified). The composer now keeps attachment chips visible with an accessible progress bar during the request, locks input/attachment mutation until the upload settles (guarding the imperative handle, paste, drop, Enter, and click paths via both state and a ref for the same-event gap), and retains attachments for retry on failure. Error/success upload-clearing logic in runSend and the imperative submit was traced and is consistent with runTurnLifecycle, which throws on a failed POST so the composer's catch restores inputs.
Overall this is a solid, carefully-reasoned change. The two observations below are about the new full-composer lock: it introduces a behavioral change (attachment turns previously cleared immediately and let the user keep composing) with no way to cancel an in-flight upload, and the XHR abort branch is currently unreachable.
Verdict: COMMENT — The change is correct, well-factored (shared response parsing, XHR scoped to attachment turns), and well-tested; findings are non-blocking UX/robustness considerations.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | packages/web/src/lib/chat-api.ts |
No way to cancel an in-flight attachment upload; abort branch is dead code |
| P3 | design | packages/web/src/components/chat/ChatComposer.tsx |
Full composer lock is a behavioral change from the prior non-blocking send |
| P3 | code-quality | packages/web/src/lib/chat-api.ts |
Progress bar can display 100% + "Uploading files…" while the server is still processing |
zoolsher
left a comment
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR cleanly routes attachment-bearing turns through XHR for real multipart upload progress while preserving the existing fetch path for text-only turns. The composer correctly retains attachments for retry, synchronously guards duplicate sends and attachment mutation, and exposes accessible aggregate progress; response parsing remains consistent across both transports. One non-blocking recovery gap remains for requests that never settle. The targeted tests could not be rerun because this checkout has no installed rstest binary.
Verdict: COMMENT — The implementation is sound overall, but stalled attachment requests can leave the newly locked composer unusable without reloading.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | packages/web/src/lib/chat-api.ts |
Stalled uploads can lock the composer indefinitely |
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
Jessie-QingYu
left a comment
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: ✅ APPROVE
The PR adds real multipart upload progress for chat attachments by using XMLHttpRequest (only when attachments are present and a progress callback is supplied) while retaining the fetch path for text-only turns. parseTurnResponse was refactored into a status/text-based parseTurnResponseText shared by both transports, keeping the JSON/error-parsing contract identical. The composer now surfaces an accessible aggregate progress bar, keeps attachment chips visible while uploading, and locks the composer (textarea, send button, file input, paste, drop, and imperative handle methods) until the send settles, restoring text/attachments on failure for retry.
I verified both onSend call sites (Chat.tsx via runTurnLifecycle and ChatComponent.tsx handleDraftSend) throw on !result.ok, so the composer's catch correctly restores inputs; the XHR path resolves for all HTTP statuses and only rejects on genuine transport/abort errors, matching fetch semantics. The upload-in-flight ref closes the same-tick double-submit gap, and addPendingFiles guards paste/drop entry points. inputId is set before transport selection, credentials are sent (withCredentials), and Content-Type/boundary is left to the browser. i18n keys were added to both existing locales. Overall this is a solid, low-risk change with good test coverage.
Verdict: APPROVE — Clean, well-tested change that preserves the existing API contract and correctly gates all input paths during an in-flight upload; no correctness or security defects found.
No issues found. Code looks good!
zoolsher
left a comment
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR cleanly confines XHR to multipart attachment sends, preserves the existing fetch path for text-only turns, and threads upload progress through the composer while retaining failed attachments for retry. The locking and response-parsing changes fit the existing component/API boundaries, but the rendered progress remains aggregate-only despite the stated per-file behavior.
Verdict: COMMENT — The upload transport and retry flow look sound, but the advertised per-file progress is not implemented for multi-file uploads.
1 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | design | packages/web/src/components/chat/composer/PendingUploadsList.tsx |
Multi-file uploads expose only aggregate progress |
There was a problem hiding this comment.
🔁 This review has been superseded. See the latest review.
zoolsher
left a comment
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
The PR cleanly confines XHR to attachment-bearing multipart turns, preserves the text-only fetch path, and consistently propagates progress through the composer while retaining failed attachments for retry. Call sites, backend multipart parsing, metadata locking, and response/error handling were verified beyond the diff.
Two P2 issues remain: the implementation no longer provides the described per-file progress, and a stalled XHR can lock the entire composer indefinitely. Targeted tests could not be rerun because this checkout has no installed rstest binary.
Verdict: COMMENT — The upload transport and retry flow are sound, but aggregate-only reporting and unrecoverable stalled uploads leave two non-blocking product gaps.
2 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | design | packages/web/src/components/chat/composer/PendingUploadsList.tsx |
Multi-file uploads expose only aggregate progress |
| P2 | error-handling | packages/web/src/lib/chat-api.ts |
A stalled upload can lock the composer indefinitely |
…omposer Review feedback on the aggregate progress bar was that it wiped the typed message, could not be cancelled, and pushed the composer around on every send. Progress now rides inside each attachment chip as a ring, taking the remove button's slot rather than adding a row. The browser reports progress for the whole multipart body, but that body is written sequentially, so each file occupies a known ordered byte range within it — mapping the aggregate onto those ranges is a real per-file reading, not an even split. Part headers and boundaries are ignored, so a ring completes a hair early; an incomputable body size spins the ring instead of inventing a percentage. Keeping the request's one visual element inside geometry that already exists is what removes the shift: the chip and composer boxes measure pixel-identical before and during an upload. A turn carrying attachments now keeps its text until the server accepts it, because the request lasts long enough to cancel and wiping text the user may be about to reclaim reads as loss. Text-only turns still clear optimistically. Cancelling aborts the real request through an AbortSignal threaded from the composer to postSessionTurn. The draft is left exactly as it was, and the send failure banner is suppressed for AbortError — a cancel is a deliberate act, not a transport failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Jessie-QingYu
left a comment
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This adds an XHR multipart path with cancellation and composer locking while preserving fetch for text-only sends. The component and transport call chains, changed public types, server input-id admission path, and relevant tests were reviewed; targeted tests could not run because this shallow clone has no node_modules (rstest is unavailable).
Verdict: REQUEST_CHANGES — The patch can report fabricated per-file progress and can duplicate a turn when a retained upload is retried after an ambiguous cancellation or transport failure.
2 finding(s) — 1 posted as inline comments below; 1 listed in this table only (no matching diff line).
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | correctness | packages/web/src/components/chat/Chat.tsx |
Keep the turn input ID stable across an upload retry |
| P1 | correctness | packages/web/src/components/chat/composer/PendingUploadsList.tsx |
Do not derive per-file progress from multipart request progress |
| // of dividing by zero. | ||
| if (total === 0) return sizes.map(() => overall); | ||
|
|
||
| const transferred = overall * total; |
There was a problem hiding this comment.
[P1] correctness — Do not derive per-file progress from multipart request progress
XMLHttpRequest.upload.loaded/total counts every multipart byte, including the text, reasoning, workspace, part headers, and boundaries that are sent before the files. Thus a small file can show 100% before its first byte is transmitted (and a zero-byte file is immediately complete), so these accessible per-file values are not real progress. Display the single aggregate request fraction for the attachment group instead, or only expose per-file percentages if the transport provides known per-part byte ranges.
3 participants
Summary
XMLHttpRequest.uploadfetchpath and multipart API contractInvestigation
Chat attachments were previously held as local
Fileobjects inChatComposerand appended to the chat turn'sFormDataonly when the message was submitted. The multipart request usedfetch, which does not expose request-body upload progress in the browser. This change uses XHR only for turns containing attachments and displays that aggregate request progress once for the attachment group, avoiding unsupported estimates of individual multipart boundaries.Test plan
pnpm --filter rome-web exec rstest run src/lib/chat-api.test.ts src/components/chat/ChatComposer.test.tsx src/components/project-selector.test.tsx --silent passed-only(24/24)pnpm --filter rome-web typecheckpnpm --filter rome-web buildpnpm --filter rome-web test(174 files, 1,422 tests; run before final rebase, whose intervening upstream commits did not touch this scope)pnpm typecheck(run before final rebase, whose intervening upstream commits did not touch this scope)No issue was designated for automatic closure.