docs: triage the cat-factory slice-5 overlay-host request - #96
Merged
Conversation
Accept the substance (a pick-one, app-state-keyed modal host with
framework-managed behaviour — focus trap + return, scroll lock, stacked
Escape, a11y — is a real, framework-shaped gap; even our own
nuxt-modal-journey example hand-rolls this chrome without it), redirect
the shape: no defineModule({ overlays }) descriptor seam (slots are the
ingress, per the slice-4 precedent), no /testing subpath, no Nuxt
manifest threading, no route-sync helper, and not Vue-only — the
behaviour is engine-first with React and Vue hosts in the same train.
Corrects the stale Gap D picture: the peer-range/hard-dep residuals the
request re-files were already closed by the shared-peer-dependency
change (#95). Counter-proposal: OverlayEntry (a ComponentEntry superset)
+ defineOverlayHost + resolveOverlay + a pure createOverlayStack in the
engine, managed OverlayOutlet hosts + useModalBehavior in both bindings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ezPjgGYoWHHg1vVNLZUQT
…haviour) Implements the counter-proposal from the cat-factory slice-5 triage: an overlay host — the pick-one, modal dual of the render-all panels — landed engine-first with both bindings in the same train. Contributions ride the existing module -> slot path; no new package, descriptor change, registry plugin, /testing subpath, or Nuxt manifest threading. Engine (@modular-frontend/core): - OverlayEntry<TSubject, TMeta> — a ComponentEntry superset (title is first-class because the shell wires it to aria-label; icons/variants stay in opaque meta), OverlayHostHandle, defineOverlayHost - resolveOverlay: pure pick-one resolver (duplicate-id validation first via the shared collapseEntriesById so the stances can't drift; null id -> null; dangling id -> null, hosts dev-warn — pairById's "missing" stance), resolveOverlayTitle - createOverlayStack: pure LIFO stack with a subscribe seam — the shared "top overlay closes first" semantics both bindings consume - Re-exported by @modular-react/core (export *) and @modular-vue/core Vue (@modular-vue/vue, re-exported from @modular-vue/core): - useOverlay (computed over both runtime slot sources + the active id), <OverlayOutlet> (Teleport, backdrop click-self -> close emit, managed behaviour, #wrap / #empty slots, subject as prop + provide, per- (entry.id, subjectKey) keying, ModuleErrorBoundary label "Overlay", stable data-modular-overlay-* e2e hooks), useOverlaySubject, useModalBehavior — the behaviour standalone for bespoke roots - injectSlotsSource shared with panels (caller-labeled error) React (@modular-react/react): - The same surface with hooks/context/portal: useOverlay, <OverlayOutlet> (empty / wrap / onClose props), useOverlaySubject / OverlaySubjectContext, useModalBehavior (version-bump re-render over the engine stack — a sync-external-store snapshot would read the ticket ref before the activation effect assigns it) Docs: - docs/overlay-host.md guide (entry shape, resolver, both hosts, the behaviour contract, the app-shell recipe, overlays-vs-neighbours) - compositions README "pick by problem shape" table extended 4 -> 5; cross-links from subject-panels.md, remote-capability-manifests.md, and the Nuxt consumer-seam guide; CHANGELOG entry - Gap D verified already closed by the shared-peer change; the new surface is born onto the unified >=0.1.0 <2.0.0 range Verified: turbo typecheck/build/test green across the workspace (214 tasks); engine + Vue + React overlay suites cover pick-one selection, dangling/duplicate stances, stacking, Escape top-first, focus trap + return, scroll lock, subject injection + rekeying, error containment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ezPjgGYoWHHg1vVNLZUQT
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a state-keyed, pick-one modal overlay host with shared core contracts, Vue and React outlets, stacking, focus and scroll management, accessibility behavior, tests, exports, examples, CI coverage, and documentation. ChangesState-keyed overlay host
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/overlay.tsx`:
- Around line 405-413: The React and Vue overlay bindings need an optional
ariaLabelledby prop exposed and forwarded to their dialog elements. Update the
relevant binding props and dialog rendering in packages/react/src/overlay.tsx
(405-413) and packages/vue/src/overlay.ts (439-449), preserving aria-label while
allowing headings rendered by wrap/#wrap to name dialogs when no title is
provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 554b8d56-c887-4ce1-8021-6e562a344899
📒 Files selected for processing (19)
CHANGELOG.mddocs/framework-mode-nuxt.mddocs/overlay-host-triage.mddocs/overlay-host.mddocs/remote-capability-manifests.mddocs/subject-panels.mdpackages/compositions/README.mdpackages/frontend-core/src/index.tspackages/frontend-core/src/overlay.test-d.tspackages/frontend-core/src/overlay.test.tspackages/frontend-core/src/overlay.tspackages/react/src/index.tspackages/react/src/overlay.test.tsxpackages/react/src/overlay.tsxpackages/vue-core/src/index.tspackages/vue/src/index.tspackages/vue/src/overlay.test.tspackages/vue/src/overlay.tspackages/vue/src/panels.ts
…uard backdrop drag-out, refocus on window swap, freeze the behaviour scope Address the PR-96 review findings, prioritizing the architectural ones: - Move the behaviour's framework-neutral DOM semantics into the engine (frontend-core overlay-dom.ts, client-only and SSR-guarded): the single app-wide sharedOverlayStack instance both bindings register on, the counted body scroll lock, and the focusable-scan/Tab-trap rules. One implementation so the behaviour cannot drift between bindings (the collapseEntriesById argument applied to focus and scroll); each binding now contributes only event/reactivity glue. This also makes the "one overlay ordering per app" claim true by construction, across bindings. - Backdrop close is now press-and-release: a press that starts inside the dialog and slips onto the backdrop (text selection, missed drag) no longer closes the overlay. Tested in both bindings. - Initial focus is re-applied when the active window swaps without closing (new optional contentKey on useModalBehavior; the outlets pass the mounted window's key), so focus follows the new content instead of falling to body. Tested in both bindings. - Close the review's test-coverage gaps: Tab/Shift+Tab wrap and escaped- focus pull-back, no-focusables fallback, initialFocus, and the React custom portal target + portalDisabled, in both bindings where relevant. - Formalize the behaviour bundle's scope: new "Conscious constraints" section in docs/overlay-host.md freezes the guarantee list as the whole contract (no pixels ever, no chrome anatomy, no background inert, no focusable-detection long tail, no platform scroll workarounds, no router awareness) so the first DOM-owning surface in the family holds its line by documented decision rather than per-request renegotiation. - Fix doc drift: useSyncExternalStore references (the React glue is a deliberate subscription-driven re-render), the per-binding stack wording, and the dedup of the React dangling-id dev warning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G9FSij7zRf1Qt1AzHLvHP6
The overlay host was the slice-5 deliverable but had no runnable example —
the panels sibling has inspector-panels; overlays had nothing. Add the
pick-one, modal mirror of inspector-panels on BOTH bindings (the behaviour
is engine-first, so both must observe the same contract), each with a
Playwright suite asserting the full managed-modal contract.
New examples (examples/{react-router,vue}/overlay-result-windows):
- app-shared exports the shared defineOverlayHost<StepRef> handle + WindowMeta.
- run-core contributes test-report (dynamic title → aria-label; a nested
bespoke useModalBehavior confirm on the shared stack) and run-logs
(useOverlaySubject). acme-extras is a consumer window added with no host
edit (namespaced id). These are the first Vue example modules to contribute
`slots`.
- shell mounts one <OverlayOutlet>, styles the headless host, and drives
activeView from local/reactive state. An in-dialog switcher demonstrates
swap-without-close (the backdrop covers the openers behind it).
- e2e covers open/close, Escape + focus return, backdrop press-and-release
guard, focus trap, window swap, subject injection, consumer contribution,
the shared Escape stack, and the dangling-id "data not crash" stance.
Register both shells in the CI examples-e2e matrix; link them from
docs/overlay-host.md and examples/README.md.
fix(vue): make OverlayOutlet/PanelsOutlet subject + class props usable in
typed templates. `type: null` with an un-cast `default: null`/`default:
undefined` collapsed the inferred $props type to `null`/`undefined`, so the
documented `<OverlayOutlet :subject :panel-class>` / `<PanelsOutlet :subject>`
template usage did not typecheck under vue-tsc. Cast the defaults to the
prop's declared type so ExtractPropTypes keeps `unknown`. Runtime unchanged.
docs(overlays): fix the Vue #wrap snippet to render `children` through a
stable functional component instead of `<component :is="() => children" />`,
which gets a fresh identity every parent re-render (e.g. when a nested
overlay bumps the shared stack) and remounts the window, dropping its state.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MRyxejS9jdwipTZGZKGnZ3
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/react-router/overlay-result-windows/shell/package.json (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse concrete version ranges for published packages.
As per learnings, keep dependency specifier usage consistent in example apps: use
workspace:*only for app-internal workspace packages, and use concrete version ranges for published module packages.
examples/react-router/overlay-result-windows/shell/package.json#L17-L20: Replaceworkspace:*with a concrete version range (e.g.^2.0.0or equivalent) for@modular-reactand@react-router-modulesdependencies.examples/react-router/overlay-result-windows/modules/run-core/package.json#L19-L20: Replaceworkspace:*with a concrete version range for the@modular-react/reactand@react-router-modules/coredependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/react-router/overlay-result-windows/shell/package.json` around lines 17 - 20, Use concrete published version ranges instead of workspace:* for `@modular-react/core`, `@modular-react/react`, `@react-router-modules/core`, and `@react-router-modules/runtime` in examples/react-router/overlay-result-windows/shell/package.json lines 17-20, and for `@modular-react/react` and `@react-router-modules/core` in examples/react-router/overlay-result-windows/modules/run-core/package.json lines 19-20. Keep workspace:* only for app-internal workspace packages and choose versions consistent with the published package releases.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/frontend-core/src/overlay-dom.ts`:
- Around line 1-8: Remove the stale `collapseEntriesById` reference from the
module-level documentation in `overlay-dom.ts`, while preserving the description
of shared focus, Tab wrapping, body scroll locking, and `sharedOverlayStack`
semantics.
---
Nitpick comments:
In `@examples/react-router/overlay-result-windows/shell/package.json`:
- Around line 17-20: Use concrete published version ranges instead of
workspace:* for `@modular-react/core`, `@modular-react/react`,
`@react-router-modules/core`, and `@react-router-modules/runtime` in
examples/react-router/overlay-result-windows/shell/package.json lines 17-20, and
for `@modular-react/react` and `@react-router-modules/core` in
examples/react-router/overlay-result-windows/modules/run-core/package.json lines
19-20. Keep workspace:* only for app-internal workspace packages and choose
versions consistent with the published package releases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4cf4e31-8419-45e7-8252-dcac6c54a234
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (63)
.github/workflows/ci.ymlCHANGELOG.mddocs/overlay-host-triage.mddocs/overlay-host.mdexamples/README.mdexamples/react-router/overlay-result-windows/README.mdexamples/react-router/overlay-result-windows/app-shared/package.jsonexamples/react-router/overlay-result-windows/app-shared/src/app-types.tsexamples/react-router/overlay-result-windows/app-shared/src/index.tsexamples/react-router/overlay-result-windows/app-shared/src/overlay.tsexamples/react-router/overlay-result-windows/app-shared/tsconfig.jsonexamples/react-router/overlay-result-windows/modules/acme-extras/package.jsonexamples/react-router/overlay-result-windows/modules/acme-extras/src/index.tsxexamples/react-router/overlay-result-windows/modules/acme-extras/tsconfig.jsonexamples/react-router/overlay-result-windows/modules/run-core/package.jsonexamples/react-router/overlay-result-windows/modules/run-core/src/index.tsxexamples/react-router/overlay-result-windows/modules/run-core/tsconfig.jsonexamples/react-router/overlay-result-windows/shell/e2e/smoke.spec.tsexamples/react-router/overlay-result-windows/shell/index.htmlexamples/react-router/overlay-result-windows/shell/package.jsonexamples/react-router/overlay-result-windows/shell/playwright.config.tsexamples/react-router/overlay-result-windows/shell/src/components/Home.tsxexamples/react-router/overlay-result-windows/shell/src/components/Layout.tsxexamples/react-router/overlay-result-windows/shell/src/components/WindowChrome.tsxexamples/react-router/overlay-result-windows/shell/src/main.tsxexamples/react-router/overlay-result-windows/shell/tsconfig.jsonexamples/react-router/overlay-result-windows/shell/vite.config.tsexamples/vue/overlay-result-windows/README.mdexamples/vue/overlay-result-windows/app-shared/package.jsonexamples/vue/overlay-result-windows/app-shared/src/app-types.tsexamples/vue/overlay-result-windows/app-shared/src/index.tsexamples/vue/overlay-result-windows/app-shared/src/overlay.tsexamples/vue/overlay-result-windows/app-shared/tsconfig.jsonexamples/vue/overlay-result-windows/modules/acme-extras/package.jsonexamples/vue/overlay-result-windows/modules/acme-extras/src/SecurityReportWindow.vueexamples/vue/overlay-result-windows/modules/acme-extras/src/index.tsexamples/vue/overlay-result-windows/modules/acme-extras/tsconfig.jsonexamples/vue/overlay-result-windows/modules/run-core/package.jsonexamples/vue/overlay-result-windows/modules/run-core/src/RunLogsWindow.vueexamples/vue/overlay-result-windows/modules/run-core/src/TestReportWindow.vueexamples/vue/overlay-result-windows/modules/run-core/src/index.tsexamples/vue/overlay-result-windows/modules/run-core/tsconfig.jsonexamples/vue/overlay-result-windows/shell/e2e/smoke.spec.tsexamples/vue/overlay-result-windows/shell/index.htmlexamples/vue/overlay-result-windows/shell/package.jsonexamples/vue/overlay-result-windows/shell/playwright.config.tsexamples/vue/overlay-result-windows/shell/src/App.vueexamples/vue/overlay-result-windows/shell/src/components/Home.vueexamples/vue/overlay-result-windows/shell/src/components/ShellLayout.vueexamples/vue/overlay-result-windows/shell/src/components/WindowChrome.vueexamples/vue/overlay-result-windows/shell/src/main.tsexamples/vue/overlay-result-windows/shell/tsconfig.jsonexamples/vue/overlay-result-windows/shell/vite.config.tspackages/frontend-core/src/index.tspackages/frontend-core/src/overlay-dom.test.tspackages/frontend-core/src/overlay-dom.tspackages/frontend-core/src/overlay.tspackages/react/src/overlay.test.tsxpackages/react/src/overlay.tsxpackages/vue-core/src/index.tspackages/vue/src/overlay.test.tspackages/vue/src/overlay.tspackages/vue/src/panels.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/vue-core/src/index.ts
- packages/vue/src/panels.ts
- packages/vue/src/overlay.ts
- packages/react/src/overlay.tsx
- docs/overlay-host.md
- packages/frontend-core/src/overlay.ts
- docs/overlay-host-triage.md
Addresses the review comment: when a window ships no `title`, the dialog set only `aria-label` (from the resolved title), so a title-less window ended up with an unnamed dialog. Add an optional `ariaLabelledby` prop to `<OverlayOutlet>` on both bindings, forwarded to the dialog's `aria-labelledby`, so a heading the window renders in `wrap`/`#wrap` can name the modal. `aria-label` is preserved (ARIA resolves `aria-labelledby` first when both are present). Tests on both bindings assert the attribute forwards for a title-less window (aria-label absent). Docs updated (props lists + a11y guarantee). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MRyxejS9jdwipTZGZKGnZ3
…om docstring Addresses the review comment: the module docstring referenced a `collapseEntriesById` argument that no function in this file takes — a stale leftover from an earlier draft. Remove the parenthetical; the surrounding description of the shared focusable/Tab/scroll-lock/sharedOverlayStack semantics is unchanged. Comment-only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MRyxejS9jdwipTZGZKGnZ3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Accept the substance (a pick-one, app-state-keyed modal host with
framework-managed behaviour — focus trap + return, scroll lock, stacked
Escape, a11y — is a real, framework-shaped gap; even our own
nuxt-modal-journey example hand-rolls this chrome without it), redirect
the shape: no defineModule({ overlays }) descriptor seam (slots are the
ingress, per the slice-4 precedent), no /testing subpath, no Nuxt
manifest threading, no route-sync helper, and not Vue-only — the
behaviour is engine-first with React and Vue hosts in the same train.
Corrects the stale Gap D picture: the peer-range/hard-dep residuals the
request re-files were already closed by the shared-peer-dependency
change (#95). Counter-proposal: OverlayEntry (a ComponentEntry superset)
engine, managed OverlayOutlet hosts + useModalBehavior in both bindings.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_012ezPjgGYoWHHg1vVNLZUQT
Summary by CodeRabbit