feat (desktop): settings refactor and search#818
Conversation
📝 WalkthroughWalkthroughAdds a centralized settings search/state system, many new SettingsView UI components (Account, Appearance, Behavior, Project/ScriptsEditor, Ringtones, Terminal, Sidebar), and a trpc Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI (ScriptsEditor)
participant TRPC as Electron TRPC (main)
participant FS as Filesystem
UI->>TRPC: fetchProjectConfig(projectId)
TRPC->>FS: ensure/read config.json
FS-->>TRPC: config content
TRPC-->>UI: config content
UI->>TRPC: updateConfig({ projectId, setup, teardown })
TRPC->>FS: read existing config.json
TRPC->>FS: write merged config.json
FS-->>TRPC: success / error
TRPC-->>UI: { success: true } or throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
# Conflicts: # apps/desktop/src/renderer/routes/_authenticated/settings/account/page.tsx # apps/desktop/src/renderer/routes/_authenticated/settings/terminal/page.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/renderer/routes/_authenticated/settings/project/$projectId/page.tsx (1)
37-44: Use a constant for the error message or explore accessing the error code instead of string matching.The check
error.message.includes("not found")relies on message content that could change or accidentally match unrelated errors. The tRPC router properly throwsTRPCError({ code: "NOT_FOUND", ... }), so investigate whethererror.codeorerror.data?.codeis accessible on the client side through the proxy client. If available, prefer checking the error code. Otherwise, extract the error message to a named constant (const NOT_FOUND_MESSAGE = "not found") to centralize the string and make changes easier to track across the codebase (this pattern is also used in workspace and dashboard pages).
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/config/config.ts`:
- Around line 136-142: The current mutation builds a new config object with only
setup and teardown and calls writeFileSync(configPath, ...), which will discard
any other existing fields; instead read and parse the existing config at
configPath (handle missing/invalid JSON), merge the existing object with the new
values from input.setup and input.teardown (overwriting those two keys but
preserving all other keys), then stringify and write the merged object back
using writeFileSync; reference the existing symbols configPath, the local config
variable, input.setup/input.teardown, and the writeFileSync call when making
this change.
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings/ThemeCard.tsx`:
- Around line 69-75: In ThemeCard.tsx the accentColors.map callback uses the
color string as the React key which can cause duplicate-key warnings if two
accent colors share the same hex; update the key to a stable unique value such
as combining the map index with the color (e.g., use index + '-' + color) or use
the map index alone in the accentColors.map inside ThemeCard to guarantee
uniqueness for each <div> rendered.
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/TerminalSettings/TerminalSettings.tsx`:
- Around line 579-586: The JSX in TerminalSettings.tsx uses a redundant ternary
for the div className where both branches are identical; simplify by replacing
the ternary expression with a single static className string (referencing the
component/JSX that checks showSessions and the boolean props showPersistence and
showLinkBehavior) or, if different styling was intended when showPersistence or
showLinkBehavior are true, change the ternary to return the distinct className
(e.g., add a border-top class) so the conditional actually affects the rendered
classes.
- Around line 757-763: The code is iterating over sessions (which can include
dead sessions) when marking terminals killed; change the loop to iterate over
aliveSessions instead so only currently alive sessions are marked via
markTerminalKilledByUser before calling killAllDaemonSessions.mutate(), leaving
the rest unchanged; update the onClick handler that currently calls
setConfirmKillAllOpen(false), the for...of loop over sessions,
markTerminalKilledByUser(session.sessionId), and killAllDaemonSessions.mutate()
to use aliveSessions in the loop.
🧹 Nitpick comments (18)
apps/desktop/src/renderer/stores/settings-state.ts (1)
76-90: Consider addinguseOpenSettingshook for symmetry.
useCloseSettingsis exposed butuseOpenSettingsis not. For API consistency, consider adding:export const useOpenSettings = () => useSettingsStore((state) => state.openSettings);apps/desktop/src/renderer/routes/_authenticated/settings/layout.tsx (1)
20-65: Consider consolidating section/path mappings into a lookup object.
SECTION_ORDER,getSectionFromPath, andgetPathFromSectionduplicate the section-to-path relationship. Per coding guidelines, prefer lookup objects over repeated conditionals:Proposed refactor using a unified lookup
-// Order of sections for auto-navigation -const SECTION_ORDER: SettingsSection[] = [ - "account", - "team", - "appearance", - "ringtones", - "keyboard", - "behavior", - "terminal", -]; - -// Map route paths to section names -function getSectionFromPath(pathname: string): SettingsSection | null { - if (pathname.includes("/settings/account")) return "account"; - if (pathname.includes("/settings/team")) return "team"; - if (pathname.includes("/settings/appearance")) return "appearance"; - if (pathname.includes("/settings/ringtones")) return "ringtones"; - if (pathname.includes("/settings/keyboard")) return "keyboard"; - if (pathname.includes("/settings/behavior")) return "behavior"; - if (pathname.includes("/settings/terminal")) return "terminal"; - if (pathname.includes("/settings/project")) return "project"; - if (pathname.includes("/settings/workspace")) return "workspace"; - return null; -} - -// Map section names to route paths -function getPathFromSection(section: SettingsSection): string { - switch (section) { - case "account": - return "/settings/account"; - case "team": - return "/settings/team"; - case "appearance": - return "/settings/appearance"; - case "ringtones": - return "/settings/ringtones"; - case "keyboard": - return "/settings/keyboard"; - case "behavior": - return "/settings/behavior"; - case "terminal": - return "/settings/terminal"; - default: - return "/settings/account"; - } -} +// Section to path mapping (order defines auto-navigation priority) +const SECTION_PATH_MAP: Record<SettingsSection, string> = { + account: "/settings/account", + team: "/settings/team", + appearance: "/settings/appearance", + ringtones: "/settings/ringtones", + keyboard: "/settings/keyboard", + behavior: "/settings/behavior", + terminal: "/settings/terminal", + project: "/settings/project", + workspace: "/settings/workspace", +}; + +// Order for auto-navigation (excludes project/workspace) +const SECTION_ORDER: SettingsSection[] = [ + "account", "team", "appearance", "ringtones", "keyboard", "behavior", "terminal", +]; + +function getSectionFromPath(pathname: string): SettingsSection | null { + for (const [section, path] of Object.entries(SECTION_PATH_MAP)) { + if (pathname.includes(path)) return section as SettingsSection; + } + return null; +} + +function getPathFromSection(section: SettingsSection): string { + return SECTION_PATH_MAP[section] ?? SECTION_PATH_MAP.account; +}apps/desktop/src/renderer/screens/main/components/SettingsView/RingtonesSettings/RingtonesSettings.tsx (4)
29-155: Consider extractingRingtoneCardto its own file.The coding guidelines specify "One component per file - do not create multi-component files."
RingtoneCardis a substantial component (120+ lines) that could be extracted toRingtoneCard.tsxwithin this folder.Suggested file structure
RingtonesSettings/ ├── index.ts ├── RingtonesSettings.tsx └── RingtoneCard.tsx
201-203: Use prefixed console logging with context pattern.Per coding guidelines, console logging should use a prefixed pattern like
[domain/operation] messagefor better traceability.Suggested fix
- console.error("Failed to stop ringtone:", error); + console.error("[RingtonesSettings/handleTogglePlay] Failed to stop ringtone:", error);Apply similar pattern to lines 211-213 and 222-224.
Also applies to: 211-213, 222-224
178-182: Error swallowed silently during cleanup.While cleanup errors are often acceptable to ignore, the empty catch block with a comment is borderline. Consider at least debug-level logging to aid troubleshooting.
Suggested fix
electronTrpcClient.ringtone.stop.mutate().catch(() => { - // Ignore errors during cleanup + // Cleanup errors are non-critical but log for debugging + console.debug("[RingtonesSettings/cleanup] Failed to stop ringtone preview"); });
215-225: Potential UI inconsistency:playingIdset before async operation completes.Setting
playingIdat line 216 before thepreview.mutate()call completes means the UI shows "playing" state even if the mutation fails. Although there's error handling that resetsplayingIdon failure, there's a brief window where UI state is inconsistent. Consider settingplayingIdonly after successful mutation.Suggested improvement
- // Play the new sound - setPlayingId(ringtone.id); - try { await electronTrpcClient.ringtone.preview.mutate({ filename: ringtone.filename, }); + // Set playing state only after successful mutation + setPlayingId(ringtone.id); } catch (error) { console.error("Failed to play ringtone:", error); - setPlayingId(null); + return; }apps/desktop/src/renderer/screens/main/components/SettingsView/ProjectSettings/ProjectSettings.tsx (1)
14-16: Consider showing a loading state instead of returningnull.When data is loading, rendering
nullcauses a flash of empty content. TheuseQueryhook providesisPending/isLoadingthat could be used to show a skeleton or loading indicator for better UX.Additionally, there's no explicit error handling if the query fails. Consider handling the error case as well.
💡 Suggested improvement
export function ProjectSettings({ projectId }: ProjectSettingsProps) { - const { data: project } = electronTrpc.projects.get.useQuery({ + const { data: project, isPending, error } = electronTrpc.projects.get.useQuery({ id: projectId, }); - if (!project) { - return null; + if (isPending) { + return <div className="p-6">Loading...</div>; + } + + if (error || !project) { + return <div className="p-6 text-muted-foreground">Failed to load project settings</div>; }apps/desktop/src/renderer/routes/_authenticated/settings/appearance/page.tsx (1)
11-21: Consistent implementation following the established pattern.The visibility filtering logic mirrors the account settings page exactly, which is good for maintainability.
Consider extracting this repeated pattern into a reusable hook if more settings pages follow this structure:
💡 Optional: Extract to a custom hook
// In a shared location, e.g., renderer/hooks/use-visible-settings-items.ts export function useVisibleSettingsItems(section: SettingsSection) { const searchQuery = useSettingsSearchQuery(); return useMemo(() => { if (!searchQuery) return null; return getMatchingItemsForSection(searchQuery, section).map( (item) => item.id, ); }, [searchQuery, section]); }Then each page becomes:
function AppearanceSettingsPage() { const visibleItems = useVisibleSettingsItems("appearance"); return <AppearanceSettings visibleItems={visibleItems} />; }apps/desktop/src/renderer/screens/main/components/SettingsView/BehaviorSettings/BehaviorSettings.tsx (1)
32-36: Consider logging the mutation error.The error is caught for rollback but not logged. Per coding guidelines, errors should not be swallowed silently—at minimum log them with context.
Suggested improvement
onError: (_err, _vars, context) => { + console.error("[BehaviorSettings/setConfirmOnQuit] mutation failed:", _err); if (context?.previous !== undefined) { utils.settings.getConfirmOnQuit.setData(undefined, context.previous); } },apps/desktop/src/renderer/screens/main/components/SettingsView/SettingsSidebar/ProjectsSettings.tsx (1)
31-35: Consider whether this effect is necessary.Per coding guidelines, prefer avoiding
useEffectunless absolutely necessary. This effect auto-expands all projects when data loads. An alternative would be to derive the expanded state or use a ref to track if initial expansion has occurred, but given this is syncing async data to local state, the current approach is acceptable.apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx (2)
33-37: Consider adding error handling for the sign-out mutation.The mutation has
onSuccessbut noonErrorhandler. A failed sign-out attempt would be silently ignored.Suggested improvement
const signOutMutation = electronTrpc.auth.signOut.useMutation({ onSuccess: () => toast.success("Signed out"), + onError: (error) => toast.error(`Failed to sign out: ${error.message}`), });
94-96: Simplify the onClick handler.The arrow function wrapper is unnecessary since
signOuttakes no arguments.Suggested change
-<Button variant="outline" onClick={() => signOut()}> +<Button variant="outline" onClick={signOut}> Sign Out </Button>apps/desktop/src/renderer/screens/main/components/SettingsView/ProjectSettings/ScriptsEditor/ScriptsEditor.tsx (3)
59-147: Multiple components in a single file violates coding guidelines.As per coding guidelines, there should be one component per file.
ScriptEntryRowandScriptsSectionshould be extracted to separate files.Consider extracting to:
ScriptEntryRow.tsxScriptsSection.tsxThis improves maintainability and follows the project conventions.
296-340: Duplicate import file handlers can be consolidated.
handleImportSetupFileandhandleImportTeardownFileare nearly identical. Consider extracting a shared helper.Suggested refactor
+const handleImportFile = useCallback( + async (setScripts: React.Dispatch<React.SetStateAction<ScriptEntry[]>>) => { + try { + const result = await window.ipcRenderer.invoke("open-file-dialog", { + filters: [{ name: "Scripts", extensions: ["sh", "bash", "zsh"] }], + }); + if (result && typeof result === "string") { + const content = await window.ipcRenderer.invoke("read-script-file", result); + if (content && typeof content === "string") { + setScripts((prev) => [...prev, { id: generateId(), content }]); + setHasChanges(true); + } + } + } catch (error) { + console.error("Failed to import file:", error); + // Consider: toast.error("Failed to import script file"); + } + }, + [], +); + +const handleImportSetupFile = useCallback( + () => handleImportFile(setSetupScripts), + [handleImportFile], +); + +const handleImportTeardownFile = useCallback( + () => handleImportFile(setTeardownScripts), + [handleImportFile], +);
101-103: Silent error handling - consider providing user feedback.When file reading fails, the error is logged but the user receives no indication. Consider showing a toast notification.
} catch (error) { - console.error("Failed to read script file:", error); + console.error("[ScriptsEditor/drop] Failed to read script file:", error); + // Consider adding: toast.error("Failed to read script file"); }apps/desktop/src/renderer/screens/main/components/SettingsView/TerminalSettings/TerminalSettings.tsx (3)
156-197: Use object parameters for functions with 2+ arguments.Per coding guidelines, functions with 2+ parameters should accept a single params object with named properties.
handleCellChange,handleCellBlur, andhandleCommandsChangeuse positional arguments.Suggested refactor
-const handleCellChange = ( - rowIndex: number, - column: PresetColumnKey, - value: string, -) => { +const handleCellChange = ({ + rowIndex, + column, + value, +}: { + rowIndex: number; + column: PresetColumnKey; + value: string; +}) => { setLocalPresets((prev) => prev.map((p, i) => (i === rowIndex ? { ...p, [column]: value } : p)), ); }; -const handleCellBlur = (rowIndex: number, column: PresetColumnKey) => { +const handleCellBlur = ({ + rowIndex, + column, +}: { + rowIndex: number; + column: PresetColumnKey; +}) => { // ... }; -const handleCommandsChange = (rowIndex: number, commands: string[]) => { +const handleCommandsChange = ({ + rowIndex, + commands, +}: { + rowIndex: number; + commands: string[]; +}) => { // ... };Note: This would require updating the
PresetRowcomponent's callback signatures as well.
303-307: Unsafe type assertion for link behavior value.The
valuefromonValueChangeis typed asstring, but it's cast toTerminalLinkBehaviorwithout validation. While theSelectcomponent currently only offers valid options, this could silently break if options change.Suggested validation
+const TERMINAL_LINK_BEHAVIORS = ["external-editor", "file-viewer"] as const; + const handleLinkBehaviorChange = (value: string) => { + if (!TERMINAL_LINK_BEHAVIORS.includes(value as TerminalLinkBehavior)) { + console.error("[TerminalSettings/handleLinkBehaviorChange] Invalid link behavior:", value); + return; + } setTerminalLinkBehavior.mutate({ behavior: value as TerminalLinkBehavior, }); };Alternatively, import
TERMINAL_LINK_BEHAVIORSfrom the schema if it's already exported.
345-350: Clarify the purpose of the delayed invalidation.The
setTimeoutwith a 300ms delay before invalidating the query is a workaround pattern. Consider adding a comment explaining why this delay is necessary (e.g., waiting for daemon state to settle).onSettled: () => { + // Delay invalidation to allow daemon process state to settle setTimeout(() => { utils.terminal.listDaemonSessions.invalidate(); }, 300); },
| {accentColors.map((color) => ( | ||
| <div | ||
| key={color} | ||
| className="h-2 w-5 rounded-sm" | ||
| style={{ backgroundColor: color }} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Potential duplicate key when accent colors share the same hex value.
Using color as the key could cause React warnings or rendering issues if a theme defines two accent colors with identical hex values. Consider using the array index or a combination of index and color.
Suggested fix
- {accentColors.map((color) => (
+ {accentColors.map((color, index) => (
<div
- key={color}
+ key={index}
className="h-2 w-5 rounded-sm"
style={{ backgroundColor: color }}
/>
))}📝 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.
| {accentColors.map((color) => ( | |
| <div | |
| key={color} | |
| className="h-2 w-5 rounded-sm" | |
| style={{ backgroundColor: color }} | |
| /> | |
| ))} | |
| {accentColors.map((color, index) => ( | |
| <div | |
| key={index} | |
| className="h-2 w-5 rounded-sm" | |
| style={{ backgroundColor: color }} | |
| /> | |
| ))} |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings/ThemeCard.tsx`
around lines 69 - 75, In ThemeCard.tsx the accentColors.map callback uses the
color string as the React key which can cause duplicate-key warnings if two
accent colors share the same hex; update the key to a stable unique value such
as combining the map index with the color (e.g., use index + '-' + color) or use
the map index alone in the accentColors.map inside ThemeCard to guarantee
uniqueness for each <div> rendered.
| {showSessions && ( | ||
| <div | ||
| className={ | ||
| showPersistence || showLinkBehavior | ||
| ? "rounded-md border border-border/60 p-4 space-y-3" | ||
| : "rounded-md border border-border/60 p-4 space-y-3" | ||
| } | ||
| > |
There was a problem hiding this comment.
Redundant conditional - both branches return the same className.
The ternary condition returns identical strings in both branches, making it dead code.
Suggested fix
{showSessions && (
<div
- className={
- showPersistence || showLinkBehavior
- ? "rounded-md border border-border/60 p-4 space-y-3"
- : "rounded-md border border-border/60 p-4 space-y-3"
- }
+ className="rounded-md border border-border/60 p-4 space-y-3"
>If visual separation was intended (like a border-top), the condition should produce different values.
📝 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.
| {showSessions && ( | |
| <div | |
| className={ | |
| showPersistence || showLinkBehavior | |
| ? "rounded-md border border-border/60 p-4 space-y-3" | |
| : "rounded-md border border-border/60 p-4 space-y-3" | |
| } | |
| > | |
| {showSessions && ( | |
| <div | |
| className="rounded-md border border-border/60 p-4 space-y-3" | |
| > |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/TerminalSettings/TerminalSettings.tsx`
around lines 579 - 586, The JSX in TerminalSettings.tsx uses a redundant ternary
for the div className where both branches are identical; simplify by replacing
the ternary expression with a single static className string (referencing the
component/JSX that checks showSessions and the boolean props showPersistence and
showLinkBehavior) or, if different styling was intended when showPersistence or
showLinkBehavior are true, change the ternary to return the distinct className
(e.g., add a border-top class) so the conditional actually affects the rendered
classes.
| onClick={() => { | ||
| setConfirmKillAllOpen(false); | ||
| for (const session of sessions) { | ||
| markTerminalKilledByUser(session.sessionId); | ||
| } | ||
| killAllDaemonSessions.mutate(); | ||
| }} |
There was a problem hiding this comment.
Iterating over sessions instead of aliveSessions when marking killed sessions.
sessions may include dead sessions. Marking already-dead sessions as "killed by user" is unnecessary. Consider using aliveSessions for consistency with the UI display.
Suggested fix
onClick={() => {
setConfirmKillAllOpen(false);
- for (const session of sessions) {
+ for (const session of aliveSessions) {
markTerminalKilledByUser(session.sessionId);
}
killAllDaemonSessions.mutate();
}}📝 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.
| onClick={() => { | |
| setConfirmKillAllOpen(false); | |
| for (const session of sessions) { | |
| markTerminalKilledByUser(session.sessionId); | |
| } | |
| killAllDaemonSessions.mutate(); | |
| }} | |
| onClick={() => { | |
| setConfirmKillAllOpen(false); | |
| for (const session of aliveSessions) { | |
| markTerminalKilledByUser(session.sessionId); | |
| } | |
| killAllDaemonSessions.mutate(); | |
| }} |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/TerminalSettings/TerminalSettings.tsx`
around lines 757 - 763, The code is iterating over sessions (which can include
dead sessions) when marking terminals killed; change the loop to iterate over
aliveSessions instead so only currently alive sessions are marked via
markTerminalKilledByUser before calling killAllDaemonSessions.mutate(), leaving
the rest unchanged; update the onClick handler that currently calls
setConfirmKillAllOpen(false), the for...of loop over sessions,
markTerminalKilledByUser(session.sessionId), and killAllDaemonSessions.mutate()
to use aliveSessions in the loop.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx`:
- Around line 31-35: The signOutMutation created by
electronTrpc.auth.signOut.useMutation currently only handles onSuccess and will
swallow errors; add an onError handler on signOutMutation that logs the error
with context (e.g., console.error or the app logger) and surfaces a user-facing
notification (e.g., toast.error("Failed to sign out: " + err.message)); update
the signOutMutation options to include onError and ensure the error object is
included in the log and message so failures are visible to developers and users.
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/ProjectSettings/ScriptsEditor/ScriptsEditor.tsx`:
- Around line 245-250: The updateConfigMutation created via
electronTrpc.config.updateConfig.useMutation lacks an onError handler so
failures are swallowed; add an onError callback to that mutation (alongside the
existing onSuccess) that receives the error and surfaces it (for example by
calling your app's notification/error UI and logging it), and restore any state
as needed (e.g., keep setHasChanges(true) if save failed) and optionally
invalidate or refetch via utils.config.getConfigContent.invalidate({ projectId
}) only on success—update the mutation definition for updateConfigMutation to
include this onError logic.
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts`:
- Around line 310-325: Both functions take multiple positional args and must be
refactored to single object parameters: change getMatchingItemsForSection(query:
string, section: SettingsSection) to getMatchingItemsForSection({ query, section
}: { query: string; section: SettingsSection }) and change isItemVisible(itemId:
SettingItemId, visibleItems?: SettingItemId[] | null) to isItemVisible({ itemId,
visibleItems }: { itemId: SettingItemId; visibleItems?: SettingItemId[] | null
}); update all call sites (the ~5 getMatchingItemsForSection callers and ~15+
isItemVisible callers) to pass an object with the named properties, update type
annotations/imports where these functions are declared/used, and run/adjust
tests or usages to ensure the new signatures compile; keep function names
unchanged so only call sites need updates.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/screens/main/components/SettingsView/ProjectSettings/ScriptsEditor/ScriptsEditor.tsx (4)
24-26: Consider reusing the existinggenerateIdutility.There's already a
generateIdfunction inapps/desktop/src/renderer/stores/tabs/utils.tsthat serves the same purpose. Consider importing and reusing it to avoid duplication.Suggested change
-function generateId(): string { - return Math.random().toString(36).substring(2, 9); -} +import { generateId } from "renderer/stores/tabs/utils";Note: The existing utility uses a prefix parameter (
generateId(prefix: string)), so you may need to adapt calls or create a wrapper.
102-104: Use prefixed console logging pattern.Per coding guidelines, console logging should follow the
[domain/operation] messagepattern for better traceability.Suggested change
- } catch (error) { - console.error("Failed to read script file:", error); - } + } catch (error) { + console.error("[ScriptsEditor/drop] Failed to read script file:", error); + }
60-151: Multiple components in one file violates coding guidelines.The coding guidelines specify "One component per file - do not create multi-component files." This file contains three components:
ScriptEntryRow,ScriptsSection, andScriptsEditor.Consider extracting
ScriptEntryRowandScriptsSectioninto separate files within this directory structure.Suggested structure:
ScriptsEditor/ ├── index.ts ├── ScriptsEditor.tsx ├── ScriptEntryRow.tsx └── ScriptsSection.tsxAlso applies to: 164-216, 218-421
300-341: Reduce duplication between import handlers and fix logging pattern.
handleImportSetupFileandhandleImportTeardownFileare nearly identical. Consider extracting shared logic and applying the prefixed console logging pattern.Suggested refactor
+const handleImportFile = useCallback( + async (setScripts: React.Dispatch<React.SetStateAction<ScriptEntry[]>>) => { + try { + const result = await window.ipcRenderer.invoke("open-file-dialog", { + filters: [{ name: "Scripts", extensions: ["sh", "bash", "zsh"] }], + }); + if (result && typeof result === "string") { + const content = await window.ipcRenderer.invoke( + "read-script-file", + result, + ); + if (content && typeof content === "string") { + setScripts((prev) => [...prev, { id: generateId(), content }]); + setHasChanges(true); + } + } + } catch (error) { + console.error("[ScriptsEditor/import] Failed to import file:", error); + } + }, + [], +); + +const handleImportSetupFile = useCallback( + () => handleImportFile(setSetupScripts), + [handleImportFile], +); + +const handleImportTeardownFile = useCallback( + () => handleImportFile(setTeardownScripts), + [handleImportFile], +);apps/desktop/src/renderer/routes/_authenticated/settings/presets/page.tsx (1)
7-9: Consider route-level redirect to avoid a render pass.
Using abeforeLoadredirect prevents a transient UI render and keeps navigation purely in the router layer.♻️ Suggested refactor
-import { createFileRoute, Navigate } from "@tanstack/react-router"; +import { createFileRoute, redirect } from "@tanstack/react-router"; export const Route = createFileRoute("/_authenticated/settings/presets/")({ - component: PresetsRedirect, + beforeLoad: () => { + throw redirect({ to: "/settings/terminal" }); + }, + component: () => null, }); - -// Presets have been merged into Terminal settings -function PresetsRedirect() { - return <Navigate to="/settings/terminal" replace />; -}
| const { data: session, isPending: isLoading } = authClient.useSession(); | ||
| const user = session?.user; | ||
| const signOutMutation = electronTrpc.auth.signOut.useMutation({ | ||
| onSuccess: () => toast.success("Signed out"), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
fd -t f "AccountSettings.tsx" --path "*apps/desktop*"Repository: superset-sh/superset
Length of output: 297
🏁 Script executed:
# If found, read the file to verify the code
cat -n apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx | head -50Repository: superset-sh/superset
Length of output: 1829
🏁 Script executed:
# Search for other mutation patterns in the renderer to see how errors are handled elsewhere
rg "useMutation" apps/desktop/src/renderer --type tsx -A 3 -B 1 | head -100Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for useMutation with onError patterns in renderer
rg "useMutation\(" apps/desktop/src/renderer -A 4 | head -80Repository: superset-sh/superset
Length of output: 10331
🏁 Script executed:
# Search for console.error patterns with context prefix in the file or similar files
rg "\[auth/" apps/desktop/src/renderer --type ts | head -20Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Look for logging patterns in the renderer directory
rg "console\." apps/desktop/src/renderer/screens/main --type ts -B 1 -A 1 | head -60Repository: superset-sh/superset
Length of output: 6512
Handle sign-out errors (don't fail silently).
The signOutMutation lacks an onError handler, allowing sign-out failures to pass unnoticed. This violates the guideline "Never swallow errors silently." Add an onError to log with context and notify the user.
Suggested fix
const signOutMutation = electronTrpc.auth.signOut.useMutation({
onSuccess: () => toast.success("Signed out"),
+ onError: (error) => {
+ console.error("[auth/signOut] Sign out failed", error);
+ toast.error("Sign out failed");
+ },
});📝 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.
| const { data: session, isPending: isLoading } = authClient.useSession(); | |
| const user = session?.user; | |
| const signOutMutation = electronTrpc.auth.signOut.useMutation({ | |
| onSuccess: () => toast.success("Signed out"), | |
| }); | |
| const { data: session, isPending: isLoading } = authClient.useSession(); | |
| const user = session?.user; | |
| const signOutMutation = electronTrpc.auth.signOut.useMutation({ | |
| onSuccess: () => toast.success("Signed out"), | |
| onError: (error) => { | |
| console.error("[auth/signOut] Sign out failed", error); | |
| toast.error("Sign out failed"); | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/AccountSettings/AccountSettings.tsx`
around lines 31 - 35, The signOutMutation created by
electronTrpc.auth.signOut.useMutation currently only handles onSuccess and will
swallow errors; add an onError handler on signOutMutation that logs the error
with context (e.g., console.error or the app logger) and surfaces a user-facing
notification (e.g., toast.error("Failed to sign out: " + err.message)); update
the signOutMutation options to include onError and ensure the error object is
included in the log and message so failures are visible to developers and users.
| const updateConfigMutation = electronTrpc.config.updateConfig.useMutation({ | ||
| onSuccess: () => { | ||
| setHasChanges(false); | ||
| utils.config.getConfigContent.invalidate({ projectId }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Add error handling for the mutation.
The mutation lacks an onError handler. Per coding guidelines, errors should never be swallowed silently. Users won't receive feedback if the save operation fails.
Suggested change
const updateConfigMutation = electronTrpc.config.updateConfig.useMutation({
onSuccess: () => {
setHasChanges(false);
utils.config.getConfigContent.invalidate({ projectId });
},
+ onError: (error) => {
+ console.error("[ScriptsEditor/save] Failed to save config:", error);
+ // Consider adding user-facing error notification here
+ },
});📝 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.
| const updateConfigMutation = electronTrpc.config.updateConfig.useMutation({ | |
| onSuccess: () => { | |
| setHasChanges(false); | |
| utils.config.getConfigContent.invalidate({ projectId }); | |
| }, | |
| }); | |
| const updateConfigMutation = electronTrpc.config.updateConfig.useMutation({ | |
| onSuccess: () => { | |
| setHasChanges(false); | |
| utils.config.getConfigContent.invalidate({ projectId }); | |
| }, | |
| onError: (error) => { | |
| console.error("[ScriptsEditor/save] Failed to save config:", error); | |
| // Consider adding user-facing error notification here | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/ProjectSettings/ScriptsEditor/ScriptsEditor.tsx`
around lines 245 - 250, The updateConfigMutation created via
electronTrpc.config.updateConfig.useMutation lacks an onError handler so
failures are swallowed; add an onError callback to that mutation (alongside the
existing onSuccess) that receives the error and surfaces it (for example by
calling your app's notification/error UI and logging it), and restore any state
as needed (e.g., keep setHasChanges(true) if save failed) and optionally
invalidate or refetch via utils.config.getConfigContent.invalidate({ projectId
}) only on success—update the mutation definition for updateConfigMutation to
include this onError logic.
| export function getMatchingItemsForSection( | ||
| query: string, | ||
| section: SettingsSection, | ||
| ): SettingsItem[] { | ||
| return searchSettings(query).filter((item) => item.section === section); | ||
| } | ||
|
|
||
| /** | ||
| * Helper to check if an item should be visible based on search results. | ||
| * Returns true if no search filter is active OR if the item is in the visible list. | ||
| */ | ||
| export function isItemVisible( | ||
| itemId: SettingItemId, | ||
| visibleItems: SettingItemId[] | null | undefined, | ||
| ): boolean { | ||
| return !visibleItems || visibleItems.includes(itemId); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, locate and verify the file exists
find . -path "*/apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts" -type fRepository: superset-sh/superset
Length of output: 184
🏁 Script executed:
# Check the lines 310-325 in the file to confirm the functions
if [ -f "apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts" ]; then
sed -n '310,325p' "apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts"
fiRepository: superset-sh/superset
Length of output: 588
🏁 Script executed:
# Find all usages of getMatchingItemsForSection in the codebase
rg "getMatchingItemsForSection" --type ts --type tsx -nRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find all usages of isItemVisible in the codebase
rg "isItemVisible" --type ts --type tsx -nRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find all usages of getMatchingItemsForSection
rg "getMatchingItemsForSection" -n --max-count 20Repository: superset-sh/superset
Length of output: 1844
🏁 Script executed:
# Find all usages of isItemVisible
rg "isItemVisible" -n --max-count 20Repository: superset-sh/superset
Length of output: 2640
🏁 Script executed:
# Let's look at a few actual call sites to understand the usage pattern
sed -n '29,37p' "apps/desktop/src/renderer/screens/main/components/SettingsView/AppearanceSettings/AppearanceSettings.tsx"Repository: superset-sh/superset
Length of output: 290
🏁 Script executed:
# Check one of the getMatchingItemsForSection call sites
sed -n '16,16p' "apps/desktop/src/renderer/routes/_authenticated/settings/appearance/page.tsx"Repository: superset-sh/superset
Length of output: 131
🏁 Script executed:
# Check another isItemVisible call site
sed -n '108,124p' "apps/desktop/src/renderer/screens/main/components/SettingsView/TerminalSettings/TerminalSettings.tsx"Repository: superset-sh/superset
Length of output: 485
Refactor multi-parameter functions to use object parameters.
Both functions violate the guideline requiring object parameters for 2+ parameters. Update callers accordingly:
getMatchingItemsForSectionhas 5 call sitesisItemVisiblehas 15+ call sites
♻️ Proposed refactor
-export function getMatchingItemsForSection(
- query: string,
- section: SettingsSection,
-): SettingsItem[] {
- return searchSettings(query).filter((item) => item.section === section);
-}
+export function getMatchingItemsForSection({
+ query,
+ section,
+}: {
+ query: string;
+ section: SettingsSection;
+}): SettingsItem[] {
+ return searchSettings(query).filter((item) => item.section === section);
+}
@@
-export function isItemVisible(
- itemId: SettingItemId,
- visibleItems: SettingItemId[] | null | undefined,
-): boolean {
- return !visibleItems || visibleItems.includes(itemId);
-}
+export function isItemVisible({
+ itemId,
+ visibleItems,
+}: {
+ itemId: SettingItemId;
+ visibleItems?: SettingItemId[] | null;
+}): boolean {
+ return !visibleItems || visibleItems.includes(itemId);
+}🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/SettingsView/settings-search.ts`
around lines 310 - 325, Both functions take multiple positional args and must be
refactored to single object parameters: change getMatchingItemsForSection(query:
string, section: SettingsSection) to getMatchingItemsForSection({ query, section
}: { query: string; section: SettingsSection }) and change isItemVisible(itemId:
SettingItemId, visibleItems?: SettingItemId[] | null) to isItemVisible({ itemId,
visibleItems }: { itemId: SettingItemId; visibleItems?: SettingItemId[] | null
}); update all call sites (the ~5 getMatchingItemsForSection callers and ~15+
isItemVisible callers) to pass an object with the named properties, update type
annotations/imports where these functions are declared/used, and run/adjust
tests or usages to ensure the new signatures compile; keep function names
unchanged so only call sites need updates.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
UI/UX Improvements
✏️ Tip: You can customize this high-level summary in your review settings.