feat(js): add graph change callback and set tags api - #230
Conversation
📝 WalkthroughWalkthroughThe PR adds tag-filtered graph subscriptions and exposes them to JavaScript runtime objects. It wires graph notifications through ChangesGraph runtime and JavaScript APIs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JavaScript
participant JSObject
participant PatchRuntime
participant GraphObserver
participant MessageNode
JavaScript->>JSObject: subscribe with onGraphChange
JSObject->>PatchRuntime: subscribeGraph(query, callback)
PatchRuntime->>GraphObserver: register subscription
PatchRuntime->>GraphObserver: notify after graph mutation
GraphObserver->>JSObject: deliver matching snapshot
JSObject->>MessageNode: send callback data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Deploying patchies with
|
| Latest commit: |
4b80bee
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ed5eec76.patchies.pages.dev |
| Branch Preview URL: | https://graph-change-callbacks-and-o.patchies.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
ui/src/lib/runtime/services/GraphObserver.ts (1)
67-90: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the graph reference to avoid a redundant call.
getSnapshotcallsthis.getGraph()twice, once at Line 68 and once at Line 74.getSnapshotruns once per subscription on every notify cycle, so caching the result in a local variable avoids doing the same work twice per call.♻️ Proposed refactor to cache the graph reference
private getSnapshot(query: GraphChangeQuery): GraphSnapshot { - const nodes = this.getGraph().objects.flatMap((node) => { + const graph = this.getGraph(); + const nodes = graph.objects.flatMap((node) => { const tags = getUserTags(node.data.tags); return nodeMatchesTags(tags, query.tags) ? [{ ...node, tags }] : []; }); const matchingNodeIds = new Set(nodes.map((node) => node.id)); - const edges = (this.getGraph().connections ?? []).flatMap((edge) => + const edges = (graph.connections ?? []).flatMap((edge) =>🤖 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 `@ui/src/lib/runtime/services/GraphObserver.ts` around lines 67 - 90, Update getSnapshot to store this.getGraph() in a local graph variable once, then use that variable for both objects and connections instead of calling getGraph() twice.ui/src/objects/canvas/CanvasDom.svelte (1)
404-406: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
setTagsfully replacesdata.tags; verify this preserves derived core tags.All three sites implement
setTagsthe same way: they overwritedata.tagswith onlygetUserTags(tags), the newly supplied list. The design doc statessetTags"does not change its derived core tags," which impliesdata.tagscan hold both user tags and reservedcore/-prefixed tags together (confirmed byGraphObserver.getSnapshot, which readsgetUserTags(node.data.tags)to strip reserved tags out of that same field). None of these three sites preserve anycore/-prefixed tag that might already be present indata.tagsbefore the overwrite.No code in this batch currently writes a
core/-prefixed tag intodata.tags, so there is no observable effect today. Confirm whether any current or planned mechanism ever writes reserved tags intodata.tags. If it does, or will, merge with the existing reserved tags instead of replacing the whole array.
ui/src/objects/canvas/CanvasDom.svelte#L404-L406: merge existingcore/-prefixed entries fromdata.tagswithgetUserTags(tags)instead of replacing the array outright.ui/src/objects/dom/DomRuntimeNode.svelte#L267-L269: apply the same merge fix.ui/src/objects/three/ThreeDom.svelte#L396-L398: apply the same merge fix.🛡️ Proposed fix pattern (apply at each site)
- setTags(tags: string[]) { - updateNodeData(nodeId, { tags: getUserTags(tags) }); - }, + setTags(tags: string[]) { + const coreTags = (data.tags ?? []).filter((tag) => tag.startsWith('core/')); + updateNodeData(nodeId, { tags: [...coreTags, ...getUserTags(tags)] }); + },🤖 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 `@ui/src/objects/canvas/CanvasDom.svelte` around lines 404 - 406, Update setTags in ui/src/objects/canvas/CanvasDom.svelte#L404-L406, ui/src/objects/dom/DomRuntimeNode.svelte#L267-L269, and ui/src/objects/three/ThreeDom.svelte#L396-L398 to preserve existing core/-prefixed entries from data.tags while replacing the user-tag portion with getUserTags(tags). Apply the same merge behavior at all three sites.ui/src/lib/runtime/services/GraphObserver.test.ts (1)
7-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAwait the queued microtask before asserting on
notify().
notify()defers its work withqueueMicrotask. These three tests callobserver.notify()and assert immediately, with noawaitanywhere in the test body. Because the test function body has noawait, it runs synchronously to completion, includingexpect(...), before the queued microtask executes.As a result, each assertion only reflects the state set by
subscribe()'s own synchronous immediate notification, not by thenotify()call each test is meant to exercise. The expected values happen to match whatsubscribe()alone produces, so the tests pass without validatingnotify()'s deferred behavior at all. A regression that makes the deferred microtask push an incorrect snapshot would not be caught by any of these three tests.Await a resolved promise after
observer.notify()so the queued microtask flushes before the assertion runs.✅ Proposed fix to flush the queued microtask before asserting
observer.subscribe({ tags: ['shader/foo/*'] }, (snapshot) => snapshots.push(snapshot)); observer.notify(); + await Promise.resolve(); expect(snapshots).toEqual([]);Apply the same
await Promise.resolve();after eachobserver.notify()call in the other two affected tests.Also applies to: 21-38, 40-71
🤖 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 `@ui/src/lib/runtime/services/GraphObserver.test.ts` around lines 7 - 19, Update all three affected tests around GraphObserver.notify to await a resolved promise immediately after each observer.notify() call, before asserting snapshots. This flushes the queued microtask and ensures the assertions validate notify’s deferred behavior rather than only subscribe’s synchronous notification.ui/src/lib/runtime/services/PatchRuntime.ts (1)
299-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
syncMessageConnectionsinsidesyncConnections.Both methods map the graph connections to editor edges and call
this.message.updateEdges(...). CallsyncMessageConnections()fromsyncConnections()to keep one mapping site.♻️ Proposed refactor
private syncConnections(): void { - const edges = this.graph.getConnections().map(getEditorEdgeFromRuntimeConnection); + const edges = this.graph.getConnections().map(getEditorEdgeFromRuntimeConnection); @@ - this.message.updateEdges(edges); + this.syncMessageConnections(edges); this.audio.audioService.updateEdges(edges); @@ - private syncMessageConnections(): void { - this.message.updateEdges(this.graph.getConnections().map(getEditorEdgeFromRuntimeConnection)); + private syncMessageConnections( + edges = this.graph.getConnections().map(getEditorEdgeFromRuntimeConnection) + ): void { + this.message.updateEdges(edges); }As per coding guidelines: "Extract shared functions instead of duplicating the same logic across multiple places".
🤖 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 `@ui/src/lib/runtime/services/PatchRuntime.ts` around lines 299 - 324, Update syncConnections() to call the existing syncMessageConnections() method instead of independently mapping graph connections and invoking this.message.updateEdges. Keep the remaining system edge updates unchanged and retain syncMessageConnections() as the single message-connection mapping site.Source: Coding guidelines
ui/src/lib/runtime/utils/runtime-test-utils.ts (1)
152-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
getObjectClassfor class selection.The nested ternary repeats the mapping in
getObjectClass. Each new test object type must be added in three places. Derive the class fromgetObjectClassinstead.♻️ Proposed refactor
- const ObjectClass: TextObjectClass = - objectType === ButtonObject.type - ? ButtonObject - : objectType === JSObject.type - ? JSObject - : PatchRuntimeTestObject; + const ObjectClass: TextObjectClass = this.getObjectClass(objectType) ?? PatchRuntimeTestObject;As per coding guidelines: "Extract shared functions instead of duplicating the same logic across multiple places".
🤖 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 `@ui/src/lib/runtime/utils/runtime-test-utils.ts` around lines 152 - 159, Replace the nested ternary in the class-selection logic with a call to the existing getObjectClass method, preserving the current objectType mapping and ObjectClass typing while ensuring future test object types use the shared mapping.Source: Coding guidelines
ui/src/lib/runtime/services/OnGraphChange.integration.test.ts (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnregister the compiler node in
afterEach.
MessageSystem.getInstance()is a shared singleton across test files. The runtime registerscompilerIdwhen it creates thejsobject.afterEachunregisters onlyglslId, so a staleshader-compilerregistration can remain and affect later tests.♻️ Proposed change
afterEach(() => { + messageSystem.unregisterNode(compilerId); messageSystem.unregisterNode(glslId); messageSystem.updateEdges([]); });🤖 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 `@ui/src/lib/runtime/services/OnGraphChange.integration.test.ts` around lines 12 - 15, Update the afterEach cleanup to also unregister the compilerId node registered by the js object, while retaining the existing glslId unregister and edge reset through the shared messageSystem singleton.
🤖 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 `@ui/src/lib/runtime/services/GraphObserver.ts`:
- Around line 40-61: Update notifySubscription to wrap
subscription.callback(snapshot) in a try/catch, ensuring callback failures are
isolated and do not interrupt notification of other subscriptions. Apply this in
notifySubscription so both subscribe and notify retain their existing behavior
while continuing to process unrelated subscriptions.
In `@ui/src/objects/code/CodeBlockBase.svelte`:
- Line 52: Update the default data-triggered execution handler in CodeBlockBase
so it uses the local executeCode() flow instead of onExecute, preserving
clearConsole() and the lineErrors/hasError resets for consumers that do not
provide an override. Ensure the data-triggered call site around the affected
execution path continues to invoke that default behavior.
In `@ui/src/objects/js/JSBlockNode.svelte`:
- Around line 52-54: Update setSetting in JSBlockNode to route node setting
changes through the SettingsManager from extraContext.settings by calling
settingsManager.setValue instead of directly merging data.settings. For kv
fields, also invoke sendSettingsValueChanged so persisted values and change
notifications remain synchronized.
---
Nitpick comments:
In `@ui/src/lib/runtime/services/GraphObserver.test.ts`:
- Around line 7-19: Update all three affected tests around GraphObserver.notify
to await a resolved promise immediately after each observer.notify() call,
before asserting snapshots. This flushes the queued microtask and ensures the
assertions validate notify’s deferred behavior rather than only subscribe’s
synchronous notification.
In `@ui/src/lib/runtime/services/GraphObserver.ts`:
- Around line 67-90: Update getSnapshot to store this.getGraph() in a local
graph variable once, then use that variable for both objects and connections
instead of calling getGraph() twice.
In `@ui/src/lib/runtime/services/OnGraphChange.integration.test.ts`:
- Around line 12-15: Update the afterEach cleanup to also unregister the
compilerId node registered by the js object, while retaining the existing glslId
unregister and edge reset through the shared messageSystem singleton.
In `@ui/src/lib/runtime/services/PatchRuntime.ts`:
- Around line 299-324: Update syncConnections() to call the existing
syncMessageConnections() method instead of independently mapping graph
connections and invoking this.message.updateEdges. Keep the remaining system
edge updates unchanged and retain syncMessageConnections() as the single
message-connection mapping site.
In `@ui/src/lib/runtime/utils/runtime-test-utils.ts`:
- Around line 152-159: Replace the nested ternary in the class-selection logic
with a call to the existing getObjectClass method, preserving the current
objectType mapping and ObjectClass typing while ensuring future test object
types use the shared mapping.
In `@ui/src/objects/canvas/CanvasDom.svelte`:
- Around line 404-406: Update setTags in
ui/src/objects/canvas/CanvasDom.svelte#L404-L406,
ui/src/objects/dom/DomRuntimeNode.svelte#L267-L269, and
ui/src/objects/three/ThreeDom.svelte#L396-L398 to preserve existing
core/-prefixed entries from data.tags while replacing the user-tag portion with
getUserTags(tags). Apply the same merge behavior at all three sites.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55956b99-62a9-451c-9be1-000647b8f61b
⛔ Files ignored due to path filters (1)
ui/src/lib/generated/object-schemas.generated.tsis excluded by!**/*.generated.*,!**/generated/**
📒 Files selected for processing (23)
docs/design-docs/specs/171-graph-systems-and-composable-code-representations.mdui/src/lib/codemirror/patchies-completions.tsui/src/lib/js-runner/JSRunner.test.tsui/src/lib/js-runner/JSRunner.tsui/src/lib/objects/v2/ObjectContext.tsui/src/lib/objects/v2/ObjectService.tsui/src/lib/objects/v2/nodes/index.tsui/src/lib/runtime/adapters/MessageAdapter.tsui/src/lib/runtime/services/GraphObserver.test.tsui/src/lib/runtime/services/GraphObserver.tsui/src/lib/runtime/services/GraphSubscription.test.tsui/src/lib/runtime/services/OnGraphChange.integration.test.tsui/src/lib/runtime/services/PatchRuntime.tsui/src/lib/runtime/services/graph-tags.test.tsui/src/lib/runtime/services/graph-tags.tsui/src/lib/runtime/utils/runtime-test-utils.tsui/src/objects/canvas/CanvasDom.svelteui/src/objects/code/CodeBlockBase.svelteui/src/objects/dom/DomRuntimeNode.svelteui/src/objects/js/JSBlockNode.svelteui/src/objects/js/JSObject.test.tsui/src/objects/js/JSObject.tsui/src/objects/three/ThreeDom.svelte
| subscribe(query: GraphChangeQuery, callback: GraphChangeCallback): () => void { | ||
| const subscription = { query, callback }; | ||
|
|
||
| this.subscriptions.add(subscription); | ||
| this.notifySubscription(subscription); | ||
|
|
||
| return () => this.subscriptions.delete(subscription); | ||
| } | ||
|
|
||
| notify(): void { | ||
| if (this.notificationQueued) return; | ||
|
|
||
| this.notificationQueued = true; | ||
|
|
||
| queueMicrotask(() => { | ||
| this.notificationQueued = false; | ||
|
|
||
| for (const subscription of this.subscriptions) { | ||
| this.notifySubscription(subscription); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Isolate subscription callbacks from each other.
notifySubscription calls subscription.callback(snapshot) at Line 105 without a try/catch. GraphObserver is a single shared instance for the whole patch (see PatchRuntime.ts: private graphObserver = new GraphObserver(...)). If one subscription's callback throws, the for loop in notify() (Line 57-59) stops. Other, unrelated subscriptions do not receive their pending notification in that batch. The same unguarded call happens in subscribe() at Line 44.
The design doc states each subscription must reconcile independently and a failing callback must not block other graph systems. Add a try/catch around the callback invocation in notifySubscription so this applies to both call sites.
🔒 Proposed fix to isolate subscription callback failures
subscription.lastSnapshotKey = snapshotKey;
- subscription.callback(snapshot);
+
+ try {
+ subscription.callback(snapshot);
+ } catch (error) {
+ console.error('GraphObserver subscription callback failed', error);
+ }
}Also applies to: 92-106
🤖 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 `@ui/src/lib/runtime/services/GraphObserver.ts` around lines 40 - 61, Update
notifySubscription to wrap subscription.callback(snapshot) in a try/catch,
ensuring callback failures are isolated and do not interrupt notification of
other subscriptions. Apply this in notifySubscription so both subscribe and
notify retain their existing behavior while continuing to process unrelated
subscriptions.
| function setSetting(key: string, value: unknown) { | ||
| updateNodeData(nodeId, { settings: { ...data.settings, [key]: value } }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SettingsManager persistence behavior and other setting-change call sites.
fd -t f -g 'SettingsManager*' --exec rg -n -C6 'persist|kv|set\(' {}
rg -n -C4 'onSettingsValueChange' --type=svelteRepository: heypoom/patchies
Length of output: 6563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate candidate files =="
fd -t f 'JSBlockNode.svelte|JSObject|SettingsManager|updateNodeData|createKVStore|createSettingsAPI' .
echo
echo "== JSBlockNode outline =="
ast-grep outline ui/src/objects/js/JSBlockNode.svelte --view expanded || true
echo
echo "== JSBlockNode relevant content =="
cat -n ui/src/objects/js/JSBlockNode.svelte | sed -n '1,180p'
echo
echo "== SettingsManager full file =="
fd -t f 'SettingsManager.*' -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}
echo
echo "== search settingsAPI and createKVStore/createKVStore usages =="
rg -n -C4 'createSetting|createSettings|createKVStore|getSettings|setSetting|SettingsManager|data.settings|updateNodeData' --glob '*.ts' --glob '*.svelte' .Repository: heypoom/patchies
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JSBlockNode around setSetting and JSObject usage =="
sed -n '1,130p' ui/src/objects/js/JSBlockNode.svelte | cat -n
echo
echo "== JSBlockNode calls to JSObject settings events =="
rg -n -C3 'JSObject|onSettingsValueChange|onSettingsRevertAll|settingsValues|settingsSchema|setSetting|updateNodeData' ui/src/objects/js/JSBlockNode.svelte ui/src/objects/js -g '*.svelte' -g '*.ts'
echo
echo "== JSObject implementation/files =="
fd -t f 'JSObject.*' ui/src/objects/js -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,260p" "$1" | cat -n' sh {}
echo
echo "== SettingsManager full implementation/files =="
fd -t f 'SettingsManager.*' ui/src -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "1,260p" "$1" | cat -n' sh {}Repository: heypoom/patchies
Length of output: 35983
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings schema/API files =="
fd -t f 'settings.*|create-settings-api.*|types.*' ui/src/lib/settings
echo
for f in $(fd -t f . ui/src/lib/settings); do
echo "--- $f"; wc -l "$f"; cat -n "$f";
done
echo
echo "== JSBlockNode imports/props/usages relevant to settings =="
rg -n -C4 'import .*JSObject|JSObject|settingsSchema|settingsValues|onSettingsValueChange|onSettingsRevertAll|setSetting|execut' ui/src/objects/js/JSBlockNode.svelte ui/src -g '*.svelte' -g '*.ts' --max-count 200Repository: heypoom/patchies
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings lib files only =="
for f in $(find ui/src/lib/settings -maxdepth 1 -type f); do
echo "--- $f"; wc -l "$f"; cat -n "$f";
done
echo
echo "== CodeBlockBase settings events and JSObject registration/ref =="
rg -n -C5 'CodeBlockBase|ObjectSettings|JSObject|JSObjectNode|createSettingsAPI|sendSettingsValueChanged|sendSettingsValue' ui/src \
--glob '*.svelte' --glob '*.ts' --glob '*.tsx' --max-count 250
echo
echo "== settings persistence fields =="
rg -n -C3 'settingsSchema|settings:|persistence:|type: .*settings|onSettingsValueChange|settingsValues' ui/src/lib/ui ui/src/components ui/src/objects js -g '*.svelte' -g '*.ts' --max-count 250Repository: heypoom/patchies
Length of output: 50373
Route JS view setting changes through SettingsManager.
setSetting updates data.settings, but JSObject only uses its SettingsManager from extraContext.settings; the view is not connected as a JSObject setting callback. Use settingsManager.setValue(...) for node settings updates and also fire sendSettingsValueChanged(...) for kv fields, so persisted values and setting change notifications stay in sync.
🤖 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 `@ui/src/objects/js/JSBlockNode.svelte` around lines 52 - 54, Update setSetting
in JSBlockNode to route node setting changes through the SettingsManager from
extraContext.settings by calling settingsManager.setValue instead of directly
merging data.settings. For kv fields, also invoke sendSettingsValueChanged so
persisted values and change notifications remain synchronized.
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 `@ui/src/lib/runtime/services/GraphObserver.test.ts`:
- Around line 78-98: Update the GraphObserver callback-failure test to either
assert logger.warn was called with the expected message and thrown error, or
remove the warn spy and related cleanup if logging is not part of the contract;
preserve the existing assertion that later subscriptions still receive the
snapshot.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c4e632a-7311-4dc5-9574-0a791c6ccd43
📒 Files selected for processing (10)
ui/src/lib/js-runner/JSRunner.tsui/src/lib/runtime/index.tsui/src/lib/runtime/services/GraphObserver.test.tsui/src/lib/runtime/services/GraphObserver.tsui/src/lib/runtime/services/OnGraphChange.integration.test.tsui/src/lib/runtime/services/PatchRuntime.tsui/src/lib/runtime/services/graph-tags.tsui/src/lib/runtime/utils/runtime-test-utils.tsui/src/objects/code/CodeBlockBase.svelteui/src/objects/js/JSObject.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/src/lib/runtime/services/graph-tags.ts
- ui/src/lib/runtime/services/OnGraphChange.integration.test.ts
- ui/src/lib/runtime/utils/runtime-test-utils.ts
- ui/src/objects/js/JSObject.test.ts
- ui/src/lib/runtime/services/GraphObserver.ts
- ui/src/lib/js-runner/JSRunner.ts
- ui/src/lib/runtime/services/PatchRuntime.ts
| it('continues notifying subscriptions when a callback fails', () => { | ||
| const observer = new GraphObserver(() => ({ | ||
| objects: [{ id: 'fragment', type: 'js', data: { tags: ['shader/foo/function'] } }], | ||
| connections: [] | ||
| })); | ||
| const warn = vi.spyOn(logger, 'warn').mockImplementation(() => {}); | ||
| const snapshots: string[][] = []; | ||
|
|
||
| try { | ||
| observer.subscribe({ tags: ['shader/foo/*'] }, () => { | ||
| throw new Error('broken callback'); | ||
| }); | ||
| observer.subscribe({ tags: ['shader/foo/*'] }, ({ nodes }) => | ||
| snapshots.push(nodes.map(({ id }) => id)) | ||
| ); | ||
|
|
||
| expect(snapshots).toEqual([['fragment']]); | ||
| } finally { | ||
| warn.mockRestore(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'GraphObserver\.(test\.)?ts|logger' || true
echo
echo "Test outline:"
ast-grep outline ui/src/lib/runtime/services/GraphObserver.test.ts || true
echo
echo "Test relevant lines:"
cat -n ui/src/lib/runtime/services/GraphObserver.test.ts | sed -n '1,140p'
echo
echo "Source outline:"
ast-grep outline ui/src/lib/runtime/services/GraphObserver.ts || true
echo
echo "Source relevant lines:"
cat -n ui/src/lib/runtime/services/GraphObserver.ts | sed -n '1,220p'Repository: heypoom/patchies
Length of output: 10284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Other logger.warn tests in GraphObserver files:"
rg -n "logger|warn|onGraphChange" ui/src/lib/runtime/services/GraphObserver* || trueRepository: heypoom/patchies
Length of output: 705
Assert the warning call or remove the spy.
GraphObserver logs callback failures with logger.warn('Error in onGraphChange() handler:', error), but the test only asserts that the second subscription still runs. Check the expected warning message plus an error argument, or remove the logger.warn spy if logging is not part of the caller contract.
🤖 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 `@ui/src/lib/runtime/services/GraphObserver.test.ts` around lines 78 - 98,
Update the GraphObserver callback-failure test to either assert logger.warn was
called with the expected message and thrown error, or remove the warn spy and
related cleanup if logging is not part of the contract; preserve the existing
assertion that later subscriptions still receive the snapshot.
1 participant
Adds the
onGraphChange({ tags }, callback)API for listening to Patchies graph changes, and asetTagsAPI for setting tags onto objects.Summary by CodeRabbit
setTags.onGraphChangesubscriptions for matching graph nodes and connections.