-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
remove Cody "upsells"—and all Cody links if Cody is disabled #63430
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e376594
to
8d5e64e
Compare
616db65
to
cd03c32
Compare
de60df0
to
494543b
Compare
75e44f2
to
5774fc0
Compare
8290bf0
to
e263029
Compare
639a1b6
to
76406c3
Compare
38822ce
to
7e2c6e6
Compare
3266452
to
a4e7dd7
Compare
camdencheek
approved these changes
Jun 26, 2024
a93139d
to
9af69c0
Compare
At a high level, we don't want to show annoying ads/upsells for Cody that are not useful. And if Cody is disabled, we don't want to show *any* links to Cody. - Dotcom - Navbar - Unauthed: "Cody" single link to /cody (marketing page) - Authed: "Cody" dropdown with "Dashboard" (/cody/manage) and "Chat" (/cody/chat) - Routes - /cody: always the marketing page - /cody/manage: requires sign-in, shows Cody PLG subscription status for the user (Free plan is auto-opted-into by default) - /cody/chat: requires sign-in - Enterprise with Cody enabled on instance - Navbar - Cody NOT enabled for current user: "Cody" single link to /cody/dashboard - Cody enabled for current user: "Cody" dropdown with "Dashboard" (/cody/manage) and "Chat" (/cody/chat) - Routes - /cody: this link should not be present anywhere, but redirect to /cody/dashboard - /cody/manage: informational page, with editor/web links for Cody-enabled users and a "contact admin to get access" message for Cody-disabled users - /cody/chat: chat for Cody-enabled users, redirect to /cody/manage for Cody-disabled users - Enterprise with Cody NOT enabled on instance (`"cody.enabled": false` in site config) - Navbar: no Cody link or dropdown - Routes: all Cody routes 404 This is an example of what we will KEEP for users on instances with Cody enabled but who do not themselves yet have access to Cody. This is useful because it informs users how to get access to Cody, and presumably their site admin wants people to request it who want to use it. ![image](https://github.com/sourcegraph/sourcegraph/assets/1976/c2adb086-44ec-4240-ad44-95981763fb72) Fixes https://linear.app/sourcegraph/issue/SRCH-529/hide-cody-ai-tab-and-cody-upsell-if-cody-is-not-enabled This ended up being a much bigger change than I expected because I found error-prone code that needed cleaning up: - Improve how we determine if Cody is enabled in the frontend code. Previously, we checked the license features in some places, `cody.enabled` site config in others, and the user's current RBAC permissions for Cody in yet others. The most error-prone was checking the license features, since a license may entitle the instance to Cody but the site admin may still choose to disable it. There were no places in the frontend code where checking the license's entitlements was actually correct, so I changed everything to checking either `window.context.codyEnabledOnInstance` or `window.context.codyEnabledForCurrentUser`. - Did the same for `window.context.codeSearchEnabledOnInstance` for symmetry. - Removed "helper" functions that just checked 1 or 2 boolean values on `window.context` related to this, in favor of accessing `window.context` directly. Globals aren't great, and we should use React context or something similar, but now that the JSContext has the right fields (i.e., enabled instead of licensed), it's simpler and there is no need for helper functions. - Removed prop drilling of the `licenseFeatures` that was unnecessary since these values are available in globals and were being set from globals at some arbitrary point in the React component hierarchy anyway. - Updated the GlobalNavbar test snapshots. Run in 3 modes: (1) dotcom mode, (2) `"cody.enabled": false` in site config, (3) normal `sg start`. - When Cody is disabled in site config (with `"cody.enabled": false`), all links and UI elements about Cody are hidden from all users. Previously, when Cody was disabled, users would see some links informing them about Cody.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At a high level, we don't want to show annoying ads/upsells for Cody that are not useful. And if Cody is disabled, we don't want to show any links to Cody.
Detailed desired behavior
"cody.enabled": false
in site config)This is an example of what we will KEEP for users on instances with Cody enabled but who do not themselves yet have access to Cody. This is useful because it informs users how to get access to Cody, and presumably their site admin wants people to request it who want to use it.
Fixes https://linear.app/sourcegraph/issue/SRCH-529/hide-cody-ai-tab-and-cody-upsell-if-cody-is-not-enabled
Unexpected code changes needed
This ended up being a much bigger change than I expected because I found error-prone code that needed cleaning up:
cody.enabled
site config in others, and the user's current RBAC permissions for Cody in yet others. The most error-prone was checking the license features, since a license may entitle the instance to Cody but the site admin may still choose to disable it. There were no places in the frontend code where checking the license's entitlements was actually correct, so I changed everything to checking eitherwindow.context.codyEnabledOnInstance
orwindow.context.codyEnabledForCurrentUser
.window.context.codeSearchEnabledOnInstance
for symmetry.window.context
related to this, in favor of accessingwindow.context
directly. Globals aren't great, and we should use React context or something similar, but now that the JSContext has the right fields (i.e., enabled instead of licensed), it's simpler and there is no need for helper functions.licenseFeatures
that was unnecessary since these values are available in globals and were being set from globals at some arbitrary point in the React component hierarchy anyway.Test plan
Run in 3 modes: (1) dotcom mode, (2)
"cody.enabled": false
in site config, (3) normalsg start
.Changelog
"cody.enabled": false
), all links and UI elements about Cody are hidden from all users. Previously, when Cody was disabled, users would see some links informing them about Cody.