useupdatemutation usedeletemutation should include an id#9
Conversation
WalkthroughMutation hooks now expose Promise-based APIs ( Changes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
KevinBonnoron
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
tests/hooks/useDeleteMutation.test.tsx (2)
35-41: Null id path covered; consider mirroring for mutate().Add a similar case that calls mutate() (non-async) with a null id to ensure error state updates without throwing.
9-15: Avoidanyfor mocks; infer from helper.Type the mock from
getMockCollectionMethodsto keep strictness.- let mockDelete: any; + type CollectionMethods = ReturnType<typeof getMockCollectionMethods>; + let mockDelete: CollectionMethods['delete'];As per coding guidelines.
src/hooks/useDeleteMutation.ts (4)
31-50: Prevent state updates after unmount (StrictMode-friendly).Wrap updates with an
isMountedref to avoid React warnings if the component unmounts mid‑mutation.-import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; @@ - const mutateAsync = useCallback( + const isMounted = useRef(true); + useEffect(() => () => { isMounted.current = false; }, []); + + const mutateAsync = useCallback( async (options?: CommonOptions): Promise<void> => { @@ - try { - setIsPending(true); - setError(null); - await recordService.delete(id, options); - } catch (err) { - const errorMessage = err instanceof Error ? err.message : 'Error deleting record'; - setError(errorMessage); - throw new Error(errorMessage); - } finally { - setIsPending(false); - } + try { + if (isMounted.current) { setIsPending(true); setError(null); } + await recordService.delete(id, options); + } catch (err) { + const errorMessage = err instanceof Error ? err.message : 'Error deleting record'; + if (isMounted.current) { setError(errorMessage); } + throw new Error(errorMessage); + } finally { + if (isMounted.current) { setIsPending(false); } + } }, [recordService, id], );
6-24: Update the JSDoc example to match the new API.Example still shows per‑call id and boolean return. Update to pass
idat init and usemutateAsync()returningPromise<void>.Also applies to: 10-10
25-25: Consider renaming the generic to avoid shadowingRecord<…>.
Recordis a well‑known TS utility. PreferTRecordfor clarity.-export function useDeleteMutation<Record extends RecordModel = RecordModel>(collectionName: string, id: Record['id'] | null): UseDeleteMutationResult { +export function useDeleteMutation<TRecord extends RecordModel = RecordModel>(collectionName: string, id: TRecord['id'] | null): UseDeleteMutationResult {
31-50: Optional: support request cancellation viarequestKey.If
options?.requestKeyis provided, expose acancel()or internally callpb.cancelRequest(key)on unmount.Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/useDeleteMutation-example.tsx(2 hunks)examples/useUpdateMutation-example.tsx(2 hunks)src/hooks/useDeleteMutation.ts(3 hunks)src/hooks/useUpdateMutation.ts(2 hunks)src/types/useDeleteMutation.type.ts(1 hunks)src/types/useUpdateMutation.type.ts(1 hunks)tests/hooks/useCollection.test.tsx(1 hunks)tests/hooks/useDeleteMutation.test.tsx(1 hunks)tests/hooks/useRecord.test.tsx(2 hunks)tests/hooks/useUpdateMutation.test.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: TypeScript strictness: avoid any; prefer unknown when type is uncertain
Never add comments in code
**/*.{ts,tsx}: Enable strict typing and avoid any; prefer unknown when necessary
Use proper type guards to ensure runtime type safety
Files:
examples/useDeleteMutation-example.tsxtests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsxtests/hooks/useDeleteMutation.test.tsxsrc/types/useDeleteMutation.type.tssrc/hooks/useUpdateMutation.tsexamples/useUpdateMutation-example.tsxtests/hooks/useUpdateMutation.test.tsxsrc/hooks/useDeleteMutation.tssrc/types/useUpdateMutation.type.ts
examples/**/*
📄 CodeRabbit inference engine (.cursorrules)
Maintain examples directory with basic/advanced usage and TypeScript imports
Files:
examples/useDeleteMutation-example.tsxexamples/useUpdateMutation-example.tsx
tests/hooks/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/hooks/**/*.{test,spec}.{ts,tsx}: Write hook tests mirroring the hooks structure under tests/hooks/
Mock PocketBase client methods with Vitest vi.fn() and test loading, success, and error states
Verify real-time subscription setup and cleanup in tests
Use renderHook from @testing-library/react and wrap hooks with PocketBaseProvider using a mocked client
Use waitFor for assertions only: do not pass an async callback; perform actions before waitFor; always return/await the waitFor promise
Files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsxtests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
tests/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
tests/**/*.{ts,tsx}: Use Vitest as the test runner
Use React Testing Library’s renderHook for hook tests
Do not manually mock console.error in tests (handled by Vitest config)
Mock PocketBase methods comprehensively in tests
Test all hook states: loading, success, error
Files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsxtests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
src/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/hooks/**/*.{ts,tsx}: All data-fetching hooks must return a discriminated union QueryResult with isLoading, isSuccess, isError, data, and error states
Hooks must set up PocketBase real-time subscriptions via pb.collection().subscribe() and clean up subscriptions in useEffect
Support request cancellation by accepting requestKey and using pb.cancelRequest(key) where applicable
Use TanStack Query-style naming: isLoading/isSuccess/isError for query hooks; isPending for mutations; fields data and error
Use useEffect for data fetching/subscriptions and always return a cleanup function for subscriptions
Memoize fetcher functions passed into useEffect with useCallback and avoid inline object/array creation in dependency arrays
Implement React StrictMode cancellation pattern: useRef for default values and a cancelled flag in async effects to prevent stale updates
In subscription handlers, use functional setState form setData(current => ...) to avoid adding data to effect dependencies
Provide generic type parameters for records in hooks, e.g., useCollection() and useAuth()
Place all custom hooks (useAuth, useCollection, useRecord, useCreateMutation, useUpdateMutation, useDeleteMutation, usePocketBase) in src/hooks/
src/hooks/**/*.{ts,tsx}: Follow TanStack Query state names in hooks: use isLoading, isSuccess, isError, error, data (not isPending)
Use signIn/signOut for auth hooks (not login/logout) and signUp (not register)
Implement real-time subscriptions in hooks with proper cleanup
Use useEffect for side effects; do not use useCallback for data fetching
Ensure hooks surface graceful error states and meaningful error messages
Ensure cleanup of real-time subscriptions to prevent memory leaks
Files:
src/hooks/useUpdateMutation.tssrc/hooks/useDeleteMutation.ts
src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
Files:
src/hooks/useUpdateMutation.tssrc/hooks/useDeleteMutation.ts
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
Applied to files:
examples/useDeleteMutation-example.tsxsrc/hooks/useUpdateMutation.tsexamples/useUpdateMutation-example.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : useCollection and useRecord must support a transformers option (array of RecordTransformer<T>) applied sequentially (reduce) to initial fetch and real-time updates; default include dateTransformer()
Applied to files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/lib/utils.ts : applyTransformers() must catch transformer errors, log to console, and return the original record to avoid breaking flow
Applied to files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : By default, apply dateTransformer() to convert created/updated fields to Date objects; allow custom transformers or none via transformers: []
Applied to files:
tests/hooks/useCollection.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Use renderHook from testing-library/react and wrap hooks with PocketBaseProvider using a mocked client
Applied to files:
tests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:42.057Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: .cursorrules:0-0
Timestamp: 2025-10-18T12:22:42.057Z
Learning: Applies to tests/**/*.{ts,tsx} : Mock PocketBase methods comprehensively in tests
Applied to files:
tests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Mock PocketBase client methods with Vitest vi.fn() and test loading, success, and error states
Applied to files:
tests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
🧬 Code graph analysis (7)
examples/useDeleteMutation-example.tsx (1)
src/hooks/useDeleteMutation.ts (1)
useDeleteMutation(25-71)
tests/hooks/useRecord.test.tsx (1)
tests/test-utils.tsx (1)
record(86-88)
tests/hooks/useDeleteMutation.test.tsx (2)
src/hooks/useDeleteMutation.ts (1)
useDeleteMutation(25-71)tests/test-utils.tsx (1)
createWrapper(114-116)
src/hooks/useUpdateMutation.ts (2)
src/types/useUpdateMutation.type.ts (1)
UseUpdateMutationResult(8-39)src/hooks/usePocketBase.ts (1)
usePocketBase(24-31)
examples/useUpdateMutation-example.tsx (1)
src/hooks/useUpdateMutation.ts (1)
useUpdateMutation(26-73)
tests/hooks/useUpdateMutation.test.tsx (2)
src/hooks/useUpdateMutation.ts (1)
useUpdateMutation(26-73)tests/test-utils.tsx (1)
createWrapper(114-116)
src/hooks/useDeleteMutation.ts (2)
src/types/useDeleteMutation.type.ts (1)
UseDeleteMutationResult(6-35)src/hooks/usePocketBase.ts (1)
usePocketBase(24-31)
🔇 Additional comments (21)
tests/hooks/useCollection.test.tsx (1)
741-741: LGTM! Unused parameter convention applied.Prefixing the unused parameter with an underscore is a good practice that signals the parameter is intentionally ignored while the transformer throws an error.
src/types/useUpdateMutation.type.ts (1)
10-23: LGTM! API surface correctly updated.The type definitions properly reflect the new API where
idis provided at hook initialization and both synchronous (mutate) and asynchronous (mutateAsync) variants are exposed.src/hooks/useUpdateMutation.ts (4)
26-26: LGTM! Hook signature correctly updated.The addition of the
idparameter aligns with the PR objectives and provides better encapsulation of the record being updated.
32-52: LGTM! Solid implementation with proper validation and error handling.The
mutateAsyncimplementation correctly:
- Validates
idpresence before attempting update- Manages
isPendingstate through try-finally- Handles both Error and non-Error exceptions
- Rethrows errors after setting state for caller handling
54-61: LGTM! Proper fire-and-forget wrapper.The
mutatefunction correctly wrapsmutateAsyncand swallows errors since they're already handled and stored in state. This provides the synchronous API while maintaining error observability through theerrorstate.
63-72: LGTM! Return value and dependencies correctly updated.The memoized return value properly includes both
mutateandmutateAsync, and the dependency array correctly includesidsincemutateAsyncdepends on it.tests/hooks/useRecord.test.tsx (2)
427-427: LGTM! Unused parameter convention applied.Prefixing with underscore correctly signals the parameter is intentionally unused in the faulty transformer.
480-480: LGTM! Modern syntax improvement.Using template literals is more readable and consistent with modern JavaScript conventions.
examples/useUpdateMutation-example.tsx (2)
29-33: LGTM! Correctly using the new API.The example properly demonstrates the updated
mutateAsynccall with onlybodyParams, relying on theidpassed during hook initialization.
19-19: No cleanup or request cancellation logic exists; this is the library's current design pattern.Verification found no
AbortController,AbortSignal, oruseEffectcleanup patterns inuseUpdateMutationor similar mutation hooks (useCreateMutation,useDeleteMutation). WheneditingIdtransitions (null → value → null), pending mutations continue in the background without cancellation. However, the example handles this implicitly: the form hides wheneditingIdis null, preventing stale state from displaying. If cleanup is required, add auseEffectto cancel requests whenidchanges, but this would need to be applied consistently across all mutation hooks.tests/hooks/useUpdateMutation.test.tsx (5)
18-28: LGTM! Initial state verification includes new API.The test correctly verifies both
mutateandmutateAsyncare exposed as functions, aligning with the new API surface.
30-34: LGTM! Provider guard tested.Good addition to verify the hook throws when used outside the provider context.
36-42: LGTM! ID validation tested.The test correctly verifies that
mutateAsyncthrows whenidisnull, which is the expected behavior per the implementation.
44-138: LGTM! Comprehensive mutateAsync test coverage.The test suite thoroughly covers:
- Successful updates with and without options
- Error handling for Error and non-Error exceptions
isPendingstate transitions during mutation lifecycle
140-209: LGTM! Complete mutate test coverage.The test suite properly verifies the synchronous
mutatewrapper including success paths, options, and error handling scenarios.src/types/useDeleteMutation.type.ts (1)
8-19: LGTM! Type definitions correctly updated.The changes mirror the
useUpdateMutationpattern, providing both synchronous (mutate) and asynchronous (mutateAsync) APIs with clear documentation.examples/useDeleteMutation-example.tsx (2)
19-19: LGTM! Correctly using the new API.The hook invocation properly passes
deletingIdto scope the deletion, andmutateAsyncis correctly destructured for async operation handling.
33-37: LGTM! Confirmation logic properly encapsulated.The
confirmDeletefunction provides a clean separation between UI confirmation and mutation logic.tests/hooks/useDeleteMutation.test.tsx (3)
20-27: API usage and initial state assertions look correct.New signature and mutate/mutateAsync presence verified. LGTM.
29-33: Provider guard test is solid.Covers the expected error message. LGTM.
43-57: Mutation flows (success, options, error, non-Error, pending) are well covered.Good coverage of state transitions and error propagation. Once the hook exposes isError, add assertions for it (false on success, true on error).
If you add isError to the hook, update these tests to assert it. I can provide a follow‑up diff.
Also applies to: 58-69, 71-86, 88-101, 104-129
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
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)
src/hooks/useUpdateMutation.ts (1)
6-25: Update JSDoc example and description to match the new API.Docs still show mutate taking id; the hook now accepts id at initialization and mutate/mutateAsync take body/options only.
Apply:
@@ - * @param id - The ID of the record to update + * @param id - The ID of the record to update @@ - * const { mutate, isPending, isSuccess, error } = useUpdateMutation<Post>('posts'); + * const { mutateAsync, isPending, isSuccess, error } = useUpdateMutation<Post>('posts', 'record_id'); @@ - * const updated = await mutate('record_id', { title: 'Updated Title' }); + * const updated = await mutateAsync({ title: 'Updated Title' });src/hooks/useDeleteMutation.ts (1)
13-24: Update JSDoc example for the new id-based API.Example still shows mutate taking id; switch to id at hook init and mutateAsync() with options.
@@ - * const { mutate, isPending, isSuccess, error } = useDeleteMutation('posts'); + * const { mutateAsync, isPending, isSuccess, error } = useDeleteMutation('posts', 'record_id'); @@ - * const success = await mutate('record_id'); - * if (success) { - * console.log('Deleted successfully'); - * } + * await mutateAsync(); + * console.log('Deleted successfully');Also applies to: 25-26
♻️ Duplicate comments (2)
examples/useDeleteMutation-example.tsx (1)
21-35: Race condition: shared isPending state across id changes.This issue was previously flagged and remains unresolved. When
deletingIdchanges (line 26), the hook reinitializes with a newid, but the previous mutation'sfinallyblock (line 33) will still execute and setdeletingIdtonull. This creates multiple problems:
Concurrent deletions: If user rapidly clicks delete on posts A then B:
- Post A's
finallyexecutes →setDeletingId(null)- Hook reinitializes with
id=null- Post B's mutation still running but
isPendingfrom the new hook instance may be incorrectPremature null assignment: The
finallyblock always setsdeletingId(null)even though another deletion might be queued or in progress.Hook reinitialization overhead: Each
deletingIdchange triggers a complete hook reinitialization, discarding the previous hook's state.Recommended fix: Track pending state per post ID rather than using a single shared state:
-const [deletingId, setDeletingId] = useState<string | null>(null); -const { mutateAsync: deletePost, isPending, isSuccess, isError, error } = useDeleteMutation('posts', deletingId); +const [deletingIds, setDeletingIds] = useState<Set<string>>(new Set()); +const { mutateAsync: deletePost, isError, error } = useDeleteMutation('posts', null); const handleDelete = async (postId: string) => { - if (isPending) { + if (deletingIds.has(postId)) { return; } - setDeletingId(postId); + setDeletingIds(prev => new Set(prev).add(postId)); try { - await deletePost(); + // Pass id to a version of the hook that accepts it at mutation time, + // or refactor to use a single hook instance that accepts id in mutateAsync console.log('Post deleted successfully'); } catch (err) { console.error('Failed to delete post:', err); } finally { - setDeletingId(null); + setDeletingIds(prev => { + const next = new Set(prev); + next.delete(postId); + return next; + }); } };Alternative: If the hook API requires
idat initialization and cannot accept it at mutation time, consider using separate hook instances per deletion or implementing abort logic whenidchanges.src/hooks/useDeleteMutation.ts (1)
52-59: Remove the inline comment in the empty catch.Matches a prior review; keep the silent catch but drop the comment.
- mutateAsync(options).catch(() => { - // Error is already handled in mutateAsync - }); + mutateAsync(options).catch(() => {});As per coding guidelines.
🧹 Nitpick comments (6)
src/hooks/useCreateMutation.ts (1)
33-49: Simplify options handling.Line 38 uses an unnecessary conditional to pass the
optionsparameter. Since PocketBase'screate()method already acceptsoptionsas an optional parameter, you can simplify by always passing it:-const record = options ? await recordService.create(bodyParams, options) : await recordService.create(bodyParams); +const record = await recordService.create(bodyParams, options);The error handling (lines 40-43) correctly converts non-Error exceptions and re-throws after updating state, which is appropriate for the
mutateAsyncpattern.tests/hooks/useCreateMutation.test.tsx (1)
25-28: Good coverage; add two tiny assertions and one follow-up test.
- While pending, also assert error === null and isError === false to lock the contract.
- After an error, add a subsequent-success test to ensure error/isError reset.
- Optionally type mocks as vi.Mock for better IntelliSense.
Apply examples:
@@ await waitFor(() => { - expect(result.current.isPending).toBe(true); - expect(result.current.isSuccess).toBe(false); + expect(result.current.isPending).toBe(true); + expect(result.current.isSuccess).toBe(false); + expect(result.current.isError).toBe(false); + expect(result.current.error).toBeNull(); });New follow-up test (mutateAsync block):
@@ describe('mutateAsync', () => { + it('should clear error on subsequent success', async () => { + const wrapper = createWrapper(mockPocketBase); + const { result } = renderHook(() => useCreateMutation('test'), { wrapper }); + mockCreate.mockRejectedValueOnce(new Error('boom')); + await expect(result.current.mutateAsync({ title: 'A' })).rejects.toThrow('boom'); + await waitFor(() => { + expect(result.current.isError).toBe(true); + expect(result.current.error).toBe('boom'); + }); + const ok = { id: '2', title: 'B' }; + mockCreate.mockResolvedValueOnce(ok); + const out = await result.current.mutateAsync({ title: 'B' }); + expect(out).toEqual(ok); + await waitFor(() => { + expect(result.current.isPending).toBe(false); + expect(result.current.isSuccess).toBe(true); + expect(result.current.isError).toBe(false); + expect(result.current.error).toBeNull(); + }); + });And (optional) type the mock:
- let mockCreate: any; + import type { Mock } from 'vitest'; + let mockCreate: Mock;Based on learnings.
Also applies to: 30-34, 36-54, 55-68, 69-85, 87-103, 104-129, 132-149, 151-167, 169-185, 187-203
src/hooks/useUpdateMutation.ts (2)
32-52: Preserve original error stack and simplify the update call.
- Keep the original Error when possible to retain stack context.
- You can call update with options as undefined; no need for the ternary.
const mutateAsync = useCallback( async (bodyParams: Partial<Record>, options?: RecordOptions): Promise<Record> => { if (!id) { throw new Error('ID is required'); } try { setIsPending(true); setError(null); - const record = options ? await recordService.update(id, bodyParams, options) : await recordService.update(id, bodyParams); + const record = await recordService.update(id, bodyParams, options); return record as Record; } catch (err) { - const errorMessage = err instanceof Error ? err.message : 'Error updating record'; - setError(errorMessage); - throw new Error(errorMessage); + const errorMessage = err instanceof Error ? err.message : 'Error updating record'; + setError(errorMessage); + if (err instanceof Error) throw err; + throw new Error(errorMessage); } finally { setIsPending(false); } }, [recordService, id], );
63-73: Optionally compute isSuccess from isError for clarity.Not required, but improves readability and avoids repeating the negation logic.
- (): UseUpdateMutationResult<Record> => ({ + (): UseUpdateMutationResult<Record> => ({ mutate, mutateAsync, isPending, - isError: !!error, - error, - isSuccess: !isPending && !error, + isError: !!error, + error, + isSuccess: !isPending && !Boolean(error), }),tests/hooks/useDeleteMutation.test.tsx (1)
20-28: Tests look solid; add mutate(null-id) and error-reset checks.
- Add a test for mutate() when id is null to ensure state reflects the error (since mutate swallows throws).
- Add a follow-up success after a failure to verify error/isError reset.
Examples:
@@ it('should handle null id', async () => { const wrapper = createWrapper(mockPocketBase); const { result } = renderHook(() => useDeleteMutation('test', null), { wrapper }); await expect(result.current.mutateAsync()).rejects.toThrow('ID is required'); }); + +it('should surface error state when mutate is called with null id', async () => { + const wrapper = createWrapper(mockPocketBase); + const { result } = renderHook(() => useDeleteMutation('test', null), { wrapper }); + result.current.mutate(); + await waitFor(() => { + expect(result.current.error).toBe('ID is required'); + expect(result.current.isError).toBe(true); + expect(result.current.isSuccess).toBe(false); + }); +});Also optional typing:
- let mockDelete: any; + import type { Mock } from 'vitest'; + let mockDelete: Mock;Based on learnings.
Also applies to: 30-43, 44-71, 72-89, 90-106, 107-133, 135-168, 170-203
src/hooks/useDeleteMutation.ts (1)
31-50: Preserve original error stack when possible.Rethrow the original Error to keep the stack; otherwise construct a new Error.
const mutateAsync = useCallback( async (options?: CommonOptions): Promise<void> => { if (!id) { throw new Error('ID is required'); } @@ - } catch (err) { - const errorMessage = err instanceof Error ? err.message : 'Error deleting record'; - setError(errorMessage); - throw new Error(errorMessage); + } catch (err) { + const errorMessage = err instanceof Error ? err.message : 'Error deleting record'; + setError(errorMessage); + if (err instanceof Error) throw err; + throw new Error(errorMessage); } finally { setIsPending(false); } }, [recordService, id], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
examples/useCreateMutation-example.tsx(2 hunks)examples/useDeleteMutation-example.tsx(2 hunks)examples/useUpdateMutation-example.tsx(4 hunks)src/hooks/useCreateMutation.ts(2 hunks)src/hooks/useDeleteMutation.ts(3 hunks)src/hooks/useUpdateMutation.ts(2 hunks)src/types/index.ts(1 hunks)src/types/useCollection.type.ts(1 hunks)src/types/useCreateMutation.type.ts(1 hunks)src/types/useDeleteMutation.type.ts(1 hunks)src/types/useMutationCommon.type.ts(1 hunks)src/types/useQueryCommon.type.ts(1 hunks)src/types/useRecord.type.ts(1 hunks)src/types/useUpdateMutation.type.ts(1 hunks)tests/hooks/useCreateMutation.test.tsx(1 hunks)tests/hooks/useDeleteMutation.test.tsx(1 hunks)tests/hooks/useUpdateMutation.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
Applied to files:
src/types/useCreateMutation.type.tstests/hooks/useCreateMutation.test.tsxsrc/types/useDeleteMutation.type.tsexamples/useDeleteMutation-example.tsxsrc/hooks/useDeleteMutation.tstests/hooks/useUpdateMutation.test.tsxsrc/hooks/useCreateMutation.tsexamples/useUpdateMutation-example.tsxsrc/types/useMutationCommon.type.tsexamples/useCreateMutation-example.tsxsrc/hooks/useUpdateMutation.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/**/*.{ts,tsx} : Provide generic type parameters for records in hooks, e.g., useCollection<Post>() and useAuth<User>()
Applied to files:
src/types/useRecord.type.tssrc/types/useCollection.type.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Use renderHook from testing-library/react and wrap hooks with PocketBaseProvider using a mocked client
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useUpdateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsx
📚 Learning: 2025-10-18T12:22:42.057Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: .cursorrules:0-0
Timestamp: 2025-10-18T12:22:42.057Z
Learning: Applies to tests/**/*.{ts,tsx} : Mock PocketBase methods comprehensively in tests
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useUpdateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Mock PocketBase client methods with Vitest vi.fn() and test loading, success, and error states
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useUpdateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : useCollection and useRecord must support a transformers option (array of RecordTransformer<T>) applied sequentially (reduce) to initial fetch and real-time updates; default include dateTransformer()
Applied to files:
src/types/useQueryCommon.type.tssrc/types/useCollection.type.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : Support request cancellation via the requestKey option in useCollection and useRecord
Applied to files:
src/types/useCollection.type.ts
🔇 Additional comments (19)
src/types/useQueryCommon.type.ts (1)
4-19: LGTM! Improved naming clarity.The rename from
UseCommonOptionstoUseQueryCommonOptionsbetter reflects the purpose of this interface for query-related hooks. The separation of query and mutation common options improves type organization.src/types/useRecord.type.ts (1)
3-10: LGTM! Consistent with type refactoring.The update to use
UseQueryCommonOptionsaligns with the broader refactoring and maintains consistency across query-related hooks.src/types/useCreateMutation.type.ts (2)
24-24: LGTM! Promise-based async mutation added.The addition of
mutateAsyncprovides a promise-based API for users who need to await the mutation result. This complements the fire-and-forgetmutatefunction.
9-16: Breaking change verification complete—no issues found.The codebase correctly implements the pattern where
mutate()returnsvoid(fire-and-forget) whilemutateAsync()returns aPromise<Record>for async operations. All three pattern searches confirmed no code awaitsmutate(), assigns its return value, or chains.then()on it. The test suite validates both methods exist, and the example demonstrates proper usage ofmutateAsync()for Promise-based workflows. The implementation follows standard React Query conventions.examples/useCreateMutation-example.tsx (2)
25-25: LGTM! Good input validation.The trim check prevents blank submissions and improves UX.
28-36: LGTM! Proper async mutation handling.The use of
mutateAsyncwith try-catch provides good error handling, and resetting all form fields (including status) after successful creation improves UX.src/types/useMutationCommon.type.ts (1)
1-21: LGTM! Well-structured common mutation state.The interface properly captures the standard mutation state flags (isPending, isSuccess, isError, error) and aligns with established patterns. Clear documentation for each property enhances usability.
Based on learnings.
examples/useUpdateMutation-example.tsx (2)
29-33: LGTM! Simplified update payload.The updated API where the id is passed to the hook initialization (line 19) and the mutation receives only the data payload is cleaner and aligns with the PR objective.
19-19: Hook appropriately handles null id with explicit error.Verification complete. The
useUpdateMutationhook accepts null id and explicitly checks for it, throwing an error "ID is required" whenmutateAsyncis called with null id. The example defensively checks!editingIdbefore submission, preventing the error case. Behavior is documented and appropriate.src/types/useCollection.type.ts (1)
3-10: LGTM! Consistent with type refactoring.The update to use
UseQueryCommonOptionsmaintains consistency across all query-related hooks (useCollection, useRecord).src/types/index.ts (1)
8-8: Export migration verified and complete.The export change from
'./useCommon.type'to'./useQueryCommon.type'has been properly implemented:
- Old file removed
- New file exists with correct export
- All internal references (useRecord.type.ts, useCollection.type.ts) updated to import from new location
- No broken imports or dangling references remain
src/types/useUpdateMutation.type.ts (1)
1-25: LGTM! Type definitions align with the new initialization-time id API.The type definitions correctly reflect the new API pattern where the record id is provided at hook initialization rather than at mutation time. The separation of
mutate(synchronous trigger) andmutateAsync(returns Promise) is well-documented and consistent with the UseMutationCommonOptions pattern.src/types/useDeleteMutation.type.ts (1)
1-21: LGTM! Type definitions consistent with the mutation API pattern.The type definitions follow the same pattern as
UseUpdateMutationResult, with the id provided at initialization andmutate/mutateAsyncmethods that don't require the id parameter. The documentation clearly distinguishes synchronous and asynchronous behaviors.tests/hooks/useUpdateMutation.test.tsx (1)
1-215: LGTM! Comprehensive test coverage for the new mutation API.The test suite thoroughly covers:
- Initial state validation including the new
isErrorflag- Provider context requirement
- Null id handling with appropriate error
- Both
mutateandmutateAsynccode paths- Success scenarios with and without options
- Error handling for both
Errorinstances and non-Error exceptions- Pending state transitions during async operations
The tests align well with the updated API surface and provide strong confidence in the hook's behavior.
Based on learnings
examples/useDeleteMutation-example.tsx (1)
63-63: Verify button state with per-post pending logic.The button currently uses a global
isPendingcheck (lines 63, 72), which becomes incorrect with the race condition described above. If you implement per-post pending tracking, update these checks to use the per-post state:-disabled={isPending} +disabled={deletingIds.has(post.id)}-{isPending ? 'Deleting...' : 'Delete'} +{deletingIds.has(post.id) ? 'Deleting...' : 'Delete'}Also applies to: 72-72
src/hooks/useCreateMutation.ts (2)
51-58: LGTM! The synchronous mutate wrapper correctly delegates to mutateAsync.The
mutatefunction appropriately:
- Delegates to
mutateAsyncfor the actual mutation logic- Suppresses the unhandled promise rejection since
mutateAsyncalready updates the error state- Provides a fire-and-forget API as documented
This pattern aligns with the mutation hook requirements and maintains a clean separation between synchronous and asynchronous mutation triggers.
Based on learnings
60-70: LGTM! Return object correctly exposes the mutation API.The memoized return value properly includes:
- Both
mutateandmutateAsyncfor different usage patterns- State flags
isPending,isSuccess,isErroras requirederrorfor error details- Correct dependency array including both mutation functions
Based on learnings
src/hooks/useDeleteMutation.ts (1)
61-71: Return shape looks consistent (isError included).No changes needed; matches project conventions.
Based on learnings.
src/hooks/useUpdateMutation.ts (1)
26-26: Test coverage for null id path already exists—no action needed.The requested test for
useUpdateMutationwith a null id is already implemented at lines 40–42 oftests/hooks/useUpdateMutation.test.tsx, and it mirrors theuseDeleteMutationpattern exactly as suggested in the review comment.
KevinBonnoron
force-pushed
the
8-useupdatemutation-usedeletemutation-should-include-an-id
branch
from
eeaf31b to
133ab95
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/hooks/useDeleteMutation.ts (1)
52-59: Remove inline comment per coding guidelines.The inline comment violates the "no code comments" guideline and was previously flagged. Keep the silent catch but remove the comment.
const mutate = useCallback( (options?: CommonOptions): void => { - mutateAsync(options).catch(() => { - // Error is already handled in mutateAsync - }); + mutateAsync(options).catch(() => {}); }, [mutateAsync], );As per coding guidelines.
🧹 Nitpick comments (8)
src/types/useDeleteMutation.type.ts (1)
9-14: Clarify id flow in JSDoc.Since id is now provided at hook initialization (not to mutate), add a note in this JSDoc to prevent misuse.
src/types/useCreateMutation.type.ts (1)
9-25: Avoid shadowing TSRecord; rename generic toTRecord.Improves readability and avoids confusion with
Record<…, …>.-export interface UseCreateMutationResult<Record extends RecordModel> extends UseMutationCommonOptions { +export interface UseCreateMutationResult<TRecord extends RecordModel> extends UseMutationCommonOptions { - mutate: (bodyParams: Partial<Record>, options?: RecordOptions) => void; + mutate: (bodyParams: Partial<TRecord>, options?: RecordOptions) => void; - mutateAsync: (bodyParams: Partial<Record>, options?: RecordOptions) => Promise<Record>; + mutateAsync: (bodyParams: Partial<TRecord>, options?: RecordOptions) => Promise<TRecord>; }src/types/useUpdateMutation.type.ts (1)
9-25: Rename generic toTRecordfor consistency and clarity.Mirror the create type to avoid
Recordshadowing.-export interface UseUpdateMutationResult<Record extends RecordModel> extends UseMutationCommonOptions { +export interface UseUpdateMutationResult<TRecord extends RecordModel> extends UseMutationCommonOptions { - mutate: (bodyParams: Partial<Record>, options?: RecordOptions) => void; + mutate: (bodyParams: Partial<TRecord>, options?: RecordOptions) => void; - mutateAsync: (bodyParams: Partial<Record>, options?: RecordOptions) => Promise<Record>; + mutateAsync: (bodyParams: Partial<TRecord>, options?: RecordOptions) => Promise<TRecord>; }examples/useUpdateMutation-example.tsx (3)
28-38: Reset status after successful update to avoid stale UI.console.log('Post updated:', updatedPost); setEditingId(null); setEditTitle(''); setEditContent(''); + setEditStatus('draft');
50-54: Reset status on cancel for consistency.const cancelEdit = () => { setEditingId(null); setEditTitle(''); setEditContent(''); + setEditStatus('draft'); };
78-80: Prevent accidental actions during in‑flight update.Disable Cancel while pending.
- <button type="button" onClick={cancelEdit}> + <button type="button" onClick={cancelEdit} disabled={isPending}> Cancel </button>tests/hooks/useCreateMutation.test.tsx (1)
3-15: Type mocks asvi.Mockinstead ofany.Improves type safety and removes the need for lint ignores elsewhere.
-import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; ... - let mockCreate: any; + let mockCreate: vi.Mock;tests/hooks/useUpdateMutation.test.tsx (1)
3-11: Replaceany+ biome ignore with proper mock typing.Use
vi.Mockand drop the suppressive comment.-import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; -// biome-ignore lint/suspicious/noExplicitAny: Mock function type -let mockUpdate: any; +let mockUpdate: vi.Mock;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
examples/useCreateMutation-example.tsx(2 hunks)examples/useDeleteMutation-example.tsx(2 hunks)examples/useUpdateMutation-example.tsx(3 hunks)src/hooks/useCreateMutation.ts(2 hunks)src/hooks/useDeleteMutation.ts(3 hunks)src/hooks/useUpdateMutation.ts(2 hunks)src/types/index.ts(1 hunks)src/types/useCollection.type.ts(1 hunks)src/types/useCreateMutation.type.ts(1 hunks)src/types/useDeleteMutation.type.ts(1 hunks)src/types/useMutationCommon.type.ts(1 hunks)src/types/useQueryCommon.type.ts(1 hunks)src/types/useRecord.type.ts(1 hunks)src/types/useUpdateMutation.type.ts(1 hunks)tests/hooks/useCollection.test.tsx(1 hunks)tests/hooks/useCreateMutation.test.tsx(1 hunks)tests/hooks/useDeleteMutation.test.tsx(1 hunks)tests/hooks/useRecord.test.tsx(2 hunks)tests/hooks/useUpdateMutation.test.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : useCollection and useRecord must support a transformers option (array of RecordTransformer<T>) applied sequentially (reduce) to initial fetch and real-time updates; default include dateTransformer()
Applied to files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsxsrc/types/useQueryCommon.type.tssrc/types/useCollection.type.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/lib/utils.ts : applyTransformers() must catch transformer errors, log to console, and return the original record to avoid breaking flow
Applied to files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : By default, apply dateTransformer() to convert created/updated fields to Date objects; allow custom transformers or none via transformers: []
Applied to files:
tests/hooks/useRecord.test.tsxtests/hooks/useCollection.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCreateMutation,useUpdateMutation,useDeleteMutation}.{ts,tsx} : Mutation hooks must expose mutate() and state flags isPending, isSuccess, isError, error
Applied to files:
tests/hooks/useCreateMutation.test.tsxsrc/types/useDeleteMutation.type.tssrc/hooks/useCreateMutation.tssrc/types/useMutationCommon.type.tssrc/hooks/useDeleteMutation.tsexamples/useCreateMutation-example.tsxtests/hooks/useUpdateMutation.test.tsxsrc/hooks/useUpdateMutation.tssrc/types/useCreateMutation.type.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Use renderHook from testing-library/react and wrap hooks with PocketBaseProvider using a mocked client
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:42.057Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: .cursorrules:0-0
Timestamp: 2025-10-18T12:22:42.057Z
Learning: Applies to tests/**/*.{ts,tsx} : Mock PocketBase methods comprehensively in tests
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to tests/hooks/**/*.{test,spec}.{ts,tsx} : Mock PocketBase client methods with Vitest vi.fn() and test loading, success, and error states
Applied to files:
tests/hooks/useCreateMutation.test.tsxtests/hooks/useDeleteMutation.test.tsxtests/hooks/useUpdateMutation.test.tsx
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/**/*.{ts,tsx} : Provide generic type parameters for records in hooks, e.g., useCollection<Post>() and useAuth<User>()
Applied to files:
src/types/useRecord.type.tssrc/types/useCollection.type.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/types/index.ts : All exported types must be explicitly defined in src/types/index.ts and referenced by hooks and utilities
Applied to files:
src/types/index.ts
📚 Learning: 2025-10-18T12:22:11.405Z
Learnt from: CR
PR: KevinBonnoron/pocketbase-react-hooks#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-18T12:22:11.405Z
Learning: Applies to src/hooks/{useCollection,useRecord}.{ts,tsx} : Support request cancellation via the requestKey option in useCollection and useRecord
Applied to files:
src/types/useCollection.type.ts
🔇 Additional comments (14)
tests/hooks/useCollection.test.tsx (1)
741-741: LGTM! Good code hygiene.Prefixing the unused parameter with an underscore is a standard convention to indicate the parameter is intentionally unused, since the transformer immediately throws an error.
tests/hooks/useRecord.test.tsx (1)
427-427: LGTM! Good stylistic improvements.Both changes improve code quality:
- Prefixing unused parameter with underscore follows convention
- Template literal is more idiomatic than string concatenation
Also applies to: 480-480
src/types/useQueryCommon.type.ts (1)
4-4: LGTM! Better semantic naming.Renaming
UseCommonOptionstoUseQueryCommonOptionsimproves clarity by distinguishing query-specific options from mutation options, especially now thatUseMutationCommonOptionshas been introduced.src/types/useRecord.type.ts (1)
3-3: LGTM! Consistent with type rename.The import and extension changes correctly follow the rename from
UseCommonOptionstoUseQueryCommonOptions.Also applies to: 10-10
src/types/useCollection.type.ts (1)
3-3: LGTM! Consistent with type rename.The import and extension changes correctly follow the rename from
UseCommonOptionstoUseQueryCommonOptions.Also applies to: 10-10
examples/useCreateMutation-example.tsx (3)
18-18: LGTM! Improved mutation API and validation.Good improvements:
- Using
mutateAsyncaligns with the new Promise-based mutation API- Adding
isErrorto the destructured values provides explicit error state tracking- Input validation with
trim()prevents empty submissions- Resetting
statusto'draft'after creation maintains consistent UXAlso applies to: 25-26, 29-30, 36-36
44-44: Verify the heading hierarchy change is intentional.The headings have been swapped:
- Line 44 (outer): "Create Post" → "Create New Post"
- Line 47 (form): "Create New Post" → "Create Post"
This reverses the hierarchy. Verify this is intentional and not an accidental swap during refactoring.
Also applies to: 47-47
64-64: LGTM! Improved error handling.Moving error display to the form context (with
isError && errorguard) and removing the early return addresses the previous review feedback about redundant error handling. This provides better UX by showing errors inline within the form.src/types/useMutationCommon.type.ts (1)
1-21: LGTM! Good abstraction for mutation state.This interface provides a clean, reusable base for mutation result types. The properties align with the learning that mutation hooks must expose state flags
isPending,isSuccess,isError, anderror. Well-documented with clear JSDoc comments.Based on learnings.
tests/hooks/useDeleteMutation.test.tsx (4)
17-34: LGTM: Initial state and provider tests are comprehensive.Tests correctly verify:
- All returned fields including new
isErrorflag- Both
mutateandmutateAsyncfunction presence- Provider requirement with appropriate error message
36-42: LGTM: Null ID validation test is correct.Properly validates that
mutateAsyncthrows "ID is required" whenidis null, covering an important edge case.
44-133: LGTM: ComprehensivemutateAsynctest coverage.Tests thoroughly cover:
- Success path with and without options
- Error handling (Error instances and non-Error values)
- State transitions (
isPending,isSuccess,isError)- Proper promise resolution/rejection
135-204: LGTM:mutatewrapper tests are thorough.Tests correctly verify:
- Fire-and-forget behavior (returns
undefined)- Delegation to
mutateAsyncwith proper arguments- Error handling without throwing
- State updates via
waitForsrc/hooks/useDeleteMutation.ts (1)
61-71: LGTM: Return object now includes requiredisErrorflag.The previously missing
isErrorfield has been properly added and derived from the error state. The return object now correctly exposes all mutation state per project conventions.Based on learnings.
| const [deletingId, setDeletingId] = useState<string | null>(null); | ||
| const { mutateAsync: deletePost, isPending, isSuccess, isError, error } = useDeleteMutation('posts', deletingId); | ||
|
|
||
| const handleDelete = async (id: string) => { | ||
| if (window.confirm('Are you sure you want to delete this post?')) { | ||
| setDeletingId(id); | ||
| try { | ||
| const success = await deletePost(id); | ||
| if (success) { | ||
| console.log('Post deleted successfully'); | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to delete post:', err); | ||
| } finally { | ||
| setDeletingId(null); | ||
| } | ||
| const handleDelete = async (postId: string) => { | ||
| if (isPending) { | ||
| return; | ||
| } | ||
|
|
||
| setDeletingId(postId); | ||
| try { | ||
| await deletePost(); | ||
| console.log('Post deleted successfully'); | ||
| } catch (err) { | ||
| console.error('Failed to delete post:', err); | ||
| } finally { | ||
| setDeletingId(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical: Closure bug causes "ID is required" error on every delete.
The delete flow is broken due to closure capture timing:
- Initial render:
deletingId=null→ hook initialized withid=null→deletePostwill throw - User clicks delete →
handleDelete('A')captures currentdeletePost(withid=null) - Line 26:
setDeletingId('A')queues state update but doesn't take effect yet - Line 28:
await deletePost()calls the captured function → throws "ID is required" because it usesid=null
The state update happens after the function reference is captured, so deletePost() always uses id=null.
Solution: Move the ID into the function call rather than hook initialization:
- const [deletingId, setDeletingId] = useState<string | null>(null);
- const { mutateAsync: deletePost, isPending, isSuccess, isError, error } = useDeleteMutation('posts', deletingId);
+ const { mutateAsync: deletePost, isPending, isSuccess, isError, error } = useDeleteMutation('posts', null);
const handleDelete = async (postId: string) => {
if (isPending) {
return;
}
- setDeletingId(postId);
try {
- await deletePost();
+ await deletePost(postId);
console.log('Post deleted successfully');
} catch (err) {
console.error('Failed to delete post:', err);
- } finally {
- setDeletingId(null);
}
};However, this requires updating the hook API to accept the ID as an argument to mutateAsync instead of at initialization. Alternatively, redesign the hook to not require pre-initialization with an ID.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In examples/useDeleteMutation-example.tsx around lines 18-35, the delete handler
captures a stale deletePost closure because the hook was initialized with
deletingId=null; change the pattern so the ID is passed when calling mutateAsync
rather than at hook initialization: update the example to call the mutation with
the id (e.g., await deletePost(postId)) and keep deletingId state only for
UI/loading purposes; then update the useDeleteMutation hook signature and types
to remove the required id parameter on init and accept the id argument in
mutateAsync (adjust internal logic to read the id from the mutate call), and
update all other call sites/tests to the new API accordingly.
| const mutate = useCallback( | ||
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | ||
| mutateAsync(bodyParams, options).catch(() => { | ||
| // Error is already handled in mutateAsync | ||
| }); | ||
| }, | ||
| [mutateAsync], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove inline comment per coding guidelines.
The inline comment violates the "no code comments" guideline. Keep the silent catch but remove the comment.
const mutate = useCallback(
(bodyParams: Partial<Record>, options?: RecordOptions): void => {
- mutateAsync(bodyParams, options).catch(() => {
- // Error is already handled in mutateAsync
- });
+ mutateAsync(bodyParams, options).catch(() => {});
},
[mutateAsync],
);As per coding guidelines.
📝 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.
| const mutate = useCallback( | |
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | |
| mutateAsync(bodyParams, options).catch(() => { | |
| // Error is already handled in mutateAsync | |
| }); | |
| }, | |
| [mutateAsync], | |
| ); | |
| const mutate = useCallback( | |
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | |
| mutateAsync(bodyParams, options).catch(() => {}); | |
| }, | |
| [mutateAsync], | |
| ); |
🤖 Prompt for AI Agents
In src/hooks/useCreateMutation.ts around lines 51 to 58, remove the inline
comment inside the catch block that reads "// Error is already handled in
mutateAsync" while preserving the silent catch behavior; simply leave the catch
callback body empty (or use an empty statement) so the promise rejection is
swallowed without any inline comments, adhering to the "no code comments"
guideline.
| const mutate = useCallback( | ||
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | ||
| mutateAsync(bodyParams, options).catch(() => { | ||
| // Error is already handled in mutateAsync | ||
| }); | ||
| }, | ||
| [mutateAsync], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove inline comment per coding guidelines.
The inline comment violates the "no code comments" guideline. Keep the silent catch but remove the comment.
const mutate = useCallback(
(bodyParams: Partial<Record>, options?: RecordOptions): void => {
- mutateAsync(bodyParams, options).catch(() => {
- // Error is already handled in mutateAsync
- });
+ mutateAsync(bodyParams, options).catch(() => {});
},
[mutateAsync],
);As per coding guidelines.
📝 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.
| const mutate = useCallback( | |
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | |
| mutateAsync(bodyParams, options).catch(() => { | |
| // Error is already handled in mutateAsync | |
| }); | |
| }, | |
| [mutateAsync], | |
| ); | |
| const mutate = useCallback( | |
| (bodyParams: Partial<Record>, options?: RecordOptions): void => { | |
| mutateAsync(bodyParams, options).catch(() => {}); | |
| }, | |
| [mutateAsync], | |
| ); |
🤖 Prompt for AI Agents
In src/hooks/useUpdateMutation.ts around lines 54 to 61 there is an inline
comment inside the catch block; remove that comment and leave the silent catch
body intact (i.e., keep .catch(() => { }) with an empty block), then reformat to
satisfy linting/formatting rules.
| export * from './useCommon.type'; | ||
| export * from './useCreateMutation.type'; | ||
| export * from './useDeleteMutation.type'; | ||
| export * from './useQueryCommon.type'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the missing export for UseMutationCommonOptions.
The rename from useCommon.type to useQueryCommon.type is consistent with the interface rename. However, the new UseMutationCommonOptions interface introduced in src/types/useMutationCommon.type.ts is not exported from this index file.
According to the codebase learnings, all exported types must be explicitly defined in src/types/index.ts. Verify whether other mutation type files import UseMutationCommonOptions directly or expect it to be re-exported from here.
Based on learnings.
Run the following script to check imports of UseMutationCommonOptions:
🏁 Script executed:
#!/bin/bash
# Check if UseMutationCommonOptions is imported anywhere and whether the export is needed
echo "=== Checking imports of UseMutationCommonOptions ==="
rg -n "import.*UseMutationCommonOptions" --type=ts --type=tsx
echo -e "\n=== Checking if useMutationCommon.type is referenced ==="
rg -n "from.*useMutationCommon" --type=ts --type=tsxLength of output: 166
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check if UseMutationCommonOptions is imported anywhere and whether the export is needed
echo "=== Checking imports of UseMutationCommonOptions ==="
rg -n "import.*UseMutationCommonOptions" --type=ts
echo -e "\n=== Checking if useMutationCommon.type is referenced ==="
rg -n "from.*useMutationCommon" --type=ts
echo -e "\n=== Checking current exports in src/types/index.ts ==="
cat src/types/index.ts
echo -e "\n=== Checking if useMutationCommon.type file exists ==="
fd "useMutationCommon" src/types/Length of output: 1317
Add missing export for useMutationCommon.type in src/types/index.ts.
The verification confirms the issue: UseMutationCommonOptions is imported directly by three mutation type files (useCreateMutation.type.ts, useDeleteMutation.type.ts, useUpdateMutation.type.ts), and all three of these files are exported from the index. However, src/types/index.ts does not export useMutationCommon.type itself, breaking the consistent export pattern where all type files are re-exported from the index.
Add export * from './useMutationCommon.type'; to src/types/index.ts to make UseMutationCommonOptions accessible to external consumers through the index file.
🤖 Prompt for AI Agents
In src/types/index.ts around line 8, the file currently exports
useQueryCommon.type but is missing an export for useMutationCommon.type; add the
missing re-export by adding export * from './useMutationCommon.type'; so
UseMutationCommonOptions and related types are available through the index
alongside the other type files.
| export interface UseDeleteMutationResult extends UseMutationCommonOptions { | ||
| /** | ||
| * Function to delete a record. Returns true on success, false on error. | ||
| * Function to delete a record synchronously. Triggers the mutation but doesn't wait for completion. | ||
| * | ||
| * @param id - The ID of the record to delete | ||
| * @param options - Optional PocketBase common options (headers, fetch, etc.) | ||
| */ | ||
| mutate: (id: string, options?: CommonOptions) => Promise<boolean>; | ||
| mutate: (options?: CommonOptions) => void; | ||
|
|
||
| /** | ||
| * True when the mutation is in progress | ||
| */ | ||
| isPending: boolean; | ||
|
|
||
| /** | ||
| * True when the mutation completed successfully (not pending and no error) | ||
| */ | ||
| isSuccess: boolean; | ||
|
|
||
| /** | ||
| * Error message if the mutation failed, null otherwise | ||
| * Function to delete a record asynchronously. Returns a promise that resolves when deletion is complete. | ||
| * | ||
| * @param options - Optional PocketBase common options (headers, fetch, etc.) | ||
| */ | ||
| error: string | null; | ||
| mutateAsync: (options?: CommonOptions) => Promise<void>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm delete return type; align mutateAsync accordingly.
PocketBase’s delete traditionally returns no body (often boolean/void). Your mutateAsync returns Promise<void>. Please verify SDK behavior and adjust if it actually returns boolean to prevent downstream typing drift.
🌐 Web query:
In PocketBase 0.26.x JS SDK, what is the return type of collection.delete(id, options)?
💡 Result:
The method returns a Promise. [1][2]
Sources:
[1] Type definitions (pocketbase-js) showing RecordService.delete(...) : Promise.
[2] Third‑party TypeScript definitions for pocketbase JS SDK v0.26.x showing delete(...) : Promise.
Update mutateAsync return type to Promise<boolean> to match PocketBase SDK behavior.
PocketBase 0.26.x's collection.delete(id, options) returns Promise<boolean>, not void. The current signature returns Promise<void>, which creates a typing mismatch and prevents consumers from accessing the deletion result (whether it succeeded).
Update line 16 from:
mutateAsync: (options?: CommonOptions) => Promise<void>;
to:
mutateAsync: (options?: CommonOptions) => Promise<boolean>;
🤖 Prompt for AI Agents
In src/types/useDeleteMutation.type.ts around lines 7 to 21, the mutateAsync
signature incorrectly returns Promise<void> but PocketBase's collection.delete
returns Promise<boolean>; update the type so mutateAsync returns
Promise<boolean> instead of Promise<void>, ensuring callers can receive the
deletion result and the type matches the SDK.
KevinBonnoron
deleted the
8-useupdatemutation-usedeletemutation-should-include-an-id
branch
1 participant
Summary by CodeRabbit