Add configurable physical keyboard controls - #20
Open
obra wants to merge 47 commits into
Open
Conversation
Record the approved Remux behavior for configurable hardware-keyboard bindings, exact visible-pane search, and physical-keyboard-aware terminal chrome before implementation. The design reuses the existing current-session, pane, attachment, connection, and new-session flows; keeps keyboard persistence separate from terminal appearance settings; and defines a minimal GhosttyKit viewport-text API instead of reconstructing terminal state or issuing network capture requests. This follows Jesse’s request for configurable Command-arrow window navigation, Command-Shift-arrow session navigation, Home and overlay shortcuts, a global Command-K palette, and a full-height terminal with auto-hiding floating controls when a physical keyboard is connected.
Translate the approved physical-keyboard design into eleven TDD-sized, independently verifiable tasks across Remux and its GhosttyKit fork. The plan fixes the domain interfaces, persistence boundary, dynamic command routing, settings capture UI, hardware-keyboard and floating-chrome state, visible-viewport C API, palette search and routing, and separate simulator versus physical-device acceptance gates. It also records exact paths, focused verification commands, and frequent commit boundaries while preserving the existing touch path and untracked worktrees.
Use the repository's actual Remux and RemuxUIOnly schemes, RemuxAppTests and RemuxAppUITests source roots, root project.yml, and an installed iPad simulator identifier. This correction was found by executing the first plan command before any production code changed.
Define Remux's fixed application command set and documented default bindings for window navigation, session navigation, Home, current-session windows, panes, attachments, and the global command palette. Bindings support Command, Shift, Option, and Control combinations, can be cleared, and reject bare keys or duplicates before becoming settings. The duplicate error names the existing command that owns the chord so the settings UI can explain the conflict. The physical-keyboard button-bar preference defaults to enabled in the same independently persisted settings value. Focused evidence: KeyboardSettingsTests executed 4 tests with 0 failures on the iPad Air 11-inch simulator. The rebased checkout's stale GhosttyKit 20260720 binary was also replaced with the checksum-verified pinned 20260726 release required by upstream/main before the tests could compile.
Add a dedicated keyboard-settings.json repository rather than extending terminal-settings.json. Missing storage returns the documented defaults, saves validate before writing, and loads validate decoded content so duplicate chords cannot become live through hand-edited or corrupt JSON. Wire the repository through live, UI-test, and unit-test dependency graphs. Focused evidence: KeyboardSettingsRepositoryTests executed 3 tests with 0 failures on the iPad Air simulator, covering defaults, round-trip persistence, and decoded duplicate rejection.
Publish priority UIKeyCommands from the active responder and intercept configured chords before Ghostty terminal input handling. Root routing owns Home, command palette presentation, and wrapping session navigation in the same order shown on Home; terminal routing reuses the existing swipe, windows, panes, and attachment actions. Load keyboard settings alongside terminal settings so live responders update from persisted configuration.
Expose the global auto-hide preference and every optional app binding through the existing terminal settings flow. Capture real modified hardware chords with normalized printable keys, preserve navigation keys, allow explicit clearing, and reject bare or duplicate shortcuts before persisting the updated settings.
Project Bluetooth, Magic Keyboard, and wired keyboard attachment from GameController, with a deterministic UI-test override. Keep the existing measured bottom bar when disconnected, but render it as an unmeasured terminal overlay when connected. The default preference starts hidden, terminal taps reveal it for three seconds, chrome interactions restart that timer, and disabling auto-hide keeps the overlay visible.
Present Command-K from Home or a terminal with Add Connection, host-preselected New Session actions, and route-aware navigation and overlay commands. Snapshot every retained pane through the new Ghostty viewport symbol, match visible lines case-insensitively with host/session/window context, and route selections back to the exact session, window, and pane without persistent highlighting.
Treat an attached physical keyboard as an independent reason for the selected terminal responder to own first responder, even when software-keyboard chrome is hidden. The responder still yields to transient input owners and resigns on disconnect when software keyboard mode remains hidden. Add a focused policy regression test covering the default hidden-chrome state.
Call the new Ghostty viewport snapshot function through the generated module interface so stale frameworks fail at build time instead of silently disabling palette search. Also keep the keyboard monitor's Sendable NotificationCenter reference normally isolated, removing an unnecessary Swift concurrency escape while retaining the token storage needed for deinit cleanup.
Use the validated settings value as the object written by the file-backed repository. This preserves the validation contract and removes the ignored-result warning from Release builds.
Add a simulator UI acceptance test that opens Terminal settings, enters Physical Keyboard settings, and verifies the default-enabled auto-hide toggle plus configurable Command Palette binding affordance. The test asserts user-visible behavior and stable accessibility identifiers without depending on SwiftUI's internal row element type.
Keep the global physical-keyboard responder from reclaiming first responder while a UIKit text input is active. This allows connection fields and command-palette search to remain editable even as SwiftUI updates the responder bridge. Add simulator coverage for editing and initial button-bar auto-hide, plus a live localhost SSH/tmux acceptance test that verifies the bar reveals over an unchanged terminal frame and Cmd-K opens the command palette. The live harness uses an allowlisted disposable tmux session and cleans it after the test.
Complete the physical-keyboard feature review pass and the requested key-binding row presentation. Canonicalize printable bindings before duplicate detection and reject empty keys or unsupported modifier bits. Keep every navigation action in the command palette while disabling actions unavailable in the current context. Search only ready, live panes and identify each viewport result with its pane number. Separate terminal text-input readiness from app-command responder readiness so a connected physical keyboard can still navigate away from a disconnected selected session without allowing terminal input. Render binding glyphs as spaced tokens at the main row text height, and cover the behavior with focused unit and UI acceptance tests.
Finish the command-only first-responder path introduced for physical-keyboard navigation. The responder advertised app key commands while terminal input was unavailable, but its deferred reconciliation logic still used text-input readiness as the only enablement condition. That caused it to resign or never become first responder, so command shortcuts could be lost after terminal interaction. Use the combined responder-enable state throughout reconciliation and cover actual first-responder acquisition without enabling terminal text input. This restores the live localhost Cmd-K command-palette flow.
Align search-result pane numbers with the geometric pane ordering used by the pane picker by sharing one ordering function across both projections. Include selected-terminal readiness in command-palette availability so terminal-only actions are visibly disabled for disconnected sessions while global navigation remains available. Show duplicate-binding validation inside the active shortcut-capture sheet, where the user can see and correct it. Extend focused unit and UI acceptance coverage for all three review findings.
Extend the approved physical-keyboard design with the concrete Remux design-system treatment for the command palette and keyboard settings entry. Define the palette as the exclusive keyboard input owner while presented so typing after Command-K cannot leak into the terminal and normal terminal focus resumes after dismissal. This records Jesse's follow-up requirements before implementation and adds immediate palette focus to the focused-test acceptance boundary.
Record the TDD sequence for making the Cmd-K palette the active keyboard input owner, applying Remux's shared dynamic surfaces to the palette and Physical Keyboard settings entry, and validating the finished work on simulator and Jesse's iPhone. The plan keeps the responder change separate from the visual integration and preserves the live-device acceptance boundary.
Route command-palette presentation into terminal responder ownership so an active terminal yields hardware-keyboard input while the palette is visible. Defer SwiftUI field focus until the palette is mounted, preserve child accessibility identifiers, and cover immediate Cmd-K typing with a UI regression plus the terminal focus projection with unit tests. Render the palette and Physical Keyboard settings row using the existing grouped app palette instead of unmatched material/default surfaces. Preserve any external first responder in the global command responder so the configurable shortcut capture control cannot lose keystrokes to app navigation.
Treat REMUX_UI_TESTING as disconnected from a physical keyboard unless the dedicated override is present. This prevents the simulator host keyboard from auto-hiding terminal controls in unrelated UI tests while retaining production GCKeyboard detection and explicit physical-keyboard coverage. Update terminal UI helpers to reveal auto-hidden chrome through a screen tap when needed, wait for the shortcut capture sheet before typing, and avoid asserting simulator software-keyboard behavior that Remux does not own. The existing configured live-SSH test remains the acceptance gate for tap-to-reveal and overlay sizing.
Change Remux's Home default from Command-H to Command-Shift-H because iPadOS owns Command-H globally. Reject Command-H during keyboard-settings validation and explain the collision in the capture UI so users cannot save a binding the app cannot reliably receive. Update the unit and UI contracts for the new default, keep the capture-sheet invalid-input check independent of OS-reserved chords, and verify the settings flow repeatedly to distinguish the original system interception from responder timing.
Fold configurable font-size commands into the original physical-keyboard design because no keyboard settings have been deployed. Specify Command-Plus and Command-Minus defaults, app-global one-point adjustments, effective-device-size behavior, bounds, persistence, active-session refresh, and test coverage without introducing a migration or compatibility layer. Also reconcile the written design with the already-approved Command-Shift-H default and Command-H reservation.
Define one TDD-sized implementation task that extends the existing keyboard command registry, routes Command-Plus and Command-Minus as app-global terminal settings, starts automatic sizing from the effective device default, clamps to the existing range, persists through RemuxRootModel, refreshes active sessions, and verifies settings visibility and full suites. The plan intentionally adds no migration because keyboard settings have not been deployed.
Register configurable Command-Plus and Command-Minus app commands, route them globally from both root and active terminal surfaces, and persist bounded font-size adjustments through the existing terminal settings update path. Automatic sizing uses the current device size for the first adjustment and explicit settings thereafter. Add focused coverage for default bindings, effective-size mutation and clamping, global routing, persistence, and settings UI visibility. Preserve Command Palette as the final settings row so its established shortcut-capture flow remains intact.
Route keyboard font-size deliveries into a model-owned task chain so each adjustment waits for its predecessor, including repository persistence, before reading and mutating terminal settings. This preserves shortcut delivery order without changing the existing persistence and active-session fanout path. Add a clamp-boundary regression with a repository that blocks the first save. The test submits Increase then Decrease at size 24, verifies the second save cannot start early, and requires both in-memory and persisted state to finish at 23.
Move non-terminal shortcuts from a hidden sibling view into a parent hosting controller that remains in UIKit's active responder chain while terminals, text fields, and overlays own focus. Publish priority UIKeyCommands and retain raw press routing so configured global chords reach the current app command handler without stealing ordinary text input. Give the command palette an immediately focused UIKit search field with Escape dismissal, arrow-key selection, Return activation, selection highlighting, and design-system surfaces. Suspend global commands during shortcut capture and let selected Ghostty terminals recover unexpected focus loss without displacing an active text field. Add responder-chain, raw chord, palette selection, focus recovery, and UI regression coverage. The implementation follows Apple's documented key command and physical-keyboard responder-chain behavior.
Specify a content-sized floating Command-K chooser with a single-line input, synchronous pre-query selection, arrow and Return activation, and Escape dismissal through the active responder chain. Define configurable Command-N as the existing New Window action in the selected ready remote tmux session. Explicitly exclude pane creation, connection creation, and settings migration, and preserve physical-device acceptance as the pre-PR gate.
Define four test-first slices for the command registry, terminal topology dispatch, compact floating palette behavior, and live SSH/device acceptance. Preserve the no-migration boundary and require Jesse's physical-keyboard approval before any pull request.
Register New Window as a terminal-scoped app command with a fresh Command-N default and no settings migration. Only route it while the selected remote terminal is ready. Map the command through a tested terminal-surface route to the same topology interaction and model action used by the existing New Window chooser button, preserving tracing and input refocus behavior.
Implement the requested floating Command-K interaction without replacing the existing palette flow. The chooser now sizes itself to a one-line input and at most six result rows, uses the app's existing surfaces, and initializes the first enabled selection before the user types. Publish Up and Down as priority UIKeyCommands from the focused text field alongside Return and Escape. This keeps selection, activation, and dismissal reliable even while UIKit text input owns first-responder focus. Add state, layout, responder, and UI coverage for immediate selection, keyboard navigation before a query, result reset behavior, compact geometry, activation, and dismissal.
Register the active Command-K field with the stable app keyboard command center and publish priority Up, Down, Return, and Escape commands from the hosting controller. Text input remains first responder for immediate typing, while palette controls no longer depend on a transient UITextField being queried for key commands. Keep raw press forwarding as a fallback and unregister the palette owner when its field leaves the window. Add responder-chain coverage proving a focused text field resolves each palette action to the app controller, dispatches the expected callback, and removes those commands after dismissal.
Add a cleanup-harnessed localhost SSH UI test that sends Command-N to a ready terminal, opens the existing current-session window sheet with Command-O, verifies exactly two visible windows, and asks the harness to confirm the remote tmux window count before cleanup.\n\nThis exercises the actual SSH and tmux behavior instead of merely asserting a generated command string.
Register app-wide key commands only on the root hosting controller and terminal-only commands only on the focused Ghostty responder. Use distinct Objective-C actions so responder-chain dispatch cannot be intercepted by the wrong owner.\n\nA live SSH regression showed the previous duplicate Command-K registrations falling through as a literal k in the remote shell while terminal-only Command-N and Command-O still worked. Add responder-chain coverage that enforces single ownership and proves a focused terminal resolves the command palette action through the global host.
Cmd-N could be missed when the Ghostty terminal was not the first responder, most visibly on the first shortcut after the command palette had owned text focus. That focus dependency also made terminal-scoped app shortcuts follow a different delivery path from every other configurable app shortcut. Publish all configured app commands from the stable root hosting controller, register the selected terminal surface as the destination for terminal-scoped commands, and forward those commands only when the router says the selected terminal is ready. The terminal responder keeps its raw key-press resolver as a fallback but no longer publishes a competing UIKeyCommand set. Extend responder-chain coverage to Cmd-N and strengthen the live SSH regression so it opens and dismisses the palette, presses Cmd-N exactly once, verifies exactly two tmux windows, and cleans the generated remote session. Verification before commit: 59 focused responder/router tests passed; the cleanup-gated live SSH Cmd-N test passed and verified an exact remote window count of 2.
Route the current command availability set from the workspace shell through the app keyboard command center into the stable hosting responder. This keeps global commands available throughout the app while preventing terminal-only shortcuts from being claimed on Home or while the selected terminal is disconnected. Apply the same availability guard to raw press resolution and selector dispatch so cached or fallback delivery cannot consume an unavailable binding. Refresh availability whenever the selected-session route context changes. Replace the all-commands responder expectation with a Home/no-terminal route contract and verify that an unavailable Command-N press is left unhandled. Focused router and responder coverage passes 58 tests.
Project terminal input availability from the observed Ghostty surface back into the workspace shell instead of reading a nested adapter from an unrelated observable owner. Key readiness by runtime attempt so reconnecting the same workspace cannot inherit a stale ready state, and prune attempts when their terminal entries leave the active set. This keeps terminal-only shortcuts unpublished until a selected terminal can accept input, publishes them as soon as topology makes the terminal ready, and removes them again on disconnect. Add false-to-true-to-false routing coverage and reconnect isolation tests. Verified with 60 focused router/responder tests and the cleanup-gated live Command-N test, which created exactly one additional remote tmux window and removed its generated session.
Record Jesse's approved Command-K palette refinement before implementation. The design raises the minimum external margin to 20 points, adds a distinct 10-point card inset, and moves the search and result titles to smaller semantic Dynamic Type styles while preserving the established row heights. The specification also defines the outer-card and inner-surface relationship, keeps the existing Remux colors and keyboard behavior unchanged, and sets focused geometry, typography, accessibility, and device acceptance criteria.
Resolve two implementation ambiguities found during the design self-review. The inner content group now uses Remux's established 12-point continuous corner treatment, and the external margin is measured against every edge of the palette presentation container.
Translate the approved spacing and typography design into one focused TDD implementation task and a separate verification/device gate. The plan names the SwiftUI and UIKit edits, records the expected red and green tests, and preserves all existing keyboard, search, color, and row-height contracts. Include rendered geometry coverage, the full unit and UI suites, signed iPhone installation, and an explicit hardware acceptance boundary before any pull request.
Apply the approved Balanced palette treatment with a 20-point presentation margin, a 10-point inset around a clipped grouped surface, and smaller semantic Dynamic Type styles. Preserve the established row heights, colors, keyboard focus, and selection behavior. Cover the named layout contract, the real UIKit field configuration, rendered screen margins, and immediate keyboard focus.
Refresh command-palette results whenever route-aware command availability changes while keeping the active query and preserving a still-valid selection. Cover both readiness directions so an open palette cannot retain stale enabled state. Expose the adapter's structured visible-pane projection through the production viewport snapshot path and exercise deterministic metadata joins, local pane-read omission, cross-session search, and exact window/pane routing. Replace the capture-only clearedBinding seam with a real settings Clear UI mutation test, and remove the reviewed trailing whitespace. This is the local portion of the final whole-branch review requested for the physical-keyboard settings work. GhosttyKit publication, the three-attempt Panes-to-Windows architecture boundary, and physical-device acceptance remain documented external/manual concerns rather than being worked around in this commit.
Separate query-result replacement from readiness-only availability refreshes. Query edits again select the first enabled result, while route/readiness updates preserve a still-valid selection without leaving stale enabled state. Strengthen the palette state regression test so the previously selected item remains present in the new result set; the test failed under the shared preservation behavior and passes with the two contracts separated.
Track the query that produced the displayed palette results. A command-readiness refresh now preserves selection only when it applies to that same query; if it lands during the query debounce, it resets to the first enabled result and records the new query. Add a focused interleaving regression test that failed when a readiness refresh converted a pending query replacement into selection-preserving behavior. Keep the same-query readiness tests explicit and green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughAdds configurable physical-keyboard commands, command-palette search and routing, persisted keyboard settings, terminal viewport snapshots, floating keyboard chrome, responder integration, and unit/UI coverage across the app and Xcode project. ChangesKeyboard command system
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Keyboard as Physical keyboard
participant Host as AppKeyboardCommandHostingController
participant Center as AppKeyboardCommandCenter
participant Root as RemuxWorkspaceShell
participant Terminal as GhosttySurfaceScreen
Keyboard->>Host: Send configured key chord
Host->>Center: Resolve and dispatch command
Center->>Root: Execute global route
Center->>Terminal: Execute selected-terminal route
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
RemuxApp/Sources/App/CommandPaletteView.swift (1)
309-311: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
isDisjoint(with:)to satisfy SwiftLint.♻️ Proposed change
- guard modifierFlags.intersection(actionModifiers).isEmpty else { + guard modifierFlags.isDisjoint(with: actionModifiers) else { return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxApp/Sources/App/CommandPaletteView.swift` around lines 309 - 311, Update the modifierFlags check in the relevant command-palette predicate to use Swift’s isDisjoint(with:) instead of intersection(...).isEmpty, preserving the existing guard behavior and return value.Source: Linters/SAST tools
RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift (1)
154-156: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid the fatal-trap dictionary initializer on a caller-supplied array.
Dictionary(uniqueKeysWithValues:)traps if two entries share apaneID. TodayTmuxTerminalSession.visiblePaneTexts()guarantees uniqueness, but this overload is directly callable (tests already pass literal arrays), so a duplicate pane entry would crash instead of degrading.🛡️ Proposed change
- let textByPaneID = Dictionary( - uniqueKeysWithValues: visiblePaneTexts.map { ($0.paneID, $0.text) } - ) + let textByPaneID = Dictionary( + visiblePaneTexts.map { ($0.paneID, $0.text) }, + uniquingKeysWith: { first, _ in first } + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift` around lines 154 - 156, Update the textByPaneID construction in TmuxTerminalScreenAdapter to use a duplicate-tolerant dictionary-building approach instead of Dictionary(uniqueKeysWithValues:). Ensure repeated paneID entries do not trap and retain the intended value-selection behavior, while preserving the existing mapping from paneID to text.RemuxAppTests/RemuxRootModelTests.swift (1)
78-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the expected clamped size instead of hardcoding
23.The expectation only holds while
TerminalSettings.maximumFontSize == 24; expressing it relative to the constant keeps the test honest if the bound changes.♻️ Proposed change
- XCTAssertEqual(harness.model.terminalSettings.fontSize, 23) + XCTAssertEqual( + harness.model.terminalSettings.fontSize, + TerminalSettings.maximumFontSize - 1 + ) let saved = try await repository.loadSettings() - XCTAssertEqual(saved.fontSize, 23) + XCTAssertEqual(saved.fontSize, TerminalSettings.maximumFontSize - 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxAppTests/RemuxRootModelTests.swift` around lines 78 - 80, Update the assertions in the relevant RemuxRootModel test to derive the expected font size from TerminalSettings.maximumFontSize rather than hardcoding 23, preserving the clamping expectation for both harness.model.terminalSettings.fontSize and saved.fontSize.RemuxAppTests/GhosttyTerminalResponderViewTests.swift (1)
8-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the existing
waitUntilhelper and full window teardown in these two new tests.Both tests pump the run loop for a fixed 20 ms to await first-responder transitions, while the rest of the file uses the
await waitUntil { … }helper (Line 874) for the same condition — the fixed pump is the more flake-prone of the two on loaded CI machines. They also only setwindow.isHidden = trueon teardown, unlike the later tests which additionally resign first responder, remove the view, and clearrootViewController, so a dangling first responder can leak into subsequent tests.♻️ Suggested adjustment (shown for the first test)
- let viewController = UIViewController() - let window = UIWindow(frame: UIScreen.main.bounds) - window.rootViewController = viewController - window.makeKeyAndVisible() - viewController.view.addSubview(view) - defer { window.isHidden = true } + let viewController = UIViewController() + let window = UIWindow(frame: UIScreen.main.bounds) + window.rootViewController = viewController + window.makeKeyAndVisible() + viewController.view.addSubview(view) + defer { + _ = view.resignFirstResponder() + view.removeFromSuperview() + window.isHidden = true + window.rootViewController = nil + } view.update( isEnabled: true, wantsFirstResponder: true, activationToken: 1, sendText: { _ in true }, sendPaste: { _ in true }, sendKeyEvent: { _ in true } ) - RunLoop.main.run(until: Date().addingTimeInterval(0.02)) - XCTAssertTrue(view.isFirstResponder) + XCTAssertTrue(await waitUntil { view.isFirstResponder })Requires marking the tests
async.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxAppTests/GhosttyTerminalResponderViewTests.swift` around lines 8 - 66, Update testSelectedResponderRecoversAfterUnexpectedFocusLoss and testSelectedResponderDoesNotDisplaceExternalTextInput to be async and replace fixed RunLoop.main delays with the existing waitUntil helper for each first-responder transition. Expand both defers to resign the active responder, remove the test view, clear the window’s rootViewController, and hide the window so no responder or view state leaks between tests.RemuxApp/Sources/App/AppKeyboardCommandResponder.swift (1)
178-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
updateUIViewControllernever propagates new content to the hosting controller.
contentis captured once inmakeUIViewController; subsequent SwiftUI updates re-create the representable with a freshAnyView, butcontentController.rootViewis never reassigned, so the child hierarchy keeps rendering the first-built view value (including the environment values captured at that time). TodayRootView()happens to be inert, but this is a latent staleness trap for anything passed through this host.♻️ Proposed fix
struct AppKeyboardCommandHost<Content: View>: UIViewControllerRepresentable { @@ func updateUIViewController( _ controller: AppKeyboardCommandHostingController, context: Context ) { + controller.updateContent(AnyView(content)) controller.update( settings: center.settings, availableCommands: center.availableCommands, commandCenter: center ) center.register(controller) }// AppKeyboardCommandHostingController func updateContent(_ rootView: AnyView) { contentController.rootView = rootView }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxApp/Sources/App/AppKeyboardCommandResponder.swift` around lines 178 - 203, Update AppKeyboardCommandHostingController to expose an updateContent method that assigns the supplied AnyView to contentController.rootView, then call it from updateUIViewController using the representable’s current content before updating settings and commands. Preserve the existing registration and controller-update behavior.RemuxApp/Sources/Ghostty/GhosttyTerminalResponderFocusPolicy.swift (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant disjunction.
isResponderEnabled(isInputAvailable && !transient) always impliesareAppKeyboardCommandsEnabled, so the second clause collapses.♻️ Simplification
var wantsFirstResponder: Bool { isSelected && (keyboardMode.enablesSystemKeyboard || isPhysicalKeyboardConnected) - && (isResponderEnabled || areAppKeyboardCommandsEnabled) + && areAppKeyboardCommandsEnabled }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxApp/Sources/Ghostty/GhosttyTerminalResponderFocusPolicy.swift` around lines 19 - 23, Update the wantsFirstResponder computed property to remove the redundant areAppKeyboardCommandsEnabled disjunction, using only isResponderEnabled alongside the existing selection and keyboard conditions.RemuxAppUITests/RemuxAppUITests.swift (2)
97-139: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSelecting the Command Palette "Set" button by list position is fragile.
setButtons.element(boundBy: setButtons.count - 1)assumes the Command Palette row is always the last "Set" button in document order. If the settings list order changes (e.g., a new command is added after it, or Home ends up last), this silently captures the wrong binding and the subsequent"Press the shortcut for Command Palette"assertion becomes the only thing catching the drift — it would fail, but for a confusing reason. The very next test (testSettingsClearRemovesBinding) already demonstrates a robust, identifier-scoped alternative (keyboard-settings.binding.home).♻️ Suggested more robust selector
- let setButtons = app.buttons.matching( - NSPredicate(format: "label == %@", "Set") - ) - XCTAssertGreaterThan(setButtons.count, 0) - setButtons.element(boundBy: setButtons.count - 1).tap() + let commandPaletteSet = app.buttons + .matching(identifier: "keyboard-settings.binding.commandPalette") + .matching(NSPredicate(format: "label == %@", "Set")) + .firstMatch + XCTAssertTrue(commandPaletteSet.waitForExistence(timeout: 2)) + commandPaletteSet.tap()Please confirm the actual per-row accessibility identifier used in
KeyboardSettingsViewfor the Command Palette binding row (not included in this review batch) before applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxAppUITests/RemuxAppUITests.swift` around lines 97 - 139, Replace the positional selection of the Command Palette “Set” button in testSettingsExposePhysicalKeyboardDefaults with a selector scoped to that binding row, using the actual per-row accessibility identifier exposed by KeyboardSettingsView. Confirm the identifier before updating the test, then tap the row’s Set button while preserving the existing capture and validation assertions.
2794-2816: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
password.tap()fixes focus for this shared helper, but the duplicate inline form-fill intestCaptureDesignReviewScreens(~3438-3440) still lacks it.That test builds the connection form inline instead of calling
fillConnectionForm(), and types intopwdwithout tapping it first. If the added.tap()here is fixing a real focus race (likely introduced by the new keyboard responder chain), the untouched duplicate is still exposed to the same flakiness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@RemuxAppUITests/RemuxAppUITests.swift` around lines 2794 - 2816, The inline connection form setup in testCaptureDesignReviewScreens must match fillConnectionForm’s focus behavior. Locate the pwd secure text field entry and tap the field before typing the password, preserving the existing wait and input flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-07-26-command-palette-focus-and-surfaces.md`:
- Around line 72-82: Update Step 4’s command-palette initialization so results
and the first enabled selection are assigned synchronously in the view
initializer, rather than inside the asynchronous task. Keep the task limited to
yielding after responder reconciliation and then setting isSearchFocused,
preserving immediate keyboard navigation and activation readiness.
In `@docs/superpowers/plans/2026-07-26-physical-keyboard-commands.md`:
- Around line 422-434: Update the final verification checklist near “Step 3: Run
the full Remux simulator suite” to include a separate full non-live RemuxUIOnly
test command. Require recording skipped live tests separately, and make clear
that focused UI tests do not replace this complete UI-only suite.
- Around line 38-40: Update Step 1’s default-chord tests to cover the complete
command registry rather than a fixed nine-command list. Make the test
table-driven over AppKeyboardCommand.allCases, including future bindings such as
increaseFontSize, decreaseFontSize, and newWindow, while preserving the existing
clearing, custom-chord, modifier validation, and duplicate-binding cases.
- Around line 323-339: The “Build and install the forked XCFramework” step
relies on a machine-specific source path and is not reproducible for checkout or
CI. Update Step 3 to document or add a repository-relative artifact and
prerequisite that produces the exact viewport-text-snapshot GhosttyKit build,
while preserving the header verification and pinned-framework replacement
instructions.
In `@docs/superpowers/plans/2026-07-27-command-palette-spacing-and-typography.md`:
- Around line 50-58: Align the palette symbol references in this plan and the
related physical-keyboard plan with the chosen name, replacing
LibraryHomePalette usages with RemuxAppPalette, including rowSurface references.
If compatibility is intentionally required, explicitly define and retain a
LibraryHomePalette alias so all snippets compile consistently.
In
`@docs/superpowers/plans/2026-07-27-floating-command-palette-and-new-remote-window.md`:
- Around line 11-15: Update the global constraint in
docs/superpowers/plans/2026-07-27-floating-command-palette-and-new-remote-window.md:11-15
to require 20-point screen margins instead of 16. Also update the card geometry
requirement and related acceptance language in
docs/superpowers/specs/2026-07-27-floating-command-palette-and-new-remote-window-design.md:20-28
to consistently specify 20 points.
In
`@docs/superpowers/specs/2026-07-27-floating-command-palette-and-new-remote-window-design.md`:
- Around line 52-59: Clarify the Command-N behavior in the terminal-scoped
action contract: when no ready remote terminal is selected, make it an explicit
no-op that consumes the key without sending remote bytes, or intentionally allow
responder fall-through. Update the command router and associated tests to
enforce the chosen behavior, while preserving window creation for ready remote
sessions.
In `@RemuxApp/Sources/Ghostty/GhosttyTerminalResponderView.swift`:
- Around line 404-410: Update the local AppKeyboardCommand handling in
GhosttyTerminalResponderView.pressesBegan so matched commands are dispatched
through the command center, or validated against the current available-command
set, before invoking appKeyboardCommandHandler. Preserve hardware command
mapping and unmatched-press bubbling, and prevent terminal-only commands from
firing when unavailable.
In `@RemuxAppUITests/RemuxAppUITests.swift`:
- Around line 2832-2844: Update the fallback reveal tap in openHomeFromTerminal
to dispatch the normalized-center tap through the existing terminal element’s
coordinate API rather than app.coordinate. Preserve the terminal.exists check
and subsequent optionalTerminalHomeButton lookup.
In `@RemuxAppUITests/ShortcutPaletteUITests.swift`:
- Around line 91-96: Update the reveal-tap fallback in the terminal setup to tap
the confirmed `terminal` element rather than using `app.coordinate`. Preserve
the existing `terminal.ctrl` existence check and fallback timing, but perform
the normalized center tap through `terminal.coordinate` so the terminal surface
receives it.
---
Nitpick comments:
In `@RemuxApp/Sources/App/AppKeyboardCommandResponder.swift`:
- Around line 178-203: Update AppKeyboardCommandHostingController to expose an
updateContent method that assigns the supplied AnyView to
contentController.rootView, then call it from updateUIViewController using the
representable’s current content before updating settings and commands. Preserve
the existing registration and controller-update behavior.
In `@RemuxApp/Sources/App/CommandPaletteView.swift`:
- Around line 309-311: Update the modifierFlags check in the relevant
command-palette predicate to use Swift’s isDisjoint(with:) instead of
intersection(...).isEmpty, preserving the existing guard behavior and return
value.
In `@RemuxApp/Sources/Ghostty/GhosttyTerminalResponderFocusPolicy.swift`:
- Around line 19-23: Update the wantsFirstResponder computed property to remove
the redundant areAppKeyboardCommandsEnabled disjunction, using only
isResponderEnabled alongside the existing selection and keyboard conditions.
In `@RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift`:
- Around line 154-156: Update the textByPaneID construction in
TmuxTerminalScreenAdapter to use a duplicate-tolerant dictionary-building
approach instead of Dictionary(uniqueKeysWithValues:). Ensure repeated paneID
entries do not trap and retain the intended value-selection behavior, while
preserving the existing mapping from paneID to text.
In `@RemuxAppTests/GhosttyTerminalResponderViewTests.swift`:
- Around line 8-66: Update testSelectedResponderRecoversAfterUnexpectedFocusLoss
and testSelectedResponderDoesNotDisplaceExternalTextInput to be async and
replace fixed RunLoop.main delays with the existing waitUntil helper for each
first-responder transition. Expand both defers to resign the active responder,
remove the test view, clear the window’s rootViewController, and hide the window
so no responder or view state leaks between tests.
In `@RemuxAppTests/RemuxRootModelTests.swift`:
- Around line 78-80: Update the assertions in the relevant RemuxRootModel test
to derive the expected font size from TerminalSettings.maximumFontSize rather
than hardcoding 23, preserving the clamping expectation for both
harness.model.terminalSettings.fontSize and saved.fontSize.
In `@RemuxAppUITests/RemuxAppUITests.swift`:
- Around line 97-139: Replace the positional selection of the Command Palette
“Set” button in testSettingsExposePhysicalKeyboardDefaults with a selector
scoped to that binding row, using the actual per-row accessibility identifier
exposed by KeyboardSettingsView. Confirm the identifier before updating the
test, then tap the row’s Set button while preserving the existing capture and
validation assertions.
- Around line 2794-2816: The inline connection form setup in
testCaptureDesignReviewScreens must match fillConnectionForm’s focus behavior.
Locate the pwd secure text field entry and tap the field before typing the
password, preserving the existing wait and input flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd637aab-6075-47fe-9bf8-dd1a974d7aaf
📒 Files selected for processing (47)
Remux.xcodeproj/project.pbxprojRemuxApp/Sources/App/AppKeyboardCommandResponder.swiftRemuxApp/Sources/App/AppKeyboardCommandRouter.swiftRemuxApp/Sources/App/CommandPaletteSearch.swiftRemuxApp/Sources/App/CommandPaletteView.swiftRemuxApp/Sources/App/KeyboardSettingsView.swiftRemuxApp/Sources/App/KeyboardShortcutCaptureView.swiftRemuxApp/Sources/App/PhysicalKeyboardMonitor.swiftRemuxApp/Sources/App/RemuxApp.swiftRemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/App/RemuxRootModel.swiftRemuxApp/Sources/App/RootView.swiftRemuxApp/Sources/Domain/AppKeyboardCommand.swiftRemuxApp/Sources/Domain/KeyboardSettings.swiftRemuxApp/Sources/Domain/TerminalSettings.swiftRemuxApp/Sources/Domain/TerminalViewportSnapshot.swiftRemuxApp/Sources/Ghostty/GhosttyKitControlSurface.swiftRemuxApp/Sources/Ghostty/GhosttySurfaceScreen.swiftRemuxApp/Sources/Ghostty/GhosttyTerminalResponderFocusPolicy.swiftRemuxApp/Sources/Ghostty/GhosttyTerminalResponderView.swiftRemuxApp/Sources/Ghostty/PhysicalKeyboardChromeState.swiftRemuxApp/Sources/Persistence/KeyboardSettingsRepository.swiftRemuxApp/Sources/Tmux/TmuxPaneSurface.swiftRemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swiftRemuxApp/Sources/Tmux/TmuxTerminalSession.swiftRemuxAppTests/AppKeyboardCommandRouterTests.swiftRemuxAppTests/CommandPaletteSearchTests.swiftRemuxAppTests/GhosttyTerminalResponderFocusPolicyTests.swiftRemuxAppTests/GhosttyTerminalResponderViewTests.swiftRemuxAppTests/KeyboardSettingsRepositoryTests.swiftRemuxAppTests/KeyboardSettingsTests.swiftRemuxAppTests/KeyboardShortcutCaptureTests.swiftRemuxAppTests/PhysicalKeyboardChromeStateTests.swiftRemuxAppTests/RemuxRootModelTests.swiftRemuxAppTests/TerminalSettingsTests.swiftRemuxAppTests/TmuxTerminalScreenAdapterTests.swiftRemuxAppTests/TmuxTerminalSessionShutdownDrainTests.swiftRemuxAppUITests/RemuxAppUITests.swiftRemuxAppUITests/ShortcutPaletteUITests.swiftdocs/superpowers/plans/2026-07-26-command-palette-focus-and-surfaces.mddocs/superpowers/plans/2026-07-26-global-font-size-key-bindings.mddocs/superpowers/plans/2026-07-26-physical-keyboard-commands.mddocs/superpowers/plans/2026-07-27-command-palette-spacing-and-typography.mddocs/superpowers/plans/2026-07-27-floating-command-palette-and-new-remote-window.mddocs/superpowers/specs/2026-07-26-physical-keyboard-commands-design.mddocs/superpowers/specs/2026-07-27-command-palette-spacing-and-typography-design.mddocs/superpowers/specs/2026-07-27-floating-command-palette-and-new-remote-window-design.md
Comment on lines
+38
to
+40
| - [ ] **Step 1: Write failing default, clearing, custom-chord, and duplicate tests** | ||
|
|
||
| Create tests asserting all nine default chords, that `nil` clears a command, that a custom modified chord persists in the value, that a chord with no modifier is rejected, and that assigning a chord already used by another command throws `KeyboardSettings.ValidationError.duplicateBinding`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the final command set, not a fixed nine-command subset.
Later plans add three more default bindings (increaseFontSize, decreaseFontSize, and newWindow), so the documented final set contains twelve commands. Make this test table-driven over AppKeyboardCommand.allCases or update it whenever the registry grows; otherwise newly added defaults can ship without coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-26-physical-keyboard-commands.md` around lines
38 - 40, Update Step 1’s default-chord tests to cover the complete command
registry rather than a fixed nine-command list. Make the test table-driven over
AppKeyboardCommand.allCases, including future bindings such as increaseFontSize,
decreaseFontSize, and newWindow, while preserving the existing clearing,
custom-chord, modifier validation, and duplicate-binding cases.
Comment on lines
+52
to
+59
| The action is terminal-scoped: | ||
|
|
||
| - it is available only when a remote session is selected and ready; | ||
| - it creates one new tmux window in that existing remote session; | ||
| - it does not create a Remux connection, a new Remux saved session, or a tmux | ||
| pane; | ||
| - outside a ready terminal it is unavailable and does not claim the key | ||
| sequence. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve unavailable Command-N key ownership.
“Does not claim the key sequence” conflicts with the implementation contract that configured commands receive app priority and unavailable commands are consumed rather than sent to Ghostty. Define the expected behavior explicitly: outside a ready terminal, Command-N should be a no-op that sends no remote bytes, or the responder must intentionally fall through; update the router and tests to match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@docs/superpowers/specs/2026-07-27-floating-command-palette-and-new-remote-window-design.md`
around lines 52 - 59, Clarify the Command-N behavior in the terminal-scoped
action contract: when no ready remote terminal is selected, make it an explicit
no-op that consumes the key without sending remote bytes, or intentionally allow
responder fall-through. Update the command router and associated tests to
enforce the chosen behavior, while preserving window creation for ready remote
sessions.
CodeRabbit identified two correctness gaps in the physical-keyboard responder path. SwiftUI updates could leave AppKeyboardCommandHost rendering its initial content, and Ghostty's raw UIPress fallback dispatched configured chords directly to whichever terminal responder still held focus. Refresh the embedded UIHostingController root view during representable updates. Add an availability-aware command-center entry point, make raw terminal dispatch acknowledge whether the current app owner accepted a command, and defer rejected or stale-owner chords through the responder chain without interpreting them as terminal input. Add behavior tests covering rendered host updates, command availability and shortcut-capture suspension, stale terminal ownership, accepted raw chords, and deferred configured chords. Replace fixed-delay focus tests with bounded condition waits and full UIKit teardown.
Replace positional and app-window coordinate UI interactions with the controls that own each behavior. The keyboard row identifier was overriding every descendant identifier in SwiftUI's accessibility tree, so remove that container identifier and address the Set and Clear buttons by their command-specific identifiers. Also focus the password field before the design-capture input, express the responder policy and modifier test with their direct predicates, and derive the queued font-size expectation from the domain limit. Focused keyboard-settings UI tests and the affected unit suites pass.
Update the implementation records to match the shipped branch: palette results and selection exist synchronously, the established LibraryHomePalette symbol remains in use, all twelve command defaults are covered, and the approved card margin is twenty points. Clarify that unavailable Command-N falls through without claiming the key or sending terminal bytes. Add the full non-live UI gate and replace machine-specific Ghostty paths with the repository-relative source workflow while documenting the remaining remux-ghostty PR h3nock#7 release and checksum dependency for clean checkouts.
This was referenced Jul 31, 2026
Contributor
Author
|
If you want this, but with cleaner history, I've got Codex rebuilding this as a stack of cleaner commits or PRs. I'm very happy to split it into a couple logical pieces. (I was starting with making the button bar have an optional "overlay the window and hide until the screen is tapped" mode) If you'd rather not pollute the codebase with external keyboard functionality, I get that. Let me know what sounds good. |
Open
This was referenced Aug 1, 2026
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.
Summary
Add configurable, app-wide physical-keyboard controls to Remux.
session windows, new windows, panes, attachments, font size, and the command
palette
while exposing terminal-only commands only when the selected terminal is
ready
Cmd-H; Home defaults toCmd-Shift-H, and shortcut capturerejects the system-owned chord
Cmd-Kpalette with immediate text focus, arrow-keyselection, Return activation, and Escape dismissal
hosts, opening the current session's windows, panes, and attachments
result back to its pane
connected; tapping the terminal reveals it over the content without reducing
the terminal viewport
Cmd-Ncreate exactly one new tmux window in the current remote sessionDefault bindings are:
Cmd-Left/Cmd-Right: previous / next windowCmd-Shift-Left/Cmd-Shift-Right: previous / next sessionCmd-Shift-H: HomeCmd-O: current-session windowsCmd-N: new remote windowCmd-P: panesCmd-A: attachmentsCmd-K: command paletteCmd-+/Cmd--: increase / decrease font sizeWhy
Remux should remain fully navigable when an iPad hardware keyboard is attached,
and the terminal should use the space recovered from the software-keyboard
button bar. The command palette provides a single keyboard-first entry point to
existing actions and the text visible across retained terminal surfaces.
Dependency
Visible-terminal search depends on
h3nock/remux-ghostty#7,
which exposes a non-mutating viewport snapshot. That PR is ready for review,
but Remux still needs a new immutable GhosttyKit release asset and checksum
before a clean checkout can update its dependency pin.
Validation
RemuxUIOnly: 12 passed, 26 live-only tests skipped, 0 failedthe feature works on hardware
Summary by CodeRabbit