Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant useServerAction
participant Action
Note over Component: User calls execute(input)
Component->>useServerAction: execute(input)
useServerAction->>useServerAction: startTransition(async action)
useServerAction->>Action: Perform async action
Action-->>useServerAction: Return result/error
useServerAction->>useServerAction: Update refs (executedAt, input)
useServerAction-->>Component: Resolve Promise with result
Component->>useServerAction: reset()
useServerAction->>useServerAction: Reset refs
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/openapi
@orpc/react
@orpc/openapi-client
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/svelte-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
![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 (4)
packages/react/src/hooks/action-hooks.ts (4)
83-91: Consider freezingPENDING_STATEto avoid accidental mutation
PENDING_STATEis intended to be a constant baseline snapshot.
Because it is later spread into the returned object (...currentState), an inadvertent mutation would affect every subsequent result.
You can safeguard against this by making it immutable:-const PENDING_STATE = { +const PENDING_STATE = Object.freeze({ data: undefined, error: null, isIdle: false, isPending: true, isSuccess: false, isError: false, status: 'pending', -} +}) as const
105-110: Typing omission hidesexecutedAt&inputfrom state but not from business logicState is now declared with
Omit<...,'executedAt' | 'input'>, which matches the ref-based design.
However, nothing prevents future contributors from mistakenly trying tosetState({ executedAt })and wondering why TS complains.A small doc‐comment above
INITIAL_STATEor the state declaration explaining that “executedAtandinputdeliberately live in refs to avoid re-renders” would save time.
Alternatively, extract the state type into a named alias (InternalActionState) to make the omission explicit.
122-153: Promise wrapper leaks unhandled rejection on interceptor error pathsInside
startTransitionyou wrap the async body withsafe(), so most errors are captured.
However, if React itself throws (e.g., duringsetState) or an unexpected runtime error escapes beforesafe, the outer Promise will neither resolve nor reject, leaving callers hanging.You can propagate fatal errors by surrounding the
awaitblock withtry/catchand callingreject:-return new Promise((resolve) => { +return new Promise((resolve, reject) => { startTransition(async () => { - const result = await safe(/* … */) - setState({ … }) - resolve(result) + try { + const result = await safe(/* … */) + setState({ … }) + resolve(result) + } catch (err) { + reject(err) + throw err // still surface to React error boundaries + } }) })This keeps the hook predictable for imperative consumers.
155-167:useMemodependencies omitoptions.interceptorsmutations
executerecompiles when the values insideoptions.interceptorschange (spread into the memo list earlier), butresultonly depends onresetandexecute.
If the parent component swaps interceptors, the returnedexecuteis fresh, yet the outerresultobject (and referential equality) remains unchanged. DownstreamuseEffecthooks that rely onresult.executeidentity may miss the update.Safest path: include
executealone (it already closes over updated interceptors) and document thatexecuteidentity can change whenoptions.interceptorsmutate, or explicitly add a// eslint-disable-next-line react-hooks/exhaustive-depswith rationale.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react/src/hooks/action-hooks.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: publish-commit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react/src/hooks/action-hooks.ts (1)
122-160: Potential race condition with timestamp-based execution trackingThe implementation addresses race conditions by tracking the execution timestamp, but using a timestamp as an execution ID could potentially lead to collisions if two executions happen in the exact same millisecond.
Consider using a more unique identifier like a Symbol or incrementing number instead:
+const executionIdRef = useRef(0) const execute = useCallback(async (input: TInput, executeOptions: UseServerActionExecuteOptions<TInput, TOutput, UnactionableError<TError>> = {}) => { + const currentExecutionId = ++executionIdRef.current const executedAt = new Date() executedAtRef.current = executedAt // ... return new Promise((resolve) => { startTransition(async () => { // ... - if (executedAtRef.current === executedAt) { + if (executionIdRef.current === currentExecutionId) { setState({ // ... }) } // ... }) }) }, [/* ... */])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react/src/hooks/action-hooks.test.tsx(3 hunks)packages/react/src/hooks/action-hooks.ts(3 hunks)
🔇 Additional comments (10)
packages/react/src/hooks/action-hooks.ts (5)
6-6: Import dependency updated to include useTransitionThe import now includes
useTransitionfrom React, which is used to manage asynchronous state transitions.
83-91: Well-structured PENDING_STATE constantGood addition of a separate constant for pending state, which helps improve code readability and maintainability.
112-114: Better state management with useTransition and refsMoving
executedAtto a ref and using React'suseTransitionis a good improvement for managing asynchronous state updates, allowing React to better prioritize renders.
116-120: Reset function doesn't cancel in-flight transitionsThe current reset implementation clears refs and state but doesn't actually cancel pending transitions. When an in-flight action completes after reset, its callback still executes (though state updates are prevented).
Consider enhancing the reset function to support true cancellation:
+const currentExecutionRef = useRef<symbol | null>(null) const execute = useCallback(async (input: TInput, executeOptions: UseServerActionExecuteOptions<TInput, TOutput, UnactionableError<TError>> = {}) => { + const execId = Symbol('execution') + currentExecutionRef.current = execId const executedAt = new Date() executedAtRef.current = executedAt // ... return new Promise((resolve) => { startTransition(async () => { // ... - if (executedAtRef.current === executedAt) { + if (currentExecutionRef.current === execId) { setState({ // ... }) } // ... }) }) }, [/* ... */]) const reset = useCallback(() => { executedAtRef.current = undefined + currentExecutionRef.current = null setInput(undefined) setState({ ...INITIAL_STATE }) }, [])This would provide a more robust way to handle cancellation, as mentioned in previous review comments.
162-174: Good use of useMemo for result derivationThe
useMemohook efficiently combines state, refs, and functions into a consistent result object. The conditional selection betweenPENDING_STATEand regular state based onisPendingis well implemented.packages/react/src/hooks/action-hooks.test.tsx (5)
6-8: Good test hygiene with beforeEachAdding a global
beforeEachto clear all mocks ensures a clean slate for each test, preventing cross-test contamination.
11-13: Improved test structure with reusable mock handlerExtracting the handler into a reusable mock function makes the tests more maintainable and allows for dynamic behavior changes in individual tests.
119-168: Good coverage for synchronous action errorsThis test properly verifies that synchronous errors in the action are correctly handled by the hook's state transitions.
211-269: Thorough testing of concurrent executionsThis test effectively verifies that multiple parallel executions work correctly, with each promise resolving independently and only the latest execution affecting the final state.
271-312: Good coverage of reset behavior during executionThis test confirms that reset immediately clears state even during a pending transition, and that subsequent state updates from in-flight actions are properly ignored.
However, note that this test also reinforces what was mentioned earlier - reset doesn't actually cancel the in-flight operation, it just prevents state updates. The promise still resolves with the expected result (line 304).
1 participant
Closes: https://github.com/unnoq/orpc/issues/451
Summary by CodeRabbit
New Features
Refactor
Tests