chore: display the stop button in a compose section #13519
Conversation
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
📝 WalkthroughWalkthroughAdds unit tests for ComposeActions covering Start/Stop visibility across container-state combinations and during asynchronous start/stop operations (introduces Deferred helper and window.startContainersByLabel / window.stopContainersByLabel stubs). Modifies ComposeActions.svelte internals: introduces derived booleans someNeedStart, someNeedStop and toggles hideStartForStop, hideStopForStart; startCompose/stopCompose set these toggles before initiating actions. Start/Stop button bindings (hidden, enabled, inProgress) are updated to use the new reactive variables. No public/exported API changes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey @vzhukovs - I've reviewed your changes - here's some feedback:
- Consider defining explicit showStart and showStop reactive helpers (e.g.
$: showStart = someStopped && !isStarting) instead of negating someStopped/someRunning in the hidden prop for better readability. - The new test only checks that the buttons get hidden—add complementary assertions for when the buttons should be visible under different container states to cover both show/hide paths.
- Add a safe fallback (
compose.containers ?? []) in your reactive someRunning/someStopped declarations to avoid potential runtime errors if containers ever becomes undefined.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider defining explicit showStart and showStop reactive helpers (e.g. `$: showStart = someStopped && !isStarting`) instead of negating someStopped/someRunning in the hidden prop for better readability.
- The new test only checks that the buttons get hidden—add complementary assertions for when the buttons should be visible under different container states to cover both show/hide paths.
- Add a safe fallback (`compose.containers ?? []`) in your reactive someRunning/someStopped declarations to avoid potential runtime errors if containers ever becomes undefined.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Grabacion.de.pantalla.2025-08-08.a.las.10.07.04.mov
While testing the PR, I noticed an issue with the bulk stop functionality. When stopping all services from the parent compose resource, the Run and Stop buttons disappear immediately. This prevents the user from receiving any visual feedback from buttons (loader spinner around buttons) that the services are in the process of shutting down.
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
Updated a bit behaviour. Now buttons don't disappear during toggling state: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/renderer/src/lib/compose/ComposeActions.svelte (4)
31-36: Normalize derived flags to booleans; consider resetting toggle flags after actions
- Derivations can become undefined when
compose.containersis falsy, which then flips semantics in!someNeedStart/Stop. Make them explicitly boolean.- Also consider resetting
hideStartForStop/hideStopForStartonce actions complete to avoid stale state lingering (even if not currently used whenactionInProgressis false).Apply this diff to normalize booleans:
-$: someNeedStart = compose.containers?.some(c => c.state !== 'RUNNING'); -$: someNeedStop = compose.containers?.some(c => c.state === 'RUNNING'); +$: someNeedStart = compose.containers?.some(c => c.state !== 'RUNNING') ?? false; +$: someNeedStop = compose.containers?.some(c => c.state === 'RUNNING') ?? false;And update
inProgressto reset toggles after completion (outside the selected range):function inProgress(inProgress: boolean, state?: string): void { compose.actionInProgress = inProgress; if (inProgress) { compose.actionError = ''; } else { // reset visibility guards after any action hideStartForStop = false; hideStopForStart = false; } if (state) compose.status = state; for (const container of compose.containers) { container.actionInProgress = inProgress; if (inProgress) container.actionError = ''; if (state) container.state = state; } onUpdate(compose); }
67-77: Guard against re-entrancy/double-clicks instartComposeEven though the UI disables the button while in progress, it’s cheap to harden the handler against rapid clicks or programmatic calls.
async function startCompose(): Promise<void> { + if (compose.actionInProgress) return; hideStopForStart = !someNeedStop; inProgress(true, 'STARTING'); try { await window.startContainersByLabel(compose.engineId, composeLabel, compose.name); } catch (error) { handleError(String(error)); } finally { inProgress(false); } }
78-88: Guard against re-entrancy/double-clicks instopComposeSame rationale as
startCompose.async function stopCompose(): Promise<void> { + if (compose.actionInProgress) return; hideStartForStop = !someNeedStart; inProgress(true, 'STOPPING'); try { await window.stopContainersByLabel(compose.engineId, composeLabel, compose.name); } catch (error) { handleError(String(error)); } finally { inProgress(false); } }
133-139: Readable hidden logic: extract to named reactive variablesThe ternary with nested conditions is correct but a bit dense. Extracting into
startHiddenimproves readability and keeps template clean.Inside the script (outside this range):
$: startHidden = !compose.actionInProgress ? !someNeedStart : (compose.status === 'STOPPING' && hideStartForStop);Then update here:
- hidden={ - !compose.actionInProgress - ? !someNeedStart - : (compose.status === 'STOPPING' && hideStartForStop) - } + hidden={startHidden}packages/renderer/src/lib/compose/ComposeActions.spec.ts (3)
163-197: Stabilize the STOPPING-phase test; avoid resolving if you only assert “during STOPPING”You resolve the deferred promise but don’t wait for the action to complete, so assertions still run in the STOPPING phase by accident. Either remove the resolve or assert both the in-progress and post-completion states to make the test intention explicit.
Option A (keep it “during STOPPING”): remove the resolve
- dStop.resolve(); - expect(ui.getByRole('button', { name: 'Start Compose', hidden: true })).toHaveClass('hidden'); expect(ui.getByRole('button', { name: 'Stop Compose' })).not.toHaveClass('hidden');Option B (also assert after completion): wait for completion and then assert the final visibility
dStop.resolve(); await waitFor(() => expect(composeAllRunning.actionInProgress).toBe(false)); // add final-state assertions here
199-233: Same stabilization for the STARTING-phase testMirror the approach from the STOPPING test: either don’t resolve when asserting “during STARTING” or explicitly wait for completion and assert final state.
Option A (STOPPING-phase only):
- dStart.resolve(); - expect(ui.getByRole('button', { name: 'Stop Compose', hidden: true })).toHaveClass('hidden'); expect(ui.getByRole('button', { name: 'Start Compose' })).not.toHaveClass('hidden');Option B (also assert after completion):
dStart.resolve(); await waitFor(() => expect(composeAllStopped.actionInProgress).toBe(false)); // add final-state assertions here
235-270: Clarify intent for partial RUNNING set; avoid accidental timing dependenceSame pattern: if the goal is to assert “during STOPPING”, don’t resolve; if you want to assert both phases, wait for completion before final assertions.
Option A:
- dStop.resolve(); - expect(ui.getByRole('button', { name: 'Start Compose' })).not.toHaveClass('hidden'); expect(ui.getByRole('button', { name: 'Stop Compose' })).not.toHaveClass('hidden');Option B:
dStop.resolve(); await waitFor(() => expect(composePartial.actionInProgress).toBe(false)); // add final-state assertions here
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/renderer/src/lib/compose/ComposeActions.spec.ts(3 hunks)packages/renderer/src/lib/compose/ComposeActions.svelte(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/renderer/src/lib/compose/ComposeActions.spec.ts (1)
packages/renderer/src/lib/container/ContainerInfoUI.ts (1)
ContainerInfoUI(47-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: smoke e2e tests (development)
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: smoke e2e tests (production)
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: Windows
- GitHub Check: build website
🔇 Additional comments (4)
packages/renderer/src/lib/compose/ComposeActions.svelte (1)
147-154: LGTM: Stop button visibility now correctly reflects running containersThe updated hidden logic ensures the Stop button is shown whenever any container is running, matching the PR objective. Enabling/disabling tied to
actionInProgressis consistent.packages/renderer/src/lib/compose/ComposeActions.spec.ts (3)
21-21: LGTM: Scoped queries withwithinUsing
within(container)avoids global-query leakage and keeps assertions localized.
23-23: LGTM: Importing Mock type for safe castingExplicit
Mocktyping for window stubs makes intent clear and avoids brittle any-casts.
68-82: LGTM: Deferred helper to simulate async actionsThe
Deferredpattern cleanly simulates long-running start/stop operations and keeps tests deterministic.
…3519) * chore: display the stop button in a compose section Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com> * chore: display the stop button in a compose section Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com> --------- Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
What does this PR do?
Display the stop button if any container in the compose section has running status.
Screenshot / video of UI
What issues does this PR fix or reference?
#10406
How to test this PR?
Deploy the following compose file:
and try to manually run/stop the containers in compose section of Container List.