Skip to content

Commit 3135053

Browse files
tvdeyenclaude
andcommitted
Extract page tree loading into service class
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]>
1 parent 04afa30 commit 3135053

File tree

7 files changed

+250
-163
lines changed

7 files changed

+250
-163
lines changed

app/controllers/alchemy/admin/pages_controller.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,10 @@ def index
5757
items = items.page(params[:page] || 1).per(items_per_page)
5858
@pages = items
5959
else
60-
@pages = Alchemy::Page.preload_sitemap(
60+
@pages = Alchemy::PageTreePreloader.new(
6161
language: @current_language,
6262
user: current_alchemy_user
63-
)
63+
).call
6464
end
6565
end
6666

app/controllers/alchemy/api/pages_controller.rb

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,14 @@ def index
2828
def nested
2929
@page = Page.find_by(id: params[:page_id]) || Language.current_root_page
3030

31+
# Preload the full tree from this page
32+
preloaded_pages = PageTreePreloader.new(from: @page, user: current_alchemy_user).call
33+
3134
render json: PageTreeSerializer.new(
32-
@page,
35+
preloaded_pages.first,
3336
ability: current_ability,
3437
user: current_alchemy_user,
35-
elements: params[:elements],
36-
full: true
38+
elements: params[:elements]
3739
)
3840
end
3941

app/models/alchemy/page.rb

Lines changed: 1 addition & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -163,43 +163,6 @@ class Page < BaseRecord
163163
# Class methods
164164
#
165165
class << self
166-
def preload_sitemap(language: nil, user: nil)
167-
scope = language ? with_language(language.id).contentpages : contentpages
168-
169-
# Load ALL pages for the language with their base associations in one query
170-
all_pages = scope.preload(*sitemap_preload_associations, :folded_pages).to_a
171-
172-
# Get folded page IDs for this specific user upfront (one fast query)
173-
folded_page_ids = if user && Alchemy.user_class < ActiveRecord::Base
174-
FoldedPage.folded_for_user(user).pluck(:page_id).to_set
175-
else
176-
Set.new
177-
end
178-
179-
# Group pages by parent_id for efficient lookup
180-
pages_by_parent = all_pages.group_by(&:parent_id)
181-
182-
# Manually populate the children association for each page
183-
# This prevents N+1 queries when the view calls page.children
184-
all_pages.each do |page|
185-
children_records = pages_by_parent[page.id] || []
186-
187-
# If page is folded for this user, set children to empty array
188-
# This prevents rendering children of folded pages
189-
page.association(:children).target = if folded_page_ids.include?(page.id)
190-
[]
191-
else
192-
# Set the association target directly to avoid database queries
193-
# sorted by lft (left) to maintain tree order
194-
children_records.sort_by(&:lft)
195-
end
196-
page.association(:children).loaded!
197-
end
198-
199-
# Return only root pages - their children are now preloaded
200-
pages_by_parent[nil] || []
201-
end
202-
203166
# Associations to preload for sitemap rendering
204167
# Override this method to customize preloading for your application
205168
#
@@ -306,7 +269,7 @@ def ransackable_attributes(_auth_object = nil)
306269
# Preloaded children for this page after unfolding
307270
# Sets each child's children to empty to prevent recursive rendering
308271
def preloaded_children
309-
children = self.children.preload(*self.class.sitemap_preload_associations)
272+
children = self.children.preload(*self.class.sitemap_preload_associations, :folded_pages)
310273
children.each do |child|
311274
child.association(:children).target = []
312275
child.association(:children).loaded!

app/serializers/alchemy/page_tree_serializer.rb

Lines changed: 11 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,43 +7,23 @@ def attributes
77
end
88

99
def pages
10-
tree = []
11-
path = [{id: object.parent_id, children: tree}]
12-
page_list = object.self_and_descendants.includes(:public_version, {language: :site})
1310
base_level = object.level - 1
14-
# Load folded pages in advance
15-
folded_user_pages = FoldedPage.folded_for_user(opts[:user]).pluck(:page_id)
16-
folded_depth = Float::INFINITY
17-
18-
page_list.each_with_index do |page, i|
19-
has_children = page_list[i + 1] && page_list[i + 1].parent_id == page.id
20-
folded = has_children && folded_user_pages.include?(page.id)
11+
build_pages_tree([object], base_level)
12+
end
2113

