Skip to content

[CP Staging] fix: Downgrade Onyx to v3.0.45 #85767

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.

Sign up for GitHub

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

Merged
mountiny merged 2 commits into from
Mar 19, 2026
Merged

[CP Staging] fix: Downgrade Onyx to v3.0.45 #85767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
872a52a
Revert "Merge pull request #85248 from callstack-internal/VickyStash/…
chrispader Mar 19, 2026
08efc40
Merge branch 'main' into @chrispader/dowgrade-onyx-to-v3.0.45
chrispader Mar 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@
"react-native-localize": "^3.5.4",
"react-native-nitro-modules": "0.29.4",
"react-native-nitro-sqlite": "9.2.0",
"react-native-onyx": "3.0.46",
"react-native-onyx": "3.0.45",
"react-native-pager-view": "8.0.0",
"react-native-pdf": "7.0.2",
"react-native-permissions": "^5.4.0",
Expand Down
51 changes: 21 additions & 30 deletions ...omponents/ReportActionItem/MoneyRequestReportPreview/MoneyRequestReportPreviewContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {useIsFocused} from '@react-navigation/native';
import {useFocusEffect} from '@react-navigation/native';
import {hasSeenTourSelector} from '@selectors/Onboarding';
import {FlashList} from '@shopify/flash-list';
import type {FlashListRef, ListRenderItemInfo} from '@shopify/flash-list';
Expand Down Expand Up @@ -552,40 +552,31 @@ function MoneyRequestReportPreviewContent({
carouselTransactionsRef.current = carouselTransactions;
}, [carouselTransactions]);

