fix: DataMapper: Document node is a duplicate of the header#3135
Draft
matheusandre1 wants to merge 2 commits intoKaotoIO:mainfrom
Draft
fix: DataMapper: Document node is a duplicate of the header#3135matheusandre1 wants to merge 2 commits intoKaotoIO:mainfrom
matheusandre1 wants to merge 2 commits intoKaotoIO:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR eliminates duplicate document node rendering by skipping the root node when it's structured (schema-attached), adjusts child traversal to always display structured root's children, and recalibrates depth during traversal. Test assertions were tightened to validate node removal and button state during schema operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Member
|
This would rather have to be addressed in |
Contributor
Author
|
I will fix this. |
…ble node filtering in UI panels
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ui/src/models/datamapper/document-tree.ts (2)
52-78: Optional: hoistisStructuredout oftraverse.
isStructureddepends solely onthis.root.nodeData.isPrimitiveand is invariant for the whole flattening pass, but it is recomputed on every recursive call. Hoisting it (and derivingisRootonly where needed) makes the traversal a bit tighter and the structured-root special-case more obvious.♻️ Proposed refactor
flatten(expansionState: Record<string, boolean>, startDepth = 0): FlattenedNode[] { const result: FlattenedNode[] = []; + const isStructured = !this.root.nodeData.isPrimitive; const traverse = (node: DocumentTreeNode, depth: number) => { - const isRoot = node === this.root; - const isStructured = !this.root.nodeData.isPrimitive; - - // Always add current node to result, EXCEPT for the root node when it has a schema - if (!(isRoot && isStructured)) { + const isStructuredRoot = isStructured && node === this.root; + + // Skip the structured root; it is represented by the DocumentHeader. + if (!isStructuredRoot) { result.push({ treeNode: node, depth, index: result.length, path: node.path, }); } - // Only traverse children if: - // 1. Node has children - // 2. Node is expanded (checked via expansion state) - // OR Node is the structured root node (we always show its children) - const isExpanded = expansionState[node.path] ?? false; - const shouldTraverse = (isRoot && isStructured) || isExpanded; - - if (node.children.length > 0 && shouldTraverse) { - node.children.forEach((child) => { - traverse(child, isRoot && isStructured ? depth : depth + 1); - }); + // Children are shown when the node is expanded, or unconditionally for the + // structured root (so the schema's root field is always visible). + const shouldTraverse = isStructuredRoot || (expansionState[node.path] ?? false); + if (shouldTraverse && node.children.length > 0) { + const childDepth = isStructuredRoot ? depth : depth + 1; + node.children.forEach((child) => traverse(child, childDepth)); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/document-tree.ts` around lines 52 - 78, Hoist the invariant check this.root.nodeData.isPrimitive out of the recursive function: compute const isStructured = !this.root.nodeData.isPrimitive once before defining/traversing with traverse, remove its recomputation inside traverse, and keep deriving isRoot = node === this.root only where needed; update uses in the conditional that skips adding the structured root and in the depth calculation (the rest of traverse, expansionState, result, node.children.forEach, and shouldTraverse logic remain unchanged).
36-48: Stale JSDoc — update "always includes the root node".The contract in the docblock no longer matches the implementation: with a structured (schema-attached) root, the root is intentionally skipped and its children are always traversed regardless of expansion state. Please refresh the "Key behavior" bullets and the
startDepthnote (which now applies to the structured root's children when the root is skipped).📝 Proposed doc update
/** * Flattens the tree into an array of visible nodes based on expansion state * * Key behavior: - * - Always includes the root node - * - Only includes children if parent is expanded + * - Includes the root node only when it is primitive (no attached schema). + * A structured root is skipped and its children are always included at + * `startDepth`, effectively taking the root's visual slot. + * - For all other nodes, children are included only when the parent is expanded. * - Recursively checks expansion state down the tree * - Calculates correct depth for indentation * * `@param` expansionState - Record mapping node paths to their expansion state - * `@param` startDepth - Starting depth for the root node (default: 0) + * `@param` startDepth - Starting depth for the root (or for the structured + * root's children when the root is skipped). Default: 0. * `@returns` Array of flattened nodes with depth and index information */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/datamapper/document-tree.ts` around lines 36 - 48, Update the JSDoc on the tree-flattening function in document-tree.ts to reflect the current implementation: remove or change the "Always includes the root node" bullet to say that a structured (schema-attached) root is skipped and its children are traversed regardless of the root's expansion state, keep the bullet about only including children when the parent is expanded for non-structured nodes, and revise the startDepth note to clarify that when the root is skipped the provided startDepth applies to the structured root's children (i.e., the visible top-level nodes); also ensure the param names (expansionState, startDepth) and the "depth/indentation" behavior are accurate in the docblock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ui/src/models/datamapper/document-tree.ts`:
- Around line 52-78: Hoist the invariant check this.root.nodeData.isPrimitive
out of the recursive function: compute const isStructured =
!this.root.nodeData.isPrimitive once before defining/traversing with traverse,
remove its recomputation inside traverse, and keep deriving isRoot = node ===
this.root only where needed; update uses in the conditional that skips adding
the structured root and in the depth calculation (the rest of traverse,
expansionState, result, node.children.forEach, and shouldTraverse logic remain
unchanged).
- Around line 36-48: Update the JSDoc on the tree-flattening function in
document-tree.ts to reflect the current implementation: remove or change the
"Always includes the root node" bullet to say that a structured
(schema-attached) root is skipped and its children are traversed regardless of
the root's expansion state, keep the bullet about only including children when
the parent is expanded for non-structured nodes, and revise the startDepth note
to clarify that when the root is skipped the provided startDepth applies to the
structured root's children (i.e., the visible top-level nodes); also ensure the
param names (expansionState, startDepth) and the "depth/indentation" behavior
are accurate in the docblock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfc8d7e0-9f52-4c14-832f-54aa3163248d
📒 Files selected for processing (4)
packages/ui/src/components/View/SourcePanel.tsxpackages/ui/src/components/View/SourceTargetView.test.tsxpackages/ui/src/components/View/TargetPanel.tsxpackages/ui/src/models/datamapper/document-tree.ts
Member
|
I think there is more architecturally sound way, but currently I don't have time to look into this issue. I'll look into later. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #3132
I duplicated the code, could you advise me if it's correct or if in this case I need to create a utility for it?
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests