Perf/lhn options cache #36
Conversation
…ith Onyx.clear and waitForBatchedUpdates
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| const reportActionsForDisplay = sortedReportActions.filter( | ||
| (reportAction) => | ||
| (!(isWhisperAction(reportAction) && !isReportPreviewAction(reportAction) && !isMoneyRequestAction(reportAction)) || isActionableMentionWhisper(reportAction)) && | ||
| shouldReportActionBeVisible(reportAction, reportAction.reportActionID, isWriteActionAllowed) && | ||
| reportAction.actionName !== CONST.REPORT.ACTIONS.TYPE.CREATED && | ||
| reportAction.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE, | ||
| ); |
There was a problem hiding this comment.
I think we could make this part more readable, or maybe explain it somehow. I'm afraid it might be difficult to maintain. Wdyt?
| lastVisibleReportActions[reportID] = reportActionForDisplay; | ||
|
|
||
| const lastActorAccountID = report?.lastActorAccountID; | ||
| let lastActorDetails: Partial<PersonalDetails> | null = lastActorAccountID && allPersonalDetails?.[lastActorAccountID] ? allPersonalDetails[lastActorAccountID] : null; |
There was a problem hiding this comment.
| let lastActorDetails: Partial<PersonalDetails> | null = lastActorAccountID && allPersonalDetails?.[lastActorAccountID] ? allPersonalDetails[lastActorAccountID] : null; | |
| let lastActorDetails = allPersonalDetails?.[lastActorAccountID] ?? null; |
Would that work?
| if (!lastActorDetails && reportActionForDisplay) { | ||
| const lastActorDisplayName = reportActionForDisplay.person?.[0]?.text; | ||
| lastActorDetails = lastActorDisplayName | ||
| ? { | ||
| displayName: lastActorDisplayName, | ||
| accountID: lastActorAccountID, | ||
| } | ||
| : null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it a dead code? Is lastActorDetails used somewhere?
| getCachedReportActionsForDisplay, | ||
| getCachedLastMessageText, |
There was a problem hiding this comment.
I don't see usage of these 2. Is it for follow up PRs or something?
| function getCachedLastMessageText(reportID: string, computeFn?: () => string): string | undefined { | ||
| let cached = lastMessageTextCache[reportID]; | ||
| if (!cached && computeFn) { | ||
| cached = computeFn(); | ||
| lastMessageTextCache[reportID] = cached; | ||
| } | ||
| return cached; | ||
| } |
There was a problem hiding this comment.
How about empty string? Not sure if its a real issue but it would cause cache miss. Would be nice to have a unit tests for that
| * | ||
| * @param reportID - The ID of the report to invalidate cache for | ||
| */ | ||
| function invalidateCacheForReport(reportID: string) { |
There was a problem hiding this comment.
This one also relies on allPersonalDetails, allReports, and allReportNameValuePairsOnyxConnect. Is it intentional that the cache is invalidated only for REPORT_METADATA and REPORT_ACTIONS? Will this not introduce stale data if eg only allPersonalDetails changes?
3 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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