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

pass config to webview through new Observable-based webview API #5751

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sqs
Copy link
Member

@sqs sqs commented Sep 30, 2024

The webview protocol message config had 2 problems:

  1. It was sent (in ChatController.sendConfig) whenever ChatController thought it might have changed, which meant a bunch of unnecessary calls to sendConfig sprinkled throughout ChatController.
  2. It is a different schema from the other configuration, with configFeatures and other things having custom logic to construct them in ChatController.

This solves problem 1. Now, the config is sent through the legacyConfig(): LegacyWebviewConfig observable webview API method to the webview, which means it gets an up-to-date view of the config. It uses the same type, so this does not yet address problem 2, but it makes it much easier to address it.

Removes the old config webview protocol message.

There is no user-facing behavior change.

Test plan

CI and e2e

@sqs sqs requested review from olafurpg and a team September 30, 2024 10:14
@sqs sqs changed the base branch from main to sqs/sign-in-form September 30, 2024 10:14
Base automatically changed from sqs/sign-in-form to main September 30, 2024 10:42
@sqs sqs force-pushed the sqs/webview-config branch 2 times, most recently from 5857cd1 to 6f6b91b Compare September 30, 2024 23:57
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Ad-hoc testing by @pkukielka indicates this regresses login redirection for free/pro users in JetBrains, and exacerbates a problem where theme styles sometimes do not apply.

Frankly, the rate of these changes is too high given the latency of the JetBrains QA and release cycle. We need authors to do more manual testing on JetBrains and contribute JetBrains tests if we want to keep producing releases.

@sqs sqs marked this pull request as draft October 2, 2024 09:58
@sqs sqs force-pushed the sqs/webview-config branch 3 times, most recently from ee38167 to be8ca39 Compare October 3, 2024 06:09
Copy link

github-actions bot commented Oct 3, 2024

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces the usage of an unsafe_ function or abuses PromptString.

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