diff --git a/apps/api/prisma/migrations/20260822092528_step_tech_step_source/migration.sql b/apps/api/prisma/migrations/20260822092528_step_tech_step_source/migration.sql new file mode 100644 index 0000000..47089e0 --- /dev/null +++ b/apps/api/prisma/migrations/20260822092528_step_tech_step_source/migration.sql @@ -0,0 +1,2 @@ +-- AlterTable +ALTER TABLE "step_tech_step" ADD COLUMN "source" TEXT NOT NULL DEFAULT 'auto'; diff --git a/apps/api/prisma/schema.prisma b/apps/api/prisma/schema.prisma index 3f2c681..4aa57db 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -698,14 +698,27 @@ model Step { /// techniques from scratch (`recipe.service.ts`'s `updateRecipe` deletes /// and recreates every `Step`/`StepTechStep`, never a partial patch) — /// graceful degradation, not a permanent gap. +/// +/// `source` distinguishes a `"manual"` row — written immediately when a +/// user submits a `StepTechStepCorrection` that asserts a technique +/// (`recipe-tech-step-correction.service.ts`'s `applyManualCorrection`), +/// not just recorded as a pending suggestion — from an `"auto"` row the +/// classifier itself produced (`tech-step-matcher.ts`). Both kinds coexist +/// in the same ordered sequence; the detail view (`apps/web`) renders them +/// with a different highlight color so a viewer can tell which is which. +/// `backfillTechSteps` (`scripts/backfill-tech-steps.ts`) only ever +/// deletes/recreates `"auto"` rows — a `"manual"` row survives a +/// classifier/corpus change until a user (or a future moderation feature) +/// explicitly changes it again. model StepTechStep { - stepId Int @map("step_id") - techStepId Int @map("tech_step_id") + stepId Int @map("step_id") + techStepId Int @map("tech_step_id") order Int start Int? end Int? - contextStart Int? @map("context_start") - contextEnd Int? @map("context_end") + contextStart Int? @map("context_start") + contextEnd Int? @map("context_end") + source String @default("auto") step Step @relation(fields: [stepId], references: [id], onDelete: Cascade) techStep TechStep @relation(fields: [techStepId], references: [id], onDelete: Cascade) diff --git a/apps/api/src/modules/recipe/recipe-tech-step-correction.service.ts b/apps/api/src/modules/recipe/recipe-tech-step-correction.service.ts index 9830681..6ae000e 100644 --- a/apps/api/src/modules/recipe/recipe-tech-step-correction.service.ts +++ b/apps/api/src/modules/recipe/recipe-tech-step-correction.service.ts @@ -3,10 +3,11 @@ import { ErrorCode, type StepTechStepCorrectionView, type SubmitTechStepCorrectionInput, + type SubmitTechStepCorrectionResult, } from "@batch-cooking/shared"; import type { Prisma } from "@prisma/client"; import { prisma } from "../../db/prisma.js"; -import { assertRecipeVisible } from "./recipe.service.js"; +import { assertRecipeVisible, toStepTechStepViews } from "./recipe.service.js"; /** * User-submitted corrections to a step's detected techniques @@ -83,6 +84,126 @@ async function assertTechStepsExist(ids: number[]): Promise { } } +/** + * Renumbers every one of `stepId`'s `StepTechStep` rows' `order` by + * ascending `start` (nulls-still-possible legacy rows, see that model's + * schema doc comment, sort last) — the dense, reading-order 0-based + * sequence `@@id([stepId, order])` requires, regardless of whether a + * caller just inserted, updated, or deleted a row. Simpler and less + * error-prone than shifting only the affected neighbors' `order` by hand. + * + * Two passes, through a disjoint negative range first: updating straight + * into the final 0..N-1 positions in one pass risks a transient + * `(stepId, order)` collision (e.g. the row destined for `order: 0` isn't + * necessarily the one already sitting there) — `order` is always `>= 0` + * in real usage, so a negative range can never collide with a live row. + * + * Exported for `scripts/backfill-tech-steps.ts` to reuse after it + * recomputes just the `"auto"` subset of a step's rows, so the combined + * `"auto"` + `"manual"` sequence still ends up in one coherent + * reading-order. + */ +export async function renumberStepTechSteps( + tx: Prisma.TransactionClient, + stepId: number, +): Promise { + const rows = await tx.stepTechStep.findMany({ where: { stepId } }); + const sorted = [...rows].sort( + (a, b) => (a.start ?? Number.POSITIVE_INFINITY) - (b.start ?? Number.POSITIVE_INFINITY), + ); + for (const [index, row] of sorted.entries()) { + await tx.stepTechStep.update({ + where: { stepId_order: { stepId, order: row.order } }, + data: { order: -(index + 1) }, + }); + } + for (const [index] of sorted.entries()) { + await tx.stepTechStep.update({ + where: { stepId_order: { stepId, order: -(index + 1) } }, + data: { order: index }, + }); + } +} + +/** + * Applies a correction's *effect* on `stepId`'s real `StepTechStep` + * sequence, immediately — not just recorded as a pending suggestion for + * `services/tech-step-llm-worker` to eventually process (see + * `StepTechStepCorrection`'s schema doc comment; this is *in addition to* + * that offline feedback loop, not instead of it). `previousTechStepId`/ + * `correctedTechStepId` mean exactly what they do on + * `StepTechStepCorrection` itself (`SubmitTechStepCorrectionInput`'s doc + * comment, `packages/shared`): + * + * - `correctedTechStepId` set (add or relabel): a `"manual"` row is + * written at the correction's own `[start, end)` — updating the + * existing entry in place when one matching `previousTechStepId` + * overlaps this span, otherwise inserting a new one. No `contextStart`/ + * `contextEnd` — a correction only ever carries the tight span the user + * themselves selected/clicked, nothing wider to highlight around it. + * - `previousTechStepId` alone (remove, `correctedTechStepId: null`): the + * matching existing entry is deleted outright. A no-op if none matches + * (nothing to remove). + * + * Runs inside the same transaction {@link submitTechStepCorrection} uses + * for the audit-trail insert, so a request never leaves the two effects + * (the permanent correction record, the live sequence change) only + * partially applied. + */ +async function applyManualCorrection( + tx: Prisma.TransactionClient, + stepId: number, + span: { start: number; end: number }, + previousTechStepId: number | null, + correctedTechStepId: number | null, +): Promise { + const existing = await tx.stepTechStep.findMany({ where: { stepId } }); + + const target = + previousTechStepId !== null + ? existing.find( + (row) => + row.techStepId === previousTechStepId && + row.start !== null && + row.end !== null && + row.start < span.end && + span.start < row.end, + ) + : undefined; + + if (correctedTechStepId !== null) { + if (target) { + await tx.stepTechStep.update({ + where: { stepId_order: { stepId, order: target.order } }, + data: { + techStepId: correctedTechStepId, + start: span.start, + end: span.end, + contextStart: null, + contextEnd: null, + source: "manual", + }, + }); + } else { + const nextOrder = existing.reduce((max, row) => Math.max(max, row.order), -1) + 1; + await tx.stepTechStep.create({ + data: { + stepId, + techStepId: correctedTechStepId, + order: nextOrder, + start: span.start, + end: span.end, + source: "manual", + }, + }); + } + } else if (target) { + await tx.stepTechStep.delete({ where: { stepId_order: { stepId, order: target.order } } }); + } + + await renumberStepTechSteps(tx, stepId); +} + function toCorrectionView(correction: CorrectionWithTechSteps): StepTechStepCorrectionView { return { id: correction.id, @@ -100,10 +221,14 @@ function toCorrectionView(correction: CorrectionWithTechSteps): StepTechStepCorr /** * Records one correction to `stepId`'s detected techniques, submitted by - * `correctorId` — see {@link SubmitTechStepCorrectionInput}'s doc comment - * (`packages/shared`) for what `previousTechStepId`/`correctedTechStepId` - * each mean. Never edited/deleted afterward (see `StepTechStepCorrection`'s - * schema doc comment) — this is a pure insert. + * `correctorId`, and immediately applies its effect to the step's real + * `StepTechStep` sequence (a `"manual"`-tagged row — see + * {@link applyManualCorrection}) — see + * {@link SubmitTechStepCorrectionInput}'s doc comment (`packages/shared`) + * for what `previousTechStepId`/`correctedTechStepId` each mean. The audit + * record itself is never edited/deleted afterward (see + * `StepTechStepCorrection`'s schema doc comment) — only the live sequence + * changes on a later correction to the same span. * * @throws {HttpError} `404 STEP_NOT_FOUND`/`404 RECIPE_NOT_FOUND` — see * {@link loadVisibleStepOrThrow}. `400 INVALID_CORRECTION_SPAN` if @@ -117,7 +242,7 @@ export async function submitTechStepCorrection( input: SubmitTechStepCorrectionInput, correctorId: number, viewerHouseId: number | null, -): Promise { +): Promise { try { const step = await loadVisibleStepOrThrow(recipeId, stepId, correctorId, viewerHouseId); @@ -134,19 +259,37 @@ export async function submitTechStepCorrection( ); await assertTechStepsExist(techStepIds); - const created = await prisma.stepTechStepCorrection.create({ - data: { - stepId: step.id, - correctorId, - start: input.start, - end: input.end, - previousTechStepId: input.previousTechStepId ?? null, - correctedTechStepId: input.correctedTechStepId ?? null, - }, - include: correctionInclude, + const { correction, techSteps } = await prisma.$transaction(async (tx) => { + const createdCorrection = await tx.stepTechStepCorrection.create({ + data: { + stepId: step.id, + correctorId, + start: input.start, + end: input.end, + previousTechStepId: input.previousTechStepId ?? null, + correctedTechStepId: input.correctedTechStepId ?? null, + }, + include: correctionInclude, + }); + + await applyManualCorrection( + tx, + step.id, + { start: input.start, end: input.end }, + input.previousTechStepId ?? null, + input.correctedTechStepId ?? null, + ); + + const freshTechSteps = await tx.stepTechStep.findMany({ + where: { stepId: step.id }, + orderBy: { order: "asc" }, + include: { techStep: true }, + }); + + return { correction: createdCorrection, techSteps: freshTechSteps }; }); - return toCorrectionView(created); + return { correction: toCorrectionView(correction), techSteps: toStepTechStepViews(techSteps) }; } catch (err) { throw err; // see loadVisibleStepOrThrow's catch comment } diff --git a/apps/api/src/modules/recipe/recipe.service.ts b/apps/api/src/modules/recipe/recipe.service.ts index ea6df3b..86d9697 100644 --- a/apps/api/src/modules/recipe/recipe.service.ts +++ b/apps/api/src/modules/recipe/recipe.service.ts @@ -132,18 +132,30 @@ function toRecipeSummaryView(recipe: RecipeWithDetails): RecipeSummaryView { * existed) still has a perfectly good match to show, just without the * wider highlight, so those two are included only when both are present * rather than dropping the whole entry over a still-missing "nice to have". + * + * Exported — also called by `recipe-tech-step-correction.service.ts` to + * shape the fresh `StepTechStep` sequence it returns right after applying + * a manual correction, so both places convert the exact same way rather + * than risking two slightly different views of the same rows. */ -function toStepTechStepViews( +export function toStepTechStepViews( techSteps: RecipeWithDetails["steps"][number]["techSteps"], ): StepTechStepView[] { const views: StepTechStepView[] = []; for (const stepTechStep of techSteps) { - const { start, end, contextStart, contextEnd, techStep } = stepTechStep; + const { start, end, contextStart, contextEnd, techStep, source } = stepTechStep; if (start === null || end === null) continue; views.push({ techStep: { id: techStep.id, key: techStep.key }, start, end, + // `source` is a plain DB `String`, not a Prisma enum (see + // `StepTechStep`'s schema doc comment) — narrowed here rather than + // trusting the column's own type, so a value this app never wrote + // (a manual DB edit, a future migration gone wrong) degrades to the + // safer "auto" reading instead of surfacing an invalid + // `StepTechStepView.source` to the frontend. + source: source === "manual" ? "manual" : "auto", ...(contextStart !== null && contextEnd !== null ? { contextStart, contextEnd } : {}), }); } diff --git a/apps/api/src/modules/sources/sources.service.ts b/apps/api/src/modules/sources/sources.service.ts index 36e8a45..0a93f78 100644 --- a/apps/api/src/modules/sources/sources.service.ts +++ b/apps/api/src/modules/sources/sources.service.ts @@ -225,6 +225,12 @@ export async function previewSourceItem( end: match.end, contextStart: match.contextStart, contextEnd: match.contextEnd, + // A draft preview has no persisted `StepTechStep` row to + // read a real `source` from at all (it isn't a saved + // recipe yet — see `DraftRecipeStepView`'s own doc + // comment) — always the classifier's own live match, + // never a correction, so always "auto". + source: "auto", }, ] : []; diff --git a/apps/api/src/scripts/backfill-tech-steps.ts b/apps/api/src/scripts/backfill-tech-steps.ts index c14e240..5facc3e 100644 --- a/apps/api/src/scripts/backfill-tech-steps.ts +++ b/apps/api/src/scripts/backfill-tech-steps.ts @@ -1,12 +1,15 @@ +import { pathToFileURL } from "node:url"; import { prisma } from "../db/prisma.js"; import { techStepClassifier } from "../lib/recipe-matching/tech-step-matcher.js"; +import { renumberStepTechSteps } from "../modules/recipe/recipe-tech-step-correction.service.js"; /** - * Recomputes every existing `Step`'s `StepTechStep` sequence against the - * *current* classifier (`tech-step-matcher.ts`/`tech-step-training-data.ts`), - * the same way `updateRecipe` does when a user resaves a recipe through the - * UI — always `"fr"` (`DEFAULT_TECH_STEP_LOCALE` in `recipe.service.ts`; - * there's no persisted per-recipe locale to recover for a step that already + * Recomputes every existing `Step`'s `"auto"`-sourced `StepTechStep` + * entries against the *current* classifier + * (`tech-step-matcher.ts`/`tech-step-training-data.ts`), the same way + * `updateRecipe` does when a user resaves a recipe through the UI — + * always `"fr"` (`DEFAULT_TECH_STEP_LOCALE` in `recipe.service.ts`; there's + * no persisted per-recipe locale to recover for a step that already * exists, so this matches real resave behavior exactly rather than * guessing). * @@ -16,13 +19,26 @@ import { techStepClassifier } from "../lib/recipe-matching/tech-step-matcher.js" * `contextStart`/`contextEnd` columns a previous session added) keeps * whatever it was matched with at the time until it's next resaved. * + * `"manual"`-sourced entries (a viewer's correction, applied immediately — + * see `recipe-tech-step-correction.service.ts`'s `applyManualCorrection`) + * are never touched by this: only rows with `source: "auto"` are deleted + * and recreated, and any fresh classifier match overlapping an existing + * `"manual"` entry's span is dropped rather than inserted — a manual + * correction is meant to *override* the classifier at that exact spot, + * and recomputing must never silently reintroduce (or duplicate-highlight) + * what a user already corrected. `renumberStepTechSteps` + * (`recipe-tech-step-correction.service.ts`) folds the surviving `"auto"` + + * untouched `"manual"` rows back into one coherent reading-order sequence + * afterward. + * * Exported (not just called from this file's own CLI guard below) so * `retrain-tech-steps.ts` can run it as one step of its own larger * maintainer workflow, without shelling out to a second process. * - * Safe to re-run: each step's technique sequence is fully replaced (delete - * + recreate) from the classifier's current output, same as a real edit — - * running it twice in a row with no corpus change in between is a no-op. + * Safe to re-run: with no manual entries and no corpus change since the + * last run, this is a no-op (the same `"auto"` matches get deleted and + * recreated identically); with manual entries present, they're preserved + * on every run by construction. */ export async function backfillTechSteps(): Promise<{ total: number; changed: number }> { const steps = await prisma.step.findMany({ select: { id: true, description: true } }); @@ -31,20 +47,47 @@ export async function backfillTechSteps(): Promise<{ total: number; changed: num let changed = 0; for (const step of steps) { const matches = await techStepClassifier.matchTechStepSpans(step.description, "fr"); - await prisma.$transaction([ - prisma.stepTechStep.deleteMany({ where: { stepId: step.id } }), - prisma.stepTechStep.createMany({ - data: matches.map((match, order) => ({ - stepId: step.id, - techStepId: match.techStepId, - order, - start: match.start, - end: match.end, - contextStart: match.contextStart, - contextEnd: match.contextEnd, - })), - }), - ]); + + await prisma.$transaction(async (tx) => { + const manualRows = await tx.stepTechStep.findMany({ + where: { stepId: step.id, source: "manual" }, + }); + + const nonOverlappingMatches = matches.filter( + (match) => + !manualRows.some( + (manual) => + manual.start !== null && + manual.end !== null && + manual.start < match.end && + match.start < manual.end, + ), + ); + + await tx.stepTechStep.deleteMany({ where: { stepId: step.id, source: "auto" } }); + + if (nonOverlappingMatches.length > 0) { + // Placeholder orders, disjoint from the untouched manual rows' + // existing ones (`renumberStepTechSteps` below folds everything + // into a clean 0..N-1 sequence right after — these just need to + // not collide with `@@id([stepId, order])` for this insert). + const startOrder = manualRows.reduce((max, row) => Math.max(max, row.order), -1) + 1; + await tx.stepTechStep.createMany({ + data: nonOverlappingMatches.map((match, index) => ({ + stepId: step.id, + techStepId: match.techStepId, + order: startOrder + index, + start: match.start, + end: match.end, + contextStart: match.contextStart, + contextEnd: match.contextEnd, + source: "auto", + })), + }); + } + + await renumberStepTechSteps(tx, step.id); + }); changed += 1; } @@ -57,8 +100,18 @@ export async function backfillTechSteps(): Promise<{ total: number; changed: num // imports `backfillTechSteps` above — the standard ESM "is this the entry // module" check, first needed in this codebase by that new script; every // prior script here (`seed-runtime.ts`) was always only ever run directly, -// never imported. -const isMainModule = import.meta.url === `file://${process.argv[1]}`; +// never imported. `pathToFileURL` (not a naive `` `file://${process.argv[1]}` `` +// concatenation) is required for this to actually work on Windows — a +// native Windows path (backslashes, no leading slash before the drive +// letter) doesn't survive being pasted directly after `file://`, so the +// comparison against `import.meta.url` (already a real, correctly-escaped +// `file:///D:/...` URL) always came out false: this guard silently never +// matched, so running this script directly (`tsx +// src/scripts/backfill-tech-steps.ts`) did *nothing* — no error, no +// output, `backfillTechSteps()` simply never called — found only by +// running it for real and noticing zero output where several log lines +// were expected. +const isMainModule = import.meta.url === pathToFileURL(process.argv[1] ?? "").href; if (isMainModule) { backfillTechSteps() .then(() => prisma.$disconnect()) diff --git a/apps/api/test/recipe/recipe-tech-step-correction.test.ts b/apps/api/test/recipe/recipe-tech-step-correction.test.ts index 6722510..240c218 100644 --- a/apps/api/test/recipe/recipe-tech-step-correction.test.ts +++ b/apps/api/test/recipe/recipe-tech-step-correction.test.ts @@ -75,8 +75,13 @@ describe("Recipe tech-step corrections", () => { expect(res.body.code).to.equal(ErrorCode.NOT_AUTHENTICATED); }); - it("records a correction adding a missing technique (no previousTechStepId)", async () => { + it("records a correction adding a missing technique (no previousTechStepId), and applies it immediately to the step's own techSteps", async () => { const { agent, profileId } = await signup(); + // "Faire mijoter la sauce." names no technique the classifier itself + // registers a bare-word anchor for at this exact span in isolation + // (see tech-step-training-data.ts) — irrelevant here either way, + // since this test's whole point is the *manual* addition, not + // whatever the classifier does or doesn't auto-detect for it. const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); const simmerId = await techStepId("simmer"); @@ -85,25 +90,59 @@ describe("Recipe tech-step corrections", () => { .send({ start: 6, end: 13, correctedTechStepId: simmerId }); expect(res.status).to.equal(201); - expect(res.body.previousTechStep).to.equal(null); - expect(res.body.correctedTechStep).to.deep.equal({ id: simmerId, key: "simmer" }); - expect(res.body.start).to.equal(6); - expect(res.body.end).to.equal(13); + expect(res.body.correction.previousTechStep).to.equal(null); + expect(res.body.correction.correctedTechStep).to.deep.equal({ id: simmerId, key: "simmer" }); + expect(res.body.correction.start).to.equal(6); + expect(res.body.correction.end).to.equal(13); + // The step's real technique sequence reflects the correction right + // away — not just the permanent audit record above (see + // `applyManualCorrection`, `recipe-tech-step-correction.service.ts`). + expect(res.body.techSteps).to.deep.equal([ + { techStep: { id: simmerId, key: "simmer" }, start: 6, end: 13, source: "manual" }, + ]); }); - it("records a correction relabeling an existing match (both ids set)", async () => { + it("records a correction relabeling an existing match (both ids set), updating the existing techSteps entry in place", async () => { const { agent, profileId } = await signup(); const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); const simmerId = await techStepId("simmer"); const boilId = await techStepId("boil"); + // First correction creates the "manual" entry this test then relabels + // — exercises the UPDATE branch of `applyManualCorrection`, not the + // INSERT one the previous test already covers. + await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, correctedTechStepId: simmerId }); const res = await agent .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) .send({ start: 6, end: 13, previousTechStepId: simmerId, correctedTechStepId: boilId }); expect(res.status).to.equal(201); - expect(res.body.previousTechStep).to.deep.equal({ id: simmerId, key: "simmer" }); - expect(res.body.correctedTechStep).to.deep.equal({ id: boilId, key: "boil" }); + expect(res.body.correction.previousTechStep).to.deep.equal({ id: simmerId, key: "simmer" }); + expect(res.body.correction.correctedTechStep).to.deep.equal({ id: boilId, key: "boil" }); + // Still exactly one entry — the relabel updated the existing row + // rather than adding a second one alongside it. + expect(res.body.techSteps).to.deep.equal([ + { techStep: { id: boilId, key: "boil" }, start: 6, end: 13, source: "manual" }, + ]); + }); + + it("deletes the matching techSteps entry when correctedTechStepId is null (a removal)", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + const simmerId = await techStepId("simmer"); + await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, correctedTechStepId: simmerId }); + + const res = await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, previousTechStepId: simmerId, correctedTechStepId: null }); + + expect(res.status).to.equal(201); + expect(res.body.correction.correctedTechStep).to.equal(null); + expect(res.body.techSteps).to.deep.equal([]); }); it("is not restricted to the recipe's author — any viewer who can see it may correct it", async () => { diff --git a/apps/web/cypress/component/highlight-tech-steps.cy.tsx b/apps/web/cypress/component/highlight-tech-steps.cy.tsx index a581538..7eaa4b1 100644 --- a/apps/web/cypress/component/highlight-tech-steps.cy.tsx +++ b/apps/web/cypress/component/highlight-tech-steps.cy.tsx @@ -6,18 +6,20 @@ import { splitDescriptionByTechSteps } from "../../src/features/recipes/steps/hi // `expect`, not for rendering. `.cy.tsx` (not `.cy.ts`) only because that's // what `cypress.config.ts`'s component `specPattern` looks for. -/** Builds a `StepTechStepView` — `context` omitted entirely (not just undefined) when absent, matching what the API actually sends for an older, not-yet-recomputed match (see `StepTechStepView`'s own doc comment). */ +/** Builds a `StepTechStepView` — `context` omitted entirely (not just undefined) when absent, matching what the API actually sends for an older, not-yet-recomputed match (see `StepTechStepView`'s own doc comment). `source` defaults to `"auto"`, the common case every test not specifically about the manual/auto distinction uses. */ function techStep( key: string, id: number, start: number, end: number, context?: { start: number; end: number }, + source: StepTechStepView["source"] = "auto", ): StepTechStepView { return { techStep: { id, key }, start, end, + source, ...(context ? { contextStart: context.start, contextEnd: context.end } : {}), }; } @@ -25,7 +27,7 @@ function techStep( describe("splitDescriptionByTechSteps", () => { it("returns the whole description as one plain segment when there are no matches", () => { expect(splitDescriptionByTechSteps("Servir immédiatement", [])).to.deep.equal([ - { text: "Servir immédiatement", techStep: null, isKeyword: false }, + { text: "Servir immédiatement", techStep: null, isKeyword: false, source: null }, ]); }); @@ -37,25 +39,25 @@ describe("splitDescriptionByTechSteps", () => { techStep("simmer", 1, 6, 13), ]); expect(result).to.deep.equal([ - { text: "Faire ", techStep: null, isKeyword: false }, - { text: "mijoter", techStep: { id: 1, key: "simmer" }, isKeyword: true }, - { text: " à feu doux", techStep: null, isKeyword: false }, + { text: "Faire ", techStep: null, isKeyword: false, source: null }, + { text: "mijoter", techStep: { id: 1, key: "simmer" }, isKeyword: true, source: "auto" }, + { text: " à feu doux", techStep: null, isKeyword: false, source: null }, ]); }); it("handles a match at the very start, with nothing before it", () => { const result = splitDescriptionByTechSteps("Hacher les oignons", [techStep("chop", 2, 0, 6)]); expect(result).to.deep.equal([ - { text: "Hacher", techStep: { id: 2, key: "chop" }, isKeyword: true }, - { text: " les oignons", techStep: null, isKeyword: false }, + { text: "Hacher", techStep: { id: 2, key: "chop" }, isKeyword: true, source: "auto" }, + { text: " les oignons", techStep: null, isKeyword: false, source: null }, ]); }); it("handles a match at the very end, with nothing after it", () => { const result = splitDescriptionByTechSteps("Faire cuire", [techStep("cook", 3, 6, 11)]); expect(result).to.deep.equal([ - { text: "Faire ", techStep: null, isKeyword: false }, - { text: "cuire", techStep: { id: 3, key: "cook" }, isKeyword: true }, + { text: "Faire ", techStep: null, isKeyword: false, source: null }, + { text: "cuire", techStep: { id: 3, key: "cook" }, isKeyword: true, source: "auto" }, ]); }); @@ -71,6 +73,7 @@ describe("splitDescriptionByTechSteps", () => { text: "Préchauffer", techStep: { id: 4, key: "preheat" }, isKeyword: true, + source: "auto", }); }); @@ -88,17 +91,23 @@ describe("splitDescriptionByTechSteps", () => { it("drops a match whose end is past the end of the description", () => { const result = splitDescriptionByTechSteps("Cuire", [techStep("cook", 3, 0, 999)]); - expect(result).to.deep.equal([{ text: "Cuire", techStep: null, isKeyword: false }]); + expect(result).to.deep.equal([ + { text: "Cuire", techStep: null, isKeyword: false, source: null }, + ]); }); it("drops a match with a negative start", () => { const result = splitDescriptionByTechSteps("Cuire", [techStep("cook", 3, -1, 5)]); - expect(result).to.deep.equal([{ text: "Cuire", techStep: null, isKeyword: false }]); + expect(result).to.deep.equal([ + { text: "Cuire", techStep: null, isKeyword: false, source: null }, + ]); }); it("drops a match whose start isn't before its end", () => { const result = splitDescriptionByTechSteps("Cuire", [techStep("cook", 3, 3, 3)]); - expect(result).to.deep.equal([{ text: "Cuire", techStep: null, isKeyword: false }]); + expect(result).to.deep.equal([ + { text: "Cuire", techStep: null, isKeyword: false, source: null }, + ]); }); it("drops a later match that overlaps one already accepted", () => { @@ -110,7 +119,7 @@ describe("splitDescriptionByTechSteps", () => { techStep("cook", 2, 0, 5), ]); expect(result).to.deep.equal([ - { text: "Cuire au four", techStep: { id: 3, key: "bake" }, isKeyword: true }, + { text: "Cuire au four", techStep: { id: 3, key: "bake" }, isKeyword: true, source: "auto" }, ]); }); @@ -118,6 +127,19 @@ describe("splitDescriptionByTechSteps", () => { expect(splitDescriptionByTechSteps("", [])).to.deep.equal([]); }); + it("carries a manual correction's source through its segments, distinct from an auto match", () => { + const text = "Faire mijoter le riz, puis dresser dans les assiettes"; + const result = splitDescriptionByTechSteps(text, [ + techStep("simmer", 1, 6, 13), + techStep("plate", 2, 28, 35, undefined, "manual"), + ]); + const keywordSegments = result.filter((s) => s.isKeyword); + expect(keywordSegments.map((s) => ({ key: s.techStep?.key, source: s.source }))).to.deep.equal([ + { key: "simmer", source: "auto" }, + { key: "plate", source: "manual" }, + ]); + }); + describe("with a context span wider than the keyword", () => { it("splits into context-before / keyword / context-after around a keyword in the middle of its clause", () => { // The motivating example: "Dans une poêle chaude, faire chauffer une @@ -128,12 +150,23 @@ describe("splitDescriptionByTechSteps", () => { techStep("preheat", 4, 9, 21, { start: 0, end: 21 }), ]); expect(result).to.deep.equal([ - { text: "Dans une ", techStep: { id: 4, key: "preheat" }, isKeyword: false }, - { text: "poêle chaude", techStep: { id: 4, key: "preheat" }, isKeyword: true }, + { + text: "Dans une ", + techStep: { id: 4, key: "preheat" }, + isKeyword: false, + source: "auto", + }, + { + text: "poêle chaude", + techStep: { id: 4, key: "preheat" }, + isKeyword: true, + source: "auto", + }, { text: ", faire chauffer une noix de beurre", techStep: null, isKeyword: false, + source: null, }, ]); }); @@ -143,8 +176,13 @@ describe("splitDescriptionByTechSteps", () => { techStep("preheat", 4, 0, 11, { start: 0, end: 19 }), ]); expect(result).to.deep.equal([ - { text: "préchauffer", techStep: { id: 4, key: "preheat" }, isKeyword: true }, - { text: " le four", techStep: { id: 4, key: "preheat" }, isKeyword: false }, + { + text: "préchauffer", + techStep: { id: 4, key: "preheat" }, + isKeyword: true, + source: "auto", + }, + { text: " le four", techStep: { id: 4, key: "preheat" }, isKeyword: false, source: "auto" }, ]); }); @@ -153,9 +191,19 @@ describe("splitDescriptionByTechSteps", () => { techStep("preheat", 4, 17, 28, { start: 7, end: 28 }), ]); expect(result).to.deep.equal([ - { text: "mettre ", techStep: null, isKeyword: false }, - { text: "le four à ", techStep: { id: 4, key: "preheat" }, isKeyword: false }, - { text: "préchauffer", techStep: { id: 4, key: "preheat" }, isKeyword: true }, + { text: "mettre ", techStep: null, isKeyword: false, source: null }, + { + text: "le four à ", + techStep: { id: 4, key: "preheat" }, + isKeyword: false, + source: "auto", + }, + { + text: "préchauffer", + techStep: { id: 4, key: "preheat" }, + isKeyword: true, + source: "auto", + }, ]); }); @@ -171,7 +219,9 @@ describe("splitDescriptionByTechSteps", () => { // contextEnd (5) is before the keyword's own end (13) — malformed. techStep("bake", 3, 0, 13, { start: 0, end: 5 }), ]); - expect(result).to.deep.equal([{ text: "Cuire au four", techStep: null, isKeyword: false }]); + expect(result).to.deep.equal([ + { text: "Cuire au four", techStep: null, isKeyword: false, source: null }, + ]); }); }); }); diff --git a/apps/web/cypress/e2e/recipes.feature b/apps/web/cypress/e2e/recipes.feature index cb88867..29914e2 100644 --- a/apps/web/cypress/e2e/recipes.feature +++ b/apps/web/cypress/e2e/recipes.feature @@ -27,7 +27,7 @@ Feature: Managing a recipe from the catalog When I focus the highlighted technique "Cuire" Then the tooltip should show "Cuire" - Scenario: Corrects a detected technique from its highlight + Scenario: Corrects a detected technique from its highlight, visible immediately as a manual match Given the recipe catalog contains "Omelette" And recipe 2's detail is available And the tech steps reference list has options @@ -37,7 +37,13 @@ Feature: Managing a recipe from the catalog Then I should see the technique correction options When I choose "Mijoter" as the correct technique Then the correction request should have been made - And I should see "Merci, votre correction a été enregistrée." + # The highlighted *word* stays "Cuire" (a relabel changes which + # technique a span means, not the literal text at that span, still + # "Cuire" in the source description) — now styled as a manual + # correction, with its tooltip naming the newly-assigned technique. + And the highlighted technique "Cuire" should be marked as a manual correction + When I focus the highlighted technique "Cuire" + Then the tooltip should show "Mijoter (correction manuelle)" Scenario: Deletes a recipe after a two-step confirmation, then clears the selection Given the recipe catalog contains "Omelette" diff --git a/apps/web/cypress/e2e/recipes.ts b/apps/web/cypress/e2e/recipes.ts index 5acf3ee..e534164 100644 --- a/apps/web/cypress/e2e/recipes.ts +++ b/apps/web/cypress/e2e/recipes.ts @@ -43,7 +43,7 @@ const omeletteDetail = { // "Cuire" -> the `cook` technique, matching real reference-seed-data.ts // (`\bcui(re|sez|sant|sson)\b`) — "poêle" itself matches nothing // (that's `panFry`'s "sauter", a different word). - techSteps: [{ techStep: { id: 1, key: "cook" }, start: 0, end: 5 }], + techSteps: [{ techStep: { id: 1, key: "cook" }, start: 0, end: 5, source: "auto" }], }, ], }; @@ -70,19 +70,25 @@ Given("deleting recipe 2 will succeed", () => { // Step 2 is `omeletteDetail`'s "Cuire à la poêle." step, whose only // existing match is `cook` (id 1) — see that fixture above. The response -// mirrors `StepTechStepCorrectionView` (packages/shared), reassigning the -// match to `simmer` (id 3, "Mijoter" — see `the tech steps reference list -// has options`, reference-data.steps.ts). +// mirrors `SubmitTechStepCorrectionResult` (packages/shared): the audit +// record (reassigning the match to `simmer`, id 3, "Mijoter" — see `the +// tech steps reference list has options`, reference-data.steps.ts) plus +// the step's fresh `techSteps`, now showing that same reassignment as a +// `"manual"`-sourced entry — the API applies a correction immediately, it +// doesn't just record it (see `StepTechStepView.source`'s own doc comment). Given('correcting step 2\'s "Cuire" match will succeed', () => { cy.intercept("POST", "**/recipes/2/steps/2/corrections", { statusCode: 201, body: { - id: 1, - start: 0, - end: 5, - previousTechStep: { id: 1, key: "cook" }, - correctedTechStep: { id: 3, key: "simmer" }, - createdAt: new Date().toISOString(), + correction: { + id: 1, + start: 0, + end: 5, + previousTechStep: { id: 1, key: "cook" }, + correctedTechStep: { id: 3, key: "simmer" }, + createdAt: new Date().toISOString(), + }, + techSteps: [{ techStep: { id: 3, key: "simmer" }, start: 0, end: 5, source: "manual" }], }, }).as("correction"); }); @@ -99,6 +105,13 @@ When("I choose {string} as the correct technique", (label: string) => { cy.contains(".tech-step-correction-popover__list button", label).click(); }); +Then( + "the highlighted technique {string} should be marked as a manual correction", + (text: string) => { + cy.contains(".step-tech-step", text).should("have.class", "step-tech-step--manual"); + }, +); + Then("the correction request should have been made", () => { // Asserts the actual span, not just that *a* request fired — a real bug // (StepDescription.tsx's click handler reading a shared, still-mutating diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index 67b23b0..9cc6b9a 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -21,6 +21,7 @@ import { type SourceView, type StepTechStepCorrectionView, type SubmitTechStepCorrectionInput, + type SubmitTechStepCorrectionResult, type TechStepView, type ThemePreference, type UnitView, @@ -275,12 +276,12 @@ export class ApiClient { return this._request(`/recipes/${id}/favorite`, { method: "DELETE" }); } - /** Submits a correction to one of `stepId`'s detected techniques — see `SubmitTechStepCorrectionInput`'s doc comment (`packages/shared`) for what `previousTechStepId`/`correctedTechStepId` each mean. Open to any viewer who can see the recipe, not just its author. */ + /** Submits a correction to one of `stepId`'s detected techniques — see `SubmitTechStepCorrectionInput`'s doc comment (`packages/shared`) for what `previousTechStepId`/`correctedTechStepId` each mean. Open to any viewer who can see the recipe, not just its author. The response's `techSteps` is the step's fresh, immediately up-to-date technique sequence — see `SubmitTechStepCorrectionResult`'s doc comment. */ public submitTechStepCorrection( recipeId: number, stepId: number, input: SubmitTechStepCorrectionInput, - ): Promise { + ): Promise { return this._request(`/recipes/${recipeId}/steps/${stepId}/corrections`, { method: "POST", body: JSON.stringify(input), diff --git a/apps/web/src/features/recipes/RecipeDetailPanel.tsx b/apps/web/src/features/recipes/RecipeDetailPanel.tsx index 9ef04b9..47324e1 100644 --- a/apps/web/src/features/recipes/RecipeDetailPanel.tsx +++ b/apps/web/src/features/recipes/RecipeDetailPanel.tsx @@ -205,6 +205,16 @@ export function RecipeDetailPanel({

{t("recipes.stepsTitle")}

+ {/* Discoverability hint for the highlight/correction feature below + — nothing about the steps list itself otherwise signals that a + highlighted technique or a plain-text selection is interactive. + Tied to `showActions`, same reasoning as `StepDescription`'s own + `editable` prop right below. */} + {showActions && ( +

+ {t("recipes.techStepCorrection.discoverabilityHint")} +

+ )}
    {recipe.steps.map((step) => (
  1. diff --git a/apps/web/src/features/recipes/recipes.scss b/apps/web/src/features/recipes/recipes.scss index 9689e52..a9ed09d 100644 --- a/apps/web/src/features/recipes/recipes.scss +++ b/apps/web/src/features/recipes/recipes.scss @@ -627,12 +627,35 @@ } } +// A `"manual"`-sourced match (a viewer's correction, applied immediately — +// see `StepTechStepView.source`) — same shape as `.step-tech-step`, but in +// `--color-tag` (Turmeric) instead of `--color-primary` (Basil), so the two +// origins are distinguishable at a glance, not just via the tooltip text. +.step-tech-step--manual { + background: color-mix(in srgb, var(--color-tag) 18%, transparent); + text-decoration-color: var(--color-tag); + + &:hover, + &:focus-visible { + background: color-mix(in srgb, var(--color-tag) 28%, transparent); + } +} + // `.step-tech-step-context` (the wider clause a `.step-tech-step` keyword // was found in) used to be highlighted here too, more subtly — turned back // off (see `StepDescription.tsx`'s doc comment): the backend still // computes and persists `contextStart`/`contextEnd`, this file just no // longer gives that class any styling to render with. +// Discoverability hint above the steps list (RecipeDetailPanel.tsx) — +// muted so it reads as a small aside, not competing with the steps +// themselves for attention. +.recipe-detail-panel__tech-step-hint { + margin: 0 0 var(--space-sm); + font-size: var(--font-size-sm); + color: var(--color-text-muted); +} + // --- Tech-step correction (StepDescription.tsx editable mode) --------------- // Deliberately *not* `position: absolute` (unlike `.calendar-popover`) — see @@ -699,12 +722,6 @@ } } -.step-tech-step-correction-confirmation { - margin-top: var(--space-xs); - font-size: var(--font-size-sm); - color: var(--color-success, var(--color-primary)); -} - // --- Favorite star toggle (detail panel header) ----------------------------- .favorite-star-button { position: absolute; diff --git a/apps/web/src/features/recipes/steps/StepDescription.tsx b/apps/web/src/features/recipes/steps/StepDescription.tsx index b37100a..042478f 100644 --- a/apps/web/src/features/recipes/steps/StepDescription.tsx +++ b/apps/web/src/features/recipes/steps/StepDescription.tsx @@ -1,14 +1,11 @@ -import type { StepTechStepCorrectionView, StepTechStepView } from "@batch-cooking/shared"; -import { Fragment, useRef, useState } from "react"; +import type { StepTechStepView, SubmitTechStepCorrectionResult } from "@batch-cooking/shared"; +import { Fragment, useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { Tooltip } from "../../../components/ui/Tooltip"; import { splitDescriptionByTechSteps } from "./highlight-tech-steps"; import { TechStepCorrectionPopover } from "./TechStepCorrectionPopover"; import { type TextSelectionRange, useTextSelection } from "./use-text-selection"; -/** How long the post-submit confirmation message stays visible — long enough to read, short enough not to linger once the user has moved on. */ -const CONFIRMATION_DISPLAY_MS = 4000; - /** * A recipe step's description, with every detected technique's exact * matched words highlighted and given a {@link Tooltip} naming the @@ -28,7 +25,11 @@ const CONFIRMATION_DISPLAY_MS = 4000; * * `techStep.key` resolves its tooltip label through `catalog.techSteps.` * i18n, the same pattern every other reference catalog (diets, units, …) - * uses for its display text. + * uses for its display text. A keyword's `source` (`"auto"` — the + * classifier — vs `"manual"` — a viewer's correction, applied immediately) + * gets its own modifier class (`.step-tech-step--manual`), a different + * color, so the two are visually distinguishable at a glance rather than + * only via the tooltip text. * * `editable` (off by default) additionally lets the viewer select text or * click an existing highlight to open a {@link TechStepCorrectionPopover} — @@ -37,6 +38,15 @@ const CONFIRMATION_DISPLAY_MS = 4000; * renders exactly as before (no extra wrapping elements, no `data-offset`, * no click handlers) — this mode is purely additive, not a rewrite of the * read-only rendering. + * + * Maintains its own local copy of `techSteps` (seeded from the prop, then + * replaced with whatever `POST .../corrections` returns on a successful + * submit — see `SubmitTechStepCorrectionResult`'s doc comment, + * `packages/shared`) so a correction's effect (a new/relabeled/removed + * highlight) appears immediately, without needing the parent to re-fetch + * the whole recipe. Resynced whenever the `techSteps` prop itself changes + * (e.g. the parent reloaded the recipe for an unrelated reason) so this + * never keeps showing stale local state past that. */ export function StepDescription({ description, @@ -53,7 +63,10 @@ export function StepDescription({ stepId?: number; }) { const { t } = useTranslation(); - const segments = splitDescriptionByTechSteps(description, techSteps); + const [liveTechSteps, setLiveTechSteps] = useState(techSteps); + useEffect(() => setLiveTechSteps(techSteps), [techSteps]); + + const segments = splitDescriptionByTechSteps(description, liveTechSteps); const containerRef = useRef(null); const { getSelectionRange } = useTextSelection(containerRef); @@ -62,7 +75,6 @@ export function StepDescription({ selectedText: string; previousTechStepId: number | null; } | null>(null); - const [showConfirmation, setShowConfirmation] = useState(false); function handleMouseUp() { if (!editable) return; @@ -75,9 +87,8 @@ export function StepDescription({ }); } - function handleSubmitted(_correction: StepTechStepCorrectionView) { - setShowConfirmation(true); - window.setTimeout(() => setShowConfirmation(false), CONFIRMATION_DISPLAY_MS); + function handleSubmitted(result: SubmitTechStepCorrectionResult) { + setLiveTechSteps(result.techSteps); } // Tracks each segment's own absolute start offset into `description` as @@ -103,8 +114,9 @@ export function StepDescription({ // A segment's own text/techStep don't uniquely identify it (the // same word can appear twice in one description) — index is the // only thing that does, but this list is fully regenerated from - // `description`/`techSteps` on every render (never reordered or - // spliced in place), so using it as part of the key is safe here. + // `description`/`liveTechSteps` on every render (never reordered + // or spliced in place), so using it as part of the key is safe + // here. const key = `${index}-${segment.text}`; if (!segment.techStep || !segment.isKeyword) { @@ -123,8 +135,14 @@ export function StepDescription({ } const techStep = segment.techStep; + const isManual = segment.source === "manual"; + const tooltipLabel = isManual + ? t("recipes.techStepCorrection.manualTooltip", { + technique: t(`catalog.techSteps.${techStep.key}`), + }) + : t(`catalog.techSteps.${techStep.key}`); return ( - + {/* A real