test(ui): add cross-browser Shadow DOM coverage - #420
Conversation
🦋 Changeset detectedLatest commit: 03d7661 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…test/shadow-dom-cross-browser # Conflicts: # docs/shadow-dom-isolation-plan.md # packages/ui/src/components/bible-version-picker.shadow-isolation.stories.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4a04a7a3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "build-storybook": "pnpm generate:i18n && pnpm build:css && pnpm build:storybook-css && storybook build", | ||
| "test:integration": "vitest run" | ||
| "test:integration": "vitest run", | ||
| "test:shadow-browser": "VITEST_STORY_TAG=shadow-dom vitest run --project storybook" |
There was a problem hiding this comment.
This command runs only stories tagged shadow-dom, but RapidCloseReopenIgnoresInvalidOpenerEvidence in shadow-overlay-production-seam.shadow-isolation.stories.tsx retains only tags: ['!dev']. Consequently, the Firefox/WebKit matrix silently skips the scenario covering disconnected openers and openers moved into the light DOM, another shadow root, or another document, even though docs/shadow-dom-isolation-plan.md now says those rapid-reopen journeys receive Firefox and WebKit coverage. Add the selection tag to that story so regressions in this cross-root/document behavior are exercised by the new jobs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cae444479d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void expect( | ||
| focused === overlay || | ||
| (focused !== null && currentDialog !== null && currentDialog.contains(focused)), | ||
| ).toBe(true); |
There was a problem hiding this comment.
When polling does not run until the replacement dialog has already reopened, the currentDialog branch lets this step pass without ever verifying focus during the overlay-only interval. A regression that briefly drops focus to the document after the first dialog unmounts would therefore escape the new cross-browser coverage as long as the reopened dialog later receives focus. Make the close/reopen gap deterministic and require currentDialog to be absent and the retained overlay to own focus before proceeding to the reopen step.
AGENTS.md reference: packages/ui/AGENTS.md:L63-L65
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 744bf47459
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { createRoot, type Root } from 'react-dom/client'; | ||
| import { expect, waitFor } from 'storybook/test'; | ||
| import { waitFor } from 'storybook/test'; | ||
| import { expect } from 'vitest'; |
There was a problem hiding this comment.
When these stories are opened through ordinary pnpm storybook rather than the Vitest browser project, no Vitest worker context exists, so the runner-only vitest export fails when the play assertions execute. The same import was added to all seven edited Shadow DOM story files, leaving their standalone Storybook interactions broken even though the Vitest jobs pass. Keep the portable expect from storybook/test, or use runner-neutral checks inside polling callbacks.
AGENTS.md reference: packages/ui/AGENTS.md:L59-L61
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c097d66034
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| globalThis.addEventListener('unhandledrejection', (event) => { | ||
| if (event.reason instanceof Event) event.preventDefault(); |
There was a problem hiding this comment.
This listener suppresses every unhandled rejection whose reason is an Event, not just the optional Fonts API stylesheet failure described above it. If another React stylesheet/resource load fails—or application code rejects with a load event—preventDefault() hides that failure from Vitest, allowing the browser suite to pass despite an unexpected resource regression. Check that the event originated from the expected Fonts API link before suppressing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7339f4144
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const unexpectedConsoleErrors = consoleError.mock.calls.filter( | ||
| ([firstArgument]) => !(firstArgument instanceof Event), | ||
| ); |
There was a problem hiding this comment.
Although the setup-level rejection filter is now scoped to the Fonts API stylesheet, this new per-story filter still discards every console.error whose first argument is an Event. During either realistic lifecycle story, an unrelated stylesheet, image, or resource failure reported as an event will therefore satisfy the “no unexpected console errors” assertion and let the cross-browser suite pass. Match the expected Fonts API link here as well instead of exempting all events.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 0 must-fix. Spec: 0 must-fix. Primary concern: none. One non-blocking suggestion to reuse Storybook's existing wait utility.
Review evidence
- Scope: reviewed the PR HEAD against its merge-base, YPE-5946, repository guidance, story selection, runtime CSS changes, and prior bot feedback.
- Source trace: the dedicated Firefox/WebKit matrix selects the 22 Shadow DOM stories and two document-root spacing regressions; the default Chromium integration selection remains unchanged. Earlier bot concerns are addressed in the current code.
- CI: Storybook Tests reported 24/24 focused tests passing in each Firefox and WebKit job and 649/649 integration tests passing. These are CI results, not locally rerun tests.
- Limits: the actual Safari 26.6.2 smoke result is author-reported in the PR documentation, not independently reproduced here. Screen-reader validation is deferred per ticket. The suggested wait-utility substitution has not been tested; verify it in all three browsers before adopting it.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| const deadline = Date.now() + timeout; | ||
| let lastError: unknown; | ||
|
|
||
| while (Date.now() <= deadline) { |
There was a problem hiding this comment.
suggestion: Could this delegate to Storybook's waitFor with a five-second timeout instead of maintaining a polling loop?
For Agents: reuse the existing wait utility
Storybook 10.5.10 re-exports Testing Library's
waitFor, which handles promise-returning callbacks and accepts{ timeout: 5_000 }. Keep this helper as a local five-second-default wrapper so its callers need not change. That removes the custom deadline/retry/sleep logic; the current loop cannot time out while a callback promise remains pending. The Firefox CI run after this loop was introduced still failed, so the history does not establish that the loop is needed to fix those failures. The tradeoff is Storybook's instrumented scheduling: please run the focused Chromium, Firefox, and WebKit suites before switching. This is a cleanup suggestion, not a blocker.
Written by Code Reviewer bot on behalf of Cam.
| // The --ci flag will skip prompts and not open a browser | ||
| storybookScript: 'pnpm storybook --no-open', | ||
| tags: { include: ['integration'] }, | ||
| tags: { include: storyTags }, |
There was a problem hiding this comment.
praise: Selecting focused story tags through the existing Storybook project keeps the Chromium integration default intact while adding Firefox and WebKit coverage.
For Agents: focused browser selection
The
integrationfallback remains the default, while the new job supplies theshadow-dom,cross-browsertag set. This avoids a separate browser-specific Storybook project and its duplicated setup.
Written by Code Reviewer bot on behalf of Cam.
2 participants
Summary
Jira
YPE-5946 — Add cross-browser Shadow DOM coverage
Validation
pnpm lintNotes
journey-to-the-shadow-domThe PR appears safe to merge.
Summary
The PR expands focused Shadow DOM Storybook coverage across Chromium, Firefox, and Playwright WebKit while recording isolated Safari validation.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Storybook Shadow DOM stories] --> B{Browser target} B --> C[Chromium integration suite] B --> D[Firefox focused suite] B --> E[Playwright WebKit focused suite] C --> F[Lifecycle and isolation assertions] D --> F E --> F G[Isolated SafariDriver smoke] --> FReviews (21) · Last reviewed commit: "test(ui): reuse Storybook wait helper"