fix(cue): wire toggle + activity through main, drop dead preload bridges - #983
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 (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR refactors Cue subscription management from renderer IPC to in-process engine delegation. It adds a new ChangesCue Subscription Remote Toggle via In-Process Engine Callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/web-server/web-server-factory.ts`:
- Around line 2305-2309: The subscription IDs are not globally unique because
they are composed only from session.sessionId and sub.name (which is only unique
within a pipeline), causing setCueSubscriptionEnabled(...) to match and toggle
the wrong subscription; update both the listing and toggle paths to use a truly
stable identifier (e.g., include the pipeline discriminator or a persisted
per-subscription id) instead of `${session.sessionId}::${sub.name}`: modify
where IDs are composed (look for usage of session.sessionId and sub.name in
web-server-factory.ts) to emit the new unique id and update
setCueSubscriptionEnabled(...) and any resolver that matches by name to resolve
by this new id consistently so toggles operate on the exact subscription.
🪄 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: 7907f637-c6bf-4532-8e4f-76920cccd74b
📒 Files selected for processing (6)
src/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/main/cue/cue-engine.tssrc/main/index.tssrc/main/preload/process.tssrc/main/web-server/web-server-factory.ts
💤 Files with no reviewable changes (1)
- src/main/preload/process.ts
Greptile SummaryThis PR fixes three dead IPC bridges in the Cue control surface by routing
Confidence Score: 4/5Safe to merge; the fix correctly removes dead IPC bridges and replaces them with direct engine calls. The main thing to watch is the unguarded read-modify-write in The core change is straightforward and well-tested (12 new tests, 115+58 total pass). The only open concern is the read-modify-write in src/main/cue/cue-engine.ts — the Important Files Changed
Sequence DiagramsequenceDiagram
participant WebUI as Web UI / CLI
participant WebServer as WebServerFactory
participant Engine as CueEngine
participant FS as cue.yaml (YAML)
note over WebUI,FS: getCueSubscriptions
WebUI->>WebServer: "GET /cue/subscriptions?sessionId=X"
WebServer->>Engine: getCueGraphData()
Engine-->>WebServer: CueGraphSession[]
WebServer-->>WebUI: CueSubscriptionInfo[] (flattened)
note over WebUI,FS: toggleCueSubscription
WebUI->>WebServer: POST /cue/subscriptions/:id/toggle
WebServer->>Engine: setSubscriptionEnabled(id, enabled)
Engine->>FS: readCueConfigFile(projectRoot)
FS-->>Engine: raw YAML
Engine->>FS: writeCueConfigFile(projectRoot, mutated YAML)
Engine->>Engine: refreshSession(sessionId, projectRoot)
Engine-->>WebServer: boolean
WebServer-->>WebUI: success / false
note over WebUI,FS: getCueActivity
WebUI->>WebServer: "GET /cue/activity?sessionId=X&limit=N"
WebServer->>Engine: getCueActivityLog()
Engine-->>WebServer: CueRunResult[]
WebServer-->>WebUI: CueActivityEntry[] (filtered + mapped)
|
| const file = readCueConfigFile(projectRoot); | ||
| if (!file) return false; | ||
|
|
||
| let parsed: unknown; | ||
| try { | ||
| parsed = yaml.load(file.raw); | ||
| } catch (err) { | ||
| captureException(err, { operation: 'setSubscriptionEnabled:yamlLoad', sessionId }); | ||
| return false; | ||
| } | ||
| if (!parsed || typeof parsed !== 'object') return false; | ||
| const subs = (parsed as Record<string, unknown>).subscriptions; | ||
| if (!Array.isArray(subs)) return false; | ||
|
|
||
| let found = false; | ||
| for (const sub of subs) { | ||
| if (!sub || typeof sub !== 'object') continue; | ||
| if ((sub as Record<string, unknown>).name === subName) { | ||
| (sub as Record<string, unknown>).enabled = enabled; | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!found) return false; | ||
|
|
||
| try { | ||
| const serialized = yaml.dump(parsed, { lineWidth: -1, noRefs: true }); | ||
| writeCueConfigFile(projectRoot, serialized); | ||
| } catch (err) { | ||
| captureException(err, { operation: 'setSubscriptionEnabled:yamlWrite', sessionId }); | ||
| return false; | ||
| } | ||
|
|
||
| this.refreshSession(sessionId, projectRoot); |
There was a problem hiding this comment.
Unguarded read-modify-write on cue.yaml
readCueConfigFile → in-memory mutation → writeCueConfigFile is not atomic. If two toggle requests for subscriptions in the same session arrive concurrently (e.g., rapid web-UI clicks, or a simultaneous pipeline-editor save that also calls writeCueConfigFile), whichever write lands second silently discards the first write's changes. Existing callers use writeCueConfigFile too (the pipeline editor save path), so the window is real even for a single user. A simple guard — a per-projectRoot mutex or a "read the file again immediately before writing and compare" — would close it.
|
@chr1syy thanks for the follow-up — nice surgical cleanup, and the test coverage on both new paths ( One concern from the bot reviews I'd like your read on before I approve: Subscription identity (CodeRabbit, major).
Whichever you prefer is fine, but the current shape is foot-gun-y once someone has two pipelines. Read-modify-write atomicity (Greptile, P2). Same trade-off as the pipeline editor's save path, as you noted, so I'd just file it as a known limitation rather than blocking on it here. If you want to preemptively add a single-flight lock per project root in a follow-up that'd be welcome, but not for this PR. Happy to merge as soon as the identity question is resolved (either a fix or a "won't happen because X" answer). |
Two review fixes for PR RunMaestro#982: - **Subscription id disambiguation (CodeRabbit major on follow-up RunMaestro#983, Pedram).** The old `\${sessionId}::\${name}` shape collides when two pipelines under the same session each legitimately define a sub with the same name. A downstream toggle would resolve by name and mutate the wrong row. New format `\${sessionId}::\${pipeline}::\${name}` carries the pipeline discriminator. Extracted compose/parse/pipeline- key helpers into a new `src/shared/cue/subscription-id.ts` module so the toggle path in PR RunMaestro#983 reuses the same identity contract. `pipelineKeyForSubscription` prefers explicit \`pipeline_name\` and falls back to the same suffix-stripping as `yamlToPipeline.ts`'s `getBasePipelineName` for legacy YAML. Compose throws on hand-edited YAML where any component contains the `::` separator instead of silently producing an unparseable id. - **schedule_days surfaced in CLI output (Greptile P2 + Pedram).** `cue list` used to render day-pinned schedules as just the times (or `undefined` when only days were set). New `formatCueSchedule` helper renders `\"07:00 (Mon, Wed, Fri)\"` when both are present and `\"days: Sat, Sun\"` when only days are present. Tests: - New `subscription-id.test.ts` with 14 cases covering compose, parse, pipeline-key derivation, collision avoidance, and the `::` guard. - `web-server-factory.test.ts`: updated the flatten test to assert the new id format; added regressions for (a) two pipelines/same name id disambiguation, (b) legacy YAML fallback to base-name stripping, (c) schedule_days rendering alongside schedule_times and standalone. - 68/68 tests pass in the touched suites; prettier + ESLint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oot mutex Two follow-up fixes layered on top of PR RunMaestro#983's toggle/activity migration: - **Pipeline-disambiguated subscription id (CodeRabbit major, Pedram).** `CueEngine.setSubscriptionEnabled` now parses `${sessionId}::${pipeline}::${name}` via `parseCueSubscriptionId` (the helper added in the parent RunMaestro#982 update) and matches the YAML row by BOTH pipeline AND name. Activity-log mapping in the web-server factory composes ids the same way via `composeCueSubscriptionId`, so a web UI could navigate from an activity row to the toggle callback without re-deriving identity. Without this, two pipelines under one session that each define a sub named "Foo" produced indistinguishable ids and the first-match heuristic would silently toggle the wrong row. - **Per-projectRoot write serialisation (Greptile P2).** The read → mutate → write cycle on cue.yaml is not atomic on the filesystem. Two concurrent toggles for subs in the same project (rapid web-UI clicks, or a toggle racing the pipeline editor's save) would otherwise let whichever write lands second silently discard the first's flip. The engine now keeps a `yamlWriteChains: Map<projectRoot, Promise>` and appends each call to its project's chain, so writes serialise per file even under concurrent invocation. The map entry is dropped when the chain settles to keep this from leaking across the engine's lifetime. `setSubscriptionEnabled` is now `async` and returns `Promise<boolean>`. Updated the `setCueSubscriptionEnabled` dep type and the `main/index.ts` wiring accordingly. Tests: - `cue-engine.test.ts`: rewrote the existing 5 tests for the new id format and async signature; added a "cross-pipeline no silent toggle" regression (the CodeRabbit case verbatim — id `session-1::Pipeline B::Foo` must not match a row with `pipeline_name: Pipeline A`), a happy-path test for two same-named subs in one session, and a serialisation test that asserts strict `read,write,read,write` ordering on two Promise.all'd toggles. - `web-server-factory.test.ts`: bumped id format in the toggle + activity expectations and added a fallback test for activity entries whose `CueRunResult.pipelineName` is absent. 194/194 tests pass across the three touched suites; prettier + ESLint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ba16d45 to
5324e61
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/shared/cue/subscription-id.ts (1)
37-45:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftLegacy subscriptions without
pipeline_nameare still ambiguous.This only disambiguates rows when the pipeline can actually be recovered. For legacy subs where
pipeline_nameis missing and the name does not encode a distinct suffix,pipelineKeyForSubscription()collapses toname, so two same-named subs in one session still serialize to the samesessionId::name::nameID. That reopens the exact wrong-row toggle bug for the legacy path this helper is trying to preserve. Please either materialize a real pipeline discriminator before exposing remote IDs, or reject unresolved legacy rows instead of composing a supposedly stable ID.Also applies to: 55-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/cue/subscription-id.ts` around lines 37 - 45, pipelineKeyForSubscription currently falls back to the raw name for legacy rows, which can produce non-unique keys; update pipelineKeyForSubscription (and its sibling logic around lines 55-69) to detect when pipeline_name is missing and the name normalization does not change the value (i.e., no distinct suffix) and then either (A) refuse to produce a key by throwing a clear error or returning a failure so callers can reject unresolved legacy subscriptions, or (B) materialize a stable discriminator (e.g., derive/append a stored pipeline id or a deterministic hash from subscription metadata) before composing the remote ID; locate and change pipelineKeyForSubscription and any related functions that build sessionId::name::name IDs to implement one of these two options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/shared/cue/subscription-id.ts`:
- Around line 37-45: pipelineKeyForSubscription currently falls back to the raw
name for legacy rows, which can produce non-unique keys; update
pipelineKeyForSubscription (and its sibling logic around lines 55-69) to detect
when pipeline_name is missing and the name normalization does not change the
value (i.e., no distinct suffix) and then either (A) refuse to produce a key by
throwing a clear error or returning a failure so callers can reject unresolved
legacy subscriptions, or (B) materialize a stable discriminator (e.g.,
derive/append a stored pipeline id or a deterministic hash from subscription
metadata) before composing the remote ID; locate and change
pipelineKeyForSubscription and any related functions that build
sessionId::name::name IDs to implement one of these two options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8b7a04a-3945-496b-a226-ec81c22b9182
📒 Files selected for processing (8)
src/__tests__/main/cue/cue-engine.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/shared/cue/subscription-id.test.tssrc/main/cue/cue-engine.tssrc/main/index.tssrc/main/preload/process.tssrc/main/web-server/web-server-factory.tssrc/shared/cue/subscription-id.ts
💤 Files with no reviewable changes (1)
- src/main/preload/process.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/index.ts
- src/tests/main/web-server/web-server-factory.test.ts
…er IPC (#982) * fix(cue): answer get_cue_subscriptions from main, not via dead renderer IPC The web-server factory used to fulfil `get_cue_subscriptions` (the CLI's `maestro-cli cue list` message) by forwarding `remote:getCueSubscriptions` to the renderer and waiting 30 s for a response — but no renderer code ever registered a handler for that channel. The CLI's 10 s timeout fired on every call and `cue list --json` surfaced as `Command timed out waiting for cue_subscriptions`. Mirror the `triggerCueSubscription` pattern: inject a `getCueGraphData` dep that calls `cueEngine.getGraphData()` directly in main, then flatten the result into `CueSubscriptionInfo[]` inside the callback. The engine lives in main anyway, so the renderer round-trip was never needed. Per-subscription `lastTriggered` / `triggerCount` aren't tracked by the engine yet (only per-session) — leave the numeric fields zero so the CLI renders "never triggered" rather than fabricating values. Adds three regression tests in `web-server-factory.test.ts`: - flattens engine graph data into CueSubscriptionInfo[] without IPC - filters by sessionId when supplied - returns [] and warns when the dep is missing The dead `remote:toggleCueSubscription` and `remote:getCueActivity` bridges have the same shape — same diagnosis, same fix path — but neither is reachable from any current CLI command. Tracked for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cue): pipeline-disambiguated sub ids + surface schedule_days Two review fixes for PR #982: - **Subscription id disambiguation (CodeRabbit major on follow-up #983, Pedram).** The old `\${sessionId}::\${name}` shape collides when two pipelines under the same session each legitimately define a sub with the same name. A downstream toggle would resolve by name and mutate the wrong row. New format `\${sessionId}::\${pipeline}::\${name}` carries the pipeline discriminator. Extracted compose/parse/pipeline- key helpers into a new `src/shared/cue/subscription-id.ts` module so the toggle path in PR #983 reuses the same identity contract. `pipelineKeyForSubscription` prefers explicit \`pipeline_name\` and falls back to the same suffix-stripping as `yamlToPipeline.ts`'s `getBasePipelineName` for legacy YAML. Compose throws on hand-edited YAML where any component contains the `::` separator instead of silently producing an unparseable id. - **schedule_days surfaced in CLI output (Greptile P2 + Pedram).** `cue list` used to render day-pinned schedules as just the times (or `undefined` when only days were set). New `formatCueSchedule` helper renders `\"07:00 (Mon, Wed, Fri)\"` when both are present and `\"days: Sat, Sun\"` when only days are present. Tests: - New `subscription-id.test.ts` with 14 cases covering compose, parse, pipeline-key derivation, collision avoidance, and the `::` guard. - `web-server-factory.test.ts`: updated the flatten test to assert the new id format; added regressions for (a) two pipelines/same name id disambiguation, (b) legacy YAML fallback to base-name stripping, (c) schedule_days rendering alongside schedule_times and standalone. - 68/68 tests pass in the touched suites; prettier + ESLint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to fix/cue-remote-subscriptions (PR RunMaestro#982). The same dead-bridge shape existed for `remote:toggleCueSubscription` and `remote:getCueActivity`: the web-server factory forwarded each request to the renderer and waited for a response on an IPC channel no renderer code had ever subscribed to. Web-UI toggles silently no-op'd after a 10 s stall and the activity tab rendered empty after a 30 s stall. Mirror the `triggerCueSubscription` / `getCueGraphData` pattern: add two deps on `WebServerFactoryDependencies` — `setCueSubscriptionEnabled` and `getCueActivityLog` — and wire them in `main/index.ts` from `cueEngine`. The toggle flow needs an engine-side mutation, so add `CueEngine.setSubscriptionEnabled(subscriptionId, enabled)`. It parses the `${sessionId}::${name}` id format used by `getCueGraphData`'s flattener, looks up the owning session via `deps.getSessions()`, reads → parses → mutates → writes the YAML, and calls `refreshSession`. Comments and field ordering aren't preserved on the round-trip (same trade-off as the pipeline editor's save path). Activity mapping converts `CueRunResult[]` → `CueActivityEntry[]`, applies the optional `sessionId` filter before slicing to `limit`, and collapses engine-side `timeout`/`stopped` statuses into the web-facing `failed` enum so dashboards don't render `?`. Dead-code cleanup: the matching preload bridges (`onRemoteGetCueSubscriptions` / `sendRemoteGetCueSubscriptionsResponse`, `onRemoteToggleCueSubscription` / `sendRemoteToggleCueSubscriptionResponse`, `onRemoteGetCueActivity` / `sendRemoteGetCueActivityResponse`) now have zero callers — `grep -rn` confirms no references anywhere in `src/`. Removed from `src/main/preload/process.ts`. Tests: - `web-server-factory.test.ts`: 7 new tests covering both new callback paths — dispatch through dep, false-return propagation, missing-dep warning, sessionId filter before limit, status-enum collapsing. - `cue-engine.test.ts`: 5 new tests for `setSubscriptionEnabled` covering malformed ids, unresolved session, missing YAML, missing subscription, and the happy path (parse → mutate → write → refresh). All 173 tests in the two suites pass; prettier + ESLint clean on touched files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oot mutex Two follow-up fixes layered on top of PR RunMaestro#983's toggle/activity migration: - **Pipeline-disambiguated subscription id (CodeRabbit major, Pedram).** `CueEngine.setSubscriptionEnabled` now parses `${sessionId}::${pipeline}::${name}` via `parseCueSubscriptionId` (the helper added in the parent RunMaestro#982 update) and matches the YAML row by BOTH pipeline AND name. Activity-log mapping in the web-server factory composes ids the same way via `composeCueSubscriptionId`, so a web UI could navigate from an activity row to the toggle callback without re-deriving identity. Without this, two pipelines under one session that each define a sub named "Foo" produced indistinguishable ids and the first-match heuristic would silently toggle the wrong row. - **Per-projectRoot write serialisation (Greptile P2).** The read → mutate → write cycle on cue.yaml is not atomic on the filesystem. Two concurrent toggles for subs in the same project (rapid web-UI clicks, or a toggle racing the pipeline editor's save) would otherwise let whichever write lands second silently discard the first's flip. The engine now keeps a `yamlWriteChains: Map<projectRoot, Promise>` and appends each call to its project's chain, so writes serialise per file even under concurrent invocation. The map entry is dropped when the chain settles to keep this from leaking across the engine's lifetime. `setSubscriptionEnabled` is now `async` and returns `Promise<boolean>`. Updated the `setCueSubscriptionEnabled` dep type and the `main/index.ts` wiring accordingly. Tests: - `cue-engine.test.ts`: rewrote the existing 5 tests for the new id format and async signature; added a "cross-pipeline no silent toggle" regression (the CodeRabbit case verbatim — id `session-1::Pipeline B::Foo` must not match a row with `pipeline_name: Pipeline A`), a happy-path test for two same-named subs in one session, and a serialisation test that asserts strict `read,write,read,write` ordering on two Promise.all'd toggles. - `web-server-factory.test.ts`: bumped id format in the toggle + activity expectations and added a fallback test for activity entries whose `CueRunResult.pipelineName` is absent. 194/194 tests pass across the three touched suites; prettier + ESLint clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5324e61 to
983e5b1
Compare
Summary
Follow-up to #982 (`fix/cue-remote-subscriptions`). Same dead-bridge shape existed for two more cue IPC channels — fixing them and removing the unused preload bridges so the cue control surface is consistent.
What was broken
The web-server factory forwarded `remote:toggleCueSubscription` and `remote:getCueActivity` to the renderer and waited 10 s / 30 s for a response on IPC channels no renderer code had ever subscribed to. Net effect:
Fix
Mirror the `triggerCueSubscription` / `getCueGraphData` pattern from #982. Two new deps on `WebServerFactoryDependencies`:
Wired in `main/index.ts` from `cueEngine`. The toggle needs an engine-side mutation, so this PR adds `CueEngine.setSubscriptionEnabled`, which parses the `${sessionId}::${name}` id format (same format `getCueGraphData`'s flattener emits in #982), looks up the owning session, reads → parses → mutates → writes the YAML, and calls `refreshSession`. Comments and field ordering aren't preserved on the round-trip — same trade-off as the pipeline editor's save path.
Activity mapping converts engine `CueRunResult[]` → web `CueActivityEntry[]`, applies the optional `sessionId` filter before slicing to `limit` (so the caller gets N matching entries, not N total of which some are filtered), and collapses engine-side `timeout` / `stopped` into the web-facing `failed` enum.
Dead-code cleanup
Removed the six matching preload bridges from `src/main/preload/process.ts`:
`grep -rn` confirms zero remaining references anywhere in `src/`. The trigger bridge (`onRemoteTriggerCueSubscription`) is kept — it's still wired and used by `useRemoteIntegration`.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes