You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Reviewed against master and exercised the endpoint on the live instance. This is a clear improvement over the old client-side hasMatchingChild filter, and it fixes a real bug on the way: _activateFileBrowserSession means the panel stops showing the previous session's tree after a tab switch, and capturing the owner session at render time means a mid-search switch can no longer preview or download the wrong session's file. Escaping looks right throughout (name, path, type, the data-owner attribute, and the download href, which is both encodeURIComponentd and escapeHtmld). Both hide paths are covered: closeFileBrowserPanel and the settings-ui branch are the only two sites that remove visible.
One measured issue and a few small ones.
1. depth=5 on the search request makes almost every search claim "(truncated)"
The search request reuses depth=5. The server caps maxDepth at 10 and the search branch sets truncated = true the moment it hits the depth wall, so on any real repo the status line reads "N matches (truncated)" even when nothing was cut from the match list. Measured on this repo just now, same query:
So at depth 5 it both misses files and cries wolf on the one signal that is supposed to mean "there is more". The flat match list is separately bounded by maxMatches (1000) on top of the shared maxFiles (5000), so sending depth=10 on the q request costs nothing and matches the PR title. The tree request should keep depth=5.
2. The 256 cap is a duplicated magic number
query.length > 256 in filterFileBrowser mirrors MAX_QUERY_LENGTH in src/utils/file-query.ts. If those ever drift the failure is confusing rather than loud: the server compiles a too-long query to null, falls through to the tree branch, and _validateFileBrowserSearchEnvelope rejects the tree envelope, so the user sees "Search failed" instead of the length message. A comment naming MAX_QUERY_LENGTH as the source of truth is enough.
3. Complexity
This adds roughly 420 net lines to panels-ui.js and a hand-rolled state machine over treeEpoch, searchEpoch, inFlight, treeInFlight, normalState, deferredDirectoryTarget and view, with guard predicates checking up to ten conditions, some of which re-read the input element's value. The 53 tests are genuinely good and behavioral, so I am not worried about correctness today. I am worried about the next person changing it. Two asks:
Would an AbortController per request plus one monotonic request id collapse most of searchEpoch / treeEpoch / inFlight / treeInFlight? Stale responses would be aborted rather than fetched and then discarded.
Either way, please add a block comment above _ensureFileBrowserState stating the invariants: what each epoch protects, why the owner session is captured instead of read live, and what deferredDirectoryTarget is for. This repo documents its invariants and this is exactly the code that needs it.
4. Minor
_activateFileBrowserSession clears fileBrowserExpandedDirs on every session switch, even with the panel closed, so switching away and back loses the expansion state. Keying expansion per session would be nicer. Still better than the current behavior, so not a blocker.
A directory matched by search that sits at exactly depth 5 exists as a tree node with no children, so clicking it expands to nothing. That is the tree depth, not the search depth, so (1) does not fix it. Just noting it.
Planning to take this with the next release. Test-merged against #339 with git merge-tree: clean, so the two can land in either order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
UI
Uses the existing File Viewer search input and row styling; no markup or CSS changes.