22-
if page.depth > folded_depth
23-
next
24-
else
25-
folded_depth = Float::INFINITY
26-
end
14+
private
2715

28-
# If this page is folded, skip all pages that are on a higher level (further down the tree).
29-
if folded && !opts[:full]
30-
folded_depth = page.depth
31-
end
16+
def build_pages_tree(pages, level)
17+
pages.map do |page|
18+
# Use association target directly to avoid triggering queries
19+
children = page.association(:children).loaded? ? page.association(:children).target : []
20+
has_children = children.any?
21+
folded = !has_children && !page.leaf?
3222

33-
if page.parent_id != path.last[:id]
34-
if path.map { |o| o[:id] }.include?(page.parent_id) # Lower level
35-
path.pop while path.last[:id] != page.parent_id
36-
else # One level up
37-
path << path.last[:children].last
38-
end
23+
page_hash(page, level, folded).tap do |hash|
24+
hash[:children] = build_pages_tree(children, level + 1)
3925
end
40-
41-
level = path.count + base_level
42-
43-
path.last[:children] << page_hash(page, level, folded)
4426
end
45-
46-
tree
4727
end
4828

4929
protected
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
# frozen_string_literal: true
2+
3+
module Alchemy
4+
# Preloads page trees with all associations and children
5+
#
6+
# This service efficiently loads page trees to avoid N+1 queries.
7+
# It handles folded pages and preloads all necessary associations.
8+
#
9+
# @example Preload all pages for a language
10+
# preloader = Alchemy::PageTreePreloader.new(language: language, user: current_user)
11+
# root_pages = preloader.call
12+
#
13+
# @example Preload subtree from a specific page
14+
# preloader = Alchemy::PageTreePreloader.new(from: page, user: current_user)
15+
# page_with_descendants = preloader.call
16+
#
17+
class PageTreePreloader
18+
# @param from [Page, nil] Starting page for loading descendants
19+
# @param language [Language, nil] Language to load pages for (loads all language's contentpages)
20+
# @param user [User, nil] User for folding support
21+
def initialize(from: nil, language: nil, user: nil)
22+
@from = from
23+
@language = language
24+
@user = user
25+
end
26+
27+
# Preloads and returns the page tree
28+
#
29+
# @return [Array<Page>] Root pages with preloaded children, or array with single page when using from:
30+
def call
31+
pages = load_pages
32+
return pages if pages.empty?
33+
34+
folded_page_ids = load_folded_page_ids
35+
preload_children_associations(pages, folded_page_ids: folded_page_ids)
36+
37+
return_result(pages)
38+
end
39+
40+
private
41+
42+
attr_reader :from, :language, :user
43+
44+
# Load the initial page collection
45+
def load_pages
46+
if from
47+
# Load subtree from specific page
48+
from.self_and_descendants.preload(*preload_associations, :folded_pages).to_a
49+
elsif language
50+
# Load all contentpages for language
51+
Page.with_language(language.id).contentpages.preload(*preload_associations, :folded_pages).to_a
52+
else
53+
Rails.logger.warn("Neither a start page not language given! Skipping preloading pages.")
54+
[]
55+
end
56+
end
57+
58+
# Load folded page IDs for the user
59+
def load_folded_page_ids
60+
if user && Alchemy.user_class < ActiveRecord::Base
61+
FoldedPage.folded_for_user(user).pluck(:page_id).to_set
62+
else
63+
Set.new
64+
end
65+
end
66+
67+
# Preload children associations for a collection of pages
68+
# This manually populates the children association to prevent N+1 queries
69+
#
70+
# @param pages [Array<Page>] The pages to preload children for
71+
# @param folded_page_ids [Set] Optional set of page IDs that should have empty children
72+
# @return [void]
73+
def preload_children_associations(pages, folded_page_ids: Set.new)
74+
# Group pages by parent_id for efficient lookup
75+
pages_by_parent = pages.group_by(&:parent_id)
76+
77+
# Manually populate the children association for each page
78+
pages.each do |page|
79+
children_records = pages_by_parent[page.id] || []
80+
81+
# If page is folded, set children to empty array
82+
page.association(:children).target = if folded_page_ids.include?(page.id)
83+
[]
84+
else
85+
children_records.sort_by(&:lft)
86+
end
87+
page.association(:children).loaded!
88+
end
89+
end
90+
91+
# Return appropriate result based on input
92+
def return_result(pages)
93+
if from
94+
# Return the starting page in an array (which now has preloaded descendants)
95+
# We need to return the actual instance from the pages array, not the @from instance
96+
# because the children associations were set on the pages array instances
97+
starting_page = pages.find { |p| p.id == from.id } || from
98+
[starting_page]
99+
else
100+
# Return only root pages - their children are now preloaded
101+
pages.group_by(&:parent_id)[nil] || []
102+
end
103+
end
104+
105+
# Associations to preload for sitemap rendering
106+
def preload_associations
107+
Page.sitemap_preload_associations
108+
end
109+
end
110+
end

spec/models/alchemy/page_spec.rb

Lines changed: 0 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -404,95 +404,6 @@ module Alchemy
404404
end
405405
end
406406

407-
describe ".preload_sitemap" do
408-
let(:user) { create(:alchemy_dummy_user) }
409-
let!(:root_page) { create(:alchemy_page, :language_root, language: language) }
410-
let!(:child_page_1) { create(:alchemy_page, parent: root_page, language: language) }
411-
let!(:child_page_2) { create(:alchemy_page, parent: root_page, language: language) }
412-
let!(:grandchild_page) { create(:alchemy_page, parent: child_page_1, language: language) }
413-
let!(:layoutpage) { create(:alchemy_page, :layoutpage, language: language) }
414-
415-
subject { Page.preload_sitemap(language: language, user: user) }
416-
417-
it "returns only root pages" do
418-
expect(subject).to eq([root_page])
419-
end
420-
421-
it "does not include layout pages" do
422-
expect(subject).to_not include(layoutpage)
423-
end
424-
425-
it "preloads children association" do
426-
result = subject.first
427-
expect(result.association(:children)).to be_loaded
428-
end
429-
430-
it "preloads children in correct tree order" do
431-
result = subject.first
432-
expect(result.children).to eq([child_page_1, child_page_2])
433-
end
434-
435-
it "preloads grandchildren" do
436-
result = subject.first
437-
expect(result.children.first.children).to eq([grandchild_page])
438-
end
439-
440-
it "preloads public_version association" do
441-
result = subject.first
442-
expect(result.association(:public_version)).to be_loaded
443-
end
444-
445-
it "preloads language and site associations" do
446-
result = subject.first
447-
expect(result.association(:language)).to be_loaded
448-
expect(result.language.association(:site)).to be_loaded
449-
end
450-
451-
context "without language parameter" do
452-
subject { Page.preload_sitemap(user: user) }
453-
454-
it "returns all content pages" do
455-
expect(subject).to include(root_page)
456-
end
457-
end
458-
459-
context "with folded pages" do
460-
before do
461-
child_page_1.fold!(user.id, true)
462-
end
463-
464-
it "does not include children of folded pages" do
465-
result = subject.first
466-
expect(result.children.first.children).to eq([])
467-
end
468-
469-
it "still returns the folded page itself" do
470-
result = subject.first
471-
expect(result.children).to include(child_page_1)
472-
end
473-
end
474-
475-
context "without user" do
476-
subject { Page.preload_sitemap(language: language) }
477-
478-
it "returns pages with all children loaded" do
479-
result = subject.first
480-
expect(result.children.first.children).to eq([grandchild_page])
481-
end
482-
end
483-
484-
context "with multiple languages" do
485-
let!(:klingon_root) { create(:alchemy_page, :language_root, language: klingon) }
486-
let!(:klingon_child) { create(:alchemy_page, parent: klingon_root, language: klingon) }
487-
488-
it "only returns pages for the specified language" do
489-
result = Page.preload_sitemap(language: language, user: user)
490-
all_pages = [result, *result.flat_map(&:children)].flatten
491-
expect(all_pages).to_not include(klingon_root, klingon_child)
492-
end
493-
end
494-
end
495-
496407
describe ".copy" do
497408
let(:page) { create(:alchemy_page, name: "Source") }
498409

0 commit comments

Comments
 (0)