-
-
Notifications
You must be signed in to change notification settings - Fork 318
Server side render page sitemap #3476
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3476 +/- ##
==========================================
+ Coverage 97.16% 97.18% +0.02%
==========================================
Files 285 287 +2
Lines 7466 7502 +36
==========================================
+ Hits 7254 7291 +37
+ Misses 212 211 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c9ae856 to
3135053
Compare
67707f0 to
1adf4a8
Compare
| @@ -0,0 +1,159 @@ | |||
| <li id="page_<%= @page.id %>" class="sitemap-item <%= @page.page_layout %>" data-slug="<%= @page.slug %>" data-restricted="<%= @page.restricted %>" data-page-id="<%= @page.id %>" data-folded="<%= @page.folded?(current_alchemy_user&.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.
This partial is huge. Maybe we extract some things into smaller partials to make it easier to read?
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.
Yeah. I am a bit uncertain, because every render call in Rails is very costly. It has been rendered front end side because of this. Rendering is already the largest part of the response (500ms vs 30ms DB for 1000 pages ten levels deep)
Need to do some benchmarking first.
| <% if can?(:edit_content, @page) %> | ||
| <span class="page-icon<%= @page.root? ? '' : ' handle' %>"> | ||
| <% if @page.locked? %> | ||
| <sl-tooltip content="<%= Alchemy.t("This page is locked", name: @page.locker_name) %>" class="like-hint-tooltip" placement="bottom-start"> |
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.
Maybe add an icon with tooltip helper / component? On this partial, I think there's five instances of disable_sitemap_tool(icon_name:, tooltip:)
<div class="sitemap_tool disabled">
<sl-tooltip content="<%= tooltip %>" class="like-hint-tooltip" placement="bottom-start">
<%= render_icon(icon_name) %>
</sl-tooltip>
</div>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.
See above. Let's do some benchmarking
| </div> | ||
| <div class="page_infos"> | ||
| <% if @page.locked? %> | ||
| <span class="page_status locked"> |
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.
3 instances of page_status_icon
<span class="page_status">
<alchemy-icon name="<%= icon_name %>" size="1x"></alchemy-icon>
<%= status_text %>
</span>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.
making even more render calls is exponentially increasing the server render times. Will do some benchmarking
Eager load all pages and associations and render the whole page tree on the server via a view component. Skipping folded pages for current user.
Useful for handling turbo stream responses in JS code
Replace the Handlebars-based sitemap implementation with web components and use Turbo Streams to replace the children.
Extract Page.preload_sitemap into PageTreePreloader service class following Single Responsibility Principle. This also fixes N+1 queries in the fold action by preloading the :folded_pages association in Page#preloaded_children, reducing 100+ individual queries to a single bulk query when unfolding pages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use a page to start from loading the page tree and return that page with preloaded children. Easier API then the double-state from before. Signed-off-by: Thomas von Deyen <[email protected]>
Signed-off-by: Thomas von Deyen <[email protected]>
Current LTS Signed-off-by: Thomas von Deyen <[email protected]>
Reduce attributes we do not use Signed-off-by: Thomas von Deyen <[email protected]>
Thanks Herb :) Signed-off-by: Thomas von Deyen <[email protected]>
the sitemap should not now about how to handle the folder icon and button Signed-off-by: Thomas von Deyen <[email protected]>
Using a mutation oberserver to watch for child mutations and reinit Sortable after a page was unfolded and children got replaced. Signed-off-by: Thomas von Deyen <[email protected]>
Only load pages that do not have a folded parent. This will drasticly reduce the amount of associated records that need to be preloaded for large trees with lots of folded page tranches. Signed-off-by: Thomas von Deyen <[email protected]>
If someone wants to adjust how we preload whole page trees they can confgure a different class. Signed-off-by: Thomas von Deyen <[email protected]>
1adf4a8 to
b3ae52b
Compare
This PR migrates the admin sitemap from JavaScript to server-side rendering with custom elements and Turbo Streams.
Changes
PageTreePreloaderservice classBenefits
🤖 Generated with Claude Code