Skip to content

refactor(ui): shared ConfirmDialog, @base-ui/react, remove Tailwind and unused deps - #57

Merged
datj9 merged 3 commits into
mainfrom
refactor/ui-primitives-and-deps
Sep 24, 2026
Merged

datj9 merged 3 commits into
mainfrom
refactor/ui-primitives-and-deps

Conversation

@datj9

@datj9 datj9 commented Sep 22, 2026

Copy link
Copy Markdown
Owner

Plan item D1a. A refactor meant to change no behaviour. D1b (feat/owner-controls-ux) builds on this branch.

Dependencies removed

Package Why
@base-ui-components/react@1.0.0-rc.0 replaced by the already-listed @base-ui/react@^1.7.0
cmdk, sonner, next-themes, class-variance-authority, clsx 0 imports (checked again with grep after the change)
tailwindcss, @tailwindcss/postcss, postcss.config.mjs no utility class is used anywhere (see below)

base-ui migration (1.0.0-rc.0 → @base-ui/react@1.7.0)

I compared the .d.ts files of both packages in node_modules for every part we use (Dialog Root/Trigger/Portal/Backdrop/Popup/Title/Description/Close, Menu Root/Trigger/Portal/Positioner/Popup/Item):

  • Part names, initialFocus / finalFocus (RefObject form), closeOnClick, render, side / align / sideOffset: unchanged.
  • Data attributes the CSS depends on (data-starting-style, data-ending-style, data-open, data-highlighted): unchanged.
  • className now accepts string | undefined, so the two copy-pasted css() shims and src/lib/ui css() are gone.
  • Changes in upstream defaults to be aware of: (1) Popup with no initialFocus now focuses the popup itself when opened by touch; mouse and keyboard still focus the first tabbable element. This affects only the artifact delete dialog and the admin delete-account dialog, and is arguably safer. (2) Menu closeParentOnEsc default changed. We have no submenus, so this does nothing here.

Tailwind removal: how I checked it

Tailwind generated no utilities we used. It emitted a few (.hidden, .table, .sr-only, …) only because those words appear in comments and prose, and unlayered app CSS or CSS-module hashing shadowed all of them. It did supply preflight, and every screen is laid out on top of it (margins, list bullets, heading sizes, button chrome, a { text-decoration: inherit }). So:

  • styles/reset.css is tailwindcss@4.3.3/preflight.css copied in (MIT, attributed). It is wrapped in @layer base exactly as Tailwind emitted it, and each --theme() call is replaced by the value it resolved to.
  • I built main and this branch and diffed the compiled CSS. The @layer base output matches rule for rule. The only differences are the html font stack written as a literal (same value) and the ::placeholder currentColor fallback for browsers without color-mix, which Tailwind's own lightningcss pass used to add. The :root token output is identical.
  • Removed the @theme inline block. It was a self-referential mapping that only utilities read.
  • .sr-only gains padding:0; border-width:0, which Tailwind's .sr-only utility used to merge in underneath.
  • tests/unit/token-contrast.test.ts passes.

New primitives

app/_components/ui/confirm-dialog.tsx: ConfirmDialog. Every prop has a doc comment in the file.

title: ReactNode; body: ReactNode; confirmLabel: ReactNode
cancelLabel?: ReactNode            // default 'Cancel'
tone?: 'default' | 'danger'        // exposed as data-tone; both tones look the same (design.md has one accent)
busy?: boolean                     // aria-disabled on confirm; onConfirm is not re-fired while busy
error?: string | null              // renders <p class="form-error" role="alert"> inside the popup
onConfirm: () => void              // does NOT close the dialog; the caller decides
open?: boolean; onOpenChange?: (open: boolean) => void   // controlled, or uncontrolled with a trigger
trigger?: { label: ReactNode; className?: string; testId?: string }
initialFocus?: 'confirm' | 'cancel'   // omitted = base-ui default
finalFocus?: RefObject<HTMLElement | null>
testId?: string; confirmTestId?: string

app/_components/ui/dialog.module.css: one modal shell (backdrop, popup, popupWide, title, description, actions, confirm, cancel). It replaces publish-dialog.module.css, most of delete-dialog.module.css (only the trigger is left), and the duplicated frame in share-dialog.module.css.
src/lib/ui/class-name.ts: a single cx(...parts) helper. It replaces clsx and the css() shims, and has unit tests.

