Skip to content

Fix browser back navigation history handoff #1897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 3 commits into from
Mar 21, 2026
Merged

Fix browser back navigation history handoff #1897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6617c4
Add regression test for browser back history
austinywang Mar 21, 2026
9063f52
Fix browser back history handoff
austinywang Mar 21, 2026
8846e45
Fix browser tab favicon not updating on navigation
austinywang Mar 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 153 additions & 20 deletions Sources/Panels/BrowserPanel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2525,6 +2525,7 @@ final class BrowserPanel: Panel, ObservableObject {
navigationDelegate.didFinish = { [weak self] webView in
Task { @MainActor [weak self] in
guard let self, self.isCurrentWebView(webView, instanceID: boundWebViewInstanceID) else { return }
self.realignRestoredSessionHistoryToLiveCurrentIfPossible()
boundHistoryStore.recordVisit(url: webView.url, title: webView.title)
self.refreshFavicon(from: webView)
self.applyBrowserThemeModeIfNeeded()
Expand Down Expand Up @@ -2867,20 +2868,109 @@ final class BrowserPanel: Panel, ObservableObject {
backHistoryURLStrings: [String],
forwardHistoryURLStrings: [String]
) {
realignRestoredSessionHistoryToLiveCurrentIfPossible()

let nativeBack = webView.backForwardList.backList.compactMap {
Self.serializableSessionHistoryURLString($0.url)
}
let nativeForward = webView.backForwardList.forwardList.compactMap {
Self.serializableSessionHistoryURLString($0.url)
}

if usesRestoredSessionHistory {
let back = restoredBackHistoryStack.compactMap { Self.serializableSessionHistoryURLString($0) }
// `restoredForwardHistoryStack` stores nearest-forward entries at the end.
let forward = restoredForwardHistoryStack.reversed().compactMap { Self.serializableSessionHistoryURLString($0) }
return (back, forward)
let restoredForward = restoredForwardHistoryStack.reversed().compactMap {
Self.serializableSessionHistoryURLString($0)
}

if isLiveSessionHistoryAlignedWithRestoredCurrent {
return (
back,
restoredForward.isEmpty ? nativeForward : restoredForward
)
}

return (back + nativeBack, nativeForward)
}

let back = webView.backForwardList.backList.compactMap {
Self.serializableSessionHistoryURLString($0.url)
return (nativeBack, nativeForward)
}

private func resolvedLiveSessionHistoryURL() -> URL? {
if let webViewURL = Self.remoteProxyDisplayURL(for: webView.url),
Self.serializableSessionHistoryURLString(webViewURL) != nil {
return webViewURL
}
let forward = webView.backForwardList.forwardList.compactMap {
Self.serializableSessionHistoryURLString($0.url)
if let currentURL,
Self.serializableSessionHistoryURLString(currentURL) != nil {
return currentURL
}
return nil
}

private var isLiveSessionHistoryAlignedWithRestoredCurrent: Bool {
let liveCurrent = Self.serializableSessionHistoryURLString(resolvedLiveSessionHistoryURL())
let restoredCurrent = Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL)
guard let liveCurrent, let restoredCurrent else { return true }
return liveCurrent == restoredCurrent
}

private func realignRestoredSessionHistoryToLiveCurrentIfPossible() {
guard usesRestoredSessionHistory else { return }
guard let liveCurrent = resolvedLiveSessionHistoryURL(),
let liveCurrentString = Self.serializableSessionHistoryURLString(liveCurrent) else {
return
}
guard Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL) != liveCurrentString else {
return
}

let restoredBack = restoredBackHistoryStack.compactMap { Self.serializableSessionHistoryURLString($0) }
let restoredForward = restoredForwardHistoryStack.reversed().compactMap {
Self.serializableSessionHistoryURLString($0)
}
let restoredCurrent = Self.serializableSessionHistoryURLString(restoredHistoryCurrentURL)

if let backIndex = restoredBack.lastIndex(of: liveCurrentString) {
let newBack = Array(restoredBack[..<backIndex])
var newForward = Array(restoredBack[(backIndex + 1)...])
if let restoredCurrent {
newForward.append(restoredCurrent)
}
newForward.append(contentsOf: restoredForward)

restoredBackHistoryStack = Self.sanitizedSessionHistoryURLs(newBack)
restoredForwardHistoryStack = Array(Self.sanitizedSessionHistoryURLs(newForward).reversed())
restoredHistoryCurrentURL = liveCurrent
refreshNavigationAvailability()
return
}

if let forwardIndex = restoredForward.firstIndex(of: liveCurrentString) {
var newBack = restoredBack
if let restoredCurrent {
newBack.append(restoredCurrent)
}
newBack.append(contentsOf: restoredForward[..<forwardIndex])
let newForward = Array(restoredForward[(forwardIndex + 1)...])

restoredBackHistoryStack = Self.sanitizedSessionHistoryURLs(newBack)
restoredForwardHistoryStack = Array(Self.sanitizedSessionHistoryURLs(newForward).reversed())
restoredHistoryCurrentURL = liveCurrent
refreshNavigationAvailability()
return
}
return (back, forward)

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()
Comment on lines +2965 to +2973

@greptile-apps greptile-apps Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Stale 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:

  1. Recompute restoredBack, restoredForward, and restoredCurrent arrays via compactMap.
  2. Fail both lastIndex/firstIndex searches.
  3. 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.

}

func restoreSessionNavigationHistory(
Expand Down Expand Up @@ -2927,10 +3017,16 @@ final class BrowserPanel: Panel, ObservableObject {
webViewObservers.append(titleObserver)

// Loading state
let loadingObserver = webView.observe(\.isLoading, options: [.new]) { [weak self] webView, _ in
// Capture the KVO-provided value at observation time rather than reading
// webView.isLoading inside the deferred Task. For fast navigations (e.g.
// back-forward cache), isLoading can flip true→false before the first Task
// runs, causing handleWebViewLoadingChanged(true) to be missed entirely.
// That skips favicon/loading-state cleanup and leaves stale icons visible.
let loadingObserver = webView.observe(\.isLoading, options: [.new]) { [weak self] webView, change in
let newValue = change.newValue ?? webView.isLoading
Task { @MainActor in
guard let self, self.isCurrentWebView(webView, instanceID: observedWebViewInstanceID) else { return }
self.handleWebViewLoadingChanged(webView.isLoading)
self.handleWebViewLoadingChanged(newValue)
}
}
webViewObservers.append(loadingObserver)
Expand Down Expand Up @@ -3209,6 +3305,27 @@ final class BrowserPanel: Panel, ObservableObject {
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }

// SPAs often inject <link rel="icon"> via JavaScript after the initial
// HTML loads. If no link tag was found, wait briefly and retry once to
// give client-side scripts time to add the tag.
if discoveredURL == nil {
try? await Task.sleep(nanoseconds: 600_000_000)
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }
if let href = await self.evaluateJavaScriptString(
js,
in: webView,
timeoutNanoseconds: 400_000_000
) {
let trimmed = href.trimmingCharacters(in: .whitespacesAndNewlines)
if !trimmed.isEmpty, let u = URL(string: trimmed) {
discoveredURL = u
}
}
guard self.isCurrentWebView(webView, instanceID: refreshWebViewInstanceID) else { return }
guard self.isCurrentFaviconRefresh(generation: refreshGeneration) else { return }
}

let fallbackURL = URL(string: "/favicon.ico", relativeTo: pageURL)
let iconURL = discoveredURL ?? fallbackURL
guard let iconURL else { return }
Expand Down Expand Up @@ -3879,20 +3996,29 @@ extension BrowserPanel {
func goBack() {
guard canGoBack else { return }
if usesRestoredSessionHistory {
guard let targetURL = restoredBackHistoryStack.popLast() else {
realignRestoredSessionHistoryToLiveCurrentIfPossible()

if (isLiveSessionHistoryAlignedWithRestoredCurrent || !nativeCanGoBack),
let targetURL = restoredBackHistoryStack.popLast() {
if let current = resolvedCurrentSessionHistoryURL() {
restoredForwardHistoryStack.append(current)
}
restoredHistoryCurrentURL = targetURL
refreshNavigationAvailability()
navigateWithoutInsecureHTTPPrompt(
to: targetURL,
recordTypedNavigation: false,
preserveRestoredSessionHistory: true
)
return
}
if let current = resolvedCurrentSessionHistoryURL() {
restoredForwardHistoryStack.append(current)

if nativeCanGoBack {
webView.goBack()
return
}
restoredHistoryCurrentURL = targetURL

refreshNavigationAvailability()
Comment on lines +4016 to 4021

@greptile-apps greptile-apps Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Potential extra back navigation after exhausting restored stack

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:

  1. isLiveSessionHistoryAlignedWithRestoredCurrent = true (live A == restored A).
  2. restoredBackHistoryStack.popLast() → nil (stack is empty) → the if block is skipped.
  3. 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.

navigateWithoutInsecureHTTPPrompt(
to: targetURL,
recordTypedNavigation: false,
preserveRestoredSessionHistory: true
)
return
}

Expand All @@ -3903,6 +4029,13 @@ extension BrowserPanel {
func goForward() {
guard canGoForward else { return }
if usesRestoredSessionHistory {
realignRestoredSessionHistoryToLiveCurrentIfPossible()

if nativeCanGoForward {
webView.goForward()
return
}

guard let targetURL = restoredForwardHistoryStack.popLast() else {
refreshNavigationAvailability()
return
Expand Down Expand Up @@ -5168,8 +5301,8 @@ extension BrowserPanel {
let resolvedCanGoBack: Bool
let resolvedCanGoForward: Bool
if usesRestoredSessionHistory {
resolvedCanGoBack = !restoredBackHistoryStack.isEmpty
resolvedCanGoForward = !restoredForwardHistoryStack.isEmpty
resolvedCanGoBack = nativeCanGoBack || !restoredBackHistoryStack.isEmpty
resolvedCanGoForward = nativeCanGoForward || !restoredForwardHistoryStack.isEmpty
} else {
resolvedCanGoBack = nativeCanGoBack
resolvedCanGoForward = nativeCanGoForward
Expand Down
84 changes: 84 additions & 0 deletions cmuxTests/BrowserConfigTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,6 +1515,49 @@ final class BrowserJavaScriptDialogDelegateTests: XCTestCase {

@MainActor
final class BrowserSessionHistoryRestoreTests: XCTestCase {
private func writeBrowserFixturePage(
at url: URL,
title: String,
file: StaticString = #filePath,
line: UInt = #line
) throws {
let html = """
<html>
<head><title>\(title)</title></head>
<body>\(title)</body>
</html>
"""

do {
try html.write(to: url, atomically: true, encoding: .utf8)
} catch {
XCTFail("Failed to write browser fixture page: \(error)", file: file, line: line)
throw error
}
}

private func waitForBrowserPanel(
_ panel: BrowserPanel,
url: URL,
timeout: TimeInterval = 5.0,
file: StaticString = #filePath,
line: UInt = #line
) {
let deadline = Date().addingTimeInterval(timeout)
while Date() < deadline {
RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01))
if panel.preferredURLStringForOmnibar() == url.absoluteString && !panel.isLoading {
return
}
}

XCTFail(
"Timed out waiting for browser panel to load \(url.absoluteString). Current=\(panel.preferredURLStringForOmnibar() ?? "nil") loading=\(panel.isLoading)",
file: file,
line: line
)
}

func testSessionNavigationHistorySnapshotUsesRestoredStacks() {
let panel = BrowserPanel(workspaceId: UUID())

Expand Down Expand Up @@ -1578,6 +1621,47 @@ final class BrowserSessionHistoryRestoreTests: XCTestCase {
XCTAssertTrue(panel.canGoForward)
}

func testGoBackPrefersLiveWKWebViewHistoryBeforeRestoredFallback() throws {
let tempDir = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-browser-history-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: tempDir, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: tempDir) }

let pageA = tempDir.appendingPathComponent("a.html")
let pageB = tempDir.appendingPathComponent("b.html")
let pageC = tempDir.appendingPathComponent("c.html")
try writeBrowserFixturePage(at: pageA, title: "A")
try writeBrowserFixturePage(at: pageB, title: "B")
try writeBrowserFixturePage(at: pageC, title: "C")

let panel = BrowserPanel(
workspaceId: UUID(),
initialURL: pageB
)
waitForBrowserPanel(panel, url: pageB)

panel.restoreSessionNavigationHistory(
backHistoryURLStrings: [pageA.absoluteString],
forwardHistoryURLStrings: [],
currentURLString: pageB.absoluteString
)

_ = browserLoadRequest(URLRequest(url: pageC), in: panel.webView)
waitForBrowserPanel(panel, url: pageC)

let snapshot = panel.sessionNavigationHistorySnapshot()
XCTAssertEqual(
snapshot.backHistoryURLStrings,
[pageA.absoluteString, pageB.absoluteString]
)

panel.goBack()
waitForBrowserPanel(panel, url: pageB)

panel.goBack()
waitForBrowserPanel(panel, url: pageA)
Comment on lines +1658 to +1662

@greptile-apps greptile-apps Bot Mar 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test does not exercise the "extra back" regression path

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")

}

func testWebViewReplacementAfterProcessTerminationUpdatesInstanceIdentity() {
let panel = BrowserPanel(
workspaceId: UUID(),
Expand Down
Loading