From 933f44737efdab265a5c825527f66d39464e956c Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Sun, 8 Mar 2026 16:15:14 -0700 Subject: [PATCH] feat: split worktree/diff-type into separate controls (#248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: split worktree/diff-type into separate controls The single dropdown that mixed diff type selection (uncommitted, last-commit, branch) with worktree switching was confusing — entering a worktree silently replaced all dropdown options with no persistent indicator of which worktree was active. Split into two controls: - Context dropdown: switches between main repo and worktrees (only shown when worktrees exist, highlighted when a worktree is active) - View dropdown: always shows the same diff type options regardless of context Server changes: - GitContext now exposes worktrees as a separate field - Removed getWorktreeDiffOptions() and back-to-main sentinel handling - Simplified /api/diff/switch (no more diffOptions replacement) Client changes: - New activeWorktreePath state + handleWorktreeSwitch callback - handleDiffSwitch composes worktree prefix from context automatically - Derived activeDiffBase strips worktree prefix for dropdown display Co-Authored-By: Claude Opus 4.6 * refactor: derive activeWorktreePath from diffType instead of separate state Eliminates redundant state — activeWorktreePath and activeDiffBase are now both derived in a single useMemo from the composite diffType string. Co-Authored-By: Claude Opus 4.6 * refactor: move DiffOption, WorktreeInfo, GitContext to @plannotator/shared Eliminates duplicate interface definitions across server and client. Types now live in packages/shared/types.ts and are imported by both packages/server/git.ts and packages/review-editor/. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- bun.lock | 1 + packages/review-editor/App.tsx | 87 +++++++++++-------- .../review-editor/components/FileTree.tsx | 67 ++++++++++---- packages/review-editor/package.json | 1 + packages/server/git.ts | 47 +--------- packages/server/review.ts | 30 +------ packages/shared/types.ts | 19 ++++ tests/manual/test-worktree-review.ts | 37 ++++---- 8 files changed, 145 insertions(+), 144 deletions(-) diff --git a/bun.lock b/bun.lock index a34b0beb..a5be0522 100644 --- a/bun.lock +++ b/bun.lock @@ -144,6 +144,7 @@ "version": "0.0.1", "dependencies": { "@pierre/diffs": "^1.1.0-beta.19", + "@plannotator/shared": "workspace:*", "@plannotator/ui": "workspace:*", "highlight.js": "^11.11.1", "react": "^19.2.3", diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index 9cf80f5f..e97ee5e6 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -18,6 +18,7 @@ import { DiffViewer } from './components/DiffViewer'; import { ReviewPanel } from './components/ReviewPanel'; import { FileTree } from './components/FileTree'; import { DEMO_DIFF } from './demoData'; +import type { DiffOption, WorktreeInfo, GitContext } from '@plannotator/shared/types'; declare const __APP_VERSION__: string; @@ -29,17 +30,6 @@ interface DiffFile { deletions: number; } -interface DiffOption { - id: string; - label: string; -} - -interface GitContext { - currentBranch: string; - defaultBranch: string; - diffOptions: DiffOption[]; -} - interface DiffData { files: DiffFile[]; rawPatch: string; @@ -355,16 +345,30 @@ const ReviewApp: React.FC = () => { }); }, []); - // Switch diff type (uncommitted, staged, last-commit, branch, etc.) - const handleDiffSwitch = useCallback(async (newDiffType: string) => { - if (newDiffType === diffType || newDiffType === 'separator') return; + // Derive worktree path and base diff type from the composite diffType string + const { activeWorktreePath, activeDiffBase } = useMemo(() => { + if (diffType.startsWith('worktree:')) { + const rest = diffType.slice('worktree:'.length); + const lastColon = rest.lastIndexOf(':'); + if (lastColon !== -1) { + const sub = rest.slice(lastColon + 1); + if (['uncommitted', 'last-commit', 'branch'].includes(sub)) { + return { activeWorktreePath: rest.slice(0, lastColon), activeDiffBase: sub }; + } + } + return { activeWorktreePath: rest, activeDiffBase: 'uncommitted' }; + } + return { activeWorktreePath: null, activeDiffBase: diffType }; + }, [diffType]); + // Shared helper: fetch a diff switch and update state + const fetchDiffSwitch = useCallback(async (fullDiffType: string) => { setIsLoadingDiff(true); try { const res = await fetch('/api/diff/switch', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ diffType: newDiffType }), + body: JSON.stringify({ diffType: fullDiffType }), }); if (!res.ok) throw new Error('Failed to switch diff'); @@ -373,31 +377,39 @@ const ReviewApp: React.FC = () => { rawPatch: string; gitRef: string; diffType: string; - diffOptions?: { id: string; label: string }[]; error?: string; }; - const newFiles = parseDiffToFiles(data.rawPatch); - setFiles(newFiles); + setFiles(parseDiffToFiles(data.rawPatch)); setDiffType(data.diffType); setActiveFileIndex(0); setPendingSelection(null); setDiffError(data.error || null); - - // Update dropdown options on worktree mode transitions - if (data.diffOptions) { - setGitContext(prev => prev ? { ...prev, diffOptions: data.diffOptions! } : prev); - } - - // Note: We keep existing annotations - they may still be relevant - // or user can clear them manually } catch (err) { console.error('Failed to switch diff:', err); setDiffError(err instanceof Error ? err.message : 'Failed to switch diff'); } finally { setIsLoadingDiff(false); } - }, [diffType]); + }, []); + + // Switch diff type (uncommitted, last-commit, branch) — composes worktree prefix if active + const handleDiffSwitch = useCallback(async (baseDiffType: string) => { + const fullDiffType = activeWorktreePath + ? `worktree:${activeWorktreePath}:${baseDiffType}` + : baseDiffType; + if (fullDiffType === diffType) return; + await fetchDiffSwitch(fullDiffType); + }, [diffType, activeWorktreePath, fetchDiffSwitch]); + + // Switch worktree context (or back to main repo) + const handleWorktreeSwitch = useCallback(async (worktreePath: string | null) => { + if (worktreePath === activeWorktreePath) return; + const fullDiffType = worktreePath + ? `worktree:${worktreePath}:uncommitted` + : 'uncommitted'; + await fetchDiffSwitch(fullDiffType); + }, [activeWorktreePath, fetchDiffSwitch]); // Select annotation - switches file if needed and scrolls to it const handleSelectAnnotation = useCallback((id: string | null) => { @@ -788,10 +800,14 @@ const ReviewApp: React.FC = () => { onToggleHideViewed={() => setHideViewedFiles(prev => !prev)} enableKeyboardNav={!showExportModal} diffOptions={gitContext?.diffOptions} - activeDiffType={diffType} + activeDiffType={activeDiffBase} onSelectDiff={handleDiffSwitch} isLoadingDiff={isLoadingDiff} width={fileTreeResize.width} + worktrees={gitContext?.worktrees} + activeWorktreePath={activeWorktreePath} + onSelectWorktree={handleWorktreeSwitch} + currentBranch={gitContext?.currentBranch} /> @@ -850,16 +866,11 @@ const ReviewApp: React.FC = () => { <>

No changes

- {diffType === 'uncommitted' && "No uncommitted changes to review."} - {diffType === 'staged' && "No staged changes. Stage some files with git add."} - {diffType === 'unstaged' && "No unstaged changes. All changes are staged."} - {diffType === 'last-commit' && "No changes in the last commit."} - {diffType === 'branch' && `No changes between this branch and ${gitContext?.defaultBranch || 'main'}.`} - {diffType?.startsWith('worktree:') && ( - diffType.endsWith(':last-commit') ? "No changes in the last commit in this worktree." : - diffType.endsWith(':branch') ? `No changes vs ${gitContext?.defaultBranch || 'main'} in this worktree.` : - "No uncommitted changes in this worktree." - )} + {activeDiffBase === 'uncommitted' && `No uncommitted changes${activeWorktreePath ? ' in this worktree' : ' to review'}.`} + {activeDiffBase === 'staged' && "No staged changes. Stage some files with git add."} + {activeDiffBase === 'unstaged' && "No unstaged changes. All changes are staged."} + {activeDiffBase === 'last-commit' && `No changes in the last commit${activeWorktreePath ? ' in this worktree' : ''}.`} + {activeDiffBase === 'branch' && `No changes vs ${gitContext?.defaultBranch || 'main'}${activeWorktreePath ? ' in this worktree' : ''}.`}

)} diff --git a/packages/review-editor/components/FileTree.tsx b/packages/review-editor/components/FileTree.tsx index 4c283118..bd0e540d 100644 --- a/packages/review-editor/components/FileTree.tsx +++ b/packages/review-editor/components/FileTree.tsx @@ -1,5 +1,6 @@ import React, { useEffect, useCallback, useState, useMemo } from 'react'; import { CodeAnnotation } from '@plannotator/ui/types'; +import type { DiffOption, WorktreeInfo } from '@plannotator/shared/types'; import { buildFileTree, getAncestorPaths, getAllFolderPaths } from '../utils/buildFileTree'; import { FileTreeNodeItem } from './FileTreeNode'; @@ -11,11 +12,6 @@ interface DiffFile { deletions: number; } -interface DiffOption { - id: string; - label: string; -} - interface FileTreeProps { files: DiffFile[]; activeFileIndex: number; @@ -31,6 +27,10 @@ interface FileTreeProps { onSelectDiff?: (diffType: string) => void; isLoadingDiff?: boolean; width?: number; + worktrees?: WorktreeInfo[]; + activeWorktreePath?: string | null; + onSelectWorktree?: (path: string | null) => void; + currentBranch?: string; } export const FileTree: React.FC = ({ @@ -48,6 +48,10 @@ export const FileTree: React.FC = ({ onSelectDiff, isLoadingDiff, width, + worktrees, + activeWorktreePath, + onSelectWorktree, + currentBranch, }) => { // Keyboard navigation: j/k or arrow keys const handleKeyDown = useCallback((e: KeyboardEvent) => { @@ -163,9 +167,43 @@ export const FileTree: React.FC = ({ - {/* Diff selector dropdown - between header and file list */} + {/* Worktree context switcher — only shown when worktrees exist */} + {worktrees && worktrees.length > 0 && onSelectWorktree && ( +
+
Context
+
+ +
+ + + +
+
+
+ )} + + {/* Diff type selector — always the same base options */} {diffOptions && diffOptions.length > 0 && onSelectDiff && ( -
+
+ {worktrees && worktrees.length > 0 && ( +
View
+ )}
- {/* Dropdown arrow */}
{isLoadingDiff ? ( diff --git a/packages/review-editor/package.json b/packages/review-editor/package.json index f3e43e1b..a41165cd 100644 --- a/packages/review-editor/package.json +++ b/packages/review-editor/package.json @@ -7,6 +7,7 @@ "./styles": "./index.css" }, "dependencies": { + "@plannotator/shared": "workspace:*", "@plannotator/ui": "workspace:*", "@pierre/diffs": "^1.1.0-beta.19", "highlight.js": "^11.11.1", diff --git a/packages/server/git.ts b/packages/server/git.ts index 59cf8a0d..1eb62837 100644 --- a/packages/server/git.ts +++ b/packages/server/git.ts @@ -7,6 +7,8 @@ import { $ } from "bun"; +export type { DiffOption, WorktreeInfo, GitContext } from "@plannotator/shared/types"; + export type DiffType = | "uncommitted" | "staged" @@ -15,23 +17,6 @@ export type DiffType = | "branch" | `worktree:${string}`; -export interface DiffOption { - id: DiffType | "separator"; - label: string; -} - -export interface WorktreeInfo { - path: string; - branch: string | null; // null = detached HEAD - head: string; -} - -export interface GitContext { - currentBranch: string; - defaultBranch: string; - diffOptions: DiffOption[]; -} - export interface DiffResult { patch: string; label: string; @@ -137,7 +122,7 @@ export async function getGitContext(): Promise { diffOptions.push({ id: "branch", label: `vs ${defaultBranch}` }); } - // Discover worktrees and add them as diff options + // Discover worktrees (exposed separately from diff options) const [worktrees, currentTreePath] = await Promise.all([ getWorktrees(), $`git rev-parse --show-toplevel`.quiet().then(r => r.text().trim()).catch(() => null), @@ -145,17 +130,7 @@ export async function getGitContext(): Promise { const otherWorktrees = worktrees.filter(wt => wt.path !== currentTreePath); - if (otherWorktrees.length > 0) { - diffOptions.push({ id: "separator", label: "" }); - for (const wt of otherWorktrees) { - const label = wt.branch - ? `${wt.branch} (worktree)` - : `${wt.path.split("/").pop()} (worktree)`; - diffOptions.push({ id: `worktree:${wt.path}`, label }); - } - } - - return { currentBranch, defaultBranch, diffOptions }; + return { currentBranch, defaultBranch, diffOptions, worktrees: otherWorktrees }; } /** @@ -214,20 +189,6 @@ export function parseWorktreeDiffType(diffType: string): { path: string; subType return { path: rest, subType: "uncommitted" }; } -/** - * Build diff options for worktree mode: back-to-main, separator, then - * standard diff types with the worktree path baked into each id. - */ -export function getWorktreeDiffOptions(worktreePath: string, defaultBranch: string): DiffOption[] { - const prefix = `worktree:${worktreePath}:`; - return [ - { id: "back-to-main" as DiffType, label: "\u2190 Back to main repo" }, - { id: "separator", label: "" }, - { id: `${prefix}uncommitted` as DiffType, label: "Uncommitted changes" }, - { id: `${prefix}last-commit` as DiffType, label: "Last commit" }, - { id: `${prefix}branch` as DiffType, label: `vs ${defaultBranch}` }, - ]; -} /** * Run git diff with the specified type diff --git a/packages/server/review.ts b/packages/server/review.ts index b8173981..670b9591 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -10,7 +10,7 @@ */ import { isRemoteSession, getServerPort } from "./remote"; -import { type DiffType, type GitContext, runGitDiff, getWorktreeDiffOptions, getFileContentsForDiff } from "./git"; +import { type DiffType, type GitContext, runGitDiff, getFileContentsForDiff } from "./git"; import { getRepoInfo } from "./repo"; import { handleImage, handleUpload, handleAgents, handleServerReady, handleDraftSave, handleDraftLoad, handleDraftDelete, type OpencodeClient } from "./shared-handlers"; import { contentHash, deleteDraft } from "./draft"; @@ -19,7 +19,7 @@ import { createEditorAnnotationHandler } from "./editor-annotations"; // Re-export utilities export { isRemoteSession, getServerPort } from "./remote"; export { openBrowser } from "./browser"; -export { type DiffType, type DiffOption, type GitContext } from "./git"; +export { type DiffType, type DiffOption, type GitContext, type WorktreeInfo } from "./git"; export { handleServerReady as handleReviewServerReady } from "./shared-handlers"; // --- Types --- @@ -153,13 +153,6 @@ export async function startReviewServer( } const defaultBranch = gitContext?.defaultBranch || "main"; - const previousDiffType = currentDiffType; - - // Handle "back to main repo" sentinel - const isBackToMain = newDiffType === ("back-to-main" as DiffType); - if (isBackToMain) { - newDiffType = "uncommitted" as DiffType; - } // Run the new diff const result = await runGitDiff(newDiffType, defaultBranch); @@ -170,27 +163,12 @@ export async function startReviewServer( currentDiffType = newDiffType; currentError = result.error; - // Build response — include diffOptions on worktree mode transitions - const response: Record = { + return Response.json({ rawPatch: currentPatch, gitRef: currentGitRef, diffType: currentDiffType, ...(currentError && { error: currentError }), - }; - - if (isBackToMain) { - // Exiting worktree mode: restore original dropdown options - response.diffOptions = gitContext?.diffOptions; - } else if ( - newDiffType.startsWith("worktree:") && - !previousDiffType.startsWith("worktree:") - ) { - // Entering worktree mode: send worktree-scoped dropdown options - const wtPath = newDiffType.slice("worktree:".length).replace(/:(?:uncommitted|last-commit|branch)$/, ""); - response.diffOptions = getWorktreeDiffOptions(wtPath, defaultBranch); - } - - return Response.json(response); + }); } catch (err) { const message = err instanceof Error ? err.message : "Failed to switch diff"; diff --git a/packages/shared/types.ts b/packages/shared/types.ts index c5451579..2d21c2d9 100644 --- a/packages/shared/types.ts +++ b/packages/shared/types.ts @@ -8,3 +8,22 @@ export interface EditorAnnotation { comment?: string; createdAt: number; } + +// Git diff types shared between server and client +export interface DiffOption { + id: string; + label: string; +} + +export interface WorktreeInfo { + path: string; + branch: string | null; + head: string; +} + +export interface GitContext { + currentBranch: string; + defaultBranch: string; + diffOptions: DiffOption[]; + worktrees: WorktreeInfo[]; +} diff --git a/tests/manual/test-worktree-review.ts b/tests/manual/test-worktree-review.ts index 072c7284..ea7cdbec 100644 --- a/tests/manual/test-worktree-review.ts +++ b/tests/manual/test-worktree-review.ts @@ -14,12 +14,11 @@ * What to test: * * WORKTREE FEATURES: - * 1. Dropdown shows worktrees below a separator - * 2. Selecting a worktree enters "worktree mode" — dropdown shows - * Back to main / Uncommitted / Last commit / vs main - * 3. "Last commit" in feature-auth shows the committed auth module - * 4. "Uncommitted" in feature-auth shows only the rate-limit file - * 5. "Back to main repo" restores the original dropdown + * 1. Context dropdown appears above View dropdown, listing available worktrees + * 2. Selecting a worktree in Context switches files; pill gets highlighted border + * 3. View dropdown (Uncommitted/Last commit/vs main) stays the same in any context + * 4. Can switch directly between worktrees without going "back to main" first + * 5. Selecting main branch in Context restores the main repo view * 6. Empty worktree shows appropriate empty state messages * 7. Detached HEAD worktree uses directory name as label * @@ -1075,12 +1074,11 @@ const { patch: rawPatch, label: gitRef, error: diffError } = await runGitDiff( console.error("Git context discovered:"); console.error(` Current branch: ${gitContext.currentBranch}`); console.error(` Default branch: ${gitContext.defaultBranch}`); -console.error(` Diff options:`); -for (const opt of gitContext.diffOptions) { - if (opt.id === "separator") { - console.error(` ────────────────`); - } else { - console.error(` ${opt.label} (${opt.id})`); +console.error(` Diff options: ${gitContext.diffOptions.map(o => o.label).join(', ')}`); +if (gitContext.worktrees.length > 0) { + console.error(` Worktrees:`); + for (const wt of gitContext.worktrees) { + console.error(` ${wt.branch || wt.path.split('/').pop()} (${wt.path})`); } } console.error(""); @@ -1089,13 +1087,14 @@ console.error("Starting review server..."); console.error("Browser should open automatically."); console.error(""); console.error("=== WORKTREE TESTS ==="); -console.error(" 1. Dropdown shows worktrees below a separator"); -console.error(" 2. Select 'feature-auth' → worktree mode (Back / Uncommitted / Last commit / vs main)"); -console.error(" 3. 'Uncommitted' shows rate-limit.ts"); -console.error(" 4. 'Last commit' shows committed auth module (5 files)"); -console.error(" 5. 'Back to main repo' restores original dropdown"); -console.error(" 6. 'empty-branch' → worktree mode, all options empty"); -console.error(" 7. 'wt-detached' → label uses directory name, not branch"); +console.error(" 1. 'Context' dropdown appears above 'View' dropdown listing worktrees"); +console.error(" 2. Select 'feature-auth' in Context → highlighted pill, files update"); +console.error(" 3. 'Uncommitted' in View shows rate-limit.ts"); +console.error(" 4. 'Last commit' in View shows committed auth module (5 files)"); +console.error(" 5. Switch Context back to main branch → restores main repo files"); +console.error(" 6. Switch directly between worktrees without returning to main"); +console.error(" 7. 'empty-branch' context → all View options show empty state"); +console.error(" 8. Detached HEAD worktree uses directory name as label"); console.error(""); console.error("=== EXPANDABLE DIFF CONTEXT TESTS ==="); console.error(" 8. registry.ts — 4 disjoint hunks. Between each pair, you should see");