Conversation
Shivam25092001
requested review from
AbhishekChorotiya,
ArushKapoorJuspay,
aritro2002,
sakksham7 and
seekshiva
as code owners
Shivam25092001
force-pushed
the
feat/superposition-b
branch
from
27cb975 to
36b3495
Compare
🔍 Code Review by opencodeStatus: 🚨 Critical Issues (Must Fix)1. React Hooks Violations (ESLint Errors)
Why critical: Breaks React's Rules of Hooks, causes undefined behavior. 2. Unused Open Statements (Build Warnings)
|
| File | Issue | Location |
|---|---|---|
| DynamicFieldsUtils.res | Unused match case _ => "unknown" | Line 744 |
✅ Verified Good
- Build passes (713 files, 0 errors)
- No debug logs
- No Belt modules
- Proper validation patterns
- Correct shared-code integration
Required Fixes
1. ReactFinalFormField.res (Lines 12-15):
// Change from:
let field = switch validationRule {
| Some(rule) => ReactFinalForm.useField(name, ~config={validate: createValidator(rule)})
| None => ReactFinalForm.useField(name)
}
// To:
let config = validationRule->Option.map(rule => {validate: createValidator(rule)})
let field = ReactFinalForm.useField(name, ~config?)2. DynamicFields.res (Lines 543, 649, 680):
Move useRef calls to top of component, pass refs to JSX instead of calling inside render.
3. Remove unused open Utils from:
- VpaIdPaymentInput.res:2
- EmailPaymentInput.res:2
Verification Commands
npm run re:build # Must pass
npm run test:hooks # Must show 0 errorsOnce fixed, this PR is ready for approval.
Reviewed by opencode AI using hyperswitch-web skill guidelines
| ~localeObject=localeString->Obj.magic, | ||
| ) | ||
|
|
||
| let line1Field: ReactFinalForm.Field.fieldProps = ReactFinalForm.useField( |
There was a problem hiding this comment.
line1Field: ReactFinalForm.Field.fieldProps, no need to add such types
|
|
||
| let formatBSB = bsb => { | ||
| let formatted = bsb->String.replaceRegExp(%re("/\D+/g"), "") | ||
| let formatted = bsb->String.replaceRegExp(%re("/\\D+/g"), "") |
There was a problem hiding this comment.
reason for this change?
| fieldName="Blik code" | ||
| setValue={setblikCode} | ||
| value=blikCode | ||
| setValue={_ => ()} |
There was a problem hiding this comment.
I see many places where you’ve added _ => (). If it’s the same everywhere, just remove setValue if it’s not required.
| let setDocumentNumber = Recoil.useSetRecoilState(RecoilAtoms.userDocumentNumber) | ||
|
|
||
| let pixCNPJ = Recoil.useRecoilValueFromAtom(userPixCNPJ) | ||
| let pixCPF = Recoil.useRecoilValueFromAtom(userPixCPF) | ||
|
|
||
| React.useEffect(() => { | ||
| switch documentType { | ||
| | "cpf" => setDocumentNumber(_ => pixCPF) | ||
| | "cnpj" => setDocumentNumber(_ => pixCNPJ) | ||
| | _ => setDocumentNumber(_ => RecoilAtoms.defaultFieldValues) | ||
| } | ||
| None | ||
| }, (documentType, pixCNPJ, pixCPF)) |
There was a problem hiding this comment.
this was a special case for pix,
refer to #1358,
check of it's working.
|
|
||
| @react.component | ||
| let make = () => { | ||
| let make = (~name="email") => { |
There was a problem hiding this comment.
I noticed that in many files you’ve added a name parameter with a default value (e.g., giftCardNumber) in components like GiftCardNumberInput, EmailPaymentInput, etc.
I don’t think consumers will need to pass a different name for these inputs. Do we really need to expose the name parameter here?
|
@Shivam25092001 , could you please pull the latest changes from main and resolve the merge conflicts? There have been quite a lot of updates. Also, I noticed that you've used fc in several places—please use the full form instead. Additionally, there are many places where types have been explicitly added during declaration; since these can be inferred automatically, they aren’t necessary. |
🚫 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! 🙌 |
3 participants
Type of Change
Description
Superposition in web
shared-codePR: Fix superposition hyperswitch-sdk-utils#46shared-codebranch:fix-superposition-xHow did you test it?
TESTING MATRIX
Checklist
npm run re:build