Call sites: both privacy-switch confirmations are now <ConfirmDialog> (the two copy-pasted components are removed), and so are the artifact delete dialog and the admin delete-account dialog. user-table.tsx no longer imports app/a/[id]/delete-dialog.module.css. All test ids, copy, focus targets and open/close semantics are kept. user-table still closes on failure and still has no catch, because that fix belongs to D1b.

One small visual change

The publish-public and set-private dialogs used their own button sizing: a 24px padded confirm and a 24px padded cancel with a negative margin. They now use the shared dialog buttons, the same ones the delete dialogs already had: confirm on --control-sm (32px, promoted to 44px on pointer: coarse) and cancel as a text button with a --control-md target. This lines them up with the control scale in design.md and tokens.css.

Testing

  • pnpm install --frozen-lockfile, pnpm typecheck, pnpm lint: pass
  • pnpm vitest run --project unit: 73 files, 1182 tests pass (includes new tests/unit/class-name.test.ts)
  • pnpm build (with .env.example values): passes; compiled CSS diffed against main as described above
  • grep for every removed package across app/ src/ styles/ tests/ packages/ and the config files finds nothing

Risks

  • I did not run e2e locally (no Docker). The specs that touch this code are zz-design-system, visibility-*, share-*, trash-delete-restore and two-account-privacy. They depend only on test ids and visible copy, and I kept both. zz-design-system measures only controls that are rendered, so closed dialogs are skipped.
  • Files I don't own that still mention removed things: design.md:109 (Tailwind v4 exports note) and docs/motion.md:113 ("Toasts: Sonner defaults"). I left them unchanged. Worth a docs follow-up.
  • Merge order: plan item C also edits package.json (scripts only). Any conflict should be trivial.

🤖 Generated with Claude Code

datj9 and others added 2 commits September 23, 2026 06:49
…deps

- Move the five base-ui import sites from @base-ui-components/react@1.0.0-rc.0
  to @base-ui/react@1.7 (same part names, props and data attributes; className
  now accepts undefined, so the css() shims go).
- Add app/_components/ui/confirm-dialog.tsx + dialog.module.css: one modal
  shell (backdrop, popup, popupWide, title, description, actions, confirm,
  cancel) replacing three near-identical dialog stylesheets.
- privacy-switch's two confirm dialogs, the artifact delete dialog and the
  admin delete-account dialog all render through ConfirmDialog; share-dialog
  reuses the shell. user-table no longer imports CSS from app/a/[id].
- src/lib/ui/class-name.ts exports a single cx() helper.
- Remove unused deps: @base-ui-components/react, cmdk, sonner, next-themes,
  class-variance-authority, clsx.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
No component used a Tailwind utility class (the generated utilities were all
matches from prose/comments, shadowed by unlayered app CSS), but every screen
was laid out on top of Tailwind's preflight reset. Removing the import alone
would have changed rendering (list bullets, heading sizes, button chrome,
link underlines, margins).

- styles/reset.css is preflight.css from tailwindcss@4.3.3 (MIT), wrapped in
  @layer base exactly as Tailwind emitted it, with --theme() calls replaced by
  the values they resolved to. The compiled base layer matches the previous
  build rule-for-rule.
- Drop the @theme inline block (a self-referential mapping only utilities
  consumed) and postcss.config.mjs; Next's built-in CSS pipeline takes over.
- .sr-only gains padding:0 / border-width:0, which Tailwind's own .sr-only
  utility used to merge in underneath.
- Remove tailwindcss and @tailwindcss/postcss.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@datj9 datj9 left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review (Claude Code)

Verdict: Looks good to merge, once e2e has actually run green. No code defects found.

Findings: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 2 LOW

