Skip to content

fix: track subgraph history#2141

Open
maxy-shpfy wants to merge 1 commit into04-21-fix_annotations_in_configuration_section_respect_ignore_keys_listfrom
04-22-fix_track_subgraph_history
Open

fix: track subgraph history#2141
maxy-shpfy wants to merge 1 commit into04-21-fix_annotations_in_configuration_section_respect_ignore_keys_listfrom
04-22-fix_track_subgraph_history

Conversation

@maxy-shpfy
Copy link
Copy Markdown
Collaborator

@maxy-shpfy maxy-shpfy commented Apr 23, 2026

Description

Subgraph specs are now stored as live ComponentSpec model instances directly on the Task entity (task.subgraphSpec) rather than as serialized JSON embedded in task.componentRef.spec. A new task.resolvedComponentSpec computed property provides a unified read path that returns either a lazy proxy over the live subgraph model or the plain JSON spec from componentRef.spec.

Key changes:

  • Task gains a subgraphSpec: ComponentSpec | undefined prop and a setSubgraphSpec action. setComponentRef now automatically promotes any inline graph spec JSON to a live model via deserializeSubgraphSpec, keeping raw JSON out of the runtime model tree.
  • YamlDeserializer calls maybeDeserializeSubgraph before generating a task's $id, so subgraph entity IDs are always allocated before the parent task ID — preserving the collectIdStack / ReplayIdGenerator ordering contract.
  • collectIdStack recurses into task.subgraphSpec before pushing the parent task's $id, matching the new deserializer ordering.
  • JsonSerializer re-attaches the serialized subgraph into componentRef.spec at serialization time, so the wire format is unchanged.
  • createComponentSpecProxy provides a lazy ES Proxy over a live ComponentSpec that presents as ComponentSpecJson, used by resolvedComponentSpec to avoid upfront serialization cost.
  • NavigationStore no longer maintains a separate nestedSpecs map or a syncNestedSpecs reaction. Navigation into subgraphs now directly returns task.subgraphSpec, and getSpecAtDepth walks the live model tree. A new parentContext computed property exposes the parent spec and task ID when navigating inside a subgraph.
  • IO actions (renameInput, renameOutput, deleteInput, deleteOutput) now accept an optional ParentContext and propagate port renames and deletions to the parent spec's bindings and task arguments via new io.parentPropagation helpers.
  • deleteNode on NodeTypeManifest now accepts an optional ParentContext so input and output node deletions can propagate to the parent spec.
  • collectValidationIssues, unpackSubgraph, createSubgraph, and all UI components (TaskDetails, RootNode, SubgraphNode, PipelineTreeContent, resolution components, etc.) are updated to use task.subgraphSpec and task.resolvedComponentSpec instead of casting task.componentRef.spec.
  • The isSubgraphTask utility function and related nestedSpecs lookups are removed in favour of the direct task.subgraphSpec !== undefined check.

Related Issue and Pull requests

Type of Change

  • Bug fix
  • New feature
  • Improvement
  • Cleanup/Refactor
  • Breaking change
  • Documentation update

Checklist

  • I have tested this does not break current pipelines / runs functionality
  • I have tested the changes on staging

Screenshots (if applicable)

Screen Recording 2026-04-22 at 10.21.26 PM.mov (uploaded via Graphite)

Test Instructions

  1. Load a pipeline that contains nested subgraphs and verify the tree view renders correctly.
  2. Navigate into a subgraph, rename or delete an input or output, navigate back, and confirm the parent spec's bindings and task arguments are updated correctly.
  3. Use "Create Subgraph" on a selection of tasks and verify the resulting subgraph task serializes correctly to YAML.
  4. Use "Unpack Subgraph" and verify the inner tasks are correctly inlined into the parent spec.
  5. Run the unit tests for componentSpecProxy, persistentUndo, io.parentPropagation, and collectIssues to confirm ID replay round-trips and parent propagation work correctly for specs with subgraphs.

Additional Comments

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

🎩 Preview

A preview build has been created at: 04-22-fix_track_subgraph_history/1c65536

