Skip to content

Finish the enum work: sender-id edit form, and scopes as chips - #317

Merged
stephane-segning merged 3 commits into
mainfrom
fix-kind-edit-and-scopes
Aug 15, 2026
Merged

Finish the enum work: sender-id edit form, and scopes as chips#317
stephane-segning merged 3 commits into
mainfrom
fix-kind-edit-and-scopes

Conversation

@stephane-segning

Copy link
Copy Markdown
Contributor

Summary

Two gaps reported against #316, both real. Stacks on it — merge #316 first.

1. The sender-id edit form still had kind as a text input

The enum migration converted the create dialog and missed this one. So a sender created from a closed vocabulary could still be edited back to free text — the more dangerous half, since create runs once and edit is the path an operator uses repeatedly.

Now the same RadioGroup, same labels and hints.

2. Provisioning a client asked for scopes as space-separated text

An operator had to already know both that scopes are space-delimited and what the valid strings are — with a typo silently producing a client that is denied at Layer 2 with no hint why. Now a ChipSelect over the real vocabulary, each chip carrying what the scope actually permits.

The list came from what the server enforces, not from the doc

grep -rhoE 'require_permission\([^,]+, *"[a-z:]+"' backends/crates/
grep -rhoE '"[a-z]+:[a-z]+"' backends/crates/sms-api/src/router.rs

Two near-misses were checked and deliberately excluded, because offering a scope nothing enforces is worse than offering none — it implies a control that does not exist:

  • message:send appears only as test fixture data in rbac.rs's unit tests. AGENTS.md records the seeded role permissions being renamed message:sendsms:send precisely because the literals had drifted from what require_permission checks.
  • provider:write appears only inside a doc comment in router.rs explaining a past bug — the constant once checked that literal, which matched nothing, permanently denying a legitimate operator token.

scopes.ts records that derivation, and states plainly that nothing mechanically ties the list to the Rust literals: a new require_permission("thing:do") will not appear there on its own. That wants an xtask parity check of the kind that already guards the state machines. Flagged, not built — so the next person adding a scope knows there are two places.

New primitive

