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. |
183801d to
3a25bb2
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. |
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 |
06a4c5b to
d2d7d7b
Compare
|
The 06a4c5b findings are not fixed in d2d7d7b. The arming rule in stable-capture.ts is unchanged, and one blocking defect and one missing validation remain.
This change alters device-facing iOS --settle arming, and no simulator run exists on the rewritten rule. The tables in the PR body predate it. After the rewrite, please run on an iPhone simulator with the RN test app, at least 4 reps of each case: (a) selector press of close; (b) plain I did not run the tests or reproduce the device timings, so the 1,502 ms and 27 ms figures are yours. I also could not check the rewrite against #3565/#3577, because |
1a9743c to
d037f58
Compare
d037f58 to
927d2ce
Compare
Rewritten on #3565's fact, out of draft —
|
There was a problem hiding this comment.
All reported issues were addressed across 10 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
453221d to
93b8220
Compare
…ed-surface fact A tab switch or a push replaces the screen broadly, so main's arming charged ordinary navigation the 1.5 s confirmation floor (#3470). The settle baseline now carries the presentedSurface fact its producer stamped before projection (#3565), and the confirmation reads it: - `absent` rules the confirmation out for the run: nothing presented, no dismissal to confirm. - `presented` admits a rootless first capture after the dismissal, as a tiny baseline already did. - `unknown` or no fact keeps the parent's arming, so the fact only ever narrows the guard. Because the fact is stamped before projection, interactive and full-tree baselines of one sheet arm alike, which the node-count draft of this layer could not do. Closes #3470 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
93b8220 to
fea0bb9
Compare
Closes #3470. Builds on #3522 (merged).
Summary
The iOS 1.5 s dismissal confirmation (#1831) now reads the producer's presented-surface fact (#3565):
absentrules it out: nothing was presented, so a tab switch or push keeps the plain window.presentedarms it, even on a rootless first capture.unknown, or no fact, keeps the parent's arming, so the fact can only narrow the guard.A stored tree outdated by a later side effect drops its fact, so an
absentread taken before a sheet opened cannot disarm that sheet's dismissal. Help text updated. 10 files.Validation
927d2ce57, iPhone 17 Pro sim, RN lab Release build (the dev client's floating button makes every screenunknown, per #3578).waitedMsdiff snapshot -ipresented, 4 nodessnapshot, then@refclosepresented, 6 nodesback --settle, Contacts New Contact sheetpresented, 70 nodespress x yclosepresented, 70 nodesabsentTab switch, same device and route: | base
40d8682331829–1872 ms | head 627–778 ms |.A full-screen modal reads
unknown, so it keeps the confirmation. Each fact branch has a unit or daemon case that fails under mutation.pnpm check:affected --run --base origin/mainatfea0bb95a(rebased onmainafter #3522 merged): all runnable checks passed, 535 test files, 4,060 tests, Fallow clean on 10 files, under the host lock. The device runs above were taken at927d2ce57. Since then the only change is help text, and the rebase onto #3522's squash is identical in content.🤖 Generated with Claude Code