fix(edit-unsuccessfull): introduce configurable relaxed diff thresholds and diagnostics#470
Open
nigeldelviero wants to merge 12 commits into
Open
Conversation
nigeldelviero
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable diffFuzzyThreshold (default 0.9) that flows from types and extension state through Task construction into MultiSearchReplaceDiffStrategy; failure diagnostics now include Levenshtein distance and tests cover threshold behavior and debug output. ChangesFuzzy Matching Threshold Configuration
Sequence DiagramsequenceDiagram
participant Webview as Webview UI
participant Cline as ClineProvider (ExtensionHost)
participant Task as Task
participant Strategy as MultiSearchReplaceDiffStrategy
participant Distance as distance()
Webview->>Cline: post updatedSettings (diffFuzzyThreshold)
Cline->>Task: create Task(diffFuzzyThreshold)
Task->>Strategy: new MultiSearchReplaceDiffStrategy(fuzzyThreshold)
Strategy->>Strategy: attempt fuzzy match
alt No sufficiently similar match
Strategy->>Distance: compute Levenshtein(searchChunk,bestMatchContent)
Distance-->>Strategy: distance value
Strategy->>Cline: return failure diagnostic (includes distance, similarity, lengths, threshold)
else Match found
Strategy-->>Task: return diff result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
webview-ui/src/components/settings/SettingsView.tsx (1)
364-432: ⚡ Quick winAdd a local webview-ui test for the buffered diff-threshold flow.
This new field now depends on
cachedState, dirty-state detection, and theupdateSettingspayload, but there’s no accompanying local test in the supplied UI changes proving that the slider edits stay buffered until Save and serializediffFuzzyThresholdcorrectly.As per coding guidelines, "
webview-ui/src/**/*.{ts,tsx}: Prefer localwebview-uitests for React/webview behavior such as component rendering, local state, hooks, form dirty-state, validation, or prop wiring. Add or update Vitest coverage underwebview-ui/src/**/__tests__instead of reaching forapps/vscode-e2e." Also, "webview-ui/src/**/SettingsView.{ts,tsx}: ForSettingsView, preserve the cached-state pattern: inputs should operate on localcachedStateuntil the user saves, and tests should distinguish automatic initialization from real user edits."Also applies to: 835-857
🤖 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/SettingsView.tsx` around lines 364 - 432, Add a local Vitest React test under webview-ui/src/**/__tests__ that mounts SettingsView, verifies the cachedState buffering behavior for the diffFuzzyThreshold slider (simulate initial value, user slider change, and ensure the component state reflects the edit but vscode.postMessage payload only contains the new diffFuzzyThreshold after calling the SettingsView handleSubmit Save flow), and assert that intermediate slider moves do not mutate the persisted setting until Save; target the SettingsView component, its cachedState behavior, the diffFuzzyThreshold controlled input, and the handleSubmit/updateSettings message to ensure serialization of diffFuzzyThreshold is correct.webview-ui/src/context/ExtensionStateContext.tsx (1)
212-212: ⚡ Quick winUse the shared default constant here too.
This duplicates the threshold default instead of reusing
DEFAULT_DIFF_FUZZY_THRESHOLD, so the webview bootstrap value can drift from the extension-host fallback later.Suggested change
import { type ProviderSettings, type ProviderSettingsEntry, type CustomModePrompts, type ModeConfig, type ExperimentId, type TodoItem, type TelemetrySetting, type OrganizationAllowList, type CloudOrganizationMembership, type ExtensionMessage, type ExtensionState, type MarketplaceInstalledMetadata, type SkillMetadata, type Command, type McpServer, RouterModels, ORGANIZATION_ALLOW_ALL, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + DEFAULT_DIFF_FUZZY_THRESHOLD, } from "`@roo-code/types`" @@ - diffFuzzyThreshold: 0.9, + diffFuzzyThreshold: DEFAULT_DIFF_FUZZY_THRESHOLD,🤖 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/context/ExtensionStateContext.tsx` at line 212, Replace the hardcoded diffFuzzyThreshold: 0.9 with the shared constant DEFAULT_DIFF_FUZZY_THRESHOLD so the webview uses the canonical default; update the object/initializer in ExtensionStateContext (where diffFuzzyThreshold is set) to reference DEFAULT_DIFF_FUZZY_THRESHOLD instead of 0.9 ensuring consistency with the extension-host fallback.
🤖 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 `@packages/types/src/global-settings.ts`:
- Around line 114-119: The persisted diffFuzzyThreshold schema currently allows
0.0–1.0 but the UI enforces 0.5–1.0, so clamp persisted values to the UI range
by updating the global schema for diffFuzzyThreshold: change the zod validator
for diffFuzzyThreshold to enforce a minimum of 0.5
(z.number().min(0.5).max(1).optional()) or alternatively apply a clamp/transform
in the schema to coerce any value below 0.5 up to 0.5; this ensures the
diffFuzzyThreshold field (symbol: diffFuzzyThreshold) cannot be persisted
outside the 50%–100% range forwarded by ClineProvider.
In `@webview-ui/src/components/settings/ContextManagementSettings.tsx`:
- Around line 413-435: The label and help text inside the SearchableSetting for
"context-diff-fuzzy-threshold" are hard-coded English; replace them with
localized strings using the project's i18n mechanism (e.g., the same translate/t
or useTranslation hook used elsewhere) so the UI picks up translation keys;
update the visible label text, the span "Diff Match Threshold", and the
descriptive help text below the Slider (references: SearchableSetting with
settingId "context-diff-fuzzy-threshold", Slider, diffFuzzyThreshold,
DEFAULT_DIFF_FUZZY_THRESHOLD, and setCachedStateField) to call the translation
function with new keys (e.g., contextManagement.diffFuzzyThreshold.label and
.description) instead of raw English.
---
Nitpick comments:
In `@webview-ui/src/components/settings/SettingsView.tsx`:
- Around line 364-432: Add a local Vitest React test under
webview-ui/src/**/__tests__ that mounts SettingsView, verifies the cachedState
buffering behavior for the diffFuzzyThreshold slider (simulate initial value,
user slider change, and ensure the component state reflects the edit but
vscode.postMessage payload only contains the new diffFuzzyThreshold after
calling the SettingsView handleSubmit Save flow), and assert that intermediate
slider moves do not mutate the persisted setting until Save; target the
SettingsView component, its cachedState behavior, the diffFuzzyThreshold
controlled input, and the handleSubmit/updateSettings message to ensure
serialization of diffFuzzyThreshold is correct.
In `@webview-ui/src/context/ExtensionStateContext.tsx`:
- Line 212: Replace the hardcoded diffFuzzyThreshold: 0.9 with the shared
constant DEFAULT_DIFF_FUZZY_THRESHOLD so the webview uses the canonical default;
update the object/initializer in ExtensionStateContext (where diffFuzzyThreshold
is set) to reference DEFAULT_DIFF_FUZZY_THRESHOLD instead of 0.9 ensuring
consistency with the extension-host fallback.
🪄 Autofix (Beta)
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: a666ced0-cab9-4afb-84fd-165bb4acb8bf
📒 Files selected for processing (9)
packages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tssrc/core/diff/strategies/__tests__/multi-search-replace.spec.tssrc/core/diff/strategies/multi-search-replace.tssrc/core/task/Task.tssrc/core/webview/ClineProvider.tswebview-ui/src/components/settings/ContextManagementSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/context/ExtensionStateContext.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/types/src/global-settings.ts`:
- Around line 114-119: The JSDoc for diffFuzzyThreshold is inconsistent with the
schema: update the comment above diffFuzzyThreshold to reflect the enforced
range in z.number().min(0.5).max(1).optional() (e.g., "Range: 0.5 (less fuzzy)
to 1.0 (exact match only)") and keep the `@default` 0.9 note; alternatively, if
the intended range is 0.0–1.0, change the schema on diffFuzzyThreshold to
z.number().min(0).max(1).optional()—but make the doc and the schema for
diffFuzzyThreshold agree.
🪄 Autofix (Beta)
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: 772812a7-b94a-4068-adc2-03593dd1e5ab
📒 Files selected for processing (1)
packages/types/src/global-settings.ts
nigeldelviero
changed the title
nigeldelviero
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
edelauna
reviewed
Jun 5, 2026
Contributor
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
🔥 - thank you for taking this on.
Had a couple comments related to my preference for how to implement this.
Also lets add in the i18n locales so that ci passes.
nigeldelviero
force-pushed
the
fix/issue-457-edit-unsuccessfull
branch
from
e5d0452 to
1af513d
Compare
…ff thresholds and diagnostics
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…a bounded window on math failure
…le Edits' section
…view to a bounded window on math failure" This reverts commit 4b2af0d.
nigeldelviero
force-pushed
the
fix/issue-457-edit-unsuccessfull
branch
from
1af513d to
73b0bb1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/types/src/global-settings.ts (1)
33-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the documented default for
diffFuzzyThreshold.Line 117 documents
@default 0.9, but Line 33 setsDEFAULT_DIFF_FUZZY_THRESHOLD = 1.0. This mismatch will mislead maintainers and tests that rely on docs for expected behavior.Proposed doc fix
/** * Fuzzy matching threshold for the multi-search-replace diff strategy. * Range: 0.5 (50% minimum similarity) to 1.0 (exact match only). - * `@default` 0.9 + * `@default` 1.0 */ diffFuzzyThreshold: z.number().min(0.5).max(1).optional(),Also applies to: 117-117
🤖 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 `@packages/types/src/global-settings.ts` at line 33, The documentation default for diffFuzzyThreshold is inconsistent with the code: the constant DEFAULT_DIFF_FUZZY_THRESHOLD is currently set to 1.0 while the JSDoc for diffFuzzyThreshold documents `@default` 0.9; update one to match the other. Decide the intended default (likely 0.9), then change the value of DEFAULT_DIFF_FUZZY_THRESHOLD or update the JSDoc `@default` in the diffFuzzyThreshold declaration so both the constant DEFAULT_DIFF_FUZZY_THRESHOLD and the documented `@default` 0.9 for diffFuzzyThreshold match, keeping naming (DEFAULT_DIFF_FUZZY_THRESHOLD, diffFuzzyThreshold) intact and run tests to confirm consistency.
🤖 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 `@src/core/diff/strategies/__tests__/multi-search-replace.spec.ts`:
- Around line 1422-1429: The test currently gates diagnostics assertions behind
a conditional on result.failParts which lets the test pass when diagnostics are
missing; update the assertions so you first assert that result.failParts exists
and has length > 0 (e.g.,
expect(result.failParts).toBeDefined()/toHaveLength(>0)), then extract const
failedPart = result.failParts[0] and unconditionally assert that failedPart has
an "error" property and that failedPart.error contains "No sufficiently similar
match found"; apply this same pattern to the second diagnostics block (the later
test that inspects result.failParts/failParts[0] around lines 1449-1462) so both
tests fail when diagnostics are absent.
---
Duplicate comments:
In `@packages/types/src/global-settings.ts`:
- Line 33: The documentation default for diffFuzzyThreshold is inconsistent with
the code: the constant DEFAULT_DIFF_FUZZY_THRESHOLD is currently set to 1.0
while the JSDoc for diffFuzzyThreshold documents `@default` 0.9; update one to
match the other. Decide the intended default (likely 0.9), then change the value
of DEFAULT_DIFF_FUZZY_THRESHOLD or update the JSDoc `@default` in the
diffFuzzyThreshold declaration so both the constant DEFAULT_DIFF_FUZZY_THRESHOLD
and the documented `@default` 0.9 for diffFuzzyThreshold match, keeping naming
(DEFAULT_DIFF_FUZZY_THRESHOLD, diffFuzzyThreshold) intact and run tests to
confirm consistency.
🪄 Autofix (Beta)
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: 80ae15f6-95ed-40f3-8528-65a3dbad983a
📒 Files selected for processing (12)
packages/types/src/global-settings.tspackages/types/src/vscode-extension-host.tssrc/core/diff/strategies/__tests__/multi-search-replace.spec.tssrc/core/diff/strategies/multi-search-replace.tssrc/core/task/Task.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.spec.tswebview-ui/src/components/settings/ContextManagementSettings.tsxwebview-ui/src/components/settings/SettingsView.tsxwebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsxwebview-ui/src/i18n/locales/en/settings.json
🚧 Files skipped from review as they are similar to previous changes (7)
- webview-ui/src/components/settings/SettingsView.tsx
- webview-ui/src/i18n/locales/en/settings.json
- src/core/task/Task.ts
- webview-ui/src/components/settings/ContextManagementSettings.tsx
- src/core/diff/strategies/multi-search-replace.ts
- src/core/webview/ClineProvider.ts
- packages/types/src/vscode-extension-host.ts
… diffFuzzyThreshold and fix failure-diagnostics assertions unconditional
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Issue: #452
When file edits are performed, the system previously enforced a strict similarity threshold of 1.0 (exact match) in
MultiSearchReplaceDiffStrategy. This caused frequent "Edit Unsuccessful" errors even for minor formatting, whitespace, or line-ending discrepancies.Additionally, when a match failed, the error feedback was generic (
No sufficiently similar match found), making it difficult for the AI or the user to diagnose why the edit was rejected.Root Cause
The core issue stems from strict matching logic in
MultiSearchReplaceDiffStrategywhich requires search block similarity to be exactly 1.0. This does not tolerate slight formatting changes introduced by pre-commit hooks, formatting extension rules, or AI-generated output variations.Solution
diffFuzzyThreshold(number, default: 0.90) to allow a relaxed matching similarity.ContextManagementSettings.tsxUI under the write-delay settings, with range 50% - 100%.diffFuzzyThresholdsetting fromExtensionStateto theTaskandMultiSearchReplaceDiffStrategyinstance.applyDiffto report detailed Levenshtein-based diagnostics (Levenshtein distance, character lengths, search range, and similarity scores) when matches fall below the threshold.ExtensionStatedefinition and initialized the default value (0.90) in bothglobalSettingsSchemaandExtensionStateContextProviderto prevent TypeScript compilation errors.multi-search-replace.spec.tsto assert default thresholds, success with minor whitespace variations, and the structure of detailed debug outputs when edits fail.Verification
All
multi-search-replacetests pass successfully:Screenshot
Summary by CodeRabbit
New Features
Tests