feat: superposition dynamic fields#1608
Conversation
Shivam25092001
requested review from
AbhishekChorotiya,
ArushKapoorJuspay,
aritro2002,
sakksham7 and
seekshiva
as code owners
github-actions
Bot
added
the
S-conventions-not-followed
🚫 Missing Linked IssueHi 👋 This pull request does not appear to be linked to any open issue yet. Linking your PR to an issue helps keep the project tidy and ensures the issue is closed automatically. ✔️ How to fix this
Once linked, this check will pass automatically on your next push or when you re-run the workflow. Thanks for helping maintainers! 🙌 |
Code Review: Feat superposition dynamic fields cleanClassificationFeature: Superposition Dynamic Fields with React Final Form integration Architecture OverviewThis PR introduces React Final Form (RFF) for form state management in the dynamic fields system, significantly refactoring how form inputs handle state, validation, and submission.
Tier 1 — Payment Correctness & Safety✅ No Silent-Null onFailure PatternsChecked new field components — no error-swallowing patterns detected in the added/modified files. ✅ No Card Data in LogsNo
|
| Item | Status |
|---|---|
| Silent-null patterns | ✅ None found |
| Card data exposure | ✅ None found |
| 3DS impact | ✅ N/A |
| Bundle size documented | |
| PR description | |
| Client-core coordination | |
LoaderController risk |
Verdict:
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Classification: Major refactoring — Migrates DynamicFields from Recoil-based state to React Final Form (RFF)
Risk Level: Medium (large-scale form architecture change, ~2000 lines modified)
Scope: Dynamic field rendering system, form validation, state management
Findings Overview
| Severity | Count | Issues |
|---|---|---|
| 🚨 Critical | 0 | None identified |
| 2 | Empty PR description, missing test checklist items | |
| 💡 Suggestion | 2 | Bundle size impact, error boundary consideration |
⚠️ Warnings
1. Incomplete PR Description
The PR body has empty sections for Description and Testing. For a change of this magnitude (+187/-783 in DynamicFields.res alone), the description should document:
- Migration rationale from Recoil to RFF
- Backward compatibility plan for existing field types
- Risk assessment for form validation edge cases
2. Checklist Items Not Completed
The PR checklist shows these items unchecked:
- I ran
npm run re:build - I reviewed submitted code
- I added unit tests for my changes where possible
Please confirm npm run re:build passes and code has been reviewed. Unit tests for the new field components would strengthen confidence in this refactor.
💡 Suggestions
1. Monitor bundle size impact of react-final-form
The new dependency react-final-form (v7.0.0) brings in final-form (v5.0.1) as a peer dependency (~20KB minified). Verify this doesn't materially impact the SDK bundle size budget.
2. Consider error boundary for field component crashes
Individual field components now encapsulate more logic. Consider wrapping <DynamicFieldInput.makeRow /> calls in an error boundary to prevent a single field misconfiguration from crashing the entire payment form.
Tier 1 Verification Results
| Check | Status | Notes |
|---|---|---|
| Silent-null onFailure | ✅ Pass | No new error-swallowing patterns in diff |
| Bare silent catch | ✅ Pass | No catch blocks that collapse to null |
| Card data in logs | ✅ Pass | No console logging of sensitive fields |
| v1/v2 parity | ℹ️ N/A | Appears to be form-layer only, no PaymentBody changes |
| Endpoint routing | ✅ Pass | No new URL literals |
Tier 2 Verification Results
| Check | Status | Notes |
|---|---|---|
| Submodule pointer moves | ✅ Pass | No sdk-utils version changes |
| Promote to shared-utils | ✅ Pass | New components are web-specific (RFF-dependent) |
| Version sync | ℹ️ N/A | No version bump in package.json |
Positive Notes
- ✅ Clean separation of concerns with new
DynamicFields/subdirectory - ✅ Good use of RFF
useFieldhook pattern across all new components - ✅ Proper validation integration via
DynamicFieldsUtils.resolveValidator - ✅
GenericInputFielduses RFF<Field>render prop pattern correctly - ✅
DateOfBirthrefactored to integrate with RFF validation - ✅
LoaderControllerproperly guards sdkConfigsValue update behind error check
Pre-existing Issues (unchanged)
None introduced by this PR.
Overall Assessment:
This is a well-structured migration to React Final Form that should improve form state management consistency. The risk is moderate due to the large footprint. Completing the PR description and checklist would help reviewers understand the full scope.
Verdict: 💬 Comment — Clean refactor. Please complete PR description and checklist before merge.
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: 🔄 Request Changes
🚨 1 critical ·
This is a substantial architectural change introducing React Final Form across the dynamic fields system. The dependency addition and state management changes warrant careful review of error handling and backward compatibility.
Classification: core-flow-framework · secondary: sdk-ffi-codegen
Risk: high
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
react-final-form v7.0.0 pulls in final-form v5.0.1 as a peer. The RFF v7 line requires React 16.8+. Verify compatibility with the project's React 18.2.0 and ensure no duplicate final-form instances in the bundle.
Fix: Confirm npm ls final-form produces a single instance.
| @@ -12,6 +12,7 @@ | |||
| "react": "^18.2.0", | |||
There was a problem hiding this comment.
react-final-form@^7.0.0 - verify no duplicate final-form peer dependency in bundle.
| @@ -17,6 +17,7 @@ | |||
| "react": "^18.2.0", | |||
There was a problem hiding this comment.
💡 Ensure @babel/runtime@7.29.2 addition doesn't conflict with existing babel runtime versions.
Shivam25092001
changed the title
github-actions
Bot
removed
the
S-conventions-not-followed
| @@ -0,0 +1,24 @@ | |||
| open SuperpositionTypes | |||
|
|
|||
There was a problem hiding this comment.
merge this component to GenericInputField and use switch on fieldType to render
| open SuperpositionTypes | ||
|
|
||
| // "John Doe" -> ("John", "Doe") "John" -> ("John", "") | ||
| let splitName = (value: string): (string, string) => |
There was a problem hiding this comment.
check if we can use this function getFirstAndLastNameFromFullName instead of creating splitName
There was a problem hiding this comment.
"John Doe" (two spaces) would produce ("John", " Doe") with a leading space in the last name. This case will be missed by splitName
| stateField.input.onChange(effectiveCode) | ||
| } | ||
| None | ||
| }, [effectiveCode]) |
There was a problem hiding this comment.
Incomplete useEffect Dependency Array
| }, [effectiveCode]) | |
| }, [hasStates, storedCode, effectiveCode]) |
| let (name, setName) = React.useState(() => { | ||
| let first = firstProps.input.value->Option.getOr("") | ||
| let last = lastProps.input.value->Option.getOr("") | ||
| [first, last]->Array.filter(val => val !== "")->Array.join(" ") | ||
| }) |
There was a problem hiding this comment.
This only runs ONCE at mount. If firstProps/lastProps aren't populated yet (e.g. async form init, or values come in after mount), this will be "" forever.
| | _ => () | ||
| } | ||
| setSdkConfigs(_ => updatedState) | ||
| if !isSdkConfigsError { |
There was a problem hiding this comment.
why is there need of isSdkConfigsError, if no response comes, we can set the default value, right?
|
|
||
| let validate = DynamicFieldsUtils.resolveValidator(~field=fieldConfig, ~localeObject=localeString) | ||
|
|
||
| let field = ReactFinalForm.useField(path, ~config={validate: validate}) |
There was a problem hiding this comment.
use <ReactFinalForm.Field> instead of useField
| allCardHolderNameFields->Array.get(0)->Option.map(field => field.confirmRequestWritePath) | ||
| <RenderIf condition={firstCardHolderNamePath === Some(field.confirmRequestWritePath)}> | ||
| <CardHolderNameField fields=allCardHolderNameFields /> | ||
| </RenderIf> |
There was a problem hiding this comment.
render a single component here and move the rendering logic inside the react component
| CardHolderName => <CardHolderNameField ... />
There was a problem hiding this comment.
do same for all other cases
| value | ||
| onChange={ev => { | ||
| let val = ReactEvent.Form.target(ev)["value"] | ||
| fields->Array.forEach(field => form.change(field.confirmRequestWritePath, val)) |
There was a problem hiding this comment.
primaryField is not mapped with onChange, update primaryField value here so that ReactFinalForms can properly track field state
There was a problem hiding this comment.
<PaymentInputField
fieldName={label}
value
onChange={ev => {
let val: string = ReactEvent.Form.target(ev)["value"]
// Drive the registered field through its own handler...
primaryField.input.onChange(val)
secondaryPaths->Array.forEach(path => form.change(path, val))
}}
onBlur={_ev => primaryField.input.onBlur()}
isValid
errorString
placeholder
inputRef={fieldRef}
autocomplete
/>
| ->Option.getOr(Dict.make()) | ||
| ->getArray("countries") | ||
|
|
||
| let phoneNumberCodeOptions: array<DropdownField.optionType> = countryAndCodeList->Array.reduce( |
There was a problem hiding this comment.
this value is getting recomputed on every render, please use useMemo here
| let {label} = DynamicFieldsUtils.resolveFieldTexts(~field=fieldConfig, ~localeObject=localeString) | ||
| let validate = DynamicFieldsUtils.resolveValidator(~field=fieldConfig, ~localeObject=localeString) | ||
|
|
||
| let countryAndCodeList = |
4 participants
Type of Change
Description
DynamicFields.res— Rewritten to useuseConfigurationService→getSuperpositionFinalFields; RFF<Form>wraps all fields; billing split is path-based; card fields filtered at source to avoid double-render with CardPaymentDynamicFields/— New per-field components:CardHolderNameField,StateDropdownField,EmailField,CountryDropdownField,PhoneField, etc.DateOfBirth.res— Refactored as RFF-wired DateField; this component was only getting used byDynamicFieldspackage.json— Addedreact-final-form.ReactFinalFormowns all field state, validation, and initial values.Architectural Decisions
paymentMethodsFieldsenum pipeline.setRequiredFieldsBodycontract with all consumers is unchanged — they still receiveDict.t<JSON.t>keyed by API paths.useFormStateHandlersyncsformProps.values→setRequiredFieldsBodyandformProps.valid→areRequiredFieldsValid.Why include RFF change in this PR?
fieldConfigsfrom superposition becomes very readable and easier to manage.References
Base PRs
Stacked PRs
How did you test it?
SCREEN RECORDING
Screen.Recording.2026-05-26.at.12.54.06.PM.mov
Issue
Fixes #1518
Checklist
npm run re:build