Fully Functional Recursive Subagent Workflows - #18
Open
Jacobcdsmith wants to merge 1 commit into
Open
Conversation
…d dynamic inspector select dropdown
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
📝 WalkthroughWalkthroughThe PR adds nested workflow selection, recursive subagent execution, nested run-log propagation, recursive log rendering, and Python and JavaScript metadata comments. ChangesNested subagent workflows
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ParentRun as Parent runFlow
participant Subagent as Subagent node
participant Lookup as Workflow lookup
participant ChildRun as Child workflow
ParentRun->>Subagent: Execute node
Subagent->>Lookup: Resolve workflow ID
Lookup-->>Subagent: Return workflow definition
Subagent->>ChildRun: Run with derived child state
ChildRun-->>Subagent: Return output and nested logs
Subagent-->>ParentRun: Store output and nested logs
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
frontend/src/flow/runFlow.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
findWorkflowByIdacross two files. Both files define the identical helper ([...TEMPLATES, ...loadWorkflows()].find((w) => w.id === id)), risking future divergence if workflow lookup logic changes (caching, ID-collision handling, etc.).
frontend/src/flow/runFlow.ts#L6-L6,21-24: remove the localfindWorkflowByIdand theTEMPLATES/loadWorkflowsimport used only for it; import a sharedfindWorkflowByIdfromfrontend/src/flow/workflows.tsinstead.frontend/src/flow/codegen.ts#L4-L9: remove the localfindWorkflowByIdand itsTEMPLATES/loadWorkflowsimport; import the same shared helper fromfrontend/src/flow/workflows.ts.🤖 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 `@frontend/src/flow/runFlow.ts` at line 6, The workflow lookup helper is duplicated in both flow modules. In frontend/src/flow/runFlow.ts (anchor, line 6), remove the local findWorkflowById and its TEMPLATES/loadWorkflows-only import, then import the shared helper from workflows.ts; apply the same removal and shared import in frontend/src/flow/codegen.ts (sibling, lines 4-9), preserving all existing call sites.
🤖 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 `@frontend/src/flow/Inspector.tsx`:
- Around line 57-76: Update the subagent graph select in Inspector so a stored
graph ID absent from subagentWorkflows still has a visible fallback option
labeled with that ID. Reuse the current node.data.config?.graph value, add the
fallback only when it is non-empty and does not match any workflow entry, and
preserve the existing workflow options and selection behavior.
In `@frontend/src/flow/runFlow.ts`:
- Around line 183-195: Propagate failures from nested workflow results by
detecting the first errored entry in subLogs and assigning its message to the
parent error when unwrapping __isSubagentResult. Apply this consistently at
frontend/src/flow/runFlow.ts:183-195, frontend/src/pages/Index.tsx:744-800, and
frontend/src/pages/Index.tsx:952-1008; preferably centralize the behavior in a
shared unwrapNodeResult helper and reuse it at all three sites before
constructing parent logs.
- Around line 329-384: Add a recursion guard to the nested workflow execution
around runNode’s targetWf resolution and recursive runFlow call by threading a
depth counter or visited workflow-ID set through RunOptions. Before invoking
runFlow, reject cycles or excessive nesting with a clear error, and ensure
recursive calls propagate the guard state while preserving the existing maxSteps
behavior.
In `@frontend/src/pages/Index.tsx`:
- Around line 1992-2076: Update handleLogsExpandAll to use the same depth-aware
keys as renderLogItem, including the depth suffix for every state snapshot so
expandedLogSnapshots matches uniqueKey, including top-level entries. When
expanding all, also set the corresponding sublogs-${uniqueKey} entries and
recurse through nested subLogs so nested workflow logs and their snapshots are
revealed.
In `@frontend/src/test/subagentNode.test.tsx`:
- Around line 94-137: Update the test “should render dynamic dropdown for graph
selection and exclude active workflow” by adding a stored workflow with id
“parent-flow” alongside childWf1, then assert that its rendered name is absent.
Keep the existing assertions confirming the template and Child 1 remain present.
---
Nitpick comments:
In `@frontend/src/flow/runFlow.ts`:
- Line 6: The workflow lookup helper is duplicated in both flow modules. In
frontend/src/flow/runFlow.ts (anchor, line 6), remove the local findWorkflowById
and its TEMPLATES/loadWorkflows-only import, then import the shared helper from
workflows.ts; apply the same removal and shared import in
frontend/src/flow/codegen.ts (sibling, lines 4-9), preserving all existing call
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: f208b461-35f6-4321-9e89-e11b418bbe80
⛔ Files ignored due to path filters (1)
dev_server.logis excluded by!**/*.log
📒 Files selected for processing (5)
frontend/src/flow/Inspector.tsxfrontend/src/flow/codegen.tsfrontend/src/flow/runFlow.tsfrontend/src/pages/Index.tsxfrontend/src/test/subagentNode.test.tsx
Comment on lines
+57
to
+76
| {node.data.kind === "subagent" && f.key === "graph" ? ( | ||
| <select | ||
| value={node.data.config?.[f.key] ?? ""} | ||
| onChange={(e) => | ||
| onChange(node.id, { | ||
| config: { ...node.data.config, [f.key]: e.target.value }, | ||
| }) | ||
| } | ||
| className="mt-1 w-full bg-[hsl(var(--paper))] border border-dashed border-[hsl(var(--ink-faint))] focus:border-[hsl(var(--ink))] outline-none py-1.5 px-2 text-[hsl(var(--ink))]" | ||
| > | ||
| <option value="" disabled> | ||
| -- select nested workflow -- | ||
| </option> | ||
| {subagentWorkflows.map((w) => ( | ||
| <option key={w.id} value={w.id}> | ||
| {w.name} ({w.isTemplate ? "template" : "custom"}) | ||
| </option> | ||
| ))} | ||
| </select> | ||
| ) : f.type === "textarea" ? ( |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show a fallback option when the stored graph value is missing from the list.
If node.data.config?.graph holds an ID that is not in subagentWorkflows (workflow deleted, or the value equals the workflow the user just switched into), the <select> renders with no visible selection and no label. The user cannot tell which workflow is referenced or that it is missing.
Add an option for the current value when it does not match any entry in subagentWorkflows.
🛠️ Proposed fix
<option value="" disabled>
-- select nested workflow --
</option>
+ {node.data.config?.[f.key] &&
+ !subagentWorkflows.some((w) => w.id === node.data.config?.[f.key]) && (
+ <option value={node.data.config[f.key]} disabled>
+ ⚠ unknown workflow ({node.data.config[f.key]})
+ </option>
+ )}
{subagentWorkflows.map((w) => (
<option key={w.id} value={w.id}>
{w.name} ({w.isTemplate ? "template" : "custom"})
</option>
))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {node.data.kind === "subagent" && f.key === "graph" ? ( | |
| <select | |
| value={node.data.config?.[f.key] ?? ""} | |
| onChange={(e) => | |
| onChange(node.id, { | |
| config: { ...node.data.config, [f.key]: e.target.value }, | |
| }) | |
| } | |
| className="mt-1 w-full bg-[hsl(var(--paper))] border border-dashed border-[hsl(var(--ink-faint))] focus:border-[hsl(var(--ink))] outline-none py-1.5 px-2 text-[hsl(var(--ink))]" | |
| > | |
| <option value="" disabled> | |
| -- select nested workflow -- | |
| </option> | |
| {subagentWorkflows.map((w) => ( | |
| <option key={w.id} value={w.id}> | |
| {w.name} ({w.isTemplate ? "template" : "custom"}) | |
| </option> | |
| ))} | |
| </select> | |
| ) : f.type === "textarea" ? ( | |
| {node.data.kind === "subagent" && f.key === "graph" ? ( | |
| <select | |
| value={node.data.config?.[f.key] ?? ""} | |
| onChange={(e) => | |
| onChange(node.id, { | |
| config: { ...node.data.config, [f.key]: e.target.value }, | |
| }) | |
| } | |
| className="mt-1 w-full bg-[hsl(var(--paper))] border border-dashed border-[hsl(var(--ink-faint))] focus:border-[hsl(var(--ink))] outline-none py-1.5 px-2 text-[hsl(var(--ink))]" | |
| > | |
| <option value="" disabled> | |
| -- select nested workflow -- | |
| </option> | |
| {node.data.config?.[f.key] && | |
| !subagentWorkflows.some((w) => w.id === node.data.config?.[f.key]) && ( | |
| <option value={node.data.config[f.key]} disabled> | |
| ⚠ unknown workflow ({node.data.config[f.key]}) | |
| </option> | |
| )} | |
| {subagentWorkflows.map((w) => ( | |
| <option key={w.id} value={w.id}> | |
| {w.name} ({w.isTemplate ? "template" : "custom"}) | |
| </option> | |
| ))} | |
| </select> | |
| ) : f.type === "textarea" ? ( |
🤖 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 `@frontend/src/flow/Inspector.tsx` around lines 57 - 76, Update the subagent
graph select in Inspector so a stored graph ID absent from subagentWorkflows
still has a visible fallback option labeled with that ID. Reuse the current
node.data.config?.graph value, add the fallback only when it is non-empty and
does not match any workflow entry, and preserve the existing workflow options
and selection behavior.
Comment on lines
+183
to
+195
| let subLogs: RunLog[] | undefined; | ||
| state.__last_kind = current.data.kind; | ||
| delete state.__router_branch; | ||
|
|
||
| try { | ||
| output = await runNode(current, state, gateways, opts, globalsList, secretsList); | ||
| const nodeResult = await runNode(current, state, gateways, opts, globalsList, secretsList); | ||
| if (nodeResult && typeof nodeResult === "object" && "__isSubagentResult" in nodeResult) { | ||
| const subResult = nodeResult as { output: unknown; subLogs: RunLog[] }; | ||
| output = subResult.output; | ||
| subLogs = subResult.subLogs; | ||
| } else { | ||
| output = nodeResult; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Nested workflow failures are silently swallowed at three call sites. All three sites unwrap __isSubagentResult the same way and copy output/subLogs from the child run, but none inspects subLogs for a failed nested step. A failing nested workflow is recorded only inside its own subLogs entries; the parent step's error stays unset, so the UI reports success (toast.success(...)) for a run that actually failed underneath a subagent node.
frontend/src/flow/runFlow.ts#L183-L195: after unwrapping, seterrorwhensubLogs.find((l) => l.error)is truthy, e.g.error = \Nested workflow error: ${subLogs.find((l) => l.error)?.error}`;`.frontend/src/pages/Index.tsx#L744-L800: apply the same check instepForward's unwrap block before building the step'slog.frontend/src/pages/Index.tsx#L952-L1008: apply the same check inresumeStepper's unwrap block before building the step'slog.
Consider extracting one shared unwrapNodeResult(nodeResult) helper (returning { output, subLogs, error }) in runFlow.ts and importing it at all three sites, so the error-propagation logic is defined once instead of maintained in three copies.
📍 Affects 2 files
frontend/src/flow/runFlow.ts#L183-L195(this comment)frontend/src/pages/Index.tsx#L744-L800frontend/src/pages/Index.tsx#L952-L1008
🤖 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 `@frontend/src/flow/runFlow.ts` around lines 183 - 195, Propagate failures from
nested workflow results by detecting the first errored entry in subLogs and
assigning its message to the parent error when unwrapping __isSubagentResult.
Apply this consistently at frontend/src/flow/runFlow.ts:183-195,
frontend/src/pages/Index.tsx:744-800, and frontend/src/pages/Index.tsx:952-1008;
preferably centralize the behavior in a shared unwrapNodeResult helper and reuse
it at all three sites before constructing parent logs.
Comment on lines
+329
to
+384
| const graphId = cfg.graph || ""; | ||
| const targetWf = findWorkflowById(graphId); | ||
| if (!targetWf) { | ||
| throw new Error(`Subagent workflow with ID "${graphId}" not found in library.`); | ||
| } | ||
|
|
||
| // Prepare sub-workflow initial state | ||
| let subInitialState: Record<string, unknown> = {}; | ||
| const rawInput = cfg.input || ""; | ||
| if (rawInput.trim()) { | ||
| const inputValStr = interpolate(rawInput, state, globalsList, secretsList); | ||
| try { | ||
| const parsed = JSON.parse(inputValStr); | ||
| if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { | ||
| subInitialState = parsed as Record<string, unknown>; | ||
| } else { | ||
| subInitialState = { query: inputValStr }; | ||
| } | ||
| } catch { | ||
| // Check if input references a key directly in parent state | ||
| const valueInParentState = state[inputValStr]; | ||
| if (valueInParentState && typeof valueInParentState === "object" && !Array.isArray(valueInParentState)) { | ||
| subInitialState = valueInParentState as Record<string, unknown>; | ||
| } else { | ||
| subInitialState = { query: inputValStr }; | ||
| } | ||
| } | ||
| } else { | ||
| // Deep copy parent state to child if no custom input expression is provided | ||
| try { | ||
| subInitialState = JSON.parse(JSON.stringify(state)); | ||
| } catch { | ||
| subInitialState = { ...state }; | ||
| } | ||
| } | ||
|
|
||
| // Execute recursively using runFlow | ||
| const subLogs = await runFlow({ | ||
| nodes: targetWf.nodes, | ||
| edges: targetWf.edges, | ||
| gateways, | ||
| initialState: subInitialState, | ||
| stepDelay: opts.stepDelay, | ||
| onHumanApproval: opts.onHumanApproval, | ||
| globals: globalsList, | ||
| secrets: secretsList, | ||
| }); | ||
|
|
||
| // The final state of the sub-workflow is the stateSnapshot of the last log entry | ||
| const lastLog = subLogs[subLogs.length - 1]; | ||
| const finalState = lastLog?.stateSnapshot ?? subInitialState; | ||
|
|
||
| return { | ||
| simulated: true, | ||
| subagent: cfg.graph || "unknown", | ||
| input: interpolate(cfg.input || "", state, globalsList, secretsList), | ||
| note: "subagent execution is schematic", | ||
| __isSubagentResult: true, | ||
| output: finalState, | ||
| subLogs, |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Add a cycle/depth guard before recursing into a nested workflow.
runNode resolves targetWf and calls runFlow recursively, but no depth counter or visited-workflow-ID set is threaded through RunOptions. If workflow A's subagent node references workflow B, and B's subagent node references A (directly or through more hops), each level starts its own fresh maxSteps budget, so the recursion is unbounded across levels and can overflow the call stack or hang the tab.
Inspector.tsx's subagentWorkflows filter only excludes the currently active workflow from its dropdown; it does not exclude a workflow that indirectly points back to the current chain, and it does not protect against a cycle introduced by importing/pasting JSON (importJSON, handleFileUpload in Index.tsx), which writes config.graph directly without going through Inspector at all.
Thread a depth counter (or a visited-ID set) through RunOptions and reject before recursing.
🛡️ Proposed fix
export interface RunOptions {
nodes: Node<AgentNodeData>[];
edges: Edge[];
gateways: Gateway[];
initialState?: Record<string, unknown>;
maxSteps?: number;
onLog?: (log: RunLog) => void;
stepDelay?: number;
onHumanApproval?: (req: {
nodeId: string;
name: string;
prompt: string;
channel: string;
}) => Promise<string>;
globals?: { key: string; value: string }[];
secrets?: { key: string; value: string }[];
+ subagentChain?: string[];
} case "subagent": {
const graphId = cfg.graph || "";
const targetWf = findWorkflowById(graphId);
if (!targetWf) {
throw new Error(`Subagent workflow with ID "${graphId}" not found in library.`);
}
+ const MAX_SUBAGENT_DEPTH = 8;
+ const chain = opts.subagentChain ?? [];
+ if (chain.includes(graphId) || chain.length >= MAX_SUBAGENT_DEPTH) {
+ throw new Error(
+ chain.includes(graphId)
+ ? `Subagent cycle detected: "${graphId}" is already in the execution chain.`
+ : `Subagent recursion limit (${MAX_SUBAGENT_DEPTH}) exceeded.`
+ );
+ }
// Prepare sub-workflow initial state
...
const subLogs = await runFlow({
nodes: targetWf.nodes,
edges: targetWf.edges,
gateways,
initialState: subInitialState,
stepDelay: opts.stepDelay,
onHumanApproval: opts.onHumanApproval,
globals: globalsList,
secrets: secretsList,
+ subagentChain: [...chain, graphId],
});Consider adding a subagentNode.test.tsx case that constructs a two-workflow cycle and asserts a clear error instead of a hang.
#!/bin/bash
# Description: Confirm there is no existing recursion/cycle guard for subagent workflows elsewhere in the codebase.
rg -n -C3 'subagentChain|recursionDepth|MAX_SUBAGENT|visited.*[Ww]orkflow' --type=ts
rg -n -C5 'cycl|circular' --type=ts frontend/src/flow🤖 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 `@frontend/src/flow/runFlow.ts` around lines 329 - 384, Add a recursion guard
to the nested workflow execution around runNode’s targetWf resolution and
recursive runFlow call by threading a depth counter or visited workflow-ID set
through RunOptions. Before invoking runFlow, reject cycles or excessive nesting
with a clear error, and ensure recursive calls propagate the guard state while
preserving the existing maxSteps behavior.
Comment on lines
+1992
to
+2076
| {(() => { | ||
| const renderLogItem = (l: RunLog, depth: number = 0): React.ReactNode => { | ||
| const uniqueKey = `${l.step}-${l.nodeId}-${depth}`; | ||
| const isExpanded = !!expandedLogSnapshots[uniqueKey]; | ||
| const isSubLogsExpanded = !!expandedLogSnapshots[`sublogs-${uniqueKey}`]; | ||
| const hasSubLogs = l.subLogs && l.subLogs.length > 0; | ||
|
|
||
| return ( | ||
| <div | ||
| key={uniqueKey} | ||
| className="border border-dashed border-[hsl(var(--grid-line))] p-2 font-mono text-[10px]" | ||
| style={{ | ||
| borderColor: l.error ? "hsl(var(--issue))" : undefined, | ||
| marginLeft: `${depth * 12}px`, | ||
| background: depth > 0 ? "hsl(var(--ink)/0.01)" : undefined, | ||
| }} | ||
| > | ||
| <div className="flex items-center gap-2 mb-1"> | ||
| <span className="text-[hsl(var(--ink-faint))]">#{l.step}</span> | ||
| <span className="font-semibold text-[hsl(var(--ink))]">{l.name}</span> | ||
| <span className="uppercase tracking-[0.15em] text-[9px] text-[hsl(var(--ink-soft))]">{l.kind}</span> | ||
| <span className="ml-auto text-[hsl(var(--ink-faint))]">{l.ms}ms</span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| })} | ||
| <div className="text-[hsl(var(--ink-soft))]"> | ||
| → <span className="uppercase tracking-wider">{l.label}</span> | ||
| </div> | ||
| {l.error ? ( | ||
| <pre className="mt-1 whitespace-pre-wrap text-[hsl(var(--issue))]">{l.error}</pre> | ||
| ) : ( | ||
| <pre className="mt-1 whitespace-pre-wrap text-[hsl(var(--ink))] max-h-40 overflow-auto"> | ||
| {typeof l.output === "string" ? l.output : JSON.stringify(l.output, null, 2)} | ||
| </pre> | ||
| )} | ||
|
|
||
| {hasSubLogs && ( | ||
| <div className="mt-1.5 border-t border-dashed border-[hsl(var(--grid-line))] pt-1.5"> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setExpandedLogSnapshots((prev) => ({ | ||
| ...prev, | ||
| [`sublogs-${uniqueKey}`]: !prev[`sublogs-${uniqueKey}`], | ||
| })); | ||
| }} | ||
| className="select-none font-semibold text-[hsl(var(--edge-selected))] hover:text-[hsl(var(--ink))] flex items-center gap-1 font-mono text-[9px] uppercase tracking-[0.1em]" | ||
| > | ||
| <span className={`transition-transform duration-100 ${isSubLogsExpanded ? "rotate-90" : ""}`}>▶</span> | ||
| <span>{isSubLogsExpanded ? "hide" : "show"} nested workflow logs ({l.subLogs!.length} steps)</span> | ||
| </button> | ||
| {isSubLogsExpanded && ( | ||
| <div className="mt-1.5 space-y-1.5 border-l border-dashed border-[hsl(var(--edge-selected)/0.3)] pl-1.5"> | ||
| {l.subLogs!.map((subL) => renderLogItem(subL, depth + 1))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| )} | ||
|
|
||
| {l.stateSnapshot && ( | ||
| <div className="mt-1.5 border-t border-dashed border-[hsl(var(--grid-line))] pt-1.5"> | ||
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setExpandedLogSnapshots((prev) => ({ | ||
| ...prev, | ||
| [uniqueKey]: !prev[uniqueKey], | ||
| })); | ||
| }} | ||
| className="select-none font-semibold text-[hsl(var(--ink-soft))] hover:text-[hsl(var(--ink))] flex items-center gap-1 font-mono text-[9px] uppercase tracking-[0.1em]" | ||
| > | ||
| <span className={`transition-transform duration-100 ${isExpanded ? "rotate-90" : ""}`}>▶</span> | ||
| <span>state snapshot</span> | ||
| </button> | ||
| {isExpanded && ( | ||
| <pre className="mt-1.5 p-2 bg-[hsl(var(--ink)/0.02)] border border-dashed border-[hsl(var(--grid-line))] overflow-auto max-h-48 text-[9px] leading-relaxed text-[hsl(var(--ink))] whitespace-pre"> | ||
| {JSON.stringify(l.stateSnapshot, null, 2)} | ||
| </pre> | ||
| )} | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| return filteredLogs.map((l) => renderLogItem(l, 0)); | ||
| })()} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the key mismatch that breaks "Expand Snapshots".
renderLogItem computes uniqueKey = ${l.step}-${l.nodeId}-${depth}`` and reads expandedLogSnapshots[uniqueKey] to decide whether a state snapshot is expanded. `handleLogsExpandAll` still builds its patch with the old key format `${l.step}-${l.nodeId}` (no `-${depth}` suffix). Since these strings never match, clicking "Expand Snapshots" no longer expands any state snapshot, including top-level (depth 0) ones — this is a regression for every run, not just nested runs. `handleLogsExpandAll` also never sets the new `sublogs-${uniqueKey}` toggle, so it cannot reveal nested workflow logs either.
🐛 Proposed fix
const handleLogsExpandAll = () => {
if (!runLogs) return;
const patch: Record<string, boolean> = {};
- runLogs.forEach((l) => {
- if (l.stateSnapshot) {
- patch[`${l.step}-${l.nodeId}`] = true;
- }
- });
+ const visit = (logs: RunLog[], depth: number) => {
+ logs.forEach((l) => {
+ const uniqueKey = `${l.step}-${l.nodeId}-${depth}`;
+ if (l.stateSnapshot) patch[uniqueKey] = true;
+ if (l.subLogs && l.subLogs.length > 0) {
+ patch[`sublogs-${uniqueKey}`] = true;
+ visit(l.subLogs, depth + 1);
+ }
+ });
+ };
+ visit(runLogs, 0);
setExpandedLogSnapshots(patch);
};🤖 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 `@frontend/src/pages/Index.tsx` around lines 1992 - 2076, Update
handleLogsExpandAll to use the same depth-aware keys as renderLogItem, including
the depth suffix for every state snapshot so expandedLogSnapshots matches
uniqueKey, including top-level entries. When expanding all, also set the
corresponding sublogs-${uniqueKey} entries and recurse through nested subLogs so
nested workflow logs and their snapshots are revealed.
Comment on lines
+94
to
+137
| it("should render dynamic dropdown for graph selection and exclude active workflow", () => { | ||
| const parentWfId = "parent-flow"; | ||
| mockStorage["agent_flow.active_workflow_id.v1"] = parentWfId; | ||
|
|
||
| const childWf1: Workflow = { | ||
| id: "child-1", | ||
| name: "Child 1", | ||
| nodes: [], | ||
| edges: [], | ||
| createdAt: Date.now(), | ||
| updatedAt: Date.now(), | ||
| }; | ||
|
|
||
| mockStorage["agent_flow.workflows.v1"] = JSON.stringify([childWf1]); | ||
|
|
||
| const activeNode: Node<AgentNodeData> = { | ||
| id: "subagent-node", | ||
| type: "agent", | ||
| position: { x: 0, y: 0 }, | ||
| data: { | ||
| kind: "subagent", | ||
| name: "nested", | ||
| config: { graph: "", input: "" }, | ||
| }, | ||
| }; | ||
|
|
||
| render( | ||
| <Inspector | ||
| node={activeNode} | ||
| edges={[]} | ||
| nodes={[activeNode]} | ||
| onChange={vi.fn()} | ||
| onDelete={vi.fn()} | ||
| /> | ||
| ); | ||
|
|
||
| // Verify subagent dropdown select renders | ||
| const select = screen.getByRole("combobox"); | ||
| expect(select).toBeInTheDocument(); | ||
|
|
||
| // Verify template (e.g. ReAct Agent Loop) and Child 1 are present, but active parent-flow is not | ||
| expect(screen.getByText(/ReAct Agent Loop/)).toBeInTheDocument(); | ||
| expect(screen.getByText(/Child 1/)).toBeInTheDocument(); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the exclusion test actually exercise exclusion.
This test sets the active workflow ID to "parent-flow" but never stores a workflow with that ID. mockStorage["agent_flow.workflows.v1"] only contains childWf1. The assertions only check that ReAct Agent Loop and Child 1 are present — nothing asserts that a workflow matching the active ID is absent. Because no option for "parent-flow" exists to begin with, this test passes even if the w.id !== activeId filter in Inspector.tsx is removed entirely. It does not verify the exclusion behavior it is named for.
Add a workflow matching the active ID and assert it does not render.
✅ Proposed fix
const childWf1: Workflow = {
id: "child-1",
name: "Child 1",
nodes: [],
edges: [],
createdAt: Date.now(),
updatedAt: Date.now(),
};
- mockStorage["agent_flow.workflows.v1"] = JSON.stringify([childWf1]);
+ const parentWf: Workflow = {
+ id: parentWfId,
+ name: "Parent Flow",
+ nodes: [],
+ edges: [],
+ createdAt: Date.now(),
+ updatedAt: Date.now(),
+ };
+
+ mockStorage["agent_flow.workflows.v1"] = JSON.stringify([childWf1, parentWf]); expect(screen.getByText(/ReAct Agent Loop/)).toBeInTheDocument();
expect(screen.getByText(/Child 1/)).toBeInTheDocument();
+ expect(screen.queryByText(/Parent Flow/)).not.toBeInTheDocument();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("should render dynamic dropdown for graph selection and exclude active workflow", () => { | |
| const parentWfId = "parent-flow"; | |
| mockStorage["agent_flow.active_workflow_id.v1"] = parentWfId; | |
| const childWf1: Workflow = { | |
| id: "child-1", | |
| name: "Child 1", | |
| nodes: [], | |
| edges: [], | |
| createdAt: Date.now(), | |
| updatedAt: Date.now(), | |
| }; | |
| mockStorage["agent_flow.workflows.v1"] = JSON.stringify([childWf1]); | |
| const activeNode: Node<AgentNodeData> = { | |
| id: "subagent-node", | |
| type: "agent", | |
| position: { x: 0, y: 0 }, | |
| data: { | |
| kind: "subagent", | |
| name: "nested", | |
| config: { graph: "", input: "" }, | |
| }, | |
| }; | |
| render( | |
| <Inspector | |
| node={activeNode} | |
| edges={[]} | |
| nodes={[activeNode]} | |
| onChange={vi.fn()} | |
| onDelete={vi.fn()} | |
| /> | |
| ); | |
| // Verify subagent dropdown select renders | |
| const select = screen.getByRole("combobox"); | |
| expect(select).toBeInTheDocument(); | |
| // Verify template (e.g. ReAct Agent Loop) and Child 1 are present, but active parent-flow is not | |
| expect(screen.getByText(/ReAct Agent Loop/)).toBeInTheDocument(); | |
| expect(screen.getByText(/Child 1/)).toBeInTheDocument(); | |
| }); | |
| it("should render dynamic dropdown for graph selection and exclude active workflow", () => { | |
| const parentWfId = "parent-flow"; | |
| mockStorage["agent_flow.active_workflow_id.v1"] = parentWfId; | |
| const childWf1: Workflow = { | |
| id: "child-1", | |
| name: "Child 1", | |
| nodes: [], | |
| edges: [], | |
| createdAt: Date.now(), | |
| updatedAt: Date.now(), | |
| }; | |
| const parentWf: Workflow = { | |
| id: parentWfId, | |
| name: "Parent Flow", | |
| nodes: [], | |
| edges: [], | |
| createdAt: Date.now(), | |
| updatedAt: Date.now(), | |
| }; | |
| mockStorage["agent_flow.workflows.v1"] = JSON.stringify([childWf1, parentWf]); | |
| const activeNode: Node<AgentNodeData> = { | |
| id: "subagent-node", | |
| type: "agent", | |
| position: { x: 0, y: 0 }, | |
| data: { | |
| kind: "subagent", | |
| name: "nested", | |
| config: { graph: "", input: "" }, | |
| }, | |
| }; | |
| render( | |
| <Inspector | |
| node={activeNode} | |
| edges={[]} | |
| nodes={[activeNode]} | |
| onChange={vi.fn()} | |
| onDelete={vi.fn()} | |
| /> | |
| ); | |
| // Verify subagent dropdown select renders | |
| const select = screen.getByRole("combobox"); | |
| expect(select).toBeInTheDocument(); | |
| // Verify template (e.g. ReAct Agent Loop) and Child 1 are present, but active parent-flow is not | |
| expect(screen.getByText(/ReAct Agent Loop/)).toBeInTheDocument(); | |
| expect(screen.getByText(/Child 1/)).toBeInTheDocument(); | |
| expect(screen.queryByText(/Parent Flow/)).not.toBeInTheDocument(); | |
| }); |
🤖 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 `@frontend/src/test/subagentNode.test.tsx` around lines 94 - 137, Update the
test “should render dynamic dropdown for graph selection and exclude active
workflow” by adding a stored workflow with id “parent-flow” alongside childWf1,
then assert that its rendered name is absent. Keep the existing assertions
confirming the template and Child 1 remain present.
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change introduces fully functional recursive in-browser execution of Subagent nodes.
It adds:
PR created automatically by Jules for task 8652235537390379193 started by @Jacobcdsmith
Summary by CodeRabbit
New Features
Bug Fixes