fix: update-storage-client-error-handling #510
Conversation
🦋 Changeset detectedLatest commit: 3c63979 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 |
📝 WalkthroughWalkthroughAdds an isGenericError utility in a new sdk-utilities package, wires it into davinci-client (dependency and TS refs), updates davinci-client to explicitly handle storage-read/write/remove errors using the type guard, and wraps custom storage get/set/remove calls to return structured storage errors on exceptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Storage
participant State
Client->>Storage: get(serverInfoKey)
alt Storage returns GenericError
Storage-->>Client: GenericError
Client->>Client: log error
Client-->>Client: return InternalErrorResponse
else Storage returns serverInfo
Storage-->>Client: serverInfo
Client->>State: dispatch resume(serverInfo)
State-->>Client: updated node/state
Client->>Storage: remove(serverInfoKey)
alt remove returns GenericError
Storage-->>Client: GenericError
Client->>Client: log warning
else remove success
Storage-->>Client: null
end
Client-->>Client: return current node
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/davinci-client/package.jsonpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/tsconfig.jsonpackages/davinci-client/tsconfig.lib.jsonpackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/error/error.utils.test.tspackages/sdk-utilities/src/lib/error/error.utils.tspackages/sdk-utilities/src/lib/error/index.tspackages/sdk-utilities/tsconfig.jsonpackages/sdk-utilities/tsconfig.lib.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
📚 Learning: 2025-09-23T20:50:26.537Z
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
Applied to files:
packages/davinci-client/src/lib/client.store.ts
🔇 Additional comments (14)
packages/sdk-utilities/package.json (1)
39-41: LGTM! Dependency correctly added.The addition of
@forgerock/sdk-typesas a workspace dependency is appropriate for accessing theGenericErrortype in the new type guard utility.packages/davinci-client/package.json (1)
30-30: LGTM! Dependency correctly added.The addition of
@forgerock/sdk-utilitiesenables the davinci-client to use the newisGenericErrortype guard for cleaner error handling. Based on learnings, this addresses the follow-up task for proper error handling after storage client API changes in SDKS-4361.packages/sdk-utilities/tsconfig.lib.json (1)
22-26: LGTM! TypeScript reference correctly configured.The project reference to
sdk-typesenables proper type resolution and composite builds, correctly mirroring the package dependency added inpackage.json.packages/sdk-utilities/src/index.ts (1)
10-10: LGTM! Public API export correctly added.The new error utilities export properly surfaces the
isGenericErrortype guard to consuming packages.Note: The actual implementation files (
error.utils.ts,error/index.ts) and their tests were not included in this review. Ensure these have been reviewed separately or include them for comprehensive feedback.packages/davinci-client/tsconfig.json (1)
16-18: LGTM! TypeScript reference correctly configured.The project reference to
sdk-utilitiesenables proper type resolution for the newisGenericErrorutility, correctly mirroring the package dependency.Note: The actual usage of
isGenericErrorin davinci-client source files was not included in this review. Ensure error handling call sites have been updated and reviewed separately.packages/sdk-utilities/src/lib/error/index.ts (1)
8-8: LGTM!Clean re-export pattern establishing the public API surface for the error utilities.
packages/davinci-client/tsconfig.lib.json (1)
37-39: LGTM!The TypeScript project reference correctly enables the davinci-client package to consume types and utilities from sdk-utilities.
packages/sdk-utilities/tsconfig.json (1)
6-8: LGTM!The project reference correctly establishes the dependency on sdk-types, enabling the import of the GenericError type.
packages/sdk-utilities/src/lib/error/error.utils.ts (1)
1-15: LGTM!Clean import and well-documented function signature.
packages/sdk-utilities/src/lib/error/error.utils.test.ts (2)
1-11: LGTM!Proper test setup with all necessary imports.
12-59: LGTM!The success test cases provide good coverage for valid GenericError objects with varying field completeness.
packages/davinci-client/src/lib/client.store.ts (3)
130-141: Proper error handling for storage set operation.The implementation correctly captures the storage result, checks for errors using the type guard, logs appropriately, and returns a structured error response. This addresses the known gap from the retrieved learning where storage errors weren't being handled.
Minor note: The explicit
return;on line 140 is optional in TypeScript when the return type includesvoid, but it does provide clarity in this async context.Based on learnings, this change properly implements the follow-up task to handle storage API errors.
209-247: Excellent structured error handling for the resume flow.The implementation properly handles multiple error scenarios:
- Null storage result (lines 211-221) with a helpful user-facing message
- GenericError from get operation (lines 223-234) with appropriate fallback
- GenericError from remove operation (lines 240-243) as a non-blocking warning
The sequential approach (check null → check GenericError → proceed with success path) is clear and handles all edge cases. Using
warnlevel for cleanup failures is appropriate since the resume operation itself succeeded.Based on learnings, this change properly implements the follow-up task to handle storage API errors at the call site.
12-12: Import ofisGenericErrortype guard is correct and well-utilized.The
isGenericErrortype guard is properly exported from@forgerock/sdk-utilitiesand is used appropriately throughout the file to check storage operation results for error handling (lines 131, 223, 241).
|
View your CI Pipeline Execution ↗ for commit 3c63979
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (28.94%) 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 #510 +/- ##
==========================================
+ Coverage 18.79% 18.82% +0.02%
==========================================
Files 140 142 +2
Lines 27640 27726 +86
Branches 980 991 +11
==========================================
+ Hits 5195 5219 +24
- Misses 22445 22507 +62
🚀 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 c241d68 to https://ForgeRock.github.io/ping-javascript-sdk/pr-510/c241d6823a62a3f31a9ac12dc22508302bd08a01 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/sdk-utilities - 8.5 KB (+1.0 KB, +13.4%) 📊 Minor Changes📉 @forgerock/journey-client - 82.4 KB (-0.0 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 |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks good other than the complete removal of the try-catch.
| davinciApi.endpoints.resume.initiate({ continueToken, serverInfo: storedServerInfo }), | ||
| ); | ||
| await serverInfo.remove(); | ||
| const storedServerInfo = await serverInfo.get(); |
There was a problem hiding this comment.
If we get rid of all try-catches, we have no protection from any uncaught runtime errors. This feel a bit risky to me. I know it's unlikely, but it's still non-zero.
There was a problem hiding this comment.
So - storage client shouldn't throw to begin with. Looking at it though, we can improve this on custom storage by wrapping it itself.
We can keep the try catch to be overly defensive though.
or do both. What do you think?
There was a problem hiding this comment.
My concern is that the try-catch was also wrapping the store.dispatch method as well, so the improvement of the storage implementation helps, but I'm not sure it's enough to remove the try-catches entirely. The amount of code that underlies the store.dispatch is enough to motivate a try-catch.
I think RTK avoids throwing, but I'm not entirely sure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
74-75: Consider logging or capturing the original error for debugging purposes.The catch blocks discard the original exception. While the current approach provides consistent user-facing errors, capturing the original error could aid debugging in production. This is optional and can be deferred.
♻️ Optional: Capture original error for debugging
- } catch { + } catch (e) { + // Optional: Log or include original error for debugging + // console.debug('Custom storage error:', e); return createStorageError(storeType, 'Retrieving'); }Apply similar pattern to
setandremovecatch blocks if needed.Also applies to: 113-114, 130-131
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fix-storage-error-handling.mdpackages/sdk-effects/storage/src/lib/storage.effects.tspackages/sdk-utilities/src/lib/error/error.utils.test.tspackages/sdk-utilities/src/lib/error/error.utils.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-storage-error-handling.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk-utilities/src/lib/error/error.utils.test.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
📚 Learning: 2025-09-23T20:50:26.537Z
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
Applied to files:
packages/sdk-effects/storage/src/lib/storage.effects.ts
🧬 Code graph analysis (1)
packages/sdk-utilities/src/lib/error/error.utils.ts (1)
packages/sdk-utilities/src/lib/error/index.ts (1)
isGenericError(8-8)
⏰ 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 (4)
packages/sdk-effects/storage/src/lib/storage.effects.ts (3)
71-76: LGTM! Defensive error handling for custom storage retrieval.This try/catch appropriately handles runtime exceptions from custom storage implementations and returns a structured
GenericError. The explicit type annotation ensures type safety. This addresses the follow-up task from SDKS-4361 where error handling was missing. Based on learnings, this is a needed fix.
110-115: LGTM! Consistent error handling for custom storage set operation.The try/catch correctly handles exceptions, and
result ?? nullproperly normalizes undefined returns to null, maintaining consistency with theStorageClient<Value>.setreturn type signature.
127-132: LGTM! Mirrors the set operation's error handling pattern.The implementation is consistent with the
setmethod and properly handles exceptions from custom storage implementations.packages/sdk-utilities/src/lib/error/error.utils.ts (1)
16-25: Type guard correctly validates all required GenericError properties.The implementation checks both required properties (
errorandtypeas strings). GenericError has no additional required properties—only optional ones (code,message,status) which correctly aren't validated by the guard. While the guard could be enhanced to validate thattypematches specific string literal values ('argument_error', 'auth_error', etc.), the current implementation is sufficient and follows pragmatic TypeScript type guard patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
78-80: Consider using theisGenericErrortype guard for consistency.The PR introduces an
isGenericErrortype guard in@forgerock/sdk-utilities. The current checktypeof value === 'object' && 'error' in valuecould be replaced with the type guard for:
- Consistency with other parts of the codebase (davinci-client)
- Centralized error type checking logic
- Better type narrowing
♻️ Suggested refactor
+import { isGenericError } from '@forgerock/sdk-utilities';- if (value === null || (typeof value === 'object' && 'error' in value)) { + if (value === null || isGenericError(value)) { return value; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/fix-storage-error-handling.mdpackages/davinci-client/src/lib/client.store.tspackages/sdk-effects/storage/src/lib/storage.effects.tspackages/sdk-utilities/src/lib/error/error.utils.test.tspackages/sdk-utilities/src/lib/error/error.utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/sdk-utilities/src/lib/error/error.utils.test.ts
- packages/davinci-client/src/lib/client.store.ts
- .changeset/fix-storage-error-handling.md
- packages/sdk-utilities/src/lib/error/error.utils.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
⏰ 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). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (3)
packages/sdk-effects/storage/src/lib/storage.effects.ts (3)
71-76: LGTM! Proper error handling for custom storage retrieval.This try/catch correctly wraps the custom storage get operation and returns a structured error on failure, aligning with the browser storage error handling pattern. Based on learnings, this addresses the follow-up task from SDKS-4361 for proper error handling implementation.
110-115: LGTM! Robust error handling for custom storage set operation.The try/catch correctly captures exceptions from the custom storage implementation and returns a structured error. The
result ?? nullfallback properly handles implementations that may returnundefinedinstead ofnull.
127-132: LGTM! Consistent error handling for custom storage removal.The implementation mirrors the set operation pattern and correctly handles exceptions from custom storage implementations.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4446
Description
update to use a typeguard from sdk-utilities to help clean up checks for genericerror. then implement in davinci-client where we need to check for the generic error
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.