Load existing tmux sessions in the session switcher - #48
Conversation
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe app adds SSH-based tmux session discovery, per-server refresh coordination, workspace reconciliation, and a new session switcher for active, recent, and discovered sessions. It also updates project wiring, accessibility text, terminal-sheet styling, sorting, unit tests, and UI tests. ChangesTmux session discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant SessionSwitcherView
participant RemuxRootModel
participant RemuxAppDependencies
participant TmuxSessionDiscovery
participant SSH
User->>SessionSwitcherView: Refresh available sessions
SessionSwitcherView->>RemuxRootModel: refreshTmuxSessions(for:)
RemuxRootModel->>RemuxAppDependencies: discoverTmuxSessions(for:)
RemuxAppDependencies->>TmuxSessionDiscovery: discover sessions
TmuxSessionDiscovery->>SSH: Execute tmux list-sessions command
SSH-->>TmuxSessionDiscovery: Return session output
TmuxSessionDiscovery-->>RemuxRootModel: Return parsed session names
RemuxRootModel-->>SessionSwitcherView: Publish discovery state and session rows
🚥 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 |
![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: 896c551248
ℹ️ 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".
| .filter { $0.target.server.id == serverID } | ||
| var includedNames = Set<String>() | ||
| var items = activeSessions.compactMap { session -> ActiveSessionSwitcherItem? in | ||
| guard includedNames.insert(session.target.workspace.sessionName).inserted else { return nil } |
There was a problem hiding this comment.
When two saved workspaces on the same server use the same tmux session name, this name-based deduplication drops one active runtime even though activation and persistence distinguish them by workspace ID and the workspace validator permits duplicate names. In particular, if the older duplicate is currently selected, the more recently opened runtime is retained by the display sort and the selected runtime disappears from the sheet, so it cannot be selected or disconnected there. Deduplicate only discovered available sessions, or otherwise ensure every active workspace ID remains represented.
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 (9)
RemuxApp/Sources/App/RemuxAppDependencies.swift (1)
495-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the reconciliation logic from
ConnectionProfileRepository.swift.This is the same check-dedupe-append logic as
FileBackedConnectionProfileRepository.reconcileDiscoveredWorkspaces. See the consolidated comment for the shared fix.🤖 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/RemuxAppDependencies.swift` around lines 495 - 520, Remove the duplicated check-dedupe-append implementation from reconcileDiscoveredWorkspaces and reuse the existing reconciliation logic in FileBackedConnectionProfileRepository, preserving server validation and snapshot loading behavior.RemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swift (1)
23-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared script preamble to avoid divergence.
discoveryScriptduplicates most oflaunchScript: thePATHfallback line, thetmux=$(printf %b "$1")resolution, and the not-found/not-executable marker lines are identical between the two arrays. Only theTERMexport, the session argument, and the finalexecline differ.Extract the shared lines into one place, parameterized by the final
execstatement, so a future change to PATH handling or marker text does not need to be duplicated correctly in two scripts.♻️ Example refactor
+ private static func resolutionScript(execLine: String, includeTerm: Bool) -> [String] { + var lines = [ + #"PATH="${PATH:+$PATH:}\#(fallbackRemotePath)""#, + "export PATH", + ] + if includeTerm { + lines += ["TERM=xterm-256color", "export TERM"] + } + lines += [ + #"tmux=$(printf %b "$1")"#, + ] + return lines + }Compose
launchScriptanddiscoveryScriptfrom this shared helper plus their distinct resolution/exec lines and the common not-found/not-executable tail.🤖 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/SSHTmuxControlCommandBuilder.swift` around lines 23 - 56, Extract the duplicated PATH setup, tmux argument resolution, and executable/not-found marker handling from launchScript and discoveryScript into a shared helper parameterized by the command-specific final exec portion. Rebuild both scripts using that helper while preserving launchScript’s TERM/session behavior and discoveryScript’s list-sessions behavior.RemuxApp/Sources/Persistence/ConnectionProfileRepository.swift (1)
172-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconciliation logic is duplicated across three repository implementations.
This same reconcile-and-dedupe logic (check server exists, filter empty names, dedupe against existing and newly-added names, append with
.distantPast) is repeated almost verbatim inRemuxAppDependencies.swift'sInMemoryConnectionProfileRepositoryand inDebugConnectionProfileSeederTests.swift's test double. See the consolidated comment for the shared fix.🤖 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/ConnectionProfileRepository.swift` around lines 172 - 204, Consolidate the reconcile-and-dedupe behavior used by ConnectionProfileRepository.reconcileDiscoveredWorkspaces into one shared implementation, and update InMemoryConnectionProfileRepository and the DebugConnectionProfileSeederTests test double to reuse it. Preserve server validation, empty-name filtering, deduplication against existing and newly discovered names, and .distantPast timestamps.RemuxAppTests/DebugConnectionProfileSeederTests.swift (1)
151-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the reconciliation logic from
ConnectionProfileRepository.swift.This test double repeats the same check-dedupe-append logic present in
FileBackedConnectionProfileRepositoryand theInMemoryConnectionProfileRepositoryinRemuxAppDependencies.swift. See the consolidated comment for the shared fix.🤖 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/DebugConnectionProfileSeederTests.swift` around lines 151 - 176, Update reconcileDiscoveredWorkspaces in the test double to reuse the shared reconciliation implementation from ConnectionProfileRepository.swift or the existing repository helper instead of duplicating the server validation, deduplication, and append logic. Preserve the current snapshot-loading behavior and repository contract while removing the repeated implementation.RemuxAppTests/TmuxSessionDiscoveryTests.swift (1)
5-34: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
discover()'s exit-status handling.This file tests
parseSessionNamesand the command builder, but no test callsTmuxSessionDiscovery.discoveritself. Add a test for the zero-sessions case once the fix inTmuxSessionDiscovery.swiftlands, and a test confirming a genuine remote failure still throwsTmuxSessionDiscoveryError.remoteExit. See the consolidated comment for details.🤖 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/TmuxSessionDiscoveryTests.swift` around lines 5 - 34, Add tests invoking TmuxSessionDiscovery.discover for both outcomes: verify a successful remote command with no sessions returns an empty collection, and verify a nonzero remote exit status throws TmuxSessionDiscoveryError.remoteExit. Reuse the existing test doubles or discovery setup used by TmuxSessionDiscovery rather than testing only parseSessionNames or the command builder.RemuxApp/Sources/App/RemuxRootModel.swift (2)
263-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the two
MainActor.assumeIsolatedblocks indeinit.The deinitializer enters the isolation twice. One block keeps teardown ordering explicit and removes the intermediate array.
♻️ Proposed simplification
deinit { - let refreshTasks = MainActor.assumeIsolated { - Array(tmuxSessionRefreshTasks.values) - } - for task in refreshTasks { - task.cancel() - } MainActor.assumeIsolated { + for task in tmuxSessionRefreshTasks.values { + task.cancel() + } stopAllTerminalScreenModels() } }🤖 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/RemuxRootModel.swift` around lines 263 - 268, In RemuxRootModel.deinit, merge the refreshTasks snapshot and task cancellation into a single MainActor.assumeIsolated block. Cancel the values of tmuxSessionRefreshTasks directly within that block, removing the intermediate refreshTasks array while preserving the existing teardown order.
1504-1528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping the injected
dependenciesparameter.
performTmuxSessionRefreshruns on the main actor and can readself.dependenciesdirectly. The parameter and the capture inrefreshTmuxSessionsadd indirection without changing behavior.Also applies to: 1539-1560
🤖 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/RemuxRootModel.swift` around lines 1504 - 1528, Remove the injected dependencies parameter from performTmuxSessionRefresh and have it use self.dependencies directly for tmux discovery and profile reconciliation. Update refreshTmuxSessions and every call site, including the related flow around the reported additional lines, to stop capturing and passing the redundant parameter while preserving existing refresh behavior.RemuxApp/Sources/App/ActiveSessionSwitcherView.swift (1)
222-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a localized plural instead of a manual ternary.
Line 229 builds the plural form in code.
^[\(sessions.count) session](inflect: true)produces the correct form and supports localization later.🤖 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/ActiveSessionSwitcherView.swift` around lines 222 - 233, Update the .loaded branch of the context property to use the inflected localized string format `^[\(sessions.count) session](inflect: true)` instead of manually selecting “session” or “sessions” with a ternary; leave the other discoveryState cases unchanged.RemuxAppTests/ActiveSessionSwitcherProjectionTests.swift (1)
18-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecover the ordering coverage in this test.
The two sessions now belong to different servers, so scoping reduces the result to one item. The test no longer verifies recent-open ordering that its name claims. Add a second session on the selected server, or rename the test to describe scoping and selection only.
🤖 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/ActiveSessionSwitcherProjectionTests.swift` around lines 18 - 33, Update the test around ActiveSessionSwitcherProjection.items so it continues covering recent-open ordering: add another session associated with selected.target.server while preserving the existing selected-session assertions and expected ordering. If ordering is not intended to be tested, instead rename the test to describe only server scoping and selection.
🤖 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/App/RemuxRootModel.swift`:
- Around line 1529-1536: Update the CancellationError branch in the tmux session
discovery refresh flow to first verify isCurrentTmuxSessionRefresh(server,
refreshID: refreshID), then clear tmuxSessionDiscoveryStates[server.id] before
returning. Preserve the existing behavior for stale or explicitly invalidated
refreshes.
In `@RemuxApp/Sources/SSH/TmuxSessionDiscovery.swift`:
- Around line 21-41: The discover() method should return an empty array when
tmux list-sessions exits with the supported “no server running” status and
verified tmux stderr message; preserve TmuxSessionDiscoveryError.remoteExit for
all other non-zero exits. Confirm the exact message variants and supported tmux
versions before implementing the targeted match.
---
Nitpick comments:
In `@RemuxApp/Sources/App/ActiveSessionSwitcherView.swift`:
- Around line 222-233: Update the .loaded branch of the context property to use
the inflected localized string format `^[\(sessions.count) session](inflect:
true)` instead of manually selecting “session” or “sessions” with a ternary;
leave the other discoveryState cases unchanged.
In `@RemuxApp/Sources/App/RemuxAppDependencies.swift`:
- Around line 495-520: Remove the duplicated check-dedupe-append implementation
from reconcileDiscoveredWorkspaces and reuse the existing reconciliation logic
in FileBackedConnectionProfileRepository, preserving server validation and
snapshot loading behavior.
In `@RemuxApp/Sources/App/RemuxRootModel.swift`:
- Around line 263-268: In RemuxRootModel.deinit, merge the refreshTasks snapshot
and task cancellation into a single MainActor.assumeIsolated block. Cancel the
values of tmuxSessionRefreshTasks directly within that block, removing the
intermediate refreshTasks array while preserving the existing teardown order.
- Around line 1504-1528: Remove the injected dependencies parameter from
performTmuxSessionRefresh and have it use self.dependencies directly for tmux
discovery and profile reconciliation. Update refreshTmuxSessions and every call
site, including the related flow around the reported additional lines, to stop
capturing and passing the redundant parameter while preserving existing refresh
behavior.
In `@RemuxApp/Sources/Persistence/ConnectionProfileRepository.swift`:
- Around line 172-204: Consolidate the reconcile-and-dedupe behavior used by
ConnectionProfileRepository.reconcileDiscoveredWorkspaces into one shared
implementation, and update InMemoryConnectionProfileRepository and the
DebugConnectionProfileSeederTests test double to reuse it. Preserve server
validation, empty-name filtering, deduplication against existing and newly
discovered names, and .distantPast timestamps.
In `@RemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swift`:
- Around line 23-56: Extract the duplicated PATH setup, tmux argument
resolution, and executable/not-found marker handling from launchScript and
discoveryScript into a shared helper parameterized by the command-specific final
exec portion. Rebuild both scripts using that helper while preserving
launchScript’s TERM/session behavior and discoveryScript’s list-sessions
behavior.
In `@RemuxAppTests/ActiveSessionSwitcherProjectionTests.swift`:
- Around line 18-33: Update the test around
ActiveSessionSwitcherProjection.items so it continues covering recent-open
ordering: add another session associated with selected.target.server while
preserving the existing selected-session assertions and expected ordering. If
ordering is not intended to be tested, instead rename the test to describe only
server scoping and selection.
In `@RemuxAppTests/DebugConnectionProfileSeederTests.swift`:
- Around line 151-176: Update reconcileDiscoveredWorkspaces in the test double
to reuse the shared reconciliation implementation from
ConnectionProfileRepository.swift or the existing repository helper instead of
duplicating the server validation, deduplication, and append logic. Preserve the
current snapshot-loading behavior and repository contract while removing the
repeated implementation.
In `@RemuxAppTests/TmuxSessionDiscoveryTests.swift`:
- Around line 5-34: Add tests invoking TmuxSessionDiscovery.discover for both
outcomes: verify a successful remote command with no sessions returns an empty
collection, and verify a nonzero remote exit status throws
TmuxSessionDiscoveryError.remoteExit. Reuse the existing test doubles or
discovery setup used by TmuxSessionDiscovery rather than testing only
parseSessionNames or the command builder.
🪄 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: 35a4d696-0a36-4483-a861-8fe08e279c85
📒 Files selected for processing (13)
Remux.xcodeproj/project.pbxprojRemuxApp/Sources/App/ActiveSessionSwitcherView.swiftRemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/App/RemuxRootModel.swiftRemuxApp/Sources/App/RootView.swiftRemuxApp/Sources/Persistence/ConnectionProfileRepository.swiftRemuxApp/Sources/SSH/TmuxSessionDiscovery.swiftRemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swiftRemuxAppTests/ActiveSessionSwitcherProjectionTests.swiftRemuxAppTests/ConnectionProfileRepositoryTests.swiftRemuxAppTests/DebugConnectionProfileSeederTests.swiftRemuxAppTests/RemuxRootModelTests.swiftRemuxAppTests/TmuxSessionDiscoveryTests.swift
| } catch is CancellationError { | ||
| return | ||
| } catch { | ||
| guard isCurrentTmuxSessionRefresh(server, refreshID: refreshID) else { return } | ||
| // Discovery is auxiliary to an already-running terminal. Keep its | ||
| // failure inside the sheet rather than replacing the app route. | ||
| tmuxSessionDiscoveryStates[server.id] = .failed(error.localizedDescription) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Clear the loading state when discovery reports cancellation but the refresh is still current.
invalidateTmuxSessionRefresh removes the discovery state, so cancellation caused by edit or delete is handled. If the discoverer itself throws CancellationError while the refresh is still current, the code returns and leaves tmuxSessionDiscoveryStates[server.id] at .loading. The Sessions sheet then shows "Refreshing…" and disables the Refresh button until the sheet is reopened.
🐛 Proposed fix
} catch is CancellationError {
+ guard isCurrentTmuxSessionRefresh(server, refreshID: refreshID) else { return }
+ tmuxSessionDiscoveryStates[server.id] = .idle
return
} catch {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch is CancellationError { | |
| return | |
| } catch { | |
| guard isCurrentTmuxSessionRefresh(server, refreshID: refreshID) else { return } | |
| // Discovery is auxiliary to an already-running terminal. Keep its | |
| // failure inside the sheet rather than replacing the app route. | |
| tmuxSessionDiscoveryStates[server.id] = .failed(error.localizedDescription) | |
| } | |
| } catch is CancellationError { | |
| guard isCurrentTmuxSessionRefresh(server, refreshID: refreshID) else { return } | |
| tmuxSessionDiscoveryStates[server.id] = .idle | |
| return | |
| } catch { | |
| guard isCurrentTmuxSessionRefresh(server, refreshID: refreshID) else { return } | |
| // Discovery is auxiliary to an already-running terminal. Keep its | |
| // failure inside the sheet rather than replacing the app route. | |
| tmuxSessionDiscoveryStates[server.id] = .failed(error.localizedDescription) | |
| } |
🤖 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/RemuxRootModel.swift` around lines 1529 - 1536, Update
the CancellationError branch in the tmux session discovery refresh flow to first
verify isCurrentTmuxSessionRefresh(server, refreshID: refreshID), then clear
tmuxSessionDiscoveryStates[server.id] before returning. Preserve the existing
behavior for stale or explicitly invalidated refreshes.
896c551 to
faefed5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
RemuxApp/Sources/Ghostty/GhosttyKeyboardChrome.swift (1)
213-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMention discovered sessions in the hint.
The sheet now also lists remote tmux sessions under Available and resumes them. The hint covers active, recent, and new sessions only. Consider "Switch active sessions, resume recent or available sessions, or create a new session."
🤖 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/GhosttyKeyboardChrome.swift` at line 213, Update the accessibilityHint for the session sheet to mention available sessions alongside active and recent sessions, while retaining the existing new-session guidance.RemuxApp/Sources/App/RemuxAppDependencies.swift (2)
287-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the discovery flow ID into the SSH configuration.
The function builds
tracewithflowID: "session.discovery.<serverID>", then callssshConfigurationwithtraceFlowID: nil. Root-level SSH events therefore lose correlation with the discovery flow. SettraceFlowID: trace.flowID(or the same string) so discovery traces stay joinable.🤖 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/RemuxAppDependencies.swift` around lines 287 - 294, Update the sshConfiguration call in the discovery flow to pass trace.flowID as traceFlowID instead of nil, preserving correlation with the RemuxTransportStartupTrace created for the target server.
295-297: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThrow instead of crashing when the SSH root key is missing.
liveTmuxSessionDiscovereris athrowsfunction on an auxiliary path.RemuxRootModel.performTmuxSessionRefreshalready contains discovery failures inside the Sessions sheet. ApreconditionFailurehere terminates the app for a condition that the caller can present as a local failure.♻️ Proposed change
- guard let rootKey = configuration.sshRootKey else { - preconditionFailure("Tmux discovery requires an SSH root key") - } + guard let rootKey = configuration.sshRootKey else { + throw TmuxSessionDiscoveryError.remoteExit( + status: -1, + stderr: "Tmux discovery requires an SSH root key." + ) + }A dedicated error case is preferable to reusing
remoteExit. Do you want me to add one?🤖 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/RemuxAppDependencies.swift` around lines 295 - 297, Replace the preconditionFailure in liveTmuxSessionDiscoverer with a dedicated thrown error for a missing SSH root key, and define that error using the existing error type conventions. Preserve the throws flow so RemuxRootModel.performTmuxSessionRefresh can present the discovery failure in the Sessions sheet instead of terminating the app.RemuxApp/Sources/App/SessionSwitcherView.swift (2)
654-664: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReuse a single
RelativeDateTimeFormatter.
value(for:relativeTo:)allocates and configures a newRelativeDateTimeFormatteron every call. The function runs once per recent row per render pass, and also fromRecentSessionSwitcherRow.accessibilityValueandSessionLibraryRowinRemuxApp/Sources/App/RootView.swift. Formatter creation is comparatively expensive.♻️ Proposed change
struct SessionLastOpenedText: View { let date: Date + private static let relativeFormatter: RelativeDateTimeFormatter = { + let formatter = RelativeDateTimeFormatter() + formatter.dateTimeStyle = .named + formatter.unitsStyle = .abbreviated + return formatter + }() + var body: some View { Text(Self.value(for: date)) } static func value(for date: Date, relativeTo referenceDate: Date = Date()) -> String { let elapsed = referenceDate.timeIntervalSince(date) if elapsed >= 0, elapsed < 60 { return "Opened just now" } - let formatter = RelativeDateTimeFormatter() - formatter.dateTimeStyle = .named - formatter.unitsStyle = .abbreviated - return "Opened \(formatter.localizedString(for: date, relativeTo: referenceDate))" + return "Opened \(Self.relativeFormatter.localizedString(for: date, relativeTo: referenceDate))" } }
SessionLastOpenedTextis used from the main actor only, so a static stored formatter is safe here. Confirm that no background caller exists before adopting.🤖 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/SessionSwitcherView.swift` around lines 654 - 664, Update SessionLastOpenedText.value(for:relativeTo:) to reuse a single static RelativeDateTimeFormatter instead of allocating and configuring one per call. Define and configure the formatter once within SessionLastOpenedText, confirm its callers remain main-actor-only, and preserve the existing formatting and “Opened just now” behavior.
69-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
availableSessionsfilter depends on statement order.
recentIdentitiesis mutated inside thecompactMapthat producesrecentSessions(lines 78-96) and then read by theavailableSessionsfilter (line 111). The result is correct today only because line 70 executes before line 98. A later reordering of these two assignments would silently let recent sessions reappear under Available.Compute the recent identity set explicitly before building
availableSessions, or add a short comment that records the ordering requirement.🤖 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/SessionSwitcherView.swift` around lines 69 - 112, Make the dependency between recent identity collection and available-session filtering explicit in the SessionSwitcherView initialization: compute/populate recentIdentities before constructing availableSessions, rather than relying on the current recentSessions assignment order. Keep the existing deduplication and filtering behavior unchanged so identities represented by recentSessions remain excluded from availableSessions.RemuxApp/Sources/App/RootView.swift (1)
116-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
discoveryStatesis supplied twice in the same call.Line 119 passes
model.tmuxSessionDiscoveryStatesintoSessionSwitcherProjection, and line 140 passes the same dictionary toSessionSwitcherView.discoveryStates. The view uses the second copy only to deriveisRefreshing,failedServerNames, andhasUndiscoveredServer. Moving that derivation intoSessionSwitcherProjectionwould remove the duplicated input and keep the view's surface smaller.🤖 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/RootView.swift` around lines 116 - 140, Move the discovery-state-derived values isRefreshing, failedServerNames, and hasUndiscoveredServer from SessionSwitcherView into SessionSwitcherProjection, using its existing discoveryStates input. Remove the separate discoveryStates parameter from SessionSwitcherView and stop passing it at the call site, while preserving the derived behavior.RemuxAppUITests/RemuxAppUITests.swift (1)
3561-3568: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a settle interval between swipes.
The loop swipes up to eight times with no pause, then asserts
recentRow.isHittable. SwiftUI list scrolling and the disconnect animation need time to settle, so consecutive swipes can overshoot the Recent row or run before it is laid out. Add a shortRunLoop.current.run(until:)inside the loop, matching the polling pattern used at lines 709-711 and 722-729.🤖 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 3561 - 3568, Add a short RunLoop.current.run(until:) settle interval inside the swipe loop around sessionList.swipeUp(), matching the existing polling pattern used elsewhere in the test. Keep the eight-attempt limit and recentRow visibility checks unchanged so each swipe can settle before the next attempt.RemuxAppTests/SessionSwitcherProjectionTests.swift (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe relative-value assertions depend on the test locale.
XCTAssertFalse(longerValue.contains(","))and the"sec"check assume an English-languageRelativeDateTimeFormatteroutput. If CI ever runs the unit tests under a different locale, these assertions can fail even though the behavior is correct. Pin the formatter locale in the test, or assert only on the app-supplied"Opened "prefix.🤖 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/SessionSwitcherProjectionTests.swift` around lines 17 - 23, Update the relative-value assertions in SessionLastOpenedText.value tests to avoid locale-dependent checks on commas or “sec”; either configure a fixed formatter locale for the test or assert only the app-supplied “Opened ” prefix while preserving the existing date-relative behavior.RemuxApp/Sources/Ghostty/TerminalSelectionSheetStyle.swift (1)
61-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CompactCircularChromeButtonStyledrops the pressed state.
makeBodyignoresconfiguration.isPressed, so the close button and the Sessions Refresh button show no visual press feedback. The palette gainedcontrolPressedFillat line 8, which suggests a pressed fill was intended.♻️ Proposed change
func makeBody(configuration: Configuration) -> some View { configuration.label .font(.system(size: 15, weight: .semibold)) .foregroundStyle(TerminalSelectionSheetPalette.primary) .frame(width: 36, height: 36) - .background(TerminalSelectionSheetPalette.controlFill, in: Circle()) + .background( + configuration.isPressed + ? TerminalSelectionSheetPalette.controlPressedFill + : TerminalSelectionSheetPalette.controlFill, + in: Circle() + ) .frame(width: 44, height: 44) .contentShape(Rectangle()) }🤖 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/TerminalSelectionSheetStyle.swift` around lines 61 - 71, Update CompactCircularChromeButtonStyle.makeBody to use configuration.isPressed when selecting the background fill, applying TerminalSelectionSheetPalette.controlPressedFill while pressed and retaining controlFill otherwise. Preserve the existing sizing, shape, and content layout.
🤖 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/App/RemuxRootModel.swift`:
- Around line 1039-1041: Update connectToDiscoveredSession to select the
workspace from the deterministic workspaces(for:) result, preserving the
serverID and sessionName filters, so matching workspaces are ordered by last
opened and the most recently opened one is chosen before falling back to
SavedWorkspace.
In `@RemuxApp/Sources/App/RootView.swift`:
- Around line 114-141: Update the isSessionSwitcherPresented sheet presentation
closure around SessionSwitcherView to trigger model.refreshTmuxSessions when the
sheet opens, while preserving the existing SessionSwitcherView configuration and
callbacks.
---
Nitpick comments:
In `@RemuxApp/Sources/App/RemuxAppDependencies.swift`:
- Around line 287-294: Update the sshConfiguration call in the discovery flow to
pass trace.flowID as traceFlowID instead of nil, preserving correlation with the
RemuxTransportStartupTrace created for the target server.
- Around line 295-297: Replace the preconditionFailure in
liveTmuxSessionDiscoverer with a dedicated thrown error for a missing SSH root
key, and define that error using the existing error type conventions. Preserve
the throws flow so RemuxRootModel.performTmuxSessionRefresh can present the
discovery failure in the Sessions sheet instead of terminating the app.
In `@RemuxApp/Sources/App/RootView.swift`:
- Around line 116-140: Move the discovery-state-derived values isRefreshing,
failedServerNames, and hasUndiscoveredServer from SessionSwitcherView into
SessionSwitcherProjection, using its existing discoveryStates input. Remove the
separate discoveryStates parameter from SessionSwitcherView and stop passing it
at the call site, while preserving the derived behavior.
In `@RemuxApp/Sources/App/SessionSwitcherView.swift`:
- Around line 654-664: Update SessionLastOpenedText.value(for:relativeTo:) to
reuse a single static RelativeDateTimeFormatter instead of allocating and
configuring one per call. Define and configure the formatter once within
SessionLastOpenedText, confirm its callers remain main-actor-only, and preserve
the existing formatting and “Opened just now” behavior.
- Around line 69-112: Make the dependency between recent identity collection and
available-session filtering explicit in the SessionSwitcherView initialization:
compute/populate recentIdentities before constructing availableSessions, rather
than relying on the current recentSessions assignment order. Keep the existing
deduplication and filtering behavior unchanged so identities represented by
recentSessions remain excluded from availableSessions.
In `@RemuxApp/Sources/Ghostty/GhosttyKeyboardChrome.swift`:
- Line 213: Update the accessibilityHint for the session sheet to mention
available sessions alongside active and recent sessions, while retaining the
existing new-session guidance.
In `@RemuxApp/Sources/Ghostty/TerminalSelectionSheetStyle.swift`:
- Around line 61-71: Update CompactCircularChromeButtonStyle.makeBody to use
configuration.isPressed when selecting the background fill, applying
TerminalSelectionSheetPalette.controlPressedFill while pressed and retaining
controlFill otherwise. Preserve the existing sizing, shape, and content layout.
In `@RemuxAppTests/SessionSwitcherProjectionTests.swift`:
- Around line 17-23: Update the relative-value assertions in
SessionLastOpenedText.value tests to avoid locale-dependent checks on commas or
“sec”; either configure a fixed formatter locale for the test or assert only the
app-supplied “Opened ” prefix while preserving the existing date-relative
behavior.
In `@RemuxAppUITests/RemuxAppUITests.swift`:
- Around line 3561-3568: Add a short RunLoop.current.run(until:) settle interval
inside the swipe loop around sessionList.swipeUp(), matching the existing
polling pattern used elsewhere in the test. Keep the eight-attempt limit and
recentRow visibility checks unchanged so each swipe can settle before the next
attempt.
🪄 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: e80594b9-be0b-4a48-902c-a0ae5c529081
📒 Files selected for processing (16)
Remux.xcodeproj/project.pbxprojRemuxApp/Sources/App/ActiveSessionSwitcherView.swiftRemuxApp/Sources/App/RemuxAppDependencies.swiftRemuxApp/Sources/App/RemuxRootModel.swiftRemuxApp/Sources/App/RootView.swiftRemuxApp/Sources/App/SessionSwitcherView.swiftRemuxApp/Sources/Ghostty/GhosttyKeyboardChrome.swiftRemuxApp/Sources/Ghostty/TerminalSelectionSheetStyle.swiftRemuxApp/Sources/Persistence/ConnectionProfileRepository.swiftRemuxApp/Sources/SSH/TmuxSessionDiscovery.swiftRemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swiftRemuxAppTests/ActiveSessionSwitcherProjectionTests.swiftRemuxAppTests/RemuxRootModelTests.swiftRemuxAppTests/SessionSwitcherProjectionTests.swiftRemuxAppTests/TmuxSessionDiscoveryTests.swiftRemuxAppUITests/RemuxAppUITests.swift
💤 Files with no reviewable changes (2)
- RemuxAppTests/ActiveSessionSwitcherProjectionTests.swift
- RemuxApp/Sources/App/ActiveSessionSwitcherView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- RemuxApp/Sources/Tmux/SSHTmuxControlCommandBuilder.swift
- RemuxAppTests/RemuxRootModelTests.swift
| .sheet(isPresented: $isSessionSwitcherPresented) { | ||
| ActiveSessionSwitcherView( | ||
| sessions: ActiveSessionSwitcherProjection.items( | ||
| sessions: model.activeSessions, | ||
| SessionSwitcherView( | ||
| projection: SessionSwitcherProjection( | ||
| snapshot: model.library, | ||
| activeSessions: model.activeSessions, | ||
| discoveryStates: model.tmuxSessionDiscoveryStates, | ||
| selectedSessionID: selectedTerminalID | ||
| ), | ||
| servers: model.library.servers, | ||
| currentServerID: selectedActiveSession?.target.server.id, | ||
| onSelectSession: model.showActiveSession, | ||
| onSelectActiveSession: model.showActiveSession, | ||
| onResumeSession: { workspaceID in | ||
| traceSessionOpenTap(workspaceID) | ||
| Task { await model.connect(to: workspaceID) } | ||
| }, | ||
| onResumeAvailableSession: { serverID, sessionName in | ||
| Task { | ||
| await model.connectToDiscoveredSession( | ||
| named: sessionName, | ||
| on: serverID | ||
| ) | ||
| } | ||
| }, | ||
| onDisconnectSession: model.disconnectActiveSession, | ||
| onCreateSession: beginNewWorkspaceFromTerminal | ||
| onCreateSession: beginNewWorkspaceFromTerminal, | ||
| onRefresh: model.refreshTmuxSessions, | ||
| discoveryStates: model.tmuxSessionDiscoveryStates | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The Sessions sheet does not refresh automatically when it opens.
The PR objective states that the sheet refreshes automatically when opened. This closure only builds SessionSwitcherView; it never calls model.refreshTmuxSessions. Discovery runs at launch and after a runtime reaches .connected. RemuxRootModel.handleAppLifecyclePhase cancels every refresh when the app enters the background, so after a background round trip the sheet shows the previous snapshot until the user taps Refresh.
Add a refresh trigger on presentation.
🐛 Proposed fix
.terminalSelectionSheetPresentationBackground()
.ghosttyTerminalChromePresentation(
model.terminalSettings.theme.terminalChromeColorScheme,
chromeStyle: model.terminalSettings.theme.terminalChromeStyle
)
+ .task { model.refreshTmuxSessions() }refreshTmuxSessions(for:) already coalesces per server, so a duplicate request during an in-flight refresh is a no-op.
🤖 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/RootView.swift` around lines 114 - 141, Update the
isSessionSwitcherPresented sheet presentation closure around SessionSwitcherView
to trigger model.refreshTmuxSessions when the sheet opens, while preserving the
existing SessionSwitcherView configuration and callbacks.
|
thanks @vaayne for starting this and contributing the session discovery work. i polished the ui/ux and added search on top of it. really appreciate the work |
|
@h3nock Thanks，You are awesome. |
2 participants
Summary
Discovery uses a read-only SSH exec channel to run
tmux list-sessions. It does not attach to, resize, switch, create, or otherwise mutate remote tmux sessions.Testing
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests