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 (hotkey) management in the mail application. It removes the custom Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (React)
participant HotkeyProviderWrapper
participant Hotkey Components
participant IndexedDB (hotkeys-db)
participant API Server
User->>UI (React): Opens settings/shortcuts page
UI (React)->>IndexedDB: getAllHotkeys()
IndexedDB->>API Server: (if needed) GET /api/v1/hotkeys
API Server-->>IndexedDB: Returns user hotkeys
IndexedDB-->>UI (React): Returns merged hotkeys
User->>UI (React): Clicks to record new shortcut
UI (React)->>HotkeyRecorder: Opens modal, records keys
HotkeyRecorder-->>UI (React): onHotkeyRecorded(keys)
UI (React)->>IndexedDB: saveHotkey(shortcut)
IndexedDB->>API Server: POST /api/v1/hotkeys (sync)
API Server-->>IndexedDB: Confirms save
UI (React)->>HotkeyProviderWrapper: Wraps app tree
HotkeyProviderWrapper->>Hotkey Components: Register scoped hotkeys
Hotkey Components->>UI (React): Listen for hotkey events
User->>UI (React): Triggers hotkey
UI (React)-->>Hotkey Components: Handler executes action
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: 27
🔭 Outside diff range comments (1)
apps/mail/hooks/use-hot-key.ts (1)
1-34: 💡 Verification agent🧩 Analysis chain
Dead‑code & duplicate listeners risk
This file appears to be the relic of the old custom hot‑key implementation that the PR description says was removed in favour of
react-hotkeys-hook. If no module importsuseKeyStateany more, the file should be deleted to avoid:
- 3 global listeners per page → unnecessary event processing.
- Listeners never removed on unmount, so in HMR or micro‑frontend scenarios the handlers stack up and leak.
Please run a quick search and, if unused, drop the file entirely.
-// apps/mail/hooks/use-hot-key.ts <-- safe to delete if unusedRun:
If the search is empty, we can safely delete. Otherwise we need to migrate those call‑sites to
react-hotkeys-hook.
🏁 Script executed:
#!/usr/bin/env bash # Show any remaining imports of useKeyState / use-hot-key rg -n "(useKeyState|from './hooks/use-hot-key')" | headLength of output: 305
Migrate
useKeyStatetoreact-hotkeys-hookinmail-list.tsxWe still have two usages of the old
useKeyStateinapps/mail/components/mail/mail-list.tsx, so we can’t delete the hook file yet. Please:
In
apps/mail/components/mail/mail-list.tsx:
- Replace
with the equivalent fromimport { useKeyState } from '@/hooks/use-hot-key'; const isKeyPressed = useKeyState();react-hotkeys-hook(e.g.useHotkeysoruseKeyPress).- Update any logic around
isKeyPressed("YourKey")to the new hook’s API.Once all call‑sites in
mail-list.tsx(and elsewhere) are migrated, removeapps/mail/hooks/use-hot-key.ts.This ensures we eliminate the global listeners and prevent listener leaks.
🧹 Nitpick comments (42)
Dockerfile (4)
1-2: Pin the base image to a stable tag for reproducible builds.Using
oven/bun:canarycould introduce breaking changes when the canary tag updates. Consider pinning to a specific version (e.g.oven/bun:0.8.5) or parameterizing the version with anARGto ensure build stability.
19-23: Consolidate install steps to reduce image layers.You run
bun installtwice (lines 17 and 22). If both are necessary (cache vs. full context), add a clarifying comment; otherwise, combine them:- RUN bun install - COPY . . - RUN bun install + COPY . . + RUN bun install
27-31: Parameterize and document environment settings.Hard‑coding
NODE_ENVandNODE_OPTIONSlimits flexibility. Consider usingARG NODE_ENVor reading from build‑time variables, and document why--no-experimental-fetchis required.
32-34: Add a .dockerignore to keep the build context lean.Since you copy the full repo (
COPY . .), include a.dockerignoreto excludenode_modules, build artifacts, and any sensitive files. This will reduce image size and prevent leaking secrets.docker-compose.yaml (2)
36-36: Remove trailing whitespace.Line 36 has trailing spaces which may trigger linter errors. Please delete the extra space character.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
37-47: Enhance theappservice with a restart policy and healthcheck.To improve reliability, consider:
app: restart: unless-stopped healthcheck: test: ["CMD", "curl", "-f", "http://localhost:3000/healthz"] interval: 30s timeout: 10s retries: 3This aligns it with other services and ensures automatic recovery.
apps/mail/components/draft/drafts.tsx (1)
36-37: Added ref is unused - consider removing if not needed.You've added a
searchIconRefbut it doesn't appear to be used anywhere in the component. If this is intended for future use, consider adding a TODO comment explaining its purpose, otherwise it should be removed.- const searchIconRef = useRef<any>(null); + // Either remove if unused or add a TODO comment explaining its future purposeapps/mail/components/ui/ai-sidebar.tsx (1)
9-10: Remove unused import or implement translationThe
useTranslationshook is imported but not used anywhere in the component. This appears to be part of the broader hotkey refactoring whereuseHotKeywas removed, but the translation functionality wasn't implemented.-import { useTranslations } from 'next-intl';Alternatively, implement the translations for user-facing strings like "Ask Zero a question..." on line 80.
apps/mail/lib/hotkeys/compose-hotkeys.tsx (1)
15-15: Add JSDoc comment explaining the component purposeThe component returns null (no UI) which is correct for a pure functionality component, but it's not immediately obvious to other developers why this is the case.
+/** + * Registers keyboard shortcuts for the compose email context. + * This component doesn't render any UI elements, it only sets up the hotkey listeners. + */ export function ComposeHotkeys() {apps/mail/components/home/navbar.tsx (1)
57-63: Consider removing commented codeThe commented-out button code with an explanation is taking up space without providing functionality. Consider removing it or moving the explanation to a TODO comment elsewhere.
-{/* It is better to enable this button when we implement our own mail server, no need for it honestly */} -{/* <Button - className="h-[32px] w-[110px] rounded-md bg-gray-900 text-white hover:bg-black dark:bg-white dark:text-black dark:hover:bg-white/90" - asChild -> - <Link href={session ? '/mail' : '/login'}>Get Started</Link> -</Button> */} +// TODO: Enable additional button when mail server is implementedapps/mail/i18n/config.ts (2)
2-17: Consider alphabetising and using end‑user–friendly labelsAdding
fa: 'Farsi'works, but to keep the list easy to scan and merge‑conflict friendly we normally keep the keys alphabetically sorted.
Also, most localisation pickers surface “Persian (Farsi)” to help non‑native speakers recognise the language – worth considering for UX consistency.No functional blocker, just a maintenance / UX polish point.
28-31: Tiny perf/readability nit – compute map once
availableLocalesis recomputed at every import. The object is tiny so this is not critical, but we usually cache the result withconst availableLocales: ReadonlyArray<{code:Locale;name:string}> = ...to make the intent explicit and prevent accidental mutation.-export const availableLocales = locales.map((code) => ({ +export const availableLocales: ReadonlyArray<{code: Locale; name: string}> = locales.map(code => ({ code, name: LANGUAGES[code], }));apps/mail/components/draft/drafts-list.tsx (2)
138-156: Four nearly identical handlers – collapse for DRYness
Control,Meta,Shift,Alt+Shiftall flip one boolean flag each. You can make the code shorter and avoid registering four hooks by iterating over a config list:- useHotkeys('Control', () => { resetSelectMode(); setMassSelectMode(true); }); - useHotkeys('Meta', () => { resetSelectMode(); setMassSelectMode(true); }); - useHotkeys('Shift', () => { resetSelectMode(); setRangeSelectMode(true); }); - useHotkeys('Alt+Shift', () => { resetSelectMode(); setSelectAllBelowMode(true); }); +[ + { keys: ['Control', 'Meta'], action: () => setMassSelectMode(true) }, + { keys: ['Shift'], action: () => setRangeSelectMode(true) }, + { keys: ['Alt+Shift'], action: () => setSelectAllBelowMode(true) }, +].forEach(({ keys, action }) => + keys.forEach(k => + useHotkeys(k, () => { resetSelectMode(); action(); }) + ) +);Fewer hooks → smaller re‑render matrix.
158-204: Duplicate “mark as read/unread” handlers – unify keys array
Meta+Shift+uandControl+Shift+u(same for…+i) call identical async blocks. We can DRY this using the 4thdepsargument ofuseHotkeys:- useHotkeys('Meta+Shift+u', async () => { … }); - useHotkeys('Control+Shift+u', async () => { … }); +['Meta+Shift+u', 'Control+Shift+u'].forEach(k => + useHotkeys(k, async () => { /* body unchanged */ }, {}, [mail.bulkSelected]) +);This makes maintenance of shortcuts & side‑effects easier.
apps/mail/lib/hotkeys/thread-display-hotkeys.tsx (1)
14-14: Simplify KeyboardEvent creationCreating a new KeyboardEvent with only the 'key' property is unnecessary since the event isn't being used for anything except calling preventDefault(), which is already done in the closeView function.
- closeView: () => closeView(new KeyboardEvent('keydown', { key: 'Escape' })), + closeView: () => { + // Either directly implement the closeView logic here + // or if preventDefault is needed: + const event = new KeyboardEvent('keydown', { key: 'Escape' }); + closeView(event); + },apps/mail/components/providers/hotkey-provider-wrapper.tsx (2)
16-16: Consider providing a way to dynamically change active scopesThe component initializes with only the 'global' scope active, but doesn't provide a way to dynamically change which scopes are active. This may limit flexibility as the user interacts with different parts of the application.
Consider using a state variable and context provider to allow changing active scopes:
export function HotkeyProviderWrapper({ children }: HotkeyProviderWrapperProps) { + const [activeScopes, setActiveScopes] = useState(['global']); + return ( - <HotkeysProvider initiallyActiveScopes={['global']}> + <HotkeysProvider initiallyActiveScopes={activeScopes}> + <HotkeysContext.Provider value={{ activeScopes, setActiveScopes }}> <GlobalHotkeys /> <MailListHotkeys /> <ThreadDisplayHotkeys /> <ComposeHotkeys /> {children} + </HotkeysContext.Provider> </HotkeysProvider> ); } + +// Define HotkeysContext +export const HotkeysContext = React.createContext<{ + activeScopes: string[]; + setActiveScopes: React.Dispatch<React.SetStateAction<string[]>>; +}>({ + activeScopes: ['global'], + setActiveScopes: () => {}, +});
1-24: Add error boundary for hotkey componentsThe component renders multiple hotkey components but doesn't include an error boundary. If any of these components throw an error, it could cause the entire application to crash.
'use client'; import { HotkeysProvider } from 'react-hotkeys-hook'; import { GlobalHotkeys } from '@/lib/hotkeys/global-hotkeys'; import { MailListHotkeys } from '@/lib/hotkeys/mail-list-hotkeys'; import { ThreadDisplayHotkeys } from '@/lib/hotkeys/thread-display-hotkeys'; import { ComposeHotkeys } from '@/lib/hotkeys/compose-hotkeys'; import React from 'react'; +class HotkeyErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = { hasError: false }; + } + + static getDerivedStateFromError(error) { + return { hasError: true }; + } + + componentDidCatch(error, errorInfo) { + console.error("Hotkey error:", error, errorInfo); + } + + render() { + if (this.state.hasError) { + return null; // Render nothing if there's an error in hotkey components + } + return this.props.children; + } +} + interface HotkeyProviderWrapperProps { children: React.ReactNode; } export function HotkeyProviderWrapper({ children }: HotkeyProviderWrapperProps) { return ( <HotkeysProvider initiallyActiveScopes={['global']}> + <HotkeyErrorBoundary> <GlobalHotkeys /> <MailListHotkeys /> <ThreadDisplayHotkeys /> <ComposeHotkeys /> + </HotkeyErrorBoundary> {children} </HotkeysProvider> ); }packages/db/migrations/0022_round_violations.sql (2)
8-8: Consider adding ON DELETE CASCADE to foreign keyThe current foreign key constraint has "ON DELETE no action" which could lead to orphaned records if a user is deleted. Consider using CASCADE or SET NULL depending on your data requirements.
-ALTER TABLE "mail0_user_hotkeys" ADD CONSTRAINT "mail0_user_hotkeys_user_id_mail0_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."mail0_user"("id") ON DELETE no action ON UPDATE no action; +ALTER TABLE "mail0_user_hotkeys" ADD CONSTRAINT "mail0_user_hotkeys_user_id_mail0_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."mail0_user"("id") ON DELETE CASCADE ON UPDATE no action;
1-8: Consider adding validation constraints for the shortcuts JSONB fieldThe
shortcutsfield uses JSONB type but doesn't have any validation constraints. Consider adding a check constraint to ensure the data follows the expected structure.CREATE TABLE "mail0_user_hotkeys" ( "user_id" text PRIMARY KEY NOT NULL, "shortcuts" jsonb NOT NULL, "created_at" timestamp NOT NULL, "updated_at" timestamp NOT NULL ); --> statement-breakpoint ALTER TABLE "mail0_user_hotkeys" ADD CONSTRAINT "mail0_user_hotkeys_user_id_mail0_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."mail0_user"("id") ON DELETE no action ON UPDATE no action; + +--> statement-breakpoint +-- Ensure shortcuts is a JSON array +ALTER TABLE "mail0_user_hotkeys" ADD CONSTRAINT "shortcuts_is_array" CHECK (jsonb_typeof(shortcuts) = 'array');apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
24-33: Consider using TypeScript for the custom eventFor improved type safety, consider defining a proper TypeScript interface for the 'emailHover' custom event instead of using type assertion.
- useEffect(() => { - const handleEmailHover = (event: CustomEvent<{ id: string | null }>) => { - hoveredEmailId.current = event.detail.id; - }; - - window.addEventListener('emailHover', handleEmailHover as EventListener); - return () => { - window.removeEventListener('emailHover', handleEmailHover as EventListener); - }; - }, []); + interface EmailHoverEvent extends CustomEvent { + detail: { id: string | null }; + } + + useEffect(() => { + const handleEmailHover = (event: EmailHoverEvent) => { + hoveredEmailId.current = event.detail.id; + }; + + window.addEventListener('emailHover', handleEmailHover as EventListener); + return () => { + window.removeEventListener('emailHover', handleEmailHover as EventListener); + }; + }, []);apps/mail/actions/settings.ts (2)
90-210: Consider externalizing the email templateThe large HTML template makes the function harder to read and maintain. Consider moving this template to a separate file or using a templating solution.
You could create a separate module for email templates and import the golden ticket template:
- html: `<!DOCTYPE html PUBLIC...`, + html: renderGoldenTicketTemplate(email),Then create a new file like
email-templates.tswith the template function.
202-202: Use dynamic year for copyright noticeThe copyright year is hardcoded as 2025. Use a dynamic year to ensure it remains current.
- © - <!-- -->2025<!-- --> + © + <!-- --><%= new Date().getFullYear() %><!-- -->apps/mail/components/golden.tsx (1)
67-71: Enhance error message handling from API responseWhile the code does check for API error messages, it could provide more specific feedback based on different error scenarios.
- toast.error(result.error || 'Failed to send invite', { - id: toastId, - }); + const errorMessage = result.error === 'Golden ticket already claimed' + ? 'You have already sent an invitation' + : result.error || 'Failed to send invite'; + toast.error(errorMessage, { + id: toastId, + });apps/mail/app/(routes)/settings/shortcuts/hotkey-recorder.tsx (3)
30-32: Consider handling more key formatting edge casesThe key formatting handles Space key and uppercase conversion, but consider other special keys like 'Meta', 'Control', 'Alt' that might need consistent naming.
- const key = e.key === ' ' ? 'Space' : e.key; - - const formattedKey = key.length === 1 ? key.toUpperCase() : key; + // Normalize key names for consistency + const key = e.key; + let formattedKey; + + // Special case for Space key + if (key === ' ') { + formattedKey = 'Space'; + // Format modifier keys consistently + } else if (['Control', 'Alt', 'Meta', 'Shift'].includes(key)) { + formattedKey = key; + // Convert single characters to uppercase + } else if (key.length === 1) { + formattedKey = key.toUpperCase(); + } else { + formattedKey = key; + }
39-48: Improve key release logic and add feedback for empty recordingsThe current implementation completes recording on any key release without validating the result. Add validation and feedback for empty recordings.
const handleKeyUp = (e: KeyboardEvent) => { e.preventDefault(); if (isRecording) { setIsRecording(false); if (recordedKeys.length > 0) { onHotkeyRecorded(recordedKeys); onClose(); + } else { + // Provide feedback if no keys were recorded + console.log('No keys recorded'); + // Optional: return to recording state instead of closing + setIsRecording(true); } } };
80-89: Improve accessibility of keyboard key displayWhile the UI uses
kbdtags which is correct for keyboard keys, enhance accessibility by adding appropriate ARIA attributes.<kbd key={key} className="border-muted-foreground/10 bg-accent h-6 rounded-[6px] border px-1.5 font-mono text-xs leading-6" + role="img" + aria-label={`Keyboard key ${key}`} > {key} </kbd>apps/mail/components/mail/mail.tsx (2)
254-270: Remove debug console.log statements before productionThe code contains debug console.log statements that should be removed before deploying to production.
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]);
254-270: Consider adding error handling for hotkey scope managementThe hotkey scope management doesn't handle potential errors. Add try-catch blocks for more robust error handling.
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'); - } + try { + if (threadId) { + enableScope('thread-display'); + disableScope('mail-list'); + } else { + enableScope('mail-list'); + disableScope('thread-display'); + } + } catch (error) { + console.error('Error managing hotkey scopes:', error); + } return () => { - console.log('Cleaning up mail/thread scopes'); - disableScope('thread-display'); - disableScope('mail-list'); + try { + disableScope('thread-display'); + disableScope('mail-list'); + } catch (error) { + console.error('Error cleaning up hotkey scopes:', error); + } }; }, [threadId, enableScope, disableScope]);apps/mail/config/shortcuts.ts (3)
12-34: Add keyboard shortcut conflict detection mechanismThe keyboard shortcuts are defined statically without a mechanism to detect conflicts. Consider adding a utility function to identify potential conflicts.
You could add a function to detect conflicts between shortcuts that share the same keys but different scopes:
// Add this function to detect conflicts export function detectShortcutConflicts(shortcuts: Shortcut[]): Array<{shortcut1: Shortcut, shortcut2: Shortcut}> { const conflicts: Array<{shortcut1: Shortcut, shortcut2: Shortcut}> = []; // Group shortcuts by keys (converted to string for comparison) const shortcutsByKeys: Record<string, Shortcut[]> = {}; shortcuts.forEach(shortcut => { const keyString = shortcut.keys.join('+'); if (!shortcutsByKeys[keyString]) { shortcutsByKeys[keyString] = []; } shortcutsByKeys[keyString].push(shortcut); }); // Find conflicts within the same scope Object.values(shortcutsByKeys).forEach(shortcutsWithSameKeys => { if (shortcutsWithSameKeys.length > 1) { // Group by scope const byScope: Record<string, Shortcut[]> = {}; shortcutsWithSameKeys.forEach(s => { if (!byScope[s.scope]) byScope[s.scope] = []; byScope[s.scope].push(s); }); // Check for conflicts within same scope Object.values(byScope).forEach(scopeShortcuts => { if (scopeShortcuts.length > 1) { for (let i = 0; i < scopeShortcuts.length; i++) { for (let j = i + 1; j < scopeShortcuts.length; j++) { conflicts.push({ shortcut1: scopeShortcuts[i], shortcut2: scopeShortcuts[j] }); } } } }); } }); return conflicts; } // You could then call this when initializing the app: // const conflicts = detectShortcutConflicts(keyboardShortcuts); // if (conflicts.length > 0) { // console.warn('Shortcut conflicts detected:', conflicts); // }
25-30: Consider grouping related shortcutsRelated shortcuts like archive, spam, and move operations are scattered throughout the array. Consider grouping them for better maintainability.
- { keys: ["e"], action: "archiveEmail", type: "single", description: "Archive email", scope: "mail-list" }, - { keys: ["!"], action: "markAsSpam", type: "single", description: "Mark as spam", scope: "mail-list" }, - { keys: ["v"], action: "moveToFolder", type: "single", description: "Move to folder", scope: "mail-list" }, - { keys: ["z"], action: "undoLastAction", type: "single", description: "Undo last action", scope: "global" }, - { keys: ["i"], action: "viewEmailDetails", type: "single", description: "View email details", scope: "thread-display" }, - { keys: ["o"], action: "expandEmailView", type: "single", description: "Expand email view", scope: "mail-list" }, + // Mail organization shortcuts + { keys: ["e"], action: "archiveEmail", type: "single", description: "Archive email", scope: "mail-list" }, + { keys: ["!"], action: "markAsSpam", type: "single", description: "Mark as spam", scope: "mail-list" }, + { keys: ["v"], action: "moveToFolder", type: "single", description: "Move to folder", scope: "mail-list" }, + + // View and undo shortcuts + { keys: ["z"], action: "undoLastAction", type: "single", description: "Undo last action", scope: "global" }, + { keys: ["i"], action: "viewEmailDetails", type: "single", description: "View email details", scope: "thread-display" }, + { keys: ["o"], action: "expandEmailView", type: "single", description: "Expand email view", scope: "mail-list" },
3-10: Enhance the Shortcut type with additional metadataThe Shortcut type could be enhanced with additional metadata to improve user experience and documentation.
export type Shortcut = { keys: string[]; action: string; type: ShortcutType; description: string; scope: string; preventDefault?: boolean; + category?: string; // For grouping in the UI + platformOverride?: { + mac?: string[]; + windows?: string[]; + linux?: string[]; + }; // For platform-specific key displays + isDisabled?: boolean; // To temporarily disable shortcuts };apps/mail/components/create/create-email.tsx (2)
33-35: Remove unuseduseEffectimport
useEffectis imported as a named import but the component consistently callsReact.useEffect. The named import is therefore unused and may trigger ESLint / TS warnings.-import { useEffect } from 'react';Keeping the import list minimal avoids drifting into “half‑namespaced / half‑global” usage and makes the file just a bit clearer.
464-472: Strip dev‑only console output & ensure single unregisterThe two
console.loglines left in the hot‑key scope effect are handy while developing but will quickly clutter the console in production. In addition, callingdisableScope('compose')only in the cleanup function is sufficient; calling it again from the body on every unmount duplicates work and risks racing with the cleanup.- useEffect(() => { - console.log('Enabling compose scope (CreateEmail)'); - enableScope('compose'); - - return () => { - console.log('Disabling compose scope (CreateEmail)'); - disableScope('compose'); - }; - }, [enableScope, disableScope]); + useEffect(() => { + enableScope('compose'); + return () => disableScope('compose'); + }, [enableScope, disableScope]);This keeps logs clean and guarantees exactly one matching
disableScopeperenableScope.apps/mail/components/mail/reply-composer.tsx (1)
501-513: More robust empty‑message checkSerialising an object with
JSON.stringifyfor equality tests can break if key ordering changes. A safer check is to inspect the rich‑text JSON structure directly or strip tags from HTML and test for non‑whitespace. Consider:const isMessageEmpty = !getValues('messageContent').replace(/<[^>]+>/g, '').trim() && attachments.length === 0;This prevents false negatives and removes the brittle
JSON.stringifycomparison.apps/mail/components/mail/mail-list.tsx (1)
557-563: Remove verbose mouse‑debug logsThe
console.logstatements fire on every mouse enter/leave for the mail list and will generate a lot of noise in user consoles and log aggregators.- onMouseEnter={() => { - console.log('[MailList] Mouse Enter - Enabling scope: mail-list'); - enableScope('mail-list'); - }} - onMouseLeave={() => { - console.log('[MailList] Mouse Leave - Disabling scope: mail-list'); - disableScope('mail-list'); - }} + onMouseEnter={() => enableScope('mail-list')} + onMouseLeave={() => disableScope('mail-list')}Behaviour is unchanged but logging is kept to actionable events.
apps/mail/app/api/golden-ticket/route.ts (1)
40-43: Type‑safe fallback for ResendWhen
RESEND_API_KEYis missing, a mock object is created but typed asany, losing autocompletion and increasing risk of runtime typos.- const resend = process.env.RESEND_API_KEY - ? new Resend(process.env.RESEND_API_KEY) - : { emails: { send: async (...args: any[]) => console.log(args) } }; + const resend: Pick<Resend, 'emails'> = process.env.RESEND_API_KEY + ? new Resend(process.env.RESEND_API_KEY) + : { emails: { send: async (...args: unknown[]) => console.log('[Mock resend]', ...args) } };Restricting the mock to the expected surface keeps TypeScript checks intact.
apps/mail/app/(routes)/settings/shortcuts/page.tsx (1)
40-55: Avoid using the array index as the Reactkey
key={index}leads to unstable identity when list ordering changes (e.g. after a user edits or resets shortcuts).
Prefer a deterministic key such asshortcut.action, which is unique and stable.- key={index} + key={shortcut.action}packages/db/migrations/meta/0022_snapshot.json (1)
585-635: Consider cascading delete formail0_user_hotkeys
mail0_user_hotkeys.user_id → mail0_user.idis defined withonDelete: "no action".
If a user record is removed, the orphaned hotkey row will remain and violate referential integrity in application logic.- "onDelete": "no action", + "onDelete": "cascade",Double‑check whether soft‑deletes are used; if not, a cascading delete is usually the safer option.
apps/mail/lib/hotkeys/hotkeys-db.ts (2)
3-5: Constants are well-defined but should they be configurable?The database name, store name, and version are hardcoded. Consider extracting these as environment variables or configuration options, especially the DB_VERSION which might need to be incremented in future updates.
95-104: Logic in saveHotkey function can be improvedThe function first gets all hotkeys, then maps to replace matching actions, and then checks if it needs to add a new one. This could be simplified and made more efficient.
async saveHotkey(shortcut: Shortcut): Promise<void> { const allHotkeys = await this.getAllHotkeys(); - const updatedHotkeys = allHotkeys.map((s) => (s.action === shortcut.action ? shortcut : s)); - - if (!allHotkeys.some((s) => s.action === shortcut.action)) { - updatedHotkeys.push(shortcut); - } + const index = allHotkeys.findIndex((s) => s.action === shortcut.action); + + if (index >= 0) { + allHotkeys[index] = shortcut; + } else { + allHotkeys.push(shortcut); + } - await this.saveAllHotkeys(updatedHotkeys, true); + await this.saveAllHotkeys(allHotkeys, true); }apps/mail/lib/hotkeys/use-hotkey-utils.ts (2)
15-40: Key formatting can be improvedThe
formatKeysfunction has a few issues:
- The handling of special characters like '#' and '!' is hardcoded as shift combinations
- The switch case could be replaced with a mapping object for cleaner code
- There's inconsistent return value handling between single and multiple keys
export const formatKeys = (keys: string[] | undefined): string => { if (!keys || !keys.length) return ''; - const mapKey = (key: string) => { - switch (key) { - case 'mod': - return isMac ? 'meta' : 'control'; - case '⌘': - return 'meta'; - case '#': - return 'shift+3'; - case '!': - return 'shift+1'; - default: - return key; - } - }; + const keyMap: Record<string, string> = { + 'mod': isMac ? 'meta' : 'control', + '⌘': 'meta', + '#': 'shift+3', + '!': 'shift+1', + }; + + const mapKey = (key: string) => keyMap[key] || key; - if (keys.length > 1) { - return keys.map(mapKey).join('+'); - } - - const firstKey = keys[0]; - if (!firstKey) return ''; - return mapKey(firstKey); + return keys.map(mapKey).join('+'); };
42-69: Consider using a mapping object for display keysSimilar to the previous comment, the
formatDisplayKeysfunction could be simplified with a mapping object rather than a large switch statement.export const formatDisplayKeys = (keys: string[]): string[] => { + const keyMap: Record<string, string> = { + 'mod': isMac ? '⌘' : 'Ctrl', + 'meta': '⌘', + 'control': 'Ctrl', + 'shift': '⇧', + 'alt': isMac ? '⌥' : 'Alt', + 'enter': '↵', + 'escape': 'Esc', + 'backspace': '⌫', + 'delete': '⌦', + 'space': 'Space', + }; return keys.map((key) => { - switch (key) { - case 'mod': - return isMac ? '⌘' : 'Ctrl'; - case 'meta': - return '⌘'; - case 'control': - return 'Ctrl'; - case 'shift': - return '⇧'; - case 'alt': - return isMac ? '⌥' : 'Alt'; - case 'enter': - return '↵'; - case 'escape': - return 'Esc'; - case 'backspace': - return '⌫'; - case 'delete': - return '⌦'; - case 'space': - return 'Space'; - default: - return key.length === 1 ? key.toUpperCase() : key; - } + return keyMap[key] || (key.length === 1 ? key.toUpperCase() : key); }); };
📜 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 (39)
Dockerfile(1 hunks)apps/mail/actions/settings.ts(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/golden-ticket/route.ts(1 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/golden.tsx(1 hunks)apps/mail/components/home/navbar.tsx(2 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 (9)
apps/mail/lib/hotkeys/thread-display-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/home/navbar.tsx (2)
apps/mail/components/ui/button.tsx (1)
Button(55-55)packages/db/src/schema.ts (1)
session(19-30)
apps/mail/app/(routes)/layout.tsx (3)
apps/mail/components/providers/hotkey-provider-wrapper.tsx (1)
HotkeyProviderWrapper(14-24)apps/mail/components/context/command-palette-context.tsx (1)
CommandPaletteProvider(210-216)apps/mail/lib/idb.ts (1)
dexieStorageProvider(25-114)
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/lib/hotkeys/compose-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/create/create-email.tsx (1)
apps/mail/components/mail/data.tsx (1)
contacts(219-300)
apps/mail/app/api/golden-ticket/route.ts (3)
apps/mail/app/api/utils.ts (1)
getAuthenticatedUserId(21-30)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (2)
earlyAccess(68-75)user(7-17)
apps/mail/components/mail/reply-composer.tsx (1)
apps/mail/types/index.ts (1)
Sender(32-35)
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 (28)
Dockerfile (3)
5-7: Verify global installation syntax for Bun CLI tools.Bun’s CLI syntax for global installs may differ from npm. Double‑check that
bun install -g next turbois valid; if not, switch to:RUN bun add -g next turbo
9-16: Good use of layered caching by copying manifests separately.Copying only
package.json,bun.lock,turbo.json, and individual package manifests before runningbun installleverages Docker’s cache effectively. Nice optimization!
25-26: Build step is correct.
RUN bun run buildgenerates production assets. Ensure the output directory matches your runtime CMD.docker-compose.yaml (2)
31-33: Validate environment variable naming for Redis token.You changed
SRH_TOKENto${REDIS_TOKEN}, but theappservice usesREDIS_URL. Please confirm your.envor deployment docs define bothREDIS_TOKEN(for the proxy) andREDIS_URL(for the app) correctly.
48-53: Confirm required environment variables forapp.Ensure
DATABASE_URLandREDIS_URLare set in your environment or.envfile beforedocker-compose up. Missing values will cause startup failures.packages/db/src/schema.ts (1)
50-57: Schema addition looks good!You've added a well-structured
userHotkeystable to store user-specific keyboard shortcut configurations with appropriate references and constraints. This table provides a solid foundation for persisting user hotkey preferences.apps/mail/package.json (1)
95-95: LGTM! Great choice to use a specialized library.The addition of
react-hotkeys-hookis a good approach for standardizing hotkey handling. This well-maintained library will provide better maintainability and functionality compared to a custom hook solution.packages/db/migrations/meta/_journal.json (1)
159-165: Migration entry correctly added.The new migration entry is properly formatted and consistent with the schema changes for the
userHotkeystable.i18n.json (2)
21-22: Great addition: Persian language support looks good!The addition of "fa" (Farsi) to the locale targets array aligns well with the internationalization enhancements described in the summary.
27-27: Code formatting improvement approvedThe single-line formatting of the include array is consistent with standard practice and improves readability.
apps/mail/config/navigation.ts (1)
175-175: Good enablement of keyboard shortcuts settingsThe commented-out
disabled: trueproperty effectively enables the keyboard shortcuts settings menu item, which aligns perfectly with the new keyboard shortcut management features being added.apps/mail/app/(routes)/layout.tsx (1)
4-4: Well-structured integration of hotkey providerThe integration of the
HotkeyProviderWrapperat the root layout level is the correct approach. This ensures that keyboard shortcuts are available throughout the application and properly scoped where needed.The wrapper correctly encapsulates the existing providers while maintaining the component hierarchy, and the indentation is consistent with the codebase style.
Also applies to: 10-25
apps/mail/app/(routes)/mail/layout.tsx (2)
1-10: Clean integration of the new hotkey systemThe changes properly integrate the new hotkey system by importing the necessary components and wrapping the layout content, which aligns with the broader refactoring of keyboard shortcut management.
7-11: Good structured approach to hotkey context provisionWrapping the entire mail layout with
HotkeyProviderWrapperensures hotkey functionality is available throughout the mail interface, following a clean architectural pattern for context provision.apps/mail/components/home/navbar.tsx (2)
29-35: Good UI enhancement using Button componentUsing the Button component to wrap the Link improves the UI consistency and accessibility by providing proper button styling and behavior while maintaining link navigation.
49-55: Same pattern maintained across desktop navigationThe desktop navigation follows the same pattern as the mobile navigation, maintaining consistency.
apps/mail/locales/en.json (1)
401-405: Properly localized new shortcut functionalityThe new localization strings for keyboard shortcut recording and management are well-structured and consistent with the existing naming patterns.
apps/mail/components/draft/drafts-list.tsx (3)
12-13: Import path ok – but remember to tree‑shake old hookGood switch to
react-hotkeys-hook. Ensure no leftoveruseHotKeyimports in this folder to prevent bundle size bloat.
95-97: Trailing comma after single binding is fineJust noting that the style‑guide usually omits the dangling comma when only one identifier is destructured, but this is purely cosmetic.
220-231: Good: overriding default “select‑all” behaviourThe
preventDefault()guards against browser Edit → Select All – nice touch.apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (2)
13-90: Well-structured component with good separation of concerns.The keyboard shortcuts are scoped correctly to 'mail-list' context and the component properly integrates with mail state and toast notifications. The approach of listening for an 'emailHover' event is effective for tracking which email is currently being hovered.
52-79: Good pattern for conditional toast notifications and action handlingThe implementation of
markAsUnreadhandles both the hovered email and bulk-selected scenarios elegantly. The toast promise pattern is well-used to handle loading, success, and error states.apps/mail/locales/fa.json (1)
1-431: Ready structure for Persian (Farsi) localizationThe file provides a comprehensive structure for Persian language translations with all the necessary keys matching the application's text content. This is a good first step for adding Farsi support to the application.
Now you need to populate the empty string values with actual Persian translations. Consider using professional translation services or working with native speakers to ensure accurate translations.
apps/mail/actions/settings.ts (2)
85-210: Improved email template with responsive designThe HTML email template is a significant improvement over plain text, with proper structure, styling, and responsive design elements. The template follows email development best practices with table-based layout and inline styles for maximum compatibility.
183-186:Details
✅ Verification successful
Verify Discord invite link
The Discord link appears to be a placeholder. Ensure it's the correct permanent invite link before deploying to production.
🏁 Script executed:
#!/bin/bash # Check if the Discord invite exists and is valid curl -s -I "https://discord.gg/0email" | grep "HTTP"Length of output: 62
Let's check where this invite is redirecting to:
🏁 Script executed:
#!/bin/bash curl -s -I "https://discord.gg/0email" | grep -i LocationLength of output: 101
Let's check the final HTTP status for the redirected link:
🏁 Script executed:
#!/bin/bash curl -s -o /dev/null -w "%{http_code}" "https://discord.com/invite/0email"Length of output: 78
Discord invite link is valid
The invite
https://discord.gg/0emailcorrectly redirects tohttps://discord.com/invite/0emailand returns HTTP 200. No changes are needed.apps/mail/components/golden.tsx (1)
47-77: Improved error handling and user feedbackThe refactored submit function now provides better user feedback with toast notifications for loading, success, and error states. The error handling with try/catch is a good practice.
apps/mail/lib/hotkeys/hotkeys-db.ts (2)
134-136: Race condition risk with server syncThe sync with server is called inside the transaction's
oncompletehandler, but there's no coordination between multiple concurrent saves - could lead to race conditions when multiple components try to save shortcuts simultaneously.Consider adding a synchronization mechanism (like a mutex or queue) to prevent race conditions, or at least add a check to avoid frequent server syncs:
transaction.oncomplete = async () => { if (shouldSync && serverHotkeysLoaded) { + // Add debounce or throttle mechanism here await syncWithServer(shortcuts); } resolve(); };
140-141: 🛠️ Refactor suggestionTransaction error handling could be improved
The code clears the store and then puts new data, but if an error occurs after clearing, the data would be lost. Consider using a more atomic approach.
- store.clear(); - store.put({ id: 'shortcuts', shortcuts }); + // Check if entry exists first + const getRequest = store.get('shortcuts'); + getRequest.onsuccess = () => { + // If it exists, update it, otherwise add it + if (getRequest.result) { + store.put({ id: 'shortcuts', shortcuts }); + } else { + store.add({ id: 'shortcuts', shortcuts }); + } + };Likely an incorrect or invalid review comment.
| export function ComposeHotkeys() { | ||
| const scope = 'compose'; | ||
|
|
||
| const handlers = {}; | ||
|
|
||
| const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); | ||
|
|
||
| useShortcuts(composeShortcuts, handlers, { scope }); | ||
|
|
There was a problem hiding this comment.
Empty handlers object will prevent shortcuts from functioning
The handlers object is empty, which means no callback functions will be registered for the compose shortcuts. This will prevent the shortcuts from actually doing anything when pressed.
-const handlers = {};
+const handlers = {
+ sendEmail: () => {
+ // Add implementation for sending email
+ console.log('Send email shortcut triggered');
+ // Example: document.querySelector('button[type="submit"]')?.click();
+ }
+};📝 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.
| export function ComposeHotkeys() { | |
| const scope = 'compose'; | |
| const handlers = {}; | |
| const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); | |
| useShortcuts(composeShortcuts, handlers, { scope }); | |
| export function ComposeHotkeys() { | |
| const scope = 'compose'; | |
| const handlers = { | |
| sendEmail: () => { | |
| // Add implementation for sending email | |
| console.log('Send email shortcut triggered'); | |
| // Example: document.querySelector('button[type="submit"]')?.click(); | |
| } | |
| }; | |
| const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); | |
| useShortcuts(composeShortcuts, handlers, { scope }); | |
| } |
| "helpWithShortcuts": "Help with shortcuts", | ||
| "recordHotkey": "Record Hotkey", | ||
| "pressKeys": "Press keys", | ||
| "releaseKeys": "Release keys", | ||
| "selectAll": "Select All" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify these strings exist in other language files
Ensure that these new localization strings are also added to other language files to maintain consistent multilingual support.
🏁 Script executed:
#!/bin/bash
# Check if the new shortcut strings exist in other language files
echo "Checking localization files for shortcut strings..."
for locale_file in $(find apps/mail/locales -type f -name "*.json" ! -name "en.json"); do
echo "Checking $locale_file..."
for key in "recordHotkey" "pressKeys" "releaseKeys" "selectAll"; do
if ! grep -q "\"$key\":" "$locale_file"; then
echo "WARNING: Missing \"$key\" in $locale_file"
fi
done
doneLength of output: 18675
Add missing shortcut localization keys to all non‑English locale files
The following keys introduced in apps/mail/locales/en.json (lines 401–405) are currently absent from every other locale file under apps/mail/locales:
• recordHotkey
• pressKeys
• releaseKeys
• selectAll
Affected files (non‑English):
apps/mail/locales/ar.json
apps/mail/locales/ca.json
apps/mail/locales/cs.json
apps/mail/locales/de.json
apps/mail/locales/es.json
apps/mail/locales/fa.json
apps/mail/locales/fr.json
apps/mail/locales/hi.json
apps/mail/locales/hu.json
apps/mail/locales/ja.json
apps/mail/locales/ko.json
apps/mail/locales/lv.json
apps/mail/locales/pl.json
apps/mail/locales/pt.json
apps/mail/locales/ru.json
apps/mail/locales/tr.json
Please add these keys (with appropriate translations) to each file to ensure consistent multilingual support.
| }); | ||
| }, [items, nextPageToken, isValidating, isLoading]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log statement left in production code
console.log('Drafts data:', …) is useful while debugging but will ship to users and may leak PII (message IDs). Please remove or guard behind an env check.
- console.log('Drafts data:', { … });
+ if (process.env.NODE_ENV !== 'production') {
+ // eslint-disable-next-line no-console
+ console.debug('Drafts data:', { … });
+ }Committable suggestion skipped: line range outside the PR's diff.
| useHotkeys('Meta+n', async (event) => { | ||
| event.preventDefault(); | ||
| resetSelectMode(); | ||
| selectAll(); | ||
| }); | ||
|
|
||
| // useHotKey('Control+n', async (event) => { | ||
| // // @ts-expect-error | ||
| // event.preventDefault(); | ||
| // resetSelectMode(); | ||
| // selectAll(); | ||
| // }); | ||
| useHotkeys('Control+n', async (event) => { | ||
| event.preventDefault(); | ||
| resetSelectMode(); | ||
| selectAll(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Meta/Control + n currently triggers selectAll – likely a mistake
Cmd+N / Ctrl+N traditionally means “new message”. Calling selectAll() here is surprising for users and collides with OS/browser defaults (new window in Safari/Chrome). Confirm intent; if you meant next or new draft, wire it accordingly.
- useHotkeys('Meta+n', … selectAll());
- useHotkeys('Control+n', … selectAll());
+# e.g. to start composing:
+useHotkeys(['Meta+n', 'Control+n'], () => router.push('/mail/create'));Correct hotkey mappings for “Select All” vs. “New Draft”
It looks like Cmd+N / Ctrl+N is currently wired to selectAll(), but conventionally:
- Cmd/Ctrl+A selects all items
- Cmd/Ctrl+N starts composing a new message
Please update in apps/mail/components/draft/drafts-list.tsx (around lines 232–243):
• Change the “select all” hotkey to Meta+A + Control+A
• Reserve Meta+N + Control+N for opening the new‑draft flow (e.g. router.push('/mail/create'))
- useHotkeys('Meta+n', async (event) => {
- event.preventDefault();
- resetSelectMode();
- selectAll();
- });
-
- useHotkeys('Control+n', async (event) => {
- event.preventDefault();
- resetSelectMode();
- selectAll();
- });
+ // Select all drafts with Cmd/Ctrl+A
+ useHotkeys(['Meta+a', 'Control+a'], async (event) => {
+ event.preventDefault();
+ resetSelectMode();
+ selectAll();
+ });
+
+ // Compose a new draft with Cmd/Ctrl+N
+ useHotkeys(['Meta+n', 'Control+n'], async (event) => {
+ event.preventDefault();
+ resetSelectMode();
+ router.push('/mail/create');
+ });📝 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.
| useHotkeys('Meta+n', async (event) => { | |
| event.preventDefault(); | |
| resetSelectMode(); | |
| selectAll(); | |
| }); | |
| // useHotKey('Control+n', async (event) => { | |
| // // @ts-expect-error | |
| // event.preventDefault(); | |
| // resetSelectMode(); | |
| // selectAll(); | |
| // }); | |
| useHotkeys('Control+n', async (event) => { | |
| event.preventDefault(); | |
| resetSelectMode(); | |
| selectAll(); | |
| }); | |
| // Select all drafts with Cmd/Ctrl+A | |
| useHotkeys(['Meta+a', 'Control+a'], async (event) => { | |
| event.preventDefault(); | |
| resetSelectMode(); | |
| selectAll(); | |
| }); | |
| // Compose a new draft with Cmd/Ctrl+N | |
| useHotkeys(['Meta+n', 'Control+n'], async (event) => { | |
| event.preventDefault(); | |
| resetSelectMode(); | |
| router.push('/mail/create'); | |
| }); |
| const closeView = (event: KeyboardEvent) => { | ||
| event.preventDefault(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding closeView functionality
The closeView function currently only prevents the default behavior of the keyboard event but doesn't appear to actually close any view. This seems incomplete, especially since it's being triggered with an Escape key event.
Consider implementing the actual view closing logic:
-const closeView = (event: KeyboardEvent) => {
- event.preventDefault();
-};
+const closeView = (event: KeyboardEvent) => {
+ event.preventDefault();
+ // Add logic to close the current thread view
+ // For example: router.push('/mail/inbox') or dispatch a state update action
+};📝 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(); | |
| }; | |
| const closeView = (event: KeyboardEvent) => { | |
| event.preventDefault(); | |
| // Add logic to close the current thread view | |
| // For example: router.push('/mail/inbox') or dispatch a state update action | |
| }; |
| return keyboardShortcuts.find((sc) => sc.action === action); | ||
| }; | ||
|
|
||
| const isMac = typeof window !== 'undefined' && navigator.platform.toUpperCase().indexOf('MAC') >= 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Browser compatibility consideration for platform detection
The platform detection for Mac might not work reliably across all browsers. navigator.platform is also deprecated.
-const isMac = typeof window !== 'undefined' && navigator.platform.toUpperCase().indexOf('MAC') >= 0;
+const isMac = typeof window !== 'undefined' && /Mac|iPod|iPhone|iPad/.test(navigator.userAgent);Consider using navigator.userAgent as a more reliable alternative, or even better, a feature detection approach with the UA-CH API where supported.
| 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
useShortcuts could cause performance issues
The useShortcuts function doesn't memoize handlers, which could lead to unnecessary re-renders when used in a component.
Consider wrapping the map in useMemo to prevent unnecessary hook invocations:
export function useShortcuts(
shortcuts: Shortcut[],
handlers: { [key: string]: () => void },
options: Partial<HotkeyOptions> = {},
) {
+ // Use React.useMemo to avoid recreating shortcuts on every render
+ React.useMemo(() => {
shortcuts.forEach((shortcut) => {
const handler = handlers[shortcut.action];
if (handler) {
useShortcut(shortcut, handler, options);
}
});
+ }, [shortcuts, handlers, options]);
}Additionally, using a hook inside a forEach loop is not recommended as it breaks the Rules of Hooks. This needs a more fundamental redesign.
Committable suggestion skipped: line range outside the PR's diff.
| const handleKey = useCallback( | ||
| (event: KeyboardEvent) => { | ||
| if (currentShortcut.preventDefault || preventDefault) { | ||
| event.preventDefault(); | ||
| } | ||
| callback(); | ||
| }, | ||
| [callback, preventDefault, currentShortcut], | ||
| ); |
There was a problem hiding this comment.
Redundant preventDefault in handleKey
The handleKey function checks both local preventDefault and currentShortcut.preventDefault, but the same logic is also passed as an option to useHotkeys on line 127.
const handleKey = useCallback(
(event: KeyboardEvent) => {
- if (currentShortcut.preventDefault || preventDefault) {
- event.preventDefault();
- }
callback();
},
- [callback, preventDefault, currentShortcut],
+ [callback],
);Let the underlying useHotkeys hook handle preventDefault behavior based on the options.
📝 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 handleKey = useCallback( | |
| (event: KeyboardEvent) => { | |
| if (currentShortcut.preventDefault || preventDefault) { | |
| event.preventDefault(); | |
| } | |
| callback(); | |
| }, | |
| [callback, preventDefault, currentShortcut], | |
| ); | |
| const handleKey = useCallback( | |
| (event: KeyboardEvent) => { | |
| callback(); | |
| }, | |
| [callback], | |
| ); |
| useEffect(() => { | ||
| hotkeysDB.saveHotkey(shortcut).catch(console.error); | ||
|
|
||
| hotkeysDB | ||
| .getHotkey(shortcut.action) | ||
| .then((saved) => { | ||
| if (saved && saved.keys !== shortcut.keys) { | ||
| setCurrentShortcut(saved); | ||
| } | ||
| }) | ||
| .catch(console.error); | ||
| }, [shortcut]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in useEffect
The hook first saves the shortcut unconditionally and then checks if a saved version exists, which could lead to race conditions.
useEffect(() => {
- hotkeysDB.saveHotkey(shortcut).catch(console.error);
-
hotkeysDB
.getHotkey(shortcut.action)
.then((saved) => {
if (saved && saved.keys !== shortcut.keys) {
setCurrentShortcut(saved);
+ } else if (!saved) {
+ // Only save if not already in DB
+ hotkeysDB.saveHotkey(shortcut).catch(console.error);
}
})
.catch(console.error);
}, [shortcut]);📝 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(() => { | |
| hotkeysDB.saveHotkey(shortcut).catch(console.error); | |
| hotkeysDB | |
| .getHotkey(shortcut.action) | |
| .then((saved) => { | |
| if (saved && saved.keys !== shortcut.keys) { | |
| setCurrentShortcut(saved); | |
| } | |
| }) | |
| .catch(console.error); | |
| }, [shortcut]); | |
| useEffect(() => { | |
| hotkeysDB | |
| .getHotkey(shortcut.action) | |
| .then((saved) => { | |
| if (saved && saved.keys !== shortcut.keys) { | |
| setCurrentShortcut(saved); | |
| } else if (!saved) { | |
| // Only save if not already in DB | |
| hotkeysDB.saveHotkey(shortcut).catch(console.error); | |
| } | |
| }) | |
| .catch(console.error); | |
| }, [shortcut]); |
| { | ||
| ...restOptions, | ||
| scopes: [scope], | ||
| preventDefault: shortcut.preventDefault || preventDefault, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent preventDefault handling
There are multiple places where preventDefault is handled:
- In the handleKey callback
- As a parameter to useHotkeys
- From the shortcut object itself
This can lead to confusion and unexpected behavior.
{
...restOptions,
scopes: [scope],
- preventDefault: shortcut.preventDefault || preventDefault,
+ preventDefault: Boolean(currentShortcut.preventDefault || preventDefault),
}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
Documentation