Skip to content

Conversation

@wumaolinmaoan
Copy link
Contributor

@wumaolinmaoan wumaolinmaoan commented Jan 21, 2026

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved device pixel ratio detection for better visual scaling across devices.
    • Enhanced visual effect coordinate mapping for more accurate positioning.
  • New Features

    • Added new translation capability for precise effect positioning adjustments.

✏️ Tip: You can customize this high-level summary in your review settings.

@wumaolinmaoan wumaolinmaoan requested a review from yiiqii January 21, 2026 03:35
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 21, 2026

📝 Walkthrough

Walkthrough

These changes simplify device pixel ratio calculation in the device utility and refactor coordinate mapping in the VFXItem class to use canvas bounding client rect dimensions instead of renderer-derived values, with an added pixel-to-world translation method.

Changes

Cohort / File(s) Change Summary
Device Utility Simplification
packages/effects-core/src/utils/device.ts
Simplified pixel ratio computation by replacing complex screen dimension logic with direct window.devicePixelRatio check, capping at 2 with fallback to 1
VFX Item Coordinate Mapping
packages/effects-core/src/vfx-item.ts
Refactored setPositionByPixel() to compute dimensions from canvas bounding client rect instead of renderer size; added new translateByPixel() method for pixel-to-world translation using VP ratio and canvas dimensions; removed duplicate translateByPixel() implementation

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 hops with glee
Device pixels simplified, no more complexity!
Canvas coordinates map with care,
Translation and position dance through the air! ✨
The renderer's old ways fade away,
Making the effects code cleaner today! 🎨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary change: simplifying pixel ratio calculation to use window.devicePixelRatio instead of the previous screen dimension-based approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/effects-core/src/vfx-item.ts`:
- Around line 505-523: Both setPositionByPixel and translateByPixel can divide
by canvas width/height and produce NaN when the canvas size is zero; add a guard
after reading const { width, height } =
this.composition.getEngine().canvas.getBoundingClientRect() in both methods to
early-return if width === 0 || height === 0 (or if either is falsy), preventing
math operations that would corrupt this.transform; keep the existing logic
otherwise and reference the methods setPositionByPixel and translateByPixel and
the call to getBoundingClientRect() when applying the guard.
🧹 Nitpick comments (1)
packages/effects-core/src/utils/device.ts (1)

2-3: Consider reusing canUseBOM for the window check.

Keeps BOM guards consistent within this module and centralizes the environment check.

♻️ Suggested tweak
-export function getPixelRatio (): number {
-  if (typeof window === 'object') {
+export function getPixelRatio (): number {
+  if (canUseBOM) {
     return Math.min(2, window.devicePixelRatio || 1);
   }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants