feat(cue): sleep prevention, Process Monitor integration, and trigger play button#600
Merged
feat(cue): sleep prevention, Process Monitor integration, and trigger play button#600
Conversation
…d runs Wire onPreventSleep/onAllowSleep through CueEngine and CueRunManager to the PowerManager so the system stays awake while Cue pipelines are active.
Expose active Cue processes via getCueProcessList and merge with run metadata so the Process Monitor shows a dedicated CUE RUNS section with event type badges, subscription names, and cue.stopRun kill routing.
Wire onTriggerPipeline through the pipeline editor so trigger nodes show a play button that manually fires the pipeline, with a spinner while running and disabled state when unsaved.
📝 WalkthroughWalkthroughAdds sleep-prevention hooks for Cue schedules and runs, enriches executor active-process tracking and IPC with Cue metadata (including PID and run info), wires sleep-blocking into the main power manager, adds UI trigger controls for Cue pipelines, and includes extensive tests for process listing, sleep-prevention, and renderer behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CueEngine
participant CueRunManager
participant CueExecutor
participant PowerManager
participant Renderer
Note over User,Renderer: High-level flow for schedule/run sleep prevention and process reporting
User->>CueEngine: initSession(config with time subscriptions)
CueEngine->>PowerManager: onPreventSleep("cue:schedule:{sessionId}")
PowerManager->>System: addBlockReason("cue:schedule:{sessionId}")
User->>CueRunManager: executeCuePrompt(run details)
CueRunManager->>CueExecutor: spawn child process
CueExecutor->>CueRunManager: return CueActiveProcess (pid, cmd, args, startTime)
CueRunManager->>PowerManager: onPreventSleep("cue:run:{runId}")
PowerManager->>System: addBlockReason("cue:run:{runId}")
CueExecutor->>Renderer: getCueProcessList() via IPC
Renderer->>Renderer: display "CUE RUNS" with cue metadata
Note over CueRunManager,CueExecutor: Run completes or stopped
CueRunManager->>PowerManager: onAllowSleep("cue:run:{runId}")
PowerManager->>System: removeBlockReason("cue:run:{runId}")
User->>CueEngine: teardownSession()
CueEngine->>PowerManager: onAllowSleep("cue:schedule:{sessionId}")
PowerManager->>System: removeBlockReason("cue:schedule:{sessionId}")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR adds three well-scoped features to the Maestro Cue system: system sleep prevention during active Cue work, Cue process visibility in the Process Monitor, and a "Run now" play button on pipeline trigger nodes. Sleep prevention is cleanly threaded through Process Monitor integration upgrades Trigger play button adds Key observations:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CE as CueEngine
participant RM as CueRunManager
participant EX as CueExecutor
participant PM as PowerManager
participant PM2 as ProcessMonitor (renderer)
Note over CE,PM: Session with time-based subscription starts
CE->>PM: addBlockReason("cue:schedule:sessionId")
Note over CE,EX: Subscription fires → run dispatched
CE->>RM: execute(sessionId, prompt, event, ...)
RM->>PM: addBlockReason("cue:run:runId")
RM->>EX: onCueRun({ runId, prompt, ... })
EX->>EX: spawn child process
EX-->>PM2: getCueProcessList() → [{ runId, pid, command, ... }]
PM2->>PM2: render "CUE RUNS" section
Note over RM,PM: Run completes or is stopped
RM->>PM: removeBlockReason("cue:run:runId")
Note over CE,PM: Session torn down
CE->>RM: reset() / stopAll()
CE->>PM: removeBlockReason("cue:schedule:sessionId")
Note over PM2,EX: User kills Cue run from Process Monitor
PM2->>EX: cue.stopRun(runId) [via IPC]
EX->>EX: child.kill("SIGTERM") → SIGKILL after 5s
Last reviewed commit: "feat(cue): add "Run ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| await window.maestro.process.kill(processSessionId); | ||
| if (cueRunId) { | ||
| await (window as any).maestro.cue.stopRun(cueRunId); |
There was a problem hiding this comment.
(window as any) type cast
window.maestro.cue.stopRun is already properly typed in global.d.ts (the cue namespace includes stopRun: (runId: string) => Promise<boolean>), so the (window as any) cast bypasses TypeScript's type safety for no reason.
Suggested change
| await (window as any).maestro.cue.stopRun(cueRunId); | |
| await window.maestro.cue.stopRun(cueRunId); |
| const state = this.sessions.get(sessionId); | ||
| if (!state) return; | ||
|
|
||
| // Release sleep prevention for this session's scheduled subscriptions |
There was a problem hiding this comment.
onAllowSleep call for all torn-down sessions
teardownSession always calls onAllowSleep?.('cue:schedule:' + sessionId) regardless of whether the session had any time-based subscriptions. By contrast, initSession only calls onPreventSleep when hasTimeBasedSubscriptions is true. For sessions with only file.changed or agent.completed subscriptions, this causes a spurious remove call with a reason that was never added.
PowerManager.removeBlockReason handles the missing-reason case gracefully (debug log + no-op), so there is no functional impact. Consider mirroring the guard from initSession to keep the two call sites symmetric and avoid unnecessary debug noise:
// Release sleep prevention for this session's scheduled subscriptions
if (this.hasTimeBasedSubscriptions(state.config, sessionId)) {
this.deps.onAllowSleep?.(`cue:schedule:${sessionId}`);
}| title={data.isRunning ? 'Running…' : 'Run now'} | ||
| > | ||
| {data.isRunning ? ( | ||
| <Loader2 size={14} style={{ animation: 'spin 1s linear infinite' }} /> |
There was a problem hiding this comment.
style={{ animation: 'spin 1s linear infinite' }} references the @keyframes spin animation by name as an inline style. This works in practice because Tailwind includes the keyframe whenever animate-spin is used anywhere else in the build — but it is coupling to an implicit CSS side-effect rather than explicitly declaring the class.
The same pattern also appears in NodeConfigPanel.tsx at the equivalent spinner site.
Consider using className="animate-spin" directly (the same approach used by other spinners in the codebase) so the dependency on the keyframe is explicit and PurgeCSS/Tailwind can track it:
Suggested change
| <Loader2 size={14} style={{ animation: 'spin 1s linear infinite' }} /> | |
| <Loader2 size={14} className="animate-spin" /> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx (1)
378-387: Consider consolidating duplicate pipeline lookups.The same
pipelines.find()operation is performed twice with identical logic (lines 380 and 385). While not a performance blocker for typical pipeline sizes, extracting this to a single lookup would improve readability.♻️ Suggested refactor
+ {(() => { + const p = pipelines.find((pl) => pl.nodes.some((n) => n.id === selectedNode.id)); + return ( + <NodeConfigPanel + selectedNode={selectedNode} + pipelines={pipelines} + hasOutgoingEdge={selectedNodeHasOutgoingEdge} + hasIncomingAgentEdges={hasIncomingAgentEdges} + incomingTriggerEdges={incomingTriggerEdges} + onUpdateNode={onUpdateNode} + onUpdateEdgePrompt={onUpdateEdgePrompt} + onDeleteNode={onDeleteNode} + onSwitchToAgent={onSwitchToSession} + triggerDrawerOpen={triggerDrawerOpenForConfig} + agentDrawerOpen={agentDrawerOpenForConfig} + onTriggerPipeline={onTriggerPipeline} + pipelineName={p?.name} + isSaved={!isDirty} + isRunning={p ? runningPipelineIds?.has(p.id) : false} + /> + ); + })()} - <NodeConfigPanel - selectedNode={selectedNode} - pipelines={pipelines} - hasOutgoingEdge={selectedNodeHasOutgoingEdge} - hasIncomingAgentEdges={hasIncomingAgentEdges} - incomingTriggerEdges={incomingTriggerEdges} - onUpdateNode={onUpdateNode} - onUpdateEdgePrompt={onUpdateEdgePrompt} - onDeleteNode={onDeleteNode} - onSwitchToAgent={onSwitchToSession} - triggerDrawerOpen={triggerDrawerOpenForConfig} - agentDrawerOpen={agentDrawerOpenForConfig} - onTriggerPipeline={onTriggerPipeline} - pipelineName={(() => { - const p = pipelines.find((pl) => pl.nodes.some((n) => n.id === selectedNode.id)); - return p?.name; - })()} - isSaved={!isDirty} - isRunning={(() => { - const p = pipelines.find((pl) => pl.nodes.some((n) => n.id === selectedNode.id)); - return p ? runningPipelineIds?.has(p.id) : false; - })()} - />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx` around lines 378 - 387, The code performs the same pipelines.find(...) lookup twice to locate the pipeline that contains the selected node; refactor to compute that pipeline once and reuse it for pipelineName and isRunning. Specifically, create a local const (e.g., const selectedPipeline = pipelines.find(pl => pl.nodes.some(n => n.id === selectedNode.id))) near where onTriggerPipeline is passed, then replace both inline IIFEs for pipelineName and isRunning to reference selectedPipeline (use selectedPipeline?.name for pipelineName and runningPipelineIds?.has(selectedPipeline.id) for isRunning). This removes duplicate work while preserving existing behavior.src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx (1)
144-177: Consider using<button>elements for accessibility.Both the play button (lines 146-176) and gear icon (lines 180-200) use
<div>elements withonClickhandlers. This creates accessibility issues:
- Not focusable by default (no keyboard navigation)
- Screen readers won't announce them as interactive elements
- Missing ARIA roles
♻️ Suggested change for play button
- <div - onClick={(e) => { + <button + type="button" + onClick={(e) => { e.stopPropagation(); if (!data.isRunning) { data.onTriggerPipeline!(data.pipelineName!); } }} style={{ display: 'flex', alignItems: 'center', justifyContent: 'center', cursor: data.isRunning ? 'default' : 'pointer', color: data.isRunning ? '#22c55e' : `#22c55e90`, padding: '4px 4px', borderRadius: 4, transition: 'color 0.15s', + background: 'none', + border: 'none', }} ... - </div> + </button>Apply similar changes to the gear icon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx` around lines 144 - 177, Replace the clickable <div> used for the play control with a semantic <button type="button"> inside TriggerNode (the block using data.isSaved, data.onTriggerPipeline, data.pipelineName and data.isRunning); keep the existing inline styles and event behavior but move e.stopPropagation() into the button's onClick and set the button's disabled attribute when data.isRunning is true so keyboard and screen-reader users get correct behavior, add an accessible label (e.g. aria-label={`Run ${data.pipelineName}`}) and/or aria-disabled when appropriate, and preserve the onMouseEnter/onMouseLeave color changes (but avoid relying on pointer cursor styling for accessibility) so the visual behavior remains identical while making the control focusable and announced as a button.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/renderer/components/ProcessMonitor.test.tsx`:
- Around line 1759-1768: The test 'does not render CUE RUNS section when no cue
processes' currently asserts absence too early; update it to wait for the async
load to complete by first waiting for the regular process row to appear (e.g.
await waitFor(() => expect(screen.getByText(/* regularProc label
*/)).toBeInTheDocument()) or await waitForElementToBeRemoved loading indicator)
and only then assert that screen.queryByText('CUE RUNS') is not in the document;
reference the ProcessMonitor render call, the mocked
window.maestro.process.getActiveProcesses, and the createActiveProcess helper to
locate where to add the additional wait.
In `@src/main/cue/cue-engine.ts`:
- Around line 688-695: The hasTimeBasedSubscriptions function currently treats
any subscription with event 'time.heartbeat' or 'time.scheduled' as a scheduled
subscription even if it lacks required scheduling fields; update
hasTimeBasedSubscriptions (and the check of config.subscriptions) to only return
true for 'time.heartbeat' when sub.interval_minutes is a positive number and for
'time.scheduled' when sub.schedule_times is a non-empty array (while keeping the
existing checks for sub.enabled and agent_id === sessionId); this ensures only
subscriptions that will actually schedule timers cause the session to acquire
cue:schedule:* blockers.
- Around line 673-676: The sleep blocker is being set for the whole session in
cue-engine via this.deps.onPreventSleep(`cue:schedule:${session.id}`) and only
released at teardown; instead, move the prevent/allow calls to bracket actual
scheduling lifecycle: increment a session-level active-schedule counter when a
time-based subscription is successfully scheduled (in the code that sets up
`time.heartbeat` / `time.scheduled` subscriptions) and call
this.deps.onPreventSleep when the counter transitions 0->1, and decrement and
call this.deps.onAllowSleep when a scheduled job is cancelled and the counter
transitions 1->0 (use the same key `cue:schedule:${session.id}`). Also tighten
hasTimeBasedSubscriptions to mirror scheduling conditions by verifying required
fields for scheduling (e.g., ensure subscription.type === 'time.heartbeat' has a
valid interval and 'time.scheduled' has a cron/cron-like pattern) so it only
reports subscriptions that will actually create scheduled work.
In `@src/main/cue/cue-executor.ts`:
- Around line 337-344: The activeProcesses snapshot is storing raw spawnArgs
(the fully substituted Cue argv) which may contain sensitive prompt content;
change the code that sets activeProcesses.set(runId, {...}) to store a
redacted/display-safe arg list instead of spawnArgs. Implement or call a small
helper (e.g., redactSpawnArgs or buildSafeArgList) that strips or masks
sensitive values (prompt text, sourceOutput, task bodies, long strings) and
return a sanitized array, then use that sanitized variable in place of spawnArgs
when constructing the object for activeProcesses (leave child, command, cwd,
toolType, startTime unchanged).
- Around line 443-454: The escalation logic in stopCueRun uses
entry.child.killed which only indicates a signal was sent, not that the process
exited; replace this with an exit-state check (e.g., check entry.child.exitCode
!== null) or maintain an explicit flag updated in the child's 'exit' handler
(e.g., entry.exited) and use that flag instead of child.killed before sending
SIGKILL; update stopCueRun (and ensure the place that spawns the child sets
entry.exited on 'exit') and keep SIGKILL_DELAY_MS behavior otherwise.
In `@src/main/cue/cue-run-manager.ts`:
- Line 380: The wake-lock is being released via
deps.onAllowSleep?.(`cue:run:${runId}`) before the child process actually exits;
change the logic in cue-run-manager so the blocker is only cleared after the
run's promise settles or after awaiting the child termination. Specifically,
when calling stopRun() (and when reset() paths could leave a process running)
ensure you await the executor/child completion (the run promise returned by the
executor or an explicit wait-for-exit helper) and only call
deps.onAllowSleep?.(`cue:run:${runId}`) after that await (or after confirming
the process has been stopped), and apply the same change for the other release
site(s) around lines 432-434 so every active run is awaited/stopped before
releasing the wake lock.
In `@src/main/index.ts`:
- Around line 683-697: In getCueProcesses(), don't use cueEngine.isEnabled() or
cueEngine.getActiveRuns() as the sole source of truth; instead always return
entries from getCueProcessList() and enrich each proc by looking up run metadata
from a durable run manager or metadata store (use proc.runId to call your run
lookup function—e.g., runManager.getRunById or similar) falling back to existing
proc fields when lookup fails; avoid dropping session/subscription/stop IDs when
cueEngine is disabled or when runs have swapped to outputRunId so that
cue.stopRun(...) still has the needed IDs.
In `@src/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsx`:
- Around line 157-185: The Loader2 icon inside NodeConfigPanel's run button uses
an inline style animation ('style={{ animation: "spin 1s linear infinite" }}')
which relies on an undefined keyframe; replace that with Tailwind's animate-spin
class to match other components: remove the inline style on the Loader2 element
and add className="animate-spin" (ensure you update the JSX where Loader2 is
rendered within the conditional that shows the running state in the button so
the Play icon rendering remains unchanged).
In `@src/renderer/components/ProcessMonitor.tsx`:
- Around line 669-676: The group renderer is hardcoding the child-count noun to
"session", causing the CUE section (ProcessNode id 'cue-section' created in
ProcessMonitor.tsx) to display "1 session" instead of "1 run"; update the
ProcessNode object for 'cue-section' to include an overridable count label or
suppress flag (e.g., add a property like countLabel: 'run' or hideCount: true)
and then update the generic group renderer to honor that new ProcessNode
property when rendering the count (use countLabel to pluralize or skip rendering
when hideCount is true).
---
Nitpick comments:
In `@src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx`:
- Around line 144-177: Replace the clickable <div> used for the play control
with a semantic <button type="button"> inside TriggerNode (the block using
data.isSaved, data.onTriggerPipeline, data.pipelineName and data.isRunning);
keep the existing inline styles and event behavior but move e.stopPropagation()
into the button's onClick and set the button's disabled attribute when
data.isRunning is true so keyboard and screen-reader users get correct behavior,
add an accessible label (e.g. aria-label={`Run ${data.pipelineName}`}) and/or
aria-disabled when appropriate, and preserve the onMouseEnter/onMouseLeave color
changes (but avoid relying on pointer cursor styling for accessibility) so the
visual behavior remains identical while making the control focusable and
announced as a button.
In `@src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx`:
- Around line 378-387: The code performs the same pipelines.find(...) lookup
twice to locate the pipeline that contains the selected node; refactor to
compute that pipeline once and reuse it for pipelineName and isRunning.
Specifically, create a local const (e.g., const selectedPipeline =
pipelines.find(pl => pl.nodes.some(n => n.id === selectedNode.id))) near where
onTriggerPipeline is passed, then replace both inline IIFEs for pipelineName and
isRunning to reference selectedPipeline (use selectedPipeline?.name for
pipelineName and runningPipelineIds?.has(selectedPipeline.id) for isRunning).
This removes duplicate work while preserving existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec38fbcf-69bf-479c-b18e-4bcd1375a04a
📒 Files selected for processing (21)
src/__tests__/main/cue/cue-executor.test.tssrc/__tests__/main/cue/cue-sleep-prevention.test.tssrc/__tests__/renderer/components/CuePipelineEditor/nodes/TriggerNode.test.tsxsrc/__tests__/renderer/components/CuePipelineEditor/utils/pipelineGraph.test.tssrc/__tests__/renderer/components/ProcessMonitor.test.tsxsrc/main/cue/cue-engine.tssrc/main/cue/cue-executor.tssrc/main/cue/cue-run-manager.tssrc/main/index.tssrc/main/ipc/handlers/process.tssrc/main/power-manager.tssrc/main/preload/process.tssrc/renderer/components/CueModal/CueModal.tsxsrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CuePipelineEditor/PipelineCanvas.tsxsrc/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsxsrc/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsxsrc/renderer/components/CuePipelineEditor/utils/pipelineGraph.tssrc/renderer/components/ProcessMonitor.tsxsrc/renderer/components/Settings/tabs/GeneralTab.tsxsrc/renderer/global.d.ts
Comment on lines
+673
to
+676
| // Prevent system sleep if this session has time-based subscriptions | ||
| if (this.hasTimeBasedSubscriptions(config, session.id)) { | ||
| this.deps.onPreventSleep?.(`cue:schedule:${session.id}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Electron's powerSaveBlocker API, what is the behavioral difference between prevent-display-sleepandprevent-app-suspension, and does holding prevent-display-sleep continuously also keep the display from sleeping?
💡 Result:
In Electron's powerSaveBlocker API: - prevent-display-sleep: Prevents the display from going to sleep. Keeps both the system and screen active. Example use case: playing video. - prevent-app-suspension: Prevents the application from being suspended. Keeps the system active but allows the screen to be turned off. Example use cases: downloading a file or playing audio. prevent-display-sleep has higher precedence than prevent-app-suspension. Only the highest precedence type takes effect; if both are active, prevent-display-sleep is used. Holding prevent-display-sleep continuously does keep the display from sleeping, as it explicitly prevents the display from going to sleep and keeps the screen active.
Citations:
- 1: https://electronjs.org/docs/latest/api/power-save-blocker
- 2: https://github.com/electron/electron/blob/main/docs/api/power-save-blocker.md
- 3: https://www.electronjs.org/docs/latest/api/power-save-blocker
🏁 Script executed:
# First, let's check the relevant sections of cue-engine.ts
cat -n src/main/cue/cue-engine.ts | sed -n '66,120p'Repository: RunMaestro/Maestro
Length of output: 2482
🏁 Script executed:
# Check the sleep prevention logic around lines 673-703
cat -n src/main/cue/cue-engine.ts | sed -n '673,710p'Repository: RunMaestro/Maestro
Length of output: 1554
🏁 Script executed:
# Check the hasTimeBasedSubscriptions implementation
cat -n src/main/cue/cue-engine.ts | sed -n '688,700p'Repository: RunMaestro/Maestro
Length of output: 650
🏁 Script executed:
# Look at power-manager.ts to understand how the blocker is used
rg -A 10 "cue:schedule:" src/main/power-manager.tsRepository: RunMaestro/Maestro
Length of output: 502
🏁 Script executed:
# Search for onPreventSleep usage to understand the blocker type
rg -B 5 -A 5 "onPreventSleep" src/main/Repository: RunMaestro/Maestro
Length of output: 4157
Session-level sleep blocker is held for entire session lifetime, not just during subscription execution.
At lines 673–676, onPreventSleep is called once during session initialization if any enabled time.heartbeat or time.scheduled subscription exists. At lines 702–703, onAllowSleep is called only during session teardown. This means the sleep blocker cue:schedule:{sessionId} persists for the entire session lifetime—not just when scheduled subscriptions actually execute. A session with a single low-frequency heartbeat subscription will continuously prevent sleep until the session is torn down, even during long idle periods between executions.
Additionally, hasTimeBasedSubscriptions() (lines 688–695) checks only for event type presence; it does not validate that the subscription fields required for actual scheduling are present, so the helper is broader than the actual subscription-setup conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-engine.ts` around lines 673 - 676, The sleep blocker is
being set for the whole session in cue-engine via
this.deps.onPreventSleep(`cue:schedule:${session.id}`) and only released at
teardown; instead, move the prevent/allow calls to bracket actual scheduling
lifecycle: increment a session-level active-schedule counter when a time-based
subscription is successfully scheduled (in the code that sets up
`time.heartbeat` / `time.scheduled` subscriptions) and call
this.deps.onPreventSleep when the counter transitions 0->1, and decrement and
call this.deps.onAllowSleep when a scheduled job is cancelled and the counter
transitions 1->0 (use the same key `cue:schedule:${session.id}`). Also tighten
hasTimeBasedSubscriptions to mirror scheduling conditions by verifying required
fields for scheduling (e.g., ensure subscription.type === 'time.heartbeat' has a
valid interval and 'time.scheduled' has a cron/cron-like pattern) so it only
reports subscriptions that will actually create scheduled work.
| run.result.durationMs = Date.now() - new Date(run.result.startedAt).getTime(); | ||
|
|
||
| activeRuns.delete(runId); | ||
| deps.onAllowSleep?.(`cue:run:${runId}`); |
There was a problem hiding this comment.
Hold the Cue run wake lock until the child really exits.
At Lines 380 and 432-434, onAllowSleep fires before the executor has actually finished. stopRun() only asks the child to terminate, and reset() does not stop it at all. Disabling Cue or manually stopping a slow-to-exit run can therefore drop the cue:run:* blocker while the process is still alive. Keep the blocker until the run promise settles, or stop/await every active run before releasing it.
Also applies to: 432-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-run-manager.ts` at line 380, The wake-lock is being released
via deps.onAllowSleep?.(`cue:run:${runId}`) before the child process actually
exits; change the logic in cue-run-manager so the blocker is only cleared after
the run's promise settles or after awaiting the child termination. Specifically,
when calling stopRun() (and when reset() paths could leave a process running)
ensure you await the executor/child completion (the run promise returned by the
executor or an explicit wait-for-exit helper) and only call
deps.onAllowSleep?.(`cue:run:${runId}`) after that await (or after confirming
the process has been stopped), and apply the same change for the other release
site(s) around lines 432-434 so every active run is awaited/stopped before
releasing the wake lock.
…, UX fixes - Tighten hasTimeBasedSubscriptions to only match subscriptions with valid scheduling fields (interval_minutes > 0 for heartbeat, non-empty schedule_times for scheduled) - Redact sensitive prompt content from activeProcesses args via redactSpawnArgs() helper (truncates args >200 chars) - Fix stopCueRun SIGKILL escalation to check exitCode instead of .killed flag (which only indicates signal was sent, not process exit) - Remove cueEngine.isEnabled() gate from getCueProcesses so in-flight runs remain visible even after engine disable - Convert TriggerNode play <div> to semantic <button> with disabled, aria-label, and type="button" for keyboard/screen-reader accessibility - Replace inline spin animation with Tailwind animate-spin class in both NodeConfigPanel and TriggerNode Loader2 icons - Add countLabel to ProcessNode so CUE RUNS section shows "1 run" instead of "1 session" - Deduplicate pipelines.find() lookup in PipelineCanvas NodeConfigPanel props into single selectedPipeline const - Fix ProcessMonitor test to wait for async load before asserting absence of CUE RUNS section
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/cue/cue-executor.ts (2)
462-466:⚠️ Potential issue | 🟠 MajorDon't use
exitCodealone to decide whether SIGTERM already worked.Line 465 will also be true after a normal signal exit, because Node leaves
exitCodeasnulland setssignalCodeinstead. That means this timer can still sendSIGKILLafter the child is gone, and the staleChildProcesshandle can end up targeting a recycled PID.Safer escalation guard
setTimeout(() => { - if (entry.child.exitCode === null) { - entry.child.kill('SIGKILL'); - } + const current = activeProcesses.get(runId); + if (!current) return; + if (current.child.exitCode === null && current.child.signalCode === null) { + current.child.kill('SIGKILL'); + } }, SIGKILL_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-executor.ts` around lines 462 - 466, The timeout escalation currently checks only entry.child.exitCode and may send SIGKILL after a normal signal exit; change the guard in the setTimeout to confirm the child has not exited by verifying both entry.child.exitCode and entry.child.signalCode are null (and optionally that entry.child.killed is false) before calling entry.child.kill('SIGKILL'); update the check around the escalation in cue-executor.ts where entry.child is referenced to use these combined conditions so we don't target a recycled PID.
350-353:⚠️ Potential issue | 🟠 MajorLength-based truncation still exposes prompt content.
spawnArgscontains the substituted prompt for local runs, andredactSpawnArgs()only trims long values instead of removing the prompt payload. That still sends shortsourceOutput, task text, or GitHub bodies — plus the first 80 chars of longer prompts — over IPC into Process Monitor. Please store a display argv that replaces the prompt payload with a fixed placeholder instead of post-processing the raw argv.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/cue/cue-executor.ts` around lines 350 - 353, The stored args for activeProcesses currently pass the raw spawnArgs through redactSpawnArgs which only truncates long values and still leaks prompt content; change this to compute and store a safe display argv that replaces the prompt payload with a fixed placeholder (e.g. "<PROMPT_REDACTED>") before saving to activeProcesses: preserve the original spawnArgs for execution (child/command) but create a new masked array (e.g. buildDisplayArgs(spawnArgs) or modify redactSpawnArgs to detect the prompt-containing flag/name and substitute the entire prompt value with the placeholder) and use that masked array in the activeProcesses.set call instead of the current redactSpawnArgs(spawnArgs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx`:
- Around line 180-201: Replace the interactive <div> used for the gear icon with
a semantic <button> to restore keyboard accessibility: make the element a
<button> with type="button", move the onClick handler to the button, preserve
e.stopPropagation(), aria-label="Configure" (or title), and keep the style
properties (display, alignment, cursor, color, padding, borderRadius,
transition) and mouse enter/leave logic (or replace with :hover/:focus CSS) so
selected, color, data.onConfigure and data.compositeId behavior remain
identical; ensure the Settings component is still rendered inside the button and
that focus styles are visible to keyboard users.
---
Duplicate comments:
In `@src/main/cue/cue-executor.ts`:
- Around line 462-466: The timeout escalation currently checks only
entry.child.exitCode and may send SIGKILL after a normal signal exit; change the
guard in the setTimeout to confirm the child has not exited by verifying both
entry.child.exitCode and entry.child.signalCode are null (and optionally that
entry.child.killed is false) before calling entry.child.kill('SIGKILL'); update
the check around the escalation in cue-executor.ts where entry.child is
referenced to use these combined conditions so we don't target a recycled PID.
- Around line 350-353: The stored args for activeProcesses currently pass the
raw spawnArgs through redactSpawnArgs which only truncates long values and still
leaks prompt content; change this to compute and store a safe display argv that
replaces the prompt payload with a fixed placeholder (e.g. "<PROMPT_REDACTED>")
before saving to activeProcesses: preserve the original spawnArgs for execution
(child/command) but create a new masked array (e.g. buildDisplayArgs(spawnArgs)
or modify redactSpawnArgs to detect the prompt-containing flag/name and
substitute the entire prompt value with the placeholder) and use that masked
array in the activeProcesses.set call instead of the current
redactSpawnArgs(spawnArgs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f9b9bc35-2c7c-4bea-9d7c-1d10398c5f62
📒 Files selected for processing (8)
src/__tests__/renderer/components/ProcessMonitor.test.tsxsrc/main/cue/cue-engine.tssrc/main/cue/cue-executor.tssrc/main/index.tssrc/renderer/components/CuePipelineEditor/PipelineCanvas.tsxsrc/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsxsrc/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsxsrc/renderer/components/ProcessMonitor.tsx
✅ Files skipped from review due to trivial changes (1)
- src/tests/renderer/components/ProcessMonitor.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/components/CuePipelineEditor/panels/NodeConfigPanel.tsx
- src/renderer/components/CuePipelineEditor/PipelineCanvas.tsx
- src/main/index.ts
- src/renderer/components/ProcessMonitor.tsx
src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx
Outdated
Show resolved
Hide resolved
The process label includes the tab name (e.g. "- Tab 1") making exact string matches brittle. Wait for 'UNGROUPED AGENTS' instead — a stable group-level element that confirms the async process list has loaded.
- Convert TriggerNode gear icon <div> to semantic <button> with type="button", aria-label="Configure", and proper border/background reset for keyboard accessibility - Strengthen stopCueRun SIGKILL escalation guard to check both exitCode and signalCode are null before sending SIGKILL, preventing signals to a recycled PID after normal exit - Replace length-based arg redaction with prompt-aware buildDisplayArgs() that substitutes the exact prompt payload with <PROMPT_REDACTED>, preventing prompt content from leaking into Process Monitor display
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx (1)
181-205: Avoid rendering a no-op configure control when no handler exists.Right now the button still appears actionable even if
data.onConfigureis missing. Prefer conditional rendering (or disabled styling) to prevent dead clicks.Proposed tweak
- {/* Gear icon */} - <button - type="button" - onClick={(e) => { - e.stopPropagation(); - data.onConfigure?.(data.compositeId); - }} - aria-label="Configure" - style={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - cursor: 'pointer', - color: selected ? color : `${color}60`, - padding: 4, - borderRadius: 4, - border: 'none', - backgroundColor: 'transparent', - transition: 'color 0.15s', - }} - onMouseEnter={(e) => (e.currentTarget.style.color = color)} - onMouseLeave={(e) => (e.currentTarget.style.color = selected ? color : `${color}60`)} - title="Configure" - > - <Settings size={14} /> - </button> + {/* Gear icon */} + {data.onConfigure && ( + <button + type="button" + onClick={(e) => { + e.stopPropagation(); + data.onConfigure?.(data.compositeId); + }} + aria-label="Configure" + style={{ + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + cursor: 'pointer', + color: selected ? color : `${color}60`, + padding: 4, + borderRadius: 4, + border: 'none', + backgroundColor: 'transparent', + transition: 'color 0.15s', + }} + onMouseEnter={(e) => (e.currentTarget.style.color = color)} + onMouseLeave={(e) => (e.currentTarget.style.color = selected ? color : `${color}60`)} + title="Configure" + > + <Settings size={14} /> + </button> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx` around lines 181 - 205, The Configure button in TriggerNode.tsx is still rendered and interactive when data.onConfigure is undefined; change it to conditionally render the button only when data.onConfigure is present (or render a non-interactive/disabled element without pointer events, hover handlers and click behavior) so dead clicks cannot occur. Locate the button that calls data.onConfigure?.(data.compositeId) and either wrap it with a guard like if (data.onConfigure) render the button, or set aria-disabled/disabled styles and remove onClick/onMouseEnter/onMouseLeave handlers when data.onConfigure is falsy to prevent it appearing actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx`:
- Around line 181-205: The Configure button in TriggerNode.tsx is still rendered
and interactive when data.onConfigure is undefined; change it to conditionally
render the button only when data.onConfigure is present (or render a
non-interactive/disabled element without pointer events, hover handlers and
click behavior) so dead clicks cannot occur. Locate the button that calls
data.onConfigure?.(data.compositeId) and either wrap it with a guard like if
(data.onConfigure) render the button, or set aria-disabled/disabled styles and
remove onClick/onMouseEnter/onMouseLeave handlers when data.onConfigure is falsy
to prevent it appearing actionable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3924dc48-ba9a-4f7c-a91f-bf5f2992e595
📒 Files selected for processing (3)
src/__tests__/renderer/components/ProcessMonitor.test.tsxsrc/main/cue/cue-executor.tssrc/renderer/components/CuePipelineEditor/nodes/TriggerNode.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/cue/cue-executor.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
onPreventSleep/onAllowSleepthroughCueEngineandCueRunManagertoPowerManager, keeping the system awake during scheduled subscriptions and active Cue runsgetCueProcessList, merge with run metadata, and render a dedicated "CUE RUNS" section with event type badges, subscription labels, andcue.stopRunkill routingTest plan
cue.stopRun(notprocess.kill)npm run test— 1,505 new test lines across 5 files, all passingSummary by CodeRabbit
New Features
Bug Fixes