Skip to content

fix(settings): toggle settings shortcut #1915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
janburzinski wants to merge 11 commits into
base: main
Choose a base branch
from
Open

fix(settings): toggle settings shortcut #1915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
06d41f0
fix(settings): toggle settings shortcut
janburzinski May 6, 2026
db2396e
fix(settings): preserve previous view when toggling
janburzinski May 7, 2026
01ad0b0
fix(settings): guard open callback on close
janburzinski May 7, 2026
63c5222
ENG-1101 Close settings with Escape
janburzinski May 8, 2026
f0a0af4
Merge origin/main into jan/eng-990-settings-dont-toggle
janburzinski May 8, 2026
3b10306
Merge remote-tracking branch 'origin/main' into jan/eng-990-settings-…
janburzinski May 8, 2026
cb8277b
Merge branch 'main' into jan/eng-990-settings-dont-toggle
arnestrickmann May 11, 2026
e2ea3a8
Merge remote-tracking branch 'upstream/main' into jan/eng-990-setting…
janburzinski May 12, 2026
c199746
Merge branch 'jan/eng-990-settings-dont-toggle' of https://github.com…
janburzinski May 12, 2026
4361fa8
fix(settings): preserve navigation when toggling settings
janburzinski May 12, 2026
e5b4d0d
style: format app keyboard shortcuts
janburzinski May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/renderer/app/app-menu-events.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { useEffect } from 'react';
import { menuOpenSettingsChannel } from '@shared/events/appEvents';
import { events } from '@renderer/lib/ipc';
import { useNavigate, useWorkspaceSlots } from '@renderer/lib/layout/navigation-provider';
import { toggleSettingsView } from '@renderer/lib/layout/settings-toggle';

export function AppMenuEvents({ onOpenSettings }: { onOpenSettings?: () => boolean | void }) {
const { navigate } = useNavigate();
Expand All @@ -11,9 +12,8 @@ export function AppMenuEvents({ onOpenSettings }: { onOpenSettings?: () => boole
return events.on(menuOpenSettingsChannel, () => {
const shouldOpen = onOpenSettings?.() ?? true;
if (shouldOpen === false) return;
if (currentView === 'settings') return;

navigate('settings');
toggleSettingsView(navigate, currentView);
});
Comment on lines 14 to 21
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 onOpenSettings guard runs on the close path too

onOpenSettings?.() is now called even when currentView === 'settings' (i.e., the user is toggling settings off). In App.tsx, the concrete implementation calls setView('workspace') as a side effect every time — including on close — and returns false (blocking navigation) when the app is in onboarding. While those specific side effects are harmless today, the callback is semantically an "open" guard and was never designed to control the close path. The fix is to call onOpenSettings only when the intent is to open: check currentView !== 'settings' before invoking the callback.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/renderer/app/app-menu-events.tsx
Line: 12-17

Comment:
**`onOpenSettings` guard runs on the close path too**

`onOpenSettings?.()` is now called even when `currentView === 'settings'` (i.e., the user is toggling settings *off*). In `App.tsx`, the concrete implementation calls `setView('workspace')` as a side effect every time — including on close — and returns `false` (blocking navigation) when the app is in onboarding. While those specific side effects are harmless today, the callback is semantically an "open" guard and was never designed to control the close path. The fix is to call `onOpenSettings` only when the intent is to open: check `currentView !== 'settings'` before invoking the callback.

How can I resolve this? If you propose a fix, please make it concise.

}, [navigate, onOpenSettings, currentView]);

Expand Down
5 changes: 2 additions & 3 deletions src/renderer/lib/components/app-keyboard-shortcuts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
useParams,
useWorkspaceSlots,
} from '@renderer/lib/layout/navigation-provider';
import { toggleSettingsView } from '@renderer/lib/layout/settings-toggle';
import { useShowModal } from '@renderer/lib/modal/modal-provider';

/**
Expand Down Expand Up @@ -54,9 +55,7 @@ export function AppKeyboardShortcuts() {

useHotkey(
getHotkeyRegistration('settings', keyboard),
() => {
if (currentView !== 'settings') navigate('settings');
},
() => toggleSettingsView(navigate, currentView),
{ enabled: settingsHotkey !== null }
);

Expand Down
26 changes: 26 additions & 0 deletions src/renderer/lib/layout/settings-toggle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import type { ViewId } from '@renderer/app/view-registry';
import type { NavigateFnTyped } from './navigation-provider';

type NonSettingsViewId = Exclude<ViewId, 'settings'>;

let previousView: NonSettingsViewId = 'home';
let lastToggleAt = 0;
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Module-level previousView shared across all toggle callers

previousView and lastToggleAt are module-level singletons. Both AppMenuEvents and AppKeyboardShortcuts call toggleSettingsView, so a menu-triggered toggle and a keyboard-triggered toggle that happen to arrive outside the 150 ms dedup window (e.g., under load) will each read and overwrite the same previousView. More broadly, if a future caller opens settings through a different code path that doesn't go through this helper, previousView will be stale ('home') on the next close, silently dropping the user's actual context. The guard is correct for the current callers; the risk is for future additions.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/renderer/lib/layout/settings-toggle.ts
Line: 5-7

Comment:
**Module-level `previousView` shared across all toggle callers**

`previousView` and `lastToggleAt` are module-level singletons. Both `AppMenuEvents` and `AppKeyboardShortcuts` call `toggleSettingsView`, so a menu-triggered toggle and a keyboard-triggered toggle that happen to arrive outside the 150 ms dedup window (e.g., under load) will each read and overwrite the same `previousView`. More broadly, if a future caller opens settings through a different code path that doesn't go through this helper, `previousView` will be stale (`'home'`) on the next close, silently dropping the user's actual context. The guard is correct for the current callers; the risk is for future additions.

How can I resolve this? If you propose a fix, please make it concise.


// macOS menu accelerator and renderer hotkey both fire for one Cmd+, press;
// without this guard they'd toggle then untoggle on the same keystroke.
const DEDUP_WINDOW_MS = 150;

export function toggleSettingsView(navigate: NavigateFnTyped, currentView: string): void {
const now = Date.now();
if (now - lastToggleAt < DEDUP_WINDOW_MS) return;
lastToggleAt = now;

if (currentView === 'settings') {
// Bare navigate(viewId) preserves stored params for views that statically require them.
(navigate as (viewId: ViewId) => void)(previousView);
return;
}

previousView = currentView as NonSettingsViewId;
navigate('settings');
}
Loading