Skip to content

Commit 12742fd

Browse files
msoginclaude
andcommitted
fix(timeline): address PR #48 review (C1, I1–I4, S1, S3) (GLOOK-10)
C1 [critical] — org.ts in-flight overlay bucket was missing the new required prs / avgLinesPerPr fields. Weeks with only in-flight commits would have rendered NaN bars / undefined tooltips. Zero-defaulted both in the bucket initializer. I1 [important] — PR-level P95 threshold used `floor(N * 0.95)` which at N ≤ 20 indexes the max element, making the "outliers excluded" filter a no-op on typical dev pages. Switched to `ceil(N * 0.95) - 1` so the filter actually kicks in at N ≥ 20. Added regression tests at N=20 (boundary) and N=10 (degrades to no-filter, by design). I2 [important] — avgLinesPerPr cross-week semantic was undocumented. Added an in-code comment explaining: a PR's TOTAL across all weeks decides P95 inclusion (stable filter), but only the per-week slice of lines goes into the numerator and the denominator counts PRs active in that week. Added a cross-week test that locks down the semantic. I3 [important] — spec + plan docs claimed only one chart shipping. Both updated to reflect what landed: two charts (PRs/Week + Avg Lines/PR with P95 filter), the SQL pr_number fix, the in-flight-only week edge case, and the cyan/pink color choices. I4 [important] — PRs/Week was #A78BFA (purple-400), colliding with AI% on the same chart grid (#A855F7, purple-500). Swapped to #06B6D4 (cyan). S1 — Avg Lines/PR now has suffix=" lines" on the tooltip. S3 — Label appended with " (outliers excluded)" to match the precedent set by LinesChangedChart. Skipped: S4 (PR distinctness across repos — pre-existing, separate ticket); S2/S5/S6 (covered by I1/I3 fixes). Co-Authored-By: Claude <noreply@anthropic.com>
1 parent d9351c5 commit 12742fd

7 files changed

Lines changed: 100 additions & 35 deletions

File tree

docs/superpowers/plans/2026-05-26-glook-10-prs-first-class.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@
22

33
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
44
5+
> **Scope expansion (post-implementation):** Tasks 1–4 below cover the original PRs / Week chart. During the smoke step the user requested:
6+
> - A companion **Avg Lines Changed / PR** chart (added alongside PRs / Week on both pages).
7+
> - A **P95 outlier filter** on that chart, mirroring the existing Lines Changed smoothing.
8+
> - A **fix** for the `pr_number` column missing from the org/dev timeline `SELECT` (made the new chart render zeros until corrected).
9+
>
10+
> All four shipped together. The spec at `docs/superpowers/specs/2026-05-26-glook-10-prs-first-class-design.md` has been updated to reflect what landed (algorithm, colors, semantics, edge cases). The task list below is preserved as the original plan; treat it as historical.
11+
512
**Goal:** Add a `PRs / Week` time-series chart on the org and engineer pages, mirroring the existing `Commits / Week` chart.
613

714
**Architecture:** Single data-aggregator change (extend `WeeklyBucket.prs`) cascades to both pages, which each get one new `<TimelineChart>` invocation. No new endpoint, no schema change.

docs/superpowers/specs/2026-05-26-glook-10-prs-first-class-design.md

Lines changed: 32 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
# GLOOK-10 — Surface PR count as a first-class metric in graphs
22

3+
> **Scope note (post-implementation):** Initial design called for a single `PRs / Week` chart. During implementation the user requested a companion `Avg Lines Changed / PR` chart (sized to give the "throughput vs size" story), with a P95 outlier filter mirroring the existing Lines Changed chart's smoothing. Both ship together. This doc has been updated below to reflect what landed.
4+
35
## Goal
46

5-
PR count is the most heavily weighted contributor to the IC impact score (`min(PRs/10, 1) × 2.7`), but it's currently invisible in every time-series chart on the org and engineer pages. Add a `PRs / Week` chart on each page, mirroring the existing `Commits / Week` chart, so the metric that drives impact is also visible in the time-series story.
7+
PR count is the most heavily weighted contributor to the IC impact score (`min(PRs/10, 1) × 2.7`), but it's currently invisible in every time-series chart on the org and engineer pages. Add a `PRs / Week` chart **and a companion `Avg Lines Changed / PR (outliers excluded)` chart** on each page, mirroring the existing `Commits / Week` and `Lines Changed / Week` charts respectively, so the metric that drives impact is also visible in the time-series story — both as throughput (count) and as size (avg).
68

