improved error handling during attachment downloads.#1751
Conversation
WalkthroughThis set of changes migrates email attachment handling from static message properties to dynamic fetching using a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailDisplay (React)
participant useAttachments (Hook)
participant TRPC Client
participant Server (TRPC)
participant ZeroAgent
participant MailDriver
User->>MailDisplay: View email with attachments
MailDisplay->>useAttachments: Call useAttachments(emailId)
useAttachments->>TRPC Client: Query mail.getMessageAttachments({messageId})
TRPC Client->>Server: mail.getMessageAttachments({messageId})
Server->>ZeroAgent: getMessageAttachments(messageId)
ZeroAgent->>MailDriver: getMessageAttachments(messageId)
MailDriver-->>ZeroAgent: [Attachment objects]
ZeroAgent-->>Server: [Attachment objects]
Server-->>TRPC Client: [Attachment objects]
TRPC Client-->>useAttachments: [Attachment objects]
useAttachments-->>MailDisplay: { data: [Attachment objects] }
MailDisplay-->>User: Render attachments UI
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic found 3 issues across 13 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
| const { data: session } = useSession(); | ||
| const trpc = useTRPC(); | ||
| const attachementsQuery = useQuery( | ||
| trpc.mail.getMessageAttachments.queryOptions( |
There was a problem hiding this comment.
Rule violated: Detect React performance bottlenecks and rule breaking
Passing an object literal directly as a parameter to a hook (here, as the first argument to queryOptions inside useQuery) can cause unnecessary re-renders or effect re-executions, violating the React performance rule. This should be memoized with useMemo or moved outside the hook.
Prompt for AI agents
Address the following comment on apps/mail/hooks/use-attachements.ts at line 9:
<comment>Passing an object literal directly as a parameter to a hook (here, as the first argument to queryOptions inside useQuery) can cause unnecessary re-renders or effect re-executions, violating the React performance rule. This should be memoized with useMemo or moved outside the hook.</comment>
<file context>
@@ -0,0 +1,16 @@
+import { useTRPC } from '@/providers/query-provider';
+import { useQuery } from '@tanstack/react-query';
+import { useSession } from '@/lib/auth-client';
+
+export const useAttachements = (messageId: string) => {
+ const { data: session } = useSession();
+ const trpc = useTRPC();
+ const attachementsQuery = useQuery(
+ trpc.mail.getMessageAttachments.queryOptions(
+ { messageId },
+ { enabled: !!session?.user.id && !!messageId, staleTime: 1000 * 60 * 60 },
+ ),
+ );
+
+ return attachementsQuery;
+};
</file context>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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 (9)
apps/server/src/lib/gmail-rate-limit.ts (1)
31-36: Update the comment to reflect the actual retry count.The comment on line 31 still references "max 10 attempts" but the code now uses 3 attempts. This inconsistency could confuse future developers.
/** * A schedule that: - * – retries while the error *is* a rate-limit error (max 10 attempts) + * – retries while the error *is* a rate-limit error (max 3 attempts) * – waits 60 seconds between retries (conservative for Gmail user quotas) * – stops immediately for any other error */apps/server/src/trpc/routes/mail.ts (1)
401-423: LGTM: Well-structured procedure following established patterns.The new
getMessageAttachmentsprocedure correctly implements the on-demand attachment fetching pattern described in the PR objectives. The implementation follows the established activeDriverProcedure pattern and includes proper input validation.Consider extracting the return type to a shared type definition for better maintainability:
+const MessageAttachmentSchema = z.object({ + filename: z.string(), + mimeType: z.string(), + size: z.number(), + attachmentId: z.string(), + headers: z.array(z.object({ + name: z.string(), + value: z.string(), + })), + body: z.string(), +}); getMessageAttachments: activeDriverProcedure .input( z.object({ messageId: z.string(), }), ) + .output(z.array(MessageAttachmentSchema)) .query(async ({ ctx, input }) => { const { activeConnection } = ctx; const agent = await getZeroAgent(activeConnection.id); - return agent.getMessageAttachments(input.messageId) as Promise< - { - filename: string; - mimeType: string; - size: number; - attachmentId: string; - headers: { - name: string; - value: string; - }[]; - body: string; - }[] - >; + return agent.getMessageAttachments(input.messageId); }),apps/server/src/routes/agent/mcp.ts (2)
23-25: Remove unnecessary constructor.The constructor only calls
super()and doesn't add any custom logic, making it redundant.- constructor(ctx: DurableObjectState, env: Env) { - super(ctx, env); - }
14-14: Replace banned{}type with explicit interface.The
{}type is too permissive and should be replaced with a more specific type definition.+interface McpContext { + // Define specific properties if needed, or use Record<string, never> for truly empty object +} + -export class ZeroMCP extends McpAgent<typeof env, {}, { userId: string }> { +export class ZeroMCP extends McpAgent<typeof env, McpContext, { userId: string }> {apps/mail/components/mail/mail-display.tsx (2)
382-387: Remove unusedattachmentIdfrom function parameterThe
attachmentIdproperty is included in the attachment parameter type but is never used within the function.If
attachmentIdis not needed for downloading attachments, consider simplifying the parameter type:-const downloadAttachment = async (attachment: { - body: string; - mimeType: string; - filename: string; - attachmentId: string; -}) => { +const downloadAttachment = async (attachment: { + body: string; + mimeType: string; + filename: string; +}) => {
471-476: Remove unusedattachmentIdfrom function parameterSimilar to
downloadAttachment, theattachmentIdproperty is included but never used.-const openAttachment = async (attachment: { - body: string; - mimeType: string; - filename: string; - attachmentId: string; -}) => { +const openAttachment = async (attachment: { + body: string; + mimeType: string; + filename: string; +}) => {apps/server/src/routes/agent/types.ts (1)
3-18: Inconsistent enum naming conventionThe enum values use different naming patterns: chat-related values use PascalCase without separators while mail-related values use underscores.
Consider using a consistent naming convention:
export enum IncomingMessageType { UseChatRequest = 'cf_agent_use_chat_request', ChatClear = 'cf_agent_chat_clear', ChatMessages = 'cf_agent_chat_messages', ChatRequestCancel = 'cf_agent_chat_request_cancel', - Mail_List = 'zero_mail_list_threads', - Mail_Get = 'zero_mail_get_thread', + MailList = 'zero_mail_list_threads', + MailGet = 'zero_mail_get_thread', } export enum OutgoingMessageType { ChatMessages = 'cf_agent_chat_messages', UseChatResponse = 'cf_agent_use_chat_response', ChatClear = 'cf_agent_chat_clear', - Mail_List = 'zero_mail_list_threads', - Mail_Get = 'zero_mail_get_thread', + MailList = 'zero_mail_list_threads', + MailGet = 'zero_mail_get_thread', }apps/server/src/routes/agent/index.ts (1)
759-763: Remove commented-out broadcast codeIf the broadcast functionality is no longer needed, remove the commented code to improve maintainability.
- // // Broadcast progress after each thread - // this.broadcastChatMessage({ - // type: OutgoingMessageType.Mail_List, - // folder, - // });apps/server/src/routes/agent/rpc.ts (1)
110-119: Remove or document commented-out methodsMultiple methods are commented out without explanation. Either remove them if deprecated or add TODO comments explaining why they're disabled.
If these methods are no longer needed, remove them entirely. If they're temporarily disabled, add explanatory comments:
- // async list(params: { - // folder: string; - // query?: string; - // maxResults?: number; - // labelIds?: string[]; - // pageToken?: string; - // }) { - // return await this.mainDo.list(params); - // } + // TODO: Temporarily disabled - use listThreads instead + // async list(params: { + // ...Or simply remove the commented code if it's no longer needed.
Also applies to: 136-139, 173-186
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
apps/mail/components/mail/mail-display.tsx(12 hunks)apps/mail/hooks/use-attachements.ts(1 hunks)apps/server/package.json(1 hunks)apps/server/src/lib/driver/google.ts(2 hunks)apps/server/src/lib/driver/types.ts(1 hunks)apps/server/src/lib/gmail-rate-limit.ts(1 hunks)apps/server/src/lib/party.ts(0 hunks)apps/server/src/main.ts(2 hunks)apps/server/src/routes/agent/index.ts(5 hunks)apps/server/src/routes/agent/mcp.ts(1 hunks)apps/server/src/routes/agent/rpc.ts(1 hunks)apps/server/src/routes/agent/types.ts(1 hunks)apps/server/src/trpc/routes/mail.ts(1 hunks)apps/server/wrangler.jsonc(1 hunks)
💤 Files with no reviewable changes (1)
- apps/server/src/lib/party.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/lib/gmail-rate-limit.ts (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:331-331
Timestamp: 2025-06-28T03:56:09.376Z
Learning: In apps/server/src/trpc/routes/mail.ts, the user indicated they are not using ISO format for the scheduleAt parameter, despite the frontend code showing toISOString() usage in the ScheduleSendPicker component.
apps/mail/hooks/use-attachements.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/main.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/lib/driver/types.ts (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
apps/server/src/trpc/routes/mail.ts (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:331-331
Timestamp: 2025-06-28T03:56:09.376Z
Learning: In apps/server/src/trpc/routes/mail.ts, the user indicated they are not using ISO format for the scheduleAt parameter, despite the frontend code showing toISOString() usage in the ScheduleSendPicker component.
apps/server/src/lib/driver/google.ts (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
apps/server/src/routes/agent/types.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/routes/agent/index.ts (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
apps/mail/components/mail/mail-display.tsx (2)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1328
File: apps/mail/lib/hotkeys/mail-list-hotkeys.tsx:202-209
Timestamp: 2025-06-18T17:26:50.918Z
Learning: In apps/mail/lib/hotkeys/mail-list-hotkeys.tsx, the switchCategoryByIndex function using hardcoded indices for category hotkeys does not break when users reorder categories, contrary to the theoretical index-shifting issue. The actual implementation has constraints or mechanisms that prevent hotkey targeting issues.
apps/server/src/routes/agent/mcp.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/routes/agent/rpc.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
🪛 Biome (1.9.4)
apps/server/src/routes/agent/mcp.ts
[error] 23-25: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 14-14: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ 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). (2)
- GitHub Check: Cursor BugBot
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (11)
apps/server/package.json (1)
37-37: Verify compatibility of@modelcontextprotocol/sdkandagentsupdatesRelease notes for
@modelcontextprotocol/sdkv1.13.0, v1.14.0, and v1.15.1 are not available via GitHub releases—please review the repository’s CHANGELOG or commit history to confirm no breaking changes were introduced.From
npm viewcomparison ofagents:
- Transitive bump:
@modelcontextprotocol/sdk^1.11.4 → ^1.13.3- New dependency added:
mimetext(^3.0.27)- Other bumps:
partyserver0.0.71 → 0.0.72,zod3.25.17 → 3.25.67Please manually verify:
- That your codebase’s usage of the SDK remains compatible with the new minor/patch versions.
- That the new
mimetextdependency and any other updated packages do not introduce unintended side effects.- Integration tests pass against these versions before merging.
apps/server/src/main.ts (1)
31-34: Confirm DurableMailbox removal
Searches across all .ts/.js files returned no occurrences ofDurableMailbox, and your import/export cleanup in apps/server/src/main.ts (lines 31–34) aligns with that. However, the automated check of your Cloudflare worker config (wrangler.jsonc) failed due to a parse error.Please manually verify that any durable object binding for
DurableMailboxhas been removed from your worker configuration (e.g. wrangler.toml/jsonc) to ensure there are no leftover bindings.• apps/server/src/main.ts: imports/exports updated
• NoDurableMailboxreferences in codebase
• Manual check needed for durable object bindings in wrangler.* config filesapps/mail/hooks/use-attachements.ts (1)
6-16: Well-implemented hook with proper authentication and caching.The hook correctly:
- Checks user authentication before enabling the query
- Uses proper caching with 1-hour stale time
- Conditionally enables the query based on session and messageId
- Returns the full query object for flexible usage by consumers
apps/server/src/lib/driver/types.ts (1)
54-65: Well-designed interface addition for attachment fetching.The new
getMessageAttachmentsmethod provides a comprehensive interface for fetching attachment data with all necessary metadata (filename, mimeType, size, attachmentId, headers) and the attachment body. This design supports the on-demand attachment fetching strategy mentioned in the PR objectives.apps/server/wrangler.jsonc (1)
354-354: LGTM: Thread sync optimization aligns with PR objectives.The reduction of
THREAD_SYNC_MAX_COUNTfrom 40 to 10 in production aligns with the PR's goal of reducing retry attempts and minimizing delays from Gmail API failures.apps/server/src/lib/driver/google.ts (2)
85-128: LGTM: Well-implemented on-demand attachment fetching.The new
getMessageAttachmentsmethod correctly implements the on-demand attachment fetching pattern described in the PR objectives. Key strengths:
- Proper error handling: Individual attachment failures don't break the entire operation
- Type safety: Uses proper type guards in the filter operation
- Consistent with existing patterns: Uses the same
withErrorHandlerpattern as other methods- Efficient: Only fetches attachment data when explicitly requested
426-442: LGTM: Efficient metadata-only storage strategy.The modification to store only attachment metadata (with empty body) in the
getmethod is a good optimization that aligns with the PR's goal of avoiding unnecessary data fetching and potential UI errors from incomplete data.apps/server/src/routes/agent/mcp.ts (1)
27-458: LGTM: Comprehensive MCP agent implementation.The MCP agent implementation provides a well-structured set of tools for email management. Key strengths:
- Proper resource management: Database connection is properly closed with
waitUntil- Consistent error handling: Tools use try-catch blocks where appropriate
- Good separation of concerns: Each tool has a specific, well-defined purpose
- Proper input validation: Uses Zod schemas for input validation
The commented-out bulk operations might be intentionally disabled for this iteration.
apps/server/src/routes/agent/index.ts (2)
595-601: LGTM! Clean implementation of attachment fetchingThe method follows the established pattern for driver delegation with appropriate error handling.
700-726: Well-implemented async generator for thread streamingThe implementation properly handles pagination, rate limiting, and memory efficiency through the generator pattern. The 2-second delay between pages is a good approach to avoid rate limits.
apps/server/src/routes/agent/rpc.ts (1)
72-76: Correct usage of new syncThread APIAll syncThread calls have been properly updated to use the new object parameter format
{ threadId: id }.Also applies to: 82-86, 88-92, 120-124, 126-130
apps/mail/hooks/use-attachements.ts
Outdated
| import { useQuery } from '@tanstack/react-query'; | ||
| import { useSession } from '@/lib/auth-client'; | ||
|
|
||
| export const useAttachements = (messageId: string) => { |
There was a problem hiding this comment.
Fix the spelling error in the hook name.
The hook name useAttachements contains a spelling error - it should be useAttachments (without the extra 'e'). This affects the function name and likely the filename as well.
-export const useAttachements = (messageId: string) => {
+export const useAttachments = (messageId: string) => {Also rename the file from use-attachements.ts to use-attachments.ts to maintain consistency.
📝 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.
| export const useAttachements = (messageId: string) => { | |
| -export const useAttachements = (messageId: string) => { | |
| +export const useAttachments = (messageId: string) => { |
🤖 Prompt for AI Agents
In apps/mail/hooks/use-attachements.ts at line 5, the hook name is misspelled as
useAttachements; rename the function to useAttachments to correct the spelling.
Also, rename the file from use-attachements.ts to use-attachments.ts to keep the
filename consistent with the corrected hook name.
MrgSub
force-pushed
the
07-17-improved_error_handling_during_attachment_downloads
branch
from
b0a3520 to
cc66032
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/server/src/routes/agent/index.ts (1)
623-623: Breaking API change in syncThread method signatureThe method signature has changed from
syncThread(threadId: string)tosyncThread({ threadId }: { threadId: string }). This is a breaking change that requires updates to all callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/mail/components/create/email-composer.tsx(2 hunks)apps/mail/components/mail/mail-display.tsx(12 hunks)apps/mail/components/mail/mail.tsx(2 hunks)apps/mail/components/ui/app-sidebar.tsx(1 hunks)apps/mail/components/ui/nav-main.tsx(4 hunks)apps/mail/hooks/use-attachments.ts(1 hunks)apps/server/package.json(1 hunks)apps/server/src/lib/driver/google.ts(2 hunks)apps/server/src/lib/driver/types.ts(1 hunks)apps/server/src/lib/gmail-rate-limit.ts(1 hunks)apps/server/src/lib/party.ts(0 hunks)apps/server/src/main.ts(2 hunks)apps/server/src/routes/agent/index.ts(6 hunks)apps/server/src/routes/agent/mcp.ts(1 hunks)apps/server/src/routes/agent/rpc.ts(1 hunks)apps/server/src/routes/agent/types.ts(1 hunks)apps/server/src/trpc/routes/mail.ts(1 hunks)apps/server/wrangler.jsonc(1 hunks)
💤 Files with no reviewable changes (1)
- apps/server/src/lib/party.ts
✅ Files skipped from review due to trivial changes (1)
- apps/server/src/lib/gmail-rate-limit.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/server/package.json
- apps/server/src/main.ts
- apps/server/src/lib/driver/types.ts
- apps/server/wrangler.jsonc
- apps/server/src/trpc/routes/mail.ts
- apps/server/src/routes/agent/types.ts
- apps/server/src/lib/driver/google.ts
- apps/mail/components/mail/mail-display.tsx
- apps/server/src/routes/agent/rpc.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/mail/components/ui/app-sidebar.tsx (2)
Learnt from: danteissaias
PR: Mail-0/Zero#902
File: apps/mail/components/connection/add.tsx:77-77
Timestamp: 2025-05-07T16:55:46.513Z
Learning: For the "Upgrade" link in AddConnectionDialog, using a proper <button> element instead of a <span> with onClick is recognized as an accessibility improvement but was deferred as out of scope in PR #902 (CSS variables PR).
Learnt from: snehendu098
PR: Mail-0/Zero#1323
File: apps/mail/lib/themes/theme-utils.ts:318-318
Timestamp: 2025-06-24T06:22:58.753Z
Learning: In the Mail-0/Zero theme system (apps/mail/lib/themes/theme-utils.ts), when color themes are being applied, all color values come in HSL format, so there's no need for additional format validation when converting colors with hslToHex().
apps/mail/components/ui/nav-main.tsx (4)
Learnt from: retrogtx
PR: Mail-0/Zero#1328
File: apps/mail/lib/hotkeys/mail-list-hotkeys.tsx:202-209
Timestamp: 2025-06-18T17:26:50.918Z
Learning: In apps/mail/lib/hotkeys/mail-list-hotkeys.tsx, the switchCategoryByIndex function using hardcoded indices for category hotkeys does not break when users reorder categories, contrary to the theoretical index-shifting issue. The actual implementation has constraints or mechanisms that prevent hotkey targeting issues.
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
Learnt from: danteissaias
PR: Mail-0/Zero#458
File: apps/mail/lib/email-utils.ts:126-131
Timestamp: 2025-03-16T23:14:09.209Z
Learning: When working with mailto URLs in JavaScript/TypeScript, the `url.pathname` property correctly extracts the email address from a mailto URL (e.g., for "mailto:test@example.com?subject=Test", `url.pathname` will be "test@example.com").
Learnt from: Fahad-Dezloper
PR: Mail-0/Zero#1440
File: apps/mail/components/create/ai-chat.tsx:101-113
Timestamp: 2025-06-22T19:23:10.599Z
Learning: On small and medium screens (mobile devices), buttons don't naturally lose focus after being clicked like they do on desktop browsers. This requires manual intervention using setTimeout with additional blur() calls and DOM manipulation to properly clear focus and active states in button click handlers for responsive design.
apps/mail/components/create/email-composer.tsx (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/mail/hooks/use-attachments.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/mail/components/mail/mail.tsx (4)
Learnt from: retrogtx
PR: Mail-0/Zero#1328
File: apps/mail/lib/hotkeys/mail-list-hotkeys.tsx:202-209
Timestamp: 2025-06-18T17:26:50.918Z
Learning: In apps/mail/lib/hotkeys/mail-list-hotkeys.tsx, the switchCategoryByIndex function using hardcoded indices for category hotkeys does not break when users reorder categories, contrary to the theoretical index-shifting issue. The actual implementation has constraints or mechanisms that prevent hotkey targeting issues.
Learnt from: snehendu098
PR: Mail-0/Zero#1323
File: apps/mail/lib/themes/theme-utils.ts:318-318
Timestamp: 2025-06-24T06:22:58.753Z
Learning: In the Mail-0/Zero theme system (apps/mail/lib/themes/theme-utils.ts), when color themes are being applied, all color values come in HSL format, so there's no need for additional format validation when converting colors with hslToHex().
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
Learnt from: danteissaias
PR: Mail-0/Zero#902
File: apps/mail/components/connection/add.tsx:77-77
Timestamp: 2025-05-07T16:55:46.513Z
Learning: For the "Upgrade" link in AddConnectionDialog, using a proper <button> element instead of a <span> with onClick is recognized as an accessibility improvement but was deferred as out of scope in PR #902 (CSS variables PR).
apps/server/src/routes/agent/mcp.ts (1)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
apps/server/src/routes/agent/index.ts (3)
Learnt from: retrogtx
PR: Mail-0/Zero#1468
File: apps/server/src/trpc/routes/mail.ts:386-391
Timestamp: 2025-06-27T04:59:29.731Z
Learning: In apps/server/src/trpc/routes/mail.ts, the attachment processing logic conditionally handles mixed attachment types - it preserves existing File-like objects with arrayBuffer methods while only converting serialized attachments that need processing through toAttachmentFiles.
Learnt from: retrogtx
PR: Mail-0/Zero#1734
File: apps/server/src/lib/driver/google.ts:211-221
Timestamp: 2025-07-15T06:46:33.349Z
Learning: In apps/server/src/lib/driver/google.ts, the normalization of "draft" to "drafts" in the count() method is necessary because the navigation item in apps/mail/config/navigation.ts has id: 'drafts' (plural) while the Google API returns "draft" (singular). The nav-main.tsx component matches stats by comparing stat.label with item.id, so the backend must return "drafts" for the draft counter badge to appear in the sidebar.
Learnt from: retrogtx
PR: Mail-0/Zero#1622
File: apps/server/src/lib/email-verification.ts:189-189
Timestamp: 2025-07-05T05:27:24.623Z
Learning: During testing phases, debug logging should be kept active in apps/server/src/lib/email-verification.ts for BIMI validation and email verification debugging, even if it's verbose.
🧬 Code Graph Analysis (5)
apps/mail/components/ui/app-sidebar.tsx (1)
apps/mail/components/icons/icons.tsx (1)
PencilCompose(244-256)
apps/mail/components/ui/nav-main.tsx (1)
apps/mail/components/context/command-palette-context.tsx (1)
useCommandPalette(114-120)
apps/mail/components/create/email-composer.tsx (1)
apps/mail/components/icons/icons.tsx (1)
Sparkles(561-592)
apps/mail/components/mail/mail.tsx (1)
apps/mail/components/icons/icons.tsx (1)
ChevronDown(1200-1216)
apps/server/src/routes/agent/mcp.ts (4)
apps/server/src/db/index.ts (1)
createDb(7-11)apps/server/src/db/schema.ts (1)
connection(118-142)apps/server/src/lib/prompts.ts (1)
GmailSearchAssistantSystemPrompt(233-279)apps/server/src/lib/server-utils.ts (1)
getZeroAgent(13-18)
🪛 GitHub Actions: autofix.ci
apps/mail/components/ui/nav-main.tsx
[warning] 7-7: ESLint: Identifier 'useSearchValue' is imported but never used. Consider removing this import. (no-unused-vars)
🪛 Biome (1.9.4)
apps/server/src/routes/agent/mcp.ts
[error] 39-41: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
[error] 30-30: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ 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). (2)
- GitHub Check: Cursor BugBot
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
apps/mail/components/ui/nav-main.tsx (2)
274-274: Good integration with command palette context.The destructuring of
clearAllFiltersfrom the command palette context is properly implemented.
294-294: Excellent UX improvement for filter state management.Clearing all filters when navigating to different sections provides a clean user experience. This ensures users don't get confused by lingering filters when switching contexts.
apps/mail/components/create/email-composer.tsx (2)
152-152: Appropriate use of query state for reply tracking.The
activeReplyIdstate variable properly tracks reply scenarios and integrates well with the existing query state pattern.
1228-1256: Smart conditional rendering for reply scenarios.Hiding the subject field when replying (
activeReplyIdis present) is a good UX improvement since reply subjects are typically inherited from the original message. The conditional rendering correctly includes both the input field and the AI generate button.apps/mail/hooks/use-attachments.ts (1)
5-16: Well-implemented hook for dynamic attachment fetching.The hook correctly:
- Checks authentication before enabling the query
- Uses appropriate caching with 1-hour stale time
- Follows established TRPC and React Query patterns
- Includes sensible enabling conditions
This aligns perfectly with the PR's goal of moving from static to dynamic attachment data.
apps/mail/components/ui/app-sidebar.tsx (1)
179-189: Consistent styling update for compose button prominence.The styling changes create a more prominent compose button with:
- Blue background (
bg-[#006FFE])- White text and icons (
fill-white,text-white)- Consistent color scheme across all button states
This makes the compose action more visually prominent, which is appropriate for such an important user action.
apps/mail/components/mail/mail.tsx (3)
534-534: LGTM! Equivalent height conversion to Tailwind shorthand.The change from
h-[1.875rem]toh-7maintains the same height (1.875rem = 30px) while using Tailwind's more concise class notation.
771-771: Excellent theming improvement!Removing hardcoded colors (
bg-[#006FFE]andtext-white) and replacing them with theme-aware classes (black:text-white text-muted-foreground) makes the component more flexible and consistent with the design system.
779-779: Consistent theming applied to ChevronDown icon.The conditional theming classes ensure the ChevronDown icon adapts properly to different themes, maintaining visual consistency with the button styling changes above.
apps/server/src/routes/agent/mcp.ts (2)
43-51: Verify database connection cleanup.The database connection is properly closed with
this.ctx.waitUntil(conn.end())at the end of init, which is good practice for preventing connection leaks.
52-406: Comprehensive MCP tool registration implementation.The tool registration covers all essential email operations:
- Connection management (get, set, list)
- AI-powered Gmail search query building
- Thread operations (list, get, mark read/unread, modify labels)
- Label management (get, create, list)
- Utility functions (current date)
The implementation is well-structured with proper error handling and appropriate input validation using Zod schemas.
apps/server/src/routes/agent/index.ts (5)
611-616: New method supports dynamic attachment fetching.The
getMessageAttachmentsmethod delegates to the driver's implementation, enabling the dynamic attachment fetching feature described in the PR objectives. This integrates well with the newuseAttachmentshook on the client side.
716-741: Excellent streaming implementation for thread processing.The new
streamThreadsasync generator provides better resource management by:
- Processing threads one at a time instead of loading entire batches
- Including rate limiting delays (2000ms)
- Using smaller batch sizes (maxCount = 10)
- Supporting pagination with proper termination conditions
This approach reduces memory usage and improves responsiveness.
765-780: Refactored syncThreads to use streaming and queuing.The new implementation processes threads individually through the async generator and queues each sync operation, replacing the previous batch processing approach. This provides better error isolation and progress tracking.
53-53: Performance improvement with reduced batch size.Reducing
maxCountfrom 40 to 10 aligns with the streaming approach and helps prevent rate limiting issues while maintaining good throughput.
1051-1051: Verify syncThread call is updated to new signature.This internal call to
syncThreadappears to use the new object parameter syntax correctly.
There was a problem hiding this comment.
Bug: Compose Button Accessibility: Color Contrast Issue
The "Compose" button has poor color contrast. Its blue background (bg-[#006FFE]) is paired with black text (text-black) and the PencilCompose icon also has a conflicting text-black class. This makes the button's content difficult to read or invisible in light mode, creating accessibility issues. The text and icon fill should be white for proper visibility.
apps/mail/components/ui/app-sidebar.tsx#L178-L181
Zero/apps/mail/components/ui/app-sidebar.tsx
Lines 178 to 181 in cc66032
Bug: Thread Sync Queue Parameter Mismatch
The syncThread method's signature was updated to expect an object { threadId: string }, but the queue call at this.queue('syncThread', thread.id) still passes a plain string. This parameter mismatch will cause the queued job to fail.
apps/server/src/routes/agent/index.ts#L767-L769
Zero/apps/server/src/routes/agent/index.ts
Lines 767 to 769 in cc66032
Was this report helpful? Give feedback by reacting with 👍 or 👎
MrgSub
deleted the
07-17-improved_error_handling_during_attachment_downloads
branch
1 participant
Improved Attachment Handling and UI Refinements
Description
This PR improves email attachment handling by implementing on-demand attachment fetching and enhancing error handling. It also includes several UI refinements to the email composer and navigation components.
Key changes:
Type of Change
Areas Affected
Testing Done
Security Considerations
Checklist
Additional Notes
The attachment handling improvements should resolve issues where large attachments were causing performance problems or failing to load properly. The UI refinements provide a more consistent experience across the application.