[NoQA] feat: Add ManualSendMessageVisible subspans - #98043
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.
|
2d0e956 to
bf9b7d6
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf9b7d662d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6351560c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
fa63515 to
3d797b8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d797b8451
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
|
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". |
Explanation of Change
This partitions
ManualSendMessageVisibleinto three sequential child spans. They do not overlap and theirdurations sum to the parent, so each accounts for one stage.
t0addComment,src/libs/actions/Report/index.ts:1250t1API.write(...)returns,src/libs/actions/Report/index.ts:1160t2TextCommentFragment's layout effect, which React runs synchronously during the committ3TextCommentFragment'sonLayouthandlerBoth render-side boundaries are keyed on the sent message's own
reportActionIDand marked by a componentthat already receives it. Nothing infers which row the sent message is, and no list item renderer carries
telemetry.
What each phase measures
SubmitaddCommententry toAPI.writereturningAPI.writeonly enqueues andOnyx.mergedefers the merge, so nothing is persisted and nothing has rendered yetPropagatevisibleReportActionsrecompute, which lands inPaintPaintResizeObserverdeliveryPropagateending is not the message becoming visible.t2is auseLayoutEffect, which React runssynchronously inside the commit, before the browser has done any layout or paint work.
t3is the earliestpoint at which the row can be confirmed laid out.
What this does and does not answer
Being explicit, because the value here is narrower than "the spans will show why sends are slow".
Propagateis the only phase that contains yield points, so contention lands there structurally rather thanas a finding. Across eleven measured sends it was dominant every time, at 68 to 87% of the parent. Inside it,
roughly 60% of a send sits in gaps that carry no spans at all, and separating the causes there needs a
main-thread profile, not more spans.
What the split does deliver:
Submitpresent,Propagateabsent, parentcanceledmeans theoptimistic action never reached a list. That is a countable Sentry query rather than an anecdote, and today
the two cases share one number.
Submitmeasured under 1% of the parent in every send andwas flat between a light account and a heavy account on a two-deep RHP stack, so the optimistic builders
are not a scaling problem. React's render and commit of the sent row measured 2 to 5%. The derived cascade
measured 11.5 to 47ms and did not grow with scenario weight.
Propagateand the parent together, ormoves nothing.
SubmitandPaintstaying flat is what confirms a win is real rather than a shift betweenstages.
Coverage limits: only sends made while scrolled to the bottom are measured, since the parent span does not
start otherwise. No phase covers time after the message is visible. The keypress to
addCommentintervalsits outside the parent.
Implementation notes
src/libs/telemetry/sendMessageSpans.ts, following the existing single-purpose span-helperpattern (
markSubmitExpenseEnd.ts,startScanProcessSpan.ts). No module state.activeSpansis theregistry:
getSpan(phaseId)answers whether a phase is running, which also makes every functionself-healing, since once the parent is cancelled every later call for that send is a no-op.
ManualSendMessage, so the existingcancelSpansByPrefixsweeps alreadymatch them.
ManualSendMessageVisibleonly. The effect-anchoredManualSendMessagekeeps nochildren, so the A/B comparison in feat: Add ManualSendMessageVisible #97789 stays clean and nothing here moves when that span is retired.
t2must stay a layout effect. A passive effect is scheduled with no paint guarantee and drifts by anunknown amount, charging part of
PainttoPropagate.phases first:
onLayouton the happy path, the skeleton listener on the cancel path, andcancelSpansByPrefix, which now cancels in reverse insertion order. Reverse is children-first because achild is always registered after the parent it hangs off.
cancelAllSpansis left in insertion order onpurpose: it sweeps every span family, so reversing it would start surfacing other features' cancelled
children with abort-time durations. A send interrupted by an AppState transition therefore still loses its
open phase, which the reading guide in the investigation doc calls out.
before this PR. It required a mutation in the render body of
ReportActionsListItemRenderer, which adiscarded concurrent render would anchor one pass early, and it measured 2 to 5% and was never dominant.
Its finding is recorded in the investigation doc instead of being re-measured on every send.
Cost is three additional spans per send. Runtime overhead is two
activeSpansmap lookups and one spanobject per phase, none of it on a render hot path.
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
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