Skip to content

Keep the permission pill inside Arguments and share the Permissions menu - #32971

Merged
brennanb2025 merged 15 commits into
mainfrom
brennanb2025/agent-permission-pill-inline
Oct 25, 2026
Merged

brennanb2025 merged 15 commits into
mainfrom
brennanb2025/agent-permission-pill-inline

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 4 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​116 $\color{#cf222e}{\Huge{\mathbf{−}}}$​41 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​75
Prod 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​291 $\color{#cf222e}{\Huge{\mathbf{−}}}$​106 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​185

The permission pill occupied its own column inside Arguments, leaving long text wrapped in a narrow strip. It now shares the first text line: arguments follow it immediately, and wrapped lines use the full width beneath it. The pill and text share one native scroll area, keeping the remove button visible when reached by keyboard.

In Settings → Agents, the all-agents Permissions dropdown now sits immediately after its label, matching the per-agent row. The description and existing reset warning appear underneath. Selecting Approve for me still exposes the Agents without Approve for me control.

The existing textarea and save ownership remain in place. × or two distinct Backspaces at the start remove the pill and select the agent’s own settings. Recognized typed flags still convert to the pill, and removal clears a safely recognized flag from the unsaved draft so leaving the field cannot restore it. Pending writes and IME keep their existing behavior. Settings also shares the chat composer’s permission menu rendering.

Review path: full. This PR changes a saved-choice interaction and the review reproduced a permission resurrection regression. No saved format, request or capability changes are included.

Temporary: “Picking a mode here resets agents you set separately.” is temporary, removed by #33018. This PR alone still resets separately configured agents when the all-agents mode changes. No permission behavior changes are included in the layout update. The generated, removable Settings permission flag and each agent’s own settings are intended adaptations to the existing setting. Existing chat support for an agent’s own settings is tracked in #32334.

Validation: 24 focused renderer/persistence tests passed on 402f8d5, with earlier unchanged IME coverage retained. CI is green on that head: static analysis/typecheck, all five Node 24 unit shards, and macOS/Windows packages. Independent final review completed the reference, performance and readiness checks. The affected long-field flow and stationary keyboard tooltip check passed on the same head; text, caret, permission choice and save ownership were preserved. The shared tooltip normally dismisses during ancestor scrolling and remains available at rest. No agents were launched. Screenshots cover macOS dark at the recorded viewport; native scrollbar drag was source-traced, not exercised.

Follow-up to #31315.

Screenshots

Menu, custom arguments and removal images carry from bf6bc67 because those states remain unchanged. The long-field and keyboard-focus images are new on 402f8d5.

Shared Permissions menu

Permission pill with custom Arguments

Own settings after pill removal

Long Arguments wrapping beneath the pill

Keyboard focus reveals the remove button

Stationary keyboard help beside the pill

Main integration (10-25): merged the exact fetched main tip once as 97e2d9421940, resolving the comment-audit conflict while retaining the shared exported permission-name component. Bounded reviewer acceptance and 33 named tests passed. CI is green on this merge head, including static analysis/typecheck, all five Node24 test shards and both package jobs (run40193482426; no retry). Existing screenshot/QA evidence carries without a rerun.

Approved layout A update (10-25): the all-agents Permissions dropdown sits directly after its label, with the description and temporary reset warning below. The existing Approve for me fallback control is unchanged. Bounded review caught and corrected the loading-width reservation; the final head 2c054aaebdcc2c65d5a425c207da55cce39d6c53 retains it. All 65 tests in the three existing Settings suites and the normal hook passed. CI run 40226321532 is fully green, including all five unit shards, static analysis/typecheck and both platform packages, with no retry. One qa-gate/qa-drive screenshot run on this head verified both row states and saved Own settings/Full access transitions. Earlier untouched flows carry their existing evidence; no agents were launched or earlier flows replayed. QA copies stopped gracefully, real-config hashes are unchanged, and ticket 2372 is released. No later main or stack merge was made.

All-agents Permissions row — layout A

Approve for me with the extra control

@brennanb2025
brennanb2025 marked this pull request as ready for review October 23, 2026 23:25
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
Walkthrough

The changes update shared permission-mode controls and the all-agents permission dropdown. The arguments editor now uses a textarea and supports permission-chip removal during draft editing. Agent settings render disabled controls while host settings load. Tests cover the updated controls, loading display, and argument persistence behavior.

Priority: Normal

Merge Risk: Low · up to ac3fc

The Settings → Agents warning still differs from the approved wording; update the copy before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 25 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the two main changes: the permission pill now sits inside the Arguments field, and the Settings Permissions menu is shared with the chat composer. The raw summary confirms both changes…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is detailed and covers the user-visible changes, rationale, scope, screenshots, testing, platform validation, and review status. It does not use the template headings consistently, doe…
Full details: Title check

Explanation

The title names the two main changes: the permission pill now sits inside the Arguments field, and the Settings Permissions menu is shared with the chat composer. The raw summary confirms both changes (the extracted NativeChatPermissionModeItems component and the composite textarea with a chip). The title is specific and concise, and it is 72 characters long.

Full details: Description check

Explanation

The description is detailed and covers the user-visible changes, rationale, scope, screenshots, testing, platform validation, and review status. It does not use the template headings consistently, does not include a Fixes #... issue link, and omits the checklist and agent-skill boundary confirmation, but the required information is mostly present.

Full details: Docstring Coverage

Explanation

Docstring coverage is 13.04% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 25 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@brennanb2025 brennanb2025 changed the title Settings: keep the permission pill inside the Arguments box Settings: share the permission dropdown and keep the Arguments pill inline Oct 23, 2026
@brennanb2025 brennanb2025 changed the title Settings: share the permission dropdown and keep the Arguments pill inline Keep the permission pill inline and share Settings menu with reset Undo Oct 24, 2026

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 79b0fef5-fb9f-4aa3-86f7-02aef737db3a
📥 Commits

Reviewing files that changed from the base of the PR and between fb1f4b3 and da7b3e5.

📒 Files selected for processing: 26
  • src/main/runtime/rpc/methods/agent-launch-settings-work.test.ts
  • src/main/runtime/rpc/methods/client-ui.ts
  • src/main/runtime/runtime-client-agent-launch-settings-durability.test.ts
  • src/main/runtime/runtime-client-agent-launch-settings.test.ts
  • src/main/runtime/runtime-client-settings.ts
  • src/renderer/src/components/native-chat/NativeChatPermissionModePicker.test.tsx
  • src/renderer/src/components/native-chat/NativeChatPermissionModePicker.tsx
  • src/renderer/src/components/settings/AgentCatalogRow.tsx
  • src/renderer/src/components/settings/AgentLaunchDefaultsEditor.test.tsx
  • src/renderer/src/components/settings/AgentLaunchDefaultsEditor.tsx
  • src/renderer/src/components/settings/AgentLaunchSettingsCatalog.tsx
  • src/renderer/src/components/settings/AgentLaunchSettingsSection.test.tsx
  • src/renderer/src/components/settings/AgentLaunchSettingsSection.tsx
  • src/renderer/src/components/settings/AgentPermissionChoiceControls.tsx
  • src/renderer/src/components/settings/AgentsPane.test.tsx
  • src/renderer/src/components/settings/HostAgentLaunchSettings.test.tsx
  • src/renderer/src/components/settings/HostAgentLaunchSettings.tsx
  • src/renderer/src/components/settings/agent-launch-settings-transport.test.ts
  • src/renderer/src/components/settings/agent-launch-settings-transport.ts
  • src/renderer/src/components/settings/agent-permission-conversion-persistence.test.tsx
  • src/renderer/src/components/settings/agent-permission-reset-notice.tsx
  • src/renderer/src/i18n/en-runtime-required.json
  • src/renderer/src/i18n/locales/en.json
  • src/shared/agent-launch-permission-updates.ts
  • src/shared/agent-launch-runtime-capability.ts
  • src/shared/agent-launch-settings.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

@brennanb2025 brennanb2025 changed the title Keep the permission pill inline and share Settings menu with reset Undo Keep the permission pill inside Arguments and share the Permissions menu Oct 24, 2026

@coderabbitai coderabbitai Bot 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.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (2)

Major · Preserve the choices replaced by a global permission… · AgentLaunchSettingsCatalog.tsx:258-268

src/renderer/src/components/settings/AgentLaunchSettingsCatalog.tsx:258-268
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Preserve the choices replaced by a global permission change.

When a host supports AGENT_LAUNCH_PERMISSION_CHOICE_CAPABILITY, selecting either global permission control applies a global update through permissionUpdates. The mutation response returns only the resulting settings, so the UI receives neither the replaced choices nor reset entries. The prior per-agent choices cannot be restored through this UI, and no global reset toast or scoped Undo is rendered.

Carry the affected agents’ previous choices through the mutation result and render the reset toast with scoped Undo when the host returns them. Keep the older-host path notice-only without Undo.

Minor · Replace pasted line breaks with a space, not an empty… · AgentLaunchDefaultsEditor.tsx:173-183

src/renderer/src/components/settings/AgentLaunchDefaultsEditor.tsx:173-183
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Replace pasted line breaks with a space, not an empty string.

The textarea onChange deletes every CR and LF. Pasting --foo followed by a newline and --bar saves --foo--bar. The posix tokenizer treats that as one argument. The launch command then passes one malformed flag instead of two. Replacing the line break with a space keeps the tokens separate.

Suggested fix
-            onChange={(event) => setDraft(event.target.value.replace(/[\r\n]/g, ''))}
+            onChange={(event) => setDraft(event.target.value.replace(/[\r\n]+/g, ' '))}

Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ca904e73-4faf-428a-bb9d-b999cf60d569
Commits

Reviewing files that changed from the base of the PR and between da7b3e5 and 2f2923c.

Files selected for processing: 7
  • src/renderer/src/components/settings/AgentCatalogRow.tsx
  • src/renderer/src/components/settings/AgentLaunchDefaultsEditor.tsx
  • src/renderer/src/components/settings/AgentLaunchSettingsSection.tsx
  • src/renderer/src/components/settings/AgentPermissionChoiceControls.tsx
  • src/renderer/src/components/settings/HostAgentLaunchSettings.tsx
  • src/renderer/src/components/settings/agent-permission-conversion-persistence.test.tsx
  • src/renderer/src/i18n/locales/en.json
Files skipped from review as they are similar to previous changes: 1
  • src/renderer/src/i18n/locales/en.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5ab9a66c-b3d8-441b-b3d1-2a34fe4c6f3d
Commits

Reviewing files that changed from the base of the PR and between c764619 and b05f82f.

Files selected for processing: 2
  • src/renderer/src/components/settings/AgentLaunchDefaultsEditor.tsx
  • src/renderer/src/components/ui/textarea.tsx
Files skipped from review as they are similar to previous changes: 1
  • src/renderer/src/components/settings/AgentLaunchDefaultsEditor.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bd1cf31f-7654-4b9d-9857-98158277a562
Commits

Reviewing files that changed from the base of the PR and between b05f82f and ac3fc70.

Files selected for processing: 2
  • src/renderer/src/components/settings/agent-permission-conversion-persistence.test.tsx
  • src/renderer/src/i18n/en-runtime-required.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@brennanb2025

brennanb2025 commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

The permission pill previously left long Arguments text in a narrow column. It now shares the first text line, with later lines using the full box width. The existing textarea and field owner remain; × or two distinct Backspaces remove the pill, recognized flags convert to it, and removal cannot resurrect a safely recognized flag from the unsaved draft. IME and pending writes retain their behavior.

The approved Settings → Agents layout A also places the all-agents Permissions dropdown immediately after its label, using the existing per-agent row pattern. The description and reset warning are underneath. Approve for me still exposes the Agents without Approve for me control. The reset warning is temporary, removed by #33018; #32971 alone still resets separately configured agents when the all-agents choice changes.

Review: full review, reference comparison, performance and readiness evidence carries for untouched code. The bounded layout audit found a lost loading-width reservation in 52145c19; 2c054aaebdc restores the existing fixed width, and the reviewer accepted that exact correction. No open proven finding remains. No permission/save behavior, new state or format changes were added. The generated Settings chip is an intended adaptation of the existing permission setting; the shared tooltip’s dismissal during ancestor scrolling is an intended presentation detail, with stationary keyboard availability observed separately.

Verification on 2c054aaebdcc2c65d5a425c207da55cce39d6c53: 65 tests in the three existing Settings suites passed through tc-gate and the normal commit hook passed. CI is green on this exact head: static analysis/typecheck, all five Node24 unit shards, macOS and Windows packages, and the aggregate check (run 40226321532; no retry). One screenshot run through qa-gate/qa-drive on the corrected head passed both requested row states. Semantic clicks verified Own settings and Full access update all 32 affected saved choices while the all-agents selection remains Approve for me. No agents launched. Scope of the new screenshot run is only the two changed all-agents-row states; earlier passed flows were not replayed.

Evidence carry: the main integration 97e2d9421940 retained the exported permission-name component and removed the descriptive comment, accepted by the reviewer with 33 named tests and green CI. Earlier menu/custom-argument/removal screenshots carry from bf6bc673; long wrapping, keyboard-focus/caret/editing/removal and stationary tooltip evidence carry from 402f8d5012ff. The later shipped commits 402f8d5012ff (Arguments layout/native scroll) and 52145c19/2c054aaebdc (all-agents row) change only Settings tests-row flows; each has its named tests and affected screenshots. qa_head remains bf6bc673. Native scrollbar drag remains source-traced rather than exercised; screenshots cover macOS dark at the recorded viewport. All copies stopped gracefully; all recorded processes and assigned listeners are gone, real-config hashes are unchanged, and ticket 2372 was released after evidence review.

For Brennan’s review:

All-agents Permissions row — layout A

Approve for me with the extra control

Permissions menu — unchanged flow

Custom arguments — unchanged flow

Agent’s own settings after removal — unchanged flow

Long arguments wrap beneath the pill

Keyboard focus reveals the remove button

Stationary keyboard help

@brennanb2025
brennanb2025 merged commit 27e7f8f into main Oct 25, 2026
33 checks passed
@brennanb2025
brennanb2025 deleted the brennanb2025/agent-permission-pill-inline branch October 26, 2026 01:08
smwbev added a commit to smwbev/orca that referenced this pull request Oct 26, 2026
338 commits landed on main between dbde821 and fd25eb0. All seven catalogs and en-runtime-required.json conflicted, so they are taken from main and the prune is recomputed in one pass: the previous 1,633 keys plus 4, none left the set. stablyai#32971 dropped the readers of settings.agents.loadingHost and permissionChoiceDescription, stablyai#32334 the reader of permissionEachOwnDescription, and stablyai#31859 the reader of auto.hooks.useIpcEvents.56d3ec4203. The two permission descriptions exist only in en.json, so en.json sheds 1,637 and the other six catalogs 1,635. en.json goes from 17,143 entries to 15,506; en-runtime-required.json keeps 643 of them.

catalog-nested-settings.test.ts listed loadingHost among the agent settings it checks across catalogs; with its last reader gone, the key comes off that list and the test checks the other ten.

native-chat-shared-copy-matches-catalog.test.ts merged cleanly again and still differs from main by exactly the branch's edit.
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.

1 participant