add a global multipane zoom preference - #52
Conversation
Deploying remux with
|
| Latest commit: |
cd8a1de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b0d4b674.remux-agx.pages.dev |
| Branch Preview URL: | https://feature-settings-organizatio.remux-agx.pages.dev |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds a persisted multipane zoom preference with device-specific defaults, exposes it in terminal settings, and propagates it to tmux sessions. It also replaces pane-refresh workflows with deferred, callback-based tmux mutations and adds coverage for migration, topology, ownership, lifecycle, and UI behavior. ChangesMultipane zoom preference and tmux control
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant TmuxScreenModel
participant TmuxTerminalScreenAdapter
participant TmuxSessionController
participant TmuxTerminalSession
SettingsView->>TmuxScreenModel: update zoomMultipaneWindowsByDefault
TmuxScreenModel->>TmuxTerminalScreenAdapter: apply preference
TmuxTerminalScreenAdapter->>TmuxSessionController: submit deferred window zoom
TmuxSessionController->>TmuxTerminalSession: send grouped tmux mutation
TmuxTerminalSession-->>TmuxSessionController: report command completion
TmuxSessionController-->>TmuxTerminalScreenAdapter: report successful submission
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Deploying getremux with
|
| Latest commit: |
cd8a1de
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0a0f208a.getremux.pages.dev |
| Branch Preview URL: | https://feature-settings-organizatio.getremux.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d24e0e5da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolvedWindowIDs.insert(window.id) | ||
| if window.zoomed != isEnabled { | ||
| targets.append(window.id) |
There was a problem hiding this comment.
When the preference is toggled again before a previously submitted zoom command produces a topology update, this records the window as resolved from stale topology. For example, turning the setting on queues a zoom, then turning it off while the window still appears unzoomed records it as resolved and queues no inverse; when the first command later succeeds, subsequent topology updates skip the window and it remains zoomed despite the saved setting being off. Track in-flight desired states or reevaluate resolved windows after command settlement.
Useful? React with 👍 / 👎.
| onZoomSubmitted: { [weak self] windowID in | ||
| DispatchQueue.main.async { | ||
| self?.pendingZoomOwnershipWindowIDs.insert(windowID) | ||
| } |
There was a problem hiding this comment.
If the user zooms a pane and immediately disconnects the session, ownership is not recorded until this main-queue block runs. prepareForSessionShutdown() can therefore inspect empty ownership sets and skip cleanup while the controller's already-enqueued zoom command still executes before shutdown, leaving Remux's zoom active on the server. Record pending ownership synchronously before returning from the UI action, and remove it if admission fails.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
RemuxAppTests/TmuxSessionControllerClientSizeTests.swift (2)
1121-1128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
paneIDparameter.
assertCommandWriteignorespaneID. Every call site still passes a value, and a wrong value now has no effect on the assertion. Remove the parameter, or use it to assert the command targets that pane.♻️ Proposed refactor
private func assertCommandWrite( _ writes: [String], command: String, - paneID _: TmuxPaneID, file: StaticString = `#filePath`, line: UInt = `#line` ) { XCTAssertEqual(writes, [command + "\n"], file: file, line: line) }Update each call site to drop the
paneID:argument.🤖 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/TmuxSessionControllerClientSizeTests.swift` around lines 1121 - 1128, Remove the unused paneID parameter from assertCommandWrite and update every call site to omit the paneID: argument, leaving the existing write assertion unchanged.
436-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or rename the duplicated shutdown-unzoom test.
testShutdownUnzoomSendsOnlyTheTmuxMutationuses the same fixture, the same call, and the same assertion astestExplicitUnzoomTogglesAnAuthoritativelyZoomedWindowat lines 420-434. It never shuts the controller down, so the name does not describe what it verifies. Either delete it, or make it exercise the real shutdown cleanup path throughrequestSetWindowsZoomedbeforeshutdown().🤖 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/TmuxSessionControllerClientSizeTests.swift` around lines 436 - 446, Remove the duplicate test testShutdownUnzoomSendsOnlyTheTmuxMutation, or rename and revise it to cover the actual shutdown cleanup path by invoking requestSetWindowsZoomed before shutdown(). Preserve testExplicitUnzoomTogglesAnAuthoritativelyZoomedWindow for the direct unzoom mutation assertion.RemuxAppTests/TmuxTerminalScreenAdapterTests.swift (1)
139-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case with more than two panes.
The fixture is used only with
paneCount1 and 2.TmuxMultipaneZoomDefaultPolicy.windowIDsNeedingChangebreaks the pane loop at 2, so itspaneCount == 2guard means "two or more panes". No test proves that a three-pane window is still treated as multipane. A future reader could read== 2as an exact count and change it without failing a test.💚 Proposed additional test
func testMultipaneZoomDefaultTargetsAWindowWithMoreThanTwoPanes() { var policy = TmuxMultipaneZoomDefaultPolicy(isEnabled: true) XCTAssertEqual( policy.windowIDsNeedingChange(in: topology(zoomed: false, paneCount: 3)), [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/TmuxTerminalScreenAdapterTests.swift` around lines 139 - 163, Add a test alongside the existing TmuxMultipaneZoomDefaultPolicy tests that uses topology(zoomed: false, paneCount: 3) with the policy enabled and asserts windowIDsNeedingChange returns [1], confirming windows with more than two panes are treated as multipane.RemuxApp/Sources/Persistence/TerminalSettingsRepository.swift (1)
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
TerminalSettingsdefault literals inRemuxApp/Sources/Persistence/TerminalSettingsRepository.swiftandRemuxApp/Sources/App/RemuxAppDependencies.swift. Both sites hand-write the full field list ofTerminalSettingsto express "the canonical default, withzoomMultipaneWindowsByDefaultoverridden". A new field or a changed default value inTerminalSettingswill not reach either literal.
RemuxApp/Sources/Persistence/TerminalSettingsRepository.swift#L16-L20: builddefaultSettingsfromTerminalSettings.defaultand assignzoomMultipaneWindowsByDefaulton the copy.RemuxApp/Sources/App/RemuxAppDependencies.swift#L333-L339: build the UI-testing settings fromTerminalSettings.defaultand assignzoomMultipaneWindowsByDefault = deviceDefaultZoomMultipaneWindowson the copy.🤖 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/Persistence/TerminalSettingsRepository.swift` around lines 16 - 20, Replace the hand-written TerminalSettings initializer in RemuxApp/Sources/Persistence/TerminalSettingsRepository.swift#L16-L20 with a copy of TerminalSettings.default, then override zoomMultipaneWindowsByDefault with defaultZoomMultipaneWindows. Apply the same change in RemuxApp/Sources/App/RemuxAppDependencies.swift#L333-L339, overriding the copied default with deviceDefaultZoomMultipaneWindows.
🤖 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 `@RemuxApp/Sources/Tmux/TmuxSessionController.swift`:
- Around line 856-869: Update the shutdown flow involving
prepareForSessionShutdown(), submitWindowZoom(), and shutdown() so deferred zoom
intents created during session shutdown are preserved and processed rather than
discarded when shutdown clears deferredWindowZoomIntent. Use a shutdown-safe
queue or drain the deferred intent before clearing shutdown state, ensuring the
cleanup requestSetWindowsZoomed(zoomed: false) is ultimately submitted for
Remux-owned windows.
In `@RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift`:
- Around line 172-173: Update the `.zoomPane` failure handling around
requestSetWindowsZoomed so it removes only the failed window’s pending ownership
ID instead of clearing pendingZoomOwnershipWindowIDs globally. Preserve pending
IDs for successful grouped commands until topology confirms the window changed
or disappeared, allowing them to enter ownedZoomWindowIDs and be cleaned up on
shutdown.
---
Nitpick comments:
In `@RemuxApp/Sources/Persistence/TerminalSettingsRepository.swift`:
- Around line 16-20: Replace the hand-written TerminalSettings initializer in
RemuxApp/Sources/Persistence/TerminalSettingsRepository.swift#L16-L20 with a
copy of TerminalSettings.default, then override zoomMultipaneWindowsByDefault
with defaultZoomMultipaneWindows. Apply the same change in
RemuxApp/Sources/App/RemuxAppDependencies.swift#L333-L339, overriding the copied
default with deviceDefaultZoomMultipaneWindows.
In `@RemuxAppTests/TmuxSessionControllerClientSizeTests.swift`:
- Around line 1121-1128: Remove the unused paneID parameter from
assertCommandWrite and update every call site to omit the paneID: argument,
leaving the existing write assertion unchanged.
- Around line 436-446: Remove the duplicate test
testShutdownUnzoomSendsOnlyTheTmuxMutation, or rename and revise it to cover the
actual shutdown cleanup path by invoking requestSetWindowsZoomed before
shutdown(). Preserve testExplicitUnzoomTogglesAnAuthoritativelyZoomedWindow for
the direct unzoom mutation assertion.
In `@RemuxAppTests/TmuxTerminalScreenAdapterTests.swift`:
- Around line 139-163: Add a test alongside the existing
TmuxMultipaneZoomDefaultPolicy tests that uses topology(zoomed: false,
paneCount: 3) with the policy enabled and asserts windowIDsNeedingChange returns
[1], confirming windows with more than two panes are treated as multipane.
🪄 Autofix
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: fa0511db-4047-4561-80b7-3e955d40c7b3
📒 Files selected for processing (15)
RemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/App/RootView.swiftRemuxApp/Sources/Domain/TerminalSettings.swiftRemuxApp/Sources/Ghostty/GhosttySurfaceScreen.swiftRemuxApp/Sources/Persistence/TerminalSettingsRepository.swiftRemuxApp/Sources/Tmux/TmuxScreenModel.swiftRemuxApp/Sources/Tmux/TmuxSessionController.swiftRemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swiftRemuxApp/Sources/Tmux/TmuxTerminalSession.swiftRemuxAppTests/RemuxRootModelTests.swiftRemuxAppTests/TerminalSettingsRepositoryTests.swiftRemuxAppTests/TerminalSettingsTests.swiftRemuxAppTests/TmuxSessionControllerClientSizeTests.swiftRemuxAppTests/TmuxTerminalScreenAdapterTests.swiftRemuxAppUITests/RemuxAppUITests.swift
💤 Files with no reviewable changes (1)
- RemuxApp/Sources/Tmux/TmuxTerminalSession.swift
| private func submitWindowZoom(_ intent: WindowZoomIntent) { | ||
| preconditionOnWriterQueue() | ||
| guard !awaitingTopologyMutationSettlement else { | ||
| deferredWindowZoomIntent = intent | ||
| return | ||
| } | ||
| deferredWindowZoomIntent = nil | ||
| enqueueWindowsZoom( | ||
| intent.windowIDs, | ||
| desired: intent.desired, | ||
| onZoomSubmitted: intent.onZoomSubmitted, | ||
| drainOutbound: true | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Trace the shutdown-time zoom cleanup ordering.
set -euo pipefail
# Find where prepareForSessionShutdown is invoked relative to controller shutdown.
rg -n -C 12 'prepareForSessionShutdown' --glob '*.swift'
# Inspect the session/model shutdown paths that drive the controller.
rg -n -C 10 'controller\.shutdown|func shutdown' RemuxApp/Sources/Tmux --glob '*.swift'Repository: h3nock/remux
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked Swift files containing relevant identifiers:\n'
git ls-files '*.swift' | xargs rg -n 'prepareForSessionShutdown|func shutdown|deferredWindowZoomIntent|awaitingTopologyMutationSettlement|requestSetWindowsZoomed|enqueueWindowsZoom|submitWindowZoom' || true
printf '\nRelevant Tmux directory files:\n'
git ls-files 'RemuxApp/Sources/Tmux/*.swift' || trueRepository: h3nock/remux
Length of output: 5279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'TmuxScreenModel shutdown path:\n'
sed -n '300,345p' RemuxApp/Sources/Tmux/TmuxScreenModel.swift
printf '\nTmuxTerminalScreenAdapter prepare request:\n'
sed -n '980,1030p' RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift
printf '\nTmuxSessionController zoom/deferred areas:\n'
sed -n '800,895p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
sed -n '950,995p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
sed -n '1014,1030p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
sed -n '1125,1135p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
sed -n '1350,1370p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
printf '\nTmuxSessionController shutdown:\n'
sed -n '320,380p' RemuxApp/Sources/Tmux/TmuxSessionController.swift
printf '\nTmuxTerminalSession shutdown definitions:\n'
sed -n '120,155p' RemuxApp/Sources/Tmux/TmuxTerminalSession.swiftRepository: h3nock/remux
Length of output: 14933
Propagate deferred zoom cleanup through session shutdown.
TmuxScreenModel.stop() calls prepareForSessionShutdown() before session.shutdown(), so a cleanup requestSetWindowsZoomed(zoomed: false) can enter submitWindowZoom while awaitingTopologyMutationSettlement is true. submitWindowZoom then replaces deferredWindowZoomIntent, and shutdown() clears it. Move deferred zoom intents into a shutdown-safe queue or admit them from the deferred store before clearing shutdown state so zoomed Remux-owned windows are not left zoomed.
🤖 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/TmuxSessionController.swift` around lines 856 - 869,
Update the shutdown flow involving prepareForSessionShutdown(),
submitWindowZoom(), and shutdown() so deferred zoom intents created during
session shutdown are preserved and processed rather than discarded when shutdown
clears deferredWindowZoomIntent. Use a shutdown-safe queue or drain the deferred
intent before clearing shutdown state, ensuring the cleanup
requestSetWindowsZoomed(zoomed: false) is ultimately submitted for Remux-owned
windows.
| self?.pendingZoomOwnershipWindowIDs.removeAll() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep ownership tracking for successful grouped zoom commands.
Line 172 clears every pending window when one .zoomPane command fails. requestSetWindowsZoomed submits one command per target window. A successful command can complete before its topology update. If a later command in that group fails, the successful window cannot enter ownedZoomWindowIDs when topology arrives. Session shutdown can then leave a Remux-created zoom on the server.
Track completion and failure per window, or retain each pending ID until topology confirms that window changed or disappeared.
🤖 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 172 -
173, Update the `.zoomPane` failure handling around requestSetWindowsZoomed so
it removes only the failed window’s pending ownership ID instead of clearing
pendingZoomOwnershipWindowIDs globally. Preserve pending IDs for successful
grouped commands until topology confirms the window changed or disappeared,
allowing them to enter ownedZoomWindowIDs and be cleaned up on shutdown.
1 participant
Context
#49 changed multipane windows from automatic tmux zoom to displaying the complete tmux layout by default. Zoom remained available from Panes, but it had to be enabled separately for every window and session.
That works well when the complete layout is useful, especially on larger screens, but makes dense desktop layouts difficult to use from an iPhone.
Closes #51.
What changed
Adds a persisted Zoom multipane windows setting under Windows & Panes.
This gives iPhone users the previous zoomed experience without removing the complete-layout behavior introduced in #49.
Settings behavior
The preference is persisted app-wide. Existing settings without this value adopt the device default:
Validation
Validated with focused tests, the iOS simulator, and a physical iPhone:
Summary by CodeRabbit