Repository navigation
Fix browser back navigation history handoff - #1897
Conversation
|
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 (1)
📝 WalkthroughWalkthroughSynchronizes restored session history with the live WKWebView state, adds URL resolution and alignment checks, updates session snapshot logic, and changes restored-mode goBack/goForward to prefer native WKWebView navigation when available. Includes a test validating live-history preference for back navigation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as "User / UI"
participant Panel as "BrowserPanel"
participant WebView as "WKWebView"
participant Restored as "RestoredHistoryStore"
User->>Panel: press Back
Panel->>Panel: realignRestoredSessionHistoryToLiveCurrentIfPossible()
Panel->>WebView: check nativeCanGoBack / backForwardList
alt nativeCanGoBack == true
Panel->>WebView: webView.goBack()
WebView-->>Panel: didFinish navigation (realign call)
else nativeCanGoBack == false
Panel->>Restored: pop restoredBack
Restored-->>Panel: targetURL
Panel->>Panel: set restoredHistoryCurrentURL, update stacks
Panel->>Panel: navigate(to: targetURL, preserveRestoredSessionHistory: true)
Panel->>WebView: load targetURL
WebView-->>Panel: didFinish navigation (realign call)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes browser back/forward navigation when a Key changes:
Notable concern: Once every restored back entry has been popped via Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BrowserPanel
participant RestoredStack as Restored History Stack
participant WKWebView
Note over BrowserPanel,RestoredStack: After session restore: back=[A], current=B
User->>BrowserPanel: navigates to C (live)
BrowserPanel->>WKWebView: load C
WKWebView-->>BrowserPanel: didFinish(C)
BrowserPanel->>RestoredStack: realignRestoredSessionHistoryToLiveCurrentIfPossible()
Note over RestoredStack: C not in restored stack → clear forward
User->>BrowserPanel: goBack()
BrowserPanel->>RestoredStack: realignRestoredSessionHistoryToLiveCurrentIfPossible()
Note over BrowserPanel: isAligned=false, nativeCanGoBack=true
BrowserPanel->>WKWebView: webView.goBack() → B
WKWebView-->>BrowserPanel: didFinish(B)
BrowserPanel->>RestoredStack: realign → live=B == restored=B, aligned ✓
User->>BrowserPanel: goBack()
BrowserPanel->>RestoredStack: realignRestoredSessionHistoryToLiveCurrentIfPossible()
Note over BrowserPanel: isAligned=true, nativeCanGoBack=false
BrowserPanel->>RestoredStack: popLast() → A
BrowserPanel->>WKWebView: navigateWithoutInsecureHTTPPrompt(A)
Note over WKWebView: native back now has [B] again ⚠️
WKWebView-->>BrowserPanel: didFinish(A)
Note over BrowserPanel: restored back=[], nativeCanGoBack=true → canGoBack=true ⚠️
User->>BrowserPanel: goBack() [unexpected extra press]
Note over BrowserPanel: restored pop=nil, fallthrough to webView.goBack()
BrowserPanel->>WKWebView: webView.goBack() → B (unexpected!)
Last reviewed commit: "Fix browser back his..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if nativeCanGoBack { | ||
| webView.goBack() | ||
| return | ||
| } | ||
| restoredHistoryCurrentURL = targetURL | ||
|
|
||
| refreshNavigationAvailability() |
There was a problem hiding this comment.
After navigateWithoutInsecureHTTPPrompt is used to pop a restored-history entry (e.g. navigating to page A from B), WKWebView records a new forward-history entry for B in its back list — so nativeCanGoBack remains true even after the restored stack is fully consumed.
At that point refreshNavigationAvailability() keeps canGoBack = true (because nativeCanGoBack || !restoredBackHistoryStack.isEmpty = true). The next goBack() call finds:
isLiveSessionHistoryAlignedWithRestoredCurrent=true(live A == restored A).restoredBackHistoryStack.popLast()→nil(stack is empty) → theifblock is skipped.- Falls through to
if nativeCanGoBack { webView.goBack() }→ jumps back to B.
The user ends up at B after explicitly having backed out to A — an unexpected reversal. In the old code this was hidden because canGoBack was computed as !restoredBackHistoryStack.isEmpty only, so it went false once the stack was drained.
A minimal guard would be to skip the native fallback here when the restored-session is still active and the stack is empty:
if nativeCanGoBack {
webView.goBack()
return
}
// restored stack is empty and no useful native back → let canGoBack settle to false
refreshNavigationAvailability()But this requires also re-evaluating whether nativeCanGoBack entries that were created by navigateWithoutInsecureHTTPPrompt should contribute to canGoBack at all while usesRestoredSessionHistory is true.
| guard !restoredForwardHistoryStack.isEmpty else { return } | ||
| #if DEBUG | ||
| dlog( | ||
| "browser.history.restore.forward.clear panel=\(id.uuidString.prefix(5)) " + | ||
| "current=\(liveCurrentString)" | ||
| ) | ||
| #endif | ||
| restoredForwardHistoryStack.removeAll(keepingCapacity: false) | ||
| refreshNavigationAvailability() |
There was a problem hiding this comment.
restoredHistoryCurrentURL causes repeated work on every call
When the live current URL is not found in either the restored back or forward stack, the function clears restoredForwardHistoryStack but does not update restoredHistoryCurrentURL. On every subsequent call the function will:
- Recompute
restoredBack,restoredForward, andrestoredCurrentarrays viacompactMap. - Fail both
lastIndex/firstIndexsearches. - Hit the
guard !restoredForwardHistoryStack.isEmpty(succeeds on first call, then fails on repeat calls — so no crash, but still allocates the arrays above every time).
Setting restoredHistoryCurrentURL = liveCurrent here (alongside the forward-clear) would let the early-out on line 2925 short-circuit all future calls until the URL changes again, and would make subsequent snapshots and isLiveSessionHistoryAlignedWithRestoredCurrent queries accurate immediately.
| panel.goBack() | ||
| waitForBrowserPanel(panel, url: pageB) | ||
|
|
||
| panel.goBack() | ||
| waitForBrowserPanel(panel, url: pageA) |
There was a problem hiding this comment.
The test verifies the two happy-path goBack() calls (C→B via native, B→A via restored), which both pass. However, as described in the comment on BrowserPanel.swift lines 3989–3994, calling goBack() a third time from A would unexpectedly navigate back to B because nativeCanGoBack is still true (WKWebView recorded B when navigateWithoutInsecureHTTPPrompt loaded A). Adding a third goBack() + assertion that canGoBack is false after reaching the start of history would catch that regression:
// After reaching A, no more history should be available
XCTAssertFalse(panel.canGoBack, "canGoBack should be false after reaching history start")![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmuxTests/BrowserConfigTests.swift (2)
1539-1559: Fail fast when the wait times out.
XCTFailrecords the timeout, but this helper then returns and lets the caller keep running through the remaining navigation/assertion steps. One missed wait will usually turn into several secondary failures, which makes the root cause harder to spot. Consider making this helperthrows(or returning aBooland asserting at the call site) so the test stops at the first timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserConfigTests.swift` around lines 1539 - 1559, The helper waitForBrowserPanel currently calls XCTFail on timeout but returns, allowing callers to continue; change its signature to throw (e.g., make waitForBrowserPanel(...) throws) and on timeout throw a descriptive error (or a custom WaitError) instead of just calling XCTFail, then update all callers to use try/try? so the test stops immediately when the wait fails; reference the waitForBrowserPanel function and panel.preferredURLStringForOmnibar()/panel.isLoading checks and replace the XCTFail branch with throwing the error so failures fail fast.
1624-1657: Exercise the stale-forward-history branch too.The restored forward stack is empty in this setup, so the test never proves that branching to live
pageCclears stale restored forward history. If that cleanup is part of the fix, seed one forward entry and assert it disappears after the live navigation.💡 Possible test strengthening
+ let pageD = tempDir.appendingPathComponent("d.html") + try writeBrowserFixturePage(at: pageD, title: "D") + panel.restoreSessionNavigationHistory( backHistoryURLStrings: [pageA.absoluteString], - forwardHistoryURLStrings: [], + forwardHistoryURLStrings: [pageD.absoluteString], currentURLString: pageB.absoluteString ) @@ XCTAssertEqual( snapshot.backHistoryURLStrings, [pageA.absoluteString, pageB.absoluteString] ) + XCTAssertTrue(snapshot.forwardHistoryURLStrings.isEmpty)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/BrowserConfigTests.swift` around lines 1624 - 1657, The test testGoBackPrefersLiveWKWebViewHistoryBeforeRestoredFallback never seeds a restored forward stack so it doesn't verify that navigating to a live page clears stale restored forward history; update the restoreSessionNavigationHistory call to include a non-empty forwardHistoryURLStrings (e.g. a fourth fixture page or pageC/pageD string) before calling browserLoadRequest, then after waitForBrowserPanel(panel, url: pageC) call sessionNavigationHistorySnapshot() and assert that snapshot.forwardHistoryURLStrings is empty and the backHistoryURLStrings still include the restored entries (e.g. [pageA.absoluteString, pageB.absoluteString]); this exercises the stale-forward-history branch in restoreSessionNavigationHistory and sessionNavigationHistorySnapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/BrowserConfigTests.swift`:
- Around line 1539-1559: The helper waitForBrowserPanel currently calls XCTFail
on timeout but returns, allowing callers to continue; change its signature to
throw (e.g., make waitForBrowserPanel(...) throws) and on timeout throw a
descriptive error (or a custom WaitError) instead of just calling XCTFail, then
update all callers to use try/try? so the test stops immediately when the wait
fails; reference the waitForBrowserPanel function and
panel.preferredURLStringForOmnibar()/panel.isLoading checks and replace the
XCTFail branch with throwing the error so failures fail fast.
- Around line 1624-1657: The test
testGoBackPrefersLiveWKWebViewHistoryBeforeRestoredFallback never seeds a
restored forward stack so it doesn't verify that navigating to a live page
clears stale restored forward history; update the
restoreSessionNavigationHistory call to include a non-empty
forwardHistoryURLStrings (e.g. a fourth fixture page or pageC/pageD string)
before calling browserLoadRequest, then after waitForBrowserPanel(panel, url:
pageC) call sessionNavigationHistorySnapshot() and assert that
snapshot.forwardHistoryURLStrings is empty and the backHistoryURLStrings still
include the restored entries (e.g. [pageA.absoluteString,
pageB.absoluteString]); this exercises the stale-forward-history branch in
restoreSessionNavigationHistory and sessionNavigationHistorySnapshot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d91e69d5-62f2-49c2-b745-9dbd539a8ca9
📒 Files selected for processing (2)
Sources/Panels/BrowserPanel.swiftcmuxTests/BrowserConfigTests.swift
Two issues caused stale or missing favicons in browser tabs: 1. KVO race: The isLoading observer read webView.isLoading inside a deferred Task instead of capturing the KVO change value at observation time. For fast navigations (back-forward cache), isLoading flips true→false before the Task runs, so handleWebViewLoadingChanged(true) was never called and the old favicon was never cleared. 2. SPA favicon discovery: Sites that inject <link rel="icon"> via JavaScript (e.g. React apps) had no favicon link in the DOM when didFinish fired. The fallback to /favicon.ico often 404'd, leaving the globe icon permanently. Now retries the JS query after 600ms to give client-side scripts time to add the tag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Add regression test for browser back history * Fix browser back history handoff * Fix browser tab favicon not updating on navigation Two issues caused stale or missing favicons in browser tabs: 1. KVO race: The isLoading observer read webView.isLoading inside a deferred Task instead of capturing the KVO change value at observation time. For fast navigations (back-forward cache), isLoading flips true→false before the Task runs, so handleWebViewLoadingChanged(true) was never called and the old favicon was never cleared. 2. SPA favicon discovery: Sites that inject <link rel="icon"> via JavaScript (e.g. React apps) had no favicon link in the DOM when didFinish fired. The fallback to /favicon.ico often 404'd, leaving the globe icon permanently. Now retries the JS query after 600ms to give client-side scripts time to add the tag. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Fixes #1892
Verification
./scripts/setup.sh./scripts/reload.sh --tag issue-1892-back-history(xcodebuildreached** BUILD SUCCEEDED **in/tmp/cmux-xcodebuild-issue-1892-back-history.log; the later Ghostty CLI helper step then failed because the machine ran out of disk space)xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1892-back-history build(failed to rerun once the volume hitother(28)/ out-of-space during package resolution)Summary by cubic
Fixes back/forward history handoff and stabilizes tab favicon updates after navigation. The app now prefers live web history and updates icons reliably on fast navigations and SPAs.
Fixes #1892
Written for commit 8846e45. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests