[Performance] Extend submit-to-destination-visible span with diagnostic attributes #92359
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ 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". |
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.
|
There was a problem hiding this comment.
Pull request overview
Extends the existing submit-to-destination-visible telemetry span with additional diagnostic attributes to help identify why some submit flows stall or land on unexpected destinations, and adds a focus-based span-end trigger to cover cases where layout does not re-fire after RHP transitions.
Changes:
- Add
snapshot.*,at_dismiss.*,dismiss_narrow.*,search_dismiss.*, andat_end.*span attributes to capture navigation state/timing at key points in the submit/dismiss flow. - Add a
FOCUStrigger usage onSearchMoneyRequestReportPageto end the span when the screen is revealed after RHP screens are popped. - Introduce
getTabNavigatorDiagnosticshelper and telemetry helpers (hasSubmitSpan,setSubmitSpanAttributes,getTrackingElapsedMs) to support consistent attribute recording.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/submitDismissStrategiesTest.ts | Updates telemetry mocks to include new helpers used by dismiss strategies. |
| src/pages/Search/SearchMoneyRequestReportPage.tsx | Adds a focus-based submit-to-visible trigger to handle “revealed after pop” cases. |
| src/pages/iou/request/step/IOURequestStepConfirmation.tsx | Wires hasFormBeenSubmitted() callback into the orchestrator for dismiss-time diagnostics. |
| src/pages/iou/request/step/confirmation/SubmitExpenseOrchestrator.tsx | Records snapshot + search-dismiss + dismiss-time span diagnostics during submit orchestration. |
| src/pages/iou/request/step/confirmation/submitDismissStrategies.ts | Adds narrow-dismiss diagnostics and exports buildAtDismissAttributes() for report pre-insert investigation. |
| src/pages/iou/request/step/confirmation/getTabNavigatorDiagnostics.ts | New helper to read TAB_NAVIGATOR availability/active tab/nested state for telemetry. |
| src/pages/iou/request/step/confirmation/getSubmitHandler.ts | Adds buildSnapshotAttributes() to serialize handler decision inputs into span attributes. |
| src/libs/telemetry/submitFollowUpAction.ts | Adds submit-span helpers and at_end.* attributes when ending the span. |
| src/CONST/index.ts | Adds submit span attribute prefixes and an ATTRIBUTE_VALUE_UNAVAILABLE sentinel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
See: slack convo |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d501bd0c4
ℹ️ 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 |
| return TAB_NAVIGATOR_UNAVAILABLE; | ||
| } | ||
|
|
||
| const topmostTabNavigatorRoute = rootState.routes.findLast((route) => route.name === NAVIGATORS.TAB_NAVIGATOR); |
| const activeRoute = topmostTabNavigatorRoute.state.routes?.at(index); | ||
| const nestedState = activeRoute?.state; | ||
| return { | ||
| tabNavigatorStateAvailable: true, | ||
| tabActiveName: activeRoute?.name, | ||
| activeTabState: nestedState && 'routes' in nestedState ? (nestedState as NavigationState) : undefined, | ||
| }; |
| const topmostReportID = getTopmostReportParams(rootState)?.reportID; | ||
| if (topmostReportID) { | ||
| attrs[`${prefix}topmost_report_id`] = topmostReportID; | ||
| } | ||
| attrs[`${prefix}report_id_matches`] = reportID !== undefined && topmostReportID === reportID; |
| const params = lastSplitRoute.params as Record<string, unknown> | undefined; | ||
| const reportID = typeof params?.reportID === 'string' ? params.reportID : undefined; | ||
| if (reportID) { | ||
| attrs[`${prefix}split_last_report_id`] = reportID; | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3de120950
ℹ️ 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".
| useSubmitToDestinationVisible( | ||
| [CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.DISMISS_MODAL_AND_OPEN_REPORT, CONST.TELEMETRY.SUBMIT_FOLLOW_UP_ACTION.DISMISS_MODAL_ONLY], | ||
| reportIDFromRoute, | ||
| CONST.TELEMETRY.SUBMIT_TO_DESTINATION_VISIBLE_TRIGGER.FOCUS, | ||
| ); |
There was a problem hiding this comment.
When this page is newly mounted or revealed while an RHP pop animation is still running, this focus-based trigger can fire before the existing onLayout trigger and before the destination is actually visible; React Navigation's useFocusEffect docs note focus effects may run before transition animations finish. Because endSubmitFollowUpActionSpan clears the pending action on the first trigger, the later layout callback cannot correct the measurement, so submit-to-destination-visible spans for these Search money-request reports will under-report the transition time and hide the slow paths this diagnostic is trying to investigate.
Useful? React with 👍 / 👎.
|
Closing in favor of #93452 |
Explanation of Change
Extends the
submit-to-destination-visibleSentry span with diagnostic attributes to investigate 6 production issues identified from trace analysis:What I've observed (from 11 Sentry traces):
defaulthandler fallback hit unexpectedly when fast paths should match - 40-50s blocked JS thread, likely caused by tab navigator state being temporarily unavailable during HybridApp remounts (ce57bcb9, d0c23d02, 2a89ae3d, 9789e05e)report_pre_insertroute lost after dismiss - user lands on LHN instead of pre-inserted report, up to 34.8s manual navigation (ae1d8901, 3779d897, e7e05ac2)report_in_rhp_dismissspan never ends -onLayoutdoesn't re-fire when component is revealed after RHP screens are popped (b593642b, b0153d43)dismiss_modalon narrow layout -dismissModalWithReportreturnswillOpenReport=falseunexpectedly (5bee564c)search_dismissmodal stays open after successfulRequestMoneyAPI call (1ffba096)What this PR adds:
snapshot.*- records the fullgetSubmitHandlerdecision tree inputs so handler selection can be replayed from production spansat_dismiss.*- captures split navigator and tab state at dismiss time, includingform_has_been_submittedto detect the cleanup race in pre-insert flowsdismiss_narrow.*- records root route count, topmost fullscreen route, andwill_open_reportto explain narrow dismiss decisionssearch_dismiss.*- addsafter_transition_calledandelapsed_msto distinguish "dismiss was slow" from "create flow stayed open"at_end.*- confirms whether the user landed on the expected destination reportSearchMoneyRequestReportPage(direct fix for issue 3)Fixed Issues
$
PROPOSAL: N/A
Tests
ManualSubmitToDestinationVisiblespansnapshot.*attributes (e.g.snapshot.is_search_topmost_fullscreen: true),search_dismiss.*attributes, andat_end.*attributesdismiss_narrow.*attributes (e.g.dismiss_narrow.will_open_report,dismiss_narrow.root_route_count)fast_path_handlerandat_end.*attributes are presentOffline tests
N/A - changes are purely additive telemetry attributes on an existing Sentry span. No API calls, data fetching, or Onyx operations are modified.
QA Steps
Same as tests - verify expense submission works correctly on all platforms. The diagnostic span attributes are only visible in Sentry/dev tools and do not affect user-facing behavior.
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))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