Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Jul 11, 2025

BEFORE
image

AFTER
image

BEFORE
image

AFTER
Pasted_Image_2025-07-15__12_57 PM

BEFORE
image

AFTER
image
image

Related GitHub Issue

Closes: #2579

Roo Code Task Context (Optional)

N/A

Description

This PR fixes the confusing auto-approve checkbox states by implementing the following changes:

Key Implementation Details:

  • Added derived state hasEnabledOptions to check if any auto-approve sub-options are enabled
  • Introduced effectiveAutoApprovalEnabled to ensure the master checkbox reflects the actual state
  • Master checkbox is now disabled when no sub-options are selected, preventing the confusing "checked with None" state
  • Display text now correctly shows "None" when no options are enabled, regardless of checkbox state
  • Added automatic master checkbox toggling: when the last sub-option is disabled, the master automatically unchecks
  • Added tooltip explaining why the checkbox is disabled: "Select at least one option to enable auto-approval"

Design Choices:

  • Used derived state pattern to ensure UI consistency without complex state management
  • Applied changes to both the chat UI (AutoApproveMenu) and settings page (AutoApproveSettings) for consistency
  • Updated the isAutoApproved function in ChatView to respect the new logic, preventing auto-approval when only the master is checked

Translation Updates:

  • Added new translation key autoApprove.selectOptionsFirst to all 17 supported languages
  • All translations were validated using the translation validation script

Areas for Review Focus:

  • The automatic toggling behavior when enabling/disabling sub-options
  • Consistency between chat UI and settings page behavior
  • The disabled state tooltip implementation

Test Procedure

Unit Tests Added:

  • Created comprehensive tests in AutoApproveMenu.spec.tsx covering:
    • Checkbox disabled state when no options selected
    • Display text showing "None" appropriately
    • Automatic master checkbox toggling behavior
    • User interaction scenarios
  • Added tests in ChatView.auto-approve-new.spec.tsx to verify auto-approval doesn't trigger when only master is enabled

Manual Testing Steps:

  1. Open Roo Code and navigate to the chat interface
  2. Click on the Auto-approve section to expand it
  3. Verify the master checkbox is disabled when no sub-options are selected
  4. Select one or more sub-options and verify:
    • Master checkbox becomes enabled and checked
    • Display text shows the selected options
  5. Uncheck all sub-options and verify:
    • Master checkbox automatically unchecks and becomes disabled
    • Display text shows "None"
  6. Test the same behavior in Settings > Auto-approve section
  7. Verify auto-approval only works when both master is checked AND at least one sub-option is enabled

Test Commands:

# Run the new unit tests
cd webview-ui && npx vitest src/components/chat/__tests__/AutoApproveMenu.spec.tsx
cd webview-ui && npx vitest src/components/chat/__tests__/ChatView.auto-approve-new.spec.tsx

All tests are passing ✅

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

[Screenshots to be added showing the before/after states of the auto-approve checkbox]

Documentation Updates

  • No documentation updates are required.
  • Yes, documentation updates are required. (Please describe what needs to be updated or link to a PR in the docs repository).

Additional Notes

This implementation follows the proposed solution from the issue, preventing the confusing states where:

  • Checkbox is checked while displaying "None"
  • Checkbox is unchecked while displaying selected options

The solution maintains backward compatibility with existing saved settings.

Get in Touch

[Your Discord username]


Important

Improves auto-approve checkbox logic by introducing derived states and updating UI to prevent misleading states, with consistent behavior across components and updated translations.

  • Behavior:
    • Introduces hasEnabledOptions and effectiveAutoApprovalEnabled states in AutoApproveMenu.tsx and ChatView.tsx to manage checkbox states.
    • Master checkbox is disabled when no sub-options are selected, preventing misleading states.
    • Automatic toggling of master checkbox when sub-options are changed.
    • Tooltip added to explain disabled state.
  • Components:
    • Updates AutoApproveMenu.tsx and ChatView.tsx to use new state logic.
    • Consistent behavior across chat UI and settings page.
  • Testing:
    • Adds unit tests in AutoApproveMenu.spec.tsx and ChatView.auto-approve-new.spec.tsx to cover new behavior.
  • Translations:
    • Adds autoApprove.selectOptionsFirst key to 17 languages for tooltip consistency.

This description was created by Ellipsis for fe07cc6. You can customize this summary. It will automatically update as commits are pushed.

Copilot AI review requested due to automatic review settings July 11, 2025 16:31
@hannesrudolph hannesrudolph requested review from cte, jr and mrubens as code owners July 11, 2025 16:31
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. bug Something isn't working UI/UX UI/UX related or focused labels Jul 11, 2025
@delve-auditor
Copy link

delve-auditor bot commented Jul 11, 2025

No security or compliance issues detected. Reviewed everything up to fe07cc6.

Security Overview
  • 🔎 Scanned files: 44 changed file(s)
Detected Code Changes

The diff is too large to display a summary of code changes.

Reply to this PR with @delve-auditor followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.

This comment was marked as outdated.

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jul 11, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Draft / In Progress] in Roo Code Roadmap Jul 11, 2025
@daniel-lxs daniel-lxs marked this pull request as draft July 11, 2025 22:57
@daniel-lxs daniel-lxs moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 11, 2025
- Add derived state to check if any auto-approve options are enabled
- Disable master checkbox when no sub-options selected
- Show 'None' when no options enabled regardless of checkbox state
- Add automatic master checkbox toggling
- Add tooltip explaining disabled state
- Create custom hook useAutoApprovalState for shared logic
- Add accessibility aria-labels
- Update translations for all 17 supported languages
- Add comprehensive unit tests

Closes #2579
- Fix French translation: 'automatically' -> 'automatiquement'
- Fix Hindi translation: mixed scripts in 'कॉन्फ़िgerेशन' -> 'कॉन्फ़िगरेशन'
- Fix typo: 'CodeAccordian' -> 'CodeAccordion' in test file
- Remove redundant state variables in AutoApproveMenu.tsx
- Fix translation key namespace: use settings:autoApprove.selectOptionsFirst
- Add missing translation key to all language files
@hannesrudolph hannesrudolph force-pushed the fix/issue-2579-auto-approve-checkbox branch from 907cec5 to e734ec7 Compare July 14, 2025 21:34
@hannesrudolph hannesrudolph requested a review from Copilot July 14, 2025 21:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR resolves confusing auto-approve checkbox states by introducing derived state for sub-options, disabling the master toggle when no options are selected, and updating related UI and logic. It also adds new translation keys with tooltips and updates tests.

  • Added useAutoApprovalState hook to compute hasEnabledOptions and effectiveAutoApprovalEnabled.
  • Updated AutoApproveMenu and AutoApproveSettings to disable/check the master checkbox based on sub-options, with explanatory tooltips.
  • Refactored ChatView.isAutoApproved to prevent auto-approval when only the master toggle is enabled.
  • Added toggleAriaLabel, disabledAriaLabel, and selectOptionsFirst translation keys across all locales.

