PLG: migrate subscription management page#62754
Conversation
…cegraph/sourcegraph into ty/plg-subscription-management-page
vdavid
left a comment
There was a problem hiding this comment.
TBH, I didn't do a deep-review on the large new components because they didn't have a diff with the old ones (as they are in another repo), but I did skim through them and nothing stood out as wrong.
Great job overall, this must've been a lot of work with many small details to get right. Thanks for the small improvements everywhere!
| to="/cody/subscription/manage" | ||
| onClick={() => { |
There was a problem hiding this comment.
That is more concise indeed, thanks. 👍
| return <Navigate to="/cody/manage/subscription/new" replace={true} /> | ||
| } | ||
|
|
||
| return <PageContent /> |
There was a problem hiding this comment.
I like how the edge-cases are handled here before fetching the subscription. Maybe leave a comment here or on PageContent that it's separated to avoid a costly API call if we don't need it? Otherwise, someone might be inclined to merge the two because none of the two components are that large.
| ) | ||
|
|
||
| /* eslint-disable react/forbid-dom-props */ | ||
| const CodyIcon: React.FC<{ className?: string }> = ({ className }) => ( |
There was a problem hiding this comment.
Would this work instead of inlining the svg in this component?
If so, maybe also extract BackIcon to its own file next to CodyIcon.tsx for better discoverability?
There was a problem hiding this comment.
Would this work instead of inlining the svg in this component?
No, it appeared to be a different one.
maybe also extract BackIcon to its own file next to CodyIcon.tsx for better discoverability?
It's used in one place so I colocated definition and usage. If we are going to reuse it, we should move it to a separate location.
| try { | ||
| const callerResponse = await caller.call(call) | ||
| const callApi = useCallback(async () => { | ||
| setLoading(false) |
There was a problem hiding this comment.
Do you think the ignore thing was unnecessary in the first place? If it was necessary, what change made it unnecessary? (This is a genuine question on my side, I don't have the answer. I think I hadn't seen this ignore pattern in React before and when I saw it here, I thought "there must be a more concise solution than this" but I couldn't find a better one. I'm curious if you thought about it and what the solution is as you see it.)
There was a problem hiding this comment.
I thought about the current approach (custom data fetching hook) as I was migrating the Manage Subscription page.
It works without ignore, but is not reliable for this and a bunch of other reasons:
- Re-fetching After Mutations: Multiple components may mutate a resource and then trigger a re-fetch. The current approach might not handle all edge cases and side effects effectively.
- Resource Sharing Across Components: When multiple components need the same resource, we either:
- Call useApiCaller() in each component, resulting in multiple API calls. Although caching can mitigate this, implementing it reliably can be complex.
- Prop-drill the data (as done in this PR), which is not scalable either.
I recommend using a third-party library to manage server state. I've previously used React Query, which addresses these issues and many others out of the box. React Query's useQuery and useMutation hooks are similar to Apollo's, making them familiar to our team.
I plan to create a PR demonstrating the difference in approaches using React Query.
There was a problem hiding this comment.
@vdavid, @chrsmith, please let me know what you think on https://github.com/sourcegraph/sourcegraph/pull/62939
| justify-content: space-between; | ||
| align-items: center; | ||
|
|
||
| &-col { |
There was a problem hiding this comment.
TIL this works. I thought only &.something was allowed, never tried it without the dot. Thanks for teaching me this :)
Co-authored-by: David Veszelovszki <veszelovszki@gmail.com>
| // | ||
| // It's hard to do async hook testing correctly. This might be helpful: | ||
| // https://react-hooks-testing-library.com/usage/advanced-hooks#async | ||
| class FakeCaller implements Caller { |
There was a problem hiding this comment.
Refactored to use vitest mocking utilities instead of custom ones.
cc: @chrsmith
|
Thank you @taras-yemets! I've created a QA page with some small changes here: I haven't addressed a few things in the old design light address display to keep the scope low. One QQ though, how difficult would it be to add a remove credit card button? If it adds all kinds of hidden scope, forget it, but if it's a small API call and the system handles the ramifications, it would help establish more trust with users. |
…cegraph/sourcegraph into ty/plg-subscription-management-page
|
@taras-yemets, I think the back end (our DB, Go code, and Stripe) can handle it, but the front end as a whole must be prepared for the related use case of re-adding a payment method. |
This PR migrates the Cody subscription management page from SSC to the
/cody/subscription/managepath on dotcom. The page is available only if the embedded Cody Pro UI is enabled.Fixes https://github.com/sourcegraph/self-serve-cody/issues/513
2-min Loom
Implementation Details:
refetchfunction to theuseApiCallerhook to allow resource refetching after mutations.Datetypes tostringin theInvoiceandSubscriptioninterfaces, partially addressing this discussion.UI Updates:
The UI was implemented based on Figma designs with some adjustments (cc: @rrhyne):
@sourcegraph/wilcardcomponents, resulting in some style differences (e.g., background color) from the Figma design.@sourcegraph/wilcardform fields.Screenshots
Test plan
sg start)/cody/subscription/manage- page should not be renderedsg start dotcom)/cody/managepage/cody/subscription/manageroute/cody/subscription/manageby changing the URL in browser