Conversation
🦋 Changeset detectedLatest commit: 6315896 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 |
YPE-5889
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. |
![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: bb78417ce3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 542f73ae2a
ℹ️ 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".
| return; | ||
| } | ||
| if (!container || capturedRestoreFocusRef.current) return; | ||
| if (!container || capturedRestoreFocusRef.current || restoreFocusRef.current) return; |
There was a problem hiding this comment.
When Content has a shorter exit animation than Overlay and the dialog is programmatically reopened after Content unmounts but before Overlay does, the first onCloseAutoFocus consumes and clears restoreFocusRef while modal presence is still retained by the Overlay. This guard then allows the reopen to capture the transient post-unmount focus (typically the document body/host) instead of the original opener, so the final close does not restore focus correctly. The original target needs to remain associated with the retained modal session until all modal presence ends, not merely until Content's close-autofocus fires.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a305090ab2
ℹ️ 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".
| if (event.defaultPrevented) { | ||
| restoreFocusRef.current = null; | ||
| restoreFocusRootRef.current = null; | ||
| return; |
There was a problem hiding this comment.
When Content has already unmounted during a staggered exit, restoreFocusWhenModalReleased has copied the opener into ShadowRootHost's pending-focus slot. If the dialog reopens before Overlay unmounts and the consumer prevents the final onCloseAutoFocus, this branch clears only the hook-local refs; when modal presence finally ends, setModalPresent still focuses the previously queued opener, overriding the consumer's preventDefault(). The prevented path therefore also needs to cancel the host-owned deferred restoration for this dialog session.
Useful? React with 👍 / 👎.
…ype-5889-shadow-overlay-runtime # Conflicts: # docs/shadow-dom-isolation-plan.md
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 revision, Jira requirements, PR discussion, prior review threads, changed code, nearby lifecycle code, tests, ADR, and rollout plan.
- Method: independently reviewed Standards, Spec, and Correctness; traced initial open, content-first exit, overlay-only presence, reopen, final close, prevented autofocus, invalid targets, nested overlays, cleanup, and light-DOM compatibility. Current Radix source and relevant Shadow DOM issues were checked for a maintained alternative; none supplies owner/root-aware deferred restoration, reclaim, and cancellation.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Dialog focus lifecycle | Source trace through use-shadow-dialog-focus.ts, shadow-root-host.tsx, and Radix Dialog/FocusScope/Presence |
Required ownership and root invariants preserved | Coordinator |
| Focused dialog regressions | vitest run --project unit src/components/ui/dialog.test.tsx |
5/5 passed | Coordinator |
| Rapid close/reopen Chromium evidence | Focused story run repeated three times | Passed 3/3 | Coordinator |
| UI type safety and build | UI typecheck; turbo build --force --filter=@youversion/platform-react-ui |
Passed; 3 build tasks successful | Coordinator |
| Full PR checks | Integration Tests and current check suite | 621/621 tests passed; all required checks green | CI |
- Limits: one combined local story-file run hit a transient pre-existing timing assertion and an unrelated pre-existing layout assertion. The affected rapid-reopen story then passed three consecutive focused runs, and CI passed the full suite. Cross-browser and assistive-technology validation remain documented rollout follow-ups.
- CI and bot review: all required checks passed; no unresolved current-HEAD bot finding remains.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| [canRestoreFocus], | ||
| ); | ||
|
|
||
| const reclaimFocusRestore = useCallback( |
There was a problem hiding this comment.
praise: Binding deferred restoration to the dialog owner keeps this fix narrow while preserving the original opener through interrupted exit presence.
For Agents: owner-bound focus lifecycle
The
schedule,reclaim, andcanceloperations model the exact retained-presence lifecycle without introducing the out-of-scope overlay manager. Carrying the captured root also makes final validation reject disconnected and moved targets at the point of use.
Written by Code Reviewer bot on behalf of Cam.
|
I just want to say thank you for all the pre-reviews that you do before you send it out for review. It means a lot and makes reviewing easy |
…ype-5889-shadow-overlay-runtime # Conflicts: # packages/ui/src/test/mocks/handlers.ts
2 participants
Summary
Targets
journey-to-the-shadow-dom, which contains the existing Shadow DOM overlay seam.This is pre-rollout work. The browser stories exercise the real shared overlay implementations, but no released production component currently uses this Shadow DOM overlay path.
Requirement evidence
useShadowDialogFocusretains the first captured opener through the interrupted exit;ShadowRootHostrestores it after modal release.Review scope
Required outcomes:
Permitted support work:
Non-goals:
Review only against YPE-5889 and this stated scope. Treat correctness defects, regressions, explicit repository-rule violations, and unmet required outcomes as blocking. Treat broader hardening, optional refactors, and out-of-scope improvements as follow-up work.
Verification
pnpm --filter @youversion/platform-react-ui typecheck— passedpnpm lint— passedpnpm turbo build --force --filter=@youversion/platform-react-ui— passedpnpm test— passed on clean rerun; all six workspace tasks green, UI 547/547pnpm exec prettier --check ...andgit diff --check— passedThe first concurrent full-suite run hit the existing five-second timeout in
bible-reader-controlled.test.tsx. The unchanged timed-out test passed 33/33 in isolation, and the complete suite then passed on rerun.Compatibility and limitations
Jira
The PR appears safe to merge.
Summary
The PR preserves the original focus target when a Shadow DOM dialog reopens during retained exit presence and validates that target before final restoration.
Diagram
sequenceDiagram participant O as Original opener participant D as Shadow dialog participant H as ShadowRootHost O->>D: Open dialog D->>D: Capture opener and root D->>H: Schedule restoration on close Note over D,H: Overlay remains during exit D->>H: Reclaim restoration on rapid reopen D->>H: Schedule restoration on final close H->>H: Release final modal presence H->>H: Validate connection, document, and root H->>O: Restore focus when validReviews (9) · Last reviewed commit: "Merge remote-tracking branch 'origin/jou..."