feat(apollo-wind): align modal takeover and run actions - #1116
feat(apollo-wind): align modal takeover and run actions#1116dbacomputer wants to merge 1 commit 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 08, 2026, 12:56:57 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.
🔵 Needs a closer look
The takeover overlay positioning and a remaining inconsistent “Run node” label should be addressed to match intended behavior and stated standardization.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/apollo-wind/src/components/ui/dialog.tsx:131
- For
variant="takeover", the overlay is forced to!absolute(overriding the basefixedpositioning). When the dialog is portaled todocument.body(the default when no portal container provider/override is used), an absolutely-positioned overlay can scroll out of view on long pages, leaving the modal uncovered. Consider only switching toabsolutewhen a non-body portal container is actually used (i.e.,resolvedContaineris an element).
packages/apollo-react/src/canvas/stories/templates/Flow.stories.tsx:4391 - The PR description calls out standardizing execution actions to the label
Run, but this button still readsRun node, which makes the updated examples inconsistent.
- 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 changes core overlay/modal behavior and updates many UI examples across packages, which warrants a final human review for visual/interaction regressions beyond the included tests.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
107426f to
5820dc7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed drag-and-drop reorder bug in the UX inventory list logic (downward moves insert at the wrong index), and the dialog overlay/content structure has an API-level event bubbling semantic issue that should be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
packages/apollo-react/src/canvas/components/CanvasTakeoverModal/CanvasTakeoverModal.test.tsx:32
- The compatibility wrapper still exposes
closeOnBackdropClickand sets an overlay test id, but the updated close test no longer exercises backdrop dismissal. Adding this assertion helps ensure the wrapper continues to close on backdrop interactions by default.
it('requests close from Escape and the close button', () => {
const onOpenChange = vi.fn();
render(
<CanvasTakeoverModal open title="Modal" onOpenChange={onOpenChange}>
Content
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
e3c2267 to
cad5ac0
Compare
cad5ac0 to
fd5e9b9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One Storybook example initializes dates in the future, which can introduce time-dependent behavior and should be made stable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
One updated Flow takeover example still uses an execution label (“Run node”) that conflicts with the PR’s stated standardization to “Run”.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
fd5e9b9 to
e986c44
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The takeover modal behavior is consolidated with targeted tests added, and the remaining updates are consistent Storybook/template migrations without verified regressions.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
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: - Rename the Storybook component category to `Modal`, documenting that it is implemented with the shadcn/ui Dialog component while Apollo's internal name is Modal. - Add takeover support to the core Modal: sidebar and no-sidebar examples, centered layout, Flow semantic surfaces, theme-aware curtain overlay, expand/collapse controls, and consistent radius behavior. - Add public `Modal*` aliases for the existing Dialog primitives. - Update the Sticky Footer example so header, body, footer, borders, background surfaces, and corner radius match the Flow styling used by Modal. - Add focused takeover tests for title handling, sidebar rendering, expansion, and backdrop-dismiss behavior. - Guard overlayProps click/mouse/pointer handlers so they only fire on backdrop hits, since DialogContent nests inside DialogOverlay for centering and would otherwise bubble interior interactions up to those handlers. Apollo React: - Remove the obsolete `CanvasTakeoverModal` Storybook page. - Migrate Flow Standalone takeover examples to use Apollo Wind `Modal` and `ModalContent variant="takeover"` directly. - Retain the exported `CanvasTakeoverModal` component temporarily as a deprecated compatibility wrapper delegating to the new Apollo Wind Modal, so existing consumers can keep importing the old name until a future breaking release. - Update execution action labels from `Debug`, `Run debug`, and `Run test` to `Run`, including the Play icon where the action is a button. Debug mode and debug-result panel terminology are unchanged. Storybook and layout patterns: - Update Node Property Panel, Canvas toolbar, bottom-panel action, layout-pattern, and Flow examples to use consistent `Run` labels and icon treatment. - Fix the UX inventory draggable list markup and make its Move down boundary depend on the current list length; fix the downward drag-reorder offset so items land in the intended slot.
e986c44 to
d958e61
Compare
|
Fixed in d958e61: the Component Inventory date/date-time picker defaults no longer hardcode a fixed calendar date. They're now computed relative to the current date (7 days out) so the example stays stable and doesn't drift into stale-looking or time-dependent behavior. |
There was a problem hiding this comment.
🟢 Approval recommended
The core takeover-modal behavior is covered by new focused tests and the remaining changes are largely story/example alignment without any confirmed functional regressions.
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.