Progressively render drag trails in video recordings#14291
Progressively render drag trails in video recordings#14291warp-dev-github-integration[bot] wants to merge 5 commits into
Conversation
Handoff spec for improving burned-in drag annotations so the trail is drawn progressively as the drag proceeds, instead of rendering the whole path up front. Documents the teleport-dispatch root cause, the concrete ASS renderer change in append_drag, out-of-scope items, and testing. Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
|
@warp-dev-github-integration[bot] I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes the computer-use ASS overlay renderer to synthesize a drag draw interval, emit per-segment trail dialogues, move the held indicator along path segments, and update coverage for drag overlay behavior.
Concerns
- The progressive reveal is incomplete: each trail segment is emitted as a full quad at that segment's start, so a common one-move drag still shows the entire future trail immediately at press time. See the inline comment.
- For this user-facing recording change, please include screenshots or a screen recording demonstrating it working end to end. The PR description says visual proof could not be captured in the sandbox, but repo guidance still requires visual evidence for behavior users can perceive; a local desktop run or a coding agent with computer use can capture the recording.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR reworks append_drag to pace the held indicator and trail over a bounded 600 ms synthetic draw interval, emit per-segment staggered trail dialogues weighted by cumulative path length, move the dot piecewise along consecutive points, and add a stationary endpoint hold. The structural change is sound and well-tested at the ASS-string level, and strict clippy/format pass. Two material concerns from the prior review remain unresolved on this head: the progressive reveal is incomplete for the common single-segment drag (the spec's core goal), and the spec-mandated real Linux artifact check was not captured.
Concerns
- Progressive reveal is incomplete within a segment (spec drift). The spec's core requirement is "the trail should be drawn progressively as the drag proceeds — the line grows behind the moving cursor dot — rather than appearing all at once." Each trail dialogue emits the full quad for its segment at that segment's staggered start time (
overlay.rs:822,ass_trail_quads(&clamped[*index..=*index + 1])). For the common single-segment drag (press, one move, release), there is only one segment, so the entire A→B trail appears at press time and the dot then travels across it — exactly the "entire path up front" behavior the spec was created to fix. Multi-segment drags stagger between segments but still reveal each whole segment quad ahead of the dot traversing it. The existing testdrag_emits_trail_anchor_held_and_no_ringconfirms this: a one-segment drag emits a single trail dialogue spanning the full[press, release+fade]interval. To meet the spec, split long segments (sub-divide by cumulative distance across the synthetic interval) or animate the visible geometry so the trail only extends up to the dot over the synthetic interval. The held-indicator piecewise\moveis correct; the trail reveal is the gap. - Spec-mandated real artifact check not performed (user-facing, unverified visually). The spec's Testing section explicitly requires: "record a drag, download the mp4, confirm the line draws in behind the dot rather than appearing whole." The PR body reports the full
./script/presubmitand GUI build were SIGKILLed by sandbox memory limits, so no visual proof was captured. Per the review rubric, a user-facing change requires visual proof validated against the spec; environmental constraints prevented capture here, so this is noted explicitly rather than treated as an independent blocking finding — but combined with the concern above (the very behavior the artifact check is meant to confirm does not occur for single-segment drags), the change remains visually unverified and the spec's acceptance criterion unmet. A local desktop run or a cloud agent with computer use should record a single-segment and a multi-segment drag and attach the mp4/screenshot evidence to the PR before re-review.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Prior concerns still outstanding: progressive-within-segment reveal and real artifact verification.
Request changes
Review run
https://oz.staging.warp.dev/runs/019f96d0-de64-75ab-a865-ec1559d0e430
Co-Authored-By: Oz <oz-agent@warp.dev>
There was a problem hiding this comment.
Overview
Second review pass (rework #1) of the progressive drag-trail renderer in crates/computer_use/src/overlay.rs. The rework resolves the prior progressive-reveal concern: append_drag now subdivides each path segment into distance-bounded ASS quads (<=32px) staggered by cumulative path length across the bounded 600ms synthetic draw interval, with the held indicator moving piecewise along consecutive points and a stationary endpoint hold; the prior inline thread is resolved/outdated. I independently confirmed the code-level gate: 43/43 overlay tests pass (incl. the new single_segment_drag_reveals_short_quads_behind_the_dot and updated multi-segment/split-call coverage), cargo clippy -p computer_use --all-targets -- -D warnings is clean, git diff --check is clean, and CI is green (Formatting + Clippy on all platforms, Run Linux + Windows tests, all compilation checks; MacOS tests still pending, no failures). The implementation materially matches the committed spec (.agents/specs/QUALITY-1169-progressive-drag-trail.md): synthetic draw duration, per-segment staggered quad reveal by cumulative length, piecewise dot, preserved anchor/color/\an7/\clip/smart-cut remapping, no click ring, and held-press-without-release behavior.
Concerns
- Visual proof still outstanding (user-facing, environment-limited). The spec's Testing section requires a real Linux artifact check: record a drag, download the mp4, confirm the line draws in behind the dot rather than appearing whole. The PR body documents that the full
./script/presubmitand the Linux GUI build (cargo build --bin warp) were SIGKILLed by sandbox memory limits, so no mp4/screenshot proof could be captured. This is a persistent environmental constraint (sandbox memory OOM on the GUI build), not implementation negligence — every available code check passes. Per the review rubric a user-facing change requires visual proof validated against the spec, so this remains the sole blocker; per the factory playbook's environmental-constraint carve-out it is surfaced explicitly here so a human can decide whether to accept with a manual visual check (local desktop or higher-memory runner) or re-dispatch the implementation agent on a higher-memory runner to capture the mp4/screenshot proof. I did not re-exercise the running UI myself because the same GUI build OOMs this sandbox.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Prior concerns: #1 (progressive-within-segment reveal) resolved on rework #1; #2 (real artifact verification) still outstanding as an environment-limited block.
Request changes
Review run
https://oz.staging.warp.dev/runs/019f970b-611b-7994-a6ed-e7dffcc570da
Summary
Verification
single_segment_drag_reveals_short_quads_behind_the_dot, which proves a one-move drag emits multiple ordered quads and the first quad does not contain the endpoint; updated multi-segment and split-call coverage for subdivided trails.cargo nextest run -p computer_use overlay::tests:: --no-fail-fast— 43 passed.cargo clippy -p computer_use --all-targets -- -D warnings— passed../script/format— passed.cargo nextest run -p computer_use --no-fail-fast— 59 passed; the sole failure is the existing native X11 foreground-window test (Target window ... could not be made foreground-visible), unrelated to this renderer change../script/presubmit— attempted and terminated by sandbox SIGKILL while compilingwarpdue to memory limits.CARGO_BUILD_JOBS=1 cargo build --bin warp) — attempted and terminated by sandbox SIGKILL while compilingwarp; therefore computer-use screenshots/video for single- and multi-segment drags could not be captured here. A higher-memory runner or manual Linux verification is still required for the visual artifact check.Originating thread: https://warpdev.slack.com/archives/C0BDQDW8V5E/p1784935273012619?thread_ts=1784935273.012619&cid=C0BDQDW8V5E
Co-Authored-By: Oz oz-agent@warp.dev