Skip to content

feature: show what changed when a watched page changes - #66

Open
asasemahmed wants to merge 3 commits into
CopilotKit:mainfrom
asasemahmed:feature/watch-changes
Open

asasemahmed wants to merge 3 commits into
CopilotKit:mainfrom
asasemahmed:feature/watch-changes

Conversation

@asasemahmed

@asasemahmed asasemahmed commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

What changed

A "page changes" alert now lists what changed: New, Updated and Removed lines.

  • Only relative times are muted. A line where just "3 minutes ago" or "posted 1 day ago" ticked is left out, and a check where nothing else changed stays quiet.
  • Every other number change alerts and is listed under Updated: a price ($399.99 → $279.99), stock ("Only 3 left" → "Only 0 left"), a count ("Tickets available: 12" → "0") or a version (1.2.3 → 2.0.0).
  • The line-by-line diff is used only when the saved lines belong to the committed baseline (lastHash). If a task outcome was lost after the lines were saved, the retry still alerts, with the plain "Condition met" text in place of the list, so the change is never lost.

Verification

  • Unit tests for the line comparison, including the price, stock, count, version and "1 comment" → "2 comments" cases.
  • API tests: the alert text, the quiet relative-time case, and a new test that a price-only change alerts. The price test fails on the previous version of this PR.
  • Rebased onto main. a page change stays alertable when the task outcome is lost after the baseline moves (from fix: keep monitor state from committing ahead of its task outcome (#30) #41) failed after the rebase and passes with the baseline check.
  • pnpm typecheck passes, biome ci passes on the changed files, and pnpm test: 213 passed, 0 failed, 1 skipped (POSIX-only).

Integration limits

Pages with counters (points, comment counts) now alert when a counter changes, since those are number changes.

@kvnloo

kvnloo commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Benchmarked the per-tick cost of pageLines + diffPage (this runs on every change-watch tick in service.ts), against the PR head, to answer: does this add meaningful latency to the watch loop?

Methodology. Node 24, imported apps/server/src/engine/page-diff.ts directly from this PR's head (094e58adf08b). Synthetic HN-style page text (story lines with points + relative times, ~120 chars/line). 5 warm-up runs, then 40 timed iterations; medians reported. Realistic tick = same base page with ~2% of lines changed (half content changes, half timestamp-only) plus 5 appended lines.

Measured (realistic tick).

page size pageLines diffPage total/tick
500 lines 1.03ms 2.54ms ~3.6ms
2000 lines (the pageLines cap) 4.52ms 9.98ms ~14.5ms

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: diffPage calls shape() ~5x per changed line (once per side in the two Set builds, then twice more per changed line in the added/updated filters). I verified a cached-shape variant (each line's shape computed once per side, into a Map) produces byte-identical output on the same inputs, and it cuts diffPage from 9.98ms to 5.94ms median at 2000 lines (~40%). Not needed for this to land — just a cheap win if you're ever in there.

Not validated: real page text (mine is synthetic), behavior under the worker's actual memory/GC pressure, and anything outside page-diff.ts (DB read/write of monitor-pages per tick wasn't timed).

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).

@asasemahmed asasemahmed reopened this Sep 25, 2026
@asasemahmed

Copy link
Copy Markdown
Contributor Author

Thanks for benchmarking this! Great to see that the current implementation adds negligible latency to the watch loop.

The cached shape() approach looks like a nice optimization too. I'll keep it as a separate follow-up so it doesn't expand the scope of this PR.

@davidmckayv

Copy link
Copy Markdown
Contributor

Needs a change before merge. The mute for number-only changes hides real changes on a change watch: $399.99 to $279.99, "Only 3 left" to "Only 0 left", "Tickets available: 12" to "0", and 1.2.3 to 2.0.0 all produce no alert, and the saved baseline still advances, so the change is lost rather than delayed. The PR's own unit test files "Price $12" under Updated. Keep the diff text in the alert but still alert on number changes, or make the mute opt-in, or limit it to relative times.

@asasemahmed

Copy link
Copy Markdown
Contributor Author

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.

kvnloo commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Re-checked current head ed6e9f6e34. This addresses the blocker cleanly.

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 lastHash, the next change stays alertable instead of diffing against an uncommitted snapshot.

My earlier per-tick perf result still applies to this shape; I don't see another blocker from that review path.

This branch has not been deployed

No deployments
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.

3 participants