From bda84cf98173c2e540648cecabc6e93feff997d9 Mon Sep 17 00:00:00 2001 From: Vaishnavi Kale Date: Tue, 18 Aug 2026 14:44:31 -0700 Subject: [PATCH 1/2] fix(jd-match): make the Missing badge visible and clear capability on opt-out (#866 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings from the #866 review, verified against the merged #204 code before each was acted on. Two were user-facing defects; the rest close gaps that let the new code drift. The "Missing" badge was invisible. `StatusBadge`'s `neutral` tone filled with `bg-surface-subtle`, and `SemanticMatch`'s verdict rows use that same token, so every Missing pill rendered as bare small-caps text with no chip boundary while Met and Partial rendered normally — dropping the shape channel for the one status most worth flagging, and contradicting the component's own claim that status is carried three ways. A different fill cannot fix a SHARED badge that does not know its parent: `surface-hover` collides with `surface-subtle` in dark, `card-warm` collides in light, and `surface-card` collides on the card backgrounds most callers use. A border is visible against any fill, so `neutral` gains one. `border-strong` and not `border-light`, which is 1.13:1 on a subtle row in light and 1.00:1 in dark — the same bug a second time. `useJdMatch` never cleared `capability` on opt-out. The gating effect's cleanup set only a local `cancelled` flag, so the field kept its last probe result once a user had opted in even once, making its own docblock false and handing any future consumer that does not replicate the panel's `checked` gate a stale value. Clearing is a `setState`, not a probe: opting out still touches no WebGPU and fires no `webllm_capability_detected`. Cleanup runs before the next effect run's clear, so a probe that resolves after opt-out cannot restore a value. `SemanticAnalysisOptIn` was 215 LOC, over CLAUDE.md's decomposition guideline; its six-branch state machine moves to `SemanticAnalysisStatus` (89 + 185). The `checked` gate stays with the component that owns `checked`, so the extracted piece takes two props where the inline one took three. Its status branches are now a `switch` with a `never`-typed default: a sixth `JdMatchStatus` variant used to fall through to a bare `return null` with no compiler signal. `KeywordMatch` and `SemanticMatch` opened with byte-identical header markup — #204 created that honestly by moving the keyword body across verbatim, but nothing held the two together, so renaming "alpha" in one would leave the other behind and the panel would rename itself as a user toggled between views. `JdMatchHeader` shares the header and title row; each view keeps its own `Card` and body layout at its own call site, because the duplication worth removing is the copy, not the layout. `useJdMatch.keyword` is typed as the keyword arm rather than the whole union. It can only ever build that arm, and declaring the union cost `PasteJdPanel` a re-narrowing whose false branch was unreachable. `KeywordJdMatchResult` and `SemanticJdMatchResult` are declared beside the union and replace three hand-written `Extract<>` copies. `Disclosure`'s reuse-analysis docblock enumerates the hand-rolled `
` the shared primitive deliberately does not cover. It had drifted further than the review reported — `TargetingSection` and `ResultDetail` were missing too, so its opening count was already wrong — so the list now names `SemanticMatch`, says it is a record rather than a census, and carries the command to re-derive the real set. Not fixed here: the tailor button is still gated and steered by keyword coverage while a semantic verdict is on screen. That is tracked as #867 with a repro and an implementation plan, and wiring semantic verdicts into rewrite steering is a behaviour change rather than review cleanup. Refs #866, #204 --- src/components/features/JdMatchHeader.tsx | 48 +++++ src/components/features/KeywordMatch.tsx | 21 +- src/components/features/PasteJdPanel.tsx | 3 +- .../features/SemanticAnalysisOptIn.tsx | 154 ++------------- .../features/SemanticAnalysisStatus.tsx | 185 ++++++++++++++++++ .../features/SemanticMatch.test.tsx | 48 +++++ src/components/features/SemanticMatch.tsx | 21 +- src/design-system/shared/Disclosure.tsx | 24 ++- src/design-system/shared/StatusBadge.tsx | 25 ++- src/hooks/useJdMatch.test.tsx | 73 +++++++ src/hooks/useJdMatch.ts | 44 ++++- src/lib/jd-match/index.ts | 6 +- src/lib/jd-match/types.ts | 14 ++ 13 files changed, 479 insertions(+), 187 deletions(-) create mode 100644 src/components/features/JdMatchHeader.tsx create mode 100644 src/components/features/SemanticAnalysisStatus.tsx diff --git a/src/components/features/JdMatchHeader.tsx b/src/components/features/JdMatchHeader.tsx new file mode 100644 index 00000000..af5966af --- /dev/null +++ b/src/components/features/JdMatchHeader.tsx @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 The offlinecv Authors + +/** + * JdMatchHeader — the shared heading of the JD-match card (#866 review). + * + * `KeywordMatch` and `SemanticMatch` opened with a byte-identical `
` + + * title row: the `JD match` `

` and the `alpha` pill. #204 created that + * duplicate honestly — the keyword body was moved VERBATIM out of the old + * `JdMatch.tsx` and the semantic view was written to match it — but nothing + * held the two together afterwards, so renaming `alpha` (or retiring it, which + * is the likeliest edit) in one file would silently leave the other behind. + * The two views are peers rendered by the same router; a user toggling between + * them would see the panel rename itself. + * + * ## What it deliberately does NOT absorb + * + * The `` wrapper, the arm-specific headline and disclaimer paragraphs, + * and everything below the header stay in the views. The duplication worth + * removing is the COPY — the strings that must not drift — not the layout: a + * reader of `KeywordMatch` should still see its card chrome, its two-column + * grid and its own disclaimer without following an import. So this takes the + * arm's own header lines as `children` and adds only the title row above them, + * which keeps each view's `` visible at + * its own call site. + * + * Feature-area, not `@design-system`: one heading shared by two siblings in the + * same lane is not a design-system concern, and promoting it would mint a + * primitive with two callers and a hardcoded product string in it. + */ + +import type { ReactNode } from "react"; + +export function JdMatchHeader({ children }: { children: ReactNode }) { + return ( +
+
+

+ JD match +

+ + alpha + +
+ {children} +
+ ); +} diff --git a/src/components/features/KeywordMatch.tsx b/src/components/features/KeywordMatch.tsx index 3a1609ce..2ecbf879 100644 --- a/src/components/features/KeywordMatch.tsx +++ b/src/components/features/KeywordMatch.tsx @@ -28,29 +28,18 @@ */ import type { ExtractedTerm } from "../../lib/jd-match/extract-jd-terms.ts"; -import type { JdMatchResult } from "../../lib/jd-match"; +import type { KeywordJdMatchResult } from "../../lib/jd-match"; +import { JdMatchHeader } from "./JdMatchHeader.tsx"; import { Card } from "@design-system"; -/** The keyword arm of the union — same `Extract<…>` idiom `rank.ts` uses for - * `KeywordJdMatch`, so the two narrowings cannot drift. */ -type KeywordResult = Extract; - -export function KeywordMatch({ result }: { result: KeywordResult }) { +export function KeywordMatch({ result }: { result: KeywordJdMatchResult }) { const { coverage, terms, nounsDropped } = result; const total = terms.length; const covered = coverage.covered.length; return ( -
-
-

- JD match -

- - alpha - -
+

Your resume mentions {covered} of {total} terms from this JD.

@@ -66,7 +55,7 @@ export function KeywordMatch({ result }: { result: KeywordResult }) { Diagnostic, not a verdict. We look for skills and phrases by name — we don't read context. Your JD text stays in this browser tab.

-
+
- + {/* The `checked` gate stays HERE rather than moving into the status + component: this file owns `checked`, and passing it down only to + have the child early-return would be prop plumbing for nothing. + Unticked therefore renders no line at all, so the default panel is + unchanged from its pre-#204 self down to the DOM. */} + {checked && ( + + )}
); } - -/** Muted one-liner — the tone for "this is information, not a problem". */ -function Note({ children }: { children: ReactNode }) { - return ( -

- {children} -

- ); -} - -function StatusLine({ - checked, - status, - capability, -}: { - checked: boolean; - status: JdMatchStatus; - capability: WebGpuCapability | null; -}) { - // Unticked: render nothing at all, so the default panel is unchanged from - // its pre-#204 self down to the DOM. - if (!checked) return null; - // No JD yet (or one that extracted no terms) — there is nothing to analyze, - // so a progress line would be describing work that isn't happening. - if (status.kind === "idle") return null; - - // Capability first: with the probe unresolved OR resolved-unavailable, the - // hook's `status` is `ready` holding the KEYWORD result, which is - // indistinguishable from a semantic run that degraded. Only `capability` - // separates them, which is why the controller exposes it. - if (capability === null) { - return Checking whether this browser can run on-device analysis…; - } - if (capability !== "available") { - return ( - - This browser can't run on-device analysis (it needs WebGPU) — the - keyword coverage below is unaffected. - - ); - } - - if (status.kind === "loading") { - return ( - - ); - } - - if (status.kind === "running") { - // Generic on purpose. #204's example copy ("Judging requirement 4 of 9…") - // has nothing behind it: `judgeEvidence` takes no progress callback and - // `runLlmMatch` reports only engine load + a single `onInferenceStart`, so - // a count here would be invented. Adding a batch-progress API to the LLM - // layer to satisfy one string is not warranted; truthful copy is. - return ( -

- Reading this JD and checking it against your résumé… -

- ); - } - - if (status.kind === "error") { - // Not reachable from today's controller — a semantic run only starts when - // a keyword result already exists, and the hook's catch degrades to that - // rather than to `error` (see its state-machine docblock). Rendered anyway - // because the state is public API, and a UI that dropped it would blank - // the panel the day a semantic-only consumer reaches it. So the copy - // promises no keyword fallback: in that consumer there wouldn't be one. - // - // The controller's `message` is deliberately NOT rendered: its realistic - // source is a chunk-loader failure after a deploy, whose text is a hashed - // asset URL rather than anything a user can act on. The retry it offers is - // real — the hook clears an `error` slot on the way out of the semantic - // path, so re-ticking starts a fresh run. - return ( -

- On-device analysis couldn't start. Untick the box and tick it again to - retry. -

- ); - } - - // `ready` on the semantic path with a KEYWORD result: the run completed and - // `runLlmMatch` degraded internally (engine load failure, unparseable - // extraction, or a JD it found no requirements in). Note that a CANCELLED - // run never lands here — `useJdMatch` bumps its request id before aborting, - // so the abandoned run's keyword fallback fails the write guard and is never - // shown. That is what keeps this line off the screen on every opt-out, JD - // edit and model change (#803). - if (status.result.path === "keyword") { - return ( - - On-device analysis didn't return a verdict for this JD — showing keyword - coverage instead. - - ); - } - // Semantic verdicts are on screen in the card below; nothing to add. - return null; -} diff --git a/src/components/features/SemanticAnalysisStatus.tsx b/src/components/features/SemanticAnalysisStatus.tsx new file mode 100644 index 00000000..7c95c49b --- /dev/null +++ b/src/components/features/SemanticAnalysisStatus.tsx @@ -0,0 +1,185 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 The offlinecv Authors + +/** + * SemanticAnalysisStatus — the lifecycle line under the "Analyze with on-device + * AI" checkbox (#204, extracted in the #866 review follow-up). + * + * Split out of `SemanticAnalysisOptIn`, which had grown past CLAUDE.md's ~200 + * LOC decomposition guideline with this six-branch state machine as the obvious + * seam — the same split #204 itself applied when `JdMatch` became a router over + * `KeywordMatch`/`SemanticMatch`. The sibling keeps the control and the + * composition; this file holds the "what is happening right now" rendering and + * nothing else. No new props were invented to make the split work: it takes the + * two values it already read, and the `checked` gate stayed with the component + * that owns `checked`. + * + * ## Ordering is load-bearing + * + * `capability` is checked BEFORE the status switch, and inverting the two is a + * real defect rather than a style choice. With the probe unresolved OR resolved + * to something other than `available`, the controller's `status` is `ready` + * holding the KEYWORD result — the same shape a semantic run that degraded + * produces. Switch on `status` first and a no-WebGPU browser gets told the + * model "didn't return a verdict", which is a failure report for a run that was + * never attempted. + * + * ## Exhaustiveness + * + * The status branches are a `switch` with a `never`-typed default, not the + * if-chain this started as (#866 review). A sixth `JdMatchStatus` variant used + * to fall through to a bare `return null` with no compiler signal — the new + * state would silently render nothing. Now it fails `tsc`. Same fail-closed + * property `SemanticMatch`'s `Record` lookups have. + * + * ## No-WebGPU is not an error + * + * `WebGpuUnavailableNotice` is the repo's other answer to "no WebGPU", and it + * is the wrong one here: it renders a warning-toned strip with a how-to-enable + * dialog and fires `webllm_notice_shown`. #204 asks for the opposite — the + * keyword columns keep rendering, with at most one muted line saying why the + * box did nothing. A user whose browser can't run this still has the whole + * panel they came for, so nothing is in a failed state. + * + * ## Known limitation: the progress bar can sit at 0% (#804) + * + * `loadEngine`'s "already pending" fast path returns the shared promise + * without registering the new caller's `onProgress`, so only the FIRST caller + * for a model id ever receives progress. The reachable case here is + * self-inflicted rather than the cross-consumer one #804 describes — that one + * cites `job-search/sector.ts`'s `classifySector`, which has no production + * caller, and `/jobs/` has no other `loadEngine` caller at all. What IS + * reachable: opt in, then edit the JD while the weight download is still in + * flight. The superseded run owns `initProgressCallback`, its writes are + * dropped by the controller's id guard, and the new run joined a load it can't + * hear — so the bar reads 0% until the load resolves, then moves on to + * running → ready normally. + * + * Cosmetic, not functional, and deliberately NOT worked around here: the fix + * is a progress fan-out inside `web-llm.ts`, shared by every WebLLM surface in + * the repo, which is #804's scope and not this component's. The reason it is + * tolerable meanwhile is the keyword floor — the result card below keeps + * showing full coverage throughout, so a stalled bar costs the user a progress + * readout, never the answer they came for. + */ + +import type { ReactNode } from "react"; +import { ModelLoadProgress } from "@design-system"; +import type { JdMatchStatus } from "../../hooks/useJdMatch.ts"; +import type { WebGpuCapability } from "../../lib/webllm/types.ts"; + +interface SemanticAnalysisStatusProps { + /** The controller's semantic status. */ + status: JdMatchStatus; + /** The controller's WebGPU probe result; `null` until it resolves, and + * `null` again once the user opts back out (the hook clears it). */ + capability: WebGpuCapability | null; +} + +/** Muted one-liner — the tone for "this is information, not a problem". */ +function Note({ children }: { children: ReactNode }) { + return ( +

+ {children} +

+ ); +} + +export function SemanticAnalysisStatus({ + status, + capability, +}: SemanticAnalysisStatusProps) { + // No JD yet (or one that extracted no terms) — there is nothing to analyze, + // so a progress line would be describing work that isn't happening. Narrows + // `status` for the switch below, which is why the switch needs no `idle` arm. + if (status.kind === "idle") return null; + + // Capability first — see the docblock. Not foldable into the switch. + if (capability === null) { + return Checking whether this browser can run on-device analysis…; + } + if (capability !== "available") { + return ( + + This browser can't run on-device analysis (it needs WebGPU) — the + keyword coverage below is unaffected. + + ); + } + + switch (status.kind) { + case "loading": + return ( + + ); + + case "running": + // Generic on purpose. #204's example copy ("Judging requirement 4 of 9…") + // has nothing behind it: `judgeEvidence` takes no progress callback and + // `runLlmMatch` reports only engine load + a single `onInferenceStart`, so + // a count here would be invented. Adding a batch-progress API to the LLM + // layer to satisfy one string is not warranted; truthful copy is. + return ( +

+ Reading this JD and checking it against your résumé… +

+ ); + + case "error": + // Not reachable from today's controller — a semantic run only starts when + // a keyword result already exists, and the hook's catch degrades to that + // rather than to `error` (see its state-machine docblock). Rendered anyway + // because the state is public API, and a UI that dropped it would blank + // the panel the day a semantic-only consumer reaches it. So the copy + // promises no keyword fallback: in that consumer there wouldn't be one. + // + // The controller's `message` is deliberately NOT rendered: its realistic + // source is a chunk-loader failure after a deploy, whose text is a hashed + // asset URL rather than anything a user can act on. The retry it offers is + // real — the hook clears an `error` slot on the way out of the semantic + // path, so re-ticking starts a fresh run. + return ( +

+ On-device analysis couldn't start. Untick the box and tick it again to + retry. +

+ ); + + case "ready": + // A KEYWORD result here means the run completed and `runLlmMatch` degraded + // internally (engine load failure, unparseable extraction, or a JD it + // found no requirements in). Note that a CANCELLED run never lands here — + // `useJdMatch` bumps its request id before aborting, so the abandoned + // run's keyword fallback fails the write guard and is never shown. That is + // what keeps this line off the screen on every opt-out, JD edit and model + // change (#803). + // + // A semantic result needs no line: the verdicts are in the card below. + return status.result.path === "keyword" ? ( + + On-device analysis didn't return a verdict for this JD — showing + keyword coverage instead. + + ) : null; + + default: { + // Compile-time exhaustiveness (#866 review). A sixth `JdMatchStatus` + // variant makes this assignment an error instead of silently rendering + // nothing. Returned rather than merely declared because `noUnusedLocals` + // is on; `never` is assignable to `ReactNode`, and the branch is + // unreachable for the five variants that exist, so this adds no runtime + // behaviour — the repo has no `assertNever` helper to reuse, and one + // call site does not justify minting a shared one. + const unhandled: never = status; + return unhandled; + } + } +} diff --git a/src/components/features/SemanticMatch.test.tsx b/src/components/features/SemanticMatch.test.tsx index a4c10bac..feb4e2e2 100644 --- a/src/components/features/SemanticMatch.test.tsx +++ b/src/components/features/SemanticMatch.test.tsx @@ -181,6 +181,54 @@ describe("SemanticMatch row content", () => { expect(rowTexts[3].startsWith("Missing")).toBe(true); }); + it("keeps every status badge visible against its own row (#866 review)", () => { + // The defect this pins: `neutral` filled with `bg-surface-subtle`, which is + // ALSO the `
  • `'s fill, so the "Missing" pill had no boundary and + // rendered as bare text while "Met"/"Partial" rendered as pills. That + // silently dropped the shape channel for the one status most worth + // flagging, contradicting the component's "never by colour alone" claim. + // + // Asserted as the INVARIANT rather than as a literal class string: a badge + // whose fill matches its row's fill must carry a border, whichever tone it + // is and whatever the tokens are renamed to later. + const el = render(MIXED); + const rows = [...el.querySelectorAll("li")]; + expect(rows.length).toBeGreaterThan(0); + + const fill = (cls: string): string | undefined => + cls.split(/\s+/).find((c) => c.startsWith("bg-")); + const hasBorder = (cls: string): boolean => + cls.split(/\s+/).some((c) => c === "border" || c.startsWith("border-")); + + for (const row of rows) { + const badge = row.querySelector("span[class*='rounded-full']"); + expect(badge).toBeTruthy(); + const badgeCls = badge?.className ?? ""; + if (fill(badgeCls) === fill(row.className)) { + expect( + hasBorder(badgeCls), + `badge "${badge?.textContent}" shares its row's ${fill(row.className)} fill, so it needs a border to stay visible`, + ).toBe(true); + } + } + }); + + it("gives the Missing badge a boundary that does not read as a warning", () => { + const el = render(MIXED); + const missingRow = [...el.querySelectorAll("li")].find((li) => + li.textContent?.startsWith("Missing"), + ); + const badge = missingRow?.querySelector("span[class*='rounded-full']"); + const cls = badge?.className ?? ""; + // Visible: it carries a border. + expect(cls).toMatch(/\bborder\b/); + // …and still neutral — no feedback/warning/error colouring, so an unmet + // requirement is not framed as a fault. + expect(cls).not.toMatch(/feedback-(warning|error)/); + // The word survives regardless of any of the above. + expect(badge?.textContent).toBe("Missing"); + }); + it("shows the headline tally from the pre-computed summary", () => { const text = render(MIXED).textContent ?? ""; expect(text).toContain("2 met · 1 partial · 1 missing"); diff --git a/src/components/features/SemanticMatch.tsx b/src/components/features/SemanticMatch.tsx index 9120d5c9..0722aa7a 100644 --- a/src/components/features/SemanticMatch.tsx +++ b/src/components/features/SemanticMatch.tsx @@ -47,13 +47,10 @@ */ import { Card, StatusBadge, type StatusBadgeTone } from "@design-system"; -import type { JdMatchResult } from "../../lib/jd-match"; +import type { SemanticJdMatchResult } from "../../lib/jd-match"; +import { JdMatchHeader } from "./JdMatchHeader.tsx"; import type { RequirementVerdict } from "../../lib/jd-match/llm/judge-evidence.ts"; -/** The semantic arm of the union — same `Extract<…>` idiom `rank.ts` uses for - * `KeywordJdMatch`, so the two narrowings cannot drift. */ -type SemanticResult = Extract; - type VerdictStatus = RequirementVerdict["status"]; /** Reading order of the groups. Typed as the verdict-status union (not @@ -83,20 +80,12 @@ const GROUP_BADGE_TONE: Record = { missing: "neutral", }; -export function SemanticMatch({ result }: { result: SemanticResult }) { +export function SemanticMatch({ result }: { result: SemanticJdMatchResult }) { const { verdicts, summary } = result; return ( -
    -
    -

    - JD match -

    - - alpha - -
    + {/* This is the headline `SemanticMatchSummary` was added for — its docblock in `jd-match/types.ts` spells the shape out. Tallied by `runLlmMatch`, so the view never re-counts the verdict list. */} @@ -113,7 +102,7 @@ export function SemanticMatch({ result }: { result: SemanticResult }) { against your résumé and can get it wrong — check the evidence. Your JD text stays in this browser tab.

    -
    + {GROUP_ORDER.map((status) => { const group = verdicts.filter((verdict) => verdict.status === status); diff --git a/src/design-system/shared/Disclosure.tsx b/src/design-system/shared/Disclosure.tsx index 6fb46eb4..5dca788f 100644 --- a/src/design-system/shared/Disclosure.tsx +++ b/src/design-system/shared/Disclosure.tsx @@ -15,13 +15,23 @@ * state variable.** If a caller needs render-on-demand, it needs a different * component, not a prop here. * - * Reuse analysis (CLAUDE.md 3-tier rule). Six hand-rolled `
    ` already - * exist and none of them is this: the three things they lack are the `count` - * badge slot, the `warn` mark, and a summary row that clears the 44×44 touch - * floor. Five are feature code (`Result`, `WebGpuUnavailableNotice`, - * `ModelSelector`, `RewriteReviewList`, `AtsScoreReadout`) — one-line "why did - * this happen?" toggles with no state to carry, and converting them is an - * explicit #823 non-goal. + * Reuse analysis (CLAUDE.md 3-tier rule). Hand-rolled `
    ` already exist + * elsewhere and none of them is this: the three things they lack are the + * `count` badge slot, the `warn` mark, and a summary row that clears the 44×44 + * touch floor. Most are feature code (`Result`, `WebGpuUnavailableNotice`, + * `ModelSelector`, `RewriteReviewList`, `AtsScoreReadout`, `TargetingSection`, + * `ResultDetail`, and `SemanticMatch`'s per-verdict Evidence toggle from #204) + * — one-line "why did this happen?" toggles with no state to carry, and + * converting them is an explicit #823 non-goal. + * + * Treat that list as a record, NOT a census: it is maintained by hand and has + * drifted before — `SemanticMatch` was added in the #866 review follow-up, + * which is also when `TargetingSection` and `ResultDetail` turned out to be + * missing and the count that used to open this paragraph turned out to be + * wrong. A batch-conversion sweep should re-derive the real set rather than + * trust the names here: + * + * rg -l ' = { limited: "bg-feedback-warning-bg text-feedback-warning-text", warning: "bg-feedback-warning-bg text-feedback-warning-text", info: "bg-feedback-info-bg text-feedback-info-text", - neutral: "bg-surface-subtle text-content-muted", + neutral: "border border-border-strong bg-surface-subtle text-content-muted", }; export function StatusBadge({ diff --git a/src/hooks/useJdMatch.test.tsx b/src/hooks/useJdMatch.test.tsx index b7fa2ce3..3823f65a 100644 --- a/src/hooks/useJdMatch.test.tsx +++ b/src/hooks/useJdMatch.test.tsx @@ -220,6 +220,79 @@ describe("useJdMatch — keyword-only path touches NO WebLLM machinery (#203)", expect(detectWebGpuMock).not.toHaveBeenCalled(); }); + it("clears `capability` back to null on opt-out, with no new probe (#866 review)", async () => { + // The defect: the gating effect's cleanup only set a local `cancelled` + // flag, so `capability` kept the last probe result forever once a user had + // opted in even once — making the field's own docblock ("stays `null` for a + // keyword-only consumer") false, and handing any future consumer that + // doesn't replicate `SemanticAnalysisOptIn`'s `checked` gate a stale value. + webgpu = "available"; + await mount({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: true }); + flushDebounce(); + await flushMicrotasks(); + expect(latestCapability).toBe("available"); + const probesAfterOptIn = detectWebGpuMock.mock.calls.length; + + update({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: false }); + await flushMicrotasks(); + + expect(latestCapability).toBeNull(); + // Clearing is a setState, not a probe — opting out must touch no WebGPU + // and fire no `webllm_capability_detected`. + expect(detectWebGpuMock.mock.calls.length).toBe(probesAfterOptIn); + }); + + it("a probe that resolves AFTER opt-out cannot restore capability (#866 review)", async () => { + // The race the `cancelled` flag has to win: opt in, opt out again before + // `detectWebGpu` settles, then let it settle. Cleanup runs before the next + // effect run's clear, so the late `.then` is already cancelled and the + // field stays null instead of flipping back to a value for a user who is + // no longer opted in. + let settle: (c: "available") => void = () => {}; + detectWebGpuMock.mockImplementationOnce( + () => + new Promise<"available">((resolve) => { + settle = resolve; + }), + ); + + await mount({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: true }); + flushDebounce(); + await flushMicrotasks(); + // Probe started but deliberately unresolved. + expect(latestCapability).toBeNull(); + + update({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: false }); + await flushMicrotasks(); + expect(latestCapability).toBeNull(); + + // The abandoned probe lands late. + await act(async () => { + settle("available"); + await Promise.resolve(); + await Promise.resolve(); + }); + expect(latestCapability).toBeNull(); + }); + + it("re-detects on opt back in after an opt-out cleared the value", async () => { + // Clearing must not strand the field: ticking again has to be able to + // produce a value, or the semantic path could never activate a second time. + webgpu = "available"; + await mount({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: true }); + flushDebounce(); + await flushMicrotasks(); + expect(latestCapability).toBe("available"); + + update({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: false }); + await flushMicrotasks(); + expect(latestCapability).toBeNull(); + + update({ parsed: SPARSE_RESUME, jdText: JD_TEXT, semanticOptIn: true }); + await flushMicrotasks(); + expect(latestCapability).toBe("available"); + }); + it("surfaces the probe result on `capability` once opted in (#204)", async () => { for (const detected of ["available", "no-webgpu", "unsupported-os"] as const) { webgpu = detected; diff --git a/src/hooks/useJdMatch.ts b/src/hooks/useJdMatch.ts index 6ea17cb3..fa4c5caa 100644 --- a/src/hooks/useJdMatch.ts +++ b/src/hooks/useJdMatch.ts @@ -145,7 +145,10 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { detectWebGpu } from "../lib/webllm/capability.ts"; import { computeCoverage } from "../lib/jd-match/coverage.ts"; import { extractJdTerms } from "../lib/jd-match/extract-jd-terms.ts"; -import type { JdMatchResult } from "../lib/jd-match"; +import type { + JdMatchResult, + KeywordJdMatchResult, +} from "../lib/jd-match"; import type { HeuristicParsedResume } from "../lib/heuristics/types.ts"; import type { ProgressUpdate, @@ -206,8 +209,15 @@ export interface JdMatchController { * CONTROLLER, not of `status`: render `keyword` as the floor and let * `status` layer the semantic refinement on top. `null` only when the JD is * empty/degenerate — exactly when `status` is `idle`. + * + * Typed as the KEYWORD ARM, not the whole union (#866 review). `keywordResult` + * below can only ever build `{ path: "keyword", … }` or `null`, so declaring + * the union let the compiler forget an invariant the code guarantees — and + * `PasteJdPanel` paid for it with a `keyword?.path === "keyword" ? … : null` + * re-narrowing that could never take its false branch. Naming the arm moves + * that from a runtime check to a compile-time fact. */ - keyword: JdMatchResult | null; + keyword: KeywordJdMatchResult | null; /** * The detected WebGPU capability, or `null` while detection has not run or * has not resolved. Read-only view of the hook's own probe — the consumer @@ -304,8 +314,34 @@ export function useJdMatch(options: UseJdMatchOptions): JdMatchController { // `navigator.gpu.requestAdapter()` runs on the `/jobs/` page. `detectWebGpu` // caches its result per page, so an opted-in flip re-uses a cached probe // if one exists. `cancelled` guards against a write after unmount. + // + // Opting out CLEARS the value rather than merely stopping (#866 review). + // Leaving it set made `capability`'s own docblock false the moment a user + // toggled off — it claims the field stays `null` for a keyword-only + // consumer, and every consumer that doesn't replicate `SemanticAnalysisOptIn`'s + // "check `checked` before reading `capability`" gate would have shown a + // stale probe result to an opted-out user. Now the field means what it says + // on its own, without a companion flag. + // + // Three things this deliberately does NOT do: + // - It does not probe. `setCapability(null)` is a state write; the + // `detectWebGpu()` call is on the other side of the early return, so + // opting out still touches no WebGPU and fires no analytics. + // - It does not re-render on mount. React bails out of a `setState` that + // is `Object.is`-equal to the current value, and the initial value is + // already `null`, so the keyword-only path costs one bail-out and no + // commit. + // - It does not race the in-flight probe. Cleanup sets `cancelled` before + // the next effect run's write, so an opt-out mid-probe cancels the + // pending `.then` first and clears second — a late resolve cannot + // restore a value after opt-out. Re-opting in calls `detectWebGpu()` + // again, which returns its cached promise: no second `requestAdapter()` + // and no duplicate funnel event. useEffect(() => { - if (!semanticOptIn) return; + if (!semanticOptIn) { + setCapability(null); + return; + } let cancelled = false; void detectWebGpu().then((c) => { if (!cancelled) setCapability(c); @@ -327,7 +363,7 @@ export function useJdMatch(options: UseJdMatchOptions): JdMatchController { // trimming rules to the pre-#203 `PasteJdPanel` inline `useMemo`, so a // keyword-only consumer's status becomes `ready` on the SAME commit as // `debouncedJdText` changes. - const keywordResult = useMemo(() => { + const keywordResult = useMemo(() => { if (trimmedJdText.length === 0) return null; const extracted = extractJdTerms(trimmedJdText); if (extracted.all.length === 0) return null; diff --git a/src/lib/jd-match/index.ts b/src/lib/jd-match/index.ts index efbdd3a5..6d75fe85 100644 --- a/src/lib/jd-match/index.ts +++ b/src/lib/jd-match/index.ts @@ -25,4 +25,8 @@ export type { AtsPlatform } from "./fetch-jd.ts"; export { htmlToPlaintext } from "./html-to-plaintext.ts"; -export type { JdMatchResult } from "./types.ts"; +export type { + JdMatchResult, + KeywordJdMatchResult, + SemanticJdMatchResult, +} from "./types.ts"; diff --git a/src/lib/jd-match/types.ts b/src/lib/jd-match/types.ts index 5f2257b7..6278c8ef 100644 --- a/src/lib/jd-match/types.ts +++ b/src/lib/jd-match/types.ts @@ -47,3 +47,17 @@ export type JdMatchResult = verdicts: readonly RequirementVerdict[]; summary: SemanticMatchSummary; }; + +/** + * The two arms, named once (#866 review). + * + * `Extract` was being written out at each site that + * needed one arm — `KeywordMatch`, `SemanticMatch`, and `job-search/rank.ts`'s + * own `KeywordJdMatch` — so the idiom was duplicated rather than shared, and a + * producer whose type is narrower than its declaration (`useJdMatch`'s + * `keyword`, which by construction only ever builds the keyword arm) had no + * name to say so with. Declared HERE, beside the union, so a consumer never has + * to reach into another lane for the type of a value this module produced. + */ +export type KeywordJdMatchResult = Extract; +export type SemanticJdMatchResult = Extract; From 5c96a537a18c2507302523f73e295c4a29600433 Mon Sep 17 00:00:00 2001 From: Srinivas Annam Date: Tue, 18 Aug 2026 15:09:00 -0700 Subject: [PATCH 2/2] fix(review): address review nits --- src/components/features/SemanticMatch.test.tsx | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/components/features/SemanticMatch.test.tsx b/src/components/features/SemanticMatch.test.tsx index feb4e2e2..7948a806 100644 --- a/src/components/features/SemanticMatch.test.tsx +++ b/src/components/features/SemanticMatch.test.tsx @@ -17,14 +17,12 @@ import { act } from "react"; import { createRoot, type Root } from "react-dom/client"; import { SemanticMatch } from "./SemanticMatch.tsx"; -import type { JdMatchResult } from "../../lib/jd-match"; +import type { SemanticJdMatchResult } from "../../lib/jd-match"; import type { RequirementVerdict } from "../../lib/jd-match/llm/judge-evidence.ts"; (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true; -type SemanticResult = Extract; - function verdict( id: string, text: string, @@ -42,7 +40,7 @@ function verdict( /** Wrap verdicts in the semantic arm, tallying the summary the way * `runLlmMatch` does so the header's numbers are the real ones. */ -function semantic(verdicts: readonly RequirementVerdict[]): SemanticResult { +function semantic(verdicts: readonly RequirementVerdict[]): SemanticJdMatchResult { let met = 0; let partial = 0; let missing = 0; @@ -61,7 +59,7 @@ function semantic(verdicts: readonly RequirementVerdict[]): SemanticResult { let container: HTMLDivElement | undefined; let root: Root | undefined; -function render(result: SemanticResult): HTMLDivElement { +function render(result: SemanticJdMatchResult): HTMLDivElement { const el = document.createElement("div"); document.body.appendChild(el); container = el;