Repository navigation
Fix stale browser pane content after drag splits - #1215
Merged
Merged
Conversation
…-1208-browser-pane-stale-content
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughTracks transient geometry recovery in portal sync, clears or preserves pane/portal drop contexts during hide/recovery/off-window paths, adds workspace-level sidebar/browser reset APIs and browser-panel reset logic, updates split handling to rearm portal host replacements, and adds a unit test for sidebar reset behavior. Changes
Sequence DiagramsequenceDiagram
participant Workspace
participant BrowserPanel
participant BrowserWindowPortal
participant Container as ContainerView
Workspace->>BrowserPanel: resetForWorkspaceContextChange(reason)
BrowserPanel->>BrowserPanel: hide devtools, clear omnibar/search, clear history
BrowserPanel->>BrowserPanel: replace WKWebView, rebind observers, reinit navigation
BrowserPanel->>BrowserWindowPortal: update portal bindings/state
BrowserWindowPortal->>Container: setPaneDropContext(nil) / setPaneDropContext(conditionally)
BrowserWindowPortal->>Container: setPortalDragDropZone(nil)
BrowserWindowPortal->>Container: setDropZoneOverlay(zone: nil)
BrowserWindowPortal->>BrowserWindowPortal: compare previousTransientRecoveryReason -> transientRecoveryReason
BrowserWindowPortal->>BrowserWindowPortal: if recoveredFromTransientGeometry append "transientRecovery" to refreshReasons -> force redraw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR fixes stale Key changes and concerns:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant BS as BonsplitDelegate
participant WS as Workspace
participant BP as BrowserPanel
participant BWP as BrowserWindowPortal
Note over BS,BWP: Drag-to-split layout event
BS->>WS: didSplit(originalPane, newPane)
WS->>BP: preparePortalHostReplacementForNextDistinctClaim(inPane:)
Note over BP: rearms portal host so new<br/>same-pane host can take ownership
Note over BS,BWP: Portal sync cycle (post-split geometry churn)
BWP->>BWP: syncContainerView()
Note over BWP: captures previousTransientRecoveryReason
BWP->>BWP: compute transientRecoveryReason (nil = geometry is OK)
BWP->>BWP: recoveredFromTransientGeometry = (prev≠nil && cur==nil)
alt recoveredFromTransientGeometry && !shouldHide
BWP->>BWP: refreshReasons.append("transientRecovery")
BWP->>BWP: force WKWebView refresh (clears stale tiles)
end
Note over BS,BWP: Workspace context reset (reset_sidebar / drag-context change)
WS->>WS: resetSidebarContext(reason:)
WS->>WS: clear statusEntries/logEntries/gitBranch/etc.
WS->>WS: resetBrowserPanelsForContextChange(reason:)
loop each BrowserPanel
WS->>BP: resetForWorkspaceContextChange(reason:)
BP->>BWP: BrowserWindowPortalRegistry.detach(oldWebView)
BP->>BP: create new WKWebView replacement
BP->>BP: bindWebView(replacement)
WS->>WS: bonsplitController.updateTab(title/favicon/loading)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4899
to
+4912
| let rearmBrowserPortalHostReplacement: (PaneID, String) -> Void = { paneId, reason in | ||
| for tab in controller.tabs(inPane: paneId) { | ||
| guard let panelId = self.panelIdFromSurfaceId(tab.id), | ||
| let browserPanel = self.browserPanel(for: panelId) else { | ||
| continue | ||
| } | ||
| browserPanel.preparePortalHostReplacementForNextDistinctClaim( | ||
| inPane: paneId, | ||
| reason: reason | ||
| ) | ||
| } | ||
| } | ||
| rearmBrowserPortalHostReplacement(originalPane, "workspace.didSplit.original") | ||
| rearmBrowserPortalHostReplacement(newPane, "workspace.didSplit.new") |
Contributor
There was a problem hiding this comment.
recoveredFromTransientGeometry fires even when shouldHide is still true
recoveredFromTransientGeometry evaluates to true whenever the entry transitions from having a transientRecoveryReason to having none, regardless of whether shouldHide is still true for a non-transient reason (e.g., !entry.visibleInUI). In that case, "transientRecovery" is appended to refreshReasons but the guard at the bottom (if !shouldHide, containerOwnsWebView, !refreshReasons.isEmpty) will prevent the actual refresh from firing, so the entry is harmless.
However, if a future code path reads refreshReasons before that guard (e.g., logging or metrics), it may see a misleading "transientRecovery" reason on a frame that was never actually shown. Consider narrowing the condition:
let recoveredFromTransientGeometry =
previousTransientRecoveryReason != nil &&
transientRecoveryReason == nil &&
!shouldHideThis would avoid appending the reason when the view is hidden for other reasons and keep semantics tight.
Comment on lines
+2791
to
+2807
| let oldWebView = webView | ||
| webViewObservers.removeAll() | ||
| webViewCancellables.removeAll() | ||
| BrowserWindowPortalRegistry.detach(webView: oldWebView) | ||
| oldWebView.stopLoading() | ||
| oldWebView.navigationDelegate = nil | ||
| oldWebView.uiDelegate = nil | ||
| if let oldCmuxWebView = oldWebView as? CmuxWebView { | ||
| oldCmuxWebView.onContextMenuDownloadStateChanged = nil | ||
| } | ||
|
|
||
| let replacement = Self.makeWebView() | ||
| webView = replacement | ||
| webViewInstanceID = UUID() | ||
| shouldRenderWebView = false | ||
| bindWebView(replacement) | ||
| refreshNavigationAvailability() |
Contributor
There was a problem hiding this comment.
deinit clears observers after async detach; resetForWorkspaceContextChange does it before
deinit schedules BrowserWindowPortalRegistry.detach asynchronously and then clears webViewObservers/webViewCancellables, meaning observers are still live during the brief window until the async task runs. resetForWorkspaceContextChange does the correct thing — clears observers first, then calls detach synchronously — but the inconsistency means the portal registry's sync path and the dealloc path have different guarantees around observer lifetimes.
This won't cause an immediate crash (the Task hop is a single run-loop cycle), but if a KVO notification fires on the old webView between the Task dispatch and its execution in deinit, it will hit an observer whose owning BrowserPanel is in teardown. Consider reordering deinit to match the reset path:
deinit {
developerToolsRestoreRetryWorkItem?.cancel()
developerToolsRestoreRetryWorkItem = nil
if let obs = detachedDeveloperToolsWindowCloseObserver {
NotificationCenter.default.removeObserver(obs)
}
webViewObservers.removeAll() // clear before any async work
webViewCancellables.removeAll()
let webView = webView
Task { @MainActor in
BrowserWindowPortalRegistry.detach(webView: webView)
}
}
Comment on lines
+2802
to
+2807
| let replacement = Self.makeWebView() | ||
| webView = replacement | ||
| webViewInstanceID = UUID() | ||
| shouldRenderWebView = false | ||
| bindWebView(replacement) | ||
| refreshNavigationAvailability() |
Contributor
There was a problem hiding this comment.
webViewInstanceID assigned after webView — binding window is open
Between webView = replacement (line 2803) and webViewInstanceID = UUID() (line 2804), the new webView is live but still carries the old webViewInstanceID. If bindWebView(replacement) (line 2806) or any synchronously-called initializer reads webViewInstanceID to identify the current web view (e.g., correlating completion callbacks to the right instance), it may use a stale ID for the interval between assignment and the UUID update.
Reorder to assign webViewInstanceID before assigning webView:
let replacement = Self.makeWebView()
webViewInstanceID = UUID()
webView = replacement
shouldRenderWebView = false
bindWebView(replacement)
refreshNavigationAvailability()
Comment on lines
+2491
to
2505
| XCTAssertTrue(workspace.panelGitBranches.isEmpty) | ||
| XCTAssertNil(workspace.pullRequest) | ||
| XCTAssertTrue(workspace.panelPullRequests.isEmpty) | ||
| XCTAssertTrue(workspace.surfaceListeningPorts.isEmpty) | ||
| XCTAssertTrue(workspace.listeningPorts.isEmpty) | ||
| XCTAssertFalse(browser.shouldRenderWebView) | ||
| XCTAssertNil(browser.preferredURLStringForOmnibar()) | ||
| XCTAssertFalse(browser.canGoBack) | ||
| XCTAssertFalse(browser.canGoForward) | ||
| XCTAssertNil(browser.searchState) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| @MainActor |
Contributor
There was a problem hiding this comment.
Test doesn't verify webViewInstanceID rotation or webView replacement
resetForWorkspaceContextChange explicitly rotates the WKWebView instance (key to the fix — the old stale-tile webView is discarded). The test verifies observable state after the reset but doesn't assert that:
browser.webViewInstanceIDchanged (a new UUID was issued)browser.webViewis a different object than before the reset
Without these checks, a regression that skips webView replacement (e.g., needsWorkspaceContextReset returns false prematurely) would still pass the existing assertions. Consider adding:
let priorWebView = browser.webView
let priorInstanceID = browser.webViewInstanceID
workspace.resetSidebarContext(reason: "test")
// …existing assertions…
XCTAssertNotEqual(browser.webViewInstanceID, priorInstanceID)
XCTAssertFalse(browser.webView === priorWebView)![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
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="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:2487">
P3: This assertion is currently vacuous for clear-on-reset behavior because `logEntries` is never populated before reset. Seed at least one log entry before `resetSidebarContext` so the test can catch regressions where logs are not cleared.</violation>
</file>
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:2786">
P2: Cancel/invalidate the favicon fetch task during workspace-context reset; otherwise an in-flight old task can repopulate stale favicon data after reset.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…-1208-browser-pane-stale-content
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2453-2495: The test never populates workspace.logEntries before
calling workspace.resetSidebarContext(reason:), so the assertion that logEntries
is empty only verifies the default state; add a log entry to the setup (e.g.,
append a SidebarLogEntry or whatever type is used into workspace.logEntries,
using the same test workspace instance) before the reset call so
resetSidebarContext(reason:) is exercised, then keep the existing
XCTAssertTrue(workspace.logEntries.isEmpty) after the reset to verify the
entries are cleared; locate and modify the test function around the setup lines
that set workspace.statusEntries, workspace.metadataBlocks, workspace.progress,
workspace.updatePanelGitBranch(...), workspace.updatePanelPullRequest(...), and
workspace.surfaceListeningPorts[contextPanelId].
In `@Sources/BrowserWindowPortal.swift`:
- Around line 2793-2794: The hide/transient branches currently clear
paneDropContext via containerView.setPaneDropContext(nil) and the overlay via
containerView.setDropZoneOverlay(zone: nil) but do not reset
containerView.portalDragDropZone, leaving activeDropZone still able to prefer
the stale portal zone; update each transient/hide branch (the locations around
containerView.setPaneDropContext and setDropZoneOverlay calls at the noted
spots) to also set containerView.portalDragDropZone = nil so the portal drag
zone is cleared whenever the pane context and overlay are cleared.
- Around line 3032-3034: The inspector-layout fast path is still preventing
refreshHostedWebViewPresentation(...) when hostedInspectorAdjustedDuringSync is
true, which lets transientRecovery changes get ignored; update the suppression
condition to allow a refresh when transientRecovery has just cleared (use
recoveredFromTransientGeometry or compare previousTransientRecoveryReason vs
transientRecoveryReason) so that if recoveredFromTransientGeometry is true you
call refreshHostedWebViewPresentation(...) regardless of
hostedInspectorAdjustedDuringSync; apply the same change wherever the
hostedInspectorAdjustedDuringSync check suppresses the refresh (including the
blocks around transientRecovery checks).
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2802-2807: After creating the replacement web view in the reset
path, reapply the current browserThemeMode to that replacement (the same logic
used in init / didFinish) before binding it: after let replacement =
Self.makeWebView() and before
bindWebView(replacement)/refreshNavigationAvailability(), apply the
browserThemeMode to the new webView (e.g., call the same helper or code path
that sets appearance on the web view using browserThemeMode) so the replacement
starts with the correct appearance instead of defaulting to system.
- Around line 2791-2806: Async callbacks (didFinish/didFailNavigation) and
refreshFavicon(from:) can run after webView has been replaced and repopulate the
panel with stale state; fix by having those async paths verify the emitting web
view is still the active one before mutating panel state. Concretely, in
didFinish(_:), didFailNavigation(_:with:), and refreshFavicon(from:) capture the
emitter (either the WKWebView instance or webViewInstanceID) at the start of the
Task and after any await check that it matches self.webView /
self.webViewInstanceID (return early if not); this ensures any Task started for
oldWebView will no-op after the replacement made by makeWebView()/bindWebView().
In `@Sources/Workspace.swift`:
- Around line 1722-1742: The loop currently mutates panelTitles directly causing
single-panel workspaces to skip the Workspace.title/processTitle sync; replace
the direct assignment panelTitles[browserPanel.id] = nextTitle with a call to
updatePanelTitle(panelId: browserPanel.id, title: nextTitle) (or the existing
updatePanelTitle signature) so the Workspace title sync runs; call
updatePanelTitle before computing resolvedTitle/titleUpdate, then compute
faviconUpdate/loadingUpdate as before and call
bonsplitController.updateTab(tabId, ...) — keep bonsplitController.updateTab
usage but derive title state from the post-update panel state (i.e., after
updatePanelTitle) rather than by writing panelTitles directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 160e2628-051e-4bb6-a8a4-7cc1059b4283
📒 Files selected for processing (5)
Sources/BrowserWindowPortal.swiftSources/Panels/BrowserPanel.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
Comment on lines
+2453
to
+2495
| workspace.statusEntries["task"] = SidebarStatusEntry(key: "task", value: "Issue #1208") | ||
| workspace.metadataBlocks["notes"] = SidebarMetadataBlock( | ||
| key: "notes", | ||
| markdown: "test", | ||
| priority: 0, | ||
| timestamp: Date() | ||
| ) | ||
| workspace.progress = SidebarProgressState(value: 0.5, label: "Loading") | ||
| workspace.updatePanelGitBranch(panelId: contextPanelId, branch: "issue-1208", isDirty: false) | ||
| workspace.updatePanelPullRequest( | ||
| panelId: contextPanelId, | ||
| number: 1208, | ||
| label: "PR", | ||
| url: try XCTUnwrap(URL(string: "https://example.com/pull/1208")), | ||
| status: .open | ||
| ) | ||
| workspace.surfaceListeningPorts[contextPanelId] = [3000] | ||
| workspace.recomputeListeningPorts() | ||
|
|
||
| XCTAssertTrue(browser.shouldRenderWebView) | ||
| XCTAssertNotNil(browser.preferredURLStringForOmnibar()) | ||
| XCTAssertTrue(browser.canGoBack) | ||
| XCTAssertTrue(browser.canGoForward) | ||
| XCTAssertNotNil(browser.searchState) | ||
| XCTAssertFalse(workspace.statusEntries.isEmpty) | ||
| XCTAssertFalse(workspace.metadataBlocks.isEmpty) | ||
| XCTAssertNotNil(workspace.progress) | ||
| XCTAssertNotNil(workspace.gitBranch) | ||
| XCTAssertNotNil(workspace.pullRequest) | ||
| XCTAssertEqual(workspace.listeningPorts, [3000]) | ||
|
|
||
| workspace.resetSidebarContext(reason: "test") | ||
|
|
||
| XCTAssertTrue(workspace.statusEntries.isEmpty) | ||
| XCTAssertTrue(workspace.logEntries.isEmpty) | ||
| XCTAssertTrue(workspace.metadataBlocks.isEmpty) | ||
| XCTAssertNil(workspace.progress) | ||
| XCTAssertNil(workspace.gitBranch) | ||
| XCTAssertTrue(workspace.panelGitBranches.isEmpty) | ||
| XCTAssertNil(workspace.pullRequest) | ||
| XCTAssertTrue(workspace.panelPullRequests.isEmpty) | ||
| XCTAssertTrue(workspace.surfaceListeningPorts.isEmpty) | ||
| XCTAssertTrue(workspace.listeningPorts.isEmpty) |
There was a problem hiding this comment.
Seed logEntries before asserting it clears.
Line 2487 currently only re-checks the default empty state because this test never adds anything to workspace.logEntries. That means the test still passes if resetSidebarContext(reason:) forgets to clear existing log entries. Add at least one log entry during setup so this path is actually exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2453 - 2495, The
test never populates workspace.logEntries before calling
workspace.resetSidebarContext(reason:), so the assertion that logEntries is
empty only verifies the default state; add a log entry to the setup (e.g.,
append a SidebarLogEntry or whatever type is used into workspace.logEntries,
using the same test workspace instance) before the reset call so
resetSidebarContext(reason:) is exercised, then keep the existing
XCTAssertTrue(workspace.logEntries.isEmpty) after the reset to verify the
entries are cleared; locate and modify the test function around the setup lines
that set workspace.statusEntries, workspace.metadataBlocks, workspace.progress,
workspace.updatePanelGitBranch(...), workspace.updatePanelPullRequest(...), and
workspace.surfaceListeningPorts[contextPanelId].
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/BrowserWindowPortal.swift (2)
3256-3260:⚠️ Potential issue | 🟠 MajorDon't suppress the forced refresh after transient recovery when the inspector layout also moved.
The new
transientRecoveryreason is still dropped wheneverhostedInspectorAdjustedDuringSyncis true, so inspector-attached panes can come out of recovery without the redraw this fix relies on.Suggested fix
- if !shouldHide, containerOwnsWebView, !refreshReasons.isEmpty { - if hostedInspectorAdjustedDuringSync { + if !shouldHide, containerOwnsWebView, !refreshReasons.isEmpty { + if hostedInspectorAdjustedDuringSync && !recoveredFromTransientGeometry { `#if` DEBUG dlog( "browser.portal.refresh.skip web=\(browserPortalDebugToken(webView)) " + "container=\(browserPortalDebugToken(containerView)) reason=\(source):" + "\(refreshReasons.joined(separator: ",")) adjustedDuringSync=1"Also applies to: 3267-3285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 3256 - 3260, The transientRecovery refresh reason is being skipped when hostedInspectorAdjustedDuringSync is true; ensure that when recoveredFromTransientGeometry is true you always append "transientRecovery" to refreshReasons regardless of hostedInspectorAdjustedDuringSync. Update the logic around the recoveredFromTransientGeometry check (and the similar block in the adjacent area handling inspector adjustments) so that refreshReasons.append("transientRecovery") is executed unconditionally when recoveredFromTransientGeometry is true, leaving the hostedInspectorAdjustedDuringSync handling separate.
2855-2860:⚠️ Potential issue | 🟡 MinorAlso clear
portalDragDropZonein these reset paths.
activeDropZoneprefersportalDragDropZone, so these branches can still leave the old split highlight painted afterpaneDropContextand the forwarded overlay are cleared. Please apply the same reset in the later preserved-frame transient branch as well.Suggested fix
func hideContainerView(reason: String) { containerView.setPaneTopChromeHeight(0) containerView.setSearchOverlay(nil) containerView.setPaneDropContext(nil) + containerView.setPortalDragDropZone(nil) containerView.setDropZoneOverlay(zone: nil) if !containerView.isHidden, webView.superview === containerView { webView.browserPortalNotifyHidden(reason: reason) } containerView.isHidden = true @@ containerView.setPaneDropContext(nil) + containerView.setPortalDragDropZone(nil) containerView.setDropZoneOverlay(zone: nil) return true @@ containerView.setPaneDropContext(nil) + containerView.setPortalDragDropZone(nil) containerView.setDropZoneOverlay(zone: nil) returnAlso applies to: 2874-2891, 3018-3035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 2855 - 2860, In hideContainerView and the other reset branches that clear paneDropContext / forwarded overlays (including the preserved-frame transient branch and the similar reset blocks around the preserved-frame logic), also clear the portalDragDropZone so activeDropZone won't continue to prefer the stale portal zone; update the branches that call containerView.setPaneDropContext(nil) and containerView.setDropZoneOverlay(zone: nil) to also reset portalDragDropZone (and any backing state that activeDropZone reads) so the split highlight is fully removed. Ensure you modify the hideContainerView function and the preserved-frame transient reset path to reset portalDragDropZone consistently with the other cleared state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 3256-3260: The transientRecovery refresh reason is being skipped
when hostedInspectorAdjustedDuringSync is true; ensure that when
recoveredFromTransientGeometry is true you always append "transientRecovery" to
refreshReasons regardless of hostedInspectorAdjustedDuringSync. Update the logic
around the recoveredFromTransientGeometry check (and the similar block in the
adjacent area handling inspector adjustments) so that
refreshReasons.append("transientRecovery") is executed unconditionally when
recoveredFromTransientGeometry is true, leaving the
hostedInspectorAdjustedDuringSync handling separate.
- Around line 2855-2860: In hideContainerView and the other reset branches that
clear paneDropContext / forwarded overlays (including the preserved-frame
transient branch and the similar reset blocks around the preserved-frame logic),
also clear the portalDragDropZone so activeDropZone won't continue to prefer the
stale portal zone; update the branches that call
containerView.setPaneDropContext(nil) and containerView.setDropZoneOverlay(zone:
nil) to also reset portalDragDropZone (and any backing state that activeDropZone
reads) so the split highlight is fully removed. Ensure you modify the
hideContainerView function and the preserved-frame transient reset path to reset
portalDragDropZone consistently with the other cleared state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce0d9a65-cce6-4f2d-becb-76ae58ef0d9c
📒 Files selected for processing (2)
Sources/BrowserWindowPortal.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/CmuxWebViewKeyEquivalentTests.swift
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:1717">
P2: Title resync can be skipped when `panelTitles` is unchanged, allowing Bonsplit tab titles to stay stale after browser panel context reset.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for browserPanel in browserPanels { | ||
| browserPanel.resetForWorkspaceContextChange(reason: reason) | ||
| let nextTitle = browserPanel.displayTitle | ||
| _ = updatePanelTitle(panelId: browserPanel.id, title: nextTitle) |
There was a problem hiding this comment.
P2: Title resync can be skipped when panelTitles is unchanged, allowing Bonsplit tab titles to stay stale after browser panel context reset.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 1717:
<comment>Title resync can be skipped when `panelTitles` is unchanged, allowing Bonsplit tab titles to stay stale after browser panel context reset.</comment>
<file context>
@@ -1713,29 +1713,23 @@ final class Workspace: Identifiable, ObservableObject {
for browserPanel in browserPanels {
browserPanel.resetForWorkspaceContextChange(reason: reason)
+ let nextTitle = browserPanel.displayTitle
+ _ = updatePanelTitle(panelId: browserPanel.id, title: nextTitle)
guard let tabId = surfaceIdFromPanelId(browserPanel.id),
</file context>
This was referenced Mar 12, 2026
This was referenced Mar 19, 2026
This was referenced Mar 25, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…er-pane-stale-content Fix stale browser pane content after drag splits
This was referenced Apr 6, 2026
This was referenced Apr 29, 2026
This was referenced May 9, 2026
This was referenced May 16, 2026
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
WKWebViewdoes not keep stale tiles in the old subframeTesting
./scripts/reload.sh --tag fix-browser-drag-redrawSummary by cubic
Fixes stale browser content after drag-to-split and pane reparenting. Keeps the right pane host attached, preserves drag/drop context through SwiftUI churn, and forces
WKWebViewredraws; addresses Linear #1208.WKWebViewdrops stale tiles.resetSidebarContextto clear sidebar state and reset all browser panels to a clean new-tab state; updates tab title/icon/loading;TerminalControllernow uses this.Written for commit 8aafb68. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
New Features
Tests