ReviewOS
Reload

also looking at this

reviewos/reviewos.org

feat(reviews): the suggestions get an interface, priced as designed

#6
Merged glennmichael123 wants to merge feat/suggested-reviewers-panel into main
9 files +741 -10
resources/views/[owner]/[repository]/pull/[number].stxmodified+191-4
@@ -11,6 +11,7 @@
1111 * where forges get slow, and it is slowest exactly when the review matters.
1212 */
1313import { repositoryForView, repositoryPath } from '../../../../functions/repo'
14import { cookieJarFromHeader } from '../../../../functions/identity'
1415import { startsCollapsed } from '../../../../../app/Actions/Pull/manifest'
1516import { approvalsSatisfied, diffTotals, isGenerated, mergeBlockers, parseDiff, pullRequestDiff, requirementsSatisfied, blockedBy, buildStack, orphanMessage, orphanReason, stackSummary, refreshMergeability, renderDiffFile, highlightDiffFile, loadReviewThreads, anchorThreads, threadSlotFor } from '../../../../functions/review'
1617import { renderMarkdownHighlighted } from '../../../../functions/markdown'
@@ -42,7 +43,19 @@ const markdownContext = { owner, repository: repositoryName }
4243 */
4344const serveContext = typeof __stxServeContext === 'undefined' ? undefined : __stxServeContext
4445
45const access = await repositoryForView(owner, repositoryName, serveContext?.cookies)
46/*
47 * Two pipelines serve this page and they disagree about what a request looks
48 * like. The frontend server hands views `__stxServeContext` with the cookies
49 * already parsed; a `route.serve()` boot - production, the e2e suite - hands
50 * them the raw headers instead and no serve context at all. Ask whichever
51 * arrived: a page that only reads the jar renders every reader on the other
52 * pipeline as a stranger, which is how `currentUser` never looking at a
53 * cookie shipped, and this is the same lesson one layer up.
54 */
55const headerBag = typeof headers === 'undefined' ? undefined : headers
56const viewerCookies = serveContext?.cookies ?? cookieJarFromHeader(headerBag?.cookie)
57
58const access = await repositoryForView(owner, repositoryName, viewerCookies)
4659const repoRow: any = access?.repository ?? null
4760
4861const pullRequest = repoRow
@@ -143,14 +156,182 @@ for (const file of files) {
143156 diffHtmlByPath[file.path] = renderDiffFile(file, {
144157 expandable: true,
145158 tokens: await highlightDiffFile(file),
146 // `fold` rather than the streamed viewer's `fetch`: this page runs no
147 // client script, so nothing is going to ask for the rows of a folded file
148 // and a header on its own would be a file nobody can ever read.
159 // `fold` rather than the streamed viewer's `fetch`: the diff on this page
160 // runs no client script, so nothing is going to ask for the rows of a
161 // folded file and a header on its own would be a file nobody can ever
162 // read. The one script the page carries belongs to the suggested-reviewers
163 // panel and touches nothing in the diff.
149164 collapsed: startsCollapsed(file) ? 'fold' : false,
150165 threadsAt: threadSlotFor(fileThreads, file.path),
151166 })
152167}
153168
169const descriptionHtml = pullRequest?.body
170 ? await renderMarkdownHighlighted(String(pullRequest.body), markdownContext)
171 : ''
172
173const protection = repoRow && pullRequest
174 ? await db
175 .selectFrom('protected_branches')
176 .selectAll()
177 .where('repository_id', '=', Number(repoRow.id))
178 .where('pattern', '=', pullRequest.base_branch)
179 .executeTakeFirst()
180 : undefined
181
182let requiredChecks: string[] = []
183try {
184 const parsed = JSON.parse(String(protection?.required_checks ?? '[]'))
185 if (Array.isArray(parsed))
186 requiredChecks = parsed.map(String)
187}
188catch {
189 requiredChecks = []
190}
191
192const checkRows = pullRequest && requiredChecks.length > 0
193 ? await db
194 .selectFrom('check_runs')
195 .select(['name', 'status', 'conclusion', 'started_at'])
196 .where('repository_id', '=', Number(repoRow!.id))
197 .where('head_sha', '=', pullRequest.head_sha)
198 .execute()
199 : []
200
201const checkResult = requirementsSatisfied(
202 checkRows.map((row: any) => ({
203 name: String(row.name),
204 status: row.status,
205 conclusion: row.conclusion,
206 startedAt: Date.parse(String(row.started_at ?? '')) || 0,
207 })),
208 requiredChecks,
209)
210
211const approval = approvalsSatisfied({
212 reviews: reviews.map((review: any) => ({
213 reviewerId: Number(review.reviewer_id),
214 state: String(review.state),
215 commitSha: review.commit_sha as string | null,
216 })),
217 headSha: pullRequest ? (pullRequest.head_sha as string | null) : null,
218 requiredApprovals: Number(protection?.required_approvals ?? 0),
219 dismissStaleReviews: Boolean(protection?.dismiss_stale_reviews),
220})
221
222const unresolvedThreads = threads.filter((thread: any) => !thread.resolved).length
223
224/*
225 * Mergeability, cached against the two commits it was computed from.
226 *
227 * The roadmap wants this computed in the background; it is computed here for
228 * now, and the cache is what makes that acceptable: the answer is recomputed
229 * only when one of the branches has moved, so a hundred people opening this
230 * page cost one merge between them rather than a hundred. Nothing here can move
231 * a ref, because `git merge-tree` merges in memory.
232 */
233const mergeability = pullRequest
234 ? await refreshMergeability(owner, repositoryName, {
235 id: Number(pullRequest.id),
236 base_sha: String(pullRequest.base_sha),
237 head_sha: String(pullRequest.head_sha),
238 mergeable_state: pullRequest.mergeable_state as string | null,
239 mergeable_base_sha: pullRequest.mergeable_base_sha as string | null,
240 mergeable_head_sha: pullRequest.mergeable_head_sha as string | null,
241 mergeable_conflicts: pullRequest.mergeable_conflicts as string | null,
242 })
243 : { state: 'unknown' as const, treeSha: null, conflictingPaths: [], recomputed: false }
244
245const blockers = pullRequest
246 ? mergeBlockers(
247 {
248 state: pullRequest.state as 'open' | 'closed' | 'merged',
249 draft: Boolean(pullRequest.draft),
250 mergeable: mergeability.state === 'clean'
251 ? true
252 : (mergeability.state === 'unknown' ? null : false),
253 stackParent: null,
254 },
255 {
256 requiredApprovals: Number(protection?.required_approvals ?? 0),
257 requireThreadsResolved: Boolean(protection?.require_conversation_resolution),
258 requireLinearHistory: Boolean(protection?.require_linear_history),
259 allowedStrategies: ['merge', 'squash', 'rebase'],
260 requiredChecks,
261 },
262 {
263 approvals: approval.approvals,
264 blockingReviews: approval.blocking,
265 unresolvedThreads,
266 checks: checkResult,
267 },
268 'merge',
269 )
270 : []
271
272/*
273 * The stack this pull request belongs to.
274 *
275 * Loaded for the whole repository in one query rather than by walking parent
276 * links a level at a time: a stack is small, and a round trip per level is the
277 * shape that makes a deep stack slow to open.
278 */
279const stackRows = pullRequest
280 ? await db
281 .selectFrom('pull_requests')
282 .select(['id', 'number', 'title', 'state', 'head_branch', 'base_branch', 'stack_parent_id', 'draft'])
283 .where('repository_id', '=', Number(repoRow.id))
284 .execute()
285 : []
286
287const stackMembers = stackRows.map((row: any) => ({
288 id: Number(row.id),
289 number: Number(row.number),
290 title: String(row.title),
291 state: String(row.state),
292 headBranch: String(row.head_branch),
293 baseBranch: String(row.base_branch),
294 stackParentId: row.stack_parent_id ? Number(row.stack_parent_id) : null,
295 draft: Boolean(row.draft),
296}))
297
298const currentMember = pullRequest ? stackMembers.find((entry: any) => entry.id === Number(pullRequest.id)) : undefined
299const stack = currentMember ? buildStack(stackMembers, currentMember.id) : []
300const stackText = stackSummary(stack)
301const stackOrphan = currentMember ? orphanMessage(orphanReason(currentMember, stackMembers)) : null
302
303/*
304 * Only this pull request's own blockers are known here, so the rest of the
305 * stack is judged on what can be checked cheaply. A member whose readiness is
306 * unknown counts as not ready, which errs toward saying "waiting on" rather
307 * than promising a merge that would then be refused.
308 */
309const stackReadiness = stack.map((entry: any) => ({
310 id: entry.id,
311 blockers: currentMember && entry.id === currentMember.id ? blockers : (entry.state === 'open' && !entry.draft ? [] : ['not ready']),
312}))
313
314const stackBlocker = currentMember ? blockedBy(stack, currentMember.id, stackReadiness) : null
315
316/*
317 * The suggested-reviewers panel is offered, not filled: the endpoint behind it
318 * costs a `git log` over the changed paths, so the page hands the component a
319 * URL and the component asks only when the reader opens it. Built here because
320 * the page knows how a pull request is addressed and the component should not.
321 *
322 * Offered to signed-in readers who may review, on open pull requests only: an
323 * anonymous reader cannot ask anybody for a review, and a suggestion on a
324 * merged or closed pull request is dead weight. The endpoint answers public
325 * repositories more loosely than this gate implies - keying the gate on
326 * `can('pull:review')` keeps the two aligned if that ability's rung ever moves.
327 *
328 * Nothing here can throw: both lines only string-format values already loaded,
329 * and in this file that property is load-bearing - a server script that throws
330 * renders the page as not found.
331 */
332const suggestUrl = `/api/repos/pulls/suggested-reviewers?owner=${encodeURIComponent(owner)}&repo=${encodeURIComponent(repositoryName)}&number=${number}`
333const offerSuggestions = Boolean(pullRequest && pullRequest.state === 'open' && access?.viewer && access.can('pull:review'))
334
154335const title = pullRequest
155336 ? `${pullRequest.title} by ${authorName ?? 'someone'} · ${owner}/${repositoryName} #${number}`
156337 : 'Pull request not found'
@@ -294,6 +475,12 @@ const statePill = pullRequest
294475 @endif
295476 </section>
296477
478 {{-- Who should look, above who has looked. Hidden rather than empty
479 for readers who cannot ask anybody: see the gate in the script. --}}
480 @if (offerSuggestions)
481 <SuggestedReviewers url="/api/repos/pulls/suggested-reviewers?owner=reviewos&repo=reviewos.org&number=6" />
482 @endif
483
297484 <section class="panel side-panel">
298485 <h2 class="side-heading">Reviews</h2>
299486 @if (reviews.length === 0)