Checks run locally on d486647 (branch is based on current main):

  • pnpm install --frozen-lockfile: pass (lockfile consistent after the 848-line removal)
  • pnpm typecheck: pass
  • pnpm lint: pass
  • pnpm vitest run --project unit: 73 files, 1182 tests pass
  • pnpm build: pass. Compiled CSS still contains the @layer base block from styles/reset.css and no cascade-layer polyfill (:not(#\#)) output, so removing postcss.config.mjs and falling back to Next's default PostCSS config did not flatten the layer. Unlayered globals and CSS modules still win over the reset.
  • grep for @base-ui-components, clsx, cmdk, sonner, next-themes, class-variance-authority, tailwind, and the removed css() helper across app/ src/ styles/ tests/ packages/ and config files: no remaining imports (only attribution comments in reset.css)
  • No dangling references to --dur-share / --dur-delete / --dur-publish or to classes deleted from share-dialog.module.css / delete-dialog.module.css. publish-dialog.module.css has no importers left.
  • No e2e selector depends on a removed CSS-module class; specs use test ids, and all test ids are kept.

What I checked and found no problem with:

  • ConfirmDialog call sites keep test ids, copy, initialFocus/finalFocus, and open/close semantics. Both privacy dialogs still close before calling choose(). Delete still refreshes the live-link count on open. The admin delete still has no busy/close change (deferred to D1b, as the PR says).
  • The dialog .confirm reproduces the .button-sm sizing and pointer: coarse promotion it used to get from the global class. The global :focus-visible outline still applies.
  • cx() is correct and tested for undefined/false/null/empty parts.

LOW

  1. e2e has not run anywhere for this PR. Both check runs failed during setup with pull access denied for minio/minio, before any test step ran. This is an infra problem, not caused by this PR. But combined with "did not run e2e locally", the dialog refactor (visibility-*, trash-delete-restore, two-account-privacy, zz-design-system) is still unverified in a browser. Fix the MinIO image reference in the workflow, or re-run once it pulls, before merging.
  2. ConfirmDialog has no component-level test for its own contract: onConfirm is not re-fired while busy, the error slot has role="alert", and initialFocus maps to the right button. The unit project runs in environment: 'node', so today only e2e covers this. This is acceptable for a no-behaviour-change refactor. Worth adding once there is a DOM test project, because D1b will start relying on these guarantees.

Docs follow-up (already noted in the PR, not in this diff): design.md:109 still mentions Tailwind v4 exports, and docs/motion.md:113 still says "Toasts: Sonner defaults".

…t.css

While busy, Esc, a backdrop press or the cancel button could close the
dialog mid-request: a failure then rendered into a closed dialog, and a
success landed after the person had cancelled. Close requests are now
canceled via base-ui's eventDetails.cancel() (keeps uncontrolled dialogs
open) and not forwarded (keeps controlled ones open); Cancel goes
aria-disabled. The public props are unchanged.

reset.css vendors Tailwind's preflight; MIT requires the permission
notice, not only the copyright line, so it is now included in full.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@datj9

datj9 commented Sep 23, 2026

Copy link
Copy Markdown
Owner Author

Review of D1a (base-ui migration, ConfirmDialog, Tailwind removal, dep removal).

Fixed in 463748b

Checked, no defect

  • reset.css matches upstream v4.3.3 preflight.css line for line, apart from the documented --theme() substitutions.
  • Built CSS, main vs this branch: @layer base is equivalent. The dropped @layer theme vars do not matter: --ease-out is used in marketing/dashboard, but tokens.css defines it unlayered, so it already won. Only .sr-only among the dropped utilities is used, and it is covered. No other @layer exists in app CSS, so dropping the layer-order statement changes nothing.
  • The dialog chunk differs only by the shared module, plus the documented publish/private button sizing.
  • base-ui 1.7: undefined initialFocus/finalFocus fall back to the defaults. Menu.Item render={<a>} leaves Enter to native link activation. Title/Description still wire aria-labelledby/-describedby.
  • Removed deps: nothing references them in app/src/tests/scripts/packages or the eslint/prettier configs.

Left as is (pre-existing, behaviour-preserving)

  • A stale error stays on reopen in the artifact delete dialog, same as main. ConfirmDialog leaves it to callers; D1b callers should clear it on open.
  • The ::placeholder currentColor fallback for browsers without color-mix is gone (documented in the PR body; affects pre-2023 browsers only).
  • Confirmations use Dialog rather than AlertDialog, same as before.

Gates: typecheck, lint, unit (74 files / 1186 tests) and pnpm build all pass.

@datj9
datj9 merged commit 97f7a7a into main Sep 24, 2026
1 of 3 checks passed
datj9 added a commit that referenced this pull request Sep 24, 2026
feat: owner-controls UX — confirmations, shared share state, manual radiogroup (D1b, stacked on #57)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant