fix(cue): seed lastWrittenRootsRef with per-pipeline write roots (#847)#849
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new utility to compute the single write-root for Cue pipelines (preferring sessionId, falling back to sessionName), collapses multi-agent roots to a common ancestor when appropriate, integrates this into pipeline layout seeding, and adds comprehensive tests for single- and multi-pipeline and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as usePipelineLayout
participant Resolver as resolvePipelinesWriteRoots
participant PipelineFn as resolvePipelineWriteRoot
participant SessionsId as sessionsById
participant SessionsName as sessionsByName
Hook->>Resolver: request roots for pipelines
Resolver->>PipelineFn: for each pipeline -> resolve write root
PipelineFn->>SessionsId: lookup by sessionId (first)
alt sessionId not found
PipelineFn->>SessionsName: lookup by sessionName (fallback)
end
PipelineFn-->>Resolver: return single root or null
Resolver-->>Hook: return Set of distinct non-null roots
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 fixes a bug (#847) where a deleted cross-directory Cue pipeline would reappear after closing and reopening the Cue view. The root cause was that Confidence Score: 5/5Safe to merge — the fix correctly seeds lastWrittenRootsRef with the common-ancestor write root, and all remaining findings are minor style/edge-case suggestions. The root cause is correctly diagnosed and fixed: resolvePipelinesWriteRoots now computes the common-ancestor write root that mirrors handleSave's partitioning. The only finding is a P2 edge case where the ?? fallback fires when a session is found by ID but has an undefined projectRoot — unlikely in practice. All 13 new tests pass and 556 existing Cue tests are unaffected. pipelineRoots.ts line 54–61 (the ?? fallback) could use a minor tightening, but it does not block merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant L as usePipelineLayout
participant H as resolvePipelinesWriteRoots
participant P as usePipelinePersistence (handleSave)
participant D as Disk (cue.yaml)
Note over U,D: Load phase (fixed by this PR)
U->>L: Opens Cue view
L->>D: loadPipelineLayout()
D-->>L: savedLayout (with writtenRoots)
L->>H: resolvePipelinesWriteRoots(pipelines, sessionsById, sessionsByName)
H-->>L: {/project} — common ancestor, NOT /project/a or /project/Digest
L->>L: lastWrittenRootsRef ← union(savedLayout.writtenRoots, {/project})
Note over U,D: Delete + Save phase
U->>P: click X → Confirm → Save
P->>P: currentRoots = {}
P->>P: previousRoots = {/project}
P->>D: writeYaml(/project, empty)
D-->>P: verified empty
Note over U,D: Reopen (bug fixed)
U->>L: Opens Cue view
L->>D: getGraphData()
D-->>L: no pipelines found
L->>U: pipeline stays gone ✓
Reviews (1): Last reviewed commit: "fix(cue): seed lastWrittenRootsRef with ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const root = | ||
| sessionsById.get(data.sessionId)?.projectRoot ?? | ||
| sessionsByName.get(data.sessionName)?.projectRoot; | ||
| if (root) { | ||
| roots.add(root); | ||
| } else { | ||
| missingRoot = true; | ||
| } |
There was a problem hiding this comment.
projectRoot, not just missing session
The ?? operator falls back to sessionsByName not only when the session is absent from sessionsById, but also when the session is found but has projectRoot: undefined. If another session happens to share data.sessionName and carries a different root, the helper silently seeds that root instead of treating the agent as unresolvable — diverging from handleSave's missingRoot branch.
The test at line 151–155 covers this path with an empty byName map, so the fallback path is not exercised. A safer approach is to check for the session record first:
const session =
sessionsById.get(data.sessionId) ??
sessionsByName.get(data.sessionName);
const root = session?.projectRoot;This keeps the "prefer ID, fall back to name" intent while ensuring a session found by ID — even one without a projectRoot — is never silently replaced by a same-named session.
There was a problem hiding this comment.
Thanks — I considered this but kept the current form. The helper's explicit purpose is parity with handleSave's resolveRoot (usePipelinePersistence.ts:127-133), which uses if (byId?.projectRoot) return byId.projectRoot; if (byName?.projectRoot) .... That falls back to the byName lookup when the ID-resolved session exists but has a falsy projectRoot — the exact case your suggestion would stop falling back in.
Switching to const session = byId ?? byName; const root = session?.projectRoot; would make the helper stricter than handleSave, which defeats the whole point of the helper (keep lastWrittenRootsRef in sync with where handleSave will actually write).
If the underlying fallback-on-undefined behavior is wrong, it should be fixed in handleSave first (out of scope for this PR) and the helper updated alongside to preserve parity. I'll leave a comment in the code pointing at this invariant so a future consolidation patch can move both sites at once.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/hooks/cue/usePipelineLayout.ts`:
- Around line 213-214: The current construction of sessionsByName using new
Map(sessions.map(...)) implements a last-wins collision rule which diverges from
handleSave's first-wins behavior and can seed lastWrittenRootsRef with the wrong
projectRoot; change the sessionsByName creation in usePipelineLayout to iterate
sessions and call sessionsByName.set(s.name, s) only if
sessionsByName.has(s.name) is false (i.e., first-wins), mirroring handleSave's
semantics so sessionName lookups and lastWrittenRootsRef seeding match
handleSave.
🪄 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: a6f6842d-8cdb-420c-90e6-26fbe19f653f
📒 Files selected for processing (3)
src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineRoots.test.tssrc/renderer/components/CuePipelineEditor/utils/pipelineRoots.tssrc/renderer/hooks/cue/usePipelineLayout.ts
CodeRabbit review on RunMaestro#849: the load-side sessionsByName map used `new Map(sessions.map(...))` (last-wins on duplicate names), while handleSave builds the equivalent map with explicit first-wins (`if (!sessionsByName.has(s.name)) sessionsByName.set(...)`). For pipelines that fall back to sessionName lookup, this could resolve to a different projectRoot than handleSave will actually write to — defeating the parity invariant that `resolvePipelinesWriteRoots` exists to guarantee. Aligned the map construction with handleSave's behavior. No behavior change in the common case (unique session names) — strictly a fix for the duplicate-name edge case.
jSydorowicz21
force-pushed
the
fix/847-cue-scenario-deletion-persist
branch
2 times, most recently
from
a0db9af to
d8d441b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/components/CuePipelineEditor/utils/pipelineRoots.ts (2)
52-62: Minor: empty-stringsessionId/sessionNamecould accidentally hit map entries.If
data.sessionIdordata.sessionNameis an empty string (or a stray map key like''exists),sessionsById.get('')will behave like any other lookup rather than being treated as "missing". In practice agents should always have a non-empty identifier, so this is more of a defensive hardening note than a real bug — consider guarding with a truthiness check on the id/name before the lookup if upstream data isn't guaranteed to be well-formed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/utils/pipelineRoots.ts` around lines 52 - 62, The loop over agents can treat empty-string sessionId/sessionName as valid map keys; update the logic in the block that reads AgentNodeData (data.sessionId / data.sessionName) so you first check truthiness (e.g., if (data.sessionId) and/or if (data.sessionName)) before calling sessionsById.get(...) or sessionsByName.get(...), and only add to roots or set missingRoot when a non-empty identifier fails to resolve; ensure you still mark missingRoot = true when both id and name are absent or non-resolving.
75-78: Nit:computeCommonAncestorPathnever returnsnullfor a non-empty input.Per
src/shared/cue-path-utils.ts(lines 17–39), the function only returnsnullwhen the input array is empty; otherwise it falls back topath.sepwhen there's no shared prefix. Since we've already ensuredroots.size >= 2here, thecommonRoot === nullbranch on line 76 is effectively unreachable, and the unrelated-trees case is actually caught by theallDescendantscheck on line 77 (wherepath.sepwon't be a descendant-or-equal for any real agent root). The current code is correctly defensive, so no change needed — just worth being aware of where the real guard lives, in casecomputeCommonAncestorPath's contract ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/CuePipelineEditor/utils/pipelineRoots.ts` around lines 75 - 78, computeCommonAncestorPath(...) never returns null for non-empty input, so the explicit commonRoot === null branch is unreachable; remove that branch and collapse the logic to compute commonRoot = computeCommonAncestorPath([...roots]) and return allDescendants ? commonRoot : null (where allDescendants uses isDescendantOrEqual). Also add a brief inline comment near computeCommonAncestorPath / commonRoot stating that computeCommonAncestorPath only returns null for empty arrays so the null check was removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/components/CuePipelineEditor/utils/pipelineRoots.ts`:
- Around line 52-62: The loop over agents can treat empty-string
sessionId/sessionName as valid map keys; update the logic in the block that
reads AgentNodeData (data.sessionId / data.sessionName) so you first check
truthiness (e.g., if (data.sessionId) and/or if (data.sessionName)) before
calling sessionsById.get(...) or sessionsByName.get(...), and only add to roots
or set missingRoot when a non-empty identifier fails to resolve; ensure you
still mark missingRoot = true when both id and name are absent or non-resolving.
- Around line 75-78: computeCommonAncestorPath(...) never returns null for
non-empty input, so the explicit commonRoot === null branch is unreachable;
remove that branch and collapse the logic to compute commonRoot =
computeCommonAncestorPath([...roots]) and return allDescendants ? commonRoot :
null (where allDescendants uses isDescendantOrEqual). Also add a brief inline
comment near computeCommonAncestorPath / commonRoot stating that
computeCommonAncestorPath only returns null for empty arrays so the null check
was removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 209a4a7e-c39d-4a2e-9224-967cd83e42a8
📒 Files selected for processing (3)
src/__tests__/renderer/components/CuePipelineEditor/utils/pipelineRoots.test.tssrc/renderer/components/CuePipelineEditor/utils/pipelineRoots.tssrc/renderer/hooks/cue/usePipelineLayout.ts
✅ Files skipped from review due to trivial changes (1)
- src/tests/renderer/components/CuePipelineEditor/utils/pipelineRoots.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/hooks/cue/usePipelineLayout.ts
Two CodeRabbit nitpicks on RunMaestro#849: 1. Defensive: guard session map lookups with a truthiness check on `data.sessionId` / `data.sessionName` so a stray `''` key in the session maps can't accidentally resolve an agent that should have been treated as missing. Added a regression test. 2. Doc: note that `computeCommonAncestorPath` never returns null for non-empty input, so the `commonRoot === null` branch is effectively unreachable — the real unrelated-trees guard is the `allDescendants` check. Kept the null check as defensive (matches handleSave's pattern) with an inline comment explaining the intent.
jSydorowicz21
force-pushed
the
fix/847-cue-scenario-deletion-persist
branch
from
d8d441b to
276a3d9
Compare
Two CodeRabbit nitpicks on RunMaestro#849: 1. Defensive: guard session map lookups with a truthiness check on `data.sessionId` / `data.sessionName` so a stray `''` key in the session maps can't accidentally resolve an agent that should have been treated as missing. Added a regression test. 2. Doc: note that `computeCommonAncestorPath` never returns null for non-empty input, so the `commonRoot === null` branch is effectively unreachable — the real unrelated-trees guard is the `allDescendants` check. Kept the null check as defensive (matches handleSave's pattern) with an inline comment explaining the intent.
jSydorowicz21
force-pushed
the
fix/847-cue-scenario-deletion-persist
branch
from
276a3d9 to
dc1b469
Compare
|
Merged — thanks for the careful diagnosis and the fix, @jSydorowicz21. The walkthrough in the PR description and the parity reasoning in your review reply made this an easy take. Appreciate the time you put into this and the regression tests for the cross-directory case. 🙏 |
…stry dedup RC brought two Cue-related commits since the cue-polish merge point: #849 — resolvePipelinesWriteRoots: seed lastWrittenRootsRef with per-pipeline write roots (common ancestor) rather than each agent's individual projectRoot, so delete+save clears YAML at the correct location. #873 — owner_agent_id: computeOwnershipWarning + runnableSubscriptions filter in cue-session-runtime-service resolves shared-workspace trigger duplication at init time, surfaces a dashboard warning, and respects explicit YAML config. Our cue-polish commit contained a registry-level first-registered-wins approach for the same #867 problem. That approach is now superseded: runnableSubscriptions already filters unowned subs for non-owner sessions before the trigger-source loop runs, so the claimSharedTriggerOwner check was unreachable for non-owners and redundant for the owner. Resolution: - Keep RC's ownershipWarning/runnableSubscriptions (#873) — more complete, user-visible, handles explicit owner_agent_id YAML config - Remove sharedTriggerOwners map + 3 methods from cue-session-registry.ts - Remove claimSharedTriggerOwner + releaseSharedTriggersForSession calls from cue-session-runtime-service.ts - Delete cue-shared-trigger-dedup.test.ts (tests the removed registry approach; cue-session-state.test.ts from #873 covers computeOwnershipWarning instead) - Keep all other cue-polish fixes: #866 dormant visibility, #868 Run Now dedup, #847 error-node pipeline filter, SessionsTable ownershipWarning indicator 26 371 tests passing, 0 failures.
2 participants
Fixes #847.
The bug
A Cue Scenario deleted via the X button in the dropdown, with Save clicked, reappears after closing and reopening the Cue view.
Root cause
PR #845 added cross-directory pipeline support. When a pipeline's agents span subdirectories (e.g.
/project/aand/project/Digest),handleSaveresolves their common ancestor (/project) and writes ONE YAML at/project/.maestro/cue.yaml.The load path in
usePipelineLayout.ts:201-220was still seedinglastWrittenRootsRefwith each agent's individualprojectRoot— so the common ancestor where the YAML actually lives was missing from the set.That set becomes
previousRootsinhandleSave's clear loop (usePipelinePersistence.ts:254). On delete+save:currentRoots = {}— the deleted pipeline had no survivors at its root.previousRoots = {/project/a, /project/Digest}— missing/project./project/.maestro/cue.yamlis never touched.getGraphData()still reports the pipeline from/project/.maestro/cue.yaml, so the "deleted" pipeline reappears.The same mis-seeding had a secondary effect: on every delete+save, the clear loop was also writing empty
cue.yamlfiles at the subdirectory roots that were never actually write targets, creating stray config files at sub-paths.Fix
Extract the write-root resolution into a shared pure helper
resolvePipelineWriteRoot(src/renderer/components/CuePipelineEditor/utils/pipelineRoots.ts) mirroringhandleSave's happy-path partitioning:null.Use
resolvePipelinesWriteRootsinusePipelineLayout.ts's load-time seeding in place of the per-agent-root loop. The persistedsavedLayout.writtenRootsunion is preserved as the authoritative source when agents have since been renamed or deleted.Walkthrough after the fix (the #847 repro)
/project/.maestro/cue.yamlwith agents in/project/aand/project/Digest.loadedRootswith:savedLayout.writtenRoots(previously saved:{/project}, or empty if never saved via the new version).{/project}(common ancestor of the two sub-directories).previousRoots ⊇ {/project}.currentRoots = {}.previousRoots = {/project}. The loop writes empty YAML at/project/.maestro/cue.yaml.onSaveSuccess → refreshGraphDatare-reads.getGraphData()finds no pipeline. ✓Scope
handleSave's inline partitioning is left unchanged — it carries three branches of error reporting (unresolvedPipelines, "agents span unrelated project roots", missing root) that the helper intentionally does not replicate. A doc comment on the helper calls out the invariant it must stay in sync with.What is NOT changed
handleSaveitself (its partitioning still correct — the bug was only on the load side).writeYaml/readYamlIPC handlers.Tests
13 new cases in
pipelineRoots.test.ts:missingRootparity with handleSave), undefinedprojectRoot.resolvePipelinesWriteRoots: union of distinct roots, dedup of identical roots, skip unresolvable, cross-directory explicit assertion that individual sub-roots are NOT in the set.Verification
npx tsc --noEmit— cleannpx prettier --checkon touched files — cleannpx eslinton touched files — cleanpipelineRoots.test.ts— 13 passed (new)Manual test notes for reviewers
With the debug package from #847, the repro flow is:
On
rc: the pipeline reappears.With this PR: it stays gone, and no stray empty
cue.yamlfiles appear under the sub-directories.Summary by CodeRabbit
Refactor
Tests