XS⚠️ ◾ Simplify MCP tools permission prompt copy#995
Conversation
The dialog explaining each MCP tool request was verbose: a two-clause description plus a full-sentence explanation of each of the three buttons. Trim it to short, scannable copy that states what YakShaver wants and why, and makes clear what happens if you don't allow it, matching the terser tone used elsewhere (e.g. the screen recording permission dialog). Relates to #968
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
|
Skipped the automatic walkthrough recording (config: Reason: this change is a plain-text copy edit inside Wiring up a scripted trigger for a tool-approval prompt (e.g. a debug IPC hook) would be a reasonable follow-up if walkthroughs of permission-dialog copy become valuable, but doing so here would mean modifying the app just to make it recordable, which is out of scope for a copy-only PR. Before/after copy is documented in the PR description instead. |
There was a problem hiding this comment.
Pull request overview
This PR simplifies the user-facing copy in the MCP tools permission dialog to make it shorter and more scannable, aligning with the tone used by similar permission prompts.
Changes:
- Shortened the generic dialog description and the bespoke
capture_video_framepurpose text. - Condensed the three option bullets (Allow / Allow Always / Review-Correct).
- Removed the redundant
toolLabelvariable used only for repeating the tool name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {!showCorrectionForm && ( | ||
| <ul className="space-y-1.5 text-sm text-white/70"> | ||
| <li> | ||
| <span className="font-medium text-white/90">Allow</span> — let YakShaver use{" "} | ||
| {`"${toolLabel}"`} this one time. | ||
| <span className="font-medium text-white/90">Allow</span> — use it this once. | ||
| </li> | ||
| <li> | ||
| <span className="font-medium text-white/90">Allow Always</span> — let YakShaver | ||
| use {`"${toolLabel}"`} now and skip this prompt next time. | ||
| <span className="font-medium text-white/90">Allow Always</span> — use it now and | ||
| don't ask again. | ||
| </li> | ||
| <li> | ||
| <span className="font-medium text-white/90">Review / Correct…</span> — | ||
| don't run {`"${toolLabel}"`} as-is. You can leave a note telling YakShaver what to | ||
| change and try again, or stop the step entirely. | ||
| don't run it yet. Tell YakShaver what to change, or stop this step. | ||
| </li> |
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
🔍 muster review — PR #995Lenses: code-review ✅ · codex-rescue FindingsNone. The code-review lens found the diff clean: copy-only change, the unused Summary: blocking: 0 · major: 0 · minor: 0 · nit: 0 🔭 crows-nest: merge-gate decision = blocked — reason: review degraded (a lens failed) — a missing review is not a green light. This is an environment limitation (the codex-rescue second-opinion lens isn't installed here), not a finding against the code itself — CI is green and the PR is mergeable. A human reviewer's sign-off (or a second automated pass once codex-rescue is available) can clear this. |
tomek-i
added
armada:blocked
Summary
The MCP tools permission dialog (
ApprovalDialog.tsx) explained itself with a two-clause description plus a full sentence for each of the three response options, making it slower to scan than it needs to be. This trims the copy to short, plain language that states what YakShaver wants, why, and what happens if you don't allow it — matching the terser tone already used by the sibling screen-recording permission dialog.Closes #968
What changed
src/ui/src/components/workflow/ApprovalDialog.tsxcapture_video_framepurpose copy, and the three option bullets (Allow / Allow Always / Review-Correct); removed the now-unusedtoolLabelvariable that only existed to repeat the tool name in each bullet.Before / after
Description (generic tool with a server):
To keep working on your task, YakShaver needs to use the "X" tool (from Y). It only runs when you say it's OK.YakShaver needs to use "X" (from Y) to keep going.capture_video_framepurpose:YakShaver wants to capture a still frame from your screen recording so it can see what you're pointing at and keep working on your task accurately. It only runs when you say it's OK.YakShaver wants to grab a frame from your recording, so it can see what you're pointing at.Option bullets:
Allow — let YakShaver use "X" this one time./Allow Always — let YakShaver use "X" now and skip this prompt next time./Review / Correct… — don't run "X" as-is. You can leave a note telling YakShaver what to change and try again, or stop the step entirely.Allow — use it this once./Allow Always — use it now and don't ask again./Review / Correct… — don't run it yet. Tell YakShaver what to change, or stop this step.The tool name still appears once, prominently, in the dialog title (
Can YakShaver use "X"?), so repeating it in every bullet was redundant rather than clarifying.Decisions
Acceptance criteria
MacScreenRecordingPermissionDialog("YakShaver needs permission to record your screen.").Testing
ApprovalDialog.tsx(pure copy change, no logic/behavior change) and no existing test asserts the old copy strings.max-w-lg, and the new copy is shorter than before, so there's no truncation/layout risk.Ran the full validation suite in the worktree:
npm run build— clean (tsc, no errors).npm rebuild better-sqlite3 --build-from-source && npx vitest run --reporter=verbose --exclude 'src/ui/**'— 68 test files / 696 tests passed.npm --prefix src/ui install && npm --prefix src/ui test— 35 test files / 261 tests passed.npm run lint— clean (one pre-existing, unrelatedinfo-level suggestion inCloud360LiveView.tsx, not touched by this change).Screenshots
No screenshot captured (headless worktree without a display); the diff is copy-only within the existing dialog layout — see the before/after text above.
Follow-up items
ApprovalDialog.tsx's copy/behavior; adding a lightweight render test (e.g. asserting the title/description render for a couple of tool-name shapes) would guard future copy regressions but is out of scope for this copy-only PR.🤖 Generated with Claude Code