Remove SearchTransaction > isActionLoading#74404
Merged
luacmartins merged 20 commits intoExpensify:mainfrom Nov 19, 2025
Merged
Remove SearchTransaction > isActionLoading#74404luacmartins merged 20 commits intoExpensify:mainfrom
luacmartins merged 20 commits intoExpensify:mainfrom
Conversation
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.
|
This reverts commit 3687c4d.
melvin-bot
bot
requested review from
mananjadhav and
trjExpensify
and removed request for
a team
|
@mananjadhav 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}
| let shouldShow = true; | ||
| if (queryJSON && isActionLoadingSet?.has(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportItem.reportID}`)) { | ||
|
|
||
| const isActionLoading = isActionLoadingSet?.has(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportItem.reportID}`); |
Contributor
There was a problem hiding this comment.
❌ Logic Error (context)
Critical bug: The condition logic was inverted incorrectly. The original code checked if the report IS loading:
if (queryJSON && isActionLoadingSet?.has(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportItem.reportID}`))But the refactored code checks if it's NOT loading:
if (queryJSON && !isActionLoading)This reverses the intended behavior and will cause reports to be hidden when they're loading, and shown when they shouldn't be filtered.
Fix: Change line 1543 from:
if (queryJSON && !isActionLoading) {to:
if (queryJSON && isActionLoading) {
Member
Author
There was a problem hiding this comment.
The original logic actually checked if not loading. It was changed here but I think it was a mistake cc @DylanDylann
| if (queryJSON && !transactionItem.isActionLoading) { | ||
|
|
||
| const isActionLoading = isActionLoadingSet?.has(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${transactionItem.reportID}`); | ||
| if (queryJSON && !isActionLoading) { |
Contributor
There was a problem hiding this comment.
❌ Logic Error (context)
Critical bug: The condition logic was inverted incorrectly. The original code checked if the transaction IS loading (if (queryJSON && !transactionItem.isActionLoading)), meaning it would apply status filtering when the transaction is NOT loading.
The refactored code also checks if (queryJSON && !isActionLoading), but the logic differs from the original in the getReportSections function at line 1543. To maintain consistency with the original behavior:
- If the original intent was to filter while loading, then line 1543 should be
if (queryJSON && isActionLoading) - If the original intent was to filter when NOT loading, then line 1543 is wrong
Based on the original getReportSections code checking isActionLoadingSet?.has(...) (meaning IS loading), there's an inconsistency that needs clarification. Please verify the intended behavior across both functions.
Member
Author
There was a problem hiding this comment.
The original condition checked if not loading and that's still the case with the new logic.
Member
Author
|
@mananjadhav Yes! Let me know if anything is not clear (please check linked issue for context) |
Collaborator
|
I'll start with the review Monday. Thanks. |
trjExpensify
previously approved these changes
Nov 17, 2025
Contributor
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
PR for a #whatsnext project, all good. 👍
Contributor
|
@mananjadhav all yours |
mananjadhav
reviewed
Nov 18, 2025
Collaborator
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Reviewed the code, testing it.
Contributor
|
@s77rt conflicts |
Member
Author
|
Resolved conflicts |
Collaborator
|
I tested on web. Will finish the checklist today. |
Collaborator
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-search-transaction.mp4Android: mWeb Chromemweb-chrome-search-transaction.moviOS: HybridAppios-search-transaction.moviOS: mWeb Safarimweb-safari-search-transaction.movMacOS: Chrome / Safariweb-search-transaction.movMacOS: Desktopdesktop-search-transaction.mov |
Collaborator
|
@luacmartins Both my native builds are taking unexpectedly longer. Is it possible to trigger adhoc builds? I've uploaded screencasts for the other 4 platforms. |
Contributor
|
🚧 @luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
Contributor
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Collaborator
|
Got the native builds. Going to quickly test them now. |
mananjadhav
approved these changes
Nov 19, 2025
Collaborator
|
@luacmartins All yours. |
luacmartins
approved these changes
Nov 19, 2025
Contributor
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.2.62-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.2.62-5 🚀
|
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
6 participants
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.
Explanation of Change
Fixed Issues
$ #73956
PROPOSAL:
Tests
Approveon the right, click on itPayafterwardsPayScreen.Recording.2025-11-16.at.3.05.38.AM.mov
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
MacOS: Desktop