Clone main #15
Clone main #15
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Summary of ChangesHello @visz11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new, robust test case for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughAdded a devDependency for routing, introduced a new test validating dependent queries across routes, and adjusted memo initialization in useBaseQuery to include an explicit initial value. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as Test Runner
participant App as App (MemoryRouter)
participant QC as QueryClientProvider
participant P as Parent useQuery ['parent']
participant C as Child useQuery ['sub', parent.id]
U->>App: Mount App at index route
App->>QC: Provide QueryClient context
Note over App,QC: Parent query preloaded in cache
U->>App: Navigate to /sub
App->>P: Read parent data (cache)
P-->>App: Return parent { id }
App->>C: Initialize child query with key ['sub', parent.id]
C->>QC: Fetch child data via QueryClient
QC-->>C: child data (e.g., "child123")
C-->>App: Render child data
App-->>U: Output shows child data, no errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
codeant-ai
bot
added
the
size:L
Clone mainTL;DR: Adds dependent query test case and fixes query client memoization in solid-query package. Refacto PR SummaryImplements test coverage for dependent queries in routing scenarios and resolves query client memoization issues. Change HighlightsClick to expand
Sequence DiagramsequenceDiagram
participant R as Router
participant C as Component
participant Q as useQuery
participant QC as QueryClient
participant API as QueryFn
R->>C: Navigate to /sub
C->>Q: Parent query (id: 123)
Q->>QC: Get cached data
QC-->>Q: Return parent data
Q-->>C: Parent result
C->>Q: Child query (depends on parent.id)
Q->>API: Fetch child data
API-->>Q: Return child123
Q-->>C: Child result
C-->>R: Render child123
Testing GuideClick to expand
|
|
/refacto-visz |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fix for dependent queries, particularly in scenarios involving suspense, along with a corresponding test case. The fix in useBaseQuery.ts adjusts the memoization of the QueryClient to handle reactivity more robustly. The new test case is a valuable addition, though I've suggested a small improvement to remove a sleep call to prevent potential flakiness. The added @solidjs/router dev dependency is appropriate for the new test.
| await sleep(200) | ||
|
|
||
| expect(errorHandler).not.toHaveBeenCalled() | ||
|
|
||
| await waitFor(() => { | ||
| expect(rendered.getByText('child123')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
Using sleep in tests can introduce flakiness. It's generally better to rely on waitFor to check for the expected outcome.
In this case, if the waitFor assertion for the success state (child123) passes, it implies that no error was thrown and the errorHandler was not called. You can safely move the expect(errorHandler).not.toHaveBeenCalled() assertion after the waitFor block and remove the sleep, making the test more robust.
await waitFor(() => {
expect(rendered.getByText('child123')).toBeInTheDocument()
})
expect(errorHandler).not.toHaveBeenCalled()
Code Review: Query Client Memo Implementation👍 Well Done
📁 Selected files for review (4)
🎯 Custom Instructions
📝 Additional Comments
|
![refacto-visz[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| const client = createMemo( | ||
| () => useQueryClient(queryClient?.()), | ||
| useQueryClient(queryClient?.()), | ||
| ) |
There was a problem hiding this comment.
Query Client Equality
Initial value parameter in createMemo causes equality comparison between function result and initial value on every evaluation. This breaks memoization when query client instances differ, leading to unnecessary re-computations and potential query observer recreation.
const client = createMemo(
() => useQueryClient(queryClient?.())
)
Commitable Suggestion
| const client = createMemo( | |
| () => useQueryClient(queryClient?.()), | |
| useQueryClient(queryClient?.()), | |
| ) | |
| const client = createMemo( | |
| () => useQueryClient(queryClient?.()) | |
| ) |
Standards
- ISO-IEC-25010-Reliability-Maturity
- SRE-Performance-Reliability
| const client = createMemo( | ||
| () => useQueryClient(queryClient?.()), | ||
| useQueryClient(queryClient?.()), | ||
| ) |
There was a problem hiding this comment.
Memo Equality Comparison
Memo equality comparison uses function call result as equality check causing unnecessary re-computations. The useQueryClient call executes on every memo evaluation instead of providing stable equality reference.
Standards
- ISO-IEC-25010-Performance-Efficiency-Time-Behavior
- Optimization-Pattern-Memoization-Efficiency
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/solid-query/src/useBaseQuery.ts (1)
119-123: Good fix: initialize memo with an explicit client.This avoids a transient undefined on first read and stabilizes updates. Minor micro-optimization: compute the initial client once.
- const client = createMemo( - () => useQueryClient(queryClient?.()), - useQueryClient(queryClient?.()), - ) + const initialClient = useQueryClient(queryClient?.()) + const client = createMemo( + () => useQueryClient(queryClient?.()), + initialClient, + )packages/solid-query/src/__tests__/useQuery.test.tsx (1)
6051-6124: Dependent-queries-across-routes test adds valuable coverage; consider flakiness guard.Test reads well and validates the regression. To reduce timing flakiness, you can drop the fixed sleep(200) and assert the absence of errors after the child renders, or gate the negative assertion behind a waitFor on child render.
Example:
- await sleep(200) - - expect(errorHandler).not.toHaveBeenCalled() - - await waitFor(() => { - expect(rendered.getByText('child123')).toBeInTheDocument() - }) + await waitFor(() => { + expect(rendered.getByText('child123')).toBeInTheDocument() + }) + expect(errorHandler).not.toHaveBeenCalled()Alternatively, set a small waitFor timeout for the negative assertion if you still want a pre-child check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/solid-query/package.json(1 hunks)packages/solid-query/src/__tests__/useQuery.test.tsx(3 hunks)packages/solid-query/src/useBaseQuery.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/solid-query/src/useBaseQuery.ts (1)
packages/solid-query/src/QueryClientProvider.tsx (1)
useQueryClient(14-25)
packages/solid-query/src/__tests__/useQuery.test.tsx (3)
packages/solid-query/src/__tests__/utils.tsx (2)
createQueryClient(35-37)sleep(51-55)packages/solid-query/src/useQuery.ts (1)
useQuery(36-50)packages/solid-query/src/QueryClientProvider.tsx (1)
QueryClientProvider(32-47)
🪛 GitHub Actions: autofix.ci
packages/solid-query/package.json
[error] 1-1: Dependency mismatch detected between package.json and lockfile. Run 'pnpm install' (without --frozen-lockfile) or update the lockfile to reflect changes in package.json.
🔇 Additional comments (3)
packages/solid-query/src/__tests__/useQuery.test.tsx (2)
5-5: Import of Suspense is appropriate.
15-15: Router imports look correct for MemoryRouter usage.packages/solid-query/package.json (1)
72-72: Lockfile already includes @solidjs/router—no update required
pnpm-lock.yaml records @solidjs/[email protected] and has no unstaged changes.
2 participants
CodeAnt-AI Description
Fix query client being undefined and prevent crashes for dependent queries during navigation
What Changed
Impact
✅ Fewer query startup errors✅ No route-change crashes for dependent queries✅ Reliable child data rendering after navigation💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit