feature (desktop): settings clean up #545
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request adds TRPC procedures for retrieving Electron app version and checking for updates, and introduces a client-side settings search system that filters settings sections and items. Settings components are refactored to support conditional rendering based on visibility flags passed through the UI hierarchy. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Settings UI<br/>(Sidebar)
participant Search as Search Module<br/>(settings-search.ts)
participant Content as Settings Content<br/>(SettingsContent)
participant Components as Settings Components<br/>(Account/Appearance/etc.)
User->>UI: Types search query
UI->>Search: searchSettings(query)
Search-->>UI: SettingsItem[] results
UI->>Search: getMatchCountBySection(query)
Search-->>UI: Record<Section, count>
UI->>UI: Update activeSection if<br/>current has no matches
UI->>Content: Pass searchQuery +<br/>activeSection
Content->>Search: getMatchingItemsForSection<br/>(query, section)
Search-->>Content: Filtered SettingsItem[]
Content->>Content: Compute matchingIds
Content->>Components: Pass visibleItems=matchingIds
rect rgb(230, 245, 250)
note over Components: Conditional Rendering
Components->>Components: Render only items<br/>in visibleItems
end
Components-->>User: Display filtered<br/>settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
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 |
AviPeltz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsx (1)
38-48: LGTM!The search input implementation follows proper controlled component patterns with good visual design.
Consider adding an
aria-labelattribute for improved accessibility:🔎 Optional accessibility enhancement
<input type="text" placeholder="Search settings..." + aria-label="Search settings" value={searchQuery} onChange={(e) => onSearchChange(e.target.value)} className="w-full h-8 pl-8 pr-3 text-sm bg-accent/50 rounded-md border-0 outline-none focus:ring-1 focus:ring-ring placeholder:text-muted-foreground" />apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx (1)
11-18: Consider renaming for clarity.The constant name
SECTION_ORDERdoesn't clearly indicate it only covers general sections (excluding dynamic project/workspace sections). Consider renaming toGENERAL_SECTION_ORDERor adding a comment explaining the scope.🔎 Suggested improvement
-// Order of sections in the sidebar -const SECTION_ORDER = [ +// Order of general sections in the sidebar (excludes dynamic project/workspace sections) +const GENERAL_SECTION_ORDER = [ "account", "appearance", "ringtones", "keyboard", "presets", "behavior", ] as const;And update the reference on line 34:
- const firstMatch = SECTION_ORDER.find( + const firstMatch = GENERAL_SECTION_ORDER.find( (section) => (matchCounts[section] ?? 0) > 0, );apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsx (1)
33-52: LGTM!The filtering logic is well-implemented with sensible UX: when a project name matches, all its workspaces are shown; when only workspace names match, only those workspaces are displayed.
Consider trimming the search query to avoid whitespace-only matches causing confusion:
🔎 Optional enhancement
// Filter groups based on search query + const trimmedQuery = searchQuery.trim(); const filteredGroups = groups .map((group) => { const projectMatches = group.project.name .toLowerCase() - .includes(searchQuery.toLowerCase()); + .includes(trimmedQuery.toLowerCase()); const matchingWorkspaces = group.workspaces.filter((ws) => - ws.name.toLowerCase().includes(searchQuery.toLowerCase()), + ws.name.toLowerCase().includes(trimmedQuery.toLowerCase()), ); // Include if project name matches or any workspace matches if (projectMatches || matchingWorkspaces.length > 0) { return { ...group, workspaces: projectMatches ? group.workspaces : matchingWorkspaces, }; } return null; }) .filter(Boolean) as typeof groups;apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
208-225: Timer not cleared when preview mutation fails.If
trpcClient.ringtone.preview.mutate()throws at line 212,playingIdis reset tonull(line 217), but the timer set at lines 222-225 continues running. SincesetPlayingIdhappens before thetryblock containing the timer setup, this specific scenario is actually safe. However, the timer is set unconditionally after the try-catch, meaning it will run even after an error resetsplayingId.Consider moving the timer setup inside the try block after the successful mutation:
🔎 Proposed fix
// Play the new sound setPlayingId(ringtone.id); try { await trpcClient.ringtone.preview.mutate({ filename: ringtone.filename, }); + + // Auto-reset after the ringtone's actual duration (with 500ms buffer) + const durationMs = ((ringtone.duration ?? 5) + 0.5) * 1000; + previewTimerRef.current = setTimeout(() => { + setPlayingId((current) => (current === ringtone.id ? null : current)); + previewTimerRef.current = null; + }, durationMs); } catch (error) { console.error("Failed to play ringtone:", error); setPlayingId(null); } - - // Auto-reset after the ringtone's actual duration (with 500ms buffer) - const durationMs = ((ringtone.duration ?? 5) + 0.5) * 1000; - previewTimerRef.current = setTimeout(() => { - setPlayingId((current) => (current === ringtone.id ? null : current)); - previewTimerRef.current = null; - }, durationMs); },apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (1)
89-98: Missing user feedback on clipboard copy.The copy-to-clipboard action at line 93 doesn't provide any feedback to the user. Consider showing a toast notification on success (toast is already imported).
🔎 Proposed fix
<button type="button" className="flex items-center gap-2 text-sm font-mono hover:text-foreground text-muted-foreground" onClick={() => { - navigator.clipboard.writeText(version ?? ""); + navigator.clipboard.writeText(version ?? ""); + toast.success("Version copied to clipboard"); }} >apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts (1)
135-146: Type assertion may produce incomplete record.The
countsobject will only contain keys for sections that have matches. Casting toRecord<SettingsSection, number>implies all sections are present, but sections with zero matches will beundefinedwhen accessed.Consider initializing with all sections set to 0, or changing the return type to
Partial<Record<SettingsSection, number>>.🔎 Proposed fix (Option 1: Initialize all sections)
export function getMatchCountBySection( query: string, ): Record<SettingsSection, number> { const matches = searchSettings(query); - const counts: Record<string, number> = {}; + const counts: Record<SettingsSection, number> = { + account: 0, + appearance: 0, + ringtones: 0, + keyboard: 0, + presets: 0, + behavior: 0, + }; for (const item of matches) { counts[item.section] = (counts[item.section] || 0) + 1; } - return counts as Record<SettingsSection, number>; + return counts; }🔎 Proposed fix (Option 2: Use Partial type)
export function getMatchCountBySection( query: string, -): Record<SettingsSection, number> { +): Partial<Record<SettingsSection, number>> { const matches = searchSettings(query); const counts: Record<string, number> = {}; for (const item of matches) { counts[item.section] = (counts[item.section] || 0) + 1; } - return counts as Record<SettingsSection, number>; + return counts; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use type safety and avoid
anytypes unless absolutely necessary in TypeScript files
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/**/src/**/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Structure component folders with one component per file using format
ComponentName/ComponentName.tsxwithindex.tsbarrel export
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Move Node.js functionality needed in Electron renderer to
src/main/lib/and communicate via IPC
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/lib/trpc/routers/auto-update/index.tsapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
apps/desktop/src/{shared/ipc-channels.ts,main/**/*ipcs.ts,renderer/**/*.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Define all Electron IPC channel types in
apps/desktop/src/shared/ipc-channels.tsbefore implementing handlers
Files:
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/index.tsxapps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
📚 Learning: 2025-12-28T01:56:39.031Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T01:56:39.031Z
Learning: Applies to apps/desktop/src/**/*.{ts,tsx} : Move Node.js functionality needed in Electron renderer to `src/main/lib/` and communicate via IPC
Applied to files:
apps/desktop/src/lib/trpc/routers/auto-update/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For Electron interprocess communication, ALWAYS use tRPC as defined in `src/lib/trpc`
Applied to files:
apps/desktop/src/lib/trpc/routers/auto-update/index.ts
📚 Learning: 2025-12-21T04:39:28.543Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-12-21T04:39:28.543Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from `trpc/server/observable` instead of async generators, as the library explicitly checks `isObservable(result)` and throws an error otherwise
Applied to files:
apps/desktop/src/lib/trpc/routers/auto-update/index.ts
🧬 Code graph analysis (7)
apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsx (2)
apps/desktop/src/renderer/screens/main/components/SettingsView/ThemeCard.tsx (1)
ThemeCard(11-95)apps/desktop/src/renderer/stores/markdown-preferences/store.ts (1)
MarkdownStyle(4-4)
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (3)
packages/ui/src/components/ui/sonner.tsx (1)
toast(40-40)apps/desktop/src/main/lib/auth/auth-service.ts (1)
signOut(344-347)apps/desktop/src/shared/auto-update.ts (1)
AUTO_UPDATE_STATUS(1-7)
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsx (3)
apps/desktop/src/renderer/stores/app-state.ts (2)
SettingsSection(5-13)useCloseSettings(84-85)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (1)
GeneralSettings(115-160)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsx (1)
ProjectsSettings(14-159)
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (2)
packages/ui/src/components/ui/label.tsx (1)
Label(24-24)packages/ui/src/components/ui/switch.tsx (1)
Switch(31-31)
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (1)
apps/desktop/src/renderer/stores/ringtone/store.ts (1)
AVAILABLE_RINGTONES(12-12)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (1)
apps/desktop/src/main/lib/auto-updater.ts (2)
checkForUpdates(63-73)checkForUpdatesInteractive(75-121)
apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx (2)
apps/desktop/src/renderer/stores/app-state.ts (2)
useSettingsSection(79-80)useSetSettingsSection(81-82)apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (1)
SettingsContent(17-46)
⏰ 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: Build
🔇 Additional comments (23)
apps/desktop/src/lib/trpc/routers/auto-update/index.ts (2)
39-41: LGTM!The
getVersionquery is correctly implemented and follows tRPC patterns for exposing Electron app version to the renderer process.
47-49: LGTM!The mutation correctly invokes
checkForUpdatesInteractive()as a fire-and-forget operation. Since the function handles UI dialogs internally and emits status updates via the event emitter (consumed by thesubscribeprocedure), no return value or await is needed.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsx (2)
10-11: LGTM!The new props for search functionality are properly typed and follow React conventions for controlled input patterns.
21-21: LGTM!Good optimization—
matchCountsis only computed when a search query exists, avoiding unnecessary work during normal browsing.apps/desktop/src/renderer/screens/main/components/SettingsView/index.tsx (2)
26-41: Verify auto-selection behavior for project/workspace sections.The auto-selection logic only considers sections in
SECTION_ORDER(general sections). When the active section is "project" or "workspace" and a search has no matches in those sections, the user won't be auto-navigated to a general section with matches.This might be intentional to keep project/workspace navigation separate, but consider whether users would expect to be redirected to the first matching general section in this scenario.
If auto-navigation from project/workspace to general sections is desired during search, the logic should check if
activeSectionis in the general sections list before attempting to find a match:🔎 Potential enhancement
useEffect(() => { if (!searchQuery) return; const matchCounts = getMatchCountBySection(searchQuery); const currentHasMatches = (matchCounts[activeSection] ?? 0) > 0; if (!currentHasMatches) { + // For project/workspace sections, also check if we should switch to general sections // Find first section with matches - const firstMatch = SECTION_ORDER.find( + const firstMatch = GENERAL_SECTION_ORDER.find( (section) => (matchCounts[section] ?? 0) > 0, ); if (firstMatch) { setActiveSection(firstMatch); } } }, [searchQuery, activeSection, setActiveSection]);
48-55: LGTM!Props are correctly passed to child components, enabling the search functionality throughout the settings hierarchy.
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsContent.tsx (2)
14-24: LGTM!The search query integration is clean and efficient. The conditional computation of
matchingItemsonly when a search query exists avoids unnecessary processing.
28-43: LGTM!The selective passing of
visibleItemsto specific settings components is appropriate. Project/workspace sections handle their own filtering viaProjectsSettings, while keyboard and presets sections may not require item-level filtering.apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings.tsx (2)
23-28: LGTM!The visibility logic is well-structured with clear, descriptive boolean flags. The pattern of checking
showAllfirst, then specific item IDs, provides good readability and maintainability.
48-99: LGTM!The conditional rendering is cleanly implemented. Each section is appropriately gated by its visibility flag while preserving the original content and functionality.
apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings.tsx (1)
5-69: LGTM!The implementation follows the same clean pattern established in other settings components. The conditional rendering for the confirm-quit setting is properly implemented with appropriate visibility flags.
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsx (1)
11-17: LGTM!The
searchQueryprop is properly typed and follows the same pattern as other settings components.apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx (2)
120-127: LGTM!The filtering logic efficiently shows only sections with matches during search and provides a clean empty state by returning null when no sections match.
135-156: LGTM!The count badge implementation provides helpful visual feedback during search, showing users how many matching items exist in each section. The conditional rendering ensures badges only appear when relevant.
apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (3)
152-159: LGTM!The visibility prop interface and conditional rendering logic are well-structured. The
showAllfallback whenvisibleItemsis null/undefined ensures backward compatibility.
165-176: LGTM!Good cleanup pattern—clearing the timer and stopping any in-progress ringtone preview on unmount. Using tRPC for the stop mutation aligns with the coding guidelines for Electron IPC.
248-274: LGTM!The conditional rendering with Fragment wrapper cleanly shows/hides the notification section based on search visibility. The tip text is appropriately grouped with the section it describes.
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (2)
9-17: LGTM!Clean prop interface for visibility filtering. The visibility flags follow the same pattern as other settings components.
18-27: LGTM!Good use of tRPC hooks for fetching version and update status. The
isCheckingderived state is cleanly computed from the query result.apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts (4)
3-9: LGTM!Well-defined interface for searchable settings items with appropriate metadata fields.
11-121: LGTM!Comprehensive settings catalog with clear organization by section. Keywords are well-chosen for discoverability.
123-133: LGTM!Clean search implementation with case-insensitive matching across title, description, and keywords. Returning all items on empty query is the expected UX behavior.
148-153: LGTM!Clean composition filtering search results by section.
| <p className="text-sm text-muted-foreground"> | ||
| {isChecking ? "Checking..." : "Up to date"} | ||
| </p> |
There was a problem hiding this comment.
Status text doesn't reflect actual update state.
The text always shows "Up to date" when not checking, but updateStatus can have other states like READY (update available) or ERROR. This could mislead users.
🔎 Proposed fix
<p className="text-sm text-muted-foreground">
- {isChecking ? "Checking..." : "Up to date"}
+ {updateStatus?.status === AUTO_UPDATE_STATUS.CHECKING
+ ? "Checking..."
+ : updateStatus?.status === AUTO_UPDATE_STATUS.READY
+ ? "Update available"
+ : updateStatus?.status === AUTO_UPDATE_STATUS.ERROR
+ ? "Check failed"
+ : updateStatus?.status === AUTO_UPDATE_STATUS.DOWNLOADING
+ ? "Downloading..."
+ : "Up to date"}
</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <p className="text-sm text-muted-foreground"> | |
| {isChecking ? "Checking..." : "Up to date"} | |
| </p> | |
| <p className="text-sm text-muted-foreground"> | |
| {updateStatus?.status === AUTO_UPDATE_STATUS.CHECKING | |
| ? "Checking..." | |
| : updateStatus?.status === AUTO_UPDATE_STATUS.READY | |
| ? "Update available" | |
| : updateStatus?.status === AUTO_UPDATE_STATUS.ERROR | |
| ? "Check failed" | |
| : updateStatus?.status === AUTO_UPDATE_STATUS.DOWNLOADING | |
| ? "Downloading..." | |
| : "Up to date"} | |
| </p> |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx
around lines 109-111, the status text currently shows "Checking..." only when
isChecking is true and otherwise always "Up to date", which ignores updateStatus
values like READY or ERROR; change the render to derive the displayed string
from updateStatus when not checking (e.g., map READY -> "Update available",
ERROR -> "Update error", IDLE or UPDATED -> "Up to date") or use a switch/lookup
to produce the correct message, keeping "Checking..." when isChecking is true
and ensure any unexpected updateStatus falls back to a sensible default.
| keywords: string[]; | ||
| }[] = [ | ||
| { | ||
| id: "account", | ||
| label: "Account", | ||
| icon: <HiOutlineUser className="h-4 w-4" />, | ||
| keywords: [ | ||
| "profile", | ||
| "email", | ||
| "name", | ||
| "sign out", | ||
| "logout", | ||
| "version", | ||
| "update", | ||
| "check for updates", | ||
| ], | ||
| }, | ||
| { | ||
| id: "appearance", | ||
| label: "Appearance", | ||
| icon: <HiOutlinePaintBrush className="h-4 w-4" />, | ||
| keywords: [ | ||
| "theme", | ||
| "dark", | ||
| "light", | ||
| "colors", | ||
| "custom themes", | ||
| "markdown", | ||
| "style", | ||
| "tufte", | ||
| ], | ||
| }, | ||
| { | ||
| id: "ringtones", | ||
| label: "Ringtones", | ||
| icon: <HiOutlineBell className="h-4 w-4" />, | ||
| keywords: [ | ||
| "notification", | ||
| "sound", | ||
| "audio", | ||
| "alert", | ||
| "bell", | ||
| "tone", | ||
| "completed tasks", | ||
| ], | ||
| }, | ||
| { | ||
| id: "keyboard", | ||
| label: "Keyboard Shortcuts", | ||
| icon: <HiOutlineCommandLine className="h-4 w-4" />, | ||
| keywords: [ | ||
| "hotkeys", | ||
| "keys", | ||
| "bindings", | ||
| "terminal", | ||
| "workspace", | ||
| "window", | ||
| "layout", | ||
| "commands", | ||
| ], | ||
| }, | ||
| { | ||
| id: "presets", | ||
| label: "Presets", | ||
| icon: <HiOutlineCog6Tooth className="h-4 w-4" />, | ||
| keywords: [ | ||
| "terminal", | ||
| "commands", | ||
| "claude", | ||
| "codex", | ||
| "gemini", | ||
| "cursor", | ||
| "opencode", | ||
| "launch", | ||
| "agent", | ||
| ], | ||
| }, | ||
| { | ||
| id: "behavior", | ||
| label: "Behavior", | ||
| icon: <HiOutlineAdjustmentsHorizontal className="h-4 w-4" />, | ||
| keywords: [ | ||
| "confirm", | ||
| "quit", | ||
| "quitting", | ||
| "dialog", | ||
| "preferences", | ||
| "exit", | ||
| "close", | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "keywords" apps/desktop/src/renderer/screens/main/components/SettingsView/Repository: superset-sh/superset
Length of output: 2453
🏁 Script executed:
# Find the settings-search file
find apps/desktop/src -name "*settings-search*" -o -name "*settingsSearch*"Repository: superset-sh/superset
Length of output: 145
🏁 Script executed:
# Check the structure and see where keywords are referenced
rg -A 3 "keywords" apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.tsRepository: superset-sh/superset
Length of output: 1025
🏁 Script executed:
head -30 apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxRepository: superset-sh/superset
Length of output: 749
🏁 Script executed:
# Check the end of GeneralSettings.tsx to see what's exported
tail -20 apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxRepository: superset-sh/superset
Length of output: 676
🏁 Script executed:
# Search for imports of GeneralSettings to see how it's used
rg -n "GeneralSettings\|from.*GeneralSettings" apps/desktop/src/renderer/screens/main/components/SettingsView/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the full line count of GeneralSettings.tsx
wc -l apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsxRepository: superset-sh/superset
Length of output: 166
🏁 Script executed:
# Check how searchSettings is used and if it references GeneralSettings keywords
rg -B 5 -A 10 "searchSettings\|useSearch" apps/desktop/src/renderer/screens/main/components/SettingsView/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check the parent component that uses GeneralSettings
find apps/desktop/src/renderer/screens/main/components/SettingsView -name "*.tsx" | head -10 | xargs -I {} basename {}Repository: superset-sh/superset
Length of output: 269
🏁 Script executed:
# Look at the settings store to understand SettingsSection type
rg -A 10 "type SettingsSection\|export.*SettingsSection" apps/desktop/src/renderer/stores/Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Check if keywords from GeneralSettings are used in the search or if they're just metadata
rg -n "GENERAL_SECTIONS" apps/desktop/src/renderer/screens/main/components/SettingsView/Repository: superset-sh/superset
Length of output: 496
🏁 Script executed:
# Check the SettingsSidebar component to see how it uses GeneralSettings and matchCounts
cat -n apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/SettingsSidebar.tsx | head -80Repository: superset-sh/superset
Length of output: 2539
🏁 Script executed:
# Look for where matchCounts is calculated or passed
rg -B 5 -A 5 "matchCounts" apps/desktop/src/renderer/screens/main/components/SettingsView/ | head -50Repository: superset-sh/superset
Length of output: 5585
🏁 Script executed:
# Check the main SettingsView component
cat -n apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsView.tsx | head -100Repository: superset-sh/superset
Length of output: 175
🏁 Script executed:
# Check the getMatchCountBySection function to see if it uses GENERAL_SECTIONS
cat -n apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts | head -150Repository: superset-sh/superset
Length of output: 4374
🏁 Script executed:
# Look for any usage of GENERAL_SECTIONS keywords in the codebase
rg "GENERAL_SECTIONS" apps/desktop/src/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 1455
🏁 Script executed:
# Check if keywords from GENERAL_SECTIONS are ever read or used
rg "\.keywords" apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ -B 2 -A 2Repository: superset-sh/superset
Length of output: 46
Remove unused keywords from GENERAL_SECTIONS or clarify their purpose.
The keywords added to GENERAL_SECTIONS in GeneralSettings.tsx are never consumed. The search functionality in settings-search.ts uses SETTINGS_ITEMS.keywords exclusively—not GENERAL_SECTIONS.keywords. The searchSettings() and getMatchCountBySection() functions only reference items from SETTINGS_ITEMS and their keywords fields. The keywords you added to each section definition are dead code with no impact on search coverage.
Either remove them if they're unnecessary, or if the intent is to use section-level keywords for search, update the search implementation to consume them.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/GeneralSettings.tsx
around lines 22–111, the keywords arrays on GENERAL_SECTIONS are never used by
the search code; remove the dead section-level keywords or wire them into the
search. Either delete the keywords fields and adjust the type to remove unused
props, or update the search implementation (settings-search.ts) to incorporate
section keywords when building the searchable index — e.g. merge each
section.keywords into the corresponding SETTINGS_ITEMS[key].keywords or have
search functions check section.keywords as well — and update types/tests
accordingly.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.