✅ Local runtime verification — fix confirmed

I verified this PR locally with real test runs under tmux, including a real-filesystem demonstration of the watcher leak (beyond the mocked unit test). Verdict: the fix is correct and the new regression test genuinely catches the bug — safe to merge.

The bug

useGitBranchName's effect calls an async setupWatcher() that awaits fsPromises.access() before fs.watch(). If the component unmounts during that await, the cleanup runs while watcher is still undefined (so watcher?.close() is a no-op), and then — when access() resolves — fs.watch() is created after unmount. That watcher is never closed: a leaked OS file handle that keeps invoking fetchBranchName() (a git subprocess) on every .git/logs/HEAD change, for a component that no longer exists.

The fix adds a disposed flag set in cleanup and checked right after the await, bailing out before fs.watch() (plus a defensive watcher = undefined).

How I verified

All runs are the real vitest suite executed under tmux; the source file is swapped between base (c5935826c) and PR (6bde99ced) for each A/B.

1) The new test is a genuine regression test (base ✗ / PR ✓)

Running the PR's test file against each source:

PR shipped state (PR source + PR test): 8/8 tests pass.

2) Real-filesystem demonstration (not mocked)

I rendered the real hook with a real fs.watch on a real temp .git/logs/HEAD, forced the unmount-during-access race, then appended to the real reflog and waited for a real inotify event. Only @qwen-code/qwen-code-core is mocked (to count fetchBranchName→execCommand calls instead of shelling real git):

This shows the real-world consequence: on base, a live OS watcher survives unmount and keeps spawning git; the PR eliminates it.

3) PR's stated checks (on the PR state) — prettier --check ✓, eslint ✓, tsc --noEmit (cli typecheck) ✓, all rc=0.

Notes for the maintainer (non-blocking)

• Which test guards the race: only the new should not create watcher if setup completes after unmount fails on base. The two un-skipped tests (should update branch name when .git/logs/HEAD changes, should cleanup watcher on unmount) pass on both base and PR — they are de-flaked, un-skipped general coverage (replacing the flaky waitFor with a deterministic flushAsyncEffects + a real watch-callback spy), not the regression guard for this specific race. Good cleanup, just worth knowing which one is load-bearing.
• Fix is complete: each effect invocation gets its own disposed closure (correct across cwd changes), and the if (disposed) return; → fs.watch() path is synchronous, so there's no residual window after the check.

Recommendation: minimal, correct, well-covered. 👍 to merge.

_{Verified by running the real vitest suite under tmux with base/PR source A/B, plus a real-fs.watch leak demo on a real temp reflog. Hook source diff is exactly the 6-line disposed-flag change.}

@qwen-code /triage

Nice cleanup, and the test additions here (un-skipping the two watcher tests + the new unmount-race test) are genuinely useful. The production change has been overtaken by main, though.

useGitBranchName.ts on main already guards this exact race — there's an if (cancelled) return; between await fsPromises.access(gitLogsHeadPath, …) and fs.watch(…), so the watcher can't be created after unmount. This branch was opened before that landed, so it now conflicts (CONFLICTING / DIRTY) and the new disposed flag duplicates the existing cancelled guard.

