added shortcuts in /mail #392
Conversation
|
@Saksham1387 is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
|
WalkthroughThe changes add keyboard navigation to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmailList
participant KeyboardNav
User->>EmailList: Press key (R, E, or Arrow)
EmailList->>KeyboardNav: Handle key event via hook
KeyboardNav->>EmailList: Trigger action (reply/archive or update focus)
EmailList->>User: Update UI with focused thread/action executed
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 2 workspace projects This error happened while installing a direct dependency of /tmp/eslint/packages/eslint-config ✨ Finishing Touches
🪧 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/web/components/email-list/EmailList.tsx (2)
387-397: Consider adding visual confirmation for keyboard shortcutsWhile the keyboard shortcuts work well, users might benefit from a visual confirmation (e.g., a toast notification) when using the 'R' shortcut for reply.
Consider adding a toast notification:
if (e.key === "r" || e.key === "R") { if (isCmdOrCtrl) { e.preventDefault(); } const thread = threads[focusedIndex]; if (thread) { setOpenThreadId(thread.id); markReadThreads([thread.id], () => refetch()); scrollToId(thread.id); + toast.info("Replying to email"); } }
373-375: Consider adding support for contentEditable elementsThe keyboard event handler checks for INPUT and TEXTAREA elements, but it doesn't check for contentEditable elements, which might be used in rich text components.
if ( document.activeElement?.tagName === "INPUT" || document.activeElement?.tagName === "TEXTAREA" || + document.activeElement?.getAttribute("contenteditable") === "true" ) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/email-list/EmailList.tsx(4 hunks)apps/web/components/email-list/EmailListItem.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/components/email-list/EmailList.tsx
[error] 369-369: This hook does not specify all of its dependencies: scrollToId
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
🔇 Additional comments (7)
apps/web/components/email-list/EmailListItem.tsx (3)
29-29: LGTM: Added focused prop to EmailListItemThe optional
focusedprop correctly extends the component's interface to support keyboard navigation state.
79-81: Hover styling correctly handles focused stateThe conditional styling for hover effects now properly considers the focused state, ensuring appropriate visual feedback during keyboard navigation.
85-85: Good visual indication for keyboard focusThe z-index and ring styling provide clear visual feedback for the currently focused item during keyboard navigation, improving accessibility.
apps/web/components/email-list/EmailList.tsx (4)
367-368: LGTM: Added focusedIndex state for keyboard navigationThe state variable is correctly initialized to track the currently focused email in the list.
370-408: Well-implemented keyboard shortcutsThe implementation of keyboard shortcuts is clean and follows good practices:
- Proper checks to avoid interfering with input fields
- Clear usage of arrow keys for navigation
- Command/Control + 'R' for reply (opens the thread)
- Command/Control + 'E' for archive
This directly addresses the PR objectives to enhance keyboard navigation.
493-493: Correctly passing index to map functionAdding the index parameter to the map function enables tracking the currently focused item.
505-505: Properly passing focused state to EmailListItemThe focused state is correctly passed to the EmailListItem component based on the focusedIndex.
| useEffect(() => { | ||
| function handleKeyDown(e: KeyboardEvent) { | ||
| if ( | ||
| document.activeElement?.tagName === "INPUT" || | ||
| document.activeElement?.tagName === "TEXTAREA" | ||
| ) | ||
| return; | ||
|
|
||
| const isCmdOrCtrl = e.metaKey || e.ctrlKey; | ||
|
|
||
| if (e.key === "ArrowDown") { | ||
| setFocusedIndex((prev) => Math.min(prev + 1, threads.length - 1)); | ||
| } | ||
|
|
||
| if (e.key === "ArrowUp") { | ||
| setFocusedIndex((prev) => Math.max(prev - 1, 0)); | ||
| } | ||
|
|
||
| if (e.key === "r" || e.key === "R") { | ||
| if (isCmdOrCtrl) { | ||
| e.preventDefault(); | ||
| } | ||
| const thread = threads[focusedIndex]; | ||
| if (thread) { | ||
| setOpenThreadId(thread.id); | ||
| markReadThreads([thread.id], () => refetch()); | ||
| scrollToId(thread.id); | ||
| } | ||
| } | ||
|
|
||
| if (e.key === "e" || e.key === "E") { | ||
| if (isCmdOrCtrl) { | ||
| e.preventDefault(); | ||
| } | ||
| const thread = threads[focusedIndex]; | ||
| if (thread) { | ||
| onArchive(thread); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener("keydown", handleKeyDown); | ||
| return () => window.removeEventListener("keydown", handleKeyDown); | ||
| }, [threads, focusedIndex, setOpenThreadId, onArchive, refetch]); |
There was a problem hiding this comment.
Missing dependency in useEffect dependency array
The scrollToId function is used inside the effect but is not included in the dependency array, which could lead to stale closures.
Update the dependency array to include all dependencies:
- }, [threads, focusedIndex, setOpenThreadId, onArchive, refetch]);
+ }, [threads, focusedIndex, setOpenThreadId, onArchive, refetch, scrollToId]);📝 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.
| useEffect(() => { | |
| function handleKeyDown(e: KeyboardEvent) { | |
| if ( | |
| document.activeElement?.tagName === "INPUT" || | |
| document.activeElement?.tagName === "TEXTAREA" | |
| ) | |
| return; | |
| const isCmdOrCtrl = e.metaKey || e.ctrlKey; | |
| if (e.key === "ArrowDown") { | |
| setFocusedIndex((prev) => Math.min(prev + 1, threads.length - 1)); | |
| } | |
| if (e.key === "ArrowUp") { | |
| setFocusedIndex((prev) => Math.max(prev - 1, 0)); | |
| } | |
| if (e.key === "r" || e.key === "R") { | |
| if (isCmdOrCtrl) { | |
| e.preventDefault(); | |
| } | |
| const thread = threads[focusedIndex]; | |
| if (thread) { | |
| setOpenThreadId(thread.id); | |
| markReadThreads([thread.id], () => refetch()); | |
| scrollToId(thread.id); | |
| } | |
| } | |
| if (e.key === "e" || e.key === "E") { | |
| if (isCmdOrCtrl) { | |
| e.preventDefault(); | |
| } | |
| const thread = threads[focusedIndex]; | |
| if (thread) { | |
| onArchive(thread); | |
| } | |
| } | |
| } | |
| window.addEventListener("keydown", handleKeyDown); | |
| return () => window.removeEventListener("keydown", handleKeyDown); | |
| }, [threads, focusedIndex, setOpenThreadId, onArchive, refetch]); | |
| useEffect(() => { | |
| function handleKeyDown(e: KeyboardEvent) { | |
| if ( | |
| document.activeElement?.tagName === "INPUT" || | |
| document.activeElement?.tagName === "TEXTAREA" | |
| ) | |
| return; | |
| const isCmdOrCtrl = e.metaKey || e.ctrlKey; | |
| if (e.key === "ArrowDown") { | |
| setFocusedIndex((prev) => Math.min(prev + 1, threads.length - 1)); | |
| } | |
| if (e.key === "ArrowUp") { | |
| setFocusedIndex((prev) => Math.max(prev - 1, 0)); | |
| } | |
| if (e.key === "r" || e.key === "R") { | |
| if (isCmdOrCtrl) { | |
| e.preventDefault(); | |
| } | |
| const thread = threads[focusedIndex]; | |
| if (thread) { | |
| setOpenThreadId(thread.id); | |
| markReadThreads([thread.id], () => refetch()); | |
| scrollToId(thread.id); | |
| } | |
| } | |
| if (e.key === "e" || e.key === "E") { | |
| if (isCmdOrCtrl) { | |
| e.preventDefault(); | |
| } | |
| const thread = threads[focusedIndex]; | |
| if (thread) { | |
| onArchive(thread); | |
| } | |
| } | |
| } | |
| window.addEventListener("keydown", handleKeyDown); | |
| return () => window.removeEventListener("keydown", handleKeyDown); | |
| }, [threads, focusedIndex, setOpenThreadId, onArchive, refetch, scrollToId]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 369-369: This hook does not specify all of its dependencies: scrollToId
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
|
would take a look at how we do this in other places in the codebase to keep consistency.
|
|
I made the required changes and changed the implementation same as it is been done in other places |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/components/email-list/EmailList.tsx (3)
368-411: Consider removing commented-out codeThe PR has left a significant block of commented-out code. While this can be helpful during development and testing, it's generally a good practice to remove commented-out code before merging to production to keep the codebase clean.
413-413: Remove unused state declarationThis commented-out line can be removed along with the other commented code to maintain cleanliness.
595-613: Consider adding prop to disable keyboard navigation in certain contextsFor better user experience, consider adding a prop to disable keyboard navigation when the user is:
- Focusing on input fields outside of the email list
- Using keyboard shortcuts that might conflict with the email navigation
This would prevent unexpected actions when users are interacting with other parts of the interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/components/email-list/EmailList.tsx(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/web/components/email-list/EmailList.tsx
[error] 415-415: This hook does not specify all of its dependencies: onArchive
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 415-415: This hook does not specify all of its dependencies: refetch
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 415-415: This hook does not specify all of its dependencies: scrollToId
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
🔇 Additional comments (4)
apps/web/components/email-list/EmailList.tsx (4)
595-613: Good implementation of the keyboard navigation hookThe implementation of
useEmailListKeyboardNavis clean and follows best practices by:
- Delegating to the reusable
useTableKeyboardNavigationhook- Using a callback function (
handleKeyAction) with proper dependency array- Maintaining a clear separation of concerns
This approach makes the code more maintainable and reusable.
431-431: Effective integration of keyboard navigationGood job integrating the custom hook with the existing component. This cleanly replaces the previous commented-out implementation with a more modular approach.
512-513: Correctly passing index to map function and using it for selection stateThe addition of the index parameter to the map function and using it to determine the selected state in
EmailListItemis the right approach for implementing keyboard navigation.Also applies to: 537-537
3-3: Ensure imports are properly usedThe import line has been updated to add new hooks. Make sure all imported hooks are being used in the component; otherwise, they should be removed.
| const handleAction = useCallback( | ||
| async (index: number, action: "reply" | "archive") => { | ||
| const thread = threads[index]; | ||
| if (!thread) return; | ||
|
|
||
| if (action === "reply") { | ||
| setOpenThreadId(thread.id); | ||
| markReadThreads([thread.id], () => refetch()); | ||
| scrollToId(thread.id); | ||
| } else if (action === "archive") { | ||
| onArchive(thread); | ||
| } | ||
| }, | ||
| [threads, setOpenThreadId], | ||
| ); |
There was a problem hiding this comment.
Missing dependencies in useCallback dependency array
The handleAction callback has missing dependencies in its dependency array:
- }, [threads, setOpenThreadId],
+ }, [threads, setOpenThreadId, onArchive, refetch, scrollToId],These dependencies are used inside the callback but not included in the dependency array, which could lead to stale closures and unexpected behavior.
📝 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 handleAction = useCallback( | |
| async (index: number, action: "reply" | "archive") => { | |
| const thread = threads[index]; | |
| if (!thread) return; | |
| if (action === "reply") { | |
| setOpenThreadId(thread.id); | |
| markReadThreads([thread.id], () => refetch()); | |
| scrollToId(thread.id); | |
| } else if (action === "archive") { | |
| onArchive(thread); | |
| } | |
| }, | |
| [threads, setOpenThreadId], | |
| ); | |
| const handleAction = useCallback( | |
| async (index: number, action: "reply" | "archive") => { | |
| const thread = threads[index]; | |
| if (!thread) return; | |
| if (action === "reply") { | |
| setOpenThreadId(thread.id); | |
| markReadThreads([thread.id], () => refetch()); | |
| scrollToId(thread.id); | |
| } else if (action === "archive") { | |
| onArchive(thread); | |
| } | |
| }, | |
| [threads, setOpenThreadId, onArchive, refetch, scrollToId], | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 415-415: This hook does not specify all of its dependencies: onArchive
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 415-415: This hook does not specify all of its dependencies: refetch
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 415-415: This hook does not specify all of its dependencies: scrollToId
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
Fixes: #343
I have added the mentioned shortcuts
E --> archive
R --> Reply
and have also added Arrow up/down
Summary by CodeRabbit