fix(jd-match): make the Missing badge visible and clear capability on opt-out (#866 review)

[Vaishnavi1709]

Summary

Seven of the eight findings from the #866 review addendum. Each was re-verified against the merged #204 code before being acted on; all seven were still real, and one turned out to be worse than reported. The eighth is deferred — see below.

Two user-facing defects:

• 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/Partial rendered normally — dropping the shape channel for the status most worth flagging, and contradicting the component's own "carried three ways, never by colour alone" claim. A different fill can't fix a shared badge that doesn't know its parent: surface-hover collides with surface-subtle in dark, card-warm collides in light, surface-card collides on the card backgrounds most callers use. So neutral gains a border, which is visible against any fill. border-strong rather than border-light — the latter is 1.13:1 on a subtle row in light and 1.00:1 in dark, i.e. the identical bug a second time.
• capability never reset 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 doesn't 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 that keeps the new code from drifting: SemanticAnalysisOptIn 215 → 89 LOC (state machine extracted to SemanticAnalysisStatus, now an exhaustive switch with a never default); a shared JdMatchHeader so the two peer views can't rename the panel out from under each other; useJdMatch.keyword typed as the keyword arm, deleting a re-narrowing whose false branch was unreachable; Disclosure's <details> registry updated.

Refs #866, #204

Deferred

The tailor button is still gated and steered by keyword coverage while a semantic verdict is on screen. Tracked as #867, which already carries a repro and an implementation plan. Not folded in here: wiring semantic verdicts into rewrite steering is a behaviour change needing its own buildJdRewriteContextFromVerdicts, not review cleanup. Tailor behaviour in this PR is byte-identical to what shipped in #866.

Review focus

• src/design-system/shared/StatusBadge.tsx:47 — neutral is now the one tone carrying a border, so it's shaped differently from its four siblings. Is an outlined chip beside filled ones the right call, or should all five gain the border?
• src/hooks/useJdMatch.ts:~310 — setCapability(null) runs on every keyword-only mount, relying on React's Object.is bail-out to cost nothing. Is that the right trade against gating it behind a ref?
• src/components/features/SemanticAnalysisStatus.tsx:~70 — the capability checks sit deliberately before the status switch, and inverting them makes a no-WebGPU browser read "didn't return a verdict" for a run never attempted. Is that ordering constraint obvious enough at the callsite to survive a later edit?
• src/lib/jd-match/types.ts:51 — the new named arms don't retype job-search/rank.ts's own KeywordJdMatch, which is still a separate Extract<>. Left alone as another lane's API — right call, or should it point at the canonical one?

Test plan

• npm run typecheck clean
• npm run lint clean
• npm run verify green (also enforced by the pre-push hook on this branch)
• npm test full suite green — 367 files / 5965 tests, 0 failures
• npx fallow audit --base origin/main — no issues in 13 changed files (the duplication report is flagged inherited, pre-existing in useJdMatch.test.tsx)
• No JD-match: JdMatch.tsx semantic verdict UI + opt-in toggle + keyword fallback view #204 regression: KeywordMatch.parity.test.tsx — whose golden strings were captured from the pre-JD-match: JdMatch.tsx semantic verdict UI + opt-in toggle + keyword fallback view #204 component — still passes, so the shared-header extraction is byte-identical output. The 24 PasteJdPanel.semantic tests and the 11 SemanticAnalysisOptIn tests pass unmodified
• JD-match semantic runs are hidden but never cancelled — abandoned runs stack on the shared MLCEngine #803 untouched — diffed for every cancellation symbol (controllerRef, AbortController, abort, requestIdRef, setSlotIfCurrent, mountedRef, queueMicrotask): zero hits; src/lib/jd-match/llm/ unchanged
• Fail-before validation — each fix confirmed to be caught by its test: removing the capability reset fails 2 tests; restoring the same-fill badge fails 2; reverting the narrowed type fails typecheck (Property 'coverage' does not exist); and adding a 6th JdMatchStatus variant errors under the new switch (TS2322 … not assignable to type 'never') while the old if-chain compiled silently

[cloudflare-workers-and-pages]

Deploying offlinecv with    Cloudflare Pages

Latest commit:	5c96a53
Status:	✅  Deploy successful!
Preview URL:	https://5da235ec.offlinecv.pages.dev
Branch Preview URL:	https://fix-jd-match-post-866-review.offlinecv.pages.dev

View logs

[s-annam]

Stance: APPROVE. 0 Blocking findings → APPROVE per the verdict rule (0 Blocking → APPROVE regardless of Secondary/Nits).

This is a tight, well-scoped follow-up to the #866 review addendum — seven of its eight findings addressed, the eighth (tailor-button gating) correctly deferred to #867 with a real, open, already-scoped issue rather than folded in as a behaviour change. Verified independently:

• Missing badge fix (StatusBadge.tsx:47): neutral gains border-border-strong. Confirmed neutral has exactly one caller (SemanticMatch), so no other StatusBadge consumer is affected. Re-derived the contrast math in the new docblock by hand from tokens.css's actual hex values — border-light on bg-subtle really is ~1.12:1 in light and exactly 1.00:1 in dark (--color-border-light and --color-bg-subtle are the literal same hex, #334155, in the dark palette), and border-strong holds ~2.2–2.3:1 in both themes. The claims round-trip to the tokens, not just to prose.
• capability reset on opt-out (useJdMatch.ts:~314): traced the effect's cleanup/re-run ordering by hand — React runs the previous effect's cleanup (cancelled = true) before the new effect body's setCapability(null), so a probe that resolves after opt-out is provably dropped, not just usually dropped. The three new tests (immediate clear, late-resolve race, re-opt-in) pin exactly that sequence and all pass.
• SemanticAnalysisOptIn LOC: 215 → 89, extracted state machine now 185 LOC in its own file, both under the ~200 LOC guideline. The extraction is a faithful lift — every branch, comment and copy string survived — and the old silent-fallthrough if-chain became a switch with a never-typed default, which I confirmed is truly unreachable (JdMatchStatus has exactly 5 variants, all 5 handled).
• Description accuracy (gate 3f): every checkable claim in the Summary/Test plan/Review-focus round-trips — test counts (11 SemanticAnalysisOptIn + 3 KeywordMatch.parity unmodified, 53 total across the two touched test files) match what I ran; rank.ts's own KeywordJdMatch really is untouched (confirmed via grep), matching the Review-focus question about it rather than a silent miss.
• Gates: npm run typecheck/lint clean, npx fallow audit --base origin/main → "No issues in 13 changed files" (the 14 clone-group warning is pre-existing useJdMatch.test.tsx duplication, excluded as inherited — not attributable to this diff), no fixtures touched (3a n/a), no raw elements/hardcoded colors in the diff (3b/3c clean), no skill/script files touched (3e n/a).

AC checklist

No issue is formally Closesd by this PR (title/body Refs #866, #204). Judged against the #866 review addendum's own 8 findings instead: 7 addressed and verified above (badge, capability reset, LOC split, exhaustive switch, shared header, Disclosure docblock, the Extract<> naming), 1 correctly deferred to open issue #867 (verified it exists, is open, and already carries a repro + plan).

Secondary

None.

Nits

1. src/lib/job-search/rank.ts:70 — still declares its own local KeywordJdMatch = Extract<JdMatchResult, {path:"keyword"}> rather than importing this PR's new KeywordJdMatchResult. Already surfaced by the author as an open question in ## Review focus, not a silent miss — my answer: leave it. rank.ts is a different lane (job-search) with its own established name; pointing it at the jd-match type is a fine future cleanup but isn't required by this PR's stated scope, and types.ts's new docblock doesn't actually claim rank.ts was migrated (it names the problem history, not a completed consolidation) — so no docblock/code mismatch here.
2. The per-arm disclaimer paragraphs in KeywordMatch.tsx/SemanticMatch.tsx still duplicate the trailing "...JD text stays in this browser tab" sentence — JdMatchHeader's own docblock explicitly scopes itself to the title row only and leaves the body paragraphs to each view on purpose ("the <Card> wrapper... and everything below the header stay in the views"). Deliberate, documented scope cut, not an oversight — flagging only so it isn't lost if a future copy pass touches one and not the other.
3. Two of the new SemanticMatch.test.tsx tests (badge-visibility invariant + Missing-badge-specific check) verify overlapping ground; and one new useJdMatch.test.tsx test repeats the mount/opt-in/opt-out setup of its immediate predecessor rather than extending it. Cosmetic test-authoring style, not worth a re-run.

Fixed in 5c96a53

• src/components/features/SemanticMatch.test.tsx locally redeclared type SemanticResult = Extract<JdMatchResult, {path:"semantic"}> instead of importing this PR's own new SemanticJdMatchResult — the exact duplication pattern this PR exists to remove, just in test-only code. Swapped to the import (3 call sites) and dropped the now-unused JdMatchResult import. Non-behavioral; npm run verify green afterward. Landed as a plain push (not a collapse) since this is a named contributor's in-repo branch, not a maintainer/agent-authored one — force-pushing to rewrite it isn't this run's call to make. The branch is now two commits; a maintainer merging it may want /collapse-pr first depending on this repo's one-commit-per-PR norm, but I'm not rewriting someone else's branch to enforce that myself.

---

Reviewed by: Claude Sonnet 5 (high)