fix(hig): replace hand-rolled controls with native appkit and swiftui equivalents - #2104
Merged
datlechin merged 1 commit intoAug 13, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
datlechin
merged commit Aug 13, 2026
6d79728
into
worktree-single-window-connections
3 of 4 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit of the whole UI for hand-rolled controls, faked chrome and HIG violations, with the native replacement verified against the installed SDK for each one. 67 candidates were reviewed adversarially; 6 were refuted, 10 cleared as justified custom code, and the surviving 45 are implemented here.
Stacked on #2097. Base is
worktree-single-window-connections, notmain, because several items build on that branch's commits (the editor tab strip, the workspace registry). Retarget once #2097 lands.On
NSSegmentedControl.role = .tabsWorth recording, since it was the starting point. The API is real but
API_AVAILABLE(macos(27.0)), and this app targets 14.0. Availability is the smaller problem: a segmented control has no per-segment close button, hover fill, tooltip or scrolling track, so it cannot express a document tab strip at any target. Neither canNSTabView. macOS ships no public closable-tab control; Safari and Finder use the privateNSTabBarbehindNSWindow's tab group, which cannot show per-connection tab sets in one window. The strip stays custom, and what it was actually missing was native behaviour, not a native class. One place does use the API: the inspector's split-column alert, guarded, with.valueSelection.Bugs found on the way
Dropon.keyboardShortcut(.defaultAction).proceed().orderswhileuserswas selected truncatedusers.Cmd+W; it ran Close Tab instead.What changed
NSToolbarItems and the two pairs becameNSToolbarItemGroups with subitems and no view, so Icon Only, display-mode customization and the overflow menu work. Only the connection group stays hosted; it carries two SwiftUI popovers and a variable-length row.NSTitlebarAccessoryViewControllerat.bottom, repointed on workspace switch, with the window minimum accounting for it. Tabs are now realButtons with a contextual menu and.isTabBar.NSAlertor a proper modal. Three custom result views deleted. The pairing path no longer nestsrunModalinside a continuation.alternateSelectedControlTextColor; selection dims on window emphasis; tag badges, Pro badge, Vim indicator and the date picker derive a legible label from their fill.Verification
Builds clean,
swiftlint --strictclean on all 101 touched files, 111 tests pass across 11 new suites, no failures.Reviewer notes
NSHostingViewnever responded tosetEnabled:, so the validation table had no visual effect. Any wrong entry will now show as a permanently dimmed button. Worth a pass over the toolbar on first launch.newWindowForTabremoves AppKit's tab-bar plus button. Correct for a single-window app, but it is a visible affordance going away. The toolbar's New Tab was repointed first so it did not go inert.Localizable.xcstringsis deliberately not in this PR. The working copy was dirty with keys from unrelated work, including one for a menu item this PR removes. The new strings fall back to their English key until a build regenerates the catalog.PersistedTabRoundTripTests.swiftcalledtoPersistedTab(windowGroupIndex:), removed on the base branch without the test being updated, which broke the whole test target. Changed to set the field directly, preserving coverage.Single commit rather than one per item: the changes share new types (
Color+Emphasis,MotionAccessibility,DialogFooter), so any split would produce intermediate commits that do not build.https://claude.ai/code/session_01A3rb597qZtq4h5xZxwg43W