Public: anyone can read this; the crew can change it.
The header's Talk link carries ?about= on a repository, issue or pull page the viewer actually sees #10635
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/10495-the-header-s-talk-link-carries-about-on → main 3 commits fork 1 thread · 1 unresolved
5 files
changed +75
−4
src/web/layout.ts +19 −3
| @@ -112,6 +112,14 @@ export interface PageOptions { | ||
| 112 | 112 | jsonLd?: string; |
| 113 | 113 | /** Breadcrumb segments rendered in the topbar, e.g. owner / repo. */ |
| 114 | 114 | crumbs?: { label: string; href: string }[]; |
| 115 | /** | |
| 116 | * What the viewer is looking at, for the header's Talk link: `owner/repo` | |
| 117 | * or `owner/repo#N`. Only a page that rendered that subject for this viewer | |
| 118 | * passes it (repository home, issue, pull); it is never derived from the | |
| 119 | * path, so a 404 and a refusal keep the bare /talk and cannot tell a | |
| 120 | * private repository from a missing one. | |
| 121 | */ | |
| 122 | about?: string; | |
| 115 | 123 | body: Html; |
| 116 | 124 | /** |
| 117 | 125 | * CSS this request computed: label colors mixed from the labels actually |
| @@ -296,18 +304,24 @@ function featureItems(ctx: Ctx, user: User): MenuItem[] { | ||
| 296 | 304 | : [{ separator: true, label: "" }, { label: "Features from apps", heading: true }, ...entries]; |
| 297 | 305 | } |
| 298 | 306 | |
| 307 | /** `/talk`, or `/talk?about=…` when the page rendered a subject. */ | |
| 308 | function talkHref(about: string | undefined): string { | |
| 309 | return about === undefined ? "/talk" : `/talk?about=${encodeURIComponent(about).replace(/%2F/g, "/")}`; | |
| 310 | } | |
| 311 | ||
| 299 | 312 | function userMenu( |
| 300 | 313 | ctx: Ctx, |
| 301 | 314 | user: User, |
| 302 | 315 | theme: ThemeName | undefined, |
| 303 | 316 | signOut: MenuItem, |
| 317 | about?: string, | |
| 304 | 318 | ): Html { |
| 305 | 319 | const items: (MenuItem | Html)[] = [ |
| 306 | 320 | { label: "Your profile", href: `/${user.username}`, icon: "user" }, |
| 307 | 321 | { label: "Repositories", href: "/repos", icon: "repo" }, |
| 308 | 322 | { label: "Apps", href: "/apps", icon: "rocket" }, |
| 309 | 323 | { label: "Notes", href: "/notes", icon: "comment" }, |
| 310 | { label: "Talk", href: "/talk", icon: "comment" }, | |
| 324 | { label: "Talk", href: talkHref(about), icon: "comment" }, | |
| 311 | 325 | { label: "Explore", href: "/explore", icon: "search" }, |
| 312 | 326 | { label: "Packs", href: "/packs", icon: "rocket" }, |
| 313 | 327 | { label: "Displays", href: "/displays", icon: "monitor" }, |
| @@ -623,6 +637,7 @@ function topbar( | ||
| 623 | 637 | theme: ThemeName | undefined, |
| 624 | 638 | crumbs: PageOptions["crumbs"], |
| 625 | 639 | tools?: Html, |
| 640 | about?: string, | |
| 626 | 641 | ): Html { |
| 627 | 642 | const user = ctx.auth.user; |
| 628 | 643 | return html`<a class="skip" href="#main">Skip to content</a> |
| @@ -647,7 +662,7 @@ function topbar( | ||
| 647 | 662 | <span class="topbar-tools"> |
| 648 | 663 | ${tools ?? html`${searchBar()} |
| 649 | 664 | ${user |
| 650 | ? html`<a class="btn btn-icon" href="/talk" aria-label="Talk to the forge" data-shortcut="g t" ${hintAttr("g t", "Talk to the forge")} | |
| 665 | ? html`<a class="btn btn-icon" href="${talkHref(about)}" aria-label="Talk to the forge" data-shortcut="g t" ${hintAttr("g t", "Talk to the forge")} | |
| 651 | 666 | >${icon("comment")}</a |
| 652 | 667 | ><a class="small" href="/asks" data-shortcut="g q" ${hintAttr("g q")}>Asks</a |
| 653 | 668 | >${newMenu(ctx)}${userMenu( |
| @@ -661,6 +676,7 @@ function topbar( | ||
| 661 | 676 | icon: "signOut", |
| 662 | 677 | } |
| 663 | 678 | : { label: "Sign out", post: "/logout", icon: "signOut" }, |
| 679 | about, | |
| 664 | 680 | )}` |
| 665 | 681 | : html`${guestMenu(ctx, theme)}${btn("Sign in", { |
| 666 | 682 | href: signInHref(ctx), |
| @@ -740,7 +756,7 @@ export function page(options: PageOptions, status = 200): Response { | ||
| 740 | 756 | |
| 741 | 757 | // The body's markup. Every `icon()` left a `<use>` behind; they are aimed |
| 742 | 758 | // at the one hashed sprite file below, so no page carries a drawing. |
| 743 | const shell = html`${options.bare ? null : topbar(ctx, theme, options.crumbs, options.tools)} | |
| 759 | const shell = html`${options.bare ? null : topbar(ctx, theme, options.crumbs, options.tools, options.about)} | |
| 744 | 760 | <main |
| 745 | 761 | class="${options.bare |
| 746 | 762 | ? "bare" |
src/web/pages/repo.ts +2
| @@ -1370,6 +1370,7 @@ async function codeHome(ctx: Ctx, repo: Repo): Promise<Response> { | ||
| 1370 | 1370 | title: repo.name, |
| 1371 | 1371 | description: repoLine({ private: repo.visibility === "private", name: repo.name, description: repo.description }), |
| 1372 | 1372 | ogImage: `/og/repo/${repo.ownerName}~${repo.name}.png`, |
| 1373 | about: `${repo.ownerName}/${repo.name}`, | |
| 1373 | 1374 | feed: { |
| 1374 | 1375 | href: `${repoBase(repo)}/activity.atom`, |
| 1375 | 1376 | title: `${repo.ownerName}/${repo.name} activity`, |
| @@ -1445,6 +1446,7 @@ async function codeHome(ctx: Ctx, repo: Repo): Promise<Response> { | ||
| 1445 | 1446 | title: repo.name, |
| 1446 | 1447 | description: repoLine({ private: repo.visibility === "private", name: repo.name, description: repo.description }), |
| 1447 | 1448 | ogImage: `/og/repo/${repo.ownerName}~${repo.name}.png`, |
| 1449 | about: `${repo.ownerName}/${repo.name}`, | |
| 1448 | 1450 | feed: { |
| 1449 | 1451 | href: `${repoBase(repo)}/activity.atom`, |
| 1450 | 1452 | title: `${repo.ownerName}/${repo.name} activity`, |
src/web/studio/change.ts +1
| @@ -763,6 +763,7 @@ async function changeConversation( | ||
| 763 | 763 | ctx, |
| 764 | 764 | title: `${pull.title} · ${repo.name}`, |
| 765 | 765 | ogImage: cardPath("pull", repo, pull), |
| 766 | about: `${repo.ownerName}/${repo.name}#${pull.number}`, | |
| 766 | 767 | description: threadLine({ |
| 767 | 768 | private: repo.visibility === "private", |
| 768 | 769 | word, |
src/web/studio/tell.ts +1
| @@ -621,6 +621,7 @@ async function studioTell( | ||
| 621 | 621 | ctx, |
| 622 | 622 | title: `${issue.title} · ${repo.name}`, |
| 623 | 623 | ogImage: cardPath("issue", repo, issue), |
| 624 | about: `${repo.ownerName}/${repo.name}#${issue.number}`, | |
| 624 | 625 | description: threadLine({ |
| 625 | 626 | private: repo.visibility === "private" || issue.sealed, |
| 626 | 627 | word: wordInfo?.word ?? (issue.state === "open" ? "open" : "closed"), |
test/talk-page.test.ts +52 −1
| @@ -10,9 +10,10 @@ import { mkdtemp, rm } from "node:fs/promises"; | ||
| 10 | 10 | import { tmpdir } from "node:os"; |
| 11 | 11 | import { join } from "node:path"; |
| 12 | 12 | import { createApp, type App } from "../src/app.ts"; |
| 13 | import { createRepo } from "../src/actions/repos.ts"; | |
| 13 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 14 | 14 | import { sayNo } from "../src/actions/conversation_confirms.ts"; |
| 15 | 15 | import { openIssue } from "../src/actions/issues.ts"; |
| 16 | import { openPull } from "../src/actions/pulls.ts"; | |
| 16 | 17 | import { clock, STREAMS_MAX } from "../src/web/pages/talk.ts"; |
| 17 | 18 | import { serveTalk } from "../src/tui/ssh-talk.ts"; |
| 18 | 19 | import { aboutLine, parseAbout } from "../src/core/talk_about.ts"; |
| @@ -752,3 +753,53 @@ describe("?about= names the subject", () => { | ||
| 752 | 753 | expect(run(bob, "--about ../x")).toEqual({ out: "", code: 0 }); |
| 753 | 754 | }); |
| 754 | 755 | }); |
| 756 | ||
| 757 | describe("the header's Talk link carries the subject the page rendered", () => { | |
| 758 | const header = (page: string) => /<header class="topbar"[\s\S]*?<\/header>/.exec(page)?.[0] ?? ""; | |
| 759 | const talkLinks = (page: string) => [...header(page).matchAll(/href="(\/talk[^"]*)"/g)].map((m) => m[1]); | |
| 760 | ||
| 761 | test("an issue page and a repository page link /talk?about=…", async () => { | |
| 762 | const made = await createRepo(app, alice, { name: "hdr-pub", visibility: "public" }); | |
| 763 | if (!made.ok) throw new Error(String(made.error)); | |
| 764 | const issue = openIssue(app, alice, made.value, { title: "Hello", body: "x" }); | |
| 765 | if (!issue.ok) throw new Error(String(issue.error)); | |
| 766 | const n = issue.value.number; | |
| 767 | const links = talkLinks(await (await get(`/alice/hdr-pub/issues/${n}`, bob)).text()); | |
| 768 | expect(links).toContain(`/talk?about=alice/hdr-pub%23${n}`); | |
| 769 | expect(links.length).toBeGreaterThanOrEqual(1); | |
| 770 | expect(new Set(links).size).toBe(1); | |
| 771 | expect(talkLinks(await (await get("/alice/hdr-pub", bob)).text())).toContain("/talk?about=alice/hdr-pub"); | |
| 772 | expect(talkLinks(await (await get("/explore", bob)).text())).toEqual(["/talk", "/talk"]); | |
| 773 | }); | |
| 774 | ||
| 775 | test("a pull page links /talk?about=owner/repo%23N", async () => { | |
| 776 | const made = await createRepo(app, alice, { name: "hdr-pull", visibility: "public" }); | |
| 777 | if (!made.ok) throw new Error(String(made.error)); | |
| 778 | for (const [branch, file] of [[undefined, "a.txt"], ["topic", "t.txt"]] as const) { | |
| 779 | const done = await commitToBranch(app, alice, made.value, { | |
| 780 | ...(branch === undefined ? {} : { branch }), | |
| 781 | message: "change", | |
| 782 | files: [{ path: file, content: "x" }], | |
| 783 | }); | |
| 784 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 785 | } | |
| 786 | const pull = await openPull(app, alice, made.value, { title: "P", body: "", baseBranch: "main", headBranch: "topic" }); | |
| 787 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); | |
| 788 | const res = await get(`/alice/hdr-pull/pulls/${pull.value.number}`, bob); | |
| 789 | expect(res.status).toBe(200); | |
| 790 | expect(talkLinks(await res.text())).toContain(`/talk?about=alice/hdr-pull%23${pull.value.number}`); | |
| 791 | }); | |
| 792 | ||
| 793 | test("a 404 under a private and under a missing repository render the same header", async () => { | |
| 794 | const made = await createRepo(app, alice, { name: "hdr-priv", visibility: "private" }); | |
| 795 | if (!made.ok) throw new Error(String(made.error)); | |
| 796 | const a = await get("/alice/hdr-priv/issues/1", bob); | |
| 797 | const b = await get("/alice/hdr-nowhere/issues/1", bob); | |
| 798 | expect(a.status).toBe(404); | |
| 799 | expect(b.status).toBe(404); | |
| 800 | // The rest of the chrome echoes the asked path (the "new issue" link), so compare the Talk entries. | |
| 801 | const [ta, tb] = [talkLinks(await a.text()), talkLinks(await b.text())]; | |
| 802 | expect(ta).toEqual(tb); | |
| 803 | expect(ta).toEqual(["/talk", "/talk"]); | |
| 804 | }); | |
| 805 | }); |
The pull page passes
about, but no test would fail without this line. Add a test that fetches a pull page and expects/talk?about=owner/repo%23Nin the topbar. The issue said the pull page passes it, so it needs the same cover as the issue page.