Repository navigation
feat: polish subagent activity workspace - #936
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds native Codex subagent lifecycle handling, persists bounded subagent metadata, models live and hydrated lifecycle narratives, introduces a detailed Subagents panel, and supports subagent-scoped cumulative diffs and navigation. ChangesNative subagent lifecycle and persistence
Web subagent experience
Guidance updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/stores/diffStore.ts (1)
591-609: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the subagent review scope when
"changes"is activated for the same thread.
setRightPanelTabdeletessubagentReviewScopeByThread[threadId]whenever"changes"is activated, including after a refocus. The subagent panel code sets the scope and then callssetRightPanelTab(..., "changes"), so opening/switching back to the Changes tab can silently clear the scope that was just selected. Keep the stored scope intact instead of deleting it on"changes"activation.🤖 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 `@apps/web/src/stores/diffStore.ts` around lines 591 - 609, Update setRightPanelTab to stop deleting subagentReviewScopeByThread[threadId] when the "changes" tab is activated. Preserve the stored review scope across opening and refocusing the Changes tab, while leaving the existing openTabs and activeTab updates unchanged.
🧹 Nitpick comments (3)
apps/web/src/components/chat/narrative/build-narrative.ts (1)
292-308: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParticipants/children/hooks recomputed per lifecycle row instead of once per agent.
For an agent with multiple lifecycle events (started + N "updated" markers + finished),
subagentLifecycleParticipants(an O(n).findovertoolCalls), thechildrenfilter, and thehooksfilter are all recomputed identically for every row of the sameevt.call. Sincechildren/hooksare currently unused bySubagentRow(it only readsparticipants/lifecycle), this work is pure waste;participantsis used but is identical for every row of the same agent handoff.Hoist these three values into a per-agent lookup (e.g. compute once when first encountering each
evt.call.id, cache in aMap) and reuse across all lifecycle rows for that agent.As per coding guidelines, "Validate and normalize once at process or trust boundaries, then rely on established invariants inside hot paths and loops; do not repeatedly re-parse, re-check, or re-clamp values" and "Bound all work and retention."
🤖 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 `@apps/web/src/components/chat/narrative/build-narrative.ts` around lines 292 - 308, In the narrative-building flow around the subagent event branch, cache the derived participants, children, and hooks per evt.call.id in a bounded per-agent Map, computing them only on the first lifecycle row for each agent. Reuse the cached values for subsequent rows while preserving the existing filtering and subagentLifecycleParticipants behavior.Source: Coding guidelines
apps/web/src/components/chat/narrative/build-persisted-narrative.ts (2)
219-264: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
recordToToolCallis invoked redundantly for the same records.For every top-level Agent record,
recordToToolCall(t)runs in thetopLevelloop (Line 219) and its result is discarded whent.tool_name === AGENT_TOOL_NAME; it then runs again for the same record inside theagentRecordsloop (Line 226), and a third time viaallToolCalls = tools.map(recordToToolCall)(Line 264) for every tool record including these Agent records. Consider building oneMap<string, ToolCall>(or a single pass) and reusing it fortopLevel,agentRecords,lifecycleMarkers, andallToolCallsinstead of recomputing.As per coding guidelines, "Validate and normalize once at process or trust boundaries, then rely on established invariants inside hot paths and loops; do not repeatedly re-parse, re-check, or re-clamp values."
♻️ Sketch of the fix
- for (const t of topLevel) { - const call = recordToToolCall(t); - if (t.tool_name !== AGENT_TOOL_NAME) { - timeline.push({ kind: "tool", call, sortOrder: t.sort_order }); - } - } - const agentRecords = tools.filter((tool) => tool.tool_name === AGENT_TOOL_NAME); - for (const t of agentRecords) { - const call = recordToToolCall(t); + const callById = new Map(tools.map((t) => [t.id, recordToToolCall(t)])); + for (const t of topLevel) { + if (t.tool_name !== AGENT_TOOL_NAME) { + timeline.push({ kind: "tool", call: callById.get(t.id)!, sortOrder: t.sort_order }); + } + } + const agentRecords = tools.filter((tool) => tool.tool_name === AGENT_TOOL_NAME); + for (const t of agentRecords) { + const call = callById.get(t.id)!;Then reuse
callByIdformarker: recordToToolCall(marker)and replaceconst allToolCalls = tools.map(recordToToolCall);with[...callById.values()].🤖 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 `@apps/web/src/components/chat/narrative/build-persisted-narrative.ts` around lines 219 - 264, Eliminate redundant recordToToolCall conversions in the narrative-building flow by creating a single callById Map for all tool records before the topLevel and agentRecords loops. Reuse the cached ToolCall for top-level tools, agent subagent entries, lifecycle marker fields, and replace allToolCalls mapping with the cached values; preserve existing ordering and filtering behavior.Source: Coding guidelines
295-314: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winChildren/participants/hooks recomputed per lifecycle row instead of once per agent.
Same pattern as
build-narrative.ts: for every lifecycle row of the sameevt.call(started, each "updated" marker, finished),childRecords/childrenare re-filtered/sorted/mapped,subagentLifecycleParticipantsre-scansallToolCalls, andhooksis re-filtered — all producing identical results per agent.children/hooksare currently unused bySubagentRow, so this is pure waste on the hot narrative-build path.As per coding guidelines, "Bound all work and retention... do not repeatedly re-parse, re-check, or re-clamp values."
🤖 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 `@apps/web/src/components/chat/narrative/build-persisted-narrative.ts` around lines 295 - 314, In the subagent handling within the narrative builder, compute each agent’s children, lifecycle participants, and hooks once per unique evt.call.id rather than recomputing them for every lifecycle row. Reuse the cached derived values when constructing each subagent item, preserving the existing filtering, sorting, mapping, and participant-selection 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.
Outside diff comments:
In `@apps/web/src/stores/diffStore.ts`:
- Around line 591-609: Update setRightPanelTab to stop deleting
subagentReviewScopeByThread[threadId] when the "changes" tab is activated.
Preserve the stored review scope across opening and refocusing the Changes tab,
while leaving the existing openTabs and activeTab updates unchanged.
---
Nitpick comments:
In `@apps/web/src/components/chat/narrative/build-narrative.ts`:
- Around line 292-308: In the narrative-building flow around the subagent event
branch, cache the derived participants, children, and hooks per evt.call.id in a
bounded per-agent Map, computing them only on the first lifecycle row for each
agent. Reuse the cached values for subsequent rows while preserving the existing
filtering and subagentLifecycleParticipants behavior.
In `@apps/web/src/components/chat/narrative/build-persisted-narrative.ts`:
- Around line 219-264: Eliminate redundant recordToToolCall conversions in the
narrative-building flow by creating a single callById Map for all tool records
before the topLevel and agentRecords loops. Reuse the cached ToolCall for
top-level tools, agent subagent entries, lifecycle marker fields, and replace
allToolCalls mapping with the cached values; preserve existing ordering and
filtering behavior.
- Around line 295-314: In the subagent handling within the narrative builder,
compute each agent’s children, lifecycle participants, and hooks once per unique
evt.call.id rather than recomputing them for every lifecycle row. Reuse the
cached derived values when constructing each subagent item, preserving the
existing filtering, sorting, mapping, and participant-selection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 087d9b6c-2a20-4ea2-a761-559bdbe92e0a
📒 Files selected for processing (55)
AGENTS.mdDESIGN.mdapps/server/drizzle/0024_smart_glorian.sqlapps/server/drizzle/0025_wise_night_nurse.sqlapps/server/drizzle/0026_furry_red_hulk.sqlapps/server/drizzle/meta/0024_snapshot.jsonapps/server/drizzle/meta/0025_snapshot.jsonapps/server/drizzle/meta/0026_snapshot.jsonapps/server/drizzle/meta/_journal.jsonapps/server/src/__tests__/database-schema-patches.test.tsapps/server/src/__tests__/tool-call-record-repo.test.tsapps/server/src/providers/codex/__tests__/codex-app-server-handshake.test.tsapps/server/src/providers/codex/__tests__/codex-event-mapper.test.tsapps/server/src/providers/codex/__tests__/codex-provider-subagent-turn.test.tsapps/server/src/providers/codex/codex-app-server.tsapps/server/src/providers/codex/codex-event-mapper.tsapps/server/src/providers/codex/codex-provider.tsapps/server/src/providers/codex/codex-types.tsapps/server/src/repositories/tool-call-record-repo.tsapps/server/src/services/__tests__/narrative-store.test.tsapps/server/src/services/narrative-store.tsapps/server/src/store/database.tsapps/server/src/store/schema.tsapps/web/src/__tests__/diffStore.test.tsapps/web/src/components/chat/ThreadOverview.branchless-pr.test.tsxapps/web/src/components/chat/ThreadOverview.tsxapps/web/src/components/chat/narrative/NarrativeFlow.tsxapps/web/src/components/chat/narrative/PersistedNarrative.tsxapps/web/src/components/chat/narrative/SubagentRow.tsxapps/web/src/components/chat/narrative/__tests__/SubagentRow.test.tsxapps/web/src/components/chat/narrative/__tests__/build-narrative-counts.test.tsapps/web/src/components/chat/narrative/__tests__/parallel-subagent-nesting.test.tsapps/web/src/components/chat/narrative/build-narrative.tsapps/web/src/components/chat/narrative/build-persisted-narrative.test.tsapps/web/src/components/chat/narrative/build-persisted-narrative.tsapps/web/src/components/chat/narrative/subagent-lifecycle.tsapps/web/src/components/chat/narrative/types.tsapps/web/src/components/diff/CumulativeView.tsxapps/web/src/components/diff/DiffPanel.tsxapps/web/src/components/diff/__tests__/CumulativeView.test.tsxapps/web/src/components/diff/__tests__/DiffPanel.files.test.tsxapps/web/src/components/panels/SubagentChangeSummary.tsxapps/web/src/components/panels/SubagentsPanel.tsxapps/web/src/components/panels/__tests__/SubagentsPanel.test.tsxapps/web/src/components/subagents/SubagentIdentityGlyph.tsxapps/web/src/components/subagents/SubagentLifecycleStatus.tsxapps/web/src/components/subagents/__tests__/subagent-projection.test.tsapps/web/src/components/subagents/subagent-projection.tsapps/web/src/index.cssapps/web/src/lib/__tests__/open-subagent-detail.test.tsapps/web/src/lib/open-subagent-detail.tsapps/web/src/stores/diffStore.tsdocs/guides/ui-components.mdpackages/contracts/src/index.tspackages/contracts/src/models/tool-call-record.ts
60e7e57 to
410ac50
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 3 minutes. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
Polish the thread-scoped subagent experience across chat, Thread Overview, and the maximized Subagents workspace.
Why
Delegated work needs a clear lifecycle in the parent thread and a focused place to inspect each subagent without mixing its internal events into the main conversation.
Key Changes
Verification
bun run verifyexceeded the local 60-second command window and remains for CI.Related to #916
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
New Features
Bug Fixes