Conversation
- refactor hotkeys - add hotkey recorder
- added route for hotkeys - added table for hotkeys - indexdb for hotkeys (synced with db)
…nn/Zero into feature/language-support-fa
New hotkeys hooks
feat: add farsi support
add: add dockerfile
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a comprehensive overhaul of keyboard shortcut management in the mail application, transitioning from a custom hotkey hook to the standardized Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Shortcuts Page)
participant HotkeyRecorder
participant hotkeysDB (IndexedDB)
participant API Server
participant Database
User->>UI (Shortcuts Page): Opens Shortcuts Settings
UI (Shortcuts Page)->>hotkeysDB: getAllHotkeys()
hotkeysDB->>API Server: (on first load) GET /api/v1/hotkeys
API Server->>Database: Query user_hotkeys
Database-->>API Server: Return shortcuts
API Server-->>hotkeysDB: Return shortcuts
hotkeysDB-->>UI (Shortcuts Page): Return merged shortcuts
User->>HotkeyRecorder: Clicks to record new shortcut
HotkeyRecorder->>User: Shows modal, listens for keys
User->>HotkeyRecorder: Presses new key combo
HotkeyRecorder->>UI (Shortcuts Page): onHotkeyRecorded(newKeys)
UI (Shortcuts Page)->>hotkeysDB: saveHotkey(updatedShortcut)
hotkeysDB->>API Server: PUT /api/v1/hotkeys (sync)
API Server->>Database: Upsert shortcut
Database-->>API Server: Success
API Server-->>hotkeysDB: Success
Possibly related PRs
Suggested reviewers
Poem
✨ 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:
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: 16
🧹 Nitpick comments (18)
packages/db/migrations/0022_round_violations.sql (1)
7-8: Consider cascading on user deletionIf a user row is deleted, keeping an “orphaned” hotkey row is rarely useful.
-ON DELETE no action +ON DELETE cascadePlease confirm that cascading aligns with your data‑retention policy.
apps/mail/lib/hotkeys/global-hotkeys.tsx (2)
11-19: Memoise handlers & shortcut list to avoid re‑registration every renderBecause
handlersandglobalShortcutsare re‑created on each render, the underlying
react-hotkeys-hookwill register / unregister listeners every time ‑ an unnecessary cost
and a potential source of subtle race conditions.-import { useShortcuts } from './use-hotkey-utils'; +import { useShortcuts } from './use-hotkey-utils'; +import { useMemo } from 'react'; @@ - const handlers = { + const handlers = useMemo(() => ({ goToDrafts: () => router.push('/mail/draft'), inbox: () => router.push('/mail/inbox'), sentMail: () => router.push('/mail/sent'), search: () => { console.log('search'); }, newEmail: () => router.push('/mail/compose'), - }; + }), [router]); @@ - const globalShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); + const globalShortcuts = useMemo( + () => keyboardShortcuts.filter((s) => s.scope === scope), + [], + );
15-17:searchhandler is a stub
console.log('search')is a placeholder. If search navigation is planned, wire it up
(e.g.router.push('/mail/search')) or leave a TODO comment so it is not forgotten.apps/mail/lib/hotkeys/thread-display-hotkeys.tsx (1)
17-20: Same re‑registration issue as inGlobalHotkeysWrap
threadDisplayShortcuts&handlersinuseMemoto avoid needless
listener churn.docker-compose.yaml (1)
36-36: Remove trailing whitespace.There's a trailing whitespace on this line that should be removed.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (2)
52-63: Strengthen the hoveredEmailId checkThe conditional check for
hoveredEmailId.currentwould accept empty strings as valid IDs, which could lead to unexpected behavior.- if (hoveredEmailId.current) { + if (hoveredEmailId.current && hoveredEmailId.current.trim().length > 0) {
35-50: Consider using useMemo for allIds calculationFor large thread lists, recalculating allIds on every render could impact performance.
+ const allIds = useMemo(() => + items.map((item) => item.threadId ?? item.id), + [items] + ); const selectAll = useCallback(() => { if (mail.bulkSelected.length > 0) { setMail((prev) => ({ ...prev, bulkSelected: [], })); } else if (items.length > 0) { - const allIds = items.map((item) => item.threadId ?? item.id); setMail((prev) => ({ ...prev, bulkSelected: allIds, }));apps/mail/app/(routes)/settings/shortcuts/hotkey-recorder.tsx (3)
26-37: Improve key normalization for special keysThe current key normalization only handles space and single characters, but doesn't normalize other special keys consistently.
- const key = e.key === ' ' ? 'Space' : e.key; - - const formattedKey = key.length === 1 ? key.toUpperCase() : key; + // Normalize key names + let formattedKey; + switch (e.key) { + case ' ': + formattedKey = 'Space'; + break; + case 'Control': + formattedKey = 'Ctrl'; + break; + case 'Meta': + formattedKey = 'Cmd'; + break; + case 'Escape': + formattedKey = 'Esc'; + break; + default: + formattedKey = e.key.length === 1 ? e.key.toUpperCase() : e.key; + }
70-72: Avoid type casting MessageKeyThe translation key is being force-cast to
MessageKey, which could lead to type safety issues.- {t('pages.settings.shortcuts.actions.recordHotkey' as MessageKey)} + {t('pages.settings.shortcuts.actions.recordHotkey')}Similarly for the other instances on line 77-78.
23-57: Consider using a ref for recordedKeys to avoid unnecessary effect runsThe current implementation includes
recordedKeysin the dependencies array while also updating it inside the effect, potentially causing unnecessary re-runs.export function HotkeyRecorder({ isOpen, onClose, onHotkeyRecorded, currentKeys, }: HotkeyRecorderProps) { const t = useTranslations(); const [recordedKeys, setRecordedKeys] = useState<string[]>([]); + const recordedKeysRef = useRef<string[]>([]); const [isRecording, setIsRecording] = useState(false); useEffect(() => { if (!isOpen) return; const handleKeyDown = (e: KeyboardEvent) => { e.preventDefault(); if (!isRecording) return; const key = e.key === ' ' ? 'Space' : e.key; const formattedKey = key.length === 1 ? key.toUpperCase() : key; - if (!recordedKeys.includes(formattedKey)) { - setRecordedKeys((prev) => [...prev, formattedKey]); + if (!recordedKeysRef.current.includes(formattedKey)) { + recordedKeysRef.current = [...recordedKeysRef.current, formattedKey]; + setRecordedKeys([...recordedKeysRef.current]); } }; const handleKeyUp = (e: KeyboardEvent) => { e.preventDefault(); if (isRecording) { setIsRecording(false); - if (recordedKeys.length > 0) { - onHotkeyRecorded(recordedKeys); + if (recordedKeysRef.current.length > 0) { + onHotkeyRecorded(recordedKeysRef.current); onClose(); } } }; window.addEventListener('keydown', handleKeyDown); window.addEventListener('keyup', handleKeyUp); return () => { window.removeEventListener('keydown', handleKeyDown); window.removeEventListener('keyup', handleKeyUp); }; - }, [isOpen, isRecording, recordedKeys, onHotkeyRecorded, onClose]); + }, [isOpen, isRecording, onHotkeyRecorded, onClose]); useEffect(() => { if (isOpen) { setRecordedKeys([]); + recordedKeysRef.current = []; setIsRecording(true); } }, [isOpen]);apps/mail/components/mail/mail.tsx (1)
254-270: Remove console.log statementsThere are several
console.logstatements that should be removed before production deployment.useEffect(() => { if (threadId) { - console.log('Enabling thread-display scope, disabling mail-list'); enableScope('thread-display'); disableScope('mail-list'); } else { - console.log('Enabling mail-list scope, disabling thread-display'); enableScope('mail-list'); disableScope('thread-display'); } return () => { - console.log('Cleaning up mail/thread scopes'); disableScope('thread-display'); disableScope('mail-list'); }; }, [threadId, enableScope, disableScope]);apps/mail/app/api/v1/hotkeys/route.ts (1)
1-91: Extract common authentication and database operations into utility functionsThe three route handlers share common patterns that could be extracted into reusable utility functions to reduce duplication and improve maintainability.
Consider creating utility functions for:
- Authenticating the request and returning a standardized error response if needed
- The upsert operation with common error handling
- A common error response formatter
Example:
// Auth utility async function authenticateRequest(headers: Headers) { const session = await auth.api.getSession({ headers }); if (!session) { return { authenticated: false, error: NextResponse.json({ error: 'Unauthorized' }, { status: 401 }) }; } return { authenticated: true, session }; } // DB upsert utility async function upsertUserHotkeys(userId: string, shortcuts: Shortcut[]) { const now = new Date(); try { await db .insert(userHotkeys) .values({ userId, shortcuts, createdAt: now, updatedAt: now, }) .onConflictDoUpdate({ target: [userHotkeys.userId], set: { shortcuts, updatedAt: now, }, }); return { success: true }; } catch (error) { console.error('Error upserting user hotkeys:', error); return { success: false, error }; } }apps/mail/components/mail/reply-composer.tsx (1)
924-937: Remove debug console logs from production codeWhile adding hotkey scope management is good, the console logging statements should be removed from production code.
useEffect(() => { if (composerIsOpen) { - console.log('Enabling compose scope (ReplyCompose)'); enableScope('compose'); } else { - console.log('Disabling compose scope (ReplyCompose)'); disableScope('compose'); } return () => { - console.log('Cleaning up compose scope (ReplyCompose)'); disableScope('compose'); }; }, [composerIsOpen, enableScope, disableScope]);apps/mail/components/create/create-email.tsx (1)
464-472: Remove debug console logs from production codeSimilar to the reply-composer component, the console logging statements should be removed from production code.
useEffect(() => { - console.log('Enabling compose scope (CreateEmail)'); enableScope('compose'); return () => { - console.log('Disabling compose scope (CreateEmail)'); disableScope('compose'); }; }, [enableScope, disableScope]);apps/mail/components/mail/mail-list.tsx (1)
495-501: Clear dangling TODO & debugging artefactsThis block still contains a
// TODO: debugcomment left over from development. It signals unfinished work and will quickly turn into dead code.- // TODO: debug const allIds = items.map((item) => item.id);Either finish the debugging task or remove the comment before merging.
apps/mail/lib/hotkeys/hotkeys-db.ts (1)
126-143: Make bulk‑save API public
saveAllHotkeysisprivate, yet the UI needs an efficient “reset to
defaults” operation (see comment inpage.tsx). Consider exposing it or
adding a dedicatedresetToDefaults()to avoidO(N²)writes.apps/mail/hooks/use-mail-navigation.ts (1)
222-285: Consider refactoring repeated key handlingThe implementation for handling repeated key presses is complex with nested timeouts and requestAnimationFrame. While it works correctly, it might be worth considering a more declarative approach or extracting this logic to a separate hook for better maintainability.
A throttling or debouncing hook might provide a cleaner implementation:
- useEffect(() => { - let isProcessingKey = false; - const MOVE_DELAY = 100; // Decreased from 150ms to 100ms for faster movement - - const handleKeyDown = async (event: KeyboardEvent) => { - if (isQuickActionMode) return; - - // For non-repeat events (initial press), let the useHotkeys handlers manage it - if (!event.repeat) return; - - if (event.key === 'ArrowUp' || event.key === 'ArrowDown') { - event.preventDefault(); // Prevent default browser behavior - - // If we're already processing a previous key event, don't stack them - if (isProcessingKey) return; - - // Check if enough time has passed since the last movement - const now = Date.now(); - if (now - lastMoveTime.current < MOVE_DELAY) return; - - isProcessingKey = true; - lastMoveTime.current = now; - - await new Promise<void>((resolve) => { - requestAnimationFrame(() => { - if (event.key === 'ArrowUp') { - setFocusedIndex((prev) => { - const newIndex = prev === null ? items.length - 1 : Math.max(0, prev - 1); - const threadElement = getThreadElement(newIndex); - if (threadElement && containerRef.current) { - threadElement.scrollIntoView({ block: 'nearest', behavior: 'auto' }); - } - return newIndex; - }); - } else if (event.key === 'ArrowDown') { - setFocusedIndex((prev) => { - const newIndex = prev === null ? 0 : Math.min(items.length - 1, prev + 1); - const threadElement = getThreadElement(newIndex); - if (threadElement && containerRef.current) { - threadElement.scrollIntoView({ block: 'nearest', behavior: 'auto' }); - } - return newIndex; - }); - } - isProcessingKey = false; - resolve(); - }); - }); - } - }; - - window.addEventListener('keydown', handleKeyDown); - - return () => { - window.removeEventListener('keydown', handleKeyDown); - }; + // Reuse the same arrow handlers with throttling + const throttledArrowUp = useThrottledCallback(handleArrowUp, 100); + const throttledArrowDown = useThrottledCallback(handleArrowDown, 100); + + // Only handle repeated key presses, initial press is handled by useHotkeys + useEffect(() => { + const handleKeyDown = (event: KeyboardEvent) => { + if (isQuickActionMode || !event.repeat) return; + + if (event.key === 'ArrowUp') { + event.preventDefault(); + throttledArrowUp(event); + } else if (event.key === 'ArrowDown') { + event.preventDefault(); + throttledArrowDown(event); + } + }; + + window.addEventListener('keydown', handleKeyDown); + return () => window.removeEventListener('keydown', handleKeyDown); }, [ handleArrowUp, handleArrowDown, isQuickActionMode, items.length, getThreadElement, containerRef, ]);You would need to implement or import a
useThrottledCallbackhook, but this approach would be more maintainable and reuse your existing handlers.apps/mail/lib/hotkeys/use-hotkey-utils.ts (1)
85-131: useShortcut hook integrates database persistence with hotkey registrationThe
useShortcuthook effectively bridges user-customized shortcuts with the hotkey registration system. It handles loading/saving to the database and properly configures the hotkey with the right options.The error handling could be improved beyond just console.error to provide better feedback to users when shortcut operations fail.
Consider adding more robust error handling:
useEffect(() => { - hotkeysDB.saveHotkey(shortcut).catch(console.error); + hotkeysDB.saveHotkey(shortcut).catch((error) => { + console.error('Failed to save hotkey:', shortcut.action, error); + // Consider adding some user feedback or error reporting here + }); hotkeysDB .getHotkey(shortcut.action) .then((saved) => { if (saved && saved.keys !== shortcut.keys) { setCurrentShortcut(saved); } }) - .catch(console.error); + .catch((error) => { + console.error('Failed to retrieve hotkey:', shortcut.action, error); + // Consider adding some user feedback or fallback behavior + }); }, [shortcut]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
Dockerfile(1 hunks)apps/mail/app/(routes)/layout.tsx(1 hunks)apps/mail/app/(routes)/mail/layout.tsx(1 hunks)apps/mail/app/(routes)/settings/shortcuts/hotkey-recorder.tsx(1 hunks)apps/mail/app/(routes)/settings/shortcuts/page.tsx(2 hunks)apps/mail/app/api/v1/hotkeys/route.ts(1 hunks)apps/mail/components/create/create-email.tsx(22 hunks)apps/mail/components/draft/drafts-list.tsx(8 hunks)apps/mail/components/draft/drafts.tsx(1 hunks)apps/mail/components/mail/keyboard-shortcuts.tsx(0 hunks)apps/mail/components/mail/mail-list.tsx(15 hunks)apps/mail/components/mail/mail.tsx(3 hunks)apps/mail/components/mail/reply-composer.tsx(13 hunks)apps/mail/components/providers/hotkey-provider-wrapper.tsx(1 hunks)apps/mail/components/ui/ai-sidebar.tsx(1 hunks)apps/mail/config/navigation.ts(1 hunks)apps/mail/config/shortcuts.ts(1 hunks)apps/mail/hooks/use-hot-key.ts(2 hunks)apps/mail/hooks/use-mail-navigation.ts(6 hunks)apps/mail/i18n/config.ts(1 hunks)apps/mail/lib/hotkeys/compose-hotkeys.tsx(1 hunks)apps/mail/lib/hotkeys/global-hotkeys.tsx(1 hunks)apps/mail/lib/hotkeys/hotkeys-db.ts(1 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(1 hunks)apps/mail/lib/hotkeys/thread-display-hotkeys.tsx(1 hunks)apps/mail/lib/hotkeys/use-hotkey-utils.ts(1 hunks)apps/mail/locales/en.json(2 hunks)apps/mail/locales/fa.json(1 hunks)apps/mail/package.json(1 hunks)docker-compose.yaml(1 hunks)i18n.json(1 hunks)packages/db/migrations/0022_round_violations.sql(1 hunks)packages/db/migrations/meta/0022_snapshot.json(1 hunks)packages/db/migrations/meta/_journal.json(1 hunks)packages/db/src/schema.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/mail/components/mail/keyboard-shortcuts.tsx
🧰 Additional context used
🧬 Code Graph Analysis (5)
apps/mail/lib/hotkeys/global-hotkeys.tsx (2)
apps/mail/config/shortcuts.ts (1)
keyboardShortcuts(12-34)apps/mail/lib/hotkeys/use-hotkey-utils.ts (1)
useShortcuts(133-144)
apps/mail/components/providers/hotkey-provider-wrapper.tsx (4)
apps/mail/lib/hotkeys/global-hotkeys.tsx (1)
GlobalHotkeys(7-26)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
MailListHotkeys(13-91)apps/mail/lib/hotkeys/thread-display-hotkeys.tsx (1)
ThreadDisplayHotkeys(10-22)apps/mail/lib/hotkeys/compose-hotkeys.tsx (1)
ComposeHotkeys(6-16)
apps/mail/components/mail/reply-composer.tsx (1)
apps/mail/types/index.ts (1)
Sender(32-35)
apps/mail/components/create/create-email.tsx (1)
apps/mail/components/mail/data.tsx (1)
contacts(219-300)
apps/mail/lib/hotkeys/use-hotkey-utils.ts (3)
apps/mail/config/shortcuts.ts (2)
Shortcut(3-10)keyboardShortcuts(12-34)apps/mail/lib/hotkeys/hotkeys-db.ts (1)
hotkeysDB(146-146)apps/mail/lib/idb.ts (1)
keys(63-65)
🪛 YAMLlint (1.35.1)
docker-compose.yaml
[error] 36-36: trailing spaces
(trailing-spaces)
🪛 Biome (1.9.4)
apps/mail/app/(routes)/settings/shortcuts/page.tsx
[error] 75-75: Shouldn't redeclare 'Shortcut'. Consider to delete it or rename it.
'Shortcut' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (63)
apps/mail/package.json (1)
95-95: Dependency addition looks goodThe addition of
react-hotkeys-hookaligns with the keyboard shortcut management overhaul mentioned in the PR summary. The version constraint^5.0.1allows for compatible minor updates which is appropriate.i18n.json (2)
21-22: Farsi language support added correctlyAdding "fa" (Persian/Farsi) to the locale targets list follows the established pattern of language code entries. The formatting is consistent with other entries.
27-27: Array formatting change - no functional impactThe formatting change from multi-line to single-line for the include array is cosmetic only and doesn't affect functionality.
packages/db/src/schema.ts (1)
50-57: Well-structured user hotkeys tableThe new
userHotkeystable is well-designed with appropriate fields:
- Primary key referencing user.id establishes correct one-to-one relationship
- JSONB type for shortcuts allows flexible storage of hotkey configurations
- Standard timestamp fields maintain consistency with other tables
This schema addition properly supports the keyboard shortcut persistence requirements described in the PR summary.
apps/mail/components/ui/ai-sidebar.tsx (1)
9-10: Import changes aligned with keyboard shortcut refactoring.The changes to import statements, including adding
useTranslationsand repositioningcreateContextanduseContext, support the broader refactoring effort to replace the custom hotkey handling with the standardizedreact-hotkeys-hooklibrary.apps/mail/config/navigation.ts (1)
175-175: Enabling shortcuts settings menu item.Commenting out the
disabled: trueproperty for the shortcuts settings menu now makes this feature accessible to users, which aligns with the keyboard shortcut management enhancements introduced in this PR.packages/db/migrations/meta/_journal.json (1)
158-165: New migration for user hotkeys table.The addition of a new migration entry for "0022_round_violations" supports the implementation of persistent, user-customizable keyboard shortcuts. This is a critical component of the overall keyboard shortcut management system.
apps/mail/i18n/config.ts (2)
2-17: Added Farsi language support and improved formatting.The changes include adding Farsi (Persian) language support to the application and standardizing the formatting with single quotes. This enhances the internationalization capabilities of the application.
24-31: Improved code formatting for locale configuration.The reformatting of the
defaultLocaledeclaration andavailableLocalesmapping improves code consistency and readability.apps/mail/app/(routes)/layout.tsx (2)
4-4: Good integration of the new HotkeyProviderWrapperThe import has been properly added to support the new global keyboard shortcut system.
10-25: Implementation of wrapper component looks correctThe HotkeyProviderWrapper has been properly implemented as the outermost wrapper, providing hotkey context to the entire application. This aligns with the new keyboard shortcut management system that uses the react-hotkeys-hook library.
apps/mail/app/(routes)/mail/layout.tsx (1)
2-3: Correctly imported new hotkey-related componentsThe GlobalHotkeys and HotkeyProviderWrapper imports have been correctly added to support the new hotkey functionality.
apps/mail/locales/en.json (1)
401-405: Appropriate localization strings added for new hotkey featuresThe added localization strings for "recordHotkey", "pressKeys", "releaseKeys", and "selectAll" properly support the new keyboard shortcut recording and management functionality.
apps/mail/lib/hotkeys/compose-hotkeys.tsx (1)
1-5: Good client component setup with proper importsThe component is correctly marked with 'use client' and imports the necessary dependencies for hotkey functionality.
packages/db/migrations/0022_round_violations.sql (1)
1-6: Add defaults & timezone‑aware timestamps
created_at/updated_atwill always have to be provided by application code.
It is more robust to let the database populate them and to store them astimestamptzrather thantimestamp, avoiding silent TZ‑shifts.- "created_at" timestamp NOT NULL, - "updated_at" timestamp NOT NULL + "created_at" timestamptz NOT NULL DEFAULT now(), + "updated_at" timestamptz NOT NULL DEFAULT now()You may also want to add a trigger to keep
updated_atin sync on UPDATE.Would you like a follow‑up snippet for that trigger?
apps/mail/hooks/use-hot-key.ts (1)
7-24: Orphaned global key listeners – verify necessityWith the migration to
react-hotkeys-hook, this file now duplicates global
keydown/keyuptracking. If nothing consumesuseKeyStateanymore, these
listeners add overhead and may interfere with the new library.Please grep for
useKeyState(; if unused, delete the hook entirely.apps/mail/components/providers/hotkey-provider-wrapper.tsx (3)
1-8: Well-structured imports and client directive.The client directive and imports are properly organized, bringing in the necessary components from the hotkeys library and the individual hotkey handler components.
10-12: Good type definition for props.Clean interface definition with proper typing for React children.
14-24: Well-implemented provider structure.The HotkeyProviderWrapper component correctly initializes the HotkeysProvider with global scope and includes all necessary hotkey handlers. The component follows React best practices by placing the children inside the provider wrapper.
apps/mail/components/draft/drafts-list.tsx (6)
12-12: Good migration to standard library hook.Successfully replaced the custom
useHotKeyhook with the standarduseHotkeysfrom the react-hotkeys-hook library.
95-96: Trailing comma added for consistency.Good code style change to add a trailing comma in the destructured object.
138-156: Proper modifier key handlers.The implementation for Control, Meta, Shift, and Alt+Shift modifiers correctly sets the selection modes.
158-204: Well-implemented mark as read/unread shortcuts.The mark as read/unread shortcut handlers correctly implement the functionality for both Meta and Control combinations, with proper error handling and user feedback.
220-242: Successfully uncommented and implemented selection shortcuts.The Meta+a, Control+a, Meta+n, and Control+n shortcuts are now properly implemented with event.preventDefault() to avoid browser default behaviors.
However, Meta+n and Control+n are calling selectAll() which seems inconsistent with typical "new" shortcut functionality. This might be intentional but could be confusing to users expecting "new item" behavior.
Is using Meta+n/Control+n for "select all" intentional? These key combinations often represent "new" functionality in many applications.
344-345: Trailing comma added for consistency.Good code style change to add a trailing comma in the object.
docker-compose.yaml (2)
32-32: Security improvement: Using environment variable for Redis token.Replaced hardcoded token with environment variable
${REDIS_TOKEN}, improving security by avoiding credentials in the codebase.
37-52: Well-configured app service for containerization.The app service is properly configured with:
- Container name and build context
- Port mapping for the application
- Dependencies on required services
- Essential environment variables
This enables containerized deployment of the application alongside its dependencies.
Dockerfile (5)
1-7: Good choice of base image and global dependencies.Using
oven/bun:canaryis appropriate for a Bun-based application. The global installation ofnextandturboensures the necessary build tools are available.
9-16: Efficient layer caching strategy for dependencies.The file strategically copies configuration files first, then creates necessary directories and copies package manifests before installing dependencies. This approach optimizes Docker layer caching and speeds up builds.
17-24: Two-phase installation to ensure dependency resolution.Installing dependencies twice (once with manifests, once with full context) ensures all dependencies are properly resolved, avoiding "missing dependencies" errors that can occur in monorepo setups.
25-30: Appropriate build and environment configuration.The application is built in production mode, and the NODE_OPTIONS environment variable correctly addresses the Next.js TextEncoder issue by disabling experimental fetch.
32-34: Proper container configuration for running the application.Exposing port 3000 and using the
--host 0.0.0.0flag ensures the application is accessible from outside the container. Usingbun run startfollows best practices for running the application.apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
24-33: Good implementation of the custom event handlingThe event listener setup for
emailHoveris well-implemented with proper cleanup in the effect's return function.apps/mail/components/mail/mail.tsx (2)
272-274: Good usage of useCallback for the handleClose functionThe
handleClosefunction is correctly memoized withuseCallback, which prevents unnecessary re-renders and improves performance.
254-270:Details
✅ Verification successful
Verify scope management implementation across the app
The hotkey scope management looks correct, but let's ensure it's consistent with the broader application architecture.
Let's check how other components handle hotkey scopes:
🏁 Script executed:
#!/bin/bash # Find other instances of hotkey scope management echo "Checking hotkey scope management patterns across the app..." rg -B 3 -A 3 "enableScope|disableScope" --glob "*.{ts,tsx}"Length of output: 7098
Scope management implementation is consistent across components
- apps/mail/components/create/create-email.tsx
• useEffect enablescompose, cleanup disablescompose- apps/mail/components/mail/reply-composer.tsx
• useEffect togglescomposebased oncomposerIsOpen, cleanup disablescompose- apps/mail/components/mail/mail-list.tsx
• onMouseEnter enablesmail-list, onMouseLeave disablesmail-list- apps/mail/components/mail/mail.tsx
• useEffect switches betweenthread-displayandmail-listbased onthreadId, cleanup disables bothNo mismatches or missing clean‑up calls were found. If exclusive behavior between compose and mail scopes is required, consider adding corresponding disables; otherwise, this implementation aligns with the broader architecture.
apps/mail/config/shortcuts.ts (2)
1-10: LGTM! Type definitions are well-structured and descriptive.The shortcut type definitions are clear, strongly-typed, and provide good documentation through their naming.
12-34: Shortcuts configuration is comprehensive and well-organizedThe keyboard shortcuts are organized with clear scopes, descriptive names, and comprehensive configuration.
Consider grouping shortcuts by scope to improve maintainability as the list grows:
export const keyboardShortcuts: Shortcut[] = [ // Global shortcuts { keys: ["c"], action: "newEmail", type: "single", description: "Compose new email", scope: "global" }, { keys: ["g", "d"], action: "goToDrafts", type: "combination", description: "Go to drafts", scope: "global" }, // ...other global shortcuts // Mail list shortcuts { keys: ["#"], action: "delete", type: "single", description: "Delete email", scope: "mail-list" }, { keys: ["mod", "a"], action: "selectAll", type: "combination", description: "Select all emails", scope: "mail-list", preventDefault: true }, // ...other mail-list shortcuts // Thread display shortcuts { keys: ["r"], action: "reply", type: "single", description: "Reply to email", scope: "thread-display" }, // ...other thread-display shortcuts // Compose shortcuts { keys: ["mod", "Enter"], action: "sendEmail", type: "combination", description: "Send email", scope: "compose" }, // ...other compose shortcuts ];apps/mail/components/mail/reply-composer.tsx (4)
24-43: Improved import organization and new hotkey hookImport organization looks good, with related imports grouped together.
272-281: Fixed indentation in the ccRecipients and bccRecipients blocksCode formatting improvements with consistent indentation.
500-512: Fixed JSON structure formattingThe indentation of the stringified JSON structure is now more readable.
527-540: Improved Promise.all handling formattingThe Promise.all code block is now properly formatted with consistent indentation.
apps/mail/components/create/create-email.tsx (6)
2-7: Better import organization with DropdownMenu componentsImports are now better organized with related DropdownMenu components grouped together.
14-35: Added hotkey context and organized importsThe import for useHotkeysContext and reorganization of React hooks improves code organization.
564-615: Improved dropdown structure formattingThe code formatting for the contact selection dropdown is now more consistent and readable.
679-736: Better formatting for CC recipient inputThe code structure for the CC recipient input and dropdown is improved with consistent formatting.
768-825: Consistent formatting for BCC recipient inputThe BCC recipient input and dropdown follow the same improved formatting pattern as the others.
103-103: Consistent implementation of hotkey scope managementThe implementation of hotkey scope management is consistent with the approach used in reply-composer, which is good for maintainability.
Consider extracting this pattern to a custom hook to reduce duplication:
// useComposeHotkeyScope.ts import { useEffect } from 'react'; import { useHotkeysContext } from 'react-hotkeys-hook'; export function useComposeHotkeyScope() { const { enableScope, disableScope } = useHotkeysContext(); useEffect(() => { enableScope('compose'); return () => { disableScope('compose'); }; }, [enableScope, disableScope]); }This would simplify your component:
// In your component import { useComposeHotkeyScope } from '@/hooks/useComposeHotkeyScope'; export function CreateEmail() { useComposeHotkeyScope(); // Rest of component... }Also applies to: 464-472
apps/mail/components/mail/mail-list.tsx (1)
33-38:Details
❌ Incorrect review comment
Remove legacy
use-hot-keydependency
useKeyStateis imported from@/hooks/use-hot-key, but the PR description says the custom hot‑key hook was removed in favor ofreact‑hotkeys‑hook.
Leaving this import in place is brittle — the hook might already be deleted or will be in a follow‑up, resulting in a runtime crash when the file is bundled.-import { useKeyState } from '@/hooks/use-hot-key'; +// import { useKeyState } from '@/hooks/use-hot-key'; // ➜ removed +import { useKeyState } from '@/lib/hotkeys/use-key-state'; // or migrate logic inline with react‑hotkeys‑hookPlease migrate
useKeyStateor drop the feature if it’s no longer needed.
🏁 Script executed:
#!/bin/bash # Search for occurrences of useKeyState and the old/new hook paths rg "useKeyState" -n . rg "@\/hooks\/use-hot-key" -n . fd use-key-state -t fLength of output: 416
#!/bin/bash # Check for usages of react-hotkeys-hook rg -n "react-hotkeys-hook" --glob '*.{ts,tsx,js,jsx}' # Look for any files or imports related to a new key-state hook rg -n "use-key-state" --glob '*.{ts,tsx,js,jsx}' rg -n "@/lib/hotkeys" --glob '*.{ts,tsx,js,jsx}' fd hotkeys -t d
No action needed for useKeyState import
The customuseKeyStatehook (apps/mail/hooks/use-hot-key.ts) is still present and in use, so the import from@/hooks/use-hot-keyis valid. There is no replacement hook under@/lib/hotkeysor elsewhere in the codebase—no migration is required at this time.Likely an incorrect or invalid review comment.
apps/mail/hooks/use-mail-navigation.ts (8)
3-3: Import updated to use react-hotkeys-hook libraryThe import has been correctly updated to use the standardized
useHotkeyshook from the react-hotkeys-hook library, replacing the custom implementation as part of the broader migration to a more standardized approach to hotkey management.
34-61: Well-structured helper functions for thread element access and scrollingThe
getThreadElementandscrollIntoViewfunctions are well-implemented with proper dependency arrays and error handling. The scrolling behavior uses "smooth" and "nearest" options for a better user experience.
85-108: Arrow Up handler implementation looks goodThe handler correctly prevents default behavior, manages keyboard focus state, and includes special handling for quick action mode. The implementation also includes fallback to the hovered item when no item is focused.
110-133: Arrow Down handler implementation looks goodSimilar to the Arrow Up handler, this implementation correctly manages focus state with proper fallbacks and smooth scrolling behavior.
135-153: Enter handler with quick action supportThe Enter handler now correctly supports both regular thread navigation and triggering of quick action buttons when in quick action mode. Good use of DOM querying to find and interact with the relevant UI elements.
155-164: Tab and Arrow Left/Right handlers for quick action navigationThese handlers properly manage the quick action mode state and navigation between quick action buttons. The implementation ensures users can toggle in and out of quick action mode and navigate between buttons with clear boundaries.
Also applies to: 166-188
190-203: Escape handler with mode-aware behaviorThe Escape handler intelligently handles different states - first exiting quick action mode if active, and then clearing keyboard focus entirely if pressed again. This provides a natural and intuitive escape sequence for users.
205-211: Registration of hotkeys with the new libraryAll keyboard handlers are now properly registered using the
useHotkeyshook from the react-hotkeys-hook library, replacing the previous custom implementation. This standardizes the hotkey handling approach across the application.packages/db/migrations/meta/0022_snapshot.json (1)
585-635: Well-structured database schema for user hotkeysThe new
mail0_user_hotkeystable is properly designed with:
user_idas primary key with foreign key constraint tomail0_usershortcutsas a JSONB field for flexible storage of keyboard shortcuts- Proper timestamp fields for audit history
- Consistent naming conventions with other tables
This schema design aligns well with the application's requirements for persistent, user-specific keyboard shortcuts.
apps/mail/lib/hotkeys/use-hotkey-utils.ts (6)
1-5: Clean imports and dependenciesThe utility file has minimal and clear imports, properly leveraging the keyboard shortcuts configuration and the hotkeys database for persistence.
6-11: Effective shortcut retrieval with fallbackThe
findShortcutfunction properly retrieves user-configured shortcuts from the database with a fallback to default shortcuts if none are found. This ensures a consistent user experience while allowing for customization.
15-40: Effective key formatting for hotkey registrationThe
formatKeysfunction properly handles different key formats and special characters, with platform-specific adjustments. This ensures that hotkeys are correctly registered with the underlying library.
42-69: Comprehensive display formatting for key combinationsThe
formatDisplayKeysfunction provides user-friendly, platform-specific representations of keyboard shortcuts. It handles a wide range of special keys with appropriate symbols, enhancing the user experience.
71-83: Well-defined options type with sensible defaultsThe
HotkeyOptionstype and default values provide a clear interface for customizing shortcut behavior, with sensible default values that work for most use cases.
13-13:Details
✅ Verification successful
Platform detection might not work in SSR environments
The platform detection for Mac is done at module level, which might not correctly adapt when code is rendered server-side first.
🏁 Script executed:
#!/bin/bash # Check if the application uses SSR or is purely client-side # Check for Next.js SSR-related components rg -A 1 -B 1 "getServerSideProps|getInitialProps|getStaticProps" --type ts --type tsx # Check if there are any server components fd -e tsx -e jsx -E "node_modules" | xargs grep -l "use server"Length of output: 179
🏁 Script executed:
#!/bin/bash # Ensure ripgrep recognizes .tsx files and search for Next.js SSR methods rg --type-add 'tsx:*.tsx' --type ts --type tsx "getServerSideProps|getInitialProps|getStaticProps" -n . # Check for Next.js in dependencies grep -R '"next"' -n package.json # List any .tsx files under apps/mail to confirm React/Next usage fd -t f -e tsx apps/mailLength of output: 509
Platform detection is safe – no SSR in this project
We searched for Next.js SSR methods (getServerSideProps, getInitialProps, getStaticProps) and found none, and there’s no Next.js dependency. Thetypeof window !== 'undefined'guard ensuresisMacis only evaluated in the browser. No changes needed.
| const searchIconRef = useRef<any>(null); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if searchIconRef is being used
The searchIconRef is created but doesn't appear to be used anywhere in the visible component code. This could be a leftover from development or preparation for future functionality.
🏁 Script executed:
#!/bin/bash
# Look for usage of searchIconRef in this file or related components
rg "searchIconRef" apps/mail/components/draft/Length of output: 127
Remove unused searchIconRef declaration
searchIconRef is defined but never referenced elsewhere in apps/mail/components/draft/drafts.tsx. To clean up dead code, remove its declaration (or add a comment explaining planned future use if still needed).
• Location:
apps/mail/components/draft/drafts.tsx, line 36
| <HotkeyProviderWrapper> | ||
| <AppSidebar /> | ||
| <KeyboardShortcuts /> | ||
| <GlobalHotkeys /> | ||
| <div className="w-full bg-white md:py-3 md:pr-2 dark:bg-black">{children}</div> | ||
| </> | ||
| </HotkeyProviderWrapper> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential duplicate HotkeyProviderWrapper
The layout is wrapped with HotkeyProviderWrapper, but the root layout (apps/mail/app/(routes)/layout.tsx) already includes this wrapper. Nested providers might cause unexpected behavior.
🏁 Script executed:
#!/bin/bash
# Check if the HotkeyProviderWrapper is implemented with a React Context that supports nesting
rg -A 10 "createContext|HotkeyProviderWrapper" --type tsx --type jsxLength of output: 98
🏁 Script executed:
#!/bin/bash
# Search for the HotkeyProviderWrapper definition and its implementation
rg -A 20 "HotkeyProviderWrapper" .
# Search for any createContext usage that might indicate nested context handling
rg -A 10 "createContext" .Length of output: 12957
Remove nested HotkeyProviderWrapper to prevent duplicate context
The HotkeyProviderWrapper is already applied at the root layout (apps/mail/app/(routes)/layout.tsx), and its implementation renders <GlobalHotkeys /> internally. Wrapping the mail‐specific layout again causes a second provider and duplicate <GlobalHotkeys />, which can lead to handlers firing twice or scope conflicts.
Please update apps/mail/app/(routes)/mail/layout.tsx as follows:
• Remove the import and usage of HotkeyProviderWrapper.
• Remove the extra <GlobalHotkeys /> (it’s provided by the wrapper at the root).
Suggested diff:
- import { HotkeyProviderWrapper } from '@/components/providers/hotkey-provider-wrapper';
+ // removed redundant wrapper import
export default function MailLayout({ children }: { children: React.ReactNode }) {
- return (
- <HotkeyProviderWrapper>
- <AppSidebar />
- <GlobalHotkeys />
- <div className="w-full bg-white md:py-3 md:pr-2 dark:bg-black">
- {children}
- </div>
- </HotkeyProviderWrapper>
- );
+ return (
+ <>
+ <AppSidebar />
+ <div className="w-full bg-white md:py-3 md:pr-2 dark:bg-black">
+ {children}
+ </div>
+ </>
+ );
}| export function ComposeHotkeys() { | ||
| const scope = 'compose'; | ||
|
|
||
| const handlers = {}; | ||
|
|
||
| const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); | ||
|
|
||
| useShortcuts(composeShortcuts, handlers, { scope }); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Empty handlers object requires verification
The ComposeHotkeys component is filtering shortcuts correctly by scope, but the handlers object is empty. This might be intentional if the useShortcuts hook manages the handlers internally, but should be verified.
🏁 Script executed:
#!/bin/bash
# Check the implementation of useShortcuts to understand how handlers are used
rg -A 15 "export function useShortcuts" --type tsx --type ts
# Also check if there are other similar hotkey components to compare implementation
rg -A 5 "const handlers = {}" --type tsx --type tsLength of output: 263
To ensure we understand how handlers is used and whether it must be populated, let’s locate the useShortcuts implementation and compare other hotkey components’ usage:
🏁 Script executed:
#!/bin/bash
# Find the useShortcuts hook definition
rg -n "function useShortcuts" -A10
# Find all direct usages of useShortcuts(
rg -n "useShortcuts(" -A5
# List all TSX files that use the hook
rg -l "useShortcuts" -g "*.tsx"
# For each hotkeys component, show how handlers is declared
for file in $(rg -l "useShortcuts" -g "*hotkeys.tsx"); do
echo "File: $file"
rg -n "const handlers" -A2 "$file" || echo " No handlers declaration found"
doneLength of output: 2579
ComposeHotkeys handlers must be implemented
The useShortcuts hook only binds shortcuts when there’s a matching handler function. With an empty handlers object in compose-hotkeys.tsx, none of the compose‑scoped shortcuts will ever fire. Other hotkey components declare their handlers (for example, see thread-display-hotkeys.tsx, mail-list-hotkeys.tsx, global-hotkeys.tsx).
Please populate the handlers object in apps/mail/lib/hotkeys/compose-hotkeys.tsx (around lines 8–12) to map each action from keyboardShortcuts to its callback. For example:
const handlers = {
sendEmail: () => {/* trigger send */},
saveDraft: () => {/* trigger save */},
// …add handlers for all compose actions
};Ensure each key matches an action in your compose scope shortcuts.
| const closeView = (event: KeyboardEvent) => { | ||
| event.preventDefault(); | ||
| }; | ||
|
|
||
| export function ThreadDisplayHotkeys() { | ||
| const scope = 'thread-display'; | ||
|
|
||
| const handlers = { | ||
| closeView: () => closeView(new KeyboardEvent('keydown', { key: 'Escape' })), | ||
| }; |
There was a problem hiding this comment.
Handler prevents default on a synthetic event – does nothing
closeView(new KeyboardEvent(...)) creates a fresh event whose preventDefault
has no effect on the real key press.
Moreover, the keyboardShortcuts config shown in the repo does not define
an action named "closeView", so this hotkey will never fire.
-const closeView = (event: KeyboardEvent) => {
- event.preventDefault();
-};
+// TODO: replace with actual close‑thread logic invoked by Esc
+function closeThread() {
+ // e.g. navigate back or update state
+}
@@
- const handlers = {
- closeView: () => closeView(new KeyboardEvent('keydown', { key: 'Escape' })),
- };
+ const handlers = { closeThread };Don’t forget to add the corresponding "closeThread" action to
config/shortcuts.ts.
📝 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 closeView = (event: KeyboardEvent) => { | |
| event.preventDefault(); | |
| }; | |
| export function ThreadDisplayHotkeys() { | |
| const scope = 'thread-display'; | |
| const handlers = { | |
| closeView: () => closeView(new KeyboardEvent('keydown', { key: 'Escape' })), | |
| }; | |
| // TODO: replace with actual close‑thread logic invoked by Esc | |
| function closeThread() { | |
| // e.g. navigate back or update state | |
| } | |
| export function ThreadDisplayHotkeys() { | |
| const scope = 'thread-display'; | |
| const handlers = { closeThread }; | |
| } |
| @@ -1,4 +1,4 @@ | |||
| import { useCallback, useRef, useLayoutEffect, useState, useEffect } from "react"; | |||
| import { useCallback, useRef, useLayoutEffect, useState, useEffect } from 'react'; | |||
There was a problem hiding this comment.
Remove unused React imports – will break the build under noUnusedLocals
Only useCallback is referenced; the remainder cause TS/ESLint errors.
-import { useCallback, useRef, useLayoutEffect, useState, useEffect } from 'react';
+import { useCallback } from 'react';📝 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 { useCallback, useRef, useLayoutEffect, useState, useEffect } from 'react'; | |
| import { useCallback } from 'react'; |
| function Shortcut({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) { | ||
| const [isRecording, setIsRecording] = useState(false); | ||
| const displayKeys = formatDisplayKeys(keys); | ||
|
|
||
| const handleHotkeyRecorded = async (newKeys: string[]) => { | ||
| try { | ||
| // Find the original shortcut to preserve its type and description | ||
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | ||
| if (!originalShortcut) { | ||
| throw new Error('Original shortcut not found'); | ||
| } | ||
|
|
||
| const updatedShortcut: Shortcut = { | ||
| ...originalShortcut, | ||
| keys: newKeys, | ||
| }; | ||
|
|
||
| await hotkeysDB.saveHotkey(updatedShortcut); | ||
| toast.success('Shortcut saved successfully'); | ||
| } catch (error) { | ||
| console.error('Failed to save shortcut:', error); | ||
| toast.error('Failed to save shortcut'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Name collision with imported type Shortcut
A component named Shortcut is declared while a type with the same identifier
is imported earlier (import { keyboardShortcuts, type Shortcut } …).
Typescript allows this but tools (see Biome hint) flag it as a redeclaration and
it confuses readers (“is this a type or component?”).
Rename the component, e.g. ShortcutItem:
-function Shortcut({ children, keys, action }: { ... }) {
+function ShortcutItem({ children, keys, action }: { ... }) {and update the JSX usage above.
📝 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.
| function Shortcut({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) { | |
| const [isRecording, setIsRecording] = useState(false); | |
| const displayKeys = formatDisplayKeys(keys); | |
| const handleHotkeyRecorded = async (newKeys: string[]) => { | |
| try { | |
| // Find the original shortcut to preserve its type and description | |
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | |
| if (!originalShortcut) { | |
| throw new Error('Original shortcut not found'); | |
| } | |
| const updatedShortcut: Shortcut = { | |
| ...originalShortcut, | |
| keys: newKeys, | |
| }; | |
| await hotkeysDB.saveHotkey(updatedShortcut); | |
| toast.success('Shortcut saved successfully'); | |
| } catch (error) { | |
| console.error('Failed to save shortcut:', error); | |
| toast.error('Failed to save shortcut'); | |
| } | |
| }; | |
| function ShortcutItem({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) { | |
| const [isRecording, setIsRecording] = useState(false); | |
| const displayKeys = formatDisplayKeys(keys); | |
| const handleHotkeyRecorded = async (newKeys: string[]) => { | |
| try { | |
| // Find the original shortcut to preserve its type and description | |
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | |
| if (!originalShortcut) { | |
| throw new Error('Original shortcut not found'); | |
| } | |
| const updatedShortcut: Shortcut = { | |
| ...originalShortcut, | |
| keys: newKeys, | |
| }; | |
| await hotkeysDB.saveHotkey(updatedShortcut); | |
| toast.success('Shortcut saved successfully'); | |
| } catch (error) { | |
| console.error('Failed to save shortcut:', error); | |
| toast.error('Failed to save shortcut'); | |
| } | |
| }; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Shouldn't redeclare 'Shortcut'. Consider to delete it or rename it.
'Shortcut' is defined here:
(lint/suspicious/noRedeclare)
| const handleHotkeyRecorded = async (newKeys: string[]) => { | ||
| try { | ||
| // Find the original shortcut to preserve its type and description | ||
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | ||
| if (!originalShortcut) { | ||
| throw new Error('Original shortcut not found'); | ||
| } | ||
|
|
||
| const updatedShortcut: Shortcut = { | ||
| ...originalShortcut, | ||
| keys: newKeys, | ||
| }; | ||
|
|
||
| await hotkeysDB.saveHotkey(updatedShortcut); | ||
| toast.success('Shortcut saved successfully'); | ||
| } catch (error) { | ||
| console.error('Failed to save shortcut:', error); | ||
| toast.error('Failed to save shortcut'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
UI never updates after saving a new hot‑key
handleHotkeyRecorded persists the change but doesn’t update local state, so
the displayed key combo stays stale until the page is reloaded.
Quick fix inside handleHotkeyRecorded:
await hotkeysDB.saveHotkey(updatedShortcut);
+setShortcuts((prev) =>
+ prev.map((s) => (s.action === action ? updatedShortcut : s)),
+);
toast.success('Shortcut saved successfully');This keeps the UI and IndexedDB in sync.
📝 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 handleHotkeyRecorded = async (newKeys: string[]) => { | |
| try { | |
| // Find the original shortcut to preserve its type and description | |
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | |
| if (!originalShortcut) { | |
| throw new Error('Original shortcut not found'); | |
| } | |
| const updatedShortcut: Shortcut = { | |
| ...originalShortcut, | |
| keys: newKeys, | |
| }; | |
| await hotkeysDB.saveHotkey(updatedShortcut); | |
| toast.success('Shortcut saved successfully'); | |
| } catch (error) { | |
| console.error('Failed to save shortcut:', error); | |
| toast.error('Failed to save shortcut'); | |
| } | |
| }; | |
| const handleHotkeyRecorded = async (newKeys: string[]) => { | |
| try { | |
| // Find the original shortcut to preserve its type and description | |
| const originalShortcut = keyboardShortcuts.find(s => s.action === action); | |
| if (!originalShortcut) { | |
| throw new Error('Original shortcut not found'); | |
| } | |
| const updatedShortcut: Shortcut = { | |
| ...originalShortcut, | |
| keys: newKeys, | |
| }; | |
| await hotkeysDB.saveHotkey(updatedShortcut); | |
| setShortcuts((prev) => | |
| prev.map((s) => (s.action === action ? updatedShortcut : s)), | |
| ); | |
| toast.success('Shortcut saved successfully'); | |
| } catch (error) { | |
| console.error('Failed to save shortcut:', error); | |
| toast.error('Failed to save shortcut'); | |
| } | |
| }; |
| request.onupgradeneeded = (event) => { | ||
| const db = (event.target as IDBOpenDBRequest).result; | ||
| if (db.objectStoreNames.contains(STORE_NAME)) { | ||
| db.deleteObjectStore(STORE_NAME); | ||
| } | ||
| db.createObjectStore(STORE_NAME, { keyPath: 'id' }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unconditional object‑store deletion wipes user data on every version bump
Inside onupgradeneeded you always do db.deleteObjectStore(STORE_NAME) before
createObjectStore. Any saved shortcuts are lost whenever DB_VERSION
increments.
Safer upgrade logic:
-if (db.objectStoreNames.contains(STORE_NAME)) {
- db.deleteObjectStore(STORE_NAME);
-}
-db.createObjectStore(STORE_NAME, { keyPath: 'id' });
+if (!db.objectStoreNames.contains(STORE_NAME)) {
+ db.createObjectStore(STORE_NAME, { keyPath: 'id' });
+}Preserve data unless the schema actually changes.
📝 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.
| request.onupgradeneeded = (event) => { | |
| const db = (event.target as IDBOpenDBRequest).result; | |
| if (db.objectStoreNames.contains(STORE_NAME)) { | |
| db.deleteObjectStore(STORE_NAME); | |
| } | |
| db.createObjectStore(STORE_NAME, { keyPath: 'id' }); | |
| }; | |
| request.onupgradeneeded = (event) => { | |
| const db = (event.target as IDBOpenDBRequest).result; | |
| if (!db.objectStoreNames.contains(STORE_NAME)) { | |
| db.createObjectStore(STORE_NAME, { keyPath: 'id' }); | |
| } | |
| }; |
| request.onsuccess = () => { | ||
| this.db = request.result; | ||
| resolve(); | ||
| }; | ||
|
|
||
| request.onupgradeneeded = (event) => { | ||
| const db = (event.target as IDBOpenDBRequest).result; | ||
| if (db.objectStoreNames.contains(STORE_NAME)) { | ||
| db.deleteObjectStore(STORE_NAME); | ||
| } | ||
| db.createObjectStore(STORE_NAME, { keyPath: 'id' }); | ||
| }; | ||
|
|
||
| request.onsuccess = async () => { | ||
| this.db = request.result; | ||
| if (!serverHotkeysLoaded) { | ||
| serverHotkeysLoaded = true; | ||
| const serverHotkeys = await fetchServerHotkeys(); | ||
| if (serverHotkeys?.length) { | ||
| const mergedHotkeys = this.mergeWithDefaults(serverHotkeys); | ||
| await this.saveAllHotkeys(mergedHotkeys, false); | ||
| } else { | ||
| await this.initializeDefaultShortcuts(); | ||
| } | ||
| } | ||
| resolve(); | ||
| }; |
There was a problem hiding this comment.
Duplicate onsuccess handler – first one is overwritten
request.onsuccess is assigned twice (lines 49‑52 and again 62‑75).
The first assignment is silently lost, which can skip this.db = request.result
or double‑resolve the promise in rare cases.
Merge the logic into a single handler:
-request.onsuccess = () => {
- this.db = request.result;
- resolve();
-};
-
-request.onsuccess = async () => {
+request.onsuccess = async () => {
this.db = request.result;
...Alternatively use addEventListener('success', …).
| export function useShortcuts( | ||
| shortcuts: Shortcut[], | ||
| handlers: { [key: string]: () => void }, | ||
| options: Partial<HotkeyOptions> = {}, | ||
| ) { | ||
| shortcuts.forEach((shortcut) => { | ||
| const handler = handlers[shortcut.action]; | ||
| if (handler) { | ||
| useShortcut(shortcut, handler, options); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring useShortcuts hook to avoid hook rule violations
The useShortcuts function calls hooks inside a forEach loop, which could potentially violate React's rules of hooks depending on how it's implemented. React hooks should be called in the same order on every render.
A safer approach would be:
- export function useShortcuts(
- shortcuts: Shortcut[],
- handlers: { [key: string]: () => void },
- options: Partial<HotkeyOptions> = {},
- ) {
- shortcuts.forEach((shortcut) => {
- const handler = handlers[shortcut.action];
- if (handler) {
- useShortcut(shortcut, handler, options);
- }
- });
- }
+ export function useShortcuts(
+ shortcuts: Shortcut[],
+ handlers: { [key: string]: () => void },
+ options: Partial<HotkeyOptions> = {},
+ ) {
+ // Create a stable array of shortcuts that won't change between renders
+ // if the shortcuts array reference changes but content is the same
+ const stableShortcuts = useMemo(
+ () => shortcuts.filter(shortcut => !!handlers[shortcut.action]),
+ [shortcuts, handlers]
+ );
+
+ // Register each shortcut in order
+ // This ensures the hooks are always called in the same order
+ for (let i = 0; i < stableShortcuts.length; i++) {
+ const shortcut = stableShortcuts[i];
+ const handler = handlers[shortcut.action];
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ useShortcut(shortcut, handler, options);
+ }
+ }Note that even with this approach, you need to disable the ESLint rule because React's linter can't verify that the loop has a stable length. A better approach might be to register all shortcuts in a single hook call if the underlying library supports it.
Committable suggestion skipped: line range outside the PR's diff.
6 participants
READ CAREFULLY THEN REMOVE
Remove bullet points that are not relevant.
PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI.
Description
Please provide a clear description of your changes.
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Additional Notes
Add any other context about the pull request here.
Screenshots/Recordings
Add screenshots or recordings here if applicable.
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
react-hotkeys-hook.