Conversation
🦋 Changeset detectedLatest commit: e6a27da The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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. |
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.
Review evidence
- Scope: reviewed pinned revision
ce97bde, YPE-5890, the full merge-base diff, surrounding Storybook/MSW behavior, and existing review history. - Method: independently traced the old and new polling paths, checked all migrated selectors, errors, assertions, and timeout call sites, and consulted Oracle on stability, security, and alignment.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Helper behavior and timing | Source equivalence across all five migrated stories | Preserved, including explicit five-second element waits and default ShadowRoot waits | Coordinator |
| Full Storybook project | pnpm --filter @youversion/platform-react-ui test:integration |
617 passed; the global font mock resolves the nine font-request rejections seen on the prior commit | CI |
| Build, unit tests, typecheck, lint, size, i18n, changeset | Required PR checks | Passed | CI |
| Diff integrity | git diff --check 8c240ed...ce97bde |
Passed | Coordinator |
- Limits: an isolated local Storybook rerun could not start because this orb lacks Playwright's pinned Chromium binary; the authenticated CI browser run is green.
- CI and bot review: all required checks pass; no unresolved human or bot threads.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| timeout?: number; | ||
| } | ||
|
|
||
| export async function waitForElement<ElementType extends Element>( |
There was a problem hiding this comment.
praise: Keeping this helper limited to retry mechanics preserves the scenarios as readable evidence instead of turning it into a testing framework.
For Agents: narrow extraction
The helper retains caller-owned selectors and failure messages, while the production story keeps its explicit five-second element waits and ShadowRoot waits keep their original default timing. Ordinary jsdom helpers also remain independent of
storybook/test.
Written by Code Reviewer bot on behalf of Cam.
|
|
||
| export const globalHandlers = [ | ||
| // Font stylesheet loaded by YouVersionProvider | ||
| http.get('*/v1/fonts/1/stylesheet', () => |
There was a problem hiding this comment.
thought: This global mock is justified by the complete Storybook run, with the tradeoff that inherited previews now use fallback typography instead of loading the real brand font.
For Agents: deterministic font loading
Before this handler, all 617 assertions passed but nine stories produced unhandled stylesheet rejections and failed the job. The handler makes that run deterministic and green. Because the response is intentionally empty CSS, Storybook is no longer evidence for the actual Untitled Serif asset in stories that inherit global handlers; the dedicated font tests continue to own that contract.
Written by Code Reviewer bot on behalf of Cam.
…ype-5890-consolidate-storybook-dom-wait-helpers # Conflicts: # packages/ui/src/test/mocks/handlers.ts
2 participants
Summary
Targets
journey-to-the-shadow-dom, which contains the Shadow DOM Storybook evidence this cleanup consolidates.storybook/testwaitForElementimplementations and repeated ShadowRoot pollingRequirement evidence
packages/ui/src/test/storybook-dom.tsowns the common element and ShadowRoot polling mechanics.waitForElementcall in the production overlay story receives its explicit{ timeout: 5_000 }option. ShadowRoot waits retain their original default timing.shadow-root-host.test.tsxis unchanged and the UI unit project passes 544/544.Review scope
Required outcomes:
ShadowRootHostassertions unchangedPermitted support work:
waitForShadowRoothelper for matching repeated polling sitesNon-goals:
Please treat a finding as blocking only when it identifies an unmet in-scope requirement, a documented repository-standard violation in added or modified code, or a concrete regression or defect caused or worsened by this diff. Label other valid improvements as non-blocking follow-ups.
Verification
pnpm --filter @youversion/platform-react-ui exec vitest run --project storybook <five migrated story files>— 21/21 passed in Chromiumpnpm --filter @youversion/platform-react-ui test— 544/544 passedpnpm --filter @youversion/platform-react-ui typecheck— passedpnpm lint— passedgit diff --check— passed91c7512— no implementation, compatibility, or documented-standard violationsThe unauthenticated local complete Storybook run executes all 73 assertions successfully but exits nonzero on nine existing network rejections in unrelated stories. The repository's Storybook CI workflow injects the staging credentials absent locally and is the authoritative complete-project evidence. The default concurrent workspace test run also reproduced an existing five-second controlled-reader timeout; that unchanged UI suite passes 544/544 in isolation.
Audit and compatibility notes
Jira
The PR appears safe to merge.
Summary
Consolidates repeated Storybook DOM polling into a Storybook-specific helper while preserving scenario-specific interactions, assertions, error messages, and production element-wait timeouts.
waitForElementandwaitForShadowRoothelpers.Reviews (3) · Last reviewed commit: "Merge remote-tracking branch 'origin/jou..."