spec: file tree search (#10320)#10460
Conversation
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
Reviewed the spec-only addition for File Tree / Project Explorer search. The behavior contract covers activation, matching, filtering, navigation, restoration, tests, and telemetry, and the supplemental security pass did not identify a design-level security concern.
Concerns
- The show-all setting is internally inverted between the behavior contract and Settings/API surface.
- Several implementation pointers do not match the current Warp file tree, keybinding, or telemetry surfaces, which would send implementation work down stale paths.
Verdict
Found: 0 critical, 4 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
|
|
||
| ## Settings / API surface | ||
|
|
||
| - `file_tree.search.show_all_on_filter` — `bool`, default `false`. When |
There was a problem hiding this comment.
true means showing all/dimming and false means hiding, but this says the inverse with default false; choose one semantic/default so implementation and users do not invert the toggle.
| - Keybinding registration for `Cmd+F` (mac) / `Ctrl+F` (win/linux), scoped | ||
| to the new `KeyContext::FileTreeFocus` so it does not collide with editor | ||
| or terminal `Cmd+F`. |
There was a problem hiding this comment.
KeyContext::FileTreeFocus does not exist in the current keybinding system; file tree bindings are registered with FixedBinding scoped to id!(FileTreeView::ui_name()), so the spec should either propose the new context architecture or use the existing scoped binding mechanism.
| - New module `app/src/file_tree/search.rs` for the matching pipeline and | ||
| match-list state. | ||
| - Update `app/src/file_tree/header.rs` to add the magnifier button. | ||
| - Update `app/src/file_tree/tree_view.rs` for the filter + highlight render | ||
| pipeline and auto-expand on match. |
There was a problem hiding this comment.
app/src/code/file_tree/view.rs and view/render.rs are the relevant files); update the pointers or describe the intended module moves so implementers do not build against stale paths.
| Extend the existing `file_tree.opened` event with an optional `search_used` | ||
| boolean per tree-panel session (set `true` if the user opened the search box | ||
| at any point during that session). No new event types are introduced. |
There was a problem hiding this comment.
TelemetryEvent::FileTreeToggled / "File Tree Toggled", not file_tree.opened, and it is emitted when opening; specify the actual event/payload timing needed to attach search_used after a session has observed search usage.
|
Round-1 fixes for oz-for-oss review: (1) renamed setting to |
|
/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
Reviewed the file-tree search spec for completeness, clarity, feasibility, issue alignment, internal consistency, and high-level security concerns. The spec covers the main behavior, settings, tests, and telemetry shape, but two core matching semantics need to be pinned down before implementation.
Concerns
- Folder matches are underspecified: the spec says children become in-context matches, but later hidden-mode and navigation/count behavior do not define whether those descendants are visible, counted, highlighted, or navigable.
- Unicode matching is underspecified for a case-insensitive search: NFC normalization is defined, but non-ASCII case folding is not.
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
| - Case-insensitive substring match. | ||
| - Match is checked against each path component (folder names and the file | ||
| basename) and against the full relative path string. | ||
| - A folder match reveals all of the folder's children as in-context matches |
There was a problem hiding this comment.
| basename) and against the full relative path string. | ||
| - A folder match reveals all of the folder's children as in-context matches | ||
| (folder-as-context). | ||
| - Matching is locale-insensitive for ASCII; Unicode normalized using NFC |
There was a problem hiding this comment.
|
Round-2 fixes: (1) folder-match descendant semantics now explicit — descendants visible (dim mode) or visible (hidden mode), counted via union (no double-count), only the folder name itself highlighted, navigation cycles depth-first/lex through file matches and folder-match descendants; acceptance + tests added; (2) Unicode case folding now explicitly Unicode Default Case Folding per TR#21 (full folding, status C+F), applied AFTER NFC normalization; locale-independent (no Turkish-specific I/i); test coverage for |
|
/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 spec defines in-tree file search behavior for activation, matching, visibility modes, restoration, tests, and telemetry. The supplemental security pass did not identify design-level security findings.
Concerns
- The Unicode matching contract is internally contradictory: it requires full case folding, but also permits Rust
str::to_lowercase(), which does not satisfy the listedßand sigma cases. - Hide-mode visibility is inconsistent with the folder-match descendant semantics; one section says all descendants of a matched folder remain visible, while the live-filter rule says only matching files plus ancestors render.
Verdict
Found: 1 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
| - In Rust, the equivalent is `str::to_lowercase()` on a valid UTF-8 | ||
| string, which performs full Unicode lowercasing per | ||
| `SpecialCasing.txt`. Implementations MAY use this directly. |
There was a problem hiding this comment.
🚨 [CRITICAL] str::to_lowercase() is lowercasing, not Unicode default case folding; allowing it contradicts the required ß → ss and sigma equivalence tests, so specify an actual full case-folding implementation or relax those requirements.
| - When `false`: non-matching siblings are **hidden** entirely; only | ||
| matching files plus their ancestor folders (auto-expanded) are | ||
| rendered. |
There was a problem hiding this comment.
|
Addressed round-N review: Critical — Unicode case-folding contradiction. The previous wording said the spec required full Unicode Default Case Folding (TR#21 C+F) but ALSO that Resolved by deleting the Important — Hide-mode descendant visibility inconsistency. B2a.1 says all descendants of a matched folder are visible; B3's hide-mode rule said 'only matching files plus their ancestor folders are rendered'. Reconciled in B3 by stating that the rendered set in hide mode is (1) every node in the union match set (direct matches + folder-match descendants per B2a) plus (2) ancestor folders of those nodes. Folder-match descendants qualify as 'matching' for the hide rule because the match set is the union per B2a, so a matched folder DOES keep all descendants visible in hide mode. Wording in B3 explicitly supersedes the earlier 'filename matches plus ancestors' phrasing. Commit: 713003d |
|
/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
Reviewed the spec-only PR for File Tree search. The spec covers activation, filtering, restore behavior, settings, tests, and telemetry, and the supplemental security pass did not identify a design-level security finding.
Concerns
- The match semantics are internally inconsistent: matching against each row's full relative path makes descendants of a matched folder direct matches, while later sections and tests treat them as context-only unless their own names match.
- The settings summary should use the same union match-set wording as the behavior contract so implementers do not read hide mode as filename-only filtering.
Verdict
Found: 0 critical, 1 important, 1 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
| - Case-insensitive substring match. | ||
| - Match is checked against each path component (folder names and the file | ||
| basename) and against the full relative path string. | ||
| - A folder match reveals all of the folder's descendants as in-context |
There was a problem hiding this comment.
src matches src/a.rs), which contradicts B2a.3 and T_folder_match_highlight where descendants are context-only unless their own name matches. Specify whether ancestor-component hits are context matches, or remove full-relative-path matching from descendant rows.
| When `false`, non-matching siblings are hidden entirely (only matching | ||
| files and their ancestor folders are rendered). |
There was a problem hiding this comment.
💡 [SUGGESTION] This setting summary still says only matching files plus ancestors render, which conflicts with B2a's folder-descendant rule; align it with the union match set wording.
| When `false`, non-matching siblings are hidden entirely (only matching | |
| files and their ancestor folders are rendered). | |
| When `false`, non-matching siblings are hidden entirely; rendered nodes are | |
| the union match set (including folder-match descendants per B2a) plus the | |
| ancestor folders needed to reveal those nodes. |
1 participant
Summary
Spec for #10320 — adds in-tree search to the File Tree / Project Explorer, openable via
Cmd+F(Ctrl+Fon Windows/Linux) when the tree has focus and via a discoverable magnifier button in the tree header.This is a spec-only PR. No implementation in this change.
Highlights
Cmd+Fis scoped to a newKeyContext::FileTreeFocusso it does not collide with editor / terminalCmd+F.F3/Shift+F3(andEnter/Shift+Enter).file_tree.search.show_all_on_filtertoggles to hide instead.Escand clear restore prior selection and tree expand state.Scope
Telemetry
Extends the existing
file_tree.openedevent with an optionalsearch_usedboolean per session.Tag:
ready-to-specCloses part of #10320 (spec only — implementation will follow once the spec is approved).