Review fixes for #298: teardown resolves pending permissions + is terminal - #300
Merged
davertay-j merged 9 commits intoAug 10, 2026
Conversation
teardown() now denies (and thereby releases) a parked permissions continuation before releasing the web view. Without this, a background tab whose page requested Bluetooth authorization - the permissions alert chrome only mounts for the active tab - would, upon eviction, leak the CheckedContinuation, hold the WKScriptMessage reply open forever, and leave the page's promise permanently unsettled. Co-authored-by: David T <davertay-j@users.noreply.github.com>
teardown() now marks the model as torn down and webView() returns nil from then on instead of lazily creating a replacement. Previously a stray host update after eviction could silently spin up a fresh WKWebView and re-initialize the session controller on a model that the TabSessionCache no longer accounts for - a zombie session that would never be evicted or torn down again. View hosts treat a nil web view as a stale mount and leave their container empty for SwiftUI to remove. Co-authored-by: David T <davertay-j@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit aca6c8a. Configure here.
| func evictAll() { | ||
| for tabIndex in lruOrder { | ||
| evict(tabIndex) | ||
| } |
There was a problem hiding this comment.
Evict loops skip live sessions
High Severity
evictAll() and evictAllExceptActive() walk lruOrder with for-in while evict(_:) removes entries from that same array. Shrinking the array during iteration skips tab indexes, so some cached sessions are never torn down. “Remove all data” and memory-pressure paths can leave live web views, JS contexts, and BLE state running after they should be gone.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit aca6c8a. Configure here.
Collaborator
Author
There was a problem hiding this comment.
Dude, that isn't how Swift works.
RequestDevice attaches its advertisement listener and starts scanning before awaitSelection() reaches the visibility gate, so a background tab's requestDevice briefly forwards advertisements - filtered by that tab's options - to the shared selector. If the active tab has a picker open, those ads would be injected into its list. Gate showAdvertisement on tab visibility as well so they are dropped instead. Co-authored-by: David T <davertay-j@users.noreply.github.com>
performInitialLoad's task completed unconditionally: a session evicted (or replaced) while the web config loaded would still receive a fully built WebContainerModel - an orphaned container graph nobody accounts for. Mirror the staleness guard the submit path already has: only populate the loading model if it still belongs to the displayed session or a cached one. Co-authored-by: David T <davertay-j@users.noreply.github.com>
The pin persists while the grid is displayed so that returning to the last-viewed tab is cheap, but under actual memory pressure that meant one invisible WKWebView survived evictAllExceptActive(). When no tab is displayed, nothing is worth protecting: evict everything and let every tab revert to reload-on-revisit. Co-authored-by: David T <davertay-j@users.noreply.github.com>
goBackToPriorPage was installed on the opened session's nav bar by window.open handling and then persisted for the session's lifetime, so revisiting that tab later (e.g. from the grid) still showed an enabled back button wired to a long-departed opener context. Activation now resets the closure; the window.open flow re-installs it immediately after activating, so genuine opener-back is unaffected. Co-authored-by: David T <davertay-j@users.noreply.github.com>
callAsyncJavaScript's completion is not cancellable, so a page that never resumes it (wedged, or the continuation lost inside WebKit) would park the drain task forever while the buffer filled toward overflow. Deliveries now race a 30s timeout; a timeout means the page is beyond recovery and converges exactly like an overflow - cancel the queue and report so the owner tears the session down. The losing delivery is left to resolve (or leak inside WebKit) on its own. Co-authored-by: David T <davertay-j@users.noreply.github.com>
A web view re-parented into the keep-alive underlay could remain first responder, leaving its keyboard floating over the newly displayed tab or the grid. The outgoing session now relinquishes editing focus whenever the displayed session changes. The other half of the review note - the underlay's opacity(0.001) keep-alive technique being heuristic - stays deferred to the #297 on-device verification (checklist step 10 with the documented offscreen-frame fallback); changing the hosting strategy blind could break BLE keep-alive. Co-authored-by: David T <davertay-j@users.noreply.github.com>
cleanWebCache() was fire-and-forget, so 'Remove all data' rebuilt and reloaded the displayed tab while the removal was still in flight - the reloading page could read (and re-persist) cookies and storage that were about to be wiped. The wipe is now awaited (single async removeData over all types since the distant past, which also covers the per-record fan-out the old code did) before the session reset fires. The wipe hook is injectable on SettingsModel for the new SettingsTests ordering test. Co-authored-by: David T <davertay-j@users.noreply.github.com>
davertay-j
marked this pull request as ready for review
August 10, 2026 23:06
davertay-j
changed the base branch from
main
to
dt/cursor/topaz-multiple-tab-support-201b
August 10, 2026 23:06
davertay-j
merged commit Aug 10, 2026
1c4a2de
into
dt/cursor/topaz-multiple-tab-support-201b
4 checks passed
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
2 participants
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.
Review fixes for #298 (stacked)
Stacked on #298 (
dt/cursor/topaz-multiple-tab-support-201b) — the first 9 commits here are #298 itself; the top nine commits are new (2 critical fixes + 7 follow-up suggestions from the review). Draft PR opened primarily to run CI on the stacked commits (CI only triggers on PRs targetingmain). Intended to be merged into #298's branch (or rebased once #298 lands), not merged tomainindependently.Critical fixes
1. Resolve pending permissions request on session teardown (
519f629)WebPageModel.teardown()now denies (and thereby releases) a parked permissions continuation before releasing the web view, and resetspresentPermissionsDialog. Without this, a background tab whose page requested Bluetooth authorization — the permissions alert chrome only mounts for the active tab — would, upon eviction, leak theCheckedContinuation, hold theWKScriptMessagereply open forever, and leave the page's promise permanently unsettled.2. Make session teardown terminal:
webView()refuses to resurrect (aca6c8a)teardown()marks the model as torn down (isTornDown) andwebView()returnsnilfrom then on instead of lazily creating a replacement. Previously a stray host update after eviction could silently spin up a freshWKWebViewand re-initialize the session controller on a model thatTabSessionCacheno longer accounts for — a zombie session that would never be evicted or torn down again.Follow-up fixes (review suggestions)
baeec30—TabGatedDeviceSelectoralso gatesshowAdvertisement: a background tab's transient scan (before itsawaitSelectionis rejected) can no longer inject its advertisements into the active tab's open picker.6fc2bb0—performInitialLoadnow mirrors the submit path's staleness guard: a session evicted while the web config loaded no longer receives an orphanedWebContainerModel.3dee714— Memory warning while the tab grid is showing evicts all sessions (nothing is displayed, so the still-pinned last-viewed tab is background too).742a5b1— Re-activating a live session clears a stalegoBackToPriorPageclosure; only the window.open flow re-installs it.96948c7—JsEventDeliveryQueuebounds each delivery with a 30 s timeout: a page that never resumescallAsyncJavaScript's (non-cancellable) callback converges like an overflow instead of stranding the drain task forever.a46fea2— The outgoing session resigns keyboard focus when the displayed tab changes, so a web view moving to the keep-alive underlay can't leave a stale keyboard over the incoming view. (The underlay keep-alive technique itself stays as-is, pending Integrated on-device verification of multi-tab + BLE behavior #297 on-device verification.)5060fb1— "Remove all data" awaits the (now fully async) web data wipe before resetting sessions, so the reloaded page can't race the removal and re-persist wiped data. NewSettingsTeststarget with an ordering test.Tests
WebViewTests/WebPageModelTests.swift(5 tests): teardown denies pending permissions (would hang pre-fix), teardown idempotent/terminal,webView()stable pre-teardown andnilpost-teardown.TabGatedDeviceSelectorTests: backgroundshowAdvertisementnever reaches the picker.JsEventDeliveryQueueTests: wedged delivery times out and abandons the page; timely deliveries don't trip.SettingsTests/SettingsModelTests.swift: session reset fires only after the wipe completes.