feat(frontend): show node output reference hint in config modal - #23
feat(frontend): show node output reference hint in config modal#23LukasHirt wants to merge 2 commits into
Conversation
de4c87c to
bcc04fc
Compare
Action nodes (tag/comment/move/copy/rename/notify) already recorded their
result on the NodeResult execution record, but never wrote it into the
shared vars map that {{...}} template rendering reads from — unlike LLM
nodes, whose output is wired in via vars["llm.output"]. That meant an
action's result was, in practice, never referenceable from a downstream
node's template, even though nothing in the code or UI signaled that.
Mirror the llm.output pattern by writing each action's result into vars
under "<actionType>.output" (tag.output, comment.output, move.output,
copy.output, rename.output, notify.output) right where result.Output is
already set. This makes the upcoming frontend output hint truthful
instead of aspirational.
Signed-off-by: Lukas Hirt <info@hirt.cz>
Users configuring a node had no reliable way to learn the exact {{...}}
syntax needed to reference its output from a downstream node — only a
couple of fields carried it as scattered placeholder text.
Add a persistent "Output" section to NodeDetailsPanel.vue, shown for
every node type, that states the verified-correct template token(s):
- llm node: {{llm.output}}
- action nodes: {{tag.output}}, {{comment.output}}, {{move.output}},
{{copy.output}}, {{rename.output}}, {{notify.output}} — matching the
vars keys the executor now writes (see the paired backend commit)
- trigger node: {{file.name}} / {{file.content}}, available whenever
the run targets a specific file
The WorkflowNodeData.outputVariable field was checked and confirmed to
have no effect on runtime behavior (backend/pkg/executor/executor.go
hardcodes the "llm.output" key regardless of it), so the LLM hint
intentionally does not offer a "customized" variant of the token.
Adds a Vitest component-mount test for NodeDetailsPanel.vue (the first
of its kind in this package) covering LLM, action, and trigger nodes.
Signed-off-by: Lukas Hirt <info@hirt.cz>
bcc04fc to
91931a1
Compare
dj4oC
left a comment
There was a problem hiding this comment.
Review: feat(frontend): show node output reference hint in config modal
Stats: +221/-0 across 4 files (backend + frontend)
Overview
Adds a persistent "Output" section to NodeDetailsPanel showing the exact {{...}} token(s) a node's result is referenceable by downstream. To make the hint true rather than aspirational, the backend fix comes first: runAction() in executor.go previously set result.Output on the NodeResult for every action type but never wrote anything into the shared vars map, so {{tag.output}}, {{move.output}}, etc. silently did not work despite looking plausible. This PR closes that gap for all six action types, mirroring the existing llm.output convention, and only then documents the (now-true) tokens in the UI.
I independently verified the two central claims against main:
runAction()onmaindoes indeed setresult.Outputfor every case but never touchesvars— confirmed by reading the full function.- The pre-existing
TestRunTriggerLLMActiondoes verify{{llm.output}}substitution end-to-end (fGraph.taggedWith == "summary:a short summary"), so that half of the "ground truth" investigation checks out too.
Both hold up — this PR's premise is solid, not just asserted.
Code quality & style
- The backend change is minimal and mechanical (one
vars[...] = ...line per case), which is the right size for what's actually a bug fix wearing a "docs" PR's clothes. - Keying
move/copydynamically asvars[actionType+".output"](rather than a sharedaction.output) is a good call — called out directly in a comment — since it lets a graph with both acopyand amovenode address each independently. frontend/src/components/NodeDetailsPanel.vue's inline comment enumerates every vars key the executor actually writes, with a direct instruction that nothing be listed here without a matching backend write. That's a good guardrail against the hint drifting out of sync with reality again.
Suggestions
actionOutputHintsis typedRecord<string, OutputHint>rather thanRecord<ActionType, OutputHint>. All six currentActionTypevalues are covered today, but the looserstringkey means TypeScript won't force an update here if a new action type is ever added — it'll silently fall through to?? null(no hint shown) rather than a compile error. Tightening the type would make this self-enforcing the same way the rest of the PR tries to keep the hint accurate.- The trigger-node hint doesn't vary by
triggerType. It always shows{{file.name}}/{{file.content}}for any trigger node, including a Schedule Trigger — which, per the executor (and the investigation in the sibling PR #22, currently also open), can never actually populate those vars, since the scheduler always callsRunwith an emptyresourcePath. The current wording ("e.g. a file-event trigger, or a manual run targeting a file") doesn't claim schedule triggers qualify, so nothing here is technically false — but given how rigorously this PR verifies everything else against ground truth, explicitly branching the message fortriggerType === 'schedule'(e.g., "not available — this trigger never supplies a file") would close the one case where the hint could mislead by omission. - Minor duplication in the test file: a new
mountOutputHintPanelhelper is added alongside the pre-existingmountPanelhelper inNodeDetailsPanel.spec.ts, with slightly different setup (addsstubsforoc-icon/oc-button/oc-text-input, which the original helper doesn't use). Consider extending the existing helper instead of maintaining two similar-but-different mount configs in the same file. - PR-description nit: the description states this is "the first component-mount test in this package" —
NodeDetailsPanel.spec.tsalready hadmount()-based tests using a realcreateGettext()plugin onmainprior to this PR. Doesn't affect the code, just flagging since the PR leans heavily on precise claims elsewhere.
Cross-PR note (FYI, not blocking)
Both this PR and the currently-open PR #22 modify NodeDetailsPanel.vue and NodeDetailsPanel.spec.ts in overlapping regions (new template blocks and new computeds inserted right around the same spot). Not a problem for either PR individually, but whichever merges second will need a straightforward manual conflict resolution.
Test coverage
Good, and appropriately TDD'd per the PR description (new test added first, confirmed to fail for the right reason, then fixed). TestActionOutputIsReferenceableDownstream chains a real tag → comment graph and asserts the rendered output through the fake Comment call — a meaningful end-to-end check, not just a unit check of the vars map. Frontend tests cover LLM, three action types (tag/move/notify), and a trigger node; comment/copy/rename aren't separately asserted in the component test, though they share the same code path as tag/move, so risk is low.
Security / performance
None — additive vars map entries only, no new I/O, no new inputs from untrusted sources.
Summary
Small, well-verified, low-risk PR that fixes a real latent bug (action outputs were never actually referenceable) before documenting the fix. Suggestions above are polish, not blockers.
🤖 Generated with Claude Code
Summary
Users configuring any node in the workflow canvas had no reliable way to learn the exact
{{...}}template syntax needed to reference that node's output from a downstream node. The only hints were scattered placeholder strings on a couple of specific fields (LLM prompt/comment/message), not a consistent, always-visible piece of UI, and their accuracy had never been verified against the executor.Investigation (executor.go ground truth)
Read
backend/pkg/executor/executor.goin full. Findings:runLLM()writesvars["llm.output"] = output(line 172) and setsresult.Outputon theNodeResult. So{{llm.output}}genuinely works today. Confirmed by the pre-existingTestRunTriggerLLMActiontest, which already asserts a downstream action node receives the rendered{{llm.output}}.runAction()setresult.Outputon the execution-result record, but none of them wrote anything into the sharedvarsmap. I proved this with a new failing test (TestActionOutputIsReferenceableDownstream, added first per TDD) that chains atagaction into acommentaction referencing{{tag.output}}— before the fix, the comment literally contained the unexpanded string{{tag.output}}. So action outputs were not referenceable at all prior to this PR, despiteresult.Outputexisting — a real trap for anyone assuming otherwise.WorkflowNodeData.outputVariable(frontend/src/types/workflow.ts): grepped the whole repo — it's declared on the type but never read anywhere, frontend or backend.runLLM()unconditionally uses the literal key"llm.output"regardless of this field. It currently has zero effect on runtime behavior, so the new hint intentionally does not offer a "customized variable name" variant — that would have been fictional.file.name/file.contentare populated intovarsonce per run whenever aresourcePathis supplied (line 66-76), independent of trigger type. This is meaningfully "the trigger's output" and is surfaced the same way.Changes
Backend (
backend/pkg/executor/executor.go) — small, scoped fix so the new UI hint is true rather than aspirational: each action case now also writes its result intovarsunder"<actionType>.output"(tag.output,comment.output,move.output,copy.output,rename.output,notify.output), mirroring the existingllm.outputconvention (a single global key per node-kind, not per node-id — same characteristic the existingllm.outputkey already has). AddedTestActionOutputIsReferenceableDownstreamcovering this end-to-end through a chained tag → comment workflow.Frontend (
frontend/src/components/NodeDetailsPanel.vue) — added a persistent "Output" section, shown for every node type, stating only verified-correct tokens:{{llm.output}}{{tag.output}}, comment:{{comment.output}}, move:{{move.output}}, copy:{{copy.output}}, rename:{{rename.output}}, notify:{{notify.output}}{{file.name}},{{file.content}}Added
frontend/tests/unit/NodeDetailsPanel.spec.ts— the first component-mount test in this package, using@vue/test-utils+createGettext()(real gettext plugin, sinceuseGettext()throws without it) withoc-icon/oc-button/oc-text-inputstubbed (host-shell design-system components unavailable in unit tests). Covers LLM, tag/move/notify action nodes, and a trigger node.Both new tests were written first, confirmed to fail for the right reason (fictional/missing wiring), then made to pass.
Test plan
cd backend && go test ./...— all packages pass, including newTestActionOutputIsReferenceableDownstreamcd backend && go vet ./...— cleancd frontend && npm run test:unit— 3 files / 9 tests passcd frontend && npm run check:types— cleancd frontend && npm run lint— cleanScope note: did not touch node categorization, the "+" button, node positioning, the OK button, or auto-open-on-add behavior — kept strictly to the output-hint concern.