ChipSelect in @vsms/ui. Like RadioGroup, it renders inline with no portal and no transition, so it cannot hit the focus-trap bug that made Select unusable inside a drawer (#315).

Headless UI 2.2.10 exports no CheckboxGroup — checked, not assumed; tsc rejected it — so grouping is a plain role="group" wrapper rather than a new dependency.

serializeScopes emits in vocabulary order rather than click order, so two identical grants do not diff against each other.

Verification

Verified live — the sender edit drawer:

kindIsTextInput: false
radioOptions: ["Alphanumeric — A brand name, 3–11 characters",
               "Short code — All digits"]

NOT verified live — the scope chips rendering. The old #client-scopes input is confirmed gone from the apps panel, and typecheck/build/tests pass, but I could not get the provision drawer open in the browser harness to see the chips themselves. Saying so rather than implying otherwise.

$ pnpm typecheck (admin + @vsms/ui)   # clean
$ pnpm test                           # 129 passed
$ SKIP_ENV_VALIDATION=true pnpm build # ✓ 23/23 routes
$ pnpm exec biome check frontends     # clean
$ cargo xtask r6                      # R6 OK

Risk Assessment

The unverified half is the one with more surface. ChipSelect is a new component whose only call site I could not observe rendering. Its mechanism is the same inline, no-portal shape RadioGroup already proves in a drawer, and the value round-trip (parseScopes/serializeScopes) is pure and typechecked — but "renders correctly in the provision panel" is currently an inference.

parseScopes silently drops unknown values. A client provisioned before this with a scope outside the list — or with a typo, which was easy — will show fewer chips than it has scopes, and saving would then remove them. That is arguably correct (the extras were never enforced) but it is a silent narrowing, not a prompt.

AI Usage Declaration

  • A human directed this change and is accountable for it.
  • Claims in this PR were verified against a running system, except where explicitly marked otherwise above.

Reviewer Focus

  1. Open the provision panel — the one thing I could not check.
  2. parseScopes dropping unknowns silently. Should an unrecognised stored scope surface as a warning chip instead of vanishing?

Checklist

  • docs/roadmap.md checked — no milestone/gate/dependency change.
  • Framework surprise recorded in the component: Headless UI 2.2.10 has no CheckboxGroup.
  • New R1 exceptions — none.

@changeset-bot

changeset-bot Bot commented Aug 15, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 7bba704

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

stephane-segning and others added 3 commits August 15, 2026 17:52
Two gaps reported against the enum migration, both real.

**1. The sender-id *edit* form still had `kind` as a text input.** The
migration converted the create dialog and missed this one — so a sender
created from a closed vocabulary could still be edited back to free text.
That is the more dangerous half: create runs once, edit is the path an
operator uses repeatedly. Now the same `RadioGroup`, with the same labels
and hints.

**2. Provisioning a client asked for scopes as space-separated text.** An
operator had to already know both that scopes are space-delimited *and*
what the valid strings are, with a typo silently producing a client that
is denied at Layer 2 with no hint why. Now a `ChipSelect` over the real
vocabulary, each chip carrying what the scope actually permits.

The scope list was derived from what the server enforces, not from the
design doc or memory:

    grep -rhoE 'require_permission\([^,]+, *"[a-z:]+"' backends/crates/
    grep -rhoE '"[a-z]+:[a-z]+"' backends/crates/sms-api/src/router.rs

Two near-misses were checked and deliberately excluded, because offering a
scope nothing enforces is worse than offering none — it implies a control
that does not exist:

  - `message:send` appears only as test fixture data in `rbac.rs`'s own
    unit tests. AGENTS.md records the seeded role permissions being
    renamed `message:send` -> `sms:send` precisely because the literals had
    drifted from what `require_permission` checks.
  - `provider:write` appears only inside a doc comment in `router.rs`
    explaining a past bug — the constant once checked that literal, which
    matched nothing, permanently denying a legitimate operator token.

`scopes.ts` records that derivation, and states plainly that nothing
mechanically ties the list to the Rust literals: a new
`require_permission("thing:do")` will not appear there on its own. That
wants an `xtask` parity check of the kind that already guards the state
machines. Not built here — flagged, so the next person adding a scope
knows there are two places.

New `ChipSelect` primitive in `@vsms/ui`. Like `RadioGroup`, it renders
inline with no portal and no transition, so it cannot hit the focus-trap
bug that made `Select` unusable inside a drawer (#315). Headless UI 2.2.10
exports no `CheckboxGroup` — checked, not assumed, `tsc` rejected it — so
grouping is a plain `role="group"` wrapper rather than a new dependency.

`serializeScopes` emits in vocabulary order rather than click order, so
two identical grants do not diff against each other.

Verified live: the sender edit drawer now renders
`["Alphanumeric — A brand name, 3–11 characters", "Short code — All
digits"]` with `kindIsTextInput: false`.

NOT verified live: the scope chips rendering. The old `#client-scopes`
input is confirmed gone from the apps panel, and typecheck/build/tests
pass, but I could not get the provision drawer open in the browser
harness to see the chips themselves. Stated rather than implied.

typecheck (admin + @vsms/ui), 129 tests, 23/23 routes, biome, R6 — clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review finding on #316, confirmed and broader than reported.

`FormField` renders `<Label htmlFor={htmlFor}>`, and `RadioGroup`/
`ChipSelect` accept no `id`. So every field converted from an `<Input>` to
a group left the label pointing at an element that does not exist:

    sender-kind            no element carries this id
    new-sender-kind        no element carries this id
    registration-status    no element carries this id
    client-scopes          no element carries this id

The review flagged two (#316's); the other two are this branch's own. All
four were dangling — before the conversion each had a real `<Input id=…>`
to associate with.

Adding an `id` to the group would not fix it. HTML's `for` only associates
with *labelable* form controls, and a `role="radiogroup"`/`role="group"`
wrapper is not one — the reference would still be invalid, just no longer
obviously broken.

So `FormField` gains `control="group"`. In that mode it emits no `for` at
all; the label carries `groupLabelId(htmlFor)` and the grouped control
points back with `aria-labelledby`. Both sides derive the id from the same
exported function, so they cannot drift — which matters, because a second
hand-written string is how the original `htmlFor` came to reference
nothing in the first place.

The four call sites move from `aria-label` (which worked, but duplicated
the visible label as a second string) to `aria-labelledby` pointing at the
visible label itself.

Verified statically, since this is a DOM-identity property rather than a
behavioural one: no element carried any of the four ids before, and each
grouped field now pairs `control="group"` with a matching
`groupLabelId(...)`, checked per id rather than in aggregate.

typecheck (admin + @vsms/ui), 129 tests, 23/23 routes, biome, R6 — clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Biome's `a11y/useSemanticElements` is right: the native element beats the
ARIA role, and a checkbox group is exactly what `<fieldset>` is for.

Worth recording how it was found, because the mistake is reusable. A local
`pnpm exec biome check frontends` passed. CI runs `pnpm biome ci .` — a
different command over a different scope — and failed. That is the third
time this pattern has cost a round trip: `cargo clippy -p xtask` vs
`--workspace` for the R6 guard, and `npm publish --dry-run` vs `npm pack`
for the SDK. Run CI's exact command, not a plausible neighbour of it.

`min-w-0` is load-bearing, not tidying: a `<fieldset>` carries a UA
`min-width: min-content` that a `<div>` does not, which would otherwise
stop the chips wrapping inside a narrow drawer.

Verified with CI's own command this time: `pnpm biome ci .` — 394 files,
no fixes applied, exit 0. typecheck (admin + @vsms/ui), 129 tests, 23/23
routes all still clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@stephane-segning
stephane-segning force-pushed the fix-kind-edit-and-scopes branch from bbc6a22 to 7bba704 Compare August 15, 2026 15:53
@stephane-segning
stephane-segning merged commit 0e63689 into main Aug 15, 2026
8 checks passed
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