Recognize callback - #745
Open
eugeniobet-ping wants to merge 3 commits into
Open
Conversation
|
📝 WalkthroughWalkthroughThe Journey client now supports PingOne Recognize callbacks. A browser-based end-to-end test collects configuration, runs a Journey, integrates the Recognize SDK, handles callback results, and logs final authentication status. ChangesPingOne Recognize callback support
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
cerebrl
reviewed
Aug 4, 2026
cerebrl
left a comment
cerebrl
left a comment
Collaborator
There was a problem hiding this comment.
This is looking good. We just need some unit tests for the callback, and I'd like to see at least some e2e tests that test at least some portion of this feature. I know we can't fully automate the flow, but, like some of our other tests, can we at least test that the parts that can be automated?
eugeniobet-ping
marked this pull request as ready for review
August 11, 2026 15:52
Contributor
|
View your CI Pipeline Execution ↗ for commit 3049a50
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-callback-test.ts`:
- Line 147: Remove sensitive values from logging in
e2e/recognize-app/src/index-callback-test.ts: at lines 147, 103, and 112 log
only non-sensitive status or error identifiers instead of callback data, the
full Web SDK configuration, or unfiltered webSDKOptions; at line 221 do not log
the session token. Preserve the existing test behavior while ensuring JWTs,
usernames, transaction data, and session tokens never reach the browser console
or page.
- Around line 151-154: Update the JWT payload decoding in the callback handling
around recognizeCallback.setRecognizeId to treat the token segment as Base64url:
convert URL-safe characters, restore required padding, decode the resulting
bytes as UTF-8, then parse the JSON. Preserve the existing payload.sub logging
and recognize ID assignment behavior.
- Around line 174-187: Update the client.init() handling in the wrapper promise
so a returned RecognizeError populates the client-error inputs and resolves
instead of rejecting or aborting the Journey flow. Ensure client.dispose() runs
in a finally block, allowing journeyClient.next(step) to execute for both
initialization success and failure.
In `@packages/journey-client/src/lib/callbacks/ping-one-recognize-callback.ts`:
- Around line 25-79: Replace the class-based PingOneRecognizeCallback
implementation with a factory function that returns the callback contract’s
operation and accessor methods while preserving the existing payload behavior
and setters. Update the callback factory and related callback type/contract
references to construct and consume the factory result instead of instantiating
PingOneRecognizeCallback, and remove the class dependency from the client
package.
In `@packages/sdk-types/src/lib/am-callback.types.ts`:
- Line 23: Move the runtime callbackType registry containing
PingOneRecognizeCallback out of the *.types.ts module into a non-types runtime
module, then export/import its derived callback type wherever the type contract
is required. Keep the registry values and callback type behavior unchanged, and
ensure am-callback.types.ts contains only type or interface declarations.
🪄 Autofix
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 Plus
Run ID: 4c890ece-d911-4409-a6db-2a3fd600f6a4
📒 Files selected for processing (6)
e2e/recognize-app/src/index-callback-test.htmle2e/recognize-app/src/index-callback-test.tspackages/journey-client/src/lib/callbacks/factory.tspackages/journey-client/src/lib/callbacks/ping-one-recognize-callback.tspackages/journey-client/src/types.tspackages/sdk-types/src/lib/am-callback.types.ts
| resolve(); | ||
| }, | ||
| complete: (data) => { | ||
| log(`[recognize] complete — data: ${JSON.stringify(data)}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove authentication data from logs.
The test writes callback configuration, Recognize result data, JWTs, and session tokens to the browser console and page. Log only non-sensitive status and error identifiers.
e2e/recognize-app/src/index-callback-test.ts#L147-L147: Do not logdata, because it can containjwt.e2e/recognize-app/src/index-callback-test.ts#L103-L103: Do not log the full Web SDK configuration, because it includes username and transaction data.e2e/recognize-app/src/index-callback-test.ts#L112-L112: Do not log unfilteredwebSDKOptions.e2e/recognize-app/src/index-callback-test.ts#L221-L221: Do not log the session token.
📍 Affects 1 file
e2e/recognize-app/src/index-callback-test.ts#L147-L147(this comment)e2e/recognize-app/src/index-callback-test.ts#L103-L103e2e/recognize-app/src/index-callback-test.ts#L112-L112e2e/recognize-app/src/index-callback-test.ts#L221-L221
🤖 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-callback-test.ts` at line 147, Remove sensitive
values from logging in e2e/recognize-app/src/index-callback-test.ts: at lines
147, 103, and 112 log only non-sensitive status or error identifiers instead of
callback data, the full Web SDK configuration, or unfiltered webSDKOptions; at
line 221 do not log the session token. Preserve the existing test behavior while
ensuring JWTs, usernames, transaction data, and session tokens never reach the
browser console or page.
Comment on lines
+151
to
+154
| const payload = JSON.parse(atob(data.jwt.split('.')[1])); | ||
| if (payload.sub) { | ||
| log(`[recognize] recognizeId from JWT sub: ${payload.sub}`); | ||
| recognizeCallback.setRecognizeId(payload.sub); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find existing Base64url or JWT decoding helpers before adding another implementation.
rg -n -C 4 --glob '*.{ts,tsx}' 'base64url|base64Url|atob\(|TextDecoder|split\('\''\.'\''\)' packages e2eRepository: ForgeRock/ping-javascript-sdk
Length of output: 7404
Decode the JWT payload as Base64url. Normalize - and _, restore padding, and decode the bytes as UTF-8 before parsing JSON. Otherwise, valid JWTs can leave recognizeId unset.
🤖 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-callback-test.ts` around lines 151 - 154, Update
the JWT payload decoding in the callback handling around
recognizeCallback.setRecognizeId to treat the token segment as Base64url:
convert URL-safe characters, restore required padding, decode the resulting
bytes as UTF-8, then parse the JSON. Preserve the existing payload.sub logging
and recognize ID assignment behavior.
Comment on lines
+174
to
+187
| .then((err) => { | ||
| if (err) { | ||
| log(`[recognize] init error: ${err}`); | ||
| reject(err); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| log(`[recognize] init threw: ${err}`); | ||
| console.error('[recognize] init threw:', err); | ||
| reject(err); | ||
| }); | ||
| }); | ||
|
|
||
| client.dispose(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For the repository version of @forgerock/recognize, does a client returned by recognize()requiredispose()afterinit() rejects, and which fields are guaranteed on initialization errors?
💡 Result:
In the repository version of @forgerock/recognize, the client returned by recognize does not require a call to dispose if init rejects, because an initialization failure prevents the client from successfully reaching a state that necessitates cleanup [1]. Regarding initialization errors and guaranteed fields: The library utilizes standardized error handling through RecognizeError and RecognizeErrorCode [1]. When the init method fails, it throws a RecognizeError rather than resolving with it [1]. Because the process fails at the point of initialization, the client instance is typically not considered fully established or active, and you should rely on catching the thrown exception to handle the error, rather than inspecting the client object for guaranteed fields [1]. Developers should ensure that their implementation catches this exception to prevent unhandled promise rejections [1]. To prevent potential memory leaks or stale listeners in cases where the client was successfully initialized but later needs to be stopped, you must call dispose [1]. Specifically, ensure that any abort controller used with the client is properly signaled (e.g., calling aborter.abort) during the disposal process to guarantee that all event listeners are cleaned up [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline e2e/recognize-app/src/index-callback-test.ts 2>/dev/null || true
printf '%s\n' '--- target lines ---'
sed -n '130,240p' e2e/recognize-app/src/index-callback-test.ts
printf '%s\n' '--- recognize imports and API usage ---'
rg -n -C 3 'recognize|client\.init|client\.dispose|journeyClient\.(next|fail)|callback|client-error' e2e/recognize-app/src package.json e2e/recognize-app 2>/dev/null
printf '%s\n' '--- package manifests and lockfile references ---'
rg -n -C 2 '"`@forgerock/recognize`"|`@forgerock/recognize`|ping-javascript-sdk' --glob 'package.json' --glob '*lock*' --glob '*.yaml' --glob '*.yml' .Repository: ForgeRock/ping-javascript-sdk
Length of output: 42673
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- recognize package files ---'
git ls-files packages/recognize
printf '%s\n' '--- recognize package outline ---'
ast-grep outline packages/recognize 2>/dev/null || true
printf '%s\n' '--- init/dispose definitions and references ---'
rg -n -C 5 'class .*Recognize|function recognize|const recognize|init\s*[:=]|dispose\s*[:=]|dispose\(|RecognizeError|errorCode|client-error|setClientError' packages/recognize packages/journey-clientRepository: ForgeRock/ping-javascript-sdk
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- recognize implementation ---'
sed -n '34,185p' packages/recognize/src/lib/recognize.ts
printf '%s\n' '--- recognize types and error codes ---'
sed -n '32,145p' packages/recognize/src/lib/recognize.types.ts
cat packages/recognize/src/lib/defs/recognize-error-code.ts
printf '%s\n' '--- initialization-related tests ---'
sed -n '1,180p' packages/recognize/src/lib/recognize.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 15708
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
impl = Path("packages/recognize/src/lib/recognize.ts").read_text()
app = Path("e2e/recognize-app/src/index-callback-test.ts").read_text()
checks = {
"init_declares_error_union": "Promise<RecognizeError | void>" in impl,
"init_returns_import_error": "return createRecognizeError(RECOGNIZE_ERROR_CODE.SDK_WEB_ASSEMBLY_IMPORT_FAILED" in impl,
"init_returns_attach_error": "return createRecognizeError(RECOGNIZE_ERROR_CODE.SDK_ERROR" in impl
and "invalid element <${tag.toLowerCase()}" in impl,
"element_created_before_mount_append": impl.index("element = document.createElement(tag);")
< impl.index("options.container.appendChild(element);"),
"app_rejects_returned_init_error": ".then((err) =>" in app
and "if (err)" in app
and "reject(err);" in app,
"app_disposes_after_await_only": app.index("client.dispose();")
> app.index("await new Promise<void>"),
"app_advances_after_recognize_branch": "step = await journeyClient.next(step);" in app,
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: ForgeRock/ping-javascript-sdk
Length of output: 421
Handle initialization errors without aborting the Journey flow.
client.init() returns a RecognizeError; it does not reject for these failures. Set the client-error inputs, resolve the wrapper promise, and call client.dispose() in finally so journeyClient.next(step) still runs.
🤖 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-callback-test.ts` around lines 174 - 187, Update
the client.init() handling in the wrapper promise so a returned RecognizeError
populates the client-error inputs and resolves instead of rejecting or aborting
the Journey flow. Ensure client.dispose() runs in a finally block, allowing
journeyClient.next(step) to execute for both initialization success and failure.
Comment on lines
+25
to
+79
| export class PingOneRecognizeCallback extends BaseCallback { | ||
| constructor(public override payload: Callback) { | ||
| super(payload); | ||
| } | ||
|
|
||
| public getOperationType(): PingOneRecognizeOperationType { | ||
| return this.getOutputByName<PingOneRecognizeOperationType>('operationType', 'AUTHENTICATE'); | ||
| } | ||
|
|
||
| public getServiceURL(): string { | ||
| return this.getOutputByName<string>('websocketURL', ''); | ||
| } | ||
|
|
||
| public getCustomerName(): string { | ||
| return this.getOutputByName<string>('customerName', ''); | ||
| } | ||
|
|
||
| public getUsername(): string { | ||
| return this.getOutputByName<string>('username', ''); | ||
| } | ||
|
|
||
| public getTransactionData(): string { | ||
| return this.getOutputByName<string>('transactionData', ''); | ||
| } | ||
|
|
||
| public getOptions(): Record<string, unknown> { | ||
| return this.getOutputByName<Record<string, unknown>>('webSDKOptions', {}); | ||
| } | ||
|
|
||
| public getWebSDKConfig(): PingOneRecognizeWebSDKConfig { | ||
| return { | ||
| customer: { name: this.getCustomerName() }, | ||
| transaction: { data: this.getTransactionData() }, | ||
| username: this.getUsername(), | ||
| ws: { url: this.getServiceURL() }, | ||
| ...this.getOptions(), | ||
| }; | ||
| } | ||
|
|
||
| public setSignedJwt(jwt: string): void { | ||
| this.setInputValue(jwt, 'IDToken1signedJwt'); | ||
| } | ||
|
|
||
| public setRecognizeId(recognizeId: string): void { | ||
| this.setInputValue(recognizeId, 'IDToken1recognizeId'); | ||
| } | ||
|
|
||
| public setClientError(errorMessage: string): void { | ||
| this.setInputValue(errorMessage, 'IDToken1clientError'); | ||
| } | ||
|
|
||
| public setClientErrorCode(errorCode: string): void { | ||
| this.setInputValue(errorCode, 'IDToken1clientErrorCode'); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Use a factory-created callback implementation instead of a class.
This new client-package implementation introduces PingOneRecognizeCallback as a class. Adapt the callback factory and callback contract so the implementation does not require a class.
As per coding guidelines: "packages/*/src/**/*.{ts,tsx}: Initialize client packages through factory functions; do not use classes or singletons."
🤖 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/journey-client/src/lib/callbacks/ping-one-recognize-callback.ts`
around lines 25 - 79, Replace the class-based PingOneRecognizeCallback
implementation with a factory function that returns the callback contract’s
operation and accessor methods while preserving the existing payload behavior
and setters. Update the callback factory and related callback type/contract
references to construct and consume the factory result instead of instantiating
PingOneRecognizeCallback, and remove the class dependency from the client
package.
Source: Coding guidelines
| PasswordCallback: 'PasswordCallback', | ||
| PingOneProtectEvaluationCallback: 'PingOneProtectEvaluationCallback', | ||
| PingOneProtectInitializeCallback: 'PingOneProtectInitializeCallback', | ||
| PingOneRecognizeCallback: 'PingOneRecognizeCallback', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move callbackType out of this type-contract file.
Line 23 adds runtime code to a *.types.ts file. Move the runtime registry to a non-.types.ts module and import its derived type where needed.
As per coding guidelines: "*.types.ts files may contain only type contracts (type and interface); they must contain no runtime code or enum declarations."
🤖 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/sdk-types/src/lib/am-callback.types.ts` at line 23, Move the runtime
callbackType registry containing PingOneRecognizeCallback out of the *.types.ts
module into a non-types runtime module, then export/import its derived callback
type wherever the type contract is required. Keep the registry values and
callback type behavior unchanged, and ensure am-callback.types.ts contains only
type or interface declarations.
Source: Coding guidelines
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (36.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
===========================================
+ Coverage 18.07% 70.96% +52.89%
===========================================
Files 155 139 -16
Lines 24398 7871 -16527
Branches 1203 1484 +281
===========================================
+ Hits 4410 5586 +1176
+ Misses 19988 2285 -17703
🚀 New features to boost your workflow:
|
Contributor
|
Deployed 235507d to https://ForgeRock.github.io/ping-javascript-sdk/pr-745/235507d460477e375c1b59852a60a70fc532185a branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
Interface Mapping Out of DateThe Drift reportTo fix, run: pnpm mapping:generateThen commit the updated |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 93.9 KB (new) 📊 Minor Changes📈 @forgerock/sdk-types - 9.1 KB (+0.0 KB) ➖ No Changes➖ @forgerock/davinci-client - 56.7 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
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