Fix input box revert issue and configuration loss during profile switch #955

[System233]

Fixes #955

Description

This PR made the following changes:

1. Added a saveApiConfiguration operation to provide a configuration save function without forced UI updates.

2. handleInputChange uses saveApiConfiguration to save to avoid overwriting user input when quickly switching input boxes

3. Merge a large number of async calls in updateApiConfiguration to reduce the time consumption of upsertApiConfiguration events (reduce the impact of the disorder of asynchronous tasks. Before RPC and transactions are implemented, the apiConfiguration merging phenomenon that occurs when switching profiles at high speed will not be completely solved)

4. Improved model loading requests. Users can get model list feedback in time when entering URLs, avoiding redundant interval updates

2025-02-13.20-48-53.mp4

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?

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

Related Issues

Reviewers

---

Important

Fixes input box revert and configuration loss during profile switch, improves performance and loading logic in ClineProvider.ts, ApiOptions.tsx, and ExtensionStateContext.tsx.

• Behavior:
  • Fix input box reverting issue by changing onBlur to onInput in ApiOptions.tsx.
  • Resolve configuration loss during profile switch by ensuring updateGlobalState and storeSecret calls are wrapped in Promise.all in ClineProvider.ts.
  • Improve profile switching performance by optimizing asynchronous operations with Promise.all in ClineProvider.ts.
  • Enhance local model loading logic by replacing useInterval with setTimeout in ApiOptions.tsx.
• Error Handling:
  • Update error messages for API configuration save and rename operations in ClineProvider.ts.
• Misc:
  • Remove softUpdate logic from handleInputChange in ExtensionStateContext.tsx to ensure immediate synchronization.

^{This description was created by for 5c10e96b68f1660532706f8a1857073f0d8e6508. It will automatically update as commits are pushed.}

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7316f82

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

[System233]

Why? Is this unit test correct?

[System233]

Why? Is this unit test correct?

Fixed.
Removed the test "handles buildApiHandler error in updateApiConfiguration" because upsertApiConfiguration will no longer actively update the UI to prevent overwriting user input.

[mrubens]

Awesome - this looks way cleaner! Do you mind explaining which issues were fixed for you from each part of these changes? I confess that all of this code has gotten very complicated and I don't understand all of it.

[bramburn]

nice mate!

[System233]

Awesome - this looks way cleaner! Do you mind explaining which issues were fixed for you from each part of these changes? I confess that all of this code has gotten very complicated and I don't understand all of it.

The content of the revision is basically the same as the summary generated by AI, there is one error that needs to be corrected, I will set the PR as a draft first.

Fixed

[System233]

Awesome - this looks way cleaner! Do you mind explaining which issues were fixed for you from each part of these changes? I confess that all of this code has gotten very complicated and I don't understand all of it.

code updated, see Description for details of the changes

[System233]

nice mate!

Thanks! 😄

[System233]

After testing, the issue with editing fields in the settings panel has been resolved, but the same issue still occurs during the API response process. This may be due to the rapid updates to ExtensionStateContext during the conversation, which causes the UI to continuously re-render with old values and prevents input from being accepted.

Fixing this issue would require a large-scale change to separate the state of different components. It is recommended to mark the settings as read-only during the API response process to bypass this problem.

[samhvw8]

@mrubens @System233 i have an idea that remove upsertApiConfiguration and replace it with saveApiConfiguration,
we will have a little bit more change, on handleSubmit we will add 1 more loadApiConfiguration to that section, so we will decouple save & load api

[samhvw8]

@System233 @mrubens like this System233#1 ?

[bramburn]

So my fix might still be worth working on... I started working on a local storage save for the webview UI and adding a queue system for it.

…

On Fri, 14 Feb 2025, 10:45 System233, ***@***.***> wrote: After testing, the issue with editing fields in the settings panel has been resolved, but the same issue still persists during the API response process. This might be due to the rapid updates of ExtensionStateContext during the conversation. Fixing this issue would require widespread changes, so it is recommended to mark the settings as read-only during the API response process. — Reply to this email directly, view it on GitHub <#979 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUTT3L3OSEPQDG2ILU52RD2PXCN3AVCNFSM6AAAAABXCDUWNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYHE2TOOJWGA> . You are receiving this because you commented.Message ID: ***@***.***> [image: System233]*System233* left a comment (RooCodeInc/Roo-Code#979) <#979 (comment)> After testing, the issue with editing fields in the settings panel has been resolved, but the same issue still persists during the API response process. This might be due to the rapid updates of ExtensionStateContext during the conversation. Fixing this issue would require widespread changes, so it is recommended to mark the settings as read-only during the API response process. — Reply to this email directly, view it on GitHub <#979 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUTT3L3OSEPQDG2ILU52RD2PXCN3AVCNFSM6AAAAABXCDUWNKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNJYHE2TOOJWGA> . You are receiving this because you commented.Message ID: ***@***.***>

[System233]

So my fix might still be worth working on... I started working on a local storage save for the webview UI and adding a queue system for it.

To achieve this, the simplest solution is to cut off the connection of the settings panel to the ExtensionStateContext.
The perfect solution would be to eliminate all postStateToWebview calls and replace it with RPC/ACK.

@mrubens Could you please continue to review this PR? Thanks.

[mrubens]

So my fix might still be worth working on... I started working on a local storage save for the webview UI and adding a queue system for it.

To achieve this, the simplest solution is to cut off the connection of the settings panel to the ExtensionStateContext. The perfect solution would be to eliminate all postStateToWebview calls and replace it with RPC/ACK.

@mrubens Could you please continue to review this PR? Thanks.

Sorry for the delay - I’m on vacation this weekend and don’t have much time at the laptop. I can try tonight though. @samhvw8 wrote a lot of the original code so I’m curious for his thoughts as well. Thank you!

[System233]

Since the issue was raised, I’ve been unable to use the settings panel properly for several consecutive days. The configuration items I enter through the keyboard are often automatically deleted, and switching profiles can corrupt the correct profile, displaying a hybrid profile made up of fields from multiple profiles. This leads to issues like #1017 and #990 (which I’ve encountered firsthand), so I’m eager to fix this issue as soon as possible.

@samhvw8, your suggestion is exactly what I initially thought. In the first version of this PR(5c10e96), the functionality of upsertApiConfiguration was identical to saveApiConfiguration, and I intentionally renamed upsertApiConfiguration to saveApiConfiguration. Later, to fix the issue where the new profile wasn’t automatically switched when added, I planned to handle both saving and loading operations in onSelectConfig. However, I realized that there’s no guaranteed synchronization between loadApiConfiguration and saveApiConfiguration. Due to potential disk I/O or communication delays etc., saveApiConfiguration might execute after loadApiConfiguration, causing a profile switch to fail or resulting in a mix of old and new profiles (though, since both profiles are essentially the same, the mix still leads to the same result). Therefore, I decided to keep the original upsertApiConfiguration (which already encapsulates saving and UI refresh functionality in a synchronous operation) and introduce a new saveApiConfiguration to address this issue.

Currently, many patches related to onBlur/onInput in the project are a result of synchronization issues. I'm not sure how much you all know about data races, but synchronization is not exclusive to native languages like C/C++. Asynchronous tasks also need synchronization. Without proper synchronization mechanisms, operations must be atomic. If neither synchronization nor atomicity is ensured, there’s a risk of data corruption. Unfortunately, many parts of the Roo project haven’t addressed this, and I don't want to introduce RPC sync into the current PR, it should be a separate patch.

Finally, to resolve the issue of the settings panel rendering with old values when the state is updated, I had to revert the changes made in #889 and restore onBlur back to the original onInput.

[mrubens]

Thank you for driving this forward, and sorry I haven’t been that available while on vacation! I’m going to review this as soon as I can.

[System233]

