Fix run button state management and prevent concurrent computations#3964
Fix run button state management and prevent concurrent computations#3964ayolab wants to merge 1 commit into
Conversation
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
📝 WalkthroughWalkthroughThis PR introduces ChangesComputation Status Initialization and State Handling
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/utils/split-button.tsx`:
- Line 257: The Stop/menu action currently only uses
disabled={computationStarting}, allowing repeated stop requests while runnable
remains RUNNING; update the guard so the UI and handler both prevent duplicates
by disabling the control when computationStarting is true OR runnable?.status
=== 'RUNNING' (e.g., change disabled to computationStarting || runnable?.status
=== 'RUNNING' on the menu item/button) and add an early return in
actionOnRunnable() to no-op if computationStarting || runnable?.status ===
'RUNNING' to ensure clicks cannot trigger duplicate stop requests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86c1a6ff-961b-4490-bd7f-ffa7a580ee34
📒 Files selected for processing (6)
src/components/computing-status/use-computing-status.tssrc/components/run-button-container.jsxsrc/components/run-button.jsxsrc/components/utils/running-status.tssrc/components/utils/split-button.tsxsrc/redux/reducer.ts
💤 Files with no reviewable changes (1)
- src/components/run-button-container.jsx
There was a problem hiding this comment.
This is not because of your dev but very close and I encountered this while testing : I start a security analysis, then I stop it quickly. Then I try to unbuild the node and I can't : the node is stuck :
This is a quite big problem. If you don't know where it is coming from maybe we should create another anomaly ticket.
(The bug is on dev opf as well)
Mathieu-Deharbe
left a comment
Mathieu-Deharbe
left a comment
There was a problem hiding this comment.
Your code looks good and corrects the bug so I should approve it, but those 2 bugs are worrying me.
| const runningRunnable = useMemo( | ||
| () => activeRunnables.find((r) => getStatus(r) === RunningStatus.RUNNING) ?? null, | ||
| [activeRunnables, getStatus] | ||
| ); |
There was a problem hiding this comment.
Isn't there a risk that this will return an array ?
| const disabledOption = | ||
| computationStarting || // disable if fetch starting a computation is pending | ||
| (runningStatus === RunningStatus.RUNNING && computationStopped); // disable if already stopped once | ||
|
|
There was a problem hiding this comment.
I thought that code was here in order to prevent a restart of a computation before a stop is handled in the back but it looks like I was wrong because even with that code I have this bug :
- start a computation
- cancel it immediately
- restart the same computation quickly
- fails (and then "succeeds" sometimes because the computation finished in the back and its result is returned)
So it looks like you are right to remove this, but I am a bit lost. One more bug ?
2 participants
No description provided.