Desktop: Resolves #14763: Add search support for settings screen#14769
Desktop: Resolves #14763: Add search support for settings screen#14769Kartikeya-guthub wants to merge 21 commits intolaurent22:devfrom
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
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 search feature to the desktop Config screen: new search state and handlers, search/filter/highlight utilities, sidebar search input with enable/disable section logic and keyboard navigation, search-aware rendering for settings/plugins, and a “No settings match your search.” state. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Config as ConfigScreen
participant Utils as SearchUtils
participant Sidebar as Sidebar
participant View as SettingsView
User->>Config: Type query / Click clear
Config->>Utils: Normalise query, split terms, compute matches
Utils-->>Config: filteredSections, matchingSectionNames
Config->>Sidebar: update searchQuery, searchStarted, provide isSectionMatch callback
Config->>View: compute sectionsToRender, pass searchQuery for highlights
alt no results
View-->>User: "No settings match your search."
else results
View-->>User: Render matching settings with highlighted matches
end
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 469-472: When filteredSections is empty the code computes
selectedSectionName = '' and passes an empty filteredSections into Sidebar,
which later allows keyboard events to emit an undefined section and crash in
sidebar_selectionChange when accessing event.section.name; fix by guarding
empty-state in ConfigScreen: ensure selectedSectionName is set only when
filteredSections.length > 0 (e.g., keep previous selection only if it exists,
otherwise set selectedSectionName to null/undefined or to the first
filteredSections[0].name), compute selectedSection from that guarded value
(selectedSection), and prevent rendering or calling Sidebar (or pass a safe
non-empty sections array) when filteredSections is empty so
sidebar_selectionChange cannot receive an undefined event.section; update any
code that assumes selectedSectionName is a string accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 539c7c5f-1afa-4294-a7ea-c5f0a30a0738
📒 Files selected for processing (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts (1)
19-26: Minor type signature mismatch fordescription.The
SettingIteminterface definesdescriptionas(_appType: AppType) => string, but this helper returns a function that ignores its parameter. While this works at runtime, it's slightly inconsistent with the actual interface.✨ Proposed fix
const makeSetting = (label: string, description: string): SettingItem => { const setting: SearchableSetting = { label: () => label, - description: () => description, + description: (_appType: AppType) => description, }; return setting as SettingItem; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts` around lines 19 - 26, The helper makeSetting returns a description function that ignores the AppType parameter, which mismatches SettingItem's description signature; update makeSetting so description is declared as (appType: AppType) => string (e.g., description: (appType: AppType) => description) or explicitly accept and ignore the parameter (description: (_appType: AppType) => description) to match SettingItem/SearchableSetting types; keep the return cast to SettingItem and adjust the label similarly if needed.
🤖 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.search.test.ts`:
- Around line 5-17: Tests are validating a duplicated local copy of the private
settingMatchesSearch implementation from ConfigScreen.tsx, so extract that logic
into a shared utility (e.g., ConfigScreen.utils.ts) and export
settingMatchesSearch (same signature: (md: SearchableSetting, searchQuery:
string): boolean) so both ConfigScreen.tsx (replace the private method with an
import) and ConfigScreen.search.test.ts can import and test the single
implementation; if you cannot extract now, add a clear comment above the local
copy in ConfigScreen.search.test.ts pointing to the original private method in
ConfigScreen.tsx to indicate intentional duplication.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts`:
- Around line 19-26: The helper makeSetting returns a description function that
ignores the AppType parameter, which mismatches SettingItem's description
signature; update makeSetting so description is declared as (appType: AppType)
=> string (e.g., description: (appType: AppType) => description) or explicitly
accept and ignore the parameter (description: (_appType: AppType) =>
description) to match SettingItem/SearchableSetting types; keep the return cast
to SettingItem and adjust the label similarly if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7565403-7355-4dd1-9536-2b5896bdf17b
📒 Files selected for processing (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts (1)
6-13: Consider aligning mock signature with actual SettingItem type.The
descriptionmethod inSettingItemaccepts anAppTypeparameter:description?: (_appType: AppType) => string. The mock ignores this parameter, which works but could mask issues if the implementation changes.♻️ Optional: Match the actual signature
+import { AppType, SettingItem } from '@joplin/lib/models/Setting'; -import { SettingItem } from '@joplin/lib/models/Setting'; const makeSetting = (label: string, description: string): SettingItem => { const setting: SearchableSetting = { label: () => label, - description: () => description, + description: (_appType: AppType) => description, }; return setting as SettingItem; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts` around lines 6 - 13, The mock factory makeSetting returns a SearchableSetting but its description currently ignores the AppType parameter; update the description property to match the real SettingItem signature by declaring description: ( _appType: AppType ) => description so it accepts the AppType argument (and add an import or type reference for AppType if required). Keep label as () => label and ensure the returned setting is still cast as SettingItem.packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
480-491: Omit the optionalinputRefprop for cleaner code.Since
SearchInputhasinputRefas an optional prop (marked with?in its Props interface), passinginputRef={null}is unnecessary. Remove it entirely for better readability.🤖 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 480 - 491, In the ConfigScreen component (searchInputComp rendering), remove the unnecessary explicit inputRef={null} prop passed to the SearchInput component since inputRef is optional; update the JSX where SearchInput is used (searchInputComp) to omit the inputRef prop and keep the other props (value={this.state.searchQuery}, onChange={this.onSearchQueryChange}, onSearchButtonClick={this.onSearchButtonClick}, searchStarted={!!this.state.searchQuery}, placeholder) unchanged.
🤖 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.search.test.ts`:
- Around line 1-45: This file has CRLF line endings; convert all line endings in
ConfigScreen.search.test.ts to LF to satisfy ESLint. Update the file so every
line uses Unix-style line endings (LF) — no changes to code like SettingItem,
makeSetting, settingMatchesSearch, or the test blocks — just normalize the file
ending format and re-run linter/CI to confirm the error is resolved.
In `@packages/app-desktop/gui/ConfigScreen/configScreenUtils.ts`:
- Around line 1-16: The file has CRLF line endings causing ESLint errors;
convert all lines in packages/app-desktop/gui/ConfigScreen/configScreenUtils.ts
to LF endings. Open the file containing the settingMatchesSearch function and
normalize line endings (e.g., via your editor or run a tool such as sed -i
's/\r$//' on the file) so the whole file uses LF instead of CRLF, then re-run
lint to confirm the CRLF errors are resolved.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts`:
- Around line 6-13: The mock factory makeSetting returns a SearchableSetting but
its description currently ignores the AppType parameter; update the description
property to match the real SettingItem signature by declaring description: (
_appType: AppType ) => description so it accepts the AppType argument (and add
an import or type reference for AppType if required). Keep label as () => label
and ensure the returned setting is still cast as SettingItem.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 480-491: In the ConfigScreen component (searchInputComp
rendering), remove the unnecessary explicit inputRef={null} prop passed to the
SearchInput component since inputRef is optional; update the JSX where
SearchInput is used (searchInputComp) to omit the inputRef prop and keep the
other props (value={this.state.searchQuery},
onChange={this.onSearchQueryChange},
onSearchButtonClick={this.onSearchButtonClick},
searchStarted={!!this.state.searchQuery}, placeholder) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cf175f9-ab49-4a72-8468-66eee00f75b3
📒 Files selected for processing (3)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.tspackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/configScreenUtils.ts
| import { AppType, SettingItem } from '@joplin/lib/models/Setting'; | ||
|
|
||
| export const settingMatchesSearch = (md: Pick<SettingItem, 'label' | 'description'>, searchQuery: string): boolean => { | ||
| if (!searchQuery) return true; | ||
|
|
||
| const q = searchQuery.trim().toLowerCase(); | ||
| if (!q) return true; | ||
|
|
||
| const labelText = md.label ? md.label() : ''; | ||
| const descriptionText = md.description ? md.description(AppType.Desktop) : ''; | ||
|
|
||
| const label = String(labelText || '').toLowerCase(); | ||
| const description = String(descriptionText || '').toLowerCase(); | ||
|
|
||
| return label.includes(q) || description.includes(q); | ||
| }; |
There was a problem hiding this comment.
Fix CRLF line endings to LF.
ESLint reports CRLF line endings throughout this file. The repository expects LF line endings for consistency. Convert all line endings to LF before merging.
🔧 Fix line endings
Run the following command to convert line endings:
sed -i 's/\r$//' packages/app-desktop/gui/ConfigScreen/configScreenUtils.tsOr configure your editor to use LF line endings for this repository.
🧰 Tools
🪛 ESLint
[error] 1-2: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 2-3: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 3-16: Prefer default export on a file with single export.
(import/prefer-default-export)
[error] 3-4: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 4-5: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 5-6: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 6-7: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 7-8: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 8-9: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 9-10: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 10-11: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 11-12: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 12-13: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 13-14: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 14-15: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
[error] 15-16: Expected linebreaks to be 'LF' but found 'CRLF'.
(linebreak-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/configScreenUtils.ts` around lines 1 -
16, The file has CRLF line endings causing ESLint errors; convert all lines in
packages/app-desktop/gui/ConfigScreen/configScreenUtils.ts to LF endings. Open
the file containing the settingMatchesSearch function and normalize line endings
(e.g., via your editor or run a tool such as sed -i 's/\r$//' on the file) so
the whole file uses LF instead of CRLF, then re-run lint to confirm the CRLF
errors are resolved.
|
As per the PR guidelines could you please add a video and AI disclosure section to your PR? |
Updated PR description with Problem / Solution / Test Plan, added video and AI disclosure as requested. |
|
Hello, for information, the issue has been updated with detailed spec on how it should work: If you would like to continue working on this pull request, please check the spec. |
Done👍 |
|
Thank you for working on this! Since this modifies an important part of the desktop app's UI:
|
Tested with NVDA on Windows and added a short video as well. The search field is reachable by keyboard, announced correctly by the screen reader, and filtering works without using a mouse. Matching results remain keyboard accessible, and focus behaves normally while navigating between the sidebar and filtered settings. |
coderabbitai
Bot
added
accessibility
Thanks @personalizedrefrigerator — I synced with the latest dev changes and addressed the two accessibility issues flagged by CI:
I also rechecked keyboard navigation for the sidebar/search flow locally. CI should reflect the updated state after this push. |
personalizedrefrigerator
left a comment
personalizedrefrigerator
left a comment
There was a problem hiding this comment.
Thank you for working on this! I've tested the pull request locally.
Observed issues:
- The up/down arrows only sometimes changes the selected tab:
Screencast.from.2026-03-23.08-52-36.webm
To reproduce: 1) start searching, 2) use the tab key to move keyboard focus to the tab list, and 3) press the up/down arrows. For me, this scrolls the tab list, rather than changes tabs. However, after clicking on the tab list, the arrow keys can successfully change the selected tab.
- Low contrast in highlight marks while searching: It's difficult to read highlighted text in light mode (see WCAG 2.2: Contrast/minimum):
| }; | ||
|
|
||
| describe('ConfigScreen search', () => { | ||
| test('empty query returns true', () => { |
There was a problem hiding this comment.
(Optional) It may be possible to simplify the tests in this file using test.each:
- Since all tests in this file call
settingMatchesSearchand assert different output,test.eachmay help reduce duplicate setup code and make it easier to add new tests. - However, the setup logic is already quite simple.
test.eachmight not provide much benefit.
| chunks.push(React.createElement('mark', { | ||
| key: `${index}-${end}`, | ||
| className: 'config-search-highlight', | ||
| }, source.slice(index, end))); |
There was a problem hiding this comment.
In general, please prefer JSX to React.createElement (this file may need to be changed to a .tsx file).
coderabbitai
Bot
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (2)
160-178:⚠️ Potential issue | 🟡 MinorPrevent disabled entries from still navigating to
#.The early return stops the section change, but the anchor's default click still runs. A greyed-out section can therefore still move the hash/scroll position, so it is not actually “not clickable”.
🔧 Suggested change
- onClick={() => { - if (disabled) return; - props.onSelectionChange({ section: section }); + onClick={event => { + event.preventDefault(); + if (disabled) return; + props.onSelectionChange({ 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 160 - 178, The anchor StyledListItem still navigates to '#' when disabled because the onClick early-return doesn't prevent the anchor's default; update the onClick handler on StyledListItem to accept the click event and call event.preventDefault() when disabled (e.g., onClick={(e) => { if (disabled) { e.preventDefault(); return; } props.onSelectionChange({ section }); }}), and optionally remove or set href to undefined when disabled to avoid native navigation; refer to StyledListItem, onClick, href='#', tabIndex and disabled to locate the change.
160-170:⚠️ Potential issue | 🟠 MajorSearch mode is no longer a real tab interface.
When a query is active, the right pane can show combined matches from multiple sections. In that state these controls are filters, not tabs, so
role='tablist'/role='tab'gives assistive tech the wrong interaction model. Either switch the search state to button/list semantics, or add an explicit “All results” tab/panel pair.Also applies to: 233-242
🤖 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 160 - 170, The items rendered by StyledListItem are currently given role='tab' but when a query/search is active they act as filters (not true tabs); update the logic so that the role and ARIA attributes switch based on the search state (e.g., use a boolean like isSearching or queryActive): when not searching keep role='tab' with aria-controls/aria-selected and tabIndex as-is; when searching change role to a button/list-item (e.g., role='button' or role='listitem') and replace aria-selected with the appropriate attribute (aria-pressed for buttons or aria-checked if using a listbox), and adjust tabIndex accordingly; apply the same conditional change for the other similar block (lines around 233-242) and keep references such as StyledListItem, buttonRefs.current[index], and the id/setting-tab-... naming consistent.
🤖 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/configScreenUtilsImpl.tsx`:
- Around line 8-16: The search logic is inconsistent: searchTerms(tokenizes) but
settingMatchesSearch and related match helpers still require the whole
normalized phrase to appear in a single field, so multi-word queries (e.g.,
"editor markdown") can highlight both tokens but fail to filter; fix by changing
settingMatchesSearch (and the other match helpers referenced) to use
searchTerms(searchQuery) and verify that every token is found somewhere in the
setting's searchable text (e.g., name, description, key, category) — i.e., build
a normalized combined field(s) or check each token against all searchable fields
and only include the setting if all tokens are present (order/contiguity not
required), keeping the existing highlightSearchText tokenization and reusing
searchTerms to ensure consistent behavior.
---
Outside diff comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 160-178: The anchor StyledListItem still navigates to '#' when
disabled because the onClick early-return doesn't prevent the anchor's default;
update the onClick handler on StyledListItem to accept the click event and call
event.preventDefault() when disabled (e.g., onClick={(e) => { if (disabled) {
e.preventDefault(); return; } props.onSelectionChange({ section }); }}), and
optionally remove or set href to undefined when disabled to avoid native
navigation; refer to StyledListItem, onClick, href='#', tabIndex and disabled to
locate the change.
- Around line 160-170: The items rendered by StyledListItem are currently given
role='tab' but when a query/search is active they act as filters (not true
tabs); update the logic so that the role and ARIA attributes switch based on the
search state (e.g., use a boolean like isSearching or queryActive): when not
searching keep role='tab' with aria-controls/aria-selected and tabIndex as-is;
when searching change role to a button/list-item (e.g., role='button' or
role='listitem') and replace aria-selected with the appropriate attribute
(aria-pressed for buttons or aria-checked if using a listbox), and adjust
tabIndex accordingly; apply the same conditional change for the other similar
block (lines around 233-242) and keep references such as StyledListItem,
buttonRefs.current[index], and the id/setting-tab-... naming consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3a3cef9-727b-42f5-a175-fc87bb27a997
📒 Files selected for processing (7)
.eslintignore.gitignorepackages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.tspackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-desktop/gui/ConfigScreen/configScreenUtils.tspackages/app-desktop/gui/ConfigScreen/configScreenUtilsImpl.tsxpackages/app-desktop/gui/ConfigScreen/style.scss
✅ Files skipped from review due to trivial changes (4)
- packages/app-desktop/gui/ConfigScreen/style.scss
- .gitignore
- .eslintignore
- packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts
coderabbitai
Bot
added
accessibility
Thanks for the detailed review sir, I’ve addressed the reported issues:
I’ve re-tested keyboard navigation and search behavior locally, including keyboard-only interaction. I’ve also attached a short video demonstrating: Joplin.DEV.-.C__Users_Lenovo_.config_joplindev-desktop.-.Options.2026-03-24.17-45-12.mp4
Please let me know if anything still needs adjustment. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (1)
162-178:⚠️ Potential issue | 🟠 MajorCancel the anchor default before bailing out on disabled tabs.
Line 176 returns early, but Line 162 still makes this a real link. A mouse click on a disabled section will therefore still follow
#, so the item is not actually non-clickable. Prevent the default click first, then handle selection.🛠️ Proposed fix
- onClick={() => { - if (disabled) return; - props.onSelectionChange({ section: section }); - }} + onClick={event => { + event.preventDefault(); + if (disabled) return; + props.onSelectionChange({ 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 162 - 178, The anchor still navigates to '#' when clicking a disabled tab because the onClick handler returns early after the browser has already processed the click; update the click handler on the anchor (the element that sets ref via buttonRefs.current[index] and uses Setting.isSubSection and props.onSelectionChange) to call event.preventDefault() as the first step, then if disabled return, otherwise call props.onSelectionChange({ section }). Ensure the handler signature accepts the click event so you can call preventDefault() before any early returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 162-178: The anchor still navigates to '#' when clicking a
disabled tab because the onClick handler returns early after the browser has
already processed the click; update the click handler on the anchor (the element
that sets ref via buttonRefs.current[index] and uses Setting.isSubSection and
props.onSelectionChange) to call event.preventDefault() as the first step, then
if disabled return, otherwise call props.onSelectionChange({ section }). Ensure
the handler signature accepts the click event so you can call preventDefault()
before any early returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05bcbe5b-8090-4923-abac-a2f2aadced4c
📒 Files selected for processing (1)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx
| // These screens handle their own loading/saving of settings and have bespoke rendering. | ||
| // When screenComp is null, it means we are viewing the regular settings. | ||
| const screenComp = this.state.screenName ? <div className="config-screen-content-wrapper" style={{ overflow: 'scroll', flex: 1 }}>{this.screenFromName(this.state.screenName)}</div> : null; | ||
| const screenComp = !isSearchActive && selectedSection?.isScreen && this.state.screenName ? <div className="config-screen-content-wrapper" style={{ overflow: 'scroll', flex: 1 }}>{this.screenFromName(this.state.screenName)}</div> : null; |
There was a problem hiding this comment.
Style: For readability, consider breaking this up into multiple lines. For example,
| const screenComp = !isSearchActive && selectedSection?.isScreen && this.state.screenName ? <div className="config-screen-content-wrapper" style={{ overflow: 'scroll', flex: 1 }}>{this.screenFromName(this.state.screenName)}</div> : null; | |
| const screenComp = !isSearchActive && selectedSection?.isScreen && this.state.screenName ? ( | |
| <div | |
| className="config-screen-content-wrapper" | |
| style={{ overflow: 'scroll', flex: 1 }} | |
| >{this.screenFromName(this.state.screenName)}</div> | |
| ) : null; |
| <> | ||
| {screenComp} | ||
| <div style={containerStyle}> | ||
| <div style={{ display: 'flex', flex: 1, flexDirection: 'column' }}> |
| <div style={{ display: 'flex', flex: 1, flexDirection: 'column' }}> | ||
| <div style={{ ...theme.textStyle, maxWidth: 640 }}> |
There was a problem hiding this comment.
As above, please use RSCSS for new styles. See spec/desktop_styling for details.
|
Thanks for the suggestions @personalizedrefrigerator I've updated the I’m also refactoring the new inline styles to use RSCSS and aligning them with the guidelines in Let me know if anything else stands out. |
|
@personalizedrefrigerator Hi, I’ve addressed all the requested changes and updated the PR accordingly. Would appreciate a re-review when you have time. Thanks! |
|
Hello, unfortunately we no longer need this pull request as there's a duplicate here, that we will probably merge instead: #14820 |
4 participants
Resolves #14763
Problem
The desktop configuration screen currently does not provide a way to search settings, which makes it difficult to quickly locate specific options when many settings are available. Users must manually scan through sections even when they already know the setting name or part of its description.
Solution
This change adds a search field to the desktop settings screen that filters visible settings by matching setting labels, descriptions, and section titles in a case-insensitive way.
The existing SearchInput component is reused so behaviour remains consistent with other parts of the application. The search field is placed in the sidebar at the top, like in the mobile app, and includes a clear button similar to the regular search field. The right panel displays all matching settings, while sections without matching settings remain visible in the sidebar but are grayed out and not clickable. By default, all matching settings are displayed, and users can click sections to filter to just those sections.
Filtering logic was extracted into a helper to keep ConfigScreen simpler and testable. Empty and null label/description values are handled safely to avoid crashes. Search also applies to plugin settings, and the search term is highlighted in labels, descriptions, and section titles.
This change only affects filtering behaviour in the desktop configuration screen and does not modify setting values or persistence.
Test Plan
Tested with NVDA on Windows.
*The new settings search field is reachable via keyboard, announced correctly as an editable search field, and can be used to filter settings without a mouse.
*Matching results remain navigable by keyboard, and focus behaves normally when moving between the sidebar and filtered settings.
Video
2026-03-19.19-33-01.mp4
AI Assistance Disclosure
AI-assisted: Yes
Used AI assistance for:
All code, testing, and final verification were manually reviewed before submission.