diff --git a/.env.example b/.env.example index d17785f..cf00a63 100644 --- a/.env.example +++ b/.env.example @@ -21,3 +21,15 @@ JWT_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars # over plain HTTP a Secure cookie is silently never sent back by the # browser, so login "succeeds" but every subsequent request 401s. # COOKIE_SECURE=false + +# Only needed to run the optional `tech-step-llm-worker` service — shared +# between it and "app" (docker-compose.yml). Generate your own the same +# way as JWT_SECRET above; leave both this and the service commented +# out/unset to run without it. +# INTERNAL_WORKER_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars + +# Optional — cron expression (node-cron syntax) the worker wakes up on to +# run its audit/feedback-loop jobs. Default: weekly, Sunday 03:00 — a +# provisional floor, not a calibrated value (see +# services/tech-step-llm-worker/README.md). +# TECH_STEP_WORKER_CRON=0 3 * * 0 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f17379c..919405d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,6 +15,10 @@ env: DATABASE_URL: "postgresql://ci:ci@localhost:5432/batchcooking_ci?schema=public" # Test-only secret, never used outside CI — real deployments must set their own. JWT_SECRET: "ci-only-secret-not-used-anywhere-else-32chars+" + # Same reasoning as JWT_SECRET above — lets tech-step-worker.routes.test.ts + # exercise the success path (matching secret), not just the "unset" + # rejection every environment that doesn't set this gets by default. + INTERNAL_WORKER_SECRET: "ci-only-worker-secret-not-used-anywhere-else-32chars+" jobs: # Four independent jobs, no needs: between them — each starts in parallel diff --git a/.gitignore b/.gitignore index 93666d2..3507f9b 100644 --- a/.gitignore +++ b/.gitignore @@ -157,3 +157,8 @@ tmp-mockups/ # IA .claude/ + +# Cypress run artifacts — regenerated locally/in CI, never meant to be committed +apps/web/cypress/screenshots/ +apps/web/cypress/videos/ +apps/web/cypress/downloads/ diff --git a/apps/api/.env.example b/apps/api/.env.example index 7dbf7af..9cd6e69 100644 --- a/apps/api/.env.example +++ b/apps/api/.env.example @@ -1,14 +1,20 @@ -NODE_ENV=development -PORT=3000 -# Match whatever you set in the root .env (POSTGRES_USER/PASSWORD/DB) — -# do not commit the real value. -DATABASE_URL="postgresql://changeme:changeme@localhost:5432/batchcooking?schema=public" - -# Required, no default on purpose — generate your own, e.g.: -# node -e "console.log(require('crypto').randomBytes(48).toString('hex'))" -JWT_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars - -# Optional — defaults shown (see src/config/env.ts) -# JWT_EXPIRES_IN=7d -# AUTH_COOKIE_NAME=session -# CORS_ORIGIN=http://localhost:5173 +NODE_ENV=development +PORT=3000 +# Match whatever you set in the root .env (POSTGRES_USER/PASSWORD/DB) — +# do not commit the real value. +DATABASE_URL="postgresql://changeme:changeme@localhost:5432/batchcooking?schema=public" + +# Required, no default on purpose — generate your own, e.g.: +# node -e "console.log(require('crypto').randomBytes(48).toString('hex'))" +JWT_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars + +# Optional — defaults shown (see src/config/env.ts) +# JWT_EXPIRES_IN=7d +# AUTH_COOKIE_NAME=session +# CORS_ORIGIN=http://localhost:5173 + +# Only needed if you're running services/tech-step-llm-worker locally — +# every /internal/tech-steps/* request is rejected outright while unset. +# Generate your own the same way as JWT_SECRET above; must match the +# worker's own INTERNAL_WORKER_SECRET. +# INTERNAL_WORKER_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars diff --git a/apps/api/.env.test.example b/apps/api/.env.test.example index 2d467ad..975619d 100644 --- a/apps/api/.env.test.example +++ b/apps/api/.env.test.example @@ -13,3 +13,8 @@ DATABASE_URL="postgresql://changeme:changeme@localhost:5432/batchcooking_test?sc # Required, no default on purpose — generate your own, e.g.: # node -e "console.log(require('crypto').randomBytes(48).toString('hex'))" JWT_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars + +# Optional — only needed to exercise tech-step-worker.routes.test.ts's +# success path (a request with a matching secret); every other test runs +# fine without it. Any value at least 32 chars works locally. +# INTERNAL_WORKER_SECRET=changeme-generate-a-real-random-secret-at-least-32-chars diff --git a/apps/api/prisma/migrations/20260822090000_tech_step_correction_and_suggestion/migration.sql b/apps/api/prisma/migrations/20260822090000_tech_step_correction_and_suggestion/migration.sql new file mode 100644 index 0000000..3210717 --- /dev/null +++ b/apps/api/prisma/migrations/20260822090000_tech_step_correction_and_suggestion/migration.sql @@ -0,0 +1,47 @@ +-- CreateTable +CREATE TABLE "step_tech_step_correction" ( + "id" SERIAL NOT NULL, + "step_id" INTEGER NOT NULL, + "corrector_id" INTEGER NOT NULL, + "start" INTEGER NOT NULL, + "end" INTEGER NOT NULL, + "previous_tech_step_id" INTEGER, + "corrected_tech_step_id" INTEGER, + "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "consumed_at" TIMESTAMP(3), + + CONSTRAINT "step_tech_step_correction_pkey" PRIMARY KEY ("id") +); + +-- CreateTable +CREATE TABLE "tech_step_training_suggestion" ( + "id" SERIAL NOT NULL, + "tech_step_id" INTEGER NOT NULL, + "locale" TEXT NOT NULL, + "suggested_synonyms" TEXT[], + "suggested_utterances" TEXT[], + "source_type" TEXT NOT NULL, + "source_correction_id" INTEGER, + "status" TEXT NOT NULL DEFAULT 'pending', + "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + + CONSTRAINT "tech_step_training_suggestion_pkey" PRIMARY KEY ("id") +); + +-- AddForeignKey +ALTER TABLE "step_tech_step_correction" ADD CONSTRAINT "step_tech_step_correction_step_id_fkey" FOREIGN KEY ("step_id") REFERENCES "step"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "step_tech_step_correction" ADD CONSTRAINT "step_tech_step_correction_corrector_id_fkey" FOREIGN KEY ("corrector_id") REFERENCES "user_profiles"("id") ON DELETE CASCADE ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "step_tech_step_correction" ADD CONSTRAINT "step_tech_step_correction_previous_tech_step_id_fkey" FOREIGN KEY ("previous_tech_step_id") REFERENCES "tech_step"("id") ON DELETE SET NULL ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "step_tech_step_correction" ADD CONSTRAINT "step_tech_step_correction_corrected_tech_step_id_fkey" FOREIGN KEY ("corrected_tech_step_id") REFERENCES "tech_step"("id") ON DELETE SET NULL ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "tech_step_training_suggestion" ADD CONSTRAINT "tech_step_training_suggestion_tech_step_id_fkey" FOREIGN KEY ("tech_step_id") REFERENCES "tech_step"("id") ON DELETE RESTRICT ON UPDATE CASCADE; + +-- AddForeignKey +ALTER TABLE "tech_step_training_suggestion" ADD CONSTRAINT "tech_step_training_suggestion_source_correction_id_fkey" FOREIGN KEY ("source_correction_id") REFERENCES "step_tech_step_correction"("id") ON DELETE SET NULL ON UPDATE CASCADE; 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 f1cf994..4aa57db 100644 --- a/apps/api/prisma/schema.prisma +++ b/apps/api/prisma/schema.prisma @@ -121,6 +121,10 @@ model UserProfile { /// list regardless of that real-world cardinality. administeredHouses House[] @relation("HouseAdmin") preferences UserPreference? + /// Tech-step corrections this profile has submitted (any profile that can + /// view a recipe may correct its tech-step matches, not just its author — + /// see `StepTechStepCorrection.correctorId`). + techStepCorrections StepTechStepCorrection[] @@map("user_profiles") } @@ -634,7 +638,16 @@ model TechStep { id Int @id @default(autoincrement()) key String @unique - steps StepTechStep[] + steps StepTechStep[] + /// Corrections where this technique was the *previous* (possibly wrong) + /// match — see `StepTechStepCorrection.previousTechStepId`. + correctionsAsPrevious StepTechStepCorrection[] @relation("PreviousTechStep") + /// Corrections where this technique was the *corrected* (user-asserted) + /// match — see `StepTechStepCorrection.correctedTechStepId`. + correctionsAsCorrected StepTechStepCorrection[] @relation("CorrectedTechStep") + /// Training-corpus suggestions targeting this technique — see + /// `TechStepTrainingSuggestion`. + trainingSuggestions TechStepTrainingSuggestion[] @@map("tech_step") } @@ -650,8 +663,11 @@ model Step { picture String? order Int - recipe Recipe @relation(fields: [recipeId], references: [id], onDelete: Cascade) - techSteps StepTechStep[] + recipe Recipe @relation(fields: [recipeId], references: [id], onDelete: Cascade) + techSteps StepTechStep[] + /// User-submitted corrections to this step's detected techniques — see + /// `StepTechStepCorrection`. + corrections StepTechStepCorrection[] @@map("step") } @@ -682,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) @@ -697,3 +726,87 @@ model StepTechStep { @@id([stepId, order]) @@map("step_tech_step") } + +/// One user-submitted correction to a `Step`'s detected techniques — +/// captures ADD (a missing technique the classifier didn't find), +/// REMOVE (a wrong technique it did), or RELABEL (both) as a single shape: +/// `previousTechStepId` is the (possibly absent) match being corrected, +/// `correctedTechStepId` is what the user asserts instead (absent means +/// "no technique belongs here"). Both `null` at once is invalid (nothing +/// would have changed) — enforced service-side, not by the schema, same +/// posture as other cross-field invariants in this codebase (e.g. +/// `RecipeIngredientView`'s no-duplicate-ingredient check). +/// +/// `start`/`end` are the user's selected `[start, end)` span within +/// `Step.description` (`String.prototype.slice` convention, same as +/// `StepTechStep`) — what they highlighted before assigning a technique to +/// it, not necessarily identical to any existing `StepTechStep` span. +/// +/// Never edited/deleted once created (an audit trail of what was actually +/// submitted) — only `consumedAt` changes, stamped once +/// `services/tech-step-llm-worker` has turned this correction into a +/// `TechStepTrainingSuggestion` for a maintainer to review, so the same +/// correction isn't proposed twice on the next scheduled run. +model StepTechStepCorrection { + id Int @id @default(autoincrement()) + stepId Int @map("step_id") + /// Any profile that could *view* the recipe when they submitted this, not + /// necessarily its author — see `assertRecipeVisible`, + /// `recipe.service.ts`. + correctorId Int @map("corrector_id") + start Int + end Int + previousTechStepId Int? @map("previous_tech_step_id") + correctedTechStepId Int? @map("corrected_tech_step_id") + createdAt DateTime @default(now()) @map("created_at") + consumedAt DateTime? @map("consumed_at") + + step Step @relation(fields: [stepId], references: [id], onDelete: Cascade) + corrector UserProfile @relation(fields: [correctorId], references: [id], onDelete: Cascade) + previousTechStep TechStep? @relation("PreviousTechStep", fields: [previousTechStepId], references: [id], onDelete: SetNull) + correctedTechStep TechStep? @relation("CorrectedTechStep", fields: [correctedTechStepId], references: [id], onDelete: SetNull) + trainingSuggestions TechStepTrainingSuggestion[] + + @@map("step_tech_step_correction") +} + +/// A candidate addition to `TECH_STEP_TRAINING_DATA` +/// (`tech-step-training-data.ts`), proposed by `services/tech-step-llm-worker` +/// from one of two sources (`sourceType`): +/// +/// - `"correction"` — a user's `StepTechStepCorrection`, turned into +/// suggested synonyms/utterances by the worker's LLM +/// (`transform-corrections` job). +/// - `"llm_audit"` — a low-confidence NLP clause on an *existing* recipe the +/// worker periodically samples and re-judges with its LLM +/// (`audit-low-confidence` job); no `sourceCorrectionId` in this case. +/// +/// Deliberately never auto-applied to `tech-step-training-data.ts` — a +/// maintainer reviews `status: "pending"` rows (see +/// `list-pending-training-suggestions.ts`) and edits that file by hand, +/// same "generated suggestion, human-reviewed source of truth" split as a +/// linter's autofix vs. a human-authored diff. `retrain-tech-steps.ts` then +/// flips `status` to `"applied"`/`"rejected"` once a maintainer has acted on +/// a batch, so the same suggestion isn't reviewed twice. +/// +/// `suggestedSynonyms`/`suggestedUtterances` are native Postgres arrays +/// (`String[]`), not a join table — unlike this schema's other list-shaped +/// data (`RecipeDiet`, `UserProfileAllergy`...), these strings are free text +/// proposed once for a human to read, not ids referencing another catalog +/// table, so there's nothing for a join table to normalize against. +model TechStepTrainingSuggestion { + id Int @id @default(autoincrement()) + techStepId Int @map("tech_step_id") + locale String + suggestedSynonyms String[] @map("suggested_synonyms") + suggestedUtterances String[] @map("suggested_utterances") + sourceType String @map("source_type") + sourceCorrectionId Int? @map("source_correction_id") + status String @default("pending") + createdAt DateTime @default(now()) @map("created_at") + + techStep TechStep @relation(fields: [techStepId], references: [id]) + sourceCorrection StepTechStepCorrection? @relation(fields: [sourceCorrectionId], references: [id], onDelete: SetNull) + + @@map("tech_step_training_suggestion") +} diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 4a5c86d..635cb55 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -7,6 +7,7 @@ import { errorLogger } from "./middlewares/error-logger.js"; import { requestLogger } from "./middlewares/request-logger.js"; import { authRouter } from "./modules/auth/auth.routes.js"; import { houseRouter } from "./modules/house/house.routes.js"; +import { techStepWorkerRouter } from "./modules/internal/tech-step-worker.routes.js"; import { planningRouter } from "./modules/planning/planning.routes.js"; import { preferencesRouter } from "./modules/preferences/preferences.routes.js"; import { profileRouter } from "./modules/profile/profile.routes.js"; @@ -38,6 +39,12 @@ export function createServer(): ExpressServer { server.mountRouter("/auth", authRouter); server.mountRouter("/house", houseRouter); + // Not user-facing — `services/tech-step-llm-worker` only, guarded by + // `requireInternalWorker` on every route within (see that router's own + // doc comment), never `requireAuth`. Mounted alongside the other routers + // rather than nested under one of them since it isn't scoped to a single + // recipe/step the way `recipeRouter`'s own correction routes are. + server.mountRouter("/internal/tech-steps", techStepWorkerRouter); server.mountRouter("/planning", planningRouter); server.mountRouter("/preferences", preferencesRouter); server.mountRouter("/profile", profileRouter); diff --git a/apps/api/src/config/env.ts b/apps/api/src/config/env.ts index 7f52193..47a86c4 100644 --- a/apps/api/src/config/env.ts +++ b/apps/api/src/config/env.ts @@ -60,6 +60,17 @@ const envSchema = z.object({ .string() .optional() .transform((value) => (value === undefined || value === "" ? undefined : value === "true")), + /** + * Shared secret `services/tech-step-llm-worker` sends as an + * `X-Internal-Worker-Secret` header on every call to `/internal/tech-steps/*` + * (`requireInternalWorker`, `middlewares/require-internal-worker.ts`). + * Optional with no default in the schema itself (unlike `JWT_SECRET`) so + * an environment that doesn't run the worker at all (e.g. this repo's + * existing test suite) never needs to set it — but `requireInternalWorker` + * itself rejects every request outright when it's unset, so the surface + * fails closed rather than open if a real deployment forgets to set it. + */ + INTERNAL_WORKER_SECRET: z.string().min(32).optional(), }); /** Parsed, validated environment — import this instead of reading `process.env` directly anywhere else. */ diff --git a/apps/api/src/lib/recipe-matching/tech-step-eval-dataset.ts b/apps/api/src/lib/recipe-matching/tech-step-eval-dataset.ts new file mode 100644 index 0000000..8a0d951 --- /dev/null +++ b/apps/api/src/lib/recipe-matching/tech-step-eval-dataset.ts @@ -0,0 +1,263 @@ +/** + * Hand-labeled evaluation set for {@link techStepClassifier} — what + * `tech-step-eval.test.ts` runs the real classifier against to compute + * precision/recall/F1 (`tech-step-evaluator.ts`), the objective gate any + * future change to `tech-step-training-data.ts` must clear (see that + * module's own doc comment). + * + * Deliberately *not* reusing `TECH_STEP_TRAINING_DATA`'s own `utterances` + * verbatim — scoring the classifier against the exact sentences it was + * trained on would measure memorization, not generalization. Every + * description below is original phrasing; where a case still needs to name + * a technique's own verb to be labeled with confidence (most of them, see + * this file's own limits below), it's at least a different sentence shape + * than anything in the training corpus. + * + * `expectedKeys` is a multiset in reading order (see + * `TechStepEvalOutcome`'s doc comment in `tech-step-evaluator.ts` for why + * order isn't scored but repetition is) of `TechStep.key`s — resolved to + * real DB ids and back by `tech-step-eval-runner.ts`, this file only ever + * deals in stable keys so it doesn't need DB access to author or read. + * + * Known limit of this dataset, confirmed against a real run (see + * `MIN_OVERALL_F1`'s own doc comment, `tech-step-eval-runner.ts`): most + * cases anchor on a technique's own registered synonym, but `_classifyClause` + * only falls back to that anchor when the intent classifier's own score is + * *below* `CONFIDENCE_THRESHOLD` — a confidently *wrong* whole-clause + * classification (e.g. "Blanchissez les haricots verts..." scoring + * confidently as `peel` despite the correct `blanch` anchor) overrides the + * anchor just as readily as a confidently *right* one does, so this + * dataset genuinely does measure real classifier failures, not just a + * synthetic floor. A handful of such real mismatches are expected and + * intentionally left uncorrected here (see `MIN_OVERALL_F1`'s doc comment) + * — fixing the classifier's actual behavior on them is corpus work for a + * future change, not something to hide by loosening this dataset's own + * expectations to match whatever it currently outputs. + */ + +export interface TechStepEvalCase { + description: string; + locale: "fr" | "en"; + expectedKeys: string[]; +} + +export const TECH_STEP_EVAL_DATASET: TechStepEvalCase[] = [ + // --- One straightforward case per technique (fr), covering all 26 --- + { + description: "Faites cuire les pâtes al dente dans une grande casserole d'eau bien salée.", + locale: "fr", + expectedKeys: ["cook"], + }, + { + description: "Faites bouillir l'eau dans une grande casserole avant d'y plonger les pâtes.", + locale: "fr", + expectedKeys: ["boil"], + }, + { + description: "Plongez les beignets dans l'huile très chaude pour les faire frire.", + locale: "fr", + expectedKeys: ["fry"], + }, + { + description: "Faites fondre le chocolat noir au bain-marie en remuant.", + locale: "fr", + expectedKeys: ["melt"], + }, + { + description: "Déglacez la casserole avec un trait de vinaigre balsamique.", + locale: "fr", + expectedKeys: ["deglaze"], + }, + { + description: "Laissez frémir la sauce tomate vingt minutes à couvert.", + locale: "fr", + expectedKeys: ["simmer"], + }, + { + description: "Faites rôtir la volaille entière sur la broche du four.", + locale: "fr", + expectedKeys: ["roast"], + }, + { + description: "Faites griller les brochettes de poulet quelques minutes de chaque côté.", + locale: "fr", + expectedKeys: ["grill"], + }, + { + description: "Faites sauter les champignons à feu vif dans une poêle très chaude.", + locale: "fr", + expectedKeys: ["panFry"], + }, + { + description: "Blanchissez les haricots verts trois minutes avant de les refroidir.", + locale: "fr", + expectedKeys: ["blanch"], + }, + { + description: "Laissez mariner les brochettes de poulet deux heures au frais.", + locale: "fr", + expectedKeys: ["marinate"], + }, + { + description: "Hachez grossièrement le persil frais avant de le parsemer.", + locale: "fr", + expectedKeys: ["chop"], + }, + { + description: "Épluchez les carottes avant de les couper en rondelles.", + locale: "fr", + expectedKeys: ["peel"], + }, + { + description: "Émincez finement l'échalote pour la vinaigrette.", + locale: "fr", + expectedKeys: ["mince"], + }, + { + description: "Mélangez la farine, le sucre et les œufs dans un grand saladier.", + locale: "fr", + expectedKeys: ["mix"], + }, + { + description: "Fouettez énergiquement la crème jusqu'à ce qu'elle épaississe.", + locale: "fr", + expectedKeys: ["whisk"], + }, + { + description: "Incorporez délicatement la farine tamisée à la préparation.", + locale: "fr", + expectedKeys: ["foldIn"], + }, + { + description: "Réservez la pâte au réfrigérateur pendant que vous préparez la garniture.", + locale: "fr", + expectedKeys: ["setAside"], + }, + { + description: "Assaisonnez le poisson avec du sel, du poivre et un filet de citron.", + locale: "fr", + expectedKeys: ["season"], + }, + { + description: "Égouttez soigneusement le riz dans une passoire fine.", + locale: "fr", + expectedKeys: ["drain"], + }, + { + description: + "Faites dorer les morceaux de veau sur toutes leurs faces avant de mouiller avec le bouillon.", + locale: "fr", + expectedKeys: ["brown"], + }, + { + description: "Laissez reposer la viande dix minutes avant de la trancher.", + locale: "fr", + expectedKeys: ["rest"], + }, + { + description: "Préchauffez le four à 200 degrés avant d'y glisser le gratin.", + locale: "fr", + expectedKeys: ["preheat"], + }, + { + description: "Enfournez la tarte pendant trente-cinq minutes jusqu'à ce qu'elle soit dorée.", + locale: "fr", + expectedKeys: ["bake"], + }, + { + description: "Dressez harmonieusement les légumes autour de la pièce de viande.", + locale: "fr", + expectedKeys: ["plate"], + }, + { + description: "Nappez le fond du moule d'une fine couche de caramel.", + locale: "fr", + expectedKeys: ["coat"], + }, + + // --- English coverage (same technique verbs, distinct sentences) --- + { + description: "Simmer the stock gently for forty minutes, skimming occasionally.", + locale: "en", + expectedKeys: ["simmer"], + }, + { + description: "Peel the potatoes and rinse them under cold water.", + locale: "en", + expectedKeys: ["peel"], + }, + { + description: "Whisk the eggs with a pinch of salt until frothy.", + locale: "en", + expectedKeys: ["whisk"], + }, + { + description: "Season the soup generously with black pepper before serving.", + locale: "en", + expectedKeys: ["season"], + }, + { + description: "Make sure the chicken is cooked through before serving.", + locale: "en", + expectedKeys: ["cook"], + }, + + // --- Multi-technique sentences, in reading order --- + { + description: "Préchauffez le four, puis faites rôtir le poulet pendant une heure.", + locale: "fr", + expectedKeys: ["preheat", "roast"], + }, + { + description: "Faites revenir les oignons, puis déglacez la poêle avec du vin blanc.", + locale: "fr", + expectedKeys: ["brown", "deglaze"], + }, + { + description: + "Faites cuire les légumes à la vapeur, puis assaisonnez-les avec des herbes fraîches.", + locale: "fr", + expectedKeys: ["cook", "season"], + }, + { + description: + "Émincez l'oignon, faites-le suer, puis mouillez avec le bouillon et laissez mijoter.", + locale: "fr", + expectedKeys: ["mince", "simmer"], + }, + + // --- No technique mentioned at all --- + { + description: "Répartissez les convives autour de la table avant de commencer le repas.", + locale: "fr", + expectedKeys: [], + }, + { + description: "Rangez les couverts propres dans le tiroir de la cuisine.", + locale: "fr", + expectedKeys: [], + }, + { + description: "Take the dishes and glasses out of the cupboard.", + locale: "en", + expectedKeys: [], + }, + + // --- Documented false-positive traps, re-verified with fresh wording --- + // `brown`'s EN synonyms are verb forms only ("browned"/"browning"), not + // bare "brown" — precisely so this doesn't false-positive (see that + // entry's own comment in tech-step-training-data.ts). + { + description: "This recipe calls for two tablespoons of brown sugar.", + locale: "en", + expectedKeys: [], + }, + // `rest`'s EN synonyms are anchored phrases ("let it rest"/"resting + // for"...), not bare "rest" — so a sentence using the word in its + // "remainder" sense must not anchor `rest` at all. + { + description: "There is no time to rest before the guests arrive.", + locale: "en", + expectedKeys: [], + }, +]; diff --git a/apps/api/src/lib/recipe-matching/tech-step-eval-runner.ts b/apps/api/src/lib/recipe-matching/tech-step-eval-runner.ts new file mode 100644 index 0000000..d095251 --- /dev/null +++ b/apps/api/src/lib/recipe-matching/tech-step-eval-runner.ts @@ -0,0 +1,71 @@ +import { prisma } from "../../db/prisma.js"; +import { TECH_STEP_EVAL_DATASET } from "./tech-step-eval-dataset.js"; +import { + computeTechStepMetrics, + type TechStepEvalOutcome, + type TechStepEvalResult, +} from "./tech-step-evaluator.js"; +import { techStepClassifier } from "./tech-step-matcher.js"; + +/** + * Regression floor (not a target) both `runTechStepEvalSuite`'s consumers + * gate on — exported from here (not defined separately in each consumer) + * so the CI regression gate and `scripts/retrain-tech-steps.ts`'s + * pre-backfill gate can never silently drift to different thresholds. + * + * Calibrated against a real run: the classifier trained on the corpus as + * of this constant's introduction scored **0.815** aggregate F1 + * (33 TP / 9 FP / 6 FN) against `TECH_STEP_EVAL_DATASET` — `0.8` leaves a + * small margin below that for run-to-run noise while still catching a + * real regression (not a floor picked blind before ever running this + * suite — see this feature's plan document for that earlier state). The + * mismatches this run surfaced (e.g. "Blanchissez les haricots verts..." + * misclassified as `peel`, a handful of anchor-less sentences expected to + * match nothing instead scoring confidently as some technique) are real, + * known classifier weaknesses — evidence this harness is doing its job, + * not something to quietly paper over by loosening the dataset's own + * expectations. Improving them is corpus work for a future change, gated + * by this same suite. + */ +export const MIN_OVERALL_F1 = 0.8; + +/** + * Runs {@link TECH_STEP_EVAL_DATASET} against the real, currently-trained + * `techStepClassifier` and returns the aggregate/per-technique metrics + * (`computeTechStepMetrics`, `tech-step-evaluator.ts`) — the one place this + * DB-touching "resolve ids to keys, then score" logic lives, shared by + * `test/recipe-matching/tech-step-eval.test.ts` (this feature's CI + * regression gate) and `scripts/retrain-tech-steps.ts` (the same gate, run + * by a maintainer before applying a corpus change). Kept out of + * `tech-step-evaluator.ts` itself, which is deliberately pure/DB-free (see + * that module's own doc comment) so its scoring logic stays unit-testable + * without a database. + */ +export async function runTechStepEvalSuite(): Promise { + const techSteps = await prisma.techStep.findMany({ select: { id: true, key: true } }); + const keyById = new Map(techSteps.map((techStep) => [techStep.id, techStep.key])); + + const outcomes: TechStepEvalOutcome[] = []; + for (const evalCase of TECH_STEP_EVAL_DATASET) { + const techStepIds = await techStepClassifier.matchTechSteps( + evalCase.description, + evalCase.locale, + ); + const actualKeys = techStepIds.map((id) => { + const key = keyById.get(id); + // A `techStepId` the classifier resolved that isn't in the seeded + // catalog would be a bug in the classifier or the seed data, not + // this dataset — fail loudly rather than silently dropping it (see + // `_train`'s own comment in `tech-step-matcher.ts` on the + // equivalent, deliberately silent `undefined` case it has to + // tolerate for a different reason). + if (key === undefined) { + throw new Error(`Unknown TechStep id ${id} returned for "${evalCase.description}"`); + } + return key; + }); + outcomes.push({ expectedKeys: evalCase.expectedKeys, actualKeys }); + } + + return computeTechStepMetrics(outcomes); +} diff --git a/apps/api/src/lib/recipe-matching/tech-step-evaluator.ts b/apps/api/src/lib/recipe-matching/tech-step-evaluator.ts new file mode 100644 index 0000000..6158189 --- /dev/null +++ b/apps/api/src/lib/recipe-matching/tech-step-evaluator.ts @@ -0,0 +1,134 @@ +/** + * Precision/recall/F1 for {@link techStepClassifier}'s output against a + * hand-labeled evaluation set (`tech-step-eval-dataset.ts`) — the objective + * counterpart to the "inspected by eye" verdict every corpus change used to + * get before this module existed. Every future edit to + * `tech-step-training-data.ts` (including the LLM-assisted suggestions the + * worker in `services/tech-step-llm-worker` proposes) is expected to run + * through `tech-step-eval.test.ts`'s regression gate, which calls + * {@link computeTechStepMetrics} — a corpus change that raises recall on one + * technique but silently tanks another's precision should fail loudly here, + * not get merged on the strength of a few manually-checked examples. + * + * Pure (no DB/model access) so it's unit-testable on its own — same + * convention as `tech-step-matcher.ts`'s own pure helpers (`normalizeText`, + * `splitIntoClauses`): this module only ever receives already-resolved + * `TechStep.key` strings, never DB ids or a live classifier instance, so it + * has nothing to mock to test. + */ + +/** True/false-positive/negative counts for one technique (or the aggregate across all of them), plus the precision/recall/F1 derived from them. */ +export interface TechStepMetrics { + truePositives: number; + falsePositives: number; + falseNegatives: number; + precision: number; + recall: number; + f1: number; +} + +/** + * One evaluation case's outcome — what {@link TechStepEvalCase.expectedKeys} + * said should be found, against what the classifier actually returned for + * that case (already mapped from `TechStepMatch.techStepId` back to + * `TechStep.key`, see `tech-step-eval.test.ts`). + * + * Both lists are *multisets*, not sets — a description that names the same + * technique twice (rare, but not impossible: "faire cuire, puis... remettre + * à cuire") is expected to produce two matches, and comparing as plain sets + * would silently treat a classifier that only found one of them as a + * perfect match. + */ +export interface TechStepEvalOutcome { + expectedKeys: string[]; + actualKeys: string[]; +} + +/** {@link computeTechStepMetrics}'s result — the aggregate across every case, plus a breakdown per technique so a regression hiding behind a healthy overall F1 (one technique's recall collapsing, offset by another's improving) is still visible. */ +export interface TechStepEvalResult { + overall: TechStepMetrics; + byKey: Record; +} + +interface RawCounts { + tp: number; + fp: number; + fn: number; +} + +function emptyCounts(): RawCounts { + return { tp: 0, fp: 0, fn: 0 }; +} + +/** Counts occurrences of each key in a multiset, e.g. `["cook", "cook", "bake"]` -> `{cook: 2, bake: 1}`. */ +function countByKey(keys: string[]): Map { + const counts = new Map(); + for (const key of keys) { + counts.set(key, (counts.get(key) ?? 0) + 1); + } + return counts; +} + +/** + * Standard vacuous-truth convention for the `0/0` cases: precision defaults + * to `1` when nothing was predicted for a key (`tp + fp === 0` — no false + * accusation to be precise about), recall defaults to `1` when nothing was + * expected (`tp + fn === 0` — nothing to have missed). Neither inflates F1 + * on its own: a technique the classifier fully misses still has `recall = + * 0` (there *were* expected occurrences, just none matched), which is what + * pulls F1 down to `0` for that case regardless of precision's vacuous `1`. + */ +function toMetrics(counts: RawCounts): TechStepMetrics { + const { tp, fp, fn } = counts; + const precision = tp + fp === 0 ? 1 : tp / (tp + fp); + const recall = tp + fn === 0 ? 1 : tp / (tp + fn); + const f1 = precision + recall === 0 ? 0 : (2 * precision * recall) / (precision + recall); + return { truePositives: tp, falsePositives: fp, falseNegatives: fn, precision, recall, f1 }; +} + +/** + * Aggregates every {@link TechStepEvalOutcome} into one overall + * precision/recall/F1 plus a per-technique breakdown. + * + * Counted key by key, multiset-style, per outcome: for a given technique, + * `min(expectedCount, actualCount)` true positives, any actual occurrences + * beyond that are false positives, any expected occurrences short of that + * are false negatives — generalizes the usual set-based TP/FP/FN definition + * to handle a technique mentioned (or matched) more than once in the same + * step without over- or under-counting it. + */ +export function computeTechStepMetrics(outcomes: TechStepEvalOutcome[]): TechStepEvalResult { + const overallCounts = emptyCounts(); + const countsByKey = new Map(); + + for (const outcome of outcomes) { + const expectedCounts = countByKey(outcome.expectedKeys); + const actualCounts = countByKey(outcome.actualKeys); + const allKeys = new Set([...expectedCounts.keys(), ...actualCounts.keys()]); + + for (const key of allKeys) { + const expected = expectedCounts.get(key) ?? 0; + const actual = actualCounts.get(key) ?? 0; + const tp = Math.min(expected, actual); + const fp = Math.max(0, actual - expected); + const fn = Math.max(0, expected - actual); + + overallCounts.tp += tp; + overallCounts.fp += fp; + overallCounts.fn += fn; + + const keyCounts = countsByKey.get(key) ?? emptyCounts(); + keyCounts.tp += tp; + keyCounts.fp += fp; + keyCounts.fn += fn; + countsByKey.set(key, keyCounts); + } + } + + const byKey: Record = {}; + for (const [key, counts] of countsByKey) { + byKey[key] = toMetrics(counts); + } + + return { overall: toMetrics(overallCounts), byKey }; +} diff --git a/apps/api/src/lib/recipe-matching/tech-step-matcher.ts b/apps/api/src/lib/recipe-matching/tech-step-matcher.ts index 9f7b580..9bfff45 100644 --- a/apps/api/src/lib/recipe-matching/tech-step-matcher.ts +++ b/apps/api/src/lib/recipe-matching/tech-step-matcher.ts @@ -252,7 +252,32 @@ export function splitIntoClauses( * — `0.75` sits comfortably above the noise floor and below every genuine * match seen so far. */ -const CONFIDENCE_THRESHOLD = 0.75; +export const CONFIDENCE_THRESHOLD = 0.75; + +/** + * One clause's full classification detail — the finer-grained sibling of + * {@link TechStepMatch}, exposing the raw intent/score + * `TechStepClassifierService`'s private `_classifyClause` normally + * collapses into a single accepted-or-fallback verdict. Nothing on the + * interactive save/read path needs this (that's exactly what + * `_classifyClause`'s threshold + fallback logic is for) — it exists for + * `services/tech-step-llm-worker`'s "audit low-confidence clauses" job + * (`modules/internal/tech-step-worker.service.ts`'s `getAuditBatch`), which + * needs to see *which* clauses the classifier itself wasn't sure about, not + * just its final best-effort verdict. + */ +export interface TechStepClauseClassification { + /** The clause's own text (`description.slice(start, end)`, trimmed). */ + clauseText: string; + start: number; + end: number; + /** The clause's NER anchor's own implied technique `uid`, if it had one — same as `TechStepClause.anchor.uid`. */ + anchorUid: string | null; + /** The intent classifier's own top guess for this clause, whatever its score — `null` only when it returned node-nlp's `"None"` sentinel. Unlike {@link TechStepMatch}, never silently replaced by the anchor's uid — the whole point of this type is to expose the classifier's raw opinion, confident or not. */ + intentUid: string | null; + /** The intent classifier's own confidence for `intentUid` — `0` when `intentUid` is `null` (nothing to have a score about). */ + score: number; +} /** * Trains and owns the `node-nlp` model behind {@link matchTechStepSpans} — @@ -388,6 +413,66 @@ export class TechStepClassifierService { } } + /** + * Splits `description` into clauses exactly like {@link matchTechStepSpans} + * does, but returns each clause's *raw* classification detail + * ({@link TechStepClauseClassification}) instead of the threshold-applied, + * anchor-fallback-resolved `TechStepMatch` — see that type's doc comment + * for why/who needs this. Deliberately a separate traversal rather than a + * shared refactor with `matchTechStepSpans`/`_classifyClause`: this method + * exists purely to add a new, additive read path without risking a + * behavior change to the two already-relied-on methods above. + */ + public async classifyClauses( + description: string, + locale: string, + ): Promise { + try { + await this._ensureTrained(); + if (description.trim().length === 0) return []; + + const nerResult = await this._manager.process(locale, description); + const candidates: TechniqueCandidate[] = nerResult.entities + .filter((entity) => entity.type === "enum") + .map((entity) => ({ + uid: entity.entity, + start: entity.start, + end: entity.end + 1, + })); + + const clauses = splitIntoClauses(description, candidates); + const results: TechStepClauseClassification[] = []; + for (const clause of clauses) { + const clauseText = description.slice(clause.start, clause.end).trim(); + const anchorUid = clause.anchor?.uid ?? null; + if (clauseText.length === 0) { + results.push({ + clauseText, + start: clause.start, + end: clause.end, + anchorUid, + intentUid: null, + score: 0, + }); + continue; + } + const result = await this._manager.process(locale, clauseText); + const intentUid = result.intent !== "None" ? result.intent : null; + results.push({ + clauseText, + start: clause.start, + end: clause.end, + anchorUid, + intentUid, + score: intentUid === null ? 0 : result.score, + }); + } + return results; + } catch (err) { + throw err; // see matchTechStepSpans()'s catch comment above + } + } + /** * Convenience wrapper around {@link matchTechStepSpans} for callers that * only care about *which* techniques matched, not where — e.g. diff --git a/apps/api/src/middlewares/require-internal-worker.ts b/apps/api/src/middlewares/require-internal-worker.ts new file mode 100644 index 0000000..f74535e --- /dev/null +++ b/apps/api/src/middlewares/require-internal-worker.ts @@ -0,0 +1,50 @@ +import { timingSafeEqual } from "node:crypto"; +import { HttpError } from "@batch-cooking/error-tools"; +import { ErrorCode } from "@batch-cooking/shared"; +import type { NextFunction, Request, Response } from "express"; +import { env } from "../config/env.js"; + +/** Header `services/tech-step-llm-worker` sends its shared secret on. Not `Authorization`/a bearer scheme — this isn't a user session, just one internal caller authenticating to another, same "one flat shared secret" shape as e.g. a webhook signing header. */ +const INTERNAL_WORKER_SECRET_HEADER = "x-internal-worker-secret"; + +/** + * Express middleware guarding `/internal/tech-steps/*` — the surface + * `services/tech-step-llm-worker` (a process outside this monorepo, no + * Prisma access of its own, see that service's own README) reads + * low-confidence NLP clauses and pending `StepTechStepCorrection`s from, + * and posts `TechStepTrainingSuggestion`s back to. Never reachable by an + * end user's session cookie — deliberately a *different* auth mechanism + * than {@link requireAuth} (`require-auth.ts`), not layered on top of it, + * since the worker has no `UserProfile`/session of its own to authenticate + * as. + * + * Fails closed: an unset `INTERNAL_WORKER_SECRET` (the default in any + * environment that doesn't run the worker, see `config/env.ts`) rejects + * every request rather than leaving the surface open, same posture as a + * misconfigured `JWT_SECRET` would if it had a working fallback. + * + * @throws {HttpError} `401 NOT_AUTHENTICATED` if the header is missing, + * wrong, or the server has no secret configured at all — never + * distinguishes the reason, same posture as {@link requireAuth}. + */ +export function requireInternalWorker(req: Request, _res: Response, next: NextFunction): void { + const provided = req.header(INTERNAL_WORKER_SECRET_HEADER); + if (env.INTERNAL_WORKER_SECRET === undefined || provided === undefined) { + next(new HttpError(401, ErrorCode.NOT_AUTHENTICATED, "Not authenticated")); + return; + } + + // `timingSafeEqual` throws on mismatched buffer lengths rather than + // returning `false` — checked separately first. A length mismatch alone + // already means "not equal", so this loses no timing-attack protection + // (an attacker learns nothing beyond what a differing length itself + // already reveals, no different from `!==` on the common case where the + // secret's real length isn't a secret worth protecting). + const expected = Buffer.from(env.INTERNAL_WORKER_SECRET); + const actual = Buffer.from(provided); + if (expected.length !== actual.length || !timingSafeEqual(expected, actual)) { + next(new HttpError(401, ErrorCode.NOT_AUTHENTICATED, "Not authenticated")); + return; + } + next(); +} diff --git a/apps/api/src/modules/internal/tech-step-worker.routes.ts b/apps/api/src/modules/internal/tech-step-worker.routes.ts new file mode 100644 index 0000000..748cd9a --- /dev/null +++ b/apps/api/src/modules/internal/tech-step-worker.routes.ts @@ -0,0 +1,49 @@ +import { wrapAsyncHandler } from "@batch-cooking/express-tools"; +import { + auditBatchQuerySchema, + submitTrainingSuggestionsSchema, + workerBatchQuerySchema, +} from "@batch-cooking/shared"; +import { Router } from "express"; +import { requireInternalWorker } from "../../middlewares/require-internal-worker.js"; +import { + getAuditBatch, + getPendingCorrections, + submitTrainingSuggestions, +} from "./tech-step-worker.service.js"; + +/** + * Router mounted at `/internal/tech-steps` in app.ts — every route requires + * {@link requireInternalWorker}, never {@link requireAuth} + * (`middlewares/require-auth.ts`): this is `services/tech-step-llm-worker` + * authenticating as itself, not a user session. See that middleware's own + * doc comment for why the two are deliberately separate mechanisms. + */ +export const techStepWorkerRouter = Router(); + +techStepWorkerRouter.get( + "/audit-batch", + requireInternalWorker, + wrapAsyncHandler(async (req, res) => { + const input = auditBatchQuerySchema.parse(req.query); + res.status(200).json(await getAuditBatch(input.locale, input.limit)); + }), +); + +techStepWorkerRouter.get( + "/pending-corrections", + requireInternalWorker, + wrapAsyncHandler(async (req, res) => { + const input = workerBatchQuerySchema.parse(req.query); + res.status(200).json(await getPendingCorrections(input.limit)); + }), +); + +techStepWorkerRouter.post( + "/training-suggestions", + requireInternalWorker, + wrapAsyncHandler(async (req, res) => { + const input = submitTrainingSuggestionsSchema.parse(req.body); + res.status(201).json(await submitTrainingSuggestions(input)); + }), +); diff --git a/apps/api/src/modules/internal/tech-step-worker.service.ts b/apps/api/src/modules/internal/tech-step-worker.service.ts new file mode 100644 index 0000000..607c0f1 --- /dev/null +++ b/apps/api/src/modules/internal/tech-step-worker.service.ts @@ -0,0 +1,201 @@ +import { HttpError } from "@batch-cooking/error-tools"; +import { + ErrorCode, + type PendingTechStepCorrectionView, + type SubmitTrainingSuggestionsInput, + type TechStepAuditClauseView, +} from "@batch-cooking/shared"; +import { prisma } from "../../db/prisma.js"; +import { + CONFIDENCE_THRESHOLD, + techStepClassifier, +} from "../../lib/recipe-matching/tech-step-matcher.js"; + +/** + * Read/write surface `services/tech-step-llm-worker` calls through + * `/internal/tech-steps/*` (`tech-step-worker.routes.ts`, guarded by + * `requireInternalWorker`) — the worker has no Prisma client or database + * credentials of its own (see that service's own README), so every + * corrections/audit-sample read and every suggestion write goes through + * here rather than the worker touching this schema directly. Keeps + * `apps/api` the single owner of the schema/migrations, and keeps the + * worker a pure "read some text, run inference, post a suggestion" process + * with nothing to keep in sync if the schema changes shape. + */ + +/** + * How many of the most recently created `Step`s {@link getAuditBatch} scans + * per call before filtering down to low-confidence clauses — a fixed + * recency-biased sample, not every `Step` in the database, to keep this + * endpoint's cost bounded regardless of how large the recipe catalog gets. + * Recently-added steps are also the steps most likely to still use + * vocabulary the training corpus hasn't caught up with yet, which is + * exactly what this audit is for. A smarter sampling strategy (e.g. + * weighted by how often a recipe is actually viewed/planned) is future + * work, not needed for this feature's first version. + */ +const AUDIT_SAMPLE_SIZE = 200; + +/** + * Every low-confidence clause found across a recency-biased sample of + * existing `Step`s (see {@link AUDIT_SAMPLE_SIZE}), for + * `services/tech-step-llm-worker`'s `audit-low-confidence` job to get a + * second opinion on. "Low-confidence" mirrors exactly what + * `TechStepClassifierService._classifyClause` itself distrusts (a clause + * with an NER anchor but a classifier score under + * {@link CONFIDENCE_THRESHOLD}) — the same clauses that pipeline already + * has to fall back to keyword-anchor guessing for, not an arbitrary + * separate cutoff. + */ +export async function getAuditBatch( + locale: string, + limit: number, +): Promise { + try { + const steps = await prisma.step.findMany({ + orderBy: { id: "desc" }, + take: AUDIT_SAMPLE_SIZE, + select: { id: true, recipeId: true, description: true }, + }); + + const results: TechStepAuditClauseView[] = []; + for (const step of steps) { + if (results.length >= limit) break; + const clauses = await techStepClassifier.classifyClauses(step.description, locale); + for (const clause of clauses) { + if (results.length >= limit) break; + const isLowConfidence = clause.anchorUid !== null && clause.score < CONFIDENCE_THRESHOLD; + if (!isLowConfidence) continue; + results.push({ + stepId: step.id, + recipeId: step.recipeId, + clauseText: clause.clauseText, + anchorKey: clause.anchorUid, + intentKey: clause.intentUid, + score: clause.score, + locale, + }); + } + } + return results; + } catch (err) { + throw err; // see recipe.service.ts's equivalent catch comment + } +} + +/** + * Every `StepTechStepCorrection` not yet turned into a + * `TechStepTrainingSuggestion` (`consumedAt IS NULL`), oldest first — a + * FIFO queue the worker's `transform-corrections` job drains, `limit` at a + * time. + * + * `correctedTechStepId IS NOT NULL` on top of `consumedAt IS NULL`: a + * correction that *removes* a match ("no technique belongs here", + * `correctedTechStepId: null` — see `StepTechStepCorrection`'s schema doc + * comment) has no technique to propose new positive training data *for*. + * Surfacing it here would leave it permanently unconsumable (the worker + * has nothing to submit a suggestion for, so it would never stamp + * `consumedAt`, and it would keep re-appearing in every future batch + * forever) — excluded at the source instead, not filtered/skipped + * downstream by the worker. + */ +export async function getPendingCorrections( + limit: number, +): Promise { + try { + const corrections = await prisma.stepTechStepCorrection.findMany({ + where: { consumedAt: null, correctedTechStepId: { not: null } }, + orderBy: { createdAt: "asc" }, + take: limit, + include: { + step: { select: { id: true, recipeId: true, description: true } }, + previousTechStep: { select: { key: true } }, + correctedTechStep: { select: { key: true } }, + }, + }); + + return corrections.map((correction) => ({ + id: correction.id, + stepId: correction.step.id, + recipeId: correction.step.recipeId, + clauseText: correction.step.description.slice(correction.start, correction.end), + start: correction.start, + end: correction.end, + previousTechStepKey: correction.previousTechStep?.key ?? null, + correctedTechStepKey: correction.correctedTechStep?.key ?? null, + })); + } catch (err) { + throw err; // see recipe.service.ts's equivalent catch comment + } +} + +/** + * Persists a batch of `TechStepTrainingSuggestion`s and, for every + * suggestion sourced from a correction, stamps that correction's + * `consumedAt` in the same transaction — so a worker run that crashes + * partway through never leaves a correction consumed with no matching + * suggestion, or a suggestion created against a correction still (wrongly) + * eligible to be picked up again by the next run. + * + * @throws {HttpError} `404 TECH_STEP_NOT_FOUND` if any `techStepKey` in the + * batch doesn't match a reference `TechStep` — rejects the *whole* batch + * rather than skipping the bad entries, on the theory that a worker + * sending an unknown key is more likely a version-skew bug (its own + * taxonomy copy, `services/tech-step-llm-worker/src/tech-step-taxonomy.ts`, + * drifting from this API's `TechStep` catalog) than a one-off it should + * silently tolerate. + */ +export async function submitTrainingSuggestions( + input: SubmitTrainingSuggestionsInput, +): Promise<{ created: number }> { + try { + const techStepKeys = [ + ...new Set(input.suggestions.map((suggestion) => suggestion.techStepKey)), + ]; + const techSteps = await prisma.techStep.findMany({ + where: { key: { in: techStepKeys } }, + select: { id: true, key: true }, + }); + const techStepIdByKey = new Map(techSteps.map((techStep) => [techStep.key, techStep.id])); + const missingKeys = techStepKeys.filter((key) => !techStepIdByKey.has(key)); + if (missingKeys.length > 0) { + throw new HttpError( + 404, + ErrorCode.TECH_STEP_NOT_FOUND, + `Unknown techStepKey(s): ${missingKeys.join(", ")}`, + ); + } + + await prisma.$transaction(async (tx) => { + for (const suggestion of input.suggestions) { + // Non-null by construction — every key in `input.suggestions` was + // just confirmed present in `techStepIdByKey` above (the `missingKeys` + // check would have thrown otherwise). + const techStepId = techStepIdByKey.get(suggestion.techStepKey); + if (techStepId === undefined) continue; + + await tx.techStepTrainingSuggestion.create({ + data: { + techStepId, + locale: suggestion.locale, + suggestedSynonyms: suggestion.suggestedSynonyms, + suggestedUtterances: suggestion.suggestedUtterances, + sourceType: suggestion.sourceType, + sourceCorrectionId: suggestion.sourceCorrectionId ?? null, + }, + }); + + if (suggestion.sourceCorrectionId !== null && suggestion.sourceCorrectionId !== undefined) { + await tx.stepTechStepCorrection.update({ + where: { id: suggestion.sourceCorrectionId }, + data: { consumedAt: new Date() }, + }); + } + } + }); + + return { created: input.suggestions.length }; + } catch (err) { + throw err; // see recipe.service.ts's equivalent catch comment + } +} 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 new file mode 100644 index 0000000..6ae000e --- /dev/null +++ b/apps/api/src/modules/recipe/recipe-tech-step-correction.service.ts @@ -0,0 +1,323 @@ +import { HttpError } from "@batch-cooking/error-tools"; +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, toStepTechStepViews } from "./recipe.service.js"; + +/** + * User-submitted corrections to a step's detected techniques + * (`StepTechStepCorrection` in schema.prisma) — kept in its own module + * rather than folded into `recipe.service.ts`, same "one file per concern" + * split that file itself follows for `tech-step-matcher.ts`. Deliberately + * open to *any* viewer who can see the recipe, not just its author (unlike + * every write path in `recipe.service.ts`, which uses `assertIsAuthor`) — + * correcting a mislabeled technique isn't editing the recipe's own + * content, and restricting it to authors would starve the training-data + * feedback loop (`services/tech-step-llm-worker`) of the volume it needs. + */ + +type CorrectionWithTechSteps = Prisma.StepTechStepCorrectionGetPayload<{ + include: { previousTechStep: true; correctedTechStep: true }; +}>; + +const correctionInclude = { + previousTechStep: true, + correctedTechStep: true, +} satisfies Prisma.StepTechStepCorrectionInclude; + +/** + * Loads `stepId`'s current `description` length (the only thing a + * correction needs from the step itself), or throws — `404 STEP_NOT_FOUND` + * if no such step exists, or if it exists but doesn't belong to `recipeId` + * (the route's own `:id`/`:stepId` nesting is meaningless otherwise — a + * request naming a real step under the wrong recipe should look identical + * to naming one that doesn't exist, same "don't leak which part was wrong" + * posture `assertRecipeVisible` already has for visibility). Otherwise + * whatever {@link assertRecipeVisible} throws (`404 RECIPE_NOT_FOUND`, + * never `403`) if the recipe exists but isn't visible to the viewer. + */ +async function loadVisibleStepOrThrow( + recipeId: number, + stepId: number, + viewerId: number, + viewerHouseId: number | null, +): Promise<{ id: number; descriptionLength: number }> { + try { + const step = await prisma.step.findUnique({ + where: { id: stepId }, + select: { id: true, recipeId: true, description: true }, + }); + if (!step || step.recipeId !== recipeId) { + throw new HttpError(404, ErrorCode.STEP_NOT_FOUND, `Step ${stepId} not found`); + } + await assertRecipeVisible(step.recipeId, viewerId, viewerHouseId); + return { id: step.id, descriptionLength: step.description.length }; + } catch (err) { + throw err; // see recipe.service.ts's equivalent catch comment + } +} + +/** Throws `404 TECH_STEP_NOT_FOUND` if any id in `ids` doesn't match a reference `TechStep` row — same shape as `recipe.service.ts`'s `assertIngredientsExist`/`assertUnitsExist` for the recipe payload's own reference ids. */ +async function assertTechStepsExist(ids: number[]): Promise { + try { + if (ids.length === 0) return; + const found = await prisma.techStep.findMany({ + where: { id: { in: ids } }, + select: { id: true }, + }); + const foundIds = new Set(found.map((techStep) => techStep.id)); + const missing = ids.filter((id) => !foundIds.has(id)); + if (missing.length > 0) { + throw new HttpError( + 404, + ErrorCode.TECH_STEP_NOT_FOUND, + `TechStep ids not found: ${missing.join(", ")}`, + ); + } + } catch (err) { + throw err; // see loadVisibleStepOrThrow's catch comment + } +} + +/** + * 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, + start: correction.start, + end: correction.end, + previousTechStep: correction.previousTechStep + ? { id: correction.previousTechStep.id, key: correction.previousTechStep.key } + : null, + correctedTechStep: correction.correctedTechStep + ? { id: correction.correctedTechStep.id, key: correction.correctedTechStep.key } + : null, + createdAt: correction.createdAt.toISOString(), + }; +} + +/** + * Records one correction to `stepId`'s detected techniques, submitted by + * `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 + * `start`/`end` fall outside the step's current `description` (it may + * have been edited since the user last saw it). `404 TECH_STEP_NOT_FOUND` + * if either tech-step id doesn't exist. + */ +export async function submitTechStepCorrection( + recipeId: number, + stepId: number, + input: SubmitTechStepCorrectionInput, + correctorId: number, + viewerHouseId: number | null, +): Promise { + try { + const step = await loadVisibleStepOrThrow(recipeId, stepId, correctorId, viewerHouseId); + + if (input.start >= step.descriptionLength || input.end > step.descriptionLength) { + throw new HttpError( + 400, + ErrorCode.INVALID_CORRECTION_SPAN, + `Span [${input.start}, ${input.end}) falls outside step ${stepId}'s description (length ${step.descriptionLength})`, + ); + } + + const techStepIds = [input.previousTechStepId, input.correctedTechStepId].filter( + (id): id is number => id !== null && id !== undefined, + ); + await assertTechStepsExist(techStepIds); + + 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 { correction: toCorrectionView(correction), techSteps: toStepTechStepViews(techSteps) }; + } catch (err) { + throw err; // see loadVisibleStepOrThrow's catch comment + } +} + +/** + * Every correction submitted so far for `stepId`, most recent first — + * mainly useful for a user checking what's already been submitted (by + * anyone) for a span before adding another (see `StepTechStepCorrectionView`'s + * doc comment, `packages/shared`). + * + * @throws {HttpError} `404 STEP_NOT_FOUND`/`404 RECIPE_NOT_FOUND` — see {@link loadVisibleStepOrThrow}. + */ +export async function listTechStepCorrections( + recipeId: number, + stepId: number, + viewerId: number, + viewerHouseId: number | null, +): Promise { + try { + const step = await loadVisibleStepOrThrow(recipeId, stepId, viewerId, viewerHouseId); + const corrections = await prisma.stepTechStepCorrection.findMany({ + where: { stepId: step.id }, + orderBy: { createdAt: "desc" }, + include: correctionInclude, + }); + return corrections.map(toCorrectionView); + } catch (err) { + throw err; // see loadVisibleStepOrThrow's catch comment + } +} diff --git a/apps/api/src/modules/recipe/recipe.routes.ts b/apps/api/src/modules/recipe/recipe.routes.ts index 669bdc7..f7fa8fd 100644 --- a/apps/api/src/modules/recipe/recipe.routes.ts +++ b/apps/api/src/modules/recipe/recipe.routes.ts @@ -4,6 +4,7 @@ import { createRecipeSchema, ErrorCode, listRecipesSchema, + submitTechStepCorrectionSchema, updateRecipeSchema, } from "@batch-cooking/shared"; import { Router } from "express"; @@ -17,6 +18,10 @@ import { removeFavorite, updateRecipe, } from "./recipe.service.js"; +import { + listTechStepCorrections, + submitTechStepCorrection, +} from "./recipe-tech-step-correction.service.js"; /** Router mounted at `/recipes` in app.ts. Every route requires a session — the catalog is shared across households, not public (same reasoning as `planning`/`house`: it's app content, not signup-time reference data). */ export const recipeRouter = Router(); @@ -30,6 +35,15 @@ function parseRecipeId(rawId: string | undefined): number { return id; } +/** Same shape as {@link parseRecipeId}, for the `:stepId` route param of the tech-step-correction routes below — a distinct function only so the error message names the right param. */ +function parseStepId(rawId: string | undefined): number { + const id = Number(rawId); + if (!Number.isInteger(id)) { + throw new HttpError(400, ErrorCode.VALIDATION_ERROR, "stepId must be an integer"); + } + return id; +} + recipeRouter.get( "/", requireAuth, @@ -109,3 +123,29 @@ recipeRouter.delete( res.status(204).end(); }), ); + +// Open to any authenticated viewer who can see the recipe, not just its +// author — see recipe-tech-step-correction.service.ts's own doc comment +// for why. +recipeRouter.post( + "/:id/steps/:stepId/corrections", + requireAuth, + wrapAsyncHandler(async (req, res) => { + const id = parseRecipeId(req.params.id); + const stepId = parseStepId(req.params.stepId); + const input = submitTechStepCorrectionSchema.parse(req.body); + const { id: correctorId, houseId } = res.locals.userProfile; + res.status(201).json(await submitTechStepCorrection(id, stepId, input, correctorId, houseId)); + }), +); + +recipeRouter.get( + "/:id/steps/:stepId/corrections", + requireAuth, + wrapAsyncHandler(async (req, res) => { + const id = parseRecipeId(req.params.id); + const stepId = parseStepId(req.params.stepId); + const { id: viewerId, houseId } = res.locals.userProfile; + res.status(200).json(await listTechStepCorrections(id, stepId, viewerId, houseId)); + }), +); 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 519f107..5facc3e 100644 --- a/apps/api/src/scripts/backfill-tech-steps.ts +++ b/apps/api/src/scripts/backfill-tech-steps.ts @@ -1,60 +1,123 @@ +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"; /** - * One-off maintenance script: recomputes every existing `Step`'s - * `StepTechStep` sequence against the *current* classifier + * 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). + * no persisted per-recipe locale to recover for a step that already + * exists, so this matches real resave behavior exactly rather than + * guessing). * * Needed because tech-step detection only ever runs at create/update time * (`recipe.service.ts`'s `matchStepsTechSteps`), never retroactively — a * step saved before a classifier/corpus change (new vocabulary, or the - * `contextStart`/`contextEnd` columns this same session added) keeps - * whatever it was matched with at the time until it's next resaved. Run - * this after a corpus change to bring every existing step in sync without - * asking users to open and resave every recipe by hand: + * `contextStart`/`contextEnd` columns a previous session added) keeps + * whatever it was matched with at the time until it's next resaved. * - * pnpm --filter api exec tsx src/scripts/backfill-tech-steps.ts + * `"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. * - * 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. + * 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: 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. */ -async function backfillTechSteps(): Promise { +export async function backfillTechSteps(): Promise<{ total: number; changed: number }> { const steps = await prisma.step.findMany({ select: { id: true, description: true } }); console.info(`Recomputing tech steps for ${steps.length} step(s)...`); 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; } console.info(`Done — ${changed} step(s) recomputed.`); + return { total: steps.length, changed }; } -backfillTechSteps() - .then(() => prisma.$disconnect()) - .catch(async (err) => { - console.error(err); - await prisma.$disconnect(); - process.exit(1); - }); +// Only runs when this file is executed directly (`tsx +// src/scripts/backfill-tech-steps.ts`), not when `retrain-tech-steps.ts` +// 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. `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()) + .catch(async (err) => { + console.error(err); + await prisma.$disconnect(); + process.exit(1); + }); +} diff --git a/apps/api/src/scripts/list-pending-training-suggestions.ts b/apps/api/src/scripts/list-pending-training-suggestions.ts new file mode 100644 index 0000000..17089a3 --- /dev/null +++ b/apps/api/src/scripts/list-pending-training-suggestions.ts @@ -0,0 +1,71 @@ +import { prisma } from "../db/prisma.js"; + +/** + * Maintainer-facing report of every `TechStepTrainingSuggestion` still + * `status: "pending"` (`TechStepTrainingSuggestion`'s own schema doc + * comment) — generated by `services/tech-step-llm-worker`'s scheduled + * jobs, from either a user correction or the worker's own low-confidence + * audit (`sourceType`). What a maintainer reads *before* hand-editing + * `tech-step-training-data.ts` and running `retrain-tech-steps.ts` — this + * script never writes anything, purely a read-only report to stdout: + * + * pnpm --filter api exec tsx src/scripts/list-pending-training-suggestions.ts + * + * Grouped by technique key so every suggestion for the same entry in + * `TECH_STEP_TRAINING_DATA` is read together, matching how that file + * itself is organized (one block per technique). + */ +async function listPendingTrainingSuggestions(): Promise { + const suggestions = await prisma.techStepTrainingSuggestion.findMany({ + where: { status: "pending" }, + orderBy: [{ techStepId: "asc" }, { createdAt: "asc" }], + include: { techStep: { select: { key: true } } }, + }); + + if (suggestions.length === 0) { + console.info("No pending training suggestions."); + return; + } + + const byTechStepKey = new Map(); + for (const suggestion of suggestions) { + const key = suggestion.techStep.key; + const group = byTechStepKey.get(key); + if (group) { + group.push(suggestion); + } else { + byTechStepKey.set(key, [suggestion]); + } + } + + const lines: string[] = [`# Pending tech-step training suggestions (${suggestions.length})`, ""]; + for (const [techStepKey, group] of byTechStepKey) { + lines.push(`## ${techStepKey}`, ""); + for (const suggestion of group) { + const source = + suggestion.sourceCorrectionId !== null + ? `${suggestion.sourceType} (correction #${suggestion.sourceCorrectionId})` + : suggestion.sourceType; + lines.push(`- id ${suggestion.id} · locale ${suggestion.locale} · source: ${source}`); + if (suggestion.suggestedSynonyms.length > 0) { + lines.push(` - synonyms: ${suggestion.suggestedSynonyms.join(", ")}`); + } + if (suggestion.suggestedUtterances.length > 0) { + lines.push( + ` - utterances: ${suggestion.suggestedUtterances.map((u) => `"${u}"`).join(", ")}`, + ); + } + } + lines.push(""); + } + + console.info(lines.join("\n")); +} + +listPendingTrainingSuggestions() + .then(() => prisma.$disconnect()) + .catch(async (err) => { + console.error(err); + await prisma.$disconnect(); + process.exit(1); + }); diff --git a/apps/api/src/scripts/retrain-tech-steps.ts b/apps/api/src/scripts/retrain-tech-steps.ts new file mode 100644 index 0000000..0e870e7 --- /dev/null +++ b/apps/api/src/scripts/retrain-tech-steps.ts @@ -0,0 +1,98 @@ +import { prisma } from "../db/prisma.js"; +import { + MIN_OVERALL_F1, + runTechStepEvalSuite, +} from "../lib/recipe-matching/tech-step-eval-runner.js"; +import { backfillTechSteps } from "./backfill-tech-steps.js"; + +/** Parses `--applied=1,2,3`/`--rejected=4,5` from argv into id arrays — both optional, both empty by default (a run with neither flag only re-gates + backfills, doesn't touch any suggestion's status). */ +function parseSuggestionIds(flag: "applied" | "rejected"): number[] { + const prefix = `--${flag}=`; + const arg = process.argv.find((value) => value.startsWith(prefix)); + if (arg === undefined) return []; + return arg + .slice(prefix.length) + .split(",") + .map((value) => value.trim()) + .filter((value) => value.length > 0) + .map((value) => { + const id = Number(value); + if (!Number.isInteger(id)) { + throw new Error(`--${flag}: "${value}" is not a valid integer id`); + } + return id; + }); +} + +/** + * Maintainer workflow closing the loop on a training-corpus change (see + * this feature's plan document): + * + * 1. A maintainer has already hand-edited `tech-step-training-data.ts` + * (informed by `list-pending-training-suggestions.ts`'s report), and + * decided which `TechStepTrainingSuggestion` ids they incorporated + * (`--applied=`) or explicitly discarded (`--rejected=`). + * 2. This script re-runs the F1 regression gate + * ({@link runTechStepEvalSuite} against {@link MIN_OVERALL_F1}) — + * refuses to backfill at all if the edited corpus scores worse than + * the floor, so a bad edit never reaches every existing recipe. + * 3. Backfills every `Step`'s `StepTechStep` sequence against the new + * corpus ({@link backfillTechSteps}). + * 4. Marks the given suggestion ids `applied`/`rejected`, so + * `list-pending-training-suggestions.ts`'s next report doesn't + * surface them again. + * + * Usage: + * + * pnpm --filter api exec tsx src/scripts/retrain-tech-steps.ts --applied=12,13 --rejected=14 + * + * `--applied`/`--rejected` are both optional — omitting both still runs + * the gate + backfill, just leaves every suggestion's `status` untouched + * (useful for re-running the backfill alone after a corpus edit made with + * no suggestions involved at all). + */ +async function retrainTechSteps(): Promise { + const appliedIds = parseSuggestionIds("applied"); + const rejectedIds = parseSuggestionIds("rejected"); + + console.info("Evaluating the current classifier against the labeled evaluation set..."); + const { overall } = await runTechStepEvalSuite(); + console.info( + `F1 ${overall.f1.toFixed(3)} (precision ${overall.precision.toFixed(3)}, recall ${overall.recall.toFixed(3)})`, + ); + if (overall.f1 < MIN_OVERALL_F1) { + throw new Error( + `Aggregate F1 ${overall.f1.toFixed(3)} is below the ${MIN_OVERALL_F1} regression floor — refusing to backfill. Revert or fix the corpus change and re-run.`, + ); + } + + const { total, changed } = await backfillTechSteps(); + console.info(`Backfilled ${changed}/${total} step(s).`); + + if (appliedIds.length > 0) { + // `updateMany`'s own `count` (rows actually matched/updated), not + // `appliedIds.length` (what was merely *asked for*) — an id that + // doesn't exist (typo, already-processed id) would otherwise log a + // success count that silently doesn't match what really changed. + const { count } = await prisma.techStepTrainingSuggestion.updateMany({ + where: { id: { in: appliedIds } }, + data: { status: "applied" }, + }); + console.info(`Marked ${count}/${appliedIds.length} suggestion(s) as applied.`); + } + if (rejectedIds.length > 0) { + const { count } = await prisma.techStepTrainingSuggestion.updateMany({ + where: { id: { in: rejectedIds } }, + data: { status: "rejected" }, + }); + console.info(`Marked ${count}/${rejectedIds.length} suggestion(s) as rejected.`); + } +} + +retrainTechSteps() + .then(() => prisma.$disconnect()) + .catch(async (err) => { + console.error(err); + await prisma.$disconnect(); + process.exit(1); + }); diff --git a/apps/api/test/internal/tech-step-worker.routes.test.ts b/apps/api/test/internal/tech-step-worker.routes.test.ts new file mode 100644 index 0000000..b969270 --- /dev/null +++ b/apps/api/test/internal/tech-step-worker.routes.test.ts @@ -0,0 +1,306 @@ +import { ErrorCode } from "@batch-cooking/shared"; +import { expect } from "chai"; +import request from "supertest"; +import { createApp } from "../../src/app.js"; +import { env } from "../../src/config/env.js"; +import { prisma } from "../../src/db/prisma.js"; +import { resetDatabase } from "../../test-support/reset-db.js"; + +const SECRET_HEADER = "X-Internal-Worker-Secret"; + +/** Resolves a reference tech step's id by its `reference-seed-data.ts` uid (also its DB `key`) — mirrors `recipe.test.ts`'s own `techStepId` helper. */ +async function techStepId(key: string): Promise { + const techStep = await prisma.techStep.findFirstOrThrow({ where: { key } }); + return techStep.id; +} + +/** A minimal author + recipe + step fixture — these routes have no notion of a session/viewer, so nothing here needs to go through `/auth/signup` the way `recipe.test.ts`'s fixtures do. */ +async function createRecipeWithStep( + description = "Faire mijoter la sauce.", +): Promise<{ stepId: number; recipeId: number }> { + const author = await prisma.userProfile.create({ + data: { + firstName: "Test", + lastName: "Author", + email: `${crypto.randomUUID()}@example.test`, + passwordHash: "not-a-real-hash", + }, + }); + const recipe = await prisma.recipe.create({ + data: { + name: "Recette", + authorId: author.id, + portions: 4, + steps: { create: [{ description, order: 0 }] }, + }, + include: { steps: true }, + }); + const step = recipe.steps[0]; + if (!step) throw new Error("expected the fixture recipe to have one step"); + return { stepId: step.id, recipeId: recipe.id }; +} + +describe("Internal tech-step worker routes", () => { + const app = createApp(); + + beforeEach(async () => { + await resetDatabase(); + }); + + after(async () => { + await prisma.$disconnect(); + }); + + describe("requireInternalWorker", () => { + it("rejects a request with no secret header with 401 NOT_AUTHENTICATED", async () => { + const res = await request(app).get("/internal/tech-steps/audit-batch"); + + expect(res.status).to.equal(401); + expect(res.body.code).to.equal(ErrorCode.NOT_AUTHENTICATED); + }); + + it("rejects a request with the wrong secret with 401 NOT_AUTHENTICATED", async () => { + const res = await request(app) + .get("/internal/tech-steps/audit-batch") + .set(SECRET_HEADER, "definitely-not-the-right-secret"); + + expect(res.status).to.equal(401); + expect(res.body.code).to.equal(ErrorCode.NOT_AUTHENTICATED); + }); + + it("rejects even the correct secret with 401 NOT_AUTHENTICATED on a plain user-facing route (no bypass of requireAuth)", async () => { + const res = await request(app) + .get("/recipes") + .query({ tab: "publique" }) + .set(SECRET_HEADER, env.INTERNAL_WORKER_SECRET ?? "irrelevant-unset-in-this-env"); + + expect(res.status).to.equal(401); + expect(res.body.code).to.equal(ErrorCode.NOT_AUTHENTICATED); + }); + }); + + // Every test below needs a real configured secret to exercise the success + // path — skipped (not failed) in an environment that hasn't set one, same + // "optional, but the surface fails closed without it" posture + // `INTERNAL_WORKER_SECRET` itself has (see config/env.ts). Both this + // repo's `.env.test.example` and `.github/workflows/ci.yml` set one, so + // this only actually skips in an environment that deliberately diverges + // from both. + describe("with a configured secret", () => { + before(function skipWithoutConfiguredSecret() { + if (env.INTERNAL_WORKER_SECRET === undefined) { + // biome-ignore lint/suspicious/noExplicitAny: mocha's `this.skip()` isn't typed without @types/mocha (not a dependency here) — same untyped-`this` shape a plain JS mocha callback would have. + (this as any).skip(); + } + }); + + function withSecret(req: request.Test): request.Test { + return req.set(SECRET_HEADER, env.INTERNAL_WORKER_SECRET as string); + } + + describe("GET /internal/tech-steps/audit-batch", () => { + // A true low-confidence positive case can't be asserted here without + // a live trained classifier to verify the exact sentence against + // first — same limitation `tech-step-eval-dataset.ts` documents for + // the same reason (no local Postgres was reachable in the session + // that introduced this file). This test instead covers the + // deterministic negative: a step the classifier confidently resolves + // (proven by `tech-step-matcher.test.ts`'s own identical-sentence + // case) must produce zero audit entries — nothing here should ever + // flag a confident match as worth a second opinion. + it("finds nothing to audit in a step the classifier confidently resolves", async () => { + await createRecipeWithStep("Faire mijoter à feu doux"); + + const res = await withSecret( + request(app).get("/internal/tech-steps/audit-batch").query({ locale: "fr" }), + ); + + expect(res.status).to.equal(200); + expect(res.body).to.deep.equal([]); + }); + + it("finds nothing to audit in a step naming no technique at all", async () => { + await createRecipeWithStep("Ranger les couverts dans le tiroir"); + + const res = await withSecret( + request(app).get("/internal/tech-steps/audit-batch").query({ locale: "fr" }), + ); + + expect(res.status).to.equal(200); + expect(res.body).to.deep.equal([]); + }); + + it("rejects a non-positive limit with 400 VALIDATION_ERROR", async () => { + const res = await withSecret( + request(app).get("/internal/tech-steps/audit-batch").query({ limit: 0 }), + ); + + expect(res.status).to.equal(400); + expect(res.body.code).to.equal(ErrorCode.VALIDATION_ERROR); + }); + }); + + describe("GET /internal/tech-steps/pending-corrections", () => { + it("returns unconsumed corrections, oldest first, excluding already-consumed ones", async () => { + const { stepId, recipeId } = await createRecipeWithStep(); + const simmerId = await techStepId("simmer"); + const author = await prisma.recipe + .findUniqueOrThrow({ where: { id: recipeId } }) + .then((recipe) => recipe.authorId); + + const older = await prisma.stepTechStepCorrection.create({ + data: { stepId, correctorId: author, start: 6, end: 13, correctedTechStepId: simmerId }, + }); + const consumed = await prisma.stepTechStepCorrection.create({ + data: { + stepId, + correctorId: author, + start: 0, + end: 5, + correctedTechStepId: simmerId, + consumedAt: new Date(), + }, + }); + const newer = await prisma.stepTechStepCorrection.create({ + data: { stepId, correctorId: author, start: 6, end: 13, correctedTechStepId: simmerId }, + }); + + const res = await withSecret(request(app).get("/internal/tech-steps/pending-corrections")); + + expect(res.status).to.equal(200); + const ids = (res.body as Array<{ id: number }>).map((entry) => entry.id); + expect(ids).to.deep.equal([older.id, newer.id]); + expect(ids).to.not.include(consumed.id); + }); + + it("respects ?limit=", async () => { + const { stepId, recipeId } = await createRecipeWithStep(); + const simmerId = await techStepId("simmer"); + const author = await prisma.recipe + .findUniqueOrThrow({ where: { id: recipeId } }) + .then((recipe) => recipe.authorId); + await prisma.stepTechStepCorrection.createMany({ + data: [ + { stepId, correctorId: author, start: 0, end: 5, correctedTechStepId: simmerId }, + { stepId, correctorId: author, start: 6, end: 13, correctedTechStepId: simmerId }, + ], + }); + + const res = await withSecret( + request(app).get("/internal/tech-steps/pending-corrections").query({ limit: 1 }), + ); + + expect(res.status).to.equal(200); + expect(res.body).to.have.length(1); + }); + }); + + describe("POST /internal/tech-steps/training-suggestions", () => { + it("creates a suggestion and marks its source correction consumed", async () => { + const { stepId, recipeId } = await createRecipeWithStep(); + const simmerId = await techStepId("simmer"); + const author = await prisma.recipe + .findUniqueOrThrow({ where: { id: recipeId } }) + .then((recipe) => recipe.authorId); + const correction = await prisma.stepTechStepCorrection.create({ + data: { stepId, correctorId: author, start: 6, end: 13, correctedTechStepId: simmerId }, + }); + + const res = await withSecret( + request(app) + .post("/internal/tech-steps/training-suggestions") + .send({ + suggestions: [ + { + techStepKey: "simmer", + locale: "fr", + suggestedSynonyms: ["frémissonner"], + suggestedUtterances: ["laisser frémissonner à feu très doux"], + sourceType: "correction", + sourceCorrectionId: correction.id, + }, + ], + }), + ); + + expect(res.status).to.equal(201); + expect(res.body).to.deep.equal({ created: 1 }); + + const suggestions = await prisma.techStepTrainingSuggestion.findMany({ + where: { techStepId: simmerId }, + }); + expect(suggestions).to.have.length(1); + expect(suggestions[0]?.sourceCorrectionId).to.equal(correction.id); + expect(suggestions[0]?.status).to.equal("pending"); + + const updatedCorrection = await prisma.stepTechStepCorrection.findUniqueOrThrow({ + where: { id: correction.id }, + }); + expect(updatedCorrection.consumedAt).to.not.equal(null); + }); + + it("accepts an llm_audit suggestion with no sourceCorrectionId", async () => { + const res = await withSecret( + request(app) + .post("/internal/tech-steps/training-suggestions") + .send({ + suggestions: [ + { + techStepKey: "boil", + locale: "fr", + suggestedSynonyms: ["bouillonner"], + suggestedUtterances: [], + sourceType: "llm_audit", + }, + ], + }), + ); + + expect(res.status).to.equal(201); + expect(res.body).to.deep.equal({ created: 1 }); + }); + + it("rejects sourceType 'correction' with no sourceCorrectionId with 400 VALIDATION_ERROR", async () => { + const res = await withSecret( + request(app) + .post("/internal/tech-steps/training-suggestions") + .send({ + suggestions: [ + { + techStepKey: "boil", + locale: "fr", + suggestedSynonyms: ["bouillonner"], + suggestedUtterances: [], + sourceType: "correction", + }, + ], + }), + ); + + expect(res.status).to.equal(400); + expect(res.body.code).to.equal(ErrorCode.VALIDATION_ERROR); + }); + + it("rejects an unknown techStepKey with 404 TECH_STEP_NOT_FOUND", async () => { + const res = await withSecret( + request(app) + .post("/internal/tech-steps/training-suggestions") + .send({ + suggestions: [ + { + techStepKey: "not-a-real-tech-step", + locale: "fr", + suggestedSynonyms: [], + suggestedUtterances: [], + sourceType: "llm_audit", + }, + ], + }), + ); + + expect(res.status).to.equal(404); + expect(res.body.code).to.equal(ErrorCode.TECH_STEP_NOT_FOUND); + }); + }); + }); +}); diff --git a/apps/api/test/recipe-matching/tech-step-eval.test.ts b/apps/api/test/recipe-matching/tech-step-eval.test.ts new file mode 100644 index 0000000..1d60c67 --- /dev/null +++ b/apps/api/test/recipe-matching/tech-step-eval.test.ts @@ -0,0 +1,37 @@ +import { expect } from "chai"; +import { prisma } from "../../src/db/prisma.js"; +import { + MIN_OVERALL_F1, + runTechStepEvalSuite, +} from "../../src/lib/recipe-matching/tech-step-eval-runner.js"; +import { resetDatabase } from "../../test-support/reset-db.js"; + +/** + * Regression gate for `TECH_STEP_TRAINING_DATA` — every change to that + * corpus (including a maintainer applying suggestions from + * `TechStepTrainingSuggestion`, see `scripts/retrain-tech-steps.ts`) must + * keep this suite green. Runs {@link runTechStepEvalSuite} (the real + * trained classifier against `tech-step-eval-dataset.ts`) and asserts the + * aggregate F1 doesn't fall below {@link MIN_OVERALL_F1} — see that + * constant's own doc comment (`tech-step-eval-runner.ts`) for the real run + * it was calibrated against. + */ + +describe("tech-step-eval", () => { + beforeEach(async () => { + await resetDatabase(); + }); + + after(async () => { + await prisma.$disconnect(); + }); + + it(`scores at least ${MIN_OVERALL_F1} aggregate F1 against the labeled evaluation set`, async () => { + const { overall, byKey } = await runTechStepEvalSuite(); + + expect( + overall.f1, + `aggregate F1 ${overall.f1.toFixed(3)} (precision ${overall.precision.toFixed(3)}, recall ${overall.recall.toFixed(3)}) fell below the ${MIN_OVERALL_F1} floor — per-technique breakdown: ${JSON.stringify(byKey)}`, + ).to.be.at.least(MIN_OVERALL_F1); + }); +}); diff --git a/apps/api/test/recipe/recipe-tech-step-correction.test.ts b/apps/api/test/recipe/recipe-tech-step-correction.test.ts new file mode 100644 index 0000000..240c218 --- /dev/null +++ b/apps/api/test/recipe/recipe-tech-step-correction.test.ts @@ -0,0 +1,280 @@ +import type { SignupInput } from "@batch-cooking/shared"; +import { ErrorCode } from "@batch-cooking/shared"; +import { faker } from "@faker-js/faker"; +import { expect } from "chai"; +import request from "supertest"; +import { createApp } from "../../src/app.js"; +import { prisma } from "../../src/db/prisma.js"; +import { resetDatabase } from "../../test-support/reset-db.js"; + +/** See `auth.test.ts` — generated rather than hardcoded, no test fixture looks like a real person's data. */ +function buildSignupPayload(): SignupInput { + const firstName = faker.person.firstName(); + const lastName = faker.person.lastName(); + return { + firstName, + lastName, + email: faker.internet.email({ firstName, lastName }).toLowerCase(), + password: faker.internet.password({ length: 16 }), + }; +} + +/** Resolves a reference tech step's id by its `reference-seed-data.ts` uid (also its DB `key`) — mirrors `recipe.test.ts`'s own `techStepId` helper. */ +async function techStepId(key: string): Promise { + const techStep = await prisma.techStep.findFirstOrThrow({ where: { key } }); + return techStep.id; +} + +describe("Recipe tech-step corrections", () => { + const app = createApp(); + + async function signup(): Promise<{ agent: ReturnType; profileId: number }> { + const agent = request.agent(app); + const res = await agent.post("/auth/signup").send(buildSignupPayload()); + return { agent, profileId: res.body.id }; + } + + /** A `PUBLIC` recipe with one step — every viewer can see this, so most tests below don't need to juggle visibility on top of the correction logic itself. */ + async function createPublicRecipeWithStep( + authorId: number, + description = "Faire mijoter la sauce.", + ): Promise<{ recipeId: number; stepId: number }> { + const recipe = await prisma.recipe.create({ + data: { + name: "Recette", + authorId, + visibility: "PUBLIC", + portions: 4, + steps: { create: [{ description, order: 0 }] }, + }, + include: { steps: true }, + }); + const step = recipe.steps[0]; + if (!step) throw new Error("expected the fixture recipe to have one step"); + return { recipeId: recipe.id, stepId: step.id }; + } + + beforeEach(async () => { + await resetDatabase(); + }); + + after(async () => { + await prisma.$disconnect(); + }); + + describe("POST /recipes/:id/steps/:stepId/corrections", () => { + it("rejects requests without a session cookie with 401 NOT_AUTHENTICATED", async () => { + const { profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + + const res = await request(app) + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 0, end: 5, correctedTechStepId: await techStepId("simmer") }); + + expect(res.status).to.equal(401); + expect(res.body.code).to.equal(ErrorCode.NOT_AUTHENTICATED); + }); + + 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"); + + const res = await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, correctedTechStepId: simmerId }); + + expect(res.status).to.equal(201); + 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), 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.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 () => { + const { profileId: authorId } = await signup(); + const { agent: otherAgent } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(authorId); + + const res = await otherAgent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, correctedTechStepId: await techStepId("simmer") }); + + expect(res.status).to.equal(201); + }); + + it("rejects both previousTechStepId and correctedTechStepId absent with 400 VALIDATION_ERROR", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + + const res = await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 0, end: 5 }); + + expect(res.status).to.equal(400); + expect(res.body.code).to.equal(ErrorCode.VALIDATION_ERROR); + }); + + it("rejects end <= start with 400 VALIDATION_ERROR", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + + const res = await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 5, end: 5, correctedTechStepId: await techStepId("simmer") }); + + expect(res.status).to.equal(400); + expect(res.body.code).to.equal(ErrorCode.VALIDATION_ERROR); + }); + + it("rejects a span past the end of the step's description with 400 INVALID_CORRECTION_SPAN", async () => { + const { agent, profileId } = await signup(); + const description = "Court."; + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId, description); + + const res = await agent.post(`/recipes/${recipeId}/steps/${stepId}/corrections`).send({ + start: 0, + end: description.length + 10, + correctedTechStepId: await techStepId("simmer"), + }); + + expect(res.status).to.equal(400); + expect(res.body.code).to.equal(ErrorCode.INVALID_CORRECTION_SPAN); + }); + + it("rejects an unknown correctedTechStepId with 404 TECH_STEP_NOT_FOUND", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + + const res = await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 0, end: 5, correctedTechStepId: 999_999 }); + + expect(res.status).to.equal(404); + expect(res.body.code).to.equal(ErrorCode.TECH_STEP_NOT_FOUND); + }); + + it("rejects a step that exists but isn't visible to the viewer with 404 RECIPE_NOT_FOUND", async () => { + const { profileId: authorId } = await signup(); + const { agent: otherAgent } = await signup(); + const recipe = await prisma.recipe.create({ + data: { + name: "Secrète", + authorId, + portions: 4, + steps: { create: [{ description: "Faire mijoter la sauce.", order: 0 }] }, + }, + include: { steps: true }, + }); + const step = recipe.steps[0]; + if (!step) throw new Error("expected the fixture recipe to have one step"); + + const res = await otherAgent + .post(`/recipes/${recipe.id}/steps/${step.id}/corrections`) + .send({ start: 0, end: 5, correctedTechStepId: await techStepId("simmer") }); + + expect(res.status).to.equal(404); + expect(res.body.code).to.equal(ErrorCode.RECIPE_NOT_FOUND); + }); + + it("rejects a stepId that belongs to a different recipe than the URL's :id with 404 STEP_NOT_FOUND", async () => { + const { agent, profileId } = await signup(); + const { recipeId: otherRecipeId } = await createPublicRecipeWithStep(profileId); + const { stepId } = await createPublicRecipeWithStep(profileId); + + const res = await agent + .post(`/recipes/${otherRecipeId}/steps/${stepId}/corrections`) + .send({ start: 0, end: 5, correctedTechStepId: await techStepId("simmer") }); + + expect(res.status).to.equal(404); + expect(res.body.code).to.equal(ErrorCode.STEP_NOT_FOUND); + }); + }); + + describe("GET /recipes/:id/steps/:stepId/corrections", () => { + it("returns every correction submitted for the step, most recent first", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + const simmerId = await techStepId("simmer"); + const boilId = await techStepId("boil"); + + await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, correctedTechStepId: simmerId }); + await agent + .post(`/recipes/${recipeId}/steps/${stepId}/corrections`) + .send({ start: 6, end: 13, previousTechStepId: simmerId, correctedTechStepId: boilId }); + + const res = await agent.get(`/recipes/${recipeId}/steps/${stepId}/corrections`); + + expect(res.status).to.equal(200); + expect(res.body).to.have.length(2); + expect(res.body[0].correctedTechStep).to.deep.equal({ id: boilId, key: "boil" }); + expect(res.body[1].correctedTechStep).to.deep.equal({ id: simmerId, key: "simmer" }); + }); + + it("returns an empty list when nothing has been submitted yet", async () => { + const { agent, profileId } = await signup(); + const { recipeId, stepId } = await createPublicRecipeWithStep(profileId); + + const res = await agent.get(`/recipes/${recipeId}/steps/${stepId}/corrections`); + + expect(res.status).to.equal(200); + expect(res.body).to.deep.equal([]); + }); + }); +}); diff --git a/apps/web/cypress/component/TechStepCorrectionPopover.cy.tsx b/apps/web/cypress/component/TechStepCorrectionPopover.cy.tsx new file mode 100644 index 0000000..effbb22 --- /dev/null +++ b/apps/web/cypress/component/TechStepCorrectionPopover.cy.tsx @@ -0,0 +1,121 @@ +import "../../src/i18n/i18n"; +import { TechStepCorrectionPopover } from "../../src/features/recipes/steps/TechStepCorrectionPopover"; + +// Mounts the popover in isolation (no StepDescription/selection plumbing +// around it) — same "generic component test" posture as CheckboxOption.cy.tsx, +// but this one needs `../../src/i18n/i18n` imported for its side effect +// (initializes the default i18next instance `useTranslation` falls back to +// with no `` in the tree — see that module's own doc +// comment) since, unlike Checkbox/Radio, this component calls +// `useTranslation()`. + +const cook = { id: 1, key: "cook" }; +const simmer = { id: 3, key: "simmer" }; + +function mountPopover( + overrides: Partial<{ + previousTechStepId: number | null; + onClose: () => void; + onSubmitted: (correction: unknown) => void; + }> = {}, +) { + cy.mount( +
+ {/* A genuinely separate sibling to click for the "outside click closes it" test — clicking blindly at a viewport coordinate would risk still landing inside the popover, which fills most of the mounted area on its own. */} +
+ {})} + onSubmitted={overrides.onSubmitted ?? (() => {})} + /> +
, + ); +} + +describe("TechStepCorrectionPopover", () => { + beforeEach(() => { + cy.intercept("GET", "**/reference/tech-steps", { statusCode: 200, body: [cook, simmer] }).as( + "getTechSteps", + ); + }); + + it("shows the selected text and every technique option once loaded", () => { + mountPopover(); + cy.wait("@getTechSteps"); + + cy.contains(".tech-step-correction-popover__selection", "Cuire").should("be.visible"); + cy.get(".tech-step-correction-popover__list button").should("have.length", 2); + }); + + it("offers a 'no technique here' option only when correcting an existing match", () => { + mountPopover({ previousTechStepId: null }); + cy.wait("@getTechSteps"); + cy.get(".tech-step-correction-popover__remove").should("not.exist"); + + mountPopover({ previousTechStepId: cook.id }); + cy.wait("@getTechSteps"); + cy.get(".tech-step-correction-popover__remove").should("exist"); + }); + + it("submits the selected technique and calls onSubmitted", () => { + // Asserting on the resolved `@submitCorrection` interception below, + // rather than inside this handler — a Chai assertion failing *inside* + // a `cy.intercept` callback surfaces as an opaque "onResponse cannot be + // called twice" Cypress internal error instead of a normal assertion + // failure, found while writing this exact test. + cy.intercept("POST", "**/recipes/2/steps/2/corrections", { + statusCode: 201, + body: { + id: 1, + start: 0, + end: 5, + previousTechStep: null, + correctedTechStep: simmer, + createdAt: new Date().toISOString(), + }, + }).as("submitCorrection"); + const onSubmitted = cy.stub().as("onSubmitted"); + mountPopover({ onSubmitted }); + cy.wait("@getTechSteps"); + + cy.contains(".tech-step-correction-popover__list button", "Mijoter").click(); + + cy.wait("@submitCorrection").its("request.body").should("deep.equal", { + start: 0, + end: 5, + previousTechStepId: null, + correctedTechStepId: simmer.id, + }); + cy.get("@onSubmitted").should("have.been.calledOnce"); + }); + + it("shows an error message and stays open when the submission fails", () => { + cy.intercept("POST", "**/recipes/2/steps/2/corrections", { + statusCode: 404, + body: { code: 4051, message: "TechStep not found" }, + }).as("submitCorrection"); + const onClose = cy.stub().as("onClose"); + mountPopover({ onClose }); + cy.wait("@getTechSteps"); + + cy.contains(".tech-step-correction-popover__list button", "Cuire").click(); + + cy.wait("@submitCorrection"); + cy.get(".field-error").should("be.visible"); + cy.get("@onClose").should("not.have.been.called"); + }); + + it("calls onClose on an outside click", () => { + const onClose = cy.stub().as("onClose"); + mountPopover({ onClose }); + cy.wait("@getTechSteps"); + + cy.get('[data-testid="outside-popover"]').click(); + + cy.get("@onClose").should("have.been.calledOnce"); + }); +}); diff --git a/apps/web/cypress/component/highlight-tech-steps.cy.tsx b/apps/web/cypress/component/highlight-tech-steps.cy.tsx index a581538..f06a6c4 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,42 @@ 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" }, + ]); + }); + + it("never lets one match's wider context swallow another match's own keyword span", () => { + // The motivating real bug (found via live testing, not invented for + // this test): "simmer" is the only NER candidate `splitIntoClauses` + // found, so its context spans the *entire* description — before this + // was fixed, that wide context advanced `cursor` past 39, silently + // dropping "setAside"'s own keyword span (a manual correction on + // "materiel", a word with no relation to "simmer" at all) instead of + // rendering it. + const text = "Faire mijoter la sauce, puis ranger le materiel."; + const result = splitDescriptionByTechSteps(text, [ + techStep("simmer", 1, 6, 13, { start: 0, end: 48 }), + techStep("setAside", 2, 39, 47, undefined, "manual"), + ]); + const keywordSegments = result.filter((s) => s.isKeyword); + expect( + keywordSegments.map((s) => ({ key: s.techStep?.key, text: s.text, source: s.source })), + ).to.deep.equal([ + { key: "simmer", text: "mijoter", source: "auto" }, + { key: "setAside", text: "materiel", source: "manual" }, + ]); + expect(result.map((s) => s.text).join("")).to.equal(text); + }); + 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 +173,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 +199,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 +214,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 +242,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 bc0b5d1..29914e2 100644 --- a/apps/web/cypress/e2e/recipes.feature +++ b/apps/web/cypress/e2e/recipes.feature @@ -27,6 +27,24 @@ 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, 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 + And correcting step 2's "Cuire" match will succeed + When I visit "/recettes/2" + And I click the highlighted technique "Cuire" + Then I should see the technique correction options + When I choose "Mijoter" as the correct technique + Then the correction request should have been made + # 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" And recipe 2's detail is available diff --git a/apps/web/cypress/e2e/recipes.ts b/apps/web/cypress/e2e/recipes.ts index 14a75f5..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" }], }, ], }; @@ -68,6 +68,62 @@ Given("deleting recipe 2 will succeed", () => { cy.intercept("DELETE", "**/recipes/2", { statusCode: 204 }).as("deleteRecipe"); }); +// 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 `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: { + 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"); +}); + +When("I click the highlighted technique {string}", (text: string) => { + cy.contains(".step-tech-step", text).click(); +}); + +Then("I should see the technique correction options", () => { + cy.get(".tech-step-correction-popover").should("be.visible"); +}); + +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 + // `offset` variable by reference instead of a value captured at render + // time) once sent `end` all the way to the end of the description + // instead of "Cuire"'s own tight [0, 5) span, and a request-fired-only + // assertion here didn't catch it — found only via manual testing. + cy.wait("@correction") + .its("request.body") + .should("deep.include", { start: 0, end: 5, previousTechStepId: 1 }); +}); + Then("the recipe {string} should not be visible in the table", (name: string) => { cy.contains(".recipe-table__name", name).should("not.exist"); }); diff --git a/apps/web/cypress/support/step_definitions/reference-data.steps.ts b/apps/web/cypress/support/step_definitions/reference-data.steps.ts index f105815..a457b4a 100644 --- a/apps/web/cypress/support/step_definitions/reference-data.steps.ts +++ b/apps/web/cypress/support/step_definitions/reference-data.steps.ts @@ -40,6 +40,21 @@ Given("the sources reference list is empty", () => { cy.intercept("GET", "**/reference/sources", { statusCode: 200, body: [] }); }); +// `id`/`key` pairs mirror `recipes.ts`'s `omeletteDetail` fixture (`cook`, +// id 1, is the step's existing match) plus a second option +// (`simmer`/"Mijoter") for `recipes.feature`'s correction scenario to +// re-assign to — TechStepCorrectionPopover's own picker needs at least two +// choices for that scenario to be a meaningful correction, not a no-op. +Given("the tech steps reference list has options", () => { + cy.intercept("GET", "**/reference/tech-steps", { + statusCode: 200, + body: [ + { id: 1, key: "cook" }, + { id: 3, key: "simmer" }, + ], + }); +}); + // The real first entry (`theMealDb`) mirrors what's actually seeded // (`reference-seed-data.ts`'s `registerAllRecipeSources`/ // `syncRecipeSources`); the second is illustrative only — a future diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index dcdca30..9cc6b9a 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -19,6 +19,10 @@ import { type SafeUserProfile, type SignupInput, type SourceView, + type StepTechStepCorrectionView, + type SubmitTechStepCorrectionInput, + type SubmitTechStepCorrectionResult, + type TechStepView, type ThemePreference, type UnitView, type UpdateRecipeInput, @@ -179,6 +183,11 @@ export class ApiClient { return this._request("/reference/units"); } + /** Reference list of detected cooking techniques — static, non-administrable (`TechStepCorrectionPopover`'s technique picker). Public — no session required. */ + public getTechSteps(): Promise { + return this._request("/reference/tech-steps"); + } + /** Reference list of implemented recipe sources (onboarding wizard's source step, `/parametres/foyer`) — empty until a concrete source is registered. Public — no session required. */ public getSources(): Promise { return this._request("/reference/sources"); @@ -267,6 +276,26 @@ 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. 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 { + return this._request(`/recipes/${recipeId}/steps/${stepId}/corrections`, { + method: "POST", + body: JSON.stringify(input), + }); + } + + /** Every correction submitted so far for `stepId`, most recent first. */ + public getTechStepCorrections( + recipeId: number, + stepId: number, + ): Promise { + return this._request(`/recipes/${recipeId}/steps/${stepId}/corrections`); + } + /** Fetches the current user's household (with its member list), or `null` if they don't have one yet. */ public getCurrentHouse(): Promise { return this._request("/house/current"); diff --git a/apps/web/src/features/recipes/RecipeDetailPanel.tsx b/apps/web/src/features/recipes/RecipeDetailPanel.tsx index 065fec7..47324e1 100644 --- a/apps/web/src/features/recipes/RecipeDetailPanel.tsx +++ b/apps/web/src/features/recipes/RecipeDetailPanel.tsx @@ -205,11 +205,34 @@ 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. {step.picture && } - + {/* Tied to `showActions` (not unconditionally on): that flag + already distinguishes a full recipe view from a lightweight + preview (`RecipePickerDialog`'s browsing step, `showActions={false}`) + — offering technique corrections in a quick "pick a recipe + for planning" preview would be more distracting than + useful there, even though the API itself allows it for any + viewer who can see the recipe. */} +
    2. ))}
diff --git a/apps/web/src/features/recipes/recipes.scss b/apps/web/src/features/recipes/recipes.scss index ed923a0..a9ed09d 100644 --- a/apps/web/src/features/recipes/recipes.scss +++ b/apps/web/src/features/recipes/recipes.scss @@ -627,12 +627,101 @@ } } +// 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 +// `TechStepCorrectionPopover.tsx`'s doc comment for why this renders inline +// in the document flow right below the step's own description instead of +// floating anchored at the selection. +.tech-step-correction-popover { + margin-top: var(--space-xs); + padding: var(--space-md); + background: var(--color-surface-alt); + border: 1px solid var(--color-border); + border-radius: var(--radius-md); + + &__selection { + font-weight: 600; + margin: 0 0 var(--space-sm); + } + + &__list { + display: flex; + flex-wrap: wrap; + gap: var(--space-xs); + list-style: none; + margin: 0 0 var(--space-sm); + padding: 0; + + button { + padding: 0.3rem 0.6rem; + font-size: var(--font-size-sm); + color: var(--color-text); + background: var(--color-surface); + border: 1px solid var(--color-border); + border-radius: var(--radius-pill); + cursor: pointer; + + &:hover:not(:disabled) { + background: color-mix(in srgb, var(--color-primary) 14%, transparent); + border-color: var(--color-primary); + } + + &:disabled { + opacity: 0.5; + cursor: not-allowed; + } + } + + // Nested (rather than a sibling `&__remove` block) so its border-color + // wins over the plain `button` rule above by class-count specificity, + // no `!important` needed. + .tech-step-correction-popover__remove { + color: var(--color-error); + border-color: var(--color-error); + } + } + + &__cancel { + background: none; + border: none; + color: var(--color-text-muted); + text-decoration: underline; + cursor: pointer; + padding: 0; + font-size: var(--font-size-sm); + } +} + // --- 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 7e5641e..042478f 100644 --- a/apps/web/src/features/recipes/steps/StepDescription.tsx +++ b/apps/web/src/features/recipes/steps/StepDescription.tsx @@ -1,8 +1,10 @@ -import type { StepTechStepView } from "@batch-cooking/shared"; -import { Fragment } 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"; /** * A recipe step's description, with every detected technique's exact @@ -23,48 +25,161 @@ import { splitDescriptionByTechSteps } from "./highlight-tech-steps"; * * `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} — + * see `use-text-selection.ts` for how a browser selection is translated + * into an absolute `[start, end)` span. When `!editable`, every segment + * 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, techSteps, + editable = false, + recipeId, + stepId, }: { description: string; techSteps: StepTechStepView[]; + /** Requires `recipeId`/`stepId` when `true` — omit (or leave `false`) for a read-only view with nothing real to correct against yet (e.g. `RecipeDetailPanel`'s `"loaded-draft"` unsaved-preview branch). */ + editable?: boolean; + recipeId?: number; + 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); + + const [activeCorrection, setActiveCorrection] = useState<{ + range: TextSelectionRange; + selectedText: string; + previousTechStepId: number | null; + } | null>(null); + + function handleMouseUp() { + if (!editable) return; + const range = getSelectionRange(); + if (!range) return; + setActiveCorrection({ + range, + selectedText: description.slice(range.start, range.end), + previousTechStepId: null, + }); + } + + function handleSubmitted(result: SubmitTechStepCorrectionResult) { + setLiveTechSteps(result.techSteps); + } + + // Tracks each segment's own absolute start offset into `description` as + // the map below walks them in order — segments are contiguous and cover + // the whole description (see `splitDescriptionByTechSteps`'s doc + // comment), so a running total is exact, no re-derivation needed. + let offset = 0; return ( -

- {segments.map((segment, index) => { - // 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. - const key = `${index}-${segment.text}`; - if (!segment.techStep) return {segment.text}; + <> +

+ {segments.map((segment, index) => { + const start = offset; + offset += segment.text.length; + // Captured now, not read as `offset` later inside a click + // handler below — `offset` keeps mutating for every subsequent + // segment this same `.map()` pass renders, so a closure + // referencing it directly would see its *final* value (the end + // of the whole description) whenever it actually fires, long + // after render — found via a real correction submitted with + // `end` far past this segment's own text. + const end = offset; + // 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`/`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.isKeyword) { - // Context-only run — rendered as plain text, same as a segment - // with no technique at all (see this component's doc comment for - // why the wider-clause highlight was turned back off). - return {segment.text}; - } - return ( - - {/* A real - - ); - })} -

+ if (!segment.techStep || !segment.isKeyword) { + // Context-only or plain run — rendered as plain text in + // read-only mode, same as before this component supported + // `editable` at all (see this component's doc comment for why + // the wider-clause highlight itself was turned back off). + // Editable mode still wraps it in a `data-offset` span so a + // selection starting/ending in plain text resolves correctly. + if (!editable) return {segment.text}; + return ( + + {segment.text} + + ); + } + + 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 + + ); + })} +

+ {editable && activeCorrection && recipeId !== undefined && stepId !== undefined && ( + setActiveCorrection(null)} + onSubmitted={handleSubmitted} + /> + )} + ); } diff --git a/apps/web/src/features/recipes/steps/TechStepCorrectionPopover.tsx b/apps/web/src/features/recipes/steps/TechStepCorrectionPopover.tsx new file mode 100644 index 0000000..57d9c09 --- /dev/null +++ b/apps/web/src/features/recipes/steps/TechStepCorrectionPopover.tsx @@ -0,0 +1,147 @@ +import { + ErrorCode, + type SubmitTechStepCorrectionResult, + type TechStepView, +} from "@batch-cooking/shared"; +import { useEffect, useRef, useState } from "react"; +import { useTranslation } from "react-i18next"; +import { ApiError, apiClient } from "../../../api/client"; +import { errorMessageService } from "../../../services/error-message.service"; +import type { TextSelectionRange } from "./use-text-selection"; + +/** + * Small non-modal popover letting a viewer assign a technique to a selected + * span of a step's description, or clear/relabel an existing match — + * opened by `StepDescription`'s editable mode. Same `mousedown`-outside- + * close pattern as `PlanningPage`'s `CalendarPopover`, not `Dialog.tsx`'s + * native `` — this is a small, contextual pick-one-option surface, + * not a page-blocking modal. + * + * Rendered inline right below the step's own description block (see + * `StepDescription.tsx`), not floating anchored at the selection's exact + * position — simpler and more robust than tracking a caret-anchored + * position across scroll/resize, at the cost of a little visual distance + * from the selected text itself. + * + * Submitting takes effect immediately — the API applies it to the step's + * real `StepTechStep` sequence as it records the correction (a `"manual"`- + * tagged entry, see `StepTechStepCorrection`'s schema doc comment) and + * returns the fresh sequence, which `onSubmitted` hands back to + * `StepDescription` to render right away, styled differently from an + * `"auto"` match. + */ +export function TechStepCorrectionPopover({ + recipeId, + stepId, + selectedText, + range, + previousTechStepId, + onClose, + onSubmitted, +}: { + recipeId: number; + stepId: number; + /** The selected span's own text — shown so the user confirms what they're tagging before picking a technique. */ + selectedText: string; + range: TextSelectionRange; + /** Set when correcting an already-detected match (opened from clicking its highlight) rather than a fresh selection — passed through as-is on submit, and offers a "remove" option `null` doesn't. */ + previousTechStepId: number | null; + onClose: () => void; + onSubmitted: (result: SubmitTechStepCorrectionResult) => void; +}) { + const { t } = useTranslation(); + const popoverRef = useRef(null); + const [techSteps, setTechSteps] = useState(null); + const [isSubmitting, setIsSubmitting] = useState(false); + const [error, setError] = useState(null); + + useEffect(() => { + let cancelled = false; + apiClient + .getTechSteps() + .then((list) => { + if (!cancelled) setTechSteps(list); + }) + .catch(() => { + if (!cancelled) setTechSteps([]); + }); + return () => { + cancelled = true; + }; + }, []); + + useEffect(() => { + function handleClickOutside(e: MouseEvent) { + if (popoverRef.current && !popoverRef.current.contains(e.target as Node)) { + onClose(); + } + } + document.addEventListener("mousedown", handleClickOutside); + return () => document.removeEventListener("mousedown", handleClickOutside); + }, [onClose]); + + async function submit(correctedTechStepId: number | null) { + setIsSubmitting(true); + setError(null); + try { + const result = await apiClient.submitTechStepCorrection(recipeId, stepId, { + start: range.start, + end: range.end, + previousTechStepId, + correctedTechStepId, + }); + onSubmitted(result); + onClose(); + } catch (err) { + const code = err instanceof ApiError ? err.code : ErrorCode.INTERNAL_ERROR; + setError(errorMessageService.getLabel(code)); + setIsSubmitting(false); + } + } + + return ( +
+

+ {t("recipes.techStepCorrection.selectionLabel", { text: selectedText })} +

+ {techSteps === null ? ( +

{t("recipes.loading")}

+ ) : ( +
    + {previousTechStepId !== null && ( +
  • + +
    • + )} + {techSteps.map((techStep) => ( +
  • + +
    • + ))} +
+ )} + {error &&

{error}

} + +
+ ); +} diff --git a/apps/web/src/features/recipes/steps/highlight-tech-steps.ts b/apps/web/src/features/recipes/steps/highlight-tech-steps.ts index 1e0d33b..f98bf52 100644 --- a/apps/web/src/features/recipes/steps/highlight-tech-steps.ts +++ b/apps/web/src/features/recipes/steps/highlight-tech-steps.ts @@ -18,6 +18,8 @@ export interface DescriptionSegment { techStep: StepTechStepView["techStep"] | null; /** Always `false` when `techStep` is `null`. */ isKeyword: boolean; + /** Mirrors the source `StepTechStepView.source` this segment came from — `null` when `techStep` is `null` (nothing to attribute a source to). See `StepDescription.tsx` for how `"auto"` vs `"manual"` render differently. */ + source: StepTechStepView["source"] | null; } /** @@ -26,34 +28,54 @@ export interface DescriptionSegment { * (the keyword) and, when present, `contextStart`/`contextEnd` (the wider * clause it was found in — see `StepTechStepView`, resolved server-side by * `tech-step-matcher.ts`'s `matchTechStepSpans`). An entry with no context - * (older data, saved before that column pair existed — see - * `StepTechStep`'s schema doc comment) degrades to a keyword-only segment, - * same as before context spans existed at all. + * (older data, saved before that column pair existed, or a manual + * correction — see `StepTechStep`'s schema doc comment) degrades to a + * keyword-only segment, same as before context spans existed at all. * * `techSteps` is expected already sorted by `start` (the API returns it in * `StepTechStep.order`, which *is* reading order — see that model's schema - * doc comment) but this re-sorts defensively (by context start when - * present, since context always starts at or before its own keyword) - * rather than assuming it, and silently drops any entry whose bounds don't - * make sense against `description` or a previously-accepted entry's own - * bounds — a malformed/out-of-date span degrades to "just don't highlight - * that one" rather than a garbled slice or a crash. + * doc comment) but this re-sorts defensively by each entry's own tight + * `start` rather than assuming it, and silently drops any entry whose own + * bounds don't make sense against `description` or a previously-accepted + * entry's own tight keyword span — a malformed/out-of-date span degrades to + * "just don't highlight that one" rather than a garbled slice or a crash. + * + * Two entries' tight keyword spans are never allowed to overlap (the later + * one is dropped, same as always), but two entries' wider *context* clauses + * are allowed to overlap each other and are silently clipped to make room — + * context is cosmetic only (`StepDescription.tsx` renders it identically to + * plain text) and must never cost a *different* entry its own real keyword + * highlight. This matters far more than it looks: a clause `splitIntoClauses` + * (`tech-step-matcher.ts`) found only one NER candidate in gets that + * candidate's context spanning the *entire* clause — often the entire + * description — so without clipping, a single auto-detected match anywhere + * in a step could silently swallow every manual correction added anywhere + * else in that same step's description, with no error, just an unstyled + * word in the rendered text. Found via live testing: a "simmer" match's + * whole-description context ate a manual "setAside" correction added to a + * later, otherwise-plain word in the same step. */ export function splitDescriptionByTechSteps( description: string, techSteps: StepTechStepView[], ): DescriptionSegment[] { - const sorted = [...techSteps].sort( - (a, b) => (a.contextStart ?? a.start) - (b.contextStart ?? b.start), - ); + const sorted = [...techSteps].sort((a, b) => a.start - b.start); - const segments: DescriptionSegment[] = []; - let cursor = 0; - for (const { techStep, start, end, contextStart, contextEnd } of sorted) { + // First pass: decide which entries survive at all, using only each + // entry's own tight keyword span for the cross-entry overlap check + // (`start < keywordCursor`) — the one thing two independent matches are + // never allowed to genuinely share. An entry's own context is still + // validated against its *own* keyword span here (`wideStart > start`, + // `end > wideEnd`, `wideEnd > description.length`) — a self-inconsistent + // span is dropped regardless of any other entry. + const valid: StepTechStepView[] = []; + let keywordCursor = 0; + for (const entry of sorted) { + const { start, end, contextStart, contextEnd } = entry; const wideStart = contextStart ?? start; const wideEnd = contextEnd ?? end; if ( - wideStart < cursor || + start < keywordCursor || wideStart > start || start >= end || end > wideEnd || @@ -61,12 +83,31 @@ export function splitDescriptionByTechSteps( ) { continue; } + valid.push(entry); + keywordCursor = end; + } + + const segments: DescriptionSegment[] = []; + let cursor = 0; + for (const [index, entry] of valid.entries()) { + const { techStep, start, end, contextStart, contextEnd, source } = entry; + const next = valid[index + 1]; + // Clipped against `cursor` (this entry can't render context over + // territory already emitted) and the next surviving entry's own tight + // `start` (this entry's context can't reach into a neighbor's real + // keyword span) — provably within `[cursor, start]`/`[end, next.start]` + // respectively given `valid`'s own non-overlapping-tight-span + // invariant from the first pass, so never produces a negative-length + // slice. + const wideStart = Math.max(contextStart ?? start, cursor); + const wideEnd = Math.min(contextEnd ?? end, next?.start ?? description.length); if (wideStart > cursor) { segments.push({ text: description.slice(cursor, wideStart), techStep: null, isKeyword: false, + source: null, }); } if (start > wideStart) { @@ -74,16 +115,27 @@ export function splitDescriptionByTechSteps( text: description.slice(wideStart, start), techStep, isKeyword: false, + source, }); } - segments.push({ text: description.slice(start, end), techStep, isKeyword: true }); + segments.push({ text: description.slice(start, end), techStep, isKeyword: true, source }); if (wideEnd > end) { - segments.push({ text: description.slice(end, wideEnd), techStep, isKeyword: false }); + segments.push({ + text: description.slice(end, wideEnd), + techStep, + isKeyword: false, + source, + }); } cursor = wideEnd; } if (cursor < description.length) { - segments.push({ text: description.slice(cursor), techStep: null, isKeyword: false }); + segments.push({ + text: description.slice(cursor), + techStep: null, + isKeyword: false, + source: null, + }); } return segments; } diff --git a/apps/web/src/features/recipes/steps/use-text-selection.ts b/apps/web/src/features/recipes/steps/use-text-selection.ts new file mode 100644 index 0000000..3d6da88 --- /dev/null +++ b/apps/web/src/features/recipes/steps/use-text-selection.ts @@ -0,0 +1,84 @@ +import { type RefObject, useCallback } from "react"; + +/** A `[start, end)` character range into a step's original `description` string — same convention as `StepTechStepView.start`/`end`. */ +export interface TextSelectionRange { + start: number; + end: number; +} + +/** + * Reads the browser's current text selection, translated into a + * {@link TextSelectionRange} into a step's original `description` string — + * the shape `POST /recipes/:id/steps/:stepId/corrections` expects (see + * `SubmitTechStepCorrectionInput`, `packages/shared`). + * + * Works by walking up from each end of the selection's `Range` to the + * nearest ancestor carrying a `data-offset` attribute — set by + * `StepDescription`'s editable mode on every {@link DescriptionSegment}'s + * own wrapping element (``/`