feat(boringstack): explicit, secure, correct CRUD UI+API contract in refine-prompt#129
Open
agjs wants to merge 1 commit into
Open
feat(boringstack): explicit, secure, correct CRUD UI+API contract in refine-prompt#129agjs wants to merge 1 commit into
agjs wants to merge 1 commit into
Conversation
…refine-prompt (kill hollow list-only UI)
The boringstack build kept shipping a hollow feature: the UI scaffold is a
list-only page plus stub hooks, and reachability only checked route + i18n keys,
so a feature with no real create/edit/delete wiring passed as "verified". This
rewrites the per-resource refine prompt into an explicit contract.
API layer: full user-scoped CRUD (list/get-one/create/update/delete), each route
under requireAuth passing user.id. SECURITY: get/update/delete must filter by BOTH
row id AND authenticated userId (and(eq(id), eq(userId))) — an id-only lookup is
horizontal privilege escalation. Service extends the scaffold's listForUser+create
convention (never rename them).
UI layer: real mutations (create/update/delete) + a real list query, a list that
renders records with Edit/Delete, a Zod-validated create/edit form, and a delete
confirmation — wired end to end (create → appears → edit → delete). Wiring the
mutations is how the i18n error/confirm keys become "used" — never delete keys.
Error idiom (was wrong): the typed api-client resolves HTTP 4xx/5xx as a result
object { data, error } and does NOT throw. Both queries and mutations must
`const { data, error } = await apiClient.…; if (error) throw error; return data;`
or React Query runs onSuccess on failures and queries silently return undefined.
domainFields (persistence columns + form inputs) is the entity's **Fields** only;
**Display** is a rendering hint (may be computed/relationship/read-only) and must
not become columns or inputs. Slice-aware, with a behavior fallback so no
instruction dangles a "Product Context above" reference on the no-slice path.
Required tests force the behavior (prose alone fails): API service tests must
include an ownership-isolation test (user B cannot read/update/delete user A's
row); the UI vitest must drive create AND edit AND delete from the rendered list,
not just create.
Addresses the reviewer panel's fresh findings: wrong error idiom (3/4 agree),
unverified security clause, create-only UI test, fieldSource conflating
Fields+Display, untested slice branch. Drops an unrelated .gitignore change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The boringstack build kept shipping a hollow feature: the UI scaffold is a list-only page plus stub hooks (
useCreatereturns its input), and reachability only checked route + i18n keys — so a feature with no real create/edit/delete wiring passed as "verified" (#50).What this changes (piece 1 of #50 — the per-resource prompt contract)
API layer — full user-scoped CRUD (list / get-one / create / update / delete), each route under
requireAuthpassinguser.id.userId(and(eq(id), eq(userId))). An id-only lookup is horizontal privilege escalation.listForUser+createconvention (never renames them).UI layer — real mutations + real list query, a list that renders records with Edit/Delete, a Zod-validated create/edit form, and a delete confirmation, wired end to end (create → appears → edit → delete). Wiring the mutations is how the i18n error/confirm keys become "used" — never delete keys.
Error idiom (was wrong) — the typed api-client resolves HTTP 4xx/5xx as
{ data, error }and does not throw. Both queries and mutations mustconst { data, error } = await apiClient.…; if (error) throw error; return data;, or React Query runsonSuccesson failures and queries silently returnundefined.domainFields — persistence columns + form inputs come from the entity's Fields only; Display is a rendering hint (may be computed/relationship/read-only) and must not become columns or inputs. Slice-aware with a behavior fallback (no dangling "Product Context above" on the no-slice path).
Tests force the behavior (prose alone fails): API service tests must include an ownership-isolation test (user B cannot touch user A's row); the UI vitest must drive create AND edit AND delete from the rendered list.
Reviewer panel
Passed the local harness-review panel after addressing its fresh findings: wrong error idiom (3/4 agree), unverified security clause, create-only UI test,
fieldSourceconflating Fields+Display, and the untested slice branch. Also dropped an unrelated.gitignorechange.Prompt-only change (no runtime behavior beyond what the model is told to generate). Follow-up in #50: mutations-first scaffold + semantic reachability.