A cornucopia of fixes and polish

- **Carry over custom describer when pasting a graph.** - **Make relative module/graph URLs droppable.** - **Pass `outerGraph` correctly to make module-based types runnable again.** - **Teach `GraphBasedNodeHandler` to describe module types.** - **Supply proper loading context when retrieving component types.** - **Supply proper loader context when retrieving type metadata.** - **Allow deleting star edges.** - **Make ad hoc wiring work again.** - **docs(changeset): A cornucopia of fixes and polish** Among other things, fixes #4007
breadboard-ai · Dec 19, 2024 · df6926d · df6926d
1 parent fdf37ae
commit df6926d
Show file tree

Hide file tree

Showing 9 changed files with 99 additions and 81 deletions.
diff --git a/.changeset/long-apples-fetch.md b/.changeset/long-apples-fetch.md
@@ -0,0 +1,7 @@
+---
+"@breadboard-ai/visual-editor": patch
+"@google-labs/breadboard": patch
+"@breadboard-ai/shared-ui": patch
+---
+
+A cornucopia of fixes and polish
diff --git a/packages/breadboard/src/graph-based-node-handler.ts b/packages/breadboard/src/graph-based-node-handler.ts
@@ -6,6 +6,7 @@
 
 import { resolveGraph } from "./loader/loader.js";
 import { invokeGraph } from "./run/invoke-graph.js";
