Repository navigation
Fix stale surface-to-panel rebinding - #6581
Merged
Merged
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:
📝 WalkthroughWalkthrough
ChangesSurface-to-panel mapping API refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes stale surface-to-panel rebinding (#6536) by making the invalid state unrepresentable:
Confidence Score: 5/5The change is safe to merge — it is a targeted refactor of a single data structure that was the confirmed root cause of a focus/input routing bug. All write paths to the surface-to-panel map have been migrated to the new exclusive-binding API. The bindSurface logic correctly handles all rebind combinations (panel moves to new surface, surface moves to new panel, cross-rebind), and removeSurfaceMappings correctly uses the reverse index so a stale close callback cannot drop a live mapping. The four regression tests exercise all the cases called out in the PR description. No actor isolation, blocking runtime, test-only seam, or algorithmic complexity concerns were found. No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeModel.swift (1)
68-73: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAvoid full-map rebuild on each surface bind.
Line 69 currently rescans and rebuilds the entire mapping for every bind. In production batch/rebind paths, this creates avoidable O(n²) behavior and extra allocations. Prefer maintaining a reverse index (
panelId -> surfaceId) so rebinding can remove the prior surface in O(1) and update both maps directly.As per coding guidelines, “for production code over scalable user data, flag nested full-collection scans [and] per-target rescans for batch actions.”
🤖 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 `@Packages/macOS/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeModel.swift` around lines 68 - 73, The bindSurface method currently filters and rebuilds the entire surfaceIdToPanelId dictionary on every call, creating O(n²) behavior during batch operations. To fix this, introduce a reverse mapping data structure that maintains panelId to surfaceId associations. In the bindSurface method, instead of using the filter operation to rebuild the entire map, check if the panelId already has an existing surfaceId mapped to it using the reverse index, remove that old mapping from surfaceIdToPanelId, and then directly insert the new surfaceId to panelId binding. Update both the forward and reverse maps simultaneously to keep them in sync, enabling O(1) rebinding operations without full dictionary rescans.Source: Coding guidelines
🤖 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 `@Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/PaneTreeModelTests.swift`:
- Around line 121-138: The test closedPanelCleanupKeepsReboundSurfaceMapping
currently uses two different surface IDs (closedPanelTabId and reboundTabId)
which does not accurately test the bug scenario of a surface being rebound
before cleanup runs. Refactor the test to use a single surface ID that is first
bound to closedPanelId, then rebound to livePanelId before calling
removeSurfaceMappings for the closed panel. After the cleanup, assert that the
surface mapping still points to livePanelId and the stale alias to closedPanelId
has been removed, directly covering the actual stale-rebind scenario described
in the bug.
---
Outside diff comments:
In `@Packages/macOS/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeModel.swift`:
- Around line 68-73: The bindSurface method currently filters and rebuilds the
entire surfaceIdToPanelId dictionary on every call, creating O(n²) behavior
during batch operations. To fix this, introduce a reverse mapping data structure
that maintains panelId to surfaceId associations. In the bindSurface method,
instead of using the filter operation to rebuild the entire map, check if the
panelId already has an existing surfaceId mapped to it using the reverse index,
remove that old mapping from surfaceIdToPanelId, and then directly insert the
new surfaceId to panelId binding. Update both the forward and reverse maps
simultaneously to keep them in sync, enabling O(1) rebinding operations without
full dictionary rescans.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83e398be-2d43-4616-948e-9c544f1398e8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Packages/macOS/CmuxPanes/Sources/CmuxPanes/Model/PaneTreeModel.swiftPackages/macOS/CmuxPanes/Tests/CmuxPanesTests/PaneTreeModelTests.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swift
azooz2003-bit
added a commit
that referenced
this pull request
Jul 3, 2026
…ace-map mutations through paneTree.bindSurface/removeSurfaceMapping, get-only bridge; thread allowTextBoxFocusDefault to newTerminalSurfaceLocal; add DetachedSurfaceTransfer.directoryDisplayLabel (round 50-fix)
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6536.
Root cause by code analysis: the workspace pane model allowed the same cmux panel/PTY id to be reachable from more than one Bonsplit tab id. Forward lookup was keyed by surface id, while reverse lookup scanned a dictionary that could contain stale duplicate panel mappings. If lifecycle churn rebound a live panel to a new surface while an old surface mapping survived, a tab could resolve to another tab's live terminal panel until restart rebuilt clean state.
This PR makes the invalid state unrepresentable:
PaneTreeModel.surfaceIdToPanelIdis now read-only outside the model.bindSurface,removeSurfaceMapping, andremoveSurfaceMappings.Testing
9d602f494for stale surface rebinding.xcodebuild.Localization
No user-facing strings changed. Localization audit: changed Swift production code only for internal bookkeeping APIs and comments, plus tests; no new UI text, settings text, alerts, menus, schema text, docs copy, or web messages were introduced by this PR.