Skip to content

fix(settings): make Cmd+, only open settings, never toggle#1898

Merged
Davidknp merged 1 commit intogeneralaction:mainfrom
janburzinski:jan/eng-990-settings-panel-button-doesnt-open-settings
May 6, 2026
Merged

fix(settings): make Cmd+, only open settings, never toggle#1898
Davidknp merged 1 commit intogeneralaction:mainfrom
janburzinski:jan/eng-990-settings-panel-button-doesnt-open-settings

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

fixes settings shortcut

Both the macOS menu accelerator and the in-app hotkey for Cmd+,
fired navigate('settings') unconditionally. When already on the
settings view, double-fires through the closeModal side-effect
could appear as a toggle. Guard both paths with a currentView
check so pressing the shortcut on the settings view is a no-op.

Refs ENG-990
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR prevents Cmd+, (and the menu item equivalent) from acting as a toggle when settings is already the current view — it now simply does nothing if you're already on settings instead of re-navigating.

  • app-keyboard-shortcuts.tsx: Guards the hotkey handler with if (currentView !== 'settings') before calling navigate('settings').
  • app-menu-events.tsx: Adds useWorkspaceSlots() to read currentView, applies the same guard inside the menuOpenSettingsChannel listener, and correctly adds currentView to the useEffect dependency array so the closure never goes stale.

Confidence Score: 5/5

Safe to merge — the change is narrow, both code paths are handled consistently, and no regressions are introduced.

Both touch points (hotkey handler and IPC menu listener) receive the same guard. The useEffect dependency array in app-menu-events.tsx is correctly updated so the closure always reflects the latest currentView, avoiding any stale-capture bug. The logic is straightforward and easy to verify.

No files require special attention.

Important Files Changed

Filename Overview
src/renderer/lib/components/app-keyboard-shortcuts.tsx Adds a currentView !== 'settings' guard to the hotkey handler; currentView was already in scope from an existing useWorkspaceSlots() call, so no new hook is needed.
src/renderer/app/app-menu-events.tsx Introduces useWorkspaceSlots() to obtain currentView, applies the same early-return guard inside the IPC listener, and properly adds currentView to the effect's dependency array to keep the closure fresh.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Cmd+, pressed\n(hotkey or menu)"] --> B{"currentView\n=== 'settings'?"}
    B -- "Yes (already on settings)" --> C["Return — do nothing"]
    B -- "No" --> D{"onOpenSettings\ncallback returns false?\n(menu path only)"}
    D -- "Yes" --> C
    D -- "No / not present" --> E["navigate('settings')"]
Loading

Reviews (1): Last reviewed commit: "fix(settings): make Cmd+, only open sett..." | Re-trigger Greptile

@Davidknp Davidknp merged commit 699a60e into generalaction:main May 6, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants