frontend: settings search#4151
Draft
shonsirsha wants to merge 1 commit into
Draft
Conversation
Collaborator
Author
|
@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
6 times, most recently
from
b7a1f10 to
86fc55c
Compare
shonsirsha
force-pushed
the
frontend-search-settings
branch
from
86fc55c to
5bf0fd2
Compare
Collaborator
Author
|
@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
Comment on lines
+24
to
+27
| .clearButton img { | ||
| height: 20px; | ||
| width: 20px; | ||
| } |
There was a problem hiding this comment.
Target SVG icons in the clear-button sizing rule.
Line 24 only targets img, but this button renders icon components, so the 20px sizing rule can be skipped if they render as svg. Use a selector that covers both.
Proposed fix
-.clearButton img {
+.clearButton img,
+.clearButton svg {
height: 20px;
width: 20px;
}📝 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
| .clearButton img { | |
| height: 20px; | |
| width: 20px; | |
| } | |
| .clearButton img, | |
| .clearButton svg { | |
| height: 20px; | |
| width: 20px; | |
| } |
🤖 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/components/forms/search-input.module.css` around lines 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.
Comment on lines
+67
to
+90
| SETTINGS_SEARCH_PAGE_ORDER.map(page => { | ||
| const pageSearchResults = searchResultsByPage[page]; | ||
| if (pageSearchResults.length === 0) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <div className={styles.pageGroup} key={page}> | ||
| <SubTitle className={styles.pageTitle}>{pageTitles[page]}</SubTitle> | ||
| {pageSearchResults.map(searchResult => { | ||
| const url = getSettingsSearchResultURL(searchResult, firstDeviceID); | ||
| if (!url) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <SettingsItem | ||
| key={searchResult.id} | ||
| onClick={() => navigate(url)} | ||
| settingName={searchResult.title} | ||
| /> | ||
| ); | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
Skip rendering page groups when every result in that page is non-navigable.
A page group can render only the title with no items when all entries resolve to undefined URLs (e.g., device page without an available device ID). Filter navigable items first, and return null when none remain.
Suggested fix
SETTINGS_SEARCH_PAGE_ORDER.map(page => {
- const pageSearchResults = searchResultsByPage[page];
- if (pageSearchResults.length === 0) {
+ const pageSearchResults = searchResultsByPage[page];
+ const navigableResults = pageSearchResults
+ .map(searchResult => ({
+ searchResult,
+ url: getSettingsSearchResultURL(searchResult, firstDeviceID),
+ }))
+ .filter(
+ (item): item is { searchResult: TSettingsSearchItem; url: string } =>
+ item.url !== undefined,
+ );
+
+ if (navigableResults.length === 0) {
return null;
}
@@
- {pageSearchResults.map(searchResult => {
- const url = getSettingsSearchResultURL(searchResult, firstDeviceID);
- if (!url) {
- return null;
- }
-
+ {navigableResults.map(({ searchResult, url }) => {
return (
<SettingsItem
key={searchResult.id}
onClick={() => navigate(url)}
settingName={searchResult.title}
/>
);
})}🤖 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/components/settings-search-content.tsx`
around lines 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.
Comment on lines
+44
to
+46
| <WithSettingsTabs devices={devices} hasAccounts={hasAccounts}> | ||
| <Tabs devices={devices} hasAccounts={hasAccounts} /> | ||
| </WithSettingsTabs> |
There was a problem hiding this comment.
Avoid mounting Tabs twice in mobile settings.
Line 44-46 mounts a <Tabs /> child inside WithSettingsTabs, but WithSettingsTabs already mounts its own <Tabs />. When search is inactive, both trees mount and each runs TabWithVersionCheck/useLoad(getVersion), which doubles version fetch work.
Proposed fix
diff --git a/frontends/web/src/routes/settings/components/tabs.tsx b/frontends/web/src/routes/settings/components/tabs.tsx
@@
type TWithSettingsTabsProps = {
children: ReactNode;
devices: TDevices;
hasAccounts: boolean;
hideMobileMenu?: boolean;
+ renderDefaultTabs?: boolean;
};
@@
export const WithSettingsTabs = ({
children,
devices,
hideMobileMenu,
hasAccounts,
+ renderDefaultTabs = true,
}: TWithSettingsTabsProps) => {
@@
- <div className="hide-on-small">
- <Tabs
- hideMobileMenu={hideMobileMenu}
- devices={devices}
- hasAccounts={hasAccounts}
- />
- </div>
+ {renderDefaultTabs ? (
+ <div className="hide-on-small">
+ <Tabs
+ hideMobileMenu={hideMobileMenu}
+ devices={devices}
+ hasAccounts={hasAccounts}
+ />
+ </div>
+ ) : null}
{showSearchResults ? (
<SettingsSearchContent
devices={devices}
searchResults={searchResults}
/>
) : children}diff --git a/frontends/web/src/routes/settings/mobile-settings.tsx b/frontends/web/src/routes/settings/mobile-settings.tsx
@@
- <WithSettingsTabs devices={devices} hasAccounts={hasAccounts}>
+ <WithSettingsTabs devices={devices} hasAccounts={hasAccounts} renderDefaultTabs={false}>
<Tabs devices={devices} hasAccounts={hasAccounts} />
</WithSettingsTabs>📝 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
| <WithSettingsTabs devices={devices} hasAccounts={hasAccounts}> | |
| <Tabs devices={devices} hasAccounts={hasAccounts} /> | |
| </WithSettingsTabs> | |
| type TWithSettingsTabsProps = { | |
| children: ReactNode; | |
| devices: TDevices; | |
| hasAccounts: boolean; | |
| hideMobileMenu?: boolean; | |
| renderDefaultTabs?: boolean; | |
| }; | |
| export const WithSettingsTabs = ({ | |
| children, | |
| devices, | |
| hideMobileMenu, | |
| hasAccounts, | |
| renderDefaultTabs = true, | |
| }: TWithSettingsTabsProps) => { | |
| // ... existing code ... | |
| {renderDefaultTabs ? ( | |
| <div className="hide-on-small"> | |
| <Tabs | |
| hideMobileMenu={hideMobileMenu} | |
| devices={devices} | |
| hasAccounts={hasAccounts} | |
| /> | |
| </div> | |
| ) : null} | |
| {showSearchResults ? ( | |
| <SettingsSearchContent | |
| devices={devices} | |
| searchResults={searchResults} | |
| /> | |
| ) : children} |
Suggested change
| <WithSettingsTabs devices={devices} hasAccounts={hasAccounts}> | |
| <Tabs devices={devices} hasAccounts={hasAccounts} /> | |
| </WithSettingsTabs> | |
| <WithSettingsTabs devices={devices} hasAccounts={hasAccounts} renderDefaultTabs={false}> | |
| <Tabs devices={devices} hasAccounts={hasAccounts} /> | |
| </WithSettingsTabs> |
🤖 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/mobile-settings.tsx` around lines 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.
Comment on lines
+130
to
+140
| it('does not include export logs when the setting is hidden in debug builds', () => { | ||
| const results = filterSettingsSearchItems(getItems(), 'export logs'); | ||
|
|
||
| expect(results).toEqual([]); | ||
| }); | ||
|
|
||
| it('does not include screen lock when the setting is hidden outside mobile apps', () => { | ||
| const results = filterSettingsSearchItems(getItems(), 'screen lock'); | ||
|
|
||
| expect(results).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make env-gated assertions deterministic.
These assertions depend on ambient env flags via @/utils/env. Mock those flags in this test to avoid runner-mode-dependent behavior.
Proposed change
-import { describe, expect, it } from 'vitest';
+import { describe, expect, it, vi } from 'vitest';
@@
import {
filterSettingsSearchItems,
getSettingsSearchItems,
} from './settings-search';
+
+vi.mock('`@/utils/env`', () => ({
+ debug: true,
+ runningInAndroid: () => false,
+ runningInIOS: () => false,
+}));🤖 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/settings-search.test.ts` around lines 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.
Comment on lines
+30
to
+37
| const deviceId = Object.keys(devices)[0]; | ||
| const device = deviceId ? devices[deviceId] : undefined; | ||
| const bb02DeviceId = device === 'bitbox02' ? deviceId : undefined; | ||
| const deviceInfoResult = useLoad( | ||
| bb02DeviceId ? () => getDeviceInfo(bb02DeviceId) : null, | ||
| [bb02DeviceId], | ||
| ); | ||
| const deviceInfo = deviceInfoResult?.success ? deviceInfoResult.deviceInfo : undefined; |
There was a problem hiding this comment.
Avoid deriving device capabilities from only the first device.
Using Object.keys(devices)[0] makes capability-gated search results depend on object key order instead of the active settings device, which can surface wrong results when multiple devices exist.
Suggested direction
type TUseSettingsSearchArgs = {
devices: TDevices;
hasAccounts: boolean;
+ selectedDeviceID?: string;
};
@@
export const useSettingsSearch = ({
devices,
hasAccounts,
+ selectedDeviceID,
}: TUseSettingsSearchArgs) => {
@@
- const deviceId = Object.keys(devices)[0];
+ const deviceId = selectedDeviceID ?? Object.keys(devices)[0];🤖 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/use-settings-search.ts` around lines 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.
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.
No description provided.