Skip to content

GLOOK-63: date range selector for Team and Engineer pages - #107

Merged
msogin merged 23 commits into
mainfrom
feat/glook-63-range-selector
Oct 28, 2026
Merged

msogin merged 23 commits into
mainfrom
feat/glook-63-range-selector

Conversation

@msogin

@msogin msogin commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

GLOOK-63: date range for the Team and Engineer pages

Adds a range picker (Current report, Past 90 days, Past 12 months) to /report/[id]/team and /report/[id]/dev/[login], stored in ?range=. Current report keeps today's code path unchanged.

How range figures are computed

  • Figures are recomputed from canonical rows, never summed across overlapping daily reports. Commits are counted once per commit_sha, Jira issues once per issue_key, and PRs once per (repo, pr_number). Only completed reports count.
  • New tables pr_activity and pr_activity_coverage, added to schema.sql, db/mysql.ts and db/sqlite.ts, with no foreign key to reports. Report runs now record merged PRs authored and reviewed. The review search pages at 100.
  • The impact score scales with the period: each volume cap becomes cap × days / 14, so today's 14-day scores don't change.
    • In a range, volume caps use measured days and PR/review caps use PR-covered days, so missing history never reads as low activity.
    • A developer with no PR coverage has the PR and review terms dropped and the rest rescaled.
  • The page notes measured days ("N of M days measured") and the PR coverage start.
  • Team Pulse, LLM summaries and Claude Code spend stay on the current report and are labelled that way.
  • API: /api/report/[id], /dev/[login], /commits and /jira-issues accept ?range=. An invalid value returns 400.

Backfill

npm run backfill:prs -- --org <org> --months 12 is a resumable, one-off script that runs month by month. It never runs automatically.

Verified locally (podman, real data)

  • Backfill: 1032 of 1044 slices collected. All 12 failures were for one GitHub account that can't be searched.

  • Most recent report:

    Range Developers PRs Reviews Median impact Max impact
    90d 80 2,080 3,117 4.8 7.5
    12m 87 8,677 14,139 4.0 7.3
  • No score saturates.

Tests

3511 pass. The 2 failures predate this branch and come from untracked duplicate " 2" files in the local tree.

Follow-ups (not in this PR)

  • The Engineer page's Jira list can briefly show the previous range's data during a range switch.
  • The Team Pulse label follows the selected range, not the loaded data.
  • upsertPrActivity isn't batched.

Spec: docs/superpowers/specs/2026-10-27-glook-63-range-selector-design.md
Plan: docs/superpowers/plans/2026-10-27-glook-63-range-selector.md

🤖 Generated with Claude Code

msogin and others added 16 commits October 27, 2026 11:06
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…indows

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…riven Team labels, mock merged PRs)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@msogin msogin left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🤖 Claude review (review-loop: Backend Correctness + Full-stack Gaps personas, plus Smartling fullstack review)

Verdict: With fixes. Range query SQL, schema (MySQL/SQLite parity, translateSQL conflict keys), param validation (400 on unknown range), and tooltip cache keys look solid. The blockers are range propagation in navigation and the range-only-developer paths. One scoring-semantics question needs the author's answer.

Inline: 1 🔴, 4 🟡, 1 🟣, 1 🔵.

Non-blocking follow-ups:

  • Perf: each range request does a GROUP BY over all org commit_analyses/jira_issues for canonical-ID dedupe (range.ts canonicalCommitIdsSql / canonicalJiraIdsSql), repeated by the team, dev, commits and jira endpoints. Cost scales with history, not the window. Restrict it to the overlapping report ids, or cache per (org, window).
  • pr-activity/store.ts: one INSERT per PR, run serially and outside a transaction. Batch it, or wrap it in a transaction for backfill-sized loads.
  • Backfill picks developers only from reports with created_at >= first slice. Developers seen only in older reports get no coverage, so they show "No PR data" (not undercounted).
  • Jira dedupe keeps the latest copy per issue_key, so reassigned or reopened issues credit the latest assignee. Fine, but worth documenting.
  • Zero-overlap range shows an empty table with "0 of N days measured". An explicit empty state and a test would help.
  • getRangeJiraIssues JSON.parse(r.labels): one bad row 500s the endpoint. Consider try/catch → [].
  • Tests: no page-level coverage for range-preserving navigation, range-only developer summary, or loading/error during a range switch.

Additional line-level findings (lines outside the diff hunks):

  • src/app/report/[id]/team/dev-table.tsx:194: 🔴 Range dropped on row click → 404 for range-only developers. With 90d/12m selected the Team table lists developers who aren't in the current report (dev.ts has a path specifically for them). This link drops range, so the dev page loads in current mode and getDevReport throws DeveloperNotFoundError. For everyone else the range is silently reset.
onClick={() => router.push(withRange(`/report/${reportId}/dev/${dev.github_login}`, range))}

Please add a test asserting ?range= survives the click.

  • src/app/report/[id]/team/team-table.tsx:94: 🟡 The team drill-down builds fresh URLSearchParams without range, so selecting a team resets the page to Current report. Copy the current range into params.

  • src/app/report/[id]/dev/[login]/page.tsx:153: 🟡 The summary endpoint looks the developer up in the current report's developer_stats only, so for a range-only developer (now a 200 from /dev/[login]?range=) the Performance Summary card always errors. Skip the fetch, or show a "not in current report" state when the dev has no current-report row (e.g. an inCurrentReport flag from the API).

  • src/lib/github.ts:815: 🔵 When total_count > 1000, owed is capped at 1000, so a truncated result looks complete and the runner and backfill record coverage for it. countReviewedPRs handles this case. Consider returning an unverified/itemsComplete flag when expectedTotal > SEARCH_RESULT_CAP and withholding coverage. Unlikely in practice (more than 1000 merged PRs per dev per slice).

Comment thread src/app/report/[id]/team/page.tsx Outdated
title="Current Projects"
subtitle={`${selectedTeamName} · ${activeReport!.period_days}d`}
subtitle={`${selectedTeamName} · ${activeReport!.period_days}d${payloadRange !== 'current' ? ' · Current report' : ''}`}
developerHref={(login) => `/report/${params.id}/dev/${login}`}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 developerHref drops range too, which hits the same range-only-dev 404 as the DevTable row click. Use withRange(...).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4315c23: developerHref now uses withRange, and so do the row click and the team drill-down.

Comment thread src/app/report/[id]/team/page.tsx Outdated

// Main report data. keepPreviousData stops the tables blanking while a new range loads.
// Developer rows are passed through untouched so pr_covered_days reaches aggregateTeams.
const { data: reportData, isLoading } = useSWR(withRange(`/api/report/${params.id}`, range), { keepPreviousData: true });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 keepPreviousData: true with no pending or error UI: after a range switch, the picker shows the new range over the old range's data until the fetch resolves, and indefinitely if it fails (error isn't read). Show a dimmed or spinner state while range !== payloadRange, plus an error banner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 15c00e9. The new RangeLoadStatus shows "Loading…" and dims the range-driven content while the selected range differs from the data on screen. An SWR error shows a banner ("Couldn't load X. Showing Y.") with Retry. The Engineer page gets the same treatment.

Comment thread src/lib/report-runner.ts
for (const s of memStats) {
s.totalReviews = reviewCounts.get(s.githubLogin) || 0;
s.impactScore = computeImpactScore(s);
s.impactScore = computeImpactScore({ ...s, periodDays: days });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟣 Scoring change, please confirm intent. Passing periodDays: days here (and at L694) rescales the impact caps for every new report that isn't 14 days long. Older reports keep the 14-day-cap scores, so scores stop being comparable across reports. The Team page's current mode also now feeds period_days into aggregateTeams, so team scores on old 30d reports change while developer scores don't. If this is intended, please call it out in the release notes. If not, keep 14 for the current path and scale only for ranges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is intended: the user decided on 2026-10-27 that caps scale with period_days for every report. 14-day reports don't change. Older reports longer than 14 days keep their 14-day-cap developer scores, while their team scores use the scaled caps. That's documented in the CLAUDE.md GLOOK-63 note (5ed9bc1), and I'll include it in the release notes.

msogin and others added 7 commits October 27, 2026 17:36
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…or range-only developers

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rage

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…a labels

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nsaction

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…g the period

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@msogin

msogin commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

I've addressed the review in 4315c23..5ed9bc1.

Line-level findings:

  • 🔴 Range dropped on the dev-table row click (4315c23): the click now uses withRange, and a jsdom test checks that ?range=90d survives it.
  • 🟡 Team drill-down resets the range (4315c23): fixed. The drill-down copies the current range and adds nothing for current.
  • 🟡 Summary card errors for a range-only developer (15c00e9): getDevReport now returns inCurrentReport. When it is false, the page skips the summary fetch and shows "Not in the current report — no summary."
  • 🔵 Merged-PR search truncated at the 1000 cap (9e633bf): when total_count is above 1000, the search now returns itemsComplete: false. The runner withholds coverage, and the backfill records the slice as failed.

Follow-ups, all done:

  • Perf (a2b80e2): commit and Jira dedupe now runs only over completed reports that overlap the window, with a test. Side effect: a copy from a report outside the window can no longer win.
  • Batched upserts (2579fd0): upsertPrActivity writes multi-row INSERTs of 200 inside one transaction.
  • Backfill developer selection (b0596d9): the backfill now picks developers from completed reports whose window overlaps the period.
  • Jira dedupe keeps the latest copy (5ed9bc1): documented in CLAUDE.md.
  • Zero-overlap range (15c00e9): explicit "No reports in this range." state, with a test.
  • Bad labels row (a2b80e2): it now reads as [] instead of returning a 500.
  • Page-level tests: added for range-preserving navigation, the range-only developer summary, and loading and error during a range switch.

Tests: 3527 of 3529 pass. The 2 failures are pre-existing and come from local duplicate files.

@msogin
msogin merged commit cbf3311 into main Oct 28, 2026
1 check passed
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.

1 participant