[NoQA] Add report_action_count and money_request_preview_count to ManualSendMessage - #96285
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.
|
![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: 476cb54d40
ℹ️ 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".
| const reportActions = Object.values(getAllReportActions(reportID)); | ||
| const moneyRequestPreviewCount = reportActions.filter(isReportPreviewAction).length; |
There was a problem hiding this comment.
When a chat has a large action list (the scenario this dimension is meant to isolate), Object.values(getAllReportActions(reportID)) and the preview filter run synchronously after the send press but before ManualSendMessage is started. That work also delays addComment, so user-visible send latency grows with list size while the span under-reports it; capture the span start time before the scan or start the span first and set these attributes afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The span now starts before counting, so the count work is inside the measured window
| const reportActions = Object.values(getAllReportActions(reportID)); | ||
| const moneyRequestPreviewCount = reportActions.filter(isReportPreviewAction).length; |
There was a problem hiding this comment.
When a report contains hidden/deleted money-request previews (for example REPORT_PREVIEW actions with shouldShow === false and no pending action), getAllReportActions() still returns them even though the report-view visibility helpers remove them from the rendered action list. This stamps money_request_preview_count and report_action_count as high for chats that no longer render those preview cards, which makes the new Sentry grouping misrepresent the list weight the span is meant to measure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, counts now come off getSortedReportActionsForDisplay, so hidden/deleted previews are excluded. Also added unit tests
| * `MoneyRequestReportPreview` items the chat actually renders. | ||
| */ | ||
| function getSendMessageListWeight(reportActions: ReportActions, canUserPerformWriteAction?: boolean): SendMessageListWeight { | ||
| const visibleReportActions = getSortedReportActionsForDisplay(reportActions, canUserPerformWriteAction); |
There was a problem hiding this comment.
question: Should we pass reportID to getSortedReportActionsForDisplay ? ReportAction.reportID is optional, and the function excludes actions when neither the action nor the argument reportID provides one.
There was a problem hiding this comment.
Good catch! Passing reportID now so those actions don't get dropped.
| }); | ||
|
|
||
| const {reportActionCount, moneyRequestPreviewCount} = getSendMessageListWeight(getAllReportActions(reportID), canUserPerformWriteAction(report, isReportArchived)); | ||
| getSpan(spanID)?.setAttributes({ |
There was a problem hiding this comment.
nab: The test instructions expect these attributes in the "Starting span" log, but startSpan() logs its options before this code runs. Could we compute the counts first and include them directly in startSpan(...).attributes?
There was a problem hiding this comment.
Makes sense, moved the counts up
| }, | ||
| }); | ||
|
|
||
| const {reportActionCount, moneyRequestPreviewCount} = getSendMessageListWeight(getAllReportActions(reportID), canUserPerformWriteAction(report, isReportArchived)); |
There was a problem hiding this comment.
question: I am not 100% sure about this. But wanted to clarify. Should these counts use the currently paginated report actions instead of all cached actions? Otherwise, they may include previews not present in the rendered list.
There was a problem hiding this comment.
I checked it and decided to keep counting all cached actions rather than the paginated list, and here's the thinking: the goal is just to have a rough "how heavy is this chat" bucket so we can group send latency in Sentry, not an exact count of what's on screen. On top of that, we only start this span when the user is scrolled to the bottom, and at that point the cached actions and what's actually mounted are basically the same, so the difference is tiny in real usage.
Going with the paginated chain would mean wiring pagination into the composer just to shave off that small gap, which didn't feel worth it. I did update the doc comment so this scope is clear and we're not claiming it matches the rendered list exactly
|
Reviewing this again. |
| import type {ReportActions} from '@src/types/onyx'; | ||
|
|
||
| type SendMessageListWeight = { | ||
| /** Number of renderable actions in the report (display-filtered, matching what the report view shows). */ |
There was a problem hiding this comment.
nit: As we're counting cached actions. So may be?
| /** Number of renderable actions in the report (display-filtered, matching what the report view shows). */ | |
| /** Number of renderable actions in the report (display-filtered). */ |
|
Code changes look fine, working on the checklist. |
…-preview-count-attributes # Conflicts: # src/pages/inbox/report/ReportActionCompose/useComposerSubmit.ts
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb-preview-count.mov |
|
🚧 cristipaval has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.4.44-0 🚀
|
No help site changes requiredI reviewed the changes in this PR and no updates to Expensify's help site files ( Why: This PR is a purely internal observability change. It adds two dimensional attributes —
There is no change to any user-facing feature, UI label, setting, workflow, or product behavior that a customer would see or that the public help site documents. Help site articles cover customer-facing product guidance, and nothing in this PR alters that surface. If you believe there's a customer-facing aspect I missed, reply with |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.44-6 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
This PR adds two dimensional attributes to the
ManualSendMessagespan:report_action_count— total actions in the chat's listmoney_request_preview_count— how many of them areREPORT_PREVIEWitemsWith these we can group
ManualSendMessagein Sentry by preview count , to see how send time changes as previews grow, confirm the fixes helped, and catch regressions in preview-heavy chats.Fixed Issues
$ #96379
PROPOSAL:
Tests
ManualSendMessage_and verify the "Starting span" log includesreport_action_countandmoney_request_preview_count(alongsidesend_message_source),and that the values match the number of actions / report previews in the chat.
Offline tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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