Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update refactors the recipient input functionality in the mail application. Inline recipient input logic and UI for "To", "Cc", and "Bcc" fields in the email creation and reply components are removed and replaced with a new reusable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateEmail/ReplyCompose
participant EmailInput
participant Contacts (future)
User->>CreateEmail/ReplyCompose: Focus recipient field ("to", "cc", "bcc")
CreateEmail/ReplyCompose->>EmailInput: Render with props (emails, input value, handlers)
User->>EmailInput: Type or paste email / select from dropdown
EmailInput->>EmailInput: Handle input, show dropdown, navigate with keyboard
User->>EmailInput: Press Enter/Tab/Space or click contact
EmailInput->>CreateEmail/ReplyCompose: Call addEmail handler
CreateEmail/ReplyCompose->>EmailInput: Update emails prop
EmailInput->>User: Display updated email tags
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 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: 4
🧹 Nitpick comments (4)
apps/mail/components/create/email-input.tsx (2)
31-33:dropdownRefis never read – remove or use it.The ref is attached to the DOM node but there are no reads (
dropdownRef.current) or side‑effects that depend on it.
Stale refs add noise and hint at dead code; consider deleting the ref or using it for imperative scroll/focus logic.
110-126: Add ARIA roles for better accessibility.The suggestion buttons behave like listbox options. Expose this to screen‑readers:
- <button + <button + role="option"Consider wrapping the dropdown in a container with
role="listbox"and wiringaria-selectedto the highlighted item.apps/mail/components/mail/reply-composer.tsx (2)
219-229: DuplicatefilterContactsimplementation – extract to a shared util.
CreateEmailandReplyComposenow hold identical filtering logic. Centralise it (e.g.lib/mail/filterContacts.ts) to:
- Avoid divergence.
- Reduce bundle size.
- Simplify future changes (e.g., fuzzy‑matching).
352-358:composerStateloading guard works, but awaitinghandleSendEmailhides submission errors.If
handleSendEmailthrows,.finallystill clears the loading flag.
Consider:- await handleSendEmail(data).finally(() => { - composerDispatch({ type: 'SET_LOADING', payload: false }); - }); + try { + await handleSendEmail(data); + } finally { + composerDispatch({ type: 'SET_LOADING', payload: false }); + }This pattern preserves the same behaviour yet is easier to read/debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/components/create/create-email.tsx(2 hunks)apps/mail/components/create/email-input.tsx(1 hunks)apps/mail/components/mail/reply-composer.tsx(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/mail/components/create/create-email.tsx (1)
apps/mail/components/create/email-input.tsx (1)
EmailInput(18-132)
apps/mail/components/create/email-input.tsx (1)
apps/mail/components/create/editor-autocomplete.ts (1)
handleKeyDown(166-212)
apps/mail/components/mail/reply-composer.tsx (4)
apps/mail/components/mail/data.tsx (1)
contacts(219-300)apps/mail/hooks/use-contacts.ts (1)
useContacts(6-35)apps/mail/types/index.ts (1)
Sender(32-35)apps/mail/components/create/email-input.tsx (1)
EmailInput(18-132)
🔇 Additional comments (1)
apps/mail/components/mail/reply-composer.tsx (1)
1190-1191: Button usestype="button"+ manualhandleSubmit– switch totype="submit.With the enclosing
<form>already wired toonSubmit={handleSubmit(onSubmit)}, you can rely on native form submission:- onClick={handleSubmit(onSubmit)} - type="button" + type="submit"This removes an extra function allocation per render and avoids double‑calling
handleSubmitif the user presses Enter inside a field.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/mail/components/create/create-email.tsx (1)
471-748: Remove unused state and effects mentioned in previous review.As noted in a previous review, you should also remove the now-unused state variables like
selectedContactIndex*,*DropdownRef, and the threeuseEffectscroll handlers that were used for the previous implementation.🧰 Tools
🪛 Biome (1.9.4)
[error] 722-722: 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 (1)
apps/mail/components/create/create-email.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/components/create/create-email.tsx (1)
apps/mail/components/create/email-input.tsx (1)
EmailInput(18-132)
🔇 Additional comments (1)
apps/mail/components/create/create-email.tsx (1)
28-28: LGTM: EmailInput component imported correctly.The new EmailInput component is properly imported from the local module.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/mail/lib/utils.ts (2)
433-437: Prefer optional chaining to satisfy linter and simplify the condition
match && match[1]can be replaced withmatch?.[1]which is terser and addresses the Biome useOptionalChain hint.- if (match && match[1]) { + if (match?.[1]) { return match[1]; }🧰 Tools
🪛 Biome (1.9.4)
[error] 436-437: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
505-527: DuplicatemonthNamesarrays – extract a shared constantBoth date patterns rebuild the
monthNameslist. Moving it outside thepatternsarray avoids repeated allocation and keeps the code DRY.+const MONTH_NAMES = ['january','february','march','april','may','june', + 'july','august','september','october','november','december']; ... - const monthNames = ['january', ...]; - const monthIndex = monthNames.findIndex(...) + const monthIndex = MONTH_NAMES.findIndex(...)apps/mail/components/mail/search-bar.tsx (2)
235-241: New form fields (cc,bcc, …) never reach the final query
cc,bcc, and several others are collected in state but never pushed intosearchTerms.
If this is intentional, remove them fromSearchForm; otherwise add the missing filters:+if (data.cc) searchTerms.push(`cc:${data.cc.toLowerCase()}`); +if (data.bcc) searchTerms.push(`bcc:${data.bcc.toLowerCase()}`);
697-697:select-noneprevents users from copy/pasting inside the search boxApplying
select-noneto the<Input>element stops text selection entirely, which degrades usability (e.g. copying part of the query).
Unless this was a deliberate design choice, consider removing the class.apps/mail/components/mail/mail-list.tsx (2)
118-126: Effect may fire unnecessarily & cause double renders
setSearchValueinside the effect depends onsearchValuebut it’s not listed in the deps array, whilesemanticSearchQueryis.
When the hook updates the parent atom,searchValue.valuechanges andsemanticSearchQuerybecomes identical, but the effect won’t re‑run. Include the setter’s stable reference and the value to avoid stale closures:- }, [semanticSearchQuery]); + }, [semanticSearchQuery, searchValue.value, setSearchValue]);
379-390: Duplicate snippet / badge markup – extract into a small componentThe highlighted body preview and “main term” badge are rendered twice (demo vs. real thread). To keep both branches in sync and cut duplication, move lines 377‑390 into a
ThreadPreviewchild component.This also makes it easier to adjust styling in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/mail/components/mail/mail-list.tsx(8 hunks)apps/mail/components/mail/search-bar.tsx(7 hunks)apps/mail/lib/utils.ts(1 hunks)apps/mail/locales/en.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/mail/locales/en.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/components/mail/search-bar.tsx (2)
apps/mail/hooks/use-search-value.ts (1)
useSearchValue(19-21)apps/mail/lib/utils.ts (2)
parseNaturalLanguageDate(496-544)parseNaturalLanguageSearch(455-494)
🪛 Biome (1.9.4)
apps/mail/lib/utils.ts
[error] 436-437: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| // From pattern | ||
| { | ||
| regex: /^from\s+([^:\s]+)/i, | ||
| transform: (match: string[]) => `from:${match[1]}` | ||
| }, | ||
| // To pattern | ||
| { | ||
| regex: /^to\s+([^:\s]+)/i, | ||
| transform: (match: string[]) => `to:${match[1]}` | ||
| }, | ||
| // Subject pattern | ||
| { | ||
| regex: /^subject\s+([^:\s]+)/i, | ||
| transform: (match: string[]) => `subject:${match[1]}` | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Natural‑language patterns only capture a single word – multi‑word queries will be truncated
The current regexes stop matching at the first whitespace, so a query like
"subject quarterly earnings" becomes subject:quarterly instead of subject:"quarterly earnings".
Consider capturing everything to the end of the string (optionally trimming quotes) so the intent isn’t lost:
- regex: /^subject\s+([^:\s]+)/i,
- transform: (m) => `subject:${m[1]}`
+ regex: /^subject\s+(.+)/i,
+ transform: (m) => `subject:"${m[1].trim()}"` // quote to keep spacesApply the same idea to the from and to patterns or add a second pattern that is not anchored with ^ for in‑sentence matches.
📝 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.
| // From pattern | |
| { | |
| regex: /^from\s+([^:\s]+)/i, | |
| transform: (match: string[]) => `from:${match[1]}` | |
| }, | |
| // To pattern | |
| { | |
| regex: /^to\s+([^:\s]+)/i, | |
| transform: (match: string[]) => `to:${match[1]}` | |
| }, | |
| // Subject pattern | |
| { | |
| regex: /^subject\s+([^:\s]+)/i, | |
| transform: (match: string[]) => `subject:${match[1]}` | |
| }, | |
| // From pattern | |
| { | |
| regex: /^from\s+([^:\s]+)/i, | |
| transform: (match: string[]) => `from:${match[1]}` | |
| }, | |
| // To pattern | |
| { | |
| regex: /^to\s+([^:\s]+)/i, | |
| transform: (match: string[]) => `to:${match[1]}` | |
| }, | |
| // Subject pattern | |
| { | |
| regex: /^subject\s+(.+)/i, | |
| transform: (m) => `subject:"${m[1].trim()}"` // quote to keep spaces | |
| }, |
| bcc: string; | ||
| q: string; | ||
| dateRange: DateRange; | ||
| category: string; | ||
| folder: string; | ||
| has: any; | ||
| fileName: any; | ||
| deliveredTo: string; | ||
| unicorn: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid any in the form schema – use explicit types
has and fileName are declared as any, defeating type‑safety. They appear to hold simple strings.
- has: any;
- fileName: any;
+ has: string;
+ fileName: string;Update the corresponding default values ('') to maintain consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bcc: string; | |
| q: string; | |
| dateRange: DateRange; | |
| category: string; | |
| folder: string; | |
| has: any; | |
| fileName: any; | |
| deliveredTo: string; | |
| unicorn: string; | |
| }; | |
| bcc: string; | |
| q: string; | |
| dateRange: DateRange; | |
| category: string; | |
| folder: string; | |
| - has: any; | |
| - fileName: any; | |
| + has: string; | |
| + fileName: string; | |
| deliveredTo: string; | |
| unicorn: string; | |
| }; |
| import { preloadThread, useThread, useThreads } from '@/hooks/use-threads'; | ||
| import { ThreadContextMenu } from '@/components/context/thread-context'; | ||
| import { cn, FOLDERS, formatDate, getEmailLogo } from '@/lib/utils'; | ||
| import { cn, FOLDERS, formatDate, getEmailLogo, getMainSearchTerm, parseNaturalLanguageSearch, parseNaturalLanguageDate } from '@/lib/utils'; |
There was a problem hiding this comment.
Unused import will break noUnusedLocals builds
parseNaturalLanguageDate is imported but never used in this file – TypeScript’s noUnusedLocals/eslint no-unused-vars rules will fail CI.
-import { cn, FOLDERS, formatDate, getEmailLogo, getMainSearchTerm, parseNaturalLanguageSearch, parseNaturalLanguageDate } from '@/lib/utils';
+import { cn, FOLDERS, formatDate, getEmailLogo, getMainSearchTerm, parseNaturalLanguageSearch } from '@/lib/utils';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { cn, FOLDERS, formatDate, getEmailLogo, getMainSearchTerm, parseNaturalLanguageSearch, parseNaturalLanguageDate } from '@/lib/utils'; | |
| import { cn, FOLDERS, formatDate, getEmailLogo, getMainSearchTerm, parseNaturalLanguageSearch } from '@/lib/utils'; |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
24-24: Consider usage of destructured state variables.You're correctly destructuring
[removingEmails, setRemovingEmails]from the hook, but these variables don't appear to be used in the component. If they're intended for future functionality, consider adding a TODO comment explaining the future use case, or remove them if they're not needed.apps/mail/hooks/use-mail-navigation.ts (1)
164-166: Direct navigation on Enter key.The direct call to
onNavigatebypasses the conditional check innavigateToThread, which allows navigation even when no thread is currently open. This creates inconsistent behavior compared to arrow key navigation.Consider using the same navigation pattern for consistency:
- const message = items[focusedIndex]; - const threadId = message.threadId ?? message.id; - onNavigate(threadId); + navigateToThread(focusedIndex);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/hooks/use-mail-list-hotkeys.ts(1 hunks)apps/mail/hooks/use-mail-navigation.ts(5 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
apps/mail/hooks/use-mail-list-hotkeys.ts (1)
useMailListHotkeys(3-7)
🔇 Additional comments (7)
apps/mail/hooks/use-mail-list-hotkeys.ts (1)
1-7: Well-structured hook implementation!This hook follows React best practices with a clean implementation that manages email removal state. The use of a Set for handling unique email IDs and the
as constassertion for the return type are good TypeScript practices.apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (2)
4-4: LGTM! Proper hook import.The hook is correctly imported from its dedicated file.
98-98: LGTM! Proper integration with handlers.The
archiveEmailfunction is correctly added to the handlers object.apps/mail/hooks/use-mail-navigation.ts (4)
70-75: Improved navigation logic.The conditional navigation is a good improvement. By checking if a thread is already open before navigating, you prevent unexpected UI changes when using keyboard navigation.
101-107: Consistent navigation handling.Good improvement to call
navigateToThreadafter scrolling the thread into view, ensuring navigation aligns with visual focus.
112-113: LGTM! Consistent navigation pattern.The pattern of scrolling into view and then navigating is consistently applied.
131-137: LGTM! Consistent navigation pattern.The pattern of scrolling into view and then navigating is consistently applied in all arrow key handlers.
| const archiveEmail = useCallback(async () => { | ||
| const emailId = hoveredEmailId.current; | ||
| if (!emailId) return; | ||
|
|
||
| // Immediately remove the email from the list | ||
| const updatedItems = items.filter((item) => item.id !== emailId); | ||
| mutate({ threads: updatedItems, nextPageToken: null }, false); | ||
|
|
||
| // Show success message | ||
| toast.success(t('common.mail.archived')); | ||
| }, [items, mutate, t]); |
There was a problem hiding this comment.
Missing server-side archive action.
The archiveEmail function updates the local UI state by filtering out the archived email from the list, but there doesn't appear to be a server-side API call to actually archive the email on the backend. This could lead to inconsistency when the user refreshes the page.
Consider adding a server-side archive action similar to how markAsUnreadAction is called:
const archiveEmail = useCallback(async () => {
const emailId = hoveredEmailId.current;
if (!emailId) return;
+ // Call server-side archive action
+ toast.promise(archiveEmailAction({ ids: [emailId] }), {
+ loading: t('common.actions.loading'),
+ success: async () => {
// Immediately remove the email from the list
const updatedItems = items.filter((item) => item.id !== emailId);
mutate({ threads: updatedItems, nextPageToken: null }, false);
- // Show success message
- toast.success(t('common.mail.archived'));
+ return t('common.mail.archived');
+ },
+ error: t('common.mail.failedToArchive'),
+ });
}, [items, mutate, t]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const archiveEmail = useCallback(async () => { | |
| const emailId = hoveredEmailId.current; | |
| if (!emailId) return; | |
| // Immediately remove the email from the list | |
| const updatedItems = items.filter((item) => item.id !== emailId); | |
| mutate({ threads: updatedItems, nextPageToken: null }, false); | |
| // Show success message | |
| toast.success(t('common.mail.archived')); | |
| }, [items, mutate, t]); | |
| const archiveEmail = useCallback(async () => { | |
| const emailId = hoveredEmailId.current | |
| if (!emailId) return | |
| // Call server-side archive action | |
| toast.promise(archiveEmailAction({ ids: [emailId] }), { | |
| loading: t('common.actions.loading'), | |
| success: async () => { | |
| // Immediately remove the email from the list | |
| const updatedItems = items.filter((item) => item.id !== emailId) | |
| mutate({ threads: updatedItems, nextPageToken: null }, false) | |
| return t('common.mail.archived') | |
| }, | |
| error: t('common.mail.failedToArchive'), | |
| }) | |
| }, [items, mutate, t]) |
| export default function useMailListHotkeys() { | ||
| const [removingEmails, setRemovingEmails] = useState<Set<string>>(new Set()); | ||
|
|
||
| return [removingEmails, setRemovingEmails] as const; | ||
| } |
There was a problem hiding this comment.
Duplicate hook implementation.
This hook implementation is identical to the one in apps/mail/hooks/use-mail-list-hotkeys.ts. Having two identical implementations creates confusion and maintenance issues.
Remove this duplicate and import the hook from the dedicated file:
- export default function useMailListHotkeys() {
- const [removingEmails, setRemovingEmails] = useState<Set<string>>(new Set());
-
- return [removingEmails, setRemovingEmails] as const;
- }
+ export { useMailListHotkeys as default } from './use-mail-list-hotkeys';…fic searches, improving search bar state management, and refining search value handling across components.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/mail/components/mail/search-bar.tsx (1)
79-88: Avoidanyin the form schema – use explicit types.
hasandfileNameare declared asany, defeating type‑safety. They appear to hold simple strings.- has: any; - fileName: any; + has: string; + fileName: string;Update the corresponding default values (
'') to maintain consistency.
🧹 Nitpick comments (4)
apps/mail/hooks/use-threads.ts (1)
102-107: Added debug logging for email fetching parameters.This console log provides visibility into the parameters being used for fetching emails, which is helpful for troubleshooting.
Consider using a conditional debug flag to control these logs in production:
- console.log('Fetching emails with params:', { + if (process.env.NODE_ENV !== 'production') { + console.log('Fetching emails with params:', { q: query, folder, max: max?.toString() ?? defaultPageSize.toString(), pageToken: pageToken ?? '', + }); });apps/mail/components/mail/search-bar.tsx (3)
249-265: Refactor duplicate search parsing logic.The error handling path contains very similar logic to the main search path. Consider extracting this to a shared helper function to avoid duplication.
+ const createSearchQuery = (searchTerm: string): string => { + const parsedTerm = parseNaturalLanguageSearch(searchTerm); + if (parsedTerm !== searchTerm) { + return parsedTerm; + } else { + if (searchTerm.includes('@')) { + return `from:${searchTerm}`; + } else { + return `(from:${searchTerm} OR from:"${searchTerm}" OR subject:"${searchTerm}" OR "${searchTerm}")`; + } + } + }; // In try block - const parsedTerm = parseNaturalLanguageSearch(searchTerm); - if (parsedTerm !== searchTerm) { - searchTerms.push(parsedTerm); - } else { - if (searchTerm.includes('@')) { - searchTerms.push(`from:${searchTerm}`); - } else { - searchTerms.push( - `(from:${searchTerm} OR from:"${searchTerm}" OR subject:"${searchTerm}" OR "${searchTerm}")`, - ); - } - } + searchTerms.push(createSearchQuery(searchTerm)); // In catch block - const parsedTerm = parseNaturalLanguageSearch(searchTerm); - if (parsedTerm !== searchTerm) { - searchTerms.push(parsedTerm); - } else { - if (searchTerm.includes('@')) { - searchTerms.push(`from:${searchTerm}`); - } else { - searchTerms.push( - `(from:${searchTerm} OR from:"${searchTerm}" OR subject:"${searchTerm}" OR "${searchTerm}")`, - ); - } - } + searchTerms.push(createSearchQuery(searchTerm));
234-240: Consider removing detailed debug logs before production.While valuable during development, these detailed logs should be conditional in production environments.
- console.log('Final search query:', { + if (process.env.NODE_ENV !== 'production') { + console.log('Final search query:', { value: searchQuery, highlight: data.q, folder: data.folder ? data.folder.toUpperCase() : '', isLoading: true, isAISearching: false, + }); });
727-934: Large commented-out code block should be removed.There's a substantial block of commented-out code related to the advanced search UI. If this functionality is no longer needed, the code should be removed to improve maintainability. If it might be needed later, consider moving it to a separate file or document.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/mail/app/api/driver/google.ts(1 hunks)apps/mail/components/mail/mail-list.tsx(9 hunks)apps/mail/components/mail/search-bar.tsx(11 hunks)apps/mail/hooks/use-search-value.ts(1 hunks)apps/mail/hooks/use-threads.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/mail/hooks/use-search-value.ts
- apps/mail/app/api/driver/google.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mail/components/mail/mail-list.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/mail/hooks/use-threads.ts (1)
apps/mail/lib/utils.ts (1)
defaultPageSize(143-143)
apps/mail/components/mail/search-bar.tsx (2)
apps/mail/hooks/use-search-value.ts (1)
useSearchValue(19-21)apps/mail/lib/utils.ts (2)
parseNaturalLanguageDate(496-544)parseNaturalLanguageSearch(455-494)
🔇 Additional comments (9)
apps/mail/hooks/use-threads.ts (1)
95-95: Refactored destructuring of thekeyparameter.The destructuring now uses placeholder commas to skip the first and fifth elements of the array rather than assigning them to specific variables. This is cleaner and indicates these values aren't needed in this scope.
apps/mail/components/mail/search-bar.tsx (8)
2-2: Switched to local natural language parsing utilities.Using local parsing functions instead of remote AI enhancement is a good architectural decision that reduces external dependencies and should improve response times.
24-26: Updated search suggestions with more helpful examples.The new suggestions better reflect the enhanced search capabilities, showing users the types of searches they can perform.
100-111: Added new form fields matching the expanded search capabilities.The state now tracks more field types, properly initializing them to empty strings and reflecting the SearchForm type definition.
178-221: Implemented local natural language parsing for search queries.The code now properly handles:
- Parsing natural language date queries
- Extracting date ranges and converting them to Gmail query format
- Cleaning up the query text after date extraction
- Processing special search patterns
This is a significant improvement that provides better search functionality without relying on external services.
224-229: Added support for additional search filters.New filters for folder, has, fileName, deliveredTo, and unicorn enrich the search capabilities. The implementation correctly formats each filter according to Gmail's query syntax.
295-307: Improved handling of quoted phrases in input.The code now detects when the cursor is inside quoted text and suppresses suggestions in this context. This provides a better user experience by not interrupting the user's flow when typing specific phrases.
694-694: Addedselect-noneclass to search input.This prevents accidental text selection when interacting with the search input, improving the user experience.
964-964: Added null check to prevent potential runtime error.The added check prevents accessing properties of an undefined match result, which could lead to runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/mail/components/mail/reply-composer.tsx (3)
37-37: Verify if useContacts hook is needed.The
useContactshook is imported but not actively used in the component. The filtered contact logic (lines 243-256) that would use it is commented out.If this refactoring intentionally removes the contacts functionality, you can remove this unused import:
-import { useContacts } from '@/hooks/use-contacts';
243-256: Consider removing or uncommenting the filtered contacts logic.These commented blocks are either leftover code or planned for future implementation. If they're not needed, consider removing them for cleaner code.
If you plan to use this functionality later, consider adding a TODO comment explaining why it's commented out.
949-949: Consider restoring keyboard shortcuts for sending email.Line 949 comments out the keyboard handler, and line 1188 updates the submit button to use handleSubmit(onSubmit). However, without the keyboard handler, users can't use Cmd/Ctrl+Enter to send emails.
Consider re-implementing the keyboard shortcut:
-// onKeyDown={handleKeyDown} +onKeyDown={(e) => { + if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { + e.preventDefault(); + if (!composerState.isLoading) { + handleSubmit(onSubmit)(); + } + } +}}Also applies to: 1188-1188
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/components/create/create-email.tsx(4 hunks)apps/mail/components/create/email-input.tsx(1 hunks)apps/mail/components/mail/reply-composer.tsx(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mail/components/create/email-input.tsx
- apps/mail/components/create/create-email.tsx
🔇 Additional comments (7)
apps/mail/components/mail/reply-composer.tsx (7)
216-218: LGTM - Form state tracking for input fields.These watch calls properly track the input field values which are needed for the EmailInput components.
220-229: Nice utility function for contact filtering.The function is well implemented with proper null checks and case insensitivity.
231-241: Well structured email handling function.This function properly validates emails, prevents duplicates, and maintains form state. Good practice to trim input and mark unsaved changes.
258-258: Good refactoring of the handleSendEmail function.Accepting form values as a parameter rather than using getValues() internally makes the function more explicit and testable.
Also applies to: 301-301
354-358: Improved async submission flow with loading state.Good implementation of loading state management that prevents concurrent submissions.
720-733: Great refactoring using the EmailInput component.Using the reusable EmailInput component for all recipient fields creates a more consistent UI and centralizes the recipient handling logic. This is a significant improvement to maintainability.
Also applies to: 736-749, 753-766
962-962: Good accessibility improvement.Adding
tabIndex={-1}to buttons that should be skipped in keyboard navigation improves accessibility.Also applies to: 983-983
| // // Add a function to handle starting the resize | ||
| // const handleResizeStart = (e: React.MouseEvent) => { | ||
| // setIsResizing(true); | ||
| // resizeStartY.current = e.clientY; | ||
| // startHeight.current = editorHeight; | ||
| // }; | ||
|
|
||
| // // Handle keyboard shortcuts for sending email | ||
| // const handleKeyDown = (e: React.KeyboardEvent) => { | ||
| // // Check for Cmd/Ctrl + Enter | ||
| // if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { | ||
| // e.preventDefault(); | ||
| // if (isFormValid) { | ||
| // handleSubmit(onSubmit)(); | ||
| // } | ||
| // } | ||
| // }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Reconsider commenting out these utility functions.
The editor resizing and keyboard shortcut handling functions have been commented out rather than removed. If these features are useful, consider reimplementing them.
🏁 Script executed:
#!/bin/bash
# Check if these features are implemented elsewhere
echo "Checking for editor resize and keyboard handling implementation elsewhere"
rg -A 5 "handleResizeStart|handleKeyDown.*Enter" --type tsxLength of output: 246
🏁 Script executed:
#!/bin/bash
# Retry search for handleResizeStart and handleKeyDown without specifying file types
echo "Searching for commented out utility functions across the codebase"
rg -A5 "handleResizeStart|handleKeyDown" .Length of output: 10768
Address commented-out utilities in reply-composer.tsx
The two local helpers—handleResizeStart (drag‐to‐resize) and handleKeyDown (Cmd/Ctrl + Enter send)—are currently only defined here and have no live implementation elsewhere. Please either fully remove these commented blocks or restore them properly so the reply editor supports:
- Drag‐to‐resize functionality
- Cmd/Ctrl + Enter keyboard shortcut for sending
Location to update:
apps/mail/components/mail/reply-composer.tsxlines ~487–503
- // const handleResizeStart = (e: React.MouseEvent) => {
- // setIsResizing(true);
- // resizeStartY.current = e.clientY;
- // startHeight.current = editorHeight;
- // };
+ // TODO: re-enable or remove drag‐to‐resize logic here
- // const handleKeyDown = (e: React.KeyboardEvent) => {
- // if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') {
- // e.preventDefault();
- // if (isFormValid) {
- // handleSubmit(onSubmit)();
- // }
- // }
- // };
+ // TODO: re-enable or remove Cmd/Ctrl+Enter send shortcut here📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // // Add a function to handle starting the resize | |
| // const handleResizeStart = (e: React.MouseEvent) => { | |
| // setIsResizing(true); | |
| // resizeStartY.current = e.clientY; | |
| // startHeight.current = editorHeight; | |
| // }; | |
| // // Handle keyboard shortcuts for sending email | |
| // const handleKeyDown = (e: React.KeyboardEvent) => { | |
| // // Check for Cmd/Ctrl + Enter | |
| // if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') { | |
| // e.preventDefault(); | |
| // if (isFormValid) { | |
| // handleSubmit(onSubmit)(); | |
| // } | |
| // } | |
| // }; | |
| // TODO: re-enable or remove drag‐to‐resize logic here | |
| // TODO: re-enable or remove Cmd/Ctrl+Enter send shortcut here |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mail/components/mail/search-bar.tsx (1)
85-88: Avoidany– keep the form schema type‑safe
hasandfileNameare still typed asany.
This silently disables type‑checking and was already pointed out in a previous review.
Declare them asstring(or a dedicated union if the set of allowed values is finite).
🧹 Nitpick comments (1)
apps/mail/components/mail/search-bar.tsx (1)
292-305: Quoted‑phrase detection mis‑identifies “inside quotes”The current check counts how many fragments the substring before the last quote splits into:
const isOpeningQuote = textBeforeCursor.slice(0, lastQuoteIndex).split('"').length % 2 === 0;This returns
falseforHe said "hello(only one quote so far), meaning the algorithm thinks we are outside a quoted phrase, while we are actually inside.A simpler & reliable approach is to count quotes directly:
const quotesBeforeCursor = (textBeforeCursor.match(/"/g) ?? []).length; const insideQuotedPhrase = quotesBeforeCursor % 2 === 1;Then suppress suggestions when
insideQuotedPhraseis true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/mail/app/api/driver/google.ts(2 hunks)apps/mail/components/mail/search-bar.tsx(12 hunks)apps/mail/components/ui/nav-main.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mail/app/api/driver/google.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/components/mail/search-bar.tsx (2)
apps/mail/hooks/use-search-value.ts (1)
useSearchValue(19-21)apps/mail/lib/utils.ts (2)
parseNaturalLanguageDate(496-544)parseNaturalLanguageSearch(455-494)
🔇 Additional comments (4)
apps/mail/components/ui/nav-main.tsx (4)
22-22: Good addition of the nuqs library for query state management.Adding
useQueryStatefrom nuqs provides a more reactive approach to managing URL query parameters compared to directly accessing them fromuseSearchParams.
59-59: Great refactoring to reactive query state.Replacing the direct access to search parameters with
useQueryStatecreates a reactive binding to the URL state, which improves state synchronization across the component.
121-123: Clean simplification of URL construction logic.The conditional logic for appending the category parameter has been simplified to always append it when present, which standardizes URL construction for navigation links across the application.
127-127: Correctly updated dependency array.Adding
categoryto the dependency array ofuseCallbackensures the function is properly re-memoized when the category changes, maintaining consistency with the reactive pattern.
| let searchQuery = searchTerms.join(' '); | ||
| searchQuery = extractMetaText(searchQuery) || ''; | ||
|
|
||
| searchQuery = extractMetaText(searchQuery) || '' | ||
|
|
||
| const folder = data.folder ? data.folder.toUpperCase() : ''; | ||
| console.log('Final search query:', { | ||
| value: searchQuery, | ||
| highlight: data.q, | ||
| folder: data.folder ? data.folder.toUpperCase() : '', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
extractMetaText wipes out the composed Gmail query
extractMetaText is a helper for parsing AI chat responses, but here it’s invoked on the final query that you just built from the user form.
For inputs like
after:2024/03/01 before:2024/03/31 from:alice@example.com
the fallback branch inside extractMetaText reduces the string to only after: (see the ^.*?(from:|to:|subject:|is:|has:|after:|before:) replace).
That corrupts the query before it is stored in global state, leading to empty or broken search results.
Proposed fix: skip the call entirely for locally‑constructed queries.
- let searchQuery = searchTerms.join(' ');
- searchQuery = extractMetaText(searchQuery) || '';
+ const searchQuery = searchTerms.join(' ');(You can still keep the helper for the AI‑flow, but behind an explicit flag.)
📝 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.
| let searchQuery = searchTerms.join(' '); | |
| searchQuery = extractMetaText(searchQuery) || ''; | |
| searchQuery = extractMetaText(searchQuery) || '' | |
| const folder = data.folder ? data.folder.toUpperCase() : ''; | |
| console.log('Final search query:', { | |
| value: searchQuery, | |
| highlight: data.q, | |
| folder: data.folder ? data.folder.toUpperCase() : '', | |
| // build the query directly without AI‐flow parsing | |
| const searchQuery = searchTerms.join(' '); | |
| console.log('Final search query:', { | |
| value: searchQuery, | |
| highlight: data.q, | |
| folder: data.folder ? data.folder.toUpperCase() : '', | |
| }); |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores