Skip to content

fix(e2e): use role strings in agent-centric workflow badge assertions#996

Open
lsm wants to merge 3 commits intodevfrom
task/fix-space-agent-centric-workflow-e2e-assertion
Open

fix(e2e): use role strings in agent-centric workflow badge assertions#996
lsm wants to merge 3 commits intodevfrom
task/fix-space-agent-centric-workflow-e2e-assertion

Conversation

@lsm
Copy link
Copy Markdown
Owner

@lsm lsm commented Mar 26, 2026

Fix agent badge assertions in space-agent-centric-workflow.e2e.ts Test 3 to use ROLE_A/ROLE_B ("coder"/"reviewer") instead of AGENT_A_NAME/AGENT_B_NAME ("Coder Agent"/"Reviewer Agent").

WorkflowNode.tsx renders sa.name which is the role, not the display name. The companion test space-multi-agent-editor.e2e.ts already uses the correct constants.

Copy link
Copy Markdown
Owner Author

@lsm lsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by claude-sonnet-4-6 (Anthropic)

Model: claude-sonnet-4-6 | Client: NeoKai | Provider: Anthropic

Verdict: APPROVE (with notes)

The fix is correct and minimal. renders (line 410) which is the role string (e.g. 'coder'/'reviewer'), not the display name (e.g. 'Coder Agent'/'Reviewer Agent'). The companion test already uses / for these assertions (lines 153-154), confirming the correct pattern. The 4 changed assertions are all that was needed.

Remaining constants are correctly used

  • / for / (dropdown option text): correct, the UI renders 'Coder Agent (coder)'
  • / passed to RPC: correct, these set the agent display name

Note on test failure

The test run shows a pre-existing failure at line 123 ( workflow nodes), which is completely unrelated to this PR. That line is in test 1 and was not modified by this PR — it fails because produces 2 non-task-agent nodes when the test expects 1. That is a separate bug to investigate.

Copy link
Copy Markdown
Owner Author

@lsm lsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by claude-opus-4-6 (Anthropic)

Model: claude-opus-4-6 | Client: NeoKai | Provider: Anthropic

Summary

The e2e fix is correct and well-targeted. WorkflowNode.tsx renders sa.name (the role string, e.g. "coder") as badge text, not the display name ("Coder Agent"). The 4 changed assertions in Test 3 now correctly use ROLE_A/ROLE_B matching the pattern in space-multi-agent-editor.e2e.ts.

P1 — PR description does not match the commit content

The commit c471637e4 only changes the e2e test file, but the commit message and PR description make no mention of the SDK mock addition in packages/daemon/tests/unit/room/room-runtime-service-worker-mcp.test.ts. That change appears to be from commit 4f9f5cb80 (already merged to dev). The PR description should accurately reflect all changes being proposed, or unrelated changes should be split into separate PRs.

P3 — Minor dead code in the changed file

AGENT_A_NAME and AGENT_B_NAME are no longer used in any assertion after this change. They are still used as substrings of AGENT_A_OPTION/AGENT_B_OPTION for the setupMultiAgentStep calls (which pass labels like "Coder Agent (coder)"), so they are not truly dead — just no longer directly asserted. No action needed, but worth noting.

Verification

  • WorkflowNode.tsx line 410 renders {sa.name} in badge spans
  • WorkflowNode.test.tsx line 275-276 explicitly comments "Canvas shows slot role names (not agent names)" and asserts .toContain('coder') / .toContain('reviewer')
  • space-multi-agent-editor.e2e.ts lines 153-154 already uses ROLE_A/ROLE_B for the same badge assertions
  • The 4 changed assertions (text=${ROLE_A} / text=${ROLE_B}) match what the component actually renders

Recommendation

APPROVE — the core fix is correct. The P1 about the incomplete PR description is a process concern, not a code quality concern with the actual fix.

@lsm lsm force-pushed the task/fix-space-agent-centric-workflow-e2e-assertion branch from c471637 to 738c6d5 Compare March 26, 2026 21:09
lsm added 2 commits March 26, 2026 19:48
…sertions

The WorkflowNode component renders sa.name which is the role (e.g. "coder"),
not the display name (e.g. "Coder Agent"). The companion test file
space-multi-agent-editor.e2e.ts already uses ROLE_A/ROLE_B correctly.
Move setStartStepId inside the setNodes callback so React StrictMode's
double-invoke of state updaters doesn't add two nodes per click. Also
wrap addStep in useCallback for a stable identity.
@lsm lsm force-pushed the task/fix-space-agent-centric-workflow-e2e-assertion branch from 513355f to a6eb0d4 Compare March 26, 2026 23:48
The visual editor canvas renders asynchronously. Without waiting for the
add-step button to be present, clicks can race with the initial render.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant