chore(site): migrate all <Dialog />s off MUI - #27506
Conversation
The dialog de-MUI branch had accidentally downgraded vite from 8.0.16 to 8.0.10 (with the corresponding pnpm-lock churn) via an unrelated change. Restore site/package.json and site/pnpm-lock.yaml to match main.
…ponent
Replace hand-rolled inline { open; onOpenChange? } / { open; onClose } prop
types with composition of the underlying dialog's props:
- TaskFeedbackDialog composes ComponentProps<typeof Dialog>.
- UpdateBuildParametersDialog and MissingTemplateVariablesDialog compose
MUI DialogProps, matching the components they still render.
This restores the prior DialogProps-style composition and forwards the full
dialog prop surface instead of a fixed subset.
- Stop exporting ConfirmDialogType; it is only used within ConfirmDialog, and the dangling export failed knip (lint job). - Update the WorkspacePage orphan-on-delete test to select the orphan checkbox by test id. The migrated radix Checkbox sets the test id on the role=checkbox button itself, so the previous within(...).getByRole did not match (test-js job).
UpdateBuildParametersDialog and MissingTemplateVariablesDialog were the only
remaining users of MUI Dialog in site/src. Port them to the internal Dialog
primitives (DialogContent/Header/Title/Description/Footer + Button), keeping
the form submit semantics, the loading state, and the public
{ open, onClose, ... } API so call sites are unchanged. radix now wires
aria-labelledby/aria-describedby from the title/description, removing the
duplicated update-build-parameters-title id.
Revert the earlier 8.0.16 restore. vite 8.0.16 breaks the local dev site; 8.0.10 is the working version for this branch. The 8.0.16 bump is handled on a separate branch.
jakehwll
changed the title
<Dialog /> (globally)
jakehwll
changed the title
<Dialog />s off MUI
jakehwll
changed the title
<Dialog />s off MUI<Dialog />s off MUI
Restore site/package.json and site/pnpm-lock.yaml to match main. The vite version bump is handled on a separate branch and does not belong in the dialog migration.
jakehwll
changed the title
<Dialog />s off MUI
![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: 1086bb2211
ℹ️ 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".
…oxes Constrain the migrated parameter dialogs to the viewport and make their form regions scrollable so the footer stays reachable for long parameter and variable lists. Feed the schedule dialog checkboxes their checked state so the check icon reflects the selection, and add interaction coverage that exercises the prevention options, submit, and cancel flows.
| const warnBoxClassName = | ||
| "mt-6 flex gap-2 rounded-lg border border-solid border-border-warning bg-surface-orange p-3 leading-snug text-content-warning"; |
There was a problem hiding this comment.
🥲 If we're re-using this twice could we not just make a little component or something?
There was a problem hiding this comment.
Honestly, this needs some design attention 😅 We've got a backlog ticket for it, I think I might leave it for then...
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
jakehwll
changed the title
<Dialog />s off MUI
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, well-executed migration. It removes @mui/material/Dialog from all of site/src (verified: no @mui/material/Dialog imports and no components/Dialogs/ references remain), folds ConfirmDeleteDialog into ConfirmDialog type="delete" consistently across callers, and the deleted RTL tests for ConfirmDialog/DeleteDialog were re-expressed as story play functions that cover more than the originals. ScheduleDialog's checkboxes moved from uncontrolled MUI inputs to controlled radix Checkboxes wired to form values, closing a latent visual/state desync. Pariston tried to build a case against the premise and could not; Hisoka put it best: "I came to fight the migration and it fought back clean."
No P0-P2 findings. Severity: 3 P3, 1 P4, 4 Nits, 5 Notes.
The P3s cluster on two themes. Coverage: the two dialogs rewritten wholesale (UpdateBuildParametersDialog, MissingTemplateVariablesDialog) ship with no story or test, while every other rewrite in the PR gained play coverage. UpdateBuildParametersDialog carries formik + Yup + a submit path, so "manually dogfooded once" is the only proof its submit works, and site/AGENTS.md mandates story coverage as the test mechanism. The authoring agent will not return to add it, so this is a human's call, not a silent gap. Accessibility/convention: ScheduleDialog and AnnouncementBannerDialog render a DialogContent with a title but no DialogDescription and without the aria-describedby={undefined} opt-out that this PR's own ConfirmDialog uses, so radix emits its dev accessibility warning on open (the "no console error" bar the PR claims) and leaves a dead aria-describedby reference. And ConfirmDialog's rewrite dropped the doc that warned callers a type="delete" dialog with no onConfirm silently closes without deleting.
The Nits and Notes cover shared-primitive reuse (ConfirmDialog hand-rolls the DialogActions footer), a danger to warning (red to orange) color shift on WorkspaceDeleteDialog's warning boxes that reads as a design decision bundled into a mechanical PR, and duplicated scrollable-form scaffold worth watching if a third such dialog appears.
Process: the PR description is a model diagnosis (goal, per-phase reasoning, non-obvious decisions, and the surfaces that could bleed), and the WorkspacePage test change is a real root-cause fix (the radix Checkbox carries the testid on the role=checkbox element), verified passing. CI is green (16 passed, 14 skipped).
🤖 This review was automatically generated with Coder Agents.
…migration
- ScheduleDialog and AnnouncementBannerDialog set aria-describedby={undefined}
so radix does not point at a missing description or warn on open (CRF-4).
- ConfirmDialog documents that onConfirm falls back to onClose (a delete
dialog without it closes without deleting) and reuses the shared
DialogActions footer instead of a hand-rolled one (CRF-5, CRF-6, CRF-11).
- Add story play coverage for the wholesale-rewritten UpdateBuildParametersDialog
and MissingTemplateVariablesDialog, and Update/Cancel coverage for
AnnouncementBannerDialog (CRF-1, CRF-3, CRF-13).
- Restore the destructive (red) styling on WorkspaceDeleteDialog's task and
orphan warning boxes that the migration had softened to orange (CRF-7).
- MissingTemplateVariablesDialog uses Array.prototype.with instead of
mutate-then-copy, and the AnnouncementBannerDialog preview comment now
carries intent (CRF-14, CRF-10, CRF-12).
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 re-review. The fix commit (4ae7a3d) is a model correction pass: it maps each change to its finding, restores rather than rewrites, and stays proportional (128 insertions for 11 findings, no scope drift). Every previously-open finding that called for a code change is fixed at root cause, verified by the panel against the components and fixtures rather than the commit labels:
- CRF-1 / CRF-3 / CRF-13 (untested rewritten dialogs): UpdateBuildParametersDialog, MissingTemplateVariablesDialog, and AnnouncementBannerDialog now ship stories whose play functions type into the real inputs and assert the submit payloads (onUpdate/onSubmit) and cancel paths (onClose/onCancel). Bisky and Mafu-san confirmed these are real submit-path assertions, not render-only shells.
- CRF-4 (aria-describedby opt-out): closed as a class, not just the two flagged files. Mafu-san grepped every changed dialog; each now either renders a DialogDescription or passes aria-describedby={undefined}.
- CRF-5 / CRF-6 / CRF-11 (ConfirmDialog): the onConfirm-fallback doc is restored and names the consequence, the footer now reuses the shared DialogActions helper (safe because the dialog mounts no form and radix portals the content), and the hideCancel doc is one line.
- CRF-7 (color regression): the destructive (red) tokens are restored on both warning boxes and the orphan checkbox, matching the pre-migration render.
- CRF-14 (mutate-then-copy): now prev.with(index, ...). Hisoka and Meruem traced that the useEffect seeds the array to full length before any onChange can fire, so .with cannot throw.
Netero re-ran with tsc, knip, and biome all green, and none of the 13 panel reviewers raised a new finding. Komugi cleared the new play functions as deterministic (no timers, no unseeded entropy, single-checkbox queries unambiguous). Hisoka summed it up: "Boring, careful code. Silence is the rating it earned."
Two Notes remain acknowledged without a code change and without a tracking ticket: CRF-8 (DeleteDialog resetConfirmation fires only on a user-initiated close, harmless today because every reuse-across-entities caller keys the instance) and CRF-9 (the scrollable-form scaffold is duplicated across the two form dialogs, to be folded when a third appears). The panel re-verified both and found no new evidence to re-raise, and both match the reviewers' own not-worth-changing-now disposition. Neither is agent-acceptable as permanent: a maintainer should either file a tracking issue or explicitly accept them as won't-fix.
Note: the storybook vitest project could not be executed in the review worktree (init_emotion_react_browser_development_esm, a vitest/browser version mismatch in the harness, unrelated to this PR), so the story verdicts above are by inspection against the components and fixtures; PR CI reports the storybook project green.
🤖 This review was automatically generated with Coder Agents.
The prior fix changed the task and orphan warning boxes to destructive (red) tokens on the assumption that the pre-migration warnContainer was red. theme.roles.danger is actually defined entirely in orange tones (colors.orange[*] for background, outline, text, and fill.solid), so the migration's border-warning/surface-orange/content-warning mapping already matched the original render. Restore it.
Explain that radix's Dialog.Content spreads consumer props after its own
aria-describedby={descriptionId}, so passing aria-describedby={undefined}
when there is no DialogDescription drops the dangling reference and
silences radix's dev warning. Simplify the conditional spread to
{...(!description && { "aria-describedby": undefined })}.
| // Radix defaults aria-describedby to a descriptionId that dangles | ||
| // (and warns) without a DialogDescription, so opt out when absent. | ||
| {...(!description && { "aria-describedby": undefined })} |
There was a problem hiding this comment.
Im happy to chalk this up to being too defensive against Radix. We have no instances of a <ConfirmDialog /> currently that lacks a description. However, I'm for this with our future accessibility work in mind!
There was a problem hiding this comment.
this is so nasty it almost makes me wonder if description just shouldn't be optional
There was a problem hiding this comment.
fwiw I feel like quoting Radix's docs would be clearer than the current comment about dangling(???)
If you want to remove the description entirely, remove this part and pass
aria-describedby={undefined}toDialog.Content.
There was a problem hiding this comment.
Done in 6cba6a6. Made description required on ConfirmDialog and dropped the aria-describedby={undefined} guard entirely, so radix always wires to a real DialogDescription and the "nasty" opt-out is gone. tsc confirms every existing caller already passes a description, so no call sites changed. This also resolves @aqandrew's note, since the dangling-describedby comment no longer exists.
🤖 Replied by Coder Agents on behalf of Jake Howell.
| }} | ||
| > | ||
| <DialogContent | ||
| variant={type === "delete" ? "destructive" : "default"} |
There was a problem hiding this comment.
ok but the annoying thing here is that we also use "confirm" for delete dialogs all over the place. this component and the way we use it is a total mess.
There was a problem hiding this comment.
Agreed the confirmText/delete-button semantics across callers are messy. That's broader than this migration's scope (mechanical de-MUI), so leaving it for the dialog design cleanup rather than reshaping the API here. Flagging for that follow-up.
🤖 Replied by Coder Agents on behalf of Jake Howell.
There was a problem hiding this comment.
why are we not using <ErrorAlert> here? :despair:
...and then I realize this is in AgentsPage
There was a problem hiding this comment.
Correction to my last reply: this DeleteSkillDialog was migrated in this PR (ConfirmDeleteDialog → ConfirmDialog), so it is in scope. It keeps the Alert/AlertDescription pattern the surrounding AgentsPage code already uses rather than ErrorAlert, matching that page's local convention. Happy to switch it to ErrorAlert if you'd prefer consistency with the rest of the app, just say the word.
🤖 Replied by Coder Agents on behalf of Jake Howell.
Replace the within(canvasElement.ownerDocument.body) queries in the ConfirmDialog story play functions with storybook's screen, which already targets the portaled dialog in document.body, per review feedback.
…itive Bake max-h-[90vh] and overflow-y-auto into DialogContent so tall dialogs scroll within the viewport without each consumer hand-rolling a max height and a grid/overflow scaffold. Drop the one-off max-h-[90vh] grid-rows-[...] plus inner overflow wrappers from UpdateBuildParametersDialog and MissingTemplateVariablesDialog. Also clarify the ConfirmDialog onConfirm doc (onClose doubles as the confirm handler when omitted) and restore the shorter banner-preview comment.
# Conflicts: # site/src/modules/workspaces/WorkspaceMoreActions/UpdateBuildParametersDialog.tsx # site/src/modules/workspaces/WorkspaceUpdateDialogs.tsx
# Conflicts: # site/src/pages/DeploymentSettingsPage/OAuth2AppsSettingsPage/EditOAuth2AppPageView.tsx
4 participants
Removes Material UI from every dialog, moving them onto the internal shadcn/radix
Dialogprimitives. After this change there are no@mui/material/Dialogusages left insite/src.Changes
components/Dialogs/*→components/Dialog/*and folded the oldConfirmDeleteDialogintoConfirmDialog(type="delete").ConfirmDialog,DeleteDialog,WorkspaceDeleteDialog,ScheduleDialog, andAnnouncementBannerDialogonto the internal primitives (nativeInput/Label/Checkbox/Linkinstead of MUI).UpdateBuildParametersDialogandMissingTemplateVariablesDialog.ComponentProps<typeof Dialog>/ MUIDialogProps) instead of hand-rolled{ open; onOpenChange }shapes.Testing
AI-Driven manual dogfood sweep in a live instance (premium license), driven in a browser. Each dialog checked for logical rendering (layout, variant styling, buttons, no overlap/blank/console error) and function (open, primary action, cancel/close, guard states):
ConfirmDialog(delete / info / success)DeleteDialog(type-to-confirm)WorkspaceDeleteDialogScheduleDialogAnnouncementBannerDialogChangeWorkspaceVersionDialogDownloadLogsDialogTemplatePageHeaderdeleteFileDialog(create/rename/delete)MissingTemplateVariablesDialog(migrated)PublishTemplateVersionDialogUpdateBuildParametersDialog(migrated)WorkspaceUpdateDialogs)ResetPasswordDialogModelFormDialogs(form + delete)MCPServerFormDialogsdeleteHuman tested:
Things that have a chance to bleed:
Should be known that each of these renders through the already-verified
ConfirmDialog/DeleteDialog, so the underlying component is covered even where the specific trigger wasn't reachable.Plan & decision log
Goal: finish the de-MUI migration everywhere and confirm every affected modal renders logically and functions.
Phase 1 - complete the migration
Dialog(UpdateBuildParametersDialog,MissingTemplateVariablesDialog); ported both to the internal primitives, preserving the{ open, onClose, ... }public API (mapped toonOpenChangeinternally) so call sites were unchanged. radix now wiresaria-labelledby/aria-describedby, removing a duplicated element id.Phase 2 - sighting sweep (live browser, premium license)
MissingTemplateVariablesDialog,FileDialog,PublishTemplateVersionDialog, template delete - all PASS.UpdateBuildParametersDialog: reached by enabling classic parameter flow + pushing a version with a required parameter - PASS (renders + submits).Decisions
ConfirmDialog-wrapper prop types explicit (composingDialogPropsthere reintroduced a MUI smell).ConfirmDialogTypeexport (knip) and updated theWorkspacePageorphan-delete test: the radixCheckboxputs the test id on therole=checkboxbutton itself, so the previouswithin(...).getByRoleno longer matched.