Conversation
- added cc and bcc when saving drafts - save drafts less aggresively
chore: simplify and fix the dev env
* Create prompts with XML formatting * Include XML formatted prompts in generate func * remove unused regex and add helper functions/warnings * error handling * Update apps/mail/lib/prompts.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * lint issues * Update prompts.ts * #706 (comment) Coderabbit fix 1 * erabbitai bot 3 days ago⚠️ Potential issue errorOccurred state is stale inside finally React state setters (setErrorOccurred) are asynchronous; the errorOccurred value captured at render time will not yet reflect changes made earlier in the same event loop. Consequently, the logic deciding whether to collapse/expand may run with an outdated flag. - } finally { - setIsLoading(false); - if (!errorOccurred || isAskingQuestion) { - setIsExpanded(true); - } else { - setIsExpanded(false); // Collapse on errors - } - } + } finally { + setIsLoading(false); + // Use a local flag to track errors deterministically + const hadError = isAskingQuestion ? false : !!errorFlagRef.current; + setIsExpanded(!hadError); + } You can create const errorFlagRef = useRef(false); and update errorFlagRef.current = true every time an error is detected, ensuring reliable behaviour irrespective of React batching. Committable suggestion skipped: line range outside the PR's diff. * #706 (comment) * #706 (comment) * #706 (comment) --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…users (#726) * feat(i18n): add Vietnamese language support Add Vietnamese ('vi') to the list of supported languages in the i18n configuration and JSON file to expand language options. * Add a new Vietnamese translation file to support Vietnamese language users. * Clear Vietnamese translation strings
Co-authored-by: needle <122770437+needleXO@users.noreply.github.com>
* Updated lockfile * Updated home page session validation --------- Co-authored-by: Adam <x_1337@outlook.com>
* Create route og image * resolve coderabbit nitpicks --------- Co-authored-by: Adam <x_1337@outlook.com>
…ror messages instead of throwing unauthorized errors. This improves readability and maintains functionality across various actions.
* Adds labels from the provider: - labels page in settings - labels page translations - added labels for google provider * useLabels and labels in sidebar * fix rate limit parts of labels route * Added labels to mail-list * - add rate limiting - move useThreadsLabel --------- Co-authored-by: Adam <x_1337@outlook.com>
- updates lib/auth.ts to use the new method - updates actions/user.ts - updates app/(routes)/settings/danger-zone/page.tsx
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@MrgSub has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis update introduces comprehensive label management and account deletion features to the mail application. New API routes and hooks enable CRUD operations for user-defined email labels, integrating with the Gmail API for label synchronization. The settings UI is expanded with dedicated pages for label management and a "Danger Zone" section for account deletion, including confirmation dialogs and internationalization support. Navigation and configuration files are updated to reflect these new sections. Additionally, utility enhancements include color palette resources for labels and improved handling of OAuth token revocation during user deletion, ensuring thorough cleanup of user data. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant API
participant GoogleDriver
participant Database
User->>SettingsPage: Open "Labels" or "Danger Zone"
SettingsPage->>API: Fetch labels / Initiate account deletion
API->>GoogleDriver: Perform label CRUD / Revoke token
GoogleDriver-->>API: Return result (labels, status)
API-->>SettingsPage: Return data/status
SettingsPage-->>User: Show labels UI / Show deletion confirmation
User->>SettingsPage: Create/Update/Delete label or Confirm deletion
SettingsPage->>API: Send label mutation / Deletion request
API->>GoogleDriver: Update labels / Revoke token
GoogleDriver-->>API: Success/Failure
API->>Database: Clean up user data (on deletion)
Database-->>API: Success/Failure
API-->>SettingsPage: Show result (success/error)
SettingsPage-->>User: Display toast/redirect
Suggested labels
Suggested reviewers
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:
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 (
|
…mponent - Added posthog-js version 1.236.6 to package.json and bun.lock. - Introduced search functionality by implementing handleFilterByLabel in NavMain component. - Updated NavItem to trigger label filtering on click.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
apps/mail/config/navigation.ts (1)
77-82: 🛠️ Refactor suggestionInconsistency between ID and displayed text
The item ID has been changed to 'trash', but the title still uses 'navigation.sidebar.bin' and the URL is still '/mail/bin'. This creates an inconsistency that might cause confusion in the codebase.
Consider updating both the title and URL to match the new 'trash' ID for consistency:
{ id: 'trash', - title: 'navigation.sidebar.bin', - url: '/mail/bin', + title: 'navigation.sidebar.trash', + url: '/mail/trash', icon: DeleteIcon, disabled: false, },
🧹 Nitpick comments (25)
apps/mail/lib/constants.ts (1)
113-130: Good implementation of Gmail color palette with consistent formattingThe
GMAIL_COLORSconstant provides a comprehensive set of color pairs that match Gmail's label color options. This will ensure visual consistency when synchronizing with Gmail labels.One minor suggestion would be to add a brief comment explaining the purpose of this palette and how it differs from the larger
COLORSpalette defined elsewhere.+// Color palette matching Gmail's label colors for consistency with Gmail API integration export const GMAIL_COLORS: GmailColor[] = [ { textColor: '#000000', backgroundColor: '#E2E2E2' }, { textColor: '#D50000', backgroundColor: '#F28B82' },apps/mail/app/(routes)/settings/labels/colors.ts (1)
1-104: Comprehensive color palette implementationThe extensive color palette (104 colors) provides users with a rich set of options for label customization. This is well-structured as a standalone module that can be imported where needed.
Consider adding some grouping or categorization for these colors to help with organization and potentially to improve the user experience in the color selection UI.
+// Group colors by shades/categories for better organization +export const COLOR_CATEGORIES = { + GRAYSCALE: COLORS.slice(0, 8), + PRIMARY: COLORS.slice(8, 17), + PASTEL: COLORS.slice(17, 26), + // Add more categories as needed +}; export const COLORS = [ '#000000', '#434343',apps/mail/actions/user.ts (1)
6-24: Secure implementation of user deletion with proper validationThe
deleteUserfunction implements appropriate session validation before proceeding with account deletion. The error handling for missing session or user is well implemented.Consider adding more specific error types or codes to help with client-side error handling and internationalization.
export async function deleteUser() { const headersList = await headers(); const session = await auth.api.getSession({ headers: headersList }); - if (!session) return { success: false, error: 'Session not found' }; + if (!session) return { success: false, error: 'Session not found', code: 'AUTH_SESSION_MISSING' }; const userId = session.user?.id; - if (!userId) return { success: false, error: 'User not found' }; + if (!userId) return { success: false, error: 'User not found', code: 'AUTH_USER_MISSING' };apps/mail/components/mail/mail-list.tsx (1)
393-393: Consider removing commented code.The commented-out
MailLabelscomponent should be removed rather than left as a comment if it's no longer needed. Commented code can confuse future developers and create maintenance issues.- {/* <MailLabels labels={threadLabels} /> */}apps/mail/components/mail/render-labels.tsx (1)
1-18: Well-structured label rendering component.The
RenderLabelscomponent follows React best practices:
- It's a functional component with proper props typing
- It uses a hook for data fetching with a sensible default value
- It applies both className-based and inline styles for label appearance
- Each label has a proper key attribute for React reconciliation
One potential improvement would be to handle the empty state or loading state explicitly, though the current implementation with default empty array is sufficient for basic use cases.
Consider adding a loading state indicator and empty state handling:
export const RenderLabels = ({ ids }: { ids: string[] }) => { - const { data: labels = [] } = useThreadLabels(ids); + const { data: labels = [], isLoading } = useThreadLabels(ids); + + if (isLoading) { + return <span className="animate-pulse bg-primary/10 rounded px-1.5 py-0.5 opacity-50"></span>; + } + + if (!labels.length) { + return null; + } return labels.map((label: Label) => ( <span key={label.id} className="bg-primary/10 text-primary rounded px-1.5 py-0.5" style={{ backgroundColor: label.color?.backgroundColor, color: label.color?.textColor }} > {label.name} </span> )); };apps/mail/config/navigation.ts (1)
182-186: Icon inconsistency for danger-zoneThe danger-zone section uses the KeyboardIcon, which doesn't semantically represent a danger or deletion area. This might confuse users about the section's purpose.
Consider using a more appropriate icon that represents danger, such as a shield with an exclamation mark or a warning triangle:
{ title: 'navigation.settings.danger-zone', url: '/settings/danger-zone', - icon: KeyboardIcon, + icon: AlertTriangleIcon, // or another warning-related icon },apps/mail/app/api/v1/thread-labels/route.ts (1)
33-33: Optimize Promise handling for thread labelsThe current implementation uses
awaitinside themapfunction, which doesn't fully leverage Promise.all's concurrency benefits.- const labels = await Promise.all(threadIds.map(async (id) => await driver.getLabel(id))); + const labels = await Promise.all(threadIds.map((id) => driver.getLabel(id)));apps/mail/locales/en.json (1)
414-424: Expand labels localization for UI actions.The localization for the "Labels" section is minimal compared to other settings sections. Consider adding entries for common actions like create, edit, delete, etc. to support full internationalization of the labels management UI.
"labels": { "title": "Labels", "description": "Manage your labels for better organization of your emails." + "create": "Create Label", + "edit": "Edit Label", + "delete": "Delete Label", + "name": "Label Name", + "color": "Color", + "save": "Save Changes", + "cancel": "Cancel", + "noLabels": "No labels created yet. Click the button above to create one.", + "deleteConfirmation": "Are you sure you want to delete this label?", + "loading": "Loading labels..." },apps/mail/app/(routes)/settings/labels/page.tsx (9)
19-31: Verify import usage.The
GMAIL_COLORSconstant is imported but never used in the component. Consider removing this unused import.import { SettingsCard } from '@/components/settings/settings-card'; import { Check, Pencil, Plus, Trash2 } from 'lucide-react'; import { ScrollArea } from '@/components/ui/scroll-area'; import { Separator } from '@/components/ui/separator'; import { Button } from '@/components/ui/button'; - import { GMAIL_COLORS } from '@/lib/constants'; import { Badge } from '@/components/ui/badge'; import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label';
77-84: Use constants for default values consistently.The reset method in handleClose uses hardcoded default values that should be extracted to constants for consistency.
+ const DEFAULT_BG_COLOR = '#E2E2E2'; + const DEFAULT_TEXT_COLOR = '#000000'; + const handleClose = () => { setIsDialogOpen(false); setEditingLabel(null); form.reset({ name: '', - color: { backgroundColor: '#E2E2E2', textColor: '#000000' }, + color: { backgroundColor: DEFAULT_BG_COLOR, textColor: DEFAULT_TEXT_COLOR }, }); };
96-99: Use internationalization for button text.The "Create Label" text is hardcoded instead of using the translation function.
<Button onClick={() => setEditingLabel(null)}> <Plus className="mr-2 h-4 w-4" /> - Create Label + {t('common.mail.createNewLabel')} </Button>
104-105: Use internationalization for dialog title.The dialog title texts are hardcoded instead of using the translation function.
<DialogHeader> - <DialogTitle>{editingLabel ? 'Edit Label' : 'Create New Label'}</DialogTitle> + <DialogTitle>{editingLabel ? t('pages.settings.labels.edit') : t('pages.settings.labels.create')}</DialogTitle> </DialogHeader>
112-116: Internationalize label form fields.Form field labels should use the translation function for internationalization.
<FormItem> - <FormLabel>Label Name</FormLabel> + <FormLabel>{t('pages.settings.labels.name')}</FormLabel> <FormControl> - <Input placeholder="Enter label name" {...field} /> + <Input placeholder={t('pages.settings.labels.namePlaceholder')} {...field} /> </FormControl>
123-123: Internationalize color label.The "Color" label should use the translation function.
- <Label>Color</Label> + <Label>{t('pages.settings.labels.color')}</Label>
156-158: Internationalize button text.Button text should use the translation function.
<Button type="button" variant="outline" onClick={handleClose}> - Cancel + {t('common.actions.cancel')} </Button>
159-161: Internationalize save button text.Save button text should use the translation function.
<Button onClick={form.handleSubmit(onSubmit)}> - {editingLabel ? 'Save Changes' : 'Create Label'} + {editingLabel ? t('common.actions.saveChanges') : t('pages.settings.labels.create')} </Button>
180-182: Internationalize empty state message.The empty state message should use the translation function.
<p className="text-muted-foreground py-4 text-center text-sm"> - No labels created yet. Click the button above to create one. + {t('pages.settings.labels.noLabels')} </p>apps/mail/app/(routes)/settings/danger-zone/page.tsx (3)
40-117: Enhance error handling with more specific error messages.Currently, the error handling only logs the error and shows a generic "Failed to delete account" message. The user would benefit from more specific error information when possible.
Improve the error handling to display more specific error messages:
} catch (error) { console.error('Failed to delete account:', error); - toast.error('Failed to delete account'); + const errorMessage = error instanceof Error ? error.message : 'Failed to delete account'; + toast.error(t('pages.settings.danger-zone.delete_error', { error: errorMessage })); } finally {
49-49: Type assertion is unnecessary and potentially misleading.The type assertion
'' as 'DELETE'is unnecessary and might hide type errors. The empty string doesn't match the literal type 'DELETE'.Remove the type assertion and use a proper empty string:
defaultValues: { - confirmText: '' as 'DELETE', + confirmText: '', },
84-87: Avoid hard-coded colors in favor of design system variables.The text color is hardcoded as
text-red-600 dark:text-red-400which may not follow the design system if colors are updated.Consider using design system variables or a more abstract class for destructive colors:
- <div className="border-destructive/50 bg-destructive/10 flex items-center gap-2 rounded-md border px-3 py-2 text-sm text-red-600 dark:text-red-400"> + <div className="border-destructive/50 bg-destructive/10 flex items-center gap-2 rounded-md border px-3 py-2 text-sm text-destructive"> <AlertTriangle className="h-4 w-4" /> <span>{t('pages.settings.danger-zone.warning')}</span> </div>apps/mail/hooks/use-labels.ts (3)
15-18: The fetcher function could benefit from error handling.The current fetcher function doesn't handle errors properly, which may lead to unclear error messages.
Improve the fetcher function with better error handling:
const fetcher = async (url: string) => { - const response = await axios.get(url); - return response.data; + try { + const response = await axios.get(url); + return response.data; + } catch (error) { + const errorMessage = error instanceof Error ? error.message : 'Unknown error'; + console.error(`Failed to fetch from ${url}:`, errorMessage); + throw error; + } };
104-132: Configure error retry strategy in SWR.The current SWR configuration disables revalidation on focus and reconnect but doesn't configure an error retry strategy, which might lead to excessive retries on failure.
Add explicit error retry configuration:
return useSWR<Label[]>( key, async (url) => { try { const response = await fetch(url, { method: 'GET', headers: { 'Content-Type': 'application/json' }, }); if (!response.ok) { throw new Error('Failed to fetch thread labels'); } return response.json(); } catch (error) { toast.error('Failed to fetch thread labels'); throw error; } }, { revalidateOnFocus: false, revalidateOnReconnect: false, dedupingInterval: 5000, + onErrorRetry: (error, key, config, revalidate, { retryCount }) => { + // Only retry up to 3 times + if (retryCount >= 3) return; + + // Retry after 5 seconds + setTimeout(() => revalidate({ retryCount }), 5000); + }, }, );
36-36: Consider using{ revalidate: false }for SWR mutate.When using
mutatewith SWR, using the object syntax is preferred over the boolean parameter for better type safety and future compatibility.Update the mutate call:
- await mutate([...(labels || []), newLabel], false); + await mutate([...(labels || []), newLabel], { revalidate: false });Also make similar changes in other mutate calls in the file.
apps/mail/app/api/v1/labels/route.ts (1)
18-21: Rate limit window value may be unnecessarily high.The current rate limit allows 60 requests per minute per user, which seems excessive for label management operations. Consider a lower limit to prevent API abuse.
Adjust the rate limit to a more reasonable value:
const ratelimit = getRatelimitModule({ prefix: `ratelimit:get-labels-${userId}`, - limiter: Ratelimit.slidingWindow(60, '1m'), + limiter: Ratelimit.slidingWindow(20, '1m'), });Apply similar adjustments to other rate limit configurations in this file.
apps/mail/app/api/driver/google.ts (1)
988-1004: Handle potential edge cases in createLabel method.The
createLabelmethod doesn't validate the label name, which could lead to issues if an invalid label name is provided. Gmail has restrictions on label names that should be enforced.Add validation for the label name before creating:
createLabel: async (label) => { + if (!label.name || label.name.trim() === '') { + throw new Error('Label name cannot be empty'); + } + + // Gmail doesn't allow certain characters in label names + if (/[/\\]/.test(label.name)) { + throw new Error('Label name cannot contain slashes'); + } const res = await gmail.users.labels.create({ userId: 'me', requestBody: { name: label.name, labelListVisibility: 'labelShow', messageListVisibility: 'show', color: label.color ? { backgroundColor: label.color.backgroundColor, textColor: label.color.textColor, } : undefined, }, }); return res.data; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/mail/actions/user.ts(1 hunks)apps/mail/app/(routes)/settings/[...settings]/page.tsx(2 hunks)apps/mail/app/(routes)/settings/danger-zone/page.tsx(1 hunks)apps/mail/app/(routes)/settings/labels/colors.ts(1 hunks)apps/mail/app/(routes)/settings/labels/page.tsx(1 hunks)apps/mail/app/api/driver/google.ts(4 hunks)apps/mail/app/api/driver/types.ts(1 hunks)apps/mail/app/api/v1/labels/route.ts(1 hunks)apps/mail/app/api/v1/thread-labels/route.ts(1 hunks)apps/mail/components/create/create-email.tsx(2 hunks)apps/mail/components/mail/mail-list.tsx(3 hunks)apps/mail/components/mail/render-labels.tsx(1 hunks)apps/mail/components/settings/settings-card.tsx(2 hunks)apps/mail/components/ui/nav-main.tsx(3 hunks)apps/mail/config/navigation.ts(3 hunks)apps/mail/hooks/use-labels.ts(1 hunks)apps/mail/hooks/use-threads.ts(2 hunks)apps/mail/lib/auth.ts(2 hunks)apps/mail/lib/constants.ts(1 hunks)apps/mail/locales/en.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/mail/app/(routes)/settings/[...settings]/page.tsx (1)
apps/mail/app/(routes)/settings/labels/page.tsx (1)
LabelsPage(33-217)
apps/mail/actions/user.ts (2)
packages/db/src/schema.ts (1)
session(19-30)apps/mail/lib/auth.ts (1)
auth(327-330)
apps/mail/components/create/create-email.tsx (2)
apps/mail/components/ui/dropdown-menu.tsx (4)
DropdownMenu(171-171)DropdownMenuTrigger(172-172)DropdownMenuContent(173-173)DropdownMenuItem(174-174)apps/mail/components/ui/button.tsx (1)
Button(55-55)
apps/mail/components/settings/settings-card.tsx (1)
apps/mail/components/ui/card.tsx (3)
CardHeader(55-55)CardTitle(55-55)CardDescription(55-55)
apps/mail/components/mail/mail-list.tsx (1)
apps/mail/components/mail/render-labels.tsx (1)
RenderLabels(6-18)
apps/mail/lib/auth.ts (4)
packages/db/src/schema.ts (4)
user(7-17)connection(77-94)account(32-48)session(19-30)apps/mail/app/api/driver/google.ts (1)
driver(195-1042)apps/mail/actions/utils.ts (1)
getActiveDriver(33-63)packages/db/src/index.ts (1)
db(17-17)
apps/mail/app/api/driver/google.ts (1)
apps/mail/lib/auth.ts (1)
auth(327-330)
🔇 Additional comments (22)
apps/mail/lib/constants.ts (1)
108-111: Well-defined interface for Gmail colorsThe
GmailColorinterface clearly defines the structure for color pairs used in Gmail labels. This provides good type safety for the color definitions that follow.apps/mail/app/(routes)/settings/[...settings]/page.tsx (2)
11-11: Added import for new labels page componentGood addition of the import for the
LabelsPagecomponent, maintaining consistent import style with other settings pages.
20-20: Registration of labels page in settings navigationThe
LabelsPagecomponent is correctly registered in thesettingsPagesmapping with the key 'labels', allowing it to be accessed via the settings route system.apps/mail/actions/user.ts (1)
1-5: Well-structured server action setupThe 'use server' directive correctly marks this as a server action, and the imports are appropriate for the functionality.
apps/mail/components/mail/mail-list.tsx (3)
47-47: Good addition of RenderLabels component import.The import of the new RenderLabels component is aligned with the label management feature being introduced.
50-50: Label type import looks correct.Adding the Label type import from the use-labels hook provides proper type checking for the label-related functionality.
361-363: Good implementation of the new label rendering approach.Replacing the inline label rendering with the dedicated
RenderLabelscomponent improves code organization and maintenance. The space-x-2 class ensures proper spacing between multiple labels.apps/mail/components/create/create-email.tsx (2)
610-649: Good UX improvement with conditional From field rendering.Conditionally rendering the "From" email dropdown only when multiple aliases are available (
aliases.length > 1) improves UI clarity by hiding unnecessary UI elements when they don't provide value to the user.
787-787: Good enhancement for attachment display.Adding the
max-h-[40vh]class to limit the maximum height of the attachments container provides better user experience by enabling vertical scrolling for a large number of attachments instead of extending the popover indefinitely.apps/mail/hooks/use-threads.ts (1)
147-175: Well-implemented useThreadLabels hook.The new hook provides a clean way to fetch label data for a list of thread IDs. The implementation includes:
- Proper error handling with toast notifications for a better user experience
- SWR configuration with appropriate caching settings (no revalidation on focus/reconnect)
- Input validation by only making the request when there are IDs to fetch
A minor optimization could be to add a condition to skip the fetch if the ids array is empty, but the current implementation handles this by setting the key to null.
apps/mail/config/navigation.ts (2)
158-158: Security section is now enabledThe security settings page is now accessible to users by commenting out the disabled property.
165-169: New labels settings page addedThe addition of the labels navigation item is well-structured and consistent with other settings items.
apps/mail/lib/auth.ts (2)
88-107: Well-implemented user deletion flow with proper cleanupThe implementation properly handles the user deletion flow with comprehensive cleanup of user data across multiple tables.
The code correctly:
- Retrieves the active driver
- Fetches and revokes the user's refresh token
- Cleans up associated data from all relevant tables (account, session, connection, user)
- Has appropriate error handling
96-100:Details
❓ Verification inconclusive
Verify token revocation resilience
The implementation currently stops the deletion process if token revocation fails. Consider whether the user deletion should proceed even if token revocation fails.
🏁 Script executed:
#!/bin/bash # Analyze error handling in other auth deletion flows rg -A 5 -B 5 "delete.*user|account.*delete" --glob "*.ts" | grep -v "node_modules"Length of output: 4388
Consider proceeding with DB cleanup even if token revocation fails
The current flow aborts all downstream deletes when
driver.revokeRefreshToken(...)returnsfalse:const revoked = await driver.revokeRefreshToken(refreshToken || '') if (!revoked) { console.error('Failed to revoke refresh token') return }That means if revocation fails (due to network hiccup or third-party downtime), none of the following cleanup steps run:
- delete account
- delete sessions
- delete connections
- delete user record
Unless you intend to fully roll back user deletion on revoke failure, you’ll end up with orphaned data. Consider instead logging the failure (or surfacing it) but continuing with the rest of the deletion, or explicitly documenting that revoke errors should block all cleanup.
• Location:
apps/mail/lib/auth.tslines 96–100apps/mail/components/settings/settings-card.tsx (1)
9-9: Well-implemented action prop for SettingsCardThe addition of the optional
actionprop enhances the component's flexibility, allowing for contextual actions to be displayed in the card header.The implementation:
- Properly types the action prop
- Uses flexbox for appropriate layout
- Maintains backward compatibility with existing usage
Also applies to: 17-17, 27-33
apps/mail/app/api/v1/thread-labels/route.ts (4)
7-21: Well-implemented authentication and rate limitingThe API route correctly implements authentication and rate limiting with appropriate error responses.
22-31: Proper request validationThe implementation correctly validates the presence of thread IDs in the request parameters.
35-45: Type safety for filtered labelsThe implementation correctly uses a type predicate to ensure type safety for the filtered labels.
46-50: Complete error handlingThe error handling is appropriate with proper logging and error responses.
apps/mail/components/ui/nav-main.tsx (1)
64-64: Good addition of labels hook.The
useLabelshook integration enables dynamic label fetching for the navigation sidebar.apps/mail/locales/en.json (1)
299-300: Navigation items added correctly.The new navigation entries for "Labels" and "Danger Zone" settings are properly added to the localization file.
apps/mail/app/api/driver/types.ts (1)
51-51: Good addition of token revocation method.The
revokeRefreshTokenmethod with a boolean return type for success/failure is appropriate for token lifecycle management.
| const { success, message } = await auth.api.deleteUser({ | ||
| body: { | ||
| callbackURL: '/', | ||
| }, | ||
| headers: headersList, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify redirection after account deletion
The implementation uses a root path ('/') as the callbackURL for redirecting after account deletion. Ensure this is the intended behavior and consider whether a more specific landing page or confirmation page would be more appropriate.
🏁 Script executed:
#!/bin/bash
# Check if there's a specific landing page for post-deletion in the codebase
# Look for any references to post-deletion pages or routes
echo "Checking for post-deletion routes or pages:"
rg -i "after.*delet|post.*delet|account.*removed" --type ts --type tsxLength of output: 197
🏁 Script executed:
#!/bin/bash
# Search for any pages/components related to deletion confirmation
echo "Searching for TS/TSX files with ‘delete’ in their name:"
find . -type f \( -iname "*delete*.ts" -o -iname "*delete*.tsx" \)
echo
echo "Searching for all occurrences of callbackURL:"
rg "callbackURL" -nLength of output: 618
Ensure post-deletion redirect
The call in apps/mail/actions/user.ts (line 18) uses callbackURL: '/'. Currently there’s no dedicated “account deleted” or confirmation page in the codebase—other flows send users to /settings/connections (email verify) or /mail (login). Please update this to a more appropriate route or create a confirmation page, for example:
- Create
/account/deleted(or/goodbye) and setcallbackURLto that. - Or ensure the homepage (
/) clearly displays a post-deletion message.
| {!pathname.includes('/settings') && ( | ||
| <Collapsible defaultOpen={true} className="group/collapsible"> | ||
| <SidebarMenuItem className="mb-4" style={{ height: 'auto' }}> | ||
| <CollapsibleTrigger asChild> | ||
| <SidebarMenuButton>Labels</SidebarMenuButton> | ||
| </CollapsibleTrigger> | ||
| <CollapsibleContent style={{ height: 'auto' }}> | ||
| <SidebarMenuSub style={{ height: 'auto' }} className="mr-0 pr-0"> | ||
| <div className="space-y-1 pb-2" style={{ height: 'auto' }}> | ||
| {labels.map((label) => ( | ||
| <NavItem | ||
| key={label.id} | ||
| title={label.name} | ||
| href={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`} | ||
| icon={() => ( | ||
| <div | ||
| className="size-4 rounded-md" | ||
| style={{ backgroundColor: label.color?.backgroundColor || '#E2E2E2' }} | ||
| /> | ||
| )} | ||
| url={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`} | ||
| /> | ||
| ))} | ||
| </div> | ||
| </SidebarMenuSub> | ||
| </CollapsibleContent> | ||
| </SidebarMenuItem> | ||
| </Collapsible> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify NavItem props and add empty state handling.
The implementation of the collapsible Labels section is good, but has a few areas for improvement:
- There's duplication in the URL definition - both
hrefandurlprops contain identical values - There's no handling for an empty labels array or loading state
- Multiple inline
style={{ height: 'auto' }}declarations could be consolidated with a CSS class
{!pathname.includes('/settings') && (
<Collapsible defaultOpen={true} className="group/collapsible">
<SidebarMenuItem className="mb-4" style={{ height: 'auto' }}>
<CollapsibleTrigger asChild>
<SidebarMenuButton>Labels</SidebarMenuButton>
</CollapsibleTrigger>
<CollapsibleContent style={{ height: 'auto' }}>
<SidebarMenuSub style={{ height: 'auto' }} className="mr-0 pr-0">
<div className="space-y-1 pb-2" style={{ height: 'auto' }}>
- {labels.map((label) => (
+ {labels.length === 0 ? (
+ <p className="text-muted-foreground px-2 py-1 text-sm">No labels available</p>
+ ) : (
+ labels.map((label) => (
<NavItem
key={label.id}
title={label.name}
- href={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`}
+ href={`/mail/inbox?q=${encodeURIComponent(label.name.toLowerCase())}`}
icon={() => (
<div
className="size-4 rounded-md"
style={{ backgroundColor: label.color?.backgroundColor || '#E2E2E2' }}
/>
)}
- url={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`}
+ url={`/mail/inbox?q=${encodeURIComponent(label.name.toLowerCase())}`}
/>
- ))}
+ )))}
</div>
</SidebarMenuSub>
</CollapsibleContent>
</SidebarMenuItem>
</Collapsible>
)}📝 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.
| {!pathname.includes('/settings') && ( | |
| <Collapsible defaultOpen={true} className="group/collapsible"> | |
| <SidebarMenuItem className="mb-4" style={{ height: 'auto' }}> | |
| <CollapsibleTrigger asChild> | |
| <SidebarMenuButton>Labels</SidebarMenuButton> | |
| </CollapsibleTrigger> | |
| <CollapsibleContent style={{ height: 'auto' }}> | |
| <SidebarMenuSub style={{ height: 'auto' }} className="mr-0 pr-0"> | |
| <div className="space-y-1 pb-2" style={{ height: 'auto' }}> | |
| {labels.map((label) => ( | |
| <NavItem | |
| key={label.id} | |
| title={label.name} | |
| href={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`} | |
| icon={() => ( | |
| <div | |
| className="size-4 rounded-md" | |
| style={{ backgroundColor: label.color?.backgroundColor || '#E2E2E2' }} | |
| /> | |
| )} | |
| url={`/mail/inbox?q=${encodeURIComponent(label.name.toLocaleLowerCase())}`} | |
| /> | |
| ))} | |
| </div> | |
| </SidebarMenuSub> | |
| </CollapsibleContent> | |
| </SidebarMenuItem> | |
| </Collapsible> | |
| )} | |
| {!pathname.includes('/settings') && ( | |
| <Collapsible defaultOpen={true} className="group/collapsible"> | |
| <SidebarMenuItem className="mb-4" style={{ height: 'auto' }}> | |
| <CollapsibleTrigger asChild> | |
| <SidebarMenuButton>Labels</SidebarMenuButton> | |
| </CollapsibleTrigger> | |
| <CollapsibleContent style={{ height: 'auto' }}> | |
| <SidebarMenuSub style={{ height: 'auto' }} className="mr-0 pr-0"> | |
| <div className="space-y-1 pb-2" style={{ height: 'auto' }}> | |
| {labels.length === 0 ? ( | |
| <p className="text-muted-foreground px-2 py-1 text-sm"> | |
| No labels available | |
| </p> | |
| ) : ( | |
| labels.map((label) => ( | |
| <NavItem | |
| key={label.id} | |
| title={label.name} | |
| href={`/mail/inbox?q=${encodeURIComponent( | |
| label.name.toLowerCase() | |
| )}`} | |
| icon={() => ( | |
| <div | |
| className="size-4 rounded-md" | |
| style={{ | |
| backgroundColor: | |
| label.color?.backgroundColor || '#E2E2E2', | |
| }} | |
| /> | |
| )} | |
| url={`/mail/inbox?q=${encodeURIComponent( | |
| label.name.toLowerCase() | |
| )}`} | |
| /> | |
| )) | |
| )} | |
| </div> | |
| </SidebarMenuSub> | |
| </CollapsibleContent> | |
| </SidebarMenuItem> | |
| </Collapsible> | |
| )} |
| getUserLabels(): Promise<any>; | ||
| getLabel: (labelId: string) => Promise<any>; | ||
| createLabel(label: { | ||
| name: string; | ||
| color?: { backgroundColor: string; textColor: string }; | ||
| }): Promise<any>; | ||
| updateLabel( | ||
| id: string, | ||
| label: { name: string; color?: { backgroundColor: string; textColor: string } }, | ||
| ): Promise<any>; | ||
| deleteLabel(id: string): Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and consistency in interface methods.
The new label management methods are good additions, but there are a few improvements that could be made for type safety and consistency:
- Return types use
Promise<any>, which isn't type-safe - Inconsistent method declaration styles (method syntax vs. property syntax)
- Redundant label type definition in multiple methods
+ interface Label {
+ id?: string;
+ name: string;
+ color?: { backgroundColor: string; textColor: string };
+ }
+
- getUserLabels(): Promise<any>;
- getLabel: (labelId: string) => Promise<any>;
+ getUserLabels(): Promise<Label[]>;
+ getLabel(labelId: string): Promise<Label>;
createLabel(label: {
name: string;
color?: { backgroundColor: string; textColor: string };
- }): Promise<any>;
+ }): Promise<Label>;
updateLabel(
id: string,
label: { name: string; color?: { backgroundColor: string; textColor: string } },
- ): Promise<any>;
+ ): Promise<Label>;
deleteLabel(id: string): Promise<void>;📝 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.
| getUserLabels(): Promise<any>; | |
| getLabel: (labelId: string) => Promise<any>; | |
| createLabel(label: { | |
| name: string; | |
| color?: { backgroundColor: string; textColor: string }; | |
| }): Promise<any>; | |
| updateLabel( | |
| id: string, | |
| label: { name: string; color?: { backgroundColor: string; textColor: string } }, | |
| ): Promise<any>; | |
| deleteLabel(id: string): Promise<void>; | |
| // Define a reusable Label type | |
| interface Label { | |
| id?: string; | |
| name: string; | |
| color?: { backgroundColor: string; textColor: string }; | |
| } | |
| // Updated method signatures | |
| getUserLabels(): Promise<Label[]>; | |
| getLabel(labelId: string): Promise<Label>; | |
| createLabel(label: { | |
| name: string; | |
| color?: { backgroundColor: string; textColor: string }; | |
| }): Promise<Label>; | |
| updateLabel( | |
| id: string, | |
| label: { name: string; color?: { backgroundColor: string; textColor: string } }, | |
| ): Promise<Label>; | |
| deleteLabel(id: string): Promise<void>; |
| <span className="dark:text-whitemix-blend-difference text-black"> | ||
| {label.name} | ||
| </span> |
There was a problem hiding this comment.
Fix incorrect CSS class name.
There's a malformed CSS class name which appears to be a concatenation error.
- <span className="dark:text-whitemix-blend-difference text-black">
+ <span className="dark:text-white mix-blend-difference text-black">
{label.name}
</span>📝 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.
| <span className="dark:text-whitemix-blend-difference text-black"> | |
| {label.name} | |
| </span> | |
| <span className="dark:text-white mix-blend-difference text-black"> | |
| {label.name} | |
| </span> |
| const handleDelete = async (id: string) => { | ||
| try { | ||
| await deleteLabel(id); | ||
| } catch (err) { | ||
| console.error('Error deleting label:', err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add confirmation dialog and user feedback for delete action.
The delete handler lacks confirmation and user feedback.
const handleDelete = async (id: string) => {
+ if (!confirm("Are you sure you want to delete this label?")) {
+ return;
+ }
+
+ // Show loading state
try {
await deleteLabel(id);
+ toast.success("Label deleted successfully");
} catch (err) {
console.error('Error deleting label:', err);
+ toast.error("Failed to delete label");
}
};Committable suggestion skipped: line range outside the PR's diff.
| if (!labels) { | ||
| return NextResponse.json([], { status: 200 }); | ||
| } | ||
| return NextResponse.json(labels.filter((label: Label) => label.type === 'user')); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case where labels is null or undefined more explicitly.
Currently, the code filters labels only if they're not null, but doesn't handle the case where getUserLabels() returns undefined in a clear way.
Ensure consistent handling of null or undefined labels:
const labels = await driver.getUserLabels();
- if (!labels) {
+ if (!labels || !Array.isArray(labels)) {
return NextResponse.json([], { status: 200 });
}
return NextResponse.json(labels.filter((label: Label) => label.type === 'user'));📝 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 (!labels) { | |
| return NextResponse.json([], { status: 200 }); | |
| } | |
| return NextResponse.json(labels.filter((label: Label) => label.type === 'user')); | |
| const labels = await driver.getUserLabels(); | |
| if (!labels || !Array.isArray(labels)) { | |
| return NextResponse.json([], { status: 200 }); | |
| } | |
| return NextResponse.json(labels.filter((label: Label) => label.type === 'user')); |
| const result = await driver?.createLabel(label); | ||
| return NextResponse.json(result); |
There was a problem hiding this comment.
Missing null check before creating a label.
The code doesn't check if the driver is available before attempting to create a label, unlike the GET handler.
Add a null check to prevent potential runtime errors:
const driver = await getActiveDriver();
+ if (!driver) {
+ return NextResponse.json({ error: 'Email driver not configured' }, { status: 500 });
+ }
const result = await driver?.createLabel(label);
return NextResponse.json(result);Apply similar checks in the PATCH and DELETE handlers as well.
📝 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 result = await driver?.createLabel(label); | |
| return NextResponse.json(result); | |
| const driver = await getActiveDriver(); | |
| if (!driver) { | |
| return NextResponse.json( | |
| { error: 'Email driver not configured' }, | |
| { status: 500 } | |
| ); | |
| } | |
| const result = await driver?.createLabel(label); | |
| return NextResponse.json(result); |
| getUserLabels: async () => { | ||
| const res = await gmail.users.labels.list({ | ||
| userId: 'me', | ||
| }); | ||
| return res.data.labels; | ||
| }, |
There was a problem hiding this comment.
Missing error handling in getUserLabels method.
Unlike other methods in this file, getUserLabels doesn't use the withErrorHandler wrapper, which means errors won't be standardized and logged properly.
Add proper error handling to maintain consistency:
- getUserLabels: async () => {
- const res = await gmail.users.labels.list({
- userId: 'me',
- });
- return res.data.labels;
- },
+ getUserLabels: async () => {
+ return withErrorHandler('getUserLabels', async () => {
+ const res = await gmail.users.labels.list({
+ userId: 'me',
+ });
+ return res.data.labels;
+ });
+ },Apply similar changes to the other new label management methods (getLabel, createLabel, updateLabel, deleteLabel).
📝 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.
| getUserLabels: async () => { | |
| const res = await gmail.users.labels.list({ | |
| userId: 'me', | |
| }); | |
| return res.data.labels; | |
| }, | |
| getUserLabels: async () => { | |
| return withErrorHandler('getUserLabels', async () => { | |
| const res = await gmail.users.labels.list({ | |
| userId: 'me', | |
| }); | |
| return res.data.labels; | |
| }); | |
| }, |
| revokeRefreshToken: async (refreshToken: string) => { | ||
| if (!refreshToken) { | ||
| return false; | ||
| } | ||
| try { | ||
| await auth.revokeToken(refreshToken); | ||
| return true; | ||
| } catch (error: any) { | ||
| console.error('Failed to revoke Google token:', error.message); | ||
| return false; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Token revocation implementation could be improved.
The revokeRefreshToken method doesn't follow the error handling pattern used elsewhere in the file. It should use the withErrorHandler pattern for consistency and better error tracking.
Refactor the method to use the established error handling pattern:
- revokeRefreshToken: async (refreshToken: string) => {
- if (!refreshToken) {
- return false;
- }
- try {
- await auth.revokeToken(refreshToken);
- return true;
- } catch (error: any) {
- console.error('Failed to revoke Google token:', error.message);
- return false;
- }
- },
+ revokeRefreshToken: async (refreshToken: string) => {
+ if (!refreshToken) {
+ return false;
+ }
+ return withErrorHandler(
+ 'revokeRefreshToken',
+ async () => {
+ await auth.revokeToken(refreshToken);
+ return true;
+ },
+ { hasRefreshToken: !!refreshToken }
+ ).catch(() => false); // Return false on any error
+ },📝 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.
| revokeRefreshToken: async (refreshToken: string) => { | |
| if (!refreshToken) { | |
| return false; | |
| } | |
| try { | |
| await auth.revokeToken(refreshToken); | |
| return true; | |
| } catch (error: any) { | |
| console.error('Failed to revoke Google token:', error.message); | |
| return false; | |
| } | |
| }, | |
| revokeRefreshToken: async (refreshToken: string) => { | |
| if (!refreshToken) { | |
| return false; | |
| } | |
| return withErrorHandler( | |
| 'revokeRefreshToken', | |
| async () => { | |
| await auth.revokeToken(refreshToken); | |
| return true; | |
| }, | |
| { hasRefreshToken: !!refreshToken } | |
| ).catch(() => false); // Return false on any error | |
| }, |
| const message = data.message.replace(/<br>/g, '</p><p>'); | ||
| const msg = createMimeMessage(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review HTML transformation in createDraft method.
Replacing <br> tags with </p><p> tags might lead to invalid HTML if the original content doesn't have matching paragraph tags or if <br> tags are used within other elements.
Check how this affects different email formats:
🏁 Script executed:
#!/bin/bash
# Let's look for test files or examples that might show how drafts are formatted
echo "Searching for email/draft test files..."
fd -t f "test.*draft|draft.*test" -e ts -e tsx -e js -e jsx
echo "Searching for references to createDraft with HTML content..."
rg "createDraft.*html|createDraft.*message" -A 5 -B 5Length of output: 337
🏁 Script executed:
#!/bin/bash
echo "Locating google.ts..."
fd -t f google.ts
echo "Showing code around lines 900–950 in apps/mail/app/api/driver/google.ts"
sed -n '900,950p' apps/mail/app/api/driver/google.tsLength of output: 1970
Enhance HTML line-break conversion in createDraft
The current replacement
const message = data.message.replace(/<br>/g, '</p><p>');can produce unbalanced or invalid HTML (e.g. leading </p>, missing opening <p>, or breaks inside other tags). Please adjust the logic to ensure well-formed markup (for example, wrap the entire message in <p>…</p> before splitting on </p><p>, or use a small HTML-safe utility to convert line breaks).
Please address in:
- apps/mail/app/api/driver/google.ts around line 916 (inside
createDraft)
Recommended fixes:
- Prepend
<p>and append</p>arounddata.messagebefore replacing<br> - Or tokenize on
\n/<br>and rebuild paragraphs explicitly - Add unit tests covering cases with:
- Messages that already include
<p>tags - Mixed use of
<br>and other HTML elements - Edge cases (empty strings, no breaks)
- Messages that already include
- Updated NavItem to include an onClick prop for the Link component, allowing for custom click behavior. - Maintained existing functionality with prefetch and target attributes.
10 participants
Summary by CodeRabbit
New Features
Improvements
Localization
Bug Fixes