frontend: settings search#4151
Conversation
|
@coderabbitai review! |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
shonsirsha
force-pushed
the
frontend-search-settings
branch
7 times, most recently
from
86fc55c to
5bf0fd2
Compare
|
@coderabbitai review! |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
frontends/web/src/routes/settings/bb02-settings.tsx (1)
40-40: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename
BB02Settingsprops typing to match theTPropsrule.
TWrapperPropsis used as a component props type forBB02Settings, which breaks the project convention. KeepTPropsfor one component and inline the other component’s intersection type to avoid introducing another props-type alias name.As per coding guidelines, "Component props types must be named `TProps` (prefixed with `T`)."♻️ Minimal fix
-type TWrapperProps = TCommonProps & TPagePropsWithSettingsTabs; - type TProps = TCommonProps; @@ -const BB02Settings = ({ deviceID, devices, hasAccounts }: TWrapperProps) => { +const BB02Settings = ({ deviceID, devices, hasAccounts }: TCommonProps & TPagePropsWithSettingsTabs) => {Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontends/web/src/routes/settings/bb02-settings.tsx` at line 40, The props type alias TWrapperProps used for the BB02Settings component violates the convention requiring component props to be named TProps; rename TWrapperProps to TProps for BB02Settings and replace the other component’s separate alias by inlining its intersection type (TCommonProps & TPagePropsWithSettingsTabs) where that component expects props, ensuring any other occurrence (see the second instance around lines ~52) is updated similarly so only one alias named TProps is used per component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontends/web/src/components/forms/search-input.module.css`:
- Around line 24-27: The CSS rule for the clear button currently targets only
img (.clearButton img) so SVG icon components won't get the 20px sizing; update
the selector for the .clearButton sizing rule to target both img and svg (or use
a group/:is() selector) so icon components rendered as <svg> receive the same
height/width styling—locate the .clearButton rule in search-input.module.css and
change its selector accordingly.
In `@frontends/web/src/routes/settings/components/settings-search-content.tsx`:
- Around line 67-90: The page group renders a title even when all its entries
yield non-navigable URLs; update the rendering in the SETTINGS_SEARCH_PAGE_ORDER
loop to first map/filter pageSearchResults using
getSettingsSearchResultURL(firstDeviceID) to produce a list of navigable items,
return null if that filtered list is empty, and then render the group (SubTitle
and SettingsItem) by iterating over the filtered items (use searchResult.id,
searchResult.title and navigate(url) as before) so no empty pageGroup or
orphaned title is rendered.
In `@frontends/web/src/routes/settings/mobile-settings.tsx`:
- Around line 44-46: WithSettingsTabs already renders its own Tabs, so the
mobile settings file is mounting Tabs twice which causes duplicate
TabWithVersionCheck/useLoad(getVersion) calls; remove the inner <Tabs /> usage
in frontends/web/src/routes/settings/mobile-settings.tsx (leave
<WithSettingsTabs devices={devices} hasAccounts={hasAccounts}> without a nested
<Tabs />) or refactor so WithSettingsTabs only renders children when explicitly
provided, ensuring TabWithVersionCheck and useLoad(getVersion) run exactly once.
In `@frontends/web/src/routes/settings/settings-search.test.ts`:
- Around line 130-140: The test assertions rely on ambient env flags provided by
'`@/utils/env`' so make them deterministic by mocking that module in this test;
for the "export logs" case mock the debug/build flag (e.g. isDebugBuild or
similar exported name) to false before calling
getItems()/filterSettingsSearchItems, and for the "screen lock" case mock the
mobile-app flag (e.g. isMobileApp or isNativeMobile) to false, using your test
runner's module-mocking API (jest.mock/vi.mock) at the top of
settings-search.test.ts so getItems() observes the mocked env values and the
expectations become runner-independent.
In `@frontends/web/src/routes/settings/use-settings-search.ts`:
- Around line 30-37: The code is incorrectly deriving the target device from
Object.keys(devices)[0] which depends on object key order; change it to use the
actual active/selected device ID (e.g., the settings or component prop that
represents the current device) instead of the first key. Replace uses of
deviceId/device/device === 'bitbox02'/bb02DeviceId with the activeDeviceId
(falling back to a safe default or undefined), and pass that activeDeviceId into
useLoad (i.e., useLoad(activeDeviceId ? () => getDeviceInfo(activeDeviceId) :
null, [activeDeviceId])) so deviceInfoResult and deviceInfo reflect the real
selected device. Ensure all references to devices and bb02DeviceId are updated
to use that active device variable.
---
Duplicate comments:
In `@frontends/web/src/routes/settings/bb02-settings.tsx`:
- Line 40: The props type alias TWrapperProps used for the BB02Settings
component violates the convention requiring component props to be named TProps;
rename TWrapperProps to TProps for BB02Settings and replace the other
component’s separate alias by inlining its intersection type (TCommonProps &
TPagePropsWithSettingsTabs) where that component expects props, ensuring any
other occurrence (see the second instance around lines ~52) is updated similarly
so only one alias named TProps is used per component.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff3e2423-fa3d-4793-8c02-73daa6b779b6
📒 Files selected for processing (26)
CHANGELOG.mdfrontends/web/src/components/forms/search-input.module.cssfrontends/web/src/components/forms/search-input.tsxfrontends/web/src/locales/en/app.jsonfrontends/web/src/routes/device/no-device-connected.tsxfrontends/web/src/routes/router.tsxfrontends/web/src/routes/settings/about.tsxfrontends/web/src/routes/settings/advanced-settings.tsxfrontends/web/src/routes/settings/bb02-settings.module.cssfrontends/web/src/routes/settings/bb02-settings.tsxfrontends/web/src/routes/settings/components/settings-content.module.cssfrontends/web/src/routes/settings/components/settings-content.test.tsxfrontends/web/src/routes/settings/components/settings-content.tsxfrontends/web/src/routes/settings/components/settings-search-content.module.cssfrontends/web/src/routes/settings/components/settings-search-content.test.tsxfrontends/web/src/routes/settings/components/settings-search-content.tsxfrontends/web/src/routes/settings/components/tabs.module.cssfrontends/web/src/routes/settings/components/tabs.tsxfrontends/web/src/routes/settings/general.module.cssfrontends/web/src/routes/settings/general.tsxfrontends/web/src/routes/settings/mobile-settings.tsxfrontends/web/src/routes/settings/settings-search.test.tsfrontends/web/src/routes/settings/settings-search.tsfrontends/web/src/routes/settings/use-settings-highlight.tsfrontends/web/src/routes/settings/use-settings-search.test.tsxfrontends/web/src/routes/settings/use-settings-search.ts
💤 Files with no reviewable changes (1)
- frontends/web/src/routes/settings/general.module.css
shonsirsha
force-pushed
the
frontend-search-settings
branch
from
5bf0fd2 to
aa9fc69
Compare
shonsirsha
force-pushed
the
frontend-search-settings
branch
from
aa9fc69 to
faf5219
Compare
| }; | ||
|
|
||
| [0, 100, 300].forEach(delay => { | ||
| window.setTimeout(scrollToHighlightedItem, delay); |
There was a problem hiding this comment.
please add a comment why the 0, 100 and 300 delay.
Also I am not sure where when and how the scrolling works, could you mention in the commit message how to test or what the intention is?
| sections, | ||
| }: TProps) => { | ||
| const highlightedItemID = useSettingsHighlight(); | ||
| const highlightedItemRef = useRef<HTMLDivElement | null>(null); |
There was a problem hiding this comment.
not sure what highlightedItemRef is used for, it seems to be forever null maybe leftover code, remove?
|
|
||
| return ( | ||
| <> | ||
| {sections.map(section => { |
There was a problem hiding this comment.
nit: outer fragment has no purpose.
| {sections.map(section => { | |
| return sections.map(section => { |
| const updateSearchTerm = (value: string) => { | ||
| setSearchTerm(value); | ||
| const nextSearchParams = new URLSearchParams(searchParams); | ||
| if (value.toLowerCase().trim()) { |
There was a problem hiding this comment.
| if (value.toLowerCase().trim()) { | |
| if (value.trim()) { |
|
|
||
| useEffect(() => { | ||
| setSearchTerm(searchTermFromParams); | ||
| }, [searchTermFromParams]); |
There was a problem hiding this comment.
The next function takes a value, sets setSearchTerm, but then uses searchParams for nextSearchParams. I find it a bit hard mentally to follow.
GPT comment on this effect:
That’s usually fine, but it creates dual sources of truth: React state / URL params
You could simplify by making URL params the sole source of truth and removing local state entirely.
I think with that this hook would become a bit simpler to follow.
| <Input ref={ref} type="text" {...inputProps}> | ||
| {String(inputProps.value ?? '').trim().length > 0 ? ( | ||
| <button | ||
| aria-label={clearButtonLabel} |
There was a problem hiding this comment.
nit: just hardcode aria-label={t('generic.close')} and remove the prop.
|
|
||
| type TProps = TClearProps | TSearchProps; | ||
|
|
||
| export const SearchInput = forwardRef<HTMLInputElement, TProps>((props, ref) => { |
There was a problem hiding this comment.
With this PR there are now different "variant's" so I checked the other 2 places where <SearchInput> is used.
Not directly related to this PR, but in the other 2 places 'ref' is used to scroll to the results, but in both places the original list is replaced by the actually found results. Sometimes it quickly starts scrolling but then immediately updates the list with the found results stopping the scrolling a spit second later.
The search in settings has scrolling but doesn't need this ref to do so.
So it looks to me that we could drop ref here and the scrolling in address list and account overview.
| clearButtonLabel={t('generic.close')} | ||
| onChange={event => updateSearchTerm(event.currentTarget.value)} | ||
| onClear={() => updateSearchTerm('')} | ||
| placeholder={t('generic.search')} |
There was a problem hiding this comment.
The placeholder of this input field is "Search…" (t('generic.search')), in account it is hardcoded to "Search transactions..." (no translation) and the one in address-list is "Search address" (t('addresses.searchPlaceholder')).
Not a blocker, but would be nice to translate the account search.
|
|
||
| const whenAccountsExist = ({ hasAccounts }: TSettingsSearchContext) => hasAccounts; | ||
| const whenDeviceExists = ({ deviceIDs }: TSettingsSearchContext) => deviceIDs.length > 0; | ||
| const whenDeviceSupportsBluetooth = ({ deviceInfo }: TSettingsSearchContext) => !!deviceInfo?.bluetooth; |
There was a problem hiding this comment.
It is not ideal to have to maintain the same logic for showing setting-items in two times, in search results (here) and for the actual settings.
Would it make sense to import these functions to use the code to conditionally render the settings items i.e. in bb02-settings etc.?
2 participants
Adds search to the settings area so users can find settings without manually scanning each tab.
Behaviour:
Technical notes: