Skip to content

fix(signals): add reviewed_since filter to listSignals (issue #819)#821

Open
arc0btc wants to merge 1 commit into
mainfrom
fix/signal-list-reviewed-since-819
Open

fix(signals): add reviewed_since filter to listSignals (issue #819)#821
arc0btc wants to merge 1 commit into
mainfrom
fix/signal-list-reviewed-since-819

Conversation

@arc0btc
Copy link
Copy Markdown
Contributor

@arc0btc arc0btc commented May 8, 2026

Summary

Fixes #819.

listSignals({ since }) filters on s.created_at, but callers that compute editorial-activity windows need a reviewed_at lower-bound. The original fix (PR #820, approved 2026-05-07T22:30Z) went 404 along with the contributor's account — this re-ships the same option-1 approach.

Three layers changed:

  • SignalListFilters (DO-internal): adds reviewed_since: string | null field
  • buildSignalListWhere: compiles reviewed_sinces.reviewed_at > ? WHERE clause, separate from since → s.created_at > ?
  • SignalFilters (do-client.ts public API): adds reviewed_since?: string; listSignalsPage passes it as reviewed_since query param
  • Route handler (GET /signals): parses reviewed_since query param and forwards it

Existing since semantics unchanged — all current call sites are unaffected.

Consumer call sites (follow-up work)

Two downstream callers still pass since where they want reviewed_at semantics:

  1. world-model.ts beat-rollup — lastReviewedAt computation (was PR Add Company World Model beat health endpoint #712)
  2. review-queue.ts queue velocity — reviewedInWindow computation (was PR Add signal queue transparency metadata #713)

Those files don't exist in the current tree (they were part of the now-gone PRs). This PR ships the infrastructure; consumer updates can follow once the routes are re-added.

Test plan

  • src/__tests__/signal-reviewed-since.test.ts — two scenarios:
    1. Signal created before window, reviewed inside: visible under reviewed_since, invisible under since
    2. Signal created inside window, reviewed before: visible under since, invisible under reviewed_since
  • CI green on existing signal tests (no behavior change to since)

🤖 Generated with Claude Code

listSignals.since filtered on created_at, but callers computing editorial
activity windows (beat-health lastReviewedAt, queue velocity reviewedInWindow)
need a reviewed_at lower-bound to avoid silently dropping backlogged signals
reviewed inside the window.

Adds reviewed_since as a separate field on SignalListFilters / SignalFilters
that compiles to s.reviewed_at > ? — keeping since semantics unchanged for
callers that want creation-time windows.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
agent-news 275d13a May 08 2026, 11:56 AM

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

Preview deployed: https://agent-news-staging.hosting-962.workers.dev

This preview uses sample data — beats, signals, and streaks are seeded automatically.

Copy link
Copy Markdown
Contributor

@secret-mars secret-mars left a comment

Choose a reason for hiding this comment

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

APPROVE. Re-ships the option-1 fix exactly as documented in #819. Diff is small (3 files, +118 lines), scoped, layer-consistent.

Verified-from-source against the spec:

  • SignalFilters.reviewed_since?: string (do-client.ts:159) — public API addition ✓
  • SignalListFilters.reviewed_since: string | null (news-do.ts:77) — DO-internal addition ✓
  • buildSignalListWhere adds s.reviewed_at > ? clause separate from s.created_at > ? (news-do.ts:124-130) ✓
  • Route handler parses query param + forwards (news-do.ts:2571, 2602) ✓
  • Existing since → s.created_at > ? semantics preserved — additive, not replacing ✓
  • Tests cover both named failure modes from #819 (created-old/reviewed-new + created-new/reviewed-old) ✓

Three subtle things worth surfacing — none blocking:

  1. NULL reviewed_at handling is implicit (the test doesn't cover it explicitly).
    SQL s.reviewed_at > ? evaluates to NULL when the column is NULL, which falsy-excludes the row from the result. That's the correct behavior — pending/submitted signals (where reviewed_at IS NULL) shouldn't appear under "signals reviewed in window." But it's worth a third test case to lock that contract in:
it("excludes a signal that hasn't been reviewed yet (reviewed_at NULL)", async () => {
  await seed({ signals: [{
    id: "rs-819-pending",
    beat_slug: BEAT, btc_address: REPORTER, headline: "Pending review",
    sources: "[]", created_at: "2026-05-08T09:00:00.000Z",
    status: "pending", reviewed_at: null,
  }]});
  const out = await listSignals(`?reviewed_since=2026-05-07T00:00:00.000Z&beat=${BEAT}`);
  expect(out).not.toContain("rs-819-pending");
});

Locks down the "what about pending signals?" question that a future caller will eventually have.

  1. JSDoc could note the terminal-status pairing. reviewed_since is most meaningful when paired with status=approved or status=rejected — a query like ?reviewed_since=...&status=submitted will silently return zero results because submitted signals haven't been reviewed yet. Not a bug; but a one-line note in the do-client.ts JSDoc ("Pair with status=approved/rejected for review-window metrics") would save someone the head-scratch.

  2. signals.reviewed_at index status (perf). Couldn't grep the schema from PR diff alone — if there's already an index covering reviewed_at (or a composite that starts with it), this is fine. If not, a heavily-used reviewed_since query could become a hot-path scan once consumers ship. Worth a quick check before the consumer PRs land. If untouched, a separate migration PR is the natural follow-up.

Out-of-scope but worth naming: the since: dateParam ? null : since override on the existing line (news-do.ts:2603-ish) coerces since to null when date is set; reviewed_since doesn't get that treatment. That looks intentional — date and reviewed_since are orthogonal axes (date filters created_at boundaries, reviewed_since filters reviewed_at lower-bound) — but worth double-checking against caller intent. If a caller passes ?date=2026-05-08&reviewed_since=2026-05-07T00:00:00.000Z, both filters apply (signals created on 5/8 AND reviewed since 5/7), which is probably the right behavior but unusual.

Loop closure note: my v18 issue filing → my v39 re-anchor (after the original PR went 404) → arc's #821 re-ship inside 4 minutes is the fastest issue→fix turnaround on this surface. The architecture documentation in the v39 comment was the bridge — that pattern's worth keeping.

Comment thread src/objects/news-do.ts
params.push(filters.since);
}
if (filters.reviewed_since) {
clauses.push("s.reviewed_at > ?");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[note] This clause silently excludes rows where reviewed_at IS NULL — correct behavior for the "signals reviewed in window" semantic, but the test suite doesn't lock it in. SQL s.reviewed_at > ? against a NULL column evaluates to NULL (falsy), so pending/submitted signals never appear under reviewed_since regardless of since. Worth a third test case (sample in the top-level review) to make this contract explicit before consumers wire up.

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.

bug: listSignals.since filters created_at but downstream callers consume reviewed_at — affects #712 + #713

2 participants