fix(davinci-client): fix collector value types and simplify node reducer imports#637
fix(davinci-client): fix collector value types and simplify node reducer imports#637ancheetah wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: eaae82b 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 |
📝 WalkthroughWalkthroughThis PR refactors the collector value type system by introducing a centralized ChangesCollector Value Type System Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit eaae82b
☁️ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/little-lamps-float.md:
- Around line 1-3: The changeset currently marks '`@forgerock/davinci-client`' as
'patch' but you renamed a public TypeScript export from CollectorInputTypes to
CollectorValueTypes which is a breaking API change; update the changeset entry
to use a breaking bump (change the release type from 'patch' to 'major' for
'`@forgerock/davinci-client`') and include a short note in the changeset text
explaining the exported type rename (mention CollectorInputTypes →
CollectorValueTypes) so release tooling and consumers are aware it's a breaking
change.
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 52-55: The reducer handling the DeviceAuthenticationCollector must
handle action payloads where action.payload.value can be a DeviceValue object
instead of a string; currently DeviceAuthenticationCollector still compares
option.value (a string) directly to action.payload.value which throws when value
is an object. Update the DeviceAuthenticationCollector reducer (the branch that
processes updateCollectorValues) to normalize the incoming value: if typeof
action.payload.value === 'object' (a DeviceValue), use its id (e.g.
action.payload.value.id) to find the matching option, otherwise use the string
value; then proceed with the existing update logic. Ensure you reference
updateCollectorValues and option.value in your change so valid DeviceValue
payloads resolve by id rather than failing the direct comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a91301c6-6773-4427-b0aa-ef1c2ae1d581
📒 Files selected for processing (9)
.changeset/little-lamps-float.md.gitignorepackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/client.types.test-d.tspackages/davinci-client/src/lib/client.types.tspackages/davinci-client/src/lib/node.reducer.tspackages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
💤 Files with no reviewable changes (1)
- packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
| --- | ||
| '@forgerock/davinci-client': patch | ||
| --- |
There was a problem hiding this comment.
Use a breaking-change bump for this release.
Line 2 sets this as a patch, but this PR removes/renames a public exported type (CollectorInputTypes → CollectorValueTypes). That is a compile-time breaking API change for TypeScript consumers.
Suggested changeset fix
---
-'`@forgerock/davinci-client`': patch
+'`@forgerock/davinci-client`': major
---📝 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.
| --- | |
| '@forgerock/davinci-client': patch | |
| --- | |
| --- | |
| '`@forgerock/davinci-client`': major | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/little-lamps-float.md around lines 1 - 3, The changeset currently
marks '`@forgerock/davinci-client`' as 'patch' but you renamed a public TypeScript
export from CollectorInputTypes to CollectorValueTypes which is a breaking API
change; update the changeset entry to use a breaking bump (change the release
type from 'patch' to 'major' for '`@forgerock/davinci-client`') and include a
short note in the changeset text explaining the exported type rename (mention
CollectorInputTypes → CollectorValueTypes) so release tooling and consumers are
aware it's a breaking change.
| export const updateCollectorValues = createAction<{ | ||
| id: string; | ||
| value: | ||
| | string | ||
| | string[] | ||
| | boolean | ||
| | PhoneNumberInputValue | ||
| | PhoneNumberExtensionInputValue | ||
| | FidoRegistrationInputValue | ||
| | FidoAuthenticationInputValue; | ||
| value: CollectorValueTypes; | ||
| index?: number; |
There was a problem hiding this comment.
Device-auth updater now rejects valid DeviceValue payloads.
Line 54 now allows DeviceValue, but the DeviceAuthenticationCollector reducer path still compares option.value (string) to action.payload.value directly. When update() is narrowed to DeviceValue, that comparison fails and throws “No option found matching value to update” for valid inputs.
Suggested reducer fix (accept `DeviceValue` and resolve by id)
if (collector.type === 'DeviceAuthenticationCollector') {
- if (typeof action.payload.id !== 'string') {
- throw new Error('Index argument must be a string');
- }
- // Iterate through the options object and find option to update
- const option = collector.output.options.find(
- (option) => option.value === action.payload.value,
- );
+ const selectedId =
+ typeof action.payload.value === 'string'
+ ? action.payload.value
+ : typeof action.payload.value === 'object' &&
+ action.payload.value !== null &&
+ 'id' in action.payload.value &&
+ typeof action.payload.value.id === 'string'
+ ? action.payload.value.id
+ : null;
+
+ if (!selectedId) {
+ throw new Error('Value argument must be a valid device selection');
+ }
+
+ const option = collector.output.options.find((option) => option.value === selectedId);
if (!option) {
throw new Error('No option found matching value to update');
}
// Remap values back to DaVinci spec
collector.input.value = {
type: option.type,
id: option.value,
value: option.content,
};
+ return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/davinci-client/src/lib/node.reducer.ts` around lines 52 - 55, The
reducer handling the DeviceAuthenticationCollector must handle action payloads
where action.payload.value can be a DeviceValue object instead of a string;
currently DeviceAuthenticationCollector still compares option.value (a string)
directly to action.payload.value which throws when value is an object. Update
the DeviceAuthenticationCollector reducer (the branch that processes
updateCollectorValues) to normalize the incoming value: if typeof
action.payload.value === 'object' (a DeviceValue), use its id (e.g.
action.payload.value.id) to find the matching option, otherwise use the string
value; then proceed with the existing update logic. Ensure you reference
updateCollectorValues and option.value in your change so valid DeviceValue
payloads resolve by id rather than failing the direct comparison.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (18.21%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
==========================================
+ Coverage 18.07% 18.21% +0.13%
==========================================
Files 155 155
Lines 24398 24436 +38
Branches 1203 1212 +9
==========================================
+ Hits 4410 4450 +40
+ Misses 19988 19986 -2
🚀 New features to boost your workflow:
|
|
Deployed 3f02cf4 to https://ForgeRock.github.io/ping-javascript-sdk/pr-637/3f02cf485c45bfff8156ab2504f07724e00f65ea branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📉 @forgerock/davinci-client - 53.9 KB (-0.3 KB) ➖ No Changes➖ @forgerock/storage - 1.5 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
2 participants
JIRA Ticket
Description
client.types.tsCollectorInputTypes→CollectorValueTypesand addedDeviceValueto the union.CollectorValueType<T>conditional type: grouped branches by return type (string, boolean, string[], specialized, category catch-alls), fixedDeviceAuthenticationCollectorto returnDeviceValueinstead ofstring, addedneverforActionCollectorandNoValueCollector, and added aSingleValueAutoCollectorcatch-all.node.reducer.ts/client.store.tsCollectors(fromnode.types) andCollectorValueTypes(fromclient.types), collapsing verbose inline union annotations to single-type references.updater-narrowing.types.test-d.tsAPI reports
CollectorInputTypes→CollectorValueTypesexport.Summary by CodeRabbit
New Features
DeviceValuehandling.Refactor