エクスポート機能の安定性改善とタイムライン値の正規化

[terisuke]

変更内容

エクスポート機能の改善

• エクスポートジョブの古いレコードの自動クリーンアップ機能を追加
• エクスポート品質のバリデーション強化
• FFmpegタイムアウト設定を追加
• エクスポートキューのTTL管理を実装
• ポーリング処理のクリーンアップを改善

タイムライン機能の改善

• タイムライン値（start_at_position, duration, start, end）を整数ミリ秒に正規化
• DragHandler、TrimHandlerでの値の正規化を実装
• AutoSaveManagerでのエフェクトタイミングの正規化
• タイムラインストアでの値の一貫性を保証

データベース

• export_jobsテーブルのupdated_at自動更新トリガーを追加

検証コマンド

npm run type-check
npm run lint
npm run test

関連タスク

• DEVELOPMENT_STATUS.md のエクスポート安定性改善項目
• specs/001-proedit-mvp-browser/tasks.md のバグ修正項目

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
pro-edit	Ready	Preview	Comment	Oct 19, 2025 1:18am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

[claude]

Pull Request Review: エクスポート機能の安定性改善とタイムライン値の正規化

Overall Assessment

This PR introduces important improvements to export job stability and timeline value precision. The changes are well-structured and address real production concerns around timing precision and job lifecycle management.

Rating: ✅ Approve with Minor Suggestions

---

Code Quality & Best Practices

✅ Strengths

1. Defensive Programming

   • Strong validation added in app/api/render/route.ts:30-32 with isValidQuality() check
   • Configuration validation via validateExportServiceConfig() prevents runtime errors
   • Proper error handling with descriptive messages

2. Consistent Normalization Pattern

   • Timeline value normalization (Math.max(0, Math.round(value))) applied consistently across:
     • DragHandler.ts:147
     • TrimHandler.ts:209
     • autosave.ts:257-283
     • stores/timeline.ts:15-33
   • This ensures integer millisecond precision throughout the system

3. Code Organization

   • Helper functions properly extracted (isValidQuality, validateExportServiceConfig, cleanupStaleExportJobsForUser)
   • Clear separation of concerns between validation, cleanup, and business logic

4. Import Organization

   • Imports properly organized in app/api/render/route.ts:1-4 (external, then internal)

⚠️ Areas for Improvement

1. Type Safety Issue (app/api/render/route.ts:24-32)

   const requestedQuality = body.quality ?? "1080p";
   
   if (!isValidQuality(requestedQuality)) {
     return NextResponse.json({ error: "Invalid export quality" }, { status: 400 });
   }
   const quality: ExportQuality = requestedQuality;

   Issue: Type assertion after validation, but TypeScript doesn't know this is safe.

   Suggestion: Use type predicate more explicitly:

   const requestedQuality = body.quality ?? "1080p";
   if (!isValidQuality(requestedQuality)) {
     return NextResponse.json({ error: "Invalid export quality" }, { status: 400 });
   }
   // requestedQuality is now typed as ExportQuality thanks to type predicate
   const quality = requestedQuality;

2. Magic Numbers (lib/export/queue.ts:21, app/api/render/route.ts:118)

   const EXPORT_JOB_STALE_TIMEOUT_MS = Number(process.env.EXPORT_JOB_STALE_TIMEOUT_MS ?? 5 * 60 * 1000);
   const EXPORT_QUEUE_JOB_TTL_MS = Number(process.env.EXPORT_QUEUE_JOB_TTL_MS ?? 10 * 60 * 1000);

   Suggestion: Extract to constants file or add comments explaining the relationship between these two timeouts:

   // Stale job timeout: 5 minutes (jobs not updated within this time are marked failed)
   const EXPORT_JOB_STALE_TIMEOUT_MS = Number(process.env.EXPORT_JOB_STALE_TIMEOUT_MS ?? 5 * 60 * 1000);
   // Queue TTL: 10 minutes (pending jobs older than this are removed from queue)
   const EXPORT_QUEUE_JOB_TTL_MS = Number(process.env.EXPORT_QUEUE_JOB_TTL_MS ?? 10 * 60 * 1000);

3. Error Handling Inconsistency (app/api/render/route.ts:130-136, 149-155)

   • Some database errors only log warnings (console.warn) instead of throwing
   • This could silently fail to clean up stale jobs

   Suggestion: Consider whether silent failures are appropriate or if cleanup failures should be surfaced

---

Potential Bugs & Issues

🔴 Critical

None identified - No critical bugs found.

🟡 Medium Priority

1. Race Condition in Queue Cleanup (lib/export/queue.ts:99-127)

   • cleanupExpiredPendingJobs() is called in both enqueueExportJob() and runNextJob()
   • Multiple concurrent calls could lead to redundant cleanup operations

   Impact: Low - cleanup is idempotent, but slightly inefficient

   Suggestion: Consider debouncing cleanup or using a timestamp-based check to avoid unnecessary iterations

2. Polling Cleanup on Component Unmount (app/editor/[projectId]/EditorClient.tsx:368-371)

   } finally {
     clearExportPoll()
   }

   Issue: The finally block ensures cleanup, which is good, but the polling interval is still referenced via exportPollRef.current

   Verification needed: Ensure clearExportPoll() properly clears the interval to avoid memory leaks

3. Normalization in AutoSave (features/timeline/utils/autosave.ts:174-178)

   if (hasTimingDiff) {
     useTimelineStore.getState().setEffects(sanitizedEffects);
   }

   Issue: Updating store state during autosave could trigger additional autosave cycles

   Verification needed: Confirm this doesn't create an infinite loop or unnecessary saves

🟢 Low Priority

1. Duplicate Normalization (stores/timeline.ts)
   • Effects are normalized in both setEffects() and updateEffect()
   • This is defensive but could be optimized if effects are already normalized upstream

---

Performance Considerations

✅ Positive

1. FFmpeg Timeout Protection (lib/export/server.ts:87, 123)

   • Prevents hanging FFmpeg processes from blocking the queue
   • Configurable via environment variable

2. Queue Deduplication (lib/export/queue.ts:96-102)

   • Prevents duplicate jobs from being queued for the same project
   • Good memory efficiency

3. Efficient Cleanup (lib/export/queue.ts:99-113)

   • Only runs when queue has pending jobs
   • Early return optimization

⚠️ Potential Issues

1. Stale Job Cleanup on Every Request (app/api/render/route.ts:41)

   await cleanupStaleExportJobsForUser(supabase, user.id);

   Impact: Adds database query to every export request

   Suggestion: Consider:

   • Rate limiting this cleanup (e.g., once per minute per user)
   • Moving to background job or scheduled task
   • Using database-level cleanup with periodic cron job

2. Synchronous Array Operations (lib/export/queue.ts:105-113)

   state.pending = state.pending.filter((job) => {
     if (!job.enqueuedAt) return true;
     return now - job.enqueuedAt <= EXPORT_QUEUE_JOB_TTL_MS;
   });

   Impact: Low for typical queue sizes, but could be optimized

   Suggestion: If queue grows large, consider using a Set or Map for O(1) lookups

---

Security Concerns

✅ Good Practices

1. Server-Side Validation (app/api/render/route.ts:30-32)

   • Quality parameter validated server-side, not just client-side
   • Prevents malicious quality values

2. Configuration Validation (app/api/render/route.ts:35-38)

   • Checks for required environment variables before processing
   • Prevents information disclosure through error messages

3. User Isolation (app/api/render/route.ts:41, 125-136)

   • Cleanup scoped to user ID (.eq("user_id", userId))
   • Prevents cross-user data leakage

⚠️ Minor Concerns

1. Error Message Exposure (app/api/render/route.ts:116-117)

   return NextResponse.json({ error: configError }, { status: 500 });

   Issue: Configuration errors might expose server setup details

   Suggestion: Use generic message for production:

   return NextResponse.json({ 
     error: process.env.NODE_ENV === 'production' 
       ? 'Export service unavailable' 
       : configError 
   }, { status: 500 });

---

Test Coverage

❌ Missing Tests

No test files added for new functionality.

The PR introduces significant new logic but lacks corresponding tests:

1. Unit Tests Needed:

   • isValidQuality() validation logic
   • validateExportServiceConfig() edge cases
   • cleanupStaleExportJobsForUser() with various job states
   • cleanupExpiredPendingJobs() TTL logic
   • Normalization functions in handlers and store
   • getSanitizedEffects() timing diff detection

2. Integration Tests Needed:

   • Export job lifecycle with stale timeout
   • Queue behavior with expired jobs
   • Polling cleanup on error/success/timeout

3. Existing Tests:

   • tests/unit/timeline.test.ts exists but doesn't cover new normalization logic
   • No export-related tests found

Recommendation: Add test coverage before merging, especially for:

• Edge cases in cleanup logic (e.g., jobs without enqueuedAt)
• FFmpeg timeout behavior
• Normalization with negative/float values

Test suggestions:

// Example test for normalization
describe('normalizeTimelineValue', () => {
  it('should round float values to integers', () => {
    expect(normalizeTimelineValue(1000.7)).toBe(1001)
  })
  
  it('should clamp negative values to 0', () => {
    expect(normalizeTimelineValue(-100)).toBe(0)
  })
  
  it('should handle undefined', () => {
    expect(normalizeTimelineValue(undefined)).toBeUndefined()
  })
})

