From d722d5a5f096695d686ceae63659dd09bbedab91 Mon Sep 17 00:00:00 2001 From: Charles Sexton Date: Sat, 27 Jun 2026 12:52:35 +0100 Subject: [PATCH] harden the build-verify gate and fix the revise-rating spec contradiction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most demo failures only surfaced at the deploy build because the gate did not run the compiler: the scaffold skill said "add a verify script" and the fast model wrote lint+test, dropping tsc. Make the factory produce the right config by construction, and fix the spec defect behind duplicate feedback rows. - scaffold-code-app (.agents + .claude): prescribe exact package.json scripts so verify = typecheck (tsc -b) + lint + test; split test types into tsconfig.test.json so they stop polluting the app build; pin eslint to es2022 - AGENTS.md: gate must compile; fixtures typed from the generated model; one un-mocked smoke render through the real module graph - dataverse-schema.md: add update_feedback contract + statecode note; §4 one row per rating not per submission; §9 BDD scenario for revise-updates-not-creates - instantiation §4 and task-1/task-3 gates updated to match Co-Authored-By: Claude Opus 4.8 (1M context) --- .agents/skills/scaffold-code-app/SKILL.md | 53 ++++++++++++++++++++--- .claude/skills/scaffold-code-app/SKILL.md | 53 ++++++++++++++++++++--- AGENTS.md | 28 ++++++++++-- specs/dataverse-schema.md | 34 ++++++++++++++- specs/session-feedback-instantiation.md | 6 ++- specs/tasks/task-1-data-layer.md | 5 ++- specs/tasks/task-3-rate-modal.md | 19 ++++++-- 7 files changed, 171 insertions(+), 27 deletions(-) diff --git a/.agents/skills/scaffold-code-app/SKILL.md b/.agents/skills/scaffold-code-app/SKILL.md index 27449ce..62736e9 100644 --- a/.agents/skills/scaffold-code-app/SKILL.md +++ b/.agents/skills/scaffold-code-app/SKILL.md @@ -71,15 +71,54 @@ sign-in is handled later by the `dataverse-scaffold-tables` skill — don't run ### 4. Set up the quality tooling (make the app verifiable) The Vite starter ships **no test runner** and at most a minimal linter, but every build task is gated -by `npm run verify`. Make the project verifiable now, before any feature work: +by `npm run verify`. Make the project verifiable now, before any feature work. Do not paraphrase this +list or trust a gate that merely "looks right": the gate only protects the build if it runs the +**compiler**, so set the scripts exactly as below. -- Add **Vitest + React Testing Library + jsdom**, wired to `npm test`. -- Add **ESLint + typescript-eslint + eslint-plugin-react-hooks**, wired to `npm run lint`. -- Add an **`npm run verify`** script that runs typecheck + lint + test in one command. +**a. Test runner.** Add **Vitest + React Testing Library + jsdom**, wired to `npm test`. Add +`src/setupTests.ts` importing `@testing-library/jest-dom`, referenced from the Vitest config. -See **AGENTS.md (repo root) → Stack notes** for the exact pins. After this, `npm run verify` runs -clean on the empty app, so the first task gate (Task 1) has a working harness to run against rather -than being set up live. +**b. Linter.** Add **ESLint + typescript-eslint + eslint-plugin-react-hooks** (plus the Vite +`react-refresh` plugin the starter expects), wired to `npm run lint`. Set the ESLint +`languageOptions.ecmaVersion` to **2022** to match the app's `target`, not the starter's `2020`. + +**c. The gate must typecheck.** Set these `package.json` scripts **verbatim**. The typecheck step is +what catches the class of error a fast model produces (reading a response by the wrong property, a +string where a Choice integer is required, a create payload missing a required field). A gate without +it passes green while the build is broken: + +```json +"typecheck": "tsc -b", +"lint": "eslint .", +"test": "vitest", +"verify": "npm run typecheck && npm run lint && npm run test -- --run" +``` + +`npm run verify` **must** run `tsc`. If it only runs lint and test, type errors stay hidden until +`npm run build` at deploy. That is a configuration failure, the exact failure mode this harness exists +to remove. + +**d. Keep test types out of the app build, but still typecheck the tests.** Tests must not force their +globals into the production compile, yet they must still be type-checked by the gate. Three moves: + +- In `tsconfig.app.json`, **exclude** the test files and keep `types` to app types only (no + `vitest/globals`): + + ```json + "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx", "src/setupTests.ts"] + ``` + +- Add a **`tsconfig.test.json`** that includes the test files and carries the test globals: + `"types": ["vite/client", "vitest/globals", "@testing-library/jest-dom"]`. +- **Reference both** from the root `tsconfig.json` so `tsc -b` (the gate's typecheck and the build's + first half) checks the app and test projects in one pass. + +This is why the app build never needs `vitest/globals` bolted into `tsconfig.app.json`: the tests get +their globals from their own project, and a wrong fixture shape still fails `tsc` at the gate. + +See **AGENTS.md (repo root) → Stack notes** for the exact pins and the test-authoring rules (typed +fixtures, one un-mocked smoke render). After this, `npm run verify` runs clean on the empty app, so +the first task gate (Task 1) has a working harness to run against rather than being set up live. ## After scaffolding diff --git a/.claude/skills/scaffold-code-app/SKILL.md b/.claude/skills/scaffold-code-app/SKILL.md index 27449ce..62736e9 100644 --- a/.claude/skills/scaffold-code-app/SKILL.md +++ b/.claude/skills/scaffold-code-app/SKILL.md @@ -71,15 +71,54 @@ sign-in is handled later by the `dataverse-scaffold-tables` skill — don't run ### 4. Set up the quality tooling (make the app verifiable) The Vite starter ships **no test runner** and at most a minimal linter, but every build task is gated -by `npm run verify`. Make the project verifiable now, before any feature work: +by `npm run verify`. Make the project verifiable now, before any feature work. Do not paraphrase this +list or trust a gate that merely "looks right": the gate only protects the build if it runs the +**compiler**, so set the scripts exactly as below. -- Add **Vitest + React Testing Library + jsdom**, wired to `npm test`. -- Add **ESLint + typescript-eslint + eslint-plugin-react-hooks**, wired to `npm run lint`. -- Add an **`npm run verify`** script that runs typecheck + lint + test in one command. +**a. Test runner.** Add **Vitest + React Testing Library + jsdom**, wired to `npm test`. Add +`src/setupTests.ts` importing `@testing-library/jest-dom`, referenced from the Vitest config. -See **AGENTS.md (repo root) → Stack notes** for the exact pins. After this, `npm run verify` runs -clean on the empty app, so the first task gate (Task 1) has a working harness to run against rather -than being set up live. +**b. Linter.** Add **ESLint + typescript-eslint + eslint-plugin-react-hooks** (plus the Vite +`react-refresh` plugin the starter expects), wired to `npm run lint`. Set the ESLint +`languageOptions.ecmaVersion` to **2022** to match the app's `target`, not the starter's `2020`. + +**c. The gate must typecheck.** Set these `package.json` scripts **verbatim**. The typecheck step is +what catches the class of error a fast model produces (reading a response by the wrong property, a +string where a Choice integer is required, a create payload missing a required field). A gate without +it passes green while the build is broken: + +```json +"typecheck": "tsc -b", +"lint": "eslint .", +"test": "vitest", +"verify": "npm run typecheck && npm run lint && npm run test -- --run" +``` + +`npm run verify` **must** run `tsc`. If it only runs lint and test, type errors stay hidden until +`npm run build` at deploy. That is a configuration failure, the exact failure mode this harness exists +to remove. + +**d. Keep test types out of the app build, but still typecheck the tests.** Tests must not force their +globals into the production compile, yet they must still be type-checked by the gate. Three moves: + +- In `tsconfig.app.json`, **exclude** the test files and keep `types` to app types only (no + `vitest/globals`): + + ```json + "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx", "src/setupTests.ts"] + ``` + +- Add a **`tsconfig.test.json`** that includes the test files and carries the test globals: + `"types": ["vite/client", "vitest/globals", "@testing-library/jest-dom"]`. +- **Reference both** from the root `tsconfig.json` so `tsc -b` (the gate's typecheck and the build's + first half) checks the app and test projects in one pass. + +This is why the app build never needs `vitest/globals` bolted into `tsconfig.app.json`: the tests get +their globals from their own project, and a wrong fixture shape still fails `tsc` at the gate. + +See **AGENTS.md (repo root) → Stack notes** for the exact pins and the test-authoring rules (typed +fixtures, one un-mocked smoke render). After this, `npm run verify` runs clean on the empty app, so +the first task gate (Task 1) has a working harness to run against rather than being set up live. ## After scaffolding diff --git a/AGENTS.md b/AGENTS.md index 65eebed..de0b992 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -73,10 +73,14 @@ diverge from its spec. honour its "out of scope" line. - **Stop at the verify gate.** Run the task's checks, report against its "done when", and wait. Do not roll straight into the next task. -- **The gate is one command.** `npm run verify` chains typecheck + lint + test so the check is - mechanical, not remembered. Run it at every task gate; `npm run build` runs once before deploy. +- **The gate is one command, and it must compile.** `npm run verify` chains **typecheck + lint + + test** (`npm run typecheck && npm run lint && npm run test -- --run`, where `typecheck` is `tsc -b`). + The typecheck is not optional: drop it and a wrong response shape, a string where a Choice integer is + required, or a create payload missing a required field all pass the gate and only surface at + `npm run build` on deploy. Run `verify` at every task gate; `npm run build` runs once before deploy. There is no CI or git hook: the demo build is a throwaway local project (no GitHub repo on the day), - so `verify` at each gate is the enforcement. + so `verify` at each gate is the enforcement, which is exactly why it has to run the compiler and not + just lint and test. - **Per task:** typecheck + lint (near-instant). **On the data-layer task:** the audit pass (the checklist in tasks.md Task 1). The test suite exists from Task 1's smoke test and peaks at the modal milestone (Task 3). @@ -93,9 +97,25 @@ diverge from its spec. - The template does **not** ship a test runner. Add **Vitest + React Testing Library + jsdom** explicitly, wired to `npm test`. Tests mock the generated `Eppc_*Service` modules (schema §9) and never touch live Dataverse. +- **Typecheck the tests, but keep their globals out of the app build.** Test files live under `src` + but must be excluded from `tsconfig.app.json`; give them a `tsconfig.test.json` carrying + `vitest/globals`, and reference both from the root `tsconfig.json` so `tsc -b` checks them together. + Never add `vitest/globals` to `tsconfig.app.json` to make the build pass; that is the app build + compiling tests, which the exclusion fixes. +- **Derive fixtures from the generated model types; never hand-author the shape.** Build mock sessions + and feedback as typed values (`const s: Eppc_sessions = { ... }`, or a small typed factory) and read + service results by their real property names (the SDK returns `response.data`, not `.value`). A mock + that invents its own shape will agree with a wrong app and pass while the build breaks; a typed + fixture turns the same mistake into a `tsc` error. The create payload must satisfy the generated + `Eppc_feedbacks` model, including any required system field it types (e.g. `statecode: 0`). +- **Keep one un-mocked smoke render.** Alongside the mocked unit tests, the suite must include one test + that imports and renders `App` through the **real module graph**, mocking only at the `Eppc_*Service` + boundary, not your own modules. Mocking the module under test hides import-resolution failures and + default-vs-named import mistakes that throw at runtime (`type is invalid ... got: undefined`) while + the mocked suite stays green. - Lint is a named gate, but the template's ESLint setup is minimal or absent. Pin **ESLint + typescript-eslint + eslint-plugin-react-hooks** on `npm run lint`; the react-hooks rule catches a - class of bug a fast model will produce. + class of bug a fast model will produce. Set `ecmaVersion` to **2022** to match the app's `target`. - Load `Barlow` and `Barlow Semi Condensed` from Google Fonts (contract §1.6). ## When something is ambiguous diff --git a/specs/dataverse-schema.md b/specs/dataverse-schema.md index 60d1e73..e5d6386 100644 --- a/specs/dataverse-schema.md +++ b/specs/dataverse-schema.md @@ -137,7 +137,9 @@ columns: ## 4. Feedback — `eppc_feedback` -Written at runtime, one row per rating submission. Anonymous by design. +Written at runtime: **one row per rating**, created on first submit and **updated in place** when the +attendee revises it (the card's Edit affordance, instantiation §3.5). Not one row per submission, a +revise must not duplicate. Anonymous by design. ```yaml table: @@ -302,14 +304,33 @@ read_feedback_for_session: # rated state / counts select: [eppc_rating, eppc_comments, _eppc_sessionid_value] filter: _eppc_sessionid_value eq -create_feedback: # Rate modal submit +create_feedback: # Rate modal submit (first rating for this session) service: Eppc_feedbacksService.create payload: eppc_rating: 5 # integer 1..5, NOT "5" eppc_comments: "Great session" # omit/empty when optional and blank "eppc_SessionId@odata.bind": "/eppc_sessions()" + statecode: 0 # Active; the generated model types statecode as required on create + +update_feedback: # Rate modal submit when revising an existing rating (the Edit affordance) + service: Eppc_feedbacksService.update + id: + changedFields: + eppc_rating: 3 # the revised integer 1..5 + eppc_comments: "Sharper on reflection" # omit/empty when optional and blank ``` +> **Revise updates, it does not create.** The Rate modal's Edit affordance (instantiation §3.5, §4) +> revises a rating. Hold the `eppc_feedbackid` that `create` returns in app state and call `update` on +> it; calling `create` a second time duplicates the row. Anonymity sets the boundary: there is no +> attendee key, so "the existing rating" means the row this app session created, not one matched across +> devices. Cross-device dedupe is deliberately out of scope (§4 anonymity note). +> +> **System fields on create.** The generated `Eppc_feedbacks` model types `statecode` (and may type +> others) as **required** even though Dataverse defaults it (Active = `0`) on create. Satisfy the +> generated type: send `statecode: 0`. The generated model is the source of truth for the payload +> shape, and `tsc` at the task gate is what surfaces a missing field, not the deploy build. + --- ## 9. BDD scenarios (data behaviour) @@ -354,4 +375,13 @@ Feature: Submitting session feedback to Dataverse When they submit and Eppc_feedbacksService.create resolves Then that session's card renders its rated footer And the rated count increments by one + + Scenario: Revising a rating updates the existing row, not creates a new one + Given a session rated 4 in this app session, whose feedback id is held in state + And the Rate modal is reopened via the card's Edit affordance + When the attendee changes the rating to 3 and submits + Then Eppc_feedbacksService.update is called with that feedback id + And Eppc_feedbacksService.create is not called again + And the session still has exactly one feedback row + And the rated count does not change ``` diff --git a/specs/session-feedback-instantiation.md b/specs/session-feedback-instantiation.md index 8af4ffc..1e11d4b 100644 --- a/specs/session-feedback-instantiation.md +++ b/specs/session-feedback-instantiation.md @@ -118,8 +118,10 @@ Per the contract content card (4.6), filled as: hover previews; submit requires a non-zero draft (and a comment when `requireComment`); success animates once then rests (contract 5.2-5.5). - **Persistence:** ratings persist via the app's data layer with optimistic reflect (contract 5.6). - In the build app this is `Eppc_feedbacksService.create` to Dataverse (see the Dataverse schema); the - standalone reference uses `localStorage` key `eppc26_ratings`, merging seed data under saved data. + In the build app a **first** rating is `Eppc_feedbacksService.create` and a **revise** (the card's + Edit affordance, §3.5) is `Eppc_feedbacksService.update` on the `eppc_feedbackid` the create returned + (Dataverse schema §8), never a second `create`, which would duplicate the row. The standalone + reference uses `localStorage` key `eppc26_ratings`, merging seed data under saved data. - **Counts:** `ratedCount` = sessions with a rating > 0; `totalCount` = all sessions. --- diff --git a/specs/tasks/task-1-data-layer.md b/specs/tasks/task-1-data-layer.md index bc94b26..833ea9c 100644 --- a/specs/tasks/task-1-data-layer.md +++ b/specs/tasks/task-1-data-layer.md @@ -31,7 +31,10 @@ - One smoke test proves the data layer: mocking the generated service, it asserts the read path returns typed sessions ordered by `eppc_starttime`. The test runner itself was set up by `scaffold-code-app`; this task just adds the first test, giving the data layer its own automated - gate so the Task 3 milestone runs against an already-exercised harness. + gate so the Task 3 milestone runs against an already-exercised harness. The fixture is a typed + `Eppc_sessions[]` value (not a hand-shaped object) and the read is taken from the service result's + real `response.data` shape, so any drift from the generated model fails `tsc` rather than passing a + self-consistent mock (AGENTS.md Stack notes). **Done when:** - `tsc` is clean. diff --git a/specs/tasks/task-3-rate-modal.md b/specs/tasks/task-3-rate-modal.md index 8cc3f48..9371332 100644 --- a/specs/tasks/task-3-rate-modal.md +++ b/specs/tasks/task-3-rate-modal.md @@ -27,16 +27,27 @@ - The rate modal as specified — track-coloured top bar, header, identity row, star picker with live word caption, optional comment field, and submit-button states that name the blocking reason in the label until valid. The spec sections own the copy and the states. -- The write that closes the loop: `Eppc_feedbacksService.create` on submit with the lookup bound via - `@odata.bind`, the card reflecting the result optimistically, and counts updating — this is the - task's reason to exist. +- The write that closes the loop: `Eppc_feedbacksService.create` on a first submit with the lookup + bound via `@odata.bind`, the card reflecting the result optimistically, and counts updating — this is + the task's reason to exist. +- **Revise updates, it does not create:** the card's Edit affordance reopens the modal and a second + submit calls `Eppc_feedbacksService.update` on the `eppc_feedbackid` the create returned (schema §8), + never a second `create`. One row per session, not one per submission. - The success state, then return to the list. **Done when (milestone — run the tests here):** - `tsc` and lint are clean, and the **test run passes** the schema §9 BDD scenarios (rating maps to the integer, submit blocked at draft 0, optional comment omitted when blank, required comment - blocks submit, one card per seeded session). + blocks submit, one card per seeded session, **and revising updates the existing row rather than + creating a second**). +- The suite includes **one un-mocked smoke render** of `App` through the real module graph (mocking + only the `Eppc_*Service` boundary), so a default-vs-named import or import-resolution break is caught + here rather than in the browser console (AGENTS.md Stack notes). +- The create payload satisfies the generated `Eppc_feedbacks` model: rating is the Choice **integer**, + the lookup is `"eppc_SessionId@odata.bind"` (never `_eppc_sessionid_value`), and required system + fields are present (`statecode: 0`). A quick re-run of the Task 1 audit checklist against the write + payload, now that it exists. - Submitting a rating creates a real Feedback row in Dataverse and the card flips to the rated state (manual check on the live app, not part of the mocked suite). - Submit disabled-state labels match the spec (`Select a rating to submit`, `Add a comment to submit`).