Remove ReportConnection and ReportActionsConnection files #53385
Conversation
This file contains 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
This comment was marked as outdated.
This comment was marked as outdated.
fabioh8010
reviewed
Dec 4, 2024
Comment on lines
+94
to
+95
| // Dynamic Import to avoid circular dependency | ||
| const UnreadIndicatorUpdaterHelper = () => import('./UnreadIndicatorUpdater'); |
There was a problem hiding this comment.
Yes, without it the tests - both unit and perf are failing
src/libs/actions/Task.ts
Outdated
| ReportConnection.updateReportData(parentReportID, { | ||
| lastReadTime: currentTime, | ||
| }); | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${parentReportID}`, {lastReadTime: currentTime}); |
There was a problem hiding this comment.
This change is different from the previous behaviour in ReportConnection.ts, where we update the local allReports instead of doing Onyx operations. It would be good to understand why we were doing that before and if it's safe to use Onyx operations instead.
There was a problem hiding this comment.
It looks like it will be removed anyway: https://github.com/Expensify/App/pull/51174/files#r1866068419
There was a problem hiding this comment.
Nevertheless, I've asked the author why this approach was selected initially.
There was a problem hiding this comment.
huh yeah I agree this is weird, what is the conclussion?
There was a problem hiding this comment.
Overall the ReportConnection.updateReportData function, as well as some other changes including reportLastReadTime memoized value were introduced in this PR to fix this issue: #50792.
I've tested the issue over my PR and it works as expected.
I've just tried to remove the updates raised in that PR as it was recommended, and there is no issue for me as well. So I think we can get rid of it, did it in: d01ac8d
ios_no_new_line1.mp4
…eport-connection # Conflicts: # src/libs/ReportUtils.ts
VickyStash
changed the title
…eport-connection # Conflicts: # src/libs/actions/Report.ts
fabioh8010
approved these changes
Dec 5, 2024
VickyStash
changed the title
melvin-bot
bot
requested review from
thesahindia
and removed request for
a team
|
@thesahindia 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] |
|
@thesahindia Any ETA on reviewing this one? |
48 tasks
VickyStash
commented
Dec 11, 2024
| @@ -606,7 +604,7 @@ const ContextMenuActions: ContextMenuAction[] = [ | |||
| successIcon: Expensicons.Checkmark, | |||
| shouldShow: ({type, isProduction}) => type === CONST.CONTEXT_MENU_TYPES.REPORT && !isProduction, | |||
| onPress: (closePopover, {reportID}) => { | |||
| const report = ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]; | |||
| const report = ReportUtils.getReport(reportID); | |||
There was a problem hiding this comment.
Left ReportUtils.getReport in this file, cause it's already used there, and changing it gives the error:
Only call Onyx.connect() from inside a /src/libs/** file. React components and non-library code should not use Onyx.connect() rulesdir/prefer-onyx-connect-in-libs]
|
@mountiny I've applied the feedback, please take another look 👀 |
|
Asked @thesahindia for retest |
thesahindia
approved these changes
Dec 11, 2024
…eport-connection # Conflicts: # src/libs/ModifiedExpenseMessage.ts
|
@mountiny All yours! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
github-actions
bot
added
the
DeployBlockerCash
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
These have been flaky after the hybrid app switcharoo |
mountiny
removed
the
DeployBlockerCash
50 tasks
60 tasks
50 tasks
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.76-0 🚀
|
8 tasks
|
@VickyStash Found KI when checking the PR - #54126, #54129 |
8 tasks
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.76-12 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Remove ReportConnection and ReportActionsConnection files as they are not necessary anymore and have no effect on the app performance.
TTI before: 7916ms, 7662ms,7273ms
TTI after ReportConnection: 7942ms, 8223ms, 7302ms
Fixed Issues
$ #53141
PROPOSAL: N/A
Tests
Part 1:
3.1 Create a chat
3.2 Send several messages from Users A and B
3.3. Submit expense manual/scan/distance
3.4. React to the message/update it/delete
Part 2:
Offline tests
Same, as in the Tests section.
QA Steps
Same, as in the Tests section.
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
android0.mp4
Android: mWeb Chrome
android_web0.mp4
iOS: Native
ios0.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web0.mp4
MacOS: Desktop
desktop0.mp4