Repository navigation
Improve Onyx PRs reviewing process #80097
Description
We are often seeing PRs being merged into Onyx that are not following adequate security and quality practices. The changes are sometimes "innocent" but if we don't maintain a high quality bar of the codebase they can will out of control, we will lose coverage and that's where the tricky bugs start happening. Here are some examples:
- fix: result isn't changed when the member collection change react-native-onyx#708 - looks like a low-quality solution and there are no unit tests whatsoever
- fix: Send collection key as
matchedKeyin collection callbacks react-native-onyx#720 - checklist empty, unit tests added but no manual testing - fix: result isn't changed when the member collection change v2 react-native-onyx#716 - follow up of the “low-quality” PR, no unit tests added/changed
- fix: onyx snapshot cache & status react-native-onyx#711 - new logic but no tests added
- fix: status is infinity loading for the none-exist key react-native-onyx#709 - no tests added/changed
I would say that all PRs in Onyx must have manual, automated tests and videos unless specifically written and approved why they don't/can't have.
Let's think into ways to improve this process and not let bugs/uncovered code be shipped into such critical library that is Onyx.
Issue Owner
Current Issue Owner: @fabioh8010Metadata
Metadata
Assignees
Labels
Type
Projects
- Status
Activity
melvin-bot commentedon Feb 13, 2026
This issue has not been updated in over 15 days. @fabioh8010, @mountiny eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
mountiny commentedon Feb 15, 2026
Havent prioritized this yet, but @fabioh8010 feel free to create PRs for the actions that will require the automated tests
MrMuzyk commentedon Feb 26, 2026
Hey, I'll take over this one from @fabioh8010 :)
MrMuzyk commentedon Feb 26, 2026
I've opened a draft PR and will continue tomorrow.
@mountiny I've added a descirption in the PR describing what I've done so far. I've talked with @fabioh8010 on what was discussed before and went in this direction. Let me know if there is something else you'd like to have for sure.
One thing I've considered is having an escape hatch mechanism that would allow us skipping the tests if really really necessary. We can punt it right now if you don't think it's needed.
MrMuzyk commentedon Feb 26, 2026
One more note - I've initially considered adding some coverage metrics and checks related to that but as this a big change I didn't go this way without discussing it.
If this is something that would interest you let me know :)
mountiny commentedon Feb 26, 2026
That sounds good, yeah I think we do not need to add the test coverage automation for now
fabioh8010 commentedon Mar 2, 2026
Two more things we can do:
Desktopreferences from https://github.com/Expensify/react-native-onyx/blob/main/.github/PULL_REQUEST_TEMPLATE.mdmountiny commentedon Mar 2, 2026
Both sounds good
MrMuzyk commentedon Mar 3, 2026
I've updated the PR to include both of these things
MrMuzyk commentedon Mar 18, 2026
@mountiny PR got merged a while ago and it's doing it's job. Is there something else you'd like to see added on this front or we're good to close here?
mountiny commentedon Mar 18, 2026
Nice, thanks! I think this is good for now and if anything else arises we can handle that separately
fabioh8010 commentedon Mar 20, 2026
@mountiny @MrMuzyk Can we re-open this issue?
In this PR Expensify/react-native-onyx#757 there is no
Screenshots/Videossection in the PR description, however the PR Checklist Validation is passing.MrMuzyk commentedon Mar 23, 2026
Hmm, so basically what happend is that the contributor:
I believe once it would be taken out of draft the job we be triggered again and then fail.
There is room for improvement - we should validate checklist during draft stage as well. I'll prep a PR for that today
fabioh8010 commentedon Mar 23, 2026
@MrMuzyk
If we can confirm that should be enough then, we don't really need to validate on draft state.
MrMuzyk commentedon Mar 23, 2026
it runs on 'ready_for_review' as well so Im pretty sure it will re-check once it's marked. Let's observe for now then
MrMuzyk commentedon Mar 27, 2026
Hey @mountiny 👋
@fabioh8010 gave me the idea that we can have scripts that verify generated API docs in a similar way as we validate actions
Im not sure whether we need a proposal but since I've already prepared it I'll share it
Proposal: Validate API docs are up-to-date in CI
Background: The react-native-onyx library auto-generates two API reference files — API.md and API-INTERNAL.md — from JSDoc annotations in the TypeScript source code. The generation pipeline compiles TypeScript to JavaScript (npm run build), then extracts documentation using jsdoc-to-markdown (npm run build:docs). Both generated markdown files are committed to the repository so consumers can reference them directly on GitHub without building locally. The repository already enforces that compiled GitHub Actions stay in sync with their sources via a CI workflow (validateGithubActions.yml) and a companion shell script (verifyActions.sh) that rebuilds artifacts and fails the check if a diff is detected.
Problem: When a contributor modifies JSDoc annotations or function signatures in the library source code, if they submit a PR without regenerating the API docs, then the committed documentation silently drifts out of sync with the actual code — misleading consumers who rely on it.
Solution: Add a new GitHub Actions workflow (validateDocs.yml) and companion shell script (verifyDocs.sh) that mirror the existing validateGithubActions pattern.
PR is here. I've undrafted it and got some comments so will deal with it shortly