feat(preferences): filter settings editor by selected category (#17268) - #17526
feat(preferences): filter settings editor by selected category (#17268)#17526safisa wants to merge 12 commits into
Conversation
…se-theia#17268) When a category is selected in the Settings tree, show only that category's preferences in the right pane, and render the full category path as a breadcrumb above the search box. Match VS Code behavior. Behavior: - First-ever load defaults to the "Commonly Used" category. - Selecting a leaf preference filters to the leaf's nearest non-leaf ancestor. - Typing in search clears the category filter (search replaces category). Clicking a category while searching narrows the search results (filters compose). - A category filter persists across scope-tab switches; it is cleared on schema reload if the filtered node no longer exists. Implementation: - PreferenceTreeModel tracks _categoryFilterId, exposes the isDescendantOfCategory helper, computes breadcrumb labels, and pushes them to PreferencesSearchbarWidget via the existing push pattern. - passesCurrentFilters composes scope, search, and category. - PreferencesEditorWidget handles a new PreferenceFilterChangeSource. Category, re-running its renderer hide/show pass. - PreferencesSearchbarWidget renders the breadcrumb with `›` separators above the search input via a new updateBreadcrumb(labels) method.
Moving the category check out of passesCurrentFilters keeps the left tree intact: all sibling categories remain visible (matching VS Code), and the category filter only narrows the right pane. The editor widget consults the model's categoryFilterId via the now-public isDescendantOfCategory helper. Also adds aria attributes on the breadcrumb (<nav aria-label="Settings">, aria-hidden on the › separator) and clarifies the strict-descendant contract on isDescendantOfCategory.
- Drop the search-bar breadcrumb in favor of the in-editor section headers (h1/h2) that VSCode uses. Composite ancestors of the selected category are kept visible above its content via a new isCompositeAncestorOfCategory model helper. - Pin those visible headers with stacked `position: sticky` so the scrollbar moves only the items list underneath them. - Skip hidden renderers in findFirstVisibleChildID: at scrollTop=0, display:none rows report offsetTop=offsetHeight=0 and were matching first, hijacking the selection back to "Commonly Used" whenever the user scrolled to the top of a long category.
|
Hi @safisa, sorry, somehow this PR fell of the plate, could you please rebase to let the CI checks run again? Unfortunately I cannot retrigger the playwright run, but the failures there might be related to your changes, could you please have a quick look? TIA! |
The settings editor now only shows preferences of the selected category. Make the page object search for a preference that is not visible before interacting with it, and match only visible preference rows so hidden 'Commonly Used' duplicates are ignored.
|
Hi @ndoschek, |
ndoschek
left a comment
ndoschek
left a comment
There was a problem hiding this comment.
Thanks for working on this! I really like the concept, and it also fits nicely with the AI Configuration rework that's currently under development. I tried the branch though, and the interaction still needs some work: the search state and the tree selection get out of sync, and clearing a search leaves you in a confusing state. A few concrete problems I noticed while testing:
-
Scrolling the search results auto-selects the first category
When you enter a search query or hen settings UI is opened with a query (e.g.ai-featuresorwindow), I first see a scrollable list of all search results, but as soon as I scroll, the first category gets selected on its own and only the settings of this category are shown. I'd expect to be able to scroll through all results freely and only narrow down once I explicitly select a category. -
The tree keeps a stale selection
If you have search results and select a category (collapsible node) and you then clear the search results, this category stays highlighted in the tree even though the detail page shows the first category.
Secondly it is worse for nested child nodes: If I search
ai-features, select the child categoryAgent Settings, then clear the search, the tree collapses butAgent Settingsis still the active selection (but this is not clear as the tree nodes are all collapsed and this selection is invisble), so I'm left looking at only that this category's details with no cue as to why.
I'd expect clearing the search to deselect the category, keep collapsing the tree and return to the first/default
Commonly Usedpage.
So basically always match the selection to what the detail page is showing. Since this is a changed behaviour of the settings ui it is important that we pay attention to keep the UX intuitive at last. -
The selection is stored across application restarts
I am not entirely sure about the logic that saves the last selected category on application close/restart. I tested a few scenarios and i end up with always having the AI Features pre-selected on a fresh start or a restart, although I had clearly selected other categories before closing/restarting.
I would be fine to not store it and starting fresh, but no strong opinion here. This could also be a follow up I think. -
The sticky headers for long pages
E.g. for the extensions category this does not work properly. If we keep all sub headers as sticky headings we might end up in a state that you cannot see any settings anymore as you have way too many headings piling up, see screenshot below. I would expect to only show the current sub headers you are currently in in as sticky header.
I haven't done a code review yet, but I think the feature needs some rework first. Could you take another look at the implementation and give it a bit of manual testing too, in case I overlooked a case, TIA!
github-project-automation
Bot
moved this from Waiting on reviewers
to Waiting on author
in PR Backlog
Address review feedback: - scrolling (incl. search results) no longer applies the category filter - starting a search clears the selection; clearing it returns to Commonly Used - startup deterministically selects Commonly Used instead of restoring the stored scroll target, and the selection now survives schema rebuilds - only ancestor headers and the current subsection header are pinned
|
All four points are addressed in 4432ec2:
Added unit tests for the selection/filter interaction in |
|
Hi @ndoschek |
|
Yes we are aware, it will be up soon again, I will retrigger it then. |
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
What this PR does
Brings the Settings UI closer to VS Code: selecting a category in the left tree narrows the editor to that category and its descendants, with ancestor headers pinned via position: sticky.
The approach is a split one. PreferenceTreeModel owns a new _categoryFilterId (derived from the tree selection, cleared on search) plus selection lifecycle logic (default "Commonly Used" selection, restoring the selection after schema rebuilds, selectPreservingCategoryFilter so that editor scroll sync never narrows the view). The actual hiding happens in PreferencesEditorWidget.hideIfFailsFilters via a new isOutsideSelectedCategory predicate, so the left tree stays complete. A new pinCategoryHeaders sets inline top offsets on header rows. The playwright page object was adapted to search for preferences that the filter hides.
Build and lint are clean, and the 15 new unit tests in preference-tree-model.spec.ts pass.
Critical points
I ran the branch in the browser. The sticky headers work well now (no pile-up, deep categories such as Extensions and Text Editor > Suggestions behave, content is not hidden under the stack), so review point 4 from the earlier round looks resolved.
The tree/editor desync from review point 2 is still reachable, though through a different path: getScrollTarget treats Category like Search, so a still-focused setting control pulls the tree selection back to the previous category while the editor shows the new one. I reproduced it twice, both when clicking a different category and when clearing a search. Details in the inline comment on getScrollTarget. That is the one I would fix before merging.
Beyond that, the main things worth a look are the duplicated ancestor check (core already has CompositeTreeNode.isAncestor), the loose startsWith match for the "Commonly Used" node, the forced reflow per header in pinCategoryHeaders, and the now dead PreferencesEditorState.
| * If the preference is not visible, search for its id, which clears the category filter | ||
| * and reveals the preference. | ||
| */ | ||
| protected async ensurePreferenceVisible(preferenceId: string): Promise<void> { |
There was a problem hiding this comment.
This fills the search box and never clears it, so every later call on the page object runs against a filtered editor. findPreferenceId walks section titles and following siblings, which breaks as soon as an unrelated search is active. Clearing the search box after the interaction would keep the helper free of side effects.
There was a problem hiding this comment.
Leaving the search term in place is intentional. Hidden rows keep the hidden class and stay in the DOM, so the xpath in findPreferenceId still finds them while a search is active. Clearing the search right after the interaction would hide the preference again (the editor returns to Commonly Used), and the :visible waits in waitForModified/waitForUnmodified that follow would then time out.
- A category change no longer scrolls back to the last focused control, which re-selected the previous category in the tree. - Fall back to Commonly Used (or clear the selection) when the selected category has no settings in the current scope or after a schema change, instead of showing an empty page. - Look up the Commonly Used node by its exact id. - Use CompositeTreeNode.isAncestor for the category filter and derive the sticky header stack from the model; measure heights before writing styles. - Drop the unused editor widget state. - Playwright: wait for the modification indicator without the :visible filter, so a hidden row cannot satisfy the detached wait.
The first visible row was matched against the scroll top, where the pinned header of the outermost ancestor always sits, so a scope or schema change re-selected the parent category in the tree while the editor kept showing the selected sub-category. Look for the first row below the pinned header stack instead, and place scroll targets below the stack. Adds a Playwright regression test with page object helpers to select a category and read the selected one.
|
Addressed the review comments in 0d9c56d and e7c5ff8 (merged master first):
New unit tests cover the fallback ( |
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up review after commits 0d9c56ddd and e7c5ff8f4.
All points from my last review are addressed: the ancestor-check duplication now uses CompositeTreeNode.isAncestor, "Commonly Used" is looked up by exact node id, the em dashes are gone, pinCategoryHeaders measures heights before writing styles, the sticky header stack is derived from the model's ancestor chain instead of iteration order, a category with no visible settings in the current scope now falls back to Commonly Used (or clears the selection) instead of rendering blank, the dead PreferencesEditorState/StatefulWidget code is removed, and getScrollTarget no longer scrolls back to a focused control on a category change (the exact tree/editor desync I had reported).
e7c5ff8f4 fixes a further desync the author found themselves: switching scope while a nested sub-category (e.g. Text Editor > Files) was selected used to bump the tree selection up to the parent category while the editor kept showing the sub-category. The fix (excluding the pinned ancestor-header stack from the "first visible row" calculation, and offsetting scroll targets by the stack height) looks correct on read, comes with its own unit and Playwright regression tests, and I verified it live in the browser (User to Workspace scope switch with "Files" selected under "Text Editor" keeps "Files" selected, sticky headers render correctly, no console errors).
I didn't find any new issues in the changes since my last review, so no new inline comments this round. The one open thread from my previous review (search box not cleared in theia-preference-view.ts) was thumbs-downed by the author with a reasonable explanation, so I'm leaving it as is.
|
Thanks a lot for all the updates @safisa! One small thing I noticed: with a category like I think the fallback moves the editor's category but the tree highlight doesn't follow along, so they end up pointing at different nodes. It's minor (it sorts itself out as soon as you click anything), but it's the same kind of desync we were chasing earlier, so probably worth having a final look. |
The row remembered as the top of the editor is replayed into the tree on scope and schema changes even when that row is no longer shown, e.g. after the selection fell back to Commonly Used. Only sync the tree when the row is still visible. Adds a Playwright regression test for the reported scenario.
|
Good catch, fixed in ab9a6cc: the row remembered as the top of the editor was still synced into the tree on the scope change although it was no longer shown. The tree now only follows rows that are still visible, and a Playwright test covers the Security > Workspace round trip. |
3 participants
What it does
Closes #17268.
Brings the Settings UI in line with VSCode: selecting a category in the left tree narrows the editor on the right to that category's content, with stacked ancestor headers pinned at the top.
position: stickyso only the items list scrolls beneath them.How to test
Follow-ups
Breaking changes
Attribution
Review checklist
Reminder for reviewers