Conversation
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Good work overall — the migration from the broken REST multipart upload to ConnectRPC gRPC-Web client-streaming is well-structured. The streaming protocol, chunking, progress reporting, abort handling, and response mapping are all correct. The useFileAttachments hook remains compatible without changes.
Two major issues to address before merge:
- Weakened type union (
UploadFileRequest): The{ case: undefined }variant allows invalid requests to type-check. Remove it. - Unnecessary defensive URL fallbacks:
config.apiBaseUrlis always defined; the multi-layered fallback in transport URL construction is dead code that obscures intent.
Two minor suggestions for robustness and maintainability are also included.
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
All four review comments have been addressed and verified in the actual code:
- ✅
{ case: undefined }variant removed —UploadFileRequestunion is now strict overmetadata|chunk. - ✅ Defensive URL fallbacks removed — transport URL is now a clean single-line derivation from
config.apiBaseUrl. - ✅ Registry resolver fails loudly — throws on unexpected external dependency instead of silently returning
undefined. - ✅ Provenance comment added — descriptor block now documents the source proto and maintenance expectation.
The implementation correctly migrates file uploads from the non-existent REST endpoint to the ConnectRPC gRPC-Web client-streaming endpoint. LGTM.
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
New changes reviewed — all clean. No issues found.
AttachmentPreviewStrip.tsx: Minimal, single-linedata-statusattribute addition. Exposes existing typed state for e2e testability without affecting production rendering. ✅test/e2e/file-upload.spec.ts: Well-structured e2e test following established project patterns. Assertions cover the full upload lifecycle: chip visibility, status transition tocompleted, message display with attachment count text. Timeouts are appropriate. ✅test/e2e/fixtures/test-upload.txt: Minimal fixture. ✅
Previously approved core upload changes (files-connect.ts, files.ts) remain untouched. LGTM — ready to merge.
Summary
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
The architectural pivot from gRPC-Web to raw Connect protocol envelopes via fetch is well-reasoned — @connectrpc/connect-web doesn't support client-streaming in browsers, so this is a necessary workaround.
The envelope serialization/parsing logic (createEnvelope, readEnvelope, concatEnvelopes, parseEndStream) is correct per the Connect protocol spec. The response handling properly validates flags, parses the end-stream message, and checks for trailing data. files-connect.ts is cleanly simplified to export only message descriptors. files.ts is now a thin adapter layer, which is good separation.
Two major issues:
-
Misleading progress reporting —
onUploadProgressfires during in-memory envelope building, reaching 100% before thefetchstarts. For large files on slow connections, the user sees a full progress bar while no data has been sent. Consider usingXMLHttpRequest(which supportsupload.onprogress) for real byte-level progress, or remove progress reporting entirely. -
Duplicate type definitions —
UploadProgressEventandUploadProgressHandlerare independently defined in bothfiles.tsandupload-file-connect.ts.
Two minor items:
-
Unused dependencies —
@connectrpc/connectand@connectrpc/connect-webremain inpackage.jsonbut are no longer imported. -
Dead service descriptor — The
serviceblock in the proto descriptor is no longer consumed.
Summary
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
All four review comments verified in the actual code:
-
✅ Real upload progress via XHR —
postUploadRequestusesXMLHttpRequestwithxhr.upload.onprogressfor byte-level network progress. Envelope building no longer fires progress callbacks. Abort signal integration is correct with proper cleanup in all terminal paths (onload, onerror, onabort). -
✅ Centralized progress types — Single source of truth in
src/api/types/upload.ts. Bothfiles.tsandupload-file-connect.tsimport from there.files.tsre-exports for external consumers. -
✅ Unused dependencies removed —
@connectrpc/connectand@connectrpc/connect-webremoved frompackage.jsonandpnpm-lock.yaml. Zero references in source. -
✅ Dead service descriptor removed —
files-connect.tsno longer contains the unusedserviceblock.
The XHR implementation is clean: proper arraybuffer response handling, correct header setup, and the lengthComputable fallback to body.byteLength is a good touch. The overall architecture — descriptors in files-connect.ts, Connect protocol transport in upload-file-connect.ts, domain mapping in files.ts — has clear separation of concerns. LGTM.
Summary
Test & Lint Summary
|
Summary
Testing
Closes #54