Desktop: Resolves #14763: add settings search to config screen (new spec) - #14820
Merged
laurent22 merged 29 commits intoApr 30, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a settings search feature to the desktop Config screen: sidebar search input, search result grouping/filtering, highlighted matches, shared search utilities, SCSS styles, type tightening for settings, and related tests and integration tweaks. Changes
Sequence DiagramssequenceDiagram
actor User
participant Sidebar as Sidebar
participant ConfigScreen as ConfigScreen
participant SearchEngine as Search Engine
participant Renderer as Renderer
User->>Sidebar: Type query / click search controls
Sidebar->>ConfigScreen: onSearchQueryChange(query)
ConfigScreen->>SearchEngine: searchResultGroups(query, sections, appType)
SearchEngine-->>ConfigScreen: SearchResultGroup[]
ConfigScreen->>ConfigScreen: Compute matchedSections & searchSectionFilter
ConfigScreen->>Renderer: Render search results panel (with renderSearchText)
Renderer->>Renderer: Highlight labels/descriptions via highlightSearchText
Renderer-->>User: Display highlighted, optionally filtered results
sequenceDiagram
participant ConfigScreen
participant SettingComponent
participant SettingLabel
participant SettingDescription
ConfigScreen->>SettingComponent: pass renderSearchText callback
SettingComponent->>SettingLabel: render label via renderSearchText
SettingComponent->>SettingDescription: render description via renderSearchText
SettingLabel->>SettingLabel: return highlighted JSX
SettingDescription->>SettingDescription: return highlighted JSX
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/lib/components/shared/config/config-shared.ts (1)
69-74: Replaceanytypes with a properly-typedSettingsMapalias to comply with TypeScript guidelines.The
settings: anyparameter appears in both theSearchResultGroupsStateinterface and thematchedSearchSectionsfunction signature. Whilst the eslint-disable comments reference legacy code, this is an opportunity to introduce proper typing where the code is being actively modified.♻️ Suggested typing cleanup
+type SettingsMap = Record<string, unknown>; + interface SearchResultGroupsState { device: AppType; - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- Old code before rule was applied - settings: any; + settings: SettingsMap; query: string; } @@ export const matchedSearchSections = ( device: AppType, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- Old code before rule was applied - settings: any, + settings: SettingsMap, groups: SearchResultGroup[], ): MatchedSearchSection[] => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/components/shared/config/config-shared.ts` around lines 69 - 74, Replace the loose any usage by introducing a SettingsMap alias and applying it to the SearchResultGroupsState.settings property and the matchedSearchSections function parameter: define SettingsMap (e.g., a record keyed by string or AppType-specific keys to the proper settings shape) and change the type of settings in the SearchResultGroupsState interface and the matchedSearchSections signature from any to SettingsMap so callers and consumers (SearchResultGroupsState, matchedSearchSections) get proper typing and avoid the eslint-disable comment.packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
524-528: Avoid duplicating highlight style construction.
markStyleduplicates the style logic already defined inrenderSearchHighlightedText, which risks drift. Reuse the helper for section-title highlighting (or add the required duplication note if you intentionally keep both blocks).♻️ Suggested simplification
- const markStyle: React.CSSProperties = { - backgroundColor: theme.searchMarkerBackgroundColor, - color: theme.searchMarkerColor, - padding: 0, - }; - const searchContent = filteredMatchedSections.map(({ section }) => { const sectionComp = section.isScreen ? ( <div style={{ ...theme.textStyle, marginBottom: 20 }}> {_('This section opens in its own screen and is matched by section title.')} </div> ) : this.sectionToComponent(section.name, section, settings, true); @@ - {highlightSearchText(Setting.sectionNameToLabel(section.name), this.state.searchQuery, markStyle)} + {this.renderSearchHighlightedText(Setting.sectionNameToLabel(section.name))} </h2> {sectionComp} </div> ); });As per coding guidelines, "When duplicating a substantial block of code, add a comment above it noting the duplication and referencing the original location".
Also applies to: 547-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx` around lines 524 - 528, The markStyle definition duplicates the highlight styling in renderSearchHighlightedText; either reuse that helper there or add an explicit duplication comment pointing to renderSearchHighlightedText to prevent drift—modify the section-title rendering to call or derive styles from renderSearchHighlightedText (or add a comment above the markStyle block referencing renderSearchHighlightedText as the authoritative source) so both places share the same style logic and stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 189-193: When searchMode is true the click handler only sets
selectedSectionName and searchSectionFilter, leaving screenName stale and
causing wrong content after exiting search; update the click path (where
selectedSectionName/searchSectionFilter are set) to also call
switchSection(sectionName) or explicitly set screenName to sectionName so screen
state stays in sync (ensure you reuse the existing switchSection(...) method
rather than duplicating logic and keep selectedSectionName, searchSectionFilter,
and screenName consistent).
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 115-116: The component currently determines search mode using the
raw props.searchQuery (e.g., isSearching = !!props.searchQuery), which treats
whitespace-only input as an active search; update all such checks (including the
occurrences around the isSearching computation and the other spots noted) to
trim the query before evaluating — e.g., use props.searchQuery?.trim().length >
0 (or equivalent) when computing isSearching and any conditional that toggles
the clear/search UI so that whitespace-only strings do not activate search mode.
- Around line 166-183: The component currently sets aria-selected and tabIndex
from selected even when isDisabled is true; compute an effectiveSelected (e.g.
const effectiveSelected = selected && !isDisabled) and use that for
aria-selected and tabIndex (aria-selected={effectiveSelected} and
tabIndex={effectiveSelected ? 0 : -1}) so disabled sections are not
keyboard-focusable or marked selected; update any usage of selected for these
attributes on StyledListItem (and keep href/aria-disabled logic unchanged).
- Around line 227-239: The search input (SearchInput inside
StyledSearchContainer) is currently a child of the element with role='tablist'
(StyledRoot) which violates ARIA tabs semantics; move the entire
StyledSearchContainer out of the StyledRoot so that StyledRoot contains only the
tab elements (buttons) and the SearchInput becomes a sibling element (before or
after StyledRoot) while preserving existing styling and props
(props.searchQuery, props.onSearchQueryChange, props.onSearchButtonClick, etc.);
update the JSX around StyledRoot and StyledSearchContainer to remove SearchInput
from inside StyledRoot and ensure no role or tabindex changes are inadvertently
applied to the SearchInput or its container.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 524-528: The markStyle definition duplicates the highlight styling
in renderSearchHighlightedText; either reuse that helper there or add an
explicit duplication comment pointing to renderSearchHighlightedText to prevent
drift—modify the section-title rendering to call or derive styles from
renderSearchHighlightedText (or add a comment above the markStyle block
referencing renderSearchHighlightedText as the authoritative source) so both
places share the same style logic and stay in sync.
In `@packages/lib/components/shared/config/config-shared.ts`:
- Around line 69-74: Replace the loose any usage by introducing a SettingsMap
alias and applying it to the SearchResultGroupsState.settings property and the
matchedSearchSections function parameter: define SettingsMap (e.g., a record
keyed by string or AppType-specific keys to the proper settings shape) and
change the type of settings in the SearchResultGroupsState interface and the
matchedSearchSections signature from any to SettingsMap so callers and consumers
(SearchResultGroupsState, matchedSearchSections) get proper typing and avoid the
eslint-disable comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 141694da-6399-4863-b6bb-2d4a77d1474b
📒 Files selected for processing (9)
.eslintignore.gitignorepackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingDescription.tsxpackages/app-desktop/gui/ConfigScreen/controls/SettingLabel.tsxpackages/app-desktop/gui/ConfigScreen/searchHighlight.tsxpackages/lib/components/shared/config/config-shared.ts
personalizedrefrigerator
left a comment
personalizedrefrigerator
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for working on this! I've left a few comments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (3)
166-186:⚠️ Potential issue | 🟠 MajorDisabled sections should not remain selected or focusable.
At Line 180 and Line 182, disabled tabs can still be marked selected and keep
tabIndex={0}, which conflicts with “disabled for keyboard interaction”.♿ Suggested accessibility fix
const selected = props.selection === section.name; const isSearching = !!props.searchQuery; const hasMatch = matchedSectionNames.has(section.name); const isDisabled = isSearching && !hasMatch; + const isActiveTab = selected && !isDisabled; @@ - aria-selected={selected} + aria-selected={isActiveTab} aria-disabled={isDisabled} - tabIndex={selected ? 0 : -1} + tabIndex={isActiveTab ? 0 : -1} @@ - selected={selected} + selected={isActiveTab}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx` around lines 166 - 186, The tab for a disabled section (isDisabled) must not appear selected or be keyboard-focusable: update the return props on StyledListItem (used in Sidebar.tsx) so that when isDisabled is true you force selected to false (override the computed selected), set tabIndex to -1, and ensure aria-selected is false; keep aria-disabled true. Adjust the values derived from selected and isDisabled (the selected variable and tabIndex assignment near Setting.isSubSection and buttonRefs usage) so disabled tabs cannot be focused or announced as selected.
227-239:⚠️ Potential issue | 🟠 MajorKeep
tablistownership semantic: move search input outside the tablist.
SearchInputis currently rendered as a child of therole='tablist'container. This should be a sibling container so the tablist owns tabs only.According to the WAI-ARIA Tabs Pattern, should an element with `role="tablist"` contain only `role="tab"` owned elements, or can it include a text input used for search?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx` around lines 227 - 239, The SearchInput is currently placed inside the element using role='tablist' (StyledRoot), violating the ARIA tabs pattern; move the StyledSearchContainer (and its SearchInput) out of StyledRoot so the tablist contains only tab elements (the buttons variable). Update the JSX so StyledSearchContainer is rendered as a sibling to StyledRoot (e.g., place it immediately before StyledRoot) and leave StyledRoot with role='tablist' containing only the tabs/buttons; keep props and handlers (props.searchQuery, props.onSearchQueryChange, props.onSearchButtonClick) unchanged.
115-116:⚠️ Potential issue | 🟠 MajorUse trimmed query when toggling search mode.
Line 115, Line 167, and Line 234 treat whitespace-only input as active search, which can disable sections and show “search started” state incorrectly.
🧭 Suggested consistency fix
export default function Sidebar(props: Props) { const buttonRefs = useRef<HTMLElement[]>([]); + const isSearching = props.searchQuery.trim().length > 0; @@ - const isSearching = !!props.searchQuery; @@ - const isSearching = !!props.searchQuery; @@ - searchStarted={!!props.searchQuery} + searchStarted={isSearching}Also applies to: 167-170, 229-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx` around lines 115 - 116, The current checks use props.searchQuery directly which treats whitespace-only input as an active search; compute a trimmed query once (e.g., const trimmedQuery = (props.searchQuery || "").trim()) and replace uses of props.searchQuery when determining isSearching and any toggles/conditional UI (replace isSearching = !!props.searchQuery with isSearching = trimmedQuery.length > 0 and use trimmedQuery for the "search started" state and section-disable logic) so whitespace-only queries are treated as empty.packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
189-197:⚠️ Potential issue | 🟠 MajorKeep
screenNamein sync when selecting sections during search mode.In the search-mode branch, only
selectedSectionName/searchSectionFilterare updated. If the selected section is screen-backed, clearing search can leavescreenNamestale.🛠️ Suggested state-sync fix
if (searchMode) { + const section = this.sectionByName(sectionName); this.setState({ selectedSectionName: sectionName, + screenName: section.isScreen ? section.name : '', searchSectionFilter: sectionName, }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx` around lines 189 - 197, When handling a section selection while searchMode is true, also keep screenName in sync: inside the search-mode branch where you call this.setState({ selectedSectionName, searchSectionFilter }), include logic to set screenName to sectionName if the chosen section is screen-backed (otherwise clear it, e.g. ""), so that later clearing the search doesn't leave a stale screenName; use the same detection used elsewhere (the same check used by switchSection or the section metadata) and update screenName in the same setState call.
🧹 Nitpick comments (2)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
378-383: Use theme-defined highlight colours instead of hard-coded RGB values.Both highlight style blocks hard-code colour values, which makes theme consistency harder to maintain.
As per coding guidelines: "For GUI style changes in Desktop and mobile clients, refer to
packages/lib/theme.tsfor all styling information".Also applies to: 526-530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx` around lines 378 - 383, The highlight styles in renderSearchHighlightedText (which calls highlightSearchText with hard-coded RGB strings) must be replaced with theme-defined colour tokens from packages/lib/theme.ts; import the appropriate exports from that module and pass them instead of the literal 'rgb(...)' values (also update the similar hard-coded block used elsewhere where highlightSearchText is called), ensuring you use the exported theme variable names (e.g., the module's highlight background/foreground constants) so the component uses theme-defined colours tied to state.searchQuery and highlightSearchText.packages/app-desktop/gui/ConfigScreen/Sidebar.test.js (1)
70-113: Consolidate repeated search test setup with helpers ortest.each.There’s repeated
searchResultsconstruction and render setup across multiple tests. This is a good candidate for a shared helper or parameterised cases.Based on learnings: "Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Avoid duplicating code in tests; use
test.eachor shared helpers instead of repeating similar test blocks".Also applies to: 153-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.test.js` around lines 70 - 113, Extract the repeated search-setup into a small helper and/or use test.each to parameterize cases: create a helper (e.g., renderWithSearch) that accepts {searchQuery, searchResultGroups, onSelectionChange} and calls render(React.createElement(Sidebar.default, {...defaultProps, ...opts})), then replace duplicated setups in tests "should disable sections without search matches", "should prevent clicking disabled sections", "should allow clicking enabled sections during search", and "should restore enabled state when search is cleared" to call renderWithSearch or a test.each table that passes different searchResultGroups and expectations; reference the existing symbols Sidebar.default, defaultProps, searchResultGroups, searchQuery, onSelectionChange and update the tests to use the helper or parameterized inputs to remove repeated construction and render calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.test.js`:
- Around line 115-126: The test "should skip disabled sections during keyboard
navigation" currently only checks that onSelectionChange was called; change it
to assert the actual target selection to ensure it navigated to the Appearance
tab. In the test in Sidebar.test.js, after firing ArrowDown on the generalTab,
assert that the mock onSelectionChange received the selection for 'appearance'
(or that the component selection prop/state equals 'appearance') rather than
only using toHaveBeenCalled(); reference the onSelectionChange mock and the
Sidebar component used in the render call to locate where to change the
expectation.
---
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 189-197: When handling a section selection while searchMode is
true, also keep screenName in sync: inside the search-mode branch where you call
this.setState({ selectedSectionName, searchSectionFilter }), include logic to
set screenName to sectionName if the chosen section is screen-backed (otherwise
clear it, e.g. ""), so that later clearing the search doesn't leave a stale
screenName; use the same detection used elsewhere (the same check used by
switchSection or the section metadata) and update screenName in the same
setState call.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 166-186: The tab for a disabled section (isDisabled) must not
appear selected or be keyboard-focusable: update the return props on
StyledListItem (used in Sidebar.tsx) so that when isDisabled is true you force
selected to false (override the computed selected), set tabIndex to -1, and
ensure aria-selected is false; keep aria-disabled true. Adjust the values
derived from selected and isDisabled (the selected variable and tabIndex
assignment near Setting.isSubSection and buttonRefs usage) so disabled tabs
cannot be focused or announced as selected.
- Around line 227-239: The SearchInput is currently placed inside the element
using role='tablist' (StyledRoot), violating the ARIA tabs pattern; move the
StyledSearchContainer (and its SearchInput) out of StyledRoot so the tablist
contains only tab elements (the buttons variable). Update the JSX so
StyledSearchContainer is rendered as a sibling to StyledRoot (e.g., place it
immediately before StyledRoot) and leave StyledRoot with role='tablist'
containing only the tabs/buttons; keep props and handlers (props.searchQuery,
props.onSearchQueryChange, props.onSearchButtonClick) unchanged.
- Around line 115-116: The current checks use props.searchQuery directly which
treats whitespace-only input as an active search; compute a trimmed query once
(e.g., const trimmedQuery = (props.searchQuery || "").trim()) and replace uses
of props.searchQuery when determining isSearching and any toggles/conditional UI
(replace isSearching = !!props.searchQuery with isSearching =
trimmedQuery.length > 0 and use trimmedQuery for the "search started" state and
section-disable logic) so whitespace-only queries are treated as empty.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 378-383: The highlight styles in renderSearchHighlightedText
(which calls highlightSearchText with hard-coded RGB strings) must be replaced
with theme-defined colour tokens from packages/lib/theme.ts; import the
appropriate exports from that module and pass them instead of the literal
'rgb(...)' values (also update the similar hard-coded block used elsewhere where
highlightSearchText is called), ensuring you use the exported theme variable
names (e.g., the module's highlight background/foreground constants) so the
component uses theme-defined colours tied to state.searchQuery and
highlightSearchText.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.test.js`:
- Around line 70-113: Extract the repeated search-setup into a small helper
and/or use test.each to parameterize cases: create a helper (e.g.,
renderWithSearch) that accepts {searchQuery, searchResultGroups,
onSelectionChange} and calls render(React.createElement(Sidebar.default,
{...defaultProps, ...opts})), then replace duplicated setups in tests "should
disable sections without search matches", "should prevent clicking disabled
sections", "should allow clicking enabled sections during search", and "should
restore enabled state when search is cleared" to call renderWithSearch or a
test.each table that passes different searchResultGroups and expectations;
reference the existing symbols Sidebar.default, defaultProps,
searchResultGroups, searchQuery, onSelectionChange and update the tests to use
the helper or parameterized inputs to remove repeated construction and render
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6762ee5-841d-45f2-a44f-3318fcf03c25
📒 Files selected for processing (8)
.eslintignore.gitignorepackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.test.jspackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-desktop/gui/ConfigScreen/searchHighlight.integration.test.tsxpackages/app-desktop/gui/ConfigScreen/searchHighlight.test.tsxpackages/lib/components/shared/config/config-search.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/lib/components/shared/config/config-search.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- .eslintignore
Contributor
Author
Hi personalizedrefrigerator, I’ll update the implementation accordingly later. thanks for the detailed review! |
Contributor
Author
|
I want to confirm the scope for search indexing in screen-based sections (Encryption / Web Clipper / Keyboard Shortcuts). In the current implementation, these sections can be matched by section title, but their internal screen controls are not fully indexed through the same metadata path as regular settings. Should we treat this as acceptable for this issue, or should we also index screen-internal fields so they can be matched by query? |
Collaborator
For full-screen sections that lack searchable metadata: Unless indexing screen-internal text can be done without significantly increasing the size and complexity of this pull request, I would suggest only searching the section header for now. |
laurent22
reviewed
Mar 19, 2026
laurent22
left a comment
laurent22
left a comment
Owner
There was a problem hiding this comment.
Thanks for the implementation. Please address our comments, and also CodeRabbit's comments
Collaborator
|
I've added a new test that should help auto-detect certain accessibility issues. I'm merging upstream changes into this pull request so that the test runs in CI. |
Collaborator
The accessibility checker has flagged a few potential issues:
Details+ Array [
+ Object {
+ "description": "Ensure elements with an ARIA role that require child roles contain them",
+ "help": "Certain ARIA roles must contain particular children",
+ "helpUrl": "https://dequeuniversity.com/rules/axe/4.11/aria-required-children?application=playwright",
+ "id": "aria-required-children",
+ "impact": "critical",
+ "nodes": Array [
+ Object {
+ "all": Array [],
+ "any": Array [
+ Object {
+ "data": Object {
+ "messageKey": "unallowed",
+ "values": "input[tabindex], button[aria-label]",
+ },
+ "id": "aria-required-children",
+ "impact": "critical",
+ "message": "Element has children which are not allowed: input[tabindex], button[aria-label]",
+ "relatedNodes": Array [
+ Object {
+ "html": "<input placeholder=\"Search settings...\" spellcheck=\"false\" class=\"sc-jsJBEP sc-iHGNWf cGZLNz dPRrXf\" type=\"search\" value=\"\">",
+ "target": Array [
+ "input[placeholder=\"Search settings...\"]",
+ ],
+ },
+ Object {
+ "html": "<button aria-label=\"Search\" class=\"sc-koXPp jxDRVO\"><span class=\"sc-bmzYkS hQFofX icon-search\"></span></button>",
+ "target": Array [
+ ".sc-fXSgeo > .sc-eeDRCY.kILkiy > .sc-koXPp.jxDRVO[aria-label=\"Search\"]",
+ ],
+ },
+ ],
+ },
+ ],
+ "failureSummary": "Fix any of the following:
+ Element has children which are not allowed: input[tabindex], button[aria-label]",
+ "html": "<div class=\"sc-esYiGF ffDzkK settings-sidebar _scrollbar2\" role=\"tablist\">",
+ "impact": "critical",
+ "none": Array [],
+ "target": Array [
+ ".sc-esYiGF",
+ ],
+ },
+ ],
+ "tags": Array [
+ "cat.aria",
+ "wcag2a",
+ "wcag131",
+ "EN-301-549",
+ "EN-9.1.3.1",
+ "RGAAv4",
+ "RGAA-9.3.1",
+ ],
+ },
+ Object {
+ "description": "Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds",
+ "help": "Elements must meet minimum color contrast ratio thresholds",
+ "helpUrl": "https://dequeuniversity.com/rules/axe/4.11/color-contrast?application=playwright",
+ "id": "color-contrast",
+ "impact": "serious",
+ "nodes": Array [
+ Object {
+ "all": Array [],
+ "any": Array [
+ Object {
+ "data": Object {
+ "bgColor": "#989ba0",
+ "colorParse": undefined,
+ "contrastRatio": 4.29,
+ "expectedContrastRatio": "4.5:1",
+ "fgColor": "#32373f",
+ "fontSize": "9.0pt (12px)",
+ "fontWeight": "normal",
+ "messageKey": null,
+ "shadowColor": undefined,
+ },
+ "id": "color-contrast",
+ "impact": "serious",
+ "message": "Element has insufficient color contrast of 4.29 (foreground color: #32373f, background color: #989ba0, font size: 9.0pt (12px), font weight: normal). Expected contrast ratio of 4.5:1",
+ "relatedNodes": Array [
+ Object {
+ "html": "<input placeholder=\"Search settings...\" spellcheck=\"false\" class=\"sc-jsJBEP sc-iHGNWf cGZLNz dPRrXf\" type=\"search\" value=\"\">",
+ "target": Array [
+ "input[placeholder=\"Search settings...\"]",
+ ],
+ },
+ Object {
+ "html": "<div class=\"sc-esYiGF ffDzkK settings-sidebar _scrollbar2\" role=\"tablist\">",
+ "target": Array [
+ ".sc-esYiGF",
+ ],
+ },
+ ],
+ },
+ ],
+ "failureSummary": "Fix any of the following:
+ Element has insufficient color contrast of 4.29 (foreground color: #32373f, background color: #989ba0, font size: 9.0pt (12px), font weight: normal). Expected contrast ratio of 4.5:1",
+ "html": "<input placeholder=\"Search settings...\" spellcheck=\"false\" class=\"sc-jsJBEP sc-iHGNWf cGZLNz dPRrXf\" type=\"search\" value=\"\">",
+ "impact": "serious",
+ "none": Array [],
+ "target": Array [
+ "input[placeholder=\"Search settings...\"]",
+ ],
+ },
+ Object {
+ "all": Array [],
+ "any": Array [
+ Object {
+ "data": Object {
+ "bgColor": "#26292f",
+ "colorParse": undefined,
+ "contrastRatio": 3.78,
+ "expectedContrastRatio": "4.5:1",
+ "fgColor": "#7f8289",
+ "fontSize": "9.0pt (12px)",
+ "fontWeight": "normal",
+ "messageKey": null,
+ "shadowColor": undefined,
+ },
+ "id": "color-contrast",
+ "impact": "serious",
+ "message": "Element has insufficient color contrast of 3.78 (foreground color: #7f8289, background color: #26292f, font size: 9.0pt (12px), font weight: normal). Expected contrast ratio of 4.5:1",
+ "relatedNodes": Array [
+ Object {
+ "html": "<div class=\"sc-fjvvzt jcdFkQ\">Plugins</div>",
+ "target": Array [
+ ".sc-fjvvzt",
+ ],
+ },
+ Object {
+ "html": "<div class=\"sc-esYiGF ffDzkK settings-sidebar _scrollbar2\" role=\"tablist\">",
+ "target": Array [
+ ".sc-esYiGF",
+ ],
+ },
+ ],
+ },
+ ],
+ "failureSummary": "Fix any of the following:
+ Element has insufficient color contrast of 3.78 (foreground color: #7f8289, background color: #26292f, font size: 9.0pt (12px), font weight: normal). Expected contrast ratio of 4.5:1",
+ "html": "<div class=\"sc-fjvvzt jcdFkQ\">Plugins</div>",
+ "impact": "serious",
+ "none": Array [],
+ "target": Array [
+ ".sc-fjvvzt",
+ ],
+ },
+ ],
+ "tags": Array [
+ "cat.color",
+ "wcag2aa",
+ "wcag143",
+ "TTv5",
+ "TT13.c",
+ "EN-301-549",
+ "EN-9.1.4.3",
+ "ACT",
+ "RGAAv4",
+ "RGAA-3.2.1",
+ ],
+ },
+ ] |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (2)
221-231:⚠️ Potential issue | 🟠 MajorKeep the search box outside the
tablist.
StyledRootis exposed asrole='tablist', so nestingSearchInputunder it breaks the tablist ownership rules and matches the ARIA violation already reported for this PR. The search container needs to be a sibling wrapper, not a child of the tablist.♿ One way to restructure this
+export const StyledSidebar = styled.div` + display: flex; + flex-direction: column; + background-color: ${(props: StyleProps) => props.theme.backgroundColor2}; +`; + export default function Sidebar(props: Props) { ... return ( - <StyledRoot className='settings-sidebar _scrollbar2' role='tablist'> - <StyledSearchContainer> - <SearchInput - inputRef={null} - value={props.searchQuery} - onChange={props.onSearchQueryChange} - onSearchButtonClick={props.onSearchButtonClick} - searchStarted={isSearching} - placeholder={_('Search settings...')} - /> - </StyledSearchContainer> - {buttons} - </StyledRoot> + <StyledSidebar className='settings-sidebar'> + <StyledSearchContainer> + <SearchInput + inputRef={null} + value={props.searchQuery} + onChange={props.onSearchQueryChange} + onSearchButtonClick={props.onSearchButtonClick} + searchStarted={isSearching} + placeholder={_('Search settings...')} + /> + </StyledSearchContainer> + <StyledRoot className='_scrollbar2' role='tablist'> + {buttons} + </StyledRoot> + </StyledSidebar> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx` around lines 221 - 231, The search input is currently nested inside StyledRoot which is rendered with role='tablist', violating ARIA tablist ownership; move the StyledSearchContainer (the wrapper for SearchInput) out of the StyledRoot so it becomes a sibling wrapper before or after StyledRoot rather than a child, ensuring StyledRoot only contains tab elements. Update the JSX that renders StyledRoot, StyledSearchContainer and SearchInput so that SearchInput is not a descendant of the element with role='tablist' (reference: StyledRoot, StyledSearchContainer, SearchInput, role='tablist').
161-185:⚠️ Potential issue | 🟠 MajorDon’t leave a disabled section in the roving tab stop.
If the active section stops matching, this still renders it as selected with
tabIndex={0}. That keeps a greyed-out tab focusable and conflicts with the new “disabled for keyboard interaction” behaviour. Please derive aneffectiveSelectedvalue fromselected && !isDisabledand use that for the selected state, tab index, and selected styling.♿ Suggested guard
function renderButton(section: SettingMetadataSection, index: number) { const selected = props.selection === section.name; const hasMatch = matchedSectionNames.has(section.name); const isDisabled = isSearching && !hasMatch; + const effectiveSelected = selected && !isDisabled; return ( <StyledListItem key={section.name} href={isDisabled ? undefined : '#'} role='tab' ref={(item: HTMLElement) => { buttonRefs.current[index] = item; }} id={`setting-tab-${section.name}`} aria-controls={`setting-section-${section.name}`} - aria-selected={selected} + aria-selected={effectiveSelected} aria-disabled={isDisabled} - tabIndex={selected ? 0 : -1} + tabIndex={effectiveSelected ? 0 : -1} isSubSection={Setting.isSubSection(section.name)} - selected={selected} + selected={effectiveSelected} disabled={isDisabled} onClick={() => { if (isDisabled) return; props.onSelectionChange({ section: section }); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx` around lines 161 - 185, The current logic keeps a disabled section focusable because selected is set from props.selection even when isDisabled is true; derive an effectiveSelected = selected && !isDisabled and use that instead of selected for the accessible/interaction state: replace usages of selected in aria-selected, tabIndex, the StyledListItem selected prop, and any roving-tabstop/ref focus logic (e.g., where buttonRefs/current index or onKeyDown depend on selection) so a section that is disabled (isSearching && !matchedSectionNames.has(section.name)) is not treated as focusable/selected; keep props.selection and isDisabled calculations as-is but switch UI/ARIA consumers to effectiveSelected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Line 72: The "Plugins" divider in Sidebar.tsx is being dimmed by applying
opacity: 0.38 to its container, which reduces text contrast; remove that opacity
on the divider/container (the element rendering the "Plugins" heading in the
Sidebar component) and instead apply a dedicated colour token for a visually
quieter state (e.g., use an existing neutral/disabled text token or add a new
token) to the divider/heading text so the element retains full opacity while
meeting contrast requirements.
---
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 221-231: The search input is currently nested inside StyledRoot
which is rendered with role='tablist', violating ARIA tablist ownership; move
the StyledSearchContainer (the wrapper for SearchInput) out of the StyledRoot so
it becomes a sibling wrapper before or after StyledRoot rather than a child,
ensuring StyledRoot only contains tab elements. Update the JSX that renders
StyledRoot, StyledSearchContainer and SearchInput so that SearchInput is not a
descendant of the element with role='tablist' (reference: StyledRoot,
StyledSearchContainer, SearchInput, role='tablist').
- Around line 161-185: The current logic keeps a disabled section focusable
because selected is set from props.selection even when isDisabled is true;
derive an effectiveSelected = selected && !isDisabled and use that instead of
selected for the accessible/interaction state: replace usages of selected in
aria-selected, tabIndex, the StyledListItem selected prop, and any
roving-tabstop/ref focus logic (e.g., where buttonRefs/current index or
onKeyDown depend on selection) so a section that is disabled (isSearching &&
!matchedSectionNames.has(section.name)) is not treated as focusable/selected;
keep props.selection and isDisabled calculations as-is but switch UI/ARIA
consumers to effectiveSelected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9829fc8e-4dd9-4669-9b32-84e8d349ac3c
📒 Files selected for processing (8)
.eslintignore.gitignorepackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-desktop/gui/ConfigScreen/searchHighlight.test.tsxpackages/app-desktop/gui/ConfigScreen/searchHighlight.tsxpackages/lib/components/shared/config/config-search.test.tspackages/lib/components/shared/config/config-shared.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/app-desktop/gui/ConfigScreen/searchHighlight.tsx
- packages/lib/components/shared/config/config-search.test.ts
- packages/app-desktop/gui/ConfigScreen/searchHighlight.test.tsx
- packages/lib/components/shared/config/config-shared.ts
- .eslintignore
- .gitignore
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
525-547: Screen-only sections display a placeholder message in search results.Consider whether linking to or automatically navigating to the screen section would provide a better UX, so users don't have to manually click the sidebar. This is optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx` around lines 525 - 547, Screen-only sections currently render a static placeholder when matched in search (handled in filteredMatchedSections mapping when section.isScreen); replace that placeholder with an interactive element (link/button) that navigates to or opens the screen section so users don't have to use the sidebar. Implement a small handler (e.g., openScreenSection(sectionName)) on the component and wire the interactive element's onClick to it; keep the existing icon/title rendering via Setting.sectionNameToIcon and renderSearchHighlightedText(Setting.sectionNameToLabel(section.name)), and fall back to the current placeholder only if navigation cannot be performed. Ensure the new handler uses your app's existing navigation/sidebar API to show the screen for the given section name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 525-547: Screen-only sections currently render a static
placeholder when matched in search (handled in filteredMatchedSections mapping
when section.isScreen); replace that placeholder with an interactive element
(link/button) that navigates to or opens the screen section so users don't have
to use the sidebar. Implement a small handler (e.g.,
openScreenSection(sectionName)) on the component and wire the interactive
element's onClick to it; keep the existing icon/title rendering via
Setting.sectionNameToIcon and
renderSearchHighlightedText(Setting.sectionNameToLabel(section.name)), and fall
back to the current placeholder only if navigation cannot be performed. Ensure
the new handler uses your app's existing navigation/sidebar API to show the
screen for the given section name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b5d1907-2e81-439f-a27c-487fd8f827b3
📒 Files selected for processing (2)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.tsx
Contributor
Author
Hi personalizedrefrigerator, thanks so much for the detailed accessibility feedback! I’ve applied the fixes by improving the search placeholder contrast for WCAG readability, removing opacity-based dimming on the Plugins divider to preserve active text contrast, and moving SearchInput outside the tablist while marking the Plugins divider as presentational so tablist semantics remain correct. |
coderabbitai
Bot
added
enhancement
Contributor
Author
|
By the way, I’ve also uploaded a new demo video showing the latest behavior. If there’s anything else you’d like me to adjust, please feel free to let me know |
coderabbitai
Bot
added
plugins
Owner
|
Claude gives this as a review. I didn't check all of it but I see at least 1 and 3 appear to be correct: Critical / Bugs1. Crash on initial render —
|
coderabbitai
Bot
added
enhancement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
516-523:⚠️ Potential issue | 🟠 MajorSearch filters are still exposed as tabs for one shared results region.
This mounts a single
role='region'for search results, but the sidebar still receives a selected section and renders enabled section buttons as tabs targeting that same shared panel. Either render search-mode sidebar entries as filter buttons, or mount real per-section tabpanels during search.Also applies to: 581-588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx` around lines 516 - 523, The search UI currently mounts a single shared results panel (the element with id 'setting-section-search-results' pushed into tabComponents) while the sidebar still renders section entries as tabs targeting that shared panel; update ConfigScreen.tsx so that in search mode the sidebar renders those entries as filter buttons or, alternatively, create distinct tabpanel elements per section instead of the single role='region' panel. Locate the code that constructs tabComponents (the push of the div with id 'setting-section-search-results') and the sidebar rendering logic that uses the selected section, then either switch the sidebar entries to non-tab controls when isSearchMode is true or change the tabComponents construction to produce separate panels for each enabled section so each sidebar tab targets its own tabpanel. Ensure ARIA roles are consistent: filter buttons should not be role='tab' and per-section panels must retain role='region' (or role='tabpanel') and unique ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 464-466: screenComp being truthy (from this.state.screenName and
screenFromName) hides the settings container even when the search UI is active
and replacing that screen; update the hide condition to also verify search is
not active/has no results. Replace the shouldHideSettingsContainer assignment
with a check that screenComp is truthy AND the search UI is closed (e.g.
!this.state.searchOpen) and there are no search results (e.g.
!this.state.searchResults || this.state.searchResults.length === 0), and make
the same change where the same logic appears (the other block around the lines
referenced) so editable settings in search results retain their save/apply
handlers.
---
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 516-523: The search UI currently mounts a single shared results
panel (the element with id 'setting-section-search-results' pushed into
tabComponents) while the sidebar still renders section entries as tabs targeting
that shared panel; update ConfigScreen.tsx so that in search mode the sidebar
renders those entries as filter buttons or, alternatively, create distinct
tabpanel elements per section instead of the single role='region' panel. Locate
the code that constructs tabComponents (the push of the div with id
'setting-section-search-results') and the sidebar rendering logic that uses the
selected section, then either switch the sidebar entries to non-tab controls
when isSearchMode is true or change the tabComponents construction to produce
separate panels for each enabled section so each sidebar tab targets its own
tabpanel. Ensure ARIA roles are consistent: filter buttons should not be
role='tab' and per-section panels must retain role='region' (or role='tabpanel')
and unique ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bd56e66-29d6-49fd-868c-e5921d74247b
📒 Files selected for processing (5)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-mobile/components/screens/ConfigScreen/ConfigScreen.tsxpackages/lib/components/shared/config/config-search-text.tspackages/lib/components/shared/config/config-search.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/lib/components/shared/config/config-search.test.ts
- packages/lib/components/shared/config/config-search-text.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-mobile/components/screens/ConfigScreen/ConfigScreen.tsx
Contributor
Author
|
Thanks for the review! I've pushed fixes for the confirmed bugs. Here's a rundown: Fixed: (1), (2), (3), (4), (5), (6), (9), (12), and the two missing test cases from (11). (7): The (10): The |
Owner
|
Thank you @slimuCS! We will make the feature part of next pre-release 3.7 |
This was referenced May 12, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #14763
Problem
Desktop settings search did not meet the updated 0318 behavior requirements. Users needed a filtering workflow (not only navigation) that can show all matches in the right panel, keep all sections visible in the sidebar, and clearly indicate matched vs non-matched states.
Solution
This PR implements the desktop settings search model in line with the detailed spec:
Test Plan
Automated
yarn workspace @joplin/app-desktop tsc --noEmit yarn linter-precommit yarn workspace @joplin/app-desktop test gui/ConfigScreen/searchHighlight.test.tsx gui/ConfigScreen/searchHighlight.integration.test.tsxCoverage focus in this PR:
Video
Demo_0407.mp4
AI Assistance Disclosure
Note
This PR supersedes #14781. The updated spec changes the interaction model enough that I restarted the implementation in a new branch to keep the diff and review history clearer.