Repository navigation
Reuse unchanged derived values and skip no-op derived writes - #76
Conversation
|
One test from the analysis is not in this PR: It renders Results:
So it showed the problem, but it can't confirm the fix. If we want the hook itself to tolerate a fresh but equal guide list, the Test sourceimport {act, renderHook} from '@testing-library/react-native';
import OnyxListItemProvider from '@components/OnyxListItemProvider';
import {CurrentReportIDContextProvider} from '@hooks/useCurrentReportID';
import {SidebarOrderedReportsContextProvider, useSidebarOrderedReports} from '@hooks/useSidebarOrderedReports';
import SidebarUtils from '@libs/SidebarUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Report} from '@src/types/onyx';
import type {OnyxMultiSetInput} from 'react-native-onyx';
import React from 'react';
import Onyx from 'react-native-onyx';
import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct';
jest.mock('@libs/SidebarUtils', () => ({
sortReportsToDisplayInLHN: jest.fn(),
getReportsToDisplayInLHN: jest.fn(),
updateReportsToDisplayInLHN: jest.fn(),
filterReportsForInboxTab: jest.fn((reportIDs: string[]) => reportIDs),
getInboxTabSummary: jest.fn(() => ({counts: {}, hasStaleUnreadReport: false})),
}));
jest.mock('@libs/Navigation/Navigation', () => ({
getActiveRouteWithoutParams: jest.fn(() => ''),
isNavigationReady: jest.fn(() => Promise.resolve()),
getTopmostReportId: jest.fn(),
}));
jest.mock('@libs/ReportUtils', () => ({
parseReportRouteParams: jest.fn(() => ({reportID: undefined})),
getReportIDFromLink: jest.fn(() => ''),
}));
const mockSidebarUtils = jest.mocked(SidebarUtils);
const GUIDE_ACCOUNT_ID = 8;
const REPORT_COUNT = 50;
function buildReports(): Record<string, Report> {
const reports: Record<string, Report> = {};
for (let i = 1; i <= REPORT_COUNT; i++) {
reports[`${ONYXKEYS.COLLECTION.REPORT}${i}`] = {
reportID: String(i),
reportName: `Chat ${i}`,
lastVisibleActionCreated: '2024-01-01 10:00:00',
type: CONST.REPORT.TYPE.CHAT,
};
}
return reports;
}
function TestWrapper({children}: {children: React.ReactNode}) {
return (
<OnyxListItemProvider>
<CurrentReportIDContextProvider>
<SidebarOrderedReportsContextProvider>{children}</SidebarOrderedReportsContextProvider>
</CurrentReportIDContextProvider>
</OnyxListItemProvider>
);
}
describe('useSidebarOrderedReports on an equal guide accountIDs recompute', () => {
beforeAll(() => {
Onyx.init({keys: ONYXKEYS});
});
beforeEach(async () => {
jest.clearAllMocks();
const reports = buildReports();
await act(async () => {
await Onyx.clear();
await Onyx.set(ONYXKEYS.SESSION, {accountID: 12345, email: 'test@example.com', authTokenType: CONST.AUTH_TOKEN_TYPES.ANONYMOUS});
await Onyx.multiSet({
[ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.DEFAULT,
[ONYXKEYS.COLLECTION.POLICY]: {},
[ONYXKEYS.COLLECTION.TRANSACTION]: {},
[ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS]: {},
[ONYXKEYS.BETAS]: [],
[ONYXKEYS.DERIVED.REPORT_ATTRIBUTES]: {reports: {}},
[ONYXKEYS.DERIVED.GUIDE_ACCOUNT_IDS]: [GUIDE_ACCOUNT_ID],
...reports,
} satisfies OnyxMultiSetInput);
});
mockSidebarUtils.getReportsToDisplayInLHN.mockReturnValue(reports);
mockSidebarUtils.updateReportsToDisplayInLHN.mockImplementation(({displayedReports}) => displayedReports);
mockSidebarUtils.sortReportsToDisplayInLHN.mockReturnValue([]);
await waitForBatchedUpdatesWithAct();
});
afterAll(async () => {
await act(async () => {
await Onyx.clear();
});
});
it('should not re-evaluate every report when the guide accountIDs are rewritten with the same content', async () => {
// Given the LHN has built its list of reports
renderHook(() => useSidebarOrderedReports(), {wrapper: TestWrapper});
await waitForBatchedUpdatesWithAct();
mockSidebarUtils.updateReportsToDisplayInLHN.mockClear();
mockSidebarUtils.getReportsToDisplayInLHN.mockClear();
// When the derived engine flushes after an unrelated personal-details write. It writes a fresh array with
// skipCacheCheck, exactly like setDerivedValue, so Onyx notifies even though the content is unchanged.
await act(async () => {
await Onyx.set(ONYXKEYS.DERIVED.GUIDE_ACCOUNT_IDS, [GUIDE_ACCOUNT_ID], {skipCacheCheck: true});
});
await waitForBatchedUpdatesWithAct();
// Then no call may re-check all reports, the guide-hydration branch is only for a real change of guides
const reEvaluatedReportCounts = mockSidebarUtils.updateReportsToDisplayInLHN.mock.calls.map(([params]) => params.updatedReportsKeys.length);
expect(reEvaluatedReportCounts).not.toContain(REPORT_COUNT);
expect(mockSidebarUtils.getReportsToDisplayInLHN).not.toHaveBeenCalled();
});
}); |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
| if (newDerivedValue === derivedValue) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
In case we hit this we will still reach the finally block, right?
There was a problem hiding this comment.
Yes, finally is ran after return is called.
Confirmed with this simple script
There was a problem hiding this comment.
Do we really need this new file? Can't we reuse the existing test files?
There was a problem hiding this comment.
Do we really need this new file? Can't we reuse the existing test files?
There was a problem hiding this comment.
Had to keep this one. Rest files are moved to OnyxDerivedTest
It needs a fresh engine, and OnyxDerivedTest.tsx can't give it one.
The test checks what happens after an app restart, when the derived value is already on disk. The engine reads that saved value only once, when init() starts (src/libs/actions/OnyxDerived/index.ts:59-74). So the test has to save the value to Onyx first and only then call initOnyxDerivedValues().
There was a problem hiding this comment.
Do we really need this new file? Can't we reuse the existing test files?
|
This looks good @sosek108 , once it's ready feel free to merge to my PR! |
2 participants
Explanation of Change
This targets the Onyx 3.0.117 bump in Expensify#98956.
On 3.0.117,
useOnyxwithout a selector compares by reference only. Before, a top-levelshallowEqualkept an equal new value at the old reference. OnyxDerived writes withskipCacheCheck, andGUIDE_ACCOUNT_IDSandLOGIN_TO_ACCOUNT_ID_MAPbuild a new array or map on everyPERSONAL_DETAILS_LISTwrite. So after the bump, every personal-details write re-renders all 10 consumers of these keys, even when nothing they use has changed. InuseSidebarOrderedReportsit is worse: the new reference looks like guides being loaded, so the LHN re-checks every report.The two configs now return the current value when the new result is shallow-equal to it. The engine skips the write when compute returns the value it already holds, so a no-op recompute no longer notifies subscribers or rewrites the whole value to disk. That applies to every derived key, and every config already copies before changing anything, so a real update is never skipped.
Reassure, 10 renames of a user who is in none of the reports or rows, compared with main on 3.0.115:
The full Reassure suite shows no regression from this fix. Two regressions come from the bump itself and are not addressed here:
getReportsToDisplayInLHNon 15k reports is about 30% slower, andMoneyRequestReportActionsListrenders once more when a transaction on an unrelated report changes.Fixed Issues
$ Expensify#98956
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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