const isFocused = useIsFocused();
const isFocusedRef = useRef(isFocused);
useFocusEffect(

@github-actions github-actions Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-12 (docs)

The setTimeout inside the useFocusEffect callback has no cleanup mechanism. If the screen loses focus or the component unmounts before CONST.ANIMATED_TRANSITION elapses, the timer callback will still fire, potentially calling scrollToIndex on an unmounted or stale ref. The previous implementation guarded against this with isFocusedRef.current, but that guard has been removed.

Return a cleanup function from the useFocusEffect callback to clear the timer:

useFocusEffect(
    useCallback(() => {
        const index = carouselTransactions.findIndex(...);
        if (index < 0) {
            return;
        }
        const newTransaction = carouselTransactions.at(index);
        const timeoutId = setTimeout(() => {
            if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
                return;
            }
            carouselRef.current?.scrollToIndex({ ... });
        }, CONST.ANIMATED_TRANSITION);

        return () => clearTimeout(timeoutId);
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [newTransactionIDs]),
);

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

useCallback(() => {
const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.has(transaction.transactionID));

useEffect(() => {
isFocusedRef.current = isFocused;
}, [isFocused]);

useEffect(() => {
const index = carouselTransactions.findIndex((transaction) => newTransactionIDs?.has(transaction.transactionID));

if (index < 0) {
return;
}
const newTransaction = carouselTransactions.at(index);
setTimeout(() => {
if (!isFocusedRef.current) {
return;
}
// If the new transaction is not available at the index it was on before the delay, avoid the scrolling
// because we are scrolling to either a wrong or unavailable transaction (which can cause crash).
if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
if (index < 0) {
return;
}
const newTransaction = carouselTransactions.at(index);
setTimeout(() => {
// If the new transaction is not available at the index it was on before the delay, avoid the scrolling
// because we are scrolling to either a wrong or unavailable transaction (which can cause crash).
if (newTransaction?.transactionID !== carouselTransactionsRef.current.at(index)?.transactionID) {
return;
Comment on lines +563 to +567

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Cancel the delayed carousel scroll when the screen blurs

This setTimeout() now survives a blur because useFocusEffect is not returning a cleanup and the previous isFocusedRef.current check was removed. If the user navigates away within CONST.ANIMATED_TRANSITION after a new expense arrives, stack navigators that keep the old screen mounted will still execute scrollToIndex() against the hidden carousel, so coming back later can land on an unexpected item. The earlier implementation explicitly skipped this delayed scroll once the screen lost focus.

Useful? React with 👍 / 👎.

}

carouselRef.current?.scrollToIndex({
index,
viewOffset: -2 * styles.gap2.gap,
animated: true,
});
}, CONST.ANIMATED_TRANSITION);
carouselRef.current?.scrollToIndex({
index,
viewOffset: -2 * styles.gap2.gap,
animated: true,
});
}, CONST.ANIMATED_TRANSITION);

// We only want to scroll to a new transaction when the set of new transaction IDs changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [newTransactionIDs]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [newTransactionIDs]),
);

@github-actions github-actions Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-5 (docs)

The eslint-disable-next-line react-hooks/exhaustive-deps comment lacks a justification explaining why the dependencies are intentionally omitted (e.g., carouselTransactions and styles are used in the callback but not listed).

Add an explanatory comment before the disable:

// We only want to scroll to a new transaction when the set of new transaction IDs changes.
// eslint-disable-next-line react-hooks/exhaustive-deps

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

const onViewableItemsChanged = useRef(({viewableItems}: {viewableItems: ViewToken[]; changed: ViewToken[]}) => {
const newIndex = viewableItems.at(0)?.index;
Expand Down
4 changes: 3 additions & 1 deletion src/components/ReportActionItem/MoneyRequestReportPreview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {useIsFocused} from '@react-navigation/native';
import type {ListRenderItem} from '@shopify/flash-list';
import React, {useCallback, useMemo, useRef, useState} from 'react';
import type {LayoutChangeEvent} from 'react-native';
Expand Down Expand Up @@ -121,8 +122,9 @@ function MoneyRequestReportPreview({
selector: hasOnceLoadedReportActionsSelector,
});
const newTransactions = useNewTransactions(hasOnceLoadedReportActions, transactions);
const isFocused = useIsFocused();
// We only want to highlight the new expenses if the screen is focused.
const newTransactionIDs = new Set(newTransactions.map((transaction) => transaction.transactionID));
const newTransactionIDs = isFocused ? new Set(newTransactions.map((transaction) => transaction.transactionID)) : undefined;

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve new transaction IDs while the preview is unfocused

useNewTransactions() only reports additions on the render where the transaction list grows (src/hooks/useNewTransactions.ts:17-26). By replacing the set with undefined whenever useIsFocused() is false, a transaction that arrives while this preview is mounted in the background is dropped on that one render, and the next focus render already treats the updated list as the baseline. In practice, expenses created while another screen is on top will never be highlighted or auto-scrolled when the user returns to this preview.

Useful? React with 👍 / 👎.


const transactionPreviewContainerStyles = [styles.h100, reportPreviewStyles.transactionPreviewCarouselStyle];

Expand Down
9 changes: 3 additions & 6 deletions src/pages/workspace/withPolicy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {ComponentType} from 'react';
import React, {useEffect} from 'react';
import React from 'react';
import type {OnyxEntry} from 'react-native-onyx';
import useOnyx from '@hooks/useOnyx';
import type {PlatformStackRouteProp} from '@libs/Navigation/PlatformStackNavigation/types';
Expand Down Expand Up @@ -91,12 +91,9 @@ export default function <TProps extends WithPolicyProps>(WrappedComponent: Compo
/* eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing */
const isLoadingPolicy = !hasLoadedApp || (!!policyID && isLoadingOnyxValue(policyResults, policyDraftResults));

useEffect(() => {
if (!policyID) {
return;
}
if (policyID && policyID.length > 0) {

@github-actions github-actions Bot Mar 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-8 (docs)

updateLastAccessedWorkspace(policyID) is a side effect (Onyx.set) called directly in the component render body. This will execute on every render, including re-renders caused by unrelated state or prop changes, React Strict Mode double-invocations, and concurrent rendering retries. The previous useEffect approach was correct because it limited execution to when policyID actually changed.

Restore the useEffect wrapper:

useEffect(() => {
    if (!policyID) {
        return;
    }
    updateLastAccessedWorkspace(policyID);
}, [policyID]);

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

updateLastAccessedWorkspace(policyID);
}, [policyID]);
}

return (
<WrappedComponent
Expand Down
8 changes: 0 additions & 8 deletions tests/actions/ReportTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,8 +1520,6 @@ describe('actions/Report', () => {
currentUserAccountID: TEST_USER_ACCOUNT_ID,
});

await waitForBatchedUpdates();

// Need the reportActionID to delete the comments
const newComment = PersistedRequests.getAll().at(1);
const reportActionID = newComment?.data?.reportActionID as string | undefined;
Expand Down Expand Up @@ -2041,9 +2039,6 @@ describe('actions/Report', () => {
const newComment = PersistedRequests.getAll().at(0);
const reportActionID = newComment?.data?.reportActionID as string | undefined;
const reportAction = TestHelper.buildTestReportComment(created, TEST_USER_ACCOUNT_ID, reportActionID);

await waitForBatchedUpdates();

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});

// wait for Onyx.connect execute the callback and start processing the queue
Expand Down Expand Up @@ -2237,7 +2232,6 @@ describe('actions/Report', () => {
expect(requests?.at(0)?.data?.reportComment).toBe('value3');

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false});
await waitForBatchedUpdates();

TestHelper.expectAPICommandToHaveBeenCalled(WRITE_COMMANDS.UPDATE_COMMENT, 1);
});
Expand Down Expand Up @@ -2336,8 +2330,6 @@ describe('actions/Report', () => {
expect(requests?.at(0)?.data?.reportComment).toBe('value3');

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false});
await waitForBatchedUpdates();

TestHelper.expectAPICommandToHaveBeenCalled(WRITE_COMMANDS.UPDATE_COMMENT, 1);
});

Expand Down
4 changes: 0 additions & 4 deletions tests/actions/SessionTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,6 @@ describe('Session', () => {

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false});

await waitForBatchedUpdates();

expect(getAllPersistedRequests().length).toBe(0);
});

Expand Down Expand Up @@ -229,8 +227,6 @@ describe('Session', () => {

await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false});

await waitForBatchedUpdates();

expect(getAllPersistedRequests().length).toBe(0);
});

