Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces a new hook, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailList as MailList Component
User->>MailList: Presses Ctrl/Shift/Alt key (keydown)
MailList-->>MailList: Set corresponding key state to true
User->>MailList: Releases key (keyup)/Window loses focus (blur)
MailList-->>MailList: Reset key state to false
User->>MailList: Clicks on a mail item
MailList-->>MailList: Check key states and update selection logic accordingly
Possibly related PRs
Suggested reviewers
Poem
✨ 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: 0
🧹 Nitpick comments (3)
apps/mail/components/mail/mail-list.tsx (3)
391-428: Event handler implementation looks good but consider refactoring for DRY.The key event handlers are well implemented, but there's some duplication in logic between
handleKeyDown,handleKeyUp, andhandleBlur.Consider extracting a helper function to reset all key states:
+ const resetKeyStates = () => { + isCtrlKeyPressed.current = false; + isShiftKeyPressed.current = false; + isAltKeyPressed.current = false; + }; const handleKeyUp = (e: KeyboardEvent) => { if (e.key === 'Control' || e.key === 'Meta') { - isCtrlKeyPressed.current = false; + resetKeyStates(); setMassSelectMode(false); } // ... similar for other keys }; const handleBlur = () => { - isCtrlKeyPressed.current = false; - isShiftKeyPressed.current = false; - isAltKeyPressed.current = false; + resetKeyStates(); setMassSelectMode(false); setRangeSelectMode(false); setSelectAllBelowMode(false); };
455-499: Mail click handler refactoring looks good but consider consistency.The refactoring to use key refs directly in the click handler is a good approach as it directly checks the current key state. However, there's an inconsistency between how selection mode is determined here versus the
selectModevariable computed on lines 445-451.Consider one of these approaches for consistency:
- Remove the state variables and
selectModecomputation entirely if not needed for UI rendering- Update the
selectModecomputation to use the refs instead of state variables- Create a helper function that determines the selection mode based on current key states
// Option 2: Update selectMode computation const selectMode: MailSelectMode = isCtrlKeyPressed.current ? 'mass' : isShiftKeyPressed.current ? 'range' : (isAltKeyPressed.current && isShiftKeyPressed.current) ? 'selectAllBelow' : 'single';
445-451: Consider updating selectMode computation for consistency.While not part of the changed code, the
selectModecomputation is now inconsistent with the actual behavior in thehandleMailClickfunction. The selection mode is now determined by the key refs, but this computation still uses the state variables.Update the computation to use the key refs for consistency:
const selectMode: MailSelectMode = isCtrlKeyPressed.current ? 'mass' : isShiftKeyPressed.current ? 'range' : (isAltKeyPressed.current && isShiftKeyPressed.current) ? 'selectAllBelow' : 'single';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/mail-list.tsx(6 hunks)
🔇 Additional comments (3)
apps/mail/components/mail/mail-list.tsx (3)
282-284: Good addition of key state tracking with useRef.The introduction of these refs to track key states is a good approach as it provides more responsive handling of key combinations compared to state variables, which can be subject to batching delays.
432-443: Good cleanup implementation.The cleanup function properly removes event listeners and resets all state variables. This is important to prevent memory leaks and unexpected behavior when the component unmounts.
519-520: Dependency array updated correctly.Good job updating the dependency array to include
itemsinstead ofselectMode. This is appropriate since the function now depends onitemsfor range selection and no longer uses theselectModevariable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/mail/hooks/use-hot-key.ts (1)
21-23: Implement cleanup for event listeners in the hook.The
useKeyStatehook correctly returns a memoized callback function to check key states, but it should also initialize the event listeners when first used and ensure they're properly cleaned up on component unmount.export function useKeyState() { + useEffect(() => { + initializeKeyListeners(); + }, []); return useCallback((key: string) => keyStates.get(key) || false, []); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/components/mail/mail-list.tsx(7 hunks)apps/mail/hooks/use-hot-key.ts(1 hunks)
🔇 Additional comments (8)
apps/mail/hooks/use-hot-key.ts (1)
3-4: Global state management implementation.A new
Mapis implemented to track keyboard key states globally across the application.apps/mail/components/mail/mail-list.tsx (7)
142-146: Good prevention of default behavior for Ctrl/Cmd clicking.This is a good addition to prevent browser default behavior (like text selection) when using Ctrl+click or Cmd+click for mail selection.
284-284: Implementing the new key state hook.The new approach using
useKeyStatehook will provide a more centralized way to track keyboard states.
376-387: Clean implementation of selection mode determination.The
getSelectModefunction effectively simplifies the selection logic by dynamically determining the mode based on current key states, which is much cleaner than maintaining multiple state variables.
390-392: Simplified mail click handling.Using the
getSelectModefunction to determine the selection mode on demand when a mail is clicked is an excellent improvement that removes the need to track and update selection mode state variables.
451-451: Updated dependency array in useCallback.The dependency array for
handleMailClickhas been properly updated to include thegetSelectModefunction.
473-473: Dynamically applying select-none class based on Shift key state.This is a good optimization to disable text selection when using Shift key for range selection.
496-496: Dynamically determining selection mode for Thread component.The
selectModeprop is now dynamically determined using thegetSelectModefunction, which ensures the Thread component always has the current selection mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/components/mail/mail-list.tsx (1)
497-497: Consider memoizing the selection mode value.While the getSelectMode function itself is memoized, calling it directly in the JSX might cause unnecessary recalculations during renders. Consider storing the result in a memoized value.
-selectMode={getSelectMode()} +selectMode={selectMode}With this addition above the return statement:
const selectMode = useMemo(() => getSelectMode(), [getSelectMode]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/mail-list.tsx(8 hunks)
🔇 Additional comments (8)
apps/mail/components/mail/mail-list.tsx (8)
37-37: Good addition of the useKeyState hook.Using a centralized hook for key state management is a good approach for handling keyboard interactions consistently across the application.
71-71: Good implementation of key state in the Thread component.This allows for more responsive UI interactions based on keyboard state.
143-147: Proper handling of modifier keys during mouse events.Preventing the default browser behavior when Control or Meta keys are pressed is important to avoid text selection while performing multi-selection operations.
285-285: Consistent use of useKeyState hook in the MailList component.This maintains consistency with the pattern established in the Thread component.
377-388: Well-structured selection mode logic.The getSelectMode implementation elegantly handles different keyboard combinations and follows a clear priority order for selection modes:
- Control/Meta for mass selection
- Shift for range selection
- Alt+Shift for selecting all below
- Default to single selection
This is a significant improvement over managing separate state variables.
392-393: Clean implementation of dynamic selection mode.Getting the selection mode at the moment of click ensures the most up-to-date keyboard state is used for determining selection behavior.
474-474: Good UX enhancement to prevent text selection.Adding the 'select-none' class when Shift is pressed prevents accidental text selection during range selection operations, which improves the user experience.
452-452: Updated dependency array to include getSelectMode.Good job including getSelectMode in the dependency array of handleMailClick's useCallback. This ensures the callback is properly updated when the key detection logic changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/hooks/use-hot-key.ts (1)
47-49: Consider consolidating initialization logic.The
initKeyListeners()is called in bothuseKeyStateanduseHotKey. While this redundancy ensures listeners are initialized regardless of which hook is used, it would be cleaner to have a single source of initialization.Consider creating a shared initialization hook or moving all initialization to the
initKeyListenersfunction with a module-level call:let listenersInit = false; function initKeyListeners() { if (typeof window !== 'undefined' && !listenersInit) { // Event listener setup... listenersInit = true; } } +// Initialize once at module level +if (typeof window !== 'undefined') { + // Use setTimeout to ensure this runs after module evaluation + setTimeout(() => initKeyListeners(), 0); +} export function useKeyState() { - useEffect(() => { - initKeyListeners(); - }, []); return useCallback((key: string) => keyStates.get(key) || false, []); } export const useHotKey = ( // ... ) => { // ... - useEffect(() => { - initKeyListeners(); - }, []); // ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/hooks/use-hot-key.ts(2 hunks)
🔇 Additional comments (4)
apps/mail/hooks/use-hot-key.ts (4)
3-4: Global state introduction looks good.Introducing a Map to track key states is a clean approach for maintaining global keyboard state information.
5-6: Flag to prevent duplicate initialization is appropriate.Using a boolean flag to ensure listeners are only initialized once is a good practice to prevent memory leaks from multiple event listeners.
27-33: Well-implemented hook for key state.The
useKeyStatehook is well-designed with proper initialization in useEffect and a memoized getter using useCallback to prevent unnecessary re-renders.
7-25:⚠️ Potential issueFix parameter order in blur event handler.
There's a bug in the blur event handler. The Map's forEach method provides value first, then key:
forEach(value, key), but your code is using the parameters in reverse order.window.addEventListener('blur', () => { - keyStates.forEach((_, key) => { + keyStates.forEach((value, key) => { keyStates.set(key, false); }); });Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/hooks/use-hot-key.ts (1)
27-29: Consider using useEffect for initialization.While setTimeout works, it's more React-idiomatic to initialize listeners in a useEffect hook inside a component that first needs this functionality.
-if (typeof window !== 'undefined') { - setTimeout(() => initKeyListeners(), 0); -} +// Move initialization to components that use the hook +// Then inside the component: +// useEffect(() => { +// if (typeof window !== 'undefined') { +// initKeyListeners(); +// } +// }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/mail/app/(auth)/login/login-client.tsx(1 hunks)apps/mail/components/create/create-email.tsx(1 hunks)apps/mail/components/draft/drafts.tsx(1 hunks)apps/mail/components/mail/mail-list.tsx(8 hunks)apps/mail/components/ui/nav-main.tsx(1 hunks)apps/mail/hooks/use-hot-key.ts(2 hunks)apps/mail/hooks/use-notes.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- apps/mail/components/draft/drafts.tsx
- apps/mail/components/ui/nav-main.tsx
- apps/mail/components/create/create-email.tsx
- apps/mail/hooks/use-notes.tsx
- apps/mail/app/(auth)/login/login-client.tsx
🔇 Additional comments (8)
apps/mail/hooks/use-hot-key.ts (3)
3-6: Good approach for tracking key states.Using a global Map to track keyboard states and a flag to prevent multiple listener initializations is a clean approach.
7-25: Event listener implementation looks good.The implementation properly handles keydown, keyup, and blur events to maintain accurate key states and cleans up when the window loses focus.
31-33: Simple and effective hook implementation.The useKeyState hook provides a clean API for components to check key states. The useCallback ensures stable references.
apps/mail/components/mail/mail-list.tsx (5)
143-147: Nice handling of modifier key interactions.Preventing default behavior when Control or Meta keys are pressed avoids text selection issues during multi-select operations.
377-388: Excellent refactoring of selection mode logic.Replacing multiple state variables with a single function that dynamically determines the selection mode based on key states is a clean improvement that reduces state management complexity.
392-393: Good integration with handleMailClick.Using getSelectMode() to determine the selection mode on demand ensures accurate handling based on the current key state.
474-474: Smart UI enhancement for selection experience.Adding select-none class when Shift is pressed prevents text selection during range selection operations, improving the user experience.
497-497: Consistent application of selection mode.Passing the dynamically determined selection mode to the Thread component ensures consistent behavior throughout the component tree.
2 participants
Summary by CodeRabbit
New Features
useKeyState, for simplified key state management across components.Chores