feat: superposition dynamic fields#1590
Conversation
… feat-sdk-config-api
…itch-web into feat-sdk-config-api
🚫 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! 🙌 |
🚫 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! 🙌 |
🚫 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! 🙌 |
2 similar comments
🚫 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! 🙌 |
🚫 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! 🙌 |
🚫 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! 🙌 |
1 similar comment
🚫 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! 🙌 |
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
This PR introduces a major architectural change migrating dynamic field handling from Recoil-based state management to React Final Form (RFF). While the approach is sound, there are several blocking issues that must be addressed before merge.
Verdict: Request Changes
Findings
🚨 Silent-null onFailure pattern in EmailField.res
Line 14 uses switch paths->Array.get(0) with None => React.null fallback. If no paths are provided, the component silently renders nothing. This is a silent failure pattern that makes debugging difficult.
Add an explicit error or warning when paths array is empty.
| None => {
Js.Console.error("EmailField: No paths provided")
React.null
}
CryptoCurrencyNetworks.res useEffect
Line 46-50 has a React.useEffect that calls field.input.onChange(initialNetwork) but the dependency array only includes [initialNetwork]. If field reference changes (though unlikely with RFF), the effect may use stale closure.
Add field to dependency array or use a ref-stable field accessor.
country_code extraction in PhoneCountryCodeDropdownField.res
Line 54 extracts country code via String.split("#")->Array.get(1). This assumes a specific format that isn't validated. Malformed values will silently return empty string.
Add validation that the split produces expected parts, or use a typed parser.
CountryDropdownField.res
Lines 14-16 construct initialIso from defaultCountry or first option, but defaultCountry is already a country name while firstOption is presumably an ISO code. Mixing name and code types may cause initialization bugs.
Verify that defaultCountry and firstOption are both ISO codes before assigning to initialIso.
💡 Missing key in dynamic array rendering
DynamicFields.res line 159-167 maps rows without stable keys. Using row index as key causes React reconciliation issues when rows reorder.
Use a stable identifier from the field configuration (like confirmRequestWritePath) as the key.
💡 Unused inputValue state in CardHolderNameField.res
The inputValue state is set but never read outside the handleChange function. RFF already manages field state - this local state is redundant.
Remove the local state and derive display value from firstField.input.value if needed.
Architectural Concerns
-
No fallback for RFF context failures: The entire form assumes RFF context is present. If
ReactFinalForm.useFormis called outside a<Form>wrapper, it will throw at runtime without graceful degradation. -
Base64 encoding risk in flattenObject:
DynamicFields.resline 176 callsflattenObject(false)- verify this doesn't introduce injection vectors if field values contain special characters. -
V1/V2 parity: If
DynamicFields.reschanged, ensureDynamicFieldsV2.res(if exists) reflects similar RFF migration to maintain version parity.
Positive Notes
- Clean separation of concerns with new field components
- Proper use of RFF
useFieldhook with validation config - Good TypeScript-style typing with ReScript variants in
DynamicFieldInput.res - Significant code reduction in
DynamicFieldsUtils.res(-634 lines)
Please address the blocking issues above.
… feat-sdk-config-api Resolved conflicts: - APIUtils.res: kept FetchSdkConfigs route alongside main's renamed paymentIntentId - PaymentHelpers.res: dropped duplicate getCardEligibilityErrorText (moved to EligibilityHelpers on main), kept fetchSdkConfigs - Elements.res: kept forwardSdkConfigsDataToIframe with main's hasSdkAuthorization guard - Hyper.res: kept resolveProfileId and main's isReadyPromise; adopted main's direct clientSecret usage (no extraction from sdkAuthorization) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c5c380e to
cf3cdbc
Compare
1f71111 to
5349296
Compare
|
Replaced with PR: #1608 due to commit conflicts and unclear history |
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