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

Reset feature flags when switching through logins #2182

Merged
merged 8 commits into from
Dec 8, 2023

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Dec 7, 2023

Fixes #2181
Fixes #2154

This PR attempts to clean up the auth flow and makes it so that when the user account is switched, we never accidentely use feature flags of a wrong server URL and also ensures we reconfigure Autocomplete so it works.

It does this by removing the relationship between the AuthProvider and a specific webview. This is necessary because:

  • With features like Autocomplete, we have things that depend on the AuthProvider unrelated on wether a web view is enabled or not
  • We have more than one web view right now and this doesn't make sense

Instead, we make it so that the web views subscribe to the auth state like every other service.

In untangling this, the biggest issue was undocumented dependencies into how events were passed. What I noticed was that the ContextProvider plays a major role in initializing the web views. This is something we should clean up in a follow up PR (IMO the web views should subscribe to the ContextProvider and not the other way around).

I also noticed that the cody.auth.sync action isn't really necessary as we can subscribe to the AuthProvider via addChangeListener anyways (which most of the services did).

Also: Apparently there were two ways on when we reconfigure the Configuration object. I now made sure both ways at least reconfigure the same components by extracting a onConfigurationChange function inside main.ts.

Two unrelated fixes:

  • The code to update the upgrade state when the editor was focused would also run on enterprise instances causing a bunch of errors in the console. Fixed.
  • Added the remote URL to the completion loggers we can easily see where the request is sent to.

⚠️ This PR has a potential to break areas depending on auth, so please take some time to test it. I've been testing it as thorough as I could though!

Test plan

The canonical test case was:

  • Be signed in to dotcom and have the starcoder-hybrid feature flag enable
  • Now switch account to an enterprise instance
  • Ensure that requests are made to the enterprise instance and that we do not use starcoder model strings
output.mp4

@philipp-spiess philipp-spiess changed the title Remove webview dependency from auth provider Reset feature flags when switching through providers Dec 7, 2023
@philipp-spiess philipp-spiess marked this pull request as ready for review December 7, 2023 16:06
@philipp-spiess philipp-spiess self-assigned this Dec 8, 2023
@philipp-spiess philipp-spiess changed the title Reset feature flags when switching through providers Reset feature flags when switching through logins Dec 8, 2023
@philipp-spiess
Copy link
Contributor Author

@sourcegraph/cody-vscode Friendly nudge for a PR review 🤗

Copy link
Contributor

@DanTup DanTup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem reasonable to me, though I'm not very familiar with all of this (nor do I have an enterprise account to test), so you might want additional opinions.

I did add a question about some code that was recently removed that this puts back though.

Comment on lines 62 to 64
interface CurrentUserIdAndVerifiedEmailQuery {
currentUser: { id: string; hasVerifiedEmail: boolean } | null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like code that was recently removed in 5af5735 - what's the reason for adding it back?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started using the query again in https://github.com/sourcegraph/cody/pull/2182/files#diff-1456bee1a58eb80a929388788427f609653aef419f9c9b84e53c1d904446c516R324-R334 to fix the issue that was querying enterprise instances about the cody pro plan.

I think we can fix this differently though and not do any query on window focus unless connected to dotcom (so we avoid any query).

@philipp-spiess
Copy link
Contributor Author

I tested it some more between dotcom and enterprise instances and I think the change is good. Since I really think this is something that we should land sooner rather than later (so we can start dogfooding immediately) I'll go ahead and merge this now and cut an insider build right away. We can always revert!

@philipp-spiess philipp-spiess merged commit b46bb30 into main Dec 8, 2023
14 checks passed
@philipp-spiess philipp-spiess deleted the ps/auth-adventure branch December 8, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants