Skip to content

Commit 92ec78f

Browse files
poteatpoteat
authored
diff: a docked pane reads the repository before it opens, so it never lands on Loading diff (#95488)
1 parent 2287e5d commit 92ec78f

5 files changed

Lines changed: 92 additions & 10 deletions

File tree

‎mods/diff/README.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ drawing's `viewport` says), the terminal is wide enough (144 columns when
2020
the person never chose, 110 when they kept it open before; a person who
2121
closed it is left alone) and file checkpointing is on; a subagent's edit
2222
opens nothing, and where the surface does not say, nothing opens by
23-
itself. An open the engine leaves waiting undrawn is withdrawn, so no
24-
later resize seats it, and the next edit asks again.
23+
itself. A docked pane fetches before it opens, as the built-in panel
24+
primes its data, so it never lands on `Loading diff…`; an open the engine
25+
leaves waiting undrawn is withdrawn, so no later resize seats it, and the
26+
next edit asks again.
2527

2628
Under the fullscreen layout a terminal under 110 columns gets the
2729
built-in's line asking for a wider one and nothing opens. Without that

‎mods/diff/hooks/register.ts‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ import Views from './views'
3434
*
3535
* Git runs when the built-in's would: `session.start` binds the host and
3636
* registers `/diff`; `/diff` or the main loop's first checkpointed edit with
37-
* room pins the backend, until `/clear`; only a placed, open pane fetches.
37+
* room pins the backend, until `/clear`; a docked pane fetches, then opens.
3838
*
3939
* @param on the engine's registrar
4040
*/
@@ -53,6 +53,7 @@ export function register(on: On) {
5353
let isRefreshing = false
5454
let isRefreshQueued = false
5555
let generation = 0
56+
let landed = 0
5657
let bodyStamp: string | null = null
5758
let bodyBase: string | null = null
5859

@@ -409,6 +410,12 @@ export function register(on: On) {
409410

410411
dialogRows = isDialog ? Views.dialogRowsOf(model) : null
411412

413+
const landedBefore = landed
414+
415+
if (!isDialog) {
416+
await refresh(engine).catch(() => undefined)
417+
}
418+
412419
const opened = await engine.openPane(
413420
isDialog
414421
? { ...dialogPane(), focus: true }
@@ -432,7 +439,11 @@ export function register(on: On) {
432439
Record.recorderOf(engine).shown(trigger, Record.widthBucketOf(columns))
433440
}
434441

435-
void refresh(engine)
442+
const isStale = isDialog || landed !== landedBefore
443+
444+
if (isStale) {
445+
void refresh(engine)
446+
}
436447

437448
return true
438449
}
@@ -860,11 +871,15 @@ export function register(on: On) {
860871
result.deny === undefined &&
861872
result.isError !== true
862873

863-
const isStale =
864-
isPaneOpen &&
865-
(isEdit ? hasEdited : result === undefined || result.deny === undefined)
874+
const hasLanded = isEdit
875+
? hasEdited
876+
: result === undefined || result.deny === undefined
866877

867-
if (isStale) {
878+
if (hasLanded) {
879+
landed += 1
880+
}
881+
882+
if (hasLanded && isPaneOpen) {
868883
scheduleRefresh(engine)
869884
}
870885

‎mods/diff/tests/fixtures/beneath/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
export type * from './beneath.js'
22
export * from './left-waiting.js'
3+
export * from './slow-diff-ms.js'
34

45
export * as default from '.'

‎mods/diff/tests/fixtures/beneath/slow-diff-ms.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
/**
2+
* How long a slow repository takes to answer `git diff` in the tests that
3+
* time the pane's opening against its first fetch: well inside git's timeout.
4+
*/
5+
export const SLOW_DIFF_MS = 3000

‎mods/diff/tests/register.test.ts‎

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -564,10 +564,16 @@ describe('register', () => {
564564
'and withdrew the pane the engine left waiting, so no resize seats it',
565565
).toEqual(['diff'])
566566

567+
expect(
568+
world.runs.map(run => Fixtures.gitWordOf(run.argv)).slice(0, 2),
569+
'the repository was found and read once before the open, as the ' +
570+
'built-in primes; nothing polls for a pane no one sees',
571+
).toEqual(['rev-parse --show-toplevel', 'status'])
572+
567573
expect(
568574
world.runs.map(run => Fixtures.gitWordOf(run.argv)),
569-
'the repository was found, nothing fetched for a pane no one sees',
570-
).toEqual(['rev-parse --show-toplevel'])
575+
'no HEAD poll for a withdrawn pane',
576+
).not.toContain(Fixtures.POLL_WORD)
571577

572578
isNarrow = false
573579

@@ -622,6 +628,59 @@ describe('register', () => {
622628
).toEqual(['diff'])
623629
})
624630

631+
test('a docked pane opens once its first fetch settled', async ($, on) => {
632+
const opened: string[] = []
633+
const clock = Fixtures.startsSession(on)
634+
635+
on('process.run', async ($, e) => {
636+
if (e.argv.includes('--shortstat')) {
637+
await clock.sleep(Fixtures.SLOW_DIFF_MS)
638+
}
639+
640+
return { value: Fixtures.gitIn(e.argv) }
641+
})
642+
643+
on('ui.open', ($, e) => {
644+
opened.push(e.id)
645+
646+
return { value: undefined }
647+
})
648+
649+
on('ui.close', () => ({ value: undefined }))
650+
on('ui.invalidate', () => ({ value: undefined }))
651+
on('ui.render', { component: 'PromptHint' }, () => Fixtures.HINT_DRAWN)
652+
on('session.messages', () => ({ value: [] }))
653+
on('settings.read', () => ({ value: {} }))
654+
on('tool.call', () => ({ result: 'edited' }))
655+
mock.store(on)
656+
mock.env(on, {})
657+
658+
await $.session.start(Fixtures.SESSION)
659+
await $.ui.render(Fixtures.HINT)
660+
661+
await $.tool.call({
662+
tool: 'Edit',
663+
file_path: '/work/app.ts',
664+
old_string: '1',
665+
new_string: '2',
666+
})
667+
668+
await clock.advance(Fixtures.SLOW_DIFF_MS - 1)
669+
670+
expect(opened, 'git has not answered: no pane, no Loading frame').toEqual(
671+
[],
672+
)
673+
674+
await clock.advance(Fixtures.SETTLE_MS)
675+
676+
expect(opened, 'the fetch settled: the pane opens filled').toEqual(['diff'])
677+
678+
const drawn = Fixtures.textOf(await $.ui.render(Fixtures.PANE))
679+
680+
expect(drawn).toContain('1 file changed')
681+
expect(drawn).not.toContain('Loading diff')
682+
})
683+
625684
test('/clear closes the pane it finds open', async ($, on) => {
626685
const world = Fixtures.inRepository(on)
627686

0 commit comments

Comments
 (0)