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..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; @@ -181,6 +179,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;