Skip to content

fix(ci): wait for fragment navigation policy reports - #25017

Merged
nwparker merged 1 commit into
mainfrom
fix/mobile-preview-fragment-report-wait
Oct 3, 2026
Merged

nwparker merged 1 commit into
mainfrom
fix/mobile-preview-fragment-report-wait

Conversation

@nwparker

@nwparker nwparker commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​7 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​6
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​12 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​12

ELI5

A mobile preview browser check could fail while the browser correctly blocked a fragment-link navigation. The check now waits for the browser’s policy report before reading it.

What Changed

The existing preview driver accepts a report to await after the click, using its existing report collector and test-abort diagnostics. The fragment-link case also checks that the click actually succeeded. The shipped content policy, preview component and hidden-link control keep their existing behavior and assertions.

Why

The previous check read a presence assertion after a fixed 200 ms settle. That does not establish whether an asynchronous policy report has arrived. Reusing the existing nonce-scoped report wait makes the browser’s actual report the precondition, instead of increasing a delay or weakening the assertion.

Linked Issue

Unblocks the mobile browser CI failure on #24790; refs #13287. This is a separate test-driver correction, with no additional OpenCode product scope.

Visual Proof

N/A — browser test synchronization only; no product UI or policy changes.

Testing

  • Reproduced the exact CI assertion with real headless Chromium 154 and a controlled one-second delay collecting its native frame-src report. The click succeeded; the old driver read only image/font reports before the frame report was recorded.
  • The corrected driver passed the same delayed-report control and preserved the hidden-link arm’s no-navigation assertion.
  • All 32 existing preview cases passed on Chromium and installed Playwright WebKit in the fully materialized final source. Private HOME, all four XDG directories, temp and background launch were used.
  • Formatting, strict lint and all six changed-line quality scans passed for both changed files. Every one of 32,227 unowned main leaves remains exact.

The CI browser was Chromium 154.0.8037.57; local Chromium was 154.0.8037.97. The delayed collection is a controlled reproduction of the presence-reading race, not a claim to reproduce CI’s exact network scheduling. An initial local WebKit run could not find the installed browser after HOME isolation; only that engine was rerun with its explicit existing cache path, followed by the full final-source suite.

Review

Two existing test files change. The driver reuses the current collector, nonce ownership, cancellation and failure diagnostics. Public CI passed: 15 successful checks and 17 intentional skips on the exact final head.

Agent skill upstream boundary

  • Not applicable; no upstream skill source was copied or translated.

Notes

The change is confined to browser test synchronization. It adds no platform behavior, remote protocol or source-control assumptions.

Checklist

  • Small, focused change; mechanism and alternative explained.
  • No product visual change.
  • Existing positive and negative browser controls retained.
  • Source ownership, lint and formatting verified.
  • Public CI passed.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • reportAfterAct option on the arm driver — openPreviewArm now awaits the browser's nonce-scoped CSP report after the action, reusing pollReportsUntil/untilAborted/describePreviewFrame rather than adding a new wait path. Placed after settleAfterMount and guarded on !actError.
  • Fragment-tap case waits on evidence — the shown arm passes reportAfterAct: 'frame-src' and asserts shown.actError is null, replacing the fixed 200 ms settle as the precondition for reading shown.reported.

The wait is sound: a srcdoc frame has no URL for frame-src to match, so no mount-time report can satisfy it — it only resolves on the refused fragment navigation, and the existing expect(shown.reported).toContain('frame-src') still pins the outcome. The hidden arm's absence assertion is unchanged and remains the documented absence arm to the shown presence arm.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 89eecff2-7787-4dcd-9406-8dd1f33bf87f
📥 Commits

Reviewing files that changed from the base of the PR and between 5e65871 and 5180a42.

📒 Files selected for processing (2)
  • config/scripts/mobile-web-app-html-preview-render.test.mjs
  • config/scripts/mobile-web-app-preview-arm-driver.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The preview arm driver accepts an optional reportAfterAct setting. When enabled and the action succeeds, it waits for a matching CSP refusal report. If the report does not arrive, the timeout diagnostic includes the expected refusal and a frame diagnosis. The fragment-navigation test requests a frame-src report after the tap and checks that the action has no error.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 5180a

The test now waits for the fragment-navigation refusal report, which cannot be satisfied by an earlier matching report from this fixture. No concrete merge-blocking risk is evident; public CI is still pending.

Architecture Summary

Architecture risk: 🔵 Low · up to 5180a

The change affects 1 system.

Changed systems: config

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — config (service) was modified; 2 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in config/scripts/mobile-web-app-html-preview-render.test.mjs: The shown arm now requests a frame-src report after the tap and checks actError is null. Previously it opened the arm without waiting for the report or asserting that the tap succeeded.
  • observed — Modified behavior in config/scripts/mobile-web-app-preview-arm-driver.mjs: Imports CSP report polling and frame-diagnosis helpers for the driver.
  • observed — Modified behavior in config/scripts/mobile-web-app-preview-arm-driver.mjs: Adds the optional reportAfterAct setting, defaulting to null, to openPreviewArm’s options.
  • observed — Modified behavior in config/scripts/mobile-web-app-preview-arm-driver.mjs: When reportAfterAct is set and actError is absent, waits for a matching refusal report for this nonce, abort signal, and arm; if the report does not arrive, the timeout diagnostic includes the expected refusal and a frame diagnosis.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the CI fix: waiting for fragment-navigation policy reports.
Description check ✅ Passed The description covers the change, reason, testing, visual-proof exception, review notes, and checklist. It identifies related issues, though the template asks for a direct issue link.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

1 participant