Public: anyone can read this; the crew can change it.
An open pull's page renders in under 20 ms: its `render` phase is 60–70 ms today because reads hide inside it #8404
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/8389-an-open-pull-s-page-renders-in-under-20 → main 2 commits fork 3 threads · 3 unresolved
7 files
changed +411
−96
src/web/studio/asks.ts +66 −47
| @@ -312,6 +312,7 @@ function askCandidatesFor( | ||
| 312 | 312 | db: Database, |
| 313 | 313 | user: User | undefined, |
| 314 | 314 | allRepos: readonly Repo[], |
| 315 | only?: string, | |
| 315 | 316 | ): AskCandidate[] { |
| 316 | 317 | const out: AskCandidate[] = []; |
| 317 | 318 | if (!user) return out; |
| @@ -321,8 +322,10 @@ function askCandidatesFor( | ||
| 321 | 322 | const ownerFacts = ownerFactRepoIds(db, allRepos, user); |
| 322 | 323 | const repos = allRepos.filter((r) => ownerFacts.has(r.id)); |
| 323 | 324 | const byId = new Map(repos.map((r) => [r.id, r])); |
| 324 | const pulls = listOpenPullsVisibleTo(db, user, 50).filter((p) => | |
| 325 | byId.has(p.repoId), | |
| 325 | // `only` narrows to one pull's own asks, for its page: the same fifty | |
| 326 | // pulls are listed, so it asks exactly what the full read would say of it. | |
| 327 | const pulls = listOpenPullsVisibleTo(db, user, 50).filter( | |
| 328 | (p) => byId.has(p.repoId) && (only === undefined || p.id === only), | |
| 326 | 329 | ); |
| 327 | 330 | const checks = latestRunsForIssues( |
| 328 | 331 | db, |
| @@ -519,6 +522,65 @@ function askCandidatesFor( | ||
| 519 | 522 | const rec = a.kind === "handover" ? advice.get(a.pull!.id) : undefined; |
| 520 | 523 | if (rec !== undefined) out[i] = { ...a, recommendation: rec }; |
| 521 | 524 | } |
| 525 | // Pauses, parked issues, declines and waves carry no pull: one pull's page | |
| 526 | // never shows them, so it never reads them. | |
| 527 | if (only === undefined) repoAsksInto(db, out, repos, byId); | |
| 528 | // Open work assigned to the viewer that no ask above already covers: a | |
| 529 | // pull that is an ask and assigned to them is one row, the ask's. | |
| 530 | const taken = new Set(out.flatMap((a) => [a.pull?.id, a.issue?.id].filter((i) => i !== undefined))); | |
| 531 | const visible = new Map(allRepos.map((r) => [r.id, r])); | |
| 532 | const mine = listAssignedTo(db, user.id, 50).filter( | |
| 533 | (i) => !taken.has(i.id) && (only === undefined || i.id === only), | |
| 534 | ); | |
| 535 | const elsewhere = getReposByIds( | |
| 536 | db, | |
| 537 | [...new Set(mine.map((i) => i.repoId))].filter((id) => !visible.has(id)), | |
| 538 | ); | |
| 539 | // An assignment can outlive the access that made it: each repo needs a real read. | |
| 540 | // Asked once per repository, not once per assigned row. | |
| 541 | const reads = new Map<string, boolean>(); | |
| 542 | const readable = (repo: Repo | undefined): repo is Repo => { | |
| 543 | if (repo === undefined) return false; | |
| 544 | if (visible.has(repo.id)) return true; | |
| 545 | let ok = reads.get(repo.id); | |
| 546 | if (ok === undefined) reads.set(repo.id, (ok = canRead(db, repo, user))); | |
| 547 | return ok; | |
| 548 | }; | |
| 549 | const joinedMine = new Map( | |
| 550 | joinMany(db, mine.filter((i) => i.kind === "pull")).map((p) => [p.id, p]), | |
| 551 | ); | |
| 552 | for (const issue of mine) { | |
| 553 | const repo = visible.get(issue.repoId) ?? elsewhere.get(issue.repoId); | |
| 554 | if (!readable(repo) || repo.archivedAt !== null) continue; | |
| 555 | if (!mayView(db, repo, issue, user)) continue; | |
| 556 | const pull = joinedMine.get(issue.id); | |
| 557 | if (issue.kind === "pull" && pull === undefined) continue; | |
| 558 | // A pull a pass gave up on and assigned to this person says why, and can go back. | |
| 559 | const reason = pull === undefined || issue.assigneeIsAgent ? undefined : passReasonOf(db, issue.id, issue.assigneeId!); | |
| 560 | const back = reason === undefined ? undefined : lastBuilderOf(db, repo, issue)?.username; | |
| 561 | out.push({ | |
| 562 | kind: "assigned", | |
| 563 | id: `assigned:${issue.id}`, | |
| 564 | since: issue.updatedAt, | |
| 565 | repo, | |
| 566 | ...(pull === undefined ? { issue } : { pull }), | |
| 567 | who: issue.authorName, | |
| 568 | authorIsAgent: issue.authorIsAgent, | |
| 569 | ...(reason === undefined ? {} : { why: reason }), | |
| 570 | ...(back === undefined ? {} : { builder: back }), | |
| 571 | }); | |
| 572 | } | |
| 573 | return out; | |
| 574 | } | |
| 575 | ||
| 576 | /** The asks a repository makes that are not a pull's: a ceiling pause, a | |
| 577 | * parked issue, a planner's decline, a wave waiting for its nod. */ | |
| 578 | function repoAsksInto( | |
| 579 | db: Database, | |
| 580 | out: AskCandidate[], | |
| 581 | repos: readonly Repo[], | |
| 582 | byId: ReadonlyMap<string, Repo>, | |
| 583 | ): void { | |
| 522 | 584 | const crew = crewForRepos( |
| 523 | 585 | db, |
| 524 | 586 | repos.map((r) => r.id), |
| @@ -581,50 +643,6 @@ function askCandidatesFor( | ||
| 581 | 643 | proposal, |
| 582 | 644 | who: getUserById(db, proposal.authorId)?.username ?? "the planner", |
| 583 | 645 | }); |
| 584 | // Open work assigned to the viewer that no ask above already covers: a | |
| 585 | // pull that is an ask and assigned to them is one row, the ask's. | |
| 586 | const taken = new Set(out.flatMap((a) => [a.pull?.id, a.issue?.id].filter((i) => i !== undefined))); | |
| 587 | const visible = new Map(allRepos.map((r) => [r.id, r])); | |
| 588 | const mine = listAssignedTo(db, user.id, 50).filter((i) => !taken.has(i.id)); | |
| 589 | const elsewhere = getReposByIds( | |
| 590 | db, | |
| 591 | [...new Set(mine.map((i) => i.repoId))].filter((id) => !visible.has(id)), | |
| 592 | ); | |
| 593 | // An assignment can outlive the access that made it: each repo needs a real read. | |
| 594 | // Asked once per repository, not once per assigned row. | |
| 595 | const reads = new Map<string, boolean>(); | |
| 596 | const readable = (repo: Repo | undefined): repo is Repo => { | |
| 597 | if (repo === undefined) return false; | |
| 598 | if (visible.has(repo.id)) return true; | |
| 599 | let ok = reads.get(repo.id); | |
| 600 | if (ok === undefined) reads.set(repo.id, (ok = canRead(db, repo, user))); | |
| 601 | return ok; | |
| 602 | }; | |
| 603 | const joinedMine = new Map( | |
| 604 | joinMany(db, mine.filter((i) => i.kind === "pull")).map((p) => [p.id, p]), | |
| 605 | ); | |
| 606 | for (const issue of mine) { | |
| 607 | const repo = visible.get(issue.repoId) ?? elsewhere.get(issue.repoId); | |
| 608 | if (!readable(repo) || repo.archivedAt !== null) continue; | |
| 609 | if (!mayView(db, repo, issue, user)) continue; | |
| 610 | const pull = joinedMine.get(issue.id); | |
| 611 | if (issue.kind === "pull" && pull === undefined) continue; | |
| 612 | // A pull a pass gave up on and assigned to this person says why, and can go back. | |
| 613 | const reason = pull === undefined || issue.assigneeIsAgent ? undefined : passReasonOf(db, issue.id, issue.assigneeId!); | |
| 614 | const back = reason === undefined ? undefined : lastBuilderOf(db, repo, issue)?.username; | |
| 615 | out.push({ | |
| 616 | kind: "assigned", | |
| 617 | id: `assigned:${issue.id}`, | |
| 618 | since: issue.updatedAt, | |
| 619 | repo, | |
| 620 | ...(pull === undefined ? { issue } : { pull }), | |
| 621 | who: issue.authorName, | |
| 622 | authorIsAgent: issue.authorIsAgent, | |
| 623 | ...(reason === undefined ? {} : { why: reason }), | |
| 624 | ...(back === undefined ? {} : { builder: back }), | |
| 625 | }); | |
| 626 | } | |
| 627 | return out; | |
| 628 | 646 | } |
| 629 | 647 | |
| 630 | 648 | /** |
| @@ -816,8 +834,9 @@ export function scoredFor( | ||
| 816 | 834 | user: User | undefined, |
| 817 | 835 | repos: readonly Repo[], |
| 818 | 836 | now = Date.now(), |
| 837 | only?: string, | |
| 819 | 838 | ): Scored[] { |
| 820 | const asks = askCandidatesFor(db, user, repos); | |
| 839 | const asks = askCandidatesFor(db, user, repos, only); | |
| 821 | 840 | const scores = consequencesOf(db, asks, now); |
| 822 | 841 | return asks.map((a) => ({ |
| 823 | 842 | ...a, |
src/web/studio/change.ts +91 −44
| @@ -412,7 +412,7 @@ async function pullPanels(ctx: Ctx, repo: Repo, pull: Pull, spent: Spent = newSp | ||
| 412 | 412 | > |
| 413 | 413 | ${wordBadge(word)} <span class="muted small">${note}</span> |
| 414 | 414 | ${when( |
| 415 | tellsAnswered(db, repo.id, [pull]).get(pull.number), | |
| 415 | tick(spent, "tells", () => tellsAnswered(db, repo.id, [pull]).get(pull.number)), | |
| 416 | 416 | (t) => html`<span class="sep" aria-hidden="true">·</span |
| 417 | 417 | >${tellLine(base, t)}`, |
| 418 | 418 | )} |
| @@ -427,7 +427,9 @@ async function pullPanels(ctx: Ctx, repo: Repo, pull: Pull, spent: Spent = newSp | ||
| 427 | 427 | </p>`, |
| 428 | 428 | ); |
| 429 | 429 | |
| 430 | const preview = when(record, () => previewCard(ctx, repo, pull)); | |
| 430 | const preview = tick(spent, "preview", () => | |
| 431 | when(record, () => previewCard(ctx, repo, pull)), | |
| 432 | ); | |
| 431 | 433 | return { |
| 432 | 434 | record, |
| 433 | 435 | db, |
| @@ -498,9 +500,13 @@ async function changeConversation( | ||
| 498 | 500 | : await lap(spent, "diff", () => |
| 499 | 501 | wallOf(wordCtx.dir, status.baseOid, status.headOid), |
| 500 | 502 | ); |
| 501 | const crewDecision = decisionsFor(ctx, [repo], { evenDismissed: true }).find( | |
| 502 | (d) => d.pull?.id === pull.id, | |
| 503 | ); | |
| 503 | // Everything the Decide card, the vision preview and Take it back read is | |
| 504 | // read here, under `decide`; only this pull's own asks (`pull`). | |
| 505 | const decideAt = performance.now(); | |
| 506 | const crewDecision = decisionsFor(ctx, [repo], { | |
| 507 | evenDismissed: true, | |
| 508 | pull: pull.id, | |
| 509 | }).find((d) => d.pull?.id === pull.id); | |
| 504 | 510 | // A person's own change carries no crew decision (`decisionsFor` reads |
| 505 | 511 | // agent pulls only — #208's territory), but the change's own page still |
| 506 | 512 | // owes its writer the same route and the same words, whoever opened it. |
| @@ -591,6 +597,7 @@ async function changeConversation( | ||
| 591 | 597 | ctx.auth.user !== undefined && canAdmin(db, repo, ctx.auth.user) |
| 592 | 598 | ? await previewVision(ctx.app, repo, pull) |
| 593 | 599 | : null; |
| 600 | const decisionForms = decision ? decisionForm(decision) : null; | |
| 594 | 601 | const decisionSection = when(decisionBody, (body) => |
| 595 | 602 | card(body, { |
| 596 | 603 | head: html`${icon("alert")} |
| @@ -626,15 +633,36 @@ async function changeConversation( | ||
| 626 | 633 | ); |
| 627 | 634 | |
| 628 | 635 | const siblings = variantSiblings(db, [pull])[0] ?? []; |
| 636 | spent.decide += performance.now() - decideAt; | |
| 629 | 637 | const glyph = originGlyph(repo); |
| 630 | 638 | const headOid = status?.headOid ?? pull.headOid; |
| 639 | const pageHead = tick(spent, "head", () => ({ | |
| 640 | top: html`${repoHeader(ctx, repo, "pulls", { | |
| 641 | compact: true, | |
| 642 | ownTitle: true, | |
| 643 | })} | |
| 644 | ${changeHead(ctx, repo, pull, status === undefined ? null : { headOid, conflicts: status.conflicts.length })} | |
| 645 | ${changeTabs(repo, pull, "change")}`, | |
| 646 | crumbs: crumbsFor(repo), | |
| 647 | palette: repoPalette(ctx, repo), | |
| 648 | })); | |
| 649 | const gate = tick( | |
| 650 | spent, | |
| 651 | "gate", | |
| 652 | () => html`${holdCard(ctx, repo, pull, { act: true })} | |
| 653 | ${approveHereCard(ctx, repo, pull, headOid, `${thread}/approve-here`)} | |
| 654 | ${teamsCard(teamVerdicts(db, { ...pull, headOid }))} | |
| 655 | ${when(wall, (w) => wallCard(w, writer ? thread : undefined))}`, | |
| 656 | ); | |
| 657 | const look = tick(spent, "looks", () => | |
| 658 | when(record, (r) => lookCard(ctx, repo, r, pull, pull.headOid)), | |
| 659 | ); | |
| 631 | 660 | const reviewsAt = performance.now(); |
| 632 | 661 | const reviews = listReviews(db, pull.id); |
| 633 | 662 | const reviewerMarks = marksByName( |
| 634 | 663 | db, |
| 635 | 664 | reviews.map((r) => r.authorName), |
| 636 | 665 | ); |
| 637 | spent.reviews += performance.now() - reviewsAt; | |
| 638 | 666 | const talk = conversation(ctx, repo, pull, { |
| 639 | 667 | openVerb: "opened this", |
| 640 | 668 | extra: reviews.map((review) => |
| @@ -645,13 +673,23 @@ async function changeConversation( | ||
| 645 | 673 | ? `${thread}/edit` |
| 646 | 674 | : undefined, |
| 647 | 675 | }); |
| 676 | const reply = composer(ctx, { | |
| 677 | action: `${thread}/comments`, | |
| 678 | // Closing is its author's or a writer's (`setIssueState`): nobody | |
| 679 | // else is shown a button the server would only refuse. | |
| 680 | ...(ctx.auth.user !== undefined && | |
| 681 | (ctx.auth.user.id === pull.authorId || writer) | |
| 682 | ? { stateAction: `${thread}/state` } | |
| 683 | : {}), | |
| 684 | state: pull.state, | |
| 685 | locked: pull.mergedAt !== null, | |
| 686 | closeLabel: "Close pull request", | |
| 687 | crewNote: threadNote(ctx.app.db, repo, pull), | |
| 688 | }); | |
| 689 | spent.reviews += performance.now() - reviewsAt; | |
| 648 | 690 | |
| 649 | const body = html`${repoHeader(ctx, repo, "pulls", { | |
| 650 | compact: true, | |
| 651 | ownTitle: true, | |
| 652 | })} | |
| 653 | ${changeHead(ctx, repo, pull, status === undefined ? null : { headOid, conflicts: status.conflicts.length })} | |
| 654 | ${changeTabs(repo, pull, "change")} | |
| 691 | // From here on nothing is read: `render` only builds the page. | |
| 692 | const body = html`${pageHead.top} | |
| 655 | 693 | ${when(siblings.length > 0, () => |
| 656 | 694 | html`<p class="muted small variant-link"> |
| 657 | 695 | <a href="${base}/variants/${pull.variantGroup ?? ""}" |
| @@ -660,33 +698,18 @@ async function changeConversation( | ||
| 660 | 698 | </p>`, |
| 661 | 699 | )} |
| 662 | 700 | <div id="pull-check">${checkLine}</div> |
| 663 | ${when(runLine, (l) => l)} ${holdCard(ctx, repo, pull, { act: true })} | |
| 664 | ${approveHereCard(ctx, repo, pull, headOid, `${thread}/approve-here`)} | |
| 665 | ${teamsCard(teamVerdicts(db, { ...pull, headOid }))} | |
| 666 | ${when(wall, (w) => wallCard(w, ctx.auth.user !== undefined && canWrite(db, repo, ctx.auth.user) ? thread : undefined))} ${decisionSection} | |
| 701 | ${when(runLine, (l) => l)} ${gate} ${decisionSection} | |
| 667 | 702 | ${when(visionPreview, (v) => visionPreviewCard(v))} |
| 668 | 703 | ${takeBackSection} |
| 669 | ${when(record, (r) => lookCard(ctx, repo, r, pull, pull.headOid))} | |
| 704 | ${look} | |
| 670 | 705 | <div class="split"> |
| 671 | 706 | <div> |
| 672 | 707 | ${talk.node} |
| 673 | ${composer(ctx, { | |
| 674 | action: `${thread}/comments`, | |
| 675 | // Closing is its author's or a writer's (`setIssueState`): nobody | |
| 676 | // else is shown a button the server would only refuse. | |
| 677 | ...(ctx.auth.user !== undefined && | |
| 678 | (ctx.auth.user.id === pull.authorId || writer) | |
| 679 | ? { stateAction: `${thread}/state` } | |
| 680 | : {}), | |
| 681 | state: pull.state, | |
| 682 | locked: pull.mergedAt !== null, | |
| 683 | closeLabel: "Close pull request", | |
| 684 | crewNote: threadNote(ctx.app.db, repo, pull), | |
| 685 | })} | |
| 708 | ${reply} | |
| 686 | 709 | </div> |
| 687 | 710 | <aside><div id="pull-preview">${preview}</div></aside> |
| 688 | 711 | </div> |
| 689 | ${decision ? decisionForm(decision) : null} | |
| 712 | ${decisionForms} | |
| 690 | 713 | ${liveMount({ |
| 691 | 714 | repo: base, |
| 692 | 715 | thread: "pull", |
| @@ -705,22 +728,19 @@ async function changeConversation( | ||
| 705 | 728 | title: pull.title, |
| 706 | 729 | body: pull.body, |
| 707 | 730 | }), |
| 708 | crumbs: crumbsFor(repo), | |
| 709 | palette: repoPalette(ctx, repo), | |
| 731 | crumbs: pageHead.crumbs, | |
| 732 | palette: pageHead.palette, | |
| 710 | 733 | body, |
| 711 | 734 | scriptJs: `${liveClientJs}\n${liveRegionsJs}`, |
| 712 | 735 | }); |
| 713 | 736 | // Each phase is named once, from what the laps added up to; render is what |
| 714 | 737 | // is left of the page's own time, so the phases never exceed `app`. |
| 715 | const { diff, checks, reviews: reviewMs } = spent; | |
| 716 | notePhase(request, "diff", diff); | |
| 717 | notePhase(request, "checks", checks); | |
| 718 | notePhase(request, "reviews", reviewMs); | |
| 719 | notePhase( | |
| 720 | request, | |
| 721 | "render", | |
| 722 | Math.max(0, performance.now() - began - diff - checks - reviewMs), | |
| 723 | ); | |
| 738 | let read = 0; | |
| 739 | for (const [name, ms] of Object.entries(spent)) { | |
| 740 | notePhase(request, name, ms); | |
| 741 | read += ms; | |
| 742 | } | |
| 743 | notePhase(request, "render", Math.max(0, performance.now() - began - read)); | |
| 724 | 744 | return response; |
| 725 | 745 | } |
| 726 | 746 | |
| @@ -728,12 +748,29 @@ async function changeConversation( | ||
| 728 | 748 | interface Spent { |
| 729 | 749 | diff: number; |
| 730 | 750 | checks: number; |
| 751 | tells: number; | |
| 752 | preview: number; | |
| 753 | head: number; | |
| 754 | gate: number; | |
| 755 | decide: number; | |
| 756 | looks: number; | |
| 731 | 757 | reviews: number; |
| 732 | 758 | } |
| 733 | const newSpent = (): Spent => ({ diff: 0, checks: 0, reviews: 0 }); | |
| 759 | /** In the order Server-Timing names them. */ | |
| 760 | const newSpent = (): Spent => ({ | |
| 761 | diff: 0, | |
| 762 | checks: 0, | |
| 763 | tells: 0, | |
| 764 | preview: 0, | |
| 765 | head: 0, | |
| 766 | gate: 0, | |
| 767 | decide: 0, | |
| 768 | looks: 0, | |
| 769 | reviews: 0, | |
| 770 | }); | |
| 734 | 771 | |
| 735 | 772 | /** Run `work` and add the time it took to `spent[key]`. */ |
| 736 | async function lap<T>(spent: Spent, key: "diff" | "checks", work: () => Promise<T>): Promise<T> { | |
| 773 | async function lap<T>(spent: Spent, key: keyof Spent, work: () => Promise<T>): Promise<T> { | |
| 737 | 774 | const started = performance.now(); |
| 738 | 775 | try { |
| 739 | 776 | return await work(); |
| @@ -741,3 +778,13 @@ async function lap<T>(spent: Spent, key: "diff" | "checks", work: () => Promise< | ||
| 741 | 778 | spent[key] += performance.now() - started; |
| 742 | 779 | } |
| 743 | 780 | } |
| 781 | ||
| 782 | /** The same for a synchronous read: no await, no microtask. */ | |
| 783 | function tick<T>(spent: Spent, key: keyof Spent, work: () => T): T { | |
| 784 | const started = performance.now(); | |
| 785 | try { | |
| 786 | return work(); | |
| 787 | } finally { | |
| 788 | spent[key] += performance.now() - started; | |
| 789 | } | |
| 790 | } |
src/web/studio/decisions.ts +4 −3
| @@ -28,16 +28,17 @@ const dollars = (n: number): string => (n < 10 ? n.toFixed(2) : n.toFixed(0)); | ||
| 28 | 28 | /** |
| 29 | 29 | * The decisions waiting on the viewer, worded and pictured. A change's own |
| 30 | 30 | * page passes `evenDismissed`: setting an ask aside quiets the lists, never |
| 31 | * the decision on the thing itself. | |
| 31 | * the decision on the thing itself, and names its `pull`: only that pull's | |
| 32 | * own decisions are read, never the repository's whole list. | |
| 32 | 33 | */ |
| 33 | 34 | export function decisionsFor( |
| 34 | 35 | ctx: Ctx, |
| 35 | 36 | allRepos: readonly Repo[], |
| 36 | options: { evenDismissed?: boolean } = {}, | |
| 37 | options: { evenDismissed?: boolean; pull?: string } = {}, | |
| 37 | 38 | ): Decision[] { |
| 38 | 39 | const db = ctx.app.db; |
| 39 | 40 | const candidates = options.evenDismissed |
| 40 | ? scoredFor(db, ctx.auth.user, allRepos) | |
| 41 | ? scoredFor(db, ctx.auth.user, allRepos, Date.now(), options.pull) | |
| 41 | 42 | : waitingOn(db, ctx.auth.user, allRepos); |
| 42 | 43 | // The pictures, in one read: a green pull's row leads with its preview. |
| 43 | 44 | const repoIds = [...new Set(candidates.map((c) => c.repo.id))]; |
test/asks-one-pull.test.ts added +124
| @@ -0,0 +1,124 @@ | ||
| 1 | /** | |
| 2 | * A pull's own page reads only that pull's asks (#8389): `scoredFor` with | |
| 3 | * `only` says exactly what the full read says of that pull, and nothing of | |
| 4 | * the other pulls or of the repository's own asks (a parked issue). | |
| 5 | */ | |
| 6 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 7 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 8 | import { tmpdir } from "node:os"; | |
| 9 | import { join } from "node:path"; | |
| 10 | import { createApp, type App } from "../src/app.ts"; | |
| 11 | import { holdPull } from "../src/actions/holds.ts"; | |
| 12 | import { addComment, assignIssue, openIssue } from "../src/actions/issues.ts"; | |
| 13 | import { openPull } from "../src/actions/pulls.ts"; | |
| 14 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 15 | import type { Pull } from "../src/core/pulls.ts"; | |
| 16 | import type { Repo } from "../src/core/repos.ts"; | |
| 17 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 18 | import { createUser, type User } from "../src/core/users.ts"; | |
| 19 | import { BREAKER_NOTE } from "../src/crew/breaker.ts"; | |
| 20 | import { scoredFor } from "../src/web/studio/asks.ts"; | |
| 21 | import { quietLogs } from "./helpers/globals.ts"; | |
| 22 | ||
| 23 | quietLogs(); | |
| 24 | ||
| 25 | let dataDir: string; | |
| 26 | let app: App; | |
| 27 | let alice: User; | |
| 28 | let repo: Repo; | |
| 29 | const pulls: Record<"green" | "other" | "assigned" | "held", Pull> = {} as never; | |
| 30 | ||
| 31 | async function pull(n: number, green: boolean): Promise<Pull> { | |
| 32 | const done = await commitToBranch(app, alice, repo, { | |
| 33 | branch: `topic/${n}`, | |
| 34 | base: "main", | |
| 35 | message: `change ${n}`, | |
| 36 | files: [{ path: `F${n}.md`, content: `${n}\n` }], | |
| 37 | }); | |
| 38 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 39 | const opened = await openPull(app, alice, repo, { | |
| 40 | title: `change ${n}`, | |
| 41 | body: "", | |
| 42 | baseBranch: "main", | |
| 43 | headBranch: `topic/${n}`, | |
| 44 | }); | |
| 45 | if (!opened.ok) throw new Error(JSON.stringify(opened.error)); | |
| 46 | if (green) { | |
| 47 | const run = createRun(app.db, { | |
| 48 | repoId: repo.id, | |
| 49 | kind: "check", | |
| 50 | issueId: opened.value.id, | |
| 51 | env: `pull-${opened.value.number}`, | |
| 52 | commitOid: opened.value.headOid, | |
| 53 | reason: "pull", | |
| 54 | }); | |
| 55 | claimRun(app.db, run.id); | |
| 56 | finishRun(app.db, run.id, { state: "ok", summary: "check passed" }); | |
| 57 | } | |
| 58 | return opened.value; | |
| 59 | } | |
| 60 | ||
| 61 | beforeAll(async () => { | |
| 62 | dataDir = await mkdtemp(join(tmpdir(), "forge-asks-one-pull-")); | |
| 63 | app = createApp({ dataDir, port: 0 }); | |
| 64 | const made = await createUser(app.db, { | |
| 65 | origin: "seed", | |
| 66 | trusted: true, | |
| 67 | username: "alice", | |
| 68 | email: "[email protected]", | |
| 69 | password: "a-good-passphrase", | |
| 70 | adminIfFirst: true, | |
| 71 | }); | |
| 72 | if (!made.ok) throw new Error(String(made.error)); | |
| 73 | alice = made.value; | |
| 74 | const created = await createRepo(app, alice, { name: "r", visibility: "public" }); | |
| 75 | if (!created.ok) throw new Error(String(created.error)); | |
| 76 | repo = created.value; | |
| 77 | const seeded = await commitToBranch(app, alice, repo, { | |
| 78 | message: "start", | |
| 79 | files: [{ path: "README.md", content: "r\n" }], | |
| 80 | }); | |
| 81 | if (!seeded.ok) throw new Error(JSON.stringify(seeded.error)); | |
| 82 | pulls.green = await pull(1, true); | |
| 83 | pulls.other = await pull(2, true); | |
| 84 | pulls.assigned = await pull(3, false); | |
| 85 | pulls.held = await pull(4, true); | |
| 86 | if (!assignIssue(app, alice, repo, pulls.assigned, alice).ok) throw new Error("assign"); | |
| 87 | if (!holdPull(app, alice, repo, pulls.held, "not yet").ok) throw new Error("hold"); | |
| 88 | const parked = openIssue(app, alice, repo, { title: "parked" }); | |
| 89 | if (!parked.ok) throw new Error(String(parked.error)); | |
| 90 | if (!addComment(app, alice, repo, parked.value, BREAKER_NOTE).ok) throw new Error("park"); | |
| 91 | }); | |
| 92 | ||
| 93 | afterAll(async () => { | |
| 94 | app.close(); | |
| 95 | await rm(dataDir, { recursive: true, force: true }); | |
| 96 | }); | |
| 97 | ||
| 98 | test("the full read holds a look, an assignment and a parked issue, and nothing for a held pull", () => { | |
| 99 | const ids = scoredFor(app.db, alice, [repo]).map((a) => a.id); | |
| 100 | expect(ids).toContain(`look:${pulls.green.id}`); | |
| 101 | expect(ids).toContain(`look:${pulls.other.id}`); | |
| 102 | expect(ids).toContain(`assigned:${pulls.assigned.id}`); | |
| 103 | expect(ids.some((id) => id.startsWith("parked:"))).toBe(true); | |
| 104 | expect(ids.some((id) => id.endsWith(pulls.held.id))).toBe(false); | |
| 105 | }); | |
| 106 | ||
| 107 | test("narrowed to one pull, the read says exactly what the full read says of it", () => { | |
| 108 | const now = Date.now(); | |
| 109 | const full = scoredFor(app.db, alice, [repo], now); | |
| 110 | for (const p of Object.values(pulls)) { | |
| 111 | const narrowed = scoredFor(app.db, alice, [repo], now, p.id); | |
| 112 | expect(narrowed.every((a) => a.pull?.id === p.id)).toBe(true); | |
| 113 | expect(narrowed.map((a) => ({ id: a.id, consequence: a.consequence }))).toEqual( | |
| 114 | full.filter((a) => a.pull?.id === p.id).map((a) => ({ id: a.id, consequence: a.consequence })), | |
| 115 | ); | |
| 116 | } | |
| 117 | expect(scoredFor(app.db, alice, [repo], now, pulls.green.id).map((a) => a.id)).toEqual([ | |
| 118 | `look:${pulls.green.id}`, | |
| 119 | ]); | |
| 120 | expect(scoredFor(app.db, alice, [repo], now, pulls.assigned.id).map((a) => a.id)).toEqual([ | |
| 121 | `assigned:${pulls.assigned.id}`, | |
| 122 | ]); | |
| 123 | expect(scoredFor(app.db, alice, [repo], now, pulls.held.id)).toEqual([]); | |
| 124 | }); |
test/helpers/pull-phases.ts added +4
| @@ -0,0 +1,4 @@ | ||
| 1 | /** The Server-Timing phases an open pull's page names after `app`, in order (#8389). */ | |
| 2 | export const PULL_PAGE_PHASES = [ | |
| 3 | "session", "diff", "checks", "tells", "preview", "head", "gate", "decide", "looks", "reviews", "render", | |
| 4 | ] as const; |
test/pull-page-phases.test.ts added +119
| @@ -0,0 +1,119 @@ | ||
| 1 | /** | |
| 2 | * An open pull's page reads everything before it renders (#8389): each read | |
| 3 | * is its own Server-Timing phase, named in order, and `render` is what is | |
| 4 | * left — for an open pull with a check, a look and a preview record. The | |
| 5 | * 20 ms budget is read from production's Server-Timing, not asserted here: | |
| 6 | * a wall-clock bound on a shared machine flakes. | |
| 7 | */ | |
| 8 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 9 | import { mkdirSync, writeFileSync } from "node:fs"; | |
| 10 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 11 | import { tmpdir } from "node:os"; | |
| 12 | import { dirname, join } from "node:path"; | |
| 13 | import { createApp, type App } from "../src/app.ts"; | |
| 14 | import { enableApp } from "../src/actions/apps.ts"; | |
| 15 | import { openPull } from "../src/actions/pulls.ts"; | |
| 16 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 17 | import { getAppByRepo } from "../src/core/apps.ts"; | |
| 18 | import { recordLook } from "../src/core/looks.ts"; | |
| 19 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 20 | import { createSession } from "../src/core/sessions.ts"; | |
| 21 | import { createUser } from "../src/core/users.ts"; | |
| 22 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 23 | import { quietLogs } from "./helpers/globals.ts"; | |
| 24 | import { PULL_PAGE_PHASES } from "./helpers/pull-phases.ts"; | |
| 25 | import { PNG } from "./helpers/studio.ts"; | |
| 26 | ||
| 27 | quietLogs(); | |
| 28 | ||
| 29 | let dataDir: string; | |
| 30 | let app: App; | |
| 31 | let servers: Servers; | |
outdated
Make this a non-exported const and share it with pull-timing.test.ts instead of repeating the list. |
||
| 32 | let cookie = ""; | |
| 33 | let number = 0; | |
| 34 | const cells = process.env["FORGE_CELLS"]; | |
| 35 | ||
| 36 | beforeAll(async () => { | |
| 37 | process.env["FORGE_CELLS"] = "off"; | |
| 38 | dataDir = await mkdtemp(join(tmpdir(), "forge-pull-page-phases-")); | |
| 39 | app = createApp({ dataDir, port: 0 }); | |
| 40 | servers = await serve(app); | |
| 41 | const made = await createUser(app.db, { | |
| 42 | origin: "seed", | |
| 43 | trusted: true, | |
| 44 | username: "alice", | |
| 45 | email: "[email protected]", | |
| 46 | password: "a-good-passphrase", | |
| 47 | adminIfFirst: true, | |
| 48 | }); | |
| 49 | if (!made.ok) throw new Error(String(made.error)); | |
| 50 | const alice = made.value; | |
| 51 | cookie = `forge_session=${createSession(app.db, alice.id).token}`; | |
| 52 | const repo = await createRepo(app, alice, { name: "r", visibility: "public" }); | |
| 53 | if (!repo.ok) throw new Error(String(repo.error)); | |
| 54 | for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) { | |
| 55 | const done = await commitToBranch(app, alice, repo.value, { | |
| 56 | ...(branch === undefined ? {} : { branch }), | |
| 57 | message: "change", | |
| 58 | files: [{ path: file, content: file === "forge.json" ? '{"run":"bun run x.ts"}' : "x" }], | |
| 59 | }); | |
| 60 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 61 | } | |
| 62 | const enabled = await enableApp(app, alice, repo.value, { slug: "r" }); | |
| 63 | if (!enabled.ok) throw new Error(String(enabled.error)); | |
| 64 | const pull = await openPull(app, alice, repo.value, { | |
| 65 | title: "a change to look at", | |
| 66 | body: "", | |
| 67 | baseBranch: "main", | |
| 68 | headBranch: "topic", | |
| 69 | }); | |
| 70 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); | |
| 71 | number = pull.value.number; | |
| 72 | const env = `pull-${number}`; | |
| 73 | for (const kind of ["check", "deploy"] as const) { | |
| 74 | const run = createRun(app.db, { | |
| 75 | repoId: repo.value.id, | |
| 76 | kind, | |
| 77 | issueId: pull.value.id, | |
| 78 | env, | |
| 79 | commitOid: pull.value.headOid, | |
| 80 | reason: "pull", | |
| 81 | }); | |
| 82 | claimRun(app.db, run.id); | |
| 83 | finishRun(app.db, run.id, { state: "ok", summary: `${kind} passed` }); | |
| 84 | } | |
| 85 | const record = getAppByRepo(app.db, repo.value.id)!; | |
| 86 | const rel = join("looks", record.id, `${env}-desktop.png`); | |
| 87 | mkdirSync(dirname(join(dataDir, rel)), { recursive: true }); | |
| 88 | writeFileSync(join(dataDir, rel), PNG); | |
| 89 | recordLook(app.db, { appId: record.id, env, commitOid: pull.value.headOid, kind: "desktop", path: rel }); | |
| 90 | }); | |
| 91 | ||
| 92 | afterAll(async () => { | |
| 93 | await servers.stop(); | |
| 94 | app.close(); | |
| 95 | if (cells === undefined) delete process.env["FORGE_CELLS"]; | |
| 96 | else process.env["FORGE_CELLS"] = cells; | |
| 97 | await rm(dataDir, { recursive: true, force: true }); | |
| 98 | }); | |
| 99 | ||
| 100 | async function timed(): Promise<{ names: string[]; ms: (n: string) => number; body: string }> { | |
| 101 | const res = await fetch(`${servers.url}/alice/r/pulls/${number}`, { headers: { cookie } }); | |
| 102 | expect(res.status).toBe(200); | |
| 103 | const body = await res.text(); | |
| 104 | const parts = (res.headers.get("server-timing") ?? "").split(", "); | |
| 105 | const all = parts.map((p) => p.split(";")[0]!); | |
| 106 | const ms = (n: string) => Number(parts[all.indexOf(n)]!.match(/dur=([\d.]+)/)![1]); | |
| 107 | return { names: all.slice(all.indexOf("app") + 1), ms, body }; | |
| 108 | } | |
| 109 | ||
| 110 | test("an open pull's page names each of its reads as a phase before render", async () => { | |
| 111 | const got = await timed(); | |
| 112 | expect(got.names).toEqual([...PULL_PAGE_PHASES]); | |
| 113 | // What was seeded is drawn: the look, the preview and the check. | |
| 114 | expect(got.body).toContain("<h2>Look</h2>"); | |
| 115 | expect(got.body).toContain("<h2>Preview</h2>"); | |
| 116 | expect(got.body).toContain('data-word="look"'); | |
| 117 | const sum = got.names.reduce((t, n) => t + got.ms(n), 0); | |
| 118 | expect(sum).toBeLessThanOrEqual(got.ms("app") + 0.1 * got.names.length); | |
| 119 | }); |
test/pull-timing.test.ts +3 −2
| @@ -13,6 +13,7 @@ import type { User } from "../src/core/users.ts"; | ||
| 13 | 13 | import { createUser } from "../src/core/users.ts"; |
| 14 | 14 | import { serve, type Servers } from "../src/http/server.ts"; |
| 15 | 15 | import { quietLogs } from "./helpers/globals.ts"; |
| 16 | import { PULL_PAGE_PHASES } from "./helpers/pull-phases.ts"; | |
| 16 | 17 | |
| 17 | 18 | quietLogs(); |
| 18 | 19 | |
| @@ -100,14 +101,14 @@ afterAll(async () => { | ||
| 100 | 101 | await rm(dataDir, { recursive: true, force: true }); |
| 101 | 102 | }); |
| 102 | 103 | |
| 103 | test("a pull names session, diff, checks, reviews and render after app, summing to no more than app", async () => { | |
| 104 | test("a pull names its read phases and render after app, summing to no more than app", async () => { | |
| 104 | 105 | const res = await fetch(`${servers.url}/alice/r/pulls/${number}`, { headers: { cookie } }); |
| 105 | 106 | expect(res.status).toBe(200); |
| 106 | 107 | await res.arrayBuffer(); |
| 107 | 108 | const parts = (res.headers.get("server-timing") ?? "").split(", "); |
| 108 | 109 | const names = parts.map((p) => p.split(";")[0]!); |
| 109 | 110 | const after = names.slice(names.indexOf("app") + 1); |
| 110 | expect(after).toEqual(["session", "diff", "checks", "reviews", "render"]); | |
| 111 | expect(after).toEqual([...PULL_PAGE_PHASES]); | |
| 111 | 112 | const ms = (n: string) => Number(parts[names.indexOf(n)]!.match(/dur=([\d.]+)/)![1]); |
| 112 | 113 | const sum = after.reduce((t, n) => t + ms(n), 0); |
| 113 | 114 | // Each is rounded to 0.1 ms, so allow one step per phase. |
new above
offline
The
onlyfilter needs a test showing the narrowed read equals the full read's asks for that pull and omits repo-level asks.