From 21d56a5da41cb0cc0baa4e8b07307f0593cb1ce3 Mon Sep 17 00:00:00 2001 From: Kevin L'Hereec Date: Thu, 23 Jul 2026 16:34:48 +0200 Subject: [PATCH] fix: stop protecting stale, unverified pc-builder.io catalog prices Bug found: "Gigabyte GAMING OC GeForce RTX 5070 Ti 16 GB Video Card" kept showing a stale price of 431.17 even after pc-builder.io itself no longer had a fixed offer for it (its "Add to Build" HTML only had a generic Amazon search link, no .price-box element at all, confirmed by fetching the live page directly). Root cause: scrapeComponentType's "never lose a better price" rule (added to make ScrapedPartsController#compare's persisted merchant prices durable across catalog refreshes) was too broad. It treated ANY existing price as worth protecting whenever the incoming catalog price was null or higher - including a price that had only ever come from a *previous* pc-builder.io scrape itself (priceSource: null), with no independent verification. Since pc-builder.io's own listings change constantly (a GPU can lose its priced Amazon link if the referenced ASIN goes out of stock), that logic effectively froze old pc-builder.io prices forever even after the site removed them. Fix: extracted the decision into shouldKeepExistingPrice(), which now only protects a price when priceSource is a real merchant name (set by updatePriceIfCheaper after a live comparison confirmed it on the merchant's own page) - never when priceSource is null (an unverified, previous pc-builder.io catalog price). A plain re-scrape can always correct/clear an unverified price, matching whatever pc-builder.io currently shows. 4 new unit tests for shouldKeepExistingPrice covering: unverified pc-builder.io price gets overwritten (the regression case), verified merchant price is protected from worse/missing catalog data, verified merchant price is still replaced by a genuinely cheaper catalog price, and no-existing-part returns false. Also re-ran `node ace scrape:parts` to apply the fix to already-cached data with stale unverified prices (affected ~1500 rows across the catalog before the fix, mostly likely unaffected in practice but no longer silently trusted going forward). --- app/services/part_scraper_service.ts | 40 +++++++++++++++++++------- tests/functional/scraped_parts.spec.ts | 36 +++++++++++++++++++++++ 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/app/services/part_scraper_service.ts b/app/services/part_scraper_service.ts index b3d7b08..2dde524 100644 --- a/app/services/part_scraper_service.ts +++ b/app/services/part_scraper_service.ts @@ -251,6 +251,33 @@ async function fetchCategoryProducts( return rows } +/** + * Decides whether a catalog refresh row should overwrite an existing + * ScrapedPart's price fields, or leave them untouched. + * + * Only a price that was actually *verified* via a merchant comparison + * (priceSource set to a merchant name by updatePriceIfCheaper, e.g. + * "LDLC") is protected from being overwritten by a plain catalog refresh + * — that price was confirmed live on the merchant's own page. + * + * A price with priceSource === null just came from a *previous* + * pc-builder.io catalog scrape, with no independent verification. + * pc-builder.io's own listings change constantly — a GPU can have a fixed + * Amazon price today and only a generic "search on Amazon" link (no + * price) tomorrow, if the ASIN it referenced runs out of stock. Treating + * that old, unverified pc-builder.io price as untouchable would freeze it + * forever even after the site itself removed it, which is exactly the + * bug this function fixes (a €431.17 price kept showing for a GPU + * pc-builder.io no longer prices at all). + */ +function shouldKeepExistingPrice( + existing: { price: number | null; priceSource: string | null } | null, + incomingPrice: number | null +): boolean { + if (!existing || existing.price === null || existing.priceSource === null) return false + return incomingPrice === null || existing.price <= incomingPrice +} + /** * Scrapes a single component type's mapped category and upserts the * resulting rows into the scraped_parts cache table. @@ -280,15 +307,7 @@ async function scrapeComponentType( .andWhere('sourceRef', row.sourceRef) .first() - // If a previous price comparison already resolved (and persisted) a - // real price for this part from a merchant, don't blow it away just - // because this catalog refresh came back with no price — keep - // whichever is best (see updatePriceIfCheaper for the "best of all - // known sources" logic used when a comparison finds a lower price). - const hasBetterExistingPrice = - existing?.price !== null && - existing?.price !== undefined && - (row.price === null || existing.price <= row.price) + const keepExistingPrice = shouldKeepExistingPrice(existing, row.price) await ScrapedPart.updateOrCreate( { source: SOURCE, sourceRef: row.sourceRef }, @@ -297,7 +316,7 @@ async function scrapeComponentType( name: row.name, imageUrl: row.imageUrl, scrapedAt, - ...(hasBetterExistingPrice + ...(keepExistingPrice ? {} : { price: row.price, @@ -402,6 +421,7 @@ export { ensureFreshCategory, isCategoryStale, updatePriceIfCheaper, + shouldKeepExistingPrice, CATEGORY_CONFIG, CATEGORY_SLUGS, CACHE_TTL_HOURS, diff --git a/tests/functional/scraped_parts.spec.ts b/tests/functional/scraped_parts.spec.ts index bb2f1ad..e5d4d82 100644 --- a/tests/functional/scraped_parts.spec.ts +++ b/tests/functional/scraped_parts.spec.ts @@ -7,6 +7,7 @@ import { isCategoryStale, ensureFreshCategory, updatePriceIfCheaper, + shouldKeepExistingPrice, CACHE_TTL_HOURS, } from '#services/part_scraper_service' @@ -215,3 +216,38 @@ test.group('Best-known-price persistence', () => { assert.equal(part.priceSource, 'Grosbill') }) }) + +test.group('Catalog refresh price protection (shouldKeepExistingPrice)', () => { + test('an unverified pc-builder.io price is NOT protected and gets overwritten', ({ assert }) => { + // Regression test: a GPU had price=431.17 from a previous pc-builder.io + // catalog scrape (priceSource: null, never independently verified). + // pc-builder.io later stopped showing a fixed price for it (only a + // generic Amazon search link). The stale 431.17 must NOT be kept. + const existing = { price: 431.17, priceSource: null } + + assert.isFalse(shouldKeepExistingPrice(existing, null)) + assert.isFalse(shouldKeepExistingPrice(existing, 999)) + }) + + test('a verified merchant price IS protected from a worse/missing catalog price', ({ + assert, + }) => { + const existing = { price: 350, priceSource: 'LDLC' } + + assert.isTrue(shouldKeepExistingPrice(existing, null)) + assert.isTrue(shouldKeepExistingPrice(existing, 500)) + }) + + test('a verified merchant price is replaced if the catalog price is actually cheaper', ({ + assert, + }) => { + const existing = { price: 350, priceSource: 'LDLC' } + + assert.isFalse(shouldKeepExistingPrice(existing, 200)) + }) + + test('no existing part means nothing to keep', ({ assert }) => { + assert.isFalse(shouldKeepExistingPrice(null, 100)) + assert.isFalse(shouldKeepExistingPrice(null, null)) + }) +})