Desktop: Resolves #14763: Added support for settings search in the Desktop App#14921
Open
ARPIT-NAYAK-LEGEND wants to merge 12 commits intolaurent22:devfrom
Open
Desktop: Resolves #14763: Added support for settings search in the Desktop App#14921ARPIT-NAYAK-LEGEND wants to merge 12 commits intolaurent22:devfrom
ARPIT-NAYAK-LEGEND wants to merge 12 commits intolaurent22:devfrom
Conversation
- Added search field to top of the sidebar - Search settings by label, description, and section title - Highlight matching text in section names (yellow background) - Gray out sections without matching settings - Added clear button to reset search - Support plugin settings filtering Implements laurent22#14763
- Added search field to top of the sidebar - Search settings by label, description, and section title - Highlight matching text in section names (yellow background) - Gray out sections without matching settings - Added clear button to reset search - Support plugin settings filtering Implements laurent22#14763
Contributor
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
Contributor
📝 WalkthroughWalkthroughAdds a searchable desktop ConfigScreen with sidebar filtering and highlighted matches, many new setting controls (including font and plugin management UIs), unit tests for search, and updates to ignore files to exclude two new GUI modules from linters/build outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Sidebar as Sidebar (UI)
participant Config as ConfigScreen
participant Renderer as SettingsRenderer
participant Repo as RepoAPI
participant Plugin as PluginService
User->>Sidebar: Type search query
Sidebar->>Config: onSearchQueryChange(query)
Config->>Config: set state.searchQuery / compute filteredSections
Config->>Renderer: render(filteredSections)
Renderer->>Renderer: omit non-matching metadata entries
Sidebar->>Sidebar: highlight matches (HighlightedText)
alt plugin search
Sidebar->>Repo: search(query)
Repo-->>Sidebar: manifests/results
Sidebar->>Plugin: install/update actions
Plugin-->>Config: update plugin settings/state
end
Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
Author
|
I have read the CLA Document and I hereby sign the CLA. |
![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: 12
🧹 Nitpick comments (7)
packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js (1)
7-10: Consider state-aware tooltip text for the toggle action.When expanded, the button still announces “Show Advanced Settings”. Switching title text by state would better match the actual action.
♻️ Proposed tweak
- title: (0, locale_1._)('Show Advanced Settings') + title: (0, locale_1._)(props.advancedSettingsVisible ? 'Hide Advanced Settings' : 'Show Advanced Settings')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js` around lines 7 - 10, ToggleAdvancedSettingsButton uses a static title "Show Advanced Settings" regardless of props.advancedSettingsVisible; update the title to be state-aware by switching the localized string based on props.advancedSettingsVisible (e.g., use _('Show Advanced Settings') when false and _('Hide Advanced Settings') when true) so the Button_1.default title reflects the action; modify the title prop in the ToggleAdvancedSettingsButton return to compute the correct string from props.advancedSettingsVisible.packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts (1)
3-277: Flatten this into onedescribe()and table-drive the repeated cases.The file currently nests multiple
describe()blocks and repeats the same setup/assertion shape across the matching tests. A single top-level suite withtest.eachwould follow the repo's Jest conventions and keep the coverage easier to maintain. As per coding guidelines, 'When creating Jest tests, there should be only onedescribe()statement in the file' and 'Avoid duplicating code in tests; usetest.eachor shared helpers instead of repeating similar test blocks'.🤖 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 3 - 277, Flatten the multiple nested describe blocks into a single top-level describe and convert repeating test patterns into table-driven tests using test.each: collapse the groups that call instance.matchesSearchQuery into one table-driven test referencing matchesSearchQuery, similarly collapse all getFilteredSections cases into a test.each that supplies searchQuery, sections, and expected filtered results, and convert the setSearchQuery/clearSearch tests into parameterized cases or small helpers calling setSearchQuery and clearSearch; keep the existing setup that constructs ConfigScreenComponent and sets instance.state, and reference the unique symbols matchesSearchQuery, getFilteredSections, setSearchQuery, clearSearch, and ConfigScreenComponent when making the changes so assertions and state updates run against the same instance.packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (1)
83-128: Move the new search-bar metrics into the shared theme.The added
8px,6px 12px,4px, and0.7values hard-code a second set of desktop UI tokens here, which makes the sidebar harder to keep consistent across themes. Based on learnings, 'For GUI style changes in Desktop and mobile clients, refer topackages/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/Sidebar.tsx` around lines 83 - 128, The styles in StyledSearchContainer, StyledSearchInput, and StyledClearButton use hard-coded tokens (8px, 6px 12px, 4px, 0.7) — move these into the shared theme and consume them from props.theme instead; add new token names (e.g. searchPadding, searchInputPadding, searchBorderRadius, searchActiveOpacity or similar) to the shared theme interface and theme objects, then replace the hard-coded values in those styled components to use (props: StyleProps) => props.theme.<tokenName> so the sidebar uses centralized theme tokens.packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js (2)
44-52: Module-level singleton may cause stale data and testing difficulties.The
repoApi_singleton (line 44) persists across component mounts and the entire application lifecycle. While this provides caching benefits, it has drawbacks:
- Stale data: If manifests change on the server, users must restart the app to see updates (even
reinitialize()only refreshes after 5 minutes per the relevant snippet).- Testing: The singleton makes unit testing difficult as state leaks between tests.
- Memory: The cached manifests are never released.
Consider whether this is intentional behaviour or if a context-based approach would be more appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js` around lines 44 - 52, The module-level singleton repoApi_ used by repoApi() causes stale state and test leakage; replace it with a non-persistent, test-friendly approach: remove the persistent repoApi_ variable and make repoApi() return a fresh RepositoryApi instance (RepositoryApi_.ofDefaultJoplinRepo or RepositoryApi_ constructor) each call or accept an injectable factory/context so callers can supply/mock the repo API; alternatively add an explicit reset/getInstance mechanism (e.g., repoApi.clear() or an injectable getRepoApi function) so tests can control lifecycle and the component can obtain a context-scoped repo API rather than relying on the module-level singleton.
213-218: ConfusingallDeletedlogic could be simplified.The expression
!pluginItems.find(it => it.deleted !== true)is logically correct but hard to parse. UsingArray.every()would be clearer and more idiomatic.function renderUserPlugins(pluginItems) { - const allDeleted = !pluginItems.find(it => it.deleted !== true); + const allDeleted = pluginItems.every(it => it.deleted === true); if (!pluginItems.length || allDeleted) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js` around lines 213 - 218, In renderUserPlugins, replace the confusing allDeleted calculation based on find with a clearer Array.every check: compute allDeleted as pluginItems.every(it => it.deleted === true) (or pluginItems.every(it => !!it.deleted)) so the intent is obvious; keep the existing branching that returns the empty-state when !pluginItems.length || allDeleted unchanged and reference the allDeleted variable and pluginItems array when making the replacement.packages/app-desktop/gui/ConfigScreen/Sidebar.js (1)
143-144: Unused variableisFiltered.The variable
isFilteredis computed on line 143 but never used. ThebuttonStylelogic on lines 145-149 useshasMatchesinstead. EitherisFilteredshould be removed, or it was intended to be used in place ofhasMatches.function renderButton(section, index) { const selected = props.selection === section.name; - const isFiltered = props.filteredSections && !props.filteredSections.has(section.name); const hasMatches = !props.filteredSections || props.filteredSections.has(section.name) || !props.searchQuery;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/Sidebar.js` around lines 143 - 144, The local variable isFiltered is computed but never used; remove the unused declaration (the const isFiltered = ... line) to avoid dead code in Sidebar.js, or if the intended logic was to use it in the buttonStyle decision, replace usages of hasMatches in that block with the correct isFiltered-based expression (using section.name, props.filteredSections and props.searchQuery) so the buttonStyle reflects the intended filtered state.packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js (1)
1-4: Review the TypeScript source file instead of the compiled output.This file is compiled JavaScript output (note the CommonJS
Object.definePropertypattern andsourceMappingURLreference at line 220). Changes made here will be overwritten on the next build. Review and modifypackages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsxinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js` around lines 1 - 4, The PR edits the compiled output (PluginBox.js) instead of the TSX source; stop modifying the generated CommonJS file and make your changes in the TypeScript source (PluginBox.tsx). Revert any changes to the compiled symbols (exports UpdateState, InstallState, default_1) in the JS file, then implement the intended fixes/logic/formatting in the PluginBox.tsx component so they survive rebuilds; ensure the build produces the updated PluginBox.js and remove any direct edits to the compiled file from the commit.
🤖 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.js`:
- Around line 41-42: The compiled JS (ConfigScreen.js) is out of sync with the
TSX source—specifically it still creates and passes filteredSections and uses
the old getFilteredSections flow (e.g., the constructor/state with
filteredSections and any usages that pass filteredSections to Sidebar).
Regenerate the JS artifact from the final ConfigScreen.tsx so the runtime
matches source: update the state initialization and remove the obsolete
filteredSections/getFilteredSections logic and any prop passing of
filteredSections to Sidebar (search-related state should match TSX), then
rebuild so ConfigScreen.js reflects the current ConfigScreen component
implementation.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts`:
- Around line 19-28: The tests reference removed internals
(state.filteredSections and getFilteredSections) of ConfigScreen; update them to
assert rendered output instead: set the component's searchQuery via the
ConfigScreen props or simulate typing into the search input (targeting the
ConfigScreen component and its search input), then verify which section headers
and setting rows are present/hidden in the DOM rather than inspecting
state.filteredSections or calling getFilteredSections; replace any assertions
referencing state.filteredSections/getFilteredSections with queries for elements
by text, role, or test-id corresponding to the section names and setting labels
rendered by ConfigScreen so the suite tests render-time filtering behavior.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx`:
- Around line 453-467: Don't drop non-matching sections from the tab model: keep
the original sections array intact and stop assigning a pre-filtered list to the
UI model (avoid replacing sections with filteredSections); instead compute a
separate boolean/map of match results for each section (use
Setting.sectionNameToLabel(section.name) when comparing names) and use that to
drive "matched" state and reduced-opacity/no-match display in Sidebar and
tabpanels; update the code around filteredSections and any uses in rendering
(including the tabpanel rendering logic) to consult the match map (and continue
using this.matchesSearchQuery for metadata checks) so the active tab remains
present even when it doesn't match the search.
In `@packages/app-desktop/gui/ConfigScreen/controls/FontSearch.js`:
- Around line 12-23: The loading state is incorrectly derived from
filteredAvailableFonts, so an empty monospace filter shows "Loading..."; change
isLoadingFonts to depend on whether font data has loaded (e.g.,
availableFonts.length === 0 or an explicit loaded flag) instead of
filteredAvailableFonts.length, and keep filteredAvailableFonts empty when no
monospace matches; update the declaration of isLoadingFonts and any related
checks that reference it (symbols: isLoadingFonts, filteredAvailableFonts,
availableFonts, isMonoBoxChecked, setFilteredAvailableFonts) so the combobox can
show an empty result rather than a loading indicator.
In `@packages/app-desktop/gui/ConfigScreen/controls/HighlightedText.tsx`:
- Around line 49-51: The HighlightedText component is using a hard-coded
backgroundColor ('#ffeb3b') in its inline style; replace that literal with a
theme-driven token imported from the app theme so highlights follow light/dark
and global tokens. Update the style at the backgroundColor key inside
HighlightedText to use the theme token (e.g., theme.highlightBackground or the
appropriate token exported from packages/lib/theme.ts) by importing the theme or
useTheme hook from that module and applying theme.<token> instead of '#ffeb3b',
ensuring the token exists in packages/lib/theme.ts and adjusting any
padding/contrast if needed.
- Line 15: The declaration for the local variable parts uses Array<{ text:
string; highlighted: boolean }>, which violates the
`@typescript-eslint/array-type` rule; update the type to the shorthand form (an
array of the same object shape) where parts is defined in HighlightedText (the
parts variable) so the type becomes the equivalent T[] syntax.
In `@packages/app-desktop/gui/ConfigScreen/controls/MissingPasswordHelpLink.js`:
- Around line 1-19: This is a compiled JS artifact (MissingPasswordHelpLink.js)
that should not be committed; remove this file from the repo and revert any
other transpiled outputs in the same directory, then ensure the TypeScript
source (MissingPasswordHelpLink.tsx) remains and the build/CI generates the
corresponding JS during the build step; also verify .gitignore includes the
generated JS pattern and that no code references (e.g.,
MacOSMissingPasswordHelpLink or openMissingPasswordFAQ) were accidentally edited
in the TSX source—if needed restore the original MissingPasswordHelpLink.tsx
from source and run the build to regenerate the JS locally to confirm everything
compiles.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js`:
- Around line 166-173: The Update button's title mapping and disabled logic are
wrong: change the title handling in PluginBox (the title variable set from
props.updateState) so UpdateState.CanUpdate => _('Update'), UpdateState.Updating
=> _('Updating...'), UpdateState.Idle => _('Update') (or a neutral label) and
UpdateState.HasBeenUpdated => _('Updated'); then make the button disabled
whenever props.updateState !== UpdateState.CanUpdate (replace the current
disabled check that only tests HasBeenUpdated) so the button is enabled
exclusively for UpdateState.CanUpdate; keep the onClick (props.onUpdate({ item
})) as-is.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js`:
- Around line 149-162: Wrap the async body of the onInstall callback in a
try-catch: when calling pluginService.installPlugin(filePath) and subsequent
code that updates pluginSettings and calls props.onChange, catch any thrown
error, log it (console.error or existing logger) and surface a user-friendly
error (e.g., show a dialog or set an error state) then return early so
pluginSettings/props.onChange are not mutated on failure; keep references to
onInstall, pluginService.installPlugin, pluginSettings, props.onChange,
pluginService.serializePluginSettings, and PluginService_1.defaultPluginSetting
to locate and protect the code paths.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.js`:
- Around line 30-44: The async search enqueued in useEffect can return
out-of-order and overwrite newer results; capture the current props.searchQuery
into a local const (e.g. const query = props.searchQuery) inside the effect
before pushing to asyncSearchQueue.current, then inside the enqueued async
callback compare that captured query to the latest props.searchQuery (or a ref
holding latest query) after the await props.repoApi().search(...) and before
calling setManifests/setSearchResultCount; if they differ, discard the response
to avoid stale updates. Use the existing asyncSearchQueue, setManifests, and
setSearchResultCount symbols and ensure the check runs both when results are
empty and when populated.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.js`:
- Around line 73-90: The search input StyledSearchInput is using theme.color2
over theme.backgroundColor causing poor contrast in light theme; update
StyledSearchInput to use theme.color for the input text color (replace uses of
props.theme.color2 with props.theme.color) so the input text pairs correctly
with theme.backgroundColor like other main content elements; ensure this change
is applied only to the color property inside StyledSearchInput and keep all
other styling (placeholder color, focus border, etc.) unchanged.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 224-245: The search input and clear button are incorrectly
rendered as children of the element with role='tablist' (StyledRoot) which
should only contain tabs; move the search controls out of the StyledRoot so the
element with role='tablist' contains only the section tabs (the variable
buttons). Concretely, render a wrapper (e.g., StyledSearchContainer with the
existing StyledSearchInput and StyledClearButton using props.searchQuery,
props.onSearchQueryChange and props.onClearSearch) above or before the
StyledRoot, and keep StyledRoot (role='tablist') around only the buttons
variable; ensure aria/role attributes remain unchanged on StyledRoot and that
search markup uses the existing props handlers.
---
Nitpick comments:
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts`:
- Around line 3-277: Flatten the multiple nested describe blocks into a single
top-level describe and convert repeating test patterns into table-driven tests
using test.each: collapse the groups that call instance.matchesSearchQuery into
one table-driven test referencing matchesSearchQuery, similarly collapse all
getFilteredSections cases into a test.each that supplies searchQuery, sections,
and expected filtered results, and convert the setSearchQuery/clearSearch tests
into parameterized cases or small helpers calling setSearchQuery and
clearSearch; keep the existing setup that constructs ConfigScreenComponent and
sets instance.state, and reference the unique symbols matchesSearchQuery,
getFilteredSections, setSearchQuery, clearSearch, and ConfigScreenComponent when
making the changes so assertions and state updates run against the same
instance.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js`:
- Around line 1-4: The PR edits the compiled output (PluginBox.js) instead of
the TSX source; stop modifying the generated CommonJS file and make your changes
in the TypeScript source (PluginBox.tsx). Revert any changes to the compiled
symbols (exports UpdateState, InstallState, default_1) in the JS file, then
implement the intended fixes/logic/formatting in the PluginBox.tsx component so
they survive rebuilds; ensure the build produces the updated PluginBox.js and
remove any direct edits to the compiled file from the commit.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js`:
- Around line 44-52: The module-level singleton repoApi_ used by repoApi()
causes stale state and test leakage; replace it with a non-persistent,
test-friendly approach: remove the persistent repoApi_ variable and make
repoApi() return a fresh RepositoryApi instance
(RepositoryApi_.ofDefaultJoplinRepo or RepositoryApi_ constructor) each call or
accept an injectable factory/context so callers can supply/mock the repo API;
alternatively add an explicit reset/getInstance mechanism (e.g., repoApi.clear()
or an injectable getRepoApi function) so tests can control lifecycle and the
component can obtain a context-scoped repo API rather than relying on the
module-level singleton.
- Around line 213-218: In renderUserPlugins, replace the confusing allDeleted
calculation based on find with a clearer Array.every check: compute allDeleted
as pluginItems.every(it => it.deleted === true) (or pluginItems.every(it =>
!!it.deleted)) so the intent is obvious; keep the existing branching that
returns the empty-state when !pluginItems.length || allDeleted unchanged and
reference the allDeleted variable and pluginItems array when making the
replacement.
In
`@packages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js`:
- Around line 7-10: ToggleAdvancedSettingsButton uses a static title "Show
Advanced Settings" regardless of props.advancedSettingsVisible; update the title
to be state-aware by switching the localized string based on
props.advancedSettingsVisible (e.g., use _('Show Advanced Settings') when false
and _('Hide Advanced Settings') when true) so the Button_1.default title
reflects the action; modify the title prop in the ToggleAdvancedSettingsButton
return to compute the correct string from props.advancedSettingsVisible.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.js`:
- Around line 143-144: The local variable isFiltered is computed but never used;
remove the unused declaration (the const isFiltered = ... line) to avoid dead
code in Sidebar.js, or if the intended logic was to use it in the buttonStyle
decision, replace usages of hasMatches in that block with the correct
isFiltered-based expression (using section.name, props.filteredSections and
props.searchQuery) so the buttonStyle reflects the intended filtered state.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 83-128: The styles in StyledSearchContainer, StyledSearchInput,
and StyledClearButton use hard-coded tokens (8px, 6px 12px, 4px, 0.7) — move
these into the shared theme and consume them from props.theme instead; add new
token names (e.g. searchPadding, searchInputPadding, searchBorderRadius,
searchActiveOpacity or similar) to the shared theme interface and theme objects,
then replace the hard-coded values in those styled components to use (props:
StyleProps) => props.theme.<tokenName> so the sidebar uses centralized theme
tokens.
🪄 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: e088694a-4353-4414-88c7-1d511af7dd7f
⛔ Files ignored due to path filters (14)
packages/app-desktop/gui/ConfigScreen/ButtonBar.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/ConfigScreen.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/Sidebar.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/FontSearch.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/HighlightedText.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/MissingPasswordHelpLink.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/SettingDescription.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/SettingHeader.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/SettingLabel.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.js.mapis excluded by!**/*.map
📒 Files selected for processing (21)
.eslintignore.gitignoregitpackages/app-desktop/gui/ConfigScreen/ButtonBar.jspackages/app-desktop/gui/ConfigScreen/ConfigScreen.jspackages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.tspackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.jspackages/app-desktop/gui/ConfigScreen/Sidebar.tsxpackages/app-desktop/gui/ConfigScreen/controls/FontSearch.jspackages/app-desktop/gui/ConfigScreen/controls/HighlightedText.jspackages/app-desktop/gui/ConfigScreen/controls/HighlightedText.tsxpackages/app-desktop/gui/ConfigScreen/controls/MissingPasswordHelpLink.jspackages/app-desktop/gui/ConfigScreen/controls/SettingComponent.jspackages/app-desktop/gui/ConfigScreen/controls/SettingDescription.jspackages/app-desktop/gui/ConfigScreen/controls/SettingHeader.jspackages/app-desktop/gui/ConfigScreen/controls/SettingLabel.jspackages/app-desktop/gui/ConfigScreen/controls/ToggleAdvancedSettingsButton.jspackages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.jspackages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.jspackages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.js
Comment on lines
+19
to
+28
| instance.state = { | ||
| searchQuery: '', | ||
| searching: false, | ||
| filteredSections: new Set<string>(), | ||
| selectedSectionName: 'general', | ||
| screenName: '', | ||
| changedSettingKeys: [], | ||
| needRestart: false, | ||
| fonts: [], | ||
| }; |
Contributor
There was a problem hiding this comment.
These tests still target the old helper-based search implementation.
ConfigScreen.tsx no longer exposes state.filteredSections or getFilteredSections(), so this suite is asserting against an API that is gone from the source under review. Rework the tests around the current render-time filtering behaviour instead of the removed helper state.
Also applies to: 67-217
🤖 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 - 28, The tests reference removed internals (state.filteredSections and
getFilteredSections) of ConfigScreen; update them to assert rendered output
instead: set the component's searchQuery via the ConfigScreen props or simulate
typing into the search input (targeting the ConfigScreen component and its
search input), then verify which section headers and setting rows are
present/hidden in the DOM rather than inspecting state.filteredSections or
calling getFilteredSections; replace any assertions referencing
state.filteredSections/getFilteredSections with queries for elements by text,
role, or test-id corresponding to the section names and setting labels rendered
by ConfigScreen so the suite tests render-time filtering behavior.
Comment on lines
+453
to
+467
| // Filter sections based on search query | ||
| let filteredSections = sections; | ||
| if (this.state.searchQuery) { | ||
| filteredSections = sections.filter(section => { | ||
| // Check if section name matches | ||
| if (this.matchesSearchQuery(section.name)) return true; | ||
| // Check if any metadata (label or description) matches | ||
| for (const md of section.metadatas) { | ||
| if (this.matchesSearchQuery(md.label) || this.matchesSearchQuery(md.description)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't remove non-matching sections from the tab model.
Pre-filtering sections here lets the active tab disappear from both the sidebar and the rendered tabpanels as soon as the query stops matching it, so the content area loses its current panel until the user reselects something. It also prevents the reduced-opacity “no match” state because Sidebar no longer sees the full list, and section.name misses the visible/localised section label. Keep the full section list, track matches separately, and compare section names via Setting.sectionNameToLabel(section.name).
Also applies to: 481-517
🤖 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 453 -
467, Don't drop non-matching sections from the tab model: keep the original
sections array intact and stop assigning a pre-filtered list to the UI model
(avoid replacing sections with filteredSections); instead compute a separate
boolean/map of match results for each section (use
Setting.sectionNameToLabel(section.name) when comparing names) and use that to
drive "matched" state and reduced-opacity/no-match display in Sidebar and
tabpanels; update the code around filteredSections and any uses in rendering
(including the tabpanel rendering logic) to consult the match map (and continue
using this.matchesSearchQuery for metadata checks) so the active tab remains
present even when it doesn't match the search.
Comment on lines
+12
to
+23
| const isLoadingFonts = filteredAvailableFonts.length === 0; | ||
| (0, react_1.useEffect)(() => { | ||
| if (subtype === Setting_1.SettingItemSubType.MonospaceFontFamily) { | ||
| setIsMonoBoxChecked(true); | ||
| } | ||
| }, [subtype]); | ||
| (0, react_1.useEffect)(() => { | ||
| if (!isMonoBoxChecked) | ||
| return setFilteredAvailableFonts(availableFonts); | ||
| const localMonospacedFonts = availableFonts.filter((font) => monospaceKeywords.some((word) => font.toLowerCase().includes(word)) || | ||
| knownMonospacedFonts.includes(font.toLowerCase())); | ||
| setFilteredAvailableFonts(localMonospacedFonts); |
Contributor
There was a problem hiding this comment.
Don't show “Loading...” when the filter just found no monospace fonts.
isLoadingFonts is derived from filteredAvailableFonts.length === 0, so an empty monospace match set is treated the same as “fonts are still loading”. Base the loading state on availableFonts (or an explicit loaded flag) and leave the empty-filter case to the combobox.
Also applies to: 28-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/controls/FontSearch.js` around lines 12
- 23, The loading state is incorrectly derived from filteredAvailableFonts, so
an empty monospace filter shows "Loading..."; change isLoadingFonts to depend on
whether font data has loaded (e.g., availableFonts.length === 0 or an explicit
loaded flag) instead of filteredAvailableFonts.length, and keep
filteredAvailableFonts empty when no monospace matches; update the declaration
of isLoadingFonts and any related checks that reference it (symbols:
isLoadingFonts, filteredAvailableFonts, availableFonts, isMonoBoxChecked,
setFilteredAvailableFonts) so the combobox can show an empty result rather than
a loading indicator.
packages/app-desktop/gui/ConfigScreen/controls/HighlightedText.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+166
to
+173
| let title = (0, locale_1._)('Update'); | ||
| if (props.updateState === UpdateState.Updating) | ||
| title = (0, locale_1._)('Updating...'); | ||
| if (props.updateState === UpdateState.Idle) | ||
| title = (0, locale_1._)('Updated'); | ||
| if (props.updateState === UpdateState.HasBeenUpdated) | ||
| title = (0, locale_1._)('Updated'); | ||
| return React.createElement(Button_1.default, { ml: 1, level: Button_1.ButtonLevel.Recommended, onClick: () => props.onUpdate({ item }), title: title, disabled: props.updateState === UpdateState.HasBeenUpdated }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the source TypeScript file for PluginBox
fd -e tsx -e ts "PluginBox" --exec grep -l "UpdateState" {}
# Check how UpdateState.Idle is used elsewhere in the codebase
rg -n "UpdateState\.Idle" --type ts --type tsx -C 2Repository: laurent22/joplin
Length of output: 260
🏁 Script executed:
#!/bin/bash
# Read the source PluginBox.tsx file to see the actual logic
echo "=== PluginBox.tsx source code ==="
cat -n ./packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.tsx | head -250
# Find UpdateState enum definition
echo -e "\n=== Searching for UpdateState enum definition ==="
rg -n "enum UpdateState|type UpdateState" --max-count 5
# Check how UpdateState.Idle is used in the codebase
echo -e "\n=== UpdateState.Idle usage ==="
rg -n "UpdateState\.Idle" -A 2 -B 2
# Check other UpdateState values and their context
echo -e "\n=== All UpdateState enum values ==="
rg -n "UpdateState\.\w+" -oRepository: laurent22/joplin
Length of output: 13580
🏁 Script executed:
#!/bin/bash
# Check PluginsStates.tsx to understand state flow
echo "=== PluginsStates.tsx update state logic ==="
sed -n '250,270p' packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.tsx
# Check mobile version for comparison
echo -e "\n=== PluginInfoModal.tsx mobile button handling ==="
rg -n "updateState|UpdateState\." packages/app-mobile/components/screens/ConfigScreen/plugins/PluginInfoModal.tsx -A 2 -B 1 | head -80
# Check if there's any UI/UX documentation or intent
echo -e "\n=== Search for update button intent ==="
rg -n "CanUpdate" packages/app-desktop/gui/ConfigScreen/controls/plugins/ -A 2 -B 2Repository: laurent22/joplin
Length of output: 3892
Fix update button state handling for UpdateState.Idle and UpdateState.Updating.
The update button displays incorrect text and does not disable properly for non-actionable states. When updateState is Idle (no update available), the button incorrectly shows "Updated" text and remains enabled. Additionally, the button should be disabled during Updating state when no further user action is possible.
Currently, only HasBeenUpdated disables the button. The button should be enabled exclusively when updateState === UpdateState.CanUpdate, matching the mobile app implementation which correctly hides the button for Idle and disables it for all states except CanUpdate.
Proposed fix
function renderUpdateButton() {
if (!props.onUpdate) return null;
let title = _('Update');
if (props.updateState === UpdateState.Updating) title = _('Updating...');
- if (props.updateState === UpdateState.Idle) title = _('Updated');
if (props.updateState === UpdateState.HasBeenUpdated) title = _('Updated');
return <Button
ml={1}
level={ButtonLevel.Recommended}
onClick={() => props.onUpdate({ item })}
title={title}
- disabled={props.updateState === UpdateState.HasBeenUpdated}
+ disabled={props.updateState !== UpdateState.CanUpdate}
/>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let title = (0, locale_1._)('Update'); | |
| if (props.updateState === UpdateState.Updating) | |
| title = (0, locale_1._)('Updating...'); | |
| if (props.updateState === UpdateState.Idle) | |
| title = (0, locale_1._)('Updated'); | |
| if (props.updateState === UpdateState.HasBeenUpdated) | |
| title = (0, locale_1._)('Updated'); | |
| return React.createElement(Button_1.default, { ml: 1, level: Button_1.ButtonLevel.Recommended, onClick: () => props.onUpdate({ item }), title: title, disabled: props.updateState === UpdateState.HasBeenUpdated }); | |
| let title = (0, locale_1._)('Update'); | |
| if (props.updateState === UpdateState.Updating) | |
| title = (0, locale_1._)('Updating...'); | |
| if (props.updateState === UpdateState.HasBeenUpdated) | |
| title = (0, locale_1._)('Updated'); | |
| return React.createElement(Button_1.default, { ml: 1, level: Button_1.ButtonLevel.Recommended, onClick: () => props.onUpdate({ item }), title: title, disabled: props.updateState !== UpdateState.CanUpdate }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginBox.js` around
lines 166 - 173, The Update button's title mapping and disabled logic are wrong:
change the title handling in PluginBox (the title variable set from
props.updateState) so UpdateState.CanUpdate => _('Update'), UpdateState.Updating
=> _('Updating...'), UpdateState.Idle => _('Update') (or a neutral label) and
UpdateState.HasBeenUpdated => _('Updated'); then make the button disabled
whenever props.updateState !== UpdateState.CanUpdate (replace the current
disabled check that only tests HasBeenUpdated) so the button is enabled
exclusively for UpdateState.CanUpdate; keep the onClick (props.onUpdate({ item
})) as-is.
Comment on lines
+149
to
+162
| const onInstall = (0, react_1.useCallback)(async () => { | ||
| const result = await (0, bridge_1.default)().showOpenDialog({ | ||
| filters: [{ name: 'Joplin Plugin Archive', extensions: ['jpl'] }], | ||
| }); | ||
| const filePath = result && result.length ? result[0] : null; | ||
| if (!filePath) | ||
| return; | ||
| const plugin = await pluginService.installPlugin(filePath); | ||
| const newSettings = (0, immer_1.produce)(pluginSettings, (draft) => { | ||
| draft[plugin.manifest.id] = (0, PluginService_1.defaultPluginSetting)(); | ||
| }); | ||
| props.onChange({ value: pluginService.serializePluginSettings(newSettings) }); | ||
| // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied | ||
| }, [pluginSettings, props.onChange]); |
Contributor
There was a problem hiding this comment.
Missing error handling in onInstall callback.
The onInstall callback calls pluginService.installPlugin(filePath) without a try-catch. According to the relevant code snippet from PluginService.ts, installPlugin can throw errors during loadPluginFromPath or file system operations. An unhandled rejection could leave the UI in an inconsistent state.
Proposed fix (apply to source .ts file)
const onInstall = (0, react_1.useCallback)(async () => {
const result = await (0, bridge_1.default)().showOpenDialog({
filters: [{ name: 'Joplin Plugin Archive', extensions: ['jpl'] }],
});
const filePath = result && result.length ? result[0] : null;
if (!filePath)
return;
- const plugin = await pluginService.installPlugin(filePath);
- const newSettings = (0, immer_1.produce)(pluginSettings, (draft) => {
- draft[plugin.manifest.id] = (0, PluginService_1.defaultPluginSetting)();
- });
- props.onChange({ value: pluginService.serializePluginSettings(newSettings) });
+ try {
+ const plugin = await pluginService.installPlugin(filePath);
+ const newSettings = (0, immer_1.produce)(pluginSettings, (draft) => {
+ draft[plugin.manifest.id] = (0, PluginService_1.defaultPluginSetting)();
+ });
+ props.onChange({ value: pluginService.serializePluginSettings(newSettings) });
+ } catch (error) {
+ logger.error('Failed to install plugin:', error);
+ // Consider showing user-facing error notification
+ }
}, [pluginSettings, props.onChange]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const onInstall = (0, react_1.useCallback)(async () => { | |
| const result = await (0, bridge_1.default)().showOpenDialog({ | |
| filters: [{ name: 'Joplin Plugin Archive', extensions: ['jpl'] }], | |
| }); | |
| const filePath = result && result.length ? result[0] : null; | |
| if (!filePath) | |
| return; | |
| const plugin = await pluginService.installPlugin(filePath); | |
| const newSettings = (0, immer_1.produce)(pluginSettings, (draft) => { | |
| draft[plugin.manifest.id] = (0, PluginService_1.defaultPluginSetting)(); | |
| }); | |
| props.onChange({ value: pluginService.serializePluginSettings(newSettings) }); | |
| // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied | |
| }, [pluginSettings, props.onChange]); | |
| const onInstall = (0, react_1.useCallback)(async () => { | |
| const result = await (0, bridge_1.default)().showOpenDialog({ | |
| filters: [{ name: 'Joplin Plugin Archive', extensions: ['jpl'] }], | |
| }); | |
| const filePath = result && result.length ? result[0] : null; | |
| if (!filePath) | |
| return; | |
| try { | |
| const plugin = await pluginService.installPlugin(filePath); | |
| const newSettings = (0, immer_1.produce)(pluginSettings, (draft) => { | |
| draft[plugin.manifest.id] = (0, PluginService_1.defaultPluginSetting)(); | |
| }); | |
| props.onChange({ value: pluginService.serializePluginSettings(newSettings) }); | |
| } catch (error) { | |
| logger.error('Failed to install plugin:', error); | |
| // Consider showing user-facing error notification | |
| } | |
| // eslint-disable-next-line `@seiyab/react-hooks/exhaustive-deps` -- Old code before rule was applied | |
| }, [pluginSettings, props.onChange]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/PluginsStates.js`
around lines 149 - 162, Wrap the async body of the onInstall callback in a
try-catch: when calling pluginService.installPlugin(filePath) and subsequent
code that updates pluginSettings and calls props.onChange, catch any thrown
error, log it (console.error or existing logger) and surface a user-friendly
error (e.g., show a dialog or set an error state) then return early so
pluginSettings/props.onChange are not mutated on failure; keep references to
onInstall, pluginService.installPlugin, pluginSettings, props.onChange,
pluginService.serializePluginSettings, and PluginService_1.defaultPluginSetting
to locate and protect the code paths.
Comment on lines
+30
to
+44
| (0, react_1.useEffect)(() => { | ||
| setSearchResultCount(null); | ||
| asyncSearchQueue.current.push(async () => { | ||
| if (!props.searchQuery) { | ||
| setManifests([]); | ||
| setSearchResultCount(null); | ||
| } | ||
| else { | ||
| const r = await props.repoApi().search(props.searchQuery); | ||
| setManifests(r); | ||
| setSearchResultCount(r.length); | ||
| } | ||
| }); | ||
| // eslint-disable-next-line @seiyab/react-hooks/exhaustive-deps -- Old code before rule was applied | ||
| }, [props.searchQuery]); |
Contributor
There was a problem hiding this comment.
Ignore stale plugin-search responses.
Every query change enqueues another async search, but the resolving callback never checks that its result still belongs to the latest props.searchQuery. With a queue concurrency of 10, an older slower search can overwrite the newer results. Capture the query in the effect and discard responses once the input has moved on.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/controls/plugins/SearchPlugins.js`
around lines 30 - 44, The async search enqueued in useEffect can return
out-of-order and overwrite newer results; capture the current props.searchQuery
into a local const (e.g. const query = props.searchQuery) inside the effect
before pushing to asyncSearchQueue.current, then inside the enqueued async
callback compare that captured query to the latest props.searchQuery (or a ref
holding latest query) after the await props.repoApi().search(...) and before
calling setManifests/setSearchResultCount; if they differ, discard the response
to avoid stale updates. Use the existing asyncSearchQueue, setManifests, and
setSearchResultCount symbols and ensure the check runs both when results are
empty and when populated.
Comment on lines
+73
to
+90
| exports.StyledSearchInput = styled.input ` | ||
| flex: 1; | ||
| padding: 8px; | ||
| border: 1px solid ${(props) => props.theme.dividerColor}; | ||
| border-radius: 4px; | ||
| background-color: ${(props) => props.theme.backgroundColor}; | ||
| color: ${(props) => props.theme.color2}; | ||
| font-size: ${(props) => Math.round(props.theme.fontSize)}px; | ||
|
|
||
| &:focus { | ||
| outline: none; | ||
| border-color: ${(props) => props.theme.colorFaded}; | ||
| } | ||
|
|
||
| &::placeholder { | ||
| color: ${(props) => props.theme.colorFaded}; | ||
| } | ||
| `; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "theme.ts" -o -name "theme.js" | head -20Repository: laurent22/joplin
Length of output: 158
🏁 Script executed:
cat packages/lib/theme.ts | head -100Repository: laurent22/joplin
Length of output: 2657
🏁 Script executed:
rg "color2" packages/lib/theme.ts -A 2 -B 2Repository: laurent22/joplin
Length of output: 822
🏁 Script executed:
rg "backgroundColor" packages/lib/theme.ts -A 2 -B 2Repository: laurent22/joplin
Length of output: 2493
🏁 Script executed:
rg "StyledSearchInput\|color2.*backgroundColor" packages/app-desktop -A 2 -B 2Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
cat packages/lib/themes/light.ts | head -80Repository: laurent22/joplin
Length of output: 1954
🏁 Script executed:
cat packages/lib/themes/dark.ts | head -80Repository: laurent22/joplin
Length of output: 1874
🏁 Script executed:
rg "color2\|backgroundColor" packages/lib/themes/light.ts | head -30Repository: laurent22/joplin
Length of output: 42
🏁 Script executed:
grep -n "color\|backgroundColor" packages/lib/themes/light.ts | head -40Repository: laurent22/joplin
Length of output: 752
🏁 Script executed:
cat packages/app-desktop/gui/ConfigScreen/Sidebar.js | head -150Repository: laurent22/joplin
Length of output: 6079
🏁 Script executed:
rg "styled\." packages/app-desktop/gui/ConfigScreen/Sidebar.js -A 5Repository: laurent22/joplin
Length of output: 1740
Fix colour contrast issue in search input.
The StyledSearchInput uses theme.color2 for text on theme.backgroundColor, which creates a severe contrast problem in the light theme (white text on white background). The color2 colour is explicitly designed to pair with backgroundColor2 for sidebar use. Other sidebar elements in this file correctly use color2, but the search input should match the text–background pairing used throughout the main content area.
Change to use theme.color instead:
Suggested fix
exports.StyledSearchInput = styled.input `
flex: 1;
padding: 8px;
border: 1px solid ${(props) => props.theme.dividerColor};
border-radius: 4px;
background-color: ${(props) => props.theme.backgroundColor};
- color: ${(props) => props.theme.color2};
+ color: ${(props) => props.theme.color};
font-size: ${(props) => Math.round(props.theme.fontSize)}px;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| exports.StyledSearchInput = styled.input ` | |
| flex: 1; | |
| padding: 8px; | |
| border: 1px solid ${(props) => props.theme.dividerColor}; | |
| border-radius: 4px; | |
| background-color: ${(props) => props.theme.backgroundColor}; | |
| color: ${(props) => props.theme.color2}; | |
| font-size: ${(props) => Math.round(props.theme.fontSize)}px; | |
| &:focus { | |
| outline: none; | |
| border-color: ${(props) => props.theme.colorFaded}; | |
| } | |
| &::placeholder { | |
| color: ${(props) => props.theme.colorFaded}; | |
| } | |
| `; | |
| exports.StyledSearchInput = styled.input ` | |
| flex: 1; | |
| padding: 8px; | |
| border: 1px solid ${(props) => props.theme.dividerColor}; | |
| border-radius: 4px; | |
| background-color: ${(props) => props.theme.backgroundColor}; | |
| color: ${(props) => props.theme.color}; | |
| font-size: ${(props) => Math.round(props.theme.fontSize)}px; | |
| &:focus { | |
| outline: none; | |
| border-color: ${(props) => props.theme.colorFaded}; | |
| } | |
| &::placeholder { | |
| color: ${(props) => props.theme.colorFaded}; | |
| } | |
| `; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.js` around lines 73 - 90, The
search input StyledSearchInput is using theme.color2 over theme.backgroundColor
causing poor contrast in light theme; update StyledSearchInput to use
theme.color for the input text color (replace uses of props.theme.color2 with
props.theme.color) so the input text pairs correctly with theme.backgroundColor
like other main content elements; ensure this change is applied only to the
color property inside StyledSearchInput and keep all other styling (placeholder
color, focus border, etc.) unchanged.
- Added private getFilteredSections method to ConfigScreenComponent - Updated render method to use the new method - Fixed all test type annotations with MockSection and MockMetadata interfaces - Mocked setState in tests for proper state updates - Updated tests to use toStrictEqual for array comparisons - Added proper eslint-disable-next-line comments for any type usage - All 21 tests now passing without TypeScript errors
- Added React.ChangeEvent<HTMLInputElement> type annotation for search input onChange parameter - Fixes TS7006 error during build process - Resolves CI workflow failure
… and trailing comma)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (2)
225-244:⚠️ Potential issue | 🟠 MajorKeep the search controls outside the
tablist.The input and clear button are rendered as children of the element with
role='tablist', so the tablist no longer contains only tabs. That breaks the settings navigator's accessibility structure. Render the search container above the tab-only wrapper instead.🤖 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 225 - 244, The search input and clear button are currently rendered inside the element with role='tablist' (StyledRoot), which violates the tablist-only-tabs accessibility rule; move the search container (StyledSearchContainer and its children StyledSearchInput and StyledClearButton using props.searchQuery, props.onSearchQueryChange, props.onClearSearch) out of the StyledRoot that has role='tablist' so the tablist contains only tab elements—render the search block immediately above (or outside) StyledRoot while preserving the same conditional rendering logic and event handlers.
92-99:⚠️ Potential issue | 🟠 MajorUse the main-surface text token for the search field.
StyledSearchInputpairstheme.color2withtheme.backgroundColor. Those tokens belong to different surfaces, and in the light theme this makes the input text effectively unreadable. Switch the text colour totheme.color.Based on learnings, "For GUI style changes in Desktop and mobile clients, refer to `packages/lib/theme.ts` for all styling information".Possible fix
export const StyledSearchInput = styled.input` flex: 1; padding: 8px; border: 1px solid ${(props: StyleProps) => props.theme.dividerColor}; border-radius: 4px; background-color: ${(props: StyleProps) => props.theme.backgroundColor}; - color: ${(props: StyleProps) => props.theme.color2}; + color: ${(props: StyleProps) => props.theme.color}; font-size: ${(props: StyleProps) => Math.round(props.theme.fontSize)}px;🤖 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 92 - 99, StyledSearchInput uses theme.color2 which is for a different surface and causes low contrast; update StyledSearchInput (the styled input component) to use the main-surface text token (props.theme.color) instead of props.theme.color2 so the search field text matches the primary surface token defined in packages/lib/theme.ts and remains readable across themes.
🤖 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.js`:
- Around line 333-362: When filtering removes the currently selected section,
set a fallback selection from filteredSections and keep screenName in sync so
the right-hand pane always shows a visible panel. Specifically, after computing
filteredSections use something like selected = filteredSections.find(s => s.name
=== this.state.selectedSectionName) || filteredSections[0]; then update
this.state.selectedSectionName (or call the existing selection handler
sidebar_selectionChange) and also set this.state.screenName to null or to the
matching screen for that section so screenComp stays in sync; ensure this logic
runs before building tabComponents and handles the empty filteredSections case
(clear selection/screenName).
- Around line 84-87: The filter currently only checks section.name inside the
sections.filter, causing visible headings to be missed; update the match to also
test the section's displayed label (e.g., use the rendered/display label
property or a helper like getSectionLabel) so that this.matchesSearchQuery runs
against both the internal key and the user-facing label (for example:
this.matchesSearchQuery(section.name) ||
this.matchesSearchQuery(getSectionLabel(section)) or fallback to section.label
|| section.name). Modify the filter logic in ConfigScreen (the block using
sections.filter and matchesSearchQuery) to include that label check to support
localized and displayed headings.
---
Duplicate comments:
In `@packages/app-desktop/gui/ConfigScreen/Sidebar.tsx`:
- Around line 225-244: The search input and clear button are currently rendered
inside the element with role='tablist' (StyledRoot), which violates the
tablist-only-tabs accessibility rule; move the search container
(StyledSearchContainer and its children StyledSearchInput and StyledClearButton
using props.searchQuery, props.onSearchQueryChange, props.onClearSearch) out of
the StyledRoot that has role='tablist' so the tablist contains only tab
elements—render the search block immediately above (or outside) StyledRoot while
preserving the same conditional rendering logic and event handlers.
- Around line 92-99: StyledSearchInput uses theme.color2 which is for a
different surface and causes low contrast; update StyledSearchInput (the styled
input component) to use the main-surface text token (props.theme.color) instead
of props.theme.color2 so the search field text matches the primary surface token
defined in packages/lib/theme.ts and remains readable across themes.
🪄 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: 5355480e-e275-44f4-886a-f2a47599db45
⛔ Files ignored due to path filters (2)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.js.mapis excluded by!**/*.mappackages/app-desktop/gui/ConfigScreen/Sidebar.js.mapis excluded by!**/*.map
📒 Files selected for processing (5)
packages/app-desktop/gui/ConfigScreen/ConfigScreen.jspackages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.tspackages/app-desktop/gui/ConfigScreen/ConfigScreen.tsxpackages/app-desktop/gui/ConfigScreen/Sidebar.jspackages/app-desktop/gui/ConfigScreen/Sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx
Comment on lines
+84
to
+87
| return sections.filter(section => { | ||
| // Check if section name matches | ||
| if (this.matchesSearchQuery(section.name)) | ||
| return true; |
Contributor
There was a problem hiding this comment.
Match against the rendered section label as well as the internal key.
This branch only checks section.name, so visible headings such as Note History or Spell Checker can miss even when the user types the exact label they see. It also makes section-name search brittle in localised UIs. Include the displayed section label in the section-level match.
Possible fix
return sections.filter(section => {
- // Check if section name matches
- if (this.matchesSearchQuery(section.name))
+ const sectionLabel = Setting_1.default.sectionNameToLabel(section.name);
+ if (this.matchesSearchQuery(section.name) || this.matchesSearchQuery(sectionLabel))
return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.js` around lines 84 - 87,
The filter currently only checks section.name inside the sections.filter,
causing visible headings to be missed; update the match to also test the
section's displayed label (e.g., use the rendered/display label property or a
helper like getSectionLabel) so that this.matchesSearchQuery runs against both
the internal key and the user-facing label (for example:
this.matchesSearchQuery(section.name) ||
this.matchesSearchQuery(getSectionLabel(section)) or fallback to section.label
|| section.name). Modify the filter logic in ConfigScreen (the block using
sections.filter and matchesSearchQuery) to include that label check to support
localized and displayed headings.
Comment on lines
+333
to
+362
| const settingComps = shared.settingsToComponents2(this, Setting_1.AppType.Desktop, settings, this.state.selectedSectionName); | ||
| // screenComp is a custom config screen, such as the encryption config screen or keymap config screen. | ||
| // 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 ? React.createElement("div", { className: "config-screen-content-wrapper", style: { overflow: 'scroll', flex: 1 } }, this.screenFromName(this.state.screenName)) : null; | ||
| if (screenComp) | ||
| containerStyle.display = 'none'; | ||
| const sections = shared.settingsSections({ device: Setting_1.AppType.Desktop, settings }); | ||
| // Filter sections based on search query | ||
| const filteredSections = this.getFilteredSections(sections); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied | ||
| const needRestartComp = this.state.needRestart ? (React.createElement("div", { style: Object.assign(Object.assign({}, theme.textStyle), { padding: 10, paddingLeft: 24, backgroundColor: theme.warningBackgroundColor, color: theme.color }) }, | ||
| this.restartMessage(), | ||
| React.createElement("a", { style: Object.assign(Object.assign({}, theme.urlStyle), { marginLeft: 10 }), href: "#", onClick: () => { void this.restartApp(); } }, (0, locale_1._)('Restart now')))) : null; | ||
| const rightStyle = Object.assign(Object.assign({}, style), { flex: 1 }); | ||
| delete style.width; | ||
| const tabComponents = []; | ||
| for (const section of filteredSections) { | ||
| const sectionId = `setting-section-${section.name}`; | ||
| let content = null; | ||
| const visible = section.name === this.state.selectedSectionName; | ||
| if (visible) { | ||
| content = (React.createElement(React.Fragment, null, | ||
| screenComp, | ||
| React.createElement("div", { style: containerStyle }, settingComps))); | ||
| } | ||
| tabComponents.push(React.createElement("div", { key: sectionId, id: sectionId, className: `setting-tab-panel ${!visible ? '-hidden' : ''}`, hidden: !visible, "aria-labelledby": `setting-tab-${section.name}`, tabIndex: 0, role: 'tabpanel' }, content)); | ||
| } | ||
| return (React.createElement("div", { className: "config-screen", role: "main", style: { display: 'flex', flexDirection: 'row', height: this.props.style.height } }, | ||
| React.createElement(Sidebar_1.default, { selection: this.state.selectedSectionName, onSelectionChange: this.sidebar_selectionChange, sections: filteredSections, searchQuery: this.state.searchQuery, onSearchQueryChange: this.setSearchQuery, onClearSearch: this.clearSearch }), |
Contributor
There was a problem hiding this comment.
Fallback to a visible section while filtering.
When the query filters out this.state.selectedSectionName, the sidebar renders no selected tab and the right-hand pane renders no visible panel. Keyboard users also lose the only tabbable section until they click a result. Please pick a visible section whenever the current one drops out of filteredSections, and keep screenName in sync with that fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/app-desktop/gui/ConfigScreen/ConfigScreen.js` around lines 333 -
362, When filtering removes the currently selected section, set a fallback
selection from filteredSections and keep screenName in sync so the right-hand
pane always shows a visible panel. Specifically, after computing
filteredSections use something like selected = filteredSections.find(s => s.name
=== this.state.selectedSectionName) || filteredSections[0]; then update
this.state.selectedSectionName (or call the existing selection handler
sidebar_selectionChange) and also set this.state.screenName to null or to the
matching screen for that section so screenComp stays in sync; ensure this logic
runs before building tabComponents and handles the empty filteredSections case
(clear selection/screenName).
…lity violation The search container is now a sibling of the tablist rather than a child, ensuring the tablist only contains tab elements as required by ARIA specifications. This resolves the aria-required-children violation in integration tests.
coderabbitai
bot
added
enhancement
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx (1)
109-111: Consider preserving a visible focus indicator.Removing
outline: noneand relying solely onborder-colorchange may not provide sufficient contrast for all users or themes. A subtlebox-shadowfocus ring is often a more accessible alternative.♻️ Optional enhancement for better focus visibility
&:focus { - outline: none; + outline: 2px solid ${(props: StyleProps) => props.theme.colorFaded}; + outline-offset: -2px; border-color: ${(props: StyleProps) => props.theme.colorFaded}; }🤖 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 109 - 111, In the &:focus rule inside Sidebar.tsx (the styled component using StyleProps), restore a visible focus indicator instead of keeping outline: none; remove outline: none and add an accessible focus ring such as a subtle box-shadow (or an outline fallback) that uses props.theme.colorFaded for color contrast so keyboard focus remains visible across themes; update the &:focus block for the relevant styled component to combine border-color change with a box-shadow-based focus ring.
🤖 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/Sidebar.tsx`:
- Around line 109-111: In the &:focus rule inside Sidebar.tsx (the styled
component using StyleProps), restore a visible focus indicator instead of
keeping outline: none; remove outline: none and add an accessible focus ring
such as a subtle box-shadow (or an outline fallback) that uses
props.theme.colorFaded for color contrast so keyboard focus remains visible
across themes; update the &:focus block for the relevant styled component to
combine border-color change with a box-shadow-based focus ring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd56bab6-0f00-4741-bae3-d71497ea7bdd
📒 Files selected for processing (1)
packages/app-desktop/gui/ConfigScreen/Sidebar.tsx
Author
|
@laurent22 can you review the changes made and if they are up top the mark according to the specifications. |
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
1 participant
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.
Description
This pull request implements search functionality for the Joplin Desktop settings screen, allowing users to quickly find settings by searching across labels, descriptions, and section names.
Features Implemented
Implementation Details
Files Modified:
packages/app-desktop/gui/ConfigScreen/ConfigScreen.tsx- Added search state management and filtering logicpackages/app-desktop/gui/ConfigScreen/Sidebar.tsx- Added search UI with styled componentspackages/app-desktop/gui/ConfigScreen/controls/HighlightedText.tsx- New component for highlighting matching textpackages/app-desktop/gui/ConfigScreen/ConfigScreen.search.test.ts- Comprehensive Jest test suite (22 test cases)Testing
Automated Tests: 22 Jest test cases covering:
Build Status: ✅ Successfully compiled with 0 errors
Notes
AI Assistance Disclosure
Gemini 3.1 was used to refine this PR description and give various test ideas.
Screencast_20260327_153220.webm