Skip to content

move: Cancel long running promises on react-router navigation#155

Merged
lukaw3d merged 4 commits intomasterfrom
lw/cancel-on-navigate
Feb 25, 2025
Merged

move: Cancel long running promises on react-router navigation#155
lukaw3d merged 4 commits intomasterfrom
lw/cancel-on-navigate

Conversation

@lukaw3d
Copy link
Contributor

@lukaw3d lukaw3d commented Jan 13, 2025

Before 8576711 we relied on full reloads when navigating to cancel the deposit/withdraw long running promises. After adding react-router we need to at least cancel repeating the flows (we don't need to cancel a started flow).

@lukaw3d lukaw3d requested a review from lubej January 13, 2025 19:02
@lukaw3d lukaw3d mentioned this pull request Jan 13, 2025
@lukaw3d lukaw3d self-assigned this Jan 29, 2025
@lukaw3d lukaw3d force-pushed the lw/cancel-on-navigate branch from 349aee3 to 76ee509 Compare February 18, 2025 10:10
@github-actions
Copy link

github-actions bot commented Feb 18, 2025

Deployed to Cloudflare Pages

Latest commit: 93df51796780f9a2dc5583dca0b425ff928b6820
Status:✅ Deploy successful!
Preview URL: https://29c0bc7d.rose-app.pages.dev
Alias: https://pr-155.rose-app.pages.dev

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

async function step3(consensusAccount: ConsensusAccount, sapphireAddress: `0x${string}`) {
// Note: don't use outside state vars. They are outdated.
try {
await new Promise(r => setTimeout(r, 1000)) // Handle React StrictMode: step3 is called by useEffect on mount
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by StrictMode? I don't see it included in the code.
I guess this issue comes from different context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:O we don't have React.StrictMode in this repo?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so. I personally remove it, because of double renders, and it is just confusing when you are debugging. And if there is an issue, I manually add it back, for the time being.

lukaw3d added 4 commits February 25, 2025 23:10
Before 8576711 we relied on full reloads when
navigating to cancel the deposit/withdraw long running promises. After adding
react-router we need to at least cancel repeating the flows (we don't need to
cancel a started flow).
Before, useUnmountSignal would trigger when App.tsx was unmounted. That's
unintuitive but worked because there's no way to switch between deposit and
withdraw without destroying App.
After: Parts of useDeposit that were needed for App.tsx are now split away, and
step3 is auto-run when useDeposit gets mounted (in Deposit.tsx). And
useUnmountSignal triggers when Deposit is unmounted.
Before, useUnmountSignal would trigger when App.tsx was unmounted. That's
unintuitive but worked because there's no way to switch between deposit and
withdraw without destroying App.
After: Parts of useWithdraw that were needed for App.tsx are now split away.
useUnmountSignal triggers when Withdraw is unmounted.
@lukaw3d lukaw3d force-pushed the lw/cancel-on-navigate branch from ed4169d to 93df517 Compare February 25, 2025 16:10
@lukaw3d lukaw3d merged commit 4e0992c into master Feb 25, 2025
@lukaw3d lukaw3d deleted the lw/cancel-on-navigate branch February 25, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments