feat(clerk-js,types): Signals fetchStatus #6549
Conversation
🦋 Changeset detectedLatest commit: 2336512 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@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: |
📝 WalkthroughWalkthroughIntroduces a ResourceFetch event and payload to the event system. Adds a runAsyncResourceTask utility to standardize resource task lifecycle and emissions. Refactors SignInFuture methods to use the new wrapper, removes its fetchStatus property, and updates finalize to accept an optional navigate parameter. Renames signInSignal to signInResourceSignal, adds signInFetchSignal, and updates computed signals to include fetchStatus. State now exposes signInFetchSignal and listens to resource:fetch to update it. Types are updated accordingly for SignInFutureResource and State. Adds unit tests for runAsyncResourceTask. Includes a changeset for minor version bumps and notes experimental Signal fetchStatus support. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
packages/clerk-js/src/core/resources/SignIn.ts (3)
579-582: Don’t swallow errors from create() inside runAsyncTask; abort on failureawait this.create(...) returns { error } (it doesn’t throw), so failures won’t short-circuit this flow. We then proceed to read supportedFirstFactors and likely throw a secondary, misleading error. Propagate the original error to the outer runAsyncTask.
- if (!this.resource.id) { - await this.create({ identifier: email }); - } + if (!this.resource.id) { + const { error } = await this.create({ identifier: email }); + if (error) { + throw error; + } + }
624-631: Same issue: propagate create() errors in SSO pathAs above, ensure failures from create() abort the rest of the task; otherwise subsequent logic runs in an invalid state.
- if (!this.resource.id) { - await this.create({ - strategy, - redirectUrl: redirectUrlWithAuthToken, - actionCompleteRedirectUrl: redirectUrlComplete, - }); - } + if (!this.resource.id) { + const { error } = await this.create({ + strategy, + redirectUrl: redirectUrlWithAuthToken, + actionCompleteRedirectUrl: redirectUrlComplete, + }); + if (error) { + throw error; + } + }
1-72: Autofix blocked by import resolution errors
Running ESLint --fix onpackages/clerk-js/src/core/resources/SignIn.tsfailed with import/no-unresolved errors for@clerk/shared/*, so the simple-import-sort/imports fix couldn’t be applied.Please:
- Ensure
@clerk/sharedis installed and your workspace is set up correctly.- Configure ESLint’s import resolver (e.g. via
eslint-import-resolver-typescript) so it can resolve your TypeScript path aliases.- Rerun your workspace linter (
pnpm -w lint --fixor equivalent) to automatically sort the imports.
🧹 Nitpick comments (10)
.changeset/twelve-crabs-return.md (1)
1-6: Confirm semver: removing a property from @clerk/types may be a breaking changeAI summary notes that
fetchStatuswas removed fromSignInFutureResourcein@clerk/types. If any external consumer references that type, this is a type-level breaking change and should be a major bump. If it’s strictly experimental and not part of the public API surface, please confirm and consider documenting that rationale here; otherwise, adjust the versioning.You can also expand the changeset notes for discoverability:
-[Experimental] Signal `fetchStatus` support. +[Experimental] Signal `fetchStatus` support. + +This introduces `resource:fetch` events and `runAsyncTask` to standardize fetch lifecycle handling. +The sign-in computed signal now exposes `fetchStatus` ('idle' | 'fetching'). + +Note: `fetchStatus` was removed from `SignInFutureResource` in `@clerk/types` and moved to the signal layer. +If you were using it directly from `SignInFutureResource`, migrate to `state.signInSignal().fetchStatus`.packages/types/src/state.ts (1)
35-36: Unify fetch status typing and update docs to reflect the new field
- Prefer a shared type alias for fetch status to avoid drifting unions across packages.
- Update the comment to mention the new
fetchStatusfield.Apply this local diff to use a shared alias:
- fetchStatus: 'idle' | 'fetching'; + fetchStatus: FetchStatus;And add this type near the top of the file to centralize the union (or export it from a shared types module if you prefer to reuse across packages):
export type FetchStatus = 'idle' | 'fetching';Additionally, consider amending the doc comment above
signInSignalto mention it returnsfetchStatusalongsideerrorsandsignIn, and whether it’s experimental.packages/clerk-js/src/core/events.ts (2)
13-14: Document the new event for clarity and future discoverabilityAdd JSDoc to describe when
resource:fetchis emitted and its semantics.ResourceError: 'resource:error', - ResourceFetch: 'resource:fetch', + /** + * Emitted when an async task begins/ends for a resource. + * Semantics: + * - status: 'fetching' is emitted when a task starts + * - status: 'idle' is emitted when a task completes + */ + ResourceFetch: 'resource:fetch',
19-20: Consider centralizing the fetch-status type
'idle' | 'fetching'appears in multiple places. To avoid drift, export a sharedFetchStatustype from@clerk/types(e.g.,packages/types/src/state.ts) and consume it here:// Example: import type { FetchStatus } from '@clerk/types'; export type ResourceFetchPayload = { resource: BaseResource; status: FetchStatus };This keeps the union consistent across packages.
packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts (1)
11-11: Avoidanyin tests; prefer a type-only import for the resourceStick to type-only imports to satisfy the typing without pulling runtime code.
+import type { BaseResource } from '../../core/resources/internal'; -const resource = {} as any; // runAsyncTask doesn't depend on resource being a BaseResource +const resource = {} as unknown as BaseResource; // runAsyncTask doesn't depend on resource being a BaseResourcepackages/clerk-js/src/utils/runAsyncTask.ts (1)
8-12: Optional: return a discriminated union instead of null-able errorA discriminated union improves ergonomics and avoids returning
error: nullon success.Example:
export type AsyncTaskResult<T> = | { ok: true; result: T } | { ok: false; error: unknown }; export async function runAsyncTask<T>(...): Promise<AsyncTaskResult<T>> { /* ... */ }packages/clerk-js/src/core/state.ts (2)
36-40: Avoid duplicating the fetch-status union across filesThe union 'idle' | 'fetching' is hard-coded here and in signals.ts. Prefer a single exported type to prevent drift.
Apply this diff here (paired with the signals.ts change below) to reuse a shared type:
import type { BaseResource } from './resources/Base'; +import type { FetchStatus } from './signals'; - private onResourceFetch = (payload: { resource: BaseResource; status: 'idle' | 'fetching' }) => { + private onResourceFetch = (payload: { resource: BaseResource; status: FetchStatus }) => { if (payload.resource instanceof SignIn) { this.signInFetchSignal({ status: payload.status }); } };
10-13: Mark signal properties as readonly (public by intent)These are effectively public API surface on State; make them explicit and immutable to align with the guidelines and prevent accidental reassignment.
- signInResourceSignal = signInResourceSignal; - signInErrorSignal = signInErrorSignal; - signInFetchSignal = signInFetchSignal; - signInSignal = signInComputedSignal; + public readonly signInResourceSignal = signInResourceSignal; + public readonly signInErrorSignal = signInErrorSignal; + public readonly signInFetchSignal = signInFetchSignal; + public readonly signInSignal = signInComputedSignal;packages/clerk-js/src/core/resources/SignIn.ts (1)
567-575: Add unit tests for error propagation in nested flowsNow that create() returns { error }, paths that call it inside another runAsyncTask should assert that an error aborts subsequent steps (e.g., factor lookup, navigation). Add tests for:
- sendEmailCode: failing create() causes outer runAsyncTask to emit resource:error and return { error } without attempting prepare_first_factor
- sso: failing create() prevents reading firstFactorVerification and navigation
I can draft focused tests in utils/tests or resources/tests with a stubbed eventBus and fake __internal_basePost. Want me to open a patch with those?
Also applies to: 579-595, 617-638
packages/clerk-js/src/core/signals.ts (1)
7-10: Centralize fetch status type for reuse across modulesExport a shared type to avoid duplicating the union and improve maintainability (aligns with the paired change suggested in state.ts).
-export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); -export const signInErrorSignal = signal<{ error: unknown }>({ error: null }); -export const signInFetchSignal = signal<{ status: 'idle' | 'fetching' }>({ status: 'idle' }); +export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); +export const signInErrorSignal = signal<{ error: unknown }>({ error: null }); +export type FetchStatus = 'idle' | 'fetching'; +export const signInFetchSignal = signal<{ status: FetchStatus }>({ status: 'idle' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (9)
.changeset/twelve-crabs-return.md(1 hunks)packages/clerk-js/src/core/events.ts(2 hunks)packages/clerk-js/src/core/resources/SignIn.ts(8 hunks)packages/clerk-js/src/core/signals.ts(1 hunks)packages/clerk-js/src/core/state.ts(3 hunks)packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts(1 hunks)packages/clerk-js/src/utils/runAsyncTask.ts(1 hunks)packages/types/src/signIn.ts(0 hunks)packages/types/src/state.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/types/src/signIn.ts
🧰 Additional context used
📓 Path-based instructions (11)
.changeset/**
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Automated releases must use Changesets.
Files:
.changeset/twelve-crabs-return.md
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/utils/runAsyncTask.tspackages/types/src/state.tspackages/clerk-js/src/utils/__tests__/runAsyncTask.spec.tspackages/clerk-js/src/core/events.tspackages/clerk-js/src/core/signals.tspackages/clerk-js/src/core/resources/SignIn.tspackages/clerk-js/src/core/state.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts
🧬 Code Graph Analysis (5)
packages/clerk-js/src/utils/runAsyncTask.ts (1)
packages/clerk-js/src/core/events.ts (1)
eventBus(31-31)
packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts (2)
packages/clerk-js/src/core/events.ts (1)
eventBus(31-31)packages/clerk-js/src/utils/runAsyncTask.ts (1)
runAsyncTask(8-30)
packages/clerk-js/src/core/signals.ts (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
SignIn(74-483)
packages/clerk-js/src/core/resources/SignIn.ts (1)
packages/clerk-js/src/utils/runAsyncTask.ts (1)
runAsyncTask(8-30)
packages/clerk-js/src/core/state.ts (4)
packages/types/src/state.ts (1)
State(28-58)packages/clerk-js/src/core/signals.ts (3)
signInResourceSignal(7-7)signInErrorSignal(8-8)signInFetchSignal(9-9)packages/clerk-js/src/core/events.ts (1)
eventBus(31-31)packages/clerk-js/src/core/resources/SignIn.ts (1)
SignIn(74-483)
🪛 ESLint
packages/clerk-js/src/core/resources/SignIn.ts
[error] 1-72: Run autofix to sort these imports!
(simple-import-sort/imports)
⏰ 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: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/clerk-js/src/core/state.ts (1)
21-22: Wiring resource:fetch to fetch signal looks correctListening to resource:fetch and forwarding status to signInFetchSignal is the right integration point. The guard on SignIn ensures we don’t bleed fetch state across resources.
Also applies to: 36-40
packages/clerk-js/src/core/resources/SignIn.ts (1)
530-536: Refactor to runAsyncTask is clean where there’s a single API callThese methods wrap a single __internal_basePost and correctly delegate lifecycle events to runAsyncTask. No correctness issues spotted.
Also applies to: 597-604, 640-648
packages/clerk-js/src/core/signals.ts (1)
11-19: Computed shape and wiring look goodsignInComputedSignal now composes resource, errors, and fetchStatus cleanly. The returned surface matches the updated types.
| } catch (err: unknown) { | ||
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | ||
| return { error: err }; | ||
| } | ||
|
|
||
| return { error: null }; | ||
| }); | ||
| } | ||
|
|
||
| async sendEmailCode({ email }: { email: string }): Promise<{ error: unknown }> { | ||
| eventBus.emit('resource:error', { resource: this.resource, error: null }); | ||
| try { |
There was a problem hiding this comment.
Avoid sending identifier: null; include the field only when defined
Using identifier || previousIdentifier can yield null, which will serialize as identifier: null. Prefer nullish coalescing and omit the key when undefined.
- const previousIdentifier = this.resource.identifier;
- await this.resource.__internal_basePost({
- path: this.resource.pathRoot,
- body: { identifier: identifier || previousIdentifier, password },
- });
+ const previousIdentifier = this.resource.identifier;
+ const resolvedIdentifier = identifier ?? previousIdentifier ?? undefined;
+ await this.resource.__internal_basePost({
+ path: this.resource.pathRoot,
+ body: {
+ ...(resolvedIdentifier !== undefined ? { identifier: resolvedIdentifier } : {}),
+ password,
+ },
+ });📝 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.
| const previousIdentifier = this.resource.identifier; | |
| await this.resource.__internal_basePost({ | |
| path: this.resource.pathRoot, | |
| body: { identifier: identifier || previousIdentifier, password }, | |
| }); | |
| } catch (err: unknown) { | |
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | |
| return { error: err }; | |
| } | |
| return { error: null }; | |
| }); | |
| const previousIdentifier = this.resource.identifier; | |
| const resolvedIdentifier = identifier ?? previousIdentifier ?? undefined; | |
| await this.resource.__internal_basePost({ | |
| path: this.resource.pathRoot, | |
| body: { | |
| ...(resolvedIdentifier !== undefined ? { identifier: resolvedIdentifier } : {}), | |
| password, | |
| }, | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/resources/SignIn.ts around lines 569 to 574,
replace the identifier || previousIdentifier pattern (which can produce
identifier: null) with logic that uses nullish coalescing and only adds the
identifier key when defined; for example, compute const id = identifier ??
previousIdentifier and build the request body by always including password but
only adding identifier if id !== undefined (and/or !== null if you want to avoid
null explicitly), then pass that body to __internal_basePost so identifier is
omitted from the serialized payload when not defined.
| it('emits fetching/idle and returns result on success', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const task = vi.fn().mockResolvedValue('ok'); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBe('ok'); | ||
| expect(error).toBeNull(); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
|
|
||
| it('emits error and returns error on failure', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const thrown = new Error('fail'); | ||
| const task = vi.fn().mockRejectedValue(thrown); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBeUndefined(); | ||
| expect(error).toBe(thrown); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:error', { | ||
| resource, | ||
| error: thrown, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(4, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add a concurrency test to catch in-flight status race conditions
If two tasks run concurrently for the same resource, the current approach can emit idle while another task is still fetching, leading to flickering fetchStatus. Add a test that runs two overlapping tasks and asserts that resource:fetch does not flip to idle until both complete (or decide on desired semantics).
Example test to add:
it('handles overlapping tasks without prematurely emitting idle', async () => {
const emitSpy = vi.spyOn(eventBus, 'emit');
let resolveA: (v: unknown) => void;
const taskA = vi.fn(() => new Promise(res => (resolveA = res)));
const taskB = vi.fn().mockResolvedValue('ok');
// Start A (fetching)
const pA = runAsyncTask(resource, taskA);
// Start B (still fetching)
const pB = runAsyncTask(resource, taskB);
// Resolve B first
await pB;
// At this point, we expect no final 'idle' yet if A is still in flight
// Adjust assertion based on desired semantics; e.g., last call isn't 'idle' until A resolves
const lastCall = emitSpy.mock.calls[emitSpy.mock.calls.length - 1];
expect(lastCall[0]).toBe('resource:fetch');
expect(lastCall[1]).toMatchObject({ resource, status: 'fetching' });
// Now resolve A; expect idle
resolveA!('ok');
await pA;
const finalCall = emitSpy.mock.calls[emitSpy.mock.calls.length - 1];
expect(finalCall).toEqual(['resource:fetch', { resource, status: 'idle' }]);
});🏁 Script executed:
#!/bin/bash
# Locate the implementation of runAsyncTask to verify concurrency handling
rg -n "export async function runAsyncTask" -C8 packages/clerk-js/src/utilsLength of output: 1346
Enhance runAsyncTask to handle overlapping tasks without premature “idle” emits
The current implementation in packages/clerk-js/src/utils/runAsyncTask.ts always emits resource:fetch → idle as soon as any task resolves, even if other tasks for the same resource are still in flight. This will cause flickering of the fetch status when two tasks overlap.
To address this, you should:
• Introduce an in-flight counter (e.g. a WeakMap<BaseResource, number>) inside runAsyncTask:
– Increment the counter before calling await task().
– Decrement in a finally block.
– Only emit { status: 'idle' } once the counter reaches zero.
• Add a concurrency test in packages/clerk-js/src/utils/__tests__/runAsyncTask.spec.ts to verify that overlapping tasks don’t flip back to idle until all tasks complete. For example:
it('does not emit idle until all overlapping tasks complete', async () => {
const emitSpy = vi.spyOn(eventBus, 'emit');
let resolveA!: (value: unknown) => void;
const taskA = vi.fn(() => new Promise(res => (resolveA = res)));
const taskB = vi.fn().mockResolvedValue('ok');
const pA = runAsyncTask(resource, taskA);
const pB = runAsyncTask(resource, taskB);
await pB;
// Should still be 'fetching' because A is not done
const last = emitSpy.mock.calls.at(-1)!;
expect(last).toEqual(['resource:fetch', { resource, status: 'fetching' }]);
resolveA('ok');
await pA;
// Now finally emit 'idle'
const final = emitSpy.mock.calls.at(-1)!;
expect(final).toEqual(['resource:fetch', { resource, status: 'idle' }]);
});
There was a problem hiding this comment.
@dstaley I'm not sure if this will happen in practice, but it's an interesting case to think about. WDYT?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
technically yes, but I don't think there's a valid instance in which it could happen. You would need to trigger two async methods on the signIn instance at one time, and I'm not sure there's a legitimate case where you'd want that to work as intended. Could be a nice refactor in the future though, but probably YAGNI for now.
| import { eventBus } from '../core/events'; | ||
| import type { BaseResource } from '../core/resources/internal'; | ||
|
|
||
| /** | ||
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | ||
| * Flow APIs. | ||
| */ | ||
| export async function runAsyncTask<T>( | ||
| resource: BaseResource, | ||
| task: () => Promise<T>, | ||
| ): Promise<{ result?: T; error: unknown }> { | ||
| eventBus.emit('resource:error', { resource, error: null }); | ||
| eventBus.emit('resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
|
|
||
| try { | ||
| const result = await task(); | ||
| return { result, error: null }; | ||
| } catch (err) { | ||
| eventBus.emit('resource:error', { resource, error: err }); | ||
| return { error: err }; | ||
| } finally { | ||
| eventBus.emit('resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use event constants and guard against overlapping task races with in-flight counting
- Replace string event names with
events.*for consistency and type-safety. - Track an in-flight counter per resource so
idleisn’t emitted until all concurrent tasks complete. This prevents flickeringfetchStatusin UIs when overlapping tasks occur.
Apply:
-import { eventBus } from '../core/events';
+import { events, eventBus } from '../core/events';
import type { BaseResource } from '../core/resources/internal';
/**
* Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom
* Flow APIs.
*/
export async function runAsyncTask<T>(
resource: BaseResource,
task: () => Promise<T>,
): Promise<{ result?: T; error: unknown }> {
- eventBus.emit('resource:error', { resource, error: null });
- eventBus.emit('resource:fetch', {
- resource,
- status: 'fetching',
- });
+ // Keep a per-resource in-flight counter to avoid premature 'idle' on overlapping tasks.
+ // WeakMap ensures resources can be GC'd when no longer referenced.
+ const inflight: WeakMap<BaseResource, number> =
+ // Reuse a shared map on the eventBus instance to avoid module duplication across bundles/tests.
+ // @ts-expect-error: attach internal state
+ (eventBus.__inflight ??= new WeakMap<BaseResource, number>());
+
+ const inc = (inflight.get(resource) ?? 0) + 1;
+ inflight.set(resource, inc);
+
+ eventBus.emit(events.ResourceError, { resource, error: null });
+ if (inc === 1) {
+ eventBus.emit(events.ResourceFetch, {
+ resource,
+ status: 'fetching',
+ });
+ }
try {
const result = await task();
return { result, error: null };
} catch (err) {
- eventBus.emit('resource:error', { resource, error: err });
+ eventBus.emit(events.ResourceError, { resource, error: err });
return { error: err };
} finally {
- eventBus.emit('resource:fetch', {
- resource,
- status: 'idle',
- });
+ const dec = (inflight.get(resource) ?? 1) - 1;
+ if (dec <= 0) {
+ inflight.delete(resource);
+ eventBus.emit(events.ResourceFetch, {
+ resource,
+ status: 'idle',
+ });
+ } else {
+ inflight.set(resource, dec);
+ }
}
}Note: If you’d rather not attach internal state to eventBus, hoist the WeakMap to module scope.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { eventBus } from '../core/events'; | |
| import type { BaseResource } from '../core/resources/internal'; | |
| /** | |
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | |
| * Flow APIs. | |
| */ | |
| export async function runAsyncTask<T>( | |
| resource: BaseResource, | |
| task: () => Promise<T>, | |
| ): Promise<{ result?: T; error: unknown }> { | |
| eventBus.emit('resource:error', { resource, error: null }); | |
| eventBus.emit('resource:fetch', { | |
| resource, | |
| status: 'fetching', | |
| }); | |
| try { | |
| const result = await task(); | |
| return { result, error: null }; | |
| } catch (err) { | |
| eventBus.emit('resource:error', { resource, error: err }); | |
| return { error: err }; | |
| } finally { | |
| eventBus.emit('resource:fetch', { | |
| resource, | |
| status: 'idle', | |
| }); | |
| } | |
| } | |
| import { events, eventBus } from '../core/events'; | |
| import type { BaseResource } from '../core/resources/internal'; | |
| /** | |
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | |
| * Flow APIs. | |
| */ | |
| export async function runAsyncTask<T>( | |
| resource: BaseResource, | |
| task: () => Promise<T>, | |
| ): Promise<{ result?: T; error: unknown }> { | |
| // Keep a per-resource in-flight counter to avoid premature 'idle' on overlapping tasks. | |
| // WeakMap ensures resources can be GC'd when no longer referenced. | |
| const inflight: WeakMap<BaseResource, number> = | |
| // Reuse a shared map on the eventBus instance to avoid module duplication across bundles/tests. | |
| // @ts-expect-error: attach internal state | |
| (eventBus.__inflight ??= new WeakMap<BaseResource, number>()); | |
| const inc = (inflight.get(resource) ?? 0) + 1; | |
| inflight.set(resource, inc); | |
| eventBus.emit(events.ResourceError, { resource, error: null }); | |
| if (inc === 1) { | |
| eventBus.emit(events.ResourceFetch, { | |
| resource, | |
| status: 'fetching', | |
| }); | |
| } | |
| try { | |
| const result = await task(); | |
| return { result, error: null }; | |
| } catch (err) { | |
| eventBus.emit(events.ResourceError, { resource, error: err }); | |
| return { error: err }; | |
| } finally { | |
| const dec = (inflight.get(resource) ?? 1) - 1; | |
| if (dec <= 0) { | |
| inflight.delete(resource); | |
| eventBus.emit(events.ResourceFetch, { | |
| resource, | |
| status: 'idle', | |
| }); | |
| } else { | |
| inflight.set(resource, dec); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
@dstaley this is an interesting suggestion...do you think this could happen in practice?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
same thoughts as #6549 (comment)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
packages/clerk-js/src/core/resources/SignIn.ts (3)
618-639: Inline create call to avoid nested runAsyncTask; add else-branch for invalid status.
- Avoid nested runAsyncTask by calling __internal_basePost directly.
- Mirror the stricter error handling used elsewhere (authenticateWithRedirectOrPopup) when status is not 'unverified' or URL missing.
Apply:
- if (!this.resource.id) { - await this.create({ - strategy, - redirectUrl: redirectUrlWithAuthToken, - actionCompleteRedirectUrl: redirectUrlComplete, - }); - } + if (!this.resource.id) { + await this.resource.__internal_basePost({ + path: this.resource.pathRoot, + body: { + strategy, + redirectUrl: redirectUrlWithAuthToken, + actionCompleteRedirectUrl: redirectUrlComplete, + }, + }); + } @@ - if (status === 'unverified' && externalVerificationRedirectURL) { - windowNavigate(externalVerificationRedirectURL); - } + if (status === 'unverified' && externalVerificationRedirectURL) { + windowNavigate(externalVerificationRedirectURL); + } else { + clerkInvalidFAPIResponse(status, SignIn.fapiClient.buildEmailAddress('support')); + }
579-595: Avoid nested runAsyncTask in sendEmailCodeReplacing
await this.create(...)(which itself wraps a runAsyncTask) with a direct__internal_basePostcall prevents duplicateresource:fetch/idleemissions.• File: packages/clerk-js/src/core/resources/SignIn.ts
Method:sendEmailCode(lines 579–595)Apply this diff:
@@ packages/clerk-js/src/core/resources/SignIn.ts:579-582 - if (!this.resource.id) { - await this.create({ identifier: email }); - } + if (!this.resource.id) { + await this.resource.__internal_basePost({ + path: this.resource.pathRoot, + body: { identifier: email }, + }); + }
1-73: Sort imports in SignIn.ts to satisfy simple-import-sort/importsThe import of
runAsyncTaskis out of place—it needs to be grouped with the other../../utils/*imports and all relative imports must be alphabetized. Please reorder the imports (or run your linter’s autofix once the tooling is working) as in the example below:Affected file:
packages/clerk-js/src/core/resources/SignIn.tsExample diff:
import { createValidatePassword } from '../../utils/passwords/password'; +import { runAsyncTask } from '../../utils/runAsyncTask'; import { clerkInvalidFAPIResponse, clerkInvalidStrategy, clerkMissingOptionError, clerkMissingWebAuthnPublicKeyOptions, clerkVerifyEmailAddressCalledBeforeCreate, clerkVerifyPasskeyCalledBeforeCreate, clerkVerifyWeb3WalletCalledBeforeCreate, } from '../errors'; import { eventBus } from '../events'; import { BaseResource, UserData, Verification } from './internal';• Move the
runAsyncTaskimport up to join the other../../utils/*imports
• Ensure all relative imports (../../…,../…,./…) are in ascending alphabetical order
♻️ Duplicate comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
569-575: Avoid serializing identifier: null in password() body.Using identifier || previousIdentifier can yield null, resulting in identifier: null being sent. Prefer nullish coalescing and omit the key when unresolved.
Apply:
- const previousIdentifier = this.resource.identifier; - await this.resource.__internal_basePost({ - path: this.resource.pathRoot, - body: { identifier: identifier || previousIdentifier, password }, - }); + const previousIdentifier = this.resource.identifier; + const resolvedIdentifier = identifier ?? previousIdentifier ?? undefined; + await this.resource.__internal_basePost({ + path: this.resource.pathRoot, + body: { + ...(resolvedIdentifier != null ? { identifier: resolvedIdentifier } : {}), + password, + }, + });
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
641-648: Optionally omit undefined navigate to keep payload minimal.setActive likely tolerates navigate: undefined, but you can elide it when not provided.
Apply:
- await SignIn.clerk.setActive({ session: this.resource.createdSessionId, navigate }); + await SignIn.clerk.setActive({ + session: this.resource.createdSessionId, + ...(navigate ? { navigate } : {}), + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/core/resources/SignIn.ts(9 hunks)packages/types/src/signIn.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/types/src/signIn.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/resources/SignIn.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/resources/SignIn.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/core/resources/SignIn.ts
🧬 Code Graph Analysis (1)
packages/clerk-js/src/core/resources/SignIn.ts (2)
packages/clerk-js/src/utils/runAsyncTask.ts (1)
runAsyncTask(8-30)packages/types/src/clerk.ts (1)
SetActiveNavigate(123-123)
🪛 ESLint
packages/clerk-js/src/core/resources/SignIn.ts
[error] 1-73: Run autofix to sort these imports!
(simple-import-sort/imports)
⏰ 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). (22)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
🔇 Additional comments (5)
packages/clerk-js/src/core/resources/SignIn.ts (5)
509-527: LGTM: reset password email code handled via runAsyncTask.Good guard on missing sign-in and clear factor lookup before preparing the first factor.
531-536: LGTM: verification of reset password email code centralized via runAsyncTask.Concise and consistent with the new async handling pattern.
546-551: LGTM: submit reset password wrapped with runAsyncTask.Defaulting signOutOfOtherSessions to true is sensible.
560-565: LGTM: create() migration to runAsyncTask.Matches the new event-driven fetch status flow.
599-604: LGTM: email code verification via runAsyncTask.Consistent usage of the centralized async task wrapper.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/clerk-js/src/core/signals.ts (4)
9-9: Avoid duplicating the 'idle' | 'fetching' literal union; centralize the typeDefining the fetch status union inline here risks drift if more statuses are added via events/types. Prefer a shared type alias (ideally from @clerk/types); if not available, at least localize it.
-export const signInFetchSignal = signal<{ status: 'idle' | 'fetching' }>({ status: 'idle' }); +type FetchStatus = 'idle' | 'fetching'; +export const signInFetchSignal = signal<{ status: FetchStatus }>({ status: 'idle' });If a canonical type already exists (e.g., ResourceFetchStatus), import and use that instead to ensure consistency across the codebase.
11-19: Stable return shape and fetchStatus propagation look good; avoid returning undefined for signInNice improvement keeping the return shape stable and surfacing fetchStatus. Minor nit: if __internal_future can ever be undefined, the current expression may return undefined instead of null. Consider nullish coalescing.
- return { errors, fetchStatus, signIn: signIn ? signIn.__internal_future : null }; + return { errors, fetchStatus, signIn: signIn?.__internal_future ?? null };Additionally, consider adding/confirming a unit test that:
- Asserts fetchStatus is 'idle' by default and transitions to 'fetching' when the corresponding event is emitted.
- Asserts errors is the empty parsed shape when error is null.
43-45: Use nullish check instead of generic falsy check to avoid swallowing legitimate valuesThis fixes the prior null handling—thanks. To be precise, prefer error == null to only skip null/undefined and not other falsy values.
- if (!error) { + if (error == null) { return parsedErrors; }
55-95: Avoid shadowing the outer error variable inside forEachThe parameter name error in the forEach callback shadows the outer error identifier, which can be confusing and may trip lint rules (no-shadow). Rename it for clarity.
- error.errors.forEach(error => { - if ('meta' in error && error.meta && 'paramName' in error.meta) { - switch (error.meta.paramName) { + error.errors.forEach(apiError => { + if ('meta' in apiError && apiError.meta && 'paramName' in apiError.meta) { + switch (apiError.meta.paramName) { case 'first_name': - parsedErrors.fields.firstName = error; + parsedErrors.fields.firstName = apiError; break; case 'last_name': - parsedErrors.fields.lastName = error; + parsedErrors.fields.lastName = apiError; break; case 'email_address': - parsedErrors.fields.emailAddress = error; + parsedErrors.fields.emailAddress = apiError; break; case 'identifier': - parsedErrors.fields.identifier = error; + parsedErrors.fields.identifier = apiError; break; case 'phone_number': - parsedErrors.fields.phoneNumber = error; + parsedErrors.fields.phoneNumber = apiError; break; case 'password': - parsedErrors.fields.password = error; + parsedErrors.fields.password = apiError; break; case 'username': - parsedErrors.fields.username = error; + parsedErrors.fields.username = apiError; break; case 'code': - parsedErrors.fields.code = error; + parsedErrors.fields.code = apiError; break; case 'captcha': - parsedErrors.fields.captcha = error; + parsedErrors.fields.captcha = apiError; break; case 'legal_accepted': - parsedErrors.fields.legalAccepted = error; + parsedErrors.fields.legalAccepted = apiError; break; default: - parsedErrors.global.push(error); + parsedErrors.global.push(apiError); break; } } else { - parsedErrors.global.push(error); + parsedErrors.global.push(apiError); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/core/signals.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/signals.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/signals.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/signals.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/core/signals.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/core/signals.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/core/signals.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/core/signals.ts
⏰ 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). (14)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Unit Tests (22, **)
| import type { SignIn } from './resources/SignIn'; | ||
|
|
||
| export const signInSignal = signal<{ resource: SignIn | null }>({ resource: null }); | ||
| export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename to signInResourceSignal is clear; verify callers and mark as internal
Good separation of responsibilities by renaming to signInResourceSignal. If this is not part of the public API, add an @internal JSDoc to comply with docs guidelines and prevent accidental external usage. Also, please verify there are no lingering imports/usages of the old signInSignal.
Run this to ensure no leftover references exist:
Apply this diff to mark the export as internal:
+/** @internal */
export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null });🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any remaining references to the old name
rg -n --heading '\bsignInSignal\b' -g '!**/node_modules/**'Length of output: 365
Update leftover signInSignal references & mark new export as internal
We’ve got three remaining signInSignal references that need to be switched over (and the new export marked @internal):
-
packages/clerk-js/src/core/signals.ts
+/** @internal */ export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); -
packages/clerk-js/src/core/state.ts (line 13)
-export const signInSignal = signInComputedSignal; +export const signInResourceSignal = signInComputedSignal;
-
packages/types/src/state.ts (line 32)
-signInSignal: Signal<SignIn | null>; +signInResourceSignal: Signal<SignIn | null>;
-
packages/react/src/hooks/useClerkSignal.ts (lines 21 & 38)
-clerk.__internal_state!.signInSignal(); +clerk.__internal_state!.signInResourceSignal(); -return clerk.__internal_state.signInSignal(); +return clerk.__internal_state.signInResourceSignal();
Please make these changes to fully remove the old signInSignal and ensure the new export is marked @internal.
📝 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.
| export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); | |
| /** @internal */ | |
| export const signInResourceSignal = signal<{ resource: SignIn | null }>({ resource: null }); |
🤖 Prompt for AI Agents
In packages/clerk-js/src/core/signals.ts around line 7, mark the new export
signInResourceSignal as internal by adding a JSDoc @internal comment immediately
above its declaration; then update the three remaining references of the old
signInSignal to the new name across the codebase: change usages in
packages/clerk-js/src/core/state.ts (line 13), packages/types/src/state.ts (line
32), and packages/react/src/hooks/useClerkSignal.ts (lines 21 and 38) to import
and use signInResourceSignal instead of signInSignal, ensuring any type
imports/exports are updated accordingly so imports resolve and there are no
leftover references to signInSignal.
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | ||
| * Flow APIs. | ||
| */ | ||
| export async function runAsyncTask<T>( |
There was a problem hiding this comment.
🙃 Should this be runAsyncResourceTask or something instead? The name implies it being relatively generic, but that's not the case
There was a problem hiding this comment.
yeah, lemme think on a new name. I was hoping I could co-locate this in a spot that made it obvious that it was for the custom flows implementation, but there's not really a good spot that makes that clear.
| import { eventBus } from '../core/events'; | ||
| import type { BaseResource } from '../core/resources/internal'; | ||
|
|
||
| /** | ||
| * Wrap an async task with handling for emitting error and fetch events, which reduces boilerplate. Used in our Custom | ||
| * Flow APIs. | ||
| */ | ||
| export async function runAsyncTask<T>( | ||
| resource: BaseResource, | ||
| task: () => Promise<T>, | ||
| ): Promise<{ result?: T; error: unknown }> { | ||
| eventBus.emit('resource:error', { resource, error: null }); | ||
| eventBus.emit('resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
|
|
||
| try { | ||
| const result = await task(); | ||
| return { result, error: null }; | ||
| } catch (err) { | ||
| eventBus.emit('resource:error', { resource, error: err }); | ||
| return { error: err }; | ||
| } finally { | ||
| eventBus.emit('resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
@dstaley this is an interesting suggestion...do you think this could happen in practice?
| it('emits fetching/idle and returns result on success', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const task = vi.fn().mockResolvedValue('ok'); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBe('ok'); | ||
| expect(error).toBeNull(); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
|
|
||
| it('emits error and returns error on failure', async () => { | ||
| const emitSpy = vi.spyOn(eventBus, 'emit'); | ||
| const thrown = new Error('fail'); | ||
| const task = vi.fn().mockRejectedValue(thrown); | ||
|
|
||
| const { result, error } = await runAsyncTask(resource, task); | ||
|
|
||
| expect(task).toHaveBeenCalledTimes(1); | ||
| expect(result).toBeUndefined(); | ||
| expect(error).toBe(thrown); | ||
|
|
||
| expect(emitSpy).toHaveBeenNthCalledWith(1, 'resource:error', { | ||
| resource, | ||
| error: null, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(2, 'resource:fetch', { | ||
| resource, | ||
| status: 'fetching', | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(3, 'resource:error', { | ||
| resource, | ||
| error: thrown, | ||
| }); | ||
| expect(emitSpy).toHaveBeenNthCalledWith(4, 'resource:fetch', { | ||
| resource, | ||
| status: 'idle', | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@dstaley I'm not sure if this will happen in practice, but it's an interesting case to think about. WDYT?
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/clerk-js/src/core/resources/SignIn.ts (2)
579-596: Avoid nested runAsyncResourceTask calls to prevent status flickerCalling
this.create(...)here triggers anotherrunAsyncResourceTaskwithin an existing one, producing multiple fetch/error emissions for a single logical operation. Inline the base post to keep a single lifecycle.- if (!this.resource.id) { - await this.create({ identifier: email }); - } + if (!this.resource.id) { + await this.resource.__internal_basePost({ + path: this.resource.pathRoot, + body: { identifier: email }, + }); + }
618-639: Handle unexpected verification state like the non-future API doesMirror
authenticateWithRedirectOrPopupby surfacing an explicit error if the state is notunverifiedwith a URL. This provides clear feedback and consistency.if (status === 'unverified' && externalVerificationRedirectURL) { windowNavigate(externalVerificationRedirectURL); - } + } else { + // Stay consistent with authenticateWithRedirectOrPopup + clerkInvalidFAPIResponse(status, SignIn.fapiClient.buildEmailAddress('support')); + }Additionally, consider inlining the initial
createrequest (like the sendEmailCode suggestion) to avoid nested wrappers.
♻️ Duplicate comments (1)
packages/clerk-js/src/core/resources/SignIn.ts (1)
569-575: Avoid sendingidentifier: null; omit when not providedUsing
identifier || previousIdentifiercan yieldidentifier: null, which will be serialized and sent. Use nullish coalescing and conditionally include the key.- return runAsyncResourceTask(this.resource, async () => { - const previousIdentifier = this.resource.identifier; - await this.resource.__internal_basePost({ - path: this.resource.pathRoot, - body: { identifier: identifier || previousIdentifier, password }, - }); - }); + return runAsyncResourceTask(this.resource, async () => { + const previousIdentifier = this.resource.identifier; + const resolvedIdentifier = identifier ?? previousIdentifier ?? undefined; + await this.resource.__internal_basePost({ + path: this.resource.pathRoot, + body: { + ...(resolvedIdentifier !== undefined ? { identifier: resolvedIdentifier } : {}), + password, + }, + }); + });
🧹 Nitpick comments (6)
packages/clerk-js/src/utils/runAsyncResourceTask.ts (2)
12-29: Potentially incorrect fetchStatus with overlapping tasksThis emits a fetch "fetching" at start and "idle" in finally for every task. With overlapping tasks on the same resource, one task finishing will flip status to idle even if others are still in-flight, leading to flicker or stale status. Consider either:
- Enriching the payload with a requestId to let consumers track active tasks, or
- Maintaining a per-resource in-flight counter (e.g., WeakMap) and emitting "idle" only when the counter hits 0.
If you keep the current semantics, please confirm that consumers treat fetch events correctly in presence of concurrency.
4-7: JSDoc: Document event semantics and orderingAdd a short note that the function emits:
resource:errorwitherror: null, thenresource:fetchwithstatus: 'fetching',resource:erroron failure, andresource:fetchwithstatus: 'idle'in finally.This will help future consumers and test writers.
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts (2)
6-6: Align test name with the function under testMinor naming mismatch; improves clarity in test reports.
-describe('runAsyncTask', () => { +describe('runAsyncResourceTask', () => {
11-11: Avoidanyin tests; use a typed stubKeep tests type-safe. Import the type and use a minimal stub.
- const resource = {} as any; // runAsyncTask doesn't depend on resource being a BaseResource + import type { BaseResource } from '../../core/resources/internal'; + // runAsyncResourceTask doesn't depend on resource shape beyond identity; a minimal stub is sufficient + const resource = {} as unknown as BaseResource;packages/clerk-js/src/core/resources/SignIn.ts (2)
509-528: Prefer consistent error helpers over rawErrorUse existing Clerk error helpers for consistency (e.g.,
clerkVerify...or a dedicated error builder) instead of genericnew Error(...). This aligns error codes/messages and improves DX.
508-649: Add tests for SignInFuture wrappers’ fetchStatus/error emissionsYou added focused tests for the utility, which is great. Given the refactor, please add unit/integration tests for a representative subset of
SignInFuturemethods (e.g., password, sendEmailCode, sso, finalize) to assert:
- Emission ordering for
resource:errorandresource:fetch- Error propagation in returned
{ error }- No duplicate or unexpected emissions (especially avoiding nested wrapper double-emits if you keep the current structure)
I can help scaffold these tests if useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/clerk-js/src/core/resources/SignIn.ts(9 hunks)packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts(1 hunks)packages/clerk-js/src/utils/runAsyncResourceTask.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{ts,tsx,d.ts}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Packages should export TypeScript types alongside runtime code
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
**/*
⚙️ CodeRabbit Configuration File
If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
Files:
packages/clerk-js/src/utils/runAsyncResourceTask.tspackages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.tspackages/clerk-js/src/core/resources/SignIn.ts
packages/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Unit tests should use Jest or Vitest as the test runner.
Files:
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts
packages/{clerk-js,elements,themes}/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/monorepo.mdc)
Visual regression testing should be performed for UI components.
Files:
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/typescript.mdc)
**/__tests__/**/*.{ts,tsx}: Create type-safe test builders/factories
Use branded types for test isolation
Implement proper mock types that match interfaces
Files:
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts
🧬 Code Graph Analysis (3)
packages/clerk-js/src/utils/runAsyncResourceTask.ts (1)
packages/clerk-js/src/core/events.ts (1)
eventBus(31-31)
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts (2)
packages/clerk-js/src/core/events.ts (1)
eventBus(31-31)packages/clerk-js/src/utils/runAsyncResourceTask.ts (1)
runAsyncResourceTask(8-30)
packages/clerk-js/src/core/resources/SignIn.ts (2)
packages/clerk-js/src/utils/runAsyncResourceTask.ts (1)
runAsyncResourceTask(8-30)packages/types/src/clerk.ts (1)
SetActiveNavigate(123-123)
⏰ 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). (4)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/clerk-js/src/utils/runAsyncResourceTask.ts (1)
8-12: Return type should reflect that error can be nullThe implementation always returns
error: nullon success. Tighten the return type accordingly to improve type safety for consumers.-export async function runAsyncResourceTask<T>( - resource: BaseResource, - task: () => Promise<T>, -): Promise<{ result?: T; error: unknown }> { +export async function runAsyncResourceTask<T>( + resource: BaseResource, + task: () => Promise<T>, +): Promise<{ result?: T; error: unknown | null }> {Likely an incorrect or invalid review comment.
packages/clerk-js/src/utils/__tests__/runAsyncResourceTask.spec.ts (2)
13-35: LGTM: success path covers emissions and return shapeGood coverage of emission order and result/error assertions.
37-64: LGTM: failure path validates error propagation and emissionsThe test accurately asserts emission order and returned error.
packages/clerk-js/src/core/resources/SignIn.ts (6)
62-62: LGTM: centralized wrapper importImporting
runAsyncResourceTaskhere to standardize lifecycle handling is the right direction.
531-537: LGTM: verification wrapper and action look correct
546-552: LGTM: reset password submission wrapped correctly
560-566: LGTM: create flow standardized via wrapper
599-605: LGTM: email code verification routed via wrapper
641-649: LGTM: finalize supports navigate and standardizes lifecyclePassing
navigateintosetActiveis a clean extension for custom flows.
Description
This PR implements
fetchStatusfor our experimental Custom Flow APIs, which are not yet intended for public use. It also refactors the methods inSignInFutureto use arunAsyncTaskutility function that handles fetch status and error event emission.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit