Skip to content

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

Merged
jerelvelarde merged 2 commits into
CopilotKit:mainfrom
asasemahmed:feature/watch-changes
Oct 6, 2026
Merged

jerelvelarde merged 2 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.

  • Whether a check alerts is decided from the whole page text with only relative times ("3 minutes ago", "posted 1 day ago") blanked out. If that text is unchanged, the watch stays quiet; any other change anywhere on the page alerts, including past the 300-character line and 2,000-line limits of the summary.
  • The summary pairs old and new lines one to one. A line where only a number changed (a price, stock, count or version) is listed under Updated; a removed line is listed under Removed even when a similar numbered line remains.
  • When the change is outside the summarized lines, the alert quotes the page as before.
  • The summary 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 next check still alerts with the plain text.
  • Main's alertSequence notice 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 typecheck passes, biome ci passes on the changed files, and pnpm test: 434 passed, 1 failed, 3 skipped. The failure is worker 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.

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

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread apps/server/src/engine/page-diff.ts Outdated
(line) =>
!after.has(line) &&
!afterTimeless.has(timeless(line)) &&
!afterNumberless.has(numberless(line)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Comment thread apps/server/src/engine/service.ts Outdated
? diffPage(previousPage.lines, lines)
: undefined;
// Only relative times changed ("3 minutes ago"): keep watching quietly.
const quiet = Boolean(diff && !meaningfulPageDiff(diff));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@asasemahmed
asasemahmed force-pushed the feature/watch-changes branch from ed6e9f6 to ef1584e Compare October 6, 2026 22:04
@asasemahmed

Copy link
Copy Markdown
Contributor Author

Thanks for catching those. Both are fixed:

  • Removed numbered listing: the summary now pairs old and new lines one to one, so a remaining "Listing 102" can no longer hide a removed "Listing 101".
  • Change past the stored lines: quiet is now decided from the whole page text with only relative times blanked out, not from the trimmed lines, so a change anywhere on the page alerts. The alert quotes the page when the change is outside the summary.

Rebased onto main with the alertSequence notice key kept. Both cases have API tests that fail on the previous head.

@jerelvelarde jerelvelarde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@jerelvelarde
jerelvelarde merged commit 2a68bb6 into CopilotKit:main Oct 6, 2026
7 checks passed
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.

4 participants