-
Notifications
You must be signed in to change notification settings - Fork 3
Release PR #506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Release PR #506
Conversation
📝 WalkthroughWalkthroughRemoved several changeset files and bumped many package versions (mostly 1.3.0 → 2.0.0; device-client 0.0.1 → 2.0.0). Added top-level 2.0.0 entries to multiple CHANGELOGs. No source code or public API changes. Changes
Sequence Diagram(s)(omitted — changes are metadata/version bumps and deletions; no new multi-component control flow to diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. 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 e73b12f
☁️ Nx Cloud last updated this comment at |
8753b12 to
d58c175
Compare
@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 (18.82%) 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 #506 +/- ##
==========================================
+ 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:
|
|
Deployed 2cce169 to https://ForgeRock.github.io/ping-javascript-sdk/pr-506/2cce169e3eb49bac813e20845004ba20b860103e 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%) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
edb5c2a to
a86d5eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-effects/logger/CHANGELOG.md`:
- Around line 3-8: Update the 2.0.0 changelog entry in CHANGELOG.md to include
the rationale for the major bump (e.g., rebranding or architectural changes), an
explicit "Breaking changes" subsection listing any API/behavioral changes users
must address, and a short "Migration" subsection with concrete steps or code
examples to help users upgrade; reference the PR (`#502`) and commit (9ad4062) for
details and link to relevant docs or upgrade guides if available.
🧹 Nitpick comments (3)
packages/device-client/CHANGELOG.md (1)
1-7: Consider adding more context about breaking changes.The changelog entry is correctly formatted but quite minimal for a major version bump (0.0.1 → 2.0.0). Users upgrading would benefit from understanding what breaking changes or API modifications are included in version 2.
Consider enhancing the description with:
- Key breaking changes introduced
- Migration guidance or notes
- Links to detailed migration documentation
This would improve the upgrade experience for package consumers.
packages/sdk-types/CHANGELOG.md (1)
3-8: Consider adding more detail about what changed in v2.0.0.The changelog entry correctly follows the established format but provides limited information about the major version bump. Users upgrading from v1.x would benefit from understanding what breaking changes (if any) justify the major version, or clarification that this is a coordinated monorepo version synchronization.
📝 Example of a more informative changelog entry
## 2.0.0 ### Major Changes -- [`#502`](https://github.com/ForgeRock/ping-javascript-sdk/pull/502) [`9ad4062`](https://github.com/ForgeRock/ping-javascript-sdk/commit/9ad406268dd568d8d6f6447a07b656e317a9da8d) Thanks [`@ryanbas21`](https://github.com/ryanbas21)! - releasing version 2 of the ping javascript sdk +- [`#502`](https://github.com/ForgeRock/ping-javascript-sdk/pull/502) [`9ad4062`](https://github.com/ForgeRock/ping-javascript-sdk/commit/9ad406268dd568d8d6f6447a07b656e317a9da8d) Thanks [`@ryanbas21`](https://github.com/ryanbas21)! - Release version 2.0.0 of the Ping JavaScript SDK. This is a coordinated major version bump across all packages in the monorepo. No breaking API changes in this package; see individual package changelogs for details on new features and fixes included in this release.packages/protect/CHANGELOG.md (1)
3-8: Document breaking changes or migration path for 2.0.0.The changelog entry for version 2.0.0 is generic and doesn't specify what breaking changes exist or how users should migrate from 1.3.0. While the README provides comprehensive usage documentation, it lacks a migration guide. Consider adding breaking change details or a link to migration documentation in the changelog.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.changeset/eleven-baboons-battle.md.changeset/full-bikes-boil.md.changeset/hip-owls-rule.mdpackages/davinci-client/CHANGELOG.mdpackages/davinci-client/package.jsonpackages/device-client/CHANGELOG.mdpackages/device-client/package.jsonpackages/journey-client/CHANGELOG.mdpackages/journey-client/package.jsonpackages/oidc-client/CHANGELOG.mdpackages/oidc-client/package.jsonpackages/protect/CHANGELOG.mdpackages/protect/package.jsonpackages/sdk-effects/iframe-manager/CHANGELOG.mdpackages/sdk-effects/iframe-manager/package.jsonpackages/sdk-effects/logger/CHANGELOG.mdpackages/sdk-effects/logger/package.jsonpackages/sdk-effects/oidc/CHANGELOG.mdpackages/sdk-effects/oidc/package.jsonpackages/sdk-effects/sdk-request-middleware/CHANGELOG.mdpackages/sdk-effects/sdk-request-middleware/package.jsonpackages/sdk-effects/storage/CHANGELOG.mdpackages/sdk-effects/storage/package.jsonpackages/sdk-types/CHANGELOG.mdpackages/sdk-types/package.jsonpackages/sdk-utilities/CHANGELOG.mdpackages/sdk-utilities/package.json
💤 Files with no reviewable changes (3)
- .changeset/hip-owls-rule.md
- .changeset/eleven-baboons-battle.md
- .changeset/full-bikes-boil.md
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/journey-client/package.json
- packages/davinci-client/CHANGELOG.md
- packages/oidc-client/package.json
- packages/device-client/package.json
- packages/sdk-utilities/CHANGELOG.md
- packages/sdk-types/package.json
- packages/sdk-effects/iframe-manager/CHANGELOG.md
- packages/oidc-client/CHANGELOG.md
- packages/protect/package.json
- packages/sdk-effects/sdk-request-middleware/package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T15:14:00.044Z
Learnt from: ryanbas21
Repo: ForgeRock/ping-javascript-sdk PR: 430
File: packages/journey-client/src/lib/callbacks/name-callback.ts:9-15
Timestamp: 2025-10-22T15:14:00.044Z
Learning: In packages/journey-client, callback classes are internal implementation details not part of the public API. The callbacks barrel (src/lib/callbacks/index.ts) intentionally only exports the base JourneyCallback class. Internal code imports concrete callback classes directly from their individual files (e.g., factory.ts, journey-client.ts).
Applied to files:
packages/journey-client/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pr
🔇 Additional comments (10)
packages/sdk-effects/logger/package.json (1)
3-3: LGTM!The version bump from 1.3.0 to 2.0.0 is consistent with the coordinated major release across the monorepo.
packages/sdk-effects/iframe-manager/package.json (1)
3-3: LGTM!Version bump to 2.0.0 is consistent with the coordinated major release across all packages in this Changesets-managed release PR.
packages/davinci-client/package.json (1)
3-3: LGTM!The version bump from 1.3.0 to 2.0.0 is correctly formatted and aligns with the coordinated major release across the SDK packages. The
workspace:*specifiers for internal dependencies will be resolved to their published versions (2.0.0) at release time.packages/sdk-effects/oidc/package.json (1)
3-3: LGTM!Version bump to 2.0.0 is consistent with the coordinated major release across the SDK packages.
packages/sdk-effects/oidc/CHANGELOG.md (1)
3-13: LGTM!The 2.0.0 changelog entry is well-structured with proper PR/commit references and correctly documents the major release along with the dependency updates to the 2.0.0 versions of
@forgerock/sdk-typesand@forgerock/sdk-utilities.packages/sdk-utilities/package.json (1)
3-3: LGTM!Version bump from 1.3.0 to 2.0.0 is consistent with the coordinated major release across all packages in this monorepo. The package.json structure is valid, and this change aligns with the Changesets release workflow.
packages/sdk-effects/storage/package.json (1)
3-3: LGTM!Version bump to 2.0.0 is consistent with the major release across the monorepo. The
workspace:*reference for@forgerock/sdk-typeswill be resolved to the correct version at publish time by the Changesets tooling.packages/sdk-effects/storage/CHANGELOG.md (1)
3-12: LGTM!The 2.0.0 changelog entry follows the standard Changesets format with proper PR/commit references and correctly notes the dependency update to
@forgerock/sdk-types@2.0.0. Version alignment is consistent withpackage.json.packages/journey-client/CHANGELOG.md (1)
3-19: LGTM!The changelog entry for version 2.0.0 is well-formatted and accurate. All PR references, commit hashes, and dependency versions match the PR objectives. The structure is consistent with the existing changelog format.
packages/sdk-effects/sdk-request-middleware/CHANGELOG.md (1)
3-8: LGTM! Changelog entry follows standard release format.The version 2.0.0 changelog entry is properly formatted with correct references to PR
#502and the associated commit. While the description is generic, this is typical for Changesets-automated release PRs where detailed changes are tracked in individual changeset files.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ## 2.0.0 | ||
|
|
||
| ### Major Changes | ||
|
|
||
| - [#502](https://github.com/ForgeRock/ping-javascript-sdk/pull/502) [`9ad4062`](https://github.com/ForgeRock/ping-javascript-sdk/commit/9ad406268dd568d8d6f6447a07b656e317a9da8d) Thanks [@ryanbas21](https://github.com/ryanbas21)! - releasing version 2 of the ping javascript sdk | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add more context to the changelog entry.
The changelog entry for version 2.0.0 is quite generic. Consider documenting:
- The specific reason for the major version bump (e.g., rebranding, architectural changes, etc.)
- Whether there are breaking changes users need to address
- Migration guidance if needed
This helps package consumers understand the impact of upgrading and whether they need to make changes to their code.
🤖 Prompt for AI Agents
In `@packages/sdk-effects/logger/CHANGELOG.md` around lines 3 - 8, Update the
2.0.0 changelog entry in CHANGELOG.md to include the rationale for the major
bump (e.g., rebranding or architectural changes), an explicit "Breaking changes"
subsection listing any API/behavioral changes users must address, and a short
"Migration" subsection with concrete steps or code examples to help users
upgrade; reference the PR (`#502`) and commit (9ad4062) for details and link to
relevant docs or upgrade guides if available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
2c38f40 to
f0cccc7
Compare
f0cccc7 to
e73b12f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-effects/storage/CHANGELOG.md`:
- Around line 11-14: The OIDC client’s error handling is inconsistent: update
the revoke, info, and logout flows that call storageClient.get()/set()/remove()
to use the sdk-utilities isGenericError type guard instead of relying on
implicit checks like '!tokens || !("accessToken" in tokens)' or ad-hoc '"error"
in tokens' checks; import isGenericError and replace those conditional branches
in the revoke, info, and logout handlers (and any other storageClient
interaction) to explicitly check isGenericError(result) and handle/log the error
path consistently while preserving the existing success-path logic.
🧹 Nitpick comments (1)
packages/sdk-effects/storage/CHANGELOG.md (1)
5-7: Consider enhancing the major version description.The major version bump to 2.0.0 is documented with a brief description "releasing version 2 of the ping javascript sdk." For major releases, users typically benefit from more detailed documentation of:
- What breaking changes are included
- What migration steps are required
- What deprecated APIs were removed
If this information is documented elsewhere (e.g., a migration guide), consider adding a reference link here.
| - [#510](https://github.com/ForgeRock/ping-javascript-sdk/pull/510) [`3c63979`](https://github.com/ForgeRock/ping-javascript-sdk/commit/3c63979f83486e0914b61b6accfd5345e6eff152) Thanks [@ryanbas21](https://github.com/ryanbas21)! - Fix error handling in storage client and davinci-client | ||
| - Add `isGenericError` type guard to sdk-utilities for runtime error validation | ||
| - Fix storage client to properly catch errors from custom storage implementations, honoring the errors-as-values contract | ||
| - Improve davinci-client error handling to use explicit error checks instead of try-catch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for storage client calls in OIDC client
echo "=== Searching for storage client method calls in OIDC client ==="
rg -n 'getItem|setItem|removeItem' packages/sdk-clients/oidc-client/src/ --type=ts || echo "No direct storage calls found"
# Search for error handling patterns
echo -e "\n=== Searching for error handling around storage operations ==="
rg -n -B3 -A3 'getItem|setItem|removeItem' packages/sdk-clients/oidc-client/src/ --type=ts || echo "No pattern found"
# Search for isGenericError usage in OIDC client
echo -e "\n=== Searching for isGenericError usage in OIDC client ==="
rg -n 'isGenericError' packages/sdk-clients/oidc-client/src/ --type=ts || echo "isGenericError not found"
# Check if OIDC client imports from storage client
echo -e "\n=== Searching for storage client imports in OIDC client ==="
rg -n 'from.*storage' packages/sdk-clients/oidc-client/src/ --type=ts || echo "No storage imports found"Repository: ForgeRock/ping-javascript-sdk
Length of output: 747
🏁 Script executed:
#!/bin/bash
# Find OIDC client directory
echo "=== Looking for OIDC client ==="
find . -type d -name "*oidc*" 2>/dev/null | head -20
# List packages directory
echo -e "\n=== Contents of packages directory ==="
ls -la packages/ | head -30
# Search for files mentioning oidc
echo -e "\n=== Files mentioning OIDC ==="
find . -type f -name "*.ts" -o -name "*.json" | xargs grep -l "oidc-client" 2>/dev/null | head -10Repository: ForgeRock/ping-javascript-sdk
Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Explore OIDC client structure
echo "=== OIDC client source files ==="
find packages/oidc-client/src -type f -name "*.ts" 2>/dev/null | head -20
# Search for storage client calls
echo -e "\n=== Searching for storage client method calls ==="
rg -n 'getItem|setItem|removeItem' packages/oidc-client/src/ --type=ts -C3 2>/dev/null || echo "No matches"
# Search for error handling patterns
echo -e "\n=== Searching for isGenericError ==="
rg -n 'isGenericError' packages/oidc-client/src/ --type=ts 2>/dev/null || echo "Not found"
# Search for storage imports
echo -e "\n=== Searching for storage-related imports ==="
rg -n 'from.*storage|import.*storage' packages/oidc-client/src/ --type=ts 2>/dev/null || echo "Not found"
# Search for StorageClient usage
echo -e "\n=== Searching for StorageClient ==="
rg -n 'StorageClient|storage\.' packages/oidc-client/src/ --type=ts -C2 2>/dev/null | head -50Repository: ForgeRock/ping-javascript-sdk
Length of output: 7154
🏁 Script executed:
#!/bin/bash
# Examine client.store.ts for storage client usage
echo "=== client.store.ts: storage client usage ==="
rg -n 'storage\.' packages/oidc-client/src/lib/client.store.ts -C4 --type=ts | head -80
# Examine logout.request.ts for storage client usage
echo -e "\n=== logout.request.ts: storage client usage ==="
rg -n 'storageClient\.' packages/oidc-client/src/lib/logout.request.ts -C4 --type=ts
# Look for try-catch blocks around storage calls
echo -e "\n=== try-catch patterns in client.store.ts ==="
rg -n 'try|catch' packages/oidc-client/src/lib/client.store.ts --type=ts -C2 | head -60Repository: ForgeRock/ping-javascript-sdk
Length of output: 3196
🏁 Script executed:
#!/bin/bash
# Look for getState implementation which likely calls storage
echo "=== getState implementation ==="
rg -n 'getState' packages/oidc-client/src/lib/client.store.ts -A15 --type=ts | head -100
# Look for actual storage method calls in client.store
echo -e "\n=== Actual storage method calls in client.store ==="
rg -n '\bget\(|\bset\(|\bremove\(' packages/oidc-client/src/lib/client.store.ts --type=ts -B2 -A2
# Check the initialization and storageClient creation
echo -e "\n=== Storage client initialization ==="
rg -n 'createStorage|StorageClient' packages/oidc-client/src/lib/client.store.ts -B2 -A5 --type=tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 5795
🏁 Script executed:
#!/bin/bash
# Search for isGenericError in entire codebase
echo "=== isGenericError definition and usage ==="
rg -n 'isGenericError' . --type=ts | head -30
# Check sdk-utilities for isGenericError
echo -e "\n=== In sdk-utilities ==="
find packages/sdk-utilities -type f -name "*.ts" | xargs grep -l 'isGenericError' 2>/dev/null
# Check all error handling patterns in logout.request.ts
echo -e "\n=== logout.request.ts full error handling ==="
cat packages/oidc-client/src/lib/logout.request.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 6440
OIDC client appears to have been excluded from PR #510's error handling improvements.
The OIDC client uses storageClient.get(), storageClient.set(), and storageClient.remove() but was not updated to use the new isGenericError type guard. Error handling is inconsistent:
- Line 243-246 (revoke): Explicitly checks
if (tokens && 'error' in tokens) - Lines 338, 440, 517 (revoke, info, logout): Only check
if (!tokens || !('accessToken' in tokens))— relying on implicit error detection rather than explicit error validation
Consider whether OIDC client error handling should be updated to use isGenericError consistently, or clarify if it's covered by a separate task.
🤖 Prompt for AI Agents
In `@packages/sdk-effects/storage/CHANGELOG.md` around lines 11 - 14, The OIDC
client’s error handling is inconsistent: update the revoke, info, and logout
flows that call storageClient.get()/set()/remove() to use the sdk-utilities
isGenericError type guard instead of relying on implicit checks like '!tokens ||
!("accessToken" in tokens)' or ad-hoc '"error" in tokens' checks; import
isGenericError and replace those conditional branches in the revoke, info, and
logout handlers (and any other storageClient interaction) to explicitly check
isGenericError(result) and handle/log the error path consistently while
preserving the existing success-path logic.
This PR was opened by the Changesets release GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.
Releases
@forgerock/davinci-client@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
#510
3c63979Thanks @ryanbas21! - Fix error handling in storage client and davinci-clientisGenericErrortype guard to sdk-utilities for runtime error validation#504
7834cdeThanks @ancheetah! - Improve FIDO module error handling when no optionsUpdated dependencies [
3c63979,9ad4062]:@forgerock/device-client@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/journey-client@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
#500
e99b374Thanks @ancheetah! - Add support for KBAallowUserDefinedQuestionsflagUpdated dependencies [
3c63979,9ad4062]:@forgerock/oidc-client@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
3c63979,9ad4062]:@forgerock/protect@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/iframe-manager@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/sdk-logger@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/sdk-oidc@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
3c63979,9ad4062]:@forgerock/sdk-request-middleware@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/storage@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
#510
3c63979Thanks @ryanbas21! - Fix error handling in storage client and davinci-clientisGenericErrortype guard to sdk-utilities for runtime error validationUpdated dependencies [
9ad4062]:@forgerock/sdk-types@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdk@forgerock/sdk-utilities@2.0.0
Major Changes
9ad4062Thanks @ryanbas21! - releasing version 2 of the ping javascript sdkPatch Changes
#510
3c63979Thanks @ryanbas21! - Fix error handling in storage client and davinci-clientisGenericErrortype guard to sdk-utilities for runtime error validationUpdated dependencies [
9ad4062]:Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.