SDKS-4816: Fix device client type, update READMEs and e2e apps#543
Merged
SDKS-4816: Fix device client type, update READMEs and e2e apps#543
Conversation
📝 WalkthroughWalkthroughThis PR adds TypeScript type annotations across several e2e apps and docs, makes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
🦋 Changeset detectedLatest commit: b4e6029 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit b4e6029
☁️ Nx Cloud last updated this comment at |
@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 (14.75%) 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 #543 +/- ##
==========================================
- Coverage 18.79% 14.75% -4.05%
==========================================
Files 140 153 +13
Lines 27640 26265 -1375
Branches 980 1054 +74
==========================================
- Hits 5195 3875 -1320
+ Misses 22445 22390 -55
🚀 New features to boost your workflow:
|
Contributor
|
Deployed 828590d to https://ForgeRock.github.io/ping-javascript-sdk/pr-543/828590df5f7c103b09b3819efa1123809b0781f2 branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 87.3 KB (new) 📊 Minor Changes📈 @forgerock/device-client - 9.2 KB (+0.0 KB) ➖ No Changes➖ @forgerock/sdk-logger - 1.6 KB 13 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/oidc-client/README.md (1)
19-35:⚠️ Potential issue | 🟡 MinorFix the
token.exchangequickstart call signature.The quickstart shows passing a single object, but the API expects positional
codeandstatearguments as per the actual implementation.📘 Suggested README fix
-const newTokens = await oidcClient.token.exchange({ - /* code, state */ -}); // Returns new tokens or error +const code = '...'; +const state = '...'; +const newTokens = await oidcClient.token.exchange(code, state); // Returns new tokens or error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/oidc-client/README.md` around lines 19 - 35, The README example incorrectly calls oidcClient.token.exchange with a single object; update the quickstart to call the actual implementation signature by passing code and state as positional arguments to oidcClient.token.exchange (e.g., call exchange(code, state)), and update the surrounding comment to reflect it returns new tokens or an error; locate the example block using the symbol oidcClient.token.exchange and replace the single-object call with the positional-argument form.
🤖 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/oidc-client/README.md`:
- Around line 19-35: The README example incorrectly calls
oidcClient.token.exchange with a single object; update the quickstart to call
the actual implementation signature by passing code and state as positional
arguments to oidcClient.token.exchange (e.g., call exchange(code, state)), and
update the surrounding comment to reflect it returns new tokens or an error;
locate the example block using the symbol oidcClient.token.exchange and replace
the single-object call with the positional-argument form.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/soft-moose-read.mde2e/davinci-app/components/fido.tse2e/device-client-app/src/utils/index.tse2e/oidc-app/src/utils/oidc-app.tse2e/protect-app/src/protect-native.tspackages/device-client/README.mdpackages/device-client/src/lib/types/profile-device.types.tspackages/journey-client/README.mdpackages/oidc-client/README.md
ancheetah
changed the title
cerebrl
approved these changes
Mar 4, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/journey-client/README.md (1)
127-131: Narrow caught errors in TypeScript examples for robustness.While strict mode alone doesn't require type narrowing of catch variables, narrowing with
instanceof Erroris a best practice that makes documentation examples more defensive and aligns with TypeScript ecosystem standards. This improves clarity for users working with strict configurations.Suggested doc fix
} catch (error) { - console.error('Initialization failed:', error.message); + console.error( + 'Initialization failed:', + error instanceof Error ? error.message : String(error), + ); }Also applies to: 240-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/README.md` around lines 127 - 131, The example's catch block uses an un-narrowed catch variable; update the try/catch around the journey({ config }) call to treat the caught value as unknown and narrow it with instanceof Error before accessing message — reference the journey function and the client variable in the example and replace direct error.message access with a guarded check (e.g., if (error instanceof Error) { console.error('Initialization failed:', error.message) } else { console.error('Initialization failed:', error) }) so the TypeScript example is robust under strict settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/oidc-client/README.md`:
- Line 282: Change the phrase "error handling pattern" to the hyphenated
compound modifier "error-handling pattern" in the README sentence that starts
"The library uses a consistent error handling pattern..." so the line reads "The
library uses a consistent error-handling pattern." This is a simple grammar
update to the documentation string referencing the check for an `error`
property.
- Around line 206-213: The example calls oidcClient.token.exchange(code, state)
but URLSearchParams.get may return null; validate that the retrieved code and
state are non-null strings before calling token.exchange (check
urlParams.get('code') and urlParams.get('state') and handle the missing-case
with an error/log or redirect), and only pass the validated string values to
oidcClient.token.exchange to avoid runtime errors in the token exchange flow.
- Around line 73-77: The example uses authResponse.code and authResponse.state
without first checking that authorize.background() succeeded; update the sample
to guard the authorize.background() result (authResponse) and only call
oidcClient.token.exchange(authResponse.code, authResponse.state) when
authResponse indicates success (not an error), e.g., by branching on the result
type or presence of an error field before passing authResponse.code and
authResponse.state to token.exchange(); reference authorize.background(),
authResponse, and token.exchange() when implementing the guard.
---
Nitpick comments:
In `@packages/journey-client/README.md`:
- Around line 127-131: The example's catch block uses an un-narrowed catch
variable; update the try/catch around the journey({ config }) call to treat the
caught value as unknown and narrow it with instanceof Error before accessing
message — reference the journey function and the client variable in the example
and replace direct error.message access with a guarded check (e.g., if (error
instanceof Error) { console.error('Initialization failed:', error.message) }
else { console.error('Initialization failed:', error) }) so the TypeScript
example is robust under strict settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e69d5cb-6e09-4752-b621-63b429c219ba
📒 Files selected for processing (7)
e2e/davinci-app/components/fido.tse2e/device-client-app/src/utils/index.tse2e/oidc-app/src/utils/oidc-app.tse2e/protect-app/src/protect-native.tspackages/device-client/README.mdpackages/journey-client/README.mdpackages/oidc-client/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/oidc-app/src/utils/oidc-app.ts
- e2e/device-client-app/src/utils/index.ts
- e2e/protect-app/src/protect-native.ts
- packages/device-client/README.md
- e2e/davinci-app/components/fido.ts
Comment on lines
+73
to
+77
| const authResponse = await oidcClient.authorize.background(); | ||
|
|
||
| // Get tokens | ||
| const tokens = await oidcClient.token.exchange(authResponse.code, authResponse.state); | ||
|
|
There was a problem hiding this comment.
Guard authorize.background() result before using code/state.
This example calls token.exchange() before narrowing out the error branch. If authResponse is an error object, code/state are not guaranteed.
Suggested doc fix
// Start authorization in the background
const authResponse = await oidcClient.authorize.background();
-// Get tokens
-const tokens = await oidcClient.token.exchange(authResponse.code, authResponse.state);
+if ('error' in authResponse) {
+ console.error('Authorization failed:', authResponse.error);
+ return;
+}
+
+// Get tokens
+const tokens = await oidcClient.token.exchange(authResponse.code, authResponse.state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/oidc-client/README.md` around lines 73 - 77, The example uses
authResponse.code and authResponse.state without first checking that
authorize.background() succeeded; update the sample to guard the
authorize.background() result (authResponse) and only call
oidcClient.token.exchange(authResponse.code, authResponse.state) when
authResponse indicates success (not an error), e.g., by branching on the result
type or presence of an error field before passing authResponse.code and
authResponse.state to token.exchange(); reference authorize.background(),
authResponse, and token.exchange() when implementing the guard.
Comment on lines
+206
to
+213
| const urlParams = new URLSearchParams(window.location.search); | ||
| const code = urlParams.get('code'); | ||
| const state = urlParams.get('state'); | ||
|
|
||
| const tokens = await oidcClient.token.exchange(code, state); | ||
| if ('error' in tokens) { | ||
| console.error('Failed to exchange code for tokens:', tokens.error); | ||
| } |
There was a problem hiding this comment.
Validate code and state before token exchange.
URLSearchParams.get() can return null, but token.exchange() requires string inputs. The example should guard missing params.
Suggested doc fix
const urlParams = new URLSearchParams(window.location.search);
const code = urlParams.get('code');
const state = urlParams.get('state');
-const tokens = await oidcClient.token.exchange(code, state);
+if (!code || !state) {
+ console.error('Missing authorization code/state in callback URL');
+ return;
+}
+
+const tokens = await oidcClient.token.exchange(code, state);
if ('error' in tokens) {
console.error('Failed to exchange code for tokens:', tokens.error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const urlParams = new URLSearchParams(window.location.search); | |
| const code = urlParams.get('code'); | |
| const state = urlParams.get('state'); | |
| const tokens = await oidcClient.token.exchange(code, state); | |
| if ('error' in tokens) { | |
| console.error('Failed to exchange code for tokens:', tokens.error); | |
| } | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const code = urlParams.get('code'); | |
| const state = urlParams.get('state'); | |
| if (!code || !state) { | |
| console.error('Missing authorization code/state in callback URL'); | |
| return; | |
| } | |
| const tokens = await oidcClient.token.exchange(code, state); | |
| if ('error' in tokens) { | |
| console.error('Failed to exchange code for tokens:', tokens.error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/oidc-client/README.md` around lines 206 - 213, The example calls
oidcClient.token.exchange(code, state) but URLSearchParams.get may return null;
validate that the retrieved code and state are non-null strings before calling
token.exchange (check urlParams.get('code') and urlParams.get('state') and
handle the missing-case with an error/log or redirect), and only pass the
validated string values to oidcClient.token.exchange to avoid runtime errors in
the token exchange flow.
| }); // Returns new tokens or error | ||
| const existingTokens = await oidcClient.token.get(); // Returns existing tokens or error | ||
| const response = await oidcClient.token.revoke(); // Revokes an access token and returns the response or an error | ||
| The library uses a consistent error handling pattern. All methods return either a success response or an error object. Check if the response contains an `error` property: |
There was a problem hiding this comment.
Use hyphenated compound modifier: error-handling pattern.
Minor doc grammar fix for consistency/readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~282-~282: Use a hyphen to join words.
Context: ...ing The library uses a consistent error handling pattern. All methods return eit...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/oidc-client/README.md` at line 282, Change the phrase "error
handling pattern" to the hyphenated compound modifier "error-handling pattern"
in the README sentence that starts "The library uses a consistent error handling
pattern..." so the line reads "The library uses a consistent error-handling
pattern." This is a simple grammar update to the documentation string
referencing the check for an `error` property.
Collaborator
Author
|
Improved OIDC client README again with AI. Please re-review the readme. |
cerebrl
approved these changes
Mar 6, 2026
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.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4816
Description
Summary by CodeRabbit
New Features
Documentation
Chores