fix(react-start): Update useServerFn return type to be undefined after redirect #6295
Conversation
📝 WalkthroughWalkthroughThree hooks updated: two Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
|
View your CI Pipeline Execution ↗ for commit 31cadc5
☁️ Nx Cloud last updated this comment at |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/react/start-supabase-basic/src/hooks/useMutation.ts (1)
35-35: Missing dependency in useCallback array.The callback uses both
opts.fnandopts.onSuccess(line 24), but the dependency array only includesopts.fn. This violates the exhaustive-deps rule and can cause stale closures where changes toopts.onSuccessare not reflected in the callback.🔎 Recommended fix
- [opts.fn], + [opts],Or destructure and include both dependencies:
+ const { fn, onSuccess } = opts const mutate = React.useCallback( async (variables: TVariables): Promise<TData | undefined> => { setStatus('pending') setSubmittedAt(Date.now()) setVariables(variables) // try { - const data = await opts.fn(variables) + const data = await fn(variables) if (data !== undefined) { - await opts.onSuccess?.({ data }) + await onSuccess?.({ data }) setStatus('success') setError(undefined) setData(data) return data } } catch (err) { setStatus('error') setError(err as TError) } }, - [opts.fn], + [fn, onSuccess], )e2e/react-start/basic-auth/src/hooks/useMutation.ts (1)
35-35: Missing dependency in useCallback array.The callback uses both
opts.fnandopts.onSuccess(line 24), but the dependency array only includesopts.fn. This violates the exhaustive-deps rule and can cause stale closures where changes toopts.onSuccessare not reflected in the callback.🔎 Recommended fix
- [opts.fn], + [opts],Or destructure and include both dependencies:
+ const { fn, onSuccess } = opts const mutate = React.useCallback( async (variables: TVariables): Promise<TData | undefined> => { setStatus('pending') setSubmittedAt(Date.now()) setVariables(variables) // try { - const data = await opts.fn(variables) + const data = await fn(variables) if (data !== undefined) { - await opts.onSuccess?.({ data }) + await onSuccess?.({ data }) setStatus('success') setError(undefined) setData(data) return data } } catch (err: any) { setStatus('error') setError(err) } }, - [opts.fn], + [fn, onSuccess], )
🧹 Nitpick comments (1)
packages/react-start/src/useServerFn.ts (1)
29-29: Type assertion defeats the purpose of the type change.The
as anycast on line 29 bypasses TypeScript's type checking, undermining the type safety improvement made on line 6. While this may be necessary to satisfy TypeScript's callback typing constraints, it means callers won't benefit from the updated return type signature.Consider whether the type assertion can be refined to preserve some type safety, for example:
) as (...args: Parameters<T>) => ReturnType<T> | Promise<void>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/react-start/basic-auth/src/hooks/useMutation.tsexamples/react/start-supabase-basic/src/hooks/useMutation.tspackages/react-start/src/useServerFn.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/react-start/src/useServerFn.tse2e/react-start/basic-auth/src/hooks/useMutation.tsexamples/react/start-supabase-basic/src/hooks/useMutation.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/react-start/src/useServerFn.tse2e/react-start/basic-auth/src/hooks/useMutation.tsexamples/react/start-supabase-basic/src/hooks/useMutation.ts
🧬 Code graph analysis (1)
examples/react/start-supabase-basic/src/hooks/useMutation.ts (1)
examples/solid/kitchen-sink/src/useMutation.tsx (2)
variables(37-39)data(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
packages/react-start/src/useServerFn.ts (1)
6-6: Type broadening correctly reflects redirect behavior.The addition of
Promise<void>to the return type accurately captures the scenario whererouter.navigateis called during a redirect (line 22), which returnsPromise<void>.examples/react/start-supabase-basic/src/hooks/useMutation.ts (1)
4-4: Signature change correctly accommodates redirect scenarios.The updated signature
Promise<TData | void>properly reflects that the mutation function may return void when a redirect occurs, aligning with theuseServerFntype changes.e2e/react-start/basic-auth/src/hooks/useMutation.ts (1)
4-4: Signature change correctly accommodates redirect scenarios.The updated signature
Promise<TData | void>properly reflects that the mutation function may return void when a redirect occurs, aligning with theuseServerFntype changes.
| if (data) { | ||
| await opts.onSuccess?.({ data }) | ||
| setStatus('success') | ||
| setError(undefined) | ||
| setData(data) | ||
| return data | ||
| } |
There was a problem hiding this comment.
Truthiness check will fail for valid falsy return values.
The if (data) check on line 23 uses JavaScript truthiness, which evaluates to false for valid data values like 0, false, "", or NaN. This will incorrectly skip success handling when TData is a falsy type.
🔎 Proposed fix using explicit undefined check
- if (data) {
+ if (data !== undefined) {
await opts.onSuccess?.({ data })
setStatus('success')
setError(undefined)
setData(data)
return data
}Additional concern: When data is undefined (redirect scenario), the status remains 'pending' indefinitely. Consider whether the status should transition to a different state (e.g., 'idle' or a new 'redirected' state) to accurately reflect the mutation's completion.
Note: This file is nearly identical to examples/react/start-supabase-basic/src/hooks/useMutation.ts. Consider consolidating these duplicate implementations into a shared utility.
🤖 Prompt for AI Agents
In e2e/react-start/basic-auth/src/hooks/useMutation.ts around lines 23 to 29,
the code uses a truthiness check (`if (data)`) which incorrectly treats valid
falsy values (0, false, "", NaN) as no-data and also leaves status as 'pending'
when data is undefined (redirect scenario); change the check to an explicit
undefined test (e.g., `if (data !== undefined)`) so valid falsy responses
trigger the onSuccess flow, set status to 'success' and setData for that case,
and add an explicit branch for `data === undefined` to transition status to a
suitable state (e.g., 'idle' or a new 'redirected' state) to mark the mutation
complete; also consider consolidating this file with the nearly identical
example at examples/react/start-supabase-basic/src/hooks/useMutation.ts into a
shared utility.
| if (data) { | ||
| await opts.onSuccess?.({ data }) | ||
| setStatus('success') | ||
| setError(undefined) | ||
| setData(data) | ||
| return data | ||
| } |
There was a problem hiding this comment.
Truthiness check will fail for valid falsy return values.
The if (data) check on line 23 uses JavaScript truthiness, which evaluates to false for valid data values like 0, false, "", or NaN. This will incorrectly skip success handling when TData is a falsy type.
🔎 Proposed fix using explicit undefined check
- if (data) {
+ if (data !== undefined) {
await opts.onSuccess?.({ data })
setStatus('success')
setError(undefined)
setData(data)
return data
}Additional concern: When data is undefined (redirect scenario), the status remains 'pending' indefinitely. Consider whether the status should transition to a different state (e.g., 'idle' or a new 'redirected' state) to accurately reflect the mutation's completion.
📝 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.
| if (data) { | |
| await opts.onSuccess?.({ data }) | |
| setStatus('success') | |
| setError(undefined) | |
| setData(data) | |
| return data | |
| } | |
| if (data !== undefined) { | |
| await opts.onSuccess?.({ data }) | |
| setStatus('success') | |
| setError(undefined) | |
| setData(data) | |
| return data | |
| } |
🤖 Prompt for AI Agents
In examples/react/start-supabase-basic/src/hooks/useMutation.ts around lines 23
to 29, the current truthiness check "if (data)" will skip valid falsy results
(0, false, "", NaN); change the branch to explicitly check for undefined (e.g.,
"if (data !== undefined)") so success handling always runs for valid falsy
TData, call opts.onSuccess with the data, set error undefined and setData(data)
as before; additionally handle the case where data === undefined (redirect
scenario) by transitioning status out of 'pending' to an appropriate state (for
example setStatus('idle') or add/assign a 'redirected' status) so the mutation
does not remain stuck in 'pending'.
1 participant
useServerFnreturns a promise that resolves toundefinedwhen the server function redirects. This PR updates the return type to reflect that.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.