feat: add selector-targeted drag gestures - #1567
Conversation
|
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Review: direction is right, two defects to fix, and one encapsulation themeDirection: yes. A target-authored drag is the correct primitive to add — it belongs at the interaction layer next to
Gates I ran locally on Below: two defects I reproduced, then the encapsulation review you asked for. 1. Recording round-trip corrupts partially-specified drag timings
function compact(values: Array<string | number | undefined>): string[] {
return values.filter((value): value is string | number => value !== undefined).map(String);
}Drag is the first gesture with three independent optional positionals, so a hole in the middle shifts everything after it into the wrong slot. Reproduced against this branch:
The consequence is the bad kind: a recorded script silently replays a different gesture. For the long-press-drag reorder this feature exists to drive, moving 600ms out of Fix is either positional placeholders for drag, or encode the timings as a trailing triple that is all-or-nothing. Worth a round-trip test over each of the 8 present/absent combinations. 2.
|
| Module | Coupling |
|---|---|
packages/contracts/src/gesture-normalization.ts:264,313 |
owns the encoding |
src/daemon/handlers/interaction-gesture.ts:237-238 |
writes positionals[1], positionals[2] |
src/daemon/session-script-writer.ts:345 |
reads positionals.slice(1, 3) |
src/daemon/handlers/session-replay-target-token.ts:8 |
reads positionals[1] |
Reorder drag's positionals and three of these break silently — and defect 1 above is exactly what a hole in that layout already does. The daemon rewrite in particular would read better as a re-encode than an index patch:
gesturePayloadToPositionals({
...input,
source: recording.sourceSelector ?? input.source,
destination: recording.destinationSelector ?? input.destination,
})…which keeps the layout knowledge in the one module that owns it, and stops being wrong the moment placeholders land for defect 1.
The type model doesn't absorb drag, so each layer widens or casts around it:
resolveExecutionProfilereturns'hold-drag'(interaction-gesture.ts:198), butGestureExecutionProfileis'endpoint-hold' | 'timed-pan'andbuildDragGesturePlanstamps the plan'timed-pan'. So the response advertises a profile that is not in the union and is not what executed. The declared return type isstring | undefined, so nothing catches it. Either admit'hold-drag'into the union and carry it onto the plan, or rename the response field so it isn't read as the plan's profile.kind: GestureIntent | 'drag'— an inline widening rather than the exportedGestureCommandInputmodel.capabilities.tstypes its parameterGestureSemanticInput | { intent: 'drag' }even thoughGestureCommandInputis exported from contracts for exactly this. Two spellings of one concept.- Two casts where a discriminated narrow would do:
options.gesture as GestureSemanticInput(gesture-command.ts:85) andresolved as ResolvedInteractionTarget & { point: Point }(line 169). The first is a direct consequence of narrowing via the separateresolvedDragvariable instead of branching onoptions.gesture.intent— which is also why the same block needsdragGesture?.sourceHoldMson a value that cannot be undefined there.
None of these is individually serious. Together they mean the next gesture that carries targets repeats all of it. Branching once on the discriminant, and letting GestureCommandInput be the single spelling, removes most of the casts and the fake-pan shim at the same time.
Smaller notes
- Duration sum is validated too late. Each timing is capped at 10s in
readGesturePayload, but the total is only checked insidebuildDragGesturePlan— aftercaptureGestureViewportand both target resolutions.gesture drag src dst 10000 10000 10000pays a snapshot and two resolutions before failingINVALID_ARGS. The sum check belongs in contracts next to the per-field ones. readNonEmptyStringvalidatesvalue.trim().length > 0but returns the untrimmed string, so' id="x" 'reaches the resolver with padding.sourceHoldMshasmin: 1whiledestinationHoldMshasmin: 0. Presumably deliberate (you must hold to activate; you needn't hold to release) but it's undocumented and will read as a typo.prepareDragTargetreturns{ target }and both call sites immediately unwrap.target.- No corpus coverage.
grepfindsgesture dragnowhere underexamples/ortest/— only inwebsite/docs. Unit coverage is solid, but the repo'stest-app:replay:*corpora are how gestures usually earn their keep, and removing the prototype assets left the new command with no replay fixture. A smalldrag.adagainst a test-app reorder target would also give the timing semantics somewhere to regress.
One thing I could not verify
Drag lowers to executionProfile: 'timed-pan', so it shares the runner's .sampled path with gesture pan. While testing something unrelated on iOS earlier today I saw gesture pan wall-time not scale with its requested durationMs (400 / 1200 / 3000ms all completing in roughly the same time). I could not confirm that: every one of those trials used a pan geometry that was inert, so the fast return may just be an early-out, and a re-test at a moving geometry was blocked by another daemon owning the simulator. Flagging it only because this feature's whole value rests on an 800ms activation hold actually lasting 800ms on device. If it hasn't been checked directly, it's worth asserting the observed contact duration once on each platform rather than inferring it from the plan.
|
Reviewed exact head
CLI/Node/MCP/daemon routing, ref admission/expiry, portable ref rewriting, duration planning, CI, and the reported device runs otherwise look sound. The device evidence is described but not attached/reproducible, so that remains a readiness residual. |
Deep code-quality audit (structural)Follow-up to my earlier comment, which covered two reproduced defects and the positional-layout coupling. This pass is purely about structure: is this the simplest shape this feature can take? I don't think it is, and I think there are two code-judo moves that delete most of the new complexity rather than rearrange it. Not approving on structure yet. Behavior looks right and the gates are green — that isn't the bar here. File-size rule: clean. Nothing crosses 1k because of this PR. 1. Drag logic is in the wrong module, and that's what forces the casts
That work already has a canonical home in the same directory. const resolved = await resolveInteractionTarget(runtime, options, {
action, requireInteractive, promoteToHittableAncestor,
expectedResolvedTarget: options.expectedResolvedTarget,
});
const point = requireResolvedPoint(resolved);
…
return { ...resolved, … }; // disclosure / selectorChain / evidence propagate by spreadThe new code reimplements that spine instead of using it:
Note what that first row means: the cast at :169 exists only because the canonical helper wasn't used. The judo move: put
I'll grant the one real asymmetry: drag has two endpoints and the spread convention carries one. So the destination needs something bespoke. But the source is the recorded, replay-verified identity — it can use the canonical path as-is, and only the destination needs a small addition. That is a much smaller delta than the current 153 new lines in the wrong file. 2.
|
Could this reuse
|
|
Addressed all four review comments in
Red proofs were run for sparse timing corruption, fake-pan capability reporting, skipped destination verification, missing ADR matrix coverage, and late total-duration validation. Final local gate: |
2 participants
Summary
Adds a generic selector/ref-targeted drag gesture across the CLI, Node client, MCP, recording, and replay surfaces.
Public API
CLI:
Node:
MCP:
Defaults are 800 ms source hold, 500 ms movement, and 0 ms destination hold. The combined plan is capped at 10 seconds.
Validation
pnpm check:affected --runpnpm build:androidcompleted before device validation.Load-bearing red proof:
@e2~s42instead of a portable selector.Recording and replay notes
The recording format supports one target-v1 annotation per action. Drag uses the source as that action identity and verifies it before replay dispatch; the destination is still materialized to a durable selector chain and resolved before any device gesture. Strict publication refuses either endpoint if it remains a bare ref.
Video evidence
The native-iOS, fallback-iOS, and fallback-Android recordings were intentionally removed with the throwaway prototype directory, so the branch and PR history stay generic. The files are available for manual attachment to this description; GitHub CLI cannot upload user-attachment blobs.