Repository navigation
feat(files): multipart upload module + attach files to comments - #413
Merged
Merged
Conversation
…ntract
- regenerate packages/components/schema.graphql against the deployed ported backend
(image/avatar/bannerImage fields are now String scalars, not { url } objects)
- update fragments to select image fields as scalars; drop { url } subselections
- update components/utils to consume image fields as strings (coalesce null -> undefined
where passed to string|undefined props)
- regenerate Relay __generated__ artifacts
- rebased onto origin/master
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XyuGcRJryi9hQiWRVXU7Js
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd # Conflicts: # packages/components/modules/profiles/web/ProfileMembers/MemberItem/index.tsx # packages/components/schema.graphql
Master's InviteMemberDialog queried image { url }; on this branch Profile.image
is a String scalar, so select it directly and use it as avatarUrl.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
README claimed a FileUploadProvider JSX wrapper; the store is a module-level zustand singleton, so no provider exists. Documented the shared-state implication for multiple targets and fixed the edges.map any cast. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
- Record ETags per chunk index as chunks complete (new recordChunkEtag store action); completedChunks was chunkIndex+1, which under 3-way parallel uploads mis-counted on out-of-order completion, and etags were only stored at the very end — a paused upload resumed with wrong slices and empty parts. - Resume now uploads exactly the chunks with missing ETags (non-contiguous safe) against their original presigned URLs. - Track initiatedAt/expiresIn from the initiate response; resuming past expiry aborts the stale upload (best effort) and transparently re-initiates. - Extract shared startUpload used by upload/retry/expired-resume. - Drop parentContentType/parentObjectId from initiate payloads — the API accepts parent_id only, so the fields were silently ignored; attaching is done via fileAttachToTarget. - Select id on FilesList file nodes (was masked by the removed any cast); remove leftover merge-conflict marker in ProfileMembers MemberItem. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…paths chunkFile slicing, uploadChunks (out-of-order ETag indexing, retry/backoff, abort short-circuit), the upload store (recordChunkEtag sparse counting, pause/resume/retry transitions), and useChunkedUpload (initiate/complete payloads, sparse resume, expired-URL re-initiation, failed-state handling). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…ume works Two bugs made in-session pause/resume unreachable from the UI (the jest tests exercised resumeUpload directly on a hand-crafted PAUSED state, masking both): - The catch in upload/resume/retry set status=FAILED on any thrown error, including the AbortController rejection a pause triggers — clobbering the PAUSED the store just set and firing onUploadError for a deliberate pause. A shared handleUploadError now leaves PAUSED/ABORTED untouched. - UploadingFileItem.handleResume called the store's resumeFile (PAUSED→PENDING) before resumeUpload, whose guard requires PAUSED — so resume no-op'd and the file stranded in PENDING. Removed the redundant call; the hook flips it. - resumeUpload now computes pending chunks before treating URL expiry as fatal: if every chunk already uploaded, it only re-issues the (URL-free) complete call instead of discarding the whole upload and re-uploading from byte zero. Added regression tests for both fixed behaviors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…App design) Decouple the three concerns so the pieces can be mounted to match the Figma comment design while keeping today's behaviour as defaults: - useFileSelect: headless hidden-input select (accept/size/count validation), so the trigger is pluggable. - FileUploadTrigger: owns the upload logic and renders as a dropzone (default) or a compact paperclip button (as="button"). FileUpload stays as a back-compat dropzone wrapper. - FileThumbnail + FileChip: presentational 40x40 thumbnail (image or type-icon tile) and the compact bordered chip from the design. - UploadingFileItem / AttachedFileItem gain variant='chip'; AttachedFileItem gains editable (× remove on files the user can change). - FileUploadList gains variant (cards|chips), layout (stack|horizontal), and editable — cards/stack remain the defaults so nothing changes for existing callers. Wire comments to the design: CommentItem renders the chip list (editable while editing); CommentUpdate renders the paperclip trigger button. Tests: useFileSelect unit coverage; full components suite green (37). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
…ion bar
- AttachedFileItem and FileUploadTrigger now use the design-system IconButton
(which renders a spinner via isLoading) instead of raw MUI: the remove/delete
action shows isLoading={isDeletingFile}, the attach button isLoading while
attaching.
- Wire the comment editor's attach icon: SocialInput forwards a new optional
SocialUpsertActionsProps to its SocialUpsertActions slot, and CommentUpdate
passes a module-level CommentFilesUpsertActions (target-bound) whose
AttachmentIcon is the real FileUploadTrigger. Removes the standalone attach
button that used to sit below the input.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
- The attached-file preview used file.thumbnail, which the backend leaves null when easy_thumbnails can't generate one, and downloads used the raw `file` field (a host-less /media path the frontend origin can't serve). Query the absolute `url` field and use it: preview falls back to thumbnail || url so images always render, and download opens `url`. - FileChip max-width 264 -> 232px to match the design. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Previously the uploading item decoded the source File (via createObjectURL) into an image preview. Even for a 40px tile the browser decodes the full-res bitmap, which is wasteful for large images. Show a generic type icon during upload; the real thumbnail arrives when FileAttachToTarget prependEdges the committed File node. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
Adds file attachment to CommentCreate, mirroring CommentUpdate. Because the comment does not exist at selection time, files upload immediately and their relay ids are held by a new useDeferredFileAttachments hook; on successful create they are attached to the new comment in one FileAttachToTarget mutation (the comment's FilesList_files connection already exists via the CommentItem response, so the prependEdge lands). The attach icon lives in a create-specific SocialUpsertActions bar and shows a loading state while uploads are in flight. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
The upload store is a global singleton, so every FileUploadList (one per comment) was rendering all in-flight uploads — a file attached during comment creation appeared on existing comments. Tag each upload with a scope: the auto-attach path scopes to its target id; the deferred (create) path scopes to a per-composer id. FileUploadList now shows only uploads matching its target, and CommentCreate renders a new target-less UploadingFilesList for its own scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
In a composer (e.g. new comment) an uploaded-but-not-yet-attached file kept showing its progress bar and a pause control, with no way to undo it. Once COMPLETED the chip now shows the file size instead of the bar (card hides FileProgress) and offers a remove control. Removing works correctly because the deferred hook now reads the completed uploads for its scope straight from the store at attach time (fileRelayId is persisted on completion), so a removed file is excluded from the attach; cleanup is scoped to the composer instead of clearing all completed uploads globally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0146oeiGsELS6QVpVUQHDQXd
|
|
Important Review skippedToo many files! This PR contains 151 files, which is 51 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configuration
📒 Files selected for processing (151)
You can disable this status message by setting the ✨ 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 |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Master refactored the comments module into a shared hook layer (#392) and converted tailwind to MUI styled components (#408), touching the same components this branch extends with file attachments. Resolved by taking master's structure and re-applying the attachment feature on top: - CommentCreate/CommentUpdate/CommentItem rebuilt on master's useCommentCreateForm / useCommentUpdateForm / useCommentItem hooks and styled components, keeping the file upsert actions, UploadingFilesList and FileUploadList. - useCommentCreateForm now surfaces the created comment id to onSuccess, which the deferred-attachment flow needs to attach uploads once the comment exists. This branch makes thumbnails resolve to a plain String, so master's `image/avatar { url }` selections and readers had to be adapted: AddContactToGroupItem's fragment plus three consumers new in master. Relay compiles and tsc is clean; the remaining lint output is pre-existing warnings only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
Three unrelated CI failures on this branch, all fallout from the master merge:
- The components package moved to Vitest on master. The shared setup aliases
globalThis.jest = vi, so jest.fn() kept working and hid the problem, but
vi.mock is hoisted by the transform only when written literally — reached
through the alias it never hoists, so the real module was imported and the
"mock" had no mock methods. Ported the three files-module specs to vi,
including jest.requireActual -> await vi.importActual.
- The image/avatar -> String schema change was only half applied to fixtures:
25 sites across 5 files still used the old { url } shape, plus a
createNodeProfile helper whose parameter was typed { url: string }. The
earlier sweep caught readers, not mock payload shapes.
- Math.random() generated local-only ids (upload store keys, composer scopes)
and tripped Sonar's S2245. Replaced with a counter-based nextLocalId, which
also avoids crypto.randomUUID's secure-context requirement under jsdom.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
3 of 4 tasks
…ragment selects CommentItem's fragment now spreads ...FilesList_target, so every mocked Comment must carry isFilesEnabled, filesCount, files and the __isFilesInterface/__isNode discriminators. Shapes taken from a live backend response, not guessed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
Master migrated component tests from Cypress to Playwright (#427), deleting every .cy.tsx spec. Conflict was Members.cy.tsx — deleted there, modified here (thumbnail String conversion); took the deletion and applied the equivalent conversion to its replacement, Members.story.tsx. Also converted two single-line `image: { url }` mocks in GroupChatEdit that the earlier multi-line sweep missed. `image`/`avatar` are String on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
groupDetailsResponse still wrapped the image as `{ url }` behind a conditional,
which the literal-pattern sweep could not match. `image` is a String on this
branch, so the avatar rendered nothing and the pre-fill assertion failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
attachTo is async, so calling it bare from onSuccess left a floating promise (Sonar typescript:S9383, the sole MAJOR holding new-code reliability at C). Mutation failures are already reported through attachTo's own onError, so this stays fire-and-forget — the catch just keeps a rejection from escaping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
nossila
marked this pull request as ready for review
October 6, 2026 14:52
nossila
requested review from
anicioalexandre,
matheusysd and
priscilladeroode
as code owners
October 6, 2026 14:52
matheusysd
reviewed
Oct 6, 2026
matheusysd
left a comment
matheusysd
left a comment
Contributor
There was a problem hiding this comment.
[Critical] Breaking change without a changeset. image { url } → String across ~10 modules breaks Relay compilation for every consumer/fork and requires a lockstep backend deploy, but there's no .changeset/version bump.
Fix: add a major changeset with a migration note (image { url } → image, x.image?.url → x.image ?? undefined). Ideally split the image-contract change into its own PR so the files module can ship independently.
…re-renders Addresses matheusysd's review on #413. Cross-composer file loss: clearCompleted() deleted every COMPLETED upload in the global store, so attaching on comment A wiped files waiting in the create composer. Attach-side cleanup now removes exactly the files it just attached, and the store gains clearScope() for the "discard this composer" case. Partial batches: Promise.all meant one paused or failed upload discarded the ids of the ones that finished. Now allSettled + attach the fulfilled ids. Resume and retry land outside the original batch, so useChunkedUpload's onUploadComplete attaches late arrivals too, deduped against the batch so nothing attaches twice. Wrong-comment attachment: attachTo now snapshots its scope before awaiting, so files added while it settles belong to the next comment, and it releases the whole snapshot afterwards so a paused upload cannot ride along. The composer also blocks submit via hasPendingFiles while anything is unfinished. Re-render storm: every progress tick replaces the store's Map, so whole-store subscribers re-rendered on each XHR event. Lists now read through a scoped useScopedUploads selector and action consumers pick individual actions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
The image/avatar/bannerImage fields went from an object to a String across the
components package, which breaks Relay compilation for every consumer and needs
the matching backend. That is a major for @baseapp-frontend/components and had no
changeset.
Records the migration (drop the selection set; `x.image?.url` -> `x.image ?? undefined`),
names the fragments changed so a fork can diff against them, and notes that mock
payloads need the same fix since a stale `{ url }` fixture renders nothing silently
instead of failing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
matheusysd
approved these changes
Oct 6, 2026
anicioalexandre
approved these changes
Oct 6, 2026
Per anicioalexandre's review and the styling conventions: styled() is the primary approach, Tailwind the last resort. The grid moves into a co-located styled.tsx using theme.spacing(1), which is the same 8px `gap-2` resolved to — no visual change. Applied to the CommentUpdate bar too, which carried the identical class. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
Picks up the fileAttachToTarget example fix (FilesInterface, not CommentsInterface) plus drift the committed snapshot had missed: field descriptions, and several File fields tightening from nullable to non-null (fileContentType, fileName, name, description, uploadId). Relay artifacts recompiled; tsc clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013LkamMd6XMPeYNHbAV1DZ9
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a files module (
@baseapp-frontend/components/files) — multipart chunked upload plus attaching files to anyFilesInterface-enabled object — and wires it into comments. Consumes the backendbaseapp_filespackage (PR onsilverlogic/baseapp-backend).What's included
Files module (
packages/components/modules/files)useChunkedUpload): 5 MB chunks, concurrent parts, XHR progress, exponential-backoff retry, and in-session pause/resume/retry viaAbortController. Backed by a zustand upload store.FileUploadTrigger(dropzone or compact icon button),FileUploadList/UploadingFilesList,AttachedFileItem,FileChip,FileThumbnail— split so consumers can mount just the pieces they need and reproduce the BaseApp Figma design.useFileUploadLogic(upload → auto-attach to an existing target) anduseDeferredFileAttachments(upload now, attach after the target is created).FileAttachToTarget/FileDeletemutations,FilesList_targetonFilesInterface,FileItem_file.files/native) for a later pass.Comments integration
CommentUpdate: attach icon in theSocialInputaction bar; files auto-attach to the existing comment.CommentCreate: attach icon uploads immediately, then files attach to the new comment on successful create (the comment'sFilesList_filesconnection already exists via theCommentItem_commentresponse, so the@prependEdgelands). Composer shows its own upload progress; completed items hide the progress bar and expose a remove control.Schema / other
schema.graphqlsynced with backend (ContentPost implements CommentsInterface, files types) and image fields adapted to theStringcontract across profiles/messages/notifications.Tests
chunkFile,uploadChunk/uploadChunks(retry/backoff/ETag), the upload store,useFileSelect, and pause/resume paths.pnpm relaycompiles per package against the committed schema;tsc --noEmitclean; eslint clean.Status
Draft — depends on the backend
baseapp_filesPR. Opening for early review of the module API and comments wiring.🤖 Generated with Claude Code