diff --git a/apps/web/cypress/component/highlight-tech-steps.cy.tsx b/apps/web/cypress/component/highlight-tech-steps.cy.tsx index 7eaa4b1..f06a6c4 100644 --- a/apps/web/cypress/component/highlight-tech-steps.cy.tsx +++ b/apps/web/cypress/component/highlight-tech-steps.cy.tsx @@ -140,6 +140,29 @@ describe("splitDescriptionByTechSteps", () => { ]); }); + 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 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 3edbf18..f98bf52 100644 --- a/apps/web/src/features/recipes/steps/highlight-tech-steps.ts +++ b/apps/web/src/features/recipes/steps/highlight-tech-steps.ts @@ -28,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, source } 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 || @@ -63,6 +83,24 @@ 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({