[No QA] Fix is_warm labeling on report-open spans - #98852
Conversation
|
🚧 rlinoz has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
TMisiukiewicz
changed the title
|
@abzokhattab 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] |
|
@codex @MelvinBot review the PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * orchestrator, and the money-request views): the open-report span must still close even though the list | ||
| * body — which would otherwise own this mark once mounted — isn't mounted while the skeleton shows. | ||
| * Closes the open-report span as a cold open (`warm: false`) while a skeleton shows. Call it from a component | ||
| * mounted exactly while the skeleton is on screen — the list body that would otherwise close the span isn't |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain sentences without em dashes. This JSDoc description line uses an em dash to join two clauses.
Replace the em dash with a period and start a new sentence:
* Closes the open-report span as a cold open (`warm: false`) while a skeleton shows. Call it from a component
* mounted exactly while the skeleton is on screen. The list body that would otherwise close the span isn't
* mounted yet.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| /** | ||
| * Mark all 'open_report*' telemetry spans as finished. | ||
| * Mark all 'open_report*' telemetry spans as finished. Keyed by `reportID` so it still ends when the report | ||
| * hasn't loaded; the report-shape attributes are then left off. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should use two separate sentences instead of a semicolon. This JSDoc line joins two independent statements with a semicolon.
Split into two sentences:
* Mark all 'open_report*' telemetry spans as finished. Keyed by `reportID` so it still ends when the report
* hasn't loaded. The report-shape attributes are then left off.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| <NavigationDeferredMount | ||
| waitForUpcomingTransition={false} | ||
| placeholder={ | ||
| // Deferral, not a data wait — closing the span here would time the defer and tag a cached report cold. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain sentences without em dashes. This comment uses an em dash to join two clauses.
Replace the em dash with a period and start a new sentence:
// Deferral, not a data wait. Closing the span here would time the defer and tag a cached report cold.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| /** | ||
| * Report-actions loading skeleton. Mounted only while the skeleton shows, so it hosts the hook that cancels | ||
| * the otherwise never-ending send-message span (tagged with `skeletonName`). | ||
| * Report-actions loading skeleton. Hosts the skeleton-phase span marks — cancelling the never-ending |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
Comments should read as plain sentences without em dashes. This JSDoc description uses two em dashes to bracket a clause.
Rewrite without em dashes, for example:
* Report-actions loading skeleton. Hosts the skeleton-phase span marks: cancelling the never-ending
* send-message span, and closing the open-report span as cold. Mounted here means visible, while a
* parent's copy of the skeleton condition can drift from what actually renders.Reviewed at: f281e4a | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Review — telemetry
|
4 participants
Explanation of Change
The
ManualOpenReportspan is closed by whichever component gets there first: a skeleton closes it and tagsis_warm=false, the finished list closes it fromonLayoutand tagsis_warm=true. The problem was that the decision "a skeleton is showing" lived in the parents, and every copy of it was wrong in a different way:ReportActionsSkeletonGuardcheckedshouldShowInitialSkeleton, but a skeleton actually renders on the widershouldShowLoadingSkeletonand onshouldShowDerivedTimingSkeleton.ReportActionscheckedshouldShowAppLoadSkeleton, which excludesshouldWaitForTransactions— the exact case the line below it renders a skeleton for.MoneyRequestReportViewchecked one of its three skeleton branches and missed the other two.MoneyRequestReportActionsListchecked a flag calledshouldShowOpenReportLoadingSkeletonthat renders nothing at all. It means "initial actions still loading", which stays true after the transaction table is on screen, so it fired cold after the content was already visible.!report, which is the same condition one of those skeletons renders for, so that branch could never close at all.The result: cold opens showed a skeleton, nobody tagged them, and the span kept running until the messages rendered — landing in the
is_warm=truebucket with the full cold wait baked in. That's why warm p95 (~1.1s) looked worse than cold p95 (~600ms).What changed:
ReportActionsLoadingSkeleton, the component every chat-path skeleton renders through. Mounted means visible, so the mark can't drift from the condition. The two parent gates are deleted rather than corrected.MoneyRequestReportViewshares named booleans between its three skeleton branches and the mark, and takesreportIDFromRouteas a prop — its skeletons render before the report lands in Onyx, soreport?.reportIDwasundefinedand the mark silently did nothing.markOpenReportEndkeys the span onreportIDinstead of reading it off the report object, so opens where the report isn't in Onyx yet can close at all.MoneyRequestReportActionsListno longer marks cold — it renders no skeleton. Its layout close is now an unconditionalwarm: true, which is correct because a cold open would already have been closed by the skeleton, making this call a no-op.shouldMarkOpenReportEnd={false}. It's a deliberate deferral, not a wait on data, so timing it would tag a cached report as cold.Nothing user-visible changes and nothing gets faster — only the
is_warmlabel. How cold opens are measured is unchanged: they still stop the clock when the skeleton appears, not when the content does.Expect warm p95 to drop as the mislabeled opens leave, and cold p95 to dip as those same opens join it as fast time-to-skeleton samples. Cold getting "faster" is bookkeeping, not a win. Sentry alert thresholds on this span should be re-baselined off the first clean week.
Fixed Issues
$ #98880
PROPOSAL:
Tests
ManualOpenReportis_warm: trueis_warm: falseis_warm: falseOffline tests
N/A
QA Steps
N/A
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