feat(ui): prove nested and concurrent Shadow DOM overlay behavior (YPE-5355) - #375
feat(ui): prove nested and concurrent Shadow DOM overlay behavior (YPE-5355)#375abharms wants to merge 24 commits into
Conversation
🦋 Changeset detectedLatest commit: 0d6b80f 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…ype-5355-nested-concurrent-shadow-dom-overlays # Conflicts: # docs/shadow-dom-isolation-plan.md
abharms
changed the title
|
@codex review |
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: 0336397c2f
ℹ️ 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: 0c62006e8d
ℹ️ 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: 73532013d9
ℹ️ 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".
abharms
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0481b00059
ℹ️ 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: 57fb02fc0d
ℹ️ 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: 0323bce573
ℹ️ 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: 4ce1652fa4
ℹ️ 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: 48f40d5bdf
ℹ️ 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: 5dd0b8923d
ℹ️ 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: 03333e1bf9
ℹ️ 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: 8fd4650e6d
ℹ️ 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: 1ef3dee951
ℹ️ 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: d61a6fa556
ℹ️ 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".
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Jira ticket: YPE-5355
Summary
Standards: 0 must-fix. Worst: none.
Spec: 0 must-fix on this SHA. Worst: none.
CI: pass.
This PR changes UI. I would approve. Cam must look at the overlay proof first.
There are no before or after screenshots on this PR. Add them with before-and-after.
For Agents: pin SHA
Pin: d61a6fa vs main. Ticket YPE-5355. Author abharms.
UI proof (Storybook/Chromium overlays): COMMENT. Would approve. Do not APPROVE.
Empty changeset. Production Dialog/Popover/ShadowRootHost wiring is deferred. Do not treat that as a missing requirement on this SHA.
Greptile 5/5. Partner bots: Greptile and Codex. Codex P2s on earlier SHAs are resolved. Do not reopen. Last Codex note about tabbable() without getShadowRoot is not a must-fix: YPE-5355 proves nested overlays in one shadow root, not nested custom-element shadows inside the owner.
CI: Build Greptile Review Integration Tests Lint Lint Commit Messages Lint PR Title Locale Ownership Require Changeset Test Type Check i18n Check pass. Coverage skipped.
By Code Reviewer bot, sent on behalf of Cam.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Jira ticket: YPE-5355
Summary
Standards: 1 must-fix. Worst: the model still returns an opener that cannot take focus.
Spec: 0 must-fix on this SHA. Worst: none.
CI: pass.
This PR changes UI. Do not approve until the restore contract lives in the model.
There are no before or after screenshots on this PR. Add them with before-and-after.
For Agents: pin SHA
Pin: d61a6fa vs main. Follow-up: Standards must-fix on unmount openerTarget. Do not reopen Codex threads. Spec still 0.
UI: COMMENT. Do not APPROVE.
CI unchanged pass. Greptile 5/5. Partner: Greptile + Codex.
By Code Reviewer bot, sent on behalf of Cam.
Co-authored-by: Cameron Pak <cameronandrewpak@gmail.com>
| /** @internal */ | ||
| export type ShadowOverlayKind = 'modal' | 'nonmodal'; | ||
| /** @internal */ | ||
| export type ShadowOverlayPhase = 'active' | 'exiting'; |
There was a problem hiding this comment.
praise: I like the @internal JSDoc comments!
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Jira ticket: YPE-5355
Summary
Standards: 0 must-fix. Worst: none.
Spec: 0 must-fix on this SHA. Worst: none.
CI: pass.
I would approve the code. This UI proof still needs Cam's visual review. The PR has no image artifacts, so add before-and-after screenshots with before-and-after.
For Agents: pin and review state
- Pin HEAD:
0d6b80f9380cc284c3f8456f8d987861fed184b5 - Base:
c7649829d890818fb1b66a4853d33997c27f1b4f - Review is COMMENT only because this PR changes Storybook UI proof and Cam must inspect it visually.
- Prior unfocusable-opener finding is fixed. The model returns ordered focus candidates, and the adapter advances when a candidate does not accept focus.
- Greptile: 5/5 on this SHA. Codex and Greptile findings on earlier SHAs are resolved or outdated. No new inline comments.
- CI: Build, Bundle Size, Integration Tests, Lint, Lint Commit Messages, Lint PR Title, Locale Ownership, Test, Type Check, i18n Check, Require Changeset, and Greptile Review pass. Publish coverage badges skipped.
- PR body validation counts are stale after later commits: it says 8/8 unit tests and cites audit SHA
7353201.
By Code Reviewer bot, sent on behalf of Cam.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Jira ticket: YPE-5355
Summary
Standards: 0 must-fix. Worst: none.
Spec: 0 must-fix on this SHA. Worst: none.
CI: pass.
I would approve the code. This UI proof still needs Cam's visual review. The PR has no image artifacts, so add before-and-after screenshots with before-and-after.
For Agents: pin and review state
- Pin HEAD:
0d6b80f9380cc284c3f8456f8d987861fed184b5 - Base:
c7649829d890818fb1b66a4853d33997c27f1b4f - Review is COMMENT only because this PR changes Storybook UI proof and Cam must inspect it visually.
- Prior unfocusable-opener finding is fixed. The model returns ordered focus candidates, and the adapter advances when a candidate does not accept focus.
- Greptile: 5/5 on this SHA. Codex and Greptile findings on earlier SHAs are resolved or outdated. No new inline comments.
- CI: Build, Bundle Size, Integration Tests, Lint, Lint Commit Messages, Lint PR Title, Locale Ownership, Test, Type Check, i18n Check, Require Changeset, and Greptile Review pass. Publish coverage badges skipped.
- PR body validation counts are stale after later commits: it says 8/8 unit tests and cites audit SHA
7353201.
By Code Reviewer bot, sent on behalf of Cam.
|
Hey Austin, thank you for your hard work on this. I was processing back and forth with my agent to figure out if there's any possible way that this could be simpler because this approach feels a little bit advanced and a little bit it triggered a small warning sign in my head to just validate and double-check if this is the easiest path to solve what we're trying to solve. At this point, I want to give you the information and then give you the option to make the full decision on whether or not you pursue something different or you continue with this. See AI slop verdict
|
4 participants
Summary
ShadowRootHostand revalidatedRequirement evidence
shadow-overlay-ownership.test.tsand theShadow overlay ownershipChromium storyShadowRoot.activeElementassertionsValidation
pnpm --filter @youversion/platform-react-ui exec vitest run --project unit src/lib/shadow-overlay-ownership.test.ts— 8/8 passedpnpm --filter @youversion/platform-react-ui exec vitest run --project storybook src/lib/shadow-overlay-ownership.stories.tsx— Chromium proof passedpnpm --filter @youversion/platform-react-ui typecheck— passedpnpm lint— passedpnpm test— the ticket tests pass; an unchanged BibleReader test intermittently exceeds its 5-second timeout under full parallel load and passes in isolation (46/46)7353201— 0 findingsCompatibility and limitations
ShadowRootHost, Dialog, Popover, and directVerseActionPopoverruntime paths are unchangedJira
YPE-5355
Greptile Summary
The PR defines and proves a proposed root-owned LIFO ownership model for nested and concurrent overlays while leaving production integration explicitly unsupported.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Root[Shadow root] --> Registry[Root-owned overlay registry] Registry --> Stack[LIFO overlay stack] Stack --> Owner[Topmost eligible owner] Owner --> Focus[Focus ownership] Owner --> Dismissal[Escape and outside dismissal] Owner --> Inertness[Modal inertness] Owner --> Exit[Descendant-first exit ordering] Exit --> Restore[Ordered focus restoration] Registry -. production wiring deferred .-> Host[ShadowRootHost]Reviews (18): Last reviewed commit: "fix(ui): defer focus acceptance to overl..." | Re-trigger Greptile
Context used (3)