---

Database Migration

✅ Migration Quality

File: supabase/migrations/008_export_jobs_updated_at.sql

Good:

• Uses IF NOT EXISTS for idempotency
• Reuses existing update_updated_at_column() function (DRY principle)
• Clear naming convention

Verification needed:

• Ensure update_updated_at_column() function exists in earlier migrations
• Test migration rollback scenario (no down migration provided)

Suggestion: Consider adding a down migration:

-- Down migration (if needed)
DROP TRIGGER IF EXISTS update_export_jobs_updated_at ON export_jobs;
ALTER TABLE export_jobs DROP COLUMN IF EXISTS updated_at;

---

Architecture & Design

✅ Good Architectural Decisions

1. Separation of Concerns

   • Queue management separated from export processing
   • Cleanup logic isolated in dedicated functions
   • Normalization centralized in store and handlers

2. Configuration via Environment Variables

   • All timeouts and limits configurable
   • Good for deployment flexibility

3. Graceful Degradation

   • Cleanup failures are logged but don't block exports
   • Missing enqueuedAt handled gracefully

💡 Suggestions

1. Observability

   • Consider adding metrics/telemetry for:
     • Number of stale jobs cleaned up
     • Queue processing latency
     • FFmpeg timeout frequency

2. Documentation

   • Add JSDoc comments for public functions
   • Document relationship between timeouts
   • Add comments explaining normalization rationale

---

Summary & Recommendations

Must Fix Before Merge

1. ❌ Add test coverage for critical paths (cleanup, normalization, validation)

Should Fix Before Merge

1. ⚠️ Add error message sanitization for production (security)
2. ⚠️ Verify autosave normalization doesn't cause save loops
3. ⚠️ Add JSDoc comments to new functions
4. ⚠️ Consider rate-limiting stale job cleanup

Nice to Have

1. 💡 Add observability/metrics
2. 💡 Optimize cleanup to avoid running on every request
3. 💡 Add migration rollback script
4. 💡 Extract timeout constants to shared config file

---

Specific File Feedback

app/api/render/route.ts

• Lines 30-32: Good validation, see type safety note above
• Lines 35-38: Excellent config validation
• Lines 41: Consider rate-limiting this cleanup call
• Lines 125-136: Good defensive programming with .eq("user_id", userId)

features/timeline/handlers/DragHandler.ts & TrimHandler.ts

• Consistent normalization pattern - good!
• Consider: Extract normalizeTimelineValue() to shared utility to reduce duplication

features/timeline/utils/autosave.ts

• Lines 174-178: Verify this doesn't trigger save loops
• Good: Timing diff detection prevents unnecessary updates

lib/export/queue.ts

• Lines 96-102: Excellent deduplication logic
• Lines 105-113: Consider adding early return when no expired jobs found
• Lines 76-84: Good cleanup of runningPerUser map

stores/timeline.ts

• Lines 3-33: Well-structured normalization helpers
• Consider: These could be unit-tested in isolation

supabase/migrations/008_export_jobs_updated_at.sql

• Simple and correct - good!
• Missing: Down migration

---

Overall: This is a solid PR that addresses real production concerns. The main blocker is lack of test coverage. Once tests are added and the minor security concern is addressed, this should be good to merge.

Great work on the consistent normalization pattern and defensive cleanup logic! 🎉

---

Review generated by Claude Code

[claude]

Pull Request Review: Export Stability Improvements and Timeline Value Normalization

Summary

This PR introduces significant improvements to export job stability and timeline value consistency. The changes are well-structured and address important reliability concerns in the video export pipeline.

---

✅ Strengths

1. Export Reliability Improvements

• FFmpeg timeout handling (lib/export/server.ts:128-166): Excellent addition of runFfmpegWithTimeout to prevent hanging export jobs
• Stale job cleanup (app/api/render/route.ts:133-167): Proactive cleanup of zombie jobs prevents resource leaks
• Queue TTL management (lib/export/queue.ts:109-128): Prevents memory leaks from abandoned jobs in the queue

2. Timeline Value Normalization

• Consistent rounding (stores/timeline.ts:13-34): Normalizing timing values to integer milliseconds prevents floating-point precision issues
• Comprehensive coverage: Applied across DragHandler, TrimHandler, AutoSaveManager, and timeline store
• Type safety: Good use of TypeScript generics in sanitization functions

3. Database Migration

• Updated_at tracking (supabase/migrations/008_export_jobs_updated_at.sql): Properly leverages existing trigger function for automatic timestamp updates

---

🔍 Issues Found

HIGH PRIORITY

1. Race Condition in Export Polling (app/editor/[projectId]/EditorClient.tsx:298-377)

Issue: Interval is not properly cleared when component unmounts or new export starts

// Line 369-371: exportPollRef.current may be overwritten without clearing previous interval
exportPollRef.current = setInterval(() => {
  void pollJobStatus()
}, 2000)

Impact: Multiple polling intervals can run simultaneously, causing excessive API calls and memory leaks

Recommendation:

const pollJobStatus = async () => {
  // Clear any existing interval first
  clearExportPoll()
  
  // ... polling logic ...
  
  // Set new interval
  exportPollRef.current = setInterval(() => {
    void pollJobStatus()
  }, 2000)
}

2. Export Quality Validation Missing Type Guard Return (app/api/render/route.ts:122-124)

Issue: isValidQuality uses type predicate but could be strengthened

function isValidQuality(value: string): value is ExportQuality {
  return Object.prototype.hasOwnProperty.call(EXPORT_PRESETS, value);
}

Recommendation: Use safer property check:

function isValidQuality(value: string): value is ExportQuality {
  return value in EXPORT_PRESETS;
}

3. Potential Memory Leak in Queue State (lib/export/queue.ts:78-83)

Issue: runningPerUser Map may not be cleaned up if job fails before finally block

// Line 78-83: If error occurs before finally, counter stays incremented
const currentCount = Math.max(0, (state.runningPerUser.get(job.userId) ?? 1) - 1);
if (currentCount <= 0) {
  state.runningPerUser.delete(job.userId);
}

Recommendation: Ensure cleanup happens even on early errors by wrapping job execution more defensively

MEDIUM PRIORITY

4. Hard-coded Poll Interval (app/editor/[projectId]/EditorClient.tsx:369)

Issue: 2-second polling interval is hard-coded

exportPollRef.current = setInterval(() => {
  void pollJobStatus()
}, 2000)

Recommendation: Make configurable via environment variable for different environments (dev vs prod)

5. Missing Error Handling for Normalization (stores/timeline.ts:13-15)

Issue: No validation that input is actually a number before calling Math operations

function normalizeTimelineValue(value: number | undefined): number | undefined {
  if (value === undefined) return undefined
  return Math.max(0, Math.round(value))
}

Recommendation: Add runtime type check:

function normalizeTimelineValue(value: number | undefined): number | undefined {
  if (value === undefined) return undefined
  if (typeof value !== 'number' || !Number.isFinite(value)) return 0
  return Math.max(0, Math.round(value))
}

LOW PRIORITY

6. Inconsistent Error Logging (lib/export/queue.ts:70 vs lib/export/server.ts:429)

Issue: Some errors use console.error, others use console.warn
Recommendation: Standardize logging approach and consider using structured logging library

7. Magic Numbers (lib/export/server.ts:19-22)

Issue: Default timeout values are hard-coded

const EXPORT_FFMPEG_TIMEOUT_SECONDS = Math.max(1, Number(process.env.EXPORT_FFMPEG_TIMEOUT_SECONDS ?? 10 * 60));

Recommendation: Extract to named constants with documentation about why those specific values

---

🔒 Security Considerations

✅ Good Practices

1. Input validation: Quality parameter is properly validated before use (line 30-32 in route.ts)
2. User authorization: Jobs are properly scoped to user_id throughout
3. File path sanitization: sanitizeFilename properly handles malicious input (server.ts:196-206)
4. Signed URLs: Exports use time-limited signed URLs instead of direct access

⚠️ Potential Concerns

1. Environment variable validation: Consider validating that timeout values are within acceptable ranges to prevent DoS via config
2. Resource limits: No maximum queue size check could allow queue flooding

---

⚡ Performance Considerations

✅ Improvements

1. Queue management: Proper concurrency limits prevent resource exhaustion
2. FFmpeg timeout: Prevents runaway processes from consuming resources indefinitely
3. Stale job cleanup: Reduces database bloat and improves query performance

⚠️ Watch Points

1. Polling overhead: 2-second intervals for all active exports could strain the API under high load
2. Database updates: Each progress update writes to database - consider batching if exports are long
3. Normalization overhead: Sanitizing all effects on every update - acceptable for small timelines but may need optimization for large projects

---

🧪 Test Coverage

Status: ⚠️ Tests not updated

The PR introduces significant new logic but doesn't include:

• Tests for FFmpeg timeout handling
• Tests for stale job cleanup logic
• Tests for timeline value normalization
• Tests for queue TTL expiration

Recommendation: Add unit tests for:

// Example test cases needed:
describe('runFfmpegWithTimeout', () => {
  it('should timeout and kill FFmpeg process after specified duration')
  it('should clear timeout when FFmpeg completes successfully')
})

describe('Timeline value normalization', () => {
  it('should round floating point values to integers')
  it('should handle negative values by clamping to 0')
  it('should handle NaN and Infinity gracefully')
})

---

📋 Recommendations Summary

Before Merge (Critical)

1. Fix race condition in export polling interval management
2. Add proper cleanup to prevent memory leaks in queue state
3. Add runtime type validation to normalization functions

Follow-up Tasks (Recommended)

1. Add unit tests for new timeout and cleanup logic
2. Make polling interval configurable
3. Standardize error logging approach
4. Add maximum queue size limits
5. Consider progress update batching for long exports

---

🎯 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5 - Good with minor issues)

This PR demonstrates solid engineering with thoughtful improvements to system reliability. The export timeout handling and stale job cleanup are particularly well-implemented. The timeline normalization is a smart fix for precision issues.

The main concerns are around race conditions in the polling logic and missing test coverage. Once the critical issues are addressed, this will be a strong improvement to the codebase.

Recommendation: Request changes to fix the polling race condition, then approve after verification.

[claude]

Pull Request Review: Export Stability and Timeline Normalization

Summary

This PR introduces significant improvements to export reliability and timeline value consistency. Overall, the implementation is well-architected with good defensive programming practices. I've identified several areas for improvement and potential issues.

---

🟢 Strengths

1. Export Job Cleanup (app/api/render/route.ts:20-56)

• Excellent: Proactive cleanup of stale export jobs before creating new ones
• Good pattern: Using EXPORT_JOB_STALE_TIMEOUT_MS constant for configurability
• Robust validation: validateExportServiceConfig() checks for required environment variables

2. Timeline Value Normalization (stores/timeline.ts:6-34)

• Smart approach: Centralizing normalization logic prevents floating-point inconsistencies
• Comprehensive: All timing fields are normalized (start_at_position, duration, start, end)
• Defensive: Handles undefined, NaN, and Infinity edge cases

3. FFmpeg Timeout Implementation (lib/export/server.ts:130-166)

• Critical feature: Prevents indefinite hangs on failed exports
• Clean implementation: Properly cleans up timeout handlers
• Good UX: Clear error messages when timeouts occur

4. Queue TTL Management (lib/export/queue.ts:96-119)

• Prevents memory leaks: Expired jobs are removed from the queue
• Observable: Logs cleanup actions for debugging

---

🟡 Issues & Recommendations

High Priority

1. Race Condition in Export Polling (app/editor/[projectId]/EditorClient.tsx:303-382)

Issue: The finally block clears the interval, but if pollJobStatus throws after the interval is set, the interval might still be running.

Current code:

try {
  await new Promise<void>((resolve, reject) => {
    // ... polling logic
    exportPollRef.current = setInterval(() => {
      void pollJobStatus()
    }, EXPORT_POLL_INTERVAL_MS)
  })
} finally {
  clearExportPoll()
}

Recommendation: The clearExportPoll() is already called inside the catch block of pollJobStatus. Consider whether the finally is redundant or if there are edge cases where the interval isn't cleared.

2. Inconsistent Quality Type (app/api/render/route.ts:21-32)

Issue: The PR description mentions "480p/720p/1080p/4K" but the type definition only includes '720p' | '1080p' | '4k' (no 480p).

Location: features/export/types.ts:3

export type ExportQuality = '720p' | '1080p' | '4k'

Recommendation: Either:

• Update the PR description to remove 480p reference, OR
• Add 480p to EXPORT_PRESETS if it's intended to be supported

3. Potential Data Loss in Auto-Save (features/timeline/utils/autosave.ts:173-179)

Issue: When timing normalization detects differences, the store is updated synchronously:

if (hasTimingDiff) {
  // Refresh store with sanitized values to keep UI state consistent
  useTimelineStore.getState().setEffects(sanitizedEffects);
}

Concern: This could trigger another auto-save cycle. While debouncing may prevent issues, it's worth verifying this doesn't cause save loops.

Recommendation: Add a comment explaining why this is safe, or add a flag to prevent recursive saves.

4. Missing Database Constraint Validation (supabase/migrations/008_export_jobs_updated_at.sql:1-9)

Issue: The migration assumes the update_updated_at_column() function exists.

Recommendation: Add a check or comment referencing where this function is defined (likely in an earlier migration). Consider adding:

-- Requires: update_updated_at_column() function from migration 001 or 002

Medium Priority

5. Environment Variable Type Safety (app/editor/[projectId]/EditorClient.tsx:37-40)

Good practice: Validates and defaults EXPORT_POLL_INTERVAL_MS
Suggestion: Extract this pattern into a utility function for reuse:

// lib/utils/env.ts
export function getEnvNumber(key: string, defaultValue: number): number {
  const raw = Number(process.env[key] ?? defaultValue)
  return Number.isFinite(raw) && raw > 0 ? raw : defaultValue
}

This same pattern appears in:

• lib/export/queue.ts:21-23 (EXPORT_QUEUE_JOB_TTL_MS)
• lib/export/server.ts:19-22 (EXPORT_FFMPEG_TIMEOUT_SECONDS)

6. Silent Failure in Cleanup (app/api/render/route.ts:136-137)

if (staleError) {
  console.warn("[RenderAPI] Failed to fetch stale export jobs:", staleError);
  return; // ⚠️ Silent failure
}

Recommendation: Consider if this should surface to the user or retry. Silent failures can hide DB connectivity issues.

7. Queue State Mutation Safety (lib/export/queue.ts:96-119)

The cleanupExpiredPendingJobs mutates the state array directly:

state.pending = state.pending.filter(...)

Recommendation: This is fine for server-side code, but add a comment explaining that state is a mutable singleton for clarity.

Low Priority

8. Unused Variable (app/register-sw.tsx:79)

const registration = await navigator.serviceWorker.ready // ❌ Unused

Status: ✅ Already fixed in the PR (line removed)

9. Magic Number (features/timeline/handlers/DragHandler.ts:147, TrimHandler.ts:209)

The Math.max(0, ...) ensures non-negative values. Consider adding a named constant:

const MIN_TIMELINE_VALUE = 0

10. Missing Tests

Observation: No test files were modified.
Recommendation: Add tests for:

• Timeline value normalization edge cases (NaN, Infinity, negative values)
• Export job cleanup logic
• FFmpeg timeout behavior

---

🔒 Security Analysis

✅ Good Practices

1. Environment validation: validateExportServiceConfig() prevents misconfiguration
2. RLS compliance: Migration 007 shows proper Row Level Security policies
3. Service role key protection: Used only server-side in render route

⚠️ Considerations

1. Service role key exposure: Ensure SUPABASE_SERVICE_ROLE_KEY is never exposed to client bundles
2. User isolation: Cleanup function correctly scopes to userId ✅
3. SQL injection: Parameterized queries via Supabase client ✅

---

⚡ Performance Considerations

Positive

1. Debounced polling: Uses configurable EXPORT_POLL_INTERVAL_MS (default 2s)
2. Queue management: TTL prevents unbounded memory growth
3. Integer normalization: Math.round() is fast and consistent

Concerns

1. Polling overhead: Consider WebSockets/Supabase Realtime for export job updates instead of polling
2. Store update on every save: The hasTimingDiff check and subsequent setEffects could be expensive for large timelines

Recommendation: Profile the normalization performance with 100+ timeline effects.

---

📋 Test Coverage Recommendations

Add tests for:

1. Timeline normalization:

   • Floating-point values → integers
   • NaN, Infinity, undefined handling
   • Negative values clamped to 0

2. Export job cleanup:

   • Stale jobs are marked as failed
   • Timestamp threshold calculation
   • User isolation (only own jobs cleaned)

3. Queue TTL:

   • Expired jobs removed
   • enqueuedAt undefined handling
   • Concurrent job limits respected

4. FFmpeg timeout:

   • Timeout kills process
   • Cleanup on success
   • Error message format

---

🎯 Verdict

Recommendation: Approve with minor revisions

This PR demonstrates solid engineering with:

• ✅ Good error handling
• ✅ Defensive programming
• ✅ Configurable timeouts
• ✅ Data consistency improvements

Before merging:

1. ✅ Verify auto-save doesn't create save loops (test manually)
2. 🔴 Resolve 480p vs quality type inconsistency
3. 🟡 Consider adding basic tests for core normalization logic
4. 🟡 Document the update_updated_at_column() dependency in migration 008

Estimated risk: Low-Medium (mostly defensive improvements)

---

📚 Additional Notes

• Migration 008 is idempotent (uses IF NOT EXISTS) ✅
• Environment variable table in README is helpful ✅
• Japanese comments are clear and descriptive ✅

Great work on improving export stability! 🚀

[claude]

PR Review: エクスポート機能の安定性改善とタイムライン値の正規化

Overview

This PR implements export stability improvements and timeline value normalization. The changes are well-structured and address important reliability issues. Overall quality is good, with some recommendations for improvement.

Code Quality & Best Practices ✅

Strengths

• Consistent normalization pattern: Timeline value normalization is implemented consistently across DragHandler, TrimHandler, AutoSaveManager, and timeline store
• Environment configuration: Good use of getEnvNumber utility for safe environment variable parsing with fallbacks
• Error handling: Comprehensive error handling in export polling and FFmpeg timeout logic
• Code organization: Clean separation of concerns between queue management, server-side processing, and client polling

Recommendations

1. Type Safety in app/api/render/route.ts:127

// Current:
function isValidQuality(value: string): value is ExportQuality {
  return value in EXPORT_PRESETS;
}

This is good! However, consider adding explicit type assertion in the calling code to avoid potential type widening issues.

2. Magic Numbers in lib/export/queue.ts:87

const EXPORT_JOB_STALE_TIMEOUT_MS = Number(process.env.EXPORT_JOB_STALE_TIMEOUT_MS ?? 5 * 60 * 1000);

Should use getEnvNumber for consistency:

const EXPORT_JOB_STALE_TIMEOUT_MS = getEnvNumber('EXPORT_JOB_STALE_TIMEOUT_MS', 5 * 60 * 1000, { min: 1 });

3. Defensive Programming in stores/timeline.ts:25
The normalizeTimelineValue function handles edge cases well, but the NaN check could be simplified:

function normalizeTimelineValue(value: number | undefined): number | undefined {
  if (value === undefined) return undefined
  if (!Number.isFinite(value)) return 0  // This already handles NaN
  return Math.max(0, Math.round(value))
}

Potential Bugs & Issues ⚠️

1. Race Condition in app/editor/[projectId]/EditorClient.tsx:377-380

if (EXPORT_POLL_INTERVAL_MS > 0) {
  clearExportPoll()
  exportPollRef.current = setInterval(() => {
    void pollJobStatus()
  }, EXPORT_POLL_INTERVAL_MS)
}

Issue: If EXPORT_POLL_INTERVAL_MS is 0, polling never starts but the promise never resolves unless the job is already complete.
Fix: Either document this behavior or add a fallback mechanism.

2. Incomplete Database Cleanup in app/api/render/route.ts:137

async function cleanupStaleExportJobsForUser(
  supabase: SupabaseClient,
  userId: string
): Promise<void> {
  const threshold = new Date(Date.now() - EXPORT_JOB_STALE_TIMEOUT_MS).toISOString();
  // ...marks jobs as failed but doesn't clean up associated storage
}

Issue: Stale export jobs are marked as failed, but their temporary files or storage artifacts might not be cleaned up.
Recommendation: Consider adding cleanup for associated storage objects.

3. Silent Failure in features/timeline/utils/autosave.ts:176

if (hasTimingDiff) {
  // Refresh store with sanitized values to keep UI state consistent.
  // This update runs while isSaving=true, so AutoSave's mutex prevents recursive saves.
  useTimelineStore.getState().setEffects(sanitizedEffects);
}

Concern: Modifying store state during save operation could lead to unexpected behavior if the save fails. The comment mentions the mutex prevents recursive saves, but there's no error handling if setEffects throws.

4. Truncated Line in Diff Output
The diff shows line 115 in stores/timeline.ts is truncated. Need to verify the complete implementation of the duration calculation.

Performance Considerations 🚀

Positive

• Polling interval configurable: Good that EXPORT_POLL_INTERVAL_MS is configurable
• Queue TTL management: Automatic cleanup of stale jobs prevents memory leaks
• Lazy normalization: Only normalizes when storing/persisting, not on every render

Concerns

1. Redundant Normalization
Timeline values are normalized in multiple places:

• DragHandler/TrimHandler (during user interaction)
• AutoSaveManager (before persistence)
• Timeline store (on setEffects)

Recommendation: Consider normalizing at the boundary (store setters) only, to reduce redundant Math.round() calls.

2. FFmpeg Timeout Implementation in lib/export/server.ts:129

timeoutHandle = setTimeout(() => {
  timedOut = true;
  command.kill("SIGKILL");
}, timeoutSeconds * 1000);

Concern: Using SIGKILL doesn't allow FFmpeg to clean up temporary files.
Recommendation: Use SIGTERM first, then SIGKILL after a grace period.

Security Concerns 🔒

1. Service Role Key Validation ✅
Good that you validate SUPABASE_SERVICE_ROLE_KEY exists before processing exports (route.ts:127-128).

2. Input Validation ✅
Quality validation using type guard is excellent (route.ts:127-130).

3. User Isolation ✅
Export job cleanup is scoped to userId, preventing cross-user data access.

Minor Concerns

1. Error Message Information Disclosure

return NextResponse.json({ error: configError }, { status: 500 });

Consider whether internal configuration errors should be exposed to clients. A generic "Service temporarily unavailable" might be safer.

2. Concurrent Export Limits
Good that MAX_CONCURRENT_EXPORTS_PER_USER is configurable, but ensure this is documented in security guidelines to prevent resource exhaustion attacks.

Test Coverage 📊

Current State

• Unit tests exist for timeline placement logic (tests/unit/timeline.test.ts)
• No tests found for:
  • New normalization functions in DragHandler/TrimHandler
  • AutoSaveManager sanitization logic
  • Export queue TTL management
  • FFmpeg timeout behavior
  • getEnvNumber utility

Recommendations

1. Add unit tests for normalization:

// tests/unit/timeline-normalization.test.ts
describe('Timeline Value Normalization', () => {
  it('should round fractional milliseconds to integers', () => {
    expect(normalizeTimelineValue(1500.7)).toBe(1501)
  })
  
  it('should handle negative values', () => {
    expect(normalizeTimelineValue(-100)).toBe(0)
  })
  
  it('should handle NaN and Infinity', () => {
    expect(normalizeTimelineValue(NaN)).toBe(0)
    expect(normalizeTimelineValue(Infinity)).toBe(0)
  })
})

2. Add integration tests for export queue:

// tests/integration/export-queue.test.ts
describe('Export Queue TTL', () => {
  it('should remove jobs older than TTL', async () => {
    // Test stale job cleanup
  })
  
  it('should respect concurrent export limits', async () => {
    // Test MAX_CONCURRENT_EXPORTS enforcement
  })
})

3. Add tests for env utility:

// tests/unit/env.test.ts
describe('getEnvNumber', () => {
  it('should parse valid numbers', () => {
    process.env.TEST_VAR = '100'
    expect(getEnvNumber('TEST_VAR', 50)).toBe(100)
  })
  
  it('should return default for invalid values', () => {
    process.env.TEST_VAR = 'not-a-number'
    expect(getEnvNumber('TEST_VAR', 50)).toBe(50)
  })
  
  it('should enforce minimum values', () => {
    process.env.TEST_VAR = '5'
    expect(getEnvNumber('TEST_VAR', 50, { min: 10 })).toBe(50)
  })
})

Documentation 📝

Positive

• Environment variables well-documented in README.md
• Inline comments explain complex logic (e.g., mutex behavior in autosave)
• Migration includes helpful header comment

Improvements Needed

1. Add JSDoc for public functions:

/**
 * Safely parse numeric environment variables.
 * Falls back to defaultValue when unset, NaN, or below the optional minimum.
 * 
 * @param key - Environment variable name
 * @param defaultValue - Fallback value
 * @param options - Optional minimum value constraint
 * @returns Parsed number or default
 * 
 * @example
 * const timeout = getEnvNumber('TIMEOUT_MS', 5000, { min: 1000 })
 */
export function getEnvNumber(...)

2. Document the EXPORT_POLL_INTERVAL_MS=0 behavior in README or code comments.

3. Add migration rollback script for 008_export_jobs_updated_at.sql.

Additional Observations

Positive Changes

1. Removed unused variable in app/register-sw.tsx:79 - good cleanup
2. Consistent error handling throughout export pipeline
3. Database trigger for updated_at is a clean solution
4. Removed 480p quality - simplifies the codebase

Questions

1. Is there a reason EXPORT_JOB_STALE_TIMEOUT_MS defaults to 5 minutes while EXPORT_QUEUE_JOB_TTL_MS defaults to 10 minutes? Should these be aligned or is there a specific reason for the difference?

2. The finally block in EditorClient.tsx:378 includes a "safety net" comment - has there been an issue with intervals not being cleared? Consider if this indicates a larger cleanup issue.

Summary

Must Fix 🔴

1. Race condition when EXPORT_POLL_INTERVAL_MS=0
2. Use getEnvNumber for EXPORT_JOB_STALE_TIMEOUT_MS

Should Fix 🟡

1. Add tests for new normalization logic
2. Add tests for getEnvNumber utility
3. Consider storage cleanup in cleanupStaleExportJobsForUser
4. Use SIGTERM before SIGKILL for FFmpeg timeout

Nice to Have 🟢

1. Add JSDoc comments for public APIs
2. Document EXPORT_POLL_INTERVAL_MS=0 behavior
3. Add migration rollback script
4. Reduce redundant normalization calls

Verdict

Approved with minor changes recommended. The core functionality is solid and represents significant improvements to export stability and timeline consistency. The concerns raised are mostly minor and can be addressed in follow-up work. Great job on the comprehensive implementation!

---

Review completed by Claude Code - [Automated PR Review]