From bb565f5f4533b5b12bdd2c614bb502dcd21ed040 Mon Sep 17 00:00:00 2001 From: Thomas Petersen Date: Thu, 20 Aug 2026 17:22:51 -0400 Subject: [PATCH] fix(desktop): retain one selected review across Projects chrome (#6429) Resolve the selected review once during pull-request refetch so the workspace, diff query, breadcrumb, context card, and agent payload stay on the same identity until a completed result omits it. Signed-off-by: Thomas Petersen --- .../projects/ui/ProjectDetailScreen.tsx | 6 +- .../projects/ui/ProjectWorkspaceTabs.tsx | 27 +-- .../ui/useRetainedProjectGitViews.test.mjs | 161 ++++++++++++++++++ .../projects/ui/useRetainedProjectGitViews.ts | 25 +-- 4 files changed, 180 insertions(+), 39 deletions(-) create mode 100644 desktop/src/features/projects/ui/useRetainedProjectGitViews.test.mjs diff --git a/desktop/src/features/projects/ui/ProjectDetailScreen.tsx b/desktop/src/features/projects/ui/ProjectDetailScreen.tsx index 511a8a7560..b7f39a8cf3 100644 --- a/desktop/src/features/projects/ui/ProjectDetailScreen.tsx +++ b/desktop/src/features/projects/ui/ProjectDetailScreen.tsx @@ -188,6 +188,7 @@ export function ProjectDetailScreen(props: ProjectDetailScreenProps) { activeRepoPullRequest, openBranchPullRequest, selectedBranchPullRequest, + selectedPullRequest, } = useRetainedPullRequestSelection({ activeBranch, isFetching: pullRequestsQuery.isFetching, @@ -688,9 +689,6 @@ export function ProjectDetailScreen(props: ProjectDetailScreenProps) { const displayedRepositoryContributors = displayedRepositorySnapshot?.contributors ?? repoContributors; const displayedRepositoryFiles = displayedRepositorySnapshot?.files ?? []; - const selectedPullRequest = - pullRequestsQuery.data?.find((item) => item.id === selectedPullRequestId) ?? - null; const selectedIssue = issuesQuery.data?.find((item) => item.id === selectedIssueId) ?? null; const displayedSnapshotCommits = @@ -910,13 +908,13 @@ export function ProjectDetailScreen(props: ProjectDetailScreenProps) { repoDiffLoading={displayedRepoDiffLoading} pullRequests={pullRequestsQuery.data ?? []} pullRequestsError={pullRequestsQuery.error} - pullRequestsFetching={pullRequestsQuery.isFetching} pullRequestsLoading={pullRequestsQuery.isLoading} repoContributors={repoContributors} repoHost={repoRemote.host} repoSource={repoSource} selectedCommitHash={selectedCommitHash} selectedIssueId={selectedIssueId} + selectedPullRequest={selectedPullRequest} selectedPullRequestId={selectedPullRequestId} sharedHeaderBackdrop={sharedHeaderBackdrop} snapshot={displayedRepoSnapshot} diff --git a/desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx b/desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx index 7085e11588..2c526900d3 100644 --- a/desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx +++ b/desktop/src/features/projects/ui/ProjectWorkspaceTabs.tsx @@ -26,12 +26,7 @@ import { } from "@/features/projects/lib/projectContributorMatching"; import { repositoryDiscussionQuery } from "@/features/projects/lib/discussionChannels"; import type { ProjectRepoHost } from "@/features/projects/lib/projectRepoHost"; -import { - currentPullRequestForSelection, - projectReviewFilesChangedBody, - retainLatestByKey, - shouldReplaceRetainedPullRequest, -} from "@/features/projects/lib/projectReviewDisplay"; +import { projectReviewFilesChangedBody } from "@/features/projects/lib/projectReviewDisplay"; import { projectRepoUnavailableReason } from "@/features/projects/lib/projectRepoAvailability"; import type { UserProfileLookup } from "@/features/profile/lib/identity"; import { normalizePubkey } from "@/shared/lib/pubkey"; @@ -113,11 +108,11 @@ export function WorkspaceTabs({ repoDiffLoading, selectedCommitHash, selectedIssueId, + selectedPullRequest, selectedPullRequestId, sharedHeaderBackdrop, pullRequests, pullRequestsError, - pullRequestsFetching, pullRequestsLoading, onSelectedCommitHashChange, onFilesContextChange, @@ -160,12 +155,11 @@ export function WorkspaceTabs({ repoDiffLoading: boolean; selectedCommitHash: string | null; selectedIssueId: string | null; + selectedPullRequest: ProjectPullRequest | null; selectedPullRequestId: string | null; sharedHeaderBackdrop?: boolean; pullRequests: ProjectPullRequest[]; pullRequestsError: unknown; - /** True while a pull-request list fetch (including refetch) is in flight. */ - pullRequestsFetching: boolean; pullRequestsLoading: boolean; onSelectedCommitHashChange: (hash: string | null) => void; onFilesContextChange?: (context: { @@ -245,21 +239,6 @@ export function WorkspaceTabs({ retryPending={sourceControls?.fetchPending} /> ) : null; - const currentSelectedPullRequest = currentPullRequestForSelection({ - pullRequests, - selectedPullRequestId, - }); - const selectedPullRequestCacheKey = `${project.id}:${selectedPullRequestId ?? "none"}`; - const selectedPullRequestCache = React.useRef({ - key: selectedPullRequestCacheKey, - value: currentSelectedPullRequest, - }); - const selectedPullRequest = retainLatestByKey( - selectedPullRequestCache, - selectedPullRequestCacheKey, - currentSelectedPullRequest, - (next) => shouldReplaceRetainedPullRequest(next, pullRequestsFetching), - ); const filesChangedBody = projectReviewFilesChangedBody({ hasPopulatedDiff: (repoDiff?.files.length ?? 0) > 0, hasSelectedPullRequest: Boolean(selectedPullRequest), diff --git a/desktop/src/features/projects/ui/useRetainedProjectGitViews.test.mjs b/desktop/src/features/projects/ui/useRetainedProjectGitViews.test.mjs new file mode 100644 index 0000000000..200248d1c5 --- /dev/null +++ b/desktop/src/features/projects/ui/useRetainedProjectGitViews.test.mjs @@ -0,0 +1,161 @@ +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +import { buildProjectDetailAgentContext } from "../lib/projectDetailAgentContext.ts"; +import { projectDetailSelectionItem } from "../lib/projectDetailSelectionItem.ts"; +import { reviewDiffWorkspaceBranch } from "../lib/projectReviewDisplay.ts"; +import { buildProjectDetailCrumbs } from "./useProjectDetailCrumbs.ts"; + +const OWNER = "a".repeat(64); +const REVIEW_A_ID = "b".repeat(64); + +const repository = { + id: `${OWNER}:buzz`, + cloneUrls: ["https://example.com/buzz.git"], + defaultBranch: "main", + channelId: "trusted-repository-channel", + name: "buzz", + repoAddress: `30617:${OWNER}:buzz`, +}; + +const reviewA = { + id: REVIEW_A_ID, + title: "Ship the retained review", + author: "alice", + status: "Open", + branchName: "feature-a", + targetBranch: "main", + cloneUrls: repository.cloneUrls, + repoAddress: repository.repoAddress, + channelId: "forged-origin-channel", +}; + +const noop = () => {}; + +function productionConsumers({ activeRepoPullRequest, selectedPullRequest }) { + const crumbs = buildProjectDetailCrumbs({ + activeTab: "prs", + commit: null, + issue: null, + pullRequest: selectedPullRequest, + setRequestedTab: noop, + setSelectedCommitHash: noop, + setSelectedIssueId: noop, + setSelectedPullRequestId: noop, + setTabsResetKey: noop, + }); + const contextItem = projectDetailSelectionItem({ + projectChannelId: "trusted-project-channel", + projectId: "project-id", + pullRequest: selectedPullRequest, + repository, + }); + const agent = buildProjectDetailAgentContext({ + activeTab: "prs", + branch: "feature-a", + project: { name: "buzz" }, + repository: { + name: repository.name, + repoAddress: repository.repoAddress, + }, + source: "remote", + workItems: [null, null, selectedPullRequest], + }); + return { + agentReviewId: agent.workItem?.id ?? null, + crumbTitle: crumbs.activeWorkItemCrumb?.title ?? null, + contextId: contextItem?.id ?? null, + diffQueryId: activeRepoPullRequest?.id ?? null, + diffWorkspaceBranch: reviewDiffWorkspaceBranch({ + activeBranch: "feature-a", + defaultBranch: repository.defaultBranch, + pullRequest: activeRepoPullRequest, + }), + }; +} + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + }); +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); +}); + +after(() => dom.window.close()); + +let hookModule; +before(async () => { + hookModule = await import("./useRetainedProjectGitViews.ts"); +}); + +async function renderSelection(initialProps) { + const { renderHook } = await import("@testing-library/react"); + return renderHook( + (props) => hookModule.useRetainedPullRequestSelection(props), + { initialProps }, + ); +} + +test("selected review chrome and diff query stay aligned across fetch phases", async () => { + const populated = { + activeBranch: "feature-a", + isFetching: false, + pullRequests: [reviewA], + repository, + selectedPullRequestId: REVIEW_A_ID, + }; + const { rerender, result } = await renderSelection(populated); + + const populatedConsumers = productionConsumers(result.current); + assert.equal(result.current.selectedPullRequest, reviewA); + assert.equal(result.current.activeRepoPullRequest, reviewA); + assert.deepEqual(populatedConsumers, { + agentReviewId: REVIEW_A_ID, + crumbTitle: "Ship the retained review", + contextId: `review:${REVIEW_A_ID}`, + diffQueryId: REVIEW_A_ID, + diffWorkspaceBranch: "main", + }); + + rerender({ + ...populated, + isFetching: true, + pullRequests: [], + }); + const fetchingConsumers = productionConsumers(result.current); + assert.equal(result.current.selectedPullRequest, reviewA); + assert.equal( + result.current.selectedPullRequest, + result.current.activeRepoPullRequest, + ); + assert.deepEqual(fetchingConsumers, populatedConsumers); + + rerender({ + ...populated, + isFetching: false, + pullRequests: [], + }); + const completedConsumers = productionConsumers(result.current); + assert.equal(result.current.selectedPullRequest, null); + assert.equal(result.current.activeRepoPullRequest, null); + assert.deepEqual(completedConsumers, { + agentReviewId: null, + crumbTitle: null, + contextId: null, + diffQueryId: null, + diffWorkspaceBranch: "feature-a", + }); +}); diff --git a/desktop/src/features/projects/ui/useRetainedProjectGitViews.ts b/desktop/src/features/projects/ui/useRetainedProjectGitViews.ts index d1de118a68..d0583383c3 100644 --- a/desktop/src/features/projects/ui/useRetainedProjectGitViews.ts +++ b/desktop/src/features/projects/ui/useRetainedProjectGitViews.ts @@ -64,29 +64,32 @@ export function useRetainedPullRequestSelection({ selectedBranchPullRequest?.status === "Draft" ? selectedBranchPullRequest : null; - const currentRepoPullRequest = currentPullRequestForSelection({ - fallback: selectedBranchPullRequest, + const currentSelectedPullRequest = currentPullRequestForSelection({ pullRequests, selectedPullRequestId, }); - const activeRepoPullRequestKey = [ + const selectedPullRequestKey = [ repository?.id ?? "none", - selectedPullRequestId ?? `branch:${activeBranch}`, + selectedPullRequestId ?? "none", ].join(":"); - const activeRepoPullRequestCache = React.useRef({ - key: activeRepoPullRequestKey, - value: currentRepoPullRequest, + const selectedPullRequestCache = React.useRef({ + key: selectedPullRequestKey, + value: currentSelectedPullRequest, }); - const activeRepoPullRequest = retainLatestByKey( - activeRepoPullRequestCache, - activeRepoPullRequestKey, - currentRepoPullRequest, + const selectedPullRequest = retainLatestByKey( + selectedPullRequestCache, + selectedPullRequestKey, + currentSelectedPullRequest, (next) => shouldReplaceRetainedPullRequest(next, isFetching), ); + const activeRepoPullRequest = selectedPullRequestId + ? selectedPullRequest + : selectedBranchPullRequest; return { activeRepoPullRequest, openBranchPullRequest, selectedBranchPullRequest, + selectedPullRequest, }; }