Skip to content

Commit 347ea1f

Browse files
committed
fix: make settle tail list real actionable targets
Post-merge benchmark of #1167 (React Navigation prevent-remove flow, iOS sim, claude-haiku/sonnet) found the unchanged-interactive tail regressing to chrome-only noise in exactly the cases it was built for: - buildSettleTailEntries required `hittable === true`, stricter than what `snapshot -i` itself shows for the same interactive-only capture. Right after a dismiss animation, real buttons commonly report `hittable: false`/undefined while application/window containers pass, so the tail surfaced two useless chrome lines and dropped the actionable button. Fixed by dropping the hittable requirement and excluding structural application/window roles instead. - withoutKeyboardKeys only stripped `Key` nodes; real keyboard chrome (shift/Emoji/return/Dictate/Next keyboard) are XCUIElementTypeButton nodes and leaked through as fresh added-line refs, which suppressed the tail trigger for exactly the post-fill case it exists for. Fixed by detecting the whole keyboard subtree structurally (via parentIndex, not a locale-fragile label list): descendants collapse out of the diff, and the keyboard container's own ref no longer counts as a "meaningful" added ref for the trigger decision. Nobody presses shift via settle diff refs, so collapsing keyboard chrome does not block a user from explicitly targeting the keyboard itself.
1 parent 5c08f16 commit 347ea1f

4 files changed

Lines changed: 510 additions & 39 deletions

File tree

‎src/commands/interaction/runtime/settle.test.ts‎

Lines changed: 224 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -313,10 +313,12 @@ test('the settled diff line list is bounded with a truncation marker', async ()
313313
assert.equal(diff.truncated, true);
314314
});
315315

