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: fcca96c 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR centralizes collector value typing by adding ChangesCollector Value Type System Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 fcca96c
☁️ 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.
There was a problem hiding this comment.
CollectorInputTypes hasn't been released yet so this is non-breaking. The patch covers the fix for the DeviceAuthenticationCollector
There was a problem hiding this comment.
@ancheetah, that makes sense — if CollectorInputTypes was never part of a published release, renaming it carries no breaking-change risk for consumers, and a patch bump is the right level for the DeviceAuthenticationCollector fix. Thanks for the context!
| 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.
There was a problem hiding this comment.
Thank you, I need to investigate this further. I am not sure what the input type should be. This may be a bug.
There was a problem hiding this comment.
🧩 Analysis chain
Good call to investigate further — the key question is what shape the caller is expected to pass when updating a DeviceAuthenticationCollector. Let me pull up the relevant type definitions to help clarify.
🏁 Script executed:
#!/bin/bash
# Find DeviceValue type definition
rg -n "DeviceValue" --type ts -C 5
echo "---"
# Find DeviceAuthenticationCollector definition and its options shape
rg -n "DeviceAuthenticationCollector" --type ts -C 5
echo "---"
# Find the options shape for object select collectors
rg -n "returnObjectSelectCollector" --type ts -C 10
Length of output: 37404
@ancheetah — happy to help clarify what's happening here. The codebase tells a clear story:
The original contract (and existing test)
node.reducer.test.ts (line 595) passes value: '42036625-37a5-4c7a-b7c4-ef778838c8e1' — a plain string (the device UUID). The reducer then:
- Looks up the option whose
option.valuematches that string. - Constructs the
DeviceValueobject ({ type, id, value }) from the matched option. - Stores it into
collector.input.value.
So the original design intent was: caller passes a string, reducer builds the DeviceValue.
What this PR changed
CollectorValueType<DeviceAuthenticationCollector> was changed from string → DeviceValue in client.types.ts. That type is now exposed through the narrowed Updater<DeviceAuthenticationCollector>, so TypeScript would tell callers to pass a DeviceValue object — but the reducer still does a string comparison (option.value === action.payload.value), which breaks at runtime when an object is passed.
The two design paths you need to choose between:
| Caller passes | Reducer does | Type change needed | |
|---|---|---|---|
| A — original intent | string (device UUID) |
Looks it up → builds DeviceValue |
Revert CollectorValueType<DeviceAuthenticationCollector> back to string |
| B — caller-constructed | DeviceValue object |
Stores it directly (or matches by .id) |
Keep DeviceValue, but update the reducer to not do a string comparison |
Option A matches the existing test and the reducer's current lookup/construction logic. Option B would be a breaking change to the public update() API for this collector type.
Would you like to open a follow-up issue to track this so it doesn't get lost, or do you expect to address it in this PR?
@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.20%) 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.20% +0.12%
==========================================
Files 155 155
Lines 24398 24434 +36
Branches 1203 1212 +9
==========================================
+ Hits 4410 4448 +38
+ Misses 19988 19986 -2
🚀 New features to boost your workflow:
|
|
Deployed 1a373ab to https://ForgeRock.github.io/ping-javascript-sdk/pr-637/1a373abdbff338811daa70864cc2c98ef6961b64 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 |
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
My main concern here is the affect on the public api
| } | ||
|
|
||
| // @public (undocumented) | ||
| export type CollectorInputTypes = string | string[] | boolean | PhoneNumberInputValue | PhoneNumberExtensionInputValue | FidoRegistrationInputValue | FidoAuthenticationInputValue; |
There was a problem hiding this comment.
hmm, this would be a breaking change, no?
| category: 'SingleValueAutoCollector'; | ||
| } ? string : T extends { | ||
| category: 'MultiValueCollector'; | ||
| } ? string[] : CollectorInputTypes; |
There was a problem hiding this comment.
also seems to have been removed here?
| } | ||
|
|
||
| // @public (undocumented) | ||
| export type CollectorInputTypes = string | string[] | boolean | PhoneNumberInputValue | PhoneNumberExtensionInputValue | FidoRegistrationInputValue | FidoAuthenticationInputValue; |
|
|
||
| export type InitFlow = () => Promise<FlowNode | InternalErrorResponse>; | ||
|
|
||
| export type CollectorInputTypes = |
There was a problem hiding this comment.
This could also be breaking?
There was a problem hiding this comment.
This is just changing the name of a type that hasn't been released yet. I think I introduced CollectorInputTypes in the single checkbox feature PR. Here I am just renaming it to CollectorValueTypes. So it's aliasing this union as a new type. Is that still considered breaking?
There was a problem hiding this comment.
If it hasn't been released yet we're good.
ancheetah
force-pushed
the
update-davinci-types
branch
from
eaae82b to
db80c05
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/node.reducer.ts (1)
39-40: ⚡ Quick winUse
import typeforCollectorValueTypes/Collectorsinnode.reducer.ts.In
packages/davinci-client/src/lib/node.reducer.ts(lines 39-40),CollectorValueTypesandCollectorsare imported as runtime values but are declared/exported as type aliases (export type ...) and only used in type positions in this file. Converting them to type-only imports avoids any runtime import emission/retention in stricter TS/module setups.💡 Proposed fix
-import { CollectorValueTypes } from './client.types.js'; -import { Collectors } from './node.types.js'; +import type { CollectorValueTypes } from './client.types.js'; +import type { Collectors } from './node.types.js';🤖 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 39 - 40, Change the imports of the type-only symbols to type-only imports: replace the runtime imports of CollectorValueTypes and Collectors with type-only imports (import type { CollectorValueTypes } from '...'; import type { Collectors } from '...') in node.reducer.ts so those aliases are only emitted for TypeScript type-checking and not as runtime/ESM imports; ensure these imports remain used only in type positions in functions/types within the file (e.g., where CollectorValueTypes or Collectors are referenced).
🤖 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.
Nitpick comments:
In `@packages/davinci-client/src/lib/node.reducer.ts`:
- Around line 39-40: Change the imports of the type-only symbols to type-only
imports: replace the runtime imports of CollectorValueTypes and Collectors with
type-only imports (import type { CollectorValueTypes } from '...'; import type {
Collectors } from '...') in node.reducer.ts so those aliases are only emitted
for TypeScript type-checking and not as runtime/ESM imports; ensure these
imports remain used only in type positions in functions/types within the file
(e.g., where CollectorValueTypes or Collectors are referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d50e7841-0470-4f58-9d09-d8c5851b86c8
📒 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
✅ Files skipped from review due to trivial changes (1)
- .gitignore
| const inputValue = action.payload.value; | ||
| if (typeof inputValue !== 'string') { | ||
| throw new Error('Value argument must be a string'); |
There was a problem hiding this comment.
I know we shouldn't throw here but adding anyways so when we rebase the reducer refactoring we don't miss this.
ancheetah
force-pushed
the
update-davinci-types
branch
from
db80c05 to
fcca96c
Compare
3 participants
JIRA Ticket
Description
client.types.tsCollectorInputTypes→CollectorValueTypes.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
Refactor
Documentation
Chores
Tests