Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces FirestoreSnapshotManager-driven React Query integration with an observable-based API. Introduces loading-state operators, refined state unions, and static subject factories. Removes manager, observers, and subject-based queryFn factories. Updates hooks and Zustand stores to emit initial loading states. Adjusts react-query-observable subscription handling and public generics. Documentation and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Component
participant RQ as React Query
participant OQ as observableQueryOptions
participant RX as RxJS Observable
participant FS as Firestore SDK
UI->>RQ: useQuery(options from *SnapshotQueryOptions)
RQ->>OQ: invoke queryFnFromObservableFn(observableFn)
OQ->>RX: observableFn(context)
alt Document ref provided
RX->>FS: fromDocumentRef(ref, snapshotOptions)
FS-->>RX: DocumentSnapshot stream
RX->>RX: pipe(takeUntil(close), documentSnapshotState(listener), startWithDocumentSnapshotLoadingState)
RX-->>RQ: emit Loading → Data/Error states
else Query provided
RX->>FS: fromQuery(query, snapshotOptions)
FS-->>RX: QuerySnapshot stream
RX->>RX: pipe(takeUntil(close), querySnapshotState(listener), startWithQuerySnapshotLoadingState)
RX-->>RQ: emit Loading → Data/Error states
else Disabled
RX-->>RQ: emit Disabled state
end
note over RQ, RX: Cache removal/abort triggers subscription.unsubscribe()<br/>via finalize → cleanup
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 2 inconclusive)❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
Poem
✨ 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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Firestore observable architecture by introducing new operators for loading states and removing the complex FirestoreSnapshotManager in favor of a simpler observable-based approach using the @valian/react-query-observable package.
Key Changes:
- Adds
startWithQuerySnapshotLoadingStateandstartWithDocumentSnapshotLoadingStateoperators to provide initial loading states - Refactors state interfaces to export individual state types for better type safety
- Replaces custom query functions with observable-based query options using the new
react-query-observablepackage
Reviewed Changes
Copilot reviewed 59 out of 64 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rxjs-firebase/src/operators/startWithQuerySnapshotLoadingState.ts | New operator to provide initial loading state for query snapshots |
| packages/rxjs-firebase/src/operators/startWithDocumentSnapshotLoadingState.ts | New operator to provide initial loading state for document snapshots |
| packages/rxjs-firebase/src/states/QuerySnapshotState.ts | Exports state interfaces and adds readonly modifiers |
| packages/rxjs-firebase/src/subjects/QuerySnapshotSubject.ts | Adds factory method for creating subjects from queries |
| packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts | Refactored to use observableQueryOptions instead of custom queryFn |
| packages/react-query-observable/src/queryFn/observableQueryFn.ts | Simplified observable query function implementation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/react-kitchen-sink/package.json (1)
32-39: Pin all dependency and devDependency versions to exact releases
The following packages across multiple package.json files currently use caret ranges (e.g.^1.2.3), violating our “no ranges” policy. Update each to the exact version number and replace anyworkspace:*refs withworkspace:<exact>where applicable:
- @commitlint/cli
- @commitlint/config-conventional
- @lerna-lite/cli, @lerna-lite/publish, @lerna-lite/version
- @types/react, @types/react-dom
- @vitest/coverage-v8, @vitest/eslint-plugin, vitest, vitest-mock-extended
- eslint, jsdom, prettier, publint
- ts-node, tsdown, typescript
- @testing-library/react
- firebase, react, react-dom, rxjs
- observable-hooks, type-fest, @tanstack/react-query
packages/rxjs-firebase/src/subjects/__tests__/DocumentSnapshotSubject.test.ts (1)
109-116: Fix incorrect expectation: BehaviorSubject is closed after complete()
DocumentSnapshotSubject.complete()callssuper.complete(), sosubject.closedshould be true.Apply:
- expect(subject.closed).toBe(false) // BehaviorSubject doesn't close when notification$ completes + expect(subject.closed).toBe(true)packages/rxjs-firebase/src/operators/__tests__/querySnapshot.test.ts (1)
6-11: Mock path must match import path to actually stub fromQueryYou import from the barrel
../../sourcebut mock../../source/fromQuery. This can bypass the mock. Align them.Apply:
-import { fromQuery } from '../../source' +import { fromQuery } from '../../source/fromQuery'(Keep
vi.mock('../../source/fromQuery')as-is.)packages/rxjs-firebase/src/subjects/__tests__/QuerySnapshotSubject.test.ts (1)
141-148: Fix assertion: BehaviorSubject is closed after complete()
BehaviorSubject.complete()setsclosedto true. The current expectation will fail.Apply:
- expect(subject.closed).toBe(false) // BehaviorSubject doesn't close when notification$ completes + expect(subject.closed).toBe(true)packages/rxjs-firebase/src/operators/documentSnapshotState.ts (1)
36-46:snapshot.data()may beundefined; tighten typing under theexists()branch.TS doesn’t narrow
data()based onexists(). Use a non-null assertion or cast to satisfyDocumentSnapshotDataState.data.- data: snapshot.data(), + data: snapshot.data()!, } as const satisfies DocumentSnapshotDataState<AppModelType, DbModelType>packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (1)
18-33: Query options interface extends waitForData controls but they’re unusedQueryFnFromQuerySnapshotSubjectFactoryOptions is included but not honored. Either implement the behavior or drop from the type.
Example wiring:
-import { of } from 'rxjs' +import { of, filter, first, timeout } from 'rxjs' @@ - : fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( - querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), - ), + : (() => { + let stream$ = fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( + querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), + ) + if (props.waitForData) { + stream$ = stream$.pipe( + filter((s: any) => s?.size > 0), + first(), + ...(props.waitForDataTimeout ? [timeout(props.waitForDataTimeout)] as const : []), + ) + } + return stream$ + })(),
🧹 Nitpick comments (44)
packages/rxjs-firebase/src/states/DocumentSnapshotState.ts (2)
3-10: Making Disabled/Loading states public is good; add a discriminant to simplify narrowingMultiple boolean flags complicate narrowing. A single status: 'disabled' | 'loading' | ... makes downstream code cleaner and safer.
-export interface DocumentSnapshotDisabledState { +export interface DocumentSnapshotDisabledState { + status: 'disabled' snapshot?: undefined exists?: undefined isLoading: false hasError: false disabled: true data?: undefined } -export interface DocumentSnapshotLoadingState { +export interface DocumentSnapshotLoadingState { + status: 'loading' snapshot?: undefined exists?: undefined isLoading: true hasError: false disabled: false data?: undefined }Also applies to: 12-19
54-60: Union updated to non-generic error variant — good; consider adding a top-level discriminant for all variantsThis will simplify exhaustive switches and prevent boolean-flag combinations from slipping through.
-export type DocumentSnapshotState<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData> = +export type DocumentSnapshotState<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData> = | DocumentSnapshotDisabledState | DocumentSnapshotLoadingState | DocumentSnapshotErrorState | DocumentDoesNotExistState<AppModelType, DbModelType> | DocumentSnapshotDataState<AppModelType, DbModelType>Follow-up: add status: 'error' | 'not-exists' | 'data' to the remaining variants for uniform narrowing.
packages/react-kitchen-sink/src/rxjs/__tests__/schemaQuerySnapshotSubject.test.ts (1)
19-19: Spy should be stubbed to avoid invoking the real static factoryAs written, spyOn will call through and construct a real QuerySnapshotSubject, which may subscribe and touch Firestore helpers with a dummy query object. Stub the spy and restore mocks after the test to avoid brittle behavior.
Apply:
+import { EMPTY } from 'rxjs' ... - vi.spyOn(QuerySnapshotSubject, 'fromQuery') + const subjectMock = new QuerySnapshotSubject(EMPTY as any) + vi.spyOn(QuerySnapshotSubject, 'fromQuery').mockReturnValue(subjectMock as any)And update the instance expectation:
- expect(subject).toBeInstanceOf(QuerySnapshotSubject) + expect(subject).toBe(subjectMock)Optionally add at file bottom or in a surrounding describe:
+import { afterEach } from 'vitest' +afterEach(() => vi.restoreAllMocks())packages/zustand-firestore/src/hooks/useQueryStore.ts (1)
44-47: Stabilize listener dependencies to avoid stale handlersquerySnapshotState(options) captures onSnapshot/onError/onComplete from props but the observable isn’t recreated when those change. Either memoize a listener or include the handlers in the dependency array.
Minimal change:
- const snapshotState$ = useObservable( + const snapshotState$ = useObservable( (inputs$) => inputs$.pipe( switchMap(([query, snapshotOptions]) => { @@ - return fromQuery(query, snapshotOptions).pipe( - querySnapshotState(options), + return fromQuery(query, snapshotOptions).pipe( + querySnapshotState({ + onSnapshot: options.onSnapshot, + onError: options.onError, + onComplete: options.onComplete, + }), startWithQuerySnapshotLoadingState<AppModelType, DbModelType>(), ) }), ), - [options.query, snapshotListenOptions], + [options.query, snapshotListenOptions, options.onSnapshot, options.onError, options.onComplete], )packages/zustand-firestore/src/hooks/useDocumentStore.ts (1)
41-44: Prevent stale listener capture in document pipelineMirror the query-store change to keep handlers current.
- return fromDocumentRef(ref, snapshotOptions).pipe( - documentSnapshotState(options), + return fromDocumentRef(ref, snapshotOptions).pipe( + documentSnapshotState({ + onSnapshot: options.onSnapshot, + onError: options.onError, + onComplete: options.onComplete, + }), startWithDocumentSnapshotLoadingState<AppModelType, DbModelType>(), )And update dependencies:
- [options.ref, snapshotListenOptions], + [options.ref, snapshotListenOptions, options.onSnapshot, options.onError, options.onComplete],packages/react-kitchen-sink/src/hooks/useSchemaDocumentStore.ts (1)
45-66: Avoid stale listeners in sentry-wrapped document pipelinesentryDocumentSnapshotListener(ref, options) captures handlers; include them in deps or memoize the wrapped listener.
Either memoize:
const listener = useMemo( () => sentryDocumentSnapshotListener(ref, options), [ref, options.onSnapshot, options.onError, options.onComplete], )and pass listener to documentSnapshotState, or extend the dependency array with the three handler refs so the observable recomputes when they change.
packages/react-query-observable/src/observableQueryOptions.ts (1)
21-21: Type narrowing for observableFn matches new signature—confirm callsites updatedEnsure all observableQueryOptions usages now supply (ctx) => Observable with the new ObservableQueryFunction<TQueryFnData, TQueryKey> shape exposed by ./types.
Optionally avoid leaking the non-React Query prop into queryOptions:
-export const observableQueryOptions = <...>(options: ObservableQueryOptions<...>) => - queryOptions({ - queryFn: queryFnFromObservableFn(options.observableFn), +export const observableQueryOptions = <...>(options: ObservableQueryOptions<...>) => { + const { observableFn, ...rest } = options + return queryOptions({ + queryFn: queryFnFromObservableFn(observableFn), staleTime: ({ state }) => (state.dataUpdateCount === 0 || state.status === 'error' ? 0 : Infinity), gcTime: 10_000, - ...options, + ...rest, }) +}packages/rxjs-firebase/src/subjects/__tests__/DocumentSnapshotSubject.test.ts (1)
3-6: Reset vitest-mock-extended mocks between tests for isolationTop-level mocks aren’t reset; add
mockResetinbeforeEachand import it.Apply:
-import { mock } from 'vitest-mock-extended' +import { mock, mockReset } from 'vitest-mock-extended'beforeEach(() => { snapshot$ = new Subject<DocumentSnapshot>() + mockReset(mockDocumentSnapshot) + mockReset(nonExistingSnapshot) + vi.clearAllMocks() })Also applies to: 28-30
packages/react-kitchen-sink/src/hooks/useSchemaQueryStore.ts (1)
64-68: Generics are inferable here (nit)Type args to
startWithQuerySnapshotLoadingStatecan be omitted; inference from upstreamquerySnapshotStateis sufficient.Apply:
- startWithQuerySnapshotLoadingState< - SchemaDocumentOutput<TCollectionSchema, TOptions>, - SchemaDocumentInput<TCollectionSchema> - >(), + startWithQuerySnapshotLoadingState(),packages/rxjs-firebase/src/subjects/QuerySnapshotSubject.ts (2)
1-1: Filename violates unicorn/filename-caseRename file to kebab-case and update imports.
Example:
- QuerySnapshotSubject.ts → query-snapshot-subject.ts
29-31: Getter return type subtlety (nit)State union includes
data: []for loading/disabled. Cast to a readonly array to avoid potential TS friction.Apply:
- get data(): AppModelType[] { - return this.value.data - } + get data(): readonly AppModelType[] { + return this.value.data as readonly AppModelType[] + }packages/rxjs-firebase/src/operators/__tests__/documentSnapshotState.test.ts (1)
70-79: Rename misleading test title.The test named “should start with initial state” now asserts a final data emission; rename to reflect the new behavior.
- it('should start with initial state', () => { + it('should emit final data state without initial loading', () => {packages/rxjs-firebase/src/operators/__tests__/querySnapshotState.test.ts (1)
73-79: Align test name with behavior.Update the “start with initial state” title to reflect final-state-first emission.
- it('should start with initial state', () => { + it('should emit final list state without initial loading', () => {packages/rxjs-firebase/src/subjects/DocumentSnapshotSubject.ts (1)
1-6: Fix module resolution and filename casing (lint blockers).
- ESLint reports import-x/no-unresolved for '@firebase/firestore' and 'rxjs'. Verify these are declared where this package resolves deps (dependencies or peerDependencies) and that TS path mapping is correct. If you don’t intentionally depend on '@firebase/*', prefer 'firebase/firestore' for types to avoid internal package coupling.
- Rule unicorn/filename-case flags this filename; rename to document-snapshot-subject.ts to satisfy kebab-case.
packages/react-kitchen-sink/src/rxjs/__tests__/documentSnapshotSubject.test.ts (2)
4-4: Import matchers from the dedicated entry.Per guidelines, import argument matchers from vitest-mock-extended/matchers.
Apply:
-import { anyObject, mock } from 'vitest-mock-extended' +import { mock } from 'vitest-mock-extended' +import { anyObject } from 'vitest-mock-extended/matchers'
10-10: Stub the static factory to avoid real Firestore side effects.The spy currently calls through, which will subscribe and invoke onSnapshot via the library. Mock the implementation to keep the test hermetic and fast.
Apply:
- vi.spyOn(DocumentSnapshotSubject, 'fromDocumentRef') + const noop$ = new Subject<DocumentSnapshot>() + vi.spyOn(DocumentSnapshotSubject, 'fromDocumentRef').mockImplementationOnce(() => new DocumentSnapshotSubject(noop$))Add the necessary imports (outside this hunk):
import { Subject } from 'rxjs' import { type DocumentSnapshot } from '@firebase/firestore'Also consider:
import { afterEach } from 'vitest' afterEach(() => vi.restoreAllMocks())packages/react-kitchen-sink/src/react-query/__tests__/querySnapshotQueryOptions.test.ts (1)
22-26: Tidy up timers and client between tests.Add teardown to prevent cross-test leakage and restore globals.
You can add:
import { afterEach } from 'vitest' afterEach(() => { queryClient.clear() // or queryClient.destroy() if available in your version vi.useRealTimers() vi.restoreAllMocks() })packages/rxjs-firebase/src/subjects/__tests__/QuerySnapshotSubject.test.ts (1)
33-35: Reset mocks and clean up subjects between testsAdd mock cleanup to avoid cross-test leakage and finalize the Subject.
import { beforeEach, describe, expect, it, vi } from 'vitest' -import { mock } from 'vitest-mock-extended' +import { mock } from 'vitest-mock-extended' ... let snapshot$: Subject<QuerySnapshot> - beforeEach(() => { - snapshot$ = new Subject<QuerySnapshot>() - }) + beforeEach(() => { + vi.clearAllMocks() + snapshot$ = new Subject<QuerySnapshot>() + }) + + afterEach(() => { + snapshot$.complete() + })Also applies to: 9-10, 144-159
packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts (2)
1-1: File naming violates kebab-case ruleIf you enforce
unicorn/filename-case, rename tocatch-query-snapshot-error.tsor adjust ESLint config for this package.
14-27: Clarify completion semantics on error replacementYou emit an error state without completing. If intentional (to keep the stream open after errors), add a brief comment; otherwise, return a completing observable.
Option A (document current behavior):
- catchError( - () => + catchError( + () => new Observable<QuerySnapshotErrorState>((subscriber) => { subscriber.next({ size: 0, empty: true, isLoading: false, hasError: true, disabled: false, data: [], }) }), ),Option B (complete after emitting once):
- new Observable<QuerySnapshotErrorState>((subscriber) => { - subscriber.next({ ... }) - }), + of<QuerySnapshotErrorState>({ + size: 0, + empty: true, + isLoading: false, + hasError: true, + disabled: false, + data: [], + }),packages/rxjs-firebase/src/operators/__tests__/documentSnapshot.test.ts (2)
16-21: Reset mocks with vi.clearAllMocks is good; consider also using mockReset for vitest-mock-extended mocksFor stricter isolation per guidelines,
mockReset(mockObserver)or reset any top-level mocks you reuse across tests.
57-73: Disabled-state path is correct; assert no inner subscriptionOptionally assert
fromDocumentRefwasn’t called when input is null/undefined.flush() expect(mockObserver.next).toHaveBeenNthCalledWith(1, disabled) expect(mockObserver.next).toHaveBeenNthCalledWith(2, disabled) expect(mockObserver.complete).toHaveBeenCalledAfter(mockObserver.next) + expect(fromDocumentRef).not.toHaveBeenCalled()packages/rxjs-firebase/src/operators/startWithQuerySnapshotLoadingState.ts (2)
1-1: File naming violates kebab-case ruleConsider
start-with-query-snapshot-loading-state.tsto satisfyunicorn/filename-case.
20-27: Initial loading state constant duplicationConsider centralizing this object (e.g., export
initialQuerySnapshotLoadingState) to avoid drift across operators/tests.- startWith({ + startWith(initialQuerySnapshotLoadingState),(Define
initialQuerySnapshotLoadingStatein../states/QuerySnapshotStateor a constants module.)packages/react-kitchen-sink/src/react-query/__tests__/schemaDocumentSnapshotQueryOptions.test.ts (2)
21-25: Tidy up QueryClient between testsDestroy or clear the client to avoid cache bleed/flaky tests.
beforeEach(() => { queryClient = new QueryClient() }) + +afterEach(() => { + queryClient.clear() + vi.clearAllMocks() +})
50-59: Null id path: prefer explicit null typing to avoid double-castAvoid
null as unknown as string. Passid: nulland type the options param generically to acceptstring | null.- id: null as unknown as string, + id: null,(Adjust the generic in
schemaDocumentSnapshotQueryOptionsif needed to allowid: string | null.)packages/rxjs-firebase/src/operators/querySnapshot.ts (1)
27-36: Nit: explicitly set snapshot to undefined in disabled statePurely for clarity/self-documentation (typing already enforces it).
Apply:
- return of({ + return of({ + snapshot: undefined, empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], } as const satisfies QuerySnapshotDisabledState)packages/rxjs-firebase/src/operators/startWithDocumentSnapshotLoadingState.ts (1)
25-29: Nit: include optional fields explicitly in loading stateAdds readability; no behavior change.
startWith({ + snapshot: undefined, + exists: undefined, isLoading: true, hasError: false, disabled: false, + data: undefined, } as const satisfies DocumentSnapshotLoadingState),packages/react-kitchen-sink/src/react-query/__tests__/documentSnapshotQueryOptions.test.ts (1)
22-24: Reset mocks and dispose QueryClient per testPrevents test-order coupling and memory leaks.
Apply:
-import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ beforeEach(() => { queryClient = new QueryClient() + vi.clearAllMocks() }) + + afterEach(() => { + queryClient.clear() + })packages/rxjs-firebase/src/states/QuerySnapshotState.ts (3)
33-41: Prefer readonly arrays for immutability guaranteesExpose
dataasReadonlyArray<AppModelType>to prevent accidental mutation by consumers while preserving type inference.-export interface QuerySnapshotDataState<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData> { +export interface QuerySnapshotDataState<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData> { readonly snapshot: QuerySnapshot<AppModelType, DbModelType> readonly empty: boolean readonly size: number readonly isLoading: false readonly hasError: false readonly disabled: false - readonly data: AppModelType[] + readonly data: ReadonlyArray<AppModelType> }
23-31: Clarify Error state's invariants (size/empty) or narrow typesCurrent
QuerySnapshotErrorStateusesempty: booleanandsize: number, butcatchQuerySnapshotErroremitsempty: trueandsize: 0. If we don't plan to surface last-known values in Error, narrow the interface to reflect actual emissions; otherwise, document that these may mirror prior data.export interface QuerySnapshotErrorState { readonly snapshot?: undefined - readonly empty: boolean - readonly size: number + readonly empty: true + readonly size: 0 readonly isLoading: false readonly hasError: true readonly disabled: false readonly data: [] }
1-1: Verify Firestore import sourceImporting types from '@firebase/firestore' can couple us to internal packages. If feasible, prefer 'firebase/firestore' for public surface types.
packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
68-71: Avoid deleting a newer subscription from the mapIf a previous subscription’s finalize runs after a new one is set for the same key, unconditional
deletemight remove the new entry. The diff above guards with pointer equality.packages/rxjs-firebase/src/operators/querySnapshotState.ts (3)
2-2: Resolve ESLint import error: add rxjs as a dependencySame unresolved import for
rxjshere; ensure the workspace/package declares it.
36-36: Confirm converter usage to satisfy AppModelType
doc.data()is typed via Firestore converters; ensure callers supply a converter soAppModelTypeis respected. If not guaranteed, consideras AppModelTypeor validating at the boundary.
43-44: Ensure catchQuerySnapshotError completesThe current implementation (see catchQuerySnapshotError.ts) emits an error state but never completes, which can keep downstream finalize/complete semantics from running. Consider returning
of(errorState)or callingsubscriber.complete()afternext.packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (1)
45-56: Redundant disabled emission.Because
enabled: !!query, the!query ? of(disabled...)branch will never run. Either:
- keep as defensive code (harmless), or
- simplify the observable to assume
queryexists.- observableFn: () => - !query - ? of({ - empty: true, - size: 0, - isLoading: false, - hasError: false, - disabled: true, - data: [], - } as const satisfies QuerySnapshotState<AppModelType, DbModelType>) - : fromQuery(query, snapshotOptions).pipe(querySnapshotState(listener)), + observableFn: () => fromQuery(query!, snapshotOptions).pipe(querySnapshotState(listener)),packages/react-kitchen-sink/src/react-query/__tests__/schemaQuerySnapshotQueryOptions.test.ts (4)
23-27: Tighten test isolation: reset mocks, timers, and QueryClientAdd cleanup to avoid cross-test bleed (guidelines call for resetting mocks in beforeEach/afterEach).
Apply:
import { beforeEach, describe, expect, it, vi } from 'vitest' -import { mock } from 'vitest-mock-extended' +import { mock, mockReset } from 'vitest-mock-extended' @@ beforeEach(() => { queryClient = new QueryClient() - vi.useFakeTimers() - Date.now = vi.fn(() => 1_482_363_367_071) + vi.useFakeTimers() + vi.setSystemTime(1_482_363_367_071) }) + + afterEach(() => { + vi.clearAllMocks() + vi.useRealTimers() + queryClient.clear() + queryClient.destroy() + })Note: If you hoist shared mocks to the describe scope, call mockReset(mockInstance) in beforeEach per guidelines.
26-26: Prefer setSystemTime over stubbing Date.nowWith fake timers active, vi.setSystemTime is the intended API and auto-restores with useRealTimers.
- Date.now = vi.fn(() => 1_482_363_367_071) + vi.setSystemTime(1_482_363_367_071)
29-37: Factory mock placement (optional)Creating the factory inside the test is fine. If reused across tests, hoist to describe scope and type as DeepMocked<SchemaFirestoreQueryFactory> for clearer intent and to enable mockReset in beforeEach.
42-57: Complete the Subject to avoid dangling subscriptionsCompleting the mocked Subject keeps subscriptions tidy even if future changes wait for completion.
subject.next(snapshot) - await expect(promise).resolves.toEqual({ + await expect(promise).resolves.toEqual({ snapshot, size: 2, empty: false, isLoading: false, hasError: false, disabled: false, data: [{ id: '1' }, { id: '2' }], }) + subject.complete()packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (1)
58-59: Redundant gcTimeobservableQueryOptions already sets gcTime: 10_000. Remove duplication unless you intend to override it here.
- gcTime: 10_000,packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (2)
48-61: Optionally emit an initial loading state for better UXIf consumers expect an immediate loading emission, prepend startWithQuerySnapshotLoadingState before mapping to state.
-import { fromQuery, querySnapshotState } from '@valian/rxjs-firebase' +import { fromQuery, querySnapshotState } from '@valian/rxjs-firebase' +import { startWithQuerySnapshotLoadingState } from '@valian/rxjs-firebase/operators' @@ - : fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( - querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), - ), + : fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( + startWithQuerySnapshotLoadingState(), + querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), + ),
63-63: Redundant gcTimeobservableQueryOptions sets gcTime: 10_000 already. Remove unless overriding.
- gcTime: 10_000,
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (6)
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts (4)
1-1: Use public Firebase import and fix filename case (build blocker).
- Replace private
@firebase/firestorewith publicfirebase/firestore.- Rename file to
catch-document-snapshot-error.tsand update imports across the repo.-import { type DocumentData } from '@firebase/firestore' +import { type DocumentData } from 'firebase/firestore'
2-2: DropObservableimport; useof(pairs with refactor below). Also verifyrxjsis declared.This import will change after using
of(...)insidecatchError.-import { catchError, Observable, type OperatorFunction } from 'rxjs' +import { catchError, of, type OperatorFunction } from 'rxjs'Run to confirm deps in the package manifest(s):
#!/bin/bash set -euo pipefail fd -a '^package\.json$' packages/rxjs-firebase | while read -r pkg; do echo "== $pkg ==" jq -r ' "name: " + (.name // "?"), "version: " + (.version // "?"), "deps.rxjs: " + (.dependencies.rxjs // "—"), "peer.rxjs: " + (.peerDependencies.rxjs // "—"), "dev.rxjs: " + (.devDependencies.rxjs // "—"), "peer.firebase: " + (.peerDependencies.firebase // "—") ' "$pkg" done
10-16: Emit-once-and-complete error state; avoid dangling subscription.Model the error state as a value and emit with
of(...)rather than a customnew Observable(...)that never completes.-const ERROR_STATE = new Observable<DocumentSnapshotErrorState>((subscriber) => { - subscriber.next({ - isLoading: false, - hasError: true, - disabled: false, - }) -}) +const ERROR_STATE: DocumentSnapshotErrorState = { + isLoading: false, + hasError: true, + disabled: false, +}
26-26: Returnof(ERROR_STATE)insidecatchError.Ensures a single emission and completion.
->(): OperatorFunction<TState, TState | DocumentSnapshotErrorState> => catchError(() => ERROR_STATE) +>(): OperatorFunction<TState, TState | DocumentSnapshotErrorState> => + catchError(() => of(ERROR_STATE))packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts (2)
6-15: Fallback observable never completes; replace with a plain value constantThe module-scoped Observable emits but does not complete, leaving subscribers hanging on error paths. Emit a single value that completes via of(...).
Apply:
-const ERROR_STATE = new Observable<QuerySnapshotErrorState>((subscriber) => { - subscriber.next({ - size: 0, - empty: true, - isLoading: false, - hasError: true, - disabled: false, - data: [], - }) -}) +const ERROR_STATE: QuerySnapshotErrorState = { + size: 0, + empty: true, + isLoading: false, + hasError: true, + disabled: false, + data: [], +}
17-21: Ensure catchError returns a completing fallback streamReturn of(ERROR_STATE) so consumers receive the error state and the stream completes deterministically.
Apply:
->(): OperatorFunction<State, State | QuerySnapshotErrorState> => catchError(() => ERROR_STATE) +>(): OperatorFunction<State, State | QuerySnapshotErrorState> => catchError(() => of(ERROR_STATE))
🧹 Nitpick comments (5)
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts (1)
10-16: Optional: align constant naming with codebase conventions.If the repo enforces camelCase for constants, consider
errorState. Otherwise, keep UPPER_SNAKE for “true” constants.-const ERROR_STATE: DocumentSnapshotErrorState = { +const errorState: DocumentSnapshotErrorState = { isLoading: false, hasError: true, disabled: false, }And update usage:
- catchError(() => of(ERROR_STATE)) + catchError(() => of(errorState))packages/rxjs-firebase/README.md (4)
185-195: Optional: show generics for stronger typing in the example.Helps users see how to type snapshot data.
-import { documentSnapshot } from '@valian/rxjs-firebase' +import { documentSnapshot } from '@valian/rxjs-firebase' // ... -of(userRef /* or null */) - .pipe(documentSnapshot()) +of(userRef /* or null */) + .pipe(documentSnapshot<User>())
202-211: Optional: mirror generics in query example.-import { querySnapshot } from '@valian/rxjs-firebase' +import { querySnapshot } from '@valian/rxjs-firebase' // ... -of(todosQuery /* or null */) - .pipe(querySnapshot()) +of(todosQuery /* or null */) + .pipe(querySnapshot<Todo>())
218-222: Clarify 'await' usage in examples.Top-level await may not be valid in users’ setups; mark as inside an async function.
-const exists = await documentExists(fromDocumentRef(doc(db, 'users', userId)).pipe(documentSnapshotState()), 5_000) +// inside an async function: +const exists = await documentExists(fromDocumentRef(doc(db, 'users', userId)).pipe(documentSnapshotState()), 5_000)
229-232: Same 'await' clarification for waitForData.-const ready = await waitForData(state$) +// inside an async function: +const ready = await waitForData(state$)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/react-kitchen-sink/README.md(3 hunks)packages/rxjs-firebase/README.md(3 hunks)packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts(1 hunks)packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-kitchen-sink/README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Ensure all new code conforms to the project's ESLint and Prettier rules
**/*.{ts,tsx}: Follow standard TypeScript naming conventions for variables, functions, and React components
Variable and function names should be camelCase
Files:
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.tspackages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts
packages/rxjs-firebase/**/*.ts
📄 CodeRabbit inference engine (packages/rxjs-firebase/GEMINI.md)
Implement library code in TypeScript
Files:
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.tspackages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/typescript-usage-rule.mdc)
**/*.ts: Utilize TypeScript features to ensure type safety
Prefer interfaces over types when defining object shapes
Use generics to create reusable components and functions
Enforce strict typing and avoid 'any' type as much as possible
Files:
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.tspackages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: valian-ca/react-firebase#0
File: packages/rxjs-firebase/GEMINI.md:0-0
Timestamp: 2025-08-16T16:32:27.169Z
Learning: Applies to packages/rxjs-firebase/**/*.ts : Implement library code in TypeScript
📚 Learning: 2025-08-16T16:32:27.169Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: packages/rxjs-firebase/GEMINI.md:0-0
Timestamp: 2025-08-16T16:32:27.169Z
Learning: Applies to packages/rxjs-firebase/**/*.ts : Implement library code in TypeScript
Applied to files:
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.tspackages/rxjs-firebase/README.mdpackages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts
📚 Learning: 2025-08-16T16:32:15.795Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: packages/react-firestore/GEMINI.md:0-0
Timestamp: 2025-08-16T16:32:15.795Z
Learning: Applies to packages/react-firestore/**/*.{ts,tsx} : Ensure all code conforms to the project’s ESLint rules
Applied to files:
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts
🧬 Code graph analysis (2)
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts (1)
packages/rxjs-firebase/src/states/DocumentSnapshotState.ts (3)
DocumentSnapshotErrorState(21-28)DocumentSnapshotDataState(42-52)DocumentDoesNotExistState(30-40)
packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts (1)
packages/rxjs-firebase/src/states/QuerySnapshotState.ts (2)
QuerySnapshotErrorState(23-31)QuerySnapshotDataState(33-41)
🪛 ESLint
packages/rxjs-firebase/src/operators/catchDocumentSnapshotError.ts
[error] 1-1: Filename is not in kebab case. Rename it to catch-document-snapshot-error.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@firebase/firestore'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts
[error] 1-1: Filename is not in kebab case. Rename it to catch-query-snapshot-error.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@firebase/firestore'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
🔇 Additional comments (6)
packages/rxjs-firebase/README.md (5)
37-43: Good reframe of primary operators section.Clear, discoverable entry point for users migrating from prior APIs.
61-64: Immediate loading example reads well.Operator order here is fine; the helper will sync-emit before the first snapshot.
85-88: Also update the helper import example block to use public Firebase path (consistency).Mirror the import-path fix in all query examples in this section.
Would you like me to sweep-replace all occurrences of
@firebase/firestore→firebase/firestorein the README?
259-266: Fix RxJS and Firebase imports; align operator imports with v7/8 style.
- Import
mapfromrxjs(tree-shakable path), notrxjs/operators.- Add missing Firebase imports used below.
[ suggest_optional_refactor ]-import { combineLatest } from 'rxjs' -import { map } from 'rxjs/operators' +import { combineLatest, map } from 'rxjs' +import { doc, collection, query, where } from 'firebase/firestore' // + ensure `db` is defined
139-147: Confirm BehaviorSubject wording matches implementation.Docs say these are
BehaviorSubjects. If they’re wrappers exposingObservableonly, adjust wording.Would you like me to open a follow-up to sync README wording with
DocumentSnapshotSubject/QuerySnapshotSubjectimplementations?Also applies to: 165-166
packages/rxjs-firebase/src/operators/catchQuerySnapshotError.ts (1)
17-21: Generics and return type look solidOperatorFunction<State, State | QuerySnapshotErrorState> is appropriate and keeps downstream typing precise.
Nr9
force-pushed
the
feat/firestore-observable
branch
from
1704ebf to
63a98f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/rxjs-firebase/README.md (1)
241-251: Add missing Firestore imports and clarify db scope in Dynamic Queries example.Snippet uses
query,collection, andwherebut doesn’t import them;dbis also implicit.Apply within the snippet:
-import { combineLatest, switchMap } from 'rxjs' +import { combineLatest, switchMap } from 'rxjs' +import { collection, query, where } from 'firebase/firestore' // ensure `db` is in scope function createFilteredTodos(userId$: Observable<string>, completed$: Observable<boolean>) { return combineLatest([userId$, completed$]).pipe( switchMap(([userId, completed]) => { - const q = query(collection(db, 'todos'), where('userId', '==', userId), where('completed', '==', completed)) + const q = query( + collection(db, 'todos'), + where('userId', '==', userId), + where('completed', '==', completed), + ) return fromQuery(q).pipe(querySnapshotState()) }), ) }Optionally note where
dbcomes from (e.g., “assumes an initialized Firestore instance db”).packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (1)
2-5: ESLint import resolution errors — configure TS resolver for workspaceImports are flagged as unresolved by ESLint. Configure the TypeScript resolver to point at both root and package tsconfigs.
Add to
packages/react-kitchen-sink/eslint.config.mjs:export default [ { files: ['**/*.ts', '**/*.tsx'], languageOptions: { /* ... */ }, + settings: { + 'import/resolver': { + typescript: { + project: ['../../tsconfig.json', './tsconfig.json'], + }, + }, + }, }, ]packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (1)
2-10: ESLint import resolution errors — configure TS resolver for workspaceSame unresolved-import errors here; apply the resolver settings update in this package to fix.
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (1)
2-3: ESLint: unresolved imports for workspace packages — configure TypeScript resolver.These modules exist in the workspace, but ESLint can’t resolve them. Add the import resolver config and ensure the resolver is installed.
- settings.import/resolver: node: {} and typescript: { project: './tsconfig.json' }
- devDeps: eslint-import-resolver-typescript
Run to verify the config is present:
#!/bin/bash # Show ESLint config files and search for import/resolver fd -a 'eslint.config.*|.eslintrc.*' | xargs -I{} sh -c 'echo --- {}; cat {}' rg -n "import/resolver|eslint-import-resolver-typescript" -SAlso applies to: 5-10
🧹 Nitpick comments (5)
packages/react-kitchen-sink/README.md (2)
39-41: Use consistent import paths for options buildersExamples mix
@valian/react-kitchen-sink/react-queryand@valian/react-kitchen-sink. Pick one surface and use it throughout to reduce confusion.Would you like to standardize all examples on the root export (preferred) if everything is re-exported there?
Also applies to: 73-74
149-152: Self-contained snippet: defineuserId
userIdis referenced but not defined in this block. Add a stub to keep the snippet copy-pasteable.-import { schemaQuerySnapshotSubject } from '@valian/react-kitchen-sink' +import { schemaQuerySnapshotSubject } from '@valian/react-kitchen-sink' + +const userId = 'user123'packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (2)
56-58: Emit an initial loading state for better UX and README parityDocs show checking
data.isLoading. Consider emitting a loading state before the first snapshot to align behavior.-import { fromQuery, querySnapshotState } from '@valian/rxjs-firebase' +import { fromQuery, querySnapshotState, startWithQuerySnapshotLoadingState } from '@valian/rxjs-firebase' @@ - : fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( - querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), - ), + : fromQuery(factory.prepare(query, snapshotOptions), snapshotOptions).pipe( + startWithQuerySnapshotLoadingState(), + querySnapshotState(sentrySchemaQuerySnapshotListener(factory.collectionName, query, listener)), + ),Please verify the exact export path for
startWithQuerySnapshotLoadingState(package root vs. operators subpath).
28-29: Avoid conflating schema meta options with Firestore listen options
snapshotOptions?: TOptions & SnapshotListenOptionscouples unrelated option domains and can complicate typing. Prefer separate params.Proposed shape:
schemaOptions?: TOptionssnapshotOptions?: SnapshotListenOptionsThen call:
factory.prepare(query, schemaOptions)andfromQuery(..., snapshotOptions).packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (1)
33-39: Prevent accidental enable when query is null.Current spread order lets props.enabled override enabled: !!query. Safer to gate on query first.
Apply:
observableQueryOptions<QuerySnapshotState<AppModelType, DbModelType>, TError, TData, TQueryKey>({ observableFn: () => !query ? of({ empty: true, size: 0, isLoading: false, hasError: false, disabled: true, data: [], } as const satisfies QuerySnapshotState<AppModelType, DbModelType>) : fromQuery(query, snapshotOptions).pipe(querySnapshotState(listener)), - enabled: !!query, - gcTime: 10_000, - ...props, + ...props, + enabled: !!query && (props.enabled ?? true), + gcTime: props.gcTime ?? 10_000, meta: { ...props.meta, type: 'snapshot', query, }, })Also applies to: 51-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/react-kitchen-sink/README.md(3 hunks)packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts(2 hunks)packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts(2 hunks)packages/react-kitchen-sink/src/react-query/schemaDocumentSnapshotQueryOptions.ts(2 hunks)packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts(3 hunks)packages/rxjs-firebase/README.md(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-kitchen-sink/src/react-query/schemaDocumentSnapshotQueryOptions.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Ensure all new code conforms to the project's ESLint and Prettier rules
**/*.{ts,tsx}: Follow standard TypeScript naming conventions for variables, functions, and React components
Variable and function names should be camelCase
Files:
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.tspackages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.tspackages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/typescript-usage-rule.mdc)
**/*.ts: Utilize TypeScript features to ensure type safety
Prefer interfaces over types when defining object shapes
Use generics to create reusable components and functions
Enforce strict typing and avoid 'any' type as much as possible
Files:
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.tspackages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.tspackages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts
🧠 Learnings (2)
📚 Learning: 2025-08-16T16:32:15.795Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: packages/react-firestore/GEMINI.md:0-0
Timestamp: 2025-08-16T16:32:15.795Z
Learning: Applies to packages/react-firestore/**/*.{ts,tsx} : Ensure all code conforms to the project’s ESLint rules
Applied to files:
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.tspackages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts
📚 Learning: 2025-08-16T16:32:27.169Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: packages/rxjs-firebase/GEMINI.md:0-0
Timestamp: 2025-08-16T16:32:27.169Z
Learning: Applies to packages/rxjs-firebase/**/*.ts : Implement library code in TypeScript
Applied to files:
packages/rxjs-firebase/README.md
🧬 Code graph analysis (3)
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (4)
packages/react-query-observable/src/observableQueryOptions.ts (2)
ObservableQueryOptions(6-22)observableQueryOptions(24-37)packages/react-kitchen-sink/src/rxjs/types.ts (2)
SchemaQuerySnapshotState(21-24)SchemaQuerySnapshotStateListener(44-50)packages/rxjs-firebase/src/operators/querySnapshotState.ts (1)
querySnapshotState(17-44)packages/react-kitchen-sink/src/sentry/sentrySchemaQuerySnapshotListener.ts (1)
sentrySchemaQuerySnapshotListener(6-43)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (5)
packages/react-query-observable/src/observableQueryOptions.ts (2)
ObservableQueryOptions(6-22)observableQueryOptions(24-37)packages/rxjs-firebase/src/states/DocumentSnapshotState.ts (1)
DocumentSnapshotState(54-59)packages/rxjs-firebase/src/operators/documentSnapshotState.ts (2)
DocumentSnapshotStateListener(12-23)documentSnapshotState(25-61)packages/rxjs-firebase/src/subjects/DocumentSnapshotSubject.ts (1)
fromDocumentRef(41-47)packages/react-kitchen-sink/src/sentry/sentryDocumentSnapshotListener.ts (1)
sentryDocumentSnapshotListener(5-41)
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (4)
packages/react-query-observable/src/observableQueryOptions.ts (2)
ObservableQueryOptions(6-22)observableQueryOptions(24-37)packages/rxjs-firebase/src/states/QuerySnapshotState.ts (1)
QuerySnapshotState(43-47)packages/rxjs-firebase/src/operators/querySnapshotState.ts (2)
QuerySnapshotStateListener(8-15)querySnapshotState(17-44)packages/rxjs-firebase/src/subjects/QuerySnapshotSubject.ts (1)
fromQuery(39-45)
🪛 ESLint
packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts
[error] 2-2: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 3-3: Unable to resolve path to module '@valian/react-query-observable'.
(import-x/no-unresolved)
[error] 4-4: Unable to resolve path to module '@valian/rxjs-firebase'.
(import-x/no-unresolved)
[error] 5-5: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts
[error] 2-2: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 3-3: Unable to resolve path to module '@valian/react-query-observable'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@valian/rxjs-firebase'.
(import-x/no-unresolved)
[error] 10-10: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts
[error] 2-2: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 3-3: Unable to resolve path to module '@valian/react-query-observable'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@valian/rxjs-firebase'.
(import-x/no-unresolved)
[error] 10-10: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
🔇 Additional comments (13)
packages/rxjs-firebase/README.md (7)
46-55: Good fix: use public Firebase imports.Using
firebase/firestorehere is correct for docs and aligns with the public SDK.
71-80: Good fix: public path for Firestore query helpers.
firebase/firestoreis the right module path forcollection,query, andwhere.
101-106: Good fix: public Firestore import.
docimported fromfirebase/firestoreis correct.
112-117: Good fix: public Firestore imports.
collection,orderBy,query,whereimported fromfirebase/firestoreis correct.
142-151: Good fix: DocumentSnapshotSubject example imports.Using
docfromfirebase/firestoreis correct.
162-170: Good fix: QuerySnapshotSubject example imports.
collectionandqueryfromfirebase/firestoreis correct.
218-223: Good fix: documentExists example import.
docfromfirebase/firestoreis correct.packages/react-kitchen-sink/src/react-query/schemaQuerySnapshotQueryOptions.ts (1)
45-55: LGTM on observable wiring and metaThe observableFn,
enabledderivation, andmetasurface look solid and consistent with the new API.Also applies to: 59-68
packages/react-kitchen-sink/src/react-query/documentSnapshotQueryOptions.ts (2)
41-51: Enabled vs. disabled-state emission — confirm intentYou emit a disabled state via
of({... disabled: true })and also setenabled: !!ref. If consumers overrideenabledin...props, the disabled emission is useful; otherwise it’s unreachable. Confirm this dual-path is intentional.
41-59: Solid migration to observableQueryOptionsConstruction of the observable, Sentry-wired listener, and
meta.documentReflook correct. Nice use ofas const satisfiesto keep state precise.packages/react-kitchen-sink/src/react-query/querySnapshotQueryOptions.ts (3)
19-21: Good: narrow option surface by omitting observableFn.Cleanly reuses ObservableQueryOptions while preventing callers from supplying a conflicting observableFn.
40-50: Disabled-state fallback looks correct.of({... disabled: true }) provides a consistent state when query is absent and keeps consumers from handling undefined. Nice use of satisfies for type safety.
55-59: Meta enrichment LGTM.type: 'snapshot' and echoing query into meta are helpful for devtools/diagnostics.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores