feat(davinci-client): add fido2 collectors#428
Conversation
🦋 Changeset detectedLatest commit: b327c79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
WalkthroughAdds WebAuthn/FIDO2 collectors and ObjectValue auto-collector support across types, utilities, store, reducer, and tests; extends validation shapes and updates updater/validator signatures to accept new collector categories and FIDO input value types. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant Store as ClientStore
participant Reducer as NodeReducer
participant Utils as CollectorUtils
Note over UI,Store: User action triggers collector update
UI->>Store: updateCollector(collectorId, value)
Store->>Reducer: dispatch(UPDATE_COLLECTOR, payload)
alt collector.type is SingleValueAutoCollector | ProtectCollector
Reducer->>Utils: returnSingleValueAutoCollector(field, idx, collectorType)
Utils-->>Reducer: SingleValueAutoCollector object
else collector.type is ObjectValueAutoCollector | FidoRegistrationCollector | FidoAuthenticationCollector
Reducer->>Utils: returnObjectValueAutoCollector(field, idx, collectorType)
Utils-->>Reducer: ObjectValueAutoCollector object (includes attestation/assertion)
end
Reducer->>Reducer: validate value (validation rules present or null)
alt valid
Reducer-->>Store: update state with new value
else invalid
Reducer-->>Store: emit validation error (generic non-validated message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Changes span type-system extensions, new collector categories, reducer logic branches, utility API surface changes, and broad test updates — heterogeneous edits requiring cross-file verification. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 b327c79
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (19.05%) 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 #428 +/- ##
==========================================
+ Coverage 18.75% 19.05% +0.30%
==========================================
Files 138 138
Lines 27368 27477 +109
Branches 951 975 +24
==========================================
+ Hits 5132 5237 +105
- Misses 22236 22240 +4
🚀 New features to boost your workflow:
|
@forgerock/davinci-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 a6c3b0c to https://ForgeRock.github.io/ping-javascript-sdk/pr-428/a6c3b0c1015f623ca16df10b00f3b6c0a6ce487a branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/journey-client - 82.0 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 |
![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 (1)
packages/davinci-client/src/lib/client.store.ts (1)
236-289: Allow AutoCollector updates with FIDO2 payloads.
returnAutoCollectornow emits FIDO2 collectors whoseinput.valueis aFido2RegistrationInputValue/Fido2AuthenticationInputValue, but the updater here still only acceptsstring | string[] | PhoneNumberInputValue. As a result,update(fidoCollector)(attestationPayload)no longer type-checks even though that payload is required for the new collectors. Please widen the accepted value union (and theUpdatertype inclient.types.ts) to include the FIDO2 input interfaces so the feature is actually usable without unsafe casts.-import type { - SingleValueCollectors, - MultiSelectCollector, - ObjectValueCollectors, - PhoneNumberInputValue, - AutoCollectors, - MultiValueCollectors, -} from './collector.types.js'; +import type { + SingleValueCollectors, + MultiSelectCollector, + ObjectValueCollectors, + PhoneNumberInputValue, + AutoCollectors, + MultiValueCollectors, + Fido2RegistrationInputValue, + Fido2AuthenticationInputValue, +} from './collector.types.js'; @@ - return function (value: string | string[] | PhoneNumberInputValue, index?: number) { + return function ( + value: + | string + | string[] + | PhoneNumberInputValue + | Fido2RegistrationInputValue + | Fido2AuthenticationInputValue, + index?: number, + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/davinci-client/src/lib/client.store.ts(2 hunks)packages/davinci-client/src/lib/collector.types.ts(5 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(3 hunks)packages/davinci-client/src/lib/collector.utils.ts(9 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/davinci-client/src/lib/node.types.test-d.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(601-605)Fido2AuthenticationCollector(606-610)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/collector.types.ts (4)
SingleValueCollectors(174-179)ObjectValueCollectors(415-420)MultiValueCollectors(250-252)AutoCollectors(617-621)packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)
packages/davinci-client/src/lib/davinci.utils.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-316)Fido2AuthenticationInputValue(318-330)
packages/davinci-client/src/lib/node.types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(601-605)Fido2AuthenticationCollector(606-610)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)ProtectField(156-161)packages/davinci-client/src/lib/collector.utils.ts (1)
returnAutoCollector(272-385)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (6)
AutoCollectorTypes(567-571)InferAutoCollectorType(630-640)ValidatedTextCollector(184-184)ObjectValueCollectors(415-420)MultiValueCollectors(250-252)AutoCollectors(617-621)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-316)Fido2AuthenticationInputValue(318-330)Fido2RegistrationCollector(601-605)Fido2AuthenticationCollector(606-610)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(423-425)returnFido2RegistrationCollector(433-435)returnFido2AuthenticationCollector(443-445)
⏰ 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: pr
- GitHub Check: Mend Code Security Check
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
d2c6b35 to
33ad81a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/davinci-client/src/lib/davinci.types.ts (2)
163-191: Export the FIDO2 option interfaces.
Fido2RegistrationOptions(line 163) andFido2AuthenticationOptions(line 183) are used in exported field types but are not themselves exported. TypeScript will emitTS4058during declaration generation because exported members reference private names.Apply this diff to export both interfaces:
-interface Fido2RegistrationOptions +export interface Fido2RegistrationOptions extends Omit<PublicKeyCredentialCreationOptions, 'challenge' | 'user'> { ... -interface Fido2AuthenticationOptions +export interface Fido2AuthenticationOptions extends Omit<PublicKeyCredentialRequestOptions, 'challenge' | 'allowCredentials'> {
186-190: FixallowCredentialsto be an array.The
allowCredentialsproperty is typed as a single object (lines 186-190), but the WebAuthn spec defines it asPublicKeyCredentialDescriptor[]. This breaks type compatibility and will cause runtime issues when multiple credentials are allowed.Apply this diff to correct the type:
allowCredentials?: { id: number[]; transports?: AuthenticatorTransport[]; type: PublicKeyCredentialType; - }; + }[];
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/node.reducer.ts (1)
182-189: Add explicit return or break to prevent fall-through.The FIDO2 case branches (lines 183-187) don't explicitly return or break, allowing fall-through to line 190 if neither credential option is present. While the
breakon line 188 prevents fall-through to the next case, the logic would be clearer with explicit returns in each branch.Consider this refactor for clarity:
case 'FIDO2': { if ('publicKeyCredentialCreationOptions' in field) { - return returnFido2RegistrationCollector(field, idx); + return returnFido2RegistrationCollector(field, idx); } else if ('publicKeyCredentialRequestOptions' in field) { - return returnFido2AuthenticationCollector(field, idx); + return returnFido2AuthenticationCollector(field, idx); } + // Fall through to default handling if neither option is present break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/davinci-client/src/lib/client.store.ts(2 hunks)packages/davinci-client/src/lib/collector.types.ts(5 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(3 hunks)packages/davinci-client/src/lib/collector.utils.ts(9 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/davinci.utils.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (6)
AutoCollectorTypes(548-552)InferAutoCollectorType(611-621)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(598-602)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)Fido2RegistrationCollector(582-586)Fido2AuthenticationCollector(587-591)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(423-425)returnFido2RegistrationCollector(433-435)returnFido2AuthenticationCollector(443-445)
packages/davinci-client/src/lib/node.types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(582-586)Fido2AuthenticationCollector(587-591)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/collector.types.ts (4)
SingleValueCollectors(174-179)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(598-602)packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)ProtectField(156-161)packages/davinci-client/src/lib/collector.utils.ts (1)
returnAutoCollector(272-385)
⏰ 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 (11)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
806-957: LGTM!The new
returnAutoCollectortests comprehensively validate the FIDO2 registration, authentication, and Protect collector structures. Each test correctly verifies input/output shapes, config mappings, and validation rules.
995-995: LGTM!Correctly updated to test an empty array instead of an empty object, aligning with the multi-value collector's array-based validation logic.
packages/davinci-client/src/lib/client.store.ts (1)
294-348: LGTM!The
validatemethod correctly extends support toAutoCollectors, including the new FIDO2 collectors. The category check properly includesSingleValueAutoCollector, and the JSDoc is appropriately updated.packages/davinci-client/src/lib/node.types.ts (1)
26-48: LGTM!The FIDO2 collector types are correctly imported and added to the public
Collectorsunion, extending the type system to support WebAuthn flows.packages/davinci-client/src/lib/node.reducer.ts (2)
300-324: LGTM!The FIDO2 collector update handling correctly validates input types and required properties (
attestationValuefor registration,assertionValuefor authentication) before updating collector values. The logic follows the established pattern used for other object-value collectors.
225-235: LGTM!The refinement from
SingleValueAutoCollectortoProtectCollector(line 228) provides more specific type checking while maintaining the string value validation. This aligns with the expanded auto-collector type system.packages/davinci-client/src/lib/collector.types.ts (1)
305-621: LGTM!The type system extensions for FIDO2 support are well-structured:
- New input value interfaces properly use
PublicKeyCredentialfrom the WebAuthn API- The
AutoCollectorgeneric is enhanced withIVfor clarity and optional validation- New collector types (
Fido2RegistrationCollector,Fido2AuthenticationCollector) correctly extend the base- Union types and inference mappings are complete and consistent
packages/davinci-client/src/lib/collector.utils.ts (4)
272-291: LGTM!The refactoring to remove the
dataparameter and construct the validation array upfront is cleaner and more maintainable. The validation logic correctly handles therequiredproperty common to FIDO2 fields.
314-365: LGTM!The FIDO2 collector branches are well-implemented:
- Proper discrimination based on credential options properties
- Config correctly captures
publicKeyCredentialCreationOptions/publicKeyCredentialRequestOptions,action, andtrigger- Input value initialized to
{}for object-based WebAuthn credentials- Validation array conditionally included based on content
423-445: LGTM!The wrapper functions follow the established pattern and correctly delegate to
returnAutoCollectorwith the appropriate collector type. The removal of thedataparameter fromreturnProtectCollectoraligns with the signature changes.
705-709: LGTM!The
returnValidatorsignature correctly extends to includeAutoCollectors, and the return type change fromRecord<string, string>toRecord<string, unknown>properly accommodates object-based input values used by FIDO2 collectors.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks like a really good start. I think we should think about the category for these new collectors. I also would like to know how DaVinci expects errors to be structured.
There was a problem hiding this comment.
I feel like we should start splitting these files up that are getting pretty long. Maybe split by category of collector? Thoughts?
There was a problem hiding this comment.
Same goes for the accompanying unit test file... it's very long. We could have a /collectors folder which hosts the utils and types. Break down by category as you suggested. The types could also probably be broken down by category. Question is do we want to do this refactoring now or save it for later?
There was a problem hiding this comment.
I'll leave this up to you. If you have bigger fish to fry, then we can do it later. I just thought I'd bring it up as I always get squeamish with long running files like this :)
There was a problem hiding this comment.
Will save this for the end of the sprint if there's time
There was a problem hiding this comment.
Sounds good. No rush. We can always just put a tech debt story in the backlog as well.
| key: field.key, | ||
| value: {}, | ||
| type: field.type, | ||
| validation: validationArray.length ? validationArray : undefined, |
There was a problem hiding this comment.
Can we use null instead of undefined?
There was a problem hiding this comment.
I gave null a try for the ObjectValueAutoCollectors. Let me know if you like it better. As code rabbit pointed out below, we'll have to update our other validator props in other collectors to be more consistent.
| return returnProtectCollector(field, idx); | ||
| } | ||
| case 'FIDO2': { | ||
| if ('publicKeyCredentialCreationOptions' in field) { |
There was a problem hiding this comment.
Rather than going off of a property's presence, would it be easier to use the action property's value: "REGISTER" or "AUTHENTICATE"?
There was a problem hiding this comment.
That's a good idea. I assume we are always guaranteed these values and it's not something user-configured?
There was a problem hiding this comment.
You're assumption is correct. These properties should not change, as it's a part of the API. I only suggest using the action property as it's a bit more concise and "self-documenting".
| 'publicKeyCredentialCreationOptions' in field | ||
| ) { | ||
| return { | ||
| category: 'SingleValueAutoCollector', |
There was a problem hiding this comment.
I think we should have a different category for this. SingleValue doesn't feel all that accurate. It's more of an ObjectValue, like device or phone number collectors, as opposed to Protect, which is just a string value, yeah? So, ObjectValueAutoCollector?
| input: { | ||
| key: field.key, | ||
| value: data || '', | ||
| value: '', |
There was a problem hiding this comment.
Yes, from my understanding this data passed in here comes from formData on the DaVinci response (which is typically prefilled data). Neither WebAuthn or Protect come with prefilled data as far as I know so I think it's ok to remove this.
| collector.category === 'SingleValueCollector' || | ||
| collector.category === 'ValidatedSingleValueCollector' || | ||
| collector.category === 'SingleValueAutoCollector' | ||
| collector.type === 'ProtectCollector' |
There was a problem hiding this comment.
If we change the category of the FIDO2 collectors to ObjectValueAutoCollectors, then we could potentially not need this change.
There was a problem hiding this comment.
I thought about this too. I'll go ahead and make that change to the category
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
33ad81a to
fe032c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/node.reducer.ts (1)
300-324: Consider consolidating duplicate FIDO2 validation logic.The validation blocks for
Fido2RegistrationCollector(lines 300-311) andFido2AuthenticationCollector(lines 313-324) are nearly identical, differing only in the property name check (attestationValuevsassertionValue). This duplication could be reduced with a helper function or consolidated logic.Example refactor to reduce duplication:
+ const isFido2Collector = + collector.type === 'Fido2RegistrationCollector' || + collector.type === 'Fido2AuthenticationCollector'; + + if (isFido2Collector) { - if (collector.type === 'Fido2RegistrationCollector') { if (typeof action.payload.id !== 'string') { throw new Error('Index argument must be a string'); } if (typeof action.payload.value !== 'object') { throw new Error('Value argument must be an object'); } - if (!('attestationValue' in action.payload.value)) { - throw new Error('Value argument must contain an attestationValue property'); + const requiredProp = collector.type === 'Fido2RegistrationCollector' + ? 'attestationValue' + : 'assertionValue'; + if (!(requiredProp in action.payload.value)) { + throw new Error(`Value argument must contain a ${requiredProp} property`); } collector.input.value = action.payload.value; + return; } - - if (collector.type === 'Fido2AuthenticationCollector') { - if (typeof action.payload.id !== 'string') { - throw new Error('Index argument must be a string'); - } - if (typeof action.payload.value !== 'object') { - throw new Error('Value argument must be an object'); - } - if (!('assertionValue' in action.payload.value)) { - throw new Error('Value argument must contain an assertionValue property'); - } - collector.input.value = action.payload.value; - }packages/davinci-client/src/lib/collector.types.ts (1)
555-576: Unify handling of thevalidationfield. It’s currently declared asvalidation?: ValidationRequired[] | nullbut some code paths assignundefined(e.g. collector.utils.ts lines 523–527 and 650–654) while others usenull. Decide on a single “no validations” sentinel—eithernullorundefined—and update the interface and all assignments to match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.ts(5 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(3 hunks)packages/davinci-client/src/lib/collector.utils.ts(9 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/davinci.utils.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/davinci-client/src/lib/node.reducer.test.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
Fido2RegistrationOptions(163-171)Fido2AuthenticationOptions(183-191)
packages/davinci-client/src/lib/node.types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.utils.ts (2)
returnSingleValueAutoCollector(273-324)returnObjectValueAutoCollector(333-419)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(457-459)returnFido2RegistrationCollector(467-469)returnFido2AuthenticationCollector(477-479)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)packages/davinci-client/src/lib/collector.types.ts (7)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)SingleValueCollectors(174-179)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
⏰ 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 (6)
packages/davinci-client/src/lib/collector.types.ts (2)
305-311: LGTM! FIDO2 input value types are well-defined.The optional fields (
attestationValue?andassertionValue?) align with the WebAuthn API flow where these values are populated after the browser credential operation completes.
547-553: Good separation of auto collector categories.Splitting
SingleValueAutoCollectorTypesandObjectValueAutoCollectorTypesprovides clear type boundaries and makes the FIDO2 collectors (which handle complex objects) distinct from simple value collectors like Protect.packages/davinci-client/src/lib/node.reducer.ts (1)
182-189: LGTM! Action-based routing aligns with prior feedback.Using
field.actionto distinguish between REGISTER and AUTHENTICATE flows is more concise and self-documenting than checking for property presence.Based on learnings
packages/davinci-client/src/lib/collector.utils.ts (3)
326-419: LGTM! NewreturnObjectValueAutoCollectorfollows established patterns.The function correctly:
- Validates required field properties
- Builds validation arrays based on field requirements
- Distinguishes between FIDO2 registration and authentication via
field.action- Initializes
input.valueto an empty object{}(appropriate for FIDO2 flows)- Returns properly typed collectors
589-605: Good defensive programming with optional chaining.Adding optional chaining (
field.options?.) prevents potential runtime errors if the field structure is malformed or incomplete.Also applies to: 617-624
736-743: Ensure returnValidator handles FIDO2 object values
The ‘required’ rule counts object keys (e.g. attestationValue/assertionValue) but there are no tests exercising returnValidator for Fido2RegistrationCollector or Fido2AuthenticationCollector. Add or update validator tests to confirm FIDO2 inputs pass/ fail as intended.
| key: field.key, | ||
| value: {}, | ||
| type: field.type, | ||
| validation: validationArray.length ? validationArray : null, |
There was a problem hiding this comment.
Inconsistent validation assignment: null vs array length check.
Lines 365, 388, and 411 use validationArray.length ? validationArray : null, setting validation to null when empty. However, other collectors in this file (e.g., lines 525, 652) use undefined for empty validation. This inconsistency should be resolved.
Consider standardizing to undefined (or omitting the field entirely):
- validation: validationArray.length ? validationArray : null,
+ validation: validationArray.length ? validationArray : undefined,Or even better, only include validation when present:
input: {
key: field.key,
value: {},
type: field.type,
- validation: validationArray.length ? validationArray : null,
+ ...(validationArray.length && { validation: validationArray }),
},Also applies to: 388-388, 411-411
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/collector.utils.ts around lines 365, 388 and
411, the code sets validation to null when validationArray is empty (validation:
validationArray.length ? validationArray : null) which is inconsistent with
other collectors that use undefined; update these three locations to standardize
behavior by either (a) setting validation to undefined when empty (validation:
validationArray.length ? validationArray : undefined) or preferably (b) only
include the validation field when validationArray has items (i.e., conditionally
add the validation property to the returned object when validationArray.length >
0) so all collectors use the same undefined/omitted convention.
There was a problem hiding this comment.
Where are the other instances of undefined being assigned?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all instances where validation is assigned in collector.utils.ts
rg -n "validation.*validationArray" packages/davinci-client/src/lib/collector.utils.ts
Length of output: 411
@cerebrl The other instances where undefined is assigned to validation are:
-
Line 525 - in
returnMultiValueCollector:validation: validationArray.length ? validationArray : undefined,
-
Line 652 - in
returnObjectCollector:validation: validationArray.length ? validationArray : undefined,
In contrast, the new returnObjectValueAutoCollector function uses null for validation on lines 365, 388, and 411. This creates an inconsistency with the existing pattern in the codebase where returnMultiValueCollector and returnObjectCollector both use undefined when the validation array is empty.
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
There was a problem hiding this comment.
Nice! This is actually helpful!
There was a problem hiding this comment.
@cerebrl You're welcome! I'm glad the analysis was helpful. Let me know if you need anything else as you work through the validation inconsistency. 😊
|
Added ObjectValueAutoCollector type and unit tests for AutoCollectors in the node reducer. |
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I think this can be merged in, but I left a couple of small, non-blocking comments to think about.
| } | ||
|
|
||
| if (collectorType === 'ProtectCollector') { | ||
| if (collectorType === 'ProtectCollector' && field.type === 'PROTECT') { |
There was a problem hiding this comment.
Is this addition to the conditional still needed now that we've narrowed the AutoCollector types?
There was a problem hiding this comment.
good catch, no longer needed
| }); | ||
| } | ||
|
|
||
| if (collectorType === 'Fido2RegistrationCollector' && field.action === 'REGISTER') { |
There was a problem hiding this comment.
This isn't a big thing, but does the tested collectorType not narrow enough? It would be nice if the type being Fido2RegistrationCollector would be enough without the need of the added field.action === 'REGISTER'. This would make it consistent with the rest of this file's conditions.
There was a problem hiding this comment.
Narrowing on just collectorType is not enough because the field input is then left ambiguous and typescript will complain about the public key credential options.
If you take a look at returnObjectCollector we do something similar - narrow on field.type. In the case of FIDO2 the field.type is FIDO2 for both registration/authentication so we can't use that. It's actually enough to narrow if we just use field.action but it feels safer to explicitly have the collectorType conditional in case other field types have a similar action.
There was a problem hiding this comment.
I guess I misspoke. I should have asked if we can narrow on field.action alone? Does the collectorType matter in this sense? It doesn't seem to impact the logic at all.
There was a problem hiding this comment.
Yes, we can narrow on field.action alone. This would also remove the need for returning a generic ObjectValueAutoCollector at the end because at that point the field is of type never.
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
fe032c0 to
7471c5c
Compare
|
Updated validation types and narrowing on ObjectValueAutoCollectors. |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/node.reducer.test.ts (2)
925-1028: Consider typing the mock input value explicitly.The TODO comment on line 927 suggests explicitly typing
mockInputValueasFido2RegistrationInputValue. While the test works with the implicit typing, explicit typing would improve type safety and catch mismatches earlier.Apply this diff to add explicit typing:
- // todo: declare inputValue type as Fido2RegistrationInputValue - const mockInputValue = { + const mockInputValue: Fido2RegistrationInputValue = { attestationValue: {Otherwise, the test comprehensively validates the Fido2RegistrationCollector pathway with appropriate mock data structures.
1030-1117: Consider typing the mock input value explicitly.Similar to the registration test, the TODO comment on line 1032 suggests explicitly typing
mockInputValueasFido2AuthenticationInputValuefor improved type safety.Apply this diff to add explicit typing:
- // todo: declare inputValue type as Fido2AuthenticationInputValue - const mockInputValue = { + const mockInputValue: Fido2AuthenticationInputValue = { assertionValue: {The test otherwise provides excellent coverage of the Fido2AuthenticationCollector update pathway with realistic mock credentials and options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.test-d.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(10 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(10 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(12 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/davinci-client/src/lib/node.reducer.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/davinci.utils.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.utils.ts (2)
returnSingleValueAutoCollector(273-324)returnObjectValueAutoCollector(333-401)
packages/davinci-client/src/lib/node.reducer.test.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
Fido2RegistrationOptions(163-171)Fido2AuthenticationOptions(183-191)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)packages/davinci-client/src/lib/collector.types.ts (7)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)SingleValueCollectors(174-179)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
⏰ 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 (27)
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
305-305: LGTM! Test reflects nullable validation field.The addition of
validation: nullcorrectly tests the expanded input shape for multi-value collectors, aligning with the broader changes to make validation fields optional across the collector type system.packages/davinci-client/src/lib/node.reducer.test.ts (2)
440-440: LGTM! Validation field updates are consistent.The addition of
validation: nullacross these test expectations correctly reflects the expanded input shapes that now include optional validation metadata. This change is consistent with the broader type system updates for multi-value and object-value collectors.Also applies to: 475-475, 526-526, 576-576, 634-634, 677-677, 742-742, 786-786, 826-826, 852-852
867-923: LGTM! Comprehensive ProtectCollector test coverage.The test suite properly validates the ProtectCollector's update pathway, including the correct input structure, configuration mapping, and value updates. The test expectations align with the SingleValueAutoCollector category and the Protect-specific configuration schema.
packages/davinci-client/src/lib/davinci.types.ts (4)
152-153: LGTM! Phone number validation flag added.The addition of the
validatePhoneNumber: booleanfield extends the PhoneNumberField type to support validation control, which aligns with the validation enhancements across the collector system.
163-191: LGTM! FIDO2 option interfaces are properly exported.The
Fido2RegistrationOptionsandFido2AuthenticationOptionsinterfaces are correctly exported and properly extend the WebAuthn spec types. The overrides forchallenge,user, andallowCredentialscorrectly usenumber[]arrays instead ofBufferSourcetypes, which aligns with the DaVinci API's JSON-serializable format.
173-201: LGTM! FIDO2 field types are well-structured.The
Fido2RegistrationFieldandFido2AuthenticationFieldtypes properly capture the FIDO2 flow metadata, including the credential options, action discriminators, trigger mechanisms, and required flags. The discriminated union on theactionfield ('REGISTER' vs 'AUTHENTICATE') enables type-safe handling in downstream code.
212-218: LGTM! SingleValueFields union expanded correctly.The union now includes
ProtectField,Fido2RegistrationField, andFido2AuthenticationField, properly extending the single-value field types to support the new auto-collector pathways.packages/davinci-client/src/lib/collector.utils.test.ts (4)
480-480: LGTM! Validation field updates are consistent.The addition of
validation: nullacross these test expectations correctly reflects the nullable validation field introduced in the type definitions. This ensures tests remain aligned with the updated collector interfaces.Also applies to: 631-631, 671-671, 713-713, 754-754
812-842: LGTM! ProtectCollector test is comprehensive.The test validates the
returnSingleValueAutoCollectorutility with a ProtectField, correctly verifying the category, type, input/output structures, and configuration mapping. The test expectations align with the SingleValueAutoCollector category and Protect-specific config schema.
844-959: LGTM! FIDO2 collector tests are thorough.Both Fido2RegistrationCollector and Fido2AuthenticationCollector tests comprehensively validate the
returnObjectValueAutoCollectorutility, including:
- Correct category assignment (ObjectValueAutoCollector)
- Proper input/output structure mapping
- Configuration extraction from field properties
- Validation array handling (required vs null)
- Action discriminator handling
The mock credential options are realistic and align with WebAuthn specifications.
997-997: LGTM! Empty multi-value corrected.Changing the empty value expectation from
{}to[]correctly reflects that multi-value collectors expect array values, not object values. This aligns with the MultiValueCollector input type definition wherevalueisstring[].packages/davinci-client/src/lib/collector.types.ts (8)
205-205: LGTM! Validation field made nullable.Making the
validationfield nullable (ValidationRequired[] | null) allows collectors to explicitly represent the absence of validation rules, which is appropriate for optional validation scenarios. This change is consistently applied across multi-value collector interfaces.Also applies to: 226-226
305-311: LGTM! FIDO2 input value interfaces added.The
Fido2RegistrationInputValueandFido2AuthenticationInputValueinterfaces properly capture the WebAuthn credential payloads. Using optional properties forattestationValueandassertionValueallows these collectors to start with empty input and be populated after credential ceremonies complete.
326-326: LGTM! Object collector validation fields updated.Consistently making the
validationfield nullable across object-value collector interfaces allows explicit representation of no-validation scenarios. The expansion to includeValidationPhoneNumberin line 375 properly supports phone number validation rules.Also applies to: 350-350, 375-375
547-553: LGTM! AutoCollector taxonomy expanded.The split into
SingleValueAutoCollectorTypesandObjectValueAutoCollectorTypeswith a unifiedAutoCollectorTypesunion properly organizes the new FIDO2 collectors alongside the existing Protect collector. The addition ofObjectValueAutoCollectortoAutoCollectorCategoriesenables category-based discrimination in the type system.
558-558: LGTM! Generic parameter renamed for clarity.Renaming the generic from
VtoIV(Input Value) more clearly indicates this type parameter represents the input value shape rather than a generic value. This improves code readability and self-documentation.Also applies to: 567-567
578-602: LGTM! New auto-collector types properly defined.The new collector types (
ProtectCollector,Fido2RegistrationCollector,Fido2AuthenticationCollector,ObjectValueAutoCollector) are correctly defined using theAutoCollectorgeneric with appropriate categories, type literals, and input value types. The type parameters properly constrain the input values to their respective shapes.
604-609: LGTM! AutoCollectors union expanded correctly.The
AutoCollectorsunion now includes all five auto-collector types, enabling exhaustive pattern matching and type narrowing across the FIDO2 and Protect flows.
618-630: LGTM! Type inference expanded for new collectors.The
InferAutoCollectorTypeconditional type properly maps each collector type literal to its corresponding type, with a sensible fallback toSingleValueAutoCollectorfor unmapped cases. The inference chain correctly handles all five auto-collector variants.packages/davinci-client/src/lib/client.store.ts (3)
37-38: LGTM! Necessary type imports added.The FIDO2 input value types are correctly imported to support the new collector types in the update method signature.
266-279: Verify that SingleValueAutoCollector category is intentionally excluded from validation.The update method now accepts both
SingleValueAutoCollectorandObjectValueAutoCollectorcategories. However, in the validate method (lines 338-349), onlyObjectValueAutoCollectoris allowed whileSingleValueAutoCollectoris excluded. This appears intentional since ProtectCollector (a SingleValueAutoCollector) may not have validation rules. Please confirm this design decision is correct.
281-288: LGTM! Function signature correctly expanded to handle FIDO2 input values.The updater function signature now properly includes
Fido2RegistrationInputValueandFido2AuthenticationInputValueto support the new ObjectValueAutoCollector types.packages/davinci-client/src/lib/collector.utils.ts (5)
267-324: LGTM! Well-refactored single-value auto collector.The refactored
returnSingleValueAutoCollectorappropriately narrows the scope toProtectFieldand initializes the input value to an empty string. The removal of thedataparameter makes sense since auto collectors don't receive prefilled data.
326-401: LGTM! FIDO2 object-value auto collector implementation is solid.The new
returnObjectValueAutoCollectorfunction properly handles FIDO2 registration and authentication collectors with:
- Appropriate object-value initialization (
{})- Conditional validation when
requiredis true- Proper branching on
field.actionto distinguish registration from authentication- Consistent use of
nullfor empty validation arraysThe implementation aligns well with the existing collector patterns.
507-507: Excellent! Validation consistency improved.The validation assignment has been standardized to use
nullinstead ofundefinedwhen the validation array is empty (lines 507, 634). This resolves the inconsistency that was flagged in previous reviews and aligns with the pattern used in the new FIDO2 collectors (lines 365, 388).Also applies to: 634-634
571-587: LGTM! Defensive programming with optional chaining.The addition of optional chaining (
field.options?.map) with nullish coalescing (?? []) provides better safety against undefined options while maintaining the expected array return type. This prevents potential runtime errors.Also applies to: 599-606
718-725: LGTM! Validator signature correctly expanded for object values.The
returnValidatorfunction signature now acceptsAutoCollectorsand handlesRecord<string, unknown>values, which is necessary for validating FIDO2 collectors with object-shaped input values. The function logic appropriately handles empty objects in the required validation check.
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
7471c5c to
c1ae746
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/davinci-client/src/lib/davinci.types.ts (1)
183-191: Authentication options fix is correct; minor optional tweak
allowCredentialsnaming and array shape are correct.- Optional: consider
readonly number[]forchallenge/idto signal immutability (non-blocking).Also applies to: 193-201
packages/davinci-client/src/lib/client.store.ts (2)
281-289: Type-safety improvement: specialize updater value by collector typeCurrent
valueunion permits shapes that don’t apply to some collectors. Consider a generic signature to infer the expected value from the collector, reducing runtime errors.Example approach (illustrative):
- Define
UpdaterFor<C>mapping collector → expected value.- Change
update<C extends AllowedCollectors>(collector: C): (value: UpdaterFor<C>, index?: number) => ...Would you like a typed diff introducing
UpdaterForand updatingupdateaccordingly?
310-315: Validate signature vs runtime gating
validateacceptsAutoCollectors, but onlyObjectValueAutoCollectoris validated at runtime. Either:
- Narrow the parameter type to only validatable categories, or
- Add a brief JSDoc clarifying that only some categories have validation rules.
Non-blocking; current behavior is safe due to gating.
Also applies to: 338-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.changeset/tired-melons-wish.md(1 hunks)packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.test-d.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(10 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(10 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(12 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)packages/journey-client/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/davinci.utils.ts
- packages/davinci-client/src/lib/collector.types.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/davinci-client/src/lib/node.types.test-d.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)
packages/davinci-client/src/lib/node.types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)packages/davinci-client/src/lib/collector.types.ts (7)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)SingleValueCollectors(174-179)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)Fido2RegistrationInputValue(305-307)Fido2AuthenticationInputValue(309-311)Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(439-441)returnFido2RegistrationCollector(449-451)returnFido2AuthenticationCollector(459-461)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)Fido2RegistrationField(173-181)Fido2AuthenticationField(193-201)packages/davinci-client/src/lib/collector.utils.ts (2)
returnSingleValueAutoCollector(273-324)returnObjectValueAutoCollector(333-401)
packages/davinci-client/src/lib/node.reducer.test.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)Fido2RegistrationCollector(583-587)Fido2AuthenticationCollector(588-592)packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
Fido2RegistrationOptions(163-171)Fido2AuthenticationOptions(183-191)
⏰ 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: Mend Security Check
🔇 Additional comments (30)
packages/journey-client/package.json (1)
36-36: Align vite versions in dependencies and devDependencies, and clarify if this change is intentional.The version mismatch is confirmed: vite 6.3.6 in dependencies (line 36) and vite 6.3.4 in devDependencies (line 41). This divergence should be resolved by updating devDependencies to match:
"devDependencies": { "@vitest/coverage-v8": "^1.2.0", - "vite": "6.3.4", + "vite": "6.3.6", "vitest": "^1.2.0" }Please confirm whether this vite version bump to 6.3.6 is necessary for the FIDO2 collector changes or if it is incidental.
.changeset/tired-melons-wish.md (1)
1-5: Changeset looks goodMinor bump and succinct summary for adding WebAuthn/FIDO2 collectors is appropriate.
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
301-307: Type test aligns with new nullable validationAdding
validation: nullmatches updated collector input shape (ValidationRequired[] | null).packages/davinci-client/src/lib/davinci.types.ts (3)
152-154: PhoneNumberField additions OK
requiredandvalidatePhoneNumberfields look correct and consistent with other fields.
163-171: FIDO2 registration types are well-formedExported
Fido2RegistrationOptionsandFido2RegistrationFieldmirror WebAuthn, with binary IDs represented asnumber[]. Looks good.Also applies to: 173-181
212-218: Union extension is appropriateIncluding FIDO2 fields in
SingleValueFieldsaligns with how fields are grouped for rendering.packages/davinci-client/src/lib/client.store.ts (1)
271-276: Category gating for update is correctAllowing both SingleValueAutoCollector and ObjectValueAutoCollector is needed for Protect and FIDO2 paths.
packages/davinci-client/src/lib/node.types.test-d.ts (1)
36-38: Collector union tests updated correctlyIncluding FIDO2 collectors in the
Collectorsunion checks matches the new public surface.Also applies to: 221-241
packages/davinci-client/src/lib/node.types.ts (1)
26-27: LGTM! Clean type extension for FIDO2 support.The addition of
Fido2RegistrationCollectorandFido2AuthenticationCollectorto the public type exports andCollectorsunion is consistent with the broader FIDO2 support being added across the codebase.Also applies to: 47-48
packages/davinci-client/src/lib/node.reducer.test.ts (4)
440-440: LGTM! Consistent validation handling.Adding
validation: nullto the input shapes across multiple collectors standardizes the validation property handling throughout the test suite, aligning with the updated collector factory functions.Also applies to: 475-475, 526-526, 576-576, 634-634, 677-677, 742-742, 786-786, 826-826, 852-852
867-923: LGTM! Comprehensive ProtectCollector test coverage.The test properly validates the update flow for ProtectCollector with SingleValueAutoCollector category, ensuring correct value assignment.
925-1028: LGTM! Thorough Fido2RegistrationCollector test coverage.The test validates the complete update flow for FIDO2 registration, including the ObjectValueAutoCollector category, input value structure with attestation, and output configuration with publicKeyCredentialCreationOptions.
1030-1117: LGTM! Comprehensive Fido2AuthenticationCollector test coverage.The test properly validates the update flow for FIDO2 authentication, covering the ObjectValueAutoCollector category, input value structure with assertion, and output configuration with publicKeyCredentialRequestOptions.
packages/davinci-client/src/lib/node.reducer.ts (6)
29-30: LGTM! Necessary imports for FIDO2 support.The new imports for
returnFido2RegistrationCollector,returnFido2AuthenticationCollector, and their associated types align with the broader FIDO2 functionality being added.Also applies to: 52-55
70-75: LGTM! Proper type extension for FIDO2 input values.Extending the
updateCollectorValuesaction's value union to includeFido2RegistrationInputValueandFido2AuthenticationInputValuecorrectly handles the object-based input structures required for WebAuthn flows.
99-100: LGTM! Consistent initial state extension.Adding the new FIDO2 collector types to
initialCollectorValuesmaintains consistency with the extended collector system.
179-189: LGTM! Clean FIDO2 routing logic.The updated
returnProtectCollectorcall (removing the data parameter) aligns with the refactored function signature. The new FIDO2 branch properly routes to registration or authentication collectors based on thefield.actiondiscriminator. Thebreakstatement ensures that iffield.actionis neither 'REGISTER' nor 'AUTHENTICATE', execution falls through to the generic collector handling below, which is appropriate fallback behavior.
225-235: LGTM! Correct type narrowing for ProtectCollector.The condition now checks
collector.type === 'ProtectCollector'instead of category, which correctly handles the ProtectCollector as a SingleValueAutoCollector with string input values. This aligns with the new collector categorization where ProtectCollector has the category 'SingleValueAutoCollector' but needs special handling.
300-324: LGTM! Proper FIDO2 input validation and assignment.Both
Fido2RegistrationCollectorandFido2AuthenticationCollectorupdate handlers correctly:
- Validate that
idis a string (required for tracking)- Validate that
valueis an object- Check for required properties (
attestationValueorassertionValue)- Assign the value to
collector.input.valueThis follows the same pattern as the existing
PhoneNumberCollectorhandler and provides appropriate error messages.packages/davinci-client/src/lib/collector.utils.test.ts (4)
23-24: LGTM! Updated imports for new auto-collector API.The new imports
returnSingleValueAutoCollectorandreturnObjectValueAutoCollectoralong with the FIDO2 and Protect field types enable testing of the refactored auto-collector functionality.Also applies to: 30-33
480-480: LGTM! Consistent validation handling across tests.Setting
validation: nullfor collectors without validation rules and updating the multi-value validator test expectation from{}to[]standardizes the validation property handling throughout the test suite.Also applies to: 526-526, 576-576, 631-631, 671-671, 713-713, 754-754, 997-997
812-842: LGTM! Comprehensive ProtectCollector test.The test validates the
returnSingleValueAutoCollectorfunction with aProtectField, verifying the correct SingleValueAutoCollector category, input/output structure, and config mapping for behavioral data collection and device identification settings.
844-959: LGTM! Thorough FIDO2 collector tests.Both test cases comprehensively validate:
Fido2RegistrationCollector: ObjectValueAutoCollector category, empty object input value initialization, required validation whenrequired: true, and config with publicKeyCredentialCreationOptionsFido2AuthenticationCollector: Similar structure withrequired: falseresulting invalidation: null, and config with publicKeyCredentialRequestOptionsThe tests properly cover the two FIDO2 action types and their distinct option structures.
packages/davinci-client/src/lib/collector.utils.ts (7)
29-31: LGTM! Necessary type imports for auto-collector refactoring.The imports for
AutoCollectors,SingleValueAutoCollectorTypes,ObjectValueAutoCollectorTypes, and the FIDO2 field types support the new auto-collector API structure.Also applies to: 36-37
267-324: LGTM! Refactored for single-value auto-collectors.The renamed
returnSingleValueAutoCollectorfunction correctly:
- Narrows the field type to
ProtectField- Removes the
dataparameter and always initializesvalueto an empty string (appropriate for auto-collected fields that are populated automatically)- Uses
SingleValueAutoCollectorTypesfor type narrowing- Returns proper structure for ProtectCollector with config object
This aligns with the past review discussion about removing prefilled data handling for auto-collectors.
326-401: LGTM! New object-value auto-collector for FIDO2.The
returnObjectValueAutoCollectorfunction properly:
- Handles
Fido2RegistrationField | Fido2AuthenticationFieldwithObjectValueAutoCollectorTypes- Initializes input value to an empty object
{}- Adds required validation when
field.required === true- Uses
validation: nullwhen the validation array is empty (consistent with the broader standardization)- Branches on
field.action('REGISTER' vs 'AUTHENTICATE') to return the appropriate config with eitherpublicKeyCredentialCreationOptionsorpublicKeyCredentialRequestOptionsThe use of
nullfor empty validation is consistent with the pattern established in other collectors and aligns with the past review discussion.
439-461: LGTM! Clean factory functions for FIDO2 collectors.The updated
returnProtectCollectornow delegates toreturnSingleValueAutoCollectorwithout a data parameter, and the newreturnFido2RegistrationCollectorandreturnFido2AuthenticationCollectorfunctions provide convenient wrappers that delegate toreturnObjectValueAutoCollector. This follows the established pattern in the file.
507-507: LGTM! Standardized validation handling.Setting
validationtonullwhen the validation array is empty (rather thanundefined) provides consistency across all collectors. This aligns with the past review discussion about standardizing validation property handling.Also applies to: 634-634
571-587: LGTM! Added defensive optional chaining.The optional chaining operators (
field.options?.) prevent potential runtime errors if the options array is unexpectedly undefined, even though type constraints should ensure options exist. The nullish coalescing (?? []) provides a safe fallback to an empty array.Also applies to: 599-606
718-725: LGTM! Extended validator to support auto-collectors.The
returnValidatorfunction now acceptsAutoCollectorsin its union type and the return function acceptsRecord<string, unknown>as a possible value type. This properly handles validation for the new FIDO2 object-value collectors that work with complex objects rather than just strings or arrays.
|
|
||
| describe('The node collector reducer with Fido2RegistrationFieldValue', () => { | ||
| it('should handle collector updates ', () => { | ||
| // todo: declare inputValue type as Fido2RegistrationInputValue |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Address the TODO comments for type declarations.
The TODO comments indicate that the mockInputValue variables should be explicitly typed as Fido2RegistrationInputValue and Fido2AuthenticationInputValue respectively. This would improve type safety and make the test intentions clearer.
Apply this diff to add explicit type declarations:
- // todo: declare inputValue type as Fido2RegistrationInputValue
- const mockInputValue = {
+ const mockInputValue: Fido2RegistrationInputValue = {
attestationValue: {And for the authentication test:
- // todo: declare inputValue type as Fido2AuthenticationInputValue
- const mockInputValue = {
+ const mockInputValue: Fido2AuthenticationInputValue = {
assertionValue: {Also applies to: 1032-1032
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/node.reducer.test.ts around lines 927 and
1032, the test TODOs require explicit typing for the mock input values: change
the declaration at line 927 to declare mockInputValue as
Fido2RegistrationInputValue and the declaration at line 1032 to declare
mockInputValue as Fido2AuthenticationInputValue; ensure those types are imported
from their source (or available in scope) and adjust any object literal fields
to satisfy the type if the compiler surfaces errors.
c1ae746 to
a9cc3db
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We've restored the optional data parameter to the returnProtectCollector function and updated the node reducer to pass prefilled data when available. This ensures Protect collectors can be initialized with existing data from formData, maintaining compatibility with the existing Protect SDK integration.
We verified this fix by re-running @forgerock/davinci-suites:e2e-ci--src/protect.test.ts.
Suggested Fix changes
diff --git a/packages/davinci-client/src/lib/collector.utils.ts b/packages/davinci-client/src/lib/collector.utils.ts
index 6865b44..533f919 100644
--- a/packages/davinci-client/src/lib/collector.utils.ts
+++ b/packages/davinci-client/src/lib/collector.utils.ts
@@ -434,10 +434,15 @@ export function returnSingleSelectCollector(field: SingleSelectField, idx: numbe
* @function returnProtectCollector - Creates a ProtectCollector object based on the provided field and index.
* @param {DaVinciField} field - The field object containing key, label, type, and links.
* @param {number} idx - The index to be used in the id of the ProtectCollector.
+ * @param {string} [data] - Optional data to prefill the collector with.
* @returns {ProtectCollector} The constructed ProtectCollector object.
*/
-export function returnProtectCollector(field: ProtectField, idx: number) {
- return returnSingleValueAutoCollector(field, idx, 'ProtectCollector');
+export function returnProtectCollector(field: ProtectField, idx: number, data?: string) {
+ const collector = returnSingleValueAutoCollector(field, idx, 'ProtectCollector');
+ if (data) {
+ collector.input.value = data;
+ }
+ return collector;
}
/**
diff --git a/packages/davinci-client/src/lib/node.reducer.ts b/packages/davinci-client/src/lib/node.reducer.ts
index 34ed242..36a8909 100644
--- a/packages/davinci-client/src/lib/node.reducer.ts
+++ b/packages/davinci-client/src/lib/node.reducer.ts
@@ -177,7 +177,8 @@ export const nodeCollectorReducer = createReducer(initialCollectorValues, (build
return returnSubmitCollector(field, idx);
}
case 'PROTECT': {
- return returnProtectCollector(field, idx);
+ const str = data as string;
+ return returnProtectCollector(field, idx, str);
}
case 'FIDO2': {
if (field.action === 'REGISTER') {
Or Apply changes locally with:
npx nx-cloud apply-locally 6YAS-Jj8r
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 To learn more about Self Healing CI, please visit nx.dev
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
a9cc3db to
1a29b13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/davinci-client/src/lib/collector.utils.ts (2)
326-401: Validation assignment pattern is inconsistent with existing collectors.Lines 365 and 388 assign
nullwhenvalidationArrayis empty, which differs from the original pattern in this file where other collectors useundefined(e.g., lines 525, 652 in the original code). This inconsistency was already noted in previous review discussions.
507-507: Validation assignment changed fromundefinedtonull.This line previously used
undefinedwhenvalidationArraywas empty. The change tonullaligns with the new ObjectValueAutoCollector pattern but creates inconsistency with this function's previous behavior. This validation assignment inconsistency is already under discussion in previous review comments.
🧹 Nitpick comments (3)
packages/davinci-client/src/lib/node.reducer.test.ts (2)
925-1028: Good FidoRegistrationCollector test coverage.The test suite properly validates the FIDO2 registration flow with appropriate mock data. The test structure follows existing patterns and correctly validates the ObjectValueAutoCollector category and credential creation options.
Consider adding explicit type annotations to the mockInputValue for improved type safety and IDE support:
- // todo: declare inputValue type as Fido2RegistrationInputValue - const mockInputValue = { + const mockInputValue: FidoRegistrationInputValue = { attestationValue: {Note: This addresses the TODO comment and improves type checking.
1030-1117: Thorough FidoAuthenticationCollector test coverage.The test suite appropriately validates the FIDO2 authentication flow with proper mock assertion data and correctly validates the ObjectValueAutoCollector category and credential request options.
Consider adding explicit type annotation to the mockInputValue for improved type safety:
- // todo: declare inputValue type as Fido2AuthenticationInputValue - const mockInputValue = { + const mockInputValue: FidoAuthenticationInputValue = { assertionValue: {Note: This addresses the TODO comment and improves type checking.
packages/davinci-client/src/lib/client.store.ts (1)
281-288: Consider extracting the value parameter union into a named type.The value parameter union now includes five distinct types. While functional, extracting this into a named type (e.g.,
CollectorInputValue) would improve readability and maintainability, especially if more input value types are added in the future.Example:
type CollectorInputValue = | string | string[] | PhoneNumberInputValue | FidoRegistrationInputValue | FidoAuthenticationInputValue;Then use:
- return function ( - value: - | string - | string[] - | PhoneNumberInputValue - | FidoRegistrationInputValue - | FidoAuthenticationInputValue, - index?: number, - ) { + return function (value: CollectorInputValue, index?: number) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/tired-melons-wish.md(1 hunks)packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.test-d.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(10 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(10 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(12 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)packages/journey-client/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/node.types.test-d.ts
- packages/davinci-client/src/lib/node.types.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/davinci-client/src/lib/davinci.utils.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)
packages/davinci-client/src/lib/client.store.ts (2)
packages/davinci-client/src/lib/client.store.utils.ts (1)
handleUpdateValidateError(51-66)packages/davinci-client/src/lib/collector.types.ts (7)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)SingleValueCollectors(174-179)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(439-441)returnFidoRegistrationCollector(449-451)returnFidoAuthenticationCollector(459-461)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)FidoRegistrationField(173-181)FidoAuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)FidoRegistrationField(173-181)FidoAuthenticationField(193-201)packages/davinci-client/src/lib/collector.utils.ts (2)
returnSingleValueAutoCollector(273-324)returnObjectValueAutoCollector(333-401)
packages/davinci-client/src/lib/node.reducer.test.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-171)FidoAuthenticationOptions(183-191)
⏰ 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 (24)
packages/journey-client/package.json (1)
36-36: Vite version mismatch confirmed: dependencies (6.3.6) vs devDependencies (6.3.4).The version inconsistency is verified. Line 36 sets vite to 6.3.6 in dependencies, while line 41 keeps it at 6.3.4 in devDependencies. This divergence between build and development environments should be resolved.
Align both to the same version:
"dependencies": { "@forgerock/sdk-logger": "workspace:*", "@forgerock/sdk-request-middleware": "workspace:*", "@forgerock/sdk-types": "workspace:*", "@forgerock/sdk-utilities": "workspace:*", "@forgerock/storage": "workspace:*", "@reduxjs/toolkit": "catalog:", "tslib": "^2.3.0", - "vite": "6.3.6", + "vite": "6.3.4", "vitest-canvas-mock": "^0.3.3" },Or update devDependencies to 6.3.6 to match dependencies. Confirm your intent—is this split deliberate or unintentional?
packages/davinci-client/src/lib/collector.types.test-d.ts (1)
305-305: LGTM! Type test correctly reflects nullable validation.The addition of
validation: nullin the type test correctly validates that theMultiSelectCollectortype allows null validation, consistent with the updated type definitions incollector.types.ts.packages/davinci-client/src/lib/davinci.types.ts (2)
163-201: Well-structured FIDO2 type definitions.The new
FidoRegistrationOptions,FidoAuthenticationOptions,FidoRegistrationField, andFidoAuthenticationFieldtypes are well-designed and align with the WebAuthn specification. The interfaces correctly extend the standard PublicKey credential types with appropriate customizations for the DaVinci client.
212-218: Good expansion of SingleValueFields union.The inclusion of
FidoRegistrationFieldandFidoAuthenticationFieldin theSingleValueFieldsunion correctly integrates the new FIDO2 field types into the existing type hierarchy.packages/davinci-client/src/lib/collector.types.ts (2)
305-311: Clean FIDO2 input value interfaces.The
FidoRegistrationInputValueandFidoAuthenticationInputValueinterfaces are appropriately simple and correctly use optional properties for the WebAuthn credential values.
547-630: Comprehensive AutoCollector type system expansion.The expansion of the AutoCollector type system to support object-value collectors is well-designed:
- Clear separation between
SingleValueAutoCollectorTypesandObjectValueAutoCollectorTypes- Proper generic parameter naming (
IVfor input value)- Complete type inference mapping
- Consistent union definitions
.changeset/tired-melons-wish.md (1)
1-5: Appropriate changeset for feature addition.The changeset correctly indicates a minor version bump for the new WebAuthn/FIDO2 collectors feature.
packages/davinci-client/src/lib/davinci.utils.ts (2)
25-30: Proper import of new FIDO2 input value types.The import statement correctly includes the new
FidoAuthenticationInputValueandFidoRegistrationInputValuetypes needed for the formData type union.
57-59: FormData union appropriately expanded.The formData type union now includes
FidoRegistrationInputValueandFidoAuthenticationInputValue, ensuring type safety when handling FIDO2 collector data in submit requests.packages/davinci-client/src/lib/node.reducer.test.ts (1)
867-923: Comprehensive ProtectCollector test coverage.The new test suite for
ProtectCollectorproperly validates the update flow, including the correct handling of theSingleValueAutoCollectorcategory and the config output structure.packages/davinci-client/src/lib/collector.utils.test.ts (3)
812-842: Solid test coverage for ProtectCollector utility.The test properly validates that
returnSingleValueAutoCollectorcreates a correctProtectCollectorwith the appropriate category, config output, and empty input value.
844-912: Comprehensive FidoRegistrationCollector test.The test thoroughly validates the
returnObjectValueAutoCollectorfunction for FIDO2 registration, including:
- Correct ObjectValueAutoCollector category
- Proper handling of required validation
- Complete publicKeyCredentialCreationOptions in config
- Empty object as initial input value
914-959: Well-structured FidoAuthenticationCollector test.The test appropriately validates FIDO2 authentication collector creation with:
- Correct ObjectValueAutoCollector category
- Null validation when not required
- Complete publicKeyCredentialRequestOptions in config
- Proper action and trigger values
packages/davinci-client/src/lib/node.reducer.ts (3)
179-189: Clean FIDO2 collector routing logic.The new FIDO2 case correctly routes to registration or authentication collectors based on the
field.actionproperty. The use of the action property for routing (as discussed in past reviews) is more concise and self-documenting than checking for property presence.
300-324: Robust FIDO2 collector update handlers.The update handlers for
FidoRegistrationCollectorandFidoAuthenticationCollectorfollow the established pattern with:
- Proper type guards and validation
- Clear, specific error messages
- Correct property validation (attestationValue for registration, assertionValue for authentication)
- Direct assignment of validated input values
228-228: Correct type check for ProtectCollector.The change from checking
SingleValueAutoCollectorcategory to checkingProtectCollectortype is appropriate, as it provides more specific type narrowing for the Protect-specific update logic.packages/davinci-client/src/lib/client.store.ts (3)
37-38: LGTM!The new FIDO2 input value type imports are necessary and correctly sourced.
309-314: LGTM!The validate method signature correctly extends to accept AutoCollectors, consistent with the update method changes.
338-349: LGTM!The validation category checks correctly include ObjectValueAutoCollector, and the error messaging is appropriately generic and consistent with the update method.
packages/davinci-client/src/lib/collector.utils.ts (5)
29-37: LGTM!The new imports for AutoCollector types and FIDO2 field types are necessary and correctly sourced.
267-324: LGTM!The function rename to
returnSingleValueAutoCollectorcorrectly reflects the narrowed type scope, and the removal of thedataparameter is appropriate for auto-collectors that don't support prefilled data.
439-441: LGTM!The refactored
returnProtectCollectorcorrectly delegates toreturnSingleValueAutoCollectorand the removal of thedataparameter is appropriate.
443-461: LGTM!The new FIDO2 collector wrapper functions follow the established pattern and correctly delegate to
returnObjectValueAutoCollectorwith appropriate type parameters.
571-606: LGTM!The addition of optional chaining for
field.optionsis a defensive improvement that prevents potential runtime errors. The nullish coalescing fallback to empty arrays is appropriate.
|
Updated naming |
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
1a29b13 to
31d3196
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/davinci-client/src/lib/collector.types.ts (1)
569-570: Avoid?+| nulloninput.validationfor AutoCollectorUse one absence sentinel and stay consistent with the rest of the file.
Apply:
- validation?: ValidationRequired[] | null; + validation: ValidationRequired[] | null;packages/davinci-client/src/lib/node.reducer.test.ts (2)
927-940: Type the registrationmockInputValueasFidoRegistrationInputValueRemoves the TODO and improves intent.
- // todo: declare inputValue type as FidoRegistrationInputValue - const mockInputValue = { + const mockInputValue: import('./collector.types.js').FidoRegistrationInputValue = { attestationValue: {
1032-1046: Type the authenticationmockInputValueasFidoAuthenticationInputValueRemoves the TODO and tightens coverage of shapes.
- // todo: declare inputValue type as FidoAuthenticationInputValue - const mockInputValue = { + const mockInputValue: import('./collector.types.js').FidoAuthenticationInputValue = { assertionValue: {
🧹 Nitpick comments (8)
.changeset/tired-melons-wish.md (1)
1-5: Clarify release notes with user-facing detailsAdd bullets listing new public types/collectors and any migration notes (e.g., new exports, stricter input.validation nullability).
Example:
- New: FidoRegistrationCollector, FidoAuthenticationCollector, ObjectValueAutoCollector
- New types: FidoRegistrationOptions, FidoAuthenticationOptions, FidoRegistrationInputValue, FidoAuthenticationInputValue
- Node reducer routes FIDO2 fields based on action (REGISTER/AUTHENTICATE)
packages/davinci-client/src/lib/node.reducer.test.ts (2)
21-22: Make type-only imports explicitPrevent accidental runtime imports by using
import type.-import { FidoAuthenticationOptions, FidoRegistrationOptions } from './davinci.types.js'; +import type { FidoAuthenticationOptions, FidoRegistrationOptions } from './davinci.types.js';
970-1002: Add negative-path assertions for FIDO updatesCover error branches when required properties are missing to bump patch coverage.
@@ }); }); describe('The node collector reducer with FidoRegistrationFieldValue', () => { @@ expect(nodeCollectorReducer(state, action)).toStrictEqual([ @@ ]); }); + + it('should throw if attestationValue is missing', () => { + const badAction = { + type: 'node/update', + payload: { id: 'fido2-registration-0', value: {} }, + }; + const state: FidoRegistrationCollector[] = [{ + category: 'ObjectValueAutoCollector', + error: null, + type: 'FidoRegistrationCollector', + id: 'fido2-registration-0', + name: 'fido2-registration', + input: { key: 'fido2-registration', value: {}, type: 'FIDO2', validation: null }, + output: { key: 'fido2-registration', type: 'FIDO2', config: { publicKeyCredentialCreationOptions: { challenge: [], rp: { name: '' }, user: { id: [], name: '', displayName: '' }, pubKeyCredParams: [] }, action: 'REGISTER', trigger: 'BUTTON' } }, + }]; + expect(() => nodeCollectorReducer(state, badAction)).toThrow('attestationValue'); + }); @@ describe('The node collector reducer with FidoAuthenticationFieldValue', () => { @@ expect(nodeCollectorReducer(state, action)).toStrictEqual([ @@ ]); }); + + it('should throw if assertionValue is missing', () => { + const badAction = { + type: 'node/update', + payload: { id: 'fido2-authentication-0', value: {} }, + }; + const state: FidoAuthenticationCollector[] = [{ + category: 'ObjectValueAutoCollector', + error: null, + type: 'FidoAuthenticationCollector', + id: 'fido2-authentication-0', + name: 'fido2-authentication', + input: { key: 'fido2-authentication', value: {}, type: 'FIDO2', validation: null }, + output: { key: 'fido2-authentication', type: 'FIDO2', config: { publicKeyCredentialRequestOptions: { challenge: [] }, action: 'AUTHENTICATE', trigger: 'BUTTON' } }, + }]; + expect(() => nodeCollectorReducer(state, badAction)).toThrow('assertionValue'); + });Also applies to: 1092-1116
packages/davinci-client/src/lib/davinci.types.ts (1)
173-201: Constraintriggerand share FIDO field baseTighten types to catch mismatches early and reduce duplication.
-export type FidoRegistrationField = { +type FidoTrigger = 'BUTTON' | 'AUTO'; +type FidoAction = 'REGISTER' | 'AUTHENTICATE'; +type FidoFieldBase<A extends FidoAction> = { type: 'FIDO2'; key: string; label: string; - publicKeyCredentialCreationOptions: FidoRegistrationOptions; - action: 'REGISTER'; - trigger: string; + action: A; + trigger: FidoTrigger; required: boolean; }; + +export type FidoRegistrationField = FidoFieldBase<'REGISTER'> & { + publicKeyCredentialCreationOptions: FidoRegistrationOptions; +}; @@ -export type FidoAuthenticationField = { - type: 'FIDO2'; - key: string; - label: string; - publicKeyCredentialRequestOptions: FidoAuthenticationOptions; - action: 'AUTHENTICATE'; - trigger: string; - required: boolean; -}; +export type FidoAuthenticationField = FidoFieldBase<'AUTHENTICATE'> & { + publicKeyCredentialRequestOptions: FidoAuthenticationOptions; +};packages/davinci-client/src/lib/collector.utils.test.ts (2)
23-37: Use type-only imports for type annotationsPrevents emitting runtime imports for interfaces/types.
- returnObjectValueCollector, - returnSingleValueAutoCollector, - returnObjectValueAutoCollector, + returnObjectValueCollector, + returnSingleValueAutoCollector, + returnObjectValueAutoCollector, } from './collector.utils.js'; -import type { +import type { DaVinciField, DeviceAuthenticationField, DeviceRegistrationField, FidoAuthenticationField, FidoRegistrationField, PhoneNumberField, ProtectField, ReadOnlyField, RedirectField, StandardField, } from './davinci.types.js'; +// Keep collector.types imports as type-only too +import type { + MultiSelectCollector, + PhoneNumberCollector, + PhoneNumberOutputValue, + ValidatedTextCollector, +} from './collector.types.js';Also applies to: 26-44
844-912: Add error-case tests for auto collectorsCover missing
key/typeso the error aggregation paths are exercised.@@ describe('returnObjectValueAutoCollector', () => { it('should create a valid FidoRegistrationCollector', () => { @@ }); it('should create a valid FidoAuthenticationCollector', () => { @@ }); + + it('returns error when required field props are missing (REGISTER)', () => { + const badField = { action: 'REGISTER', trigger: 'BUTTON', required: true } as unknown as FidoRegistrationField; + const result = returnObjectValueAutoCollector(badField, 1, 'FidoRegistrationCollector'); + expect(result.error).toMatch(/Key is not found|Type is not found/); + }); + + it('returns error when required field props are missing (AUTHENTICATE)', () => { + const badField = { action: 'AUTHENTICATE', trigger: 'BUTTON', required: false } as unknown as FidoAuthenticationField; + const result = returnObjectValueAutoCollector(badField, 1, 'FidoAuthenticationCollector'); + expect(result.error).toMatch(/Key is not found|Type is not found/); + });Also applies to: 914-958
packages/davinci-client/src/lib/node.reducer.ts (1)
225-235: Consider consistency in type narrowing checks.Lines 226-227 check
collector.category, but line 228 checkscollector.type === 'ProtectCollector'. While this is functionally correct (ProtectCollector has category 'SingleValueAutoCollector'), mixing category and type checks in the same conditional chain can be confusing for maintainability.Consider whether all checks should use
collector.typefor consistency, or add a comment explaining why this specific type check is necessary here.packages/davinci-client/src/lib/collector.utils.ts (1)
326-401: Consider validation property consistency across collectors.The function correctly implements the object-value auto collector pattern for FIDO2 collectors. However, lines 365 and 388 use
nullfor empty validation arrays. Based on past review comments, there's been discussion about standardizing validation to eitherundefinedor omitting the property entirely when empty, rather than usingnull.While this uses the same pattern as the recently updated
returnMultiValueCollector(line 507) andreturnObjectCollector(line 634), you may want to confirm whether the team has decided to standardize onnullacross the codebase or use a different approach.Based on past review comments discussing validation consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.changeset/tired-melons-wish.md(1 hunks)package.json(1 hunks)packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.test-d.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(10 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(10 hunks)packages/davinci-client/src/lib/davinci.types.ts(3 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(12 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/davinci-client/src/lib/davinci.utils.ts
- packages/davinci-client/src/lib/collector.types.test-d.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/davinci-client/src/lib/node.types.ts
- packages/davinci-client/src/lib/node.types.test-d.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(439-441)returnFidoRegistrationCollector(449-451)returnFidoAuthenticationCollector(459-461)
packages/davinci-client/src/lib/node.reducer.test.ts (3)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-171)FidoAuthenticationOptions(183-191)
packages/davinci-client/src/lib/collector.utils.test.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)FidoRegistrationField(173-181)FidoAuthenticationField(193-201)packages/davinci-client/src/lib/collector.utils.ts (2)
returnSingleValueAutoCollector(273-324)returnObjectValueAutoCollector(333-401)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (1)
ProtectCollector(51-51)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)FidoRegistrationField(173-181)FidoAuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
⏰ 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 (11)
packages/davinci-client/src/lib/collector.types.ts (1)
305-311: DOM types are already properly configured in davinci-client—no action needed.The
PublicKeyCredentialtypes are correctly available. The package'stsconfig.lib.jsonexplicitly includes"lib": ["es2022", "dom", "dom.iterable"], which is the configuration used to build the library sources where these types are referenced. The review comment's concern has already been addressed in the codebase.Likely an incorrect or invalid review comment.
packages/davinci-client/src/lib/node.reducer.ts (4)
29-30: LGTM! Imports and type unions properly extended.The imports for FIDO collector utilities and types are correctly added, and the type unions (updateCollectorValues action payload and initialCollectorValues) are properly extended to include FIDO2 registration and authentication types.
Also applies to: 52-56, 70-76, 99-100
179-181: LGTM! Protect collector handling updated correctly.The removal of the
dataparameter fromreturnProtectCollectoris consistent with the function signature change in collector.utils.ts and aligns with the fact that Protect collectors don't have prefilled data.
182-189: LGTM! FIDO2 routing logic correctly implemented.The FIDO2 case properly discriminates between registration and authentication based on
field.action, as discussed in previous reviews. The logic correctly delegates to the appropriate collector factory function for each action type.
300-324: LGTM! FIDO validation logic follows established patterns.The validation blocks for
FidoRegistrationCollectorandFidoAuthenticationCollectorcorrectly follow the same pattern asPhoneNumberCollector, with appropriate type checks and property validation forattestationValueandassertionValuerespectively.packages/davinci-client/src/lib/collector.utils.ts (6)
29-31: LGTM! Import additions are correct.The new type imports for FIDO collectors and auto-collector types are properly structured and align with the new functionality being added.
Also applies to: 36-37
267-324: LGTM! Function refactored appropriately for single-value auto collectors.The rename from
returnAutoCollectortoreturnSingleValueAutoCollectorand the removal of thedataparameter correctly reflect the separation of concerns between single-value and object-value auto collectors. The field type is properly narrowed toProtectFieldonly.
439-441: LGTM! Protect collector simplified appropriately.The removal of the
dataparameter and delegation toreturnSingleValueAutoCollectorcorrectly reflects the refactoring and the fact that Protect collectors don't have prefilled data.
453-461: LGTM! FIDO authentication collector wrapper correctly implemented.The wrapper function properly delegates to
returnObjectValueAutoCollectorwith the correct collector type and has accurate JSDoc documentation.
716-751: LGTM! Validator correctly extended for auto collectors.The function signature properly extends to include
AutoCollectors, and the value parameter typeRecord<string, unknown>appropriately handles object values from FIDO collectors. The validation logic correctly handles strings, arrays, and objects.
571-606: LGTM! Code clarity improvements.The addition of optional chaining and nullish coalescing operators (
field.options?.find,field.options?.map,?? []) improves null safety and makes the code more robust against potential undefined values.
| export type AutoCollectors = | ||
| | ProtectCollector | ||
| | FidoRegistrationCollector | ||
| | FidoAuthenticationCollector | ||
| | SingleValueAutoCollector | ||
| | ObjectValueAutoCollector; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Public re-exports check for new AutoCollectors
Verify the package’s public barrel (src/types.ts or equivalent) re-exports FidoRegistrationCollector, FidoAuthenticationCollector, and ObjectValueAutoCollector so app consumers don’t import from internal paths.
Also applies to: 618-630
🏁 Script executed:
#!/bin/bash
# Look for public re-exports of FIDO collectors
rg -nP --glob "packages/davinci-client/src/types.ts" \
-C2 "Fido(Reg|Auth)enticationCollector|ObjectValueAutoCollector"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if types.ts exists at the specified path
if [ -f "packages/davinci-client/src/types.ts" ]; then
echo "=== types.ts EXISTS ==="
echo "First 50 lines:"
head -50 "packages/davinci-client/src/types.ts"
else
echo "=== types.ts NOT FOUND at packages/davinci-client/src/types.ts ==="
echo "Searching for public barrel files:"
fd -e ts -e js "types|index|barrel" "packages/davinci-client/src" | grep -E "(types|index|barrel)" | head -20
fiLength of output: 2514
🏁 Script executed:
#!/bin/bash
# Find package.json and check export configuration
if [ -f "packages/davinci-client/package.json" ]; then
echo "=== package.json exports field ==="
jq '.exports // "No exports field"' "packages/davinci-client/package.json"
fiLength of output: 105
🏁 Script executed:
#!/bin/bash
# Search for where AutoCollectors or the new collector types are exported
echo "=== Searching for re-exports of new collectors ==="
rg -nP "export.*?(FidoRegistration|FidoAuthentication|ObjectValueAutoCollector|AutoCollectors)" "packages/davinci-client/src" -C2Length of output: 6563
🏁 Script executed:
#!/bin/bash
# View complete types.ts to see all exports
wc -l "packages/davinci-client/src/types.ts"
echo "=== Last 100 lines of types.ts ==="
tail -100 "packages/davinci-client/src/types.ts"Length of output: 2711
🏁 Script executed:
#!/bin/bash
# Explicitly search for AutoCollectors export in types.ts
echo "=== Checking for AutoCollectors in types.ts ==="
grep -n "AutoCollectors\|FidoRegistrationCollector\|FidoAuthenticationCollector\|ObjectValueAutoCollector" "packages/davinci-client/src/types.ts" || echo "No matches found"Length of output: 64
Missing public re-exports for new AutoCollector types
The new collector types (FidoRegistrationCollector, FidoAuthenticationCollector, ObjectValueAutoCollector, and the AutoCollectors union) defined in collector.types.ts (lines 604–610) are not re-exported in the public barrel file (packages/davinci-client/src/types.ts).
App consumers will be forced to import directly from internal paths instead of the public API. Add these exports to types.ts following the existing pattern (e.g., export type FidoRegistrationCollector = collectors.FidoRegistrationCollector;).
🤖 Prompt for AI Agents
In packages/davinci-client/src/types.ts (add near the other collector
re-exports), export the new types from the internal collectors namespace: add
type re-exports for FidoRegistrationCollector, FidoAuthenticationCollector,
ObjectValueAutoCollector, and the AutoCollectors union following the existing
pattern (e.g., export type FidoRegistrationCollector =
collectors.FidoRegistrationCollector;). Ensure the file imports or references
the collectors namespace already used by other collector re-exports and place
the new lines next to those to keep public API consistent.
| export type SingleValueFields = | ||
| | StandardField | ||
| | ValidatedField | ||
| | SingleSelectField | ||
| | ProtectField | ||
| | FidoRegistrationField | ||
| | FidoAuthenticationField; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Confirm public unions are exported from the package barrel
You added FIDO fields to SingleValueFields. Ensure these are surfaced via the package’s top-level exports for consumers.
🏁 Script executed:
#!/bin/bash
rg -nP "export\s+type\s+.*Fido(Authentication|Registration)Field" packages/davinci-client/srcLength of output: 183
I need to verify if these types are re-exported from the package's main barrel file. Let me check the package's top-level exports:
🏁 Script executed:
#!/bin/bash
# Find the package barrel file (typically index.ts at root of src)
fd -name "index.ts" packages/davinci-client/src -type f | head -5Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check what's exported from the barrel
cat packages/davinci-client/src/index.ts 2>/dev/null || echo "File not found at that location"Length of output: 271
🏁 Script executed:
#!/bin/bash
# Search for re-exports of SingleValueFields to see the pattern
rg -nP "export.*SingleValueFields" packages/davinci-client/srcLength of output: 84
🏁 Script executed:
#!/bin/bash
# Search for re-exports of FIDO types
rg -nP "export.*Fido(Authentication|Registration)Field" packages/davinci-client/srcLength of output: 483
🏁 Script executed:
#!/bin/bash
# Check package.json main entry
cat packages/davinci-client/package.json | grep -A2 -B2 '"main"'Length of output: 149
🏁 Script executed:
#!/bin/bash
# List files in packages/davinci-client/src to understand structure
ls -la packages/davinci-client/src/Length of output: 345
I see there's a separate types.ts file at the package root. Let me check if that's where public types are exported:
🏁 Script executed:
#!/bin/bash
# Check the types.ts file at root of src
cat packages/davinci-client/src/types.tsLength of output: 2636
🏁 Script executed:
#!/bin/bash
# Check if FIDO types are mentioned anywhere in types.ts or related files
rg -n "SingleValueFields|FidoRegistrationField|FidoAuthenticationField" packages/davinci-client/src/types.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# See what imports are in types.ts
head -30 packages/davinci-client/src/types.tsLength of output: 1357
🏁 Script executed:
#!/bin/bash
# Check package.json exports configuration
cat packages/davinci-client/package.json | jq '.exports'Length of output: 70
Let me check if FIDO collector types exist and what pattern is used for collectors:
🏁 Script executed:
#!/bin/bash
# Search collector.types.js for FIDO types
rg -n "FidoRegistration|FidoAuthentication" packages/davinci-client/src/lib/collector.types.tsLength of output: 678
🏁 Script executed:
#!/bin/bash
# Check what's exported from collector.types.ts
rg -n "^export type" packages/davinci-client/src/lib/collector.types.ts | head -20Length of output: 1477
🏁 Script executed:
#!/bin/bash
# Look for all collector type exports to understand the pattern
rg -n "Collector" packages/davinci-client/src/lib/collector.types.ts | grep "export type"Length of output: 3101
🏁 Script executed:
#!/bin/bash
# Check if davinci.types exports SingleValueFields or FIDO types
rg -n "^export" packages/davinci-client/src/lib/davinci.types.tsLength of output: 1622
Add missing FIDO types to public API exports in src/types.ts
The review comment is correct. FidoRegistrationCollector and FidoAuthenticationCollector are defined in collector.types.ts and exported locally, but they're not re-exported from the package's public types barrel at src/types.ts. The existing pattern shows other collector types (DeviceRegistrationCollector, DeviceAuthenticationCollector, etc.) are surfaced via src/types.ts, but the FIDO collectors are missing from this public API. Additionally, SingleValueFields itself is not exported, though it's now part of the public schema.
Add the following exports to packages/davinci-client/src/types.ts:
FidoRegistrationCollectorFidoAuthenticationCollectorSingleValueFields
🤖 Prompt for AI Agents
In packages/davinci-client/src/types.ts (and note SingleValueFields is defined
in packages/davinci-client/src/lib/davinci.types.ts around lines 212-219), the
public types barrel is missing exports for FidoRegistrationCollector,
FidoAuthenticationCollector and SingleValueFields; update src/types.ts to
re-export FidoRegistrationCollector and FidoAuthenticationCollector from
collector.types.ts and add an export for SingleValueFields (ensuring the symbol
is exported from davinci.types.ts if not already), so these types are surfaced
by the package public API.
ancheetah
force-pushed
the
SDKS-4372-webauthn-collectors
branch
from
31d3196 to
b327c79
Compare
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 (2)
packages/davinci-client/src/lib/davinci.utils.ts (1)
41-48: Bug: ObjectValueAutoCollector is filtered out of submit.
FIDO collectors won’t be included in formData. Include ObjectValueAutoCollector in the filter.Apply:
- const collectors = node.client?.collectors?.filter( + const collectors = node.client?.collectors?.filter( (collector) => collector.category === 'MultiValueCollector' || collector.category === 'SingleValueCollector' || collector.category === 'ValidatedSingleValueCollector' || collector.category === 'ObjectValueCollector' || - collector.category === 'SingleValueAutoCollector', + collector.category === 'SingleValueAutoCollector' || + collector.category === 'ObjectValueAutoCollector', );packages/davinci-client/src/lib/node.reducer.ts (1)
249-268: Fix validation: check value type, not id; correct message.
Current code validates id (always string) and errors mention “Index”. Validate the value is a string instead.- if (collector.type === 'DeviceAuthenticationCollector') { - if (typeof action.payload.id !== 'string') { - throw new Error('Index argument must be a string'); - } + if (collector.type === 'DeviceAuthenticationCollector') { + if (typeof action.payload.value !== 'string') { + throw new Error('Value 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, );And:
- if (collector.type === 'DeviceRegistrationCollector') { - if (typeof action.payload.id !== 'string') { - throw new Error('Index argument must be a string'); - } + if (collector.type === 'DeviceRegistrationCollector') { + if (typeof action.payload.value !== 'string') { + throw new Error('Value argument must be a string'); + }
♻️ Duplicate comments (2)
packages/davinci-client/src/lib/node.reducer.test.ts (2)
925-1028: Type TODOs for FIDO registration test; add negative-case.
Explicitly type mockInputValue as FidoRegistrationInputValue and add a test where attestationValue is missing to assert thrown error.
Apply:- // todo: declare inputValue type as FidoRegistrationInputValue - const mockInputValue = { + const mockInputValue: FidoRegistrationInputValue = { attestationValue: {And add a case expecting 'Value argument must contain an attestationValue property'.
1030-1117: Type TODOs for FIDO authentication test; add negative-case.
Explicitly type mockInputValue as FidoAuthenticationInputValue and add a test where assertionValue is missing to assert thrown error.
Apply:- // todo: declare inputValue type as FidoAuthenticationInputValue - const mockInputValue = { + const mockInputValue: FidoAuthenticationInputValue = { assertionValue: {
🧹 Nitpick comments (5)
packages/davinci-client/src/lib/node.reducer.test.ts (1)
867-923: LGTM: Protect collector update test added. Covers SingleValueAutoCollector path. Consider adding an invalid (non-string) value case to assert error.packages/davinci-client/src/lib/node.reducer.ts (1)
300-324: LGTM: FIDO update guards are strict and correct. Presence checks for attestationValue/assertionValue look good. Consider asserting non-null values of underlying ArrayBuffers in future tests.packages/davinci-client/src/lib/davinci.types.ts (1)
152-154: LGTM: FIDO fields/options added and ComplexValueFields updated.
- PhoneNumberField additions look fine.
- FIDO option shapes match WebAuthn (allowCredentials corrected to array).
Optional: re-export FidoRegistrationOptions/FidoAuthenticationOptions (and Fido*Field) from src/types.ts for a smoother public API.Also applies to: 163-201, 206-215
packages/davinci-client/src/lib/collector.types.ts (1)
555-569: Validation field inconsistency in AutoCollector interface.While other collector interfaces were updated to use explicit nullability (
validation: ValidationRequired[] | null), theAutoCollectorinterface still uses the optional operator (validation?: ValidationRequired[] | null), creating a triple state (undefined, null, or array).For consistency with lines 205, 226, 326, 350, and 375, consider:
input: { key: string; value: IV; type: string; - validation?: ValidationRequired[] | null; + validation: ValidationRequired[] | null; };Note: If undefined/omission has semantic meaning distinct from null for auto-collectors, document why this interface differs from the others.
packages/davinci-client/src/lib/collector.utils.ts (1)
718-751: Good extension of validator to support auto-collectors.Adding
AutoCollectorsto the validator signature and updating the value parameter to acceptRecord<string, unknown>correctly enables validation for FIDO and other object-value auto-collectors. The validation logic properly handles object values by checkingObject.keys(value).length.For improved type clarity, consider adding an explicit return type annotation to the validator function:
- return (value: string | string[] | Record<string, unknown>) => { + return (value: string | string[] | Record<string, unknown>): string[] => {Based on learnings (past review comment mentioned this at line 725).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.changeset/tired-melons-wish.md(1 hunks)package.json(1 hunks)packages/davinci-client/src/lib/client.store.ts(4 hunks)packages/davinci-client/src/lib/collector.types.test-d.ts(1 hunks)packages/davinci-client/src/lib/collector.types.ts(10 hunks)packages/davinci-client/src/lib/collector.utils.test.ts(8 hunks)packages/davinci-client/src/lib/collector.utils.ts(10 hunks)packages/davinci-client/src/lib/davinci.types.ts(2 hunks)packages/davinci-client/src/lib/davinci.utils.ts(2 hunks)packages/davinci-client/src/lib/node.reducer.test.ts(12 hunks)packages/davinci-client/src/lib/node.reducer.ts(8 hunks)packages/davinci-client/src/lib/node.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/node.types.ts(2 hunks)packages/davinci-client/src/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/davinci-client/src/lib/collector.utils.test.ts
- packages/davinci-client/src/lib/client.store.ts
🧰 Additional context used
🧬 Code graph analysis (8)
packages/davinci-client/src/lib/node.types.ts (2)
packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/types.ts (2)
FidoRegistrationCollector(52-52)FidoAuthenticationCollector(53-53)
packages/davinci-client/src/lib/node.types.test-d.ts (2)
packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/types.ts (2)
FidoRegistrationCollector(52-52)FidoAuthenticationCollector(53-53)
packages/davinci-client/src/lib/node.reducer.ts (2)
packages/davinci-client/src/lib/collector.types.ts (5)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/lib/collector.utils.ts (3)
returnProtectCollector(439-441)returnFidoRegistrationCollector(449-451)returnFidoAuthenticationCollector(459-461)
packages/davinci-client/src/lib/node.reducer.test.ts (4)
packages/davinci-client/src/lib/collector.types.ts (3)
ProtectCollector(578-582)FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)packages/davinci-client/src/types.ts (3)
ProtectCollector(51-51)FidoRegistrationCollector(52-52)FidoAuthenticationCollector(53-53)packages/davinci-client/src/lib/node.reducer.ts (1)
nodeCollectorReducer(107-326)packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-171)FidoAuthenticationOptions(183-191)
packages/davinci-client/src/lib/collector.types.ts (1)
packages/davinci-client/src/types.ts (3)
FidoRegistrationCollector(52-52)FidoAuthenticationCollector(53-53)ProtectCollector(51-51)
packages/davinci-client/src/types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationCollector(583-587)FidoAuthenticationCollector(588-592)
packages/davinci-client/src/lib/davinci.utils.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(295-298)FidoRegistrationInputValue(305-307)FidoAuthenticationInputValue(309-311)
packages/davinci-client/src/lib/collector.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (3)
ProtectField(156-161)FidoRegistrationField(173-181)FidoAuthenticationField(193-201)packages/davinci-client/src/lib/collector.types.ts (7)
SingleValueAutoCollectorTypes(548-548)ObjectValueAutoCollectorTypes(549-552)InferAutoCollectorType(618-630)ValidatedTextCollector(184-184)ObjectValueCollectors(396-401)MultiValueCollectors(250-252)AutoCollectors(604-609)
⏰ 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 (18)
.changeset/tired-melons-wish.md (1)
1-5: Changeset looks good!The changeset file is correctly formatted and addresses the TODO noted in the PR objectives. The minor version bump is appropriate for the addition of new FIDO2 collectors, and the description accurately reflects the changes.
package.json (1)
111-111: No concerns with the tslib dependency.The repositioning of
tslibat line 111 is a benign change with no functional impact. The version specification remains unchanged at^2.5.0.packages/davinci-client/src/lib/collector.types.test-d.ts (1)
305-306: LGTM: nullable validation reflected for MultiSelect input.
Matches widened typing (Validation[] | null).packages/davinci-client/src/lib/davinci.utils.ts (1)
52-60: Verify JSON-safe serialization for WebAuthn credentials.
FidoRegistrationInputValue/FidoAuthenticationInputValue carry PublicKeyCredential which contains ArrayBuffers. Ensure they’re converted (e.g., base64url) before placing in formData to avoid non-serializable payloads. If handled elsewhere, note where.
Would you like a small helper (publicKeyCredentialToJSON) wired in here or in the collectors?packages/davinci-client/src/types.ts (1)
52-53: LGTM: FIDO collector types exposed in public API.packages/davinci-client/src/lib/node.types.test-d.ts (1)
36-38: LGTM: Collectors union extended with FIDO types. Types align with collector additions.Also applies to: 237-239
packages/davinci-client/src/lib/node.types.ts (1)
26-28: LGTM: FIDO collectors included in Collectors union. Matches reducer behavior.Also applies to: 47-49
packages/davinci-client/src/lib/node.reducer.test.ts (2)
440-441: LGTM: MultiValueCollector input.validation set to null. Consistent with types.Also applies to: 475-476
526-527: LGTM: input.validation null for object/value collectors. Matches new input typing.Also applies to: 576-577, 635-635, 677-677, 743-743, 787-787, 827-827, 852-852
packages/davinci-client/src/lib/node.reducer.ts (1)
180-189: LGTM: FIDO route via action is clear and self-documenting. Good alignment with API guarantees.
Please confirm field.action is always REGISTER/AUTHENTICATE across backends.packages/davinci-client/src/lib/collector.types.ts (5)
305-311: LGTM! Well-designed FIDO2 input value interfaces.The interfaces correctly use WebAuthn's
PublicKeyCredentialtype and follow standard terminology (attestation for registration, assertion for authentication). Making the properties optional is appropriate since they're populated after the WebAuthn ceremony completes.
205-205: Good consistency improvement for validation fields.Making validation explicitly nullable (
validation: ValidationRequired[] | null) instead of optional removes ambiguity about whether absence is represented bynullorundefined. Line 375 correctly extends the validation union to includeValidationPhoneNumberfor phone number collectors.Also applies to: 226-226, 326-326, 350-350, 375-375
547-553: Well-structured auto-collector type hierarchy.The type hierarchy clearly separates categories (
SingleValueAutoCollectorvsObjectValueAutoCollector) from specific implementations, enabling proper type narrowing in utility functions.
583-602: LGTM! Properly defined FIDO collector type aliases.The type aliases correctly specialize
AutoCollectorwith appropriate categories, types, and input value interfaces. The FIDO collectors use their respective input value types (FidoRegistrationInputValue,FidoAuthenticationInputValue), while the genericObjectValueAutoCollectorappropriately defaults toRecord<string, unknown>.
604-609: LGTM! Complete auto-collector type system.The
AutoCollectorsunion correctly includes all five collector types, andInferAutoCollectorTypeprovides proper type narrowing following the same pattern as other collector inference utilities in this file.Also applies to: 618-630
packages/davinci-client/src/lib/collector.utils.ts (3)
267-324: Good refactoring - function properly specialized for single-value auto-collectors.Renaming to
returnSingleValueAutoCollectorand constraining the field type toProtectFieldmakes the function's scope clear. Removing thedataparameter is appropriate since auto-collectors don't receive prefilled data.
439-461: LGTM! Properly implemented collector wrapper functions.The wrapper functions follow the established pattern in this file, delegating to the appropriate generic functions with correct collector types. JSDoc is accurate and complete.
326-401: Rewrite not needed—original review comment is based on inaccurate information.Lines 365 and 388 in
returnObjectValueAutoCollectorare already consistent with each other, both using the same null ternary pattern:validationArray.length ? validationArray : null. The review comment's reference to lines 525 and 652 usingundefinedis incorrect—no such undefined patterns exist in the file, and the correct line numbers would be 507 and 634, which also use the same null pattern. Within the function under review (lines 326-401), validation handling is consistent. Any file-wide standardization of validation patterns is outside the scope of this function's changes.Likely an incorrect or invalid review comment.
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4372
Description
Adds FIDO2 registration and authentication collectors and unit tests
TODO: changeset?
Summary by CodeRabbit
New Features
Chores
Tests