316-
test('keyboard Key nodes never spend the settled diff budget', async () => {
316+
test('keyboard Key nodes and chrome buttons never spend the settled diff budget', async () => {
317317
const before = buttonSnapshot();
318-
// A fill-style settled tree: a summoned keyboard (container + keys) plus the
319-
// content change the agent actually cares about.
318+
// A fill-style settled tree: a summoned keyboard (container + keys + real
319+
// XCUIElementTypeButton chrome like shift/Emoji/return/Dictate/Next
320+
// keyboard — NOT Key nodes) plus the content change the agent actually
321+
// cares about.
320322
const keyboardTree = makeSnapshotState([
321323
{
322324
index: 0,
@@ -343,8 +345,17 @@ test('keyboard Key nodes never spend the settled diff budget', async () => {
343345
rect: { x: key * 10, y: 520, width: 10, height: 40 },
344346
hittable: true,
345347
})),
346-
...['Next', 'Back', 'Home'].map((label, extra) => ({
348+
...['shift', 'Emoji', 'return', 'Dictate', 'Next keyboard'].map((label, extra) => ({
347349
index: extra + 28,
350+
depth: 1,
351+
parentIndex: 1,
352+
type: 'Button',
353+
label,
354+
rect: { x: extra * 40, y: 780, width: 40, height: 40 },
355+
hittable: true,
356+
})),
357+
...['Next', 'Back', 'Home'].map((label, extra) => ({
358+
index: extra + 33,
348359
depth: 0,
349360
type: 'Button',
350361
label,
@@ -371,11 +382,102 @@ test('keyboard Key nodes never spend the settled diff budget', async () => {
371382
const texts = diff.lines.map((line) => line.text).join('\n');
372383
assert.match(texts, /Results for alpenglow/);
373384
assert.match(texts, /keyboard/);
374-
// The container line survives as the keyboard signal; individual keys do not.
385+
// The container line survives as the keyboard signal; individual keys and
386+
// chrome buttons (real XCUIElementTypeButton nodes, not Key nodes) do not.
375387
assert.ok(!diff.lines.some((line) => /\[key\]/.test(line.text)));
388+
assert.ok(!diff.lines.some((line) => /shift|Emoji|return|Dictate|Next keyboard/.test(line.text)));
376389
assert.equal(diff.summary.additions, 5);
377390
});
378391

392+
test('keyboard-only changes (container + chrome) do not suppress the settle tail trigger', async () => {
393+
// The field and its screen buttons are unchanged by the fill itself; the
394+
// keyboard summoning is the only tree change. Before #1167's fix, the
395+
// chrome buttons' fresh added-line refs (or, after the diff-only fix, the
396+
// container's own added ref) would read as "the diff already handed back a
397+
// target" and suppress the tail — leaving the still-relevant screen
398+
// buttons invisible.
399+
const controls = [
400+
{
401+
index: 0,
402+
depth: 0,
403+
type: 'TextField',
404+
label: 'Search',
405+
rect: { x: 0, y: 0, width: 200, height: 40 },
406+
hittable: true,
407+
},
408+
{
409+
index: 1,
410+
depth: 0,
411+
type: 'Button',
412+
label: 'Cancel',
413+
rect: { x: 0, y: 50, width: 100, height: 40 },
414+
hittable: true,
415+
},
416+
{
417+
index: 2,
418+
depth: 0,
419+
type: 'Button',
420+
label: 'Search now',
421+
rect: { x: 110, y: 50, width: 100, height: 40 },
422+
hittable: true,
423+
},
424+
];
425+
const before = makeSnapshotState(controls);
426+
const settledTree = makeSnapshotState([
427+
...controls,
428+
{
429+
index: 3,
430+
depth: 0,
431+
type: 'Keyboard',
432+
label: 'keyboard',
433+
rect: { x: 0, y: 500, width: 400, height: 300 },
434+
hittable: true,
435+
},
436+
{
437+
index: 4,
438+
depth: 1,
439+
parentIndex: 3,
440+
type: 'Button',
441+
label: 'shift',
442+
rect: { x: 0, y: 520, width: 30, height: 40 },
443+
hittable: true,
444+
},
445+
{
446+
index: 5,
447+
depth: 1,
448+
parentIndex: 3,
449+
type: 'Button',
450+
label: 'return',
451+
rect: { x: 350, y: 520, width: 50, height: 40 },
452+
hittable: true,
453+
},
454+
]);
455+
let captures = 0;
456+
const device = createSettleDevice({
457+
stored: before,
458+
captureSnapshot: () => {
459+
captures += 1;
460+
return { snapshot: captures === 1 ? before : settledTree };
461+
},
462+
});
463+
464+
const result = await device.interactions.fill(selector('label=Search'), 'hello', {
465+
session: 'default',
466+
settle: {},
467+
});
468+
469+
const settle = result.settle;
470+
assert.ok(settle);
471+
// Only the keyboard container line is added; its chrome buttons collapse.
472+
assert.equal(settle.diff?.summary.additions, 1);
473+
assert.ok(!settle.diff?.lines.some((line) => /shift|return/i.test(line.text)));
474+
assert.ok(settle.tail, 'keyboard-only additions must not suppress the tail');
475+
assert.deepEqual(
476+
settle.tail?.map((entry) => entry.label),
477+
['Search', 'Cancel', 'Search now'],
478+
);
479+
});
480+
379481
test('added lines win diff-budget slots over removals under truncation', async () => {
380482
// 120 removals precede the additions positionally; the fresh-ref additions
381483
// must still survive the 80-line cap.
@@ -514,13 +616,19 @@ test('a removals-only settled diff (modal dismiss) attaches an unchanged interac
514616
assert.ok(settle);
515617
assert.deepEqual(settle.diff?.summary, { additions: 0, removals: 3, unchanged: 2 });
516618
assert.ok(!settle.diff?.lines.some((line) => line.kind === 'added'));
517-
// The StaticText survives but is not interactive; only the hittable button
518-
// makes the tail.
519-
assert.deepEqual(settle.tail, [{ ref: 'e1', role: 'button', label: 'Add to cart' }]);
619+
// The tail matches `snapshot -i`'s own bar for the settled tree: both
620+
// surviving elements are candidates, `hittable` is not required (#1167 post-
621+
// merge benchmark: real buttons commonly report `hittable: false`/undefined
622+
// right after a dismiss animation, so requiring it silently dropped exactly
623+
// the elements the tail exists to surface).
624+
assert.deepEqual(settle.tail, [
625+
{ ref: 'e1', role: 'button', label: 'Add to cart' },
626+
{ ref: 'e2', role: 'text', label: 'Price: $12' },
627+
]);
520628
assert.equal(settle.tailTruncated, undefined);
521629
});
522630

523-
test('the unchanged interactive tail excludes non-hittable and covered candidates', async () => {
631+
test('the unchanged interactive tail includes non-hittable candidates but excludes covered ones', async () => {
524632
// Every surviving node's attributes (hittable/blocked-relevant fields) must
525633
// match a `before` line exactly, or it would read as an added line and
526634
// suppress the tail entirely (see the trigger-condition test above).
@@ -594,9 +702,73 @@ test('the unchanged interactive tail excludes non-hittable and covered candidate
594702
const settle = result.settle;
595703
assert.ok(settle);
596704
assert.equal(settle.diff?.summary.additions, 0);
597-
// Not hittable, then covered, are both excluded; only the plain hittable
598-
// button remains.
599-
assert.deepEqual(settle.tail, [{ ref: 'e3', role: 'button', label: 'Share' }]);
705+
// `hittable: false` no longer excludes a candidate (matches `snapshot -i`'s
706+
// own bar); `interactionBlocked: 'covered'` still does.
707+
assert.deepEqual(settle.tail, [
708+
{ ref: 'e1', role: 'button', label: 'Add to cart' },
709+
{ ref: 'e3', role: 'button', label: 'Share' },
710+
]);
711+
});
712+
713+
test('the unchanged interactive tail excludes structural application/window chrome', async () => {
714+
// The flagship #1167 post-merge benchmark case: after a dialog dismiss, the
715+
// old `hittable === true` bar let a lone application/window pair through
716+
// (both routinely report `hittable: true` on their full-screen root frame)
717+
// while excluding the real button because its `hittable` state was
718+
// undefined right after the dismiss animation.
719+
const before = makeSnapshotState([
720+
{ index: 0, depth: 0, type: 'Application', label: 'React Navigation Example' },
721+
{ index: 1, depth: 0, type: 'Window' },
722+
{
723+
index: 2,
724+
depth: 0,
725+
type: 'Button',
726+
label: 'Discard and go back',
727+
rect: { x: 10, y: 20, width: 200, height: 40 },
728+
},
729+
{
730+
index: 3,
731+
depth: 0,
732+
type: 'Button',
733+
label: 'Cancel',
734+
rect: { x: 10, y: 80, width: 200, height: 40 },
735+
},
736+
]);
737+
// The dialog's own Cancel button is dismissed with it; the application,
738+
// window, and the real actionable button survive unchanged. None of the
739+
// three carry `hittable: true` here, mirroring the real post-dismiss
740+
// capture shape.
741+
const after = makeSnapshotState([
742+
{ index: 0, depth: 0, type: 'Application', label: 'React Navigation Example' },
743+
{ index: 1, depth: 0, type: 'Window' },
744+
{
745+
index: 2,
746+
depth: 0,
747+
type: 'Button',
748+
label: 'Discard and go back',
749+
rect: { x: 10, y: 20, width: 200, height: 40 },
750+
},
751+
]);
752+
let captures = 0;
753+
const device = createSettleDevice({
754+
stored: before,
755+
captureSnapshot: () => {
756+
captures += 1;
757+
return { snapshot: captures === 1 ? before : after };
758+
},
759+
});
760+
761+
const result = await device.interactions.press(selector('label=Cancel'), {
762+
session: 'default',
763+
settle: {},
764+
});
765+
766+
const settle = result.settle;
767+
assert.ok(settle);
768+
assert.equal(settle.diff?.summary.additions, 0);
769+
// Application/window chrome is dropped; the real button is the only entry,
770+
// despite carrying no `hittable: true` flag.
771+
assert.deepEqual(settle.tail, [{ ref: 'e3', role: 'button', label: 'Discard and go back' }]);
600772
});
601773

602774
test('the unchanged interactive tail is capped with a truncation marker', async () => {
@@ -657,3 +829,43 @@ test('buildSettleTailEntries dedups candidates already carrying an excluded ref'
657829

658830
assert.deepEqual(result.tail, [{ ref: 'e2', role: 'button', label: 'Share' }]);
659831
});
832+
833+
test('buildSettleTailEntries drops application/window chrome and does not require hittable', () => {
834+
const settledNodes = makeSnapshotState([
835+
{ index: 0, depth: 0, type: 'Application', label: 'Example' },
836+
{ index: 1, depth: 0, type: 'Window' },
837+
{
838+
index: 2,
839+
depth: 0,
840+
type: 'Button',
841+
label: 'Discard and go back',
842+
rect: { x: 10, y: 20, width: 100, height: 40 },
843+
// Deliberately no `hittable` field, mirroring a real post-dismiss
844+
// capture: the tail bar no longer requires `hittable === true`.
845+
},
846+
]).nodes;
847+
848+
const result = buildSettleTailEntries(settledNodes, new Set());
849+
850+
assert.deepEqual(result.tail, [{ ref: 'e3', role: 'button', label: 'Discard and go back' }]);
851+
});
852+
853+
test('buildSettleTailEntries drops the keyboard container and its chrome descendants', () => {
854+
const settledNodes = makeSnapshotState([
855+
{
856+
index: 0,
857+
depth: 0,
858+
type: 'Button',
859+
label: 'Send',
860+
rect: { x: 10, y: 20, width: 100, height: 40 },
861+
hittable: true,
862+
},
863+
{ index: 1, depth: 0, type: 'Keyboard', label: 'keyboard' },
864+
{ index: 2, depth: 1, parentIndex: 1, type: 'Key', label: 'q' },
865+
{ index: 3, depth: 1, parentIndex: 1, type: 'Button', label: 'shift' },
866+
]).nodes;
867+
868+
const result = buildSettleTailEntries(settledNodes, new Set());
869+
870+
assert.deepEqual(result.tail, [{ ref: 'e1', role: 'button', label: 'Send' }]);
871+
});

0 commit comments

Comments
 (0)