diff --git a/.claude/skills/check/SKILL.md b/.claude/skills/check/SKILL.md index 16d2f26..d79492b 100644 --- a/.claude/skills/check/SKILL.md +++ b/.claude/skills/check/SKILL.md @@ -9,8 +9,11 @@ Run checks from the repository root. This is a pnpm 10/Turborepo workspace; do not substitute `npm`, `npx`, or a bare `tsc` command. 1. Inspect `git status --short` first and leave unrelated work untouched. -2. Run `pnpm check`. It is the normal fast gate and runs the workspace build, - type generation/type-checking, and unit tests. +2. Run `pnpm check`. It is the normal fast gate: `check-style`, `lint`, the + workspace build, type generation/type-checking, and unit tests, in that + order. `pnpm lint` also enforces the layering, so a boundary violation + fails here rather than in review; `pnpm lint --fix` settles the + formatting-shaped rules. 3. Run `pnpm audit` as a separate dependency-health check. Report an audit failure as a finding; do not change dependencies unless the user asks. 4. Run `pnpm test:e2e` when the user requests full verification, the change diff --git a/.claude/skills/review-code/SKILL.md b/.claude/skills/review-code/SKILL.md index 7cfbeb4..ce32677 100644 --- a/.claude/skills/review-code/SKILL.md +++ b/.claude/skills/review-code/SKILL.md @@ -10,31 +10,69 @@ report objective, actionable findings ordered by severity. Do not make fixes. Use the repository's real gates where useful: +- `pnpm lint` for the style rules and the layering. - `pnpm check-types` for workspace type-checking. - `pnpm test` for unit tests. - `pnpm audit` for dependency health. - `pnpm test:e2e` when the review includes browser behavior or a user-facing flow. Do not run the Docker build unless the reviewed change affects it. -There is no configured lint or coverage script. State either as unavailable -rather than substituting a different tool or treating its absence as a passing -result. +There is no configured coverage script. State that as unavailable rather than +substituting a different tool or treating its absence as a passing result. -Review against this architecture: +## What is already proven, and what is left for you -- Browser code in `apps/dfm/app/` calls only app-owned `/api/*` endpoints. -- `apps/dfm/server/` is the only layer that handles the Toolpath SDK or API - key. Validate client input and upstream responses at this boundary. -- API keys and raw presigned/artifact URLs must not be logged, persisted, or - exposed to the browser. CAD uploads should stay direct to the presigned URL. +`pnpm lint` fails on any of these, so do not spend the review restating them. +Run it, and if it is clean, say so and move on: + +- browser code under `app/` importing `server/`, by alias or by relative path; +- a runtime (non-type) Toolpath SDK import outside `apps/dfm/server`; +- `app/shared/` importing any other layer; +- the style rules listed in the AGENTS.md table. + +The review is for what no check can see: + +- Browser code calls only app-owned `/api/*` endpoints, and the server validates + client input and upstream responses at that boundary. +- API keys and raw presigned/artifact URLs are not logged, persisted, or handed + to the browser. CAD uploads stay direct to the presigned URL. - Part analysis and feature enrichment are asynchronous jobs; route behavior - must correctly handle queued, running, succeeded, and failed states. -- Put pure client-domain logic in `app/shared/` and cover it with focused - tests. Keep React components and Hono handlers as thin integration layers. + handles queued, running, succeeded, and failed correctly. +- Pure client-domain logic sits in `app/shared/` with focused tests, and React + components and Hono handlers stay thin. + +## Blast radius + +For the change under review, ask what a plausible next requirement in the same +area would cost. If a single change of intent forces edits across several +modules, the finding is the coupling, not the edit — name the specific import or +shared shape that causes the ripple, and where the seam should be. + +Some files already amplify that cost. Get the current set rather than trusting a +list — a typed table goes stale within a few commits: + +```sh +find apps/dfm/app -name '*.ts' -o -name '*.tsx' | grep -v '\.test\.' \ + | xargs wc -l | sort -rn | sed -n '2,8p' +``` + +`part-inspector.tsx`, `map-features.tsx`, `metrics.ts`, `rules.ts`, +`best-reading.ts`, and `rule-editor.tsx` have held the top of that list; check +whether the diff moved any of them. Size alone is not a finding. Growth in one +of them, in a change that had a seam available, is. + +## Guides and sensors + +When a finding is a rule that a command could prove rather than a judgment about +this diff, say so and name the check that would prove it — a rule in +`eslint.config.js`, a case in `scripts/check-style.mjs`, or a test. A convention +repeated in review is a convention that will be violated again. + +## Reporting Also flag demonstrated duplication, missing error handling, unbounded work, large response payloads, excessive rerenders, and unnecessary client-bundle -growth. Do not make speculative refactor suggestions. For every finding give -the exact path, evidence, impact, and a concrete remediation direction; note -when a safe refactor needs tests first. End with a prioritized plan only when -findings justify one. +growth. Do not make speculative refactor suggestions. For every finding give the +exact path, evidence, impact, and a concrete remediation direction; note when a +safe refactor needs tests first. End with a prioritized plan only when findings +justify one. diff --git a/.claude/skills/review-testing/SKILL.md b/.claude/skills/review-testing/SKILL.md index e9b2bd1..f37e38c 100644 --- a/.claude/skills/review-testing/SKILL.md +++ b/.claude/skills/review-testing/SKILL.md @@ -18,10 +18,21 @@ counting test files. Use this repository's actual test layout and commands: and builds the production Docker image on pull requests and `main` pushes. Run the narrowest relevant command(s), plus `pnpm check-types` when type errors -could hide test failures. Coverage tooling is not configured; report that fact -plainly but do not install it or treat a numeric target as the goal. +could hide test failures, and `pnpm lint` — it is the structural test layer, and +it proves the layering that no unit test asserts. Coverage tooling is not +configured; report that fact plainly but do not install it or treat a numeric +target as the goal. -Prioritize missing behavior at these seams: +## Where a test belongs + +`apps/dfm/docs/README.md` § Testing settles this, its reasons are in +`docs/directions-replay.md`, and the `setup-testing` skill restates it for +writing. Read one of those rather than working from instinct, and audit against +it: a click-on-part test outside `tests/on-the-part.spec.ts`, or any test built +on captured Engine JSON, is a finding on placement alone regardless of what it +asserts. + +## Behavioral seams to prioritize - session-cookie and API-key handling, including redaction; - Zod validation and app-owned API response contracts; @@ -30,9 +41,28 @@ Prioritize missing behavior at these seams: - pure rule, scoring, selection, measurement, and report transformations; - representative UI interactions and the end-to-end part-analysis flow. +## Sensor coverage + +Separately from missing tests, report which behaviors and which AGENTS.md rules +have no automated proof at all — nothing in Vitest, Playwright, `eslint.config.js`, +or `scripts/check-style.mjs` that would fail if they were broken. Naming that set +is itself the finding, because those are the rules a long session drifts off +first. The AGENTS.md style table marks its own unproven rules as judgment; check +whether that list is still honest, in both directions — a rule marked judgment +that now has a sensor is as stale as one claiming a sensor it lacks. + +Two tests are sensors rather than coverage, and are audited as rules: + +- `app/styles.test.ts` — cascade-layer ordering and light/dark role parity. +- `app/kit-usage.test.ts` — a ratchet holding raw ` + + + {/* + Its own way out, not the toggle's. + + Done used to call the same handler the Create button does, which + *toggles* — and with the draft already put down by the confirm, it + started a fresh drawing instead of leaving. Finishing and starting + again are two things. + */} + + + + + ) + } + + return ( +
onHoverFace(null)}> + +

