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. WalkthroughIntroduces a new package packages/react-query-observable integrating RxJS Observables with TanStack React Query, including implementation, tests, configs, and docs. Updates repository rules and tooling. Adjusts keywords in react-kitchen-sink, bumps an ESLint config version, and removes two re-exports from rxjs-firebase operators. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Caller
participant QC as QueryClient
participant QF as queryFnFromObservableFn
participant OBS as Observable$
participant QCA as QueryCache
participant REG as queriesSubscriptions
UI->>QC: fetchQuery({ queryKey, queryFn })
QC->>QF: invoke(queryFn, { queryKey, signal, client })
QF->>REG: unsubscribe existing (by hash) if any
QF->>OBS: subscribe()
QF-->>QC: Promise resolves on first next(value)
OBS-->>QC: next(value) (subsequent)
QC->>QC: setQueryData(queryKey, value)
alt Error before first value
OBS-->>QF: error(err)
QF-->>QC: reject(err)
else Error after first value
OBS-->>QCA: locate query by key
QCA->>QCA: update state to error, idle, clear data
note right of QCA: If query not found, throw "Query not found"
end
rect rgba(230, 240, 255, 0.5)
note over QCA,REG: Cleanup on removal/abort
QCA-->>QF: notify removal (matching key)
QF->>OBS: unsubscribe()
QF->>REG: delete(key)
end
opt Abort before first value
QC-->>QF: signal.abort
QF-->>QC: reject("Query aborted")
QF->>OBS: unsubscribe()
QF->>REG: delete(key)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.Please share your feedback with us on this Discord post. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.cursor/rules/package-management.mdc (1)
41-43: Fix typo and clarify pinning requirement.Update wording to match enforced policy.
-## Dependencies and Dev Dependencies - -- use `pnpm` to manage dependencies and dev dependencies. -- dependencies are specified wit pinned versions. +## Dependencies and Dev Dependencies + +- Use `pnpm` to manage dependencies and devDependencies. +- Use pinned (exact) versions for dependencies and devDependencies (no ^ or ~ ranges).
🧹 Nitpick comments (37)
packages/react-query-observable/.gitignore (1)
1-2: Ignore patterns OK; consider directory-form for clarity.Using trailing slashes makes intent explicit.
-coverage -lib +coverage/ +lib/packages/react-query-observable/tsconfig.json (1)
1-4: Add excludes to avoid scanning build/coverage outputs.Prevents tsc from walking lib/coverage if present.
{ "extends": "../../tsconfig.json", - "include": ["@types", "src"] + "include": ["@types", "src"], + "exclude": ["lib", "coverage", "dist"] }.cursor/rules/typescript-usage-rule.mdc (1)
3-3: Include TSX files in rule coverage
Update the glob to cover both .ts and .tsx per repo guidelines.
Apply:-globs: **/*.ts +globs: **/*.{ts,tsx}packages/react-query-observable/.npmignore (1)
1-13: Ignore tsdown.config.mjs in .npmignore
The package contains tsdown.config.mjs but it isn’t ignored—add it to prevent publishing your build config. (Optionally add tsdown.config.cjs if you ever emit a CJS config.)packages/react-query-observable/.npmignore GEMINI.md -tsdown.config.json +tsdown.config.json +tsdown.config.mjs.cursor/rules/naming-conventions-rule.mdc (1)
3-3: Globs should cover TSX since rules mention React component naming.Current globs exclude .tsx, so PascalCase enforcement won’t apply to components.
-globs: **/*.ts +globs: **/*.{ts,tsx}packages/react-query-observable/tsdown.config.mjs (2)
3-9: Enable type declarations generation (dts) to ship .d.ts files.If this package exposes types (likely), add dts: true so tsdown outputs declarations alongside JS. Decls can also be auto-enabled when package.json has "types", but being explicit avoids surprises. (tsdown.dev)
Apply:
export default defineConfig({ outDir: 'lib', platform: 'browser', format: ['esm', 'cjs'], target: 'es2020', + dts: true, fixedExtension: true, })
3-9: Optional: consider importing from 'tsdown/config' to satisfy strict resolvers.Docs show using defineConfig from 'tsdown/config'. If your resolver still flags 'tsdown', switching to this path can placate import rules across tools. Keep only if the linter still complains after adding the dependency. (tsdown.dev)
Possible change:
- import { defineConfig } from 'tsdown' + import { defineConfig } from 'tsdown/config'packages/react-query-observable/CHANGELOG.md (4)
5-5: Polish phrasing: start bullet with capital letter.“remove the use...” → “Remove the use...”.
Apply:-- remove the use of rxjs `of` for catchError ([#42](https://github.com/valian-ca/react-firebase/pull/42)) +- Remove the use of RxJS `of` for catchError ([#42](https://github.com/valian-ca/react-firebase/pull/42))
11-29: Package name appears copy/pasted from a different library.Each “version bump only” line references @valian/rxjs-firebase, but this changelog is for @valian/react-query-observable. Please correct to avoid confusion.
Patch:-This was a version bump only for @valian/rxjs-firebase to align it with other projects, there were no code changes. +This was a version bump only for @valian/react-query-observable to align it with other projects; there were no code changes.Repeat the same correction for 1.0.8, 1.0.7, 1.0.6, 1.0.5, and 1.0.4 entries.
35-37: Possible typo: “react-kitchen-sin”.If the intended project is “react-kitchen-sink”, fix the spelling.
-- @valian/react-kitchen-sin +- @valian/react-kitchen-sink
43-44: Move the “Changelog” header to the top.Conventional changelogs start with the H1 title at the top, followed by entries.
Suggested structure:
- Line 1: “# Changelog”
- Then versions in descending order.
packages/react-query-observable/eslint.config.mjs (3)
3-3: The disable comment may be unnecessary.import-x/no-rename-default is meant to prevent “import { default as x } ...”. You’re using a standard default import. Consider removing the disable to keep the config clean.
-// eslint-disable-next-line import-x/no-rename-default import vitest from '@vitest/eslint-plugin'
14-20: Ensure import.meta.dirname is supported in your Node version.Some environments may not yet support import.meta.dirname. If CI/node lints fail, compute it via fileURLToPath/dirname.
+import { fileURLToPath } from 'node:url' +import { dirname } from 'node:path' +const __dirname = dirname(fileURLToPath(import.meta.url)) ... projectService: true, - tsconfigRootDir: import.meta.dirname, + tsconfigRootDir: __dirname,
26-30: Rules scoped to .ts only; is .tsx intentionally excluded?If you want consistent filename and type-definition style for TSX too, mirror these rules for .tsx. If not, ignore.
- files: ['**/*.ts'], + files: ['**/*.{ts,tsx}'],.cursor/rules/tests.mdc (2)
78-80: Fix the URL string example (it currently contains Markdown link syntax).In code, use a plain string, not a markdown link.
-configStub.getApiUrl.mockReturnValue('[https://api.test.com](https://api.test.com)'); +configStub.getApiUrl.mockReturnValue('https://api.test.com');
28-35: Add a reminder to restore spies when using vi.spyOn.mockReset handles vitest-mock-extended mocks; spies still need vi.restoreAllMocks() to avoid cross-test leakage.
You could append:beforeEach(() => { vi.restoreAllMocks(); // for vi.spyOn })packages/react-query-observable/package.json (3)
37-54: Optional: add "files" and "sideEffects" to optimize publish footprint and treeshaking.Not required, but recommended for libraries.
Example:
"publishConfig": { "access": "public", "exports": { ".": { "types": { "import": "./lib/index.d.mts", "require": "./lib/index.d.cts" }, "import": "./lib/index.mjs", "require": "./lib/index.cjs", "default": "./lib/index.mjs" } }, "main": "./lib/index.cjs", "module": "./lib/index.mjs", "types": "./lib/index.d.mts" }, + "files": ["lib", "README.md", "LICENSE", "CHANGELOG.md"], + "sideEffects": false,
59-61: Engine constraint is strict; widen if possible for broader consumption.If the library doesn’t rely on Node 22-only features, consider supporting Node >=18 (still LTS widely in the wild).
55-58: Remove top-level fields pointing to .ts in package.json (packages/react-query-observable/package.json L55-58)
publishConfig already overrides exports, main, module and types to the built files; dropping the root entries avoids TS editor and Node-runtime resolution issues outside the workspace.packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
1-3: Use a type-only import for Unsubscribable
Switch to:import type { Unsubscribable } from 'rxjs' export const queriesSubscriptions = new Map<string, Unsubscribable>()Or, to remove the rxjs dependency entirely, inline a minimal structural type:
type UnsubscribableLike = { unsubscribe(): void } export const queriesSubscriptions = new Map<string, UnsubscribableLike>()Ensure rxjs is hoisted or installed at the workspace root (or added as a devDependency here) so ESLint’s import resolver can locate it.
packages/react-query-observable/src/__tests__/queriesWithObservable.test.ts (1)
12-16: Use identity assertion for Map value.Since the same object reference is retrieved,
toBeis slightly stricter than deep-equal.- expect(queriesSubscriptions.get('key')).toEqual({ unsubscribe }) + expect(queriesSubscriptions.get('key')).toBe(({ unsubscribe } as unknown as { unsubscribe: () => void }))Alternatively, keep the object in a variable to avoid the cast:
- const unsubscribe = vi.fn() - queriesSubscriptions.set('key', { unsubscribe }) - expect(queriesSubscriptions.get('key')).toEqual({ unsubscribe }) + const unsubscribe = vi.fn() + const entry = { unsubscribe } + queriesSubscriptions.set('key', entry) + expect(queriesSubscriptions.get('key')).toBe(entry)packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts (1)
50-58: Add an override test to prove user options win over defaults.You already test gcTime override; consider also asserting custom retry and staleTime override.
Example:
it('should allow overriding gcTime option', () => { const options = observableQueryOptions({ queryKey, observableFn: () => of('value'), gcTime: 1234, }) expect(options.gcTime).toBe(1234) }) + + it('should allow overriding retry and staleTime', () => { + const options = observableQueryOptions({ + queryKey, + observableFn: () => of('value'), + retry: 1, + staleTime: 42, + }) + expect(options.retry).toBe(1) + expect(options.staleTime).toBe(42) + })packages/react-query-observable/src/types.ts (1)
4-6: Type alias looks good; optional naming nit.Consider
TData(orTQueryFnData) to align with TanStack’s naming.-export type ObservableQueryFunction<T = unknown, TQueryKey extends QueryKey = QueryKey> = ( +export type ObservableQueryFunction<TData = unknown, TQueryKey extends QueryKey = QueryKey> = ( context: QueryFunctionContext<TQueryKey>, -) => Observable<T> +) => Observable<TData>packages/react-query-observable/README.md (2)
96-107: Use consistent operator imports (RxJS 7 allows from 'rxjs').You import operators from both 'rxjs' and 'rxjs/operators'. Prefer one style for consistency; importing from 'rxjs' is fine in v7.
-import { map, retry } from 'rxjs/operators' +import { map, retry } from 'rxjs'
16-18: Document peer installs explicitly to reduce setup friction.Since these are peers, add a one-liner showing how to install them alongside this package.
### Peer Dependencies - - `@tanstack/react-query` ^5.80.0 - `rxjs` ^7.8.0 + +Install peers (example): +```bash +pnpm add @tanstack/react-query rxjs +```packages/react-query-observable/src/__tests__/observableQueryFn.test.ts (5)
13-16: Add afterEach cleanup to avoid cache/timer leakage.Destroy/clear the client between tests to prevent cross-test effects.
- beforeEach(() => { + beforeEach(() => { queryClient = new QueryClient() queriesSubscriptions.clear() }) + afterEach(() => { + queryClient.clear() + queriesSubscriptions.clear() + })
54-58: Avoid relying on Subject.observed; assert behavior instead.
observedisn’t always reliable/typed across RxJS versions. Verify unsubscription by asserting the first subject can no longer affect cache.- // previous subject should have been unsubscribed - expect(firstSubject.observed).toBe(false) - expect(secondSubject.observed).toBe(true) + // previous subject should have been unsubscribed (no further cache updates) + const before = queryClient.getQueryData(queryKey) + firstSubject.next(999) + expect(queryClient.getQueryData(queryKey)).toBe(before)
105-127: Nit: also assert subscribe() was wired and callback invoked once.Minor strengthening: verify we actually subscribed to the cache and invoked the captured callback.
- // Simulate a 'removed' event from the cache to trigger cleanup - const captured = subscribeSpy.mock.calls.at(-1)?.[0] - captured?.({ type: 'removed', query: { queryKey } } as QueryCacheNotifyEvent) + // Simulate a 'removed' event from the cache to trigger cleanup + expect(subscribeSpy).toHaveBeenCalledTimes(1) + const notify = subscribeSpy.mock.calls[0]?.[0] + expect(typeof notify).toBe('function') + notify?.({ type: 'removed', query: { queryKey } } as QueryCacheNotifyEvent)
1-129: Consider adding tests for AbortSignal cancellation and “error after first value with active query”.Two valuable scenarios:
- Aborted query should unsubscribe and reject as canceled.
- Error after first value with a registered query should transition the query state to error (no “Query not found”).
I can draft these tests if you want to cover them in this PR.
1-129: Hook AbortSignal in queryFn for proper cancellation semantics.Currently, cancellation doesn’t unsubscribe or reject, which can leak a subscription. Tie
context.signaltounsubscribe()and reject as canceled.Example (in src/queryFn/observableQueryFn.ts; pseudo-diff):
const subcription = observable$.subscribe({ /* ... */ }) const unsubscribeFromQueryCache = context.client.getQueryCache().subscribe(/* ... */) const unsubscribe = () => { unsubscribeFromQueryCache() subcription.unsubscribe() queriesSubscriptions.delete(queryKeyHash) } queriesSubscriptions.set(queryKeyHash, { unsubscribe }) + + // Abort handling + if (context.signal.aborted) { + unsubscribe() + // Prefer TanStack's CancelledError if available for your version + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors + reject(new Error('Aborted')) + return + } + const onAbort = () => { + unsubscribe() + // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors + reject(new Error('Aborted')) + } + context.signal.addEventListener('abort', onAbort, { once: true })If your TanStack version exports a CancelledError, use that instead of
new Error('Aborted')to mark the query as canceled rather than errored.packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
18-18: Typo: rename “subcription” to “subscription”.Improves readability and avoids future confusion.
- const subcription = observable$.subscribe({ + const subscription = observable$.subscribe({ ... - subcription.unsubscribe() + subscription.unsubscribe()Also applies to: 65-65
12-14: Scope subscriptions per QueryClient to avoid cross-client collisions.Using a single Map keyed only by hashed queryKey can clash when multiple QueryClients use the same key. Prefer a WeakMap<QueryClient, Map<string, Unsubscribable>>.
If acceptable, I can update
queriesWithObservable.tsand this file to:// queriesWithObservable.ts export const queriesSubscriptions = new WeakMap<QueryClient, Map<string, Unsubscribable>>()And here:
const client = context.client let clientMap = queriesSubscriptions.get(client) if (!clientMap) { clientMap = new Map() queriesSubscriptions.set(client, clientMap) } clientMap.get(queryKeyHash)?.unsubscribe() // later clientMap.set(queryKeyHash, { unsubscribe })Also applies to: 68-69
packages/react-query-observable/src/__tests__/client.test.ts (4)
12-15: Prefer fake timers to patching Date.now directly.Use vi.useFakeTimers/vi.setSystemTime for isolation and auto-restore.
- Date.now = vi.fn(() => 1_482_363_367_071) + vi.useFakeTimers() + vi.setSystemTime(1_482_363_367_071)Remember to restore in afterEach:
afterEach(() => vi.useRealTimers())
234-245: Grammar: “should unsubscribe …”Minor wording fix for clarity.
-it('should unsubscribes from the observable when the query is removed from the cache', async () => { +it('should unsubscribe from the observable when the query is removed from the cache', async () => {
17-64: Add a test for abort behavior.Given long-lived subscriptions, we should ensure the query respects AbortSignal and cleans up on abort.
I can add a spec like:
- start fetchQuery; immediately abort via query.cancelQueries({ queryKey }); assert promise rejects with “aborted” and subject.observed is false.
66-94: Test early-complete path.Cover “observable completes without emitting” to assert the promise rejects and no leaks remain in queriesSubscriptions.
packages/react-query-observable/src/observableQueryOptions.ts (1)
34-40: Defaults look good; consider documenting rationale.
- retry: false and staleTime: Infinity after first data are sensible for push-based sources.
- gcTime: 10_000 is aggressive; if streams are bursty, you may want to surface this in README and expose an override example.
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.cursor/rules/naming-conventions-rule.mdc(1 hunks).cursor/rules/nx-rules.mdc(1 hunks).cursor/rules/package-management.mdc(1 hunks).cursor/rules/tests.mdc(1 hunks).cursor/rules/typescript-usage-rule.mdc(1 hunks)package.json(1 hunks)packages/react-kitchen-sink/package.json(1 hunks)packages/react-query-observable/.gitignore(1 hunks)packages/react-query-observable/.npmignore(1 hunks)packages/react-query-observable/CHANGELOG.md(1 hunks)packages/react-query-observable/LICENSE(1 hunks)packages/react-query-observable/README.md(1 hunks)packages/react-query-observable/eslint.config.mjs(1 hunks)packages/react-query-observable/package.json(1 hunks)packages/react-query-observable/src/__tests__/client.test.ts(1 hunks)packages/react-query-observable/src/__tests__/observableQueryFn.test.ts(1 hunks)packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts(1 hunks)packages/react-query-observable/src/__tests__/queriesWithObservable.test.ts(1 hunks)packages/react-query-observable/src/index.ts(1 hunks)packages/react-query-observable/src/observableQueryOptions.ts(1 hunks)packages/react-query-observable/src/queryFn/observableQueryFn.ts(1 hunks)packages/react-query-observable/src/queryFn/queriesWithObservable.ts(1 hunks)packages/react-query-observable/src/types.ts(1 hunks)packages/react-query-observable/tsconfig.json(1 hunks)packages/react-query-observable/tsdown.config.mjs(1 hunks)packages/react-query-observable/vitest.config.mjs(1 hunks)
🧰 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
Files:
packages/react-query-observable/src/index.tspackages/react-query-observable/src/__tests__/observableQueryOptions.test.tspackages/react-query-observable/src/__tests__/queriesWithObservable.test.tspackages/react-query-observable/src/queryFn/queriesWithObservable.tspackages/react-query-observable/src/queryFn/observableQueryFn.tspackages/react-query-observable/src/__tests__/client.test.tspackages/react-query-observable/src/types.tspackages/react-query-observable/src/observableQueryOptions.tspackages/react-query-observable/src/__tests__/observableQueryFn.test.ts
package.json
📄 CodeRabbit inference engine (.cursor/rules/package-management.mdc)
Root package.json must declare pnpm as the packageManager and define monorepo workspaces
Files:
package.json
{package.json,**/package.json}
📄 CodeRabbit inference engine (.cursor/rules/package-management.mdc)
Use pinned (exact) versions for dependencies and devDependencies in all package.json files (no ^ or ~ ranges)
Files:
package.jsonpackages/react-kitchen-sink/package.jsonpackages/react-query-observable/package.json
🧠 Learnings (27)
📚 Learning: 2025-08-16T16:32:59.589Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/typescript-usage-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:59.589Z
Learning: Applies to src/**/*.{ts,tsx} : Utilize TypeScript features to ensure type safety
Applied to files:
packages/react-query-observable/tsconfig.json.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:32:59.589Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/typescript-usage-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:59.589Z
Learning: Applies to src/**/*.{ts,tsx} : Enforce strict typing and avoid 'any' type as much as possible
Applied to files:
packages/react-query-observable/tsconfig.json.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 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/**/*.{test,spec}.{ts,tsx} : All new features must be accompanied by Jest tests
Applied to files:
packages/react-query-observable/.npmignorepackages/react-query-observable/src/__tests__/observableQueryFn.test.ts
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Applies to package.json : Root package.json must declare pnpm as the packageManager and define monorepo workspaces
Applied to files:
.cursor/rules/package-management.mdc.cursor/rules/nx-rules.mdc
📚 Learning: 2025-08-16T16:31:55.693Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: GEMINI.md:0-0
Timestamp: 2025-08-16T16:31:55.693Z
Learning: Use pnpm for all package-related operations in this monorepo
Applied to files:
.cursor/rules/package-management.mdc.cursor/rules/nx-rules.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Use pnpm for all dependency and devDependency management in the monorepo
Applied to files:
.cursor/rules/package-management.mdc
📚 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: Use pnpm for all package management operations
Applied to files:
.cursor/rules/package-management.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Applies to pnpm-lock.yaml : Maintain a single consolidated pnpm-lock.yaml at the repository root kept up to date via pnpm install
Applied to files:
.cursor/rules/package-management.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Applies to pnpm-workspace.yaml : Define workspace structure in pnpm-workspace.yaml at the repository root
Applied to files:
.cursor/rules/package-management.mdc.cursor/rules/nx-rules.mdc
📚 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: Use pnpm for package management and scripts (build, test, lint, type-check)
Applied to files:
.cursor/rules/package-management.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Applies to {package.json,**/package.json} : Use pinned (exact) versions for dependencies and devDependencies in all package.json files (no ^ or ~ ranges)
Applied to files:
.cursor/rules/package-management.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Prefer pnpm commands in docs and scripts (e.g., pnpm install, pnpm add <pkg>, pnpm run <script>, pnpm -r build)
Applied to files:
.cursor/rules/package-management.mdc
📚 Learning: 2025-08-16T16:32:50.728Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/package-management.mdc:0-0
Timestamp: 2025-08-16T16:32:50.728Z
Learning: Use pnpm workspace-aware operations (e.g., pnpm -r <script>, pnpm install --filter <package-name>)
Applied to files:
.cursor/rules/package-management.mdc.cursor/rules/nx-rules.mdc
📚 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-query-observable/eslint.config.mjs.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:32:34.658Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/naming-conventions-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:34.658Z
Learning: Applies to src/**/*.{ts,tsx} : Follow standard TypeScript and JavaScript naming conventions for variables, functions, and components.
Applied to files:
.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:31:55.693Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: GEMINI.md:0-0
Timestamp: 2025-08-16T16:31:55.693Z
Learning: Applies to **/*.{ts,tsx} : Ensure all new code conforms to the project's ESLint and Prettier rules
Applied to files:
.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:32:34.658Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/naming-conventions-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:34.658Z
Learning: Applies to src/**/*.{ts,tsx} : Variable and function names should be camelCase.
Applied to files:
.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:32:34.658Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/naming-conventions-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:34.658Z
Learning: Applies to src/**/*.tsx : Component names should be PascalCase.
Applied to files:
.cursor/rules/naming-conventions-rule.mdc
📚 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} : Write all new code in TypeScript
Applied to files:
.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:31:55.693Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: GEMINI.md:0-0
Timestamp: 2025-08-16T16:31:55.693Z
Learning: Use TypeScript for all new code
Applied to files:
.cursor/rules/naming-conventions-rule.mdc.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-09-03T14:16:52.181Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/nx-rules.mdc:0-0
Timestamp: 2025-09-03T14:16:52.181Z
Learning: If tasks exist, ask which task to help with, then use nx_current_running_task_output to fetch its terminal output
Applied to files:
.cursor/rules/nx-rules.mdc
📚 Learning: 2025-09-03T14:16:52.181Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/nx-rules.mdc:0-0
Timestamp: 2025-09-03T14:16:52.181Z
Learning: If the user wants to rerun a task, always use `nx run <taskId>` to ensure it runs in the Nx context
Applied to files:
.cursor/rules/nx-rules.mdc
📚 Learning: 2025-09-03T14:16:52.181Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/nx-rules.mdc:0-0
Timestamp: 2025-09-03T14:16:52.181Z
Learning: If a task is marked continuous, do not offer to rerun it; instead, use nx_current_running_task_output to verify output
Applied to files:
.cursor/rules/nx-rules.mdc
📚 Learning: 2025-09-03T14:16:52.181Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/nx-rules.mdc:0-0
Timestamp: 2025-09-03T14:16:52.181Z
Learning: Use nx_current_running_tasks_details to list tasks when helping with tasks/commands (test, build, lint, etc.)
Applied to files:
.cursor/rules/nx-rules.mdc
📚 Learning: 2025-09-03T14:16:52.181Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/nx-rules.mdc:0-0
Timestamp: 2025-09-03T14:16:52.181Z
Learning: If the user needs help with an Nx configuration or project graph error, use nx_workspace to get any errors
Applied to files:
.cursor/rules/nx-rules.mdc
📚 Learning: 2025-08-16T16:32:59.589Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/typescript-usage-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:59.589Z
Learning: Applies to src/**/*.{ts,tsx} : Use generics to create reusable components and functions
Applied to files:
.cursor/rules/typescript-usage-rule.mdc
📚 Learning: 2025-08-16T16:32:59.589Z
Learnt from: CR
PR: valian-ca/react-firebase#0
File: .cursor/rules/typescript-usage-rule.mdc:0-0
Timestamp: 2025-08-16T16:32:59.589Z
Learning: Applies to src/**/*.{ts,tsx} : Prefer interfaces over types when defining object shapes
Applied to files:
.cursor/rules/typescript-usage-rule.mdc
🧬 Code graph analysis (6)
packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts (2)
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)packages/react-query-observable/src/observableQueryOptions.ts (1)
observableQueryOptions(26-40)
packages/react-query-observable/src/__tests__/queriesWithObservable.test.ts (1)
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)
packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-6)packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)
packages/react-query-observable/src/__tests__/client.test.ts (2)
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)packages/react-query-observable/src/observableQueryOptions.ts (1)
observableQueryOptions(26-40)
packages/react-query-observable/src/observableQueryOptions.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-6)packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
queryFnFromObservableFn(7-70)
packages/react-query-observable/src/__tests__/observableQueryFn.test.ts (2)
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
queryFnFromObservableFn(7-70)
🪛 ESLint
packages/react-query-observable/src/queryFn/queriesWithObservable.ts
[error] 1-1: Filename is not in kebab case. Rename it to queries-with-observable.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/tsdown.config.mjs
[error] 1-1: Unable to resolve path to module 'tsdown'.
(import-x/no-unresolved)
packages/react-query-observable/src/queryFn/observableQueryFn.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-fn.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
packages/react-query-observable/src/types.ts
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/observableQueryOptions.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-options.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
🪛 LanguageTool
packages/react-query-observable/CHANGELOG.md
[grammar] ~3-~3: There might be a mistake here.
Context: ## 1.1.0 (2025-09-03) ### 🩹 Fixes - remove the use of rxjs of for catchErr...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... ## 1.0.3 (2025-08-16) ### 🚀 Features - react-query integration - @valian/react-...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...# 🚀 Features - react-query integration - @valian/react-kitchen-sin - rxjs firebas...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ... integration - @valian/react-kitchen-sin - rxjs firebase ### ❤️ Thank You - Julie...
(QB_NEW_EN)
packages/react-query-observable/README.md
[grammar] ~21-~21: There might be a mistake here.
Context: ...ables as data sources for TanStack Query - 🧠 Smart caching: First emission res...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... TanStack Query - 🧠 Smart caching: First emission resolves the query; later emis...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...ion resolves the query; later emissions update cache - ♻️ **Automatic subscription cle...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ... the query; later emissions update cache - ♻️ Automatic subscription cleanup: S...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...ption cleanup**: Subscriptions are tied to query lifecycle - 🛡️ *Error handling...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...ubscriptions are tied to query lifecycle - 🛡️ Error handling: Pre-first-value ...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...irst-value errors surface to the runtime - 🧭 Dynamic staleness: staleTime is...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...s Infinity while subscribed, otherwise 0 - 📦 TypeScript first: Strong types fo...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...Strong types for options and observable function ## Quick Start ```ts import { useQuery } f...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...yOptions: - queryFn: generated from observableFn-staleTime: Infinity` when there is an active sub...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...ctive subscription for queryKey, else 0 - retry: false - Refetch-related flags: `refe...
(QB_NEW_EN)
[grammar] ~76-~76: There might be a mistake here.
Context: ...ion for queryKey, else 0 - retry: false - Refetch-related flags: refetchInterval...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...e query promise rejects with that error. - If the observable errors after the first...
(QB_NEW_EN)
🔇 Additional comments (20)
.cursor/rules/package-management.mdc (1)
2-2: Front matter simplification LGTM.Rule remains always-on for Cursor; no issues.
packages/react-query-observable/LICENSE (1)
1-19: MIT license looks correct and complete..cursor/rules/nx-rules.mdc (1)
36-36: LGTM — clarification reads fine.Minor reformatting; content unchanged.
packages/react-query-observable/tsdown.config.mjs (2)
3-9: Pin Node engines to meet tsdown’s minimum (>= 20.19).tsdown requires Node 20.19+. Add an "engines" field in this package.json (or root) so CI/users don’t run incompatible Node. (tsdown.dev)
Add to package.json:
{ "engines": { "node": ">=20.19" } }
3-9: Fixed extension option is valid; LGTM.fixedExtension: true is a supported config (forces .mjs/.cjs). No change needed. (tsdown.dev)
packages/react-query-observable/vitest.config.mjs (3)
5-7: Confirm Node test environment is appropriate.If any tests touch DOM APIs (React Query devtools, window, etc.), switch to 'happy-dom' or 'jsdom'. Otherwise, 'node' keeps tests lean.
You can flip env quickly:- environment: 'node', + environment: 'happy-dom',
8-20: Coverage config is solid; 100% thresholds enforced.Include/exclude patterns look correct for src with barrel indexes excluded.
21-23: Sequence hooks set to 'list' is fine for deterministic lifecycle.Good for avoiding inter-test hook interleaving surprises.
packages/react-query-observable/eslint.config.mjs (2)
33-41: Good test linting setup.vitest recommended config plus targeted rule overrides look right; expect-expect covers expectObservable too.
44-47: d.ts filenames kebab-case rule is a nice touch.Keeps ambient declarations consistent.
.cursor/rules/tests.mdc (3)
84-128: Great, comprehensive matcher guidance.Covers the full set and when to use them; clear and actionable.
131-153: Strong “no any” guidance—approved.The minimal local interface pattern is exactly what we want in this repo.
155-201: Solid coverage of DeepMocked and mocked helpers.Examples demonstrate correct inference restoration; nothing to change.
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
3-3: No cleanup gaps: allqueriesSubscriptions.setcalls in observableQueryFn.ts are paired withunsubscribeanddeleteon removal, and tests clear the registry.packages/react-query-observable/src/index.ts (1)
1-2: LGTM: minimal public surface exported.Exports align with the intended API. Keep internal registries/queryFn helpers private unless explicitly needed.
packages/react-query-observable/src/__tests__/queriesWithObservable.test.ts (1)
3-3: Update import if file is renamed to kebab-case.Keep test path in sync with the suggested filename change.
-import { queriesSubscriptions } from '../queryFn/queriesWithObservable' +import { queriesSubscriptions } from '../queryFn/queries-with-observable'packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts (2)
16-33: LGTM: defaults are validated clearly.Covers queryFn presence, retry false, staleTime 0 before first data, and gcTime default.
7-7: Update import if queries file is renamed to kebab-case.Keep tests consistent with the filename change.
-import { queriesSubscriptions } from '../queryFn/queriesWithObservable' +import { queriesSubscriptions } from '../queryFn/queries-with-observable'packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
1-1: No change required: QueryFunctionContext in v5 includes aclient: QueryClientproperty, socontext.clientwill type-check as expected. (tanstack.com)
1-1: No changes needed for filename or dependency declarations
The file nameobservableQueryFn.tscomplies with the.tsoverride ineslint.config.mjs(camelCase is allowed), and@tanstack/react-queryis already listed in both peerDependencies and devDependencies of this package.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new package @valian/react-query-observable that enables RxJS Observable integration with TanStack React Query. The package provides utilities to use RxJS observables as data sources for React Query with automatic subscription management, smart caching, and error handling.
- Adds a complete package with TypeScript support and comprehensive test coverage
- Implements
observableQueryOptionsfunction that converts RxJS observables to React Query options - Provides automatic subscription lifecycle management and cleanup
Reviewed Changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-query-observable/src/observableQueryOptions.ts | Core function that creates React Query options from RxJS observables |
| packages/react-query-observable/src/queryFn/observableQueryFn.ts | Query function implementation that handles observable subscription and lifecycle |
| packages/react-query-observable/src/types.ts | TypeScript type definitions for observable query functions |
| packages/react-query-observable/src/queryFn/queriesWithObservable.ts | Global map for tracking active subscriptions |
| packages/react-query-observable/src/index.ts | Package exports |
| packages/react-query-observable/src/tests/* | Comprehensive test suite covering all functionality |
| packages/react-query-observable/package.json | Package configuration with dependencies and build setup |
| packages/react-query-observable/*.md | Documentation and licensing |
| packages/react-query-observable/.config. | Build and test configuration files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
packages/react-query-observable/src/types.ts (1)
1-2: Fix ESLint “no-unresolved”: add peer/dev deps and TS import resolver.Declare externals for this package and enable the TS resolver so the linter resolves these imports.
Apply in packages/react-query-observable/package.json:
{ + "peerDependencies": { + "@tanstack/react-query": "^5.80.0", + "rxjs": "^7.8.0" + }, + "devDependencies": { + "@tanstack/react-query": "^5.80.0", + "rxjs": "^7.8.0", + "eslint-import-resolver-typescript": "^3.6.1" + } }And ensure ESLint settings include:
{ + "settings": { + "import/resolver": { + "typescript": true + } + } }Verify with:
#!/bin/bash jq -r '.peerDependencies["@tanstack/react-query"], .peerDependencies["rxjs"]' packages/react-query-observable/package.json jq -r '.devDependencies["@tanstack/react-query"], .devDependencies["rxjs"]' packages/react-query-observable/package.json rg -n --glob 'packages/react-query-observable/src/**/*.ts' -e "from '@tanstack/react-query'" -e "from 'rxjs'"packages/react-query-observable/src/__tests__/observableQueryFn.test.ts (1)
134-145: Always restore rxjsConfig.onUnhandledError in finally.Prevents cross-test bleed if an assertion throws.
- let captured: unknown - const originalHandler = rxjsConfig.onUnhandledError - rxjsConfig.onUnhandledError = (err) => { - captured = err - } - subject.error(new Error('late-error')) - await new Promise<void>((resolve) => { - setTimeout(resolve, 0) - }) - expect((captured as Error).message).toBe('Query not found') - rxjsConfig.onUnhandledError = originalHandler + let captured: unknown + const originalHandler = rxjsConfig.onUnhandledError + try { + rxjsConfig.onUnhandledError = (err) => { + captured = err + } + subject.error(new Error('late-error')) + await new Promise<void>((resolve) => setTimeout(resolve, 0)) + expect((captured as Error).message).toBe('Query not found') + } finally { + rxjsConfig.onUnhandledError = originalHandler + }packages/react-query-observable/src/observableQueryOptions.ts (2)
1-1: Rename file to kebab-case to satisfy unicorn/filename-case.observableQueryOptions.ts → observable-query-options.ts; update all imports/exports accordingly.
1-2: Resolve “no-unresolved” by declaring peer/dev deps.Same as types.ts: add @tanstack/react-query and rxjs as peerDependencies (and devDependencies for local builds).
packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
38-66: Clean up on error/complete and don’t throw when query is missing.
- Ensure the cache listener is detached on error/complete to avoid leaks.
- If the query is gone after first emission, no-op instead of throwing (previously flagged).
error: (error) => { if (!firstValueReturned) { firstValueReturned = true // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors - reject(error) + reject(error) + unsubscribe() } else { // After the first value has been resolved, propagate the error into the query state const query = context.client.getQueryCache().find({ queryKey: context.queryKey }) - if (!query) { - throw new Error('Query not found', { cause: error }) - } else { + if (query) { query.setState({ ...query.state, data: undefined, // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment error, errorUpdateCount: query.state.errorUpdateCount + 1, errorUpdatedAt: Date.now(), fetchStatus: 'idle', isInvalidated: false, status: 'error', }) - } + } + unsubscribe() } }, complete: () => { - if (!firstValueReturned) reject(new Error('Observable completed without returning a value')) + if (!firstValueReturned) { + reject(new Error('Observable completed without returning a value')) + } + unsubscribe() },
🧹 Nitpick comments (1)
packages/react-query-observable/src/observableQueryOptions.ts (1)
33-40: Prevent options from overriding gcTime (if 10_000 is intended as fixed).Currently ...options can overwrite gcTime. Either move ...options before gcTime or omit gcTime from the input type.
Option A (omit from type, keep order):
export interface ObservableQueryOptions< @@ > extends Omit< UnusedSkipTokenOptions<TQueryFnData, TError, TData, TQueryKey>, - | 'queryFn' + | 'queryFn' | 'staleTime' + | 'gcTime' | 'refetchInterval' | 'refetchIntervalInBackground' | 'refetchOnWindowFocus' | 'refetchOnMount' | 'refetchOnReconnect' > {Option B (keep type, enforce default by order):
- queryOptions({ - queryFn: queryFnFromObservableFn(options.observableFn), - staleTime: ({ state }) => (state.dataUpdateCount === 0 || state.status === 'error' ? 0 : Infinity), - gcTime: 10_000, - ...options, - }) + queryOptions({ + ...options, + queryFn: queryFnFromObservableFn(options.observableFn), + staleTime: ({ state }) => (state.dataUpdateCount === 0 || state.status === 'error' ? 0 : Infinity), + gcTime: 10_000, + })
📜 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 (5)
packages/react-query-observable/src/__tests__/observableQueryFn.test.ts(1 hunks)packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts(1 hunks)packages/react-query-observable/src/observableQueryOptions.ts(1 hunks)packages/react-query-observable/src/queryFn/observableQueryFn.ts(1 hunks)packages/react-query-observable/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-query-observable/src/tests/observableQueryOptions.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Ensure all new code conforms to the project's ESLint and Prettier rules
Files:
packages/react-query-observable/src/__tests__/observableQueryFn.test.tspackages/react-query-observable/src/types.tspackages/react-query-observable/src/queryFn/observableQueryFn.tspackages/react-query-observable/src/observableQueryOptions.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/**/*.{test,spec}.{ts,tsx} : All new features must be accompanied by Jest tests
Applied to files:
packages/react-query-observable/src/__tests__/observableQueryFn.test.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/react-query-observable/src/types.ts
🧬 Code graph analysis (3)
packages/react-query-observable/src/__tests__/observableQueryFn.test.ts (2)
packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
queryFnFromObservableFn(14-85)
packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-8)packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)
packages/react-query-observable/src/observableQueryOptions.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-8)packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
queryFnFromObservableFn(14-85)
🪛 ESLint
packages/react-query-observable/src/types.ts
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/queryFn/observableQueryFn.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-fn.ts.
(unicorn/filename-case)
[error] 5-5: 'QueryFunctionContext' is defined but never used. Allowed unused vars must match /^_/u.
(@typescript-eslint/no-unused-vars)
[error] 7-7: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 8-8: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/observableQueryOptions.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-options.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
🔇 Additional comments (5)
packages/react-query-observable/src/types.ts (1)
4-8: Type alias looks solid.Good generics and type-only imports; API surface is clear.
packages/react-query-observable/src/__tests__/observableQueryFn.test.ts (1)
10-123: Test coverage and scenarios look comprehensive.Behavior across first-value resolution, aborts, cache removal, and error propagation is well exercised.
packages/react-query-observable/src/observableQueryOptions.ts (1)
37-38: Stale-time heuristic LGTM.Returns 0 until first success or on error; Infinity thereafter to rely on stream updates.
packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
20-22: Good: pre-emptively unsubscribe previous subscription for this key.Prevents multiple observers per queryKey. LGTM.
1-8: All good—no action required.@tanstack/react-queryandrxjsare correctly declared as peerDependencies (both at ^5.80.0 and ^7.8.0, respectively), and the package’s tsconfig inherits the root bundler resolution, so imports will resolve.
Nr9
force-pushed
the
feat/react-query-observable
branch
from
b72b4da to
121c13a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/react-query-observable/tsdown.config.mjs (1)
1-1: Fix unresolved 'tsdown' import by adding the devDependencyESLint flags import-x/no-unresolved for 'tsdown'. Add tsdown as a devDependency in @valian/react-query-observable (or at the workspace root) so this config resolves. This was raised earlier and still appears unresolved in the latest static analysis.
Run to confirm where tsdown is declared and that this package’s build script references it:
#!/bin/bash set -euo pipefail pkg="packages/react-query-observable/package.json" echo "Build script:" jq -r '.scripts.build // "MISSING"' "$pkg" echo -e "\ntsdown in this package:" jq -r '.devDependencies.tsdown // .dependencies.tsdown // "MISSING"' "$pkg" echo -e "\ntsdown across workspace:" fd -a package.json | xargs -I {} bash -lc 'printf "%s -> %s\n" "{}" "$(jq -r \'.devDependencies.tsdown // .dependencies.tsdown // "MISSING"\' {})"' echo -e "\nIf missing, install (pnpm example):" echo "pnpm -w add -D tsdown" echo "pnpm add -D tsdown -F @valian/react-query-observable"packages/react-query-observable/src/types.ts (1)
1-2: Resolve ESLint “no-unresolved” by declaring peer + dev deps (and TS import resolver).
Same as earlier note; ensure this package declares/hoists externals so types resolve.Add to packages/react-query-observable/package.json:
{ "name": "react-query-observable", + "peerDependencies": { + "@tanstack/react-query": "^5.80.0", + "rxjs": "^7.8.0" + }, + "devDependencies": { + "@tanstack/react-query": "^5.80.0", + "rxjs": "^7.8.0", + "eslint-import-resolver-typescript": "^3.6.1" + } }Ensure ESLint uses the TS resolver (package or repo config):
{ "settings": { "import/resolver": { - // ... + "typescript": true } } }Run to verify:
#!/bin/bash jq '.peerDependencies, .devDependencies' packages/react-query-observable/package.json rg -n "from ['\"]@tanstack/react-query['\"]" packages/react-query-observable rg -n "from ['\"]rxjs['\"]" packages/react-query-observablepackages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
1-1: Rename file to kebab-case to satisfy ESLint.
queriesWithObservable.ts → queries-with-observable.ts; update imports.Apply in observableQueryFn import:
- import { queriesSubscriptions } from './queriesWithObservable' + import { queriesSubscriptions } from './queries-with-observable'Verify references:
#!/bin/bash rg -n "queriesWithObservable" -g "packages/**"packages/react-query-observable/src/observableQueryOptions.ts (1)
1-1: Rename file to kebab-case and fix import to kebab-case query-fn.- import { queryFnFromObservableFn } from './queryFn/observableQueryFn' + import { queryFnFromObservableFn } from './queryFn/observable-query-fn'Also rename this file to observable-query-options.ts and update barrels/tests accordingly:
#!/bin/bash rg -n "observableQueryOptions|observableQueryFn" -g "packages/**"packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
1-1: Rename file to kebab-case and update import sites.
observableQueryFn.ts → observable-query-fn.ts; fix imports in observableQueryOptions.ts.#!/bin/bash rg -n "observableQueryFn" -g "packages/**"
8-79: Make cleanup idempotent, remove throw on missing query, and always clean listeners to prevent leaks.
Prevents dangling cache/abort listeners and avoids unhandled exceptions after GC.-import { type Observable } from 'rxjs' +import { type Observable, type Unsubscribable } from 'rxjs' @@ export const queryFnFromObservableFn = <T = unknown, TObservable extends Observable<T> = Observable<T>, TQueryKey extends QueryKey = QueryKey>( observableFn: ObservableQueryFunction<T, TObservable, TQueryKey>, onUnsubscribe?: (observable$: TObservable) => void, ): QueryFunction<T, TQueryKey> => - async (context) => { + async (context) => { const queryKeyHash = hashKey(context.queryKey) queriesSubscriptions.get(queryKeyHash)?.unsubscribe() - return new Promise<T>((resolve, reject) => { + return new Promise<T>((resolve, reject) => { if (context.signal.aborted) { reject(new Error('Query aborted')) return } - let firstValueReturned = false - const observable$ = observableFn(context) - const subscription = observable$.subscribe({ + let firstValueReturned = false + let unsubscribed = false + let unsubscribeFromQueryCache: () => void = () => {} + let abortListener: (() => void) | undefined + const observable$ = observableFn(context) + let subscription: Unsubscribable | undefined + const unsubscribe = () => { + if (unsubscribed) return + unsubscribed = true + if (abortListener) context.signal.removeEventListener('abort', abortListener) + unsubscribeFromQueryCache() + subscription?.unsubscribe() + queriesSubscriptions.delete(queryKeyHash) + onUnsubscribe?.(observable$) + } + subscription = observable$.subscribe({ next: (data) => { if (!firstValueReturned) { firstValueReturned = true resolve(data) } else { context.client.setQueryData<T>(context.queryKey, data) } }, error: (error) => { if (!firstValueReturned) { firstValueReturned = true // eslint-disable-next-line @typescript-eslint/prefer-promise-reject-errors - reject(error) + reject(error) + unsubscribe() } else { // After the first value has been resolved, propagate the error into the query state const query = context.client.getQueryCache().find({ queryKey: context.queryKey }) - if (!query) { - throw new Error('Query not found', { cause: error }) - } else { + if (query) { query.setState({ ...query.state, data: undefined, // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment error, errorUpdateCount: query.state.errorUpdateCount + 1, errorUpdatedAt: Date.now(), fetchStatus: 'idle', isInvalidated: false, status: 'error', }) - } + } + unsubscribe() } }, complete: () => { - if (!firstValueReturned) reject(new Error('Observable completed without returning a value')) + if (!firstValueReturned) { + reject(new Error('Observable completed without returning a value')) + } + unsubscribe() }, }) - const unsubscribeFromQueryCache = context.client.getQueryCache().subscribe((event: QueryCacheNotifyEvent) => { + unsubscribeFromQueryCache = context.client.getQueryCache().subscribe((event: QueryCacheNotifyEvent) => { if (event.type === 'removed' && queryKeyHash === hashKey(event.query.queryKey as QueryKey)) { unsubscribe() } }) - const abortListener = () => { + abortListener = () => { unsubscribe() if (!firstValueReturned) reject(new Error('Query aborted')) } context.signal.addEventListener('abort', abortListener, { once: true }) - const unsubscribe = () => { - unsubscribeFromQueryCache() - subscription.unsubscribe() - queriesSubscriptions.delete(queryKeyHash) - context.signal.removeEventListener('abort', abortListener) - onUnsubscribe?.(observable$) - } queriesSubscriptions.set(queryKeyHash, { unsubscribe }) }) }
🧹 Nitpick comments (7)
packages/react-query-observable/tsdown.config.mjs (1)
3-9: Switch to neutral platform, externalize peers, add sourcemaps, and update package.json packaging
- tsdown.config.mjs:
• changeplatform: 'browser'→platform: 'neutral'
• addexternal: ['@tanstack/react-query', 'rxjs'], sourcemap: true,- package.json:
• add"files": ["lib"]to ensure only your build is published
• replacewith"exports": "./src/index.ts""exports": { ".": { "import": "./lib/index.js", "require": "./lib/index.cjs" } }- peerDependencies already list
@tanstack/react-queryandrxjs—no unbundled peers remain.packages/react-query-observable/src/observableQueryOptions.ts (1)
22-23: Optional: expose onUnsubscribe hook through options.
Lets callers clean external resources when the query is GC’d/aborted.export interface ObservableQueryOptions< TQueryFnData = unknown, TError = DefaultError, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, > extends Omit< @@ > { - observableFn: ObservableQueryFunction<TQueryFnData, Observable<TQueryFnData>, TQueryKey> + observableFn: ObservableQueryFunction<TQueryFnData, Observable<TQueryFnData>, TQueryKey> + onUnsubscribe?: (observable$: Observable<TQueryFnData>) => void } @@ queryOptions({ - queryFn: queryFnFromObservableFn(options.observableFn), + queryFn: queryFnFromObservableFn(options.observableFn, options.onUnsubscribe), staleTime: ({ state }) => (state.dataUpdateCount === 0 || state.status === 'error' ? 0 : Infinity), gcTime: 10_000, ...options, })Also applies to: 34-35
packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
13-13: Nit: drop redundant async.
The function returns a manually constructed Promise; async is unnecessary.- async (context) => { + (context) => {packages/react-query-observable/eslint.config.mjs (4)
2-3: Drop the unnecessary disable comment for import-x/no-rename-default.No default import rename is occurring here; remove the directive unless a specific false-positive is observed.
import { config } from '@valian/eslint-config' -// eslint-disable-next-line import-x/no-rename-default import vitest from '@vitest/eslint-plugin'
10-11: Make ignores resilient when linting from repo root.Use recursive patterns so the package stays ignored regardless of CWD.
- ignores: ['coverage/', 'dist/', 'lib/'], + ignores: ['**/coverage/**', '**/dist/**', '**/lib/**'],
26-31: Apply rules to TSX as well.Ensure filename-case and consistent-type-definitions cover .tsx files.
- files: ['**/*.ts'], + files: ['**/*.{ts,tsx}'], rules: { '@typescript-eslint/consistent-type-definitions': ['error', 'interface'], 'unicorn/filename-case': ['error', { cases: { camelCase: true, pascalCase: true } }], },
44-47: Confirm intent: kebab-case for .d.ts but camel/Pascal for others.If uniformity is desired, align filename-case across TS/TSX and D.TS; otherwise, keep as-is.
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.cursor/rules/naming-conventions-rule.mdc(1 hunks).cursor/rules/package-management.mdc(1 hunks).cursor/rules/tests.mdc(1 hunks).cursor/rules/typescript-usage-rule.mdc(1 hunks)package.json(1 hunks)packages/react-kitchen-sink/package.json(1 hunks)packages/react-query-observable/.gitignore(1 hunks)packages/react-query-observable/.npmignore(1 hunks)packages/react-query-observable/CHANGELOG.md(1 hunks)packages/react-query-observable/LICENSE(1 hunks)packages/react-query-observable/README.md(1 hunks)packages/react-query-observable/eslint.config.mjs(1 hunks)packages/react-query-observable/package.json(1 hunks)packages/react-query-observable/src/__tests__/client.test.ts(1 hunks)packages/react-query-observable/src/__tests__/observableQueryFn.test.ts(1 hunks)packages/react-query-observable/src/__tests__/observableQueryOptions.test.ts(1 hunks)packages/react-query-observable/src/__tests__/queriesWithObservable.test.ts(1 hunks)packages/react-query-observable/src/index.ts(1 hunks)packages/react-query-observable/src/observableQueryOptions.ts(1 hunks)packages/react-query-observable/src/queryFn/observableQueryFn.ts(1 hunks)packages/react-query-observable/src/queryFn/queriesWithObservable.ts(1 hunks)packages/react-query-observable/src/types.ts(1 hunks)packages/react-query-observable/tsconfig.json(1 hunks)packages/react-query-observable/tsdown.config.mjs(1 hunks)packages/react-query-observable/vitest.config.mjs(1 hunks)packages/rxjs-firebase/src/operators/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rxjs-firebase/src/operators/index.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/react-query-observable/src/index.ts
- package.json
- packages/react-query-observable/.gitignore
- packages/react-query-observable/tsconfig.json
- packages/react-query-observable/src/tests/observableQueryOptions.test.ts
- .cursor/rules/tests.mdc
- packages/react-query-observable/src/tests/queriesWithObservable.test.ts
- .cursor/rules/naming-conventions-rule.mdc
- .cursor/rules/typescript-usage-rule.mdc
- packages/react-query-observable/.npmignore
- packages/react-query-observable/src/tests/client.test.ts
- packages/react-query-observable/vitest.config.mjs
- packages/react-query-observable/README.md
- packages/react-query-observable/package.json
- packages/react-query-observable/LICENSE
- .cursor/rules/package-management.mdc
- packages/react-kitchen-sink/package.json
- packages/react-query-observable/src/tests/observableQueryFn.test.ts
- packages/react-query-observable/CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
Ensure all new code conforms to the project's ESLint and Prettier rules
Files:
packages/react-query-observable/src/queryFn/queriesWithObservable.tspackages/react-query-observable/src/observableQueryOptions.tspackages/react-query-observable/src/queryFn/observableQueryFn.tspackages/react-query-observable/src/types.ts
🧠 Learnings (2)
📓 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: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-query-observable/eslint.config.mjspackages/react-query-observable/src/types.ts
🧬 Code graph analysis (2)
packages/react-query-observable/src/observableQueryOptions.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-8)packages/react-query-observable/src/queryFn/observableQueryFn.ts (1)
queryFnFromObservableFn(8-80)
packages/react-query-observable/src/queryFn/observableQueryFn.ts (2)
packages/react-query-observable/src/types.ts (1)
ObservableQueryFunction(4-8)packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
queriesSubscriptions(3-3)
🪛 ESLint
packages/react-query-observable/src/queryFn/queriesWithObservable.ts
[error] 1-1: Filename is not in kebab case. Rename it to queries-with-observable.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/observableQueryOptions.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-options.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/queryFn/observableQueryFn.ts
[error] 1-1: Filename is not in kebab case. Rename it to observable-query-fn.ts.
(unicorn/filename-case)
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/src/types.ts
[error] 1-1: Unable to resolve path to module '@tanstack/react-query'.
(import-x/no-unresolved)
[error] 2-2: Unable to resolve path to module 'rxjs'.
(import-x/no-unresolved)
packages/react-query-observable/tsdown.config.mjs
[error] 1-1: Unable to resolve path to module 'tsdown'.
(import-x/no-unresolved)
🔇 Additional comments (6)
packages/react-query-observable/tsdown.config.mjs (1)
8-8: Good call on fixedExtensionfixedExtension: true will keep ESM import specifiers valid at runtime (.js extensions), avoiding Node/Edge ESM resolution issues.
packages/react-query-observable/src/types.ts (1)
4-8: Type alias looks solid and correctly generic.
Covers common cases and composes well with React Query context.packages/react-query-observable/src/queryFn/queriesWithObservable.ts (1)
3-3: Simple, correct registry.
Appropriate typing and default initialization.packages/react-query-observable/src/observableQueryOptions.ts (1)
25-38: Wrapper reads well; sensible defaults for staleTime and gcTime.
Composition via queryOptions is clean.packages/react-query-observable/eslint.config.mjs (2)
13-24: TS parserOptions block looks solid.Good use of projectService and tsconfigRootDir for flat config setups.
33-41: No changes needed for vitest config overrides
vitest.configs.recommendedis an object (not an array) and doesn’t include afileskey, so spreading it after yourfiles: ['**/*.test.ts']won’t override your glob. It also registers thevitestplugin, so your separate override for custom rules resolves correctly.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor
Chores