From 5967054971627b9879c802af0d1d87a3a037e458 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:32:01 -0400 Subject: [PATCH 1/5] chat PR pane: never auto-open; explicit open persists until closed Removes the webhook-driven auto-pop of the floating PR pane on both the ADE chat surface (AgentChatPane) and the CLI session surface (WorkViewArea). The pane now opens only via an explicit toggle and, because per-chat companion state persists as before, an opened pane stays open across chat switches and restarts until closed. - useChatPrAutoPop -> useChatPrPaneOpen: drops the prs.onEvent subscription and the getForLane baseline seed; keeps the hydrate/ persist effects and their stale-flush guard intact. - ChatPrPane: removes delta detection helpers, the transient delta banner, FieldPulse flashes, and their now-unused imports. - Tests: persistence suite kept; pop/delta suites removed; WorkViewArea pin-routing assertions moved onto the pane props. --- .../components/chat/AgentChatPane.tsx | 22 +- .../components/chat/ChatPrPane.test.tsx | 61 +---- .../renderer/components/chat/ChatPrPane.tsx | 197 ++------------ .../components/chat/chatCompanionUiState.ts | 2 +- .../components/chat/useChatPrAutoPop.test.tsx | 241 ------------------ .../components/chat/useChatPrAutoPop.ts | 122 --------- .../chat/useChatPrPaneOpen.test.tsx | 118 +++++++++ .../components/chat/useChatPrPaneOpen.ts | 61 +++++ .../terminals/WorkViewArea.test.tsx | 30 +-- .../components/terminals/WorkViewArea.tsx | 15 +- 10 files changed, 234 insertions(+), 635 deletions(-) delete mode 100644 apps/desktop/src/renderer/components/chat/useChatPrAutoPop.test.tsx delete mode 100644 apps/desktop/src/renderer/components/chat/useChatPrAutoPop.ts create mode 100644 apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx create mode 100644 apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.ts diff --git a/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx b/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx index 431c0d9dc7..5162fd2275 100644 --- a/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx +++ b/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx @@ -205,7 +205,7 @@ import { ChatActionsDrawerPanel, type ChatActionsTab } from "./ChatActionsDrawer import { ChatSourcesPanel } from "./ChatSourcesPanel"; import { CrossMachineHandoffModal } from "./CrossMachineHandoffModal"; import { ChatPrPane } from "./ChatPrPane"; -import { useChatPrAutoPop } from "./useChatPrAutoPop"; +import { useChatPrPaneOpen } from "./useChatPrPaneOpen"; import { patchChatCompanionUiState, readChatCompanionUiState, @@ -3994,16 +3994,11 @@ export function AgentChatPane({ // a chat on another machine reports paths on that machine's disk, so opening // one has to ask that machine — not whichever machine this tab is bound to. const chatWorkspacePaths = useWorkspacePathOpener({ laneId, navigate, runtimePin: chatRuntimePin }); - // Left PR floating pane (ADE chats only). Auto-pops on webhook-driven PR - // changes; shared with the CLI session surface via useChatPrAutoPop. - // `persistKey` makes open/closed per chat and durable across restarts. - // Declared HERE, below `chatRuntimePin`, because a chat on another machine - // must read its PR from that machine — the pane and its auto-pop take the - // same pin every other call this chat makes already takes. - const { prPaneOpen, setPrPaneOpen, prPaneDelta } = useChatPrAutoPop(laneId, { - persistKey: companionStateKey, - runtimePin: chatRuntimePin, - }); + // Left PR floating pane (ADE chats only). It never auto-opens — only an + // explicit toggle moves it; shared with the CLI session surface via + // useChatPrPaneOpen. `persistKey` makes open/closed per chat and durable + // across restarts: open it once for a chat and it stays open until closed. + const { prPaneOpen, setPrPaneOpen } = useChatPrPaneOpen(companionStateKey); const renderedSession = useMemo( () => ( renderedSessionId @@ -4108,7 +4103,7 @@ export function AgentChatPane({ companionHydrationKeyRef.current = null; return; } - // `prPaneOpen` is owned by useChatPrAutoPop's own persist effect; the patch + // `prPaneOpen` is owned by useChatPrPaneOpen's own persist effect; the patch // helper does the read-merge-write, so a drawer toggle can't clobber it. patchChatCompanionUiState(companionStateKey, { chatActionsOpen, @@ -13468,7 +13463,7 @@ export function AgentChatPane({ // Gate the reserve on the surface that actually renders those panes. Both the // PR pane and the chat-actions pane live in the `selectedSessionId` branch // below; the empty/draft surface renders neither. `prPaneOpen` is persisted - // per lane by `useChatPrAutoPop`, so without this gate a lane that once had + // per chat by `useChatPrPaneOpen`, so without this gate a lane that once had // the PR pane open pays a 276px left gutter on the new-chat screen — shoving // the hero composer sideways to clear a pane that is not on screen. const sessionSurfaceMounted = Boolean(selectedSessionId); @@ -13976,7 +13971,6 @@ export function AgentChatPane({ branchName={laneGitBranch} sessionTitle={selectedSession?.title ?? null} sessionId={renderedSessionId} - delta={prPaneDelta} onClose={() => setPrPaneOpen(false)} runtimePin={chatRuntimePin} />, diff --git a/apps/desktop/src/renderer/components/chat/ChatPrPane.test.tsx b/apps/desktop/src/renderer/components/chat/ChatPrPane.test.tsx index 234396f0ea..2b5e7620ee 100644 --- a/apps/desktop/src/renderer/components/chat/ChatPrPane.test.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatPrPane.test.tsx @@ -29,12 +29,7 @@ vi.mock("framer-motion", () => { }; }); -import { - ChatPrPane, - chatPrSignature, - detectChatPrDelta, - type ChatPrSignature, -} from "./ChatPrPane"; +import { ChatPrPane } from "./ChatPrPane"; import type { PrCheck, PrEventPayload, PrReview, PrStatus, PrSummary } from "../../../shared/types"; import { clearPrReadInFlightForTest } from "../../lib/prReadCache"; import { useAppStore } from "../../state/appStore"; @@ -68,13 +63,6 @@ function makePr(over: Partial = {}): PrSummary { }; } -const sig = (over: Partial = {}): ChatPrSignature => ({ - exists: true, - state: "open", - headSha: "aaa111", - ...over, -}); - const originalAde = (globalThis.window as { ade?: unknown }).ade; function installAde(over?: { @@ -177,45 +165,6 @@ afterEach(() => { vi.clearAllMocks(); }); -describe("detectChatPrDelta", () => { - it("pops for a newly created / first-linked PR", () => { - expect(detectChatPrDelta(sig({ exists: false, state: null, headSha: null }), makePr())?.kind).toBe("created"); - }); - - it("pops for lifecycle transitions", () => { - expect(detectChatPrDelta(sig(), makePr({ state: "merged" }))?.kind).toBe("merged"); - expect(detectChatPrDelta(sig(), makePr({ state: "closed" }))?.kind).toBe("closed"); - expect(detectChatPrDelta(sig({ state: "draft" }), makePr({ state: "open" }))?.kind).toBe("ready"); - expect(detectChatPrDelta(sig({ state: "closed" }), makePr({ state: "open" }))?.kind).toBe("reopened"); - expect(detectChatPrDelta(sig(), makePr({ state: "draft" }))?.kind).toBe("draft"); - }); - - it("pops for a new commit (head sha change)", () => { - expect(detectChatPrDelta(sig({ headSha: "aaa111" }), makePr({ headSha: "bbb222" }))?.kind).toBe("commit"); - }); - - it("does NOT pop for checks/review-only changes", () => { - // Same state + same head sha, only checks/review changed. - const next = makePr({ checksStatus: "failing", reviewStatus: "changes_requested" }); - expect(detectChatPrDelta(sig(), next)).toBeNull(); - }); - - it("does NOT pop when the PR was unlinked / removed", () => { - expect(detectChatPrDelta(sig(), null)).toBeNull(); - }); -}); - -describe("chatPrSignature", () => { - it("captures existence, state, and head sha", () => { - expect(chatPrSignature(makePr({ state: "merged", headSha: "zzz" }))).toEqual({ - exists: true, - state: "merged", - headSha: "zzz", - }); - expect(chatPrSignature(null)).toEqual({ exists: false, state: null, headSha: null }); - }); -}); - describe("ChatPrPane", () => { it("refreshes a linked PR on mount and hides stale running checks once merged", async () => { const stalePr = makePr({ @@ -390,14 +339,6 @@ describe("ChatPrPane", () => { expect(screen.getByText("−38")).toBeTruthy(); }); - it("shows the delta line describing what just changed", async () => { - installAde(); - renderPane({ delta: { kind: "merged", label: "Merged", tone: "good", nonce: 1 } }); - // Title still says the PR is open; "Merged" here is the delta line. - expect(await screen.findByText("Merged")).toBeTruthy(); - expect(screen.getByText("just now")).toBeTruthy(); - }); - it("renders enriched check counts from getChecks", async () => { installAde({ getChecks: [ diff --git a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx index 972e08edad..8571767359 100644 --- a/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx +++ b/apps/desktop/src/renderer/components/chat/ChatPrPane.tsx @@ -1,8 +1,6 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useNavigate } from "react-router-dom"; -import { AnimatePresence, motion } from "framer-motion"; import { - ArrowRight, ArrowsClockwise, ArrowSquareOut, CheckCircle, @@ -11,7 +9,6 @@ import { Check, GithubLogo, GitPullRequest, - Lightning, MinusCircle, Sparkle, X, @@ -19,7 +16,7 @@ import { } from "@phosphor-icons/react"; import { cn } from "../ui/cn"; import { useCopyToClipboard } from "../../hooks/useCopyToClipboard"; -import type { OpenProjectBinding, PrCheck, PrReview, PrState, PrStatus, PrSummary } from "../../../shared/types"; +import type { OpenProjectBinding, PrCheck, PrReview, PrStatus, PrSummary } from "../../../shared/types"; import { formatPrBadgeLabel } from "../prs/shared/prFormatters"; import { PrUserAvatar } from "../prs/shared/PrUserAvatar"; import { ChatPrInlineCreator } from "./ChatPrInlineCreator"; @@ -41,95 +38,15 @@ import { NO_CI_REASON } from "../../../shared/prChecksRollup"; * Live data flow: the GitHub webhook relay lands events in the main process, * which fires `prs-updated`; we re-read the lane's summary and hot-refresh the * enriched detail (checks / reviews / merge status) immediately rather than - * waiting for the next background polling tick. The parent (AgentChatPane) owns - * the auto-pop decision and hands us a `delta` describing what just changed. + * waiting for the next background polling tick. */ -// --------------------------------------------------------------------------- -// Delta detection — shared with AgentChatPane, which owns the auto-pop. -// --------------------------------------------------------------------------- - -export type ChatPrDeltaKind = - | "created" - | "merged" - | "closed" - | "reopened" - | "ready" - | "draft" - | "commit"; - -export type ChatPrDeltaTone = "good" | "bad" | "warn" | "info"; - -export type ChatPrDelta = { - kind: ChatPrDeltaKind; - label: string; - tone: ChatPrDeltaTone; - /** Bumped each time a new delta fires so the pane restarts its fade timer. */ - nonce: number; -}; - -export type ChatPrSignature = { - exists: boolean; - state: PrState | null; - headSha: string | null; -}; - -export function chatPrSignature(pr: PrSummary | null): ChatPrSignature { - return { exists: Boolean(pr), state: pr?.state ?? null, headSha: pr?.headSha ?? null }; -} - -/** - * Returns a pop-worthy delta when the transition from `prev` → `next` warrants - * an auto-pop: a newly created / linked PR, a lifecycle change (merged / - * closed / reopened / ready / draft), or a new commit push. Check and review - * changes intentionally return null — they update in the panel but must not - * pop it. Returns null when nothing pop-worthy changed. - */ -export function detectChatPrDelta( - prev: ChatPrSignature, - next: PrSummary | null, -): Omit | null { - if (!next) return null; // PR removed / unlinked — never pop. - if (!prev.exists) { - return { kind: "created", label: "Pull request opened", tone: "good" }; - } - if (prev.state !== next.state) { - switch (next.state) { - case "merged": - return { kind: "merged", label: "Merged", tone: "good" }; - case "closed": - return { kind: "closed", label: "Closed", tone: "bad" }; - case "draft": - return { kind: "draft", label: "Converted to draft", tone: "warn" }; - case "open": - return prev.state === "draft" - ? { kind: "ready", label: "Marked ready for review", tone: "good" } - : { kind: "reopened", label: "Reopened", tone: "info" }; - default: - return { kind: "reopened", label: "Updated", tone: "info" }; - } - } - if (prev.headSha && next.headSha && prev.headSha !== next.headSha) { - return { kind: "commit", label: "New commit pushed", tone: "info" }; - } - return null; -} - -const DELTA_VISIBLE_MS = 4200; - const titleBarIconButton = "inline-flex h-6 w-6 shrink-0 items-center justify-center rounded-md text-fg/45 transition-colors hover:bg-white/[0.06] hover:text-fg/85 disabled:pointer-events-none disabled:opacity-40"; const paneAction = "inline-flex w-full items-center gap-2 rounded-lg border border-white/[0.06] bg-white/[0.02] px-2.5 py-1.5 text-left text-[12px] font-medium text-fg/65 transition-colors hover:border-white/[0.10] hover:bg-white/[0.04] hover:text-fg/85"; -const deltaToneClass: Record = { - good: "text-emerald-300/90", - bad: "text-red-300/90", - warn: "text-amber-300/90", - info: "text-sky-300/90", -}; - function stateTone(state: PrSummary["state"]): { dot: string; label: string } { switch (state) { case "open": return { dot: "bg-emerald-400", label: "Open" }; @@ -250,30 +167,12 @@ function isMergeReady(pr: PrSummary, status: PrStatus | null): boolean { ); } -/** One-shot highlight flash replayed whenever `nonce` changes; plain otherwise. */ -function FieldPulse({ nonce, active, children }: { nonce: number; active: boolean; children: React.ReactNode }) { - if (!active) return <>{children}; - return ( - - {children} - - ); -} - function PrDetails({ pr, checks, reviews, status, relay, - delta, - deltaVisible, copied, onOpenAde, onOpenGitHub, @@ -284,8 +183,6 @@ function PrDetails({ reviews: PrReview[] | null; status: PrStatus | null; relay: RelayState; - delta: ChatPrDelta | null; - deltaVisible: boolean; copied: boolean; onOpenAde: () => void; onOpenGitHub: () => void; @@ -302,46 +199,18 @@ function PrDetails({ : null; const reviewInfo = reviewView(reviews, pr.reviewStatus); const mergeReady = isMergeReady(pr, status); - const pulseHeader = deltaVisible && Boolean(delta) && delta!.kind !== "commit"; - const pulseChecks = deltaVisible && delta?.kind === "commit"; - const nonce = delta?.nonce ?? 0; return (
- -
- - {tone.label} - {formatPrBadgeLabel(pr)} - - - {live.label} - -
- - - - {delta && deltaVisible ? ( - - - {delta.label} - just now - - - ) : null} - +
+ + {tone.label} + {formatPrBadgeLabel(pr)} + + + {live.label} + +

{pr.title}

@@ -362,22 +231,20 @@ function PrDetails({
) : null} - -
- {checksInfo ? ( - - {checksInfo.icon} - {checksInfo.text} - - ) : null} - {pr.additions > 0 || pr.deletions > 0 ? ( - - +{pr.additions} - −{pr.deletions} - - ) : null} -
- +
+ {checksInfo ? ( + + {checksInfo.icon} + {checksInfo.text} + + ) : null} + {pr.additions > 0 || pr.deletions > 0 ? ( + + +{pr.additions} + −{pr.deletions} + + ) : null} +
{reviewInfo ? (
@@ -419,7 +286,6 @@ export const ChatPrPane = React.memo(function ChatPrPane({ branchName, sessionTitle = null, sessionId = null, - delta = null, onClose, runtimePin = null, }: { @@ -433,8 +299,6 @@ export const ChatPrPane = React.memo(function ChatPrPane({ sessionTitle?: string | null; /** The chat whose explicit PR links should be shown first. */ sessionId?: string | null; - /** Describes the PR change that triggered this pane's auto-pop (owned by the parent). */ - delta?: ChatPrDelta | null; /** Closes the pane — wired to the title bar's ✕ (the header PR pill also toggles it). */ onClose?: () => void; /** See `ChatGitToolbar.runtimePin` — the machine this lane's PR row lives on. */ @@ -470,7 +334,6 @@ export const ChatPrPane = React.memo(function ChatPrPane({ const [reviews, setReviews] = useState(null); const [status, setStatus] = useState(null); const [relay, setRelay] = useState(null); - const [deltaVisible, setDeltaVisible] = useState(false); // Manual title-bar ↻ sync in flight. const [syncing, setSyncing] = useState(false); // Backend reconcile-on-focus running (project-scoped); drives the subtle @@ -668,14 +531,6 @@ export const ChatPrPane = React.memo(function ChatPrPane({ return () => { cancelled = true; }; }, [prRepoOwner, prRepoName]); - // Show the delta line for a few seconds after each new delta, then fade it. - useEffect(() => { - if (!delta) { setDeltaVisible(false); return; } - setDeltaVisible(true); - const id = window.setTimeout(() => setDeltaVisible(false), DELTA_VISIBLE_MS); - return () => window.clearTimeout(id); - }, [delta?.nonce, delta]); - // Same rule the sidebar badge follows: a PR id only resolves on the machine // that owns it, so a pinned pane's "Open in ADE" would land on an empty PRs // tab. `openLanePr` sends a foreign PR to GitHub instead. @@ -749,8 +604,6 @@ export const ChatPrPane = React.memo(function ChatPrPane({ reviews={reviews} status={status} relay={relay} - delta={delta} - deltaVisible={deltaVisible} copied={copied} onOpenAde={openInAde} onOpenGitHub={() => void openInGitHub()} diff --git a/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts b/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts index 07361b626d..ee9c84e2c9 100644 --- a/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts +++ b/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts @@ -122,7 +122,7 @@ export function writeChatCompanionUiState(key: string, state: ChatCompanionUiSta * Merge `patch` into the stored record for `key`. * * The namespace has two independent owners — the chat shell's drawer state and - * `useChatPrAutoPop`'s `prPaneOpen` — so a whole-record write from either one + * `useChatPrPaneOpen`'s `prPaneOpen` — so a whole-record write from either one * clobbers the other unless it reads forward first. Doing the read-merge-write * here makes that structural instead of a convention each caller has to honour. */ diff --git a/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.test.tsx b/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.test.tsx deleted file mode 100644 index 7ed0a36c9a..0000000000 --- a/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.test.tsx +++ /dev/null @@ -1,241 +0,0 @@ -/* @vitest-environment jsdom */ - -import { act, renderHook, waitFor } from "@testing-library/react"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; - -import { useChatPrAutoPop } from "./useChatPrAutoPop"; -import { - chatCompanionUiStorageKey, - readChatCompanionUiState, - resetChatCompanionUiStateCacheForTests, - writeChatCompanionUiState, - DEFAULT_CHAT_COMPANION_UI_STATE, -} from "./chatCompanionUiState"; -import type { PrEventPayload, PrSummary } from "../../../shared/types"; - -function makePr(over: Partial = {}): PrSummary { - return { - id: "pr1", - laneId: "lane1", - projectId: "proj", - repoOwner: "o", - repoName: "r", - githubPrNumber: 42, - githubUrl: "https://github.com/o/r/pull/42", - githubNodeId: null, - title: "PR", - state: "open", - baseBranch: "main", - headBranch: "feat", - checksStatus: "none", - reviewStatus: "none", - additions: 1, - deletions: 0, - mergeConflicts: null, - behindBaseBy: null, - headSha: "aaa111", - lastSyncedAt: new Date().toISOString(), - createdAt: "2026-06-30T00:00:00Z", - updatedAt: "rev-1", - creationStrategy: null, - ...over, - }; -} - -const originalAde = (globalThis.window as { ade?: unknown }).ade; -let emit: ((event: PrEventPayload) => void) | null = null; - -function installAde(initial: PrSummary | null) { - emit = null; - (globalThis.window as { ade?: unknown }).ade = { - prs: { - getForLane: vi.fn().mockResolvedValue(initial), - onEvent: vi.fn().mockImplementation((cb: (event: PrEventPayload) => void) => { - emit = cb; - return () => { emit = null; }; - }), - }, - }; -} - -function emitUpdated(prs: PrSummary[]) { - act(() => emit?.({ type: "prs-updated", polledAt: "now", prs })); -} - -beforeEach(() => { - window.localStorage.clear(); - resetChatCompanionUiStateCacheForTests(); -}); - -afterEach(() => { - (globalThis.window as { ade?: unknown }).ade = originalAde; - window.localStorage.clear(); - resetChatCompanionUiStateCacheForTests(); - vi.clearAllMocks(); -}); - -describe("useChatPrAutoPop", () => { - it("starts closed and does not pop for a PR that already existed on open", async () => { - installAde(makePr({ state: "open" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - // Let the silent baseline seed resolve. - await waitFor(() => expect(window.ade.prs.getForLane).toHaveBeenCalled()); - await act(async () => {}); - // Re-emitting the same open PR (e.g. a checks refresh) must not pop. - emitUpdated([makePr({ state: "open", checksStatus: "passing" })]); - expect(result.current.prPaneOpen).toBe(false); - expect(result.current.prPaneDelta).toBeNull(); - }); - - it("pops with a lifecycle delta when the PR merges", async () => { - installAde(makePr({ state: "open" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - await act(async () => {}); - emitUpdated([makePr({ state: "merged" })]); - expect(result.current.prPaneOpen).toBe(true); - expect(result.current.prPaneDelta?.kind).toBe("merged"); - }); - - it("pops for a new commit push (head sha change)", async () => { - installAde(makePr({ state: "open", headSha: "aaa111" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - await act(async () => {}); - emitUpdated([makePr({ state: "open", headSha: "bbb222" })]); - expect(result.current.prPaneOpen).toBe(true); - expect(result.current.prPaneDelta?.kind).toBe("commit"); - }); - - it("re-pops on each qualifying event with a fresh nonce", async () => { - installAde(makePr({ state: "open", headSha: "aaa111" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - await act(async () => {}); - emitUpdated([makePr({ state: "open", headSha: "bbb222" })]); - const first = result.current.prPaneDelta?.nonce; - emitUpdated([makePr({ state: "merged", headSha: "bbb222" })]); - expect(result.current.prPaneDelta?.kind).toBe("merged"); - expect(result.current.prPaneDelta?.nonce).not.toBe(first); - }); - - it("degrades to no-op when the prs bridge is unavailable", () => { - (globalThis.window as { ade?: unknown }).ade = {}; - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - expect(result.current.prPaneOpen).toBe(false); - expect(result.current.prPaneDelta).toBeNull(); - }); -}); - -describe("useChatPrAutoPop persistence", () => { - it("seeds the pane open from stored companion state", () => { - installAde(makePr({ state: "open" })); - writeChatCompanionUiState("chat-1", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); - resetChatCompanionUiStateCacheForTests(); - - const { result } = renderHook(() => useChatPrAutoPop("lane1", { persistKey: "chat-1" })); - // Seeded on the FIRST render — not after an effect — so the pane never - // flashes closed on chat open. - expect(result.current.prPaneOpen).toBe(true); - }); - - it("persists a manual toggle to localStorage", async () => { - installAde(null); - const { result } = renderHook(() => useChatPrAutoPop("lane1", { persistKey: "chat-1" })); - await act(async () => {}); - - act(() => result.current.setPrPaneOpen(true)); - expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(true); - expect(JSON.parse(window.localStorage.getItem(chatCompanionUiStorageKey("chat-1"))!).prPaneOpen).toBe(true); - - act(() => result.current.setPrPaneOpen(false)); - expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(false); - }); - - it("persists the webhook auto-pop too", async () => { - installAde(makePr({ state: "open" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1", { persistKey: "chat-1" })); - await act(async () => {}); - - emitUpdated([makePr({ state: "merged" })]); - - expect(result.current.prPaneOpen).toBe(true); - expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(true); - }); - - it("an already-open PR still does not pop on chat open with a persist key", async () => { - installAde(makePr({ state: "open" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1", { persistKey: "chat-1" })); - await waitFor(() => expect(window.ade.prs.getForLane).toHaveBeenCalled()); - await act(async () => {}); - - emitUpdated([makePr({ state: "open", checksStatus: "passing" })]); - - expect(result.current.prPaneOpen).toBe(false); - expect(result.current.prPaneDelta).toBeNull(); - // The silent baseline seed must not have written an "open" pane either. - expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(false); - }); - - it("re-seeds per chat when the persist key changes", async () => { - installAde(null); - writeChatCompanionUiState("chat-2", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); - const { result, rerender } = renderHook( - ({ key }: { key: string }) => useChatPrAutoPop("lane1", { persistKey: key }), - { initialProps: { key: "chat-1" } }, - ); - await act(async () => {}); - expect(result.current.prPaneOpen).toBe(false); - - rerender({ key: "chat-2" }); - await act(async () => {}); - expect(result.current.prPaneOpen).toBe(true); - // Switching chats must not write chat-1's value into chat-2 (or vice versa). - expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(false); - expect(readChatCompanionUiState("chat-2").prPaneOpen).toBe(true); - }); - - it("never writes the outgoing chat's value into the incoming chat's record", async () => { - // Regression: the hydrate and persist effects share a fiber and flush in - // declaration order, so on the commit where the key changes the persist - // effect still closes over the OUTGOING chat's `prPaneOpen`. Asserting the - // settled value cannot catch that — a corrective render repairs it — so - // watch the writes themselves. - installAde(null); - writeChatCompanionUiState("chat-2", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); - resetChatCompanionUiStateCacheForTests(); - - const chat2Key = chatCompanionUiStorageKey("chat-2"); - const writes: string[] = []; - const originalSetItem = Storage.prototype.setItem; - const setItem = vi - .spyOn(Storage.prototype, "setItem") - .mockImplementation(function (this: Storage, key: string, value: string) { - if (key === chat2Key) writes.push(value); - originalSetItem.call(this, key, value); - }); - - try { - const { rerender } = renderHook( - ({ key }: { key: string }) => useChatPrAutoPop("lane1", { persistKey: key }), - { initialProps: { key: "chat-1" } }, - ); - await act(async () => {}); - rerender({ key: "chat-2" }); - await act(async () => {}); - - // chat-1 was closed; chat-2 is open. No write to chat-2 may carry `false`. - const clobbered = writes.filter((value) => value.includes("\"prPaneOpen\":false")); - expect(clobbered).toEqual([]); - expect(readChatCompanionUiState("chat-2").prPaneOpen).toBe(true); - } finally { - setItem.mockRestore(); - } - }); - - it("keeps working with no persist key", async () => { - installAde(makePr({ state: "open" })); - const { result } = renderHook(() => useChatPrAutoPop("lane1")); - await act(async () => {}); - emitUpdated([makePr({ state: "merged" })]); - expect(result.current.prPaneOpen).toBe(true); - expect(window.localStorage.length).toBe(0); - }); -}); diff --git a/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.ts b/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.ts deleted file mode 100644 index 7005f1e31f..0000000000 --- a/apps/desktop/src/renderer/components/chat/useChatPrAutoPop.ts +++ /dev/null @@ -1,122 +0,0 @@ -import { useEffect, useRef, useState, type Dispatch, type SetStateAction } from "react"; - -import { - chatPrSignature, - detectChatPrDelta, - type ChatPrDelta, - type ChatPrSignature, -} from "./ChatPrPane"; -import { patchChatCompanionUiState, readChatCompanionUiState } from "./chatCompanionUiState"; -import type { OpenProjectBinding } from "../../../shared/types"; - -export type UseChatPrAutoPop = { - prPaneOpen: boolean; - setPrPaneOpen: Dispatch>; - /** The PR change that triggered the most recent auto-pop; handed to ChatPrPane. */ - prPaneDelta: ChatPrDelta | null; -}; - -/** - * Drives the floating PR pane's auto-pop for a lane's linked PR, shared by the - * ADE chat surface (AgentChatPane) and the CLI session surface (WorkViewArea). - * - * On each webhook-driven `prs-updated` it re-reads this lane's PR summary and - * pops the pane for a pop-worthy change — a newly created/linked PR, a lifecycle - * change (merged/closed/reopened/ready/draft), or a new commit push. Checks and - * review changes update the pane but do not pop it (see detectChatPrDelta). The - * baseline is seeded silently so an already-open PR never pops on chat open. - * Re-pops on each qualifying event; rapid bursts collapse into one visible pop - * because the pane only ever shows the latest delta. - * - * `persistKey` (the surface's per-chat companion-state key) makes the pane's - * open/closed state per chat AND durable across restarts: without it the pane - * is bare component state, so every chat switch and every app launch reopens - * from "closed" regardless of what the user left open. - */ -export function useChatPrAutoPop( - laneId: string | null | undefined, - opts?: { persistKey?: string | null; runtimePin?: OpenProjectBinding | null }, -): UseChatPrAutoPop { - const persistKey = opts?.persistKey ?? null; - // The lane's PR lives on the lane's machine, so both the seed read and the - // event feed have to come from there — see `ChatGitToolbar.runtimePin`. Keyed - // on the pin KEY, read through a ref: a local pin is a fresh object on every - // cross-machine merge, and re-subscribing re-anchors the pinned event pump. - const runtimePin = opts?.runtimePin ?? null; - const runtimePinRef = useRef(runtimePin); - runtimePinRef.current = runtimePin; - const runtimePinKey = runtimePin?.key ?? null; - const [prPaneOpen, setPrPaneOpen] = useState( - () => (persistKey ? readChatCompanionUiState(persistKey).prPaneOpen : false), - ); - const [prPaneDelta, setPrPaneDelta] = useState(null); - const prevPrSigRef = useRef(null); // null = not yet seeded - const nonceRef = useRef(0); - // Which key the current `prPaneOpen` was hydrated from. - const hydratedPersistKeyRef = useRef(persistKey); - // Set by the hydrate effect and consumed by the persist effect below. Both - // effects belong to the same fiber and run in declaration order within one - // passive-effect flush, so on the commit where `persistKey` changes the - // persist effect still closes over the OUTGOING chat's `prPaneOpen`. Marking - // the hydration here — rather than relying on the key ref, which the hydrate - // effect has already advanced by then — makes the persist effect skip exactly - // that one stale flush instead of writing chat A's value into chat B's record. - const pendingHydrationKeyRef = useRef(null); - - useEffect(() => { - if (hydratedPersistKeyRef.current === persistKey) return; - hydratedPersistKeyRef.current = persistKey; - pendingHydrationKeyRef.current = persistKey; - setPrPaneOpen(persistKey ? readChatCompanionUiState(persistKey).prPaneOpen : false); - }, [persistKey]); - - // Every transition persists — the toolbar toggle, the ✕, and the webhook - // auto-pop below all land here because they all move the same state. The - // patch merges inside the store, so the chat shell's drawer fields on the - // same record survive. - useEffect(() => { - if (!persistKey || hydratedPersistKeyRef.current !== persistKey) return; - if (pendingHydrationKeyRef.current === persistKey) { - // Stale flush from the key change. If hydration changed the value, the - // corrective render re-runs this effect with the real one; if it didn't, - // storage already agrees and there is nothing to write either way. - pendingHydrationKeyRef.current = null; - return; - } - if (readChatCompanionUiState(persistKey).prPaneOpen === prPaneOpen) return; - patchChatCompanionUiState(persistKey, { prPaneOpen }); - }, [persistKey, prPaneOpen]); - - useEffect(() => { - setPrPaneDelta(null); - prevPrSigRef.current = null; - // The prs bridge can be absent in some surfaces / test harnesses / early - // boot; degrade to "no auto-pop" instead of crashing the host surface. - const prs = window.ade?.prs; - if (!laneId || !prs?.getForLane || !prs?.onEvent) return; - let cancelled = false; - prs - .getForLane(laneId, runtimePinRef.current) - .then((pr) => { - if (!cancelled && prevPrSigRef.current === null) prevPrSigRef.current = chatPrSignature(pr); - }) - .catch(() => {}); - const unsubscribe = prs.onEvent((event) => { - if (event.type !== "prs-updated") return; - const next = event.prs.find((pr) => pr.laneId === laneId) ?? null; - const prev = prevPrSigRef.current; - prevPrSigRef.current = chatPrSignature(next); - if (prev === null) return; // just seeded from this event — no pop - const change = detectChatPrDelta(prev, next); - if (!change) return; - setPrPaneDelta({ ...change, nonce: ++nonceRef.current }); - setPrPaneOpen(true); - }, runtimePinRef.current); - return () => { - cancelled = true; - unsubscribe(); - }; - }, [laneId, runtimePinKey]); - - return { prPaneOpen, setPrPaneOpen, prPaneDelta }; -} diff --git a/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx new file mode 100644 index 0000000000..80fc6ef959 --- /dev/null +++ b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx @@ -0,0 +1,118 @@ +/* @vitest-environment jsdom */ + +import { act, renderHook } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { useChatPrPaneOpen } from "./useChatPrPaneOpen"; +import { + chatCompanionUiStorageKey, + readChatCompanionUiState, + resetChatCompanionUiStateCacheForTests, + writeChatCompanionUiState, + DEFAULT_CHAT_COMPANION_UI_STATE, +} from "./chatCompanionUiState"; + +const originalAde = (globalThis.window as { ade?: unknown }).ade; + +beforeEach(() => { + window.localStorage.clear(); + resetChatCompanionUiStateCacheForTests(); +}); + +afterEach(() => { + (globalThis.window as { ade?: unknown }).ade = originalAde; + window.localStorage.clear(); + resetChatCompanionUiStateCacheForTests(); + vi.clearAllMocks(); +}); + +describe("useChatPrPaneOpen", () => { + it("starts closed", () => { + const { result } = renderHook(() => useChatPrPaneOpen("chat-1")); + expect(result.current.prPaneOpen).toBe(false); + }); + + it("seeds the pane open from stored companion state", () => { + writeChatCompanionUiState("chat-1", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); + resetChatCompanionUiStateCacheForTests(); + + const { result } = renderHook(() => useChatPrPaneOpen("chat-1")); + // Seeded on the FIRST render — not after an effect — so the pane never + // flashes closed on chat open. + expect(result.current.prPaneOpen).toBe(true); + }); + + it("persists a manual toggle to localStorage", () => { + const { result } = renderHook(() => useChatPrPaneOpen("chat-1")); + + act(() => result.current.setPrPaneOpen(true)); + expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(true); + expect(JSON.parse(window.localStorage.getItem(chatCompanionUiStorageKey("chat-1"))!).prPaneOpen).toBe(true); + + act(() => result.current.setPrPaneOpen(false)); + expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(false); + }); + + it("re-seeds per chat when the persist key changes", async () => { + writeChatCompanionUiState("chat-2", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); + const { result, rerender } = renderHook( + ({ key }: { key: string }) => useChatPrPaneOpen(key), + { initialProps: { key: "chat-1" } }, + ); + await act(async () => {}); + expect(result.current.prPaneOpen).toBe(false); + + rerender({ key: "chat-2" }); + await act(async () => {}); + expect(result.current.prPaneOpen).toBe(true); + // Switching chats must not write chat-1's value into chat-2 (or vice versa). + expect(readChatCompanionUiState("chat-1").prPaneOpen).toBe(false); + expect(readChatCompanionUiState("chat-2").prPaneOpen).toBe(true); + }); + + it("never writes the outgoing chat's value into the incoming chat's record", async () => { + // Regression: the hydrate and persist effects share a fiber and flush in + // declaration order, so on the commit where the key changes the persist + // effect still closes over the OUTGOING chat's `prPaneOpen`. Asserting the + // settled value cannot catch that — a corrective render repairs it — so + // watch the writes themselves. + writeChatCompanionUiState("chat-2", { ...DEFAULT_CHAT_COMPANION_UI_STATE, prPaneOpen: true }); + resetChatCompanionUiStateCacheForTests(); + + const chat2Key = chatCompanionUiStorageKey("chat-2"); + const writes: string[] = []; + const originalSetItem = Storage.prototype.setItem; + const setItem = vi + .spyOn(Storage.prototype, "setItem") + .mockImplementation(function (this: Storage, key: string, value: string) { + if (key === chat2Key) writes.push(value); + originalSetItem.call(this, key, value); + }); + + try { + const { rerender } = renderHook( + ({ key }: { key: string }) => useChatPrPaneOpen(key), + { initialProps: { key: "chat-1" } }, + ); + await act(async () => {}); + rerender({ key: "chat-2" }); + await act(async () => {}); + + // chat-1 was closed; chat-2 is open. No write to chat-2 may carry `false`. + const clobbered = writes.filter((value) => value.includes("\"prPaneOpen\":false")); + expect(clobbered).toEqual([]); + expect(readChatCompanionUiState("chat-2").prPaneOpen).toBe(true); + } finally { + setItem.mockRestore(); + } + }); + + it("keeps working with no persist key and writes nothing to storage", async () => { + const { result } = renderHook(() => useChatPrPaneOpen(null)); + await act(async () => {}); + + act(() => result.current.setPrPaneOpen(true)); + expect(result.current.prPaneOpen).toBe(true); + expect(window.localStorage.length).toBe(0); + }); +}); diff --git a/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.ts b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.ts new file mode 100644 index 0000000000..c24f9c3f06 --- /dev/null +++ b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.ts @@ -0,0 +1,61 @@ +import { useEffect, useRef, useState, type Dispatch, type SetStateAction } from "react"; + +import { patchChatCompanionUiState, readChatCompanionUiState } from "./chatCompanionUiState"; + +export type UseChatPrPaneOpen = { + prPaneOpen: boolean; + setPrPaneOpen: Dispatch>; +}; + +/** + * Owns the floating PR pane's open/closed state, shared by the ADE chat + * surface (AgentChatPane) and the CLI session surface (WorkViewArea). + * + * The pane never auto-opens — only an explicit user toggle moves it. Once + * opened it stays open until the user closes it. + * + * `persistKey` (the surface's per-chat companion-state key) makes that + * open/closed state per chat AND durable across restarts: without it the pane + * is bare component state, so every chat switch and every app launch reopens + * from "closed" regardless of what the user left open. + */ +export function useChatPrPaneOpen(persistKey: string | null): UseChatPrPaneOpen { + const [prPaneOpen, setPrPaneOpen] = useState( + () => (persistKey ? readChatCompanionUiState(persistKey).prPaneOpen : false), + ); + // Which key the current `prPaneOpen` was hydrated from. + const hydratedPersistKeyRef = useRef(persistKey); + // Set by the hydrate effect and consumed by the persist effect below. Both + // effects belong to the same fiber and run in declaration order within one + // passive-effect flush, so on the commit where `persistKey` changes the + // persist effect still closes over the OUTGOING chat's `prPaneOpen`. Marking + // the hydration here — rather than relying on the key ref, which the hydrate + // effect has already advanced by then — makes the persist effect skip exactly + // that one stale flush instead of writing chat A's value into chat B's record. + const pendingHydrationKeyRef = useRef(null); + + useEffect(() => { + if (hydratedPersistKeyRef.current === persistKey) return; + hydratedPersistKeyRef.current = persistKey; + pendingHydrationKeyRef.current = persistKey; + setPrPaneOpen(persistKey ? readChatCompanionUiState(persistKey).prPaneOpen : false); + }, [persistKey]); + + // Every transition persists — the toolbar toggle and the ✕ both land here. + // The patch merges inside the store, so the chat shell's drawer fields on + // the same record survive. + useEffect(() => { + if (!persistKey || hydratedPersistKeyRef.current !== persistKey) return; + if (pendingHydrationKeyRef.current === persistKey) { + // Stale flush from the key change. If hydration changed the value, the + // corrective render re-runs this effect with the real one; if it didn't, + // storage already agrees and there is nothing to write either way. + pendingHydrationKeyRef.current = null; + return; + } + if (readChatCompanionUiState(persistKey).prPaneOpen === prPaneOpen) return; + patchChatCompanionUiState(persistKey, { prPaneOpen }); + }, [persistKey, prPaneOpen]); + + return { prPaneOpen, setPrPaneOpen }; +} diff --git a/apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx b/apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx index 7c1a899ab8..448b76ce48 100644 --- a/apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx +++ b/apps/desktop/src/renderer/components/terminals/WorkViewArea.test.tsx @@ -177,8 +177,12 @@ vi.mock("../chat/ChatPrPane", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - ChatPrPane: ({ laneId }: { laneId: string }) => ( -
+ ChatPrPane: ({ laneId, runtimePin }: { laneId: string; runtimePin?: { key: string } | null }) => ( +
), }; }); @@ -562,7 +566,7 @@ describe("WorkViewArea", () => { expect(terminals.map((terminal) => terminal.getAttribute("data-session-id"))).toContain("session-1"); }); - it("pins the PR pane and auto-pop reads to the owning machine for a foreign running CLI", async () => { + it("pins the PR pane reads to the owning machine for a foreign running CLI", async () => { const session = { ...makeRunningSession("session-foreign", "pty-foreign"), toolType: "codex" as const }; const runtimePin = { kind: "remote", @@ -593,18 +597,16 @@ describe("WorkViewArea", () => { const local = within(view.container); // A lane's PR lives in ITS machine's database, so a foreign session gets the - // same PR affordance as a local one — the reads just carry the pin. Before - // this, both the pane and its auto-pop were suppressed outright, which is - // what made a remote session's PR invisible until the tab was rebound. - await waitFor(() => { - expect(prsMocks.getForLane).toHaveBeenCalledWith("lane-1", runtimePin); - }); - expect(prsMocks.onEvent).toHaveBeenCalledWith(expect.any(Function), runtimePin); + // same PR affordance as a local one — the pane is handed the resolved pin + // so its reads carry it. Before this, the pane was suppressed outright, + // which made a remote session's PR invisible until the tab was rebound. fireEvent.click(local.getByRole("button", { name: "Toggle PR pane" })); - expect((await local.findByTestId("chat-pr-pane")).getAttribute("data-lane-id")).toBe("lane-1"); + const pane = await local.findByTestId("chat-pr-pane"); + expect(pane.getAttribute("data-lane-id")).toBe("lane-1"); + expect(pane.getAttribute("data-runtime-pin-key")).toBe("remote:target-b:project-b"); }); - it("keeps PR auto-pop and pane controls enabled for a local running CLI", async () => { + it("keeps PR pane controls enabled for a local running CLI", async () => { const session = { ...makeRunningSession("session-local", "pty-local"), toolType: "codex" as const }; const view = render( { ); const local = within(view.container); - await waitFor(() => { - expect(prsMocks.getForLane).toHaveBeenCalledWith("lane-1", null); - expect(prsMocks.onEvent).toHaveBeenCalledTimes(1); - }); fireEvent.click(local.getByRole("button", { name: "Toggle PR pane" })); expect((await local.findByTestId("chat-pr-pane")).getAttribute("data-lane-id")).toBe("lane-1"); }); diff --git a/apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx b/apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx index 7ffc796314..3758bbee10 100644 --- a/apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx +++ b/apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx @@ -42,7 +42,7 @@ import { resolveModelDescriptorWithRuntimeCatalog, createUnknownModelPlaceholder import { WorkStartSurface } from "./WorkStartSurface"; import { CliSessionWorkSurfaceHeader } from "./CliSessionWorkSurfaceHeader"; import { ChatPrPane } from "../chat/ChatPrPane"; -import { useChatPrAutoPop } from "../chat/useChatPrAutoPop"; +import { useChatPrPaneOpen } from "../chat/useChatPrPaneOpen"; import { isChatToolType, primarySessionLabel, stripTerminalLabelControls, formatToolTypeLabel } from "../../lib/sessions"; import { SmartTooltip } from "../ui/SmartTooltip"; import { cn } from "../ui/cn"; @@ -758,8 +758,9 @@ const CLI_FLOATING_PANE_CARD_CLASS = * overlaid on top of the terminal. The overlay is absolutely positioned inside a * wrapper that sizes the terminal — it never changes the terminal host's box, so * the PTY's ResizeObserver never fires and the running CLI process is not - * re-flowed (no SIGWINCH). The pill in the header toggles it, and it auto-pops on - * webhook-driven PR changes via the same useChatPrAutoPop hook the ADE chat uses. + * re-flowed (no SIGWINCH). The pill in the header toggles it; it never + * auto-opens and shares the persisted open state with the ADE chat pane via the + * same useChatPrPaneOpen hook. */ function CliSessionSurface({ session, @@ -800,11 +801,8 @@ function CliSessionSurface({ // Persist the pane per CLI session so reopening the surface restores it, the // same way the ADE chat pane keys its companion UI state. // PR reads follow the lane's machine now, so a foreign CLI session gets the - // same pill, auto-pop and pane as a local one — the pin just routes them. - const { prPaneOpen, setPrPaneOpen, prPaneDelta } = useChatPrAutoPop(session.laneId, { - persistKey: session.id, - runtimePin, - }); + // same pill and pane as a local one — the pin just routes them. + const { prPaneOpen, setPrPaneOpen } = useChatPrPaneOpen(session.id); const supportsSplit = layoutVariant !== "grid-tile"; const prFloating = prPaneOpen && Boolean(session.laneId) && supportsSplit; return ( @@ -853,7 +851,6 @@ function CliSessionSurface({ setPrPaneOpen(false)} runtimePin={runtimePin} /> From 7b01f40e4f1d73c4f23f2891387e39fafddba270 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Sat, 22 Aug 2026 16:58:38 -0400 Subject: [PATCH 2/5] quality: apply dual-review findings - Gate the chat header PR-pill pane toggle on a selected session so the draft/new-chat surface falls back to the toolbar's inline PR menu instead of persisting open state for a pane that cannot render there. - Drop dead window.ade save/restore + clearAllMocks scaffolding from the renamed hook's test file. - Refresh docs (composer-and-ui.md, pull-requests/README.md) and the companion-state field comment for the no-auto-open behavior. --- .../src/renderer/components/chat/AgentChatPane.tsx | 8 +++++++- .../src/renderer/components/chat/chatCompanionUiState.ts | 2 +- .../renderer/components/chat/useChatPrPaneOpen.test.tsx | 4 ---- docs/features/chat/composer-and-ui.md | 4 ++-- docs/features/pull-requests/README.md | 6 +++--- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx b/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx index 5162fd2275..1aa414e466 100644 --- a/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx +++ b/apps/desktop/src/renderer/components/chat/AgentChatPane.tsx @@ -12593,7 +12593,13 @@ export function AgentChatPane({ lifecycleSessionId={selectedSessionId ?? null} showGitToolbar={showWorkspaceChrome} prSessionId={renderedSessionId} - onTogglePrPane={showWorkspaceChrome && laneId ? () => setPrPaneOpen((v) => !v) : undefined} + // Only wire the pane toggle where the pane actually renders (a selected + // session). On the draft/new-chat surface an unwired pill falls back to + // the toolbar's inline PR menu instead of toggling persisted state for + // a pane that cannot appear there. + onTogglePrPane={showWorkspaceChrome && laneId && Boolean(selectedSessionId) + ? () => setPrPaneOpen((v) => !v) + : undefined} prPaneOpen={prPaneOpen} runtimePin={chatRuntimePin} trailingActions={chatHeaderTrailingActions} diff --git a/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts b/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts index ee9c84e2c9..fe04ef5417 100644 --- a/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts +++ b/apps/desktop/src/renderer/components/chat/chatCompanionUiState.ts @@ -20,7 +20,7 @@ export type ChatCompanionUiState = { iosSimulatorOpen: boolean; appControlOpen: boolean; terminalDrawerOpen: boolean; - /** Floating PR pane (left side). Persisted per chat, incl. webhook auto-pop. */ + /** Floating PR pane (left side). Persisted per chat; explicit open/close only. */ prPaneOpen: boolean; }; diff --git a/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx index 80fc6ef959..d29bc6aeb1 100644 --- a/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx +++ b/apps/desktop/src/renderer/components/chat/useChatPrPaneOpen.test.tsx @@ -12,18 +12,14 @@ import { DEFAULT_CHAT_COMPANION_UI_STATE, } from "./chatCompanionUiState"; -const originalAde = (globalThis.window as { ade?: unknown }).ade; - beforeEach(() => { window.localStorage.clear(); resetChatCompanionUiStateCacheForTests(); }); afterEach(() => { - (globalThis.window as { ade?: unknown }).ade = originalAde; window.localStorage.clear(); resetChatCompanionUiStateCacheForTests(); - vi.clearAllMocks(); }); describe("useChatPrPaneOpen", () => { diff --git a/docs/features/chat/composer-and-ui.md b/docs/features/chat/composer-and-ui.md index 606128b897..1bc547bcba 100644 --- a/docs/features/chat/composer-and-ui.md +++ b/docs/features/chat/composer-and-ui.md @@ -52,8 +52,8 @@ subagents, computer use). The pane derives all visible state from the | `ChatUserMinimap.tsx`, `chatUserMinimap.logic.ts` | Tick rail down the transcript's **left** gutter, one clean tick per user message with no guide hairline, gated on the `chatUserMinimapEnabled` appearance setting and mouse pointers only (`[@media(pointer:fine)]`). Ticks are positioned by percentage of the full message-list height, so they compress instead of overflowing and there is no marker cap or subsampling — the entry index stays 1:1 with the tick index, which is what pointer→index mapping depends on. The whole rail is a single `