fix!: update-error-handling-across-packages #524
Conversation
🦋 Changeset detectedLatest commit: c2ead9b 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 |
📝 WalkthroughWalkthroughThe pull request introduces breaking changes across multiple packages with major version bumps to the Journey client error handling. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 c2ead9b
☁️ 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: 3
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/src/lib/client.store.ts (1)
156-176: Unsafe type assertion may cause runtime issues.On line 166,
createJourneyObject(stored.step) as JourneyStepis unsafe. Ifstored.steplacksauthId,createJourneyObjectreturnsJourneyLoginFailure(notJourneyStep), which would then be incorrectly typed and passed toself.next().Consider validating the result type before proceeding:
🐛 Proposed fix
} else if (isStoredStep(stored)) { - previousStep = createJourneyObject(stored.step) as JourneyStep; + const journeyObject = createJourneyObject(stored.step); + if ('type' in journeyObject && journeyObject.type === 'Step') { + previousStep = journeyObject as JourneyStep; + } else if (isGenericError(journeyObject)) { + throw new Error(`Error recreating step from storage: ${journeyObject.message || journeyObject.error}`); + } else { + throw new Error('Stored step is not a valid Step type for resume operation.'); + } }
🤖 Fix all issues with AI agents
In @.changeset/chilly-sheep-follow.md:
- Around line 25-40: The "After:" label and the following TypeScript example are
accidentally inside the fenced code block; update the changeset so the initial
code block ends before the "After:" label, place "After:" on its own line, then
open a new ```ts fenced block containing the result example (the const result =
await client.start() snippet and its if/else handling) and close that
fence—ensure the original "Before" code block (const step = await
client.start();) remains unchanged and that both fences are properly balanced
around the examples.
- Around line 1-6: Remove the '@forgerock/storage' entry from this changeset
header so it no longer lists a major bump; edit the top YAML list to include
only '@forgerock/sdk-oidc', '@forgerock/journey-client', and
'@forgerock/oidc-client' (leaving their versions as-is) because the storage
package has no changes in this PR and its API changes were already handled in
the separate changeset referenced by SDKS-4361/hip-owls-rule.md.
In `@packages/journey-client/src/lib/client.store.test.ts`:
- Around line 17-22: The test's type guard isGenericError currently checks for
'error' and 'type' but production's guard checks for 'error' and 'message';
update isGenericError in client.store.test.ts to check for 'error' in result &&
'message' in result to match the production check (or refactor by extracting a
shared isGenericError utility and import it into both the test and
client.store.ts so both use the identical implementation).
🧹 Nitpick comments (1)
packages/journey-client/src/lib/journey.utils.ts (1)
40-53: Thedefaultcase is unreachable given the current logic.The if-else chain (lines 32-38) guarantees
typeis always one ofStepType.Step,StepType.LoginSuccess, orStepType.LoginFailure. The switch exhaustively handles all three values, making the default case dead code.Consider using TypeScript's exhaustive check pattern to catch future enum additions at compile time:
♻️ Suggested refactor using exhaustive check
switch (type) { case StepType.LoginSuccess: return createJourneyLoginSuccess(step); case StepType.LoginFailure: return createJourneyLoginFailure(step); case StepType.Step: return createJourneyStep(step); default: - return { - error: 'unknown_step_type', - message: 'Unable to determine step type from server response', - type: 'unknown_error', - }; + // Exhaustive check - TypeScript will error if a case is missed + const _exhaustiveCheck: never = type; + return { + error: 'unknown_step_type', + message: `Unable to determine step type from server response: ${_exhaustiveCheck}`, + type: 'unknown_error', + }; }
| --- | ||
| '@forgerock/storage': major | ||
| '@forgerock/sdk-oidc': major | ||
| '@forgerock/journey-client': major | ||
| '@forgerock/oidc-client': major | ||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git diff --name-only | grep -E "(storage|changeset)" | head -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Check for storage package modifications across all package paths
fd --type f -e ts -e json . packages/ 2>/dev/null | grep -i storage | grep -v node_modules | head -30Repository: ForgeRock/ping-javascript-sdk
Length of output: 497
🏁 Script executed:
# Look at the changeset file context and any related storage changes
cat .changeset/chilly-sheep-follow.md 2>/dev/null || echo "File not found"Repository: ForgeRock/ping-javascript-sdk
Length of output: 1877
🏁 Script executed:
# Search for GenericError in storage packages to understand the API change
rg "GenericError" --type ts packages/ 2>/dev/null | grep -E "(storage|sdk-effects)" | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 1706
🏁 Script executed:
# Check if storage files were modified in this PR
git diff --name-only HEAD | grep -i storageRepository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
# Check the actual storage.effects.ts file to see the current API
cat -n packages/sdk-effects/storage/src/lib/storage.effects.ts | head -50Repository: ForgeRock/ping-javascript-sdk
Length of output: 1695
🏁 Script executed:
# Search for any recent changes or TODOs related to storage error handling
rg -n "TODO|FIXME|GenericError" packages/sdk-effects/storage/src/lib/storage.effects.ts -B2 -A2Repository: ForgeRock/ping-javascript-sdk
Length of output: 1229
🏁 Script executed:
# Check if there are multiple changeset entries for storage
find .changeset -type f -name "*.md" ! -name ".gitkeep" -exec grep -l "storage" {} \;Repository: ForgeRock/ping-javascript-sdk
Length of output: 134
🏁 Script executed:
cat .changeset/hip-owls-rule.mdRepository: ForgeRock/ping-javascript-sdk
Length of output: 522
🏁 Script executed:
# Also verify if storage package has any actual code changes
rg -n "export" packages/sdk-effects/storage/src/lib/storage.effects.ts | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 361
Remove @forgerock/storage from this changeset.
The changeset marks @forgerock/storage as a major version bump, but the storage package has no changes in this PR. The storage API's transition to return GenericError | null was already completed in SDKS-4361 and is documented in the separate hip-owls-rule.md changeset. The breaking changes described here apply only to journey-client methods, not storage.
🤖 Prompt for AI Agents
In @.changeset/chilly-sheep-follow.md around lines 1 - 6, Remove the
'@forgerock/storage' entry from this changeset header so it no longer lists a
major bump; edit the top YAML list to include only '@forgerock/sdk-oidc',
'@forgerock/journey-client', and '@forgerock/oidc-client' (leaving their
versions as-is) because the storage package has no changes in this PR and its
API changes were already handled in the separate changeset referenced by
SDKS-4361/hip-owls-rule.md.
| Before: | ||
|
|
||
| ```ts | ||
| const step = await client.start(); | ||
| if (step) { | ||
| // Use step | ||
| } | ||
|
|
||
| After: const result = await client.start(); | ||
| if ('error' in result) { | ||
| // Handle error | ||
| console.error(result.message); | ||
| } else { | ||
| // Use step | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Migration guide has a formatting issue.
The "After:" section on line 33 is incorrectly placed inside the code block. This will render incorrectly in the changeset documentation.
📝 Suggested fix
Before:
```ts
const step = await client.start();
if (step) {
// Use step
}
+```
-After: const result = await client.start();
+After:
+
+```ts
+const result = await client.start();
if ('error' in result) {
// Handle error
console.error(result.message);
} else {
// Use step
}
```📝 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.
| Before: | |
| ```ts | |
| const step = await client.start(); | |
| if (step) { | |
| // Use step | |
| } | |
| After: const result = await client.start(); | |
| if ('error' in result) { | |
| // Handle error | |
| console.error(result.message); | |
| } else { | |
| // Use step | |
| } | |
| ``` | |
| Before: | |
🤖 Prompt for AI Agents
In @.changeset/chilly-sheep-follow.md around lines 25 - 40, The "After:" label
and the following TypeScript example are accidentally inside the fenced code
block; update the changeset so the initial code block ends before the "After:"
label, place "After:" on its own line, then open a new ```ts fenced block
containing the result example (the const result = await client.start() snippet
and its if/else handling) and close that fence—ensure the original "Before" code
block (const step = await client.start();) remains unchanged and that both
fences are properly balanced around the examples.
| /** | ||
| * Type guard to check if a result is a GenericError | ||
| */ | ||
| function isGenericError(result: unknown): result is GenericError { | ||
| return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; | ||
| } |
There was a problem hiding this comment.
Inconsistent type guard implementations.
This type guard checks for 'error' in result && 'type' in result, while the production code in client.store.ts (lines 142-143) checks for 'error' in obj && 'message' in obj. Consider aligning both guards to check the same properties for consistency.
♻️ Suggested alignment
function isGenericError(result: unknown): result is GenericError {
- return typeof result === 'object' && result !== null && 'error' in result && 'type' in result;
+ return typeof result === 'object' && result !== null && 'error' in result && 'message' in result;
}Or consider extracting a shared isGenericError utility that both test and production code can import.
📝 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.
| /** | |
| * Type guard to check if a result is a GenericError | |
| */ | |
| function isGenericError(result: unknown): result is GenericError { | |
| return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; | |
| } | |
| /** | |
| * Type guard to check if a result is a GenericError | |
| */ | |
| function isGenericError(result: unknown): result is GenericError { | |
| return typeof result === 'object' && result !== null && 'error' in result && 'message' in result; | |
| } |
🤖 Prompt for AI Agents
In `@packages/journey-client/src/lib/client.store.test.ts` around lines 17 - 22,
The test's type guard isGenericError currently checks for 'error' and 'type' but
production's guard checks for 'error' and 'message'; update isGenericError in
client.store.test.ts to check for 'error' in result && 'message' in result to
match the production check (or refactor by extracting a shared isGenericError
utility and import it into both the test and client.store.ts so both use the
identical implementation).
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (38.46%) 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 #524 +/- ##
==========================================
+ Coverage 18.79% 18.83% +0.04%
==========================================
Files 140 142 +2
Lines 27640 27766 +126
Branches 980 990 +10
==========================================
+ Hits 5195 5230 +35
- Misses 22445 22536 +91
🚀 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 9b7519c to https://ForgeRock.github.io/ping-javascript-sdk/pr-524/9b7519c0c54f5073e9409b8e42f3c9c422b5f53c branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 83.0 KB (+0.6 KB) ➖ 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 |
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4715
Description
Implement error handling throughout the api surface rather than undefined
Summary by CodeRabbit
start,next,resume,terminate) now return standardized error objects instead of undefined when failures occur.✏️ Tip: You can customize this high-level summary in your review settings.