SDKS-4361: Improve storage client type #417
Conversation
🦋 Changeset detectedLatest commit: 93595d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
WalkthroughStandardizes storage API returns to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant OIDC as "OIDC Client"
participant Storage as "StorageClient"
participant AS as "Auth Server"
rect rgb(245,250,255)
note right of OIDC: logout/revoke flow now preserves inner responses (session/revoke/delete)
App->>OIDC: logoutµ(config, storageClient)
OIDC->>Storage: get() -- retrieve tokens
alt tokens present
OIDC->>AS: revoke(token)
AS-->>OIDC: revokeResponse (ok / GenericError)
OIDC->>Storage: remove()
Storage-->>OIDC: deleteResponse (null / GenericError)
OIDC-->>App: Success { sessionResponse, revokeResponse, deleteResponse }
else no tokens
OIDC-->>App: Success { sessionResponse: null, revokeResponse: null, deleteResponse: null }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
View your CI Pipeline Execution ↗ for commit 93595d2
☁️ Nx Cloud last updated this comment at |
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
from
286fc00 to
a3f1cf9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #417 +/- ##
==========================================
+ Coverage 55.63% 56.19% +0.56%
==========================================
Files 32 32
Lines 2051 2091 +40
Branches 344 353 +9
==========================================
+ Hits 1141 1175 +34
- Misses 910 916 +6 🚀 New features to boost your workflow:
|
|
Deployed 77507d6 to https://ForgeRock.github.io/ping-javascript-sdk/pr-417/77507d6d9e98357e14e47f96a546dd5aa914aa02 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 9.2 KB (new) 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
| if (value === null) { | ||
| return value; | ||
| } | ||
| try { |
There was a problem hiding this comment.
Can you elaborate as to why you've nested an additional try-catch here?
There was a problem hiding this comment.
As a follow up, it seems you're wrapping the storage access in a try-catch. This includes the native methods as well. If we're going to do this in the other methods, let's do it consistently here too. Also, can we unnest these try-catches and do them more sequentially?
There was a problem hiding this comment.
I had options and I went back and forth and gave up and settled on nested 😅 . I can un-nest them.
Yeah I can wrap the storage access in a try catch in the get method. Seems we were already doing this for set and remove but forgot about get
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
2 times, most recently
from
857304d to
24fdd8d
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.changeset/slow-teeth-melt.md (1)
8-8: Stray character at EOF.There’s an orphaned “8” on the last line. Remove it.
-8 +packages/oidc-client/src/lib/client.store.ts (5)
375-391: Fix type mismatch: success result fields must be null per typesRevokeSuccessResult in client.types.ts requires revokeResponse/deleteResponse to be null on success, but this block passes union-typed vars. Assign explicit nulls.
Apply this diff:
- const result: RevokeSuccessResult = { - revokeResponse, - deleteResponse, - }; + const result: RevokeSuccessResult = { + revokeResponse: null, + deleteResponse: null, + };
201-204: Propagate storage.set errors (now returns GenericError | null)set() result is ignored. If storage write fails, callers will think exchange succeeded. Handle and fail the effect when set() returns an error.
Minimal refactor:
- }).pipe( - Micro.tap(async (tokens) => { - await storageClient.set(tokens); - }), - ); + }).pipe( + Micro.flatMap((tokens) => + Micro.promise(() => storageClient.set(tokens)).pipe( + Micro.flatMap((res) => + res && 'error' in res + ? Micro.fail({ + error: 'Token storage failure', + message: res.message, + type: 'state_error', + status: res.status ?? 'unknown', + } as const) + : Micro.succeed(tokens), + ), + ), + ), + );
290-300: Propagate storage.remove/set errors during background renewalBoth remove() and set() return GenericError | null but errors are ignored. This can leave stale or missing tokens after “successful” renewals.
Example approach:
- Micro.tap(async (tokens) => { - await store.dispatch( + Micro.flatMap(async (tokens) => { + await store.dispatch( oidcApi.endpoints.revoke.initiate({ accessToken: tokens.accessToken, clientId: config.clientId, endpoint: wellknown.revocation_endpoint, }), ); - await storageClient.remove(); - await storageClient.set(tokens); + const del = await storageClient.remove(); + if (del && 'error' in del) { + return Micro.fail({ + error: 'Token storage removal failure', + message: del.message, + type: 'state_error', + status: del.status ?? 'unknown', + } as const); + } + const set = await storageClient.set(tokens); + if (set && 'error' in set) { + return Micro.fail({ + error: 'Token storage write failure', + message: set.message, + type: 'state_error', + status: set.status ?? 'unknown', + } as const); + } + return Micro.succeed(tokens); }),
334-341: Handle GenericError from storage.get before field checksget() may return GenericError. The current check treats that as “No access token found”, masking the real error.
Apply this diff:
- const tokens = await storageClient.get(); - - if (!tokens || !('accessToken' in tokens)) { + const tokens = await storageClient.get(); + if (tokens && 'error' in tokens) { + return { + error: 'Error occurred while retrieving tokens', + message: tokens.message, + status: tokens.status ?? 'unknown', + type: 'state_error', + }; + } + if (!tokens || !('accessToken' in tokens)) { return { error: 'No access token found', type: 'state_error', }; }
436-443: Handle GenericError from storage.get in user.infoSame masking issue as revoke(). Return the actual storage error instead.
Apply this diff:
- const tokens = await storageClient.get(); - - if (!tokens || !('accessToken' in tokens)) { + const tokens = await storageClient.get(); + if (tokens && 'error' in tokens) { + return { + error: 'Error occurred while retrieving tokens', + message: tokens.message, + status: tokens.status ?? 'unknown', + type: 'state_error', + }; + } + if (!tokens || !('accessToken' in tokens)) { return { error: 'No access token found', type: 'auth_error', }; }
🧹 Nitpick comments (7)
.changeset/slow-teeth-melt.md (1)
6-7: Tighten up release notes for clarity.Spell out the exact public change and add punctuation. Suggested tweak:
-- Standardizes return types on storage client and updates tests -- Improves OIDC client where storage client methods are used +- Standardizes StorageClient.set/remove return types to `GenericError | null`; updates tests. +- Improves OIDC client usage where storage client methods are called.packages/oidc-client/src/lib/client.store.ts (1)
279-289: Guard token_endpoint presence before exchange in background renewYou validated authorization_endpoint, but buildTokenExchangeµ requires token_endpoint. Add a check for clearer errors.
Example:
const attemptAuthorizeGetTokensµ = authorizeµ( wellknown, config, log, store, authorizeOptions, ).pipe( + Micro.tap(() => { + if (!wellknown.token_endpoint) { + return Micro.fail({ + error: 'Wellknown missing token endpoint', + type: 'wellknown_error', + } as const); + } + return Micro.unit; + }),packages/sdk-effects/storage/src/lib/storage.effects.ts (3)
41-47: Avoid implicit any: annotate value as string | nullPrevents implicit-any and clarifies intent.
Apply:
- let value; + let value: string | null; @@ - let value; + let value: string | null; @@ - let value; + let value: string | null;Also applies to: 67-73, 93-100
124-124: Return null directly in async functions (avoid Promise.resolve)Minor cleanup; async will wrap automatically.
- return Promise.resolve(null); + return null; @@ - return Promise.resolve(null); + return null; @@ - return Promise.resolve(null); + return null; @@ - return Promise.resolve(null); + return null; @@ - return Promise.resolve(null); + return null; @@ - return Promise.resolve(null); + return null;Also applies to: 136-136, 147-147, 160-160, 172-172, 183-183
30-31: Make return type explicit and drop the assertionPrefer a typed signature over a trailing cast.
-export function createStorage<Value>(config: StorageConfig) { +export function createStorage<Value>(config: StorageConfig): StorageClient<Value> { @@ - } as StorageClient<Value>; + };Also applies to: 192-193
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (2)
178-178: Await the set before get to avoid potential flakinessEven though the mock updates synchronously, awaiting is safer and clearer.
- storageInstance.set(testObject); + await storageInstance.set(testObject);
11-41: Optional: align storage mocks closer to Web Storage APIReal Storage methods are synchronous and accept/return strings. Simplifying mocks (sync getItem/setItem and no extra JSON.stringify/parse) would reduce test indirection. Not required for this PR.
Also applies to: 45-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/slow-teeth-melt.md(1 hunks)packages/oidc-client/src/lib/client.store.ts(2 hunks)packages/oidc-client/src/lib/client.types.ts(1 hunks)packages/oidc-client/src/lib/logout.request.ts(4 hunks)packages/sdk-effects/storage/src/lib/storage.effects.test.ts(12 hunks)packages/sdk-effects/storage/src/lib/storage.effects.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (1)
packages/sdk-types/src/lib/tokens.types.ts (1)
CustomStorageObject(18-22)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
packages/oidc-client/src/lib/client.types.ts (2)
packages/oidc-client/src/types.ts (1)
GenericError(6-6)packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
⏰ 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)
.changeset/slow-teeth-melt.md (1)
1-4: Double-check version-bump level — bump to MAJOR if StorageClient return types changed.Both packages exist in the repo: packages/sdk-effects/storage/package.json (@forgerock/storage) and packages/oidc-client/package.json (@forgerock/oidc-client). If StorageClient.set/remove now return GenericError | null (a public return-type change), this is breaking — change the entries in .changeset/slow-teeth-melt.md from minor → major.
packages/oidc-client/src/lib/logout.request.ts (3)
10-13: Good: type-only imports reduce runtime footprintUsing type-only imports for OauthTokens/OidcConfig/WellKnownResponse/StorageClient/Logout* types is correct and tree-shake friendly.
26-27: Good: storageClient typed as StorageClientSignature now reflects the concrete client API and aligns with storage package changes.
53-58: Inner-error detection is correct and aligned with StorageClient return typeChecking for 'error' on session/revoke/delete responses matches the new GenericError | null semantics.
packages/oidc-client/src/lib/client.types.ts (4)
11-14: Success result fields set to null is clear and unambiguousThis contract is fine, but ensure implementations assign null on success (see revoke/logout code paths).
16-20: RevokeErrorResult shape is sensibleCarrying both inner responses (nullable) on error is useful for diagnostics.
22-24: LogoutSuccessResult extends RevokeSuccessResult with null sessionResponseConsistent with “null on success” convention.
26-31: LogoutErrorResult mirrors inner responses for troubleshootingLooks good. Confirm all callers/tests expect this standalone error shape (not intersections).
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
41-64: Thanks for un-nesting the try/catches around retrieval and parseAddresses the earlier feedback and improves readability without changing behavior.
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (1)
146-154: Tests correctly assert null on success and structured errors on failureCovers new return semantics and error shapes across backends.
Also applies to: 166-174, 224-231, 235-244, 246-253, 297-303, 327-335
| sessionResponse, | ||
| revokeResponse, | ||
| deleteResponse, | ||
| }; |
There was a problem hiding this comment.
Fix type mismatch: success result fields must be null per types
LogoutSuccessResult requires sessionResponse/revokeResponse/deleteResponse to be null on success. Assign explicit nulls.
Apply this diff:
- const result: LogoutSuccessResult = {
- sessionResponse,
- revokeResponse,
- deleteResponse,
- };
+ const result: LogoutSuccessResult = {
+ sessionResponse: null,
+ revokeResponse: null,
+ deleteResponse: null,
+ };📝 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.
| sessionResponse, | |
| revokeResponse, | |
| deleteResponse, | |
| }; | |
| const result: LogoutSuccessResult = { | |
| sessionResponse: null, | |
| revokeResponse: null, | |
| deleteResponse: null, | |
| }; |
🤖 Prompt for AI Agents
In packages/oidc-client/src/lib/logout.request.ts around lines 69 to 72, the
success result currently populates sessionResponse, revokeResponse, and
deleteResponse with actual responses, but the LogoutSuccessResult type requires
these three fields to be null on success; update the success branch to set
sessionResponse = null, revokeResponse = null, and deleteResponse = null
(instead of returning the response objects) so the returned object matches the
LogoutSuccessResult shape.
There was a problem hiding this comment.
They should all be null at this point because of the type narrowing above but I can change it back to explicit nulls if anyone has an issue with this.
There was a problem hiding this comment.
@ancheetah I understand your reasoning about the type narrowing, but I think there might be a subtle distinction here.
The type narrowing above eliminates responses that have an error property, but it doesn't necessarily make the values null. The sessionResponse, revokeResponse, and deleteResponse could still be successful response objects (without error properties) rather than null values.
Looking at the LogoutSuccessResult type definition, it explicitly requires these fields to be null on success:
export type LogoutSuccessResult = {
sessionResponse: null;
revokeResponse: null;
deleteResponse: null;
};So while the type narrowing ensures these aren't error responses, they might still be successful response objects. For strict type compliance and clarity, I'd recommend the explicit null assignments to match the type definition exactly.
That said, if you're confident about the runtime behavior and the current approach works, I defer to your judgment on the codebase!
There was a problem hiding this comment.
Not a blocker for me, but it may be better to have the hard coded null values. Since type checking is not existent in the run time, but we do expect to only have null values at this point.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I changed it back to null :)
| set: (value: Value) => Promise<GenericError | null>; | ||
| remove: () => Promise<GenericError | null>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Standardized return types for set/remove — good; audit call sites for handling GenericError | null
Change aligns API. Ensure all callers check the returned value and handle errors instead of assuming void.
Run to list likely call sites to review:
🏁 Script executed:
#!/bin/bash
# Find set/remove usages that should now handle GenericError|null
rg -nP --type=ts -C2 '\bawait\s+[\w\.\[\]$]+\.\s*(set|remove)\s*\('Length of output: 10817
Handle GenericError | null returned from storage.set/remove at callers
Several call sites await set/remove but ignore the returned GenericError | null — update them to check the result and handle errors.
- packages/davinci-client/src/lib/client.store.ts — await serverInfo.set(serverSlice) (line 126); await serverInfo.remove() (line 200)
- packages/oidc-client/src/lib/client.store.ts — await storageClient.set(tokens) (line 202); await storageClient.remove() and await storageClient.set(tokens) (lines 298–299)
🤖 Prompt for AI Agents
In packages/sdk-effects/storage/src/lib/storage.effects.ts around lines 11-12
the storage API methods return Promise<GenericError | null> but several callers
in other files ignore this; update the listed call sites to await and check the
result, e.g. assign the await to a variable, if the result is non-null treat it
as an error and handle it consistently (log via existing logger and either throw
or return an appropriate failure value), specifically: in
packages/davinci-client/src/lib/client.store.ts check the result of await
serverInfo.set(serverSlice) at line 126 and of await serverInfo.remove() at line
200; in packages/oidc-client/src/lib/client.store.ts check the result of await
storageClient.set(tokens) at line 202 and both await storageClient.remove() and
await storageClient.set(tokens) at lines 298–299; ensure you do not swallow
errors and follow the module's existing error-handling pattern (log + throw or
return error) so callers receive/observe failures.
There was a problem hiding this comment.
There are a few places in the OIDC client and DaVinci client where we call storage client methods and do not handle the error. The storage client does not throw, but instead returns a GenericError that must be checked for. I have not touched these as it may be out of the scope of the ticket. Here's where we can improve. Let me know if these should be addressed at this time.
- In the davinci client, the
serverInfostore uses local storage which may throw if the browser's storage quota is exceeded or the browser is in incognito mode with blocked cookies or disabled local storage.
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/davinci-client/src/lib/client.store.ts#L69-L72 - In the oidc client, there are several places where we wrap a storage client method in a Micro which will never fail due to storage client never throwing. Similarly, in the snippet below there is a set of calls which will never throw and the error is not handled.
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/oidc-client/src/lib/client.store.ts#L290-L300
There was a problem hiding this comment.
🧩 Analysis chain
Let me examine the specific locations you've identified to better understand the scope of the changes needed.
@ancheetah, you've identified a valid architectural concern. The API change from void to GenericError | null creates a breaking change that requires all call sites to handle potential errors.
Based on the scope considerations:
Recommendation: Create a follow-up issue
This should be addressed in a separate PR because:
-
Scope separation: The current ticket (SDKS-4361) focuses on standardizing storage client return types. Adding error handling across multiple clients expands the scope significantly.
-
Impact assessment: Proper error handling will require:
- Reviewing error handling patterns in each client
- Deciding on consistent error propagation strategies
- Potentially updating client APIs if errors need to bubble up
- Comprehensive testing of error scenarios
-
Risk management: Bundling these changes could make the current PR harder to review and increase the risk of introducing bugs.
Would you like me to create a GitHub issue to track this follow-up work? I can include the specific locations you've identified and provide context about the error handling improvements needed.
(\ /)
( ._. )
o_("_")("_")
thinking deeply about architectural decisions
🏁 Script executed:
#!/bin/bash
# Check the DaVinci client store usage
echo "=== DaVinci client store usage ==="
cat packages/davinci-client/src/lib/client.store.ts | sed -n '65,75p'
echo -e "\n=== OIDC client store usage ==="
cat packages/oidc-client/src/lib/client.store.ts | sed -n '285,305p'
# Find all storage client method calls that might need error handling
echo -e "\n=== All storage client set/remove calls ==="
rg -nP --type=ts -A2 -B2 'await\s+.*\.(set|remove)\s*\(' packages/davinci-client packages/oidc-client
Length of output: 2922
Perfect! Now I can see the exact locations and contexts. This reinforces my recommendation for a separate issue.
Analysis of the call sites:
-
OIDC Client (lines 202, 298-299): These are inside
Micro.tapblocks, which are designed for side effects. Storage failures here shouldn't necessarily crash the authentication flow, but they should be logged and potentially expose the errors to callers for observability. -
DaVinci Client (lines 126, 200):
- Line 126: Storing continuation state - failure here could impact the user's ability to resume flows
- Line 200: Cleanup after resume - failure here is less critical but should be logged
The error handling strategy needs to be thoughtful:
- Should storage errors fail the entire operation or just log warnings?
- How should errors be surfaced to application developers?
- What's the fallback behavior when storage is unavailable?
This definitely warrants a dedicated issue and PR to:
- Define consistent error handling patterns for each client
- Implement proper error checking at all call sites
- Add comprehensive tests for storage failure scenarios
- Update documentation about storage error handling
Would you like me to create a GitHub issue to track this work? I can include:
- The specific locations identified
- Context about the API change that created this need
- Recommendations for error handling strategies
- Links back to this PR and discussion
This approach keeps your current PR focused and ensures the error handling work gets the attention it deserves.
✏ Learnings added
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
from
24fdd8d to
a9eff34
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
The test failures are occurring because the test environment lacks DOM APIs (localStorage and sessionStorage) that are required by the storage effects module. The error messages consistently show "ReferenceError: localStorage is not defined" and "ReferenceError: sessionStorage is not defined" at line 28 of the storage effects file.
This failure is classified as 'environment_state' because:
- The errors are not related to the actual code changes in the pull request, which focused on standardizing storage client return types and improving OIDC client error handling
- The failures are caused by missing browser APIs in the Node.js test environment, not by the logic changes made in the PR
- This is a test environment configuration issue where the storage effects module expects browser globals that aren't available in the test runner
- The same storage code was working before, indicating this is an environmental setup problem rather than a code problem
The solution would require configuring the test environment to provide mocks for localStorage and sessionStorage, which is a testing infrastructure concern rather than a code fix related to the PR changes.
A code change would likely not resolve this issue, so no action was taken.
⚙️ An Nx Cloud workspace admin can disable these reviews in workspace settings.
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
from
a9eff34 to
ea41302
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/sdk-types/src/lib/tokens.types.ts (1)
8-8: Use a type-only import for GenericErrorTo avoid emitting a runtime import for a type, switch to a type-only import.
-import { GenericError } from './error.types.js'; +import type { GenericError } from './error.types.js';packages/oidc-client/src/lib/logout.request.ts (1)
53-58: Prefer a type guard for clearer narrowingReplace repeated
'error' in xchecks with a reusable type guard to improve readability and inference.Example (add once near the top of the file):
function isGenericError(x: unknown): x is { error: unknown } { return !!x && typeof x === 'object' && 'error' in x; }Then:
- const isInnerRequestError = - (sessionResponse && 'error' in sessionResponse) || - (revokeResponse && 'error' in revokeResponse) || - (deleteResponse && 'error' in deleteResponse); + const isInnerRequestError = + isGenericError(sessionResponse) || + isGenericError(revokeResponse) || + isGenericError(deleteResponse);packages/sdk-effects/storage/src/lib/storage.effects.test.ts (2)
188-198: Await the set before get to avoid race potentialMinor stability improvement: ensure the write completes before read.
- storageInstance.set(testObject); + await storageInstance.set(testObject);
80-101: Align mock method signatures with CustomStorageObjectFor stricter type conformance and clarity, mirror the interface parameter/return types.
- get: vi.fn(async (key: string): Promise<string | GenericError> => { + get: vi.fn(async (key: string): Promise<string | null | GenericError> => { @@ - set: vi.fn(async (key: string, valueToSet: unknown): Promise<void | GenericError> => { + set: vi.fn(async (key: string, valueToSet: string): Promise<void | GenericError> => {packages/sdk-effects/storage/src/lib/storage.effects.ts (4)
70-82: Catch exceptions from custom get()If a custom store throws, return a standardized retrieving error (consistent with browser storage path).
- if (storeType === 'custom') { - const value = await config.custom.get(key); + if (storeType === 'custom') { + let value: string | GenericError | null; + try { + value = await config.custom.get(key); + } catch { + return createStorageError(storeType, 'Retrieving'); + } if (value === null || (typeof value === 'object' && 'error' in value)) { return value; } try { const parsed = JSON.parse(value); return parsed as Value; } catch { return createStorageError(storeType, 'Parsing'); } }
101-114: Avoid shadowing and redundant Promise.resolve in async functionUse a distinct variable name and return the value directly.
set: async function storageSet(value: Value): Promise<GenericError | null> { const valueToStore = JSON.stringify(value); if (storeType === 'custom') { - const value = await config.custom.set(key, valueToStore); - return Promise.resolve(value ?? null); + try { + const err = await config.custom.set(key, valueToStore); + return err ?? null; + } catch { + return createStorageError(storeType, 'Storing'); + } } try { await storageTypes[storeType].setItem(key, valueToStore); - return Promise.resolve(null); + return null; } catch { return createStorageError(storeType, 'Storing'); } },
115-127: Symmetric error handling for custom remove(); remove redundant Promise.resolveMirror the set() improvements for consistency.
remove: async function storageRemove(): Promise<GenericError | null> { if (storeType === 'custom') { - const value = await config.custom.remove(key); - return Promise.resolve(value ?? null); + try { + const err = await config.custom.remove(key); + return err ?? null; + } catch { + return createStorageError(storeType, 'Removing'); + } } try { await storageTypes[storeType].removeItem(key); - return Promise.resolve(null); + return null; } catch { return createStorageError(storeType, 'Removing'); } },
11-12: Audit unhandled set/remove calls for error handling
- packages/davinci-client/src/lib/client.store.ts: line 126 (
await serverInfo.set…), line 200 (await serverInfo.remove())- packages/oidc-client/src/lib/client.store.ts: line 202 (
await storageClient.set…), line 298 (await storageClient.remove())Capture each call’s
GenericError | nullreturn and handle or log errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/slow-teeth-melt.md(1 hunks)packages/oidc-client/src/lib/client.store.test.ts(1 hunks)packages/oidc-client/src/lib/client.store.ts(2 hunks)packages/oidc-client/src/lib/client.types.ts(1 hunks)packages/oidc-client/src/lib/logout.request.test.ts(1 hunks)packages/oidc-client/src/lib/logout.request.ts(4 hunks)packages/sdk-effects/storage/src/lib/storage.effects.test.ts(13 hunks)packages/sdk-effects/storage/src/lib/storage.effects.ts(2 hunks)packages/sdk-types/src/lib/tokens.types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/oidc-client/src/lib/client.store.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/oidc-client/src/lib/client.store.ts
- .changeset/slow-teeth-melt.md
- packages/oidc-client/src/lib/client.types.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/**/?(*.)test.@(ts|tsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Write unit tests with Vitest for package code
Files:
packages/oidc-client/src/lib/logout.request.test.tspackages/sdk-effects/storage/src/lib/storage.effects.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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.496Z
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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 (3)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (1)
packages/sdk-types/src/lib/tokens.types.ts (1)
CustomStorageObject(20-24)
packages/oidc-client/src/lib/logout.request.ts (2)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
StorageClient(9-13)packages/oidc-client/src/lib/config.types.ts (1)
OauthTokens(29-35)
🔇 Additional comments (4)
packages/sdk-types/src/lib/tokens.types.ts (1)
21-23: API shape looks good; consistent with StorageClientThe widened return types align with the new StorageClient contract and tests.
Please confirm any custom storage implementations in the repo/workspaces have been updated to match the new
CustomStorageObjectsignature (especiallyset/removereturningPromise<void | GenericError>). If needed, I can generate a script to find them.packages/oidc-client/src/lib/logout.request.ts (3)
10-13: Good: convert to type-only importsReduces emitted code and keeps typings clean.
26-26: Good: parameterized StorageClient typeSwitching to
StorageClient<OauthTokens>matches the public storage API.
68-72: Fix type mismatch: success result fields must be null per types
The success object should setsessionResponse,revokeResponse, anddeleteResponseto null explicitly to satisfyLogoutSuccessResult.- const result: LogoutSuccessResult = { - sessionResponse, - revokeResponse, - deleteResponse, - }; + const result: LogoutSuccessResult = { + sessionResponse: null, + revokeResponse: null, + deleteResponse: null, + };
| Object.defineProperty(global, 'sessionStorage', { value: null }); | ||
| Object.defineProperty(global, 'localStorage', { value: null }); |
There was a problem hiding this comment.
Avoid redefining localStorage/sessionStorage with non-configurable stubs
storage.effects.test.ts already installs both globals via Object.defineProperty without configurable/writable, so this redefinition throws TypeError: Cannot redefine property: localStorage as soon as the suite loads. Please gate the override and only redefine when the descriptor is reconfigurable (or relax the original stub first) so the tests can execute.
-Object.defineProperty(global, 'sessionStorage', { value: null });
-Object.defineProperty(global, 'localStorage', { value: null });
+const disableBrowserStorage = (key: 'localStorage' | 'sessionStorage') => {
+ const descriptor = Object.getOwnPropertyDescriptor(globalThis, key);
+
+ if (descriptor && descriptor.configurable === false && descriptor.writable === false) {
+ return;
+ }
+
+ Object.defineProperty(globalThis, key, {
+ value: null,
+ configurable: true,
+ writable: true,
+ });
+};
+
+disableBrowserStorage('sessionStorage');
+disableBrowserStorage('localStorage');📝 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.
| Object.defineProperty(global, 'sessionStorage', { value: null }); | |
| Object.defineProperty(global, 'localStorage', { value: null }); | |
| const disableBrowserStorage = (key: 'localStorage' | 'sessionStorage') => { | |
| const descriptor = Object.getOwnPropertyDescriptor(globalThis, key); | |
| // If the property is already non-configurable and non-writable, skip redefinition | |
| if (descriptor && descriptor.configurable === false && descriptor.writable === false) { | |
| return; | |
| } | |
| Object.defineProperty(globalThis, key, { | |
| value: null, | |
| configurable: true, | |
| writable: true, | |
| }); | |
| }; | |
| disableBrowserStorage('sessionStorage'); | |
| disableBrowserStorage('localStorage'); |
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
from
ea41302 to
bed870c
Compare
|
Updates:
|
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 (1)
packages/oidc-client/src/lib/client.store.test.ts (1)
76-84: Update customStorage to match the new CustomStorageObject signature.The
customStorageobject'ssetandremovemethods returnPromise<void>, but the updatedCustomStorageObjectinterface (perpackages/sdk-types/src/lib/tokens.types.ts) now requiresPromise<void | GenericError>. Additionally, no tests verify error handling when custom storage operations fail.Update the custom storage implementation and add error-path tests:
const customStorage = { get: async (key: string): Promise<string | null> => storeObj[key] || null, - set: async (key: string, valueToSet: string) => { + set: async (key: string, valueToSet: string): Promise<void | GenericError> => { storeObj[key] = valueToSet; }, - remove: async (key: string) => { + remove: async (key: string): Promise<void | GenericError> => { delete storeObj[key]; }, };Add a test suite that exercises error returns from
set/remove, for example:it('Handles storage set error', async () => { const failingStorage = { get: async () => null, set: async () => ({ error: 'storage unavailable' }), remove: async () => null, }; const oidcClient = await oidc({ config, storage: { type: 'custom', name: 'oidcTokens', custom: failingStorage }, }); // Assert that set errors are handled appropriately });Based on learnings (SDKS-4361: call sites need proper error handling).
🧹 Nitpick comments (2)
packages/sdk-effects/storage/src/lib/storage.effects.ts (2)
30-54: Consider handling the default case explicitly.The error factory standardizes error shapes nicely across storage backends. However, the
defaultcase (line 45) doesn't setstorageName, which will result inundefinedin the error message. Since TypeScript's type system should prevent unknown storage types from reaching this function, consider either:
- Removing the default case (let TypeScript enforce exhaustiveness), or
- Explicitly setting
storageName = 'unknown'and throwing an errorApply this diff to make the switch exhaustive:
- let storageName; + let storageName: string; switch (storeType) { case 'localStorage': storageName = 'local'; break; case 'sessionStorage': storageName = 'session'; break; case 'custom': storageName = 'custom'; break; - default: - break; }
84-99: Consider unnesting the try/catch blocks as per previous review feedback.The nested try/catch blocks (lines 85-92 for retrieval, lines 94-99 for parsing) are functionally correct but could be more readable if unnested and sequential. This aligns with the previous review comment requesting consistent and unnested error handling.
Apply this diff to unnest the blocks:
- let value: string | null; - try { - value = await storageTypes[storeType].getItem(key); - if (value === null) { - return value; - } - } catch { - return createStorageError(storeType, 'Retrieving'); - } - - try { - const parsed = JSON.parse(value); - return parsed as Value; - } catch { - return createStorageError(storeType, 'Parsing'); - } + let value: string | null; + try { + value = await storageTypes[storeType].getItem(key); + } catch { + return createStorageError(storeType, 'Retrieving'); + } + + if (value === null) { + return value; + } + + try { + const parsed = JSON.parse(value); + return parsed as Value; + } catch { + return createStorageError(storeType, 'Parsing'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/slow-teeth-melt.md(1 hunks)packages/oidc-client/src/lib/client.store.test.ts(1 hunks)packages/oidc-client/src/lib/client.store.ts(2 hunks)packages/oidc-client/src/lib/client.types.ts(1 hunks)packages/oidc-client/src/lib/logout.request.test.ts(1 hunks)packages/oidc-client/src/lib/logout.request.ts(4 hunks)packages/sdk-effects/storage/src/lib/storage.effects.test.ts(15 hunks)packages/sdk-effects/storage/src/lib/storage.effects.ts(2 hunks)packages/sdk-types/src/lib/tokens.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/oidc-client/src/lib/logout.request.test.ts
- .changeset/slow-teeth-melt.md
- packages/oidc-client/src/lib/client.store.ts
🧰 Additional context used
📓 Path-based instructions (1)
packages/**/?(*.)test.@(ts|tsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Write unit tests with Vitest for package code
Files:
packages/oidc-client/src/lib/client.store.test.tspackages/sdk-effects/storage/src/lib/storage.effects.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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.496Z
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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 (4)
packages/oidc-client/src/lib/client.types.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
packages/oidc-client/src/lib/logout.request.ts (2)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
StorageClient(9-13)packages/oidc-client/src/lib/config.types.ts (1)
OauthTokens(29-35)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (1)
packages/sdk-types/src/lib/tokens.types.ts (1)
CustomStorageObject(20-24)
⏰ 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 (19)
packages/oidc-client/src/lib/client.store.test.ts (1)
16-17: LGTM! Valid test environment setup.Setting
global.localStoragetonullcorrectly simulates the absence of the localStorage API, ensuring that the stubbedsessionStorageis used during tests.packages/sdk-types/src/lib/tokens.types.ts (2)
8-9: LGTM! Import supports new error return types.The
GenericErrorimport is required for the updatedCustomStorageObjectsignature.
21-23: LGTM! Type signatures align with standardized error handling.The updated
CustomStorageObjectmethods now correctly returnGenericErrorunions, aligning with the PR objective to standardize storage client return types. This is an additive change—existing implementations returningvoidremain compatible.packages/oidc-client/src/lib/logout.request.ts (4)
10-13: LGTM! Type-only imports improve module boundaries.Using
typeimports forOauthTokens,OidcConfig,WellKnownResponse,StorageClient,LogoutErrorResult, andLogoutSuccessResultis correct and enables better tree-shaking.
26-26: LGTM! Clearer type annotation.Using
StorageClient<OauthTokens>directly is clearer and more explicit than theReturnTypeutility.
53-57: LGTM! Simplified deleteResponse handling.The removal of the
undefinedfallback and direct use ofdeleteResponsealigns with the updatedstorageClient.remove()signature, which now returnsPromise<GenericError | null>. The error check on line 57 correctly identifiesGenericErrorby the presence of theerrorproperty.
69-72: Type mismatch: Response fields must benullperLogoutSuccessResultdefinition.The
LogoutSuccessResulttype requires all three fields to benull:
RevokeSuccessResultdefinesrevokeResponse: nullanddeleteResponse: nullLogoutSuccessResultextends it withsessionResponse: nullThe current code assigns actual response values, violating the type contract.
Apply this fix:
} else { const result: LogoutSuccessResult = { - sessionResponse, - revokeResponse, - deleteResponse, + sessionResponse: null, + revokeResponse: null, + deleteResponse: null, }; return Micro.succeed(result); }packages/sdk-effects/storage/src/lib/storage.effects.test.ts (5)
124-130: LGTM!The test setup correctly clears mock state and storage between tests. The switch from resetting mock methods to clearing the
customStoreobject (line 129) aligns well with the new object-based mock pattern.
132-199: LGTM!The localStorage tests correctly verify the new return types (null on success) and comprehensively test object/array serialization. The assertions properly ensure localStorage methods are called exclusively without cross-contamination from other storage backends.
201-266: LGTM!The sessionStorage tests correctly verify the new return types and comprehensively test object/array handling. The test coverage for parsing and serialization is thorough.
268-359: LGTM!The custom storage tests comprehensively cover both success and error paths. The error handling tests (lines 297-306, 328-337, 349-358) properly verify the GenericError shape, and the object/array handling tests ensure proper serialization/deserialization. The test coverage is thorough and aligns well with the new API.
78-112: Mock implementation is correct and properly validates the interface contract.After reviewing the implementation in
storage.effects.ts, line 100 showsconst valueToStore = JSON.stringify(value);which is then passed toconfig.custom.set(key, valueToStore)on line 102. TheCustomStorageObjectinterface (defined inpackages/sdk-types/src/lib/tokens.types.ts) specifies thatsetmust receive a string:set: (key: string, valueToSet: string) => Promise<void | GenericError>.The mock's type check on line 92 (
typeof valueToSet !== 'string') serves as defensive validation of the interface contract. While this condition won't trigger during normal operation (since the implementation always stringifies), it correctly tests that the mock adheres to its interface specification. The actual error scenario is tested via thevalueToSet === '"bad-value"'condition, which works as intended.Likely an incorrect or invalid review comment.
packages/oidc-client/src/lib/client.types.ts (4)
11-14: LGTM!The change to make
revokeResponseanddeleteResponsealwaysnullin the success result is correct. Success paths should not carry error payloads, making this type more precise and aligned with the storage API changes.
16-20: LGTM!The change from an intersection type to a standalone error type improves clarity. The fields
revokeResponseanddeleteResponseasGenericError | nullallow callers to determine which specific operations failed, providing better error observability.
22-24: LGTM!The change to make
sessionResponsealwaysnullin the success result is consistent with theRevokeSuccessResultchanges and correctly represents that success paths should not carry error payloads.
26-31: LGTM!The change to a standalone error type with three
GenericError | nullfields (sessionResponse,revokeResponse,deleteResponse) provides comprehensive error observability for the logout flow, allowing callers to determine which specific operations succeeded or failed.packages/sdk-effects/storage/src/lib/storage.effects.ts (3)
101-114: LGTM!The
set()implementation correctly stringifies values, handles both custom and browser storage paths, and returns the standardizedGenericError | nullresponse. The nullish coalescing operator (line 105) properly convertsvoidtonullfor custom storage. The error handling is clean and not nested.
115-127: LGTM!The
remove()implementation correctly handles both custom and browser storage paths with consistent error handling. The nullish coalescing operator (line 118) properly convertsvoidtonull, and the try/catch block is clean and not nested.
128-128: LGTM!The type assertion to
StorageClient<Value>is appropriate and ensures the returned object matches the public interface. This is a standard pattern for factory functions that return interface-conforming objects.
| if (storeType === 'custom') { | ||
| const value = await config.custom.get(key); | ||
| if (value === null) { | ||
| if (value === null || (typeof value === 'object' && 'error' in value)) { |
There was a problem hiding this comment.
I'm a bit confused by this. If custom.get can only return a string or null, why are we checking for objects and error properties?
There was a problem hiding this comment.
It can return a GenericError as well
ancheetah
force-pushed
the
SDKS-4361-storage-client-type
branch
from
bed870c to
93595d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/slow-teeth-melt.md(1 hunks)packages/oidc-client/src/lib/client.store.test.ts(1 hunks)packages/oidc-client/src/lib/client.store.ts(1 hunks)packages/oidc-client/src/lib/client.types.ts(1 hunks)packages/oidc-client/src/lib/logout.request.test.ts(1 hunks)packages/oidc-client/src/lib/logout.request.ts(3 hunks)packages/sdk-effects/storage/src/lib/storage.effects.test.ts(15 hunks)packages/sdk-effects/storage/src/lib/storage.effects.ts(2 hunks)packages/sdk-types/src/lib/tokens.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/oidc-client/src/lib/logout.request.test.ts
- .changeset/slow-teeth-melt.md
🧰 Additional context used
📓 Path-based instructions (1)
packages/**/?(*.)test.@(ts|tsx)
📄 CodeRabbit inference engine (CLAUDE.md)
Write unit tests with Vitest for package code
Files:
packages/oidc-client/src/lib/client.store.test.tspackages/sdk-effects/storage/src/lib/storage.effects.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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.496Z
Learnt from: ancheetah
PR: ForgeRock/ping-javascript-sdk#417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.496Z
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 (4)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
packages/oidc-client/src/lib/logout.request.ts (2)
packages/sdk-effects/storage/src/lib/storage.effects.ts (1)
StorageClient(9-13)packages/oidc-client/src/lib/config.types.ts (1)
OauthTokens(29-35)
packages/sdk-effects/storage/src/lib/storage.effects.test.ts (1)
packages/sdk-types/src/lib/tokens.types.ts (1)
CustomStorageObject(20-24)
packages/oidc-client/src/lib/client.types.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-23)
⏰ 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 (22)
packages/oidc-client/src/lib/client.store.test.ts (2)
16-17: LGTM: Simulating localStorage absence.Setting
localStoragetonullproperly simulates environments where localStorage is unavailable, ensuring tests exercise fallback storage paths.
76-99: Verify storage error handling in tests.The custom storage mock now returns
void(no errors) for all operations. Per the PR objectives, "Updates storage unit tests to check for errors in custom storage implementations." Consider adding test cases that exercise error returns fromsetandremoveto verify that OIDC client handles these errors appropriately.Based on the retrieved learning, call sites may not be handling storage errors yet. Do you want to add tests that verify error propagation when custom storage operations fail?
packages/oidc-client/src/lib/client.store.ts (1)
373-396: LGTM: Revoke flow correctly handles new storage API.The revoke flow correctly adapts to the new storage client API where
remove()returnsGenericError | null. The type narrowing logic properly detects errors from both the revoke request and storage deletion, and the success path correctly returns nulls per theRevokeSuccessResulttype.packages/sdk-types/src/lib/tokens.types.ts (2)
8-9: LGTM: Type-only import for GenericError.Using a type-only import is appropriate since
GenericErroris only referenced in type positions, preventing unnecessary runtime dependencies.
20-24: LGTM: Standardized error handling for storage operations.The updated
CustomStorageObjectinterface standardizes error returns toGenericErrorfor all operations, providing consistent error handling across the storage API. Note this is a breaking change for existing custom storage implementations.packages/oidc-client/src/lib/logout.request.ts (3)
10-13: LGTM: Type-only imports reduce runtime dependencies.Converting to type-only imports prevents unnecessary runtime dependencies and clarifies that these are only used for type checking.
26-26: LGTM: Using StorageClient interface for better abstraction.Changing from
ReturnType<typeof createStorage<OauthTokens>>toStorageClient<OauthTokens>improves type clarity and decouples from the implementation.
53-74: LGTM: Logout flow correctly handles new storage API.The logout flow properly adapts to the new storage client API where
remove()returnsGenericError | null. The explicit null assignments in the success path (lines 69-71) provide runtime clarity, as discussed in previous reviews.packages/sdk-effects/storage/src/lib/storage.effects.test.ts (6)
7-9: LGTM: Updated imports for error handling.Removing unused
Mocktype and addingGenericErrorimport aligns with the new error handling patterns in custom storage tests.
78-112: LGTM: Comprehensive error handling in custom storage mock.The updated custom storage mock properly implements the new
CustomStorageObjectinterface withGenericErrorreturns for error cases:
getreturns error when key is missingsetreturns error for invalid valuesremovereturns error when key is missingThis provides good test coverage for error propagation through the storage layer.
158-166: LGTM: Verifying null return on successful set.Test correctly verifies that
setreturnsnullon success, aligning with the new standardized API.
297-306: LGTM: Testing error propagation from custom storage get.Test properly verifies that errors from custom storage
getare propagated as structuredGenericErrorobjects.
328-337: LGTM: Testing error propagation from custom storage set.Test properly verifies that errors from custom storage
setare propagated as structuredGenericErrorobjects.
349-358: LGTM: Testing error propagation from custom storage remove.Test properly verifies that errors from custom storage
removeare propagated as structuredGenericErrorobjects, completing comprehensive error handling coverage.packages/oidc-client/src/lib/client.types.ts (4)
11-14: LGTM! Success type correctly uses null.The change from
GenericError | nulltonullfor success results is semantically correct—a successful operation returns no errors.
16-20: LGTM! Error type properly decoupled from success type.Removing the inheritance and making this a standalone type with explicit error fields is clearer and more maintainable. The
GenericError | nullfields appropriately capture individual operation failures.
22-24: LGTM! Logout success correctly extends revoke success.The composition is logical—logout includes revoke operations plus session cleanup—and maintains the consistent pattern of null fields for successful operations.
26-31: LGTM! Comprehensive error capture for logout operations.The standalone error type with
GenericError | nullfor all three operations (session, revoke, delete) enables precise error reporting while maintaining consistency with the broader storage API changes.packages/sdk-effects/storage/src/lib/storage.effects.ts (4)
11-12: API standardization looks good.The updated return types (
Promise<GenericError | null>) establish a consistent pattern across all storage operations. Based on learnings, call site updates for error handling are tracked as a follow-up task.
69-100: LGTM! Get implementation correctly handles all storage types.The implementation properly:
- Handles custom storage returning
string | null | GenericError(line 72)- Uses try-catch for browser storage retrieval and parsing
- Returns standardized errors via
createStorageError- Maintains consistent null-on-missing semantics
101-114: LGTM! Set implementation correctly delegates and handles errors.The implementation:
- Properly delegates to custom storage and coalesces undefined to null (line 105)
- Uses try-catch for browser storage with standardized error creation
- Returns null on success per the new API contract
115-127: LGTM! Remove implementation follows consistent patterns.The implementation mirrors the set method:
- Delegates to custom storage with null coalescing (line 118)
- Uses try-catch for browser storage
- Returns standardized errors
- Sequential try-catch structure avoids nesting complexity
| function createStorageError( | ||
| storeType: 'localStorage' | 'sessionStorage' | 'custom', | ||
| action: 'Storing' | 'Retrieving' | 'Removing' | 'Parsing', | ||
| ): GenericError { | ||
| let storageName; | ||
| switch (storeType) { | ||
| case 'localStorage': | ||
| storageName = 'local'; | ||
| break; | ||
| case 'sessionStorage': | ||
| storageName = 'session'; | ||
| break; | ||
| case 'custom': | ||
| storageName = 'custom'; | ||
| break; | ||
| default: | ||
| break; | ||
| } | ||
|
|
||
| return { | ||
| error: `${action}_error`, | ||
| message: `Error ${action.toLowerCase()} value from ${storageName} storage`, | ||
| type: action === 'Parsing' ? 'parse_error' : 'unknown_error', | ||
| }; | ||
| } |
There was a problem hiding this comment.
Handle the default case in the switch statement.
Lines 45-46 have an empty default case. If storeType doesn't match any case, storageName remains undefined, resulting in error messages like "Error storing value from undefined storage".
Either remove the unreachable default case or initialize storageName to handle unexpected values defensively.
Apply this diff to initialize storageName:
function createStorageError(
storeType: 'localStorage' | 'sessionStorage' | 'custom',
action: 'Storing' | 'Retrieving' | 'Removing' | 'Parsing',
): GenericError {
- let storageName;
+ let storageName: string;
switch (storeType) {
case 'localStorage':
storageName = 'local';
break;
case 'sessionStorage':
storageName = 'session';
break;
case 'custom':
storageName = 'custom';
break;
default:
- break;
+ storageName = 'unknown';
+ break;
}
return {
error: `${action}_error`,
message: `Error ${action.toLowerCase()} value from ${storageName} storage`,
type: action === 'Parsing' ? 'parse_error' : 'unknown_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.
| function createStorageError( | |
| storeType: 'localStorage' | 'sessionStorage' | 'custom', | |
| action: 'Storing' | 'Retrieving' | 'Removing' | 'Parsing', | |
| ): GenericError { | |
| let storageName; | |
| switch (storeType) { | |
| case 'localStorage': | |
| storageName = 'local'; | |
| break; | |
| case 'sessionStorage': | |
| storageName = 'session'; | |
| break; | |
| case 'custom': | |
| storageName = 'custom'; | |
| break; | |
| default: | |
| break; | |
| } | |
| return { | |
| error: `${action}_error`, | |
| message: `Error ${action.toLowerCase()} value from ${storageName} storage`, | |
| type: action === 'Parsing' ? 'parse_error' : 'unknown_error', | |
| }; | |
| } | |
| function createStorageError( | |
| storeType: 'localStorage' | 'sessionStorage' | 'custom', | |
| action: 'Storing' | 'Retrieving' | 'Removing' | 'Parsing', | |
| ): GenericError { | |
| let storageName: string; | |
| switch (storeType) { | |
| case 'localStorage': | |
| storageName = 'local'; | |
| break; | |
| case 'sessionStorage': | |
| storageName = 'session'; | |
| break; | |
| case 'custom': | |
| storageName = 'custom'; | |
| break; | |
| default: | |
| storageName = 'unknown'; | |
| break; | |
| } | |
| return { | |
| error: `${action}_error`, | |
| message: `Error ${action.toLowerCase()} value from ${storageName} storage`, | |
| type: action === 'Parsing' ? 'parse_error' : 'unknown_error', | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/sdk-effects/storage/src/lib/storage.effects.ts around lines 30 to
54, the switch leaves storageName undefined for unexpected storeType values
causing messages like "from undefined storage"; initialize storageName
defensively (e.g., let storageName = 'unknown') or set the default case to
assign 'unknown' (or 'custom' as desired) so returned error.message always
contains a valid storage name; keep the rest of the function unchanged.
4 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4361
Description
setandremovestorage client return type toGenericError | nullAdded changeset minor for OIDC client and storage
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores