Skip to content

fix(desktop): preserve multiline approval descriptions - #62092

Open
BlackishGreen33 wants to merge 3 commits into
NousResearch:mainfrom
BlackishGreen33:bg/desktop/preserve-multiline-approval
Open

fix(desktop): preserve multiline approval descriptions#62092
BlackishGreen33 wants to merge 3 commits into
NousResearch:mainfrom
BlackishGreen33:bg/desktop/preserve-multiline-approval

Conversation

@BlackishGreen33

@BlackishGreen33 BlackishGreen33 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #61249.

Desktop approval descriptions can be hard to review: the floating fallback cuts them down to one line, while the inline tool approval card does not show them at all.

This change preserves line breaks, wraps long text, and lets very long descriptions scroll inside a limited-height area on both approval surfaces.

This keeps the existing plain-text approval payload. It does not add a new rich-content field or renderer.

Related Issue

Fixes #61249

Changes Made

  • Preserve line breaks in fallback approval descriptions.
  • Show approval descriptions on inline tool cards.
  • Wrap long text and limit description height with scrolling.
  • Add focused regression tests for both surfaces.

Verification

  • npm --workspace apps/desktop run test:ui -- approval.test.tsx approval-group.test.tsx (18 passed)
  • npm --workspace apps/desktop run typecheck
  • npm --workspace apps/desktop exec eslint -- src/components/assistant-ui/tool/approval.tsx src/components/assistant-ui/tool/approval.test.tsx
  • npx prettier --check apps/desktop/src/components/assistant-ui/tool/approval.tsx apps/desktop/src/components/assistant-ui/tool/approval.test.tsx
  • npm audit (0 vulnerabilities)
  • git diff --check

Checklist

  • Linked the source issue.
  • Searched for existing open PRs.
  • Added focused test coverage.
  • Kept the change limited to the Desktop approval surfaces.

Copilot AI review requested due to automatic review settings July 10, 2026 13:20

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: fixes feature issue #61249 (desktop approval bar clamps the description to one line). Distinct from the closed alternate approaches #45828 / #44948 (#44864 cluster). Keeps the plain-text approval payload; no new rich-content field.

@rudidev08

Copy link
Copy Markdown

This PR fixes the floating bar, but the approval card on the tool call still doesn't show the description. Its "Command" toggle shows a placeholder instead of the real command. Asking "what folder are you in?" runs pwd, but the card only shows:

<terminal> (plugin approval rule)

So you approve without seeing the command. Showing the description on that card too would close it.

Copy link
Copy Markdown
Contributor Author

Thanks for catching this. You're right: once the inline tool approval card is mounted, the floating fallback is hidden, so the description was not visible there. The Command toggle could therefore show only the plugin approval label.

I've updated the PR to show the approval description on the inline card as well, with the same multi-line wrapping and bounded scrolling behavior. I also added a regression test for the <terminal> (plugin approval rule) case.

The follow-up is in 350e1bc1b. The focused tests, Desktop typecheck/build, and all required CI checks are passing.

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets and removed P3 Low — cosmetic, nice to have labels Jul 11, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused follow-up. No blocking problems were identified.

  • Current main still applies truncate to the floating description at apps/desktop/src/components/assistant-ui/tool/approval.tsx:86 and does not render a description in the inline approval bar.
  • PR head 350e1bc1b replaces the former with wrapped, bounded scrolling text at approval.tsx:82-89 and adds the inline description at approval.tsx:176-180.
  • The regression tests cover both surfaces, including the plugin-label scenario raised in the discussion (approval.test.tsx:74-84, 156-167).

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 11, 2026
@alt-glitch alt-glitch added comp/tools Tool registry, model_tools, toolsets and removed comp/tools Tool registry, model_tools, toolsets sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 11, 2026
@teknium1 teknium1 added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 11, 2026
@alt-glitch alt-glitch removed the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 12, 2026
@teknium1 teknium1 added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 12, 2026
# Conflicts:
#	apps/desktop/src/components/assistant-ui/tool/approval.test.tsx
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to open #62411: both render inline approval descriptions, while this PR also preserves multiline, bounded scrolling on the floating fallback. The added fallback scope makes this competing related work, not a duplicate.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have and removed sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Desktop approval bar truncates the approval description to one line — no way to review a multi-line change/diff before approving

5 participants