+ Press an arrow on the part, or pick one here. +

+
+ {directions.map((direction, index) => ( + + ))} +
+ + + 0}> + {draft.direction === null ? ( +

Choose a way up first.

+ ) : ( + <> +
+ {/* + Chaining off by default: on it, a stray click adds a run rather + than a face — a bigger mistake to notice and a bigger one to + undo. + */} + + {/* + A profile *is* the boundary contour of its direction — that is + what the Engine means by the word — so this reads its own + contours rather than walking the part. + */} + + {draft.faces.length > 0 ? ( + + ) : null} +
+

+ {draft.chaining + ? 'Click a face, then another, to take the run between them.' + : 'Click faces on the part to add them, and again to take them off.'} + {perimeter.length === 0 && draft.faces.length === 0 ? ( + // Said rather than left as a grey button: a control nobody can + // press and nobody can find out why is one that reads as broken. + <> + {' '} + Profile follows the surface a chosen face + sits in, so choose one first. + + ) : null} +

+ {/* + A running list, because "four faces are chosen" is not something + anybody can check against the part — the far side of it is not on + screen, and a face behind another cannot be counted at all. + */} + {/* + A feature is one continuous piece of geometry: an operation runs + over faces that touch, and a reading drawn from two unconnected + groups is one no toolpath could follow. Said with the count of + pieces rather than a bare refusal, so somebody knows which face to + take off. + */} + {runs.length > 1 ? ( +

+ These are {runs.length} separate pieces —{' '} + {runs.map((run) => run.length).join(' and ')} faces. A feature has to be one. +

+ ) : null} + {draft.faces.length === 0 ? ( +

+ No faces yet. +

+ ) : ( + + )} + + )} + + + + {/* + Guessed from the faces, and it keeps guessing as they change — a type + filled in from three faces should not stick once there are five. + Naming one stops it, because disagreeing with the guess is the reason + the field is editable at all. + */} + + {guesses.length === 0 ? null : ( +

+ {draft.named ? 'These faces read as' : 'Guessed —'}{' '} + {guesses.slice(0, 3).map((guess, at) => ( + + {at > 0 ? ', ' : ''} + {' '} + + ({guess.faces} of {draft.faces.length}) + + + ))} + . +

+ )} + + + {/* + The useful half. + + Most of the time the Engine has already reported what somebody is about + to draw, and mapping the reported one is better than making a second + reading of the same geometry. So these are offered, and the list going + empty is the signal that this really is new. + */} + {draft.faces.length === 0 ? null : ( + + {/* + Faces already being machined, which is the more urgent half. + + "Nothing covers all of these, this is new" answers a question about + the **shape** — is the Engine already describing it. It says nothing + about the **plan**, and a face already cut from somewhere is one this + reading is about to take: cut once means the press that maps this + takes it off whatever holds it now. Somebody drawing over a mapped + wall should be told before, not find out from a coverage figure + afterwards. + */} + {cutNow.size === 0 ? null : ( +

+ {cutNow.size} of these {cutNow.size === 1 ? 'faces is' : 'faces are'} already machined + — {[...cutNow.values()].join(', ')}. Mapping this takes{' '} + {cutNow.size === 1 ? 'it' : 'them'} off whatever cuts{' '} + {cutNow.size === 1 ? 'it' : 'them'} now. +

+ )} + {already.length === 0 ? ( +

+ Nothing covers all {draft.faces.length} of these. This is new. +

+ ) : ( + <> + {/* + Which way up they are cut from, because it changes the advice + entirely. A reading covering the same faces from the **other + side of the part** is not the same operation, and offering it as + one is the panel giving bad advice. + */} +

+ {already.length} reading{already.length === 1 ? '' : 's'} already cover + {already.length === 1 ? 's' : ''} all of these + {sameWayUp.length > 0 + ? ` — ${String(sameWayUp.length)} from this way up, so map one instead of drawing a second.` + : '. None from this way up, so a made reading may still be the right answer.'} +

+ + + )} + + )} + + {/* + Said while it is being drawn, not asked for afterwards. + + A reading is only half of a decision, and somebody drawing one already + knows what they mean to do with it — making them say it again in another + panel is asking the same question twice. Left unset it is made and not + yet mapped, which is a real state and the one the next screen offers to + fix. + */} +
+ Cut it + + onDraft({ + ...draft, + // The same three presses everywhere else: empty means take it off + // both, and pressing what it already holds is how that is said. + passes: + passes.length === 0 + ? [] + : passes.every((pass) => draft.passes.includes(pass)) + ? draft.passes.filter((pass) => !passes.includes(pass)) + : [...new Set([...draft.passes, ...passes])], + }) + } + /> + + + + +
+
+ ) +} diff --git a/apps/dfm/app/components/cut-from.tsx b/apps/dfm/app/components/cut-from.tsx new file mode 100644 index 0000000..ddb4c03 --- /dev/null +++ b/apps/dfm/app/components/cut-from.tsx @@ -0,0 +1,61 @@ +import { directionCss } from 'shared/direction-colors' +import type { Vec3 } from '@toolpath/api' +import { directionLabel } from 'shared/report' + +/** + * The other ways up a made reading could be cut from. + * + * Drawing one is two decisions — which faces, and from where — and the second + * is the one somebody changes their mind about: the faces are a fact about the + * part, the way up is a choice about the setup. Redrawing the faces to change + * it is asking them to redo the half that was right. + * + * Only made readings get this. A reported one is the Engine's answer to "what + * is cuttable from here", and pointing it elsewhere would be inventing an + * answer it never gave. + */ +export const CutFrom = ({ + directions, + current, + onCutFrom, +}: { + directions: ReadonlyArray + /** The way up it is cut from now, so that one reads as held rather than offered. */ + current: Vec3 + onCutFrom: (direction: number) => void +}) => { + const here = directionLabel(current) + + return ( +
+ + Cut from + + {directions.map((direction, index) => { + const holds = directionLabel(direction) === here + + return ( + + ) + })} +
+ ) +} diff --git a/apps/dfm/app/components/face-count.tsx b/apps/dfm/app/components/face-count.tsx new file mode 100644 index 0000000..45dce5e --- /dev/null +++ b/apps/dfm/app/components/face-count.tsx @@ -0,0 +1,89 @@ +/** + * How many faces a reading has, and a way into them. + * + * A control, not a caption. A face is what a plan is made of — cut once, + * counted by coverage, taken by a claim — so the count is the doorway to the + * level below, and it says so by looking pressable. + * + * One component because it appears in three places that must agree: the mapping + * lists, the confirmed directions, and the datasheet. Three copies of a number + * that means "how much of this is being cut" is three chances for one of them + * to still be counting what the reading *covers*. + */ + +/** The pencil. A picture of the verb, so the row does not have to spell it. */ +const Pencil = () => ( + +) + +export const FaceCount = ({ + faces, + cut, + onShow, +}: { + /** Every face the reading covers. */ + faces: number + /** How many of them it is cutting in the pass being shown. */ + cut: number + onShow: () => void +}) => { + const whole = cut === faces + + /* + * The words the button used to wear, moved into its name. + * + * It read `Edit Feature (14 regions)` on every row of a list where every row + * has one, which is eleven characters of the same sentence repeated down the + * panel — and on a part with long feature names it was the widest thing in + * the row. The pencil says *edit* and the number says *how many*; nobody + * needed to be told the number counts regions when the thing it opens is a + * list of them. + * + * An `aria-label` is right here where it was wrong before: the visible label + * is now a picture and a figure, so there is no text for it to disagree with + * — and "(14)" on its own is not a control anybody could name. + */ + const said = whole + ? `Edit feature, ${String(faces)} regions` + : `Edit feature, cutting ${String(cut)} of its ${String(faces)} regions — ${String( + faces - cut, + )} went to another reading` + + return ( + + ) +} diff --git a/apps/dfm/app/components/face-list.test.tsx b/apps/dfm/app/components/face-list.test.tsx new file mode 100644 index 0000000..26e1c9f --- /dev/null +++ b/apps/dfm/app/components/face-list.test.tsx @@ -0,0 +1,572 @@ +// @vitest-environment jsdom +import { cleanup, fireEvent, render, screen, within } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { FaceList } from './face-list' +import { PartViewProvider, type PartView } from './part-view' +import { EMPTY_PLAN, PASSES, type SetupPlan } from 'shared/setups' +import { setFaceCut } from 'shared/faces' +import { setPassFor } from 'shared/plan-actions' +import { TEST_DIRECTIONS, testFeature, testReport } from 'shared/test-part' + +/** + * The face editor. The clicks it arms happen on a mesh no fixture mounts (F51), + * so what the part does with them is pinned in `faces.test.ts` and what the + * panel says about them is pinned here. + */ +afterEach(cleanup) + +const profile = testFeature('profile', 'profile', TEST_DIRECTIONS[0]!, [0, 1]) +const wall = testFeature('wall', 'wall', TEST_DIRECTIONS[1]!, [2]) +const features = [profile, wall] +const report = testReport(features) + +/** What every FaceList in here is looking at, with the plan under test in it. */ +const view = (plan: SetupPlan): PartView => ({ + part: report, + directions: TEST_DIRECTIONS, + plan, + scores: new Map(), + verdicts: [], + unit: 'mm', + showingPass: 'rough', +}) + +const editor = (plan: SetupPlan = EMPTY_PLAN) => { + const onSetFace = vi.fn() + const onSelectAll = vi.fn() + const onSelectFree = vi.fn() + const onRetype = vi.fn() + const onHoverFace = vi.fn() + + const panel = (shown: SetupPlan) => ( + + + + ) + + const { rerender } = render(panel(plan)) + + return { + onSetFace, + onSelectAll, + onSelectFree, + onRetype, + onHoverFace, + rerender: (next: SetupPlan) => rerender(panel(next)), + } +} + +/** Face 2 handed to the profile, which the Engine never reported there. */ +const handed = setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, PASSES, 2, true) + +describe('adding a face to a reading', () => { + it('says what a click on the part will do, because that is where it is done', () => { + // Nothing to arm: in here a click has only one meaning. + editor() + + expect(screen.queryByRole('button', { name: 'Add a face' })).not.toBeInTheDocument() + expect(screen.getByRole('group', { name: 'Clicking a face' })).toBeInTheDocument() + expect(screen.getByText(/Puts it in or takes it out/)).toBeInTheDocument() + }) + + it('lists the handed face, so there is a row to take it back off', () => { + editor(handed) + + expect(screen.getByRole('checkbox', { name: /face 2 /i })).toBeInTheDocument() + }) + + it('marks it as added, because an unmarked row would read as the Engine own answer', () => { + editor(handed) + + expect(screen.getByText('added')).toBeInTheDocument() + }) + + it('counts it among the reading faces', () => { + // One of the profile's own two is cut, plus the one handed to it. + editor(handed) + + expect(screen.getByText('1 of 3 faces')).toBeInTheDocument() + }) +}) + +describe('what the list highlights', () => { + /* + * A highlight that is on everything points at nothing. Cut rows used to carry + * a fill of their own, and on a reading whose faces are all cut — which is + * most of them — that is every line lit the moment it opens. + */ + const rowFor = (label: RegExp) => + screen.getByRole('checkbox', { name: label }).closest('div')?.className ?? '' + + it('leaves a cut face unfilled, because the tick already says it is cut', () => { + editor(setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, PASSES, 0, true)) + + expect(screen.getByRole('checkbox', { name: /face 0 /i, checked: true })).toBeInTheDocument() + expect(rowFor(/face 0 /i)).not.toContain('bg-info') + }) + + it('fills the one being worked on, which is what the list cannot otherwise show', () => { + editor() + fireEvent.click(screen.getByRole('button', { name: /Show what else covers face 1/ })) + + expect(rowFor(/face 1 /i)).toContain('bg-info/20') + expect(rowFor(/face 0 /i)).not.toContain('bg-info') + }) +}) + +describe('what the tick says', () => { + /** Finished from its own way up, and roughed nowhere. */ + const finishedOnly: SetupPlan = { + setups: [{ id: 'a', directionIndex: 0, name: '+Z' }], + assigned: { profile: { finish: 'a' } }, + } + + it('is ticked for a face this reading finishes, with roughing on screen', () => { + /* + * Paul's screenshot: "0 of 12 faces" above an expanded row showing that + * very reading with F lit. The tick writes both passes and was reading one. + */ + editor(finishedOnly) + + expect(screen.getByRole('checkbox', { name: /face 0 /i })).toHaveProperty('checked', true) + expect(screen.getByText('2 faces')).toBeInTheDocument() + }) + + it('reads mixed when only one pass holds it, like the pass buttons do', () => { + editor(finishedOnly) + + expect(screen.getByRole('checkbox', { name: /face 0 /i })).toHaveProperty('indeterminate', true) + }) + + it('fills a half-cut face up rather than emptying it', () => { + // Pressing a dashed control takes the rest back — the rule R, F and Both + // already follow. + const { onSetFace } = editor(finishedOnly) + + fireEvent.click(screen.getByRole('checkbox', { name: /face 0 /i })) + expect(onSetFace).toHaveBeenCalledWith(profile, 0, true) + }) +}) + +describe('the order the faces come in', () => { + const named = () => screen.getAllByRole('checkbox').map((box) => box.getAttribute('aria-label')) + + /* + * Scoped to the list, because the four names now appear twice on purpose. + * + * The headings say what this reading *has*; the key beside the switch says + * what the colours *mean*. Asking the page finds both, and the two are + * answering different questions. + */ + const heading = (label: string) => + within(screen.getByRole('list', { name: 'Faces' })).queryByText(label) + + it('groups them by what the plan does with each', () => { + /* + * The question the panel is opened with. A face roughed here and finished + * from the other side costs a second setup, and that fact was spread + * through a column of rows for the eye to gather. + */ + const roughed = setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, ['rough'], 1, true) + editor(roughed) + + expect(heading('Roughed only')).toBeInTheDocument() + expect(heading('Not cut here')).toBeInTheDocument() + }) + + it('puts the faces it cuts above the ones it does not', () => { + editor(setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, PASSES, 1, true)) + + expect(named()?.[0]).toMatch(/face 1 /i) + }) + + it('names no group it has no faces for', () => { + // An empty heading is a claim about the reading that is not true of it — + // which is a different statement from the colour key, where the same four + // words mean "this is what green would mean". + editor() + + expect(heading('Roughed only')).not.toBeInTheDocument() + expect(heading('Finished only')).not.toBeInTheDocument() + }) + + it('is the key to the part as well as a list', () => { + // The headings carry the swatch the model is painted in, so a row's group + // says what colour that face is wearing without a second lookup. + editor(setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, PASSES, 1, true)) + + expect(heading('Roughed and finished')).toBeInTheDocument() + }) +}) + +describe('a face handed to the reading being edited', () => { + /* + * Paul's case: a wall the Engine sees only from one direction, added to a + * group it sees from the other. Adding it means "this face is part of the + * feature I am editing", not "enable the reading the Engine reported". + */ + const handed = setFaceCut(EMPTY_PLAN, TEST_DIRECTIONS, features, profile, PASSES, 2, true) + + it('lists the reading being edited among the face readings, and says which it is', () => { + /* + * The Engine's own list is `regionIdxs`, and an added face is by definition + * not in it — so the row opened onto a list not containing the reading the + * face had just been added to. The only row was the other direction, with + * its passes off, and pressing it enabled the face there instead. + */ + editor(handed) + fireEvent.click(screen.getByRole('button', { name: /Show what else covers face 2/ })) + + expect(screen.getByText('this one')).toBeInTheDocument() + }) +}) + +describe('clicking a face row', () => { + it('opens it, rather than taking the face out of the reading', () => { + /* + * The whole row used to be a `