Feat: isEmptyReport derived value #60697
Conversation
…into feat/derived-values/is-empty-report
…into feat/derived-values/is-empty-report
…into feat/derived-values/is-empty-report
…into feat/derived-values/is-empty-report
|
@hoangzinh 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] |
|
PR #60497 was merged. You can try to merge latest main @kacper-mikolajczak |
|
Thanks @hoangzinh! Latest main was merged - waiting for CI checks 🏁 |
|
@hoangzinh All checks have passed, PR is ready to be reviewed 👍 🆚 👎 |
|
Hi @hoangzinh! PR is ready to be reviewed |
|
Yes, I will review it today. Btw, tbh, for final review, I would like to wait for this PR #60497 to be deployed to Prod. It could be reverted if there are regressions issues. |
Agree, that makes sense! 👍 |
|
Btw, there are some conflicts, are you available to fix it please? |
Done ✅ |
| // Add correct default value for REPORT_DRAFT collection | ||
| dependencies[7] ??= {}; |
There was a problem hiding this comment.
With this custom, does it need to add ONYXKEYS.COLLECTION.REPORT_DRAFT to the dependencies list?
There was a problem hiding this comment.
Yes, this is really unfortunate to have that but at the same time it looks like a simplest way to get the fallback value.
Do you know what is the reason some onyx collections have empty value of {}, while some, like REPORT_DRAFT can result in undefined?
The reason it was required here is check in areAllDependenciesSet that checks against undefined and will bail out of all updates when REPORT_DRAFT collection is empty - making all the reportAttributes out of sync.
There was a problem hiding this comment.
CC @TMisiukiewicz who is the main man behind idea of reportAttributes - what do you think of this Tomek, could that be avoided? Thanks!
Did any other empty collection resulted in undefined? If yes, how did you solve it?
There was a problem hiding this comment.
Is it because reportDraft_ does not always exist?
There was a problem hiding this comment.
Yes, it may be undefined, which will fail areAllDependenciesSet check.
There was a problem hiding this comment.
I feel that PR will take time. What do you guys think if
- @kacper-mikolajczak go head and cherry-pick those code into this PR
- or @TMisiukiewicz will help to commit that commit into this PR
- or hold on that PR.
There was a problem hiding this comment.
Thank you @kacper-mikolajczak #60697 (comment)
There was a problem hiding this comment.
I am down for all of the options! @TMisiukiewicz how much time do you think PR with the improvement you mentioned needs to be completed?
There was a problem hiding this comment.
@kacper-mikolajczak I just finished addressing all the comments from @hoangzinh, I hope we'll be able to push things forward quickly
There was a problem hiding this comment.
The reportDrafts are there as a dependency, because isEmpty value calculation depends on them but they are not part of what we want to provide from this derived value.
With those reasons, I think we don't really need to add reportDrafts are there as a dependency, because when reportDrafts are updated, we don't recalculate anything, because updates will be empty here
App/src/libs/actions/OnyxDerived/configs/reportAttributes.ts
Lines 72 to 74 in 884ba29
|
Hi @hoangzinh The PR is ready for final review round! Thanks to @TMisiukiewicz support, the Thanks guys :) |
|
Awesome @kacper-mikolajczak. Will try to review again today |
| @@ -107,6 +108,7 @@ export default createOnyxDerivedValueConfig({ | |||
|
|
|||
| acc[report.reportID] = { | |||
| reportName: generateReportName(report), | |||
| isEmpty: generateIsEmptyReport(report), | |||
There was a problem hiding this comment.
Could you contribute to the unit test for Derived value here?
There was a problem hiding this comment.
Sure! I tried to add another test, but I've noticed that the tests are not working as expected (they are not failing regardless of assertion made). Will address it (and contact Tomek to double check if there is indeed a regression) and get back to you when resolved 👍
There was a problem hiding this comment.
It turns out is something with async nature of tests because they are not correctly fulfilled (the async callback is not even ran).
Also, what would might help is waiting for Onyx.clear() in beforeEach.
We are still trying to figure out 100% what's happening.
| // Add correct default value for REPORT_DRAFT collection | ||
| dependencies[7] ??= {}; |
There was a problem hiding this comment.
The reportDrafts are there as a dependency, because isEmpty value calculation depends on them but they are not part of what we want to provide from this derived value.
With those reasons, I think we don't really need to add reportDrafts are there as a dependency, because when reportDrafts are updated, we don't recalculate anything, because updates will be empty here
App/src/libs/actions/OnyxDerived/configs/reportAttributes.ts
Lines 72 to 74 in 884ba29
You are right, it won't recalculate anything int this case. Thank you 👍 Wondering what are the implications there - am I assuming right that the Noticed however, the same happens for |
|
@kacper-mikolajczak do you know how to create |
|
@hoangzinh Sorry, I don't have knowledge about how the feature works in the app. Maybe asking some folks at slack could help. |
|
Yep, asking here https://expensify.slack.com/archives/C01GTK53T8Q/p1746795799334219 |
Let me know if anything has changed in that matter and how we should proceed (or how I can help you), thank you! |
|
@TMisiukiewicz @hoangzinh I have created an issue regarding failing tests: #61897 As the regression is not only related to this PR, we might address it there separately, what do you think? I have pushed changes to |
|
Yes, I think we can remove Btw, does it mean we can cont process with this PR? |
If the QA is positive, then I am good with taking that further (the tests are not showing proper outcomes so we can't rely on them) 👍 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-05-14.at.21.41.29.android.movAndroid: mWeb ChromeScreen.Recording.2025-05-14.at.21.49.24.moviOS: HybridAppScreen.Recording.2025-05-14.at.21.39.04.ios.moviOS: mWeb SafariScreen.Recording.2025-05-14.at.21.16.35.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2025-05-14.at.20.41.03.web.movMacOS: DesktopScreen.Recording.2025-05-14.at.21.02.10.desktop.mov |
| /** | ||
| * Check if the report is empty, meaning it has no visible messages (i.e. only a "created" report action). | ||
| * Added caching mechanism via derived values. | ||
| */ | ||
| function isEmptyReport(report: OnyxEntry<Report>): boolean { |
There was a problem hiding this comment.
NAB, but if all that isEmptyReport does is add caching on top generateEmptyReport, wouldn't it make more sense to concatenate them into a single function? Something like:
function isEmptyReport(report: OnyxEntry<Report>, useCache = true): boolean {
if (!report) {
return true;
}
if (useCache && reportAttributes?.[report.reportID]) {
return reportAttributes?.[report.reportID].isEmpty;
}
if (report.lastMessageText) {
return false;
}
const lastVisibleMessage = getLastVisibleMessage(report.reportID);
return !lastVisibleMessage.lastMessageText;
}There was a problem hiding this comment.
Hi @mjasikowski, thanks for the comment!
Why we need to separate them into two is that when generating reportAttributes under OnyxDerived, we need to make sure it is fresh value. In other words, we need to force cache refresh when cache expires.
I have followed the pattern that was used with generateReportName + getReportName combo.
Let me know if that makes sense, thanks!
|
@hoangzinh what's the next step for us with this PR? |
|
Hi folks! Please let me know what are the next steps for this PR, thanks! |
|
@kacper-mikolajczak all good here. We will wait for @mjasikowski to merge and deploy it to staging for testing. |
|
Awesome, thanks! |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.48-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.48-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.49-5 🚀
|
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.1.50-0 🚀
|
Explanation of Change
This PR introduces implementation of
isEmptyderived value as a part of #59350.[Blocker] It is dependent on #60497 which contains fixes to how derived values work in general.
Fixed Issues
$ #59350
PROPOSAL: Proposal
Tests
Offline tests
Same as in Tests
QA Steps
Same as in Tests
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))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.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios_web.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4