Allow workspace and status filter to be negatable#95023
Conversation
melvin-bot
Bot
requested review from
aimane-chnaif and
flaviadefaria
and removed request for
a team and
flaviadefaria
|
@aimane-chnaif Please 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d15fbc7a5b
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@aimane-chnaif let's merge the text filter validation PR first, so we can resolve some of the type/lint error here. |
|
@aimane-chnaif ready for your review |
|
Conflicts again |
|
Every card's "View transactions" now shows a spurious App/src/libs/CardNavigationUtils.ts Line 10 in d3ef0a8 The PR removed the parser's
Fix recommendation: update these callers to stop passing |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS}:${Array.isArray(query.status) ? query.status.join(',') : query.status}`; | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.GROUP_BY}:${query.groupBy}`; | ||
|
|
||
| if (query.policyID) { | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_FILTER_KEYS.POLICY_ID}:${Array.isArray(query.policyID) ? query.policyID.join(',') : query.policyID} `; | ||
| } |
There was a problem hiding this comment.
Bug or expected?: Existing saved & recent searches orphaned after deploy
Removing status, policyID from orderedQuery changes primaryHash for every query.
Saved searches persist keyed by that hash and match the active view
App/src/pages/Search/SearchTypeMenuNarrow.tsx
Line 175 in d3ef0a8
so pre-existing entries stop highlighting as active and re-saving duplicates them.
No migration in frontend so confirm the backend re-keys nvp_savedSearches/nvp_recentSearches.
There was a problem hiding this comment.
Definitely not expected. Updated to preserve the order
| if (typeKey) { | ||
| if (Array.isArray(queryJSON.status)) { | ||
| const validStatuses = queryJSON.status.filter((status) => Object.values(CONST.SEARCH.STATUS[typeKey as keyof typeof CONST.SEARCH.DATA_TYPES]).includes(status)); | ||
|
|
||
| if (validStatuses.length) { | ||
| filtersForm[FILTER_KEYS.STATUS] = queryJSON.status.join(','); | ||
| } else { | ||
| filtersForm[FILTER_KEYS.STATUS] = CONST.SEARCH.STATUS.EXPENSE.ALL; | ||
| } | ||
| } else { | ||
| filtersForm[FILTER_KEYS.STATUS] = queryJSON.status; | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor: Invalid/stale status persists in the filter form
Status validation against CONST.SEARCH.STATUS[typeKey] + ALL fallback was dropped so a query invalid for its type (e.g. type:invoice status:approved) now keeps the bad value and re-serializes it silently.
There was a problem hiding this comment.
I don't think we need to keep this. buildFilterFormValuesFromQuery is used to synchronize the queryJSON to the SEARCH_ADVANCED_FILTERS_FORM onyx data. First, it only handles fallback to ALL when the invalid status is more than 1. It's also inconsistent, the queryJSON filter will still contain the invalid filters and send to the BE, but the FE only onyx data replaces it with ALL (an empty string)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fe57537e6
ℹ️ 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 sortedQueryJSONStatus = Array.isArray(status) ? status.sort().join(',') : status; | ||
| const isDataLoaded = | ||
| (searchResults?.data != null || searchResults?.errors != null) && searchResults?.search?.type === queryJSON?.type && sortedSearchResultStatus === sortedQueryJSONStatus; | ||
| const isDataLoaded = (searchResults?.data != null || searchResults?.errors != null) && searchResults?.search?.type === queryJSON?.type && searchResults.search.hash === queryJSON?.hash; |
There was a problem hiding this comment.
When a search request fails, getOnyxLoadingData() writes a snapshot with errors and search.type but no search.hash (checked src/libs/actions/Search.ts). Since this check now requires searchResults.search.hash === queryJSON.hash, failed snapshots never count as loaded; useSearchPageSetup() then sees isSnapshotDataLoaded === false and isLoading === false and immediately fires search() again, so backend/transient failures retry in a loop instead of showing the error state. Include the hash in failure snapshots or tolerate error snapshots without it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
queryJSON.hash is the current query json hash and searchResults.search.hash is from the BE response. I don't think we should update it optimistically. Only after the search is done, BE then return the hash. It's the same with status. We don't optimistically update the status in getOnyxLoadingData
There was a problem hiding this comment.
Also, when it fails, an error view will be shown.
🤖 ReviewSolid, coherent refactor that promotes Two things I'd want resolved before this merges, plus a validation gap and some noise to clear. 🔴 1. Confirm existing saved/recent searches aren't orphaned by the hash change
This is the same concern 🔴 2. Point me to where negated
|
| return policyID.value.map((id) => policyData?.[`${key}${id}`]).filter((data): data is NonNullable<typeof data> => !!data); | ||
| } | ||
|
|
||
| function getAllPolicyValuesMap<T extends OnyxCollectionKey>( |
There was a problem hiding this comment.
This is near-identical to getAllPolicyValues.
The only difference is array-push vs keyed-assign.
getAllPolicyValues can be Object.values(getAllPolicyValuesMap(policyID, key, data)), removing ~30 duplicated lines and the risk that the id-stripping (replace only kills the first occurrence) diverges between them.
There was a problem hiding this comment.
I intentionally created 2 variants to be more optimized (considering policies can be large). If we do Object.values(getAllPolicyValuesMap(policyID, key, data)), we create the object first and then iterate to create the array. It'll be easier to maintain though, but I think this function won't change that much.
Can't reproduce. STATUS.EXPENSE.ALL is an empty string, not "all". I've removed all usages of it. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Knip check failed |
|
Fixed |
4 participants
Explanation of Change
Fixed Issues
$ #93208
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
-prefix) status and/or workspace filter (e.g.,-status:draft,-workspace:"My workspace 1")PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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.2.mp4