PLG: extract Cody Pro routes to a separate file#63003
Conversation
| /> | ||
| )} | ||
| condition={({ isSourcegraphDotCom, licenseFeatures }) => | ||
| isSourcegraphDotCom && licenseFeatures.isCodyEnabled |
There was a problem hiding this comment.
However, Cody Pro routes are available only on dotcom, and Cody should always be enabled there. I’m leaving the license check in place “just in case” since I couldn’t find a hardcoded true for the Cody feature when in dotcom mode in our backend code.
chrsmith
left a comment
There was a problem hiding this comment.
All of this looks right, and does take a step towards a better overall architecture. (🙌 👏)
But I think we need to double check that users can still use /cody/manage (like they do today), and serve a 404 error on sourcegraph.com for our newer pages (at least until we set certain site config or user flags.)
Also, if possible, I think it would be cleaner to just have any "Cody Pro" pages serve a 404 if you aren't running on dotcom mode. Since "error: this page is only available on sourcegraph.com" seems less pleasant than just a generic 404. (Since the end result is the same: that page doesn't exist [for Enterprise users].)
| /** | ||
| * Renders the appropriate Cody Pro page component for the given route path. | ||
| */ |
There was a problem hiding this comment.
I don't think I've ever seen a /** */ style comment in the code base yet. So would you mind doing a quick sanity check too see how common this is? If we already have enough usages that either would be considered common, then we can leave this as-is.
But if most doc comments like this are using single-line (//) comments, we should try to be consistent with the larger codebase.
There was a problem hiding this comment.
I don't think I've ever seen a /** */ style comment in the code base yet. So would you mind doing a quick sanity check too see how common this is? If we already have enough usages that either would be considered common, then we can leave this as-is.
4640 uses in the monorepo only.
There was a problem hiding this comment.
Thanks @vdavid , perhaps a better question I should have asked is: do we have an "official" style guide for various languages at Sourcegraph? Even if it is just "do whatever the {Microsoft, Google} style guide for {language} says"?
There was a problem hiding this comment.
We don't have much docs guidance for these things, what we have is linter rules. Apparently none for this one, probably it was so low value that no one has uniformized it.
There was a problem hiding this comment.
I used the JSDoc /** comment because it's shown in the hover tooltips, as opposed to the /* and // comments.
|
@chrsmith, @vdavid, thanks for your reviews! This PR doesn't make changes to the conditional routes rendering depending on the |
Cody Manage and Cody Subscription routes previously lacked
isSourcegraphDotComchecks, resulting in the error “this feature is only available on sourcegraph.com” when accessed on the enterprise version. This update adds the necessary checks to restrict these routes to Sourcegraph.com.Screenshot
To reduce clutter in the web app routes file, Cody Pro-specific implementation details (such as the
isEmbeddedCodyProUIEnabledexperimental thingy) have been moved to a separate file. This keeps the logic cleaner and more organized.CodyProPagecomponent that wraps every Cody Pro page.A new CodyProPage component has been created to wrap all Cody Pro pages. This shared wrapper will be useful for performing actions on every Cody Pro page (e.g., logging page views) and for wrapping pages with specific providers (e.g., ReactQuery provider for SSC backend access, as seen in https://github.com/sourcegraph/sourcegraph/pull/62939).
Test plan