also looking at this
feat(reviews): the suggestions get an interface, priced as designed
#6
9 files
+741
-10
| @@ -193,17 +193,36 @@ async function cookieUserId(request: any): Promise<number> { | ||
| 193 | 193 | if (!header) |
| 194 | 194 | return 0 |
| 195 | 195 | |
| 196 | const found = await viewerFromCookies(cookieJarFromHeader(header)) | |
| 197 | return found?.id ?? 0 | |
| 198 | } | |
| 199 | ||
| 200 | /** | |
| 201 | * A `Cookie` header, parsed into the record `viewerFromCookies` reads. | |
| 202 | * | |
| 203 | * Exported for the pages served without a parsed cookie jar: the frontend | |
| 204 | * server hands views `__stxServeContext.cookies` ready-made, but a page | |
| 205 | * rendered through `route.serve()`'s file routing gets the raw headers and | |
| 206 | * nothing else, and a view that only asks for the jar renders every reader | |
| 207 | * there as a stranger. | |
| 208 | */ | |
| 209 | export function cookieJarFromHeader(header: unknown): Record<string, string> { | |
| 196 | 210 | const jar: Record<string, string> = {} |
| 197 | for (const part of header.split(';')) { | |
| 211 | ||
| 212 | for (const part of String(header ?? '').split(';')) { | |
| 198 | 213 | const cut = part.indexOf('=') |
| 199 | 214 | if (cut < 0) |
| 200 | 215 | continue |
| 201 | 216 | |
| 202 | jar[part.slice(0, cut).trim()] = decodeURIComponent(part.slice(cut + 1).trim()) | |
| 217 | try { | |
| 218 | jar[part.slice(0, cut).trim()] = decodeURIComponent(part.slice(cut + 1).trim()) | |
| 219 | } | |
| 220 | catch { | |
| 221 | // A malformed escape in one cookie is not a reason to drop the others. | |
| 222 | } | |
| 203 | 223 | } |
| 204 | 224 | |
| 205 | const found = await viewerFromCookies(jar) | |
| 206 | return found?.id ?? 0 | |
| 225 | return jar | |
| 207 | 226 | } |
| 208 | 227 | |
| 209 | 228 | /** |
| @@ -20,5 +20,8 @@ export type { RouteDefinition, RouteRegistry } from '@stacksjs/router' | ||
| 20 | 20 | export default { |
| 21 | 21 | api: 'api', |
| 22 | 22 | attachments: { path: 'attachments', prefix: '' }, |
| 23 | // Registers nothing; configures the renderer. See the file for why it must | |
| 24 | // load with the routes rather than in any one server's boot script. | |
| 25 | views: { path: 'views', prefix: '' }, | |
| 23 | 26 | git: { path: 'git', prefix: '' }, |
| 24 | 27 | } satisfies RouteRegistry |
| @@ -374,12 +374,37 @@ this is where the claim is either true or marketing. | ||
| 374 | 374 | History is read on the **base**. The head's recent commits are the ones being reviewed, and |
| 375 | 375 | counting them would suggest the author of the change as the reviewer of it. |
| 376 | 376 | |
| 377 | - [ ] Show the suggestions somewhere. The endpoint exists and nothing calls it, which is a feature | |
| 377 | - [x] Show the suggestions somewhere. The endpoint exists and nothing calls it, which is a feature | |
| 378 | 378 | with no interface. It is an endpoint rather than something the conversation page computes |
| 379 | 379 | inline on purpose: it costs a `git log` over the changed paths, and paying that on every render |
| 380 | 380 | of every pull request page to fill a panel most readers will not use is the wrong default. |
| 381 | 381 | Fetched when the reviewer list is opened is the shape it wants. |
| 382 | 382 | |
| 383 | A `<details>` panel in the conversation page's sidebar (`SuggestedReviewers.stx`), above the | |
| 384 | Reviews panel: who should look, above who has looked. The `<details>` opens and closes with no | |
| 385 | script; the one script it carries notices the first open - and only the first - and fetches, so | |
| 386 | the `git log` is spent when a reader asks and never at render. Each name carries its reason | |
| 387 | verbatim, and nothing submits anything: a suggestion, never a request, exactly as the item above | |
| 388 | says. Offered to signed-in readers with `pull:review` on open pull requests - an anonymous reader | |
| 389 | cannot ask anybody, and a suggestion on a merged pull request is dead weight. The endpoint answers | |
| 390 | anonymous readers of public repositories more loosely than that gate implies, which is worth a | |
| 391 | look of its own someday. | |
| 392 | ||
| 393 | `tests/e2e/suggested-reviewers-panel.test.ts` pins the surface the in-process test cannot: the | |
| 394 | endpoint answering a fetch whose only credential is a cookie, and the rendered page carrying the | |
| 395 | panel and its URL but none of the answer - the cost deferred, asserted in markup. Getting it to | |
| 396 | pass surfaced that a page rendered through `route.serve()`'s file routing has no | |
| 397 | `__stxServeContext`, only raw headers, so every view read its readers as strangers there: | |
| 398 | `cookieJarFromHeader` is the shared parse, this page asks whichever pipeline answered, and the | |
| 399 | same fallback is owed to every other view that reads `serveContext?.cookies`. | |
| 400 | ||
| 401 | And one to know before writing the next client script: stx's client bridge seeds any identifier a | |
| 402 | client script shares with the server scope into the page as a `var`. This page's server scope has | |
| 403 | a `headers` binding holding the request headers, so the panel's first draft - an innocent | |
| 404 | `fetch(url, { headers: { Accept: ... } })` - serialized the reader's session cookie into the HTML. | |
| 405 | The word `headers` in a *client* script is enough. The test now asserts the token is not in the | |
| 406 | page, whatever the next mechanism would be. | |
| 407 | ||
| 383 | 408 | - [ ] A settings screen for the merge strategies. The columns exist and `MergePullRequestAction` |
| 384 | 409 | honours them, so a repository can be configured through the API and not through the interface. |
| 385 | 410 | - [ ] Reviewer load and staleness visible to maintainers: which requests have gone unanswered, and |
| @@ -18,7 +18,7 @@ grows, so a phase getting *longer* while it is worked on is normal and honest. | ||
| 18 | 18 | | [01 - Foundation](./01-foundation.md) | Users, organizations, teams, tokens, keys | In progress (22/57) | |
| 19 | 19 | | [02 - Git hosting](./02-git-hosting.md) | Repositories on disk, smart HTTP, code browsing | In progress (120/121) | |
| 20 | 20 | | [03 - Issues](./03-issues.md) | Issues, comments, labels, milestones, markdown | Done (37/37) | |
| 21 | | [04 - Reviews](./04-reviews.md) | Pull requests, reviews, diffs, merging, stacks | In progress (71/95) | | |
| 21 | | [04 - Reviews](./04-reviews.md) | Pull requests, reviews, diffs, merging, stacks | In progress (72/95) | | |
| 22 | 22 | | [05 - Notifications and webhooks](./05-notifications-webhooks.md) | Delivery, subscriptions, webhooks | In progress (21/51) | |
| 23 | 23 | | [06 - Search and explore](./06-search-explore.md) | Indexing, search, discovery | Started (1/20) | |
| 24 | 24 | | [07 - Marketing and docs](./07-marketing-docs.md) | Landing page, documentation, self-hosting guide | In progress (21/45) | |
| @@ -0,0 +1,150 @@ | ||
| 1 | <script server> | |
| 2 | /** | |
| 3 | * Who might usefully look at this pull request, fetched when somebody asks. | |
| 4 | * | |
| 5 | * The endpoint behind this costs a `git log` over the changed paths, and the | |
| 6 | * roadmap's whole reason for it being an endpoint is that the cost is not paid | |
| 7 | * on every render of a page most readers scroll straight past. So the panel | |
| 8 | * ships closed with nothing in it, and the first open - and only the first - | |
| 9 | * asks. A `<details>` does the opening and closing with no script at all; the | |
| 10 | * script's one job is to notice the first open and fetch. | |
| 11 | * | |
| 12 | * A suggestion, never a request. Nothing here submits anything: `CODEOWNERS` | |
| 13 | * requests automatically because a file in the repository said to, and this is | |
| 14 | * the forge having an opinion, which is offered rather than acted on. Each name | |
| 15 | * carries its reason verbatim - "3 commits here, last 5d ago, 2 waiting on | |
| 16 | * them" - because a name nobody can account for is one people either click | |
| 17 | * without thinking or ignore. | |
| 18 | * | |
| 19 | * Props: url, the suggested-reviewers endpoint with owner, repo and number | |
| 20 | * already in it. Built by the page, so this component does not know how a pull | |
| 21 | * request is addressed. | |
| 22 | */ | |
| 23 | const fetchUrl = String(url ?? '') | |
| 24 | </script> | |
| 25 | ||
| 26 | <details class="panel side-panel suggest-box" data-suggested-reviewers data-url=""> | |
| 27 | <summary class="side-heading suggest-summary">Suggested reviewers</summary> | |
| 28 | ||
| 29 | {{-- | |
| 30 | The sentence is the closed state, the no-JavaScript state, and the loading | |
| 31 | state's starting point, all three. A reader with scripts off who opens the | |
| 32 | panel gets an honest sentence rather than a spinner that never stops. | |
| 33 | --}} | |
| 34 | <p class="muted side-empty" data-suggest-status>Who has worked on these files.</p> | |
| 35 | <ul class="suggest-list" data-suggest-list hidden></ul> | |
| 36 | </details> | |
| 37 | ||
| 38 | <script type="module"> | |
| 39 | const box = document.querySelector('[data-suggested-reviewers]') | |
| 40 | ||
| 41 | if (box) { | |
| 42 | const status = box.querySelector('[data-suggest-status]') | |
| 43 | const list = box.querySelector('[data-suggest-list]') | |
| 44 | let asked = false | |
| 45 | ||
| 46 | box.addEventListener('toggle', async () => { | |
| 47 | // Only the first open fetches. The answer does not change while the page | |
| 48 | // is up, and the git log behind it is not free. | |
| 49 | if (!box.open || asked) | |
| 50 | return | |
| 51 | ||
| 52 | asked = true | |
| 53 | status.textContent = 'Looking through the history…' | |
| 54 | ||
| 55 | try { | |
| 56 | // A bare fetch, no options, and that is load-bearing: stx's client | |
| 57 | // bridge seeds any identifier a client script shares with the server | |
| 58 | // scope into the page, and this page's server scope has a `headers` | |
| 59 | // binding holding the request headers - cookie included. Writing | |
| 60 | // `{ headers: ... }` here serialized the reader's session token into | |
| 61 | // the HTML. The endpoint answers JSON without being asked. | |
| 62 | const answer = await fetch(box.dataset.url) | |
| 63 | if (!answer.ok) | |
| 64 | throw new Error(String(answer.status)) | |
| 65 | ||
| 66 | const body = await answer.json() | |
| 67 | const suggestions = Array.isArray(body?.suggestions) ? body.suggestions : [] | |
| 68 | ||
| 69 | if (suggestions.length === 0) { | |
| 70 | status.textContent = 'Nobody else has history in these files.' | |
| 71 | return | |
| 72 | } | |
| 73 | ||
| 74 | // textContent throughout: a handle is user-chosen text, and building | |
| 75 | // markup out of one would hand every user a script slot in every | |
| 76 | // sidebar that suggests them. | |
| 77 | for (const suggestion of suggestions) { | |
| 78 | const item = document.createElement('li') | |
| 79 | item.className = 'suggest-item' | |
| 80 | ||
| 81 | const who = document.createElement('a') | |
| 82 | who.className = 'mono suggest-handle' | |
| 83 | who.href = `/${suggestion.handle}` | |
| 84 | who.textContent = suggestion.handle | |
| 85 | ||
| 86 | const why = document.createElement('span') | |
| 87 | why.className = 'muted suggest-reason' | |
| 88 | why.textContent = suggestion.reason | |
| 89 | ||
| 90 | item.append(who, why) | |
| 91 | list.append(item) | |
| 92 | } | |
| 93 | ||
| 94 | status.hidden = true | |
| 95 | list.hidden = false | |
| 96 | } | |
| 97 | catch { | |
| 98 | // Nothing to offer is the same outcome as not being able to ask. | |
| 99 | status.textContent = 'Suggestions are unavailable.' | |
| 100 | } | |
| 101 | }) | |
| 102 | } | |
| 103 | </script> | |
| 104 | ||
| 105 | <style> | |
| 106 | .suggest-summary { | |
| 107 | cursor: pointer; | |
| 108 | list-style: none; | |
| 109 | display: flex; | |
| 110 | align-items: center; | |
| 111 | gap: 6px; | |
| 112 | } | |
| 113 | ||
| 114 | .suggest-summary::-webkit-details-marker { display: none; } | |
| 115 | ||
| 116 | {{-- | |
| 117 | A drawn marker rather than the native one: the native triangle renders at | |
| 118 | text size beside a 12px uppercase heading and dwarfs it. | |
| 119 | --}} | |
| 120 | .suggest-summary::before { | |
| 121 | content: ''; | |
| 122 | width: 7px; | |
| 123 | height: 7px; | |
| 124 | border-right: 1.5px solid var(--muted); | |
| 125 | border-bottom: 1.5px solid var(--muted); | |
| 126 | transform: rotate(-45deg); | |
| 127 | transition: transform 120ms ease; | |
| 128 | flex: none; | |
| 129 | } | |
| 130 | ||
| 131 | .suggest-box[open] .suggest-summary::before { transform: rotate(45deg); } | |
| 132 | ||
| 133 | .suggest-list { | |
| 134 | margin-top: 9px; | |
| 135 | display: flex; | |
| 136 | flex-direction: column; | |
| 137 | gap: 7px; | |
| 138 | } | |
| 139 | ||
| 140 | .suggest-item { | |
| 141 | display: flex; | |
| 142 | flex-direction: column; | |
| 143 | gap: 1px; | |
| 144 | font-size: 13.5px; | |
| 145 | } | |
| 146 | ||
| 147 | .suggest-handle { width: fit-content; } | |
| 148 | ||
| 149 | .suggest-reason { font-size: 12.5px; } | |
| 150 | </style> | |
| @@ -9,6 +9,15 @@ import { viewerFromCookies as viewerFromCookiesImpl } from '../../app/Actions/Id | ||
| 9 | 9 | |
| 10 | 10 | export const viewerFromCookies = viewerFromCookiesImpl |
| 11 | 11 | |
| 12 | /** | |
| 13 | * A raw `Cookie` header, parsed into the jar `viewerFromCookies` reads. | |
| 14 | * For pages served without `__stxServeContext`: see the note in | |
| 15 | * `app/Actions/Identity/lookup.ts`. | |
| 16 | */ | |
| 17 | import { cookieJarFromHeader as cookieJarFromHeaderImpl } from '../../app/Actions/Identity/lookup' | |
| 18 | ||
| 19 | export const cookieJarFromHeader = cookieJarFromHeaderImpl | |
| 20 | ||
| 12 | 21 | /** |
| 13 | 22 | * The owners somebody may create a repository under: their own account, then |
| 14 | 23 | * the organizations they belong to. The same rule the create endpoint enforces. |
| @@ -11,6 +11,7 @@ | ||
| 11 | 11 | * where forges get slow, and it is slowest exactly when the review matters. |
| 12 | 12 | */ |
| 13 | 13 | import { repositoryForView, repositoryPath } from '../../../../functions/repo' |
| 14 | import { cookieJarFromHeader } from '../../../../functions/identity' | |
| 14 | 15 | import { startsCollapsed } from '../../../../../app/Actions/Pull/manifest' |
| 15 | 16 | import { approvalsSatisfied, diffTotals, isGenerated, mergeBlockers, parseDiff, pullRequestDiff, requirementsSatisfied, blockedBy, buildStack, orphanMessage, orphanReason, stackSummary, refreshMergeability, renderDiffFile, highlightDiffFile, loadReviewThreads, anchorThreads, threadSlotFor } from '../../../../functions/review' |
| 16 | 17 | import { renderMarkdownHighlighted } from '../../../../functions/markdown' |
| @@ -42,7 +43,19 @@ const markdownContext = { owner, repository: repositoryName } | ||
| 42 | 43 | */ |
| 43 | 44 | const serveContext = typeof __stxServeContext === 'undefined' ? undefined : __stxServeContext |
| 44 | 45 | |
| 45 | const 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 | */ | |
| 55 | const headerBag = typeof headers === 'undefined' ? undefined : headers | |
| 56 | const viewerCookies = serveContext?.cookies ?? cookieJarFromHeader(headerBag?.cookie) | |
| 57 | ||
| 58 | const access = await repositoryForView(owner, repositoryName, viewerCookies) | |
| 46 | 59 | const repoRow: any = access?.repository ?? null |
| 47 | 60 | |
| 48 | 61 | const pullRequest = repoRow |
| @@ -143,14 +156,182 @@ for (const file of files) { | ||
| 143 | 156 | diffHtmlByPath[file.path] = renderDiffFile(file, { |
| 144 | 157 | expandable: true, |
| 145 | 158 | 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. | |
| 149 | 164 | collapsed: startsCollapsed(file) ? 'fold' : false, |
| 150 | 165 | threadsAt: threadSlotFor(fileThreads, file.path), |
| 151 | 166 | }) |
| 152 | 167 | } |
| 153 | 168 | |
| 169 | const descriptionHtml = pullRequest?.body | |
| 170 | ? await renderMarkdownHighlighted(String(pullRequest.body), markdownContext) | |
| 171 | : '' | |
| 172 | ||
| 173 | const 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 | ||
| 182 | let requiredChecks: string[] = [] | |
| 183 | try { | |
| 184 | const parsed = JSON.parse(String(protection?.required_checks ?? '[]')) | |
| 185 | if (Array.isArray(parsed)) | |
| 186 | requiredChecks = parsed.map(String) | |
| 187 | } | |
| 188 | catch { | |
| 189 | requiredChecks = [] | |
| 190 | } | |
| 191 | ||
| 192 | const 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 | ||
| 201 | const 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 | ||
| 211 | const 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 | ||
| 222 | const 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 | */ | |
| 233 | const 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 | ||
| 245 | const 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 | */ | |
| 279 | const 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 | ||
| 287 | const 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 | ||
| 298 | const currentMember = pullRequest ? stackMembers.find((entry: any) => entry.id === Number(pullRequest.id)) : undefined | |
| 299 | const stack = currentMember ? buildStack(stackMembers, currentMember.id) : [] | |
| 300 | const stackText = stackSummary(stack) | |
| 301 | const 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 | */ | |
| 309 | const 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 | ||
| 314 | const 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 | */ | |
| 332 | const suggestUrl = `/api/repos/pulls/suggested-reviewers?owner=${encodeURIComponent(owner)}&repo=${encodeURIComponent(repositoryName)}&number=${number}` | |
| 333 | const offerSuggestions = Boolean(pullRequest && pullRequest.state === 'open' && access?.viewer && access.can('pull:review')) | |
| 334 | ||
| 154 | 335 | const title = pullRequest |
| 155 | 336 | ? `${pullRequest.title} by ${authorName ?? 'someone'} · ${owner}/${repositoryName} #${number}` |
| 156 | 337 | : 'Pull request not found' |
| @@ -294,6 +475,12 @@ const statePill = pullRequest | ||
| 294 | 475 | @endif |
| 295 | 476 | </section> |
| 296 | 477 | |
| 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 | ||
| 297 | 484 | <section class="panel side-panel"> |
| 298 | 485 | <h2 class="side-heading">Reviews</h2> |
| 299 | 486 | @if (reviews.length === 0) |
| @@ -0,0 +1,25 @@ | ||
| 1 | import { route } from '@stacksjs/router' | |
| 2 | import ui from '../config/ui' | |
| 3 | ||
| 4 | /** | |
| 5 | * Not a route: where the file-based views find their components. | |
| 6 | * | |
| 7 | * Two servers render stx here and only one of them was told. The dev frontend | |
| 8 | * goes through bun-plugin-stx, which loads `config/ui.ts` itself and searches | |
| 9 | * `resources/components`. Everything that boots through `route.serve()` - the | |
| 10 | * API server behind `./buddy dev`, the e2e suite, a production boot - renders | |
| 11 | * through bun-router's file-based routing instead, which never reads that | |
| 12 | * config: it falls back to `<viewsDir>/components`, a directory this project | |
| 13 | * does not have, and every `<Component />` tag on those boots renders as an | |
| 14 | * "[Error loading component]" line in the page. `<CsrfField />` is one of | |
| 15 | * them, so any form served that way loses its token and submits into a 403. | |
| 16 | * | |
| 17 | * This file runs with the other route files, which both boot paths import | |
| 18 | * before serving, so the render config is set wherever rendering happens. | |
| 19 | * | |
| 20 | * `componentsDir` only, deliberately. `config/ui.ts` also names a layoutsDir | |
| 21 | * of `resources/layouts`, which does not exist - layouts live under | |
| 22 | * `resources/views/layouts`, exactly where bun-router's fallback looks, so | |
| 23 | * forwarding the config value would break every page while fixing nothing. | |
| 24 | */ | |
| 25 | route.bunRouter.views({ componentsDir: ui.componentsDir }) | |
| @@ -0,0 +1,313 @@ | ||
| 1 | // The suggested-reviewers panel, through the real routes, with the credential | |
| 2 | // a browser actually carries. | |
| 3 | // | |
| 4 | // The ranking is unit tested and the git-to-forge joins are covered by | |
| 5 | // suggest-reviewers.test.ts, which calls the action in process. What neither | |
| 6 | // can see is the surface this file pins: the endpoint answering a fetch whose | |
| 7 | // only credential is a cookie (docs/todo's "a signed-in browser is not a | |
| 8 | // signed-in test client" - every other caller of this endpoint holds a | |
| 9 | // bearer), and the page offering the panel without paying for it. | |
| 10 | // | |
| 11 | // The panel's design constraint is that the `git log` is spent when the reader | |
| 12 | // opens it, never at render. No browser here runs the script, so that is | |
| 13 | // asserted the only way markup can assert it: the rendered page carries the | |
| 14 | // panel and the URL it would ask, and none of the answer. | |
| 15 | ||
| 16 | import { afterAll, beforeAll, describe, expect, test } from 'bun:test' | |
| 17 | import { mkdirSync, mkdtempSync, rmdirSync, rmSync, writeFileSync } from 'node:fs' | |
| 18 | import { tmpdir } from 'node:os' | |
| 19 | import { join, resolve } from 'node:path' | |
| 20 | import process from 'node:process' | |
| 21 | ||
| 22 | const created = { | |
| 23 | authorId: 0, | |
| 24 | expertId: 0, | |
| 25 | repositoryId: 0, | |
| 26 | handle: '', | |
| 27 | expertHandle: '', | |
| 28 | authorToken: '', | |
| 29 | name: '', | |
| 30 | diskPath: '', | |
| 31 | temp: '', | |
| 32 | } | |
| 33 | ||
| 34 | let available = false | |
| 35 | let port = 0 | |
| 36 | let server: any = null | |
| 37 | ||
| 38 | function unique(prefix: string): string { | |
| 39 | return `${prefix}${Buffer.from(crypto.getRandomValues(new Uint8Array(5))).toString('hex')}` | |
| 40 | } | |
| 41 | ||
| 42 | /** Commit as a specific person, which is what the suggester reads. */ | |
| 43 | async function git(cwd: string, as: { name: string, email: string } | null, ...args: string[]): Promise<string> { | |
| 44 | const child = Bun.spawn(['git', ...args], { | |
| 45 | cwd, | |
| 46 | stdout: 'pipe', | |
| 47 | stderr: 'pipe', | |
| 48 | env: { | |
| 49 | ...process.env, | |
| 50 | GIT_AUTHOR_NAME: as?.name ?? 'E2E', | |
| 51 | GIT_AUTHOR_EMAIL: as?.email ?? 'e2e@example.com', | |
| 52 | GIT_COMMITTER_NAME: 'E2E', | |
| 53 | GIT_COMMITTER_EMAIL: 'e2e@example.com', | |
| 54 | GIT_TERMINAL_PROMPT: '0', | |
| 55 | }, | |
| 56 | }) | |
| 57 | ||
| 58 | const [stdout, stderr, code] = await Promise.all([ | |
| 59 | new Response(child.stdout).text(), | |
| 60 | new Response(child.stderr).text(), | |
| 61 | child.exited, | |
| 62 | ]) | |
| 63 | ||
| 64 | if (code !== 0) | |
| 65 | throw new Error(`git ${args.join(' ')} exited ${code}: ${stderr.trim()}`) | |
| 66 | ||
| 67 | return stdout.trim() | |
| 68 | } | |
| 69 | ||
| 70 | /** The read a browser makes: a cookie, and nothing else. */ | |
| 71 | async function fetchPage(path: string, cookieToken?: string): Promise<{ status: number, html: string }> { | |
| 72 | const answer = await fetch(`http://127.0.0.1:${port}${path}`, { | |
| 73 | headers: { | |
| 74 | Accept: 'text/html', | |
| 75 | ...(cookieToken ? { Cookie: `auth-token=${cookieToken}` } : {}), | |
| 76 | }, | |
| 77 | }) | |
| 78 | ||
| 79 | return { status: answer.status, html: await answer.text() } | |
| 80 | } | |
| 81 | ||
| 82 | beforeAll(async () => { | |
| 83 | created.temp = mkdtempSync(join(tmpdir(), 'reviewos-suggest-panel-')) | |
| 84 | ||
| 85 | try { | |
| 86 | const { injectGlobalAutoImports } = await import('@stacksjs/server') | |
| 87 | const { route } = await import('@stacksjs/router') | |
| 88 | ||
| 89 | await injectGlobalAutoImports() | |
| 90 | await (globalThis as any).db.selectFrom('users').select(['id']).limit(1).execute() | |
| 91 | ||
| 92 | await route.importRoutes() | |
| 93 | server = await route.serve({ port: 0, hostname: '127.0.0.1' }) | |
| 94 | port = Number((server as any)?.port ?? (server as any)?.server?.port ?? 0) | |
| 95 | ||
| 96 | if (!port) | |
| 97 | throw new Error('the router did not report a port') | |
| 98 | ||
| 99 | const { repositoryPath } = await import('../../app/Actions/Git/storage') | |
| 100 | const { initBare } = await import('../../app/Actions/Git/git') | |
| 101 | const { createToken } = await import('@stacksjs/auth') | |
| 102 | ||
| 103 | const make = async (prefix: string): Promise<{ id: number, handle: string, email: string }> => { | |
| 104 | const handle = unique(prefix) | |
| 105 | const email = `${handle}@example.com` | |
| 106 | const row: any = await (globalThis as any).db | |
| 107 | .insertInto('users') | |
| 108 | .values({ name: 'Panel Tester', email, handle, password: 'x' }) | |
| 109 | .returning(['id']) | |
| 110 | .executeTakeFirst() | |
| 111 | ||
| 112 | return { id: Number(row?.id), handle, email } | |
| 113 | } | |
| 114 | ||
| 115 | const author = await make('spa') | |
| 116 | const expert = await make('spe') | |
| 117 | ||
| 118 | created.authorId = author.id | |
| 119 | created.handle = author.handle | |
| 120 | created.expertId = expert.id | |
| 121 | created.expertHandle = expert.handle | |
| 122 | ||
| 123 | const issued: any = await createToken(author.id, 'suggested reviewers panel test') | |
| 124 | created.authorToken = String(issued?.plainTextToken ?? issued?.token ?? issued) | |
| 125 | ||
| 126 | created.name = unique('repo') | |
| 127 | const resolvedPath = repositoryPath(created.handle, created.name) | |
| 128 | created.diskPath = resolvedPath.path! | |
| 129 | ||
| 130 | const repository: any = await (globalThis as any).db | |
| 131 | .insertInto('repositories') | |
| 132 | .values({ | |
| 133 | owner_type: 'user', | |
| 134 | owner_id: created.authorId, | |
| 135 | name: created.name, | |
| 136 | description: 'created by the suggested reviewers panel end to end test', | |
| 137 | visibility: 'public', | |
| 138 | default_branch: 'main', | |
| 139 | disk_path: resolvedPath.relative!, | |
| 140 | }) | |
| 141 | .returning(['id']) | |
| 142 | .executeTakeFirst() | |
| 143 | ||
| 144 | created.repositoryId = Number(repository?.id) | |
| 145 | ||
| 146 | mkdirSync(resolve(created.diskPath, '..'), { recursive: true }) | |
| 147 | await initBare(created.diskPath, 'main') | |
| 148 | ||
| 149 | const work = join(created.temp, 'seed') | |
| 150 | mkdirSync(work) | |
| 151 | await git(work, null, 'init', '--initial-branch=main') | |
| 152 | ||
| 153 | writeFileSync(join(work, 'touched.ts'), 'export const a = 0\n') | |
| 154 | await git(work, null, 'add', '.') | |
| 155 | await git(work, null, 'commit', '-m', 'the base') | |
| 156 | ||
| 157 | // The expert's history is what the endpoint will name, when asked. | |
| 158 | for (let round = 1; round <= 3; round += 1) { | |
| 159 | writeFileSync(join(work, 'touched.ts'), `export const a = ${round}\n`) | |
| 160 | await git(work, expert, 'add', '.') | |
| 161 | await git(work, expert, 'commit', '-m', `expert round ${round}`) | |
| 162 | } | |
| 163 | ||
| 164 | await git(work, null, 'push', created.diskPath, 'main') | |
| 165 | const baseSha = await git(work, null, 'rev-parse', 'HEAD') | |
| 166 | ||
| 167 | await git(work, null, 'checkout', '-b', 'change') | |
| 168 | writeFileSync(join(work, 'touched.ts'), 'export const a = 99\n') | |
| 169 | await git(work, { name: 'Author', email: `${created.handle}@example.com` }, 'add', '.') | |
| 170 | await git(work, { name: 'Author', email: `${created.handle}@example.com` }, 'commit', '-m', 'change the touched file') | |
| 171 | const headSha = await git(work, null, 'rev-parse', 'HEAD') | |
| 172 | await git(work, null, 'push', created.diskPath, 'change') | |
| 173 | ||
| 174 | await (globalThis as any).db | |
| 175 | .insertInto('pull_requests') | |
| 176 | .values({ | |
| 177 | repository_id: created.repositoryId, | |
| 178 | number: 1, | |
| 179 | title: 'Change the touched file', | |
| 180 | body: 'Opened by the suggested reviewers panel end to end test.', | |
| 181 | author_id: created.authorId, | |
| 182 | state: 'open', | |
| 183 | head_branch: 'change', | |
| 184 | head_sha: headSha, | |
| 185 | base_branch: 'main', | |
| 186 | base_sha: baseSha, | |
| 187 | draft: false, | |
| 188 | additions: 1, | |
| 189 | deletions: 1, | |
| 190 | changed_files: 1, | |
| 191 | }) | |
| 192 | .execute() | |
| 193 | ||
| 194 | available = true | |
| 195 | } | |
| 196 | catch (error) { | |
| 197 | console.warn(`[e2e] skipping: ${error instanceof Error ? error.message : String(error)}`) | |
| 198 | available = false | |
| 199 | } | |
| 200 | }, 120_000) | |
| 201 | ||
| 202 | afterAll(async () => { | |
| 203 | try { | |
| 204 | if (created.repositoryId) | |
| 205 | await (globalThis as any).db.deleteFrom('repositories').where('id', '=', created.repositoryId).execute() | |
| 206 | ||
| 207 | for (const id of [created.authorId, created.expertId]) { | |
| 208 | if (id) | |
| 209 | await (globalThis as any).db.deleteFrom('users').where('id', '=', id).execute() | |
| 210 | } | |
| 211 | } | |
| 212 | catch { /* the temp files still go, below */ } | |
| 213 | ||
| 214 | if (created.diskPath) { | |
| 215 | rmSync(created.diskPath, { recursive: true, force: true }) | |
| 216 | ||
| 217 | try { | |
| 218 | rmdirSync(resolve(created.diskPath, '..')) | |
| 219 | } | |
| 220 | catch { /* somebody else's repository lives there too */ } | |
| 221 | } | |
| 222 | ||
| 223 | if (created.temp) | |
| 224 | rmSync(created.temp, { recursive: true, force: true }) | |
| 225 | ||
| 226 | try { | |
| 227 | server?.stop?.(true) | |
| 228 | } | |
| 229 | catch { /* already down */ } | |
| 230 | }) | |
| 231 | ||
| 232 | describe('the suggested reviewers panel', () => { | |
| 233 | /** | |
| 234 | * The fetch the panel's script makes, with the credential a browser holds. | |
| 235 | * Every other caller of this endpoint in the suite authenticates with a | |
| 236 | * bearer, and a bearer works whether or not the cookie path does. | |
| 237 | */ | |
| 238 | test('the endpoint answers a cookie-only fetch with the expert and the why', async () => { | |
| 239 | if (!available) | |
| 240 | return | |
| 241 | ||
| 242 | const answer = await fetch( | |
| 243 | `http://127.0.0.1:${port}/api/repos/pulls/suggested-reviewers?owner=${created.handle}&repo=${created.name}&number=1`, | |
| 244 | { headers: { Accept: 'application/json', Cookie: `auth-token=${created.authorToken}` } }, | |
| 245 | ) | |
| 246 | ||
| 247 | expect(answer.status).toBe(200) | |
| 248 | ||
| 249 | const body: any = await answer.json() | |
| 250 | const suggestion = (body?.suggestions ?? []).find((entry: any) => entry.handle === created.expertHandle) | |
| 251 | ||
| 252 | expect(suggestion).toBeDefined() | |
| 253 | // The stable prefix only: the "last Nd ago" half reads the clock. | |
| 254 | expect(String(suggestion.reason)).toContain('3 commits here') | |
| 255 | }, 30_000) | |
| 256 | ||
| 257 | test('the page offers the panel to a signed-in reviewer, without paying for it', async () => { | |
| 258 | if (!available) | |
| 259 | return | |
| 260 | ||
| 261 | const { status, html } = await fetchPage(`/${created.handle}/${created.name}/pull/1`, created.authorToken) | |
| 262 | ||
| 263 | expect(status).toBe(200) | |
| 264 | expect(html).toContain('data-suggested-reviewers') | |
| 265 | ||
| 266 | // The URL the script would ask, addressed by the server so the client does | |
| 267 | // not know how a pull request is named. | |
| 268 | expect(html).toContain('/api/repos/pulls/suggested-reviewers?owner=') | |
| 269 | expect(html).toContain(`number=1`) | |
| 270 | ||
| 271 | // And none of the answer. The expert's handle appearing here would mean | |
| 272 | // the git log ran at render time, which is the cost the endpoint exists | |
| 273 | // to defer. | |
| 274 | expect(html).not.toContain(created.expertHandle) | |
| 275 | }, 30_000) | |
| 276 | ||
| 277 | /** | |
| 278 | * The reader's credential stays out of the document. stx's client bridge | |
| 279 | * seeds any identifier a client script shares with the server scope into | |
| 280 | * the page, and the server scope holds the request headers - so a script | |
| 281 | * that so much as says `headers` serializes the session cookie into the | |
| 282 | * HTML. That shipped, briefly, as `fetch(url, { headers: ... })`; this is | |
| 283 | * the assertion that keeps it shipped out. | |
| 284 | */ | |
| 285 | test('the page never carries the session token that fetched it', async () => { | |
| 286 | if (!available) | |
| 287 | return | |
| 288 | ||
| 289 | const { html } = await fetchPage(`/${created.handle}/${created.name}/pull/1`, created.authorToken) | |
| 290 | ||
| 291 | expect(html).not.toContain(created.authorToken) | |
| 292 | expect(html).not.toContain('auth-token') | |
| 293 | }, 30_000) | |
| 294 | ||
| 295 | test('the closed panel already says what it is, for readers without scripts', async () => { | |
| 296 | if (!available) | |
| 297 | return | |
| 298 | ||
| 299 | const { html } = await fetchPage(`/${created.handle}/${created.name}/pull/1`, created.authorToken) | |
| 300 | ||
| 301 | expect(html).toContain('Who has worked on these files.') | |
| 302 | }, 30_000) | |
| 303 | ||
| 304 | test('a reader with no session is not offered a panel they cannot use', async () => { | |
| 305 | if (!available) | |
| 306 | return | |
| 307 | ||
| 308 | const { status, html } = await fetchPage(`/${created.handle}/${created.name}/pull/1`) | |
| 309 | ||
| 310 | expect(status).toBe(200) | |
| 311 | expect(html).not.toContain('data-suggested-reviewers') | |
| 312 | }, 30_000) | |
| 313 | }) | |