Fix payment loading for custom payment pages #2901
Conversation
Magnusrm
changed the title
This reverts commit 74458bf.
|
Could it be added to the <OrderDetailsProvider>
{hasProcess ? (
<ProcessNavigationProvider>
<PaymentProvider>
<Provider value={undefined}>{children}</Provider>
</PaymentProvider>
</ProcessNavigationProvider>
) : (
<Provider value={undefined}>{children}</Provider>
)}
</OrderDetailsProvider> |
I did try this, but it did not work. The backend would get stuck and eventually fail with PDF generation failed. I did not manage to find out why 🤔 |
| <PresentationComponent type={ProcessTaskType.Data}> | ||
| <Form /> | ||
| </PresentationComponent> | ||
| <PaymentProvider> |
There was a problem hiding this comment.
This could probably be placed late in <FormProvider> instead, otherwise the Payment component will now crash the app if placed in:
- Stateless forms
- Custom receipts (!)
- Sub forms
None of these are probably likely use-cases, but it's better to validate that in the component than having the whole app crash because of it. FormProvider is 'the place' for all providers needed by form components, at least.
| export const PaymentContext = createContext<PaymentContextProps | undefined>(undefined); | ||
|
|
||
| export const PaymentProvider: React.FC<PaymentContextProvider> = ({ children }) => { | ||
| const [loading, setLoading] = useState<boolean>(false); |
There was a problem hiding this comment.
It's not clear to me why this has to be a separate state. Why not use busy directly, instead of setting loading when busy && !paymentError? Double state-keeping like this can lead to brittleness, as I've already seen in the busy state for that matter.. If this is loading should probably be a function of 'is the call to process/next running', which you can get from tanstack (see for example useIsSaving()).
| if (loading) { | ||
| return <Loader reason='Navigating to external payment solution' />; | ||
| } |
There was a problem hiding this comment.
There doesn't seem to be a way to get away from this loader, but that might be intentional? 🤔
| return <Loader reason='Navigating to external payment solution' />; | ||
| } | ||
|
|
||
| return <PaymentContext.Provider value={contextValue}>{children}</PaymentContext.Provider>; |
There was a problem hiding this comment.
I suspect this can lead to re-renders of all form components when anything in this provider changes. The contextValue object is at least not memoized, so this context will provide a new value every re-render. Have you checked if this affects anything when (for example) submitting a form (not necessarily a form with payment)? If useProcessNavigation() causes this to re-render because process navigation became busy, that might lead to expensive re-renders immediately when the submit button is pressed.
There was a problem hiding this comment.
Btw, these things are difficult to get right, which is why we made createZustandContext() and used that pattern in many of these global form-providers. That way you can set all the state you want, change it however you want, but never cause full-tree re-renders unless every component in the tree uses the state from zustand.
@Magnusrm What happens if you just load the instance with |
Description
This PR moves the "automatic" logic from the PaymentComponent into new context, "PaymentPayment".
This allows us to set a loading state that displays loading of the whole form, and not hust the payment component, while we are navigating to Nets or other payment providers and while we are navigating to the next process step after a payment is complete.
The payment provider seems to need to be placed within the PDFWrapper in order to be able to call
next()fromuseProcessNavigation.Related Issue(s)
Verification/QA
kind/*label to this PR for proper release notes grouping