+import { invokeDescriber } from "./sandboxed-run-module.js";
 import {
   GraphDescriptor,
   GraphToRun,
@@ -18,27 +19,9 @@ import {
   NodeTypeIdentifier,
   Schema,
 } from "./types.js";
-import { hash } from "./utils/hash.js";
-import { Throttler } from "./utils/throttler.js";
 
 export { GraphBasedNodeHandler, toNodeHandlerMetadata };
 
-type NodeDescriberThrottler = Throttler<
-  [InputValues | undefined, GraphDescriptor, NodeDescriberContext],
-  NodeDescriberResult
->;
-
-type DescribeThrottlerWithHash = {
-  throttler: NodeDescriberThrottler;
-  hash: number;
-};
-
-const DESCRIBE_THROTTLE_DELAY = 5000;
-const DESCRIBE_RESULT_CACHE = new Map<
-  NodeTypeIdentifier,
-  DescribeThrottlerWithHash
->();
-
 class GraphBasedNodeHandler implements NodeHandlerObject {
   #graph: GraphToRun;
   #type: NodeTypeIdentifier;
@@ -66,29 +49,53 @@ class GraphBasedNodeHandler implements NodeHandlerObject {
 
   async describe(
     inputs?: InputValues,
-    _inputSchema?: Schema,
-    _outputSchema?: Schema,
+    inputSchema?: Schema,
+    outputSchema?: Schema,
     context?: NodeDescriberContext
   ) {
     if (!context) {
       return { inputSchema: {}, outputSchema: {} };
     }
-    const inputsHash = hash(inputs);
-    let describeThrottler = DESCRIBE_RESULT_CACHE.get(this.#type);
-    if (!describeThrottler || describeThrottler.hash !== inputsHash) {
-      describeThrottler = {
-        throttler: new Throttler(describeGraph, DESCRIBE_THROTTLE_DELAY),
-        hash: inputsHash,
-      };
 
-      DESCRIBE_RESULT_CACHE.set(this.#type, describeThrottler);
+    const graphStore = context?.graphStore;
+    if (!graphStore) {
+      return emptyDescriberResult();
+    }
+    const adding = graphStore.addByDescriptor(this.#graph.graph);
+    if (!adding.success) {
+      return emptyDescriberResult();
+    }
+    if (this.#graph.moduleId) {
+      const moduleId = this.#graph.moduleId;
+      const graph = this.#graph.graph;
+      const sandbox = context.graphStore?.sandbox;
+      if (!sandbox) {
+        console.warn("Sandbox was not supplied to describe node type");
+        return emptyDescriberResult();
+      }
+      const result = await invokeDescriber(
+        moduleId,
+        sandbox,
+        graph,
+        inputs || {},
+        inputSchema,
+        outputSchema
+      );
+      if (!result) {
+        return emptyDescriberResult();
+      }
+      return result;
+    } else {
+      const inspectableGraph = graphStore.inspect(
+        adding.result,
+        this.#graph.subGraphId || ""
+      );
+      if (!inspectableGraph) {
+        return emptyDescriberResult();
+      }
+      const result = await inspectableGraph.describe(inputs);
+      return result;
     }
-    return describeThrottler.throttler.call(
-      {},
-      inputs,
-      this.#descriptor,
-      context
-    );
   }
 
   get metadata() {
@@ -106,27 +113,6 @@ function toNodeHandlerMetadata(graph: GraphDescriptor): NodeHandlerMetadata {
   });
 }
 
-async function describeGraph(
-  inputs: InputValues | undefined,
-  graph: GraphDescriptor,
-  context: NodeDescriberContext
-) {
-  const graphStore = context?.graphStore;
-  if (!graphStore) {
-    return emptyDescriberResult();
-  }
-  const adding = graphStore.addByDescriptor(graph);
-  if (!adding.success) {
-    return emptyDescriberResult();
-  }
-  const inspectableGraph = graphStore.inspect(adding.result, "");
-  if (!inspectableGraph) {
-    return emptyDescriberResult();
-  }
-  const result = await inspectableGraph.describe(inputs);
-  return result;
-}
-
 /**
  * A utility function to filter out empty (null or undefined) values from
  * an object.

diff --git a/packages/breadboard/src/handler.ts b/packages/breadboard/src/handler.ts
@@ -5,8 +5,9 @@
  */
 
 import { GraphBasedNodeHandler } from "./graph-based-node-handler.js";
-import { MutableGraphStore } from "./inspector/types.js";
-import { SENTINEL_BASE_URL } from "./loader/loader.js";
+import { contextFromMutableGraph } from "./inspector/graph-store.js";
+import { MutableGraph } from "./inspector/types.js";
+import { getGraphUrl } from "./loader/loader.js";
 import type {
   InputValues,
   Kit,
@@ -88,26 +89,30 @@ export async function getHandler(
   throw new Error(`No handler for node type "${type}"`);
 }
 
-export async function getGraphHandlerFromStore(
+export async function getGraphHandlerFromMutableGraph(
   type: NodeTypeIdentifier,
-  store: MutableGraphStore
+  mutable: MutableGraph
 ): Promise<NodeHandlerObject | undefined> {
   const nodeTypeUrl = graphUrlLike(type)
-    ? new URL(type, SENTINEL_BASE_URL)
+    ? getGraphUrl(type, contextFromMutableGraph(mutable))
     : undefined;
   if (!nodeTypeUrl) {
     return undefined;
   }
-  const result = store.addByURL(type, [], {}).mutable;
+  const store = mutable.store;
+  const result = store.addByURL(type, [], {
+    outerGraph: mutable.graph,
+  }).mutable;
   return new GraphBasedNodeHandler({ graph: result.graph }, type);
 }
 
 export async function getGraphHandler(
   type: NodeTypeIdentifier,
   context: NodeHandlerContext
 ): Promise<NodeHandlerObject | undefined> {
-  const { base = SENTINEL_BASE_URL } = context;
-  const nodeTypeUrl = graphUrlLike(type) ? new URL(type, base) : undefined;
+  const nodeTypeUrl = graphUrlLike(type)
+    ? getGraphUrl(type, context)
+    : undefined;
   if (!nodeTypeUrl) {
     return undefined;
   }

diff --git a/packages/breadboard/src/inspector/graph-store.ts b/packages/breadboard/src/inspector/graph-store.ts
@@ -34,14 +34,16 @@ import { createBuiltInKit } from "./graph/kits.js";
 import { graphUrlLike } from "../utils/graph-url-like.js";
 import { getGraphUrl, getGraphUrlComponents } from "../loader/loader.js";
 
-export { GraphStore, makeTerribleOptions, contextFromStore };
+export { GraphStore, makeTerribleOptions, contextFromMutableGraph };
 
-function contextFromStore(store: MutableGraphStore): NodeHandlerContext {
+function contextFromMutableGraph(mutable: MutableGraph): NodeHandlerContext {
+  const store = mutable.store;
   return {
     kits: [...store.kits],
     loader: store.loader,
     sandbox: store.sandbox,
     graphStore: store,
+    outerGraph: mutable.graph,
   };
 }
 

diff --git a/packages/breadboard/src/inspector/graph/describer-manager.ts b/packages/breadboard/src/inspector/graph/describer-manager.ts
@@ -40,7 +40,7 @@ import { GraphDescriptorHandle } from "./graph-descriptor-handle.js";
 import { combineSchemas, removeProperty } from "../../schema.js";
 import { Result } from "../../editor/types.js";
 import { invokeGraph } from "../../run/invoke-graph.js";
-import { contextFromStore } from "../graph-store.js";
+import { contextFromMutableGraph } from "../graph-store.js";
 import { SchemaDiffer } from "../../utils/schema-differ.js";
 import { UpdateEvent } from "./event.js";
 
@@ -114,7 +114,7 @@ class NodeTypeDescriberManager implements DescribeResultCacheArgs {
   ): Promise<NodeDescriberFunction | undefined> {
     let handler: NodeHandler | undefined;
     try {
-      handler = await getHandler(type, contextFromStore(this.mutable.store));
+      handler = await getHandler(type, contextFromMutableGraph(this.mutable));
     } catch (e) {
       console.warn(`Error getting describer for node type ${type}`, e);
     }

diff --git a/packages/breadboard/src/inspector/graph/kits.ts b/packages/breadboard/src/inspector/graph/kits.ts
@@ -5,7 +5,7 @@
  */
 
 import { toNodeHandlerMetadata } from "../../graph-based-node-handler.js";
-import { getGraphHandlerFromStore } from "../../handler.js";
+import { getGraphHandlerFromMutableGraph } from "../../handler.js";
 import {
   GraphDescriptor,
   Kit,
@@ -294,7 +294,7 @@ class CustomNodeType implements InspectableNodeType {
   constructor(type: string, mutable: MutableGraph) {
     this.#type = type;
     this.#mutable = mutable;
-    this.#handlerPromise = getGraphHandlerFromStore(type, mutable.store);
+    this.#handlerPromise = getGraphHandlerFromMutableGraph(type, mutable);
   }
 
   async #readMetadata() {
@@ -308,11 +308,9 @@ class CustomNodeType implements InspectableNodeType {
   }
 
   currentMetadata(): NodeHandlerMetadata {
-    const graph = this.#mutable.store.addByURL(
-      this.#type,
-      [this.#mutable.id],
-      {}
-    ).mutable;
+    const graph = this.#mutable.store.addByURL(this.#type, [this.#mutable.id], {
+      outerGraph: this.#mutable.graph,
+    }).mutable;
     return toNodeHandlerMetadata(graph.graph);
   }
 

diff --git a/packages/breadboard/src/run/node-invoker.ts b/packages/breadboard/src/run/node-invoker.ts
@@ -63,7 +63,12 @@ export class NodeInvoker {
     const { kits = [], base = SENTINEL_BASE_URL, state } = this.#context;
     let outputs: OutputValues | undefined = undefined;
 
-    const handler = await getHandler(descriptor.type, this.#context);
+    const outerGraph = this.#graph.graph;
+
+    const handler = await getHandler(descriptor.type, {
+      ...this.#context,
+      outerGraph,
+    });
 
     const newContext: NodeHandlerContext = {
       ...this.#context,
@@ -73,7 +78,7 @@ export class NodeInvoker {
       // if this.#graph is a subgraph.
       // Or it equals to "board" it this is not a subgraph
       // TODO: Make this more elegant.
-      outerGraph: this.#graph.graph,
+      outerGraph,
       base,
       kits,
       requestInput: this.#requestedInputs.createHandler(

diff --git a/packages/shared-ui/src/elements/editor/graph.ts b/packages/shared-ui/src/elements/editor/graph.ts
@@ -599,7 +599,7 @@ export class Graph extends PIXI.Container {
           nodePortBeingEdited.label !== "" &&
           topTarget instanceof GraphNode &&
           topTarget !== nodeBeingEdited &&
-          isSameGraph
+          !isSameGraph
         ) {
           const targetAllowsAdHocPorts =
             (nodePortBeingEdited.type === GraphNodePortType.IN &&
@@ -790,8 +790,10 @@ export class Graph extends PIXI.Container {
       // Also check that the user isn't trying to attach a star port to a named
       // port.
       if (
-        (targetEdgeDescriptor.out === "*" && targetEdgeDescriptor.in !== "*") ||
-        (targetEdgeDescriptor.out !== "*" && targetEdgeDescriptor.in === "*")
+        !creatingAdHocEdge &&
+        ((targetEdgeDescriptor.out === "*" &&
+          targetEdgeDescriptor.in !== "*") ||
+          (targetEdgeDescriptor.out !== "*" && targetEdgeDescriptor.in === "*"))
       ) {
         canMakeWireModification = false;
       }

diff --git a/packages/visual-editor/src/runtime/util.ts b/packages/visual-editor/src/runtime/util.ts
@@ -39,7 +39,8 @@ export function isBoardArrayBehavior(schema: Schema): boolean {
 }
 
 export function edgeToString(edge: Edge): string {
-  return `${edge.from}:${edge.out}->${edge.to}:${edge.in}`;
+  const edgeIn = edge.out === "*" ? "*" : edge.in;
+  return `${edge.from}:${edge.out}->${edge.to}:${edgeIn}`;
 }
 
 export function inspectableEdgeToString(edge: InspectableEdge): string {
@@ -543,10 +544,12 @@ export function generateAddEditSpecFromDescriptor(
         edits.push({ type: "addedge", edge, graphId });
       }
 
+      const existingMetadata = structuredClone(targetGraph.metadata() ?? {});
+      let updateGraphMetadata = false;
+
       // Comments.
       const comments = sourceGraph.metadata?.comments;
       if (comments) {
-        const existingMetadata = structuredClone(targetGraph.metadata() ?? {});
         existingMetadata.comments ??= [];
         for (const sourceComment of comments) {
           const comment = structuredClone(sourceComment);
@@ -559,8 +562,18 @@ export function generateAddEditSpecFromDescriptor(
             graphOffset
           );
           existingMetadata.comments.push(comment);
+          updateGraphMetadata = true;
         }
+      }
+
+      // Also copy "describer", if present
+      const describer = sourceGraph.metadata?.describer;
+      if (describer) {
+        existingMetadata.describer = describer;
+        updateGraphMetadata = true;
+      }
 
+      if (updateGraphMetadata) {
         edits.push({
           type: "changegraphmetadata",
           metadata: { ...existingMetadata },