image-attachment-support #1760
image-attachment-support #1760
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR introduces a comprehensive attachment system enabling file uploads for chat messages and enhanced note sessions. It adds a generic, reusable attachment storage layer with manifest tracking and concurrency control, integrates file input into message and editor UIs, and extends the AI enhancement workflow to process attached files as model inputs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatInput as Chat Input<br/>(UI)
participant Storage as Chat Attachment<br/>Storage
participant Transport as Chat Transport
participant Resolver as Attachment<br/>Resolver
participant Model as Language<br/>Model
User->>ChatInput: Select file, type message, send
ChatInput->>Storage: saveChatAttachment(groupId, file)
Storage-->>ChatInput: PersistedChatAttachment
ChatInput->>ChatInput: Build message with<br/>data-chat-file parts
ChatInput->>Transport: send(message, attachments)
Transport->>Resolver: resolveChatFileReferences(message, groupId)
Resolver->>Storage: readChatAttachmentAsDataURL(groupId, id)
Storage-->>Resolver: data URL
Resolver->>Resolver: Convert data-chat-file<br/>to file part with data URL
Resolver-->>Transport: Resolved message
Transport->>Model: stream(resolvedMessage)
Model-->>Transport: Streamed response
Transport-->>ChatInput: Display response
sequenceDiagram
participant User
participant Editor as Editor<br/>(raw/enhanced)
participant Header as Header/Tabs
participant Storage as Session Attachment<br/>Storage
participant NoteInput as Note Input<br/>Manager
participant EditorContent as Editor<br/>Content
User->>Header: Click Attachments tab
Header->>NoteInput: Switch to attachments view
User->>Header: Upload file
Header->>Header: File picker → select file
Header->>NoteInput: onUploadAttachments([file])
NoteInput->>Storage: saveSessionAttachment(sessionId, file)
Storage-->>NoteInput: PersistedAttachment
NoteInput->>NoteInput: Track in state, generate objectUrl
NoteInput-->>Header: Render in attachments list
User->>EditorContent: Switch back to editor tab
User->>Editor: Type/paste content<br/>(with attachment reference)
Editor->>NoteInput: onContentChange(content)
NoteInput->>NoteInput: Collect attachment references<br/>from editor
NoteInput->>NoteInput: Sync references with state<br/>Prune orphans
sequenceDiagram
participant User
participant Enhance as Enhance Task<br/>Workflow
participant AttachmentResolver as Resolve Session<br/>Attachments
participant Model as Language<br/>Model
User->>Enhance: Trigger enhance on session<br/>with attachments
Enhance->>Enhance: Load session attachments<br/>via transformArgs
Enhance->>AttachmentResolver: resolveSessionAttachments(sessionId, attachments)
loop For each image attachment
AttachmentResolver->>AttachmentResolver: Check size & MIME type
AttachmentResolver->>AttachmentResolver: Read file, convert to<br/>base64 data URL
AttachmentResolver-->>Enhance: FilePart[] with data URLs
end
Enhance->>Enhance: Build messages array:<br/>enhancedPrompt + attachmentParts
Enhance->>Model: streamText(messages)
Model-->>Enhance: Generated enhancement
Enhance-->>User: Display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (11)
apps/desktop/src/components/main/body/sessions/note-input/attachments/index.tsx (2)
7-94: Consider keyboard navigation for attachment cards.The
AttachmentCardcomponent renders clickable elements (remove button, link) but the card itself is not keyboard-navigable. For better accessibility, consider making the entire card focusable when it represents an interactive element (e.g., viewable attachment).For example, wrap non-link attachments in a button or add
tabIndex={0}with appropriate keyboard handlers if cards should be selectable or viewable:<button type="button" onClick={() => onView?.(attachment.id)} className="flex flex-col gap-3 p-4 border border-neutral-200 rounded-lg bg-white hover:bg-neutral-50 transition-colors relative text-left w-full" > {/* existing content */} </button>
115-115: Improve empty state message for accessibility.The message "Use the + icon above to add one" assumes visual context and may not be helpful for screen reader users or if the UI changes. Consider a more descriptive message.
-<p>No attachments yet. Use the + icon above to add one.</p> +<p>No attachments yet. Add files using the attachment button.</p>apps/desktop/src/store/zustand/ai-task/task-configs/enhance-workflow.ts (1)
17-17: Attachment-aware prompt construction looks good; avoidArray<any>for messagesThe new
attachmentPartsresolution and inclusion in the single user message content correctly reusesresolveSessionAttachmentsand ensures only supported attachments reach the model. The overall control flow and abort handling remain intact.To keep type-safety, consider replacing
Array<any>formessageswith a concrete type (e.g., a small localtype EnhanceMessage = { role: "user"; content: typeof attachmentParts[number][] | ... }) so future changes to attachment parts or content shape are checked by TypeScript instead of being hidden behindany.Also applies to: 162-166, 186-200
apps/desktop/src/components/chat/message/normal.tsx (1)
152-214: Factor out shared attachment UI and harden link attributes
FileAttachmentandChatFileAttachmentare almost identical aside from accessingpartvspart.data. You could reduce duplication by extracting a smallAttachmentViewthat takes{ url, mediaType, filename }, and have both components delegate to it.Also, since links open in a new tab, consider using
rel="noreferrer noopener"to future‑proof againstwindow.openerissues if non‑data/blob URLs are ever introduced.apps/desktop/src/store/zustand/ai-task/task-configs/enhance-transform.ts (1)
42-61: Align attachment typing with shared resolver to avoid driftThe new
attachmentsplumbing intransformArgslooks consistent with howresolveSessionAttachmentsexpectsAttachmentReference(id, fileName, mimeType, size, fileUrl), and the failure‑to‑load behavior (log +[]) is a good safety net.The one concern is that this attachment shape is now effectively duplicated here via the inline mapping in
getSessionAttachments, whileAttachmentReferenceis separately defined inshared/resolve-attachments.ts. If either side adds/removes fields later, it’s easy for them to go out of sync without a type error.Consider exporting a shared
AttachmentReferencetype from a common module (or fromshared/resolve-attachments.ts) and reusing it here so the compiler enforces schema consistency between storage, transform, and resolver.Also applies to: 384-398
apps/desktop/src/chat/resolve-attachments.ts (1)
6-52: Clarify contract whenchatGroupIdis missing to avoid leakingdata-chat-filepartsThe transformation from
data-chat-file→file(viareadChatAttachmentAsDataURL) and the passthrough ofblob:file parts both look reasonable.However, when
chatGroupIdis falsy you currently:if (part.type === "data-chat-file") { if (!chatGroupId) { return part; // remains a non-standard part type } // ... }If
resolveChatFileReferencesis ever called without achatGroupIdwhilemessagesstill containdata-chat-fileparts, those parts will flow downstream unchanged, which may not be understood by consumers expecting only the standard UI/FileUIPartvariants.To make this behavior more explicit/robust, consider either:
- Short‑circuiting at the top: when
!chatGroupId, justreturn messages;(if the invariant is that this helper is only meaningfully used when a group id exists), and/or- Emitting a warning if
!chatGroupIdbut adata-chat-filepart is encountered, so it’s easier to detect incorrect call sites.This keeps runtime behavior the same today but guards against future misuse of the helper.
apps/desktop/src/components/chat/view.tsx (1)
71-116: Make theparts↔attachmentsalignment inensurePersistedAttachmentPartsexplicitThe overall send pipeline looks good:
- New messages get a group id and title via
deriveChatTitle.ensurePersistedAttachmentPartspersists any non‑persisted attachments and rewrites correspondingfileparts intodata-chat-filereferences.- The same normalized
partsandmetadataare stored in Tinybase and sent asHyprUIMessage.One subtle point is in how
ensurePersistedAttachmentPartsassociates saved attachments back toparts:const newlySaved: PersistedChatAttachment[] = []; for (const attachment of attachments) { if (attachment.persisted) continue; const saved = await saveChatAttachment(chatGroupId, attachment.file); newlySaved.push(saved); } let savedIndex = 0; return parts.map((part) => { if (part.type !== "file") return part; const saved = newlySaved[savedIndex++]; if (!saved) return part; return { type: "data-chat-file", data: { /* from saved */ }, }; });This implicitly assumes:
- Every
filepart inpartsthat needs persistence corresponds to one of the non‑persistedattachments, and- Their relative order matches, so incrementing
savedIndexlinearly is correct.If
partsever contains additionalfileentries that aren’t represented inattachments(or if the ordering diverges), the tailfileparts will remain as rawfileparts (often withblob:URLs) and won’t go through the storage/data-chat-filepath, which can be surprising.To harden this, consider:
- Documenting this 1:1 ordering contract where
attachmentsis constructed, and/or- Passing an explicit identifier from
attachmentsintoparts(or vice versa) and matching by that id instead of by position.That would make the persistence mapping more robust to future changes in how
partsandattachmentsare built.Also applies to: 187-240
apps/desktop/src/components/main/body/sessions/note-input/index.tsx (3)
318-463: Attachment ↔ editor sync and pruning logic is careful and well‑structuredThe combination of:
collectAttachmentRefsto find referenced ids/srcs in the current document,collectReferencesFromAllViewsto merge in references from raw + all enhanced notes,findOrphanedAttachmentsto filter only persisted, non‑pending, truly unreferenced attachments, andsyncAttachmentsWithEditorto (a) backfill missingattachmentIds based onsrcand (b) remove image nodes without a backing attachment,gives a nice, conservative attachment‑cleanup story. Notably:
- Skipping prune when any
data:URLs are referenced avoids touching content while in‑flight inline data URLs are present.- Only pruning attachments that are persisted and not in
pendingAttachmentSavesguards against races with saves.- Cross‑view reference collection prevents deleting attachments just because they’re not present in the currently edited view.
Given how subtle this logic is, it would be worth having a few targeted tests (or at least manual scenarios) that cover:
- Deleting attachments referenced only in enhanced notes,
- Session switching during save,
- Inline copy/paste images that never become persisted attachments.
But from a code standpoint, the flow is coherent and defensive.
Also applies to: 524-575, 623-703, 864-898, 901-995
187-306: Session attachment add/save/cancel pipeline looks robust; consider minor cleanups onlyThe
useSessionAttachments+ NoteInput wiring for adding and persisting attachments is quite thorough:
handleFilesAdded:
- Filters to
image/*.- Deduplicates identical files via a signature (
name-size-lastModified-type).- Creates optimistic attachments with object URLs and tracks them in
createdAttachmentUrls.- Inserts corresponding image nodes into the editor (when available), with the first file honoring the provided insertion position.
- Async saving:
- Uses
pendingAttachmentSavesto track in‑flight saves, including acancelledflag.- On success, updates the optimistic attachment in place with persisted metadata (
mapPersistedAttachment), clears the object URL, and updates the editor nodesrcto the persistedfileUrl.- On session change or cancellation, deletes the saved attachment from storage, removes nodes from the editor, and cleans up state.
- On error, cleans up object URLs, nodes, and attachment state.
- Teardown:
cleanupObjectUrlsin the effect cleanup ensures any leaked object URLs are revoked.This is a solid pattern for optimistic UI + filesystem persistence.
Non‑blocking possible tweaks:
- Consider gating some of the console logging (especially the verbose
[attachments]logs inside hot paths) behind a debug flag if logs become noisy in production.- If you expect a lot of attachments per session, you could eventually clear old entries from
pendingAttachmentSavesmore aggressively, but with current usage it’s unlikely to matter.Overall, the lifecycle looks correct and resilient to races (removal during save, session changes, etc.).
Also applies to: 531-575, 705-856
107-125: Editor mounting and tab behavior for attachments integrates cleanlyThe additions around:
- Forcing a fallback away from the
attachmentstab whenshouldShowAttachmentsTabbecomes false,useMountedEditorView/EditorViewContainerto keep a single raw/enhanced editor mounted while switching tabs, and- The new
alt+ahotkey inuseTabShortcuts,all fit well with the existing tab model:
- The editor instance is reused across raw/enhanced tab changes, and file handling is only enabled when a raw editor is active (
data-allow-file-dropflag).- Attachments tab is purely a viewer; clicks on the outer container only focus the editor when the user is on an editable tab.
- Mounting logic gracefully falls back from an enhanced editor to raw if the enhanced note disappears.
No functional issues stand out here; just keep in mind that any future editor types that can host attachments should probably be wired into
EditorViewContainerand the file‑drop gating in the same way.Also applies to: 132-161, 187-275, 500-518, 1070-1085, 1087-1226
apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts (1)
31-42: Simplify title default expression insaveSessionAttachment
{ title: file.name || file.name || "attachment" }is redundant; the secondfile.namenever changes the result.You can simplify without changing behavior:
- { title: file.name || file.name || "attachment" }, + { title: file.name || "attachment" },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
apps/desktop/package.json(1 hunks)apps/desktop/src-tauri/capabilities/default.json(1 hunks)apps/desktop/src/chat/resolve-attachments.ts(1 hunks)apps/desktop/src/chat/transport.ts(2 hunks)apps/desktop/src/chat/types.ts(1 hunks)apps/desktop/src/components/chat/attachments/storage.ts(1 hunks)apps/desktop/src/components/chat/input.tsx(5 hunks)apps/desktop/src/components/chat/message/normal.tsx(3 hunks)apps/desktop/src/components/chat/message/types.ts(1 hunks)apps/desktop/src/components/chat/session.tsx(3 hunks)apps/desktop/src/components/chat/view.tsx(5 hunks)apps/desktop/src/components/main/body/sessions/note-input/attachments/index.tsx(1 hunks)apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts(1 hunks)apps/desktop/src/components/main/body/sessions/note-input/enhanced/editor.tsx(3 hunks)apps/desktop/src/components/main/body/sessions/note-input/enhanced/index.tsx(3 hunks)apps/desktop/src/components/main/body/sessions/note-input/header.tsx(8 hunks)apps/desktop/src/components/main/body/sessions/note-input/index.tsx(4 hunks)apps/desktop/src/components/main/body/sessions/note-input/raw.tsx(5 hunks)apps/desktop/src/shared/attachments/constants.ts(1 hunks)apps/desktop/src/shared/attachments/storage.ts(1 hunks)apps/desktop/src/store/zustand/ai-task/shared/resolve-attachments.ts(1 hunks)apps/desktop/src/store/zustand/ai-task/task-configs/enhance-transform.ts(4 hunks)apps/desktop/src/store/zustand/ai-task/task-configs/enhance-workflow.ts(3 hunks)apps/desktop/src/store/zustand/ai-task/task-configs/index.ts(1 hunks)apps/desktop/src/store/zustand/tabs/schema.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
apps/desktop/src/components/chat/message/types.ts (2)
apps/desktop/src/chat/types.ts (1)
ChatDataParts(19-21)apps/desktop/src/chat/tools.ts (1)
Tools(45-58)
apps/desktop/src/components/chat/session.tsx (1)
apps/desktop/src/chat/transport.ts (1)
CustomChatTransport(13-65)
apps/desktop/src/components/main/body/sessions/note-input/attachments/index.tsx (1)
apps/desktop/src/components/main/body/sessions/note-input/index.tsx (1)
Attachment(27-39)
apps/desktop/src/chat/transport.ts (2)
apps/desktop/src/chat/types.ts (1)
HyprUIMessage(23-23)apps/desktop/src/chat/resolve-attachments.ts (1)
resolveChatFileReferences(6-52)
apps/desktop/src/chat/resolve-attachments.ts (2)
apps/desktop/src/chat/types.ts (1)
HyprUIMessage(23-23)apps/desktop/src/components/chat/attachments/storage.ts (1)
readChatAttachmentAsDataURL(47-52)
apps/desktop/src/store/zustand/ai-task/shared/resolve-attachments.ts (1)
apps/desktop/src/shared/attachments/constants.ts (1)
ATTACHMENT_SIZE_LIMIT(1-1)
apps/desktop/src/store/zustand/ai-task/task-configs/enhance-workflow.ts (1)
apps/desktop/src/store/zustand/ai-task/shared/resolve-attachments.ts (1)
resolveSessionAttachments(15-65)
apps/desktop/src/store/zustand/ai-task/task-configs/enhance-transform.ts (1)
apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts (1)
loadSessionAttachments(25-29)
apps/desktop/src/components/main/body/sessions/note-input/enhanced/index.tsx (1)
packages/tiptap/src/editor/index.tsx (1)
JSONContent(17-17)
apps/desktop/src/components/main/body/sessions/note-input/index.tsx (6)
apps/desktop/src/components/main/body/sessions/note-input/header.tsx (1)
useEditorTabs(456-492)apps/desktop/src/components/main/body/sessions/note-input/attachments/index.tsx (1)
Attachments(96-131)apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts (4)
loadSessionAttachments(25-29)removeSessionAttachment(44-49)saveSessionAttachment(31-42)PersistedAttachment(8-12)packages/tiptap/src/shared/utils.ts (2)
isValidTiptapContent(10-17)EMPTY_TIPTAP_DOC(8-8)apps/desktop/src/components/main/body/sessions/note-input/raw.tsx (1)
RawEditor(16-108)apps/desktop/src/components/main/body/sessions/note-input/enhanced/index.tsx (1)
Enhanced(12-50)
apps/desktop/src/components/chat/view.tsx (2)
apps/desktop/src/chat/types.ts (1)
HyprUIMessage(23-23)apps/desktop/src/components/chat/attachments/storage.ts (2)
PersistedChatAttachment(8-11)saveChatAttachment(28-34)
apps/desktop/src/components/chat/attachments/storage.ts (2)
apps/desktop/src/shared/attachments/storage.ts (2)
ManifestEntry(15-22)createAttachmentStorage(99-396)apps/desktop/src/shared/attachments/constants.ts (1)
ATTACHMENT_SIZE_LIMIT(1-1)
apps/desktop/src/shared/attachments/storage.ts (2)
apps/desktop/src/components/chat/attachments/storage.ts (1)
ManifestCorruptionError(13-13)apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts (1)
ManifestCorruptionError(14-14)
apps/desktop/src/components/main/body/sessions/note-input/enhanced/editor.tsx (1)
packages/tiptap/src/editor/index.tsx (1)
JSONContent(17-17)
apps/desktop/src/components/main/body/sessions/note-input/attachments/storage.ts (2)
apps/desktop/src/shared/attachments/storage.ts (2)
ManifestEntry(15-22)createAttachmentStorage(99-396)apps/desktop/src/shared/attachments/constants.ts (1)
ATTACHMENT_SIZE_LIMIT(1-1)
apps/desktop/src/components/chat/input.tsx (4)
apps/desktop/src/chat/types.ts (1)
HyprUIMessage(23-23)apps/desktop/src/components/chat/attachments/storage.ts (3)
PersistedChatAttachment(8-11)saveChatAttachment(28-34)removeChatAttachment(36-41)apps/desktop/src/hooks/useCurrentModelModalitySupport.ts (1)
useCurrentModelModalitySupport(21-80)apps/desktop/src/shared/attachments/constants.ts (1)
ATTACHMENT_SIZE_LIMIT(1-1)
apps/desktop/src/components/chat/message/normal.tsx (1)
apps/desktop/src/components/chat/message/types.ts (1)
Part(7-7)
apps/desktop/src/components/main/body/sessions/note-input/raw.tsx (1)
packages/tiptap/src/shared/extensions/index.ts (1)
FileHandlerConfig(20-23)
apps/desktop/src/components/main/body/sessions/note-input/header.tsx (2)
apps/desktop/src/store/zustand/tabs/schema.ts (1)
EditorView(19-19)apps/desktop/src/contexts/listener.tsx (1)
useListener(37-49)
🪛 Biome (2.1.2)
apps/desktop/src/shared/attachments/storage.ts
[error] 199-199: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 199-199: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (macos, macos-14)
- GitHub Check: fmt
🔇 Additional comments (21)
apps/desktop/src-tauri/capabilities/default.json (1)
72-77: Capability addition is appropriate and well-scoped.The new
fs:allow-removepermission properly enables file deletion for the attachment system while maintaining security boundaries. The path scope$DATA/hyprnote/**/*is consistent with other filesystem operations already permitted in this configuration (read, write, mkdir), and the format matches the existing pattern for fs:allow-* capabilities.apps/desktop/src/components/main/body/sessions/note-input/raw.tsx (5)
1-12: LGTM! Clean import additions.The new imports (
useCallbackandFileHandlerConfig) are properly organized and directly support the file handling functionality being added.
16-23: Well-structured prop extension.The component signature cleanly extends the API to support file attachments and content change notifications while maintaining backward compatibility through optional callbacks.
49-63: Excellent callback implementation.The
handleChangeproperly orchestrates content persistence and change notification with correct memoization and dependencies. The ordering (persist first, then notify) is sensible.
75-95: Solid file handler implementation.The
fileHandlerConfigcorrectly implements both drop and paste handlers:
onDropforwards the provided position parameteronPastederives the cursor position fromeditor.state.selection.from(standard TipTap/ProseMirror API)- Returning
falseprevents default file handling behavior- The
satisfiesoperator ensures type safety
97-107: Clean integration with NoteEditor.The
fileHandlerConfigis properly passed through, enabling file handling when the callback is provided while maintaining backward compatibility when it's not.apps/desktop/src/shared/attachments/constants.ts (1)
1-1: LGTM!The 10 MB attachment size limit is clearly defined and appropriately scoped for use across the attachment system.
apps/desktop/src/components/main/body/sessions/note-input/enhanced/index.tsx (2)
3-3: LGTM!The
JSONContentimport is correctly added to support the new callback prop type.
14-18: LGTM!The
onContentChangecallback is cleanly integrated as an optional prop and properly forwarded to theEnhancedEditor. The implementation is backward compatible and type-safe.Also applies to: 47-47
apps/desktop/src/components/main/body/sessions/note-input/enhanced/editor.tsx (1)
1-1: LGTM!The refactoring properly separates content persistence (
saveContent) from the change handler and usesuseCallbackwith correct dependencies. The optionalonContentChangecallback is safely invoked only when provided.Also applies to: 41-55
apps/desktop/src/components/main/body/sessions/note-input/attachments/index.tsx (1)
133-139: LGTM!The
formatAttachmentTimestamphelper properly handles invalid dates by returning the original value, preventing runtime errors.apps/desktop/src/store/zustand/tabs/schema.ts (1)
13-13: LGTM!The new "attachments" view type and corresponding type guard follow the established patterns for editor view variants. The implementation is consistent and type-safe.
Also applies to: 29-31
apps/desktop/src/chat/types.ts (1)
10-23: LGTM!The new
ChatFileReferencePartandChatDataPartstypes cleanly extend the AI SDK'sUIMessagewith custom data parts for attachment references. The type structure aligns well with the attachment system introduced in this PR.apps/desktop/src/store/zustand/ai-task/task-configs/index.ts (1)
47-53: LGTM!The
attachmentsarray addition to the enhance task arguments is well-typed with all necessary metadata fields (id, fileName, mimeType, size, fileUrl).Note: The field is named
fileNamehere whileChatFileReferencePartinapps/desktop/src/chat/types.tsusesfilename. Ensure this naming difference is intentional based on the different contexts (task config vs. chat message parts).apps/desktop/package.json (1)
76-76: Dependency version verified and no security vulnerabilities found.The
async-mutexpackage version 0.5.0 exists in the npm registry and has no known security vulnerabilities. The dependency addition is appropriate for the concurrency control requirements.apps/desktop/src/components/chat/message/types.ts (1)
5-7: Part typing updated correctly to support data partsUsing
UIMessagePart<ChatDataParts, Tools>wires in the newdata-chat-fileparts cleanly while preserving tool-part typing; this looks consistent with the rest of the chat types.apps/desktop/src/components/chat/session.tsx (1)
25-31: chatGroupId propagation into transport is consistent and hook-safePassing
chatGroupIdthroughuseTransportintoCustomChatTransportand memoizing on[registry, model, chatGroupId]keeps the hook order stable and correctly scopes the transport per chat group.Also applies to: 162-173
apps/desktop/src/components/chat/message/normal.tsx (1)
66-83: New branches for file/data-chat-file parts match the updated Part unionRouting
"file"parts toFileAttachmentand"data-chat-file"parts toChatFileAttachmentaligns with theParttyping and keeps existing reasoning/text/tool handling untouched.apps/desktop/src/components/chat/view.tsx (1)
242-261: Chat title derivation behavior looks reasonableThe
deriveChatTitlehelper’s precedence (trimmedContent→ first attachment filename →"New chat") and 50‑character ellipsis cutoff provide a clear, predictable title without overcomplicating things. This should work well both for text‑only chats and file‑only chats.apps/desktop/src/store/zustand/ai-task/shared/resolve-attachments.ts (1)
21-65: Fix base64 conversion to handle large files safely—use modern native API or FileReader fallbackThe bug is confirmed. Line 88 in
readAttachmentAsDataURLusesbtoa(String.fromCharCode(...new Uint8Array(fileData))), which crashes when spreading large Uint8Arrays into function arguments (JavaScript engines limit to ~65k arguments; 10MB file = ~10M arguments).The @tauri-apps/plugin-fs v2
readFilereturns a Promise, and the current code is unsafe for the configured 10MB limit.Rather than chunking
String.fromCharCode, use the modern native approach or a robust fallback:try { const fileData = await readFile(relativePath, { baseDir: BaseDirectory.Data, }); - const base64 = btoa(String.fromCharCode(...new Uint8Array(fileData))); - + // Use native Uint8Array.toBase64() if available (Stage 4, Sept 2025+) + // Otherwise fall back to Blob + FileReader for broader compatibility + let base64: string; + if (typeof (new Uint8Array()).toBase64 === 'function') { + base64 = new Uint8Array(fileData).toBase64(); + } else { + const blob = new Blob([fileData], { type: mimeType }); + base64 = await new Promise<string>((resolve, reject) => { + const reader = new FileReader(); + reader.onload = () => { + const result = reader.result as string; + resolve(result.split(',')[1]); // extract base64 part + }; + reader.onerror = reject; + reader.readAsDataURL(blob); + }); + } + return `data:${mimeType};base64,${base64}`; } catch (error) {This avoids argument-limit failures and uses the fastest available method. Modern Uint8Array.toBase64() is fastest; FileReader is a robust alternative.
Likely an incorrect or invalid review comment.
apps/desktop/src/components/chat/attachments/storage.ts (1)
8-52: Chat attachment storage wrapper looks consistentThe typed wrapper cleanly mirrors the shared storage API, and
PersistedChatAttachmentcorrectly matchesManifestEntry & { filePath; fileUrl }. Config (base path, size limit, no title) is appropriate for chat usage.
| private chatGroupId?: string, | ||
| ) {} |
There was a problem hiding this comment.
Guard against attachment resolution failures so sends still succeed
Right now, if resolveChatFileReferences throws (e.g., readChatAttachmentAsDataURL fails), sendMessages will reject and the whole send will fail, even though we could safely fall back to the original messages without resolved files.
To keep chat more robust (and consistent with the more defensive session attachment resolver), consider wrapping the resolver in a try/catch and defaulting back to options.messages on error:
- const resolvedMessages = await resolveChatFileReferences(
- options.messages,
- this.chatGroupId,
- );
+ let resolvedMessages = options.messages;
+ try {
+ resolvedMessages = await resolveChatFileReferences(
+ options.messages,
+ this.chatGroupId,
+ );
+ } catch (error) {
+ console.error(
+ "[chat] failed to resolve attachments; falling back to original messages",
+ error,
+ );
+ }This preserves the new attachment support while avoiding a hard failure path when resolution misbehaves.
Also applies to: 25-29, 43-45
🤖 Prompt for AI Agents
In apps/desktop/src/chat/transport.ts around lines 17-18 (also apply same change
at 25-29 and 43-45): the calls that run resolveChatFileReferences can throw and
currently bubble up, causing sendMessages to fail; wrap each
resolveChatFileReferences invocation in a try/catch, on catch log or warn the
error, and fall back to using the original options.messages (or the previous
messages variable) so the send proceeds without resolved attachments; ensure
types remain compatible when falling back and avoid swallowing unexpected errors
(only suppress attachment-resolution failures).
| const handleFileChange = useCallback( | ||
| async (event: ChangeEvent<HTMLInputElement>) => { | ||
| const files = event.target.files; | ||
| if (files && files.length > 0) { | ||
| const validFiles: Array<{ | ||
| file: File; | ||
| persisted?: PersistedChatAttachment; | ||
| }> = []; | ||
| let errorMessage: string | null = null; | ||
|
|
||
| for (const file of Array.from(files)) { | ||
| if (file.size > ATTACHMENT_SIZE_LIMIT) { | ||
| errorMessage = `File "${file.name}" exceeds ${Math.round(ATTACHMENT_SIZE_LIMIT / 1024 / 1024)}MB limit`; | ||
| break; | ||
| } | ||
|
|
||
| if (chatGroupId) { | ||
| try { | ||
| const persisted = await saveChatAttachment(chatGroupId, file); | ||
| validFiles.push({ file, persisted }); | ||
| } catch (error) { | ||
| errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| break; | ||
| } | ||
| } else { | ||
| validFiles.push({ file }); | ||
| } | ||
| } | ||
|
|
||
| if (errorMessage) { | ||
| setAttachmentError(errorMessage); | ||
| } else { | ||
| setAttachmentError(null); | ||
| setAttachedFiles((prev) => [...prev, ...validFiles]); | ||
| } | ||
|
|
||
| if (fileInputRef.current) { | ||
| fileInputRef.current.value = ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid leaving persisted attachments without UI references on partial failures
In handleFileChange, if some files are successfully persisted (pushed into validFiles) and a later file triggers errorMessage (size or saveChatAttachment error), you break the loop and then skip setAttachedFiles entirely in the errorMessage branch. That leaves already‑persisted attachments on disk and in the manifest but not in attachedFiles, so the user can’t see or clean them up via the UI.
You can keep the current “stop on first error” behavior but always add successfully processed files to state, even when an error occurred:
- if (errorMessage) {
- setAttachmentError(errorMessage);
- } else {
- setAttachmentError(null);
- setAttachedFiles((prev) => [...prev, ...validFiles]);
- }
+ if (errorMessage) {
+ setAttachmentError(errorMessage);
+ } else {
+ setAttachmentError(null);
+ }
+
+ if (validFiles.length > 0) {
+ setAttachedFiles((prev) => [...prev, ...validFiles]);
+ }This keeps the UX (surface the error) while ensuring all successfully saved attachments are tracked in component state.
🤖 Prompt for AI Agents
In apps/desktop/src/components/chat/input.tsx around lines 165-205, the handler
stops on first error and only updates attachedFiles when there was no error,
which can leave already-persisted attachments orphaned; change the logic so
after the for-loop (regardless of whether errorMessage was set) you always
append the validFiles to state via setAttachedFiles(prev => [...prev,
...validFiles]), then setAttachmentError if errorMessage exists (or null if
not), and finally clear fileInputRef; keep the “stop on first error” behavior
inside the loop but ensure persisted entries are always added to component state
even when a later file fails.
| function HeaderTabAttachments({ | ||
| isActive, | ||
| onClick = () => {}, | ||
| onUploadClick, | ||
| }: { | ||
| isActive: boolean; | ||
| onClick?: () => void; | ||
| onUploadClick?: () => void; | ||
| }) { | ||
| const handleUploadClick = useCallback( | ||
| (event: React.MouseEvent) => { | ||
| event.stopPropagation(); | ||
| onUploadClick?.(); | ||
| }, | ||
| [onUploadClick], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Import React for namespace types or switch to named event types
HeaderTabAttachments and handleAttachmentInputChange use React.MouseEvent / React.ChangeEvent, but this file only imports hooks from "react" and doesn’t bring the React namespace into scope. In TS with the modern JSX runtime, JSX itself doesn’t require React, but these type references still do, so this will fail type‑checking.
Two straightforward fixes:
- Add a type‑only React import and keep the current annotations:
-import { useCallback, useEffect, useRef, useState } from "react";
+import { useCallback, useEffect, useRef, useState } from "react";
+import type React from "react";- Or, switch to named event types (no
Reactnamespace needed):
- const handleUploadClick = useCallback(
- (event: React.MouseEvent) => {
+ const handleUploadClick = useCallback(
+ (event: MouseEvent) => {
@@
- const handleAttachmentInputChange = useCallback(
- (event: React.ChangeEvent<HTMLInputElement>) => {
+ const handleAttachmentInputChange = useCallback(
+ (event: ChangeEvent<HTMLInputElement>) => {and update the top import accordingly:
-import { useCallback, useEffect, useRef, useState } from "react";
+import {
+ type ChangeEvent,
+ type MouseEvent,
+ useCallback,
+ useEffect,
+ useRef,
+ useState,
+} from "react";Either approach will restore type safety and compile cleanly.
Also applies to: 362-372
I'll verify this review comment by inspecting the actual file and checking the imports and code usage.
🏁 Script executed:
#!/bin/bash
# Find and read the header.tsx file to check imports and target lines
find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1Length of output: 70
🏁 Script executed:
#!/bin/bash
# Read the file to examine imports and relevant code sections
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
if [ -n "$file_path" ]; then
echo "=== TOP 40 LINES (IMPORTS) ==="
head -40 "$file_path"
echo ""
echo "=== LINES 60-85 ==="
sed -n '60,85p' "$file_path"
echo ""
echo "=== LINES 355-375 ==="
sed -n '355,375p' "$file_path"
else
echo "File not found"
fiLength of output: 2526
🏁 Script executed:
#!/bin/bash
# Search for all React.MouseEvent and React.ChangeEvent usages in the file
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
if [ -n "$file_path" ]; then
echo "=== React.MouseEvent usages ==="
rg -n "React\.MouseEvent" "$file_path"
echo ""
echo "=== React.ChangeEvent usages ==="
rg -n "React\.ChangeEvent" "$file_path"
fiLength of output: 196
🏁 Script executed:
#!/bin/bash
# Search for React import statements in the file
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
if [ -n "$file_path" ]; then
echo "=== All import statements ==="
rg -n "^import" "$file_path"
fiLength of output: 931
🏁 Script executed:
#!/bin/bash
# Verify if React is imported as namespace or value
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
if [ -n "$file_path" ]; then
echo "=== React namespace or value imports ==="
rg -n "import.*React" "$file_path" | head -20
fiLength of output: 40
🏁 Script executed:
#!/bin/bash
# Check tsconfig.json jsx setting to understand how React types are resolved
find . -name "tsconfig.json" -o -name "tsconfig.*.json" | xargs -I {} sh -c 'echo "=== {} ===" && jq -r ".compilerOptions | {jsx, jsxImportSource}" {} 2>/dev/null || echo "Could not parse"' 2>/dev/null | head -50Length of output: 932
🏁 Script executed:
#!/bin/bash
# Check package.json React version
if [ -f "package.json" ]; then
echo "=== React version in package.json ==="
jq -r '.dependencies.react // .devDependencies.react // "not found"' package.json
fiLength of output: 47
🏁 Script executed:
#!/bin/bash
# Double-check the exact content around the function definition
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
if [ -n "$file_path" ]; then
echo "=== Complete context of HeaderTabAttachments (lines 60-100) ==="
sed -n '60,100p' "$file_path"
fiLength of output: 994
🏁 Script executed:
#!/bin/bash
# Read the desktop tsconfig.json directly to check jsx setting
cat ./apps/desktop/tsconfig.jsonLength of output: 604
🏁 Script executed:
#!/bin/bash
# Search for all React.* type usages to get full scope
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
rg -n "React\.[A-Z]" "$file_path"Length of output: 195
🏁 Script executed:
#!/bin/bash
# Verify if TypeScript would actually fail—check if this file has tsconfig refs
file_path=$(find . -type f -name "header.tsx" -path "*/sessions/note-input/*" | head -1)
dir=$(dirname "$file_path")
# Walk up and find the nearest tsconfig or package.json
while [ "$dir" != "." ] && [ "$dir" != "/" ]; do
if [ -f "$dir/tsconfig.json" ]; then
echo "Found tsconfig.json at: $dir/tsconfig.json"
head -20 "$dir/tsconfig.json"
break
fi
dir=$(dirname "$dir")
doneLength of output: 536
🏁 Script executed:
#!/bin/bash
# Check if there are any type or value React imports elsewhere in related files
find ./apps/desktop/src/components/main/body/sessions -name "*.tsx" -o -name "*.ts" | xargs rg "^import.*React" | head -20Length of output: 367
Import React for namespace types or switch to named event types
The file uses React.MouseEvent, React.ChangeEvent, and React.ReactNode without importing the React namespace. With jsx: "react-jsx" and strict: true, TypeScript compilation will fail.
Instances found:
- Line 44:
React.ReactNode - Line 75:
React.MouseEvent(inHeaderTabAttachments) - Line 136:
React.MouseEvent(additional handler) - Line 363:
React.ChangeEvent<HTMLInputElement>(inhandleAttachmentInputChange) - Line 604:
React.ReactNode
Two straightforward fixes:
- Add a type‑only React import:
-import { useCallback, useEffect, useRef, useState } from "react";
+import type React from "react";
+import { useCallback, useEffect, useRef, useState } from "react";- Or, switch to named imports (e.g.,
MouseEvent,ChangeEvent,ReactNode):
-import { useCallback, useEffect, useRef, useState } from "react";
+import {
+ type ChangeEvent,
+ type MouseEvent,
+ type ReactNode,
+ useCallback,
+ useEffect,
+ useRef,
+ useState,
+} from "react";Then replace all React.* type usages accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/components/main/body/sessions/note-input/header.tsx (see
occurrences around lines 44, 65–80, 136, 363, 604), the code uses React
namespace types (React.ReactNode, React.MouseEvent, React.ChangeEvent) but does
not import React, which breaks TypeScript under jsx: "react-jsx" and strict
mode; fix by adding a type-only import from 'react' (e.g., import type React
from "react";) or import the specific types as named type imports (e.g., import
type { MouseEvent, ChangeEvent, ReactNode } from "react";) and then update the
type references accordingly (either keep React.* or switch to the named types),
ensuring no runtime import is added.
| function sanitizeFileName(name: string) { | ||
| const sanitized = name | ||
| .replace(/[<>:"/\\|?*\u0000-\u001F]/g, "_") | ||
| .slice(0, 255); | ||
| return sanitized.length > 0 ? sanitized : "attachment"; | ||
| } |
There was a problem hiding this comment.
Address lint error for control characters in filename sanitization
The regex in sanitizeFileName intentionally strips ASCII control characters via \u0000-\u001F, but Biome flags this as a control‑character regex error. To avoid the lint error while keeping behavior, you can move the control‑character check out of the regex:
- function sanitizeFileName(name: string) {
- const sanitized = name
- .replace(/[<>:"/\\|?*\u0000-\u001F]/g, "_")
- .slice(0, 255);
- return sanitized.length > 0 ? sanitized : "attachment";
- }
+ function sanitizeFileName(name: string) {
+ const sanitized = Array.from(name)
+ .map((char) => {
+ const code = char.charCodeAt(0);
+ if (code < 32 || /[<>:"/\\|?*]/.test(char)) {
+ return "_";
+ }
+ return char;
+ })
+ .join("")
+ .slice(0, 255);
+ return sanitized.length > 0 ? sanitized : "attachment";
+ }This preserves the “strip invalid filesystem and control chars” behavior but should satisfy the linter.
🧰 Tools
🪛 Biome (2.1.2)
[error] 199-199: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 199-199: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI Agents
In apps/desktop/src/shared/attachments/storage.ts around lines 197 to 202, the
regex currently includes the control-character range \u0000-\u001F which
triggers a linter error; remove that range from the regex used to strip
filesystem-invalid characters, then perform a separate pass to remove control
characters (e.g., filter characters by codePoint <= 0x1F or use a replace with a
predicate) either before or after the regex replacement, preserve the .slice(0,
255) truncation and the fallback to "attachment" when the result is empty, and
ensure the final output remains the same while avoiding control-character ranges
in the regex.
| async function removeAll(id: string) { | ||
| const baseDir = BASE_DIR(id); | ||
| const dirExists = await exists(baseDir, { | ||
| baseDir: BaseDirectory.Data, | ||
| }); | ||
|
|
||
| if (dirExists) { | ||
| await removeFile(baseDir, { | ||
| baseDir: BaseDirectory.Data, | ||
| recursive: true, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard removeAll with the same mutex used for other manifest operations
removeAll currently bypasses the per‑id mutex, while load, save, remove, and readAsDataURL all run inside mutex.runExclusive. That means removeAll can race with in‑flight operations on the same id, potentially deleting directories while another call is reading or writing the manifest/files.
Consider wrapping removeAll in the same mutex pattern:
- async function removeAll(id: string) {
- const baseDir = BASE_DIR(id);
- const dirExists = await exists(baseDir, {
- baseDir: BaseDirectory.Data,
- });
-
- if (dirExists) {
- await removeFile(baseDir, {
- baseDir: BaseDirectory.Data,
- recursive: true,
- });
- }
- }
+ async function removeAll(id: string) {
+ const mutex = getManifestMutex(id);
+ return await mutex.runExclusive(async () => {
+ acquireMutex(id);
+ try {
+ const baseDir = BASE_DIR(id);
+ const dirExists = await exists(baseDir, {
+ baseDir: BaseDirectory.Data,
+ });
+
+ if (dirExists) {
+ await removeFile(baseDir, {
+ baseDir: BaseDirectory.Data,
+ recursive: true,
+ });
+ }
+ } finally {
+ releaseMutex(id);
+ }
+ });
+ }This keeps all per‑id filesystem changes serialized.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/shared/attachments/storage.ts around lines 334 to 346,
removeAll is not using the per-id mutex and can race with
load/save/remove/readAsDataURL; fix by running the existing per-id mutex for
this id (the same mutex used by other manifest ops) and move the current body
into a mutex.runExclusive(async () => { ... }) call so the exists/removeFile
calls are executed under the lock, preserving the same error propagation/return
behavior.
| async function readAsDataURL( | ||
| id: string, | ||
| attachmentId: string, | ||
| ): Promise<string | null> { | ||
| const mutex = getManifestMutex(id); | ||
| return await mutex.runExclusive(async () => { | ||
| acquireMutex(id); | ||
| try { | ||
| const manifest = await readManifest(id); | ||
| const entry = manifest[attachmentId]; | ||
|
|
||
| if (!entry) { | ||
| return null; | ||
| } | ||
|
|
||
| const relativePath = ATTACHMENT_FILE_PATH( | ||
| id, | ||
| attachmentId, | ||
| entry.fileName, | ||
| ); | ||
| const existsOnDisk = await exists(relativePath, { | ||
| baseDir: BaseDirectory.Data, | ||
| }); | ||
|
|
||
| if (!existsOnDisk) { | ||
| return null; | ||
| } | ||
|
|
||
| const fileData = await readFile(relativePath, { | ||
| baseDir: BaseDirectory.Data, | ||
| }); | ||
|
|
||
| const base64 = btoa(String.fromCharCode(...new Uint8Array(fileData))); | ||
|
|
||
| return `data:${entry.mimeType};base64,${base64}`; | ||
| } finally { |
There was a problem hiding this comment.
Avoid String.fromCharCode(...bytes) for base64 conversion to prevent crashes on larger files
In readAsDataURL, this line:
const base64 = btoa(String.fromCharCode(...new Uint8Array(fileData)));spreads every byte as a separate argument to String.fromCharCode. For multi‑MB files (you allow up to 10 MB), this can easily exceed the engine’s argument limit and throw a RangeError, even though the file size is within your configured limit.
Switch to a chunked conversion to stay within safe argument counts:
- const fileData = await readFile(relativePath, {
- baseDir: BaseDirectory.Data,
- });
-
- const base64 = btoa(String.fromCharCode(...new Uint8Array(fileData)));
-
- return `data:${entry.mimeType};base64,${base64}`;
+ const fileData = await readFile(relativePath, {
+ baseDir: BaseDirectory.Data,
+ });
+
+ const bytes = new Uint8Array(fileData);
+ const chunkSize = 8_192;
+ let binary = "";
+
+ for (let i = 0; i < bytes.length; i += chunkSize) {
+ // Build the binary string in manageable chunks to avoid exceeding the
+ // argument limit of String.fromCharCode.
+ binary += String.fromCharCode(
+ ...bytes.subarray(i, i + chunkSize),
+ );
+ }
+
+ const base64 = btoa(binary);
+ return `data:${entry.mimeType};base64,${base64}`;This keeps memory usage reasonable and avoids call‑stack/argument‑limit issues for attachments up to your configured max size.
No description provided.