-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[No QA] Fix ManualSendMessage metrics measurements #82321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
e049759
8b06a71
447467a
e79e241
87b4c97
506c5a5
f756382
567daab
b598864
9f5a0fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| import {useMemo, useRef, useState} 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 scrollOffsetRef = useRef(0); | ||
|
|
||
| return useMemo(() => ({flatListRef, scrollPosition, setScrollPosition, scrollOffsetRef}), [scrollPosition]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is compiled by React Compiler, so we don't need |
||
| } | ||
|
|
||
| export default useActionListContextValue; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -280,6 +280,7 @@ | |
| shouldPlaySound?: boolean; | ||
| isInSidePanel?: boolean; | ||
| pregeneratedResponseParams?: PregeneratedResponseParams; | ||
| reportActionID?: string; | ||
| }; | ||
|
|
||
| type AddActionsParams = { | ||
|
|
@@ -292,6 +293,7 @@ | |
| file?: FileObject; | ||
| isInSidePanel?: boolean; | ||
| pregeneratedResponseParams?: PregeneratedResponseParams; | ||
| reportActionID?: string; | ||
| }; | ||
|
|
||
| type AddAttachmentWithCommentParams = { | ||
|
|
@@ -312,7 +314,7 @@ | |
| /** @deprecated This value is deprecated and will be removed soon after migration. Use the email from useCurrentUserPersonalDetails hook instead. */ | ||
| let deprecatedCurrentUserLogin: string | undefined; | ||
|
|
||
| Onyx.connect({ | ||
| key: ONYXKEYS.SESSION, | ||
| callback: (value) => { | ||
| // When signed out, val is undefined | ||
|
|
@@ -326,7 +328,7 @@ | |
| }, | ||
| }); | ||
|
|
||
| Onyx.connect({ | ||
| key: ONYXKEYS.CONCIERGE_REPORT_ID, | ||
| callback: (value) => (conciergeReportIDOnyxConnect = value), | ||
| }); | ||
|
|
@@ -334,7 +336,7 @@ | |
| // map of reportID to all reportActions for that report | ||
| const allReportActions: OnyxCollection<ReportActions> = {}; | ||
|
|
||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT_ACTIONS, | ||
| callback: (actions, key) => { | ||
| if (!key || !actions) { | ||
|
|
@@ -346,7 +348,7 @@ | |
| }); | ||
|
|
||
| let allReports: OnyxCollection<Report>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.COLLECTION.REPORT, | ||
| waitForCollectionCallback: true, | ||
| callback: (value) => { | ||
|
|
@@ -355,7 +357,7 @@ | |
| }); | ||
|
|
||
| let allPersonalDetails: OnyxEntry<PersonalDetailsList> = {}; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||
| callback: (value) => { | ||
| allPersonalDetails = value ?? {}; | ||
|
|
@@ -370,7 +372,7 @@ | |
| }); | ||
|
|
||
| let onboarding: OnyxEntry<Onboarding>; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.NVP_ONBOARDING, | ||
| callback: (val) => { | ||
| if (Array.isArray(val)) { | ||
|
|
@@ -381,7 +383,7 @@ | |
| }); | ||
|
|
||
| let deprecatedIntroSelected: OnyxEntry<IntroSelected> = {}; | ||
| Onyx.connect({ | ||
| key: ONYXKEYS.NVP_INTRO_SELECTED, | ||
| callback: (val) => (deprecatedIntroSelected = val), | ||
| }); | ||
|
|
@@ -609,7 +611,18 @@ | |
| * @param isInSidePanel - Whether the comment is being added from the side panel | ||
| * @param pregeneratedResponseParams - Optional params for pre-generated response (API only, no optimistic action - used when response display is delayed) | ||
| */ | ||
| function addActions({report, notifyReportID, ancestors, timezoneParam, currentUserAccountID, text = '', file, isInSidePanel = false, pregeneratedResponseParams}: AddActionsParams) { | ||
| function addActions({ | ||
| report, | ||
| notifyReportID, | ||
| ancestors, | ||
| timezoneParam, | ||
| currentUserAccountID, | ||
| text = '', | ||
| file, | ||
| isInSidePanel = false, | ||
| pregeneratedResponseParams, | ||
| reportActionID, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Would an alternative here be to return the |
||
| }: AddActionsParams) { | ||
| if (!report?.reportID) { | ||
| return; | ||
| } | ||
|
|
@@ -620,7 +633,7 @@ | |
| let commandName: typeof WRITE_COMMANDS.ADD_COMMENT | typeof WRITE_COMMANDS.ADD_ATTACHMENT | typeof WRITE_COMMANDS.ADD_TEXT_AND_ATTACHMENT = WRITE_COMMANDS.ADD_COMMENT; | ||
|
|
||
| if (text && !file) { | ||
| const reportComment = buildOptimisticAddCommentReportAction(text, undefined, undefined, undefined, reportID); | ||
| const reportComment = buildOptimisticAddCommentReportAction(text, undefined, undefined, undefined, reportID, reportActionID); | ||
| reportCommentAction = reportComment.reportAction; | ||
| reportCommentText = reportComment.commentText; | ||
| } | ||
|
|
@@ -848,11 +861,22 @@ | |
| } | ||
|
|
||
| /** Add a single comment to a report */ | ||
| function addComment({report, notifyReportID, ancestors, text, timezoneParam, currentUserAccountID, shouldPlaySound, isInSidePanel, pregeneratedResponseParams}: AddCommentParams) { | ||
| function addComment({ | ||
| report, | ||
| notifyReportID, | ||
| ancestors, | ||
| text, | ||
| timezoneParam, | ||
| currentUserAccountID, | ||
| shouldPlaySound, | ||
| isInSidePanel, | ||
| pregeneratedResponseParams, | ||
| reportActionID, | ||
| }: AddCommentParams) { | ||
| if (shouldPlaySound) { | ||
| playSound(SOUNDS.DONE); | ||
| } | ||
| addActions({report, notifyReportID, ancestors, timezoneParam, currentUserAccountID, text, isInSidePanel, pregeneratedResponseParams}); | ||
| addActions({report, notifyReportID, ancestors, timezoneParam, currentUserAccountID, text, isInSidePanel, pregeneratedResponseParams, reportActionID}); | ||
| } | ||
|
|
||
| function reportActionsExist(reportID: string): boolean { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -52,8 +52,16 @@ function cancelAllSpans() { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| function cancelSpansByPrefix(prefix: string) { | ||||||
| for (const [spanId] of activeSpans.entries()) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per code style conventions:
Suggested change
|
||||||
| if (spanId.startsWith(prefix)) { | ||||||
| cancelSpan(spanId); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| function getSpan(spanId: string) { | ||||||
| return activeSpans.get(spanId); | ||||||
| } | ||||||
|
|
||||||
| export {startSpan, endSpan, getSpan, cancelSpan, cancelAllSpans}; | ||||||
| export {startSpan, endSpan, getSpan, cancelSpan, cancelAllSpans, cancelSpansByPrefix}; | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ import {useFocusEffect, useIsFocused} from '@react-navigation/native'; | |
| import {accountIDSelector} from '@selectors/Session'; | ||
| import {deepEqual} from 'fast-equals'; | ||
| import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import type {FlatList, ViewStyle} from 'react-native'; | ||
| import type {ViewStyle} from 'react-native'; | ||
| // We use Animated for all functionality related to wide RHP to make it easier | ||
| // to interact with react-navigation components (e.g., CardContainer, interpolator), which also use Animated. | ||
| // eslint-disable-next-line no-restricted-imports | ||
|
|
@@ -22,6 +22,7 @@ import ScreenWrapper from '@components/ScreenWrapper'; | |
| import ScrollView from '@components/ScrollView'; | ||
| import useShowWideRHPVersion from '@components/WideRHPContextProvider/useShowWideRHPVersion'; | ||
| import WideRHPOverlayWrapper from '@components/WideRHPOverlayWrapper'; | ||
| import useActionListContextValue from '@hooks/useActionListContextValue'; | ||
| import useAppFocusEvent from '@hooks/useAppFocusEvent'; | ||
| import useArchivedReportsIdSet from '@hooks/useArchivedReportsIdSet'; | ||
| import {useCurrentReportIDState} from '@hooks/useCurrentReportID'; | ||
|
|
@@ -87,7 +88,7 @@ import { | |
| isTaskReport, | ||
| isValidReportIDFromPath, | ||
| } from '@libs/ReportUtils'; | ||
| import {cancelSpan} from '@libs/telemetry/activeSpans'; | ||
| import {cancelSpan, cancelSpansByPrefix} from '@libs/telemetry/activeSpans'; | ||
| import {isNumeric} from '@libs/ValidationUtils'; | ||
| import type {ReportsSplitNavigatorParamList, RightModalNavigatorParamList} from '@navigation/types'; | ||
| import {setShouldShowComposeInput} from '@userActions/Composer'; | ||
|
|
@@ -113,7 +114,6 @@ import useReportWasDeleted from './hooks/useReportWasDeleted'; | |
| import ReactionListWrapper from './ReactionListWrapper'; | ||
| import ReportActionsView from './report/ReportActionsView'; | ||
| import ReportFooter from './report/ReportFooter'; | ||
| import type {ActionListContextType, ScrollPosition} from './ReportScreenContext'; | ||
| import {ActionListContext} from './ReportScreenContext'; | ||
|
|
||
| type ReportScreenNavigationProps = | ||
|
|
@@ -167,7 +167,6 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr | |
| const prevIsFocused = usePrevious(isFocused); | ||
| const [firstRender, setFirstRender] = useState(true); | ||
| const isSkippingOpenReport = useRef(false); | ||
| const flatListRef = useRef<FlatList>(null); | ||
| const hasCreatedLegacyThreadRef = useRef(false); | ||
| const {isBetaEnabled} = usePermissions(); | ||
| const {isOffline} = useNetwork(); | ||
|
|
@@ -317,7 +316,6 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr | |
| const [childReport] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${linkedAction?.childReportID}`, {canBeMissing: true}); | ||
|
|
||
| const [isBannerVisible, setIsBannerVisible] = useState(true); | ||
| const [scrollPosition, setScrollPosition] = useState<ScrollPosition>({}); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I moved the above state into a reusable hook to use on this page and in |
||
|
|
||
| const viewportOffsetTop = useViewportOffsetTop(); | ||
|
|
||
|
|
@@ -652,6 +650,9 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr | |
|
|
||
| // We need to cancel telemetry span when user leaves the screen before full report data is loaded | ||
| cancelSpan(`${CONST.TELEMETRY.SPAN_OPEN_REPORT}_${reportID}`); | ||
|
|
||
| // Cancel any pending send-message spans to prevent orphaned spans when navigating away | ||
| cancelSpansByPrefix(CONST.TELEMETRY.SPAN_SEND_MESSAGE); | ||
cristipaval marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
| }, [reportID]); | ||
|
|
||
|
|
@@ -876,7 +877,7 @@ function ReportScreen({route, navigation, isInSidePanel = false}: ReportScreenPr | |
| }; | ||
| }, [report?.reportID, didSubscribeToReportLeavingEvents, reportIDFromRoute, report?.pendingFields, currentUserAccountID]); | ||
|
|
||
| const actionListValue = useMemo((): ActionListContextType => ({flatListRef, scrollPosition, setScrollPosition}), [flatListRef, scrollPosition, setScrollPosition]); | ||
| const actionListValue = useActionListContextValue(); | ||
|
|
||
| // This helps in tracking from the moment 'route' triggers useMemo until isLoadingInitialReportActions becomes true. It prevents blinking when loading reportActions from cache. | ||
| useEffect(() => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| import lodashDebounce from 'lodash/debounce'; | ||
| import noop from 'lodash/noop'; | ||
| import React, {memo, useCallback, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import React, {memo, useCallback, useContext, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import type {BlurEvent, MeasureInWindowOnSuccessCallback, TextInputSelectionChangeEvent} from 'react-native'; | ||
| import {View} from 'react-native'; | ||
| import type {OnyxEntry} from 'react-native-onyx'; | ||
|
|
@@ -38,6 +38,7 @@ import {canUseTouchScreen} from '@libs/DeviceCapabilities'; | |
| import DomUtils from '@libs/DomUtils'; | ||
| import FS from '@libs/Fullstory'; | ||
| import getNonEmptyStringOnyxID from '@libs/getNonEmptyStringOnyxID'; | ||
| import {rand64} from '@libs/NumberUtils'; | ||
| import Performance from '@libs/Performance'; | ||
| import {getLinkedTransactionID, getReportAction, isMoneyRequestAction} from '@libs/ReportActionsUtils'; | ||
| import { | ||
|
|
@@ -64,6 +65,7 @@ import willBlurTextInputOnTapOutsideFunc from '@libs/willBlurTextInputOnTapOutsi | |
| import AgentZeroProcessingRequestIndicator from '@pages/inbox/report/AgentZeroProcessingRequestIndicator'; | ||
| import ParticipantLocalTime from '@pages/inbox/report/ParticipantLocalTime'; | ||
| import ReportTypingIndicator from '@pages/inbox/report/ReportTypingIndicator'; | ||
| import {ActionListContext} from '@pages/inbox/ReportScreenContext'; | ||
| import {hideEmojiPicker, isActive as isActiveEmojiPickerAction, isEmojiPickerVisible} from '@userActions/EmojiPickerAction'; | ||
| import {addAttachmentWithComment, setIsComposerFullSize} from '@userActions/Report'; | ||
| import {isBlockedFromConcierge as isBlockedFromConciergeUserAction} from '@userActions/User'; | ||
|
|
@@ -91,7 +93,7 @@ type SuggestionsRef = { | |
|
|
||
| type ReportActionComposeProps = Pick<ComposerWithSuggestionsProps, 'reportID' | 'isComposerFullSize' | 'lastReportAction'> & { | ||
| /** A method to call when the form is submitted */ | ||
| onSubmit: (newComment: string) => void; | ||
| onSubmit: (newComment: string, reportActionID?: string) => void; | ||
|
|
||
| /** The report currently being looked at */ | ||
| report: OnyxEntry<OnyxTypes.Report>; | ||
|
|
@@ -165,6 +167,8 @@ function ReportActionCompose({ | |
| canBeMissing: true, | ||
| }); | ||
| const ancestors = useAncestors(transactionThreadReport ?? report); | ||
| const {scrollOffsetRef} = useContext(ActionListContext); | ||
|
|
||
| /** | ||
| * Updates the Highlight state of the composer | ||
| */ | ||
|
|
@@ -355,16 +359,22 @@ function ReportActionCompose({ | |
| }); | ||
| attachmentFileRef.current = null; | ||
| } else { | ||
| Performance.markStart(CONST.TIMING.SEND_MESSAGE, {message: newCommentTrimmed}); | ||
| startSpan(CONST.TELEMETRY.SPAN_SEND_MESSAGE, { | ||
| name: 'send-message', | ||
| op: CONST.TELEMETRY.SPAN_SEND_MESSAGE, | ||
| attributes: { | ||
| [CONST.TELEMETRY.ATTRIBUTE_REPORT_ID]: reportID, | ||
| [CONST.TELEMETRY.ATTRIBUTE_MESSAGE_LENGTH]: newCommentTrimmed.length, | ||
| }, | ||
| }); | ||
| onSubmit(newCommentTrimmed); | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎. |
||
| if (isScrolledToBottom) { | ||
| Performance.markStart(CONST.TIMING.SEND_MESSAGE, {message: newCommentTrimmed}); | ||
| startSpan(`${CONST.TELEMETRY.SPAN_SEND_MESSAGE}_${reportActionID}`, { | ||
|
Comment on lines
+367
to
+368
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This starts a Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's fine, we only want to measure the text messages There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we cleanup when the user moves away from the report page, so we won't have inflated spans |
||
| name: 'send-message', | ||
| op: CONST.TELEMETRY.SPAN_SEND_MESSAGE, | ||
| attributes: { | ||
| [CONST.TELEMETRY.ATTRIBUTE_REPORT_ID]: reportID, | ||
| [CONST.TELEMETRY.ATTRIBUTE_MESSAGE_LENGTH]: newCommentTrimmed.length, | ||
| }, | ||
| }); | ||
| } | ||
| onSubmit(newCommentTrimmed, reportActionID); | ||
| } | ||
| }, | ||
| [ | ||
|
|
@@ -378,6 +388,7 @@ function ReportActionCompose({ | |
| personalDetail.timezone, | ||
| isInSidePanel, | ||
| onSubmit, | ||
| scrollOffsetRef, | ||
| ], | ||
| ); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are misusing state here:
scrollPositionis an observed property that can change very frequently. CallingsetScrollPositionisn'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:
[scrollPosition](src/hooks/useActionListContextValue.ts)is state but never read - only written viasetScrollPosition. Every write triggers unnecessary re-renders.[ReportActionsList.tsx](src/pages/inbox/report/ReportActionsList.tsx)maintains TWO refs tracking the same scroll offset:scrollingVerticalOffset(line 219)scrollOffsetRef(line 176)Changes Required
1. Convert scrollPosition from state to ref
**File: src/hooks/useActionListContextValue.ts**
Change from:
To:
Update return statement to remove
setScrollPositionand usescrollPositionRef.2. Update type definitions
**File: src/pages/inbox/ReportScreenContext.ts**
Change
ActionListContextType:Update default context value accordingly.
3. Update useReportScrollManager
**File: src/hooks/useReportScrollManager/index.native.ts**
Change from:
To:
Update
scrollToBottomcallback (line 33) to use ref:4. Eliminate duplicate scroll tracking in ReportActionsList
**File: src/pages/inbox/report/ReportActionsList.tsx**
Remove local
scrollingVerticalOffsetref declaration (line 219).Replace all usages with
scrollOffsetRef:scrollOffsetRef.currenttogetUnreadMarkerIndexscrollOffsetReftouseReportUnreadMessageScrollTrackingscrollingVerticalOffset.current = ...(keep onlyscrollOffsetRef.current = ...)scrollOffsetRef.currentFiles Modified
Performance Benefits
diff