Repository navigation
fix(wework): isolate codex subagent stream state - #1645
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds end-to-end subagent isolation: the Codex executor now tracks a ChangesExecutor: Root-thread and subagent filtering
Frontend: Subagent activity stream and status UI
Sequence DiagramsequenceDiagram
participant Codex as Codex RPC
participant HM as CodexRunState.handle_message
participant EM as CodexNotificationEventMapper
participant RS as responseApiStream
participant Hook as useWorkbenchPaneSession
participant UI as SubagentStatusIndicator
Codex->>HM: thread/started {threadId}
HM->>HM: set root_thread_id
Codex->>HM: item/started {subagent item}
HM->>HM: is_subagent_message? → ignore
Codex->>EM: subagent/activity notification
EM->>RS: response.subagent.activity event
RS->>Hook: onSubagentActivity(payload)
Hook->>Hook: updateRuntimeSubagentStatuses
Hook->>UI: subagentStatuses updated → re-render
Codex->>HM: turn/completed {root agent}
HM->>HM: is_root_codex_turn_event? → emit ExecutionOutcome
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 8
🧹 Nitpick comments (2)
executor/src/agents/codex.rs (1)
781-813: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating the duplicated root-detection helpers into a shared module.
codex_agent_pathandis_root_codex_turn_eventare now defined independently inagents/codex.rs,runtime_work/codex_notifications.rs, andruntime_work/codex_rollout.rs(withstream_thread_id/is_root_turn_markerrelated). They already diverge subtly — e.g.is_root_codex_turn_eventchecks theturnobject first here but the top-levelparamsfirst incodex_notifications.rs. Functionally equivalent today, but the copies can drift if the "root" definition changes. Extracting them into onepub(crate)helper would keep the subagent contract single-sourced.🤖 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 `@executor/src/agents/codex.rs` around lines 781 - 813, Consolidate the duplicated root-detection logic so the “root” contract is defined once and reused across codex-related modules. Move the shared behavior behind a single pub(crate) helper for symbols like codex_agent_path and is_root_codex_turn_event (and the related stream_thread_id/is_root_turn_marker path checks) and update agents/codex.rs, runtime_work/codex_notifications.rs, and runtime_work/codex_rollout.rs to call that helper instead of maintaining separate copies. Keep the helper’s precedence/field-normalization rules consistent so the turn-vs-params lookup cannot drift between implementations.wework/src/stream/responseApiStream.test.ts (1)
5-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the non-happy-path normalization branches.
This only asserts the snake_case happy path.
emitSubagentActivityalso accepts camelCase aliases and drops payloads withoutagent_path; those are the branches most likely to regress.Suggested test additions
+ test('emits camelCase subagent activity payloads', () => { + const onSubagentActivity = vi.fn() + + emitResponseApiEvent( + { onSubagentActivity }, + 'response.subagent.activity', + { + task_id: 1, + subtask_id: 2, + device_id: 'device-1', + local_task_id: 'local-1', + data: { + agentPath: '/root/worker', + agentName: 'worker', + agentThreadId: 'thread-1', + kind: 'started', + status: 'running', + occurredAtMs: 12345, + }, + }, + createResponseApiStreamState() + ) + + expect(onSubagentActivity).toHaveBeenCalledWith( + expect.objectContaining({ + agent_path: '/root/worker', + agent_name: 'worker', + agent_thread_id: 'thread-1', + occurred_at_ms: 12345, + }) + ) + }) + + test('ignores subagent activity payloads without agent_path', () => { + const onSubagentActivity = vi.fn() + + emitResponseApiEvent( + { onSubagentActivity }, + 'response.subagent.activity', + { + task_id: 1, + subtask_id: 2, + device_id: 'device-1', + local_task_id: 'local-1', + data: { kind: 'started', status: 'running' }, + }, + createResponseApiStreamState() + ) + + expect(onSubagentActivity).not.toHaveBeenCalled() + })As per coding guidelines, "Run tests before committing and follow the project’s testing principles: AAA, mock external services, test edge cases, and keep tests isolated."
🤖 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/src/stream/responseApiStream.test.ts` around lines 5 - 40, Expand the emitSubagentActivity coverage in responseApiStream.test.ts to include the normalization edge cases: add a test for camelCase input aliases being mapped correctly by emitResponseApiEvent, and add a test that verifies payloads missing agent_path are dropped and do not call onSubagentActivity. Use the existing emitResponseApiEvent and createResponseApiStreamState helpers so the new tests stay isolated and focused on the normalization branches.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 `@executor/src/runtime_work/handler.rs`:
- Around line 1844-1852: The local_task_finished helper in handler.rs does not
treat archived tasks as finished, so archived transcripts can still appear
running. Update local_task_finished to recognize the archived status alongside
the existing done/complete/failed/error/cancelled/canceled values, while keeping
the running flag check intact. Use the local_task_finished function and the
status normalization logic in RuntimeTaskLink to locate and adjust the matching
list.
In `@executor/src/runtime_work/runtime_handle_messages.rs`:
- Around line 176-179: The turn completion handling in handle_messages should
also be gated by thread identity, not just agent_path, because a child
turn/completed event with only threadId can still complete the root cached
assistant message once root_thread_id has been seen. Update the matching logic
around is_root_codex_turn_event and the turn/completed branch to require the
event’s threadId to match the root thread before calling
complete_runtime_assistant_message, and keep the missing-path behavior in the
root-detection logic consistent with that same rule.
- Around line 898-913: The item-specific delta check in
process_block_accepts_delta_for_item bypasses the terminal-state guard when
process_item_id is Some, which can allow late updates to done/failed blocks.
Update the Some(item_id) branch to still validate the block with
process_block_accepts_delta after matching type, process_kind, and
process_item_id, so item-matched deltas are rejected for terminal blocks just
like the None path.
In `@wework/src/components/layout/DesktopWorkbenchMain.tsx`:
- Around line 770-775: The right-side summary in DesktopWorkbenchMain is
receiving chatColumnWidth, which can be a percentage string when the panel is
closed and prevents SubagentStatusIndicator from auto-expanding. Update the
right prop logic to pass a numeric width in the full-width desktop layout, using
the existing chatColumnWidth computation or a derived numeric value from
DesktopWorkbenchMain/paneSession so SubagentStatusIndicator gets a number
instead of '100%'. Keep the fix localized to the SubagentStatusIndicator usage
and preserve the existing statuses prop wiring.
In `@wework/src/components/layout/SubagentStatusIndicator.test.tsx`:
- Around line 17-39: The SubagentStatusIndicator test suite only covers the
desktop auto-expand and hover flow, so it misses the compact/tap behavior in the
component. Add a dedicated test for the compact variant of
SubagentStatusIndicator that renders it in compact mode and verifies the panel
opens and closes via click/tap interactions, using the existing test ids like
subagent-status-hover-region and subagent-status-panel to keep the coverage
aligned with the component behavior.
In `@wework/src/components/layout/SubagentStatusIndicator.tsx`:
- Around line 42-66: The SubagentStatusIndicator trigger is hover/focus-only, so
touch users can’t open or close the panel; add a real click toggle to the button
in SubagentStatusIndicator so panelVisible can change on tap as well as
hover/focus. Update the button handler alongside the existing hover/focus state
logic, and keep aria-expanded in sync with the same open/close state used by
MobileWorkbenchLayout.
In `@wework/src/i18n/locales/en/common.json`:
- Around line 75-76: The `subagents_running` and `subagents_count` strings in
the `common.json` locale currently use a single plural form, which makes
`SubagentStatusIndicator` render incorrect English when `count === 1`. Update
the locale entries to support proper singular/plural messaging for the summary
text, and make sure the templates used by `SubagentStatusIndicator` select the
singular form for 1 and the plural form otherwise.
In `@wework/src/stream/responseApiStream.ts`:
- Around line 385-393: Forward the missing agent_id through the subagent
activity mapping so the primary identifier is preserved. Update the
onSubagentActivity payload construction in responseApiStream.ts to read agent_id
from the incoming data, alongside agent_name, agent_thread_id, kind, status, and
occurred_at_ms, and include it in the object passed through base so
RuntimeSubagentActivityPayload reaches the session/UI layer intact.
---
Nitpick comments:
In `@executor/src/agents/codex.rs`:
- Around line 781-813: Consolidate the duplicated root-detection logic so the
“root” contract is defined once and reused across codex-related modules. Move
the shared behavior behind a single pub(crate) helper for symbols like
codex_agent_path and is_root_codex_turn_event (and the related
stream_thread_id/is_root_turn_marker path checks) and update agents/codex.rs,
runtime_work/codex_notifications.rs, and runtime_work/codex_rollout.rs to call
that helper instead of maintaining separate copies. Keep the helper’s
precedence/field-normalization rules consistent so the turn-vs-params lookup
cannot drift between implementations.
In `@wework/src/stream/responseApiStream.test.ts`:
- Around line 5-40: Expand the emitSubagentActivity coverage in
responseApiStream.test.ts to include the normalization edge cases: add a test
for camelCase input aliases being mapped correctly by emitResponseApiEvent, and
add a test that verifies payloads missing agent_path are dropped and do not call
onSubagentActivity. Use the existing emitResponseApiEvent and
createResponseApiStreamState helpers so the new tests stay isolated and focused
on the normalization branches.
🪄 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: 6970ce5b-c133-42b4-a89c-34b2629ee8d2
📒 Files selected for processing (21)
executor/src/agents/codex.rsexecutor/src/runtime_work/codex_notifications.rsexecutor/src/runtime_work/codex_rollout.rsexecutor/src/runtime_work/events.rsexecutor/src/runtime_work/handler.rsexecutor/src/runtime_work/runtime_handle_messages.rsexecutor/src/runtime_work/transcript.rsexecutor/tests/process_manager_contract.rswework/src/components/layout/DesktopWorkbenchMain.tsxwework/src/components/layout/MobileWorkbenchLayout.tsxwework/src/components/layout/SubagentStatusIndicator.test.tsxwework/src/components/layout/SubagentStatusIndicator.tsxwework/src/components/layout/useWorkbenchPaneSession.tswework/src/features/workbench/runtimePaneMessages.tswework/src/i18n/locales/en/common.jsonwework/src/i18n/locales/zh-CN/common.jsonwework/src/stream/chatStream.tswework/src/stream/responseApiStream.test.tswework/src/stream/responseApiStream.tswework/src/types/api.tswework/src/types/workbench.ts
Comment on lines
+1844
to
+1852
| fn local_task_finished(link: &RuntimeTaskLink) -> bool { | ||
| !link.running | ||
| && matches!( | ||
| link.status | ||
| .replace(['_', '-'], "") | ||
| .to_ascii_lowercase() | ||
| .as_str(), | ||
| "done" | "complete" | "completed" | "failed" | "error" | "cancelled" | "canceled" | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat archived local tasks as finished.
archive_task sets status = "archived" and running = false, but this helper excludes "archived", so stale streaming messages can still make archived transcripts report running: true.
Suggested fix
- "done" | "complete" | "completed" | "failed" | "error" | "cancelled" | "canceled"
+ "done" | "complete" | "completed" | "failed" | "error" | "cancelled" | "canceled"
+ | "archived"📝 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
| fn local_task_finished(link: &RuntimeTaskLink) -> bool { | |
| !link.running | |
| && matches!( | |
| link.status | |
| .replace(['_', '-'], "") | |
| .to_ascii_lowercase() | |
| .as_str(), | |
| "done" | "complete" | "completed" | "failed" | "error" | "cancelled" | "canceled" | |
| ) | |
| fn local_task_finished(link: &RuntimeTaskLink) -> bool { | |
| !link.running | |
| && matches!( | |
| link.status | |
| .replace(['_', '-'], "") | |
| .to_ascii_lowercase() | |
| .as_str(), | |
| "done" | "complete" | "completed" | "failed" | "error" | "cancelled" | "canceled" | |
| | "archived" | |
| ) |
🤖 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 `@executor/src/runtime_work/handler.rs` around lines 1844 - 1852, The
local_task_finished helper in handler.rs does not treat archived tasks as
finished, so archived transcripts can still appear running. Update
local_task_finished to recognize the archived status alongside the existing
done/complete/failed/error/cancelled/canceled values, while keeping the running
flag check intact. Use the local_task_finished function and the status
normalization logic in RuntimeTaskLink to locate and adjust the matching list.
Comment on lines
+176
to
+179
| "turn/completed" if is_root_codex_turn_event(params) => { | ||
| complete_runtime_assistant_message(store, local_task_id, request) | ||
| } | ||
| "turn/completed" => {} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Also gate turn completion by thread id.
Line 176 only checks agent_path, while Line 286 treats missing paths as root. After root_thread_id is observed, a child turn/completed carrying only threadId will still mark the root cached assistant message done.
Suggested fix
- "turn/completed" if is_root_codex_turn_event(params) => {
+ "turn/completed" if is_root_codex_turn_event(params) && !self.is_subagent_thread(params) => {
complete_runtime_assistant_message(store, local_task_id, request)
}Also applies to: 280-286
🤖 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 `@executor/src/runtime_work/runtime_handle_messages.rs` around lines 176 - 179,
The turn completion handling in handle_messages should also be gated by thread
identity, not just agent_path, because a child turn/completed event with only
threadId can still complete the root cached assistant message once
root_thread_id has been seen. Update the matching logic around
is_root_codex_turn_event and the turn/completed branch to require the event’s
threadId to match the root thread before calling
complete_runtime_assistant_message, and keep the missing-path behavior in the
root-detection logic consistent with that same rule.
Comment on lines
+898
to
+913
| fn process_block_accepts_delta_for_item( | ||
| block: &Value, | ||
| block_type: &str, | ||
| process_kind: &str, | ||
| process_item_id: Option<&str>, | ||
| ) -> bool { | ||
| process_block_accepts_delta(block, block_type) | ||
| && string_field(block, "process_kind").as_deref() == Some(process_kind) | ||
| if string_field(block, "type").as_deref() != Some(block_type) | ||
| || string_field(block, "process_kind").as_deref() != Some(process_kind) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| match process_item_id { | ||
| Some(item_id) => string_field(block, "process_item_id").as_deref() == Some(item_id), | ||
| None => process_block_accepts_delta(block, block_type), | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Preserve the terminal-block guard for item-matched deltas.
When process_item_id is present, Line 911 bypasses process_block_accepts_delta, so a late delta can mutate a done/failed process block.
Suggested fix
- if string_field(block, "type").as_deref() != Some(block_type)
+ if !process_block_accepts_delta(block, block_type)
|| string_field(block, "process_kind").as_deref() != Some(process_kind)
{
return false;
}📝 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
| fn process_block_accepts_delta_for_item( | |
| block: &Value, | |
| block_type: &str, | |
| process_kind: &str, | |
| process_item_id: Option<&str>, | |
| ) -> bool { | |
| process_block_accepts_delta(block, block_type) | |
| && string_field(block, "process_kind").as_deref() == Some(process_kind) | |
| if string_field(block, "type").as_deref() != Some(block_type) | |
| || string_field(block, "process_kind").as_deref() != Some(process_kind) | |
| { | |
| return false; | |
| } | |
| match process_item_id { | |
| Some(item_id) => string_field(block, "process_item_id").as_deref() == Some(item_id), | |
| None => process_block_accepts_delta(block, block_type), | |
| } | |
| fn process_block_accepts_delta_for_item( | |
| block: &Value, | |
| block_type: &str, | |
| process_kind: &str, | |
| process_item_id: Option<&str>, | |
| ) -> bool { | |
| if !process_block_accepts_delta(block, block_type) | |
| || string_field(block, "process_kind").as_deref() != Some(process_kind) | |
| { | |
| return false; | |
| } |
🤖 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 `@executor/src/runtime_work/runtime_handle_messages.rs` around lines 898 - 913,
The item-specific delta check in process_block_accepts_delta_for_item bypasses
the terminal-state guard when process_item_id is Some, which can allow late
updates to done/failed blocks. Update the Some(item_id) branch to still validate
the block with process_block_accepts_delta after matching type, process_kind,
and process_item_id, so item-matched deltas are rejected for terminal blocks
just like the None path.
Comment on lines
+770
to
+775
| right={ | ||
| (paneSession.subagentStatuses?.length ?? 0) > 0 ? ( | ||
| <SubagentStatusIndicator | ||
| statuses={paneSession.subagentStatuses} | ||
| availableWidth={chatColumnWidth} | ||
| /> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass a numeric width here.
When the right panel is closed, chatColumnWidth is '100%', but SubagentStatusIndicator only auto-expands for numeric widths. In the full-width desktop layout the summary therefore stays collapsed unless the user hovers.
🤖 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/src/components/layout/DesktopWorkbenchMain.tsx` around lines 770 -
775, The right-side summary in DesktopWorkbenchMain is receiving
chatColumnWidth, which can be a percentage string when the panel is closed and
prevents SubagentStatusIndicator from auto-expanding. Update the right prop
logic to pass a numeric width in the full-width desktop layout, using the
existing chatColumnWidth computation or a derived numeric value from
DesktopWorkbenchMain/paneSession so SubagentStatusIndicator gets a number
instead of '100%'. Keep the fix localized to the SubagentStatusIndicator usage
and preserve the existing statuses prop wiring.
Comment on lines
+17
to
+39
| describe('SubagentStatusIndicator', () => { | ||
| test('expands automatically when title space is available', () => { | ||
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={900} />) | ||
|
|
||
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | ||
| expect(screen.getByText('worker')).toBeInTheDocument() | ||
| expect(screen.getByText('0ffa96e5')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| test('collapses when title space is constrained and opens on hover', () => { | ||
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={360} />) | ||
|
|
||
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | ||
|
|
||
| fireEvent.mouseEnter(screen.getByTestId('subagent-status-hover-region')) | ||
|
|
||
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | ||
|
|
||
| fireEvent.mouseLeave(screen.getByTestId('subagent-status-hover-region')) | ||
|
|
||
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the compact/tap path too.
This suite only exercises the desktop auto-expand/hover behavior, so it would not catch the mobile regression path introduced by rendering the indicator in compact mode. Add a click/tap test for the compact variant.
As per coding guidelines, **/*.{py,ts,tsx,js,jsx} should “test edge cases” and keep tests isolated.
Suggested test
describe('SubagentStatusIndicator', () => {
@@
test('collapses when title space is constrained and opens on hover', () => {
render(<SubagentStatusIndicator statuses={statuses} availableWidth={360} />)
@@
expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument()
})
+
+ test('opens in compact mode on tap', () => {
+ render(<SubagentStatusIndicator statuses={statuses} availableWidth={0} compact />)
+
+ fireEvent.click(screen.getByTestId('subagent-status-toggle-button'))
+
+ expect(screen.getByTestId('subagent-status-panel')).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
| describe('SubagentStatusIndicator', () => { | |
| test('expands automatically when title space is available', () => { | |
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={900} />) | |
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | |
| expect(screen.getByText('worker')).toBeInTheDocument() | |
| expect(screen.getByText('0ffa96e5')).toBeInTheDocument() | |
| }) | |
| test('collapses when title space is constrained and opens on hover', () => { | |
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={360} />) | |
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | |
| fireEvent.mouseEnter(screen.getByTestId('subagent-status-hover-region')) | |
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | |
| fireEvent.mouseLeave(screen.getByTestId('subagent-status-hover-region')) | |
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | |
| }) | |
| }) | |
| describe('SubagentStatusIndicator', () => { | |
| test('expands automatically when title space is available', () => { | |
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={900} />) | |
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | |
| expect(screen.getByText('worker')).toBeInTheDocument() | |
| expect(screen.getByText('0ffa96e5')).toBeInTheDocument() | |
| }) | |
| test('collapses when title space is constrained and opens on hover', () => { | |
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={360} />) | |
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | |
| fireEvent.mouseEnter(screen.getByTestId('subagent-status-hover-region')) | |
| expect(screen.getByTestId('subagent-status-panel')).toBeInTheDocument() | |
| fireEvent.mouseLeave(screen.getByTestId('subagent-status-hover-region')) | |
| expect(screen.queryByTestId('subagent-status-panel')).not.toBeInTheDocument() | |
| }) | |
| test('opens in compact mode on tap', () => { | |
| render(<SubagentStatusIndicator statuses={statuses} availableWidth={0} compact />) | |
| fireEvent.click(screen.getByTestId('subagent-status-toggle-button')) | |
| expect(screen.getByTestId('subagent-status-panel')).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 `@wework/src/components/layout/SubagentStatusIndicator.test.tsx` around lines
17 - 39, The SubagentStatusIndicator test suite only covers the desktop
auto-expand and hover flow, so it misses the compact/tap behavior in the
component. Add a dedicated test for the compact variant of
SubagentStatusIndicator that renders it in compact mode and verifies the panel
opens and closes via click/tap interactions, using the existing test ids like
subagent-status-hover-region and subagent-status-panel to keep the coverage
aligned with the component behavior.
Source: Coding guidelines
Comment on lines
+42
to
+66
| return ( | ||
| <div | ||
| data-testid="subagent-status-hover-region" | ||
| className={cn('relative flex shrink-0 items-center', className)} | ||
| onMouseEnter={() => setHovered(true)} | ||
| onMouseLeave={() => setHovered(false)} | ||
| onFocus={() => setHovered(true)} | ||
| onBlur={() => setHovered(false)} | ||
| > | ||
| <button | ||
| type="button" | ||
| data-testid="subagent-status-toggle-button" | ||
| className={cn( | ||
| 'flex h-8 max-w-[11rem] items-center gap-1.5 rounded-full border border-border/70 bg-surface px-2.5 text-xs text-text-secondary shadow-sm hover:bg-background', | ||
| compact && 'h-9 min-w-[44px] justify-center px-2' | ||
| )} | ||
| aria-label={t('workbench.subagents_status')} | ||
| aria-expanded={panelVisible} | ||
| > | ||
| <Bot className="h-4 w-4 shrink-0 text-primary" /> | ||
| {!compact && <span className="truncate">{summary}</span>} | ||
| <ChevronDown | ||
| className={cn('h-3.5 w-3.5 shrink-0 transition-transform', panelVisible && 'rotate-180')} | ||
| /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a real toggle for touch devices.
panelVisible only changes via hover/focus, but the trigger button never toggles anything on click. That makes the compact mobile usage in MobileWorkbenchLayout.tsx effectively non-interactive on touch-only devices.
Proposed fix
- const [hovered, setHovered] = useState(false)
+ const [expanded, setExpanded] = useState(false)
@@
- const panelVisible = autoExpanded || hovered
+ const panelVisible = autoExpanded || expanded
@@
<div
data-testid="subagent-status-hover-region"
className={cn('relative flex shrink-0 items-center', className)}
- onMouseEnter={() => setHovered(true)}
- onMouseLeave={() => setHovered(false)}
- onFocus={() => setHovered(true)}
- onBlur={() => setHovered(false)}
+ onMouseEnter={() => setExpanded(true)}
+ onMouseLeave={() => !compact && setExpanded(false)}
+ onFocus={() => setExpanded(true)}
+ onBlur={event => {
+ if (!event.currentTarget.contains(event.relatedTarget as Node | null)) {
+ setExpanded(false)
+ }
+ }}
>
<button
type="button"
data-testid="subagent-status-toggle-button"
+ onClick={() => setExpanded(value => !value)}
className={cn(
'flex h-8 max-w-[11rem] items-center gap-1.5 rounded-full border border-border/70 bg-surface px-2.5 text-xs text-text-secondary shadow-sm hover:bg-background',
compact && 'h-9 min-w-[44px] justify-center px-2'
)}
aria-label={t('workbench.subagents_status')}
aria-expanded={panelVisible}
+ aria-controls="subagent-status-panel"
>
@@
<div
+ id="subagent-status-panel"
data-testid="subagent-status-panel"
className="absolute right-0 top-[calc(100%+0.5rem)] z-popover w-64 rounded-lg border border-border/80 bg-background p-2 shadow-lg"
>🤖 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/src/components/layout/SubagentStatusIndicator.tsx` around lines 42 -
66, The SubagentStatusIndicator trigger is hover/focus-only, so touch users
can’t open or close the panel; add a real click toggle to the button in
SubagentStatusIndicator so panelVisible can change on tap as well as
hover/focus. Update the button handler alongside the existing hover/focus state
logic, and keep aria-expanded in sync with the same open/close state used by
MobileWorkbenchLayout.
Comment on lines
+75
to
+76
| "subagents_running": "{{count}} running", | ||
| "subagents_count": "{{count}} subagents", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid singular-count copy like 1 subagents.
SubagentStatusIndicator uses these templates directly for the summary text, so count === 1 currently renders incorrect English in the button label.
🤖 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/src/i18n/locales/en/common.json` around lines 75 - 76, The
`subagents_running` and `subagents_count` strings in the `common.json` locale
currently use a single plural form, which makes `SubagentStatusIndicator` render
incorrect English when `count === 1`. Update the locale entries to support
proper singular/plural messaging for the summary text, and make sure the
templates used by `SubagentStatusIndicator` select the singular form for 1 and
the plural form otherwise.
Comment on lines
+385
to
+393
| handlers.onSubagentActivity?.({ | ||
| ...base, | ||
| agent_path: agentPath, | ||
| agent_name: stringField(data, 'agent_name') ?? stringField(data, 'agentName'), | ||
| agent_thread_id: stringField(data, 'agent_thread_id') ?? stringField(data, 'agentThreadId'), | ||
| kind: stringField(data, 'kind'), | ||
| status: stringField(data, 'status'), | ||
| occurred_at_ms: | ||
| optionalNumberField(data, 'occurred_at_ms') ?? optionalNumberField(data, 'occurredAtMs'), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --glob 'wework/src/**' --glob 'executor/src/**' '\bagent_id\b|\bagentId\b|response\.subagent\.activity' \
wework/src/types/api.ts \
wework/src/stream/responseApiStream.ts \
wework/src/components/layout/useWorkbenchPaneSession.ts \
wework/src/components/layout/SubagentStatusIndicator.tsx \
executor/src/runtime_work/events.rsRepository: wecode-ai/Wegent
Length of output: 13983
Forward agent_id through this mapper. RuntimeSubagentActivityPayload already includes it, and the executor emits it on response.subagent.activity; omitting it here drops the primary subagent identifier before the session/UI layer can use 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/src/stream/responseApiStream.ts` around lines 385 - 393, Forward the
missing agent_id through the subagent activity mapping so the primary identifier
is preserved. Update the onSubagentActivity payload construction in
responseApiStream.ts to read agent_id from the incoming data, alongside
agent_name, agent_thread_id, kind, status, and occurred_at_ms, and include it in
the object passed through base so RuntimeSubagentActivityPayload reaches the
session/UI layer intact.
This was referenced Jul 3, 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
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 by CodeRabbit
New Features
Bug Fixes