@maxy-shpfy maxy-shpfy force-pushed the 04-22-fix_track_subgraph_history branch from f53b532 to 1c65536 Compare April 23, 2026 05:19
@maxy-shpfy maxy-shpfy marked this pull request as ready for review April 23, 2026 05:27
@maxy-shpfy maxy-shpfy requested a review from a team as a code owner April 23, 2026 05:27
if (!active) return false;
const task = active.tasks.find((t) => t.$id === taskEntityId);
return task?.subgraphSpec !== undefined;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This is an AI-generated code review comment.

** silently ignores its spec parameter**

The method now unconditionally uses this.activeSpec instead of the passed spec argument. All current callers happen to pass the active spec, so behaviour is correct today — but the public API is now misleading. A future caller that passes a different spec (e.g. a secondary view or prefetch path) will silently search the wrong spec with no type error.

Consider either dropping the parameter entirely and updating callers, or using the passed spec directly:

isTaskSubgraph(spec: ComponentSpec, taskEntityId: string): boolean {
  const task = spec.tasks.find((t) => t.$id === taskEntityId);
  return task?.subgraphSpec !== undefined;
}

case "outputs":
return spec.outputs.map(outputModelToSpec);
case "implementation":
return serializer.serialize(spec).implementation;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This is an AI-generated code review comment.

Proxy implementation getter triggers full serialization on every access

case "implementation":
  return serializer.serialize(spec).implementation;

serializer.serialize(spec) recursively serializes the entire spec (all tasks, bindings, arguments) every time .implementation is accessed on the proxy. The proxy is created fresh on each access to task.resolvedComponentSpec (a new Proxy object is returned each time), so MobX observer components can trigger this serialization repeatedly per render cycle.

In practice the main consumers only need .implementation to type-check (e.g. isSubgraph, isGraphImplementation). A lighter alternative: since we already know a subgraphSpec exists when this proxy is created, we can return a sentinel graph implementation directly:

case "implementation":
  // Subgraph proxies always have a graph implementation.
  // Full serialization is deferred to serializer.serialize() callers.
  return { graph: serializer.buildGraph(spec) };

Or, if the full serialized implementation is genuinely needed here, consider caching via a MobX @computed on the Task model rather than re-running on every proxy property access.

if (argIdx >= 0) {
parentTask.setArgument(newName, parentTask.arguments[argIdx].value);
parentTask.removeArgumentByName(oldName);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This is an AI-generated code review comment.

Renamed argument drifts to end of task's argument list

parentTask.setArgument(newName, parentTask.arguments[argIdx].value);
parentTask.removeArgumentByName(oldName);

setArgument appends at the end when newName doesn't already exist, then removeArgumentByName removes the old entry. The renamed argument moves from its original position to the end of parentTask.arguments. This changes the argument ordering in the serialized YAML on every input rename (minor, but affects output stability).

An in-place rename preserves the position:

if (argIdx >= 0) {
  const arg = parentTask.arguments[argIdx];
  parentTask.arguments[argIdx] = { ...arg, name: newName };
}

(Wrapping this in a @modelAction on Task, e.g. renameArgument(oldName, newName), would be cleaner.)

Copy link
Copy Markdown
Collaborator

@camielvs camielvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I make a subgraph, go into it and UNDO the editor fails

image.png

interestingly, in this failure state I can still submit my run... we should probably fix that
image.png

Copy link
Copy Markdown
Collaborator Author

maxy-shpfy commented Apr 23, 2026

@camielvs This is fixed in downstream PR

export function createComponentSpecProxy(
spec: ComponentSpec,
): ComponentSpecJson {
return new Proxy({} as ComponentSpecJson, {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make this more readable can we extract the Proxy value into a variable?

*/
export function collectIdStack(spec: ComponentSpec): string[] {
const ids: string[] = [];
collectIds(spec, ids);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting to move away from an explicit return. Is that because of the recursion now introduced?

Copy link
Copy Markdown
Collaborator

@camielvs camielvs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. nice to see a lot of deleted (simplified/reused) code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants