-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migrate Discover Page to Inertia #3227
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?
Migrate Discover Page to Inertia #3227
Conversation
- Inline DiscoverPage component into Index.tsx - Convert BlackFridayButton to use Inertia Link - Revert _head.html.erb changes - Add specific prop value assertions in tests
- Generate dynamic page titles based on query, tags, and taxonomy - Use TagPageMetaPresenter.title for tag-only pages - Build taxonomy breadcrumb titles from ancestor labels - Titles follow format: [query/tags] | [taxonomy path] | Gumroad
- Add protocol (scheme) to all URL helpers in Discover Layout - Fix dashboard, login, signup, library, and products URLs - Fix discover URLs for logo and taxonomy navigation
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.
@Pradumn27 _a Added fixes as per previous review
Some few decisions I made:
For browser history, I followed the same pattern used in Library/Index.tsx - using replaceState for back/forward navigation and pushState for user actions. This keeps the history stack clean.
The recommended products carousel gets its initial data from server props. When users switch taxonomies without a full reload, we hit the existing /discover/recommended_products endpoint to keep the carousel relevant.
For taxonomy navigation, I went with client-side state management instead of router.get(). The problem with router.get() was that it would reset component state, so things like open filter panels would close when switching categories. Managing it through the reducer fixes that and the popstate listener handles back/forward correctly.
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.
- Removed
@hide_layouts = true(was preventing Inertia from loading) - Added
autocomplete_dataprop for server-side autocomplete - Added server-side page title generation to match client-side logic
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.
Added autocompleteData prop to pass server data to Search component
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.
Now handles null/undefined wishlists with loading placeholder
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.
Footer component moved to shared location for reuse
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, there are still changes remaining, added comments, please check them.
Also, can you mention why is search autocomplete apis not migrated here? please always add comments with context about the changes done and not done in the PR.
Also, in the video in the PR we can't see the recommended wishlist products, so please add them here so we can see they work correctly too
| title_parts = [] | ||
| if params[:query].present? | ||
| title_parts << "Search results for \"#{params[:query]}\"" | ||
| elsif params[:tags].present? && !params[:taxonomy].present? | ||
| presenter = Discover::TagPageMetaPresenter.new(params[:tags], @search_results[:total]) | ||
| title_parts << presenter.title | ||
| elsif params[:tags].present? | ||
| tags = params[:tags].is_a?(Array) ? params[:tags] : params[:tags].split(",") | ||
| title_parts << tags.map { |t| t.strip.gsub(/[-\s]+/, " ") }.join(", ") | ||
| end | ||
| if params[:taxonomy].present? | ||
| title_parts << params[:taxonomy].split("/").map { |slug| Discover::TaxonomyPresenter::TAXONOMY_LABELS[slug] || slug }.join(" » ") | ||
| end | ||
| title_parts << "Gumroad" | ||
| set_meta_tag(title: title_parts.join(" | ")) |
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 being done? please add comments explaining when you add anything different from the previous behaviour
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.
Page title for initial render and SEO crawlers. The frontend also generates titles via discoverTitleGenerator() when users navigate with filters, but we need this server-side for the first page load and search engines that don't execute JS.
|
@Pradumn27 _a added fixes
I have added in PR description
Added now |
@yashranaway _a, the And regarding your other concerns for not migrating it, we have migrated search at a lot of places in the codebase to inertia so it shouldn't be a problem to migrate it here |
|
The only remaining blocker is server-components/Discover/ProductPage.tsx, which shares the Search component. We'll wait until that blocker is resolved. Happy to migrate the autocomplete endpoints to Inertia once lands. |
Issue: #3047
Fixes: #3047
Previous PR: #3075, #3163 #3208
Description
Problem
The Discover page currently uses React on Rails, which requires maintaining separate rendering logic and doesn't benefit from Inertia's client-side navigation.
Solution
Migrate the Discover page to Inertia following the established patterns in the codebase. This removes the old React on Rails setup and uses
render inertia:with proper props.Changes
render inertia:instead of setting@react_discover_propslayout "inertia"for the index actionrecommended_products,recommended_wishlists, and other deferred dataRecommendedWishlistscomponent now accepts wishlists as props instead of fetchingload_pack("inertia")from inertia layoutrouter.visitwithpreserveStatefor URL updates instead of manual history managementEndpoints
/discover/discover/recommended_products/discover/search_autocomplete(GET)/discover/search_autocomplete(DELETE)Note on Search Autocomplete
The search autocomplete endpoints are kept as JSON APIs for now. The
Searchcomponent is shared with the Product page (server-components/Discover/ProductPage.tsx), which is still on React on Rails. Once the Product page migration lands (#3264 / #3061), search autocomplete can be migrated to Inertia as a follow-up.Before/After
Before
Screen.Recording.2026-01-23.at.11.53.37.PM.mov
After
Screen.Recording.2026-01-23.at.11.59.15.PM.mov
Screen.Recording.2026-01-30.at.9.51.54.AM.mov
Test Results
Controller tests: 10/10 passing
System tests: 52/64 passing (12 failures unrelated to migration)
The 12 remaining failures are pre-existing issues:
within_cart_itemlooking forh4elementChecklist
AI Disclosure
Claude Opus 4.5 was used to assist with code generation and migration patterns.