79
## Non-goals
810

@@ -44,20 +46,19 @@ Extend the `WeeklyBucket` interface and the `aggregateWeekly()` implementation:
4446
export interface WeeklyBucket {
4547
week: string;
4648
commits: number;
47-
prs: number; // NEW: distinct pr_number values that week
49+
prs: number; // NEW: distinct pr_number values that week
50+
avgLinesPerPr: number; // NEW: avg lines per PR active in week (outliers excluded)
4851
linesAdded: number;
4952
linesRemoved: number;
5053
// ... (rest unchanged)
5154
}
5255
```
5356

54-
Implementation: in the per-week accumulator, add a `prNumbers: Set<string>` alongside the existing `activeDevs: Set<string>`. For each commit row:
57+
**`prs`**in the per-week accumulator, add `prNumbers: Set<string>` alongside the existing `activeDevs: Set<string>`. For each commit row: `if (c.pr_number != null) w.prNumbers.add(String(c.pr_number))`. Emit `prs: w.prNumbers.size`.
5558

56-
```ts
57-
if (c.pr_number != null) w.prNumbers.add(String(c.pr_number));
58-
```
59+
**`avgLinesPerPr`** — first-pass compute `prLineTotals: Map<pr_number, totalLines>` across all weeks. Derive a P95 threshold over distinct PRs using `sortedPrTotals[Math.ceil(N * 0.95) - 1]` (NOT `floor``floor` returns the max at N≤20 and the filter degenerates to a no-op). In the per-week accumulator, track `prNumbersP95: Set<string>` and `prLinesP95: number`; only add a commit's lines to these if its PR's total ≤ threshold. Emit `avgLinesPerPr = prNumbersP95.size > 0 ? round(prLinesP95 / prNumbersP95.size) : 0`.
5960

60-
Emit `prs: w.prNumbers.size` from the final `.map(...)`.
61+
**Semantic of `avgLinesPerPr`** — per-week slice of activity. A PR's *total* across all weeks decides whether it's an outlier (stable filter across the chart), but only the lines from this week's commits go into this week's numerator, and the denominator is the count of PRs active in this week. A 400-line PR split 200/200 across two weeks shows avg=200 in each; a 400-line PR landing in one week shows avg=400 there. This is "average per-PR activity in week W," not "size of the average PR overall."
6162

6263
### Definition
6364

@@ -73,28 +74,27 @@ The `commit_analyses` rows already carry `pr_number` and they're already passed
7374

7475
### Both pages: `org/page.tsx` and `dev/[login]/page.tsx`
7576

76-
1. Add `prs: number` to the local `WeeklyData` interface (mirrors `WeeklyBucket`).
77-
2. Add **one new `<TimelineChart>` invocation** immediately after the existing `Commits / Week` chart:
77+
1. Add `prs: number` and `avgLinesPerPr: number` to the local `WeeklyData` interface (mirrors `WeeklyBucket`).
78+
2. Add **two new `<TimelineChart>` invocations** immediately after the existing `Commits / Week` chart:
7879

7980
```tsx
81+
<TimelineChart data={timeline} valueKey="prs" label="PRs / Week" color="#06B6D4" />
8082
<TimelineChart
8183
data={timeline}
82-
valueKey="prs"
83-
label="PRs / Week"
84-
color="#A78BFA" // purple-400 — distinct from greens (commits) and amber (lines)
84+
valueKey="avgLinesPerPr"
85+
label="Avg Lines Changed / PR (outliers excluded)"
86+
color="#EC4899"
87+
suffix=" lines"
8588
/>
8689
```
8790

88-
3. No `computeValue` or `inFlightValue` props — `prs` is a direct numeric field.
89-
4. Tooltip / scale / height all inherit from the existing `TimelineChart` defaults.
91+
3. No `computeValue` or `inFlightValue` props — both are direct numeric fields.
92+
4. Tooltip / scale / height inherit from `TimelineChart` defaults.
9093

9194
### Color choice
9295

93-
`#A78BFA` (Tailwind purple-400). Reasoning:
94-
- Greens are already taken by commits / lines-added.
95-
- Ambers / reds are lines-removed.
96-
- Blues are sometimes used for AI%.
97-
- Purple sits cleanly in the unused part of the existing palette and won't clash on either page.
96+
- `#06B6D4` (Tailwind cyan-500) for **PRs / Week** — distinct from `#A855F7` (purple-500) used by AI Assisted % on the same grid; earlier purple-400 (`#A78BFA`) was too close to the AI hue.
97+
- `#EC4899` (Tailwind pink-500) for **Avg Lines Changed / PR** — reads as "size, not count," and doesn't collide with amber (used by Avg Complexity on the dev page).
9898

9999
### Placement
100100

@@ -104,21 +104,26 @@ The new chart goes **immediately after** Commits/Week on both pages. Rationale:
104104

105105
| Layer | What | Where |
106106
|---|---|---|
107-
| Unit — aggregator | `aggregateWeekly()` emits `prs` = distinct `pr_number` count per week. Cases: rows with no `pr_number` → 0; rows with duplicate `pr_number` → counted once; rows with multiple distinct PRs → all counted; PR spanning two weeks → counted in each. | New file `src/lib/__tests__/unit/timeline.test.ts` (no existing tests on `timeline.ts`; verified via `ls`). |
108-
| Visual / interactive | None. Repo has no RTL/Playwright harness; the chart is a copy of an existing chart with a different field. Manual smoke. | n/a |
107+
| Unit — `prs` aggregation | Cases: rows with no `pr_number` → 0; duplicates → counted once; multiple distinct PRs; PR spanning two weeks → counted in each; mixed PR + direct-push rows. | `src/lib/__tests__/unit/timeline.test.ts` |
108+
| Unit — `avgLinesPerPr` aggregation | Cases: zero PRs → 0; single-PR-multi-commit sum; avg across distinct PRs; direct-push lines excluded from numerator; integer rounding; **P95 outlier exclusion at N=20 boundary**; **degrades to no-filter at N<20**; **cross-week semantic lockdown** (per-week slice / per-week distinct count). | same file |
109+
| Visual / interactive | None. Repo has no RTL/Playwright harness. Manual smoke. | n/a |
109110

110111
## Edge cases
111112

112113
| Case | Behavior |
113114
|---|---|
114115
| Week with 0 commits and 0 PRs | Renders as empty bar (same as Commits/Week handles the no-commits case today). |
115-
| Commit row has `pr_number = null` (direct push) | Contributes to commits count, contributes 0 to PRs count. Correctly reflects: "this work landed without a PR." |
116-
| Many small PRs in one week | Bar is tall — same visual treatment as a high commit count. |
117-
| PR with 50 commits across one week | Counted as 1 PR — distinct on pr_number. The volume is visible in the Commits/Week chart; this chart specifically tells the story of PR throughput, not commit volume per PR. |
116+
| Commit row has `pr_number = null` (direct push) | Contributes to commits count, contributes 0 to PRs count, contributes 0 to `avgLinesPerPr` numerator. Correctly reflects: "this work landed without a PR." |
117+
| Many small PRs in one week | PRs/Week bar is tall; Avg Lines/PR stays low. |
118+
| PR with 50 commits across one week | Counted as 1 PR. Total of its lines / 1 = the avg. |
119+
| Org-wide week has only in-flight commits (no shipped) | In-flight overlay creates a bucket with `prs: 0, avgLinesPerPr: 0` — both new charts render as zero, no NaN/undefined. |
120+
| < 20 PRs in the lookback window | `avgLinesPerPr` P95 filter degenerates to no-filter (you can't meaningfully exclude top 5% of a tiny population). Chart shows raw average. |
118121

119122
## Files touched
120123

121-
- **Modify** `src/lib/report/timeline.ts``prs` field on `WeeklyBucket`, `prNumbers` set in the accumulator, `prs: w.prNumbers.size` in the final map.
122-
- **Modify** `src/app/report/[id]/org/page.tsx``WeeklyData` interface gains `prs`; one new `<TimelineChart>` invocation after Commits/Week.
124+
- **Modify** `src/lib/report/timeline.ts``prs` + `avgLinesPerPr` on `WeeklyBucket`, accumulator state, PR-level P95 threshold.
125+
- **Modify** `src/lib/report/org.ts` — add `pr_number` to the `commit_analyses` SELECT; zero-default `prs`/`avgLinesPerPr` in the in-flight overlay bucket initializer.
126+
- **Modify** `src/lib/report/dev.ts` — add `pr_number` to the `commit_analyses` SELECT.
127+
- **Modify** `src/app/report/[id]/org/page.tsx``WeeklyData` interface gains both fields; two new `<TimelineChart>` invocations after Commits/Week.
123128
- **Modify** `src/app/report/[id]/dev/[login]/page.tsx` — same two changes.
124-
- **Modify or Create** `src/lib/__tests__/unit/timeline.test.ts` — unit tests for the new `prs` aggregation.
129+
- **Create** `src/lib/__tests__/unit/timeline.test.ts` — unit tests for both aggregations.

src/app/report/[id]/dev/[login]/page.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,13 +321,14 @@ export default function DevDetailPage() {
321321
data={timeline}
322322
valueKey="prs"
323323
label="PRs / Week"
324-
color="#A78BFA"
324+
color="#06B6D4"
325325
/>
326326
<TimelineChart
327327
data={timeline}
328328
valueKey="avgLinesPerPr"
329-
label="Avg Lines Changed / PR"
329+
label="Avg Lines Changed / PR (outliers excluded)"
330330
color="#EC4899"
331+
suffix=" lines"
331332
/>
332333
<TimelineChart
333334
data={timeline}

src/app/report/[id]/org/page.tsx

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,13 +279,14 @@ export default function OrgDetailPage() {
279279
data={timeline}
280280
valueKey="prs"
281281
label="PRs / Week"
282-
color="#A78BFA"
282+
color="#06B6D4"
283283
/>
284284
<TimelineChart
285285
data={timeline}
286286
valueKey="avgLinesPerPr"
287-
label="Avg Lines Changed / PR"
287+
label="Avg Lines Changed / PR (outliers excluded)"
288288
color="#EC4899"
289+
suffix=" lines"
289290
/>
290291
<TimelineChart data={timeline} valueKey="activeDevs" label="Active Developers / Week" color="#10B981" />
291292
<LinesChangedChart data={timeline} />

src/lib/__tests__/unit/timeline.test.ts

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,15 +110,54 @@ describe('aggregateWeekly — avgLinesPerPr field', () => {
110110
expect(out[0].avgLinesPerPr).toBe(10);
111111
});
112112

113-
it('excludes outlier PRs above P95 from the average', () => {
113+
it('excludes outlier PRs above P95 from the average (N=21)', () => {
114114
const rows: any[] = [];
115115
for (let i = 1; i <= 20; i++) {
116116
rows.push(row({ pr_number: i, lines_added: 50, lines_removed: 0 }));
117117
}
118-
// One huge PR — above P95 of the 21-PR population, should be excluded from the avg.
119118
rows.push(row({ pr_number: 999, lines_added: 10000, lines_removed: 0 }));
120119
const out = aggregateWeekly(rows);
121120
expect(out[0].prs).toBe(21);
122121
expect(out[0].avgLinesPerPr).toBe(50);
123122
});
123+
124+
it('excludes outlier PRs at the N=20 boundary (where the old floor() math was a no-op)', () => {
125+
const rows: any[] = [];
126+
for (let i = 1; i <= 19; i++) {
127+
rows.push(row({ pr_number: i, lines_added: 50, lines_removed: 0 }));
128+
}
129+
rows.push(row({ pr_number: 999, lines_added: 10000, lines_removed: 0 }));
130+
const out = aggregateWeekly(rows);
131+
expect(out[0].prs).toBe(20);
132+
expect(out[0].avgLinesPerPr).toBe(50);
133+
});
134+
135+
it('degrades to no-filter for N<20 (cannot meaningfully exclude top 5% of tiny populations)', () => {
136+
// N=10: 9 small + 1 huge. With <20 samples, all are included.
137+
const rows: any[] = [];
138+
for (let i = 1; i <= 9; i++) {
139+
rows.push(row({ pr_number: i, lines_added: 50, lines_removed: 0 }));
140+
}
141+
rows.push(row({ pr_number: 999, lines_added: 9550, lines_removed: 0 }));
142+
const out = aggregateWeekly(rows);
143+
expect(out[0].prs).toBe(10);
144+
expect(out[0].avgLinesPerPr).toBe(1000); // (9*50 + 9550) / 10
145+
});
146+
147+
it('cross-week semantic: a PR spanning two weeks contributes only its per-week slice to each week\'s avg', () => {
148+
// PR 42: 200 lines week A, 200 lines week B (total 400)
149+
// PR 99: 100 lines week A only
150+
// Week A: 2 PRs (42, 99), slice lines = 200 + 100 = 300, avg = 150
151+
// Week B: 1 PR (42), slice lines = 200, avg = 200
152+
const out = aggregateWeekly([
153+
row({ pr_number: 42, committed_at: MON_A, lines_added: 200, lines_removed: 0 }),
154+
row({ pr_number: 99, committed_at: MON_A, lines_added: 100, lines_removed: 0 }),
155+
row({ pr_number: 42, committed_at: MON_B, lines_added: 200, lines_removed: 0 }),
156+
]);
157+
expect(out).toHaveLength(2);
158+
expect(out[0].prs).toBe(2);
159+
expect(out[0].avgLinesPerPr).toBe(150);
160+
expect(out[1].prs).toBe(1);
161+
expect(out[1].avgLinesPerPr).toBe(200);
162+
});
124163
});

src/lib/report/org.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,8 @@ export async function getOrgReport(reportId: string) {
8989
bucket = {
9090
week: weekKey,
9191
commits: 0,
92+
prs: 0,
93+
avgLinesPerPr: 0,
9294
linesAdded: 0,
9395
linesRemoved: 0,
9496
linesP95Added: 0,

src/lib/report/timeline.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ export function aggregateWeekly(commits: any[], opts?: { trackDevs?: boolean }):
5252

5353
// Total lines per PR across all weeks — used to apply a P95 outlier filter to the
5454
// per-week avgLinesPerPr so one giant refactor PR can't dominate the chart.
55+
//
56+
// Threshold math: index = ceil(N * 0.95) - 1. With <= comparison this excludes
57+
// the top ~5% even at small N (floor(N*0.95) returns the max itself at N≤20,
58+
// making the filter a no-op for typical dev pages).
5559
const prLineTotals = new Map<string, number>();
5660
for (const c of commits) {
5761
if (c.pr_number == null || !c.committed_at) continue;
@@ -60,7 +64,7 @@ export function aggregateWeekly(commits: any[], opts?: { trackDevs?: boolean }):
6064
}
6165
const sortedPrTotals = [...prLineTotals.values()].sort((a, b) => a - b);
6266
const prP95Threshold = sortedPrTotals.length > 0
63-
? sortedPrTotals[Math.floor(sortedPrTotals.length * 0.95)]
67+
? sortedPrTotals[Math.ceil(sortedPrTotals.length * 0.95) - 1]
6468
: Infinity;
6569

6670
const weeklyMap = new Map<string, {
@@ -116,7 +120,13 @@ export function aggregateWeekly(commits: any[], opts?: { trackDevs?: boolean }):
116120
if (c.pr_number != null) {
117121
const key = String(c.pr_number);
118122
w.prNumbers.add(key);
119-
// Exclude PRs whose total size exceeds P95 from the avgLinesPerPr signal.
123+
// avgLinesPerPr semantic: per-week slice of activity. A PR's TOTAL across all weeks
124+
// decides whether it's an outlier (so the filter is stable across the chart), but
125+
// only the lines from this week's commits go into this week's numerator, and the
126+
// denominator is the count of PRs active in this week. A 400-line PR split 200/200
127+
// across two weeks shows avg=200 in each; a 400-line PR landing in one week shows
128+
// avg=400 there. This is "average per-PR activity in week W," not "size of the
129+
// average PR overall."
120130
if ((prLineTotals.get(key) ?? 0) <= prP95Threshold) {
121131
w.prNumbersP95.add(key);
122132
w.prLinesP95 += la + lr;

0 commit comments

Comments
 (0)