From e26af33b228792bb72349ca4e849a96381578a57 Mon Sep 17 00:00:00 2001 From: Bilal Bakr <62337003+Bil0000@users.noreply.github.com> Date: Fri, 18 Sep 2026 01:38:11 +0300 Subject: [PATCH] fix(web): keep PR panel actions in the current thread (#12320) Co-authored-by: Julius Marminge <51714798+juliusmarminge@users.noreply.github.com> Co-authored-by: Claude Fable 5 --- apps/web/src/components/ChatView.tsx | 27 +- .../PullRequestDetailPanel.test.tsx | 335 ++++++++++++++++++ .../pullRequest/PullRequestDetailPanel.tsx | 11 +- .../PullRequestListEmptyState.test.tsx | 54 --- .../PullRequestsUnavailableState.test.tsx | 73 ---- .../pullRequestDetail.logic.test.ts | 166 ++++++--- .../pullRequest/pullRequestDetail.logic.ts | 62 +++- .../pullRequestLinkContextMenu.test.ts | 13 - .../pullRequestPresentation.test.tsx | 28 -- 9 files changed, 502 insertions(+), 267 deletions(-) create mode 100644 apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx delete mode 100644 apps/web/src/components/pullRequest/PullRequestListEmptyState.test.tsx delete mode 100644 apps/web/src/components/pullRequest/PullRequestsUnavailableState.test.tsx delete mode 100644 apps/web/src/components/pullRequest/pullRequestLinkContextMenu.test.ts delete mode 100644 apps/web/src/components/pullRequest/pullRequestPresentation.test.tsx diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 64f6b0319a1e31c6ae1b6eed6a135b2c2b9e3e57..267d420c9d44052995da53ed025c8c5aed16cff8 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -210,7 +210,7 @@ import { selectThreadPreviewMiniPlayer, usePreviewMiniPlayerStore, } from "../previewMiniPlayerStore"; -import { isThreadOwnPullRequest } from "./pullRequest/pullRequestDetail.logic"; +import { pullRequestPanelContext } from "./pullRequest/pullRequestDetail.logic"; import { PullRequestDetailPanel } from "./pullRequest/PullRequestDetailPanel"; import { PullRequestDetailGhost } from "./pullRequest/PullRequestGhosts"; import { PullRequestsUnavailableState } from "./pullRequest/PullRequestsUnavailableState"; @@ -9627,22 +9627,15 @@ export default function ChatView(props: ChatViewProps) { repository: renderedRightPanelSurface.repository, number: renderedRightPanelSurface.number, }} - context={ - isThreadOwnPullRequest( - { - projectId: linkedThreadPullRequest?.projectId ?? null, - repository: linkedThreadPullRequest?.repository ?? null, - number: linkedThreadPullRequest?.number ?? null, - }, - { - projectId: renderedRightPanelSurface.projectId, - repository: renderedRightPanelSurface.repository, - number: renderedRightPanelSurface.number, - }, - ) - ? "thread" - : "page" - } + context={pullRequestPanelContext( + { + projectId: activeThreadMetadata?.projectId ?? null, + pullRequests: activeThreadMetadata?.pullRequests, + linkedPullRequest: activeThreadMetadata?.linkedPullRequest, + branchPullRequest: activeThreadMetadata?.branchPullRequest, + }, + renderedRightPanelSurface, + )} composerDraftTarget={composerDraftTarget} onBack={ activeThreadRef !== null && pullRequestsSurfaceAvailable && visiblePullRequestCount > 1 diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx new file mode 100644 index 0000000000000000000000000000000000000000..4bdf6ee7d5359a2ba06bf1c1d060708395d5b931 --- /dev/null +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.test.tsx @@ -0,0 +1,335 @@ +import { + EnvironmentId, + ProjectId, + ThreadId, + type ScopedThreadRef, + type PullRequestDetailView, + type ThreadPullRequestLink, +} from "@t3tools/contracts"; +import { DEFAULT_CLIENT_SETTINGS } from "@t3tools/contracts/settings"; +import { act, type ReactNode, type ReactElement, type ComponentProps } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; +import { DraftId, useComposerDraftStore } from "~/composerDraftStore"; + +const { newThread, prepareThread, refresh, Wrapper, Trigger } = vi.hoisted(() => ({ + newThread: vi.fn(), + prepareThread: vi.fn(), + refresh: vi.fn(), + Wrapper: ({ children }: { children?: ReactNode }) => children, + Trigger: ({ children, render }: { children?: ReactNode; render?: ReactElement }) => ( + <> + {render} + {children} + + ), +})); +vi.mock("@effect/atom-react", () => ({ useAtomValue: () => [] })); +vi.mock("~/state/server", () => ({ primaryServerKeybindingsAtom: {} })); +vi.mock("~/state/entities", () => ({ useProjects: () => [], useServerConfigs: () => new Map() })); +vi.mock("~/state/environments", () => ({ + useEnvironments: () => ({ environments: [] }), + usePrimaryEnvironmentId: () => EnvironmentId.make("env-1"), +})); +vi.mock("~/hooks/useSettings", () => ({ + useClientSettings: (select: (settings: typeof DEFAULT_CLIENT_SETTINGS) => unknown) => + select(DEFAULT_CLIENT_SETTINGS), +})); +vi.mock("~/hooks/useLiveRefresh", () => ({ useLiveRefresh: () => {} })); +vi.mock("~/hooks/useHandleNewThread", () => ({ useNewThreadHandler: () => newThread })); +vi.mock("~/lib/sourceControlActions", () => ({ + usePreparePullRequestThreadAction: () => ({ run: prepareThread }), +})); +vi.mock("~/state/use-atom-command", () => ({ useAtomCommand: () => vi.fn() })); +vi.mock("~/state/pullRequests", () => ({ + pullRequestEnvironment: { detail: () => "detail", activity: () => "activity" }, + usePullRequestTurnRefresh: () => 0, + useSharedPullRequestSummary: () => null, +})); +vi.mock("~/state/vcs", () => ({ vcsEnvironment: { listRefs: () => null } })); +vi.mock("~/state/query", () => ({ + useEnvironmentQuery: (query: string) => ({ + data: query === "detail" ? detail : null, + isPending: false, + isSuccess: true, + error: null, + refresh, + }), +})); +vi.mock("~/state/usePullRequestStack", () => ({ + usePullRequestStack: () => ({ + data: null, + isSuccess: true, + isPending: false, + error: null, + refresh, + }), +})); +vi.mock("../ui/toast", () => ({ toastManager: { add: vi.fn(), update: vi.fn() } })); +vi.mock("../ui/tooltip", () => ({ + TooltipProvider: Wrapper, + Tooltip: Wrapper, + TooltipTrigger: Trigger, + TooltipPopup: () => null, +})); +vi.mock("../ui/menu", () => ({ + Menu: Wrapper, + MenuPopup: Wrapper, + MenuTrigger: Trigger, + MenuItem: "button", + MenuRadioGroup: Wrapper, + MenuRadioItem: "button", + MenuSeparator: () => null, + MenuShortcut: () => null, +})); +vi.mock("../ui/alert-dialog", () => ({ + AlertDialog: () => null, + AlertDialogPopup: Wrapper, + AlertDialogHeader: Wrapper, + AlertDialogTitle: Wrapper, + AlertDialogDescription: Wrapper, + AlertDialogFooter: Wrapper, + AlertDialogClose: Wrapper, +})); +vi.mock("./PullRequestMarkdown", () => ({ + PullRequestMarkdownContext: Wrapper, + PullRequestMarkdown: () => null, +})); +vi.mock("~/browser/useOpenLink", () => ({ useOpenLink: () => vi.fn() })); +vi.mock("./PullRequestThreadLinks", () => ({ PullRequestThreadLinks: () => null })); +vi.mock("./PullRequestSummaryTab", () => ({ + PullRequestSummaryTab: ({ + onFixFinding, + }: ComponentProps) => ( + + ), +})); +vi.mock("./PullRequestCodeTab", () => ({ + default: ({ + onAddToAgentSelection, + }: ComponentProps) => ( + + ), +})); + +import { PullRequestDetailPanel } from "./PullRequestDetailPanel"; +import { pullRequestPanelContext } from "./pullRequestDetail.logic"; + +const detail: PullRequestDetailView = { + provider: "github", + projectId: ProjectId.make("project"), + projectTitle: "Project", + workspaceRoot: "/workspace", + repository: "owner/repo", + number: 1, + title: "Test pull request", + body: "Original description", + url: "https://github.com/owner/repo/pull/1", + author: { login: "author", name: null, avatarUrl: null }, + viewer: "author", + state: "open", + isDraft: false, + mergeability: "conflicting", + additions: 1, + deletions: 0, + changedFiles: 1, + headBranch: "feature", + baseBranch: "main", + createdAt: "2026-09-01T00:00:00Z", + updatedAt: "2026-09-01T00:00:00Z", + mergedAt: null, + closedAt: null, + reviewers: [], + labels: [], + checks: [{ name: "Unit tests", status: "success", description: null, url: null }], + comments: [], + commentCount: 0, + commentsTruncated: false, + reviewThreads: [], + commits: [], + mergeCapabilities: { merge: false, squash: false, rebase: false }, + capabilities: { + diff: true, + comment: false, + search: true, + actions: [], + mergeMethods: [], + review: { inlineComment: false, reply: false, resolve: false, verdicts: [] }, + reviewers: { request: false, listCandidates: false }, + edit: { changeRequest: true, comment: false }, + }, + viewerPermissions: { + actions: [], + comment: false, + resolve: false, + verdicts: [], + requestReviewers: false, + }, +}; + +const threadRef: ScopedThreadRef = { + environmentId: EnvironmentId.make("env-1"), + threadId: ThreadId.make("thread-1"), +}; +const draftId = DraftId.make("draft-1"); +const newDraftId = DraftId.make("new-draft"); +let renderer: ReactTestRenderer; + +beforeEach(() => { + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + vi.stubGlobal("window", { addEventListener: vi.fn(), removeEventListener: vi.fn() }); + useComposerDraftStore.setState({ draftsByThreadKey: {} }); + newThread + .mockReset() + .mockResolvedValue({ draftId: newDraftId, threadId: ThreadId.make("new-thread") }); + prepareThread.mockReset().mockResolvedValue({ + _tag: "Success", + value: { branch: "feature", worktreePath: "/workspace/pr" }, + }); +}); +afterEach(() => { + act(() => renderer?.unmount()); + vi.unstubAllGlobals(); +}); + +async function click(label: string) { + const button = renderer.root + .findAllByType("button") + .find( + (node) => + node.props["aria-label"] === label || + node.findAll((child) => child.children.includes(label)).length > 0, + ); + expect(button, label).toBeDefined(); + await act(async () => + button!.props.onClick({ + nativeEvent: new Event("click"), + preventDefault() {}, + stopPropagation() {}, + }), + ); +} + +const actions = [ + "Resolve conflicts", + "Ask a question", + "Explain this PR", + "Fix findings in this thread", + "Fix check", + "Add to agent", +]; + +// The surface ChatView opens for `detail`, and the thread states it can be opened beside. The +// context prop is derived here the way ChatView derives it, so a wrong answer from the thread's +// link list fails these cases rather than only a hand-picked prop. +const surface = { projectId: detail.projectId, repository: detail.repository, number: 1 }; +const link = (number: number, source: ThreadPullRequestLink["source"]): ThreadPullRequestLink => ({ + host: "github.com", + repository: detail.repository, + number, + url: `https://github.com/${detail.repository}/pull/${number}`, + source, + linkedAt: "2026-09-01T00:00:00Z", + snapshot: null, + stack: null, +}); +const stackThread = { + projectId: detail.projectId, + pullRequests: [link(3, "manual"), link(2, "stack"), link(1, "stack")], + // The server's one-slot field names the top layer; the panel shows the bottom one. + linkedPullRequest: { ...surface, number: 3, url: link(3, "manual").url }, +}; +const unrelatedThread = { + projectId: detail.projectId, + pullRequests: [link(9, "created")], + linkedPullRequest: { ...surface, number: 9, url: link(9, "created").url }, +}; + +describe.each([ + ["own PR, a lower layer of the thread's stack", stackThread, threadRef], + ["another PR beside the current thread", unrelatedThread, threadRef], + ["PR beside an unsent draft", null, draftId], + ["standalone PR page", null, undefined], +] as const)("%s", (_name, thread, target) => { + const context = thread ? pullRequestPanelContext(thread, surface) : "page"; + + function render() { + renderer = create( + ({ + terminalFocus: false, + terminalOpen: false, + previewFocus: false, + previewOpen: false, + })} + />, + ); + } + + it(`${thread === stackThread ? "hides" : "offers"} the checkout`, async () => { + await act(async () => render()); + const checkout = renderer.root + .findAllByType("button") + .filter((node) => node.props["aria-label"] === "Check out"); + expect(checkout).toHaveLength(thread === stackThread ? 0 : 1); + }); + + it.each(actions)("%s writes to the correct composer", async (action) => { + if (target) useComposerDraftStore.getState().setPrompt(target, "Keep my draft"); + await act(async () => render()); + if (action === "Add to agent") await click("Code"); + await click(target ? action : action.replace("in this thread", "in a thread")); + const draft = useComposerDraftStore.getState().getComposerDraft(target ?? newDraftId); + if (action === "Resolve conflicts") expect(draft?.prompt).toContain("resolve every conflict"); + else if (action === "Fix check") expect(draft?.prompt).toContain("Fix the failing check"); + else if (action.startsWith("Fix findings")) + expect(draft?.prompt).toContain("Fix the actionable findings"); + else expect(draft?.reviewComments?.length).toBeGreaterThan(0); + if (action === "Add to agent") { + expect(draft?.prompt).toContain("Fix this line"); + expect(draft?.reviewComments).toEqual( + expect.arrayContaining([expect.objectContaining({ id: "note-1", diff: "+broken()" })]), + ); + } + if (target) { + expect(draft?.prompt).toContain("Keep my draft"); + expect(newThread).not.toHaveBeenCalled(); + expect(prepareThread).not.toHaveBeenCalled(); + } else { + expect(newThread).toHaveBeenCalled(); + } + }); +}); diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index b5b5fa2e4a3c5490de0359b85de68284bc5f04fd..5a2afa9cdaeb9e6aa8b2e0879a58fd9bbf828af0 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -138,7 +138,6 @@ import { isStackedPullRequestBase, pullRequestActionMenuHasGroup, pullRequestActionNeedsHostRefresh, - pullRequestComposerTarget, pullRequestCheckoutCommand, pullRequestFindingKey, pullRequestHandoffLabels, @@ -511,10 +510,7 @@ export function PullRequestDetailPanel({ * again is at best a no-op and at worst git refusing a branch two checkouts. */ context?: "page" | "thread"; - /** - * The open thread's composer. Beside the thread whose own pull request this is, hand-offs - * land here instead of opening a new thread — the branch is already under the reader's feet. - */ + /** The open thread's composer. */ composerDraftTarget?: ScopedThreadRef | DraftId; /** * Beside a thread, the way back to that thread's list of pull requests. The tab strip can @@ -1064,10 +1060,7 @@ export function PullRequestDetailPanel({ reviewComments?: ReadonlyArray; }; - // Beside the thread whose own pull request this is, a task belongs in that thread's composer: - // the branch is already checked out under it, so opening a second thread would only scatter - // the work. - const attachTarget = pullRequestComposerTarget(context, composerDraftTarget); + const attachTarget = composerDraftTarget ?? null; const handoffLabels = pullRequestHandoffLabels(attachTarget !== null); const writeTaskToComposer = (target: ScopedThreadRef | DraftId, task: ThreadTask) => { diff --git a/apps/web/src/components/pullRequest/PullRequestListEmptyState.test.tsx b/apps/web/src/components/pullRequest/PullRequestListEmptyState.test.tsx deleted file mode 100644 index cef54e639d29136b3567f7e44885643bd6f26b01..0000000000000000000000000000000000000000 --- a/apps/web/src/components/pullRequest/PullRequestListEmptyState.test.tsx +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Which of the four states wins, and which of them offer to ask the hosts again. The component is - * called as a plain function and its tree read for text: the elements are walked rather than - * invoked, so the button's own hooks never run outside a render. - */ -import { isValidElement, type ReactElement, type ReactNode } from "react"; -import { describe, expect, it } from "vite-plus/test"; - -import { PullRequestListEmptyState } from "./PullRequestListEmptyState"; - -function textOf(node: ReactNode): string { - if (typeof node === "string" || typeof node === "number") return String(node); - if (Array.isArray(node)) return node.map(textOf).join(" "); - if (!isValidElement(node)) return ""; - return textOf((node as ReactElement<{ children?: ReactNode }>).props.children); -} - -const baseProps = { - query: "", - filtered: false, - searching: false, - hasProjects: true, - canLoadMore: false, - loadingMore: false, - refreshing: false, - onClearQuery: () => {}, - onLoadMore: () => {}, - onRefresh: () => {}, -}; - -function render(props: Partial): string { - return textOf(PullRequestListEmptyState({ ...baseProps, ...props })); -} - -describe("PullRequestListEmptyState", () => { - it("asks for a project ahead of anything a search or a filter could say", () => { - const text = render({ hasProjects: false, searching: true, query: "fix", filtered: true }); - expect(text).toContain("No projects in this workspace"); - expect(text).toContain("Add project"); - }); - - it("leaves the retry off the states where asking again could not change the answer", () => { - expect(render({ hasProjects: false })).not.toContain("Check again"); - expect(render({ searching: true, query: "fix" })).not.toContain("Check again"); - }); - - it("offers the retry once the hosts have answered", () => { - expect(render({})).toContain("Check again"); - expect(render({ filtered: true })).toContain("Check again"); - expect(render({ query: "fix" })).toContain("Check again"); - expect(render({ canLoadMore: true })).toContain("Load more pull requests"); - expect(render({ refreshing: true })).toContain("Checking..."); - }); -}); diff --git a/apps/web/src/components/pullRequest/PullRequestsUnavailableState.test.tsx b/apps/web/src/components/pullRequest/PullRequestsUnavailableState.test.tsx deleted file mode 100644 index 12136adbc632ea93ab5ccfa142420b8de16aade6..0000000000000000000000000000000000000000 --- a/apps/web/src/components/pullRequest/PullRequestsUnavailableState.test.tsx +++ /dev/null @@ -1,73 +0,0 @@ -import { isValidElement, type ReactElement, type ReactNode } from "react"; -import { renderToStaticMarkup } from "react-dom/server"; -import { describe, expect, it } from "vite-plus/test"; - -import { PullRequestsUnavailableState } from "./PullRequestsUnavailableState"; - -function textOf(node: ReactNode): string { - if (typeof node === "string" || typeof node === "number") return String(node); - if (Array.isArray(node)) return node.map(textOf).join(" "); - if (!isValidElement(node)) return ""; - return textOf((node as ReactElement<{ children?: ReactNode }>).props.children); -} - -describe("PullRequestsUnavailableState", () => { - it("can explain an unsupported environment without offering a futile retry", () => { - const text = textOf( - PullRequestsUnavailableState({ - title: "Pull requests unavailable", - error: "Update this environment's T3 Code server to browse pull requests.", - }), - ); - - expect(text).toContain("Pull requests unavailable"); - expect(text).toContain("Update this environment's T3 Code server"); - expect(text).not.toContain("Retry"); - }); - - it("retains the retry for transient load failures", () => { - const html = renderToStaticMarkup( - {}} - gitHubUrl="https://github.com/pingdotgg/t3code/pull/42" - />, - ); - - expect(html).toContain("Retry"); - expect(html).toContain("Open on GitHub"); - expect(html).toContain('href="https://github.com/pingdotgg/t3code/pull/42"'); - expect(html).toContain('target="_blank"'); - expect(html).toContain('rel="noopener noreferrer"'); - }); - - it("can offer the browser without offering a retry", () => { - const html = renderToStaticMarkup( - , - ); - - expect(html).toContain("Open on GitHub"); - expect(html).not.toContain("Retry"); - }); - - it("can offer a retry without offering GitHub", () => { - const html = renderToStaticMarkup( - {}} />, - ); - - expect(html).toContain("Retry"); - expect(html).not.toContain("Open on GitHub"); - }); - - it("renders no action content without a retry or browser target", () => { - const html = renderToStaticMarkup( - , - ); - - expect(html).not.toContain('data-slot="empty-content"'); - expect(html).not.toContain("href="); - }); -}); diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 96133395f0484c1b24436e54b8f08e95790cf424..5e75851f08ad617d8772f57040c5940b411a1fc9 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -8,6 +8,7 @@ import { type PullRequestDetail, type PullRequestDetailView, type PullRequestReviewThread, + type ThreadPullRequestLink, } from "@t3tools/contracts"; import { describe, expect, it } from "vite-plus/test"; import { formatInlineContextReference } from "~/lib/composerContextReferences"; @@ -26,7 +27,7 @@ import { stripPullRequestHandoffReferences, isPullRequestVerdictStale, isStackedPullRequestBase, - isThreadOwnPullRequest, + pullRequestPanelContext, latestPullRequestReviewOutcomes, newestPullRequestCommitAt, mergePullRequestThreadComments, @@ -34,9 +35,7 @@ import { pullRequestActionMenuHasGroup, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, - pullRequestComposerTarget, pullRequestFindingKey, - pullRequestHandoffLabels, pullRequestReviewOutcome, readableFailure, readPullRequestDetailSnapshot, @@ -250,33 +249,6 @@ describe("pull request primary control", () => { }); }); -describe("pull request handoff labels", () => { - it("names the open thread when actions write to its composer", () => { - expect(pullRequestHandoffLabels(true)).toEqual({ - fixFinding: "Fix in this thread", - fixCheck: "Fix in this thread", - fixFindings: "Fix findings in this thread", - }); - }); - - it("keeps the standalone pull request page labels", () => { - expect(pullRequestHandoffLabels(false)).toEqual({ - fixFinding: "Fix in a thread", - fixCheck: "Fix", - fixFindings: "Fix findings in a thread", - }); - }); -}); - -describe("pull request composer target", () => { - it("rejects a page composer so agent comments cannot open another thread", () => { - const target = { environmentId: "env-1", threadId: "thread-1" }; - - expect(pullRequestComposerTarget("page", target)).toBeNull(); - expect(pullRequestComposerTarget("thread", target)).toBe(target); - }); -}); - describe("stacked pull request classification", () => { it("requires a known default branch", () => { expect(isStackedPullRequestBase("main", [{ name: "main", isDefault: false }])).toBe(false); @@ -1373,40 +1345,124 @@ describe("how the branch stands against its base", () => { }); }); -describe("whether the panel is showing the thread's own pull request", () => { - const surface = { projectId: "proj-a", repository: "acme/app", number: 7 }; +describe("pull request panel context beside a thread", () => { + // Shapes copied from real threads: a thread that opened a stack holds the top layer as a + // manual link and every lower layer as a "stack" link, with the legacy field pointing at + // whichever one the server chose. Snapshots are null until the sync reactor's first pass. + const link = ( + number: number, + overrides: Partial = {}, + ): ThreadPullRequestLink => ({ + host: "github.com", + repository: "pingdotgg/t3code", + number, + url: `https://github.com/pingdotgg/t3code/pull/${number}`, + source: "manual", + linkedAt: "2026-09-09T00:00:00Z", + snapshot: null, + stack: null, + ...overrides, + }); + const surface = ( + number: number, + overrides: Partial[1]> = {}, + ) => ({ + projectId: "proj-a", + host: "github.com", + repository: "pingdotgg/t3code", + number, + ...overrides, + }); + const stackThread = { + projectId: "proj-a", + pullRequests: [ + link(10856), + link(10832, { source: "stack" }), + link(10677, { source: "stack" }), + link(10854, { source: "stack" }), + link(10855, { source: "stack" }), + ], + linkedPullRequest: { + projectId: "proj-a", + repository: "pingdotgg/t3code", + number: 10856, + url: "https://github.com/pingdotgg/t3code/pull/10856", + }, + }; - it("matches on project, repository and number together", () => { - expect( - isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/app", number: 7 }, surface), - ).toBe(true); + it("treats every layer of the thread's stack as its own, not only the one the legacy field names", () => { + for (const number of [10856, 10832, 10677, 10854, 10855]) { + expect(pullRequestPanelContext(stackThread, surface(number)), `#${number}`).toBe("thread"); + } }); - it("rejects a second checkout of the same repository under another project", () => { - expect( - isThreadOwnPullRequest({ projectId: "proj-b", repository: "acme/app", number: 7 }, surface), - ).toBe(false); + it("does not let the legacy field decide when the thread holds a link list", () => { + // Every prior regression flipped here: a server-side change to which link the legacy field + // resolves to must not turn the thread's own second link into a checkout-able stranger. + const thread = { + projectId: "proj-a", + pullRequests: [link(11101, { source: "created" }), link(11105, { source: "stack" })], + linkedPullRequest: { + projectId: "proj-a", + repository: "pingdotgg/t3code", + number: 11105, + url: "https://github.com/pingdotgg/t3code/pull/11105", + }, + }; + expect(pullRequestPanelContext(thread, surface(11101))).toBe("thread"); + expect(pullRequestPanelContext(thread, surface(11105))).toBe("thread"); + expect(pullRequestPanelContext({ ...thread, linkedPullRequest: null }, surface(11101))).toBe( + "thread", + ); }); - it("rejects another repository or another number", () => { - expect( - isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/web", number: 7 }, surface), - ).toBe(false); - expect( - isThreadOwnPullRequest({ projectId: "proj-a", repository: "acme/app", number: 8 }, surface), - ).toBe(false); + it("is the page for a pull request the thread is not linked to", () => { + expect(pullRequestPanelContext(stackThread, surface(12320))).toBe("page"); + expect(pullRequestPanelContext(stackThread, surface(10856, { repository: "acme/web" }))).toBe( + "page", + ); + }); + + it("is the page under another project's checkout of the same repository", () => { + expect(pullRequestPanelContext(stackThread, surface(10856, { projectId: "proj-b" }))).toBe( + "page", + ); + }); + + it("recognizes an unsynced manual link, and matches host and repository case-insensitively", () => { + const thread = { projectId: "proj-a", pullRequests: [link(7, { host: "GitHub.com" })] }; + expect(pullRequestPanelContext(thread, surface(7, { repository: "PingDotGG/T3Code" }))).toBe( + "thread", + ); + expect(pullRequestPanelContext(thread, surface(7, { host: undefined }))).toBe("thread"); + expect(pullRequestPanelContext(thread, surface(7, { host: "gitlab.com" }))).toBe("page"); + }); + + it("ignores a dismissed stack member the reader chose not to see", () => { + const thread = { + projectId: "proj-a", + pullRequests: [link(1), link(2, { source: "stack-dismissed" })], + }; + expect(pullRequestPanelContext(thread, surface(2))).toBe("page"); }); - it("rejects a thread with no project or no pull request of its own", () => { + it("falls back to the legacy fields only for a thread with no link list", () => { + const legacy = { + projectId: "proj-a", + repository: "pingdotgg/t3code", + number: 3, + url: "https://github.com/pingdotgg/t3code/pull/3", + }; expect( - isThreadOwnPullRequest({ projectId: null, repository: "acme/app", number: 7 }, surface), - ).toBe(false); + pullRequestPanelContext({ projectId: "proj-a", linkedPullRequest: legacy }, surface(3)), + ).toBe("thread"); expect( - isThreadOwnPullRequest( - { projectId: "proj-a", repository: "acme/app", number: null }, - surface, - ), - ).toBe(false); + pullRequestPanelContext({ projectId: "proj-a", branchPullRequest: legacy }, surface(3)), + ).toBe("thread"); + expect(pullRequestPanelContext({ projectId: "proj-a", pullRequests: [] }, surface(3))).toBe( + "page", + ); + expect(pullRequestPanelContext({ projectId: null }, surface(3))).toBe("page"); }); }); diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 56e22e643e1e5b5e91d5567582af2cb01cbef761..733e314b770f4fc045d459f195a0fc7d64070846 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -21,8 +21,14 @@ import { type PullRequestState, type PullRequestUpdateMethod, type SourceControlProviderKind, + type ThreadLinkedPullRequest, + type ThreadPullRequestLink, type VcsRef, } from "@t3tools/contracts"; +import { + threadPullRequestKeysEqual, + visibleThreadPullRequests, +} from "@t3tools/shared/threadPullRequests"; import { inferReviewCommentFenceLanguage, type ReviewCommentContext } from "~/reviewCommentContext"; import { reviewCommentContextId } from "~/lib/composerContextRecords"; @@ -163,28 +169,55 @@ export function editPullRequestThreadComment< return comments.map((comment) => (comment.id === commentId ? { ...comment, body } : comment)); } +type LegacyLinkedPullRequest = Pick; + /** - * Whether the pull request on a right-panel surface is the thread's own one. Repository and - * number are not enough: one environment can hold two checkouts of the same repository under + * How the detail panel behaves beside a thread: "thread" for a pull request the thread itself + * is linked to (any layer of its stack), "page" for any other one the reader opened there. + * + * Decided from the thread's full link list, never from the single legacy `linkedPullRequest`: + * that field is one server-chosen link out of many, and a thread's own second link or lower + * stack layer would otherwise be handed a checkout button for a branch it already works on. The + * legacy fields only answer for servers that predate link lists. Repository and number are not + * enough either way: one environment can hold two checkouts of the same repository under * different projects, and the other project's checkout is somebody else's branch. */ -export function isThreadOwnPullRequest( +export function pullRequestPanelContext( thread: { readonly projectId: string | null; - readonly repository: string | null; - readonly number: number | null; + readonly pullRequests?: ReadonlyArray | undefined; + readonly linkedPullRequest?: LegacyLinkedPullRequest | null | undefined; + readonly branchPullRequest?: LegacyLinkedPullRequest | null | undefined; }, surface: { readonly projectId: string; + readonly host?: string | undefined; readonly repository: string; readonly number: number; }, -): boolean { - return ( - thread.projectId === surface.projectId && - thread.repository === surface.repository && - thread.number === surface.number - ); +): "page" | "thread" { + if (thread.projectId !== surface.projectId) return "page"; + const links = visibleThreadPullRequests(thread.pullRequests ?? []); + if (links.length > 0) { + const repository = surface.repository.toLowerCase(); + return links.some((link) => + surface.host !== undefined + ? threadPullRequestKeysEqual(link, { + host: surface.host, + repository: surface.repository, + number: surface.number, + }) + : link.number === surface.number && link.repository.toLowerCase() === repository, + ) + ? "thread" + : "page"; + } + const legacy = thread.linkedPullRequest ?? thread.branchPullRequest ?? null; + return legacy !== null && + legacy.repository === surface.repository && + legacy.number === surface.number + ? "thread" + : "page"; } /** Names where a pull-request task will land, without letting each surface guess independently. */ @@ -202,13 +235,6 @@ export function pullRequestHandoffLabels(inThisThread: boolean) { }; } -export function pullRequestComposerTarget( - context: "page" | "thread", - target: T | null | undefined, -): T | null { - return context === "thread" ? (target ?? null) : null; -} - /** Whether the open pull-request action group contains at least one action. */ export function pullRequestActionMenuHasGroup( showsDraftToggle: boolean, diff --git a/apps/web/src/components/pullRequest/pullRequestLinkContextMenu.test.ts b/apps/web/src/components/pullRequest/pullRequestLinkContextMenu.test.ts deleted file mode 100644 index eb6f47b4c803aa12263d6dac564ec13cdcb31053..0000000000000000000000000000000000000000 --- a/apps/web/src/components/pullRequest/pullRequestLinkContextMenu.test.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { describe, expect, it } from "vite-plus/test"; - -import { openOnHostLabel } from "./pullRequestLinkContextMenu"; - -describe("pull request link context menu", () => { - it("names every host it knows, and says nothing false about one it does not", () => { - expect(openOnHostLabel("github")).toBe("Open on GitHub"); - expect(openOnHostLabel("gitlab")).toBe("Open on GitLab"); - expect(openOnHostLabel("bitbucket")).toBe("Open on Bitbucket"); - expect(openOnHostLabel("azure-devops")).toBe("Open on Azure DevOps"); - expect(openOnHostLabel("something-else")).toBe("Open on host"); - }); -}); diff --git a/apps/web/src/components/pullRequest/pullRequestPresentation.test.tsx b/apps/web/src/components/pullRequest/pullRequestPresentation.test.tsx deleted file mode 100644 index df38d75152cf20b6fab49b70871d2150500909c2..0000000000000000000000000000000000000000 --- a/apps/web/src/components/pullRequest/pullRequestPresentation.test.tsx +++ /dev/null @@ -1,28 +0,0 @@ -import { act } from "react"; -import { create, type ReactTestRenderer } from "react-test-renderer"; -import { afterEach, expect, it, vi } from "vite-plus/test"; - -import { PullRequestActorAvatar } from "./pullRequestPresentation"; - -let renderer: ReactTestRenderer | undefined; - -afterEach(async () => { - await act(async () => renderer?.unmount()); - vi.unstubAllGlobals(); -}); - -it("falls back to the actor initial when a remote avatar fails", async () => { - vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); - await act(async () => { - renderer = create( - , - ); - }); - - await act(async () => renderer!.root.findByType("img").props.onError()); - - expect(renderer!.root.findAllByType("img")).toHaveLength(0); - expect(renderer!.root.findByType("span").children).toEqual(["O"]); -}); -- 2.54.0