Skip to content

Commit e43287c

Browse files
committed
fix(scroll): count recycled-cell and fresh-signature progress as movement
Two false stops survived the first cut: - The surface signature coalesced to `identifier ?? label ?? value`, so a recycled cell that keeps its identifier and slot while its text changes looked unchanged and a genuinely advancing list was reported stuck. The signature now keys on identifier, label, and value together. - `scrollSurfaceIsStuck` accepted any four captures with at most two distinct signatures, including `A,A,A,B`, and both loops stopped on the pass that finally advanced. It now also requires the newest signature to be one the window already showed, so rubber-banding trips but a fresh signature continues.
1 parent 93de350 commit e43287c

6 files changed

Lines changed: 114 additions & 13 deletions

File tree

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import { test } from 'vitest';
2+
import assert from 'node:assert/strict';
3+
import { capture, recycledCellSnapshot } from './scroll-edge-state-fixtures.ts';
4+
5+
test('surface signature changes when a recycled cell keeps its slot but changes text', async () => {
6+
const before = await capture(recycledCellSnapshot('First story'));
7+
const after = await capture(recycledCellSnapshot('Second story'));
8+
assert.ok(before.fingerprint && after.fingerprint);
9+
assert.notEqual(before.fingerprint, after.fingerprint);
10+
});
11+
12+
test('surface signature is stable when nothing on the container moves', async () => {
13+
const first = await capture(recycledCellSnapshot('First story'));
14+
const second = await capture(recycledCellSnapshot('First story'));
15+
assert.ok(first.fingerprint);
16+
assert.equal(first.fingerprint, second.fingerprint);
17+
});

‎packages/capture-kit/src/snapshot/__tests__/scroll-edge-state-fixtures.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,32 @@ export function scrollSnapshot(hiddenContentBelow: boolean): SnapshotNode[] {
6666
];
6767
}
6868

69+
/**
70+
* A scrolled container whose one visible cell keeps a stable identifier and slot while only its text
71+
* varies — the recycled-cell shape where identity alone would hide real progress.
72+
*/
73+
export function recycledCellSnapshot(label: string): SnapshotNode[] {
74+
return [
75+
{
76+
index: 1,
77+
ref: 'e1',
78+
type: 'ScrollView',
79+
label: 'Feed',
80+
hiddenContentBelow: true,
81+
rect: { x: 0, y: 100, width: 400, height: 600 },
82+
},
83+
{
84+
index: 2,
85+
ref: 'e2',
86+
parentIndex: 1,
87+
type: 'StaticText',
88+
identifier: 'cell-0',
89+
label,
90+
rect: { x: 0, y: 640, width: 400, height: 56 },
91+
},
92+
];
93+
}
94+
6995
export async function captureThrows(scope: string | undefined): Promise<AppError> {
7096
try {
7197
await captureScrollEdgeState({

‎packages/capture-kit/src/snapshot/__tests__/scroll-edge-state-pass-orchestration.test.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,34 @@ test('runScrollEdgePasses: a moving fingerprint never trips no-progress and scro
358358
assert.equal(result.passes, 5);
359359
});
360360

361+
test('runScrollEdgePasses: a fresh surface signature continues after identical passes', async () => {
362+
let scrollCalls = 0;
363+
// The surface sits at `a` for three captures, then a pass finally reveals `b`. `b` is a signature
364+
// the window has not seen, so the loop keeps going rather than stopping as no-progress on the pass
365+
// that actually advanced.
366+
const captures = [
367+
{ canScroll: true, fingerprint: 'a' },
368+
{ canScroll: true, fingerprint: 'a' },
369+
{ canScroll: true, fingerprint: 'a' },
370+
{ canScroll: true, fingerprint: 'b' },
371+
{ canScroll: false, fingerprint: 'b' },
372+
];
373+
let index = 0;
374+
const result = await runScrollEdgePasses({
375+
edge: 'bottom',
376+
captureState: async () => ({
377+
emptySnapshot: false,
378+
...(captures[index++] ?? { canScroll: false }),
379+
}),
380+
scroll: async () => {
381+
scrollCalls += 1;
382+
return undefined;
383+
},
384+
});
385+
assert.equal(result.passes, 4);
386+
assert.equal(scrollCalls, 4);
387+
});
388+
361389
test('unique container scope is retained across edge pass captures', async () => {
362390
const scopes: Array<string | undefined> = [];
363391
const snapshots = [scrollSnapshot(true), scrollSnapshot(true), scrollSnapshot(false)];
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { test } from 'vitest';
2+
import assert from 'node:assert/strict';
3+
import { scrollSurfaceIsStuck } from '../scroll-edge-state.ts';
4+
5+
const cases: ReadonlyArray<readonly [string, readonly string[], boolean]> = [
6+
['identical captures', ['a', 'a', 'a', 'a'], true],
7+
['rubber-band alternation', ['bottom', 'top', 'bottom', 'top'], true],
8+
['two stable signatures', ['a', 'b', 'a', 'b'], true],
9+
['parked after one detour', ['a', 'b', 'a', 'a'], true],
10+
// A newest signature the window has not seen is a pass that just made progress: keep scrolling.
11+
['fresh progress after identical passes', ['a', 'a', 'a', 'b'], false],
12+
['three distinct signatures', ['a', 'b', 'c', 'a'], false],
13+
['below the minimum length', ['a', 'a', 'a'], false],
14+
];
15+
16+
for (const [label, signatures, expected] of cases) {
17+
test(`scrollSurfaceIsStuck: ${label}`, () => {
18+
assert.equal(scrollSurfaceIsStuck(signatures), expected);
19+
});
20+
}

‎packages/capture-kit/src/snapshot/scroll-edge-state.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -81,15 +81,19 @@ export async function scrollSurfaceFingerprint(
8181

8282
/**
8383
* Has a scroll stopped making progress? True once `recentSignatures` holds at least
84-
* `SCROLL_EDGE_STUCK_MIN_LENGTH` captures within the last `SCROLL_EDGE_STUCK_WINDOW` and no more
85-
* than two of them are distinct — the signature of a container that ignores the gesture or is only
86-
* rubber-banding. Callers cap `recentSignatures` to the window. Exported so `scroll --until` reuses
87-
* the one definition of "stuck" rather than a second copy.
84+
* `SCROLL_EDGE_STUCK_MIN_LENGTH` captures within the last `SCROLL_EDGE_STUCK_WINDOW`, no more than
85+
* two are distinct, AND the newest capture is one the window already showed — the signature of a
86+
* container that ignores the gesture or only rubber-bands. A newest signature the window has not seen
87+
* is a pass that just made real progress, so `A,A,A,B` continues while `A,B,A,B` is stuck. Callers
88+
* cap `recentSignatures` to the window. Exported so `scroll --until` reuses the one definition of
89+
* "stuck" rather than a second copy.
8890
*/
8991
export function scrollSurfaceIsStuck(recentSignatures: readonly string[]): boolean {
9092
if (recentSignatures.length < SCROLL_EDGE_STUCK_MIN_LENGTH) return false;
9193
const window = recentSignatures.slice(-SCROLL_EDGE_STUCK_WINDOW);
92-
return new Set(window).size <= SCROLL_EDGE_STUCK_MAX_DISTINCT;
94+
if (new Set(window).size > SCROLL_EDGE_STUCK_MAX_DISTINCT) return false;
95+
const newest = window.at(-1);
96+
return newest !== undefined && window.slice(0, -1).includes(newest);
9397
}
9498

9599
/**

‎packages/capture-kit/src/snapshot/scroll-edge-state/selection.ts‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,13 +210,14 @@ function isUsableRect(rect: SnapshotNode['rect']): rect is NonNullable<SnapshotN
210210

211211
/**
212212
* A stable signature of the content the container currently shows, built from the container's own
213-
* descendants (via the parent chain), each keyed by identity and rounded position. Scrolling shifts
214-
* descendant rects, so a real pass always changes it; a gesture the container ignores leaves it
215-
* byte-identical. Descendant scoping — not geometric overlap — is the point: it excludes the
216-
* keyboard, its typing predictions, and scroll-indicator overlays, which are not under the scroller
217-
* and would otherwise flicker and mask a stuck container. Rounded to whole pixels so layout jitter
218-
* never fakes movement; a false "unchanged" only defers to a later pass or the pass limit. An empty
219-
* string (a container with no positioned descendants) leaves both loops to their edge/pass signals.
213+
* descendants (via the parent chain), each keyed by its identity fields and rounded position.
214+
* Scrolling shifts descendant rects, and a recycled cell that keeps its slot while its text changes
215+
* re-keys, so a real pass always changes it; a gesture the container ignores leaves it byte-identical.
216+
* Descendant scoping — not geometric overlap — is the point: it excludes the keyboard, its typing
217+
* predictions, and scroll-indicator overlays, which are not under the scroller and would otherwise
218+
* flicker and mask a stuck container. Rounded to whole pixels so layout jitter never fakes movement;
219+
* a false "unchanged" only defers to a later pass or the pass limit. An empty string (a container with
220+
* no positioned descendants) leaves both loops to their edge/pass signals.
220221
*/
221222
function buildSurfaceFingerprint(container: SnapshotNode, nodes: SnapshotNode[]): string {
222223
const byIndex = new Map<number, SnapshotNode>();
@@ -236,7 +237,12 @@ function buildSurfaceFingerprint(container: SnapshotNode, nodes: SnapshotNode[])
236237
}
237238

238239
function surfaceIdentity(node: SnapshotNode): string {
239-
return (node.identifier ?? node.label ?? node.value ?? '').trim();
240+
// All three content fields, not first-present: a recycled cell keeps its identifier and slot while
241+
// its label/value change, and that text change is exactly the progress the signature must register.
242+
const identifier = (node.identifier ?? '').trim();
243+
const label = (node.label ?? '').trim();
244+
const value = (node.value ?? '').trim();
245+
return `${identifier}${label}${value}`;
240246
}
241247

242248
function isDescendantOf(

0 commit comments

Comments
 (0)