Recognize setup#691
Open
eugeniobet-ping wants to merge 20 commits into
Open
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ors for initialization and element validation
…rove type definitions
|
…d SDK
Renames the public web-component type surface from `RecognizeWc*` to
`RecognizeWebComponent*` (e.g. `RecognizeWcConfig` →
`RecognizeWebComponentConfiguration`, `RecognizeWcClient` →
`RecognizeWebComponentClient`, `RecognizeWcCompleteDetail` →
`RecognizeWebComponentCompleteData`) for clarity at the package boundary,
and re-exports `RecognizeError` from the package entry point.
Reworks `RecognizeError` and the error-code taxonomy:
- `RecognizeErrorCode` switches from string values to numeric values
grouped by domain (SDK 1xxx, CAMERA 2xxx, CORE 3xxx, BIOM 4xxx,
SERVER 5xxx, SECURITY 6xxx). Several codes are renamed to match the
upstream SDK vocabulary (`CAMERA_MISSING` → `CAMERA_NOT_FOUND`,
`SDK_CONFIGURATION_FAILED` → `SDK_INVALID_CONFIGURATION`,
`SDK_STORAGE_ERROR` → `SDK_STORAGE_FAILED`,
`SDK_DYNAMIC_LINKING_MALFORMED_PAYLOAD` →
`SDK_DYNAMIC_LINKING_PAYLOAD_MALFORMED`), and new codes are added
(`SDK_OUTDATED_APP`, `SDK_INVALID_CUSTOMER_PROPERTIES`,
`SDK_INVALID_CLIENT_STATE`, `BIOM_GENUINE_PRESENCE_NOT_ESTABLISHED`).
- `RecognizeError`'s constructor now accepts a standard `ErrorOptions`
bag (`new RecognizeError(code, { cause })`) and forwards it to the
base `Error` constructor, so `error.message` is the enum name and
`error.cause` follows the platform contract instead of being assigned
manually.
- The `createRecognizeError` factory is removed; `recognize.ts` now
constructs `RecognizeError` directly. The SDK→proxy error map is
frozen with `Object.freeze` to prevent accidental mutation at runtime.
Updates `recognize()` accordingly:
- Init failures now throw `RecognizeError` (with `SDK_ERROR` /
`SDK_WEB_ASSEMBLY_IMPORT_FAILED` and a descriptive `cause`) instead of
returning the error or throwing a plain `Error`.
- Subscribes to two new web-component events, `begin-stream` and
`stop-stream`, and forwards them through the observer pipeline.
- Internal renames for readability (`effectiveConfig` → `config`,
`abortController` → `aborter`, `attachListeners` →
`addEventListeners`, `applyConfig` → `setAttributes`) and a global
`HTMLElementTagNameMap` augmentation for `kl-auth` / `kl-enroll`.
Refreshes the bundled keyless SDK (`recognize-sdk/index.{js,d.ts}`,
`wasm.{js,wasm}`, `pthreads/wasm.{js,wasm}`) to a new upstream build.
The package's ESLint config now ignores `src/lib/recognize-sdk/**/*`
for dependency-checks since the bundled artifact pulls in transitive
imports that don't belong in the package manifest.
Updates the e2e example and unit tests to match: the example config
includes `authorizationToken`, and the spec asserts the new
`{ message, cause }` shape on thrown `RecognizeError`s.
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the Changes
E2E recognize-app
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant recognize
participant SDK as Keyless SDK
participant KlElement as kl-auth/kl-enroll
participant Observer
Caller->>recognize: recognize(config)
recognize-->>Caller: RecognizeWebComponentClient
Caller->>recognize: subscribe(observer)
Caller->>recognize: init({ mode:'mount' or 'attach' })
recognize->>SDK: dynamic import
recognize->>KlElement: create / attach element
recognize->>KlElement: set attributes
recognize->>KlElement: add event listeners
KlElement-->>recognize: finished / error / step events
recognize->>Observer: next / complete / error
Caller->>recognize: dispose()
recognize->>KlElement: remove()
recognize->>recognize: clear observers and state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
e2e/recognize-app/src/index.ts (1)
4-11: ⚡ Quick winAvoid embedding runtime auth/config material in source.
Keeping token/key/customer/ws values inline is risky once replaced with real values. Prefer env-driven injection with a fail-fast check for missing required values.
Proposed refactor
+const env = import.meta.env; +const required = (key: string): string => { + const value = env[key]; + if (!value) throw new Error(`Missing required env var: ${key}`); + return value; +}; + const client = recognize({ - authorizationToken: 'USER_AUTHORIZATION_FROM_CUSTOMER', - customer: 'CUSTOMER_NAME', - key: 'IMAGE_ENCRYPTION_PUBLIC_KEY', - keyID: 'IMAGE_ENCRYPTION_KEY_ID', - transactionData: 'DATA_FROM_CUSTOMER_SERVER_TO_BE_SIGNED', - wsURL: 'KEYLESS_AUTHENTICATION_SERVICE_URL', + authorizationToken: required('VITE_RECOGNIZE_AUTHORIZATION_TOKEN'), + customer: required('VITE_RECOGNIZE_CUSTOMER'), + key: required('VITE_RECOGNIZE_KEY'), + keyID: required('VITE_RECOGNIZE_KEY_ID'), + transactionData: required('VITE_RECOGNIZE_TRANSACTION_DATA'), + wsURL: required('VITE_RECOGNIZE_WS_URL'), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/recognize-app/src/index.ts` around lines 4 - 11, The recognize function call in the initialization contains hardcoded sensitive values for authorizationToken, customer, key, keyID, transactionData, and wsURL which should not be embedded in source code. Replace these hardcoded string values with reads from environment variables using process.env. Then add validation logic before calling recognize to fail fast if any required environment variables are missing, throwing an error with a clear message listing which values are undefined.packages/recognize/src/lib/defs/constants.ts (1)
2-11: ⚡ Quick winTighten
CAMERA_ONLY_DISABLE_STEPStyping and immutability.
string[]allows invalid step IDs and accidental mutation. This constant is effectively a fixed contract, so make it readonly and typed to the SDK step union.Suggested change
+import type { KeylessComponentsStep } from '../recognize-sdk/index.js'; + /** `@internal` */ -export const CAMERA_ONLY_DISABLE_STEPS: string[] = [ +export const CAMERA_ONLY_DISABLE_STEPS: readonly KeylessComponentsStep[] = [ 'bootstrap', 'camera-instructions', 'done', 'error', 'microphone-permission', 'server-computation', 'stm-choice', 'stm-qrcode', -]; +] as const;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/defs/constants.ts` around lines 2 - 11, The CAMERA_ONLY_DISABLE_STEPS constant is currently typed as string[] which allows invalid step IDs and accidental mutations. Change the type annotation from string[] to readonly and a specific SDK step union type (replace the generic string type with the appropriate union type that represents valid step identifiers in the SDK). This will prevent invalid values from being assigned and protect the constant from being mutated after initialization.packages/recognize/src/lib/recognize.types.ts (1)
112-114: ⚡ Quick winNarrow attach-mode
elementtype to the supported custom elements.
element: HTMLElementforces validation at runtime for a shape we already know statically. Narrowing improves API safety and DX.Suggested refactor
export type RecognizeWebComponentInitOptions = | { mode: 'mount'; container: HTMLElement; type: RecognizeSessionType; username: string } - | { mode: 'attach'; element: HTMLElement; username: string }; + | { mode: 'attach'; element: RecognizeWebComponent; username: string };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/recognize.types.ts` around lines 112 - 114, The attach mode in the RecognizeWebComponentInitOptions union type uses the generic HTMLElement type for the element property, which is too broad and requires runtime validation. Instead, narrow the element type to a union of the specific supported custom element types that attach mode actually accepts. Identify all supported custom element types in your codebase, create a union type for them, and replace the element: HTMLElement property in the attach mode object with element: YourCustomElementUnionType to provide compile-time type safety and improve developer experience.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/recognize-app/src/index.ts`:
- Around line 32-36: The client.init() promise chain in the mount initialization
section only handles the fulfilled path with .then(), leaving promise rejections
unhandled which can cause flaky E2E test failures. Add a .catch() handler after
the .then() block to properly handle any rejections that occur during the init()
call, ensuring errors from rejected promises are logged and don't destabilize
the test execution.
In `@packages/recognize/src/lib/recognize-sdk/index.d.ts`:
- Around line 1070-1077: There is a type inconsistency between the
KeylessVideoFrameQualityEvent constructor and the
KeylessVideoFrameQualityEventDetail interface. The constructor accepts timestamp
as a number, but the interface defines timestamp as a Date, creating a
contradictory event contract. To fix this, ensure both the constructor parameter
in KeylessVideoFrameQualityEvent and the timestamp property in
KeylessVideoFrameQualityEventDetail use the same type. Either convert the number
to a Date in the constructor before storing it in the event detail, or change
both to consistently use number if that better represents the internal timestamp
format.
In `@packages/recognize/src/lib/recognize.spec.ts`:
- Around line 61-80: The test "throws if init is called twice without dispose"
uses a try-catch block to verify that the second client.init() call throws an
error, but there is no failing assertion outside the catch block. This means the
test will pass even if init() does not throw. Convert this test to use Jest's
rejects matcher (expect(...).rejects.toThrow() or expect(...).rejects.toEqual())
to properly validate that the promise rejects with the expected error, ensuring
the test fails if init() does not throw as expected. Apply the same fix to the
related test also mentioned at lines 108-124.
- Around line 43-57: The unsubscribe test does not actually verify that the
observer stops receiving events because no event is dispatched after the init()
call in the unsubscribe test. The test needs to trigger an event or simulate a
scenario after init() that would cause the client to emit an event, and then
verify that the unsubscribed observer's next callback is not invoked when that
event occurs. This will properly validate that unsubscribe removes the observer
from the subscription and prevents future event delivery.
In `@packages/recognize/src/lib/recognize.ts`:
- Around line 147-151: In the dispose() method, you need to call aborter.abort()
before setting aborter to null. Currently, the code only sets aborter = null
without signaling cancellation to the listeners registered with aborter.signal,
which leaves stale event listeners attached. Add a call to aborter.abort() right
before the assignment to ensure all listeners are properly notified and cleaned
up, preventing memory leaks and event cross-contamination across sessions.
- Around line 63-67: The onError function directly accesses e.error.message
without verifying that e.error is an Error object, which can cause the handler
to crash since ErrorEvent.error is not guaranteed to be an Error. Add defensive
checks to verify that e.error exists and is an Error before accessing its
message property, and provide a fallback error code or message when e.error is
not a valid Error object with a message property. This will prevent the handler
from throwing and ensure observer notifications are still sent even when the
error object is malformed.
In `@packages/recognize/src/lib/recognize.types.ts`:
- Around line 57-60: The `init` method in the `RecognizeWebComponentClient`
interface has an incorrect return type. Since the runtime implementation throws
`RecognizeError` on failure rather than resolving with it, change the return
type of the `init` method from `Promise<void | RecognizeError>` to
`Promise<void>`. This aligns the type signature with the actual behavior and
prevents incorrect error handling patterns where callers might expect to handle
errors from the resolved value rather than catching thrown exceptions.
- Around line 67-76: The `disableSteps` property in the
`RecognizeWebComponentConfiguration` interface is currently typed as `string[]`,
which allows invalid step names to pass type checking. Change the type of the
`disableSteps` property from `string[]` to use the appropriate SDK step type
(such as the enum or union type used elsewhere in the SDK for step definitions)
to enforce valid step names at compile-time and strengthen the SDK contract.
---
Nitpick comments:
In `@e2e/recognize-app/src/index.ts`:
- Around line 4-11: The recognize function call in the initialization contains
hardcoded sensitive values for authorizationToken, customer, key, keyID,
transactionData, and wsURL which should not be embedded in source code. Replace
these hardcoded string values with reads from environment variables using
process.env. Then add validation logic before calling recognize to fail fast if
any required environment variables are missing, throwing an error with a clear
message listing which values are undefined.
In `@packages/recognize/src/lib/defs/constants.ts`:
- Around line 2-11: The CAMERA_ONLY_DISABLE_STEPS constant is currently typed as
string[] which allows invalid step IDs and accidental mutations. Change the type
annotation from string[] to readonly and a specific SDK step union type (replace
the generic string type with the appropriate union type that represents valid
step identifiers in the SDK). This will prevent invalid values from being
assigned and protect the constant from being mutated after initialization.
In `@packages/recognize/src/lib/recognize.types.ts`:
- Around line 112-114: The attach mode in the RecognizeWebComponentInitOptions
union type uses the generic HTMLElement type for the element property, which is
too broad and requires runtime validation. Instead, narrow the element type to a
union of the specific supported custom element types that attach mode actually
accepts. Identify all supported custom element types in your codebase, create a
union type for them, and replace the element: HTMLElement property in the attach
mode object with element: YourCustomElementUnionType to provide compile-time
type safety and improve developer experience.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3341a87f-d24b-40e1-a502-9af34de77df2
⛔ Files ignored due to path filters (3)
packages/recognize/src/lib/recognize-sdk/pthreads/wasm.wasmis excluded by!**/*.wasmpackages/recognize/src/lib/recognize-sdk/wasm.wasmis excluded by!**/*.wasmpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
e2e/recognize-app/eslint.config.mjse2e/recognize-app/package.jsone2e/recognize-app/src/index.htmle2e/recognize-app/src/index.tse2e/recognize-app/src/styles.csse2e/recognize-app/tsconfig.app.jsone2e/recognize-app/tsconfig.jsone2e/recognize-app/vite.config.tspackages/recognize/README.mdpackages/recognize/eslint.config.mjspackages/recognize/package.jsonpackages/recognize/src/index.tspackages/recognize/src/lib/classes/recognize-error.tspackages/recognize/src/lib/defs/constants.tspackages/recognize/src/lib/defs/recognize-error-code.tspackages/recognize/src/lib/defs/recognize-sdk-to-recognize-proxy-error-map.tspackages/recognize/src/lib/recognize-sdk/index.d.tspackages/recognize/src/lib/recognize-sdk/index.jspackages/recognize/src/lib/recognize-sdk/pthreads/wasm.jspackages/recognize/src/lib/recognize-sdk/wasm.datapackages/recognize/src/lib/recognize-sdk/wasm.jspackages/recognize/src/lib/recognize.spec.tspackages/recognize/src/lib/recognize.tspackages/recognize/src/lib/recognize.types.tspackages/recognize/tsconfig.jsonpackages/recognize/tsconfig.lib.jsonpackages/recognize/tsconfig.spec.jsonpackages/recognize/vitest.config.mtstsconfig.json
Comment on lines
+32
to
+36
| client | ||
| .init({ mode: 'mount', container: appEl, type: 'auth', username: 'USERNAME' }) | ||
| .then((err) => { | ||
| if (err) console.error('[recognize] init error', err); | ||
| }); |
There was a problem hiding this comment.
Handle the rejected init() promise path.
Only the fulfilled path is handled. If init() rejects, this can become an unhandled rejection and destabilize/flaky-fail E2E runs.
Proposed fix
if (appEl) {
client
.init({ mode: 'mount', container: appEl, type: 'auth', username: 'USERNAME' })
.then((err) => {
if (err) console.error('[recognize] init error', err);
- });
+ })
+ .catch((err) => {
+ console.error('[recognize] init rejected', 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.
Suggested change
| client | |
| .init({ mode: 'mount', container: appEl, type: 'auth', username: 'USERNAME' }) | |
| .then((err) => { | |
| if (err) console.error('[recognize] init error', err); | |
| }); | |
| client | |
| .init({ mode: 'mount', container: appEl, type: 'auth', username: 'USERNAME' }) | |
| .then((err) => { | |
| if (err) console.error('[recognize] init error', err); | |
| }) | |
| .catch((err) => { | |
| console.error('[recognize] init rejected', err); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/recognize-app/src/index.ts` around lines 32 - 36, The client.init()
promise chain in the mount initialization section only handles the fulfilled
path with .then(), leaving promise rejections unhandled which can cause flaky
E2E test failures. Add a .catch() handler after the .then() block to properly
handle any rejections that occur during the init() call, ensuring errors from
rejected promises are logged and don't destabilize the test execution.
Comment on lines
+1070
to
+1077
| export declare class KeylessVideoFrameQualityEvent extends CustomEvent<KeylessVideoFrameQualityEventDetail> { | ||
| constructor(filters: KeylessVideoFrameQualityFilter[], timestamp: number); | ||
| } | ||
|
|
||
| /** @public */ | ||
| export declare interface KeylessVideoFrameQualityEventDetail { | ||
| filters: KeylessVideoFrameQualityFilter[]; | ||
| timestamp: Date; |
There was a problem hiding this comment.
Fix inconsistent timestamp typing in KeylessVideoFrameQualityEvent.
KeylessVideoFrameQualityEvent takes timestamp: number, but KeylessVideoFrameQualityEventDetail exposes timestamp: Date. This creates a contradictory event contract for consumers.
Suggested fix
export declare interface KeylessVideoFrameQualityEventDetail {
filters: KeylessVideoFrameQualityFilter[];
- timestamp: Date;
+ timestamp: number;
}📝 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.
Suggested change
| export declare class KeylessVideoFrameQualityEvent extends CustomEvent<KeylessVideoFrameQualityEventDetail> { | |
| constructor(filters: KeylessVideoFrameQualityFilter[], timestamp: number); | |
| } | |
| /** @public */ | |
| export declare interface KeylessVideoFrameQualityEventDetail { | |
| filters: KeylessVideoFrameQualityFilter[]; | |
| timestamp: Date; | |
| export declare class KeylessVideoFrameQualityEvent extends CustomEvent<KeylessVideoFrameQualityEventDetail> { | |
| constructor(filters: KeylessVideoFrameQualityFilter[], timestamp: number); | |
| } | |
| /** `@public` */ | |
| export declare interface KeylessVideoFrameQualityEventDetail { | |
| filters: KeylessVideoFrameQualityFilter[]; | |
| timestamp: number; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize-sdk/index.d.ts` around lines 1070 -
1077, There is a type inconsistency between the KeylessVideoFrameQualityEvent
constructor and the KeylessVideoFrameQualityEventDetail interface. The
constructor accepts timestamp as a number, but the interface defines timestamp
as a Date, creating a contradictory event contract. To fix this, ensure both the
constructor parameter in KeylessVideoFrameQualityEvent and the timestamp
property in KeylessVideoFrameQualityEventDetail use the same type. Either
convert the number to a Date in the constructor before storing it in the event
detail, or change both to consistently use number if that better represents the
internal timestamp format.
Comment on lines
+43
to
+57
| it('unsubscribe removes the observer so it no longer receives events', async () => { | ||
| const client = recognize(CONFIG); | ||
| const next = vi.fn(); | ||
| const unsub = client.subscribe({ next }); | ||
|
|
||
| unsub(); | ||
| await client.init({ | ||
| mode: 'mount', | ||
| container: document.createElement('div'), | ||
| type: 'auth', | ||
| username: 'user', | ||
| }); | ||
|
|
||
| expect(next).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This unsubscribe test does not exercise event delivery.
No event is dispatched after init(), so the assertion passes even if unsubscribe is broken.
Suggested change
it('unsubscribe removes the observer so it no longer receives events', async () => {
@@
await client.init({
mode: 'mount',
- container: document.createElement('div'),
+ container: document.createElement('div'),
type: 'auth',
username: 'user',
});
+ const el = (document.querySelector('kl-auth') ??
+ document.createElement('kl-auth')) as HTMLElement;
+ el.dispatchEvent(new CustomEvent('step-change', { detail: {} }));
expect(next).not.toHaveBeenCalled();
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize.spec.ts` around lines 43 - 57, The
unsubscribe test does not actually verify that the observer stops receiving
events because no event is dispatched after the init() call in the unsubscribe
test. The test needs to trigger an event or simulate a scenario after init()
that would cause the client to emit an event, and then verify that the
unsubscribed observer's next callback is not invoked when that event occurs.
This will properly validate that unsubscribe removes the observer from the
subscription and prevents future event delivery.
Comment on lines
+61
to
+80
| it('throws if init is called twice without dispose', async () => { | ||
| const client = recognize(CONFIG); | ||
| const container = document.createElement('div'); | ||
| document.body.appendChild(container); | ||
|
|
||
| await client.init({ mode: 'mount', container, type: 'auth', username: 'user' }); | ||
|
|
||
| try { | ||
| await client.init({ mode: 'mount', container, type: 'auth', username: 'user' }); | ||
| } catch (e: unknown) { | ||
| expect(e).toBeInstanceOf(Error); | ||
|
|
||
| if (e instanceof Error) { | ||
| expect(e.message).toBe('SDK_ERROR'); | ||
| expect(e.cause).toBe( | ||
| 'init() called more than once — call dispose() before re-initializing', | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Convert throw assertions to rejects to prevent false positives.
Both tests can pass even when init() does not throw because there is no failing assertion outside catch.
Suggested change
it('throws if init is called twice without dispose', async () => {
@@
- try {
- await client.init({ mode: 'mount', container, type: 'auth', username: 'user' });
- } catch (e: unknown) {
- expect(e).toBeInstanceOf(Error);
-
- if (e instanceof Error) {
- expect(e.message).toBe('SDK_ERROR');
- expect(e.cause).toBe(
- 'init() called more than once — call dispose() before re-initializing',
- );
- }
- }
+ await expect(
+ client.init({ mode: 'mount', container, type: 'auth', username: 'user' }),
+ ).rejects.toMatchObject({
+ message: 'SDK_ERROR',
+ cause: 'init() called more than once — call dispose() before re-initializing',
+ });
@@
it('throws for attach mode with an unsupported element', async () => {
@@
- try {
- await client.init({ mode: 'attach', element: div, username: 'user' });
- } catch (e: unknown) {
- expect(e).toBeInstanceOf(Error);
-
- if (e instanceof Error) {
- expect(e.message).toBe('SDK_ERROR');
- expect(e.cause).toBe(
- 'invalid element <div> — options.element must be a <kl-auth> or <kl-enroll> custom element',
- );
- }
- }
+ await expect(client.init({ mode: 'attach', element: div, username: 'user' })).rejects.toMatchObject({
+ message: 'SDK_ERROR',
+ cause: 'invalid element <div> — options.element must be a <kl-auth> or <kl-enroll> custom element',
+ });
});Also applies to: 108-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize.spec.ts` around lines 61 - 80, The test
"throws if init is called twice without dispose" uses a try-catch block to
verify that the second client.init() call throws an error, but there is no
failing assertion outside the catch block. This means the test will pass even if
init() does not throw. Convert this test to use Jest's rejects matcher
(expect(...).rejects.toThrow() or expect(...).rejects.toEqual()) to properly
validate that the promise rejects with the expected error, ensuring the test
fails if init() does not throw as expected. Apply the same fix to the related
test also mentioned at lines 108-124.
Comment on lines
+147
to
+151
| dispose: (): void => { | ||
| if (element === null) return; | ||
|
|
||
| aborter = null; | ||
|
|
There was a problem hiding this comment.
Call abort() in dispose() before dropping the controller.
Listeners are registered with aborter.signal, but dispose() only sets aborter = null. That leaves stale listeners attached to prior elements and can leak/cross-deliver events across sessions.
Suggested change
dispose: (): void => {
if (element === null) return;
+ aborter?.abort();
aborter = null;
element.remove();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize.ts` around lines 147 - 151, In the
dispose() method, you need to call aborter.abort() before setting aborter to
null. Currently, the code only sets aborter = null without signaling
cancellation to the listeners registered with aborter.signal, which leaves stale
event listeners attached. Add a call to aborter.abort() right before the
assignment to ensure all listeners are properly notified and cleaned up,
preventing memory leaks and event cross-contamination across sessions.
Comment on lines
+57
to
+60
| export interface RecognizeWebComponentClient { | ||
| subscribe: (observer: RecognizeWebComponentObserver) => RecognizeWebComponentUnsubscribe; | ||
| init(options: RecognizeWebComponentInitOptions): Promise<void | RecognizeError>; | ||
| dispose: () => void; |
There was a problem hiding this comment.
Align init return type with actual behavior (Promise<void>).
The runtime implementation throws RecognizeError on failure; it does not resolve with a RecognizeError. The current signature encourages incorrect handling patterns.
Suggested fix
export interface RecognizeWebComponentClient {
subscribe: (observer: RecognizeWebComponentObserver) => RecognizeWebComponentUnsubscribe;
- init(options: RecognizeWebComponentInitOptions): Promise<void | RecognizeError>;
+ init(options: RecognizeWebComponentInitOptions): Promise<void>;
dispose: () => void;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize.types.ts` around lines 57 - 60, The
`init` method in the `RecognizeWebComponentClient` interface has an incorrect
return type. Since the runtime implementation throws `RecognizeError` on failure
rather than resolving with it, change the return type of the `init` method from
`Promise<void | RecognizeError>` to `Promise<void>`. This aligns the type
signature with the actual behavior and prevents incorrect error handling
patterns where callers might expect to handle errors from the resolved value
rather than catching thrown exceptions.
Comment on lines
+67
to
+76
| export interface RecognizeWebComponentConfiguration { | ||
| authorizationToken?: string; | ||
| customer: string; | ||
| datadogEnv?: string; | ||
| datadogToken?: string; | ||
| disableDatadog?: boolean; | ||
| disableLogger?: boolean; | ||
| disablePoweredBy?: boolean; | ||
| disableSteps?: string[]; | ||
| enableCameraFlash?: boolean; |
There was a problem hiding this comment.
Use SDK step type for disableSteps instead of string[].
Typing this as string[] allows invalid step names to pass compile-time checks and weakens the SDK contract.
Suggested fix
import type {
KeylessAuthElement,
+ KeylessComponentsStep,
KeylessEnrollElement,
KeylessFinishedEventDetail,
@@
export interface RecognizeWebComponentConfiguration {
@@
- disableSteps?: string[];
+ disableSteps?: KeylessComponentsStep[];📝 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.
Suggested change
| export interface RecognizeWebComponentConfiguration { | |
| authorizationToken?: string; | |
| customer: string; | |
| datadogEnv?: string; | |
| datadogToken?: string; | |
| disableDatadog?: boolean; | |
| disableLogger?: boolean; | |
| disablePoweredBy?: boolean; | |
| disableSteps?: string[]; | |
| enableCameraFlash?: boolean; | |
| export interface RecognizeWebComponentConfiguration { | |
| authorizationToken?: string; | |
| customer: string; | |
| datadogEnv?: string; | |
| datadogToken?: string; | |
| disableDatadog?: boolean; | |
| disableLogger?: boolean; | |
| disablePoweredBy?: boolean; | |
| disableSteps?: KeylessComponentsStep[]; | |
| enableCameraFlash?: boolean; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/recognize/src/lib/recognize.types.ts` around lines 67 - 76, The
`disableSteps` property in the `RecognizeWebComponentConfiguration` interface is
currently typed as `string[]`, which allows invalid step names to pass type
checking. Change the type of the `disableSteps` property from `string[]` to use
the appropriate SDK step type (such as the enum or union type used elsewhere in
the SDK for step definitions) to enforce valid step names at compile-time and
strengthen the SDK contract.
…on-cancelable events from Keyless SDK v3
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/recognize/src/lib/recognize.ts (1)
126-166: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
init()against overlapping callselement !== nullis checked before theawait import(...), so two concurrentinit()calls can both pass the guard and create duplicate elements/listeners. Track an in-flight initialization or reject whileinit()is pending.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/recognize.ts` around lines 126 - 166, `RecognizeWebComponent.init()` only checks `element !== null` before the async `import('./recognize-sdk/index.js')`, so concurrent calls can both pass and create duplicate elements/listeners. Add an in-flight initialization guard in `init()` (for example, a shared pending flag/promise on `RecognizeWebComponent`) and reject or no-op when initialization is already running, then clear that state once the attach/create path completes or fails.
🧹 Nitpick comments (2)
packages/recognize/src/lib/recognize.ts (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDebug
console.logshipped in a public SDK path.Styled
console.logonrecoverable-errorwill surface in every consuming app's console. Consider gating behind a debug flag or removing before release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/recognize.ts` at line 68, The styled console.log in recognize() for the recoverable-error path should not ship in the public SDK because it will always print in consuming apps. Remove the log or guard it behind an explicit debug flag in recognize.ts so the element.dispose() recovery path stays silent by default, and make sure any remaining debug output is only enabled in development or opt-in diagnostics.packages/recognize/src/lib/recognize.types.ts (1)
74-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider strongly-typing
theme,themeOptions,localizationPacks,localizationVariables,loggerLevelinstead ofunknown.The underlying
RootElement(inrecognize-sdk/index.d.ts) types these asTheme,KeylessThemeOptions,LocalizationPack[],LocalizationVariables, andLoggerLevelrespectively — all already exported from the SDK declarations. Typing themunknownhere discards compile-time safety/autocomplete for consumers of the publicRecognizeWebComponentConfigurationAPI.♻️ Proposed fix
+import type { Theme, KeylessThemeOptions, LocalizationPack, LocalizationVariables, LoggerLevel } from './recognize-sdk/index.js'; + export interface RecognizeWebComponentConfiguration { ... - localizationPacks?: unknown[]; - localizationVariables?: unknown; - loggerLevel?: string; + localizationPacks?: LocalizationPack[]; + localizationVariables?: LocalizationVariables; + loggerLevel?: LoggerLevel; ... - theme?: unknown; - themeOptions?: unknown; + theme?: Theme; + themeOptions?: KeylessThemeOptions; ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/recognize.types.ts` around lines 74 - 81, The public RecognizeWebComponentConfiguration type is using unknown for several SDK-backed fields, which removes type safety and autocomplete for consumers. Update the type definitions in recognize.types.ts to match the exported RootElement types from recognize-sdk/index.d.ts, specifically using Theme, KeylessThemeOptions, LocalizationPack[], LocalizationVariables, and LoggerLevel for theme, themeOptions, localizationPacks, localizationVariables, and loggerLevel. Keep the existing RecognizeWebComponentConfiguration interface as the place to align these field types so the API stays strongly typed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/recognize/src/lib/recognize.ts`:
- Around line 126-166: `RecognizeWebComponent.init()` only checks `element !==
null` before the async `import('./recognize-sdk/index.js')`, so concurrent calls
can both pass and create duplicate elements/listeners. Add an in-flight
initialization guard in `init()` (for example, a shared pending flag/promise on
`RecognizeWebComponent`) and reject or no-op when initialization is already
running, then clear that state once the attach/create path completes or fails.
---
Nitpick comments:
In `@packages/recognize/src/lib/recognize.ts`:
- Line 68: The styled console.log in recognize() for the recoverable-error path
should not ship in the public SDK because it will always print in consuming
apps. Remove the log or guard it behind an explicit debug flag in recognize.ts
so the element.dispose() recovery path stays silent by default, and make sure
any remaining debug output is only enabled in development or opt-in diagnostics.
In `@packages/recognize/src/lib/recognize.types.ts`:
- Around line 74-81: The public RecognizeWebComponentConfiguration type is using
unknown for several SDK-backed fields, which removes type safety and
autocomplete for consumers. Update the type definitions in recognize.types.ts to
match the exported RootElement types from recognize-sdk/index.d.ts, specifically
using Theme, KeylessThemeOptions, LocalizationPack[], LocalizationVariables, and
LoggerLevel for theme, themeOptions, localizationPacks, localizationVariables,
and loggerLevel. Keep the existing RecognizeWebComponentConfiguration interface
as the place to align these field types so the API stays strongly typed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cb0709c-4efd-44f8-89cd-f8cde3bb2997
⛔ Files ignored due to path filters (2)
packages/recognize/src/lib/recognize-sdk/pthreads/wasm.wasmis excluded by!**/*.wasmpackages/recognize/src/lib/recognize-sdk/wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (12)
e2e/recognize-app/src/index.tspackages/recognize/src/index.tspackages/recognize/src/lib/classes/recognize-error.tspackages/recognize/src/lib/defs/recognize-error-code.tspackages/recognize/src/lib/defs/recognize-sdk-to-recognize-proxy-error-map.tspackages/recognize/src/lib/recognize-sdk/index.d.tspackages/recognize/src/lib/recognize-sdk/index.jspackages/recognize/src/lib/recognize-sdk/pthreads/wasm.jspackages/recognize/src/lib/recognize-sdk/wasm.jspackages/recognize/src/lib/recognize.spec.tspackages/recognize/src/lib/recognize.tspackages/recognize/src/lib/recognize.types.ts
💤 Files with no reviewable changes (2)
- packages/recognize/src/lib/defs/recognize-error-code.ts
- packages/recognize/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/recognize-app/src/index.ts
- packages/recognize/src/lib/classes/recognize-error.ts
- packages/recognize/src/lib/recognize.spec.ts
…eb component options Update bundled Keyless SDK (index.js, .d.ts, wasm binaries, LICENSE) and wire up the new surface: recognition-start event, aspectRatio/cameraAspectRatio/cameraInstructions and enableDatadogPII configuration, CORE_NOT_ENOUGH_CIRCUITS (3005) and SERVER_AUTHORIZATION_FAILED (5003) error codes, and mappings for SERVER_FORBIDDEN, SERVER_INVALID_STATE, and SERVER_NO_ATTEMPTS_LEFT. Refactor recognize.ts event listener wiring to route recognition-failure through the shared onError path and drop leftover debug logging. Also add pnpm onlyBuiltDependencies allowlist and remove the empty dependencies block from the root package.json. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+76
to
+78
| new KeylessRecoverableErrorEvent({ | ||
| error: new Error('SERVER_RECOGNITION_FAILED', { cause: event.detail }), | ||
| }), |
| }; | ||
|
|
||
| const onError = (event: ErrorEvent): void => { | ||
| if (event instanceof KeylessRecoverableErrorEvent) { |
…vent in mock Aliasing the mock class directly to ErrorEvent made every dispatched ErrorEvent pass the instanceof check in onError and try to call element.dispose(), which does not exist on the underlying custom element. Use a distinct subclass so plain error events skip the recoverable path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/recognize/src/lib/recognize.ts (1)
158-166: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClear observers even when
dispose()runs beforeinit().A subscriber registered before initialization survives
dispose()because Line 159 returns beforeobservers.clear(), so a laterinit()can notify a stale observer.Proposed fix
dispose: (): void => { - if (element === null) return; + if (element === null) { + observers.clear(); + return; + } aborter = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/recognize/src/lib/recognize.ts` around lines 158 - 166, The dispose logic in recognize.ts currently returns early in the dispose() method when element is null, which leaves observers uncleared if dispose() runs before init(). Update the dispose() implementation to always clear observers, even when no element exists yet, while still preserving the existing cleanup for aborter, element.remove(), and element reset; use the dispose() method and observers.clear() as the key symbols to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/recognize/src/lib/recognize.ts`:
- Around line 158-166: The dispose logic in recognize.ts currently returns early
in the dispose() method when element is null, which leaves observers uncleared
if dispose() runs before init(). Update the dispose() implementation to always
clear observers, even when no element exists yet, while still preserving the
existing cleanup for aborter, element.remove(), and element reset; use the
dispose() method and observers.clear() as the key symbols to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0c034a1-9926-418e-8419-e87fe97a4b34
⛔ Files ignored due to path filters (3)
packages/recognize/src/lib/recognize-sdk/pthreads/wasm.wasmis excluded by!**/*.wasmpackages/recognize/src/lib/recognize-sdk/wasm.wasmis excluded by!**/*.wasmpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
package.jsonpackages/recognize/src/lib/classes/recognize-error.tspackages/recognize/src/lib/defs/recognize-error-code.tspackages/recognize/src/lib/defs/recognize-sdk-to-recognize-proxy-error-map.tspackages/recognize/src/lib/recognize-sdk/LICENSEpackages/recognize/src/lib/recognize-sdk/index.d.tspackages/recognize/src/lib/recognize-sdk/index.jspackages/recognize/src/lib/recognize-sdk/pthreads/wasm.jspackages/recognize/src/lib/recognize-sdk/wasm.jspackages/recognize/src/lib/recognize.spec.tspackages/recognize/src/lib/recognize.tspackages/recognize/src/lib/recognize.types.ts
💤 Files with no reviewable changes (1)
- packages/recognize/src/lib/classes/recognize-error.ts
✅ Files skipped from review due to trivial changes (1)
- packages/recognize/src/lib/recognize-sdk/LICENSE
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/recognize/src/lib/defs/recognize-error-code.ts
- packages/recognize/src/lib/defs/recognize-sdk-to-recognize-proxy-error-map.ts
- packages/recognize/src/lib/recognize.spec.ts
- packages/recognize/src/lib/recognize.types.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
@forgerock/recognizelibrary with arecognize()client that supportssubscribe,init, anddispose, plus typed recognition events and configuration options.RecognizeError/RecognizeErrorCodeand improved error handling behavior.Documentation
recognizelibrary.Tests
Chores