Conversation
WalkthroughAdds a FirestoreSnapshotManager with subject factories, refactors React Query option builders to use factory-produced queryFns, restructures rxjs/sentry subject types and exports, introduces many Vitest test suites and configs, and updates package manifests, READMEs, and dev tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Component/Hook
participant RQ as React Query
participant FSM as FirestoreSnapshotManager
participant Sub as SnapshotSubject
participant FS as Firestore
UI->>RQ: useQuery with snapshot options
RQ->>FSM: request subject factory (ref/query/factory, snapshotOptions, listener)
FSM-->>RQ: returns (queryKey) => Sub
RQ->>Sub: instantiate via factory(queryKey)
Sub->>FS: attach onSnapshot
Sub-->>RQ: emit SnapshotState via observer
RQ-->>UI: deliver state update
sequenceDiagram
participant RQ as React Query Cache
participant FSM as FirestoreSnapshotManager
participant Sub as SnapshotSubject
participant Sentry as Sentry
RQ-->>FSM: notify removed { meta.type: 'snapshot', queryKey }
FSM->>FSM: lookup onClose callback by queryKey
alt callback found
FSM->>Sub: invoke close()
FSM->>FSM: unregister callback
else not found
FSM->>Sentry: captureMessage(...) / log warning
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (104)
packages/zustand-firestore/src/store/__tests__/useContextStoreSubscription.test.ts (5)
16-21: Wrap emission in act and assert via waitFor to avoid effect timing flakinessThe subscription is typically established in a React effect. Wrapping the emission in act and asserting with waitFor reduces flakiness across React/test environments.
Apply these diffs:
-import { renderHook } from '@testing-library/react' +import { renderHook, act, waitFor } from '@testing-library/react'- it('subscribes to context store and updates state', () => { + it('subscribes to context store and updates state', async () => {- subject.next({ value: 2 }) - expect(store.getState()).toEqual({ value: 2 }) + act(() => { + subject.next({ value: 2 }) + }) + await waitFor(() => { + expect(store.getState()).toEqual({ value: 2 }) + })
1-7: Resolve ESLint import resolution errors for react/testing-library/rxjs/zustandESLint flags
import-x/no-unresolvedfor react, @testing-library/react, rxjs, and zustand. In a workspace, ensure this package declares appropriate peerDeps/devDeps and that ESLint’s resolver knows about your TS project references.Recommendations:
- In packages/zustand-firestore/package.json:
- peerDependencies: react, rxjs, zustand (versions matching the workspace), with devDependencies mirroring these for tests.
- devDependencies (tests/build): @testing-library/react, vitest.
- Configure ESLint resolver (e.g., .eslintrc.js) so import-x resolves workspace projects:
- settings.import/resolver.typescript.project: ['packages/*/tsconfig.json']
- settings.import/resolver.node.extensions: ['.ts', '.tsx', '.js', '.jsx']
If helpful, I can propose concrete package.json and ESLint config snippets aligned with your repo’s versions.
1-1: Rename file to kebab-case to satisfy unicorn/filename-caseESLint suggests renaming
useContextStoreSubscription.test.tstouse-context-store-subscription.test.ts. If you switch to a JSX wrapper later, consider.test.tsx.
16-18: Minor: asObservable() is optional hereSubject is already an Observable. Passing
subjectdirectly works and can simplify the call.- useContextStoreSubscription(Ctx, subject.asObservable()) + useContextStoreSubscription(Ctx, subject)
5-5: Add a cleanup test to ensure unsubscription on unmountVerifying unsubscription guards against leaks and accidental updates after teardown.
Apply these diffs:
-import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest'- }) -}) + }) + + it('unsubscribes from observable on unmount', () => { + const subject = new Subject<{ value: number }>() + const store = createStore<{ value: number }>(() => ({ value: 0 })) + const Ctx = createContext<StoreApi<{ value: number }>>(store) + const spy = vi.spyOn(store, 'setState') + + const { unmount } = renderHook(() => { + useContextStoreSubscription(Ctx, subject) + }) + unmount() + + act(() => { + subject.next({ value: 123 }) + }) + expect(spy).not.toHaveBeenCalled() + }) +})Also applies to: 21-22
packages/zustand-firestore/src/store/__tests__/useStoreSubscription.test.ts (5)
1-5: Importactto avoid React testing warnings during updatesEmitting values into the Subject triggers state updates; importing and using
actprevents “not wrapped in act(...)” warnings and ensures effects flush before assertions.Apply this diff:
-import { renderHook } from '@testing-library/react' +import { renderHook, act } from '@testing-library/react'
17-19: Wrap Subject emissions inactto flush effects before assertingThis avoids potential timing flakiness and React act warnings.
Apply this diff:
- subject.next({ value: 1 }) - expect(store.getState()).toEqual({ value: 1 }) + act(() => { + subject.next({ value: 1 }) + }) + expect(store.getState()).toEqual({ value: 1 })
20-21: Optional: Wrapunmount()inactfor consistent cleanup semanticsNot strictly required with RTL’s renderHook, but wrapping can silence edge-case act warnings in stricter setups.
Apply this diff:
- unmount() + act(() => { + unmount() + })
1-1: Rename file to kebab-case per ESLint ruleESLint (unicorn/filename-case) expects kebab-case. Suggested rename:
- useStoreSubscription.test.ts → use-store-subscription.test.ts
1-4: Dev dependencies are declared correctly; verify ESLint import resolver setup
- packages/zustand-firestore/package.json already lists:
- rxjs (v7.8.2)
- zustand (v5.0.7)
- @testing-library/react (v16.3.0)
- No additional devDependencies are required.
- If you’re still seeing
import-x/no-unresolved:
- Confirm your ESLint config (via @valian/eslint-config) includes
import/resolversettings for Node, TypeScript, and workspace paths.- Install/configure
eslint-import-resolver-nodeandeslint-import-resolver-typescriptso imports from@testing-library/react,rxjs, andzustandresolve correctly.packages/react-kitchen-sink/src/react-query/observers/__tests__/documentSnapshotQueryClientObserver.test.ts (5)
16-18: Tighten assertion on next() to verify exact payload and call orderAssert with arguments and call order so regressions are caught precisely.
Apply this diff:
- observer.next({ isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + const state = { isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState + observer.next(state) + expect(client.setQueryData).toHaveBeenNthCalledWith(1, queryKey, state)
1-4: Assert Sentry capture on error and make call verification deterministicSince the observer reports errors via captureException, assert it was called and also verify the 2nd call args to setQueryData.
Apply this diff:
+import { captureException } from '@sentry/react'- observer.error(new Error('x')) - expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { + const err = new Error('x') + observer.error(err) + expect(captureException).toHaveBeenCalledWith(err) + expect(client.setQueryData).toHaveBeenNthCalledWith(2, queryKey, { isLoading: false, hasError: true, disabled: false, })Also applies to: 19-25
26-32: Also verify the complete() call order and total invocationsThis makes the test resilient to extra/duplicated updates.
Apply this diff:
- expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { + expect(client.setQueryData).toHaveBeenNthCalledWith(3, queryKey, { isLoading: false, hasError: false, disabled: true, }) + expect(client.setQueryData).toHaveBeenCalledTimes(3)
1-1: Type queryKey as QueryKey (readonly) to align with React Query typingImproves type safety and avoids widening to mutable string[].
Apply this diff:
-import { type QueryClient } from '@tanstack/react-query' +import { type QueryClient, type QueryKey } from '@tanstack/react-query'- const queryKey = ['k'] + const queryKey: QueryKey = ['k'] as constAlso applies to: 13-13
1-1: Rename test file to kebab-case to satisfy unicorn/filename-caseESLint suggests: document-snapshot-query-client-observer.test.ts.
packages/zustand-firestore/src/contexts/__tests__/createQuerySnapshotStoreContext.test.ts (1)
1-1: Test filename casing vs. rule unicorn/filename-case.Either rename the file to kebab-case (create-query-snapshot-store-context.test.ts) or disable the rule for test files in ESLint config. If you prefer a one-off suppression, add a file-level disable.
packages/zustand-firestore/src/store/__tests__/createDocumentSnapshotStore.test.ts (1)
1-1: Filename casing nitpick.If enforcing unicorn/filename-case, rename to kebab-case (create-document-snapshot-store.test.ts) or relax the rule for test files.
packages/zustand-firestore/src/contexts/__tests__/createDocumentSnapshotStoreContext.test.ts (1)
1-1: Filename casing nitpick.Align with unicorn/filename-case by renaming to kebab-case (create-document-snapshot-store-context.test.ts) or disable the rule for tests.
packages/zustand-firestore/src/store/__tests__/createQuerySnapshotStore.test.ts (1)
8-15: Make the test less brittle by asserting a partial shape.If new state fields are added later, this exact equality will fail unnecessarily. Using a partial match focuses on invariants that matter here.
- expect(store.getState()).toEqual({ + expect(store.getState()).toMatchObject({ empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], })packages/react-kitchen-sink/src/react-query/observers/__tests__/querySnapshotQueryClientObserver.test.ts (4)
16-25: Make the “next” assertion precise and isolate phases.Asserting only “called” can mask regressions. Recommend asserting the exact payload and clearing mocks before the next phase to keep expectations independent.
Apply this diff:
- observer.next({ - empty: false, - size: 1, - isLoading: false, - hasError: false, - disabled: false, - data: [{}], - } as QuerySnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + const snapshot = { + empty: false, + size: 1, + isLoading: false, + hasError: false, + disabled: false, + data: [{}], + } as QuerySnapshotState + observer.next(snapshot) + expect(client.setQueryData).toHaveBeenCalledWith(queryKey, snapshot) + expect(client.setQueryData).toHaveBeenCalledTimes(1) + vi.clearAllMocks()
1-9: Assert Sentry capture on error (import the mocked module).You already mock Sentry; import it to assert capture is invoked with the thrown error.
Apply this diff to import the mocked module:
import { mock } from 'vitest-mock-extended' +import * as Sentry from '@sentry/react' import { querySnapshotQueryClientObserver } from '../querySnapshotQueryClientObserver'
26-37: Verify Sentry capture, and isolate error/complete assertions.This tightens guarantees that errors are reported and keeps each phase’s expectations independent.
Apply this diff:
- observer.error(new Error('x')) + const error = new Error('x') + observer.error(error) + expect(Sentry.captureException).toHaveBeenCalledWith(error) expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: true, disabled: false, data: [], }) + expect(client.setQueryData).toHaveBeenCalledTimes(1) + vi.clearAllMocks() - observer.complete() + observer.complete() expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], }) + expect(Sentry.captureException).not.toHaveBeenCalled() + expect(client.setQueryData).toHaveBeenCalledTimes(1)
1-1: Filename violates kebab-case rule flagged by ESLint.Either rename to kebab-case or add an override for tests if you prefer the current style.
Options:
- Rename file to: query-snapshot-query-client-observer.test.ts
- Or add an ESLint override for test files in .eslintrc:
{
"overrides": [
{
"files": ["/*.test.ts", "/*.test.tsx"],
"rules": { "unicorn/filename-case": "off" }
}
]
}packages/react-kitchen-sink/rollup.config.js (1)
4-18: Consider addingreact/jsx-runtimetoexternalfor React 17+ automatic JSX runtimeIf the TS config uses
jsx: "react-jsx"(common), Rollup may try to bundlereact/jsx-runtimeunless it’s explicitly externalized. Safe, low-effort tweak:'observable-hooks', 'react', + 'react/jsx-runtime', 'rxjs', 'type-fest',packages/zustand-firestore/src/hooks/__tests__/useSnapshotListenOptions.test.ts (4)
1-30: Rename file to kebab-case to satisfy lint ruleESLint (unicorn/filename-case) requires kebab-case. Please rename:
Current:
- packages/zustand-firestore/src/hooks/tests/useSnapshotListenOptions.test.ts
Suggested:
- packages/zustand-firestore/src/hooks/tests/use-snapshot-listen-options.test.ts
13-14: Nit: rerender without extraneous propsThe hook under test doesn’t accept props in this case; passing {} to rerender is unnecessary.
- rerender({}) + rerender()
17-28: Type consistency: 'source' isn’t part of Firestore’s SnapshotListenOptionsFirestore’s public SnapshotListenOptions only defines includeMetadataChanges; source belongs to get/read options. Since the hook under test intentionally supports source too, make the test’s props explicit with an extended type to avoid excess property errors and clarify intent.
-import { type SnapshotListenOptions } from '@firebase/firestore' +import type { SnapshotListenOptions } from 'firebase/firestore' +type ExtendedSnapshotListenOptions = SnapshotListenOptions & { + source?: 'default' | 'cache' | 'server' +} - it('keeps only defined fields', () => { - const { result, rerender } = renderHook((props) => useSnapshotListenOptions(props), { - initialProps: { includeMetadataChanges: true } as SnapshotListenOptions, - }) + it('keeps only defined fields', () => { + const { result, rerender } = renderHook<ExtendedSnapshotListenOptions>( + (props) => useSnapshotListenOptions(props), + { initialProps: { includeMetadataChanges: true } }, + )Follow-up: confirm that the implementation’s accepted type matches this extended shape, or refactor the hook’s input type accordingly if this is the intended API.
17-28: Add coverage for boolean false and referential stability with unchanged propsFalse values are “defined” and should be retained. Also worth asserting identity stability when props remain the same across rerenders.
You can append this test:
it('retains false booleans and memoizes with unchanged props', () => { const { result, rerender } = renderHook((props?: { includeMetadataChanges?: boolean }) => useSnapshotListenOptions(props ?? {}), ) rerender({ includeMetadataChanges: false }) expect(result.current).toEqual({ includeMetadataChanges: false }) const sameRef = result.current rerender({ includeMetadataChanges: false }) expect(result.current).toBe(sameRef) })packages/react-kitchen-sink/src/schema/firestoreDataConverter.ts (1)
22-24: Annotate explicit return type (AppModelType) to lock in the contract.Small ergonomics win and clearer intent for consumers; avoids accidental widening if inference changes.
-export const firestoreDataConverter = <AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData>( - snapshot: QueryDocumentSnapshot<AppModelType, DbModelType>, -) => handleFirestoreDataTypes(snapshot.data()) +export const firestoreDataConverter = < + AppModelType = DocumentData, + DbModelType extends DocumentData = DocumentData +>( + snapshot: QueryDocumentSnapshot<AppModelType, DbModelType>, +): AppModelType => handleFirestoreDataTypes(snapshot.data())packages/react-kitchen-sink/src/schema/__tests__/firestoreDataConverter.test.ts (1)
25-27: Type the mock with the two generic parameters to mirror production signature.Aligns the test type with the updated function signature and avoids potential inference drift.
- const snapshot = mock<QueryDocumentSnapshot>() + const snapshot = mock<QueryDocumentSnapshot<DocumentData, DocumentData>>()packages/react-kitchen-sink/src/schema/__tests__/ZodSchemaError.test.ts (1)
1-1: Rename file to kebab-case to satisfy filename-case ruleUnicorn filename-case rule expects test files to be kebab-case.
Rename:
- ZodSchemaError.test.ts → zod-schema-error.test.ts
vitest.config.mjs (1)
1-7: Prefer defineWorkspace for multi-project setupUsing defineWorkspace simplifies multi-package discovery and mirrors Vitest’s recommended pattern for monorepos.
Apply:
-import { defineConfig } from 'vitest/config' - -export default defineConfig({ - test: { - projects: ['packages/*'], - }, -}) +import { defineWorkspace } from 'vitest/config' + +export default defineWorkspace([ + 'packages/*', +])packages/react-kitchen-sink/src/sentry/index.ts (1)
4-5: Consider importing via the sentry barrel to reduce path churnNow that these listeners are exported here, prefer: import { sentryDocumentSnapshotListener } from '../sentry' in call sites instead of deep file paths.
packages/rxjs-firebase/src/subjects/QuerySnapshotSubject.ts (1)
32-35: Complete the BehaviorSubject on close() to signal downstream teardownYou stop the source via
takeUntil, but the subject itself remains open. Completing the subject helps subscribers finalize and prevents dangling observers.Apply this diff:
close(): void { this.notification$.next() this.notification$.complete() + this.complete() }packages/react-kitchen-sink/package.json (2)
39-50: Add @firebase/firestore to devDependencies for test-time resolutionTests and ESLint import resolution in this package reference
@firebase/firestore. Keeping it as a peer is correct for consumers, but adding it to devDependencies ensures local tests/builds resolve and compile.Apply this diff:
"devDependencies": { "vitest-mock-extended": "^3.1.0", "@sentry/core": "^10.1.0", "@sentry/react": "^10.1.0", "@tanstack/react-query": "^5.84.2", + "@firebase/firestore": "^4.7.0", "firebase": "^12.0.0", "react": "^19.1.0", "react-dom": "^19.1.0", "rxjs": "^7.8.2", "zod": "^4.0.17", "zod-firebase": "^2.0.0", "zustand": "^5" },
62-64: Avoid duplicating a dependency in both peer and optional deps; use peerDependenciesMeta insteadPublishing the same package in both peerDependencies and optionalDependencies can lead to confusing installs and duplicate copies. Prefer marking the peer as optional via peerDependenciesMeta.
Apply this diff to replace optionalDependencies with peerDependenciesMeta:
- "optionalDependencies": { - "zustand": "^5" - }, + "peerDependenciesMeta": { + "zustand": { + "optional": true + } + },packages/react-kitchen-sink/src/sentry/__tests__/sentryDocumentSnapshotListener.test.ts (3)
1-1: Rename file to kebab-case to satisfy lint ruleESLint’s
unicorn/filename-caserequires kebab-case. Suggest:sentry-document-snapshot-listener.test.ts.
22-37: Consider tightening assertions for Sentry payloadsOptionally assert the breadcrumb messages and captureException metadata (tags/fingerprint) to guard against regressions in the Sentry integration contract.
Example:
expect(addBreadcrumb).toHaveBeenCalledWith( expect.objectContaining({ category: 'firestore', message: 'received document snapshot for (/c/id)', }), ) expect(captureException).toHaveBeenCalledWith(error, expect.objectContaining({ tags: expect.objectContaining({ firestore: true, 'schema.collection': 'c' }), fingerprint: ['firestore', 'c'], }))
1-2: Ensure@firebase/firestoreis installed and ESLint passes in this package
- Add
@firebase/firestoreto packages/react-kitchen-sink/package.json devDependencies:"devDependencies": { // …
- "@firebase/firestore": "^4.7.0"
}2. Re-run ESLint on the Sentry listener tests to confirm no more unresolved-import errors: ```bash pnpm -w --filter @valian/react-kitchen-sink exec eslint \ src/sentry/__tests__/sentryDocumentSnapshotListener.test.ts \ src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts
- If ESLint still reports
import/no-unresolved:
– In your root ESLint config (eslint.config.mjs), enable the TypeScript resolver:– Or switch to eslint-plugin-import-x with its pnpm-workspace/TS resolver.export default { settings: { 'import/resolver': { typescript: true, }, }, };packages/zustand-firestore/vitest.config.mjs (1)
3-25: Config looks good; consider restoring mocks between testsThe jsdom env, coverage, and strict thresholds are clear and deliberate. Adding
restoreMocks: truecomplementsclearMocksand helps prevent accidental state leakage from jest/vitest mocks.Apply this diff:
test: { environment: 'jsdom', - globals: false, - clearMocks: true, + globals: false, + clearMocks: true, + restoreMocks: true,packages/zustand-firestore/README.md (6)
43-53: Memoize Firestore query to avoid unnecessary resubscriptions and store churnCreating the query object inside the component body will re-create it on every render. Unless your hook normalizes identity (e.g., via queryEqual), this can cause resubscriptions or a new store instance each render.
Consider memoizing the query:
export function TodoList() { - const todosQuery = query( - collection(db, 'todos'), - where('completed', '==', false), - ) + const todosQuery = useMemo( + () => + query( + collection(db, 'todos'), + where('completed', '==', false), + ), + [], + )
60-66: Avoid index as React list key; prefer a stable identifierUsing the array index as a key can lead to reconciliation issues on reorders/insertions. Prefer a unique, stable key (e.g., Firestore document id).
- {data.map((todo, index) => ( - <li key={index}>{todo.title}</li> - ))} + {data.map((todo) => ( + <li key={todo.id}>{todo.title}</li> + ))}
90-97: Memoize document reference to avoid resubscriptionsSimilar to queries, constructing a DocumentReference on each render can cause resubscriptions if the hook compares by reference.
export function UserProfile({ userId }: { userId: string }) { - const ref = doc(db, 'users', userId) + const ref = useMemo(() => doc(db, 'users', userId), [userId])
117-121: Optional: memoize conditional ref for consistencyNot required, but aligning with the memoization pattern can help prevent accidental resubscriptions when parent props change.
export function MaybeUserProfile({ userId }: { userId?: string }) { - const ref = userId ? doc(db, 'users', userId) : null + const ref = useMemo( + () => (userId ? doc(db, 'users', userId) : null), + [userId], + )
132-136: Clarify “?” markers in API shape to avoid ambiguityThe “?” markers can be misread as TypeScript optional property syntax. Explicitly state what “optional” means in runtime terms (e.g., undefined before first snapshot or when disabled).
Both stores expose the same state shapes as `@valian/rxjs-firebase`: - Query store state includes: `data[]`, `size`, `empty`, `isLoading`, `hasError`, `disabled`, and `snapshot`. -- Document store state includes: `data?`, `exists?`, `isLoading`, `hasError`, `disabled`, and `snapshot`. +- Document store state includes: `data?`, `exists?`, `isLoading`, `hasError`, `disabled`, and `snapshot`. + +Note: +- A `?` indicates the field may be `undefined` before the first snapshot arrives or when the subscription is disabled. +- `exists` reflects the last known document existence; it is `undefined` until the first snapshot.
141-147: Tighten wording in “Contributing/Support” for clarity and toneMinor stylistic improvement.
-## Contributing - -Contributions are welcome! Please feel free to submit a Pull Request. +## Contributing + +Contributions are welcome—open a Pull Request anytime. ## Support -If you encounter any issues or have questions, please open an issue in the repository. +If you have questions or run into issues, please open an issue in this repository.packages/react-kitchen-sink/eslint.config.mjs (2)
32-35: Use vitest plugin rules via .rules instead of spreading the entire config blockSpreading
...vitest.configs.recommendedinto a per-files object can merge unintended keys. For Flat Config, it’s safer to add the plugin and spread only.rules.- { - files: ['**/*.{test,spec}.{ts,tsx}'], - ...vitest.configs.recommended, - }, + { + files: ['**/*.{test,spec}.{ts,tsx}'], + plugins: { vitest }, + // If you rely on global test APIs, also add: + // languageOptions: { globals: vitest.environments.env.globals }, + rules: { + ...vitest.configs.recommended.rules, + }, + },
36-41: Consolidate test overrides and consider relaxing filename-case for testsYou can merge this block into the one above to avoid duplication. If you intend to keep camelCase test filenames (as in current tests), consider turning off
unicorn/filename-casefor test files to silence the warnings flagged by static analysis.- { - files: ['**/*.{test,spec}.{ts,tsx}'], - rules: { - 'max-classes-per-file': 'off', - }, - }, + { + files: ['**/*.{test,spec}.{ts,tsx}'], + plugins: { vitest }, + // languageOptions: { globals: vitest.environments.env.globals }, + rules: { + ...vitest.configs.recommended.rules, + 'max-classes-per-file': 'off', + // Optional: if you want to keep camelCase tests + // 'unicorn/filename-case': 'off', + }, + },Would you like me to update the config to disable filename-case for tests, or do you prefer to rename the files to kebab-case?
packages/react-kitchen-sink/vitest.config.mjs (1)
3-25: LGTM; consider adding lcov for local tooling compatibilityConfig looks solid. If you use tools like Codecov or prefer local HTML viewing, adding 'lcov' (and/or 'html') as coverage reporters can help.
- reporter: ['text', 'cobertura'], + reporter: ['text', 'cobertura', 'lcov'],packages/react-kitchen-sink/src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts (1)
17-43: Strengthen expectations to prove both breadcrumbs are emitted and error context is capturedCurrent assertions only check that breadcrumbs/errors were emitted at least once, which could miss regressions. Assert call counts or payloads to ensure all three hooks emit the intended telemetry.
it('forwards calls and adds breadcrumbs/captures errors', () => { @@ - listener.onSnapshot?.({ + listener.onSnapshot?.({ isLoading: false, hasError: false, disabled: false, empty: true, size: 0, data: [], snapshot: stub<QuerySnapshot>(), } as const) - expect(onSnapshot).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + expect(onSnapshot).toHaveBeenCalledTimes(1) + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'received query snapshot for (q)', + }), + ) @@ const error = new Error('x') listener.onError?.(error) - expect(onError).toHaveBeenCalledWith(error) - expect(captureException).toHaveBeenCalled() + expect(onError).toHaveBeenCalledWith(error) + expect(captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: expect.objectContaining({ firestore: true }), + fingerprint: ['firestore', 'c'], + }), + ) @@ listener.onComplete?.() - expect(onComplete).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + expect(onComplete).toHaveBeenCalledTimes(1) + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'closed query snapshot for (q)', + }), + )packages/react-kitchen-sink/src/sentry/__tests__/handleNotAwaitedPromise.test.ts (3)
15-22: Prefer microtask flush over setTimeout for deterministic promise rejection handlingUsing a macrotask delay can be flaky. Flushing the microtask queue is sufficient and more deterministic for handling rejections.
it('captures rejection errors', async () => { const error = new Error('boom') handleNotAwaitedPromise(Promise.reject(error)) - await new Promise((resolve) => { - setTimeout(resolve, 0) - }) + // Flush microtasks so the rejection handler runs + await Promise.resolve() expect(captureException).toHaveBeenCalledWith(error, undefined) })
30-33: Silence Biome’s noThenProperty for intentional thenable test doubleThis test intentionally crafts a thenable without catch. Biome flags it; add a targeted ignore to keep linters green without changing behavior.
it('does nothing when object has no catch', () => { - // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function + // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function + /* biome-ignore lint/suspicious/noThenProperty: crafting a thenable-like object for test */ handleNotAwaitedPromise({ then: () => {} } as unknown as Promise<void>) expect(captureException).not.toHaveBeenCalled() })
1-8: Optional: add a test to validate context forwardingYou can strengthen coverage by asserting that a provided Sentry capture context is forwarded.
describe('handleNotAwaitedPromise', () => { @@ it('captures rejection errors', async () => { @@ }) + it('forwards capture context when provided', async () => { + const error = new Error('boom') + const context = { tags: { x: 'y' } } + handleNotAwaitedPromise(Promise.reject(error), context) + await Promise.resolve() + expect(captureException).toHaveBeenCalledWith(error, context) + }) +packages/zustand-firestore/src/hooks/__tests__/useDocumentStore.test.ts (3)
2-2: Wrap emissions in act, restore spies, and unmount to avoid test leaks and act warningsThese tests may intermittently warn about “not wrapped in act(...)” and can leak spies/subscriptions across tests. Small fixes:
- import
act- restore spies after each test
- emit inside
act- unmount the hook
Apply this diff:
-import { renderHook } from '@testing-library/react' +import { renderHook, act } from '@testing-library/react' @@ describe('useDocumentStore', () => { @@ + afterEach(() => { + vi.restoreAllMocks() + }) @@ - vi.spyOn(rx, 'fromDocumentRef').mockReturnValue(subject.asObservable()) + vi.spyOn(rx, 'fromDocumentRef').mockReturnValueOnce(subject.asObservable()) @@ - const { result } = renderHook(() => useDocumentStore({ ref: stub<DocumentReference>() })) + const { result, unmount } = renderHook(() => useDocumentStore({ ref: stub<DocumentReference>() })) @@ - subject.next(snapshot) + act(() => { + subject.next(snapshot) + }) @@ expect(result.current.getState()).toEqual({ @@ }) + unmount()Also applies to: 14-15, 22-22, 24-24, 29-29, 38-39
1-7: Linter “unresolved import” hints: ensure resolver/peers are satisfied in testsESLint warnings about unresolved modules are likely due to pnpm workspace/peer setup:
@firebase/firestoreis a peer (good for runtime) but not a dev dep; tests import it directly. Either:
- add
@firebase/firestoreto this package’s devDependencies, or- switch type-only imports to
firebase/firestore, or- configure ESLint’s import resolver to consider workspace packages.
Let me know your preference; I can patch the manifest or resolver config.
1-1: Filename doesn’t match kebab-case ruleRule: unicorn/filename-case. Either rename to
use-document-store.test.tsor relax the rule for test files.packages/zustand-firestore/src/hooks/__tests__/useQueryStore.test.ts (3)
27-52: Sturdy live subscription test; tighten with act/cleanupThe subject-driven snapshot and state mapping looks correct. Minor improvements:
- emit inside
act- restore spy after test
- unmount to avoid lingering subscriptions
- nit: setting
exists()onQueryDocumentSnapshotis unnecessary (always true)Apply this diff:
-import { renderHook } from '@testing-library/react' +import { renderHook, act } from '@testing-library/react' @@ describe('useQueryStore', () => { + afterEach(() => { + vi.restoreAllMocks() + }) @@ - vi.spyOn(rx, 'fromQuery').mockReturnValue(subject.asObservable()) + vi.spyOn(rx, 'fromQuery').mockReturnValueOnce(subject.asObservable()) @@ - const { result } = renderHook(() => useQueryStore({ query: stub<Query>() })) + const { result, unmount } = renderHook(() => useQueryStore({ query: stub<Query>() })) @@ - queryDocumentSnapshot.exists.mockReturnValue(true) + // exists() is always true for QueryDocumentSnapshot; this line can be removed. @@ - subject.next(querySnapshot) + act(() => { + subject.next(querySnapshot) + }) @@ expect(result.current.getState()).toEqual({ @@ }) + unmount()
1-7: Linter “unresolved import” hints mirror the other test fileSame remedies apply: add
@firebase/firestoreas a devDependency, import types fromfirebase/firestore, or configure ESLint resolver for the workspace.
1-1: Filename doesn’t match kebab-case ruleRename to
use-query-store.test.tsor relax the rule in ESLint for test files.packages/react-kitchen-sink/README.md (1)
83-115: Example looks great; optional: prefer stable keys over indexUsing the array index as a React key is fine for docs, but a stable identifier (e.g., document id) is preferable in real apps.
packages/zustand-firestore/package.json (1)
31-39: Move runtime deps to peers (good) and add a small DX tweak for tests
- Moving
rxjsandzustandto peerDependencies is appropriate for a library.- To resolve ESLint/Vitest complaints about type-only imports from
@firebase/firestorein tests, consider adding it as a devDependency as well (it can remain a peer for consumers).Suggested patch:
"devDependencies": { "firebase": "^12.0.0", "react": "^19.1.0", "react-dom": "^19.1.0", "rxjs": "^7.8.2", "zustand": "^5.0.7", "@testing-library/react": "^16.3.0", - "vitest-mock-extended": "^3.1.0" + "vitest-mock-extended": "^3.1.0", + "@firebase/firestore": "^4.7.0" },Also applies to: 40-46, 69-76
packages/react-kitchen-sink/src/rxjs/__tests__/documentSnapshotSubject.test.ts (1)
1-1: Rename test file to kebab-case to satisfy ESLint.Rule unicorn/filename-case suggests document-snapshot-subject.test.ts.
packages/react-kitchen-sink/src/rxjs/__tests__/schemaDocumentSnapshotSubject.test.ts (1)
1-1: Rename test file to kebab-case to satisfy ESLint.Rule unicorn/filename-case suggests schema-document-snapshot-subject.test.ts.
packages/react-kitchen-sink/src/sentry/__tests__/processErrorWithSentryCaptureContext.test.ts (2)
1-1: Import ErrorEvent from @sentry/core to align with implementation and avoid resolver issues.The implementation uses the core Sentry event type; importing from @sentry/core keeps tests consistent and may resolve import-x/no-unresolved warnings.
- import { type ErrorEvent } from '@sentry/react' + import { type ErrorEvent } from '@sentry/core'
1-1: Rename test file to kebab-case to satisfy ESLint.Rule unicorn/filename-case suggests process-error-with-sentry-capture-context.test.ts.
packages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (2)
8-14: Module mock is fine, but add cleanup to prevent cross-test interferenceAdd afterEach to clear and restore mocks so subsequent tests aren’t affected by call counts or module mocks.
vi.mock('@valian/rxjs-firebase', () => ({ fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), QuerySnapshotSubject: class { close = vi.fn() subscribe = vi.fn() }, })) + +afterEach(() => { + vi.clearAllMocks() + vi.restoreAllMocks() +})
26-31: Tighten assertions and ensure type-safe spy return
- The spy return value should satisfy the expected Firestore Query type to avoid TS friction.
- Assert
fromQuerywas called with the prepared query object, not just “called”.- vi.spyOn(collections.users, 'prepare').mockReturnValue({ id: 'q' }) - const subject = schemaQuerySnapshotSubject(collections.users, { name: 'q' }) - expect(collections.users.prepare).toHaveBeenCalled() - expect(fromQuery).toHaveBeenCalled() + const prepared = { id: 'q' } as unknown as import('@firebase/firestore').Query + vi.spyOn(collections.users, 'prepare').mockReturnValue(prepared) + const subject = schemaQuerySnapshotSubject(collections.users, { name: 'q' }) + expect(collections.users.prepare).toHaveBeenCalledWith({ name: 'q' }, undefined) + expect(fromQuery).toHaveBeenCalledWith(prepared, undefined) expect(subject).toBeInstanceOf(QuerySnapshotSubject)Would you like me to add a second test that passes options and a listener to verify they’re forwarded correctly?
packages/react-kitchen-sink/src/react-query/__tests__/querySnapshotQueryOptions.test.ts (2)
18-21: Strengthen assertions: verify full meta and queryFn wiringAdd checks for meta fields and that queryFn is a function.
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.querySnapshotSubjectFactory).toHaveBeenCalledWith(query, snapshotOptions, listener) expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.query).toBe(query) + expect(typeof opts.queryFn).toBe('function')
1-6: Address ESLint: filename case and import resolution
- Rename file to kebab case: query-snapshot-query-options.test.ts.
- If using TS path aliases, ensure eslint resolver supports TS (e.g., eslint-import-resolver-typescript for eslint-plugin-import-x) to avoid false “no-unresolved”.
packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromDocumentSnapshotSubjectFactory.test.ts (5)
3-3: Unify Firestore import path with the rest of the suiteOther tests import types from @firebase/firestore. Align this import to reduce resolver inconsistencies.
-import { type DocumentSnapshot } from 'firebase/firestore' +import { type DocumentSnapshot } from '@firebase/firestore'
8-12: Add test cleanup for isolationClear mocks between tests so setQueryData call counts and spies don’t leak across cases.
import { queryFnFromDocumentSnapshotSubjectFactory } from '../queryFnFromDocumentSnapshotSubjectFactory' class MockDocumentSnapshotSubject<T> extends Subject<DocumentSnapshotState<T>> { close = vi.fn() } + +afterEach(() => { + vi.clearAllMocks() + vi.restoreAllMocks() +})
15-23: Pass queryKey and assert factory invocation to reduce brittle undefined usageMake the subjectFactory call explicit by providing queryKey and asserting it was used.
it('returns loading state when waitForData is false', async () => { - const subject = new MockDocumentSnapshotSubject() - const queryFn = queryFnFromDocumentSnapshotSubjectFactory(vi.fn().mockReturnValue(subject)) + const subject = new MockDocumentSnapshotSubject() + const factory = vi.fn().mockReturnValue(subject) + const queryFn = queryFnFromDocumentSnapshotSubjectFactory(factory) const controller = new AbortController() const queryClient = mock<QueryClient>() - const result = await queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const queryKey = ['k'] + const result = await queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey }), + ) expect(result).toEqual({ isLoading: true, hasError: false, disabled: false }) expect(subject.close).not.toHaveBeenCalled() + expect(factory).toHaveBeenCalledWith(queryKey) })
25-34: Abort test: include queryKey and assert factory call for consistencyMake this test consistent with the one above.
it('closes subject on abort', async () => { - const subject = new MockDocumentSnapshotSubject() - const queryFn = queryFnFromDocumentSnapshotSubjectFactory(vi.fn().mockReturnValue(subject)) + const subject = new MockDocumentSnapshotSubject() + const factory = vi.fn().mockReturnValue(subject) + const queryFn = queryFnFromDocumentSnapshotSubjectFactory(factory) const controller = new AbortController() const queryClient = mock<QueryClient>() - const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const queryKey = ['k'] + const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey })) controller.abort() await promise expect(subject.close).toHaveBeenCalled() + expect(factory).toHaveBeenCalledWith(queryKey) })
1-7: Address ESLint: filename case and import resolution
- Rename file to kebab case: query-fn-from-document-snapshot-subject-factory.test.ts.
- Ensure resolver config supports TS and monorepo packages to eliminate “no-unresolved”.
packages/react-kitchen-sink/src/react-query/__tests__/documentSnapshotQueryOptions.test.ts (2)
18-21: Enhance assertions on meta and queryFnVerify meta fields and function shape to better lock the contract.
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.documentSnapshotSubjectFactory).toHaveBeenCalledWith(ref, snapshotOptions, listener) expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.documentRef).toBe(ref) + expect(typeof opts.queryFn).toBe('function')
1-6: Address ESLint: filename case and import resolution
- Rename file to kebab case: document-snapshot-query-options.test.ts.
- Confirm deps and eslint TS resolver setup to avoid false unresolved import errors.
packages/react-kitchen-sink/src/react-query/__tests__/schemaQuerySnapshotQueryOptions.test.ts (2)
33-41: Assert full meta content for schema-based optionsAlso check that snapshotManager, collection name, and schemaQuery are reflected in meta.
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.schemaQuerySnapshotSubjectFactory).toHaveBeenCalledWith( collections.users, { name: 'q' }, snapshotOptions, listener, ) expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.collection).toBe(collections.users.collectionName) + expect(opts.meta?.schemaQuery).toEqual({ name: 'q' }) + expect(typeof opts.queryFn).toBe('function')
1-8: Address ESLint: filename case and import resolution
- Rename file to kebab case: schema-query-snapshot-query-options.test.ts.
- Ensure zod, zod-firebase and @valian/rxjs-firebase are declared in package.json and that your ESLint resolver supports TS.
packages/react-kitchen-sink/src/rxjs/documentSnapshotSubject.ts (1)
10-14: Optional: add explicit return type for clarity and consistencyType inference works, but adding the explicit return type improves readability and keeps this factory in sync with the explicit generics used on construction.
Apply this minimal diff:
-) => +): DocumentSnapshotSubject<AppModelType, DbModelType> => new DocumentSnapshotSubject<AppModelType, DbModelType>( fromDocumentRef(ref, options), sentryDocumentSnapshotListener(ref, listener), )packages/react-kitchen-sink/src/sentry/__tests__/ErrorWithSentryCaptureContext.test.ts (2)
1-1: Fix lint: filename case and Sentry type import resolution
- unicorn/filename-case: rename the file to kebab-case.
- import-x/no-unresolved: prefer importing ErrorEvent from @sentry/types to avoid depending on @sentry/core in tests.
Suggested changes:
- Rename file: ErrorWithSentryCaptureContext.test.ts → error-with-sentry-capture-context.test.ts
- Update the import:
-import { type ErrorEvent } from '@sentry/core' +import { type ErrorEvent } from '@sentry/types'If you prefer to keep @sentry/core, ensure the package is listed in this package’s devDependencies to satisfy import linting.
3-3: Resolve import for vitest-mock-extendedESLint flags import-x/no-unresolved. If the workspace relies on hoisted deps, add vitest-mock-extended to this package’s devDependencies or configure your lint rules for monorepo resolution.
packages/react-kitchen-sink/src/hooks/useSchemaQueryStore.ts (2)
27-27: Consider addingmetaOptionsandfactoryto useObservable depsCurrently, the observable won’t re-evaluate when metaOptions or factory change, which could yield stale queries. Add them to the dependency array on Line 62 to reflect all inputs used to compute the stream.
Recommended update (Line 62):
- Before: [options.query, snapshotListenOptions]
- After: [options.query, snapshotListenOptions, metaOptions, factory]
15-16: Resolve lint: module resolution for external depsStatic analysis flags unresolved imports for zod-firebase and zustand. If your monorepo rules require local package deps, add these to this package’s dependencies/devDependencies or adjust eslint-plugin-import settings for workspaces.
packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromQuerySnapshotSubjectFactory.test.ts (2)
43-101: Optional: use fake timers to reduce flakiness in “waitForData” testThe test relies on setTimeout(…, 0). Using vi.useFakeTimers() and advancing timers can make this fully deterministic and remove timing sensitivities on slower CI runners.
Example pattern (no diff, for illustration):
- vi.useFakeTimers()
- schedule subject.next(...)
- vi.runAllTimers() (or vi.advanceTimersByTime(timeout))
- await promise
1-1: Fix lint: filename case and unresolved imports
- unicorn/filename-case: rename file to kebab-case.
- import-x/no-unresolved: ensure the imported packages are available to this package (workspace or local devDependencies): @firebase/firestore, @tanstack/react-query, @valian/rxjs-firebase, rxjs, vitest-mock-extended.
Rename suggestion:
- queryFnFromQuerySnapshotSubjectFactory.test.ts → query-fn-from-query-snapshot-subject-factory.test.ts
packages/react-kitchen-sink/src/rxjs/schemaQuerySnapshotSubject.ts (2)
19-22: Return type and listener typing are solid; consider explicit generics for symmetryThe function’s return type is the new SchemaQuerySnapshotSubject alias, which is correct. For symmetry with documentSnapshotSubject, you could pass explicit generics to QuerySnapshotSubject, though inference here is already accurate.
No change required; optional style tweak only.
8-8: Resolve lint: zod-firebase importESLint flagged import-x/no-unresolved. Ensure zod-firebase is declared in this package’s dependencies or adjust the monorepo import resolver settings.
packages/react-kitchen-sink/src/react-query/__tests__/FirestoreSnaphotManager.test.ts (2)
10-21: Replace class-based mocks with constructor-function stubs to satisfy max-classes-per-file and keep tests leanUsing two inline classes triggers the ESLint max-classes-per-file rule. Constructor-function stubs provide the same behavior without introducing classes.
Apply this diff:
-vi.mock('@valian/rxjs-firebase', () => ({ - fromDocumentRef: vi.fn().mockReturnValue({ subscribe: vi.fn() }), - fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), - DocumentSnapshotSubject: class { - close = vi.fn() - subscribe = vi.fn() - }, - QuerySnapshotSubject: class { - close = vi.fn() - subscribe = vi.fn() - }, -})) +vi.mock('@valian/rxjs-firebase', () => { + const DocumentSnapshotSubject = vi.fn(function () { + // @ts-expect-error - dynamic test doubles + this.close = vi.fn() + // @ts-expect-error - dynamic test doubles + this.subscribe = vi.fn() + }) + const QuerySnapshotSubject = vi.fn(function () { + // @ts-expect-error - dynamic test doubles + this.close = vi.fn() + // @ts-expect-error - dynamic test doubles + this.subscribe = vi.fn() + }) + return { + fromDocumentRef: vi.fn().mockReturnValue({ subscribe: vi.fn() }), + fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), + DocumentSnapshotSubject, + QuerySnapshotSubject, + } +})
1-1: Consider kebab-case filename and fixing the “Snaphot” typoESLint suggests kebab-case for filenames. Also, “Snaphot” appears to be a typo of “Snapshot” across files. Consider renaming to keep consistency and discoverability (e.g., firestore-snapshot-manager.test.ts and corresponding source).
packages/react-kitchen-sink/src/rxjs/schemaDocumentSnapshotSubject.ts (2)
8-10: Default TOptions to MetaOutputOptions for ergonomicsAdding a default type makes the API easier to consume when callers don’t need to specialize options.
export const schemaDocumentSnapshotSubject = < TCollectionSchema extends CollectionSchema, - TOptions extends MetaOutputOptions, + TOptions extends MetaOutputOptions = MetaOutputOptions, >(
1-1: Consider kebab-case filename to satisfy ESLintRule unicorn/filename-case suggests schema-document-snapshot-subject.ts.
packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromQuerySnapshotSubjectFactory.ts (2)
26-33: Optional: prefer timeout() operator over takeUntil(timer()) for explicit timeout errorstimeout() produces a TimeoutError instead of completing without emission, which can be easier to handle and reason about in React Query.
Apply this diff and adjust imports:
-import { firstValueFrom, skipWhile, takeUntil, timer } from 'rxjs' +import { firstValueFrom, skipWhile, timeout } from 'rxjs' @@ - return firstValueFrom( - subject$.pipe( - takeUntil(timer(options.waitForDataTimeout ?? 10_000)), - skipWhile(({ isLoading }) => isLoading), - ), - ) + return firstValueFrom( + subject$.pipe( + timeout(options.waitForDataTimeout ?? 10_000), + skipWhile(({ isLoading }) => isLoading), + ), + )
1-1: Consider kebab-case filename to satisfy ESLintSuggested: query-fn-from-query-snapshot-subject-factory.ts.
packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromDocumentSnapshotSubjectFactory.ts (2)
26-33: Optional: prefer timeout() operator over takeUntil(timer())Ensures a clear TimeoutError is surfaced to React Query instead of an EmptyError due to completion without emission.
Apply this diff and adjust imports:
-import { firstValueFrom, skipWhile, takeUntil, timer } from 'rxjs' +import { firstValueFrom, skipWhile, timeout } from 'rxjs' @@ - return firstValueFrom( - subject$.pipe( - takeUntil(timer(options.waitForDataTimeout ?? 10_000)), - skipWhile(({ isLoading }) => isLoading), - ), - ) + return firstValueFrom( + subject$.pipe( + timeout(options.waitForDataTimeout ?? 10_000), + skipWhile(({ isLoading }) => isLoading), + ), + )
1-1: Consider kebab-case filename to satisfy ESLintSuggested: query-fn-from-document-snapshot-subject-factory.ts.
packages/react-kitchen-sink/src/react-query/FirestoreSnaphotManager.ts (3)
45-48: Don’t throw on missing subscription; log and continueThrowing here will crash the app during cache GC if metadata is off or cleanup was already invoked. A warn + bail-out is safer in production.
Apply this diff:
- const closeSnapshot = this.#onClose.get(snapshotId) - if (!closeSnapshot) { - throw new Error(`Subscription for key ${snapshotId} not found`) - } + const closeSnapshot = this.#onClose.get(snapshotId) + if (!closeSnapshot) { + // Non-fatal: log and skip to avoid crashing during cache cleanup + console.warn(`[FirestoreSnapshotManager] Subscription for key ${snapshotId} not found`) + return + }
36-36: Typo in public class name: “Snaphot” → “Snapshot”Public API types should avoid typos. Consider renaming the class (and file) to FirestoreSnapshotManager before this ships broadly.
If you opt to rename, update references across the repo (imports and types). I can generate a codemod for the monorepo if helpful.
39-54: Consider exposing a dispose() to unsubscribe the QueryCache listenerIf the manager has a defined lifecycle (e.g., per app), keeping the subscription forever may be undesirable. Store the unsubscribe handle and provide dispose().
Example addition outside the selected range:
// store unsubscribe in a private field and expose dispose() private #unsubscribe?: () => void constructor(queryClient: QueryClient) { this.#unsubscribe = queryClient.getQueryCache().subscribe((event: QueryCacheNotifyEvent) => { // ... }) } dispose() { this.#unsubscribe?.() this.#unsubscribe = undefined }packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (3)
52-67: Pass only waitForData options to the factory; avoid leaking unrelated propsPassing the entire props object to queryFnFromQuerySnapshotSubjectFactory is unnecessary; only waitForData and waitForDataTimeout are consumed.
Apply this diff:
- }: QuerySnapshotQueryOptions<AppModelType, DbModelType, TError, TData, TQueryKey>, + }: QuerySnapshotQueryOptions<AppModelType, DbModelType, TError, TData, TQueryKey>, ): QuerySnapshotQueryOptionsResult<AppModelType, DbModelType, TError, TData, TQueryKey> => queryOptions({ queryFn: queryFnFromQuerySnapshotSubjectFactory( - snapshotManager.querySnapshotSubjectFactory(query, snapshotOptions, listener), - props, + snapshotManager.querySnapshotSubjectFactory(query, snapshotOptions, listener), + { waitForData: props.waitForData, waitForDataTimeout: props.waitForDataTimeout }, ),Also applies to: 68-71
72-77: Drop redundant initialData: undefinedinitialData is already omitted from the public options type. Setting it to undefined is redundant.
Apply this diff:
staleTime: Infinity, retry: false, gcTime: 10_000, - initialData: undefined, ...props,
15-15: Follow-up if you rename the manager classIf you fix the “Snaphot” typo in the class/file, remember to update this import:
Apply this diff after the rename:
-import { type FirestoreSnaphotManager } from './FirestoreSnaphotManager' +import { type FirestoreSnapshotManager } from './FirestoreSnapshotManager'packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (2)
52-66: Pass only waitForData options to the factoryLimit the second argument to queryFnFromDocumentSnapshotSubjectFactory to its actual option surface.
Apply this diff:
- }: DocumentSnapshotQueryOptions<AppModelType, DbModelType, TError, TData, TQueryKey>, + }: DocumentSnapshotQueryOptions<AppModelType, DbModelType, TError, TData, TQueryKey>, ): DocumentSnapshotQueryOptionsResult<AppModelType, DbModelType, TError, TData, TQueryKey> => queryOptions({ queryFn: queryFnFromDocumentSnapshotSubjectFactory( - snapshotManager.documentSnapshotSubjectFactory(ref, snapshotOptions, listener), - props, + snapshotManager.documentSnapshotSubjectFactory(ref, snapshotOptions, listener), + { waitForData: props.waitForData, waitForDataTimeout: props.waitForDataTimeout }, ),Also applies to: 67-71
15-15: Follow-up if you rename the manager classUpdate the import when fixing the manager class/file typo.
Apply this diff after the rename:
-import { type FirestoreSnaphotManager } from './FirestoreSnaphotManager' +import { type FirestoreSnapshotManager } from './FirestoreSnapshotManager'packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (1)
60-75: Pass only waitForData options to the factoryAvoid passing the entire props as factory options; only waitForData fields are used.
Apply this diff:
- }: SchemaQuerySnapshotQueryOptions<TCollectionSchema, TOptions, TError, TData, TQueryKey>, + }: SchemaQuerySnapshotQueryOptions<TCollectionSchema, TOptions, TError, TData, TQueryKey>, ): SchemaQuerySnapshotQueryOptionsResult<TCollectionSchema, TOptions, TError, TData, TQueryKey> => queryOptions({ queryFn: queryFnFromQuerySnapshotSubjectFactory( - snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), - props, + snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), + { waitForData: props.waitForData, waitForDataTimeout: props.waitForDataTimeout }, ),Also applies to: 76-81
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
packages/react-kitchen-sink/src/schema/__tests__/ZodSchemaError.test.ts (3)
1-1: Import Firestore type from the public moduleUse the public Firestore entrypoint to avoid internal package coupling and resolver errors.
-import { type QueryDocumentSnapshot } from '@firebase/firestore' +import { type QueryDocumentSnapshot } from 'firebase/firestore'
2-2: Use the correct Sentry Event type; fix unresolved import and typing
@sentry/reactdoesn’t export an ErrorEvent suited for beforeSend; useEventfrom@sentry/types.-import { type ErrorEvent } from '@sentry/react' +import { type Event as SentryEvent } from '@sentry/types'- const event = error.beforeSend(mock<ErrorEvent>()) + const event = error.beforeSend(mock<SentryEvent>())Also applies to: 19-19
1-1: Rename test file to kebab-case to satisfy lint ruleESLint (unicorn/filename-case) requires kebab-case for filenames. Rename:
git mv packages/react-kitchen-sink/src/schema/__tests__/ZodSchemaError.test.ts \ packages/react-kitchen-sink/src/schema/__tests__/zod-schema-error.test.tspackages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (1)
1-4: Fix ESLint findings: rename file to kebab-case and configure import resolver
- Rename file to: packages/react-kitchen-sink/src/rxjs/tests/schema-query-snapshot-subject.test.ts.
- Configure ESLint’s import resolver for your monorepo to resolve @valian/rxjs-firebase, zod, and zod-firebase. With eslint-plugin-import-x, use the typescript and node resolvers.
Example .eslintrc settings (adjust paths to your tsconfigs):
settings: { 'import-x/resolver': { typescript: { alwaysTryTypes: true, project: ['tsconfig.base.json', 'packages/*/tsconfig.json'], }, node: { extensions: ['.ts', '.tsx', '.js', '.jsx'], }, }, }Run this script to verify the rename and resolver settings are present:
#!/bin/bash set -euo pipefail echo "Check renamed test file" if [ -f packages/react-kitchen-sink/src/rxjs/__tests__/schema-query-snapshot-subject.test.ts ]; then echo "OK: kebab-case test file exists" else echo "MISSING: packages/react-kitchen-sink/src/rxjs/__tests__/schema-query-snapshot-subject.test.ts" fi echo echo "Scan ESLint config(s) for import(-x) resolver setup" fd -HI '(\.eslintrc.*|eslint\.config\.(js|cjs|ts|mjs))' -t f \ --exec bash -lc 'echo "---- {} ----"; rg -n -C2 "(import(-x)?/resolver|typescript.*resolver)" "{}" || true'packages/react-kitchen-sink/src/react-query/observers/__tests__/querySnapshotQueryClientObserver.test.ts (1)
1-4: Fix ESLint “unresolved import” warnings by ensuring deps/resolver are set (duplicate of prior review).The unresolved import errors for @tanstack/react-query, @valian/rxjs-firebase, and vitest-mock-extended match the earlier review. Please ensure:
- @valian/rxjs-firebase is added to devDependencies for this package.
- ESLint import resolver is configured for workspaces/TS.
See the previous comment and script already provided; same guidance applies here.
packages/react-kitchen-sink/src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts (1)
1-1: Import QuerySnapshot type from the public SDKThis mirrors prior feedback: prefer 'firebase/firestore' over '@firebase/firestore' for stability and to satisfy no-unresolved.
Apply this diff:
-import { type QuerySnapshot } from '@firebase/firestore' +import type { QuerySnapshot } from 'firebase/firestore'packages/react-kitchen-sink/src/schema/__tests__/firestoreDataConverter.test.ts (1)
1-2: Do not import from '@firebase/firestore'; use the public 'firebase/firestore' entry.Importing from the internal package can break types/bundling. Consolidate on the public entry.
-import { type QueryDocumentSnapshot } from '@firebase/firestore' -import { GeoPoint, Timestamp } from 'firebase/firestore' +import { type QueryDocumentSnapshot, GeoPoint, Timestamp } from 'firebase/firestore'packages/react-kitchen-sink/README.md (1)
51-51: Remove unused QueryClient import from example.It’s not used in the snippet.
-import { QueryClient, useQueryClient, useSuspenseQuery } from '@tanstack/react-query' +import { useQueryClient, useSuspenseQuery } from '@tanstack/react-query'packages/react-kitchen-sink/src/rxjs/__tests__/schemaDocumentSnapshotSubject.test.ts (1)
1-1: Fix spy return type: doc() returns DocumentReference, and assert fromDocumentRef argsFactory.read.doc returns a DocumentReference, not a DocumentSnapshot. Update the spy and strengthen the assertion to validate the exact args passed to fromDocumentRef.
Apply:
- import { type DocumentSnapshot } from '@firebase/firestore' + import { type DocumentReference } from '@firebase/firestore' @@ - const documentSnapshot = mock<DocumentSnapshot>() - vi.spyOn(collections.users.read, 'doc').mockReturnValue(documentSnapshot) + const documentRef = mock<DocumentReference>() + vi.spyOn(collections.users.read, 'doc').mockReturnValue(documentRef) @@ - expect(fromDocumentRef).toHaveBeenCalled() + expect(fromDocumentRef).toHaveBeenCalledWith(documentRef, undefined)Also applies to: 28-35, 33-33
🧹 Nitpick comments (43)
packages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (3)
8-14: Make mocks verifiable: use a mock constructor and track callsYour current class-based mock works for instanceof, but you can’t assert constructor calls/args. Switching to a mock constructor preserves instanceof and enables call assertions.
Apply this diff:
-vi.mock('@valian/rxjs-firebase', () => ({ - fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), - QuerySnapshotSubject: class { - close = vi.fn() - subscribe = vi.fn() - }, -})) +vi.mock('@valian/rxjs-firebase', () => { + const fromQuery = vi.fn().mockReturnValue({ subscribe: vi.fn() }) + const QuerySnapshotSubject = vi.fn(function (this: any) { + this.close = vi.fn() + this.subscribe = vi.fn() + }) + return { fromQuery, QuerySnapshotSubject } +})
25-31: Strengthen assertions: verify options pass-through and constructor invocationAssert the arguments passed to prepare and fromQuery, and that QuerySnapshotSubject is constructed. Also pass options to validate the pass-through.
it('creates a QuerySnapshotSubject from factory.prepare', () => { - vi.spyOn(collections.users, 'prepare').mockReturnValue({ id: 'q' }) - const subject = schemaQuerySnapshotSubject(collections.users, { name: 'q' }) - expect(collections.users.prepare).toHaveBeenCalled() - expect(fromQuery).toHaveBeenCalled() - expect(subject).toBeInstanceOf(QuerySnapshotSubject) + vi.spyOn(collections.users, 'prepare').mockReturnValue({ id: 'q' } as any) + const options = { includeMetadataChanges: true } as const + const subject = schemaQuerySnapshotSubject(collections.users, { name: 'q' }, options) + expect(collections.users.prepare).toHaveBeenCalledWith({ name: 'q' }, options) + expect(fromQuery).toHaveBeenCalledWith({ id: 'q' }, options) + expect(QuerySnapshotSubject).toHaveBeenCalledTimes(1) + expect(subject).toBeInstanceOf(QuerySnapshotSubject) })
2-2: Test hygiene: restore mocks between testsRestore spies/mocks to avoid cross-test leakage.
-import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it, vi, afterEach } from 'vitest' @@ +afterEach(() => { + vi.restoreAllMocks() +})Also applies to: 15-15
packages/react-kitchen-sink/src/react-query/observers/__tests__/querySnapshotQueryClientObserver.test.ts (6)
1-1: Rename file to kebab-case to satisfy ESLint unicorn/filename-case.Rename to: query-snapshot-query-client-observer.test.ts
8-8: Optional: Place vi.mock before importing the SUT to avoid hoist surprises.Vitest hoists vi.mock, so this works. For clarity and to avoid confusion if hoist behavior changes, consider moving the mock above the import of querySnapshotQueryClientObserver.
12-15: Mocking QueryClient is fine; consider mockDeep for robustness.Current approach works. If you want stronger type guarantees for class methods and future calls, use mockDeep from vitest-mock-extended.
Apply this diff:
-import { mock } from 'vitest-mock-extended' +import { mockDeep } from 'vitest-mock-extended' ... - const client = mock<QueryClient>({ setQueryData: vi.fn() }) + const client = mockDeep<QueryClient>()
16-25: Strengthen the “next” assertion by verifying the exact payload.Asserting only “was called” can miss shape regressions. Capture the object and assert with the same reference.
Apply this diff:
- observer.next({ - empty: false, - size: 1, - isLoading: false, - hasError: false, - disabled: false, - data: [{}], - } as QuerySnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + const snapshotState: QuerySnapshotState = { + empty: false, + size: 1, + isLoading: false, + hasError: false, + disabled: false, + data: [{}], + } + observer.next(snapshotState) + expect(client.setQueryData).toHaveBeenCalledWith(queryKey, snapshotState)
26-35: Also assert Sentry captureException is called on error.This test already verifies the cache update on error. Also check that Sentry receives the error.
Apply this diff:
- observer.error(new Error('x')) + const err = new Error('x') + observer.error(err) expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: true, disabled: false, data: [], }) + expect(captureException).toHaveBeenCalledWith(err)Additionally add this import at the top of the file:
import { captureException } from '@sentry/react'
36-45: Optional: Assert the total number of cache writes to guard call count.This makes the test resilient to accidental extra updates.
Apply this diff:
expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], }) + expect(client.setQueryData).toHaveBeenCalledTimes(3)packages/react-kitchen-sink/src/sentry/__tests__/processErrorWithSentryCaptureContext.test.ts (1)
18-20: Strengthen assertion: verify parent beforeSend runs before causeGiven the recursion order, root error.beforeSend should run before cause.beforeSend. This makes the behavior explicit and guards against regressions.
Apply this diff:
expect(spy).toHaveBeenCalledWith(event) expect(causeSpy).toHaveBeenCalledWith(event) + // Ensure the root error's beforeSend runs before the cause's beforeSend + expect(spy.mock.invocationCallOrder[0]).toBeLessThan(causeSpy.mock.invocationCallOrder[0])packages/react-kitchen-sink/src/sentry/__tests__/sentryDocumentSnapshotListener.test.ts (1)
22-41: Assert breadcrumb and captureException payloads for stronger guaranteesVerify the breadcrumb messages and error context/fingerprints/tags to ensure Sentry integration is wired as designed.
Apply this diff:
- listener.onSnapshot?.({ - isLoading: false, - hasError: false, - disabled: false, - snapshot: stub<DocumentSnapshot>(), - exists: true, - data: {}, - } as const) - expect(onSnapshot).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + const state = { + isLoading: false, + hasError: false, + disabled: false, + snapshot: stub<DocumentSnapshot>(), + exists: true, + data: {}, + } as const + listener.onSnapshot?.(state) + expect(onSnapshot).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'received document snapshot for (/c/id)', + data: expect.objectContaining({ state }), + }), + ) @@ - listener.onError?.(error) - expect(onError).toHaveBeenCalledWith(error) - expect(captureException).toHaveBeenCalled() + listener.onError?.(error) + expect(onError).toHaveBeenCalledWith(error) + expect(captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: expect.objectContaining({ firestore: true, 'schema.collection': 'c' }), + fingerprint: ['firestore', 'c'], + contexts: expect.objectContaining({ + snapshot: expect.objectContaining({ ref }), + }), + }), + ) @@ - listener.onComplete?.() - expect(onComplete).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + listener.onComplete?.() + expect(onComplete).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'closed document snapshot for (/c/id)', + }), + )packages/react-kitchen-sink/src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts (1)
23-43: Assert breadcrumb messages and captureException contextStrengthen the test by validating the exact breadcrumb messages and the Sentry context, tags, and fingerprint.
Apply this diff:
- listener.onSnapshot?.({ - isLoading: false, - hasError: false, - disabled: false, - empty: true, - size: 0, - data: [], - snapshot: stub<QuerySnapshot>(), - } as const) - expect(onSnapshot).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + const state = { + isLoading: false, + hasError: false, + disabled: false, + empty: true, + size: 0, + data: [], + snapshot: stub<QuerySnapshot>(), + } as const + listener.onSnapshot?.(state) + expect(onSnapshot).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'received query snapshot for (q)', + data: expect.objectContaining({ state }), + }), + ) @@ - listener.onError?.(error) - expect(onError).toHaveBeenCalledWith(error) - expect(captureException).toHaveBeenCalled() + listener.onError?.(error) + expect(onError).toHaveBeenCalledWith(error) + expect(captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: expect.objectContaining({ firestore: true, 'schema.collection': 'c' }), + fingerprint: ['firestore', 'c'], + contexts: expect.objectContaining({ + query: expect.objectContaining({ query }), + }), + }), + ) @@ - listener.onComplete?.() - expect(onComplete).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + listener.onComplete?.() + expect(onComplete).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'closed query snapshot for (q)', + }), + )packages/react-kitchen-sink/src/sentry/__tests__/handleNotAwaitedPromise.test.ts (2)
32-33: Suppress Biome false positive for crafted thenableBiome flags adding a 'then' property, but here it's intentional to validate the guard. Add a biome-ignore to keep lint clean.
Apply this diff:
it('does nothing when object has no catch', () => { - // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function + // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function + /* biome-ignore lint/suspicious/noThenProperty: intentionally constructing non-promise thenable to test guard */ handleNotAwaitedPromise({ then: () => {} } as unknown as Promise<void>) expect(captureException).not.toHaveBeenCalled() })
15-22: Optional: use microtask flush instead of setTimeout for stabilityReplacing the zero-timeout with a microtask flush makes the test slightly faster and less flaky.
Apply this diff:
- await new Promise((resolve) => { - setTimeout(resolve, 0) - }) + // Flush microtasks so the rejection handler runs + await Promise.resolve()packages/react-kitchen-sink/src/react-query/observers/__tests__/documentSnapshotQueryClientObserver.test.ts (4)
12-18: Assert the exact payload on next() rather than just "called".Verifying the precise state written to the cache makes this test more robust.
Apply this diff:
- observer.next({ isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + const state = { isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState + observer.next(state) + expect(client.setQueryData).toHaveBeenCalledWith(queryKey, state)
8-8: Import captureException to assert it’s invoked on error.You already mock @sentry/react. Import captureException so the test can assert it was called with the thrown error.
+import { captureException } from '@sentry/react' vi.mock('@sentry/react', () => ({ captureException: vi.fn() }))
19-25: Also assert Sentry is called with the error.This closes the loop on the observer’s error path.
- observer.error(new Error('x')) + const err = new Error('x') + observer.error(err) + expect(captureException).toHaveBeenCalledWith(err) expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { isLoading: false, hasError: true, disabled: false, })
1-1: Rename file to kebab-case to satisfy ESLint.ESLint unicorn/filename-case suggests: document-snapshot-query-client-observer.test.ts
packages/react-kitchen-sink/src/schema/firestoreDataConverter.ts (2)
15-16: Use a “plain object” guard to avoid transforming class instances.value instanceof Object matches many class instances. Restrict recursion to plain objects to avoid unintended transformations (e.g., DocumentReference, custom classes).
- case value instanceof Object: + case typeof value === 'object' && value !== null && value.constructor === Object: return Object.fromEntries(Object.entries(value).map(([key, v]) => [key, handleFirestoreDataTypes(v)])) as T
1-1: Rename file to kebab-case to satisfy ESLint.ESLint unicorn/filename-case suggests: firestore-data-converter.ts
packages/react-kitchen-sink/src/schema/__tests__/firestoreDataConverter.test.ts (2)
27-29: Optionally assert snapshot.data() was called once.This tightens the contract of the converter relying on snapshot.data().
const out = firestoreDataConverter(snapshot) + expect(snapshot.data).toHaveBeenCalledTimes(1) expect(out.date).toBe(now)
1-1: Rename file to kebab-case to satisfy ESLint.ESLint unicorn/filename-case suggests: firestore-data-converter.test.ts
packages/react-kitchen-sink/README.md (1)
56-59: Memoize FirestoreSnapshotManager to avoid re-instantiating on each render.Creating a new manager per render is unnecessary; memoize it against the client.
+import { useMemo } from 'react' @@ const client = useQueryClient() - const manager = new FirestoreSnapshotManager(client) + const manager = useMemo(() => new FirestoreSnapshotManager(client), [client])packages/react-kitchen-sink/src/rxjs/__tests__/documentSnapshotSubject.test.ts (1)
1-1: Rename file to kebab-case to satisfy ESLint.ESLint unicorn/filename-case suggests: document-snapshot-subject.test.ts
packages/react-kitchen-sink/src/rxjs/__tests__/schemaDocumentSnapshotSubject.test.ts (1)
1-6: Address ESLint errors: filename case and unresolved imports
- Rename file to schema-document-snapshot-subject.test.ts (unicorn/filename-case).
- Ensure ESLint import resolver knows the monorepo/TS project references, or add missing deps in the relevant package.
Consider adding (or verifying) this in your ESLint config if you're using eslint-plugin-import-x:
- settings:
- import-x/resolver:
- typescript:
- project: true
Alternatively, disable import resolution for test files if it’s a false positive in your workspace setup.
packages/react-kitchen-sink/src/react-query/__tests__/querySnapshotQueryOptions.test.ts (2)
18-21: Tighten assertions: validate meta fields and default optionsVerify we carry through metadata and the expected defaults (staleTime/gcTime/retry).
Apply:
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.querySnapshotSubjectFactory).toHaveBeenCalledWith(query, snapshotOptions, listener) expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.query).toBe(query) + expect(opts.retry).toBe(false) + expect(opts.staleTime).toBe(Infinity) + expect(opts.gcTime).toBe(10_000)
1-6: Fix ESLint issues: filename case and import resolution
- Rename file to query-snapshot-query-options.test.ts.
- Configure import resolver for the monorepo/TS project or ensure packages are resolvable in this workspace.
If needed, add an import-x typescript resolver configuration similar to the one suggested in the other test, or relax the rule for test files if these are false positives.
packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromDocumentSnapshotSubjectFactory.test.ts (3)
27-35: Pass a queryKey in the loading-state test to avoid undefined key usagequeryFn subscribes with a queryClient observer bound to the queryKey; passing one keeps the test semantically correct and future-proof.
Apply:
- const result = await queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const result = await queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] }), + )
48-76: Make timing deterministic with fake timersThese tests rely on setTimeout(0). Using vi.useFakeTimers() + vi.runAllTimers() reduces flakiness and speeds up the suite.
Example refactor within this test:
it('waits for non-loading snapshot when waitForData is true', async () => { @@ - const controller = new AbortController() + const controller = new AbortController() + vi.useFakeTimers() @@ - setTimeout(() => { + setTimeout(() => { subject.next({ isLoading: false, hasError: false, disabled: false, exists: true, data: {}, snapshot, }) }, 0) - const result = await promise + vi.runAllTimers() + const result = await promiseYou can apply the same pattern in the "default timeout" test below.
1-6: ESLint issues: filename case and unresolved imports
- Rename to query-fn-from-document-snapshot-subject-factory.test.ts.
- Ensure rxjs and other imports resolve in this package or configure the import resolver.
Adopt the same ESLint resolver settings used elsewhere in tests if these are workspace-related false positives.
packages/react-kitchen-sink/src/react-query/__tests__/documentSnapshotQueryOptions.test.ts (2)
18-21: Expand assertions to cover meta fields and defaultsCheck that meta carries the ref and snapshotManager, and defaults are applied.
Apply:
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.documentSnapshotSubjectFactory).toHaveBeenCalledWith(ref, snapshotOptions, listener) expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.documentRef).toBe(ref) + expect(opts.retry).toBe(false) + expect(opts.staleTime).toBe(Infinity) + expect(opts.gcTime).toBe(10_000)
1-6: Resolve ESLint issues: filename and import resolution
- Rename to document-snapshot-query-options.test.ts.
- Ensure the ESLint import resolver and dependencies are configured for this workspace.
Use a shared ESLint config for tests that sets the import-x typescript resolver to project: true or relax import resolution for test files if needed.
packages/react-kitchen-sink/src/react-query/__tests__/schemaQuerySnapshotQueryOptions.test.ts (2)
1-7: Address filename case and resolver for monorepo paths
- Rename the file to kebab-case to satisfy the rule.
- The import-x/no-unresolved errors are often due to missing TS path resolver config in ESLint for monorepos. Ensure eslint settings include a TS resolver pointing at your project tsconfig(s).
Example (eslint config):
- settings.import-x/resolver.typescript.project should include workspace tsconfigs (e.g., ["tsconfig.base.json", "packages/*/tsconfig.json"]).
- File rename: schema-query-snapshot-query-options.test.ts
20-41: Strengthen assertions: check queryFn presence and meta completenessGood coverage of the factory call and meta.type. Consider asserting the presence of queryFn and the full meta payload to prevent regressions.
Apply this diff to add assertions:
it('builds query options with meta and queryFn from manager factory', () => { @@ }) - expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) + expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) + expect(typeof opts.queryFn).toBe('function') @@ - expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta).toMatchObject({ + snapshotManager, + collection: collections.users.collectionName, + schemaQuery: { name: 'q' }, + }) })packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromQuerySnapshotSubjectFactory.test.ts (3)
1-7: Fix filename case and resolver warnings for tests
- Rename the file to kebab case to satisfy unicorn/filename-case.
- Configure ESLint’s TS resolver to avoid import-x/no-unresolved for workspace packages.
Rename to: query-fn-from-query-snapshot-subject-factory.test.ts
ESLint settings suggestion:
- settings.import-x/resolver.typescript.project: include your workspace tsconfigs.
51-60: Stabilize abort-close assertion to avoid race flakinessSince the queryFn returns synchronously (waitForData=false), abort and the close callback may be processed on the next tick. Add a small tick/yield before the assertion to prevent flaky failures.
Apply this diff:
const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) controller.abort() - await promise - expect(subject.close).toHaveBeenCalled() + await promise + // yield to allow abort handler to run + await new Promise((r) => setTimeout(r, 0)) + expect(subject.close).toHaveBeenCalled()
62-120: Add coverage for waitForData + early abort (cancellation semantics)Currently, abort during waitForData relies on the timeout to settle. Add a test ensuring the Promise settles on abort without waiting for the timeout (once implementation supports it).
Proposed test to add:
it('settles promptly when aborted while waiting for data', async () => { const subject = new MockQuerySnapshotSubject() const queryFn = queryFnFromQuerySnapshotSubjectFactory(vi.fn().mockReturnValue(subject), { waitForData: true, waitForDataTimeout: 10_000, }) const controller = new AbortController() const queryClient = mock<QueryClient>() const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] })) controller.abort() await expect(promise).rejects.toThrow() })If you want, I can wire the abort signal into the Rx pipeline in the implementation to make this pass.
packages/react-kitchen-sink/src/react-query/__tests__/FirestoreSnapshotManager.test.ts (1)
1-1: Rename test file to kebab case and consider relaxing max-classes-per-file for tests
- Rename to firestore-snapshot-manager.test.ts.
- max-classes-per-file is triggered by inline mock classes. Either:
- disable the rule for this file, or
- change mocks to return plain objects (as in the diff above) to avoid class declarations.
Example disable at top of file:
/* eslint-disable max-classes-per-file */packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromQuerySnapshotSubjectFactory.ts (2)
42-49: Propagate abort signal into waitForData pipeline to cancel promptlyWhen waitForData is true, aborts currently rely on the timeout to settle. Wire the AbortSignal into the stream so the Promise resolves/rejects promptly on abort, avoiding up to 10s hangs.
Apply this diff to the pipeline:
- return firstValueFrom( - subject$.pipe( - takeUntil(timer(options.waitForDataTimeout ?? 10_000)), - skipWhile(({ isLoading }) => isLoading), - ), - ) + return firstValueFrom( + subject$.pipe( + // complete on timeout or abort, whichever comes first + takeUntil(timer(options.waitForDataTimeout ?? 10_000)), + skipWhile(({ isLoading }) => isLoading), + ), + )And add abort to the takeUntil with an extra operator outside this range:
- Import fromEvent and merge from rxjs.
- Replace the single takeUntil(timer(...)) with:
takeUntil(merge(timer(options.waitForDataTimeout ?? 10_000), fromEvent(signal, 'abort')))Outside-range import addition:
// add to imports import { fromEvent, merge } from 'rxjs'I can update the tests to cover abort during waitForData once this is in.
1-1: Rename to kebab-case to satisfy filename-case ruleRename file to query-fn-from-query-snapshot-subject-factory.ts.
packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromDocumentSnapshotSubjectFactory.ts (2)
39-46: Add abort awareness to the waitForData streamMirror the query snapshot variant: ensure abort cancels the waitForData Promise promptly rather than waiting for timeout.
Apply this diff to the pipeline:
- return firstValueFrom( - subject$.pipe( - takeUntil(timer(options.waitForDataTimeout ?? 10_000)), - skipWhile(({ isLoading }) => isLoading), - ), - ) + return firstValueFrom( + subject$.pipe( + // complete on timeout or abort, whichever comes first + takeUntil(timer(options.waitForDataTimeout ?? 10_000)), + skipWhile(({ isLoading }) => isLoading), + ), + )Outside-range import addition and change (same as the query snapshot file):
- Add fromEvent and merge imports.
- Replace takeUntil(timer(...)) with takeUntil(merge(timer(options.waitForDataTimeout ?? 10_000), fromEvent(signal, 'abort'))).
import { fromEvent, merge } from 'rxjs'
1-1: Rename to kebab-case to satisfy filename-case ruleRename file to query-fn-from-document-snapshot-subject-factory.ts.
packages/react-kitchen-sink/src/react-query/FirestoreSnapshotManager.ts (1)
46-48: Consider whether throwing an error is appropriate here.The error thrown when a subscription is not found could crash the application. Since the query cache removal event is triggered by the framework, consider logging a warning instead or handling this gracefully, as it might occur during edge cases like hot module replacement or concurrent cleanup operations.
Consider this safer approach:
const closeSnapshot = this.#onClose.get(snapshotId) if (!closeSnapshot) { - throw new Error(`Subscription for key ${snapshotId} not found`) + console.warn(`FirestoreSnapshotManager: Subscription for key ${snapshotId} not found during cleanup`) + return }
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
packages/react-kitchen-sink/src/schema/__tests__/ZodSchemaError.test.ts (1)
1-26: Rename file to satisfy filename-case ruleESLint (unicorn/filename-case) requires kebab-case for test filenames. Rename to: zod-schema-error.test.ts
♻️ Duplicate comments (14)
packages/react-kitchen-sink/README.md (1)
51-51: Remove unusedQueryClientimport from the example.The example doesn’t use
QueryClient; keep imports minimal.Apply this diff:
-import { QueryClient, useQueryClient, useSuspenseQuery } from '@tanstack/react-query' +import { useQueryClient, useSuspenseQuery } from '@tanstack/react-query'packages/react-kitchen-sink/src/rxjs/__tests__/schemaDocumentSnapshotSubject.test.ts (1)
28-35: Fix spy return type and strengthen assertion: doc() returns DocumentReference, not DocumentSnapshot.As per the factory and schemaDocumentSnapshotSubject implementation,
read.docreturns aDocumentReference, which is then fed intofromDocumentRef(see packages/rxjs-firebase/src/source/fromDocumentRef.ts). The test currently mocks aDocumentSnapshotand only checks thatfromDocumentRefwas called, not that it received the expected reference and options. Update the spy to return aDocumentReferenceand assert the exact call.Apply:
- const documentSnapshot = mock<DocumentSnapshot>() - vi.spyOn(collections.users.read, 'doc').mockReturnValue(documentSnapshot) + const documentRef = mock<DocumentReference>() + vi.spyOn(collections.users.read, 'doc').mockReturnValue(documentRef) @@ - expect(fromDocumentRef).toHaveBeenCalled() + expect(fromDocumentRef).toHaveBeenCalledWith(documentRef, undefined)packages/react-kitchen-sink/src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts (1)
1-1: Use public Firebase import path for QuerySnapshotAvoid the internal '@firebase/firestore' import to fix no-unresolved and prevent coupling to private internals. Use the public 'firebase/firestore' type import.
-import { type QuerySnapshot } from '@firebase/firestore' +import type { QuerySnapshot } from 'firebase/firestore'packages/react-kitchen-sink/src/schema/__tests__/ZodSchemaError.test.ts (3)
12-12: Make the message assertion resilient to path formattingAssert using the snapshot’s path rather than a hardcoded string. Optionally align the mocked path with Firestore’s format (no leading slash).
Apply this diff:
- ref: { path: '/c/id', parent: { id: 'c' } }, + ref: { path: 'c/id', parent: { id: 'c' } }, @@ - expect(error.message).toBe('Zod Error for /c/id') + expect(error.message).toBe(`Zod Error for ${snapshot.ref.path}`)Also applies to: 17-17
1-1: Import Firestore types from the public module to fix resolver errorUse the public Firestore entrypoint; the internal package isn’t resolvable and isn’t a stable API surface.
Apply this diff:
-import { type QueryDocumentSnapshot } from '@firebase/firestore' +import { type QueryDocumentSnapshot } from 'firebase/firestore'
2-2: Use the correct Sentry Event type; fix unresolved import and typingbeforeSend receives an Event from @sentry/types, not ErrorEvent from @sentry/react. This also resolves the linter error.
Apply this diff:
-import { type ErrorEvent } from '@sentry/react' +import { type Event as SentryEvent } from '@sentry/types' @@ - const event = error.beforeSend(mock<ErrorEvent>()) + const event = error.beforeSend(mock<SentryEvent>())Also applies to: 19-19
packages/react-kitchen-sink/src/sentry/__tests__/sentryDocumentSnapshotListener.test.ts (1)
1-1: Use public Firestore types, not internal '@firebase/firestore'Importing from '@firebase/firestore' couples tests to internal modules and fails lint/module resolution. Switch to the public SDK path.
-import { type DocumentReference, type DocumentSnapshot } from '@firebase/firestore' +import type { DocumentReference, DocumentSnapshot } from 'firebase/firestore'packages/react-kitchen-sink/src/sentry/__tests__/processErrorWithSentryCaptureContext.test.ts (1)
1-1: Fix type import: use public Sentry types packageImport the Sentry event type from '@sentry/types' (public types package) instead of '@sentry/react' to resolve no-unresolved and keep type deps minimal.
Apply this diff:
-import { type ErrorEvent } from '@sentry/react' +import type { Event as ErrorEvent } from '@sentry/types'packages/react-kitchen-sink/src/sentry/__tests__/ErrorWithSentryCaptureContext.test.ts (1)
1-1: Fix type import: use public Sentry types packageImport the event type from '@sentry/types' instead of '@sentry/core' to fix no-unresolved and keep only the public types dependency.
Apply this diff:
-import { type ErrorEvent } from '@sentry/core' +import type { Event as ErrorEvent } from '@sentry/types'packages/react-kitchen-sink/src/schema/__tests__/firestoreDataConverter.test.ts (1)
1-2: Do not import from '@firebase/firestore'; use the public 'firebase/firestore' entryStick to the public module to avoid bundling/type issues and to satisfy lint rules. Also consolidate imports.
-import { type QueryDocumentSnapshot } from '@firebase/firestore' -import { GeoPoint, Timestamp } from 'firebase/firestore' +import { type DocumentData, type QueryDocumentSnapshot, GeoPoint, Timestamp } from 'firebase/firestore'Optionally, be explicit with generics on your mock to align with the production signature:
- const snapshot = mock<QueryDocumentSnapshot>() + const snapshot = mock<QueryDocumentSnapshot<DocumentData, DocumentData>>()packages/react-kitchen-sink/src/rxjs/__tests__/documentSnapshotSubject.test.ts (1)
1-1: Use the public Firestore entry point (firebase/firestore)Importing from the internal
@firebase/firestorepackage causes type/bundle issues and ESLint import resolution errors. Switch to the public entry.Apply this diff:
-import { type DocumentReference } from '@firebase/firestore' +import { type DocumentReference } from 'firebase/firestore'packages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (1)
1-1: ESLint import resolution for workspace packagesIf ESLint flags
@valian/rxjs-firebase, configureeslint-plugin-importresolvers for monorepo/TypeScript as noted in another comment.packages/react-kitchen-sink/src/react-query/observers/__tests__/querySnapshotQueryClientObserver.test.ts (1)
1-5: Unresolved imports and filename-case: align package deps and namingAs noted earlier, ensure @valian/rxjs-firebase (and other test-time imports) are present in devDependencies and ESLint resolvers are configured. Rename file to kebab-case per rule.
- querySnapshotQueryClientObserver.test.ts -> query-snapshot-query-client-observer.test.ts
You can reuse the dependency/resolver script from the first file’s comment.
packages/react-kitchen-sink/src/react-query/__tests__/FirestoreSnapshotManager.test.ts (1)
11-25: Stabilize tests by mocking local rxjs wrappers used by FirestoreSnapshotManagerFirestoreSnapshotManager constructs subjects via local wrappers (../rxjs and ../rxjs/schemaDocumentSnapshotSubject). While your current mocks of @valian/rxjs-firebase are sufficient for the underlying classes, mocking the local wrappers makes the tests fully isolated and avoids accidental behavior changes if those wrappers add logic.
Apply this diff to add the missing mocks (note the paths are relative to this test file):
vi.mock('@sentry/react', () => ({ captureMessage: vi.fn(), })) vi.mock('@valian/rxjs-firebase', () => ({ fromDocumentRef: vi.fn().mockReturnValue({ subscribe: vi.fn() }), fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), DocumentSnapshotSubject: class { close = vi.fn() subscribe = vi.fn() }, QuerySnapshotSubject: class { close = vi.fn() subscribe = vi.fn() }, })) +vi.mock('../../rxjs', () => ({ + documentSnapshotSubject: vi.fn().mockReturnValue({ close: vi.fn(), subscribe: vi.fn() }), + schemaQuerySnapshotSubject: vi.fn().mockReturnValue({ close: vi.fn(), subscribe: vi.fn() }), +})) +vi.mock('../../rxjs/schemaDocumentSnapshotSubject', () => ({ + schemaDocumentSnapshotSubject: vi.fn().mockReturnValue({ close: vi.fn(), subscribe: vi.fn() }), +}))
🧹 Nitpick comments (64)
packages/react-kitchen-sink/README.md (5)
50-50: Usetsxcode fences for JSX-containing examples.Both examples include JSX; using
tsximproves syntax highlighting.Apply these diffs:
-```typescript +```tsx-```typescript +```tsxAlso applies to: 82-82
57-58: Memoize the manager to avoid re-instantiation on every render.Creating
FirestoreSnapshotManageron each render is avoidable; memoize it.Apply this diff:
- const client = useQueryClient() - const manager = new FirestoreSnapshotManager(client) + const client = useQueryClient() + const manager = useMemo(() => new FirestoreSnapshotManager(client), [client])Add the missing import at the top of that snippet:
import { useMemo } from 'react'
84-84: Remove unuseduseSchemaDocumentStoreimport.Only
useSchemaQueryStoreis used in the example.Apply this diff:
-import { useSchemaDocumentStore, useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks' +import { useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks'
109-111: Prefer stable keys over array indices in lists.Use a document ID or another stable unique field to prevent unnecessary re-renders.
Apply this diff if
todo.idis available:- {data.map((todo, i) => ( - <li key={i}>{todo.title}</li> - ))} + {data.map((todo) => ( + <li key={todo.id}>{todo.title}</li> + ))}
123-123: Tighten phrasing in the contributing section.Shorter and more direct wording reads better.
Apply this diff:
-Contributions are welcome! Please feel free to submit a Pull Request. +Contributions welcome—open a pull request.packages/react-kitchen-sink/src/rxjs/__tests__/schemaDocumentSnapshotSubject.test.ts (2)
10-16: Ensure the mock is effectively applied before module usage.In Vitest, mocks are hoisted, but ordering can still surprise when dependencies are imported eagerly. To be safe, declare
vi.mock('@valian/rxjs-firebase', ...)before importing modules that depend on it (e.g.,schemaDocumentSnapshotSubject) or convert the subject import into a dynamic import after the mock.If you prefer to keep static imports, move this mock above Line 8. Alternatively:
vi.mock('@valian/rxjs-firebase', () => ({ /* ... */ })) const { schemaDocumentSnapshotSubject } = await import('../schemaDocumentSnapshotSubject')
1-1: Rename file to kebab-case to satisfy lint rule.ESLint flags filename casing. Rename to
schema-document-snapshot-subject.test.ts.packages/react-kitchen-sink/src/sentry/__tests__/handleNotAwaitedPromise.test.ts (4)
15-22: Prefer microtask flush over setTimeout and assert single invocationUsing setTimeout(0) flushes a macro task and is slower/flakier than flushing the microtask queue. Also, asserting the number of calls prevents silent double-captures.
Apply this diff:
it('captures rejection errors', async () => { const error = new Error('boom') handleNotAwaitedPromise(Promise.reject(error)) - await new Promise((resolve) => { - setTimeout(resolve, 0) - }) - expect(captureException).toHaveBeenCalledWith(error, undefined) + // Flush microtasks; ensures the attached .catch runs + await Promise.resolve() + await Promise.resolve() + expect(captureException).toHaveBeenCalledWith(error, undefined) + expect(captureException).toHaveBeenCalledTimes(1) })
30-34: Silence Biome false positive for intentional thenableThis test intentionally creates a thenable without catch. Biome flags it (lint/suspicious/noThenProperty). Ignore the rule locally to keep the test intent explicit.
Apply this diff:
it('does nothing when object has no catch', () => { - // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function + // biome-ignore lint/suspicious/noThenProperty: intentionally creating a thenable without catch to verify guard + // eslint-disable-next-line unicorn/no-thenable, @typescript-eslint/no-empty-function handleNotAwaitedPromise({ then: () => {} } as unknown as Promise<void>) expect(captureException).not.toHaveBeenCalled() })
1-36: Rename test file to kebab-case to satisfy lint ruleESLint (unicorn/filename-case) wants kebab-case. Rename file to: handle-not-awaited-promise.test.ts.
Would you like me to include the rename and update any references in a follow-up commit?
10-35: Optional: add coverage for context and non-function catchTwo edge cases worth locking down:
- Passing a CaptureContext should forward it to captureException.
- Objects with a catch property that is not a function should be ignored (current implementation guards against this).
I can add tests like:
it('forwards CaptureContext when provided', async () => { const error = new Error('boom') const context = { level: 'error', tags: { foo: 'bar' } } as any handleNotAwaitedPromise(Promise.reject(error), context) await Promise.resolve() await Promise.resolve() expect(captureException).toHaveBeenCalledWith(error, context) }) it('does nothing when catch is not a function', () => { // @ts-expect-error: intentionally wrong shape handleNotAwaitedPromise({ catch: 123 }) expect(captureException).not.toHaveBeenCalled() })Want me to push these tests?
packages/react-kitchen-sink/src/sentry/__tests__/sentrySchemaQuerySnapshotListener.test.ts (5)
1-12: Reset mocks between tests to avoid cross-test leakageAdd afterEach(() => vi.clearAllMocks()) to keep tests isolated if/when more tests are added.
-import { describe, expect, it, vi } from 'vitest' +import { describe, expect, it, vi, afterEach } from 'vitest' @@ vi.mock('@sentry/react', () => ({ addBreadcrumb: vi.fn(), captureException: vi.fn(), })) +afterEach(() => { + vi.clearAllMocks() +})
23-34: Assert the forwarded state and breadcrumb payloads, not just that calls occurredStrengthen assertions to verify the exact state forwarding and breadcrumb content. This catches accidental regressions in message/category/payload.
- listener.onSnapshot?.({ - isLoading: false, - hasError: false, - disabled: false, - empty: true, - size: 0, - data: [], - snapshot: stub<QuerySnapshot>(), - } as const) - expect(onSnapshot).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + const state = { + isLoading: false, + hasError: false, + disabled: false, + empty: true, + size: 0, + data: [], + snapshot: stub<QuerySnapshot>(), + } as const + listener.onSnapshot?.(state) + expect(onSnapshot).toHaveBeenCalledWith(state) + expect(addBreadcrumb).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + category: 'firestore', + message: `received query snapshot for (${query.name})`, + data: { state }, + }), + )
35-39: Verify captureException context (tags, fingerprint, contexts.query) to match implementationAsserting the capture context prevents accidental loss of helpful Sentry metadata.
- expect(captureException).toHaveBeenCalled() + expect(captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: expect.objectContaining({ firestore: true, 'schema.collection': 'c' }), + fingerprint: ['firestore', 'c'], + contexts: { query: { query } }, + }), + )
40-43: Check the closing breadcrumb content and call orderThis ensures the “closed” breadcrumb is emitted, with the right message and as the second breadcrumb.
- expect(addBreadcrumb).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + category: 'firestore', + message: `closed query snapshot for (${query.name})`, + }), + )
4-4: Optional: remove vitest-mock-extended to avoid the extra dev dependencyIf you prefer to keep the test lightweight and avoid the no-unresolved on vitest-mock-extended, you can replace mock/stub usage with plain values. Note: casting query to any preserves compatibility with the listener’s expected type.
-import { mock, stub } from 'vitest-mock-extended' +// (optional) remove vitest-mock-extended usage - const query = mock<{ name: string }>() - query.name = 'q' + const query = { name: 'q' } as any @@ - snapshot: stub<QuerySnapshot>(), + snapshot: undefined as unknown as QuerySnapshot,Also applies to: 14-15, 30-30
packages/react-kitchen-sink/src/sentry/__tests__/sentryDocumentSnapshotListener.test.ts (1)
30-41: Strengthen assertions to reduce false positivesAssert argument shapes and call counts to lock in the Sentry payloads and breadcrumb messages.
- expect(onSnapshot).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + expect(onSnapshot).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledTimes(1) + expect(addBreadcrumb).toHaveBeenLastCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'received document snapshot for (/c/id)', + }), + ) @@ - expect(captureException).toHaveBeenCalled() + expect(captureException).toHaveBeenCalledWith( + error, + expect.objectContaining({ + tags: expect.objectContaining({ firestore: true, 'schema.collection': 'c' }), + fingerprint: ['firestore', 'c'], + }), + ) @@ - expect(onComplete).toHaveBeenCalled() - expect(addBreadcrumb).toHaveBeenCalled() + expect(onComplete).toHaveBeenCalled() + expect(addBreadcrumb).toHaveBeenCalledTimes(2) + expect(addBreadcrumb).toHaveBeenLastCalledWith( + expect.objectContaining({ + category: 'firestore', + message: 'closed document snapshot for (/c/id)', + }), + )packages/react-kitchen-sink/src/sentry/__tests__/processErrorWithSentryCaptureContext.test.ts (1)
2-7: Optional: restore spies after each testAdd afterEach(vi.restoreAllMocks) to keep tests isolated.
-import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' @@ import { ErrorWithSentryCaptureContext } from '../ErrorWithSentryCaptureContext' import { processErrorWithSentryCaptureContext } from '../processErrorWithSentryCaptureContext' +afterEach(() => { + vi.restoreAllMocks() +})packages/react-kitchen-sink/src/sentry/__tests__/ErrorWithSentryCaptureContext.test.ts (1)
2-6: Optional: restore spies after each testAdd afterEach(vi.restoreAllMocks) for isolation across tests.
-import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' @@ import { ErrorWithSentryCaptureContext } from '../ErrorWithSentryCaptureContext' +afterEach(() => { + vi.restoreAllMocks() +})packages/react-kitchen-sink/src/schema/firestoreDataConverter.ts (3)
22-24: LGTM on the generic signature; add an explicit return type for clarityThe expanded
<AppModelType, DbModelType extends DocumentData>signature aligns well with Firestore’s typed snapshots. Consider adding an explicit return type to make intent unambiguous and help inference at call sites.-export const firestoreDataConverter = <AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData>( - snapshot: QueryDocumentSnapshot<AppModelType, DbModelType>, -) => handleFirestoreDataTypes(snapshot.data()) +export const firestoreDataConverter = < + AppModelType = DocumentData, + DbModelType extends DocumentData = DocumentData +>( + snapshot: QueryDocumentSnapshot<AppModelType, DbModelType>, +): AppModelType => handleFirestoreDataTypes(snapshot.data())
15-16: Avoid converting non-plain objects; restrict recursion to POJOs
value instanceof Objectwill recurse into any object (including Firestore DocumentReference or custom class instances), potentially mutating types that should be preserved. Restrict to plain objects.- case value instanceof Object: - return Object.fromEntries(Object.entries(value).map(([key, v]) => [key, handleFirestoreDataTypes(v)])) as T + case value !== null && typeof value === 'object' && (value as object).constructor === Object: + return Object.fromEntries( + Object.entries(value as Record<string, unknown>).map(([key, v]) => [key, handleFirestoreDataTypes(v)]), + ) as T
1-1: Review filename-case rule enforcement and Firebase import resolution
- Verified that
react-kitchen-sinkdeclaresfirebase@^12.0.0inpackages/react-kitchen-sink/package.json, so
import … from 'firebase/firestore'will resolve correctly.- No ESLint config (
.eslintrc.*oreslint.config.js) enforcingunicorn/filename-casewas found in the repo.
If you’re using a shared or workspace-level ESLint setup that does enforce kebab-case filenames, please confirm its location and rules.- If
unicorn/filename-caseis enabled, rename this file:
• packages/react-kitchen-sink/src/schema/firestoreDataConverter.ts → firestore-data-converter.tsOtherwise, the import and filename can remain as-is.
packages/react-kitchen-sink/src/schema/__tests__/firestoreDataConverter.test.ts (1)
6-6: Update import path if you rename the implementation file to kebab-caseIf you adopt kebab-case for the source file, adjust this import accordingly.
-import { firestoreDataConverter } from '../firestoreDataConverter' +import { firestoreDataConverter } from '../firestore-data-converter'packages/react-kitchen-sink/src/rxjs/__tests__/documentSnapshotSubject.test.ts (3)
8-14: Ensure the mock is applied before importing the module under testTo guarantee
documentSnapshotSubjectuses the mocked@valian/rxjs-firebaseimplementation, register the mock before importing../documentSnapshotSubject. This avoids leaking real implementations in ESM environments.Apply this diff to hoist the mock and import the SUT after:
-6 import { documentSnapshotSubject } from '../documentSnapshotSubject' - -8 vi.mock('@valian/rxjs-firebase', () => ({ +8 vi.mock('@valian/rxjs-firebase', () => ({ fromDocumentRef: vi.fn().mockReturnValue({ subscribe: vi.fn() }), DocumentSnapshotSubject: class { close = vi.fn() subscribe = vi.fn() }, })) + +import { documentSnapshotSubject } from '../documentSnapshotSubject'Alternatively, keep imports as-is and switch to a dynamic import after mocking:
vi.mock('@valian/rxjs-firebase', /* factory */) const { documentSnapshotSubject } = await import('../documentSnapshotSubject')Also applies to: 6-6
1-1: Rename file to kebab-case to satisfy ESLint (unicorn/filename-case)Rename to: document-snapshot-subject.test.ts
2-2: ESLint “Unable to resolve path” — configure import resolvers for workspace packagesIf ESLint still reports unresolved imports for
@valian/rxjs-firebaseorvitest-mock-extended, configureeslint-plugin-importresolvers for TypeScript and node to include workspace packages.Add to your ESLint config:
settings: { 'import/resolver': { typescript: { project: ['./packages/*/tsconfig.json'] }, node: { extensions: ['.ts', '.tsx', '.js', '.mjs'] }, }, },Also applies to: 4-4
packages/react-kitchen-sink/package.json (1)
62-64: Avoid duplicatingzustandin devDependencies and optionalDependenciesHaving
zustandin both is redundant. Keep it only as optional if runtime-optional, or as a peer if required by consumers.Suggested change (keep as optional only):
"devDependencies": { - "vitest-mock-extended": "^3.1.0", + "vitest-mock-extended": "^3.1.0", "@sentry/core": "^10.1.0", "@sentry/react": "^10.1.0", "@tanstack/react-query": "^5.84.2", "firebase": "^12.0.0", "react": "^19.1.0", "react-dom": "^19.1.0", "rxjs": "^7.8.2", "zod": "^4.0.17", "zod-firebase": "^2.0.0", - "zustand": "^5" + // "zustand": "^5" // remove here if kept as optional }, @@ "optionalDependencies": { "zustand": "^5" },Also applies to: 39-50
packages/react-kitchen-sink/src/react-query/__tests__/documentSnapshotQueryOptions.test.ts (1)
1-1: Rename file to kebab-case (unicorn/filename-case)Rename to: document-snapshot-query-options.test.ts
packages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (2)
8-14: Mock before importing the SUT to guarantee isolationMove the
vi.mock('@valian/rxjs-firebase', ...)block before importing../schemaQuerySnapshotSubjectto ensure the SUT uses the mockedfromQueryandQuerySnapshotSubject.Apply this diff:
-6 import { schemaQuerySnapshotSubject } from '../schemaQuerySnapshotSubject' - -8 vi.mock('@valian/rxjs-firebase', () => ({ +8 vi.mock('@valian/rxjs-firebase', () => ({ fromQuery: vi.fn().mockReturnValue({ subscribe: vi.fn() }), QuerySnapshotSubject: class { close = vi.fn() subscribe = vi.fn() }, })) + +import { schemaQuerySnapshotSubject } from '../schemaQuerySnapshotSubject'Also applies to: 6-6
1-1: Rename file to kebab-caseRename to: schema-query-snapshot-subject.test.ts
packages/react-kitchen-sink/src/react-query/__tests__/querySnapshotQueryOptions.test.ts (1)
1-1: Rename file to kebab-caseRename to: query-snapshot-query-options.test.ts
packages/react-kitchen-sink/src/react-query/observers/__tests__/documentSnapshotQueryClientObserver.test.ts (3)
16-18: Assert exact payload passed to setQueryData on next()Strengthen the test by asserting the exact state passed through to QueryClient rather than a loose “called” assertion.
Apply this diff:
- observer.next({ isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + const state = { isLoading: false, hasError: false, disabled: false } as DocumentSnapshotState + observer.next(state) + expect(client.setQueryData).toHaveBeenCalledWith(queryKey, state)
8-8: Verify Sentry capture on error pathYou’re mocking Sentry but not asserting it’s invoked. Add an assertion and a type-only import to ensure we don’t regress error reporting.
Apply these diffs:
+import { captureException } from '@sentry/react'observer.error(new Error('x')) + expect(captureException).toHaveBeenCalledWith(expect.any(Error)) expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { isLoading: false, hasError: true, disabled: false, })Also applies to: 19-24
26-32: Also assert total call count for setQueryDataThis ensures no extra/unexpected cache writes happen.
Apply this diff:
observer.complete() expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { isLoading: false, hasError: false, disabled: true, }) + expect(client.setQueryData).toHaveBeenCalledTimes(3)packages/react-kitchen-sink/src/react-query/__tests__/schemaQuerySnapshotQueryOptions.test.ts (4)
1-1: Import Firestore types from public moduleAvoid internal @firebase/* imports; use firebase/firestore for type-only imports.
Apply this diff:
-import { type SnapshotListenOptions } from '@firebase/firestore' +import { type SnapshotListenOptions } from 'firebase/firestore'
3-3: Use the schema-specific listener type to match factory APIschemaQuerySnapshotSubjectFactory expects SchemaQuerySnapshotStateListener; aligning the test’s listener type reduces the chance of drift.
Apply these diffs:
-import { type QuerySnapshotStateListener } from '@valian/rxjs-firebase' +import { type SchemaQuerySnapshotStateListener } from '../../rxjs/types'- const listener = mock<QuerySnapshotStateListener>() + const listener = mock<SchemaQuerySnapshotStateListener<any, any>>()If you intentionally unified listener types across schema/non-schema paths, feel free to skip; otherwise this better matches FirestoreSnapshotManager.schemaQuerySnapshotSubjectFactory’s signature.
Also applies to: 24-24
33-41: Broaden assertions: ensure meta is fully populatedYou can assert meta fields to guard against regressions in the options builder.
Apply this diff:
expect(opts.queryKey).toEqual(queryOptions({ queryKey: ['k'] }).queryKey) expect(snapshotManager.schemaQuerySnapshotSubjectFactory).toHaveBeenCalledWith( collections.users, { name: 'q' }, snapshotOptions, listener, ) - expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.type).toBe('snapshot') + expect(opts.meta?.snapshotManager).toBe(snapshotManager) + expect(opts.meta?.collection).toBe(collections.users.collectionName) + expect(opts.meta?.schemaQuery).toEqual({ name: 'q' }) + expect(opts.staleTime).toBe(Infinity) + expect(opts.retry).toBe(false) + expect(opts.gcTime).toBe(10_000)
1-7: Resolve ESLint “unresolved import” and filename-case warnings
- Ensure devDependencies exist for all test-time imports (firebase, @tanstack/react-query, @valian/rxjs-firebase, zod, zod-firebase, vitest-mock-extended).
- Rename the file to kebab-case as per the rule.
You can reuse the dependency check script provided in the other comment, and consider renaming:
- schemaQuerySnapshotQueryOptions.test.ts -> schema-query-snapshot-query-options.test.ts
packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromDocumentSnapshotSubjectFactory.test.ts (5)
1-1: Import Firestore types from public moduleUse firebase/firestore rather than internal @firebase/* package.
Apply this diff:
-import { type DocumentSnapshot } from '@firebase/firestore' +import { type DocumentSnapshot } from 'firebase/firestore'
32-33: Pass a queryKey in all QueryFunctionContext mocks for consistencyA few tests omit queryKey; while they pass today, providing it makes behavior consistent and guards against future usage changes.
Apply these diffs:
- const result = await queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const result = await queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] }), + )- const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const promise = queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] }), + )Also applies to: 42-43
96-107: Consider closing/unsubscribing the subject on waitForData timeoutCurrently, a timeout leads to a rejected promise but the subscription remains active (until abort or GC), which can leak resources and keep pushing cache updates to a failed query.
If this is unintended, adjust the production code to close on timeout/error for the waitForData branch. For example, in queryFnFromDocumentSnapshotSubjectFactory.ts:
// Pseudocode patch in queryFnFromDocumentSnapshotSubjectFactory.ts waitForData branch try { return await firstValueFrom( subject$.pipe( takeUntil(timer(options.waitForDataTimeout ?? 10_000)), skipWhile(({ isLoading }) => isLoading), ), ) } catch (err) { subscription.unsubscribe() subject$.close() throw err }I can prep a similar change for the query snapshot variant if you want to apply it consistently.
109-131: Use fake timers to reduce flakiness in timeout-based testsRelying on setTimeout(0) can be flaky under load. Fake timers make these tests deterministic and faster.
Example pattern (no diff due to multiple insert points):
- At the start of the relevant tests: vi.useFakeTimers()
- Schedule emissions, then vi.runAllTimers() or vi.advanceTimersByTime(timeout)
- After each test: vi.useRealTimers()
1-7: Resolve ESLint import and filename-case warnings
- Ensure devDependencies exist for firebase, @tanstack/react-query, @valian/rxjs-firebase, rxjs, vitest-mock-extended.
- Rename file to kebab-case: query-fn-from-document-snapshot-subject-factory.test.ts.
Reuse the dependency check script from the earlier comment.
packages/react-kitchen-sink/src/react-query/observers/__tests__/querySnapshotQueryClientObserver.test.ts (3)
16-24: Assert exact payload passed to setQueryData on next()Mirror the stronger assertion used elsewhere by checking the exact state.
Apply this diff:
- observer.next({ + const state = { empty: false, size: 1, isLoading: false, hasError: false, disabled: false, data: [{}], - } as QuerySnapshotState) - expect(client.setQueryData).toHaveBeenCalled() + } as QuerySnapshotState + observer.next(state) + expect(client.setQueryData).toHaveBeenCalledWith(queryKey, state)
8-8: Verify Sentry capture on error pathYou already mock Sentry; assert that captureException is called when error() is invoked.
Apply these diffs:
+import { captureException } from '@sentry/react'observer.error(new Error('x')) + expect(captureException).toHaveBeenCalledWith(expect.any(Error)) expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: true, disabled: false, data: [], })Also applies to: 26-35
36-45: Also assert total call count for setQueryDataEnsure only the intended three cache writes occur.
Apply this diff:
observer.complete() expect(client.setQueryData).toHaveBeenCalledWith(queryKey, { empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], }) + expect(client.setQueryData).toHaveBeenCalledTimes(3)packages/react-kitchen-sink/src/react-query/queryFn/__tests__/queryFnFromQuerySnapshotSubjectFactory.test.ts (5)
1-1: Import Firestore types from public modulePrefer firebase/firestore over @firebase/*.
Apply this diff:
-import { type QuerySnapshot } from '@firebase/firestore' +import { type QuerySnapshot } from 'firebase/firestore'
39-40: Provide queryKey in all QueryFunctionContext mocksKeep tests consistent and future-proof by always including queryKey.
Apply these diffs:
- const result = await queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const result = await queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] }), + )- const promise = queryFn(mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient })) + const promise = queryFn( + mock<QueryFunctionContext>({ signal: controller.signal, client: queryClient, queryKey: ['k'] }), + )Also applies to: 56-57
122-133: Consider closing/unsubscribing the subject on waitForData timeoutSame rationale as the document variant: avoid keeping live subscriptions after a rejected promise due to timeout.
Proposed change (in queryFnFromQuerySnapshotSubjectFactory.ts) for the waitForData branch:
try { return await firstValueFrom( subject$.pipe( takeUntil(timer(options.waitForDataTimeout ?? 10_000)), skipWhile(({ isLoading }) => isLoading), ), ) } catch (err) { subscription.unsubscribe() subject$.close() throw err }
135-153: Use fake timers to stabilize timer-driven testsSwap real timers for vi.useFakeTimers() to make timeout tests deterministic and fast.
Pattern:
- vi.useFakeTimers() at test start
- schedule subject.next(...)
- vi.runAllTimers() or vi.advanceTimersByTime(ms)
- vi.useRealTimers() in afterEach
1-7: Resolve ESLint import and filename-case warnings
- Ensure devDependencies for firebase, @tanstack/react-query, @valian/rxjs-firebase, rxjs, vitest-mock-extended are present.
- Rename to kebab-case: query-fn-from-query-snapshot-subject-factory.test.ts.
Reuse the earlier script to validate package.json entries.
packages/react-kitchen-sink/src/react-query/__tests__/FirestoreSnapshotManager.test.ts (3)
39-47: Reset mocks between tests to avoid cross-test leakageMinor improvement: clear all mock call histories in beforeEach for repeatable assertions.
Apply this diff:
beforeEach(() => { + vi.clearAllMocks() subscribers.length = 0 const queryCache = mock<QueryCache>() queryClient.getQueryCache.mockReturnValue(queryCache)
96-114: Fix typo in test nameDouble “schema” in the test description is a nit.
Apply this diff:
- it('schema schemaDocumentSnapshotSubjectFactory produce subjects and register closures', () => { + it('schemaDocumentSnapshotSubjectFactory produces subjects and registers closures', () => {
1-1: Silence known ESLint false-positives for tests (filename-case, max-classes-per-file)Given the linter hints and that this is a test file with tiny mock classes, you can pragmatically disable these rules here.
Apply this diff at the very top of the file:
+/* eslint-disable unicorn/filename-case, max-classes-per-file */ import { type DocumentReference, type Query } from '@firebase/firestore'packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromQuerySnapshotSubjectFactory.ts (3)
33-40: Close the subject before unsubscribing so the observer receives “complete” (disabled=true)Calling close() first lets QuerySnapshotSubject complete, which in turn triggers the query client observer’s complete handler to stamp disabled: true into the cache. Then unsubscribe to detach the observer. Current order drops the complete notification.
Apply this diff:
signal.addEventListener( 'abort', () => { - subscription.unsubscribe() - subject$.close() + subject$.close() + subscription.unsubscribe() }, { once: true }, )
19-28: Optional: surface cancellation via an AbortError instead of returning a disabled stateReact Query treats thrown AbortError as a canceled fetch; returning a “disabled” state is a valid design choice too. If you prefer canonical cancellation semantics, throw instead.
Apply this diff:
- if (signal.aborted) { - return { - empty: true, - size: 0, - isLoading: false, - hasError: false, - disabled: true, - data: [], - } as const - } + if (signal.aborted) { + throw new DOMException('Aborted', 'AbortError') + }If you keep the current behavior for consistency with the rest of the API, consider documenting it in the options JSDoc.
42-49: Timeout path will reject the query; confirm this is intentionalWhen waitForData is true, takeUntil(timer(...)) completes the stream without emitting if data never arrives, causing firstValueFrom to reject. That will mark the query as errored. If the intended UX is to remain in loading or return a non-error sentinel, consider handling the timeout differently (e.g., returning an initial state).
packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromDocumentSnapshotSubjectFactory.ts (3)
30-37: Close-before-unsubscribe for consistent “complete” propagationSame rationale as the query snapshot variant: closing first lets the observer receive complete and mark the cache disabled.
Apply this diff:
signal.addEventListener( 'abort', () => { - subscription.unsubscribe() - subject$.close() + subject$.close() + subscription.unsubscribe() }, { once: true }, )
19-25: Optional: consider throwing AbortError instead of returning a disabled stateReturning a disabled state is consistent with your snapshot pattern; alternatively, throwing an AbortError signals cancellation to React Query.
Apply this diff:
- if (signal.aborted) { - return { - isLoading: false, - hasError: false, - disabled: true, - } as const - } + if (signal.aborted) { + throw new DOMException('Aborted', 'AbortError') + }
39-46: Timeout behavior will reject; verify desired UXIf waitForData times out, firstValueFrom will reject. Ensure consumers expect an error state vs. continuing to load or returning a sentinel value.
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (2)
68-71: Pass only the needed options to the queryFn factory.Passing the whole props object is safe but noisy. Limiting to the factory’s option shape clarifies intent and avoids accidental coupling.
- queryFn: queryFnFromDocumentSnapshotSubjectFactory( - snapshotManager.documentSnapshotSubjectFactory(ref, snapshotOptions, listener), - props, - ), + queryFn: queryFnFromDocumentSnapshotSubjectFactory( + snapshotManager.documentSnapshotSubjectFactory(ref, snapshotOptions, listener), + { + waitForData: props.waitForData, + waitForDataTimeout: props.waitForDataTimeout, + }, + ),
42-50: Align input key typing with DataTag for stronger inference.The result type brands queryKey as DataTag<...>, but the input accepts any QueryKey. Consider requiring the input queryKey to already be DataTag-typed to guarantee type-level alignment and better inference downstream. If that’s too strict for callers, keep as-is.
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (2)
77-80: Limit options passed to the queryFn factory to its shape.Mirror the document variant: only pass waitForData/waitForDataTimeout.
- queryFn: queryFnFromQuerySnapshotSubjectFactory( - snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), - props, - ), + queryFn: queryFnFromQuerySnapshotSubjectFactory( + snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), + { + waitForData: props.waitForData, + waitForDataTimeout: props.waitForDataTimeout, + }, + ),
50-57: Consider requiring a DataTag queryKey at input for stronger typing.Like the document builder, the result brands the key, but the input accepts any QueryKey. Consider tightening the input to require a DataTag-branded key to improve type safety.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (1)
2-8: ESLint unresolved imports (duplicate of prior feedback).This repeats the earlier issue about unresolved '@tanstack/react-query' and '@valian/rxjs-firebase' imports due to ESLint resolver configuration in a monorepo/TS workspace. Please refer to the previous guidance to adjust the import resolver and devDeps.
Also applies to: 9-9
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (1)
2-14: Resolve ESLint “unresolved module” for @tanstack/react-query and zod-firebaseThese imports are flagged by ESLint. This is a workspace resolver issue (not a code issue). Please ensure ESLint’s TypeScript resolver is configured to include all workspace tsconfig files.
Run this to verify dependency declarations and resolver config presence:
#!/bin/bash set -euo pipefail echo "== Deps declared in package.json ==" fd -t f package.json -E node_modules | while read -r f; do tanstack=$(jq -r '(.dependencies["@tanstack/react-query"] // .devDependencies["@tanstack/react-query"] // empty)' "$f") zodfb=$(jq -r '(.dependencies["zod-firebase"] // .devDependencies["zod-firebase"] // empty)' "$f") if [[ -n "$tanstack" || -n "$zodfb" ]]; then echo "-> $f" [[ -n "$tanstack" ]] && echo " @tanstack/react-query: $tanstack" [[ -n "$zodfb" ]] && echo " zod-firebase: $zodfb" fi done echo echo "== ESLint resolver config ==" rg -nC2 -S "import/resolver" -g "!.*/node_modules/**" --glob ".eslintrc.*" || true rg -nC2 -S "parserOptions.*project" -g "!.*/node_modules/**" --glob ".eslintrc.*" || true echo echo "Tip: Ensure settings.import/resolver.typescript.project includes both './tsconfig.json' and './packages/*/tsconfig.json'."
🧹 Nitpick comments (5)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (1)
68-71: Pass only the recognized queryFn options instead of the entire props.
queryFnFromDocumentSnapshotSubjectFactoryonly consumeswaitForDataandwaitForDataTimeout. Passing the entirepropsobject is looser than necessary and obscures intent.Apply this diff:
queryFn: queryFnFromDocumentSnapshotSubjectFactory( snapshotManager.documentSnapshotSubjectFactory(ref, snapshotOptions, listener), - props, + { + waitForData: props.waitForData, + waitForDataTimeout: props.waitForDataTimeout, + }, ),packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (2)
24-48: Unify queryKey typing: require DataTag on input as well to match the result typeThe result interface narrows queryKey to a DataTag, but the input options accept a plain QueryKey. Tightening the input to DataTag improves type-safety and aligns with your invalidation/tagging strategy.
Apply:
export interface SchemaQuerySnapshotQueryOptions< TCollectionSchema extends CollectionSchema, TOptions extends MetaOutputOptions, TError = DefaultError, TData = SchemaQuerySnapshotState<TCollectionSchema, TOptions>, TQueryKey extends QueryKey = QueryKey, > extends Omit< - UnusedSkipTokenOptions<SchemaQuerySnapshotState<TCollectionSchema, TOptions>, TError, TData, TQueryKey>, + UnusedSkipTokenOptions<SchemaQuerySnapshotState<TCollectionSchema, TOptions>, TError, TData, TQueryKey>, | 'queryFn' + | 'queryKey' | 'initialData' | 'staleTime' | 'refetchInterval' | 'refetchIntervalInBackground' | 'refetchOnWindowFocus' | 'refetchOnMount' | 'refetchOnReconnect' | 'retryOnMount' | 'retry' >, QueryFnFromQuerySnapshotSubjectFactoryOptions { + queryKey: DataTag<TQueryKey, SchemaQuerySnapshotState<TCollectionSchema, TOptions>, TError> factory: SchemaFirestoreQueryFactory<TCollectionSchema> query: SchemaQuerySpecification<TCollectionSchema, TOptions> snapshotOptions?: TOptions & SnapshotListenOptions listener?: SchemaQuerySnapshotStateListener<TCollectionSchema, TOptions> }
67-75: Avoid passing the entire props object into queryFn factory (reduce coupling/accidental leakage)Limit the options passed to the queryFn factory to the specific fields it understands to avoid accidental coupling if more fields are added to props later.
Apply:
export const schemaQuerySnapshotQueryOptions = < @@ >( snapshotManager: FirestoreSnapshotManager, { factory, query, snapshotOptions, listener, - ...props + waitForData, + waitForDataTimeout, + ...rest }: SchemaQuerySnapshotQueryOptions<TCollectionSchema, TOptions, TError, TData, TQueryKey>, ): SchemaQuerySnapshotQueryOptionsResult<TCollectionSchema, TOptions, TError, TData, TQueryKey> => queryOptions({ - queryFn: queryFnFromQuerySnapshotSubjectFactory( - snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), - props, - ), + queryFn: queryFnFromQuerySnapshotSubjectFactory( + snapshotManager.schemaQuerySnapshotSubjectFactory(factory, query, snapshotOptions, listener), + { waitForData, waitForDataTimeout }, + ), staleTime: Infinity, retry: false, gcTime: 10_000, - ...props, + ...rest, meta: { - ...props.meta, + ...rest.meta, type: 'snapshot', snapshotManager, collection: factory.collectionName, schemaQuery: query, }, })packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (2)
68-71: Pass only factory options to the queryFn factory (avoid leaking unrelated props).Narrow the second argument to just the factory options to prevent accidental coupling with future keys in
props.- queryFn: queryFnFromQuerySnapshotSubjectFactory( - snapshotManager.querySnapshotSubjectFactory(query, snapshotOptions, listener), - props, - ), + queryFn: queryFnFromQuerySnapshotSubjectFactory( + snapshotManager.querySnapshotSubjectFactory(query, snapshotOptions, listener), + { waitForData: props.waitForData, waitForDataTimeout: props.waitForDataTimeout }, + ),
72-76: Default gcTime (10s) may be too aggressive for snapshot queries.With live snapshots, a short
gcTimecan cause frequent teardown/re-create cycles when components mount/unmount rapidly. It’s overrideable viaprops, but consider a higher default (e.g., 60s) or document the trade-offs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts(1 hunks)packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts(1 hunks)packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (3)
packages/react-kitchen-sink/src/rxjs/index.ts (2)
DocumentSnapshotState(5-5)DocumentSnapshotStateListener(6-6)packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromDocumentSnapshotSubjectFactory.ts (2)
QueryFnFromDocumentSnapshotSubjectFactoryOptions(8-11)queryFnFromDocumentSnapshotSubjectFactory(13-53)packages/react-kitchen-sink/src/react-query/FirestoreSnapshotManager.ts (1)
FirestoreSnapshotManager(37-129)
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (3)
packages/react-kitchen-sink/src/rxjs/index.ts (2)
QuerySnapshotState(7-7)QuerySnapshotStateListener(8-8)packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromQuerySnapshotSubjectFactory.ts (2)
QueryFnFromQuerySnapshotSubjectFactoryOptions(8-11)queryFnFromQuerySnapshotSubjectFactory(13-59)packages/react-kitchen-sink/src/react-query/FirestoreSnapshotManager.ts (1)
FirestoreSnapshotManager(37-129)
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (3)
packages/react-kitchen-sink/src/rxjs/types.ts (2)
SchemaQuerySnapshotState(21-24)SchemaQuerySnapshotStateListener(44-50)packages/react-kitchen-sink/src/react-query/queryFn/queryFnFromQuerySnapshotSubjectFactory.ts (2)
QueryFnFromQuerySnapshotSubjectFactoryOptions(8-11)queryFnFromQuerySnapshotSubjectFactory(13-59)packages/react-kitchen-sink/src/react-query/FirestoreSnapshotManager.ts (1)
FirestoreSnapshotManager(37-129)
🪛 ESLint
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts
[error] 8-8: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@valian/rxjs-firebase'.
(import-x/no-unresolved)
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts
[error] 8-8: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@valian/rxjs-firebase'.
(import-x/no-unresolved)
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts
[error] 8-8: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 14-14: Unable to resolve path to module 'zod-firebase'.
(import-x/no-unresolved)
🔇 Additional comments (6)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (1)
76-81: Good: snapshot meta fields are authoritative.Spreading props.meta first and then setting type/snapshotManager/documentRef ensures callers can’t override critical cleanup metadata. This addresses the earlier leak risk.
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (2)
76-84: Sane defaults for snapshot-backed queries (staleTime, retry, gcTime)Defaults make sense for live snapshot streams: never stale, don’t retry, and a short GC to release subscriptions promptly on unmount. No changes requested.
85-91: meta.type is locked to 'snapshot' (prevents subscription leaks) — resolvedGood fix placing spread before type so callers cannot override meta.type.
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (3)
78-82: Great fix: meta ordering now prevents overridingtype: 'snapshot'.Spreading
props.metafirst and then forcingtype,snapshotManager, andqueryensures cleanup can’t be broken by callers. This closes the leakage hole raised previously.
36-36: Inconsistency with summary:waitForData/waitForDataTimeoutare still public.The code extends
QueryFnFromQuerySnapshotSubjectFactoryOptions, so these options remain in the public API, contrary to the AI summary’s note about removal. Confirm that keeping them exposed is intentional.
42-50: No changes needed forDataTagusage
Confirmed that @tanstack/react-query v5 re-exports theDataTagtype from @tanstack/query-core and that it correctly composes withUnusedSkipTokenOptions. ThequeryOptions(...)helpers produce aDataTag-narrowedqueryKey, so the return type is assignable toQuerySnapshotQueryOptionsResult<…>.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/react-kitchen-sink/README.md (3)
26-26: Peer dependency aligned to zod-firebase v2 — good catchThanks for updating the peer dependency to
zod-firebase^2 to match the examples.
32-32: Typo fixed: FirestoreSnapshotManagerAppreciate correcting “Snaphot” → “Snapshot” in the public docs.
35-39: Fix imports in the React Query example: remove unused and add missing
- Remove unused:
QueryClient,doc, anddb.- Add missing:
z(fromzod) and the collection builder (imported here ascollectionsBuilder) to match usage below.These omissions will break copy/paste usage of the snippet.
Apply this diff:
-import { QueryClient, useQueryClient, useSuspenseQuery } from '@tanstack/react-query' -import { doc } from 'firebase/firestore' -import { db } from './firebase' -import { documentSnapshotQueryOptions, FirestoreSnapshotManager } from '@valian/react-kitchen-sink/react-query' +import { useQueryClient, useSuspenseQuery } from '@tanstack/react-query' +import { documentSnapshotQueryOptions, FirestoreSnapshotManager } from '@valian/react-kitchen-sink/react-query' +import { z } from 'zod' +import { collectionsBuilder } from 'zod-firebase'
🧹 Nitpick comments (3)
packages/react-kitchen-sink/README.md (3)
43-45: Schema/usage mismatch: email referenced but not in schemaYou render
data.data?.emailbutUserZoddoesn’t defineApply this diff to add an optional, validated email:
const UserZod = z.object({ name: z.string(), + email: z.string().email().optional(), })Also applies to: 64-64
99-101: Prefer stable keys over array index in listsUse a stable identifier (e.g., Firestore doc
id) instead ofito avoid unnecessary re-renders and subtle bugs.- {data.map((todo, i) => ( - <li key={i}>{todo.title}</li> + {data.map((todo) => ( + <li key={todo.id}>{todo.title}</li> ))}If your store doesn’t expose
id, consider adding it or using another stable key.
113-113: Tighten phrasing per style lintMinor wording improvement for a more concise tone.
-Contributions are welcome! Please feel free to submit a Pull Request. +Contributions welcome. Please open a Pull Request.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/react-kitchen-sink/README.md(1 hunks)packages/rxjs-firebase/README.md(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rxjs-firebase/README.md
🧰 Additional context used
🪛 LanguageTool
packages/react-kitchen-sink/README.md
[style] ~113-~113: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...ontributing Contributions are welcome! Please feel free to submit a Pull Request. ## Support If ...
(FEEL_FREE_TO_STYLE_ME)
| import { useStore } from 'zustand' | ||
| import { useSchemaDocumentStore, useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks' | ||
| import { createFirestoreFactory, collectionSchema } from 'zod-firebase' | ||
|
|
There was a problem hiding this comment.
Fix imports in the Zustand example
- Remove unused
useSchemaDocumentStore. - Replace unused
createFirestoreFactory, collectionSchemawithcollectionsBuilder(matches usage). - Add missing
zimport.
This makes the snippet copy/paste-ready.
import { useStore } from 'zustand'
-import { useSchemaDocumentStore, useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks'
-import { createFirestoreFactory, collectionSchema } from 'zod-firebase'
+import { useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks'
+import { collectionsBuilder } from 'zod-firebase'
+import { z } from 'zod'📝 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.
| import { useStore } from 'zustand' | |
| import { useSchemaDocumentStore, useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks' | |
| import { createFirestoreFactory, collectionSchema } from 'zod-firebase' | |
| import { useStore } from 'zustand' | |
| import { useSchemaQueryStore } from '@valian/react-kitchen-sink/hooks' | |
| import { collectionsBuilder } from 'zod-firebase' | |
| import { z } from 'zod' |
🤖 Prompt for AI Agents
In packages/react-kitchen-sink/README.md around lines 75 to 78, the Zustand
example imports include unused and incorrect symbols; update the import line to
remove useSchemaDocumentStore, replace createFirestoreFactory and
collectionSchema with collectionsBuilder, and add an import for z from 'zod' so
the snippet matches usage and is copy/paste-ready.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Bug Fixes