also looking at this
feat(reviews): the suggestions get an interface, priced as designed
#6
9 files
+741
-10
| @@ -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 |