Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions apps/web/src/components/ChatMarkdown.browser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ vi.mock("../localApi", () => ({
}));

import ChatMarkdown from "./ChatMarkdown";
import { ThreadPullRequestLinkContext } from "./chat/ThreadPullRequestLinkContext";
import {
__resetEnvironmentApiOverridesForTests,
__setEnvironmentApiOverrideForTests,
Expand All @@ -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.
Expand Down Expand Up @@ -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(
<ThreadPullRequestLinkContext.Provider value={THREAD_PULL_REQUEST_LINK}>
<ChatMarkdown
text={`Opened [the PR](${pullRequestUrl}); see [its files](${pullRequestUrl}/files).`}
cwd="/repo/project"
environmentId={CHAT_MARKDOWN_ENVIRONMENT_ID}
threadId={CHAT_MARKDOWN_THREAD_ID}
/>
</ThreadPullRequestLinkContext.Provider>,
);

// 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(
<ChatMarkdown text="[OpenAI](https://openai.com/docs)" cwd="/repo/project" />,
Expand Down
42 changes: 34 additions & 8 deletions apps/web/src/components/chat/ChatWebLink.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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,
Expand All @@ -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<HTMLAnchorElement>) => {
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)) {
Expand All @@ -44,7 +61,7 @@ export const ChatWebLink = memo(function ChatWebLink({
event.preventDefault();
event.stopPropagation();
},
[href, threadRef],
[href, opensPullRequestTab, pullRequestLink, threadRef],
);

const handleContextMenu = useCallback(
Expand All @@ -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(
Expand All @@ -79,7 +105,7 @@ export const ChatWebLink = memo(function ChatWebLink({
copyTextWithToast(href, "Link address");
}
},
[href],
[href, opensPullRequestTab, threadRef],
);

return (
Expand Down
18 changes: 18 additions & 0 deletions apps/web/src/components/chat/ThreadPullRequestLinkContext.ts
Original file line number Diff line number Diff line change
@@ -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<ThreadPullRequestLink | null>(null);

export function useThreadPullRequestLink(): ThreadPullRequestLink | null {
return useContext(ThreadPullRequestLinkContext);
}
43 changes: 43 additions & 0 deletions apps/web/src/components/pull-requests/pullRequests.logic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
formatPullRequestChecksSummary,
groupPullRequests,
groupTimelineRows,
isLinkToPullRequest,
linkThreadsToPullRequests,
matchesPullRequestQuery,
matchesPullRequestSelection,
Expand Down Expand Up @@ -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");
Expand Down
37 changes: 37 additions & 0 deletions apps/web/src/components/pull-requests/pullRequests.logic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
44 changes: 29 additions & 15 deletions apps/web/src/routes/_chat.$environmentId.$threadId.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -644,14 +654,16 @@ function ChatThreadRouteView() {
return (
<>
<SidebarInset className="h-svh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground md:h-dvh">
<ChatView
environmentId={threadRef.environmentId}
threadId={threadRef.threadId}
onDiffPanelOpen={markDiffOpened}
reserveTitleBarControlInset={!sidebarVisible}
composerFocusRequest={composerFocusRequest}
routeKind="server"
/>
<ThreadPullRequestLinkContext.Provider value={threadPullRequestLink}>
<ChatView
environmentId={threadRef.environmentId}
threadId={threadRef.threadId}
onDiffPanelOpen={markDiffOpened}
reserveTitleBarControlInset={!sidebarVisible}
composerFocusRequest={composerFocusRequest}
routeKind="server"
/>
</ThreadPullRequestLinkContext.Provider>
</SidebarInset>
<ChatRightPanelInlineSidebar
open={sidebarVisible}
Expand All @@ -668,13 +680,15 @@ function ChatThreadRouteView() {
return (
<>
<SidebarInset className="h-svh min-h-0 overflow-hidden overscroll-y-none bg-background text-foreground md:h-dvh">
<ChatView
environmentId={threadRef.environmentId}
threadId={threadRef.threadId}
onDiffPanelOpen={markDiffOpened}
composerFocusRequest={composerFocusRequest}
routeKind="server"
/>
<ThreadPullRequestLinkContext.Provider value={threadPullRequestLink}>
<ChatView
environmentId={threadRef.environmentId}
threadId={threadRef.threadId}
onDiffPanelOpen={markDiffOpened}
composerFocusRequest={composerFocusRequest}
routeKind="server"
/>
</ThreadPullRequestLinkContext.Provider>
</SidebarInset>
<RightPanelSheet
open={sidebarVisible}
Expand Down
Loading