Repository navigation
feature: show what changed when a watched page changes - #66
Conversation
|
Benchmarked the per-tick cost of Methodology. Node 24, imported Measured (realistic tick).
p95 stayed under 20ms in all cases. Verdict: negligible — at watch intervals measured in minutes, ~15ms/tick at the 2000-line cap needs no offloading or batching. One measured inefficiency, if you want it: Not validated: real page text (mine is synthetic), behavior under the worker's actual memory/GC pressure, and anything outside Benchmark script kept locally; happy to share it if useful. Posted by Kevin's agent on his behalf — AI-assisted (Muse, Meta's Muse Spark). |
|
Thanks for benchmarking this! Great to see that the current implementation adds negligible latency to the watch loop. The cached |
|
Needs a change before merge. The mute for number-only changes hides real changes on a change watch: |
094e58a to
ed6e9f6
Compare
|
Addressed the review: only relative times are muted now, so price, stock, count and version changes alert and show under Updated. Rebasing onto #41 also showed the saved lines could run ahead of a lost outcome, so the diff now only uses the committed baseline. |
|
Re-checked current head The mute is now narrow to relative-time-only churn; price/stock/count/version changes still produce a meaningful update. The extra baseline guard is also the right fail-safe: if saved lines ran ahead of the committed My earlier per-tick perf result still applies to this shape; I don't see another blocker from that review path. |
jerelvelarde
left a comment
There was a problem hiding this comment.
Value: useful detailed change receipts. Template: clear checks, recovery evidence and integration limits. Security: no additional permission issue identified. However, two real sample monitor API regressions each expect one alert and get zero: removing a numbered listing, and changing text beyond the stored excerpt. The 32 supplied focused tests pass but miss these cases. Also resolve the current service.ts conflict while preserving main's alertSequence behavior.
| (line) => | ||
| !after.has(line) && | ||
| !afterTimeless.has(timeless(line)) && | ||
| !afterNumberless.has(numberless(line)), |
There was a problem hiding this comment.
[P2] A current line with the same numberless shape is not proof that a removed line was updated. For ['Ticket 1: available','Ticket 2: available'] becoming ['Ticket 1: available'], afterNumberless contains both shapes and the removal disappears; diff is empty and the monitor emits no alert. Use one-to-one matching that consumes unchanged lines before pairing updates, and cover deletion of one numbered listing.
| ? diffPage(previousPage.lines, lines) | ||
| : undefined; | ||
| // Only relative times changed ("3 minutes ago"): keep watching quietly. | ||
| const quiet = Boolean(diff && !meaningfulPageDiff(diff)); |
There was a problem hiding this comment.
[P2] pageLines truncates each line to 300 characters and stops after 2000 lines, so an empty display diff cannot prove only relative timestamps changed. Changing 'x'.repeat(300)+' Price: $50' to the same prefix+' Price: $40' changes the hash but emits zero alerts in the sample monitor API. Compare full normalized content or track truncation and retain a generic alert when equality cannot be established; cap only the displayed summary.
ed6e9f6 to
ef1584e
Compare
|
Thanks for catching those. Both are fixed:
Rebased onto main with the |
jerelvelarde
left a comment
There was a problem hiding this comment.
Re-reviewed author update: both previous alert-loss defects are fixed. Removing a numbered listing now alerts, and whole-page fingerprints detect changes beyond the bounded summary. Template describes verification and limits; no new actionable security finding. Integrated current main and corrected API-test isolation so independent tests get fresh limiter middleware while retaining DB/session fixtures; production rate limits remain unchanged. All 44 page-diff/API/monitor-recovery tests, changed-file Biome and diff checks pass. Approval applies to this head; merge only after all seven refreshed CI checks pass, including the browser job whose prior WORKER_FAILURE was not diagnosed as transient.
What changed
A "page changes" alert now lists what changed: New, Updated and Removed lines.
lastHash). If a task outcome was lost after the lines were saved, the next check still alerts with the plain text.alertSequencenotice key is kept, so returning to a seen page still alerts.Verification
Unit tests: line pairing, the price/stock/count/version cases, a removed numbered listing, relative-time-only lines, and the page fingerprint.
API tests on the sample monitor: the alert text, quiet on relative-time-only changes, an alert on a price-only change, an alert when a numbered listing is removed, and an alert for a change past the saved lines (a long line and a page over 2,000 lines). The last two reproduce the review cases and fail on the previous head.
Rebased onto main.
pnpm typecheckpasses,biome cipasses on the changed files, andpnpm test: 434 passed, 1 failed, 3 skipped. The failure isworker recovery preserves downloads when metadata cannot be inspected, which needs symlink permission on Windows and fails on main the same way.Maintainer integration verification: current main merged; each independent API test now gets fresh middleware state while preserving database/session fixtures, fixing CI request-budget exhaustion without changing production limits. All 44 focused page-diff/API/monitor-recovery tests, changed-file Biome and diff hygiene pass on the updated head. Full-suite/platform/browser/container checks are running in refreshed CI.
Integration limits
Pages with counters (points, comment counts) alert when a counter changes, since those are number changes.