Repository navigation
Conversation
Size Report
Startup median (7 runs, lower is better):
|
Gate —
|
|
I found two problems at 183801d, so this PR should not merge as it stands. CI is still pending: Smoke Tests is queued, and since this change alters iOS settle arming, a later failure there could not be ruled unrelated without its log. The arming rule in The tests do not catch this. The daemon-route test in settle-observation.test.ts#L21 used a 3-node Continue/Cancel dialog and asserted Could this layer skip the node-count classifier entirely? If #3565's typed I read the fixtures against the parent's arm but did not run the navigation table or mutation check. The device numbers are yours and I did not reproduce them. They support the finding rather than clear it. Before merge, #3565 must land, then this layer needs the rewrite above and the interactive-versus-full parity test. |
b78640a to
e8bcff3
Compare
183801d to
3a25bb2
Compare
… on screen replacement DRAFT / BLOCKED BY #3565 — do not merge. The rule below reads a property of the baseline CAPTURE, and #3565 replaces it with a fact stamped by the capture producer. Kept as the reviewed starting point and the device evidence, not as a shippable rule. The layer under this one arms on a complete-viewport post-action projection plus a <50% identity overlap. A tab switch and a push are exactly that: both screens project completely and the rows of one tab are not the rows of the next, so ordinary navigation pays the 1.5s floor (#3470's whole complaint). This narrows the trigger to the pre-action tree showing a surface presented over app content, which an interactive capture reports as the surface alone: a presented pageSheet projects 4 nodes, a system permission prompt 5 nodes under its own Alert root, while an ordinary screen of the same app projects 24+ including its nav or tab bar. Whether the surface was actually replaced stays per-capture. Why it is still a draft: on device the same automation sheet projects 4 nodes through an interactive capture and 6 through a full one, so arming survives only on the route whose baseline happens to be the interactive flavour. A dismissal preceded by a plain `snapshot`, or taken on the generic `back` route, loses the #1831 coverage this rule exists to keep. The chrome heuristic was considered and rejected: it regresses modals that carry their own nav bar against `main`. The durable rule is a typed `presentedSurface` fact from the capture producer (#3565); reading it must keep this parent's arming when the producer answers 'unknown', and the count bound and the Alert special case both go with `showsPresentedSurface`. The three navigation cases are mutation-checked against the parent's arming: restoring the viewport-completeness arm fails exactly those three and nothing else.
e8bcff3 to
84f170e
Compare
3a25bb2 to
76bbe24
Compare
|
The rebase onto main at 76bbe24 leaves the two problems from the earlier review (183801d) in place, so this still needs changes before it can merge. The arming rule is still a node count, which gives different answers for different views of the same screen. In stable-capture.ts, In settle-observation.test.ts, the daemon-route dismissal case no longer asserts For live evidence, after the rebase please run at least 4 reps each on an iPhone simulator with the RN test app. The routes are (a) a selector press of close, (b) a plain I read the code only. I did not run the suites or reproduce the device timings. #3565 is not merged yet, so I could not check how the rewrite will consume the fact. All CI checks were still queued or running when I looked, and the Integration Tests, Coverage and iOS Smoke Tests jobs overlap this change, so I can't rule out a failure there yet. Before merge, #3565 must land, then the arming rewrite, the two tests and the device runs above. |
|
Confirming the disposition, taking the two test asks now, and answering your closing question. Head since: Stays a draft, and gets rewritten, not repaired. When #3565 lands, this layer becomes The rebase you asked for is not possible yet: #3565 is still an open issue — no PR, no branch on the remote — so there is nothing to rebase onto and no Answer to "is the count-based rule needed beyond that?" — no. I kept it only because the fact does not exist yet, and the two heuristics that could substitute for it both fail the way the count bound does. Measured on the same screens: Your line estimate understates how small the change is, which only helps the case: the rule in production code is 19 non-doc lines ( Both test asks are in, and I measured them rather than writing them blind. Reproduced through the generic
One deviation to flag, since it inverts your instruction. You asked for a case that fails at this head; as a plain Evidence this will carry when it leaves draft — on an iPhone simulator with the RN test app, at least 4 reps each of (a) a selector press of close, (b) a plain And noted on your own verification boundary: the device rows here are mine and unreproduced, the navigation table and mutation check were read not run. The parity reproduction above is a unit run on the injected clock, not a device run — it establishes the arming divergence, not the wall-clock behaviour on a real dismissal. |
…n, and pin the parity gap The review on this PR found that padding the provider-scenario dialog past the arming rule's size bound (SHARED_SCREEN_NODES) silently removed the only test that checked the 1.5s confirmation through the daemon: the transition stopped being broad (4 of 6 identities survive), so `waitedMs >= 1_500` had to become `captures in 2..3`. Revert that padding, which restores the unpadded dialog and its original assertion; all four provider-scenario cases pass on this rule. Add the parity case the review asked for: the same presented sheet dismissed from an interactive baseline (4 nodes, as `snapshot -i` projects it) and from a full-tree read of the same screen (6 nodes), both asserted to confirm. Measured on the injected clock, the interactive baseline confirms for 1,502ms and the full-tree baseline for 27ms with `settled: true` either way — #1831's guard lost on the route whose baseline happens to be the richer flavour. Marked `fails`, so the lane stays green and the case turns red ("expected to fail but passed") the moment #3565's `presentedSurface` replaces the count bound: that red is the signal to drop the marker, and the assertion is the rewrite's acceptance test.
e43dd7a to
06a4c5b
Compare
|
The red What actually failed was The requested parity case is a good test in the wrong file. That suite was already 801 lines and the 1,000-line tripwire lets an over-tripwire test file hold its size, not grow, so adding 134 lines to it was never allowed. The same run reports Fixed the way the finding names rather than by shrinking the assertions: the three raw trees the case needs (the sheet as an interactive capture projects it, the same screen as a full read, and the app screen a dismissal lands on) now live in Re-pushed as the layer's single commit |
|
The new commit 06a4c5b only adds tests, so the arming defect from the earlier review (76bbe24) is still there, and the required simulator runs have not been taken. CI is pending: both Smoke Tests jobs are queued with no log yet, and since this PR changes iOS --settle arming, a later smoke failure could not be ruled unrelated. I did not run the generic-settle suite, did not reproduce the device timings, and could not check the rewrite against #3565/#3577 because they are not merged. The timings and the 1,502 ms / 27 ms split in the parity case are your numbers. The ratchet's "801 at merge-base" also disagrees with the 902 lines I see at 84f170e, and I did not run the ratchet script. No conflicts. The main problem is in stable-capture.ts. This change also needs device proof on the rewritten rule, because the tables in the body show the state before it. Please run on an iPhone simulator with the RN test app, at least 4 reps each of: (a) selector press of close, (b) plain Not blocking: the |
Blocked by #3565. Closes #3470. Third layer of the #3470 stack: #3512 (merged) → #3522 → this PR.
Do not merge. This layer holds the narrowing's shape and its device evidence. It will be rewritten to read #3565's producer-stamped
presentedSurfacefact instead of the node-count rule below, andpresentedSurface: 'unknown'will keep this parent's arming — for the trigger itself that ismain's rule — so a producer that has not learned to answer degrades to #3512's behaviour and never to "no confirmation".What this layer does
The layer under it (#3512) decoupled the confirmation from the requested window but left
main's trigger in place: it arms whenever the post-action capture carries a complete viewport projection and shares under half its node identities with the pre-action tree. A tab switch or a push is exactly that — both screens project completely, and the rows of one tab are not the rows of the next — so ordinary navigation keeps paying the 1.5 s floor. That is #3470's actual complaint, and it is what this layer narrows.The trigger becomes the pre-action tree showing a surface presented over app content, which an interactive capture reports as the surface alone. Live ground truth from the iPhone 18 Pro session below: a presented
pageSheetprojects 4 nodes and a system permission prompt 5 nodes under its ownAlertroot, while an ordinary screen of the same app projects 24+ nodes including its own nav or tab bar. Whether the surface was actually replaced stays per-capture.Why it stays a draft
The rule keys on how the baseline was captured, not on what the screen is. Measured on device (#3512 review, iPhone 17, four reps per route,
settle.quietMs/waitedMsas the signal):waitedMs@--settle-quiet 150press 'label="Close automation sheet"' --settlelate=0snapshotthenpress @ref --settleback --system --settleover a stored full treeThe sheet is
pageSheet; the full flavour carries the same surface as the interactive one plus a depth-2 duplicate of each label at the same rect, so 4 → 6 nodes crosses the bound of 5 and the verdict flips on the flavour alone. Route (b) needs nothing exotic — one extra label child in the app's own sheet flips it. Dismissal confirmation, which is the #1831 coverage this rule exists to keep, is therefore lost on 2 of 3 routes.Re-measured at this head on the iPhone 18 Pro (
BDA9B772), same scenario, three reps per route, the only variable being which branch's build was installed:snapshotthen@refpress (full baseline, 6 nodes)Both runs were taken before the stack's restacks; the rule and the tests that pinned those numbers are byte-identical to the head they were taken at (
git diff 183801de7 HEADoverstable-capture.ts,stable-capture.fixtures.tsandstable-capture.test.tsis empty), so they describe this head.The generic-route
backcase gives the cleanest A/B, because there the loop's own diff says which shape arrived. When the arrival is broad (24 additions / 25 removals / 3 unchanged), the parent arms and this head does not, on the same device and the same build path:waitedMs/ capturesSo the same broad arrival that costs the parent the floor's 1.5 s costs this head two captures: the confirmation is gone on a transition that genuinely needs it, not merely on navigation that does not.
Two caveats I want on the record rather than buried:
late=0, sheet gone). What the narrowing removes is the guard — the 1.5 s hold that catches a coherent-but-transitional tree — so the exposure is a wrongsettled: trueon a slower dismissal, not a wrong answer in this run.back --systemdoes not dismiss thispageSheeton the iPhone 18 Pro (the sheet is still up after the command, three independent runs), so the dismissal form of the reviewer's route (c) could not be exercised on this device; its numbers above are the reviewer's iPhone 17 measurement and I am carrying them, not confirming them. The generic-route numbers are mine.The reviewer also measured what
main's viewport-root arm keys on, on the same screens, both flavours: every iOS tree has anApplicationroot at0,0,402x874, presented or not, interactive or full. So restoring that arm for full-tree baselines re-arms the tab-switch and push cases exactly as at the merge base — it relocates the #3470 cost onto the routes that take a plainsnapshotfirst, rather than fixing it. The chrome heuristic was considered and rejected for the same class of reason: it regresses modals that carry their own nav bar, relative tomain.The durable rule (#3565)
The producer stamps a typed presented-surface fact and this layer reads it:
runStableCaptureLooptakes the baseline capture rather than bare nodes;simulator-ax-bridge—apple/snapshot-bridge/*.m, which today reports no window or modality at all — and the runner has its own presentation code), plus the wire type and the cross-language golden table undercontracts/fixtures/;presentedSurface: 'unknown'keeps this parent's arming (i.e.main's trigger, decoupled from the requested window by fix(settle): arm the iOS broad-transition confirmation at any requested quiet window #3512), so the fact can only ever narrow the guard, never switch it off;showsPresentedSurface,isBlockingSystemAlertRootand theTINY_STABLE_TREE_NODE_COUNTarming bound all go — the rule becomesif (baseline.presentedSurface === 'absent') return null;, plus passing the baselineSnapshotStateinstead of bare nodes intorunStableCaptureLoop.Tests
The three navigation cases (#3470's tab switch, push, and push-onto-a-sparse-screen) are written as a table and are mutation-checked against the parent's arming: restoring the viewport-completeness arm fails exactly those three and nothing else in the file. The
Alert-root case pins the one surface the projection names outright. Layer 2's window-resolution cases and layer 1's both-windows floor cases pass unchanged on this branch, which is the point: narrowing the trigger must not touch when the confirmation is owed once it is armed.Two cases pin the finding so the rewrite is graded against it rather than re-argued.
generic-settle.test.tsdismisses onepageSheetfrom an interactive baseline and from a full-tree read of the same screen and asserts both confirm for 1.5 s; it is markedtest.fails, so the lane stays green today and vitest reports "expected to fail but passed" the moment #3565's fact makes arming flavour-independent — that red is the signal to drop the marker, and the assertion is the rewrite's acceptance test. It runs on the injected clock, so it costs no wall-clock time.The provider-scenario dialog is left exactly as its parent has it. An earlier revision of this layer padded it past the arming rule's size bound, which quietly removed the only daemon-route check of the confirmation (the transition stopped being broad — 4 of 6 node identities survive — so
waitedMs >= 1_500had to becomecaptures in 2..3). That padding is reverted: the flagship case asserts the 1.5 s floor again and all four provider-scenario cases pass on this rule, which is whysettle-observation.test.tsis no longer part of this PR's diff.pnpm format,pnpm lint,tsc -bclean.pnpm check:affected --run --base fix/3470-settle-quiet-per-capture-backendat06a4c5b86: all runnable checks passed (exit 0) — 296 related test files / 2,050 tests + 1 expected fail, Fallow clean on the changed files — held under the machine-wide lock, started2026-10-13T13:31:39Z. The run ate43dd7abcwas red on one thing only, the test-file-size ratchet, and that finding was correct: the parity case grewgeneric-settle.test.tsfrom 801 to 1,035 lines, past the 1,000-line tripwire. It is repaired by moving the three raw trees the case needs into__tests__/support/presented-sheet-trees.ts, which is how the suite stays at 963 lines without dropping an assertion; the case and the helper that drives the route stay where they were.The rule and the fixtures behind the navigation table are byte-identical to
a3dc159c8, the head every device measurement above was taken on (git diff a3dc159c8 HEAD -- stable-capture.ts stable-capture.fixtures.ts stable-capture.test.tsis empty), so those numbers still describe this head; the commit since then only adds test coverage.Evidence required before this leaves draft
From the review on #3512 (#3512 (comment)). Once #3565's fact exists, run on an iPhone simulator with the RN test app, at least 4 reps each of:
snapshotthen@refpress of close,back --settle,each reporting
settle.waitedMs >= 1500and an emptydiff snapshot -itaken 2 s later, plus one selector tab switch that stays near 800 ms — the #3470 cost this layer exists to remove, asserted rather than assumed. Plus the two route regressions the current rule fails:settleAfterInteractionwith a full-tree selector-resolution baseline, andsettleObservationover a stored full tree, both assertingwaitedMs >= 1500.The parity case in
generic-settle.test.tsis the unit-level stand-in for those two route regressions; the device rows are what close them. The numbers in "Why it stays a draft" above are the before state on the count rule and are not this exit criteria met.