Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a wide-ranging refactor and feature expansion across the mail application. Major changes include: a new Microsoft mail driver and authentication provider, a comprehensive overhaul of the email composition UI (with the introduction of a modular Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant AI Sidebar
participant API
participant EmailDriver
participant WritingStyleService
User->>UI: Compose or reply to email
UI->>AI Sidebar: (optional) Request suggestion
AI Sidebar->>API: /api/ai-search or /api/chat
API->>AI Sidebar: AI-powered suggestion or search results
User->>UI: Accept suggestion, send email
UI->>API: sendEmail (with ISendEmail object)
API->>EmailDriver: Send email
API->>WritingStyleService: updateWritingStyleMatrix(connectionId, emailBody)
WritingStyleService-->>API: Update style matrix in DB
API->>UI: Email sent confirmation
sequenceDiagram
participant ScriptUser
participant CLI
participant SeederScript
participant DB
participant WritingStyleService
ScriptUser->>CLI: bun scripts seed-style seed --connection-id ...
CLI->>SeederScript: Parse args, prompt if missing
SeederScript->>DB: Validate connection ID
SeederScript->>WritingStyleService: updateWritingStyleMatrix for each email
WritingStyleService->>DB: Update style matrix
SeederScript->>CLI: Output progress and results
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (54)
apps/mail/app/api/utils.ts (1)
73-76: 💡 Verification agent
🧩 Analysis chain
Verify rate limit detection logic with API documentation
The rate limit detection logic checks for HTTP 429 status code and specific error reasons. Please verify that these patterns match the actual error response formats from all the APIs being called.
🌐 Web query:
What are the common rate limit error response formats for Microsoft Graph API?💡 Result:
Microsoft Graph API rate limits and error responses are designed to manage and control the usage of the API, ensuring reliability and resource efficiency. When these limits are exceeded, specific error responses are returned to the client. Here are the common error response formats for Microsoft Graph API rate limits:
Error Response Format
HTTP Status Code 429 - Too Many Requests: This is the standard HTTP status code returned when rate limits are exceeded. It indicates that the client has made too many requests within a given timeframe[2][3].
Retry-After Header: Along with the 429 status code, Microsoft Graph returns a
Retry-Afterheader. This header specifies the number of seconds the client should wait before making the next request[2][4].JSON Error Response Object: Error responses from Microsoft Graph API are typically returned in JSON format. This object provides detailed information about the error, which can help clients handle the error gracefully[3].
Example JSON Error Response
{ "error": { "code": "Throttled", "message": "Too many requests. Please wait before sending more requests.", "innerError": { "date": "2025-01-01T00:00:00", "request-id": "request-id", "client-request-id": "client-request-id" } } }Additional Details
- Request Types: The throttling behavior can vary based on the request type. For example, writes might be throttled more frequently than reads[2].
- Service-Specific Limits: Different Microsoft Graph services have their own specific throttling limits, which can lead to varied error responses depending on the service being accessed[1].
In general, when you encounter rate limit errors with Microsoft Graph API, it's advisable to implement exponential backoff strategies or use features like Microsoft Graph Data Connect for large data extractions to avoid hitting these limits[2].
Citations:
- 1: https://learn.microsoft.com/en-us/graph/throttling-limits
- 2: https://learn.microsoft.com/en-us/graph/throttling
- 3: https://learn.microsoft.com/en-us/graph/errors
- 4: https://learn.microsoft.com/en-us/graph/connecting-external-content-api-limits
- 5: https://learn.microsoft.com/en-us/answers/questions/219222/graph-api-increase-rate-limiting-throttling
- 6: https://github.com/microsoftgraph/microsoft-graph-docs-contrib/blob/main/concepts/throttling-limits.md
- 7: https://learn.microsoft.com/en-us/answers/questions/825267/regarding-rate-limit-for-graph-api
- 8: https://github.com/microsoftgraph/microsoft-graph-docs-contrib/blob/main/api-reference/beta/resources/security-error-codes.md
- 9: https://learn.microsoft.com/en-us/answers/questions/1195604/microsoft-graph-api-request-and-response-max-paylo
- 10: https://learn.microsoft.com/en-us/answers/questions/1284905/throttling-limit-for-get-mail-from-office-365-usin
🏁 Script executed:
#!/bin/bash # Display the contents around the rate limit detection logic in utils.ts sed -n '1,200p' apps/mail/app/api/utils.tsLength of output: 2751
🏁 Script executed:
#!/bin/bash # Search for usage of withExponentialBackoff across the codebase rg -n "withExponentialBackoff" --color=neverLength of output: 394
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of the Microsoft Graph API driver to inspect error handling sed -n '1,200p' apps/mail/app/api/driver/microsoft.tsLength of output: 6951
Expand rate limit detection logic to cover Microsoft Graph API
The current check only covers HTTP 429 and Google’s
rateLimitExceeded/userRateLimitExceededreasons. Microsoft Graph returns a JSON error with a string code ("Throttled") (and typically setsstatusCodeorresponse.statusto 429), so we should include those too.• File:
apps/mail/app/api/utils.ts
• Location: insidewithExponentialBackoff(around lines 73–76)Suggested diff:
- const isRateLimit = - error?.code === 429 || - error?.errors?.[0]?.reason === 'rateLimitExceeded' || - error?.errors?.[0]?.reason === 'userRateLimitExceeded'; + const isRateLimit = + // HTTP 429 from Google or generic fetch + error?.code === 429 || + // HTTP status on Graph client error + error?.statusCode === 429 || + error?.response?.status === 429 || + // Google API retry reasons + error?.errors?.[0]?.reason === 'rateLimitExceeded' || + error?.errors?.[0]?.reason === 'userRateLimitExceeded' || + // Microsoft Graph throttling + error?.error?.code === 'Throttled';📝 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.const isRateLimit = // HTTP 429 from Google or generic fetch error?.code === 429 || // HTTP status on Graph client error error?.statusCode === 429 || error?.response?.status === 429 || // Google API retry reasons error?.errors?.[0]?.reason === 'rateLimitExceeded' || error?.errors?.[0]?.reason === 'userRateLimitExceeded' || // Microsoft Graph throttling error?.error?.code === 'Throttled';apps/mail/app/globals.css (1)
64-64: 💡 Verification agent
🧩 Analysis chain
Verify background CSS variable value
The
--backgroundCSS variable in the dark theme has been changed from an HSL color value to simply1, which is not a valid CSS color format. This might cause rendering issues for elements using this variable.
🏁 Script executed:
#!/bin/bash # Check where the --background variable is used in the codebase rg -A 2 -B 2 "var\(--background\)" --type css --type tsx --type jsxLength of output: 97
🏁 Script executed:
#!/bin/bash # Retry searching for uses of --background without relying on type filters rg -A2 -B2 'var(--background)' --glob '*.css' --glob '*.tsx' --glob '*.jsx'Length of output: 77
🏁 Script executed:
#!/bin/bash # Search entire repository for any usage of the --background CSS variable rg -n 'var\(--background\)'Length of output: 272
Fix invalid
--backgroundCSS variable valueThe
--backgroundvariable in the dark theme is currently set to1, but it’s used throughout the code insidehsl(var(--background)). Supplying just1tohsl()is invalid (it expects three or four HSL components). This will break background colors in:
- apps/mail/tailwind.config.ts (line 30)
background: 'hsl(var(--background))',- apps/mail/app/globals.css (line 266)
background-color: hsl(var(--background));- apps/mail/components/create/prosemirror.css (line 72)
background-color: hsl(var(--background));Please update the variable to a valid HSL triplet (e.g.
--background: 210 10% 20%;or similar) so that allhsl(var(--background))calls render correctly.apps/mail/lib/providers.tsx (1)
4-4:
⚠️ Potential issueUnused import
The
EditorProvideris imported but not used in the component. Consider either removing the unused import or implementing it as intended.-import { EditorProvider } from '@/components/providers/editor-provider';📝 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.apps/mail/app/layout.tsx (2)
60-64: 💡 Verification agent
🧩 Analysis chain
Removed CookieProvider wrapper
You've removed the
<CookieProvider>wrapper around the children and UI components. This change might impact how cookies or state are managed in the application. Make sure all components previously relying on the CookieProvider context still function correctly.
🏁 Script executed:
#!/bin/bash # Check for components using CookieProvider context echo "Searching for components that might depend on CookieProvider..." rg -n "useCookies" --type=ts --type=tsxLength of output: 204
🏁 Script executed:
#!/bin/bash echo "Searching for CookieProvider references..." rg -n "CookieProvider" echo "Searching for useCookies references..." rg -n "useCookies" echo "Searching for react-cookie imports..." rg -n "react-cookie"Length of output: 1094
Restore
<CookieProvider>in Mail LayoutThe
useCookieshook (used incookie-dialog.tsx) relies on theCookieProvidercontext. Since you’ve removed it fromapps/mail/app/layout.tsx, any component callinguseCookieswill now throw at runtime.• File:
apps/mail/app/layout.tsx(around lines 60–64)
Wrap these nodes in<CookieProvider>…</CookieProvider>.Suggested diff:
- {children} - {cookies} - <Toast /> - <Analytics /> + <CookieProvider> + {children} + {cookies} + <Toast /> + <Analytics /> + </CookieProvider>📝 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.<CookieProvider> {children} {cookies} <Toast /> <Analytics /> </CookieProvider> {/* {isEuRegion && <CookieConsent />} */}
55-57: 💡 Verification agent
🧩 Analysis chain
Added suppressHydrationWarning to body element
Adding
suppressHydrationWarningon the body element will prevent React from showing hydration mismatch warnings. While this resolves hydration warnings, it's important to understand why these warnings occur in the first place. If there are actual hydration mismatches, consider fixing the root cause rather than suppressing the warnings.
🏁 Script executed:
#!/bin/bash # Check for components that might cause hydration issues # Look for components that use browser-specific APIs echo "Searching for components using browser APIs that might cause hydration issues..." rg -n "typeof window !== 'undefined'" --type=ts --type=tsx rg -n "document\." --type=ts --type=tsx rg -n "window\." --type=ts --type=tsxLength of output: 403
🏁 Script executed:
#!/bin/bash echo "Re-running search for browser-specific APIs in TS/TSX files..." rg -n "window\." -g '*.ts' -g '*.tsx' rg -n "document\." -g '*.ts' -g '*.tsx' rg -n "navigator\." -g '*.ts' -g '*.tsx'Length of output: 11145
Ensure hydration consistency by isolating client-only logic
Adding
suppressHydrationWarningon<body>will silence React’s hydration-mismatch warnings, but it doesn’t solve the underlying issue. Our search uncovered dozens of components and hooks readingwindow,document, ornavigatorduring render, for example:
• apps/mail/lib/email-utils.client.tsx
• apps/mail/lib/hotkeys/mail-list-hotkeys.tsx
• apps/mail/hooks/use-mobile.tsx
• apps/mail/components/ui/nav-main.tsx
• apps/mail/components/mail/mail-iframe.tsx
(and many more)To eliminate real mismatches, please:
• Mark any component that uses browser APIs at render time as a client component with a top-of-file
use clientdirective.
• Defer side-effects and globals (window.*,document.*,navigator.*) touseEffector event handlers, not the initial render.
• Ensure that server-rendered markup exactly matches the client’s first render; once you’ve corrected these, you can safely removesuppressHydrationWarning..github/workflows/ci.yml (1)
13-15:
⚠️ Potential issueFix environment variable usage in runs-on.
The current implementation using
${{ env.RUNNER_IMAGE }}in theruns-onfield is not valid according to GitHub Actions syntax.GitHub Actions doesn't allow the
envcontext in theruns-onfield. Change to either:- env: - RUNNER_IMAGE: ubuntu-latest - runs-on: ${{ env.RUNNER_IMAGE }} + runs-on: ubuntu-latestOr if you need to reuse the runner configuration:
- env: - RUNNER_IMAGE: ubuntu-latest - runs-on: ${{ env.RUNNER_IMAGE }} + runs-on: ubuntu-latest🧰 Tools
🪛 actionlint (1.7.4)
15-15: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
apps/mail/components/mail/nav-main.tsx (1)
1-21:
⚠️ Potential issueComponent implementation is incomplete.
The file contains only a skeleton of the NavMain component with a placeholder comment for the actual content. This component sets up state management for collapsed/expanded views but doesn't implement any UI rendering.
The component should be completed by replacing the placeholder comment with the actual navigation elements. Consider implementing the toolbar mentioned in the PR objectives that appears on hover.
.github/workflows/main.yml (1)
24-27:
⚠️ Potential issueFix environment variable reference in runs-on configuration
The workflow is trying to use an environment variable in the
runs-onfield, but there's an issue with how it's referenced.According to the static analysis tool, the "env" context is not allowed at the
runs-onfield level. GitHub Actions requires direct values or matrix variables here.Fix by using a hardcoded value or matrix variable:
- timeout-minutes: 15 - env: - RUNNER_IMAGE: ubuntu-latest - runs-on: ${{ env.RUNNER_IMAGE }} + timeout-minutes: 15 + runs-on: ubuntu-latest📝 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.timeout-minutes: 15 runs-on: ubuntu-latest🧰 Tools
🪛 actionlint (1.7.4)
27-27: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
apps/mail/scripts/seed-style/seeder.ts (2)
80-92: 🛠️ Refactor suggestion
Add validation for command-line arguments.
Currently, validation is only performed for interactive prompts but not for command-line arguments. Consider adding similar validation for the command-line arguments to ensure consistency.
handler: async (inputs) => { - const connectionId = inputs.connectionId ?? await getConnectionId() + let connectionId = inputs.connectionId; + if (connectionId) { + // Validate command-line provided connectionId + const connection = await db.query.connection.findFirst({ + where: (table, ops) => ops.eq(table.id, connectionId), + columns: { id: true }, + }); + + if (!connection) { + console.error('Invalid Connection ID provided via command line'); + connectionId = await getConnectionId(); + } + } else { + connectionId = await getConnectionId(); + } const style = inputs.style ?? await getStyle() const resetStyleMatrix = inputs.resetStyleMatrix ?? await getResetStyleMatrix(connectionId) const size = inputs.size ?? await getNumberOfEmails(mapping[style].length) if (resetStyleMatrix) { await runResetStyleMatrix(connectionId) } await runSeeder(connectionId, style, size) },📝 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.}, handler: async (inputs) => { let connectionId = inputs.connectionId; if (connectionId) { // Validate command-line provided connectionId const connection = await db.query.connection.findFirst({ where: (table, ops) => ops.eq(table.id, connectionId), columns: { id: true }, }); if (!connection) { console.error('Invalid Connection ID provided via command line'); connectionId = await getConnectionId(); } } else { connectionId = await getConnectionId(); } const style = inputs.style ?? await getStyle() const resetStyleMatrix = inputs.resetStyleMatrix ?? await getResetStyleMatrix(connectionId) const size = inputs.size ?? await getNumberOfEmails(mapping[style].length) if (resetStyleMatrix) { await runResetStyleMatrix(connectionId) } await runSeeder(connectionId, style, size) },
112-118: 🛠️ Refactor suggestion
Add error handling for database operations.
The reset operation doesn't have any error handling for the database delete operation, which could fail for various reasons.
if (confirmed) { console.warn('Resetting style matrix for connection', connectionId) - await runResetStyleMatrix(connectionId) - console.warn('Reset style matrix for connection', connectionId) + try { + await runResetStyleMatrix(connectionId) + console.log('Reset style matrix for connection', connectionId) + } catch (error) { + console.error('Failed to reset style matrix:', error) + } } else { console.warn('Aborted reset') }📝 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.if (confirmed) { console.warn('Resetting style matrix for connection', connectionId) try { await runResetStyleMatrix(connectionId) console.log('Reset style matrix for connection', connectionId) } catch (error) { console.error('Failed to reset style matrix:', error) } } else { console.warn('Aborted reset') }apps/mail/app/api/driver/types.ts (1)
12-19: 🛠️ Refactor suggestion
Address TODO comment for better type safety.
There's a TODO comment about adding a generic type parameter, which should be addressed to improve type safety.
export interface ParsedDraft { id: string; to?: string[]; subject?: string; content?: string; - // todo: add <T> - rawMessage?: any; + rawMessage?: Record<string, unknown>; }Alternatively, if you need to maintain the flexibility of accepting different message formats:
export interface ParsedDraft<T = Record<string, unknown>> { id: string; to?: string[]; subject?: string; content?: string; - // todo: add <T> - rawMessage?: any; + rawMessage?: T; }📝 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 interface ParsedDraft { id: string; to?: string[]; subject?: string; content?: string; rawMessage?: Record<string, unknown>; }export interface ParsedDraft<T = Record<string, unknown>> { id: string; to?: string[]; subject?: string; content?: string; rawMessage?: T; }apps/mail/components/create/editor.tsx (2)
90-96: 🛠️ Refactor suggestion
Good additions to the interface
The new props (
onEditorReady,readOnly, andhideToolbar) improve the component's flexibility. However, I notice that whilereadOnlyis used throughout the code, thehideToolbarproperty is defined but not actually implemented in the component logic.You should implement the
hideToolbarfunctionality by conditionally rendering the MenuBar component in the editor. Consider adding this to the Editor component:- <EditorContent + <EditorContent + slotBefore={!hideToolbar ? <MenuBar /> : undefined} immediatelyRender={false} initialContent={initialValue || defaultEditorContent}Committable suggestion skipped: line range outside the PR's diff.
351-351:
⚠️ Potential issueImplementation of the hideToolbar prop
While you've added the
hideToolbarprop to the component's props interface, it's not being used to actually hide the toolbar. The toolbar visibility should be conditional based on this prop.Implement the toolbar hiding functionality by ensuring the MenuBar is conditionally rendered:
return ( <div className={`relative w-full ${className || ''}`} onClick={focusEditor} ... > <EditorRoot> <EditorContent + slotBefore={!hideToolbar ? <MenuBar /> : undefined} immediatelyRender={false} initialContent={initialValue || defaultEditorContent}📝 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.return ( <div className={`relative w-full ${className || ''}`} onClick={focusEditor} ... > <EditorRoot> <EditorContent slotBefore={!hideToolbar ? <MenuBar /> : undefined} immediatelyRender={false} initialContent={initialValue || defaultEditorContent}apps/mail/app/(full-width)/about/page.tsx (1)
131-136:
⚠️ Potential issueEmail address mismatch
There's an inconsistency between the mailto link (using personal email "nizabizaher@gmail.com") and the displayed email address ("contact@0.email").
Consider updating the mailto link to use the same email that's displayed to users:
- href="mailto:nizabizaher@gmail.com" + href="mailto:contact@0.email" className="inline-flex items-center text-blue-400 hover:text-blue-300"📝 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.href="mailto:contact@0.email" className="inline-flex items-center text-blue-400 hover:text-blue-300" > <Mail className="mr-2 h-4 w-4" /> contact@0.email </a>apps/mail/components/ui/app-sidebar.tsx (1)
142-145: 🛠️ Refactor suggestion
Empty dialog title and description
The DialogTitle and DialogDescription components are rendered but empty. This could impact accessibility as screen readers rely on these elements.
Either add appropriate content to these elements or remove them:
- <DialogTitle></DialogTitle> - <DialogDescription></DialogDescription>apps/mail/components/icons/animated/alert.tsx (2)
67-72: 🛠️ Refactor suggestion
Missing className application
The component accepts a
classNameprop, but it's not applied to any element in the component.Apply the className prop to the container div:
<div - className="hover:bg-accent flex cursor-pointer select-none items-center justify-center rounded-md transition-colors duration-200" + className={cn("hover:bg-accent flex cursor-pointer select-none items-center justify-center rounded-md transition-colors duration-200", className)} onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave} {...props}📝 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.<div className={cn( "hover:bg-accent flex cursor-pointer select-none items-center justify-center rounded-md transition-colors duration-200", className )} onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave} {...props} >
31-43: 🛠️ Refactor suggestion
Size prop is not applied to SVG
The component accepts a
sizeprop (default 28), but it's not applied to the SVG element, which has hard-coded width and height attributes.Apply the size prop to the SVG element:
<motion.svg xmlns="http://www.w3.org/2000/svg" - width="15" - height="15" + width={size} + height={size} viewBox="0 0 24 24"apps/mail/components/create/voice.tsx (3)
91-107: 🛠️ Refactor suggestion
Clean-up ElevenLabs connection on unmount
useConversationlikely holds a WebSocket / media stream. If the component disappears while connected, the session and microphone stay open.const conversation = useConversation({...}); useEffect(() => { return () => { if (conversation.status === 'connected') { conversation.endSession().catch(console.error); } }; }, [conversation]);📝 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.const conversation = useConversation({ onConnect: () => { console.log("Connected to ElevenLabs"); }, onDisconnect: () => { console.log("Disconnected from ElevenLabs"); }, onMessage: (message) => { console.log("Received message:", message); }, onError: (error: string | Error) => { setErrorMessage(typeof error === "string" ? error : error.message); console.error("Error:", error); }, }); useEffect(() => { return () => { if (conversation.status === 'connected') { conversation.endSession().catch(console.error); } }; }, [conversation]); const { status, isSpeaking } = conversation;
60-71:
⚠️ Potential issueNull-safety for optional sender / recipient fields
latestMessage.sender.nameandt.namecan beundefined, which will render literal “undefined” in the prompt.From: ${latestMessage.sender.name ?? latestMessage.sender.email} To: ${latestMessage.to.map((t: Sender) => t.name ?? t.email).join(', ')} Subject: ${latestMessage.subject ?? '(No subject)'}Committable suggestion skipped: line range outside the PR's diff.
50-80: 🛠️ Refactor suggestion
Avoid state-updates after component unmount & reduce overfetching
fetchThreadContentsfires 20 concurrent requests every timethreadschanges, but:
- If the component unmounts before all requests settle, the later
setEmailContentwill raise a React warning (“Can’t perform a React state update on an unmounted component”).- Making up to 20 separate
/api/driver/:idcalls can quickly hit rate-limits. The driver already returns snippets – consider a batched endpoint or using the first page data instead.useEffect(() => { - if (!threads || threads.length === 0) return; + if (!threads?.length) return; ... - const fetchThreadContents = async () => { + const abort = new AbortController(); + const fetchThreadContents = async () => { const threadContents = await Promise.all( - threads.slice(0, 20).map(async (thread) => { + threads.slice(0, 20).map(async (thread) => { try { - const response = await fetch(`/api/driver/${thread.id}`); + const response = await fetch(`/api/driver/${thread.id}`, { signal: abort.signal });and return a cleanup:
- fetchThreadContents(); + fetchThreadContents(); + return () => abort.abort(); // prevent setState after unmountCommittable suggestion skipped: line range outside the PR's diff.
apps/mail/lib/email-utils.client.tsx (1)
164-168:
⚠️ Potential issueSanitize HTML before using
dangerouslySetInnerHTML
dangerouslySetInnerHTMLis flagged by Biome because it can enable XSS ifcontentis user-supplied.
Even though the CSP is strict, defense-in-depth is advisable. Consider running the HTML through a sanitizer such as DOMPurify before rendering:-import DOMPurify from 'dompurify'; ... -<div ... dangerouslySetInnerHTML={{ __html: content }} /> +import DOMPurify from 'dompurify'; +... +<div + ... + dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }} +/>If bundle size is a concern, a small allow-list sanitizer or server-side sanitisation can be used instead.
📝 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.// At the top of apps/mail/lib/email-utils.client.tsx import DOMPurify from 'dompurify'; // …other imports // …later in your JSX render… <Row style={{ background: 'transparent', padding: 0, margin: 0 }}> <Column style={{ background: 'transparent', padding: 0, margin: 0 }}> <div style={{ background: 'transparent', fontSize: '16px', lineHeight: '1.5' }} dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(content) }} /> </Column> </Row>🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
apps/mail/app/api/ai-search/route.ts (1)
35-37:
⚠️ Potential issueValidate request body shape with Zod
messagesis assumed to be an array; malformed input will throw at runtime. Add zod validation before use:const BodySchema = z.object({ messages: z.array(z.object({ role: z.string(), content: z.string() })) }); const parseResult = BodySchema.safeParse(await req.json()); if (!parseResult.success) { return new Response('Invalid body', { status: 400 }); } const { messages } = parseResult.data;apps/mail/components/golden.tsx (1)
155-160:
⚠️ Potential issueInvalid Tailwind class
mr-– missing value
mr-is not a valid Tailwind shorthand and will be ignored, leaving extra spacing un-applied.- <span className="mr-">Send invite</span> + <span className="mr-2">Send invite</span>Pick a value (
mr-1,mr-2, …) that matches adjacent spacing so the text isn’t glued to the shortcut hint.📝 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.<Button disabled={!email} type="submit" className="h-7"> - <span className="mr-">Send invite</span> + <span className="mr-2">Send invite</span> <div className="flex h-5 items-center justify-center gap-1 rounded-sm dark:bg-black/10 px-1 bg-white/10"> <Command className="h-2 w-2 text-black dark:text-[#929292]" /> <CurvedArrow className="mt-1.5 h-3 w-3 fill-black dark:fill-[#929292]" /> </div>apps/mail/components/ui/nav-user.tsx (1)
104-113: 🛠️ Refactor suggestion
⚠️ Potential issueUnsafe optional-chaining → possible
TypeError(.splitonundefined)If
activeAccountisundefined(or bothname&undefined, then.split()is invoked – exactly what Biome flagged.(activeAccount?.name || activeAccount?.email).split(' ')Minimal fix:
- {(activeAccount?.name || activeAccount?.email) + {(activeAccount?.name ?? activeAccount?.email ?? '') .split(' ')Recommended: extract a helper to de-duplicate the same pattern that appears three times in this component.
const getInitials = (name?: string | null, email?: string | null) => (name ?? email ?? '') .split(' ') .map((n) => n[0]) .join('') .toUpperCase() .slice(0, 2);Then replace all inline calculations with
getInitials(...).🧰 Tools
🪛 Biome (1.9.4)
[error] 111-111: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
apps/mail/components/create/create-email.tsx (2)
4-10:
⚠️ Potential issueRemove unused imports to avoid lint / build failures
useHotkeysContext,useSettings, anduseDraftare imported but never referenced. Most TS/ESLint configs treat this as an error and break CI/CD.-import { useHotkeysContext } from 'react-hotkeys-hook'; -import { useSettings } from '@/hooks/use-settings'; -import { useDraft } from '@/hooks/use-drafts';📝 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.import { DialogClose } from '@/components/ui/dialog'; import { EmailComposer } from './email-composer'; import { useSession } from '@/lib/auth-client'; import { useTranslations } from 'next-intl';
68-76: 🛠️ Refactor suggestion
Eliminate redundant
console.logwrapper aroundposthog.capture
posthog.capturealready records the event and returnsvoid(or a boolean in older SDKs). Wrapping it inconsole.logyieldsundefinedin the console and adds noise.-console.log(posthog.capture('Create Email Sent with CC and BCC')); +posthog.capture('Create Email Sent with CC and BCC');Apply the same adjustment for the CC-only, BCC-only, and default branches.
📝 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.if (data.cc && data.bcc) { posthog.capture('Create Email Sent with CC and BCC'); } else if (data.cc) { posthog.capture('Create Email Sent with CC'); } else if (data.bcc) { posthog.capture('Create Email Sent with BCC'); } else { posthog.capture('Create Email Sent'); }apps/mail/components/mail/mail-iframe.tsx (2)
72-90:
⚠️ Potential issuePotential memory-leak: generated Blob URLs are never revoked
template()creates a newBlobURL on everyhtml/imagesEnabledchange.
Because theurlvariable lives only inside thethencallback the cleanup section cannot access it (note that it’s commented out anyway), so every navigation / re-render leaves an orphaned object URL in memory.- template(html, imagesEnabled).then((htmlDoc) => { - if (!iframeRef.current) return; - const url = URL.createObjectURL(new Blob([htmlDoc], { type: 'text/html' })); + let url: string; + template(html, imagesEnabled).then((htmlDoc) => { + if (!iframeRef.current) return; + url = URL.createObjectURL(new Blob([htmlDoc], { type: 'text/html' })); iframeRef.current.src = url; … }); return () => { - // URL.revokeObjectURL(url); + if (url) URL.revokeObjectURL(url); };Keeping the revoke ensures that the browser’s in-memory blob store is freed when the component unmounts or the dependencies change.
Also applies to: 91-94
50-55: 🛠️ Refactor suggestion
Effect overrides manual “show images” toggle for trusted senders
When the sender is trusted the effect always forces
imagesEnabledtotrue. After the user clicks “Disable images” the value flips tofalsebut will be set back totrueon the next settings mutation (becauseisTrustedSenderstaystrue).
Persist the user override, e.g.:- useEffect(() => { - if (isTrustedSender) { - setImagesEnabled(true); - } - }, [isTrustedSender]); + useEffect(() => { + if (isTrustedSender && !imagesEnabled) { + setImagesEnabled(true); + } + }, [isTrustedSender, imagesEnabled]);Committable suggestion skipped: line range outside the PR's diff.
apps/mail/components/ui/ai-sidebar.tsx (1)
83-174:
⚠️ Potential issueRemove the unnecessary React Fragment flagged by lint
The fragment adds no semantic or styling value because it wraps a single
ResizablePanel.
Dropping it silencesnoUselessFragmentsand avoids an extra element in the tree.- {open && ( - <> - <ResizablePanel + {open && ( + <ResizablePanel defaultSize={25} … > … - </ResizablePanel> - </> + </ResizablePanel> )}📝 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.{open && ( <ResizablePanel defaultSize={25} minSize={20} maxSize={45} className="bg-panelLight dark:bg-panelDark ml- mr-1.5 mt-1 h-[calc(98vh+9px)] border-[#E7E7E7] shadow-sm md:rounded-2xl md:border md:shadow-sm dark:border-[#252525]" > <div className={cn('h-[calc(98vh+15px)]', 'flex flex-col', '', className)}> <div className="flex h-full flex-col"> {/* ...rest of content... */} </div> </div> </ResizablePanel> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 84-173: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/mail/components/home/HomeContent.tsx (3)
148-153:
⚠️ Potential issue
handleKeyDownis defined but never usedThe function listens for Cmd/Ctrl + Enter submission but is not bound to any element or event emitter, so the shortcut never fires.
Either attach it to the form element / relevant input, or remove the unused code to avoid dead-weight.
105-118: 🛠️ Refactor suggestion
Glow position isn’t recalculated on window resize / tab list layout change
handleTabChangeis only executed on tab change and initial mount. If the window is resized (or orientation changes) the cachedrectvalues become stale, causing the glow to mis-align.Add a
resizelistener that recomputes the glow:useEffect(() => { + const handler = () => handleTabChange(tabs.find(t => t.value === valueRef.current)?.value ?? ''); + window.addEventListener('resize', handler); return () => window.removeEventListener('resize', handler); -}, [handleTabChange]); +}, [handleTabChange]);Committable suggestion skipped: line range outside the PR's diff.
68-70: 🛠️ Refactor suggestion
Over-restrictive email validation may reject valid addresses
z.string().email().min(9)requires at least 9 characters, so a perfectly valid address likea@b.cowill fail.
Unless you have a business reason for this length check, consider removing the.min(9)constraint.- email: z.string().email().min(9), + email: z.string().email(),📝 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.const betaSignupSchema = z.object({ email: z.string().email(), });apps/mail/components/create/email-composer.tsx (2)
306-308:
⚠️ Potential issueEmpty stub:
handleGenerateReplyis unimplementedLeaving an empty handler will confuse future maintainers and may lead to runtime issues if the button becomes callable.
Either implement the logic or remove the button until ready.
162-189: 🛠️ Refactor suggestion
Repeated
setValue('bcc', …)inside loop can introduce duplicates & unnecessary rendersFor every BCC recipient you call
setValue, potentially causing:
- Duplicate addresses when a recipient already exists in the array.
- Multiple re-renders during a single effect.
Refactor to accumulate addresses first, then call
setValueonce:- latestEmail.bcc?.forEach((recipient) => { - … - form.setValue('bcc', [...(bccEmails || []), recipient.email]); - setShowBcc(true); - }); + const newBcc = new Set(bccEmails ?? []); + latestEmail.bcc?.forEach(r => { + const e = r.email.toLowerCase(); + if (e !== userEmail && !newBcc.has(r.email)) newBcc.add(r.email); + }); + if (newBcc.size) { + form.setValue('bcc', Array.from(newBcc)); + setShowBcc(true); + }Committable suggestion skipped: line range outside the PR's diff.
apps/mail/app/api/driver/google.ts (2)
30-33: 🛠️ Refactor suggestion
Add missing Base-64 padding before decoding
base64-jsthrowsInvalidLengthErrorif the input length is not a multiple of four. Gmail’s URL-safe payloads frequently come without the=padding, sofromBinarywill intermittently blow up on real-world messages.-const bytes = toByteArray(str.replace(/-/g, '+').replace(/_/g, '/')); +const normalized = str.replace(/-/g, '+').replace(/_/g, '/'); +const padded = normalized + '='.repeat((4 - (normalized.length % 4)) % 4); +const bytes = toByteArray(padded);This keeps the helper deterministic and prevents sporadic “invalid encoding” crashes.
📝 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.function fromBinary(str: string) { const normalized = str.replace(/-/g, '+').replace(/_/g, '/'); const padded = normalized + '='.repeat((4 - (normalized.length % 4)) % 4); const bytes = toByteArray(padded); return new TextDecoder().decode(bytes); }
688-690:
⚠️ Potential issueRemove hard-coded debug logging
console.log('decodedBody', bodyData);for a single thread ID will leak noisy logs in production and potentially expose user data.-if (id === '196784c9e42c15cb') { - console.log('decodedBody', bodyData); -}📝 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.apps/mail/components/context/thread-context.tsx (2)
131-136:
⚠️ Potential issue
toast.promiseignores thefinallykey – cleanup never runs
sonner’stoast.promiseaccepts onlyloading | success | error.
Thefinallycallback here is silently ignored, so the background-queue never drains and SWR never revalidates.Refactor to:
toast.promise(promise, { loading: t('common.actions.moving'), success: t('common.actions.moved'), error: t('common.actions.failedToMove'), }).finally(async () => { await Promise.all([mutate(), mutateStats()]); setMail({ ...mail, bulkSelected: [] }); targets.forEach((threadId) => setBackgroundQueue({ type: 'delete', threadId })); });Apply the same fix to
handleReadUnread.
99-104:
⚠️ Potential issue
isStarredcan throw whenthreadDatais undefined
threadData?.messages.some(...)calls.someonundefinedwhen the data is not yet loaded, leading to a runtime error.-return threadData?.messages.some((message) => +return threadData?.messages?.some((message) => message.tags?.some((tag) => tag.name.toLowerCase() === 'starred'), );apps/mail/components/create/ai-chat.tsx (1)
3-3:
⚠️ Potential issueIncorrect icon component names – build will fail
lucide-reactexportsArrowUp,Check,X(and so on) – not the*Iconsuffixed variants. Importing the wrong identifiers will raise compile-time errors.-import { ArrowUpIcon, Mic, CheckIcon, XIcon, Plus, Command } from 'lucide-react'; +import { ArrowUp, Mic, Check, X, Plus, Command } from 'lucide-react';Update the corresponding JSX usages (
<Check />,<X />,<ArrowUp />…).📝 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.-import { ArrowUpIcon, Mic, CheckIcon, XIcon, Plus, Command } from 'lucide-react'; +import { ArrowUp, Mic, Check, X, Plus, Command } from 'lucide-react';apps/mail/components/mail/mail.tsx (2)
32-35:
⚠️ Potential issueUnused imports – will fail
noUnusedLocals/eslintrules
FilterandXare no longer referenced (the only usages are commented-out).
Leaving them imported breaks the build ifstrictlint rules are enabled. Simply remove the names or the whole import line.
262-267:
⚠️ Potential issueMissing dependency in
handleClosecallback
handleClosecalls bothsetThreadIdandsetActiveReplyIdbut only listssetThreadIdin its dependency array.
If the setter returned byuseQueryStateever changes (e.g. during HMR) the callback will capture a stale reference.-const handleClose = useCallback(() => { - setThreadId(null); - setActiveReplyId(null); -}, [setThreadId]); +const handleClose = useCallback(() => { + setThreadId(null); + setActiveReplyId(null); +}, [setThreadId, setActiveReplyId]);apps/mail/components/mail/mail-list.tsx (2)
464-514: 🛠️ Refactor suggestion
Quick-action toolbar accessibility & pointer-events
The floating toolbar is absolutely positioned with
top-[-1]whenindex !== 0.
Negative pixel values can be brittle; consider usinginset-y-1or a relative transform so the toolbar never overlaps neighbouring rows.Buttons inside the hover-toolbar are the only focusable items rendered while it’s visible.
For keyboard / screen-reader users the toolbar is unreachable because it is conditionally rendered only on mouse-hover.
Consider keeping the toolbar in the DOM withopacity-0and togglingpointer-events/opacityinstead so it can still participate in the tab order, or expose the same actions via the context menu.
16-18:
⚠️ Potential issue
Users,Briefcase,StickyNote, andStaricons are used but never imported
Components referenced further down (<Users/>,<Briefcase/>,<StickyNote/>,<Star/>) will throwReferenceErrorat compile-time. Add the missing imports, e.g.:-import { Bell, GroupPeople, Lightning, People, Tag, User, Star2, Archive, Trash, Archive2, ChevronDown } from '../icons/icons'; +import { + Bell, + GroupPeople, + Lightning, + People, + Tag, + User, + Users, + Briefcase, + StickyNote, + Star, + Star2, + Archive, + Trash, + Archive2, + ChevronDown, +} from '../icons/icons';📝 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.-import { Bell, GroupPeople, Lightning, People, Tag, User, Star2, Archive, Trash, Archive2, ChevronDown } from '../icons/icons'; +import { + Bell, + GroupPeople, + Lightning, + People, + Tag, + User, + Users, + Briefcase, + StickyNote, + Star, + Star2, + Archive, + Trash, + Archive2, + ChevronDown, +} from '../icons/icons';apps/mail/lib/prompts.ts (2)
158-168:
⚠️ Potential issueEscape user-supplied context before embedding into XML
currentSubject,currentDraft, and every element ofrecipientsare interpolated verbatim into the XML prompt.
Because these strings come from user input they can legally contain<,&,", etc. – characters that break the XML and leak prompt-injection vectors.- const currentSubjectContent = currentSubject ? `\n\n<current_subject>${currentSubject}</current_subject>\n\n` : ''; - const currentDraftContent = currentDraft ? `\n\n<current_draft>${currentDraft}</current_draft>\n\n` : ''; - const recipientsContent = recipients ? `\n\n<recipients>${recipients.join(', ')}</recipients>\n\n` : ''; + const currentSubjectContent = currentSubject + ? `\n\n<current_subject>${escapeXml(currentSubject)}</current_subject>\n\n` + : ''; + const currentDraftContent = currentDraft + ? `\n\n<current_draft>${escapeXml(currentDraft)}</current_draft>\n\n` + : ''; + const recipientsContent = recipients + ? `\n\n<recipients>${recipients.map(escapeXml).join(', ')}</recipients>\n\n` + : '';Failing to escape these fields can mangle the prompt and allow the user to smuggle instructions that override your system rules.
Please sanitize before release.Also applies to: 170-176
178-183: 🛠️ Refactor suggestion
Escape & minify the injected JSON profile
styleProfileJSONis inserted raw. Any&,<,>or quote from future metric keys/values will invalidate the XML.
Additionally the pretty-print (null, 2) inflates the token count (~×3) for no functional gain.- const styleProfileJSON = JSON.stringify(styleProfile, null, 2); + const styleProfileJSON = escapeXml(JSON.stringify(styleProfile));(You can keep indentation if you value readability, but escaping is mandatory.)
📝 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 StyledEmailAssistantSystemPrompt = (userName: string, styleProfile: WritingStyleMatrix) => { const safeName = escapeXml(userName); - const styleProfileJSON = JSON.stringify(styleProfile, null, 2); + const styleProfileJSON = escapeXml(JSON.stringify(styleProfile)); return ` <system_prompt>apps/mail/lib/ai.ts (1)
48-51:
⚠️ Potential issueEnvironment-key mismatch: OPENAI_API_KEY vs Groq usage
generateTextis called withgroq('llama-3.1-8b-instant'), yet the guard still checks forprocess.env.OPENAI_API_KEY.
On a fresh deployment that only configuresGROQ_API_KEYthis branch will throw “OpenAI API key is not configured”.Either:
- if (!process.env.OPENAI_API_KEY) { - throw new Error('OpenAI API key is not configured'); - } + if (!process.env.GROQ_API_KEY) { + throw new Error('Groq API key is not configured'); + }or remove the check entirely if the Groq SDK surfaces its own error.
apps/mail/services/writing-style-service.ts (1)
131-155:
⚠️ Potential issue
takeTopCoveragealways returns an empty object when the first bucket meets the thresholdBecause the predicate uses
< coverage, a first entry that pushesrunning / totalequal to 0.95 fails thetakeWhiletest and nothing is kept.Example: counts = { hello: 95, hi: 5 }, coverage = 0.95
runningbecomes 95 → 95 / 100 = 0.95 not<0.95 ⇒ entry dropped ⇒{}returned.- takeWhile(([_, count]) => { - running += count - return running / total < coverage - }), + takeWhile(([_, count]) => { + running += count + return running / total <= coverage + }),Use
<=(or push the entry before the test) so the most-frequent tokens are retained up to the desired coverage.📝 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.const takeTopCoverage = (data: Record<string, number>, coverage = TAKE_TOP_COVERAGE) => { const total = pipe( data, values(), sum(), ) if (total === 0) { return {} } let running = 0 return pipe( data, entries(), sortBy(([_, count]) => -count), takeWhile(([_, count]) => { running += count return running / total <= coverage }), fromEntries(), ) }apps/mail/components/mail/thread-display.tsx (2)
271-285: 🛠️ Refactor suggestion
Star toggle only updates local state – server remains stale
handleToggleStarflipsisStarredand fires a toast but never persists the change.
On the next refetch the UI will revert becauseemailData.latest.tagsis still the source of truth.- setIsStarred(newStarredState); - if (newStarredState) { + try { + // Persist the change first + await modifyLabels([threadId], { + addLabels: newStarredState ? ['STARRED'] : [], + removeLabels: newStarredState ? [] : ['STARRED'], + }); + + setIsStarred(newStarredState); + if (newStarredState) { toast.custom((id) => <SuccessEmailToast message={t('common.actions.addedToFavorites')} />); - } else { + } else { toast.custom((id) => ( <SuccessEmailToast message={t('common.actions.removedFromFavorites')} /> )); - } - mutateThreads(); + } + mutateThreads(); + } catch { + toast.error(t('common.actions.failedToMove')); // Re-use an existing i18n key + }[pflag_critical_issue]
📝 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.// Add handleToggleStar function const handleToggleStar = useCallback(async () => { if (!emailData || !threadId) return; const newStarredState = !isStarred; try { // Persist the change first await modifyLabels([threadId], { addLabels: newStarredState ? ['STARRED'] : [], removeLabels: newStarredState ? [] : ['STARRED'], }); setIsStarred(newStarredState); if (newStarredState) { toast.custom((id) => <SuccessEmailToast message={t('common.actions.addedToFavorites')} />); } else { toast.custom((id) => ( <SuccessEmailToast message={t('common.actions.removedFromFavorites')} /> )); } mutateThreads(); } catch { toast.error(t('common.actions.failedToMove')); // Re-use an existing i18n key } }, [emailData, threadId, isStarred, mutateThreads, t]);
240-266: 🛠️ Refactor suggestion
Background queue entry is never cleaned up
You enqueue the thread (
setBackgroundQueue({ type: 'add', … })) but the counterpart delete call is commented out.
If the request fails – or even succeeds – the queue will grow indefinitely, breaking the visual “in-progress” cues.Add the deletion in both
finallyandcatchblocks:finally: async () => { await Promise.all([mutateStats(), mutateThreads()]); - // setBackgroundQueue({ type: 'delete', threadId: `thread:${threadId}` }); + setBackgroundQueue({ type: 'delete', threadId: `thread:${threadId}` }); }, + error: () => { + setBackgroundQueue({ type: 'delete', threadId: `thread:${threadId}` }); + },Committable suggestion skipped: line range outside the PR's diff.
apps/mail/components/mail/mail-display.tsx (2)
151-214: 🛠️ Refactor suggestion
MailDisplayLabelsstill expectsstring[], losing colour & id informationEverywhere else labels are now objects (
{ id, name }).
Accepting plain strings forces upstream callers tomap(t => t.name)and dropsid, which will bite when you need it for actions (e.g. filter by label).Refactor signature to
labels: { id: string; name: string }[]and adjust mapping.
734-774:
⚠️ Potential issueLarge attachments decoded in the main thread
atob+ array allocation copies the entire file into memory; for multi-MB files this freezes the UI.
Prefer the Streams API:- const byteCharacters = atob(attachment.body); - … - const blob = new Blob([byteArray], { type: attachment.mimeType }); + const resp = await fetch(`data:${attachment.mimeType};base64,${attachment.body}`); + const blob = await resp.blob();This keeps parsing in the browser’s decoding pipeline and is far more memory-efficient.
Committable suggestion skipped: line range outside the PR's diff.
apps/mail/app/api/driver/microsoft.ts (3)
235-249:
⚠️ Potential issue
parseOutgoingis built but never usedYou build a raw MIME message (
parseOutgoing) yet discard it, sending Graph’s JSON payload instead.
Either:
- Remove the expensive call, or
- Switch to
/me/sendMail’ssaveToSentItems=truewith the raw MIME inbinarycontent to preserve headers.Current form wastes CPU and slows sends.
419-431:
⚠️ Potential issue
modifyLabelsignoresremoveLabelsand may duplicate categoriesGraph patches replace the entire
categoriesarray.
If you only sendaddLabels, previously-assigned labels linger; duplicates are also possible.- categories: options.addLabels, + categories: [ + ...new Set([ + ...(message.categories ?? []).filter((c) => !options.removeLabels.includes(c)), + ...options.addLabels, + ]), + ],Make sure to fetch the current categories first or include them in the request body.
Committable suggestion skipped: line range outside the PR's diff.
305-310: 🛠️ Refactor suggestion
Hard-coded
.top(3)ignoresmaxResults
list()promisesmaxResultsbut always returns three items.
Replace with:- .top(3) + .top(maxResults)Also bump the
skipcalculation when you re-enable pagination.📝 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.const response = await client .api('/me/messages') // .filter(searchQuery) - .top(3) + .top(maxResults) // .skip(pageToken ? parseInt(pageToken.toString()) : 0) .get();
2 participants
when you hover, toolbar
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Chores
Documentation
Style
Tests