Repository navigation
fix(theme): guard against post-load dark.css in-flight race and navigation unload - #381
TimeToBuildBob wants to merge 3 commits into
Conversation
Two non-deterministic failures in DashboardThemePaintTest: 1. Navigation unload race: after `location.href = '/'`, polling `__awAndroidThemeState()` on the unloading `/early` document returns undefined (JS context being destroyed), causing a 15-second timeout. Fix: await `location.href == url` before polling theme state. 2. Pre-hook-install in-flight race: the `__awPresentAtHookInstall + readyState === 'complete'` shortcut assumed any link present at hook install time had already dispatched load/error. But when `/settings` resolves before `evaluateJavascript` queues in the renderer, dark.css is appended to the DOM with the flag set while it is still in-flight. Fix: use `performance.getEntriesByName(href, 'resource').responseEnd` to distinguish a completed response (return true) from an in-flight one (return false, await the listener already registered above). Closes ActivityWatch#332 Git-Session-Id: b2ca8cf4-a63c-44d5-86d5-eb8ee21a703d
[Medium impact] The runtime effect stays within the Android dashboard's theme and reveal flow. The new error test must be made to pass before merging. Findings
SummaryThe PR guards stylesheet settlement against a post-load request race and waits for navigation before polling theme state.
Reviews (2) · Last reviewed commit: "test(theme): force pre-install styleshee..." · Reviewed by Greptile |
|
All six checks passed at 7e7bcee, including Test E2E (29) and the R8 smoke test. Greptile’s remaining P2 is valid: the string assertions do not deterministically exercise the pre-install/in-flight ordering. I have left that thread open and handed the test follow-up to the branch owner; its active worktree/branch leases prevent a concurrent edit in this monitoring pass. A green run alone does not establish that #332 is resolved. |
🤖 AI code reviewWebUiTheme.kt moves stylesheet event listener registration before the pre-install check and uses PerformanceResourceTiming entries to decide whether an existing stylesheet has completed. DashboardThemePaintTest adds concurrent request handlers and navigation synchronization, WebUiThemeHookTest adds two WebView scenarios, and WebUiThemeTest adds assertions for the timing API identifiers. Needs a look — P2 onlyConfidence 4/5 1 finding ·
|
| commit | score | findings | engine | when |
|---|---|---|---|---|
7e7bcee79f70 |
4/5 | 1 | subscription | 2026-10-26 12:23 UTC |
Reviewed b3033f4f1fbc · panel: codex:gpt-6.1-sol + claude-code:sonnet + grok-build:grok-4.6 · subscription engine · 60s · about this reviewer
Maintainer commands
@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.
The optional OpenRouter verifier was unavailable. Findings retain their original severity.
| return true; | ||
| } | ||
| // Request still in-flight; the listener above will fire on completion. | ||
| return false; |
There was a problem hiding this comment.
style-src 'none' can reject dark.css and dispatch its error before hook installation without creating a resource timing entry. The link then has no sheet, getEntriesByName returns [], and this branch returns false. Because the newly attached error listener missed the event, checkSettled never reports settlement and the readable fallback remains gated despite the stylesheet having definitively failed.
There was a problem hiding this comment.
Follow-up at c0d5b95: added and ran preInstallFailureSettlesAfterTimingEviction in a real WebView (API 36, WebView 133.0.6943.137). It waits for the link error before injecting the hook, clears Resource Timing, verifies no matching entries and zero prior reports, then observes settlement. This passes on unchanged production code. A CSP trial on the same WebView also settled; it produced a timing entry and did not satisfy the claimed null-sheet premise. The new test does not prove behavior on every supported WebView, so I am leaving this finding open rather than claiming a production repair. The local reviewer repeated that this test would time out, but the actual instrumentation run passed (2/2); no speculative early-reveal fallback was added. Next verification is the API 29 CI run and, if needed, an executable null-sheet/no-entry reproduction on an affected WebView.
Git-Session-Id: ca3909ca-7ffe-556a-bdc5-a46667ffaab1
|
@greptileai review |
…stalled API 29 E2E (run 40334359221) timed out at the location.href wait after navigating from /early to /. The test HTTP server handled one connection at a time, so a leftover /dark.css held on the latch blocked GET /. Wait on location.pathname instead of an exact href match, and accept stylesheetRequests >= 3 so an extra leftover CSS request cannot skip the exact count. Git-Session-Id: eeec9750-b606-5166-a191-9aa3ffd35156
|
API 29 E2E failed on c0d5b95 at The paint-test HTTP server handled one connection at a time, so a leftover Greptile withdrew the |
|
Convergence adjudication deferred (freshness gate). Current head
No classification, dismissal, or merge recommendation on this unreviewed head. No Greptile re-trigger (convergence cap). Current-head snapshot (not merge evidence):
Awaiting automatic current-head review ( |
|
The freshness deferral is now cleared: the updated Bob review explicitly covers Resolved the two Greptile threads with existing evidence: the deterministic pre-install/in-flight and failed-stylesheet tests address “Checks miss the race”; “New theme test times out” was explicitly withdrawn by Greptile and is contradicted by the passing instrumentation runs. No extra inline replies or review re-trigger. The separate Bob P2 |
Fixes two independent non-deterministic failures in
DashboardThemePaintTest.lightDocumentStaysHiddenUntilDarkStylesheetApplies(closes #332).Root causes
1. Navigation unload race (test-side fix)
After
evaluate(webView, "location.href = '/'"), the test immediately pollswindow.__awAndroidThemeState(). During the/early→/transition, the old page's JS context is still being destroyed.evaluateJavascripton the unloading document returnsundefined(the function does not exist), causing the 15-secondawaitto time out.Fix: synchronize on
location.href == urlbefore polling theme state, then wait forstylesheetRequests == 3anddocument.readyState == 'complete':await { evaluate(webView, "location.href") == "\"$url\"" } await { stylesheetRequests.get() == 3 } await { evaluate(webView, "document.readyState") == "\"complete\"" } await { evaluate(webView, "window.__awAndroidThemeState()") == "\"pending\"" }2. Post-load fetch race (production code fix)
The
__awPresentAtHookInstall + readyState === 'complete'shortcut indarkStylesheetSettledassumed any dark.css link present at hook-install time had already completed itsload/errorevent cycle (because the hook fires fromonPageFinished, after the windowloadevent).However, aw-webui's App.vue appends dark.css inside a
fetch('/settings')callback (post-load). If the fetch resolves before theevaluateJavascriptcallback runs in the renderer, dark.css is already in the DOM with__awPresentAtHookInstall = truebut still in-flight. The old shortcut then returnedtrue(settled) immediately, triggeringreportThemeSettledbefore dark.css had applied.Fix: use
performance.getEntriesByName(link.href, 'resource')to check whether the response was received before the hook ran:responseEnd > 0→ completed before hook; null sheet = failed → reveal nowresponseEnd === 0/ no entry → still in-flight; listener registered above will fire on completionThe event listener is now registered before the
__awPresentAtHookInstallcheck, ensuring it is in place for the in-flight case.Files changed
WebUiTheme.ktdarkStylesheetSettled: move listener registration before pre-install check; use PerformanceResourceTiming to distinguish completed vs in-flightDashboardThemePaintTest.ktlocation.href+stylesheetRequests+readyStatesync guards before final__awAndroidThemeStatepollWebUiThemeTest.ktgetEntriesByNameandresponseEndare present in hook JSRelation to prior work
document.documentElement || document); it did not address this race, despite the task (CI: emulator jobs (Test E2E, R8 smoke) fail non-deterministically — red run gives no code signal #332) initially being misattributed to it.window.__awAndroidThemeState is not a function) is the navigation unload race (fix 1 above).Co-Authored-By: Bob bob@superuserlabs.org