better command palette and new shortcuts#1165
Conversation
|
""" WalkthroughThe changes refactor the command palette system in the mail application. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MailComponent
participant CommandPaletteContext
participant CommandPaletteProvider
participant GlobalHotkeys
User->>MailComponent: Clicks "Search & Filters" button or uses hotkey
MailComponent->>CommandPaletteContext: setOpen(true)
CommandPaletteContext->>CommandPaletteProvider: Opens command palette UI
User->>GlobalHotkeys: Presses 'mod+shift+f'
GlobalHotkeys->>CommandPaletteContext: clearAllFilters()
CommandPaletteContext->>MailComponent: Filters are cleared
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🔭 Outside diff range comments (1)
apps/mail/components/context/command-palette-context.tsx (1)
211-233: 💡 Verification agent❓ Verification inconclusive
Check useEffect dependencies for completeness.
The
useEffecthook usessearchValuebut doesn't include it in the dependency array. This could lead to stale closures.
🏁 Script executed:
#!/bin/bash # Description: Check if searchValue is used elsewhere in similar patterns # This will help verify if the current pattern is intentional or an oversight rg -A 5 -B 5 "setSearchValue.*searchValue" apps/mail/Length of output: 55
Use a functional state update to avoid a stale
searchValueclosureIn the effect that loads active filters (apps/mail/components/context/command-palette-context.tsx:211–233), you’re spreading the outer
searchValuebut not including it in deps. Instead of adding it to the dependency array (which would re-run the effect on every search change), switch to the functional form ofsetSearchValueso you always get the latest state without extra deps:• File: apps/mail/components/context/command-palette-context.tsx
Lines: 211–233Suggested diff:
- if (query) { - setSearchValue({ - ...searchValue, - value: query, - highlight: getMainSearchTerm(query), - }); - } + if (query) { + setSearchValue(prev => ({ + ...prev, + value: query, + highlight: getMainSearchTerm(query), + })); + }This preserves the empty dependency array while avoiding stale closures.
🧹 Nitpick comments (3)
apps/mail/components/mail/mail.tsx (2)
529-570: Consider refactoring complex button implementation for better maintainability.While the functionality is correct, this button implementation contains significant complexity that could be improved:
- Complex conditional rendering logic could be extracted into helper functions
- The responsive text logic could be simplified
- Consider extracting the clear button into a separate component
+const getButtonText = (activeFilters: any[], isMobile: boolean) => { + if (activeFilters.length > 0) { + return isMobile + ? `${activeFilters.length} filter${activeFilters.length > 1 ? 's' : ''}` + : activeFilters.map((f) => f.display).join(', '); + } + return isMobile ? 'Search...' : 'Search & Filters'; +}; <Button variant="outline" className={cn( 'text-muted-foreground relative flex h-9 w-full select-none items-center justify-start overflow-hidden rounded-[0.5rem] border bg-white text-left text-sm font-normal shadow-none ring-0 focus-visible:ring-0 focus-visible:ring-offset-0 dark:bg-[#141414]', )} onClick={() => setOpen(!open)} > - <span className="hidden truncate pr-20 lg:inline-block"> - {activeFilters.length > 0 - ? activeFilters.map((f) => f.display).join(', ') - : 'Search & Filters'} - </span> - <span className="inline-block truncate pr-20 lg:hidden"> - {activeFilters.length > 0 - ? `${activeFilters.length} filter${activeFilters.length > 1 ? 's' : ''}` - : 'Search...'} - </span> + <span className="hidden truncate pr-20 lg:inline-block"> + {getButtonText(activeFilters, false)} + </span> + <span className="inline-block truncate pr-20 lg:hidden"> + {getButtonText(activeFilters, true)} + </span>
566-568: Consider making keyboard shortcut display platform-aware.The hardcoded "⌘ K" shortcut hint may not be appropriate for Windows users who would expect "Ctrl K".
Consider using a utility function to display the correct modifier key based on the platform:
-<kbd className="bg-muted pointer-events-none hidden h-5 select-none items-center gap-1 rounded border px-1.5 font-mono text-[10px] font-medium opacity-100 sm:flex"> - <span className="text-sm">⌘</span> K -</kbd> +<kbd className="bg-muted pointer-events-none hidden h-5 select-none items-center gap-1 rounded border px-1.5 font-mono text-[10px] font-medium opacity-100 sm:flex"> + <span className="text-sm">{navigator.platform.includes('Mac') ? '⌘' : 'Ctrl'}</span> K +</kbd>apps/mail/components/context/command-palette-context.tsx (1)
183-1886: Consider breaking down this large component for better maintainability.The
CommandPalettecomponent is over 1700 lines long, making it difficult to maintain and test. Consider extracting the different views (search, filter, labels, etc.) into separate components.Consider this structure:
command-palette/ ├── CommandPalette.tsx (main component with context provider) ├── views/ │ ├── MainView.tsx │ ├── SearchView.tsx │ ├── FilterView.tsx │ ├── LabelsView.tsx │ ├── SavedSearchesView.tsx │ ├── FilterBuilderView.tsx │ └── HelpView.tsx ├── hooks/ │ └── useCommandPalette.ts └── utils/ └── localStorage.tsThis would improve:
- Code organization and navigation
- Testability of individual views
- Team collaboration (less merge conflicts)
- Performance (potentially with React.lazy for views)
🧰 Tools
🪛 Biome (1.9.4)
[error] 424-424: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 598-598: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 957-958: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1416-1417: 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 (5)
apps/mail/app/(routes)/layout.tsx(1 hunks)apps/mail/components/context/command-palette-context.tsx(6 hunks)apps/mail/components/mail/mail.tsx(3 hunks)apps/mail/config/shortcuts.ts(1 hunks)apps/mail/lib/hotkeys/global-hotkeys.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/mail/lib/hotkeys/global-hotkeys.tsx (1)
apps/mail/components/context/command-palette-context.tsx (1)
useCommandPalette(123-129)
apps/mail/app/(routes)/layout.tsx (2)
apps/mail/components/context/command-palette-context.tsx (1)
CommandPaletteProvider(1888-1894)apps/mail/components/providers/hotkey-provider-wrapper.tsx (1)
HotkeyProviderWrapper(13-24)
apps/mail/components/mail/mail.tsx (2)
apps/mail/components/context/command-palette-context.tsx (1)
useCommandPalette(123-129)apps/mail/lib/utils.ts (1)
cn(51-51)
🔇 Additional comments (10)
apps/mail/config/shortcuts.ts (1)
143-150: LGTM! Well-structured shortcut addition.The new
clearAllFiltersshortcut follows the established pattern and uses an appropriate key combination. ThepreventDefault: trueflag correctly prevents browser default behavior.apps/mail/app/(routes)/layout.tsx (2)
2-2: LGTM! Proper context provider integration.The import and provider wrapping follows React context best practices.
7-13: Good provider hierarchy design.Wrapping the entire layout with
CommandPaletteProvideroutsideHotkeyProviderWrapperensures that hotkey handlers can access the command palette context throughout the app.apps/mail/lib/hotkeys/global-hotkeys.tsx (3)
1-1: Correct hook import for command palette integration.The import follows the established pattern for accessing context via hooks.
8-8: Proper context consumption.Destructuring the required functions from the command palette context is the correct approach.
13-14: Well-implemented hotkey handlers.The handlers correctly call the corresponding context functions and follow the established pattern.
apps/mail/components/mail/mail.tsx (2)
29-29: Correct context hook import.The import follows the established pattern for accessing command palette context.
392-392: Proper context consumption.Destructuring all the required state and functions from the command palette context is appropriate.
apps/mail/components/context/command-palette-context.tsx (2)
68-74: Context type updates look good!The addition of
activeFiltersandclearAllFiltersto the context type properly exposes the filter state and management functionality.
802-802: Nice UX improvement on the Clear All button!The addition of hover effects and muted text styling provides better visual feedback to users.
| <Suspense> | ||
| <CommandPalette>{children}</CommandPalette> | ||
| </Suspense> |
There was a problem hiding this comment.
Add a fallback to the Suspense wrapper.
The Suspense component should have a fallback to show while the component is loading, otherwise users might see a blank screen.
- <Suspense>
+ <Suspense fallback={<div>Loading command palette...</div>}>
<CommandPalette>{children}</CommandPalette>
</Suspense>📝 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.
| <Suspense> | |
| <CommandPalette>{children}</CommandPalette> | |
| </Suspense> | |
| <Suspense fallback={<div>Loading command palette...</div>}> | |
| <CommandPalette>{children}</CommandPalette> | |
| </Suspense> |
🤖 Prompt for AI Agents
In apps/mail/components/context/command-palette-context.tsx around lines 1890 to
1892, the Suspense component wrapping CommandPalette lacks a fallback property.
Add a fallback attribute to Suspense with a suitable loading indicator or
placeholder component to display while CommandPalette is loading, preventing a
blank screen for users.
| <CommandPaletteContext.Provider | ||
| value={{ | ||
| open, | ||
| setOpen, | ||
| openModal: () => { | ||
| setOpen(true); | ||
| }, | ||
| activeFilters, | ||
| clearAllFilters, | ||
| }} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Memoize the openModal function to prevent unnecessary re-renders.
The openModal function is recreated on every render, which will cause all context consumers to re-render unnecessarily.
+ const openModal = useCallback(() => {
+ setOpen(true);
+ }, []);
+
return (
<CommandPaletteContext.Provider
value={{
open,
setOpen,
- openModal: () => {
- setOpen(true);
- },
+ openModal,
activeFilters,
clearAllFilters,
}}
>📝 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.
| <CommandPaletteContext.Provider | |
| value={{ | |
| open, | |
| setOpen, | |
| openModal: () => { | |
| setOpen(true); | |
| }, | |
| activeFilters, | |
| clearAllFilters, | |
| }} | |
| > | |
| const openModal = useCallback(() => { | |
| setOpen(true); | |
| }, []); | |
| return ( | |
| <CommandPaletteContext.Provider | |
| value={{ | |
| open, | |
| setOpen, | |
| openModal, | |
| activeFilters, | |
| clearAllFilters, | |
| }} | |
| > |
🤖 Prompt for AI Agents
In apps/mail/components/context/command-palette-context.tsx around lines 1856 to
1866, the openModal function is recreated on every render causing unnecessary
re-renders of context consumers. To fix this, memoize the openModal function
using useCallback with setOpen as a dependency so that the function instance
remains stable across renders.
ahmetskilinc
force-pushed
the
05-30-better_command_palette_and_new_shortcuts
branch
from
851ddf6 to
f18f1b3
Compare
Merge activity
|
2 participants
TL;DR
Refactored the command palette to improve the search and filter experience in the mail app.
What changed?
CommandPaletteProviderto wrap the entire mail app layoutHow to test?
Why make this change?
This change improves the user experience by:
Summary by CodeRabbit