Recognize setup#691
Open
eugeniobet-ping wants to merge 12 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
E2E recognize-app
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
+63
to
+67
| const onError = (e: ErrorEvent) => { | ||
| const code: RecognizeErrorCode = | ||
| RECOGNIZE_SDK_TO_RECOGNIZE_PROXY_ERROR_MAP[e.error.message] ?? RecognizeErrorCode.SDK_ERROR; | ||
|
|
||
| const error: RecognizeError = new RecognizeError(code, { cause: e.error }); |
There was a problem hiding this comment.
Harden error event parsing to avoid handler crashes.
ErrorEvent.error is not guaranteed to be an Error. Accessing e.error.message directly can throw and skip observer notification.
Suggested change
const onError = (e: ErrorEvent) => {
- const code: RecognizeErrorCode =
- RECOGNIZE_SDK_TO_RECOGNIZE_PROXY_ERROR_MAP[e.error.message] ?? RecognizeErrorCode.SDK_ERROR;
+ const reason =
+ e.error instanceof Error
+ ? e.error.message
+ : typeof e.error === 'string'
+ ? e.error
+ : e.message;
+ const code: RecognizeErrorCode =
+ RECOGNIZE_SDK_TO_RECOGNIZE_PROXY_ERROR_MAP[reason] ?? RecognizeErrorCode.SDK_ERROR;
- const error: RecognizeError = new RecognizeError(code, { cause: e.error });
+ const error: RecognizeError = new RecognizeError(code, { cause: e.error ?? e });🤖 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 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.
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.
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 entry withrecognize()client support for subscribe/init/dispose.RecognizeErrorCodeandRecognizeErrorhandling, plus expanded typed event and configuration surfaces.Documentation
Tests
Chores