Conversation
🦋 Changeset detectedLatest commit: f173256 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 |
📝 WalkthroughWalkthroughPatch release changeset plus documentation, type/import cleanups, API-surface documentation updates, a behavioral change in iframe redirect error handling, and an un-skipped e2e FIDO test. No broad runtime API signature changes in source code; changes are primarily docs, types, and tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
View your CI Pipeline Execution ↗ for commit f173256
☁️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/device-client/src/lib/device.store.ts (2)
339-344:⚠️ Potential issue | 🟡 MinorJSDoc description inconsistency: "Get profile devices" should be "Delete profile device".
The
@function deleteannotation is correct, but the description on line 340 still says "Get profile devices" which doesn't match the delete operation. Similarly, the@paramdescription on line 344 says "used to update a profile device" but should say "used to delete a profile device".📝 Proposed fix for JSDoc description
/** - * Get profile devices + * Delete a profile device * * `@async` * `@function` delete - * `@param` {ProfileDevicesQuery} query - The query used to update a profile device + * `@param` {ProfileDevicesQuery} query - The query used to delete a profile device * `@returns` {Promise<null | { error: unknown }>} - A promise that resolves to null or an error object if the response is not valid. */
316-321:⚠️ Potential issue | 🟡 MinorJSDoc description inconsistency: "Get profile devices" should describe the update operation.
The description on line 317 says "Get profile devices" but this is the
updatefunction. The description should reflect the update operation.📝 Proposed fix for JSDoc description
/** - * Get profile devices + * Update a profile device * * `@async` * `@function` update * `@param` {ProfileDevicesQuery} query - The query used to update a profile device
🤖 Fix all issues with AI agents
In `@packages/protect/README.md`:
- Around line 73-77: The README code sample has a typo: the client variable is
spelled "jouneyClient" instead of "journeyClient", which would break copy‑paste;
update the snippet to use the correct symbol "journeyClient.next(step)" so it
matches the rest of the docs and any referenced initialization code.
In `@packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts`:
- Around line 116-120: Update the JSDoc for the method that uses hasErrorParams
to indicate that when errorParams are present the function now resolves (not
rejects) with parsedParams for context; locate the docstring above the function
containing hasErrorParams/searchParams/errorParams and change wording from
"rejects on error" to something like "resolves with parsed params when
errorParams are present" and mention cleanup() is called before resolve to
preserve expected behavior.
In `@packages/sdk-effects/storage/README.md`:
- Around line 44-54: The README example uses the type GenericError in the
CustomStorageObject method signatures but doesn't show where it comes from;
update the example to import or document GenericError (e.g., reference
GenericError alongside CustomStorageObject/CustomStorageConfig) so readers can
resolve the type. Specifically, add or mention an import that includes
GenericError (the same module that exports
CustomStorageObject/CustomStorageConfig, or note it comes from
`@forgerock/sdk-types`) and ensure the README example references those symbols
(GenericError, CustomStorageObject, CustomStorageConfig) so the example compiles
for consumers.
In `@packages/sdk-types/README.md`:
- Around line 125-135: Update the README's build and test commands to reference
the correct Nx project name: replace occurrences of "nx build storage" and "nx
test storage" with "nx build sdk-types" and "nx test sdk-types" respectively so
the commands match the actual package; ensure the changes are made in
packages/sdk-types/README.md where the "Building" and "Testing" sections define
those commands.
In `@packages/sdk-utilities/README.md`:
- Around line 84-90: The README’s test command misspells the Nx project target;
update the command string in packages/sdk-utilities/README.md from "nx test
sdk-utiities" to the correct project name "nx test sdk-utilities" so the Nx test
target (sdk-utilities) will run correctly.
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/fido/README.md (1)
7-8: Good addition—browser support note is helpful.The note clearly states the browser requirement for WebAuthn APIs, which is important context for developers.
Optional enhancement: Consider adding browser compatibility details.
To make this more actionable for developers, you could optionally enhance the note with:
- A link to browser compatibility information (e.g., MDN or caniuse.com)
- Mention of how to detect/handle unsupported browsers
- Minimum browser versions if applicable
Example:
-**Note**: To use this module, browser support is required for `navigator.credentials.create` and `navigator.credentials.get` +**Note**: To use this module, browser support is required for [`navigator.credentials.create`](https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create) and [`navigator.credentials.get`](https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/get). Check browser compatibility and consider feature detection before using this module.packages/sdk-effects/iframe-manager/README.md (1)
89-103: Catch block example may be misleading given the new resolve behavior.Since
errorParamsnow causes the promise to resolve (not reject), the catch block at lines 89-103 will only handle truly unexpected errors orinternal_errorcases. The check forerrorResult.error || errorResult.error_descriptionat lines 94-98 within the catch block appears redundant with the new behavior, as server errors viaerrorParamsshould now be handled in the success path (lines 79-82).Consider simplifying the catch block to only handle internal errors:
📝 Suggested simplification
} catch (errorResult) { - // Failure case: Check if it's a known error from the server or an internal error if (errorResult && errorResult.type === 'internal_error') { // Timeout or iframe access error console.error(`Iframe operation failed: ${errorResult.message}`); - } else if (errorResult && (errorResult.error || errorResult.error_description)) { - // Error reported by the authorization server via errorParams - console.error('Silent login failed. Server returned error:', errorResult); - // const errorCode = errorResult.error; - // const errorDesc = errorResult.error_description; } else { // Other unexpected error console.error('An unexpected error occurred:', errorResult); } }
| const myCustomStore: CustomStorageObject = { | ||
| get: async (key: string): Promise<string | null | GenericError> => { | ||
| /* ... custom logic ... */ | ||
| }, | ||
| set: async (key, value) => { | ||
| set: async (key: string, valueToSet: string): Promise<void | GenericError> => { | ||
| /* ... custom logic ... */ | ||
| }, | ||
| remove: async (key) => { | ||
| remove: async (key: string): Promise<void | GenericError> => { | ||
| /* ... custom logic ... */ | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Missing GenericError type reference in custom storage example.
The custom storage methods reference GenericError in their return types (e.g., Promise<string | null | GenericError>) but there's no import or explanation of where this type comes from.
📝 Suggested addition to imports or explanation
Either add to the import:
import { createStorage, type CustomStorageConfig, type CustomStorageObject, type GenericError } from '@forgerock/storage';Or add a note explaining that GenericError is imported from @forgerock/sdk-types.
🤖 Prompt for AI Agents
In `@packages/sdk-effects/storage/README.md` around lines 44 - 54, The README
example uses the type GenericError in the CustomStorageObject method signatures
but doesn't show where it comes from; update the example to import or document
GenericError (e.g., reference GenericError alongside
CustomStorageObject/CustomStorageConfig) so readers can resolve the type.
Specifically, add or mention an import that includes GenericError (the same
module that exports CustomStorageObject/CustomStorageConfig, or note it comes
from `@forgerock/sdk-types`) and ensure the README example references those
symbols (GenericError, CustomStorageObject, CustomStorageConfig) so the example
compiles for consumers.
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
ancheetah
force-pushed
the
readmes-2.0
branch
from
aa00986 to
7042fda
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/device-client/src/lib/device.store.ts (1)
339-344:⚠️ Potential issue | 🟡 MinorRemaining copy-paste JSDoc issues in the
profilesection.The
@functiontag fix on line 343 is correct, but the surrounding JSDoc still has leftover issues from copy-paste:
- Line 340: Description says "Get profile devices" — should be "Delete a profile device" (same issue on line 317 for
update, which says "Get profile devices" instead of "Update a profile device").- Line 344:
@paramsays "The query used to update a profile device" — should say "delete".- Line 322 (the
updatemethod's@returns): Minor typo — "or or an error object" has a duplicated "or".Since this PR targets comment fixes, consider cleaning these up as well.
📝 Proposed JSDoc fixes for the profile section
For the
updatemethod (lines 316–322):/** - * Get profile devices + * Update a profile device * * `@async` * `@function` update * `@param` {ProfileDevicesQuery} query - The query used to update a profile device - * `@returns` {Promise<ProfileDevice | { error: unknown }>} - A promise that resolves to the response data or or an error object if the response is not valid. + * `@returns` {Promise<ProfileDevice | { error: unknown }>} - A promise that resolves to the response data or an error object if the response is not valid. */For the
deletemethod (lines 339–344):/** - * Get profile devices + * Delete a profile device * * `@async` * `@function` delete - * `@param` {ProfileDevicesQuery} query - The query used to update a profile device + * `@param` {ProfileDevicesQuery} query - The query used to delete a profile device * `@returns` {Promise<null | { error: unknown }>} - A promise that resolves to null or an error object if the response is not valid. */packages/sdk-effects/logger/README.md (1)
78-110:⚠️ Potential issue | 🟡 MinorClarify that level filtering applies before delegating to custom logger.
The current text "all log calls will be delegated" is misleading—the implementation applies level filtering before delegation, so calls below the current level never reach the custom logger. Update to:
Suggested fix
-When a custom logger is provided, all log calls will be delegated to your implementation. If no custom logger is provided, the logger falls back to the browser's native `console` methods. +When a custom logger is provided, log calls are first filtered by the current level, then delegated to your implementation. If no custom logger is provided, the logger falls back to the browser's native `console` methods.packages/sdk-effects/iframe-manager/README.md (2)
49-54:⚠️ Potential issue | 🔴 CriticalCritical: Inconsistent error handling documentation.
Lines 49-53 state that failures resolve with error objects, but line 54 says it returns "An
Error" for validation failures (missing/emptysuccessParamsorerrorParams). This is ambiguous:
- Does line 54 mean the promise rejects with an Error (throwing synchronously or rejecting the promise)?
- Or does it mean the promise resolves with an Error object?
This inconsistency will confuse developers about whether they need a
try-catchblock for validation errors.Recommendation: Clarify whether validation errors reject the promise or resolve with an error object. If they reject, update the "On Failure" heading to distinguish between resolution-based errors and rejection-based errors.
89-103:⚠️ Potential issue | 🟠 MajorMajor: Catch block appears inconsistent with documented behavior.
The documentation states (lines 49-53) that internal errors resolve with objects like
{ type: 'internal_error', message: '...' }. However, the example's catch block (lines 89-103) attempts to handle these internal_error objects, which would never execute if they resolve rather than reject.Additionally:
- Lines 94-98 check for
error/error_descriptionin the catch block, but these are already handled at lines 79-82 in the try block.- If the promise always resolves (as stated in line 75), this entire catch block would only execute for the validation Error mentioned in line 54.
Recommendation: Clarify the catch block's purpose:
- If internal_error cases truly resolve (per docs), remove lines 91-93 from the catch block
- If error parameters truly resolve (per docs), remove lines 94-98 from the catch block
- Keep the catch block only if line 54 validation errors actually reject (and document this clearly)
🧹 Nitpick comments (2)
packages/sdk-effects/storage/README.md (1)
70-70: Minor style improvement: incomplete sentence.The sentence lacks a subject. Consider revising for better readability.
✍️ Suggested style fix
-- `type`: Specifies the storage mechanism. Can be `'localStorage'`, `'sessionStorage'`, or `'custom'`. +- `type`: Specifies the storage mechanism. It can be `'localStorage'`, `'sessionStorage'`, or `'custom'`.packages/device-client/README.md (1)
233-236: Simplify the error checking logic.The condition
!Array.isArray(getResponse) || 'error' in getResponseis unnecessarily complex. Given the return typeWebAuthnDevice[] | { error: unknown }, the|| 'error' in getResponsepart is redundant since arrays won't have an 'error' property. A cleaner and more idiomatic approach would be to use just the type guard'error' in getResponse, which TypeScript will properly narrow.📝 Suggested simplification
const getResponse = await apiClient.webAuthn.get(query); -if (!Array.isArray(getResponse) || 'error' in getResponse) { +if ('error' in getResponse) { // handle get device error }
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.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 #526 +/- ##
==========================================
+ Coverage 18.79% 18.82% +0.03%
==========================================
Files 140 142 +2
Lines 27640 27730 +90
Branches 980 991 +11
==========================================
+ Hits 5195 5221 +26
- Misses 22445 22509 +64
🚀 New features to boost your workflow:
|
@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: |
|
Deployed ee435fc to https://ForgeRock.github.io/ping-javascript-sdk/pr-526/ee435fc28b00359d23ee252985553980964da0cf branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.5 KB, -100.0%) 📊 Minor Changes📈 @forgerock/device-client - 9.3 KB (+0.0 KB) ➖ No Changes➖ @forgerock/oidc-client - 23.5 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Looks good. Just left a single comment.
ancheetah
force-pushed
the
readmes-2.0
branch
from
7042fda to
f173256
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/device-client/src/lib/device.store.ts (1)
316-345:⚠️ Potential issue | 🟡 MinorJSDoc descriptions for
profile.updateandprofile.deletestill say "Get profile devices".Lines 317 and 340 both read
Get profile devices, but they document theupdateanddeletemethods respectively. The@functiontags were correctly updated, but the summary lines were missed.📝 Proposed fix
/** - * Get profile devices + * Update a profile device * * `@async` * `@function` update/** - * Get profile devices + * Delete a profile device * * `@async` * `@function` deletepackages/sdk-effects/iframe-manager/README.md (2)
49-54:⚠️ Potential issue | 🟡 MinorAPI reference incorrectly states internal errors "resolve" — they actually reject.
Lines 51–53 describe timeout, cross-origin, and setup failures as resolving, but the implementation uses
reject()for all three (lines 141, 156, and 165 iniframe-manager.effects.ts). Only the error-params case (line 50) truly resolves.Suggested fix: split the section into "Resolves" (error params) and "Rejects" (internal errors).
Proposed doc fix
- **On Failure**: Resolves with: - - `ResolvedParams`: An object containing _all_ parsed query parameters if the final redirect URL contains any key listed in `errorParams`. - - An object `{ type: 'internal_error', message: 'iframe timed out' }` if the specified `timeout` is reached before a success or error condition is met. - - An object `{ type: 'internal_error', message: 'unexpected failure' }` if there's an error accessing the iframe's content window (most likely due to a cross-origin redirect that wasn't expected or handled). - - An object `{ type: 'internal_error', message: 'error setting up iframe' }` if there was an issue creating or configuring the iframe initially. - - An `Error` if `successParams` or `errorParams` are missing or empty during setup. + **On Error Params**: Resolves with `ResolvedParams` containing _all_ parsed query parameters if the final redirect URL contains any key listed in `errorParams`. The caller must inspect the result for error keys. + **On Failure (Rejects)**: Rejects with: + - `{ type: 'internal_error', message: 'iframe timed out' }` if the specified `timeout` is reached. + - `{ type: 'internal_error', message: 'unexpected failure' }` on cross-origin access errors. + - `{ type: 'internal_error', message: 'error setting up iframe' }` on iframe creation failure. + - Throws an `Error` synchronously if `successParams` or `errorParams` are missing or empty.
89-102:⚠️ Potential issue | 🟡 MinorCatch block in example contains dead code for the new error-params flow.
Lines 94–98 check for
errorResult.error/errorResult.error_descriptioninside thecatchblock, but with the new behavior, error params now resolve (handled on lines 79–82 in thetryblock). This branch is unreachable and will confuse readers about the new contract.Suggested cleanup
} catch (errorResult) { - // Failure case: Check if it's a known error from the server or an internal error if (errorResult && errorResult.type === 'internal_error') { // Timeout or iframe access error console.error(`Iframe operation failed: ${errorResult.message}`); - } else if (errorResult && (errorResult.error || errorResult.error_description)) { - // Error reported by the authorization server via errorParams - console.error('Silent login failed. Server returned error:', errorResult); - // const errorCode = errorResult.error; - // const errorDesc = errorResult.error_description; } else { // Other unexpected error console.error('An unexpected error occurred:', errorResult); } }
3 participants
JIRA Ticket
Description
Updates readmes for most packages except
sdk-oidcandjourney-clientSummary by CodeRabbit
Documentation
New Features / API
Tests
Chores