Clean up the way we compute the current diff strategy (#2049)

* Clean up the way we compute the current diff strategy

* Add changeset

.changeset/orange-items-remember.md

@@ -0,0 +1,5 @@
+---
+"roo-cline": patch
+---
+
+Consolidate logic that computes the current diff strategy

src/core/Cline.ts

@@ -227,11 +227,8 @@ export class Cline extends EventEmitter<ClineEvents> {
 			telemetryService.captureTaskCreated(this.taskId)
 		}
 
-		// Initialize diffStrategy based on current state
-		this.updateDiffStrategy(
-			Experiments.isEnabled(experiments ?? {}, EXPERIMENT_IDS.DIFF_STRATEGY),
-			Experiments.isEnabled(experiments ?? {}, EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE),
-		)
+		// Initialize diffStrategy based on current state.
+		this.updateDiffStrategy(experiments ?? {})
 
 		onCreated?.(this)
 
@@ -266,25 +263,13 @@ export class Cline extends EventEmitter<ClineEvents> {
 		return getWorkspacePath(path.join(os.homedir(), "Desktop"))
 	}
 
-	// Add method to update diffStrategy
-	async updateDiffStrategy(experimentalDiffStrategy?: boolean, multiSearchReplaceDiffStrategy?: boolean) {
-		// If not provided, get from current state
-		if (experimentalDiffStrategy === undefined || multiSearchReplaceDiffStrategy === undefined) {
-			const { experiments: stateExperimental } = (await this.providerRef.deref()?.getState()) ?? {}
-			if (experimentalDiffStrategy === undefined) {
-				experimentalDiffStrategy = stateExperimental?.[EXPERIMENT_IDS.DIFF_STRATEGY] ?? false
-			}
-			if (multiSearchReplaceDiffStrategy === undefined) {
-				multiSearchReplaceDiffStrategy = stateExperimental?.[EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE] ?? false
-			}
-		}
-
-		this.diffStrategy = getDiffStrategy(
-			this.api.getModel().id,
-			this.fuzzyMatchThreshold,
-			experimentalDiffStrategy,
-			multiSearchReplaceDiffStrategy,
-		)
+	// Add method to update diffStrategy.
+	async updateDiffStrategy(experiments: Partial<Record<ExperimentId, boolean>>) {
+		this.diffStrategy = getDiffStrategy({
+			model: this.api.getModel().id,
+			experiments,
+			fuzzyMatchThreshold: this.fuzzyMatchThreshold,
+		})
 	}
 
 	// Storing task to disk for history

src/core/__tests__/Cline.test.ts

@@ -304,7 +304,12 @@ describe("Cline", () => {
 
 			expect(cline.diffEnabled).toBe(true)
 			expect(cline.diffStrategy).toBeDefined()
-			expect(getDiffStrategySpy).toHaveBeenCalledWith("claude-3-5-sonnet-20241022", 0.9, false, false)
+
+			expect(getDiffStrategySpy).toHaveBeenCalledWith({
+				model: "claude-3-5-sonnet-20241022",
+				experiments: {},
+				fuzzyMatchThreshold: 0.9,
+			})
 		})
 
 		it("should pass default threshold to diff strategy when not provided", async () => {
@@ -321,7 +326,11 @@ describe("Cline", () => {
 
 			expect(cline.diffEnabled).toBe(true)
 			expect(cline.diffStrategy).toBeDefined()
-			expect(getDiffStrategySpy).toHaveBeenCalledWith("claude-3-5-sonnet-20241022", 1.0, false, false)
+			expect(getDiffStrategySpy).toHaveBeenCalledWith({
+				model: "claude-3-5-sonnet-20241022",
+				experiments: {},
+				fuzzyMatchThreshold: 1.0,
+			})
 		})
 
 		it("should require either task or historyItem", () => {

src/core/config/CustomModesManager.ts

@@ -19,6 +19,7 @@ export class CustomModesManager {
 		private readonly context: vscode.ExtensionContext,
 		private readonly onUpdate: () => Promise<void>,
 	) {
+		// TODO: We really shouldn't have async methods in the constructor.
 		this.watchCustomModesFiles()
 	}
 

src/core/diff/DiffStrategy.ts

@@ -1,29 +1,28 @@
 import type { DiffStrategy } from "./types"
-import { UnifiedDiffStrategy } from "./strategies/unified"
 import { SearchReplaceDiffStrategy } from "./strategies/search-replace"
 import { NewUnifiedDiffStrategy } from "./strategies/new-unified"
 import { MultiSearchReplaceDiffStrategy } from "./strategies/multi-search-replace"
+import { EXPERIMENT_IDS, ExperimentId } from "../../shared/experiments"
+
+export type { DiffStrategy }
+
 /**
  * Get the appropriate diff strategy for the given model
  * @param model The name of the model being used (e.g., 'gpt-4', 'claude-3-opus')
  * @returns The appropriate diff strategy for the model
  */
-export function getDiffStrategy(
-	model: string,
-	fuzzyMatchThreshold?: number,
-	experimentalDiffStrategy: boolean = false,
-	multiSearchReplaceDiffStrategy: boolean = false,
-): DiffStrategy {
-	if (experimentalDiffStrategy) {
-		return new NewUnifiedDiffStrategy(fuzzyMatchThreshold)
-	}
 
-	if (multiSearchReplaceDiffStrategy) {
-		return new MultiSearchReplaceDiffStrategy(fuzzyMatchThreshold)
-	} else {
-		return new SearchReplaceDiffStrategy(fuzzyMatchThreshold)
-	}
+export type DiffStrategyName = "unified" | "multi-search-and-replace" | "search-and-replace"
+
+type GetDiffStrategyOptions = {
+	model: string
+	experiments: Partial<Record<ExperimentId, boolean>>
+	fuzzyMatchThreshold?: number
 }
 
-export type { DiffStrategy }
-export { UnifiedDiffStrategy, SearchReplaceDiffStrategy }
+export const getDiffStrategy = ({ fuzzyMatchThreshold, experiments }: GetDiffStrategyOptions): DiffStrategy =>
+	experiments[EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED]
+		? new NewUnifiedDiffStrategy(fuzzyMatchThreshold)
+		: experiments[EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE]
+			? new MultiSearchReplaceDiffStrategy(fuzzyMatchThreshold)
+			: new SearchReplaceDiffStrategy(fuzzyMatchThreshold)

src/core/prompts/__tests__/system.test.ts

@@ -171,7 +171,7 @@ describe("SYSTEM_PROMPT", () => {
 	beforeEach(() => {
 		// Reset experiments before each test to ensure they're disabled by default
 		experiments = {
-			[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: false,
+			[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: false,
 			[EXPERIMENT_IDS.INSERT_BLOCK]: false,
 		}
 	})
@@ -482,7 +482,7 @@ describe("SYSTEM_PROMPT", () => {
 		it("should disable experimental tools by default", async () => {
 			// Set experiments to explicitly disable experimental tools
 			const experimentsConfig = {
-				[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: false,
+				[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: false,
 				[EXPERIMENT_IDS.INSERT_BLOCK]: false,
 			}
 
@@ -516,7 +516,7 @@ describe("SYSTEM_PROMPT", () => {
 		it("should enable experimental tools when explicitly enabled", async () => {
 			// Set experiments for testing experimental features
 			const experimentsEnabled = {
-				[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: true,
+				[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: true,
 				[EXPERIMENT_IDS.INSERT_BLOCK]: true,
 			}
 
@@ -552,7 +552,7 @@ describe("SYSTEM_PROMPT", () => {
 		it("should selectively enable experimental tools", async () => {
 			// Set experiments for testing selective enabling
 			const experimentsSelective = {
-				[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: true,
+				[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: true,
 				[EXPERIMENT_IDS.INSERT_BLOCK]: false,
 			}
 
@@ -587,7 +587,7 @@ describe("SYSTEM_PROMPT", () => {
 
 		it("should list all available editing tools in base instruction", async () => {
 			const experiments = {
-				[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: true,
+				[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: true,
 				[EXPERIMENT_IDS.INSERT_BLOCK]: true,
 			}
 
@@ -615,7 +615,7 @@ describe("SYSTEM_PROMPT", () => {
 		})
 		it("should provide detailed instructions for each enabled tool", async () => {
 			const experiments = {
-				[EXPERIMENT_IDS.SEARCH_AND_REPLACE]: true,
+				[EXPERIMENT_IDS.DIFF_STRATEGY_SEARCH_AND_REPLACE]: true,
 				[EXPERIMENT_IDS.INSERT_BLOCK]: true,
 			}
 

src/core/webview/ClineProvider.ts

@@ -102,7 +102,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 	protected mcpHub?: McpHub // Change from private to protected
 	private latestAnnouncementId = "mar-20-2025-3-10" // update to some unique identifier when we add a new announcement
 	private settingsImportedAt?: number
-	private contextProxy: ContextProxy
+	public readonly contextProxy: ContextProxy
 	public readonly providerSettingsManager: ProviderSettingsManager
 	public readonly customModesManager: CustomModesManager
 
@@ -1539,6 +1539,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 							t("common:confirmation.just_this_message"),
 							t("common:confirmation.this_and_subsequent"),
 						)
+
 						if (
 							(answer === t("common:confirmation.just_this_message") ||
 								answer === t("common:confirmation.this_and_subsequent")) &&
@@ -1547,9 +1548,11 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 							message.value
 						) {
 							const timeCutoff = message.value - 1000 // 1 second buffer before the message to delete
+
 							const messageIndex = this.getCurrentCline()!.clineMessages.findIndex(
 								(msg) => msg.ts && msg.ts >= timeCutoff,
 							)
+
 							const apiConversationHistoryIndex =
 								this.getCurrentCline()?.apiConversationHistory.findIndex(
 									(msg) => msg.ts && msg.ts >= timeCutoff,
@@ -1570,6 +1573,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 										const nextUserMessageIndex = this.getCurrentCline()!.clineMessages.findIndex(
 											(msg) => msg === nextUserMessage,
 										)
+
 										// Keep messages before current message and after next user message
 										await this.getCurrentCline()!.overwriteClineMessages([
 											...this.getCurrentCline()!.clineMessages.slice(0, messageIndex),
@@ -1981,12 +1985,11 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 
 						await this.updateGlobalState("experiments", updatedExperiments)
 
-						// Update diffStrategy in current Cline instance if it exists
-						if (message.values[EXPERIMENT_IDS.DIFF_STRATEGY] !== undefined && this.getCurrentCline()) {
-							await this.getCurrentCline()!.updateDiffStrategy(
-								Experiments.isEnabled(updatedExperiments, EXPERIMENT_IDS.DIFF_STRATEGY),
-								Experiments.isEnabled(updatedExperiments, EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE),
-							)
+						const currentCline = this.getCurrentCline()
+
+						// Update diffStrategy in current Cline instance if it exists.
+						if (message.values[EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED] !== undefined && currentCline) {
+							await currentCline.updateDiffStrategy(updatedExperiments)
 						}
 
 						await this.postStateToWebview()
@@ -2084,13 +2087,13 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 				language,
 			} = await this.getState()
 
-			// Create diffStrategy based on current model and settings
-			const diffStrategy = getDiffStrategy(
-				apiConfiguration.apiModelId || apiConfiguration.openRouterModelId || "",
+			// Create diffStrategy based on current model and settings.
+			const diffStrategy = getDiffStrategy({
+				model: apiConfiguration.apiModelId || apiConfiguration.openRouterModelId || "",
+				experiments,
 				fuzzyMatchThreshold,
-				Experiments.isEnabled(experiments, EXPERIMENT_IDS.DIFF_STRATEGY),
-				Experiments.isEnabled(experiments, EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE),
-			)
+			})
+
 			const cwd = this.cwd
 
 			const mode = message.mode ?? defaultModeSlug
@@ -2146,6 +2149,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 	public async handleModeSwitch(newMode: Mode) {
 		// Capture mode switch telemetry event
 		const currentTaskId = this.getCurrentCline()?.taskId
+
 		if (currentTaskId) {
 			telemetryService.captureModeSwitch(currentTaskId, newMode)
 		}
@@ -2162,8 +2166,10 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 		// If this mode has a saved config, use it
 		if (savedConfigId) {
 			const config = listApiConfig?.find((c) => c.id === savedConfigId)
+
 			if (config?.name) {
 				const apiConfig = await this.providerSettingsManager.loadConfig(config.name)
+
 				await Promise.all([
 					this.updateGlobalState("currentApiConfigName", config.name),
 					this.updateApiConfiguration(apiConfig),
@@ -2175,6 +2181,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 
 			if (currentApiConfigName) {
 				const config = listApiConfig?.find((c) => c.name === currentApiConfigName)
+
 				if (config?.id) {
 					await this.providerSettingsManager.setModeConfig(newMode, config.id)
 				}

src/exports/roo-code.d.ts

@@ -252,11 +252,11 @@ type GlobalSettings = {
 	fuzzyMatchThreshold?: number | undefined
 	experiments?:
 		| {
-				experimentalDiffStrategy: boolean
 				search_and_replace: boolean
+				experimentalDiffStrategy: boolean
+				multi_search_and_replace: boolean
 				insert_content: boolean
 				powerSteering: boolean
-				multi_search_and_replace: boolean
 		  }
 		| undefined
 	language?:

src/exports/types.ts

@@ -255,11 +255,11 @@ type GlobalSettings = {
 	fuzzyMatchThreshold?: number | undefined
 	experiments?:
 		| {
-				experimentalDiffStrategy: boolean
 				search_and_replace: boolean
+				experimentalDiffStrategy: boolean
+				multi_search_and_replace: boolean
 				insert_content: boolean
 				powerSteering: boolean
-				multi_search_and_replace: boolean
 		  }
 		| undefined
 	language?:

src/schemas/index.ts

@@ -275,11 +275,11 @@ export type CustomSupportPrompts = z.infer<typeof customSupportPromptsSchema>
  */
 
 export const experimentIds = [
-	"experimentalDiffStrategy",
 	"search_and_replace",
+	"experimentalDiffStrategy",
+	"multi_search_and_replace",
 	"insert_content",
 	"powerSteering",
-	"multi_search_and_replace",
 ] as const
 
 export const experimentIdsSchema = z.enum(experimentIds)
@@ -291,11 +291,11 @@ export type ExperimentId = z.infer<typeof experimentIdsSchema>
  */
 
 const experimentsSchema = z.object({
-	experimentalDiffStrategy: z.boolean(),
 	search_and_replace: z.boolean(),
+	experimentalDiffStrategy: z.boolean(),
+	multi_search_and_replace: z.boolean(),
 	insert_content: z.boolean(),
 	powerSteering: z.boolean(),
-	multi_search_and_replace: z.boolean(),
 })
 
 export type Experiments = z.infer<typeof experimentsSchema>

src/shared/experiments.ts

@@ -4,11 +4,11 @@ import { AssertEqual, Equals, Keys, Values } from "../utils/type-fu"
 export type { ExperimentId }
 
 export const EXPERIMENT_IDS = {
-	DIFF_STRATEGY: "experimentalDiffStrategy",
-	SEARCH_AND_REPLACE: "search_and_replace",
+	DIFF_STRATEGY_SEARCH_AND_REPLACE: "search_and_replace",
+	DIFF_STRATEGY_UNIFIED: "experimentalDiffStrategy",
+	DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE: "multi_search_and_replace",
 	INSERT_BLOCK: "insert_content",
 	POWER_STEERING: "powerSteering",
-	MULTI_SEARCH_AND_REPLACE: "multi_search_and_replace",
 } as const satisfies Record<string, ExperimentId>
 
 type _AssertExperimentIds = AssertEqual<Equals<ExperimentId, Values<typeof EXPERIMENT_IDS>>>
@@ -20,11 +20,11 @@ interface ExperimentConfig {
 }
 
 export const experimentConfigsMap: Record<ExperimentKey, ExperimentConfig> = {
-	DIFF_STRATEGY: { enabled: false },
-	SEARCH_AND_REPLACE: { enabled: false },
+	DIFF_STRATEGY_SEARCH_AND_REPLACE: { enabled: false },
+	DIFF_STRATEGY_UNIFIED: { enabled: false },
+	DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE: { enabled: false },
 	INSERT_BLOCK: { enabled: false },
 	POWER_STEERING: { enabled: false },
-	MULTI_SEARCH_AND_REPLACE: { enabled: false },
 }
 
 export const experimentDefault = Object.fromEntries(

webview-ui/src/components/settings/AdvancedSettings.tsx

@@ -68,8 +68,8 @@ export const AdvancedSettings = ({
 							setCachedStateField("diffEnabled", e.target.checked)
 							if (!e.target.checked) {
 								// Reset both experimental strategies when diffs are disabled.
-								setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY, false)
-								setExperimentEnabled(EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE, false)
+								setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED, false)
+								setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE, false)
 							}
 						}}>
 						<span className="font-medium">{t("settings:advanced.diff.label")}</span>
@@ -87,22 +87,31 @@ export const AdvancedSettings = ({
 							</label>
 							<Select
 								value={
-									experiments[EXPERIMENT_IDS.DIFF_STRATEGY]
+									experiments[EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED]
 										? "unified"
-										: experiments[EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE]
+										: experiments[EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE]
 											? "multiBlock"
 											: "standard"
 								}
 								onValueChange={(value) => {
 									if (value === "standard") {
-										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY, false)
-										setExperimentEnabled(EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE, false)
+										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED, false)
+										setExperimentEnabled(
+											EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE,
+											false,
+										)
 									} else if (value === "unified") {
-										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY, true)
-										setExperimentEnabled(EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE, false)
+										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED, true)
+										setExperimentEnabled(
+											EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE,
+											false,
+										)
 									} else if (value === "multiBlock") {
-										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY, false)
-										setExperimentEnabled(EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE, true)
+										setExperimentEnabled(EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED, false)
+										setExperimentEnabled(
+											EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE,
+											true,
+										)
 									}
 								}}>
 								<SelectTrigger className="w-full">
@@ -121,13 +130,11 @@ export const AdvancedSettings = ({
 								</SelectContent>
 							</Select>
 							<div className="text-vscode-descriptionForeground text-sm mt-1">
-								{!experiments[EXPERIMENT_IDS.DIFF_STRATEGY] &&
-									!experiments[EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE] &&
-									t("settings:advanced.diff.strategy.descriptions.standard")}
-								{experiments[EXPERIMENT_IDS.DIFF_STRATEGY] &&
-									t("settings:advanced.diff.strategy.descriptions.unified")}
-								{experiments[EXPERIMENT_IDS.MULTI_SEARCH_AND_REPLACE] &&
-									t("settings:advanced.diff.strategy.descriptions.multiBlock")}
+								{experiments[EXPERIMENT_IDS.DIFF_STRATEGY_UNIFIED]
+									? t("settings:advanced.diff.strategy.descriptions.unified")
+									: experiments[EXPERIMENT_IDS.DIFF_STRATEGY_MULTI_SEARCH_AND_REPLACE]
+										? t("settings:advanced.diff.strategy.descriptions.multiBlock")
+										: t("settings:advanced.diff.strategy.descriptions.standard")}
 							</div>
 						</div>