fix(terminal): make periodic refreshes non-blocking - #264
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)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds asynchronous, coalesced terminal refreshes backed by immutable snapshots. It updates agent-task and workflow panels to use snapshots, resets state across session changes, and adds timing diagnostics for terminal and extension dispatch operations. ChangesTerminal refresh and diagnostics
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant RefreshCoordinator
participant RuntimeControllers
participant InterruptHandler
App->>RefreshCoordinator: request terminal refresh
RefreshCoordinator->>RuntimeControllers: load agent and workflow sections
RuntimeControllers-->>RefreshCoordinator: return snapshot sections
RefreshCoordinator-->>App: publish terminalRefreshResult
App->>InterruptHandler: process refresh result
InterruptHandler->>App: apply valid snapshot
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
==========================================
+ Coverage 84.31% 84.33% +0.01%
==========================================
Files 335 337 +2
Lines 33942 34496 +554
==========================================
+ Hits 28619 29093 +474
- Misses 3628 3688 +60
- Partials 1695 1715 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
internal/terminal/refresh_acceptance_internal_test.go (2)
467-468: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead
listCallsthrough a mutex-guarded accessor.
agentTaskControllerStub.ListincrementslistCallsunderstub.mu, but Line 467 and Line 484 read the field directly. The read is safe today because the load completed before this point, howeverapplyTerminalRefreshSnapshotstarts watch goroutines against the same stub. Add acalls()accessor toagentTaskControllerStub, asrefreshToolTaskControlleralready provides, and use it here.🤖 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 `@internal/terminal/refresh_acceptance_internal_test.go` around lines 467 - 468, Update agentTaskControllerStub with a mutex-guarded calls() accessor for listCalls, matching refreshToolTaskController, and replace the direct listCalls reads in the affected refresh test flow with this accessor while preserving the existing toolCalls handling.
376-385: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe test loader reads
Appstate from the worker goroutine.
loadTerminalRefreshSnapshotis documented ininternal/terminal/terminal_refresh_data.go(Line 87-88) as not readingApp. This stub loader readsapp.sessionIDinside the worker goroutine, which breaks that contract and can report a race undergo test -raceif a later test variant writessessionIDwhile a refresh is in flight. Userequest.SessionID, which is captured on the UI thread.♻️ Proposed adjustment
- app.refreshLoader = func(context.Context, *terminalRefreshRequest) terminalRefreshSnapshot { - snapshot := newTerminalRefreshSnapshot(app.sessionID) + app.refreshLoader = func(_ context.Context, request *terminalRefreshRequest) terminalRefreshSnapshot { + snapshot := newTerminalRefreshSnapshot(request.SessionID)🤖 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 `@internal/terminal/refresh_acceptance_internal_test.go` around lines 376 - 385, Update the refreshLoader stub to use request.SessionID when constructing the terminalRefreshSnapshot instead of reading app.sessionID from the worker goroutine. Preserve the existing snapshot contents and loader behavior.internal/extension/manager_dispatch.go (2)
240-247: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the per-dispatch attribute allocation when debug logging is disabled.
logDispatchDurationruns on everytickandrenderdispatch. Theattributesslice is allocated and filled before the log level is evaluated, so the allocation happens on each frame even when the handler discards debug records. Check the level first, or build the slice only when it is needed.♻️ Proposed adjustment
- attributes := make([]any, 0, extensionDiagnosticAttributes) - attributes = append(attributes, - slog.String("operation", operation), - slog.Duration("duration", duration), - slog.String("outcome", outcome), - slog.Int("count", count), - ) - manager.logger.Debug("extension callback dispatch", attributes...) - - if duration < extensionSlowDispatchThreshold { - return - } + slow := duration >= extensionSlowDispatchThreshold + if !slow && !manager.logger.Enabled(context.Background(), slog.LevelDebug) { + return + } + + attributes := make([]any, 0, extensionDiagnosticAttributes) + attributes = append(attributes, + slog.String("operation", operation), + slog.Duration("duration", duration), + slog.String("outcome", outcome), + slog.Int("count", count), + ) + manager.logger.Debug("extension callback dispatch", attributes...) + + if !slow { + return + }As per coding guidelines: "Keep the default render path hot and allocation-conscious".
🤖 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 `@internal/extension/manager_dispatch.go` around lines 240 - 247, Update logDispatchDuration so it checks whether debug logging is enabled before allocating or populating the attributes slice. Return or skip the logging work when the logger discards Debug records, while preserving the existing extension callback dispatch message and attributes when enabled.Source: Coding guidelines
86-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd parentheses to the mixed boolean condition.
Go binds
&&tighter than||, so the condition is correct. Parentheses make the intent explicit and prevent a wrong edit later.♻️ Proposed adjustment
- if event == nil || event.Name != "tick" && event.Name != "render" { + if event == nil || (event.Name != "tick" && event.Name != "render") {🤖 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 `@internal/extension/manager_dispatch.go` around lines 86 - 88, Update the condition in the event dispatch logic to parenthesize the combined event-name check, explicitly grouping the `event.Name != "tick"` and `event.Name != "render"` comparisons under the existing nil check while preserving the current behavior.internal/extension/manager_diagnostics_internal_test.go (1)
145-168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the fake clock fail fast instead of blocking.
timesholds exactly two values. If the dispatch path callsdispatchNowa third time, the receive at Line 162 blocks until the package test timeout, and the failure reason is not visible. Add a default branch that returns a sentinel time, so an unexpected call count fails the test quickly.♻️ Proposed adjustment
manager.dispatchNow = func() time.Time { - now := <-times + var now time.Time + select { + case now = <-times: + default: + return startedAt.Add(time.Hour) + } + if now.Equal(startedAt) { close(started) }🤖 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 `@internal/extension/manager_diagnostics_internal_test.go` around lines 145 - 168, Update the fake clock in assertDispatchIncludesLockWait so manager.dispatchNow fails fast when times is exhausted instead of blocking. Add a non-blocking default path that returns a sentinel time for unexpected calls, while preserving the existing queued timestamps and started signaling behavior.internal/terminal/workflows_internal_test.go (2)
307-343: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore coverage for the rejected-run cases.
The previous table-driven cases for a missing run and a run owned by another session were removed.
openWorkflowDetailnow validates against the snapshot, so those rules still apply and are still worth asserting.TestOpenWorkflowDetailRequiresSnapshotcovers only the empty-snapshot case and the missing-detail case. Add a case where the snapshot holds a run whoseTask.OwnerSessionIDdiffers fromapp.sessionID. Prefer a table for these variants.The
getFailsandlinksFailfields onworkflowPanelInspectormay now be unreferenced after the removal. Remove them if no test sets them.As per coding guidelines: "Prefer table-driven tests for core behavior".
Run the following script to check whether the stub failure flags are still used:
#!/bin/bash # Description: Find remaining uses of the workflowPanelInspector failure flags. set -euo pipefail rg -n -C 3 '\b(getFails|linksFail)\b' internal/terminal🤖 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 `@internal/terminal/workflows_internal_test.go` around lines 307 - 343, Extend TestOpenWorkflowDetailRequiresSnapshot with table-driven cases covering an empty snapshot and a snapshot containing a run whose Task.OwnerSessionID differs from app.sessionID, preserving the expected errors and ensuring no detail lookup occurs. Search workflowPanelInspector usages for getFails and linksFail, and remove those fields if no tests still configure them.Source: Coding guidelines
129-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail the test when
loadWorkflowDetailsreturns an error.
seedWorkflowPanelSnapshotdiscards the error and leavesworkflowDetailSnapshotValidfalse. A seeding failure then surfaces as an unrelated assertion failure in the calling test. Accept*testing.T, uset.Context(), and require no error.♻️ Proposed adjustment
-func seedWorkflowPanelSnapshot(app *App, stub *workflowPanelInspector) { +func seedWorkflowPanelSnapshot(t *testing.T, app *App, stub *workflowPanelInspector) { + t.Helper() + if stub == nil || stub.run == nil { return } app.workflowPanelSnapshot = []database.WorkflowRunEntity{*stub.run} app.workflowPanelSnapshotValid = true - details, err := loadWorkflowDetails(context.Background(), stub, []string{stub.run.Task.ID}) - if err == nil { - app.workflowProgress = details.ProgressByRun - app.workflowSteps = details.StepsByRun - app.workflowDetailSnapshotValid = true - } + details, err := loadWorkflowDetails(t.Context(), stub, []string{stub.run.Task.ID}) + require.NoError(t, err) + + app.workflowProgress = details.ProgressByRun + app.workflowSteps = details.StepsByRun + app.workflowDetailSnapshotValid = true }Update the five call sites at Lines 174, 204, 245, 256, and 286, and Line 315, to pass
t.🤖 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 `@internal/terminal/workflows_internal_test.go` around lines 129 - 143, Update seedWorkflowPanelSnapshot to accept *testing.T, use t.Context() for loadWorkflowDetails, and fail immediately with a test assertion when it returns an error instead of silently skipping snapshot initialization. Update all six call sites in the affected tests to pass t.internal/terminal/agent_tasks_behavior_internal_test.go (1)
555-575: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a timeout to
applyAgentRefreshInterrupt.The loop blocks on
app.screen.EventQ()with no deadline. If a refresh result is never posted, the test hangs until the package timeout and the failure reason is not reported.awaitInterruptininternal/terminal/refresh_acceptance_internal_test.goalready implements a bounded wait in the same package. Reuse it or add an equivalenttime.Afterbranch.♻️ Proposed adjustment
for { - event := <-app.screen.EventQ() - - interrupt, ok := event.(*tcell.EventInterrupt) - if !ok { - continue - } + var event tcell.Event + + select { + case event = <-app.screen.EventQ(): + case <-time.After(time.Second): + t.Fatal("agent refresh interrupt was not published") + } + + interrupt, ok := event.(*tcell.EventInterrupt) + if !ok { + continue + }🤖 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 `@internal/terminal/agent_tasks_behavior_internal_test.go` around lines 555 - 575, Update applyAgentRefreshInterrupt to use the bounded-wait behavior from awaitInterrupt, or an equivalent time.After/select timeout, while waiting for app.screen.EventQ(). Ensure a missing terminalRefreshResult fails promptly with a clear timeout error instead of blocking indefinitely.internal/terminal/app.go (1)
352-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
scopedOrderandscopedEnabledinitialization.
initializeAppViewStatesets both fields at Lines 353-354.initializeAppInputStatesets them again at Lines 375-376. Keep one owner for these fields.♻️ Proposed cleanup
app.composerImages = []imageAttachment{} app.bracketedPaste = false - app.scopedOrder = []string{} - app.scopedEnabled = map[string]bool{} }🤖 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 `@internal/terminal/app.go` around lines 352 - 377, Remove the duplicate scopedOrder and scopedEnabled assignments from initializeAppInputState, keeping their initialization in initializeAppViewState as the single owner.internal/terminal/agent_tasks.go (1)
703-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated branch.
Both paths call
app.stopAgentTaskWatch(taskID)and return. Theif len(app.agentTaskSessionStack) > 0block no longer changes behavior.♻️ Proposed simplification
func (app *App) handleAgentTaskWatchError(_ context.Context, taskID, message string) { app.addSystemMessage(message) - if len(app.agentTaskSessionStack) > 0 { - app.stopAgentTaskWatch(taskID) - - return - } - // Leave the failed watch stopped. The next periodic snapshot can retry it // without recursively publishing another event from this handler. app.stopAgentTaskWatch(taskID) }🤖 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 `@internal/terminal/agent_tasks.go` around lines 703 - 715, Collapse handleAgentTaskWatchError by removing the redundant agentTaskSessionStack conditional and invoking app.stopAgentTaskWatch(taskID) once after app.addSystemMessage(message). Preserve the existing stop behavior without the unnecessary early return or branch-specific comment.internal/terminal/refresh.go (1)
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the recover block.
The named
panicValuevariable is unused. SonarCloud flags it. Also add a short comment that states why the panic is swallowed, because an empty recover hides real defects.♻️ Proposed simplification
- defer func() { - if panicValue := recover(); panicValue != nil { - return - } - }() + // The screen event queue can be closed during shutdown; a send then panics. + defer func() { _ = recover() }()🤖 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 `@internal/terminal/refresh.go` around lines 128 - 132, Update the deferred recovery block in the refresh logic to call recover without assigning its result, removing the unused panicValue variable. Add a brief comment explaining why panics are intentionally swallowed in this path.Source: Linters/SAST tools
🤖 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 `@internal/terminal/agent_tasks_behavior_internal_test.go`:
- Around line 1646-1649: Update the refresh apply path used by
app.refreshAgentTasksPanel so that when terminalRefreshSnapshot.hasErrors() is
true, it sets app.statusMessage to a user-visible error message while retaining
stale section data. Ensure partial_error continues to diagnostics but persistent
database failures are also surfaced through the status message.
In `@internal/terminal/agent_tasks.go`:
- Around line 1105-1126: Update deliverKnownAgentTaskCompletionText and its
callers so workflow-child detection still uses the in-memory
isTrackedWorkflowChild check before invoking finishAgentTaskCompletion. Preserve
the optimization that avoids isPersistedWorkflowChild on this completion path,
but ensure workflow children linked through workflow tables are suppressed and
do not call deliverAgentTaskCompletions.
In `@internal/terminal/terminal_refresh_data.go`:
- Around line 510-512: Make the refresh ordering deterministic in the append
loops over activeByID and listed by sorting entries with a stable task key such
as Task.ID or Task.CreatedAt before appending. Preserve the existing collection
contents while ensuring newly discovered agent tasks and workflow runs retain
consistent UI positions across refreshes.
- Around line 556-576: The missing-lookup branch in the task refresh logic
should retain tasks that are absent from the snapshot. Update the !found
handling for lookups.Value[previous.Task.ID] to return previous, nil instead of
removing the task, and preserve the existing behavior for nil and terminal task
states.
In `@internal/terminal/workflows.go`:
- Around line 113-129: Preserve the context in openWorkflowDetail and pass it
into openWorkflowDetailFromSnapshot. When workflowDetailSnapshotValid is false,
request a workflow refresh using that context before returning the loading
error, so the detail view is reopened after the next snapshot and
workflowPanelRunID is populated.
---
Nitpick comments:
In `@internal/extension/manager_diagnostics_internal_test.go`:
- Around line 145-168: Update the fake clock in assertDispatchIncludesLockWait
so manager.dispatchNow fails fast when times is exhausted instead of blocking.
Add a non-blocking default path that returns a sentinel time for unexpected
calls, while preserving the existing queued timestamps and started signaling
behavior.
In `@internal/extension/manager_dispatch.go`:
- Around line 240-247: Update logDispatchDuration so it checks whether debug
logging is enabled before allocating or populating the attributes slice. Return
or skip the logging work when the logger discards Debug records, while
preserving the existing extension callback dispatch message and attributes when
enabled.
- Around line 86-88: Update the condition in the event dispatch logic to
parenthesize the combined event-name check, explicitly grouping the `event.Name
!= "tick"` and `event.Name != "render"` comparisons under the existing nil check
while preserving the current behavior.
In `@internal/terminal/agent_tasks_behavior_internal_test.go`:
- Around line 555-575: Update applyAgentRefreshInterrupt to use the bounded-wait
behavior from awaitInterrupt, or an equivalent time.After/select timeout, while
waiting for app.screen.EventQ(). Ensure a missing terminalRefreshResult fails
promptly with a clear timeout error instead of blocking indefinitely.
In `@internal/terminal/agent_tasks.go`:
- Around line 703-715: Collapse handleAgentTaskWatchError by removing the
redundant agentTaskSessionStack conditional and invoking
app.stopAgentTaskWatch(taskID) once after app.addSystemMessage(message).
Preserve the existing stop behavior without the unnecessary early return or
branch-specific comment.
In `@internal/terminal/app.go`:
- Around line 352-377: Remove the duplicate scopedOrder and scopedEnabled
assignments from initializeAppInputState, keeping their initialization in
initializeAppViewState as the single owner.
In `@internal/terminal/refresh_acceptance_internal_test.go`:
- Around line 467-468: Update agentTaskControllerStub with a mutex-guarded
calls() accessor for listCalls, matching refreshToolTaskController, and replace
the direct listCalls reads in the affected refresh test flow with this accessor
while preserving the existing toolCalls handling.
- Around line 376-385: Update the refreshLoader stub to use request.SessionID
when constructing the terminalRefreshSnapshot instead of reading app.sessionID
from the worker goroutine. Preserve the existing snapshot contents and loader
behavior.
In `@internal/terminal/refresh.go`:
- Around line 128-132: Update the deferred recovery block in the refresh logic
to call recover without assigning its result, removing the unused panicValue
variable. Add a brief comment explaining why panics are intentionally swallowed
in this path.
In `@internal/terminal/workflows_internal_test.go`:
- Around line 307-343: Extend TestOpenWorkflowDetailRequiresSnapshot with
table-driven cases covering an empty snapshot and a snapshot containing a run
whose Task.OwnerSessionID differs from app.sessionID, preserving the expected
errors and ensuring no detail lookup occurs. Search workflowPanelInspector
usages for getFails and linksFail, and remove those fields if no tests still
configure them.
- Around line 129-143: Update seedWorkflowPanelSnapshot to accept *testing.T,
use t.Context() for loadWorkflowDetails, and fail immediately with a test
assertion when it returns an error instead of silently skipping snapshot
initialization. Update all six call sites in the affected tests to pass t.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 389016f5-b603-4d42-bb7e-2d156d30bdf3
📒 Files selected for processing (17)
internal/extension/manager.gointernal/extension/manager_diagnostics_internal_test.gointernal/extension/manager_dispatch.gointernal/extension/manager_timer.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/commands.gointernal/terminal/prompt_response.gointernal/terminal/refresh.gointernal/terminal/refresh_acceptance_internal_test.gointernal/terminal/session_commands.gointernal/terminal/session_panel.gointernal/terminal/terminal_refresh_data.gointernal/terminal/workflows.gointernal/terminal/workflows_internal_test.go
01a2fb2 to
5dcee13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/terminal/refresh.go (1)
292-305: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid debug work when debug logging is disabled.
terminalRefreshDiagnostics.logconstructsattributesbefore the logger can filter a debug record. Every completed refresh logs the total operation, and timed sections add more calls. Gate debug attribute construction onlogger.Enabled, then construct warning attributes only for slow operations.Proposed change
func (diagnostics *terminalRefreshDiagnostics) log( operation string, duration time.Duration, outcome string, count int, ) { - attributes := []any{ - slog.String("operation", operation), - slog.Duration("duration", duration), - slog.String("outcome", outcome), - } - if count > 0 { - attributes = append(attributes, slog.Int("count", count)) - } - logger := slog.Default() - logger.Debug("terminal refresh", attributes...) + if logger.Enabled(context.Background(), slog.LevelDebug) { + attributes := []any{ + slog.String("operation", operation), + slog.Duration("duration", duration), + slog.String("outcome", outcome), + } + if count > 0 { + attributes = append(attributes, slog.Int("count", count)) + } + logger.Debug("terminal refresh", attributes...) + } if duration < terminalSlowOperationThreshold { return } + attributes := []any{ + slog.String("operation", operation), + slog.Duration("duration", duration), + slog.String("outcome", outcome), + } + if count > 0 { + attributes = append(attributes, slog.Int("count", count)) + }As per coding guidelines, “Keep the default render path hot and allocation-conscious.”
🤖 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 `@internal/terminal/refresh.go` around lines 292 - 305, Update terminalRefreshDiagnostics.log to obtain the logger and check logger.Enabled for debug level before constructing the refresh attributes or emitting the debug record. Preserve diagnostics for enabled debug logging, and defer construction of warning attributes until after confirming duration meets terminalSlowOperationThreshold, keeping the normal render path allocation-conscious.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 `@internal/terminal/refresh_acceptance_internal_test.go`:
- Around line 213-219: Synchronize the canceled refresh test with completion of
the refresh goroutine before inspecting screen.EventQ(). Add or reuse a
completion signal for the refresh execution or screen publication path, wait for
that signal after awaitSignal, then assert that no event was published; keep the
existing cancellation and empty-queue assertions.
---
Nitpick comments:
In `@internal/terminal/refresh.go`:
- Around line 292-305: Update terminalRefreshDiagnostics.log to obtain the
logger and check logger.Enabled for debug level before constructing the refresh
attributes or emitting the debug record. Preserve diagnostics for enabled debug
logging, and defer construction of warning attributes until after confirming
duration meets terminalSlowOperationThreshold, keeping the normal render path
allocation-conscious.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d68841e3-7fa1-43c3-87c4-36dc04773de1
📒 Files selected for processing (12)
internal/extension/manager_diagnostics_internal_test.gointernal/extension/manager_dispatch.gointernal/terminal/agent_tasks.gointernal/terminal/agent_tasks_behavior_internal_test.gointernal/terminal/app.gointernal/terminal/async_events.gointernal/terminal/prompt_response.gointernal/terminal/refresh.gointernal/terminal/refresh_acceptance_internal_test.gointernal/terminal/terminal_refresh_data.gointernal/terminal/workflows.gointernal/terminal/workflows_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/terminal/prompt_response.go
- internal/terminal/async_events.go
- internal/extension/manager_dispatch.go
- internal/extension/manager_diagnostics_internal_test.go
- internal/terminal/workflows.go
- internal/terminal/terminal_refresh_data.go
- internal/terminal/agent_tasks.go
- internal/terminal/workflows_internal_test.go
- internal/terminal/app.go
- internal/terminal/agent_tasks_behavior_internal_test.go
5dcee13 to
887ce6a
Compare
887ce6a to
b7d5f0c
Compare
|
1 participant
No description provided.