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

ConfigHandler: make updateIdeSettings work (better) #3456

Closed
wants to merge 1 commit into from

Conversation

owtaylor
Copy link

Description

ConfigHandler.updateIdeSettings() was largely ineffective because the ideSettingsPromise was stored in the ProfileLoader object, so updating to a new ideSettingsPromise resulted in the config being reloaded with the old settings. Fix this by passing the ideSettingsPromise as a parameter to IProfileLoader.doLoadConfig().

I came up with this while working on a larger change (for automatic model selection), so I don't have a great example of when this would matter, but it does seem like this would affect the use of ideSettings.userToken in core/llm/llms/index.ts -

  if (desc.provider === "continue-proxy") {
    options.apiKey = ideSettings.userToken;
    if (ideSettings.remoteConfigServerUrl) {
      options.apiBase = new URL(
        "/proxy/v1",
        ideSettings.remoteConfigServerUrl,
      ).toString();
    }
  }

This patch does not fix the case where changing the settings affects what profiles (changes to ideSettings.remoteConfigServerUrl)

Checklist

  • The relevant docs, if any, have been updated or created
  • The relevant tests, if any, have been updated or created

Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for continuedev ready!

Name Link
🔨 Latest commit 6636386
🔍 Latest deploy log https://app.netlify.com/sites/continuedev/deploys/677c2a105db8080008b5d256
😎 Deploy Preview https://deploy-preview-3456--continuedev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ConfigHandler.updateIdeSettings() was largely ineffective because the
ideSettingsPromise was stored in the ProfileLoader object, so updating
to a new ideSettingsPromise resulted in the config being reloaded
with the old settings. Fix this by passing the ideSettingsPromise as
a parameter to IProfileLoader.doLoadConfig().
@owtaylor owtaylor force-pushed the update-ide-settings branch from 9767cee to 6636386 Compare January 6, 2025 19:07
@owtaylor
Copy link
Author

owtaylor commented Jan 6, 2025

Rebased to fix conflicts

@sestinj
Copy link
Contributor

sestinj commented Jan 10, 2025

@owtaylor I appreciate this fix! I agree that it cleans things up, and so it's kind of awkward to say this (especially 3 weeks after your PR was opened), but given that we're going to be removing the need for these remoteConfigServer features, I'm going to close this in favor of the larger change we'll make soon. Before then it feels wise to avoid any kind of shake up in such a core part of the codebase. Again, thank you for taking the time to look into this!

@sestinj sestinj closed this Jan 10, 2025
@owtaylor
Copy link
Author

@owtaylor I appreciate this fix! I agree that it cleans things up, and so it's kind of awkward to say this (especially 3 weeks after your PR was opened), but given that we're going to be removing the need for these remoteConfigServer features, I'm going to close this in favor of the larger change we'll make soon. Before then it feels wise to avoid any kind of shake up in such a core part of the codebase. Again, thank you for taking the time to look into this!

I've certainly been there as a maintainer with a contributed patch a lot of times! Thanks for letting me know the situation.

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