fix(ui): Improve reliability of git branch watcher in footer #10403
Conversation
The git branch name in the footer was not updated if the repository was initialized after the CLI had started. This commit refactors the branch watching logic to fix this issue: - The watcher now monitors the .git/HEAD file, which is more reliable for detecting branch changes. - Implements a two-step strategy: it watches the CWD for .git directory creation if no repository is found on startup, then switches to watching .git/HEAD. - Extracts the complex watching logic into a reusable useGitWatcher custom hook to improve readability. - Adds a check to ensure the detected .git is a directory, not a file. - Adds console.debug logs for easier debugging of the watcher's state.
This commit addresses several issues in the test suite for the `useGitBranchName` hook. The initial `fs/promises` mock was incomplete, causing test failures. This has been corrected to include a default export. Additionally, two previously skipped tests related to the file watcher functionality have been enabled and refactored. They were timing out due to an incompatibility between `waitFor` and `vi.useFakeTimers()`. The tests now avoid fake timers and use spies to directly control and assert the watcher's behavior, making them more robust and reliable.
Summary of ChangesHello @yoichiro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a crucial reliability improvement for the CLI's user interface by ensuring the Git branch name displayed in the footer is always accurate and responsive. It introduces a sophisticated file watching mechanism that intelligently monitors for the presence and changes within a Git repository, effectively resolving a bug where the UI failed to update when a repository was initialized post-application startup. The changes also include a comprehensive overhaul of the related test suite, bolstering the stability and maintainability of this feature. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a bug where the Git branch name failed to update when a repository was initialized after the CLI had started. The introduction of the useGitWatcher hook with its two-step watching strategy is a clever solution. The accompanying test suite improvements are also excellent, making the tests more reliable and easier to understand by removing fake timers in favor of spies and waitFor.
My review focuses on improving the robustness of the new useGitWatcher hook. I've identified a potential resource leak due to unhandled asynchronous operations on component unmount, and a cross-platform compatibility issue with fs.watch that could affect macOS users. The suggested changes address these points to make the implementation more resilient.
| function useGitWatcher(cwd: string, onBranchChange: () => void) { | ||
| // This effect implements a two-step strategy to watch for Git branch changes | ||
| // in a way that is both efficient and handles the edge case of a Git | ||
| // repository being initialized after the CLI has started. | ||
| // | ||
| // 1. Watch .git/HEAD directly: If the repository exists on startup, | ||
| // we watch the .git/HEAD file for changes. This is the most direct and | ||
| // efficient method, as this file is updated whenever the branch changes. | ||
| // | ||
| // 2. Watch for .git directory creation: If .git/HEAD doesn't exist at | ||
| // startup, we fall back to watching the current working directory (cwd) | ||
| // for the creation of the .git directory. This handles the case where the | ||
| // user runs `git init` after starting the CLI. Once the .git directory | ||
| // is detected, we switch to watching .git/HEAD as in step 1. | ||
| useEffect(() => { | ||
| onBranchChange(); // Initial fetch | ||
|
|
||
| const gitHeadPath = path.join(cwd, '.git', 'HEAD'); | ||
| let headWatcher: fs.FSWatcher | undefined; | ||
| let cwdWatcher: fs.FSWatcher | undefined; | ||
|
|
||
| const setupHeadWatcher = () => { | ||
| // Stop watching the cwd for .git creation if we are now watching HEAD. | ||
| cwdWatcher?.close(); | ||
| try { | ||
| console.debug('[GitBranchName] Setting up HEAD watcher.'); | ||
| headWatcher = fs.watch(gitHeadPath, (eventType: string) => { | ||
| if (eventType === 'change' || eventType === 'rename') { | ||
| onBranchChange(); | ||
| } | ||
| }); | ||
| } catch (e) { | ||
| // Ignore errors, which can happen if the file is deleted. | ||
| console.debug( | ||
| '[GitBranchName] Error setting up HEAD watcher. Ignoring.', | ||
| e, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const setupCwdWatcher = () => { | ||
| try { | ||
| console.debug( | ||
| '[GitBranchName] .git/HEAD not found. Setting up CWD watcher.', | ||
| ); | ||
| cwdWatcher = fs.watch(cwd, (eventType, filename) => { | ||
| if ( | ||
| (eventType === 'rename' || eventType === 'change') && | ||
| filename === '.git' | ||
| ) { | ||
| // Check if the created .git is actually a directory. | ||
| fs.promises | ||
| .stat(path.join(cwd, '.git')) | ||
| .then((stats) => { | ||
| if (stats.isDirectory()) { | ||
| console.debug( | ||
| '[GitBranchName] .git directory detected. Switching to HEAD watcher.', | ||
| ); | ||
| // .git directory was created. Try to set up the HEAD watcher. | ||
| onBranchChange(); | ||
| setupHeadWatcher(); | ||
| } else { | ||
| console.debug( | ||
| '[GitBranchName] .git was found, but it is not a directory. Ignoring.', | ||
| ); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| // Ignore stat errors. | ||
| console.debug( | ||
| '[GitBranchName] Error checking .git status. Ignoring.', | ||
| err, | ||
| ); | ||
| }); | ||
| } | ||
| }); | ||
| } catch (e) { | ||
| // Ignore errors. | ||
| console.debug( | ||
| '[GitBranchName] Error setting up CWD watcher. Ignoring.', | ||
| e, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| fsPromises | ||
| .access(gitHeadPath, fs.constants.F_OK) | ||
| .then(() => { | ||
| // .git/HEAD exists, watch it directly. | ||
| setupHeadWatcher(); | ||
| }) | ||
| .catch(() => { | ||
| // .git/HEAD does not exist, watch the cwd for .git creation. | ||
| setupCwdWatcher(); | ||
| }); | ||
|
|
||
| return () => { | ||
| console.debug('[GitBranchName] Closing watchers.'); | ||
| headWatcher?.close(); | ||
| cwdWatcher?.close(); | ||
| }; | ||
| }, [cwd, onBranchChange]); | ||
| } |
There was a problem hiding this comment.
This new useGitWatcher hook has a couple of potential issues related to race conditions and cross-platform compatibility.
-
Resource Leak on Unmount: The
useEffecthook initiates asynchronous operations (fsPromises.accessandfs.promises.stat) but doesn't handle the case where the component unmounts before these promises resolve. If unmounting occurs, the.then()callbacks could still execute, callingsetupHeadWatcherorsetupCwdWatcher. This would createfs.FSWatcherinstances that are never closed, leading to a resource leak. -
Platform Incompatibility in
fs.watch: The watcher for the current working directory (cwdWatcher) checksfilename === '.git'. However, thefilenameargument in thefs.watchcallback is not guaranteed on all platforms. Specifically on macOS, it can benullforrenameevents. This means that runninggit initmight not be detected on macOS, defeating the purpose of this new watcher logic.
I suggest adding a cleanup flag to prevent state updates after unmounting and making the filename check more robust to handle null values.
function useGitWatcher(cwd: string, onBranchChange: () => void) {
useEffect(() => {
onBranchChange(); // Initial fetch
const gitHeadPath = path.join(cwd, '.git', 'HEAD');
let headWatcher: fs.FSWatcher | undefined;
let cwdWatcher: fs.FSWatcher | undefined;
let isMounted = true;
const setupHeadWatcher = () => {
if (!isMounted) return;
// Stop watching the cwd for .git creation if we are now watching HEAD.
cwdWatcher?.close();
cwdWatcher = undefined;
try {
console.debug('[GitBranchName] Setting up HEAD watcher.');
headWatcher = fs.watch(gitHeadPath, (eventType: string) => {
if (eventType === 'change' || eventType === 'rename') {
onBranchChange();
}
});
} catch (e) {
// Ignore errors, which can happen if the file is deleted.
console.debug(
'[GitBranchName] Error setting up HEAD watcher. Ignoring.',
e,
);
}
};
const setupCwdWatcher = () => {
if (!isMounted) return;
try {
console.debug(
'[GitBranchName] .git/HEAD not found. Setting up CWD watcher.',
);
cwdWatcher = fs.watch(cwd, (eventType, filename) => {
// On macOS, filename can be null for rename events.
// To be robust, we check for the existence of the .git directory
// if the filename is '.git' or if it's null.
if (
(eventType === 'rename' || eventType === 'change') &&
(filename === '.git' || filename === null)
) {
// Check if the created .git is actually a directory.
void fsPromises
.stat(path.join(cwd, '.git'))
.then((stats) => {
if (!isMounted) return;
if (stats.isDirectory()) {
console.debug(
'[GitBranchName] .git directory detected. Switching to HEAD watcher.',
);
// .git directory was created. Try to set up the HEAD watcher.
onBranchChange();
setupHeadWatcher();
} else {
console.debug(
'[GitBranchName] .git was found, but it is not a directory. Ignoring.',
);
}
})
.catch((err) => {
// Ignore stat errors (e.g. file not found if the change was not .git creation)
if (isMounted) {
console.debug(
'[GitBranchName] Error checking .git status. Ignoring.',
err,
);
}
});
}
});
} catch (e) {
// Ignore errors.
console.debug(
'[GitBranchName] Error setting up CWD watcher. Ignoring.',
e,
);
}
};
void (async () => {
try {
await fsPromises.access(gitHeadPath, fs.constants.F_OK);
if (isMounted) {
// .git/HEAD exists, watch it directly.
setupHeadWatcher();
}
} catch {
if (isMounted) {
// .git/HEAD does not exist, watch the cwd for .git creation.
setupCwdWatcher();
}
}
})();
return () => {
isMounted = false;
console.debug('[GitBranchName] Closing watchers.');
headWatcher?.close();
cwdWatcher?.close();
};
}, [cwd, onBranchChange]);
}There was a problem hiding this comment.
Thanks, I addressed them at 6cc7bac.
There was a problem hiding this comment.
Thanks for addressing these points, yoichiro! I've reviewed the changes in 6cc7bac and confirm that the resource leak on unmount and the platform incompatibility issues have been resolved as suggested. The addition of isMounted checks and the robust filename handling in fs.watch significantly improve the reliability of the useGitWatcher hook. Great work!
This commit addresses a race condition and potential resource leak in the `useGitBranchName` hook. The changes include: - Introducing a `isMounted` flag to prevent asynchronous operations from executing after the component has unmounted. - Ensuring that the `cwdWatcher` is properly closed and cleaned up when switching to the `headWatcher`. - Improving the reliability of detecting the `.git` directory creation on macOS, where the filename in the watcher callback can be null.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of the git branch watcher not updating when a repository is initialized after the CLI starts. The introduction of the useGitWatcher hook with its two-step watching strategy is a robust solution. The accompanying test suite improvements are also excellent, making the tests more reliable and deterministic. I have one suggestion to prevent a potential resource leak in the new watcher logic.
| // Stop watching the cwd for .git creation if we are now watching HEAD. | ||
| cwdWatcher?.close(); | ||
| cwdWatcher = undefined; |
There was a problem hiding this comment.
There's a potential resource leak in setupHeadWatcher. This function can be called multiple times in quick succession, for instance if the cwdWatcher emits several events when git init is run. Each call to fs.watch creates a new watcher instance. Because the existing headWatcher isn't closed before being reassigned, the old watcher instance is leaked, continuing to consume resources in the background.
To fix this, you should ensure any existing headWatcher is closed before creating a new one.
// Clean up existing watchers to prevent resource leaks.
headWatcher?.close();
cwdWatcher?.close();
cwdWatcher = undefined;There was a problem hiding this comment.
I have addressed this at 2fb6a64. Thanks!
This commit ensures that any existing `headWatcher` instance is properly closed before a new one is created. This prevents potential resource leaks if the watcher setup logic is ever called multiple times.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of the git branch watcher not initializing correctly if the repository is created after the CLI starts. The introduction of the useGitWatcher hook with its two-step watching strategy is a solid approach. The refactoring of the test suite is also a significant improvement, making the tests more reliable and easier to understand. I've identified a couple of edge cases in the new useGitWatcher hook related to component unmounting and file deletion that could be addressed to make the implementation even more robust. Overall, this is a great improvement to the feature's reliability.
| headWatcher = fs.watch(gitHeadPath, (eventType: string) => { | ||
| if (eventType === 'change' || eventType === 'rename') { | ||
| onBranchChange(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The headWatcher is not resilient to the deletion of the .git/HEAD file. When a watched file is deleted, the fs.FSWatcher instance may stop, depending on the platform. This means if a user deletes and then re-initializes the git repository, the branch watcher will become inactive and will not recover.
To make this more robust, the hook should handle the termination of the headWatcher and fall back to setupCwdWatcher. A possible approach is to check for the file's existence within the rename event handler and re-initialize the CWD watcher if the file is gone.
There was a problem hiding this comment.
Thanks. I have addressed this at 878b647.
There was a problem hiding this comment.
Thanks for addressing this, yoichiro!
| '[GitBranchName] .git directory detected. Switching to HEAD watcher.', | ||
| ); | ||
| // .git directory was created. Try to set up the HEAD watcher. | ||
| onBranchChange(); |
There was a problem hiding this comment.
There is a potential race condition here. onBranchChange is an async function (fetchBranchName) that eventually updates React state. While there is an isMounted check before this call, the component could unmount after onBranchChange has started but before it completes its async operations (like spawnAsync) and calls the state setter. This would lead to a "Can't perform a React state update on an unmounted component" warning and a potential memory leak.
The isMounted logic should be handled by the component that owns the state (useGitBranchName in this case), typically by checking the mount status within the async callback just before updating the state.
There was a problem hiding this comment.
Thank you, I have refactored and modified some logics at 1ef34ab.
There was a problem hiding this comment.
Thank you for the update, yoichiro! I've reviewed the changes in commit 1ef34ab and can confirm that the isMounted check has been correctly implemented within the fetchBranchName callback before updating the React state. This effectively addresses the potential race condition I highlighted, preventing state updates on unmounted components.
Great work on the refactoring and the improved reliability of the Git branch watcher!
This commit improves the robustness of the 'useGitWatcher' hook. Previously, if the '.git/HEAD' file was deleted after the 'headWatcher' was initialized (e.g., by deleting and re-initializing the git repository), the watcher would stop and not recover. This change adds logic to the 'headWatcher''s 'rename' event handler. It now checks for the existence of the '.git/HEAD' file. If the file is missing, it cleans up the defunct watcher and falls back to watching the current working directory for the '.git' directory to be created again. This ensures the branch watcher can recover from repository re-initialization.
This commit addresses a potential race condition in the 'useGitBranchName' hook where a state update could be attempted on an unmounted component. The responsibility for managing the component's lifecycle is moved from the 'useGitWatcher' hook to the 'useGitBranchName' hook, which owns the state. Changes: - A 'isMounted' ref is introduced in 'useGitBranchName' to track the component's mounted status. - The 'fetchBranchName' callback now checks if the component is still mounted before attempting to update the state. - The 'useGitWatcher' hook is simplified by removing its own 'isMounted' logic, as it's no longer responsible for lifecycle management.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue of the git branch watcher not updating when a repository is initialized post-application start. The introduction of the useGitWatcher hook with a two-step watching strategy is a robust and well-thought-out solution. Additionally, the significant improvements to the test suite, moving away from fake timers to spies and waitFor, greatly enhance the reliability and maintainability of the tests. I have one high-severity suggestion to prevent a potential resource leak in the new watcher logic.
| const setupCwdWatcher = () => { | ||
| try { |
There was a problem hiding this comment.
There's a potential resource leak here. If setupCwdWatcher is called multiple times (e.g., due to multiple rename events on .git/HEAD firing in quick succession), it could create multiple fs.FSWatcher instances for the current working directory without closing the old ones. This can lead to excessive resource consumption in a long-running application. To prevent this, you should close any existing cwdWatcher before creating a new one, similar to how it's done in setupHeadWatcher.
const setupCwdWatcher = () => {
// Ensure any previous CWD watcher is closed before creating a new one.
cwdWatcher?.close();
try {There was a problem hiding this comment.
I have addressed this at 866e5b9. Thanks.
There was a problem hiding this comment.
Thank you for addressing this, yoichiro! I've reviewed the changes and confirm that adding cwdWatcher?.close(); before creating a new watcher effectively prevents the resource leak. Much appreciated!
|
Be aware that we're not reviewing big PRs from external contributors for the next couple weeks. |
|
@scidomino OK, I understand it. Well, will the reviewing from external contributors be restarted after releasing v1.0? |
That'a the plan! |
The `setupCwdWatcher` function could create multiple `fs.FSWatcher` instances without closing the old ones, leading to excessive resource consumption. This change ensures that any existing `cwdWatcher` is closed before creating a new one, mirroring the behavior of `setupHeadWatcher.
TLDR
This PR fixes a bug where the git branch name in the footer would not appear or update if the git repository was initialized after the CLI had already started. It also improves the test suite for this feature to ensure its reliability.
This change resolves issue #10402.
Dive Deeper
The core of the issue was that the application only checked for a git repository at startup. The existing file watcher wasn't set up to detect the creation of the
.gitdirectory itself.This PR introduces a more robust, two-step watching strategy:
.gitdirectory is found on startup, it watches the current working directory for the.gitdirectory to be created..gitdirectory appears, it switches its focus to watching the.git/HEADfile for any changes (i.e., branch switches or new commits).To keep the code clean and reusable, this watching logic has been extracted into a new custom hook,
useGitWatcher.Additionally, the associated test suite for the
useGitBranchNamehook has been significantly improved:fs/promises.Reviewer Test Plan
To validate this change, you can manually reproduce the original bug and confirm it's now fixed:
npm run build.cdinto it (e.g.,mkdir /tmp/test-dir && cd /tmp/test-dir).node /path/to/your/gemini-cli/bundle/cli.js).cdinto the same directory (/tmp/test-dir) and rungit init.main(ormaster) branch name.git checkout -b test-branch).test-branch.Testing Matrix
Linked issues / bugs
Fixes #10402