fix: compute correct level when child precedes parent in tree rows#677
fix: compute correct level when child precedes parent in tree rows#677spike-rabbit wants to merge 1 commit intomainfrom
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
spike-rabbit
force-pushed
the
fix/group-rows-by-parents-level
branch
from
4a0badf to
d0674d9
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the groupRowsByParents utility to correctly calculate row levels regardless of their order in the input array and adds a comprehensive test suite. A critical issue was identified in the cycle detection logic where cyclic dependencies could prevent any root nodes from being identified, potentially resulting in an empty output; a revised implementation with proper backtracking was suggested to resolve this.
spike-rabbit
force-pushed
the
fix/group-rows-by-parents-level
branch
from
7793e60 to
ea0db93
Compare
There was a problem hiding this comment.
Pull request overview
Fixes tree-row level calculation in ngx-datatable when a child row appears before its parent, preventing NaN levels (and broken indentation) by resolving levels via memoized parent recursion and adding dedicated unit coverage.
Changes:
- Reworked
groupRowsByParentsto resolverow.levelvia recursive parent traversal with memoization and cycle detection. - Added
tree.spec.tsunit tests covering ordered/unordered trees, deep nesting, orphans, cycles, passthrough behavior, and collapsed-parent flattening.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| projects/ngx-datatable/src/lib/utils/tree.ts | Replaces single forward-pass level assignment with memoized recursive resolution + cycle breaking. |
| projects/ngx-datatable/src/lib/utils/tree.spec.ts | Adds unit tests to validate level computation and flattening behavior across edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously `groupRowsByParents` computed `row.level` in a single forward pass, so a child appearing before its parent in the input array got `undefined + 1 = NaN`. The cell template then rendered `margin-left: NaNpx` and the row had no tree indent. Resolve each node's level on first touch, recursing into the parent when needed. Each node is resolved at most once per invocation, so the algorithm stays O(n).
spike-rabbit
force-pushed
the
fix/group-rows-by-parents-level
branch
from
ea0db93 to
4bb3c18
Compare
What kind of change does this PR introduce? (check one with "x")
What is the current behavior?
groupRowsByParents(projects/ngx-datatable/src/lib/utils/tree.ts) assignslevel = parent.level + 1in a single forward pass. If a child precedes its parent in the input array,parent.levelisundefined, the child getsNaN, and the cell template rendersmargin-left: NaNpx— affected rows have no tree indent.What is the new behavior?
Levels are resolved on first touch with memoized recursion, so order in the input array no longer matters. The algorithm stays O(n); cycles are broken via a per-recursion
visitedset. Root order, child order, orphan and self-reference handling are preserved.Added
tree.spec.tswith unit tests covering ordered/unordered/deep trees, orphans, cycles, pass-through, and collapsed parents.Does this PR introduce a breaking change? (check one with "x")
Other information:
npm run lintandnpm run test:unitpass locally.