Skip to content

feat(job-search): a local-only filter and narrowing controls beside the results (#809) - #905

Open
rohithgollapalli wants to merge 2 commits into
mainfrom
feat/809-local-only-refine-strip
Open

feat(job-search): a local-only filter and narrowing controls beside the results (#809)#905
rohithgollapalli wants to merge 2 commits into
mainfrom
feat/809-local-only-refine-strip

Conversation

@rohithgollapalli

@rohithgollapalli rohithgollapalli commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #809.

What the feedback actually hit

Three respondents in the Aug 2026 round reported the same thing: the search returns postings they did not ask for, and nothing they can reach makes it stop. Both halves were true, for different reasons.

Reachability. Every narrowing lever the lane has — role chips, exclude terms, target level — lives inside the four-step query form, and FindJobsPanel folds that whole form to a one-line summary the moment Search is clicked. At the exact moment a user finally has a result set to react to, all of it sits behind an "Edit search" button, in a walk, on a step they have to pick. Nobody found them.

Locality. location was a bounded soft axis and nothing else, so a stated city returned the whole feed, reordered. There was no hard location filter anywhere in the tree.

Freshers. SENIORITY_PATTERNS derives a level from title text, so a candidate with no prior title derives nothing — and the form only reveals LevelSelect once a level was derived. The gate's condition is exactly the person it excludes.

What this does

JobResultRefineStrip renders the three highest-value levers with the results, outside the fold. It is not a second query surface: it edits the same JobQuery through the same setter and reuses the same controls (LevelSelect, ChipListEditor, EditableField, Card, Checkbox), so a chip removed there is gone from the form's Narrow step too. No new primitives.

JobQuery.locationOnly adds the hard arm of the location axis. refineSearchResult drops postings locationMatches rejects — but only because the user armed a visible toggle.

The strip's LevelSelect is ungated, which is the fresher's way in.

What this deliberately does not do

It does not reopen #570 / #716. The default behaviour of every axis is byte-identical. Location, level and comp floor still rank and drop nothing unless a user explicitly says otherwise. The answer to "the search returns everything" is an explicit lever, not a re-inflated implicit boost.

No radius, no geocoding. The predicate is a string comparison over a feed's free-text location field. A distance model needs a geocoder, which is a network call this app does not get to make. Remote postings always pass, so the toggle narrows to "near me OR anywhere", never to "on-site only".

No new remover without a control. refine.ts now has three hard filters — role families, exclude terms, local-only — and all three are armed by something the user can see and clear. That rule is now written down in the lane's CLAUDE.md.

One correction to the issue thread

Option 2 already shipped. The relevance floor is not a resumption — weakMatchThreshold.ts (2.5★) + WeakMatchesSection.tsx landed in #569 and are live: cutoff, count, and a "Show weak matches (N)" disclosure. The comment on #809 saying "there is no fold, threshold, or show-all in JobSearchResults.tsx" is out of date. Nothing about the floor is declined here; there was nothing left to decide.

Separately, the issue body says refineSearchResult hard-filters on "role families, exclude terms, target level, comp floor, and location". Before this PR it applied only role families and exclude terms; level, comp floor and location were passed to rankPostings as soft axes and removed nothing. That is why acceptance criterion 1 was unreachable and why this PR needed a filter rather than only a surfacing change.

Egress

Unchanged. The filter is a local set operation over already-fetched postings; every strip edit re-ranks through refineSearchResult with no new fetch (query.locationOnly joins the live re-rank dep array in useJobSearch). providers/keywords.ts stays the sole resume-derived egress helper — untouched. work-authorization-egress.test.ts and the keyword egress tests are green.

Never-fail-closed

Both new signals follow the floor excludeTerms and the #566 role filter already apply. A filter that would reduce a non-empty set to empty is skipped, the input is kept, and locationSuppressed goes back for a notice — a blank panel the user cannot diagnose is worse than an unfiltered one. locationFilteredOut states what the filter did remove, and unticking the toggle is the recovery.

Unknown is not far. A posting whose feed omitted location (the keyless feeds are inconsistent about populating it) passes the hard filter, the same way a remote posting does. locationMatches reads that blank as a non-match because it is scoring a rating with no evidence to credit; a remover cannot borrow that read without telling the user it hid a posting "as too far away" when the app never saw a location at all. The two readers of the blank differ on purpose, and locationFilteredOut counts only postings that stated a location somewhere else.

Acceptance criteria

  • A user who states a role and a location can reach a result set that excludes non-matching postings, in at most one interaction from the results view
  • A candidate with no prior job title can express their level and see it change the ranking
  • Whatever is hidden is stated as a count and is recoverable — never silently dropped
  • Control edits still re-rank through refineSearchResult with no new fetch
  • No change to what leaves the browser; egress tests stay green

Files

File Change
src/lib/job-search/location-match.ts (new) The one location predicate + filterPostingsByLocation. Qualifier-aware equality + whole-word containment; an unstated location passes the hard filter
rank.ts Private copies deleted; imports the shared predicate. Same soft axis, now correct on Portland, OR vs Portland, ME
query-builder.ts locationOnly; withExcludeTerm/withoutExcludeTerm extracted now that two editors write that field
refine.ts Applies the filter last, after role/exclude, so the notice names the right control
search.ts locationSuppressed + locationFilteredOut on JobSearchResult
JobResultRefineStrip.tsx (new, 118 LOC) The strip
QueryFilterFields.tsx (new) LocationField + ExcludeTermsEditor + EXCLUDE_TERMS_HINT — the two fields both editors render, one definition each
JobQueryEditor.tsx Renders the shared fields instead of its own copies
LevelSelect.tsx useId() for the radiogroup label — two instances can be mounted at once
FindJobsPanel.tsx Mounts the strip outside the fold, on loaded and a non-empty ranked set
JobSearchResults.tsx Notice stack delegated to the new sibling
JobSearchNotices.tsx (new) The five filter notices, extracted out of Loaded
useJobSearch.ts query.locationOnly in the live re-rank deps
src/lib/job-search/CLAUDE.md New section: soft axes rank, three hard filters remove, all user-armed; plus "unknown is not far"
packages/core/src/index.ts, packages/core/tsconfig.build.json, scripts/check-core-package.mjs location-match.ts joins the . entry's value-edge closure (27 → 28 modules) via rank.ts, which was already on it. It reaches no fetch/WebSocket/XMLHttpRequest/EventSource; ENTRY_CLOSURES still asserts networkBearingModules: 0 and the two closures stay disjoint, so the Egress — Unchanged claim above is machine-enforced, not merely asserted

Verification

  • tsc -b --noEmit, eslint ., check:nul, check:fixtures, check:baselines — clean
  • Full suite 6392 passed / 4 failed, the 4 being the pre-existing en-IN locale failures noted below. Lane suite 643 passed / 0 failed
  • vite build green
  • fallow audit --base origin/main: dead code 0, complexity 0, duplication 6 clone groups (warn), 1 inherited finding excluded. An earlier revision of this branch did report complexity: 1 on JobSearchResults.tsx:102 Loaded (14 cyclomatic, 56.3 CRAP) — that function's notice stack now lives in JobSearchNotices.tsx

Two caveats on the local run, both verified environmental rather than introduced here:

  • src/lib/jd-extract/schema-org*.test.ts fails 4 tests on this machine (USD 1,50,000 vs USD 150,000) — the box's locale is en-IN, so toLocaleString groups in lakhs. Confirmed identical on a clean main checkout. Untouched by this PR; CI's locale should not hit it.
  • npm run verify cannot run end-to-end here (cmd.exe cannot parse the || echo subshell) and check:core dies on tar -tzf C:\… under git-bash. Each gate was run individually instead, and npm run build -w @offlinecv/core builds green.

Provenance

Written with Claude Code (Opus 5).

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 26, 2026

Copy link
Copy Markdown

Deploying offlinecv with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8b52a38
Status: ✅  Deploy successful!
Preview URL: https://c86e0976.offlinecv.pages.dev
Branch Preview URL: https://feat-809-local-only-refine-s.offlinecv.pages.dev

View logs

…he results (#809)

Three respondents in the Aug 2026 round reported the same thing: the job search
returns postings they did not ask for, and nothing they can reach makes it stop.
Both halves of that were true.

Reachability. Every narrowing lever the lane has — role chips, exclude terms,
target level — lives inside the four-step query form, and `FindJobsPanel` folds
that whole form to a one-line summary the moment Search is clicked. At the exact
moment a user finally has a result set to react to, all of it is behind an "Edit
search" button, in a walk, on a step they have to pick. `JobResultRefineStrip`
now renders the three highest-value levers with the results, outside the fold. It
is not a second query surface: it edits the same `JobQuery` through the same
setter and reuses the same controls, so a chip removed there is gone from the
form too.

Locality. `location` was a bounded soft axis and nothing else, so a stated city
returned the whole feed, reordered. `JobQuery.locationOnly` adds the hard arm:
`refineSearchResult` drops postings `locationMatches` rejects, but only because
the user armed a visible toggle. This does not reopen #570/#716 — the default
behaviour of every axis is unchanged, and the answer to "returns everything" is
an explicit lever, never a re-inflated implicit boost. Remote postings always
pass, so it narrows to "near me OR anywhere", never to "on-site only". The
predicate moved to `location-match.ts` and both readers import it, so the toggle
cannot hide a posting whose own card shows a location match. There is no radius
and no geocoding: a distance model needs a geocoder, which is a network call this
app does not make.

Freshers. `SENIORITY_PATTERNS` derives a level from title text, so a candidate
with no prior title derives nothing — and the form only reveals `LevelSelect`
once a level WAS derived. The gate's condition is exactly the person it excludes.
The strip's copy is ungated.

Both never-fail-closed, the floor `excludeTerms` and the role filter already
apply: a filter that would empty a non-empty set is skipped and flagged for a
notice, because a blank panel the user cannot diagnose is worse than an
unfiltered one. `locationFilteredOut` states what the filter did remove, and
unticking the toggle is the recovery.

No egress change: the filter is a local set operation over already-fetched
postings, it re-ranks through `refineSearchResult` with no new fetch, and
`providers/keywords.ts` stays the sole resume-derived egress helper.

`withExcludeTerm`/`withoutExcludeTerm` are extracted to `query-builder.ts` now
that two editors write that field — the `undefined`-means-`[]` contract should
not be half-remembered in one of them.

`check:core` asserts the `.` entry's value-edge closure exactly, so this moves
that constant from 27 to 28: `rank.ts` takes a value edge on `location-match.ts`
and `rank.ts` is on the barrel. The network-free claim is unaffected —
`location-match.ts` is zero-dep and pure, the closure still names no network
primitive, `./job-search` is unchanged at 11, and the two closures still share
zero modules. The surrounding prose numbers were already stale by more than this
change makes them (the emit is 65 modules with 26 unreachable, not 62/24, and 15
of the unreachable sit behind a value edge, not 13); they are corrected to the
measured values rather than carried forward with a +1 on a wrong base.
@rohithgollapalli
rohithgollapalli force-pushed the feat/809-local-only-refine-strip branch from b6588d7 to 65a7bd3 Compare August 26, 2026 21:04

@Samhit21 Samhit21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: a local-only filter and narrowing controls beside the results (#809)

Summary

Strong PR, and unusually well-reasoned about what it deliberately does not do — keeping level and comp floor as soft axes rather than re-inflating #570/#716 is the right call, and saying so in the lane's CLAUDE.md is better than leaving it as tribal knowledge. The reachability diagnosis is the valuable part: "the levers exist but fold away at the exact moment the user has results to react to" explains the feedback better than the feature request did. location-match.ts correctly collapses two private copies into one predicate so the soft axis and the hard filter cannot disagree on screen, the never-fail-closed floor matches the existing excludeTerms/role precedent, and locationFilteredOut vs locationSuppressed is a genuinely useful distinction — "declined to run" and "ran and removed N" are different things a user needs told differently.

I checked two things I expected to be problems and both were fine, so noting them so nobody re-checks: editing location in the strip cannot desync from the fetched snapshot (no provider reads query.location — it is a purely local knob, and it was already in the live re-rank deps), and the . entry's closure growth is machine-enforced safe (check-core-package.mjs asserts disjointness and networkBearingModules: 0, and check:core is green in CI).

Findings

  1. location-match.ts:55 — the predicate keeps the wrong city under a hard filter. Portland, OR matches Portland, ME; San Jose, CA matches San Jose, Costa Rica; a bare IN matches Austin, TX. Verified by running it on this branch, not inferred. Tolerable when location only nudged a rating; now it is the filter's whole job. The one finding I would want addressed.
  2. JobSearchResults.tsx:214 — both notices quote "only jobs near me" while the checkbox they point at reads "Only jobs near {location}" in exactly the situations the notices fire.
  3. Description accuracy (nit). The Files table lists 10 of the 18 changed files, omitting packages/core/src/index.ts, packages/core/tsconfig.build.json, scripts/check-core-package.mjs and JobQueryEditor.tsx. The first three are where the core package's network-free claim is written down, and this PR moves the . entry's closure from 27 to 28 modules. The claim still holdsnetworkBearingModules stays 0, the closures stay disjoint, CI enforces both — but a reviewer reading "## Egress — Unchanged" has no signal to go look. Worth a line in the body given how much care the rest of the description takes.

Two verification notes from the body worth a second pair of eyes, though neither is a code issue: the en-IN locale failures in schema-org*.test.ts reproduce on clean main (so genuinely environmental), and npm run verify never ran end-to-end locally. CI's verify is green on 65a7bd3, which covers it.

Verdict

Action: COMMENT — nothing merge-blocking. Finding 1 is the one I would want resolved before this ships, since a location filter that returns the wrong Portland is the same class of complaint #809 was filed about.

Comment thread src/lib/job-search/location-match.ts Outdated
Comment thread src/components/features/JobSearchResults.tsx Outdated

@s-annam s-annam left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against #809's acceptance criteria and the diff first, description last. I also re-ran @Samhit21's two open findings on this branch rather than taking them on trust — both reproduce, and the first one is larger than it looked.

The design is right and worth saying so plainly: soft axes rank, three user-armed hard filters remove, never-fail-closed on all three, one predicate shared by the soft axis and the hard arm so the toggle can't contradict the card. Writing that rule into the lane's CLAUDE.md is worth more than the feature. The reachability diagnosis — "the levers exist but fold away at the exact moment the user has results to react to" — explains the feedback better than the issue did.

The blockers are all one root cause: locationMatches was written as a rating hint and is now the sole predicate behind a remover, and it was promoted without being re-examined for that job. Every finding below is downstream of that.

Blocking

1. location-match.ts:55 — the predicate keeps the wrong place, and drops the right one

Reproduced by calling the real function on 65a7bd3:

query posting result
Portland, OR Portland, ME true wrong keep
San Jose, CA San Jose, Costa Rica true wrong keep
Springfield, IL Springfield, MA true wrong keep
Columbus, OH Columbus, GA true wrong keep
Kansas City, MO Kansas City, KS true wrong keep
Austin, TX IN true wrong keep
Norwich, UK OR true wrong keep
Boston Boston Consulting Group, London true wrong keep
New York, NY New York City, NY false wrong drop
SF Bay Area San Francisco, CA false wrong drop
Austin, TX Austin, TX, USA true correct
Boston, MA Chicago, IL false correct

Three causes:

  • postingCity === queryCity throws away the qualifier. Portland OR/ME, Springfield, Columbus, Kansas City MO/KS, San Jose CA/Costa Rica are exactly the pairs a location filter exists to separate.
  • The bidirectional includes matches non-token substrings. "austin, tx".includes("in") is why a bare IN posting survives an Austin filter; "norwich, uk".includes("or") is why OR does. A feed emitting a bare state or country code is enough to trigger it.
  • The two arms disagree about what they compare. The equality arm uses the pre-comma token; the includes arms use the whole string including the suffix. That asymmetry is what makes New York, NY vs New York City, NY fail — the cities differ by one token and the suffixes are identical, so neither arm can see it. The wrong-drop direction is the quieter half and gets no notice at all: never-fail-closed only fires when the set reaches zero, so a partial wrong-drop is silent.

Why Blocking rather than Secondary. As a bounded soft axis this was tolerable — a wrong location nudged a star rating and nothing vanished. Here it is the sole predicate behind a hard remover, driving a control that renders as "Only jobs near Portland, OR". A user who ticks that and gets Portland, Maine — or ticks "Only jobs near New York, NY" and loses every New York City posting, labelled "too far away" — is filing #809 again verbatim: "it is giving the jobs which I didn't mention". The PR ticks AC 1 ("can reach a result set that excludes non-matching postings") under Closes #809, so merging as-is closes the issue against a filter that fails in both directions, and nothing will reopen it.

Not asking for geocoding — the no-radius reasoning in the docblock is right and should stay. String work covers it: compare the qualifier when both sides have one, and make the fallback token-aware and consistent about which token it compares.

const [postingCity, ...postingRest] = posting.split(",").map((s) => s.trim());
const [queryCity, ...queryRest] = query.split(",").map((s) => s.trim());
if (postingCity === queryCity) {
  // Same city name: require the qualifiers not to CONFLICT.
  return postingRest.length === 0 || queryRest.length === 0
    || postingRest[0] === queryRest[0];
}
// Fallback: whole-token containment on the CITY tokens, not raw substring
// on the full strings — the asymmetry above is what breaks New York City.
const tokens = (s: string) => new Set(s.split(/\s+/).filter(Boolean));
const postingTokens = tokens(postingCity);
const queryTokens = tokens(queryCity);
return [...queryTokens].every((t) => postingTokens.has(t))
  || [...postingTokens].every((t) => queryTokens.has(t));

Shape is yours. The cases I would want pinned in location-match.test.ts: Portland, OR vs Portland, ME, Austin, TX vs IN, and New York, NY vs New York City, NY — none need unusual feed data and all three are what a user would report.

One consequence worth stating because it sizes the fix: the soft axis inherits whatever you do here, since rank.ts now reads the same predicate. That is the shared-predicate design working as intended, and it makes RatingInput.locationMatch more honest too — but rank.ts's existing expectations are the thing to re-run.

2. location-match.ts:78 — a posting whose feed omitted location is hard-dropped and reported as "too far away"

locationMatches returns false for "", which the docblock defends as "no evidence, no credit". That is correct for a rating axis and wrong for a remover, and it was carried over without re-examination. JobPosting.location is documented "" when the feed omits it, and this PR's own search.ts docblock says the keyless feeds "are inconsistent about populating a posting's location at all" — so this is the ordinary case, not an edge.

Reproduced against filterPostingsByLocation directly — one Austin posting plus three with location: "", query Austin, TX:

kept = 1   suppressed = false   → locationFilteredOut = 3

which JobSearchResults renders as "3 postings hidden as too far away". The app has no idea where those three are; it says it knows they are far. The never-fail-closed floor cannot catch this — it only triggers on kept.length === 0, and the mixed set is the likely one.

Decide the semantics and state them: either an unstated location passes the hard filter (unknown ≠ far, consistent with remote always passing), or it is dropped but counted and worded separately from "too far away". The first is the smaller change and matches the floor's own stated reasoning; whichever you pick, locationFilteredOut must not silently merge "elsewhere" with "didn't say".

Secondary

3. JobSearchResults.tsx:218 — the suppressed-filter notice states a cause that is usually false. locationSuppressed is set whenever the filter would empty a non-empty set, for any reason, but the copy is "None of these postings say where they are". An Austin candidate whose feed returned only Seattle postings — every one of which states its location — is told the postings didn't say. The PR's own refine.test.ts case (far: "Seattle, WA" + unstated: "") is exactly that mixture. Either widen the copy to cover "nothing matched" as well as "nothing stated", or split the flag. Note this one needs the assertion in JobSearchResults.test.tsx updated with it, which is why I have not attached a suggestion block for it.

4. JobSearchResults.tsx:214 — both notices quote a label that is never on screen. They say untick / turn off "only jobs near me", but localOnlyLabel returns that string only when no location is set — and the filter cannot run without a location, so whenever either notice appears the visible checkbox reads "Only jobs near {city}". The strip's own comment on that helper argues the label must name the place; the copy pointing at the label has to stay in step. Suggestion block attached for the first notice (it keeps the existing test assertions green); the second is folded into finding 3.

5. JobResultRefineStrip.tsx:115 — two LevelSelect instances can be in the DOM at once, and it hardcodes id="level-select-label". StepPanel keeps inactive panels mounted, and the strip renders on phase.kind === "loaded" independently of open. So: pick a level in the strip (which sets query.seniority, un-gating JobQueryEditor's AddPill), then click "Edit search" — two elements now share id="level-select-label", and the second radiogroup's aria-labelledby resolves to the first one's node. Duplicate ids also break getElementById and any future test keyed on it. LevelSelect should mint its id with useId() — a one-line change in a file this PR does not otherwise touch, which is why it is here rather than in the strip.

6. FindJobsPanel.tsx:256 — the mount comment does not match the mount condition. It says the strip is withheld "over the loading skeleton or the error state", but total provider failure is not an error phase: searchJobs never rejects, so degradedProviders.length === providerCount arrives as kind: "loaded" and JobSearchResults renders HardError (:153) beneath a fully interactive "Narrow these results" card. Same for the zero-match ErrorState (:158), whose copy tells the user to "Open Edit search to broaden the query" while narrowing controls sit directly above it. Gate on the hard-error condition or on result.jobs.length > 0 to match the stated intent, or correct the comment.

7. ## Verification claims fallow audit --base origin/main: … complexity 0; it is 1 on this branch. Run here on 65a7bd3:

● High complexity functions (1)
  src/components/features/JobSearchResults.tsx
    :102 Loaded HIGH
          14 cyclomatic   13 cognitive  160 lines   56.3 CRAP
✓ complexity: 1 finding · duplication: 6 clone groups (warn) · 18 changed files
  audit gate excluded 4 inherited findings

Diff-attributed, not inherited (the 4 inherited are excluded separately) — the two new notices add 20 lines and two branches to what was already the largest function in the file. Per CLAUDE.md a fallow complexity finding is report-only inside verify and never Blocking on its own, so this is not a merge gate; the finding is that the body states a gate result that does not reproduce. Extracting the notice stack into a sibling alongside the existing roleSuppressed/excludeSuppressed paragraphs would fix both the metric and findings 3–4 in one place.

8. The Files table lists 10 of 18 changed files. Omitted: packages/core/src/index.ts, packages/core/tsconfig.build.json, scripts/check-core-package.mjs, JobQueryEditor.tsx. The first three are where the core package's network-free claim is written down, and this PR moves the . entry's value-edge closure from 27 to 28 modules — under an ## Egress — Unchanged heading, with nothing pointing a reader at it.

The claim still holds — I walked the . entry's value-edge closure independently and got 28, location-match.ts being the addition (it enters via rank.ts, already on that closure), reaching no fetch/WebSocket/XMLHttpRequest/EventSource; ENTRY_CLOSURES still asserts networkBearingModules: 0, the two closures stay disjoint, and check:core is green in CI and in my local verify. A description gap, not a defect — but a reviewer should not have to discover a closure change by diffing the file list.

Nits

9. location-match.ts:23 — the docblock's consequence doesn't round-trip. "so it stays out of the dynamic-import tiers: refine.ts can filter before it has paid for rank.ts" — but refineSearchResult's first statement is await import("./rank.ts") (refine.ts:51), before any filter runs. Zero-dep and pure is true and worth saying; that particular consequence is not what the code does.

10. JobResultRefineStrip.tsx:123 — only the handlers were deduped. withExcludeTerm/withoutExcludeTerm were correctly extracted, but the Location EditableField block and the Exclude ChipListEditor block — including its hint string — are now byte-duplicated against JobQueryEditor.tsx. That hint is the exact class of shared rule the withExcludeTerm docblock argues should have one definition.

11. ## Provenance is retireddocs/CONTRIBUTING-PROCESS.mdModel provenance — retired: "no longer written, updated, or expected, and existing ones need not be removed." Nothing to do on this PR; flagging so it isn't copied into the next body.

Checked and clean

Stated so nobody re-checks:

  • AC 2, 3, 4, 5 are met. Level is ungated in the strip and offers Intern/Junior/Mid — the rungs SENIORITY_PATTERNS cannot derive — with a test pinning it. locationFilteredOut correctly counts only its own removals (excludeFiltered.length - filtered.length), so the exclude filter's drops aren't double-counted, and it is 0 on every suppressed/inert path. query.locationOnly joins the live re-rank dep array; that effect never touches rawFetchRef and keeps phase.kind === "loaded", so no fetch and the strip never unmounts mid-edit. No new egress: providers never receive query.location, locationOnly reaches no adapter, and keywords.ts is untouched.
  • Filter ordering is right, and the comment gives the real reason (the never-fail-closed check must read the set the user sees, or the notice names the wrong control).
  • refineSearchResult is the only constructor of JobSearchResultsearchJobs delegates to it — so the two new required fields cannot be half-populated.
  • House gates. No raw <button>/modal/dropzone in feature code; no hex, raw palette class, or manual dark: variant; Checkbox is the existing primitive used as designed; the strip is 137 LOC, inside the ~200 rule; the reuse analysis is in the docblock and it genuinely is not a second query surface. No fixtures touched, so the PII gate does not apply. npm run verify green locally end-to-end on 65a7bd3.
  • @Samhit21's two "expected to be problems, both fine" notes hold. No provider reads query.location, and the closure growth is machine-enforced safe.

Verdict

REQUEST_CHANGES — 2 Blocking, 6 Secondary, 3 Nits. Rule applied: ≥1 Blocking → REQUEST_CHANGES; nits never gate.

Findings 1 and 2 are one change to one file plus tests. Nothing was pushed from this review: the blockers are behavioural, and this is a named contributor's branch, so nothing here rewrites it.


Reviewed by: Claude Opus 5 (high)

Comment thread src/lib/job-search/location-match.ts Outdated
if (!posting || !query) return false;
const postingCity = posting.split(",")[0].trim();
const queryCity = query.split(",")[0].trim();
return postingCity === queryCity || posting.includes(query) || query.includes(posting);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Blocking] The predicate is unfit as a hard filter, in both directions. Verified by calling it on this branch:

query posting result
Portland, OR Portland, ME true — wrong keep
Columbus, OH Columbus, GA true — wrong keep
Austin, TX IN true — wrong keep
New York, NY New York City, NY false — wrong drop
SF Bay Area San Francisco, CA false — wrong drop

The equality arm compares the pre-comma token; the two includes arms compare the whole string including the suffix. That asymmetry is why New York City, NY fails — one extra city token, identical suffixes, so neither arm sees a match. And "austin, tx".includes("in") is why a bare IN posting survives an Austin filter.

Tolerable while location only nudged a star rating; this PR makes it the sole predicate behind a remover, driving a control labelled "Only jobs near Portland, OR". Full reasoning and a suggested shape are in the review body — the cases I'd want pinned in location-match.test.ts are Portland, OR vs Portland, ME, Austin, TX vs IN, and New York, NY vs New York City, NY.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8b52a38, and adopted the shape you sketched with one addition — the qualifier check runs on both arms, so New York, NY vs New York City, CA is false too.

if (!qualifiersAgree(posting[1], query[1])) return false;
if (posting[0] === query[0]) return true;
return containsAllWords(postingWords, queryWords) || containsAllWords(queryWords, postingWords);

Every row of your table except two now behaves:

query posting before now
Portland, OR Portland, ME true false
Columbus, OH Columbus, GA true false
Kansas City, MO Kansas City, KS true false
San Jose, CA San Jose, Costa Rica true false
Austin, TX IN true false
Norwich, UK OR true false
New York, NY New York City, NY false true
Austin, TX Austin, TX, USA true true
Boston, MA Chicago, IL false false

All three cases you asked to pin are in location-match.test.ts, plus the other wrong-keep pairs and a both-directions "only one side names a state" case.

The two I did not fix, and why they are in the docblock as known limits rather than in the tests as green:

  • SF Bay Area vs San Francisco, CA — needs an alias table. There is no string relation between the two; any rule that matched them would have to encode that those name the same place.
  • Boston vs Boston Consulting Group, London — needs a gazetteer. Boston is a whole-word prefix of Boston Consulting Group in exactly the way New York is of New York City, so no purely lexical rule separates the city refinement from the company name. I tried two: an allowlist of locality words (city/metro/area) as the only permitted extra tokens, which breaks the existing Berlin vs Berlin Office case; and "reject when the posting names a qualifier and the query does not", which breaks Austin vs Austin Metro, TX. Both trade a wrong keep for a wrong drop, and the wrong drop is the silent direction — so I stopped rather than ship a rule I could not defend.

Leaving this thread open for your call on whether either is worth an alias table in this PR or a follow-up issue. Both currently fail toward the soft axis, and the never-fail-closed floor stops either from emptying the panel.

Worth flagging the consequence you named: rank.ts inherits all of this, so the soft axis moved with it. rank.test.ts and the full lane suite are green (643 passed / 0 failed) — the rating change is in the honest direction, since RatingInput.locationMatch no longer credits Portland, Maine.

Comment thread src/lib/job-search/location-match.ts Outdated
Comment thread src/components/features/JobSearchResults.tsx Outdated
Comment thread src/components/features/JobSearchResults.tsx Outdated
Comment thread src/components/features/JobResultRefineStrip.tsx
Comment thread src/components/features/FindJobsPanel.tsx Outdated
Comment thread src/lib/job-search/location-match.ts Outdated
Comment thread src/components/features/JobResultRefineStrip.tsx Outdated
@rohithgollapalli

Copy link
Copy Markdown
Collaborator Author

@s-annam @Samhit21 — review round addressed in 8b52a38. Nine of the ten threads are fixed and resolved; the location-predicate thread is left open deliberately (two rows of the table are documented as known limits rather than fixed — reasoning is in the thread, and the call on whether either is worth an alias table here or a follow-up is yours).

The three findings that had no inline thread:

7 — fallow complexity. You were right and the body was wrong. fallow audit --base origin/main on 65a7bd3 did report complexity: 1 on JobSearchResults.tsx:102 Loaded (14 cyclomatic, 56.3 CRAP). Fixed at the metric rather than the claim, along the line you suggested: the five-branch notice stack moved to a JobSearchNotices.tsx sibling, which took findings 3 and 4 with it. Now dead code 0 · complexity 0 · duplication 6, with the 1 inherited finding excluded as before. The Verification section states the corrected numbers and names the earlier reading rather than quietly replacing it.

8 — Files table. Rewritten: all 18 files, plus the four new ones from this round. The three packages/core / scripts entries get their own row stating the closure move (27 → 28 modules, location-match.ts entering via rank.ts) and why the Egress — Unchanged heading still holds — so the next reader does not have to diff the file list to discover it. Thanks for walking the closure independently; that saved a round.

11 — ## Provenance. Left exactly as-is per your note, and noted for the next body.

Gates on 8b52a38: tsc -b --noEmit, eslint ., vite build — clean. Full suite 6392 passed / 4 failed, the 4 being the same en-IN toLocaleString failures in schema-org*.test.ts that reproduce on clean main. Lane suite 643 passed / 0 failed, including 6 new location-match cases and 1 new refine case.

Not collapsed to one commit this round — the fixup is left as a separate commit so the delta is diffable while a thread is still open. It gets collapsed before the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Job search returns everything: soft-only ranking gives a user no way to exclude

3 participants