[Preparing the UI] [No QA] Revamp search page actions bar#85508
[Preparing the UI] [No QA] Revamp search page actions bar#85508bernhardoj wants to merge 49 commits intoExpensify:mainfrom
Conversation
|
@dubielzyk-expensify @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Many of the changes are renaming. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65d7690e5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/components/Search/SearchPageHeader/SearchTypeMenuNarrow.tsx
Outdated
Show resolved
Hide resolved
|
Lint is unrelated |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
🤖 Code ReviewOverall this is a well-structured setup PR that scaffolds the new search actions bar components without wiring them to the UI yet. The renames, translations, and new type definitions are clean. A few items to address: Issues1. 2. 3. 4. File/function name mismatch in hooks
The file names should match the exported function names (either all singular or all plural). 5. 6. Missing Minor / Non-blocking
Otherwise the component architecture, the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5700da2207
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const queryString = (() => { | ||
| const {view, groupBy, sortBy, sortOrder, limit} = getCurrentSearchQueryJSON() ?? {}; | ||
| return buildFilterQueryWithSortDefaults(searchAdvancedFiltersForm, {view, groupBy}, {sortBy, sortOrder, limit}) ?? ''; | ||
| })(); | ||
| const queryJSON = buildSearchQueryJSON(queryString || buildCannedSearchQuery()); |
There was a problem hiding this comment.
Derive the save query from the live search state
If SearchSavePage is opened after a refresh/deep link, or any time SEARCH_ADVANCED_FILTERS_FORM is stale, this code rebuilds queryJSON from the cached form instead of the active search route. Unlike useSearchFiltersBar() (src/components/Search/SearchPageHeader/useSearchFiltersBar.tsx:123-124), this page never calls useFilterFormValues()/useSearchFilterSync(), so saveSearch() and applyFiltersAndNavigate() can silently save or reopen the wrong query (often an older search or the canned default).
Useful? React with 👍 / 👎.
| return ''; | ||
| } | ||
|
|
||
| const feedsForDisplay = getCardFeedsForDisplay(allFeeds, personalAndWorkspaceCards, translate, feedKeysWithCards); |
There was a problem hiding this comment.
App/src/hooks/useAutocompleteSuggestions.ts
Lines 362 to 365 in d05d2a0
Should this comment apply here too?
There was a problem hiding this comment.
|
Should this PR be [No QA] as new pages are not visible yet? |
@MelvinBot perf tests failing very often. Can you investigate this and implement fix in separate PR? |
🔍 Perf Test Flakiness InvestigationSummary: The Root Cause: The Evidence: The exact same failure ( Recommended Fix: Change - COUNT_DEVIATION: 0
+ COUNT_DEVIATION: 1Blocker: I was unable to push this fix as a separate PR because the GitHub PAT lacks the |
|
This should prob be no QA yes |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
There's typescript error. And please merge main. |
| import CONST from '@src/CONST'; | ||
| import ROUTES from '@src/ROUTES'; | ||
|
|
||
| function SearchSaveButton() { |
There was a problem hiding this comment.
This component seems unnecessary. It's just a wrapper around Button and called in two places. We could just pass the props from the parent components.
There was a problem hiding this comment.
I created this so we don't need to duplicate the same button with the same props in 2 places (as you mentioned, easier to maintain).
We could just pass the props from the parent components.
Which one?
src/libs/Navigation/AppNavigator/Navigators/RightModalNavigator.tsx
Outdated
Show resolved
Hide resolved
| SCREENS.MONEY_REQUEST.SPLIT_EXPENSE_SEARCH, | ||
| SCREENS.RIGHT_MODAL.SEARCH_MONEY_REQUEST_REPORT, | ||
| SCREENS.RIGHT_MODAL.SEARCH_REPORT, | ||
| SCREENS.RIGHT_MODAL.SEARCH_SAVE, |
There was a problem hiding this comment.
Same here and for other navigation related changes
There was a problem hiding this comment.
I think we can keep this one since we already removed the screen from the navigator, so the user can't access it directly from the URL/deeplink anymore. So, in the next PR, we just need to re-add the screen to the navigator. WDYT?
web.mp4
|
@aimane-chnaif merged main |
|
Line 48 in e3f9358 Max warnings count on main is 315 but this PR is 316 |
|
Hmm, I guess the 2 unrelated ones have been ignored, and this PR introduces another 2. Let me find the 2 that this PR introduces |


Explanation of Change
Revamp the search page actions bar. (This PR only adds the necessary component without being shown yet)
Fixed Issues
$ #84876
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4
web.search.mp4