Skip to content

fix: PIXI.js destroy/interactive エラーを修正(本番環境対応)#4

Merged
terisuke merged 1 commit intomainfrom
dev
Oct 14, 2025
Merged

fix: PIXI.js destroy/interactive エラーを修正(本番環境対応)#4
terisuke merged 1 commit intomainfrom
dev

Conversation

@terisuke
Copy link
Copy Markdown
Contributor

🐛 修正内容

Vercel本番環境で発生していたPIXI.js関連のエラーを修正しました。

問題

本番環境(https://pro-edit.vercel.app)でエディタを開くと、以下のエラーが発生:

  1. TypeError: this.cancelResize is not a function

    • Compositor破棄時にPIXI.jsの内部エラーが発生
  2. React Error #185 & #310

    • 上記エラーによりReactのライフサイクルが破壊
    • Hook呼び出しの不整合が発生
  3. PixiJS Deprecation Warning

    • interactive プロパティの非推奨警告

修正内容

1. Compositor.destroy()にtry-catchを追加

// features/compositor/utils/Compositor.ts
try {
  this.app.destroy(true, {
    children: true,
    texture: true,
  })
  logger.info('Compositor: Cleaned up all resources')
} catch (error) {
  logger.error('Compositor: Error during cleanup', error)
}

効果: cancelResizeエラーをキャッチしてReactのクリーンアップを保護

2. PIXI v7.2+非推奨APIの更新

// features/compositor/components/Canvas.tsx
// 変更前
app.stage.interactive = true

// 変更後
app.stage.eventMode = 'static'

効果: 非推奨警告を解消

✅ 検証

  • ローカル環境でビルド成功
  • TypeScriptコンパイルエラーなし
  • Lintエラーなし
  • Vercelデプロイ待機中

📊 変更ファイル

- Compositor.destroy()にtry-catchを追加してcancelResizeエラーを防止
- Canvas.tsxでinteractiveをeventMode='static'に変更(PIXI v7.2+非推奨対応)
- stage.destroy()は呼ばず、app.destroy()に一任

これによりVercel本番環境でのReact Error #185/#310を解消
@vercel
Copy link
Copy Markdown

vercel bot commented Oct 14, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
pro-edit Ready Ready Preview Comment Oct 14, 2025 8:20pm

@claude
Copy link
Copy Markdown

claude bot commented Oct 14, 2025

Code Review: PR #4 - PIXI.js Error Fixes for Production

📋 Summary

This PR addresses critical production errors related to PIXI.js cleanup and deprecation warnings. The changes are focused and well-targeted at resolving specific runtime issues in the Vercel production environment.


✅ Positive Aspects

1. Problem Identification

  • Clear understanding of the root cause (cancelResize error during PIXI app destruction)
  • Good correlation between error and fix
  • Well-documented in PR description with specific error codes

2. API Modernization

  • ✅ Correctly updates deprecated interactive property to eventMode: 'static' (Canvas.tsx:42)
  • ✅ Consistent with PIXI v7.2+ API (project uses pixi.js ^7.4.2)
  • ✅ Already consistently applied in other managers (VideoManager.ts:53, ImageManager.ts:39)

3. Error Handling

  • ✅ Try-catch wrapper prevents React lifecycle corruption
  • ✅ Proper logging of both success and error cases
  • ✅ Allows graceful degradation on cleanup failures

🔍 Code Quality & Best Practices

Strengths:

  1. Code Comments: Excellent inline documentation explaining the rationale

    • Clear warning about not calling stage.destroy() separately
    • References to PIXI v7 behavior
  2. Consistency: The eventMode change aligns with existing codebase patterns

  3. Minimal Scope: Changes are surgical and focused on the specific issue

Areas for Improvement:

1. Error Logging Lacks Context ⚠️

Location: Compositor.ts:386

Current:

} catch (error) {
  logger.error('Compositor: Error during cleanup', error)
}

Recommendation:

} catch (error) {
  logger.error('Compositor: Error during cleanup - PIXI app destroy failed', {
    error,
    timecode: this.timecode,
    effectsCount: this.currentlyPlayedEffects.size
  })
}

Reasoning: Additional context helps diagnose if the error correlates with specific states (many effects, specific timecode, etc.)

2. Silent Error Swallowing ⚠️

