fix(web): intercept approval text input in chat - #2124
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a 'DB MIGRATION' label for automated PR categorization and implements a new frontend feature to intercept chat keywords (such as 'yes', 'no', and 'always') for resolving tool call approvals. Comprehensive E2E tests were added to verify this interception logic. Feedback was provided to improve the robustness of the approval card selection in the frontend by ensuring the logic targets the most recent unresolved card, preventing potential issues during the resolution animation phase.
3df5e54 to
d5d32c8
Compare
When a tool requires approval in the web UI, typing "yes", "no", or "always" in the chat input now resolves the approval card directly instead of sending a regular message. This prevents duplicate approval prompts and "No pending approval" errors that occurred when text went through the backend message pipeline. The frontend intercepts approval keywords in sendMessage() and routes them through sendApprovalAction() — the same code path as clicking the Approve/Deny/Always buttons on the card. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates the web chat UI to treat simple approval keywords typed into the chat input as direct approval/deny actions when an approval card is pending, avoiding backend-side duplicate approval handling. Also extends E2E coverage for this behavior and tweaks GitHub label automation to ensure a migration label exists and is applied based on changed paths.
Changes:
- Intercept approval/deny keywords in
sendMessage()and route them throughsendApprovalAction()instead of posting a normal chat message. - Add Playwright E2E tests covering keyword interception and passthrough behavior.
- Update PR label automation to create/apply a “DB MIGRATION” label based on migration-related file changes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/channels/web/static/app.js |
Adds frontend keyword interception for pending approval cards before normal message sending. |
tests/e2e/scenarios/test_tool_approval.py |
Adds new E2E tests validating text-based approval interception and non-interception for normal text. |
.github/workflows/pr-label-scope.yml |
Ensures “DB MIGRATION” label exists, pins labeler action, and updates permissions. |
.github/scripts/create-labels.sh |
Adds creation of the “DB MIGRATION” label in the label bootstrap script. |
.github/labeler.yml |
Adds path-based mapping for “DB MIGRATION” label and updates header comments. |
Comments suppressed due to low confidence (2)
src/channels/web/static/app.js:763
- The interception logic only inspects the last
.approval-card. If the last card was just resolved (it stays in the DOM for the 1.5s removal timeout) while an older card is still pending, keyword input won’t be intercepted and will instead be sent as a normal message. To match the intended behavior (“skip already-resolved cards”), walk the cards from newest→oldest and select the first card that is unresolved (and has a request id) before mapping keywords tosendApprovalAction().
}
if (lastAssistant) lastAssistant.setAttribute('data-streaming', 'true');
// Mark turn as having received content so the Done safety net
// does not trigger a spurious loadHistory() for streaming responses.
src/channels/web/static/app.js:771
- The PR description lists aliases like
y,n,approve,deny,ok, but the code also intercepts additional keywords (e.g., single-lettera,reject,cancel,yes always,approve always, etc.). Interceptingain particular is very broad and will prevent users from sending a normal one-letter message whenever an approval card is visible. Either narrow the accepted keyword set to what’s intended, or update the PR description/tests to reflect and justify the expanded behavior.
// Accumulate chunks and debounce rendering at 50ms intervals
_streamBuffer += data.content;
// Force flush when buffer exceeds 10K chars to prevent memory buildup
if (_streamBuffer.length > 10000) {
appendToLastAssistant(_streamBuffer);
_streamBuffer = '';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d5d32c8 to
b131ee6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback: instead of checking the last card and then separately checking if it's resolved, find the most recent unresolved card directly. Handles the edge case where the last card is resolved (during 1.5s removal animation) but an earlier one isn't. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses review feedback: adds a test where two approval cards are visible, the newer one is resolved via button click, then typing "yes" correctly targets the older unresolved card instead of falling through. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* fix(web): intercept approval text input ("yes"/"no"/"always") in chat
When a tool requires approval in the web UI, typing "yes", "no", or
"always" in the chat input now resolves the approval card directly
instead of sending a regular message. This prevents duplicate approval
prompts and "No pending approval" errors that occurred when text went
through the backend message pipeline.
The frontend intercepts approval keywords in sendMessage() and routes
them through sendApprovalAction() — the same code path as clicking
the Approve/Deny/Always buttons on the card.
[skip-regression-check]
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: find most recent unresolved approval card for text interception
Address review feedback: instead of checking the last card and then
separately checking if it's resolved, find the most recent unresolved
card directly. Handles the edge case where the last card is resolved
(during 1.5s removal animation) but an earlier one isn't.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test: add E2E test for skip-resolved-card behavior
Addresses review feedback: adds a test where two approval cards are
visible, the newer one is resolved via button click, then typing "yes"
correctly targets the older unresolved card instead of falling through.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
sendMessage()and routes them throughsendApprovalAction()— same code path as clicking the card buttonsTest plan
test_tool_approval.py:test_text_yes_intercepts_approval— verifies "yes" resolves card, no user bubble, input clearedtest_text_no_intercepts_denial— verifies "no" shows "Denied"test_text_always_intercepts_always— verifies "always" shows "Always approved"test_text_aliases_intercepted— tests "y", "n", "approve", "deny"test_text_approval_case_insensitive— tests "Yes", "YES", "No", "ALWAYS"test_normal_text_not_intercepted_with_approval_card— regular text passes through normallytest_text_approval_resolves_real_tool_call— full E2E with real mock LLM tool callcargo fmtclean,cargo clippyzero warnings🤖 Generated with Claude Code