fix(ui): address shadow DOM review regressions (YPE-5713) - #388
Conversation
🦋 Changeset detectedLatest commit: 0df3443 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. |
abharms
changed the title
There was a problem hiding this comment.
Review
No Jira. Spec is the PR body.
Summary
Standards: 1 non-blocking issue — localized label in test query. Spec: 0 must-fix — none.
For Agents: review context
PR #388 at 0df3443; diff is 0222ae9...0df3443. Focused local UI tests could not run because dependencies were absent and npm was unavailable. Greptile is informational only.
By Code Reviewer bot, sent on behalf of Cam.
| await user.click(priorControl); | ||
| const dialog = await screen.findByRole('dialog'); | ||
| await waitFor(() => expect(document.activeElement).toBe(dialog)); | ||
| await user.click(screen.getByRole('button', { name: 'Copy' })); |
There was a problem hiding this comment.
issue (non-blocking): Avoid hard-coding the localized Copy label; focus-restoration coverage should not depend on English UI text.
For Agents: stable copy action lookup
docs/testing.md requires stable roles and behavior rather than i18n prose. This new query uses the exact English accessible name. Target the action through the translation key or a stable behavior-level hook instead.
By Code Reviewer bot, sent on behalf of Cam.
3 participants
Summary
Targets
journey-to-the-shadow-domafter PR #381.Verification
The broader 39-story run passed every test but exited nonzero for the previously reproduced Default-story font-load rejection with a placeholder app key. The focused 25-story run exits cleanly.
Scope
No public API expansion, new overlay manager, font loader, unit rewrite, or broader automatic-isolation rollout. PR #381's documented peer-concurrency and rapid/nested final-focus gaps remain production gates for YPE-5356.
Greptile Summary
This PR addresses touch re-anchoring, focus restoration, and constrained settings scrolling regressions while reconciling Shadow DOM documentation.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Verse pointer interaction] --> B{Pointer targets verse?} B -->|Yes| C[Record event-keyed verse verdict] B -->|No| D[Allow outside dismissal] C --> E[Radix deferred outside handling] E --> F[Prevent dismissal and re-anchor] D --> G[Close popover and retain outside focus] F --> H[Keep verse actions open] G --> I[Clear selection] J[Popover opens] --> K[Capture prior focus target] K --> L[Focus action content] L --> M[Popover closes] M --> N{Outside interaction retained focus?} N -->|No| O[Restore captured target] N -->|Yes| P[Preserve newly focused outside element]Reviews (3): Last reviewed commit: "test(ui): wait for placement before meas..." | Re-trigger Greptile