cue: pipeline persistence + per-root partitioning fixes, save UX hardening, frontend IPC service refactor#837
Merged
reachrazamair merged 6 commits intorcfrom Apr 14, 2026
Merged
Conversation
backend: - add safeRecordCueEvent/safeUpdateCueEventStatus wrappers in cue-db.ts; migrate 3 silent try/catch blocks in cue-run-manager.ts - add idempotency guard to initSession() — tears down existing registration before re-registering to prevent duplicate trigger sources - new CueCleanupService: periodic sweep (every 10 heartbeat ticks) evicts stale fan-in trackers (removed sessions, > 2× timeout) and stale time.scheduled dedup keys; wired via CueHeartbeat.onTick - extend CueFanInTracker with getActiveTrackerKeys/getTrackerCreatedAt/ expireTracker + fanInCreatedAt map - extend CueSessionRegistry with sweepStaleScheduledKeys frontend: - new src/renderer/services/cue.ts: sole owner of window.maestro.cue.* calls; reads return safe defaults, writes rethrow (createIpcMethod pattern matching git.ts) - migrate 8 renderer files from window.maestro.cue.* to cueService.* - new cueDirtyStore (Zustand): eliminates onDirtyChange prop chain and pipelineDirtyRef hack; CueModal reads via getState() — always fresh - add cleanup effect in CueModal to reset dirty flags on unmount Tests: +65 new tests across 7 new/extended test files.
- pruneOrphanedPromptFiles: after cue:writeYaml, delete .md files under .maestro/prompts/ that the new YAML no longer references. Handles pipeline/agent renames and deletions so stale prompt files don't accumulate. - cue-query-service: only report subscriptions whose agent_id matches the queried session (or is absent/shared). Previously every session sharing a project root surfaced every subscription, causing cross-session bleed in the Dashboard. - yamlToPipeline: trust explicit agent_id when resolving target session instead of falling back to name match. Per-project-root YAML partitioning makes agent_id the source of truth; a coincidental pipeline-name/session-name overlap must not flip the resolved agent. Fixes "Maestro swap reverts" bug where replacing an agent would snap back to the original on reload.
…in banner
Fixes a cluster of Cue pipeline editor bugs around persistence and rendering:
- Pipeline vanish after save: createPipeline assigned timestamp ids; on
reload subscriptionsToPipelines regenerated name-based ids, leaving the
saved selectedPipelineId stale and convertToReactFlowNodes skipping every
pipeline. mergePipelinesWithSavedLayout now validates the saved selection
against the live pipeline ids and falls back to the first pipeline; a
live safety-net effect in usePipelineState resets selectedPipelineId to
null whenever it points at a pipeline that no longer exists.
- Save silently doing nothing: validatePipelines used to skip empty
pipelines, so 'create N pipelines, click save' returned success without
writing anything. Empty pipelines are now flagged ('add a trigger and an
agent before saving') and handleSave refuses to no-op when the editor
has pipelines but nothing partitions to a root.
- Lost-on-save trust gap: handleSave now write-back-verifies every
cue:writeYaml by reading the file and comparing bytes, throws on
mismatch, preserves isDirty on failure, and fires explicit success/
error toast notifications so the 2-second in-button flash can no longer
be missed.
- Trigger config caught at load instead of save: pipelineToYaml was happy
to write a time.scheduled subscription with no schedule_times (or
time.heartbeat with no interval_minutes, etc.), the loader then rejected
the entire YAML for every agent in that project root on Cue toggle.
validatePipelines now mirrors the YAML schema's per-event requirements,
blocking the bad save up front.
- One bad subscription killing a whole project's config: extracted
validateSubscription and added partitionValidSubscriptions; loader now
drops individual invalid subs as warnings instead of failing the entire
load. Config-level errors (missing subscriptions array, bad settings)
remain fatal.
- Codex 'Reading additional input from stdin...' in run output: cue
child processes spawn with stdio[0]='ignore' in local mode so codex
exec doesn't emit the stdin banner before observing EOF. SSH stdin
script and SSH small-prompt paths still get a writable pipe.
Also includes editor polish from the working branch: pendingSavedViewportRef
threaded from usePipelineLayout to CuePipelineEditor so viewport restore
waits for ReactFlow to measure nodes (no more empty canvas on first open),
plus PipelineCanvas + AllPipelinesView locking tweaks and matching tests.
Tests: 25,260 passing.
📝 WalkthroughWalkthroughAdds a heartbeat-driven Cue cleanup service (fan‑in expiry and scheduled-key sweep), safe DB wrappers, lenient per‑subscription validation and loader, renderer-side Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Heartbeat as Heartbeat Loop
participant Cleanup as Cleanup Service
participant FanIn as Fan‑In Tracker
participant Registry as Session Registry
participant Log as Logger
Heartbeat->>Cleanup: onTick()
Note over Cleanup: increment tick counter
alt tick >= CLEANUP_INTERVAL_TICKS
Cleanup->>FanIn: getActiveTrackerKeys()
FanIn-->>Cleanup: [keys]
loop per key
Cleanup->>FanIn: getTrackerCreatedAt(key)
FanIn-->>Cleanup: timestamp or undefined
Cleanup->>Registry: getSessions() / getSessionTimeoutMs(owner)
alt owner not present OR age > 2×timeout
Cleanup->>FanIn: expireTracker(key)
Cleanup->>Log: warn("evicted fan-in key")
end
end
Cleanup->>Registry: sweepStaleScheduledKeys(currentMinute)
Registry-->>Cleanup: evictedCount
alt evictedCount > 0
Cleanup->>Log: info("scheduled keys evicted")
end
Cleanup->>Cleanup: reset tick counter
end
sequenceDiagram
autonumber
participant Editor as Pipeline Editor
participant Hook as usePipelineState
participant Store as useCueDirtyStore
participant Service as cueService
participant IPC as IPC Handler
Editor->>Hook: user edits -> setIsDirty(true)
Hook->>Store: setPipelineDirty(true)
Editor->>Hook: save -> handleSave()
Hook->>Service: writeYaml(root, content, promptFiles)
Service->>IPC: window.maestro.cue.writeYaml(...)
IPC->>IPC: parse content -> collect prompt file paths
IPC->>IPC: pruneOrphanedPromptFiles(projectRoot, keepPaths)
IPC-->>Service: write success
Service->>Service: readYaml() verify written content
Service-->>Hook: verified
Hook->>Store: setPipelineDirty(false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 SummaryLarge-scope Cue subsystem overhaul combining persistence bug fixes (per-root YAML partitioning, deleted-pipeline reappearance, agent swap reversion, cross-session Dashboard bleed), save UX hardening (write-back verification, explicit toast feedback, empty-pipeline validation), and structural refactors (frontend IPC service layer,
Confidence Score: 3/5Not safe to merge — saving via the YAML editor silently deletes prompt files still referenced by the config. The P1 issue in the writeYaml IPC handler causes data loss on every YAML editor save for projects with pipeline-editor-generated prompt files. The rest of the PR is well-structured and the fixes are correct, but this gap needs to be addressed before shipping. src/main/ipc/handlers/cue.ts (writeYaml handler), src/renderer/services/cue.ts (validateYaml defaultValue) Important Files Changed
Sequence DiagramsequenceDiagram
participant YE as CueYamlEditor
participant PE as usePipelineState
participant SVC as cueService
participant IPC as cue:writeYaml handler
participant REPO as cue-config-repository
participant FS as Filesystem
Note over YE,FS: YAML Editor save path (bug)
YE->>SVC: writeYaml(root, yamlContent)
SVC->>IPC: writeYaml({root, content, promptFiles=undefined})
IPC->>IPC: referencedPaths = []
IPC->>FS: writeCueConfigFile(root, content)
IPC->>REPO: pruneOrphanedPromptFiles(root, [])
REPO->>FS: DELETE all .md files in .maestro/prompts/
Note over PE,FS: Pipeline Editor save path (correct)
PE->>PE: pipelinesToYaml() returns {yaml, promptFiles}
PE->>SVC: writeYaml(root, yaml, promptFilesObj)
SVC->>IPC: writeYaml({root, content, promptFiles: {...}})
IPC->>IPC: referencedPaths = Object.keys(promptFiles)
IPC->>FS: writeCuePromptFile() for each entry
IPC->>FS: writeCueConfigFile(root, content)
IPC->>REPO: pruneOrphanedPromptFiles(root, referencedPaths)
REPO->>FS: DELETE only unreferenced .md files
PE->>SVC: readYaml(root) for write-back verify
SVC-->>PE: onDisk content
PE->>PE: byte-compare, throw on mismatch
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
241
to
247
| writeCueConfigFile(options.projectRoot, options.content); | ||
|
|
||
| // Remove any `.md` files under .maestro/prompts/ that the new YAML no | ||
| // longer references (handles pipeline/agent renames and deletions). | ||
| pruneOrphanedPromptFiles(options.projectRoot, referencedPaths); | ||
| } | ||
| ) |
There was a problem hiding this comment.
When cueService.writeYaml is called from the YAML editor (CueYamlEditor.handleSave), promptFiles is undefined, so referencedPaths stays []. pruneOrphanedPromptFiles(projectRoot, []) then deletes every .md file under .maestro/prompts/ — including files that are still referenced by prompt_file: entries in the YAML the user just wrote.
A user who edits YAML manually and hits Save will silently lose all their pipeline prompt files. On next Cue engine load, subscriptions with prompt_file references will fail to materialise.
The IPC handler should derive the keep-set from the YAML content when promptFiles is absent:
if (!options.promptFiles) {
try {
const parsed = yaml.load(options.content) as Record<string, unknown> | null;
const subs = parsed?.subscriptions;
if (Array.isArray(subs)) {
for (const sub of subs as Record<string, unknown>[]) {
if (typeof sub?.prompt_file === 'string') referencedPaths.push(sub.prompt_file);
if (typeof sub?.output_prompt_file === 'string') referencedPaths.push(sub.output_prompt_file);
}
}
} catch {
// parse error — do NOT prune when YAML is unparseable
}
}
Comment on lines
+84
to
+90
| async validateYaml(content: string): Promise<{ valid: boolean; errors: string[] }> { | ||
| return createIpcMethod({ | ||
| call: () => window.maestro.cue.validateYaml(content), | ||
| errorContext: 'Cue validateYaml', | ||
| defaultValue: { valid: true, errors: [] }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
validateYaml silently reports "valid" on IPC failure
defaultValue: { valid: true, errors: [] } means any IPC error is reported as a clean validation pass. CueYamlEditor.handleSave gates on isValid, so a failed validation call leaves isValid = true and allows saving unvalidated content.
Suggested change
| async validateYaml(content: string): Promise<{ valid: boolean; errors: string[] }> { | |
| return createIpcMethod({ | |
| call: () => window.maestro.cue.validateYaml(content), | |
| errorContext: 'Cue validateYaml', | |
| defaultValue: { valid: true, errors: [] }, | |
| }); | |
| }, | |
| async validateYaml(content: string): Promise<{ valid: boolean; errors: string[] }> { | |
| return createIpcMethod({ | |
| call: () => window.maestro.cue.validateYaml(content), | |
| errorContext: 'Cue validateYaml', | |
| defaultValue: { valid: false, errors: ['Validation service unavailable'] }, | |
| }); | |
| }, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/main/ipc/handlers/cue.ts (1)
221-246:⚠️ Potential issue | 🔴 CriticalPruning from
promptFilescan delete still-referenced prompt files
pruneOrphanedPromptFilesis fed onlyoptions.promptFileskeys. WhenpromptFilesis missing or partial, valid.maestro/prompts/*.mdfiles can be removed even if YAML still references them.Safer approach (derive keep-set from YAML content)
async (options: { projectRoot: string; content: string; promptFiles?: Record<string, string>; }): Promise<void> => { - const referencedPaths: string[] = []; + const referencedPaths = new Set<string>(); + + const collectPromptPaths = (node: unknown): void => { + if (Array.isArray(node)) { + for (const item of node) collectPromptPaths(item); + return; + } + if (!node || typeof node !== 'object') return; + const obj = node as Record<string, unknown>; + for (const [key, value] of Object.entries(obj)) { + if ( + (key === 'prompt_file' || key === 'output_prompt_file') && + typeof value === 'string' + ) { + referencedPaths.add(value); + } + collectPromptPaths(value); + } + }; + if (options.promptFiles) { const promptsBase = path.resolve(options.projectRoot, '.maestro/prompts'); for (const [relativePath, content] of Object.entries(options.promptFiles)) { @@ writeCuePromptFile(options.projectRoot, relativePath, content); - referencedPaths.push(relativePath); + referencedPaths.add(relativePath); } } writeCueConfigFile(options.projectRoot, options.content); - // Remove any `.md` files under .maestro/prompts/ that the new YAML no - // longer references (handles pipeline/agent renames and deletions). - pruneOrphanedPromptFiles(options.projectRoot, referencedPaths); + const parsedYaml = yaml.load(options.content); + collectPromptPaths(parsedYaml); + pruneOrphanedPromptFiles(options.projectRoot, referencedPaths); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/cue.ts` around lines 221 - 246, The current prune call only uses referencedPaths built from options.promptFiles so files referenced solely inside the YAML (options.content) can be deleted; before calling pruneOrphanedPromptFiles, derive the keep-set from the YAML content written by writeCueConfigFile by parsing options.content to extract any referenced prompt file paths (e.g., pipeline/agent prompt keys that resolve to .maestro/prompts/*.md), normalize them to relative paths like referencedPaths uses, then take the union of that set and the existing referencedPaths (and any explicitly written via writeCuePromptFile) and pass that union into pruneOrphanedPromptFiles(options.projectRoot, keepPaths) so missing/partial options.promptFiles won't cause referenced files to be pruned.src/renderer/components/SessionList/SessionList.tsx (1)
147-163:⚠️ Potential issue | 🟠 MajorDon’t silently swallow
cueService.getStatus()failures.The
catch {}at Line 161 swallows both expected and unexpected failures. Please handle known recoverable states explicitly (e.g., engine-not-initialized), and report unexpected errors via Sentry so production failures are observable.
As per coding guidelines:src/**/*.{ts,tsx}: “Do not silently swallow errors... For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException,captureMessage).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SessionList/SessionList.tsx` around lines 147 - 163, The catch in fetchCueStatus around cueService.getStatus() currently swallows all errors; update the fetchCueStatus error handling to detect and ignore only the known recoverable condition (e.g., check error.code or error.message for the engine-not-initialized indicator) and leave the rest to be reported: call Sentry.captureException(error) (or captureMessage) for unexpected errors and re-throw them so they surface, while keeping the existing behavior of returning early when the engine is simply not ready; references: fetchCueStatus function, cueService.getStatus(), and setCueSessionMap.src/renderer/components/CueYamlEditor/CueYamlEditor.tsx (2)
69-77:⚠️ Potential issue | 🟠 MajorAvoid silent catch-all blocks in the new
cueServicepaths.These blocks currently suppress unexpected failures with no telemetry. Keep recoverable handling, but report unexpected exceptions (and/or rethrow) so errors are visible in production.
As per coding guidelines:src/**/*.{ts,tsx}: “Do not silently swallow errors... For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException,captureMessage).”Also applies to: 143-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx` around lines 69 - 77, The silent catch-all blocks around the cueService paths (notably where cueService.validateYaml is awaited and the outer try blocks in CueYamlEditor.tsx) should not swallow unexpected errors: change the catch blocks to capture/report the exception (e.g., call Sentry.captureException(err) or captureMessage with context) and then re-throw or return as appropriate; keep the existing recoverable handling for validation failures (still setIsValid / setValidationErrors when not cancelled) but ensure unexpected exceptions are forwarded to telemetry (or re-thrown) so they surface in production—apply the same change to the other catch at the 143-151 area.
136-149:⚠️ Potential issue | 🟠 MajorEmpty YAML from disk is incorrectly ignored during refresh.
At Line 139,
if (content)treats''as falsy, so an intentionally empty.maestro/cue.yamlwon’t refresh the editor and stale content can remain visible.🐛 Proposed fix
const refreshYamlFromDisk = useCallback(async () => { try { const content = await cueService.readYaml(projectRoot); - if (content) { - setYamlContent(content); - setOriginalContent(content); + if (content !== null) { + setYamlContent(content); + setOriginalContent(content); try { const result = await cueService.validateYaml(content); setIsValid(result.valid); setValidationErrors(result.errors); } catch { // non-fatal } } } catch { // non-fatal } }, [projectRoot]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx` around lines 136 - 149, The refreshYamlFromDisk callback incorrectly ignores an intentionally empty YAML because it checks `if (content)`; change that check to explicitly test for null/undefined (e.g., `if (content != null)` or `if (content !== undefined && content !== null)`) so that an empty string "" from `cueService.readYaml(projectRoot)` still triggers `setYamlContent`, `setOriginalContent`, and the subsequent validation call (`cueService.validateYaml`, `setIsValid`, `setValidationErrors`).src/renderer/hooks/remote/useRemoteIntegration.ts (1)
887-895:⚠️ Potential issue | 🟠 MajorCapture remote Cue trigger failures instead of only logging them.
This handler now routes control flow through
cueService.triggerSubscription(...), but unexpected failures still get reduced tofalseplusconsole.error. The response fallback is fine; the missing piece is explicit Sentry reporting.Refactor sketch
+import { captureException } from '../../../utils/sentry'; ... } catch (error) { - console.error('[Remote Cue Trigger] Failed:', subscriptionName, error); + captureException(error, { + operation: 'remote-trigger-cue-subscription', + subscriptionName, + }); window.maestro.process.sendRemoteTriggerCueSubscriptionResponse(responseChannel, false); }As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production... Use Sentry utilities (
captureException,captureMessage) fromsrc/utils/sentry.tsfor explicit error reporting with context."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/remote/useRemoteIntegration.ts` around lines 887 - 895, The catch block in the onRemoteTriggerCueSubscription handler currently only logs the error and returns false; update it to explicitly report the failure to Sentry by importing and calling captureException (and optionally captureMessage) from src/utils/sentry.ts inside the catch, passing the caught error plus contextual metadata (subscriptionName, responseChannel, prompt) so Sentry records the failure; keep the existing console.error and window.maestro.process.sendRemoteTriggerCueSubscriptionResponse(responseChannel, false) behavior after reporting.
🧹 Nitpick comments (3)
src/renderer/hooks/useCue.ts (1)
4-4: Finish thecueServicemigration insiderefresh().The write paths and activity subscription now use the wrapper, but
refresh()still talks towindow.maestro.cue.*directly. That leaves two IPC contracts in the same hook and bypasses the wrapper's read-side behavior.Refactor sketch
const [statusData, runsData, logData, queueData] = await Promise.all([ - window.maestro.cue.getStatus(), - window.maestro.cue.getActiveRuns(), - window.maestro.cue.getActivityLog(100), - window.maestro.cue.getQueueStatus(), + cueService.getStatus(), + cueService.getActiveRuns(), + cueService.getActivityLog(100), + cueService.getQueueStatus(), ]);Also applies to: 61-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/useCue.ts` at line 4, The refresh() function in useCue.ts still calls window.maestro.cue.* directly, causing mixed IPC usage; change refresh() to use the cueService read/get methods (e.g., cueService.getCue, cueService.getAllCues or whichever read APIs exist) instead of window.maestro.cue.* and remove direct window.maestro calls; also update any similar direct reads between lines ~61-99 to their cueService equivalents so all read and subscription logic consistently uses the wrapper (keep existing write/subscription changes intact and ensure return types/error handling match cueService methods).src/__tests__/main/cue/cue-session-lifecycle.test.ts (1)
633-660: Either add a real trigger here or rename the test.With
subscriptions: [],initSession()never creates timers/watchers/pollers.snapshot().size === 1only proves registry dedupe, so duplicate trigger registration leaks would still pass unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/cue/cue-session-lifecycle.test.ts` around lines 633 - 660, The test uses subscriptions: [] so initSession(session, ...) never registers any trigger sources — change the test to either provide a real subscription/trigger in the mock Cue config or rename the test to reflect it only validates registry deduplication; specifically update mockLoadCueConfig call to return a config with at least one trigger/subscription (so initSession on CueSessionRuntimeService will create timers/watchers/pollers and register sources in the registry created by createCueSessionRegistry), or if you prefer keep empty subscriptions then rename the test title and assertions to state it only verifies registry dedupe for initSession rather than "does not double-register trigger sources".src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx (1)
192-250: Add the post-mountuseNodesInitialized()flip.The regression path is “false on first render, true later”. All current cases set
mockNodesInitializedbefore mount, so an effect missinguseNodesInitialized()in its dependency list would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx` around lines 192 - 250, The tests currently set mockNodesInitialized true before mounting which hides a missing post-mount flip of the useNodesInitialized() dependency; update the relevant tests (the ones named "fits view once nodes have been measured", "restores saved viewport once nodes have been measured (does NOT fitView)", "consumes the pending saved viewport after applying it", and "runs the initial viewport step exactly once") to simulate the real regression path by mounting with mockNodesInitialized = false, calling renderEditor(), then setting mockNodesInitialized = true and forcing a post-mount update (e.g., rerender or act) so the effect depending on useNodesInitialized() runs; use the existing helpers mockPendingSavedViewportRef, mockFitView, mockSetViewport and renderEditor()/rerender to drive and then assert the same expectations.
🤖 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/main/cue/config/cue-config-repository.ts`:
- Around line 137-140: The try/catch around fs.readdirSync (the block that
currently returns on error) must not silently swallow errors—import and call
captureException (and optionally captureMessage) from the Sentry utilities
(src/utils/sentry.ts) with contextual info, then re-throw the error for
unexpected failures so callers can handle it; apply the same treatment to the
other catch block around the save/prune traversal (the catch covering lines
~152-157) so both places use captureException(...) and then throw the caught
error.
In `@src/main/cue/cue-db.ts`:
- Around line 198-222: The catch blocks in safeRecordCueEvent and
safeUpdateCueEventStatus swallow DB errors without Sentry reporting; update both
functions to import and call the Sentry utility (e.g., captureException or
reportError) from src/utils/sentry.ts inside the catch, passing the caught error
and a context object that includes operation and relevant data (for
safeRecordCueEvent include { operation: 'safeRecordCueEvent', event } and for
safeUpdateCueEventStatus include { operation: 'safeUpdateCueEventStatus', id,
status }), keep the existing log('warn', ...) behavior and still return without
throwing so callers remain non-fatal.
In `@src/main/cue/cue-fan-in-tracker.ts`:
- Around line 66-67: The issue is that fanInCreatedAt entries are left behind
when fan-in completes because only fanInTrackers gets deleted; update the
cleanup logic to also remove the timestamp entries. Specifically, wherever
fanInTrackers.delete(key) or the fan-in successful completion path removes a
tracker (e.g., the completion handler and the block that currently deletes
fanInTrackers), also call fanInCreatedAt.delete(key); likewise, adjust
clearForSession() to either remove all fanInCreatedAt entries for the session or
to iterate the same active keys it uses for fanInTrackers and delete
corresponding fanInCreatedAt entries so no orphaned timestamps remain.
In `@src/main/cue/cue-run-manager.ts`:
- Around line 238-246: The child output run (created via safeRecordCueEvent with
id outputRunId and status 'running') is never finalized if the output prompt
rejects; wrap the output prompt invocation/await in a try/catch (or add a catch
on its promise) and in the rejection path call safeRecordCueEvent for
outputRunId with a terminal status (e.g., 'failed' or 'error') and include error
details (e.g., JSON.stringify(error)) so the :output DB row is finalized; mirror
this change for the other similar output-finalization site referenced
(outputRunId usage around the later block).
In `@src/main/cue/cue-yaml-loader.ts`:
- Around line 85-93: The filter logic in parseCueConfigDocument incorrectly
applies indices from partitionValidSubscriptions (subscriptionErrors) which
refer to the raw YAML array, not the normalized document.subscriptions, causing
wrong removals; fix by changing the flow so validation runs against the
normalized subscriptions instead of raw YAML: after parseCueConfigDocument
builds the normalized array (document.subscriptions), call
partitionValidSubscriptions on that normalized array (or modify
partitionValidSubscriptions to accept the normalized array) and then derive
skippedIndices from subscriptionErrors to filter the same normalized array,
ensuring symbols involved are parseCueConfigDocument,
partitionValidSubscriptions, subscriptionErrors, and document.subscriptions.
In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx`:
- Around line 649-668: The context menu's Configure/Delete/Duplicate actions
still execute when the UI is in All Pipelines view because only
onNodeContextMenu was blocked; add the same read-only guard to the context menu
action handlers so they no-op if isAllPipelinesView (or selectedPipelineId ===
null). Locate the context-menu action handlers (the Delete and Duplicate
handlers and any Configure handler invoked by the context menu) and add an early
return when isAllPipelinesView is true (or selectedPipelineId === null) before
performing any edits or calling setContextMenu-related logic; also apply the
same guard to any helper functions those handlers call so an already-open menu
cannot perform edits.
In `@src/renderer/hooks/cue/usePipelineState.ts`:
- Around line 423-441: The code currently rebuilds previousRoots from
savedStateRef.current which fails when sessions moved/removed; instead persist
the last-written root set in a dedicated ref/field (e.g., lastWrittenRootsRef)
when performing successful writes and use that persisted set when computing
previousRoots (replace the current JSON.parse(savedStateRef.current) logic), and
ensure clears use the same read-back verification path as non-empty writes
(reuse the write-and-verify routine used elsewhere) so the empty-YAML clear
isn't a silent no-op; apply the same change to the analogous logic around the
block handling lines ~476-482.
In `@src/renderer/services/cue.ts`:
- Around line 20-90: The createIpcMethod catch block in
src/renderer/services/ipcWrapper.ts currently only console.errors and returns
options.defaultValue; update it to also call captureException(error, { extra: {
context: options.errorContext } }) before returning the default so IPC failures
are reported to Sentry. Locate the createIpcMethod implementation and inside its
try/catch where it handles errors for calls that have a defaultValue, import or
reference captureException and invoke it with the caught error and an extra
field containing options.errorContext, then continue to return
options.defaultValue as before.
---
Outside diff comments:
In `@src/main/ipc/handlers/cue.ts`:
- Around line 221-246: The current prune call only uses referencedPaths built
from options.promptFiles so files referenced solely inside the YAML
(options.content) can be deleted; before calling pruneOrphanedPromptFiles,
derive the keep-set from the YAML content written by writeCueConfigFile by
parsing options.content to extract any referenced prompt file paths (e.g.,
pipeline/agent prompt keys that resolve to .maestro/prompts/*.md), normalize
them to relative paths like referencedPaths uses, then take the union of that
set and the existing referencedPaths (and any explicitly written via
writeCuePromptFile) and pass that union into
pruneOrphanedPromptFiles(options.projectRoot, keepPaths) so missing/partial
options.promptFiles won't cause referenced files to be pruned.
In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx`:
- Around line 69-77: The silent catch-all blocks around the cueService paths
(notably where cueService.validateYaml is awaited and the outer try blocks in
CueYamlEditor.tsx) should not swallow unexpected errors: change the catch blocks
to capture/report the exception (e.g., call Sentry.captureException(err) or
captureMessage with context) and then re-throw or return as appropriate; keep
the existing recoverable handling for validation failures (still setIsValid /
setValidationErrors when not cancelled) but ensure unexpected exceptions are
forwarded to telemetry (or re-thrown) so they surface in production—apply the
same change to the other catch at the 143-151 area.
- Around line 136-149: The refreshYamlFromDisk callback incorrectly ignores an
intentionally empty YAML because it checks `if (content)`; change that check to
explicitly test for null/undefined (e.g., `if (content != null)` or `if (content
!== undefined && content !== null)`) so that an empty string "" from
`cueService.readYaml(projectRoot)` still triggers `setYamlContent`,
`setOriginalContent`, and the subsequent validation call
(`cueService.validateYaml`, `setIsValid`, `setValidationErrors`).
In `@src/renderer/components/SessionList/SessionList.tsx`:
- Around line 147-163: The catch in fetchCueStatus around cueService.getStatus()
currently swallows all errors; update the fetchCueStatus error handling to
detect and ignore only the known recoverable condition (e.g., check error.code
or error.message for the engine-not-initialized indicator) and leave the rest to
be reported: call Sentry.captureException(error) (or captureMessage) for
unexpected errors and re-throw them so they surface, while keeping the existing
behavior of returning early when the engine is simply not ready; references:
fetchCueStatus function, cueService.getStatus(), and setCueSessionMap.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 887-895: The catch block in the onRemoteTriggerCueSubscription
handler currently only logs the error and returns false; update it to explicitly
report the failure to Sentry by importing and calling captureException (and
optionally captureMessage) from src/utils/sentry.ts inside the catch, passing
the caught error plus contextual metadata (subscriptionName, responseChannel,
prompt) so Sentry records the failure; keep the existing console.error and
window.maestro.process.sendRemoteTriggerCueSubscriptionResponse(responseChannel,
false) behavior after reporting.
---
Nitpick comments:
In `@src/__tests__/main/cue/cue-session-lifecycle.test.ts`:
- Around line 633-660: The test uses subscriptions: [] so initSession(session,
...) never registers any trigger sources — change the test to either provide a
real subscription/trigger in the mock Cue config or rename the test to reflect
it only validates registry deduplication; specifically update mockLoadCueConfig
call to return a config with at least one trigger/subscription (so initSession
on CueSessionRuntimeService will create timers/watchers/pollers and register
sources in the registry created by createCueSessionRegistry), or if you prefer
keep empty subscriptions then rename the test title and assertions to state it
only verifies registry dedupe for initSession rather than "does not
double-register trigger sources".
In
`@src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx`:
- Around line 192-250: The tests currently set mockNodesInitialized true before
mounting which hides a missing post-mount flip of the useNodesInitialized()
dependency; update the relevant tests (the ones named "fits view once nodes have
been measured", "restores saved viewport once nodes have been measured (does NOT
fitView)", "consumes the pending saved viewport after applying it", and "runs
the initial viewport step exactly once") to simulate the real regression path by
mounting with mockNodesInitialized = false, calling renderEditor(), then setting
mockNodesInitialized = true and forcing a post-mount update (e.g., rerender or
act) so the effect depending on useNodesInitialized() runs; use the existing
helpers mockPendingSavedViewportRef, mockFitView, mockSetViewport and
renderEditor()/rerender to drive and then assert the same expectations.
In `@src/renderer/hooks/useCue.ts`:
- Line 4: The refresh() function in useCue.ts still calls window.maestro.cue.*
directly, causing mixed IPC usage; change refresh() to use the cueService
read/get methods (e.g., cueService.getCue, cueService.getAllCues or whichever
read APIs exist) instead of window.maestro.cue.* and remove direct
window.maestro calls; also update any similar direct reads between lines ~61-99
to their cueService equivalents so all read and subscription logic consistently
uses the wrapper (keep existing write/subscription changes intact and ensure
return types/error handling match cueService methods).
🪄 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: d5353fe2-1332-42d4-b41b-2f259b79908a
📒 Files selected for processing (55)
src/__tests__/main/cue/cue-cleanup-service.test.tssrc/__tests__/main/cue/cue-completion-chains.test.tssrc/__tests__/main/cue/cue-concurrency.test.tssrc/__tests__/main/cue/cue-db.test.tssrc/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/cue/cue-executor.test.tssrc/__tests__/main/cue/cue-fan-in-tracker.test.tssrc/__tests__/main/cue/cue-ipc-handlers.test.tssrc/__tests__/main/cue/cue-multi-hop-chains.test.tssrc/__tests__/main/cue/cue-process-lifecycle.test.tssrc/__tests__/main/cue/cue-run-manager.test.tssrc/__tests__/main/cue/cue-session-lifecycle.test.tssrc/__tests__/main/cue/cue-session-registry.test.tssrc/__tests__/main/cue/cue-sleep-prevention.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/CueModal.test.tsxsrc/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.allPipelinesViewLock.test.tsxsrc/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.drag.test.tsxsrc/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsxsrc/__tests__/renderer/components/CuePipelineEditor/utils/pipelineLayout.test.tssrc/__tests__/renderer/components/CuePipelineEditor/utils/yamlToPipeline.test.tssrc/__tests__/renderer/components/CueYamlEditor.test.tsxsrc/__tests__/renderer/hooks/cue/usePipelineLayout.test.tssrc/__tests__/renderer/hooks/cue/usePipelineState.test.tssrc/__tests__/renderer/services/cue.test.tssrc/__tests__/renderer/stores/cueDirtyStore.test.tssrc/main/cue/config/cue-config-repository.tssrc/main/cue/config/cue-config-validator.tssrc/main/cue/cue-cleanup-service.tssrc/main/cue/cue-db.tssrc/main/cue/cue-engine.tssrc/main/cue/cue-fan-in-tracker.tssrc/main/cue/cue-heartbeat.tssrc/main/cue/cue-process-lifecycle.tssrc/main/cue/cue-query-service.tssrc/main/cue/cue-run-manager.tssrc/main/cue/cue-session-registry.tssrc/main/cue/cue-session-runtime-service.tssrc/main/cue/cue-yaml-loader.tssrc/main/ipc/handlers/cue.tssrc/renderer/components/CueModal/CueModal.tsxsrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CuePipelineEditor/PipelineCanvas.tsxsrc/renderer/components/CuePipelineEditor/utils/pipelineLayout.tssrc/renderer/components/CuePipelineEditor/utils/yamlToPipeline.tssrc/renderer/components/CueYamlEditor/CueYamlEditor.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/hooks/cue/usePipelineLayout.tssrc/renderer/hooks/cue/usePipelineState.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/renderer/hooks/useCue.tssrc/renderer/services/cue.tssrc/renderer/stores/cueDirtyStore.ts
Comment on lines
+137
to
+140
| try { | ||
| entries = fs.readdirSync(dir, { withFileTypes: true }); | ||
| } catch { | ||
| return; |
There was a problem hiding this comment.
Do not silently swallow filesystem errors in prune traversal
Both catch blocks suppress all failures, which hides unexpected runtime errors during save and prevents Sentry visibility.
Proposed fix
+import { captureException } from '../../../utils/sentry';
+
const walk = (dir: string) => {
let entries: fs.Dirent[];
try {
entries = fs.readdirSync(dir, { withFileTypes: true });
- } catch {
- return;
+ } catch (error) {
+ const err = error as NodeJS.ErrnoException;
+ if (err.code === 'ENOENT') return;
+ captureException(error, {
+ context: 'Cue pruneOrphanedPromptFiles readdirSync failed',
+ extra: { projectRoot, dir },
+ });
+ throw error;
}
for (const entry of entries) {
const abs = path.join(dir, entry.name);
@@
try {
fs.unlinkSync(abs);
removed.push(abs);
- } catch {
- // Ignore — a file we can't delete is not worth failing the save.
+ } catch (error) {
+ const err = error as NodeJS.ErrnoException;
+ if (err.code === 'ENOENT') continue;
+ captureException(error, {
+ context: 'Cue pruneOrphanedPromptFiles unlinkSync failed',
+ extra: { projectRoot, file: abs },
+ });
+ throw error;
}
}
};As per coding guidelines: “Do not silently swallow errors… For unexpected errors, re-throw them… Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts.”
Also applies to: 152-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/config/cue-config-repository.ts` around lines 137 - 140, The
try/catch around fs.readdirSync (the block that currently returns on error) must
not silently swallow errors—import and call captureException (and optionally
captureMessage) from the Sentry utilities (src/utils/sentry.ts) with contextual
info, then re-throw the error for unexpected failures so callers can handle it;
apply the same treatment to the other catch block around the save/prune
traversal (the catch covering lines ~152-157) so both places use
captureException(...) and then throw the caught error.
Comment on lines
+66
to
+67
| /** Tracks when the first completion arrived for each tracker key (for cleanup staleness checks). */ | ||
| const fanInCreatedAt = new Map<string, number>(); |
There was a problem hiding this comment.
Clear fanInCreatedAt on successful fan-in completion too.
Line 218 only deletes fanInTrackers. Since the new timestamp map is cleaned up everywhere else, successful completions now leave orphaned fanInCreatedAt entries behind, and clearForSession() won't remove them later because it iterates active tracker keys only.
Possible fix
if (timer) {
clearTimeout(timer);
fanInTimers.delete(key);
}
fanInTrackers.delete(key);
+ fanInCreatedAt.delete(key);
const completions = [...tracker.values()];
const event = createCueEvent('agent.completed', sub.name, {Also applies to: 184-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/cue/cue-fan-in-tracker.ts` around lines 66 - 67, The issue is that
fanInCreatedAt entries are left behind when fan-in completes because only
fanInTrackers gets deleted; update the cleanup logic to also remove the
timestamp entries. Specifically, wherever fanInTrackers.delete(key) or the
fan-in successful completion path removes a tracker (e.g., the completion
handler and the block that currently deletes fanInTrackers), also call
fanInCreatedAt.delete(key); likewise, adjust clearForSession() to either remove
all fanInCreatedAt entries for the session or to iterate the same active keys it
uses for fanInTrackers and delete corresponding fanInCreatedAt entries so no
orphaned timestamps remain.
…bility Verified each review finding against the current code and applied fixes where the bug was real: Real bugs: - cue-fan-in-tracker: success completion deleted fanInTrackers but left fanInCreatedAt entries behind, leaking timestamp keys forever (every other path — timeout/break, clearForSession, expireTracker, reset — already cleaned up correctly). - cue-run-manager: when the output-prompt onCueRun rejected, the outputRunId DB row was created with status 'running' but never finalized, leaving phantom never-ending entries in the activity log. Wrap the inner await; mark the row 'failed' on rejection then re-throw. - cue-yaml-loader: partitionValidSubscriptions returned errors keyed by RAW yaml indices, but parseCueConfigDocument silently skips non-object entries, so the indices drifted and the lenient filter dropped the wrong subscriptions. Build a raw->normalized translation table; filter by translated indices. - CuePipelineEditor: only onNodeContextMenu blocked in All Pipelines view — the Configure/Delete/Duplicate handlers themselves had no guard, so an already-open menu could still mutate state if the user switched views. Re-check isAllPipelinesView in each handler. - usePipelineState: previousRoots was rebuilt from savedStateRef.current which fails when an agent has been renamed/removed since the last save (sessionId/Name no longer resolve). Persist a dedicated lastWrittenRootsRef on every successful save (and seed it from the initial load), use that for the clear set. Empty-YAML clears now use the same write-and-verify path as non-empty writes so they can't be a silent no-op. - ipc/handlers/cue:writeYaml: prune keep-set was built only from promptFiles, so a YAML referencing pre-existing prompt files that weren't re-passed would have those files deleted as orphans. Parse the YAML and union prompt_file/output_prompt_file paths into the keep-set. - CueYamlEditor.refreshYamlFromDisk: `if (content)` skipped intentionally empty YAML. Use `if (content != null)` so an empty string still triggers state updates and revalidation. Observability (Sentry) at silent catches: - cue-db: safeRecordCueEvent / safeUpdateCueEventStatus now also captureException alongside the existing warn (kept non-throwing). - cue-config-repository.pruneOrphanedPromptFiles: report readdir/unlink failures to Sentry. Deliberately NOT re-throwing — pruning runs after a successful YAML write, and re-throwing would surface a failed-save toast even though the YAML did persist. - ipcWrapper.createIpcMethod: capture on the swallow path (defaultValue), rethrow path leaves reporting to the caller. - CueYamlEditor: load/refresh catches now report unexpected exceptions. - SessionList.fetchCueStatus: report unexpected errors; ignore the known recoverable "Cue engine not initialized" message. - useRemoteIntegration: remote Cue trigger catch now captureExceptions with subscription/channel/prompt context. Tests: - cue-session-lifecycle: renamed the idempotency test + comment to reflect that empty subscriptions test only registry dedupe, not trigger-source dedupe (which would need a wired createTriggerSource). - CuePipelineEditor.initialViewport: refactored 4 tests to mount with mockNodesInitialized=false then flip to true and rerender, so they exercise the real false->true regression path through the effect's dependency change instead of asserting after a single mount with true. - usePipelineLayout: createDefaultParams now provides the new lastWrittenRootsRef. Pushed back on: - pruneOrphanedPromptFiles re-throwing: rejected because re-throwing would surface "save failed" for a save that actually persisted. - Migrating useCue.refresh() to cueService.* reads: cueService swallows IPC failures and returns defaults, which would silently kill useCue's user-visible error banner on the dashboard. Reverted reads to direct window.maestro.cue.* with a comment explaining why this hook is the exception. Writes (enable/disable/stop/trigger) stay on cueService. Tests: 25,260 passing.
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 (1)
src/renderer/services/ipcWrapper.ts (1)
90-104:⚠️ Potential issue | 🟠 MajorUnexpected transform errors are currently swallowed in default mode.
Because
options.transform(...)is inside the sametryasoptions.call(), a transform bug can be caught and converted todefaultValue(Line 91 to Line 104), masking real defects. Keep fallback behavior for IPC-call failures, but let transform exceptions bubble.💡 Suggested fix
export async function createIpcMethod<T>(options: IpcMethodOptions<T>): Promise<T> { + let result: T; try { - const result = await options.call(); - return options.transform ? options.transform(result) : result; + result = await options.call(); } catch (error) { console.error(`${options.errorContext} error:`, error); if (options.rethrow) { // Caller is responsible for handling/reporting. throw error; } // Swallow path: the caller never sees this error, so report it to // Sentry here — otherwise IPC failures behind read methods (return // default on error) would be invisible in production. void captureException(error, { extra: { context: options.errorContext } }); return options.defaultValue as T; } + // Transform failures are unexpected; do not swallow them. + return options.transform ? options.transform(result) : result; }As per coding guidelines: “Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR). For unexpected errors, re-throw them to allow Sentry to capture them.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/services/ipcWrapper.ts` around lines 90 - 104, In createIpcMethod, the current try/catch wraps both options.call() and options.transform(), so transform errors get swallowed and converted to defaultValue; change the flow to only catch call() failures: await options.call() inside a try/catch that logs, conditionally rethrows (respecting options.rethrow), sends call errors to captureException and returns options.defaultValue on IPC-call failure, then apply options.transform(result) outside the catch so any transform exceptions bubble up (do not catch/convert them); reference createIpcMethod, options.call, options.transform, options.rethrow, captureException, and options.defaultValue when making the change.
♻️ Duplicate comments (1)
src/renderer/hooks/cue/usePipelineLayout.ts (1)
154-171:⚠️ Potential issue | 🟠 Major
lastWrittenRootsRefcan still forget roots after a session rename/removal.This set is rebuilt only from the current
sessionslookup. If a previously-written pipeline's agent no longer resolves by id/name before the editor opens, its original root never lands inloadedRoots, so the next save cannot clear that stale.maestro/cue.yaml. Seed this from data that already carries the root, or persist the written-root set alongside the saved layout and reuse it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/cue/usePipelineLayout.ts` around lines 154 - 171, lastWrittenRootsRef is only seeded from resolving agent nodes via sessions and thus can miss roots when a session was renamed/removed; update the seeding logic in usePipelineLayout to also incorporate any persisted written-root info from the saved layout or from node payloads that already include a root field so stale roots aren't lost. Specifically, when building loadedRoots (the Set used to initialize lastWrittenRootsRef.current), merge in roots from the layout's saved written-roots metadata (if present) or from pipeline.node.data (e.g., a root property on AgentNodeData) before or in addition to looking up sessionsById/sessionsByName, and ensure lastWrittenRootsRef.current is set from this combined set so renames/removals do not drop previously written roots.
🧹 Nitpick comments (1)
src/main/ipc/handlers/cue.ts (1)
221-275: Move the save/prune orchestration back out of the IPC handler.This block now does path validation, prompt-file writes, YAML parsing, keep-set derivation, and orphan cleanup. That breaks the module’s own “thin transport layer” contract and makes this save path harder to reuse and test outside Electron. A single domain-level
writeCueConfig...entry point would keep the handler back to delegation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/cue.ts` around lines 221 - 275, The IPC handler currently performs validation, writes prompt files, writes the YAML, parses it and prunes orphans — move that orchestration into a single domain API (e.g. export a function like writeCueConfigAndPrune(projectRoot, content, promptFiles)) so the handler just delegates. Implement writeCueConfigAndPrune to perform the path validation (same checks using path.isAbsolute and promptsBase), call writeCuePromptFile for each prompt, call writeCueConfigFile, parse via yaml.load to collect prompt_file/output_prompt_file into a keepPaths Set, and finally call pruneOrphanedPromptFiles; then replace the large block in the IPC handler with a single call to the new function and proper error handling. Ensure the new function uses the existing helpers writeCuePromptFile, writeCueConfigFile, pruneOrphanedPromptFiles and preserves the current try/catch fallback behavior for yaml.load.
🤖 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/main/cue/cue-db.ts`:
- Around line 207-211: The current safeRecordCueEvent sends the entire event
(including payload with possible sensitive sourceOutput) to captureException;
change this to send only minimal metadata by creating a sanitizedEvent that
strips or replaces event.payload (e.g., set payload to undefined or to a small
object containing non-sensitive fields such as payloadSize and truncated: true)
and include only identifiers like event.id, event.type, event.status, and runId
before calling captureException(err, { operation: 'safeRecordCueEvent', event:
sanitizedEvent }); ensure safeRecordCueEvent still swallows errors and does not
rethrow.
In `@src/main/ipc/handlers/cue.ts`:
- Around line 248-275: The current try/catch around yaml.load(...) can swallow a
parse error and still call pruneOrphanedPromptFiles(...), risking mass deletion;
fix by either (A) computing the keepPaths set from options.content (parsing
subscriptions) before calling writeCueConfigFile(), or (B) if yaml.load throws,
do not call pruneOrphanedPromptFiles(...) — return/skip pruning — and report the
error via the Sentry helper (captureException) with context (e.g., include
options.projectRoot and a note that parse failed for writeCueConfigFile flow);
update references to yaml.load, keepPaths, pruneOrphanedPromptFiles, and
writeCueConfigFile accordingly so pruning only runs when parsing succeeded and
errors are explicitly captured.
In `@src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx`:
- Around line 311-322: The current effect delays applying
pendingSavedViewportRef until nodesInitialized and computedNodes exist; change
it so the saved-viewport branch runs immediately: inside the useEffect for
hasInitialFitRef, first check if hasInitialFitRef.current is false and if
pendingSavedViewportRef.current exists — if so, clear
pendingSavedViewportRef.current, call reactFlowInstance.setViewport(saved) and
set hasInitialFitRef.current = true; otherwise (no saved viewport) only then
check the existing nodesInitialized && computedNodes.length > 0 condition and
call reactFlowInstance.fitView({ padding: 0.15, duration: 200 }) and set
hasInitialFitRef.current = true; keep references to pendingSavedViewportRef,
nodesInitialized, computedNodes.length and reactFlowInstance so the logic
targets the same symbols.
In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx`:
- Around line 70-73: Wrap each call to cueService.validateYaml(...) (the calls
that currently setIsValid(...) and setValidationErrors(...), including the
occurrences near where setIsValid and setValidationErrors are used) in a
try/catch; in the try keep the existing behavior, and in catch (if not
cancelled) setIsValid(false) and setValidationErrors to a meaningful error entry
(e.g., error.message or String(error)) to ensure Save is gated, then re-throw
the error so it is not silently swallowed (this applies to the validateYaml call
sites around the current setIsValid/setValidationErrors usages and the other two
occurrences mentioned).
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 895-901: The captureException call inside
remoteTriggerCueSubscription currently sends the full prompt in telemetry;
update the code to avoid sending raw prompt text by redacting or replacing
prompt with a safe diagnostic (e.g., a fixed redacted string, prompt length, or
a hash/checksum) before passing it into captureException's extra metadata; keep
subscriptionName and responseChannel as-is but ensure the prompt field is
replaced (or omitted) to prevent PII/secrets leakage.
---
Outside diff comments:
In `@src/renderer/services/ipcWrapper.ts`:
- Around line 90-104: In createIpcMethod, the current try/catch wraps both
options.call() and options.transform(), so transform errors get swallowed and
converted to defaultValue; change the flow to only catch call() failures: await
options.call() inside a try/catch that logs, conditionally rethrows (respecting
options.rethrow), sends call errors to captureException and returns
options.defaultValue on IPC-call failure, then apply options.transform(result)
outside the catch so any transform exceptions bubble up (do not catch/convert
them); reference createIpcMethod, options.call, options.transform,
options.rethrow, captureException, and options.defaultValue when making the
change.
---
Duplicate comments:
In `@src/renderer/hooks/cue/usePipelineLayout.ts`:
- Around line 154-171: lastWrittenRootsRef is only seeded from resolving agent
nodes via sessions and thus can miss roots when a session was renamed/removed;
update the seeding logic in usePipelineLayout to also incorporate any persisted
written-root info from the saved layout or from node payloads that already
include a root field so stale roots aren't lost. Specifically, when building
loadedRoots (the Set used to initialize lastWrittenRootsRef.current), merge in
roots from the layout's saved written-roots metadata (if present) or from
pipeline.node.data (e.g., a root property on AgentNodeData) before or in
addition to looking up sessionsById/sessionsByName, and ensure
lastWrittenRootsRef.current is set from this combined set so renames/removals do
not drop previously written roots.
---
Nitpick comments:
In `@src/main/ipc/handlers/cue.ts`:
- Around line 221-275: The IPC handler currently performs validation, writes
prompt files, writes the YAML, parses it and prunes orphans — move that
orchestration into a single domain API (e.g. export a function like
writeCueConfigAndPrune(projectRoot, content, promptFiles)) so the handler just
delegates. Implement writeCueConfigAndPrune to perform the path validation (same
checks using path.isAbsolute and promptsBase), call writeCuePromptFile for each
prompt, call writeCueConfigFile, parse via yaml.load to collect
prompt_file/output_prompt_file into a keepPaths Set, and finally call
pruneOrphanedPromptFiles; then replace the large block in the IPC handler with a
single call to the new function and proper error handling. Ensure the new
function uses the existing helpers writeCuePromptFile, writeCueConfigFile,
pruneOrphanedPromptFiles and preserves the current try/catch fallback behavior
for yaml.load.
🪄 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: f3d29dc0-bde3-430d-898f-1871027a13e7
📒 Files selected for processing (17)
src/__tests__/main/cue/cue-session-lifecycle.test.tssrc/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsxsrc/__tests__/renderer/hooks/cue/usePipelineLayout.test.tssrc/main/cue/config/cue-config-repository.tssrc/main/cue/cue-db.tssrc/main/cue/cue-fan-in-tracker.tssrc/main/cue/cue-run-manager.tssrc/main/cue/cue-yaml-loader.tssrc/main/ipc/handlers/cue.tssrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CueYamlEditor/CueYamlEditor.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/hooks/cue/usePipelineLayout.tssrc/renderer/hooks/cue/usePipelineState.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/renderer/hooks/useCue.tssrc/renderer/services/ipcWrapper.ts
✅ Files skipped from review due to trivial changes (3)
- src/main/cue/cue-fan-in-tracker.ts
- src/main/cue/cue-yaml-loader.ts
- src/tests/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/cue/config/cue-config-repository.ts
- src/renderer/hooks/useCue.ts
- src/tests/main/cue/cue-session-lifecycle.test.ts
- src/tests/renderer/hooks/cue/usePipelineLayout.test.ts
- src/main/cue/cue-run-manager.ts
- src/renderer/components/SessionList/SessionList.tsx
…te save, immediate viewport restore Verified each finding and applied fixes where the bug was real: PII / telemetry safety: - cue-db.safeRecordCueEvent: stripped event.payload from Sentry payload — agent.completed events carry sourceOutput (upstream agent stdout), which can include user content / secrets. Send identifiers + payloadSize only. - useRemoteIntegration.remoteTriggerCueSubscription: never send the raw prompt string to telemetry; replaced with promptLength + promptProvided so we can correlate failures against payload size without leaking body. Real bugs: - ipc/handlers/cue:writeYaml: previous catch around yaml.load fell through to pruneOrphanedPromptFiles with a partial keep-set, risking mass-delete of prompt files referenced only inside the unparseable YAML. Track parseSucceeded; on failure, captureException with context and SKIP the prune entirely (next successful save catches up). Parse now happens before the YAML write so the keep-set is authoritative when used. - CueYamlEditor: the validate-on-load and validate-on-refresh catches reported to Sentry but never set isValid=false, so the initial-state isValid=true left Save enabled even though validation never ran. Now setIsValid(false) and setValidationErrors(message) in both catches so Save is gated and the user sees what failed. - ipcWrapper.createIpcMethod: the single try/catch wrapped both options.call() and options.transform(), so transform-function exceptions were silently converted to defaultValue, masking real bugs. Restructured so only call() is in the catch; transform runs after and propagates. - CuePipelineEditor initial viewport: setViewport (pure x/y/zoom restore) doesn't depend on node measurement, but the effect was waiting for useNodesInitialized + computedNodes.length>0 before applying. Split the effect: saved viewport applies immediately on mount; fitView fallback still gates on measurement (that path needs node bounds). - usePipelineLayout / PipelineLayoutState: lastWrittenRootsRef could miss roots when an agent was renamed/removed before editor open (sessionId /Name lookup misses the project root, leaving stale YAML uncleared on the next save). Added writtenRoots?: string[] to PipelineLayoutState; persistLayout writes it from the ref; the load-time seed unions the persisted set with the session-resolved set so renames/removals don't drop previously-written roots. Tests: - initialViewport.test.tsx: saved-viewport tests no longer flip mockNodesInitialized. They now assert setViewport fires immediately on mount, matching the new contract. Pushed back on: - Reviewer asked to re-throw inside CueYamlEditor's validateYaml catches. Skipped — re-throwing would propagate to the outer loadYaml catch, conflating validateYaml failures with readYaml failures and triggering the template fallback. Gating Save (the actual safety concern) is done. - Reviewer suggested extracting writeCueConfigAndPrune into a domain API. Skipped (marked nitpick): the IPC handler is the natural home for path validation + multi-write coordination; extracting would create a one-call wrapper without separating concerns. Tests: 25,260 passing.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/handlers/cue.ts (1)
224-237:⚠️ Potential issue | 🟠 MajorReject
.maestro/promptsitself and non-.mdprompt targets.Line 232 only checks lexical containment under
.maestro/prompts, so keys like.maestro/promptsor.maestro/prompts/foo.txtstill pass. The first can create a regular file where the prompts directory should live; the second creates files thatpruneOrphanedPromptFiles()will never clean up because it only manages.mdfiles.🛠️ Proposed fix
- if (!target.startsWith(promptsBase + path.sep) && target !== promptsBase) { + if (!target.startsWith(promptsBase + path.sep)) { throw new Error( `cue:writeYaml: promptFiles key "${relativePath}" resolves outside the .maestro/prompts directory` ); } + if (path.extname(target).toLowerCase() !== '.md') { + throw new Error( + `cue:writeYaml: promptFiles key "${relativePath}" must point to a .md file under .maestro/prompts` + ); + }I'd mirror the same guard in
writeCuePromptFile()as well, since that's the real trust boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/cue.ts` around lines 224 - 237, Reject promptFiles entries that resolve exactly to the prompts directory and any targets that do not end with ".md": in the validation loop around writeCuePromptFile (the block using promptsBase, relativePath, target) add checks to throw if target === promptsBase (rejecting ".maestro/prompts" itself) and if path.extname(target) !== '.md' (reject non-.md targets); then duplicate the same validation inside writeCuePromptFile(...) so the function enforces the same trust boundary as pruneOrphanedPromptFiles and prevents creating non-markdown or directory-conflicting files.
♻️ Duplicate comments (1)
src/renderer/components/CueYamlEditor/CueYamlEditor.tsx (1)
70-90:⚠️ Potential issue | 🟠 Major
validateYaml()transport failures still bypass save gating.
cueService.validateYaml()falls back to{ valid: true, errors: [] }on IPC errors, so these catches only handle local exceptions. A failed validation round can still leaveisValid === trueand re-enable Save even though validation never ran. Use a strict/throwing validation path here, or force this workflow to treat the fallback result as invalid.As per coding guidelines
src/**/*.{ts,tsx}: “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly.”Also applies to: 115-120, 163-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx` around lines 70 - 90, validateYaml() may return a fallback success on IPC failures, which lets isValid stay true and re-enable Save; change the handler around cueService.validateYaml(initial) so transport failures are treated as validation failures: either call a strict/throwing variant (e.g., cueService.validateYamlStrict or cueService.validateYaml(initial, { strict: true })) so IPC errors propagate, or detect the fallback result shape and, if present, setIsValid(false) and setValidationErrors([...]) instead of trusting validationResult.valid; also stop swallowing unexpected exceptions — after captureException(err, ...) re-throw the error so unhandled exceptions bubble to Sentry (while still guarding cancelled before calling setIsValid/setValidationErrors).
🤖 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/CueYamlEditor/CueYamlEditor.tsx`:
- Around line 64-67: The code currently treats cueService.readYaml() returning
null as “no cue.yaml” which hides IPC/read failures; change the read path so
cueService.readYaml() distinguishes a missing file from an IPC error (or, if
changing the service isn't feasible, detect content === null in the caller and
throw or surface an error instead of defaulting to CUE_YAML_TEMPLATE).
Specifically, update the logic around cueService.readYaml(...) /
setYamlContent(...) / originalContent so only an explicit “file not found”
sentinel leads to using CUE_YAML_TEMPLATE, whereas a null return causes an
exception (or propagates the IPC error) so it can be handled by your existing
catch and reported; apply the same fix to the other usages noted (the blocks
around the second/third/fourth read locations).
In `@src/renderer/hooks/cue/usePipelineLayout.ts`:
- Around line 159-188: The persisted writtenRoots are only restored after an
early return when graphSessions is empty; move the saved writtenRoots restore so
it runs even when the live graph is empty. Concretely, in the effect inside
usePipelineLayout (the block that builds loadedRoots from
savedLayout.writtenRoots, sessionsById/sessionsByName, and pipelinesForRoots and
sets lastWrittenRootsRef.current), relocate or duplicate that logic to run
before the early return that checks graphSessions.length === 0 (or otherwise
ensure savedLayout.writtenRoots is applied regardless of graphSessions). Keep
using the same symbols (savedLayout, writtenRoots, loadedRoots,
sessionsById/sessionsByName, pipelinesForRoots, lastWrittenRootsRef) so
orphan-root metadata is rehydrated on startup even with no live pipelines.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Line 4: The import of captureException in useRemoteIntegration.ts is using a
wrong relative path; update the import to use the canonical shared Sentry
utility (import captureException from the root Sentry util module) by replacing
the '../../utils/sentry' import with the root module path (src/utils/sentry) so
that the file uses the centralized captureException implementation.
---
Outside diff comments:
In `@src/main/ipc/handlers/cue.ts`:
- Around line 224-237: Reject promptFiles entries that resolve exactly to the
prompts directory and any targets that do not end with ".md": in the validation
loop around writeCuePromptFile (the block using promptsBase, relativePath,
target) add checks to throw if target === promptsBase (rejecting
".maestro/prompts" itself) and if path.extname(target) !== '.md' (reject non-.md
targets); then duplicate the same validation inside writeCuePromptFile(...) so
the function enforces the same trust boundary as pruneOrphanedPromptFiles and
prevents creating non-markdown or directory-conflicting files.
---
Duplicate comments:
In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx`:
- Around line 70-90: validateYaml() may return a fallback success on IPC
failures, which lets isValid stay true and re-enable Save; change the handler
around cueService.validateYaml(initial) so transport failures are treated as
validation failures: either call a strict/throwing variant (e.g.,
cueService.validateYamlStrict or cueService.validateYaml(initial, { strict: true
})) so IPC errors propagate, or detect the fallback result shape and, if
present, setIsValid(false) and setValidationErrors([...]) instead of trusting
validationResult.valid; also stop swallowing unexpected exceptions — after
captureException(err, ...) re-throw the error so unhandled exceptions bubble to
Sentry (while still guarding cancelled before calling
setIsValid/setValidationErrors).
🪄 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: c2d20918-2d06-4ea0-ae5b-703f58d9a331
📒 Files selected for processing (9)
src/__tests__/renderer/components/CuePipelineEditor/CuePipelineEditor.initialViewport.test.tsxsrc/main/cue/cue-db.tssrc/main/ipc/handlers/cue.tssrc/renderer/components/CuePipelineEditor/CuePipelineEditor.tsxsrc/renderer/components/CueYamlEditor/CueYamlEditor.tsxsrc/renderer/hooks/cue/usePipelineLayout.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/renderer/services/ipcWrapper.tssrc/shared/cue-pipeline-types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/cue/cue-db.ts
- src/renderer/components/CuePipelineEditor/CuePipelineEditor.tsx
- src/renderer/services/ipcWrapper.ts
Comment on lines
+64
to
67
| const content = await cueService.readYaml(projectRoot); | ||
| if (cancelled) return; | ||
| const initial = content ?? CUE_YAML_TEMPLATE; | ||
| setYamlContent(initial); |
There was a problem hiding this comment.
Do not treat read failures as “no cue.yaml”.
cueService.readYaml() resolves to null on IPC errors, so these catch blocks never see the common failure path. On initial load, that null is converted into CUE_YAML_TEMPLATE and stored as originalContent, which can let a transient read failure overwrite a real cue.yaml on the next save; on refresh, the same failure becomes a silent no-op. This needs a strict read path, or a return shape that distinguishes ENOENT from “failed to read”.
As per coding guidelines src/**/*.{ts,tsx}: “Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly.”
Also applies to: 92-95, 154-159, 179-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/components/CueYamlEditor/CueYamlEditor.tsx` around lines 64 -
67, The code currently treats cueService.readYaml() returning null as “no
cue.yaml” which hides IPC/read failures; change the read path so
cueService.readYaml() distinguishes a missing file from an IPC error (or, if
changing the service isn't feasible, detect content === null in the caller and
throw or surface an error instead of defaulting to CUE_YAML_TEMPLATE).
Specifically, update the logic around cueService.readYaml(...) /
setYamlContent(...) / originalContent so only an explicit “file not found”
sentinel leads to using CUE_YAML_TEMPLATE, whereas a null return causes an
exception (or propagates the IPC error) so it can be handled by your existing
catch and reported; apply the same fix to the other usages noted (the blocks
around the second/third/fourth read locations).
| import { useEffect, useRef } from 'react'; | ||
| import type { Session, SessionState, ThinkingMode } from '../../types'; | ||
| import { cueService } from '../../services/cue'; | ||
| import { captureException } from '../../utils/sentry'; |
There was a problem hiding this comment.
Use the canonical Sentry utility import path.
Line 4 imports captureException from ../../utils/sentry, but this path does not point to the required shared utility module. Please import from the root Sentry utility path instead.
Suggested fix
-import { captureException } from '../../utils/sentry';
+import { captureException } from '../../../utils/sentry';As per coding guidelines, "Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
📝 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.
Suggested change
| import { captureException } from '../../utils/sentry'; | |
| import { captureException } from '../../../utils/sentry'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/remote/useRemoteIntegration.ts` at line 4, The import of
captureException in useRemoteIntegration.ts is using a wrong relative path;
update the import to use the canonical shared Sentry utility (import
captureException from the root Sentry util module) by replacing the
'../../utils/sentry' import with the root module path (src/utils/sentry) so that
the file uses the centralized captureException implementation.
…st boundary, orphan-root reseed
Verified each finding and applied fixes where the bug was real:
IPC contract — distinguish "no data" from "transport failed":
- cueService.readYaml: previously `defaultValue: null` collapsed "file
doesn't exist" and "IPC error" into the same null return, hiding
transport failures. Switched to `rethrow: true`. Handler still returns
null for missing file; IPC errors propagate so callers (CueYamlEditor's
outer catches, handleSave's write-back verification) react correctly.
handleSave's "did not persist" message is strictly more informative —
the actual IPC error message now propagates instead.
- cueService.validateYaml: previously `defaultValue: { valid: true,
errors: [] }` on IPC failure — actively dangerous because a transport
error would surface as "yaml is valid, save freely". Switched to
`rethrow: true`. CueYamlEditor's three call sites already catch the
rejection and gate Save by setting isValid=false.
writtenRoots reseed independent of graphSessions:
- usePipelineLayout: the writtenRoots seeding lived inside the main
load-layout effect, which early-returns when graphSessions is empty —
so opening the editor with no live sessions (engine disabled, no
registered sessions) never restored orphan-root metadata, and a "delete
the orphaned pipeline" save would leave the stale YAML at that root
uncleared. Added a standalone effect that runs once on mount and
unions persisted writtenRoots into lastWrittenRootsRef.current. The
main effect still re-builds the full set when graphSessions arrive.
Prompt-file trust boundary:
- ipc/handlers/cue:writeYaml: tightened promptFiles entry validation to
reject targets that resolve to the prompts directory itself
(`target === promptsBase`) and to non-.md files (the latter would never
get pruned, accumulating orphans permanently because
pruneOrphanedPromptFiles only removes .md files).
- writeCuePromptFile: duplicated the same checks inside the function so
any internal caller hits the same trust boundary as IPC callers (defense
in depth — the IPC handler's checks were the only enforcement before).
Tests updated for new contracts:
- services/cue.test.ts: readYaml asserts pass-through-null on missing
file + throws on IPC error. validateYaml asserts throws on IPC error.
- usePipelineLayout.test.ts: "does not restore layout when graphSessions
is empty" updated — pipeline state is still untouched, but
loadPipelineLayout may now fire once for the standalone writtenRoots
reseed effect.
Pushed back on:
- Reviewer claimed `'../../utils/sentry'` in useRemoteIntegration is the
wrong path. Verified: it correctly resolves to
src/renderer/utils/sentry.ts, and every other renderer file uses the
same pattern. There is no src/utils/sentry.ts. No change.
Tests: 25,261 passing.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/cue/usePipelineLayout.ts (1)
147-150:⚠️ Potential issue | 🟠 MajorInvalidate stale restores before the empty-graph return.
Line 148 can return before
latestRestoreIdRefis bumped. IfgraphSessionsflips to[]while the previouscueService.loadPipelineLayout()call is still pending, the stale request still passes thereqIdguard and re-applies the old pipelines into state.Suggested fix
useEffect(() => { - if (hasRestoredLayoutRef.current) return; - if (!graphSessions || graphSessions.length === 0) return; - - const reqId = ++latestRestoreIdRef.current; + const reqId = ++latestRestoreIdRef.current; + if (hasRestoredLayoutRef.current) return; + if (!graphSessions || graphSessions.length === 0) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/cue/usePipelineLayout.ts` around lines 147 - 150, Move the bump of latestRestoreIdRef.current to occur before the early-return check for an empty graph so stale async restores cannot pass the reqId guard; specifically, increment latestRestoreIdRef.current (the reqId used to validate cueService.loadPipelineLayout responses) before checking graphSessions.length === 0 and before returning, and ensure hasRestoredLayoutRef.current behavior remains unchanged so any in-flight loadPipelineLayout responses with older reqId are ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/hooks/cue/usePipelineLayout.ts`:
- Around line 147-150: Move the bump of latestRestoreIdRef.current to occur
before the early-return check for an empty graph so stale async restores cannot
pass the reqId guard; specifically, increment latestRestoreIdRef.current (the
reqId used to validate cueService.loadPipelineLayout responses) before checking
graphSessions.length === 0 and before returning, and ensure
hasRestoredLayoutRef.current behavior remains unchanged so any in-flight
loadPipelineLayout responses with older reqId are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b7027e8-5fa1-468b-b289-03070ea0deb6
📒 Files selected for processing (6)
src/__tests__/renderer/hooks/cue/usePipelineLayout.test.tssrc/__tests__/renderer/services/cue.test.tssrc/main/cue/config/cue-config-repository.tssrc/main/ipc/handlers/cue.tssrc/renderer/hooks/cue/usePipelineLayout.tssrc/renderer/services/cue.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/ipc/handlers/cue.ts
- src/tests/renderer/hooks/cue/usePipelineLayout.test.ts
- src/main/cue/config/cue-config-repository.ts
- src/tests/renderer/services/cue.test.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
End-to-end overhaul of the Maestro Cue subsystem combining bug fixes for a cluster of user-reported persistence/UX issues with refactors that introduce a dedicated frontend Cue IPC service, a Zustand dirty-state store, a backend cleanup service, and a partitioned YAML validator.
Refactors
Frontend Cue IPC service layer (
src/renderer/services/cue.ts)window.maestro.cue.*calls. Reads return safe defaults; writes rethrow (matches thegit.tscreateIpcMethodpattern already used elsewhere).window.maestro.cue.*and ontocueService.*.Cue dirty-state store (
src/renderer/stores/cueDirtyStore.ts)onDirtyChangeprop chain and thepipelineDirtyRefhack.CueModalreads dirty state viagetState()so the value is always fresh; a cleanup effect resets flags on unmount.YAML validator decomposition
validateSubscription(sub, prefix)andvalidateSettings(settings)from the monolithicvalidateCueConfigDocument.partitionValidSubscriptions(config)returns{ configErrors, validIndices, subscriptionErrors }— separates fatal config-level errors from per-subscription errors so the loader can drop bad subs without failing the whole file.Backend cleanup service (
src/main/cue/cue-cleanup-service.ts)time.scheduleddedup keys.CueHeartbeat.onTick; replaces ad-hoc cleanup scattered across the run manager.Run-manager error-handling cleanup
safeRecordCueEvent/safeUpdateCueEventStatuswrappers incue-db.ts.try/catchblocks incue-run-manager.tsto use the safe wrappers (errors now logged + reported to Sentry instead of swallowed).Session-registry / fan-in tracker API extensions
CueFanInTracker: addedgetActiveTrackerKeys,getTrackerCreatedAt,expireTracker, plus afanInCreatedAtmap.CueSessionRegistry: addedsweepStaleScheduledKeys.initSession()now idempotent — tears down existing registration before re-registering, preventing duplicate trigger sources from racing init paths.Editor viewport-restore refactor
pendingSavedViewportRefthreaded fromusePipelineLayouttoCuePipelineEditor. Viewport restore now waits for ReactFlow to measure nodes (useNodesInitialized) instead of racing on asetTimeoutagainstfitView— fixes "empty canvas on first open" symptoms.PipelineCanvas+ All-Pipelines-View locking polish.Fixes
Persistence
handleSavewas writing the same aggregated YAML and prompt files to every involvedprojectRoot. Now partitions pipelines by root and writes only that root's pipelines + prompts to its own.maestro/cue.yaml. Cross-root pipelines are rejected with a clear validation error.findTargetSessionno longer overrides an explicitagent_idon a coincidental name match. Combined with per-root partitioning,agent_idis the source of truth.cue-query-service.getGraphData()filters subscriptions per-session byagent_id, so each session only surfaces its own subscriptions even when sharing a project root.pruneOrphanedPromptFilesruns after everycue:writeYamlto delete.maestro/prompts/*.mdfiles the new YAML no longer references.Save UX (the "didn't save" class of bugs)
validatePipelinesnow flags completely-empty pipelines (add a trigger and an agent before saving) instead of skipping them;handleSaverefuses to no-op when the editor has pipelines but nothing partitions to a root.cue:writeYamlis followed by a write-back read and byte-for-byte compare; mismatches throw,isDirtyis preserved on failure, and explicit success/error toasts (Saved N pipelines to M projects/Your changes were NOT saved. <reason>) replace the missable 2-second in-button flash.createPipelineassigns timestamp ids; the next reload regenerates name-based ids, leaving the savedselectedPipelineIdstale andconvertToReactFlowNodesskipping every pipeline.mergePipelinesWithSavedLayoutnow validates the saved selection against live pipeline ids and falls back to the first pipeline; a live safety-net effect inusePipelineStateresetsselectedPipelineIdtonullwhenever it points at a pipeline that no longer exists.pipelineToYamlwas happy to write atime.scheduledsubscription with noschedule_times(ortime.heartbeatwith nointerval_minutes, etc.); the loader then rejected the entire YAML for every agent in that project root on Cue toggle.validatePipelinesnow mirrors the YAML schema's per-event requirements, blocking the bad save up front.Loader resilience
partitionValidSubscriptionsto drop individual invalid subs as warnings instead of failing the entire load. Config-level errors (missingsubscriptionsarray, badsettings) remain fatal.Run output
Reading additional input from stdin...in activity log. Cue child processes now spawn withstdio[0] = 'ignore'in local mode socodex execdoesn't emit the stdin banner before observing EOF. SSH stdin-script and SSH small-prompt paths still get a writable pipe. Claude was already correct — Codex needed/dev/nullfor stdin.Test plan
npm run lint— cleannpm run test— 25,260 passing, 0 failing (107 skipped, pre-existing). Includes 65+ new tests acrosscue-cleanup-service,cue-db,cue-fan-in-tracker,cue-session-lifecycle,cueDirtyStore,services/cue, plus extendedcue-engine,usePipelineState,pipelineLayout,usePipelineLayout, and two newCuePipelineEditortest files (allPipelinesViewLock,initialViewport).Agent AwithAgent Bin a pipeline → save → close + reopen modal → restart app → still resolves toAgent Btime.scheduledtrigger without entering a time → click Save → seetrigger needs at least one schedule timevalidation error.maestro/cue.yaml(e.g. deleteschedule_times) → toggle Cue off/on → engine logs a singleSkipped invalid subscription...warning instead of red error spam; other agents in the same project keep workingReading additional input from stdin...Commits
b8d35a95— refactor: backend hardening + frontend IPC service layer4cd47878— fix: prune orphan prompts, filter by session, trust agent_id1d917e5e— fix: pipeline persistence, save UX, loader resilience, codex stdin bannerSummary by CodeRabbit
New Features
Bug Fixes
Tests