Skip to content

fix(studio): make the Escape double-press window strictly under 500 ms - #2318

Merged
kantord merged 1 commit into
studio-followups/07-schedules-bff-outcomefrom
studio-followups/08-escape-tests
Oct 15, 2026
Merged

kantord merged 1 commit into
studio-followups/07-schedules-bff-outcomefrom
studio-followups/08-escape-tests

Conversation

@kantord

@kantord kantord commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Summary

The Escape double-press check compared with <= 500 while the expiry timer fires at 500, so the exact boundary raced. One rule now: less than 500 ms.

Part of #2303 ("Tiny follow-up fixes"); it does not close it. This is PR 8 of 14 in a stack of draft PRs, reviewed and merged in order.

What changed

  • chat-escape.ts: <= 500 becomes < 500; the redundant changed alias is removed (the focusin listener stays, so a focus move still cancels an armed press).
  • Tests: the 499 ms and 500 ms boundary (the clock is moved without firing the expiry timer, which would hide the race); held Escape (repeat: true) at each layer (ask deny, plan iterate, panel close, stop); a same-tick double-click on an ordinary verdict button on the chat and on the side thread.

Behavior changes

A second Escape exactly 500 ms after the first no longer clears the draft.

Verification

Biome, typecheck, the full Studio test suite and pnpm generated:check pass on this branch, run on the assembled stack. The full Playwright suite passes on the stack tip (122 passed, 30 skipped, 0 failed, including chat-approval against a real mecated build). Each new test fails with its fix reverted. The existing "one verdict" test only proved the disabled state (fireEvent flushes between clicks); the new test uses a real same-tick double-click. The plan version already existed and was not duplicated.

Not included

A planted-secret-in-ask-args test: scrubbing happens in the engine and the UI renders args as given.

Fully or partially written by an AI agent.

🤖 Generated with Claude Code

@peppescg peppescg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Daniel. The < 500 rule now agrees with the expiry timer, and moving the clock without firing the timer is the right way to pin the boundary.

Nits:

  • apps/web/src/features/chat/chat-escape.ts:124: the re-arm branch replaces expiry.current without clearing the pending timeout. A press at exactly 500 ms (or later, if the timer is delayed) now re-arms while the first timer is still queued. That timer then fires and immediately disarms the new arm, so the hint flashes and the next press re-arms instead of clearing. Add if (expiry.current !== undefined) window.clearTimeout(expiry.current); before scheduling the new timeout. A test could advance timers by a few ms after the 500 ms press and assert the hint is still shown.
  • apps/web/src/features/chat/chat-escape.test.tsx:292-295: the ordinary-ask block only checks that a held press does nothing. Follow it with a normal press and expect "deny", as the plan block does with "iterate", so the test can't pass on a layer that never acts.

@kantord
kantord force-pushed the studio-followups/08-escape-tests branch 4 times, most recently from 768dfd4 to c739d48 Compare October 15, 2026 09:29
The second press compared with <= 500 while the expiry timer fires at 500,
so the exact boundary raced. One rule now: less than 500 ms. Also drops the
redundant disarm alias and adds boundary, held-Escape and ordinary-verdict
double-click tests.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@kantord
kantord force-pushed the studio-followups/08-escape-tests branch from c739d48 to 780022c Compare October 15, 2026 12:25
@kantord
kantord merged commit 4aa238f into main Oct 15, 2026
30 of 31 checks passed
@kantord
kantord deleted the studio-followups/08-escape-tests branch October 15, 2026 13:00
@toolhive-release-app toolhive-release-app Bot mentioned this pull request Oct 20, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants