issue #685 - point 8 - Allow disabling the follow-up auto-select timer - #1214
issue #685 - point 8 - Allow disabling the follow-up auto-select timer#1214murd0cc wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThe follow-up auto-approval timeout can now be disabled with a zero value. The settings display shows ChangesFollow-up auto-approval timeout
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webview-ui/src/components/settings/AutoApproveSettings.tsx (1)
256-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused settings coverage.
The supplied regression test covers only
FollowUpSuggest. AddAutoApproveSettings.spec.tsxcoverage for the zero slider value, theDisabledlabel, and saving0throughsetCachedStateField. Include an unset/default case so the60000default cannot hide a missing value.As per coding guidelines, add focused tests for UI binding and save behavior, including explicit
0and unset/default cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/AutoApproveSettings.tsx` around lines 256 - 270, Add focused tests in AutoApproveSettings.spec.tsx for the follow-up timeout slider: verify an explicit value of 0 renders the “Disabled” label and saves 0 through setCachedStateField, and verify the unset/default state uses the 60000 default rather than masking a missing value. Keep coverage scoped to this slider’s UI binding and save behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/AutoApproveSettings.tsx`:
- Around line 266-270: Replace the hard-coded “Disabled” text in the
followupAutoApproveTimeoutMs display with the existing t(...) translation
helper, adding the corresponding translation key to the appropriate localization
resources. Preserve the current timeout formatting and conditional behavior.
---
Nitpick comments:
In `@webview-ui/src/components/settings/AutoApproveSettings.tsx`:
- Around line 256-270: Add focused tests in AutoApproveSettings.spec.tsx for the
follow-up timeout slider: verify an explicit value of 0 renders the “Disabled”
label and saves 0 through setCachedStateField, and verify the unset/default
state uses the 60000 default rather than masking a missing value. Keep coverage
scoped to this slider’s UI binding and save behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3ead47c-1ab7-4852-af1c-34376a0863fa
📒 Files selected for processing (3)
webview-ui/src/components/chat/FollowUpSuggest.tsxwebview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsxwebview-ui/src/components/settings/AutoApproveSettings.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
| {followupAutoApproveTimeoutMs === 0 | ||
| ? "Disabled" |
There was a problem hiding this comment.
Every other string in this component goes through t(), so this literal would render in English for the 17 non-English locales the repo ships. Should this use a new key under settings:autoApprove.followupQuestions (e.g. timeoutDisabled)?
| <div className="flex items-center gap-2"> | ||
| <Slider | ||
| min={1000} | ||
| min={0} |
There was a problem hiding this comment.
The existing AutoApproveSettings.spec.tsx never sets alwaysAllowFollowupQuestions: true, so this whole block — including the new min=0 and the 0 → "Disabled" branch — never mounts under test. Worth adding a 0/"Disabled" and a non-zero value case?
| data-testid="followup-timeout-slider" | ||
| /> | ||
| <span className="w-20">{followupAutoApproveTimeoutMs / 1000}s</span> | ||
| <span className="w-20"> |
There was a problem hiding this comment.
Since this is a user-visible settings state, consider adding a Playwright CT snapshot of the auto-approve timeout row showing the "Disabled" label. The settings screen already has visual tests to follow (ModelInfoView.visual.tsx, OpenAICompatible.visual.tsx), and the authoring pattern is in webview-ui/AGENTS.md under Visual Tests.
| !isAnswered && | ||
| !isFollowUpAutoApprovalPaused | ||
| !isFollowUpAutoApprovalPaused && | ||
| (followupAutoApproveTimeoutMs ?? DEFAULT_FOLLOWUP_TIMEOUT_MS) > 0 |
There was a problem hiding this comment.
Nothing clamps a negative followupAutoApproveTimeoutMs before it reaches this guard, so a > 0 vs !== 0 mutation would be indistinguishable on the values the suite tests (0, 3000, 5000). Worth adding a negative-value case?
github-actions
Bot
added
awaiting-author
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
webview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.fixture.tsx (1)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRender the localized label in the visual fixture.
TranslationContextreturns each key unchanged. Therefore,AutoApproveSettings.visual.tsxLine 17 snapshotssettings:autoApprove.followupQuestions.timeoutDisabled, notDisabled. The visual test cannot detect an incorrect or missing translation value. Map the new keys to deterministic English strings or load the English translation resources.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.fixture.tsx` around lines 20 - 25, Update AutoApproveSettingsFixture’s TranslationContext value so the new auto-approve timeout translation key resolves to a deterministic English label such as “Disabled” instead of returning the key unchanged. Preserve passthrough behavior for unrelated keys or load the existing English resources, ensuring AutoApproveSettings.visual.tsx renders the localized label in snapshots.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/settings/__tests__/AutoApproveSettings.spec.tsx`:
- Around line 165-177: Extend the tests around the follow-up timeout slider to
simulate slider changes and verify setCachedStateField receives
followupAutoApproveTimeoutMs for both 0 and a non-zero value. Add coverage for
the unset/default or disabled state, asserting the slider’s rendered value and
behavior when the default controls rendering. Preserve the existing text
assertions while covering UI binding, save behavior, and normalization for true,
false, and unset cases.
In
`@webview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.fixture.tsx`:
- Line 18: Replace the broad as any assertions in the fixture’s
ExtensionStateContext provider value with a typed provider value or typed test
factory that satisfies the ExtensionStateContext contract, including required
fields. For the i18n double assertion, remove it if possible; otherwise retain
it only with a nearby explanation documenting why it is unavoidable.
---
Nitpick comments:
In
`@webview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.fixture.tsx`:
- Around line 20-25: Update AutoApproveSettingsFixture’s TranslationContext
value so the new auto-approve timeout translation key resolves to a
deterministic English label such as “Disabled” instead of returning the key
unchanged. Preserve passthrough behavior for unrelated keys or load the existing
English resources, ensuring AutoApproveSettings.visual.tsx renders the localized
label in snapshots.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28798e0d-069b-4aaa-a029-ecd8fec76a2d
⛔ Files ignored due to path filters (1)
webview-ui/src/components/settings/__tests__/__screenshots__/auto-approve-followup-timeout-disabled-dark.pngis excluded by!**/*.png
📒 Files selected for processing (23)
webview-ui/src/components/chat/__tests__/FollowUpSuggest.spec.tsxwebview-ui/src/components/settings/AutoApproveSettings.tsxwebview-ui/src/components/settings/__tests__/AutoApproveSettings.spec.tsxwebview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.fixture.tsxwebview-ui/src/components/settings/__tests__/AutoApproveSettings.visual.tsxwebview-ui/src/i18n/locales/ca/settings.jsonwebview-ui/src/i18n/locales/de/settings.jsonwebview-ui/src/i18n/locales/en/settings.jsonwebview-ui/src/i18n/locales/es/settings.jsonwebview-ui/src/i18n/locales/fr/settings.jsonwebview-ui/src/i18n/locales/hi/settings.jsonwebview-ui/src/i18n/locales/id/settings.jsonwebview-ui/src/i18n/locales/it/settings.jsonwebview-ui/src/i18n/locales/ja/settings.jsonwebview-ui/src/i18n/locales/ko/settings.jsonwebview-ui/src/i18n/locales/nl/settings.jsonwebview-ui/src/i18n/locales/pl/settings.jsonwebview-ui/src/i18n/locales/pt-BR/settings.jsonwebview-ui/src/i18n/locales/ru/settings.jsonwebview-ui/src/i18n/locales/tr/settings.jsonwebview-ui/src/i18n/locales/vi/settings.jsonwebview-ui/src/i18n/locales/zh-CN/settings.jsonwebview-ui/src/i18n/locales/zh-TW/settings.json
🚧 Files skipped from review as they are similar to previous changes (2)
- webview-ui/src/components/settings/AutoApproveSettings.tsx
- webview-ui/src/components/chat/tests/FollowUpSuggest.spec.tsx
|
Used the PR fixer for help. Here's how it went Implemented and validated minimal fixes for all still-valid findings. Changes made Extended timeout-slider behavior coverage in AutoApproveSettings.spec.tsx. Added slider interaction assertions that verify setCachedStateField() receives followup timeout updates for both non-zero and zero transitions. Removed broad as any-style provider value usage by switching to typed ExtensionStateContextProvider with initialState. Still-valid findings: all reported items were valid and were fixed in-place with minimal edits in AutoApproveSettings.spec.tsx and AutoApproveSettings.visual.fixture.tsx. Tests passed: npx vitest run for AutoApproveSettings.spec.tsx. |
|
Implemented targeted coverage improvements by extending tests in FollowUpSuggest.spec.tsx. What changed
Validation run
No source changes were needed in AutoApproveSettings.tsx; existing tests for that area already pass in the targeted suite. |
murd0cc
changed the title
|
@edelauna Thanks for the on point review. |
2 participants
Related GitHub Issue
Tackles: #685
Description
The HOW:
The auto-select timer can now be disabled by setting the slider to 0 seconds, which displays as “Disabled” in the UI at
AutoApproveSettings.tsx:267.The timer logic in
FollowUpSuggest.tsx:47suppresses the countdown when the value is 0 (disabledTimeoutStateadded).Test Procedure
All tests pass.
Pre-Submission Checklist
FollowUpSuggest.spec.tsx:237confirms that no countdown appears when the timeout is disabled.Documentation Updates
Additional Notes
My first contribution ever. Go easy on me.
Get in Touch
I don't use discord. Github works for me, if it's good enough for you as well !
Summary by CodeRabbit
New Features
Bug Fixes