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

Improve plus user onboarding #1160

Merged
merged 5 commits into from
Feb 4, 2025

Conversation

zeroliu
Copy link
Collaborator

@zeroliu zeroliu commented Feb 4, 2025

No description provided.

@@ -265,6 +265,7 @@ export default class ChatModelManager {

async setChatModel(model: CustomModel): Promise<void> {
const modelKey = getModelKeyFromModel(model);
setModelKey(modelKey);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@logancyang Attention! This change is critical. Otherwise, there is a race condition that can lead to infinite loop. This setter must be called before the async getModelConfig call.

@@ -63,6 +64,7 @@ export default class CopilotPlugin extends Plugin {
// Initialize BrevilabsClient
this.brevilabsClient = BrevilabsClient.getInstance();
this.brevilabsClient.setPluginVersion(this.manifest.version);
await checkIsPlusUser();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function checks plus status on load

@zeroliu zeroliu force-pushed the zero/plus-mode-onboarding branch from 9282763 to b2942ff Compare February 4, 2025 00:49
@logancyang
Copy link
Owner

logancyang commented Feb 4, 2025

Awesome UX improvements! It could translate directly to more plus users!! 🔥🔥🔥

Several observations

  1. When the license key is invalid, after hitting "Check" this modal says it has expired. Could be better to say "either invalid or has expired" to be more precise.
SCR-20250203-plxi
  1. After hitting "switch", the embedding model is switched to "copilot-plus-small". Normally, for a user who has an existing index, switching embedding model leads to a modal that triggers a complete reindex. But this "switch" doesn't. WDYT is a good solution here? Simpler solutions is better.
  2. There's a merge conflict.

Otherwise LGTM! Look forward to the user feedback after it goes live!

@zeroliu zeroliu force-pushed the zero/plus-mode-onboarding branch 2 times, most recently from df614a1 to a4cd109 Compare February 4, 2025 02:20
@zeroliu zeroliu force-pushed the zero/plus-mode-onboarding branch from a4cd109 to e61d958 Compare February 4, 2025 02:25
@zeroliu
Copy link
Collaborator Author

zeroliu commented Feb 4, 2025

After hitting "switch", the embedding model is switched to "copilot-plus-small". Normally, for a user who has an existing index, switching embedding model leads to a modal that triggers a complete reindex. But this "switch" doesn't. WDYT is a good solution here? Simpler solutions is better.

This is a great insight! It makes me wonder whether auto switch embedding model is a good idea. I think two changes are needed:

  1. In the welcome modal, we should show the model to switch and highlight that changing embedding model will lead to a reindex.
  2. When switching copilot plus status to false, we should not touch the embedding model to avoid accidentally reindexing the vault. If the user tries to index with the premium models, it will throw an error in the notice. We can provide a better error message in the future.

@zeroliu
Copy link
Collaborator Author

zeroliu commented Feb 4, 2025

Updated with my proposal:

  1. in the plus welcome dialog, I added the models to switch and added a warning message if the current embedding model is not copilot-plus-small. Once the user clicks switch, we will reindex the embedding if the embedding model key changes.
Screenshot 2025-02-03 at 9 53 48 PM
  1. in the plus expired dialog, I stopped switching chat and embedding models automatically. I added a warning message instead. It also only shows if the user is currently selecting a copilot plus chat model or embedding model
Screenshot 2025-02-03 at 9 54 16 PM

@logancyang
Copy link
Owner

logancyang commented Feb 4, 2025

@zeroliu tried again and saw an issue: when a valid license key becomes invalid, the chat model still get updated to gpt-4o even though the default model value was not switched.

And if I click reset to default settings, the license key still exists. Is that intentional?

src/plusUtils.ts Outdated
updatePlusUserSettings(true);
// Do not show the welcome modal if the user is already a plus user before
// 2024/02/04 (isPlusUser === undefined)
if (isPlusUser === false) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be isPlusUser !== true? Right now this welcome modal is not showing up for new users (undefined).

@zeroliu
Copy link
Collaborator Author

zeroliu commented Feb 4, 2025

@zeroliu tried again and saw an issue: when a valid license key becomes invalid, the chat model still get updated to gpt-4o even though the default model value was not switched.

This is fixed now.

And if I click reset to default settings, the license key still exists. Is that intentional?

This is related to my last debounce and uncontrolled settings change. It's also fixed now.

Should this be isPlusUser !== true? Right now this welcome modal is not showing up for new users (undefined).

isPlusUser set to undefined was intentional to hide the welcome dialog for the existing paid users. I overlooked the impact on users who clicks resetSettings. For new users, this should be fine because the onload event will set the value to false first. I changed the default value to false now and it will show to existing paid users once they upgrade. I think about it again and think it's an acceptable experience.

@zeroliu
Copy link
Collaborator Author

zeroliu commented Feb 4, 2025

@logancyang added another round of iteration with the following changes:

  1. The welcome dialog will only show up after the user clicks "apply" in the settings. It won't affect existing users.
  2. When is plus user check passes on load, it won't auto set the default chain type any more. This may disrupt existing users workflow. In the latest version, the onload check will only set the isPlusUser state and show a the key invalid dialog if the user goes from active to inactive. The current valid plus user will not be affected by this change.

@logancyang logancyang merged commit a5300eb into logancyang:master Feb 4, 2025
2 checks passed
@logancyang logancyang mentioned this pull request Feb 5, 2025
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