feat(org): replace Active Devs chart with Avg Impact Score / Week (GLOOK-21)

[msogin]

Summary

• Replaces the Active Developers / Week timeline chart on the org summary page with Avg Impact Score / Week
• Impact data is sourced from completed reports: AVG(impact_score) across all developers per report, bucketed by week using the existing weekKeyForDate helper so week keys align with the commit timeline
• Multiple reports completing in the same week are averaged together
• Only merges avgImpact into existing timeline buckets — no phantom bars for report-only weeks with no commit activity

Changes

• src/lib/report/timeline.ts — adds avgImpact?: number to WeeklyBucket; removes now-unused trackDevs option call
• src/lib/report/org.ts — new SQL query joining reports + developer_stats to compute avg impact per report; JS post-processing buckets by week and merges into timeline
• src/app/report/[id]/org/page.tsx — swaps valueKey="activeDevs" → valueKey="avgImpact" with decimals={1}; adds ?? 0 coercion in TimelineChart values map to prevent crash on weeks with no completed report
• Tests — replaces trackDevs test with 3 new assertions; updates org-unmerged-summary.test.ts mock sequence for new DB call

Test plan

• All 736 unit tests pass (npm test)
• Org summary page shows "Avg Impact Score / Week" chart with decimal values
• Other 5 timeline charts unaffected
• Works correctly with both SQLite and MySQL

🤖 Generated with Claude Code

[msogin]

Smartling Code Review

Issues

🟡 warning — src/lib/report/org.ts — silent data drop on weeks with no commit activity

Avg impact rows are merged only into already-existing timeline buckets. If a report completed in a week with zero commit activity, its avgImpact is silently dropped. The spec documents this as intentional, but there is no code comment at the merge site explaining why. Future maintainers could mistake this for a bug. Suggest adding a comment at the merge loop.

🟡 warning — src/lib/report/org.ts — NULL avg_impact coerced to 0, inflates sample

When a report has no developer_stats rows, AVG(ds.impact_score) returns NULL. Number(row.avg_impact) || 0 coerces NULL → 0 and includes it in the weekly average as a zero-impact data point, deflating avgImpact for that week. The spec documents this edge case but describes it as "coerce to 0" without acknowledging the deflation effect. Defensible, but consider if (!Number.isFinite(val)) continue to skip zero-developer reports entirely.

🟡 warning — src/lib/report/timeline.ts — activeDevs?: number is dead code in the interface

No call site passes trackDevs: true to aggregateWeekly anymore, so activeDevs will always be undefined at runtime. It remains in the interface and creates confusion for consumers. Should be removed from WeeklyBucket while the context is fresh.

🔵 suggestion — src/app/report/[id]/org/page.tsx — ?? 0 applied universally

The ?? 0 fallback is needed for avgImpact (optional) but is now applied to all TimelineChart value mappings. Safe for existing fields that are never undefined, but silently masks future regressions. Acceptable given the codebase style.

🔵 suggestion — src/lib/__tests__/unit/report-org.test.ts — date comment inconsistency with plan doc

Test comment says "week of 2026-01-12 (Mon)" (correct). The plan doc has a typo saying "2026-01-13 (Mon)". Minor, low priority.

🟣 question — src/lib/report/org.ts — unweighted mean of developer impact scores

AVG(ds.impact_score) computes an unweighted mean across all developer rows for a report. With 1 high-impact dev and 9 low-impact devs, the result is pulled toward the low end regardless of contribution volume. Was a commit-weighted average or median considered, or is the simple unweighted mean intentional?

Recommendations

• Query placement (after the in-flight overlay sort) is correct and consistent with the spec.
• No API schema change — avgImpact is additive/optional, existing consumers unaffected.
• Test coverage is good: single-report, multi-report-same-week, and activeDevs removal cases all covered. org-unmerged-summary.test.ts correctly adds the new mock slot at position 5.

Assessment

Ready to merge? Yes
The implementation is correct, well-tested, and consistent with the documented spec. The warnings are minor: silent drop on empty weeks is intentional and documented, and the dead activeDevs field is cosmetic. No blocking issues.

[msogin]

Review Loop — 2 Personas (1 loop)

Reviewers: Sr. Backend/Data Engineer + Correctness | Sr. Frontend Engineer + TypeScript Safety
Intent: Safe to merge

Loop 1 Triage

#	Reviewer	Issue	Severity	Triage
1	Backend	INNER JOIN silently excludes completed reports with zero developer_stats rows — behavior correct, but comment at merge site is slightly misleading	Low	Defer
2	Backend	avgImpact dropped for in-flight-only weeks — timelineByWeek built before overlay loop adds new buckets	Low	Defer — matches spec intent
3	Backend	Number(row.avg_impact) || 0 coercion treats genuine 0.0 avg same as missing — but sum += 0 still increments count correctly	Low	Skip — not a real issue
4	Backend	ORDER BY r.completed_at ASC in SQL is cosmetic — JS Map bucketing is order-independent	Info	Skip — harmless
5	Backend	Plan doc comment says "2026-01-13 (Mon)" for Jan 15; test file correctly says "2026-01-12 (Mon)" — typo in doc only, not in code	Info	Skip — doc only
6	Frontend	?? 0 added to values computation correctly prevents NaN propagation when avgImpact is undefined	Positive	Safe
7	Frontend	When most recent week has no completed report (avgImpact → 0), trend delta shows negative delta vs. prior week	Low	Defer — pre-existing behavior for all optional metrics
8	Frontend	(d as any)[valueKey] cast is a pre-existing pattern shared by all other charts on this page — not a regression	Info	Skip
9	Frontend	activeDevs: number (required) → avgImpact?: number (optional) type change is clean; no other references to activeDevs remain in scope	Positive	Safe

Loop 1 Summary

#	Issue	Severity	Action
1	INNER JOIN excludes zero-dev completed reports — behavior correct, comment slightly misleading	Low	Defer
2	avgImpact silently dropped for in-flight-only weeks — matches spec	Low	Defer
3	Number() || 0 masks genuine 0.0 avg — math still correct	Low	Skip
4	SQL ORDER BY is cosmetic	Info	Skip
5	Plan doc comment typo (not in test or source)	Info	Skip
6	?? 0 prevents NaN in chart values	Positive	Safe
7	Zero-avgImpact latest week shows negative trend delta	Low	Defer
8	(d as any)[valueKey] pre-existing pattern	Info	Skip
9	Type change activeDevs → avgImpact? is clean	Positive	Safe

Ask items for author: None.

Backend Notes

• SQL correctness: Valid SQLite and MySQL. GROUP BY r.id, r.completed_at is correct. WHERE r.org = ? AND r.status = 'completed' properly scopes — no cross-org data leak.
• Week-key alignment: Both aggregateWeekly and the new impact bucketing use weekKeyForDate(new Date(timestamp)) → UTC .toISOString(). Keys are guaranteed to align.
• Merge logic: timelineByWeek holds references to original bucket objects, so bucket.avgImpact = sum / count mutates in place correctly. The if (bucket) guard prevents phantom buckets for report-only weeks.
• Test mock sequences verified: org-unmerged-summary.test.ts correctly inserts a new slot 5 ([[], null]) for the avgImpact query. The "does not set activeDevs" test correctly relies on the beforeEach default for calls 5+.

Frontend Notes

• ?? 0 fix: Without it, undefined from weeks with no completed report produces NaN in Math.max(...values), breaking bar-height computation. Correct and necessary.
• decimals={1} prop: TimelineChart handles this correctly via v.toFixed(decimals) in both tooltip and y-axis.
• Zero-data rendering: max = Math.max(...values, 1) guarantees max ≥ 1 — no divide-by-zero. Weeks with no completed report render as flat zero-height bars.

---

Verdict: ✅ Safe to merge

No blocking issues. SQL is correctly scoped, bucketing logic is sound, week-key alignment is verified, test mock sequences are accurate, and the ?? 0 fix is a genuine improvement. Both deferred edge cases align with documented spec intent.

[msogin]

Re: unweighted mean — yes, intentional. The unweighted mean is the right default here: each developer in the report is an independent contributor, and a 1-high-impact + 9-low-impact team is genuinely a low-avg-impact team. Commit-weighted would bias toward prolific committers (who may just have more WIP/refactor churn) and median would lose week-to-week resolution on small teams. The simple mean is the clearest signal for a weekly trend chart.