Multi-tab support (#287) - #298
Conversation
Extract Coordinator's navigation/script-handler machinery into a session-scoped WebPageSessionController owned by WebPageModel. The model now strongly owns its WKWebView (created lazily, torn down explicitly via an idempotent teardown()). WebPageView becomes a thin host whose dismantle still triggers teardown, preserving current behavior exactly. The controller holds the model weakly (and authorize captures it weakly) to avoid a retain cycle now that ownership is inverted. Co-authored-by: David T <davertay-j@users.noreply.github.com>
Pure, WebKit-free retention policy for live tab sessions: cap of 4 (named constant), most-recently-activated tab pinned against LRU eviction, and every eviction path invoking the session's teardown() exactly once. Includes targeted evict, evict-all-except-active (memory warnings), and evict-all (remove-all-data) operations. Not yet wired into AppModel; that lands with the multi-tab core (#291). Co-authored-by: David T <davertay-j@users.noreply.github.com>
A second awaitSelection() arriving while one is in flight previously overwrote the stored continuation, leaking it and hanging the original requester's promise forever. Unreachable with a single tab, but reachable once multiple tabs are live. The newcomer now fails fast with DeviceSelectionError.busy, leaving the in-flight selection untouched. Covers the existing TODO about leaked/double-resumed continuations with unit tests. Co-authored-by: David T <davertay-j@users.noreply.github.com>
…291) - TabSession retains a tab's full model graph (nav bar, loading model, web container with model-owned web view) across tab switches. - AppModel replaces activePageModel with activeSession + TabSessionCache: activating a cached tab reuses its live session (no reload, BLE intact); LRU-evicted tabs revert to reload-on-revisit. Fresh tabs stay uncached until their first page load begins. - Keep-alive underlay in AppContentView keeps background sessions' web views parented in the window (invisible, non-interactive) so WebKit keeps their content processes running and GATT notifications continue to be processed by background pages. Live sessions also survive the tab grid. - WebPageView hosts the shared web view inside a per-representable container so host teardown ordering during a switch can never rip the web view out of its new host; view unmount no longer tears sessions down. - Bluetooth permissions alert moves from the web page host to the active tab's chrome (WebContainerView) so background sessions can never raise UI. - window.open back-navigation now activates the opener tab's live session by index instead of strongly capturing the prior model graph. Co-authored-by: David T <davertay-j@users.noreply.github.com>
…#292) - Deleting a tab from the grid immediately tears down its live session (BLE disconnect, web view released) via a new TabGridModel.onTabDeleted callback, and clears the remembered last-opened tab if it pointed at the deleted one. Index reuse after deleting the highest tab makes the immediate teardown load-bearing: a recycled index must never inherit a stale session. - Settings 'Remove all data' now also evicts every live session so no page keeps in-memory state whose backing storage was wiped; the displayed tab rebuilds and reloads from scratch. - Memory warnings evict all background sessions (reload-on-revisit), keeping only the pinned session, so the app degrades gracefully instead of being jetsammed wholesale. - New TabsTests target covering the delete callback, index-reuse semantics, and find-or-create dedupe. Co-authored-by: David T <davertay-j@users.noreply.github.com>
) The live new-window path is WebNavigator.launchNewPage, rewired in the multi-tab core (#291) so that: - window.open/_blank pages become ordinary cached sessions (LRU-capped) while the opener tab stays live in the background underlay - the back chevron activates the opener tab's session by index: state intact while live, rebuild-from-URL if evicted, tab grid if closed - opening a URL that matches an existing tab activates that tab's live session (findOrCreateTab dedupe) instead of loading a duplicate - the opener linkage captures only a tab index, never the prior model graph, so no retain cycles or leaked sessions WebPageModel.launchNewPage was never referenced; delete it. Co-authored-by: David T <davertay-j@users.noreply.github.com>
Implement webViewWebContentProcessDidTerminate (previously unhandled: the web view went permanently blank) and propagate it through NavigationEngineDelegate -> WebPageSessionController -> WebPageModel -> AppModel, which resolves the native/JS split brain by converging to empty: the tab's session is evicted (BLE peripherals disconnected, engine shut down). The displayed tab then rebuilds its session and reloads its URL in place; background tabs quietly revert to reload-on-revisit. Subsequent Bluetooth API calls behave exactly as on a fresh page load since the rebuilt session gets a fresh engine. Co-authored-by: David T <davertay-j@users.noreply.github.com>
…#295) BluetoothEngine.handleDelegateEvent awaits each sendJsEvent, and each tab's EventBus drains serially - so a suspended web content process (e.g. app backgrounded) previously stalled that tab's entire event pipeline behind an unbounded queue. JsContext event sinks now enqueue into a per-context, order-preserving, bounded (256) JsEventDeliveryQueue that returns promptly and drains serially; individual delivery failures keep the existing log-and-continue semantics. Overflow means the page has been unresponsive under sustained traffic: the queue cancels itself and AppModel converges the tab to empty via the same discard-and-rebuild path as web content process termination (displayed tab reloads in place; background tabs revert to reload-on-revisit; other tabs unaffected since queues, engines, and event buses are per-tab). The queue is cancelled and replaced on cross-origin context swaps and on session teardown. Unit tests cover ordering, prompt enqueue while delivery is blocked, overflow-triggers-cancel+callback, post-cancel rejection, and drain resumption after a backlog clears. Co-authored-by: David T <davertay-j@users.noreply.github.com>
…#296) - New ActiveTabState (written by AppModel whenever the displayed tab changes) shares 'which tab is visible' with per-tab collaborators. - TabGatedDeviceSelector wraps the shared selector per Js context (tab taken from the context id at engine build time in the composition root): a background tab's requestDevice() rejects immediately with a page-visibility error instead of presenting UI over an unrelated tab or hanging; the active tab's flow is unchanged. - DeviceSelectionError now maps to spec-appropriate DOMExceptions (previously fell through to UnknownError): busy -> NotAllowedError, cancelled/invalid selection -> NotFoundError (Web Bluetooth: no chooser selection), pageNotVisible -> SecurityError (visibility / user-activation requirement). - Unit tests cover pass-through for the active tab, fast rejection for background/no-tab, and that a rejected background request never disturbs an in-flight active-tab selection. Co-authored-by: David T <davertay-j@users.noreply.github.com>
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>
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>
|
/quantumai-adversarial-review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated AI Review
Adversarial review of head 1c4a2de (28 files, ~1.6k LOC). Pass A ran 10 batch agents (5 high-risk on Sonnet, 5 normal on Composer); 4 Opus escalation agents followed on session/BLE/security paths. Adversary1 scored 18 candidates; Adversary2 confirmed 8 High issues. One Critical candidate (stuck shared device selector) was rejected by Adversary2 — the busy state is recoverable via picker dismissal. No Critical survivors.
High
-
F001 [Logic]
lib/Sources/App/AppModel.swift:187:193(score 91) —didReceiveMemoryWarning()callsevictAllExceptActive(), which sparesTabSessionCache.pinnedTabIndex. Fresh uncached tabs (e.g.openNewTab→buildSessionwithoutcache()) never callmarkActive, so the stale pin can protect a background tab instead of shedding it under memory pressure.- Suggestion: Evict based on the displayed tab — e.g.
if let active = activeSession, sessions.session(for: active.tabIndex) === active { sessions.evictAllExceptActive() } else { sessions.evictAll() }— rather than relying onpinnedTabIndexalone.
- Suggestion: Evict based on the displayed tab — e.g.
-
F002 [Logic]
lib/Sources/WebView/WebPageSessionController.swift:168:172(score 90) —JsEventDeliveryQueue.enqueuereturns immediately, breaking the prior guarantee thatsendJsEventcompletes beforeresolvePendingRequests.ReadCharacteristicreturns an empty response and relies on thecharacteristicvaluechangedevent mutatingthis.valuefirst; under queue backlogreadValue()can return stale ornull.- Suggestion: Return the read value in
CharacteristicResponse(like descriptor reads), or await delivery for request-coupled events before resolving the script reply.
- Suggestion: Return the read value in
-
F003 [Security]
lib/Sources/WebView/WebPageSessionController.swift:125:132(score 86) — Cross-origin teardown is scheduled asynchronously fromdidInitiateNavigation; the destination page can execute JS while the prior origin'sScriptHandlerand BLE processors remain attached untildetachOldHandlerAndWaitfinishes. (Pre-existing pattern fromCoordinator.swift, now more exposed with retained background sessions.)- Suggestion: Perform synchronous context swap in
decidePolicyFor:navigationAction, or block navigation until detach completes.
- Suggestion: Perform synchronous context swap in
-
F004 [Logic]
lib/Sources/WebView/JsEventDeliveryQueue.swift:118:137(score 85) —deliverRacingTimeoutuses a 30s wall-clock deadline that keeps running while the app/web content process is suspended. Backgrounding >30s with an in-flight delivery can false-trigger.timedOut→onOverflow→discardAndRebuildSession, dropping BLE connections on a healthy tab.- Suggestion: Probe liveness after foreground resume (or track scene phase) before calling
onOverflow; do not treat a single wall-clock sample as proof the page is wedged.
- Suggestion: Probe liveness after foreground resume (or track scene phase) before calling
-
F005 [Security]
lib/Sources/WebView/WebPageModel.swift:208:216(score 84) —requestAuthorization()has noisTornDownguard on the already-authorized fast path. In-flight script messages afterteardown()can still authorize or park a permissions continuation with no UI surface.- Suggestion: Add
guard !isTornDown else { return false }at the top ofrequestAuthorization().
- Suggestion: Add
-
F006 [Logic]
lib/Sources/App/AppModel.swift:257:257(score 83) —launchNewPagerecordsopenerTabIndexfromactiveSession?.tabIndexinstead of the calling tab'stabIndexfrombuildNavModel. Background-tabwindow.opencan wiregoBackToPriorPageto the wrong tab.- Suggestion: Use the closure's
tabIndexparameter:let openerTabIndex = tabIndex.
- Suggestion: Use the closure's
-
F007 [Logic]
lib/Sources/App/AppModel.swift:299:309(score 83) —configureSubmitAction's async first-load path assignswebContainerModelwithout theisCurrentSessionModelguard thatperformInitialLoaduses. Abandoning a fresh tab mid-load can orphan a full BLE-capable container and resurrect a deleted tab entry.- Suggestion: Guard with
isCurrentSessionModel(loadingModel, tabIndex:)before assignment, mirroringperformInitialLoad.
- Suggestion: Guard with
-
F008 [Security]
lib/Sources/DevicePicker/DeviceSelectionError.swift:17:19(score 81) —.cancellederrorDescriptionembedspresentedItemsdevice names, which flow throughtoDomError()into the JSDOMExceptionmessage — leaking nearby BLE device names on chooser dismissal.- Suggestion: Use a fixed cancellation message; log
presentedItemsnatively for diagnostics only.
- Suggestion: Use a fixed cancellation message; log
Coverage: 10 batches (5 high / 5 normal) · 28 files reviewed · Pass A: 10 agents · Escalation: 4 Opus agents · Adversary1: 18 scored · Adversary2: 10 validated · Pruned: 1 Critical + 9 High (incl. cross-origin event-delivery leak, latent selector-gate gaps, settings wipe race) · Lows withheld: 3 · Cap omitted: 0
Sent by Cursor Automation: QuantumAI adversarial review
davertay-j
changed the title
Eight findings from the automated review of #298: - Memory-warning eviction spared `pinnedTabIndex`, which lags the displayed tab while a fresh tab is uncached, so a background session could survive. `evictAll(except:)` now names the tab to spare. - The delivery queue broke the ordering `readValue()` depends on: the reply could overtake the `characteristicvaluechanged` event carrying the value. The queue now exposes a barrier and `ReadCharacteristic` holds its reply until its event has reached the page, per the spec's ordering. - The 30s delivery deadline kept expiring while the app was suspended, so backgrounding a healthy tab mid-delivery could tear down its session and drop its BLE connection. The timer is re-armed across suspensions. - `requestAuthorization()` could authorize, or park a permissions continuation with no alert left to resolve it, after teardown. - `window.open` from a background tab wired opener-back to the displayed tab rather than the calling one. - A tab deleted while its first page load was in flight came back from the dead in the grid, because the load populated it unguarded. - Dismissing the device chooser handed page script the names of nearby devices it was never granted; they are logged natively instead. The asynchronous cross-origin context swap is pre-existing and tracked separately in #310. Co-authored-by: Cursor <cursoragent@cursor.com>
Phoenix7351
left a comment
Phoenix7351
left a comment
There was a problem hiding this comment.
Looks good. Mostly just questions.
How much does this change the UI? Wondering how much work it will be to update Ether UI automation for web compose tests.
| ForEach(model.backgroundSessions) { session in | ||
| if let webContainerModel = session.loadingModel.webContainerModel { | ||
| WebPageView(model: webContainerModel.webPageModel) | ||
| .opacity(0.001) |
There was a problem hiding this comment.
We're keeping them alive by having them in the view hierarchy and just making them invisible? Seems wasteful. We can't keep webkit alive any other way?
Also, why not opacity 0 to make it fully invisible?
There was a problem hiding this comment.
Yes, the only way to keep webkit alive is to have them in the view hierarchy. The fractional opacity is a trick to ensure the view is not culled while keeping it invisible.
| /// Maximum number of tabs kept "hot" (live web view + Js context + BLE) at once. | ||
| /// Beyond this, the least-recently-activated background session is torn down and | ||
| /// its tab reverts to reload-on-revisit. | ||
| static let maxLiveSessions = 4 |
There was a problem hiding this comment.
Just curious why 4? Just a sweet spot of memory vs usability?
There was a problem hiding this comment.
Yep, just made a guess here. We expect that most folks are going to have only one tab that is using BLE and perhaps a couple of other auxiliary tabs. We are not trying to be the default browser so anticipate a low active tab count.
| @ObservationIgnored | ||
| private let maxLiveSessions: Int | ||
| private var sessions: [Int: Session] = [:] | ||
| private var lruOrder: [Int] = [] |
There was a problem hiding this comment.
I've seen LRU a couple times, but I think I've overlooked the definition. What is it? Least recently used?
There was a problem hiding this comment.
Yep, Least Recently Used.
| return (cache, sessions) | ||
| } | ||
|
|
||
| @Test func insertUnderCapRetainsAllSessions() { |
There was a problem hiding this comment.
I'm assuming AI wrote the tests for you? Can you have it follow our test name formatting with the "_"? I.e. unitBeingTested_Conditions_ExpectedResults
Really makes reading them easier.
There was a problem hiding this comment.
Good suggestion, will review these. Entire PR was written by AI btw.
4 participants
Multi-tab support: retained per-tab web sessions
Implements the plan in #287 (for #283): tabs become retained sessions — each open tab keeps its
WKWebView, JS context, andBluetoothEnginealive across tab switches, bounded by an LRU cap of 4 live sessions (active tab pinned). The primary goal is that a background tab holding a BLE connection keeps that connection and continues processing incoming GATT data while another tab is active.Tickets
All code tickets are complete with CI (lint, build, test) green on every commit:
How it fits together
TabSession(App) retains a tab's full model graph;WebPageModelnow owns itsWKWebViewand a session-scopedWebPageSessionController(the old view-lifecycleCoordinator, re-homed) with an explicit idempotentteardown().TabSessionCacheenforces the retention policy: max 4 live sessions, most-recently-activated tab pinned, every eviction callsteardown()exactly once (disconnecting BLE and shutting down that tab's engine — converge-to-empty).AppContentViewkeeps background sessions' web views parented in the window (invisible, non-interactive, behind the active page and the tab grid) so WebKit keeps their content processes running; chrome (nav bar, sheets, alerts — including the Bluetooth permissions alert, which moved from the page host toWebContainerView) mounts only for the active tab.JsEventDeliveryQueue(bounded, ordered, per context) decouples native→JS event delivery from the engine loop so a suspended page can't stall or bloat its tab's pipeline.TabGatedDeviceSelector+ActiveTabStaterestrict device selection to the visible tab;DeviceSelectionErrornow maps to spec-appropriate DOMExceptions (busy → NotAllowedError, cancelled → NotFoundError, page-not-visible → SecurityError).Manual verification checklist (#297 — requires real device + BLE peripheral)
requestDevice: triggerrequestDevice()from a backgrounded tab (e.g. delayed JS) → prompt rejection with a SecurityError, no picker UI anywhere.opacity(0.001)underlay) needs the documented offscreen-frame fallback.Notes for reviewers
TabsTests(grid model delete/index-reuse semantics),TabSessionCacheTests,JsEventDeliveryQueueTests,TabGatedDeviceSelectorTests, plus DeviceSelector busy-rejection tests.