feat: page tree supports dragging #948
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces several enhancements to the page management system in the Vue application. The changes primarily focus on improving the tree structure, search functionality, and visual representation of pages. A new Changes
Sequence DiagramsequenceDiagram
participant User
participant PageTree
participant Tree
participant LayerLines
User->>PageTree: Interact with page tree
PageTree->>Tree: Manage node interactions
Tree-->>PageTree: Update tree structure
Tree->>LayerLines: Render visual representation
LayerLines-->>Tree: Display lines based on data
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
packages/plugins/page/src/LayerLines.vue (3)
1-9: Consider improving maintainability and accessibility of the template.Several improvements could enhance the code:
- Extract hardcoded values into CSS variables or component props
- Move bitwise operations to computed properties
- Add ARIA attributes for accessibility
Consider applying these changes:
<template> <span v-for="i in level - 1" :key="i" class="gap"> - <svg v-if="lineData[i]" width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"> + <svg v-if="lineData[i]" + :width="size" + :height="size" + :viewBox="`0 0 ${size} ${size}`" + xmlns="http://www.w3.org/2000/svg" + role="img" + aria-label="Tree connection line"> <line x1="8" y1="0" x2="8" y2="12" stroke="#EBEBEB" stroke-width="1" /> - <line v-if="(lineData[i] & 1) === 1" x1="8" y1="12" x2="20" y2="12" stroke="#EBEBEB" stroke-width="1" /> - <line v-if="((lineData[i] >> 1) & 1) === 1" x1="8" y1="12" x2="8" y2="24" stroke="#EBEBEB" stroke-width="1" /> + <line v-if="hasHorizontalLine(i)" x1="8" y1="12" x2="20" y2="12" :stroke="lineColor" stroke-width="1" /> + <line v-if="hasVerticalLine(i)" x1="8" y1="12" x2="8" y2="24" :stroke="lineColor" stroke-width="1" /> </svg> </span> </template>Add these properties and methods to the script section:
const props = defineProps({ // ... existing props ... size: { type: Number, default: 24 }, lineColor: { type: String, default: '#EBEBEB' } }) const hasHorizontalLine = (index) => (props.lineData[index] & 1) === 1 const hasVerticalLine = (index) => ((props.lineData[index] >> 1) & 1) === 1
26-31: Make styles more maintainable and responsive.The gap dimensions should be consistent with the SVG size and potentially responsive:
Consider applying these changes:
<style scoped> .gap { - width: 24px; - height: 24px; + width: v-bind('size + "px"'); + height: v-bind('size + "px"'); + display: inline-block; } </style>
1-31: Consider architectural improvements for better reusability.As this component is part of a tree visualization feature:
- Consider extracting it to a shared/common components directory for reuse in other tree visualizations
- Add JSDoc comments explaining the bitwise operations in lineData
- Consider adding unit tests for the line rendering logic
Would you like me to help with:
- Moving this component to a shared location?
- Generating documentation for the bitwise operations?
- Creating unit tests for the component?
packages/plugins/page/src/Tree.vue (3)
7-11: Add drag event handling checks for improved stability.
While the drag-and-drop setup is correct, it's good practice to verify that the node is valid before setting draggable attributes. Also consider adding checks for event.dataTransfer support or restricting dragstart if necessary.+ @dragstart="(event) => node && handleDragStart(event, node)"
157-163: Check for multi-drag scenarios.
draggedNode is stored in a single ref, which may lead to unexpected behavior if multiple drags happen simultaneously in different parts of the UI. While less common, ensure the UX is guaranteed to limit one drag at a time or handle concurrency gracefully.
239-241: Be cautious with hover border rendering.
This border may visually shift elements, causing layout changes. Consider using an outline or a subtle background highlight for improved user experience.-&.hover { - border: 1px solid var(--te-common-border-checked); +&.hover { + outline: 1px solid var(--te-common-border-checked); }packages/plugins/page/src/PageTree.vue (4)
16-16: Add logic for lock and home icons.
The TODO comment highlights missing functionality for page/file lock and home indicator icons. Consider including these icons to align with your design specifications.Would you like help adding the logic for these icons? I can open a new GitHub issue and propose an implementation.
106-106: Retrieving page list from the server.
The addition of "getPageList" supports the dynamic update of tree data. Verify caching or debouncing if server calls are frequent.
281-281: Popover closure for improved accessibility.
Calling "popoverRefs[node.id]?.doClose?.()" is a standard approach to closing the popover. Consider reviewing ARIA attributes or adding a minor accessibility enhancement to avoid console warnings.
296-310: Review of updatePage function.
- Merges
page_contentand sets thefileNamebefore sending the update.- Calls
handlePageUpdatewith an option to refresh the current page if it matches the updated page ID.- The usage is consistent with your page update flow, but consider wrapping the promise in a try/catch if you need custom error messages or additional logging in calling functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/plugins/page/src/LayerLines.vue(1 hunks)packages/plugins/page/src/PageGeneral.vue(0 hunks)packages/plugins/page/src/PageTree.vue(7 hunks)packages/plugins/page/src/Tree.vue(6 hunks)packages/plugins/page/src/composable/usePage.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/plugins/page/src/PageGeneral.vue
🔇 Additional comments (14)
packages/plugins/page/src/Tree.vue (4)
14-14: Validate the layerLine indexing to avoid out-of-bounds errors.
Ensure that layerLine[rowIndex] is always defined by verifying that layerLine computed property produces an entry for every displayed node. In some edge cases—e.g., dynamic modifications—this mapping could fail and cause an undefined reference.
127-150: Validate hierarchical line calculations for edge-case scenarios.
The code constructing layer lines is relatively intricate. Edge cases like single-child trees or deep nesting might produce unexpected line rendering. Consider unit tests or verifying visual outcomes, particularly when the node's parent is not found in the immediate set.
3-5: Avoid potential index conflicts when using rowIndex as a key.
Currently, node.id is used as the key, but rowIndex is used to reference certain arrays (like layerLine). If node order changes or new nodes are added, manually referencing rowIndex may cause mismatch in scenarios where filtering or reordering modifies the data order. Consider ensuring the layerLine array always stays in sync with node order or consider a more robust way to map nodes to line data.
50-56: Props accept multiple types; ensure consistent usage.
The props “filterValue” and “rootId” are typed as [String, Object]/[String, Number], which is flexible but can introduce unexpected behaviors if not handled carefully. Validate that all code branches properly handle each potential type.
✅ Verification successful
Let me gather more information about how these props are used in the component, particularly focusing on type handling.
Let me analyze how these props are used in the component. From the search results, I can see:
filterValue(String | Object) is used in a computed property to filter nodes based on label:
return nodes.value.filter((node) => node.label.toLowerCase().includes(props.filterValue))rootId(String | Number) is used in:
- Initial data flattening:
flattenTreeData({ [props.idKey]: props.rootId, [props.childrenKey]: props.data }) - Parent node check:
if (node.parentId !== props.rootId)
The code handles both props appropriately:
filterValueis used withincludes()which works with string conversionrootIdis used in property access and equality comparison which works with both strings and numbers
Props are handled safely for all declared types.
- The
filterValueprop is used in string operations where type coercion ensures safe handling of both String and Object types - The
rootIdprop is used in comparisons and as an object key where both String and Number types are valid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching usage of filterValue and rootId to ensure consistent type checks
rg -A 3 '(filterValue|rootId)'
Length of output: 1400
Script:
#!/bin/bash
# Search for computed properties and methods that might handle these props
ast-grep --pattern 'computed(() => {
$$$
})'
# Also check for watch handlers
ast-grep --pattern 'watch($$$)'
# And check for any type assertions or checks
rg -A 2 'typeof|instanceof'
Length of output: 65949
packages/plugins/page/src/PageTree.vue (10)
3-3: Usage of v-model on search input is consistent with Vue best practices.
The binding to "state.pageSearchValue" with clearable functionality is straightforward and improves usability.
24-24: Drag-and-drop event handling seems well-integrated.
The new "@move-node" event references "handleMoveNode," which orchestrates node restructuring. Verify consistent handling of all drag events in the Tree.vue or any relevant child components.
110-110: fetchPageDetail and requestUpdatePage destructured from http.
It’s good that you separated out these HTTP methods to keep the code modular. Please ensure that these methods encapsulate robust error handling.
311-330: Review of updateFolder function.
- Makes a direct
requestUpdatePagecall, only handling partial data sincepage_contentis null. - Includes success and failure notifications to enhance user feedback.
- On success, triggers
pageSettingState.updateTreeData()to refresh the UI. This is a good pattern.
394-394: Exposing handleMoveNode from setup.
The method is returned from the setup, enabling the parent or sibling components to handle its logic. Ensure that the parent has no conflicting watchers that might override the node's movement logic.
66-66: Notify and Message utilities.
Importing "useNotify" and "useMessage" is a good approach for standardized user feedback. Confirm that these message hooks gracefully handle concurrent notifications.
✅ Verification successful
Based on the search results, I can provide a final response since I have enough information about how useNotify and useMessage are used across the codebase:
Concurrent notifications are properly handled in the codebase
The implementation shows that:
- useNotify is used for one-off notifications that don't overlap or conflict
- useMessage implements a proper pub/sub pattern with:
- Unique topic/subscriber identifiers
- Clean unsubscription on component unmount
- Proper message queue handling through subscribe/unsubscribe lifecycle
The notifications are used appropriately across the codebase with:
- Clear message types (success, error, warning, info)
- Proper error handling and cleanup
- No race conditions in notification display
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Searching for concurrent usage of useNotify or useMessage across the codebase
rg -A 5 'useNotify\(|useMessage\('
Length of output: 35283
21-22: Filtering and root ID binding are appropriate.
The usage of ":filter-value" and ":root-id" binds well to your tree data. Ensure that the draggble-tree component uses these values properly for root-level node identification and search filtering.
✅ Verification successful
Props are correctly used in the draggable tree component
The verification confirms that both filterValue and rootId props are properly implemented and utilized in the Tree component:
filterValueis used to filter nodes based on label text in thefilteredNodescomputed propertyrootIdis used as expected for:- Identifying root-level nodes in the tree structure
- Proper parent-child relationship handling during tree traversal
- Correct rendering of the tree hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify usage of filterValue and rootId in DraggbleTree
rg -A 6 'props:\s*{\s*filterValue:\s*{\s*type:\s*String' packages/plugins/page/src/Tree.vue
rg -A 6 'props:\s*{\s*rootId:\s*{\s*type:\s*Number' packages/plugins/page/src/Tree.vue
Length of output: 455
Script:
#!/bin/bash
# Let's fix the regex and search for prop definitions
rg -A 6 "props.*filterValue" packages/plugins/page/src/Tree.vue
rg -A 6 "props.*rootId" packages/plugins/page/src/Tree.vue
# Also search for the component name and its usage
fd Tree.vue --exec cat {} \; packages/plugins/page/src/
Length of output: 20753
73-73: Importing handlePageUpdate for remote update calls.
Ensure that all calls to "handlePageUpdate" are properly wrapped with error handling or promise chaining if not consumed by a top-level await.
117-117: Initializing currentNodeData with the pageId.
Binding directly to GlobalService is convenient for referencing the currently open page. Confirm if there's a fallback or default behavior when pageId is undefined.
332-361: handleMoveNode logic for post-drag operations.
- Checks whether current data is saved; if yes, we finalize the move by updating the node’s parent.
- Fetches new page detail and updates with the new parent ID, then decides whether to call
updatePageorupdateFolder. - The fallback behavior discards unsaved changes if the user confirms a prompt.
• Potential Issue: If changes are partially saved in the drag chain, concurrency conflicts might arise.
Would you like to ensure concurrency safety? We can run a deeper codebase script to see if there are any concurrency safeguards during drag and drop operations.
✅ Verification successful
Let me search for more specific drag and drop related code to analyze concurrency handling.
Based on the code analysis, I can now provide a final response about the concurrency safety of the drag and drop operations:
Drag and drop operations have sufficient safeguards against concurrency issues.
The implementation shows several safety mechanisms:
- Single source of truth: The
draggedNodestate is cleared after each drop operation - Sequential execution: The drag chain follows a clear sequence:
handleDragStartsets the dragged nodehandleDropclears state and validates before proceedingchangeTreeDataupdates the tree structurefetchPageDetailgets fresh data before updatingupdatePage/updateFolderperforms the final server update
- Unsaved changes protection: The code checks for unsaved changes and prompts user confirmation before proceeding
- Ancestor validation: Prevents invalid drops by checking if target is a descendant of dragged node
- Atomic updates: The server-side updates are performed as single operations per page/folder
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for concurrency or race condition handling in drag operations
rg -A 5 'dragstart|dragend|drop|handleMoveNode'
Length of output: 78894
Script:
#!/bin/bash
# Search for drag and drop related code and event handlers
ast-grep --pattern 'handleMoveNode($$$)'
rg -A 10 'handleDragStart|handleDragEnd|handleDrop|changeTreeData|updatePage|updateFolder'
Length of output: 24002
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
LayerLines, for visual representation of lines based on provided properties.PageTreecomponent with new methods for updating pages and folders, and improved drag-and-drop functionality.Treecomponent for better node management.Bug Fixes
PageGeneral.Improvements
usePage.PageTree.