docs(solid-query): convert guide docs to ref-based with Solid-style code#10146
Open
sukvvon wants to merge 4 commits intoTanStack:mainfrom
Open
docs(solid-query): convert guide docs to ref-based with Solid-style code#10146sukvvon wants to merge 4 commits intoTanStack:mainfrom
sukvvon wants to merge 4 commits intoTanStack:mainfrom
Conversation
|
github-actions
bot
added
the
documentation
|
View your CI Pipeline Execution ↗ for commit 786cd36
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new offline mutations example for Solid Query, refactors Solid framework guides to replace migration rule mappings with concrete code examples, and adds documentation anchors and comments throughout React framework guides for improved documentation structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
docs/framework/solid/guides/placeholder-query-data.md (1)
22-22: Heading level skip is acceptable in ref-based docs.The h3 heading appears without a preceding h2 in this file, but since this is a ref-based document (line 4), the parent h2 sections are inherited from the React guide. This is likely intentional structure, though it causes markdownlint to flag a violation when analyzing the file in isolation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/solid/guides/placeholder-query-data.md` at line 22, Add an explicit inline comment above the "Placeholder Data Memoization" h3 to indicate the heading-level skip is intentional because this is a ref-based document whose parent h2 sections are inherited from the React guide; this comment should reference the h3 ("Placeholder Data Memoization") so reviewers and linters understand the skip (or alternatively add a markdownlint disable directive at the top of the file for the specific heading-level rule) to prevent the isolated markdownlint violation.docs/framework/solid/guides/disabling-queries.md (1)
23-38: Consider reorderingMatchconditions for clearer state handling.The current order checks
todosQuery.databeforeisErrorandisLoading. If data exists from a previous fetch but a refetch fails, the error state won't be displayed. This might be intentional for stale-while-revalidate UX, but typically error/loading states are checked first.Suggested reordering for explicit state prioritization
<Switch> - <Match when={todosQuery.data}> - <ul> - <For each={todosQuery.data}>{(todo) => <li>{todo.title}</li>}</For> - </ul> - </Match> - <Match when={todosQuery.isError}> - <span>Error: {todosQuery.error.message}</span> - </Match> <Match when={todosQuery.isLoading}> <span>Loading...</span> </Match> + <Match when={todosQuery.isError}> + <span>Error: {todosQuery.error.message}</span> + </Match> + <Match when={todosQuery.data}> + <ul> + <For each={todosQuery.data}>{(todo) => <li>{todo.title}</li>}</For> + </ul> + </Match> <Match when={true}> <span>Not ready ...</span> </Match> </Switch>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/framework/solid/guides/disabling-queries.md` around lines 23 - 38, The Switch/Match block currently checks todosQuery.data before todosQuery.isError and todosQuery.isLoading, which can hide error/loading states when stale data exists; reorder the Match checks in the Switch so that todosQuery.isError and todosQuery.isLoading are evaluated before todosQuery.data (i.e., check todosQuery.isError, then todosQuery.isLoading, then todosQuery.data, finally the fallback Match when={true}) to ensure explicit prioritization of error/loading states in the rendering logic for the todosQuery component.examples/solid/offline/src/api.ts (1)
17-42: Consider adding response status checks.The fetch functions don't verify
response.okbefore parsing JSON. If the server returns an error status (e.g., 404 from line 59), callingresponse.json()on non-JSON error bodies could throw or produce unexpected results.Example fix for fetchMovie
export const fetchMovie = async ( id: string, ): Promise<{ ts: number movie: { comment: string; id: string; title: string } }> => { const response = await fetch(`/movies/${id}`) + if (!response.ok) { + throw new Error(`Failed to fetch movie: ${response.status}`) + } return await response.json() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/solid/offline/src/api.ts` around lines 17 - 42, The three API helpers (fetchMovie, fetchMovies, updateMovie) don't check response.ok before calling response.json(), so update each function to verify response.ok after fetch; if not ok, read the response body (preferably text or safe JSON parse), and throw an Error that includes the HTTP status and returned message/body so callers can handle failures. Specifically, in fetchMovie, fetchMovies, and updateMovie add a conditional like "if (!response.ok) { const errBody = await response.text(); throw new Error(`Fetch <functionName> failed ${response.status}: ${errBody}`) }" (replace <functionName> with the real function names) before returning response.json().examples/solid/offline/src/movies.ts (1)
43-45: Type the error handler parameters properly.Using
anytypes loses type safety. Consider using proper types or at minimum a more descriptive approach.Proposed fix
- onError: (_: any, __: any, context: any) => { + onError: (_error, _variables, context) => { queryClient.setQueryData(movieKeys.detail(movieId), context.previousData) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/solid/offline/src/movies.ts` around lines 43 - 45, The onError handler currently uses any for all params; change it to use meaningful types (avoid any) such as (error: unknown, variables: { movieId: string } | undefined, context: { previousData?: Movie | null } | undefined) => { queryClient.setQueryData(movieKeys.detail(movieId), context?.previousData) }; import or reference the Movie type used by your fetcher and/or define a small MutationContext type for context to improve type-safety while keeping the call to queryClient.setQueryData(movieKeys.detail(movieId), context?.previousData) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/react/guides/request-waterfalls.md`:
- Line 187: Summary: Hyphenate compound modifiers like "real world" to
"real-world" for grammar consistency. Fix: update the prose that mentions
"<Comments>" and "<Article>" to use "real-world applications" and any other
compound modifiers (e.g., "child might be nested far below the parent" if used
as a modifier) to be hyphenated; search for occurrences around the discussion of
hoisting the comments query to the parent and at the alternate locations noted
(also applies to lines referencing 368-369) and replace plain compounds with
hyphenated forms to maintain consistent compound-adjective styling.
- Around line 272-275: The fenced code block showing the waterfall steps (the
lines containing "getFeed()" and "getGraphDataById()") is missing a language
tag; update that triple-backtick block to include a language specifier (e.g.,
"text") so Markdownlint stops warning and the snippet is properly annotated.
In `@docs/framework/solid/guides/mutations.md`:
- Line 275: The heading "Persisting Offline mutations" is an h3 (###) that skips
from the document's h1; change it to h2 (## Persisting Offline mutations) so
heading levels increment correctly per markdownlint MD001 and maintain document
structure; update the heading token for "Persisting Offline mutations" to use
two hashes and ensure nearby headings remain properly nested.
- Around line 173-190: The onSuccess callbacks in the Example7 snippet use an
incorrect four-parameter signature (data, variables, onMutateResult, context);
update both the global useMutation onSuccess and the per-call mutate onSuccess
to the correct signature used by this library (e.g., onSuccess: (data,
variables, context) => { ... }) and adjust any references to the removed
parameter (onMutateResult) accordingly; specifically edit the useMutation block
and the mutate(...) call that reference addTodo, useMutation, and mutate to use
(data, variables, context) so the docs match the real API.
- Around line 143-168: The example callbacks for useMutation and mutate use an
incorrect fourth parameter name "onMutateResult"; update all
onSuccess/onError/onSettled callback signatures in the useMutation options and
in the mutate call to match the correct mutation callback shape (three params):
onSuccess(data, variables, context), onError(error, variables, context),
onSettled(dataOrUndefined, errorOrUndefined, variables, context) — or if your
API expects three params for settled, use (data, error, context); replace any
"onMutateResult" occurrences with "context" and remove the extra parameter so
the callbacks in useMutation and mutate reference the correct symbols and
ordering.
- Around line 221-255: The callbacks provided to queryClient.setMutationDefaults
use the wrong signatures—onMutate currently expects (variables, context) and
returns a context named onMutateResult, while onSuccess/onError are declared
with an extra parameter; fix by making onMutate async (variables) => { await
queryClient.cancelQueries(['todos']); const optimisticTodo = { id: uuid(),
title: variables.title }; queryClient.setQueryData(['todos'], old => [...old,
optimisticTodo]); return { optimisticTodo } } and update onSuccess and onError
to use the proper signature (data, variables, context) => {
queryClient.setQueryData(['todos'], old => old.map/todo filtering using
context.optimisticTodo.id) } so they read context.optimisticTodo (replace uses
of onMutateResult and context.client with the returned context and the outer
queryClient respectively).
In `@docs/framework/solid/guides/prefetching.md`:
- Around line 216-253: The example uses the old constructor-style API; update it
to the v1 functional API by replacing RouterContext/createRootRoute/new Route
with createRootRoute (or createRootRouteWithContext<MyContext>() for typed
context), createRoute for individual routes, build the tree with
rootRoute.addChildren([ ...childRoutes... ]), and instantiate the router via
createRouter({ routeTree: rootRoute }); also update any usages of RouterContext
methods to the new route context hooks (e.g., useRouteContext) and ensure
loader/beforeLoad signatures match the v1 createRoute API.
In `@docs/framework/solid/guides/queries.md`:
- Around line 7-95: The MD042 warnings are caused by the inline marker comments
like [//]: # 'SubscribeDescription' and [//]: # 'Example' in the markdown; wrap
the marker blocks with markdownlint disable/enable comments (e.g. insert <!--
markdownlint-disable MD042 --> before the sequence of [//]: # '...' markers and
<!-- markdownlint-enable MD042 --> after) or convert those marker lines to a
neutral HTML comment format so MD042 is suppressed for the markers (update the
marker blocks surrounding useQuery examples and the
SubscribeDescription/Example/Example2/Example3/Example4 markers accordingly).
In `@docs/framework/solid/guides/query-cancellation.md`:
- Around line 111-120: The queryFn passed to useQuery (in the todosQuery
example) does not return the Promise from client.request so the query resolves
to undefined; update the queryFn to return the client.request call (i.e., ensure
the function passed as queryFn returns the result of client.request({ document:
query, signal })) so useQuery receives the fetched data—look for GraphQLClient,
client.request, useQuery, queryFn and todosQuery in the snippet and add the
missing return.
In `@examples/solid/offline/README.md`:
- Around line 5-6: The README references running `npm run start` but
package.json only defines the `dev` script, causing the command to fail; update
the README.md instructions to use `npm run dev` (or add a `start` script to
package.json that maps to `dev`) so the command matches the actual script
name—look for the script entries in package.json and the command lines in
README.md and make them consistent.
In `@examples/solid/offline/src/App.tsx`:
- Around line 146-153: submitForm can pass an undefined comment into
updateMovie.mutate which triggers a TypeError in api.ts when
comment.toUpperCase() is called; change submitForm to ensure a defined string is
sent (e.g., compute const safeComment = comment() ??
movieQuery.data?.movie.comment ?? "" before calling updateMovie.mutate) or
validate and coerce comment() to a string and pass that into updateMovie.mutate
so api.ts always receives a string value; update the submitForm function (and
any hook return usage of comment()) to guarantee non-undefined input to
updateMovie.mutate.
In `@examples/solid/offline/src/index.tsx`:
- Around line 6-14: worker.start() is asynchronous and currently races with
initial renders; await the worker startup before calling render so MSW is ready
to intercept requests — change the flow so you await the Promise returned by
worker.start() (referencing worker.start()) and only then call render(...) to
mount <App /> (referencing render and App) to eliminate the race condition.
In `@examples/solid/offline/src/movies.ts`:
- Around line 22-42: In updateMovie's onMutate handler capture the current
comment value into a local variable before calling setComment(undefined) so the
optimistic update uses the preserved value; specifically, inside the onMutate
for updateMovie (which uses movieKeys.detail(movieId)) read comment() into e.g.
previousComment, then call setComment(undefined), and pass previousComment into
queryClient.setQueryData(...) when setting movie.comment and return
previousComment in the mutation context so rollback can restore it if needed.
---
Duplicate comments:
In `@docs/framework/solid/guides/optimistic-updates.md`:
- Around line 101-129: The mutation callbacks use a `context.client` API that
may not match Solid Query's actual context shape; verify and update
useMutation's callbacks (onMutate, onError, onSettled) to use the correct
context object and method names (e.g., confirm whether cancelQueries,
getQueryData, setQueryData, invalidateQueries are accessed via context.client or
directly via context or a queryClient provided by useMutation), then adjust
references to newTodo/onMutateResult accordingly so onMutate returns the proper
snapshot shape and onError/onSettled read that shape correctly (ensure functions
like cancelQueries(['todos', newTodo.id]) and setQueryData/getQueryData use the
actual API signature and key format used by Solid Query).
---
Nitpick comments:
In `@docs/framework/solid/guides/disabling-queries.md`:
- Around line 23-38: The Switch/Match block currently checks todosQuery.data
before todosQuery.isError and todosQuery.isLoading, which can hide error/loading
states when stale data exists; reorder the Match checks in the Switch so that
todosQuery.isError and todosQuery.isLoading are evaluated before todosQuery.data
(i.e., check todosQuery.isError, then todosQuery.isLoading, then
todosQuery.data, finally the fallback Match when={true}) to ensure explicit
prioritization of error/loading states in the rendering logic for the todosQuery
component.
In `@docs/framework/solid/guides/placeholder-query-data.md`:
- Line 22: Add an explicit inline comment above the "Placeholder Data
Memoization" h3 to indicate the heading-level skip is intentional because this
is a ref-based document whose parent h2 sections are inherited from the React
guide; this comment should reference the h3 ("Placeholder Data Memoization") so
reviewers and linters understand the skip (or alternatively add a markdownlint
disable directive at the top of the file for the specific heading-level rule) to
prevent the isolated markdownlint violation.
In `@examples/solid/offline/src/api.ts`:
- Around line 17-42: The three API helpers (fetchMovie, fetchMovies,
updateMovie) don't check response.ok before calling response.json(), so update
each function to verify response.ok after fetch; if not ok, read the response
body (preferably text or safe JSON parse), and throw an Error that includes the
HTTP status and returned message/body so callers can handle failures.
Specifically, in fetchMovie, fetchMovies, and updateMovie add a conditional like
"if (!response.ok) { const errBody = await response.text(); throw new
Error(`Fetch <functionName> failed ${response.status}: ${errBody}`) }" (replace
<functionName> with the real function names) before returning response.json().
In `@examples/solid/offline/src/movies.ts`:
- Around line 43-45: The onError handler currently uses any for all params;
change it to use meaningful types (avoid any) such as (error: unknown,
variables: { movieId: string } | undefined, context: { previousData?: Movie |
null } | undefined) => { queryClient.setQueryData(movieKeys.detail(movieId),
context?.previousData) }; import or reference the Movie type used by your
fetcher and/or define a small MutationContext type for context to improve
type-safety while keeping the call to
queryClient.setQueryData(movieKeys.detail(movieId), context?.previousData)
intact.
|
|
||
| [//]: # 'NestedExample' | ||
|
|
||
| Note that while `<Comments>` takes a prop `id` from the parent, that id is already available when the `<Article>` renders so there is no reason we could not fetch the comments at the same time as the article. In real world applications, the child might be nested far below the parent and these kinds of waterfalls are often trickier to spot and fix, but for our example, one way to flatten the waterfall would be to hoist the comments query to the parent instead: |
Contributor
There was a problem hiding this comment.
Hyphenate compound modifiers for grammar consistency.
These read as compound adjectives and should be hyphenated.
✍️ Suggested edits
- In real world applications, the child might be nested far below the parent
+ In real-world applications, the child might be nested far below the parent
- - Include all data fetching code in the main bundle, even if we seldom use it
- - Put the data fetching code in the code split bundle, but with a request waterfall
+ - Include all data-fetching code in the main bundle, even if we seldom use it
+ - Put the data-fetching code in the code-split bundle, but with a request-waterfallAlso applies to: 368-369
🧰 Tools
🪛 LanguageTool
[grammar] ~187-~187: Use a hyphen to join words.
Context: ...at the same time as the article. In real world applications, the child might be n...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/react/guides/request-waterfalls.md` at line 187, Summary:
Hyphenate compound modifiers like "real world" to "real-world" for grammar
consistency. Fix: update the prose that mentions "<Comments>" and "<Article>" to
use "real-world applications" and any other compound modifiers (e.g., "child
might be nested far below the parent" if used as a modifier) to be hyphenated;
search for occurrences around the discussion of hoisting the comments query to
the parent and at the alternate locations noted (also applies to lines
referencing 368-369) and replace plain compounds with hyphenated forms to
maintain consistent compound-adjective styling.
Comment on lines
272
to
275
| ``` | ||
| 1. |> getFeed() | ||
| 2. |> getGraphDataById() | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced code block.
Markdownlint warns about missing language specifier here. Consider marking it as plain text.
💡 Suggested change
-```
+```text
1. |> getFeed()
2. |> getGraphDataById()</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 272-272: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/react/guides/request-waterfalls.md` around lines 272 - 275,
The fenced code block showing the waterfall steps (the lines containing
"getFeed()" and "getGraphDataById()") is missing a language tag; update that
triple-backtick block to include a language specifier (e.g., "text") so
Markdownlint stops warning and the snippet is properly annotated.
Comment on lines
+143
to
+168
| ```tsx | ||
| useMutation(() => ({ | ||
| mutationFn: addTodo, | ||
| onSuccess: (data, variables, onMutateResult, context) => { | ||
| // I will fire first | ||
| }, | ||
| onError: (error, variables, onMutateResult, context) => { | ||
| // I will fire first | ||
| }, | ||
| onSettled: (data, error, variables, onMutateResult, context) => { | ||
| // I will fire first | ||
| }, | ||
| })) | ||
|
|
||
| mutate(todo, { | ||
| onSuccess: (data, variables, onMutateResult, context) => { | ||
| // I will fire second! | ||
| }, | ||
| onError: (error, variables, onMutateResult, context) => { | ||
| // I will fire second! | ||
| }, | ||
| onSettled: (data, error, variables, onMutateResult, context) => { | ||
| // I will fire second! | ||
| }, | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Same callback signature issue in Example6.
The callbacks here also use the incorrect onMutateResult parameter pattern.
Proposed fix
useMutation(() => ({
mutationFn: addTodo,
- onSuccess: (data, variables, onMutateResult, context) => {
+ onSuccess: (data, variables, context) => {
// I will fire first
},
- onError: (error, variables, onMutateResult, context) => {
+ onError: (error, variables, context) => {
// I will fire first
},
- onSettled: (data, error, variables, onMutateResult, context) => {
+ onSettled: (data, error, variables, context) => {
// I will fire first
},
}))
mutate(todo, {
- onSuccess: (data, variables, onMutateResult, context) => {
+ onSuccess: (data, variables, context) => {
// I will fire second!
},
- onError: (error, variables, onMutateResult, context) => {
+ onError: (error, variables, context) => {
// I will fire second!
},
- onSettled: (data, error, variables, onMutateResult, context) => {
+ onSettled: (data, error, variables, context) => {
// I will fire second!
},
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/solid/guides/mutations.md` around lines 143 - 168, The example
callbacks for useMutation and mutate use an incorrect fourth parameter name
"onMutateResult"; update all onSuccess/onError/onSettled callback signatures in
the useMutation options and in the mutate call to match the correct mutation
callback shape (three params): onSuccess(data, variables, context),
onError(error, variables, context), onSettled(dataOrUndefined, errorOrUndefined,
variables, context) — or if your API expects three params for settled, use
(data, error, context); replace any "onMutateResult" occurrences with "context"
and remove the extra parameter so the callbacks in useMutation and mutate
reference the correct symbols and ordering.
Comment on lines
+173
to
+190
| ```tsx | ||
| useMutation(() => ({ | ||
| mutationFn: addTodo, | ||
| onSuccess: (data, variables, onMutateResult, context) => { | ||
| // Will be called 3 times | ||
| }, | ||
| })) | ||
|
|
||
| const todos = ['Todo 1', 'Todo 2', 'Todo 3'] | ||
| todos.forEach((todo) => { | ||
| mutate(todo, { | ||
| onSuccess: (data, variables, onMutateResult, context) => { | ||
| // Will execute only once, for the last mutation (Todo 3), | ||
| // regardless which mutation resolves first | ||
| }, | ||
| }) | ||
| }) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Same callback signature issue in Example7.
Proposed fix
useMutation(() => ({
mutationFn: addTodo,
- onSuccess: (data, variables, onMutateResult, context) => {
+ onSuccess: (data, variables, context) => {
// Will be called 3 times
},
}))
const todos = ['Todo 1', 'Todo 2', 'Todo 3']
todos.forEach((todo) => {
mutate(todo, {
- onSuccess: (data, variables, onMutateResult, context) => {
+ onSuccess: (data, variables, context) => {
// Will execute only once, for the last mutation (Todo 3),
// regardless which mutation resolves first
},
})
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/solid/guides/mutations.md` around lines 173 - 190, The
onSuccess callbacks in the Example7 snippet use an incorrect four-parameter
signature (data, variables, onMutateResult, context); update both the global
useMutation onSuccess and the per-call mutate onSuccess to the correct signature
used by this library (e.g., onSuccess: (data, variables, context) => { ... })
and adjust any references to the removed parameter (onMutateResult) accordingly;
specifically edit the useMutation block and the mutate(...) call that reference
addTodo, useMutation, and mutate to use (data, variables, context) so the docs
match the real API.
Comment on lines
+221
to
+255
| ```tsx | ||
| const queryClient = new QueryClient() | ||
|
|
||
| // Define the "addTodo" mutation | ||
| queryClient.setMutationDefaults(['addTodo'], { | ||
| mutationFn: addTodo, | ||
| onMutate: async (variables, context) => { | ||
| // Cancel current queries for the todos list | ||
| await context.client.cancelQueries({ queryKey: ['todos'] }) | ||
|
|
||
| // Create optimistic todo | ||
| const optimisticTodo = { id: uuid(), title: variables.title } | ||
|
|
||
| // Add optimistic todo to todos list | ||
| context.client.setQueryData(['todos'], (old) => [...old, optimisticTodo]) | ||
|
|
||
| // Return a result with the optimistic todo | ||
| return { optimisticTodo } | ||
| }, | ||
| onSuccess: (result, variables, onMutateResult, context) => { | ||
| // Replace optimistic todo in the todos list with the result | ||
| context.client.setQueryData(['todos'], (old) => | ||
| old.map((todo) => | ||
| todo.id === onMutateResult.optimisticTodo.id ? result : todo, | ||
| ), | ||
| ) | ||
| }, | ||
| onError: (error, variables, onMutateResult, context) => { | ||
| // Remove optimistic todo from the todos list | ||
| context.client.setQueryData(['todos'], (old) => | ||
| old.filter((todo) => todo.id !== onMutateResult.optimisticTodo.id), | ||
| ) | ||
| }, | ||
| retry: 3, | ||
| }) |
Contributor
There was a problem hiding this comment.
Same callback signature issue in Example10.
The setMutationDefaults example also uses incorrect callback signatures with onMutateResult.
Proposed fix
queryClient.setMutationDefaults(['addTodo'], {
mutationFn: addTodo,
- onMutate: async (variables, context) => {
+ onMutate: async (variables) => {
// Cancel current queries for the todos list
- await context.client.cancelQueries({ queryKey: ['todos'] })
+ await queryClient.cancelQueries({ queryKey: ['todos'] })
// Create optimistic todo
const optimisticTodo = { id: uuid(), title: variables.title }
// Add optimistic todo to todos list
- context.client.setQueryData(['todos'], (old) => [...old, optimisticTodo])
+ queryClient.setQueryData(['todos'], (old) => [...old, optimisticTodo])
// Return a result with the optimistic todo
return { optimisticTodo }
},
- onSuccess: (result, variables, onMutateResult, context) => {
+ onSuccess: (result, variables, context) => {
// Replace optimistic todo in the todos list with the result
- context.client.setQueryData(['todos'], (old) =>
+ queryClient.setQueryData(['todos'], (old) =>
old.map((todo) =>
- todo.id === onMutateResult.optimisticTodo.id ? result : todo,
+ todo.id === context.optimisticTodo.id ? result : todo,
),
)
},
- onError: (error, variables, onMutateResult, context) => {
+ onError: (error, variables, context) => {
// Remove optimistic todo from the todos list
- context.client.setQueryData(['todos'], (old) =>
- old.filter((todo) => todo.id !== onMutateResult.optimisticTodo.id),
+ queryClient.setQueryData(['todos'], (old) =>
+ old.filter((todo) => todo.id !== context.optimisticTodo.id),
)
},
retry: 3,
})📝 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
| ```tsx | |
| const queryClient = new QueryClient() | |
| // Define the "addTodo" mutation | |
| queryClient.setMutationDefaults(['addTodo'], { | |
| mutationFn: addTodo, | |
| onMutate: async (variables, context) => { | |
| // Cancel current queries for the todos list | |
| await context.client.cancelQueries({ queryKey: ['todos'] }) | |
| // Create optimistic todo | |
| const optimisticTodo = { id: uuid(), title: variables.title } | |
| // Add optimistic todo to todos list | |
| context.client.setQueryData(['todos'], (old) => [...old, optimisticTodo]) | |
| // Return a result with the optimistic todo | |
| return { optimisticTodo } | |
| }, | |
| onSuccess: (result, variables, onMutateResult, context) => { | |
| // Replace optimistic todo in the todos list with the result | |
| context.client.setQueryData(['todos'], (old) => | |
| old.map((todo) => | |
| todo.id === onMutateResult.optimisticTodo.id ? result : todo, | |
| ), | |
| ) | |
| }, | |
| onError: (error, variables, onMutateResult, context) => { | |
| // Remove optimistic todo from the todos list | |
| context.client.setQueryData(['todos'], (old) => | |
| old.filter((todo) => todo.id !== onMutateResult.optimisticTodo.id), | |
| ) | |
| }, | |
| retry: 3, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/solid/guides/mutations.md` around lines 221 - 255, The
callbacks provided to queryClient.setMutationDefaults use the wrong
signatures—onMutate currently expects (variables, context) and returns a context
named onMutateResult, while onSuccess/onError are declared with an extra
parameter; fix by making onMutate async (variables) => { await
queryClient.cancelQueries(['todos']); const optimisticTodo = { id: uuid(),
title: variables.title }; queryClient.setQueryData(['todos'], old => [...old,
optimisticTodo]); return { optimisticTodo } } and update onSuccess and onError
to use the proper signature (data, variables, context) => {
queryClient.setQueryData(['todos'], old => old.map/todo filtering using
context.optimisticTodo.id) } so they read context.optimisticTodo (replace uses
of onMutateResult and context.client with the returned context and the outer
queryClient respectively).
Comment on lines
+111
to
+120
| ```tsx | ||
| const client = new GraphQLClient(endpoint) | ||
|
|
||
| const todosQuery = useQuery(() => ({ | ||
| queryKey: ['todos'], | ||
| queryFn: ({ signal }) => { | ||
| client.request({ document: query, signal }) | ||
| }, | ||
| })) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Missing return statement in queryFn.
The queryFn doesn't return the promise from client.request(), so the query will resolve with undefined instead of the actual data.
Proposed fix
const todosQuery = useQuery(() => ({
queryKey: ['todos'],
queryFn: ({ signal }) => {
- client.request({ document: query, signal })
+ return client.request({ document: query, signal })
},
}))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/framework/solid/guides/query-cancellation.md` around lines 111 - 120,
The queryFn passed to useQuery (in the todosQuery example) does not return the
Promise from client.request so the query resolves to undefined; update the
queryFn to return the client.request call (i.e., ensure the function passed as
queryFn returns the result of client.request({ document: query, signal })) so
useQuery receives the fetched data—look for GraphQLClient, client.request,
useQuery, queryFn and todosQuery in the snippet and add the missing return.
Comment on lines
+5
to
+6
| - `npm install` | ||
| - `npm run start` |
Contributor
There was a problem hiding this comment.
Incorrect script name in instructions.
The README references npm run start, but the package.json defines a dev script instead. This will cause the command to fail.
📝 Proposed fix
- `npm install`
-- `npm run start`
+- `npm run dev`📝 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
| - `npm install` | |
| - `npm run start` | |
| - `npm install` | |
| - `npm run dev` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/solid/offline/README.md` around lines 5 - 6, The README references
running `npm run start` but package.json only defines the `dev` script, causing
the command to fail; update the README.md instructions to use `npm run dev` (or
add a `start` script to package.json that maps to `dev`) so the command matches
the actual script name—look for the script entries in package.json and the
command lines in README.md and make them consistent.
Comment on lines
+146
to
+153
| function submitForm(event: SubmitEvent) { | ||
| event.preventDefault() | ||
|
|
||
| updateMovie.mutate({ | ||
| id: props.movieId, | ||
| comment: comment(), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Potential undefined comment when submitting.
comment() could return undefined if no local edits were made (it falls back to movieQuery.data?.movie.comment in the hook return). When mutating, if comment is undefined, the API call in api.ts line 75 will call comment.toUpperCase() which throws a TypeError.
Proposed fix
function submitForm(event: SubmitEvent) {
event.preventDefault()
+ const currentComment = comment()
+ if (currentComment === undefined) return
+
updateMovie.mutate({
id: props.movieId,
- comment: comment(),
+ comment: currentComment,
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/solid/offline/src/App.tsx` around lines 146 - 153, submitForm can
pass an undefined comment into updateMovie.mutate which triggers a TypeError in
api.ts when comment.toUpperCase() is called; change submitForm to ensure a
defined string is sent (e.g., compute const safeComment = comment() ??
movieQuery.data?.movie.comment ?? "" before calling updateMovie.mutate) or
validate and coerce comment() to a string and pass that into updateMovie.mutate
so api.ts always receives a string value; update the submitForm function (and
any hook return usage of comment()) to guarantee non-undefined input to
updateMovie.mutate.
Comment on lines
+6
to
+14
| worker.start() | ||
|
|
||
| render( | ||
| () => ( | ||
| <div style={{ padding: '16px' }}> | ||
| <App /> | ||
| </div> | ||
| ), | ||
| document.getElementById('root')!, |
Contributor
There was a problem hiding this comment.
Race condition: worker.start() should be awaited.
worker.start() returns a Promise. Rendering the app immediately without waiting could cause initial fetch requests to bypass MSW interception if they fire before the worker is ready.
Proposed fix
-worker.start()
-
-render(
- () => (
- <div style={{ padding: '16px' }}>
- <App />
- </div>
- ),
- document.getElementById('root')!,
-)
+worker.start().then(() => {
+ render(
+ () => (
+ <div style={{ padding: '16px' }}>
+ <App />
+ </div>
+ ),
+ document.getElementById('root')!,
+ )
+})📝 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
| worker.start() | |
| render( | |
| () => ( | |
| <div style={{ padding: '16px' }}> | |
| <App /> | |
| </div> | |
| ), | |
| document.getElementById('root')!, | |
| worker.start().then(() => { | |
| render( | |
| () => ( | |
| <div style={{ padding: '16px' }}> | |
| <App /> | |
| </div> | |
| ), | |
| document.getElementById('root')!, | |
| ) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/solid/offline/src/index.tsx` around lines 6 - 14, worker.start() is
asynchronous and currently races with initial renders; await the worker startup
before calling render so MSW is ready to intercept requests — change the flow so
you await the Promise returned by worker.start() (referencing worker.start())
and only then call render(...) to mount <App /> (referencing render and App) to
eliminate the race condition.
Comment on lines
+22
to
+42
| const updateMovie = useMutation(() => ({ | ||
| mutationKey: movieKeys.detail(movieId), | ||
| onMutate: async () => { | ||
| await queryClient.cancelQueries({ queryKey: movieKeys.detail(movieId) }) | ||
| const previousData = queryClient.getQueryData< | ||
| Awaited<ReturnType<typeof api.fetchMovie>> | ||
| >(movieKeys.detail(movieId)) | ||
|
|
||
| // remove local state so that server state is taken instead | ||
| setComment(undefined) | ||
|
|
||
| queryClient.setQueryData(movieKeys.detail(movieId), { | ||
| ...previousData, | ||
| movie: { | ||
| ...previousData?.movie, | ||
| comment: comment(), | ||
| }, | ||
| }) | ||
|
|
||
| return { previousData } | ||
| }, |
Contributor
There was a problem hiding this comment.
Bug: comment() returns undefined after clearing the signal.
On line 31, setComment(undefined) clears the local comment state. Then on line 37, comment() is called to set the optimistic cache value, but it will return undefined since the signal was just cleared. The previous comment value should be captured before clearing.
Proposed fix
onMutate: async () => {
await queryClient.cancelQueries({ queryKey: movieKeys.detail(movieId) })
const previousData = queryClient.getQueryData<
Awaited<ReturnType<typeof api.fetchMovie>>
>(movieKeys.detail(movieId))
+ // Capture the current comment before clearing
+ const newComment = comment()
+
// remove local state so that server state is taken instead
setComment(undefined)
queryClient.setQueryData(movieKeys.detail(movieId), {
...previousData,
movie: {
...previousData?.movie,
- comment: comment(),
+ comment: newComment,
},
})
return { previousData }
},📝 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 updateMovie = useMutation(() => ({ | |
| mutationKey: movieKeys.detail(movieId), | |
| onMutate: async () => { | |
| await queryClient.cancelQueries({ queryKey: movieKeys.detail(movieId) }) | |
| const previousData = queryClient.getQueryData< | |
| Awaited<ReturnType<typeof api.fetchMovie>> | |
| >(movieKeys.detail(movieId)) | |
| // remove local state so that server state is taken instead | |
| setComment(undefined) | |
| queryClient.setQueryData(movieKeys.detail(movieId), { | |
| ...previousData, | |
| movie: { | |
| ...previousData?.movie, | |
| comment: comment(), | |
| }, | |
| }) | |
| return { previousData } | |
| }, | |
| const updateMovie = useMutation(() => ({ | |
| mutationKey: movieKeys.detail(movieId), | |
| onMutate: async () => { | |
| await queryClient.cancelQueries({ queryKey: movieKeys.detail(movieId) }) | |
| const previousData = queryClient.getQueryData< | |
| Awaited<ReturnType<typeof api.fetchMovie>> | |
| >(movieKeys.detail(movieId)) | |
| // Capture the current comment before clearing | |
| const newComment = comment() | |
| // remove local state so that server state is taken instead | |
| setComment(undefined) | |
| queryClient.setQueryData(movieKeys.detail(movieId), { | |
| ...previousData, | |
| movie: { | |
| ...previousData?.movie, | |
| comment: newComment, | |
| }, | |
| }) | |
| return { previousData } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/solid/offline/src/movies.ts` around lines 22 - 42, In updateMovie's
onMutate handler capture the current comment value into a local variable before
calling setComment(undefined) so the optimistic update uses the preserved value;
specifically, inside the onMutate for updateMovie (which uses
movieKeys.detail(movieId)) read comment() into e.g. previousComment, then call
setComment(undefined), and pass previousComment into
queryClient.setQueryData(...) when setting movie.comment and return
previousComment in the mutation context so rollback can restore it if needed.
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.
🎯 Changes
ref-based withreplacerules and marker overridescreateSignal,Show/For/Switch, no destructuring)✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes
Documentation
New Examples