feat(roadmap-planner): W8 — review_latency_p50_hours per (reviewer, week)#195
Merged
Conversation
Contributor
🤖 AI Code Review
SummaryThis PR implements W8 (review_latency_p50_hours) and W2 (bot consolidation) for the roadmap-planner team analytics. It adds tracking of human review latency per reviewer × week, filters out bot reviews from metrics, and consolidates bot activity under a synthetic "bot" member. The implementation is well-structured with comprehensive tests. Review Statistics
Critical Issues
None identified. Warnings
Suggestions
Positive Feedback
ℹ️ About this reviewThis review was automatically generated using the
|
danielfbm
force-pushed
the
feat/metrics-w2-bot-consolidation
branch
from
ee78c5b to
df4d8bf
Compare
alaudabot
reviewed
May 19, 2026
| // review rows ingested before this feature shipped, reassign bot | ||
| // author/reviewer ids to the synthetic `bot` member, and compute | ||
| // first_human_review_at. Idempotent — safe to run on every boot. | ||
| if bots.Size() > 0 { |
Contributor
There was a problem hiding this comment.
Warning (bug/error-handling): Errors from ConsolidateBots are logged as warnings and execution continues. If the backfill fails, the system will run with inconsistent data (is_bot flags not set, first_human_review_at not populated). Consider whether failures should halt startup or implement a retry mechanism.
…eek) Audit follow-up B3 (2026-05-19). Depends on W2 (this PR is based on feat/metrics-w2-bot-consolidation so the `first_human_review_at` column + `is_bot` tagging are available). Aggregator.Rebuild gains a new step that walks pr_reviews ⨝ pull_requests for the rebuild window. For each non-bot review whose submitted_at equals the PR's first_human_review_at (i.e., the row that defined the first-human touch), we compute latency in hours from PR creation. p50 per (reviewer_id, week_of_pr_created_at) goes into member_week_metrics.review_latency_p50_hours via an ON CONFLICT upsert that reuses the existing PK. Two implementation notes: - p50 is computed in Go using the existing percentile() helper — portable across SQLite + Postgres without percentile_cont(). - The week_start INSERT value is formatted as YYYY-MM-DD so the PK matches what the four sibling INSERTs above emit via dialect.WeekStart(); otherwise SQLite stores a separate row keyed on the time.Time driver-format and the latency falls off the team-overview read. Also adds: - scanTimeAny / parseDateString helpers — SQLite returns week_start as TEXT through the strftime expression; Postgres returns time.Time directly. The helper handles both. Tests: - TestAggregatorReviewLatency seeds 3 PRs with latencies 1/4/9h all reviewed by `alice`, plus a bot comment that must be ignored. Verifies p50 = 4.0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
danielfbm
force-pushed
the
feat/metrics-w8-review-latency
branch
from
a237770 to
8c862ac
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit follow-up B3 — 2026-05-19. Depends on W2 (#190) for the
first_human_review_atcolumn +is_bottagging, so this PR is based on the W2 branch. Will be retargeted tomainonce W2 lands.Summary
Aggregator.Rebuildgets a new step that walkspr_reviews ⨝ pull_requestsfor the rebuild window:is_bot = 0) whosesubmitted_atequals the PR'sfirst_human_review_at(i.e., the review that defined the first-human touch), record the hours frompr.created_at.(reviewer_id, week_of_pr.created_at).member_week_metrics.review_latency_p50_hoursvia the existing PK.Implementation notes
YYYY-MM-DDso the PK matches what the four sibling INSERTs inRebuildemit viadialect.WeekStart(). Without this, SQLite stores a separate row keyed on the time.Time driver format and the team-overview read shows two rows for the same(member, week).scanTimeAny/parseDateStringhelpers — SQLite returnsweek_startthrough strftime as TEXT; Postgres returnstime.Timedirectly. The helper handles both.Test plan
go test ./...+go vet ./...— greenTestAggregatorReviewLatency— 3 PRs (latencies 1h / 4h / 9h, all reviewed by alice) + a bot comment that must be ignored → p50 = 4.0member_week_metrics.review_latency_p50_hourspopulates after W2 is enabled on the dev cluster (bot consolidation must run first sofirst_human_review_atis populated; otherwise the new step'sWHERE first_human_review_at IS NOT NULLfilters out everything)Sequencing
W1 → W2 → W8 (this PR). The PR is based on the W2 branch; once W2 merges to
main, this PR rebases tomain.🤖 Generated with Claude Code