feat(interaction): report readiness wait on a successful press, click, or longpress - #3081
Conversation
…lick, or longpress A step that waited for its target and then acted now returns data.readiness with polls and waitedMs, present only when more than one capture was needed. The runtime selector poll reports the wait on the resolved target, the shared interaction response builder attaches it without touching the warning, and a replay step's pre-dispatch gate hands its own wait to the dispatch through internal.replayReadinessWait so one step reports one combined readiness. Tests and the mutation that fails each: - press-target-readiness.test.ts "third poll" (provider scenario, polls 3): resolution.ts drops `readiness = ready.readiness`. - press-target-readiness.test.ts "first capture hits" and selector-readiness.test.ts "first capture ... no readiness": the `observed.polls.length > 1` guard becomes `> 0`. - selector-readiness.test.ts "polls until the target appears": same drop as the third-poll scenario. - interaction-touch-response.test.ts carried-wait cases: the builder stops combining `params.carriedReadiness`; the first case also pins the prior warning staying composed. - session-replay-target-verification-runtime.test.ts gate cases: the adapter stops passing gateWait to applyReplayReadinessWait; or the gate records a wait at `polls > 0`.
…he readiness diagnostic
|
Size Report
Startup median (7 runs, lower is better):
|
|
Code looks sound at 9794df6, but I can't call it fully checked yet. I did not run any tests, and I judged them from the diff and the pre-change code. All 16 checks pass, and the added tests cover the selector readiness route and the daemon response builder. There are no conflicts. The builder and integration tests would fail without this change, because the field was never set before. Only press goes through the daemon in the integration test. I checked click and longpress by reading the shared resolver and spread path. I did not check the Node client's press result mapping, or any MCP response shaping that passes readiness through. So the claim in client-api.md that the result carries data.readiness rests only on the daemon response. I also did not verify the PR body's claim that the android-lifecycle failure was load contention. Could you confirm those two points, or say which ones you already checked? Not blocking, and you can take or leave these: the assert in selector-readiness.test.ts is a weak boolean check that shows no value on failure, so narrowing the type and asserting equal on polls would read better. The --debug line in replay-e2e.md is not tied to this change, and one sentence could say replay shows the wait only under --debug, since the runner drops successful step responses. I looked for a simpler shape and found none. The change is about 29 production lines and reuses the existing polls and waitedMs. Stamping the field in the daemon would lose the poll count, which only the runtime sees. Is there a reason to prefer that route? Once the two unchecked points are confirmed, nothing else stands between this PR and merge. The #3077 soak and keep-or-drop decision can happen after merge. |
|
[claude-fable-5-1] responding on behalf of @thymikee Both points are now checked. New head: 8868fb5 (one test-only commit plus a docs sentence; no production change). 1.
2. android-lifecycle. Notes taken
Simpler shape. I kept the runtime stamp. A daemon-side route would lose the poll count, which only the runtime sees ( Checks run: typecheck, the three touched test files, layering, fallow audit, format:check. All clean. |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
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="src/__tests__/client-interactions-readiness.test.ts">
<violation number="1" location="src/__tests__/client-interactions-readiness.test.ts:31">
P3: The cast to `{ readiness?: unknown }` is unnecessary: press/click/longPress results are typed `CommandResult<'press'|'click'|'longpress'>`, which resolves to `PressCommandResponseData`/`ClickCommandResponseData`/`LongPressCommandResponseData` and already declare `readiness?: ReadinessWaitEvidence` (packages/contracts/src/interaction.ts, TouchResponseDataBase). Casting the field to `unknown` means the test no longer proves the client's published result type exposes `readiness` — if the contract drops or renames it, this test keeps compiling while the surface change goes unnoticed. Use the typed field directly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| const longPressed = await client.interactions.longPress({ selector: 'label=Foo' }); | ||
|
|
||
| for (const result of [pressed, clicked, longPressed]) { | ||
| assert.deepEqual((result as { readiness?: unknown }).readiness, readiness); |
There was a problem hiding this comment.
P3: The cast to { readiness?: unknown } is unnecessary: press/click/longPress results are typed CommandResult<'press'|'click'|'longpress'>, which resolves to PressCommandResponseData/ClickCommandResponseData/LongPressCommandResponseData and already declare readiness?: ReadinessWaitEvidence (packages/contracts/src/interaction.ts, TouchResponseDataBase). Casting the field to unknown means the test no longer proves the client's published result type exposes readiness — if the contract drops or renames it, this test keeps compiling while the surface change goes unnoticed. Use the typed field directly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. 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. At src/__tests__/client-interactions-readiness.test.ts, line 31:
<comment>The cast to `{ readiness?: unknown }` is unnecessary: press/click/longPress results are typed `CommandResult<'press'|'click'|'longpress'>`, which resolves to `PressCommandResponseData`/`ClickCommandResponseData`/`LongPressCommandResponseData` and already declare `readiness?: ReadinessWaitEvidence` (packages/contracts/src/interaction.ts, TouchResponseDataBase). Casting the field to `unknown` means the test no longer proves the client's published result type exposes `readiness` — if the contract drops or renames it, this test keeps compiling while the surface change goes unnoticed. Use the typed field directly.</comment>
<file context>
@@ -17,3 +17,17 @@ test('readinessTimeoutMs on press/click/longpress reaches the request flags', as
+ const longPressed = await client.interactions.longPress({ selector: 'label=Foo' });
+
+ for (const result of [pressed, clicked, longPressed]) {
+ assert.deepEqual((result as { readiness?: unknown }).readiness, readiness);
+ }
+});
</file context>
| assert.deepEqual((result as { readiness?: unknown }).readiness, readiness); | |
| assert.deepEqual(result.readiness, readiness); |
|
The PR is ready at 8868fb5. The earlier review (#3081 (comment)) was waiting on evidence, and the new tests and docs now cover it. All 16 checks pass, and the latest changes touch only tests and docs, so no failing route is involved. There are no conflicts, and nothing else stands in the way of merging. Not blocking: website/docs/docs/client-api.md:300 says the Node client result "carries data.readiness", but Node callers read For the record, I ran no tests and judged the new ones from the diff and the code at that commit. The client and MCP tests use a stubbed transport and check passthrough only, so they also pass on the old code and guard against a future drop, as you describe. I read replay, the CLI JSON route, and click and longpress through the daemon in code, and the integration test covers only press. I did not verify that the android-lifecycle failure was load contention, only your 3/3 isolated runs. The #3077 soak and the keep-or-drop decision can follow after merge. |
Summary
A successful press, click, or longpress that had to wait for its selector target now reports the wait as
data.readiness({ polls, waitedMs }). The field appears only when the wait took more than one poll, so first-capture hits are unchanged. The daemon response builder adds it after warning composition, so existing warnings stay intact. Replay waits remain visible asinteraction_target_readinessdiagnostics under--debug.Docs updated:
client-api.md,replay-e2e.md.Part of #3077. Part of #3069. 9 files touched. Stacked on #3072 (
proto/reliability-contract; #3071 isfeat/dispatch-disclosure).Validation
Tested commit: 9794df6
pnpm check:affected --run: every lane passed except one vitest-related failure. Of 627 files, 626 passed. The failure wastest/integration/provider-scenarios/android-lifecycle.test.ts("Android test IME recovery records could not be persisted"), which took about 7s under host load. Run alone it passes (6/6). The branch diff does not touch that file, so I count it as a contention failure. Fallow reported one unused export (runtimeScrollSnapshotin an untouched test-utils file) but audited 76 files against origin/main, so its scope is not this diff. It did not fail the gate. No fixes or commits were made.Contention reruns:
test/integration/provider-scenarios/android-lifecycle.test.ts.Live evidence: not device-facing.
Unresolved risks: