Release319 fix(transfer): fix drag 2 item to right when set filterable=true #2517
Conversation
WalkthroughThe pull request includes an update to the version number of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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
🧹 Outside diff range and nitpick comments (1)
packages/renderless/src/transfer/index.ts (1)
220-233: Consider decoupling the renderless logic from Vue component structureThe current implementation creates tight coupling between the renderless logic and Vue component structure by directly accessing component refs. Consider refactoring to pass filtered data as parameters to maintain the renderless nature of the component.
Example approach:
interface LogicFunParams { leftFilteredData: any[]; rightFilteredData: any[]; // ... other params } const logicFun = ({ props, emit, state, leftFilteredData, rightFilteredData }: LogicFunParams) => { // Use passed filtered data instead of accessing through refs }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/renderless/package.json(1 hunks)packages/renderless/src/transfer/index.ts(1 hunks)packages/renderless/src/transfer/vue.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/renderless/package.json
🔇 Additional comments (2)
packages/renderless/src/transfer/vue.ts (2)
83-83: LGTM: Consistent parameter passing
The update to include vm in the logicFun call aligns with the function signature changes and supports the PR's objective of fixing drag functionality with filterable enabled.
64-64: Verify the impact of adding the vm parameter
The addition of the vm parameter to the ITransferRenderlessParamUtils interface could potentially be a breaking change.
✅ Verification successful
Based on the results, I can now provide the final response:
The vm parameter addition is safe and follows existing patterns
The vm parameter is already part of the ISharedRenderlessParamUtils interface which is used by multiple renderless components (tree-menu, tooltip, popover, etc.). This change aligns with the existing codebase patterns and is not a breaking change since:
- The type
ITransferRenderlessParamUtilsinherits fromISharedRenderlessParamUtils - Other renderless components already use the
vmparameter in the same way - The implementation is consistent with the type definitions in
transfer.type.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing implementations that might be affected
# Search for any files importing and using the renderless function
rg -l "from.*transfer/vue.*renderless"
# Search for implementations of ITransferRenderlessParamUtils
ast-grep --pattern 'interface ITransferRenderlessParamUtils {
$$$
}'
Length of output: 113
Script:
#!/bin/bash
# Let's try different search patterns to find usages and type definitions
# Search for Transfer component imports and usages
rg -l "Transfer"
# Search for ITransferRenderlessParamUtils in typescript files
rg -t ts "ITransferRenderlessParamUtils"
# Search for transfer renderless function usage
rg "transfer.*renderless"
# Look for vue transfer files
fd "transfer.*vue"
# Search for vm parameter usage in transfer related files
rg "vm.*transfer"
Length of output: 3780
Script:
#!/bin/bash
# Let's examine the type definition and its inheritance
# Check the content of the transfer type file
cat packages/renderless/types/transfer.type.ts
# Look for ISharedRenderlessParamUtils definition
rg -t ts "interface ISharedRenderlessParamUtils"
# Check if vm is used in other renderless components
rg -t ts "vm.*}: ISharedRenderlessParamUtils"
# Check the actual implementation in vue.ts
cat packages/renderless/src/transfer/vue.ts
Length of output: 6202
| @@ -217,17 +217,20 @@ export const clearQuery = (refs: ITransferRenderlessParams['refs']) => (which: ' | |||
|
|
|||
| /** SortableJs 插件的回调逻辑， 添加，删除，更新事件后，触发本函数 */ | |||
| export const logicFun = | |||
| ({ props, emit, state }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state'>) => | |||
| ({ props, emit, state, vm }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state'>) => | |||
There was a problem hiding this comment.
Update type annotation to include vm parameter
The type annotation is missing the vm parameter in the Pick type utility.
Apply this fix:
-({ props, emit, state, vm }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state'>) =>
+({ props, emit, state, vm }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state' | 'vm'>) =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ({ props, emit, state, vm }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state'>) => | |
| ({ props, emit, state, vm }: Pick<ITransferRenderlessParams, 'emit' | 'props' | 'state' | 'vm'>) => |
| const rightPanel = vm.$refs.rightPanel | ||
| const leftPanel = vm.$refs.leftPanel | ||
|
|
There was a problem hiding this comment.
Add null checks for panel refs
The panel refs should be checked for null to prevent runtime errors.
Apply this fix:
- const rightPanel = vm.$refs.rightPanel
- const leftPanel = vm.$refs.leftPanel
+ const rightPanel = vm.$refs.rightPanel
+ const leftPanel = vm.$refs.leftPanel
+
+ if (!rightPanel?.state?.filteredData || !leftPanel?.state?.filteredData) {
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
| ? rightPanel.state.filteredData[event.oldIndex][props.props.key] | ||
| : leftPanel.state.filteredData[event.oldIndex][props.props.key] |
There was a problem hiding this comment.
Add bounds check for array access
The array access using oldIndex should be bounds-checked to prevent undefined access.
Apply this fix:
- ? rightPanel.state.filteredData[event.oldIndex][props.props.key]
- : leftPanel.state.filteredData[event.oldIndex][props.props.key]
+ ? (rightPanel.state.filteredData[event.oldIndex] || {})[props.props.key]
+ : (leftPanel.state.filteredData[event.oldIndex] || {})[props.props.key]Additionally, consider adding validation:
+ if (event.oldIndex < 0 ||
+ (isAdd && event.oldIndex >= rightPanel.state.filteredData.length) ||
+ (!isAdd && event.oldIndex >= leftPanel.state.filteredData.length)) {
+ return
+ }Committable suggestion skipped: line range outside the PR's diff.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
@opentiny/vue-renderlesspackage to 3.19.5.Bug Fixes