Skip to content

Fix root node siblings not reordered after prepend_child/add_sibling moves root to child position #482

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.

Sign up for GitHub

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

Draft
Copilot wants to merge 3 commits into
base: master
Choose a base branch
from
+79 −1
Draft

Fix root node siblings not reordered after prepend_child/add_sibling moves root to child position #482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ecf4bb
Initial plan
Copilot Jan 31, 2026
04e3abb
Fix root node siblings not reordered after prepend_child/add_sibling …
Copilot Jan 31, 2026
8dfac7c
Restore original schema files
Copilot Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/closure_tree/numeric_deterministic_ordering.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,15 @@ def _ct_reorder_children(minimum_sort_order_value = nil)
_ct.reorder_with_parent_id(_ct_id, minimum_sort_order_value, scope_conditions)
end

def _ct_reorder_prior_parent_or_roots(prior_parent, was_root)
if prior_parent
prior_parent._ct_reorder_children
elsif was_root && !_ct.dont_order_roots
scope_conditions = _ct.scope_values_from_instance(self)
_ct.reorder_with_parent_id(nil, nil, scope_conditions)
end
end

def self_and_descendants_preordered
# TODO: raise NotImplementedError if sort_order is not numeric and not null?
hierarchy_table = self.class.hierarchy_class.arel_table
Expand Down Expand Up @@ -130,11 +139,14 @@ def append_child(child_node)
end

def prepend_child(child_node)
prior_parent = child_node.parent
was_root = prior_parent.nil? && child_node.persisted?
child_node.order_value = -1
child_node.parent = self
child_node._ct_skip_sort_order_maintenance!
Copy link
Contributor

@vsmay98 vsmay98 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I’m trying to understand the need for skip_sort_order_maintenance in prepend_child. My understanding is that prepend_child is only used when moving a node from one parent/root to another. In that scenario, wouldn’t the normal flow where after_save triggers rebuild!— already handle the sort order correctly?

Copy link
Member

@seuros seuros Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR was generated from #481 .

I just wanted to see if @copilot will nail the edge-cases in a library that is well known in its dataset.

Basically Copilot shipped vibe code that pass the tests, but you can see that it could not run test because of the firewall.

Test are wrong because we need to test the same pattern in 3 records
Label For postgres
MemoryTag for sqlite
and any other mysql record.

Only then we can release a new version.

If you want to pickup it, go ahead. The docker compose will give you everything you need, and feel free to to ping me here if you need clarification.

Copy link
Contributor

@vsmay98 vsmay98 Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @seuros I did went through the #481 issue. I was looking more closely at the existing implementation in prepend_child, specifically the call to child_node._ct_skip_sort_order_maintenance!, which seems to be the reason this issue occurs. I wanted to better understand the need for this, since prepend_child is only used to move a node to a different parent, and in that case _ct_skip_sort_order_maintenance doesn’t seem to make sense. I may be misunderstanding the intent here.

if child_node.save
_ct_reorder_children
_ct_reorder_prior_parent_or_roots(prior_parent, was_root)
child_node.reload
else
child_node
Expand All @@ -161,6 +173,7 @@ def add_sibling(sibling, add_after = true)

_ct.with_advisory_lock do
prior_sibling_parent = sibling.parent
sibling_was_root = prior_sibling_parent.nil? && sibling.persisted?
reorder_from_value = if prior_sibling_parent == parent
[order_value, sibling.order_value].compact.min
else
Expand All @@ -184,7 +197,7 @@ def add_sibling(sibling, add_after = true)
sibling.update_order_value(self_ov)
end

prior_sibling_parent.try(:_ct_reorder_children) if prior_sibling_parent != parent
sibling._ct_reorder_prior_parent_or_roots(prior_sibling_parent, sibling_was_root) if prior_sibling_parent != parent
sibling
end
end
Expand Down
65 changes: 65 additions & 0 deletions test/closure_tree/label_order_value_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,69 @@ def setup
assert_equal 1, b.order_value
assert_equal 2, c.order_value
end

test 'should reorder remaining root nodes when a root node becomes a child via prepend_child' do
node_1 = Label.create(name: 'node_1')
node_2 = Label.create(name: 'node_2')
node_3 = Label.create(name: 'node_3')
node_4 = Label.create(name: 'node_4')

# Verify initial positions
assert_equal 0, node_1.order_value
assert_equal 1, node_2.order_value
assert_equal 2, node_3.order_value
assert_equal 3, node_4.order_value

# Move node_2 as a child of node_3 using prepend_child
node_3.prepend_child(node_2)

# Reload all nodes to get updated positions
node_1.reload
node_2.reload
node_3.reload
node_4.reload

# Verify node_2 is now a child of node_3 with position 0
assert_equal node_3.id, node_2._ct_parent_id
assert_equal 0, node_2.order_value

# Verify remaining root nodes have sequential positions without gaps
assert_equal 0, node_1.order_value
assert_equal 1, node_3.order_value
assert_equal 2, node_4.order_value
end

test 'should reorder remaining root nodes when a root node becomes a child via add_sibling' do
node_1 = Label.create(name: 'node_1')
node_2 = Label.create(name: 'node_2')
node_3 = Label.create(name: 'node_3')
node_4 = Label.create(name: 'node_4')

# Create a child under node_3
child = Label.create(name: 'child', parent: node_3)

# Verify initial positions
assert_equal 0, node_1.order_value
assert_equal 1, node_2.order_value
assert_equal 2, node_3.order_value
assert_equal 3, node_4.order_value
assert_equal 0, child.order_value

# Move node_2 as a sibling of child (making it a child of node_3)
child.add_sibling(node_2)

# Reload all nodes to get updated positions
node_1.reload
node_2.reload
node_3.reload
node_4.reload

# Verify node_2 is now a child of node_3
assert_equal node_3.id, node_2._ct_parent_id

# Verify remaining root nodes have sequential positions without gaps
assert_equal 0, node_1.order_value
assert_equal 1, node_3.order_value
assert_equal 2, node_4.order_value
end
end