Could you rebase onto main, drop the production hunk (it's redundant now), and keep just the test changes? That resolves the conflict and leaves a clean, valuable test-only PR. Thanks!

@copilot resolve the merge conflicts in this pull request

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflict by merging main into this branch (non-destructive — your commit history is preserved).

A note on how it resolved: main has since refactored useGitBranchName to guard the effect with a single cancelled flag, which already covers the watcher-teardown race this PR's disposed flag addressed — and additionally guards the async setState. So the hook is taken from main as-is (the disposed flag is now redundant), and this PR's net contribution is its regression test: it un-skips and implements the previously skip()-ed .git/logs/HEAD watcher test, which main still carries as a // TODO: fix. That test passes against main's implementation (8/8 locally, prettier-clean). The incidental formatting churn the 3-way merge introduced on unrelated files was reset back to main, so the net diff is just the test.

@qwen-code /triage

Thanks for the PR, @tt-a1i!

Template: The PR body doesn't follow the PR template — it's missing "What this PR does", "Why it's needed", "Reviewer Test Plan", "Risk & Scope", "Linked Issues", and the 中文说明 section. However, a collaborator has already reviewed and resolved merge conflicts on this PR, so I'm treating this as non-blocking. For future PRs, please follow the template — it helps reviewers move faster.

Direction: This addresses a real bug — an async race in useGitBranchName where fs.watch() can be created after component unmount, leaking an OS file handle that keeps spawning git subprocesses. The production fix has since been landed on main independently (via the cancelled flag), so after the conflict resolution this PR is now test-only: un-skipping two previously broken watcher tests and adding a regression test for the unmount race. That's a valuable contribution — main still carries those tests as skip() // TODO: fix.

Approach: The scope is tight — one test file, focused changes. The flushAsyncEffects helper replaces the flaky waitFor with a deterministic pattern. The new regression test (should not create watcher if setup completes after unmount) is the load-bearing one. No concerns here.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal: For a race condition where fs.watch() can be created after unmount in an async effect, I'd expect the tests to: (1) un-skip the two broken watcher tests with a more deterministic async flushing approach, (2) add a regression test that holds the fsPromises.access() promise, unmounts during the await, then verifies no watcher is created, and (3) spy on fs.watch directly rather than relying on memfs to fire events (memfs doesn't actually trigger inotify).

Comparison with the diff: The PR matches this proposal almost exactly. The flushAsyncEffects helper (vi.runAllTimers() + two microtask ticks) replaces the flaky waitFor. The watcher update test now captures the fs.watch callback via a spy and invokes it directly — much more reliable than hoping memfs fires events. The unmount race test holds access() via a deferred promise, unmounts, resolves, and asserts fs.watch was never called. Clean implementation, no over-engineering.

No critical issues found. The code follows project conventions and the test patterns are sound.

Testing

Before (main — 7 tests, 2 skipped)

 ✓ src/ui/hooks/useGitBranchName.test.ts (7 tests | 2 skipped) 38ms

 Test Files  1 passed (1)
      Tests  5 passed | 2 skipped (7)

After (this PR — 8 tests, 0 skipped)

 ✓ src/ui/hooks/useGitBranchName.test.ts (8 tests) 50ms

 Test Files  1 passed (1)
      Tests  8 passed (8)

Regression verification (removed cancelled guard from production code)

Confirmed the new test genuinely catches the bug — with if (cancelled) return; stripped from the hook:

 ❯ src/ui/hooks/useGitBranchName.test.ts (8 tests | 1 failed) 53ms
   ✓ useGitBranchName > should return branch name
   ✓ useGitBranchName > should return undefined if git command fails
   ✓ useGitBranchName > should return short commit hash if branch is HEAD (detached state)
   ✓ useGitBranchName > should return undefined if branch is HEAD and getting commit hash fails
   ✓ useGitBranchName > should update branch name when .git/logs/HEAD changes
   ✓ useGitBranchName > should handle watcher setup error silently
   ✓ useGitBranchName > should cleanup watcher on unmount
   × useGitBranchName > should not create watcher if setup completes after unmount
     → expected "bound watch" to not be called at all, but actually been called 1 times

 Test Files  1 failed (1)
      Tests  1 failed | 7 passed (8)

Only the new regression test fails — the other 7 pass regardless of the guard. This confirms the new test is the load-bearing one for this specific race condition.

— Qwen Code · qwen3.7-max

Stepping back: this is a clean, minimal test-only PR that does exactly what it says. The production fix (the cancelled flag race guard) was independently landed on main before this PR could merge, so after conflict resolution the net contribution is the test coverage improvement — and it's genuinely useful.

My independent proposal for these tests matched what the PR delivers: deterministic async flushing instead of flaky waitFor, direct fs.watch callback spying instead of relying on memfs inotify, and a regression test that holds the async gap open to verify the guard works. The PR nails all three.

The regression verification is the clincher: stripping the cancelled guard causes exactly one test to fail — the new should not create watcher if setup completes after unmount. The other 7 tests pass regardless. That's a well-targeted regression test.

The only note is the PR template non-compliance, which I've flagged as non-blocking for future PRs.

Approving. ✅

— Qwen Code · qwen3.7-max