Fix: Add hasOnceLoadedReportActions guard to all readNewestAction call sites #81462
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,9 @@ type Args = { | |
|
|
||
| /** Callback to call on every scroll event */ | ||
| onTrackScrolling: (event: NativeSyntheticEvent<NativeScrollEvent>) => void; | ||
|
|
||
| /** Whether the report actions have been loaded at least once */ | ||
| hasOnceLoadedReportActions: boolean; | ||
| }; | ||
|
|
||
| export default function useReportUnreadMessageScrollTracking({ | ||
|
|
@@ -32,14 +35,16 @@ export default function useReportUnreadMessageScrollTracking({ | |
| onTrackScrolling, | ||
| unreadMarkerReportActionIndex, | ||
| isInverted, | ||
| hasOnceLoadedReportActions, | ||
| }: Args) { | ||
| const [isFloatingMessageCounterVisible, setIsFloatingMessageCounterVisible] = useState(false); | ||
| const isFocused = useIsFocused(); | ||
| const ref = useRef<{previousViewableItems: ViewToken[]; reportID: string; unreadMarkerReportActionIndex: number; isFocused: boolean; hasOnceLoadedReportActions: boolean}>({ | ||
| reportID, | ||
| unreadMarkerReportActionIndex, | ||
| previousViewableItems: [], | ||
| isFocused: true, | ||
| hasOnceLoadedReportActions, | ||
| }); | ||
|
Comment on lines
+42
to
48
There was a problem hiding this comment.
When Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. The comment is technically correct, @MelvinBot could you please check?
Contributor
Author
There was a problem hiding this comment. The review comment identifies a real inconsistency in the ref initialization pattern, but it is not an actual bug in practice. Here's why: The concernThe ref is initialized with Why it doesn't cause a bugThe only place if (unreadActionVisible && readActionSkippedRef.current && ref.current.hasOnceLoadedReportActions) {For the stale
But if (!reportMetadata?.hasOnceLoadedReportActions) {
return;
}So by the time Additionally, RecommendationThat said, initializing the ref with the prop value ( |
||
| // We want to save the updated value on ref to use it in onViewableItemsChanged | ||
| // because FlatList requires the callback to be stable and we cannot add a dependency on the useCallback. | ||
|
|
@@ -52,6 +57,10 @@ export default function useReportUnreadMessageScrollTracking({ | |
| ref.current.isFocused = isFocused; | ||
|
Contributor
There was a problem hiding this comment. ❌ PERF-6 (docs)The Suggested fix: Eliminate the // In the onViewableItemsChanged callback:
if (unreadActionVisible && readActionSkippedRef.current && hasOnceLoadedReportActions) {
readActionSkippedRef.current = false;
readNewestAction(ref.current.reportID);
}Since Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Contributor
Author
There was a problem hiding this comment. Thanks for the review! However, I believe the current implementation is correct and follows the established pattern in this file. The // We want to save the updated value on ref to use it in onViewableItemsChanged
// because FlatList requires the callback to be stable and we cannot add a dependency on the useCallback.Because the callback never re-creates, using This same pattern is already used in this file for:
The new |
||
| }, [isFocused]); | ||
|
|
||
| useEffect(() => { | ||
| ref.current.hasOnceLoadedReportActions = hasOnceLoadedReportActions; | ||
| }, [hasOnceLoadedReportActions]); | ||
|
|
||
| /** | ||
| * On every scroll event we want to: | ||
| * Show/hide the latest message pill when user is scrolling back/forth in the history of messages. | ||
|
|
@@ -113,7 +122,7 @@ export default function useReportUnreadMessageScrollTracking({ | |
| if (unreadActionVisible && readActionSkippedRef.current) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| readActionSkippedRef.current = false; | ||
| readNewestAction(ref.current.reportID, ref.current.hasOnceLoadedReportActions); | ||
| } | ||
|
|
||
| // FlatList requires a stable onViewableItemsChanged callback for optimal performance. | ||
|
|
||
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.
We should have not prevented readNewestAction if report actions for the report exist. Solved here