feat(cue): app.startup trigger, pipeline numbering fix, DB error surfacing#621
Merged
feat(cue): app.startup trigger, pipeline numbering fix, DB error surfacing#621
Conversation
…e vanishing
Problem 1 — Silent Cue enable failure (Linux):
CueEngine.start() caught better-sqlite3 DB init errors and returned
silently. The IPC call appeared to succeed, refresh() saw enabled=false,
and the toggle flipped back with zero feedback. Users on Linux where the
native binding was missing saw the toggle just snap back to "Disabled"
with no explanation.
Fix:
- src/main/cue/cue-engine.ts: change `return` → `throw error` in the
DB init catch block. Keeps existing Sentry capture and logging, but
now propagates to the IPC layer so the renderer gets a rejected promise.
- src/renderer/components/CueModal/CueModal.tsx: add catch block in
handleToggle that calls notifyToast with the error message. Users now
see exactly why enabling failed (e.g. "Could not locate the bindings
file").
Problem 2 — Pipeline nodes vanish during drag (Linux):
onNodesChange called setPipelineState on every mousemove during drag.
Each update triggered a full convertToReactFlowNodes recomputation via
useMemo, creating a tight render loop. On Linux with slower compositing,
this caused nodes to flicker or disappear entirely.
Fix:
- src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx:
buffer drag position updates in a ref and flush once per animation
frame via requestAnimationFrame. Non-drag position changes (drop,
programmatic moves) still commit immediately. Cleanup cancels any
pending frame on unmount.
Tests updated:
- src/__tests__/main/cue/cue-engine.test.ts: 5 tests that called
engine.start() expecting silent failure now use expect().toThrow()
- src/__tests__/main/cue/cue-sleep-wake.test.ts: 1 test updated
from .not.toThrow() to .toThrow('DB init failed')
Affected surfaces:
- Cue enable/disable toggle in CueModal (all platforms)
- Pipeline canvas drag-and-drop (primarily Linux, no regression on macOS)
- IPC handler cue:enable now returns rejected promise on DB failure
- No changes to stats-db, build pipeline, or other features
Add `app.startup` as the 8th Cue event type, firing once per application launch to run initialization workflows (workspace setup, dep installs, health checks). System startup semantics: - Fires only on app launch (isSystemBoot=true), not when toggling Cue on/off - Deduplicates across YAML hot-reloads via startupFiredKeys Set - Keys persist across engine stop/start cycles (feature toggling) - Resets on session removal + next app restart - Full interop with fan-out, filters, output_prompt, chaining Pipeline numbering fix: - createPipeline now derives next number from highest existing pipeline name instead of array length, preventing duplicate names after deletion - Remove unused agentCmd variable in agentStore (lint warning) 22 files changed, 19 new startup tests, 5 yaml validation tests, 1 pipeline numbering test. 23,883 tests passing.
Adds `/// <reference types="@testing-library/jest-dom/vitest" />` to resolve TS errors for toBeInTheDocument, toHaveStyle, toHaveAttribute matchers that were missing type augmentation.
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as Maestro App
participant Engine as CueEngine
participant DB as Cue Database
participant Loader as YAML Loader
participant Reconciler as Reconciler
participant Handler as onCueRun Handler
App->>Engine: start(isSystemBoot=true)
activate Engine
Engine->>DB: initialize()
DB-->>Engine: ready
Engine->>Loader: loadCueConfig()
Loader-->>Engine: subscriptions[]
Note over Engine: Set isBootScan = true
Engine->>Engine: initSession(sessionId)
activate Engine
alt isBootScan && subscription.enabled && agent/session match
Engine->>Engine: check startupFiredKeys (sessionId:subName)
alt not fired
Engine->>Reconciler: payload/filter match?
Reconciler-->>Engine: matches
Engine->>Handler: onCueRun(sessionId, subName, prompt..., payload(reason: "system_startup"))
Handler-->>Engine: run result
Engine->>Engine: add to startupFiredKeys
else already fired
Engine->>Engine: skip (dedup)
end
else disabled or filter mismatch
Engine->>Engine: skip
end
deactivate Engine
Engine->>Engine: clear isBootScan
deactivate Engine
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)
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 a new
All other changes are clean: the pipeline numbering fix is correct, the RAF-throttled drag buffering includes a proper unmount cleanup, error toasts in Confidence Score: 4/5Safe to merge after adding a try/catch around cueEngine.start(true) in index.ts; the isSystemBoot flag issue is a behavioral edge case worth tracking but does not break the primary user path. The PR is well-tested (19 startup tests, full suite green) and the core feature logic is sound. One targeted fix remains: the missing try/catch around the throwing start() call at app boot. The isSystemBoot flag issue is a real semantic inconsistency with the docs but affects only the edge case of adding a brand-new session on a running engine, not the primary startup-fires-once flow. src/main/index.ts (missing try/catch) and src/main/cue/cue-engine.ts (isSystemBoot never resets) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[app.whenReady] --> B{maestroCue flag?}
B -- yes --> C["cueEngine.start(true)\nisSystemBoot = true"]
B -- no --> Z[End]
C --> D[initCueDb]
D -- throws --> E["throw error ⚠️\n(no try/catch in index.ts)"]
D -- ok --> F[enabled = true]
F --> G[syncSession for each session]
G --> H{app.startup sub?}
H -- no --> I[Other event watchers]
H -- yes --> J{isSystemBoot?}
J -- false --> K[Skip — user toggle]
J -- true --> L{firedKey in\nstartupFiredKeys?}
L -- yes --> M[Skip — dedup]
L -- no --> N[Add firedKey\nDispatch prompt]
O[User toggles Cue off] --> P["stop()\nstartupFiredKeys NOT cleared\nisSystemBoot stays true ⚠️"]
P --> Q["start(false)\nsyncs existing sessions\n→ firedKeys prevent re-fire ✓"]
Q --> R["addSession while running\n→ syncSession called\n→ isSystemBoot=true + no firedKey\n→ fires immediately ⚠️"]
S[removeSession] --> T[Clear firedKeys for session]
T --> U[Next app restart fires again]
Reviews (1): Last reviewed commit: "fix(test): add jest-dom vitest type refe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
530
to
533
| if (encoreFeatures.maestroCue && cueEngine) { | ||
| logger.info('Maestro Cue Encore Feature enabled — starting Cue engine', 'Startup'); | ||
| cueEngine.start(); | ||
| cueEngine.start(true); | ||
| } |
There was a problem hiding this comment.
cueEngine.start() now throws on DB init failure (changed in this PR from return to throw error), but this call site has no try/catch. Every other initialization block in this function (history manager, stats DB) is wrapped in a try/catch that logs and continues. An uncaught exception in the app.whenReady().then(async () => {...}) callback becomes an unhandled Promise rejection that can crash the Electron process on startup.
Suggested change
| if (encoreFeatures.maestroCue && cueEngine) { | |
| logger.info('Maestro Cue Encore Feature enabled — starting Cue engine', 'Startup'); | |
| cueEngine.start(); | |
| cueEngine.start(true); | |
| } | |
| if (encoreFeatures.maestroCue && cueEngine) { | |
| logger.info('Maestro Cue Encore Feature enabled — starting Cue engine', 'Startup'); | |
| try { | |
| cueEngine.start(true); | |
| } catch (error) { | |
| logger.error(`Failed to start Cue engine: ${error}`, 'Startup'); | |
| } | |
| } |
Comment on lines
+152
to
157
| /** Enable the engine and scan all sessions for Cue configs. | ||
| * @param isSystemBoot Pass `true` only at application launch (index.ts). When false (default), | ||
| * app.startup subscriptions will NOT fire — this prevents re-firing when the user toggles Cue on/off. */ | ||
| start(isSystemBoot = false): void { | ||
| if (this.enabled) return; | ||
|
|
There was a problem hiding this comment.
isSystemBoot never resets — mid-lifecycle addSession fires startup immediately
isSystemBoot is set to true at app launch and is never reset to false (not in stop(), not anywhere). This means any session added via addSession() while the engine is already running will immediately dispatch its app.startup subscriptions on the same call to syncSession — because isSystemBoot === true and the new session's key is absent from startupFiredKeys.
The documentation explicitly states: "Resets on session removal (so re-adding the session fires again on next app launch)." Under the current logic, re-adding (or newly adding) a session mid-run fires startup right away, contradicting that guarantee.
A simple fix is to not use isSystemBoot as a long-lived flag and instead pass the boot context into syncSession/loadSessionInner only for the sessions that are present at actual boot time:
// In start():
const bootSessions = isSystemBoot ? new Set(this.sessions.keys()) : new Set<string>();
// Pass bootSessions down to loadSessionInner, and only fire app.startup
// when the session id is in bootSessions.This scopes the "fire on boot" logic to the sessions that existed at startup and avoids the always-true flag leaking into later addSession calls.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx (1)
306-360:⚠️ Potential issue | 🟠 MajorCancel the pending drag frame before applying the final drop position.
If the non-dragging
positionevent arrives before the scheduled RAF flush runs, the stale buffered coordinates can still flush afterward and overwrite the final drop location. Merge or clear the buffer before entering the non-drag path.Possible fix
if (isDragging && positionUpdates.size > 0) { if (!dragBufferRef.current) dragBufferRef.current = new Map(); for (const [id, pos] of positionUpdates) { dragBufferRef.current.set(id, pos); } if (rafIdRef.current === null) { rafIdRef.current = requestAnimationFrame(flushDragBuffer); } return; } + + if (rafIdRef.current !== null) { + cancelAnimationFrame(rafIdRef.current); + rafIdRef.current = null; + } + for (const [id, pos] of dragBufferRef.current ?? []) { + if (!positionUpdates.has(id)) positionUpdates.set(id, pos); + } + dragBufferRef.current = null; if (positionUpdates.size > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx` around lines 306 - 360, Before applying the non-drag positionUpdates path, cancel any pending RAF and merge/clear the drag buffer so stale buffered coordinates cannot flush afterward: if rafIdRef.current is not null, call cancelAnimationFrame(rafIdRef.current) and set rafIdRef.current = null, then if dragBufferRef.current exists merge its entries into the current positionUpdates (or clear it) so setPipelineState sees the final positions; ensure this logic runs just before the existing positionUpdates.size > 0 branch (affecting isDragging, dragBufferRef, rafIdRef, flushDragBuffer, and setPipelineState) and preserve the existing Y-offset adjustment using stableYOffsetsRef/current convertToReactFlowNodes semantics.src/main/cue/cue-engine.ts (1)
166-175:⚠️ Potential issue | 🔴 CriticalBoot-time DB failures now crash the Electron main process.
Re-throwing here is fine for the IPC enable path, but the app-launch caller in
src/main/index.ts(Lines 528-559) still invokescueEngine.start(true)without a catch. A DB init failure will now abort startup before the renderer can surface anything to the user.🤖 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 166 - 175, The catch block in CueEngine initialization currently re-throws the DB init error (in the catch inside cue-engine.ts), which causes the Electron main process to crash when called by cueEngine.start(true); stop re-throwing here and instead fail gracefully: after logging and captureException, return a rejected/failed start result or false (or set an internal failed state) so the caller (cueEngine.start) can handle it without aborting the whole process; update the catch in the async DB init path (the block that logs "[CUE] Failed to initialize Cue database — engine will not start" and calls captureException) to remove the final "throw error" and return/resolve a safe failure value and ensure CueEngine.start(true) will not crash the process.
🧹 Nitpick comments (1)
src/renderer/components/CueModal/CueModal.tsx (1)
103-108: Fallback error message doesn't account for disable operation.The fallback
'Failed to enable Cue engine'is used even when the user is attempting to disable Cue (whenisEnabledis true). While the fallback is rarely hit since DB errors areErrorinstances, the message should be context-aware for correctness.♻️ Proposed fix
} catch (err) { notifyToast({ type: 'error', title: 'Cue', - message: err instanceof Error ? err.message : 'Failed to enable Cue engine', + message: err instanceof Error ? err.message : `Failed to ${isEnabled ? 'disable' : 'enable'} Cue engine`, }); } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CueModal/CueModal.tsx` around lines 103 - 108, The catch block in CueModal.tsx calls notifyToast with a fallback message that always says "Failed to enable Cue engine"; update that fallback to be context-aware using the existing isEnabled flag (when isEnabled is true the user is disabling, so use "Failed to disable Cue engine", otherwise "Failed to enable Cue engine") in the notifyToast message inside the catch (where notifyToast is called) so the ternary becomes err instanceof Error ? err.message : (isEnabled ? 'Failed to disable Cue engine' : 'Failed to enable Cue engine').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/maestro-cue-configuration.md`:
- Line 60: The doc has inconsistent event count references: the table row for
the `event` field currently states "One of the eight event types" while the
"Validation" section still references seven; update both places to the correct
canonical count (either seven or eight) so they match, and verify the linked
resource referenced by ./maestro-cue-events matches that same count; edit the
table line containing `event` and the "Validation" section text to the agreed
number and ensure the wording is consistent across the document.
In `@docs/maestro-cue-events.md`:
- Line 13: The documentation line claiming "Required fields: None beyond the
universal `name`, `event`, and `prompt`" is inaccurate because `prompt_file` is
a valid alternative to `prompt` (e.g., `app.startup` uses `prompt_file` without
inline `prompt`); update that sentence to list `prompt` or `prompt_file` as the
universal field(s) (for example: "`name`, `event`, and either `prompt` or
`prompt_file`") and ensure examples like `app.startup` are consistent with this
wording so readers know `prompt_file` is acceptable.
In `@src/main/cue/cue-engine.ts`:
- Around line 158-160: The field this.isSystemBoot is being set true on any
start(true) call and left true for the process, causing later
refreshSession/app.startup handlers to behave like boot-time init; change the
logic so the "system boot" flag only applies to the initial startup scan: record
whether the engine has already seen a boot (e.g. add or use a hasBooted boolean)
and set this.isSystemBoot = true only when isSystemBoot is true AND hasBooted is
false (or set this.isSystemBoot to true temporarily for the startup flow then
immediately clear it and set hasBooted = true); update the same pattern for the
other boot-setting site referenced (the code around the start/boot flow at the
other block) so subsequent refreshSession(), re-adds, or app.startup
subscriptions do not see isSystemBoot as true.
In `@src/main/index.ts`:
- Line 532: Wrap the call to cueEngine.start(true) in a try-catch so a thrown
error does not block app startup; catch the error, log it (using the existing
logger/processLogger) and continue startup so the window and other subsystems
are created, leaving the cueEngine instance intact for later retries via the
"cue:enable" IPC handler; ensure the catch does not swallow errors silently but
records enough context to diagnose initialization failures.
In `@src/renderer/components/CueHelpModal.tsx`:
- Around line 100-103: The "Startup" example in the CueHelpModal component shows
an invalid subscription shape by omitting the globally required
prompt/prompt_file fields; update the example code block inside the CueHelpModal
(the Startup example near the paragraph with className "mt-1") to include either
prompt or prompt_file in the YAML so it validates, and optionally clarify the
adjacent text if necessary (keep the sentence "No additional fields required"
but ensure the example contains the required global prompt/prompt_file to avoid
misleading copy/paste).
---
Outside diff comments:
In `@src/main/cue/cue-engine.ts`:
- Around line 166-175: The catch block in CueEngine initialization currently
re-throws the DB init error (in the catch inside cue-engine.ts), which causes
the Electron main process to crash when called by cueEngine.start(true); stop
re-throwing here and instead fail gracefully: after logging and
captureException, return a rejected/failed start result or false (or set an
internal failed state) so the caller (cueEngine.start) can handle it without
aborting the whole process; update the catch in the async DB init path (the
block that logs "[CUE] Failed to initialize Cue database — engine will not
start" and calls captureException) to remove the final "throw error" and
return/resolve a safe failure value and ensure CueEngine.start(true) will not
crash the process.
In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx`:
- Around line 306-360: Before applying the non-drag positionUpdates path, cancel
any pending RAF and merge/clear the drag buffer so stale buffered coordinates
cannot flush afterward: if rafIdRef.current is not null, call
cancelAnimationFrame(rafIdRef.current) and set rafIdRef.current = null, then if
dragBufferRef.current exists merge its entries into the current positionUpdates
(or clear it) so setPipelineState sees the final positions; ensure this logic
runs just before the existing positionUpdates.size > 0 branch (affecting
isDragging, dragBufferRef, rafIdRef, flushDragBuffer, and setPipelineState) and
preserve the existing Y-offset adjustment using stableYOffsetsRef/current
convertToReactFlowNodes semantics.
---
Nitpick comments:
In `@src/renderer/components/CueModal/CueModal.tsx`:
- Around line 103-108: The catch block in CueModal.tsx calls notifyToast with a
fallback message that always says "Failed to enable Cue engine"; update that
fallback to be context-aware using the existing isEnabled flag (when isEnabled
is true the user is disabling, so use "Failed to disable Cue engine", otherwise
"Failed to enable Cue engine") in the notifyToast message inside the catch
(where notifyToast is called) so the ternary becomes err instanceof Error ?
err.message : (isEnabled ? 'Failed to disable Cue engine' : 'Failed to enable
Cue engine').
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e46087db-9f07-4e5c-afcd-3576c2b5e369
📒 Files selected for processing (26)
docs/maestro-cue-configuration.mddocs/maestro-cue-events.mddocs/maestro-cue-examples.mdsrc/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/cue/cue-sleep-wake.test.tssrc/__tests__/main/cue/cue-startup.test.tssrc/__tests__/main/cue/cue-yaml-loader.test.tssrc/__tests__/renderer/components/CuePipelineEditor/drawers/TriggerDrawer.test.tsxsrc/__tests__/renderer/hooks/cue/usePipelineState.test.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-types.tssrc/main/cue/cue-yaml-loader.tssrc/main/index.tssrc/renderer/components/CueHelpModal.tsxsrc/renderer/components/CueModal/CueModal.tsxsrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CuePipelineEditor/cueEventConstants.tssrc/renderer/components/CuePipelineEditor/drawers/TriggerDrawer.tsxsrc/renderer/components/CuePipelineEditor/panels/triggers/TriggerConfig.tsxsrc/renderer/constants/cuePatterns.tssrc/renderer/constants/cueYamlDefaults.tssrc/renderer/hooks/cue/useCueAiChat.tssrc/renderer/hooks/cue/usePipelineState.tssrc/renderer/stores/agentStore.tssrc/shared/cue-pipeline-types.tssrc/shared/templateVariables.ts
💤 Files with no reviewable changes (1)
- src/renderer/stores/agentStore.ts
| | -------- | ------ | ---------------------------------------------------------------------- | | ||
| | `name` | string | Unique identifier. Used in logs, history, and as a reference in chains | | ||
| | `event` | string | One of the seven [event types](./maestro-cue-events) | | ||
| | `event` | string | One of the eight [event types](./maestro-cue-events) | |
There was a problem hiding this comment.
Event count wording is now inconsistent within this doc.
Line 60 says eight event types, but the Validation section still says seven. Please align both references to avoid contradictory docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/maestro-cue-configuration.md` at line 60, The doc has inconsistent event
count references: the table row for the `event` field currently states "One of
the eight event types" while the "Validation" section still references seven;
update both places to the correct canonical count (either seven or eight) so
they match, and verify the linked resource referenced by ./maestro-cue-events
matches that same count; edit the table line containing `event` and the
"Validation" section text to the agreed number and ensure the wording is
consistent across the document.
Engine: - Replace persistent isSystemBoot with scoped isBootScan that is set true only during the start() session scan and cleared immediately after, preventing later refreshSession/auto-discovery from firing app.startup subs - DB init failure now returns gracefully instead of throwing, so the Electron main process is never blocked at startup - Wrap cueEngine.start(true) in try-catch at index.ts boot site UI: - CueModal toggle error toast is now context-aware (enable vs disable) - CueHelpModal startup quick-reference example includes prompt field - CuePipelineEditor cancels pending RAF and merges drag buffer before applying non-drag position updates to prevent stale flush Docs: - Fix inconsistent event count in configuration validation section - Clarify prompt_file as alternative to prompt in events doc
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/index.ts (1)
532-539: Consider adding Sentry capture for unexpected boot failures.The try/catch correctly prevents Cue initialization failures from blocking app startup. However, while DB init errors are already captured in
cue-engine.ts, any other unexpected errors thrown fromstart()(e.g., future code changes) would only be logged here without Sentry visibility.Per coding guidelines, unexpected errors should reach Sentry. Consider adding
captureExceptionfor non-recoverable errors:♻️ Optional: Add Sentry capture for completeness
if (encoreFeatures.maestroCue && cueEngine) { logger.info('Maestro Cue Encore Feature enabled — starting Cue engine', 'Startup'); try { cueEngine.start(true); } catch (err) { logger.error( `Cue engine failed to start at boot — will remain available for retry via Settings: ${err}`, 'Startup' ); + // Report to Sentry for visibility into non-DB-init failures + import('./utils/sentry').then(({ captureException }) => { + captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { operation: 'cue.startBoot' }, + }); + }); } }Note: This is optional since DB init errors (the most likely failure) are already captured in
cue-engine.ts. This would catch edge cases from future code changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/index.ts` around lines 532 - 539, Add a Sentry capture alongside the existing logger.error inside the catch for cueEngine.start(true) so unexpected boot-time exceptions are sent to Sentry; update the catch block that currently calls logger.error(`Cue engine failed to start... ${err}`, 'Startup') to also call Sentry.captureException(err) (or the app's sentry client) after ensuring Sentry is imported/initialized and that the captured value is the original error object rather than a string so stack/context are preserved.
🤖 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/main/index.ts`:
- Around line 532-539: Add a Sentry capture alongside the existing logger.error
inside the catch for cueEngine.start(true) so unexpected boot-time exceptions
are sent to Sentry; update the catch block that currently calls
logger.error(`Cue engine failed to start... ${err}`, 'Startup') to also call
Sentry.captureException(err) (or the app's sentry client) after ensuring Sentry
is imported/initialized and that the captured value is the original error object
rather than a string so stack/context are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df46670c-68f2-4c81-bce3-e4607772f618
📒 Files selected for processing (8)
docs/maestro-cue-configuration.mddocs/maestro-cue-events.mdsrc/__tests__/main/cue/cue-sleep-wake.test.tssrc/main/cue/cue-engine.tssrc/main/index.tssrc/renderer/components/CueHelpModal.tsxsrc/renderer/components/CueModal/CueModal.tsxsrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx
✅ Files skipped from review due to trivial changes (3)
- docs/maestro-cue-configuration.md
- src/renderer/components/CueHelpModal.tsx
- docs/maestro-cue-events.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/main/cue/cue-sleep-wake.test.ts
- src/renderer/components/CueModal/CueModal.tsx
- mergePipelinesWithSavedLayout now restores saved color and name from the layout file when pipeline IDs match, preventing colors/names from being re-derived by parse order on every reload - Add fitView effect on selectedPipelineId change so switching from All Pipelines to a single pipeline (or vice versa) auto-centers the viewport on the visible nodes instead of leaving it at the stacked Y-offset position
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/CuePipelineEditor.tsx`:
- Around line 241-251: The effect in CuePipelineEditor that calls
reactFlowInstance.fitView on selection changes is overwriting a persisted
viewport restored by usePipelineLayout; change the useEffect (which references
prevSelectedIdRef, pipelineState.selectedPipelineId, reactFlowInstance) to skip
the auto-fit for the initial hydrated selection: add a simple guard/ref (e.g.,
hasHydratedSelectionRef or check pipelineLayout.viewport from usePipelineLayout)
so that when the editor mounts and a saved viewport exists you do not call
fitView, and only allow the delayed fitView for subsequent user-initiated
pipeline switches; set the guard after the first selection change to re-enable
auto-fit thereafter.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 665dae6c-db14-47d1-aaf7-fb11dca5e9b2
📒 Files selected for processing (3)
src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.tssrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CuePipelineEditor/utils/pipelineLayout.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.ts
…port Add hasHydratedSelectionRef guard so the fitView effect skips the initial selectedPipelineId change from layout restoration, preventing it from overwriting the saved viewport restored by usePipelineLayout. Subsequent user-initiated pipeline switches still trigger fitView.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx (1)
243-260:⚠️ Potential issue | 🟡 MinorThis still skips the first real pipeline switch.
If the editor opens in “All Pipelines” and there is no layout-driven selection hydration, the first user selection also falls into the Line 250 skip branch, so
fitView()never runs for that switch. The guard needs to arm on the initial render/load state, not on the firstselectedPipelineIdchange.Possible fix
const prevSelectedIdRef = useRef(pipelineState.selectedPipelineId); const hasHydratedSelectionRef = useRef(false); useEffect(() => { - if (prevSelectedIdRef.current === pipelineState.selectedPipelineId) return; - prevSelectedIdRef.current = pipelineState.selectedPipelineId; - - // Skip the initial hydration — let usePipelineLayout restore the saved viewport if (!hasHydratedSelectionRef.current) { - hasHydratedSelectionRef.current = true; + hasHydratedSelectionRef.current = pipelineState.pipelines.length > 0; + prevSelectedIdRef.current = pipelineState.selectedPipelineId; return; } + if (prevSelectedIdRef.current === pipelineState.selectedPipelineId) return; + prevSelectedIdRef.current = pipelineState.selectedPipelineId; + // Short delay so ReactFlow has rendered the new node set before fitting const timer = setTimeout(() => { reactFlowInstance.fitView({ padding: 0.15, duration: 200 }); }, 50); return () => clearTimeout(timer); -}, [pipelineState.selectedPipelineId, reactFlowInstance]); +}, [pipelineState.pipelines.length, pipelineState.selectedPipelineId, reactFlowInstance]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx` around lines 243 - 260, The current effect uses hasHydratedSelectionRef to skip the very first selection change, but that also skips the first real user switch when the editor starts in “All Pipelines”; change the guard so it only skips the initial render/load, not the first subsequent selection change: use prevSelectedIdRef (which stores pipelineState.selectedPipelineId) to detect initial uninitialized state (e.g., prevSelectedIdRef.current == null/undefined) and on that first run set prevSelectedIdRef.current and return, otherwise call reactFlowInstance.fitView on subsequent pipelineState.selectedPipelineId changes; update the useEffect that references prevSelectedIdRef, hasHydratedSelectionRef, pipelineState.selectedPipelineId, and reactFlowInstance.fitView accordingly and remove or repurpose hasHydratedSelectionRef.
🤖 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/CuePipelineEditor.tsx`:
- Around line 278-305: The flushDragBuffer handler applies buffered node
positions using stableYOffsetsRef.current, but those offsets can be stale after
the final drop; modify flushDragBuffer to, after creating newPipelines (the
updated nodes), call computePipelineYOffsets(newPipelines) to recompute the
stacked Y offsets and update stableYOffsetsRef.current (and any derived state)
so downstream pipelines are re-spaced; do this only in flushDragBuffer (the
final non-drag commit) so offsets remain frozen during drag but refresh once
after the drop. Include references: flushDragBuffer, stableYOffsetsRef,
computePipelineYOffsets, and the setPipelineState update that builds
newPipelines.
---
Duplicate comments:
In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx`:
- Around line 243-260: The current effect uses hasHydratedSelectionRef to skip
the very first selection change, but that also skips the first real user switch
when the editor starts in “All Pipelines”; change the guard so it only skips the
initial render/load, not the first subsequent selection change: use
prevSelectedIdRef (which stores pipelineState.selectedPipelineId) to detect
initial uninitialized state (e.g., prevSelectedIdRef.current == null/undefined)
and on that first run set prevSelectedIdRef.current and return, otherwise call
reactFlowInstance.fitView on subsequent pipelineState.selectedPipelineId
changes; update the useEffect that references prevSelectedIdRef,
hasHydratedSelectionRef, pipelineState.selectedPipelineId, and
reactFlowInstance.fitView accordingly and remove or repurpose
hasHydratedSelectionRef.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4413b952-5bf6-4af1-b396-f3ad151e08d1
📒 Files selected for processing (1)
src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx
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
app.startuptrigger — New 8th Cue event type that fires once per application launch for initialization workflows (workspace setup, dependency installs, health checks). System-startup semantics: does not re-fire on Cue feature toggle or YAML hot-reload, only on app restart. Full interop with fan-out, filters, chaining, and SSH.createPipelinenow derives the next number from the highest existing pipeline name instead of array length, preventing duplicate names after deletions (e.g., deleting Pipeline 2 then creating a new one no longer produces a second "Pipeline 2").agentCmdvariable (lint warning), add jest-dom vitest type reference to TriggerDrawer test.Test plan
app.startuptests covering: system boot vs feature toggle, dedup on hot-reload, stop/start persistence, removeSession reset, enabled/agent_id/filter gating, fan-out, chaining, multi-sub, multi-session, payload, sleep prevention, loggingapp.startupSummary by CodeRabbit
New Features
Bug Fixes
Enhancements
Documentation
Tests