Recognize setup#691
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ors for initialization and element validation
…rove type definitions
🦋 Changeset detectedLatest commit: 9c87943 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
…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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the Changes
E2E recognize-app
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant recognize
participant SDK as Keyless SDK
participant Element as kl-auth or kl-enroll
participant Observer
Caller->>recognize: recognize(configuration)
recognize-->>Caller: RecognizeWebComponentClient
Caller->>recognize: init(mount or attach options)
recognize->>SDK: dynamically import SDK
recognize->>Element: create or attach element
recognize->>Element: assign configuration and listeners
Element-->>recognize: recognition and lifecycle events
recognize->>Observer: next, complete, or error
Caller->>recognize: dispose()
recognize->>Element: remove element
Possibly related PRs
🚥 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
| 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.
| 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.
| 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.
| 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.
| 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.
| 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.
| 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.
There was a problem hiding this comment.
I think this comment here is valid.
There was a problem hiding this comment.
@ryanbas21, thanks for confirming. To recap the fix for when it's implemented:
dispose: (): void => {
if (element === null) return;
+ aborter?.abort();
aborter = null;
element.remove();This ensures the AbortController actually cancels the registered listeners before the reference is dropped, preventing stale listeners from lingering on the removed element.
(っ˘ω˘ς )
There was a problem hiding this comment.
This looks like a legitimate issue. Worth looking into it.
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!
| 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.
| 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.
| 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}
| 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
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/recognize/package.json`:
- Around line 31-34: Move `@aracna/web-components` from devDependencies into
dependencies or peerDependencies in packages/recognize/package.json so published
consumers can resolve the types imported by recognize-sdk/index.d.ts.
In `@packages/recognize/README.md`:
- Around line 23-34: Update the README usage example’s subscribe next handler to
access event.detail only after narrowing the RecognizeWebComponentEvent variant
that defines it, and ensure the init container passed from
document.getElementById('app') is narrowed to a non-null HTMLElement before
calling client.init.
🪄 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: f59c131f-5771-499d-a4a5-6fbc0bd5baea
📒 Files selected for processing (7)
.changeset/config.json.changeset/recognize-initial-release.mdpackages/recognize/README.mdpackages/recognize/eslint.config.mjspackages/recognize/package.jsonpackages/recognize/src/lib/recognize.spec.tspackages/recognize/typedoc.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/recognize/eslint.config.mjs
- packages/recognize/src/lib/recognize.spec.ts
| "dependencies": {}, | ||
| "devDependencies": { | ||
| "@aracna/web-components": "^1.1.18" | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if `@aracna/web-components` is imported in exported types.
# Test: Search for `@aracna/web-components` imports in the lib directory
rg '`@aracna`' packages/recognize/src/lib/Repository: ForgeRock/ping-javascript-sdk
Length of output: 3242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/recognize/package.json ---'
cat -n packages/recognize/package.json
echo
echo '--- related package manifests mentioning `@aracna/web-components` ---'
rg -n '"`@aracna/web-components`"|peerDependencies|dependencies|devDependencies' packages/recognize package.json -g 'package.json'Repository: ForgeRock/ping-javascript-sdk
Length of output: 2533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('packages/recognize/package.json')
print(p.read_text())
PYRepository: ForgeRock/ping-javascript-sdk
Length of output: 1776
Move @aracna/web-components out of devDependencies
packages/recognize/src/lib/recognize-sdk/index.d.ts is part of the published API and imports types from @aracna/web-components, so consumers need that package available to resolve @forgerock/recognize. Move it to dependencies or peerDependencies.
🤖 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/package.json` around lines 31 - 34, Move
`@aracna/web-components` from devDependencies into dependencies or
peerDependencies in packages/recognize/package.json so published consumers can
resolve the types imported by recognize-sdk/index.d.ts.
| client.subscribe({ | ||
| next: (event) => console.log('[recognize]', event.type, event.detail), | ||
| error: (err) => console.error('[recognize] error', err), | ||
| complete: (detail) => console.log('[recognize] complete', detail), | ||
| }); | ||
|
|
||
| await client.init({ | ||
| mode: 'mount', | ||
| container: document.getElementById('app'), | ||
| type: 'auth', | ||
| username: 'USERNAME', | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix TypeScript compilation errors in the usage example.
In strict mode, TypeScript users copying this example will encounter compilation errors:
event.detailwill throw an error because theRecognizeWebComponentEventunion contains variants without adetailproperty (e.g.,{ type: 'non-cancelable' }).document.getElementById('app')returnsHTMLElement | null, but thecontainerproperty strictly requires anHTMLElement.
📝 Proposed fix to make the example strictly typed
client.subscribe({
- next: (event) => console.log('[recognize]', event.type, event.detail),
+ next: (event) => console.log('[recognize]', event.type, 'detail' in event ? event.detail : undefined),
error: (err) => console.error('[recognize] error', err),
complete: (detail) => console.log('[recognize] complete', detail),
});
await client.init({
mode: 'mount',
- container: document.getElementById('app'),
+ container: document.getElementById('app') as HTMLElement,
type: 'auth',
username: 'USERNAME',
});📝 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.
| client.subscribe({ | |
| next: (event) => console.log('[recognize]', event.type, event.detail), | |
| error: (err) => console.error('[recognize] error', err), | |
| complete: (detail) => console.log('[recognize] complete', detail), | |
| }); | |
| await client.init({ | |
| mode: 'mount', | |
| container: document.getElementById('app'), | |
| type: 'auth', | |
| username: 'USERNAME', | |
| }); | |
| client.subscribe({ | |
| next: (event) => console.log('[recognize]', event.type, 'detail' in event ? event.detail : undefined), | |
| error: (err) => console.error('[recognize] error', err), | |
| complete: (detail) => console.log('[recognize] complete', detail), | |
| }); | |
| await client.init({ | |
| mode: 'mount', | |
| container: document.getElementById('app') as HTMLElement, | |
| type: 'auth', | |
| username: 'USERNAME', | |
| }); |
🤖 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/README.md` around lines 23 - 34, Update the README usage
example’s subscribe next handler to access event.detail only after narrowing the
RecognizeWebComponentEvent variant that defines it, and ensure the init
container passed from document.getElementById('app') is narrowed to a non-null
HTMLElement before calling client.init.
| */ | ||
|
|
||
| /** @public */ | ||
| export enum RecognizeErrorCode { |
There was a problem hiding this comment.
Typically we try to avoid enums because they create runtime and type level code that compiles into an iife and is not treeshaekable. They also can be considered a bad typescript practice because there are gotcha's that are not obvious to all developers.
The alternative approach would be to use a typed as const object and export the type level and object literal expression.
i.e.
// produces a runtime object and a strictly typed object
const RecognizeErrorCode = {
SDK_ERROR: 1000
} as const
// if you want a union of allowed keys
export type RecognizeErrorCodes = keyof typeof RecognizeErrorCode
// if you want a union of allowed values
export type RecognizeErrorCodes = typeof RecognizeErrorCodes[keyof typeof RecognizeErrorCodes]| const onRecognitionFailure = (event: KeylessRecognitionFailureEvent): void => { | ||
| return onError( | ||
| new KeylessRecoverableErrorEvent({ | ||
| error: new Error('SERVER_RECOGNITION_FAILED', { cause: event.detail }), |
There was a problem hiding this comment.
This seems interesting, why do we want an Error that contains an Error object?
what is the benefit of having the error key be an error object as well? Since we're already in an Error type it seems?
There was a problem hiding this comment.
We should probably update this.
| @@ -0,0 +1,11 @@ | |||
| /** @internal */ | |||
| export const CAMERA_ONLY_DISABLE_STEPS: string[] = [ | |||
There was a problem hiding this comment.
I'm not sure the benefit - if it's helpful or not, but we can make this an as const array and rather than "string[]" we can have a more type safe array of these literals.
If it's possible for any string then this can be ignored, but it seems the steps here are finite.
|
|
||
| import { RecognizeErrorCode } from './recognize-error-code.js'; | ||
|
|
||
| export const RECOGNIZE_SDK_TO_RECOGNIZE_PROXY_ERROR_MAP: Record<string, RecognizeErrorCode> = |
There was a problem hiding this comment.
This seems rather redundant, if we go with an as const object, could we just freeze the actual object and not recreate it here?
| dispose: (): void => { | ||
| if (element === null) return; | ||
|
|
||
| aborter = null; | ||
|
|
There was a problem hiding this comment.
I think this comment here is valid.
| const observers: Set<RecognizeWebComponentObserver> = new Set(); | ||
|
|
||
| let element: RecognizeWebComponent | null = null; | ||
| let aborter: AbortController | null = null; |
There was a problem hiding this comment.
This is tricky to follow, i've read it a few times and I think i've got it.
We store an abort controller in the closure.
when we add an event listener, we mutate the controller, and hoist the AbortController to line 41.
We pass the controller to each event listener.
on dispose we set it to null
In regards to each event listener, we never call abort() so the signal being passed isn't doing much.
An AbortSignal. The listener will be removed when the abort() method of the AbortController which owns the AbortSignal is called. If not specified, no AbortSignal is associated with the listener
As a result, we're really just flipping a variable to null in dispose and that's really all it's doing?
I think, we can get rid of the mutation entirely. Since an event listener per MDN will be removed when we call abort we can simply initialize one AbortSignal at the top, and not need to mutate it and keep track of it's existence as null. It's just an AbortSignal.
In dispose, we can then simply call aborter.abort() and this will clean up all the event listeners that share that signal, and properly dispose of the aborter itself.
This works if all event listeners are intended to be disposed of together, and not on their own, which I believe is the intention since the same AbortSignal is being passed.
| const setAttributes = (element: RecognizeWebComponent): void => { | ||
| for (const [k, v] of Object.entries(config)) { | ||
| element[k as keyof RecognizeWebComponentConfiguration] = v; | ||
| } | ||
| }; |
There was a problem hiding this comment.
It may be cleaner to make this a utility function and import it, that way the closure doesn't need all this state, just make setAttributes accept a config as well or an object of { config, element } ?
There was a problem hiding this comment.
We like to use the convention of separating runtime code and type definitions. We try to only export the runtime code in the root index.ts file, and then export all types needed for the dev through types/.
There was a problem hiding this comment.
We like to treat errors within the SDK as plain objects, rather than extensions of the Error class. Could we convert this to a regular object?
|
|
||
| async init(options: RecognizeWebComponentInitOptions): Promise<void> { | ||
| if (element !== null) { | ||
| throw new RecognizeError(RecognizeErrorCode.SDK_ERROR, { |
There was a problem hiding this comment.
Our SDK library overall avoids "throwing" errors and treats errors as valid returned values. The one exception is on the factory function itself. If there's a configuration error, the factory function throws when it can't return a proper API object. Since this is inside the API object, we would prefer to return the error, rather than throw it.
There was a problem hiding this comment.
Before we officially publish this, we will need to add a summary, instructions, example code, and how to find more documentation and such to this README.
| */ | ||
|
|
||
| /** @public */ | ||
| export enum RecognizeErrorCode { |
There was a problem hiding this comment.
We try to avoid enums in our code. I'm guessing this enum is preferred to avoid writing a structure for runtime and a duplicate structure for types, which both need to be identical. What we do is use a combination of as const, typeof keyof techniques along with unions.
| throw new RecognizeError(RecognizeErrorCode.SDK_ERROR, { | ||
| cause: 'init() called more than once — call dispose() before re-initializing', | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
| await import('./recognize-sdk/index.js'); | ||
| } catch (error: unknown) { | ||
| throw new RecognizeError(RecognizeErrorCode.SDK_WEB_ASSEMBLY_IMPORT_FAILED, { | ||
| cause: error, | ||
| }); | ||
| } | ||
|
|
||
| if (options.mode === 'attach') { | ||
| const tag: string = options.element.tagName; | ||
|
|
||
| if (tag !== 'KL-AUTH' && tag !== 'KL-ENROLL') { | ||
| throw new RecognizeError(RecognizeErrorCode.SDK_ERROR, { | ||
| cause: `invalid element <${tag.toLowerCase()}> — options.element must be a <kl-auth> or <kl-enroll> custom element`, | ||
| }); |
There was a problem hiding this comment.
In recognize.types.ts:59, RecognizeWebComponentClient.init is typed as Promise<void | RecognizeError>. As a result, RecognizeError should be returned in all three failure paths, or the return type should be changed to Promise<void> and document that it rejects, if the current behavior is intentional.
| const dispatch = (type: RecognizeWebComponentEvent['type'], detail: any): void => { | ||
| for (const observer of observers) { | ||
| observer.next({ type, detail }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
To avoid using any here, we could pass the whole event as one parameter, if possible.
const dispatch = (event: RecognizeWebComponentEvent) => {
observers.forEach((o) => o.next(event));
};
//...
dispatch({ type: 'begin-stream', detail: undefined });
dispatch({ type: 'data', detail: payload }); // `type` and `detail` type-checked| if (k === 'key') { | ||
| element.publicKey = v; | ||
| } else { | ||
| element[k as keyof RecognizeWebComponentConfiguration] = v; |
There was a problem hiding this comment.
Casting k as typeof RecognizeWebComponentConfiguration is an unsafe as cast and a possible structural mismatch, since not all keys of RecognizeWebComponentConfiguration
are assignable elements. One way to avoid this — although more verbose — would be to map every key explicitly:
const setAttributes = (element: RecognizeWebComponent): void => {
if (config.key !== undefined) element.publicKey = config.key;
if (config.lang !== undefined) element.setAttribute('lang', config.lang);
if (config.theme !== undefined) element.theme = config.theme;
if (config.disableSteps !== undefined) element.disableSteps = config.disableSteps;
// one line per attribute, each type-checked
};
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
Initial review comments
| import { RecognizeErrorCode } from '../defs/recognize-error-code.js'; | ||
|
|
||
| /** @public */ | ||
| export class RecognizeError extends Error { |
There was a problem hiding this comment.
Should we enforce functional patterns here?
| */ | ||
|
|
||
| /** @public */ | ||
| export enum RecognizeErrorCode { |
There was a problem hiding this comment.
Should we enforce functional patterns here?
There was a problem hiding this comment.
We should rename this file to recognize.test.ts to match the existing repo conventions.
| } | ||
|
|
||
| /** @public */ | ||
| export type RecognizeWebComponent = KeylessAuthElement | KeylessEnrollElement; |
There was a problem hiding this comment.
Is there a reason for RecognizeWebComponent to be public? In RecognizeWebComponentInitOptions we accept the type to be HTMLElement and then recast as RecognizeWebComponent here https://github.com/ForgeRock/ping-javascript-sdk/pull/691/changes#diff-b0b49fe3cbdbba36d1da1a653b81bcadac00c066e375dd2e1d996c169add6189R131-R141, so publicly we should avoid providing RecognizeWebComponent
| localizationPacks?: unknown[]; | ||
| localizationVariables?: unknown; | ||
| loggerLevel?: string; | ||
| operationID?: string; | ||
| seedEntropy?: boolean; | ||
| serviceURL: string; | ||
| theme?: unknown; | ||
| themeOptions?: unknown; |
There was a problem hiding this comment.
Instead of these unknown types, can we use already defined types like Theme, KeylessThemeOptions? We might need to add a dependency though, so may not be worth it.
| /** @public */ | ||
| export enum RecognizeErrorCode { | ||
| SDK_ERROR = 1000, | ||
| SDK_NOT_CONFIGURED = 1001, |
There was a problem hiding this comment.
We are defining error codes here, but a lot of them seem to be unused. Are they being handled by wasm pre-compiled code?
| dispose: (): void => { | ||
| if (element === null) return; | ||
|
|
||
| aborter = null; | ||
|
|
There was a problem hiding this comment.
This looks like a legitimate issue. Worth looking into it.
6 participants
Summary by CodeRabbit
Release Notes
New Features
@forgerock/recognizeSDK with arecognize()client supportingsubscribe,init, anddispose, including typed recognition events and configuration options.RecognizeErrorandRecognizeErrorCodefor clearer, code-based error reporting.Documentation
@forgerock/recognizeREADME with installation, usage, and configuration option details.Tests
Chores