SDKS-4726: Signals SDK Update & Support Additional Init Options#578
SDKS-4726: Signals SDK Update & Support Additional Init Options#578SteinGabriel merged 1 commit intomainfrom
Conversation
SteinGabriel
requested review from
ancheetah,
cerebrl,
ryanbas21 and
vatsalparikh
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds pass-through support for Changes
Sequence Diagram(s)sequenceDiagram
participant AM as "AM (server)"
participant JC as "Journey Client\nPingOneProtectInitializeCallback"
participant App as "App / protect()"
participant Signals as "window._pingOneSignals\n(Signals SDK)"
AM->>JC: callback output (may include\n`signalsInitializationOptions`)
JC->>JC: getConfig() checks output
alt signalsInitializationOptions is plain object
JC-->>App: returns SignalsInitializationOptions (pass-through)
else missing/invalid
JC-->>App: returns constructed ProtectConfig
end
App->>Signals: window._pingOneSignals.init(options)
alt options.behavioralDataCollection === true or "true"
App->>Signals: window._pingOneSignals.resumeBehavioralData()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 22 seconds.Comment |
🦋 Changeset detectedLatest commit: 096733d The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
View your CI Pipeline Execution ↗ for commit 407da51
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
Our analysis found that the @forgerock/api-report:test failures are caused by integration tests narrowly exceeding their 5000ms timeout (actual: 5.3–6.7s), not by any code introduced in this PR. Since @forgerock/api-report was not modified by these changes and no similar failures exist on other branches, this appears to be a transient CI environment performance issue.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I think this looks good. Can we just get a confirmation on the value data type for the config?
| * @type SignalsInitializationOptions - Arbitrary key-value map passed directly to the Signals SDK initialization. | ||
| * Used when AM returns a `signalsInitializationOptions` output on the `PingOneProtectInitializeCallback`. | ||
| */ | ||
| export type SignalsInitializationOptions = Record<string, unknown>; |
There was a problem hiding this comment.
I think we can narrow this down to just Record<string, string>. I'd like to ask @witrisna if the value is limited to just a string. Can you confirm this?
There was a problem hiding this comment.
Confirmed in this Slack thread that Signals SDK values are string-only.
Narrowed SignalsInitializationOptions to Record<string, string>. Also updated the getOutputByName generic in the callback to match.
By the way, this is Record<string, unknown> in the legacy SDK. It's probably fine, just wanted to flag it in case we need to update the legacy version too.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.51%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #578 +/- ##
===========================================
- Coverage 70.90% 17.51% -53.40%
===========================================
Files 53 154 +101
Lines 2021 24151 +22130
Branches 377 1146 +769
===========================================
+ Hits 1433 4229 +2796
- Misses 588 19922 +19334
🚀 New features to boost your workflow:
|
|
Deployed fdab94c to https://ForgeRock.github.io/ping-javascript-sdk/pr-578/fdab94cf4c99d60d8381a010179fdedb3d43c05c branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 90.3 KB (+0.1 KB) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/journey-client/api-report/journey-client.api.md (1)
321-332:⚠️ Potential issue | 🟠 MajorNarrow the
getConfig()return type fromRecord<string, unknown>toRecord<string, string>.The
getConfig()method exposesRecord<string, unknown>, whileSignalsInitializationOptionsinpackages/protect/src/lib/protect.types.ts(line 14) is strictlyRecord<string, string>. This type mismatch weakens interoperability for the "getConfig() → protect(config)" path and forces consumer-side casts when passing the config between these APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/api-report/journey-client.api.md` around lines 321 - 332, The public signature for getConfig() currently allows Record<string, unknown>, which conflicts with SignalsInitializationOptions (Record<string, string>); change the generic index signature in the getConfig() return type from Record<string, unknown> to Record<string, string> so callers can pass getConfig() results directly to protect(config). Update the declaration that reads getConfig(): Record<string, unknown> | { ... } to use Record<string, string> | { ... } (keep the explicit envId/consoleLogEnabled/etc. object shape as-is) and ensure exported type annotations match SignalsInitializationOptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/journey-client/api-report/journey-client.api.md`:
- Around line 321-332: The public signature for getConfig() currently allows
Record<string, unknown>, which conflicts with SignalsInitializationOptions
(Record<string, string>); change the generic index signature in the getConfig()
return type from Record<string, unknown> to Record<string, string> so callers
can pass getConfig() results directly to protect(config). Update the declaration
that reads getConfig(): Record<string, unknown> | { ... } to use Record<string,
string> | { ... } (keep the explicit envId/consoleLogEnabled/etc. object shape
as-is) and ensure exported type annotations match SignalsInitializationOptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d3a8784-2d12-4f3d-9d75-d8c913401748
📒 Files selected for processing (9)
.changeset/sdks-4726-signals-init-options.mdpackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/src/lib/callbacks/ping-protect-initialize-callback.test.tspackages/journey-client/src/lib/callbacks/ping-protect-initialize-callback.tspackages/protect/src/lib/protect.test.tspackages/protect/src/lib/protect.tspackages/protect/src/lib/protect.types.tspackages/protect/src/lib/signals-sdk.js
✅ Files skipped from review due to trivial changes (1)
- .changeset/sdks-4726-signals-init-options.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/protect/src/lib/protect.types.ts
- packages/journey-client/src/lib/callbacks/ping-protect-initialize-callback.ts
- packages/protect/src/lib/protect.test.ts
- packages/journey-client/src/lib/callbacks/ping-protect-initialize-callback.test.ts
chore(protect): update signals-sdk to v5.6.9w chore(protect): fix JSDoc param type and update copyright year to 2026 chore(changeset): add changeset for SDKS-4726 signalsInitializationOptions support fix(protect): narrow SignalsInitializationOptions to Record<string, string> docs: update api report documentation
4 participants
Summary
https://pingidentity.atlassian.net/browse/SDKS-4726
Implements support for the new
signalsInitializationOptionsoutput property onPingOneProtectInitializeCallback, allowing AM to pass arbitrary key-value config directly to the Signals SDK at initialization.Also updates the Signals SDK from v5.6.0w → v5.6.9w.
Changes
packages/journey-clientPingOneProtectInitializeCallback#getConfig()now detectssignalsInitializationOptionsin the callback outputSignalsInitializationOptionsProtectConfigfield-by-field constructionProtectConfig | SignalsInitializationOptionspackages/protectprotect()now acceptsProtectConfig | SignalsInitializationOptionsbehavioralDataCollectionguard handles bothtrue(boolean) and'true'(string)SignalsInitializationOptionstype exported fromprotect.types.tsTests
protect.test.tsto coverRecord<string, unknown>pass-through configHow to test
1. Configure the AM Journey node
In the PingOne Protect Initialize node of your AM journey, scroll to Additional Signals SDK Initialization Options and add one or more key-value pairs:
waitForWindowLoad) and a value (e.g.true).2. Run the journey with the SDK
You can use a sample app that exercises
PingOneProtectInitializeCallback(e.g.reactjs-todo-journeyinsdk-sample-apps):Important: To test it locally, the local SDK build has to be used on the sample app.
getConfig()should return the extra options object directly (e.g.{ waitForWindowLoad: "true" }).protect(config)receives that object and passes it straight to_pingOneSignals.init(config).3. Verify the fallback path still works
Remove all entries from Additional Signals SDK Initialization Options (leave it empty) and re-run the journey — the SDK should initialize using the individual node fields (
behavioralDataCollection, etc.) as before.Screenshot for reference
Summary by CodeRabbit
New Features
Tests
Chores