Tree Data Grid x Parent/Child Relation Dataset x Failures on subsequent component mount

[ghiscoding]

Discussed in #1655

^{Originally posted by tnaum-ms August 28, 2024}
I'm currently investigating an issue that arises when I mount my SlickGrid Tree Data as a React component for the second time. The problem occurs when I toggle between different data view formats in my application. Specifically, switching away from the Tree Data view and then returning to it results in a failure of the Tree Data Component.

The data set I’m working with follows a Parent/Child structure, where the id and parentId fields are of type string.
(It would be helpful to include a note in the documentation indicating the preferred or faster data input method.)

The SlickGrid Tree Data Grid throws the following error:

Each data element must implement a unique 'id' property

Upon investigation, I discovered that on the second mount (not just a second render), one of the internal data mutation functions introduces duplicates into my data set. This causes the ensureIdUniqueness function to fail.

The issue seems to be related to the function located here:

slickgrid-universal/packages/common/src/services/utilities.ts

Line 158 in ce8b400

export function unflattenParentChildArrayToTree<P, T extends P & { [childrenPropName: string]: P[]; }>(flatArray: P[], options?: { aggregators?: Aggregator[]; childrenPropName?: string; collapsedPropName?: string; identifierPropName?: string; levelPropName?: string; parentPropName?: string; initiallyCollapsed?: boolean; }): T[] {

This specific loop appears to be introducing duplicates:

slickgrid-universal/packages/common/src/services/utilities.ts

Lines 173 to 187 in ce8b400

	Object.keys(all).forEach((id) => {
	const item = all[id];
	if (!(parentPropName in item) || item[parentPropName] === null || item[parentPropName] === undefined || item[parentPropName] === '') {
	roots.push(item);
	} else if (item[parentPropName] in all) {
	const p = all[item[parentPropName]];
	if (!(childrenPropName in p)) {
	p[childrenPropName] = [];
	}
	p[childrenPropName].push(item);
	if (p[collapsedPropName] === undefined) {
	p[collapsedPropName] = options?.initiallyCollapsed ?? false;
	}
	}
	});

I'm continuing to investigate the root cause, but since this issue only occurs on the second mount, I haven’t yet identified any obvious side effects. If this is something you’ve encountered before, I would greatly appreciate any insights or suggestions you could share.

Thank you for your assistance.
Best, Tomasz

[jano-kucera]

Hi, I've hit upon this issue a long time ago, the children array didn't get reset when recreating the tree. Fixed it via this patch.

diff --git a/node_modules/@slickgrid-universal/common/dist/esm/services/utilities.js b/node_modules/@slickgrid-universal/common/dist/esm/services/utilities.js
index 2e9b2f9..d737958 100644
--- a/node_modules/@slickgrid-universal/common/dist/esm/services/utilities.js
+++ b/node_modules/@slickgrid-universal/common/dist/esm/services/utilities.js
@@ -140,7 +140,10 @@ export function unflattenParentChildArrayToTree(flatArray, options) {
     const roots = []; // items without parent which at the root
     // make them accessible by guid on this map
     const all = {};
-    inputArray.forEach((item) => all[item[identifierPropName]] = item);
+    inputArray.forEach((item) => {
+        all[item[identifierPropName]] = item;
+        delete item[childrenPropName];
+    });
     // connect childrens to its parent, and split roots apart
     Object.keys(all).forEach((id) => {
         const item = all[id];

[ghiscoding]

@jano-kucera I'm not sure why you're referencing this when it's already been fixed and pushed to production? If there's another problem or if this is a better fix than mine, then please contribute a fix as a new pull request so that it can benefit everyone. Thanks

Actually I wonder if the delete prop has better perf compare to the .findIndex() that I added in the latest fix I made, it's probably faster and if so then a PR would be nice. It might also be good to compare perf with console.time on a large dataset if possible.

slickgrid-universal/packages/common/src/services/utilities.ts

Lines 208 to 214 in 78653f9

	const existIdx = p[childrenPropName]?.findIndex((x: any) => x[identifierPropName] === item[identifierPropName]);
	if (existIdx >= 0) {
	// replace existing one when already exists (probably equal to the same item in the end)
	p[childrenPropName][existIdx] = item;
	} else {
	p[childrenPropName].push(item);
	}

cc @tnaum-ms

Also note that another user also found and provided another perf improvement related to Tree Data in this PR #1670 that I just merged today (to be released soon)

[jano-kucera]

From the code snippet, your fix does not handle removing children - that's why I commented :).
If I find time I will measure it, but I believe deleting a prop will always be faster than an index lookup.

[ghiscoding]

@jano-kucera then a Pull Request would be welcome to fix it, thanks

[jano-kucera]

Also I just realized a big problem in the implementation, you access the id field, not the identifierPropName - thus if you use any other property name for IDs (even from gridOptions), you end up children replacing each other.