Skip to content

fix(theme): guard against post-load dark.css in-flight race and navigation unload - #381

Open
TimeToBuildBob wants to merge 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/theme-hook-presethook-race
Open

TimeToBuildBob wants to merge 3 commits into
ActivityWatch:masterfrom
TimeToBuildBob:fix/theme-hook-presethook-race

Conversation

@TimeToBuildBob

Copy link
Copy Markdown
Contributor

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 polls window.__awAndroidThemeState(). During the /early→/ transition, the old page's JS context is still being destroyed. evaluateJavascript on the unloading document returns undefined (the function does not exist), causing the 15-second await to time out.

Fix: synchronize on location.href == url before polling theme state, then wait for stylesheetRequests == 3 and document.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 in darkStylesheetSettled assumed any dark.css link present at hook-install time had already completed its load/error event cycle (because the hook fires from onPageFinished, after the window load event).

However, aw-webui's App.vue appends dark.css inside a fetch('/settings') callback (post-load). If the fetch resolves before the evaluateJavascript callback runs in the renderer, dark.css is already in the DOM with __awPresentAtHookInstall = true but still in-flight. The old shortcut then returned true (settled) immediately, triggering reportThemeSettled before 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 now
  • responseEnd === 0 / no entry → still in-flight; listener registered above will fire on completion

The event listener is now registered before the __awPresentAtHookInstall check, ensuring it is in place for the in-flight case.

Files changed

File Change
WebUiTheme.kt darkStylesheetSettled: move listener registration before pre-install check; use PerformanceResourceTiming to distinguish completed vs in-flight
DashboardThemePaintTest.kt Add location.href + stylesheetRequests + readyState sync guards before final __awAndroidThemeState poll
WebUiThemeTest.kt Assert getEntriesByName and responseEnd are present in hook JS

Relation to prior work

Co-Authored-By: Bob bob@superuserlabs.org

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
@greptile-apps

greptile-apps Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[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

  1. P1 New theme test times out ▶

Summary

The PR guards stylesheet settlement against a post-load request race and waits for navigation before polling theme state.

  • The dashboard waits for dark.css before it appears.
  • The paint test waits for the new page before checking its theme.

Reviews (2) · Last reviewed commit: "test(theme): force pre-install styleshee..." · Reviewed by Greptile

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

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.

@TimeToBuildBob

TimeToBuildBob commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI code review

WebUiTheme.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 only

Confidence 4/5

1 finding · ⚠️ 1 P2

⚠️ P2 medium — mobile/src/main/java/net/activitywatch/android/WebUiTheme.kt:103

An empty Resource Timing result does not establish that the stylesheet is still loading. If dark.css has already failed and its timing entry was cleared or evicted before hook installation, this branch returns false after registering listeners for events that have already fired. checkSettled() consequently never reports completion, and __awAndroidThemeState() continues returning 'pending', leaving the dashboard hidden. The new preInstallFailureSettlesAfterTimingEviction test supplies exactly this sequence and will time out waiting for window.reports to become 1.

return entries.length === 0;

How this was verified: Traced darkStylesheetSettled: listeners are added after load/error already fired; empty getEntriesByName skips the responseEnd branch and returns false; checkSettled then bails on !darkStylesheetSettled(link). WebUiThemeHookTest.kt lines 76–85 clear timings then expect reports==1, which this path cannot produce.

Consensus: 3/3 passes agreed

Files changed (4) — the diff as I read it
  • mobile/src/androidTest/java/net/activitywatch/android/DashboardThemePaintTest.kt — Moves HTTP request handling into concurrent threads, waits for the dashboard pathname before polling theme state, and adds descriptive await messages.
  • mobile/src/androidTest/java/net/activitywatch/android/WebUiThemeHookTest.kt — Adds WebView tests for a stylesheet pending before hook installation and a failed stylesheet whose Resource Timing entries have been cleared.
  • mobile/src/main/java/net/activitywatch/android/WebUiTheme.kt — Registers stylesheet completion listeners before checking pre-install state and consults resource timing responseEnd values when the document is complete.
  • mobile/src/test/java/net/activitywatch/android/WebUiThemeTest.kt — Adds string assertions for getEntriesByName and responseEnd in ANDROID_THEME_HOOK_JS.
Previous review passes
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;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

⚠️ P2 — An empty timing list does not establish that the stylesheet is still loading. For example, a document with CSP 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.

@TimeToBuildBob TimeToBuildBob Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

@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
@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

API 29 E2E failed on c0d5b95 at DashboardThemePaintTest line 192: timed out waiting for location.href after /early → / (run 40334359221, logcat 14:06:40.711–14:06:58.965). Both WebUiThemeHookTest methods passed on that same API 29 job.

The paint-test HTTP server handled one connection at a time, so a leftover /dark.css held on the latch blocked GET /. Pushed b3033f4: concurrent handlers, wait on location.pathname, stylesheetRequests >= 3. Production hook unchanged.

Greptile withdrew the preInstallFailureSettlesAfterTimingEviction timeout claim; I am not re-triggering review. #332 stays open until this E2E is green and its acceptance criteria are re-read.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

Convergence adjudication deferred (freshness gate).

Current head b3033f4f1fbc321db2c2080aaaf173c8132b8680 postdates both review passes:

  • Bob AI review marker: 7e7bcee79f70 (2026-10-26 12:24Z)
  • Greptile last reviewed commit: c0d5b95db23e (2026-10-26 13:56Z)

No classification, dismissal, or merge recommendation on this unreviewed head. No Greptile re-trigger (convergence cap).

Current-head snapshot (not merge evidence):

  • CI: Test, Test E2E (29), and R8 smoke all passed; mergeable CLEAN.
  • Greptile P1 "New theme test times out" was withdrawn by Greptile after the API 36 reproduction; both hook tests also passed on the API 29 job at c0d5b95. Thread left open — not resolved in this pass.
  • Remaining open threads: Greptile P2 "Checks miss the race"; Bob AI-review P2 48be1d6e621d (empty timing list / missed error listener). Neither is classified here.
  • Production hook is unchanged in b3033f4 (paint-test HTTP concurrency only).

Awaiting automatic current-head review (bob-ai-review-sweep). A later cycle can adjudicate once review provenance covers this head. #332 stays open until its acceptance criteria are re-read against a reviewed green head.

@TimeToBuildBob

Copy link
Copy Markdown
Contributor Author

The freshness deferral is now cleared: the updated Bob review explicitly covers b3033f4f1fbc (15:44Z). Current-head CI passed Test, API 29 E2E and R8 smoke.

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 48be1d6e621d remains open. The proposed null-sheet/no-timing missed-error case has not been reproduced on the tested WebViews; passing those tests does not exclude it on every supported version. An unconditional empty-timing fallback would risk restoring the in-flight early reveal, so I made no speculative code change. Maintainer disposition or an affected-WebView reproduction is the next step; this is not an all-clear or a request for another review round. #332 remains open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: emulator jobs (Test E2E, R8 smoke) fail non-deterministically — red run gives no code signal

1 participant