From 6bc7fdf43574f09bbd53ecc478bcc63f36a156c8 Mon Sep 17 00:00:00 2001 From: Jeremy Vandenberg Date: Fri, 22 May 2026 19:59:20 -0400 Subject: [PATCH] docs: recommend dialogApi + use*Dialog hook as the default pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit was flagging the dialogApi-based dialogs (the project's actual convention — 38 of them, 0 inline-useState dialogs in the codebase) as violating an outdated guideline that said the opposite. @CptnFizzbin confirmed on https://github.com/CptnFizzbin/shadow-sin/pull/274#discussion_r... that the guidance is out of date and asked for it to be flipped. The premise of the old guideline — that dialogApi "renders outside the provider tree" — is no longer true: DialogApiProvider is mounted inside CharacterSheetProvider in the character routes and inside the builder root, so React context propagates normally through dialogs rendered by dialogApi.open. Updates: - AGENTS.md "Dialog patterns" first bullet: now recommends the useDialogApi + use*Dialog hook pattern as the default and points at useAddKarmaDialog / useActiveSkillDialog as examples. The other two bullets (useConfirmDialog, compound Dialog) are unchanged. - docs/ui/dialog.md - "Inline useState dialog (no DialogApi)" section intro: reframed from "prefer this" to "use only when no DialogApi available" — inline state is now positioned as a rare alternative, not the default. The code example below stays as-is. - "Guidelines" first bullet: flipped to recommend the useDialogApi + use*Dialog hook pattern. The other three bullets (onClosed vs onClose, maxWidth, no styling props) are unchanged. No code changes. CodeRabbit should stop flagging dialogApi-based dialogs as a convention violation on subsequent reviews. --- AGENTS.md | 7 +++++-- docs/ui/dialog.md | 15 ++++++++++----- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fad00537..bba464ab 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -240,8 +240,11 @@ commit ## Dialog patterns -- Prefer inline `useState`-managed dialogs (open/closed state in the parent component) over `dialogApi` for dialogs that - need access to `CharacterSheetProvider` or other React context, since `dialogApi` renders outside the provider tree. +- Prefer the `dialogApi` + `use*Dialog` hook pattern for new dialogs: the hook calls `useDialogApi()` and returns + `{ open: (props) => dialogApi.open((ctrl) => ) }`. `DialogApiProvider` is + mounted inside `CharacterSheetProvider` (in the character routes and the builder root), so React context including + `CharacterSheetProvider` propagates normally — no inline state is needed for context access. See + `useAddKarmaDialog` and `useActiveSkillDialog` for minimal examples. - Use `useConfirmDialog()` from `#/components/dialogs/confirmDialog.tsx` for confirmation prompts before destructive actions. - Use the compound `Dialog` component from `#/components/ui/dialog/dialog.tsx` for all new dialogs — it enforces diff --git a/docs/ui/dialog.md b/docs/ui/dialog.md index 65648019..5b844272 100644 --- a/docs/ui/dialog.md +++ b/docs/ui/dialog.md @@ -149,8 +149,11 @@ export const AddNoteDialog: FC = ({ open, onClose, onClose ### Inline `useState` dialog (no `DialogApi`) -Prefer this pattern when the dialog needs access to `CharacterSheetProvider` -or other React context (since `DialogApi` renders outside the provider tree). +Use this pattern only when the consumer truly has no `DialogApi` available — +uncommon in production code. `DialogApiProvider` is mounted inside +`CharacterSheetProvider` in the app routes and the builder root, so the +`useDialogApi`-based hook pattern shown above already has access to React +context and is the project default. ```tsx import Button from "@mui/material/Button" @@ -184,9 +187,11 @@ export const ExamplePage = () => { ## Guidelines -- **Prefer inline `useState` dialogs** over `DialogApi` when the dialog needs - access to `CharacterSheetProvider` or other React context — `DialogApi` - renders outside the provider tree. See `AGENTS.md` → *Dialog patterns*. +- **Prefer the `useDialogApi` + `use*Dialog` hook pattern** for new dialogs — + it's the project default and React context (including + `CharacterSheetProvider`) propagates normally because `DialogApiProvider` + is mounted inside the relevant providers. See the `DialogApi`-based + examples above and `AGENTS.md` → *Dialog patterns*. - **`onClosed` vs `onClose`**: `onClose` triggers the close animation; `onClosed` fires after it finishes. Reset form state in `onClosed` to avoid a flash of empty fields while the animation runs.