fix: prevent context condensing on settings save when provider/model unchanged #9108

daniel-lxs · 2025-11-07T23:05:04Z

Problem

When clicking Save in Settings during an active task, context condensing would trigger immediately even when the configured threshold was 100%. This happened because the extension unconditionally rebuilt the running task's API handler, causing transient changes to model metadata (contextWindow, maxTokens) which triggered the allowedTokens fast-path in context condensing logic.

Root Cause

The issue was NOT about UI component unmounting or model info flicker (PR #9101). The actual problem:

Settings Save calls upsertProviderProfile or activateProviderProfile
These methods unconditionally rebuilt the API handler: task.api = buildApiHandler(providerSettings)
Rebuilding updates model metadata even when provider/model unchanged
Context condensing has two triggers:
- contextPercent >= threshold (user's 100% setting)
- prevContextTokens > allowedTokens (fast-path based on contextWindow/maxTokens)
Transient metadata changes triggered the fast-path, causing immediate condensing regardless of threshold

Solution

Added guards in both methods to only rebuild the API handler when provider OR model ID actually changes:

if (task && task.apiConfiguration) {
    const currentProvider = task.apiConfiguration.apiProvider
    const newProvider = providerSettings.apiProvider
    const currentModelId = getModelId(task.apiConfiguration)
    const newModelId = getModelId(providerSettings)

    if (currentProvider !== newProvider || currentModelId !== newModelId) {
        task.api = buildApiHandler(providerSettings)
    }
}

Changes

Added getModelId import to ClineProvider
Added guard in upsertProviderProfile to prevent unnecessary handler rebuilds
Added guard in activateProviderProfile to prevent unnecessary handler rebuilds
Added comprehensive test suite (9 tests, all passing) to verify behavior

Testing

✅ Handler NOT rebuilt when provider/model unchanged (fixes [BUG] Context condensing triggers on settings Save during active task #4430)
✅ Handler IS rebuilt when provider changes
✅ Handler IS rebuilt when model changes
✅ No errors when no task is running
✅ All existing tests pass (310 test files, 4022 tests)

Impact

This targeted fix prevents the "condense on save" behavior while preserving handler rebuilds when genuinely needed (provider or model change).

Important

Fixes unnecessary context condensing by ensuring API handler rebuilds only on provider/model changes in ClineProvider.

Behavior:
- Prevents unnecessary context condensing by only rebuilding API handler if provider or model changes in ClineProvider.
- Fixes issue [BUG] Context condensing triggers on settings Save during active task #4430 where context condensing triggered on settings save even with 100% threshold.
Code Changes:
- Adds updateTaskApiHandlerIfNeeded() in ClineProvider to check for provider/model changes before rebuilding.
- Modifies upsertProviderProfile and activateProviderProfile to use the new guard.
Testing:
- Adds ClineProvider.apiHandlerRebuild.spec.ts with 9 tests to verify handler rebuild logic.
- Tests ensure handler is rebuilt only when provider or model changes, and not when unchanged.
- Confirms no errors occur when no task is running.

^{This description was created by}^{for 5a2be5b. You can customize this summary. It will automatically update as commits are pushed.}

…unchanged Fixes #4430 The issue was that saving settings during an active task would unconditionally rebuild the API handler in both upsertProviderProfile and activateProviderProfile, even when the provider and model hadn't changed. This caused transient changes to model metadata (contextWindow, maxTokens) which triggered the allowedTokens fast-path in context condensing logic, causing immediate condensing regardless of the configured threshold. Changes: - Added guard in upsertProviderProfile to only rebuild API handler when provider OR model changes - Added guard in activateProviderProfile to only rebuild API handler when provider OR model changes - Added comprehensive test suite to verify handler is not rebuilt unnecessarily This prevents the "condense on save" behavior reported in issue #4430 while still rebuilding the handler when genuinely needed (provider or model change).

roomote · 2025-11-08T02:05:04Z

Rooviewer See task on Roo Cloud

Review complete. Code duplication successfully resolved with clean DRY refactoring. No new issues found.

Guard logic incorrectly handles undefined model IDs, causing unnecessary rebuilds when comparing undefined !== "model" evaluates to true

Previous reviews

eabaaa6: Review #1

b39308c: Review #2

_{Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.}

src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts

src/core/webview/ClineProvider.ts

- Capture original API reference before upsertProviderProfile call - Assert against original reference instead of self-comparison - Addresses ellipsis-dev review comment

daniel-lxs · 2025-11-08T02:21:58Z

Review Response

@ellipsis-dev comment: FIXED - Updated test to capture original API reference and assert against it instead of self-comparison.

@roomote comment: INCORRECT - The current logic handles all cases correctly:

Why roomote's suggestion would break the fix:

Current logic (currentModelId !== newModelId):

Both undefined: undefined !== undefined → false → No rebuild (correct)
Both same: "gpt-4" !== "gpt-4" → false → No rebuild (correct)
Different: "gpt-4" !== "claude" → true → Rebuild (correct)
User selects model: undefined !== "gpt-4" → true → Rebuild (correct - NEW model selected!)
User clears model: "gpt-4" !== undefined → true → Rebuild (correct - model REMOVED!)

Roomote's suggested logic (currentModelId !== undefined && newModelId !== undefined && currentModelId !== newModelId):

Both undefined: Skip check → No rebuild (correct by accident)
Both same: "gpt-4" !== "gpt-4" → false → No rebuild (correct)
Different: "gpt-4" !== "claude" → true → Rebuild (correct)
User selects model: undefined !== undefined fails first check → No rebuild (BUG! Should rebuild)
User clears model: "gpt-4" !== undefined fails second check → No rebuild (BUG! Should rebuild)

The undefined transitions ARE valid model changes:

Selecting a model (undefined → "gpt-4") changes the effective model used by the task
Clearing a model ("gpt-4" → undefined) also changes it
Both require handler rebuild to reflect the new model metadata

The current implementation is correct. All tests pass including the new comprehensive test suite.

roomote · 2025-11-08T02:27:07Z

Fixaroo See task on Roo Cloud

Analysis Complete: PR Author Is Correct

After thorough analysis of the guard logic and the getModelId implementation, the PR author's response is accurate and no changes are needed.

The Guard Logic Is Correct

The current implementation:

if (currentProvider !== newProvider || currentModelId !== newModelId) {
    task.api = buildApiHandler(providerSettings)
}

Why `undefined` Transitions Are Valid Model Changes

The getModelId function returns string | undefined:

export const getModelId = (settings: ProviderSettings): string | undefined => {
	const modelIdKey = modelIdKeys.find((key) => settings[key])
	return modelIdKey ? settings[modelIdKey] : undefined
}

All scenarios work correctly:

✅ Both undefined: undefined !== undefined → false → No rebuild (correct)
✅ Both same: "gpt-4" !== "gpt-4" → false → No rebuild (correct)
✅ Different models: "gpt-4" !== "claude" → true → Rebuild (correct)
✅ User selects model: undefined !== "gpt-4" → true → Rebuild (correct!)
✅ User clears model: "gpt-4" !== undefined → true → Rebuild (correct!)

Why The Reviewer's Concern Is Unfounded

The reviewer's suggested check for undefined would break scenarios 4 and 5, where:

A user selects a model for the first time (undefined → "gpt-4")
A user clears their model selection ("gpt-4" → undefined)

Both are legitimate model changes that require rebuilding the API handler to reflect the new configuration.

Conclusion

The PR correctly fixes issue #4430 by preventing unnecessary API handler rebuilds when the provider and model haven't actually changed, while still rebuilding when they do change (including transitions to/from undefined). The implementation is sound and all tests pass.

No code changes required.

src/core/webview/ClineProvider.ts

daniel-lxs self-assigned this Nov 7, 2025

github-project-automation bot added this to Roo Code Roadmap and Roo Code Roadmap Nov 7, 2025

github-project-automation bot moved this to Triage in Roo Code Roadmap Nov 7, 2025

github-project-automation bot moved this to New in Roo Code Roadmap Nov 7, 2025

hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Nov 7, 2025

daniel-lxs marked this pull request as ready for review November 8, 2025 02:04

daniel-lxs requested review from cte, jr and mrubens as code owners November 8, 2025 02:04

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bug Something isn't working labels Nov 8, 2025

ellipsis-dev bot reviewed Nov 8, 2025

View reviewed changes

src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts Outdated Show resolved Hide resolved

roomote bot reviewed Nov 8, 2025

View reviewed changes

src/core/webview/ClineProvider.ts Outdated Show resolved Hide resolved

test: fix tautological assertion in API handler rebuild test

eabaaa6

- Capture original API reference before upsertProviderProfile call - Assert against original reference instead of self-comparison - Addresses ellipsis-dev review comment

daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Nov 8, 2025

hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Nov 8, 2025

roomote bot approved these changes Nov 8, 2025

View reviewed changes

mrubens reviewed Nov 8, 2025

View reviewed changes

src/core/webview/ClineProvider.ts Outdated Show resolved Hide resolved

DRY

5a2be5b

roomote bot approved these changes Nov 8, 2025

View reviewed changes

mrubens approved these changes Nov 8, 2025

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 8, 2025

mrubens merged commit 78fc212 into main Nov 8, 2025
11 checks passed

github-project-automation bot moved this from New to Done in Roo Code Roadmap Nov 8, 2025

mrubens deleted the fix/prevent-condense-on-settings-save branch November 8, 2025 03:58

github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Nov 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: prevent context condensing on settings save when provider/model unchanged #9108

fix: prevent context condensing on settings save when provider/model unchanged #9108

Uh oh!

daniel-lxs commented Nov 7, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

roomote bot commented Nov 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

daniel-lxs commented Nov 8, 2025 •

edited

Loading

Uh oh!

roomote bot commented Nov 8, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix: prevent context condensing on settings save when provider/model unchanged #9108

fix: prevent context condensing on settings save when provider/model unchanged #9108

Uh oh!

Conversation

daniel-lxs commented Nov 7, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root Cause

Solution

Changes

Testing

Impact

Uh oh!

roomote bot commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daniel-lxs commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review Response

Why roomote's suggestion would break the fix:

The undefined transitions ARE valid model changes:

Uh oh!

roomote bot commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Analysis Complete: PR Author Is Correct

The Guard Logic Is Correct

Why undefined Transitions Are Valid Model Changes

Why The Reviewer's Concern Is Unfounded

Conclusion

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daniel-lxs commented Nov 7, 2025 •

edited by ellipsis-dev bot

Loading

roomote bot commented Nov 8, 2025 •

edited

Loading

daniel-lxs commented Nov 8, 2025 •

edited

Loading

roomote bot commented Nov 8, 2025 •

edited

Loading

Why `undefined` Transitions Are Valid Model Changes