Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the user to switch between and configure the two implemented edit strategies #280

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

daniel-lxs
Copy link

@daniel-lxs daniel-lxs commented Jan 5, 2025

Description

Allow the user to switch between the two implemented edit strategies, unified diff and search/replace and also allowing the user to change the fuzziness of the selected strategy.
Improved the prompt used by the unified diff strategy to encourage the model to produce better diffs, the improved prompt is based on the previous prompt but restructuring it to use positive instructions, these changes are inspired on Aider's unified diff prompts.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I have used Roo Cline with both strategies selected on real-life projects while it still makes mistakes it allows me to switch to another strategy if the model gets stuck with the previous one. I recommend further testing specifically in the accuracy of the new unified diff prompt.

Checklist:

  • My code follows the patterns of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Additional context

Unified Diff:
Screenshot_20250105_174003
Search/Replace:
search_replace
Editing using diffs disabled:
Screenshot_20250105_174951

Related Issues

Reviewers


Important

This PR adds functionality to switch between 'unified' and 'search-replace' edit strategies with adjustable fuzziness, updating both backend logic and UI components.

  • Behavior:
    • Adds ability to switch between 'unified' and 'search-replace' edit strategies in Cline.ts.
    • Allows adjusting fuzziness for each strategy in UnifiedDiffStrategy and SearchReplaceDiffStrategy.
  • UI Changes:
    • Updates SettingsView.tsx to include dropdown for selecting diff strategy and sliders for fuzziness.
    • Sends selected strategy and fuzziness to backend via WebviewMessage.
  • Backend Changes:
    • Modifies getDiffStrategy() in DiffStrategy.ts to accept strategy type and fuzziness.
    • Updates ClineProvider.ts to handle new settings and persist them in global state.

This description was created by Ellipsis for cad274d. It will automatically update as commits are pushed.

- Added support for selecting between 'unified' and 'search-replace' diff strategies in the Cline class and related components.
- Updated the getDiffStrategy function to accept a strategy parameter.
- Enhanced the ClineProvider to manage and persist the selected diff strategy in the global state.
- Modified the SettingsView to allow users to choose the diff strategy and adjust related settings dynamically.
- Introduced fuzz factor control for the unified diff strategy to fine-tune matching precision.
…uctions

- Introduced a fuzz factor parameter in the UnifiedDiffStrategy constructor to improve patch application precision.
- Updated the tool description to clarify the usage of unified diff format and added detailed guidelines for applying diffs.
- Removed outdated common pitfalls and best practices sections, replacing them with more relevant instructions for handling code changes and formatting requirements.
Copy link

changeset-bot bot commented Jan 5, 2025

⚠️ No Changeset found

Latest commit: 8fc26fa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@daniel-lxs daniel-lxs marked this pull request as ready for review January 5, 2025 22:52
@daniel-lxs daniel-lxs changed the title Edit strategies Allow the user to switch between and configure the two implemented edit strategies Jan 5, 2025
src/core/Cline.ts Outdated Show resolved Hide resolved
@daniel-lxs daniel-lxs closed this Jan 5, 2025
@daniel-lxs
Copy link
Author

I need to solve an issue before this can be merged, I will open a new PR

…g tools in an existing task.

-Since the tool description is sent when the task is created, the diff strategy can't be changed on existing tasks
- Refactored code to ensure diff strategy is correctly restored from task history.
@daniel-lxs daniel-lxs reopened this Jan 6, 2025
@daniel-lxs
Copy link
Author

daniel-lxs commented Jan 6, 2025

Due to the current way we handle tasks, apply_edit can't be changed on existing tasks, this will cause the model to use apply_edit incorrectly.
We use the task history to remember the diff strategy used and reinitialize it.

@mrubens
Copy link
Collaborator

mrubens commented Jan 6, 2025

Due to the current way we handle tasks, apply_edit can't be changed on existing tasks, this will cause the model to use apply_edit incorrectly. We use the task history to remember the diff strategy used and reinitialize it.

Oh that's too bad. How badly does it get stuck? I wonder if we could recover faster by including the instructions for the diff strategy in the rejectTool message.

@daniel-lxs
Copy link
Author

Oh that's too bad. How badly does it get stuck? I wonder if we could recover faster by including the instructions for the diff strategy in the rejectTool message.

With these changes we can only use one strategy per task, meaning you can't change the strategy for existing tasks/chats.

However the PR is complete, it implements the dropdown to select the strategy and also the slider to adjust the fuzziness of the strategy individually.

For implementing strategy switching we could:

  • Lazily describe the tools, meaning the model could request the description of the tool, if one strategy fails then it can request the other one and use it, this however would probably increase the amount of errors.
  • On the reject message we could tell the model to try another strategy.
  • We give the model two different tools with each strategy.
  • Include both descriptions and allow the model to choose which strategy to use with an extra parameter.

You also mentioned reminding the model of how to implement the changes with the reject message, I considered this idea but discarded it since it can potentially use a lot of tokens if the error happens repeatedly.

@mrubens
Copy link
Collaborator

mrubens commented Jan 17, 2025

Heads up that I ran prettier on the whole codebase in #404, and it will run automatically in a pre-commit hook going forward.

In order to bring this PR up to date on formatting you'll need to run npx prettier './**/*.{js,jsx,ts,tsx,json,css,md}' --write. Sorry for any hassle with this, and let me know if you get stuck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants