refactor(bg): tab-registry + challenge-flag redesign (closes Q-016)#6
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ge keys) Centralize all per-tab session state behind one owned module instead of scattered expurge_tab_* / expurge_challenge_* literals and ad-hoc scans in index.ts. - tab-registry-resolve.ts: pure resolvers (tabForItem, brokerTabInWindow) + key helpers over a plain snapshot, unit-tested like coordinator.ts. - tab-registry.ts: imperative session-storage I/O wrappers. removeTab drops BOTH the tab key and the challenge key, and removeAllTabs sweeps both families on Stop -> no more orphan challenge keys for a recycled tab id to read as stale challenge=true. - index.ts: every raw key access / itemIdForTab / tabIdForItem / findWindowBrokerTab / setChallengeFlag / isChallenged routes through the module; four dead local defs removed. onRemoved reads the item before dropping keys and clears both via removeTab. No happy-path behavior change. Part of the challenge-flag redesign (Q-016). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n place) The observer used to arm only when the page loaded challenged and disconnect after one clear, so a challenge that appears after a clean load (a mid-run rate-limit re-gate that swaps the page in place, no navigation) was never reported -> verdict buttons could render over a live gate. Make it always-armed and change-driven: a single persistent MutationObserver that, on a debounced mutation, reports the current detectChallenge() state, deduped against the last thing reported. It never disconnects and reports transitions in BOTH directions (appear -> DETECTED, clear -> RESOLVED). The __expurgeReporterBound latch and the 250 ms mid-transition debounce are kept. Part of the challenge-flag redesign (Q-016). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oad driver The content script re-injects and reports challenge state on every broker full-page load (all v1 brokers are full-reload), and that CHALLENGE_* handler already calls pushActiveView. onUpdated's own pushActiveView on 'complete' was therefore a second, redundant push that raced the content report and could flash the verdict view over a challenge page (review #5). Remove the listener; leave an SPA caveat comment (a future same-document broker would need it back). Revert this commit to reinstate. Kept as its own commit so that revert stays trivial. Also refresh the now-stale NAVIGATE_BROKER_TAB comment that referenced the removed onUpdated push. Part of the challenge-flag redesign (Q-016). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… debounce quiesce The rewritten observer used a trailing 250ms debounce reset on every mutation, so a page mutating faster than 250ms sustained (ad/tracker churn, an animating CAPTCHA widget) never fired the callback -> an in-place challenge was never reported and verdict buttons could sit over a live gate (the case Part B exists to fix). Split the two directions: a challenge APPEARING is reported immediately on the leading edge (never debounced, never starved); a challenge CLEARING is still confirmed after a 250ms settle to ride out a widget briefly detaching, but the timer is armed once per clear and NOT reset on later mutations, so a busy page can't starve the RESOLVED report either. Load-time report stays synchronous. Review finding #1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
removeTab drops both the tab key and the challenge key, but putTab set only the tab key -- so a new broker tab opened on a recycled tab id that still carried an orphan expurge_challenge_<id> (e.g. an observer report landing after Stop) would inherit challenge=true and show the challenge view over a clean, actionable page until its content script's first report. Clear the challenge key on tab-open so the 'drop both keys' invariant holds from the open side too. Review finding #2 (also neutralizes the post-stop orphan-recreation path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
captureListingUrl gated only on isResultsPage (pathname), so a broker tab that had wandered off-host (external link, ad, unfinished redirect) could have its off-host URL recorded as a hit's listingUrl and flow into the opt-out draft. Reachability widened when Part C removed the onUpdated push that used to show the 'offsite' view and hide the verdict buttons. Add an isOnHost guard so an off-host tab captures nothing -- defense in depth for the wrong-listing case. Review finding #3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed writes Two review follow-ups in one handler: - (#7) The tracked-tab cleanup was routed through handleSkip -> removeTab, which handleSkip skips on its `!run` early-return, so a tracked tab closed while no run is loadable could orphan both keys. Drop both keys in onRemoved itself, before handleSkip (whose own removeTab then no-ops), so cleanup can't be skipped. - (#11) The untracked branch used to write two nonexistent keys on every tab close anywhere in the browser. Any orphan challenge key on an untracked tab is inert (isChallenged is read only for tracked tabs) and putTab scrubs it on id reuse, so just return early instead of touching storage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oss-window test - (#9) findBrokerTab resolved every tracked tab with sequential browser.tabs.get awaits; resolve them with Promise.all instead (stale-key pruning stays after the join). ~N serialized IPC round-trips per sidebar pull -> one parallel wave. - (#12/#15) Extract a pure isPerTabKey(key) predicate into tab-registry-resolve (unit-tested, incl. that it never matches expurge_run/expurge_profile) and use it in removeAllTabs instead of a hand-rolled inline prefix filter -> the Stop sweep's key classification now has one tested definition. - (#13) Add a brokerTabInWindow test pairing an active tab with a DIFFERENT window, so window-scoping can't silently regress out of the active-preference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Log the decisions made and issues found during this branch's work and QA: - 2026-07-03-tab-registry-challenge-state: the refactor (owned tab-registry, two atomic key families, pure/IO split, always-armed observer, dropped onUpdated, on-host listingUrl guard). Answers Q-016 (resolve on merge). - 2026-07-03-turnstile-detection-gap: QA found detectChallenge misses TPS's explicitly-rendered Cloudflare Turnstile; fix approach captured for a follow-up branch. Opens Q-018. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
What & why
Refactors per-tab run state — which was scattered as raw
expurge_tab_*/expurge_challenge_*session-storage literals and ad-hoc scans across
background/index.ts— into one owned tabregistry, and makes the content script the authoritative owner of the challenge signal. No
happy-path behavior change; a class of challenge-flag lifecycle bugs (orphan keys, in-place
reappearance, "who clears the flag") disappears. Closes Q-016.
The three parts (one commit each)
refactor(bg): own per-tab state in a tab-registry. New puretab-registry-resolve.ts(
tabForItem,brokerTabInWindow, key helpers) + I/O wrappertab-registry.ts. Split into twofiles (not the spec's one) because webextension-polyfill throws at import in node tests, so the
pure resolvers must stay polyfill-free to be unit-tested — mirrors the
coordinator.ts↔index.tspattern.removeTabnow drops both keys andremoveAllTabssweeps both familieson Stop → the orphan-challenge-key bug is gone.
feat(content): always-armed challenge observer. A persistent, never-disconnectingMutationObserverreports challenge transitions in both directions (appear → DETECTED, clear →RESOLVED), closing the in-place-reappearance gap.
fix(bg): drop redundant onUpdated push. The content report is the authoritativepost-load push;
onUpdated's push raced it and flashed the verdict view over a challenge page.Own revertable commit; SPA caveat documented.
Self-review follow-ups (also in this branch)
An extra-high-effort review of the branch surfaced 15 findings; the correctness/robustness ones are
fixed here:
debounce reset); clearing keeps the 250 ms settle but the timer isn't reset.
putTabscrubs any stale challenge key on tab (re)open — a recycled id can't inheritchallenge=true.captureListingUrlrequiresisOnHost— an off-host tab can't record a wrong listing into a draft.onRemovedclears a tracked tab's keys eagerly (can't be skipped byhandleSkip's!runreturn)and skips storage writes for untracked tabs.
findBrokerTabresolves tracked tabs in parallel;isPerTabKeypredicate extracted + tested;cross-window resolver test added.
Consciously left (rationale in commits):
lastReported-before-send (MV3 wakes the event page),late-injected-widget false DETECTED (challenge-specific selectors), numeric-vs-insertion key order
(monotonic ids), observer-never-disconnects (by design), redundant resolver param (cosmetic).
Testing
npm run typecheck && npm test && npm run buildgreen; 187 unit tests, 100% coverage on thepure modules,
web-ext lint0 errors (13 pre-existing manifest_noteswarnings, unrelated).Still owed (not blocking merge)
expurge_challenge_*after Stop; no verdict flash on a challenge load; a DOM-injected challenge flips to the challenge
view. Plus the PR chore(sidebar): review cleanup — wordmark, isMissingSkip, progress source, checklist race, external-mutation pushes #5 re-verdict checks in
temp/next-steps.md §2.plan/expurge-progress.md.🤖 Generated with Claude Code
Manual QA — Firefox 140+, 2026-07-03
expurge_challenge_*keys remain after Stoptab_closed)listingUrlguardlistingUrlTest 4 — pre-existing challenge-detection gap (does not block this PR)
On TruePeopleSearch's real gate (
/InternalCaptcha, a "Verify you are human" widget) the sidebar showsverdict buttons over the live challenge. Root cause is in
detectChallenge()(src/content/classify.ts),which this PR does not touch — so it predates this refactor and isn't caused by it. This PR reworks
challenge state management (all tests green); it doesn't change what counts as a challenge.
Diagnosed via live DOM probe: TPS renders Cloudflare Turnstile explicitly (widget iframes are
about:blank, no.cf-turnstilecontainer), so every current selector misses it. The reliable signal isthe Turnstile API script
challenges.cloudflare.com/turnstile/v0/api.js. Fix is a one-selector add todetectChallenge(), safe because the interstitial navigates away on solve (fresh load →CHALLENGE_RESOLVED).Tracked as a follow-up (
fix/challenge-detection-managed, natural pairing with the M9 broker expansion);full write-up in
temp/challenge-detection-fix.md(local handoff doc).