refactor(settings): tab the settings window and cut what it did not need - #232
Merged
Conversation
Four of the six rows in Sidebar Appearance were per-pixel tuning of one surface: light tint hex, dark tint hex, tint opacity, and a section-local reset. The same four keys are already bound by the Debug window, so this removes the copy that shipped to every user and keeps the one intended for development. The @AppStorage bindings stay on SettingsView because "Reset all settings" still writes them, which is what restores anyone who set a tint while the rows existed. Sidebar Appearance goes from six rows to two: Match Terminal Background and Show Claude Quota, both of which change behaviour rather than pixels. Also drops the nine localization keys that went dead with the rows. settings.sidebarAppearance.defaultLabel is deliberately kept -- the Workspace Colors rows still use it.
Workspace Colors carried a full palette CRUD editor inside the settings panel: a hex picker and Remove button per named color, plus a note telling you to edit settings.json for the same thing. It also had selection-highlight and notification-badge hex pickers, which are the same per-pixel tuning the sidebar tints were. All of it goes. settings.json stays the way to manage named colors, which is what the note already said. "Reset all settings" still calls WorkspaceTabColorSettings.reset() and nils both hex keys, so anything set while these rows existed is recoverable. The section keeps one row, Workspace Color Indicator, which changes what the sidebar draws rather than what shade it draws it in. Removing the editor made a whole chain dead: workspaceTabPaletteEntries and the four helpers that maintained it, an onAppear refresh, and an onReceive on UserDefaults.didChangeNotification that reloaded the palette on every defaults write. That last one ran for every settings change while the window was open. Also drops nine localization keys, including settings.sidebarAppearance.defaultLabel, whose last two users were the hex pickers removed here.
"Desktop Notifications" and "Notification Command" passed raw string literals for title, subtitle and placeholder, so they stayed English in a Japanese UI. Every other row in the file uses String(localized:). Found while inventorying the App section for the settings cut.
Settings was one scroll of nine stacked sections, so finding anything meant knowing roughly how far down it lived. Sections that answer the same question are merged: Workspace Colors and Sidebar Appearance become Appearance, Custom Commands joins Automation, and Reset sits with the general app switches it resets. That gives General, Appearance, Automation, Phone, Browser. Keyboard Shortcuts stays on its own. It renders one row per KeyboardShortcutSettings.Action, and there are 57 of them, so it would swamp whatever it shared a tab with. The tab strip lives in the header overlay rather than the scroll content, so it stays put while a tab's rows scroll under the existing blur. The header grows from 62pt to 96pt and the content inset moves with it. SettingsNavigationRequest deep links still work: the handler selects the owning tab before scrolling, because an anchor only exists while its tab is selected. browserImport is the one that still needs the scroll, since it sits mid-tab rather than at the top.
…ggle Cmd+W on a workspace's last surface now always closes the workspace, which was already the default. The opt-out is gone. Removed end to end rather than just hidden: the row and its inverted binding in SettingsView, the LastSurfaceCloseShortcutSettings enum in TabManager, both the parse and emit sides of keepWorkspaceOpenWhenClosingLastSurface in ProgramaSettingsFileStore, and three localization keys. An existing settings.json that still sets keepWorkspaceOpenWhenClosingLastSurface is harmless: the store only reads keys it knows about, so the entry is ignored rather than rejected. shouldCloseWorkspaceOnLastSurfaceShortcut keeps its other two conditions, so the close button still routes through Workspace's explicit-close path.
Clicking into an inactive Programa window now always activates the window without also focusing the pane under the pointer, which was already the default. The opt-in is gone. Every read site was gated on a flag that is now permanently false, so the gates go with it: - GhosttyNSView and ProgramaWebView lose their acceptsFirstMouse overrides entirely. NSView already returns false, so the overrides were restating the default and reading UserDefaults on a mouse-down path to do it. - MarkdownPanelPointerObserverView keeps its hitTest override, now returning nil unconditionally. Deleting it would have fallen back to NSView's default hit-testing, which is the opposite of what the view did. - Its acceptsFirstMouse override goes, and the dead branch in shouldHandle goes with it. Also removes the PaneFirstClickFocusSettings enum, both schema sides of focusPaneOnFirstClick, and three localization keys. InactivePaneFirstClickFocusTests now pins the single behaviour instead of exercising both sides of a toggle, including that writing the old defaults key no longer re-enables click-through. That last one is the real regression guard: a live read left behind would make the removal cosmetic for anyone whose settings.json still carries the key. Also drops LastSurfaceCloseShortcutSettingsTests, which covered the enum deleted in the previous commit. That commit only built the app target and missed it.
Command-palette rename now always opens with the existing name selected, so typing replaces it. That was already the default. This one reached further than the other cuts because the setting had a v2 socket method behind it so UI tests could flip it: - Sources: the CommandPaletteRenameSelectionSettings enum, the @AppStorage in CommandPaletteController, a dead read/write proxy in ContentView, both schema sides of renameSelectsExistingName, and the settings row. - Socket API: debug.command_palette.rename_input.select_all is gone, along with its catalog entry, its dispatch case, and the two client helpers in tests_v2/cmux.py. It is a debug.* method, not part of the stable surface. - ContentView's rename focus policy now asks for .selectAll directly. Other focus policies still use .caretAtEnd, so that case stays. Test changes, rather than deletions, where the coverage was still worth having. test_command_palette_rename_select_all.py kept its eight stress cycles proving the selection survives interaction with the field, and lost only the half that toggled the setting off. test_command_palette_backspace_go_back.py no longer pins the setting to get a deterministic starting state, because the behaviour is now guaranteed. CommandPaletteRenameSelectionSettingsTests is gone with the enum it covered. Both the app and the unit-test target build. tests_v2 is CI-only and unrun here.
tests/ and tests_v2/ both held python, which made the split look like versioning. It is not: tests_v2 is named for the v2 socket API, and every file there drives a running app over the control socket. Two files were on the wrong side of that line. test_text_input_refresh_cost.py and test_workspace_churn_up_arrow_lag.py both connect to a live instance, so they move to tests_v2 and their ci.yml invocations move with them. The three python files left in tests/ drive the programa CLI binary as a subprocess and need no app, which is what tests/ is for alongside the shell script guards. Adds docs/testing-layout.md explaining the four harnesses, what _v2 actually refers to, and which directory a new test belongs in. That question came up because nothing in the repo answered it.
CI caught three tests that still exercised the preference deleted earlier in this branch. They compiled because they write the raw defaults key rather than referencing the enum, so only running them surfaced it: - TabManagerCloseCurrentPanelTests.testCloseCurrentPanelKeepsWorkspaceOpen... failed outright. It set the key and expected the workspace to survive. - testClosePanelButtonStillClosesWorkspaceWhenKeepWorkspaceOpen... still passed, but only by duplicating the plain close-button test now that the preference does nothing. - AppDelegateShortcutRoutingTests.testCmdWKeepsLastSurfaceWorkspaceOpen... had the same premise. Replaced with one guard modelled on the two-workspace close test that already passes, writing the removed key first and asserting it changes nothing. That covers what actually matters: a stale settings.json entry must not bring the old behaviour back. The earlier cut verified against Sources/ and CLI/ but not programaTests/, which is how these survived.
The Browser section rendered a mock of the blank-tab import hint, reusing the hint's own strings. That included its footnote, "You can always find this in Settings > Browser", displayed to someone already looking at Settings. The Choose and Refresh buttons directly below it already offer the action the mock was advertising, so the mock only added noise. The real card is unchanged in BrowserToolbarViews, and both strings are still used there. browserImportSubtitle went with it, since the mock was its only caller.
3 tasks
…owser-cuts refactor(settings): stop duplicating the import hint inside Settings
Pairing offered three entry points and two buttons for a single action, and the two devices did not agree on what pairing even looked like. The phone had: scan a QR, paste a combined code with a "Use This Code" button, and an Advanced section with separate ticket and token fields behind its own "Connect" button. Two buttons that both connect, one of which only worked with the fields nobody was meant to use. Its own doc comment called the split fields legacy. The Mac had: a QR, the combined code shown as selectable text with a Copy button, and below that a "Can't scan? Paste the payload and token manually" disclosure repeating the same secret split in two. Both legacy paths are gone. The Mac shows the QR and the one combined code that the QR encodes; copying that is the fallback for anyone who cannot scan. The phone scans or pastes that code, and its single button is now called Connect because connecting is what it does. The ticket and token drafts stay on AppStore, since applyPairingCode still fills them and connectManually still reads them. They are just no longer bound to fields. The Connect button also picks up the isConnecting guard the removed button used to own. Both apps build. Nine localization keys dropped across the two catalogs, and pairing.connect.useCodeButton retranslated for its new label.
4 tasks
…anup refactor(pairing): one way in on each side instead of three
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
1 participant
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.
What this does
Settings was one 2,340-line scroll of nine stacked sections, so finding anything meant knowing roughly how far down it lived. It is now six tabs, and several settings that existed only as taste knobs are gone rather than relocated.
Three toggles were deleted outright, not hidden. Each one's current default became the only behaviour, and the key, the schema entry, the read sites and the tests went with it. An existing
settings.jsonthat still sets a removed key is harmless: the store only reads keys it knows, so the entry is ignored rather than rejected.Review order
The diff is large but repetitive. Start at 1 and 2; the rest is the same shape repeated.
Sources/SettingsView.swiftandSources/SettingsModels.swift— the tab split and the newSettingsTabtypeSources/ContentView.swift,Sources/TabManager.swift,Sources/Panels/MarkdownPanelView.swift— the behaviour that used to be conditional and now is notSources/TerminalController*.swift,Sources/V2CommandCatalog.swift— onedebug.*socket method removedtests_v2/,programaTests/— tests repointed at single behaviours instead of togglesdocs/testing-layout.md— new, explains whytests/andtests_v2/both existWhat was cut
settings.jsonmanages named colours, as its own note already saidDeleting the palette editor also killed an
.onReceive(UserDefaults.didChangeNotification)that reloaded the whole workspace palette on every defaults write while Settings was open, and dropping first-click-focus removed aUserDefaultsread from a mouse-down path.Also here
test_text_input_refresh_cost.pyandtest_workspace_churn_up_arrow_lag.pydrive a running app over the socket but lived intests/. Moved totests_v2/with theirci.ymlinvocations.Test plan
xcodebuild -scheme programabuildsxcodebuild -scheme programa-unit build-for-testingbuildsci.ymlre-validated as YAML after the path movessocket-integration-tests— thetests_v2edits are unverified locally, since that suite needs a live app and is CI-onlyunit-tests— two test classes were removed with the types they covered, one repointed at the new invariantCmd+,, and check the six tabs fit and the first row of each clears the headerRisk
commandPaletteSearchesAllSurfaceswas deliberately left alone. Unlike the three that went, both of its values are defensible, and it is threaded throughContentViewin six places with its own test coverage.