From 095b4f2fc03230b4e4bb9400fe2ee754fc997200 Mon Sep 17 00:00:00 2001 From: Brandon Oxendine Date: Fri, 9 Oct 2026 13:25:25 +0200 Subject: [PATCH] Add copy link and highlight to comments Opening a comment link expands the revision the comment is in, even if the link is opened while already on the page, and scrolls to it. This also fixes scrolling to URL fragments that aren't valid CSS selectors, such as comment ids starting with a digit. --- src/components/Comment.svelte | 60 ++++++++++++++++++++++++++++- src/lib/router.ts | 5 ++- src/lib/utils.ts | 12 ++++++ src/views/repos/Cob/Revision.svelte | 40 ++++++++++++++++++- tests/e2e/clipboard.spec.ts | 13 +++++++ tests/e2e/repo/patch.spec.ts | 40 +++++++++++++++++++ tests/unit/utils.test.ts | 19 +++++++++ 7 files changed, 184 insertions(+), 5 deletions(-) diff --git a/src/components/Comment.svelte b/src/components/Comment.svelte index 2c711f1f0..c4da7f301 100644 --- a/src/components/Comment.svelte +++ b/src/components/Comment.svelte @@ -1,12 +1,17 @@ -
+ + +
{#if isReply}
@@ -163,6 +212,15 @@ • edited
{/if} +
+ + + +
diff --git a/src/lib/router.ts b/src/lib/router.ts index a39c1beab..211dc3c4c 100644 --- a/src/lib/router.ts +++ b/src/lib/router.ts @@ -219,8 +219,9 @@ async function navigate( window.scrollTo(0, restoreScrollY ?? 0); return; } - if (hash.length > 1) { - const el = document.querySelector(hash); + const fragment = utils.fragmentId(hash); + if (fragment !== undefined) { + const el = document.getElementById(fragment); if (el) { const top = el.getBoundingClientRect().top + diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 11c62c78c..68fd692b3 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -246,6 +246,18 @@ export function scrollIntoView(id: string, options?: ScrollIntoViewOptions) { if (lineElement) lineElement.scrollIntoView(options); } +export function fragmentId(hash: string): string | undefined { + const fragment = hash.startsWith("#") ? hash.slice(1) : hash; + if (fragment === "") { + return undefined; + } + try { + return decodeURIComponent(fragment); + } catch { + return undefined; + } +} + // Check whether the given path has a markdown file extension. export function isMarkdownPath(path: string): boolean { return /\.(md|mkd|markdown)$/i.test(path); diff --git a/src/views/repos/Cob/Revision.svelte b/src/views/repos/Cob/Revision.svelte index 214b392cd..c31ee709c 100644 --- a/src/views/repos/Cob/Revision.svelte +++ b/src/views/repos/Cob/Revision.svelte @@ -13,7 +13,7 @@ import * as utils from "@app/lib/utils"; import { HttpdClient } from "@http-client"; import { cachedGetDiff } from "@app/views/repos/router"; - import { onMount } from "svelte"; + import { onMount, tick } from "svelte"; import CobCommitTeaser from "@app/views/repos/Cob/CobCommitTeaser.svelte"; import CommentComponent from "@app/components/Comment.svelte"; @@ -55,7 +55,41 @@ export let previousRevOid: string | undefined = undefined; export let first: boolean; - let expanded = initiallyExpanded; + function timelinesContain(commentId: string | undefined): boolean { + if (commentId === undefined) { + return false; + } + const inThread = (thread: { root: Comment; replies: Comment[] }) => + thread.root.id === commentId || + thread.replies.some(reply => reply.id === commentId); + return timelines.some(element => { + switch (element.type) { + case "thread": + return inThread(element.inner); + case "review": { + const review = element.inner[1]; + return review.id === commentId || review.threads.some(inThread); + } + case "merge": + return false; + } + }); + } + + let expanded = + initiallyExpanded || + timelinesContain(utils.fragmentId(window.location.hash)); + + async function expandToHash() { + const commentId = utils.fragmentId(window.location.hash); + if (expanded || commentId === undefined || !timelinesContain(commentId)) { + return; + } + expanded = true; + await tick(); + utils.scrollIntoView(commentId); + } + const api = new HttpdClient(baseUrl); const lastEdit = revisionEdits.at(-1); @@ -273,6 +307,8 @@ } + +
diff --git a/tests/e2e/clipboard.spec.ts b/tests/e2e/clipboard.spec.ts index 13940d1de..905355dfa 100644 --- a/tests/e2e/clipboard.spec.ts +++ b/tests/e2e/clipboard.spec.ts @@ -87,6 +87,19 @@ test("copy to clipboard", async ({ page, browserName, context }) => { await expectClipboard("Inline `code` has `back-ticks around` it.\n"); } + // Link to a patch comment. + { + await page.goto(`${cobUrl}/patches`); + await page + .getByRole("link", { name: "Taking another stab at the README" }) + .click(); + const review = page.locator(".card", { hasText: "rejected revision" }); + const reviewId = await review.getAttribute("id"); + await review.hover(); + await review.getByRole("button", { name: "Copy link to comment" }).click(); + await expectClipboard(`${page.url()}#${reviewId}`); + } + // Clear the system clipboard contents so developers don't wonder why there's // random stuff in their clipboard after running tests. await page.evaluate("navigator.clipboard.writeText('')"); diff --git a/tests/e2e/repo/patch.spec.ts b/tests/e2e/repo/patch.spec.ts index 0236acdb2..d1f77dada 100644 --- a/tests/e2e/repo/patch.spec.ts +++ b/tests/e2e/repo/patch.spec.ts @@ -154,3 +154,43 @@ test("commit listing ordering keeping stable on browser navigation", async ({ .waitFor(); await expectCorrectCommitListing(); }); + +test("link to a comment in an older revision", async ({ page }) => { + await page.goto(`${cobUrl}/patches`); + await page + .getByRole("link", { name: "Taking another stab at the README" }) + .click(); + await page.getByLabel("expand").first().click(); + const review = page.locator(".card", { hasText: "accepted revision" }); + const reviewId = (await review.getAttribute("id")) ?? ""; + expect(reviewId).toMatch(/^[a-f0-9]{40}$/); + const patchUrl = page.url(); + + await page.goto(`${patchUrl}#${reviewId}`); + await expect(review).toBeVisible(); + await expect(review).toHaveClass(/targeted/); + await expect(review).toBeInViewport(); + + await page.goto(patchUrl); + await expect(review).toBeHidden(); + await page.evaluate(id => (window.location.hash = id), reviewId); + await expect(review).toBeVisible(); + await expect(review).toHaveClass(/targeted/); + await expect(review).toBeInViewport(); +}); + +test("malformed fragment does not break the patch page", async ({ page }) => { + await page.goto(`${cobUrl}/patches`); + await page + .getByRole("link", { name: "Taking another stab at the README" }) + .click(); + await page + .getByRole("heading", { name: "Taking another stab at the README" }) + .waitFor(); + + await page.goto(`${page.url()}#%`); + await expect( + page.getByRole("heading", { name: "Taking another stab at the README" }), + ).toBeVisible(); + await expect(page.locator(".card.targeted")).toHaveCount(0); +}); diff --git a/tests/unit/utils.test.ts b/tests/unit/utils.test.ts index 4d57491a9..eff4ca504 100644 --- a/tests/unit/utils.test.ts +++ b/tests/unit/utils.test.ts @@ -429,3 +429,22 @@ describe("untwemoji", () => { expect(node.innerHTML).toEqual("ab"); }); }); + +describe("fragmentId", () => { + test.each([ + { hash: "", expected: undefined }, + { hash: "#", expected: undefined }, + { hash: "#delegate", expected: "delegate" }, + { hash: "delegate", expected: "delegate" }, + { + hash: "#0a1b2c3d4e5f60718293a4b5c6d7e8f901234567", + expected: "0a1b2c3d4e5f60718293a4b5c6d7e8f901234567", + }, + { hash: "#caf%C3%A9", expected: "café" }, + { hash: "#%", expected: undefined }, + { hash: "#%zz", expected: undefined }, + { hash: "#%E0%A4%A", expected: undefined }, + ])("fragmentId($hash) => $expected", ({ hash, expected }) => { + expect(utils.fragmentId(hash)).toEqual(expected); + }); +});