Desktop: Add search field to Config screen to filter settings#14779
Closed
Payel-Manna wants to merge 11 commits intolaurent22:devfrom
Closed
Desktop: Add search field to Config screen to filter settings#14779Payel-Manna wants to merge 11 commits intolaurent22:devfrom
Payel-Manna wants to merge 11 commits intolaurent22:devfrom
Conversation
Contributor
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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 search input to the desktop Config screen, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as ConfigScreen (UI)
participant Filter as filterSettingsByQuery
participant Meta as Settings Metadata
User->>UI: Type query / clear search
UI->>Filter: filterSettingsByQuery(sections, query, AppType)
Filter->>Meta: Read labels/descriptions (string or function)
Meta-->>Filter: Return matching keys
Filter-->>UI: Matched keys
UI->>UI: Render matched setting components or "No results"
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
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 (3)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsx (1)
4-18: Add a test for metadata entries withoutdescription.
packages/lib/models/Setting.ts(Line 604) explicitly guards for missing descriptions, so this is a real edge case worth pinning down in this suite.🧪 Suggested test addition
+ test('handles settings with no description', () => { + const sectionsWithoutDescription = [ + { + name: 'general', + metadatas: [ + { key: 'theme', label: 'Theme' }, + ], + }, + ]; + const result = filterSettingsByQuery(sectionsWithoutDescription as any, 'theme', AppType.Desktop); + expect(result).toContain('theme'); + });Based on learnings: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Focus on testing essential behaviour and edge cases — avoid adding tests for every minor detail.
Also applies to: 20-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsx` around lines 4 - 18, Add a test in ConfigScreen.test.tsx that covers metadata entries missing the description by extending the existing mockSections used in the tests (the mockSections array) with a section or metadata object that omits the description field, then render the ConfigScreen (or the specific component under test) and assert it handles the missing description safely (no crash and expected UI output, e.g., label/key is still shown). Reference the mockSections constant and the test that renders ConfigScreen to locate where to add this case; ensure the test exercises the codepath that maps metadata descriptions (covered by packages/lib/models/Setting.ts guards) so the component gracefully handles undefined description values.packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts (1)
12-15: Normalise the query once (and trim) before iterating metadata.This avoids repeated
toLowerCase()calls and treats whitespace-only input as empty search.♻️ Suggested refactor
): string[] => { + const normalisedQuery = query.trim().toLowerCase(); + if (!normalisedQuery) return []; + const matchedKeys: string[] = []; for (const section of sections) { for (const md of section.metadatas) { const label = (typeof md.label === 'function' ? md.label(appType) : md.label) || ''; const desc = (typeof md.description === 'function' ? md.description(appType) : md.description) || ''; - if (label.toLowerCase().includes(query.toLowerCase()) || desc.toLowerCase().includes(query.toLowerCase())) { + if (label.toLowerCase().includes(normalisedQuery) || desc.toLowerCase().includes(normalisedQuery)) { matchedKeys.push(md.key); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts` around lines 12 - 15, Normalize and trim the incoming query once before the loop to avoid repeated toLowerCase() calls and to treat whitespace-only input as empty; e.g., compute a single const normalizedQuery = (query || '').trim().toLowerCase() and then use normalizedQuery in the checks instead of calling query.toLowerCase() repeatedly when testing label and desc (where label is derived from md.label/appType and desc from md.description/appType) and when deciding whether to push md.key into matchedKeys.packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
494-502: Use theme-driven spacing for the search container instead of hard-coded padding.This makes the new UI consistent with app-wide styling behaviour and theme evolution.
🎨 Suggested adjustment
- <div style={{ padding: '10px 10px 0 10px' }}> + <div style={{ padding: theme.configScreenPadding, paddingBottom: 0 }}>Based on learnings: Applies to readme/dev/packages/{app-desktop,app-mobile}/**/*.{ts,tsx} : For GUI style changes in Desktop and mobile clients, refer to
packages/lib/theme.tsfor all styling information.🤖 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 494 - 502, Replace the hard-coded inline padding on the div wrapping SearchInput in the ConfigScreen render (the container around SearchInput in the ConfigScreen component) with theme-driven spacing from the shared theme (packages/lib/theme.ts); remove the style={{ padding: '10px 10px 0 10px' }} and instead apply the theme spacing via the project's styling approach (e.g., useTheme()/withTheme() or a CSS class that reads theme.space/spacing) so the container uses the centralized spacing tokens consistent with other GUI components.
🤖 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 509-521: The bug: setting containerStyle.display = "none" whenever
screenComp is truthy hides search results and keeps save/apply disabled during
search; change the condition to only hide the main config list when a specific
screen is open (this.state.screenName) and not when in search mode (e.g., when
this.state.searchQuery or a isSearching flag is present); update the logic
around screenComp usage so save/apply enablement checks the active mode instead
of blindly using screenComp (reference symbols: screenComp,
this.state.screenName, containerStyle, this.screenFromName, and the save/apply
enable code near lines using screenComp) so search results and buttons remain
visible/active while still hiding the list when a dedicated screen is shown.
- Around line 297-301: The conditional rendering of the secondary sync status
line in statusComp uses messages.length >= 1 but accesses messages[1], causing
an off-by-one; update the guard to messages.length >= 2 so the
<p>{messages[1]}</p> is only rendered when a second message exists (locate the
statusComp block and the messages array usage to apply this change).
- Around line 44-50: Update ConfigScreenComponent by replacing all any usages
with concrete types: create interfaces ConfigScreenProps and ConfigScreenState
and use React.Component<ConfigScreenProps, ConfigScreenState> and
constructor(props: ConfigScreenProps). Type the private rowStyle_ as
React.CSSProperties | null. Replace untyped event params with DOM event types
(e.g., React.MouseEvent, React.ChangeEvent<HTMLInputElement>) in handler
signatures. Define explicit interfaces for the render method params (e.g.,
interface Section { id: string; title: string; ... } and interface Setting {
key: string; value: string; type: string; ... }) and annotate renderSection and
renderSettings with those types. Finally, strongly type mapStateToProps to
accept the app RootState (or appropriate Redux state type) and return
ConfigScreenProps to remove anys from Redux mapping. Ensure all referenced
symbols (ConfigScreenComponent, rowStyle_, constructor, renderSection,
renderSettings, mapStateToProps) are updated accordingly.
In `@packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts`:
- Around line 3-8: Replace the loose any[] type on the sections parameter of
filterSettingsByQuery with the exported MetadataBySection type from the Setting
module: update the function signature in filterSettingsByQuery to use sections:
MetadataBySection and add/import MetadataBySection from the Setting module if
it's not already imported; ensure usages that access section.metadatas still
compile and adjust any related type annotations or exports so the function and
call sites use MetadataBySection consistently.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsx`:
- Around line 4-18: Add a test in ConfigScreen.test.tsx that covers metadata
entries missing the description by extending the existing mockSections used in
the tests (the mockSections array) with a section or metadata object that omits
the description field, then render the ConfigScreen (or the specific component
under test) and assert it handles the missing description safely (no crash and
expected UI output, e.g., label/key is still shown). Reference the mockSections
constant and the test that renders ConfigScreen to locate where to add this
case; ensure the test exercises the codepath that maps metadata descriptions
(covered by packages/lib/models/Setting.ts guards) so the component gracefully
handles undefined description values.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 494-502: Replace the hard-coded inline padding on the div wrapping
SearchInput in the ConfigScreen render (the container around SearchInput in the
ConfigScreen component) with theme-driven spacing from the shared theme
(packages/lib/theme.ts); remove the style={{ padding: '10px 10px 0 10px' }} and
instead apply the theme spacing via the project's styling approach (e.g.,
useTheme()/withTheme() or a CSS class that reads theme.space/spacing) so the
container uses the centralized spacing tokens consistent with other GUI
components.
In `@packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts`:
- Around line 12-15: Normalize and trim the incoming query once before the loop
to avoid repeated toLowerCase() calls and to treat whitespace-only input as
empty; e.g., compute a single const normalizedQuery = (query ||
'').trim().toLowerCase() and then use normalizedQuery in the checks instead of
calling query.toLowerCase() repeatedly when testing label and desc (where label
is derived from md.label/appType and desc from md.description/appType) and when
deciding whether to push md.key into matchedKeys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97d9051d-5127-408f-ab1e-41482ca23faa
⛔ Files ignored due to path filters (1)
packages/app-clipper/popup/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.eslintignore.gitignore.yarnrc.ymlpackages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsxpackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts
Comment on lines
+3
to
+8
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied | ||
| const filterSettingsByQuery = ( | ||
| sections: any[], | ||
| query: string, | ||
| appType: AppType, | ||
| ): string[] => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file content
cat -n packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts | head -30Repository: laurent22/joplin
Length of output: 1016
🏁 Script executed:
# Search for MetadataBySection type definition
rg "MetadataBySection" packages/app-desktop/ -A 3 -B 1Repository: laurent22/joplin
Length of output: 880
🏁 Script executed:
# Check what types are currently exported from the Setting module
rg "export.*type|export.*interface" `@joplin/lib/models/Setting.ts` -A 2Repository: laurent22/joplin
Length of output: 129
🏁 Script executed:
# Find the Setting module file
find . -name "Setting.ts" -o -name "Setting.tsx" | grep -E "lib|models"Repository: laurent22/joplin
Length of output: 92
🏁 Script executed:
# Try searching with rg for MetadataBySection definition
rg "type MetadataBySection|interface MetadataBySection" -A 5Repository: laurent22/joplin
Length of output: 443
🏁 Script executed:
# Check the packages/lib structure
ls -la packages/lib/models/ 2>/dev/null | head -20Repository: laurent22/joplin
Length of output: 1300
🏁 Script executed:
# Find SettingMetadataSection type definition
rg "type SettingMetadataSection|interface SettingMetadataSection" -A 10 packages/lib/models/Setting.tsRepository: laurent22/joplin
Length of output: 363
🏁 Script executed:
# Check current imports in the target file
cat -n packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts | head -5Repository: laurent22/joplin
Length of output: 299
🏁 Script executed:
# Verify if MetadataBySection is already exported from the Setting module and accessible
rg "export.*MetadataBySection" packages/lib/models/Setting.tsRepository: laurent22/joplin
Length of output: 117
Replace any[] with MetadataBySection type.
The sections parameter uses any[], which violates TypeScript guidelines. The proper type MetadataBySection is already exported from the Setting module and matches the code's usage (accessing section.metadatas).
🛠️ Suggested typing fix
-import { AppType } from '@joplin/lib/models/Setting';
+import { AppType, MetadataBySection } from '@joplin/lib/models/Setting';
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- Old code before rule was applied
const filterSettingsByQuery = (
- sections: any[],
+ sections: MetadataBySection,
query: string,
appType: AppType,
): string[] => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts` around lines
3 - 8, Replace the loose any[] type on the sections parameter of
filterSettingsByQuery with the exported MetadataBySection type from the Setting
module: update the function signature in filterSettingsByQuery to use sections:
MetadataBySection and add/import MetadataBySection from the Setting module if
it's not already imported; ensure usages that access section.metadatas still
compile and adjust any related type annotations or exports so the function and
call sites use MetadataBySection consistently.
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (1)
509-520:⚠️ Potential issue | 🟠 MajorSearch mode still inherits the screen-only hiding logic.
If a screen-based section is already open, Line 518 hides the same
containerStyleused for the search results at Lines 587-593, and Lines 619-620 stay disabled becausescreenCompremains truthy. Typing in the new search field from Encryption/Keymap therefore shows nothing and blocks editing.💡 Suggested change
- const screenComp = this.state.screenName ? ( + const isSearchMode = !!this.state.searchQuery; + const screenComp = !isSearchMode && this.state.screenName ? ( <div - className="config-screen-content-wrapper" - style={{ overflow: "scroll", flex: 1 }} + className='config-screen-content-wrapper' + style={{ overflow: 'scroll', flex: 1 }} > {this.screenFromName(this.state.screenName)} </div> ) : null; @@ - const mainContent = this.state.searchQuery ? ( + const mainContent = isSearchMode ? (Also applies to: 587-620
🤖 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 509 - 520, The current logic hides the shared containerStyle whenever screenComp (derived from this.state.screenName via screenFromName) is truthy, which also hides search results and keeps UI controls disabled; introduce an isSearching boolean (derived from the search input state you already have, e.g., this.state.searchQuery or this.state.searchTerm) and change the conditional checks so the containerStyle display and any disabled props only apply when screenComp is truthy AND not isSearching (e.g., if (screenComp && !isSearching) containerStyle.display = "none"; and use (screenComp && !isSearching) for button/field disabled checks) so opening a search field in a screened section shows results and enables editing.
🤖 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 1-638: The file fails lint due to indentation and spacing (rules:
indent, `@typescript-eslint/indent`, curly, comma-spacing,
type-annotation-spacing); reformat the entire ConfigScreen.tsx to use tabs
(project guideline) and correct spacing/brace placement so eslint passes — focus
on correcting indentation and spacing in the methods and JSX around
ConfigScreenComponent (constructor, componentDidMount, render,
sectionToComponent, settingToComponent, sidebar_selectionChange,
handleSettingButton) and the JSX blocks for searchBar, tabComponents, and
mainContent; run the repository formatter/eslint auto-fix (e.g. prettier/eslint
--fix) or apply the project's tab-based formatting rules to ensure all commas,
type annotations, and curly braces follow the repo style before committing.
In `@packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts`:
- Line 27: The file ends without a trailing newline causing an eol-last lint
failure; open the file containing the export default filterSettingsByQuery and
add a single newline character at the end of the file (ensure the file
terminates with a blank line so the final line ends with a newline).
---
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 509-520: The current logic hides the shared containerStyle
whenever screenComp (derived from this.state.screenName via screenFromName) is
truthy, which also hides search results and keeps UI controls disabled;
introduce an isSearching boolean (derived from the search input state you
already have, e.g., this.state.searchQuery or this.state.searchTerm) and change
the conditional checks so the containerStyle display and any disabled props only
apply when screenComp is truthy AND not isSearching (e.g., if (screenComp &&
!isSearching) containerStyle.display = "none"; and use (screenComp &&
!isSearching) for button/field disabled checks) so opening a search field in a
screened section shows results and enables editing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6f08b09-eff1-46c9-86a6-e2bab874e7c7
📒 Files selected for processing (3)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsxpackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-desktop/gui/ConfigScreen/ConfigScreen.test.tsx
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx (3)
432-442:⚠️ Potential issue | 🟠 MajorReformat the new render block to tabs; lint is failing on these added lines.
The newly added search/render block uses space-based indentation and currently violates repo indentation rules, which will keep CI red.
As per coding guidelines:
**/*.{ts,tsx,js,jsx}: Use tabs for indentation.Also applies to: 467-532
🤖 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 432 - 442, The new JSX block for searchBar is indented with spaces which violates the repo rule requiring tabs; update the indentation of the entire search/render block (the const searchBar declaration and its JSX children using SearchInput, including props value={this.state.searchQuery}, onChange={this.onSearchChange}, onSearchButtonClick={this.onSearchClear}, searchStarted and placeholder) to use tabs instead of spaces, and apply the same tabs-based reformatting to the additional affected lines around the render (the other search/render lines referenced) so linting passes.
449-451:⚠️ Potential issue | 🟠 MajorSearch mode is still coupled to screen mode and can hide results.
When
screenNameis set, Line 451 hidescontainerStyle. In search mode, the same container is reused (Line 503), so results can disappear; save/apply also remain disabled viascreenCompchecks on Line 527-528.🐛 Suggested fix
+ const isSearchMode = !!this.state.searchQuery; - 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 = !isSearchMode && this.state.screenName ? <div className='config-screen-content-wrapper' style={{ overflow: 'scroll', flex: 1 }}>{this.screenFromName(this.state.screenName)}</div> : null; - if (screenComp) containerStyle.display = 'none'; + if (!isSearchMode && screenComp) containerStyle.display = 'none'; -const mainContent = this.state.searchQuery ? ( +const mainContent = isSearchMode ? ( @@ - onSaveClick={screenComp ? null : this.onSaveClick} - onApplyClick={screenComp ? null : this.onApplyClick} + onSaveClick={!isSearchMode && screenComp ? null : this.onSaveClick} + onApplyClick={!isSearchMode && screenComp ? null : this.onApplyClick}Also applies to: 503-510, 525-529
🤖 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 449 - 451, The code hides the main container whenever screenComp is truthy (computed from this.state.screenName), which also unintentionally hides search results and disables save/apply when in search mode; update the conditional checks that use screenComp (including the assignment to containerStyle.display and the save/apply gating around where screenComp is checked) to only treat screen mode as exclusive when not in search mode (e.g., change checks from "if (screenComp)" to "if (screenComp && !this.state.isSearchMode)" or the equivalent boolean that indicates search is active), and apply the same guarded logic to the other occurrences near where search UI is rendered so screenFromName, screenComp, and containerStyle no longer interfere with search results or button states.
72-74:⚠️ Potential issue | 🟠 MajorReplace
anyin search change handler with the SearchInput event type.
onSearchChangecurrently usesevent: anyon Line 72, which violates the TypeScript type safety guideline. Use theOnChangeEventinterface exported bySearchInput.Suggested fix
-import SearchInput from '../lib/SearchInput/SearchInput'; +import SearchInput, { OnChangeEvent } from '../lib/SearchInput/SearchInput'; @@ - private onSearchChange = (event: any) => { + private onSearchChange = (event: OnChangeEvent) => { this.setState({ searchQuery: event.value }); };🤖 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 72 - 74, Replace the loose any type on the search handler by importing and using the specific OnChangeEvent from SearchInput: update the onSearchChange signature (method onSearchChange) to accept event: OnChangeEvent, import OnChangeEvent from the SearchInput module, and keep using event.value to setState({ searchQuery: event.value }) so the handler is type-safe and aligns with the SearchInput event interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 432-442: The new JSX block for searchBar is indented with spaces
which violates the repo rule requiring tabs; update the indentation of the
entire search/render block (the const searchBar declaration and its JSX children
using SearchInput, including props value={this.state.searchQuery},
onChange={this.onSearchChange}, onSearchButtonClick={this.onSearchClear},
searchStarted and placeholder) to use tabs instead of spaces, and apply the same
tabs-based reformatting to the additional affected lines around the render (the
other search/render lines referenced) so linting passes.
- Around line 449-451: The code hides the main container whenever screenComp is
truthy (computed from this.state.screenName), which also unintentionally hides
search results and disables save/apply when in search mode; update the
conditional checks that use screenComp (including the assignment to
containerStyle.display and the save/apply gating around where screenComp is
checked) to only treat screen mode as exclusive when not in search mode (e.g.,
change checks from "if (screenComp)" to "if (screenComp &&
!this.state.isSearchMode)" or the equivalent boolean that indicates search is
active), and apply the same guarded logic to the other occurrences near where
search UI is rendered so screenFromName, screenComp, and containerStyle no
longer interfere with search results or button states.
- Around line 72-74: Replace the loose any type on the search handler by
importing and using the specific OnChangeEvent from SearchInput: update the
onSearchChange signature (method onSearchChange) to accept event: OnChangeEvent,
import OnChangeEvent from the SearchInput module, and keep using event.value to
setState({ searchQuery: event.value }) so the handler is type-safe and aligns
with the SearchInput event interface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c832963d-ea7e-460e-92ba-0442d418399c
📒 Files selected for processing (2)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-desktop/gui/ConfigScreen/filterSettingsByQuery.ts
Owner
|
Hello, unfortunately we no longer need this pull request as there's a duplicate here, that will probably merge instead: #14820 |
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
2 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.
Closes #14763
Problem
The desktop Config screen had no way to search for settings by name or
description. Users had to manually browse each section to find a setting.
The mobile app already has this feature but it was missing on desktop.
Solution
Adds a
SearchInputfield at the top of the Config screen. When a queryis typed, settings are filtered across all sections by label and
description (case-insensitive). A clear (×) button resets the view.
When search is empty, normal section/tab behaviour is fully preserved.
The filter logic is extracted into
filterSettingsByQuery.ts— astandalone utility with no UI dependencies, making it fully testable.
Test Plan
Unit tests added in
ConfigScreen.test.tsxcovering:Manual testing:
Video
AI Assistance Disclosure
I used AI assistance (Claude) to help debug issues during development,
specifically to identify the correct SearchInput component API and fix
TypeScript type errors. All logic was written and verified by me.