specs(GH10207): file tree degraded-mode handling#10490
specs(GH10207): file tree degraded-mode handling#10490SagarSDagdu wants to merge 2 commits intowarpdotdev:masterfrom
Conversation
Evaluates the three questions raised in PR review on warpdotdev#10234: 1. Pre-detect too-many-files vs reactive fallback — recommend keeping the reactive fallback as the load-bearing mechanism; consider a git-ls-files probe only as a later optimization once telemetry shows the partial walk matters. 2. Toast vs persistent indicator — recommend a per-root, dismissible inline indicator carried via an `indexed_with_limit` flag on `FileTreeState`, replacing the transient toast and the dedicated event variant. Survives view-mount-after-indexing (which is why the toast was missed during manual testing). 3. Lazy-load performance — keep the existing 5k cap; instrument before tuning; consider moving lazy-load off the main thread as a follow-up. Also flags the parallel ExceededMaxFileLimit consumers in ai::project_context, ai::outline, and ai::index that this PR does not change.
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this spec-only 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 TECH.md proposes a degraded file-tree mode for repositories that exceed MAX_FILES_PER_REPO, replacing a transient toast with persistent inline state and documenting lazy-load performance follow-ups.
Concerns
- The degraded retry is specified as a depth-1
build_treeretry, but the spec does not define behavior for repos with more than the file quota directly under the root, so the fallback can still fail and reproduce the empty-tree state. - The telemetry follow-up still refers to
RepositoryIndexedWithLimitafter the spec recommends deleting that event, leaving the instrumentation source ambiguous. - One testing path names a non-existent test file, which can misdirect implementation.
Verdict
Found: 0 critical, 1 important, 2 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
- Q1: call out that the current PR's depth-1 retry still uses a 100k file quota, so a repo with >100k files directly at the root would hit ExceededMaxFileLimit on the retry and reproduce the original empty-tree bug. Spec now requires the depth-1 retry to pass `None` for the file quota; bounded cost is the count of root-level entries (subdirectories are unloaded immediately at depth=1). - Add a unit-test entry covering the >100k-files-at-root regression so the gap is validated. - Drop the `RepositoryIndexedWithLimit` event name from the telemetry follow-up since Q2 recommends dropping that event entirely; reference the persistent `indexed_with_limit` state transition and the existing `BuildTreeFailed` telemetry at local_model.rs:1001 instead.
|
/oz-review |
|
I'm re-reviewing this spec-only 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 tech spec covers the file-tree degraded-mode fallback for repositories exceeding MAX_FILES_PER_REPO, including the reactive indexing strategy, user-facing degraded indicator, lazy-load performance posture, validation plan, and follow-up work.
Concerns
- The degraded-mode filesystem skill probe needs an explicit symlink/root-containment requirement and validation so it preserves the trust boundary of the existing tree builder.
- The
FileTreeStatecompatibility guidance omitsfrom_file_tree_entry, so an implementation following the spec would either fail to compile or leave remote/file-tree constructed state underspecified.
Security
- The filesystem fallback for repo-local skills must reject symlinked or repo-escaping skill directories, not just gitignored paths.
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
| on the depth-1 retry the test asserts `IndexedRepoState::Indexed` with | ||
| `indexed_with_limit == true` and the root entry containing all top-level | ||
| files. | ||
| - Unit: `find_top_level_skill_directories_in_filesystem` honors gitignore. |
There was a problem hiding this comment.
skills directories and any path escaping the repo so degraded-mode probing preserves the existing Entry::build_tree trust boundary.
| - **Carrying `indexed_with_limit` on `FileTreeState`** is observable from | ||
| many crates that pattern-match the struct. We avoid breakage by adding a | ||
| `bool` field with a `Default` value of `false` and updating the two | ||
| constructors (`new`, `new_lazy_loaded`); existing call sites compile |
There was a problem hiding this comment.
FileTreeState also has from_file_tree_entry; specify how indexed_with_limit is initialized there (probably false unless remote metadata can carry the flag), otherwise following this spec leaves one constructor broken or ambiguous.
| when registering a root via `register_and_refresh_lazy_loaded_directory` | ||
| (`view.rs:1532-1589`). | ||
|
|
||
| If the team prefers no UI signal at all, drop the indicator entirely and |
There was a problem hiding this comment.
my proposal here is no toast, in the context of the file tree there's no significant degradation of experience. if the user searches for rules / results and things are missing, that's outside of the scope of the file tree
| independently and hit the same hard limit. They are tracked separately and | ||
| this spec does not change their behavior. | ||
|
|
||
| ## Proposed changes |
There was a problem hiding this comment.
related to this comment let's be explicit about the symlinked case and expected behavior there
|
@SagarSDagdu I just chatted with @alokedesai, we're going to talk more on Monday about if we want to just lazy load in general for the file tree instead of eagerly loading and then lazy loading only if the codebase size is too big. I'll come back here with a decision after that discussion! |
2 participants
Description
Tech spec for issue #10207 (Project Explorer fails to expand populated folders in large repos after
ExceededMaxFileLimit).Splitting this out from PR #10234 (which carries a conservative reactive fallback already) so the design questions @moirahuang raised can be debated independently of the existing code review.
The spec evaluates three open questions:
git ls-files | wc -l) and the savings are bounded to a partial walk in actx.spawnbackground task. Worth revisiting later as an optimization once telemetry tells us whether it matters.indexed_with_limitflag onFileTreeState(mirrors the existing "Codebase too large" pattern in AI settings). The transient toast was easy to miss during manual testing.Also flags the parallel
ExceededMaxFileLimitconsumers inai::project_context::model,ai::outline::native, andai::index::full_source_code_embeddingthat this PR (and #10234) do not change.Linked Issue
#10207
ready-to-specorready-to-implement.Screenshots / Videos
N/A — spec-only change.
Testing
N/A — documentation. The validation plan for the implementation lives in the
Testing and validationsection ofTECH.md.Agent Mode