From 2303f67afb37d5f71437d7776210581f28a92aec Mon Sep 17 00:00:00 2001 From: Chris Estreich Date: Sat, 29 Mar 2025 23:08:37 -0700 Subject: [PATCH] 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 | 5 +++ src/core/Cline.ts | 33 ++++----------- src/core/__tests__/Cline.test.ts | 13 +++++- src/core/config/CustomModesManager.ts | 1 + src/core/diff/DiffStrategy.ts | 33 ++++++++------- src/core/prompts/__tests__/system.test.ts | 12 +++--- src/core/webview/ClineProvider.ts | 33 +++++++++------ src/exports/roo-code.d.ts | 4 +- src/exports/types.ts | 4 +- src/schemas/index.ts | 8 ++-- src/shared/experiments.ts | 12 +++--- .../components/settings/AdvancedSettings.tsx | 41 +++++++++++-------- 12 files changed, 106 insertions(+), 93 deletions(-) create mode 100644 .changeset/orange-items-remember.md diff --git a/.changeset/orange-items-remember.md b/.changeset/orange-items-remember.md new file mode 100644 index 0000000000..180538a200 --- /dev/null +++ b/.changeset/orange-items-remember.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Consolidate logic that computes the current diff strategy diff --git a/src/core/Cline.ts b/src/core/Cline.ts index 1624ff956f..27f8e62356 100644 --- a/src/core/Cline.ts +++ b/src/core/Cline.ts @@ -227,11 +227,8 @@ export class Cline extends EventEmitter { 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 { 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>) { + this.diffStrategy = getDiffStrategy({ + model: this.api.getModel().id, + experiments, + fuzzyMatchThreshold: this.fuzzyMatchThreshold, + }) } // Storing task to disk for history diff --git a/src/core/__tests__/Cline.test.ts b/src/core/__tests__/Cline.test.ts index 68c3208876..1fa9453c3c 100644 --- a/src/core/__tests__/Cline.test.ts +++ b/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", () => { diff --git a/src/core/config/CustomModesManager.ts b/src/core/config/CustomModesManager.ts index cfbe29dcb9..efa3366aee 100644 --- a/src/core/config/CustomModesManager.ts +++ b/src/core/config/CustomModesManager.ts @@ -19,6 +19,7 @@ export class CustomModesManager { private readonly context: vscode.ExtensionContext, private readonly onUpdate: () => Promise, ) { + // TODO: We really shouldn't have async methods in the constructor. this.watchCustomModesFiles() } diff --git a/src/core/diff/DiffStrategy.ts b/src/core/diff/DiffStrategy.ts index e532aec4b0..fe354196c6 100644 --- a/src/core/diff/DiffStrategy.ts +++ b/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> + 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) diff --git a/src/core/prompts/__tests__/system.test.ts b/src/core/prompts/__tests__/system.test.ts index 0e9d643923..8fd0046501 100644 --- a/src/core/prompts/__tests__/system.test.ts +++ b/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, } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 8087898c21..0487d1bb98 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -102,7 +102,7 @@ export class ClineProvider extends EventEmitter 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 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 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 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 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 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 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 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 implements if (currentApiConfigName) { const config = listApiConfig?.find((c) => c.name === currentApiConfigName) + if (config?.id) { await this.providerSettingsManager.setModeConfig(newMode, config.id) } diff --git a/src/exports/roo-code.d.ts b/src/exports/roo-code.d.ts index 2f71c6662e..f8bd27da01 100644 --- a/src/exports/roo-code.d.ts +++ b/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?: diff --git a/src/exports/types.ts b/src/exports/types.ts index fb3260d4f0..725a458a49 100644 --- a/src/exports/types.ts +++ b/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?: diff --git a/src/schemas/index.ts b/src/schemas/index.ts index eef9ed3cd7..ff3417b8c7 100644 --- a/src/schemas/index.ts +++ b/src/schemas/index.ts @@ -275,11 +275,11 @@ export type CustomSupportPrompts = z.infer */ 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 */ 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 diff --git a/src/shared/experiments.ts b/src/shared/experiments.ts index b731863e0b..9d931d5a07 100644 --- a/src/shared/experiments.ts +++ b/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 type _AssertExperimentIds = AssertEqual>> @@ -20,11 +20,11 @@ interface ExperimentConfig { } export const experimentConfigsMap: Record = { - 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( diff --git a/webview-ui/src/components/settings/AdvancedSettings.tsx b/webview-ui/src/components/settings/AdvancedSettings.tsx index e0a909a373..a54386ad30 100644 --- a/webview-ui/src/components/settings/AdvancedSettings.tsx +++ b/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) } }}> {t("settings:advanced.diff.label")} @@ -87,22 +87,31 @@ export const AdvancedSettings = ({
- {!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")}