Skip to content

Commit 84bf36c

Browse files
hazyhaarhazyhaar
authored andcommitted
fix(tui): resolve review findings on BTW recovery, scroll geometry, and marker snapshots
- Revoke side-view lifecycle and restore parent view on BTW exit - Preserve chat and file-view scroll geometry across in-flight reloads - Trigger conservative unknown-scope file view refresh on hidden mutating tool results - Add regression coverage for BTW exit, scroll clamping, and mutation invalidation
1 parent 654e51b commit 84bf36c

5 files changed

Lines changed: 640 additions & 41 deletions

File tree

‎internal/tui/btw.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,10 @@ func (m model) leaveBTW() (model, tea.Cmd) {
195195
if m.compactInFlight {
196196
return m.appendSystemNotice("BTW compaction is still running. Wait for it to finish before returning."), nil
197197
}
198+
// The side surface is being discarded. Revoke its file-view lifetime token so
199+
// a load still queued behind BTW cannot run or land after the view closes.
200+
// The restored parent keeps its own request and snapshot untouched.
201+
m.revokeFileViewRequest()
198202
m, _ = m.clearLoopsForSessionSwitch()
199203
parent := *m.btw.parent
200204
parent.goalContinuationsSuspended = false
@@ -221,7 +225,9 @@ func (m model) leaveBTW() (model, tea.Cmd) {
221225
parent.resetFlushFrontier("· returned from btw ·")
222226
var goalCmd tea.Cmd
223227
parent, goalCmd = parent.launchGoalContinuationIfReady()
224-
return parent, batchCommands(sweepCmd, spinnerCmd, goalCmd)
228+
var fileCmd tea.Cmd
229+
parent, fileCmd = parent.recoverInvalidatedFileView()
230+
return parent, batchCommands(sweepCmd, spinnerCmd, goalCmd, fileCmd)
225231
}
226232

227233
func btwCommandUnavailable(command parsedCommand) bool {
@@ -325,7 +331,9 @@ func (m model) routeBTWMessageToParent(msg tea.Msg) (model, tea.Cmd, bool) {
325331
case agentResponseMsg:
326332
m.btw.parentNeedsInput = parent.pendingPermission != nil || parent.pendingAskUser != nil
327333
}
328-
return m, cmd, true
334+
var fileCmd tea.Cmd
335+
m, fileCmd = m.recoverInvalidatedFileView()
336+
return m, batchCommands(cmd, fileCmd), true
329337
}
330338

331339
func btwMessageRunID(msg tea.Msg) (int, bool) {
@@ -356,6 +364,8 @@ func btwMessageRunID(msg tea.Msg) (int, bool) {
356364
return typed.runID, true
357365
case specialistProgressMsg:
358366
return typed.runID, true
367+
case unknownScopeMutationMsg:
368+
return typed.runID, true
359369
case swarmSessionsMsg:
360370
return typed.runID, true
361371
case permissionRequestMsg:

‎internal/tui/btw_test.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package tui
22

33
import (
44
"context"
5+
"errors"
56
"os"
67
"path/filepath"
78
"strings"
@@ -473,3 +474,69 @@ func TestBTWCtrlCDuringRunDoesNotClearDraft(t *testing.T) {
473474
t.Fatalf("missing in-flight return guidance: %#v", got.transcript)
474475
}
475476
}
477+
478+
// TestBTWLeaveRevokesSideFileLoad verifies that leaving BTW revokes the side
479+
// surface's file-view lifetime so a still-queued side load cannot run or land
480+
// after the view closes, without cancelling the restored parent's own load.
481+
func TestBTWLeaveRevokesSideFileLoad(t *testing.T) {
482+
resetFileViewCacheForTest()
483+
m := newBTWTestModel(t)
484+
dir := t.TempDir()
485+
m.cwd = dir
486+
name := "side_view.go"
487+
if err := os.WriteFile(filepath.Join(dir, name), []byte("package side\n"), 0o644); err != nil {
488+
t.Fatal(err)
489+
}
490+
// Open a full view so the side surface inherits an active file view.
491+
m, parentLoadCmd := m.openFileView(name)
492+
if parentLoadCmd == nil {
493+
t.Fatal("expected a pending parent load command")
494+
}
495+
parentLiveSeq := m.fileView.liveSeq
496+
if parentLiveSeq == nil {
497+
t.Fatal("parent load token missing")
498+
}
499+
parentSeq := parentLiveSeq.Load()
500+
501+
// Enter BTW: the side surface detaches and schedules its own load.
502+
side, sideLoadCmd := m.handleBTWCommand("")
503+
if !side.btw.active || side.btw.parent == nil {
504+
t.Fatal("expected an active BTW conversation")
505+
}
506+
if sideLoadCmd == nil {
507+
t.Fatal("side surface should schedule its own file load")
508+
}
509+
sideLiveSeq := side.fileView.liveSeq
510+
if sideLiveSeq == nil || sideLiveSeq == parentLiveSeq {
511+
t.Fatal("side load must own a detached lifetime token")
512+
}
513+
sideSeq := sideLiveSeq.Load()
514+
515+
// Return while the side load is still queued.
516+
parent, _ := side.leaveBTW()
517+
if sideLiveSeq.Load() == sideSeq {
518+
t.Fatal("leaveBTW must revoke the side file-view request")
519+
}
520+
if parentLiveSeq.Load() != parentSeq || parentLiveSeq == sideLiveSeq {
521+
t.Fatal("leaveBTW must not revoke the restored parent's request")
522+
}
523+
524+
// The held side command must now be a superseded no-op.
525+
held := sideLoadCmd()
526+
loaded, ok := held.(fileViewLoadedMsg)
527+
if !ok {
528+
t.Fatalf("held side command produced %T, want fileViewLoadedMsg", held)
529+
}
530+
if !errors.Is(loaded.err, errFileViewSuperseded) {
531+
t.Fatalf("queued side load must be superseded after leaving BTW, got %v", loaded.err)
532+
}
533+
534+
// The parent's own load still completes and settles its snapshot.
535+
parent = deliverCommandMessages(t, parent, parentLoadCmd)
536+
if parent.fileView.loading {
537+
t.Fatal("parent load must remain valid after the side revocation")
538+
}
539+
if !strings.Contains(plainRender(t, parent.renderFileViewFull(80)), "package side") {
540+
t.Fatal("restored parent must keep its snapshot")
541+
}
542+
}

‎internal/tui/file_view.go‎

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,9 +1010,14 @@ type fileViewState struct {
10101010
path string // workspace-relative, as carried by changedFiles
10111011
mode int // fileViewDiff | fileViewFull
10121012
parentScrollOffset int
1013-
// preservedScrollOffset holds the reader's offset while an async reload
1014-
// swaps the body for the one-line loading placeholder.
1013+
// preservedScrollOffset holds the reader's latest scroll intent while an
1014+
// async reload swaps the body for the one-line loading placeholder. Scroll
1015+
// actions and prompt submission update it during the pending window, and
1016+
// handleFileViewLoaded reconciles it against the real body.
10151017
preservedScrollOffset int
1018+
// completedBodyLines is the rendered body height of the last accepted
1019+
// snapshot, including screen wrapping. A loading placeholder never updates it.
1020+
completedBodyLines int
10161021

10171022
// View session lifetime identity (UUIDv7 RFC 9562 0-alloc)
10181023
lifetimeToken [16]byte
@@ -1222,16 +1227,18 @@ func (m model) handleFileViewLoaded(msg fileViewLoadedMsg) (model, tea.Cmd) {
12221227
m.fileView.loadedRev = msg.requiredSourceRev
12231228
m.fileView.hasError = (msg.err != nil)
12241229
// Reconcile the held reading offset against the real body now that the
1225-
// loading placeholder is gone; clamp in case the file shrank.
1230+
// loading placeholder is gone. Apply the height delta exactly once to
1231+
// preserve the absolute position, while offset zero keeps following the tail.
1232+
current, maxOffset := m.chatScrollMetrics()
12261233
if m.fileView.preservedScrollOffset > 0 {
1227-
current, maxOffset := m.chatScrollMetrics()
1228-
m.chatScrollOffset = clampInt(m.fileView.preservedScrollOffset, 0, maxOffset)
1234+
m.chatScrollOffset = clampInt(m.fileView.preservedScrollOffset+current-m.fileView.completedBodyLines, 0, maxOffset)
12291235
if m.chatScrollOffset > 0 {
12301236
m.chatBodyLines = current
12311237
} else {
12321238
m.chatBodyLines = 0
12331239
}
12341240
}
1241+
m.fileView.completedBodyLines = current
12351242
return m, nil
12361243
}
12371244

@@ -1355,3 +1362,46 @@ func (m model) fileViewChangedLines() map[string]bool {
13551362
}
13561363
return changed
13571364
}
1365+
1366+
// toolResultMayMutateUnknownScope reports tools that can change workspace files
1367+
// but do not reliably report the affected paths (ChangedFiles). Their completion
1368+
// must trigger the conservative unknown-scope refresh so an open file view
1369+
// cannot keep rendering a stale snapshot. Task/TaskOutput successful results are
1370+
// suppressed from the transcript, so their completion boundaries carry this
1371+
// refresh separately.
1372+
func toolResultMayMutateUnknownScope(name string) bool {
1373+
switch name {
1374+
case "bash", "exec_command", "terminal_session", "write_stdin",
1375+
"swarm_collect", "swarm_status", "Task", "TaskOutput":
1376+
return true
1377+
}
1378+
return false
1379+
}
1380+
1381+
// invalidateFileViewForUnknownMutation performs the conservative refresh used
1382+
// when a completed action may have changed workspace files but no exact path is
1383+
// known: purge the shared render cache and, when a full file view is open,
1384+
// schedule an async reload so its snapshot cannot stay stale.
1385+
func (m model) invalidateFileViewForUnknownMutation() (model, tea.Cmd) {
1386+
defaultFileViewCache.invalidateUnknownScope()
1387+
if m.fileView.active && m.fileView.mode == fileViewFull {
1388+
return m.startFileViewRefreshCmd(m.chatColumnWidth())
1389+
}
1390+
return m, nil
1391+
}
1392+
1393+
// recoverInvalidatedFileView schedules a snapshot for a surface whose shared
1394+
// cache generation changed while another BTW surface handled the invalidation.
1395+
func (m model) recoverInvalidatedFileView() (model, tea.Cmd) {
1396+
if !m.fileView.active || m.fileView.mode != fileViewFull {
1397+
return m, nil
1398+
}
1399+
generation := m.fileView.loadedGen
1400+
if m.fileView.loading {
1401+
generation = m.fileView.desiredGen
1402+
}
1403+
if generation == defaultFileViewCache.generation() {
1404+
return m, nil
1405+
}
1406+
return m.startFileViewLoadCmd(m.chatColumnWidth())
1407+
}

0 commit comments

Comments
 (0)