Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a languages dependency, moves ChatFloatingButton into Body, extends timeline context menu with “New Tab”, reworks Settings (General, Notifications, Templates, AI wiring), and updates tinybase AI/general schema defaults and types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant SG as SettingsGeneral
participant LANG as @huggingface/languages
participant HG as useUpdateGeneral
U->>SG: Open Settings → General
SG->>LANG: Load LANGUAGES_ISO_639_1
SG-->>U: Render language selector, spoken languages, toggles, jargons
U->>SG: Change field
SG->>HG: handle.setField(field, value)
HG-->>SG: Persisted state
SG-->>U: UI updates
sequenceDiagram
autonumber
participant U as User
participant TL as TimelineItem
participant CM as ContextMenu
participant NAV as Navigation
U->>TL: Right-click item
TL->>CM: Show menu (New Tab, Delete)
U->>CM: Click "New Tab"
CM->>NAV: handleCmdClick(item) — open in background tab
NAV-->>U: Item opens in new tab
sequenceDiagram
autonumber
participant U as User
participant TE as TemplateEditor
participant HT as useUpdateTemplates
U->>TE: Open TemplateEditor
TE-->>U: Render title, system prompt, sections list
U->>TE: Edit title/section or reorder
TE->>HT: setField('sections'/'title'/'system', value)
HT-->>TE: Persisted
U->>TE: Save / Close
TE-->>U: Call onClose()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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: 9
🧹 Nitpick comments (8)
apps/desktop2/src/components/main/sidebar/timeline.tsx (1)
134-134: Remove redundant color styling.The
text-red-500class is applied both to the parentContextMenuItem(line 133) and to theTrash2icon. The icon will inherit the color from its parent, making the duplicate styling unnecessary.Apply this diff to remove the redundant styling:
- <Trash2 className="w-4 h-4 mr-2 text-red-500" /> + <Trash2 className="w-4 h-4 mr-2" />apps/desktop2/src/components/settings/templates.tsx (1)
96-99: Replace direct crypto.randomUUID() calls with a cross‑runtime id generator (feature‑detect or use nanoid)Search found many direct uses of crypto.randomUUID() (notably apps/desktop2/src/components/settings/templates.tsx and apps/desktop2/src/utils.ts) and no polyfill/fallback; apps/desktop/package.json lists nanoid. Action: centralize id generation (or update apps/desktop2/src/utils.ts) to feature‑detect crypto.randomUUID and fall back to nanoid/uuid, then replace all direct crypto.randomUUID() usages with that util.
apps/desktop2/package.json (1)
18-18: Consider pinning @huggingface/languages for reproducibilityUse an exact version (e.g., 1.0.0) to avoid future drift in metadata that might subtly change names/fields.
apps/desktop2/src/components/settings/general.tsx (4)
108-123: Persist language codes, not display namesYou currently store/display language names (e.g., "English"). Names can change or collide; ISO-639-1 codes are stable. Store codes in settings and map to names in the UI.
Minimal guidance:
- ai_language: store
code(e.g., "en"), showHF_LANGUAGES[code].name.- spoken_languages: store array of codes, show chips via
HF_LANGUAGES[code].name.- Filters should compare codes, not names.
This avoids duplicate name issues and simplifies future i18n.
Also applies to: 169-181, 131-151
197-205: Remove no‑op onBlur handlerThe onBlur callback does nothing and adds noise.
- <Textarea - value={(value.jargons ?? []).join(", ")} - onChange={(e) => handle.setField("jargons", e.target.value.split(",").map(s => s.trim()).filter(Boolean))} - onBlur={() => { - // Auto-save on blur - }} - placeholder="smart notepad, offline, X, Discord" - className="w-full resize-none" - rows={3} - /> + <Textarea + value={(value.jargons ?? []).join(", ")} + onChange={(e) => + handle.setField( + "jargons", + e.target.value.split(",").map(s => s.trim()).filter(Boolean), + ) + } + placeholder="smart notepad, offline, X, Discord" + className="w-full resize-none" + rows={3} + />
235-238: “Grant Permission” button has no actionAdd an onClick to request/guide permissions, or disable the button until wiring exists. Otherwise it’s confusing.
I can help wire this to your permissions flow or add a disabled state with a tooltip.
246-266: Duplicate SettingRow across settings screens — extract shared componentThis component is duplicated here and in notifications.tsx. Move it to a shared module (e.g., settings/SettingRow.tsx) and reuse.
apps/desktop2/src/components/settings/notification.tsx (1)
31-51: Deduplicate SettingRow (same as in general.tsx)Extract to a shared component and import in both files to avoid drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/desktop2/package.json(1 hunks)apps/desktop2/src/components/main/body/index.tsx(2 hunks)apps/desktop2/src/components/main/sidebar/timeline.tsx(2 hunks)apps/desktop2/src/components/settings/general.tsx(1 hunks)apps/desktop2/src/components/settings/notification.tsx(1 hunks)apps/desktop2/src/components/settings/templates.tsx(2 hunks)apps/desktop2/src/routes/app/main/_layout.index.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop2/src/routes/app/main/_layout.index.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/desktop2/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop2/.cursor/rules/style.mdc)
apps/desktop2/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the cn utility imported asimport { cn } from "@hypr/ui/lib/utils"
Always pass an array to cn when composing Tailwind classNames
Split cn array entries by logical grouping when composing Tailwind classNames
Files:
apps/desktop2/src/components/main/sidebar/timeline.tsxapps/desktop2/src/components/settings/notification.tsxapps/desktop2/src/components/settings/general.tsxapps/desktop2/src/components/main/body/index.tsxapps/desktop2/src/components/settings/templates.tsx
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop2/src/components/main/sidebar/timeline.tsxapps/desktop2/src/components/settings/notification.tsxapps/desktop2/src/components/settings/general.tsxapps/desktop2/src/components/main/body/index.tsxapps/desktop2/src/components/settings/templates.tsx
🧬 Code graph analysis (5)
apps/desktop2/src/components/main/sidebar/timeline.tsx (1)
packages/ui/src/components/ui/context-menu.tsx (1)
ContextMenuItem(186-186)
apps/desktop2/src/components/settings/notification.tsx (2)
apps/desktop2/src/components/settings/shared.ts (1)
useUpdateGeneral(6-24)packages/ui/src/components/ui/switch.tsx (1)
Switch(57-57)
apps/desktop2/src/components/settings/general.tsx (1)
apps/desktop2/src/components/settings/shared.ts (1)
useUpdateGeneral(6-24)
apps/desktop2/src/components/main/body/index.tsx (1)
apps/desktop2/src/components/chat/index.tsx (1)
ChatFloatingButton(11-56)
apps/desktop2/src/components/settings/templates.tsx (4)
apps/desktop/src/components/settings/views/template.tsx (1)
TemplateEditor(68-424)packages/db/src/schema.ts (1)
templates(83-88)packages/ui/src/components/ui/button.tsx (1)
Button(37-89)apps/desktop2/src/components/settings/shared.ts (1)
useUpdateTemplate(38-55)
⏰ 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: ci (macos, macos-14)
- GitHub Check: ci (windows, windows-latest)
🔇 Additional comments (8)
apps/desktop2/src/components/main/body/index.tsx (2)
8-8: LGTM!The import is correctly placed and used within the component.
31-31: No duplicate ChatFloatingButton instances Body is only rendered once inapps/desktop2/src/routes/app/main/_layout.index.tsx, so global hotkey registration and window state conflicts cannot occur.apps/desktop2/src/components/main/sidebar/timeline.tsx (2)
3-3: LGTM!The lucide-react imports are correctly used in the context menu items below.
129-132: LGTM!The "New Tab" context menu item correctly reuses the existing
handleCmdClickfunction, maintaining consistency with the existing Cmd+Click behavior.apps/desktop2/src/components/settings/templates.tsx (3)
1-11: LGTM!The imports are well-organized and all are utilized in the component. The lucide-react icons, motion/react Reorder component, and UI components are appropriately imported for the new drag-reorder template editor functionality.
101-123: LGTM!The section management handlers (reorder, update, remove, add) are well-structured and maintain consistency between local state and the persisted store. The pattern of stripping IDs before syncing to the store (
updatedSections.map(({ id, ...rest }) => rest)) correctly maintains the separation between UI state and data model.
174-206: Verify drag-and-drop functionality with keyboard accessibility.The Reorder implementation uses motion/react's Reorder components for drag-and-drop. Ensure that keyboard users can also reorder sections, as drag-and-drop interfaces can be inaccessible without keyboard alternatives.
Test the following:
- Can keyboard users (using Tab, Enter, and arrow keys) reorder sections?
- Are appropriate ARIA attributes present for screen reader users?
- Is there visual feedback during keyboard-based reordering?
If keyboard accessibility is not built into the motion/react Reorder component, you may need to add keyboard handlers or provide alternative reordering controls (e.g., up/down buttons).
apps/desktop2/src/components/settings/notification.tsx (1)
13-25: LGTM on Switch wiring and state updatesThe toggles read null-safe values and update via handle.setField. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
apps/desktop2/src/components/settings/templates.tsx (5)
47-55: Stop usinganyfor template titles/descriptions.Line [51] still relies on
(template as any), defeating the persisted schema and risking runtimeundefinedif the row shape shifts. Please type the query result once and drop the cast. For example:const templates = persisted.UI.useResultTable(USER_TEMPLATE_QUERY, quries); + type TemplateRow = persisted.Template; + const typedTemplates = templates as Record<string, TemplateRow>; … - {Object.entries(templates).map(([id, template]) => ( + {Object.entries(typedTemplates).map(([id, template]) => ( <TemplateCard key={id} id={id} - title={(template as any).title || "Untitled"} - description={(template as any).description || ""} + title={template.title ?? "Untitled"} + description={template.description ?? ""} onEdit={() => setCurrentTemplate(id)} />
81-89: Finish the “Set as default” action.Line [85] still just logs, so users can’t actually set a default template. Wire this button into the real mutation (or disable it with a tracked TODO) instead of
console.log.
97-105: Givesectionsa real type instead of(s: any).Line [99] repeats the unsafe
(s: any)map. Please reuse the persisted schema so the editor stays type-safe:- const [sections, setSections] = useState(() => - (value.sections || []).map((s: any) => ({ ...s, id: crypto.randomUUID() })) - ); + type SectionWithId = persisted.Template["sections"][number] & { id: string }; + const [sections, setSections] = useState<SectionWithId[]>(() => + (value.sections ?? []).map((section) => ({ ...section, id: crypto.randomUUID() })) + );
138-144: Implement the Delete flow before shipping.Line [141] still only logs, so “Delete” never happens. Hook this button into the actual delete mutation (with loading/cleanup) or remove it until the backend call exists—leaving it as a no-op is a broken UX.
150-202: Restore visible focus styling for keyboard users.Lines [152], [195], and [201] all include
focus-visible:ring-0, which removes any focus indicator and violates WCAG 2.4.7. Swap in an accessible focus style instead, e.g.:- className="text-lg font-semibold border-none p-0 focus-visible:ring-0" + className="text-lg font-semibold border-none p-0 focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2" … - className="border-0 bg-transparent p-0 text-base font-medium focus-visible:ring-0 focus-visible:ring-offset-0 placeholder:text-muted-foreground/60" + className="border-0 bg-transparent p-0 text-base font-medium focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2 placeholder:text-muted-foreground/60" … - className="min-h-[30px] resize-none border-0 bg-transparent p-0 text-sm text-muted-foreground focus-visible:ring-0 focus-visible:ring-offset-0 placeholder:text-muted-foreground/50" + className="min-h-[30px] resize-none border-0 bg-transparent p-0 text-sm text-muted-foreground focus-visible:ring-2 focus-visible:ring-primary focus-visible:ring-offset-2 placeholder:text-muted-foreground/50"Apply a similar treatment wherever you removed the default ring.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop2/src/components/settings/ai.tsx(3 hunks)apps/desktop2/src/components/settings/templates.tsx(3 hunks)apps/desktop2/src/store/tinybase/internal.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/desktop2/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (apps/desktop2/.cursor/rules/style.mdc)
apps/desktop2/**/*.{tsx,jsx}: When there are many Tailwind classNames with conditional logic, use the cn utility imported asimport { cn } from "@hypr/ui/lib/utils"
Always pass an array to cn when composing Tailwind classNames
Split cn array entries by logical grouping when composing Tailwind classNames
Files:
apps/desktop2/src/components/settings/ai.tsxapps/desktop2/src/components/settings/templates.tsx
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. Do not add any error handling. Keep the existing one.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop2/src/components/settings/ai.tsxapps/desktop2/src/store/tinybase/internal.tsapps/desktop2/src/components/settings/templates.tsx
🔇 Additional comments (5)
apps/desktop2/src/store/tinybase/internal.ts (3)
54-54: Schema definition correctly mirrors Zod schema.The TinyBase schema definition for
model_nameis consistent with the Zod schema change on line 26.
19-20: LGTM! Consider verifying provider identifiers.The new LLM and STT provider fields are correctly defined with sensible defaults. The schema definitions are consistent between the Zod schema and TinyBase schema.
Consider running the following script to verify that "hyprcloud" and "hyprlocal" are valid provider identifiers used elsewhere in the codebase:
26-26: Verify impact of required field on existing AI records.The
model_namefield is added as a required field without a default value. This could break existing code or data if:
- There are existing AI records in storage that don't have this field
- Code that creates/updates AI records hasn't been updated to include this field
Consider either:
- Adding a default value:
model_name: z.string().default("")or similar- Making it optional:
model_name: z.string().optional()- Or verifying that all existing usage points and data have been updated
Run the following script to find where AI records are created or the AI type is used:
apps/desktop2/src/components/settings/ai.tsx (2)
139-153: LGTM!The refactor to extract
handleClickimproves code organization and allows for combining the toggle logic with the optional provider selection callback.
130-142: Verify the usage ofhandleSelectProviderprop.The
handleSelectProvidercallback prop is defined but doesn't appear to be passed by any parent component (TranscriptionSettingsorIntelligenceSettings). Ensure this prop is being used or consider removing it if it's not yet needed.Run the following script to check if this prop is being passed anywhere:
2 participants
Uh oh!
There was an error while loading. Please reload this page.