feat(messages): support buffer-backed attachments in sendMultipart - #28
feat(messages): support buffer-backed attachments in sendMultipart#28ZhYGuoL (ZhYGuoL) wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR extends the ChangesByte-backed multipart attachment uploads
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
README.md (1)
203-204: ⚡ Quick winConsider adding the import statement for consistency.
The example uses
readFilewithout showing the import. Other attachment examples in the README (line 92) includeimport { readFile } from "node:fs/promises";for clarity.📝 Suggested addition
+import { readFile } from "node:fs/promises"; + const bytes = await readFile("photo.png");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 203 - 204, Add the missing import for readFile used in the example: include an import like import { readFile } from "node:fs/promises"; at the top of the example so the readFile call is shown with its required import (match the style used for other attachment examples).src/resources/messages.ts (1)
113-143: 💤 Low valueConsider validating mutually exclusive attachment fields.
When a part has both
attachment(bytes) andattachmentGuid(pre-uploaded), the current logic prioritizesattachmentGuidand silently ignores the byte data (line 128). While this aligns with the design ("parts with existing attachmentGuid remain unchanged"), carrying unused byte data through the call chain wastes memory and may confuse callers.Consider adding validation to detect and reject this ambiguous state:
🛡️ Optional validation to prevent ambiguous part configuration
private async normalizeMultipartParts( parts: readonly MessagePart[] ): Promise<readonly MessagePart[]> { if (!parts.some(hasByteBackedAttachment)) { return parts; } if (!this._uploadAttachment) { throw new Error( "messages.sendMultipart received a byte-backed attachment part without upload support" ); } const normalizedParts: MessagePart[] = []; for (const part of parts) { + if (hasByteBackedAttachment(part) && part.attachmentGuid) { + throw new Error( + "MessagePart cannot have both attachment (bytes) and attachmentGuid; provide one or the other" + ); + } if (!hasByteBackedAttachment(part) || part.attachmentGuid) { normalizedParts.push(part); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resources/messages.ts` around lines 113 - 143, In normalizeMultipartParts, detect parts where both attachment (byte-backed) and attachmentGuid are present and reject them instead of silently ignoring bytes: inside the loop in normalizeMultipartParts (or before processing) add a validation that throws a clear Error when a MessagePart contains both attachment and attachmentGuid, referencing the function name normalizeMultipartParts and the MessagePart fields attachment and attachmentGuid; alternatively, if you prefer to accept and free memory, explicitly delete or null out attachment when attachmentGuid is present and log a warning—ensure the validation/cleanup occurs before calling _uploadAttachment or pushing parts to normalizedParts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 198: Add a proper section header before the multipart documentation line
("Multipart sends are atomic and can mix text, mentions, and attachments.") to
match the style of other major sections (e.g., "## Send Messages", "## Send
Attachments"); insert a header like "## Multipart Messages" immediately above
that sentence so the multipart content is discoverable and consistent with the
rest of the README.
---
Nitpick comments:
In `@README.md`:
- Around line 203-204: Add the missing import for readFile used in the example:
include an import like import { readFile } from "node:fs/promises"; at the top
of the example so the readFile call is shown with its required import (match the
style used for other attachment examples).
In `@src/resources/messages.ts`:
- Around line 113-143: In normalizeMultipartParts, detect parts where both
attachment (byte-backed) and attachmentGuid are present and reject them instead
of silently ignoring bytes: inside the loop in normalizeMultipartParts (or
before processing) add a validation that throws a clear Error when a MessagePart
contains both attachment and attachmentGuid, referencing the function name
normalizeMultipartParts and the MessagePart fields attachment and
attachmentGuid; alternatively, if you prefer to accept and free memory,
explicitly delete or null out attachment when attachmentGuid is present and log
a warning—ensure the validation/cleanup occurs before calling _uploadAttachment
or pushing parts to normalizedParts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2258784-4009-4cb9-ad21-a6d75e09a2e0
📒 Files selected for processing (5)
README.mdsrc/client.tssrc/resources/messages.tssrc/types/messages.tstests/unit/messages-resource.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Keep functions focused and under reasonable cognitive complexity limits
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Group related code together and separate concerns
Don't useeval()or assign directly todocument.cookie
Validate and sanitize user input
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Avoid barrel files (index files that re-export everything)
Write assertions insideit()ortest()blocks`**/*.{ts...
Files:
src/client.tstests/unit/messages-resource.test.tssrc/resources/messages.tssrc/types/messages.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Use TypeScript for all new projects and modules
Files:
src/client.tstests/unit/messages-resource.test.tssrc/resources/messages.tssrc/types/messages.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx,js,jsx}: Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
tests/unit/messages-resource.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc)
Write comprehensive unit tests for all functions and components
Files:
tests/unit/messages-resource.test.ts
🔇 Additional comments (15)
README.md (1)
200-201: LGTM!Also applies to: 217-228
src/types/messages.ts (2)
188-196: LGTM!
199-200: LGTM!src/resources/messages.ts (5)
57-61: LGTM!
63-67: LGTM!
105-111: LGTM!
231-234: LGTM!
79-80: LGTM!src/client.ts (3)
203-204: LGTM!
403-404: LGTM!
460-463: LGTM!tests/unit/messages-resource.test.ts (4)
47-64: LGTM!
603-644: LGTM!
646-678: LGTM!
680-707: LGTM!
|
|
||
| Multipart sends are atomic and can mix text, mentions, and uploaded | ||
| attachments: | ||
| Multipart sends are atomic and can mix text, mentions, and attachments. |
There was a problem hiding this comment.
Add a section header for multipart messages.
The multipart documentation starts without a section header, inconsistent with other major sections like "## Send Messages", "## Send Attachments", and "## Chat Backgrounds". This impacts navigation and makes the document harder to scan.
📝 Suggested fix
+## Multipart Messages
+
Multipart sends are atomic and can mix text, mentions, and attachments.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Multipart sends are atomic and can mix text, mentions, and attachments. | |
| ## Multipart Messages | |
| Multipart sends are atomic and can mix text, mentions, and attachments. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 198, Add a proper section header before the multipart
documentation line ("Multipart sends are atomic and can mix text, mentions, and
attachments.") to match the style of other major sections (e.g., "## Send
Messages", "## Send Attachments"); insert a header like "## Multipart Messages"
immediately above that sentence so the multipart content is discoverable and
consistent with the rest of the README.
1 participant
Summary
Adds buffer-backed attachment parts to
messages.sendMultipart(...). Callers can passattachment: { data, fileName, … }on a multipart bubble; the SDK uploads bytes via the existingattachments.uploadflow, then sends the multipart message usingattachmentGuid/attachmentNameon the wire—same shape as today’s uploaded-GUID parts.Closes / addresses Issue #24 — Support buffer-backed attachments in sendMultipart.
Motivation
Previously, multipart sends only supported attachment bubbles via
attachmentGuid(and optionalattachmentName). Apps that already held file bytes in memory had to write temp files visible to the helper or pre-upload every part themselves. This change keeps the SDK thin: client-side normalize → upload → send, without proto or server changes.What changed
Public API (
src/types/messages.ts)MultipartAttachmentInput: extendsAttachmentInputwith an optionalmimeTypefield documented as a call-site hint only (upload RPC still infers type from bytes/filename;mimeTypeis not sent as a separate upload field).MessagePart: optionalattachment?: MultipartAttachmentInputfor byte-backed bubbles.Behavior (
src/resources/messages.ts)sendMultipartscans parts forattachmentwithoutattachmentGuid.uploadAttachmentwith theAttachmentInputpayload.attachmentGuid+attachmentName(defaulting name fromattachment.fileNamewhenattachmentNameis omitted).bubbleIndex, and text/mention/formatting fields unchanged.attachmentGuidare unchanged.Wiring (
src/client.ts)createClientconstructsAttachmentsResourcefirst, then passesuploadAttachment: (input) => attachments.upload(input)intoMessagesResourceso production uses the real upload RPC.Docs (
README.md)attachmentGuidexample retained.Tests
New coverage lives under
describe("byte-backed multipart attachments", …)intests/unit/messages-resource.test.ts.uploads byte-backed parts and sends the uploaded attachment guiduploadAttachment,sendMultipartcalls upload once with the expectedfileNameanddata. The capturedsendMultipartMessagerequest has three parts in order (text → attachment → text). The middle part’s wireattachmentis{ attachmentGuid, attachmentName }only—no rawdata—so normalization matches the intended upload-then-send contract.preserves bubbleIndex on uploaded multipart attachment partsbubbleIndexon a byte-backed part is still present on the outgoing multipart payload alongsideattachmentGuid/attachmentName.does not send multipart request when byte-backed attachment upload failsuploadAttachmentthrows,sendMultipartrejects with that error and the stubsendMultipartMessageis never called (sendCalls === 0), so a failed upload does not trigger a send.Existing multipart tests (text-only,
attachmentGuid, mentions, formatting,bubbleIndexforwarding, etc.) continue to pass unchanged.How to verify locally
bun install bun run check bun run lint bun test bun run buildChecklist
attachmentGuidmultipart partsbubbleIndex, and upload failure short-circuitNeed help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Documentation