diff --git a/.claude/commands/finalize.md b/.claude/commands/finalize.md index 1151b6ef3..5e4ace643 100644 --- a/.claude/commands/finalize.md +++ b/.claude/commands/finalize.md @@ -12,6 +12,14 @@ It guarantees three outcomes: 2. Docs are current 3. Local CI checks pass +It does **not** guarantee that remote PR review is complete after a push. GitHub's +first visible check list can look quiet before delayed checks, bot reviews, and +inline comments arrive. After pushing a finalized branch, hand off to +`/shipLane` or an equivalent PR poll loop. Use the ship-lane cadence: poll +immediately after a push, wait 270s if CI has not registered, wait 720s while CI +is running, and wait 1800s only when CI is done and the PR is just waiting on +review. + **Usage:** `/finalize` ## Execution Mode: Autonomous @@ -412,6 +420,34 @@ Kill selectively only if the parent is clearly gone (PPID == 1 on macOS/Linux). Report killed PIDs in the Phase 4 summary under "Cleanup" so the user can see what happened. +### 3k. Remote PR poll handoff + +If this finalize run is followed by a push or PR update, do not treat the first +`gh pr checks` result as authoritative proof that remote review is done. Some +checks and bot review systems appear late or post comments after the initial CI +surface looks complete. In particular: + +- `gh pr checks` can omit delayed or still-registering provider checks. +- Bot reviewers can post inline comments after CI jobs have already gone green. +- The absence of new comments immediately after a push is not evidence that no + more comments are coming. + +Handoff rule: + +```bash +# After the branch is pushed, continue with /shipLane or equivalent: +# - poll PR checks, status rollup, review comments, issue comments, and reviews +# - poll immediately after a push so early CI registration/failures are visible +# - if CI has not started yet, wait 270s +# - if any check is QUEUED/IN_PROGRESS/PENDING, wait 720s +# - if CI is done and the PR is only waiting on review, wait 1800s +# - poll again before declaring the PR clean or ready for human merge +``` + +If `/finalize` is running as a sub-step inside `/shipLane`, return a summary that +explicitly says remote checks/comments still require the ship-lane poll loop. +Do not report "PR clean" from `/finalize` alone. + --- ## Phase 4: Summary @@ -447,6 +483,11 @@ Report killed PIDs in the Phase 4 summary under "Cleanup" so the user can see wh ### Cleanup: - Orphan processes killed: N (PIDs: [list] or "none") +### Remote PR Handoff: +- Post-push polling required: YES +- Poll loop: `/shipLane` branch-specific cadence +- Reason: delayed checks and bot comments may arrive after first visible green state + ### Status: Ready to push / Issues found ``` @@ -466,3 +507,4 @@ Before marking complete: - [ ] All apps build successfully - [ ] Doc validation passed - [ ] Orphan worker processes cleaned up (vitest/tsup/tsc) — scoped to apps/ paths only +- [ ] Remote PR review is not declared clean by finalize alone; after push, `/shipLane` or an equivalent poll loop must use the branch-specific cadence and re-check comments/reviews diff --git a/apps/desktop/src/main/services/lanes/autoRebaseService.ts b/apps/desktop/src/main/services/lanes/autoRebaseService.ts index 39cccb963..308b1c924 100644 --- a/apps/desktop/src/main/services/lanes/autoRebaseService.ts +++ b/apps/desktop/src/main/services/lanes/autoRebaseService.ts @@ -112,17 +112,17 @@ function blockedMessage( laneId: string | null, reason: "conflict" | "manual" | "lookup" | "failed" | "unavailable" | null, ): string { - if (!laneId) return "Pending: auto-rebase stopped at an earlier lane. Open the Rebase tab to continue."; + if (!laneId) return "Pending: auto-rebase stopped at an earlier lane. Open the Rebase/Merge tab to continue."; if (reason === "manual") { - return `Pending: ancestor lane '${laneId}' has a fixed PR base. Rebase that lane manually from the Rebase tab before descendants can continue.`; + return `Pending: ancestor lane '${laneId}' has a fixed PR base. Rebase that lane manually from the Rebase/Merge tab before descendants can continue.`; } if (reason === "lookup" || reason === "unavailable") { - return `Pending: ancestor lane '${laneId}' needs review before descendants can continue. Open the Rebase tab to inspect it.`; + return `Pending: ancestor lane '${laneId}' needs review before descendants can continue. Open the Rebase/Merge tab to inspect it.`; } if (reason === "failed") { - return `Pending: ancestor lane '${laneId}' failed automatic rebase. Open the Rebase tab to retry.`; + return `Pending: ancestor lane '${laneId}' failed automatic rebase. Open the Rebase/Merge tab to retry.`; } - return `Pending: ancestor lane '${laneId}' has unresolved rebase conflicts. Open the Rebase tab to continue.`; + return `Pending: ancestor lane '${laneId}' has unresolved rebase conflicts. Open the Rebase/Merge tab to continue.`; } function resolveAffectedChainLaneId( @@ -459,7 +459,7 @@ export function createAutoRebaseService(args: { parentHeadSha: null, state: "rebasePending", conflictCount: 0, - message: "Pending: parent lane is unavailable. Open the Rebase tab to review the lane." + message: "Pending: parent lane is unavailable. Open the Rebase/Merge tab to review the lane." }); blocked = true; blockedLaneId = lane.id; @@ -522,7 +522,7 @@ export function createAutoRebaseService(args: { parentHeadSha, state: "rebaseConflict", conflictCount: Math.max(1, need.conflictingFiles.length), - message: `Auto-rebase blocked: ${Math.max(1, need.conflictingFiles.length)} conflict(s) expected. Open the Rebase tab to resolve and publish.` + message: `Auto-rebase blocked: ${Math.max(1, need.conflictingFiles.length)} conflict(s) expected. Open the Rebase/Merge tab to resolve and publish.` }); continue; } @@ -541,7 +541,7 @@ export function createAutoRebaseService(args: { parentHeadSha, state: "rebasePending", conflictCount: 0, - message: "PR carries an immutable base — drift detected. Rebase manually from the Rebase tab when ready." + message: "PR carries an immutable base — drift detected. Rebase manually from the Rebase/Merge tab when ready." }); continue; } @@ -572,8 +572,8 @@ export function createAutoRebaseService(args: { state: conflictHint ? "rebaseConflict" : "rebaseFailed", conflictCount: conflictHint ? 1 : 0, message: conflictHint - ? "Auto-rebase stopped due to conflicts. Open the Rebase tab to resolve, then publish." - : `Auto-rebase failed: ${rebaseRun.run.error}. Open the Rebase tab to retry.` + ? "Auto-rebase stopped due to conflicts. Open the Rebase/Merge tab to resolve, then publish." + : `Auto-rebase failed: ${rebaseRun.run.error}. Open the Rebase/Merge tab to retry.` }); continue; } @@ -625,8 +625,8 @@ export function createAutoRebaseService(args: { state: "rebaseFailed", conflictCount: 0, message: rollbackError - ? `Auto-push failed: ${pushError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase tab to retry.` - : `Auto-push failed: ${pushError}. The lane was restored to its pre-rebase state. Open the Rebase tab to retry.` + ? `Auto-push failed: ${pushError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase/Merge tab to retry.` + : `Auto-push failed: ${pushError}. The lane was restored to its pre-rebase state. Open the Rebase/Merge tab to retry.` }); } } diff --git a/apps/desktop/src/main/services/prs/prRebaseResolver.test.ts b/apps/desktop/src/main/services/prs/prRebaseResolver.test.ts index 0349af8c7..b53587497 100644 --- a/apps/desktop/src/main/services/prs/prRebaseResolver.test.ts +++ b/apps/desktop/src/main/services/prs/prRebaseResolver.test.ts @@ -141,8 +141,8 @@ describe("launchRebaseResolutionChat", () => { expect(sendMessage).toHaveBeenCalledWith( expect.objectContaining({ sessionId: "session-rebase-1", - displayText: "Rebase feature/rebase-target onto main", reasoningEffort: "high", + displayText: "Rebase feature/rebase-target onto main", }), ); expect(result).toEqual({ diff --git a/apps/desktop/src/main/services/prs/prRebaseResolver.ts b/apps/desktop/src/main/services/prs/prRebaseResolver.ts index c9cbd3cdf..d7bfecfa2 100644 --- a/apps/desktop/src/main/services/prs/prRebaseResolver.ts +++ b/apps/desktop/src/main/services/prs/prRebaseResolver.ts @@ -158,6 +158,10 @@ export async function launchRebaseResolutionChat( await deps.agentChatService.sendMessage({ sessionId: session.id, text: prompt, + // Show the short human-readable title in the transcript instead of the + // full composed prompt (which is long and technical). Mirrors + // prIssueResolver's pattern; agentChatService falls back to `text` when + // displayText is absent, which would surface the raw prompt to the user. displayText: title, ...(reasoningEffort ? { reasoningEffort } : {}), }); diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 2d29ecb20..22841aa4e 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -871,7 +871,7 @@ export function createPrService({ parentHeadSha: null, state: "rebaseFailed", conflictCount: 0, - message: `Auto-rebase failed after '${args.landedLaneName}' merged because ADE could not find a new parent lane. Open the Rebase tab to recover this lane.`, + message: `Auto-rebase failed after '${args.landedLaneName}' merged because ADE could not find a new parent lane. Open the Rebase/Merge tab to recover this lane.`, }, child.id); } return { @@ -979,8 +979,8 @@ export function createPrService({ state: "rebaseFailed", conflictCount: 0, message: rollbackError - ? `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase tab to recover this lane.` - : `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. The lane was restored to its pre-rebase state. Open the Rebase tab to recover this lane.`, + ? `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. Automatic rollback also failed: ${rollbackError}. Open the Rebase/Merge tab to recover this lane.` + : `Auto-rebase failed after '${args.landedLaneName}' merged: ${childError}. The lane was restored to its pre-rebase state. Open the Rebase/Merge tab to recover this lane.`, }, child.id); failedLaneIds.push(child.id); } diff --git a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx index 8fabc39ab..d9fa62791 100644 --- a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx +++ b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.test.tsx @@ -328,7 +328,7 @@ describe("LaneGitActionsPane rescue action", () => { expect(syncButton.getAttribute("title")).toMatch(/before rebasing and pushing/i); }); - it("treats auto-rebase conflicts as failures and links to the Rebase tab", async () => { + it("treats auto-rebase conflicts as failures and links to the Rebase/Merge tab", async () => { const user = userEvent.setup(); const resolveRebaseConflict = vi.fn(); mockAutoRebaseStatuses = [ @@ -345,7 +345,7 @@ describe("LaneGitActionsPane rescue action", () => { renderPane({ onResolveRebaseConflict: resolveRebaseConflict }); - const rebaseTabButton = await screen.findByRole("button", { name: /open rebase tab/i }); + const rebaseTabButton = await screen.findByRole("button", { name: /open rebase\/merge tab/i }); screen.getByText("AUTO-REBASE FAILED"); screen.getByText(/auto-rebase failed\. files need follow-up before this lane can be pushed\./i); diff --git a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx index 60cafb46a..f2b5b151e 100644 --- a/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx +++ b/apps/desktop/src/renderer/components/lanes/LaneGitActionsPane.tsx @@ -1546,7 +1546,7 @@ export function LaneGitActionsPane({ onResolveRebaseConflict(laneId, rebaseConflictParentLaneId); return; } - const search = new URLSearchParams({ tab: "rebase", laneId }); + const search = new URLSearchParams({ tab: "workflows", workflow: "rebase", laneId }); if (rebaseConflictParentLaneId) search.set("parentLaneId", rebaseConflictParentLaneId); navigate(`/prs?${search.toString()}`); }; @@ -1571,8 +1571,8 @@ export function LaneGitActionsPane({ {autoRebaseStatus.state !== "autoRebased" ? ( isAutoRebaseFailure ? ( ) : ( diff --git a/apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx b/apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx index c5462aa72..11ff13f9d 100644 --- a/apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx +++ b/apps/desktop/src/renderer/components/lanes/LaneRebaseBanner.tsx @@ -50,13 +50,13 @@ export function LaneRebaseBanner({
- + @@ -120,13 +120,13 @@ export function LaneRebaseBanner({
- + diff --git a/apps/desktop/src/renderer/components/lanes/LanesPage.tsx b/apps/desktop/src/renderer/components/lanes/LanesPage.tsx index 15fd9cba8..2fbc521eb 100644 --- a/apps/desktop/src/renderer/components/lanes/LanesPage.tsx +++ b/apps/desktop/src/renderer/components/lanes/LanesPage.tsx @@ -1101,7 +1101,7 @@ export function LanesPage() { const failedLane = start.run.failedLaneId ? lanesById.get(start.run.failedLaneId)?.name ?? start.run.failedLaneId : null; const detail = start.run.error ?? "Rebase failed."; setRebaseSuggestionError(`Rebase needs attention${failedLane ? ` for ${failedLane}` : ""}. ${detail}`); - navigate("/prs?tab=rebase"); + navigate("/prs?tab=workflows&workflow=rebase"); return; } @@ -1121,7 +1121,7 @@ export function LanesPage() { } catch (err) { const message = err instanceof Error ? err.message : String(err); setRebaseSuggestionError(message); - navigate("/prs?tab=rebase"); + navigate("/prs?tab=workflows&workflow=rebase"); } }, [lanesById, navigate, refreshLanes, requestPushSelection, requestRebaseScope]); diff --git a/apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx b/apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx index 7c3fa158e..a15f5e12b 100644 --- a/apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx +++ b/apps/desktop/src/renderer/components/prs/CreatePrModal.test.tsx @@ -4,8 +4,13 @@ import React from "react"; import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { MemoryRouter } from "react-router-dom"; import type { LaneSummary } from "../../../shared/types"; +function renderWithRouter(ui: React.ReactElement) { + return render({ui}); +} + function makeLane(overrides: Partial = {}): LaneSummary { return { id: "lane-1", @@ -117,7 +122,7 @@ describe("CreatePrModal queue workflow", () => { it("adds selected lanes to the queue order and removes them from the queue builder", async () => { const user = userEvent.setup(); - render(); + renderWithRouter(); await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!); await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i })); @@ -136,7 +141,7 @@ describe("CreatePrModal queue workflow", () => { it("uses the dragged queue order when creating queue PRs", async () => { const user = userEvent.setup(); - render(); + renderWithRouter(); await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!); await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i })); @@ -165,7 +170,7 @@ describe("CreatePrModal queue workflow", () => { it("lets single-PR creation target a different branch than Primary's current branch", async () => { const user = userEvent.setup(); - render(); + renderWithRouter(); // Select source lane const comboboxes = screen.getAllByRole("combobox"); @@ -191,7 +196,7 @@ describe("CreatePrModal queue workflow", () => { it("warns when the PR target branch differs from the lane base branch", async () => { const user = userEvent.setup(); - render(); + renderWithRouter(); // Select source lane const comboboxes = screen.getAllByRole("combobox"); @@ -210,7 +215,7 @@ describe("CreatePrModal queue workflow", () => { it("lets queue creation target a different branch than Primary's current branch", async () => { const user = userEvent.setup(); - render(); + renderWithRouter(); await user.click(screen.getAllByRole("button", { name: /queue workflow/i })[0]!); await user.click(screen.getByRole("checkbox", { name: /01 queue lane/i })); diff --git a/apps/desktop/src/renderer/components/prs/CreatePrModal.tsx b/apps/desktop/src/renderer/components/prs/CreatePrModal.tsx index a078a9f7f..bae39811b 100644 --- a/apps/desktop/src/renderer/components/prs/CreatePrModal.tsx +++ b/apps/desktop/src/renderer/components/prs/CreatePrModal.tsx @@ -1,4 +1,5 @@ import React from "react"; +import { useNavigate } from "react-router-dom"; import * as Dialog from "@radix-ui/react-dialog"; import { GitPullRequest, GitMerge, Stack as Layers, CheckCircle, Warning, CircleNotch, X, GitBranch, Sparkle, ArrowRight, ArrowLeft, Check, DotsSixVertical, Trash, ArrowUp, ArrowDown } from "@phosphor-icons/react"; import { useAppStore } from "../../state/appStore"; @@ -446,7 +447,7 @@ function LaneWarningPanel({ cursor: "pointer", }} > - Open Rebase Tab + Open Rebase/Merge tab Review rebase status before PR creation{rebaseLaneIds.length > 1 ? ` (${rebaseLaneIds.length} lanes)` : ""}. @@ -470,6 +471,7 @@ export function CreatePrModal({ onOpenChange: (open: boolean) => void; onCreated?: (created: PrSummary[]) => void | Promise; }) { + const navigate = useNavigate(); const lanes = useAppStore((s) => s.lanes); const primaryLane = React.useMemo(() => lanes.find((l) => l.laneType === "primary") ?? null, [lanes]); @@ -596,8 +598,9 @@ export function CreatePrModal({ const openRebaseTab = React.useCallback((laneId: string) => { onOpenChange(false); - window.location.hash = `#/prs?tab=rebase&laneId=${encodeURIComponent(laneId)}`; - }, [onOpenChange]); + const search = new URLSearchParams({ tab: "workflows", workflow: "rebase", laneId }); + navigate({ pathname: "/prs", search: `?${search.toString()}` }); + }, [navigate, onOpenChange]); // Reset on close React.useEffect(() => { diff --git a/apps/desktop/src/renderer/components/prs/PRsPage.tsx b/apps/desktop/src/renderer/components/prs/PRsPage.tsx index e10366d45..eab3a5fa1 100644 --- a/apps/desktop/src/renderer/components/prs/PRsPage.tsx +++ b/apps/desktop/src/renderer/components/prs/PRsPage.tsx @@ -11,7 +11,7 @@ import { GitHubTab } from "./tabs/GitHubTab"; import { WorkflowsTab, type WorkflowCategory } from "./tabs/WorkflowsTab"; import { SANS_FONT } from "../lanes/laneDesignTokens"; import { isMissionLaneHiddenByDefault } from "../lanes/laneUtils"; -import { buildPrsRouteSearch, parsePrsRouteState } from "./prsRouteState"; +import { buildPrsRouteSearch, parsePrsRouteState, resolvePrsActiveTab } from "./prsRouteState"; import { resolveRouteRebaseSelection } from "./shared/rebaseNeedUtils"; import type { PrSummary } from "../../../shared/types"; @@ -84,31 +84,21 @@ function PRsPageInner() { search: location.search, hash: window.location.hash, }); - const tab = routeState.tab; - const workflowTab = routeState.workflowTab; + const resolved = resolvePrsActiveTab(routeState); const routeRebaseItemId = resolveRouteRebaseSelection({ rebaseNeeds, routeItemId: routeState.laneId, }); - if (tab === "github" || tab === "normal") { - setActiveTab("normal"); - } else if (tab === "workflows") { - const nextWorkflowTab = workflowTab === "queue" || workflowTab === "integration" || workflowTab === "rebase" - ? workflowTab - : "integration"; - setActiveTab(nextWorkflowTab); - } else if (tab === "queue" || tab === "integration" || tab === "rebase") { - setActiveTab(tab); - } + setActiveTab(resolved.activeTab); - if (tab === "normal" || tab === "github") { + if (!resolved.isWorkflowRoute) { setSelectedPrId(routeState.prId ?? null); } - if (tab === "queue" || workflowTab === "queue") { + if (resolved.effectiveWorkflow === "queue") { setSelectedQueueGroupId(routeState.queueGroupId ?? null); } - if (tab === "rebase" || workflowTab === "rebase") { + if (resolved.effectiveWorkflow === "rebase") { setSelectedRebaseItemId(routeRebaseItemId); } } catch { diff --git a/apps/desktop/src/renderer/components/prs/prsRouteState.test.ts b/apps/desktop/src/renderer/components/prs/prsRouteState.test.ts index 421bd9c11..9882a5908 100644 --- a/apps/desktop/src/renderer/components/prs/prsRouteState.test.ts +++ b/apps/desktop/src/renderer/components/prs/prsRouteState.test.ts @@ -1,18 +1,18 @@ import { describe, expect, it } from "vitest"; -import { buildPrsRouteSearch, parsePrsRouteState } from "./prsRouteState"; +import { buildPrsRouteSearch, parsePrsRouteState, resolvePrsActiveTab } from "./prsRouteState"; describe("prsRouteState", () => { - it("parses route state from search params and falls back to hash params when needed", () => { + it("treats a hash PR route as authoritative over stale outer search params", () => { expect( parsePrsRouteState({ search: "?tab=normal&prId=pr-123&laneId=lane-search", hash: "#/prs?tab=workflows&workflow=queue&queueGroupId=group-hash", }), ).toEqual({ - tab: "normal", + tab: "workflows", workflowTab: "queue", - laneId: "lane-search", - prId: "pr-123", + laneId: null, + prId: null, queueGroupId: "group-hash", eventId: null, threadId: null, @@ -98,3 +98,102 @@ describe("prsRouteState", () => { ).toBe("?tab=workflows&workflow=rebase&laneId=lane-456"); }); }); + +describe("resolvePrsActiveTab", () => { + it("routes a stale tab=normal + hash workflow=rebase to the rebase workflow", () => { + const parsed = parsePrsRouteState({ + search: "?tab=normal", + hash: "#/prs?tab=workflows&workflow=rebase&laneId=lane-1", + }); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(true); + expect(resolved.effectiveWorkflow).toBe("rebase"); + expect(resolved.activeTab).toBe("rebase"); + }); + + it("falls back to integration when tab=workflows has no workflow param", () => { + const parsed = parsePrsRouteState({ search: "?tab=workflows" }); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(true); + expect(resolved.effectiveWorkflow).toBeNull(); + expect(resolved.activeTab).toBe("integration"); + }); + + it("treats a workflow-alias tab (tab=queue) as a workflow route", () => { + const parsed = parsePrsRouteState({ search: "?tab=queue&queueGroupId=g-1" }); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(true); + expect(resolved.effectiveWorkflow).toBe("queue"); + expect(resolved.activeTab).toBe("queue"); + }); + + it("keeps tab=normal on the normal tab when no workflow signal is present", () => { + const parsed = parsePrsRouteState({ search: "?tab=normal&prId=pr-1" }); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(false); + expect(resolved.activeTab).toBe("normal"); + }); + + it("returns normal when the route has no tab or workflow signal", () => { + const parsed = parsePrsRouteState({ search: "" }); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(false); + expect(resolved.activeTab).toBe("normal"); + }); + + it("keeps legacy prId-only routes on the normal PR surface", () => { + const parsed = parsePrsRouteState({ search: "?prId=pr-123" }); + const resolved = resolvePrsActiveTab(parsed); + expect(parsed.prId).toBe("pr-123"); + expect(parsed.tab).toBeNull(); + expect(resolved.isWorkflowRoute).toBe(false); + expect(resolved.activeTab).toBe("normal"); + }); + + it("prefers the hash workflow over a stale outer search workflow (BrowserRouter mock mode)", () => { + // Outer URL like `/?tab=workflows&workflow=queue#/prs?tab=workflows&workflow=rebase` + // In BrowserRouter mock mode the outer search is stale; the inner hash is the + // current in-app location and must win. + const parsed = parsePrsRouteState({ + search: "?tab=workflows&workflow=queue", + hash: "#/prs?tab=workflows&workflow=rebase", + }); + expect(parsed.workflowTab).toBe("rebase"); + const resolved = resolvePrsActiveTab(parsed); + expect(resolved.isWorkflowRoute).toBe(true); + expect(resolved.effectiveWorkflow).toBe("rebase"); + expect(resolved.activeTab).toBe("rebase"); + }); + + it("keeps hash lane ids with the hash workflow when outer search is stale", () => { + const parsed = parsePrsRouteState({ + search: "?tab=workflows&workflow=rebase&laneId=old", + hash: "#/prs?tab=workflows&workflow=rebase&laneId=new", + }); + expect(parsed.workflowTab).toBe("rebase"); + expect(parsed.laneId).toBe("new"); + }); + + it("defaults a bare hash workflows route to integration even when outer search says normal", () => { + const parsed = parsePrsRouteState({ + search: "?tab=normal", + hash: "#/prs?tab=workflows", + }); + const resolved = resolvePrsActiveTab(parsed); + expect(parsed.tab).toBe("workflows"); + expect(resolved.isWorkflowRoute).toBe(true); + expect(resolved.activeTab).toBe("integration"); + }); + + it("ignores invalid hash route signals instead of dropping valid search params", () => { + const parsed = parsePrsRouteState({ + search: "?tab=normal&prId=pr-123", + hash: "#/prs?tab=bogus", + }); + const resolved = resolvePrsActiveTab(parsed); + expect(parsed.tab).toBe("normal"); + expect(parsed.prId).toBe("pr-123"); + expect(resolved.isWorkflowRoute).toBe(false); + expect(resolved.activeTab).toBe("normal"); + }); +}); diff --git a/apps/desktop/src/renderer/components/prs/prsRouteState.ts b/apps/desktop/src/renderer/components/prs/prsRouteState.ts index ecdea45a5..0a5eae305 100644 --- a/apps/desktop/src/renderer/components/prs/prsRouteState.ts +++ b/apps/desktop/src/renderer/components/prs/prsRouteState.ts @@ -44,13 +44,20 @@ function parseOptionalId(value: string | null): string | null { export function parsePrsRouteState(args: { search?: string | null; hash?: string | null }): ParsedPrsRouteState { const searchParams = parseSearch(args.search ?? ""); const hashParams = parseHashParams(args.hash ?? ""); + const hashHasRouteSignal = + parseTab(hashParams.get("tab")) !== null || parseWorkflowTab(hashParams.get("workflow")) !== null; + const routeParams = hashHasRouteSignal ? hashParams : searchParams; - const pick = (key: string): string | null => - parseOptionalId(searchParams.get(key)) ?? parseOptionalId(hashParams.get(key)); + const pick = (key: string): string | null => parseOptionalId(routeParams.get(key)); + + // In BrowserRouter mock mode the inner hash is the current in-app location, + // while the outer search may be stale from a previous view. Once the hash + // carries any PR route signal, treat it as authoritative for the whole route. + const workflowTab = parseWorkflowTab(routeParams.get("workflow")); return { - tab: parseTab(searchParams.get("tab") ?? hashParams.get("tab")), - workflowTab: parseWorkflowTab(searchParams.get("workflow") ?? hashParams.get("workflow")), + tab: parseTab(routeParams.get("tab")), + workflowTab, laneId: pick("laneId"), prId: pick("prId"), queueGroupId: pick("queueGroupId"), @@ -60,6 +67,42 @@ export function parsePrsRouteState(args: { search?: string | null; hash?: string }; } +export type ResolvedPrsRoute = { + isWorkflowRoute: boolean; + effectiveWorkflow: PrWorkflowTab | null; + activeTab: "normal" | PrWorkflowTab; +}; + +/** + * Collapse a parsed route into a single activeTab decision. + * + * Routing bounce-back guard: the presence of a `workflow=` param, or a + * workflow-alias `tab=` value (queue/integration/rebase), is treated as + * authoritative evidence of a workflow route. This prevents a stale + * `?tab=normal` in the outer search (BrowserRouter mock mode) from shadowing + * a hash-based workflow URL. + */ +export function resolvePrsActiveTab(route: ParsedPrsRouteState): ResolvedPrsRoute { + const workflowAlias: PrWorkflowTab | null = + route.tab === "queue" || route.tab === "integration" || route.tab === "rebase" + ? route.tab + : null; + const effectiveWorkflow = route.workflowTab ?? workflowAlias; + const isWorkflowRoute = Boolean(effectiveWorkflow) || route.tab === "workflows"; + if (isWorkflowRoute) { + return { + isWorkflowRoute: true, + effectiveWorkflow, + activeTab: effectiveWorkflow ?? "integration", + }; + } + return { + isWorkflowRoute: false, + effectiveWorkflow: null, + activeTab: "normal", + }; +} + export function buildPrsRouteSearch(args: { activeTab: PrActiveTab; selectedPrId: string | null; diff --git a/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx b/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx index df0557a5a..b94a613b2 100644 --- a/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx +++ b/apps/desktop/src/renderer/components/prs/state/PrsContext.test.tsx @@ -23,6 +23,18 @@ function Harness() { ); } +function RouteHarness() { + const { activeTab, selectedPrId, selectedQueueGroupId, selectedRebaseItemId } = usePrs(); + return ( +
+
{activeTab}
+
{selectedPrId ?? ""}
+
{selectedQueueGroupId ?? ""}
+
{selectedRebaseItemId ?? ""}
+
+ ); +} + describe("PrsContext refresh", () => { beforeEach(() => { const refreshedNeed: RebaseNeed = { @@ -78,6 +90,7 @@ describe("PrsContext refresh", () => { afterEach(() => { cleanup(); globalThis.window.ade = originalAde; + window.location.hash = ""; }); it("refreshes rebase needs and auto-rebase statuses without waiting for events", async () => { @@ -102,6 +115,67 @@ describe("PrsContext refresh", () => { expect(screen.getByTestId("auto-count").textContent).toBe("1"); }); }); + + it("hydrates the Rebase/Merge workflow selection from the initial hash route", async () => { + window.location.hash = "#/prs?tab=workflows&workflow=rebase&laneId=lane-1"; + + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId("active-tab").textContent).toBe("rebase"); + }); + expect(screen.getByTestId("selected-pr-id").textContent).toBe(""); + expect(screen.getByTestId("selected-queue-group-id").textContent).toBe(""); + expect(screen.getByTestId("selected-rebase-item-id").textContent).toBe("lane-1"); + + }); + + it("does not bounce off the rebase workflow when a stale tab=normal shadows the hash", async () => { + // BrowserRouter mock mode can leave a stale `?tab=normal` in the outer + // search while the hash advances to a workflow URL. The initial route + // resolver must treat the hash workflow as authoritative. + window.history.replaceState(null, "", "/?tab=normal#/prs?tab=workflows&workflow=rebase&laneId=lane-1"); + expect(window.location.search).toBe("?tab=normal"); + expect(window.location.hash).toBe("#/prs?tab=workflows&workflow=rebase&laneId=lane-1"); + + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId("active-tab").textContent).toBe("rebase"); + }); + expect(screen.getByTestId("selected-pr-id").textContent).toBe(""); + expect(screen.getByTestId("selected-rebase-item-id").textContent).toBe("lane-1"); + + window.history.replaceState(null, "", "/"); + }); + + it("hydrates a legacy PR deep link without an explicit tab as the normal surface", async () => { + window.history.replaceState(null, "", "/?prId=pr-123"); + vi.mocked(window.ade.prs.listWithConflicts).mockResolvedValue([makeFakePr("pr-123")]); + + render( + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId("active-tab").textContent).toBe("normal"); + }); + expect(screen.getByTestId("selected-pr-id").textContent).toBe("pr-123"); + expect(screen.getByTestId("selected-queue-group-id").textContent).toBe(""); + expect(screen.getByTestId("selected-rebase-item-id").textContent).toBe(""); + + window.history.replaceState(null, "", "/"); + }); }); // --------------------------------------------------------------------------- diff --git a/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx b/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx index 2e8f014c2..c7ddd5d7b 100644 --- a/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx +++ b/apps/desktop/src/renderer/components/prs/state/PrsContext.tsx @@ -34,6 +34,8 @@ import type { PrTimelineFilters } from "../shared/PrTimeline"; import { DEFAULT_PR_TIMELINE_FILTERS } from "../shared/PrTimeline"; import { buildPrAiResolutionContextKey } from "../../../../shared/types"; import { getModelById } from "../../../../shared/modelRegistry"; +import { parsePrsRouteState, resolvePrsActiveTab } from "../prsRouteState"; +import { resolveRouteRebaseSelection } from "../shared/rebaseNeedUtils"; type PrTab = "normal" | "queue" | "integration" | "rebase"; @@ -210,22 +212,41 @@ function readPersistedReasoningLevel(): string { return "medium"; } -function readInitialTab(): PrTab { - try { - const params = new URLSearchParams(window.location.search); - const tab = params.get("tab"); - if (tab === "normal" || tab === "queue" || tab === "integration" || tab === "rebase") return tab; - } catch { /* ignore */ } - return "normal"; -} - -function readInitialPrId(): string | null { +function readInitialRouteState(): { + activeTab: PrTab; + selectedPrId: string | null; + selectedQueueGroupId: string | null; + selectedRebaseItemId: string | null; +} { try { - const params = new URLSearchParams(window.location.search); - const prId = params.get("prId"); - if (prId && prId.trim().length > 0) return prId.trim(); + const route = parsePrsRouteState({ + search: window.location.search, + hash: window.location.hash, + }); + const resolved = resolvePrsActiveTab(route); + const activeTab: PrTab = resolved.isWorkflowRoute + ? (resolved.effectiveWorkflow ?? "integration") + : "normal"; + return { + activeTab, + selectedPrId: !resolved.isWorkflowRoute ? route.prId : null, + selectedQueueGroupId: resolved.effectiveWorkflow === "queue" ? route.queueGroupId : null, + // Mirror PRsPage's resolver so the shape of this id matches what the + // rebase UI later expects. rebaseNeeds are empty at provider mount, so + // this returns the bare lane id; PRsPage's syncFromLocation effect runs + // the same resolver again once needs load and upgrades it to the + // canonical need-item key. + selectedRebaseItemId: resolved.effectiveWorkflow === "rebase" + ? resolveRouteRebaseSelection({ rebaseNeeds: [], routeItemId: route.laneId }) + : null, + }; } catch { /* ignore */ } - return null; + return { + activeTab: "normal", + selectedPrId: null, + selectedQueueGroupId: null, + selectedRebaseItemId: null, + }; } function requirePrId(prId: string): string { @@ -269,13 +290,18 @@ function diffPrIds(prev: PrWithConflicts[], next: PrWithConflicts[]): string[] { } export function PrsProvider({ children }: { children: React.ReactNode }) { - const [activeTab, setActiveTab] = useState(readInitialTab); + // Compute initial route state exactly once per provider mount. Reading + // window.location + running parsePrsRouteState/resolvePrsActiveTab on every + // render would be wasteful; useMemo with empty deps captures it once so all + // four useState calls below share a single computation. + const initialRouteState = useMemo(() => readInitialRouteState(), []); + const [activeTab, setActiveTab] = useState(initialRouteState.activeTab); const [prs, setPrs] = useState([]); const [lanes, setLanes] = useState([]); const [mergeContextByPrId, setMergeContextByPrId] = useState>({}); - const [selectedPrId, setSelectedPrId] = useState(readInitialPrId); - const [selectedQueueGroupId, setSelectedQueueGroupId] = useState(null); - const [selectedRebaseItemId, setSelectedRebaseItemId] = useState(null); + const [selectedPrId, setSelectedPrId] = useState(initialRouteState.selectedPrId); + const [selectedQueueGroupId, setSelectedQueueGroupId] = useState(initialRouteState.selectedQueueGroupId); + const [selectedRebaseItemId, setSelectedRebaseItemId] = useState(initialRouteState.selectedRebaseItemId); const [mergeMethod, setMergeMethod] = useState("squash"); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); diff --git a/apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx b/apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx index 90a49eb84..1d7aedc59 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/IntegrationTab.tsx @@ -1,4 +1,5 @@ import React from "react"; +import { useNavigate } from "react-router-dom"; import { GitMerge, GitBranch, Lightning, Eye, Sparkle, Trash, ArrowRight, ArrowSquareOut, CheckCircle, Warning, XCircle, Clock, GithubLogo, CircleNotch, ArrowsClockwise, CaretDown, CaretRight, Robot, Gear } from "@phosphor-icons/react"; import type { IntegrationProposal, @@ -120,6 +121,7 @@ function RebaseGuidancePanel({ onResimulate?: () => void; resimulateLabel?: string; }) { + const navigate = useNavigate(); return (
{ - window.location.hash = `#/prs?tab=rebase&laneId=${encodeURIComponent(laneId)}`; + const search = new URLSearchParams({ tab: "workflows", workflow: "rebase", laneId }); + navigate({ pathname: "/prs", search: `?${search.toString()}` }); }} > - OPEN REBASE TAB + Open Rebase/Merge tab {onResimulate ? (
@@ -1447,7 +1453,7 @@ export function QueueTab({ ) : null}
- Batch rebases stop at the first failure so the queue does not quietly drift. Use the Rebase tab for detailed lane history, aborts, rollbacks, and any follow-up after a partial run. + Batch rebases stop at the first failure so the queue does not quietly drift. Use the Rebase/Merge tab for detailed lane history, aborts, rollbacks, and any follow-up after a partial run.
@@ -1502,7 +1508,7 @@ export function QueueTab({ cursor: "pointer", }} > - Open failed lane in rebase tab + Open failed lane in Rebase/Merge tab ) : null} diff --git a/apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx b/apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx index c0e4c0074..6947f0690 100644 --- a/apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx +++ b/apps/desktop/src/renderer/components/prs/tabs/WorkflowsTab.tsx @@ -762,7 +762,7 @@ export function WorkflowsTab({ {([ { id: "integration" as WorkflowCategory, label: "Integration", icon: GitBranch }, { id: "queue" as WorkflowCategory, label: "Queue", icon: CaretRight }, - { id: "rebase" as WorkflowCategory, label: "Rebase", icon: Sparkle }, + { id: "rebase" as WorkflowCategory, label: "Rebase/Merge", icon: Sparkle }, ]).map((category) => { const selected = activeCategory === category.id; const catTheme = CATEGORY_THEMES[category.id]; diff --git a/apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx b/apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx index fd4f42df2..485d4b563 100644 --- a/apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx +++ b/apps/desktop/src/renderer/components/settings/LaneBehaviorSection.tsx @@ -224,15 +224,15 @@ export function LaneBehaviorSection() { )} - {/* Save + Open Rebase tab */} + {/* Save + Open Rebase/Merge tab */}