feat(jd-match): semantic verdict UI, on-device opt-in, keyword fallback view (#204) - #866
Conversation
…ck view (#204) `JdMatch.tsx` returned `null` for `path: "semantic"`, so a finished on-device match rendered a blank panel, and `semanticOptIn` had no route to a user — `PasteJdPanel` called `useJdMatch` without it, hardwiring the default `false`. This closes both. `JdMatch` becomes a router on the `JdMatchResult` discriminant. The keyword early-return leaves the semantic arm as the only remaining type, so the fall-through type-checks because TypeScript proved it; a third arm would break this file at compile time rather than fall into the semantic view. The pre-#204 body moves to `KeywordMatch` verbatim — same elements, classes, copy, empty states and `title` snippets. It is the default experience for every user who never opts in AND the fallback floor for every user who does, so `KeywordMatch.parity.test.tsx` pins its markup against golden strings captured by rendering the OLD component, not by snapshotting the new one. `SemanticMatch` groups verdicts Met → Partial → Missing in a fixed reading order, omitting empty groups. Status is carried three ways and never by colour alone: the group heading's word, a per-row `StatusBadge`, and the heading tint. `StatusBadge` gains a `neutral` tone for `missing` — a requirement the résumé doesn't evidence is ordinary information, and a `warning` pill would frame it as a fault. Evidence is a native `<details>` with an accessible name qualified by its requirement, so a screen reader's control list gets distinct names rather than N repetitions of "Evidence"; the shared `Disclosure` is the section primitive and would nest card chrome inside a card for a one-line snippet. `PasteJdPanel` owns the opt-in boolean — one `useState`, read by the control and by the hook. Default OFF: no `detectWebGpu`, so no `webllm_capability_detected` enters the funnel for a keyword-only user; no engine, no download. The panel renders `semanticResult ?? keyword`, so detecting, loading, running, degraded, opted-out and errored all fall through to the keyword floor. `SemanticAnalysisOptIn` puts the lifecycle line under the control that started the work, matching `ResumeQualityPanel` / `ResumeRewrite` / `SectionRewrite`. No WebGPU renders one muted line, not `WebGpuUnavailableNotice` — that fires `webllm_notice_shown` and frames a capability gap as a warning. Running copy is generic on purpose: `judgeEvidence` reports no per-requirement progress, so the issue's "Judging requirement 4 of 9…" would be invented. `useJdMatch` gains one field, `capability`. With opt-in on and WebGPU absent, `status` settles on `ready`-with-keyword, indistinguishable from opt-in-off and from a degraded run; the UI needs the difference to explain why ticking the box changed nothing. No orchestration moved into a component. Cancellation (#803) is unchanged and now exercised from the UI: opt-out mid-load and mid-inference, JD edits, A → B → C, model change, unmount and StrictMode all assert on the threaded signal, and a superseded run's keyword fallback is never reported to the user as a failed analysis. Closes #204
Deploying offlinecv with
|
| Latest commit: |
4bd6782
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a4df304e.offlinecv.pages.dev |
| Branch Preview URL: | https://gh-204-jdmatch-semantic-ui.offlinecv.pages.dev |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Stance: APPROVE. 0 Blocking findings → APPROVE per the verdict rule (0 Blocking → APPROVE regardless of Secondary/Nits; none of either survived here either).
Blocking
None.
Secondary
None.
Nits
None — see the one AC note below, which is disclosed reasoning rather than a defect.
AC checklist (issue #204)
All acceptance criteria met:
JdMatch.tsxis a router onresult.path—keyword→<KeywordMatch>(verified byte-identical to the pre-#204 body: same element order, class strings, copy, empty states — confirmed by reading the diff directly, not just trusting the parity test),semantic→<SemanticMatch>grouped Met → Partial → Missing,Card/StatusBadgereused. ✓- Opt-in toggle "Analyze with on-device AI", default OFF, on the JD panel. ✓ (
SemanticAnalysisOptIn,useState(false)inPasteJdPanel) - Loading →
ModelLoadProgress(same primitive other WebLLM surfaces use). ✓ - Running → step indicator. The issue's literal example copy ("Judging requirement 4 of 9…") is not implemented —
judgeEvidencehas no per-requirement progress callback to source it from, so the PR ships generic copy instead and says so explicitly in the description's "adapted rather than followed" section. This is disclosed reasoning, not an omission, so it doesn't fail gate 3f — noting it here only so the record is explicit. - No WebGPU → silently degrades to keyword columns with one muted explanatory line, no error/flicker. ✓ (verified in
SemanticAnalysisOptIn'sStatusLine, and covered by the fail-before test list) - Constraints: semantic tokens only,
Button/Card/StatusBadgeprimitives, no raw<button>, privacy disclaimer preserved. ✓ (grepped the diff for raw elements and hardcoded colors — the only hex-shaped matches were#204issue references, false positives)
Description accuracy (gate 3f)
Accurate. Every checkable claim in the Summary and Review-focus sections round-trips to the diff:
semanticResult ?? jdMatchfallthrough inPasteJdPanel.tsx— confirmed every non-ready/non-semantic state (idle, loading, running, error, opted-out, degraded) falls through to the keyword floor.- The
capability-before-status.kindordering inSemanticAnalysisOptIn'sStatusLine— confirmed the check order matches the description. - The cancelled-run guard claim — traced independently in
useJdMatch.ts:requestIdRef.currentis bumped (++requestIdRef.current) beforepreviousController?.abort()is called, andsetSlotIfCurrentchecksrequestIdRef.current !== myIdbefore writing any state, so a superseded run's late write is dropped. Matches the PR body's reasoning exactly. StatusBadge's newneutraltone reusesbg-surface-subtle/text-content-muted— the same pairing already used elsewhere in this file (the "alpha" badge), not a new token combination introduced by this PR.- Bundle-size deltas in the Test plan were not independently re-measured (would require a full build + diff) but are plausible given the change shape (hoisting
ModelLoadProgressinto a shared chunk now that both entries use it) and don't affect the verdict either way.
Gates run
- 3a (fixture PII): skipped — no fixture files touched.
- 3b (design-system reuse): pass. New feature files (
KeywordMatch,SemanticMatch,SemanticAnalysisOptIn) are the router split the issue explicitly asked for, not a parallel surface;StatusBadge's newneutraltone correctly extends the shared primitive instead of a bespoke pill. - 3c (style tokens): pass. No hardcoded hex/palette classes/manual
dark:variants (grep hits were all#NNNissue-number references). - 3d (fallow): pass.
npx fallow audit --base origin/main→ "No issues in 13 changed files"; the only duplication flagged is pre-existing and excluded as inherited. - 3e (command-level bugs): n/a — no skill/script files touched.
- 3f (description accuracy): pass, see above.
npm run typecheck: clean.npm run lint: clean.npx vitest runon the 6 touched test files: 94/94 passing.
Note for the record: this run's background /code-review (generic correctness pass) did not return within a reasonable wait and was abandoned in favor of the manual pass documented above, which covered the full diff file-by-file including independent tracing of the two logic claims called out in "Review focus". No correctness issues found.
Reviewed by: Claude Sonnet 5 (high)
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Follow-up to the APPROVE above. The background /code-review pass I flagged as stalled came back after I'd already posted (three duplicate notifications, same 8 findings each time). I verified all 8 directly against source before adding them here. None change the verdict — 0 Blocking, so the PR stays APPROVE — but two are real user-facing defects worth fixing before or shortly after merge, and the rest are legitimate cleanup.
Secondary
1. Invisible "Missing" badge (correctness/visual). Anchored below at SemanticMatch.tsx:148. StatusBadge's new neutral tone (StatusBadge.tsx:47: bg-surface-subtle text-content-muted) is applied inside VerdictRow's <li>, which is itself bg-surface-subtle (SemanticMatch.tsx:143) — both resolve to the same --color-bg-subtle token (theme.css:38). Every "Missing" verdict's badge pill has no visible fill, just small-caps text with no chip boundary, while "Met"/"Partial" badges render normally colored. This undercuts the component's own docblock claim that status is carried "THREE ways, never by colour alone" — the shape/colour channel silently drops for exactly the status class most worth flagging. Suggest either a distinct neutral background (e.g. a step darker/lighter than the row) or a border, so the pill stays visible against a bg-surface-subtle row.
2. Tailor-button gated on keyword coverage only, even when the semantic view is on screen. PasteJdPanel.tsx (jdContext, ~line 111) builds the "Tailor résumé to this job" button's visibility and payload from jdMatch.coverage (keyword) unconditionally — documented deliberately in the added comment block just above it ("Built from the KEYWORD coverage regardless of which view is on screen... Wiring the semantic verdicts into rewrite steering is its own piece of work"). That's a reasonable scope cut, but the user-visible result is: opt into semantic analysis, see a real "Missing" verdict on screen, and the button that would let you act on it can be hidden or under-informed because keyword coverage says nothing is missing. Since this is explicitly scoped out rather than an oversight, I'd suggest filing a fast-follow issue rather than blocking here — flagging so it isn't lost.
3. capability never resets to null on opt-out (useJdMatch.ts:~301-313, pre-existing #203 effect, not touched by this diff's added lines). The gating effect's cleanup only sets a local cancelled flag; it never calls setCapability(null) when semanticOptIn flips back to false. This contradicts the field's own docblock, newly added by this PR: "Stays null for a keyword-only consumer, since the probe is gated on the opt-in" (useJdMatch.ts:217-223) — false after a user has ever opted in once. Currently harmless only because the sole consumer (SemanticAnalysisOptIn's StatusLine) checks checked before ever reading capability. Since capability is now public JdMatchController API, a future consumer that doesn't replicate that gate would show a stale probe result for an opted-out user. Cheap fix: if (!semanticOptIn) { setCapability(null); return; }.
4. SemanticAnalysisOptIn.tsx is 215 LOC, over CLAUDE.md's "~200 LOC, decompose past that" feature-component rule (confirmed via wc -l). StatusLine's ~70-line, 6-branch state machine is the natural extraction candidate — the same pattern this PR already applied when splitting JdMatch into KeywordMatch/SemanticMatch.
5. Duplicated <Card> header markup between KeywordMatch.tsx (moved verbatim from the old JdMatch.tsx) and SemanticMatch.tsx (SemanticMatch.tsx:91-97, byte-identical to KeywordMatch.tsx's header block). Nothing enforces the two staying in sync; a future copy tweak (e.g. renaming "alpha") edited in one file silently drifts from the other. A small shared header component would remove the duplication — the CLAUDE.md Golden Rule reasoning applies between the two feature files here, not just vs. a primitive.
Nits
6. Disclosure.tsx's reuse-analysis docblock is now stale. It enumerates the repo's hand-rolled <details> exceptions by name (five feature-code + ModelLoadProgress) as the canonical "known debt with an owner" registry — but Disclosure.tsx itself isn't touched by this diff, so SemanticMatch.tsx's new per-verdict Evidence <details> (which explicitly leans on that registry's reasoning to justify not using the shared primitive) was never added to it. A future batch-conversion effort driven off Disclosure.tsx's list would miss this instance. One-line docblock update, whenever convenient.
7. Dead defensive narrowing — PasteJdPanel.tsx's const jdMatch = keyword?.path === "keyword" ? keyword : null; (unchanged by this diff) re-narrows a value that, per useJdMatch's own construction (keywordResult always builds {path:"keyword",...} or null), can never be anything else. The hook's keyword field is typed JdMatchResult | null rather than the narrower Extract<JdMatchResult,{path:"keyword"}> | null — the same Extract idiom this PR already uses in KeywordMatch.tsx/SemanticMatch.tsx. Tightening the hook's return type would let the compiler enforce the invariant instead of a runtime check papering over it. Not urgent — pre-existing from #203, this PR just didn't touch it.
8. StatusLine's if-chain isn't exhaustive over JdMatchStatus.kind. Anchored below at SemanticAnalysisOptIn.tsx:118. A future 6th JdMatchStatus variant falls through to the final return null with no compiler signal, unlike SemanticMatch.tsx's GROUP_ORDER/Record<VerdictStatus,...> pattern (whose own docblock notes it fails-closed on a new status by design). A switch with a never-typed default would close this gap.
Reviewed by: Claude Sonnet 5 (high) — this addendum incorporates a delayed /code-review pass, independently re-verified against source before posting.
| {/* `StatusBadge` is `w-fit`, which stops it growing but not shrinking; | ||
| the wrapper is what keeps a long requirement from squeezing it. */} | ||
| <span className="shrink-0"> | ||
| <StatusBadge tone={GROUP_BADGE_TONE[status]}> |
There was a problem hiding this comment.
This StatusBadge tone={GROUP_BADGE_TONE[status]} renders with tone="neutral" for the missing group. neutral is bg-surface-subtle text-content-muted (StatusBadge.tsx:47), and the row this sits in (VerdictRow's <li>, line 143) is itself bg-surface-subtle — same token, so the pill has no visible fill against its own row. See the Secondary finding #1 in the review body for the full writeup and a suggested fix.
| ); | ||
| } | ||
|
|
||
| function StatusLine({ |
There was a problem hiding this comment.
StatusLine branches on status.kind via a chain of ifs rather than an exhaustive switch. A future JdMatchStatus variant added to the union (currently 5 members) would fall through to the final return null with no compiler warning. See Nit #8 in the review body — not urgent, just flagging for awareness.
2 participants
Summary
JdMatch.tsxreturnednullforpath: "semantic", so a finished on-device match rendered a blank panel, andsemanticOptInhad no route to a user —PasteJdPanelcalleduseJdMatchwithout it, hardwiring the defaultfalse. This closes both.JdMatchbecomes a router on theJdMatchResultdiscriminant; the pre-#204 body moves toKeywordMatchverbatim;SemanticMatchrenders verdicts grouped Met → Partial → Missing;PasteJdPanelowns the opt-in boolean and renderssemanticResult ?? keyword, so the keyword floor survives every other state.useJdMatchgains one field (capability) — with opt-in on and WebGPU absent,statussettles onready-with-keyword, indistinguishable from opt-in-off and from a degraded run, and the UI needs that difference to explain why ticking the box changed nothing. No orchestration moved into a component.Three places where the June issue text no longer matches the repo, adapted rather than followed:
judgeEvidencetakes no progress callback andrunLlmMatchreports only engine-load progress plus a singleonInferenceStart. The running copy is generic rather than invented, and a test asserts no/requirement \d+ of \d+/appears./jd-fit/is retired (Deprecate /jd-fit/ — migrate JD paste + JD-driven rewrite into the Find Jobs tab, then remove the second entry #576/refactor: fold JD paste + tailor into /jobs/, retire /jd-fit/ (#576) #783), so the integration point is/jobs/→FindJobsPanel→PasteJdPanel.StatusBadgehad no tone for "missing"; addedneutralto the shared piece rather than hand-rolling a pill at one callsite.#804 assessment (not fixed here, deliberately): its stated scenario is stale — it cites
job-search/sector.ts'sclassifySector, which has no production caller (useCompanyTargetsusesclassifySectorHeuristic), and/jobs/has no otherloadEnginecaller at all. The reachable variant is self-inflicted: edit the JD while the weight download is in flight and the new run joins a load whoseonProgressit cannot hear, so the bar reads 0% until the load resolves. Cosmetic, not functional — the keyword floor stays on screen throughout — and the fix is a progress fan-out insideweb-llm.tsshared by every WebLLM surface, which is #804's scope. Recorded inSemanticAnalysisOptIn's docblock.Closes #204
Review focus
src/components/features/PasteJdPanel.tsx:96—semanticResult ?? jdMatchis the whole no-stale-verdict guarantee at the UI layer. Does every non-readystate really fall through to the keyword floor, including opt-out after a completed run?src/components/features/SemanticAnalysisOptIn.tsx:126—StatusLinecheckscapabilitybefore it switches onstatus.kind. Invert that order and the no-WebGPU case renders the "didn't return a verdict" degrade note instead. Is the ordering obvious enough to survive a later edit?src/components/features/SemanticAnalysisOptIn.tsx:196— the degrade note is suppressed on a cancelled run only becauseuseJdMatchbumps its request id before aborting, so the abandoned run's keyword fallback fails the write guard. Is that reasoning right, or can a superseded run still land aready-with-keyword slot?src/design-system/shared/StatusBadge.tsx:47—neutralisbg-surface-subtle text-content-muted. Muted-on-subtle is deliberately low-contrast; is it still legible enough for a badge that carries meaning, in both themes?Test plan
npm run typecheckcleannpm run lintcleannpm run verifygreen (also enforced by the pre-push hook on this branch)npm testfull suite green — 367 files / 5960 tests, 0 failuresnpx fallow audit --base origin/main— no issues in 13 changed filesKeywordMatch.parity.test.tsxwere captured by rendering the pre-JD-match: JdMatch.tsx semantic verdict UI + opt-in toggle + keyword fallback view #204 component, and the new render is byte-identical across three shapes (populated + footnote, both empty states, singular footnote fork)null; unconditionaldetectWebGpu;previousController?.abort()removed; request-id guard removed; keyword copy changed; running copy → "Judging requirement 4 of 9…"; keyword floor blanked during load; evidence made hover-only;sr-onlyname qualifier dropped; row status word dropped/jobs/+4.4 kB raw / +1.0 kB gzip./shrank 1.8 kB — bisected across five builds toModelLoadProgresshoisting out ofmain-*.jsinto the shared chunk now that both entries use it.run-llm-matchandweb-llmremain lazy chunks; zero WebLLM strings in the/jobs/entry