Skip to content

fix(tui): 分离执行活动并恢复输出区滚动#117

Merged
minorcell merged 2 commits into
1024XEngineer:mainfrom
creatang:codex/issue-99-tui-activity-scroll
Apr 2, 2026
Merged

fix(tui): 分离执行活动并恢复输出区滚动#117
minorcell merged 2 commits into
1024XEngineer:mainfrom
creatang:codex/issue-99-tui-activity-scroll

Conversation

@creatang

@creatang creatang commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #99

说明

  • 将运行时执行提示从主 transcript 中拆出,单独展示到 activity
  • 补齐工具执行、重试、取消等运行态提示展示
  • 恢复输出区整页翻页,并支持鼠标滚轮滚动

验证

  • go test ./internal/tui ./internal/runtime
  • go test ./...

@creatang creatang changed the title fix(tui): split activity from transcript and restore transcript scrolling fix(tui): ?? activity ? transcript,?????????? Apr 2, 2026
@codecov

codecov Bot commented Apr 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.36364% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/tui/update.go 83.83% 13 Missing and 3 partials ⚠️
internal/tui/view.go 93.93% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@creatang creatang changed the title fix(tui): ?? activity ? transcript,?????????? fix(tui): 分离执行活动并恢复输出区滚动 Apr 2, 2026
@minorcell

Copy link
Copy Markdown
Member

/review

Comment thread internal/tui/update.go
Detail: detail,
IsError: isError,
})
if len(a.activities) > maxActivityEntries {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inefficient circular buffer implementation causes unnecessary allocations

This pattern allocates a new backing array and copies 64 elements on every append once the buffer is full. This creates GC pressure during rapid event streams and is inefficient.

Suggested fix: Use simple slice reslicing:

if len(a.activities) > maxActivityEntries {
    a.activities = a.activities[len(a.activities)-maxActivityEntries:]
}

Or for a more robust solution, consider implementing a proper ring buffer with fixed-size array and head/tail indices.

Comment thread internal/tui/update.go
return a, tea.Batch(cmds...)
case RuntimeClosedMsg:
a.state.IsAgentRunning = false
if strings.TrimSpace(a.state.StatusText) == "" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed rebuildTranscript() may cause stale display

The original code called rebuildTranscript() after runtime closure, but this was removed. If there are pending changes to activeMessages not yet reflected in the viewport, the user might see stale content.

Suggested fix: Verify that all message updates set transcriptDirty = true before runtime closure, or restore a conditional rebuild here.

Comment thread internal/tui/update.go
bodyY := contentY + headerHeight

streamX := contentX
streamY := bodyY

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout recomputed on every mouse event

The transcriptBounds() method calls computeLayout() and renderHeader() on every mouse wheel event. During rapid scrolling, this adds unnecessary CPU overhead.

Suggested fix: Cache layout bounds after resize events and reuse them:

// In App struct
type App struct {
    // ...
    cachedTranscriptBounds struct {
        x, y, width, height int
        valid               bool
    }
}

// Invalidate on resize
func (a *App) resizeComponents() {
    a.cachedTranscriptBounds.valid = false
    // existing logic...
}

Comment thread internal/tui/update.go
@@ -47,18 +49,19 @@ func (a App) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
a.resizeComponents()
return a, tea.Batch(cmds...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return value behavior undocumented

The handleRuntimeEvent function now returns a bool indicating whether the transcript needs to be rebuilt. This is a significant behavioral change that should be documented.

Suggested fix: Add function comment:

// handleRuntimeEvent processes runtime events and returns true if the 
// transcript needs to be rebuilt (i.e., when new messages are appended 
// to activeMessages).
func (a *App) handleRuntimeEvent(event runtime.Event) (transcriptDirty bool) {

Comment thread internal/tui/commands.go
focusLabelActivity = "Activity"
focusLabelComposer = "Composer"

activityPreviewEntries = 3

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation for magic numbers

These constants lack comments explaining their purpose and rationale.

Suggested fix:

// activityPreviewEntries is the number of recent activity entries shown in the preview panel
activityPreviewEntries = 3
// maxActivityEntries is the maximum number of activity entries retained in memory to prevent unbounded growth
maxActivityEntries     = 64

Comment thread internal/tui/view.go
}
}

func (a App) activityPreviewHeight() int {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number for fixed height

The function returns hardcoded 6 without explanation. This could cause layout issues if activityPreviewEntries changes.

Suggested fix: Add comment explaining the calculation:

// activityPreviewHeight returns the height of the activity preview panel.
// Returns 6 when activities exist (2 for header/border + 3 entry lines + 1 padding), 0 otherwise.
func (a App) activityPreviewHeight() int {

Comment thread internal/tui/update.go
@@ -230,8 +227,7 @@ func (a App) updateInputPanel(msg tea.Msg, typed tea.KeyMsg, cmds []tea.Cmd) (te
if err := a.refreshModelPicker(); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tool result message created without Name field

When appending tool results to activeMessages, the message is created without preserving the tool name. The payload.Name is only logged in the activity.

Suggested fix: Consider whether the Name field should be preserved for transcript rendering consistency. Review if renderMessageBlock expects tool name information.

@minorcell

Copy link
Copy Markdown
Member

Code Review Summary

Overall, this is a well-structured PR that successfully separates activity execution from the transcript and restores scrolling. The test coverage is comprehensive. The implementation follows existing patterns and improves UX.

Key strengths: Clean separation of concerns with activityEntry, elegant dirty-tracking return value, good bounds checking for mouse events.

Priority fixes: Address the inefficient circular buffer allocation (update.go:539), add string length limits to activity entries (security), and clarify the unreachable panelActivity focus state.

Great work on the activity tracking feature! 👍

Comment thread internal/tui/update.go
a.activeMessages = append(a.activeMessages, provider.Message{Role: role, Content: content})
}

func (a *App) appendActivity(kind string, title string, detail string, isError bool) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing string length limits and function documentation

The title and detail parameters are stored without length validation. While maxActivityEntries limits entry count, each entry's strings are unbounded. A malicious or misbehaving provider could send extremely long payloads causing excessive memory consumption.

Suggested fixes:

  1. Add length limits:
const maxTitleLen = 256
const maxDetailLen = 1024

title = strings.TrimSpace(title)
detail = strings.TrimSpace(detail)
if len(title) > maxTitleLen {
    title = title[:maxTitleLen-3] + "..."
}
if len(detail) > maxDetailLen {
    detail = detail[:maxDetailLen-3] + "..."
}
  1. Add function documentation:
// appendActivity adds an execution event to the activity log.
// kind categorizes the event (tool, command, provider, etc.),
// title is the main display text, detail provides additional context,
// and isError marks error states. Empty entries are ignored.
func (a *App) appendActivity(kind string, title string, detail string, isError bool) {

Comment thread internal/tui/view.go
@@ -418,11 +421,58 @@ func (a App) focusLabel() string {
return focusLabelSessions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreachable focus state for panelActivity

The focusLabel() method handles panelActivity, but focusNext() and focusPrev() in update.go only cycle through [panelSessions, panelTranscript, panelInput]. The Activity panel cannot be focused via keyboard navigation.

Suggested fix: Either add panelActivity to the focus order array, or remove this case from focusLabel() if it's intentionally unfocusable.

@minorcell minorcell merged commit 84eac50 into 1024XEngineer:main Apr 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix:优化 TUI 事件处理与 transcript 渲染:补齐运行时事件接入并分离执行提示

2 participants