[No QA] Fix ManualSendMessage metrics measurements#82321
[No QA] Fix ManualSendMessage metrics measurements#82321cristipaval wants to merge 10 commits intomainfrom
Conversation
melvin-bot
bot
requested review from
JmillsExpensify and
MonilBhavsar
and removed request for
a team
|
@MonilBhavsar 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: 87b4c9717a
ℹ️ 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".
| }); | ||
| onSubmit(newCommentTrimmed); | ||
| const reportActionID = rand64(); | ||
| const isScrolledToBottom = !scrollPosition?.offset || scrollPosition.offset < CONST.REPORT.ACTIONS.ACTION_VISIBLE_THRESHOLD; |
There was a problem hiding this comment.
The new isScrolledToBottom check depends on scrollPosition.offset, but this value is not wired to chat scroll events in this flow (repo-wide search shows setScrollPosition is only written in useReportScrollManager/index.native.ts, where it is reset to 0 during scrollToBottom). That makes the condition effectively always true (undefined/0), so spans are still started while the user is scrolled up and the off-screen orphan-span case this change targets still occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
looking into it
There was a problem hiding this comment.
ok, changed the approach for getting the current scroll position
| Performance.markStart(CONST.TIMING.SEND_MESSAGE, {message: newCommentTrimmed}); | ||
| startSpan(`${CONST.TELEMETRY.SPAN_SEND_MESSAGE}_${reportActionID}`, { |
There was a problem hiding this comment.
This starts a send-message span before calling onSubmit, but onSubmitComment can return early when handleCreateTask(text) succeeds in ReportFooter, so no comment with that reportActionID is created and TextCommentFragment never calls endSpan for it. In that task-creation path the span is only canceled later during screen cleanup, which produces canceled/orphan send-message telemetry samples.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think that's fine, we only want to measure the text messages
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.
|
| const [childReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${linkedAction?.childReportID}`, {canBeMissing: true}); | ||
|
|
||
| const [isBannerVisible, setIsBannerVisible] = useState(true); | ||
| const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({}); |
There was a problem hiding this comment.
I moved the above state into a reusable hook to use on this page and in SearchMoneyRequestReportPage.tsx.
|
I can help if this requires C+ review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5988646ae
ℹ️ 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 isScrolledToBottom = scrollOffsetRef.current < CONST.REPORT.ACTIONS.ACTION_VISIBLE_THRESHOLD; | ||
| if (isScrolledToBottom) { | ||
| Performance.markStart(CONST.TIMING.SEND_MESSAGE, {message: newCommentTrimmed}); | ||
| startSpan(`${CONST.TELEMETRY.SPAN_SEND_MESSAGE}_${reportActionID}`, { |
There was a problem hiding this comment.
startSpan() is called before onSubmit(), but onSubmit can short-circuit without posting a comment (for example, ReportFooter.onSubmitComment() returns early when handleCreateTask(text) is true in src/pages/inbox/report/ReportFooter.tsx). In that flow no report action with this generated reportActionID is created, so TextCommentFragment never calls endSpan(...) for it; the span stays active until a later screen teardown and inflates send-message telemetry during long sessions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we cleanup when the user moves away from the report page, so we won't have inflated spans
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
@mkhutornyi I do think this one would benefit from a C+ review, thanks! |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f5a0fd970
ℹ️ 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 reportActionID = rand64(); | ||
|
|
||
| // The list is inverted, so an offset near 0 means the user is at the bottom (newest messages visible). | ||
| const isScrolledToBottom = scrollOffsetRef.current < CONST.REPORT.ACTIONS.ACTION_VISIBLE_THRESHOLD; |
There was a problem hiding this comment.
isScrolledToBottom depends on scrollOffsetRef.current, but that ref is only written in src/pages/inbox/report/ReportActionsList.tsx (I verified via rg "scrollOffsetRef.current ="), not in src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx which is used when shouldDisplayMoneyRequestActionsList is true in ReportScreen. In those money-request reports the value remains its default 0, so this condition still starts ManualSendMessage_<actionID> spans even when the user is scrolled away from the latest message; since spans are now ended by exact reportActionID, off-screen sent messages can remain open until navigation cleanup and skew send-message telemetry.
Useful? React with 👍 / 👎.
| const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({}); | ||
| const scrollOffsetRef = useRef(0); | ||
|
|
||
| return useMemo(() => ({flatListRef, scrollPosition, setScrollPosition, scrollOffsetRef}), [scrollPosition]); |
There was a problem hiding this comment.
This file is compiled by React Compiler, so we don't need useMemo here. Let's remove it since it's not needed
| file, | ||
| isInSidePanel = false, | ||
| pregeneratedResponseParams, | ||
| reportActionID, |
There was a problem hiding this comment.
question: Would an alternative here be to return the reportActionID for the newly created actions? that way it doesn't have to be passed in. NAB
| } | ||
|
|
||
| function cancelSpansByPrefix(prefix: string) { | ||
| for (const [spanId] of activeSpans.entries()) { |
There was a problem hiding this comment.
Per code style conventions:
| for (const [spanId] of activeSpans.entries()) { | |
| for (const [spanID] of activeSpans.entries()) { |
|
|
||
| function useActionListContextValue(): ActionListContextType { | ||
| const flatListRef = useRef<FlatList>(null); | ||
| const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({}); |
There was a problem hiding this comment.
Looks like we are misusing state here: scrollPosition is an observed property that can change very frequently. Calling setScrollPosition isn't intended to cause re-renders - the list itself should control its own scroll position.
Created a plan to fix it.
Refactor Scroll State Management
Problem Summary
Two inefficiencies exist in the scroll position tracking code:
- Unused state causing re-renders:
[scrollPosition](src/hooks/useActionListContextValue.ts)is state but never read - only written viasetScrollPosition. Every write triggers unnecessary re-renders. - Duplicate scroll tracking:
[ReportActionsList.tsx](src/pages/inbox/report/ReportActionsList.tsx)maintains TWO refs tracking the same scroll offset:
- Local
scrollingVerticalOffset(line 219) - Context
scrollOffsetRef(line 176) - Both updated with identical values (lines 369-370)
Changes Required
1. Convert scrollPosition from state to ref
**File: src/hooks/useActionListContextValue.ts**
Change from:
const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({});To:
const scrollPositionRef = useRef<ScrollPosition>({});Update return statement to remove setScrollPosition and use scrollPositionRef.
2. Update type definitions
**File: src/pages/inbox/ReportScreenContext.ts**
Change ActionListContextType:
type ActionListContextType = {
flatListRef: FlatListRefType;
scrollPositionRef: RefObject<ScrollPosition>; // Changed from scrollPosition/setScrollPosition
scrollOffsetRef: RefObject<number>;
};Update default context value accordingly.
3. Update useReportScrollManager
**File: src/hooks/useReportScrollManager/index.native.ts**
Change from:
const {flatListRef, setScrollPosition} = useContext(ActionListContext);To:
const {flatListRef, scrollPositionRef} = useContext(ActionListContext);Update scrollToBottom callback (line 33) to use ref:
scrollPositionRef.current = {offset: 0};4. Eliminate duplicate scroll tracking in ReportActionsList
**File: src/pages/inbox/report/ReportActionsList.tsx**
Remove local scrollingVerticalOffset ref declaration (line 219).
Replace all usages with scrollOffsetRef:
- Line 298: Pass
scrollOffsetRef.currenttogetUnreadMarkerIndex - Line 364: Pass
scrollOffsetReftouseReportUnreadMessageScrollTracking - Line 369: Remove
scrollingVerticalOffset.current = ...(keep onlyscrollOffsetRef.current = ...) - Line 380: Change condition to use
scrollOffsetRef.current
Files Modified
- src/hooks/useActionListContextValue.ts - Convert state to ref
- src/pages/inbox/ReportScreenContext.ts - Update types
- src/hooks/useReportScrollManager/index.native.ts - Use ref instead of setter
- src/pages/inbox/report/ReportActionsList.tsx - Remove duplicate ref
Performance Benefits
- Eliminates unnecessary re-renders when scroll position changes (no longer triggers state updates)
- Reduces memory overhead by removing duplicate ref tracking
- Improves code clarity with single source of truth for scroll offset
diff
diff --git a/src/hooks/useActionListContextValue.ts b/src/hooks/useActionListContextValue.ts
index 7f4c1d1a6b7..ba73ff804cc 100644
--- a/src/hooks/useActionListContextValue.ts
+++ b/src/hooks/useActionListContextValue.ts
@@ -1,13 +1,13 @@
-import {useMemo, useRef, useState} from 'react';
+import {useRef} from 'react';
import type {FlatList} from 'react-native';
import type {ActionListContextType, ScrollPosition} from '@pages/inbox/ReportScreenContext';
function useActionListContextValue(): ActionListContextType {
const flatListRef = useRef<FlatList>(null);
- const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({});
+ const scrollPositionRef = useRef<ScrollPosition>({});
const scrollOffsetRef = useRef(0);
- return useMemo(() => ({flatListRef, scrollPosition, setScrollPosition, scrollOffsetRef}), [scrollPosition]);
+ return {flatListRef, scrollPositionRef, scrollOffsetRef};
}
export default useActionListContextValue;
diff --git a/src/hooks/useReportScrollManager/index.native.ts b/src/hooks/useReportScrollManager/index.native.ts
index 39e5a1b66ee..180196dd0b3 100644
--- a/src/hooks/useReportScrollManager/index.native.ts
+++ b/src/hooks/useReportScrollManager/index.native.ts
@@ -1,44 +1,39 @@
-import {useCallback, useContext} from 'react';
+import {useContext} from 'react';
// eslint-disable-next-line no-restricted-imports
import type {ScrollView} from 'react-native';
import {ActionListContext} from '@pages/inbox/ReportScreenContext';
import type ReportScrollManagerData from './types';
function useReportScrollManager(): ReportScrollManagerData {
- const {flatListRef, setScrollPosition} = useContext(ActionListContext);
+ const {flatListRef, scrollPositionRef} = useContext(ActionListContext);
/**
* Scroll to the provided index.
*/
- const scrollToIndex = useCallback(
- (index: number) => {
- if (!flatListRef?.current) {
- return;
- }
-
- flatListRef.current.scrollToIndex({index});
- },
- [flatListRef],
- );
+ const scrollToIndex = (index: number) => {
+ if (!flatListRef?.current) {
+ return;
+ }
+ flatListRef.current.scrollToIndex({index});
+ };
/**
* Scroll to the bottom of the inverted FlatList.
* When FlatList is inverted it's "bottom" is really it's top
*/
- const scrollToBottom = useCallback(() => {
+ const scrollToBottom = () => {
if (!flatListRef?.current) {
return;
}
- setScrollPosition({offset: 0});
-
+ scrollPositionRef.current = {offset: 0};
flatListRef.current?.scrollToOffset({animated: false, offset: 0});
- }, [flatListRef, setScrollPosition]);
+ };
/**
* Scroll to the end of the FlatList.
*/
- const scrollToEnd = useCallback(() => {
+ const scrollToEnd = () => {
if (!flatListRef?.current) {
return;
}
@@ -51,18 +46,14 @@ function useReportScrollManager(): ReportScrollManagerData {
}
flatListRef.current.scrollToEnd({animated: false});
- }, [flatListRef]);
+ };
- const scrollToOffset = useCallback(
- (offset: number) => {
- if (!flatListRef?.current) {
- return;
- }
-
- flatListRef.current.scrollToOffset({offset, animated: false});
- },
- [flatListRef],
- );
+ const scrollToOffset = (offset: number) => {
+ if (!flatListRef?.current) {
+ return;
+ }
+ flatListRef.current.scrollToOffset({offset, animated: false});
+ };
return {ref: flatListRef, scrollToIndex, scrollToBottom, scrollToEnd, scrollToOffset};
}
diff --git a/src/hooks/useReportScrollManager/index.ts b/src/hooks/useReportScrollManager/index.ts
index 30c383e62c2..6b888584887 100644
--- a/src/hooks/useReportScrollManager/index.ts
+++ b/src/hooks/useReportScrollManager/index.ts
@@ -1,4 +1,4 @@
-import {useCallback, useContext} from 'react';
+import {useContext} from 'react';
import {ActionListContext} from '@pages/inbox/ReportScreenContext';
import type ReportScrollManagerData from './types';
@@ -8,50 +8,44 @@ function useReportScrollManager(): ReportScrollManagerData {
/**
* Scroll to the provided index. On non-native implementations we do not want to scroll when we are scrolling because
*/
- const scrollToIndex = useCallback(
- (index: number, isEditing?: boolean) => {
- if (!flatListRef?.current || isEditing) {
- return;
- }
+ const scrollToIndex = (index: number, isEditing?: boolean) => {
+ if (!flatListRef?.current || isEditing) {
+ return;
+ }
- flatListRef.current.scrollToIndex({index, animated: true});
- },
- [flatListRef],
- );
+ flatListRef.current.scrollToIndex({index, animated: true});
+ };
/**
* Scroll to the bottom of the inverted FlatList.
* When FlatList is inverted it's "bottom" is really it's top
*/
- const scrollToBottom = useCallback(() => {
+ const scrollToBottom = () => {
if (!flatListRef?.current) {
return;
}
flatListRef.current.scrollToOffset({animated: false, offset: 0});
- }, [flatListRef]);
+ };
/**
* Scroll to the end of the FlatList.
*/
- const scrollToEnd = useCallback(() => {
+ const scrollToEnd = () => {
if (!flatListRef?.current) {
return;
}
flatListRef.current.scrollToEnd({animated: false});
- }, [flatListRef]);
-
- const scrollToOffset = useCallback(
- (offset: number) => {
- if (!flatListRef?.current) {
- return;
- }
-
- flatListRef.current.scrollToOffset({animated: true, offset});
- },
- [flatListRef],
- );
+ };
+
+ const scrollToOffset = (offset: number) => {
+ if (!flatListRef?.current) {
+ return;
+ }
+
+ flatListRef.current.scrollToOffset({animated: true, offset});
+ };
return {ref: flatListRef, scrollToIndex, scrollToBottom, scrollToEnd, scrollToOffset};
}
diff --git a/src/pages/inbox/ReportScreenContext.ts b/src/pages/inbox/ReportScreenContext.ts
index ec56c097eee..50b1e3e707f 100644
--- a/src/pages/inbox/ReportScreenContext.ts
+++ b/src/pages/inbox/ReportScreenContext.ts
@@ -19,13 +19,12 @@ type ScrollPosition = {offset?: number};
type ActionListContextType = {
flatListRef: FlatListRefType;
- scrollPosition: ScrollPosition | null;
- setScrollPosition: (position: {offset: number}) => void;
+ scrollPositionRef: RefObject<ScrollPosition>;
scrollOffsetRef: RefObject<number>;
};
type ReactionListContextType = RefObject<ReactionListRef | null> | null;
-const ActionListContext = createContext<ActionListContextType>({flatListRef: null, scrollPosition: null, setScrollPosition: () => {}, scrollOffsetRef: {current: 0}});
+const ActionListContext = createContext<ActionListContextType>({flatListRef: null, scrollPositionRef: {current: {}}, scrollOffsetRef: {current: 0}});
const ReactionListContext = createContext<ReactionListContextType>(null);
export {ActionListContext, ReactionListContext};
diff --git a/src/pages/inbox/report/ReportActionsList.tsx b/src/pages/inbox/report/ReportActionsList.tsx
index 2640683af9e..04016ee50fc 100644
--- a/src/pages/inbox/report/ReportActionsList.tsx
+++ b/src/pages/inbox/report/ReportActionsList.tsx
@@ -216,7 +216,6 @@ function ReportActionsList({
return unsubscribe;
}, []);
- const scrollingVerticalOffset = useRef(0);
const readActionSkipped = useRef(false);
const hasHeaderRendered = useRef(false);
const linkedReportActionID = route?.params?.reportActionID;
@@ -295,7 +294,7 @@ function ReportActionsList({
currentUserAccountID,
prevSortedVisibleReportActionsObjects,
unreadMarkerTime,
- scrollingVerticalOffset: scrollingVerticalOffset.current,
+ scrollingVerticalOffset: scrollOffsetRef.current,
prevUnreadMarkerReportActionID: prevUnreadMarkerReportActionID.current,
});
if (shouldDisplayNewMarker) {
@@ -361,12 +360,11 @@ function ReportActionsList({
const {isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible, trackVerticalScrolling, onViewableItemsChanged} = useReportUnreadMessageScrollTracking({
reportID: report.reportID,
- currentVerticalScrollingOffsetRef: scrollingVerticalOffset,
+ currentVerticalScrollingOffsetRef: scrollOffsetRef,
readActionSkippedRef: readActionSkipped,
unreadMarkerReportActionIndex,
isInverted: true,
onTrackScrolling: (event: NativeSyntheticEvent<NativeScrollEvent>) => {
- scrollingVerticalOffset.current = event.nativeEvent.contentOffset.y;
scrollOffsetRef.current = event.nativeEvent.contentOffset.y;
onScroll?.(event);
if (shouldScrollToEndAfterLayout && (!hasCreatedActionAdded || isOffline)) {
@@ -377,7 +375,7 @@ function ReportActionsList({
useEffect(() => {
if (
- scrollingVerticalOffset.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
+ scrollOffsetRef.current < AUTOSCROLL_TO_TOP_THRESHOLD &&
previousLastIndex.current !== lastActionIndex &&
reportActionSize.current !== sortedVisibleReportActions.length &&
hasNewestReportAction
diff --git a/tests/perf-test/ReportActionsList.perf-test.tsx b/tests/perf-test/ReportActionsList.perf-test.tsx
index 309e5c289b4..72122f8bf02 100644
--- a/tests/perf-test/ReportActionsList.perf-test.tsx
+++ b/tests/perf-test/ReportActionsList.perf-test.tsx
@@ -70,7 +70,7 @@ beforeAll(() =>
const mockOnLayout = jest.fn();
const mockOnScroll = jest.fn();
const mockLoadChats = jest.fn();
-const mockRef = {current: null, flatListRef: null, scrollPosition: null, setScrollPosition: () => {}, scrollOffsetRef: {current: 0}};
+const mockRef = {current: null, flatListRef: null, scrollPositionRef: {current: {}}, scrollOffsetRef: {current: 0}};
const TEST_USER_ACCOUNT_ID = 1;
const TEST_USER_LOGIN = 'test@test.com';4 participants
The ManualSendMessage Sentry span was unreliable — it used a single global span ID, so any TextCommentFragment rendering (incoming messages, other pane re-renders) could end someone else's span. If the user scrolled up when sending, the message rendered off-screen, and the span could stay open for tens of seconds until something else accidentally closed it.
Explanation of Change
reportActionIDin its ID (ManualSendMessage_), so only the fragment for that exact message can end it.ActionListContextvalue creation into the newuseActionListContextValuehook, removing boilerplate fromReportScreenandSearchMoneyRequestReportPage. Added thescrollOffsetRefto it to help with the previous bullet.cancelSpansByPrefixto clean up any orphanedsend-messagespans when navigating away from a report.Fixed Issues
Part of #77176
PROPOSAL:
Tests
Tested that message sending works as expected.
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