Repository navigation
refactor(channel): share attachment infra (DM-files PR 1/2) - #1175
Conversation
Plan and epic for bringing DM file attachments to channel parity via a fileConversations join table, nullable files.driveId, and four DRY refactors that share an attachment-upload core, service-token issuer, processor binding, composer hook, and renderer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defines AttachmentMeta once in packages/db/src/schema/storage.ts (next to files) and re-exports from @pagespace/lib/types. Removes three duplicate inline shapes (channelMessages $type, attachment-utils.ts, channel messages route handler). Pure type refactor with no runtime change; sets up DM directMessages.attachmentMeta to import from the same canonical source without drifting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extracts the attachment-rendering JSX (image preview / file download card) from ChannelView and the inbox channel page into a single MessageAttachment component under components/shared. Both call sites now render <MessageAttachment message={m} /> and drop their FileIcon/FileText/Download imports plus the seven attachment-utils helpers they only used for that block. Pure code-motion; output is pixel-identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls drag/paste/picker upload state and error-handling logic out of ChannelInput into a reusable useAttachmentUpload hook parameterized by upload URL. The hook registers each in-flight upload with useEditingStore (type 'form') so SWR cannot clobber a pending upload, fixing a latent gap that affected channels and would have affected DMs. ChannelInput re-exports FileAttachment from the hook to keep existing consumers working. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR refactors attachment handling by extracting the shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 30 seconds.Comment |
Replaces module-level sessionCounter+timestamp with createId() (cuid2) for upload-session IDs — matches existing hook conventions in useVoiceMode and useDocument and removes shared mutable module state. Also drops the unused setAttachment from the hook's return; the only consumer (ChannelInput) uses uploadFile/clearAttachment exclusively. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Adds 5 colocated tests covering: success path stores the attachment and brackets the upload in startEditing/endEditing; null uploadUrl is a no-op; 413 surfaces the file-too-large toast; thrown fetch errors still release the editing session; clearAttachment resets state. The original inline ChannelInput upload logic was untested; this PR's extraction is the right moment to lock the contract once for both channels and DMs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ChannelInput.tsx had a private formatFileSize identical to the one already exported from attachment-utils.ts. Imports the shared one and removes the local copy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/hooks/useAttachmentUpload.ts`:
- Around line 38-47: The uploadFile concurrency bug: multiple invocations race
on isUploading/setIsUploading; add a persistent ref-based in-flight counter
(e.g., uploadCountRef = useRef(0)) and in uploadFile (and the other upload
handler at lines ~90-93) increment uploadCountRef.current before starting, call
setIsUploading(true) if count becomes >0, then in finally decrement the counter
and call setIsUploading(uploadCountRef.current > 0); use the same
sessionId/startEditing/endEditing logic but rely on the ref counter to ensure
the loading flag is only cleared when all uploads complete.
- Around line 57-60: The response.json() results in useAttachmentUpload
currently assume a specific shape and directly access properties like
errorData.error and result.file.id/originalName/size/mimeType which can throw;
update the fetch error and success handling in the useAttachmentUpload hook to
perform runtime shape validation (e.g., using a small runtime guard or a schema
validator) before accessing fields: verify that errorData is an object with a
string "error" property before reading it, and verify that result is an object
with a "file" object containing string id, originalName, mimeType and numeric
size before constructing the Attachment payload; if validation fails, return or
throw a clear, handled error so callers don't encounter undefined-property
runtime exceptions (apply the checks around the response.json() usages in the
error branch and the success branch that reference result.file.*).
In `@tasks/dm-file-attachments.md`:
- Line 14: Update the doc to point to the canonical definition of AttachmentMeta
in the DB schema module (storage.ts) rather than the lib re-export; change the
line that currently references packages/lib/src/types.ts to reference the
schema's AttachmentMeta and note that channelMessages.attachmentMeta and
directMessages.attachmentMeta JSONB columns import that canonical type (i.e.,
reference AttachmentMeta in the DB schema module and mention both
channelMessages.attachmentMeta and directMessages.attachmentMeta).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a87bc01-6d93-47b4-9505-396fdf7a56da
📒 Files selected for processing (12)
apps/web/src/app/api/channels/[pageId]/messages/route.tsapps/web/src/app/dashboard/inbox/channel/[pageId]/page.tsxapps/web/src/components/layout/middle-content/page-views/channel/ChannelInput.tsxapps/web/src/components/layout/middle-content/page-views/channel/ChannelView.tsxapps/web/src/components/shared/MessageAttachment.tsxapps/web/src/hooks/useAttachmentUpload.tsapps/web/src/lib/attachment-utils.tspackages/db/src/schema/chat.tspackages/db/src/schema/storage.tspackages/lib/src/types.tsplan.mdtasks/dm-file-attachments.md
Addresses three CodeRabbit findings on PR #1175: 1. Race in uploadFile — multiple invocations would each setIsUploading(false) in finally, clearing the flag while later uploads were still in flight. Adds an isUploadingRef sibling to the React state so concurrent calls early-return without waiting for a state flush. New test covers the gate. 2. Untyped response.json() — adds explicit UploadErrorBody / UploadSuccessBody casts at the trust boundary. Skips a full runtime validator: the upload endpoint is internal and we control both sides; per CLAUDE.md, validation belongs at system boundaries (user input / external APIs), not between our own server and our own client. The cast satisfies the no-any rule without runtime overhead. 3. Epic doc misdirect — fixes the line that said AttachmentMeta lives in packages/lib/src/types.ts; the canonical home is packages/db/src/schema/storage.ts (lib re-exports it; db can't depend on lib without a cycle). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(tasks): add DM file attachments epic
Plan and epic for bringing DM file attachments to channel parity via a fileConversations join table, nullable files.driveId, and four DRY refactors that share an attachment-upload core, service-token issuer, processor binding, composer hook, and renderer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(types): centralize AttachmentMeta
Defines AttachmentMeta once in packages/db/src/schema/storage.ts (next to files) and re-exports from @pagespace/lib/types. Removes three duplicate inline shapes (channelMessages $type, attachment-utils.ts, channel messages route handler). Pure type refactor with no runtime change; sets up DM directMessages.attachmentMeta to import from the same canonical source without drifting.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(channel): share MessageAttachment renderer
Extracts the attachment-rendering JSX (image preview / file download card) from ChannelView and the inbox channel page into a single MessageAttachment component under components/shared. Both call sites now render <MessageAttachment message={m} /> and drop their FileIcon/FileText/Download imports plus the seven attachment-utils helpers they only used for that block. Pure code-motion; output is pixel-identical.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(channel): extract useAttachmentUpload hook
Pulls drag/paste/picker upload state and error-handling logic out of ChannelInput into a reusable useAttachmentUpload hook parameterized by upload URL. The hook registers each in-flight upload with useEditingStore (type 'form') so SWR cannot clobber a pending upload, fixing a latent gap that affected channels and would have affected DMs. ChannelInput re-exports FileAttachment from the hook to keep existing consumers working.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(useAttachmentUpload): tighten API surface
Replaces module-level sessionCounter+timestamp with createId() (cuid2) for upload-session IDs — matches existing hook conventions in useVoiceMode and useDocument and removes shared mutable module state. Also drops the unused setAttachment from the hook's return; the only consumer (ChannelInput) uses uploadFile/clearAttachment exclusively.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(useAttachmentUpload): cover happy path, errors, edit lock
Adds 5 colocated tests covering: success path stores the attachment and brackets the upload in startEditing/endEditing; null uploadUrl is a no-op; 413 surfaces the file-too-large toast; thrown fetch errors still release the editing session; clearAttachment resets state. The original inline ChannelInput upload logic was untested; this PR's extraction is the right moment to lock the contract once for both channels and DMs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(channel): drop duplicated formatFileSize
ChannelInput.tsx had a private formatFileSize identical to the one already exported from attachment-utils.ts. Imports the shared one and removes the local copy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(useAttachmentUpload): gate concurrent uploads + type response
Addresses three CodeRabbit findings on PR #1175:
1. Race in uploadFile — multiple invocations would each setIsUploading(false) in finally, clearing the flag while later uploads were still in flight. Adds an isUploadingRef sibling to the React state so concurrent calls early-return without waiting for a state flush. New test covers the gate.
2. Untyped response.json() — adds explicit UploadErrorBody / UploadSuccessBody casts at the trust boundary. Skips a full runtime validator: the upload endpoint is internal and we control both sides; per CLAUDE.md, validation belongs at system boundaries (user input / external APIs), not between our own server and our own client. The cast satisfies the no-any rule without runtime overhead.
3. Epic doc misdirect — fixes the line that said AttachmentMeta lives in packages/lib/src/types.ts; the canonical home is packages/db/src/schema/storage.ts (lib re-exports it; db can't depend on lib without a cycle).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Pure-refactor groundwork for DM file attachments — Tasks 1–3 of the DM File Attachments epic. Channels adopt three shared modules so the second caller (DM upload route in PR 2) drops in as a thin wrapper.
AttachmentMetatype (was 3 copies — db schema, attachment-utils, channel messages route). Lives inpackages/db/src/schema/storage.tsnext tofiles; re-exported from@pagespace/lib/typesfor non-db consumers.MessageAttachmentcomponent (was 2 copies of a 50-line block —ChannelView.tsx+inbox/channel/[pageId]/page.tsx). Pixel-identical output by construction.useAttachmentUploadhook withuseEditingStoreregistration so SWR cannot clobber an in-flight upload — closes a latent gap that affected channels too.ChannelInput.tsxis ~75 lines lighter.clearAttachment. The original inline upload logic was untested; this is the right moment to lock the contract once for both channels and DMs.Zero behavior change: channel rendering, channel upload, and channel send all work identically.
Out of scope (deliberate, lands in PR 2)
AttachmentTargetdiscriminated union — the abstraction is only meaningful with a second caller, which only appears in PR 2 (DM upload route). Keeping it out of this PR avoids forcing reviewers to imagine a future branch.fileConversationsschema migration,files.driveIdnullability, DM upload endpoint, DM POST acceptance offileId, DM composer wiring. All planned intasks/dm-file-attachments.md.Test plan
pnpm turbo typecheck lint— 4/4 tasks pass, no new warningsvitest run src/hooks/__tests__/useAttachmentUpload.test.ts— 5/5 pass (new)vitest run src/lib/ai/tools/__tests__/channel-tools.test.ts— 11/11 pass/dashboard/inbox/channel/[id], scroll history with mixed image/file attachments → identical to todayuseEditingStore)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes