feat(roadmap-planner): W9 — drop members.pillar_id + cross-pillar % rewrite#196
Merged
Conversation
alaudabot
reviewed
May 19, 2026
alaudabot
left a comment
alaudabot
left a comment
Contributor
There was a problem hiding this comment.
Code Review — PR #196
Reviewed 7 files (+144/-37). No critical issues found. Two minor observations and one suggestion:
⚠️ dialect parameter on pillarsByAuthor is unused (service_extras.go:199-202)
The function signature includes dialect storage.Dialect, but the body calls rebindSimple(dialect, ...) where rebindSimple is a local helper that takes dialect as its own parameter — meaning pillarsByAuthor never actually uses the dialect argument for any dialect-specific logic. Consider removing it to reduce noise, or add a comment explaining why it's retained (e.g., if future dialect-specific queries are planned).
func (s *Service) pillarsByAuthor(
ctx context.Context,
db *sql.DB,
q storage.MemberWeekQuery,
pm *PillarMap,
) (map[string]map[string]struct{}, error) {
⚠️ Variable shadowing observation (service_extras.go:175)
revPillars := revPillarSet[reviewerID] — the name is correct and clear, but it shares a root with the method parameter pm (which is PillarMap). No action required, just noting for readability.
💡 Suggestion: add trailing newline to migration (0007_drop_members_pillar.sql:28)
The file ends without a newline. Adding one improves POSIX compliance and avoids spurious diffs in some editors.
✅ Positive: solid implementation
- Well-documented migration with clear audit rationale
- Correct index-drop-before-column-drop order for SQLite compatibility
disjointSetefficiently iterates the smaller set first- Good defensive
PillarMap.Configured()guard - Consistent storage layer changes across all three functions
- Proper
defer rows2.Close()for resource cleanup
Contributor
🤖 AI Code Review
SummaryThis PR drops the deprecated Review Statistics
Critical Issues
WarningsNone. Suggestions
Positive Feedback
ℹ️ About this reviewThis review was automatically generated using the
|
danielfbm
force-pushed
the
feat/metrics-w5-jira-key-rename
branch
from
3c96cf0 to
520cc4a
Compare
Contributor
SummaryThis PR implements W9 of the audit follow-up: dropping the unused Review Statistics
Critical Issues
None identified. Warnings
None identified. Suggestions
None identified. Positive Feedback
|
…ewrite Audit follow-up B2 + B5 (2026-05-19). Depends on W5 (this PR is based on feat/metrics-w5-jira-key-rename so the renamed column is available). The `members.pillar_id` column was a single-pillar tag set on the member that the audit found was NULL on all 21 prod rows. The cross-pillar review % was structurally pegged at 0% because it compared `ma.pillar_id <> mr.pillar_id`. Per maintainer Q9 = option (i): drop the column outright and rewrite the metric. Schema: - Migration 0007_drop_members_pillar.sql drops the index first then the column (SQLite refuses DROP COLUMN with a dependent index). API: - storage.Member.PillarID and MemberIdentity.PillarID removed (no alias). - PATCH /api/contributions/members/:id stops accepting `pillar_id`. - UpsertMember / SetMemberIdentity / ListMembers SQL updates. - prefills.go drops the round-tripped PillarID write. - storage round-trip test updates accordingly. NetworkDensity: - Cross-pillar review % rewritten in Go. Each PR's pillar set comes from PillarMap.PillarsForRepo(pr.repo_id). Each reviewer's pillar set is the union of pillars across PRs they authored in the same window (pillarsByAuthor helper). A review is cross-pillar when the two sets are disjoint AND both are non-empty. Empty either side → excluded from the denominator (we cannot answer the question). - Falls back to 0% when no pillar map is configured. Rollback: empty pillar map in config or drop the migration. 🤖 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-w9-drop-pillar-id
branch
from
f275235 to
6147316
Compare
Contributor
Review SummaryCritical Issue Found: The file contains unresolved merge conflict markers (, , ). This will cause YAML parsing failures and break the application on startup. Issues Count: 1 CriticalPlease resolve this conflict before the next deployment. |
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 B2 + B5 — 2026-05-19. Depends on W5 (#191) for the
jira_keycolumn rename, so this PR is based on the W5 branch. Will be retargeted tomainonce W5 lands.Why
members.pillar_idwas a single-pillar tag set on the member directory. The audit found it was NULL on all 21 prod rows, which meant the cross-pillar review % metric was structurally pegged at 0% because it comparedma.pillar_id <> mr.pillar_id.Per maintainer Q9 = option (i): drop the column outright. Cross-pillar % gets redefined to derive both sides from
PillarMapinstead.Diff
0007_drop_members_pillar.sqldrops the index first then the column (SQLite needs the explicit order).Member.PillarIDandMemberIdentity.PillarIDremoved (no alias).UpsertMember/SetMemberIdentity/ListMembersSQL updates in lock-step.PATCH /api/contributions/members/:idno longer acceptspillar_id.NetworkDensitycross-pillar % rewritten in Go. PR pillar set =PillarsForRepo(pr.repo_id). Reviewer pillar set = union across PRs the reviewer authored in[from, to)(newpillarsByAuthorhelper). A review is cross-pillar when the two sets are disjoint AND both are non-empty. Empty side → excluded from the denominator. Falls back to 0% when nopillars:map is configured.prefills.gostops round-trippingPillarIDthroughMemberIdentity. Storage round-trip test updated to comparedisplay_nameinstead.Test plan
go test ./...+go vet ./...— greendisplay_namefor the upsert-vs-update assertion)cross_pillar_review_pctshould land at a non-zero value for a window where reviewers cross pillars.Sequencing
W5 → W9 (this PR). The PR is based on the W5 branch; once W5 merges to
main, this PR rebases tomain.Notes
members.pillar_idis irrecoverable after the migration. If the operator ever needed a single-pillar member tag again, that'd be a fresh schema design.is_bot = 0filter to the cross-pillar query when both PRs merge (currently the filter is dropped; bot reviewers' empty pillar sets exclude them naturally).🤖 Generated with Claude Code