Fix plot_count: reconcile storyline after plot upsert#258
Conversation
After upserting a plot row, reconcile the parent storyline's plot_count (via COUNT(*)) and last_plot_time (via MAX block_timestamp). Uses idempotent queries — safe for duplicate indexer calls and backfill replays. Applied to both the real-time indexer (index/plot/route.ts) and the backfill cron (cron/backfill/route.ts). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
REQUEST CHANGES — one blocking issue, one non-blocking observation.
Blocking: 26-line reconciliation block duplicated verbatim across two files.
The exact same reconciliation logic appears in both src/app/api/index/plot/route.ts:136-158 and src/app/api/cron/backfill/route.ts:289-311. If the reconciliation query ever needs adjustment (e.g., adding error logging, handling edge cases), both copies must be updated in lockstep — this is a maintenance hazard in critical data-consistency code.
Extract into a shared helper, e.g.:
// lib/reconcile.ts
export async function reconcileStoryline(supabase: SupabaseClient, storylineId: number) { ... }Both call sites become one-liners. The logic itself is correct and idempotent — this is purely about not duplicating data-integrity code.
Non-blocking: silent failure on reconciliation errors.
Neither call site checks the error return from the count query, the latestPlot query, or the final update. If reconciliation fails (e.g., Supabase transient error), it's invisible — the indexer still returns { success: true }. Consider at minimum a console.error on failure so it's observable in logs.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The reconciliation approach is the right shape for idempotency, but both new code paths ignore database errors from the reconciliation queries and storyline update.
Findings
- [medium] The new post-upsert reconciliation can fail silently. In both routes, the
COUNT(*)query, thelatestPlotquery, and the finalstorylines.update(...)call discard Supabaseerrorresults. That means the handler can return success while leavingplot_count/last_plot_timestale, which defeats the fix and makes retries harder because the indexer thinks it already succeeded.- File:
src/app/api/index/plot/route.ts:137 - File:
src/app/api/cron/backfill/route.ts:289 - Suggestion: capture and check the
errorfield from both read queries and the storylineupdate, and fail the request/job when reconciliation does not complete.
- File:
Decision
Request changes. The idempotent reconciliation logic is correct, but it needs real error handling so indexer success actually guarantees storyline state was updated.
- Extract reconcileStorylinePlotCount to lib/reconcile.ts (DRY) - Throws on any Supabase error so callers can handle failures - Indexer route returns 500 on reconciliation failure - Backfill cron propagates error to per-event error counter - Addresses T2a (error handling) and T2b (dedup) review feedback Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
APPROVE
Both issues resolved:
-
DRY: Shared
lib/reconcile.tswithreconcileStorylinePlotCount()— both call sites are now one-liners. Clean typed signature withSupabaseClient<Database>. -
Error handling: All three Supabase operations (count, latest plot, update) now throw on error. Indexer catches and returns 500 with
console.error. Backfill propagates the throw to the existing error counter. Good separation of error strategy per caller.
Logic unchanged and still idempotent. No issues found.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The follow-up patch fixes the reconciliation error-handling gap and removes the duplication by extracting a shared helper. Failures now propagate correctly through both the realtime indexer and the backfill path.
Findings
- [info] No blocking issues found in the updated patch.
Decision
Approve. reconcileStorylinePlotCount() now throws on Supabase read/update failures, the indexer returns 500 on reconciliation failure, and the backfill path propagates the error to its existing failure handling. CI was still pending when reviewed.
Summary
plot_count(viaCOUNT(*)) andlast_plot_time(via MAXblock_timestamp)src/app/api/index/plot/route.ts) and the backfill cron (src/app/api/cron/backfill/route.ts)Fixes #257
Test plan
storylines.plot_countincrements andlast_plot_timeupdatesnpm run typecheckpasses🤖 Generated with Claude Code