Repository navigation
fix(settle): arm the iOS broad-transition confirmation at any requested quiet window - #3512
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…ts help Review on #3512: - the null answer is a real answer: '??=' reran the chrome classification on every poll for the ordinary full-screen baseline, which is the case the whole point is to keep cheap - the 1500ms floor is iOS XCTest-only and bounded by the settle deadline, so neither the CLI flag nor the MCP schema field may state it as universal - the comment said a short-window caller is held to 'its own window rather than the fixed floor', which is the opposite of what Math.max(requested, 1500) does - the navigation fixtures are compact projections, not the 24-34 nodes a live screen reports; the comment claimed otherwise - the longpress case budgets for the confirmation without asserting it, so it could not catch the baseline failing to reach the loop
|
I found a problem in b8d7540. The narrowing can disarm the dismissal confirmation on some routes, so this is not ready to merge yet. In stable-capture.ts:299, Could the classification be made well defined for every baseline? One option is to have the iOS capture producer set a typed presented-surface fact on The live A/B does not say how steps were targeted. The "dismiss sheet still confirmed" row proves the guard only for the route used, likely Coverage fails in The open inline threads still apply. These two P2 threads still stand: help text states the 1500 ms floor without iOS or the timeout cap and settle test should assert waitedMs. Three P3 threads also still apply. No open thread needs to be resolved as not applicable. A newer head, d969e79, arrived after this review. Its delta looks like it addresses both P2 threads and changes the settle.test.ts lines behind the size check, so please confirm that check at the new head. It does not change |
|
The presented-surface check in this PR is too narrow, so the #1831 regression can come back for some sheet dismissals. I reviewed d969e79 by reading the code. I did not run iOS captures.
The live A/B has one "dismiss sheet" row, and it does not say how it was targeted. Its pageSheet projects to 4 nodes, so it can only exercise the 5-node arm. The original #1831 case (Element) was not re-run. Nothing yet shows that the narrowed trigger still covers the other dismissal routes, or a sheet with more than 5 controls. On the iPhone simulator with the RN test app and the default window, please run each of these at least 4 times: (a) Could the iOS snapshot producer, which already knows the presentation it captured ( Not blocking, and fine to take or leave: the The Coverage check fails on the size ratchet for The five cubic-dev-ai threads (the post-action-observation-grammar claim scope, the |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The latest push (a565d42) only moves a test, so the problem I raised on d969e79 still stands. The new test and the PR's live A/B cover the narrow case. The fix does not yet cover every route that dismisses a modal. The main problem is in stable-capture.ts. The rule I would like the code to satisfy: a screen gets the same presented-surface answer whatever source produced its baseline. That must hold for every Could the simpler route be better? Stamping one boolean on The live runs I asked for are still missing (stable-capture.ts:218). The one 'dismiss sheet' row is a 4-node pageSheet with no route stated, so it only exercises the small-tree arm. Please run on the iPhone simulator with the RN test app at the default window, 4 or more runs each, for these cases. (a) I did not run the new test, so I cannot confirm the claim that it fails on the old loop at waitedMs=304/802. I did not run iOS captures either. The routing finding comes from reading The cubic-dev-ai P3 thread still applies: the comment at settle-dismissal-confirmation.test.ts:26 calls capture 1 selector resolution, but the press targets The checks were still queued or running when I looked, so I cannot say yet whether CI passes. This push touches only unit tests under Before merge, the presented-surface answer must be the same for every baseline source through a typed |
|
Confirmed on device, and the numbers are worse than the reading suggested — the same dismissal loses the confirmation on two of the three routes. iPhone 17 sim (
The sheet is P0 for the fix shape. I then measured what the merge-base arm keys on, on the same screens, both flavors:
Every iOS tree has an So I agree the durable rule is yours: the capture producer stamps a typed presented-surface fact on Two things I need before I take that on: confirmation that the scope should move into the capture producers here rather than a separate issue, and whether you want #3512 to hold the interim flavor-gated rule in the meantime (interactive-projection baselines keep today's behaviour; full-tree baselines fall back to the merge-base arm) or stay as it is and land the producer fact first. I won't add the interim arm without your call, since by the table above it buys route (b)/(c) coverage by re-charging tab switches on those routes. |
|
Adding a third option, with measurements, because it changes the shape of the decision. There is a host-side signal that already satisfies the invariant in the review — "the presented-surface verdict does not depend on how the baseline was captured". It is the app's own navigation chrome, not the node count. iPhone 17 sim, two reps,
Identical verdict per screen in both flavors, which is exactly what the node count failed at (4 vs 6 straddling the bound of 5). It also fixes both broken routes, because the full tree of the sheet is chrome-free too, and it cannot re-arm tab switches: their baseline carries the tab bar, so it never arms regardless of flavor. No native change, no wire field, no golden table. Two failure classes I have not solved and would be shipping:
So the choice is:
My read is that 1 is only defensible as a stepping stone you intend to delete, and its second failure class is the objection — if the over-arm matters, #3470 stays open and 2 is the only answer. Say which and I'll implement it; I'm not adding an arm that can charge every press in a chrome-less app without your call. |
70e3e8f to
356ebec
Compare
|
Thanks for the update. The code at 356ebec still has the problem from the earlier review (#3512 (comment)). The fixes for the test comments, the claim scope, the waitedMs assertion and the The first problem is unchanged and your device run now confirms it. In stable-capture.ts, The second problem is missing evidence for the changed iOS settle path in stable-capture.ts. Routes (b) and (c) are measured and fail, and route (d), a sheet with 6 or more controls, is not reported. The route-level regressions are also missing: Is there a smaller correct design? I did not find one. The chrome arm is smaller, but it trades one inferred rule for another, with the two failure classes you named, and I judged it only from your description and 2-rep table because there is no code for it at head. With a producer-stamped Before merge, the maintainer needs to choose between landing the producer-owned fact and stacking this PR on it, or holding the PR. Smoke Tests also needs to finish green. |
356ebec to
962b838
Compare
…on alone The arming bar was `requestedQuietMs >= DEFAULT_STABLE_QUIET_MS`, so the confirmation that #1831 added to hold a modal dismissal for 1.5s was a function of the window the caller asked for rather than of the transition in front of the loop. Three consequences, all reported as #3470: - `--settle-quiet 499` behaved differently from 500, with no rule that explains the boundary - a caller passing `settleQuietMs: 150` (tester-army/e2e) got no guard at all, on exactly the dismissal the guard exists for - a shorter window read as "trust a coherent transitional tree sooner", which is not what `--settle-quiet` asks for: it asks for a tighter verdict, not a different set of transitions to distrust Arming now reads the transition only, and a confirmed replacement is held for the caller's own window, floored at 1.5s. The trigger stays exactly as on main: a complete-viewport post-action projection or a tiny presented baseline, plus fewer than half the node identities carried over. The six #1831 cases run at 500ms and 150ms alike. The provider-backed scenario is a real dialog dismissal, so it now holds the floor at its 25ms window too, which is what that wait is worth on a runner round trip: measured 1510ms over 6 captures, against 1511ms over 6 for the same scenario at the default window on main — the same verdict at the same price, no longer window-dependent.
962b838 to
6e22190
Compare
Poll cadence derived from the requested window once, before the loop ran, so the 1.5s confirmation this branch extends that window to was spent sampling at the caller's grid. At `--settle-quiet 25` that is 109 capture round trips to reach a verdict the default grid reaches in 10 — a confirmation whose cost is 11x the reads it needs, on the tightest windows where settle is asked for at all. Cadence now follows the window in force. Both directions hold: a tight window still gets a grid that can measure it, and an extended wait gets the default grid rather than the caller's. Pinned by the capture count in the 25ms case: 10 here, 109 with the recompute removed (mutation-checked).
6e22190 to
bee9e1f
Compare
|
Re: #3512 (comment) — the split you asked for is now the shape of the stack, and this PR no longer contains the arm you rejected. What changed here. This head ( Where the narrowing went. #3570 — draft, base On the producer fact. Agreed it is the durable rule, and the draft states the contract you set: On the route-level evidence you asked for. The 4-rep × (a)(b)(c)(d) + tab-switch run is written as a requirement on #3470, and it belongs to the layer that changes which transitions arm — i.e. the draft, once #3565 lands. What this PR owes is evidence for the change it actually makes, which is that arming no longer depends on the window: the (a) and (b) rows above are Gate for this head: |
Gate —
|
| check | result |
|---|---|
format:check |
pass |
lint |
pass |
typecheck (tsc -b) |
pass |
check:layering |
pass |
check:fallow --base origin/fix/3469-ios-keyboard-chrome-interactive |
pass — No issues in 7 changed files (22.23 s) |
build |
pass |
vitest related (7 changed files) |
pass — 506 files / 3,962 tests |
Full vitest --project unit-core separately: 13,372 passed / 1 skipped, and one load-sensitive failure unrelated to this diff — packages/platform-apple/src/snapshot-source/lifecycle.test.ts "request cancellation after dispatch reaps the exact helper before recovery" tripped its target-identity-deadline inside the 13k-test run and passes in isolation (16/16). This layer touches no capture-source code.
CI at this head: all jobs green except the two intentionally-skipped lanes (cubic · AI code reviewer, matrix.name) — including Compatibility & Provenance.
Fallow at the previous head — fixed at the owner, not suppressed. The one gated finding this layer caused was its own: the cadence recompute pushed runStableCaptureLoop to cyclomatic 16 / cognitive 17, past the threshold. The previousQuietMs guard was redundant — requiredQuietMs is a monotonic max, so the branch could only re-derive the same number — and removing it puts the function back under the threshold while the cadence case still pins 10 captures vs 109 at --settle-quiet 25. No baseline entry, no suppression comment.
Two details from the Compatibility & Provenance log, so nobody re-chases them. The job's scope reads 729 changed files vs faf754aa6 rather than this layer's 7, because GitHub's merge ref for a stack whose base branch is itself an open PR against main (59193e095 = merge(faf754aa6 → main@6087be3a)) already carries main's content on its first parent. Fallow's gate then excluded 17 inherited findings on that basis — all 14 dead-code entries (8 unused exports in src/daemon/handlers/__tests__/session-test-harness.ts: mockPrewarmIosRunnerSession, mockNotifyIosRunnerAppRelaunched, mockPrepareIosRunner, mockStopIosRunner, mockDismissMacOsAlert +3; and 6 unused root devDependencies: @types/json-schema, ipaddr.js, pngjs, tar-stream, undici, yauzl) plus the 3 clone groups — every one in a file this layer never touches, and the mock-export set reproduces when the same rule is run directly at origin/main. They are still printed under ● headings, which is what made them look like this PR's.
|
This looks right at bee9e1f. The concerns from 356ebec are fixed. The help text now limits the 1.5 s confirmation to iOS and names the timeout deadline. The stable-capture comment, the tab/push fixtures and the extra presented-surface re-evaluation are cleaned up. One decision is left before merge: the merge order. With this change, a caller with a short window (for example Not blocking: the MCP/Node text at post-action-observation-grammar.ts#L37 should say "1500ms", like the CLI text does. CI is green at bee9e1f (19 checks, none failing), and there are no conflicts. I did not run a simulator or the tests, so the live timings at The six cubic-dev-ai threads are fixed at this head and can be resolved. Five are fixed directly: r4249620115, r4249620160, r4249620174, r4249620186 and r4250614159. r4249620152 is covered because |
|
|
Answering both, on the head you reviewed ( The merge order — the decision, and what actually happened. #3482 and #3512 landed at 10:18:56/57Z, so the question is now retrospective; recording the judgement for #3470's trail. The added cost is confined to callers who pass a sub-500 ms window — in practice the
On the provenance of the live rows — you read it correctly, and here is the precise answer. The press rows (a)/(b) and the tab-switch control were run on the working tree that became |
Part of #3470. The narrowing — arming on which transitions, rather than at which window — is not in this PR: it is the draft that sits on top of this stack and is blocked by #3565. See "Not in this PR" at the bottom.
Stacks on #3482 (
fix/3469-ios-keyboard-chrome-interactive) — it builds on the chrome-free comparison that PR adds. Retarget tomainonce #3482 lands.What changed
The confirmation
#1831added for a broad screen replacement armed on two things, and only one of them describes the transition the rule exists for. The second wasrequestedQuietMs >= DEFAULT_STABLE_QUIET_MS, which made the guard a function of the window the caller asked for:--settle-quiet 499behaved differently from 500, with no rule that explains the boundary;settleQuietMs: 150(tester-army/e2e) got no guard at all, on exactly the dismissal the guard exists for;--settle-quietasks for. It asks for a tighter verdict, not for a different set of transitions to distrust.Arming now reads the transition only, and a confirmed replacement is held for the caller's own window, floored at 1.5 s. The trigger itself is unchanged from
main: a complete-viewport post-action projection or a tiny presented baseline, plus fewer than half the node identities carried over. No narrowing, no new heuristic — this PR only removes the window coupling, so it can land and be reasoned about on its own.One consequence stated up front, because it is the point of the split: at the default window a tab switch or a push already pays the 1.5 s floor on
main, and still does here. What this layer adds is that a caller asking for a shorter window now pays it too (measured below). That is the correct reading of--settle-quiet, and it is also what makes removing the cost in the right places urgent — which is the successor layer's job, not this one's.Verification
#1831's six loop-level cases now run at both 500 ms and 150 ms, and a caller-route case pins the same floor on the observation a caller reads (settle-confirmation-floor.test.ts): the response reports the window the caller asked for, while the confirmed replacement sets the wait. Restoring therequestedQuietMs >= DEFAULT_STABLE_QUIET_MSbar fails exactly the short-window cases and nothing else, so the guard cannot silently switch itself off at a shorter window again.Cadence follows the window in force (second commit). Without it the extended wait was sampled on the caller's tight grid: at
--settle-quiet 25that is 109 capture round trips to reach a verdict the default grid reaches in 10 — a confirmation costing 11x the reads it needs, on the tightest windows where settle is asked for at all. Mutation-checked: removing the recompute fails the new capture-count assertion (109 vs 10) and nothing else in the file.Live A/B (iPhone 18 Pro simulator,
BDA9B772, RN test app, purpose-specific session)Sheet = Settings → Open automation lab → Open automation sheet (
pageSheet).lateisdiff snapshot -iafter settle returned;late=0means settle answered on the tree the UI kept.waitedMs@--settle-quiet 150press 'label="Close automation sheet"' --settlelate=0snapshotthenpress @ref --settlelate=0press <tab> --settle(control, no dismissal)mainThe generic-route case (
back --system --settle, whose baseline is the session's stored full tree) is the third dismissal route and the one the reviewer's table marks as losing coverage under the narrowing. It could not be exercised as a dismissal on this device —back --systemdoes not dismiss thispageSheeton the iPhone 18 Pro, verified across three independent runs (the sheet is still up afterwards). It was exercised over a transitionbackcan perform there, with a stored full-tree baseline and a genuinely broad arrival (the settle diff reports 24 additions / 25 removals / 3 unchanged): 1840 ms over 5 captures and 1879 ms over 5, i.e. armed, against 506–583 ms over 2 captures on the narrowing head for the same arrival. Those four numbers were taken on the stack's second-layer build, which carries this layer's arming rule verbatim.And the control row is not free — it is the cost the successor layer removes, measured so the reviewer can see exactly what this layer costs a short-window caller on ordinary navigation.
The provider-backed scenario is itself a dialog dismissal, so it now holds the floor at its 25 ms window too: 1510 ms over 6 captures, against 1511 ms over 6 for the same scenario at the default window on the base — same verdict, same price, no longer window-dependent. Its capture-count budget becomes a floor plus an explicit
waitedMs >= 1_500, and the test gains the parallel-scenario timeout its 1.5 s wait needs (that scenario already timed out at base under this machine's load, and the body it sits in reindents as a result — ~210 of this PR's lines are that formatter-forced reindent).Gates
pnpm format,pnpm lint,tsc --noEmitclean.vitest --project unit-core: 13,373 passed / 1 skipped (a first full run tripped one deadline-sensitivesnapshot-source/lifecycle.test.tscase under host load; it passes in isolation and on the clean re-run above, and this layer touches no capture-source code).interaction-contract(68) andprovider-integrationgreen; twointeraction-contractfailures seen mid-branch were confirmed present at the base commit and pass at this head.pnpm check:affected --run --base origin/fix/3469-ios-keyboard-chrome-interactiveatbee9e1f34: all runnable checks passed (exit 0) — 506 related test files / 3,962 tests, Fallow clean on the 7 changed files — held under the machine-wide lock. Counts in the gate comment on this PR.Not in this PR
The narrowing — arming on "the baseline shows a surface presented over app content" instead of on any broad screen replacement — is split out. Its node-count rule keys on how the baseline was captured (the same sheet projects 4 nodes interactively and 6 in a full tree), so it loses dismissal confirmation on 2 of 3 routes; that is measured in #3512 (comment) and the durable rule is a producer-stamped
presentedSurfacefact, which is #3565. The narrowing now sits on top of this stack as draft #3570, blocked by #3565, so this decoupling can land without waiting for it — and so no head of this branch ever ships ashowsPresentedSurfacearm.