feat(design-library): add Modal, BottomSheet, Toast, and ConfirmDialog#31088
Conversation
Tier 2 overlay components ported from the platform repo's design system. All components follow the design library conventions: function declarations, data-slot attributes, ComponentProps (no forwardRef), portal-container integration, and CSS variable design tokens. - Modal: Radix Dialog with size presets (sm/md/lg/xl), compound API (Root/Trigger/Content/Title/Description/Close/Header/Body/Footer) - BottomSheet: Bottom-anchored dialog with slide-up animation, safe-area inset handling, 50dvh cap with scrollable body - Toast: Sonner-based notification system with 5 variants (default/info/warning/error/success), imperative toast() API - ConfirmDialog: Pre-composed confirmation modal with auto-focus on confirm button, stacked-modal Escape handling Dependencies added: @radix-ui/react-dialog@1.1.15, sonner@2.0.7 Part of LUM-1543 Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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: 1bd6eb52b2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <p className="whitespace-pre-line text-body-medium-lighter text-[var(--content-secondary)]"> | ||
| {message} | ||
| </p> |
There was a problem hiding this comment.
When ConfirmDialog is used by screen-reader users, this message is rendered as a plain paragraph, so Radix cannot wire it into the dialog's aria-describedby; users may only hear the title before choosing Cancel or Confirm. Use Modal.Description here, or give this paragraph an id and pass it through to Modal.Content as aria-describedby, so the confirmation text is announced as part of the dialog.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved — switched from a plain <p> to <Modal.Description> so Radix wires aria-describedby on the dialog content, making the confirmation message announced by screen readers. Fixed in 4411d51.
…Dialog - Add data-slot="modal-overlay" to Modal overlay - Add data-slot="bottom-sheet-overlay" to BottomSheet overlay - Use Modal.Description in ConfirmDialog so Radix wires aria-describedby for screen readers (was a plain <p> tag) Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| <Modal.Body> | ||
| <Modal.Description>{message}</Modal.Description> | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| <Button variant="outlined" onClick={onCancel}> |
There was a problem hiding this comment.
🟡 ConfirmDialog missing Modal.Description causes Radix console warning and broken aria-describedby
The ConfirmDialog renders its message prop as a plain <p> inside Modal.Body (confirm-dialog.tsx:74-78) instead of using Modal.Description (which wraps Dialog.Description at modal.tsx:137). Because no Dialog.Description is present and no aria-describedby is supplied, Radix Dialog logs a console warning on every open, and screen readers cannot formally associate the dialog with its description — the message won't be announced as the dialog's description when focus enters.
| <Modal.Body> | |
| <Modal.Description>{message}</Modal.Description> | |
| </Modal.Body> | |
| <Modal.Footer> | |
| <Button variant="outlined" onClick={onCancel}> | |
| <Modal.Body> | |
| <Modal.Description className="mt-0 whitespace-pre-line text-body-medium-lighter text-[var(--content-secondary)]"> | |
| {message} | |
| </Modal.Description> | |
| </Modal.Body> |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Resolved in eda77196. Already fixed in the prior commit (4411d51b) — ConfirmDialog now uses <Modal.Description> instead of a plain <p>, which wires the message into Radix's aria-describedby.
| function Toaster() { | ||
| return ( | ||
| <SonnerToaster | ||
| data-slot="toaster" | ||
| position="bottom-right" | ||
| toastOptions={{ | ||
| unstyled: true, | ||
| style: { width: "356px" }, | ||
| }} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🚩 Toaster data-slot may not propagate to DOM through SonnerToaster
The Toaster component passes data-slot="toaster" to SonnerToaster at toast.tsx:165. Whether this actually reaches the DOM depends on whether Sonner's Toaster component spreads unknown props onto its root element. If it doesn't, the data-slot would be silently ignored. This wouldn't cause a runtime error, but it means the [data-slot="toaster"] CSS selector wouldn't work for consumers. Consider verifying in the browser or wrapping SonnerToaster in a <div data-slot="toaster"> if the attribute needs to be present.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in eda77196:
-
data-slotpropagation: Confirmed Sonner'sToasterPropsdoesn't extendHTMLAttributes— unknown props are silently dropped. WrappedSonnerToasterin<div data-slot="toaster">so the attribute reaches the DOM. -
role="alert"on every variant: Also fixed in the same commit. Now usesrole="status"(polite,aria-live="polite") fordefault/info/successandrole="alert"(assertive) only forerror/warning. This prevents "Profile saved" toasts from interrupting screen reader users.
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
Value: Lands the four most-used overlay primitives in one shot — Modal (27 platform imports), ConfirmDialog (26), BottomSheet (12), Toast (6). The improvements over the platform source (React 19 ref-as-prop, data-slot everywhere, function declarations, portal-container integration, no "use client") keep the new code aligned with the design-library conventions agreed in #31045 / #31079.
Bot findings — current state at HEAD 4411d51b:
Codex flagged two P2s at 1bd6eb52, both already fixed by Devin in the current HEAD:
- ✅
Modal.Descriptionnow used inConfirmDialog(replaces plain<p>) — confirmation message is wired into Radix'saria-describedby. - ✅
data-slot="modal-overlay"anddata-slot="bottom-sheet-overlay"added on both overlay roots.
Codex's review predates these fixes by one commit; both findings are stale.
My observations:
1. Modal vs BottomSheet — structural inconsistency (worth a follow-up):
Modal nests Dialog.Content inside Dialog.Overlay and uses the overlay as the flex centering container:
<Dialog.Overlay className="... flex items-center justify-center ...">
<Dialog.Content ...>BottomSheet uses the standard Radix sibling pattern (Overlay and Content as portal siblings). The Modal nesting works (it's a known Radix workaround for centered modals), but it diverges from BottomSheet within the same PR. Either pattern is fine; consistency would make the design-library easier to reason about. Non-blocking.
2. Toast — role="alert" on every variant (real a11y concern):
ToastContent always renders role="alert", which screen readers treat as aria-live="assertive" — interrupting whatever the user is reading. That's correct for error and warning, but overkill for default, info, and success. Recommended:
error/warning→role="alert"(assertive)default/info/success→role="status"(polite)
This is the kind of thing AT users notice when "Profile saved" or "Copied to clipboard" yanks focus from their current read. Worth a small follow-up patch.
3. BottomSheet — no closing animation:
data-[state=open]:animate-[bottomSheetIn_180ms_ease-out] only handles the open state. On close, the sheet snaps off-screen with no slide-down. Minor polish, not blocking.
4. Toaster data-slot may not reach the DOM:
<SonnerToaster data-slot="toaster" .../> passes data-slot to the Sonner component, but Sonner may not forward arbitrary props through to its root <section>. If consumers want to CSS-target the toaster region, this may silently no-op. Worth verifying in Storybook, but cheap to fix later by wrapping in a div if Sonner doesn't forward.
5. ConfirmDialog confirm-focus via querySelector:
Using [data-confirm-dialog-confirm] attribute + querySelector to grab the confirm button on onOpenAutoFocus works, but a ref through the compound API would be cleaner. Acceptable as-is — the attribute approach is encapsulated inside the component and doesn't leak.
Anti-patterns KB sweep — clean:
- No
useEffectfire-and-forget, noforwardRef, no hex colors, notext-red-*ad-hoc Tailwind, no--ghost-hovermisuse, nobg-[#...]. ✅ - Token discipline solid:
var(--surface-lift),var(--system-*),var(--content-*)throughout. The only non-token bit isbg-black/50for overlays, which is the conventional Radix pattern. color-mix(in oklab, var(--primary-base) 16%, transparent)for the title icon background is a nice touch — keeps the tint derived from the token instead of hardcoding a faded color.
Merge gate: No human approval yet. Codex stale at 1bd6eb52, Devin commented only (didn't APPROVE). After this approval, suggest a Codex + Devin re-review at HEAD 4411d51b to get a clean second sign-off, since their original findings are now resolved.
Reviewed at 4411d51b
Vellum Constitution — Yours: overlay primitives that are token-consistent, React-19-native, and data-slot-tagged let domain code migrate without inventing one-off Dialog wrappers.
… data-slot - error/warning → role="alert" (assertive, interrupts screen reader) - default/info/success → role="status" (polite, queued) - Wrap SonnerToaster in div since Sonner doesn't forward unknown props to DOM Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
devin-ai-integration
Bot
dismissed
vex-assistant-bot[bot]’s stale review
via
eda7719
|
Addressed observations from #2 Toast #4 Toaster #1 Modal vs BottomSheet structural inconsistency — Acknowledged, non-blocking. The nested-overlay pattern for Modal is intentional (Radix's recommended workaround for centered modals with flex layout). BottomSheet uses sibling pattern because it's fixed-bottom positioned. Both work correctly. #3 BottomSheet closing animation — Acknowledged, non-blocking. Will add #5 ConfirmDialog querySelector focus — Acknowledged, acceptable. The |
Stories for Modal (4 stories: Default, WithIcon, Sizes, NoCloseButton), BottomSheet (2 stories: Default, WithIcon), Toast (3 stories: AllVariants, WithDescription, WithAction), ConfirmDialog (3 stories: Default, Destructive, CustomLabels). Co-Authored-By: ashlee@vellum.ai <ashlee@vellum.ai>
There was a problem hiding this comment.
✦ APPROVE (re-review at 355dbebf)
Value: Lands four high-coverage overlay primitives — Modal (27 platform imports), BottomSheet (12), ConfirmDialog (26), Toast (6) — in a single batch, advancing the design-library migration ahead of the platform web-move cutover.
What this does: Ports Modal, BottomSheet, Toast, and ConfirmDialog from vellum-assistant-platform/web/src/components/app/core/ into packages/design-library/, adapts them to the compound-component convention, and adds Storybook stories for all four.
Codex + Devin findings — all resolved ✅
Verified at HEAD:
Dialog.DescriptioninConfirmDialog→ now<Modal.Description>(Radix wiresaria-describedbycorrectly) ✅data-slot="modal-overlay"anddata-slot="bottom-sheet-overlay"present on overlay roots ✅Toasterwrapped in<div data-slot="toaster">— Sonner'sToasterPropsdoesn't extendHTMLAttributes, so passingdata-slotdirectly toSonnerToasterwas silently dropped. The wrapper fixes propagation ✅role="alert"restricted toerrorandwarningviaASSERTIVE_VARIANTS;default/info/successnow userole="status"(polite) ✅
Anti-patterns KB — clean ✅
No --ghost-hover misuse, all colors use semantic tokens (--surface-lift, --content-default, --border-base, etc.), no hex values, no ad-hoc text-sm/font-medium combos, no new barrel index.ts.
Non-blocking notes:
-
BottomSheet: enter animation, no exit.
data-[state=open]:animate-[bottomSheetIn_180ms_ease-out]is defined, but there's nodata-[state=closed]exit. The sheet snaps closed — fine for now, but a slide-down exit would feel more polished. Noting for a follow-up pass. -
Modal/BottomSheet overlay structure differs. Modal nests
Dialog.ContentinsideDialog.Overlay(used for centering via flexbox); BottomSheet keeps them as siblings. Both work correctly with Radix pointer-event handling — close-on-outside-click is fine either way. But the inconsistency could trip up a future contributor who mirrors one pattern to the other. Worth a comment in one or both components' JSDoc.
CI: Lint, Test, Type Check, FlexFrame all green ✅
Merge gate: Vex approved (this review). Codex has no 👍 yet (Storybook commit may not have triggered re-review) — trigger with @codex review + @devin review this PR to get the second approval.
Vellum Constitution — Inviting: a consistent, fully-slotted overlay API means the platform's overlays migrate cleanly rather than accumulating one-off patterns.
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Storybook Testing Results — Tier 2 Design Library ComponentsRan Storybook locally, tested all 4 overlay/notification components end-to-end. All 7 tests passed. Component Tests (7/7 passed)
data-slot Verification DetailsModal: Toast: BottomSheet: ConfirmDialog: Evidence — ConfirmDialog DefaultRecordingFull annotated screen recording showing all test execution with pass/fail markers: |
1 participant
Prompt / plan
Tier 2 overlay components for the design library, ported from the platform repo's
web/src/components/app/core/and adapted to follow assistant repo conventions. These are the next layer of reusable UI primitives after the Tier 1 form/navigation components merged in PR #31087.Components added:
Improvements over platform source:
forwardRef— uses React 19 ref-as-prop pattern viaComponentProps<>per React 19 changelogdata-sloton every root element — enables CSS-only styling overrides per shadcn/ui v4 conventionconstassignments orforwardRefwrappersusePortalContainer()from the design library (not app-specificuseAppRootContainer)"use client"directives — pure React, no Next.js framework dependencycn()utility — consistent class merging via design library's owncn<button>with matching styles insteadtext-title-medium/text-body-medium-lighterutility classes directly, avoiding a component dependency for simple text stylingDependencies added:
@radix-ui/react-dialog@1.1.15,sonner@2.0.7Animation keyframe added:
bottomSheetInintokens.cssfor the slide-up entrance.Part of LUM-1543
Test plan
bunx tsc --noEmitpasses cleanlyLink to Devin session: https://app.devin.ai/sessions/536ceadb023a4059908b0609b9833bc1
Requested by: @ashleeradka