Skip to content

fix: report settled canvas zoom through latest callback - #5520

Open
steve8708 wants to merge 1 commit into
mainfrom
steve8708/fix-design-latest-zoom-callback
Open

steve8708 wants to merge 1 commit into
mainfrom
steve8708/fix-design-latest-zoom-callback

Conversation

@steve8708

Copy link
Copy Markdown
Contributor

Summary

  • keep the latest onZoomChange callback for debounced wheel settle
  • preserve the existing Design E2E host readout regression coverage

Validation

  • pnpm exec oxfmt --check templates/design/app/components/design/MultiScreenCanvas.tsx templates/design/app/components/design/MultiScreenCanvas.wheel.test.tsx
  • pnpm --filter design exec vitest run app/components/design/MultiScreenCanvas.wheel.test.tsx

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes and has a few items to flag 🟡

Review Details

Code Review Summary

PR #5520 makes a focused React lifecycle correction in MultiScreenCanvas: the debounced wheel/pinch-settle callback now reads the current onZoomChange through a ref, so a Design Editor re-render during a pending gesture cannot route the settled zoom report to an obsolete callback. Removing the callback from the settle effect dependencies is consistent with that ref-based design, and the surrounding cleanup remains intact.

Risk assessment: Standard (shared UI state/interaction behavior). Two independent code-review passes found no confirmed runtime, cleanup, or stale-closure bugs. One reviewer identified a low-severity coverage gap: the existing wheel test does not replace onZoomChange while the debounce is pending, so the exact regression scenario is not directly protected by a test.

🧪 Browser testing: Attempted FULL verification. The dev server was healthy, but all planned browser cases were couldnt_verify because the executor environment exposed no Chrome/browser automation tools.

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.

1 participant