Conversation
…acts-from-recieved-emails
…-emails added contacts dropdown for create email
New create email
relocate settings routing functionality to app-sidebar NavUser
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
""" WalkthroughThis update introduces enhanced contact suggestion and selection features for the email creation form, including keyboard-navigable dropdowns for the "To", "Cc", and "Bcc" fields. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateEmail
participant useContacts
participant DexieStorageProvider
User->>CreateEmail: Types in "To"/"Cc"/"Bcc" field
CreateEmail->>useContacts: Fetch contacts (on mount/session)
useContacts->>DexieStorageProvider: Query cached threads by connectionId
DexieStorageProvider-->>useContacts: Return thread data
useContacts-->>CreateEmail: Return unique contact emails
CreateEmail->>CreateEmail: Filter contacts for suggestions
CreateEmail-->>User: Show dropdown suggestions
User->>CreateEmail: Navigate/select with keyboard
CreateEmail->>CreateEmail: Add selected contact to field
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/mail/hooks/use-contacts.ts (1)
11-30: Efficient contact extraction with deduplicationThe implementation efficiently extracts unique contacts from cached thread data. The use of a Set for tracking seen emails to avoid duplicates is a good approach.
One small improvement could be considering error handling for the network request.
provider.list(`$inf$@"${session?.connectionId}"`).then((cachedThreadsResponses) => { const seen = new Set<string>(); const contacts: Sender[] = cachedThreadsResponses.reduce((acc: Sender[], { state }) => { // ...processing logic return acc; }, []); mutate(contacts); +}).catch(error => { + console.error('Error fetching contacts:', error); + mutate([]); // Ensure we set an empty array on error });apps/mail/components/create/create-email.tsx (1)
953-971: Issue with file input click handlerThe click handler on line 967 could be safer by using optional chaining.
- onClick={() => fileInputRef.current && fileInputRef.current.click()} + onClick={() => fileInputRef.current?.click()}🧰 Tools
🪛 Biome (1.9.4)
[error] 967-967: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/mail/components/create/create-email.tsx(13 hunks)apps/mail/components/home/navbar.tsx(2 hunks)apps/mail/components/ui/app-sidebar.tsx(1 hunks)apps/mail/components/ui/nav-main.tsx(1 hunks)apps/mail/components/ui/nav-user.tsx(4 hunks)apps/mail/hooks/use-contacts.ts(1 hunks)apps/mail/lib/auth.ts(4 hunks)apps/mail/lib/idb.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/mail/hooks/use-contacts.ts (2)
apps/mail/types/index.ts (1)
Sender(32-35)apps/mail/lib/idb.ts (1)
dexieStorageProvider(25-77)
apps/mail/lib/auth.ts (2)
packages/db/src/schema.ts (2)
earlyAccess(59-66)user(7-17)packages/db/src/index.ts (1)
db(17-17)
apps/mail/lib/idb.ts (2)
apps/mail/types/index.ts (1)
InitialThread(71-83)packages/db/src/index.ts (1)
db(17-17)
apps/mail/components/create/create-email.tsx (3)
apps/mail/hooks/use-contacts.ts (1)
useContacts(7-37)apps/mail/components/create/uploaded-file-icon.tsx (1)
UploadedFileIcon(35-67)apps/mail/lib/utils.ts (1)
truncateFileName(116-123)
🪛 Biome (1.9.4)
apps/mail/components/create/create-email.tsx
[error] 967-967: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
apps/mail/components/ui/app-sidebar.tsx (1)
127-127: UI refinement: Improved hover effect transparencyThe change from solid color hovers (
hover:bg-blackanddark:hover:bg-white) to semi-transparent hovers (hover:bg-black/80anddark:hover:bg-white/80) creates a more subtle and refined hover effect, improving the overall visual aesthetics of the compose button.apps/mail/lib/idb.ts (3)
3-3: Added import for thread typeThis import is required for the new list method's type definition.
25-25: Enhanced storage provider return type with list methodThe return type has been extended to include the new
listmethod, which allows retrieving cache entries by a prefix pattern. The type definition is quite specific and detailed, which is good for type safety.
41-43: Added new list method to query IndexedDB by key prefixThe implementation of the list method leverages Dexie's query capabilities to filter cache entries whose keys start with a given prefix. This addition is valuable for efficiently retrieving related cache entries, like contact information.
The implementation is clean and follows the pattern of the other methods in this provider. The error handling is consistent with the existing implementation (errors are caught at the point of use).
apps/mail/components/ui/nav-main.tsx (1)
88-94: Commented out settings button URL behaviorThe conditional logic for modifying URLs with the
fromparameter for settings buttons has been disabled. This change simplifies navigation by removing special URL handling for settings buttons.This change aligns with related navigation improvements in the codebase where more declarative approaches to navigation with Next.js Link components are being used.
apps/mail/components/home/navbar.tsx (2)
76-109: Reactivated mobile navigation containerThe mobile navigation container and its contents have been uncommented, enabling the mobile navigation experience with the Sheet component.
This change provides the necessary responsive navigation for mobile users, improving the overall application accessibility.
111-113: Reactivated desktop navigation containerThe desktop navigation container has been uncommented, enabling the desktop navigation links.
This restores the desktop navigation experience, ensuring consistent navigation across device sizes.
apps/mail/components/ui/nav-user.tsx (3)
33-34: Appropriate imports for the navigation enhancementThe addition of
usePathname,useSearchParams, andLinkare appropriate for implementing declarative navigation in Next.js.
44-58: Well-implemented URL state preservationGood implementation of the
getSettingsHrefcallback. It correctly preserves the current page's category as a "from" parameter in the settings URL, ensuring users can navigate back to their previous context.
247-254: Improved navigation patternGood refactoring from imperative navigation (using
router.push) to declarative navigation with Next.jsLinkcomponent. This is a better pattern for Next.js applications as it enables prefetching and optimization by the framework.apps/mail/hooks/use-contacts.ts (3)
1-6: Appropriate imports for the contacts hookThe imports for the hook are well-organized, including the necessary dependencies for data fetching, authentication, and component lifecycle management.
7-10: Well-designed hook signature with SWR cachingGood use of
useSWRImmutablefor caching the contacts data keyed by the session's connection ID. This ensures efficient data loading and prevents unnecessary refetches.
32-37: Clean return logicThe hook correctly handles the case when data is not yet available by returning an empty array, avoiding potential undefined errors.
apps/mail/lib/auth.ts (3)
75-75: Added ticket usage trackingGood addition of the
hasUsedTicketfield to track whether users have used their early access tickets.
83-87: Improved early access control with environment variableGood enhancement of the early access check to include the
EARLY_ACCESS_ENABLEDenvironment variable. This provides more flexibility in controlling access across different environments.
183-183: Added ticket usage to session contextProperly exposing the
hasUsedTicketstatus in the session context for client-side usage.apps/mail/components/create/create-email.tsx (9)
12-12: Appropriate import for contact functionalityAdded import for the new
useContactshook to support email address auto-suggestions.
59-68: Well-implemented contact filtering utilityThe
filterContactshelper function efficiently filters contacts based on search terms and excludes already selected emails. Good use of case-insensitive comparison and proper filtering logic.
81-84: Comprehensive state management for contact selectionAdded necessary state variables for tracking selected contacts and indices for keyboard navigation, which is important for accessibility.
101-102: Improved drag-and-drop handlingEnhanced drag-and-drop UX with better visual feedback using a counter to track drag events. This solves the common issue where nested elements can cause multiple drag enter/leave events.
Also applies to: 401-414
129-142: Optimized contact filtering with memoizationGood use of
useMemoto prevent unnecessary recalculations of filtered contacts. The dependencies are correctly specified to ensure updates when relevant state changes.
450-483: Well-implemented keyboard navigation for dropdown listsGood implementation of refs and effects for keyboard navigation in contact dropdowns, which improves accessibility. The auto-scrolling of selected items into view is a nice touch.
498-506: Improved visual feedback for drag and drop operationsThe drag overlay is now only shown when both dragging and hovering over the card, which provides better user feedback and prevents misleading UI states.
565-583: Well-implemented contact suggestion dropdownGood implementation of the contact suggestion dropdown with keyboard navigation and selection. The dropdown shows both name and email when available, which improves usability.
828-987: Improved layout organizationThe reorganization of the email form with a sticky footer for the AI assistant and action buttons improves the user experience, especially for longer emails.
🧰 Tools
🪛 Biome (1.9.4)
[error] 967-967: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| } else if (emailState.length === 0) { | ||
| // Only show error if there are no emails yet | ||
| toast.error(t('pages.createEmail.invalidEmail')); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Bug in email validation logic
The logic for showing validation errors has been modified to only trigger when no emails are present in the field, but this could allow invalid emails to be silently accepted if at least one valid email exists.
- } else if (emailState.length === 0) {
+ } else {
// Only show error if there are no emails yet
toast.error(t('pages.createEmail.invalidEmail'));
}📝 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.
| } else if (emailState.length === 0) { | |
| // Only show error if there are no emails yet | |
| toast.error(t('pages.createEmail.invalidEmail')); | |
| } | |
| }; | |
| } else { | |
| // Only show error if there are no emails yet | |
| toast.error(t('pages.createEmail.invalidEmail')); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mail/components/create/create-email.tsx (1)
251-255: Bug in email validation logicThe logic for showing validation errors has been modified to only trigger when no emails are present in the field, but this could allow invalid emails to be silently accepted if at least one valid email exists.
- } else if (emailState.length === 0) { + } else { // Only show error if there are no emails yet toast.error(t('pages.createEmail.invalidEmail')); }
🧹 Nitpick comments (1)
apps/mail/components/create/create-email.tsx (1)
979-979: Use optional chaining for fileInputRefTo prevent potential runtime errors, use optional chaining when accessing the click method on fileInputRef.current.
- onClick={() => fileInputRef.current && fileInputRef.current.click()} + onClick={() => fileInputRef.current?.click()}🧰 Tools
🪛 Biome (1.9.4)
[error] 979-979: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/components/create/ai-assistant.tsx(3 hunks)apps/mail/components/create/create-email.tsx(14 hunks)apps/mail/components/mail/reply-composer.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/mail/components/create/ai-assistant.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/mail/components/create/create-email.tsx
[error] 979-979: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
apps/mail/components/mail/reply-composer.tsx (2)
54-54: Tracking capability added with PostHogThe PostHog library is now imported to enable user behavior tracking in the email reply functionality.
325-335: Analytics tracking implemented for reply emailsGood addition of analytics events that capture different scenarios when replying to emails. This will provide valuable user behavior data on how CC and BCC features are utilized.
apps/mail/components/create/create-email.tsx (8)
12-12: Added essential imports for enhanced functionalityThe addition of the useContacts hook and PostHog integration enables improved contact suggestions and analytics tracking.
Also applies to: 36-36
60-69: Well-implemented contact filtering utilityThe new filterContacts function provides a clean way to filter contacts based on search terms and exclude already selected emails. This supports the new autocomplete dropdown functionality.
81-85: Added state management for contact selectionThese state variables effectively manage the keyboard navigation within contact suggestion dropdowns for To, CC, and BCC fields.
130-143: Efficient memoized contact filteringUsing React.useMemo for filtered contacts is a good performance optimization, ensuring that filters are only recalculated when dependencies change.
462-495: Good implementation of dropdown scrollingThe added effects ensure that keyboard navigation within contact dropdowns keeps the selected item in view by scrolling automatically. This improves the UX for keyboard users.
510-518: Enhanced drag and drop feedbackThe improved drag and drop implementation now accurately tracks when files are being dragged over the card component, providing better visual feedback to users.
577-596: Contact suggestion dropdown implementationThe contact suggestion dropdowns provide a nice user experience for selecting contacts. The implementation includes keyboard navigation, visual highlighting of selected items, and showing both contact names and emails when available.
Also applies to: 691-793
965-983: Improved attachment input handlingThe attachment input is now hidden and triggered via a button click, which provides a cleaner UI while maintaining functionality.
🧰 Tools
🪛 Biome (1.9.4)
[error] 979-979: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| // Track different email sending scenarios | ||
| if (showCc && showBcc) { | ||
| console.log(posthog.capture('Create Email Sent with CC and BCC')); | ||
| } else if (showCc) { | ||
| console.log(posthog.capture('Create Email Sent with CC')); | ||
| } else if (showBcc) { | ||
| console.log(posthog.capture('Create Email Sent with BCC')); | ||
| } else { | ||
| console.log(posthog.capture('Create Email Sent')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log calls around PostHog tracking
The console.log statements wrapping PostHog capture calls are likely unintended debug code that should be removed.
- console.log(posthog.capture('Create Email Sent with CC and BCC'));
+ posthog.capture('Create Email Sent with CC and BCC');
- console.log(posthog.capture('Create Email Sent with CC'));
+ posthog.capture('Create Email Sent with CC');
- console.log(posthog.capture('Create Email Sent with BCC'));
+ posthog.capture('Create Email Sent with BCC');
- console.log(posthog.capture('Create Email Sent'));
+ posthog.capture('Create Email Sent');📝 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.
| // Track different email sending scenarios | |
| if (showCc && showBcc) { | |
| console.log(posthog.capture('Create Email Sent with CC and BCC')); | |
| } else if (showCc) { | |
| console.log(posthog.capture('Create Email Sent with CC')); | |
| } else if (showBcc) { | |
| console.log(posthog.capture('Create Email Sent with BCC')); | |
| } else { | |
| console.log(posthog.capture('Create Email Sent')); | |
| } | |
| // Track different email sending scenarios | |
| if (showCc && showBcc) { | |
| posthog.capture('Create Email Sent with CC and BCC'); | |
| } else if (showCc) { | |
| posthog.capture('Create Email Sent with CC'); | |
| } else if (showBcc) { | |
| posthog.capture('Create Email Sent with BCC'); | |
| } else { | |
| posthog.capture('Create Email Sent'); | |
| } |
…se structure. Update getMail to return IGetThreadResponse, modify related components to utilize new structure, and enhance error handling in driver functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/components/draft/drafts.tsx (1)
15-15: Potentially unused importThe SearchBar component is imported but appears to be commented out in the only place it was used. If there are no other usages of this component in the file, this import is now redundant.
If the SearchBar removal is intentional and permanent, you can remove this unused import:
-import { SearchBar } from '../mail/search-bar';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/draft/drafts.tsx(1 hunks)
🔇 Additional comments (1)
apps/mail/components/draft/drafts.tsx (1)
77-77: Consider impact of commented out SearchBarThe SearchBar component has been commented out in the UI when no items are selected, but
searchValueis still being used inuseDrafts(searchValue.value, defaultPageSize)on line 35. Without the SearchBar, users won't have a way to input search terms for filtering drafts.Is this intentional or should search functionality be preserved? If it's intentional, consider removing the unused SearchBar import and clarifying how users can search drafts without this UI element.
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (5)
apps/mail/app/api/driver/[id]/route.ts (1)
6-27:⚠️ Potential issueMissing user authentication
Unlike the other routes that use
getAuthenticatedUserId()for authentication and user-specific rate limiting, this route only uses the mail ID for rate limiting without verifying that the current user has access to this specific mail thread.Consider adding user authentication similar to other routes:
-import { checkRateLimit, getRatelimitModule, processIP } from '../../utils'; +import { checkRateLimit, getRatelimitModule, processIP, getAuthenticatedUserId } from '../../utils'; export const GET = async (req: NextRequest, { params }: { params: Promise<{ id: string }> }) => { + const userId = await getAuthenticatedUserId(); const finalIp = processIP(req); const { id } = await params; const ratelimit = getRatelimitModule({ - prefix: `ratelimit:get-mail-${id}`, + prefix: `ratelimit:get-mail-${id}-${userId}`, limiter: Ratelimit.slidingWindow(60, '1m'), }); // ... rest of the code const driver = await getActiveDriver(); + // Potential security check: ensure this mail belongs to the user const threadResponse = await driver.get(id); return NextResponse.json(threadResponse, { status: 200, headers, }); };apps/mail/actions/settings.ts (1)
85-93:⚠️ Potential issue
sendNotification()capturesresendbefore it is initialised – runtime ReferenceError
sendNotificationcloses overresend, butconst resend = …is declared after the first possible invocation (inside theinsert(...).catch()block).
In JS theconstvariable is in TDZ; callingsendNotification()before initialisation throws.- const sendNotification = () => { - return resend.emails.send({ ... }); - }; - ... - const resend = process.env.RESEND_API_KEY + const resend = process.env.RESEND_API_KEY ? new Resend(process.env.RESEND_API_KEY) : { emails: { send: async (...args: any[]) => console.log(args) } }; + const sendNotification = () => { + return resend.emails.send({ ... }); + };Move the
resenddeclaration above, or inject it into the helper to break the circular dependency.apps/mail/hooks/use-threads.ts (2)
84-101:⚠️ Potential issueFetcher ignores the SWR key → pagination / filtering broken
useSWRInfinitebuilds a rich key (connectionId, folder, query, …) but the fetcher discards it and always callsGET /api/driver.
All pages therefore return identical inbox data, search & folder filters are lost, and you’ll hammer the same endpoint on “load more”.- () => axios.get<RawResponse>(`/api/driver`).then((res) => res.data), + (key: FetchEmailsTuple) => fetchEmails(key),This restores:
- Per‑page filtering / pagination.
- Correct use of the still‑defined
fetchEmails.- Coherent cache keys → no stale data crossover.
Don’t forget to remove the now‑unused
axios.get('/api/driver')import if you adopt the fix.
28-50: 💡 Verification agent🧩 Analysis chain
Dead ‑ but still shipped ‑
fetchEmailshelper
fetchEmailsis now orphaned: nothing calls it after the refactor.
Keeping unused helpers increases maintenance cost and sneaks through dead code paths that may still be “imported” during tree‑shaking, hurting bundle size.-const fetchEmails = async ([ ... ]: FetchEmailsTuple): Promise<RawResponse> => { - ... -};Either wire it back in (see next comment) or delete it altogether.
🏁 Script executed:
#!/bin/bash rg "fetchEmails" -nLength of output: 82
🏁 Script executed:
#!/bin/bash rg "fetchEmails\(" -nLength of output: 23
Remove unused
fetchEmailshelper
fetchEmailsis defined in apps/mail/hooks/use-threads.ts (lines 28–50) but is never called or exported anywhere in the codebase. It adds dead code paths and bloats bundle size. Please delete the entirefetchEmailsfunction block.apps/mail/app/api/driver/google.ts (1)
372-392:⚠️ Potential issue
rawpayload must be base64url, not vanilla base64Gmail’s REST API expects base64url encoding (
+ → -,/ → _, remove padding).
createDraftdoes this conversion butparseOutgoingdoes not:-const encodedMessage = Buffer.from(emailContent).toString('base64'); +const encodedMessage = Buffer.from(emailContent) + .toString('base64') + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=+$/, '');Without this, messages containing
+or/occasionally fail with
400 Bad Request "Invalid value for ByteString".Apply the same normalisation used a few lines below in
createDraft.
♻️ Duplicate comments (1)
apps/mail/actions/settings.ts (1)
54-56: Same PII concern on save failureApply the same sanitisation strategy here to avoid leaking user data in logs.
🧹 Nitpick comments (13)
apps/mail/actions/connections.ts (1)
3-3: Unused import detectedThe
getAuthenticatedUserIdfunction is imported but not used in this file.-import { getAuthenticatedUserId } from '@/app/api/utils';apps/mail/hooks/use-stats.ts (1)
3-3: Unused import detectedThe
mailCountimport is no longer used after switching to the axios-based API call.-import { mailCount } from '@/actions/mail';apps/mail/app/api/driver/notes/route.ts (1)
3-3: Unused import detectedThe
fetchThreadNotesfunction is imported but not used in the code after switching tonotesManager.getThreadNotes.-import { fetchThreadNotes } from '@/actions/notes';apps/mail/actions/mail.ts (1)
138-142: Correctly updated to access messages array in thread resultsThe changes appropriately adapt the code to work with the new
IGetThreadResponsestructure by checkingmessages.lengthand iterating through themessagesarray. The type casting in thesomecallback is redundant though.- const isThreadStarred = result.value.messages.some((message: ParsedMessage) => + const isThreadStarred = result.value.messages.some((message) => message.tags?.includes('STARRED'), );apps/mail/app/api/auth/settings/route.ts (1)
35-35: Specify return type for clarityConsider specifying the return type of the GET function to indicate it can return null, ensuring consumers are aware of this possibility.
-export const GET = async (req: NextRequest) => { +export const GET = async (req: NextRequest): Promise<NextResponse | null> => {apps/mail/components/mail/thread-display.tsx (2)
150-160: Possible N + 1 render cost inhasImagescheck
useMemoiterates over every message and scansprocessedHtmlstring on each render.
For large threads this can be expensive and will rerun wheneveremailDatareference changes (e.g. after everymutateThread()).- const hasImages = useMemo(() => { - if (!emailData) return false; - return emailData.messages.some(message => { + const hasImages = useMemo(() => { + if (!emailData?.messages?.length) return false; + // bail‑out early when we already found an image + return emailData.messages.some(({ attachments = [], processedHtml = '' }) => {Consider caching the result in the thread response coming from the server or computing it once in a
useEffect+useStateso that subsequent re‑renders don’t repeat the expensive scan.
162-163:hasMultipleParticipantsomits BCC & may mis‑count selfYou add
+1to include the sender, but:
- The sender may already be present in
to/cc.bccparticipants are ignored.If the goal is “more than two unique participants”, compute a
Setof email addresses acrossfrom,to,cc,bcc, then check.size > 2.apps/mail/actions/settings.ts (1)
14-19: Potential PII leakage in server logs
console.error('Settings validation error: Schema mismatch', { error, settings })dumps the completesettingsobject to server logs, which can contain user‑specific data (language, timezone, trusted senders, etc.).
Mask or omit sensitive fields before logging.
[security]apps/mail/components/mail/mail-list.tsx (3)
95-103:useThreadinvoked withnullbut not guardedWhen
demoistrueyou passnull, causing the hook to make a request with an invalid key (['thread', null]).
Guard inside the hook or skip the call for demo mode to avoid an unnecessary SWR entry.
486-499:selectAllTODO comment – implementation still needs range supportThe TODO indicates additional debugging; consider finishing the feature or removing the comment to avoid confusion.
590-606:handleMailClickignores failures fromsetThreadIdmutation
setThreadIdreturns a promise (URL update). You may want toawaitit or wrap invoidintentionally with a comment to silence ESLint “no‑floating‑promises” warnings for better clarity.apps/mail/hooks/use-threads.ts (1)
126-137: Duplicate fetch logic – reusefetchThread
useThreadre‑implements the fetcher instead of reusingfetchThread, duplicating axios logic and type parameters.- const { data, isLoading, error, mutate } = useSWR<IGetThreadResponse>( - session?.user.id && id ? [session.user.id, id, session.connectionId] : null, - () => axios.get<IGetThreadResponse>(`/api/driver/${id}`).then((res) => res.data), + const { data, isLoading, error, mutate } = useSWR<IGetThreadResponse>( + session?.user.id && id ? [session.user.id, id, session.connectionId] : null, + fetchThread, );Centralising the call:
• avoids silent API divergences,
• keeps the preload helper (preloadThread) and the hook in sync,
• makes mocking easier in tests.apps/mail/app/api/driver/google.ts (1)
260-280: Comment vs implementation mismatch for self‑recipientsThe comment says “allow sending to yourself”, yet the duplicate‑check prevents any second occurrence of
fromEmail:
- First hit ⇒ added to
uniqueRecipients.- Further hits ⇒ filtered out as duplicate.
Either clarify the intent or adapt the logic, e.g.:
- // Only check for duplicates, allow sending to yourself - if (!uniqueRecipients.has(email)) { + // Allow one copy to self, block multiple duplicates + if (email === fromEmail || !uniqueRecipients.has(email)) {Inconsistent behaviour here is hard to debug for end‑users picking addresses via UI autocomplete.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/mail/actions/connections.ts(5 hunks)apps/mail/actions/mail.ts(3 hunks)apps/mail/actions/notes.ts(6 hunks)apps/mail/actions/settings.ts(5 hunks)apps/mail/app/api/auth/settings/route.ts(1 hunks)apps/mail/app/api/driver/[id]/route.ts(1 hunks)apps/mail/app/api/driver/connections/route.ts(1 hunks)apps/mail/app/api/driver/count/route.ts(1 hunks)apps/mail/app/api/driver/google.ts(17 hunks)apps/mail/app/api/driver/notes/route.ts(1 hunks)apps/mail/app/api/driver/route.ts(1 hunks)apps/mail/app/api/driver/types.ts(2 hunks)apps/mail/app/api/utils.ts(1 hunks)apps/mail/components/mail/mail-list.tsx(17 hunks)apps/mail/components/mail/mail.tsx(1 hunks)apps/mail/components/mail/reply-composer.tsx(9 hunks)apps/mail/components/mail/thread-display.tsx(6 hunks)apps/mail/hooks/use-contacts.ts(1 hunks)apps/mail/hooks/use-stats.ts(2 hunks)apps/mail/hooks/use-threads.ts(4 hunks)apps/mail/types/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/mail/actions/notes.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mail/hooks/use-contacts.ts
- apps/mail/components/mail/reply-composer.tsx
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/mail/app/api/driver/connections/route.ts (4)
apps/mail/app/api/utils.ts (4)
getAuthenticatedUserId(21-30)processIP(43-52)getRatelimitModule(7-19)checkRateLimit(32-41)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (1)
connection(68-85)apps/mail/types/index.ts (1)
IConnection(64-69)
apps/mail/app/api/auth/settings/route.ts (4)
apps/mail/app/api/utils.ts (4)
getAuthenticatedUserId(21-30)processIP(43-52)getRatelimitModule(7-19)checkRateLimit(32-41)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (1)
userSettings(113-122)packages/db/src/user_settings_default.ts (1)
userSettingsSchema(13-21)
apps/mail/app/api/driver/types.ts (1)
apps/mail/types/index.ts (1)
ParsedMessage(37-62)
apps/mail/actions/settings.ts (4)
packages/db/src/user_settings_default.ts (2)
UserSettings(23-23)userSettingsSchema(13-21)apps/mail/app/error.tsx (1)
Error(6-29)packages/db/src/schema.ts (1)
earlyAccess(59-66)packages/db/src/index.ts (1)
db(17-17)
apps/mail/actions/mail.ts (3)
apps/mail/app/api/driver/types.ts (1)
IGetThreadResponse(3-8)apps/mail/app/api/driver/google.ts (1)
driver(125-871)apps/mail/types/index.ts (1)
ParsedMessage(37-62)
🔇 Additional comments (38)
apps/mail/actions/connections.ts (2)
1-9: Quote style and imports have been updated for consistencyThe file has been updated to use single quotes consistently and imports have been reorganized.
17-17: Error messages updated to use single quotesError message strings have been updated to use single quotes consistently throughout the file.
Also applies to: 23-23, 39-39, 55-55, 65-65, 77-78
apps/mail/hooks/use-stats.ts (1)
17-17: API fetching approach has been improvedThe hook now uses axios to fetch from a centralized API endpoint rather than using a custom function. This is a good improvement for consistency in the codebase.
apps/mail/app/api/driver/notes/route.ts (3)
1-5: Imports have been updated for new functionalityThe imports now include
getAuthenticatedUserIdand other necessary utilities.
8-25: Authentication and rate limiting improvedThe GET handler now properly retrieves the authenticated user ID and uses it for user-specific rate limiting. This is a good security practice.
27-32: Notes fetching logic improvedThe notes are now fetched using
notesManager.getThreadNoteswith both user ID and thread ID parameters, which provides better user-specific data handling.apps/mail/components/mail/mail.tsx (1)
454-455: Data structure usage improvedUpdated to use the
emailData.latestproperty instead of array indexing, which aligns with the new data structure format across the codebase.apps/mail/app/api/driver/connections/route.ts (3)
1-7: Clean import restructuringAppropriate imports have been added for database access (connection, eq, db) and authentication utilities (getAuthenticatedUserId).
10-22: Good security improvement: user-specific rate limitingThe change from static rate limiting to per-user rate limiting (
ratelimit:get-connections-${userId}) is a security enhancement as it prevents users from being affected by others' request patterns.
24-33: Improved data access pattern with direct DB queryReplacing the helper function with a direct Drizzle ORM query provides better control and transparency. The query correctly filters connections by the authenticated user's ID and selects only the necessary fields that match the IConnection interface.
apps/mail/app/api/driver/route.ts (4)
1-4: Good import restructuringThe imports have been appropriately reorganized to include the authentication utility and driver abstraction.
9-17: Well-structured parameter extractionEarly extraction of query parameters improves code readability and enables the use of these values in the rate limit prefix.
18-21: Enhanced rate limiting with folder and user contextIncluding both folder and user ID in the rate limit prefix (
ratelimit:list-threads-${folder}-${userId}) creates more granular rate limiting, which is a security improvement.
33-35: Good abstraction with driver patternUsing the active driver abstraction instead of a direct helper function allows for flexibility in supporting different mail providers and improves code organization.
apps/mail/app/api/driver/count/route.ts (3)
1-5: Well-structured importsThe imports follow the same pattern as other driver API routes, including authentication utilities, rate limiting, and the driver abstraction.
6-19: Consistent authentication and rate limiting patternThe endpoint correctly authenticates the user and implements user-specific rate limiting, following the established pattern across the application.
20-26: Clean implementation of count endpointThe implementation is straightforward and follows the driver abstraction pattern consistently, fetching the count and returning it with appropriate status and headers.
apps/mail/app/api/driver/[id]/route.ts (2)
1-4: Good import restructuringThe imports have been appropriately reorganized to use the driver abstraction instead of direct mail helper functions.
7-12: Improved parameter handling and rate limitingEarly awaiting and destructuring of the ID parameter improves code clarity, and the ID-specific rate limiting provides more granular control.
apps/mail/actions/mail.ts (2)
3-3: Good addition of the IGetThreadResponse importAdding this import aligns with the updated return type of the
getMailfunction and enables proper type checking.
33-33: Appropriate return type updateThe return type change from
Promise<ParsedMessage[]>toPromise<IGetThreadResponse>properly reflects the new structured data format with messages and metadata.apps/mail/app/api/driver/types.ts (3)
3-8: Well-structured interface for thread responseThe new
IGetThreadResponseinterface provides a clear structure for thread data, including the messages array and useful metadata like latest message, unread status, and reply count. This will improve type safety and developer experience throughout the codebase.
11-11: Updated method signature aligns with new interfaceThe change to the
getmethod signature to returnPromise<IGetThreadResponse>correctly aligns with the new interface, maintaining consistency in the type system.
45-45: Fixed missing semicolonAdding the semicolon after
email: stringmaintains code style consistency.apps/mail/app/api/utils.ts (5)
3-5: Added necessary imports for new functionalityThe added imports support the new
getAuthenticatedUserIdfunction and other refactored utilities.
7-19: Improved rate limit module with explicit returnThe refactored
getRatelimitModulefunction now explicitly returns the createdRatelimitinstance and has consistent formatting, improving code clarity.
21-30: Good centralization of authentication logicAdding the
getAuthenticatedUserIdfunction centralizes user authentication retrieval, promoting code reuse and consistency across API routes.
32-41: Improved formatting for checkRateLimit functionThe reformatted
checkRateLimitfunction maintains consistent code style with the rest of the file.
43-52: Improved formatting for processIP functionThe reformatted
processIPfunction maintains consistent code style with the rest of the file.apps/mail/app/api/auth/settings/route.ts (5)
1-7: Updated imports to support direct DB access and authenticationThe imports have been appropriately updated to include database-related modules and the new centralized authentication function.
10-10: Using centralized authentication functionGood use of the new
getAuthenticatedUserIdfunction for consistent authentication across API routes.
12-15: Enhanced security with user-scoped rate limitingUsing a user-specific prefix for rate limiting is a good security practice, preventing users from affecting each other's rate limits.
24-28: Direct database query replaces removed utility functionThe direct database query is well-structured and properly filters by the authenticated user ID.
33-33: Good use of schema validationUsing
userSettingsSchema.parse()ensures type safety and validates the structure of the settings before returning them to the client.apps/mail/components/mail/thread-display.tsx (1)
438-453:totalEmailsprop renamed but component may still expectlengthYou changed
totalEmails={emailData?.totalReplies}; ensureMailDisplay’s prop name & type has been updated accordingly, otherwise this is silently ignored.apps/mail/types/index.ts (1)
88-102: Type loosening may hide unintended usages
messageis now{ id: string }, yet many callers (e.g. MailList’sThreaddemo branch) rely on properties likesubject,sender, etc.
If these props are only accessed vialatestMessagethat’s fine, otherwise tighten the type (e.g.Pick<ParsedMessage,'id'>) and runtsc --noEmitto ensure no stale usages remain.apps/mail/app/api/driver/google.ts (2)
82-123: Nice addition: resilient API access with exponential back‑offThe
withExponentialBackoffwrapper is solid: jitter, max delay and early‑exit on non‑rate‑limit errors are well handled.
👍 Good defensive programming.
540-549: Good call: wrappingthreads.listwith retry logicUsing the new back‑off wrapper here dramatically reduces 429 churn.
One nit: consider exposingmaxResultsin the returned object (currently swallowed by the spread).
| // Returning null here when there are no settings so we can use the default settings with timezone from the browser | ||
| if (!result) return null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for database operations
While returning null for non-existent settings is appropriate, the function doesn't handle potential database errors. Consider adding try/catch to handle database exceptions.
- // Returning null here when there are no settings so we can use the default settings with timezone from the browser
- if (!result) return null;
+ try {
+ const [result] = await db
+ .select()
+ .from(userSettings)
+ .where(eq(userSettings.userId, userId))
+ .limit(1);
+
+ // Returning null here when there are no settings so we can use the default settings with timezone from the browser
+ if (!result) return null;
+
+ const settings = userSettingsSchema.parse(result.settings);
+
+ return NextResponse.json(settings);
+ } catch (error) {
+ console.error('Error fetching user settings:', error);
+ return NextResponse.json(
+ { error: 'Failed to fetch user settings' },
+ { status: 500, headers }
+ );
+ }📝 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.
| // Returning null here when there are no settings so we can use the default settings with timezone from the browser | |
| if (!result) return null; | |
| try { | |
| const [result] = await db | |
| .select() | |
| .from(userSettings) | |
| .where(eq(userSettings.userId, userId)) | |
| .limit(1); | |
| // Returning null here when there are no settings so we can use the default settings with timezone from the browser | |
| if (!result) return null; | |
| const settings = userSettingsSchema.parse(result.settings); | |
| return NextResponse.json(settings); | |
| } catch (error) { | |
| console.error('Error fetching user settings:', error); | |
| return NextResponse.json( | |
| { error: 'Failed to fetch user settings' }, | |
| { status: 500, headers } | |
| ); | |
| } |
| if (!emailData || !id) return; | ||
| const unreadEmails = emailData.filter(e => e.unread); | ||
| const unreadEmails = emailData.messages.filter(e => e.unread); | ||
| if (unreadEmails.length === 0) { | ||
| console.log('Marking email as read:', id); | ||
| markAsRead({ ids: [id] }).catch((error) => { | ||
| console.error('Failed to mark email as read:', error); | ||
| toast.error(t('common.mail.failedToMarkAsRead')); | ||
| }).then(() => Promise.all([mutateThread(), mutateThreads(), mutateStats()])) | ||
| }).then(() => Promise.all([mutateThread(), mutateStats()])) | ||
| } else { | ||
| console.log('Marking email as read:', id, ...unreadEmails.map(e => e.id)); | ||
| const ids = [id, ...unreadEmails.map(e => e.id)] | ||
| markAsRead({ ids }).catch((error) => { | ||
| console.error('Failed to mark email as read:', error); | ||
| toast.error(t('common.mail.failedToMarkAsRead')); | ||
| }).then(() => Promise.all([mutateThread(), mutateThreads(), mutateStats()])) | ||
| }).then(() => Promise.all([mutateThread(), mutateStats()])) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
catch().then() runs mutate*() even on failure
When markAsRead fails the .catch branch swallows the error and still proceeds to .then() which mutates the SWR cache and shows success toast from the promise passed to toast.promise.
Return/throw inside catch or move the mutation into the fulfilled branch to avoid false‑positive UI feedback.
| await db | ||
| .update(earlyAccess) | ||
| .set({ | ||
| hasUsedTicket: email, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .where(eq(earlyAccess.email, foundUser.email)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Double write to earlyAccess can race & overwrite
The second update runs unconditionally even when the insert succeeded (and already wrote hasUsedTicket: '').
Consider moving the second update inside the insert.catch success path or using an upsert to guarantee atomicity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mail/app/api/auth/settings/route.ts (1)
24-28: 🛠️ Refactor suggestionConsider adding error handling for database operations.
The database query lacks error handling. If the database operation fails, it will throw an unhandled exception.
- const [result] = await db - .select() - .from(userSettings) - .where(eq(userSettings.userId, userId)) - .limit(1); + try { + const [result] = await db + .select() + .from(userSettings) + .where(eq(userSettings.userId, userId)) + .limit(1); + + // Returning null here when there are no settings so we can use the default settings with timezone from the browser + if (!result) return NextResponse.json({ settings: defaultUserSettings }, { status: 200 }); + + const settings = userSettingsSchema.parse(result.settings); + + return NextResponse.json(settings); + } catch (error) { + console.error('Error fetching user settings:', error); + return NextResponse.json( + { error: 'Failed to fetch user settings' }, + { status: 500, headers } + ); + }
🧹 Nitpick comments (1)
apps/mail/app/api/auth/settings/route.ts (1)
31-31: Comment is no longer accurate.The comment states "Returning null here when there are no settings", but the code is returning a JSON response with default settings, not null.
- // Returning null here when there are no settings so we can use the default settings with timezone from the browser + // If no settings found, return the default settings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/app/api/auth/settings/route.ts(1 hunks)packages/db/migrations/0019_mean_war_machine.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/migrations/0019_mean_war_machine.sql
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/app/api/auth/settings/route.ts (4)
apps/mail/app/api/utils.ts (4)
getAuthenticatedUserId(21-30)processIP(43-52)getRatelimitModule(7-19)checkRateLimit(32-41)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (1)
userSettings(113-122)packages/db/src/user_settings_default.ts (2)
defaultUserSettings(3-11)userSettingsSchema(13-21)
🔇 Additional comments (2)
apps/mail/app/api/auth/settings/route.ts (2)
10-10: Good use of centralized authentication.Using the
getAuthenticatedUserId()utility function improves code consistency across API routes and centralizes authentication logic.
12-15: Good improvement on rate limiting.Including the user ID in the rate limit prefix (
ratelimit:get-settings-${userId}) makes the rate limiting user-specific rather than IP-specific only, which is more accurate and fair for users behind shared IPs.
|
|
||
| const settings = userSettingsSchema.parse(result.settings); | ||
|
|
||
| return NextResponse.json(settings); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent response structure.
When settings exist (line 35), the response structure is different from when no settings exist (line 31). The former returns settings directly, while the latter wraps them in a { settings: ... } object.
- return NextResponse.json(settings);
+ return NextResponse.json({ settings }, { status: 200 });This provides a consistent response structure regardless of whether default or user settings are returned.
📝 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 NextResponse.json(settings); | |
| return NextResponse.json({ settings }, { status: 200 }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/mail/components/mail/mail-list.tsx (1)
165-248:⚠️ Potential issueInconsistent render pattern for demo content.
The demo content is defined but never used due to the early return at line 163. Additionally, there's a redundant check at line 250 that would never be reached.
Refactor the rendering pattern to be more consistent and fix the demo mode rendering issue:
-if (!demo && (isLoading || !latestMessage || !getThreadData)) return null +if (isLoading || !latestMessage || !getThreadData) { + if (demo && demoMessage) { + // For demo mode, use demoMessage even when thread data is loading + latestMessage = demoMessage; + } else { + return null; + } +} -const demoContent = demo && latestMessage ? <div className="p-1 px-3" onClick={onClick ? onClick(latestMessage) : undefined}> - ... -</div> : null -if (demo) return demoContentThis simplifies the logic and ensures demo content is properly rendered.
♻️ Duplicate comments (1)
apps/mail/components/mail/mail-list.tsx (1)
163-163:⚠️ Potential issueIneffective loading guard condition.
This loading guard will always return
nullfor demo threads because the condition!demo && (isLoading || !latestMessage || !getThreadData)will be true whendemois true, making the entire component returnnull.The early-return check prevents demo mode from ever rendering content. Modify the condition to:
-if (!demo && (isLoading || !latestMessage || !getThreadData)) return null +if ((isLoading || !latestMessage || !getThreadData) && !demo) return nullThis preserves the loading guard for real data while allowing demo threads to render.
🧹 Nitpick comments (1)
apps/mail/components/mail/mail-list.tsx (1)
486-487: TODO comment in selectAll function.There's a TODO comment that should be addressed before deploying to production.
Is there a specific debugging issue with the
selectAllfunction that needs to be resolved? I can help review or fix any issues with the bulk selection logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/components/mail/mail-list.tsx(18 hunks)apps/mail/components/mail/mail.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mail/components/mail/mail.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/components/mail/mail-list.tsx (7)
apps/mail/hooks/use-threads.ts (1)
useThread(131-144)apps/mail/lib/utils.ts (3)
FOLDERS(10-17)getEmailLogo(358-361)formatDate(69-110)apps/mail/components/ui/avatar.tsx (1)
AvatarFallback(47-47)apps/mail/lib/email-utils.client.tsx (1)
highlightText(59-77)apps/mail/components/ui/tooltip.tsx (3)
Tooltip(59-59)TooltipTrigger(59-59)TooltipContent(59-59)apps/mail/types/index.ts (1)
ParsedMessage(37-62)apps/mail/components/ui/button.tsx (1)
Button(55-55)
🔇 Additional comments (15)
apps/mail/components/mail/mail-list.tsx (15)
14-14: Type import update adds ParsedMessage interface.The import line now includes the
ParsedMessagetype, which aligns with the updated approach of using the latest message from a thread.
23-23: Enhanced thread data fetching with useThread hook.Added
preloadThreadanduseThreadimports from the threads hook file, supporting the new thread-based data model.
83-83: New data model: Thread-based approach with dynamic data fetching.The component now receives a
demoMessageprop for demo mode and uses theuseThreadhook to fetch thread data from the server. This creates a more consistent data flow by usinglatestMessageas a single source of truth that comes either from demo data or the API response.Also applies to: 94-97
99-102: Improved thread selection logic using latestMessage references.The selection logic now properly checks for the existence of
latestMessagebefore attempting to access its properties, preventing potential null pointer exceptions.
104-104: Simplified bulk selection check using optional chaining.The bulk selection check now properly handles cases where
latestMessagemight be undefined, using the nullish coalescing operator to fall back to the message ID.
106-109: Added safety check for thread labels.The thread labels logic now includes a null check for
latestMessagebefore accessing its tags, preventing potential runtime errors.
117-117: Improved prefetching logic with demo mode handling.The mouse enter handler now skips prefetching for demo mode or when
latestMessageis unavailable, preventing unnecessary network requests.
130-130: Updated message ID reference for prefetching.Now using
latestMessage.threadIdwith fallback tomessage.idfor prefetching, aligning with the new thread-based data model.
252-337: Enhanced thread content rendering with dynamic data.The real thread content now properly uses
latestMessageandgetThreadDatafor rendering, including thread unread status fromgetThreadData.hasUnreadand proper thread replies count display.
339-351: Improved thread wrapper with null safety check.The Thread component now properly checks for
latestMessageexistence before wrapping the content in the ThreadWrapper, preventing potential rendering errors.
376-376: Added demoMessage prop to Thread component in MailListDemo.Now properly passing the item as demoMessage to support the new thread-based rendering approach in demo mode.
401-401: Added isReachingEnd to useThreads destructuring.Now extracting the isReachingEnd value from useThreads to better manage pagination state.
579-579: Updated handleMailClick parameter type.The parameter type for
handleMailClickhas been changed fromInitialThreadtoParsedMessageto align with the new data usage, making the typing more precise.
625-625: Updated key generation for Thread components.Thread components now use composite keys combining
data.idand the index, which helps React identify and rerender components efficiently when the list changes.
633-651: Improved "Load More" button logic.The "Load More" button now only disables when
isLoading(notisValidating) and only shows when there are enough items and a next page token exists but not currently validating. This provides a better UX for pagination.
|
|
||
| const content = ( | ||
| <div className="p-1 px-3" onClick={onClick ? onClick(message) : undefined}> | ||
| if (demo) return demoContent |
There was a problem hiding this comment.
Unreachable demo check due to early return.
This check is unreachable because of the early return at line 163 that already handles the demo case.
Remove this line as part of the refactoring suggested for the demo content rendering pattern.
| {Math.random() > 0.5 ? ( | ||
| <Tooltip> | ||
| <TooltipTrigger asChild> | ||
| <span className="rounded-md border border-dotted px-[5px] py-[1px] text-xs opacity-70"> | ||
| {message.totalReplies} | ||
| {Math.random() * 10} | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent className="px-1 py-0 text-xs"> | ||
| {t('common.mail.replies', { count: message.totalReplies })} | ||
| {t('common.mail.replies', { count: Math.random() * 10 })} | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ) : null} |
There was a problem hiding this comment.
Unreliable reply count using Math.random().
Using Math.random() for both the condition and the reply count will cause inconsistent UI behavior on re-renders.
In demo mode, either use a fixed value or derive it deterministically from the thread:
-{Math.random() > 0.5 ? (
+{demoMessage && demoMessage.id.charCodeAt(0) % 2 === 0 ? (
<Tooltip>
<TooltipTrigger asChild>
<span className="rounded-md border border-dotted px-[5px] py-[1px] text-xs opacity-70">
- {Math.random() * 10}
+ {(demoMessage.id.charCodeAt(0) % 5) + 1}
</span>
</TooltipTrigger>
<TooltipContent className="px-1 py-0 text-xs">
- {t('common.mail.replies', { count: Math.random() * 10 })}
+ {t('common.mail.replies', { count: (demoMessage.id.charCodeAt(0) % 5) + 1 })}
</TooltipContent>
</Tooltip>
) : null}This approach produces consistent values based on the message ID.
📝 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.
| {Math.random() > 0.5 ? ( | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <span className="rounded-md border border-dotted px-[5px] py-[1px] text-xs opacity-70"> | |
| {message.totalReplies} | |
| {Math.random() * 10} | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent className="px-1 py-0 text-xs"> | |
| {t('common.mail.replies', { count: message.totalReplies })} | |
| {t('common.mail.replies', { count: Math.random() * 10 })} | |
| </TooltipContent> | |
| </Tooltip> | |
| ) : null} | |
| {demoMessage && demoMessage.id.charCodeAt(0) % 2 === 0 ? ( | |
| <Tooltip> | |
| <TooltipTrigger asChild> | |
| <span className="rounded-md border border-dotted px-[5px] py-[1px] text-xs opacity-70"> | |
| {(demoMessage.id.charCodeAt(0) % 5) + 1} | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent className="px-1 py-0 text-xs"> | |
| {t('common.mail.replies', { count: (demoMessage.id.charCodeAt(0) % 5) + 1 })} | |
| </TooltipContent> | |
| </Tooltip> | |
| ) : null} |
|
i am speed |
5 participants
Summary by CodeRabbit
New Features
Improvements
Chores