It’s okay. At the moment, I’ve found an bug with the Dropdown component in vscrui, and I’ve already create a PR estruyf/vscrui#6. Before the patch is merged into the component library, the event handler for the Dropdown won’t update along with the component, which could potentially cause incorrect data. For now, I’m replacing it with the built-in VSCodeDropdown component to fix the issue.

[mrubens]

I just poked around a bit and it seems to work well! Nice that you were able to bring back the onInput without the typing in the boxes being laggy.

As for the vscrui - one option would be for us to use a Roo fork of it? I don't know how actively maintained it is. Let me know if you want me to create a fork within Roo, or if it's not too much trouble going back to the VS Code dropdown that's fine with me too.

[mrubens]

Maybe we should cut this?

[mrubens]

Great catch here

[mrubens]

Reminder to remove this before merging

[mrubens]

I'm game to try to release this tonight or tomorrow if you get it to a place where you're happy with it. Thanks again so much for digging into this 🙌

[samhvw8]

this cool

[System233]

One bad news is that VSCodeDropdown still has its issue, which was commented on by the upstream cline:

/*
VSCodeDropdown has an open bug where dynamically rendered options don't auto select the provided value prop. You can see this for yourself by comparing it with normal select/option elements, which work as expected.
https://github.com/microsoft/vscode-webview-ui-toolkit/issues/433

In our case, when the user switches between providers, we recalculate the selectedModelId depending on the provider, the default model for that provider, and a modelId that the user may have selected. Unfortunately, the VSCodeDropdown component wouldn't select this calculated value, and would default to the first "Select a model..." option instead, which makes it seem like the model was cleared out when it wasn't.

As a workaround, we create separate instances of the dropdown for each provider, and then conditionally render the one that matches the current provider.
*/

However, there’s good news too—vscrui has merged the patch, and we can now use the fixed version.

[mrubens]

🚀

[mrubens]

Awesome, feel free to merge whenever you feel good about it!

[hannesrudolph]

@System233 can you please shoot me a message on discord? hrudolph is my user id.

[System233]

Awesome, feel free to merge whenever you feel good about it!

Thanks. I’m working on resolving the issue where unsaved configurations in the settings panel are lost once the API response is completed, which behaves the same as the upstream cline behavior. Apart from that, no other issues have been found.

@System233 can you please shoot me a message on discord? hrudolph is my user id.

Sorry about that, I don't have a Discord account.

[System233]

Since postStateToWebview/apiConfiguration is used extensively, modifying the code would have a significant impact, and there isn’t an immediate good solution to this problem. Considering that the impact of this issue is relatively small, we can merge the PR for now and address this issue separately in the future.

Awesome, feel free to merge whenever you feel good about it!

@mrubens
I don’t have merge permissions, so the maintainers can merge the PR anytime after you’ve reviewed it. Thank you. 👌

[mrubens]

Someone in discord noticed that there are still issues with renaming profiles (check out around 2:00 into this video). Any ideas what could be going on there? Thank you!

Code_9DwSQCj3cN.mp4

[System233]

Someone in discord noticed that there are still issues with renaming profiles (check out around 2:00 into this video). Any ideas what could be going on there? Thank you!

Code_9DwSQCj3cN.mp4

I’ve checked the issue, and it’s caused by the settings not being saved. Currently, the configuration is only saved after clicking the Done button or switching profiles, but renaming doesn’t trigger the save functionality.

There’s still a synchronization issue here. I can solve it by encapsulating the save and load operations into the rename event handler.
Alternatively, I’d like to implement something similar to the versioning mechanism in SQL, where only the latest state is saved. This way, configurations can be saved immediately in the input event, while also addressing potential event ordering issues.

[System233]

patch almost ready, create a PR to fix the issue later

[System233]

I’m working on resolving the issue where unsaved configurations in the settings panel are lost once the API response is completed, which behaves the same as the upstream behavior cline

Someone in discord noticed that there are still issues with renaming profiles (check out around 2:00 into this video). Any ideas what could be going on there? Thank you!

Code_9DwSQCj3cN.mp4

All of the above fixed in #1051