Skip to content
11 changes: 9 additions & 2 deletions lib/useOnyx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
const selector = options?.selector;

// Create memoized version of selector for performance
const memoizedSelector = useMemo(() => {
const memoizedSelector = useMemo((): UseOnyxSelector<TKey, TReturnValue> | null => {
if (!selector) {
return null;
}
Expand Down Expand Up @@ -230,6 +230,11 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
}
}, [key, options?.canEvict]);

// Tracks the last memoizedSelector reference that getSnapshot() has computed with.
// When the selector changes, this mismatch forces getSnapshot() to re-evaluate
// even if all other conditions (isFirstConnection, shouldGetCachedValue, key) are false.
const lastComputedSelectorRef = useRef(memoizedSelector);

const getSnapshot = useCallback(() => {
// Check if we have any cache for this Onyx key
// Don't use cache for first connection with initWithStoredValues: false
Expand All @@ -256,10 +261,12 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
// We get the value from cache while the first connection to Onyx is being made or if the key has changed,
// so we can return any cached value right away. For the case where the key has changed, If we don't return the cached value right away, then the UI will show the incorrect (previous) value for a brief period which looks like a UI glitch to the user. After the connection is made, we only
// update `newValueRef` when `Onyx.connect()` callback is fired.
if (isFirstConnectionRef.current || shouldGetCachedValueRef.current || key !== previousKey) {
const hasSelectorChanged = lastComputedSelectorRef.current !== memoizedSelector;
if (isFirstConnectionRef.current || shouldGetCachedValueRef.current || key !== previousKey || hasSelectorChanged) {
// Gets the value from cache and maps it with selector. It changes `null` to `undefined` for `useOnyx` compatibility.
const value = OnyxUtils.tryGetCachedValue(key) as OnyxValue<TKey>;
const selectedValue = memoizedSelector ? memoizedSelector(value) : value;
lastComputedSelectorRef.current = memoizedSelector;
newValueRef.current = (selectedValue ?? undefined) as TReturnValue | undefined;

// This flag is `false` when the original Onyx value (without selector) is not defined yet.
Expand Down
30 changes: 23 additions & 7 deletions tests/unit/useOnyxTest.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you think of a way to add a failing test for this first? seems like we had this bug in place because this scenario was not covered by unit tests. For onyx I believe we should try to always create the failing test first and then work on the fix cc @fabioh8010

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see that there is an update in unit tests within this PR. Adding act() makes its behavior closer to how React works in the browser, like in few other tests here, breaking the scenario (while it shouldn't). The test in its previous form was too loose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I saw that change, but it was not clear to me if that is all that we could / should cover here. Could you create a specific test for this behaviour that was broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I'll do it.

Copy link
Contributor Author

@leshniak leshniak Feb 17, 2026

Choose a reason for hiding this comment

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

@mountiny I've updated the tests and also dug deeper into the root cause.

On the main branch rerender() is called synchronously, so updates scheduled asynchonously with prepareSubscriberUpdate() don't have time to run. As a result, the store subscriber callback
does not get a chance to toggle the isFirstConnection and shouldGetCachedValue flags, which allows the new callback to execute.

On the other hand, if we call the next useOnyx in an asynchronous workflow, those flags are toggled to false, effectively preventing the new selector from executing.

The updated unit test ensures that replacing the selector works predictably in both scenarios.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice thanks for digging into that

Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,10 @@ describe('useOnyx', () => {
it('should always use the current selector reference to return new data', async () => {
Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name'});

let selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name}`) as UseOnyxSelector<OnyxKey, string>;
let selector: UseOnyxSelector<OnyxKey, string> = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name}`) as UseOnyxSelector<
OnyxKey,
string
>;

const {result, rerender} = renderHook(() =>
useOnyx(ONYXKEYS.TEST_KEY, {
Expand All @@ -437,10 +440,22 @@ describe('useOnyx', () => {
expect(result.current[0]).toEqual('id - test_id, name - test_name');
expect(result.current[1].status).toEqual('loaded');

selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed`) as UseOnyxSelector<OnyxKey, string>;
selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed synchronously`) as UseOnyxSelector<OnyxKey, string>;

rerender(undefined);

expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed');
expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed synchronously');
expect(result.current[1].status).toEqual('loaded');

selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed after macrotask`) as UseOnyxSelector<OnyxKey, string>;

await act(async () => {
// This is necessary to avoid regressions of the selector interleaving bug, see https://github.com/Expensify/App/issues/79449
await waitForPromisesToResolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await waitForPromisesToResolve();

This is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree, we should still test it after a macrotask to avoid possible regressions in the future. We can get rid of it if the whole flow will be synchronous. This bug is hard to notice without an automated test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we leave a small comment about it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabioh8010 done ;)

rerender(undefined);
});

expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed after macrotask');
expect(result.current[1].status).toEqual('loaded');
});

Expand Down Expand Up @@ -681,15 +696,16 @@ describe('useOnyx', () => {

const dependencies = ['constant'];
let selectorCallCount = 0;
const selector = ((data) => {
selectorCallCount++;
return `${dependencies.join(',')}:${(data as {value?: string})?.value}`;
}) as UseOnyxSelector<OnyxKey, string>;

const {result, rerender} = renderHook(() =>
useOnyx(
ONYXKEYS.TEST_KEY,
{
selector: (data) => {
selectorCallCount++;
return `${dependencies.join(',')}:${(data as {value?: string})?.value}`;
},
selector,
},
dependencies,
),
Expand Down