addressed pull request comments

Author: emilbon99

client/ts/src/schematic/actions.gen.ts

@@ -12,16 +12,6 @@ import { caseconv, control, record, spatial, zod } from "@synnaxlabs/x";
 import { produce } from "immer";
 import { z } from "zod";
 
-import {
-  handleAddNode,
-  handleRemoveEdge,
-  handleRemoveNode,
-  handleSetAuthority,
-  handleSetConfig,
-  handleSetEdge,
-  handleSetLegend,
-  handleSetNodePosition,
-} from "@/schematic/actions";
 import { edgeZ, legendZ, nodeZ, type Schematic } from "@/schematic/types.gen";
 
 /** SetNodePosition moves a node to a new position. */
@@ -33,15 +23,16 @@ export const setNodePositionPayloadZ = z.object({
 export type SetNodePositionPayload = z.infer<typeof setNodePositionPayloadZ>;
 
 /**
- * AddNode appends a node to the schematic. If config is non-empty it is stored
- * under the node's key in the schematic configs map.
+ * SetNode inserts the node if no node with the same key exists, otherwise
+ * replaces the existing node in place. If config is non-empty it is
+ * stored under the node's key in the schematic configs map.
  */
-export const addNodePayloadZ = z.object({
+export const setNodePayloadZ = z.object({
   node: nodeZ,
   config: caseconv.preserveCase(zod.nullToUndefined(record.unknownZ())),
 });
 
-export type AddNodePayload = z.infer<typeof addNodePayloadZ>;
+export type SetNodePayload = z.infer<typeof setNodePayloadZ>;
 
 /** RemoveNode removes a node and any config stored under its key. */
 export const removeNodePayloadZ = z.object({
@@ -91,7 +82,7 @@ export type SetLegendPayload = z.infer<typeof setLegendPayloadZ>;
 
 export const ACTION_TYPES = {
   set_node_position: "set_node_position",
-  add_node: "add_node",
+  set_node: "set_node",
   remove_node: "remove_node",
   set_edge: "set_edge",
   remove_edge: "remove_edge",
@@ -105,7 +96,7 @@ export const actionZ = z.discriminatedUnion("type", [
     type: z.literal("set_node_position"),
     setNodePosition: setNodePositionPayloadZ,
   }),
-  z.object({ type: z.literal("add_node"), addNode: addNodePayloadZ }),
+  z.object({ type: z.literal("set_node"), setNode: setNodePayloadZ }),
   z.object({ type: z.literal("remove_node"), removeNode: removeNodePayloadZ }),
   z.object({ type: z.literal("set_edge"), setEdge: setEdgePayloadZ }),
   z.object({ type: z.literal("remove_edge"), removeEdge: removeEdgePayloadZ }),
@@ -121,9 +112,9 @@ export const setNodePosition = (payload: SetNodePositionPayload): Action => ({
   setNodePosition: payload,
 });
 
-export const addNode = (payload: AddNodePayload): Action => ({
-  type: "add_node",
-  addNode: payload,
+export const setNode = (payload: SetNodePayload): Action => ({
+  type: "set_node",
+  setNode: payload,
 });
 
 export const removeNode = (payload: RemoveNodePayload): Action => ({
@@ -156,35 +147,51 @@ export const setLegend = (payload: SetLegendPayload): Action => ({
   setLegend: payload,
 });
 
-export const reduce = (state: Schematic, action: Action): Schematic => {
-  switch (action.type) {
-    case "set_node_position":
-      handleSetNodePosition(state, action.setNodePosition);
-      break;
-    case "add_node":
-      handleAddNode(state, action.addNode);
-      break;
-    case "remove_node":
-      handleRemoveNode(state, action.removeNode);
-      break;
-    case "set_edge":
-      handleSetEdge(state, action.setEdge);
-      break;
-    case "remove_edge":
-      handleRemoveEdge(state, action.removeEdge);
-      break;
-    case "set_config":
-      handleSetConfig(state, action.setConfig);
-      break;
-    case "set_authority":
-      handleSetAuthority(state, action.setAuthority);
-      break;
-    case "set_legend":
-      handleSetLegend(state, action.setLegend);
-      break;
-  }
-  return state;
+export interface Handlers {
+  setNodePosition: (state: Schematic, payload: SetNodePositionPayload) => void;
+  setNode: (state: Schematic, payload: SetNodePayload) => void;
+  removeNode: (state: Schematic, payload: RemoveNodePayload) => void;
+  setEdge: (state: Schematic, payload: SetEdgePayload) => void;
+  removeEdge: (state: Schematic, payload: RemoveEdgePayload) => void;
+  setConfig: (state: Schematic, payload: SetConfigPayload) => void;
+  setAuthority: (state: Schematic, payload: SetAuthorityPayload) => void;
+  setLegend: (state: Schematic, payload: SetLegendPayload) => void;
+}
+
+export const createReducer =
+  (handlers: Handlers) =>
+  (state: Schematic, action: Action): Schematic => {
+    switch (action.type) {
+      case "set_node_position":
+        handlers.setNodePosition(state, action.setNodePosition);
+        break;
+      case "set_node":
+        handlers.setNode(state, action.setNode);
+        break;
+      case "remove_node":
+        handlers.removeNode(state, action.removeNode);
+        break;
+      case "set_edge":
+        handlers.setEdge(state, action.setEdge);
+        break;
+      case "remove_edge":
+        handlers.removeEdge(state, action.removeEdge);
+        break;
+      case "set_config":
+        handlers.setConfig(state, action.setConfig);
+        break;
+      case "set_authority":
+        handlers.setAuthority(state, action.setAuthority);
+        break;
+      case "set_legend":
+        handlers.setLegend(state, action.setLegend);
+        break;
+    }
+    return state;
+  };
+
+export const createReduceAll = (handlers: Handlers) => {
+  const reduce = createReducer(handlers);
+  return (state: Schematic, actions: Action[]): Schematic =>
+    produce(state, (draft) => actions.forEach((action) => reduce(draft, action)));
 };
-
-export const reduceAll = (state: Schematic, actions: Action[]): Schematic =>
-  produce(state, (draft) => actions.forEach((action) => reduce(draft, action)));

client/ts/src/schematic/actions.spec.ts

@@ -11,12 +11,11 @@ import { color } from "@synnaxlabs/x";
 import { describe, expect, it } from "vitest";
 import { z } from "zod";
 
+import { reduce, reduceAll } from "@/schematic/actions";
 import {
   type Action,
   actionZ,
-  addNode,
-  reduce,
-  reduceAll,
+  setNode,
   removeEdge,
   removeNode,
   setAuthority,
@@ -83,28 +82,31 @@ describe("schematic reducer", () => {
     });
   });
 
-  describe("addNode", () => {
-    it("should append the node to the end of the slice", () => {
+  describe("setNode", () => {
+    it("should append the node to the end of the slice when no node has the same key", () => {
       const state = empty({ nodes: [node("n1", 0, 0)] });
-      const out = reduceAll(state, [addNode({ node: node("n2", 1, 2) })]);
+      const out = reduceAll(state, [setNode({ node: node("n2", 1, 2) })]);
       expect(out.nodes).toEqual([node("n1", 0, 0), node("n2", 1, 2)]);
     });
     it("should write config under the node's key when config is non-undefined", () => {
       const out = reduceAll(empty(), [
-        addNode({ node: node("n1", 0, 0), config: { label: "Pump", color: "#f00" } }),
+        setNode({ node: node("n1", 0, 0), config: { label: "Pump", color: "#f00" } }),
       ]);
       expect(out.configs).toEqual({ n1: { label: "Pump", color: "#f00" } });
     });
     it("should leave configs untouched when the action's config is undefined", () => {
-      const out = reduceAll(empty(), [addNode({ node: node("n1", 0, 0) })]);
+      const out = reduceAll(empty(), [setNode({ node: node("n1", 0, 0) })]);
       expect(out.configs).toEqual({});
     });
-    it("should append a duplicate-key node, locking current behavior", () => {
-      const state = empty({ nodes: [node("n1", 0, 0)] });
-      const out = reduceAll(state, [addNode({ node: node("n1", 9, 9) })]);
-      expect(out.nodes).toHaveLength(2);
+    it("should replace an existing node in place when the key already exists, preserving slice index", () => {
+      const state = empty({
+        nodes: [node("n1", 0, 0), node("n2", 1, 1), node("n3", 2, 2)],
+      });
+      const out = reduceAll(state, [setNode({ node: node("n2", 9, 9) })]);
+      expect(out.nodes).toHaveLength(3);
       expect(out.nodes[0]).toEqual(node("n1", 0, 0));
-      expect(out.nodes[1]).toEqual(node("n1", 9, 9));
+      expect(out.nodes[1]).toEqual(node("n2", 9, 9));
+      expect(out.nodes[2]).toEqual(node("n3", 2, 2));
     });
   });
 
@@ -253,9 +255,9 @@ describe("schematic reducer", () => {
 
     it("should build a complete graph from an empty schematic", () => {
       const out = reduceAll(empty(), [
-        addNode({ node: node("pump", 0, 0) }),
-        addNode({ node: node("valve", 100, 0) }),
-        addNode({ node: node("tank", 200, 0) }),
+        setNode({ node: node("pump", 0, 0) }),
+        setNode({ node: node("valve", 100, 0) }),
+        setNode({ node: node("tank", 200, 0) }),
         setEdge({ edge: edge("e1", "pump", "out", "valve", "in") }),
         setEdge({ edge: edge("e2", "valve", "out", "tank", "in") }),
         setConfig({ key: "pump", config: { label: "Main Pump" } }),
@@ -277,7 +279,7 @@ describe("schematic reducer", () => {
       });
       const out = reduceAll(state, [
         removeNode({ key: "n1" }),
-        addNode({ node: node("n1", 50, 50) }),
+        setNode({ node: node("n1", 50, 50) }),
       ]);
       expect(out.nodes).toHaveLength(2);
       expect(out.nodes[1]).toEqual(node("n1", 50, 50));
@@ -296,7 +298,7 @@ describe("schematic reducer", () => {
       const state = empty();
       const actions: Action[] = [];
       for (let i = 0; i < 5; i++)
-        actions.push(addNode({ node: node(`n${i}`, i * 100, 0) }));
+        actions.push(setNode({ node: node(`n${i}`, i * 100, 0) }));
       for (let i = 0; i < 5; i++) {
         actions.push(
           setNodePosition({ key: `n${i}`, position: { x: i * 100, y: 50 } }),
@@ -338,7 +340,7 @@ describe("schematic reducer", () => {
 
   describe("single-action reduce", () => {
     it("should apply a single action without wrapping it in an array", () => {
-      const out = reduce(empty(), addNode({ node: node("n1", 1, 2) }));
+      const out = reduce(empty(), setNode({ node: node("n1", 1, 2) }));
       expect(out.nodes).toEqual([node("n1", 1, 2)]);
     });
   });

client/ts/src/schematic/actions.ts

@@ -7,65 +7,43 @@
 // License, use of this software will be governed by the Apache License, Version 2.0,
 // included in the file licenses/APL.txt.
 
-import {
-  type AddNodePayload,
-  type RemoveEdgePayload,
-  type RemoveNodePayload,
-  type SetAuthorityPayload,
-  type SetConfigPayload,
-  type SetEdgePayload,
-  type SetLegendPayload,
-  type SetNodePositionPayload,
-} from "@/schematic/actions.gen";
-import { type Schematic } from "@/schematic/types.gen";
-
-export const handleSetNodePosition = (
-  state: Schematic,
-  payload: SetNodePositionPayload,
-): void => {
-  const node = state.nodes.find((n) => n.key === payload.key);
-  if (node != null) node.position = payload.position;
-};
-
-export const handleAddNode = (state: Schematic, payload: AddNodePayload): void => {
-  state.nodes.push(payload.node);
-  if (payload.config != null) state.configs[payload.node.key] = payload.config;
-};
-
-export const handleRemoveNode = (
-  state: Schematic,
-  payload: RemoveNodePayload,
-): void => {
-  const idx = state.nodes.findIndex((n) => n.key === payload.key);
-  if (idx !== -1) state.nodes.splice(idx, 1);
-  delete state.configs[payload.key];
-};
-
-export const handleSetEdge = (state: Schematic, payload: SetEdgePayload): void => {
-  const idx = state.edges.findIndex((e) => e.key === payload.edge.key);
-  if (idx !== -1) state.edges[idx] = payload.edge;
-  else state.edges.push(payload.edge);
-};
-
-export const handleRemoveEdge = (
-  state: Schematic,
-  payload: RemoveEdgePayload,
-): void => {
-  const idx = state.edges.findIndex((e) => e.key === payload.key);
-  if (idx !== -1) state.edges.splice(idx, 1);
-};
-
-export const handleSetConfig = (state: Schematic, payload: SetConfigPayload): void => {
-  state.configs[payload.key] = payload.config;
-};
-
-export const handleSetAuthority = (
-  state: Schematic,
-  payload: SetAuthorityPayload,
-): void => {
-  state.authority = payload.value;
-};
-
-export const handleSetLegend = (state: Schematic, payload: SetLegendPayload): void => {
-  state.legend = payload.legend;
-};
+import { createReducer, createReduceAll, type Handlers } from "@/schematic/actions.gen";
+
+const handlers: Handlers = {
+  setNodePosition: (state, payload) => {
+    const node = state.nodes.find((n) => n.key === payload.key);
+    if (node != null) node.position = payload.position;
+  },
+  setNode: (state, payload) => {
+    const idx = state.nodes.findIndex((n) => n.key === payload.node.key);
+    if (idx !== -1) state.nodes[idx] = payload.node;
+    else state.nodes.push(payload.node);
+    if (payload.config != null) state.configs[payload.node.key] = payload.config;
+  },
+  removeNode: (state, payload) => {
+    const idx = state.nodes.findIndex((n) => n.key === payload.key);
+    if (idx !== -1) state.nodes.splice(idx, 1);
+    delete state.configs[payload.key];
+  },
+  setEdge: (state, payload) => {
+    const idx = state.edges.findIndex((e) => e.key === payload.edge.key);
+    if (idx !== -1) state.edges[idx] = payload.edge;
+    else state.edges.push(payload.edge);
+  },
+  removeEdge: (state, payload) => {
+    const idx = state.edges.findIndex((e) => e.key === payload.key);
+    if (idx !== -1) state.edges.splice(idx, 1);
+  },
+  setConfig: (state, payload) => {
+    state.configs[payload.key] = payload.config;
+  },
+  setAuthority: (state, payload) => {
+    state.authority = payload.value;
+  },
+  setLegend: (state, payload) => {
+    state.legend = payload.legend;
+  },
+};
+
+export const reduce = createReducer(handlers);
+export const reduceAll = createReduceAll(handlers);

client/ts/src/schematic/schematic.spec.ts

@@ -194,10 +194,10 @@ describe("Schematic", () => {
       expect(res.nodes[0].position).toEqual({ x: 100, y: 200 });
     });
 
-    test("addNode appends a node and writes its config", async () => {
+    test("setNode inserts a node and writes its config", async () => {
       const { schem } = await newWorkspaceSchematic(client);
       await client.schematics.dispatch(schem.key, "sess-1", [
-        schematic.addNode({
+        schematic.setNode({
           node: { key: "n1", position: { x: 1, y: 2 } },
           config: { label: "Pump" },
         }),
@@ -311,8 +311,8 @@ describe("Schematic", () => {
     test("applies a multi-action sequence atomically", async () => {
       const { schem } = await newWorkspaceSchematic(client);
       await client.schematics.dispatch(schem.key, "sess-1", [
-        schematic.addNode({ node: { key: "pump", position: { x: 0, y: 0 } } }),
-        schematic.addNode({ node: { key: "valve", position: { x: 100, y: 0 } } }),
+        schematic.setNode({ node: { key: "pump", position: { x: 0, y: 0 } } }),
+        schematic.setNode({ node: { key: "valve", position: { x: 100, y: 0 } } }),
         schematic.setEdge({
           edge: {
             key: "e1",

core/pkg/api/schematic/schematic_test.go

@@ -84,10 +84,10 @@ var _ = Describe("api.Service.Dispatch", func() {
 				Key:        s.Key,
 				SessionKey: "sess-1",
 				Actions: []schematic.Action{
-					schematic.NewAddNodeAction(schematic.AddNode{
+					schematic.NewSetNodeAction(schematic.SetNode{
 						Node: schematic.Node{Key: "n1", Position: spatial.XY{X: 1, Y: 2}},
 					}),
-					schematic.NewAddNodeAction(schematic.AddNode{
+					schematic.NewSetNodeAction(schematic.SetNode{
 						Node: schematic.Node{Key: "n2", Position: spatial.XY{X: 3, Y: 4}},
 					}),
 					schematic.NewSetEdgeAction(schematic.SetEdge{Edge: schematic.Edge{