Repository navigation
fix(settle): keep iOS keyboard chrome classified through interactive capture - #3482
Conversation
…capture The regular snapshot projection dropped the Keyboard container as ineligible, re-parenting ~40 Key nodes to the app root where the settle chrome classifier could no longer anchor them, and the interactive projection flattens QuickType/dock siblings beside app content entirely. Every keyboard-raising fill paid the 1.5s broad-transition confirmation window on a keyboard pop counted as a screen replacement. - Add Keyboard to both projection eligibility sets (TS + Swift, parity) - Classify keyboard chrome by exact UIKit owned-class roles and the inputView placeholder marker (which owns no subtree) - Compare the broad-transition check against a chrome-free candidate signal so a keyboard pop no longer disarms the quiet window
… format style The shared keyboard capture fixture joins the capture-kit exports map, so the layering exports manifest gains its subpath, and oxfmt ignores the fixture file the way it already ignores runner-requests.json: one captured node per reviewable diff line.
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 13 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/capture-kit/src/snapshot-chrome.ts">
<violation number="1" location="packages/capture-kit/src/snapshot-chrome.ts:165">
P2: This re-adds app-provided `UITextInputAssistantItem` buttons under `TUISystemInputAssistantView` after the above-keyboard pass exempted them, so settle filtering hides real app controls. Preserve the accessory exemption when adding owner-subtree indexes, or scope the walk to system-owned children.</violation>
</file>
<file name="packages/capture-kit/src/snapshot-chrome.fixtures.ts">
<violation number="1" location="packages/capture-kit/src/snapshot-chrome.fixtures.ts:12">
P3: The docstring's parenthetical node breakdown doesn't account for node 31. It enumerates app nodes 0-30, inputView host (32), QuickType (33-39), keyboard plane (40-79), and dock buttons (80-82), which sums to 82 nodes — but the array holds 83 nodes (0-82). Node 31 (`_UIFloatingBarContainerView`, label "Toolbar") is present in the array but missing from the breakdown, so the next paragraph's "83 nodes" doesn't reconcile with the enumeration. Add node 31 to the breakdown.</violation>
</file>
<file name="src/commands/interaction/runtime/stable-capture.ts">
<violation number="1" location="src/commands/interaction/runtime/stable-capture.ts:256">
P3: `stableCaptureReplacementSignal` runs a full `withoutSettleChrome` classification (window/container subtree walks plus ancestor scans over the whole tree) and builds a second `StableCaptureSignal` on every poll iteration, even though the screen usually hasn't changed. The loop computes `requiredQuietMs` before the existing `stableCaptureSignalsEqual(lastSignal, signal)` short-circuit fires, so the expensive chrome-free projection is rebuilt per poll while the result can only change when the candidate signal changed. Since the quiet window is computed from `baseline`, `snapshot`, `appBundleId`, `quietMs`, and the current `currentQuietMs` — all constant for an unchanged candidate — the result is identical until the signal differs. On this latency-sensitive settle path (300 ms cadence), the duplicate per-iteration tree pass is wasted work in the common stable case.</violation>
</file>
…hrome walk An app's `UITextField.inputAssistantItem` bar buttons render under the system assistant view, above the keys; the window pass exempted them and the owned-role subtree walk re-claimed them, hiding real app controls from the settle diff. An exempted node now re-joins chrome only when it declares a keyboard-owned role itself, so a system prediction cell still collapses while an app button under the same host survives. Also from review: hoist the chrome-free replacement projection behind a baseline/candidate-change guard so the 300ms poll of an unchanged screen pays no second tree pass, and count node 31 in the fixture breakdown.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Keying the memo on stableCaptureSignalsEqual is unsound: that identity omits role, parent links, and sub-pixel geometry, all of which withoutSettleChrome reads, so a newly broad replacement could reuse a non-broad verdict and skip the confirmation window. Keying it on every classifier input would cost more than running the classifier, and the measurement says the memo was never owed: one full chrome-free pass is ~42us on the 83-node real keyboard capture and ~1.4ms on a 5k-node tree, against a 300ms poll cadence. The verdict is recomputed per capture again, and the doc-comment forbids re-memoizing on that key.
|
Thanks for the fix. At 41c8a82 one problem remains, so this is not ready to merge yet. CI is green (19 checks, none failing), and there are no conflicts. The assistant-bar buttons are still dropped on the interactive capture that Not blocking, and you can take or leave these: Could this be simpler? The Keyboard eligibility change alone restores container-walk classification of every Key. If the remaining flattened surfaces (QuickType, dock, inputView host) were classified per node from one shared iOS keyboard-role table in contracts, also used by The live iOS numbers (median 1734 to 754 ms, zero On the open review threads, the first one still applies: #3482 (comment) (Window-less exemption). It is marked resolved, but the code at 41c8a82 still has the gap, which is the problem above. These three no longer apply, so please resolve them: #3482 (comment) (the fixture now names node 31 and the counts add up), #3482 (comment) (the memo was removed, and the per-poll cost is small against the 300 ms cadence), and #3482 (comment) (the unsound memo key was deleted and the result is recomputed per capture). Before merge, the assistant-view subtree claim needs to become a marker, with the no-Window regression added. |
|
Fixed at 744c8a3, following your rule exactly. Your non-blocking items, all taken:
On the |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
744c8a3 to
b3ff868
Compare
…sts are markers The assistant view and its page view are keyboard hosts the APP can also fill (UITextField.inputAssistantItem bar buttons ride them), so they join the inputView placeholder as markers: chrome themselves, claiming no subtree. Subtree claims stay with the system-only hosts — the key plane (UIKeyboardLayoutStar) and the prediction bar — where no app content can ever sit. Surface roles are chrome per node. With no subtree walk able to reach app content, the accessory re-join guard and the hosted editable-text carve-out are dead weight and are removed. The IOS_KEYBOARD_* tables live in android-input-ownership — the module the settle classifier and the tap-occlusion band already evaluate, as the eager-closure gate (ADR 0027) requires for shared symbols; a dedicated module would grow every shared entry's eager closure. The prediction cell stays out of the surface table so QuickType geometry never enters the occlusion band. Accepts two inert noise survivors in the flattened capture (floating toolbar, QuickType scroll wrapper) — noise fails loudly, a subtree claim through an app-fillable host would not. Also reverts the broad-replacement right parameter to non-optional: the memo is gone and the only caller always passes a defined signal.
b3ff868 to
faf754a
Compare
|
Live re-run at the shipped head faf754a (iPhone 17 sim, iOS 27, com.callstack.agentdevicelab, Catalog fill id=catalog-search kit/loaf --settle, 10 alternating runs, keyboard-enter reset between fills; the device was momentarily held by the parallel #3470 worker, hence the follow-up):
The 1734→786 confirmation stands. One honest delta vs the e2ad899 build's first re-run (diffLines 15, captures 3): the marker-rule rebuild trades three key-plane lines for the two accepted inert residues (QuickType UIScrollView + assistant page view are no longer subtree-claimed), so per-fill diffLines is 18 rather than 15 — still 38 fewer lines than main, and captures dropped to 2. The keyboard arrival costs one visible [keyboard] line plus two labeled-acceptable noise lines. Session closed and simulator shut down after the run. |
|
This PR is ready on the code. At faf754a, the keyboard-chrome marker no longer claims an app subtree, the breakdown fixture lists node 31, and the early return now requires no subtree chrome roots, with a regression test. Those were the points from the earlier review at 41c8a82 (#3482 (comment)). The memo on the stable-capture verdict is gone, so the verdict is recomputed for each capture. That cost is small next to the 300ms poll. Not blocking: the comment at packages/contracts/src/android-input-ownership.ts:157 says the role classes are private to the keyboard surfaces and that an app cannot host one in its own view tree, but the fourth bullet (IOS_KEYBOARD_CHROME_MARKER_ROLES) describes hosts the app can fill, so you could limit that sentence to the classes themselves (an app cannot instantiate them) and leave the subtree question to the marker bullet, or leave it as is. All five earlier inline threads from the other reviewer no longer apply, so you can resolve them: #3482 (comment), #3482 (comment), #3482 (comment), #3482 (comment) and #3482 (comment). The first, second, fourth and fifth are fixed at this commit. The third is benign now that the memo is deleted. 18 of 19 checks pass, and Smoke Tests is still running. If the iOS smoke covers fill or --settle, treat a failure there as possibly related, because this change keeps the Keyboard node in regular snapshots and feeds the settle chrome filter into the quiet window. Check the logs before calling it unrelated. Smoke Tests must finish green before merge. On evidence, I read the code and tests but did not run them. The live numbers at faf754a (1734 to 786 ms, zero [key] lines) and the 393 to 394 eager-closure count are yours, and I did not reproduce them. I also did not check on a device that iOS renders inputAssistantItem buttons as children of TUISystemInputAssistantView. The marker rule is safe either way. From reading the old code, the new regression test would fail on 41c8a82. |
thymikee
added
the
ready-for-human
|
…3572) The IOS_KEYBOARD_* docblock said the classes are "private to the keyboard surfaces — an app cannot host one in its own view tree", while the marker bullet below it describes hosts the app CAN fill. Limit the sentence to what is true of the classes themselves (an app cannot instantiate them) and leave the subtree question to the per-table bullets, which already answer it. Follow-up to #3482's final review note; no behavior change.
1 participant
Summary
--settleon iOS lost keyboard-chrome classification in interactive captures: the regular projection dropped theKeyboardcontainer as ineligible, re-parenting ~40Keynodes to the app root wherecollectKeyboardChromecould not anchor them, and the interactive projection flattens QuickType/dock siblings beside app content entirely. Every keyboard-raising fill paid the 1.5s broad-transition confirmation window on a keyboard pop read as a screen replacement.Add
Keyboardto both projection eligibility sets (TSREGULAR_ELIGIBLE_TYPES+ SwifteligibleInteractiveTypes, pinned by a sharedswift: truegolden case).Classify the flattened surfaces from one iOS keyboard-role table in
@agent-device/contracts/android-input-ownership(shared withtap-keyboard-occlusion.ts): surface roles chrome per node, subtree claims only under system-only hosts (UIKeyboardLayoutStar,TUIPredictionView), and MARKER roles (inputViewplaceholder, assistant view + page view) that own no subtree — an app can fill those hosts, so app content under them survives.The broad-transition check now compares a chrome-free candidate signal (
withoutSettleChrome), so the diff and the quiet-window verdict share one classifier.Closes #3469.
13 files. Live evidence, iPhone 17 sim (iOS 27), Catalog
fill id=catalog-search <kit|loaf> --settle, 10 alternating runs each:waitedMs[key]diff lines[keyboard]lineValidation
Tested at
faf754aa6.pnpm check:affected --runpassed there (unit, layering, fallow, Swift differential with the newswift: truegolden case, mutation-lane selection); the device table below was captured ate2ad8997e; a fresh live re-run atfaf754aa6is in the comments (median 1734 -> 786 ms, captures 5 -> 2, zero[key]lines). Live device run above executed the changed path end-to-end via the rebuilt CLI. No user-facing flags or help change. Known adjacency: thereplacementSignalwiring touches the same call site #3470 proposes to restructure; no rule/default-window change is included here.