stable query client#37
Open
AlbertSmit wants to merge 2 commits into
Open
Conversation
Add configs.jsdoc preset wiring up eslint-plugin-jsdoc with: - jsdoc/require-jsdoc (publicOnly: true — exported functions only) - jsdoc/valid-types - jsdoc/check-param-names - jsdoc/no-undefined-types Opt-in per project, not part of the default Kaliber config.
…nside React components
WalkthroughAdds a new Changestanstack-query/stable-client Rule
configs.jsdoc Preset
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rules/tanstack-query/stable-client/test.js (1)
28-38: ⚡ Quick winAdd a valid fixture for lazy initializer with an intermediate variable.
Please add coverage for
useState(() => { const qc = new QueryClient(); return qc })so this supported stable pattern stays protected from false positives.Suggested test addition
valid: [ @@ // useState with block body { code: ` function App() { const [queryClient] = useState(() => { return new QueryClient() }) return null } `, }, + // useState with local binding in lazy initializer + { + code: ` + function App() { + const [queryClient] = useState(() => { + const qc = new QueryClient() + return qc + }) + return null + } + `, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rules/tanstack-query/stable-client/test.js` around lines 28 - 38, Add a new test fixture object to the test array that covers the useState lazy initializer pattern where an intermediate variable is declared and returned, specifically testing useState(() => { const qc = new QueryClient(); return qc }). This test case should be added after the existing useState with block body test case to ensure this supported stable pattern with intermediate variable assignment is protected from false positives in the linting rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 55-60: The code snippet in the README is incomplete and will fail
when copy-pasted because it references kaliberConfig on line 58 without
declaring it. Add the missing require statement for kaliberConfig at the
beginning of the snippet, before the kaliberPlugin require statement, so that
when users copy-paste this example code, it will work standalone without
requiring them to reference other parts of the documentation.
In `@rules/tanstack-query/stable-client/index.js`:
- Around line 45-58: The isWrappedInUseState function only validates QueryClient
instantiations that are direct returns or arrow function concise bodies, missing
valid patterns where QueryClient is assigned to a variable and then returned.
Extend the logic to handle cases where the node is nested inside a
VariableDeclarator within a function body that is wrapped in useState by adding
a branch that traverses the parent chain to find the containing function body,
verifies it's actually a function, and then checks if that function is wrapped
in a useState call.
---
Nitpick comments:
In `@rules/tanstack-query/stable-client/test.js`:
- Around line 28-38: Add a new test fixture object to the test array that covers
the useState lazy initializer pattern where an intermediate variable is declared
and returned, specifically testing useState(() => { const qc = new
QueryClient(); return qc }). This test case should be added after the existing
useState with block body test case to ensure this supported stable pattern with
intermediate variable assignment is protected from false positives in the
linting rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a632d194-22cc-4e52-acec-1b890af90d52
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
README.mdindex.jsmachinery/ast.jspackage.jsonrules-overview.mdrules/tanstack-query/stable-client/index.jsrules/tanstack-query/stable-client/readme.mdrules/tanstack-query/stable-client/test.js
Comment on lines
+55
to
+60
| const kaliberPlugin = require('@kaliber/eslint-plugin') | ||
|
|
||
| module.exports = [ | ||
| ...kaliberConfig, | ||
| kaliberPlugin.configs.jsdoc, | ||
| ] |
There was a problem hiding this comment.
Make the JSDoc usage snippet self-contained.
Line 58 uses kaliberConfig, but this snippet only declares kaliberPlugin; copy-pasting it as-is will fail.
Proposed doc fix
+const kaliberConfig = require('`@kaliber/eslint-plugin/eslint.config`')
const kaliberPlugin = require('`@kaliber/eslint-plugin`')
module.exports = [
...kaliberConfig,
kaliberPlugin.configs.jsdoc,
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const kaliberPlugin = require('@kaliber/eslint-plugin') | |
| module.exports = [ | |
| ...kaliberConfig, | |
| kaliberPlugin.configs.jsdoc, | |
| ] | |
| const kaliberConfig = require('`@kaliber/eslint-plugin/eslint.config`') | |
| const kaliberPlugin = require('`@kaliber/eslint-plugin`') | |
| module.exports = [ | |
| ...kaliberConfig, | |
| kaliberPlugin.configs.jsdoc, | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 55 - 60, The code snippet in the README is incomplete
and will fail when copy-pasted because it references kaliberConfig on line 58
without declaring it. Add the missing require statement for kaliberConfig at the
beginning of the snippet, before the kaliberPlugin require statement, so that
when users copy-paste this example code, it will work standalone without
requiring them to reference other parts of the documentation.
Comment on lines
+45
to
+58
| function isWrappedInUseState(node) { | ||
| const parent = node.parent | ||
| if (!parent) return false | ||
|
|
||
| // Concise arrow body: useState(() => new QueryClient()) | ||
| if (isArrowConciseBody(parent, node) && isUseStateCall(parent.parent)) return true | ||
|
|
||
| // Block body: useState(() => { return new QueryClient() }) | ||
| if (parent.type === 'ReturnStatement') { | ||
| const blockParent = parent.parent | ||
| if (blockParent && blockParent.parent && isFunctionNode(blockParent.parent)) { | ||
| if (isUseStateCall(blockParent.parent.parent)) return true | ||
| } | ||
| } |
There was a problem hiding this comment.
isWrappedInUseState misses valid lazy-initializer forms and produces false positives.
The current logic only accepts direct-return shapes. A valid stable pattern like useState(() => { const qc = new QueryClient(); return qc }) is incorrectly reported because new QueryClient() is under VariableDeclarator, not the two hardcoded branches.
Proposed fix
function isWrappedInUseState(node) {
- const parent = node.parent
- if (!parent) return false
-
- // Concise arrow body: useState(() => new QueryClient())
- if (isArrowConciseBody(parent, node) && isUseStateCall(parent.parent)) return true
-
- // Block body: useState(() => { return new QueryClient() })
- if (parent.type === 'ReturnStatement') {
- const blockParent = parent.parent
- if (blockParent && blockParent.parent && isFunctionNode(blockParent.parent)) {
- if (isUseStateCall(blockParent.parent.parent)) return true
- }
- }
-
- return false
+ let current = node.parent
+ while (current) {
+ if (isFunctionNode(current)) {
+ return isUseStateCall(current.parent)
+ }
+ current = current.parent
+ }
+ return false
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rules/tanstack-query/stable-client/index.js` around lines 45 - 58, The
isWrappedInUseState function only validates QueryClient instantiations that are
direct returns or arrow function concise bodies, missing valid patterns where
QueryClient is assigned to a variable and then returned. Extend the logic to
handle cases where the node is nested inside a VariableDeclarator within a
function body that is wrapped in useState by adding a branch that traverses the
parent chain to find the containing function body, verifies it's actually a
function, and then checks if that function is wrapped in a useState call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
Ports the
stable-query-clientrule from@tanstack/eslint-plugin-queryinto our own plugin — without adding the dependency.Prevents creating a
new QueryClient()inside a React component body. The QueryClient must be stable across renders, otherwise the entire query cache is lost on every re-render.Why not just install the TanStack plugin?
The TanStack plugin has 6 rules. After analysis, only
stable-query-clientapplies to our (JavaScript) codebases:stable-query-clientexhaustive-depsno-unstable-depsno-rest-destructuringinfinite-query-property-ordermutation-property-orderOne useful rule doesn't justify an entire plugin dependency.
Valid patterns
Invalid patterns
Why useState and not useMemo?
React's own docs (useMemo caveats):
React can discard memoized values during Suspense, concurrent features, or future APIs.
useState's lazy initializer is guaranteed to run exactly once.Changes
rules/tanstack-query/stable-client/index.jsrules/tanstack-query/stable-client/test.jsrules/tanstack-query/stable-client/readme.mdindex.jsmachinery/ast.jsisInsideComponent,isArrowConciseBody,isUseStateCallReusable helpers moved to
machinery/ast.jsThese helpers are generic enough to be useful in future rules:
isInsideComponent(node)— walks up AST to check if a node is inside a PascalCase function (React component)isArrowConciseBody(arrowFn, bodyNode)— checks if a node is the concise body of an arrow functionisUseStateCall(node)— detectsuseState(...)orReact.useState(...)call expressionsSummary by CodeRabbit
New Features
configs.jsdocpreset for JSDoc linting with four rules (require JSDoc on exported functions, validate type syntax, check parameter names, validate defined types).QueryClientinstantiation inside React component bodies.Documentation