Expand Down
4 changes: 2 additions & 2 deletions tests/unit/APITest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ describe('APITests', () => {
});

Onyx.set(ONYXKEYS.NETWORK, {isOffline: true});
expect(NetworkStore.isOffline()).toBe(true);
expect(NetworkStore.isOffline()).toBe(false);
expect(NetworkStore.isAuthenticating()).toBe(false);
return waitForBatchedUpdates();
})
Expand Down Expand Up @@ -551,7 +551,7 @@ describe('APITests', () => {
API.write('MockCommandThree' as WriteCommand, {});

// THEN the retryable requests should immediately be added to the persisted requests
expect(PersistedRequests.getLength()).toBe(2);
expect(PersistedRequests.getAll().length).toBe(2);

// WHEN we wait for the queue to run and finish processing
return waitForBatchedUpdates();
Expand Down
25 changes: 13 additions & 12 deletions tests/unit/OptionsListUtilsTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3273,7 +3273,7 @@ describe('OptionsListUtils', () => {
expect(canCreate).toBe(false);
});

it('createOptionList() localization', async () => {
it('createOptionList() localization', () => {
renderLocaleContextProvider();
// Given a set of reports and personal details
// When we call createOptionList and extract the reports
Expand All @@ -3282,15 +3282,18 @@ describe('OptionsListUtils', () => {
// Then the returned reports should match the expected values
expect(reports.at(10)?.subtitle).toBe(`Submits to Mister Fantastic`);

await Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, CONST.LOCALES.ES);

await waitForBatchedUpdates();

// When we call createOptionList again
const newReports = createOptionList(PERSONAL_DETAILS, CURRENT_USER_ACCOUNT_ID, EMPTY_PRIVATE_IS_ARCHIVED_MAP, REPORTS).reports;
// Then the returned reports should change to Spanish
// cspell:disable-next-line
expect(newReports.at(10)?.subtitle).toBe('Se envía a Mister Fantastic');
return (
waitForBatchedUpdates()
// When we set the preferred locale to Spanish
.then(() => Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, CONST.LOCALES.ES))
.then(() => {
// When we call createOptionList again
const newReports = createOptionList(PERSONAL_DETAILS, CURRENT_USER_ACCOUNT_ID, EMPTY_PRIVATE_IS_ARCHIVED_MAP, REPORTS).reports;
// Then the returned reports should change to Spanish
// cspell:disable-next-line
expect(newReports.at(10)?.subtitle).toBe('Se envía a Mister Fantastic');
})
);
});
});

Expand Down Expand Up @@ -3364,8 +3367,6 @@ describe('OptionsListUtils', () => {
'1': getFakeAdvancedReportAction(CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT),
},
});
await waitForBatchedUpdates();

// When we call createOptionList with report 10 marked as archived
const archivedMap: PrivateIsArchivedMap = {
[`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}10`]: reportNameValuePairs.private_isArchived,
Expand Down
6 changes: 2 additions & 4 deletions tests/unit/ReportSecondaryActionUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,14 @@ describe('getSecondaryAction', () => {
beforeAll(() => {
Onyx.init({
keys: ONYXKEYS,
initialKeyStates: {
[ONYXKEYS.SESSION]: SESSION,
[ONYXKEYS.PERSONAL_DETAILS_LIST]: {[EMPLOYEE_ACCOUNT_ID]: PERSONAL_DETAILS, [APPROVER_ACCOUNT_ID]: {accountID: APPROVER_ACCOUNT_ID, login: APPROVER_EMAIL}},
},
});
});

beforeEach(async () => {
jest.clearAllMocks();
Onyx.clear();
await Onyx.merge(ONYXKEYS.SESSION, SESSION);
await Onyx.set(ONYXKEYS.PERSONAL_DETAILS_LIST, {[EMPLOYEE_ACCOUNT_ID]: PERSONAL_DETAILS, [APPROVER_ACCOUNT_ID]: {accountID: APPROVER_ACCOUNT_ID, login: APPROVER_EMAIL}});
});

it('should always return default options', () => {
Expand Down
21 changes: 7 additions & 14 deletions tests/unit/SequentialQueueTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,10 @@ describe('SequentialQueue', () => {
};
SequentialQueue.push(requestWithConflictResolution);
expect(getLength()).toBe(1);
// We know there is only one request and it's ongoing.
// We can get it and verify that the ongoing request is the second one.
const ongoingRequest = getOngoingRequest();
expect(ongoingRequest?.data?.accountID).toBe(56789);
// We know there is only one request in the queue, so we can get the first one and verify
// that the persisted request is the second one.
const persistedRequest = getAll().at(0);
expect(persistedRequest?.data?.accountID).toBe(56789);
});

it('should push two requests with conflict resolution and push', () => {
Expand Down Expand Up @@ -109,9 +109,7 @@ describe('SequentialQueue', () => {
};

SequentialQueue.push(requestWithConflictResolution);

const ongoingRequest = getOngoingRequest();
expect(ongoingRequest?.data?.accountID).toBe(56789);
expect(getLength()).toBe(2);
});

it('should replace request request in queue while a similar one is ongoing', async () => {
Expand Down Expand Up @@ -177,14 +175,9 @@ describe('SequentialQueue', () => {

expect(getLength()).toBe(4);
const persistedRequests = getAll();
const ongoingRequest = getOngoingRequest();

// The first OpenReport call is ongoing
expect(ongoingRequest?.command).toBe('OpenReport');

// We know ReconnectApp is at index 0 in the queue now, so we can get it to verify
// We know ReconnectApp is at index 1 in the queue, so we can get it to verify
// that was replaced by the new request.
expect(persistedRequests.at(0)?.data?.accountID).toBe(56789);
expect(persistedRequests.at(1)?.data?.accountID).toBe(56789);
});

// need to test a rance condition between processing the next request and then pushing a new request with conflict resolver
Expand Down
Loading