You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Here are some key observations to aid the review process:
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 Security concerns
Stored script execution / path handling: this PR introduces database-backed custom scraper scripts that are later executed in the scraping browser, and the scraper now uses Origin as part of screenshot/page save paths. Please confirm that only trusted/admin users can create or edit CustomScraperConfig, and that Origin or other path segments are sanitized to prevent path traversal or writing files outside the intended storage directory.
SourceConfig.DataOrigin changes from an enum-backed integer to a required string inside the persisted SourcesJson. Please validate that the accompanying migration rewrites existing serialized configs, otherwise previously saved scraper sources may become unreadable or fail deserialization after deployment.
Salary parsing now depends on data.Salary, but the seeded/custom scraper scripts in this PR emit SalaryToParse. Please verify the DTO/JSON contract still matches, or salary extraction for RocketJobs and similar custom scrapers may silently stop working.
The seeded CustomScraperConfig entries define scripts and domains, but do not populate TestListUrl or TestDetailsUrl. Please confirm this is intentional, because the new editor/test flows may not be usable out of the box for the seeded providers without those URLs.
b.HasData(new{Id=1L,DataOrigin="Indeed",DetailsScraperScript="() => {\r\n let description = document.querySelector('#jobDescriptionText')?.innerText ?? '';\r\n\r\n let rawSalary = Array.from(document.querySelectorAll('span[class*=\"js-match-insights-provider\"]'))\r\n .filter(span => span.textContent.includes('$'))[0]?.textContent ?? '';\r\n\r\n let keywords = [];\r\n\r\n return JSON.stringify({ Description: description, Keywords: keywords });\r\n}\r\n",DetailsScrapingEnabled=true,Domain="indeed.com",ListScraperScript="() => {\r\n let titleElements = document.querySelectorAll('h2.jobTitle');\r\n let companyElements = document.querySelectorAll(\"[data-testid='company-name']\");\r\n let locationElements = document.querySelectorAll(\"[data-testid='text-location']\");\r\n\r\n let results = Array.from(titleElements).map((titleEl, i) => {\r\n let anchor = titleEl.querySelector('a');\r\n let rawUrl = anchor?.getAttribute('href') ?? '';\r\n let url = window.location.origin + rawUrl.split('&')[0].replace('/rc/clk', '/viewjob');\r\n\r\n return {\r\n Title: titleEl.innerText.trim(),\r\n Url: url,\r\n CompanyName: companyElements[i]?.innerText.trim() ?? '',\r\n Location: locationElements[i]?.innerText.trim() ?? '',\r\n OfferKeywords: []\r\n };\r\n });\r\n\r\n return JSON.stringify(results);\r\n}\r\n",PaginationScript="() => {\r\n const nextBtn = document.querySelector(\"a[data-testid='pagination-page-next']\");\r\n if (!nextBtn) return JSON.stringify({ HasNextPage: false });\r\n nextBtn.click();\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"},new{Id=2L,DataOrigin="JustJoinIt",DetailsScraperScript="() => {\r\n let descDiv = document.querySelector('h3')?.parentNode;\r\n if (!descDiv) return JSON.stringify({ Description: '', Keywords: [] });\r\n\r\n const result = {\r\n Description: [...descDiv.childNodes].slice(1).map(x => x?.textContent).join('\\n'),\r\n Keywords: [...descDiv.querySelectorAll('h4')].map(x => x?.textContent?.trim())\r\n };\r\n\r\n return JSON.stringify(result);\r\n}\r\n",DetailsScrapingEnabled=true,Domain="justjoin.it",ListScraperScript="() => {\r\n let jobItems = document.querySelectorAll(\"a.offer-card\");\r\n\r\n let results = Array.from(jobItems).map(a => {\r\n let mainDiv = a.querySelector('div > div > div');\r\n if (!mainDiv || mainDiv.childNodes.length < 3) return null;\r\n\r\n let bottomDiv = mainDiv.childNodes[2];\r\n let infoDiv = bottomDiv.childNodes[0];\r\n let keywordsDiv = bottomDiv.childNodes[1];\r\n\r\n return {\r\n Title: mainDiv.querySelector('h3')?.textContent.trim(),\r\n Url: window.location.origin + a.getAttribute('href'),\r\n CompanyName: mainDiv.querySelector('div > div > div > div > p')?.textContent.trim(),\r\n Location: infoDiv.childNodes?.[1]?.textContent.replace('Locations', ' Locations').trim(),\r\n SalaryToParse: mainDiv.querySelector('h6')?.textContent.trim(),\r\n OfferKeywords: [...keywordsDiv.childNodes].map(x => x?.textContent.trim()).slice(1)\r\n };\r\n });\r\n\r\n return JSON.stringify(results.filter(x => x != null));\r\n}\r\n",PaginationScript="(pageNumber) => {\r\n window.scrollTo(0, pageNumber * 1800);\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"},new{Id=3L,DataOrigin="NoFluffJobs",DetailsScraperScript="() => {\r\n document.querySelector('span[data-cy=\"text-fold\"]')?.click();\r\n\r\n let result = {\r\n Description: document.querySelector('common-posting-content-wrapper')?.textContent.trim(),\r\n Keywords: [...(document.querySelector('section[commonpostingrequirements]')?.querySelectorAll('li') ?? [])].map(x => x?.textContent?.trim())\r\n };\r\n\r\n return JSON.stringify(result);\r\n}\r\n",DetailsScrapingEnabled=true,Domain="nofluffjobs.com",ListScraperScript="() => {\r\n let lists = document.querySelectorAll('nfj-postings-list[listname=\"search\"]');\r\n let jobItems = Array.from(lists).flatMap(list =>\r\n Array.from(list.querySelectorAll(\"a.posting-list-item\"))\r\n );\r\n\r\n let results = Array.from(jobItems).map(a => {\r\n let aside = a.querySelector('aside');\r\n return {\r\n Title: aside.querySelector('h3')?.textContent.trim(),\r\n Url: window.location.origin + a.getAttribute('href'),\r\n CompanyName: aside.querySelector('footer > h4')?.textContent.trim(),\r\n Location: aside.querySelector('footer > nfj-posting-item-city > div > span')?.textContent.trim(),\r\n SalaryToParse: aside.querySelector('nfj-posting-item-salary')?.textContent.trim(),\r\n OfferKeywords: [...aside.querySelectorAll('nfj-posting-item-tiles > span')].map(s => s?.textContent.trim())\r\n };\r\n });\r\n\r\n return JSON.stringify(results);\r\n}\r\n",PaginationScript="() => {\r\n const button = document.querySelector('button[nfjloadmore]');\r\n if (!button) return JSON.stringify({ HasNextPage: false });\r\n button.click();\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"},new{Id=4L,DataOrigin="Olx",DetailsScrapingEnabled=false,Domain="olx.pl",ListScraperScript="() => {\r\n let cards = document.querySelectorAll('div.jobs-ad-card');\r\n\r\n let results = Array.from(cards).map(offer => {\r\n let titleContainer = [...offer.querySelectorAll('div > div > a')].at(-1);\r\n let firstRow = offer.querySelector('div:nth-child(2) > div > div');\r\n let secondRow = offer.querySelector('div:nth-child(2) > div > div:nth-child(2)');\r\n\r\n let title = titleContainer?.textContent.trim() ?? '';\r\n let url = titleContainer?.getAttribute('href') ?? '';\r\n let companyName = titleContainer?.parentNode?.querySelector('p')?.textContent.trim() ?? '';\r\n\r\n let firstRowData = [...(firstRow?.querySelectorAll('div > div > p') ?? [])]\r\n .map(p => p.textContent.trim()).reverse();\r\n let secondRowData = [...(secondRow?.querySelectorAll('button') ?? [])]\r\n .map(b => b.textContent.trim());\r\n\r\n // First item in reversed firstRowData may be salary, last is location, rest are keywords\r\n let location = firstRowData.length > 0 ? firstRowData[firstRowData.length - 1] : '';\r\n let keywords = [...firstRowData.slice(0, -1), ...secondRowData];\r\n\r\n return {\r\n Title: title,\r\n Url: url,\r\n CompanyName: companyName,\r\n Location: location,\r\n OfferKeywords: keywords\r\n };\r\n });\r\n\r\n return JSON.stringify(results);\r\n}\r\n",PaginationScript="() => {\r\n const nextBtn = document.querySelector(\"a[data-testid='pagination-forward']\");\r\n if (!nextBtn) return JSON.stringify({ HasNextPage: false });\r\n nextBtn.click();\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"},new{Id=5L,DataOrigin="PracujPl",DetailsScrapingEnabled=false,Domain="pracuj.pl",ListScraperScript="() => {\r\n // Expand hidden descriptions\r\n document.querySelectorAll('[data-test=\"section-short-description\"] div.invisible span')\r\n .forEach(el => el.click());\r\n\r\n let offers = document.querySelectorAll('div[data-test=\"positioned-offer\"], div[data-test=\"default-offer\"]');\r\n\r\n let results = Array.from(offers).map(offer => {\r\n let title = offer.querySelector('h2[data-test=\"offer-title\"]')?.textContent.trim() ?? '';\r\n let links = [...offer.querySelectorAll('a[data-test=\"link-offer\"]')].map(a => a.getAttribute('href'));\r\n let url = links[0]?.split('?')[0] ?? '';\r\n let salary = offer.querySelector('span[data-test=\"offer-salary\"]')?.textContent.trim() ?? '';\r\n let jobKeys = [...offer.querySelectorAll('ul > li')].map(li => li.textContent.trim());\r\n let description = offer.querySelector('[data-test=\"section-short-description\"]')?.textContent.trim() ?? '';\r\n let companyName = offer.querySelector('[data-test=\"text-company-name\"]')?.textContent.trim() ?? '';\r\n let location = offer.querySelector('h4[data-test=\"text-region\"]')?.textContent.trim() ?? '';\r\n\r\n return {\r\n Title: title,\r\n Url: url,\r\n CompanyName: companyName,\r\n Location: location,\r\n SalaryToParse: salary,\r\n OfferKeywords: jobKeys,\r\n Description: description\r\n };\r\n });\r\n\r\n return JSON.stringify(results);\r\n}\r\n",PaginationScript="() => {\r\n const nextBtn = document.querySelector(\"button[data-test='bottom-pagination-button-next']\");\r\n if (!nextBtn) return JSON.stringify({ HasNextPage: false });\r\n nextBtn.click();\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"},new{Id=6L,DataOrigin="RocketJobs",DetailsScraperScript="() => {\r\n let descDiv = document.querySelector('h3')?.parentNode;\r\n if (!descDiv) return JSON.stringify({ Description: '', Keywords: [] });\r\n\r\n const result = {\r\n Description: [...descDiv.childNodes].slice(1).map(x => x?.textContent).join('\\n'),\r\n Keywords: [...descDiv.querySelectorAll('h4')].map(x => x?.textContent?.trim())\r\n };\r\n\r\n return JSON.stringify(result);\r\n}\r\n",DetailsScrapingEnabled=true,Domain="rocketjobs.pl",ListScraperScript="() => {\r\n let jobItems = document.querySelectorAll(\"a.offer-card\");\r\n\r\n let results = Array.from(jobItems).map(a => {\r\n let mainDiv = a.querySelector('div > div > div');\r\n if (!mainDiv || mainDiv.childNodes.length < 3) return null;\r\n\r\n let bottomDiv = mainDiv.childNodes[2];\r\n let infoDiv = bottomDiv.childNodes[0];\r\n let keywordsDiv = bottomDiv.childNodes[1];\r\n\r\n return {\r\n Title: mainDiv.querySelector('h3')?.textContent.trim(),\r\n Url: window.location.origin + a.getAttribute('href'),\r\n CompanyName: mainDiv.querySelector('div > div > div > div > p')?.textContent.trim(),\r\n Location: infoDiv.childNodes?.[1]?.textContent.replace('Locations', ' Locations').trim(),\r\n SalaryToParse: mainDiv.querySelector('h6')?.textContent.trim(),\r\n OfferKeywords: [...keywordsDiv.childNodes].map(x => x?.textContent.trim()).slice(1)\r\n };\r\n });\r\n\r\n return JSON.stringify(results.filter(x => x != null));\r\n}\r\n",PaginationScript="(pageNumber) => {\r\n window.scrollTo(0, pageNumber * 1200);\r\n return JSON.stringify({ HasNextPage: true });\r\n}\r\n"});
The old TryParseSalary(jobOffer, rawSalary) API was removed, but multiple scrapers in this PR still call it, so the project will not compile. Add a compatibility overload that delegates to the new parser and copies the parsed values into the JobOffer.
+public static bool TryParseSalary(JobScraper.Web.Common.Entities.JobOffer jobOffer, string rawSalary)+{+ if (!TryParseSalary(rawSalary, out var salaryMinMonth, out var salaryMaxMonth, out var currency))+ return false;++ jobOffer.SalaryMinMonth = salaryMinMonth;+ jobOffer.SalaryMaxMonth = salaryMaxMonth;+ jobOffer.SalaryCurrency = currency;++ return true;+}+
public static bool TryParseSalary(string rawSalary,
out int? salaryMinMonth,
out int? salaryMaxMonth,
out string? currency)
Suggestion importance[1-10]: 10
__
Why: This is a real build-breaking issue: SalaryParser now only exposes the string/out overload, while multiple updated scrapers still call TryParseSalary(jobOffer, data.Salary). Adding a compatibility overload is accurate and preserves the intended refactor without forcing more call-site changes.
High
Align seed data schema
This seed inserts TestDetailsUrl and TestListUrl, but the schema migration in this PR does not create those columns. A fresh database update will fail when this migration runs, so either add the columns in the schema migration first or stop seeding them here.
Why: The seed migration inserts TestDetailsUrl and TestListUrl, but those properties are not present in the earlier CustomScraperConfig migration model shown in the PR, so a fresh migration run is likely to fail on CustomScraperConfigs. The suggestion correctly identifies a schema/data mismatch and proposes a safe fix path.
High
Guard CDP browser selection
ConnectOverCDPAsync only works with Chromium, so this will fail at runtime when PlaywrightMode is OverCdp and BrowserType is Firefox or Webkit. Guard that configuration explicitly and force the Chromium browser type for CDP connections.
_browser = AppSettings.PlaywrightMode switch
{
- PlaywrightModeEnum.OverCdp => await ConnectOverCdpAsync(browserType),- PlaywrightModeEnum.Local => await LaunchLocalBrowser(browserType),- _ => throw new ArgumentOutOfRangeException(nameof(AppSettings), AppSettings.PlaywrightMode, null),+ PlaywrightModeEnum.OverCdp when ScrapeConfig.BrowserType != BrowserTypeEnum.Chromium+ => throw new InvalidOperationException("`OverCdp` is only supported with `Chromium`."),+ PlaywrightModeEnum.OverCdp => await ConnectOverCdpAsync(_playwright.Chromium),+ PlaywrightModeEnum.Local => await LaunchLocalBrowser(browserType),+ _ => throw new ArgumentOutOfRangeException(nameof(AppSettings), AppSettings.PlaywrightMode, null),
};
Suggestion importance[1-10]: 8
__
Why: This is a valid runtime issue: ConnectOverCDPAsync is Chromium-specific, but the current switch uses the selected browserType for all modes. The suggested guard makes OverCdp behavior explicit and prevents misconfigured Firefox or Webkit runs from failing unexpectedly.
Medium
Fix retry success handling
LoadUntilAsync currently throws whenever retryAttempts reaches maxAttempts, even if the final page load actually satisfies successCondition. Persist the last evaluation result and only throw when the last attempt still failed, otherwise successful second attempts will be reported as errors.
Why: This correctly identifies a logic bug in LoadUntilAsync: on the second attempt, successCondition is never evaluated and retryAttempts == maxAttempts causes a throw even after a successful load. Tracking the final success state fixes a real functional error in retry handling.
Medium
Stop endless pagination
Do not always return HasNextPage: true for scroll-based sources. This can trap the scraper in an endless pagination loop once the page can no longer scroll, and the same issue exists in the RocketJobs pagination script.
Why: Always returning HasNextPage: true can indeed create a non-terminating pagination loop if the scraper depends on that flag to stop. The proposed change improves functional correctness of the seeded PaginationScript, although the exact stopping heuristic may still need tuning for dynamic infinite-scroll pages.
Medium
Restore safe config checks
DbContext.ScraperConfigs.First() will crash on a fresh database with no scraper row, and checking only scrape.Source.Disabled skips the persisted enablement guard in ScrapeConfig. Load the config safely and keep !IsEnabled in the condition so list scraping cannot run with missing or globally disabled configuration.
-ScrapeConfig = DbContext.ScraperConfigs.First();+ScrapeConfig = await DbContext.ScraperConfigs.FirstOrDefaultAsync(cancellationToken) ?? new ScraperConfig();
SourceConfig = scrape.Source;
using var userNameScope = LogContext.PushProperty("UserName", DbContext.CurrentUserName);
using var dataOriginScope = LogContext.PushProperty("DataOrigin", Origin);
-if (scrape.Source.Disabled)+if (scrape.Source.Disabled || !IsEnabled)
{
Logger.LogWarning("Scraper is disabled. Please configure {DataOrigin} origin in scraper configuration",
Origin);
return new ScrapeResponse();
}
Suggestion importance[1-10]: 7
__
Why: This is a valid robustness improvement because DbContext.ScraperConfigs.First() can throw when no row exists, and removing the !IsEnabled check weakens the original guard around persisted scraper settings. It helps prevent crashes and keeps ListScraperBaseHandler aligned with configured enablement.
Medium
Guard details configuration access
This has the same two failure modes as the list handler: First() throws when no scraper config exists, and command.Source.Disabled alone ignores the stored ScrapeConfig state. Fall back safely when the row is missing and include !IsEnabled so details scraping cannot bypass the configured disable flag.
-ScrapeConfig = DbContext.ScraperConfigs.First();+ScrapeConfig = await DbContext.ScraperConfigs.FirstOrDefaultAsync(cancellationToken) ?? new ScraperConfig();
SourceConfig = command.Source;
using var userNameScope = LogContext.PushProperty("UserName", DbContext.CurrentUserName);
using var dataOriginScope = LogContext.PushProperty("DataOrigin", Origin);
-if (command.Source.Disabled)+if (command.Source.Disabled || !IsEnabled)
{
Logger.LogWarning("Scraper is disabled. Please configure {DataOrigin} origin in scraper configuration", Origin);
return new ScrapeResponse();
}
Suggestion importance[1-10]: 7
__
Why: This mirrors the same valid issue in DetailsScraperBaseHandler: DbContext.ScraperConfigs.First() may fail on an empty database, and relying only on command.Source.Disabled can bypass persisted config state. The suggested change improves correctness and avoids an avoidable runtime failure.
Medium
Avoid text-node indexing
Avoid indexing childNodes here, because whitespace text nodes can shift positions and make bottomDiv, infoDiv, or keywordsDiv invalid. Using element-only collections prevents fragile parsing and the same fix should be applied to the duplicated RocketJobs script as well.
-ListScraperScript = "() => {\r\n let jobItems = document.querySelectorAll(\"a.offer-card\");\r\n\r\n let results = Array.from(jobItems).map(a => {\r\n let mainDiv = a.querySelector('div > div > div');\r\n if (!mainDiv || mainDiv.childNodes.length < 3) return null;\r\n\r\n let bottomDiv = mainDiv.childNodes[2];\r\n let infoDiv = bottomDiv.childNodes[0];\r\n let keywordsDiv = bottomDiv.childNodes[1];\r\n\r\n return {\r\n Title: mainDiv.querySelector('h3')?.textContent.trim(),\r\n Url: window.location.origin + a.getAttribute('href'),\r\n CompanyName: mainDiv.querySelector('div > div > div > div > p')?.textContent.trim(),\r\n Location: infoDiv.childNodes?.[1]?.textContent.replace('Locations', ' Locations').trim(),\r\n SalaryToParse: mainDiv.querySelector('h6')?.textContent.trim(),\r\n OfferKeywords: [...keywordsDiv.childNodes].map(x => x?.textContent.trim()).slice(1)\r\n };\r\n });\r\n\r\n return JSON.stringify(results.filter(x => x != null));\r\n}\r\n",
+ListScraperScript = "() => {\r\n let jobItems = document.querySelectorAll(\"a.offer-card\");\r\n\r\n let results = Array.from(jobItems).map(a => {\r\n let mainDiv = a.querySelector('div > div > div');\r\n if (!mainDiv || mainDiv.children.length < 3) return null;\r\n\r\n let bottomDiv = mainDiv.children[2];\r\n let infoDiv = bottomDiv?.children[0];\r\n let keywordsDiv = bottomDiv?.children[1];\r\n if (!infoDiv || !keywordsDiv) return null;\r\n\r\n return {\r\n Title: mainDiv.querySelector('h3')?.textContent?.trim() ?? '',\r\n Url: window.location.origin + (a.getAttribute('href') ?? ''),\r\n CompanyName: mainDiv.querySelector('div > div > div > div > p')?.textContent?.trim() ?? '',\r\n Location: infoDiv.children?.[1]?.textContent?.replace('Locations', ' Locations').trim() ?? '',\r\n SalaryToParse: mainDiv.querySelector('h6')?.textContent?.trim() ?? '',\r\n OfferKeywords: [...keywordsDiv.children].map(x => x?.textContent?.trim() ?? '').slice(1)\r\n };\r\n });\r\n\r\n return JSON.stringify(results.filter(x => x != null));\r\n}\r\n",
Suggestion importance[1-10]: 6
__
Why: The suggestion is accurate because indexing childNodes is fragile when whitespace text nodes are present, and switching to children makes the scraper less brittle. It can prevent incorrect parsing or runtime failures in the seeded ListScraperScript, with similar relevance to the duplicated RocketJobs script.
Low
Guard missing DOM nodes
Guard against missing aside elements before dereferencing them. As written, one malformed card will throw and abort the entire NoFluffJobs scrape instead of skipping only the bad item.
Why: This is a valid robustness fix: dereferencing aside without checking can throw and abort the entire NoFluffJobs scrape for one malformed card. The impact is moderate because it hardens seeded scraper logic, though it is in an auto-generated migration designer file.
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
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.
PR Type
Enhancement, Tests
Description
Add configurable custom scraper infrastructure
Seed scripts for supported job boards
Refactor shared scraper setup and parsing
Add custom scraper and salary tests
Diagram Walkthrough
File Walkthrough
9 files
Add standalone custom scraper config entityExtend supported origins for custom scrapingExtract reusable page creation and setupRefactor shared scraper lifecycle and origin handlingExecute configured scripts for list scrapingExecute configured scripts for details scrapingAdd models for custom scraper editingAdd page logic for editing scrapersReuse shared origin and salary parsing5 files
Add single-script execution test workflowAdd end-to-end custom scraper validation flowAdd integration coverage for scripted list scrapingBuild custom scraper configs for integration testsAlign salary tests with shared parser6 files
Seed default scraper configs for known sitesCreate persistence for custom scraper configsAdd test URLs to scraper configsConvert scraper config into standalone tableSeed built-in scripts for multiple job boardsSync EF snapshot with scraper schema61 files