Repository navigation
fix(wework): show tool call details - #2382
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesTool detail display
Estimated code review effort: 4 (Complex) | ~45 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 2897-2921: Replace the direct Node REPL and generic MCP block
fixtures in the runtime-work index with an existing desktop-runner flow that
makes a real backend request producing both activities. Update the scenario
assertions to verify those backend-generated tool rows are serialized and
delivered to the desktop UI, while preserving the existing tool ordering and
detail checks.
- Around line 2988-2993: Update the ordering assertion around the existing
metrics reads to retrieve metrics for all four tool rows: earlier, nodeRepl,
genericMcp, and later. Assert their vertical positions are strictly
chronological as earlier.top < nodeRepl.top < genericMcp.top < later.top,
replacing the current pairwise check while preserving the existing failure
context.
🪄 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: 47ee769e-bb77-4a3a-b3a2-0ef378a897d0
📒 Files selected for processing (10)
docs/en/wework/tasks.mddocs/zh/wework/tasks.mdwework/e2e/desktop/task-flow.e2e.mjswework/src/components/chat/blocks/ToolBlockItem.test.tsxwework/src/components/chat/blocks/ToolBlockItem.tsxwework/src/components/chat/blocks/toolBlockActivity.test.tswework/src/components/chat/blocks/toolBlockActivity.tswework/src/components/chat/blocks/toolBlockKinds.tswework/src/i18n/locales/en/chat.jsonwework/src/i18n/locales/zh-CN/chat.json
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wework/e2e/desktop/task-flow.e2e.mjs (1)
2476-2483: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStop the cleanup release from masking the original failure.
If the
tryblock throws before line 2414, thisfinallyrunswithTimeout(control.releaseRequestUserInputResponse(), …). If the held SSE response never completes, that await rejects with the timeout message and replaces the original error. The E2E then reports "Timed out releasing the priority-filter request-user-input response" instead of the real cause.Catch and log the cleanup failure so the original error propagates.
🛡️ Proposed fix
} finally { if (!requestInputResponseReleased) { - await withTimeout( - control.releaseRequestUserInputResponse(), - DEFAULT_STEP_TIMEOUT_MS, - 'Timed out releasing the priority-filter request-user-input response' - ) + try { + await withTimeout( + control.releaseRequestUserInputResponse(), + DEFAULT_STEP_TIMEOUT_MS, + 'Timed out releasing the priority-filter request-user-input response' + ) + } catch (releaseError) { + console.warn( + `[desktop-e2e] priority-filter cleanup release failed: ${String(releaseError)}` + ) + } } }As per coding guidelines, "Diagnose problems using logs, actual code, and other concrete evidence first."
🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 2476 - 2483, Update the finally cleanup around control.releaseRequestUserInputResponse() to catch and log any withTimeout failure instead of allowing it to reject from finally. Preserve the original try-block error by completing cleanup best-effort while retaining the existing requestInputResponseReleased guard and timeout configuration.Source: Coding guidelines
🧹 Nitpick comments (2)
wework/e2e/desktop/task-flow.e2e.mjs (2)
2933-3016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated tool-detail expansion sequence into a helper.
Lines 2933-2968 and lines 2982-3017 are the same 35-line sequence. They differ only in the block selector, the row label, the three expected strings, and the screenshot name. The duplication makes both paths drift independently when the detail markup changes.
Extract one helper and call it twice.
♻️ Proposed helper
async function verifyExpandedToolDetail(control, { selector, detailText, inputText, outputText, screenshotName, }) { await control.command('click', `${selector} [data-tool-detail-toggle]`) await control.command('waitFor', `${selector} [data-testid="generic-tool-block-detail"]`, { text: detailText, visible: true, stableMs: 500, timeoutMs: DEFAULT_STEP_TIMEOUT_MS, }) await control.command('waitFor', `${selector} [data-testid="generic-tool-input"]`, { text: inputText, visible: true, timeoutMs: DEFAULT_STEP_TIMEOUT_MS, }) await control.command('waitFor', `${selector} [data-testid="generic-tool-output"]`, { text: outputText, visible: true, timeoutMs: DEFAULT_STEP_TIMEOUT_MS, }) await control.command('waitFor', `${selector} [data-tool-detail-toggle][aria-expanded="true"]`, { stableMs: 250, timeoutMs: DEFAULT_STEP_TIMEOUT_MS, }) await captureVerificationScreenshot(control, screenshotName, selector) await control.command('click', `${selector} [data-tool-detail-toggle]`) }As per coding guidelines, "extract shared logic instead of duplicating it."
🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 2933 - 3016, The tool-detail expansion and verification flow is duplicated for the node REPL and generic MCP blocks. Extract it into a shared verifyExpandedToolDetail helper near the existing test helpers, parameterized by selector, detailText, inputText, outputText, and screenshotName, then replace both inline sequences with calls using their respective values while preserving the existing release and output-observation flow.Source: Coding guidelines
482-515: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the standalone-mode list into a shared predicate.
shouldConfigureToolDetailsMcpinlines a 28-entry inverted allow-list of*_ONLYflags. Add a module-level predicate that excludes the full flow by reusing the existing active-only-mode grouping, or defineRUNS_STANDALONE_MODEexplicitly, and use it for this check to avoid new focused modes silently spawning extra MCP subprocesses.🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 482 - 515, The standalone-mode detection is duplicated inline in shouldConfigureToolDetailsMcp, risking newly added focused modes being omitted. Extract the complete *_ONLY grouping into a module-level shared predicate or explicit RUNS_STANDALONE_MODE value, then reuse it in shouldConfigureToolDetailsMcp while preserving the TOOL_BLOCK_ORDER_ONLY override and rendering-check behavior.Source: Coding guidelines
🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 8145-8148: Update the Node REPL MCP fixture response in
tool-details-mcp-server.mjs to use a distinguishing value absent from the
tool-call input, then update the corresponding model-server assertion in
task-flow.e2e.mjs and the matching UI output assertion to verify that value.
Keep the existing delivery verification and failure messages intact.
- Around line 2926-2932: Replace the fixed locale-specific text assertions in
the Node REPL and MCP tool-row waitFor calls with the existing locale-aware
helper or a stable data-testid. Apply this to
wework/e2e/desktop/task-flow.e2e.mjs lines 2926-2932, 2976-2981, 3038-3043, and
3044-3049, preserving the existing selector, visibility, stability, and timeout
options.
- Around line 3062-3067: Update the chronological screenshot capture in the
tool-block ordering flow to target the expanded processing-timeline container
instead of processing-live-preview. Keep the existing scrollIntoView call,
screenshot filename, and ordering verification behavior unchanged.
In `@wework/e2e/utils/tool-details-mcp-server.mjs`:
- Around line 78-80: Update the line handler around JSON.parse in the MCP
fixture to skip blank or whitespace-only lines, catch malformed JSON, and report
parse failures to stderr without terminating the server. Validate the parsed
value before using the `'id' in request` check so null or non-object results are
ignored safely, while preserving handling for valid requests.
---
Outside diff comments:
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 2476-2483: Update the finally cleanup around
control.releaseRequestUserInputResponse() to catch and log any withTimeout
failure instead of allowing it to reject from finally. Preserve the original
try-block error by completing cleanup best-effort while retaining the existing
requestInputResponseReleased guard and timeout configuration.
---
Nitpick comments:
In `@wework/e2e/desktop/task-flow.e2e.mjs`:
- Around line 2933-3016: The tool-detail expansion and verification flow is
duplicated for the node REPL and generic MCP blocks. Extract it into a shared
verifyExpandedToolDetail helper near the existing test helpers, parameterized by
selector, detailText, inputText, outputText, and screenshotName, then replace
both inline sequences with calls using their respective values while preserving
the existing release and output-observation flow.
- Around line 482-515: The standalone-mode detection is duplicated inline in
shouldConfigureToolDetailsMcp, risking newly added focused modes being omitted.
Extract the complete *_ONLY grouping into a module-level shared predicate or
explicit RUNS_STANDALONE_MODE value, then reuse it in
shouldConfigureToolDetailsMcp while preserving the TOOL_BLOCK_ORDER_ONLY
override and rendering-check behavior.
🪄 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: 666736f4-15ec-4cdc-9916-8162c52a870e
📒 Files selected for processing (2)
wework/e2e/desktop/task-flow.e2e.mjswework/e2e/utils/tool-details-mcp-server.mjs
| assert.ok( | ||
| requestText.includes("{ status: 'ready', value: 42 }"), | ||
| 'The Node REPL MCP server result was not delivered to the model service' | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This assertion passes without the MCP result and must use a distinguishing string.
The Node REPL tool call sends code: "nodeRepl.write({ status: 'ready', value: 42 })" (line 8124). requestText is the serialized request body, which echoes that tool-call argument back to the model server. The argument string already contains the substring { status: 'ready', value: 42 }.
The assertion therefore succeeds even when the MCP server result is missing or wrong. The failure message claims it verifies delivery, but it does not.
The generic MCP equivalent at lines 8175-8178 is correct, because Tool detail verification appears only in the fixture response.
Assert on the tool output item content, or make the fixture return a string that does not appear in the input.
🐛 Proposed fix
Change the fixture response in wework/e2e/utils/tool-details-mcp-server.mjs to a distinguishing value:
if (serverName === 'node_repl' && name === 'js') {
return {
- content: [{ type: 'text', text: "{ status: 'ready', value: 42 }" }],
+ content: [{ type: 'text', text: "{ status: 'node-repl-executed', value: 42 }" }],
isError: false,
}
}Then update the model-server assertion:
assert.ok(
- requestText.includes("{ status: 'ready', value: 42 }"),
+ requestText.includes("{ status: 'node-repl-executed', value: 42 }"),
'The Node REPL MCP server result was not delivered to the model service'
)Update the matching UI output assertion at line 2950 to the same string.
🤖 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 `@wework/e2e/desktop/task-flow.e2e.mjs` around lines 8145 - 8148, Update the
Node REPL MCP fixture response in tool-details-mcp-server.mjs to use a
distinguishing value absent from the tool-call input, then update the
corresponding model-server assertion in task-flow.e2e.mjs and the matching UI
output assertion to verify that value. Keep the existing delivery verification
and failure messages intact.
Summary
node_repl.jscalls as commands and show them as JavaScript activity instead of the generic "Call js" labelVerification
pnpm --filter wework test(265 files, 2602 tests)pnpm --filter wework exec node e2e/desktop/task-flow.e2e.mjs --tool-block-order-onlySummary by CodeRabbit
New Features
Documentation
Tests