[NoQA] Refine reviewer rules based on automated tracker feedback#90590
Merged
Valforte merged 1 commit intoMay 18, 2026
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
Refines two coding-standards rules consumed by the Claude reviewer, based on accept/reject feedback gathered via the automated tracker. Each refinement narrows the rule's firing condition so reviewers stop dismissing technically-wrong suggestions, which was eroding trust in unrelated rules.
CLEAN-REACT-PATTERNS-4(clean-react-4-no-side-effect-spaghetti.md) - Previously flagged any internal helper that returns JSX, with rationale that React Compiler "cannot independently memoize" closures. That claim is wrong for thin delegation wrappers, and three sampled PR threads all rejected the suggestion on that basis. The rule now fires only when the helper (a) contains hooks/side effects/non-trivial business logic, (b) builds deeply nested JSX inline, or (c) closes over mutable parent state. Explicit carve-outs added for switch/conditional helpers that delegate to already-extracted children, and.map()over already-extracted components. The "Incorrect" example is replaced with one that hits all three triggers.CLEAN-REACT-PATTERNS-1(clean-react-1-composition-over-config.md) - Case 3 (monolithic prop interface) previously counted all props uniformly, which flagged components that legitimately lifted state across siblings. Adds a Case 3 carve-out for coordination props whose role is structurally detectable: the same state setter passed to two or more sibling children, OR the same callback consumed by two or more sibling children. Case 4 (config-array rendering) is intentionally unchanged - evidence is too thin to lock in a threshold change.The originally proposed
PERF-9refinement was rolled back during review - the cases the new carve-out would have permitted (intermediate state that is render-driving) are exactly the casesPERF-14already wants migrated touseSyncExternalStore, so adding the carve-out would have created a contradiction between the two rules.No application code is modified - this PR touches only
.claude/skills/coding-standards/rules/*.md.Fixed Issues
$ #90591
PROPOSAL:
Tests
This PR only modifies markdown files consumed by the Claude reviewer skill - no runtime code changes, so the mobile/web platform test matrix is N/A.
.claude/skills/coding-standards/rules/and confirm the frontmatter parses and the markdown renders without broken sections.const renderRow = () => type === 'foo' ? <Foo /> : <Bar />). ConfirmCLEAN-REACT-PATTERNS-4no longer fires.CLEAN-REACT-PATTERNS-1Case 3 no longer fires on that basis alone.useStateand builds nested JSX with business logic inline. ConfirmCLEAN-REACT-PATTERNS-4still fires (regression check on the narrowed rule).Offline tests
N/A - the change does not affect runtime behavior or network/offline code paths. Rule files are consumed by the Claude reviewer pipeline, not by the app at runtime.
QA Steps
Same as Tests above. QA validation is performed by running the Claude reviewer against test PRs that exercise the narrowed and preserved firing conditions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionsrc/languages/*files and using the translation methodDesignlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
N/A - no UI changes
Android: mWeb Chrome
N/A - no UI changes
iOS: Native
N/A - no UI changes
iOS: mWeb Safari
N/A - no UI changes
MacOS: Chrome / Safari
N/A - no UI changes