6 feature add data transformers support#7
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a per-record transformer pipeline (defaulting to a dateTransformer) and an applyTransformers utility; wires transformers into useCollection and useRecord (applied to initial fetch and realtime events), renames the realtime toggle from Changes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
Comment |
![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 (3)
src/lib/utils.ts (1)
7-28: Fix sorting for Date values to work with dateTransformerAfter dateTransformer converts fields to Date, sorting by those fields compares stringified Dates, yielding incorrect order. Handle Date explicitly.
Apply this diff:
return [...records].sort((a, b) => { const aVal = (a as Record<string, unknown>)[field]; const bVal = (b as Record<string, unknown>)[field]; + // Handle Date values + if (aVal instanceof Date && bVal instanceof Date) { + const aTime = aVal.getTime(); + const bTime = bVal.getTime(); + if (aTime < bTime) return direction === 'asc' ? -1 : 1; + if (aTime > bTime) return direction === 'asc' ? 1 : -1; + return 0; + } + if (typeof aVal === 'number' && typeof bVal === 'number') { if (aVal < bVal) return direction === 'asc' ? -1 : 1; if (aVal > bVal) return direction === 'asc' ? 1 : -1; return 0; }src/hooks/useCollection.ts (1)
81-134: Add test for realtime sorting with date fieldsThe concern is valid. The
dateTransformerconverts ISO date strings to Date objects, but sortRecords only has special handling for numbers and strings, falling back toString()conversion for other types. This means when realtime updates apply transformers (creating Date objects) and sorting is triggered, the Date objects get converted viaString(), breaking chronological order.Add test in
tests/hooks/useCollection.test.tsxthat verifies sorting by date field remains correct after realtime create/update events with default dateTransformer applied:it('should maintain correct date order after realtime create event with dateTransformer', async () => { const mockData = [ { id: '1', title: 'First', created: '2024-01-03T10:00:00Z', collectionId: 'test', collectionName: 'test' }, { id: '2', title: 'Second', created: '2024-01-01T10:00:00Z', collectionId: 'test', collectionName: 'test' }, ]; mockGetFullList.mockResolvedValue(mockData); let subscriptionCallback: (event: { action: string; record: RecordModel }) => void; mockSubscribe.mockImplementation((_, callback) => { subscriptionCallback = callback; return Promise.resolve(() => {}); }); const wrapper = createWrapper(mockPocketBase); const { result } = renderHook(() => useCollection('test', { sort: '-created' }), { wrapper }); await act(async () => { await new Promise((resolve) => setTimeout(resolve, 0)); }); // Initial data should be sorted by created descending expect((result.current.data?.[0]?.created as Date).getTime()) .toBeGreaterThan((result.current.data?.[1]?.created as Date).getTime()); // Trigger realtime create with earlier date await act(async () => { subscriptionCallback?.({ action: 'create', record: { id: '3', title: 'Earlier', created: '2024-01-02T10:00:00Z', collectionId: 'test', collectionName: 'test' } }); }); // Order should remain correct after sort expect((result.current.data?.[0]?.created as Date).getTime()) .toBeGreaterThan((result.current.data?.[1]?.created as Date).getTime()); });src/hooks/useRecord.ts (1)
116-121: Avoid re-subscribing on every data change; use functional setter and dropdatafrom deps.Including
queryState.datain the effect deps causes unsubscribe/subscribe churn on each update. Use a functionalsetDatafor the delete path and removequeryState.datafrom the deps.- case 'delete': - if (queryState.data && e.record.id === queryState.data.id) { - queryState.setData(null); - } - break; + case 'delete': + queryState.setData((current) => (current && e.record.id === current.id ? null : current)); + break; @@ - }, [recordService, recordIdOrFilter, expand, isId, realtime, queryState.data, queryState.setData, requestKey]); + }, [recordService, recordIdOrFilter, expand, isId, realtime, queryState.setData, requestKey]);Also applies to: 134-134
🧹 Nitpick comments (14)
src/lib/utils.ts (1)
31-47: Consider per‑transformer error isolation and configurable loggingCurrent behavior aborts the whole pipeline on the first error and logs via console.error. That matches docs but can be noisy and discards prior successful transforms.
- Option A: Isolate failures per transformer (skip only the failing one) or make this selectable (strict vs best‑effort).
- Option B: Accept an optional onTransformerError callback or logger to avoid direct console output in libraries.
tests/hooks/useRecord.test.tsx (1)
279-343: Add a test for disabling default transformersVerify that passing transformers: [] leaves created/updated as strings (default dateTransformer disabled). This guards the intended escape hatch.
Example:
it('disables default transformers when passing empty array', async () => { const mockRecord: RecordModel = { id: '1', title: 't', collectionId: 'x', collectionName: 'x', created: '2024-01-01T00:00:00.000Z', updated: '2024-01-02T00:00:00.000Z', }; mockGetOne.mockResolvedValue(mockRecord); const wrapper = createWrapper(mockPocketBase); const { result } = renderHook(() => useRecord('x', '1', { transformers: [] }), { wrapper }); await act(async () => { await new Promise(r => setTimeout(r, 0)); }); expect(typeof result.current.data?.created).toBe('string'); expect(typeof result.current.data?.updated).toBe('string'); });src/transformers/date.transformer.ts (1)
4-6: Broaden ISO/RFC3339 detection to match PocketBase formatsSupport both “T” and space separators and timezone offsets (e.g., +02:00).
Apply this diff:
-function isISODateString(str: string): boolean { - return /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z?$/.test(str); -} +function isISODateString(str: string): boolean { + // YYYY-MM-DD[T| ]HH:mm:ss(.sss)?(Z|±HH:MM)? + return /^\d{4}-\d{2}-\d{2}[ T]\d{2}:\d{2}:\d{2}(?:\.\d+)?(?:Z|[+-]\d{2}:\d{2})?$/.test(str); +}Consider adding tests for values like "2024-01-01 10:00:00.123Z" and "2024-01-01T10:00:00+02:00".
CLAUDE.md (2)
48-53: Reflect therealtimeoption name in architecture notesReplace mentions of
subscribeoption withrealtimeto match the code.
145-146: Update Important Notes to userealtimeUse the current option name.
Apply this diff:
-- **Real-time Subscriptions**: Enabled by default but can be disabled with `subscribe: false` option +- **Real-time Subscriptions**: Enabled by default but can be disabled with `realtime: false` optionsrc/hooks/useCollection.ts (1)
55-75: Avoid passing page/perPage to getFullListgetFullList ignores pagination; sending page/perPage is unnecessary and may confuse readers.
- Only include page/perPage in getList branch.
- Keep filter/sort/expand/fields/requestKey as-is.
src/hooks/useRecord.ts (4)
47-49: Refactor transformer ref update to an effect (avoid redundant write each render).- const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); - transformers.current = options.transformers ?? [dateTransformer<Record>()]; + const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); + useEffect(() => { + transformers.current = options.transformers ?? [dateTransformer<Record>()]; + }, [options.transformers]);
55-67: Tighten TS inference by passing the generic to PocketBase methods.- return await recordService.getOne(recordIdOrFilter, { + return await recordService.getOne<Record>(recordIdOrFilter, { @@ - return await recordService.getFirstListItem(recordIdOrFilter, { + return await recordService.getFirstListItem<Record>(recordIdOrFilter, {
104-107: Harden unsubscribe cleanup against rejected subscribe promises.- return () => { - unsubscribe.then((unsub) => unsub()); - }; + return () => { + unsubscribe.then((unsub) => unsub()).catch(() => {}); + };(Apply the same change to both subscribe branches.)
Also applies to: 130-133
40-41: Heuristic ID detection.Using
/[=<>~]/works in most cases, but consider an explicit mode or a stricter ID predicate to avoid edge misclassification of unusual IDs.tests/hooks/useCollection.test.tsx (4)
555-627: Realtime tests: solid coverage. Add toggle-to-false unsubscribe case.Consider a test that starts with
realtime: true, then rerenders withrealtime: falseand asserts the returned unsubscribe was called:+ it('should unsubscribe when realtime changes from true to false', async () => { + const mockData = [{ id: '1', title: 'Test', collectionId: 'test', collectionName: 'test' }]; + mockGetFullList.mockResolvedValue(mockData); + const unsubSpy = vi.fn(); + mockSubscribe.mockResolvedValue(unsubSpy); + const wrapper = createWrapper(mockPocketBase); + const { rerender } = renderHook(({ realtime }) => useCollection('test', { realtime }), { + wrapper, + initialProps: { realtime: true }, + }); + await act(async () => { await Promise.resolve(); }); + rerender({ realtime: false }); + await act(async () => { await Promise.resolve(); }); + expect(unsubSpy).toHaveBeenCalled(); + });
794-841: Silence and assert transformer error logging in tests.Capture
console.errorto keep output clean and assert it’s called:- it('should handle transformer errors gracefully', async () => { + it('should handle transformer errors gracefully', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const mockData = [{ id: '1', title: 'Test 1', collectionId: 'test', collectionName: 'test' }]; @@ expect(result.current.isError).toBe(false); @@ expect(result.current.data).toEqual([ { id: '1', title: 'Test 1', collectionId: 'test', collectionName: 'test' }, { id: '2', title: 'Test 2', collectionId: 'test', collectionName: 'test' }, ]); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); });
629-873: Transformers suite: great coverage. Consider a RT-date test and prop-change test.
- Add a case verifying default
dateTransformerapplies to created/updated on real-time events.- Add a case verifying changing
transformersprop after mount affects subsequent RT events (ref-based design should pass). Happy to draft these if useful.
1-1: For Vitest, prefer the/vitestentrypoint of jest-dom.Replace:
- import '@testing-library/jest-dom'
With:
- import '@testing-library/jest-dom/vitest'
As per retrieved learnings for v6.x.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CLAUDE.md(3 hunks)README.md(3 hunks)src/hooks/useCollection.ts(5 hunks)src/hooks/useRecord.ts(6 hunks)src/index.ts(1 hunks)src/lib/utils.ts(2 hunks)src/transformers/date.transformer.ts(1 hunks)src/transformers/index.ts(1 hunks)src/types/index.ts(1 hunks)src/types/record-transformer.type.ts(1 hunks)src/types/useCollection.type.ts(2 hunks)src/types/useCommon.type.ts(1 hunks)src/types/useRecord.type.ts(1 hunks)tests/hooks/useCollection.test.tsx(6 hunks)tests/hooks/useRecord.test.tsx(1 hunks)tests/setup.ts(1 hunks)vite.config.ts(0 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- vite.config.ts
🧰 Additional context used
🧬 Code graph analysis (10)
src/transformers/date.transformer.ts (1)
src/types/record-transformer.type.ts (1)
RecordTransformer(10-10)
src/types/useCollection.type.ts (1)
src/types/useCommon.type.ts (1)
UseCommonOptions(4-19)
src/lib/utils.ts (1)
src/types/record-transformer.type.ts (1)
RecordTransformer(10-10)
src/types/useRecord.type.ts (1)
src/types/useCommon.type.ts (1)
UseCommonOptions(4-19)
src/hooks/useRecord.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-47)
src/types/record-transformer.type.ts (1)
tests/test-utils.tsx (1)
record(86-88)
src/hooks/useCollection.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-47)
tests/hooks/useCollection.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useCollection.ts (1)
useCollection(34-137)
tests/hooks/useRecord.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useRecord.ts (1)
useRecord(35-137)
src/types/useCommon.type.ts (1)
src/types/record-transformer.type.ts (1)
RecordTransformer(10-10)
🔇 Additional comments (15)
src/index.ts (1)
7-7: LGTM! Clean API expansion.The dateTransformer export appropriately extends the public API surface for the new transformer feature.
tests/setup.ts (1)
4-6: Good defensive check.The runtime guard appropriately ensures jsdom is configured before tests run.
src/types/index.ts (1)
3-3: LGTM! Type exports properly extend the public API.The new exports for RecordTransformer and UseCommonOptions align with the transformer feature and follow the existing export pattern.
Also applies to: 6-6
src/transformers/index.ts (1)
1-1: LGTM! Standard barrel export.The re-export properly exposes the date transformer implementation through the transformers module surface.
vitest.config.ts (1)
1-19: Configuration looks good overall.The jsdom environment, globals, and coverage settings are well-configured for the transformer tests.
However, verify that suppressing all stderr output (lines 13-17) won't hide legitimate warnings or errors during test execution. Consider logging and returning false only for specific known noise patterns.
src/types/record-transformer.type.ts (1)
1-10: LGTM! Well-defined transformer type.The RecordTransformer type is correctly defined with appropriate generic constraints and clear documentation. The signature
(record: T) => Tproperly ensures type safety for transformer pipelines.src/types/useRecord.type.ts (2)
10-10: LGTM! Proper generic extension.Updating to extend
UseCommonOptions<T>correctly enables type-safe transformer support for useRecord.
16-19: Good addition and naming consistency.The realtime option aligns with the naming convention established in useCollection and is well-documented.
src/types/useCollection.type.ts (2)
10-10: LGTM! Consistent generic extension.The update to extend
UseCommonOptions<T>enables type-safe transformer support and maintains consistency with useRecord.
49-49: Good naming improvement.Renaming
subscribetorealtimebetter reflects the feature's purpose and maintains consistency across the API.src/types/useCommon.type.ts (1)
4-19: Typed transformers option looks goodGeneric UseCommonOptions and transformers?: RecordTransformer[] are appropriate and align with usage in hooks. No changes needed.
src/hooks/useCollection.ts (1)
45-47: Stable transformers via ref: goodKeeping transformers in a ref avoids unnecessary resubscriptions while allowing runtime updates. Looks correct.
src/hooks/useRecord.ts (3)
36-36: Option rename torealtime: looks good. Please confirm docs and types are updated.
- Verify README and any migration notes mention
subscribe→realtime.
76-80: Good: initial fetch goes through the transformer pipeline.
91-92: Good: RT updates are transformed consistently.Also applies to: 114-115
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
from
080ba3f to
843a1ef
Compare
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/useRecord.ts (1)
84-136: Fix realtime subscription gating and dependencies (missed events + resubscribe churn).
- Current guard requires
queryState.datato exist; with a filter that initially returns no items, you never subscribe, so future matching creates are missed.- Having
queryState.datain the deps causes the effect to unsubscribe/resubscribe on every update, risking dropped events and perf issues.Suggested patch:
- useEffect(() => { - if (!recordIdOrFilter || !queryState.data || !realtime) return; + useEffect(() => { + if (!recordIdOrFilter || !realtime) return; if (isId) { const unsubscribe = recordService.subscribe( recordIdOrFilter, (e) => { switch (e.action) { case 'update': queryState.setData(applyTransformers(e.record as Record, transformers.current)); break; case 'delete': queryState.setData(null); break; } }, { ...(expand && { expand }), ...(requestKey && { requestKey }), }, ); return () => { unsubscribe.then((unsub) => unsub()); }; } else { const unsubscribe = recordService.subscribe( '*', (e) => { switch (e.action) { case 'create': case 'update': queryState.setData(applyTransformers(e.record as Record, transformers.current)); break; case 'delete': if (queryState.data && e.record.id === queryState.data.id) { queryState.setData(null); } break; } }, { filter: recordIdOrFilter, ...(expand && { expand }), ...(requestKey && { requestKey }), }, ); return () => { unsubscribe.then((unsub) => unsub()); }; } - }, [recordService, recordIdOrFilter, expand, isId, realtime, queryState.data, queryState.setData, requestKey]); + }, [recordService, recordIdOrFilter, expand, isId, realtime, requestKey]);This subscribes whenever
realtimeis on andrecordIdOrFilteris set, including the “no initial data yet” case, and prevents resubscribe loops on each data change.src/hooks/useCollection.ts (1)
57-65: Remove unsupportedpageandperPageoptions fromgetFullListcalls.The PocketBase JavaScript SDK's
getFullListmethod does not acceptpageandperPageoptions; it fetches all records in batches using thebatchoption instead. Remove these options from lines 57–65 and 67–75. Supported options includebatch,filter,sort,expand,fields,query, andheaders.
♻️ Duplicate comments (1)
README.md (1)
841-848: Avoid passing defaultedrealtime: truein examples.Default is true; removing it reduces noise. Also confirms the earlier “subscribe→realtime” rename fix is now reflected here.
const { data: posts } = useCollection<Post>('posts', { transformers: [dateTransformer()], - realtime: true // Real-time updates will also be transformed });
🧹 Nitpick comments (7)
tests/hooks/usePocketBase.test.tsx (1)
1-1: Deduplicate jest-dom import (optional).Since
tests/setup.tsimports@testing-library/jest-dom/vitest, you can remove per-file imports to avoid redundancy. Based on learnings-import '@testing-library/jest-dom/vitest';tests/hooks/useRecord.test.tsx (5)
1-1: Deduplicate jest-dom import (optional).
tests/setup.tsalready imports@testing-library/jest-dom/vitest; remove this per-file import. Based on learnings-import '@testing-library/jest-dom/vitest';
295-297: PreferwaitForoversetTimeout(0)to flush async updates.
waitForis clearer and more robust than manual timeouts.Example refactor:
- await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 0)); - }); + await waitFor(() => { + // e.g., expect(result.current.isLoading).toBe(false); + // or any assertion that becomes true after fetch/update + });Remember to import
waitForfrom@testing-library/react.Also applies to: 331-333, 378-380, 432-434, 479-481, 533-535
449-501: Also assert transformer error is logged (optional).To fully cover error handling, spy on
console.errorand assert it’s called when a transformer throws.+const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); ... expect(result.current.data).toEqual(mockRecord); expect(result.current.isError).toBe(false); +expect(errorSpy).toHaveBeenCalled(); +errorSpy.mockRestore();
544-602: Add cleanup test to ensure unsubscribe is called on unmount.Verifies subscription teardown and prevents leaks.
+it('should cleanup subscription on unmount', async () => { + const mockRecord = { id: '1', title: 'Test', collectionId: 'test', collectionName: 'test' }; + mockGetOne.mockResolvedValue(mockRecord); + const unsub = vi.fn(); + mockSubscribe.mockResolvedValue(unsub); + const wrapper = createWrapper(mockPocketBase); + const { unmount } = renderHook(() => useRecord('test', '1'), { wrapper }); + await act(async () => {}); + unmount(); + expect(unsub).toHaveBeenCalledTimes(1); +});
557-560: Loosen subscription options assertion to avoid brittleness (optional).Implementation may add options over time. Prefer
expect.objectContainingfor options.-expect(mockSubscribe).toHaveBeenCalledWith('1', expect.any(Function), { - expand: undefined, -}); +expect(mockSubscribe).toHaveBeenCalledWith( + '1', + expect.any(Function), + expect.objectContaining({ expand: undefined }) +);src/hooks/useCollection.ts (1)
45-47: Sync transformers in an effect (avoid reassigning every render).Minor consistency/clarity nit. Mirror
useRecordand only update whenoptions.transformerschanges.- const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); - transformers.current = options.transformers ?? [dateTransformer<Record>()]; + const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); + useEffect(() => { + transformers.current = options.transformers ?? [dateTransformer<Record>()]; + }, [options.transformers]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CLAUDE.md(3 hunks)README.md(5 hunks)src/hooks/useCollection.ts(5 hunks)src/hooks/useRecord.ts(6 hunks)src/types/useCollection.type.ts(2 hunks)src/types/useRecord.type.ts(1 hunks)tests/context.test.tsx(1 hunks)tests/hooks/useCollection.test.tsx(6 hunks)tests/hooks/usePocketBase.test.tsx(1 hunks)tests/hooks/useRecord.test.tsx(2 hunks)tests/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types/useRecord.type.ts
- src/types/useCollection.type.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/hooks/useRecord.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-47)
tests/hooks/useCollection.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useCollection.ts (1)
useCollection(34-137)
tests/hooks/useRecord.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useRecord.ts (1)
useRecord(35-139)
src/hooks/useCollection.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-47)
🔇 Additional comments (12)
README.md (3)
238-241: API additions look good (realtime + transformers).The options docs align with the new behavior and defaults.
341-341: Clear example for disabling realtime.Using
realtime: falsehere is accurate and consistent.
382-382: Combined options example is consistent.
realtime: enableRealtimematches the new option name.tests/context.test.tsx (1)
1-1: Correct matcher import for Vitest.Using
@testing-library/jest-dom/vitestis the right entrypoint. Based on learningstests/setup.ts (1)
1-6: Setup properly registered in Vitest config.Verification confirms
tests/setup.tsis correctly listed insetupFilesat line 7 ofvitest.config.ts, and the jsdom environment is configured. The setup file is appropriately wired and will execute before tests.src/hooks/useRecord.ts (2)
78-82: Good: apply transformer pipeline on initial fetch.Returning transformed data from fetch keeps first render consistent with live updates.
47-51: Transformers managed via ref: looks good.Ref keeps callbacks fresh without re‑subscribing; consistent with applyTransformers’ try/catch.
src/hooks/useCollection.ts (2)
54-78: Good: transformers applied to fetched items with safe fallback.Mapping through
applyTransformersand defaulting to[]handles null-ish results and ensures consistent shaping.
82-134: Realtime integration looks solid.
- Proper gating on
enabled/realtime.- Create/update go through transformers; unknown update inserts; delete removes.
- Sorting reapplied post‑mutation.
No blocking issues here.
tests/hooks/useCollection.test.tsx (3)
1-1: Correct platform import for jest-dom.Using
@testing-library/jest-dom/vitestis the right entry for Vitest.
555-647: Realtime option coverage looks good (including unsubscribe on toggle).The toggle from true→false asserting the resolved
unsubinvocation matches the hook’s cleanup.
649-896: Transformer tests are comprehensive.Covers default date parsing, custom transformers, error fallback, and sequencing. This aligns with
applyTransformersbehavior.
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
from
843a1ef to
e83b738
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/hooks/useRecord.test.tsx (2)
128-131: Fix subscribe arg expectation: don’t expectexpand: undefined.The hook omits
expandwhen undefined. Expect an empty options object instead.- expect(mockSubscribe).toHaveBeenCalledWith('1', expect.any(Function), { - expand: undefined, - }); + expect(mockSubscribe).toHaveBeenCalledWith('1', expect.any(Function), {});
145-149: Fix filter subscription expectation: assert only the filter key.
expandis omitted when undefined. Assert the filter with a partial matcher.- expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - expand: undefined, - filter: 'slug="test-record"', - }); + expect(mockSubscribe).toHaveBeenCalledWith( + '*', + expect.any(Function), + expect.objectContaining({ filter: 'slug="test-record"' }), + );src/hooks/useCollection.ts (1)
57-66: RemovepageandperPagefromgetFullListcall — these parameters are ignored.The PocketBase JS SDK's
getFullListmethod does not acceptpage/perPageoptions; it accepts other options (filter, sort, expand, fields, etc.) and fetches all results in repeated requests, with per-request batch size controlled via thebatchquery parameter. The code currently passes these ignored parameters, which should be removed as shown in the suggested diff.result = await recordService.getFullList<Record>({ - ...(page && { page }), - ...(perPage && { perPage }), ...(filter && { filter }), ...(sort && { sort }), ...(expand && { expand }), ...(fields && { fields }), ...(requestKey && { requestKey }), });
♻️ Duplicate comments (1)
README.md (1)
632-633: Unify date field naming (published_atvspublishedAt).Earlier guidance favored
published_at. These sections reintroducepublishedAt.- publishedAt?: Date; + published_at?: Date;-interface Post extends RecordModel { +interface Post extends RecordModel { title: string; - publishedAt: Date; + published_at: Date; created: Date; updated: Date; } ... - dateTransformer(['created', 'updated', 'publishedAt']) + dateTransformer(['created', 'updated', 'published_at'])- <p>Published: {post.publishedAt.toLocaleDateString()}</p> + <p>Published: {post.published_at.toLocaleDateString()}</p>Also applies to: 687-701, 706-707
🧹 Nitpick comments (8)
vitest.config.ts (1)
13-17: Consider: stderr suppression may hide useful diagnostics.The
onConsoleLoghandler suppresses all stderr output, which could hide useful error messages or warnings during test failures. Consider being more selective or removing this if it's not addressing a specific noisy output issue.src/lib/utils.ts (1)
40-49: Consider enhancing error context in transformer pipeline.The error handling gracefully recovers by returning the original record, but the error message doesn't indicate which transformer failed or include the record ID for debugging. Consider enhancing the error log:
- console.error('Error applying transformers:', error); + console.error(`Error applying transformer to record ${record.id}:`, error, transformer.name || transformer);This would help identify problematic transformers during development.
src/types/useCommon.type.ts (1)
15-19: Clarify default and disabling behavior in JSDoc.Note default
[dateTransformer()]and that[]disables all transforms for consistency with README./** - * Transformers to apply to the data + * Transformers to apply to the data. + * Default: `[dateTransformer()]`. Pass `[]` to disable all transformations. */ transformers?: RecordTransformer<T>[];src/hooks/useRecord.ts (1)
106-109: Harden unsubscribe cleanup to avoid unhandled rejections.Guard the promise to prevent noisy unhandled rejections during teardown.
- return () => { - unsubscribe.then((unsub) => unsub()); - }; + return () => { + void unsubscribe.then((unsub) => unsub()).catch(() => {}); + };(and same change in the filter subscription cleanup)
Also applies to: 131-133
README.md (1)
406-416: Document therealtimeoption foruseRecord.The hook supports
realtime(default true) likeuseCollection; include it here for consistency.- `fields`: Fields to return - `defaultValue`: Default value while loading - `requestKey`: Optional key passed to PocketBase for request cancellation (optional) + - `realtime`: Boolean to enable/disable real-time subscriptions (default: `true`) - `transformers`: Array of transformer functions to apply to the record (default: `[dateTransformer()]`)src/hooks/useCollection.ts (3)
45-47: Update transformers ref inside an effect (consistency withuseRecord).Functional today, but aligning patterns reduces surprises.
- const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); - transformers.current = options.transformers ?? [dateTransformer<Record>()]; + const transformers = useRef(options.transformers ?? [dateTransformer<Record>()]); + useEffect(() => { + transformers.current = options.transformers ?? [dateTransformer<Record>()]; + }, [options.transformers]);
90-103: Avoid duplicates oncreateevents.Guard pushes by id to prevent duplicates when the record already exists locally.
- case 'create': - { - newData.push(applyTransformers(e.record, transformers.current)); - } + case 'create': { + const exists = newData.some(({ id }) => id === e.record.id); + if (!exists) { + newData.push(applyTransformers(e.record, transformers.current)); + } + }
131-133: Harden unsubscribe cleanup to avoid unhandled rejections.Same rationale as
useRecord.- return () => { - unsubscribe.then((unsub) => unsub()); - }; + return () => { + void unsubscribe.then((unsub) => unsub()).catch(() => {}); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
CLAUDE.md(3 hunks)README.md(6 hunks)examples/README.md(1 hunks)src/hooks/useCollection.ts(5 hunks)src/hooks/useRecord.ts(6 hunks)src/index.ts(1 hunks)src/lib/utils.ts(2 hunks)src/transformers/date.transformer.ts(1 hunks)src/transformers/index.ts(1 hunks)src/types/index.ts(1 hunks)src/types/record-transformer.type.ts(1 hunks)src/types/useCollection.type.ts(2 hunks)src/types/useCommon.type.ts(1 hunks)src/types/useRecord.type.ts(1 hunks)tests/context.test.tsx(1 hunks)tests/hooks/useCollection.test.tsx(6 hunks)tests/hooks/usePocketBase.test.tsx(1 hunks)tests/hooks/useRecord.test.tsx(2 hunks)tests/setup.ts(1 hunks)vite.config.ts(0 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- vite.config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/transformers/index.ts
- tests/setup.ts
- src/types/record-transformer.type.ts
- src/index.ts
- src/transformers/date.transformer.ts
🧰 Additional context used
🧬 Code graph analysis (8)
tests/hooks/useRecord.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useRecord.ts (1)
useRecord(35-137)
src/types/useRecord.type.ts (2)
src/index.ts (1)
UseRecordOptions(16-16)src/types/useCommon.type.ts (1)
UseCommonOptions(4-19)
src/lib/utils.ts (1)
src/types/record-transformer.type.ts (1)
RecordTransformer(10-10)
src/types/useCollection.type.ts (1)
src/types/useCommon.type.ts (1)
UseCommonOptions(4-19)
src/hooks/useCollection.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-49)
src/hooks/useRecord.ts (2)
src/transformers/date.transformer.ts (1)
dateTransformer(8-20)src/lib/utils.ts (1)
applyTransformers(40-49)
tests/hooks/useCollection.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useCollection.ts (1)
useCollection(34-137)
src/types/useCommon.type.ts (1)
src/types/record-transformer.type.ts (1)
RecordTransformer(10-10)
🔇 Additional comments (15)
tests/context.test.tsx (1)
1-1: LGTM! Correct Vitest-specific import.The import change to
@testing-library/jest-dom/vitestaligns with the Vitest setup and ensures proper type definitions are loaded. Based on learnings.examples/README.md (1)
32-32: LGTM! Documentation updated to reflect camelCase naming.The field name change from
published_attopublishedAtaligns with JavaScript naming conventions and the transformer approach introduced in this PR.tests/hooks/usePocketBase.test.tsx (1)
1-1: LGTM! Consistent Vitest import update.The import change to
@testing-library/jest-dom/vitestis consistent with the Vitest migration across the test suite. Based on learnings.src/types/index.ts (1)
3-3: LGTM! New type exports for transformer infrastructure.The addition of
record-transformer.typeanduseCommon.typeexports enables consumers to use the transformer types and common options, supporting the new transformer feature.Also applies to: 6-6
src/types/useCollection.type.ts (2)
10-10: LGTM! Generic extends enables transformer support.The change to
extends UseCommonOptions<T>correctly propagates the generic type parameter, enabling thetransformersoption from the base interface.
49-49: LGTM! Clear API rename fromsubscribetorealtime.The rename improves clarity about the feature's purpose. Note this is a breaking change for existing users.
vitest.config.ts (1)
1-19: LGTM! Well-configured Vitest setup.The configuration properly sets up jsdom environment, coverage reporting, and test setup. The coverage exclusions are comprehensive and appropriate.
src/types/useRecord.type.ts (2)
10-10: LGTM! Generic extends enables transformer support.The change to
extends UseCommonOptions<T>correctly propagates the generic type parameter, consistent withuseCollectionchanges.
16-19: LGTM! Clear real-time option with documented default.The
realtimeoption is well-documented with its default behavior (true), providing clear API surface for controlling real-time subscriptions.src/lib/utils.ts (1)
7-7: LGTM! Non-mutating sort implementation.Creating a shallow copy with
[...records]before sorting prevents mutation of the input array, which is better defensive programming.CLAUDE.md (2)
48-52: LGTM! Clear and comprehensive transformer documentation.The documentation accurately describes the transformer pipeline, error handling, and the use of
useReffor maintaining stable references. This aligns with best practices for avoiding unnecessary subscription recreations.
56-58: Documentation updates are consistent and accurate.All changes properly reflect the new transformer feature and the
realtimeoption rename. The file structure updates correctly document the new utilities and transformer modules.Also applies to: 75-76, 141-141, 145-145
tests/hooks/useCollection.test.tsx (3)
1-1: LGTM! Correct Vitest-specific imports.The import of
@testing-library/jest-dom/vitestis the proper entry point for Vitest compatibility, and theviimport is necessary for mock functions used throughout the tests.Also applies to: 5-5
555-647: LGTM! Comprehensive realtime option test coverage.The test suite correctly reflects the option rename from
subscribetorealtime. The new test for unsubscribe behavior when realtime toggles off (lines 628-646) is a valuable addition that ensures proper cleanup.
649-896: LGTM! Comprehensive transformer test coverage.The transformer test suite thoroughly validates:
- Default
dateTransformerbehavior converting ISO strings to Date objects- Custom transformer application on initial fetch
- Transformer application on real-time create and update events
- Graceful error handling with console logging and fallback to original records
- Sequential application of multiple transformers
All test logic is sound and aligns with the documented behavior in CLAUDE.md.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/hooks/useRecord.test.tsx (1)
115-117: Good fix: options object no longer includes undefined fields.Using {} matches the hook’s conditional spread (omits undefined). Thanks for aligning with prior guidance.
tests/hooks/useCollection.test.tsx (1)
80-85: Remove undefined fields from subscribe expectations.Hook omits undefined via conditional spread; tests should expect {}.
Apply:
- expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - expand: undefined, - filter: undefined, - }); + expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), {});Do this in both occurrences.
#!/bin/bash # Find expectations that still include `expand: undefined` / `filter: undefined` rg -n "expand:\s*undefined|filter:\s*undefined" tests/hooks/useCollection.test.tsxAlso applies to: 507-513
🧹 Nitpick comments (3)
tests/hooks/useCollection.test.tsx (2)
1-1: Use waitFor for unsubscribe and drop act import.Stay consistent with the guideline “prefer waitFor”. You can wait for the unsubscribe spy directly.
- import { act, renderHook, waitFor } from '@testing-library/react'; + import { renderHook, waitFor } from '@testing-library/react'; @@ - await act(async () => { - await Promise.resolve(); - }); rerender({ realtime: false }); - await act(async () => { - await Promise.resolve(); - }); - expect(unsubSpy).toHaveBeenCalled(); + await waitFor(() => expect(unsubSpy).toHaveBeenCalled());Also applies to: 561-579
162-169: Move subscriptionCallback calls outside waitFor blocks; use waitFor only for assertions.The pattern has been verified at all 9 locations (lines 162–169, 191–199, 217–225, 245–253, 273–285, 300–308, 679–690, 722–733, 768–782). Firing callbacks inside waitFor re-executes them on each assertion failure, defeating the purpose of waitFor and potentially causing flaky tests. Refactor by invoking subscriptionCallback before waitFor, then wrap only the assertion:
- await waitFor(() => { - subscriptionCallback({ action: 'create', record: newRecord }); - expect(result.current.data).toEqual([...initialData, newRecord]); - }); + subscriptionCallback({ action: 'create', record: newRecord }); + await waitFor(() => expect(result.current.data).toEqual([...initialData, newRecord]));Apply across all real-time test cases (create/update/delete, sorting, transformers, error paths).
tests/hooks/useRecord.test.tsx (1)
151-163: MovesubscriptionCallbackinvocations outsidewaitForblocks to prevent test flakiness.The callback is being invoked inside
waitFor, which re-runs on each iteration and can trigger multiple state updates. Invoke the event once outside, then await the state change insidewaitFor.Apply this pattern across all 7 occurrences:
- await waitFor(() => { - subscriptionCallback({ action: 'update', record: updatedRecord }); - expect(result.current.data).toEqual(updatedRecord); - }); + subscriptionCallback({ action: 'update', record: updatedRecord }); + await waitFor(() => expect(result.current.data).toEqual(updatedRecord));Locations: lines 155–163, 185–193, 215–223, 245–253, 350–361, 400–411, 452–465.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(4 hunks)tests/context.test.tsx(0 hunks)tests/hooks/useCollection.test.tsx(22 hunks)tests/hooks/usePocketBase.test.tsx(0 hunks)tests/hooks/useRecord.test.tsx(11 hunks)tests/types.d.ts(0 hunks)
💤 Files with no reviewable changes (3)
- tests/types.d.ts
- tests/hooks/usePocketBase.test.tsx
- tests/context.test.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
tests/hooks/useCollection.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useCollection.ts (1)
useCollection(34-137)
tests/hooks/useRecord.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useRecord.ts (1)
useRecord(35-137)
🔇 Additional comments (2)
tests/hooks/useCollection.test.tsx (1)
582-812: Transformer pipeline tests look solid.Covers default dateTransformer, custom transformer(s), RT events, error fallback, and sequencing. LGTM.
CLAUDE.md (1)
141-147: Peer dependency documentation is accurate.The documented React and PocketBase peer dependency versions in CLAUDE.md match the actual package.json configuration exactly. No action required.
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
2 times, most recently
from
e4bf6e8 to
7e79681
Compare
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/hooks/useAuth.test.tsx (1)
171-198: Test is invalid: calling subscriptionCallback when realtime=false can be undefined.With
realtime: false, the hook does not subscribe, so no callback is registered. Calling it risks a throw and doesn’t validate behavior.Minimal fix: don’t capture/call the callback in this test (you already have a separate test asserting no subscription).
@@ - let subscriptionCallback: (e: { action: string; record: unknown }) => void; @@ - mockPocketBaseWithUser.collection = vi.fn().mockReturnValue({ - subscribe: vi.fn((id: string, callback: (e: { action: string; record: unknown }) => void) => { - subscriptionCallback = callback; - return Promise.resolve(() => {}); - }), - }); + mockPocketBaseWithUser.collection = vi.fn().mockReturnValue({ + subscribe: vi.fn(() => Promise.resolve(() => {})), + }); @@ - waitFor(() => { - subscriptionCallback({ action: 'delete', record: mockUser }); - expect(result.current.user).toEqual(mockUser); - expect(result.current.isAuthenticated).toBe(true); - }); + // no subscription => user remains unchanged + expect(result.current.user).toEqual(mockUser); + expect(result.current.isAuthenticated).toBe(true);
🧹 Nitpick comments (1)
CLAUDE.md (1)
131-132: Clarify waitFor best practices to avoid flakiness.Tighten guidance: keep callbacks synchronous, do actions before waitFor, and always return/await the promise.
- - `bun run test` - Run all tests once + - `bun run test` - Run all tests once @@ -- Use `waitFor` from `@testing-library/react` to wait for async state updates (not `act` + `setTimeout`) +- Use `waitFor` from `@testing-library/react` for assertions only (not `act` + `setTimeout`): + - Do NOT pass an async callback. + - Perform actions (user events/hook calls) before waitFor. + - Always return or await the waitFor promise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CLAUDE.md(4 hunks)tests/hooks/useAuth.test.tsx(19 hunks)tests/hooks/useCollection.test.tsx(22 hunks)tests/hooks/useCreateMutation.test.tsx(6 hunks)tests/hooks/useDeleteMutation.test.tsx(6 hunks)tests/hooks/useRecord.test.tsx(11 hunks)tests/hooks/useUpdateMutation.test.tsx(6 hunks)tests/types.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- tests/types.d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/hooks/useRecord.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useRecord.ts (1)
useRecord(35-137)
tests/hooks/useCollection.test.tsx (2)
tests/test-utils.tsx (2)
createWrapper(114-116)record(86-88)src/hooks/useCollection.ts (1)
useCollection(34-137)
🔇 Additional comments (7)
tests/hooks/useCreateMutation.test.tsx (1)
1-1: Import looks good, but usage pattern needs correction.The
waitForimport is appropriate for handling async state updates in tests.tests/hooks/useDeleteMutation.test.tsx (1)
1-1: LGTM: Import addition is appropriate.The
waitForimport is necessary for the test synchronization patterns used throughout this file.tests/hooks/useRecord.test.tsx (3)
115-117: Subscription options expectation looks correct.Hook omits undefined keys; asserting
{}here matches the implementation. Nice catch aligning with earlier feedback.
255-504: Great transformer coverage (default, custom, errors, sequencing, realtime).Thorough scenarios and correct expectations. Using
applyTransformerseffects is validated both on fetch and realtime.
555-567: Cleanup on unmount verified.Asserting
unsubcalled once prevents leaks. Solid.tests/hooks/useCollection.test.tsx (1)
300-308: Good null-currentData coverage for realtime path.Validates functional updater defensive branch when current data is null.
CLAUDE.md (1)
141-147: All documented peer dependencies and exports verified correctly.The verification confirms:
- package.json peerDependencies: React >=19.0.0 ✓ and PocketBase ^0.26.2 ✓ match the documentation
dateTransformeris properly exported at the root (src/index.ts:7)- Documented defaults and behaviors align with the codebase
No issues found.
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
from
7e79681 to
eec735d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
tests/hooks/useUpdateMutation.test.tsx (1)
110-115: Critical bug: Missingawaitand incorrectwaitForusage.This test still has the critical issues flagged in previous reviews:
- Line 110:
waitForis not awaited, causing the test to proceed immediately- Line 111:
mutateis called insidewaitFor, which can cause multiple invocations during retries- Line 114:
resolveUpdateis called insidewaitFor, breaking the test flowApply this diff to fix the test:
- waitFor(() => { - result.current.mutate('1', { title: 'Test' }); - expect(result.current.isPending).toBe(true); - expect(result.current.isSuccess).toBe(false); - resolveUpdate({ id: '1', title: 'Test' }); - }); - - await updatePromise; - - return waitFor(() => { - expect(result.current.isPending).toBe(false); - expect(result.current.isSuccess).toBe(true); - }); + result.current.mutate('1', { title: 'Test' }); + + await waitFor(() => { + expect(result.current.isPending).toBe(true); + expect(result.current.isSuccess).toBe(false); + }); + + resolveUpdate({ id: '1', title: 'Test' }); + await updatePromise; + + return waitFor(() => { + expect(result.current.isPending).toBe(false); + expect(result.current.isSuccess).toBe(true); + });tests/hooks/useRecord.test.tsx (3)
82-85: Remove undefined properties from subscribe expectation.The subscribe call at line 83 expects
expand: undefined, filter: undefined, but the hook omits undefined keys. The expectation should be{}.Apply this diff:
expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - expand: undefined, - filter: undefined, + }); - });
115-117: Removeexpand: undefinedfrom subscribe expectation to match hook behavior.The hook omits undefined option keys, so the expectation should be an empty object
{}instead of containingexpand: undefined.Apply this diff:
- expect(mockSubscribe).toHaveBeenCalledWith('1', expect.any(Function), {}); + expect(mockSubscribe).toHaveBeenCalledWith('1', expect.any(Function), {});Wait, the current code already shows
{}on line 116. Let me re-check... Actually looking at line 116, it correctly shows{}. This appears to have been fixed already. Let me skip this comment.
128-132: Remove undefined properties from subscribe expectation.Same issue as lines 82-85: the expectation includes
filter: undefinedbut the hook omits undefined keys when no filter is provided.Apply this diff:
expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - filter: 'slug="test-record"', + filter: 'slug="test-record"', });Wait, that's not right. Line 130 shows
filter: 'slug="test-record"'which is a valid value, not undefined. Let me re-read the annotated code...Actually, the filter has a real value here, so this is correct. The past comment was about when there's NO filter value. Let me skip this.
tests/hooks/useDeleteMutation.test.tsx (1)
104-119: Fix missingawaiton line 104 and refactor pending state test pattern.Line 104 is missing
await, which means the test doesn't actually wait for those assertions. Additionally, the pattern for testing intermediateisPendingstate is problematic—triggering the mutation insidewaitForis an anti-pattern.Apply this corrected pattern:
- waitFor(() => { + result.current.mutate('1'); + + await waitFor(() => { - result.current.mutate('1'); expect(result.current.isPending).toBe(true); expect(result.current.isSuccess).toBe(false); }); - await waitFor(async () => { + await waitFor(() => { resolveDelete(true); - await deletePromise; + }); + + await deletePromise; + + return waitFor(() => { expect(result.current.isPending).toBe(false); expect(result.current.isSuccess).toBe(true); });tests/hooks/useAuth.test.tsx (2)
49-52: Move action outsidewaitForcallback.Line 50 calls
result.current.signIn.email(...)inside thewaitForcallback, which is an anti-pattern. Actions should be performed before enteringwaitFor.Apply this diff:
+ result.current.signIn.email('test@example.com', 'password'); + return waitFor(() => { - result.current.signIn.email('test@example.com', 'password'); expect(result.current.isLoading).toBe(true); });
305-309: Move action outsidewaitForand remove unnecessarywaitFor.Line 306 calls
result.current.signOut()inside thewaitForcallback. SincesignOutis synchronous and we're only asserting that the mock was called,waitForis unnecessary here.Apply this diff:
- return waitFor(() => { - result.current.signOut(); + result.current.signOut(); - expect(mockClear).toHaveBeenCalled(); - }); + expect(mockClear).toHaveBeenCalled();tests/hooks/useCollection.test.tsx (2)
82-85: Remove undefined properties from subscribe expectation.The expectation includes
expand: undefined, filter: undefinedon lines 83-84, but the hook omits undefined keys when they have no value. The expectation should be{}.Apply this diff:
expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - expand: undefined, - filter: undefined, });
509-512: Remove undefined properties from subscribe expectation.Same issue as lines 82-85: the expectation on lines 510-511 includes
expand: undefined, filter: undefinedbut the hook omits these keys.Apply this diff:
expect(mockSubscribe).toHaveBeenCalledWith('*', expect.any(Function), { - expand: undefined, - filter: undefined, });
🧹 Nitpick comments (1)
CLAUDE.md (1)
131-134: Consider adding a brief example of the waitFor pattern.The guidance is clear, but adding a small code example showing the correct pattern (action before waitFor, only assertions inside) would make it more actionable for developers.
Example to add after line 134:
Example: ```typescript // ✅ Correct await result.current.mutate('1'); await waitFor(() => { expect(result.current.isSuccess).toBe(true); }); // ❌ Incorrect await waitFor(async () => { await result.current.mutate('1'); expect(result.current.isSuccess).toBe(true); });</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7e79681250bf2b93e976d11c4322ba9ba469f755 and eec735d27f6ac8d318e23bfdc003a95e2f94414f. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `CLAUDE.md` (4 hunks) * `tests/hooks/useAuth.test.tsx` (18 hunks) * `tests/hooks/useCollection.test.tsx` (22 hunks) * `tests/hooks/useCreateMutation.test.tsx` (6 hunks) * `tests/hooks/useDeleteMutation.test.tsx` (6 hunks) * `tests/hooks/useRecord.test.tsx` (11 hunks) * `tests/hooks/useUpdateMutation.test.tsx` (6 hunks) * `tests/types.d.ts` (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * tests/types.d.ts </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * tests/hooks/useCreateMutation.test.tsx </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>tests/hooks/useAuth.test.tsx (2)</summary><blockquote> <details> <summary>tests/test-utils.tsx (1)</summary> * `createWrapper` (114-116) </details> <details> <summary>src/hooks/useAuth.ts (1)</summary> * `useAuth` (33-226) </details> </blockquote></details> <details> <summary>tests/hooks/useCollection.test.tsx (3)</summary><blockquote> <details> <summary>tests/test-utils.tsx (2)</summary> * `createWrapper` (114-116) * `record` (86-88) </details> <details> <summary>src/hooks/useCollection.ts (1)</summary> * `useCollection` (34-137) </details> <details> <summary>src/types/record-transformer.type.ts (1)</summary> * `RecordTransformer` (10-10) </details> </blockquote></details> <details> <summary>tests/hooks/useRecord.test.tsx (2)</summary><blockquote> <details> <summary>tests/test-utils.tsx (2)</summary> * `createWrapper` (114-116) * `record` (86-88) </details> <details> <summary>src/hooks/useRecord.ts (1)</summary> * `useRecord` (35-137) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>tests/hooks/useUpdateMutation.test.tsx (3)</summary><blockquote> `1-1`: **LGTM – Import updated correctly.** The addition of `waitFor` to the imports is appropriate for the refactored test patterns used throughout this file. --- `28-97`: **Excellent fix – mutation pattern corrected.** These four tests now correctly call `mutate` outside of `waitFor` and use `waitFor` solely for asserting state transitions. This resolves the critical issues identified in previous reviews where mutations could be invoked multiple times during retries. --- `125-130`: **LGTM – Error handling test is correct.** The test properly verifies that the hook throws an appropriate error when used outside the provider context. </blockquote></details> <details> <summary>CLAUDE.md (1)</summary><blockquote> `48-52`: **Documentation looks comprehensive and accurate.** The transformer documentation clearly explains the feature, including: - Sequential application of transformers - Default dateTransformer behavior - Error handling that preserves original data - Use of useRef for stable references This aligns well with the implementation details from the PR. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
from
eec735d to
31b3eae
Compare
KevinBonnoron
force-pushed
the
6-feature-add-data-transformers-support
branch
from
31b3eae to
dd48f25
Compare
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests