Repository navigation
Keep selected workspace visible in sidebar - #3152
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new UI test that verifies sidebar row visibility during workspace navigation and refactors the workspace sidebar rendering to use a WorkspaceListRenderContext and ScrollViewReader-driven conditional programmatic scrolling. Changes
Sequence DiagramsequenceDiagram
participant UITest as UITest
participant ControlSock as ControlSocket
participant App as App/UI
participant ScrollProxy as ScrollViewReader
participant Row as WorkspaceRow
UITest->>ControlSock: request create/select workspace (via test control channel)
ControlSock->>App: deliver workspace create/select command
App->>App: update workspace list and selectedWorkspaceId
App->>ScrollProxy: request scrollSelectedWorkspaceIntoView(selectedWorkspaceId)
ScrollProxy->>App: check renderContext.workspaceIds contains selectedWorkspaceId
alt selected ID present
ScrollProxy->>Row: proxy.scrollTo(selectedWorkspaceId)
Row-->>ScrollProxy: row becomes visible/hittable
else selected ID missing
ScrollProxy-->>App: no-op
end
UITest->>App: query row hittability / check frame for titlebar clearance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Greptile SummaryThis PR fixes a user-reported bug where the selected workspace row could scroll out of view in the sidebar. It wraps the sidebar Confidence Score: 4/5Safe to merge; only P2 findings present — the onAppear scroll timing edge case and the two-commit policy deviation. No P0 or P1 issues. Two P2 findings: a potential no-op scroll on initial appearance before layout completes, and a regression test commit policy deviation per CLAUDE.md. Neither blocks correctness for the common path. Sources/ContentView.swift — onAppear scroll timing; cmuxUITests/WorkspaceSidebarScrollUITests.swift — two-commit policy. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ScrollViewReader wraps ScrollView] --> B{Trigger}
B -->|onAppear| C[scrollSelectedWorkspaceIntoView]
B -->|onChange: selectedTabId| C
B -->|onChange: tabs.map id| C
C --> D{selectedWorkspaceId in workspaceIds?}
D -->|No| E[return — no-op]
D -->|Yes| F[proxy.scrollTo selectedWorkspaceId]
F --> G[SwiftUI scrolls minimum needed to reveal row]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import Foundation | ||
| import XCTest | ||
|
|
||
| final class WorkspaceSidebarScrollUITests: XCTestCase { | ||
| private var socketPath = "" |
There was a problem hiding this comment.
CLAUDE.md requires a two-commit structure for regression tests — Commit 1 adds only the failing test (so CI goes red, proving the test catches the bug), Commit 2 adds the fix (CI goes green). This PR introduces both the scrollSelectedWorkspaceIntoView fix in ContentView.swift and the new WorkspaceSidebarScrollUITests in the same change, so CI history cannot demonstrate that the test would have caught the regression.
Context Used: CLAUDE.md (source)
There was a problem hiding this comment.
Addressed by rewriting the PR into the required two-commit shape: first commit is test-only, second commit contains the implementation.
— Claude Code
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 70-77: The test builds an accessibility label using a zero-based
workspace index which mismatches the app's 1-based label; update the label
construction in the test (the code that sets let label = "\(title), workspace
\(index) of \(count)") to use "\(index + 1)" instead of "\(index)"; ensure this
change is applied where pollUntil calls app.descendants(matching:
.any).matching(NSPredicate(...)) to match the app's localized workspace label
format.
- Around line 217-240: The sendLine() reader currently returns at the first
newline, truncating multi-line responses; update sendLine() (the loop using
accumulator, poll(&pollDescriptor,...), read(fd,...)) to continue reading until
read() returns 0 (peer closed EOF) or the deadline elapses instead of returning
on the first '\n', so accumulator contains the full multi-line reply before
splitting in workspaceLines(); alternatively, after writing the request you can
call shutdown(fd, SHUT_WR) to signal EOF to the server and then read until
read() == 0 so the full multi-line response is received.
🪄 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
Run ID: 4d97f8b3-cbb9-4430-8e7a-ff0ef0c60dc0
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
You’re at about 96% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/WorkspaceSidebarScrollUITests.swift">
<violation number="1" location="cmuxUITests/WorkspaceSidebarScrollUITests.swift:234">
P2: Do not return on the first newline here; `list_workspaces` responses are multi-line, so this truncates the payload to one row and makes workspace counting incorrect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if let chunk = String(bytes: buffer[0..<count], encoding: .utf8) { | ||
| accumulator.append(chunk) | ||
| if let newline = accumulator.firstIndex(of: "\n") { | ||
| return String(accumulator[..<newline]) |
There was a problem hiding this comment.
P2: Do not return on the first newline here; list_workspaces responses are multi-line, so this truncates the payload to one row and makes workspace counting incorrect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/WorkspaceSidebarScrollUITests.swift, line 234:
<comment>Do not return on the first newline here; `list_workspaces` responses are multi-line, so this truncates the payload to one row and makes workspace counting incorrect.</comment>
<file context>
@@ -0,0 +1,242 @@
+ if let chunk = String(bytes: buffer[0..<count], encoding: .utf8) {
+ accumulator.append(chunk)
+ if let newline = accumulator.firstIndex(of: "\n") {
+ return String(accumulator[..<newline])
+ }
+ }
</file context>
There was a problem hiding this comment.
Not applicable on the current branch: this test does not read list_workspaces or parse multi-line socket responses.
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
5730096 to
8abd8cd
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmuxUITests/WorkspaceSidebarScrollUITests.swift (2)
84-99: Minor: guard against busy-looping ifconditionis cheap and RunLoop returns early.
RunLoop.current.run(until:)can return earlier than the requested date if there are no input sources, turning the polling loop into a near-tight CPU loop. Adding a smallThread.sleepfloor (or usingXCTWaiter/expectation(for:evaluatedWith:)) keeps wall-clock pacing predictable in CI.♻️ Suggested tweak
- RunLoop.current.run(until: Date().addingTimeInterval(interval)) + RunLoop.current.run(until: Date().addingTimeInterval(interval)) + // Floor the cadence so the loop pace is stable on CI hosts where the + // run loop has no input sources to schedule against. + Thread.sleep(forTimeInterval: max(0, interval / 2))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift` around lines 84 - 99, The pollUntil function can busy-loop because RunLoop.current.run(until:) may return early; modify pollUntil (the function named pollUntil) to enforce a minimum sleep after the RunLoop call (e.g., Thread.sleep(forTimeInterval: min(interval, 0.01)) or a fixed small floor like 0.01s) so the loop cannot spin tightly when condition() is cheap or RunLoop returns immediately; keep the existing timeout/interval semantics and only add the small sleep after RunLoop.current.run(until:).
58-65: Scope the predicate query to sidebar cells to reduce traversal cost and false positives.
app.descendants(matching: .any)walks the entire accessibility tree on every poll iteration (~20 polls/sec × 20 workspaces). Matching against.cell/.button(and ideally a sidebar container) is significantly faster and avoids matching nested labels (e.g., a tooltip or a duplicated subview that contains the same"workspace X of Y"substring). Also considerLIKE/MATCHESwith anchors, orlabel ENDSWITH %@, since the localized template is"<title>, workspace <i> of <n>"—CONTAINS "workspace 2 of 2"will also accidentally match"workspace 2 of 22"if the count grows.♻️ Proposed refactor
- let position = "workspace \(index) of \(count)" - return pollUntil(timeout: timeout) { - let row = app.descendants(matching: .any) - .matching(NSPredicate(format: "label CONTAINS %@", position)) - .firstMatch - return row.exists && row.isHittable - } + let suffix = "workspace \(index) of \(count)" + return pollUntil(timeout: timeout) { + // Restrict to cells/buttons and anchor on the end of the label to avoid + // matching "workspace 2 of 22" when looking for "workspace 2 of 2". + let predicate = NSPredicate(format: "label ENDSWITH %@", suffix) + let row = app.cells.matching(predicate).firstMatch + return row.exists && row.isHittable + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift` around lines 58 - 65, The predicate is too broad: change the query in the closure (where you currently call app.descendants(matching: .any) and create row) to scope to the sidebar cell/button elements (e.g., matching .cell or .button and, if available, the sidebar container view) instead of the entire accessibility tree; also tighten the predicate from CONTAINS to a boundary-aware match (e.g., ENDSWITH / LIKE / MATCHES with anchors or exact formatting for the localized template) so pollUntil only inspects sidebar rows and avoids false positives like "workspace 2 of 22" and reduces traversal cost.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 84-99: The pollUntil function can busy-loop because
RunLoop.current.run(until:) may return early; modify pollUntil (the function
named pollUntil) to enforce a minimum sleep after the RunLoop call (e.g.,
Thread.sleep(forTimeInterval: min(interval, 0.01)) or a fixed small floor like
0.01s) so the loop cannot spin tightly when condition() is cheap or RunLoop
returns immediately; keep the existing timeout/interval semantics and only add
the small sleep after RunLoop.current.run(until:).
- Around line 58-65: The predicate is too broad: change the query in the closure
(where you currently call app.descendants(matching: .any) and create row) to
scope to the sidebar cell/button elements (e.g., matching .cell or .button and,
if available, the sidebar container view) instead of the entire accessibility
tree; also tighten the predicate from CONTAINS to a boundary-aware match (e.g.,
ENDSWITH / LIKE / MATCHES with anchors or exact formatting for the localized
template) so pollUntil only inspects sidebar rows and avoids false positives
like "workspace 2 of 22" and reduces traversal cost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66f49f7b-7a3a-4101-a09e-b68935942c66
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
✅ Files skipped from review due to trivial changes (2)
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/ContentView.swift
8abd8cd to
b1429db
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmuxUITests/WorkspaceSidebarScrollUITests.swift (1)
28-31: Redundant assertion duplicates final loop iteration.The loop's last iteration (
expectedCount == workspaceCount == 20) already assertswaitForWorkspaceRowHittable(index: 20, count: 20, ...)with the same timeout. This block re-checks the exact same condition immediately after, with no intervening action. Drop it (or replace with something the loop didn't already cover, e.g., asserting that workspace 1 is not hittable to prove the list actually overflowed).♻️ Proposed cleanup
- XCTAssertTrue( - waitForWorkspaceRowHittable(index: workspaceCount, count: workspaceCount, app: app, timeout: 6.0), - "Expected the newly selected bottom workspace to be visible" - ) - app.typeKey("1", modifierFlags: [.command])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift` around lines 28 - 31, Remove the redundant final XCTAssert that repeats the same condition already checked in the loop: the call asserting waitForWorkspaceRowHittable(index: workspaceCount, count: workspaceCount, app: app, timeout: 6.0) duplicates the loop's last iteration. Either delete that assertion entirely or replace it with a complementary check (for example assert that waitForWorkspaceRowHittable(index: 1, count: workspaceCount, app: app, timeout: 1.0) is false or that workspace 1 is not hittable) so the test actually verifies overflow behavior rather than repeating the same check; adjust calls to waitForWorkspaceRowHittable and workspaceCount accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 68-81: The helper launchAndEnsureForeground currently hides launch
failures by wrapping app.launch() in a non-strict XCTExpectFailure and then
treating .runningBackground as success; change it to stop suppressing launch
errors and instead poll for app.state == .runningForeground with a real timeout
(e.g., loop/sleep with deadline) after calling app.launch(), narrowing any
XCTExpectFailure to just the flaky activation call if absolutely necessary;
treat .runningBackground as a failure (call XCTFail with a clear message
including app.state.rawValue) so tests surface the real launch/activation
problem, and ensure any expected-failure annotation only wraps the minimal flaky
activation verification rather than the entire launch sequence in
launchAndEnsureForeground.
- Around line 47-60: The waitForWorkspaceRowHittable query is too broad and uses
a substring match that can collide; change it to target a specific element type
(e.g., .cells, .rows, or .staticTexts) instead of .any and use an anchored
predicate like "label BEGINSWITH %@" or an equality check so the position string
matches exactly (avoid "CONTAINS"); update the query in
waitForWorkspaceRowHittable to restrict descendants(matching: .cell) (or the
correct row element type) and use NSPredicate(format: "label BEGINSWITH %@",
position) or "label == %@", then check that that element’s exists && isHittable
to avoid matching inner text descendants or indexed collisions.
---
Nitpick comments:
In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 28-31: Remove the redundant final XCTAssert that repeats the same
condition already checked in the loop: the call asserting
waitForWorkspaceRowHittable(index: workspaceCount, count: workspaceCount, app:
app, timeout: 6.0) duplicates the loop's last iteration. Either delete that
assertion entirely or replace it with a complementary check (for example assert
that waitForWorkspaceRowHittable(index: 1, count: workspaceCount, app: app,
timeout: 1.0) is false or that workspace 1 is not hittable) so the test actually
verifies overflow behavior rather than repeating the same check; adjust calls to
waitForWorkspaceRowHittable and workspaceCount accordingly.
🪄 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
Run ID: 3e218a45-5562-44d2-b134-65ba1564d2e0
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
b1429db to
1e467f4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
9453-9455: Pass snapshots/actions intoTabItemViewinstead of the stores.This refactor moves a lot of row state up, but Line 9454 and Line 9455 still inject
tabManagerandnotificationStoreintoTabItemView. That keeps the row below theForEachboundary while reading global stores in its body/context menu, which undermines the invalidation isolation this split is trying to achieve. Please finish the decoupling here by threading immutable row snapshots plus action closures instead of the store references.As per coding guidelines, "In any SwiftUI panel whose
bodycontains aLazyVStack/LazyHStack/List/ForEachof rows, no view below that boundary may hold a reference to anObservableObject/@Observablestore ... Rows and drop-gaps receive immutable value snapshots plus closure action bundles only to prevent re-render feedback loops."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 9453 - 9455, TabItemView is still receiving Observable stores (tabManager, notificationStore); change its init and call site so it accepts an immutable TabSnapshot (or similar row model) and a bundle of action closures (e.g., onSelectTab, onCloseTab, onToggleNotification) instead of the store references; update the ForEach to build those snapshots and closures from tabManager/notificationStore outside the row boundary and pass only the value and closures into TabItemView, and modify TabItemView's stored properties and any usages of tabManager/notificationStore inside its body/context menu to use the passed-in snapshot and closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/ContentView.swift`:
- Around line 9453-9455: TabItemView is still receiving Observable stores
(tabManager, notificationStore); change its init and call site so it accepts an
immutable TabSnapshot (or similar row model) and a bundle of action closures
(e.g., onSelectTab, onCloseTab, onToggleNotification) instead of the store
references; update the ForEach to build those snapshots and closures from
tabManager/notificationStore outside the row boundary and pass only the value
and closures into TabItemView, and modify TabItemView's stored properties and
any usages of tabManager/notificationStore inside its body/context menu to use
the passed-in snapshot and closures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a9ec654-0f3a-4427-8048-5a83637d2983
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
✅ Files skipped from review due to trivial changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxUITests/WorkspaceSidebarScrollUITests.swift
1e467f4 to
2b1bf0d
Compare
![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: 2b1bf0d1b8
ℹ️ 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".
| .onChange(of: renderContext.workspaceIds) { _, tabIds in | ||
| scrollSelectedWorkspaceIntoView(scrollProxy, workspaceIds: tabIds) | ||
| } |
There was a problem hiding this comment.
The new onChange for workspaceIds triggers scrollTo(selectedTabId) even when the selected workspace did not change, so any reorder/add/remove operation can forcibly jump the sidebar back to the selected row. In particular, SidebarTabDropDelegate.performDrop mutates tabManager.tabs without changing selectedTabId, so dropping a dragged row can snap the list away from the drop area and disrupt repeated reorders when the selected workspace is elsewhere. This should be gated to selection-driven changes (or to cases where the selected row actually became non-visible).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by gating workspace-id-change scrolling to the case where the selected workspace newly appears in the list. Pure reorder/list changes no longer force-scroll back to the selected row.
— Claude Code
2b1bf0d to
85beb26
Compare
85beb26 to
b094e9c
Compare
b094e9c to
e194ceb
Compare
This branch was successfully deployed
1 participant
Summary
Testing
Issues
Summary by cubic
Keep the selected workspace visible in the sidebar by auto-scrolling the active row into view and reserving space below the titlebar controls. Refactors the sidebar into smaller pieces and adds a UI test to prevent regressions.
Bug Fixes
.id(tab.id)and defer scrolling until rows lay out viaSidebarWorkspaceRowIdsPreferenceKeyto prevent jitter.WorkspaceSidebarScrollUITeststo verify visibility during repeated Cmd+N and Cmd+1, and ensure the first row stays below the titlebar controls.Refactors
workspaceScrollArea,workspaceScrollContent,workspaceRows, andworkspaceRow; introduceWorkspaceListRenderContext; centralize scroll logic inrequestSelectedWorkspaceScroll/flushPendingSelectedWorkspaceScrolland tracklaidOutWorkspaceRowIds/pendingSelectedWorkspaceScrollId.Written for commit e194ceb. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests