FIX: Normalize nested replies at the depth cap (#42987)

This commit is contained in:
Mark VanLandingham
2026-08-31 09:05:48 -05:00
committed by GitHub
parent 5473192f2e
commit f495598f63
12 changed files with 563 additions and 64 deletions
+7 -1
View File
@@ -86,7 +86,13 @@ class NestedTopic::ListChildren
else
0
end
tree_data = loader.batch_preload_tree(children_posts, params.sort, max_depth: remaining_depth)
tree_data =
loader.batch_preload_tree(
children_posts,
params.sort,
max_depth: remaining_depth,
starting_depth: params.depth,
)
context[:children_map] = tree_data[:children_map]
context[:all_posts] = tree_data[:all_posts]
end
+1
View File
@@ -77,6 +77,7 @@ class NestedTopic::ListRoots
roots,
params.sort,
max_depth: NestedReplies::TreeLoader::PRELOAD_DEPTH,
starting_depth: 0,
)
context[:children_map] = tree_data[:children_map]
context[:all_posts] = tree_data[:all_posts]
+8 -2
View File
@@ -112,12 +112,18 @@ class NestedTopic::ShowContext
context[:siblings_map] = loader.batch_load_siblings(ancestors, params.sort)
end
def expand_reply_trees(params:, loader:, target_post:)
def expand_reply_trees(params:, loader:, target_post:, ancestors:)
starting_depth = ancestors.length
remaining_depth = [
NestedReplies::TreeLoader::PRELOAD_DEPTH,
loader.configured_max_depth - starting_depth,
].min
tree_data =
loader.batch_preload_tree(
[target_post],
params.sort,
max_depth: NestedReplies::TreeLoader::PRELOAD_DEPTH,
max_depth: [remaining_depth, 0].max,
starting_depth: starting_depth,
)
context[:children_map] = tree_data[:children_map]
context[:tree_posts] = tree_data[:all_posts]
@@ -89,7 +89,9 @@ export default class NestedPostChildren extends Component {
if (cached) {
this.childNodes = cached.childNodes;
this.page = cached.page;
this.hasMore = cached.hasMore;
this.hasMore = this.usesFlatDescendantPagination
? this.expectedCount > this.childNodes.length
: cached.hasMore;
this.loaded = true;
this._fetchedFromServer = cached.fetchedFromServer;
return;
@@ -98,15 +100,7 @@ export default class NestedPostChildren extends Component {
if (this.args.preloadedChildren?.length > 0) {
this.childNodes = this.args.preloadedChildren;
this.loaded = true;
// When cap is ON at last level, the children endpoint returns flattened
// descendants, so use total_descendant_count for the "more" threshold.
const flatten =
this.siteSettings.nested_replies_cap_nesting_depth &&
this.childDepth >= this.siteSettings.nested_replies_max_depth;
const expectedCount = flatten
? this.args.totalDescendantCount || this.args.directReplyCount || 0
: this.args.directReplyCount || 0;
this.hasMore = expectedCount > this.args.preloadedChildren.length;
this.hasMore = this.expectedCount > this.childNodes.length;
} else if (this.args.directReplyCount > 0) {
this.loadChildren();
}
@@ -132,9 +126,7 @@ export default class NestedPostChildren extends Component {
return;
}
const alreadyExists = this.childNodes.some(
(n) => n.post.id === post.id || n.post.post_number === post.post_number
);
const alreadyExists = this._includesPost(this.childNodes, post);
if (alreadyExists) {
return;
}
@@ -148,14 +140,21 @@ export default class NestedPostChildren extends Component {
return this.args.depth + 1;
}
get remainingCount() {
const flatten =
get usesFlatDescendantPagination() {
return (
this.siteSettings.nested_replies_cap_nesting_depth &&
this.childDepth >= this.siteSettings.nested_replies_max_depth;
const total = flatten
this.childDepth >= this.siteSettings.nested_replies_max_depth
);
}
get expectedCount() {
return this.usesFlatDescendantPagination
? this.args.totalDescendantCount || this.args.directReplyCount || 0
: this.args.directReplyCount || 0;
return Math.max(total - this.childNodes.length, 0);
}
get remainingCount() {
return Math.max(this.expectedCount - this.childNodes.length, 0);
}
get loadMoreLabel() {
@@ -251,7 +250,10 @@ export default class NestedPostChildren extends Component {
this.childNodes = [...this.childNodes, ...additional];
this._fetchedFromServer = true;
} else {
this.childNodes = [...this.childNodes, ...newNodes];
const additional = newNodes.filter(
(node) => !this._includesPost(this.childNodes, node.post)
);
this.childNodes = [...this.childNodes, ...additional];
}
this.page = data.page;
@@ -279,6 +281,13 @@ export default class NestedPostChildren extends Component {
return processNode(this.store, this.args.topic, nodeData);
}
_includesPost(nodes, post) {
return nodes.some(
(node) =>
node.post.id === post.id || node.post.post_number === post.post_number
);
}
<template>
<div
class="nested-post-children"
@@ -292,6 +292,99 @@ module("Integration | Component | Nested | Post", function (hooks) {
assert.verifySteps([], "does not request children while rendering");
});
test("renders server-flattened descendants once at the nesting depth cap", async function (assert) {
this.siteSettings.nested_replies_cap_nesting_depth = true;
this.siteSettings.nested_replies_max_depth = 3;
this.depth = 2;
this.post.setProperties({
direct_reply_count: 2,
total_descendant_count: 4,
});
const child = (id, postNumber) => ({
post: this.store.createRecord("post", {
id,
post_number: postNumber,
topic: this.topic,
user_id: 2,
username: `user-${id}`,
avatar_template: "/letter_avatar_proxy/v4/letter/u/25/48.png",
cooked: `<p>Post ${postNumber}</p>`,
created_at: "2026-01-01T00:00:00.000Z",
actions_summary: [],
direct_reply_count: 0,
total_descendant_count: 0,
}),
children: [],
});
const grandchild56 = child(56, 56);
const grandchild51 = child(51, 51);
this.children = [child(48, 48), child(49, 49), grandchild56, grandchild51];
await renderComponent(this);
const appEvents = getOwner(this).lookup("service:app-events");
appEvents.trigger("nested-replies:child-created", {
topicId: this.topic.id,
parentPostNumber: this.post.post_number,
post: grandchild56.post,
});
assert
.dom(".nested-post.--depth-3")
.exists({ count: 4 }, "renders every descendant at the capped depth");
assert
.dom('[data-post-id="56"]')
.exists({ count: 1 }, "renders a deep descendant exactly once");
assert
.dom('[data-post-id="51"]')
.exists({ count: 1 }, "renders descendants from each branch");
assert
.dom(".nested-post-children__load-more")
.doesNotExist("does not offer a fetch when every descendant is loaded");
});
test("recomputes capped descendant pagination when restoring child cache", async function (assert) {
this.siteSettings.nested_replies_cap_nesting_depth = true;
this.siteSettings.nested_replies_max_depth = 3;
this.depth = 2;
this.post.setProperties({
direct_reply_count: 2,
total_descendant_count: 3,
});
const cachedChild = this.store.createRecord("post", {
id: 48,
post_number: 48,
topic: this.topic,
user_id: 2,
username: "cached-user",
avatar_template: "/letter_avatar_proxy/v4/letter/c/25/48.png",
cooked: "<p>Cached post</p>",
created_at: "2026-01-01T00:00:00.000Z",
actions_summary: [],
direct_reply_count: 0,
total_descendant_count: 0,
});
this.fetchedChildrenCache.set("1:2", {
childNodes: [{ post: cachedChild, children: [] }],
page: 0,
hasMore: false,
fetchedFromServer: false,
});
this.expansionState.set(this.post.post_number, {
expanded: true,
collapsed: false,
});
await renderComponent(this);
assert.dom('[data-post-id="48"]').exists("restores the cached descendant");
assert
.dom(".nested-post-children__load-more")
.exists("offers the missing descendants despite stale cached hasMore");
});
test("renders multi-select controls", async function (assert) {
let selectedPost;
this.multiSelect = true;
@@ -372,6 +372,64 @@ module("Unit | Controller | nested", function (hooks) {
}
});
test("live deep replies target the capped-depth container", async function (assert) {
const topic = buildTopic(this.store, 724);
const root = buildPost(this.store, topic, 1001, 45);
const secondLevel = buildPost(this.store, topic, 1002, 46);
const boundaryParent = buildPost(this.store, topic, 1003, 47);
const boundaryChild = buildPost(this.store, topic, 1004, 48);
const deepPostId = 2001;
let childCreatedEvent;
secondLevel.set("reply_to_post_number", root.post_number);
boundaryParent.set("reply_to_post_number", secondLevel.post_number);
boundaryChild.set("reply_to_post_number", boundaryParent.post_number);
this.owner.lookup(
"service:site-settings"
).nested_replies_cap_nesting_depth = true;
this.owner.lookup("service:site-settings").nested_replies_max_depth = 3;
this.controller.topic = topic;
this.controller.subscribe();
[root, secondLevel, boundaryParent, boundaryChild].forEach((post) =>
this.appEvents.trigger("nested-replies:post-registered", post)
);
this.appEvents.on("nested-replies:child-created", this, (event) => {
childCreatedEvent = event;
});
pretender.get(`/posts/${deepPostId}.json`, () =>
response({
id: deepPostId,
post_number: 56,
topic_id: topic.id,
user_id: this.currentUser.id,
username: this.currentUser.username,
avatar_template: this.currentUser.avatar_template,
cooked: "<p>Deep reply</p>",
created_at: "2026-01-01T00:00:00.000Z",
actions_summary: [],
direct_reply_count: 0,
total_descendant_count: 0,
reply_to_post_number: boundaryChild.post_number,
children: [],
})
);
this.controller._onMessage(
{ type: "created", id: deepPostId, user_id: this.currentUser.id },
null,
123
);
await settled();
assert.strictEqual(
childCreatedEvent?.parentPostNumber,
boundaryParent.post_number,
"dispatches beneath the parent whose children render at the depth cap"
);
});
test("acted event refreshes actionByName so the flag modal stays in sync", async function (assert) {
const topic = buildTopic(this.store, 724);
const postId = 3001;
+137 -36
View File
@@ -106,10 +106,19 @@ module NestedReplies
scope
end
def batch_preload_tree(starting_posts, sort, max_depth:)
def batch_preload_tree(starting_posts, sort, max_depth:, starting_depth: 0)
sort = effective_sort(sort)
return batch_preload_hot_tree(starting_posts, max_depth: max_depth) if sort == "hot"
tree_data =
if sort == "hot"
batch_preload_hot_tree(starting_posts, max_depth: max_depth)
else
batch_preload_sorted_tree(starting_posts, sort, max_depth: max_depth)
end
flatten_capped_boundary(tree_data, starting_posts, sort, starting_depth: starting_depth)
end
def batch_preload_sorted_tree(starting_posts, sort, max_depth:)
all_posts = starting_posts.dup
children_map = {}
@@ -424,53 +433,145 @@ module NestedReplies
def flat_descendants_scope(parent_post_number, sort:, offset: 0, limit: CHILDREN_PER_PAGE)
sort = effective_sort(sort)
post_types = visible_post_types
order_expr = NestedReplies::Sort.sql_order_expression(sort, posts_table: "p")
hot_join = sort == "hot" ? NestedReplies::Sort.hot_score_join_sql(posts_table: "p") : ""
descendant_post_numbers =
DB.query_single(
<<~SQL,
WITH RECURSIVE descendants AS (
SELECT post_number, 1 AS depth
FROM posts
WHERE topic_id = :topic_id
AND reply_to_post_number = :parent_number
AND post_number > 1
UNION ALL
SELECT p.post_number, d.depth + 1
FROM posts p
JOIN descendants d ON p.reply_to_post_number = d.post_number
WHERE p.topic_id = :topic_id
AND p.post_number > 1
AND d.depth < :max_cte_depth
)
SELECT d.post_number
FROM descendants d
JOIN posts p ON p.post_number = d.post_number AND p.topic_id = :topic_id
#{hot_join}
WHERE p.post_type IN (:post_types)
ORDER BY #{order_expr}
OFFSET :offset
LIMIT :limit
SQL
topic_id: topic.id,
parent_number: parent_post_number,
post_types: post_types,
flat_descendant_post_numbers_by_parent(
[parent_post_number],
sort: sort,
offset: offset,
limit: limit,
max_cte_depth: 500,
)
).fetch(parent_post_number, [])
scope =
topic.posts.with_deleted.where(post_number: descendant_post_numbers).where(post_number: 2..)
NestedReplies::Sort.apply(scope, sort)
end
def flat_descendant_post_numbers_by_parent(parent_post_numbers, sort:, offset: 0, limit:)
return {} if parent_post_numbers.empty?
sort = effective_sort(sort)
order_expr = NestedReplies::Sort.sql_order_expression(sort, posts_table: "posts")
hot_join = sort == "hot" ? NestedReplies::Sort.hot_score_join_sql : ""
rows =
DB.query(
<<~SQL,
WITH RECURSIVE descendants AS (
SELECT parents.post_number AS root_post_number,
children.post_number,
1 AS depth
FROM posts parents
JOIN posts children
ON children.topic_id = parents.topic_id
AND children.reply_to_post_number = parents.post_number
WHERE parents.topic_id = :topic_id
AND parents.post_number IN (:parent_post_numbers)
AND children.post_number > 1
UNION ALL
SELECT descendants.root_post_number,
children.post_number,
descendants.depth + 1
FROM descendants
JOIN posts children
ON children.topic_id = :topic_id
AND children.reply_to_post_number = descendants.post_number
WHERE children.post_number > 1
AND descendants.depth < :max_cte_depth
), ranked AS (
SELECT descendants.root_post_number,
posts.post_number,
ROW_NUMBER() OVER (
PARTITION BY descendants.root_post_number
ORDER BY #{order_expr}
) AS row_number
FROM descendants
JOIN posts
ON posts.topic_id = :topic_id
AND posts.post_number = descendants.post_number
#{hot_join}
WHERE posts.post_type IN (:post_types)
)
SELECT root_post_number, post_number
FROM ranked
WHERE row_number > :offset
AND row_number <= :offset + :limit
ORDER BY root_post_number, row_number
SQL
topic_id: topic.id,
parent_post_numbers: parent_post_numbers,
post_types: visible_post_types,
offset: offset,
limit: limit,
max_cte_depth: 500,
)
rows
.group_by(&:root_post_number)
.transform_values { |parent_rows| parent_rows.map(&:post_number) }
end
def configured_max_depth
SiteSetting.nested_replies_max_depth
end
def flatten_capped_boundary(tree_data, starting_posts, sort, starting_depth:)
return tree_data unless SiteSetting.nested_replies_cap_nesting_depth
boundary_depth = configured_max_depth - starting_depth - 1
return tree_data if boundary_depth.negative?
boundary_parents = starting_posts
boundary_depth.times do
boundary_parents =
boundary_parents.flat_map { |post| tree_data[:children_map].fetch(post.post_number, []) }
return tree_data if boundary_parents.empty?
end
# Non-destructive: at boundary_depth 0 this is still the caller's own array.
boundary_parents =
boundary_parents.select { |post| tree_data[:children_map].key?(post.post_number) }
return tree_data if boundary_parents.empty?
descendant_numbers_by_parent =
flat_descendant_post_numbers_by_parent(
boundary_parents.map(&:post_number),
sort: sort,
limit: PRELOAD_CHILDREN_PER_PARENT,
)
descendant_numbers = descendant_numbers_by_parent.values.flatten
descendants_by_number =
load_posts_for_tree(
topic.posts.with_deleted.where(post_number: descendant_numbers),
).index_by(&:post_number)
boundary_parents.each do |parent|
tree_data[:children_map][parent.post_number] = descendant_numbers_by_parent
.fetch(parent.post_number, [])
.filter_map { |post_number| descendants_by_number[post_number] }
end
tree_data[:all_posts] = reachable_tree_posts(starting_posts, tree_data[:children_map])
tree_data
end
def reachable_tree_posts(starting_posts, children_map)
posts = []
# A reply_to_post_number cycle would otherwise make this walk unbounded.
seen = Set.new
pending = starting_posts.dup
until pending.empty?
post = pending.shift
next unless seen.add?(post.post_number)
posts << post
pending.concat(children_map.fetch(post.post_number, []))
end
posts.uniq(&:id)
end
def direct_reply_counts(post_numbers)
return {} if post_numbers.empty?
@@ -303,6 +303,49 @@ RSpec.describe "Nested replies N+1 elimination", type: :request do
end
end
describe "capped boundary preload" do
it "uses one flat-descendant query regardless of the number of boundary parents" do
SiteSetting.nested_replies_cap_nesting_depth = true
SiteSetting.nested_replies_max_depth = 2
topics =
[1, 5].map do |branch_count|
nested_topic = Fabricate(:topic, user: user)
Fabricate(:post, topic: nested_topic, user: user, post_number: 1)
Fabricate(:nested_topic, topic: nested_topic)
branch_count.times do
root = Fabricate(:post, topic: nested_topic, user: user, reply_to_post_number: 1)
boundary_parent =
Fabricate(
:post,
topic: nested_topic,
user: user,
reply_to_post_number: root.post_number,
)
Fabricate(
:post,
topic: nested_topic,
user: user,
reply_to_post_number: boundary_parent.post_number,
)
end
nested_topic
end
sign_in(user)
flat_query_counts =
topics.map do |nested_topic|
queries =
track_sql_queries { get "/n/#{nested_topic.slug}/#{nested_topic.id}.json?sort=old" }
queries.count { |query| query.include?("PARTITION BY descendants.root_post_number") }
end
expect(flat_query_counts).to eq([1, 1])
end
end
describe "hot sorting" do
def build_hot_topic(root_count)
hot_topic = Fabricate(:topic, user: user)
@@ -82,5 +82,26 @@ RSpec.describe NestedTopic::ListChildren do
response[:children].each { |child| expect(child[:children]).to eq([]) }
end
end
context "when the requested depth is one level above the cap" do
before do
SiteSetting.nested_replies_cap_nesting_depth = true
SiteSetting.nested_replies_max_depth = 3
end
let(:params) do
{ parent_post_number: parent_post.post_number, sort: "old", page: 0, depth: 2 }
end
it "returns children that have no replies of their own" do
leaf =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: parent_post.post_number)
branch =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: parent_post.post_number)
Fabricate(:post, topic: topic, user: user, reply_to_post_number: branch.post_number)
expect(result[:response][:children].map { |child| child[:id] }).to eq([leaf.id, branch.id])
end
end
end
end
@@ -60,6 +60,66 @@ RSpec.describe NestedTopic::ListRoots do
end
end
context "when nesting is capped" do
before do
SiteSetting.nested_replies_cap_nesting_depth = true
SiteSetting.nested_replies_max_depth = 3
end
it "returns deeper branches as one sorted collection at the depth boundary" do
root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1)
second_level =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number)
boundary_parent =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: second_level.post_number)
left =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: boundary_parent.post_number,
)
right =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: boundary_parent.post_number,
)
right_children =
2.times.map do
Fabricate(:post, topic: topic, user: user, reply_to_post_number: right.post_number)
end
left_children =
2.times.map do
Fabricate(:post, topic: topic, user: user, reply_to_post_number: left.post_number)
end
response =
stub_const(NestedReplies::TreeLoader, :PRELOAD_CHILDREN_PER_PARENT, 10) do
described_class.call(params: { sort: "old", page: 0 }, **dependencies)[:response]
end
boundary_json = response[:roots].first.dig(:children, 0, :children, 0)
expected_posts = [left, right, *right_children, *left_children]
expect(boundary_json[:children].map { |post_json| post_json[:id] }).to eq(
expected_posts.map(&:id),
)
expect(boundary_json[:children].map { |post_json| post_json[:children] }.uniq).to eq([[]])
end
it "keeps roots without replies when the boundary is the root level" do
SiteSetting.nested_replies_max_depth = 1
lonely = Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1)
chatty = Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1)
Fabricate(:post, topic: topic, user: user, reply_to_post_number: chatty.post_number)
response = described_class.call(params: { sort: "old", page: 0 }, **dependencies)[:response]
expect(response[:roots].map { |root_json| root_json[:id] }).to eq([lonely.id, chatty.id])
end
end
context "when staff views root posts with official notices" do
fab!(:admin) { Fabricate(:admin, refresh_auto_groups: true) }
fab!(:root_post) { Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1) }
@@ -115,5 +115,111 @@ RSpec.describe NestedTopic::ShowContext do
expect(response[:siblings]).to be_a(Hash)
end
end
context "when context reaches the capped display boundary" do
before do
SiteSetting.nested_replies_cap_nesting_depth = true
SiteSetting.nested_replies_max_depth = 3
end
it "returns all deeper branches as sorted children of the target" do
root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1)
second_level =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number)
boundary_target =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: second_level.post_number)
low_direct =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: boundary_target.post_number,
like_count: 1,
)
high_direct =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: boundary_target.post_number,
like_count: 8,
)
highest_deep =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: low_direct.post_number,
like_count: 12,
)
middle_deep =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: high_direct.post_number,
like_count: 5,
)
response =
stub_const(NestedReplies::TreeLoader, :PRELOAD_CHILDREN_PER_PARENT, 10) do
described_class.call(
params: {
target_post_number: boundary_target.post_number,
sort: "top",
},
**dependencies,
)[
:response
]
end
expected_posts = [highest_deep, high_direct, middle_deep, low_direct]
expect(response[:target_post][:children].map { |post_json| post_json[:id] }).to eq(
expected_posts.map(&:id),
)
expect(
response[:target_post][:children].map { |post_json| post_json[:children] }.uniq,
).to eq([[]])
end
it "leaves descendants for boundary pagination when the target is already at max depth" do
root = Fabricate(:post, topic: topic, user: user, reply_to_post_number: 1)
second_level =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: root.post_number)
boundary_parent =
Fabricate(:post, topic: topic, user: user, reply_to_post_number: second_level.post_number)
max_depth_target =
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: boundary_parent.post_number,
)
Fabricate(
:post,
topic: topic,
user: user,
reply_to_post_number: max_depth_target.post_number,
)
response =
described_class.call(
params: {
target_post_number: max_depth_target.post_number,
sort: "old",
},
**dependencies,
)[
:response
]
expect(response[:target_post][:children]).to eq([])
expect(response[:ancestor_chain].last).to include(
id: boundary_parent.id,
total_descendant_count: 2,
)
end
end
end
end
+1 -6
View File
@@ -27,13 +27,8 @@ RSpec.describe "Nested view depth and nesting" do
expect(nested_view).to have_post_at_depth(chain[0], depth: 0)
expect(nested_view).to have_post_at_depth(chain[1], depth: 1)
expect(nested_view).to have_post_at_depth(chain[2], depth: 2)
expect(nested_view).to have_no_post(chain[3])
expect(nested_view).to have_no_continue_thread_for(chain[2])
expect(nested_view).to have_load_more_children_for(chain[1])
nested_view.click_load_more_children(chain[1])
expect(nested_view).to have_post_at_depth(chain[3], depth: 2)
expect(nested_view).to have_no_continue_thread_for(chain[2])
end
it "allows deeper nesting with higher max depth" do