fix(settings): redesign settings UI, remove dead code (LAC-2628) - #473
fix(settings): redesign settings UI, remove dead code (LAC-2628)#473lacymorrow wants to merge 1 commit into
Conversation
…s, clean up layout (LAC-2628) - Remove duplicate Whisper model card in VoiceSettings (106 lines of copy-paste) - Remove duplicate Companion Mode from GeneralSettings (keep single source in Tools) - Delete dead NotificationSettings component (485 lines, never rendered) - Delete unused SettingsField and SettingsSection wrapper components - Consolidate duplicate types — re-export from canonical src/types/settings.ts - Remove dead navigation props (onNavigateToDevTools/Chat/Permissions) - Replace browser confirm() dialog with inline confirmation UI in reset - Compact sidebar layout (w-48 vs w-64, smaller icons, no descriptions) - Replace Brain/Sparkles icons and semantic color rainbow (AI slop cleanup) - Replace inline SVG with Lucide MousePointer2 in ToolsSettings - Remove commented-out cloud status handler in NetworkSettings - Condense shortcuts tips and cloud walkthrough to minimal prose Net: -920 lines across 12 files. TypeScript clean, 26/26 tests pass.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and simplifies the settings interface by removing several unused sections and components, including the notification settings, companion mode, and Whisper speech-to-text model configuration. It also updates the layout and styling of the settings window, replaces the native browser confirmation dialog in the advanced settings with an inline UI confirmation, and cleans up various icons and type definitions. Feedback is provided on ModularSettingsWindow.tsx to move the getCurrentWindow() call inside the useEffect hook with an empty dependency array to prevent unnecessary re-runs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const window = getCurrentWindow(); | ||
|
|
||
| useEffect(() => { | ||
| // Set up the window properly for macOS | ||
| const setupWindow = async () => { | ||
| try { | ||
| await window.setTitle("Juno Settings"); | ||
| if (window.label === "settings") { | ||
| console.log("Modular settings window initialized"); | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to setup modular settings window:", error); | ||
| console.error("Failed to setup settings window:", error); | ||
| } | ||
| }; | ||
|
|
||
| setupWindow(); | ||
| }, [window]); |
There was a problem hiding this comment.
Calling getCurrentWindow() in the component render body and using the returned window object as a dependency in useEffect can cause the effect to run unnecessarily if the window reference is not stable across renders. Since window is only used inside this effect, we can call getCurrentWindow() directly inside the useEffect and use an empty dependency array [] to ensure it only runs once on mount.
useEffect(() => {
const setupWindow = async () => {
try {
const win = getCurrentWindow();
await win.setTitle("Juno Settings");
} catch (error) {
console.error("Failed to setup settings window:", error);
}
};
setupWindow();
}, []);
1 participant
Summary
src/types/settings.ts)confirm()dialog with inline confirmation UI (banned per CLAUDE.md)MousePointer2iconTest plan
npx tsc --noEmitpasses clean