diff --git a/apps/web/src/components/ChatMarkdown.browser.tsx b/apps/web/src/components/ChatMarkdown.browser.tsx index 99cc3bfa..764fa5ed 100644 --- a/apps/web/src/components/ChatMarkdown.browser.tsx +++ b/apps/web/src/components/ChatMarkdown.browser.tsx @@ -51,6 +51,7 @@ vi.mock("../localApi", () => ({ })); import ChatMarkdown from "./ChatMarkdown"; +import { ThreadPullRequestLinkContext } from "./chat/ThreadPullRequestLinkContext"; import { __resetEnvironmentApiOverridesForTests, __setEnvironmentApiOverrideForTests, @@ -64,6 +65,12 @@ const CHAT_MARKDOWN_THREAD_REF = scopeThreadRef( CHAT_MARKDOWN_ENVIRONMENT_ID, CHAT_MARKDOWN_THREAD_ID, ); +// The pull request the thread route would hand a transcript, with its opener +// mocked so a click can be observed. +const THREAD_PULL_REQUEST_LINK = { + url: "https://github.com/Threadlines/threadlines/pull/223", + open: vi.fn(), +}; // The inline image loader reads files through react-query, so those renders // need the provider the app root supplies. @@ -540,6 +547,54 @@ describe("ChatMarkdown", () => { } }); + it("opens the thread's own pull request in its tab and leaves deeper links alone", async () => { + const { url: pullRequestUrl, open } = THREAD_PULL_REQUEST_LINK; + open.mockClear(); + const screen = await render( + + + , + ); + + // A click the link leaves alone bubbles past React's root to the window, + // still unclaimed; catching it there keeps the test page from actually + // leaving. A click the link claims is stopped before it gets here. + let leftAlone = false; + const observeClick = (event: Event) => { + leftAlone = !event.defaultPrevented; + event.preventDefault(); + }; + window.addEventListener("click", observeClick); + + try { + const click = (name: string) => { + const anchor = Array.from(document.querySelectorAll("a")).find( + (candidate) => candidate.textContent === name, + ); + if (!anchor) { + throw new Error(`No link named ${name}`); + } + leftAlone = false; + return anchor.dispatchEvent(new MouseEvent("click", { bubbles: true, cancelable: true })); + }; + + expect(click("the PR")).toBe(false); + expect(open).toHaveBeenCalledTimes(1); + + click("its files"); + expect(leftAlone).toBe(true); + expect(open).toHaveBeenCalledTimes(1); + } finally { + window.removeEventListener("click", observeClick); + await screen.unmount(); + } + }); + it("keeps normal web links unchanged", async () => { const screen = await render( , diff --git a/apps/web/src/components/chat/ChatWebLink.tsx b/apps/web/src/components/chat/ChatWebLink.tsx index b60d2615..961ebce8 100644 --- a/apps/web/src/components/chat/ChatWebLink.tsx +++ b/apps/web/src/components/chat/ChatWebLink.tsx @@ -4,8 +4,10 @@ import { memo, useCallback, type MouseEvent as ReactMouseEvent, type ReactNode } import { isElectron } from "../../env"; import { readLocalApi } from "../../localApi"; import { isPlainPrimaryClick, openUrlInBrowserPanel } from "../browser/openInBrowserPanel"; +import { isLinkToPullRequest } from "../pull-requests/pullRequests.logic"; import { stackedThreadToast, toastManager } from "../ui/toast"; import { copyTextWithToast } from "./copyTextWithToast"; +import { useThreadPullRequestLink } from "./ThreadPullRequestLinkContext"; export interface ChatWebLinkProps { href: string; @@ -20,11 +22,13 @@ export interface ChatWebLinkProps { /** * A web address in a transcript. * - * The plain click goes to the thread's own browser, because a link an agent - * wrote is about the work in front of you and leaving the app to read it costs - * you the place you were. Everything else -- modifiers, middle click, and any - * thread without a panel -- falls through to the anchor's own behaviour, so the - * link is never less capable than an ordinary one. + * The plain click stays in the app, because a link an agent wrote is about the + * work in front of you and leaving to read it costs you the place you were. A + * link to the thread's own pull request opens the Pull request tab, the same + * place the sidebar badge goes; any other page goes to the thread's browser. + * Everything else -- modifiers, middle click, and any thread without a panel -- + * falls through to the anchor's own behaviour, so the link is never less + * capable than an ordinary one. */ export const ChatWebLink = memo(function ChatWebLink({ href, @@ -33,9 +37,22 @@ export const ChatWebLink = memo(function ChatWebLink({ className, title, }: ChatWebLinkProps) { + const pullRequestLink = useThreadPullRequestLink(); + const opensPullRequestTab = + pullRequestLink !== null && isLinkToPullRequest(href, pullRequestLink.url); + const handleClick = useCallback( (event: ReactMouseEvent) => { - if (!isPlainPrimaryClick(event) || !isElectron || threadRef === null) { + if (!isPlainPrimaryClick(event)) { + return; + } + if (opensPullRequestTab) { + event.preventDefault(); + event.stopPropagation(); + pullRequestLink?.open(); + return; + } + if (!isElectron || threadRef === null) { return; } if (!openUrlInBrowserPanel(threadRef, href)) { @@ -44,7 +61,7 @@ export const ChatWebLink = memo(function ChatWebLink({ event.preventDefault(); event.stopPropagation(); }, - [href, threadRef], + [href, opensPullRequestTab, pullRequestLink, threadRef], ); const handleContextMenu = useCallback( @@ -55,14 +72,23 @@ export const ChatWebLink = memo(function ChatWebLink({ event.preventDefault(); event.stopPropagation(); + // The browser-panel entry only appears where the plain click no longer + // goes there, so every destination stays one right-click away. const clicked = await api.contextMenu.show( [ + ...(opensPullRequestTab && threadRef !== null + ? [{ id: "open-browser-panel", label: "Open in browser panel" } as const] + : []), { id: "open-external", label: "Open in external browser" }, { id: "copy-link", label: "Copy link address" }, ] as const, { x: event.clientX, y: event.clientY }, ); + if (clicked === "open-browser-panel" && threadRef !== null) { + openUrlInBrowserPanel(threadRef, href); + return; + } if (clicked === "open-external") { void api.shell.openExternal(href).catch((error: unknown) => { toastManager.add( @@ -79,7 +105,7 @@ export const ChatWebLink = memo(function ChatWebLink({ copyTextWithToast(href, "Link address"); } }, - [href], + [href, opensPullRequestTab, threadRef], ); return ( diff --git a/apps/web/src/components/chat/ThreadPullRequestLinkContext.ts b/apps/web/src/components/chat/ThreadPullRequestLinkContext.ts new file mode 100644 index 00000000..aea980e2 --- /dev/null +++ b/apps/web/src/components/chat/ThreadPullRequestLinkContext.ts @@ -0,0 +1,18 @@ +import { createContext, useContext } from "react"; + +/** + * The pull request the thread's right panel can show, handed to the transcript + * so a link to it opens the Pull request tab instead of the host's page. The + * thread route provides it; anywhere else a transcript renders, it is null and + * links behave as they always have. + */ +export interface ThreadPullRequestLink { + readonly url: string; + readonly open: () => void; +} + +export const ThreadPullRequestLinkContext = createContext(null); + +export function useThreadPullRequestLink(): ThreadPullRequestLink | null { + return useContext(ThreadPullRequestLinkContext); +} diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts index a1f48a8f..a1fffe0a 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.test.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.test.ts @@ -25,6 +25,7 @@ import { formatPullRequestChecksSummary, groupPullRequests, groupTimelineRows, + isLinkToPullRequest, linkThreadsToPullRequests, matchesPullRequestQuery, matchesPullRequestSelection, @@ -1309,6 +1310,48 @@ describe("applyPendingPullRequestReactions", () => { }); }); +describe("isLinkToPullRequest", () => { + const pullRequestUrl = "https://github.com/Threadlines/threadlines/pull/223"; + + it("matches the bare address, ignoring host casing and a trailing slash", () => { + expect(isLinkToPullRequest(pullRequestUrl, pullRequestUrl)).toBe(true); + expect( + isLinkToPullRequest("https://GitHub.com/Threadlines/threadlines/pull/223/", pullRequestUrl), + ).toBe(true); + }); + + it("leaves links into a part of the pull request the tab cannot show", () => { + expect( + isLinkToPullRequest( + "https://github.com/Threadlines/threadlines/pull/223/files", + pullRequestUrl, + ), + ).toBe(false); + expect( + isLinkToPullRequest( + "https://github.com/Threadlines/threadlines/pull/223#issuecomment-1", + pullRequestUrl, + ), + ).toBe(false); + expect( + isLinkToPullRequest( + "https://github.com/Threadlines/threadlines/pull/223?diff=split", + pullRequestUrl, + ), + ).toBe(false); + }); + + it("leaves other pull requests and unreadable addresses alone", () => { + expect( + isLinkToPullRequest("https://github.com/Threadlines/threadlines/pull/224", pullRequestUrl), + ).toBe(false); + expect( + isLinkToPullRequest("https://github.com/Other/threadlines/pull/223", pullRequestUrl), + ).toBe(false); + expect(isLinkToPullRequest("not a url", pullRequestUrl)).toBe(false); + }); +}); + describe("resolveDefaultMergeMethod", () => { it("runs the method last used on the repository while it is still allowed", () => { expect(resolveDefaultMergeMethod(["merge", "squash", "rebase"], "squash")).toBe("squash"); diff --git a/apps/web/src/components/pull-requests/pullRequests.logic.ts b/apps/web/src/components/pull-requests/pullRequests.logic.ts index 9f942e7a..c3d5a7fd 100644 --- a/apps/web/src/components/pull-requests/pullRequests.logic.ts +++ b/apps/web/src/components/pull-requests/pullRequests.logic.ts @@ -542,6 +542,43 @@ export interface ThreadPullRequest { readonly repository: string | null; } +/** + * Whether a link in a transcript points at exactly this pull request, so the + * app can show its own Pull request tab instead of the host's page. + * + * Only the bare address counts: a link into the Files tab, the checks page, + * or a particular comment names a spot our tab cannot show, and landing at the + * top of the pull request instead would be the misleading case. The comparison + * ignores the host's casing and a trailing slash, nothing else. + */ +export function isLinkToPullRequest(href: string, pullRequestUrl: string): boolean { + const link = parseHttpUrl(href); + const target = parseHttpUrl(pullRequestUrl); + if (link === null || target === null) { + return false; + } + if (link.search !== "" || link.hash !== "") { + return false; + } + return ( + link.origin.toLowerCase() === target.origin.toLowerCase() && + stripTrailingSlash(link.pathname) === stripTrailingSlash(target.pathname) + ); +} + +function parseHttpUrl(value: string): URL | null { + try { + const url = new URL(value.trim()); + return url.protocol === "https:" || url.protocol === "http:" ? url : null; + } catch { + return null; + } +} + +function stripTrailingSlash(pathname: string): string { + return pathname.endsWith("/") ? pathname.slice(0, -1) : pathname; +} + /** The thread fields both the sidebar summary and the full thread record carry. */ export interface ThreadPullRequestSubject { readonly environmentId: EnvironmentId; diff --git a/apps/web/src/routes/_chat.$environmentId.$threadId.tsx b/apps/web/src/routes/_chat.$environmentId.$threadId.tsx index 1ba1141d..9e5dd0c7 100644 --- a/apps/web/src/routes/_chat.$environmentId.$threadId.tsx +++ b/apps/web/src/routes/_chat.$environmentId.$threadId.tsx @@ -18,6 +18,7 @@ import { finalizePromotedDraftThreadByRef, useComposerDraftStore } from "../comp import { useSavedEnvironmentRegistryStore } from "../environments/runtime"; import { type DiffRouteSearch, parseDiffRouteSearch } from "../diffRouteSearch"; import { AgentsPanel } from "../components/chat/AgentsPanel"; +import { ThreadPullRequestLinkContext } from "../components/chat/ThreadPullRequestLinkContext"; import { ChatRightPanel } from "../components/ChatRightPanel"; import { useAgentsPanelSource } from "../agentsPanelStore"; import { preloadDiffPanel, schedulePreloadDiffPanel } from "../diffPanelPreload"; @@ -474,6 +475,15 @@ function ChatThreadRouteView() { }, [activateDiffTab, currentThreadKey, diffTarget, navigateToTab], ); + // What a transcript link to this pull request opens. Only offered once the + // tab can actually show it, so a link never lands on the tab's fallback. + const threadPullRequestLink = useMemo( + () => + threadPullRequest && threadPullRequestReference + ? { url: threadPullRequest.url, open: () => selectTab("pullRequest") } + : null, + [selectTab, threadPullRequest, threadPullRequestReference], + ); const closeTab = useCallback( (tab: RightPanelTab) => { const nextTab = closeRightPanelTab(currentThreadKey, tab); @@ -644,14 +654,16 @@ function ChatThreadRouteView() { return ( <> - + + + - + + +