-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migration of checkout page #3160
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
base: main
Are you sure you want to change the base?
Migration of checkout page #3160
Conversation
yashranaway
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review comments for the Checkout page Inertia migration.
|
@claude review the PR. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Pradumn27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @yashranaway _a, added some comments, please check.
Also the issue mentions to migrate:
Internal API endpoints
I don't see any endpoints getting migrated can you list the reason(if any) or migrate the APIs too if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline the CheckoutPage component here directly and use the .loggedInUserLayout to get the logged in user provider wrappers, won't need this created wrapper then
| @@ -1,3 +0,0 @@ | |||
| <%= render("shared/recaptcha_script") %> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this loaded now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yashranaway still don't see this being loaded in the component
| html = Nokogiri::HTML.parse(response.body) | ||
| expect(html.xpath("//meta[@property='gr:logged_in_user:id']/@content").text).to eq(user.external_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this removed?
| end | ||
| end | ||
|
|
||
| context "when the cart matching the `cart_id` query param has the `browser_guid` same as the current `_gumroad_guid` cookie value" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems unnecessry space, revert
|
Hey @Pradumn27 _a , updated as per review: Migrated the internal cart API following the same pattern as other Inertia migrations (collaborators, etc.)
|
Pradumn27
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yashranaway _a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change the controller location? these are still internal apis just rendered via inertia
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted, cart API stays at Api::Internal::CartsController. My bad, only the page itself needed to move to Inertia.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep reverted this too, spec is back at api/internal/.
| @@ -1,3 +0,0 @@ | |||
| <%= render("shared/recaptcha_script") %> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yashranaway still don't see this being loaded in the component
… page, fix imports and spacing
@Pradumn27 _a The recaptcha script is already loaded dynamically by the useRecaptcha hook in PaymentForm.tsx — it creates the script tag in the DOM itself. So the ERB partial was redundant. |
Issue: #3042
Fixes: #3042
Previous PR: #3081
Description
Problem
The checkout page currently uses React on Rails with ERB templates, which is inconsistent with our ongoing migration to Inertia.js. This creates maintenance overhead and prevents us from leveraging Inertia's benefits like simplified data passing and better SPA-like navigation.
Solution
Migrate the public checkout page from React on Rails to Inertia while preserving all existing functionality:
server-components/CheckoutPage.tsxtocomponents/Checkout/CheckoutPageContent.tsxpages/Checkout/Index.tsxwithdisableLayout = truerender inertia:instead of ERB + react_componentBefore/After
Before
Screen.Recording.2026-01-24.at.4.11.04.AM.mov
After
Logged out
Screen.Recording.2026-01-24.at.4.12.11.AM.mov
Logged in
Screen.Recording.2026-01-24.at.4.13.27.AM.mov
Test Results
Checklist
AI Disclosure
Claude Code (Claude Opus 4.5) was used as a coding assistant for this migration - helping with code extraction, Inertia patterns, and test updates.