Fix/pending race condition#9902
Conversation
When a user opens the file tree immediately after cloning a repository, there's a race condition where the terminal hasn't yet triggered git detection, so the file tree falls back to lazy-loading instead of triggering proper repo indexing. This fix checks if a path is a valid git repository by verifying both the .git directory exists AND .git/HEAD is a valid file. This avoids treating stale/invalid .git folders as repos. If valid, we trigger proper git detection instead of lazy-loading, ensuring the full file tree gets built even when opened immediately after a fresh clone. Fixes warpdotdev#9846
When a user opens the file tree immediately after cloning a repository, there's a race condition where the terminal hasn't yet triggered git detection, so the file tree falls back to lazy-loading instead of triggering proper repo indexing. This fix checks if a path has a .git entry (directory or file for worktrees/submodules) and triggers detection. The async detection handles validation properly, including gitfile parsing for worktrees. If the repo has previously failed to index (e.g., exceeded file limit), we fallback to lazy-loading to avoid repeatedly retrying failed detection. Fixes warpdotdev#9846
Properly validate .git files (used by worktrees/submodules) by reading the gitdir path and verifying the actual git directory has a valid HEAD. This ensures worktrees and submodules also trigger proper repo detection instead of falling back to lazy-loading.
Test verifies that when file tree is opened on a directory with a valid .git entry, proper git detection is triggered instead of falling back to shallow lazy-loading. This addresses reviewer feedback about adding test coverage for the new detection path.
The previous test passed unconditionally. Now verifies that README.md and src/ are visible in the file tree - if lazy-loading occurred instead of proper detection, these items won't be discoverable and the test fails.
Expands src/ and verifies main.rs is visible. First-level entries like README.md and src appear in both lazy-loaded and fully-indexed trees, so they don't prove the fix works. Only nested content like src/main.rs proves proper repo indexing occurred.
This ensures the file tree is never empty if detection returns None or fails. Previously we only did lazy-loading for non-git dirs, but now we always register lazy-loaded path first, then trigger detection on top - detection will replace with full content if successful.
Security: limit synchronous .git file reads to prevent malformed local folders from stalling the file tree open. gitfile format is small (~100B), so 1KB is plenty.
Use std::io::Read with a bounded buffer instead of unbounded std::fs::read. This prevents malicious/malformed .git files from causing large memory allocations or UI stalls on the view path.
Root cause: When get_repository returns None for Pending state, the file tree creates an empty entry instead of preserving the lazy-loaded fallback content. Fix: Check repo_state explicitly. If Pending and we have a lazy-loaded path registered, keep that entry instead of showing empty tree. This preserves the lazy-loaded tree while detection completes.
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR attempts to keep the file tree populated while newly cloned repositories move from lazy-loaded file tree state to full repository indexing, and adds an integration test for the regression.
Concerns
- The
Pendinghandling still returnsNonewhen the path was already pending before this view registered a lazy-loaded fallback, which can recreate the empty-tree race. - The new integration test does not force or assert the
IndexedRepoState::Pendingcase, and its nested-file interaction can also pass through the existing lazy-loading path. - For faster review, please upload screenshots or a video of the file tree behavior working end to end.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| Some(IndexedRepoState::Indexed(state)) => Some(state.entry.clone()), | ||
| // Pending - repo is being indexed, use lazy-loaded entry if available | ||
| Some(IndexedRepoState::Pending) => { | ||
| if self.registered_lazy_loaded_paths.contains(path) { |
There was a problem hiding this comment.
Pending before this view registers a lazy fallback: index_lazy_loaded_path no-ops for Pending, registered_lazy_loaded_paths remains false, and this branch returns None.
| .with_action(|app, _, _| open_file_tree_panel(app)), | ||
| ) | ||
| // With the fix, detection triggers immediately on .git dirs, so the repo | ||
| // should NOT be in lazy-loaded state. We verify by clicking on main.rs |
There was a problem hiding this comment.
src loads nested children; make it force/assert the IndexedRepoState::Pending race instead of relying on main.rs visibility.
Check is_lazy_loaded_path from the model in addition to local registry to handle the case where detection started before file tree opened.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR tries to keep the Project Explorer populated while git repository indexing is pending by preserving lazy-loaded entries and triggering repo detection from the file tree.
Concerns
- The pending-state path can still replace the root with an empty entry when detection has already set the repo to
Pending, because lazy registration is skipped in that state. - The new regression test can pass through the existing lazy-loading path and does not reliably prove the pending race is fixed.
- For faster review, please upload screenshots or a video of the file tree working end to end after opening it immediately after cloning a repository.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // Fully indexed - use the repo's entry | ||
| Some(IndexedRepoState::Indexed(state)) => Some(state.entry.clone()), | ||
| // Pending - repo is being indexed, use lazy-loaded entry if available | ||
| Some(IndexedRepoState::Pending) => { |
There was a problem hiding this comment.
Pending, index_lazy_loaded_path returns without registering a lazy entry, so this branch still falls through to create_empty_entry and shows an empty tree until full indexing completes; create or preserve a shallow fallback for the pending case.
| // which only works if full indexing happened (lazy-loading loads shallow tree). | ||
| // Note: This tests the detection path; old lazy-loading would need expand. | ||
| .with_step( | ||
| new_step_with_default_assertions("Expand src to verify nested content loads") |
There was a problem hiding this comment.
src exercises the lazy-loaded path too, so this test can pass without proving full repo indexing or the pending-race fix; reproduce the pending state or assert behavior that lazy-loading alone cannot satisfy.
Previously index_lazy_loaded_path would skip registration when repo was already Pending, causing empty tree. Now we force local registration as fallback to prevent empty tree while detection completes.
When repo is in IndexedRepoState::Pending (detection already running), the file tree previously showed empty because get_repository returns None for Pending. Now we: 1. Always register lazy-loaded fallback even if model skips it 2. Preserve the fallback entry while detection completes 3. Removed forced .git detection - let the existing detection flow work
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR attempts to preserve file-tree contents while repo metadata is pending and adds integration coverage for the clone/open race.
Concerns
- The pending fallback marks paths as locally registered without building the lazy-loaded tree or registering the watcher, so ordinary standalone directories and pending repos with no existing lazy entry can stay empty and skip future indexing.
- The new integration test does not force
IndexedRepoState::Pendingor distinguish full git indexing from the existing shallow lazy-loading path.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| if !is_lazy_loaded { | ||
| // Model may skip registration if repo is already Pending. | ||
| // Force register a local fallback entry to prevent empty tree. | ||
| self.registered_lazy_loaded_paths.insert(path.clone()); |
There was a problem hiding this comment.
index_lazy_loaded_path; when there is no existing lazy entry, the model never builds the shallow tree or watcher and future refreshes skip indexing because this set now contains the path.
| // which only works if full indexing happened (lazy-loading loads shallow tree). | ||
| // Note: This tests the detection path; old lazy-loading would need expand. | ||
| .with_step( | ||
| new_step_with_default_assertions("Expand src to verify nested content loads") |
There was a problem hiding this comment.
IndexedRepoState::Pending, and expanding src then clicking main.rs also succeeds through the existing shallow lazy-loading path.
…watcher Previously we just marked locally registered without triggering the model to build the lazy-loaded tree. Now we actually call index_lazy_loaded_path which builds the tree and registers the watcher, then handle Pending state as fallback.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates the file tree refresh path to account for repositories that are still in IndexedRepoState::Pending and adds an integration regression test for opening the file tree after cloning.
Concerns
- Pending repositories can still overwrite an existing lazy-loaded fallback with an empty root entry, so the race can continue to show an empty tree until full indexing finishes.
- Lazy-loading errors no longer preserve the previous exceeded-file-limit toast or warning log, which turns real failures into a silent empty tree.
- The new test can pass through the lazy-loading path because it expands
srcbefore clickingmain.rs; it does not prove full indexing handled the initial open. - For faster review, please upload screenshots or a video of the file tree behavior working end to end.
Verdict
Found: 0 critical, 2 important, 3 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let repo_id = repo_metadata::RepositoryIdentifier::local(path.clone()); | ||
| let repo_state = RepoMetadataModel::as_ref(ctx).repository_state(&repo_id, ctx); | ||
| let is_pending = matches!(repo_state, Some(IndexedRepoState::Pending)); | ||
| let previously_failed = matches!(repo_state, Some(IndexedRepoState::Failed(_))); |
There was a problem hiding this comment.
💡 [SUGGESTION] previously_failed is never used, so this will trip unused-variable/clippy checks; either remove it or use it in the retry decision.
| } | ||
|
|
||
| // For Pending state or failed indexing, ensure we have a placeholder entry | ||
| if is_pending || index_result.is_err() { |
There was a problem hiding this comment.
index_result error as an empty placeholder without restoring the previous ExceededMaxFileLimit toast or warning log, so lazy-loading failures become a silent empty tree; keep the old error handling before falling back.
| // For Pending state or failed indexing, ensure we have a placeholder entry | ||
| if is_pending || index_result.is_err() { | ||
| if let Some(root_dir) = self.root_directories.get_mut(path) { | ||
| root_dir.entry = Self::create_empty_entry(path); |
There was a problem hiding this comment.
create_empty_entry; index_lazy_loaded_path no-ops for Pending, so this clears the shallow fallback that the later branch is trying to preserve.
| // which only works if full indexing happened (lazy-loading loads shallow tree). | ||
| // Note: This tests the detection path; old lazy-loading would need expand. | ||
| .with_step( | ||
| new_step_with_default_assertions("Expand src to verify nested content loads") |
There was a problem hiding this comment.
💡 [SUGGESTION] This does not prove full indexing: a lazy-loaded root can show src, and expanding it can load main.rs, so the test can pass without exercising the pending-repo fallback.
- Don't overwrite existing entry when lazy-loading fails - Restore exceeded-file-limit toast and warning log for failures - Only create placeholder for Pending if no lazy-loaded fallback exists
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
Although the containing impl blocks were already gated, this adds explicit #[cfg(any(test, feature = test-util))] attributes to the force_pending_state methods themselves to ensure they are strictly unavailable in production builds and to provide better clarity for reviewers.
|
/oz-review |
|
I ran into an unexpected error while working on this. Powered by Oz |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes file tree rendering for repositories in Pending state by adding a pending lazy fallback tree, then adds an integration regression test for opening the file tree immediately after clone.
Concerns
- The new integration test forces the repository into
Pendingpermanently, but later tries to clicksrc/main.rs; the pending fallback is only first-level lazy content, and directory loading still bails whileget_repository()returnsNoneforPending, so the nested file is not materialized.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ) | ||
| .with_step( | ||
| new_step_with_default_assertions("Click main.rs to open it") | ||
| .with_click_on_saved_position("file_tree_item:main.rs"), |
There was a problem hiding this comment.
main.rs, but the test leaves the repo permanently Pending and the pending fallback is built with depth 1, so expanding src cannot materialize src/main.rs (load_directory_from_model returns while get_repository is None). Assert a top-level fallback item or exercise a real completion path before clicking nested content.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
The pending fallback is only a shallow (first-level) lazy tree. Sub-directory expansion is not supported while the repository is in Pending state because directory loading requires a real repository handle which is None for Pending. Modified the integration test to only verify visibility of top-level items (README.md and src) while the repo is forced into Pending, which correctly exercises the fix without attempting unsupported nested materialization.
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR preserves a shallow file-tree fallback while repository indexing is still Pending, avoiding the empty-tree state when the Project Explorer opens immediately after clone. It also adds an integration regression covering the Pending fallback path.
Concerns
- No blocking correctness or security concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR preserves a shallow file-tree fallback while repository indexing is still Pending, avoiding the empty-tree state when the Project Explorer opens immediately after clone. It also adds an integration regression covering the Pending fallback path.
Concerns
- No blocking correctness or security concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
moirahuang
left a comment
moirahuang
left a comment
There was a problem hiding this comment.
Appreciate you putting up a PR! I ended up also creating a PR yesterday (#9998) which gets at some of the same Pending logic. @alokedesai has said he'll review that PR today
That's cool for real |
|
Closing this since my fix was merged! |
2 participants
Description
Fix race condition where the file tree shows an empty tree when opened immediately after cloning a repository.
When a user opens the file tree immediately after cloning a repo, the terminal hasn't triggered detection yet, so the repo is in IndexedRepoState::Pending. The previous code used get_repository() which returns None for Pending state, causing an empty entry to be created. This fix checks repository_state explicitly and preserves the lazy-loaded fallback entry while detection completes.
Linked Issue
Testing
Existing file tree tests cover this functionality. The fix uses the existing lazy-loaded path infrastructure.
CHANGELOG-BUG-FIX: Fixed file tree sidebar showing empty when opened immediately after cloning a repository