redesign pane and window previews - #64
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 (8)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pane selection sheet now uses topology regions and scrollable pane preview cards. Preview sessions receive available width directly. Layout metrics separate visual tile size from capture size. Tests cover topology geometry, picker sizing, preview freshness, rendered pixels, and renamed pane controls. ChangesPane Preview Picker
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GhosttySurfaceScreen
participant GhosttySurfaceSelectionSheet
participant PanePreviewLayout
participant TmuxTerminalScreenAdapter
participant GhosttyPanePreviewSession
User->>GhosttySurfaceScreen: open pane selection sheet
GhosttySurfaceScreen->>PanePreviewLayout: calculate content height
GhosttySurfaceScreen->>GhosttySurfaceSelectionSheet: pass content height
GhosttySurfaceSelectionSheet->>PanePreviewLayout: calculate topology and picker metrics
GhosttySurfaceSelectionSheet->>TmuxTerminalScreenAdapter: request pane preview
TmuxTerminalScreenAdapter->>GhosttyPanePreviewSession: create session with available width
GhosttyPanePreviewSession-->>GhosttySurfaceSelectionSheet: provide rendered preview
User->>GhosttySurfaceSelectionSheet: select topology region or preview card
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 |
![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: 6fa0303acd
ℹ️ 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".
| itemCount: itemCount, | ||
| availableWidth: currentSheetContentWidth(), | ||
| maximumHeight: currentPaneMapMaximumHeight() | ||
| maximumContentHeight: UIScreen.main.bounds.height * 0.68 |
There was a problem hiding this comment.
Because Remux supports landscape orientations, sizing pane content to UIScreen.main.bounds.height * 0.68 leaves no room for the scaffold's additional 158 points of fixed header, context, spacing, and actions. On an 852×393 landscape phone, for example, this requests an approximately 425-point detent; combined with cards whose height is derived from the much larger landscape width, the fixed root content cannot fit and the action bar or picker content is clipped. Calculate the content cap from the available detent height after subtracting TerminalSelectionSheetLayout chrome.
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: 3
🧹 Nitpick comments (2)
RemuxAppTests/PanePreviewLayoutTests.swift (1)
54-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the topology fallback path.
The fixture uses gapped frames, so
partitionalways succeeds. No test covers the case wherePaneTopology(panes:)returns nil andtopologyMetricsreturns nil. That path drives the empty-diagram fallback inGhosttySurfaceSelectionSheet. Add a case with a layout that cannot be split, and asserttopologyMetricsreturns nil.🤖 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/PanePreviewLayoutTests.swift` around lines 54 - 84, Add a test alongside testTopologyPreservesSplitStructureWithEqualPaneArea that supplies a topology fixture whose panes cannot be recursively partitioned, causing PaneTopology(panes:) to return nil. Assert that PanePreviewLayout.topologyMetrics(panes:size:) also returns nil, covering the empty-diagram fallback path used by GhosttySurfaceSelectionSheet.RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift (1)
558-571: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the pass-through wrapper.
makePanePreviewSessionnow forwards the same two parameters tonewPanePreviewSession. The former sizing branch is gone, so the indirection adds no behavior. InlinenewPanePreviewSessioninto the protocol method.🤖 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 558 - 571, Remove the pass-through makePanePreviewSession wrapper and move its session-creation logic into the protocol method directly. Delete newPanePreviewSession once its implementation is inlined, preserving the existing leafIDs and previewAvailableWidth behavior.
🤖 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/Ghostty/GhosttySurfaceSelectionSheet.swift`:
- Around line 657-658: Update GhosttySurfaceSelectionSheet.accessibilityLabel to
use the ", active" suffix for the selected pane, matching
GhosttyWindowSelectionTile and the existing RemuxAppUITests expectations; keep
the unselected label unchanged.
- Around line 305-310: Update the pane dictionaries in the view body around
panesByID and displayIndices to use the uniquing initializer with an explicit
duplicate-resolution rule, so duplicate pane IDs do not trap. Preserve the
existing ordered-pane selection and one-based display indices, keeping the first
or otherwise established pane consistently when IDs repeat.
In `@RemuxApp/Sources/Ghostty/PanePreviewLayout.swift`:
- Around line 204-211: Update the layout calculation around captureWidth and
captureHeight so capturePointSize matches the tile aspect ratio used by
GhosttyRenderedPreviewSurface. Derive captureHeight from previewCardHeightRatio
and the inset-adjusted captureWidth, rather than defaultPreviewAspectRatio,
while preserving the existing minimum width and tile sizing behavior.
---
Nitpick comments:
In `@RemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swift`:
- Around line 558-571: Remove the pass-through makePanePreviewSession wrapper
and move its session-creation logic into the protocol method directly. Delete
newPanePreviewSession once its implementation is inlined, preserving the
existing leafIDs and previewAvailableWidth behavior.
In `@RemuxAppTests/PanePreviewLayoutTests.swift`:
- Around line 54-84: Add a test alongside
testTopologyPreservesSplitStructureWithEqualPaneArea that supplies a topology
fixture whose panes cannot be recursively partitioned, causing
PaneTopology(panes:) to return nil. Assert that
PanePreviewLayout.topologyMetrics(panes:size:) also returns nil, covering the
empty-diagram fallback path used by GhosttySurfaceSelectionSheet.
🪄 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: ad7f5e6a-1df5-4218-a72d-1a6768b81ae8
📒 Files selected for processing (10)
RemuxApp/Sources/Ghostty/GhosttyPanePreviewSession.swiftRemuxApp/Sources/Ghostty/GhosttySurfaceScreen.swiftRemuxApp/Sources/Ghostty/GhosttySurfaceSelectionSheet.swiftRemuxApp/Sources/Ghostty/GhosttyTerminalScreenModeling.swiftRemuxApp/Sources/Ghostty/PanePreviewLayout.swiftRemuxApp/Sources/Ghostty/TerminalSelectionSheetStyle.swiftRemuxApp/Sources/Tmux/TmuxTerminalScreenAdapter.swiftRemuxAppTests/GhosttyPanePreviewSessionTests.swiftRemuxAppTests/PanePreviewLayoutTests.swiftRemuxAppUITests/RemuxAppUITests.swift
💤 Files with no reviewable changes (1)
- RemuxApp/Sources/Ghostty/TerminalSelectionSheetStyle.swift
| let panesByID = Dictionary(uniqueKeysWithValues: projection.panes.map { ($0.id, $0) }) | ||
| let orderedPanes = topology?.orderedPaneIDs.compactMap { panesByID[$0] } | ||
| ?? projection.panes | ||
| let displayIndices = Dictionary( | ||
| uniqueKeysWithValues: orderedPanes.enumerated().map { ($0.element.id, $0.offset + 1) } | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid trapping on duplicate pane IDs in a view body.
Dictionary(uniqueKeysWithValues:) traps at runtime when a key repeats. Both dictionaries are built from projection data inside body, so a duplicate pane ID crashes the sheet instead of degrading. GhosttyPanePreviewSession already de-duplicates leaf IDs through Self.unique, and testDuplicatePaneIDsProduceOneCapture covers duplicate IDs, so duplicates are treated as possible upstream.
Use the uniquing initializer.
🛡️ Proposed fix
- let panesByID = Dictionary(uniqueKeysWithValues: projection.panes.map { ($0.id, $0) })
+ let panesByID = Dictionary(
+ projection.panes.map { ($0.id, $0) },
+ uniquingKeysWith: { first, _ in first }
+ )
let orderedPanes = topology?.orderedPaneIDs.compactMap { panesByID[$0] }
?? projection.panes
- let displayIndices = Dictionary(
- uniqueKeysWithValues: orderedPanes.enumerated().map { ($0.element.id, $0.offset + 1) }
- )
+ let displayIndices = Dictionary(
+ orderedPanes.enumerated().map { ($0.element.id, $0.offset + 1) },
+ uniquingKeysWith: { first, _ in first }
+ )📝 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.
| let panesByID = Dictionary(uniqueKeysWithValues: projection.panes.map { ($0.id, $0) }) | |
| let orderedPanes = topology?.orderedPaneIDs.compactMap { panesByID[$0] } | |
| ?? projection.panes | |
| let displayIndices = Dictionary( | |
| uniqueKeysWithValues: orderedPanes.enumerated().map { ($0.element.id, $0.offset + 1) } | |
| ) | |
| let panesByID = Dictionary( | |
| projection.panes.map { ($0.id, $0) }, | |
| uniquingKeysWith: { first, _ in first } | |
| ) | |
| let orderedPanes = topology?.orderedPaneIDs.compactMap { panesByID[$0] } | |
| ?? projection.panes | |
| let displayIndices = Dictionary( | |
| orderedPanes.enumerated().map { ($0.element.id, $0.offset + 1) }, | |
| 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/Ghostty/GhosttySurfaceSelectionSheet.swift` around lines 305
- 310, Update the pane dictionaries in the view body around panesByID and
displayIndices to use the uniquing initializer with an explicit
duplicate-resolution rule, so duplicate pane IDs do not trap. Preserve the
existing ordered-pane selection and one-based display indices, keeping the first
or otherwise established pane consistently when IDs repeat.
| private var accessibilityLabel: String { | ||
| isSelected ? "Selected pane" : "Pane" | ||
| isSelected ? "Pane \(displayIndex), selected" : "Pane \(displayIndex)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The pane accessibility label no longer matches the pane profiling UI test.
The label is now "Pane \(displayIndex), selected" for the active pane. RemuxAppUITests.swift lines 912-918 read firstPaneTile.label.hasSuffix(", active") and assert that exactly one pane label ends with ", active". With the new label, neither pane matches, and XCTAssertNotEqual(firstPaneIsActive, secondPaneIsActive) fails.
GhosttyWindowSelectionTile still ends its label with ", active". Align the pane label with the window label, or update the UI test to read ", selected".
🐛 Proposed fix in `GhosttyPanePreviewCard`
private var accessibilityLabel: String {
- isSelected ? "Pane \(displayIndex), selected" : "Pane \(displayIndex)"
+ isSelected ? "Pane \(displayIndex), active" : "Pane \(displayIndex)"
}📝 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.
| private var accessibilityLabel: String { | |
| isSelected ? "Selected pane" : "Pane" | |
| isSelected ? "Pane \(displayIndex), selected" : "Pane \(displayIndex)" | |
| private var accessibilityLabel: String { | |
| isSelected ? "Pane \(displayIndex), active" : "Pane \(displayIndex)" |
🤖 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/GhosttySurfaceSelectionSheet.swift` around lines 657
- 658, Update GhosttySurfaceSelectionSheet.accessibilityLabel to use the ",
active" suffix for the selected pane, matching GhosttyWindowSelectionTile and
the existing RemuxAppUITests expectations; keep the unselected label unchanged.
| let captureWidth = max(1, tileWidth - previewCaptureHorizontalInset * 2) | ||
| let captureHeight = ceil(captureWidth / defaultPreviewAspectRatio) | ||
| let tileHeight = ceil(tileWidth * previewCardHeightRatio) | ||
| return .init( | ||
| columnCount: columnCount, | ||
| tilePointSize: CGSize(width: tileWidth, height: tileHeight), | ||
| previewPointSize: CGSize(width: previewWidth, height: previewHeight), | ||
| gridSpacing: windowGridSpacing, | ||
| tilePadding: tilePadding | ||
| capturePointSize: CGSize(width: captureWidth, height: captureHeight), | ||
| gridSpacing: windowGridSpacing |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the rendered preview surface consumes the requested size.
fd -t f 'GhosttyRenderedPreviewSurface*' || true
rg -n -C 15 'struct GhosttyRenderedPreviewSurface' --type=swift
rg -n -C 5 'aspectRatio|resizable|scaledToFill|scaledToFit|clipped' --type=swift -g '*Ghostty*Preview*'Repository: h3nock/remux
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' 'Candidate preview files:'
git ls-files | grep -Ei '(Preview|Pane).*\.(swift|m|mm|h)$' | head -200
printf '%s\n' 'Preview-related symbols and image layout APIs:'
rg -n -S -C 12 \
'GhosttyRenderedPreviewSurface|RenderedPreviewSurface|capturePointSize|tilePointSize|defaultPreviewAspectRatio|previewCardHeightRatio|scaledToFill|scaledToFit|aspectRatio|resizable|clipped' \
--glob '*.swift' --glob '*.m' --glob '*.mm' --glob '*.h' .Repository: h3nock/remux
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' 'Rendered surface and tile composition:'
sed -n '532,604p' RemuxApp/Sources/Ghostty/GhosttySurfaceSelectionSheet.swift
printf '%s\n' 'Capture-size call sites:'
rg -n -C 12 'capturePointSize|windowPhysicalPixelBudget|RenderedPreview' \
RemuxApp/Sources/Ghostty/GhosttyPanePreviewSession.swift \
RemuxApp/Sources/Ghostty/GhosttySurfaceSelectionSheet.swift \
RemuxApp/Sources/Ghostty/PanePreviewLayout.swift
printf '%s\n' 'Deterministic aspect-ratio check:'
python3 - <<'PY'
tile_width = 175
tile_height = 193
capture_width = 159
capture_height = 120
print(f"capture ratio: {capture_width / capture_height:.6f}")
print(f"surface ratio: {tile_width / tile_height:.6f}")
scale = max(tile_width / capture_width, tile_height / capture_height)
scaled = (capture_width * scale, capture_height * scale)
crop_fraction = (tile_width * tile_height) / (scaled[0] * scaled[1])
print(f"aspect-fill scale: {scale:.6f}")
print(f"scaled image: {scaled[0]:.3f} x {scaled[1]:.3f}")
print(f"visible source fraction: {crop_fraction:.6f}")
print(f"visible source height fraction: {tile_width / (capture_width * tile_height / tile_width):.6f}")
PYRepository: h3nock/remux
Length of output: 23803
Match the capture aspect ratio to the tile aspect ratio
GhosttyRenderedPreviewSurface uses .aspectRatio(contentMode: .fill) and .clipped() with layout.tilePointSize. At 361pt width, the 159×120 capture is cropped horizontally by about 32% to fill the 175×193 tile. Derive capturePointSize.height from previewCardHeightRatio, including the horizontal inset.
🤖 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/PanePreviewLayout.swift` around lines 204 - 211,
Update the layout calculation around captureWidth and captureHeight so
capturePointSize matches the tile aspect ratio used by
GhosttyRenderedPreviewSurface. Derive captureHeight from previewCardHeightRatio
and the inset-adjusted captureWidth, rather than defaultPreviewAspectRatio,
while preserving the existing minimum width and tile sizing behavior.
Deploying remux with
|
| Latest commit: |
1e82666
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://94241837.remux-agx.pages.dev |
| Branch Preview URL: | https://feature-pane-picker-topology.remux-agx.pages.dev |
Deploying getremux with
|
| Latest commit: |
1e82666
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6afaa85a.getremux.pages.dev |
| Branch Preview URL: | https://feature-pane-picker-topology.getremux.pages.dev |
1 participant
Summary
Validation
Closes #61
Summary by CodeRabbit