Repository navigation
Fix: Render an expanded subagent block's rows together under its header - #1614
Conversation
An open block's member rows rendered at their own chronological positions, interleaved with the main conversation's — so expanding a header left the rows directly under it belonging to the main conversation, which read as the expansion doing nothing (rossoctl#1047 live check). The block's rows now render contiguously under the header, in arrival order within the block; the TIME and # columns still carry where each row sat, and the DIR gutter's │ still marks members. Blocks exist only in the chronological view (a sort disables grouping), so the sort-key path never sees a member row. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Block-local numbering and counts cmd/agentop/tui/events_blocks.go, cmd/agentop/tui/events_columns.go, cmd/agentop/tui/events_pane.go, cmd/agentop/tui/events_blocks_test.go |
Subagent exchanges receive block-local numbers, shared by each request and response. The # column uses those numbers for block rows and session-wide numbers for main rows. Headers report message counts. Tests cover numbering and counts. |
Grouped row rendering cmd/agentop/tui/events_pane.go, cmd/agentop/tui/events_blocks_test.go, CLAUDE.md |
Expanded block rows render together beneath each header; collapsed rows are omitted. Tests cover interleaved events and contiguous block rows. The documentation describes the grouping and numbering. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant events_pane
participant cellContext
participant column_renderers
participant event_table
participant search_results
events_pane->>cellContext: Attach block-local exchange numbers
events_pane->>column_renderers: Render grouped member rows
column_renderers-->>events_pane: Return row cells
events_pane->>event_table: Append header and grouped rows
events_pane->>search_results: Append rendered rows when search is active
Suggested labels: ready-for-ai-review
Suggested reviewers: mrsabath, esnible
Merge Risk: 
2d97f
-
Live follow can leave the detail pane on an older main event when new subagent activity arrives. The event remains available through manual navigation, so this is mergeable with a follow-up.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: rendering expanded subagent rows together beneath their header. Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 4 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Two subagents interleaved with each other and with the main conversation, one resuming long after it started: each block still renders its whole run contiguously under its own single header — membership is by subagent id over the whole session, not by time-contiguity, so interleaving cannot split a block or emit a second header for the same agent. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
The block reads as the subagent's own transcript, so its first exchange is 1 — the session-wide number the main rows keep would start a block at 94 and say nothing about how much the subagent itself did. A request and its response still share one number (the session-wide ids still decide what an exchange is; only the counting restarts per block), and the header's rollup counts the same unit (28 msgs, not 56 rows) so it agrees with the last number shown inside the block. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
The contiguity change does what it says. renderEventRow removes the duplicated cell-building path, member rows now render only from the header branch, and b.rows is the same inactive-filtered population the loop used to render, so nothing is double-counted. The sort path is unaffected because blocks exist only in the chronological view. One blocking issue: the block-local renumbering gives a request and its response different numbers when two of the same subagent's exchanges overlap. The # column exists to pair exchanges, so this breaks the invariant the PR description and the code comments both state. The fix is a few lines; details are inline.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI), Docs (CLAUDE.md)
Agent/IDE config (.claude/.vscode): none
Commits: 3 commits, all signed-off: yes
CI status: passing (all required checks green; CodeRabbit/Spellcheck skipped)
Security: no secrets, no new dependencies, no workflow/Dockerfile changes
Verified locally on 1dd7cfb (go 1.26.10): the cmd/agentop/tui package passes with -count=1, and go vet and gofmt are clean. A scratch test with overlapping exchanges in one block (below) reproduces the misnumbering, and the suggested fix produces 1,2,1,2.
| if !seen[ids[e]] { | ||
| seen[ids[e]] = true | ||
| } | ||
| blockIDs[e] = len(seen) |
There was a problem hiding this comment.
must-fix: blockIDs[e] = len(seen) assigns each row the count of distinct exchanges seen so far, not that exchange's own block-local number. It is only correct when one exchange in a block finishes before the next one starts.
When two exchanges from the same subagent overlap (which computeEventPairs and the PHASE note above explicitly allow for: A starts, B starts, A ends, B ends), you get:
req a1.1 → 1
req a1.2 → 2
resp a1.1 → 2 // should be 1
resp a1.2 → 2
The response of exchange 1 then shows as part of exchange 2, which is exactly the mis-pairing the # column is there to prevent. It also contradicts "a request and its response share one number there, and therefore here". I reproduced this with a scratch test (req a1.1, req a1.2, resp a1.1, resp a1.2, block expanded): the rows read 1,2,2,2.
Map each session id to the block-local number assigned the first time it is seen:
local := make(map[int]int, len(b.rows))
for _, j := range b.rows {
e := ctxs[j].row.event
n, ok := local[ids[e]]
if !ok {
n = len(local) + 1
local[ids[e]] = n
}
blockIDs[e] = n
}
b.msgs = len(local)With this change the same scratch test reads 1,2,1,2, and the full package stays green. (It also removes the if !seen[...] { seen[...] = true } guard, which was a no-op.)
| } | ||
| events := []pipeline.SessionEvent{ | ||
| req("m1", ""), resp("m1", ""), // session exchange 1 | ||
| req("a1.1", "a1"), resp("a1.1", "a1"), // session 2 — block-local 1 |
There was a problem hiding this comment.
suggestion: this fixture's in-block exchanges are strictly sequential (req, resp, req, resp), so it cannot catch the overlap case in events_pane.go. Please add an overlapping pair, e.g. req(a1.1), req(a1.2), resp(a1.1), resp(a1.2), and assert that the expanded # cells read 1,2,1,2. That pins the pairing invariant for block-local numbers the same way TestBlocks_ConcurrentSubagentsEachStayOneSegment pins contiguity.
Fixes review: overlapping exchanges in one subagent block got the count of exchanges seen so far, so a response could show the number of the next exchange Files: - cmd/agentop/tui/events_pane.go - cmd/agentop/tui/events_blocks_test.go Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
Re-review of 2d97f8a. Both items from the previous review are resolved. events_pane.go now maps each session id to a block-local number assigned on first sight, so a request and its response share a number even when exchanges overlap. TestBlocks_OverlappingExchangesKeepTheirNumbers pins 1,2,1,2 and the 2 msgs rollup. I checked that the new test fails on 1dd7cfb's logic ([1 2 2 2]) and passes on the fix. The incremental diff touches nothing else, and I have no new findings.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI), Docs (CLAUDE.md)
Agent/IDE config (.claude/.vscode): none
Commits: 4 commits, all signed-off: yes
CI status: passing (all checks green; Spellcheck skipped)
Verified locally on 2d97f8a (go 1.27.3): go test -count=1 ./cmd/agentop/tui/... passes, and go vet and gofmt -l are clean.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
What
Two changes to the subagent blocks from #1601, from its live check on session
3230ebba("Cortex PR 1611 Merge Conflict"):of the chronological interleaving with the main conversation.
keep session-wide numbers.
Why
Expanding
▾─ review cortex#1611 r1worked, but the rows directly under theheader were the main conversation's — a subagent's exchange spans several
main-conversation turns (and other subagents'), so its rows scattered across
the pane at their own timestamps. That reads as "expanding did nothing". And
the
#numbers the members did show were session-wide (starting at 94), whichsays nothing about how much the subagent itself did — the block reads as the
subagent's own transcript, so its first message should be 1.
The merged tests' fixture never interleaved a main row inside a subagent
exchange, so the interleaved case was never pinned.
The changes
Contiguity —
rebuildEventsTable's row loop: when a block's header isemitted and the block is open, all of
b.rows(already the inactive-filteredpopulation, in arrival order) render immediately after the header; member rows
no longer render at their own loop turn. Cell building moved into a small
renderEventRow(j)helper used by both paths sorows/visibleRows/search rows cannot drift apart. Membership is by subagent id over the whole
session, not time-contiguity, so interleaving cannot split a block into
segments or emit a second header for the same agent — pinned by
TestBlocks_ConcurrentSubagentsEachStayOneSegment(two subagents interleavedwith each other and the main conversation, one resuming long after it started).
Numbering — a block-local
blockIDsmap renumbers member exchanges from 1,built after the blocks are collected; the session-wide ids still decide what an
exchange IS (request + response share one number), only the counting restarts
per block. The header's rollup counts the same unit (
28 msgs, not56 rows)so it agrees with the last number shown inside the block.
Unchanged: the header anchors at the block's first surviving row; main rows
keep their positions and session numbers; TIME stays absolute; the DIR
gutter's
│still marks members; sorting disables grouping (blocks exist onlyin the chronological view, so the sort-key path never sees a member row);
cursor pin, search, hideInactive and the Timeline reveal all key on
visibleRows/event identity, not row order —#is display-only.Verification
TestBlocks_ExpandedMembersStayTogether(a main rowchronologically inside a subagent's exchange stays outside the expanded
block),
TestBlocks_ConcurrentSubagentsEachStayOneSegment,TestBlocks_MembersNumberFromOneInsideTheBlock(member#cells read1,1,2,2 and main rows keep 1..3; header says
2 msgs). All fail on main'sbehavior where they apply.
cmd/agentop/tuipackage green;gofmt/go vetclean.session: Enter on
▸─ review cortex#1611 r1 · 28 msgs→▾, the rows underthe header are all the subagent's glm exchanges numbered 1,1,2,2,3…, no
main-conversation rows interleaved; r2 (
34 msgs) stays collapsed.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit