Investigate `useOnyx` crash when changing `policy_-1` key to `policy__FAKE_`

[fabioh8010]

During our tests to migrate withOnyx usage to useOnyx in the codebase, we noticed this crash that is happening in useOnyx:

It shouldn't error because policy_-1 and policy__FAKE_ are both valid collection member keys.

We must investigate and propose a fix to allow this use case to happen.

Issue Owner

Current Issue Owner: @lschurr

[ishpaul777]

we have a custom hook usePolicy in which we handle this case

https://github.com/Expensify/App/blob/main/src/hooks/usePolicy.ts

A detailed RCA is in this proposal https://github.com/Expensify/App/issues/46805#issuecomment-2269371635

[nkdengineer]

@fabioh8010 The problem here is splitCollectionMemberKey function returns the wrong collection key if the key is policy__FAKE_. Instead of auto-detecting the collection key via splitCollectionMemberKey, we can pass the collect key to useOnyx hook and then use this in the check isCollectionMemberKey. Or we can detect the collection key via the matched string of previousKey and key.

What do you think?

[fabioh8010]

we can pass the collect key to useOnyx hook

@nkdengineer Do you mean passing like this?

const [policy] = useOnyx(ONYXKEYS.COLLECTION.POLICY, `${ONYXKEYS.COLLECTION.POLICY}${policyID}`);

If yes, I don't think it's a good idea – it's a change in the core API and it's too big to justify fixing this issue. We should focus on fixing splitCollectionMemberKey to allow this use case to happen. I took a look and I know what is happening, I will need to take some time to think how to make such cases possible.

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[fabioh8010]

Working on this right now.

[mountiny]

How is this looking so far @fabioh8010 any ideas for a fix?

[fabioh8010]

We are iterating on some solutions but given the current flexibility of the key / collection key it's been difficult to find one that satisfy all cases. We've achieved the closest working solution here, but is not final yet as one test is failing. cc @VickyStash

[trjExpensify]

Should we make this a deploy blocker?

[fabioh8010]

@VickyStash is working on a workaround for the Expensify Card issue here, context here. It would be better if we were able to move with the workaround, so we will have more time to think in proper solution for useOnyx.

[fabioh8010]

We've achieved the closest working solution here, but is not final yet as one test is failing.

Actually I think the failing test is just wrong, looking into it 👀

[fabioh8010]

Created a PR for this: https://github.com/Expensify/react-native-onyx/pull/581

[melvin-bot[bot]]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[s77rt]

Waiting on Onyx PR

[fabioh8010]

PR was merged!