Skip to content

feat(settings): make Find a rebindable shortcut so Cmd+F can open the filter bar - #2453

Merged
datlechin merged 1 commit into
mainfrom
fix/customizable-find-shortcut
Aug 26, 2026
Merged

feat(settings): make Find a rebindable shortcut so Cmd+F can open the filter bar#2453
datlechin merged 1 commit into
mainfrom
fix/customizable-find-shortcut

Conversation

@datlechin

Copy link
Copy Markdown
Member

A user asked to use Cmd+F for the filter drawer instead of Find. This makes that possible without changing anyone's defaults.

Root cause

Not a wrong default, a missing capability. Find… was the only command wired outside the shortcut-customization system:

  • EditMenuBuilder.swift:118-124 hardcoded keyEquivalent: "f", modifiers: .command and passed no shortcut: / keyboard:, unlike its siblings Find Next and Find Previous.
  • Cmd+F sat in ShortcutAction.reservedAppShortcuts, and reservedConflict(for:context:) returns that for every context, so ShortcutConflictResolver refused to bind Cmd+F to anything at all.

So the request was blocked by a hard gate, not declined as a preference.

Why the defaults do not move

Apple's HIG lists Command-F / Open a Find window as a standard shortcut and says "In general, don't repurpose standard keyboard shortcuts for custom actions". Measured on macOS, Mail and Xcode both give filter a non-F key (Cmd+L, Cmd+Option+J).

TablePlus binds Cmd+F to Open Row Filter and ships no grid find at all, which is where the request comes from. But TablePlus users are themselves asking for the opposite (TablePlus #2279, still open), Postico 2 ships Cmd+Option+F for the filter bar exactly as we do, and DataGrip, the only surveyed tool that has both a grid find and a filter, gives Cmd+F to Find.

This shortcut has already flip-flopped three times (CHANGELOG.md:2164:1538:658/:213), and the current arrangement is what the grid find bar with Search All Rows and the #2244 JSON-mode work were built for. A preference split between two camps is settled by rebinding, not by a fourth reversal.

The change

  • ShortcutAction.find, context .global, default Cmd+F. .global is explicit and commented, because only a global context overlaps .dataGrid, and that overlap is the entire reason the yield works.
  • Cmd+F removed from reservedAppShortcuts and from editorBuiltIns.
  • Find… now routes through KeyboardSettings like every other customizable command.
  • The vendored CodeEditSourceEditor local key monitor drops its case (commandKey, "f").

That last one is required, not cosmetic. The monitor runs before NSMenu key-equivalent matching and ignores KeyboardSettings, so without it Settings would show Find on another key while the editor still opened Find on Cmd+F. It is safe because TextViewController+FindActions.swift already declares performFind on the same controller, so the nil-targeted menu item reaches the identical showFindPanel() through the responder chain. Commit f5afbe221 already added Cmd+Shift+D and Cmd+Shift+K to that same switch, so editing it is established practice here.

One edit for the user. shortcut(for:) already yields an action's default when another action's override claims that key in an overlapping context, so binding Toggle Filters to Cmd+F stands Find down on its own. Two menu items can never both hold Cmd+F, which is the AppKit-blanks-the-loser trap in CLAUDE.md.

Fixed while here

Review caught that the recorder's Reassign button called clearShortcut unconditionally, persisting the cleared sentinel even when the losing action was still on its default. KeyboardShortcutModels.swift:390-392 says explicitly that storing the stand-down is wrong: it makes it permanent, shows the action as customized, and its Reset arrow appears to work until the next launch re-applies the clear. In this feature's headline flow that meant a user who later moved the filter bar back to Cmd+Option+F would find Find permanently unbound. Reassign now clears only a loser that has an explicit override; one on its default is left to yield and recovers on its own.

Verification

Step Result
verify.sh build PASS
verify.sh test (14 menu + keyboard suites) PASS, 92 executed, 92 passed
verify.sh lint TablePro TableProTests LocalPackages/CodeEditSourceEditor 0 violations
verify.sh docs PASS

The category switch is exhaustive with no default:, so the new case had to be handled at every site to compile at all. MainMenuShortcutCoverageTests.everyShortcutActionIsReachable pins that .find reaches exactly one menu item.

Tests added: defaults still pin .find == Cmd+F and .toggleFilters == Cmd+Option+F (a guard against reversal four); Cmd+F is no longer reserved in any context; Find yields and exactly one menu item ends up holding Cmd+F; Find… carries no hardcoded key equivalent; and Reassign leaves Find recoverable.

No UI automation: the flow is Settings > Keyboard recorder input, and ShortcutConflictResolver consults SystemHotkeyChecker, which reads live System Settings through CopySymbolicHotKeys. Asserting on it would make the test depend on the machine, so the coverage sits on the deterministic app-owned layer instead.

Notes for the reviewer

  • docs/features/keyboard-shortcuts.mdx:266 pictures the Settings > Keyboard list, which now has a Find row. The screenshot needs re-capturing; I did not fabricate one.
  • Keyboard settings sync (SyncCoordinator.swift:880). A user running this build and an older one with sync on, who rebinds Cmd+F, gets a duplicate key equivalent on the old build, which still hardcodes Cmd+F for Find. sanitized() only drops bare keys, so it does not filter this. Not fixable from this side and it heals when the other build updates.

@mintlify

mintlify Bot commented Aug 26, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟡 Building Aug 26, 2026, 9:41 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit eab6509 into main Aug 26, 2026
9 checks passed
@datlechin
datlechin deleted the fix/customizable-find-shortcut branch August 26, 2026 09:43
@datlechin

Copy link
Copy Markdown
Member Author

Follow-up after re-reading the Codex job records properly (the earlier run's findings were in the job result, not stdout, and I had missed them).

Codex adversarial-review returned No-ship on the first commit, and it was right:

a supported Settings sequence can still create two Cmd+F menu claimants

Reproduction: assign Cmd+F to Find Next (accepting the conflict against Find), then assign Cmd+F to Toggle Filters. The second assignment was accepted, because findNext was classified .editor and toggleFilters is .dataGrid, so findConflict treated them as non-overlapping. Both are nil-targeted menu key equivalents, so AppKit would blank one.

That context split was wrong on its own terms: MainSplitViewController.findNext and findPrevious branch on hasActiveGridFind and route to the grid or the editor, exactly like performFind. Fixed in 62612a6 by classifying all three find commands .global, which is the same argument the first commit used for .find.

This also closes a pre-existing hole the review surfaced: binding a data-grid action to Cmd+G previously raised no conflict against Find Next and silently cost the user a shortcut. There is now a regression test for it.

Tests: 96 executed, 96 passed. Build and lint clean.

Two review points I did not act on, with reasons:

  • Package Example loses Cmd+F (P2). Confirmed real: LocalPackages/CodeEditSourceEditor/Example has no menu Find and no performFind responder, so the deleted monitor case was its only route. Keeping the case is not an option, because it matches the literal "f" character rather than the user's configured key, so it fires before the menu whatever the user binds and defeats the whole feature. The Example still has its Show/Hide Find button. Worth raising upstream separately.
  • Comment style (P3). Trimmed to three lines stating the invariant. Doc comments of this kind are the established convention in this file (see ShortcutContext.overlaps and KeyboardSettings.shortcut(for:)); the rule targets behavior-describing inline comments.

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.

1 participant