Repository navigation
[No QA] Add a new lint rule that blocks onyx loading status check - #104729
VickyStash wants to merge 9 commits into
Conversation
|
@MelvinBot review |
ReviewThe rule works for the main patterns. CI lint passes, and the new seatbelt entries line up with the existing usages I spot-checked. I found one gap that's worth fixing before merge, and two smaller ones. 1. Aliased const [policies, policiesMetadata] = useOnyxWithoutSnapshots(ONYXKEYS.COLLECTION.POLICY);
if (policiesMetadata.status === 'loading') {...}Fix: in Smaller gaps (optional)
Everything else looks good. The exemption block comes after the main rules block, so the override applies, and the |
|
@MelvinBot review |
Re-reviewThe alias fix in Check aliased useOnyx imports works. The rule now tracks the local name of any named Nits (optional)
|
…-rule # Conflicts: # config/eslint/eslint.seatbelt.tsv
|
@nkdengineer Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2741d1807
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| describe('no-onyx-loading-status', () => { | ||
| tsRuleTester.run(ruleModule.name, ruleModule, { | ||
| valid: [ |
There was a problem hiding this comment.
Each string under valid/invalid becomes an individual RuleTester test, but these new cases omit the mandatory // Given, // When, and // Then rationale (and the lone line 93 comment is not those sections). CLAUDE.md:81, which the root instructions designate as the repository testing guidance, requires all three for every test case, so annotate each case rather than leaving the table undocumented.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Other lint tests have the same structure.
| }, | ||
|
|
||
| VariableDeclarator(node) { | ||
| if (node.id.type !== 'ArrayPattern' || node.init?.type !== 'CallExpression') { |
There was a problem hiding this comment.
When a caller keeps the tuple instead of destructuring it—for example, const result = useOnyx(key); if (result[1].status === 'loading') ...—node.id is an Identifier, so this guard returns without a report. That valid hook usage introduces the exact per-key status dependency this rule is intended to stop, allowing new migration debt and a later failure when status is removed; recognize assigned tuple/index reads rather than only ArrayPattern declarators.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I haven't found any usage like this in the codebase:
result[1].status === 'loading'
So Im not sure it's really valid for us
| for (const specifier of node.specifiers) { | ||
| const isNamedUseOnyx = specifier.type === 'ImportSpecifier' && specifier.imported.type === 'Identifier' && specifier.imported.name === USE_ONYX_IDENTIFIER; | ||
| const isDefaultUseOnyxHook = specifier.type === 'ImportDefaultSpecifier' && source.endsWith(USE_ONYX_HOOK_PATH_SUFFIX); | ||
| if (isNamedUseOnyx || isDefaultUseOnyxHook) { | ||
| useOnyxNames.add(specifier.local.name); | ||
| } |
There was a problem hiding this comment.
Hooks that wrap useOnyx aren't checked. For example, WorkspaceCompanyCardsTable uses useAllPersonalDetails. We should treat the known wrappers as useOnyx
There was a problem hiding this comment.
True, but in practice, most of these wrappers are already covered.
For example in WorkspaceCompanyCardsTable, personalDetailsMetadata you mention is only used through isLoadingOnyxValue
and the rule counts every isLoadingOnyxValue call.
There is only one place where this is actually missing: here.
We should treat the known wrappers as useOnyx
To do that, we will need to track all of the wrappers in the rule, right? I guess it can be problematic, cause new wrappers can be added/updated. Or we can consider all hooks as wrappers, which can end up with some false positives in some case.
Since the cleanup will remove the metadata from these wrappers anyway, maybe we can skip it.
What do you think?
There was a problem hiding this comment.
Makes sense as it's a rare case.
|
@VickyStash We have the conflicts. |
Explanation of Change
This PR adds a lint rule that prevents new code from depending on the per-key loading status of
useOnyxwhile we remove existing usages in batches.The rule flags:
isLoadingOnyxValuestatusfrom auseOnyxresultLocaleContextProviderandHybridAppHandlerare exempt, because they mount before Onyx finishes loading and still need the startup status. Tests are exempt too. They get cleaned up together with the code they cover.Existing usages are added to the seatbelt baseline, so current code keeps passing lint.
Fixed Issues
$ #104583
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.