Skip to content

fix: avoid recording a duplicate price_history row on every scheduled check - #45

Open
Osamaali313 wants to merge 1 commit into
clucraft:mainfrom
Osamaali313:fix-price-history-dedup
Open

fix: avoid recording a duplicate price_history row on every scheduled check#45
Osamaali313 wants to merge 1 commit into
clucraft:mainfrom
Osamaali313:fix-price-history-dedup

Conversation

@Osamaali313

Copy link
Copy Markdown

Summary

The scheduler's dedup guard never matches, so an identical price_history row is written on every scheduled check even when the price hasn't changed.

backend/src/services/scheduler.ts:

const latestPrice = await priceHistoryQueries.getLatest(product.id);
// Only record if price has changed or it's the first entry
if (!latestPrice || latestPrice.price !== scrapedData.price.price) {

price_history.price is DECIMAL(10,2), and node-postgres returns DECIMAL/NUMERIC as a string (there's no pg.types.setTypeParser in the repo). So latestPrice.price is "19.99" while scrapedData.price.price is the number 19.99. !== does no coercion, so "19.99" !== 19.99 is always true and the guard passes every time.

The intent is clearly to skip unchanged prices — the comment says so and the else branch logs Price unchanged for product ..., which is currently unreachable. The neighbouring price-drop and target-price checks already coerce with parseFloat(String(latestPrice.price)); this one comparison just missed it.

Impact

At the default 1-hour refresh_interval, each product accumulates ~24 identical rows per day. price_history grows without bound, the "Price unchanged" optimization is dead code, and the price chart / price_count stat get padded with redundant points. (Notifications are unaffected — the drop/target checks use parseFloat, so a zero delta correctly fails the threshold.)

Fix

Coerce the stored value before comparing, matching what the surrounding checks already do:

if (!latestPrice || parseFloat(String(latestPrice.price)) !== scrapedData.price.price) {

An actual price change still records a new row; an unchanged price now correctly hits the else branch.

Testing

The repo has no test harness, so I verified by tracing the runtime types: getLatest returns the raw pg row (price as string), PriceInfo.price is typed number and produced via parseFloat, and there is no global decimal type parser. Happy to add a small priceChanged() helper + unit test if you'd like a harness set up.

The dedup guard in the price scheduler compared the previous price with
the freshly scraped one using a strict `!==`. `latestPrice.price` comes
back from Postgres as a string (the column is DECIMAL), while
`scrapedData.price.price` is a number, so `"19.99" !== 19.99` is always
true and the guard never matched. As a result a new, identical
price_history row was inserted on every scheduled check even when the
price had not changed, and the "Price unchanged" branch was dead code.

The surrounding price-drop and target-price checks already coerce with
`parseFloat(String(latestPrice.price))`; apply the same coercion here so
the comparison works on numbers.
Copilot AI lite review requested due to automatic review settings August 5, 2026 19:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the scheduler’s “record price only when changed” dedup guard by coercing the latest stored price_history.price (returned from pg as a string for DECIMAL/NUMERIC) to a number before comparing it to the freshly scraped numeric price. This prevents writing redundant price_history rows on every scheduled run when the price has not changed.

Changes:

  • Coerce latestPrice.price with parseFloat(String(...)) before comparing to scrapedData.price.price.
  • Add an inline comment explaining the pg DECIMAL string vs number mismatch and why coercion is needed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +108 to +112
// Only record if price has changed or it's the first entry.
// latestPrice.price comes back from pg as a string (DECIMAL), while
// scrapedData.price.price is a number, so a raw !== is always true;
// coerce first (as the notification checks below already do).
if (!latestPrice || parseFloat(String(latestPrice.price)) !== scrapedData.price.price) {
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.

2 participants