1 issue found across 4 files

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/web/tests/integration/screenshot-caret.test.ts">

<violation number="1" location="packages/web/tests/integration/screenshot-caret.test.ts:38">
P2: This test never exercises the closed-shadow case described by the regression: `/values` contains only light-DOM inputs, and a document-level `MutationObserver` does not observe mutations inside shadow roots. Add an editable element inside a closed root and observe that root before asserting no mutations.</violation>
</file>

<sub>Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.

View guided diff | Re-trigger cubic</sub>

All reported issues were addressed across 6 files (changes from recent commits).

<sub>Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.

View guided diff | Re-trigger cubic</sub>

Thanks @DeryFerd, the approach is right: a constructed sheet in adoptedStyleSheets leaves element style attributes alone and is not blocked by style-src. I checked it against a real Chromium page served with Content-Security-Policy: default-src 'none'; style-src 'self', a document <style> that the policy blocks, an open shadow root, and a same-origin and a cross-origin iframe, each with a closed shadow root. During the capture every caret was rgba(0, 0, 0, 0). Afterwards every root was back to its original color and had no adopted sheets left, both when the screenshot succeeded and when page.screenshot threw. Reverting observe.ts to main fails 4 of your 8 tests. Good.

The implementation can be a lot smaller, though.

Must fix

1. packages/web/src/screenshot-caret.ts: drop the global token. You can return the undo function and keep a JSHandle to it: frame.evaluateHandle(hideCaret), then handle.evaluate((show) => show()) and dispose() in the finally. That removes randomUUID, TOKEN_PREFIX, Adoption, ParkedSheet, the Reflect.get/set/deleteProperty calls and the shape checks in removeCaretStylesheet. It also fixes a real leak: Reflect.set(globalThis, token, ...) creates an enumerable window property for the length of the capture. I saw Object.keys(window) include it in all three frames, and page code can read or overwrite it. Other things that can go:

   • One CSSStyleSheet per document is enough, since shadow roots of the same document can adopt the same sheet.
   • The seen set isn't needed. An element has at most one shadow root, so element.shadowRoot ?? closed.get(element) reaches each root once.
   • The per-root try/catch isn't needed either, because assigning a same-document sheet can't throw.

   I prototyped this shape. screenshot-caret.ts drops from 132 lines to about 45, and it passed the same CSP, iframe, closed-root and failure checks with no page-visible global:

   function hideCaret(): () => void {
     const closed: unknown = Reflect.get(globalThis, Symbol.for('e2e.closedShadowRoots'));
     const sheet = new CSSStyleSheet();
     sheet.replaceSync('input, textarea, [contenteditable] { caret-color: transparent !important; }');
     const roots: (Document | ShadowRoot)[] = [];
     const visit = (root: Document | ShadowRoot): void => {
       roots.push(root);
       root.adoptedStyleSheets = [...root.adoptedStyleSheets, sheet];
       for (const element of root.querySelectorAll('*')) {
         const shadow: unknown = element.shadowRoot ?? (closed instanceof WeakMap ? closed.get(element) : null);
         if (shadow instanceof ShadowRoot) visit(shadow);
       }
     };
     visit(document);
     return () => {
       for (const root of roots) root.adoptedStyleSheets = root.adoptedStyleSheets.filter((other) => other !== sheet);
     };
   }
   
   export async function withHiddenCaret<T>(page: Page, capture: () => Promise<T>): Promise<T> {
     const undos = await Promise.all(page.frames().map((frame) => inFrame(frame.evaluateHandle(hideCaret))));
     try {
       return await capture();
     } finally {
       await Promise.all(undos.map((undo) => undo && inFrame(undo.evaluate((show) => show()).finally(() => undo.dispose()))));
     }
   }

   (inFrame is withTimeout(work, 1_000, ...).catch(() => undefined). This replaces the duplicated adopt and remove timeout blocks.)

2. packages/web/src/observe.ts:79-93: the screenshot budget is now computed twice and there's an extra scope.check(). budgetMs only feeds the per-frame step, and that step is capped at 1s anyway. Keep the original single scope.read(() => withHiddenCaret(page, () => page.screenshot({...}))) with the original timeout: expression, and cut the 4-line comment down to one line.

3. Tests: if you take ask 1, the in-page functions don't need to be exported. Test behavior through capturePixels instead. Wrap page.screenshot in the test so it reads getComputedStyle(...).caretColor on the CSP page, inside the closed root and inside an iframe during the capture, and checks that everything is restored after. Right now the CSP and closed-root tests call adoptCaretStylesheet directly, so they never prove that capturePixels hides anything. There's also no iframe coverage. The unit mocks in tests/unit/observe.test.ts tell adopt and remove apart by typeof argument === 'string', which breaks as soon as the signature changes. Once the integration test covers ordering and the failure path for real, they can go.

4. packages/web/tests/integration/screenshot-caret.test.ts:69: the shadowMutations assertion can't fail. Playwright's caret: 'hide' only walks open roots (collectRoots uses node.shadowRoot), so main never writes a style inside a closed root either. Either drop it or point it at an open root.

Nits

5. .changeset/screenshot-caret-stylesheet.md: describe the user-visible effect only: screenshots no longer change input styles, so React hydration on SSR pages doesn't warn. "Constructed stylesheet adopted into every frame and shadow root" is an internal detail.
6. packages/web/src/surface.ts:1028: the artifact screenshot still uses Playwright's default caret: 'hide', so it writes the same inline styles. Consider wrapping it with withHiddenCaret and passing caret: 'initial' there too.

This was an automated review.

1 existing issue remains and no new issues found across 9 files (changes from recent commits).

<sub>Heads up: you’ve reached your flex budget. Increase your flex budget or wait for usage to reset.

Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.

View guided diff | Re-trigger cubic</sub>