Repository navigation
fix: Aim the detail cursor at the tool call, not at its message - #1865
Conversation
Enter on a tool row put the detail cursor on the assistant message that held the call. The first line of that message is its header, so the detail view scrolled to the top of the message block and not to the call. The README already says the cursor goes on the call. aimInRequest now returns the toolCalls entry. It keeps the message as the target when no entry names the call id. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
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. |
github-actions
Bot
added
the
ready-for-ai-review
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
This change does what it says. aimInRequest now returns the toolCalls entry that names the call id, so Enter on a tool row puts the detail cursor on the call itself, as the README says ("with the cursor on the call and the output open below it"). The change is small and tidy: add returns the entry it opened and handles a nil list itself, and the code falls back to the message only when no entry names the id. I found no must-fix issues.
Verification (local, on 50cda02):
go test ./tui/passes.go vet ./tui/andgofmt -l tui/are clean.- I ran a probe with a parallel turn: one assistant message calls
t1, t2and the next message returns both results. I pressed Enter on thet2row. The cursor lands onrequest.inference.messages[1].toolCalls[1], andmessages[2].toolResults[1]is open. I renderedm.View()to check it: the▌cursor gutter is on thet2entry's{line, below thet1entry, and both results show underneath. So the fix also holds beyond the single-call fixtures. - I ran a reused-id probe with two real
historyToolCalls/toolResultsentries, bothcall_0.nestToolBlocksnests only the first entry, undermessages[1]. The second entry stays in the top-levelhistoryToolCallslist, and the cursor goes to themessages[3]header. The PR description names this case as a deliberate fallback, so I don't count it against the stated fix. See the optional inline note.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI), tests
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: one job fails. Go CI (core) fails in its Test step (go test -race ./... under core/), but this PR changes only cmd/agentop/tui/, which core does not import. The same job passed on the base commit afaa765, and the GitHub log endpoint timed out for me, so I can't name the failing test. It looks unrelated or flaky, so a re-run is worth trying before merge. Go CI (authbridge agentop), the job that covers this code, passes.
| // aimInRequest is the toolCalls entry that names call id in the last assistant message of the | ||
| // open request that names it (rule 2.1), and the folds that show the call and its result: that | ||
| // entry and the next message's toolResults entry. The message index comes from the held event, | ||
| // because the tree |
There was a problem hiding this comment.
nit: the rewrap left an orphan line here: // because the tree ends early, and the sentence continues on the next line (// no longer has toolUseIds. …). Reflowing these two lines into one paragraph would read better.
| add(flat[i+1].child("toolResults"), "toolUseId") | ||
| } | ||
| if target == nil { | ||
| target = flat[i] |
There was a problem hiding this comment.
suggestion (optional, follow-up): in the reused-id case, the call this tool row means is not lost. nestToolBlocks leaves the second call_0 entry in the top-level request.inference.historyToolCalls list. I probed it with two real historyToolCalls entries, both call_0: the entry with {"command":"b"} stayed top-level, and the cursor went to the messages[3] header. If you want Enter to land on the call in this case too, the fallback could look up that list (with callWithID, or the k-th entry for the k-th message that names the id) before it falls back to flat[i]. Keeping the message is a fine fallback, though, and the description says so, so this doesn't block.
| enterOnRow(t, m, func(r *toolRow) bool { return r.ID == "call_0" }) | ||
| // nestToolBlocks gives the one call to the first message that names it, so the last | ||
| // one holds no entry to aim at and the message is the target. | ||
| assertCursorAt(t, m, "request.inference.messages[3]") |
There was a problem hiding this comment.
suggestion: every changed fixture has one call per message, so toolCalls[0] would pass even if the code always picked the first entry. A parallel-call case would pin the per-id lookup: one assistant message with ToolUseIDs: "t1, t2", matching HistoryToolCalls/ToolResults, and Enter on t2, with the cursor expected at request.inference.messages[1].toolCalls[1] and messages[2].toolResults[1] open. I ran exactly this locally and it passes.
There was a problem hiding this comment.
Review: fix: Aim the detail cursor at the tool call, not at its message
Reviewed the diff and verified empirically on head 50cda02 in a clean worktree: gofmt -l tui/ clean, go vet ./tui/ clean, go test ./tui/ passes (all tests, including the four updated cursor assertions and the ReusedID message-target case).
The change is sound: aimInRequest now returns the toolCalls entry naming the id (falling back to the message when no entry does, e.g. a reused id after nestToolBlocks reassigns it), which matches the README's "cursor goes on the call". No findings on the code.
CI note: Go CI (core) is failing, but in core/cost/ledger DST tests (TestMidnightTransitions_..., TestStartOfLocalDay_...) — files this PR does not touch. The same failures reproduce on a tree without this PR's changes, so they look environmental/pre-existing rather than caused by this diff. Everything the PR touches is green locally. May be worth a re-run.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (TUI), tests, commit/PR conventions
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, signed-off: yes
CI status: Go CI (core) failing (unrelated, see note); Go CI (authbridge agentop) pending; all other checks pass
Generated by RepoMan github-pr-review@235a7f7 (https://github.com/rossoctl/agent-skills/blob/235a7f7620139ee3e4ab0872b8439591fe52e8c1/skills/github-pr-review/SKILL.md).
fetchCatalogs writes the in-memory catalog before the durable one. The test waited for the in-memory one, then read the store. Between the two writes the store still held the old list, so the test failed on a loaded CI runner. Wait for the store. The fetch writes it last, so a match proves that both writes completed. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
CI failed in The test was racy:
Commit 58ef880 waits for the store instead. The fetch writes the store last, so a match proves that both writes completed. Assisted-By: Claude (Anthropic AI) noreply@anthropic.com |
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
clawgenti
left a comment
clawgenti
left a comment
There was a problem hiding this comment.
Summary
Note: this PR merged while the review was in progress. The must-fix below is a follow-up for a new PR.
The cursor now lands on the right node: request.inference.messages[N].toolCalls[k]. I checked this with a parallel-call probe as well (t0, t5, t1, Enter on t1 puts the cursor on toolCalls[2]). In a rendered view, though, the stated goal of this PR often fails. The aimed { line ends up on the last row of the viewport, and the call's arguments, its id and the opened result are all below the fold. This is the case the PR sets out to fix, so I've tagged it must-fix even though it is not a regression. Details and a fix that I verified are in the inline comment.
The second commit (58ef880a, the inferencerouter deflake) is correct. fetchCatalogs calls catalogs.set and then saveCatalog, so polling the store proves both writes landed. I put a 100ms sleep between the two writes: the old test fails at once (stored list = ["rits/zai-org/glm-5-3"]), and the new one passes. With -race -count=30 it is clean. Nit: the PR body doesn't mention this commit. A line under Tests would help anyone who later bisects through it.
The earlier inline nit about the orphaned // because the tree line in the aimInRequest doc comment is still open (tool_enter.go:95).
Verification (local, on 58ef880): go test ./tui/ passes, go vet passes, and gofmt -l is clean. I rendered m.View() at 80x24, 120x40 and 200x50, with assistant content of 0 to 2000 B before the call (see inline).
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (agentop TUI, inferencerouter tests), commit/PR conventions
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: all passing
| } | ||
| // The cursor goes to the call itself: the message's first line is its header, and | ||
| // Enter on a tool row means that tool. | ||
| target := add(flat[i].child("toolCalls"), "id") |
There was a problem hiding this comment.
must-fix: the cursor now targets the call, but the view often doesn't show it. showDetail does GotoTop() and then scrollToDetailCursor() (detail_pane.go:50-51). That scroll is minimal: it scrolls just far enough to bring the cursor line onto the screen. Because the target is now deeper than the message header, it lands on the bottom row with nothing of the call underneath it.
I rendered m.View() after Enter on a one-call turn (callMsgs("t1") with a {"command":"ARGS"} call and a RESULTOUT result), varying only the assistant message's content length:
| size | content | cursor row | args visible | result visible |
|---|---|---|---|---|
| 80x24 | 0 B | 16/24 | yes | no |
| 80x24 | 300 B+ | 21/24 (last body row) | no | no |
| 120x40 | 2000 B | 28/40 | yes | no |
At 80x24 with any real assistant text, the screen shows ▌ ▾ { on its last body row, and the arguments/id/name lines and the result are all off screen. The README says "with the cursor on the call and the output open below it", and that doesn't hold. In a parallel turn (3 calls, Enter on the 3rd) the earlier entries t0/t5 fill the screen and the target's own body is cut off at 80x24 and at 200x40. The fetched-later path (TestToolEnter_AimAfterFetch) goes through the same minimal scroll.
A one-place fix: when the aim applied, put the target's first row at the top instead of scrolling minimally:
if resetScroll {
m.detailVp.GotoTop()
if aimed {
first, _ := m.detailDoc.rowsOf(m.detailDoc.cursor)
m.detailVp.SetYOffset(first)
}
return
}With this change, every case above shows the args, the id and the result (cursor row 4 at 80x24 and 120x40, clamped at 200x50), and the full go test ./tui/ suite still passes. The resetScroll=false path, where the aim applies after the fetch, needs the same treatment. You could return aimed from there, or do the anchoring inside applyToolAim's caller. A test that sets a small WindowSizeMsg and asserts that the result line is in m.View() would pin this down, since assertCursorAt only checks the path.
Problem
Enter on a tool row put the detail cursor on the assistant message that held
the call. The first line of a message node is its header (
contentBytes,role,toolCallNames). The detail view therefore scrolled to the top of themessage block, above the
toolCallslist.The README says the cursor goes "on the call". The code did not do that.
Change
aimInRequestnow returns thetoolCallsentry that names the call id, andopens the call and the result that follows it. The message stays the target
when no entry names the id. This happens when
nestToolBlocksgave the call toan earlier message (a reused id), or when the tree holds no call.
Tests
TestToolEnter_AimOnCall,_AimAfterFetch,_AimInEarlierGroupnow expectthe cursor at
...messages[N].toolCalls[0].TestToolEnter_ReusedIDkeeps the message path, with a comment on why.go test ./tui/andscripts/dev/go-test-shards.sh -short cmd/agentop(1087 tests) pass.
go vetandgofmtare clean.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com