feat(apollo-wind): align modal takeover and run actions - #1116
feat(apollo-wind): align modal takeover and run actions#1116dbacomputer wants to merge 10 commits into
Conversation
|
Apollo Coded App preview deployments are ready.
|
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
🟡 Changes recommended
There are API/test coverage gaps in the new DialogContent takeover API and an accessibility/behavior issue in the deprecated CanvasTakeoverModal wrapper’s focus/backdrop-close handling that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Modal/Dialog surface across Apollo Wind and Apollo React to match the Flow visual language, adds takeover-style modal behavior to the core Dialog implementation (with public Modal* aliases), and updates Storybook/pattern examples to use the new naming and “Run” labeling consistently.
Changes:
- Added takeover modal support to
apollo-wind’sDialogContent(header, expand/collapse, optional sidebar, overlay controls) and exportedModal*aliases. - Updated
apollo-reactcanvas stories/templates to use the Apollo Wind takeover modal instead of the legacyCanvasTakeoverModalpage, and aligned action labels to “Run”. - Expanded Storybook layout patterns with a UX inventory panel and Monaco-based code sample + theme switching.
File summaries
| File | Description |
|---|---|
| packages/apollo-wind/src/index.ts | Re-exports new Modal* aliases from the dialog module. |
| packages/apollo-wind/src/components/ui/dialog.tsx | Implements takeover variant + adds Modal* aliases alongside existing Dialog* exports. |
| packages/apollo-wind/src/components/ui/dialog.stories.tsx | Renames Storybook category to “Modal”, updates docs copy, adds takeover examples, and aligns sticky-footer styling. |
| packages/apollo-react/src/canvas/stories/templates/Flow.stories.tsx | Migrates takeover usages from CanvasTakeoverModal to Modal/ModalContent and updates “Run” labeling. |
| packages/apollo-react/src/canvas/components/NodePropertyPanel/NodePropertyPanel.stories.tsx | Removes debug button usage in favor of run button across examples. |
| packages/apollo-react/src/canvas/components/CanvasTakeoverModal/CanvasTakeoverModal.tsx | Keeps CanvasTakeoverModal as a deprecated compatibility wrapper backed by Apollo Wind ModalContent variant="takeover". |
| packages/apollo-react/src/canvas/components/CanvasTakeoverModal/CanvasTakeoverModal.stories.tsx | Removes the obsolete CanvasTakeoverModal Storybook page. |
| packages/apollo-react/src/canvas/components/CanvasModeToolbar/CanvasModeToolbar.stories.tsx | Updates toolbar labels from “Run debug/Stop debug” to “Run/Stop”. |
| packages/apollo-react/src/canvas/components/CanvasBottomPanel/CanvasBottomPanel.stories.tsx | Updates bottom panel action label to “Run”. |
| apps/storybook/src/patterns/LayoutPatterns.stories.tsx | Adds UX inventory patterns (including Monaco sample + theme-based styling) and updates “Run” labeling in patterns. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
There was a problem hiding this comment.
🔵 Needs a closer look
The updated Storybook layout pattern introduces invalid HTML (<li> outside a list) and a hard-coded list-boundary condition that should be corrected before approval.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
apps/storybook/src/patterns/LayoutPatterns.stories.tsx:1348
<li>elements must be children of a<ul>/<ol>/<menu>. Here they are rendered directly inside a<div>, which produces invalid HTML and can confuse assistive tech. Use a non-list element (e.g.<div>) for the draggable rows, or wrap the collection in a<ul>.
apps/storybook/src/patterns/LayoutPatterns.stories.tsx:1404- The "Move down" control is only disabled when
index === 2, which breaks as soon as items are removed (or if the list length changes). Disable based on the current list length so the last item can’t offer a "move down" action.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Follow-up cleanup completed in
The five prior review threads remain resolved. |
There was a problem hiding this comment.
🔵 Needs a closer look
There are a few concrete API/docs consistency issues in changed code (including a takeover DialogContent portal/container behavior inconsistency) that should be addressed before approval.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dialog.tsx:192
DialogContentcomputesresolvedContainerfrom thecontainerprop, but the takeover variant returnscontentdirectly and never uses the portal container. This makescontainera no-op forvariant="takeover", which is surprising for consumers expecting consistent portal behavior.
apps/storybook/src/patterns/LayoutPatterns.stories.tsx:1472- This example expression value uses "${vars.invoiceNumber}", but the rest of the file (and other examples) use the "$vars.*" syntax. Using a different syntax here makes the sample inconsistent and potentially misleading.
packages/apollo-react/src/canvas/components/CanvasModeToolbar/CanvasModeToolbar.stories.tsx:66 - The toolbar button label was updated to "Run", but the click handler still logs "run debug". This makes the story output inconsistent with the new labeling.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces broad, cross-package modal behavior/API updates plus large Storybook pattern additions, and should get final human review despite tests passing.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-react/src/canvas/components/CanvasTakeoverModal/CanvasTakeoverModal.tsx:63
data-expandedis passed toModalContent, butDialogContentignores any provideddata-expandedwhenvariant="takeover"(it always setsdata-expandedfrom its internalexpandedstate). This prop is therefore dead code here and can mislead readers into thinking it affects styling/behavior.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
There are verified accessibility and build-stability issues in changed files that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dialog.tsx:136
- In the takeover variant, the header always renders a
DialogPrimitive.Title, even whenheaderTitleisundefined/null. That creates an empty title node which Radix will still use foraria-labelledby, preventing consumers from supplying an accessible name viaaria-labeland resulting in an unnamed dialog for screen readers. Render the title only whenheaderTitleis provided (and keep layout alignment with a flex filler).
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
Storybook visual diffBaseline is the deployed main Storybook, so changes merged to main after this branch was last updated can also appear here. Logs Updated (PT): Sep 02, 2026, 10:28:10 PM |
There was a problem hiding this comment.
🟡 Changes recommended
The new takeover modal implementation has verified issues around onPointerDownOutside handler composition and aria-labelledby="" behavior that can break close behavior and accessible naming.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The takeover dialog portal/container handling and the empty-string aria-labelledby behavior introduce correctness/accessibility issues that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dialog.test.tsx:290
- If
DialogContentremovesaria-labelledbyfor takeovers that rely onaria-label, the test should assert the attribute is absent (rather than present with an empty string), while still validating the accessible name viagetByRole(..., { name }).
packages/apollo-wind/src/components/ui/dialog.tsx:136
- Setting
aria-labelledbyto an empty string produces an invalid ARIA attribute value (it must reference one or more element IDs). If the goal is to override Radix’s default labelling soaria-labelcan be used for takeovers without a header title, prefer explicitly passingundefinedto remove the attribute.
{...(isTakeover && (headerTitle === undefined || headerTitle === null)
? { 'aria-labelledby': ariaLabelledBy }
: ariaLabelledBy
? { 'aria-labelledby': ariaLabelledBy }
: {})}
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes appear consistent with the PR goals and include targeted tests, with only a minor Storybook sample-data nit noted in review comments.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The core behavior changes are covered by targeted tests and the remaining updates are Storybook/example alignment changes with no confirmed issues found in the reviewed diffs.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
This PR aligns Modal and takeover examples with the Flow visual language, consolidates takeover behavior on the Apollo Wind core Modal, and standardizes execution actions as
Run.Apollo Wind — additions and adjustments
Modaland explain that it is implemented with the shadcn/ui Dialog component and naming conventions, while Apollo’s internal name is Modal.Modal*aliases for the existing Dialog primitives.Apollo React — removals and adjustments
CanvasTakeoverModalStorybook page.ModalandModalContent variant="takeover"directly.CanvasTakeoverModalcomponent temporarily as a deprecated compatibility wrapper. This means existing consumers can keep importing the old component name while its implementation delegates to the new Apollo Wind Modal; it can be removed in a future breaking release after consumers migrate.Debug,Run debug, andRun testtoRun, including the Play icon where the action is presented as a button. Debug mode and debug-result panel terminology remain unchanged.Storybook and layout patterns
Runlabels and icon treatment.Verification
git diff --checkpasses.