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 ↗︎
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces a comprehensive user hotkeys system and Farsi language support. It adds a new database table and API endpoints for storing user-specific keyboard shortcuts, a frontend IndexedDB-based persistence layer, and a complete React integration for recording, editing, and using hotkeys via the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Shortcuts Page)
participant HotkeysDB (IndexedDB)
participant API (/api/v1/hotkeys)
participant DB
User->>UI (Shortcuts Page): Open settings/shortcuts
UI->>HotkeysDB: getAllHotkeys()
HotkeysDB->>API: (if not initialized) GET /api/v1/hotkeys
API->>DB: Query userHotkeys table
DB-->>API: Return shortcuts
API-->>HotkeysDB: Return shortcuts
HotkeysDB-->>UI: Return merged shortcuts
User->>UI: Edit/record a shortcut
UI->>HotkeysDB: saveHotkey(shortcut)
HotkeysDB->>API: PUT /api/v1/hotkeys (sync)
API->>DB: Upsert shortcut
DB-->>API: Ack
API-->>HotkeysDB: Ack
HotkeysDB-->>UI: Ack
User->>UI: Use hotkey in app
UI->>HotkeysDB: getHotkey(action)
HotkeysDB-->>UI: Return shortcut
UI->>UI: Activate handler for shortcut
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 28
🔭 Outside diff range comments (3)
apps/mail/components/ui/ai-sidebar.tsx (1)
45-91: 💡 Verification agent🧩 Analysis chain
Removed hotkey handlers for sidebar toggle.
The keyboard shortcuts for toggling the sidebar (Meta+0, Control+0) have been removed as part of the migration to a centralized hotkey management system. Ensure that this functionality is properly implemented in the new global hotkeys component to maintain the same user experience.
🏁 Script executed:
#!/bin/bash # Check if the sidebar toggle functionality exists in the new hotkey system echo "Searching for sidebar toggle hotkeys in the new implementation..." rg -A 3 -B 3 "toggleOpen|setOpen\(.*\!open|sidebar.*toggle" --glob "*.tsx" --glob "*.ts" | grep -v "ai-sidebar.tsx"Length of output: 389
Let’s locate where global hotkeys are registered and verify if Meta+0 / Control+0 for toggling the AI sidebar exist:
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "1) Searching for explicit 'Meta+0' or 'Control+0' registrations…" rg -n 'Meta\+0|Control\+0' --glob "*.ts*" || true echo "2) Finding any useHotkeys or hotkey hook usages…" rg -n 'useHotkeys|hotkey' --glob "*.ts*" --glob "*.tsx" || true echo "3) Listing files/folders named '*hotkey*'…" fd -HI hotkeyLength of output: 6947
We need to confirm whether Meta+0 / Control+0 is wired up in
global-hotkeys.tsx—it currently doesn’t appear in any hotkey files. Let’s inspect its registrations:
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Looking for useShortcuts in global-hotkeys.tsx:" rg -n "useShortcuts" -A 3 -B 1 apps/mail/lib/hotkeys/global-hotkeys.tsxLength of output: 484
We need to confirm whether the global shortcuts config defines Meta+0 / Control+0. Please run:
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the file exporting keyboardShortcuts echo "Finding keyboardShortcuts definition…" FILE=$(rg -l "export const keyboardShortcuts" --glob "*.ts*" --glob "*.js" | head -n1) if [[ -z "$FILE" ]]; then echo "ERROR: keyboardShortcuts export not found." exit 1 fi echo "Found in: $FILE" echo "Showing export and its entries:" rg -n "export const keyboardShortcuts" -A 50 "$FILE"Length of output: 2975
Add missing AI sidebar toggle hotkeys
It looks like the Meta+0 / Control+0 shortcuts for opening and closing the AI sidebar were removed but haven’t been re‑registered in the new global hotkey system. To restore the original UX:
• In apps/mail/config/shortcuts.ts, add a global shortcut entry:
export const keyboardShortcuts: Shortcut[] = [ { keys: ["c"], action: "newEmail", … }, … + // Toggle AI sidebar + { keys: ["mod", "0"], action: "toggleAISidebar", type: "combination", description: "Toggle AI sidebar", scope: "global" }, ];• In apps/mail/lib/hotkeys/global-hotkeys.tsx, wire up the handler:
import { keyboardShortcuts } from '@/config/shortcuts'; import { useShortcuts } from './use-hotkey-utils'; +import { useAISidebar } from '@/components/ui/ai-sidebar'; export function GlobalHotkeys() { + const { setOpen } = useAISidebar(); const scope = 'global'; const handlers = { newEmail: () => { … }, …, + toggleAISidebar: () => setOpen(open => !open), }; useShortcuts(keyboardShortcuts, handlers, { scope }); return null; }This will restore the original Meta+0 / Control+0 behavior for toggling the AI sidebar.
apps/mail/app/api/auth/early-access/route.ts (1)
23-71:⚠️ Potential issueRate limiting has been removed - potential security concern
The code has been modified to remove rate limiting on the early access registration endpoint. Without rate limiting, this endpoint could be vulnerable to abuse, such as spamming registrations or conducting denial of service attacks.
Consider reimplementing rate limiting or adding alternative protection mechanisms:
import { type NextRequest, NextResponse } from 'next/server'; import { earlyAccess } from '@zero/db/schema'; import { db } from '@zero/db'; import { Resend } from 'resend'; +import { Ratelimit } from '@upstash/ratelimit'; +import { Redis } from '@upstash/redis'; // Add back rate limiting configuration +const ratelimit = new Ratelimit({ + redis: Redis.fromEnv(), + limiter: Ratelimit.slidingWindow(5, '1 h'), + analytics: true, +}); export async function POST(req: NextRequest) { try { + // Get IP for rate limiting + const ip = req.headers.get('x-forwarded-for') || 'anonymous'; + const { success, limit, reset, remaining } = await ratelimit.limit(ip); + + // Return rate limit error if exceeded + if (!success) { + return NextResponse.json( + { error: 'Too many requests, please try again later' }, + { + status: 429, + headers: { + 'X-RateLimit-Limit': limit.toString(), + 'X-RateLimit-Remaining': remaining.toString(), + 'X-RateLimit-Reset': reset.toString(), + } + } + ); + } const body = await req.json(); console.log('Request body:', body); // Rest of the function remains unchangedapps/mail/hooks/use-hot-key.ts (1)
1-33: 🛠️ Refactor suggestionConsider removing this file entirely
Since the main
useHotKeyhook has been removed in favor of thereact-hotkeys-hooklibrary, the remaininguseKeyStatehook appears to be redundant. It also has potential issues with its implementation:
- It uses a global map for state management which is non-idiomatic for React
- The
setTimeoutwith 0 delay to initialize listeners is a workaround- Event listeners are never fully cleaned up, which could cause memory leaks
Consider removing this file completely and migrating any remaining usage of
useKeyStateto the new hotkey system.If
useKeyStateis still needed, reimplement it using React's state management and proper cleanup:// Alternative implementation import { useState, useEffect } from 'react'; export function useKeyState(targetKey: string): boolean { const [keyPressed, setKeyPressed] = useState(false); useEffect(() => { const downHandler = ({ key }: KeyboardEvent) => { if (key === targetKey) { setKeyPressed(true); } }; const upHandler = ({ key }: KeyboardEvent) => { if (key === targetKey) { setKeyPressed(false); } }; window.addEventListener('keydown', downHandler); window.addEventListener('keyup', upHandler); // Cleanup return () => { window.removeEventListener('keydown', downHandler); window.removeEventListener('keyup', upHandler); }; }, [targetKey]); return keyPressed; }
🧹 Nitpick comments (27)
apps/mail/components/draft/drafts.tsx (1)
36-37: Review unused searchIconRefA new ref
searchIconRefhas been added but doesn't appear to be used anywhere in the component.If this ref isn't being used, consider removing it to avoid confusion. If it's intended for future use or is used by a component not shown in this file, add a comment explaining its purpose.
- const searchIconRef = useRef<any>(null); -Additionally, the AI summary mentions an import for
useHotKeywas removed, which aligns with the migration to react-hotkeys-hook in this PR.apps/mail/lib/hotkeys/thread-display-hotkeys.tsx (1)
17-17: Consider caching the filtered shortcutsThis filter operation will run on each render. Consider memoizing the result to avoid unnecessary recalculations.
+ const threadDisplayShortcuts = useMemo( + () => keyboardShortcuts.filter((shortcut) => shortcut.scope === scope), + [scope] + ); - const threadDisplayShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope);Don't forget to import
useMemofrom React.packages/db/migrations/0022_round_violations.sql (1)
8-8: Consider cascade deletion strategyThe current foreign key constraint uses "no action" for ON DELETE, which could lead to orphaned hotkey records if a user is deleted.
Consider using CASCADE for deletion if orphaned hotkey records would be useless:
-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;apps/mail/lib/hotkeys/global-hotkeys.tsx (1)
21-21: Consider caching the filtered shortcutsThis filter operation will run on each render. Consider memoizing the result to avoid unnecessary recalculations.
+import { useMemo } from 'react'; // ... + const globalShortcuts = useMemo( + () => keyboardShortcuts.filter((shortcut) => shortcut.scope === scope), + [scope] + ); - const globalShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope);apps/mail/components/providers/hotkey-provider-wrapper.tsx (1)
17-20: Consider conditional rendering of scope-specific hotkey componentsAll hotkey components are always rendered regardless of whether their scope is active. This could lead to unnecessary hook executions for inactive scopes.
Consider conditionally rendering the scope-specific components based on current application state:
import { useState, useEffect } from 'react'; import { usePathname } from 'next/navigation'; // Inside the component: + const pathname = usePathname(); + const [activeComponents, setActiveComponents] = useState(['global']); + + useEffect(() => { + if (pathname.includes('/mail/compose')) { + setActiveComponents(['global', 'compose']); + } else if (pathname.includes('/mail/thread')) { + setActiveComponents(['global', 'thread-display']); + } else if (pathname.includes('/mail')) { + setActiveComponents(['global', 'mail-list']); + } else { + setActiveComponents(['global']); + } + }, [pathname]); // Then in the JSX: <GlobalHotkeys /> - <MailListHotkeys /> - <ThreadDisplayHotkeys /> - <ComposeHotkeys /> + {activeComponents.includes('mail-list') && <MailListHotkeys />} + {activeComponents.includes('thread-display') && <ThreadDisplayHotkeys />} + {activeComponents.includes('compose') && <ComposeHotkeys />}Dockerfile (1)
5-6: Global installation may be unnecessaryInstalling packages globally in a Docker container is typically unnecessary since the container is dedicated to running a specific application.
-# Install turbo globally -RUN bun install -g next turbo +# These will be available through dependencies in package.jsonapps/mail/lib/hotkeys/mail-list-hotkeys.tsx (4)
22-33: Use typed custom events for better type safetyThe component uses a custom event without proper TypeScript definitions, which might lead to type safety issues.
const hoveredEmailId = useRef<string | null>(null); +// Define a custom event type +interface EmailHoverEvent extends CustomEvent { + detail: { id: string | null }; +} useEffect(() => { - const handleEmailHover = (event: CustomEvent<{ id: string | null }>) => { + const handleEmailHover = (event: EmailHoverEvent) => { hoveredEmailId.current = event.detail.id; }; - window.addEventListener('emailHover', handleEmailHover as EventListener); + window.addEventListener('emailHover', handleEmailHover as EventListener); return () => { - window.removeEventListener('emailHover', handleEmailHover as EventListener); + window.removeEventListener('emailHover', handleEmailHover as EventListener); }; }, []);
52-63: Extract duplicate toast.promise logic into a helper functionThe component has duplicate toast.promise logic for marking emails as unread that could be extracted into a helper function.
+const handleMarkAsUnread = useCallback((ids: string[]) => { + return toast.promise(markAsUnreadAction({ ids }), { + loading: t('common.actions.loading'), + success: async () => { + await Promise.all([mutate(), mutateStats()]); + return t('common.mail.markedAsUnread'); + }, + error: t('common.mail.failedToMarkAsUnread'), + }); +}, [mutate, mutateStats, t]); + const markAsUnread = useCallback(() => { if (hoveredEmailId.current) { - toast.promise(markAsUnreadAction({ ids: [hoveredEmailId.current] }), { - loading: t('common.actions.loading'), - success: async () => { - await Promise.all([mutate(), mutateStats()]); - return t('common.mail.markedAsUnread'); - }, - error: t('common.mail.failedToMarkAsUnread'), - }); + handleMarkAsUnread([hoveredEmailId.current]); return; }
65-78: Apply the extracted helper function here as wellFor consistency, use the same helper function for the bulk selection case.
const idsToMark = mail.bulkSelected; if (idsToMark.length === 0) { toast.info(t('common.mail.noEmailsToSelect')); return; } -toast.promise(markAsUnreadAction({ ids: idsToMark }), { - loading: t('common.actions.loading'), - success: async () => { - await Promise.all([mutate(), mutateStats()]); - return t('common.mail.markedAsUnread'); - }, - error: t('common.mail.failedToMarkAsUnread'), -}); +handleMarkAsUnread(idsToMark);
13-91: Consider adding a debounce mechanism to prevent action spamThere's no protection against rapidly firing shortcut actions which could lead to API spamming or race conditions.
Add debounce logic to prevent rapid firing of shortcut actions:
import { debounce } from 'lodash'; // Inside the component: const debouncedMarkAsUnread = useCallback( debounce(() => { // Existing markAsUnread logic }, 300), [mail.bulkSelected, mutate, mutateStats, t] ); // Then use debouncedMarkAsUnread in your handlers const handlers = { markAsUnread: debouncedMarkAsUnread, selectAll, };apps/mail/app/(routes)/settings/shortcuts/hotkey-recorder.tsx (2)
23-57: Limit the number of keys that can be recordedThere's no limit to how many keys can be recorded in a shortcut, which could lead to overly complex shortcuts that are difficult to remember or execute.
const handleKeyDown = (e: KeyboardEvent) => { e.preventDefault(); if (!isRecording) return; const key = e.key === ' ' ? 'Space' : e.key; const formattedKey = key.length === 1 ? key.toUpperCase() : key; + // Limit to 3 keys maximum for usability + if (recordedKeys.length >= 3) { + return; + } if (!recordedKeys.includes(formattedKey)) { setRecordedKeys((prev) => [...prev, formattedKey]); } };
81-88: Add a visual indication when no keys are presentWhen no keys are recorded or provided as current keys, there's no visual indication to the user.
<div className="flex gap-2"> + {(recordedKeys.length > 0 ? recordedKeys : currentKeys).length === 0 && ( + <div className="text-muted-foreground italic"> + {t('pages.settings.shortcuts.noKeysAssigned' as MessageKey)} + </div> + )} {(recordedKeys.length > 0 ? recordedKeys : currentKeys).map((key) => ( <kbd key={key}docker-compose.yaml (1)
36-36: Remove trailing whitespaceThere's a trailing whitespace on this line that should be removed.
- - 8079:80 - + - 8079:80🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 36-36: trailing spaces
(trailing-spaces)
apps/mail/components/mail/mail.tsx (1)
254-270: Remove console.log statements from production codeWhile the hotkey scope management logic is sound, there are several console.log statements that should be removed before pushing 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]);apps/mail/app/api/resend/send-early-access/route.ts (3)
56-61: Consider extracting the 'from' email to an environment variable.The sender email address is currently hardcoded. For better flexibility and configuration management, consider extracting this to an environment variable.
+// Get sender email from environment variable with fallback +const senderEmail = process.env.RESEND_SENDER_EMAIL || 'onboarding@0.email'; +const senderName = process.env.RESEND_SENDER_NAME || '0.email'; const response = await resend.emails.send({ - from: '0.email <onboarding@0.email>', + from: `${senderName} <${senderEmail}>`, to: email, subject: subject, html: content, });
63-71: Simplify conditional checks with optional chaining.The nested property checks can be simplified using optional chaining for better readability.
-if (response && response.data && response.data.id) { +if (response?.data?.id) { console.log(`Successfully sent email to ${email}`); successfulEmails.push(email); results.push({ email, success: true, response }); } else { console.error(`Unexpected response format for ${email}:`, response); failedEmails.push(email); results.push({ email, success: false, error: 'Unexpected response format' }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
79-81: Consider making the rate limit delay configurable.The delay between email sends is hardcoded to 500ms. For better flexibility, especially in different environments, consider making this configurable.
+// Rate limit delay in milliseconds - configurable via environment variable +const rateLimitDelay = parseInt(process.env.RESEND_RATE_LIMIT_DELAY || '500', 10); // Add a small delay between requests to avoid rate limiting if (i < emails.length - 1) { - await new Promise(resolve => setTimeout(resolve, 500)); + await new Promise(resolve => setTimeout(resolve, rateLimitDelay)); }apps/mail/components/mail/mail-list.tsx (2)
495-496: Remove TODO comment from production code.There is a TODO comment in the production code that should be addressed or removed.
- // TODO: debug const allIds = items.map((item) => item.id);
557-562: Remove console log statements from production code.There are console log statements that appear to be for debugging purposes. These should be removed from production code.
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'); }}apps/mail/app/api/resend/add-to-audience/route.ts (1)
74-74: Use optional chaining for more concise codeReplace the multiple condition checks with optional chaining for better readability.
-if (response && response.data && response.data.id) { +if (response?.data?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/mail/lib/hotkeys/hotkeys-db.ts (1)
62-68:serverHotkeysLoadedset before fetch completes => premature sync attempts
Because the flag is flipped totruebefore awaitingfetchServerHotkeys(), later calls tosaveAllHotkeys()will try to POST even if the fetch failed (e.g., offline).
Consider toggling the flag only after a successful fetch to avoid unnecessary network noise and misleading console errors.- if (!serverHotkeysLoaded) { - serverHotkeysLoaded = true; - const serverHotkeys = await fetchServerHotkeys(); + if (!serverHotkeysLoaded) { + const serverHotkeys = await fetchServerHotkeys(); + serverHotkeysLoaded = !!serverHotkeys; // true only if we got dataapps/mail/app/(routes)/settings/shortcuts/page.tsx (1)
40-52: Reset‑to‑defaults performs N writes instead of one
CallingsaveHotkeyinside a loop fires N separate transactions and N POSTs. A bulk helper (e.g., a publicsaveAllHotkeys) would cut IO dramatically and remove race conditions.apps/mail/hooks/use-mail-navigation.ts (1)
205-212: Potential double‑handling of arrow keys (repeat & non‑repeat)
useHotkeysalready registers handlers for single key presses, while the manualkeydownlistener below handlesevent.repeatlogic. When a key is held, both listeners fire:
- the
useHotkeyshandler on the initial press,- the custom listener on every repeat.
This works but can lead to visible jump‑backs if state updates collide. Consider consolidating the repeat logic into the same handler or throttling in one place only.
apps/mail/app/(full-width)/early-access/early-access.tsx (3)
109-123: Shadowing and stale state risk inhandleCustomEmails
selectedUsersdeclared inside the function shadows the state setter’s variable name.
Although currently harmless, this pattern is easy to mis‑read and can lead to bugs when refactoring.-const selectedUsers = earlyAccessUsers.filter(user => - emails.includes(user.email) -); +const matches = earlyAccessUsers.filter((user) => + emails.includes(user.email), +);
268-391: Hard‑coding a 120‑line HTML template in a string literal hurts bundle size & DXEmbedding the whole email HTML inflates the client bundle (this component ships to the browser).
Consider:
• Moving the template to a server‑side API or a static file.
• Sending only a template key from the client; let the API compose the email body.This reduces bundle bloat and keeps presentational email markup out of the React tree.
490-497: Table semantics broken byflexon<tr>Applying Flexbox to table rows can break accessibility and layout in some browsers.
Prefer styling individual<td>elements or wrap rows in<div role="row">instead of mixing table semantics with flex layouts.apps/mail/lib/hotkeys/use-hotkey-utils.ts (1)
92-104: Unconditional DB write on every mount
hotkeysDB.saveHotkey(shortcut)runs even when the user never edited a shortcut, resulting in redundant IndexedDB writes.
Guard the save behind a flag or compare versions before persisting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/mail/public/email.pngis excluded by!**/*.pngbun.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
Dockerfile(1 hunks)apps/mail/.env.example(1 hunks)apps/mail/app/(full-width)/early-access/early-access.tsx(1 hunks)apps/mail/app/(full-width)/early-access/page.tsx(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/auth/early-access/route.ts(2 hunks)apps/mail/app/api/resend/add-to-audience/route.ts(1 hunks)apps/mail/app/api/resend/send-early-access/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/home/hero.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 (9)
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/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/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/app/(full-width)/early-access/page.tsx (3)
packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (2)
earlyAccess(68-75)user(7-17)apps/mail/app/(full-width)/early-access/early-access.tsx (1)
EarlyAccessClient(86-552)
apps/mail/components/mail/reply-composer.tsx (1)
apps/mail/types/index.ts (1)
Sender(32-35)
apps/mail/components/mail/mail-list.tsx (4)
apps/mail/hooks/use-threads.ts (1)
useThread(131-144)apps/mail/lib/utils.ts (1)
cn(51-51)apps/mail/lib/email-utils.client.tsx (1)
highlightText(59-77)apps/mail/components/ui/tooltip.tsx (1)
TooltipContent(59-59)
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 (2)
apps/mail/config/shortcuts.ts (2)
Shortcut(3-10)keyboardShortcuts(12-34)apps/mail/lib/hotkeys/hotkeys-db.ts (1)
hotkeysDB(146-146)
🪛 Biome (1.9.4)
apps/mail/app/api/resend/add-to-audience/route.ts
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
apps/mail/app/api/resend/send-early-access/route.ts
[error] 63-63: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
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)
🪛 YAMLlint (1.35.1)
docker-compose.yaml
[error] 36-36: trailing spaces
(trailing-spaces)
🔇 Additional comments (59)
apps/mail/components/home/hero.tsx (1)
110-110: Verify the intention of the environment logic inversionThe conditional logic has been inverted from
process.env.NODE_ENV === 'development'toprocess.env.NODE_ENV !== 'development'. This changes which UI component is displayed based on the environment - the button group will now appear in production/staging instead of development.Is this intentional? Given the PR description contains "NEVER MERGE", this might be a temporary change for testing early access features. Please confirm the intended behavior.
apps/mail/.env.example (1)
26-26: Added configuration for Resend audience managementThe new
RESEND_AUDIENCE_IDenvironment variable supports the early access email features introduced in this PR.Since the PR includes new API routes for early access email management with Resend, this addition is appropriate for configuration purposes.
apps/mail/package.json (1)
95-95: Added react-hotkeys-hook for improved keyboard shortcut managementAdding the react-hotkeys-hook library supports the migration from a custom hotkey hook to a more comprehensive hotkey management system.
This addition aligns with the global refactoring of keyboard shortcut handling described in the AI summary. The version constraint (^5.0.1) follows standard practice, allowing minor version updates.
packages/db/migrations/meta/_journal.json (1)
158-165: New database migration added for user hotkeys.This migration entry (0022_round_violations) appears to introduce the
mail0_user_hotkeystable for supporting user-specific keyboard shortcut configurations, which aligns with the overall PR focus on implementing a comprehensive hotkeys system.i18n.json (3)
21-22: Added Farsi (Persian) localization support.The addition of "fa" to the locale targets list enables Persian language support in the application, expanding accessibility to Farsi-speaking users.
27-27: Reformatted buckets.json.include array.The formatting change simplifies the include pattern to a single line, maintaining the same functionality while improving readability.
30-31: Added missing newline at end of file.Adding a newline at the end of the file follows best practices for text files and prevents potential issues with some version control systems and text editors.
apps/mail/config/navigation.ts (1)
175-175: Enabled shortcuts settings page in navigation.Commenting out the
disabled: trueproperty makes the keyboard shortcuts settings page accessible to users, which aligns with the PR's focus on implementing a comprehensive user hotkeys system.apps/mail/i18n/config.ts (3)
2-17: LGTM: Added Farsi language supportThe addition of Farsi language with code 'fa' looks good and aligns with the PR's purpose of adding Persian language support. Also noting the consistent style change to single quotes.
24-24: LGTM: Style consistencyProperly updated to single quotes to maintain consistency with other string literals.
28-30: LGTM: Arrow function formattingProperly reformatted arrow function with parentheses around the parameter and trailing commas for better code consistency.
apps/mail/app/(routes)/mail/layout.tsx (2)
2-3: LGTM: Added hotkey components importsCorrectly imported the new hotkey-related components to support the keyboard shortcuts system.
7-11: LGTM: Implemented HotkeyProviderWrapperProperly wrapped the layout with the new hotkey provider and added the GlobalHotkeys component. This correctly implements the new architecture for keyboard shortcuts using react-hotkeys-hook.
packages/db/src/schema.ts (1)
50-57: LGTM: Added userHotkeys table for persistent shortcutsThe new userHotkeys table is well-designed with:
- userId as primary key with reference to the user table
- shortcuts as a JSONB field for flexible storage
- Proper timestamp tracking fields
This provides the necessary database structure for the new user-specific keyboard shortcut system.
apps/mail/locales/en.json (1)
402-405: LGTM: Added localization strings for hotkey functionalityThe new localization strings support the hotkey recording and management UI:
- "recordHotkey" for initiating the recording process
- "pressKeys" and "releaseKeys" for guiding users during recording
- "selectAll" for the select all shortcut action
These additions properly support the new keyboard shortcuts system.
apps/mail/app/(routes)/layout.tsx (2)
4-4: LGTM! Import added for new hotkey providerThis import enables the new hotkey management system to be integrated at the application layout level.
10-25: Hotkey management architecture improvementWrapping the entire application with
HotkeyProviderWrapperensures consistent keyboard shortcut handling throughout the app. This hierarchical approach aligns with best practices for context-based feature management in React.apps/mail/components/draft/drafts-list.tsx (6)
12-12: LGTM! Migration to react-hotkeys-hook librarySwitching from a custom hook to a well-maintained library is a good improvement for stability and maintainability.
95-96: LGTM! Added trailing comma for consistent styleThis minor formatting change improves code consistency.
138-156: LGTM! Migrated modifier key detection to react-hotkeys-hookThe implementation correctly handles the migration from the custom hook to the external library for modifier key detection.
158-204: LGTM! Migrated marking functions to react-hotkeys-hookThe implementation successfully migrates the mark as read/unread hotkey functionality to the new library.
220-242: Improved hotkey implementation with event preventionThe migration adds proper event prevention which was likely missing in the previous implementation, preventing default browser behaviors when using these shortcuts.
232-242:Details
❓ Verification inconclusive
Verify Meta+n and Control+n shortcut behavior
These shortcuts currently perform the same action as Meta+a and Control+a (selecting all items), which seems potentially redundant or confusing. Typically, 'n' shortcuts are used for "new" actions rather than "select all".
🏁 Script executed:
#!/bin/bash # Check if there are other uses of Meta+n or Control+n shortcuts in the codebase echo "Checking for Meta+n usage:" rg -i "meta\+n|cmd\+n" --type tsx --type ts echo "Checking for Control+n usage:" rg -i "control\+n|ctrl\+n" --type tsx --type ts echo "Checking keyboard shortcut config for 'n' key:" rg "'n'" ./apps/mail/config/shortcuts.tsLength of output: 441
I wasn’t able to confirm any existing Meta+n or Control+n bindings with the previous patterns. Let’s broaden the search:
🏁 Script executed:
#!/bin/bash # Search for explicit useHotkeys bindings echo "Looking for useHotkeys calls with Meta+n or Control+n..." rg -n "useHotkeys.*['\"]Meta\+n['\"]" || echo "No useHotkeys binding for Meta+n" rg -n "useHotkeys.*['\"]Control\+n['\"]" || echo "No useHotkeys binding for Control+n" # Search for any literal references to Meta+n or Ctrl+n echo "Looking anywhere else for Meta+n or Control+n literals..." rg -n "Meta\+n" || echo "No Meta+n literals found" rg -n "Control\+n" || echo "No Control+n literals found" # Find and inspect shortcut config files echo "Locating shortcuts config files..." fd -e ts -e js shortcuts for file in $(fd -e ts -e js shortcuts); do echo "--- Scanning $file for 'n' key definitions ---" rg -n "['\"]n['\"]" "$file" || echo " No 'n' entries in $file" doneLength of output: 1273
Verify Meta+n and Control+n shortcut assignments
I confirmed these are the only occurrences of Meta+n and Control+n bindings in the codebase, and there’s no global “n” shortcut defined elsewhere. They currently duplicate the select‑all behavior already mapped to Meta+a / Ctrl+a, which can be confusing since “n” is typically used for “new” actions.
Please review the following location and confirm whether this is intentional or if you’d prefer to:
- Keep “n” for select‑all (and accept the UX deviation),
- Switch to a more conventional key (e.g. Meta+a / Ctrl+a) for select‑all, or
- Reserve “n” for a “new draft” shortcut instead.
• apps/mail/components/draft/drafts-list.tsx (232–242)
useHotkeys('Meta+n', async (event) => { event.preventDefault(); resetSelectMode(); selectAll(); }); useHotkeys('Control+n', async (event) => { event.preventDefault(); resetSelectMode(); selectAll(); });apps/mail/app/api/auth/early-access/route.ts (2)
70-71: LGTM! Status code formatting consistencyThe formatting change for the status code in the response is a minor style improvement.
84-85: LGTM! Status code formatting consistencyThe formatting change for the status code in the response is a minor style improvement.
apps/mail/lib/hotkeys/thread-display-hotkeys.tsx (1)
14-14: Verify the synthetic KeyboardEvent implementationCreating a synthetic KeyboardEvent doesn't actually dispatch it to any DOM element, so this won't trigger any Escape key handlers elsewhere in the application.
If the intention is to trigger an Escape key handler elsewhere, you need to dispatch the event:
- closeView: () => closeView(new KeyboardEvent('keydown', { key: 'Escape' })), + closeView: () => { + const event = new KeyboardEvent('keydown', { key: 'Escape' }); + closeView(event); + document.dispatchEvent(event); // Dispatch to document or a specific element + },packages/db/migrations/0022_round_violations.sql (1)
1-6: LGTM - Table structure is soundThe table structure for storing user hotkeys is well-designed with appropriate columns for user identification, JSON data storage, and timestamp tracking.
apps/mail/components/providers/hotkey-provider-wrapper.tsx (1)
16-16: LGTM - Good choice of initially active scopesSetting 'global' as the initially active scope is appropriate since it handles application-wide shortcuts.
apps/mail/app/(routes)/settings/shortcuts/hotkey-recorder.tsx (1)
71-72:Details
❌ Incorrect review comment
Type assertion may indicate a missing translation
Using type assertion (
as MessageKey) suggests the translation key might not be properly defined in your type system.
🏁 Script executed:
#!/bin/bash # Check if the translation key exists in the localization files echo "Checking for translation key 'pages.settings.shortcuts.actions.recordHotkey'" rg -n "recordHotkey" --glob="*.json" apps/mail/messages/Length of output: 285
🏁 Script executed:
#!/bin/bash # Search for the translation key across all JSON files in the repo rg -n "pages.settings.shortcuts.actions.recordHotkey" --glob="*.json" rg -n "recordHotkey" --glob="*.json"Length of output: 179
🏁 Script executed:
#!/bin/bash # Show nested context for "recordHotkey" in the English locale to confirm its JSON structure rg -n '"recordHotkey"' -A5 -B5 --glob="*.json" apps/mail/locales/en.jsonLength of output: 537
🏁 Script executed:
#!/bin/bash # Locate the MessageKey type definition(s) rg -n "type MessageKey" -R . # If found, search within for the specific key # Adjust the path below based on the first command's output # e.g., rg -n "recordHotkey" path/to/message-keys.d.tsLength of output: 435
🏁 Script executed:
#!/bin/bash # Search for MessageKey definition across TypeScript files rg -n "type MessageKey" -g "*.ts" -g "*.tsx" -g "*.d.ts" rg -n "MessageKey" -g "*.ts" -g "*.tsx" -g "*.d.ts"Length of output: 2152
Ignore missing translation warning for
recordHotkey
The keypages.settings.shortcuts.actions.recordHotkeyis defined in apps/mail/locales/en.json (see line 402). Theas MessageKeyassertion here is merely satisfying the TypeScript signature of t() for inline string literals and matches existing patterns elsewhere in the codebase (e.g., in nav‑main.tsx). No change is needed.Likely an incorrect or invalid review comment.
docker-compose.yaml (2)
32-32: Good improvement for securityChanging from a hardcoded token to an environment variable improves security by allowing secure token management outside the codebase.
37-52: LGTM: Docker service configuration looks goodThe new
appservice is well structured with appropriate container name, build configuration, port mapping, dependencies, and environment variables. Using environment variables for sensitive connection strings is a good security practice.apps/mail/app/(full-width)/early-access/page.tsx (3)
7-16: Looks good: Proper data fetching with normalizationThe function correctly queries early access users and normalizes the
isEarlyAccessflag to ensure consistent data structure.
18-60: Well-implemented server action with proper error handlingThis server action follows best practices:
- Input validation
- Comprehensive error handling with logging
- Properly structured response objects
- Cache revalidation after database updates
The SQL query is safely constructed using parameterized queries with the SQL tag function to prevent SQL injection.
62-73: Effective server component implementationThe component properly fetches data and passes it to the client component with appropriate props. The structure is clean and follows the server component pattern well.
apps/mail/components/mail/mail.tsx (2)
55-55: Good refactoring to standard hotkeys libraryThe import and usage of
useHotkeysContextfrom 'react-hotkeys-hook' is a good move toward using a standard library instead of custom implementations.Also applies to: 221-221
272-274: Good use of useCallback for event handlerProperly memoizing the handler function with useCallback prevents unnecessary re-renders and follows React best practices.
apps/mail/locales/fa.json (1)
1-432:Details
✅ Verification successful
Well-structured localization file for Farsi
The new Farsi localization file follows the correct structure with all the necessary translation keys. The empty strings serve as placeholders for future translations.
Consider:
- Adding a plan or timeline for completing the translations
- Using a translation management system to help translators fill in these values
To ensure this file covers all the required keys, run this script to compare with an existing complete locale file:
🏁 Script executed:
#!/bin/bash # Check if the new Farsi locale has all the keys from the English locale # Find the English locale file ENGLISH_FILE=$(find apps/mail/locales -name "en*.json" | head -1) if [ -z "$ENGLISH_FILE" ]; then echo "Error: Could not find English locale file" exit 1 fi echo "Comparing Farsi locale with $ENGLISH_FILE..." # Use jq to extract keys from both files and compare ENGLISH_KEYS=$(jq -r 'path(..) | select(length > 0) | map(tostring) | join(".")' "$ENGLISH_FILE" | sort) FARSI_KEYS=$(jq -r 'path(..) | select(length > 0) | map(tostring) | join(".")' "apps/mail/locales/fa.json" | sort) # Compare the keys DIFF=$(diff <(echo "$ENGLISH_KEYS") <(echo "$FARSI_KEYS")) if [ -z "$DIFF" ]; then echo "✅ All keys from English locale are present in Farsi locale" else echo "❌ Missing or extra keys in Farsi locale:" echo "$DIFF" fiLength of output: 50839
Farsi locale keys verified and structurally complete
✅ All keys from
apps/mail/locales/en.jsonare present inapps/mail/locales/fa.json.Next steps (optional improvements):
- Define a plan or timeline for translating the placeholder strings.
- Integrate a translation management system (e.g., Crowdin, Lokalise) to streamline future translations.
apps/mail/config/shortcuts.ts (1)
12-33: Well structured keyboard shortcuts with clear organization by scope.The keyboard shortcuts are well-defined with a clear type structure and organized by scope. The use of platform-agnostic 'mod' key is a good practice.
One suggestion for future improvement might be to group shortcuts by scope in the code for better readability and maintenance, especially as the list grows:
-export const keyboardShortcuts: Shortcut[] = [ - { keys: ["c"], action: "newEmail", type: "single", description: "Compose new email", scope: "global" }, +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" }, + { keys: ["g", "i"], action: "inbox", type: "combination", description: "Go to inbox", scope: "global" }, + { keys: ["g", "t"], action: "sentMail", type: "combination", description: "Go to sent mail", scope: "global" }, + { keys: ["/"], action: "search", type: "single", description: "Search", scope: "global" }, + { keys: ["z"], action: "undoLastAction", type: "single", description: "Undo last action", scope: "global" }, + { keys: ["?"], action: "helpWithShortcuts", type: "single", description: "Show keyboard shortcuts", scope: "global" }, + + // Compose shortcuts { keys: ["mod", "Enter"], action: "sendEmail", type: "combination", description: "Send email", scope: "compose" }, + + // Thread display shortcuts { keys: ["r"], action: "reply", type: "single", description: "Reply to email", scope: "thread-display" }, { keys: ["a"], action: "replyAll", type: "single", description: "Reply all", scope: "thread-display" }, { keys: ["f"], action: "forward", type: "single", description: "Forward email", scope: "thread-display" }, - { keys: ["g", "d"], action: "goToDrafts", type: "combination", description: "Go to drafts", scope: "global" }, - { keys: ["g", "i"], action: "inbox", type: "combination", description: "Go to inbox", scope: "global" }, - { keys: ["g", "t"], action: "sentMail", type: "combination", description: "Go to sent mail", scope: "global" }, + { keys: ["mod", "p"], action: "printEmail", type: "combination", description: "Print email", scope: "thread-display" }, + { keys: ["i"], action: "viewEmailDetails", type: "single", description: "View email details", scope: "thread-display" }, + + // Mail list shortcuts { keys: ["#"], action: "delete", type: "single", description: "Delete email", scope: "mail-list" }, - { keys: ["/"], action: "search", type: "single", description: "Search", scope: "global" }, { keys: ["u"], action: "markAsUnread", type: "single", description: "Mark as unread", scope: "mail-list" }, { keys: ["m"], action: "muteThread", type: "single", description: "Mute thread", scope: "mail-list" }, - { keys: ["mod", "p"], action: "printEmail", type: "combination", description: "Print email", scope: "thread-display" }, { 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" }, - { keys: ["?"], action: "helpWithShortcuts", type: "single", description: "Show keyboard shortcuts", scope: "global" }, { keys: ["mod", "a"], action: "selectAll", type: "combination", description: "Select all emails", scope: "mail-list", preventDefault: true }, ];apps/mail/components/mail/mail-list.tsx (2)
154-154: The custom event emission for emailHover integrates well with the new hotkey system.The change to dispatch a custom event when mouse leaves an email thread is a good approach for communicating with other components that need to react to hover state changes.
33-34: Good refactoring to use react-hotkeys-hook package.The refactoring to use the
useHotkeysContexthook from react-hotkeys-hook is a good approach for managing keyboard shortcuts in a more structured and maintainable way.apps/mail/app/api/resend/add-to-audience/route.ts (6)
1-6: Well-structured initialization approachThe file properly imports the necessary dependencies and includes a clear comment about environment variable handling strategy, which is a good practice for API routes.
7-23: Strong environmental configuration validationThe implementation wisely validates the presence of required environment variables before proceeding with API operations. The error handling includes detailed logging with appropriate status codes.
25-37: Thorough request validationGood practice to validate the request body structure before processing, with clear error responses that include specific reasons for failure.
41-100: Well-implemented sequential processing with rate limitingThe email processing loop intelligently handles each email individually with proper error isolation. The 500ms delay between requests is an excellent approach to prevent rate limiting issues with the Resend API.
🧰 Tools
🪛 Biome (1.9.4)
[error] 74-74: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
102-124: Comprehensive response structureThe response includes detailed information about both successful and failed operations, which is excellent for client-side error handling and debugging.
125-149: Robust error handlingThe comprehensive try-catch with detailed error reporting, including stack traces and timestamp, follows best practices for API error handling. The attempt to stringify errors for diagnostics is particularly helpful for troubleshooting.
apps/mail/components/mail/reply-composer.tsx (5)
24-44: Improved import organizationThe imports have been reorganized for better readability, with React hooks consolidated into a single import statement and the new
useHotkeysContexthook added fromreact-hotkeys-hook.
161-161: Added hotkey context integrationThe component now integrates with the hotkey system by using the
useHotkeysContexthook, providing access toenableScopeanddisableScopefunctions.
270-282: Improved code formatting in recipient mappingThe formatting of the recipient mapping functions has been improved for better readability while maintaining the same functionality.
504-512: More readable JSON message structureThe JSON structure for checking empty messages has been reformatted for better readability with consistent indentation.
924-937: Added hotkey scope managementThe new effect hook properly manages the "compose" hotkey scope, enabling it when the composer is open and disabling it when closed or on cleanup. The console logging statements are useful for debugging the hotkey lifecycle.
This integration with the new hotkey system allows for more granular control of keyboard shortcuts based on the UI context.
apps/mail/components/create/create-email.tsx (5)
2-44: Reorganized import statementsThe imports have been reorganized with dropdown-related components grouped together and the new
useHotkeysContexthook added fromreact-hotkeys-hook.
103-103: Added hotkey context integrationThe component now integrates with the hotkey system by destructuring the necessary functions from
useHotkeysContext.
464-472: Added compose scope managementThe component now properly manages the "compose" hotkey scope with a dedicated effect hook. This enables contextual keyboard shortcuts when the email composer is active, improving user experience.
521-616: Improved JSX structure and formattingThe JSX structure has been improved with better indentation and consistent class naming. This makes the code more readable without changing functionality.
679-736: Enhanced contact selection UI componentsThe email recipient input fields have been refactored with more consistent styling and structure, improving maintainability while preserving functionality.
packages/db/migrations/meta/0022_snapshot.json (2)
585-635: Well-designed user hotkeys table schemaThe new
mail0_user_hotkeystable is properly structured to support the hotkey customization features:
- Uses
user_idas primary key with a foreign key tomail0_user- Employs
jsonbtype for theshortcutsfield, which allows for flexible storage of keyboard shortcut configurations- Includes standard timestamp fields for tracking creation and updates
This table design complements the frontend hotkey management changes implemented in the React components.
1-763:Details
❓ Verification inconclusive
Verify the migration deployment process
This is a substantial database schema snapshot that includes multiple tables. Before deploying to production, ensure you have a rollback strategy in place and have tested the migration in a staging environment.
🏁 Script executed:
#!/bin/bash # Check if there are existing hotkey-related tables or columns in the database # that might conflict with this migration echo "Checking for existing hotkey-related database objects..." if command -v pg_dump &> /dev/null; then echo "This would normally check your database schema, but we're in a sandbox" echo "In production, you would run:" echo " pg_dump -s -t '*hotkey*' \$DATABASE_URL" echo " pg_dump -s | grep -i 'hotkey'" else echo "PostgreSQL client not available in sandbox" fi # Check for migration-related files echo "Looking for related migration files:" fd --type f --glob "*hotkey*.sql" --exec echo "Found migration file: {}"Length of output: 396
Ensure a Robust Migration Deployment & Rollback Plan
Before rolling out this extensive schema snapshot, please verify the following:
- You’ve tested the migration (and its down‐path) in a staging environment.
- A full schema‐and‐data backup is in place for production.
- You have an automated or well‐documented rollback (down‐migration) procedure.
- All foreign keys, unique constraints and defaults behave as expected post‐migration.
- No conflicting “hotkey” objects or prior migrations exist (no
*hotkey*.sqlfiles were found in the repo).apps/mail/hooks/use-mail-navigation.ts (1)
278-285: MissingcontainerRef.currentin hook dependency list may skip re‑binding on ref changes
useEffectdepends oncontainerRef(an object that is stable) instead ofcontainerRef.current. If a new container is assigned, the scroll logic might still target the old element.Would you confirm whether
containerRefever changes? If yes, includecontainerRef.currentin the dependency array or refactor to use a callback ref.
| import { useTranslations } from 'next-intl'; | ||
| import { createContext, useContext } from 'react'; |
There was a problem hiding this comment.
Added useTranslations import but not using it.
The useTranslations hook has been imported but isn't being used anywhere in the component. Either remove the unused import or implement localization for the hardcoded strings in the component.
-import { useTranslations } from 'next-intl';Or implement it for the hardcoded "Ask Zero a question..." string:
+const t = useTranslations();
...
- Ask Zero a question...
+ {t('ai.askZeroQuestion')}Committable suggestion skipped: line range outside the PR's diff.
| 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.
Empty handlers object needs implementation
The ComposeHotkeys component filters for compose-scoped shortcuts but doesn't provide any handlers to be invoked when those shortcuts are triggered. The empty handlers object means that none of the filtered shortcuts will actually perform any actions.
Consider implementing the necessary handlers, especially for the "sendEmail" action which is defined in the shortcuts configuration. For example:
export function ComposeHotkeys() {
const scope = 'compose';
- const handlers = {};
+ const handlers = {
+ sendEmail: () => {
+ // Implementation for sending email
+ console.log('Send email shortcut triggered');
+ }
+ };
const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope);
useShortcuts(composeShortcuts, handlers, { scope });
return null;
}📝 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 }); | |
| return null; | |
| } | |
| export function ComposeHotkeys() { | |
| const scope = 'compose'; | |
| const handlers = { | |
| sendEmail: () => { | |
| // Implementation for sending email | |
| console.log('Send email shortcut triggered'); | |
| } | |
| }; | |
| const composeShortcuts = keyboardShortcuts.filter((shortcut) => shortcut.scope === scope); | |
| useShortcuts(composeShortcuts, handlers, { scope }); | |
| return null; | |
| } |
| const closeView = (event: KeyboardEvent) => { | ||
| event.preventDefault(); | ||
| }; |
There was a problem hiding this comment.
The closeView function is incomplete
The closeView function calls preventDefault() but doesn't actually implement any closing functionality. This appears to be incomplete since the synthetic event created in the handler (line 14) won't do anything useful by itself.
Consider implementing the actual close functionality:
const closeView = (event: KeyboardEvent) => {
event.preventDefault();
+ // Implement thread view closing logic here, e.g.:
+ // document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' }));
};📝 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(); | |
| // Implement thread view closing logic here, e.g.: | |
| // document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape' })); | |
| }; |
| search: () => { | ||
| console.log('search'); | ||
| }, |
There was a problem hiding this comment.
Implement search functionality
The search handler only logs to the console but doesn't implement actual search functionality.
Consider implementing actual search functionality:
search: () => {
- console.log('search');
+ // Implement search functionality
+ const searchInput = document.querySelector('[data-testid="search-input"]');
+ if (searchInput instanceof HTMLElement) {
+ searchInput.focus();
+ } else {
+ // Fallback if no search input is found
+ router.push('/mail/search');
+ }
},Committable suggestion skipped: line range outside the PR's diff.
| const handlers = { | ||
| goToDrafts: () => router.push('/mail/draft'), | ||
| inbox: () => router.push('/mail/inbox'), | ||
| sentMail: () => router.push('/mail/sent'), | ||
| search: () => { | ||
| console.log('search'); | ||
| }, | ||
| newEmail: () => router.push('/mail/compose'), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for navigation
The router navigation calls have no error handling, which could lead to unhandled exceptions.
Add try/catch blocks for error handling:
const handlers = {
- goToDrafts: () => router.push('/mail/draft'),
- inbox: () => router.push('/mail/inbox'),
- sentMail: () => router.push('/mail/sent'),
+ goToDrafts: () => {
+ try {
+ router.push('/mail/draft');
+ } catch (error) {
+ console.error('Failed to navigate to drafts:', error);
+ }
+ },
+ inbox: () => {
+ try {
+ router.push('/mail/inbox');
+ } catch (error) {
+ console.error('Failed to navigate to inbox:', error);
+ }
+ },
+ sentMail: () => {
+ try {
+ router.push('/mail/sent');
+ } catch (error) {
+ console.error('Failed to navigate to sent mail:', error);
+ }
+ },
search: () => {
console.log('search');
},
- newEmail: () => router.push('/mail/compose'),
+ newEmail: () => {
+ try {
+ router.push('/mail/compose');
+ } catch (error) {
+ console.error('Failed to navigate to compose:', error);
+ }
+ },
};Committable suggestion skipped: line range outside the PR's diff.
| function Shortcut({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) { | ||
| const [isRecording, setIsRecording] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Name collision with imported Shortcut type triggers no‑redeclare rule
A value‑level function Shortcut shadows the imported type Shortcut, which confuses static‑analysis tools and future readers. Rename the component to something like ShortcutRow (or re‑export the type under a different alias) to keep the namespaces distinct.
-function Shortcut({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) {
+function ShortcutRow({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) {Remember to update the JSX usage:
- <Shortcut
+ <ShortcutRow📝 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); | |
| function ShortcutRow({ children, keys, action }: { children: ReactNode; keys: string[]; action: string }) { | |
| const [isRecording, setIsRecording] = useState(false); |
🧰 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 selectRandomUsers = () => { | ||
| const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => user.isEarlyAccess); | ||
| const shuffled = [...nonEarlyAccessUsers].sort(() => 0.5 - Math.random()); | ||
| const selected = shuffled.slice(0, Math.min(5000, shuffled.length)); | ||
| setSelectedUsers(selected); |
There was a problem hiding this comment.
Incorrect filtering selects users that ALREADY have early‑access
nonEarlyAccessUsers is intended to hold non‑granted users, but the filter predicate currently keeps those with isEarlyAccess === true.
As a result, Randomize Early Access may pick only users who are already in the programme, producing an empty selection and confusing counts.
-const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => user.isEarlyAccess);
+const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => !user.isEarlyAccess);📝 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 selectRandomUsers = () => { | |
| const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => user.isEarlyAccess); | |
| const shuffled = [...nonEarlyAccessUsers].sort(() => 0.5 - Math.random()); | |
| const selected = shuffled.slice(0, Math.min(5000, shuffled.length)); | |
| setSelectedUsers(selected); | |
| const selectRandomUsers = () => { | |
| - const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => user.isEarlyAccess); | |
| + const nonEarlyAccessUsers = earlyAccessUsers.filter((user) => !user.isEarlyAccess); | |
| const shuffled = [...nonEarlyAccessUsers].sort(() => 0.5 - Math.random()); | |
| const selected = shuffled.slice(0, Math.min(5000, shuffled.length)); | |
| setSelectedUsers(selected); |
| return ( | ||
| <> | ||
| <Dialog open={isDialogOpen} onOpenChange={setIsDialogOpen}> | ||
| <DialogContent className="sm:max-w-md"> | ||
| <DialogHeader> | ||
| <DialogTitle>Confirm Early Access Selection</DialogTitle> | ||
| <DialogDescription> | ||
| You are about to grant early access to {selectedUsers.length} users. Please review the | ||
| list below. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <div className="max-h-[400px] overflow-auto rounded-md border p-2"> | ||
| {selectedUsers.map((user) => ( | ||
| <div key={user.id} className="border-b py-2 last:border-0"> | ||
| <p className="text-sm font-medium">{user.email}</p> | ||
| <p className="text-muted-foreground text-xs"> | ||
| Signed up: {format(new Date(user.createdAt), 'MMM d, yyyy')} | ||
| </p> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| <DialogFooter className="flex justify-between sm:justify-between"> | ||
| <Button variant="outline" onClick={() => setIsDialogOpen(false)}> | ||
| Cancel | ||
| </Button> | ||
| <Button onClick={handleConfirm} disabled={isUpdating}> | ||
| Send Early Access Emails | ||
| </Button> | ||
| </DialogFooter> | ||
| </DialogContent> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate dialog implementation – dead code & maintainability
A fully‑featured ConfirmationDialog component is declared at the top of the file but never reused; instead, the very same markup is duplicated here.
Keeping both copies in sync will be error‑prone.
Refactor to reuse ConfirmationDialog:
-<Dialog open={isDialogOpen} onOpenChange={setIsDialogOpen}>
- …duplicated markup…
-</Dialog>
+<ConfirmationDialog
+ isOpen={isDialogOpen}
+ onClose={() => setIsDialogOpen(false)}
+ selectedUsers={selectedUsers}
+ onConfirm={handleConfirm}
/>Committable suggestion skipped: line range outside the PR's diff.
| shortcuts.forEach((shortcut) => { | ||
| const handler = handlers[shortcut.action]; | ||
| if (handler) { | ||
| useShortcut(shortcut, handler, options); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Hooks are called inside a loop – violates the Rules of Hooks
useShortcut is invoked inside shortcuts.forEach.
If the shortcuts array length or ordering changes between renders, React’s hook call order will differ, leading to runtime errors.
Refactor to compute a stable list once (e.g., in useMemo) and map deterministically, or ask consumers to wrap each call explicitly.
Example fix:
export function useShortcuts(
shortcuts: Shortcut[],
handlers: Record<string, () => void>,
options: Partial<HotkeyOptions> = {},
) {
shortcuts
.map((sc) => [sc, handlers[sc.action]] as const)
.filter(([, h]) => h)
.forEach(([sc, h]) => useShortcut(sc, h!, options));
}Even better: enforce shortcuts to be static via useRef or document that it must not change.
| if (keys.length > 1) { | ||
| return keys.map(mapKey).join('+'); | ||
| } | ||
|
|
||
| const firstKey = keys[0]; | ||
| if (!firstKey) return ''; | ||
| return mapKey(firstKey); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
formatKeys joins multi‑key shortcuts with “+” – may mis‑interpret sequences
react-hotkeys-hook distinguishes between chords (ctrl+a) and sequences (g d).
Blindly converting every multi‑key array to 'key1+key2' disallows sequences such as "g d" used in Gmail‑style shortcuts.
Consider inspecting shortcut.type (already present in Shortcut) and:
return shortcut.type === 'combination'
? keys.map(mapKey).join('+') // chord
: keys.map(mapKey).join(' ') // sequenceThis preserves the original design in keyboardShortcuts.
6 participants
NEVER MERGE
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores