Complete Nested Set invariants, performance, and modern APIs - #460
Complete Nested Set invariants, performance, and modern APIs#460binaryfire wants to merge 11 commits into
Conversation
Record the approved Nested Set architecture before the implementation history. The plan defines structural ownership, scoped tree identity, stored depth, typed keys, relation behavior, repair boundaries, and worker-lifetime state. Capture the cross-driver index and eager-matching benchmarks, including their write, storage, and memory tradeoffs. Document the bounded design choices and the mechanisms deliberately rejected to keep the package scalable without speculative machinery. Specify the unit and MySQL, MariaDB, PostgreSQL, and SQLite validation matrix, plus the post-compaction and anti-overengineering rules used throughout the work.
Register the Nested Set provider through package discovery and expose Blueprint helpers for bigint, integer, UUID, and ULID node keys. Each helper creates stored depth and the scoped structural indexes needed by ancestor, descendant, child, and sibling reads, with symmetric removal support. Complete the split package dependencies and root discovery metadata so the package works both inside the monorepo and after subtree publication. Cache immutable HasNode trait membership by concrete class and give Testing one authoritative flush hook. The cache is bounded by loaded model classes, removes repeated recursive trait discovery, and retains no request data.
Exercise every Blueprint helper against the real schema builder, including parent column compatibility, mandatory depth, exact scoped index order, symmetric drops, and macro re-registration after framework state is flushed. Move the default fixtures to the canonical bigint schema and include depth in scoped fixtures so subsequent tree tests exercise the final storage contract. Prove the global test-state subscriber clears Nested Set's bounded class metadata alongside the framework's other static state.
Make connection-and-table structural freshness coroutine-local, normalize ordered scope identities, and separate structural queries from ordinary Eloquent visibility scopes. UUID, ULID, zero, empty-string, enum, date, and stringable keys now retain their exact tree identity. Maintain bounds, parentage, and stored depth together across insertion, movement, deletion, restoration, repair, and rebuild. Reject incomplete subtree repair sets, preserve post-gap snapshots, honor model vetoes, and expose truthful repair and diagnostic results. Add a real scope-aware siblings relation and correct nested existence aliases, joined column qualification, custom builders, collection linking, and recursive parent clones. Ancestor and descendant eager matching now uses bounded operation-local scope buckets and binary search where ordering permits, preserving caller order without retaining worker indexes. Keep set-based descendant deletion as the scalable default while providing protected evented deletion hooks with bounded keyset chunks. Remove stale soft-delete state, duplicate predicates, dead overrides, and obsolete structural assumptions.
Add deterministic coverage for logical connection and table freshness, custom builders, normalized compound scopes, UUID and scalar key boundaries, qualified structural reads, nested relation existence queries, sibling relations, and adaptive eager matching. Exercise stored depth and relation invalidation across every movement direction, partial selects, zero-distance operations, soft and force deletion, restoration, bulk and evented descendant paths, collection relinking, and recursive creation. Cover every named integrity category plus scoped diagnostics, repair and rebuild of roots, subtrees, orphans, cycles, vetoes, gap-shifted rows, and invalid repair boundaries. Correct inherited upstream tests so they call the real node APIs and assert the intended guards instead of passing on unrelated method failures.
Add driver-routed integration coverage for MySQL, MariaDB, PostgreSQL, and SQLite using the repository's existing database test infrastructure. Verify exact bigint, integer, UUID, and ULID parent storage; mandatory depth; scoped and unscoped index order; symmetric schema drops; and native UUID behavior where each database supports it. Exercise integer and UUID tree mutation, scope isolation, relations, soft deletion, repair, portable endpoint-window diagnostics, and persisted moved-subtree depth against real database grammars and assignment semantics.
Adopt the maintained Aimeos fork as the package reference and document typed schema helpers, mandatory stored depth, custom builders, sibling relations, scoped diagnostics, repair boundaries, and evented descendant deletion. Explain the transaction and application-serialization requirements for structural writes together with the measured read/write index tradeoff. Use generic menu scoping throughout and include one concise compound-scope example for a multi-tenant application with multiple menus per tenant without implying built-in tenancy support. Keep the guide focused on public Laravel-style usage while removing stale schema, depth, integrity, and performance guidance.
Add the durable Nested Set finding ledger with the final architecture, accepted defects, upstream test correction, rejected machinery, implementation result, measured performance tradeoffs, validation, and review outcome. Route the completed parallel work alongside the active Reverb audit without replacing any Reverb dependency. Record Nested Set's Testing cleanup dependency and completed Database revalidation while deliberately leaving the package checklist open for the owner's later full-audit decision.
📝 WalkthroughWalkthroughNested Set is modernized with stored depth, typed and scope-aware queries, Blueprint schema macros, sibling relations, iterative tree repair and traversal, lifecycle state cleanup, updated documentation, and database-driver integration coverage. ChangesNested Set modernization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant NodeModel
participant QueryBuilder
participant Database
NodeModel->>QueryBuilder: moveNode with position and target depth
QueryBuilder->>Database: patch affected bounds and depth
Database-->>QueryBuilder: updated structural rows
QueryBuilder-->>NodeModel: synchronized node state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR comprehensively redesigns Nested Set around synchronized bounds, parentage, depth, and scope invariants.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/nested-set/src/HasNode.php | Coordinates model-level structural actions, scope identity, deletion/restoration, depth maintenance, and freshness handling. |
| src/nested-set/src/Eloquent/QueryBuilder.php | Reworks scoped structural queries, mutations, integrity diagnostics, repair, and rebuild behavior. |
| src/nested-set/src/Eloquent/BaseRelation.php | Adds scope-correlated existence queries and operation-local eager-loading infrastructure. |
| src/nested-set/src/Eloquent/AncestorsRelation.php | Preserves exact tree scope across lazy, eager, and existence-query ancestor paths. |
| src/nested-set/src/Eloquent/SiblingsRelation.php | Implements scope-aware sibling loading, eager matching, and relationship existence queries. |
| src/nested-set/src/NestedSet.php | Defines typed schema helpers, stored-depth columns, structural indexes, and symmetric teardown. |
| src/nested-set/src/NodeContext.php | Tracks structural freshness by coroutine-local logical connection and table identity. |
| src/testing/src/PHPUnit/AfterEachTestSubscriber.php | Extends authoritative test-state cleanup to reset Nested Set static state. |
Reviews (4): Last reviewed commit: "fix(nested-set): guard partial structura..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php (1)
250-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove fixture models into a test-specific namespace.
IntegerNestedSetNodeandUuidNestedSetNodeare generic helpers in a shared namespace. Isolate them under a namespace ending inNestedSetDatabaseTestCaseand import them here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php` around lines 250 - 280, Move IntegerNestedSetNode and UuidNestedSetNode into a test-specific namespace ending in NestedSetDatabaseTestCase, then import both classes where they are used in the test fixture. Preserve their existing model configuration and behavior while removing reliance on the shared generic namespace.Source: Coding guidelines
src/nested-set/src/NestedSet.php (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the new string constants.
PHP 8.4 supports typed class constants; add
stringtoDEPTH,FIRST_TENANT, andSECOND_TENANT. Keep the existing untypedNestedSetcolumn-name constants as-is unless they are also being newly added, so this stays focused on introduced constants.Proposed fix
- public const DEPTH = 'depth'; + public const string DEPTH = 'depth'; - protected const FIRST_TENANT = '...'; - protected const SECOND_TENANT = '...'; + protected const string FIRST_TENANT = '...'; + protected const string SECOND_TENANT = '...';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/NestedSet.php` around lines 26 - 29, Type the newly introduced constants DEPTH, FIRST_TENANT, and SECOND_TENANT as string class constants. Update the declarations in src/nested-set/src/NestedSet.php (lines 26-29) and tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php (lines 18-20); leave existing untyped NestedSet column-name constants unchanged.Source: Coding guidelines
src/nested-set/src/Eloquent/QueryBuilder.php (1)
531-567: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the depth-before-bounds ordering requirement in
patch().
depthPatch()compares the storedlftagainst the moved subtree's original bounds, so on MySQL/MariaDB (which evaluateSETassignments left to right, using already-updated values) the depth entry must stay ahead of thelft/rgtentries in the returned array. Reordering the array later would silently corrupt depth on those drivers; a short note pins the invariant.♻️ Suggested comment
+ // MySQL evaluates SET assignments in order, so the depth patch must be + // built before lft/rgt: it compares lft against the pre-move bounds. if (($params['depth'] ?? 0) !== 0) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/Eloquent/QueryBuilder.php` around lines 531 - 567, Add a concise comment in patch() documenting that the depth column must remain before the lft and rgt updates because depthPatch() uses the original bounds and MySQL/MariaDB evaluate assignments left to right. Preserve the current ordering of the columns array and do not reorder the depthPatch(), lft, or rgt entries.src/nested-set/src/HasNode.php (1)
823-839: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the model’s date format when normalizing
DateTimeInterfacescopes.
getNestedSetScope()normalizes stored date attributes for both SQL equality and stable scope keys, so aninstanceof DateTimeInterfacevalue should usefromDateTime($value)instead of hardcodingY-m-d H:i:s, which bypasses$dateFormatand grammar-specific date formatting.♻️ Suggested change
- $value instanceof DateTimeInterface => $value->format('Y-m-d H:i:s'), + $value instanceof DateTimeInterface => $this->fromDateTime($value),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/HasNode.php` around lines 823 - 839, Update normalizeNestedSetScopeValue() so DateTimeInterface values are normalized through the model’s fromDateTime($value) method instead of a hardcoded format string, preserving the existing handling for all other value types.src/nested-set/src/Eloquent/BaseRelation.php (1)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a fresh model instance for the aliased existence query.
BaseRelation::getRelationExistenceQuery()only needs a table/connection-backed instance, butreplicate()copies attributes/relations and firesreplicatingbefore returning it. Use a new instance here to avoid the extra copy and observable side effect.♻️ Suggested change
- $query = $this->getParent()->replicate()->newQuery(); + $query = $this->getParent()->newInstance()->newQuery();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/Eloquent/BaseRelation.php` around lines 54 - 59, Update BaseRelation::getRelationExistenceQuery() to create the isolated aliased model with a fresh-instance approach instead of calling replicate(), while preserving the existing table/connection context and subsequent select($columns) behavior.tests/NestedSet/NodeTest.php (1)
2079-2137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove these helper models into a
NodeTest-scoped namespace.
CustomParentCategoryModel,EventedCategoryModel,StringKeyCategoryModel, andGloballyScopedCategoryModelare declared in the sharedHypervel\Tests\NestedSetnamespace used byNestedSetTestandScopedNodeTest, so these genericCategoryvariants are collision-prone.♻️ Suggested structure
namespace Hypervel\Tests\NestedSet\NodeTest; class CustomParentCategoryModel extends Model { /* ... */ }with matching
use Hypervel\Tests\NestedSet\NodeTest\CustomParentCategoryModel;imports in the test class.As per coding guidelines: "Use test-specific namespaces for collision-prone generic helper classes, with the test class name as the final namespace segment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/NestedSet/NodeTest.php` around lines 2079 - 2137, Move CustomParentCategoryModel, EventedCategoryModel, StringKeyCategoryModel, and GloballyScopedCategoryModel into the Hypervel\Tests\NestedSet\NodeTest namespace, and update the NodeTest class imports to reference those namespaced helpers. Leave the helper implementations unchanged and ensure other tests do not continue resolving these generic models from the shared Hypervel\Tests\NestedSet namespace.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md`:
- Line 1378: Update the nested-set-02 table entry to prevent the int|string|null
type from being interpreted as column separators, either by escaping each pipe
or rephrasing the type while preserving its meaning and the table’s six-column
structure.
---
Nitpick comments:
In `@src/nested-set/src/Eloquent/BaseRelation.php`:
- Around line 54-59: Update BaseRelation::getRelationExistenceQuery() to create
the isolated aliased model with a fresh-instance approach instead of calling
replicate(), while preserving the existing table/connection context and
subsequent select($columns) behavior.
In `@src/nested-set/src/Eloquent/QueryBuilder.php`:
- Around line 531-567: Add a concise comment in patch() documenting that the
depth column must remain before the lft and rgt updates because depthPatch()
uses the original bounds and MySQL/MariaDB evaluate assignments left to right.
Preserve the current ordering of the columns array and do not reorder the
depthPatch(), lft, or rgt entries.
In `@src/nested-set/src/HasNode.php`:
- Around line 823-839: Update normalizeNestedSetScopeValue() so
DateTimeInterface values are normalized through the model’s fromDateTime($value)
method instead of a hardcoded format string, preserving the existing handling
for all other value types.
In `@src/nested-set/src/NestedSet.php`:
- Around line 26-29: Type the newly introduced constants DEPTH, FIRST_TENANT,
and SECOND_TENANT as string class constants. Update the declarations in
src/nested-set/src/NestedSet.php (lines 26-29) and
tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php (lines
18-20); leave existing untyped NestedSet column-name constants unchanged.
In `@tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php`:
- Around line 250-280: Move IntegerNestedSetNode and UuidNestedSetNode into a
test-specific namespace ending in NestedSetDatabaseTestCase, then import both
classes where they are used in the test fixture. Preserve their existing model
configuration and behavior while removing reliance on the shared generic
namespace.
In `@tests/NestedSet/NodeTest.php`:
- Around line 2079-2137: Move CustomParentCategoryModel, EventedCategoryModel,
StringKeyCategoryModel, and GloballyScopedCategoryModel into the
Hypervel\Tests\NestedSet\NodeTest namespace, and update the NodeTest class
imports to reference those namespaced helpers. Leave the helper implementations
unchanged and ensure other tests do not continue resolving these generic models
from the shared Hypervel\Tests\NestedSet namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff9fcf07-f9bb-4729-ac6d-01c61eb8b74e
📒 Files selected for processing (33)
composer.jsondocs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.mddocs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.mdsrc/boost/docs/nested-set.mdsrc/nested-set/README.mdsrc/nested-set/composer.jsonsrc/nested-set/src/Eloquent/AncestorsRelation.phpsrc/nested-set/src/Eloquent/BaseRelation.phpsrc/nested-set/src/Eloquent/Collection.phpsrc/nested-set/src/Eloquent/DescendantsRelation.phpsrc/nested-set/src/Eloquent/QueryBuilder.phpsrc/nested-set/src/Eloquent/SiblingsRelation.phpsrc/nested-set/src/HasNode.phpsrc/nested-set/src/NestedSet.phpsrc/nested-set/src/NestedSetServiceProvider.phpsrc/nested-set/src/NodeContext.phpsrc/testing/src/PHPUnit/AfterEachTestSubscriber.phptests/Integration/NestedSet/Database/MariaDb/NestedSetDatabaseTest.phptests/Integration/NestedSet/Database/MySql/NestedSetDatabaseTest.phptests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.phptests/Integration/NestedSet/Database/Postgres/NestedSetDatabaseTest.phptests/Integration/NestedSet/Database/Sqlite/NestedSetDatabaseTest.phptests/NestedSet/Models/Category.phptests/NestedSet/Models/MenuItem.phptests/NestedSet/NestedSetSchemaTest.phptests/NestedSet/NestedSetTest.phptests/NestedSet/NodeContextTest.phptests/NestedSet/NodeTest.phptests/NestedSet/ScopedNodeTest.phptests/NestedSet/migrations/2025_07_02_000000_create_categories_table.phptests/NestedSet/migrations/2025_07_03_000000_create_menu_items_table.phptests/Testing/PHPUnit/AfterEachTestSubscriberTest.php
💤 Files with no reviewable changes (1)
- tests/NestedSet/Models/Category.php
Honor model-specific date formats when normalizing nested-set scope values without resolving a database connection for each eager result. Build relation existence aliases from a class-default model on the parent connection. This keeps nested aliases out of FROM clauses, avoids copying model state, and prevents read queries from dispatching replication events. Add load-bearing regressions for both paths, document the MySQL and MariaDB assignment-order invariant, type the related constants, and synchronize the implementation plan and audit ledger with the final behavior.
|
@coderabbitai Follow-up complete in The useful findings are addressed:
The suggested implementations needed two adjustments. I did not add test-specific namespaces for already-specific, non-colliding helpers, or blanket docblocks to test methods and closures. Both would add ceremony without correcting behavior. The focused suites and the full |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! This chat is LLM-generated and may be inaccurate. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/nested-set/src/HasNode.php (1)
431-435: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate targets as active nodes in the same tree.
These paths check only positive bounds and scope. A soft-deleted node, a node from another connection/tree, or an unsaved model with copied bounds can pass validation and then mutate the current tree using the target’s bounds and parent ID. Mirror the identity and
withoutTrashed()validation used byactionAppendToParentId().Also applies to: 461-465, 1344-1358
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/HasNode.php` around lines 431 - 435, Update appendOrPrependTo and the additional affected target-validation paths to validate the target as an active, persisted node in the same tree, not merely by bounds and scope. Mirror the identity checks and withoutTrashed() behavior used by actionAppendToParentId(), rejecting soft-deleted, unsaved, or cross-connection/tree targets before any mutation.tests/NestedSet/NodeTest.php (2)
1831-1862: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winInclude structural columns in the rebuild rollback snapshot.
This test compares only
name, so a veto could leave_lft,_rgt,parent_id, ordepthpartially persisted while the test still passes. Snapshot and compare the same structural columns used by thefixTreerollback test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/NestedSet/NodeTest.php` around lines 1831 - 1862, Update the rollback snapshot assertions in the rebuild veto test around Category::rebuildTree to include the structural columns used by the fixTree rollback test—_lft, _rgt, parent_id, and depth—in addition to name. Ensure both the pre-rebuild and post-veto queries select and compare the same complete column set.
649-676: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the distinct deletion timestamps in the re-entrant restore regression.
The test creates three timestamps but only checks which nodes are restored. A regression that uses the wrong timestamp could still satisfy these assertions. Capture the timestamp observed during restoration, or assert the timestamp-specific descendant selection directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/NestedSet/NodeTest.php` around lines 649 - 676, Strengthen testReentrantRestoreUsesEachNodesExactPreviousDeletionTimestamp by recording each node’s deletion timestamp or asserting timestamp-specific descendant selection during the restoring callbacks. Verify that re-entrant restoration uses the distinct timestamps from 12:00:00, 12:00:01, and 12:00:02, while preserving the existing restored and remaining-deleted node assertions.
🧹 Nitpick comments (1)
tests/NestedSet/NodeTest.php (1)
7-91: 📐 Maintainability & Code Quality | 🔵 TrivialRun the repository-required validation gates for this test change.
Run the changed test file, focused Nested Set tests, PHP-CS-Fixer, the full parallel suite, and Testbench’s suite where applicable.
As per coding guidelines,
**/*changes require changed tests, focused tests, PHPStan for source changes, PHP-CS-Fixer, the full parallel suite, and Testbench’s suite after Testbench changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/NestedSet/NodeTest.php` around lines 7 - 91, Run the repository-required validation for the NodeTest changes: execute tests/NestedSet/NodeTest.php, the focused Nested Set test set, PHP-CS-Fixer, the full parallel test suite, and Testbench’s suite where applicable; also run PHPStan if source changes are included. Address any failures before finalizing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/NestedSet/NodeTest.php`:
- Around line 2006-2014: Update the test around the descendants relation
existence query to assign the parent a distinct configured non-default
connection alias instead of DB::getDefaultConnection(). Keep the existing query
construction and assert that query->getModel()->getConnectionName() matches that
alias, verifying the parent connection is preserved.
---
Outside diff comments:
In `@src/nested-set/src/HasNode.php`:
- Around line 431-435: Update appendOrPrependTo and the additional affected
target-validation paths to validate the target as an active, persisted node in
the same tree, not merely by bounds and scope. Mirror the identity checks and
withoutTrashed() behavior used by actionAppendToParentId(), rejecting
soft-deleted, unsaved, or cross-connection/tree targets before any mutation.
In `@tests/NestedSet/NodeTest.php`:
- Around line 1831-1862: Update the rollback snapshot assertions in the rebuild
veto test around Category::rebuildTree to include the structural columns used by
the fixTree rollback test—_lft, _rgt, parent_id, and depth—in addition to name.
Ensure both the pre-rebuild and post-veto queries select and compare the same
complete column set.
- Around line 649-676: Strengthen
testReentrantRestoreUsesEachNodesExactPreviousDeletionTimestamp by recording
each node’s deletion timestamp or asserting timestamp-specific descendant
selection during the restoring callbacks. Verify that re-entrant restoration
uses the distinct timestamps from 12:00:00, 12:00:01, and 12:00:02, while
preserving the existing restored and remaining-deleted node assertions.
---
Nitpick comments:
In `@tests/NestedSet/NodeTest.php`:
- Around line 7-91: Run the repository-required validation for the NodeTest
changes: execute tests/NestedSet/NodeTest.php, the focused Nested Set test set,
PHP-CS-Fixer, the full parallel test suite, and Testbench’s suite where
applicable; also run PHPStan if source changes are included. Address any
failures before finalizing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b609055d-5359-476c-baf6-01178684288e
📒 Files selected for processing (9)
docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.mdsrc/nested-set/src/Eloquent/BaseRelation.phpsrc/nested-set/src/Eloquent/QueryBuilder.phpsrc/nested-set/src/HasNode.phpsrc/nested-set/src/NestedSet.phptests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.phptests/NestedSet/NestedSetTest.phptests/NestedSet/NodeTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
- docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md
- tests/NestedSet/NestedSetTest.php
- src/nested-set/src/NestedSet.php
- src/nested-set/src/Eloquent/QueryBuilder.php
Structural model-target mutations previously verified only the nested-set scope, allowing coordinates loaded from another database connection or table to drive writes against the source tree. Replace that guard with the existing full tree identity so append, prepend, before, after, and deferred parent actions require the same resolved connection, table, and scope without adding a query or retained state. Strengthen the surrounding regressions at the same time: cover every direct mutation entry point, pin the canonical cross-scope error, preserve both exact restore cutoffs, prove a rebuild veto rolls back a completed parent change, and make relation connection inheritance use a distinct alias. Update the public guide and implementation plan to state the complete boundary.
|
@coderabbitai Follow-up complete in c675792. The connection-preservation regression now uses and asserts a distinct configured alias. The outside-diff findings were handled as follows:
The changed test files, focused Nested Set groups, formatting, static analysis, full parallel suite, and package-mode checks are green. |
|
Tip For best results, initiate chat on the files or code changes.
The strengthened rollback and re-entrant restore regressions also make the transactional and timestamp-isolation guarantees explicit. Noted for future Nested Set reviews. 🐇 ✏️ Learnings added
This chat is LLM-generated and may be inaccurate. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/nested-set/src/HasNode.php (1)
208-214: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve a missing parent depth instead of coercing it to
1.
getDepth()returns?int; when it isnull,$parent->getDepth() + 1becomes1, soinsertAt()sets the wrong depth instead of falling back to the position-based depth.Proposed fix
- $targetDepth = $parent->getDepth() + 1; + $parentDepth = $parent->getDepth(); + $targetDepth = $parentDepth === null ? null : $parentDepth + 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/nested-set/src/HasNode.php` around lines 208 - 214, Update actionAppendOrPrepend so a null result from parent->getDepth() remains null instead of being coerced through addition; only increment non-null depths, allowing insertAt() to use its position-based depth fallback when the parent depth is missing.Source: MCP tools
tests/NestedSet/ScopedNodeTest.php (1)
425-434: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest name doesn't match the method it exercises.
testInsertingBeforeAnotherScopeFailscallsinsertAfterNode, notinsertBeforeNode. The cross-scope rejection ofinsertBeforeNodeitself is left untested under this name, and the misleading name will confuse future maintainers.🔧 Proposed fix
- $foo->insertAfterNode($bar); + $foo->insertBeforeNode($bar);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/NestedSet/ScopedNodeTest.php` around lines 425 - 434, Rename testInsertingBeforeAnotherScopeFails to reflect that it exercises insertAfterNode, preserving its existing cross-scope exception assertions and setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/nested-set/src/HasNode.php`:
- Around line 208-214: Update actionAppendOrPrepend so a null result from
parent->getDepth() remains null instead of being coerced through addition; only
increment non-null depths, allowing insertAt() to use its position-based depth
fallback when the parent depth is missing.
In `@tests/NestedSet/ScopedNodeTest.php`:
- Around line 425-434: Rename testInsertingBeforeAnotherScopeFails to reflect
that it exercises insertAfterNode, preserving its existing cross-scope exception
assertions and setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64402bd1-4e4f-4cbe-9442-254808dc3e41
📒 Files selected for processing (5)
docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.mdsrc/boost/docs/nested-set.mdsrc/nested-set/src/HasNode.phptests/NestedSet/NodeTest.phptests/NestedSet/ScopedNodeTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- src/boost/docs/nested-set.md
- docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md
Reload persisted nodes when structural columns were omitted so move, force-delete, and soft-delete cascades cannot calculate intervals from null state. Preserve an omitted parent depth for hand-positioned unsaved targets and let the indexed position lookup derive the correct stored depth. Reject incomplete low-level movement data and require subtree repair or rebuild roots to be persisted, keyed, scope-complete, and loaded with bounds and depth before any structural write. Keep repair and rebuild diagnostics distinct and name missing scope attributes. Add regressions for partial sources and targets, present-null builder data, soft and force deletion, incomplete repair and rebuild roots, unpersisted and keyless roots, scope projection, and the inherited insertion-before fixture. Update the implementation plan and audit ledger to record the completed boundaries without adding a duplicate finding.
|
@coderabbitai Both outside-diff findings are fixed in b2355d7. The append/prepend path now preserves a missing parent depth so the builder can derive it from the stored position. The related partial-projection paths now reload persisted structural state before move, soft delete, or force delete, while direct low-level node data rejects absent and null structural values. The misleading scope regression now exercises The same trace also tightened repair and rebuild: caller-supplied roots must be persisted, keyed, structurally complete, and concretely scoped before any database read or write. The regressions cover each failure shape and prove the table remains unchanged after refusal. Formatting, static analysis, focused Nested Set coverage, database integration coverage, and the full suite are green. |
|
Tip For best results, initiate chat on the files or code changes.
The strengthened repair/rebuild preconditions and unchanged-table regressions also make the fail-fast boundary clear. Noted for future Nested Set reviews. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md (1)
922-934: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the validation checklist.
This section lists focused PHPUnit groups and
composer fix, but omits the required full parallel suite and Testbench suite when applicable. Add PHPStan and PHP-CS-Fixer for source changes as well, and explicitly preserve the “changed tests immediately” step.As per coding guidelines, repository validation must include these checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md` around lines 922 - 934, Update the validation checklist to preserve running changed tests immediately, then add the full parallel PHPUnit suite and the Testbench suite when applicable. For source changes, also require PHPStan and PHP-CS-Fixer, while retaining the focused NestedSet groups, composer fix, driver integration checks, and final diff review.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md`:
- Around line 922-934: Update the validation checklist to preserve running
changed tests immediately, then add the full parallel PHPUnit suite and the
Testbench suite when applicable. For source changes, also require PHPStan and
PHP-CS-Fixer, while retaining the focused NestedSet groups, composer fix, driver
integration checks, and final diff review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6811d066-d573-4605-8265-0b3729e45873
📒 Files selected for processing (6)
docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.mdsrc/nested-set/src/Eloquent/QueryBuilder.phpsrc/nested-set/src/HasNode.phptests/NestedSet/NodeTest.phptests/NestedSet/ScopedNodeTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/plans/2026-07-12-0915-framework-coroutine-state-lifecycle-audit-ledger.md
1 participant
Summary
This completes the Nested Set package redesign around one database invariant: bounds, parentage, depth, and scope must agree.
For more details, see docs/plans/2026-07-29-1711-nested-set-invariants-performance-and-modern-api.md
Why
The existing package assumed integer keys, derived depth through correlated queries, mixed structural operations with ordinary Eloquent visibility scopes, and used relation matching that became expensive on large or heavily scoped result sets. Repair and rebuild could also report success while leaving invalid coordinates or failing to persist rows shifted by a gap update.
Those problems compound in long-running workers and high-traffic applications. The package now has explicit owners for each kind of state:
No request state or tree index is retained for the worker lifetime.
Schema and API
The schema helpers now create
_lft,_rgt,parent_id, anddepthtogether. Scope columns prefix three indexes:Unscoped trees omit the prefix. Existing tables need a migration that adds
depthand the final index layout before deploying this version.The public surface adds:
nestedSet,integerNestedSet,uuidNestedSet,ulidNestedSet, anddropNestedSetBlueprint methods;siblings()andsiblingsAndSelf()as full Eloquent relations;Compound scopes are supported. For example, an application may partition menu trees by both
tenant_idandmenu_id; tenant resolution and access control remain application concerns.Correctness
Structural queries now ignore visibility filters while retaining Eloquent builder extensions such as SoftDeletes. Every structural mutation publishes freshness before changing an interval and maintains depth in the same operation as its bounds.
Repair and rebuild:
Integrity checks use one portable endpoint-window query instead of a pairwise crossing join or a whole-tree PHP scan.
Performance
The eager matcher was benchmarked against both the former Hypervel implementation and the maintained Aimeos fork. Large multi-parent matches moved from hundreds or thousands of milliseconds to tens of milliseconds. The final operation-local design retained about 6 MiB in the large cases, compared with roughly 37–45 MiB for Aimeos's global index.
The database indexes materially improve scoped reads on MySQL, MariaDB, PostgreSQL, and SQLite. They add a measured structural-write cost on MySQL and PostgreSQL; MariaDB and SQLite were neutral or faster in the representative gap update. This is an intentional tradeoff for the read-heavy workload nested sets serve.
Stored depth adds a small write and storage cost while eliminating repeated correlated depth queries. No lock, retry loop, schema introspection, hidden network call, or worker-retained tree index was added.
Concurrency
A structural mutation spans multiple SQL statements. Applications must wrap each mutation in a database transaction and serialize concurrent writers targeting the same connection, table, and scope.
The package does not add an implicit distributed lock. This keeps lock ownership and failure policy with the application instead of hiding network work inside model operations.
Validation
Summary by CodeRabbit
depthsupport and refreshed schema macros with optional scoping plus typed key variants (integer/UUID/ULID).