feat(web): loop node iteration visibility in workflow execution view#1026
Merged
feat(web): loop node iteration visibility in workflow execution view#1026
Conversation
…#1014) Loop nodes in DAG workflows appeared as flat nodes with no iteration visibility. The backend already emitted loop_iteration_* events but the frontend dropped them. Changes: - Add nodeId to workflow_step SSE events in workflow-bridge - Add LoopIterationEvent/LoopIterationInfo types and extend DagNodeState - Handle workflow_step events in useSSE and route to store - Add handleLoopIteration action to workflow store - Extract loop_iteration_* events for historical REST view - Add expandable iteration sub-list in DagNodeProgress sidebar - Add iteration count badge on graph nodes (ExecutionDagNode) - Add loop type color/label to graph node type maps Fixes #1014 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds per-loop-iteration visibility: server now includes Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Owner
Author
Comprehensive PR Review — 4 AgentsPR: #1026 — feat(web): loop node iteration visibility in workflow execution view SummarySolid scope discipline and correct type design. Two HIGH bugs need fixes before merge. Verdict: REQUEST_CHANGES
HIGH —
|
…n, guard, tests - Preserve accumulated iteration state in handleDagNode by spreading existing node before overwriting with event fields (HIGH: iteration badge/list vanished on node completion) - Replace nested <button> with <span role="button"> + onKeyDown in DagNodeProgress to fix HTML spec violation and broken stopPropagation (HIGH: accessibility + mis-navigation) - Fix falsy guard `if (!iteration)` → `if (iteration === undefined)` in WorkflowExecution REST enrichment (MEDIUM: would silently drop iteration 0 in future) - Fix dead file reference in workflow-bridge comments: `useWorkflowStatus.ts` → `workflow-store.ts handleLoopIteration` (MEDIUM: misleading comment) - Add 7 unit tests for handleLoopIteration in workflow-store.test.ts covering all branches: no-nodeId no-op, ghost-node no-op, first append, upsert, total:0 preservation, accumulation, and iteration state survival across dag_node completion (MEDIUM: zero coverage for core PR logic) - Clarify two LOW comment gaps in WorkflowExecution.tsx and workflow-store.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
Fix Report — PR #1026 Review FindingsAll 9 review findings addressed in commit HIGH Issues Fixed (2/2)Issue 1: Issue 2: Nested MEDIUM Issues Fixed (3/3)Issue 3: Falsy guard silently skips iteration 0 Issue 4: Missing unit tests for Issue 5: Dead file reference in bridge comments LOW Issues Fixed (2/2)
Validation
Deferred as Follow-up Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/adapters/web/workflow-bridge.ts (1)
63-75:⚠️ Potential issue | 🟡 MinorInclude
durationon failed loop iteration events.
LoopIterationFailedEventalready carriesduration, but this payload drops it. That means failed iterations lose their elapsed time in the live UI even though the executor emitted it.Suggested fix
case 'loop_iteration_failed': return JSON.stringify({ type: 'workflow_step', runId: event.runId, nodeId: event.nodeId, step: event.iteration - 1, // total: 0 intentionally — maxIterations is not carried by loop_iteration_completed/failed events. // workflow-store.ts handleLoopIteration guards against 0 by preserving the prior wf.maxIterations value. total: 0, name: `iteration-${String(event.iteration)}`, status: 'failed', + duration: event.duration, iteration: event.iteration, timestamp: Date.now(), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/adapters/web/workflow-bridge.ts` around lines 63 - 75, The 'loop_iteration_failed' case in workflow-bridge.ts drops the event.duration even though LoopIterationFailedEvent contains it; update the JSON payload returned in the 'loop_iteration_failed' branch to include duration: event.duration (preserving the numeric value) so failed loop iterations carry elapsed time into the live UI—locate the case 'loop_iteration_failed' in the switch that returns the workflow_step object and add the duration property to that returned object.
🧹 Nitpick comments (1)
packages/web/src/components/workflows/DagNodeProgress.tsx (1)
80-92: Minor: redundant nullish coalescing after guard.The
hasIterationscheck on line 80 already ensuresnode.iterationsis non-empty, making the?? []fallback on line 82 defensive but redundant.🔧 Optional simplification
{expanded && hasIterations && ( <div className="ml-6 mt-0.5 space-y-0.5"> - {(node.iterations ?? []).map(iter => ( + {node.iterations!.map(iter => ( <div key={iter.iteration} className="flex items-center gap-2 px-2 py-1 text-xs">Alternatively, keep the
?? []as a defensive pattern if you prefer avoiding non-null assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/DagNodeProgress.tsx` around lines 80 - 92, The nullish fallback (?? []) in the map is redundant because the surrounding guard uses hasIterations (and expanded) to ensure node.iterations exists; remove the fallback from the map expression so the code uses (node.iterations.map(...)) directly. Update the JSX block that renders iterations (the conditional using expanded && hasIterations and the mapping over node.iterations) to eliminate the unnecessary ?? [], keeping StatusIcon, formatDurationMs and the existing key/fields unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/workflows/WorkflowExecution.tsx`:
- Around line 145-163: The code is reading loop iteration duration from the
wrong field (e.data.duration_ms) causing durations to be lost on reload; update
the duration read to use e.data.duration (and search for any other occurrences
of duration_ms in this file) so LoopIterationInfo.duration is populated from
e.data.duration wherever loop iteration events are parsed (e.g., the block
building iterState from e.data.duration_ms).
---
Outside diff comments:
In `@packages/server/src/adapters/web/workflow-bridge.ts`:
- Around line 63-75: The 'loop_iteration_failed' case in workflow-bridge.ts
drops the event.duration even though LoopIterationFailedEvent contains it;
update the JSON payload returned in the 'loop_iteration_failed' branch to
include duration: event.duration (preserving the numeric value) so failed loop
iterations carry elapsed time into the live UI—locate the case
'loop_iteration_failed' in the switch that returns the workflow_step object and
add the duration property to that returned object.
---
Nitpick comments:
In `@packages/web/src/components/workflows/DagNodeProgress.tsx`:
- Around line 80-92: The nullish fallback (?? []) in the map is redundant
because the surrounding guard uses hasIterations (and expanded) to ensure
node.iterations exists; remove the fallback from the map expression so the code
uses (node.iterations.map(...)) directly. Update the JSX block that renders
iterations (the conditional using expanded && hasIterations and the mapping over
node.iterations) to eliminate the unnecessary ?? [], keeping StatusIcon,
formatDurationMs and the existing key/fields unchanged.
🪄 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
Run ID: 05175123-faa4-4eee-b226-5b3324b9acd8
📒 Files selected for processing (9)
packages/server/src/adapters/web/workflow-bridge.tspackages/web/src/components/workflows/DagNodeProgress.tsxpackages/web/src/components/workflows/ExecutionDagNode.tsxpackages/web/src/components/workflows/WorkflowDagViewer.tsxpackages/web/src/components/workflows/WorkflowExecution.tsxpackages/web/src/hooks/useSSE.tspackages/web/src/lib/types.tspackages/web/src/stores/workflow-store.test.tspackages/web/src/stores/workflow-store.ts
Comment on lines
+145
to
+163
| const iteration = e.data.iteration as number | undefined; | ||
| const maxIter = e.data.maxIterations as number | undefined; | ||
| if (iteration === undefined) continue; | ||
|
|
||
| let iterStatus: LoopIterationInfo['status']; | ||
| if (e.event_type === 'loop_iteration_started') { | ||
| iterStatus = 'running'; | ||
| } else if (e.event_type === 'loop_iteration_completed') { | ||
| iterStatus = 'completed'; | ||
| } else { | ||
| iterStatus = 'failed'; | ||
| } | ||
|
|
||
| const existingIters: LoopIterationInfo[] = existing.iterations ?? []; | ||
| const iterIdx = existingIters.findIndex(it => it.iteration === iteration); | ||
| const iterState: LoopIterationInfo = { | ||
| iteration, | ||
| status: iterStatus, | ||
| duration: e.data.duration_ms as number | undefined, |
There was a problem hiding this comment.
Read the persisted loop duration field here.
The executor stores loop iteration elapsed time under data.duration, not data.duration_ms. With the current key, every historical iteration reloaded from REST gets duration: undefined, so iteration timing disappears after a refresh. The same field name should be aligned anywhere else this file reads loop iteration durations.
Suggested fix
const iterState: LoopIterationInfo = {
iteration,
status: iterStatus,
- duration: e.data.duration_ms as number | undefined,
+ duration: e.data.duration as number | undefined,
};📝 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
| const iteration = e.data.iteration as number | undefined; | |
| const maxIter = e.data.maxIterations as number | undefined; | |
| if (iteration === undefined) continue; | |
| let iterStatus: LoopIterationInfo['status']; | |
| if (e.event_type === 'loop_iteration_started') { | |
| iterStatus = 'running'; | |
| } else if (e.event_type === 'loop_iteration_completed') { | |
| iterStatus = 'completed'; | |
| } else { | |
| iterStatus = 'failed'; | |
| } | |
| const existingIters: LoopIterationInfo[] = existing.iterations ?? []; | |
| const iterIdx = existingIters.findIndex(it => it.iteration === iteration); | |
| const iterState: LoopIterationInfo = { | |
| iteration, | |
| status: iterStatus, | |
| duration: e.data.duration_ms as number | undefined, | |
| const iteration = e.data.iteration as number | undefined; | |
| const maxIter = e.data.maxIterations as number | undefined; | |
| if (iteration === undefined) continue; | |
| let iterStatus: LoopIterationInfo['status']; | |
| if (e.event_type === 'loop_iteration_started') { | |
| iterStatus = 'running'; | |
| } else if (e.event_type === 'loop_iteration_completed') { | |
| iterStatus = 'completed'; | |
| } else { | |
| iterStatus = 'failed'; | |
| } | |
| const existingIters: LoopIterationInfo[] = existing.iterations ?? []; | |
| const iterIdx = existingIters.findIndex(it => it.iteration === iteration); | |
| const iterState: LoopIterationInfo = { | |
| iteration, | |
| status: iterStatus, | |
| duration: e.data.duration as number | undefined, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/workflows/WorkflowExecution.tsx` around lines 145
- 163, The code is reading loop iteration duration from the wrong field
(e.data.duration_ms) causing durations to be lost on reload; update the duration
read to use e.data.duration (and search for any other occurrences of duration_ms
in this file) so LoopIterationInfo.duration is populated from e.data.duration
wherever loop iteration events are parsed (e.g., the block building iterState
from e.data.duration_ms).
…ve element - Add missing `workflow_step` case in useDashboardSSE switch so loop iteration events are routed to the store instead of silently dropped - Restructure DagNodeItem: replace outer <button> with <div role="row"> and change the expand toggle from <span role="button"> to a proper <button>, eliminating the interactive-element-in-interactive-element violation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tasty007
pushed a commit
to tasty007/Archon
that referenced
this pull request
Apr 10, 2026
…up (coleam00#1027) * Fix: detect squash-merged and PR-merged branches in isolation cleanup (coleam00#1026) `isolation cleanup --merged` only used `git branch --merged`, which misses squash-merged branches because the resulting commit has a different SHA. Bulk cleanup of task worktrees required a manual `gh pr list` per branch. Changes: - Add `isPatchEquivalent()` to `@archon/git` using `git cherry` to detect squash-merged branches - Add `getPrState()` to `@archon/isolation` for `gh`-based PR state lookup with per-invocation caching; soft-fails on missing gh / non-GitHub remotes - `cleanupMergedWorktrees()` now unions three signals (ancestry, patch equivalence, PR state); skips with a clear reason when PR is OPEN - Add `--include-closed` flag to `archon isolation cleanup --merged` to also remove worktrees whose PRs were closed without merging - Tests for all new code paths Fixes coleam00#1026 * fix: address review findings for squash-merge cleanup PR - branch.ts: add 'bad revision' to isPatchEquivalent expected errors so manually-deleted branches return false instead of throwing - pr-state.ts: add repoPath context to warn/debug log calls; capture ghStdout before try block to include in warn log for parse failures - pr-state.test.ts: remove redundant beforeEach reset (setupGhResponse already resets); add tests for non-ENOENT gh error and malformed JSON - cleanup-service.test.ts: add test for isPatchEquivalent unexpected throw → skipped with 'merge check failed' reason - isolation-operations.test.ts: add test for includeClosed: true forwarding through cleanupMergedEnvironments - docs: add --include-closed to all five affected docs (CLAUDE.md, reference/cli.md, book/isolation.md, book/quick-reference.md, getting-started/overview.md) - cli-internals.md: add isolation cleanup --merged flow diagram * simplify: remove redundant assignments and verbose filter in new code
Three fixes for message duplication during live workflow execution: 1. dag-executor: Add missing `tool_call_formatted` category to loop iteration tool messages. Without this, the web adapter sent tool text as both a regular SSE text event AND a structured tool_call event, causing each tool to appear twice (raw text + rendered card). Regular DAG nodes already had this metadata. 2. WorkflowLogs: Add text content dedup in SSE/DB merge. During live execution, the same text (e.g. "Starting workflow...") can appear in both DB (REST fetch) and SSE (event buffer replay). Collects DB text into a Set and skips matching SSE text messages. 3. orchestrator-agent: Suppress remainingMessage re-send in stream mode. The routing AI streams text chunks before /invoke-workflow is detected, then retracts them. Without suppression, remainingMessage re-sends the same text. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WorkflowLogs' onText handler was blindly concatenating all SSE text into a single streaming message, unlike ChatInterface which splits on workflow status text (🚀/✅). This caused the "Starting workflow" text to merge with subsequent text into one giant message, breaking text dedup against DB messages (which are stored as separate segments). The SSE message content never matched any single DB message exactly, so both appeared. Add the same workflow status boundary detection from ChatInterface: close the current streaming message and start a new one when a workflow status message arrives or when regular text follows a status message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/workflows/DagNodeProgress.tsx`:
- Around line 26-34: The row div in DagNodeProgress.tsx is not
keyboard-accessible; make it focusable and operable via keyboard by either
replacing the div with a semantic button or adding tabIndex={0} and an onKeyDown
handler that calls onNodeClick(node.nodeId) for Enter and Space, keep role="row"
or change to role="button" and update aria-pressed/aria-current as appropriate
when isActive; ensure you reference the existing onNodeClick, node.nodeId and
isActive symbols when implementing the fix.
- Around line 12-20: Create a new interface named DagNodeItemProps that
describes the props for DagNodeItem (including node: DagNodeState, isActive:
boolean, onNodeClick: (nodeId: string) => void), replace the inline prop type in
the DagNodeItem function signature with this DagNodeItemProps interface, and
update any references/usages to use the named interface to match the existing
DagNodeProgressProps pattern and improve reusability.
In `@packages/web/src/components/workflows/WorkflowLogs.tsx`:
- Around line 391-415: The current prefix-based dedup (using
[...dbTextContents].some(dc => dc.startsWith(m.content))) is too broad and can
hide unrelated SSE text; change it so prefix suppression only runs for actively
streaming/partial SSE messages (e.g., check a flag like m.isStreaming or
m.isPartial) and compare the SSE content only against the latest DB assistant
message instead of every DB message (derive the lastAssistantContent from
filteredDbMessages). Update the condition in the sseMessages loop (where
dbTextContents and dedupedSse are used) to: skip prefix-check unless m is
streaming/partial, and when applied, only test startsWith against
lastAssistantContent.
🪄 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
Run ID: 177a5b16-35d9-49e8-9d5e-c38313d54dd5
📒 Files selected for processing (6)
packages/core/src/orchestrator/orchestrator-agent.tspackages/web/src/components/workflows/DagNodeProgress.tsxpackages/web/src/components/workflows/WorkflowLogs.tsxpackages/web/src/hooks/useDashboardSSE.tspackages/web/src/stores/workflow-store.test.tspackages/workflows/src/dag-executor.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/stores/workflow-store.test.ts
Comment on lines
+12
to
+20
| function DagNodeItem({ | ||
| node, | ||
| isActive, | ||
| onNodeClick, | ||
| }: { | ||
| node: DagNodeState; | ||
| isActive: boolean; | ||
| onNodeClick: (nodeId: string) => void; | ||
| }): React.ReactElement { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether DagNodeItem currently uses inline object typing for props.
# Expected: one match on the inline "} : {" props annotation before refactor.
rg -n -C2 --type=tsx 'function DagNodeItem|^\s*}:\s*\{' packages/web/src/components/workflows/DagNodeProgress.tsxRepository: coleam00/Archon
Length of output: 86
🏁 Script executed:
# Corrected search - use -g glob pattern instead of --type=tsx
rg -n -C3 'function DagNodeItem' packages/web/src/components/workflows/DagNodeProgress.tsxRepository: coleam00/Archon
Length of output: 180
🏁 Script executed:
# Also read the file to see current state
wc -l packages/web/src/components/workflows/DagNodeProgress.tsxRepository: coleam00/Archon
Length of output: 120
🏁 Script executed:
# Check for any existing props interfaces in this file
rg -n 'interface.*Props|type.*Props' packages/web/src/components/workflows/DagNodeProgress.tsxRepository: coleam00/Archon
Length of output: 93
🏁 Script executed:
# Read lines 6-40 to see DagNodeProgressProps interface and DagNodeItem function
sed -n '6,40p' packages/web/src/components/workflows/DagNodeProgress.tsxRepository: coleam00/Archon
Length of output: 1016
Extract DagNodeItem props into a dedicated DagNodeItemProps interface.
Component props should use interfaces per the TypeScript guidelines for major abstractions. Currently, props are defined inline, which reduces reusability and inconsistency with the existing DagNodeProgressProps pattern used elsewhere in this file.
Suggested refactor
+interface DagNodeItemProps {
+ node: DagNodeState;
+ isActive: boolean;
+ onNodeClick: (nodeId: string) => void;
+}
+
function DagNodeItem({
node,
isActive,
onNodeClick,
-}: {
- node: DagNodeState;
- isActive: boolean;
- onNodeClick: (nodeId: string) => void;
-}): React.ReactElement {
+}: DagNodeItemProps): React.ReactElement {📝 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
| function DagNodeItem({ | |
| node, | |
| isActive, | |
| onNodeClick, | |
| }: { | |
| node: DagNodeState; | |
| isActive: boolean; | |
| onNodeClick: (nodeId: string) => void; | |
| }): React.ReactElement { | |
| interface DagNodeItemProps { | |
| node: DagNodeState; | |
| isActive: boolean; | |
| onNodeClick: (nodeId: string) => void; | |
| } | |
| function DagNodeItem({ | |
| node, | |
| isActive, | |
| onNodeClick, | |
| }: DagNodeItemProps): React.ReactElement { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/workflows/DagNodeProgress.tsx` around lines 12 -
20, Create a new interface named DagNodeItemProps that describes the props for
DagNodeItem (including node: DagNodeState, isActive: boolean, onNodeClick:
(nodeId: string) => void), replace the inline prop type in the DagNodeItem
function signature with this DagNodeItemProps interface, and update any
references/usages to use the named interface to match the existing
DagNodeProgressProps pattern and improve reusability.
Comment on lines
+26
to
+34
| <div | ||
| className={`w-full text-left px-2 py-1.5 rounded transition-colors cursor-pointer ${ | ||
| isActive ? 'bg-accent/10 border-l-2 border-accent' : 'hover:bg-surface-hover' | ||
| }`} | ||
| onClick={(): void => { | ||
| onNodeClick(node.nodeId); | ||
| }} | ||
| role="row" | ||
| > |
There was a problem hiding this comment.
Row selection is not keyboard-accessible.
The clickable row is a non-focusable <div> with mouse-only activation. Keyboard users can’t select a node from this control.
♿ Minimal accessibility fix
<div
className={`w-full text-left px-2 py-1.5 rounded transition-colors cursor-pointer ${
isActive ? 'bg-accent/10 border-l-2 border-accent' : 'hover:bg-surface-hover'
}`}
onClick={(): void => {
onNodeClick(node.nodeId);
}}
- role="row"
+ onKeyDown={(e): void => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onNodeClick(node.nodeId);
+ }
+ }}
+ role="button"
+ tabIndex={0}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/workflows/DagNodeProgress.tsx` around lines 26 -
34, The row div in DagNodeProgress.tsx is not keyboard-accessible; make it
focusable and operable via keyboard by either replacing the div with a semantic
button or adding tabIndex={0} and an onKeyDown handler that calls
onNodeClick(node.nodeId) for Enter and Space, keep role="row" or change to
role="button" and update aria-pressed/aria-current as appropriate when isActive;
ensure you reference the existing onNodeClick, node.nodeId and isActive symbols
when implementing the fix.
Comment on lines
+391
to
+415
| // Collect DB text content for dedup against SSE text messages. | ||
| // During live execution, the same text (e.g., "🚀 Starting workflow...") can appear | ||
| // in both DB (from REST fetch on mount) and SSE (from event buffer replay). | ||
| // Without dedup, the text shows up twice in the message list. | ||
| const dbTextContents = new Set<string>(); | ||
| for (const dm of filteredDbMessages) { | ||
| if (dm.role === 'assistant' && dm.content) { | ||
| dbTextContents.add(dm.content); | ||
| } | ||
| } | ||
|
|
||
| // Strip SSE tool calls that already appear in DB messages (completed). | ||
| // Also strip SSE text messages that are already in DB (prevents duplicate text). | ||
| const dedupedSse: ChatMessage[] = []; | ||
| for (const m of sseMessages) { | ||
| if (!m.toolCalls?.length) { | ||
| // Skip SSE text-only messages whose content already exists in DB. | ||
| if (m.content && dbTextContents.has(m.content)) { | ||
| continue; | ||
| } | ||
| // Also skip if DB has a message that starts with the SSE content | ||
| // (SSE text was flushed to DB before SSE finished accumulating). | ||
| if (m.content && [...dbTextContents].some(dc => dc.startsWith(m.content))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Prefix dedup is too broad and can hide valid live SSE text.
At Line 413, prefix matching against any DB assistant content can suppress unrelated SSE messages (especially short/common prefixes). Limit prefix-based suppression to actively streaming SSE text and preferably only against the latest DB assistant message.
Suggested adjustment
- const dbTextContents = new Set<string>();
- for (const dm of filteredDbMessages) {
- if (dm.role === 'assistant' && dm.content) {
- dbTextContents.add(dm.content);
- }
- }
+ const dbAssistantContents = filteredDbMessages
+ .filter((dm): dm is ChatMessage => dm.role === 'assistant' && !!dm.content)
+ .map(dm => dm.content);
+ const dbTextContents = new Set<string>(dbAssistantContents);
+ const latestDbAssistantContent =
+ dbAssistantContents.length > 0 ? dbAssistantContents[dbAssistantContents.length - 1] : '';
@@
- if (m.content && [...dbTextContents].some(dc => dc.startsWith(m.content))) {
+ if (m.isStreaming && m.content && latestDbAssistantContent.startsWith(m.content)) {
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/components/workflows/WorkflowLogs.tsx` around lines 391 -
415, The current prefix-based dedup (using [...dbTextContents].some(dc =>
dc.startsWith(m.content))) is too broad and can hide unrelated SSE text; change
it so prefix suppression only runs for actively streaming/partial SSE messages
(e.g., check a flag like m.isStreaming or m.isPartial) and compare the SSE
content only against the latest DB assistant message instead of every DB message
(derive the lastAssistantContent from filteredDbMessages). Update the condition
in the sseMessages loop (where dbTextContents and dedupedSse are used) to: skip
prefix-check unless m is streaming/partial, and when applied, only test
startsWith against lastAssistantContent.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/web/src/components/workflows/WorkflowLogs.tsx (1)
391-415:⚠️ Potential issue | 🟡 MinorNarrow the prefix-based SSE text dedup.
The
startsWithsuppression still checks against every persisted assistant message, so an older DB message can hide unrelated live SSE text that merely shares a short prefix. Restrict this prefix check to actively streaming SSE text and compare only against the latest DB assistant message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/workflows/WorkflowLogs.tsx` around lines 391 - 415, The prefix-based suppression is too broad: change the startsWith check so it only compares the SSE message content (m.content) against the most recent persisted assistant message, not every DB message. Compute the latest assistant content from filteredDbMessages (find the last dm where dm.role === 'assistant' && dm.content) and then, only when the SSE message is actively streaming (check the streaming flag on the SSE message, e.g., m.isStreaming or equivalent), skip the SSE message if latestAssistantContent.startsWith(m.content); remove the [...dbTextContents].some(...) scan and use latestAssistantContent instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/web/src/components/workflows/WorkflowLogs.tsx`:
- Around line 391-415: The prefix-based suppression is too broad: change the
startsWith check so it only compares the SSE message content (m.content) against
the most recent persisted assistant message, not every DB message. Compute the
latest assistant content from filteredDbMessages (find the last dm where dm.role
=== 'assistant' && dm.content) and then, only when the SSE message is actively
streaming (check the streaming flag on the SSE message, e.g., m.isStreaming or
equivalent), skip the SSE message if
latestAssistantContent.startsWith(m.content); remove the
[...dbTextContents].some(...) scan and use latestAssistantContent instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2ea9c42-56a4-4064-9d6d-7523a5a0f67d
📒 Files selected for processing (1)
packages/web/src/components/workflows/WorkflowLogs.tsx
The suppression broke the "sends remaining message before dispatching workflow" test — when the AI response contains both text and a command in a single chunk, the text was never streamed, so suppressing remainingMessage loses it entirely. The actual duplicate was in the WorkflowLogs execution view, not the routing AI path, and is already fixed by the onText message splitting and text content dedup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 10, 2026
Closed
Closed
Closed
Tyone88
pushed a commit
to Tyone88/Archon
that referenced
this pull request
Apr 16, 2026
…up (coleam00#1027) * Fix: detect squash-merged and PR-merged branches in isolation cleanup (coleam00#1026) `isolation cleanup --merged` only used `git branch --merged`, which misses squash-merged branches because the resulting commit has a different SHA. Bulk cleanup of task worktrees required a manual `gh pr list` per branch. Changes: - Add `isPatchEquivalent()` to `@archon/git` using `git cherry` to detect squash-merged branches - Add `getPrState()` to `@archon/isolation` for `gh`-based PR state lookup with per-invocation caching; soft-fails on missing gh / non-GitHub remotes - `cleanupMergedWorktrees()` now unions three signals (ancestry, patch equivalence, PR state); skips with a clear reason when PR is OPEN - Add `--include-closed` flag to `archon isolation cleanup --merged` to also remove worktrees whose PRs were closed without merging - Tests for all new code paths Fixes coleam00#1026 * fix: address review findings for squash-merge cleanup PR - branch.ts: add 'bad revision' to isPatchEquivalent expected errors so manually-deleted branches return false instead of throwing - pr-state.ts: add repoPath context to warn/debug log calls; capture ghStdout before try block to include in warn log for parse failures - pr-state.test.ts: remove redundant beforeEach reset (setupGhResponse already resets); add tests for non-ENOENT gh error and malformed JSON - cleanup-service.test.ts: add test for isPatchEquivalent unexpected throw → skipped with 'merge check failed' reason - isolation-operations.test.ts: add test for includeClosed: true forwarding through cleanupMergedEnvironments - docs: add --include-closed to all five affected docs (CLAUDE.md, reference/cli.md, book/isolation.md, book/quick-reference.md, getting-started/overview.md) - cli-internals.md: add isolation cleanup --merged flow diagram * simplify: remove redundant assignments and verbose filter in new code
Tyone88
pushed a commit
to Tyone88/Archon
that referenced
this pull request
Apr 16, 2026
…ed button, guard, tests - Preserve accumulated iteration state in handleDagNode by spreading existing node before overwriting with event fields (HIGH: iteration badge/list vanished on node completion) - Replace nested <button> with <span role="button"> + onKeyDown in DagNodeProgress to fix HTML spec violation and broken stopPropagation (HIGH: accessibility + mis-navigation) - Fix falsy guard `if (!iteration)` → `if (iteration === undefined)` in WorkflowExecution REST enrichment (MEDIUM: would silently drop iteration 0 in future) - Fix dead file reference in workflow-bridge comments: `useWorkflowStatus.ts` → `workflow-store.ts handleLoopIteration` (MEDIUM: misleading comment) - Add 7 unit tests for handleLoopIteration in workflow-store.test.ts covering all branches: no-nodeId no-op, ghost-node no-op, first append, upsert, total:0 preservation, accumulation, and iteration state survival across dag_node completion (MEDIUM: zero coverage for core PR logic) - Clarify two LOW comment gaps in WorkflowExecution.tsx and workflow-store.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tyone88
pushed a commit
to Tyone88/Archon
that referenced
this pull request
Apr 16, 2026
…1014 feat(web): loop node iteration visibility in workflow execution view
This was referenced Apr 17, 2026
This was referenced Apr 20, 2026
Closed
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…up (coleam00#1027) * Fix: detect squash-merged and PR-merged branches in isolation cleanup (coleam00#1026) `isolation cleanup --merged` only used `git branch --merged`, which misses squash-merged branches because the resulting commit has a different SHA. Bulk cleanup of task worktrees required a manual `gh pr list` per branch. Changes: - Add `isPatchEquivalent()` to `@archon/git` using `git cherry` to detect squash-merged branches - Add `getPrState()` to `@archon/isolation` for `gh`-based PR state lookup with per-invocation caching; soft-fails on missing gh / non-GitHub remotes - `cleanupMergedWorktrees()` now unions three signals (ancestry, patch equivalence, PR state); skips with a clear reason when PR is OPEN - Add `--include-closed` flag to `archon isolation cleanup --merged` to also remove worktrees whose PRs were closed without merging - Tests for all new code paths Fixes coleam00#1026 * fix: address review findings for squash-merge cleanup PR - branch.ts: add 'bad revision' to isPatchEquivalent expected errors so manually-deleted branches return false instead of throwing - pr-state.ts: add repoPath context to warn/debug log calls; capture ghStdout before try block to include in warn log for parse failures - pr-state.test.ts: remove redundant beforeEach reset (setupGhResponse already resets); add tests for non-ENOENT gh error and malformed JSON - cleanup-service.test.ts: add test for isPatchEquivalent unexpected throw → skipped with 'merge check failed' reason - isolation-operations.test.ts: add test for includeClosed: true forwarding through cleanupMergedEnvironments - docs: add --include-closed to all five affected docs (CLAUDE.md, reference/cli.md, book/isolation.md, book/quick-reference.md, getting-started/overview.md) - cli-internals.md: add isolation cleanup --merged flow diagram * simplify: remove redundant assignments and verbose filter in new code
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…ed button, guard, tests - Preserve accumulated iteration state in handleDagNode by spreading existing node before overwriting with event fields (HIGH: iteration badge/list vanished on node completion) - Replace nested <button> with <span role="button"> + onKeyDown in DagNodeProgress to fix HTML spec violation and broken stopPropagation (HIGH: accessibility + mis-navigation) - Fix falsy guard `if (!iteration)` → `if (iteration === undefined)` in WorkflowExecution REST enrichment (MEDIUM: would silently drop iteration 0 in future) - Fix dead file reference in workflow-bridge comments: `useWorkflowStatus.ts` → `workflow-store.ts handleLoopIteration` (MEDIUM: misleading comment) - Add 7 unit tests for handleLoopIteration in workflow-store.test.ts covering all branches: no-nodeId no-op, ghost-node no-op, first append, upsert, total:0 preservation, accumulation, and iteration state survival across dag_node completion (MEDIUM: zero coverage for core PR logic) - Clarify two LOW comment gaps in WorkflowExecution.tsx and workflow-store.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
…1014 feat(web): loop node iteration visibility in workflow execution view
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.
Summary
loop_iteration_*events were stored in the DB and emitted over SSE, but the frontend silently dropped them (noworkflow_stepcase inuseSSE.ts, no iteration fields inDagNodeState).loop_iteration_*data pipeline end-to-end — SSE bridge through store to UI components — adding live iteration badges on graph nodes and an expandable per-iteration sub-list in the Logs tab sidebar.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
workflow-bridge.tsworkflow_steppayloadnodeIdfield to all 3 loop iteration casesuseSSE.tsworkflow-store.tshandleLoopIterationworkflow_stepcase dispatches to storeworkflow-store.tsDagNodeState.iterationsWorkflowExecution.tsxDagNodeState(REST path)loop_iteration_*eventsDagNodeStateDagNodeProgress.tsxDagNodeStateExecutionDagNode.tsxWorkflowDagViewer.tsxExecutionDagNode.tsxcurrentIteration/maxIterationsto node dataLabel Snapshot
risk: lowsize: Mserver,webserver:workflow-bridge,web:workflow-store,web:useSSE,web:DagNodeProgress,web:ExecutionDagNodeChange Metadata
featurewebLinked Issue
Validation Evidence (required)
bun run type-check: All 9 packages pass (git, paths, isolation, workflows, core, cli, adapters, web, server)bun run lint --max-warnings 0: 0 errors, 0 warningsbun run format:check: All files formatted correctlybun run test: All packages pass, 0 failuresOne deviation from the plan: the
data as LoopIterationEventcast inuseSSE.tswas removed after ESLint flagged it as unnecessary — the switch-case discriminant already narrowsdatatoLoopIterationEventautomatically.Security Impact (required)
Compatibility / Migration
loop_iteration_*events display normally; new UI paths are guarded bynode.iterations?.length > 0remote_agent_workflow_events.dataHuman Verification (required)
bun run validatesuite (type-check, lint, format, tests) passes with exit 0nodeIdmissing on SSE event (early return in store),total: 0on completed events (preservesmaxIterationsfrom priorstartedevent), non-loop nodes unaffectedSide Effects / Blast Radius (required)
loop:nodes in the Web UI onlyiterationsdataLoopIterationInfoshape consistency across store, REST extraction, and componentsRollback Plan (required)
git revert HEAD— all changes are purely additive (new fields, new cases, new rendering) with no schema changesloop_iteration_*eventsworkflow_stepSSE events silently dropped (same behavior as before the fix)Risks and Mitigations
e.stopPropagation()on the expand button prevents triggeringonNodeClickworkflow_stepSSE events for non-DAG loops (nonodeId) causing store errorshandleLoopIterationearly-returns whenevent.nodeIdis falsySummary by CodeRabbit
New Features
Bug Fixes