Make SenderId.kind and registration status real enums - #316
Conversation
Both were bare `String` columns with no `@regex`, no `@db_enforce`, and no
CHECK. The four statuses existed only as a `KNOWN_STATUSES` array in the
console, and `kind` was a free-text `<Input>` whose placeholder read
"e.g. alphanumeric". Proven before fixing, then rolled back:
BEGIN; UPDATE sender_id_registrations SET status='banana' …
DB ACCEPTED: banana
`Provider.kind` has had a real CHECK all along, so this was inconsistency
rather than a policy.
**The `kind` vocabulary is a decision made here** — §2.5 declares
`kind String` and never enumerates it. The first draft used
`alphanumeric | numeric`; the compiler rejected it, because
`vsms-demo-seed` and `send_test_message` have both been writing
`"shortcode"` for an all-digit value since they were written. Nothing
enforced that, nothing else knew about it, and no fixture used it — an
unconstrained column quietly growing a second vocabulary in two corners of
the tree is precisely what this change ends. The enum adopts the existing
word rather than renaming it in flight.
No `long_number`: `SenderId.value` is `@length(min: 3, max: 11)` and a
full E.164 number does not fit, so advertising it would be a false
promise.
Migrations regenerated per the documented workflow, CLI matched to the pin
first (`cratestack 0.7.16` == pin `0.7.16`, checked rather than assumed).
The diff against the committed `0001_init` is exactly two CHECK
constraints and nothing else.
UI, answering the report that prompted this — a status that could not be
changed, and a free-text field where an enum belonged:
- Status is a `RadioGroup`, not a `Select`. Four values, and seeing the
other three *is* the decision that drawer exists to make. It also
cannot hit #315's portal-inside-a-focus-trap bug at all: `RadioGroup`
renders inline with no portal and no transition, which makes radio the
safer control here and not merely the friendlier one.
- Create's `kind` is a `RadioGroup` too, with labels and hints, instead
of a text box.
- Both zod schemas are `z.enum(...)` rather than `z.string().min(1)`.
- `FormField`s now pass `error` — that component supports it and this
screen never used it, so a validation failure had nothing to show.
New `RadioGroup` primitive in `@vsms/ui` (Headless UI), since none
existed.
`asSenderIdKind`/`asRegistrationStatus` narrow at the wire seam, where the
generated types still say `string`. The fallback is not defensive padding:
rows written before this migration can hold anything the old column
accepted, and a console that blanks on one is worse than one that shows it
so an operator can correct it. New writes cannot produce an unknown value.
Verified end to end on the live stack, not by build:
- migrations applied to a scratch database; `kind='banana'` and
`status='banana'` both refused by name, a valid row inserted fine
- the radio group renders all four options, picking `rejected` saved,
and the row reads `rejected` at version 2
- the same `UPDATE … 'banana'` that succeeded before now errors on
`sender_id_registrations_status_enum_check`
fmt, clippy `-D warnings`, R1, R2 parity, bootstrap-sql-check, R6, 129
frontend tests, 23/23 routes, biome — all clean. The SDK's vendored schema
was re-vendored in the same change, the step AGENTS.md records as having
made #185 red.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
![lightbridge-assistant[bot]](https://avatars.githubusercontent.com/u/128943481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🅵 Fast automated pass — SAST + a quick, diff-scoped look (no repo-wide retrieval). For a deeper, repo-aware review, mention @lightbridge-assistant on this PR.
Reviewed sdks/rust/vsms-sdk-rust/schema.cstack (this pass was the only unopened file): it is an exact byte-for-byte mirror of the already-reviewed schemas/vsms.cstack for these changes — the two new enums have identical members/comments and both SenderId.kind / SenderIdRegistration.status are retyped identically with matching policies, so nothing drifts between the two schema copies. Combined with the earlier full review (migration CHECK constraints match both Rust and TS enums and are balanced in down.sql; all backend write paths use the enum constants; all frontend z.enum schemas/defaults/resets migrated; the wire-narrowing fallbacks protect the strict enums against pre-migration rows; SAST clean), the change is correct and safe. Only findings remain the two P2 accessibility label-association regressions in create-sender-dialog.tsx and registration-review-fields.tsx.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| was an unconstrained `String` with a placeholder reading | ||
| "e.g. alphanumeric" — so the honest answer to "can I type | ||
| banana here" was yes, all the way to the database. It is a | ||
| real enum now, and there are two values, so showing both |
There was a problem hiding this comment.
Dangling htmlFor on Kind field
The old control was <Input id="new-sender-kind">, which the <Label htmlFor> referenced. The new RadioGroup renders with no id (only an aria-label="Sender ID kind"), so htmlFor="new-sender-kind" now dangles to a non-existent element. FormField's own doc makes htmlFor required because a dangling label silently breaks click-to-focus and the label→control screen-reader association — the group is announced by the separate aria-label rather than the visible "Kind" label (two names, one visual). Consider forwarding an id prop to the RadioGroup (or wiring aria-labelledby to the label). Minor, non-blocking.
Evidence: <FormField label="Kind" htmlFor="new-sender-kind"> (line 72) wraps <RadioGroup aria-label="Sender ID kind" .../> (line 84) which carries no id. Old code used <Input id="new-sender-kind">. FormField renders <Label htmlFor={htmlFor}>.
Was this useful? React 👍/👎 to give us feedback
| {/* A radio group, not a select: four values, and seeing the other | ||
| three is the decision this drawer exists to make. It also cannot | ||
| hit #315's portal-inside-a-focus-trap bug, since `RadioGroup` | ||
| renders inline with no portal and no transition. */} |
There was a problem hiding this comment.
Dangling htmlFor on Status field
Same regression as in create-sender-dialog: the replaced <SelectTrigger id="registration-status"> (which the label's htmlFor referenced) is now a <RadioGroup aria-label="Registration status"> with no id, so htmlFor="registration-status" dangles. The visible "Status" label is no longer programmatically associated with the control. Wire an id/aria-labelledby through to the RadioGroup.
Evidence: <FormField label="Status" htmlFor="registration-status"> (line 24) wraps <RadioGroup aria-label="Registration status" .../> which has no id.
Was this useful? React 👍/👎 to give us feedback
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>
|
Confirmed and fixed in The finding is real, correctly rated P2, and broader than reported. You flagged the two in this PR; the other two are #317's own. All four were dangling, and each had a real The obvious fix would have been wrong. Adding an
The call sites also move off On the review itself, since this feeds a product: accurate, correctly scoped, and it caught something neither |
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>
* Finish the enum work: sender-id edit form, and scopes as chips
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>
* Label the radio and chip groups properly, not with a dangling `for`
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>
* Use a <fieldset> for the chip group, not <div role="group">
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>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
Turns
SenderId.kindandSenderIdRegistration.statusinto real enums, backed by Postgres CHECK constraints — and replaces the two controls that motivated it with radio groups.Intent
Both were bare
Stringcolumns with no@regex, no@db_enforce, and no CHECK. The four statuses existed only as aKNOWN_STATUSESarray in the console;kindwas a free-text<Input>whose placeholder read"e.g. alphanumeric". Proven before fixing, then rolled back:Provider.kindhas had a real CHECK all along — so this was inconsistency, not a policy.The
kindvocabulary is a decision made here§2.5 declares
kind Stringand never enumerates it, so there was nothing to read off.The first draft used
alphanumeric | numeric. The compiler rejected it —vsms-demo-seedandsend_test_messagehave both been writing"shortcode"for an all-digit value since they were written. Nothing enforced it, nothing else knew about it, no fixture used it. An unconstrained column quietly growing a second vocabulary in two corners of the tree is precisely what this ends, so the enum adopts the existing word rather than renaming it in flight.No
long_number:SenderId.valueis@length(min: 3, max: 11)and a full E.164 number does not fit, so advertising it would be a false promise.UI
RadioGroup, not aSelect. Four values, and seeing the other three is the decision that drawer exists to make. It also cannot hit Fix Select being unusable inside a drawer — #274 bug, different primitive #315's portal-inside-a-focus-trap bug:RadioGrouprenders inline with no portal and no transition. For a small vocabulary in a drawer that makes radio the safer control, not merely the friendlier one.kindis aRadioGrouptoo, with labels and hints.z.enum(...)rather thanz.string().min(1).FormFields now passerror— the component supports it and this screen never used it, so a validation failure had nothing to show.RadioGroupprimitive in@vsms/ui(Headless UI); none existed.asSenderIdKind/asRegistrationStatusnarrow at the wire seam, where generated types still saystring. The fallback is not defensive padding: rows written before this migration can hold anything the old column accepted, and a console that blanks on one is worse than one that shows it so an operator can correct it. New writes cannot produce an unknown value — the CHECK refuses them.Verification — live, not by build
CLI matched to the pin first (
cratestack 0.7.16== pin0.7.16), checked rather than assumed. The regenerated0001_initdiffers from the committed one by exactly two CHECK constraints and nothing else.Against a scratch database:
Against the running console:
fmt,clippy -D warnings, R1, R2 parity,bootstrap-sql-check, R6, 129 frontend tests, 23/23 routes, biome — all clean. The SDK's vendored schema was re-vendored in the same change, the step AGENTS.md records as having made #185 red; its own guard caught the omission.Risk Assessment
This is a hard cutover on live data. The CHECK is added unconditionally, so any pre-existing row holding a value outside the vocabulary makes the migration fail on a non-empty database. There is no such row today (
kindisalphanumericeverywhere;statusispending/approved), and per AGENTS.md there is still no production deployment — but a deployment that had accumulated a stray value would need a backfill before this applies.shortcodehas never actually been written to any database — both call sites only produce it for an all-digit sender value, and every seeded sender is alphanumeric. So the value is real in code and untested in practice.The narrowing fallback silently coerces an unknown legacy value to
alphanumeric/pendingrather than surfacing it. That is the right trade for a console but does mean a bad legacy row displays as something it is not, until saved.AI Usage Declaration
Reviewer Focus
kindvocabulary. Two values is my call, taken from what the code already writes. If sender kinds should distinguish more than letters-vs-digits, this is the moment.statuscould go back toSelectnow that Fix Select being unusable inside a drawer — #274 bug, different primitive #315 makes that safe.Checklist
docs/roadmap.mdchecked — no milestone/gate/dependency change.