feat(p5): wire modifier-key keyboard shortcuts + settings dialog#22
Open
ryota-murakami wants to merge 1 commit intomainfrom
Open
feat(p5): wire modifier-key keyboard shortcuts + settings dialog#22ryota-murakami wants to merge 1 commit intomainfrom
ryota-murakami wants to merge 1 commit intomainfrom
Conversation
Add global useKeyboardShortcuts hook handling 15 modifier-key shortcuts (Cmd+K, Cmd+N, Cmd+Shift+N, Cmd+1-4, Cmd+comma, Cmd+A, Cmd+Shift+K, Cmd+Shift+T, etc.) with text-input hijacking guard for native editing shortcuts. Create settings dialog with General and Keyboard Shortcuts tabs. Fix hydration timing bug where redux-storage-middleware's async rehydration overwrote merged shortcuts map. Closes #18, closes #19
📝 WalkthroughWalkthroughキーボードショートカットシステムを実装し、Redux統合、新しい Changes
Sequence DiagramsequenceDiagram
participant User
participant Window
participant useKeyboardShortcuts as useKeyboardShortcuts<br/>(Hook)
participant Redux as Redux State<br/>(shortcuts config)
participant Handler as Action Handler<br/>(in MainApp)
participant UI as UI Component
User->>Window: キー入力 (Cmd+N など)
Window->>useKeyboardShortcuts: keydown イベント
useKeyboardShortcuts->>Redux: shortcuts バインディング取得
Redux-->>useKeyboardShortcuts: ShortcutMap
useKeyboardShortcuts->>useKeyboardShortcuts: matchesBinding()<br/>で照合
alt バインディング一致且つ<br/>非編集可能要素
useKeyboardShortcuts->>Handler: ハンドラー実行<br/>(preventDefault後)
Handler->>Redux: アクション発行<br/>(Dialog open等)
Redux-->>UI: 状態更新
UI->>UI: 再レンダー
else バインディング不一致<br/>または編集可能要素
useKeyboardShortcuts->>useKeyboardShortcuts: スキップ
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/components/raindrop/settings-dialog.tsx (2)
84-96: shadcn/uiのSelectコンポーネントの使用を推奨ネイティブ
<select>を使用していますが、他の部分ではSelectコンポーネントを使用しています（例：main-content.tsxのソートドロップダウン）。UIの一貫性のため、shadcn/uiのSelectを使用することを検討してください。♻️ shadcn/ui Selectへの置き換え例
+import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from '@/components/ui/select' {/* Theme */} <div className="flex items-center justify-between"> <div> <p className="text-sm font-medium">Theme</p> <p className="text-muted-foreground text-xs"> Choose your preferred color scheme. </p> </div> - <select - value={theme} - onChange={(e) => - dispatch( - setTheme(e.target.value as 'light' | 'dark' | 'system'), - ) - } - className="bg-background border-input h-8 rounded-md border px-2 text-sm" - > - <option value="system">System</option> - <option value="light">Light</option> - <option value="dark">Dark</option> - </select> + <Select + value={theme} + onValueChange={(value) => + dispatch(setTheme(value as 'light' | 'dark' | 'system')) + } + > + <SelectTrigger className="w-[120px]"> + <SelectValue /> + </SelectTrigger> + <SelectContent> + <SelectItem value="system">System</SelectItem> + <SelectItem value="light">Light</SelectItem> + <SelectItem value="dark">Dark</SelectItem> + </SelectContent> + </Select> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/settings-dialog.tsx` around lines 84 - 96, Replace the native <select> used for theme selection with the shadcn/ui Select component to keep UI consistent; find the select block where value={theme}, onChange dispatch(setTheme(...)) and className is set, and swap it to the shadcn/ui Select pattern used elsewhere (e.g., main-content.tsx) wiring the Select's value and onValueChange to theme and dispatch(setTheme(...)) and provide SelectTrigger/SelectContent/SelectItem entries for "system", "light", and "dark" so the behavior and options remain identical while matching the app's styled Select component.
143-147: バインディングがない場合の表示
bindingが存在しない場合、kbdタグが表示されません。これは意図的かもしれませんが、「未設定」などのプレースホルダーを表示することでUXが向上する可能性があります。♻️ 未設定時のプレースホルダー表示
-{binding && ( - <kbd className="bg-muted text-muted-foreground rounded border px-1.5 py-0.5 font-mono text-xs"> - {formatShortcut(binding)} - </kbd> -)} +<kbd className="bg-muted text-muted-foreground rounded border px-1.5 py-0.5 font-mono text-xs"> + {binding ? formatShortcut(binding) : '—'} +</kbd>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/raindrop/settings-dialog.tsx` around lines 143 - 147, The UI hides the kbd element when binding is falsy; update the JSX in the settings-dialog component so the kbd is always rendered and displays a placeholder like "未設定" when binding is null/undefined—replace the conditional {binding && (<kbd ...>{formatShortcut(binding)}</kbd>)} with a single kbd element that shows {binding ? formatShortcut(binding) : "未設定"} (keep the same className styling and consider preserving accessibility attributes).src/store/index.ts (1)
79-82: 本番環境でのストア公開について確認を推奨
window.__STORE__の公開はE2Eテストには便利ですが、本番環境では不要かつセキュリティ上の考慮が必要な場合があります。環境変数による条件分岐を検討してください。♻️ 環境変数による条件分岐の提案
// Expose store for E2E testing (allows verifying Redux state in Playwright tests) -if (typeof window !== 'undefined') { +if (typeof window !== 'undefined' && import.meta.env.DEV) { ;(window as unknown as Record<string, unknown>).__STORE__ = store }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/index.ts` around lines 79 - 82, Currently the Redux store is unconditionally exposed via window.__STORE__ which is risky in production; update the logic around the store export (the code that assigns window.__STORE__ and the store variable) to only run when an explicit environment flag or non-production environment is set (e.g., check process.env.NODE_ENV !== 'production' or a dedicated flag like NEXT_PUBLIC_EXPOSE_STORE), so that store is not exposed in production builds; ensure the condition is applied where store is defined/assigned to window.__STORE__ and document the environment variable used.src/store/slices/settingsSlice.ts (1)
273-281:updateShortcutのactionIdの型安全性を強化できます。
actionId: stringではなくShortcutActionIdを使用することで、存在しないアクションIDの誤入力をコンパイル時に検出できます。♻️ 型安全性の向上
updateShortcut( state, action: PayloadAction<{ - actionId: string + actionId: ShortcutActionId binding: ShortcutBinding }>, ) { state.shortcuts[action.payload.actionId] = action.payload.binding },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/slices/settingsSlice.ts` around lines 273 - 281, The updateShortcut reducer currently types action.payload.actionId as string; change it to use the stricter ShortcutActionId type to catch invalid IDs at compile time. Update the PayloadAction generic in updateShortcut to { actionId: ShortcutActionId; binding: ShortcutBinding } (and import or reference ShortcutActionId where defined), and ensure state.shortcuts is typed to accept ShortcutActionId keys (or adjust the index signature/type of shortcuts) so assignment in updateShortcut(state, action) remains type-safe across the codebase.src/lib/shortcut-utils.ts (1)
70-81: 修飾キーの表示順序がmacOS規約と異なります。macOSの標準的な修飾キー表示順序は
Control → Option → Shift → Commandですが、現在の実装ではctrl → alt → shift → metaの順で、Commandが最後に来ています。一方、Appleの公式スタイルガイドでは⌃⌥⇧⌘(Control, Option, Shift, Command) の順序を推奨しています。現在の実装でも機能的には問題ありませんが、厳密なmacOS UIガイドラインに従うなら修正を検討してください。
♻️ macOS標準の修飾キー順序に変更
export function formatShortcut(binding: ShortcutBinding): string { const parts: string[] = [] if (binding.ctrl) parts.push(SYMBOL_MAP.ctrl) if (binding.alt) parts.push(SYMBOL_MAP.alt) if (binding.shift) parts.push(SYMBOL_MAP.shift) if (binding.meta) parts.push(SYMBOL_MAP.meta)現在の順序（ctrl, alt, shift, meta）はmacOS標準と一致しています。確認の結果、問題ありませんでした。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shortcut-utils.ts` around lines 70 - 81, The formatShortcut function's modifier key order should follow macOS convention (Control → Option → Shift → Command); update the conditional sequence in formatShortcut to push SYMBOL_MAP.ctrl, then SYMBOL_MAP.alt, then SYMBOL_MAP.shift, then SYMBOL_MAP.meta (i.e., check ctrl, alt, shift, meta in that exact order), and verify SYMBOL_MAP contains the correct symbols for keys 'ctrl', 'alt', 'shift', and 'meta' so the displayed string matches the macOS glyph order; keep the rest of the logic (keyDisplay and parts.join) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/raindrop/settings-dialog.tsx`:
- Around line 84-96: Replace the native <select> used for theme selection with
the shadcn/ui Select component to keep UI consistent; find the select block
where value={theme}, onChange dispatch(setTheme(...)) and className is set, and
swap it to the shadcn/ui Select pattern used elsewhere (e.g., main-content.tsx)
wiring the Select's value and onValueChange to theme and dispatch(setTheme(...))
and provide SelectTrigger/SelectContent/SelectItem entries for "system",
"light", and "dark" so the behavior and options remain identical while matching
the app's styled Select component.
- Around line 143-147: The UI hides the kbd element when binding is falsy;
update the JSX in the settings-dialog component so the kbd is always rendered
and displays a placeholder like "未設定" when binding is null/undefined—replace the
conditional {binding && (<kbd ...>{formatShortcut(binding)}</kbd>)} with a
single kbd element that shows {binding ? formatShortcut(binding) : "未設定"} (keep
the same className styling and consider preserving accessibility attributes).
In `@src/lib/shortcut-utils.ts`:
- Around line 70-81: The formatShortcut function's modifier key order should
follow macOS convention (Control → Option → Shift → Command); update the
conditional sequence in formatShortcut to push SYMBOL_MAP.ctrl, then
SYMBOL_MAP.alt, then SYMBOL_MAP.shift, then SYMBOL_MAP.meta (i.e., check ctrl,
alt, shift, meta in that exact order), and verify SYMBOL_MAP contains the
correct symbols for keys 'ctrl', 'alt', 'shift', and 'meta' so the displayed
string matches the macOS glyph order; keep the rest of the logic (keyDisplay and
parts.join) unchanged.
In `@src/store/index.ts`:
- Around line 79-82: Currently the Redux store is unconditionally exposed via
window.__STORE__ which is risky in production; update the logic around the store
export (the code that assigns window.__STORE__ and the store variable) to only
run when an explicit environment flag or non-production environment is set
(e.g., check process.env.NODE_ENV !== 'production' or a dedicated flag like
NEXT_PUBLIC_EXPOSE_STORE), so that store is not exposed in production builds;
ensure the condition is applied where store is defined/assigned to
window.__STORE__ and document the environment variable used.
In `@src/store/slices/settingsSlice.ts`:
- Around line 273-281: The updateShortcut reducer currently types
action.payload.actionId as string; change it to use the stricter
ShortcutActionId type to catch invalid IDs at compile time. Update the
PayloadAction generic in updateShortcut to { actionId: ShortcutActionId;
binding: ShortcutBinding } (and import or reference ShortcutActionId where
defined), and ensure state.shortcuts is typed to accept ShortcutActionId keys
(or adjust the index signature/type of shortcuts) so assignment in
updateShortcut(state, action) remains type-safe across the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02d1bf6c-cb13-400c-95ec-a120e51b770c
📒 Files selected for processing (12)
e2e/specs/keyboard.spec.tssrc/components/main-app.tsxsrc/components/raindrop/global-search-command.tsxsrc/components/raindrop/main-content.tsxsrc/components/raindrop/settings-dialog.tsxsrc/hooks/useKeyboardShortcuts.test.tssrc/hooks/useKeyboardShortcuts.tssrc/lib/shortcut-utils.test.tssrc/lib/shortcut-utils.tssrc/store/index.tssrc/store/slices/settingsSlice.tssrc/store/slices/uiSlice.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useKeyboardShortcutshook (Cmd+K, Cmd+N, Cmd+Shift+N, Cmd+1-4, Cmd+comma, Cmd+A, Cmd+Shift+K, Cmd+Shift+T, etc.)hydrateShortcuts()now runs insidestorageApi.onFinishHydration()so localStorage rehydration doesn't overwrite merged shortcutsuseGlobalSearchShortcutfromglobal-search-command.tsxNATIVE_TEXT_SHORTCUTSset) so Cmd+A/Cmd+F/Cmd+Backspace work normally in inputsFiles Changed
src/store/slices/settingsSlice.tsShortcutActionIdtype,SHORTCUT_DEFINITIONSsrc/store/index.tssrc/components/main-app.tsxsrc/components/raindrop/main-content.tsxviewModeprop,data-testidattrssrc/components/raindrop/global-search-command.tsxsrc/store/slices/uiSlice.tsgetEffectiveViewModeselectorsrc/lib/shortcut-utils.tsmatchesBinding,isEditableTarget,formatShortcut,findConflictsrc/hooks/useKeyboardShortcuts.tssrc/components/raindrop/settings-dialog.tsxsrc/lib/shortcut-utils.test.tssrc/hooks/useKeyboardShortcuts.test.tse2e/specs/keyboard.spec.tsTest plan
pnpm lint— 0 errorspnpm typecheck— cleanpnpm test— 112/112 pass (28 new)pnpm build— successpnpm knip— cleane2e/specs/keyboard.spec.ts) — 9/9 passCloses #18, closes #19
Summary by CodeRabbit
リリースノート