From e01e0fb2ddc14542915063ecf4df16406c387c0c Mon Sep 17 00:00:00 2001 From: Christoph Pader Date: Mon, 26 Oct 2026 08:58:56 +0000 Subject: [PATCH] refactor: prepare report list refs and recycled row state Share the report list imperative contract while retaining a compatibility export for inverted FlashList. Keep row-local preview state tied to its action and let money-report edit scroll requests survive until the list commits its layout. Co-authored-by: Codex --- src/components/FlashList/types.ts | 8 +-- .../MoneyRequestReportActionsList.tsx | 19 +++++- .../MoneyRequestReportTransactionList.tsx | 10 ++- .../MoneyRequestReportUnifiedList.tsx | 19 +++--- .../LocalPDFReceiptPreview/index.tsx | 9 +-- .../ReceiptPDFOverlay/index.tsx | 4 +- src/components/ThumbnailImage.tsx | 6 +- src/pages/inbox/ActionListContext.tsx | 14 ++--- src/pages/inbox/ActionListTypes.ts | 34 +++++++++++ .../report/MoneyReportContentCreated.tsx | 1 + .../ReportActionCompose/useEditMessage.ts | 5 +- .../inbox/report/ReportActionIndexContext.tsx | 26 ++++++-- src/pages/inbox/report/ReportActionItem.tsx | 24 +++++--- .../report/ReportActionItemContentCreated.tsx | 1 + .../actionContents/ActionContentRouter.tsx | 2 +- .../actionContents/ChatMessageContent.tsx | 1 + ...equestReportActionsListRejectModalTest.tsx | 46 +++++++++++++- tests/unit/ActionListContextProviderTest.tsx | 7 +-- tests/unit/ThumbnailImageTest.tsx | 61 +++++++++++++++++++ tests/unit/hooks/useEditMessage.test.ts | 33 +++++++--- 20 files changed, 265 insertions(+), 65 deletions(-) create mode 100644 src/pages/inbox/ActionListTypes.ts create mode 100644 tests/unit/ThumbnailImageTest.tsx diff --git a/src/components/FlashList/types.ts b/src/components/FlashList/types.ts index cf7718d3d148..8563ec9959a4 100644 --- a/src/components/FlashList/types.ts +++ b/src/components/FlashList/types.ts @@ -1,7 +1,3 @@ -import type {RefObject} from 'react'; -import type {FlatList} from 'react-native'; +import type ActionListRefType from '@pages/inbox/ActionListTypes'; -/** Ref to the underlying list instance attached via `ref={}`. */ -type FlatListRefType = RefObject | null> | null; - -export default FlatListRefType; +export default ActionListRefType; diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx index 49823c8253a6..2becf033932a 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx @@ -232,6 +232,20 @@ function MoneyRequestReportActionsListContent({reportIDFromRoute, onLayout}: Mon fullPolicySnapshot: report?.policyID && fullPolicySnapshotLastModified ? {policyID: report.policyID, lastModified: fullPolicySnapshotLastModified} : undefined, }); + const [editScrollRequestID, setEditScrollRequestID] = useState(0); + const completedEditScrollRequestIDRef = useRef(0); + const requestScrollToNewestAfterEdit = () => { + // Force a layout commit even when closing the editor leaves the row's height unchanged. + setEditScrollRequestID((requestID) => requestID + 1); + }; + const onCommitLayoutEffect = () => { + if (editScrollRequestID <= completedEditScrollRequestIDRef.current) { + return; + } + completedEditScrollRequestIDRef.current = editScrollRequestID; + scrollToBottom(); + }; + // Wait for the actions to arrive before jumping, and clear the route param afterwards so a later remount // doesn't yank the user back down. useEffect(() => { @@ -251,7 +265,7 @@ function MoneyRequestReportActionsListContent({reportIDFromRoute, onLayout}: Mon const isNewestReportAction = indexWithinReportActions === visibleReportActions.length - 1; return ( - + } /> )} diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx index 23ea750eefb8..cb7daa9911ba 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportTransactionList.tsx @@ -1,5 +1,4 @@ import LinkButton from '@components/Button/composed/LinkButton'; -import type FlatListRefType from '@components/FlashList/types'; import useCopySelectionEvent from '@hooks/useCopySelectionEvent'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; @@ -18,6 +17,8 @@ import {getTransactionPendingAction} from '@libs/TransactionUtils'; import Navigation from '@navigation/Navigation'; +import type ActionListRefType from '@pages/inbox/ActionListTypes'; + import {resolveLayoutSpacing} from '@styles/layoutSpacing'; import CONST from '@src/CONST'; @@ -128,7 +129,7 @@ type MoneyRequestReportTransactionListProps = { linkedReportActionID: string | undefined; /** Ref forwarded to the underlying FlashList. */ - listRef: FlatListRefType; + listRef: ActionListRefType; /** Reports the unified list's last item index so the parent can jump to the bottom via scrollToIndex. */ onLastItemIndexChange?: (index: number) => void; @@ -139,6 +140,9 @@ type MoneyRequestReportTransactionListProps = { /** FlashList onLayout callback (distinct from the empty-state `onLayout` above). */ onListLayout: (event: LayoutChangeEvent) => void; + /** Called after FlashList finishes measuring and committing its rows. */ + onCommitLayoutEffect?: () => void; + /** FlashList onScroll callback. */ onScroll: (event: NativeSyntheticEvent) => void; @@ -189,6 +193,7 @@ function MoneyRequestReportTransactionList({ onLastItemIndexChange, accessibilityLabel, onListLayout, + onCommitLayoutEffect, onScroll, onScrollBeginDrag, onContentSizeChange, @@ -485,6 +490,7 @@ function MoneyRequestReportTransactionList({ onLastItemIndexChange={onLastItemIndexChange} accessibilityLabel={accessibilityLabel} onLayout={onListLayout} + onCommitLayoutEffect={onCommitLayoutEffect} onScroll={onScroll} onScrollBeginDrag={onScrollBeginDrag} onContentSizeChange={onContentSizeChange} diff --git a/src/components/MoneyRequestReportView/MoneyRequestReportUnifiedList.tsx b/src/components/MoneyRequestReportView/MoneyRequestReportUnifiedList.tsx index a1c1a466fc5c..44a681fdab31 100644 --- a/src/components/MoneyRequestReportView/MoneyRequestReportUnifiedList.tsx +++ b/src/components/MoneyRequestReportView/MoneyRequestReportUnifiedList.tsx @@ -1,8 +1,8 @@ import FlashList from '@components/FlashList'; -import type FlatListRefType from '@components/FlashList/types'; import useWindowDimensions from '@hooks/useWindowDimensions'; +import type ActionListRefType from '@pages/inbox/ActionListTypes'; import useScrollToEditingReportAction from '@pages/inbox/report/useScrollToEditingReportAction'; import variables from '@styles/variables'; @@ -48,15 +48,13 @@ function unifiedListItemType(item: UnifiedListItem) { } type MoneyRequestReportFlashListProps = FlashListProps & { - /** Ref to the underlying list, shared via the ActionList context (typed for the legacy FlatList). */ - ref: FlatListRefType; + /** Ref to the underlying list, shared via the common ActionList imperative API. */ + ref: ActionListRefType; }; /** - * Forwards the shared ActionList context ref to the underlying FlashList. That context slot predates this FlashList-based - * list and is still shared with the legacy report list, so it is typed for a FlatList. Mirroring InvertedFlashList, the - * ref is forwarded through @components/FlashList — which receives it as an untyped runtime prop — so no type assertion is - * needed. The scroll manager relies on the FlashList registering into this slot. + * Forwards the shared ActionList ref through @components/FlashList to the underlying list. + * FlashList implements the common imperative API consumed by the report scroll manager. */ function MoneyRequestReportFlashList(props: MoneyRequestReportFlashListProps) { return ( @@ -93,13 +91,16 @@ type MoneyRequestReportUnifiedListProps = { newTransactionID?: string; /** Ref to the underlying list, shared via the ActionList context. */ - listRef: FlatListRefType; + listRef: ActionListRefType; accessibilityLabel: string; /** Called when the list lays out. */ onLayout: (event: LayoutChangeEvent) => void; + /** Called after FlashList finishes measuring and committing its rows. */ + onCommitLayoutEffect?: () => void; + onScroll: (event: NativeSyntheticEvent) => void; /** Called when the user starts dragging the list. */ @@ -142,6 +143,7 @@ function MoneyRequestReportUnifiedList({ listRef, accessibilityLabel, onLayout, + onCommitLayoutEffect, onScroll, onScrollBeginDrag, onContentSizeChange, @@ -359,6 +361,7 @@ function MoneyRequestReportUnifiedList({ maintainVisibleContentPosition={{autoscrollToBottomThreshold: undefined}} onViewableItemsChanged={onViewableItemsChangedAdjusted} onLayout={handleLayout} + onCommitLayoutEffect={onCommitLayoutEffect} onEndReached={onEndReached} onEndReachedThreshold={0.75} onStartReached={onStartReached} diff --git a/src/components/ReportActionItem/LocalPDFReceiptPreview/index.tsx b/src/components/ReportActionItem/LocalPDFReceiptPreview/index.tsx index 27171c3e183d..84f9189259f4 100644 --- a/src/components/ReportActionItem/LocalPDFReceiptPreview/index.tsx +++ b/src/components/ReportActionItem/LocalPDFReceiptPreview/index.tsx @@ -5,9 +5,10 @@ import PDFThumbnailError from '@components/PDFThumbnail/PDFThumbnailError'; import useThemeStyles from '@hooks/useThemeStyles'; +import {useReportActionItemState} from '@pages/inbox/report/ReportActionIndexContext'; + import CONST from '@src/CONST'; -import React, {useState} from 'react'; import {View} from 'react-native'; import type LocalPDFReceiptPreviewProps from './types'; @@ -18,9 +19,9 @@ const DOCUMENT_OPTIONS = {cMapUrl: '/cmaps/', cMapPacked: true}; function LocalPDFReceiptPreview({sourceURL, shouldUseFullHeight, onLoadFailure, onLoadSuccess}: LocalPDFReceiptPreviewProps) { const styles = useThemeStyles(); - const [failedToLoad, setFailedToLoad] = useState(false); - const [containerSize, setContainerSize] = useState<{width: number; height: number} | undefined>(undefined); - const [pageAspectRatio, setPageAspectRatio] = useState(undefined); + const [failedToLoad, setFailedToLoad] = useReportActionItemState(false); + const [containerSize, setContainerSize] = useReportActionItemState<{width: number; height: number} | undefined>(undefined); + const [pageAspectRatio, setPageAspectRatio] = useReportActionItemState(undefined); const handleDocumentLoadSuccess = (pdf: PDFDocumentProxy) => { pdf.getPage(1) diff --git a/src/components/ReportActionItem/ReceiptPDFOverlay/index.tsx b/src/components/ReportActionItem/ReceiptPDFOverlay/index.tsx index df6582ee17b5..ccc4f0311263 100644 --- a/src/components/ReportActionItem/ReceiptPDFOverlay/index.tsx +++ b/src/components/ReportActionItem/ReceiptPDFOverlay/index.tsx @@ -5,6 +5,8 @@ import useThemeStyles from '@hooks/useThemeStyles'; import addEncryptedAuthTokenToURL from '@libs/addEncryptedAuthTokenToURL'; +import {useReportActionItemState} from '@pages/inbox/report/ReportActionIndexContext'; + import variables from '@styles/variables'; import {retrieveMaxCanvasArea, retrieveMaxCanvasHeight, retrieveMaxCanvasWidth} from '@userActions/CanvasSize'; @@ -125,7 +127,7 @@ function ReceiptPDFOverlay({sourceURL, isAuthTokenRequired = true, onLoadFailure // Track which URL failed so hasFailed resets automatically when fileURL changes (e.g. after auth token refresh), // mirroring the pattern in ThumbnailImage. No useEffect needed — the comparison runs synchronously during render. - const [failedURL, setFailedURL] = useState(null); + const [failedURL, setFailedURL] = useReportActionItemState(null); const hasFailed = failedURL !== null && failedURL === fileURL; // The page last drawn stays on screen while the next one renders, so the thumbnail underneath never flashes diff --git a/src/components/ThumbnailImage.tsx b/src/components/ThumbnailImage.tsx index 3f0bb956114f..8e5eb5de0693 100644 --- a/src/components/ThumbnailImage.tsx +++ b/src/components/ThumbnailImage.tsx @@ -105,7 +105,9 @@ function ThumbnailImage({ const failedToLoad = failedLoadKey !== null && failedLoadKey.url === previewSourceURL && failedLoadKey.isOffline === isOffline; const cachedDimensions = shouldDynamicallyResize && typeof previewSourceURL === 'string' ? thumbnailDimensionsCache.get(previewSourceURL) : null; - const [imageDimensions, setImageDimensions] = useState({width: cachedDimensions?.width ?? imageWidth, height: cachedDimensions?.height ?? imageHeight}); + const [measuredDimensions, setMeasuredDimensions] = useState<{url: string | ImageSourcePropType; width: number; height: number} | null>(null); + const imageDimensions = + measuredDimensions?.url === previewSourceURL ? measuredDimensions : {width: cachedDimensions?.width ?? imageWidth, height: cachedDimensions?.height ?? imageHeight}; const {thumbnailDimensionsStyles} = useThumbnailDimensions(imageDimensions.width, imageDimensions.height); const StyleUtils = useStyleUtils(); @@ -118,7 +120,7 @@ function ThumbnailImage({ thumbnailDimensionsCache.set(previewSourceURL, {width, height}); } - setImageDimensions({width, height}); + setMeasuredDimensions({url: previewSourceURL, width, height}); }; const sizeStyles = shouldDynamicallyResize ? [thumbnailDimensionsStyles] : [styles.w100, styles.h100]; diff --git a/src/pages/inbox/ActionListContext.tsx b/src/pages/inbox/ActionListContext.tsx index a2764b32c88b..f620b9e53724 100644 --- a/src/pages/inbox/ActionListContext.tsx +++ b/src/pages/inbox/ActionListContext.tsx @@ -1,10 +1,10 @@ -import type FlatListRefType from '@components/FlashList/types'; - import type {ReactNode, RefObject} from 'react'; -import type {FlatList} from 'react-native'; import React, {createContext, useContext, useLayoutEffect, useRef} from 'react'; +import type ActionListRefType from './ActionListTypes'; +import type {ActionListRef} from './ActionListTypes'; + type ActionListContextType = { scrollOffsetRef: RefObject; @@ -12,10 +12,10 @@ type ActionListContextType = { getScrollOffset: () => number; /** Each list publishes its locally-owned ref on mount; pass `null` to clear on unmount. */ - registerListRef: (ref: FlatListRefType) => void; + registerListRef: (ref: ActionListRefType) => void; /** Reads the currently registered list ref. Call from handlers only, never during render. */ - getListRef: () => FlatListRefType; + getListRef: () => ActionListRefType; }; const ActionListContext = createContext({ @@ -35,7 +35,7 @@ function useActionListContext() { */ function useActionListRef() { const {registerListRef} = useActionListContext(); - const listRef = useRef(null); + const listRef = useRef(null); useLayoutEffect(() => { registerListRef(listRef); @@ -49,7 +49,7 @@ function useActionListRef() { function ActionListContextProvider({children}: {children: ReactNode}) { // Each list owns its own ref locally and publishes it here on mount; only the register/get // callbacks live in context, so attaching `ref={}` stays local to each list. - const listRefHolder = useRef(null); + const listRefHolder = useRef(null); const scrollOffsetRef = useRef(0); const value: ActionListContextType = { diff --git a/src/pages/inbox/ActionListTypes.ts b/src/pages/inbox/ActionListTypes.ts new file mode 100644 index 000000000000..a890d2947e28 --- /dev/null +++ b/src/pages/inbox/ActionListTypes.ts @@ -0,0 +1,34 @@ +import type {RefObject} from 'react'; + +type ScrollToIndexParams = { + animated?: boolean; + index: number; + viewOffset?: number; + viewPosition?: number; +}; + +type ScrollToOffsetParams = { + animated?: boolean; + offset: number; +}; + +type ScrollToEndParams = { + animated?: boolean; +}; + +/** Common imperative API used by the report scroll manager across FlatList, FlashList, and LegendList. */ +type ActionListRef = { + scrollToIndex: (params: ScrollToIndexParams) => void; + scrollToOffset: (params: ScrollToOffsetParams) => void; + scrollToEnd: (params?: ScrollToEndParams) => void; + getNativeScrollRef?: () => unknown; + getScrollableNode?: () => unknown; + /** Measured item positions supplied by LegendList. Other list implementations can omit this. */ + getState?: () => {positionByKey: (key: string) => number | undefined; scroll?: number; contentLength?: number; scrollLength?: number} | undefined; +}; + +/** Ref to the underlying list instance attached via `ref={}`. */ +type ActionListRefType = RefObject | null; + +export default ActionListRefType; +export type {ActionListRef}; diff --git a/src/pages/inbox/report/MoneyReportContentCreated.tsx b/src/pages/inbox/report/MoneyReportContentCreated.tsx index 46096456685d..efd9f93c761b 100644 --- a/src/pages/inbox/report/MoneyReportContentCreated.tsx +++ b/src/pages/inbox/report/MoneyReportContentCreated.tsx @@ -75,6 +75,7 @@ function MoneyReportContentCreated({report, policy, transaction, transactionThre ({index: 0, /** Lets shared list implementations provide their own reliable way to reach the newest action. */ const ReportActionScrollToNewestContext = createContext<(() => void) | undefined>(undefined); -function ReportActionPositionContextProvider({children, index, isNewest}: PropsWithChildren) { - return {children}; +/** + * Each list passes the index from its row renderer and marks the last action as newest. The + * LegendList caller also sets isRecycling because it can reuse a mounted row for another action. + * Descendants use that flag to reset transient state (such as open menus or PDF load errors) + * with useRecyclingState instead of carrying it over to the next action in that row. + */ +function ReportActionPositionContextProvider({children, index, isNewest, isRecycling}: PropsWithChildren) { + return {children}; +} + +/** Uses LegendList's recycling-aware state in the main report list and useState in shared, non-recycled lists. */ +function useReportActionItemState(initialState: State | (() => State)): [State, Dispatch>] { + const {isRecycling = false} = useContext(ReportActionIndexContext); + const state = useState(initialState); + const recyclingState = useRecyclingState(initialState); + return isRecycling ? [...recyclingState] : state; } -export {ReportActionPositionContextProvider, ReportActionScrollToNewestContext}; +export {ReportActionPositionContextProvider, ReportActionScrollToNewestContext, useReportActionItemState}; export default ReportActionIndexContext; diff --git a/src/pages/inbox/report/ReportActionItem.tsx b/src/pages/inbox/report/ReportActionItem.tsx index 4474b1c0e308..e28eb707ad0a 100644 --- a/src/pages/inbox/report/ReportActionItem.tsx +++ b/src/pages/inbox/report/ReportActionItem.tsx @@ -95,7 +95,7 @@ import {displayNameOrDefaultSelector} from '@selectors/PersonalDetails'; import {agentZeroProcessingAgentIDsSelector} from '@selectors/ReportNameValuePairs'; import {deepEqual} from 'fast-equals'; import mapValues from 'lodash/mapValues'; -import React, {useContext, useEffect, useRef, useState} from 'react'; +import React, {useContext, useEffect, useRef} from 'react'; import {Keyboard, View} from 'react-native'; import type {ContextMenuAnchor} from './ContextMenu/ReportActionContextMenu'; @@ -106,6 +106,7 @@ import MiniReportActionContextMenu from './ContextMenu/MiniReportActionContextMe import {hideContextMenu, hideDeleteModal, isActiveReportAction, showContextMenu} from './ContextMenu/ReportActionContextMenu'; import LinkPreviewer from './LinkPreviewer'; import {useReportActionActiveEdit} from './ReportActionEditMessageContext'; +import {useReportActionItemState} from './ReportActionIndexContext'; import ReportActionItemContentCreated from './ReportActionItemContentCreated'; import ReportActionItemFrame from './ReportActionItemFrame'; import ReportActionItemThread from './ReportActionItemThread'; @@ -243,16 +244,16 @@ function ReportActionItem({ const theme = useTheme(); const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); - const [isContextMenuActive, setIsContextMenuActive] = useState(() => isActiveReportAction(action.reportActionID)); - const [isEmojiPickerActive, setIsEmojiPickerActive] = useState(); - const [isPaymentMethodPopoverActive, setIsPaymentMethodPopoverActive] = useState(); - const [isHidden, setIsHidden] = useState(false); + const [isContextMenuActive, setIsContextMenuActive] = useReportActionItemState(() => isActiveReportAction(action.reportActionID)); + const [isEmojiPickerActive, setIsEmojiPickerActive] = useReportActionItemState(undefined); + const [isPaymentMethodPopoverActive, setIsPaymentMethodPopoverActive] = useReportActionItemState(undefined); + const [isHidden, setIsHidden] = useReportActionItemState(false); const {isActiveReportAction: isActiveReactionListReportAction, hideReactionList} = useContext(ReactionListContext); const {updateHiddenAttachments} = useContext(AttachmentModalContext); const popoverAnchorRef = useRef>(null); - const downloadedPreviews = useRef([]); + const downloadedPreviews = useRef<{reportActionID: string; urls: string[]}>({reportActionID: action.reportActionID, urls: []}); const isReportActionLinked = linkedReportActionID && action.reportActionID && linkedReportActionID === action.reportActionID; - const [isReportActionActive, setIsReportActionActive] = useState(!!isReportActionLinked); + const [isReportActionActive, setIsReportActionActive] = useReportActionItemState(!!isReportActionLinked); const shouldBreakGrouping = shouldBreakAccessibilityGrouping(); const isScreenReaderActive = Accessibility.useScreenReaderStatus(); @@ -371,11 +372,14 @@ function ReportActionItem({ } const urls = extractLinksFromMessageHtml(action); - if (deepEqual(downloadedPreviews.current, urls) || action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) { + if ( + (downloadedPreviews.current.reportActionID === action.reportActionID && deepEqual(downloadedPreviews.current.urls, urls)) || + action.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE + ) { return; } - downloadedPreviews.current = urls; + downloadedPreviews.current = {reportActionID: action.reportActionID, urls}; expandURLPreview(reportID, action.reportActionID); }, [action, reportID]); @@ -398,7 +402,7 @@ function ReportActionItem({ return; } setIsHidden(false); - }, [latestDecision, action]); + }, [latestDecision, action, setIsHidden]); const toggleContextMenuFromActiveReportAction = () => { setIsContextMenuActive(isActiveReportAction(action.reportActionID)); diff --git a/src/pages/inbox/report/ReportActionItemContentCreated.tsx b/src/pages/inbox/report/ReportActionItemContentCreated.tsx index 8f7403298d26..bf735ecf6478 100644 --- a/src/pages/inbox/report/ReportActionItemContentCreated.tsx +++ b/src/pages/inbox/report/ReportActionItemContentCreated.tsx @@ -97,6 +97,7 @@ function ReportActionItemContentCreated({parentReportAction, transactionID, draf {isEditingInline ? ( void) | undefined; +let mockOnCommitLayoutEffect: (() => void) | undefined; +let mockShouldRenderNewestAction = true; jest.mock('@react-navigation/native', () => ({ ...jest.requireActual('@react-navigation/native'), @@ -103,6 +106,7 @@ jest.mock('@components/MoneyRequestReportView/MoneyRequestReportTransactionList' isLoadingInitialActions, listRef, onLastItemIndexChange, + onCommitLayoutEffect, visibleReportActions, renderReportAction, }: { @@ -114,17 +118,19 @@ jest.mock('@components/MoneyRequestReportView/MoneyRequestReportTransactionList' scrollToOffset: typeof mockScrollToOffset; }>; onLastItemIndexChange?: (index: number) => void; + onCommitLayoutEffect?: () => void; visibleReportActions: ReportAction[]; renderReportAction: (reportAction: ReportAction, index: number) => React.ReactElement; }) => { ReactActual.useImperativeHandle(listRef, () => ({scrollToIndex: mockScrollToIndex, scrollToEnd: mockScrollToEnd, scrollToOffset: mockScrollToOffset})); ReactActual.useLayoutEffect(() => onLastItemIndexChange?.(MOCK_UNIFIED_LAST_ITEM_INDEX), [onLastItemIndexChange]); + mockOnCommitLayoutEffect = onCommitLayoutEffect; const newestAction = visibleReportActions.at(-1); return ( {isLoadingInitialActions ? : null} - {newestAction ? renderReportAction(newestAction, visibleReportActions.length - 1) : null} + {newestAction && mockShouldRenderNewestAction ? renderReportAction(newestAction, visibleReportActions.length - 1) : null} {listFooterComponent} ); @@ -209,6 +215,12 @@ jest.mock('@pages/inbox/report/ReportActionsListItemRenderer', () => { jest.requireActual('@pages/inbox/report/ReportActionIndexContext'); return jest.fn(() => { + ReactActual.useEffect( + () => () => { + mockNewestActionUnmount(); + }, + [], + ); mockIsNewestReportAction = ReactActual.useContext(ReportActionIndexContextActual).isNewest; mockScrollToNewestAction = ReactActual.useContext(ReportActionScrollToNewestContextActual); return null; @@ -320,6 +332,8 @@ describe('MoneyRequestReportActionsList - Reject Educational Modal', () => { jest.clearAllMocks(); mockIsNewestReportAction = undefined; mockScrollToNewestAction = undefined; + mockOnCommitLayoutEffect = undefined; + mockShouldRenderNewestAction = true; jest.spyOn(NativeNavigation, 'useIsFocused').mockReturnValue(true); mockUseIsReportLoadPending.mockReturnValue(false); mockUseNetwork.mockReturnValue({isOffline: false}); @@ -329,7 +343,8 @@ describe('MoneyRequestReportActionsList - Reject Educational Modal', () => { }); }); - it('should use the unified list index when the newest action requests a bottom scroll', async () => { + it('keeps newest-action scroll requests until FlashList commits after the editor unmounts', async () => { + // Given a loaded money report with an editable newest action and an unchanged row height. await act(async () => { await Onyx.multiSet({ [`${ONYXKEYS.COLLECTION.REPORT}${FAKE_REPORT_ID}` as const]: mockReport, @@ -348,9 +363,34 @@ describe('MoneyRequestReportActionsList - Reject Educational Modal', () => { await waitForBatchedUpdatesWithAct(); expect(mockIsNewestReportAction).toBe(true); - act(() => mockScrollToNewestAction?.()); + mockScrollToIndex.mockClear(); + act(() => mockOnCommitLayoutEffect?.()); + expect(mockScrollToIndex).not.toHaveBeenCalled(); + + // When closing the editor queues a request and unmounts its consumer, without a size-change event. + const requestScroll = mockScrollToNewestAction; + mockShouldRenderNewestAction = false; + act(() => requestScroll?.()); + expect(mockNewestActionUnmount).toHaveBeenCalledTimes(1); + expect(mockScrollToIndex).not.toHaveBeenCalled(); + + // Then the list's layout commit consumes the request once and preserves its indexed landing. + const firstCommit = mockOnCommitLayoutEffect; + act(() => { + firstCommit?.(); + firstCommit?.(); + }); expect(mockScrollToIndex).toHaveBeenCalledWith({index: MOCK_UNIFIED_LAST_ITEM_INDEX, animated: false, viewPosition: 1}); + expect(mockScrollToIndex).toHaveBeenCalledTimes(1); expect(mockScrollToEnd).not.toHaveBeenCalled(); + + // A later edit needs its own layout commit; a repeated older callback cannot consume it. + mockShouldRenderNewestAction = true; + act(() => requestScroll?.()); + act(() => firstCommit?.()); + expect(mockScrollToIndex).toHaveBeenCalledTimes(1); + act(() => mockOnCommitLayoutEffect?.()); + expect(mockScrollToIndex).toHaveBeenCalledTimes(2); }); it('should show reject educational modal when reject option is selected and explanation has NOT been dismissed', async () => { diff --git a/tests/unit/ActionListContextProviderTest.tsx b/tests/unit/ActionListContextProviderTest.tsx index 37033b6ad9e5..d255306e5cdc 100644 --- a/tests/unit/ActionListContextProviderTest.tsx +++ b/tests/unit/ActionListContextProviderTest.tsx @@ -1,8 +1,7 @@ import {act, renderHook} from '@testing-library/react-native'; -import type FlatListRefType from '@components/FlashList/types'; - import {ActionListContextProvider, useActionListContext} from '@pages/inbox/ActionListContext'; +import type ActionListRefType from '@pages/inbox/ActionListTypes'; /** * `ActionListContextProvider` owns a private holder and exposes `registerListRef(ref)` / `getListRef()` @@ -18,7 +17,7 @@ describe('ActionListContextProvider', () => { it('getListRef() returns the same ref object after registerListRef(ref)', () => { const {result} = renderHook(() => useActionListContext(), {wrapper: ActionListContextProvider}); - const listRef: FlatListRefType = {current: null}; + const listRef: ActionListRefType = {current: null}; act(() => { result.current.registerListRef(listRef); @@ -29,7 +28,7 @@ describe('ActionListContextProvider', () => { it('getListRef() returns null again after registerListRef(null) (unmount path)', () => { const {result} = renderHook(() => useActionListContext(), {wrapper: ActionListContextProvider}); - const listRef: FlatListRefType = {current: null}; + const listRef: ActionListRefType = {current: null}; act(() => { result.current.registerListRef(listRef); diff --git a/tests/unit/ThumbnailImageTest.tsx b/tests/unit/ThumbnailImageTest.tsx new file mode 100644 index 000000000000..e98a1b0284b4 --- /dev/null +++ b/tests/unit/ThumbnailImageTest.tsx @@ -0,0 +1,61 @@ +import {act, render} from '@testing-library/react-native'; + +import ThumbnailImage from '@components/ThumbnailImage'; + +import React from 'react'; + +const mockUseThumbnailDimensions = jest.fn((width: number, height: number) => ({thumbnailDimensionsStyles: {width, height}})); +let mockOnMeasure: ((dimensions: {width: number; height: number}) => void) | undefined; + +jest.mock('@hooks/useLazyAsset', () => ({useMemoizedLazyExpensifyIcons: () => ({})})); +jest.mock('@hooks/useNetwork', () => () => ({isOffline: false})); +jest.mock('@hooks/useStyleUtils', () => () => ({})); +jest.mock('@hooks/useTheme', () => () => ({border: '#000'})); +jest.mock('@hooks/useThemeStyles', () => () => ({})); +jest.mock('@hooks/useThumbnailDimensions', () => (width: number, height: number) => mockUseThumbnailDimensions(width, height)); +jest.mock('@components/ImageWithSizeCalculation', () => ({ + __esModule: true, + default: ({onMeasure}: {onMeasure: typeof mockOnMeasure}) => { + mockOnMeasure = onMeasure; + return null; + }, +})); + +describe('ThumbnailImage', () => { + beforeEach(() => { + mockUseThumbnailDimensions.mockClear(); + mockOnMeasure = undefined; + }); + + it('uses dimensions for the current URL when a recycled row shows another image', () => { + // Given a measured thumbnail for the first image. + const view = render( + , + ); + act(() => mockOnMeasure?.({width: 400, height: 100})); + expect(mockUseThumbnailDimensions).toHaveBeenLastCalledWith(400, 100); + + // When the same component instance is reused for a second image. + view.rerender( + , + ); + + // Then the second image starts with its own dimensions, rather than the first image's. + expect(mockUseThumbnailDimensions).toHaveBeenLastCalledWith(200, 200); + + // When the first image returns, its measured dimensions are restored from the URL cache. + view.rerender( + , + ); + expect(mockUseThumbnailDimensions).toHaveBeenLastCalledWith(400, 100); + }); +}); diff --git a/tests/unit/hooks/useEditMessage.test.ts b/tests/unit/hooks/useEditMessage.test.ts index e945ebf00b19..786323f255c7 100644 --- a/tests/unit/hooks/useEditMessage.test.ts +++ b/tests/unit/hooks/useEditMessage.test.ts @@ -21,7 +21,7 @@ jest.mock('@libs/actions/Report', () => { return { ...actual, editReportComment: jest.fn(), - clearReportActionDrafts: jest.fn(), + clearAllReportActionDrafts: jest.fn(), }; }); @@ -58,10 +58,10 @@ jest.mock('@hooks/useReportIsArchived', () => ({ default: () => false, })); -const mockScrollToIndex = jest.fn(); +const mockScrollToBottom = jest.fn(); jest.mock('@hooks/useReportScrollManager', () => ({ __esModule: true, - default: () => ({scrollToIndex: mockScrollToIndex}), + default: () => ({scrollToBottom: mockScrollToBottom}), })); jest.mock('@libs/ReportUtils', () => { @@ -99,6 +99,7 @@ describe('useEditMessage', () => { await Onyx.clear(); }); jest.clearAllMocks(); + jest.restoreAllMocks(); }); function renderUseEditMessage(overrides?: Partial) { @@ -144,9 +145,11 @@ describe('useEditMessage', () => { expect(args?.[1]?.reportActionID).toBe(props.reportAction?.reportActionID); }); - it('scrolls to index zero after deleting the newest message draft without a list-specific callback', () => { + it('scrolls to the bottom after deleting the newest message draft without a list-specific callback', () => { + // Given the newest message uses the list registered with the scroll manager. const {hook} = renderUseEditMessage({shouldScrollToLastMessage: true}); + // When deleting the draft closes its editor. act(() => { hook.result.current.publishDraft(' '); }); @@ -154,13 +157,16 @@ describe('useEditMessage', () => { mockShowDeleteModal.mock.calls.at(0)?.[3]?.(); }); - expect(mockScrollToIndex).toHaveBeenCalledWith(0); + // Then the list receives the request immediately and owns its layout scheduling. + expect(mockScrollToBottom).toHaveBeenCalledTimes(1); }); it('uses the list-specific scroll after deleting the newest message draft', () => { + // Given the list supplies its own edit-scroll request handler. const scrollToLastMessage = jest.fn(); const {hook} = renderUseEditMessage({shouldScrollToLastMessage: true, scrollToLastMessage}); + // When deleting the draft closes its editor. act(() => { hook.result.current.publishDraft(' '); }); @@ -168,8 +174,21 @@ describe('useEditMessage', () => { mockShowDeleteModal.mock.calls.at(0)?.[3]?.(); }); + // Then scheduling belongs to that list instead of a callback on the unmounting editor. expect(scrollToLastMessage).toHaveBeenCalledTimes(1); - expect(mockScrollToIndex).not.toHaveBeenCalled(); + expect(mockScrollToBottom).not.toHaveBeenCalled(); + }); + + it('requests the newest action after saving without scheduling an editor frame', () => { + // Given a draft on the newest action. + const {hook} = renderUseEditMessage({shouldScrollToLastMessage: true}); + + // When the edit is submitted and the editor closes. + act(() => hook.result.current.publishDraft('Updated message')); + expect(mockEditReportComment).toHaveBeenCalledTimes(1); + + // Then the list already owns the scroll request before the editor can unmount. + expect(mockScrollToBottom).toHaveBeenCalledTimes(1); }); it('does not scroll after deleting a non-newest message draft', () => { @@ -181,6 +200,6 @@ describe('useEditMessage', () => { }); expect(scrollToLastMessage).not.toHaveBeenCalled(); - expect(mockScrollToIndex).not.toHaveBeenCalled(); + expect(mockScrollToBottom).not.toHaveBeenCalled(); }); });