The try-catch in destroy() suppresses all errors, which could hide other issues beyond cancelResize. Consider:

} catch (error) {
  // Only suppress known PIXI internal errors, re-throw others
  if (error instanceof Error && error.message.includes('cancelResize')) {
    logger.warn('Compositor: Suppressed known PIXI cleanup error', error)
  } else {
    logger.error('Compositor: Unexpected cleanup error', error)
    // Re-throw if critical
    throw error
  }
}

3. Canvas.tsx Cleanup Inconsistency 🔴

Location: Canvas.tsx:68

Issue: The Canvas component's cleanup also calls app.destroy() but doesn't have the same try-catch protection:

return () => {
  if (appRef.current) {
    appRef.current.destroy(true, { children: true })  // ⚠️ No try-catch here!
    appRef.current = null
    setCanvasReady(false)
  }
}

Recommendation: Apply the same defensive pattern:

return () => {
  if (appRef.current) {
    try {
      appRef.current.destroy(true, { children: true })
      logger.info('Canvas: PIXI app destroyed successfully')
    } catch (error) {
      logger.error('Canvas: Error during PIXI app cleanup', error)
    }
    appRef.current = null
    setCanvasReady(false)
  }
}

🐛 Potential Bugs

None identified

The changes are defensive and don't introduce new logic that could cause bugs.


⚡ Performance Considerations

No Performance Impact

  • Try-catch has negligible overhead in cleanup paths
  • eventMode: 'static' is equivalent to interactive: true in terms of performance
  • Changes occur in destruction/initialization paths (not hot paths)

🔒 Security Concerns

None identified

  • No user input handling
  • No new attack surfaces
  • Defensive error handling improves robustness

🧪 Test Coverage

Missing Tests ⚠️

Current State:

  • No unit tests for Compositor.destroy()
  • No unit tests for Canvas cleanup
  • Existing tests: media.test.ts, timeline.test.ts, basic.spec.ts (none cover PIXI cleanup)

Recommendations:

  1. Add Compositor Cleanup Test:
// tests/unit/compositor.test.ts
describe('Compositor', () => {
  it('should handle destroy errors gracefully', async () => {
    const app = new PIXI.Application()
    const compositor = new Compositor(app, async () => '')
    
    // Simulate cleanup error
    vi.spyOn(app, 'destroy').mockImplementation(() => {
      throw new Error('cancelResize is not a function')
    })
    
    expect(() => compositor.destroy()).not.toThrow()
  })
})
  1. Add Canvas Cleanup Test:
describe('Canvas', () => {
  it('should cleanup PIXI app on unmount', () => {
    const { unmount } = render(<Canvas width={800} height={600} />)
    
    expect(() => unmount()).not.toThrow()
  })
})

Note: While these tests would improve coverage, they're not blocking for this bug fix PR since the issue was verified in production.


📊 Risk Assessment

Category Risk Level Notes
Breaking Changes 🟢 Low No API changes, only internal fixes
Regression 🟢 Low Defensive error handling, minimal scope
Production Impact 🟢 Low Addresses existing production issue
Performance 🟢 None No hot-path changes

✅ Checklist Verification

From PR description:

  • ✅ Local build successful
  • ✅ TypeScript compilation clean
  • ✅ Lint errors resolved
  • ⏳ Vercel deployment pending

🎯 Recommendations Summary

Required Before Merge:

  1. Add try-catch to Canvas.tsx cleanup (Canvas.tsx:68)
    • Ensures consistency with Compositor.ts fix
    • Prevents same error in component unmount path

Recommended (Non-Blocking):

  1. ⚠️ Enhance error logging with context (Compositor.ts:386)
  2. ⚠️ Consider selective error suppression instead of catching all errors
  3. ⚠️ Add unit tests for cleanup paths (future PR acceptable)

Nice to Have:

  1. 💡 Document the cancelResize issue in code comments with reference to PIXI.js version
  2. 💡 Consider adding a custom error type for expected PIXI cleanup errors

🏁 Final Verdict

Status: ✅ Approve with Minor Changes

Rationale:

  • Correctly addresses production errors
  • Follows PIXI v7.2+ best practices
  • Consistent with existing codebase patterns
  • Low risk, high value fix

Action Items:

  1. Apply same try-catch pattern to Canvas.tsx cleanup
  2. Verify Vercel deployment succeeds
  3. Consider adding tests in follow-up PR

Excellent work diagnosing and fixing the production issue! The changes are well-reasoned and properly documented. 🎉


Review conducted by Claude Code - All feedback is constructive and aims to improve code quality and maintainability.

@terisuke terisuke merged commit 0cd496a into main Oct 14, 2025
3 checks passed
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