Skip to content

refactor(hig): rebuild the menu bar on native AppKit menus - #2057

Merged
datlechin merged 4 commits into
mainfrom
refactor/main-menu-appkit
Aug 10, 2026
Merged

refactor(hig): rebuild the menu bar on native AppKit menus#2057
datlechin merged 4 commits into
mainfrom
refactor/main-menu-appkit

Conversation

@datlechin

@datlechin datlechin commented Aug 10, 2026

Copy link
Copy Markdown
Member

Rebuilds the menu bar on native AppKit menus.

Why

Every editor window is a plain AppKit NSWindow, never a SwiftUI Scene. The menu bar was authored in SwiftUI Commands, which has no access to NSUserInterfaceValidations, so it could not ask the key window's responder chain whether a command applies. Everything odd in the old menu code was a substitute for that missing call:

  • CommandActionsRegistry existed, by its own comment, because @FocusedValue cannot resolve for a window that is not a scene.
  • resolvedCloseTabActions walked three fallback tiers, including two linear scans, to guess what the responder chain already knows.
  • keyWindowIsInspector and the Undo/Redo titles read NSApp.keyWindow inside a Commands body, which SwiftUI does not track, so the values went stale.
  • CommandFRoute existed only because SwiftUI drops one of two items that claim the same shortcut.
  • 24 button actions called NSApp.sendAction(_:to:from:) by hand, re-implementing nil-target dispatch without the validation half.
  • Cut, Copy, Paste, and Select All carried no enabled condition at all.

The correct pattern was already in the codebase three times and worked every time: MainWindowToolbar+Validation, InspectorViewController, and KeyHandlingTableView. The only surface not using it was the only one that could not.

What changed

Menu items are real NSMenuItems with target = nil. AppKit resolves each one against the key window's responder chain and asks that responder whether it is enabled. Hand-written enabled flags are gone.

  • TablePro/Core/Menu/ holds one builder per menu plus MenuItemFactory and MainMenuKeyEquivalentSync.
  • MainSplitViewController+<Domain>MenuActions.swift are one-line forwards into the existing MainContentCommandActions, which is unchanged.
  • MainSplitViewController+MenuValidation.swift splits a pure MenuValidationContext and static isEnabled(_:context:) from a thin adapter, the same shape MainWindowToolbar+Validation uses.
  • Menu bar is now TablePro, File, Edit, View, Database, Query, Window, Help.

Deleted: AppMenuCommands, PasteboardCommands, CommandFRoute, ResponderChainActions, AppDelegate.showHelp, and roughly 700 lines from TableProApp.swift. CommandActionsRegistry stays, reduced to what it actually is now: a lookup for two unfocused AI chat views. The menu no longer touches it.

Added because they were missing: Minimize, Zoom, Move Tab to New Window, Show Toolbar, Customize Toolbar, New Table, New View.

Removed: Show Tables Sidebar and Show Favorites Sidebar (the sidebar's tab control does this), Show Object Icons and Show Object Comments (Settings and the sidebar's View Options do this), Open Project Folder (the welcome window does this).

Shortcut change

Cmd+F now always means Find. The filter bar moves to Cmd+Option+F, which is what TablePlus, Postico, and Sequel Ace all use, and Focus Sidebar Filter moves to Ctrl+Cmd+Option+F. KeyboardSettings stores overrides only, so anyone who rebound these keeps their own binding.

The old pair both defaulted to Cmd+F, which is what CommandFRoute was working around.

Verification

An unbundled harness confirmed the parts Apple does not document, in the same topology this app has (SwiftUI App plus @NSApplicationDelegateAdaptor plus a plain NSWindow):

  • Replacing NSApp.mainMenu in applicationDidFinishLaunching sticks. SwiftUI does not reinstall its menu.
  • Nil-target validation works on a non-scene AppKit window. A selector no responder implements is disabled automatically.
  • Two items claiming one key equivalent leaves the loser blanked, which is now a test.

MainMenuBuilderTests covers menu order, title uniqueness (System Settings binds an App Shortcut by exact title), key equivalent uniqueness, every ShortcutAction reaching exactly one item, target nil-ness, and a validation truth table. All menu tests pass. Existing keyboard shortcut suites still pass. swiftlint --strict reports zero violations. Debug build succeeds.

Review notes

The behaviour worth exercising by hand is enablement across window types: switch between an editor window, a CSV inspector window, and no window at all, and check that items dim and undim correctly, since that is exactly what the old menu got wrong.


Update: the deferred commands are in, and the disconnect work is absorbed

The five commands originally listed as follow-ups now ship here. The claim that they needed missing plumbing was wrong: MainContentCommandActions already held the sidebar selection, so each needed a selectedObject accessor and a thin forward.

Database menu now carries New Database, New Table, New View, Show Table Structure, Edit View Definition, Table Maintenance, Truncate Table, Disconnect, and Reconnect. Table Maintenance is a delegate-driven submenu, filled on menuNeedsUpdate, because the operations depend on the driver and the selection.

This branch rebased onto the merged disconnect work and simplified it. ConnectionDisconnectAction, requestDisconnect() and retryConnection() are kept as-is. Deleted, because responder-chain validation makes them redundant: ConnectionWindowCommandState, CommandActionsRegistry.connectionWindow, publishConnectionCommandState(), clearConnectionCommandStateIfOwned(), the owner-identity guard, and the NSApp.keyWindow?.contentViewController as? lookup its menu code used. Its changelog entry and docs/features/tabs.mdx were corrected in place rather than with a Fixed entry, since both are unreleased.

Update: audit findings

A five-lens audit ran over the result, each finding checked by two independent skeptics. 13 claims were refuted; the rest were fixed. The two that mattered:

Undo and Redo recursed until the stack overflowed. MainSplitViewController.undo(_:) forwarded to undoChange(), which still contained a NSApp.sendAction("undo:", to: nil) probe from the SwiftUI era. That probe restarts target resolution at the key window's first responder, which now resolves back to MainSplitViewController. On main the probe was safe because nothing in the connection window implemented undo:; adding the responder method closed the loop. Cmd+Z with the grid focused would crash. The probes are gone, and Undo/Redo now validate against the live undo manager and take their title from it.

Cmd+T could crash on a window that was not connected. newWindowForTab: resolves to NSWindow, so the split view controller's validation never saw it and the item stayed enabled while connecting, failed, or disconnected. EditorWindow now validates it.

Also fixed: the text-input key-equivalent yield was written but never wired, so a focused text field would have kept losing Cmd+Delete to the menu; three View toggles never flipped their title; Integrations opened Settings instead of the Integrations window; Copy Rows resolved against a different responder than its two siblings; and switching menu titles to a Unicode ellipsis had orphaned 99 already-translated strings, so they are back on the catalog's existing form.

Removed as dead: dataGridShortcut, AppDelegate.newWindowForTab, menuChangeManager, an empty if left by the disconnect cleanup, and an app-wide NSWindow extension that had one caller.

@mintlify

mintlify Bot commented Aug 10, 2026

Copy link
Copy Markdown

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

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 10, 2026, 4:03 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.

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