feat: add configurable salary filtering to scanner#677
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR annualizes Ashby provider compensation, exposes structured ChangesSalary filtering for job scans
Sequence DiagramsequenceDiagram
participant Config
participant ScanPipeline
participant AshbyProvider
participant Reporter
Config->>ScanPipeline: salary_filter (min,max,currency)
ScanPipeline->>ScanPipeline: buildSalaryFilter(salary_filter)
ScanPipeline->>AshbyProvider: fetch jobs
AshbyProvider->>AshbyProvider: parseCompensation(job) -> job.salary
ScanPipeline->>ScanPipeline: apply salaryFilter(job.salary)
alt salaryFilter rejects
ScanPipeline->>ScanPipeline: increment totalFilteredSalary
else accepted
ScanPipeline->>Reporter: include job in results
end
ScanPipeline->>Reporter: emit final summary (includes Filtered by salary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/ashby.mjs`:
- Around line 33-51: Validate and sanitize compensation fields before using
them: guard against comp being null, coerce/validate minValue and maxValue with
Number.isFinite (e.g., const minValue = Number(comp?.minValue) and check
Number.isFinite) and multiply only valid numbers by multiplier, and ensure
currency is a string before calling .toUpperCase() (e.g., const currency =
typeof comp?.currency === 'string' ? comp.currency : ''). In the block that
computes min/max (referencing comp, minValue, maxValue, multiplier), treat
non-finite values as null, return null if both are invalid, and use the
sanitized currency when building the returned object.
In `@scan.mjs`:
- Around line 171-177: Validate and normalize salaryFilter values before
building the predicate: in the block using salaryFilter, parse and coerce
salaryFilter.min and salaryFilter.max into numeric values (e.g., strip
non-numeric characters then parseFloat or Number), ensure they are finite and
non-negative, normalize currency via filterCurrency as you already do, and
enforce ordering (if min > max either swap them or treat the filter as invalid).
If validation fails (NaN, negative, or other malformed input) log a warning and
return a no-op predicate (() => true) or otherwise handle it explicitly so
malformed YAML like "100k", negatives, or min > max do not silently produce
incorrect filtering; reference the variables salaryFilter, min, max,
filterCurrency and the early-return predicate construction when making the
change.
In `@templates/portals.example.yml`:
- Around line 61-62: The comment about salary filters is ambiguous; update the
comment around the salary filter keys 'min' and 'max' to state that filtering is
performed against annualized compensation (i.e., inputs are converted to annual
values before filtering) and that 'max: 0' means "no upper limit"; modify the
lines that currently read "# - min/max are yearly compensation filters (before
conversion to annual)" to clearly say they are annualized values used for
filtering (after conversion) and keep the "# - max: 0 means "no upper limit""
note.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dcfb2f56-9191-4691-b736-13a00ed6c08a
📒 Files selected for processing (3)
providers/ashby.mjsscan.mjstemplates/portals.example.yml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
providers/ashby.mjs (1)
34-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat blank numeric/currency fields as missing before normalization.
Number('')becomes0, so blank payload fields can be misread as valid salary bounds; and untrimmed currency strings can cause false mismatches later.Suggested fix
- const minValue = comp.minValue == null ? null : Number(comp.minValue); - const maxValue = comp.maxValue == null ? null : Number(comp.maxValue); - const currency = typeof comp.currency === 'string' ? comp.currency : ''; + const normalizeNum = (v) => { + if (v == null) return null; + if (typeof v === 'string' && v.trim() === '') return null; + const n = Number(v); + return Number.isFinite(n) ? n : null; + }; + const minValue = normalizeNum(comp.minValue); + const maxValue = normalizeNum(comp.maxValue); + const currency = typeof comp.currency === 'string' ? comp.currency.trim() : ''; @@ - if ( - (minValue != null && !Number.isFinite(minValue)) || - (maxValue != null && !Number.isFinite(maxValue)) - ) { - return null; - } + // normalizeNum already rejects non-finite valuesAs per coding guidelines,
**/*.mjs: Ensure scripts handle missing data/ directories gracefully.Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/ashby.mjs` around lines 34 - 37, The code currently converts empty strings to 0 and leaves untrimmed currency values; update the normalization for comp.minValue and comp.maxValue to treat null/undefined/empty-string (after trimming) as null and only call Number() when the trimmed value is non-empty and numeric (e.g., check comp.minValue == null || String(comp.minValue).trim() === '' then set minValue = null else set minValue = Number(trimmedValue) and similarly for maxValue), and for currency trim the string and treat an empty trimmed value as null or '' consistently (e.g., currency = typeof comp.currency === 'string' ? comp.currency.trim() || '' : ''), and apply the same empty-string handling to the other occurrences referenced around the second location (the variables at lines 62-63) so blank inputs aren’t misinterpreted as valid numbers or mismatched currencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scan.mjs`:
- Line 174: The currency normalization currently uses (salaryFilter.currency ||
'').toUpperCase() which can produce false mismatches if salaryFilter.currency
contains surrounding whitespace; update the normalization to trim the value
before upper-casing (e.g., call trim() on salaryFilter.currency before
toUpperCase) where filterCurrency is assigned and apply the same
trim+toUpperCase normalization to the other occurrences noted (lines referenced
around the assignments at 199-201) so all comparisons use trimmed, upper-cased
currency strings.
In `@templates/portals.example.yml`:
- Line 64: Update the comment that currently states "If currency mismatch (e.g.,
USD filter, EUR job), it fails" to explicitly state that rejection only occurs
when both the filter currency and the job currency are known and different;
reword to something like "Reject only if both currencies are specified and
differ (e.g., filter=USD and job=EUR) — do not reject when one side is unknown."
This clarifies the conservative behavior and prevents implying unconditional
rejection.
---
Duplicate comments:
In `@providers/ashby.mjs`:
- Around line 34-37: The code currently converts empty strings to 0 and leaves
untrimmed currency values; update the normalization for comp.minValue and
comp.maxValue to treat null/undefined/empty-string (after trimming) as null and
only call Number() when the trimmed value is non-empty and numeric (e.g., check
comp.minValue == null || String(comp.minValue).trim() === '' then set minValue =
null else set minValue = Number(trimmedValue) and similarly for maxValue), and
for currency trim the string and treat an empty trimmed value as null or ''
consistently (e.g., currency = typeof comp.currency === 'string' ?
comp.currency.trim() || '' : ''), and apply the same empty-string handling to
the other occurrences referenced around the second location (the variables at
lines 62-63) so blank inputs aren’t misinterpreted as valid numbers or
mismatched currencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21c206ff-ac56-4e99-9e53-61e1a6b12cd6
📒 Files selected for processing (3)
providers/ashby.mjsscan.mjstemplates/portals.example.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/ashby.mjs`:
- Around line 34-38: The normalizeNum function currently converts negatives to
numbers; update it so any negative numeric result is treated as invalid by
returning null: after computing n = Number(v) (in the normalizeNum function) add
a check for Number.isFinite(n) && n >= 0 and only return n in that case,
otherwise return null; keep the existing checks for null/empty-string and
Number.isFinite but ensure negative values don't propagate.
In `@templates/portals.example.yml`:
- Line 64: Update the ambiguous comment on the salary/currency mismatch (the
line reading “If both currencies are known and mismatch, the salary is
ignored/skipped.”) to explicitly state that the job fails the salary filter
(e.g., “job is rejected/skipped by the salary filter when both currencies are
known and different”) so it is clear the whole job is filtered out rather than
only the salary field being ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04064c6d-3f2f-44a6-9688-26d51529a3b6
📒 Files selected for processing (3)
providers/ashby.mjsscan.mjstemplates/portals.example.yml
2 tasks
Contributor
Author
|
All review feedback has been addressed and the remaining CodeRabbit comments are now outdated. The implementation has been hardened for malformed payloads/configs while keeping behavior conservative and backwards compatible. Happy to make any additional adjustments if needed 👍 |
Owner
|
Model contributor path, @akashgaikwad28 — I asked for exactly this on #446 (focused salary-only PR after a design issue) and you delivered #675+#677 and fixed the range-flattening bug. Just needs a rebase onto current main (scan.mjs moved with #802 today) to clear conflicts. Rebase to green and it merges as |
akashgaikwad28
force-pushed
the
feat/salary-filter
branch
from
917b238 to
000a229
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scan.mjs`:
- Around line 55-70: The INTERVAL_MULTIPLIERS constant defined in scan.mjs is
unused and duplicates the map in providers/ashby.mjs; remove the declaration of
INTERVAL_MULTIPLIERS from scan.mjs (delete the constant block) so there is no
duplicated salary-interval mapping, or alternatively extract it into a shared
module (e.g., providers/_salary-utils.mjs) and import it from both scan.mjs and
providers/ashby.mjs if you intend to reuse it; update any references to
INTERVAL_MULTIPLIERS accordingly (none expected in scan.mjs) and run tests/lint
to confirm no imports break.
In `@test-salary-filter.mjs`:
- Around line 12-106: The test file re-implements parseCompensation and
buildSalaryFilter which risks divergence; instead export parseCompensation from
providers/ashby.mjs and export buildSalaryFilter from scan.mjs, then import
those functions into test-salary-filter.mjs and remove the duplicated
implementations; update any internal helper visibility as needed (e.g.,
INTERVAL_MULTIPLIERS or normalize helpers) so the exported functions work
unchanged when imported, and adjust the test to call the imported
parseCompensation and buildSalaryFilter directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f5c8e19-0a0f-434e-a43a-0a65f6f7f6d4
📒 Files selected for processing (6)
providers/_types.jsproviders/ashby.mjsproviders/greenhouse.mjsscan.mjstemplates/portals.example.ymltest-salary-filter.mjs
Contributor
Author
|
Thanks @santifer I have rebased the branch onto the latest main and resolved the conflicts caused by the recent scanner changes. The PR is now up to date and ready for CI once the workflows are approved. Please let me know if you'd like any further adjustments. |
… tests to avoid duplication
… filter + searchForNewUrl); register test-salary-filter.mjs in SYSTEM_PATHS
…tedAt) and SYSTEM_PATHS entries
Owner
|
Merged, @akashgaikwad28 — the conservative semantics are what sold it (no-salary-data passes, malformed config disables instead of mis-filtering), and a 124-test standalone suite for a scanner feature is above and beyond. Apologies for the conflict churn at the end: main moved under you twice today with our merge wave, so I resolved both rounds in your branch (and registered test-salary-filter.mjs in SYSTEM_PATHS while at it). Issue-first → discussion → delivery: model contribution. 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces optional salary-based filtering to the scanner pipeline.
Currently, the scanner supports title/location filtering, but compensation-sensitive users still receive jobs clearly outside their desired salary range. This change adds a lightweight and conservative salary filtering layer using structured compensation data when available.
Changes
Salary Filtering
salary_filtersupportCompensation Parsing
min,max,currency)Filtering Behavior
Example Configuration
Design Notes
This implementation intentionally stays lightweight and conservative:
The goal is to reduce downstream noise while keeping behavior predictable and backwards-compatible.
Testing Performed
node scan.mjs --dry-runsuccessfullySummary by CodeRabbit
New Features
Documentation
Tests