Repository navigation
GLOOK-63: date range selector for Team and Engineer pages - #107
Conversation
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
left a comment
There was a problem hiding this comment.
🤖 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_issuesfor canonical-ID dedupe (range.tscanonicalCommitIdsSql/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.
getRangeJiraIssuesJSON.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 dropsrange, so the dev page loads in current mode andgetDevReportthrowsDeveloperNotFoundError. 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 freshURLSearchParamswithoutrange, so selecting a team resets the page to Current report. Copy the currentrangeintoparams. -
src/app/report/[id]/dev/[login]/page.tsx:153: 🟡 The summary endpoint looks the developer up in the current report'sdeveloper_statsonly, 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. aninCurrentReportflag from the API). -
src/lib/github.ts:815: 🔵 Whentotal_count > 1000,owedis capped at 1000, so a truncated result looks complete and the runner and backfill record coverage for it.countReviewedPRshandles this case. Consider returning anunverified/itemsCompleteflag whenexpectedTotal > SEARCH_RESULT_CAPand withholding coverage. Unlikely in practice (more than 1000 merged PRs per dev per slice).
| 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}`} |
There was a problem hiding this comment.
🟡 developerHref drops range too, which hits the same range-only-dev 404 as the DevTable row click. Use withRange(...).
There was a problem hiding this comment.
Fixed in 4315c23: developerHref now uses withRange, and so do the row click and the team drill-down.
|
|
||
| // 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 }); |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
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.
| for (const s of memStats) { | ||
| s.totalReviews = reviewCounts.get(s.githubLogin) || 0; | ||
| s.impactScore = computeImpactScore(s); | ||
| s.impactScore = computeImpactScore({ ...s, periodDays: days }); |
There was a problem hiding this comment.
🟣 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.
There was a problem hiding this comment.
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.
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>
|
I've addressed the review in 4315c23..5ed9bc1. Line-level findings:
Follow-ups, all done:
Tests: 3527 of 3529 pass. The 2 failures are pre-existing and come from local duplicate files. |
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]/teamand/report/[id]/dev/[login], stored in?range=. Current report keeps today's code path unchanged.How range figures are computed
commit_sha, Jira issues once perissue_key, and PRs once per(repo, pr_number). Only completed reports count.pr_activityandpr_activity_coverage, added toschema.sql,db/mysql.tsanddb/sqlite.ts, with no foreign key toreports. Report runs now record merged PRs authored and reviewed. The review search pages at 100.cap × days / 14, so today's 14-day scores don't change./api/report/[id],/dev/[login],/commitsand/jira-issuesaccept?range=. An invalid value returns 400.Backfill
npm run backfill:prs -- --org <org> --months 12is 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:
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)
upsertPrActivityisn't batched.Spec:
docs/superpowers/specs/2026-10-27-glook-63-range-selector-design.mdPlan:
docs/superpowers/plans/2026-10-27-glook-63-range-selector.md🤖 Generated with Claude Code