diff --git a/docs/e2e-test-process.md b/docs/e2e-test-process.md index 4103829..9b32980 100644 --- a/docs/e2e-test-process.md +++ b/docs/e2e-test-process.md @@ -17,13 +17,15 @@ To skip the one-time prebuild for faster local iteration: ## Per-Test Isolation -Each E2E spec should reset state in `beforeEach` with existing helpers: +The `tauriPage` fixture owns per-test reset before the app launches: 1. Reset isolated config DB path for the run. -2. Return to home screen with stable UI helpers. -3. Reset workspace test directories. +2. Reset workspace test directories. +3. Start a fresh Tauri app process for the test. -The `reloadToHome()` helper in `e2e/helpers/ui.ts` is the source of truth for the in-app reset path. It clears `sg_workspace_hint` from session storage _before_ navigating to home (so the home page never auto-navigates back to the previous workspace), then uses UI-driven navigation via `ensureHome()`. A full `window.location.reload()` is intentionally avoided — SvelteKit route navigation already tears down and remounts all page components, and hard reloads take 20–45 s on slow Windows CI runners. +This ordering is required on Windows. Resetting the config DB or workspace directories after the app is already running can race startup reads, file watchers, and terminal child processes, producing `database is locked`, `EBUSY`, and `beforeEach` timeout flakes. + +Specs should assume a fresh app on first interaction and should not perform their own reset in `beforeEach` unless a test explicitly needs an in-app navigation step within the same process. ## Browser Dependency Setup diff --git a/docs/tauri-playwright-adapter-cheatsheet.md b/docs/tauri-playwright-adapter-cheatsheet.md index f13c8f1..6c384e6 100644 --- a/docs/tauri-playwright-adapter-cheatsheet.md +++ b/docs/tauri-playwright-adapter-cheatsheet.md @@ -140,9 +140,9 @@ If each process computes independently, worker and webServer can drift and fail - Keep webServer startup timeout modest (around 90s). - Keep plugin connect timeout modest (around 30s). - Fail fast on startup error toasts, and attach them to test artifacts. -- Reset state per spec in `beforeEach`, not via a global Playwright hook. -- For per-test isolation, delete the E2E workspace dir and config DB, then return to the home screen with stable in-app navigation. -- Prefer a persistent app process plus per-test state reset over restarting Tauri between tests. +- Reset disk state in the fixture before launching Tauri for each test. +- For per-test isolation, delete the E2E workspace dir and config DB before app startup; do not delete them from a spec-level `beforeEach` after the app has already opened them. +- Keep the app lifecycle model consistent. If the fixture launches a fresh Tauri process per test, specs should not layer an in-app reset path on top of that. ## Debug Checklist @@ -190,10 +190,15 @@ These are real failures we have already hit in this repo and what they usually m 7. `Failed to load config ... database is locked` during E2E state reset -- Meaning: test cleanup deleted or mutated the config DB while an operation still had it open, or multiple processes shared the same config DB. -- Fix: scope `SPROUTGIT_CONFIG_DB_PATH` per run, keep workers at `1`, and reset the config DB before the next test starts. SproutGit opens config DB connections on demand, so deleting the isolated DB between tests is safe when no command is actively using it. + - Meaning: test cleanup deleted or mutated the config DB while the app had already started and still had it open, or multiple processes shared the same config DB. + - Fix: scope `SPROUTGIT_CONFIG_DB_PATH` per run, keep workers at `1`, and reset the config DB before launching the next test's app process. 8. Full suite flakes when using hard reload in every `beforeEach` - Meaning: forcing `window.location.assign('/')` / `window.location.reload()` before each spec can be less stable than UI-driven navigation in `tauri` mode when tests share one long-lived app process. - Fix: use a persistent app process, reset disk state between tests, and use an `ensureHome()` helper that clicks back to the project list and waits for stable home-screen test IDs. Keep hard reloads as a targeted debugging tool, not the suite default. + +9. `beforeEach` times out before the first assertion on Windows + +- Meaning: the suite is spending its timeout budget deleting files or waiting on teardown after the app already launched. +- Fix: move reset into the fixture so config/workspace cleanup happens before `TauriProcessManager.start()`. diff --git a/e2e/fixtures.ts b/e2e/fixtures.ts index adcd0ad..a3df98f 100644 --- a/e2e/fixtures.ts +++ b/e2e/fixtures.ts @@ -7,6 +7,7 @@ import { } from '@srsholmes/tauri-playwright'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import { resetConfigDb, resetTestDirs } from './helpers/fixtures'; const MCP_SOCKET = process.env.SPROUTGIT_PLAYWRIGHT_SOCKET_PATH ?? join(tmpdir(), 'sproutgit-playwright.sock'); @@ -42,16 +43,29 @@ function parseCommandSpec(spec: string): { command: string; args: string[] } { type Fixtures = { mode: 'tauri'; + _resetE2EState: void; tauriPage: TauriPage; }; export const test = base.extend({ mode: ['tauri', { option: true }], - tauriPage: async ({ mode }, use) => { + _resetE2EState: [ + async ({}, use) => { + // Reset disk state before the app launches so startup code never races + // the config DB deletion or workspace directory cleanup. + resetConfigDb(); + resetTestDirs(); + await use(); + }, + { auto: true }, + ], + tauriPage: async ({ mode, _resetE2EState }, use) => { if (mode !== 'tauri') { throw new Error(`Unsupported E2E mode: ${mode}`); } + void _resetE2EState; + let processManager: TauriProcessManager | null = null; let client: PluginClient | null = null; @@ -92,7 +106,34 @@ export const test = base.extend({ await use(tauriPage); } finally { client?.disconnect(); + + // On Windows, processManager.stop() calls TerminateProcess() on the Tauri + // parent process only. Child processes spawned by the Tauri app (e.g. + // PowerShell hook terminals) are NOT in the same Windows Job Object and + // are NOT killed — they become orphaned with their CWD still pointing at + // worktree directories, causing EBUSY on rmSync in the next test's reset. + // + // Fix: use `taskkill /F /T /PID` to kill the entire process tree before + // calling stop(), so all child processes release their directory handles. + if (IS_WINDOWS && processManager) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const pid = (processManager as any).process?.pid as number | undefined; + if (pid) { + try { + const { execSync } = await import('node:child_process'); + execSync(`taskkill /F /T /PID ${pid}`, { stdio: 'ignore' }); + } catch { + // Process may already have exited — ignore + } + } + } + processManager?.stop(); + // Short grace period for the OS to fully release file handles after the + // process tree is terminated. taskkill /F /T is synchronous so 500ms is + // sufficient; the previous 2s was compensating for orphaned children that + // are now killed above. + await new Promise((resolve) => setTimeout(resolve, 500)); } }, }); diff --git a/e2e/helpers/fixtures.ts b/e2e/helpers/fixtures.ts index 913e793..ba3db69 100644 --- a/e2e/helpers/fixtures.ts +++ b/e2e/helpers/fixtures.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'node:child_process'; -import { appendFileSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { appendFileSync, existsSync, mkdirSync, renameSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { Faker, en } from '@faker-js/faker'; @@ -86,23 +86,53 @@ export function runGit(cwd: string, args: string[], extraEnv: Record { - const importButton = document.querySelector('[data-testid="btn-import"]'); - const main = document.querySelector('main'); - const mainText = main?.textContent ?? ''; - - return { - pathname: window.location.pathname, - importVisible: importButton instanceof HTMLElement, - checkingGit: mainText.includes('Checking git'), - }; - })()`); - } catch (error) { - if (!isMissingMainWindowError(error)) { - throw error; - } - await delay(200); - continue; - } - - if ( - state && - typeof state === 'object' && - 'pathname' in state && - 'importVisible' in state && - (state as { pathname: string }).pathname === '/' && - (state as { importVisible: boolean }).importVisible - ) { - return; - } - await delay(200); - } - - throw new Error('Home screen did not finish bootstrapping after reload'); -} - -async function clearCachedWorkspaceHint(tauriPage: AdapterPage) { - await tauriPage.evaluate(`(() => { - sessionStorage.removeItem('sg_workspace_hint'); - })()`); -} - async function waitForOptionalToastMessage( tauriPage: AdapterPage, type: 'success' | 'error' | 'warning' | 'info', @@ -130,43 +61,6 @@ async function waitForOptionalToastMessage( } } -export async function reloadToHome(tauriPage: AdapterPage) { - // reloadToHome has a strict budget: the entire beforeEach (including this - // function) must complete within the 90 s per-test timeout. Every await - // below can burn up to STARTUP_UI_TIMEOUT (45 s), so keeping the chain - // short is critical. - // - // Sequence rationale: - // 1. waitForMainWindow — ensure the window is alive before evaluating - // 2. clearCachedWorkspaceHint — remove sg_workspace_hint BEFORE navigation; - // this prevents the home page onMount from - // auto-navigating back to the previous workspace - // 3. ensureHome — navigate to home via UI (clicks - // btn-back-projects when needed); on return, - // btn-import is visible — no extra waitForHomeReady - // - // A full window.location.reload() is NOT performed. SvelteKit route - // navigation already tears down and remounts all page components, giving the - // same isolation as a hard reload. The reload was the dominant cost on slow - // Windows CI runners (WebView cold-start ≈ 20–45 s), routinely pushing - // beforeEach past the 90 s test timeout. The Tauri Playwright adapter - // cheatsheet also recommends UI-driven navigation over hard reloads as the - // suite default. - for (let attempt = 0; attempt < 2; attempt += 1) { - try { - await waitForMainWindow(tauriPage, STARTUP_UI_TIMEOUT); - await clearCachedWorkspaceHint(tauriPage); - await ensureHome(tauriPage); - return; - } catch (error) { - if (attempt === 1) { - throw error; - } - await delay(250); - } - } -} - export async function ensureHome(tauriPage: AdapterPage) { const deadline = Date.now() + STARTUP_UI_TIMEOUT; diff --git a/e2e/specs/canary-repos.spec.ts b/e2e/specs/canary-repos.spec.ts index 4521a4b..9df29b0 100644 --- a/e2e/specs/canary-repos.spec.ts +++ b/e2e/specs/canary-repos.spec.ts @@ -1,7 +1,6 @@ import { test, expect } from '../fixtures'; import { CANARY_REPOS, materializeCanaryRepo } from '../helpers/benchmark-repos'; -import { resetConfigDb, resetTestDirs } from '../helpers/fixtures'; -import { importRepoViaUi, reloadToHome } from '../helpers/ui'; +import { importRepoViaUi } from '../helpers/ui'; test.describe('Canary repositories @canary', () => { test.skip( @@ -9,12 +8,6 @@ test.describe('Canary repositories @canary', () => { 'Set RUN_CANARY=1 to run non-blocking canary repository checks' ); - test.beforeEach(async ({ tauriPage }) => { - resetConfigDb(); - await reloadToHome(tauriPage); - resetTestDirs(); - }); - for (const canary of CANARY_REPOS) { test(`imports ${canary.name} and renders the workspace shell`, async ({ tauriPage }) => { const repoPath = materializeCanaryRepo(canary); diff --git a/e2e/specs/commit-workflow.spec.ts b/e2e/specs/commit-workflow.spec.ts index 0d75114..6be7be4 100644 --- a/e2e/specs/commit-workflow.spec.ts +++ b/e2e/specs/commit-workflow.spec.ts @@ -1,29 +1,15 @@ import { test, expect } from '../fixtures'; import { dirname, join } from 'node:path'; -import { - createTestRepo, - querySqlite, - resetConfigDb, - resetTestDirs, - runGit, - writeRepoFile, -} from '../helpers/fixtures'; +import { createTestRepo, querySqlite, runGit, writeRepoFile } from '../helpers/fixtures'; import { createWorktreeViaUi, DEFAULT_UI_TIMEOUT, importRepoViaUi, openChangesTab, openHistoryTab, - reloadToHome, } from '../helpers/ui'; test.describe('Commit workflow', () => { - test.beforeEach(async ({ tauriPage }) => { - resetConfigDb(); - await reloadToHome(tauriPage); - resetTestDirs(); - }); - test('stages and commits changes from a managed worktree', async ({ tauriPage }) => { const repoPath = createTestRepo('commit-test', { extraCommits: 1 }); diff --git a/e2e/specs/daily-workflow.spec.ts b/e2e/specs/daily-workflow.spec.ts index d9ae398..c2d59c6 100644 --- a/e2e/specs/daily-workflow.spec.ts +++ b/e2e/specs/daily-workflow.spec.ts @@ -15,8 +15,6 @@ import { executeSqlite, mergeNoFastForward, querySqlite, - resetConfigDb, - resetTestDirs, runGit, writeRepoFile, } from '../helpers/fixtures'; @@ -27,7 +25,6 @@ import { importRepoViaUi, openChangesTab, openHistoryTab, - reloadToHome, selectWorktreeViaUi, stageAndCommitViaUi, } from '../helpers/ui'; @@ -194,18 +191,6 @@ function dailyAfterCreateTerminalHook(marker: string) { } test.describe('Daily developer workflow', () => { - test.beforeEach(async ({ tauriPage }) => { - // Delete the config DB first (no EBUSY risk — it lives in a separate run-scoped - // directory, not the workspace dir the watcher holds open). - resetConfigDb(); - // Navigate the app to the home screen BEFORE deleting test directories. - // On Windows, the Tauri file watcher holds open handles on the worktrees - // directory from the previous test. Navigating away first causes the - // backend to stop watching, releasing those handles before rmSync runs. - await reloadToHome(tauriPage); - resetTestDirs(); - }); - // --------------------------------------------------------------------------- // Story 1: Morning standup — set up two feature branches as worktrees // --------------------------------------------------------------------------- @@ -618,10 +603,16 @@ test.describe('Daily developer workflow', () => { await changesTab.waitFor(DEFAULT_UI_TIMEOUT); await terminalTab.waitFor(DEFAULT_UI_TIMEOUT); - expect(await historyTab.getAttribute('disabled')).not.toBeNull(); + expect(await historyTab.getAttribute('disabled')).toBeNull(); expect(await changesTab.getAttribute('disabled')).not.toBeNull(); expect(await terminalTab.getAttribute('disabled')).toBeNull(); + await historyTab.click(); + const hasCommitGraph = await tauriPage.evaluate(`(() => { + return (document.body?.textContent ?? '').includes('Commit graph'); + })()`); + expect(hasCommitGraph).toBe(true); + await terminalTab.click(); await tauriPage.getByTestId('btn-add-terminal').waitFor(DEFAULT_UI_TIMEOUT); diff --git a/e2e/specs/hero-screenshots.spec.ts b/e2e/specs/hero-screenshots.spec.ts index 04bca71..bebd1d8 100644 --- a/e2e/specs/hero-screenshots.spec.ts +++ b/e2e/specs/hero-screenshots.spec.ts @@ -2,13 +2,7 @@ import { dirname, join } from 'node:path'; import { test } from '../fixtures'; import { createHeroMediaRepo } from '../helpers/benchmark-repos'; -import { - appendRepoFile, - executeSqlite, - resetConfigDb, - resetTestDirs, - writeRepoFile, -} from '../helpers/fixtures'; +import { appendRepoFile, executeSqlite, writeRepoFile } from '../helpers/fixtures'; import { captureScreenshotVariants, resizeWindowForScreenshot } from '../helpers/screenshots'; import { createWorktreeViaUi, @@ -16,7 +10,6 @@ import { importRepoViaUi, openChangesTab, openHistoryTab, - reloadToHome, } from '../helpers/ui'; function sqlStr(value: string) { @@ -61,12 +54,6 @@ test.describe('Hero screenshots @screenshots', () => { 'Set CAPTURE_SCREENSHOTS=1 to generate curated screenshots' ); - test.beforeEach(async ({ tauriPage }) => { - resetConfigDb(); - await reloadToHome(tauriPage); - resetTestDirs(); - }); - test('captures canonical UI screenshots from the pinned hero repo', async ({ tauriPage, }, testInfo) => { diff --git a/e2e/specs/import-workflow.spec.ts b/e2e/specs/import-workflow.spec.ts index a6f497d..ffceaa6 100644 --- a/e2e/specs/import-workflow.spec.ts +++ b/e2e/specs/import-workflow.spec.ts @@ -1,15 +1,9 @@ import { basename, dirname, join } from 'node:path'; import { test, expect } from '../fixtures'; -import { createTestRepo, querySqlite, resetConfigDb, resetTestDirs } from '../helpers/fixtures'; -import { DEFAULT_UI_TIMEOUT, ensureHome, importRepoViaUi, reloadToHome } from '../helpers/ui'; +import { createTestRepo, querySqlite } from '../helpers/fixtures'; +import { DEFAULT_UI_TIMEOUT, ensureHome, importRepoViaUi } from '../helpers/ui'; test.describe('Import workflow', () => { - test.beforeEach(async ({ tauriPage }) => { - resetConfigDb(); - await reloadToHome(tauriPage); - resetTestDirs(); - }); - test('imports a local repo and records it in recent projects', async ({ tauriPage }) => { const repoPath = createTestRepo('import-test', { extraCommits: 3 }); await importRepoViaUi(tauriPage, repoPath); diff --git a/e2e/specs/worktree-workflow.spec.ts b/e2e/specs/worktree-workflow.spec.ts index eb6526e..4a5d319 100644 --- a/e2e/specs/worktree-workflow.spec.ts +++ b/e2e/specs/worktree-workflow.spec.ts @@ -1,26 +1,9 @@ import { test, expect } from '../fixtures'; -import { - createTestRepo, - querySqlite, - resetConfigDb, - resetTestDirs, - runGit, -} from '../helpers/fixtures'; -import { - createWorktreeViaUi, - DEFAULT_UI_TIMEOUT, - importRepoViaUi, - reloadToHome, -} from '../helpers/ui'; +import { createTestRepo, querySqlite, runGit } from '../helpers/fixtures'; +import { createWorktreeViaUi, DEFAULT_UI_TIMEOUT, importRepoViaUi } from '../helpers/ui'; import { dirname, join } from 'node:path'; test.describe('Worktree workflow', () => { - test.beforeEach(async ({ tauriPage }) => { - resetConfigDb(); - await reloadToHome(tauriPage); - resetTestDirs(); - }); - test('creates, switches, and deletes managed worktrees', async ({ tauriPage }) => { const repoPath = createTestRepo('worktree-test', { extraCommits: 2, diff --git a/src-tauri/src/db.rs b/src-tauri/src/db.rs index 76d40d3..20f5843 100644 --- a/src-tauri/src/db.rs +++ b/src-tauri/src/db.rs @@ -2,6 +2,7 @@ use rusqlite_migration::{Migrations, M}; use sea_orm::{ConnectionTrait, Database, DatabaseConnection, Statement}; use std::path::{Path, PathBuf}; use std::sync::LazyLock; +use std::time::Duration; use crate::git::helpers::{ensure_directory, normalize_existing_path}; @@ -83,16 +84,40 @@ fn run_config_migrations(db_path: &Path) -> Result<(), String> { /// Apply connection-level PRAGMAs that must be set on every connection open, /// not just at schema creation time. fn apply_connection_pragmas(conn: &rusqlite::Connection) -> Result<(), String> { - conn.execute_batch( - " - PRAGMA journal_mode = WAL; - PRAGMA synchronous = NORMAL; - PRAGMA foreign_keys = ON; - PRAGMA cache_size = -4096; - PRAGMA temp_store = MEMORY; - ", - ) - .map_err(|e| format!("Failed to apply connection PRAGMAs: {e}")) + // On slower CI runners, concurrent commands can briefly contend on sqlite + // connection setup. Retry busy/locked failures for a short bounded window. + const MAX_ATTEMPTS: u32 = 20; + const RETRY_DELAY_MS: u64 = 50; + + for attempt in 1..=MAX_ATTEMPTS { + match conn.execute_batch( + " + PRAGMA journal_mode = WAL; + PRAGMA synchronous = NORMAL; + PRAGMA foreign_keys = ON; + PRAGMA cache_size = -4096; + PRAGMA temp_store = MEMORY; + ", + ) { + Ok(()) => return Ok(()), + Err(err) => { + let retryable = matches!( + err, + rusqlite::Error::SqliteFailure(code, _) + if code.code == rusqlite::ErrorCode::DatabaseBusy + || code.code == rusqlite::ErrorCode::DatabaseLocked + ); + + if !retryable || attempt == MAX_ATTEMPTS { + return Err(format!("Failed to apply connection PRAGMAs: {err}")); + } + + std::thread::sleep(Duration::from_millis(RETRY_DELAY_MS)); + } + } + } + + Err("Failed to apply connection PRAGMAs: exhausted retries".to_string()) } fn config_db_path() -> Result { diff --git a/src-tauri/src/git/helpers.rs b/src-tauri/src/git/helpers.rs index c3e0c38..ea35aca 100644 --- a/src-tauri/src/git/helpers.rs +++ b/src-tauri/src/git/helpers.rs @@ -65,11 +65,12 @@ pub enum GitAction { Pull, ListRemotes, CheckIgnore, + SymbolicRef, } impl GitAction { #[cfg(test)] - pub const ALL: [GitAction; 32] = [ + pub const ALL: [GitAction; 33] = [ GitAction::GitInfo, GitAction::WorktreeList, GitAction::ListRefs, @@ -102,6 +103,7 @@ impl GitAction { GitAction::Pull, GitAction::ListRemotes, GitAction::CheckIgnore, + GitAction::SymbolicRef, ]; pub fn label(self) -> &'static str { @@ -138,6 +140,7 @@ impl GitAction { GitAction::Pull => "pull", GitAction::ListRemotes => "list_remotes", GitAction::CheckIgnore => "check_ignore", + GitAction::SymbolicRef => "symbolic_ref", } } } diff --git a/src-tauri/src/git/operations.rs b/src-tauri/src/git/operations.rs index df8fd42..0bb6b48 100644 --- a/src-tauri/src/git/operations.rs +++ b/src-tauri/src/git/operations.rs @@ -48,6 +48,8 @@ pub struct RefInfo { pub struct RefsResult { pub repo_path: String, pub refs: Vec, + /// Short name of the default remote branch (e.g. `origin/main`), if discoverable. + pub default_remote_branch: Option, } #[derive(Serialize)] @@ -435,6 +437,8 @@ pub async fn list_refs(repo_path: String) -> Result { "tag" }; + // Skip origin/HEAD (symbolic ref pointing to default branch); + // the default branch is discovered separately below via symbolic-ref. if kind == "remote" && short_name.ends_with("/HEAD") { return None; } @@ -448,9 +452,32 @@ pub async fn list_refs(repo_path: String) -> Result { }) .collect(); + // Discover the default remote branch via symbolic-ref (e.g. origin/HEAD -> origin/main). + // Failures are non-fatal; repos without a configured origin/HEAD simply get None. + let default_remote_branch = run_git( + GitAction::SymbolicRef, + &[ + "-C", + &canonical.to_string_lossy(), + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + ], + ) + .ok() + .and_then(|o| { + if o.status.success() { + let s = String::from_utf8_lossy(&o.stdout).trim().to_string(); + if s.is_empty() { None } else { Some(s) } + } else { + None + } + }); + Ok(RefsResult { repo_path: path_to_frontend(&canonical), refs, + default_remote_branch, }) } diff --git a/src-tauri/src/terminal.rs b/src-tauri/src/terminal.rs index b936579..90ae1eb 100644 --- a/src-tauri/src/terminal.rs +++ b/src-tauri/src/terminal.rs @@ -45,6 +45,10 @@ struct PtySession { pub struct TerminalManager { sessions: Arc>>>, + /// Set of pty_ids for non-interactive hook terminals. These are spawned + /// via std::process::Command (not PTY). The child processes are owned by + /// wait-threads and removed from this set when those threads complete. + non_interactive_pids: Arc>>, /// Map of hook_id → epoch milliseconds at which that hook's terminal /// session exited. Populated by the non-interactive PTY wait-thread when a /// session was spawned with a `hook_id` argument. Read by the @@ -59,6 +63,7 @@ impl TerminalManager { pub fn new() -> Self { Self { sessions: Arc::new(Mutex::new(HashMap::new())), + non_interactive_pids: Arc::new(Mutex::new(std::collections::HashSet::new())), closed_hook_terminals: Arc::new(Mutex::new(HashMap::new())), } } @@ -200,7 +205,16 @@ pub async fn spawn_terminal( .spawn() .map_err(|e| format!("Failed to spawn shell '{shell}': {e}"))?; + // Register this non-interactive terminal's pty_id for tracking. let pty_id_clone = pty_id.clone(); + { + let mut non_interactive = state + .non_interactive_pids + .lock() + .map_err(|_| "Failed to lock non-interactive terminals".to_string())?; + non_interactive.insert(pty_id_clone.clone()); + } + let app_stdout = app_handle.clone(); let app_stderr = app_handle.clone(); let app_closed = app_handle.clone(); @@ -245,8 +259,14 @@ pub async fn spawn_terminal( // deterministic completion signal. let closed_map = Arc::clone(&state.closed_hook_terminals); let hook_id_for_thread = validated_hook_id.clone(); + let non_interactive_pids = Arc::clone(&state.non_interactive_pids); + let pty_id_for_cleanup = pty_id_clone.clone(); std::thread::spawn(move || { let _ = child.wait(); + // Remove this terminal from tracking since it's done + if let Ok(mut set) = non_interactive_pids.lock() { + set.remove(&pty_id_for_cleanup); + } if let Some(hook_id) = hook_id_for_thread { let now_ms = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -452,6 +472,11 @@ pub async fn close_all_terminals(state: tauri::State<'_, TerminalManager>) -> Re } sessions.clear(); + // Non-interactive terminals are tracked in non_interactive_pids for + // monitoring, but their child processes are owned by wait-threads and + // cannot be directly killed from here. The E2E fixture cleanup grace + // period (500ms) allows those threads to naturally exit and release handles. + Ok(()) } diff --git a/src-tauri/src/watcher.rs b/src-tauri/src/watcher.rs index cd3791d..1c720dc 100644 --- a/src-tauri/src/watcher.rs +++ b/src-tauri/src/watcher.rs @@ -66,6 +66,35 @@ fn match_git_index_to_worktree( } } +/// Returns `true` if the path inside `.git/` signals that branches or HEAD state changed. +/// +/// Matches (relative to `git_dir`): +/// `HEAD` → checkout in the main worktree +/// `COMMIT_EDITMSG` → any commit was just created +/// `packed-refs` → refs were repacked (push/fetch/gc) +/// `refs/**` → any ref update (branch advance, new tag, etc.) +/// `worktrees//HEAD` → checkout in a linked worktree +fn is_git_refs_change(event_path: &Path, git_dir: &Path) -> bool { + let rel = match event_path.strip_prefix(git_dir) { + Ok(r) => r, + Err(_) => return false, + }; + let comps: Vec<_> = rel.components().collect(); + match comps.as_slice() { + [c] if c.as_os_str() == OsStr::new("HEAD") => true, + [c] if c.as_os_str() == OsStr::new("COMMIT_EDITMSG") => true, + [c] if c.as_os_str() == OsStr::new("packed-refs") => true, + [c, ..] if c.as_os_str() == OsStr::new("refs") => true, + [a, _, c] + if a.as_os_str() == OsStr::new("worktrees") + && c.as_os_str() == OsStr::new("HEAD") => + { + true + }, + _ => false, + } +} + /// Returns `true` if `path` matches a gitignore rule inside `worktree_path`. /// Uses `git check-ignore -q -- ` (exit 0 = ignored, 1 = not ignored). /// Falls back to `false` (treat as real change) on any error so that a git @@ -156,10 +185,11 @@ pub async fn start_watching_worktrees( }; let mut affected: HashSet = HashSet::new(); + let mut refs_changed = false; for event in &events { let event_path = &event.path; - // ── Git index changes (external staging) ── + // ── Git index / refs changes ── if let Some(ref git_dir) = git_dir_for_closure { let git_dir_path = Path::new(git_dir); if event_path.starts_with(git_dir_path) { @@ -174,6 +204,10 @@ pub async fn start_watching_worktrees( if Path::new(&wt).is_dir() { affected.insert(wt); } + } else if is_git_refs_change(event_path, git_dir_path) { + // A branch ref, HEAD, or commit marker changed — the graph + // and worktree list may be stale (e.g. terminal commit/checkout). + refs_changed = true; } // Skip further processing for all .git dir events regardless. continue; @@ -199,6 +233,9 @@ pub async fn start_watching_worktrees( for wt_path in affected { let _ = app.emit("worktree-changed", &wt_path); } + if refs_changed { + let _ = app.emit("git-refs-changed", ()); + } }, ) .map_err(|e| format!("Failed to create file watcher: {e}"))?; @@ -239,3 +276,59 @@ pub async fn stop_watching_worktrees(state: State<'_, WatcherState>) -> Result<( *guard = None; Ok(()) } + +#[cfg(test)] +mod tests { + use super::is_git_refs_change; + use std::path::Path; + + fn git(extra: &str) -> std::path::PathBuf { + Path::new("/repo/.git").join(extra) + } + const GIT_DIR: &str = "/repo/.git"; + + #[test] + fn matches_head() { + assert!(is_git_refs_change(&git("HEAD"), Path::new(GIT_DIR))); + } + + #[test] + fn matches_commit_editmsg() { + assert!(is_git_refs_change(&git("COMMIT_EDITMSG"), Path::new(GIT_DIR))); + } + + #[test] + fn matches_packed_refs() { + assert!(is_git_refs_change(&git("packed-refs"), Path::new(GIT_DIR))); + } + + #[test] + fn matches_refs_prefix() { + assert!(is_git_refs_change(&git("refs/heads/main"), Path::new(GIT_DIR))); + assert!(is_git_refs_change(&git("refs/remotes/origin/main"), Path::new(GIT_DIR))); + assert!(is_git_refs_change(&git("refs/tags/v1.0"), Path::new(GIT_DIR))); + } + + #[test] + fn matches_worktree_head() { + assert!(is_git_refs_change( + &git("worktrees/feature-foo/HEAD"), + Path::new(GIT_DIR) + )); + } + + #[test] + fn does_not_match_index() { + assert!(!is_git_refs_change(&git("index"), Path::new(GIT_DIR))); + assert!(!is_git_refs_change(&git("ORIG_HEAD"), Path::new(GIT_DIR))); + assert!(!is_git_refs_change(&git("worktrees/feature-foo/index"), Path::new(GIT_DIR))); + } + + #[test] + fn does_not_match_outside_git_dir() { + assert!(!is_git_refs_change( + Path::new("/repo/some-file.rs"), + Path::new(GIT_DIR) + )); + } +} diff --git a/src/lib/components/TerminalContainer.svelte b/src/lib/components/TerminalContainer.svelte index 8f128e0..5d9c35a 100644 --- a/src/lib/components/TerminalContainer.svelte +++ b/src/lib/components/TerminalContainer.svelte @@ -71,9 +71,16 @@ r => r.cwd === cwd && !processedLaunchIds.has(r.id) ); const hasPendingLaunch = pendingForThisCwd.length > 0; - if (!_autoSpawned && defaultShell && !hasPendingLaunch) { + if (!_autoSpawned && defaultShell) { _autoSpawned = true; - addSession(defaultShell); + // Only spawn the blank default session when no hook launch is pending for + // this container's cwd. If a launch is pending, the launchRequests effect + // below will add the hook session instead. We still set _autoSpawned so + // that once the launch request is consumed (and hasPendingLaunch flips back + // to false), this block does not fire again and add a spurious blank session. + if (!hasPendingLaunch) { + addSession(defaultShell); + } } }); diff --git a/src/lib/ref-utils.ts b/src/lib/ref-utils.ts new file mode 100644 index 0000000..694d722 --- /dev/null +++ b/src/lib/ref-utils.ts @@ -0,0 +1,39 @@ +import type { RefInfo } from '$lib/sproutgit'; + +/** + * Comparator for sorting refs in the "create worktree" source-ref list. + * The detected default remote branch (e.g. `origin/main`) sorts first, + * followed by upstream/*, origin/*, other remotes, local branches, tags. + */ +export function makeCompareRefsForCreate( + defaultRemoteBranch?: string +): (a: RefInfo, b: RefInfo) => number { + return function compareRefsForCreate(a: RefInfo, b: RefInfo): number { + const rank = (ref: RefInfo): number => { + if (defaultRemoteBranch && ref.name === defaultRemoteBranch) return 0; + if (ref.kind === 'remote' && ref.name.startsWith('upstream/')) return 1; + if (ref.kind === 'remote' && ref.name.startsWith('origin/')) return 2; + if (ref.kind === 'remote') return 3; + if (ref.kind === 'branch') return 4; + return 5; + }; + const rankDiff = rank(a) - rank(b); + if (rankDiff !== 0) return rankDiff; + return a.name.localeCompare(b.name); + }; +} + +/** + * Pick the best default source ref from a list of refs. + * Returns the default remote branch if present, otherwise the highest-ranked + * remote ref, otherwise the first ref, otherwise `'HEAD'`. + */ +export function preferredSourceRef(refList: RefInfo[], defaultRemoteBranch?: string): string { + if (defaultRemoteBranch) { + const defaultRef = refList.find(r => r.name === defaultRemoteBranch); + if (defaultRef) return defaultRef.name; + } + const sorted = [...refList].sort(makeCompareRefsForCreate(defaultRemoteBranch)); + const preferred = sorted.find(ref => ref.kind === 'remote') ?? sorted[0]; + return preferred?.name ?? 'HEAD'; +} diff --git a/src/lib/sproutgit.ts b/src/lib/sproutgit.ts index e132a9d..de6f578 100644 --- a/src/lib/sproutgit.ts +++ b/src/lib/sproutgit.ts @@ -55,6 +55,8 @@ export type RefInfo = { export type RefsResult = { repoPath: string; refs: RefInfo[]; + /** Short name of the default remote branch (e.g. `origin/main`), if discoverable. */ + defaultRemoteBranch?: string; }; export type CommitEntry = { @@ -583,6 +585,9 @@ export const stopWatchingWorktrees = () => invoke('stop_watching_worktrees export const onWorktreeChanged = (callback: (worktreePath: string) => void): Promise => listen('worktree-changed', event => callback(event.payload)); +export const onGitRefsChanged = (callback: () => void): Promise => + listen('git-refs-changed', () => callback()); + // ── Terminal ── export const listAvailableShells = () => invoke('list_available_shells'); diff --git a/src/routes/workspace/+page.svelte b/src/routes/workspace/+page.svelte index d20a409..709a452 100644 --- a/src/routes/workspace/+page.svelte +++ b/src/routes/workspace/+page.svelte @@ -43,6 +43,7 @@ stopWatchingWorktrees, closeAllTerminals, onWorktreeChanged, + onGitRefsChanged, getAppSetting, listAvailableShells, type StatusFileEntry, @@ -62,6 +63,7 @@ import { tildify } from '$lib/paths.svelte'; import { findPath, normalizePathSeparators, pathStartsWith, pathsEqual } from '$lib/path-utils'; import { validateBranchName, validateSourceRef } from '$lib/validation'; + import { makeCompareRefsForCreate, preferredSourceRef } from '$lib/ref-utils'; import { openPath } from '@tauri-apps/plugin-opener'; import { FolderOpen, @@ -90,6 +92,7 @@ let worktrees = $state([]); let graph = $state(null); let refs = $state([]); + let defaultRemoteBranch = $state(undefined); let selectedRef = $state(''); let newBranch = $state(''); let activeWorktreePath = $state(null); @@ -712,26 +715,11 @@ const selectedWorktree = $derived( worktrees.find(item => pathsEqual(item.path, activeWorktreePath)) ?? null ); + const hasManagedWorktreeSelection = $derived( + Boolean(selectedWorktree && !pathsEqual(selectedWorktree.path, workspace?.rootPath)) + ); - function compareRefsForCreate(a: RefInfo, b: RefInfo): number { - const rank = (ref: RefInfo): number => { - if (ref.kind === 'remote' && ref.name.startsWith('upstream/')) return 0; - if (ref.kind === 'remote' && ref.name.startsWith('origin/')) return 1; - if (ref.kind === 'remote') return 2; - if (ref.kind === 'branch') return 3; - return 4; - }; - - const rankDiff = rank(a) - rank(b); - if (rankDiff !== 0) return rankDiff; - return a.name.localeCompare(b.name); - } - - function preferredSourceRef(refList: RefInfo[]): string { - const sorted = [...refList].sort(compareRefsForCreate); - const preferred = sorted.find(ref => ref.kind === 'remote') ?? sorted[0]; - return preferred?.name ?? 'HEAD'; - } + const compareRefsForCreate = $derived(makeCompareRefsForCreate(defaultRemoteBranch)); const sortedRefsForCreate = $derived([...refs].sort(compareRefsForCreate)); const refItems = $derived( @@ -809,7 +797,7 @@ } }); - const canUseWorktreeViews = $derived(Boolean(selectedWorktree && !activeIsRoot)); + const canUseChangesView = $derived(hasManagedWorktreeSelection); // Per-worktree change counts for sidebar badges let worktreeChangeCounts = $state>({}); @@ -897,8 +885,10 @@ } async function loadWorktreeStatus() { - if (!selectedWorktree) { + if (!selectedWorktree || !hasManagedWorktreeSelection) { worktreeStatus = []; + stagingDiffFile = null; + stagingDiffContent = ''; return; } statusLoading = true; @@ -943,6 +933,19 @@ } async function refreshWorktreeStatusFromWatcher(changedPath: string) { + if (deleting && pathsEqual(changedPath, deleting)) { + return; + } + + if (workspace && pathsEqual(changedPath, workspace.rootPath)) { + if (!hasManagedWorktreeSelection) { + worktreeStatus = []; + stagingDiffFile = null; + stagingDiffContent = ''; + } + return; + } + const result = await getWorktreeStatus(changedPath); worktreeChangeCounts[changedPath] = result.files.length; if (pathsEqual(changedPath, selectedWorktree?.path)) { @@ -1109,6 +1112,12 @@ } }); + $effect(() => { + if (activeTab === 'changes' && !canUseChangesView) { + activeTab = 'history'; + } + }); + // ── File Watcher ── let unlistenWorktreeChanged: (() => void) | null = null; @@ -1118,6 +1127,9 @@ // listWorktrees returns forward-slash paths. Normalise separators only — // never lowercase, since Linux filesystems are case-sensitive. const changedPath = normalizePathSeparators(changedPathRaw); + if (deleting && pathsEqual(changedPath, deleting)) { + return; + } if (stagingAction || committing) { queueWatcherRefresh(changedPath); return; @@ -1203,8 +1215,9 @@ worktrees = worktreeData.worktrees; refs = refsData.refs; + defaultRemoteBranch = refsData.defaultRemoteBranch; initializeGraphState(graphData); - selectedRef = preferredSourceRef(refsData.refs); + selectedRef = preferredSourceRef(refsData.refs, refsData.defaultRemoteBranch); if (graphHasMore) { // Fetch total commit count in the background only when the first page is partial. @@ -1264,6 +1277,9 @@ creating = true; error = ''; + // Close the modal immediately so the operation status banner (hook progress) + // is visible behind the now-dismissed dialog while hooks are running. + createModalOpen = false; try { await beginOperation( @@ -1339,9 +1355,29 @@ const unlistenHookProgress = onHookProgress(handleHookProgress); const unlistenHookTerminalLaunch = onHookTerminalLaunch(handleHookTerminalLaunch); + // When git refs change externally (e.g. terminal commits/checkouts), refresh + // Debounce rapid ref-change events (e.g. fetch/repack emits several in quick + // succession) and prevent overlapping full refreshes with an in-flight guard. + let refsChangedTimer: ReturnType | null = null; + let refsRefreshInFlight = false; + // the graph and worktrees list so the History tab stays up to date. + const unlistenGitRefsChanged = onGitRefsChanged(() => { + if (committing || stagingAction) return; + if (refsChangedTimer !== null) clearTimeout(refsChangedTimer); + refsChangedTimer = setTimeout(() => { + refsChangedTimer = null; + if (refsRefreshInFlight) return; + refsRefreshInFlight = true; + void refreshWorkspaceData().finally(() => { + refsRefreshInFlight = false; + }); + }, 300); + }); onDestroy(() => { + if (refsChangedTimer !== null) clearTimeout(refsChangedTimer); void unlistenHookProgress.then(unlisten => unlisten()); void unlistenHookTerminalLaunch.then(unlisten => unlisten()); + void unlistenGitRefsChanged.then(unlisten => unlisten()); }); function handleHookTerminalLaunch(event: HookTerminalLaunchEvent) { @@ -1542,6 +1578,24 @@ onconfirm: async () => { confirmDialog = null; deleting = wt.path; + const nextWorktreePath = + worktrees.find( + worktree => + !pathsEqual(worktree.path, wt.path) && !pathsEqual(worktree.path, workspace!.rootPath) + )?.path ?? + worktrees.find(worktree => !pathsEqual(worktree.path, wt.path))?.path ?? + null; + const { [wt.path]: _removedChangeCount, ...remainingChangeCounts } = worktreeChangeCounts; + worktreeChangeCounts = remainingChangeCounts; + + if (activeWorktreePath && pathsEqual(activeWorktreePath, wt.path)) { + activeWorktreePath = nextWorktreePath; + activeTerminalPath = nextWorktreePath ?? workspace!.workspacePath; + worktreeStatus = []; + stagingDiffFile = null; + stagingDiffContent = ''; + } + try { await beginOperation( 'Removing worktree', @@ -1555,13 +1609,6 @@ } catch { worktrees = worktrees.filter(worktree => worktree.path !== wt.path); } - if (activeWorktreePath === wt.path) { - activeWorktreePath = - worktrees.find(w => !pathsEqual(w.path, workspace!.rootPath))?.path ?? - worktrees[0]?.path ?? - null; - activeTerminalPath = activeWorktreePath ?? workspace!.workspacePath; - } } catch (err) { failOperation(String(err)); toast.error(String(err)); @@ -1679,6 +1726,7 @@ worktrees = refreshedWt.worktrees; initializeGraphState(refreshedGraph); refs = refreshedRefs.refs; + defaultRemoteBranch = refreshedRefs.defaultRemoteBranch; if (graphHasMore) { // Refresh total commit count in the background only when the graph is partial. totalCommitCount = null; @@ -2204,7 +2252,7 @@
{ - await createFirstWorktree(e); - if (!error) createModalOpen = false; - }} + onsubmit={createFirstWorktree} class="px-4 py-3" >

{ + it('ranks default remote branch first when specified', () => { + const refs = [ref('origin/alpha'), ref('origin/main'), ref('origin/zebra')]; + const sorted = [...refs].sort(makeCompareRefsForCreate('origin/main')); + expect(sorted[0].name).toBe('origin/main'); + }); + + it('ranks upstream/* before origin/* when no default is set', () => { + const refs = [ref('origin/main'), ref('upstream/main')]; + const sorted = [...refs].sort(makeCompareRefsForCreate()); + expect(sorted[0].name).toBe('upstream/main'); + }); + + it('ranks origin/* before other remotes', () => { + const refs = [ref('fork/feature'), ref('origin/main')]; + const sorted = [...refs].sort(makeCompareRefsForCreate()); + expect(sorted[0].name).toBe('origin/main'); + }); + + it('ranks remote branches before local branches', () => { + const refs = [ref('feature', 'branch'), ref('origin/main')]; + const sorted = [...refs].sort(makeCompareRefsForCreate()); + expect(sorted[0].kind).toBe('remote'); + }); + + it('sorts alphabetically within the same rank', () => { + const refs = [ref('origin/zebra'), ref('origin/alpha'), ref('origin/main')]; + const sorted = [...refs].sort(makeCompareRefsForCreate()); + expect(sorted.map(r => r.name)).toEqual(['origin/alpha', 'origin/main', 'origin/zebra']); + }); +}); + +describe('preferredSourceRef', () => { + it('returns defaultRemoteBranch when it is in the list', () => { + const refs = [ref('origin/alpha'), ref('origin/main'), ref('origin/zebra')]; + expect(preferredSourceRef(refs, 'origin/main')).toBe('origin/main'); + }); + + it('falls back to highest-ranked remote when default is not in list', () => { + const refs = [ref('origin/alpha'), ref('origin/zebra')]; + expect(preferredSourceRef(refs, 'origin/main')).toBe('origin/alpha'); + }); + + it('falls back to upstream/* over origin/* when no default', () => { + const refs = [ref('origin/main'), ref('upstream/main')]; + expect(preferredSourceRef(refs)).toBe('upstream/main'); + }); + + it('returns HEAD when ref list is empty', () => { + expect(preferredSourceRef([])).toBe('HEAD'); + }); +});