General settings improvements #1587
Conversation
📝 WalkthroughWalkthroughThis PR refactors desktop UI components and consolidates keyboard shortcuts. Changes include redesigning the settings general panel with inline language and vocabulary inputs, reorganizing the settings layout navigation, centralizing hotkey handlers for tabs, updating search focus management via context, integrating settings shortcuts, improving editor keydown handling with global shortcut whitelisting, and enhancing various UI components with keyboard badges and progress indicators. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant ShellContext as Shell<br/>Context
participant SearchContext as Search<br/>Context
participant TipTap as TipTap<br/>Editor
participant Window as Window/Tab<br/>Manager
Note over User,Window: Global Keyboard Shortcuts Flow
User->>App: Press mod+k
App->>SearchContext: useHotkeys triggers
SearchContext->>SearchContext: inputRef.current?.focus()
User->>App: Press mod+,
App->>ShellContext: useSettings hotkey fires
ShellContext->>Window: windows.show('settings')
Window->>App: Settings window opens
User->>App: Press mod+t (in editor)
App->>TipTap: handleKeyDown checks whitelist
TipTap->>App: Bypasses editor, allows global
App->>Window: Shortcut routes to tab manager
User->>App: Press mod+n (in editor)
App->>TipTap: handleKeyDown matches whitelist
TipTap->>Window: Lets global handler route
Window->>App: Close current + open new tab
Note over User,Window: Consolidated Control Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR spans 10+ files with mixed architectural and UI changes, but follows consistent refactoring patterns. Keyboard shortcut consolidation is well-contained across predictable locations. Context provider reordering and search focus migration are straightforward. Public API changes (SearchUIContextValue, useSettings export) are minimal and backward-compatible in intent. The variety of changes demands separate reasoning per cohort but individual changes are not densely complex. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apps/desktop/src/routes/app/settings/_layout.tsx (1)
98-110: Add consistent icon–label spacing and current-state hook on nav itemsImproves readability and a11y signaling.
<Button key={tab} variant="ghost" className={cn([ - "shadow-none w-full justify-start hover:bg-neutral-200", + "shadow-none w-full justify-start gap-2 hover:bg-neutral-200", activeTab === tab && "bg-neutral-200", ])} + aria-current={activeTab === tab ? "page" : undefined} onClick={() => navigate({ search: { tab } })} >apps/desktop/src/components/settings/general.tsx (8)
232-236: Use cn([...]) array style and group classes per project guidelineAligns with apps/desktop cn usage and aids readability.
- className={cn( - "flex flex-wrap items-center w-full px-2 py-1.5 gap-1.5 rounded-lg bg-white border border-neutral-200 focus-within:border-neutral-300 min-h-[38px]", - languageInputFocused && "border-neutral-300", - )} + className={cn([ + // layout + "flex flex-wrap items-center w-full min-h-[38px] px-2 py-1.5 gap-1.5", + // visuals + "rounded-lg bg-white border border-neutral-200 focus-within:border-neutral-300", + // state + languageInputFocused && "border-neutral-300", + ])}As per coding guidelines
295-298: cn([...]) array style inside options for consistencyKeep cn usage consistent here too.
- className={cn( - "flex items-center justify-between px-3 py-2 text-sm text-left transition-colors w-full", - languageSelectedIndex === index ? "bg-neutral-200" : "hover:bg-neutral-100", - )} + className={cn([ + "flex items-center justify-between px-3 py-2 text-sm text-left transition-colors w-full", + languageSelectedIndex === index ? "bg-neutral-200" : "hover:bg-neutral-100", + ])}As per coding guidelines
245-257: Add aria-label to remove-language buttonImproves screen reader clarity.
<Button type="button" variant="ghost" size="sm" className="h-3 w-3 p-0 hover:bg-transparent ml-0.5" + aria-label={`Remove ${lang}`} onClick={(e) => { e.stopPropagation(); const updated = (value.spoken_languages ?? []).filter(l => l !== lang); handle.setField("spoken_languages", updated); }} >
321-324: Use cn([...]) array style and group classes for vocabulary inputAligns with guideline and mirrors the spoken-language input container.
- className={cn( - "flex flex-wrap items-center w-full px-2 py-1.5 gap-1.5 rounded-lg bg-white border border-neutral-200 focus-within:border-neutral-300 min-h-[38px]", - vocabInputFocused && "border-neutral-300", - )} + className={cn([ + "flex flex-wrap items-center w-full min-h-[38px] px-2 py-1.5 gap-1.5", + "rounded-lg bg-white border border-neutral-200 focus-within:border-neutral-300", + vocabInputFocused && "border-neutral-300", + ])}As per coding guidelines
334-347: Add aria-label to remove-vocabulary buttonAccessibility improvement.
<Button type="button" variant="ghost" size="sm" className="h-3 w-3 p-0 hover:bg-transparent ml-0.5" + aria-label={`Remove ${vocab}`} onClick={(e) => { e.stopPropagation(); const updated = (value.jargons ?? []).filter(j => j !== vocab); handle.setField("jargons", updated); }} >
214-216: Preserve visible focus styles on SelectTriggerAvoid removing focus ring to maintain keyboard visibility.
- <SelectTrigger className="w-40 shadow-none focus:ring-0 focus:ring-offset-0"> + <SelectTrigger className="w-40 shadow-none">
236-236: Prefer refs over document.getElementById for focusing inputsSimplifies and avoids id coupling.
I can provide a small refactor using useRef() for both inputs if helpful.
96-129: Optional: use named React event types to avoid React namespaceIf you prefer to remove the React namespace entirely, import types and update signatures.
+import type { KeyboardEvent, ChangeEvent } from "react"; -const handleLanguageKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { +const handleLanguageKeyDown = (e: KeyboardEvent<HTMLInputElement>) => { -const handleVocabChange = (e: React.ChangeEvent<HTMLInputElement>) => { +const handleVocabChange = (e: ChangeEvent<HTMLInputElement>) => { -const handleVocabKeyDown = (e: React.KeyboardEvent<HTMLInputElement>) => { +const handleVocabKeyDown = (e: KeyboardEvent<HTMLInputElement>) => {Also applies to: 148-168, 131-147
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/settings/general.tsx(7 hunks)apps/desktop/src/routes/app/settings/_layout.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/routes/app/settings/_layout.tsxapps/desktop/src/components/settings/general.tsx
🧬 Code graph analysis (2)
apps/desktop/src/routes/app/settings/_layout.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/general.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
ComputelessComputer
force-pushed
the
jj-branch-24
branch
from
81c8a6c to
8c8c1bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/components/settings/general.tsx (2)
3-3: Import React types to resolveReact.KeyboardEvent/React.ChangeEventusageWithout importing the React namespace or types, TypeScript will error on lines 96, 131, and 148 where
React.KeyboardEventandReact.ChangeEventare used.Apply this diff:
-import { useMemo, useState } from "react"; +import type React from "react"; +import { useMemo, useState } from "react";
390-390: Fixcn()usage to pass array argumentsPer coding guidelines, all
cn()calls must pass Tailwind classNames as an array. Lines 390, 392, and 415 pass strings directly.As per coding guidelines.
Apply these diffs:
- <div className={cn(!hasMicrophoneAccess && "flex items-center gap-2 mb-1")}> + <div className={cn([!hasMicrophoneAccess && "flex items-center gap-2 mb-1"])}>- <h3 className={cn("text-sm font-medium mb-1", !hasMicrophoneAccess && "text-red-500")}> + <h3 className={cn(["text-sm font-medium mb-1", !hasMicrophoneAccess && "text-red-500"])}>- <div className={cn(!hasSystemAudioAccess && "flex items-center gap-2")}> + <div className={cn([!hasSystemAudioAccess && "flex items-center gap-2"])}>Also applies to: 392-392, 415-415
🧹 Nitpick comments (1)
apps/desktop/src/components/settings/general.tsx (1)
69-80: Replace mock permission handlers with real system API callsThe mock permission states and toggle handlers are placeholder code. Real implementation should request microphone and system audio access via Electron or Tauri APIs.
Do you want me to generate the real permission request implementation or open a new issue to track this task?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/components/settings/general.tsx(7 hunks)apps/desktop/src/routes/app/settings/_layout.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/routes/app/settings/_layout.tsxapps/desktop/src/components/settings/general.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/settings/general.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/settings/general.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/settings/general.tsx
🧬 Code graph analysis (2)
apps/desktop/src/routes/app/settings/_layout.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/general.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: zizmor
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (6)
apps/desktop/src/routes/app/settings/_layout.tsx (2)
47-47: Past review issue resolved: aside width is now correct.The previous invalid
max-w-52class has been properly replaced withw-52(13rem/208px), andflex-1has been removed. The aside now maintains a fixed width as intended.
98-109: Verify that overriding the ghost variant styles is intentional.The Button uses
variant="ghost"but then overrides its default hover behavior with custom classes (hover:bg-neutral-200andshadow-none). While this may be intentional for a consistent neutral design, it could conflict with the Button component's design system.Please confirm:
- Is the
shadow-noneclass necessary, or does the ghost variant already have no shadow?- Is overriding the ghost variant's hover style with
hover:bg-neutral-200the intended design?If this custom styling is intentional and differs from the design system, consider documenting why these overrides are needed or creating a dedicated button variant for navigation items.
apps/desktop/src/components/settings/general.tsx (4)
82-129: LGTM!The filtered language logic and keyboard navigation are well-implemented. The handler correctly manages Backspace to remove the last language, Arrow keys for dropdown navigation, Enter to add, and Escape to close.
131-168: LGTM!The vocabulary input handlers correctly parse comma-separated terms, filter duplicates, and support intuitive keyboard shortcuts (Backspace to remove last, Enter to add, Escape to close).
265-327: Excellent accessibility improvements!The combobox now has proper ARIA attributes (
role="combobox",aria-haspopup,aria-expanded,aria-controls,aria-activedescendant), the dropdown hasrole="listbox", and options haverole="option"witharia-selected. TheonMouseDownpreventDefault pattern correctly prevents blur on option click.These changes fully address the previous review feedback on accessibility.
330-381: LGTM!The custom vocabulary input has a clean token-based UI with proper ARIA labeling. The comma-separated input pattern is intuitive and consistent with the language input styling.
ComputelessComputer
force-pushed
the
jj-branch-24
branch
from
8c8c1bc to
8b70bab
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/desktop/src/components/settings/general.tsx (1)
3-3: Import React types to resolve React.KeyboardEvent/ChangeEvent usage.The file uses
React.KeyboardEvent(line 96) andReact.ChangeEvent(line 131) without importing the React namespace, causing TypeScript errors.Apply this diff:
-import { useMemo, useState } from "react"; +import type React from "react"; +import { useMemo, useState } from "react";
🧹 Nitpick comments (6)
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
120-130: Consider making the keyboard symbol in the badge platform-aware for better UX.The keyboard shortcut handler is properly implemented in
apps/desktop/src/contexts/shell/settings.ts(lines 11-15) and correctly handles bothmetaKey(Cmd on macOS) andctrlKey(Ctrl on Windows/Linux). However, the badge displays a hardcoded⌘symbol which will appear incorrect to Windows/Linux users who trigger the shortcut with Ctrl+,.Consider updating the badge to show the appropriate symbol for each platform:
const isMac = navigator.platform.toUpperCase().indexOf('MAC') >= 0; const modifierKey = isMac ? '⌘' : 'Ctrl'; badge: ( <KbdGroup> <Kbd className="bg-neutral-200">{modifierKey}</Kbd> <Kbd className="bg-neutral-200">,</Kbd> </KbdGroup> )apps/desktop/src/contexts/shell/settings.ts (1)
6-8: Harden shortcut handling and catch plugin errors
- Don’t trigger while typing; guard inputs/contentEditable and event.repeat.
- Normalize detection to event.code (fallback to key).
- Catch windowShow promise errors to avoid unhandled rejections.
Apply:
import { useCallback, useEffect } from "react"; export function useSettings() { const openSettings = useCallback(() => { - windowsCommands.windowShow({ type: "settings" }); + void windowsCommands + .windowShow({ type: "settings" }) + .catch((e) => console.error("openSettings failed:", e)); }, []); useEffect(() => { - const handleKeyDown = (event: KeyboardEvent) => { - if ((event.metaKey || event.ctrlKey) && event.key === ",") { - event.preventDefault(); - openSettings(); - } - }; + const isEditableTarget = (el: EventTarget | null) => { + if (!(el instanceof Element)) return false; + const tag = el.tagName.toLowerCase(); + return el.getAttribute("contenteditable") === "true" || ["input", "textarea", "select"].includes(tag); + }; + const handleKeyDown = (event: KeyboardEvent) => { + if (event.defaultPrevented || event.repeat) return; + if (isEditableTarget(event.target)) return; + const isMod = event.metaKey || event.ctrlKey; + if (isMod && (event.code === "Comma" || event.key === ",")) { + event.preventDefault(); + openSettings(); + } + }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); }, [openSettings]);Also applies to: 10-20
apps/desktop/src/components/main/body/search.tsx (1)
14-18: Unregister focus callback on unmountHave registerFocusCallback return an unsubscribe to prevent stale refs if this component unmounts/remounts.
After updating the API (see contexts/search/ui.tsx comment), adjust here:
- useEffect(() => { - registerFocusCallback(() => { - inputRef.current?.focus(); - }); - }, [registerFocusCallback]); + useEffect(() => { + const unregister = registerFocusCallback(() => { + inputRef.current?.focus(); + }); + return () => unregister?.(); + }, [registerFocusCallback]);Also: cn usage with arrays looks good.
apps/desktop/src/contexts/shell/search.ts (1)
8-18: Avoid firing Cmd/Ctrl+K while typing; normalize key detectionGuard editable targets and repeats; prefer event.code to handle layout/Shift differences.
export function useSearchShortcut({ onFocusSearch }: UseSearchShortcutProps) { useEffect(() => { - const handleKeyDown = (event: KeyboardEvent) => { - if ((event.metaKey || event.ctrlKey) && event.key === "k") { - event.preventDefault(); - onFocusSearch(); - } - }; + const isEditableTarget = (el: EventTarget | null) => { + if (!(el instanceof Element)) return false; + const tag = el.tagName.toLowerCase(); + return el.getAttribute("contenteditable") === "true" || ["input", "textarea", "select"].includes(tag); + }; + const handleKeyDown = (event: KeyboardEvent) => { + if (event.defaultPrevented || event.repeat) return; + if (isEditableTarget(event.target)) return; + const isMod = event.metaKey || event.ctrlKey; + if (isMod && (event.code === "KeyK" || event.key.toLowerCase() === "k")) { + event.preventDefault(); + onFocusSearch(); + } + };Consider extracting isEditableTarget to a shared util to DRY across listeners.
apps/desktop/src/contexts/shell/index.tsx (1)
25-36: Memoize ShellContext value to reduce re-rendersWrap the provider value in useMemo to stabilize references.
-import { createContext, useContext } from "react"; +import { createContext, useContext, useMemo } from "react"; @@ - return ( - <ShellContext.Provider value={{ chat, leftsidebar, search, settings, tabs }}> - {children} - </ShellContext.Provider> - ); + const value = useMemo(() => ({ chat, leftsidebar, search, settings, tabs }), [ + chat, + leftsidebar, + search, + settings, + tabs, + ]); + return <ShellContext.Provider value={value}>{children}</ShellContext.Provider>;apps/desktop/src/contexts/search/ui.tsx (1)
30-43: Return an unsubscribe from registerFocusCallback to avoid stale callbacksProvide a cleanup path and update the type.
interface SearchUIContextValue { @@ - registerFocusCallback: (callback: () => void) => void; + registerFocusCallback: (callback: () => void) => () => void; focusInput: () => void; } @@ - const registerFocusCallback = useCallback((callback: () => void) => { - focusCallbackRef.current = callback; - }, []); + const registerFocusCallback = useCallback((callback: () => void) => { + focusCallbackRef.current = callback; + return () => { + if (focusCallbackRef.current === callback) { + focusCallbackRef.current = null; + } + }; + }, []); @@ const value = useMemo( () => ({ @@ onBlur, registerFocusCallback, focusInput, }), - [query, filters, results, isSearching, isFocused, isIndexing, onFocus, onBlur, registerFocusCallback, focusInput], + [query, filters, results, isSearching, isFocused, isIndexing, onFocus, onBlur, registerFocusCallback, focusInput], );Optionally also clear on provider unmount:
useEffect(() => () => { focusCallbackRef.current = null; }, []);Update callers (e.g., search.tsx) to use the returned unsubscribe. Based on learnings.
Also applies to: 153-153, 201-208, 228-232
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/components/main/body/index.tsx(0 hunks)apps/desktop/src/components/main/body/search.tsx(1 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(2 hunks)apps/desktop/src/components/settings/general.tsx(7 hunks)apps/desktop/src/contexts/search/ui.tsx(5 hunks)apps/desktop/src/contexts/shell/index.tsx(1 hunks)apps/desktop/src/contexts/shell/search.ts(1 hunks)apps/desktop/src/contexts/shell/settings.ts(1 hunks)apps/desktop/src/contexts/shell/tabs.ts(1 hunks)apps/desktop/src/routes/app/main/_layout.tsx(3 hunks)packages/tiptap/src/editor/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/components/main/body/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/contexts/shell/index.tsxapps/desktop/src/routes/app/main/_layout.tsxapps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/components/settings/general.tsxapps/desktop/src/contexts/search/ui.tsxapps/desktop/src/components/main/body/search.tsx
🧬 Code graph analysis (5)
apps/desktop/src/contexts/shell/index.tsx (3)
apps/desktop/src/contexts/shell/search.ts (1)
useSearchShortcut(7-21)apps/desktop/src/contexts/shell/settings.ts (1)
useSettings(5-23)apps/desktop/src/contexts/shell/tabs.ts (1)
useTabsShortcuts(6-48)
apps/desktop/src/routes/app/main/_layout.tsx (2)
apps/desktop/src/contexts/search/ui.tsx (2)
SearchUIProvider(144-235)useSearch(237-243)apps/desktop/src/contexts/shell/index.tsx (1)
ShellProvider(25-37)
apps/desktop/src/contexts/shell/tabs.ts (1)
plugins/windows/src/ext.rs (1)
close(56-62)
apps/desktop/src/components/settings/general.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/main/body/search.tsx (1)
apps/desktop/src/contexts/search/ui.tsx (1)
useSearch(237-243)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (8)
packages/tiptap/src/editor/index.tsx (2)
70-77: Backspace prevention at document start looks correct.The implementation properly prevents backspace when the cursor is at the start of the document with no selection. The logic correctly uses ProseMirror's selection API to check both position and selection state.
Please confirm this is the intended user experience. Users might expect to be able to press backspace at the document start (even if it has no effect), and some might find preventDefault disorienting if they habitually press backspace.
79-82: Verify that completely disabling Tab is intentional.This unconditionally prevents the Tab key from functioning in the editor. While this might be intentional for accessibility (preserving Tab for keyboard navigation) or application-specific UX requirements, users typically expect Tab to work in text editors for indentation.
Please confirm:
- Is Tab intentionally disabled to keep focus navigation working?
- Should Tab insert indentation/spaces when the editor is focused?
- Are there alternative keyboard shortcuts for indentation if needed?
If Tab should remain disabled for navigation, consider adding a comment explaining the rationale.
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
2-2: LGTM!The import is correctly used in the Settings menu item badge below.
apps/desktop/src/components/settings/general.tsx (4)
96-129: LGTM! Keyboard navigation and ARIA semantics correctly implemented.The language picker keyboard handler and ARIA attributes align with accessibility best practices. The combobox pattern is properly implemented with
aria-haspopup,aria-expanded,aria-controls, andaria-activedescendant.Also applies to: 265-287
289-326: LGTM! Listbox semantics and click handling correctly implemented.The dropdown properly implements
role="listbox"androle="option"witharia-selected. TheonMouseDownpreventDefault pattern correctly prevents input blur when selecting options.
131-168: LGTM! Vocabulary input handles comma-separated terms correctly.The vocabulary input logic properly splits comma-separated terms, deduplicates against existing jargons, and supports keyboard shortcuts (Backspace to remove last, Enter to add current term).
Also applies to: 330-381
388-403: LGTM! PermissionRow extraction eliminates duplication.The
PermissionRowcomponent successfully consolidates the microphone and system audio permission sections, improving maintainability as suggested in previous reviews.Also applies to: 408-445
apps/desktop/src/routes/app/main/_layout.tsx (1)
52-80: New-tab flow and provider wiring look solidSession creation + optional close current before opening new is clear; SearchShortcutBridge cleanly bridges focusInput to ShellProvider.
Also applies to: 82-91
ComputelessComputer
force-pushed
the
jj-branch-24
branch
from
8b70bab to
758e32d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/tiptap/src/editor/index.tsx (1)
64-68: The previous review comment regarding centralization of global shortcuts still applies.The hardcoded
allowedGlobalShortcutsarray remains unchanged and should be extracted to a shared constant as suggested in the previous review.apps/desktop/src/contexts/shell/tabs.ts (1)
9-45: Unresolved: Keyboard handler still hijacks shortcuts in editable elements.The past review comment about this code remains valid. The handler currently:
- Intercepts Cmd/Ctrl shortcuts even when typing in inputs/textareas/contentEditable elements
- Lacks a repeat guard (
event.repeat)- Doesn't handle potential errors from
await appWindow.close()These issues should be addressed to prevent disrupting user input in forms and editors throughout the app.
apps/desktop/src/components/settings/general.tsx (3)
3-3: Unresolved: Missing React namespace import for type annotations.The file uses
React.KeyboardEventandReact.ChangeEvent(lines 96, 131) but doesn't import the React namespace, which will cause TypeScript errors.Apply this diff:
-import { useMemo, useState } from "react"; +import type React from "react"; +import { useMemo, useState } from "react";
68-80: Unresolved: Replace mock permission states with real API integration.The permission states remain mocked with
useStatetoggles. The past review comment provided detailed guidance on integratinglistenerCommandsfrom@hypr/plugin-listener(similar to the onboarding component).This should be addressed to provide functional permission management.
424-424: Fixcn()to use array format.Line 424 passes arguments directly to
cn()instead of wrapping them in an array, violating the coding guideline that requires allcn()calls to use the array format.As per coding guidelines. Apply this diff:
- <h3 className={cn("text-sm font-medium mb-1", !hasAccess && "text-red-500")}> + <h3 className={cn(["text-sm font-medium mb-1", !hasAccess && "text-red-500"])}>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/components/main/body/index.tsx(0 hunks)apps/desktop/src/components/main/body/search.tsx(1 hunks)apps/desktop/src/components/main/sidebar/profile/index.tsx(2 hunks)apps/desktop/src/components/settings/general.tsx(7 hunks)apps/desktop/src/contexts/search/ui.tsx(5 hunks)apps/desktop/src/contexts/shell/index.tsx(1 hunks)apps/desktop/src/contexts/shell/search.ts(1 hunks)apps/desktop/src/contexts/shell/settings.ts(1 hunks)apps/desktop/src/contexts/shell/tabs.ts(1 hunks)apps/desktop/src/routes/app/main/_layout.tsx(3 hunks)packages/tiptap/src/editor/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/components/main/body/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/contexts/shell/index.tsx
- apps/desktop/src/contexts/search/ui.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/body/search.tsxapps/desktop/src/components/settings/general.tsxapps/desktop/src/components/main/sidebar/profile/index.tsxapps/desktop/src/routes/app/main/_layout.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When using `cn` for Tailwind classNames, always pass an array
Applied to files:
apps/desktop/src/components/settings/general.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : When there are many Tailwind classNames with conditional logic, use the utility `cn` imported as `import { cn } from "hypr/utils"`
Applied to files:
apps/desktop/src/components/settings/general.tsx
📚 Learning: 2025-10-21T03:03:20.070Z
Learnt from: CR
PR: fastrepl/hyprnote#0
File: apps/desktop/.cursor/rules/style.mdc:0-0
Timestamp: 2025-10-21T03:03:20.070Z
Learning: Applies to apps/desktop/**/*.{tsx,jsx} : Group Tailwind classNames by logical sections when using `cn` (split array items by grouping)
Applied to files:
apps/desktop/src/components/settings/general.tsx
🧬 Code graph analysis (4)
apps/desktop/src/components/main/body/search.tsx (1)
apps/desktop/src/contexts/search/ui.tsx (1)
useSearch(237-243)
apps/desktop/src/contexts/shell/tabs.ts (1)
plugins/windows/src/ext.rs (1)
close(56-62)
apps/desktop/src/components/settings/general.tsx (2)
packages/utils/src/cn.ts (1)
cn(20-22)apps/desktop/src/components/main/body/search.tsx (1)
Search(7-74)
apps/desktop/src/routes/app/main/_layout.tsx (2)
apps/desktop/src/contexts/search/ui.tsx (2)
SearchUIProvider(144-235)useSearch(237-243)apps/desktop/src/contexts/shell/index.tsx (1)
ShellProvider(25-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (10)
packages/tiptap/src/editor/index.tsx (2)
70-77: LGTM!The backspace handling logic correctly prevents deletion at the document start when the selection is empty. The implementation properly checks cursor position and uses the standard event handling pattern.
79-82: Verify that tab trapping doesn't hinder keyboard navigation.Unconditionally preventing the default tab behavior may trap keyboard focus within the editor, potentially affecting accessibility for keyboard-only users. Ensure there's an alternative keyboard mechanism (such as Escape) to exit the editor.
apps/desktop/src/components/main/sidebar/profile/index.tsx (1)
2-2: LGTM!The import statement is correct and the components are properly used in the Settings menu item badge.
apps/desktop/src/contexts/shell/settings.ts (1)
5-23: LGTM!The hook correctly memoizes
openSettingswithuseCallbackand the effect dependency is stable. The keyboard shortcut registration is clean and follows the same pattern as the search hook.apps/desktop/src/routes/app/main/_layout.tsx (2)
52-71: LGTM!The
handleNewTabcallback correctly:
- Persists a new session row with required fields
- Optionally closes the current tab before opening a new one
- Opens a new session tab with the appropriate state
The logic is clear and the dependencies are correctly specified.
82-93: LGTM!The
SearchShortcutBridgecomponent appropriately bridges the search context and shell provider, extractingfocusInputfrom the search context and passing it to theShellProviderasonFocusSearch. The provider nesting is correct.apps/desktop/src/components/settings/general.tsx (2)
230-327: LGTM!The inline language search panel is well-implemented with:
- Proper keyboard navigation (ArrowUp/Down, Enter, Escape, Backspace)
- ARIA attributes for accessibility (role, aria-haspopup, aria-expanded, aria-controls, aria-activedescendant)
- Clean focus management with
onMouseDownto prevent blur- Correct
cn()array usage throughoutThe UX improvement over the previous popover approach is significant.
330-381: LGTM!The vocabulary input provides a clean token-based interface with:
- Comma-separated term support
- Backspace to remove last term
- Enter to add a term
- Proper duplicate prevention
- Consistent styling with the language panel
apps/desktop/src/components/main/body/search.tsx (1)
14-18: Code is correct—registerFocusCallback is properly memoized.The
registerFocusCallbackis defined withuseCallbackand an empty dependency array[], making it stable across renders. The effect will not unnecessarily re-run on every render.apps/desktop/src/contexts/shell/search.ts (1)
7-21: No action required - onFocusSearch is properly memoized.The callback is wrapped with
useCallback(..., [])inSearchUIProvider(line 206 ofapps/desktop/src/contexts/search/ui.tsx), giving it a stable reference that persists across renders. This prevents unnecessary re-registration of the event listener inuseSearchShortcut.
…ter to co-locate with ui component
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/components/main/body/index.tsx (2)
8-9: Imports look good; cn usage matches our guidelines.You’re importing cn and using it with array form downstream. Consider grouping array items by logical sections for readability (base layout, conditional paddings, scrollbars).
As per coding guidelines.
45-46: Shortcut hook colocated in Header is fine; minor nit on unused ref.useTabsShortcuts() here ensures lifecycle matches Header. Note: tabsScrollContainerRef is currently unused—safe to remove later.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/components/main/body/index.tsx(4 hunks)apps/desktop/src/components/main/body/search.tsx(2 hunks)apps/desktop/src/contexts/search/ui.tsx(4 hunks)apps/desktop/src/contexts/shell/index.tsx(1 hunks)apps/desktop/src/contexts/shell/settings.ts(1 hunks)apps/desktop/src/routes/app/main/_layout.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/contexts/search/ui.tsx
- apps/desktop/src/contexts/shell/settings.ts
- apps/desktop/src/contexts/shell/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop/.cursor/rules/style.mdc)
apps/desktop/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the utilitycnimported asimport { cn } from "@hypr/utils"
When usingcnfor Tailwind classNames, always pass an array
Group Tailwind classNames by logical sections when usingcn(split array items by grouping)
Files:
apps/desktop/src/components/main/body/search.tsxapps/desktop/src/routes/app/main/_layout.tsxapps/desktop/src/components/main/body/index.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/main/body/search.tsx (1)
apps/desktop/src/contexts/search/ui.tsx (1)
useSearch(219-225)
apps/desktop/src/routes/app/main/_layout.tsx (2)
apps/desktop/src/contexts/search/ui.tsx (1)
SearchUIProvider(141-217)apps/desktop/src/contexts/shell/index.tsx (1)
ShellProvider(15-25)
apps/desktop/src/components/main/body/index.tsx (1)
plugins/windows/src/ext.rs (1)
close(56-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (7)
apps/desktop/src/components/main/body/search.tsx (3)
2-2: LGTM! Clean separation of focus management concerns.The refactoring appropriately separates global focus control (via
inputReffrom context) from local UI state (viaisFocusedfor styling). This allows the context to programmatically focus the input while the component manages its own width animation.Also applies to: 8-9
30-34: Verify Escape event propagation is intentional.The Escape handler blurs the input but allows the event to propagate. If other handlers up the component tree also listen for Escape (e.g., to close modals or panels), they will fire as well. Verify this is the intended behavior.
If the Escape should only blur the input without triggering parent handlers, apply this diff:
onKeyDown={(e) => { if (e.key === "Escape") { + e.preventDefault(); + e.stopPropagation(); e.currentTarget.blur(); } }}
35-36: LGTM! Focus handlers are correctly implemented.The inline handlers appropriately sync the local
isFocusedstate with the actual focus state of the input element, enabling the width transition effect.apps/desktop/src/routes/app/main/_layout.tsx (1)
53-62: Provider reordering is valid and introduces no breaking changes.The new hierarchy correctly maintains all dependencies:
SearchUIProviderdepends onSearchEngineProvider(satisfied), andShellProviderhas no dependencies on SearchEngine or SearchUI contexts. AlluseShell()consumers remain properly nested insideShellProvider, and the reordering allows components within the Shell subtree to access SearchUI context if needed in the future (e.g., for settings improvements). No circular dependencies or scope violations detected.apps/desktop/src/components/main/body/index.tsx (3)
41-41: Tabs actions wiring LGTM.Exposing closeOthers/closeAll to TabItem is clean and keeps Header lean.
43-43: Good: central new-tab handler.useNewTab() centralizes session creation and keeps Header stateless.
355-378: The review comment fundamentally misunderstands TinyBase's API and suggests incorrect changes.TinyBase's
setRow()method is synchronous and returnsthis(for method chaining), not a Promise. The current code correctly callssetRow()withoutawait. This pattern is consistent throughout the entire codebase. The suggested diff to makehandlerasync andawait persistedStore.setRow()contradicts TinyBase's actual API and would not work.The optional chaining (
persistedStore?.setRow()) is a valid guard that gracefully handles cases wherepersistedStoreis unavailable—a pattern already accepted in the codebase. No changes are needed.Likely an incorrect or invalid review comment.
• Improved settings layout design
• General updates and refinements