😬lazy observer 💥 #7229
😬lazy observer 💥 #7229
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
WalkthroughAdds lazy loading and caching of QueryObserver and InfiniteQueryObserver to Clerk; expands query-core to re-export those observers; updates React hooks to request observer by kind ('query' | 'infinite') and to surface an internal query client with observer constructors; updates mocks to include observer exports. Changes
Sequence DiagramsequenceDiagram
participant App as React App
participant Hook as useClerkQueryClient / useBaseQuery
participant Clerk
participant Module as query-core
App->>Hook: call useClerkQueryClient()
Hook->>Clerk: access __internal_queryClient
alt observers not cached
Clerk->>Clerk: `#loadQueryCoreModule`()
Clerk->>Module: dynamic import('query-core')
Module-->>Clerk: exports (QueryClient, QueryObserver, InfiniteQueryObserver)
Clerk->>Clerk: `#cacheQueryObservers`(module)
end
Clerk->>Clerk: ensure QueryClient instance
Clerk-->>Hook: { __tag, client, QueryObserver, InfiniteQueryObserver }
App->>Hook: call useBaseQuery(options, 'query' / 'infinite')
Hook->>Clerk: read observer ctor from internal client by kind
alt observer ctor present
Hook->>Clerk: instantiate Observer with options
Clerk-->>Hook: observer result stream
else observer missing
Hook-->>App: return stable fallback / optimistic result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/react/clerk-rq/use-clerk-query-client.ts (1)
73-74: Consider typing __internal_queryClient on the Clerk interface.The
@ts-expect-errorcomments indicate__internal_queryClientis not typed on the Clerk interface. While marked as internal, adding proper types would improve type safety and developer experience.packages/clerk-js/src/core/clerk.ts (1)
226-228: Consider adding JSDoc for new private fields.The new private fields related to lazy loading would benefit from brief documentation explaining their purpose and lifecycle.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/clerk-js/src/core/clerk.ts(3 hunks)packages/clerk-js/src/core/query-core.ts(1 hunks)packages/clerk-js/src/test/mock-helpers.ts(2 hunks)packages/shared/src/react/clerk-rq/use-clerk-query-client.ts(2 hunks)packages/shared/src/react/clerk-rq/useBaseQuery.ts(4 hunks)packages/shared/src/react/clerk-rq/useInfiniteQuery.ts(2 hunks)packages/shared/src/react/clerk-rq/useQuery.ts(1 hunks)packages/shared/src/react/hooks/__tests__/mocks/clerk.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/shared/src/react/clerk-rq/useBaseQuery.ts (1)
packages/shared/src/react/clerk-rq/types.ts (1)
UseBaseQueryOptions(14-26)
packages/shared/src/react/clerk-rq/useQuery.ts (1)
packages/shared/src/react/clerk-rq/useBaseQuery.ts (1)
useBaseQuery(28-103)
packages/shared/src/react/clerk-rq/useInfiniteQuery.ts (1)
packages/shared/src/react/clerk-rq/useBaseQuery.ts (1)
useBaseQuery(28-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (10)
packages/clerk-js/src/test/mock-helpers.ts (1)
4-4: LGTM! Mock updated to match new query client structure.The mock correctly includes the newly exposed observer constructors, aligning with the production implementation.
Also applies to: 61-62
packages/shared/src/react/clerk-rq/useQuery.ts (1)
40-40: LGTM! Observer selection now delegated to useBaseQuery.The change maintains the public API while enabling lazy observer loading internally.
packages/shared/src/react/hooks/__tests__/mocks/clerk.ts (1)
1-1: LGTM! Mock query client properly includes observer constructors.The mock structure correctly matches the production implementation exposed in the PR.
Also applies to: 21-22
packages/clerk-js/src/core/query-core.ts (1)
1-3: LGTM! Clean re-export module for lazy loading.This module enables deferred loading of query-core, reducing initial bundle size.
packages/shared/src/react/clerk-rq/useInfiniteQuery.ts (1)
3-3: LGTM! Consistent with useQuery pattern.The change delegates observer selection to useBaseQuery, maintaining API compatibility while enabling lazy loading.
Also applies to: 42-42
packages/shared/src/react/clerk-rq/useBaseQuery.ts (2)
48-53: Excellent defensive programming for lazy loading.The null safety checks and fallback handling ensure the hook works correctly during SSR and before the query client loads.
Also applies to: 67-67, 73-76, 86-87, 91-91, 94-98
33-40: Type casting concern is unfounded; the implementation follows @tanstack/query-core's documented pattern.The cast on line 39 is not brittle. Both QueryObserver and InfiniteQueryObserver share the same runtime/class shape, and InfiniteQueryObserver can be used anywhere the code expects a QueryObserver constructor—this is an explicitly supported pattern in @tanstack/query-core. The library's own code uses this exact approach. Interface divergence between these observers would constitute a breaking change in the library itself, not a failure mode of this implementation.
Likely an incorrect or invalid review comment.
packages/shared/src/react/clerk-rq/use-clerk-query-client.ts (2)
60-68: LGTM! Clean type definition for internal query client.The ClerkInternalQueryClient type properly exposes observer constructors for lazy loading.
70-92: All consumers are compatible with the new tuple shape—no changes required.Verification shows three call sites. The one in
useBaseQuery.ts:32already destructures all three elements correctly. The two inuseSubscription.rq.tsx:48andusePagesOrInfinite.rq.tsx:34destructure only the first element, which remains backward compatible with the new tuple shape since JavaScript allows extracting fewer elements than provided.packages/clerk-js/src/core/clerk.ts (1)
247-252: LGTM! Proper memoization of dynamic import.The method correctly caches the import promise to avoid duplicate module loads.
| get __internal_queryClient(): | ||
| | { | ||
| __tag: 'clerk-rq-client'; | ||
| client: QueryClient; | ||
| QueryObserver: typeof QueryObserver; | ||
| InfiniteQueryObserver: typeof InfiniteQueryObserver; | ||
| } | ||
| | undefined { | ||
| if (!this.#queryObserverCtor || !this.#infiniteQueryObserverCtor) { | ||
| void this.#loadQueryCoreModule().then(module => { | ||
| this.#cacheQueryObservers(module); | ||
| }); | ||
| } | ||
|
|
||
| if (!this.#queryClient) { | ||
| void import('./query-core') | ||
| .then(module => module.QueryClient) | ||
| .then(QueryClient => { | ||
| if (this.#queryClient) { | ||
| return; | ||
| } | ||
| this.#queryClient = new QueryClient(); | ||
| // @ts-expect-error - queryClientStatus is not typed | ||
| this.#publicEventBus.emit('queryClientStatus', 'ready'); | ||
| }); | ||
| void this.#loadQueryCoreModule().then(module => { | ||
| this.#cacheQueryObservers(module); | ||
| if (this.#queryClient) { | ||
| return; | ||
| } | ||
| const { QueryClient } = module; | ||
| this.#queryClient = new QueryClient(); | ||
| // @ts-expect-error - queryClientStatus is not typed | ||
| this.#publicEventBus.emit('queryClientStatus', 'ready'); | ||
| }); | ||
| } | ||
|
|
||
| return this.#queryClient | ||
| return this.#queryClient && this.#queryObserverCtor && this.#infiniteQueryObserverCtor | ||
| ? { | ||
| __tag: 'clerk-rq-client', | ||
| client: this.#queryClient, | ||
| QueryObserver: this.#queryObserverCtor, | ||
| InfiniteQueryObserver: this.#infiniteQueryObserverCtor, | ||
| } | ||
| : undefined; | ||
| } |
There was a problem hiding this comment.
Potential race conditions and missing error handling in lazy loading.
The getter fires multiple void promises (lines 272-274, 278-287) without coordination. Issues:
- No error handling: If
import('./query-core')fails, observers remain undefined forever but no error is surfaced - Race condition: Multiple calls before observers load will start multiple import chains
- Silent failures: Consumers see
undefinedbut don't know if loading failed or is in progress
Consider:
- Adding error handling with user notification or fallback
- Using a single loading promise with proper error propagation
- Exposing loading state through the return type or a separate status field
Apply this pattern for better error handling:
+ #queryClientLoadError: Error | undefined;
+
get __internal_queryClient():
| {
__tag: 'clerk-rq-client';
client: QueryClient;
QueryObserver: typeof QueryObserver;
InfiniteQueryObserver: typeof InfiniteQueryObserver;
}
| undefined {
+ if (this.#queryClientLoadError) {
+ throw this.#queryClientLoadError;
+ }
+
if (!this.#queryObserverCtor || !this.#infiniteQueryObserverCtor) {
- void this.#loadQueryCoreModule().then(module => {
- this.#cacheQueryObservers(module);
- });
+ void this.#loadQueryCoreModule()
+ .then(module => this.#cacheQueryObservers(module))
+ .catch(err => {
+ this.#queryClientLoadError = err;
+ console.error('Failed to load query-core module:', err);
+ });
}
if (!this.#queryClient) {
- void this.#loadQueryCoreModule().then(module => {
+ void this.#loadQueryCoreModule()
+ .then(module => {
this.#cacheQueryObservers(module);
if (this.#queryClient) {
return;
}
const { QueryClient } = module;
this.#queryClient = new QueryClient();
// @ts-expect-error - queryClientStatus is not typed
this.#publicEventBus.emit('queryClientStatus', 'ready');
- });
+ })
+ .catch(err => {
+ this.#queryClientLoadError = err;
+ console.error('Failed to load query-core module:', err);
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get __internal_queryClient(): | |
| | { | |
| __tag: 'clerk-rq-client'; | |
| client: QueryClient; | |
| QueryObserver: typeof QueryObserver; | |
| InfiniteQueryObserver: typeof InfiniteQueryObserver; | |
| } | |
| | undefined { | |
| if (!this.#queryObserverCtor || !this.#infiniteQueryObserverCtor) { | |
| void this.#loadQueryCoreModule().then(module => { | |
| this.#cacheQueryObservers(module); | |
| }); | |
| } | |
| if (!this.#queryClient) { | |
| void import('./query-core') | |
| .then(module => module.QueryClient) | |
| .then(QueryClient => { | |
| if (this.#queryClient) { | |
| return; | |
| } | |
| this.#queryClient = new QueryClient(); | |
| // @ts-expect-error - queryClientStatus is not typed | |
| this.#publicEventBus.emit('queryClientStatus', 'ready'); | |
| }); | |
| void this.#loadQueryCoreModule().then(module => { | |
| this.#cacheQueryObservers(module); | |
| if (this.#queryClient) { | |
| return; | |
| } | |
| const { QueryClient } = module; | |
| this.#queryClient = new QueryClient(); | |
| // @ts-expect-error - queryClientStatus is not typed | |
| this.#publicEventBus.emit('queryClientStatus', 'ready'); | |
| }); | |
| } | |
| return this.#queryClient | |
| return this.#queryClient && this.#queryObserverCtor && this.#infiniteQueryObserverCtor | |
| ? { | |
| __tag: 'clerk-rq-client', | |
| client: this.#queryClient, | |
| QueryObserver: this.#queryObserverCtor, | |
| InfiniteQueryObserver: this.#infiniteQueryObserverCtor, | |
| } | |
| : undefined; | |
| } | |
| #queryClientLoadError: Error | undefined; | |
| get __internal_queryClient(): | |
| | { | |
| __tag: 'clerk-rq-client'; | |
| client: QueryClient; | |
| QueryObserver: typeof QueryObserver; | |
| InfiniteQueryObserver: typeof InfiniteQueryObserver; | |
| } | |
| | undefined { | |
| if (this.#queryClientLoadError) { | |
| throw this.#queryClientLoadError; | |
| } | |
| if (!this.#queryObserverCtor || !this.#infiniteQueryObserverCtor) { | |
| void this.#loadQueryCoreModule() | |
| .then(module => this.#cacheQueryObservers(module)) | |
| .catch(err => { | |
| this.#queryClientLoadError = err; | |
| console.error('Failed to load query-core module:', err); | |
| }); | |
| } | |
| if (!this.#queryClient) { | |
| void this.#loadQueryCoreModule() | |
| .then(module => { | |
| this.#cacheQueryObservers(module); | |
| if (this.#queryClient) { | |
| return; | |
| } | |
| const { QueryClient } = module; | |
| this.#queryClient = new QueryClient(); | |
| // @ts-expect-error - queryClientStatus is not typed | |
| this.#publicEventBus.emit('queryClientStatus', 'ready'); | |
| }) | |
| .catch(err => { | |
| this.#queryClientLoadError = err; | |
| console.error('Failed to load query-core module:', err); | |
| }); | |
| } | |
| return this.#queryClient && this.#queryObserverCtor && this.#infiniteQueryObserverCtor | |
| ? { | |
| __tag: 'clerk-rq-client', | |
| client: this.#queryClient, | |
| QueryObserver: this.#queryObserverCtor, | |
| InfiniteQueryObserver: this.#infiniteQueryObserverCtor, | |
| } | |
| : undefined; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/shared/src/react/clerk-rq/useBaseQuery.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/shared/src/react/clerk-rq/useBaseQuery.ts (1)
packages/shared/src/react/clerk-rq/types.ts (1)
UseBaseQueryOptions(14-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
packages/shared/src/react/clerk-rq/useBaseQuery.ts (7)
23-23: LGTM: Clean discriminator type.The
ObserverKindtype provides a clear API for selecting between query observers.
56-65: LGTM: Appropriate fallback for SSR/loading states.The stable
fallbackResultwithstatus: 'pending'correctly represents the query state before the client loads, avoiding hydration mismatches and providing a safe default.
67-72: LGTM: Safe optimistic result computation.The memoized
optimisticResultcorrectly guards against a missing observer and includes appropriate dependencies.
75-93: LGTM: Robust subscription handling.The subscription callback correctly guards against missing observers, and the snapshot functions safely fall back when the observer is unavailable, ensuring stable SSR behavior.
95-109: LGTM: Safe effect and result handling.The effect safely updates observer options, the early return properly guards against incomplete initialization, and the result tracking logic aligns with TanStack Query's behavior.
33-40: No issues found. The type cast is correct and necessary.The double type assertion
as unknown as typeof clerkQueryClient.QueryObserveris not a type safety issue. QueryObserver and InfiniteQueryObserver expose the same observer API surface and are intentionally interchangeable for the base hook logic. Both types share the required interface for all downstream operations:getOptimisticResult,subscribe,updateResult,getCurrentResult,setOptions, andtrackResult. The cast is a necessary workaround for TypeScript's nominal type system when using structurally compatible types from @tanstack/query-core, and React's useBaseQuery calls InfiniteQueryObserver as typeof QueryObserver, confirming this is a known and safe pattern.
48-53: The review comment is incorrect. Remove it.The current implementation correctly separates observer creation from option updates. The observer is intentionally created only when the query client changes, and options are updated via
setOptionsin the useEffect (lines 95-97), which already includesdefaultedOptionsin its dependency array.Adding
defaultedOptionsto the observer useMemo dependency array would cause the observer to recreate on every render sincedefaultedOptionsis a new reference each render—this would be a performance regression that contradicts the stated design ("allows for an observer to be created every time a query client changes," not on every option change). The current pattern correctly mirrors TanStack Query's design of creating the observer once and updating options separately.Likely an incorrect or invalid review comment.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
Complete the JSDoc comment.
The empty JSDoc block should document the function's purpose, the new observerKind parameter, and how the lazy observer loading works.
Apply this diff to add documentation:
/**
- *
+ * Base query hook with lazy observer loading.
+ *
+ * @param options - Query observer options
+ * @param observerKind - Type of observer to create ('query' | 'infinite')
+ * @returns Query result with data, error, and loading states
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * | |
| */ | |
| /** | |
| * Base query hook with lazy observer loading. | |
| * | |
| * @param options - Query observer options | |
| * @param observerKind - Type of observer to create ('query' | 'infinite') | |
| * @returns Query result with data, error, and loading states | |
| */ |
🤖 Prompt for AI Agents
In packages/shared/src/react/clerk-rq/useBaseQuery.ts around lines 25 to 27, the
JSDoc block is empty; replace it with a complete comment that briefly describes
the hook's purpose (what useBaseQuery does), documents the new observerKind
parameter (allowed values and effect on observer selection), explains the lazy
observer loading behavior (when the observer is created/loaded and any side
effects), and includes @param tags for all parameters and @returns for the hook
return value so consumers and IDEs can understand usage and types.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Refactor
Tests