Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a bug in the animation custom type detection logic by extracting duplicated code into a reusable utility function. The refactoring reduces code duplication but introduces a critical logic error in one location.
- Extracts repeated custom type detection logic into a new
getCustomTypeutility function - Replaces three instances of duplicated code with calls to the new utility function
- Adds a conditional check at line 563 that incorrectly determines when to reuse cached custom type values
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/vrender-animate/src/executor/utils.ts | New utility function to detect custom animation types (class vs function vs non-function) |
| packages/vrender-animate/src/executor/animate-executor.ts | Refactors duplicate code to use new utility function; introduces conditional logic for custom type caching |
| common/changes/@visactor/vrender-components/feat-fix-issue-of-animation-custom-type_2025-12-17-07-45.json | Changelog entry for vrender-components package |
| common/changes/@visactor/vrender-animate/feat-fix-issue-of-animation-custom-type_2025-12-17-07-45.json | Changelog entry for vrender-animate package |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| const custom = effect.custom ?? AnimateExecutor.builtInAnimateMap[type]; | ||
| const customType = (effect as any).customType; | ||
| const customType = effect.custom ? (effect as any).customType : getCustomType(custom); |
There was a problem hiding this comment.
The condition checks effect.custom instead of checking if customType already exists on the effect object. This is incorrect because:
- At line 562,
customcan be eithereffect.customOR a built-in animation fromAnimateExecutor.builtInAnimateMap[type] - In the
parseParamsfunction (lines 173-180), effects that go through the timeline path havecustomTypecomputed and stored - When
effect.customis not provided but a built-in animation is used, this condition will be false even thoughcustomTypemight already exist on the effect
The condition should check for the presence of customType on the effect, not effect.custom. For example: (effect as any).customType !== undefined ? (effect as any).customType : getCustomType(custom)
| const customType = effect.custom ? (effect as any).customType : getCustomType(custom); | |
| const customType = | |
| (effect as any).customType !== undefined ? (effect as any).customType : getCustomType(custom); |
| @@ -0,0 +1,5 @@ | |||
| import { isFunction } from '@visactor/vutils'; | |||
|
|
|||
There was a problem hiding this comment.
This function lacks documentation explaining what the return values (0, 1, 2) represent. Based on the implementation:
- 0: not a function or falsy value
- 1: ES6 class constructor
- 2: regular function
Adding JSDoc documentation would improve code maintainability and help other developers understand the purpose and return values of this function.
| /** | |
| * Determines the type of the given `custom` value. | |
| * | |
| * Return values: | |
| * - `0`: `custom` is falsy or not a function. | |
| * - `1`: `custom` is an ES6 class constructor. | |
| * - `2`: `custom` is a regular (non-class) function. | |
| * | |
| * @param custom - The value to check. | |
| * @returns A numeric code indicating the type of `custom`. | |
| */ |
[中文版模板 / Chinese template]
🤔 This is a ...
🔗 Related issue link
🐞 Bugserver case id
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge
🚀 Summary
copilot:summary
🔍 Walkthrough
copilot:walkthrough