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); + }); +});