refactor/simplify-provider-selection-components#2280
Conversation
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughReplaces headerAction-driven AI header with an inline Transcription/Intelligence toggle and vertical layout; moves NonHyprProviderCard into shared module with provider-type props; removes HealthCheckForAvailability and headerAction props from STT/LLM/select components; adds missing-file early-return in JSON persister; UI/style tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
54-66: Consider edge cases in configuration detection.The
useIsProviderConfiguredhook assumes all providers require an API key (line 61-62), but some providers (like "hyprnote" or custom endpoints) might not require one. This could lead to false negatives where properly configured providers appear as unconfigured.Consider checking provider requirements before validating api_key:
function useIsProviderConfigured(providerId: string) { const configuredProviders = keys.UI.useResultTable( keys.QUERIES.sttProviders, keys.STORE_ID, ); + const providerDef = PROVIDERS.find((p) => p.id === providerId); const config = configuredProviders[providerId]; if (!config || !config.api_key) { return false; } + + // Some providers may not require an API key + if (providerDef?.apiKey && !config.api_key) { + return false; + } return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/components/main/body/ai.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/main/body/ai.tsxapps/desktop/src/components/settings/ai/llm/select.tsxapps/desktop/src/components/settings/ai/llm/index.tsxapps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/settings/ai/stt/select.tsxapps/desktop/src/components/settings/ai/stt/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:32:23.055Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:23.055Z
Learning: Applies to **/*.{ts,tsx} : If there are many classNames with conditional logic, use `cn` (import from `hypr/utils`). It is similar to `clsx`. Always pass an array and split by logical grouping.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsx
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : If there are many classNames with conditional logic, use `cn` (import from `hypr/utils`), similar to `clsx`. Always pass an array and split by logical grouping.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsx
🧬 Code graph analysis (7)
apps/desktop/src/components/main/body/ai.tsx (4)
extensions/shared/types/hypr-extension.d.ts (1)
Button(159-159)packages/utils/src/cn.ts (1)
cn(20-22)apps/desktop/src/components/settings/ai/stt/index.tsx (1)
STT(4-11)apps/desktop/src/components/settings/ai/llm/index.tsx (1)
LLM(4-11)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(28-219)
apps/desktop/src/components/settings/ai/llm/index.tsx (2)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(32-188)apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(28-219)
apps/desktop/src/components/settings/ai/llm/configure.tsx (4)
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
ConfigureProviders(31-52)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)apps/desktop/src/components/settings/ai/stt/shared.tsx (1)
PROVIDERS(189-197)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/ai/stt/configure.tsx (2)
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
ConfigureProviders(19-39)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(32-188)
apps/desktop/src/components/settings/ai/stt/index.tsx (2)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(32-188)apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(28-219)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
🔇 Additional comments (10)
apps/desktop/src/components/main/body/ai.tsx (1)
57-87: LGTM! Clean refactor to inline toggle buttons.The new layout successfully removes the headerAction prop pattern and uses inline toggle buttons with proper conditional styling. The
cnusage follows the coding guidelines by passing arrays and splitting by logical grouping.apps/desktop/src/components/settings/ai/llm/index.tsx (1)
4-11: LGTM! Clean simplification.The component is now properly simplified by removing the headerAction prop and HealthCheckForAvailability dependency, resulting in a cleaner composition pattern.
apps/desktop/src/components/settings/ai/stt/select.tsx (2)
28-28: LGTM! Signature simplified.The removal of the headerAction prop aligns with the PR's objective to simplify provider selection components.
208-215: LGTM! Clear user feedback for missing configuration.The warning UI provides clear feedback when transcription model configuration is incomplete. This pattern is consistent with the LLM select component.
apps/desktop/src/components/settings/ai/stt/index.tsx (1)
4-11: LGTM! Consistent simplification.The component now mirrors the LLM component structure by removing headerAction and health check dependencies, maintaining consistency across the codebase.
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
108-111: LGTM! Clear visual distinction for configuration state.The conditional border styling provides clear visual feedback about whether a provider is configured. The use of
cnwith arrays follows the coding guidelines.apps/desktop/src/components/settings/ai/llm/configure.tsx (2)
41-58: Good implementation of configuration detection.The
useIsProviderConfiguredhook properly checks provider requirements before validating the API key (lines 53-55), which handles providers that don't require API keys correctly. This is more robust than the STT version.
109-112: LGTM! Consistent configuration state visualization.The conditional border styling matches the pattern in the STT configure component, maintaining consistency across the codebase. The use of
cnwith arrays follows the coding guidelines.apps/desktop/src/components/settings/ai/llm/select.tsx (2)
32-32: LGTM! Signature simplified.The removal of the headerAction prop is consistent with the refactoring pattern applied across both LLM and STT components.
177-184: LGTM! Clear user feedback for missing configuration.The warning UI provides clear, context-specific feedback when language model configuration is incomplete. This mirrors the pattern in the STT select component, maintaining consistency.
b7011bf to
e7756e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src/components/main/body/ai.tsx (1)
59-82: Consider extracting toggle button logic to reduce duplication.The two toggle buttons share nearly identical structure and styling, differing only in their tab key, icon, and label. This could be refactored into a mapped structure for better maintainability.
Example refactor:
+ const tabs = [ + { key: "transcription" as const, icon: AudioLinesIcon, label: "Transcription" }, + { key: "intelligence" as const, icon: SparklesIcon, label: "Intelligence" }, + ]; + <div className="flex gap-1 px-6 pt-6 pb-2"> - <Button - variant="ghost" - size="sm" - onClick={() => setActiveTab("transcription")} - className={cn([ - "gap-1.5 h-7 px-2 border border-transparent", - activeTab === "transcription" && "bg-neutral-100 border-neutral-200", - ])} - > - <AudioLinesIcon size={14} /> - <span className="text-xs">Transcription</span> - </Button> - <Button - variant="ghost" - size="sm" - onClick={() => setActiveTab("intelligence")} - className={cn([ - "gap-1.5 h-7 px-2 border border-transparent", - activeTab === "intelligence" && "bg-neutral-100 border-neutral-200", - ])} - > - <SparklesIcon size={14} /> - <span className="text-xs">Intelligence</span> - </Button> + {tabs.map(({ key, icon: Icon, label }) => ( + <Button + key={key} + variant="ghost" + size="sm" + onClick={() => setActiveTab(key)} + className={cn([ + "gap-1.5 h-7 px-2 border border-transparent", + activeTab === key && "bg-neutral-100 border-neutral-200", + ])} + > + <Icon size={14} /> + <span className="text-xs">{label}</span> + </Button> + ))} </div>apps/desktop/src/components/settings/ai/stt/select.tsx (1)
28-218: Consider extracting shared logic between STT and LLM select components.Based on the relevant code snippets, both
apps/desktop/src/components/settings/ai/stt/select.tsxandapps/desktop/src/components/settings/ai/llm/select.tsxshare nearly identical structure:
- Same function signature (no props)
- Same heading "Model being used"
- Same conditional background color logic
- Same warning UI pattern when provider/model are missing
- Similar form handling and rendering logic
The only differences are the config keys (
current_stt_*vscurrent_llm_*), warning text, and provider-specific queries. This could be refactored into a shared component that accepts a type parameter or configuration object.Note: This is a broader architectural improvement that could be addressed in a follow-up PR.
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
189-191: Consider applying consistent configuration status indicator to HyprProviderCard.
NonHyprProviderCardusesisConfiguredto show visual status (dashed vs. solid border), butHyprProviderCardalways shows a dashed border regardless of configuration state. If the Hyprnote provider can also be configured/unconfigured, consider applying the same visual treatment for consistency.If this distinction is intentional (e.g., Hyprnote is always available or has different semantics), this can be safely ignored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/components/main/body/ai.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/components/settings/ai/stt/index.tsx
- apps/desktop/src/components/settings/ai/llm/select.tsx
- apps/desktop/src/components/settings/ai/stt/configure.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/settings/ai/llm/index.tsxapps/desktop/src/components/settings/ai/stt/select.tsxapps/desktop/src/components/main/body/ai.tsxapps/desktop/src/components/settings/ai/llm/configure.tsx
🧬 Code graph analysis (2)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(32-188)
apps/desktop/src/components/main/body/ai.tsx (4)
extensions/shared/types/hypr-extension.d.ts (1)
Button(159-159)packages/utils/src/cn.ts (1)
cn(20-22)apps/desktop/src/components/settings/ai/stt/index.tsx (1)
STT(4-11)apps/desktop/src/components/settings/ai/llm/index.tsx (1)
LLM(4-11)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: fmt
🔇 Additional comments (7)
apps/desktop/src/components/main/body/ai.tsx (1)
57-87: LGTM! Clean refactor with proper utility usage.The vertical layout with inline toggle buttons effectively replaces the headerAction pattern. The use of
cnwith array syntax follows the coding guidelines, and the conditional rendering logic is straightforward.apps/desktop/src/components/settings/ai/llm/index.tsx (1)
4-10: LGTM! Clean simplification of component.The removal of the
headerActionprop andHealthCheckForAvailabilityaligns with the PR's goal to simplify provider selection components. The component now has a cleaner interface and consistent structure with the STT equivalent.apps/desktop/src/components/settings/ai/stt/select.tsx (2)
28-75: LGTM! Simplified component signature.The removal of the
headerActionprop and addition of a static heading simplifies the component interface and aligns with the broader refactor to remove headerAction dependencies across AI components.
76-215: Good UX with clear visual feedback.The conditional background color and warning message provide excellent user feedback when configuration is incomplete. The implementation is clean and the warning text is specific to transcription use case.
apps/desktop/src/components/settings/ai/llm/configure.tsx (3)
15-15: LGTM! Supporting changes for configuration status feature.The keys import supports the new
useIsProviderConfiguredhook, and the heading size adjustment improves visual hierarchy.Also applies to: 22-22
68-68: Good visual indicator for provider configuration status.The conditional border styling (dashed for unconfigured, solid for configured) effectively communicates provider setup status to users, aligning with the PR's goal to add provider configuration status indicators.
Also applies to: 109-112
41-58: No changes needed. Empty strings are already properly handled by the current logic.The hook's checks using
!config.base_urland!config.api_keyalready catch empty strings because empty strings are falsy in JavaScript. When either value is an empty string, the negation returnstrueand the function returnsfalseas intended. Additionally, theaiProviderSchemaenforces thatbase_urlis a valid URL withz.url().min(1), preventing empty strings from being stored in the first place.
e7756e6 to
9870909
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/components/main/body/ai.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(4 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(3 hunks)apps/desktop/src/store/tinybase/jsonPersister.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/store/tinybase/jsonPersister.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/store/tinybase/jsonPersister.tsapps/desktop/src/components/settings/ai/stt/index.tsxapps/desktop/src/components/settings/ai/llm/index.tsxapps/desktop/src/components/settings/ai/stt/select.tsxapps/desktop/src/components/settings/ai/llm/select.tsxapps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/main/body/ai.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:32:23.055Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:23.055Z
Learning: Applies to **/*.{ts,tsx} : If there are many classNames with conditional logic, use `cn` (import from `hypr/utils`). It is similar to `clsx`. Always pass an array and split by logical grouping.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsx
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : If there are many classNames with conditional logic, use `cn` (import from `hypr/utils`), similar to `clsx`. Always pass an array and split by logical grouping.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsx
🧬 Code graph analysis (5)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(31-187)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(27-218)
apps/desktop/src/components/settings/ai/llm/configure.tsx (3)
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
ConfigureProviders(30-51)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/main/body/ai.tsx (4)
extensions/shared/types/hypr-extension.d.ts (1)
Button(159-159)packages/utils/src/cn.ts (1)
cn(20-22)apps/desktop/src/components/settings/ai/stt/index.tsx (1)
STT(4-11)apps/desktop/src/components/settings/ai/llm/index.tsx (1)
LLM(4-11)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
🔇 Additional comments (13)
apps/desktop/src/store/tinybase/jsonPersister.ts (1)
180-186: LGTM!The early return for file-not-found errors is appropriate for a migration function. This reduces log noise when the legacy file doesn't exist, which is the expected state for most users.
apps/desktop/src/components/settings/ai/stt/configure.tsx (2)
33-33: LGTM!The heading size increase improves visual hierarchy and is consistent with the LLM configure screen.
107-110: LGTM!The conditional border styling effectively communicates configuration status. The use of
cnwith array grouping follows the coding guidelines.apps/desktop/src/components/settings/ai/llm/configure.tsx (3)
22-22: LGTM!The heading size increase improves visual hierarchy.
41-58: LGTM!The configuration validation logic correctly:
- Validates
base_urlpresence for all providers- Conditionally validates
api_keyonly for providers that require it (based onproviderDef?.apiKey)This approach handles both API-based and local providers correctly.
109-112: LGTM!The conditional border styling effectively communicates configuration status, and the use of
cnwith array grouping follows the coding guidelines.apps/desktop/src/components/settings/ai/stt/index.tsx (1)
4-10: LGTM!The removal of the
headerActionprop simplifies the component interface and aligns with the refactored AI tab layout where header actions are now handled inline.apps/desktop/src/components/main/body/ai.tsx (1)
57-88: LGTM!The refactored inline header with toggle buttons is a good improvement:
- Cleaner component interface without
headerActionprop drilling- Clear visual indication of active tab with border styling
- Proper use of
cnwith array grouping per coding guidelines- Appropriate use of
Buttoncomponent withghostvariant for toggle behaviorapps/desktop/src/components/settings/ai/llm/index.tsx (1)
4-10: LGTM!The removal of the
headerActionprop simplifies the component interface and aligns with the refactored AI tab layout.apps/desktop/src/components/settings/ai/stt/select.tsx (2)
27-27: LGTM!The removal of the
headerActionprop simplifies the component interface and aligns with the inline header approach in the AI tab.
207-214: LGTM!The missing-model warning is a good UX improvement that clearly communicates to users when configuration is incomplete. The red border styling appropriately conveys urgency while the message clearly explains the impact.
apps/desktop/src/components/settings/ai/llm/select.tsx (2)
31-31: LGTM!The removal of the
headerActionprop simplifies the component interface and aligns with the refactored AI tab layout.
176-183: LGTM!The missing-model warning effectively communicates the impact of incomplete configuration and provides clear guidance to users.
69c7875 to
10a7802
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/main/sidebar/banner/component.tsx (1)
16-38: Dismiss button is effectively invisible for keyboard users (add focus-visible styles).Because the button starts at
opacity-0and only becomes visible viagroup-hover, tabbing to it won’t reveal it. Addfocus-visible:(orfocus:) opacity/background styles so keyboard users can discover it.<Button onClick={onDismiss} size="icon" variant="ghost" aria-label="Dismiss banner" className={cn([ "absolute top-1.5 right-1.5 size-6", "opacity-0 group-hover:opacity-50 hover:!opacity-100", "hover:bg-neutral-200", + "focus-visible:opacity-100 focus-visible:bg-neutral-200", "transition-all duration-200", ])} > <X className="w-3.5 h-3.5" /> </Button>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/components/main/sidebar/banner/component.tsx(2 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(2 hunks)apps/desktop/src/components/settings/ai/shared/index.tsx(2 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/main/sidebar/banner/component.tsxapps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:32:23.055Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:23.055Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
Applied to files:
apps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsx
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) for 99% of cases instead of setError and similar patterns.
Applied to files:
apps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsx
🧬 Code graph analysis (3)
apps/desktop/src/components/main/sidebar/banner/component.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/ai/llm/configure.tsx (2)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(79-195)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)
apps/desktop/src/components/settings/ai/stt/configure.tsx (4)
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
ConfigureProviders(14-40)apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(79-195)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)apps/desktop/src/components/settings/ai/stt/shared.tsx (1)
PROVIDERS(189-197)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
🔇 Additional comments (2)
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
11-38: Nice simplification: sharedNonHyprProviderCard+ explicitproviderType/providerContext.apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
25-55: Good alignment with LLM: sharedNonHyprProviderCardusage is consistent.
69fe9c9 to
f3c27cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/store/tinybase/jsonPersister.ts (1)
179-186: Consider checking error code instead of string matching.String matching error messages can be fragile and locale-dependent. If the error object has a
codeproperty, check it directly.} catch (error) { - const errorStr = String(error); - if ( - errorStr.includes("No such file or directory") || - errorStr.includes("ENOENT") - ) { + if ( + error && + typeof error === "object" && + "code" in error && + error.code === "ENOENT" + ) { return false; } console.error("[migrateKeysJsonToSettings] error:", error); return false; }apps/desktop/src/components/settings/ai/shared/index.tsx (1)
104-104: Consider making API key field conditional for STT providers.Line 104 always shows the API key field for STT providers (
providerType === "stt"), but this should respect theconfig.apiKeyflag like LLM providers do. Local providers (lmstudio, ollama) don't need API keys.- const showApiKey = providerType === "stt" || config.apiKey; + const showApiKey = config.apiKey;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/components/main/body/ai.tsx(1 hunks)apps/desktop/src/components/main/sidebar/banner/component.tsx(2 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(2 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(3 hunks)apps/desktop/src/components/settings/ai/shared/index.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(2 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(3 hunks)apps/desktop/src/store/tinybase/jsonPersister.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/components/settings/ai/llm/index.tsx
- apps/desktop/src/components/settings/ai/stt/index.tsx
- apps/desktop/src/components/main/sidebar/banner/component.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/store/tinybase/jsonPersister.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/store/tinybase/jsonPersister.tsapps/desktop/src/components/settings/ai/stt/select.tsxapps/desktop/src/components/main/body/ai.tsxapps/desktop/src/components/settings/ai/llm/select.tsxapps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/llm/configure.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:32:23.055Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:23.055Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/llm/configure.tsx
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) for 99% of cases instead of setError and similar patterns.
Applied to files:
apps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/settings/ai/llm/configure.tsx
🧬 Code graph analysis (5)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(31-187)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(27-218)
apps/desktop/src/components/settings/ai/stt/configure.tsx (4)
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
ConfigureProviders(14-40)apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(84-200)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)apps/desktop/src/components/settings/ai/stt/shared.tsx (1)
PROVIDERS(189-197)
apps/desktop/src/components/settings/ai/shared/index.tsx (3)
apps/desktop/src/billing.tsx (1)
useBillingAccess(66-74)packages/store/src/schema-internal.ts (2)
AIProvider(43-43)aiProviderSchema(29-41)extensions/shared/types/hypr-extension.d.ts (3)
AccordionItem(120-120)AccordionTrigger(121-121)AccordionContent(119-119)
apps/desktop/src/components/settings/ai/llm/configure.tsx (2)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(84-200)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: fmt
🔇 Additional comments (10)
apps/desktop/src/components/main/body/ai.tsx (1)
57-88: LGTM!The refactored tab layout with inline buttons and content pane is clean and follows the coding guidelines. The use of
cnwith arrays for conditional styling is appropriate.apps/desktop/src/components/settings/ai/stt/select.tsx (2)
27-27: LGTM!The removal of the
headerActionprop aligns with the PR's goal to simplify the component interface and centralize provider UI logic.
207-214: LGTM!The missing-model warning provides clear user feedback and is consistent with the LLM implementation pattern.
apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
25-25: LGTM!The migration to the shared
NonHyprProviderCardcomponent successfully centralizes provider configuration UI and eliminates code duplication. The props passed (config,providerType,providers,providerContext) match the expected signature.Also applies to: 43-49
apps/desktop/src/components/settings/ai/llm/select.tsx (2)
31-31: LGTM!Removing the
headerActionprop simplifies the component interface and aligns with the broader refactoring to centralize provider UI.
176-183: LGTM!The missing-model warning provides clear, contextual feedback to users and maintains consistency with the STT implementation.
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
11-11: LGTM!Successfully migrated to the shared
NonHyprProviderCard, eliminating code duplication and centralizing provider configuration logic. The component usage is consistent with the STT implementation.Also applies to: 28-34
apps/desktop/src/components/settings/ai/shared/index.tsx (3)
26-30: LGTM!The composite key approach (
type:id) effectively prevents configuration collisions between STT and LLM providers with the same id. This addresses the major issue flagged in previous reviews.
106-123: LGTM!The form setup with auto-submit on change and validation via
aiProviderSchemafollows the coding guidelines to useuseFormfrom TanStack Form. The microtask queuing for submit prevents race conditions.
243-260: LGTM!The updated
useProviderhook correctly uses composite keys to store and retrieve provider configurations, preventing data collisions between STT and LLM providers.
f3c27cc to
adcf609
Compare
adcf609 to
575d78e
Compare
575d78e to
eeb6644
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/desktop/src/components/settings/ai/shared/index.tsx (2)
238-250: Config collision:ai_providersrows keyed byidalone while queries filter bytype.The
useProviderhook persists toai_providersusing only the provideridas the row key (lines 239, 241), butuseIsProviderConfiguredqueries filter by provider type ("stt" vs "llm" at lines 44-47). This means configuring "openai" for STT (storingtype: "stt") and then configuring "openai" for LLM (storingtype: "llm") will overwrite the same row, silently losing the STT configuration.The fix requires using a composite key that includes both type and id (e.g.,
${providerType}:${id}) in:
useProviderpersistence (lines 239-245)configuredProviderslookups (line 54)- Any other code that references provider rows by id
This ensures STT and LLM configs for the same provider id don't collide.
60-66: STT validation unconditionally requires API key for all providers.Lines 64-65 always require an
api_keyfor STT providers, but some providers likelmstudioandollamadon't require API keys (they haveapiKey: falsein their definitions). This prevents users from configuring local STT providers.Apply this diff to make the API key requirement conditional:
if (providerType === "stt") { - if (!providerDef?.baseUrl && !config.base_url) { + if (providerDef?.baseUrl == null && !config.base_url) { return false; } - if (!config.api_key) { + if (providerDef?.apiKey && !config.api_key) { return false; } } else {
🧹 Nitpick comments (1)
apps/desktop/src/store/tinybase/jsonPersister.ts (1)
179-186: Consider type-safe error handling instead of string matching.The current implementation uses
String(error)and string matching to detect missing-file errors. This approach is fragile and loses type information.Apply this diff for more idiomatic TypeScript error handling:
} catch (error) { - const errorStr = String(error); - if ( - errorStr.includes("No such file or directory") || - errorStr.includes("ENOENT") - ) { - return false; - } + if (error instanceof Error) { + // Check for filesystem "not found" errors + if (error.message.includes("ENOENT") || + error.message.includes("No such file or directory")) { + return false; + } + } console.error("[migrateKeysJsonToSettings] error:", error); return false; }This approach:
- Preserves type information by checking
error instanceof Error- Accesses the
messageproperty safely- Falls through to log unexpected error types
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/components/main/body/ai.tsx(1 hunks)apps/desktop/src/components/main/sidebar/banner/component.tsx(2 hunks)apps/desktop/src/components/settings/ai/llm/configure.tsx(3 hunks)apps/desktop/src/components/settings/ai/llm/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/llm/select.tsx(3 hunks)apps/desktop/src/components/settings/ai/shared/index.tsx(4 hunks)apps/desktop/src/components/settings/ai/stt/configure.tsx(3 hunks)apps/desktop/src/components/settings/ai/stt/index.tsx(1 hunks)apps/desktop/src/components/settings/ai/stt/select.tsx(3 hunks)apps/desktop/src/store/tinybase/jsonPersister.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/components/settings/ai/llm/select.tsx
- apps/desktop/src/components/main/body/ai.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/desktop/src/components/settings/ai/stt/select.tsxapps/desktop/src/components/settings/ai/stt/index.tsxapps/desktop/src/store/tinybase/jsonPersister.tsapps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/llm/index.tsxapps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/stt/configure.tsxapps/desktop/src/components/main/sidebar/banner/component.tsx
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/desktop/src/store/tinybase/jsonPersister.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T16:32:23.055Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:23.055Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
Applied to files:
apps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/shared/index.tsxapps/desktop/src/components/settings/ai/stt/configure.tsx
📚 Learning: 2025-11-24T16:32:19.706Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:32:19.706Z
Learning: Applies to **/*.{ts,tsx} : Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) for 99% of cases instead of setError and similar patterns.
Applied to files:
apps/desktop/src/components/settings/ai/llm/configure.tsxapps/desktop/src/components/settings/ai/stt/configure.tsx
🧬 Code graph analysis (6)
apps/desktop/src/components/settings/ai/stt/select.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(31-187)
apps/desktop/src/components/settings/ai/stt/index.tsx (2)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(31-187)apps/desktop/src/components/settings/ai/stt/select.tsx (1)
SelectProviderAndModel(27-218)
apps/desktop/src/components/settings/ai/llm/configure.tsx (4)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(79-195)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)apps/desktop/src/components/settings/ai/stt/shared.tsx (1)
PROVIDERS(189-197)packages/utils/src/cn.ts (1)
cn(20-22)
apps/desktop/src/components/settings/ai/llm/index.tsx (1)
apps/desktop/src/components/settings/ai/llm/select.tsx (1)
SelectProviderAndModel(31-187)
apps/desktop/src/components/settings/ai/stt/configure.tsx (3)
apps/desktop/src/components/settings/ai/shared/index.tsx (1)
NonHyprProviderCard(79-195)apps/desktop/src/components/settings/ai/llm/shared.tsx (1)
PROVIDERS(23-96)apps/desktop/src/components/settings/ai/stt/shared.tsx (1)
PROVIDERS(189-197)
apps/desktop/src/components/main/sidebar/banner/component.tsx (1)
packages/utils/src/cn.ts (1)
cn(20-22)
🪛 Biome (2.1.2)
apps/desktop/src/components/settings/ai/llm/configure.tsx
[error] 60-60: Unexpected constant condition.
(lint/correctness/noConstantCondition)
apps/desktop/src/components/settings/ai/stt/configure.tsx
[error] 81-81: Unexpected constant condition.
(lint/correctness/noConstantCondition)
⏰ 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). (7)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: fmt
🔇 Additional comments (8)
apps/desktop/src/components/main/sidebar/banner/component.tsx (3)
16-16: LGTM: Padding refinement.The uniform padding simplifies the spacing and aligns with the UI polish objectives of this PR.
30-35: LGTM: Well-structured hover behavior.The dismiss button styling correctly uses
cnwith arrays and logical grouping per coding guidelines. The opacity cascade (hidden → group-hover semi-transparent → hover fully visible) provides good UX, and the!importantensures direct hover takes precedence.
37-37: LGTM: Icon sizing.The reduced icon size provides better visual balance within the
size-6button container.apps/desktop/src/components/settings/ai/stt/select.tsx (1)
27-27: LGTM! Clean refactor removing headerAction prop.The removal of the
headerActionprop and addition of the missing-model warning UI improves the UX by providing clear feedback when configuration is incomplete.Also applies to: 74-74, 207-214
apps/desktop/src/components/settings/ai/llm/index.tsx (1)
4-10: LGTM! Simplified component signature.Removing the
headerActionprop streamlines the component API and aligns with the new UI layout.apps/desktop/src/components/settings/ai/stt/index.tsx (1)
4-10: LGTM! Consistent with LLM refactor.The changes mirror the LLM component updates, maintaining consistency across STT and LLM configuration flows.
apps/desktop/src/components/settings/ai/llm/configure.tsx (1)
28-34: LGTM! Clean integration with shared NonHyprProviderCard.The updated usage of
NonHyprProviderCardwithproviderType,providers, andproviderContextprops properly delegates provider configuration to the shared component.apps/desktop/src/components/settings/ai/stt/configure.tsx (1)
43-49: LGTM! Consistent integration with shared provider card.The STT configuration now uses the shared
NonHyprProviderCardwith appropriateproviderType="stt", maintaining consistency with the LLM implementation.
…vider-selection-components
Overview