Reviewed Changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
webview-ui/src/hooks/useAutoApprovalState.ts Introduced hook for derived auto-approve state
webview-ui/src/components/settings/AutoApproveSettings.tsx Integrated derived hook, disabled master checkbox, added tooltip
webview-ui/src/components/chat/AutoApproveMenu.tsx Applied hook, updated master checkbox behavior, display text, tooltip
webview-ui/src/components/chat/ChatView.tsx Updated isAutoApproved to respect sub-option enablement
webview-ui/src/i18n/locales/**/settings.json & chat.json Added new tooltip and aria-label translation keys across locales

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Draft / In Progress] in Roo Code Roadmap Jul 14, 2025
- Fix tooltip rendering to conditionally show only when needed
- Reuse useAutoApprovalState hook in ChatView to avoid code duplication
- Improve code quality and consistency across auto-approve components
- Test hasEnabledOptions computation with various toggle combinations
- Test effectiveAutoApprovalEnabled logic
- Test memoization behavior
- Test edge cases including partial objects and non-boolean values
- Ensure 100% coverage of the hook's functionality
@hannesrudolph hannesrudolph moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 15, 2025
- Simplify conditional rendering in AutoApproveMenu by using single checkbox with conditional tooltip
- Extract duplicated toggles logic into reusable useAutoApprovalToggles hook
- Fix ESLint no-unused-vars errors after refactoring
@daniel-lxs daniel-lxs force-pushed the fix/issue-2579-auto-approve-checkbox branch from 3a605ff to 1ece8e9 Compare July 16, 2025 21:31
- Added missing alwaysApproveResubmit to useAutoApprovalToggles hook
- Updated AutoApproveSettings to use the shared hook instead of creating its own toggles
- Removed unused useMemo import from AutoApproveSettings
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jul 16, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 16, 2025
@mrubens mrubens merged commit 6cf376f into main Jul 16, 2025
17 checks passed
@mrubens mrubens deleted the fix/issue-2579-auto-approve-checkbox branch July 16, 2025 21:52
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 16, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jul 16, 2025
fxcl added a commit to tameslabs/Roo-Cline that referenced this pull request Jul 16, 2025
* main:
  fix: Resolve confusing auto-approve checkbox states (RooCodeInc#5602)
  fix: prevent empty mode names from being saved (RooCodeInc#5766) (RooCodeInc#5794)
  Format time in ISO 8601 (RooCodeInc#5793)
  fix: resolve DirectoryScanner memory leak and improve file limit handling (RooCodeInc#5785)
  Fix settings dirty check (RooCodeInc#5779)
  feat: increase Ollama API timeout values and extract as constants (RooCodeInc#5778)
  fix: Exclude Terraform and Terragrunt cache directories from checkpoints (RooCodeInc#4601) (RooCodeInc#5750)
  Move less commonly used provider settings into an advanced dropdown (RooCodeInc#5762)
  feat: Add configurable error & repetition limit with unified control (RooCodeInc#5654) (RooCodeInc#5752)
  list-files must include at least the first-level directory contents (RooCodeInc#5303)
  Update evals repo link (RooCodeInc#5758)
  Feature/vertex ai model name conversion (RooCodeInc#5728)
  fix(litellm): handle baseurl with paths correctly (RooCodeInc#5697)
  Add telemetry for todos (RooCodeInc#5746)
  feat: add undo functionality for enhance prompt feature (fixes RooCodeInc#5741) (RooCodeInc#5742)
  Fix max_tokens limit for moonshotai/kimi-k2-instruct on Groq (RooCodeInc#5740)
  Changeset version bump (RooCodeInc#5735)
  Add changeset for v3.23.12 patch release (RooCodeInc#5734)
  Update the max-token calculation in model-params to use the shared logic (RooCodeInc#5720)
  Changeset version bump (RooCodeInc#5719)
  chore: add changeset for v3.23.11 patch release (RooCodeInc#5718)
  Add Kimi K2 model and better support (RooCodeInc#5717)
  Fix: Remove invalid skip-checkout parameter from GitHub Actions workflows (RooCodeInc#5676)
  feat: add Cmd+Shift+. keyboard shortcut for previous mode switching (RooCodeInc#5695)
  Changeset version bump (RooCodeInc#5708)
  chore: add changeset for v3.23.10 patch release (RooCodeInc#5707)
  Add padding to the index model options (RooCodeInc#5706)
  fix: prioritize built-in model dimensions over custom dimensions (RooCodeInc#5705)
  Update CHANGELOG.md
  Changeset version bump (RooCodeInc#5702)
  chore: add changeset for v3.23.9 patch release (RooCodeInc#5701)
  Tweaks to command timeout error (RooCodeInc#5700)
  Update contributors list (RooCodeInc#5639)
  feat: enable Claude Code provider to run natively on Windows (RooCodeInc#5615)
  feat: Add configurable timeout for command execution (RooCodeInc#5668)
  feat: add gemini-embedding-001 model to code-index service (RooCodeInc#5698)
  fix: resolve vector dimension mismatch error when switching embedding models (RooCodeInc#5616) (RooCodeInc#5617)
  fix: [5424] return the cwd in the exec tool's response so that the model is not lost after subsequent calls (RooCodeInc#5667)
  Changeset version bump (RooCodeInc#5670)
  chore: add changeset for v3.23.8 patch release (RooCodeInc#5669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:XXL This PR changes 1000+ lines, ignoring generated files. UI/UX UI/UX related or focused

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Redesign Auto-approve; don't allow confusing checkbox status with labels

4 participants