fix: add guard against concurrent execution in DeleteModal#1088
fix: add guard against concurrent execution in DeleteModal#1088Snehadas2005 wants to merge 3 commits into
Conversation
thaorell
left a comment
thaorell
left a comment
There was a problem hiding this comment.
Hey @Snehadas2005, good work here. The comment I left is not a blocking issue, so let me know how it goes
| }) => { | ||
| const { isMUITheme } = useThemeContext(); | ||
| const [inputValue, setInputValue] = useState(''); | ||
| const [isDeleting, setIsDeleting] = useState(false); |
There was a problem hiding this comment.
Consider removing isDeleting state to avoid maintaining both ref and state and having the Cancel button always visible. Test to see if hitting the Cancel button while deleting causes any problem
|
Thank you for the suggestion @thaorell! I have pushed an update that removes the |
|
/ok-to-test |
paulovmr
left a comment
paulovmr
left a comment
There was a problem hiding this comment.
Hi @Snehadas2005 , thanks for the pull request! I've reviewed and made one comment for a change, if you could take a look please. Also, could you please consider adding some test to cover this behavior?
| onClose(); | ||
| } catch (err) { | ||
| setError(extractErrorMessage(err)); | ||
| isDeletingRef.current = false; |
There was a problem hiding this comment.
Please consider having this line on the finally instead of the catch block.
Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
Snehadas2005
force-pushed
the
fix-deletemodal
branch
from
fb3758e to
a89e826
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Snehadas2005
force-pushed
the
fix-deletemodal
branch
from
4afa92d to
19de63c
Compare
Snehadas2005
force-pushed
the
fix-deletemodal
branch
from
212c374 to
9fb833f
Compare
Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com> add: test file for DeleteModal Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com> fix: Add guard against concurrent DeleteModal execution Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com> Update package-lock.json to resolve dependency conflicts Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com> fix: lint error and updated esbuild version Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
Snehadas2005
force-pushed
the
fix-deletemodal
branch
from
f12fcc7 to
a0ba22c
Compare
Signed-off-by: Sneha Das <154408198+Snehadas2005@users.noreply.github.com>
Snehadas2005
force-pushed
the
fix-deletemodal
branch
from
fed6f32 to
a700143
Compare
|
Hi @paulovmr, thank you for the review! |
| "classnames": "^2.2.6", | ||
| "date-fns": "^4.1.0", | ||
| "dompurify": "^3.2.4", | ||
| "esbuild": "0.28.0", |
There was a problem hiding this comment.
Hi @Snehadas2005, can you remove this package?
3 participants
Fixes #1042
Rapidly hitting the "Enter" key or clicking the "Delete" button twice in the
DeleteModalcould trigger multiple duplicate API calls. This occurred because React's asynchronous state updates were too slow to block subsequent event triggers within the same render cycle.This PR implements a synchronous guard to ensure only one deletion request is processed at a time.
Changes:
useRefguard (isDeletingRef) to block concurrent execution before the next render cycle immediately.handleDeletefunction to verify the lock and reset it only upon failure, ensuring the action is "one-shot" per modal session.