swadhin / opencodehub public

Complete required-reviewers-per-state policy guidance

swadhinbiswas committed 8 months ago
73b1fe0
1 parent 60d0c1b
commit 73b1fe0
4 files changed
+220 -17
Showing 4 changed files+220-17
doc/feature_audit.mdsrc/pages/[owner]/[repo]/pulls/[number].astrosrc/pages/api/repos/[owner]/[repo]/pulls/[number]/required-reviewers.tstests/integration/pr-required-reviewers-route.test.ts
▼doc/feature_audit.md
+1-1
@@ -50,7 +50,7 @@
50
50
| Stack-level approvals | ✅ | Stack approvals now include end-to-end orchestration: readiness/blocker context + per-PR summary APIs (`stacks/[stackId]/approvals`), recommended reviewer derivation, dry-run reviewer eligibility previews, and PR stack UI actions for previewing eligibility, requesting recommended reviewers, and requesting custom reviewer batches with live blocker/status feedback |
51
51
| Bulk merge (merge stacks) | ✅ | Explicit stack and selected-PR bulk merge APIs are implemented (`stacks/[stackId]/merge`, `pulls/bulk-merge`) with orchestration UX in PR surfaces: stack merge readiness/blocker checks + merge-method controls on PR stack panel, and selected-PR bulk-merge controls on the PR list workflow panel |
52
52
| Custom PR states | ✅ | State definitions and transition APIs are fully wired (`workflow/states`, `pulls/{number}/state`), PR detail UI now provides guided state transitions with inline policy hints and blocker feedback, and new PR creation auto-applies repository default custom state when configured (`pulls/index`) |
53
-| Required reviewers per state | ⚠️ | State reviewer requirements are enforced during transitions, and explicit required-reviewer status introspection is available (`pulls/{number}/required-reviewers`); richer state-specific policy UX remains incomplete |
53
+| Required reviewers per state | ✅ | State reviewer requirements are enforced during transitions, and required-reviewer introspection now supports state-policy preview mode (`pulls/{number}/required-reviewers?stateId=`) including team/user requirement progress; PR state UI surfaces live per-state reviewer policy guidance before applying transitions |
54
54
| PR merge queues | ✅ | `merge-queue.ts` (590+ lines) |
55
55
| Conflict detection before merge | ✅ | `mergeable` field in PR schema |
56
56
| Auto-merge rules | ⚠️ | Auto-merge rules engine plus explicit PR auto-merge control/status APIs now implemented (`pulls/{number}/auto-merge`); richer policy UX and rule introspection remain partial |
57
57
▼src/pages/[owner]/[repo]/pulls/[number].astro
+51-1
@@ -256,6 +256,7 @@ try {
256
256
</Button>
257
257
</div>
258
258
<div id="pr-state-hint" class="text-xs text-muted-foreground"></div>
259
+ <div id="pr-state-reviewers" class="text-xs text-muted-foreground"></div>
259
260
<div id="pr-state-msg" class="text-xs text-muted-foreground"></div>
260
261
</div>
261
262
)}
@@ -731,6 +732,7 @@ try {
731
732
const prStateSelect = document.getElementById("pr-state-select");
732
733
const prStateApply = document.getElementById("pr-state-apply");
733
734
const prStateHint = document.getElementById("pr-state-hint");
735
+ const prStateReviewers = document.getElementById("pr-state-reviewers");
734
736
const prStateMsg = document.getElementById("pr-state-msg");
735
737
let stackApprovalRecommendations = [];
736
738
@@ -794,6 +796,50 @@ try {
794
796
prStateHint.textContent = parts.join(" | ") || "Custom workflow state.";
795
797
}
796
798
799
+ async function refreshPrStateReviewerPreview() {
800
+ if (!prStateSelect || !prStateReviewers) return;
801
+ const value = prStateSelect.value || "";
802
+ if (!value.startsWith("custom:")) {
803
+ prStateReviewers.textContent = "Required reviewer policy preview is shown for custom states.";
804
+ return;
805
+ }
806
+
807
+ const stateId = value.replace("custom:", "");
808
+ prStateReviewers.textContent = "Loading reviewer policy...";
809
+ try {
810
+ const res = await fetch(
811
+ `/api/repos/${repoOwner}/${repoName}/pulls/${prNumber}/required-reviewers?stateId=${encodeURIComponent(stateId)}`,
812
+ );
813
+ const data = await res.json();
814
+ if (!res.ok) {
815
+ throw new Error(data?.error?.message || data?.message || "Failed to load state reviewer policy");
816
+ }
817
+
818
+ const lines = [];
819
+ lines.push(
820
+ `Missing required approvals: ${data?.missingRequired || 0} (${data?.approvedRequired || 0}/${data?.totalRequired || 0})`
821
+ );
822
+
823
+ const userMissing = (data?.reviewers || [])
824
+ .filter((reviewer) => !reviewer.approved)
825
+ .map((reviewer) => reviewer.username || reviewer.userId);
826
+ if (userMissing.length > 0) {
827
+ lines.push(`Pending users: ${userMissing.join(", ")}`);
828
+ }
829
+
830
+ const teamMissing = (data?.teamRequirements || [])
831
+ .filter((team) => team.missingCount > 0)
832
+ .map((team) => `${team.teamName || team.teamId} (${team.approvedCount}/${team.requiredCount})`);
833
+ if (teamMissing.length > 0) {
834
+ lines.push(`Pending teams: ${teamMissing.join(", ")}`);
835
+ }
836
+
837
+ prStateReviewers.textContent = lines.join(" | ") || "No reviewer requirements.";
838
+ } catch (error) {
839
+ prStateReviewers.textContent = error.message || "Failed to load reviewer requirements";
840
+ }
841
+ }
842
+
797
843
function getCurrentPrStateSelection() {
798
844
if (currentCustomStateId) return `custom:${currentCustomStateId}`;
799
845
return `builtin:${currentBuiltinState || "open"}`;
@@ -808,7 +854,11 @@ try {
808
854
prStateSelect.value = currentBuiltinState === "closed" ? "builtin:closed" : "builtin:open";
809
855
}
810
856
updatePrStateHint();
811
- prStateSelect.addEventListener("change", updatePrStateHint);
857
+ refreshPrStateReviewerPreview();
858
+ prStateSelect.addEventListener("change", () => {
859
+ updatePrStateHint();
860
+ refreshPrStateReviewerPreview();
861
+ });
812
862
}
813
863
814
864
prStateApply?.addEventListener("click", async (event) => {
815
865
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/required-reviewers.ts
+116-13
@@ -6,7 +6,7 @@ import { badRequest, notFound, success, unauthorized } from "@/lib/api";
6
6
import { withErrorHandler } from "@/lib/errors";
7
7
import { canReadRepo } from "@/lib/permissions";
8
8
9
-export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
9
+export const GET: APIRoute = withErrorHandler(async ({ params, locals, request }) => {
10
10
const owner = params.owner;
11
11
const repoName = params.repo;
12
12
const number = Number.parseInt(params.number || "", 10);
@@ -40,10 +40,122 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
40
40
eq(schema.pullRequests.repositoryId, repository.id),
41
41
eq(schema.pullRequests.number, number)
42
42
),
43
- columns: { id: true, number: true },
43
+ columns: { id: true, number: true, stateId: true },
44
44
});
45
45
if (!pr) return notFound("Pull request not found");
46
46
47
+ const requestUrl = new URL(request.url);
48
+ const targetStateId = requestUrl.searchParams.get("stateId");
49
+ const targetStateName = requestUrl.searchParams.get("state");
50
+ const allReviews = await db.query.pullRequestReviews.findMany({
51
+ where: eq(schema.pullRequestReviews.pullRequestId, pr.id),
52
+ orderBy: [desc(schema.pullRequestReviews.submittedAt)],
53
+ });
54
+ const latestReviewsByUser = new Map<string, typeof allReviews[number]>();
55
+ for (const review of allReviews) {
56
+ if (!latestReviewsByUser.has(review.reviewerId)) {
57
+ latestReviewsByUser.set(review.reviewerId, review);
58
+ }
59
+ }
60
+
61
+ if (targetStateId || targetStateName) {
62
+ const stateDefinition = await db.query.prStateDefinitions.findFirst({
63
+ where: targetStateId
64
+ ? and(
65
+ eq(schema.prStateDefinitions.repositoryId, repository.id),
66
+ eq(schema.prStateDefinitions.id, targetStateId)
67
+ )
68
+ : and(
69
+ eq(schema.prStateDefinitions.repositoryId, repository.id),
70
+ eq(schema.prStateDefinitions.name, targetStateName!)
71
+ ),
72
+ columns: {
73
+ id: true,
74
+ name: true,
75
+ displayName: true,
76
+ },
77
+ });
78
+ if (!stateDefinition) return notFound("State definition not found");
79
+
80
+ const stateReviewers = await db.query.prStateReviewers.findMany({
81
+ where: eq(schema.prStateReviewers.stateDefinitionId, stateDefinition.id),
82
+ with: {
83
+ user: {
84
+ columns: { id: true, username: true, displayName: true },
85
+ },
86
+ team: {
87
+ columns: { id: true, name: true },
88
+ },
89
+ },
90
+ });
91
+
92
+ const userRules = stateReviewers.filter((reviewer) => reviewer.userId);
93
+ const teamRules = stateReviewers.filter((reviewer) => reviewer.teamId);
94
+
95
+ const reviewers = userRules.map((required) => {
96
+ const latest = latestReviewsByUser.get(required.userId!);
97
+ const reviewState = latest?.state || null;
98
+ const approved = reviewState === "approved";
99
+ return {
100
+ userId: required.userId!,
101
+ username: required.user?.username || null,
102
+ displayName: required.user?.displayName || null,
103
+ approved,
104
+ reviewState,
105
+ reviewedAt: latest?.submittedAt || latest?.createdAt || null,
106
+ source: "state_policy",
107
+ };
108
+ });
109
+
110
+ const teamRequirements = [];
111
+ for (const rule of teamRules) {
112
+ const members = await db.query.teamMembers.findMany({
113
+ where: eq(schema.teamMembers.teamId, rule.teamId!),
114
+ columns: { userId: true },
115
+ });
116
+ const requiredCount = Math.max(1, rule.requiredCount ?? 1);
117
+ const approvedCount = members.filter((member) => {
118
+ const latest = latestReviewsByUser.get(member.userId);
119
+ return latest?.state === "approved";
120
+ }).length;
121
+ const missingCount = Math.max(requiredCount - approvedCount, 0);
122
+
123
+ teamRequirements.push({
124
+ teamId: rule.teamId!,
125
+ teamName: rule.team?.name || null,
126
+ requiredCount,
127
+ approvedCount,
128
+ missingCount,
129
+ });
130
+ }
131
+
132
+ const teamRequiredTotal = teamRequirements.reduce((total, rule) => total + rule.requiredCount, 0);
133
+ const teamApprovedTotal = teamRequirements.reduce(
134
+ (total, rule) => total + Math.min(rule.requiredCount, rule.approvedCount),
135
+ 0
136
+ );
137
+ const approvedUsers = reviewers.filter((reviewer) => reviewer.approved).length;
138
+ const totalRequired = reviewers.length + teamRequiredTotal;
139
+ const approvedRequired = approvedUsers + teamApprovedTotal;
140
+ const missingRequired = Math.max(totalRequired - approvedRequired, 0);
141
+
142
+ return success({
143
+ pullRequestId: pr.id,
144
+ pullRequestNumber: pr.number,
145
+ targetState: {
146
+ id: stateDefinition.id,
147
+ name: stateDefinition.name,
148
+ displayName: stateDefinition.displayName,
149
+ },
150
+ totalRequired,
151
+ approvedRequired,
152
+ missingRequired,
153
+ reviewers,
154
+ teamRequirements,
155
+ policySource: "state",
156
+ });
157
+ }
158
+
47
159
const requiredReviewers = await db.query.pullRequestReviewers.findMany({
48
160
where: and(
49
161
eq(schema.pullRequestReviewers.pullRequestId, pr.id),
@@ -56,17 +168,6 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
56
168
},
57
169
});
58
170
59
- const allReviews = await db.query.pullRequestReviews.findMany({
60
- where: eq(schema.pullRequestReviews.pullRequestId, pr.id),
61
- orderBy: [desc(schema.pullRequestReviews.submittedAt)],
62
- });
63
- const latestReviewsByUser = new Map<string, typeof allReviews[number]>();
64
- for (const review of allReviews) {
65
- if (!latestReviewsByUser.has(review.reviewerId)) {
66
- latestReviewsByUser.set(review.reviewerId, review);
67
- }
68
- }
69
-
70
171
const reviewers = requiredReviewers.map((required) => {
71
172
const latest = latestReviewsByUser.get(required.userId);
72
173
const reviewState = latest?.state || null;
@@ -88,5 +189,7 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
88
189
approvedRequired: reviewers.filter((reviewer) => reviewer.approved).length,
89
190
missingRequired: reviewers.filter((reviewer) => !reviewer.approved).length,
90
191
reviewers,
192
+ teamRequirements: [],
193
+ policySource: "assigned",
91
194
});
92
195
});
93
196
▼tests/integration/pr-required-reviewers-route.test.ts
+52-2
@@ -8,9 +8,12 @@ const {
8
8
fakeSchema: {
9
9
users: { username: {} },
10
10
repositories: { ownerId: {}, name: {}, id: {} },
11
- pullRequests: { repositoryId: {}, number: {}, id: {} },
11
+ pullRequests: { repositoryId: {}, number: {}, id: {}, stateId: {} },
12
12
pullRequestReviewers: { pullRequestId: {}, isRequired: {} },
13
13
pullRequestReviews: { pullRequestId: {}, submittedAt: {} },
14
+ prStateDefinitions: { repositoryId: {}, id: {}, name: {} },
15
+ prStateReviewers: { stateDefinitionId: {}, userId: {}, teamId: {} },
16
+ teamMembers: { teamId: {}, userId: {} },
14
17
} as any,
15
18
}));
16
19
@@ -37,7 +40,7 @@ function makeDb() {
37
40
findFirst: vi.fn(async () => ({ id: "repo-1", ownerId: "owner-1", name: "demo" })),
38
41
},
39
42
pullRequests: {
40
- findFirst: vi.fn(async () => ({ id: "pr-1", number: 12 })),
43
+ findFirst: vi.fn(async () => ({ id: "pr-1", number: 12, stateId: null })),
41
44
},
42
45
pullRequestReviewers: {
43
46
findMany: vi.fn(async () => ([
@@ -51,6 +54,34 @@ function makeDb() {
51
54
{ reviewerId: "u2", state: "changes_requested", submittedAt: new Date("2026-02-18T00:00:00Z"), createdAt: new Date("2026-02-18T00:00:00Z") },
52
55
])),
53
56
},
57
+ prStateDefinitions: {
58
+ findFirst: vi.fn(async () => ({ id: "state-1", name: "in_review", displayName: "In Review" })),
59
+ },
60
+ prStateReviewers: {
61
+ findMany: vi.fn(async () => ([
62
+ {
63
+ userId: "u1",
64
+ teamId: null,
65
+ requiredCount: 1,
66
+ user: { id: "u1", username: "alice", displayName: "Alice" },
67
+ team: null,
68
+ },
69
+ {
70
+ userId: null,
71
+ teamId: "team-1",
72
+ requiredCount: 2,
73
+ user: null,
74
+ team: { id: "team-1", name: "Platform" },
75
+ },
76
+ ])),
77
+ },
78
+ teamMembers: {
79
+ findMany: vi.fn(async () => [
80
+ { userId: "u1" },
81
+ { userId: "u3" },
82
+ { userId: "u4" },
83
+ ]),
84
+ },
54
85
},
55
86
};
56
87
}
@@ -69,6 +100,7 @@ describe("required reviewers route", () => {
69
100
const response = await requiredReviewersGet({
70
101
params: { owner: "owner-1", repo: "demo", number: "12" },
71
102
locals: {},
103
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/12/required-reviewers"),
72
104
} as any);
73
105
74
106
expect(response.status).toBe(401);
@@ -78,6 +110,7 @@ describe("required reviewers route", () => {
78
110
const response = await requiredReviewersGet({
79
111
params: { owner: "owner-1", repo: "demo", number: "12" },
80
112
locals: { user: { id: "user-1", isAdmin: false } },
113
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/12/required-reviewers"),
81
114
} as any);
82
115
83
116
const body = await readJson(response);
@@ -86,5 +119,22 @@ describe("required reviewers route", () => {
86
119
expect(body?.data?.approvedRequired).toBe(1);
87
120
expect(body?.data?.missingRequired).toBe(1);
88
121
expect(body?.data?.reviewers?.[0]?.username).toBe("alice");
122
+ expect(body?.data?.policySource).toBe("assigned");
123
+ });
124
+
125
+ it("returns state-policy reviewer requirements when stateId is provided", async () => {
126
+ const response = await requiredReviewersGet({
127
+ params: { owner: "owner-1", repo: "demo", number: "12" },
128
+ locals: { user: { id: "user-1", isAdmin: false } },
129
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/12/required-reviewers?stateId=state-1"),
130
+ } as any);
131
+
132
+ const body = await readJson(response);
133
+ expect(response.status).toBe(200);
134
+ expect(body?.data?.policySource).toBe("state");
135
+ expect(body?.data?.targetState?.id).toBe("state-1");
136
+ expect(body?.data?.reviewers?.[0]?.username).toBe("alice");
137
+ expect(body?.data?.teamRequirements?.[0]?.teamName).toBe("Platform");
138
+ expect(body?.data?.missingRequired).toBeGreaterThanOrEqual(1);
89
139
});
90
140
});
91
141