From c72bc58b67beb5848daaa0e150ae81c1ec54ce89 Mon Sep 17 00:00:00 2001 From: Luke Date: Sat, 4 Jul 2026 17:42:34 -0700 Subject: [PATCH] Fix zeroed "caught before merge" metric and empty severity trend chart Two independent bugs surfaced on the Impact/Overview pages of live instances (both masked by demo-mode fixtures): 1. "critical & major caught before merge" / "merged PRs" always read 0. prs.merged_at was never written on a live instance: the webhook pull_request.closed handler only aborted in-flight reviews, and the review path upserts merged_at back to NULL on every review. The Impact/Overview metrics (and cross-PR memory) filter on merged_at IS NOT NULL, so they stayed 0 unless the manual backfill CLI was run. Fix: add markPRClosed() and call it from the closed handler to persist merged/merged_at/closed_at from the payload. (Existing merged PRs still need `npm run backfill` to populate historically.) 2. "Findings by severity over time" chart rendered empty on the Impact page. StackedSeverityBar uses percentage-height segments, which only resolve against a definite ancestor height. .chart-bar had flex:1 1 0% + min-height (not definite); it worked on Overview only because a grid row stretched the card. Standalone on Impact the columns collapsed to 0. Fix: give .chart-bar a definite `height` (flex:1 1 auto; height:200px) so it renders in any layout while still growing to fill a stretched card. Verified: backend + web tsc clean, smoke:dao/smoke:webhooks pass, a temp-DB script drives getImpact() showing caught-before-merge flip 0->2, and the Impact chart renders live in demo mode. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/storage/dao.ts | 41 +++++++++++++++++++++++++++++++++++++++++ src/webhook/dispatch.ts | 15 ++++++++++++--- web/src/styles/base.css | 11 +++++++++-- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/storage/dao.ts b/src/storage/dao.ts index 2ed586c..d544bf7 100644 --- a/src/storage/dao.ts +++ b/src/storage/dao.ts @@ -62,6 +62,47 @@ export function recordPR(ctx: PRContext, extras: { state?: string; closedAt?: st } } +/** + * Record a PR close/merge from the webhook `pull_request.closed` event. + * + * This is the ONLY live path that persists `prs.merged_at`. The review path + * calls `recordPR(ctx, { state: "open" })` with no `mergedAt`, and that upsert + * always writes `merged_at = excluded.merged_at` (i.e. NULL), so a merge is + * never captured during review. A merge emits no further `synchronize` events, + * so setting `merged_at` here on close persists — which is what the Impact / + * Overview "caught before merge" and "merged PRs" metrics (they filter on + * `merged_at IS NOT NULL`) and cross-PR memory depend on. + * + * UPDATE-only by design: those metrics JOIN reviews → prs, so any PR that + * matters here already has a row from `recordPR` during its review. A PR the + * bot never reviewed no-ops harmlessly (it carries no findings anyway). + */ +export function markPRClosed( + owner: string, + repo: string, + number: number, + info: { merged: boolean; mergedAt?: string | null; closedAt?: string | null }, +): void { + const db = openDatabase(); + if (!db) return; + try { + db.prepare( + `UPDATE prs + SET state = ?, closed_at = ?, merged_at = ? + WHERE owner = ? AND repo = ? AND number = ?`, + ).run( + info.merged ? "merged" : "closed", + info.closedAt ?? null, + info.merged ? info.mergedAt ?? null : null, + owner, + repo, + number, + ); + } catch (err) { + logger.debug({ err }, "dao.markPRClosed failed"); + } +} + /** Insert a review row, return its rowid. Returns null when persistence is disabled. */ export function recordReview(opts: { ctx: PRContext; diff --git a/src/webhook/dispatch.ts b/src/webhook/dispatch.ts index ef89f85..5a9c57e 100644 --- a/src/webhook/dispatch.ts +++ b/src/webhook/dispatch.ts @@ -1,5 +1,5 @@ import { logger } from "../logger.js"; -import { recordEvent } from "../storage/dao.js"; +import { markPRClosed, recordEvent } from "../storage/dao.js"; import { bus } from "../realtime/bus.js"; import { runReviewJob } from "../realtime/jobs.js"; import { isPauseAll, isAutoReviewEnabled } from "../settings/overrides.js"; @@ -187,9 +187,18 @@ export async function dispatchWebhookEvent( return { status: 202, body: { status: "accepted" } }; } - // closed — abort in-flight reviews + // closed — record the close/merge, then abort in-flight reviews if (action === "closed") { - logger.info({ owner, repo, pr: number, action }, "PR closed, aborting any in-flight review"); + // Persist merge/close status. This is the only live path that sets + // prs.merged_at, which the Impact/Overview "caught before merge" and + // "merged PRs" metrics (and cross-PR memory) are gated on. + const pr = payload.pull_request; + const merged = !!pr.merged; + markPRClosed(owner, repo, number, { merged, mergedAt: pr.merged_at, closedAt: pr.closed_at }); + logger.info( + { owner, repo, pr: number, action, merged }, + merged ? "PR merged, recording merge and aborting any in-flight review" : "PR closed, aborting any in-flight review", + ); reviewer.handlePRClose(owner, repo, number); return { status: 200, body: { status: "ok" } }; } diff --git a/web/src/styles/base.css b/web/src/styles/base.css index 8930ece..062ad4c 100644 --- a/web/src/styles/base.css +++ b/web/src/styles/base.css @@ -1169,8 +1169,15 @@ label.field { display: flex; align-items: flex-end; gap: 3px; - flex: 1; - min-height: 140px; + /* `height` (not just min-height) so the bar has a *definite* height in any + layout context: percentage-based column/segment heights only resolve + against a definite ancestor. `flex: 1 1 auto` still lets the bar grow to + fill a taller stretched card (e.g. the Overview grid), but the explicit + height is the definite fallback when the card body is content-sized (e.g. + the standalone Impact "Findings by severity over time" card), which + otherwise collapsed every column to 0. */ + flex: 1 1 auto; + height: 200px; padding: 6px 0; } .chart-bar .col {