Resolves: MTV-3867 | Can click start twice #2188
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
===========================================
- Coverage 36.81% 14.06% -22.76%
===========================================
Files 158 1031 +873
Lines 2548 19143 +16595
Branches 599 3896 +3297
===========================================
+ Hits 938 2692 +1754
- Misses 1428 15802 +14374
- Partials 182 649 +467 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Aviv Turgeman <aturgema@redhat.com>
avivtur
force-pushed
the
can-click-start-multiple-times
branch
from
bbd7009 to
20fd128
Compare
|
|
|
||
| // Track migration start timestamps to prevent duplicate starts within cooldown period | ||
| const migrationStartTimestamps = new Map<string, number>(); | ||
| const MIGRATION_COOLDOWN_MS = 10000; // 10 seconds cooldown to allow k8s watch to update |
There was a problem hiding this comment.
I see in the PR description you mentioned this will only work for single session and not across multiple tabs/windows. Just want to confirm - what's the expected behavior if:
- A user opens the same plan in two browser tabs and clicks Start in both within the 10 second window?
- A user refreshes the page during the cooldown period?
I think it will be cleared in both cases, so the cooldown won't apply, right?
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| forceUpdate((prev) => prev + 1); | ||
| }, 500); |
There was a problem hiding this comment.
I understand we need to re-check the cooldown periodically to re-enable the button. But will polling every 500ms cause performance issues on list pages with many plans?
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| forceUpdate((prev) => prev + 1); | ||
| }, 500); |
There was a problem hiding this comment.
Same 500ms polling question as in PlanActionsDropdownItems. This could add up quickly on a list page with dozens of plans.
| throw new Error(t('Migration was recently started for this plan. Please wait.')); | ||
| } | ||
|
|
||
| migrationStartTimestamps.set(uid!, Date.now()); |
There was a problem hiding this comment.
Should there be some kind of cleanup here with maybe
if (!isPlanMigrationInCooldown(uid)) {
migrationStartTimestamps.delete(planUid);
}
3 participants
📝 Links
https://issues.redhat.com/browse/MTV-3867
📝 Description
It is possible to click fast enough on the start button and the confirm start twice or more in a row, which caused the plan to create more than one migration. As a side-effect the last created migration is not the active one which didn't allowed to set a cutover in case of a warm plan.
I've disabled the start button after clicking the start in the modal success, and not basing on the migration's status since it takes a second to populate and show in the UI which in that time can start the plan multiple times. (this will work only for single session and not across multiple sessions as multiple browser tabs/windows trying to acheive the same or different remote session in parallel)
I've updated the usePlanMigration hook to get the activeMigration and not the lastMigration, to allow setting cutover correctly to avoid a situation where the plan just hang waiting for cutover and it set to wrong migration resource
🎥 Demo
allow-setting-cutover-correctly.mp4
disable-after-click.mp4
📝 CC://