web: pin Server/Client Settings modal header while body scrolls

[cliffhall]

Closes #1698

Problem

Expanding enough accordion sections in the Server Settings modal grew the modal past the viewport, so the whole modal scrolled and carried the header (title + close button) out of view.

Fix

The header was a custom Group inside the modal body, and the modal used no scrollAreaComponent, so overflow wasn't confined to the body. Switched both the Server Settings and Client Settings modals from the default <Modal> (which always wraps children in Modal.Body) to the compound Modal.Root / Modal.Content / Modal.Header / Modal.Body API:

• The toggle / title / close row now lives in Modal.Header, which Mantine renders position: sticky; top: 0 by design.
• scrollAreaComponent={ScrollArea.Autosize} confines overflow to Modal.Body.
• Modal.Root keeps the close-on-escape / close-on-click-outside / focus-trap / scroll-lock / portal defaults; transitionProps restores the fade-down animation the <Modal> wrapper otherwise supplies.

Only props are used (no CSS classes / inline styles), per project conventions. Client Settings doesn't yet have enough content to scroll but is kept consistent per the issue.

Verification

Storybook ServerSettingsModal (FullyConfigured, all sections expanded, ~680px viewport), driven via Playwright. Measured in-page: the body scroll container is scrollHeight 2253 × clientHeight 612 (scrolls internally), and the header is position: sticky with its top flush to the dialog top after scrolling.

Scrolled to top	Scrolled down
Header above the first section.	Body scrolled to Custom Headers / Request Metadata — the same header stays pinned.

More detail in pr-screenshots/README.md.

Tests

Existing ServerSettingsModal / ClientSettingsModal unit tests pass unchanged (51 tests); both files stay above the ≥90 coverage gate (ClientSettings 100%, ServerSettings ~98/100/97/98). npm run ci passes (validate → coverage → smoke → Storybook).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 0s —— View job

---

Review: sticky Settings-modal header

Read both modal changes against origin/v2/main and the existing tests. This is a clean, well-scoped fix — the diagnosis (custom header living inside Modal.Body, no scrollAreaComponent, so overflow scrolled the whole dialog) is right, and the compound-API approach is the idiomatic Mantine solution.

What's good

• Header moved into Modal.Header (which Mantine renders position: sticky; top: 0) + scrollAreaComponent={ScrollArea.Autosize} confining overflow to Modal.Body is exactly the correct fix. Autosize (vs plain ScrollArea) is the right pick so the dialog only grows to content before capping at the viewport.
• Props-only, no CSS classes / inline styles — matches the project's Mantine-first conventions.
• Compound scaffolding is complete and correct: Modal.Overlay is rendered explicitly (required with Modal.Root), Modal.Content supplies role="dialog"/aria-modal, and Modal.Title keeps the aria-labelledby wiring. No accessibility regression.
• Removing the now-redundant withCloseButton={false} is correct — the compound API doesn't auto-render a close button.
• The explanatory comments on the why are genuinely useful.
• Existing tests hold without changes: the close-button test keys off button.mantine-CloseButton-root (still present via the custom CloseButton) and the text/role queries are structure-independent.

Minor points (non-blocking)

1. transitionProps duplication vs. the theme default. { transition: "fade-down", duration: 200 } is now hardcoded in both files to restore what <Modal> supplied by default. Worth a quick confirm that this still matches Mantine v8's default so these two modals animate identically to every other <Modal> in the app — if Mantine ever changes its default, these two drift silently. Not a bug, just a maintenance seam. If you want to avoid the literal in two places, a small shared const SETTINGS_MODAL_TRANSITION (or a Modal.Root default via the theme) would DRY it.

2. ThemeModal doesn't reach Modal.Root. src/theme/Modal.ts sets closeButtonProps: { "aria-label": "Close" } on the Modal component name, which does not apply to the compound Modal.Root. Harmless here because both call sites use their own <CloseButton />, so the label is preserved — just flagging that the theme default is now bypassed for these two modals, in case a future Modal.CloseButton gets added.

3. No unit assertion for the sticky/scroll behavior. Reasonable — it's CSS-driven by Mantine and not observable in happy-dom, and the Playwright/Storybook measurement in the PR body (scrollHeight 2253 × clientHeight 612, header position: sticky) is the pragmatic proof. Fine to leave as-is.

Verdict: LGTM. The change is correct, minimal, convention-adherent, and the two minor points above are optional polish rather than blockers.
· branch v2/1698-modal-sticky-header

[cliffhall]

Thanks for the thorough review! Responses to the three points:

1. transitionProps duplication — addressed (144df7e). Moved the { transition: "fade-down", duration: 200 } literal into a ThemeModalRoot (Modal.Root.extend) registered under the ModalRoot theme key, mirroring the existing ThemeScrollAreaAutosize precedent. Both call sites now drop the literal, so it lives in one place and any future compound Modal.Root animates consistently. Confirmed the key is right: Mantine's ModalRoot calls useProps("ModalRoot", …), and the explicit <Modal> wrapper still passes its own transitionProps so plain modals are unaffected.

2. ThemeModal not reaching Modal.Root — acknowledged, and the new ThemeModalRoot now covers the compound path for anything Root-level we set going forward. The closeButtonProps default specifically is still a no-op here (both modals use their own <CloseButton>), so no change needed for that; if a Modal.CloseButton is ever added it can get the label explicitly or via a ThemeModalCloseButton.

3. No unit assertion for sticky/scroll — agreed, left as-is: it's Mantine CSS not observable in happy-dom, and the Playwright measurement (scrollHeight 2253 × clientHeight 612, header position: sticky) in the PR body is the pragmatic proof.

npm run ci still passes after the refactor.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 52s —— View job

---

Re-review: ThemeModalRoot refactor (commit 144df7e)

Read the follow-up commit against origin/v2/main. This cleanly resolves point 1 from the prior review, and nothing regressed.

The refactor is correct

• ThemeModalRoot = Modal.Root.extend({ defaultProps: { transitionProps: { transition: "fade-down", duration: 200 } } }) registered under the ModalRoot theme key is the right mechanism. Mantine's compound Modal.Root resolves its own defaults via useProps("ModalRoot", …), so the key matches, and it directly mirrors the existing ThemeScrollAreaAutosize precedent (clients/web/src/theme/ScrollArea.ts) — same pattern, same rationale (set once, no per-call-site literal).
• Plain <Modal> is unaffected: Mantine's <Modal> carries its own transitionProps default and passes it explicitly down to ModalRoot, and an explicitly-passed prop wins over the ModalRoot default. So the two settings modals now animate identically to every plain <Modal>, and the literal lives in exactly one place. Confirmed as you described.
• Barrel + theme wiring is complete and consistent: index.ts re-exports ThemeModalRoot, theme.ts imports it and registers ModalRoot: ThemeModalRoot alphabetically alongside Modal: ThemeModal. No dangling export.
• The doc comment on ThemeModalRoot is genuinely useful — it explains why Modal.Root doesn't inherit the wrapper's transition and points at Server Settings / Client Settings modals: header scrolls out of view #1698.

Both modal call sites are sound

• Compound scaffolding is complete: Modal.Overlay rendered explicitly (required with Modal.Root), Modal.Content supplies role="dialog"/aria-modal, Modal.Title preserves aria-labelledby, and the custom CloseButton keeps its explicit aria-label="Close" (correctly not relying on ThemeModal's closeButtonProps, which doesn't reach the compound path — point 2 from last time).
• w="100%" added to the header Group is the right touch — Modal.Header is a flex row, so the Group needs to fill it for the flex={1} centered title + right-aligned close button to lay out as before.
• scrollAreaComponent={ScrollArea.Autosize} on both keeps the header (position: sticky; top: 0) pinned inside the scroll area while only the body overflows — the core fix.

Verdict: LGTM. The refactor is minimal, convention-adherent, and DRYs the transition into one theme location without affecting plain modals. Both prior non-blocking points are now addressed or consciously deferred. Nothing further from me.
· branch v2/1698-modal-sticky-header

[cliffhall]

Thanks for the re-review — glad the theme approach landed cleanly. No further changes; ready to merge whenever.