feat(scene): see and stop running scenes (#9015) - #2710
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughScene executions now expose running state, websocket lifecycle events, and stop APIs. Frontend scene views track that state, display elapsed runtime, prevent duplicate starts, and provide stop controls across scene lists and editing views. ChangesScene runtime control
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SceneView
participant SceneController
participant SceneManager
participant Websocket
SceneView->>SceneController: GET /api/v1/scene/running
SceneController->>SceneManager: getRunning()
SceneManager-->>SceneController: Running execution summaries
SceneController-->>SceneView: Running scene list
SceneManager->>Websocket: SCENE.STARTED
Websocket-->>SceneView: Update runningScenes
SceneView->>SceneController: POST /api/v1/scene/:scene_selector/stop
SceneController->>SceneManager: stopBySelector(sceneSelector)
SceneManager->>Websocket: SCENE.STOPPED
Websocket-->>SceneView: Remove stopped execution
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2710 +/- ##
=======================================
Coverage 99.13% 99.14%
=======================================
Files 1183 1185 +2
Lines 24565 24615 +50
=======================================
+ Hits 24353 24405 +52
+ Misses 212 210 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
front/src/components/boxs/scene/SceneBox.jsx (1)
122-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
keyon the mapped<SceneRow>list.Flagged by static analysis. Each row now carries live, per-second-updating
runningInfo; addingkey={scene.selector}gives Preact stable identity for correct diffing ifscenesever reorders/filters.Based on learnings/static analysis: "A list component should have a key to prevent re-rendering."
🔧 Suggested fix
scenes.map(scene => ( <SceneRow + key={scene.selector} boxStatus={status}🤖 Prompt for 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. In `@front/src/components/boxs/scene/SceneBox.jsx` around lines 122 - 132, Add a stable key to each mapped SceneRow in the scenes rendering block, using scene.selector as the key. Keep the existing row props and runningInfo computation unchanged.Source: Linters/SAST tools
front/src/routes/scene/runningInfo.js (1)
1-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the running-scenes tracking logic alongside these helpers.
getRunningScenes,onSceneStarted,onSceneStopped, andrefreshTickerare duplicated near-verbatim acrossSceneBox.jsx,scene/index.js, andedit-scene/index.js. This module already centralizescomputeRunningInfo/formatElapsed; extracting the stateful tracking logic here too (e.g. a small mixin/helper factory that each component wires into its own state) would remove the triplication and would have prevented the scoping bug found inedit-scene/index.js'srefreshTicker.🤖 Prompt for 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. In `@front/src/routes/scene/runningInfo.js` around lines 1 - 39, Extend the runningInfo module beyond computeRunningInfo and formatElapsed by extracting the shared getRunningScenes, onSceneStarted, onSceneStopped, and refreshTicker state-tracking behavior into a reusable helper or factory. Update SceneBox, scene/index, and edit-scene/index to use that shared implementation while preserving each component’s state wiring and correcting the edit-scene refreshTicker scoping issue.server/test/controllers/scene/scene.test.js (1)
71-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd success-path coverage for the new stop endpoints.
Only the "nothing is running" branches (
success: false) are tested forGET /scene/running,POST /scene/:scene_selector/stop, andPOST /scene/execution/:execution_id/stop. Consider adding a test that starts a scene with a slow action (e.g. adelay), confirms it appears inGET /scene/running, then stops it and assertssuccess: true/stopped > 0, to cover the success branches of the new code.As per path instructions, "Assume 100% patch coverage for server changes; add tests for every new or modified line, branch, error path, and helper."
Also applies to: 142-170
🤖 Prompt for 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. In `@server/test/controllers/scene/scene.test.js` around lines 71 - 82, Add success-path tests alongside the existing GET /api/v1/scene/running coverage for both stop endpoints: start a scene containing a slow delay action, verify it appears as running, stop it through the scene selector and execution ID endpoints, and assert success: true with stopped > 0. Ensure the tests exercise every new success branch in the stop controller flows.Source: Path instructions
🤖 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 `@front/src/routes/scene/edit-scene/index.js`:
- Around line 156-164: Update refreshTicker so it checks whether the scene
identified by this.props.scene_selector is running instead of testing whether
any entry exists in this.state.runningScenes. Keep the existing interval
creation, cleanup, and state update behavior, but scope the ticker lifecycle to
the edited scene only.
In `@front/src/routes/scene/index.js`:
- Around line 52-59: The getRunningScenes fetch overwrites websocket updates
that arrive while the request is in flight. In front/src/routes/scene/index.js
lines 52-59, front/src/components/boxs/scene/SceneBox.jsx lines 31-92, and
front/src/routes/scene/edit-scene/index.js lines 129-136, update
getRunningScenes to use the previous runningScenes state, merge it with the
fetched results, and deduplicate entries by executionId before calling setState.
In `@front/src/routes/scene/style.css`:
- Line 119: Update the border declaration to use the lowercase CSS keyword
spelling expected by Stylelint, changing currentColor to currentcolor while
preserving the existing border style and width.
- Around line 103-105: Update the color declaration in .runningStopButton
.runningContent to use a darker green that achieves at least 4.5:1 contrast
against the white background, while preserving the existing running-status
styling.
In `@server/lib/scene/scene.execute.js`:
- Around line 46-55: Update the execution flow around executeActions to create a
shallow per-execution scope before assigning abortSignal, rather than mutating
the caller-provided scope. Pass this isolated scope to executeActions so chained
scenes retain independent abort signals while preserving the existing execution
registration and event behavior.
In `@server/test/lib/scene/scene.stop.test.js`:
- Around line 143-152: Clean up the still-running “other-scene” execution before
this test exits. After verifying getRunning() contains only that execution, stop
or otherwise cancel it through the existing sceneManager API and await any
required shutdown so its 60-minute delay timer cannot keep the Node process
alive.
---
Nitpick comments:
In `@front/src/components/boxs/scene/SceneBox.jsx`:
- Around line 122-132: Add a stable key to each mapped SceneRow in the scenes
rendering block, using scene.selector as the key. Keep the existing row props
and runningInfo computation unchanged.
In `@front/src/routes/scene/runningInfo.js`:
- Around line 1-39: Extend the runningInfo module beyond computeRunningInfo and
formatElapsed by extracting the shared getRunningScenes, onSceneStarted,
onSceneStopped, and refreshTicker state-tracking behavior into a reusable helper
or factory. Update SceneBox, scene/index, and edit-scene/index to use that
shared implementation while preserving each component’s state wiring and
correcting the edit-scene refreshTicker scoping issue.
In `@server/test/controllers/scene/scene.test.js`:
- Around line 71-82: Add success-path tests alongside the existing GET
/api/v1/scene/running coverage for both stop endpoints: start a scene containing
a slow delay action, verify it appears as running, stop it through the scene
selector and execution ID endpoints, and assert success: true with stopped > 0.
Ensure the tests exercise every new success branch in the stop controller flows.
🪄 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 Plus
Run ID: 451ccb9f-9d89-4f9a-ba3a-3b26d8a7e21e
📒 Files selected for processing (25)
front/src/components/boxs/scene/SceneBox.jsxfront/src/components/boxs/scene/SceneRow.jsxfront/src/config/demo.jsfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/scene/RunningStopButton.jsxfront/src/routes/scene/SceneCard.jsxfront/src/routes/scene/edit-scene/EditActions.jsxfront/src/routes/scene/edit-scene/index.jsfront/src/routes/scene/index.jsfront/src/routes/scene/runningInfo.jsfront/src/routes/scene/style.cssserver/api/controllers/scene.controller.jsserver/api/routes.jsserver/lib/scene/index.jsserver/lib/scene/scene.actions.jsserver/lib/scene/scene.execute.jsserver/lib/scene/scene.executeActions.jsserver/lib/scene/scene.getRunning.jsserver/lib/scene/scene.stop.jsserver/test/controllers/scene/scene.test.jsserver/test/lib/scene/scene.execute.test.jsserver/test/lib/scene/scene.stop.test.jsserver/utils/constants.js
| .runningStopButton .runningContent { | ||
| color: #5eba00; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a contrast-compliant running color.
#5eba00 has roughly 2.5:1 contrast on white, making the running status difficult to read. Use a darker green that reaches 4.5:1 or better.
Proposed fix
.runningStopButton .runningContent {
- color: `#5eba00`;
+ color: `#2b6e00`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .runningStopButton .runningContent { | |
| color: #5eba00; | |
| } | |
| .runningStopButton .runningContent { | |
| color: `#2b6e00`; | |
| } |
🤖 Prompt for 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.
In `@front/src/routes/scene/style.css` around lines 103 - 105, Update the color
declaration in .runningStopButton .runningContent to use a darker green that
achieves at least 4.5:1 contrast against the white background, while preserving
the existing running-status styling.
| const abortController = new AbortController(); | ||
| scope.abortSignal = abortController.signal; | ||
| // register this execution so it can be listed and stopped while running | ||
| this.runningScenes.set(executionId, { ...runningScene, abortController }); | ||
| this.event.emit(EVENTS.WEBSOCKET.SEND_ALL, { | ||
| type: WEBSOCKET_MESSAGE_TYPES.SCENE.STARTED, | ||
| payload: runningScene, | ||
| }); | ||
| try { | ||
| await executeActions(this, this.scenes[sceneSelector].actions, scope); | ||
| await executeActions(this, scene.actions, scope); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Isolate the abort signal per execution.
Line 47 mutates the caller’s shared scope. A chained scene can overwrite its parent’s signal: stopping the parent then fails to stop subsequent parent actions, while stopping the child can stop the parent. Pass a shallow per-execution scope to executeActions instead.
Proposed fix
const abortController = new AbortController();
-scope.abortSignal = abortController.signal;
+const executionScope = {
+ ...scope,
+ abortSignal: abortController.signal,
+};
...
-await executeActions(this, scene.actions, scope);
+await executeActions(this, scene.actions, executionScope);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const abortController = new AbortController(); | |
| scope.abortSignal = abortController.signal; | |
| // register this execution so it can be listed and stopped while running | |
| this.runningScenes.set(executionId, { ...runningScene, abortController }); | |
| this.event.emit(EVENTS.WEBSOCKET.SEND_ALL, { | |
| type: WEBSOCKET_MESSAGE_TYPES.SCENE.STARTED, | |
| payload: runningScene, | |
| }); | |
| try { | |
| await executeActions(this, this.scenes[sceneSelector].actions, scope); | |
| await executeActions(this, scene.actions, scope); | |
| const abortController = new AbortController(); | |
| const executionScope = { | |
| ...scope, | |
| abortSignal: abortController.signal, | |
| }; | |
| // register this execution so it can be listed and stopped while running | |
| this.runningScenes.set(executionId, { ...runningScene, abortController }); | |
| this.event.emit(EVENTS.WEBSOCKET.SEND_ALL, { | |
| type: WEBSOCKET_MESSAGE_TYPES.SCENE.STARTED, | |
| payload: runningScene, | |
| }); | |
| try { | |
| await executeActions(this, scene.actions, executionScope); |
🤖 Prompt for 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.
In `@server/lib/scene/scene.execute.js` around lines 46 - 55, Update the execution
flow around executeActions to create a shallow per-execution scope before
assigning abortSignal, rather than mutating the caller-provided scope. Pass this
isolated scope to executeActions so chained scenes retain independent abort
signals while preserving the existing execution registration and event behavior.
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 `@server/test/lib/scene/scene.stop.test.js`:
- Around line 130-135: Replace the fixed 30ms setTimeout in the scene stop test
with a deterministic checkpoint that confirms the delay action has registered
its abort listener before stop is called. Use an observable registration hook or
controlled fake timers and microtask draining, preserving coverage of the
listener path rather than the already-aborted guard.
🪄 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 Plus
Run ID: 0d364a65-08bd-4b6d-8487-b970099ae53a
📒 Files selected for processing (3)
server/lib/scene/scene.actions.jsserver/lib/scene/scene.execute.jsserver/test/lib/scene/scene.stop.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- server/lib/scene/scene.execute.js
- server/lib/scene/scene.actions.js
ee06ece to
a2874c5
Compare
|
Thanks for the review! The branch has been rebased on top of Fixed
Not changed (with rationale)
Patch coverage is 100% on the changed lines locally ( |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/test/lib/scene/scene.stop.test.js (1)
8-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared
eventEventEmitter across all tests in this suite.
eventis created once atdescribescope whilestateManager/sceneManagerare freshly instantiated per test inbeforeEach. IfSceneManager/StateManagerregister any listeners oneventinternally, they accumulate across every test in this file instead of being scoped per test, risking cross-test bleed-through andMaxListenersExceededWarning. RecreatingeventinbeforeEachalongside the other collaborators would give tighter isolation.♻️ Proposed fix
describe('scene.stop', () => { - const event = new EventEmitter(); const brain = {}; const device = {}; let stateManager; let sceneManager; + let event; beforeEach(() => { + event = new EventEmitter(); brain.addNamedEntity = fake.returns(null);🤖 Prompt for 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. In `@server/test/lib/scene/scene.stop.test.js` around lines 8 - 21, Move the EventEmitter initialization into beforeEach so each test creates a fresh event alongside stateManager and sceneManager. Remove the describe-scoped event instance and continue passing the per-test event to StateManager and SceneManager.front/src/components/boxs/scene/SceneBox.jsx (1)
122-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
keyprop to the mappedSceneRowlist.Static analysis flags the missing
keyon the list produced byscenes.map(...). Since this block is being touched to addrunningInfo, worth fixing alongside.♻️ Proposed fix
scenes.map(scene => ( <SceneRow + key={scene.selector} boxStatus={status}🤖 Prompt for 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. In `@front/src/components/boxs/scene/SceneBox.jsx` around lines 122 - 132, Add a stable key prop to each SceneRow rendered by the scenes.map callback, using the scene’s existing unique identifier such as its selector. Keep the runningInfo calculation and all other SceneRow props unchanged.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@front/src/components/boxs/scene/SceneBox.jsx`:
- Around line 122-132: Add a stable key prop to each SceneRow rendered by the
scenes.map callback, using the scene’s existing unique identifier such as its
selector. Keep the runningInfo calculation and all other SceneRow props
unchanged.
In `@server/test/lib/scene/scene.stop.test.js`:
- Around line 8-21: Move the EventEmitter initialization into beforeEach so each
test creates a fresh event alongside stateManager and sceneManager. Remove the
describe-scoped event instance and continue passing the per-test event to
StateManager and SceneManager.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: baae7a2c-04e0-4e2d-ac2c-dcab6fc40e00
📒 Files selected for processing (25)
front/src/components/boxs/scene/SceneBox.jsxfront/src/components/boxs/scene/SceneRow.jsxfront/src/config/demo.jsfront/src/config/i18n/de.jsonfront/src/config/i18n/en.jsonfront/src/config/i18n/fr.jsonfront/src/routes/scene/RunningStopButton.jsxfront/src/routes/scene/SceneCard.jsxfront/src/routes/scene/edit-scene/EditActions.jsxfront/src/routes/scene/edit-scene/index.jsfront/src/routes/scene/index.jsfront/src/routes/scene/runningInfo.jsfront/src/routes/scene/style.cssserver/api/controllers/scene.controller.jsserver/api/routes.jsserver/lib/scene/index.jsserver/lib/scene/scene.actions.jsserver/lib/scene/scene.execute.jsserver/lib/scene/scene.executeActions.jsserver/lib/scene/scene.getRunning.jsserver/lib/scene/scene.stop.jsserver/test/controllers/scene/scene.test.jsserver/test/lib/scene/scene.execute.test.jsserver/test/lib/scene/scene.stop.test.jsserver/utils/constants.js
🚧 Files skipped from review as they are similar to previous changes (19)
- server/lib/scene/scene.getRunning.js
- server/lib/scene/scene.stop.js
- front/src/routes/scene/runningInfo.js
- front/src/components/boxs/scene/SceneRow.jsx
- server/test/controllers/scene/scene.test.js
- front/src/config/demo.js
- front/src/routes/scene/RunningStopButton.jsx
- server/utils/constants.js
- server/lib/scene/index.js
- server/lib/scene/scene.executeActions.js
- front/src/routes/scene/edit-scene/EditActions.jsx
- front/src/config/i18n/en.json
- front/src/routes/scene/edit-scene/index.js
- server/api/routes.js
- front/src/config/i18n/de.json
- server/test/lib/scene/scene.execute.test.js
- front/src/routes/scene/index.js
- server/api/controllers/scene.controller.js
- front/src/routes/scene/SceneCard.jsx
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 `@front/src/routes/scene/runningInfo.js`:
- Around line 21-38: Update mergeRunningScenes and its callers to track a
request/event revision or stop tombstones, ensuring a fetched snapshot older
than scene.started/scene.stopped websocket updates cannot re-add stopped
executions or overwrite newer websocket metadata. Preserve current state for
entries changed after the fetch began, while applying unchanged fetched entries
normally, and add coverage for stop and start events received before the fetch
resolves.
🪄 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 Plus
Run ID: 0e35483e-0b93-4cff-84ab-15ba7c74f672
📒 Files selected for processing (6)
front/src/components/boxs/scene/SceneBox.jsxfront/src/routes/scene/edit-scene/index.jsfront/src/routes/scene/index.jsfront/src/routes/scene/runningInfo.jsfront/src/routes/scene/style.cssserver/test/lib/scene/scene.stop.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- front/src/components/boxs/scene/SceneBox.jsx
- front/src/routes/scene/index.js
- front/src/routes/scene/style.css
- front/src/routes/scene/edit-scene/index.js
| /** | ||
| * @description Merge a freshly fetched list of running scenes with the entries | ||
| * already present in the state, deduplicating by executionId. This avoids a | ||
| * late-resolving fetch overwriting updates already applied by websocket events | ||
| * (scene.started / scene.stopped) received while the fetch was in flight. | ||
| * @param {Array} fetched - The list returned by GET /api/v1/scene/running. | ||
| * @param {Array} current - The running scenes currently in the state. | ||
| * @returns {Array} The merged list (fetched entries win on conflict). | ||
| * @example | ||
| * mergeRunningScenes(fetched, prevState.runningScenes); | ||
| */ | ||
| export const mergeRunningScenes = (fetched, current) => { | ||
| const byExecutionId = new Map(); | ||
| (current || []).forEach(runningScene => byExecutionId.set(runningScene.executionId, runningScene)); | ||
| (fetched || []).forEach(runningScene => byExecutionId.set(runningScene.executionId, runningScene)); | ||
| return Array.from(byExecutionId.values()); | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent stale fetches from restoring stopped executions.
Lines 34-35 make fetched data authoritative, despite the function’s stated purpose. If a scene.stopped websocket event removes an execution while GET /api/v1/scene/running is in flight, the stale response re-adds that execution; duplicate IDs can also overwrite newer websocket metadata. Use a request/event revision (or stop tombstones) so snapshots older than websocket updates cannot be applied. Add tests for stop/start events arriving before the fetch resolves.
🤖 Prompt for 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.
In `@front/src/routes/scene/runningInfo.js` around lines 21 - 38, Update
mergeRunningScenes and its callers to track a request/event revision or stop
tombstones, ensuring a fetched snapshot older than scene.started/scene.stopped
websocket updates cannot re-add stopped executions or overwrite newer websocket
metadata. Preserve current state for entries changed after the fetch began,
while applying unchanged fetched entries normally, and add coverage for stop and
start events received before the fetch resolves.
fd9de19 to
ae6f41f
Compare
Adds an in-memory registry of running scene executions in the SceneManager (keyed by a generated executionId), emits scene.started / scene.stopped websocket events, and exposes GET /api/v1/scene/running. On the frontend, the "Start" button itself becomes the running indicator (pulsing dot + instance count + live elapsed time) on the scene list, the scene editor and the dashboard scene widget. Manual re-start is prevented while a scene is already running. This is Lot 1 (observe) of the community request #9015; stopping a running scene will come in a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the ability to stop a running scene execution:
- an AbortController per execution stored in the SceneManager registry,
exposed to actions through the scope (scope.abortSignal)
- the "delay" action is now abortable, so a scene waiting in a long delay
can be interrupted immediately
- cooperative cancellation between actions (remaining actions are skipped
once the scene is stopped)
- SceneManager.stop(executionId) / stopBySelector(sceneSelector)
- POST /api/v1/scene/execution/:execution_id/stop
- POST /api/v1/scene/:scene_selector/stop
On the frontend, while a scene is running the "Start" button becomes a
running indicator ("En cours" + live elapsed time, green outline) that turns
into a red "Stop" button on hover, on the scene list, the scene editor and
the dashboard scene widget. RunningStatus was merged into RunningStopButton.
Adds demo-mode fake data for GET /api/v1/scene/running.
This is Lot 2 (stop) of the community request #9015.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- capture the scene at trigger time in execute() instead of re-reading it in the queued job (removes an unreachable defensive guard) - remove the redundant "already aborted" pre-check in the delay action (executeAction already guards against an aborted scene) - add tests covering stopBySelector on a non-matching selector and stopping a scene while it is actively waiting in a delay (abort listener path) Ensures 100% patch coverage on the changed lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- merge fetched running scenes with websocket-driven state (dedupe by executionId) to avoid a late fetch clobbering scene.started/stopped updates - scope the scene editor to the edited scene so its 1s ticker no longer runs when an unrelated scene is executing - add a key to the mapped SceneRow list - use lowercase `currentcolor` (stylelint) - clean up running executions after each stop test (release delay timers) - make the "abort during delay" test deterministic (setImmediate drain instead of a fixed sleep) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ae6f41f to
5a0cb85
Compare
github-actions
Bot
added
area:server
Pierre-Gilles
added
the
needs:cursor-review
Cursor
Pierre-Gilles
removed
the
needs:cursor-review
Pierre-Gilles
added
the
needs:cursor-review
Cursor
Pierre-Gilles
removed
the
needs:cursor-review
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid feature overall: in-memory running registry, abortable delay, websocket scene.started/scene.stopped, and the shared Running/Stop UI are well structured. CI is green (including patch coverage).
Two correctness issues should be fixed before merge — they both undermine the “see & stop running scenes” promise under realistic timing:
- Delay stop race — In Node,
abortlisteners are not invoked when the signal is already aborted ataddEventListenertime. A stop betweenexecuteAction’s aborted check and the delay listener registration leaves the timer running for the full delay. - Stale
GET /scene/runningcan revive stopped executions —mergeRunningSceneslets fetched entries win, so a late response can re-add an execution that websocketscene.stoppedalready removed. That can leave the Start button stuck as Running/Stop until refresh.
Soft / non-blocking
- Touch: the Running button stops on tap with no visible “Stop” state (hover-only swap).
#5eba00on white is low contrast for the running label.- Deleting a scene does not abort its in-flight executions (they keep the captured scene object); consider
stopBySelectorindestroy. lodash.cloneDeepkeeps the parentAbortSignalby reference until the child overwrites it; prefer stripping/newsignal when chaining scenes for clarity (current overwrite on the cloned scope object is OK in practice).
No risk:high (additive scene control, not auth/DB/host). No needs:human-review (clear community request, no taxonomy/philosophy call). Removed needs:cursor-review.
Sent by Cursor Automation: Automatic PR review
| await new Promise((resolve, reject) => { | ||
| const timer = setTimeout(resolve, timeToWaitMilliseconds); | ||
| if (abortSignal) { | ||
| abortSignal.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| clearTimeout(timer); | ||
| reject(new AbortScene('SCENE_STOPPED')); | ||
| }, | ||
| { once: true }, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: already-aborted AbortSignal does not fire abort listeners in Node.
Verified locally: after abortController.abort(), signal.addEventListener('abort', …) never runs, so setTimeout keeps going for the full delay (minutes/hours).
The comment that executeAction already caught this is not enough — there is a race between that check and listener registration. If stop lands in that window, cooperative cancel fails for the one action this PR makes interruptible.
Please reject immediately when already aborted, then attach the listener:
await new Promise((resolve, reject) => {
const timer = setTimeout(resolve, timeToWaitMilliseconds);
if (!abortSignal) {
return;
}
if (abortSignal.aborted) {
clearTimeout(timer);
reject(new AbortScene('SCENE_STOPPED'));
return;
}
abortSignal.addEventListener(
'abort',
() => {
clearTimeout(timer);
reject(new AbortScene('SCENE_STOPPED'));
},
{ once: true },
);
});A unit test that aborts before entering the delay promise (or between check and listener) would lock this in.
| export const mergeRunningScenes = (fetched, current) => { | ||
| const byExecutionId = new Map(); | ||
| (current || []).forEach(runningScene => byExecutionId.set(runningScene.executionId, runningScene)); | ||
| (fetched || []).forEach(runningScene => byExecutionId.set(runningScene.executionId, runningScene)); | ||
| return Array.from(byExecutionId.values()); |
There was a problem hiding this comment.
Stale fetch can resurrect a stopped execution.
Despite the docstring, fetched entries overwrite/re-add into the map after current is applied. Sequence:
GET /api/v1/scene/runningstarts (response still includes execution X)scene.stoppedremoves X from state- Fetch resolves →
mergeRunningScenesputs X back
Because Start is hidden whenever runningInfo is truthy, the UI can look “still running” and block a new start until a refresh or another WS event.
Fix options:
- Treat websocket removals as authoritative (e.g. short-lived stop tombstones filtered out of the merge result), or
- Snapshot a fetch generation / “stopped since fetch started” set and drop those ids when applying the response,
- Or skip merging removals the other way: start from
fetched, then add only current entries that are missing from fetched (starts during flight) — still need tombstones for stops during flight.
Please cover with a small unit test on mergeRunningScenes (or the callers) for “stopped while fetch in flight”.
3 participants
Pull Request check-list
cd server && npm run coverage) — new code covered bytest/lib/scene/scene.stop.test.js, additions toscene.execute.test.js, and controller tests (only a few defensive guards remain, e.g. "scene deleted while queued")npm run eslinton both front/server)npm run prettieron both front/server)npm run compare-translationson front) — passes (fr/en/de complete)@apicomments added for the new endpointsfront/src/config/demo.js)? — addedget /api/v1/scene/runningDescription of change
Implements the community request #9015 — Connaître les scènes en cours d'exécution et pouvoir les arrêter: see which scenes are currently running (and for how long), and stop them without restarting Gladys.
Delivered in two parts:
Lot 1 — See running executions
SceneManager(keyed by a generatedexecutionId), populated inscene.execute.jsand cleaned up in afinally.scene.getRunning()+GET /api/v1/scene/running(registered before:scene_selectorto avoid a route collision).scene.started/scene.stoppedwebsocket events.Lot 2 — Stop a running scene
AbortControllerper execution, exposed to actions viascope.abortSignal.delayaction is now abortable (a scene waiting in a long delay can be interrupted immediately), plus cooperative cancellation between actions.scene.stop(executionId)/scene.stopBySelector(sceneSelector)+POST /api/v1/scene/:scene_selector/stopandPOST /api/v1/scene/execution/:execution_id/stop.Frontend (scene list, scene editor and dashboard scene widget)
En cours+ elapsed time, green outline) that turns into a red Stop button on hover. Constant width, updates in real time over websocket. SharedRunningStopButtoncomponent.Notes
delayis interrupted instantly; a long non-interruptible action (e.g. a big HTTP request) finishes before the stop takes effect between two actions.Screenshots
Summary by CodeRabbit