From c9f572530397023374b61427023cb186bde1a9f1 Mon Sep 17 00:00:00 2001 From: Daniel Schwarz Date: Wed, 29 Jul 2026 11:24:21 +0200 Subject: [PATCH] Fix desktop Git client audit regressions --- README.md | 3 +- ROADMAP.md | 8 ++ TASKS.md | 27 +++++ crates/strand-core/src/history.rs | 33 +++++- crates/strand-core/src/refs.rs | 65 ++++++++++- crates/strand-core/src/stash.rs | 3 +- docs/learnings.md | 19 ++++ ui/src/components/Sidebar.tsx | 27 +++++ ui/src/lib/commitShortcut.test.ts | 46 ++++++++ ui/src/lib/commitShortcut.ts | 12 ++ ui/src/lib/db.ts | 104 +++++++++++++++++- ui/src/lib/reviewExport.test.ts | 70 ++++++++++++ ui/src/lib/reviewExport.ts | 21 ++++ ui/src/stores/repo.test.ts | 73 +++++++++++++ ui/src/stores/repo.ts | 102 +++++++++++++++-- ui/src/stores/workspaceReview.ts | 41 ++++++- ui/src/styles/features.css | 1 - ui/src/views/CloneDialog.test.ts | 60 ++++++++++ ui/src/views/CloneDialog.tsx | 45 +++++++- ui/src/views/LocalChanges.tsx | 13 ++- ui/src/views/Work.test.ts | 27 +++++ ui/src/views/WorkspaceManagerDialog.test.ts | 71 ++++++++++++ ui/src/views/WorkspaceManagerDialog.tsx | 115 +++++++++++++++++--- ui/src/views/Worktrees.tsx | 9 +- website/docs/everyday-git.md | 6 +- website/docs/repositories-and-workspaces.md | 2 + website/docs/reviewing-agent-changes.md | 3 + 27 files changed, 943 insertions(+), 63 deletions(-) create mode 100644 ui/src/lib/commitShortcut.test.ts create mode 100644 ui/src/lib/commitShortcut.ts create mode 100644 ui/src/stores/repo.test.ts create mode 100644 ui/src/views/CloneDialog.test.ts create mode 100644 ui/src/views/Work.test.ts create mode 100644 ui/src/views/WorkspaceManagerDialog.test.ts diff --git a/README.md b/README.md index 4bd3493..8ffeed0 100644 --- a/README.md +++ b/README.md @@ -83,7 +83,8 @@ and the live Diff settings preview. reviewed, a pinnable baseline captures everything since a commit — including work the agent already staged or committed — and a change map beside the scrollbar shows where every edit sits in the file (click to - jump). + jump). Inline feedback notes persist with their baseline/branch comparison, + so switching review targets never mixes two agents' feedback. - **Hosted pull requests** — browse the latest 100 GitHub or Azure DevOps PRs for the active repository, with the active PR for your checked-out branch opening and being followed automatically even before the PR view is opened. diff --git a/ROADMAP.md b/ROADMAP.md index 34d8496..1343da2 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2434,6 +2434,14 @@ certificate is no longer a release requirement and remains relevant only if the unmanaged MSI/EXE fallback is promoted. The normal GitHub MSI remains truthfully documented as unsigned. +**Desktop Git-client audit hardening shipped (2026-07-29):** A seven-section +macOS audit repaired embedded-terminal compositing, clone-dialog focus, +commit-form submission, stale workspace candidates, repository/worktree +refreshes and accessibility, comparison-scoped review notes, and sequencer +error reporting. Selective-stash messages now have explicit regression +coverage. The fixes retain snapshot-driven refreshes rather than adding +polling or recursive scans to hot paths. + --- ## Cross-cutting tracks (run in parallel with all milestones) diff --git a/TASKS.md b/TASKS.md index 1cf47ea..462b4b3 100644 --- a/TASKS.md +++ b/TASKS.md @@ -433,6 +433,33 @@ Detailed comparison and sequencing: [`docs/git-client-1.0-audit.md`](./docs/git- ## Frontend — components & wiring +### 2026-07-29 desktop audit hardening +- ☑ Work terminals remain composited under the stable runtime layer while + inactive panes alone are hidden (`features.css`, `Work.test.ts`). +- ☑ Clone opened from Quick Launch reclaims URL-field focus after palette + teardown and restores the real underlying opener on close + (`startCloneDialogFocusLifecycle`). +- ☑ `Mod+Enter` submits from both commit subject and description while plain + Enter remains available for body newlines (`handleCommitShortcut`). +- ☑ Workspace Manager validates persisted recents in parallel, drops missing + repositories, canonicalizes valid paths, and still includes open main tabs + (`validateRecentRepositories`, `mergeKnownRepositories`). +- ☑ Repository/worktree state refreshes after checkout, detach, checked-out + branch creation, and rename; Files consumes the authoritative snapshot + without another ignored-tree scan; worktree stats rerun on explicit list + refresh; historical file context clears outside history views + (`repo.ts`, `Sidebar`, `Worktrees`). +- ☑ Review notes persist per baseline/ref comparison, migrate the legacy + per-repository map lazily, and stay synchronized only between repo/workspace + lenses showing the same scope (`reviewNoteScope`, review-session v2). +- ☑ Worktree base detection handles fresh equal-tip primary branches without + preferring later-created peers; worktree row actions remain exposed to + accessibility APIs; non-conflict cherry-pick/signing failures surface their + real Git error instead of a false conflict pause (`detect_base_branch`, + `run_sequencer_env`, list/listitem semantics). +- ☑ Selective-stash message forwarding is regression-covered end to end + (`stash_push_paths_preserves_message_and_selection`). + ### Repo opening - ☑ Create a new local repository (initial branch + optional `.gitignore` / first commit), as required by PRD §6.1 P0 (`init_repository`, `repo_init`, diff --git a/crates/strand-core/src/history.rs b/crates/strand-core/src/history.rs index 5b2217e..7b3a02e 100644 --- a/crates/strand-core/src/history.rs +++ b/crates/strand-core/src/history.rs @@ -399,7 +399,12 @@ impl Repo { match run_git_env(&self.path, args, envs) { Ok(_) => Ok(self.operation_in_progress().is_some()), Err(e) => { - if self.has_conflicts().unwrap_or(false) || self.operation_in_progress().is_some() { + // A conflict is the expected paused outcome. Git can also + // leave CHERRY_PICK_HEAD/REVERT_HEAD behind after a *real* + // commit failure (for example, signing failed). Treating the + // marker alone as success hides that error behind a + // misleading "Ready to continue" banner. + if self.has_conflicts().unwrap_or(false) { Ok(true) } else { Err(e) @@ -707,6 +712,32 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + #[test] + fn cherry_pick_surfaces_a_commit_failure_instead_of_reporting_a_pause() { + let (repo, dir) = scratch_repo(); + write_commit(&dir, "base.txt", "base\n", "base"); + git(&dir, &["checkout", "-q", "-b", "feature"]); + let pick = write_commit(&dir, "only-feature.txt", "x\n", "add only-feature"); + git(&dir, &["checkout", "-q", "main"]); + + // Simulate a non-interactive signing failure during cherry-pick's + // commit step. Git applies/stages the change and leaves + // CHERRY_PICK_HEAD, but there is no conflict: this is a genuine error + // the UI must show, not `Ok(true)` / "Ready to continue". + git(&dir, &["config", "commit.gpgsign", "true"]); + git(&dir, &["config", "gpg.format", "openpgp"]); + git(&dir, &["config", "gpg.program", "false"]); + let err = repo.cherry_pick(&[pick], None).unwrap_err().to_string(); + assert!(err.contains("failed to sign") || err.contains("gpg failed")); + assert_eq!(repo.meta().unwrap().operation.as_deref(), Some("cherry-pick")); + assert!(!repo.has_conflicts().unwrap()); + + // Restore the repo before cleanup so this test also exercises the + // normal recovery marker path. + repo.abort_operation().unwrap(); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn merge_commit_mainline_supports_cherry_pick_and_revert() { let (repo, dir) = scratch_repo(); diff --git a/crates/strand-core/src/refs.rs b/crates/strand-core/src/refs.rs index 2842561..7698e18 100644 --- a/crates/strand-core/src/refs.rs +++ b/crates/strand-core/src/refs.rs @@ -154,11 +154,15 @@ impl Repo { // `target` since the merge-base (fewer = forked later = closer // parent), tie-break on commits the candidate has since the // merge-base (a branch still sitting at the fork point beats a - // sibling that moved on), then name for determinism. Candidates that + // sibling that moved on), then prefer the primary branch before name + // for determinism. Candidates that // *contain* target (merge-base = target tip, i.e. children or // already-merged integration branches) rank last — pinning the - // baseline at target's own tip would review nothing. - let mut best: Option<((bool, usize, usize), BaseBranch)> = None; + // baseline at target's own tip would review nothing. Equal-tip peers + // are also ambiguous unless they are the primary branch: they may + // have been created from `target` later rather than being its parent. + let primary = primary_branch(repo).map(|(name, _)| name); + let mut best: Option<((bool, usize, usize, bool), BaseBranch)> = None; if let Ok(branches) = repo.branches(Some(git2::BranchType::Local)) { for (branch, _) in branches.flatten() { let name = match branch.name() { @@ -169,7 +173,20 @@ impl Repo { let Ok(mb) = repo.merge_base(target_id, tip) else { continue }; let Ok((ahead, _)) = repo.graph_ahead_behind(target_id, mb) else { continue }; let Ok((base_ahead, _)) = repo.graph_ahead_behind(tip, mb) else { continue }; - let rank = (ahead == 0, ahead, base_ahead); + // The primary branch at the same tip is the strongest + // fallback for a fresh branch created from HEAD. A non-primary + // equal-tip branch is only a peer and may have been created + // *after* target, so it belongs with strict descendants at the + // back of the ranking. + let is_primary = primary.as_deref() == Some(name.as_str()); + let ambiguous_child_or_peer = + mb == target_id && (tip != target_id || !is_primary); + let rank = ( + ambiguous_child_or_peer, + ahead, + base_ahead, + !is_primary, + ); let better = match &best { None => true, Some((r, c)) => (rank, name.as_str()) < (*r, c.name.as_str()), @@ -587,6 +604,46 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn detect_base_branch_treats_an_equal_tip_as_a_fresh_branch_parent() { + let dir = std::env::temp_dir().join(format!( + "strand-detect-fresh-head-test-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + git(&dir, &["init", "-q", "-b", "main"]); + git(&dir, &["config", "user.name", "Test"]); + git(&dir, &["config", "user.email", "test@example.com"]); + git(&dir, &["config", "commit.gpgsign", "false"]); + git(&dir, &["config", "core.logAllRefUpdates", "true"]); + + std::fs::write(dir.join("root.txt"), "root\n").unwrap(); + git(&dir, &["add", "root.txt"]); + git(&dir, &["commit", "-q", "-m", "root"]); + let root = git(&dir, &["rev-parse", "HEAD"]); + std::fs::write(dir.join("main.txt"), "main\n").unwrap(); + git(&dir, &["add", "main.txt"]); + git(&dir, &["commit", "-q", "-m", "main tip"]); + let main_tip = git(&dir, &["rev-parse", "HEAD"]); + + // `git branch fresh` records "Created from HEAD", not the parent's + // name. Neither an older nor an equal-tip alphabetically-first sibling + // may beat the primary branch in the fallback scan. + git(&dir, &["branch", "aaa-old", &root]); + git(&dir, &["branch", "aaa-peer"]); + git(&dir, &["branch", "fresh"]); + + let repo = Repo::discover(dir.to_str().unwrap()).unwrap(); + let hit = repo.detect_base_branch("fresh").unwrap().unwrap(); + assert_eq!(hit.name, "main"); + assert_eq!(hit.merge_base, main_tip); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn refs_marks_only_branches_merged_into_primary_branch() { let dir = std::env::temp_dir().join(format!( diff --git a/crates/strand-core/src/stash.rs b/crates/strand-core/src/stash.rs index ec986d4..0c4ce6b 100644 --- a/crates/strand-core/src/stash.rs +++ b/crates/strand-core/src/stash.rs @@ -350,7 +350,7 @@ mod tests { } #[test] - fn stash_push_paths_partial() { + fn stash_push_paths_preserves_message_and_selection() { let dir = std::env::temp_dir().join(format!("strand-stash-paths-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); @@ -384,6 +384,7 @@ mod tests { let stashes = repo.stash_list().unwrap(); assert_eq!(stashes.len(), 1); + assert_eq!(stashes[0].message, "On main: partial"); let _ = std::fs::remove_dir_all(&dir); } diff --git a/docs/learnings.md b/docs/learnings.md index a443ae4..f3ac1e5 100644 --- a/docs/learnings.md +++ b/docs/learnings.md @@ -2095,3 +2095,22 @@ directory against the wrapper's known file set, deduplicate overlaps by iterating that set once, and keep an action on an unselected context row scoped to that row. `expandTreeSelection` / `resolveTreeActionTargets` own this boundary; preserve the single batched IPC call after expansion. + +**A persistent terminal host must stay compositable (2026-07-29).** A stable +xterm runtime layer can preserve PTYs, DOM nodes, and input while still making +every renderer invisible if an ancestor uses `visibility: hidden`. Keep the +runtime ancestor visible and hide only inactive terminal panes; test both +halves of that CSS contract. + +**Nested modal launchers can race focus restoration (2026-07-29).** When a +command palette launches a modal, its unmount cleanup may restore the palette +opener after the modal's native `autoFocus` runs. Reclaim modal focus on the +next animation frame, remember any control restored behind it, and return focus +to that real opener when the modal closes. + +**Equal-tip branches are peers unless provenance says otherwise +(2026-07-29).** Git may record a new branch as merely “Created from HEAD,” so +base detection needs an equal-tip fallback. Prefer the primary branch at the +same tip; do not treat arbitrary equal-tip siblings as parents because they may +have been created from the target later. An explicitly named reflog parent +still wins. diff --git a/ui/src/components/Sidebar.tsx b/ui/src/components/Sidebar.tsx index 5079c01..32777a2 100644 --- a/ui/src/components/Sidebar.tsx +++ b/ui/src/components/Sidebar.tsx @@ -429,6 +429,33 @@ export function Sidebar({ onOpenRepo, onOpenRecent, onCreateStash, onCreateTag, const [treeError, setTreeError] = useState(null); const mutationTargetsRepo = filesTreeMutation?.repoPath === meta?.path; const workingTreeRevision = selectedCommit || !mutationTargetsRepo ? 0 : filesTreeRevision; + // `repo_snapshot` already carries the authoritative tracked/untracked + // listing after commits, checkouts, and watcher events. Fold that cheap list + // into the ignored-inclusive Files cache instead of waiting for a tab switch + // (or paying for another recursive ignored scan). + useEffect(() => { + const repoPath = meta?.path; + if (!repoPath) return; + setLocalTreeCache((current) => { + if (!current || current.repoPath !== repoPath) return current; + const entries = new Map( + current.entries.filter((entry) => entry.ignored).map((entry) => [entry.path, entry]), + ); + for (const entry of workTree) entries.set(entry.path, entry); + const next = [...entries.values()].sort((a, b) => a.path.localeCompare(b.path)); + const unchanged = + next.length === current.entries.length && + next.every((entry, i) => { + const prev = current.entries[i]; + return ( + prev?.path === entry.path && + prev.status === entry.status && + prev.ignored === entry.ignored + ); + }); + return unchanged ? current : { ...current, entries: next }; + }); + }, [meta?.path, workTree]); useEffect(() => { setEmptyDirectories(new Set()); loadedIgnoredDirectoriesRef.current.clear(); diff --git a/ui/src/lib/commitShortcut.test.ts b/ui/src/lib/commitShortcut.test.ts new file mode 100644 index 0000000..4d5ac6e --- /dev/null +++ b/ui/src/lib/commitShortcut.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { handleCommitShortcut, type CommitShortcutEvent } from './commitShortcut'; + +const event = (over: Partial = {}): CommitShortcutEvent => ({ + key: '', + metaKey: false, + ctrlKey: false, + preventDefault: vi.fn(), + ...over, +}); + +describe('handleCommitShortcut', () => { + it.each([ + ['Command', { metaKey: true }], + ['Control', { ctrlKey: true }], + ])('submits and prevents the default for %s+Enter', (_label, modifier) => { + const e = event({ key: 'Enter', ...modifier }); + const submit = vi.fn(); + + handleCommitShortcut(e, submit); + + expect(e.preventDefault).toHaveBeenCalledOnce(); + expect(submit).toHaveBeenCalledOnce(); + }); + + it('leaves plain Enter untouched so a description can insert a newline', () => { + const e = event({ key: 'Enter' }); + const submit = vi.fn(); + + handleCommitShortcut(e, submit); + + expect(e.preventDefault).not.toHaveBeenCalled(); + expect(submit).not.toHaveBeenCalled(); + }); + + it('leaves other modified keys untouched', () => { + const e = event({ key: 'k', metaKey: true }); + const submit = vi.fn(); + + handleCommitShortcut(e, submit); + + expect(e.preventDefault).not.toHaveBeenCalled(); + expect(submit).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/src/lib/commitShortcut.ts b/ui/src/lib/commitShortcut.ts new file mode 100644 index 0000000..690ee18 --- /dev/null +++ b/ui/src/lib/commitShortcut.ts @@ -0,0 +1,12 @@ +export interface CommitShortcutEvent { + key: string; + metaKey: boolean; + ctrlKey: boolean; + preventDefault: () => void; +} + +export function handleCommitShortcut(event: CommitShortcutEvent, submit: () => void) { + if (event.key !== 'Enter' || (!event.metaKey && !event.ctrlKey)) return; + event.preventDefault(); + submit(); +} diff --git a/ui/src/lib/db.ts b/ui/src/lib/db.ts index b418e20..c6d93b3 100644 --- a/ui/src/lib/db.ts +++ b/ui/src/lib/db.ts @@ -115,6 +115,74 @@ export interface StoredBaseline { setAt: number; } +type ReviewNotes = Record; + +/** Versioned envelope for notes from independent review comparisons. */ +interface StoredReviewNotesV2 { + version: 2; + activeScope: string; + scopes: Record; + /** Most-recent first; bounds abandoned review sessions in SQLite. */ + order: string[]; +} + +const MAX_REVIEW_NOTE_SCOPES = 12; + +function isStoredReviewNotesV2(value: unknown): value is StoredReviewNotesV2 { + if (value == null || typeof value !== 'object') return false; + const candidate = value as Partial; + return ( + candidate.version === 2 && + typeof candidate.activeScope === 'string' && + candidate.scopes != null && + typeof candidate.scopes === 'object' && + Array.isArray(candidate.order) + ); +} + +/** + * Read one comparison's notes from the persisted value. A pre-v2 plain map is + * treated as belonging to the first scope opened after upgrade; the caller + * then writes the returned migration so existing notes are preserved. + */ +export function readReviewNotesForScope( + value: unknown, + scope: string, +): { notes: ReviewNotes; migration: StoredReviewNotesV2 | null } { + if (isStoredReviewNotesV2(value)) { + return { notes: value.scopes[scope] ?? {}, migration: null }; + } + const notes = + value != null && typeof value === 'object' ? (value as ReviewNotes) : {}; + return { + notes, + migration: { + version: 2, + activeScope: scope, + scopes: { [scope]: notes }, + order: [scope], + }, + }; +} + +/** Replace one scope without disturbing notes saved for other comparisons. */ +export function writeReviewNotesForScope( + value: unknown, + scope: string, + notes: ReviewNotes, +): StoredReviewNotesV2 { + const current = isStoredReviewNotesV2(value) + ? value + : readReviewNotesForScope(value, scope).migration!; + const order = [scope, ...current.order.filter((key) => key !== scope)] + .slice(0, MAX_REVIEW_NOTE_SCOPES); + const scopes = { ...current.scopes, [scope]: notes }; + for (const key of Object.keys(scopes)) { + if (!order.includes(key)) delete scopes[key]; + } + return { version: 2, activeScope: scope, scopes, order }; +} + /** * Per-repo review session state: the pinned baseline and the reviewed-file * map (`path → hash of the reviewed diff`). Persisted so an app restart @@ -134,11 +202,37 @@ export const reviewSession = { setReviewed(repoPath: string, reviewed: Record): Promise { return settings.set(`reviewed:${repoPath}`, reviewed); }, - getNotes(repoPath: string): Promise | null> { - return settings.get>(`review-notes:${repoPath}`); - }, - setNotes(repoPath: string, notes: Record): Promise { - return settings.set(`review-notes:${repoPath}`, notes); + async getNotes(repoPath: string, scope?: string): Promise { + const key = `review-notes:${repoPath}`; + const stored = await settings.get(key); + if (!scope) { + if (isStoredReviewNotesV2(stored)) { + return stored.scopes[stored.activeScope] ?? {}; + } + return stored as ReviewNotes | null; + } + if (stored == null) return null; + const { notes, migration } = readReviewNotesForScope(stored, scope); + if (migration) await settings.set(key, migration); + return notes; + }, + async setNotes(repoPath: string, notes: ReviewNotes, scope?: string): Promise { + const key = `review-notes:${repoPath}`; + const stored = await settings.get(key); + if (scope) { + await settings.set(key, writeReviewNotesForScope(stored, scope, notes)); + return; + } + // Compatibility for callers not yet supplying a scope: once a v2 record + // exists, update its active bucket instead of flattening it back to v1. + if (isStoredReviewNotesV2(stored)) { + await settings.set( + key, + writeReviewNotesForScope(stored, stored.activeScope, notes), + ); + return; + } + await settings.set(key, notes); }, }; diff --git a/ui/src/lib/reviewExport.test.ts b/ui/src/lib/reviewExport.test.ts index f86105d..2755eb8 100644 --- a/ui/src/lib/reviewExport.test.ts +++ b/ui/src/lib/reviewExport.test.ts @@ -4,7 +4,12 @@ import { buildReviewFeedback, buildWorkspaceReviewFeedback, collectFeedbackFiles, + reviewNoteScope, } from './reviewExport'; +import { + readReviewNotesForScope, + writeReviewNotesForScope, +} from './db'; import type { ReviewNote } from './types'; let seq = 0; @@ -302,3 +307,68 @@ describe('collectFeedbackFiles', () => { expect(collectFeedbackFiles(pool, { 'a.ts': [] })).toEqual([]); }); }); + +describe('review note scopes', () => { + const mainScope = reviewNoteScope({ + baselineOid: 'base-a', + branch: 'main', + detached: false, + headOid: 'head-1', + }); + const featureScope = reviewNoteScope({ + baselineOid: 'base-a', + branch: 'feature', + detached: false, + headOid: 'head-2', + }); + + it('changes for a new baseline or ref, but not when the same branch advances', () => { + expect( + reviewNoteScope({ + baselineOid: 'base-a', + branch: 'main', + detached: false, + headOid: 'head-2', + }), + ).toBe(mainScope); + expect(featureScope).not.toBe(mainScope); + expect( + reviewNoteScope({ + baselineOid: 'base-b', + branch: 'main', + detached: false, + headOid: 'head-1', + }), + ).not.toBe(mainScope); + }); + + it('uses the exact HEAD for detached comparisons', () => { + const detached = (headOid: string) => + reviewNoteScope({ + baselineOid: null, + branch: headOid.slice(0, 7), + detached: true, + headOid, + }); + expect(detached('111111111')).not.toBe(detached('222222222')); + }); + + it('migrates legacy notes into the first active scope without losing them', () => { + const legacy = { 'src/a.ts': [note('legacy')] }; + const first = readReviewNotesForScope(legacy, mainScope); + expect(first.notes).toEqual(legacy); + expect(first.migration).not.toBeNull(); + expect(readReviewNotesForScope(first.migration, mainScope).notes).toEqual(legacy); + expect(readReviewNotesForScope(first.migration, featureScope).notes).toEqual({}); + }); + + it('retains independent note buckets and restores them when returning to a scope', () => { + const mainNotes = { 'src/main.ts': [note('main note')] }; + const featureNotes = { 'src/feature.ts': [note('feature note')] }; + const withMain = writeReviewNotesForScope(null, mainScope, mainNotes); + const withFeature = writeReviewNotesForScope(withMain, featureScope, featureNotes); + + expect(readReviewNotesForScope(withFeature, mainScope).notes).toEqual(mainNotes); + expect(readReviewNotesForScope(withFeature, featureScope).notes).toEqual(featureNotes); + }); +}); diff --git a/ui/src/lib/reviewExport.ts b/ui/src/lib/reviewExport.ts index 7c47702..c3f4f85 100644 --- a/ui/src/lib/reviewExport.ts +++ b/ui/src/lib/reviewExport.ts @@ -1,6 +1,27 @@ import { fencedDiff } from './patchExport'; import type { ReviewNote } from './types'; +/** + * Stable identity for one Review comparison. Notes are shared between the + * repo and workspace lenses only while they are looking at this same scope. + * + * Branch names intentionally identify attached HEADs: moving HEAD forward on + * the same branch keeps an in-progress review together, while checking out a + * different branch does not leak its notes into the new comparison. Detached + * reviews use the exact HEAD OID because there is no stable ref name. + */ +export function reviewNoteScope(input: { + baselineOid: string | null; + branch: string | null; + detached: boolean; + headOid: string | null; +}): string { + const ref = input.detached + ? `detached:${input.headOid ?? input.branch ?? ''}` + : `branch:${input.branch ?? ''}`; + return JSON.stringify([input.baselineOid ?? 'inbox', ref]); +} + /** * Rendering review notes into one Markdown prompt — the hand-back half of the * agent feedback loop: annotate files/lines in the Review view, then paste the diff --git a/ui/src/stores/repo.test.ts b/ui/src/stores/repo.test.ts new file mode 100644 index 0000000..1914ad1 --- /dev/null +++ b/ui/src/stores/repo.test.ts @@ -0,0 +1,73 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const values = new Map(); +const storage: Storage = { + get length() { return values.size; }, + clear: () => values.clear(), + getItem: (key) => values.get(key) ?? null, + key: (index) => [...values.keys()][index] ?? null, + removeItem: (key) => { values.delete(key); }, + setItem: (key, value) => { values.set(key, value); }, +}; +vi.stubGlobal('localStorage', storage); +vi.stubGlobal('window', { localStorage: storage }); +vi.stubGlobal('navigator', { userAgent: '' }); +vi.stubGlobal('document', { documentElement: { dataset: {} } }); + +const { tauri } = await import('../lib/tauri'); +const { useRepo } = await import('./repo'); + +const original = useRepo.getState(); + +afterEach(() => { + useRepo.setState(original, true); + vi.restoreAllMocks(); +}); + +describe('repository navigation state', () => { + it('drops historical Files context when leaving history views', () => { + useRepo.setState({ + view: 'commits', + selectedCommit: 'deadbeef', + selectedCommitDiffs: [{ + path: 'old.txt', + old_path: null, + status: 'modified', + adds: 1, + dels: 0, + patch: '', + binary: false, + }], + selectedCommitDiffsLoading: true, + }); + + useRepo.getState().setView('work'); + + expect(useRepo.getState()).toMatchObject({ + view: 'work', + selectedCommit: null, + selectedCommitDiffs: [], + selectedCommitDiffsLoading: false, + }); + }); + + it('refreshes worktree branch labels after checkout', async () => { + const refreshLocalChanges = vi.fn(async () => {}); + const refreshLog = vi.fn(async () => {}); + const refreshWorktrees = vi.fn(async () => {}); + const checkout = vi.spyOn(tauri, 'repoCheckout').mockResolvedValue({ branch: 'feature' }); + useRepo.setState({ + activePath: '/repo', + refreshLocalChanges, + refreshLog, + refreshWorktrees, + }); + + await useRepo.getState().checkout('feature'); + + expect(checkout).toHaveBeenCalledWith('/repo', 'feature'); + expect(refreshLocalChanges).toHaveBeenCalledOnce(); + expect(refreshLog).toHaveBeenCalledOnce(); + expect(refreshWorktrees).toHaveBeenCalledOnce(); + }); +}); diff --git a/ui/src/stores/repo.ts b/ui/src/stores/repo.ts index 026dd9f..4212353 100644 --- a/ui/src/stores/repo.ts +++ b/ui/src/stores/repo.ts @@ -14,6 +14,7 @@ import { hashPatch } from '../lib/patch'; import { logColdStart, timed } from '../lib/perf'; import { isPreviewablePath } from '../lib/preview'; import { pathKey, repoFamilyName } from '../lib/repoIdentity'; +import { reviewNoteScope } from '../lib/reviewExport'; import { jsonEqual, stable } from '../lib/stable'; import { errMessage, tauri } from '../lib/tauri'; import { useSettings, type DiffMode } from './settings'; @@ -696,6 +697,19 @@ function samePath(a: string, b: string): boolean { return pathKey(a) === pathKey(b); } +/** Scope review notes to the exact comparison currently shown. */ +function activeReviewNoteScope( + state: Pick, + baselineOid = state.baseline?.oid ?? null, +): string { + return reviewNoteScope({ + baselineOid, + branch: state.meta?.branch ?? null, + detached: state.meta?.detached ?? false, + headOid: state.meta?.head_oid ?? null, + }); +} + /** * The remote tag pushes target by default: `remote.pushDefault`, the current * branch's upstream remote, `origin`, or the first configured remote. Tags @@ -1155,18 +1169,46 @@ export const useRepo = create((set, get) => ({ const oid = at ?? get().meta?.head_oid; if (!path || !oid) return; const baseline: StoredBaseline = { oid, short: oid.slice(0, 7), setAt: Date.now() }; + const scope = activeReviewNoteScope(get(), oid); set({ baseline }); void reviewSession.setBaseline(path, baseline).catch((e) => console.warn('baseline persist failed', e)); - await get().refreshReviewDiffs(); + await Promise.all([ + get().refreshReviewDiffs(), + reviewSession + .getNotes(path, scope) + .then((notes) => { + if ( + get().activePath === path && + get().baseline?.oid === oid && + activeReviewNoteScope(get(), oid) === scope + ) { + set({ reviewNotes: notes ?? {} }); + } + }) + .catch((e) => console.warn('review notes load failed', e)), + ]); }, async clearBaseline() { const path = get().activePath; + const scope = activeReviewNoteScope(get(), null); set({ baseline: null, baselineDiffs: [] }); if (path) { void reviewSession.setBaseline(path, null).catch((e) => console.warn('baseline clear failed', e)); + try { + const notes = await reviewSession.getNotes(path, scope); + if ( + get().activePath === path && + get().baseline === null && + activeReviewNoteScope(get(), null) === scope + ) { + set({ reviewNotes: notes ?? {} }); + } + } catch (e) { + console.warn('review notes load failed', e); + } } }, @@ -1202,12 +1244,20 @@ export const useRepo = create((set, get) => ({ const path = get().activePath; if (!path) return; try { - const [baseline, reviewed, notes] = await Promise.all([ + const [baseline, reviewed] = await Promise.all([ reviewSession.getBaseline(path), reviewSession.getReviewed(path), - reviewSession.getNotes(path), ]); if (get().activePath !== path) return; + const effectiveBaseline = baseline ?? null; + const scope = activeReviewNoteScope(get(), effectiveBaseline?.oid ?? null); + const notes = await reviewSession.getNotes(path, scope); + if ( + get().activePath !== path || + activeReviewNoteScope(get(), effectiveBaseline?.oid ?? null) !== scope + ) { + return; + } set({ baseline: baseline ?? null, reviewed: reviewed ?? {}, reviewNotes: notes ?? {} }); if (baseline) await get().refreshReviewDiffs(); } catch (e) { @@ -1235,8 +1285,9 @@ export const useRepo = create((set, get) => ({ if (!path || !note) return; const cur = get().reviewNotes; const next = { ...cur, [file]: [...(cur[file] ?? []), note] }; + const scope = activeReviewNoteScope(get()); set({ reviewNotes: next }); - void reviewSession.setNotes(path, next).catch((e) => + void reviewSession.setNotes(path, next, scope).catch((e) => console.warn('review notes persist failed', e)); }, @@ -1249,16 +1300,18 @@ export const useRepo = create((set, get) => ({ const next = { ...cur }; if (remaining.length === 0) delete next[file]; else next[file] = remaining; + const scope = activeReviewNoteScope(get()); set({ reviewNotes: next }); - void reviewSession.setNotes(path, next).catch((e) => + void reviewSession.setNotes(path, next, scope).catch((e) => console.warn('review notes persist failed', e)); }, clearReviewNotes() { const path = get().activePath; if (!path) return; + const scope = activeReviewNoteScope(get()); set({ reviewNotes: {} }); - void reviewSession.setNotes(path, {}).catch((e) => + void reviewSession.setNotes(path, {}, scope).catch((e) => console.warn('review notes persist failed', e)); }, @@ -1741,13 +1794,21 @@ export const useRepo = create((set, get) => ({ const path = get().activePath; if (!path) throw new Error('no repo open'); await tauri.repoCheckout(path, branch); - await Promise.all([get().refreshLocalChanges(), get().refreshLog()]); + await Promise.all([ + get().refreshLocalChanges(), + get().refreshLog(), + get().refreshWorktrees(), + ]); }, async checkoutCommit(rev) { const path = get().activePath; if (!path) throw new Error('no repo open'); await tauri.repoCheckoutCommit(path, rev); - await Promise.all([get().refreshLocalChanges(), get().refreshLog()]); + await Promise.all([ + get().refreshLocalChanges(), + get().refreshLog(), + get().refreshWorktrees(), + ]); }, async createBranch(name, startPoint, checkout) { const path = get().activePath; @@ -1755,7 +1816,9 @@ export const useRepo = create((set, get) => ({ await tauri.repoBranchCreate(path, name, startPoint, checkout); await Promise.all([ get().refreshSnapshot(), - ...(checkout ? [get().refreshDiffs(), get().refreshLog()] : []), + ...(checkout + ? [get().refreshDiffs(), get().refreshLog(), get().refreshWorktrees()] + : []), ]); }, async deleteBranch(name, force) { @@ -1777,8 +1840,13 @@ export const useRepo = create((set, get) => ({ const path = get().activePath; if (!path) throw new Error('no repo open'); await tauri.repoBranchRename(path, oldName, newName); - // Refs ride along in the snapshot; the graph's ref chips read the log. - await Promise.all([get().refreshLocalChanges(), get().refreshLog()]); + // Refs ride along in the snapshot; the graph's ref chips read the log and + // the family worktree list owns the checked-out branch labels. + await Promise.all([ + get().refreshLocalChanges(), + get().refreshLog(), + get().refreshWorktrees(), + ]); }, async setBranchUpstream(branch, upstream) { const path = get().activePath; @@ -2151,7 +2219,17 @@ export const useRepo = create((set, get) => ({ await get().refreshRecents(); }, - setView: (view) => set({ view, ...(view === 'commits' ? {} : { workFileReturn: null }) }), + setView: (view) => set({ + view, + ...(view === 'commits' ? {} : { workFileReturn: null }), + ...(view === 'commits' || view === 'branch' + ? {} + : { + selectedCommit: null, + selectedCommitDiffs: [], + selectedCommitDiffsLoading: false, + }), + }), setWorkFileReturn: (workFileReturn) => set({ workFileReturn }), revealInGraph: (hash) => set({ view: 'commits', revealCommit: hash }), clearReveal: () => set({ revealCommit: null }), diff --git a/ui/src/stores/workspaceReview.ts b/ui/src/stores/workspaceReview.ts index a1b61cd..d55b924 100644 --- a/ui/src/stores/workspaceReview.ts +++ b/ui/src/stores/workspaceReview.ts @@ -2,6 +2,7 @@ import { create } from 'zustand'; import { reviewSession, type StoredBaseline } from '../lib/db'; import { pathKey, repoFamilyName, tabWorktreeName } from '../lib/repoIdentity'; +import { reviewNoteScope } from '../lib/reviewExport'; import { errMessage, tauri } from '../lib/tauri'; import type { FileDiff, ReviewNote } from '../lib/types'; import { @@ -59,6 +60,8 @@ export interface MemberReview { /** Reviewer notes (`path → notes`), shared with the repo's own Review * session persistence — feed for the repo-grouped feedback export. */ notes: Record; + /** Comparison identity used to keep notes out of unrelated baselines/refs. */ + noteScope: string; loading: boolean; /** Human-readable fetch failure for this member, or `null`. */ error: string | null; @@ -200,14 +203,28 @@ export const useWorkspaceReview = create((set, get) => { // Prefer the single-repo store's in-memory marks + notes for the active // repo — its persistence is fire-and-forget, so the DB read can be a // beat stale. + const noteScope = reviewNoteScope({ + baselineOid: baseline?.oid ?? null, + branch: meta.branch, + detached: meta.detached, + headOid: meta.head_oid, + }); const repoState = useRepo.getState(); const active = isActiveRepo(path); + const activeScope = repoState.meta + ? reviewNoteScope({ + baselineOid: repoState.baseline?.oid ?? null, + branch: repoState.meta.branch, + detached: repoState.meta.detached, + headOid: repoState.meta.head_oid, + }) + : null; const reviewed = active ? repoState.reviewed : ((await reviewSession.getReviewed(path).catch(() => null)) ?? {}); - const notes = active + const notes = active && activeScope === noteScope ? repoState.reviewNotes - : ((await reviewSession.getNotes(path).catch(() => null)) ?? {}); + : ((await reviewSession.getNotes(path, noteScope).catch(() => null)) ?? {}); if (gen !== generation) return; patchMember(path, { @@ -222,6 +239,7 @@ export const useWorkspaceReview = create((set, get) => { unstaged, reviewed, notes, + noteScope, loading: false, error, }); @@ -257,6 +275,7 @@ export const useWorkspaceReview = create((set, get) => { unstaged: old?.unstaged ?? [], reviewed: old?.reviewed ?? {}, notes: old?.notes ?? {}, + noteScope: old?.noteScope ?? '', loading: true, error: null, }; @@ -365,12 +384,26 @@ function persistNotes( next: Record, ): void { patchMember(member.path, { notes: next }); - if (isActiveRepo(member.path)) useRepo.setState({ reviewNotes: next }); + if (isActiveRepo(member.path) && activeRepoNoteScope() === member.noteScope) { + useRepo.setState({ reviewNotes: next }); + } void reviewSession - .setNotes(member.path, next) + .setNotes(member.path, next, member.noteScope) .catch((e) => console.warn('workspace review: notes persist failed', e)); } +/** Scope currently shown by the single-repo Review lens, when available. */ +function activeRepoNoteScope(): string | null { + const state = useRepo.getState(); + if (!state.meta) return null; + return reviewNoteScope({ + baselineOid: state.baseline?.oid ?? null, + branch: state.meta.branch, + detached: state.meta.detached, + headOid: state.meta.head_oid, + }); +} + /** Write-op tail: refresh the touched member, and when it's the active repo * also run the single-repo refresh so Local Changes / topbar stay in sync. */ async function afterWrite(get: () => WorkspaceReviewState, repoPath: string): Promise { diff --git a/ui/src/styles/features.css b/ui/src/styles/features.css index be4563d..9b1c50b 100644 --- a/ui/src/styles/features.css +++ b/ui/src/styles/features.css @@ -7793,7 +7793,6 @@ textarea.clone-input { position: absolute; inset: 0; pointer-events: none; - visibility: hidden; z-index: 2; } .work-terminal-pane { diff --git a/ui/src/views/CloneDialog.test.ts b/ui/src/views/CloneDialog.test.ts new file mode 100644 index 0000000..707ef2a --- /dev/null +++ b/ui/src/views/CloneDialog.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it, vi } from 'vitest'; + +vi.stubGlobal('window', {}); + +const { startCloneDialogFocusLifecycle } = await import('./CloneDialog'); + +describe('CloneDialog focus lifecycle', () => { + it('reclaims focus after a closing palette restores a control behind the modal', () => { + const paletteInput = { focus: vi.fn() }; + const underlyingNote = { focus: vi.fn() }; + const urlInput = { focus: vi.fn() }; + let active = urlInput; + let deferred = () => {}; + const cancelFrame = vi.fn(); + + const cleanup = startCloneDialogFocusLifecycle( + paletteInput, + () => active, + () => urlInput, + (callback) => { + deferred = callback; + return 7; + }, + cancelFrame, + ); + + // Palette unmount restores its opener after React's autoFocus ran. + active = underlyingNote; + deferred(); + + expect(urlInput.focus).toHaveBeenCalledOnce(); + + cleanup(); + expect(cancelFrame).toHaveBeenCalledWith(7); + expect(underlyingNote.focus).toHaveBeenCalledOnce(); + expect(paletteInput.focus).not.toHaveBeenCalled(); + }); + + it('preserves the direct opener when autofocus already owns focus', () => { + const directOpener = { focus: vi.fn() }; + const urlInput = { focus: vi.fn() }; + let deferred = () => {}; + + const cleanup = startCloneDialogFocusLifecycle( + directOpener, + () => urlInput, + () => urlInput, + (callback) => { + deferred = callback; + return 1; + }, + vi.fn(), + ); + + deferred(); + cleanup(); + + expect(directOpener.focus).toHaveBeenCalledOnce(); + }); +}); diff --git a/ui/src/views/CloneDialog.tsx b/ui/src/views/CloneDialog.tsx index 6076f4c..4547d7b 100644 --- a/ui/src/views/CloneDialog.tsx +++ b/ui/src/views/CloneDialog.tsx @@ -5,6 +5,33 @@ import { pickDirectory } from '../lib/dialog'; import { t } from '../lib/i18n'; import { useSettings } from '../stores/settings'; +type FocusTarget = { focus: () => void }; + +/** + * Defer the dialog's initial focus until a closing command palette has restored + * its opener. The returned cleanup restores whichever control was behind the + * modal rather than the auto-focused URL input. + */ +export function startCloneDialogFocusLifecycle( + initialOpener: FocusTarget | null, + getActive: () => FocusTarget | null, + getInput: () => FocusTarget | null, + requestFrame: (callback: () => void) => number, + cancelFrame: (id: number) => void, +): () => void { + let opener = initialOpener; + const frame = requestFrame(() => { + const input = getInput(); + const active = getActive(); + if (active && active !== input) opener = active; + input?.focus(); + }); + return () => { + cancelFrame(frame); + opener?.focus(); + }; +} + /** * Modal for configuring a clone. The user pastes a URL and picks a destination; * on submit it hands `(url, dest)` to `onStartClone` and closes immediately — @@ -25,12 +52,22 @@ export function CloneDialog({ const [nameEdited, setNameEdited] = useState(false); const urlRef = useRef(null); const dialogRef = useRef(null); + const openerRef = useRef(null); + if (openerRef.current === null && document.activeElement instanceof HTMLElement) { + openerRef.current = document.activeElement; + } - // Restore focus to whatever opened the dialog when it closes, so keyboard - // flow returns to the graph/sidebar instead of falling to . + // A palette action mounts this auto-focused input before the palette's + // unmount cleanup restores its own opener. Re-claim focus one frame later so + // typing cannot escape into the view behind this aria-modal dialog. useEffect(() => { - const prev = document.activeElement as HTMLElement | null; - return () => prev?.focus?.(); + return startCloneDialogFocusLifecycle( + openerRef.current, + () => document.activeElement as HTMLElement | null, + () => urlRef.current, + (callback) => window.requestAnimationFrame(callback), + (id) => window.cancelAnimationFrame(id), + ); }, []); // Keep Tab focus inside the modal — required by the aria-modal contract, diff --git a/ui/src/views/LocalChanges.tsx b/ui/src/views/LocalChanges.tsx index 5997d24..2a05085 100644 --- a/ui/src/views/LocalChanges.tsx +++ b/ui/src/views/LocalChanges.tsx @@ -11,6 +11,7 @@ import { Diff, diffAppearanceOptions, parseCacheablePatch } from '../components/ import { DiffSearchBar, focusDiffSearchInput } from '../components/DiffSearchBar'; import { Icon } from '../components/Icon'; import { ImageDiff } from '../components/ImageDiff'; +import { handleCommitShortcut, type CommitShortcutEvent } from '../lib/commitShortcut'; import { matchTarget, scrollToDiffLine } from '../lib/diffJump'; import { pierreThemeOptions } from '../lib/pierreTheme'; import { isImagePath } from '../lib/image'; @@ -1704,6 +1705,10 @@ function CommitBar({ canCommit, hasChanges }: { canCommit: boolean; hasChanges: } } + function submitOnShortcut(event: CommitShortcutEvent) { + handleCommitShortcut(event, () => void submit()); + } + const disabled = submitting || !subject.trim() || (!canCommit && !amend); // Missing CLI does not disable the button: clicking surfaces the backend's // install/sign-in hint inline, instead of a dead control (DAN-11). @@ -1726,12 +1731,7 @@ function CommitBar({ canCommit, hasChanges }: { canCommit: boolean; hasChanges: placeholder="Commit subject" value={subject} onChange={(e) => setSubject(e.target.value)} - onKeyDown={(e) => { - if (e.key === 'Enter' && (e.metaKey || e.ctrlKey)) { - e.preventDefault(); - void submit(); - } - }} + onKeyDown={submitOnShortcut} />