Add async processing of PDF imports

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 3aa4e8e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

Pull Request Review: Add async processing of PDF imports

Summary

This PR implements a robust async job processing system for PDF imports, converting from synchronous to asynchronous processing. The implementation includes database migrations, repository layer, service layer updates, and comprehensive UI feedback for processing states.

✅ Strengths

Architecture & Design

• Well-designed job system: The form_jobs table with extensible job types is well-thought-out and future-proof
• Proper separation of concerns: Clear layering between repository, service, and UI components
• Type-safe job metadata: Using discriminated unions for JobMetadata and JobResult by job type is excellent
• Comprehensive indexing: Migration includes appropriate indexes for common query patterns
• Cascade delete: Proper foreign key relationship with ON DELETE CASCADE

Code Quality

• Excellent test coverage: Both unit and integration tests for job operations (273 lines in form-jobs.test.ts)
• Error handling: Comprehensive try-catch blocks with proper error propagation to job status
• UI/UX: Great processing indicators with elapsed time, spinner animations, and error states
• Accessibility: Proper ARIA attributes (role="status", aria-live="polite", etc.)

🔍 Issues & Concerns

1. CRITICAL: Race Condition in Async Processing ⚠️

Location: packages/forms/src/services/initialize-form.ts:142

processFormDocumentAsync(ctx, formId, job.id, documentId).catch(err => {
  console.error('Async form processing failed:', err);
  // Error already logged to database by processFormDocumentAsync
});

Issue: The async function is fire-and-forget with no monitoring or recovery mechanism. If the Node.js process crashes or restarts before processing completes, the job will be stuck in "processing" state forever.

Recommendations:

• Add a background worker that periodically checks for stale processing jobs (e.g., processing for >5 minutes)
• Consider using a proper job queue (Bull, BullMQ, pg-boss) for production reliability
• Add a timeout_at timestamp field to detect stuck jobs
• Implement job retry logic with exponential backoff

2. Security: Error Stack Exposure

Location: packages/design/src/AvailableFormList/index.tsx:186-195

{showError && form.latestJob.errorMessage && (
  <details className="margin-top-1">
    <summary>Technical details</summary>
    <pre className="font-mono-2xs margin-top-1 padding-1 bg-base-lightest">
      {form.latestJob.errorMessage}
    </pre>
  </details>
)}

Issue: Displaying full error messages (potentially including stack traces) to end users could expose sensitive system information, internal paths, or library versions.

Recommendation:

• Only show sanitized, user-friendly error messages in the UI
• Log detailed errors server-side only
• Consider adding an isAdmin check before showing technical details

3. Performance: Polling Inefficiency

Location: packages/design/src/AvailableFormList/index.tsx:37-60

if (hasProcessingForms) {
  pollIntervalRef.current = setInterval(() => {
    loadForms();
  }, 3000); // Poll every 3 seconds
}

Issues:

• Polls the entire form list every 3 seconds if any form is processing
• No exponential backoff for long-running jobs
• Multiple browser tabs would create multiple polling loops

Recommendations:

• Implement WebSocket or Server-Sent Events for real-time updates
• Use exponential backoff (3s → 5s → 10s → 30s)
• Poll only the specific form status endpoint, not the entire form list
• Use a shared worker or broadcast channel to coordinate polling across tabs

4. Type Safety Issue

Location: packages/forms/src/repository/form-jobs.ts:82,119,148

created_at: now.toISOString() as any,
completed_at: new Date().toISOString() as any,

Issue: Using as any bypasses TypeScript's type checking, potentially hiding type mismatches between SQLite (string) and PostgreSQL (Date).

Recommendation: Use the DbDate<T> generic type that's already defined in the types file to handle this properly.

5. Missing Job Cleanup

Location: Database migration doesn't include a cleanup strategy

Issue: Completed/failed jobs will accumulate indefinitely, potentially growing the table unbounded.

Recommendations:

• Add a scheduled job to archive/delete old completed jobs (e.g., >30 days)
• Consider partitioning the form_jobs table by date
• Add a retention_until timestamp field

6. Inconsistent Error Handling

Location: packages/forms/src/services/initialize-form.ts:229-236

catch (err) {
  console.error('Unexpected error in processFormDocumentAsync:', err);
  await ctx.repository.failFormJob(jobId, {
    message: (err as Error).message,
    stack: (err as Error).stack,
  });
}

Issue: The failFormJob call itself could throw an error, and there's no handling for that scenario. Also assumes err is an Error object without checking.

Recommendation:

catch (err) {
  console.error('Unexpected error in processFormDocumentAsync:', err);
  try {
    await ctx.repository.failFormJob(jobId, {
      message: err instanceof Error ? err.message : String(err),
      stack: err instanceof Error ? err.stack : undefined,
    });
  } catch (failErr) {
    console.error('Failed to mark job as failed:', failErr);
    // Consider alerting/monitoring here
  }
}

7. Missing Database Migration Rollback Test

The migration has a down function but there's no test verifying it works correctly. Rollback failures can be catastrophic in production.

8. Zod Version Compatibility

Location: packages/forms/src/util/zod.ts:7

The PR includes a Zod 3→4 compatibility fix, but it's unclear if this is tested across both versions. Consider documenting the minimum required Zod version.

💡 Minor Suggestions

1. Magic Numbers: The 3-second polling interval and 60-second timeout thresholds should be constants/config

   const POLL_INTERVAL_MS = 3000;
   const LONG_RUNNING_THRESHOLD_MS = 60000;

2. Accessibility: The elapsed time display should update via aria-live region for screen readers

3. Memory Leak Prevention: In FormProcessingIndicator.tsx:31-44, verify the interval cleanup happens on unmount even if the component re-renders

4. HTTP Status Endpoint: packages/server/src/pages/api/forms/[id]/status.ts looks minimal - ensure it has proper error handling and rate limiting

5. Database Date Handling: The fix for SQLite date issues in packages/forms/src/llm/cache/backends/database.ts:4 is good, but consider documenting why this is needed

6. Storybook Stories: Excellent coverage in FormProcessingIndicator.stories.tsx - consider adding interaction tests with @storybook/test

🧪 Test Coverage Assessment

Excellent:

• Job CRUD operations fully tested
• Edge cases covered (null results, chronological ordering, type filtering)
• Both SQLite and PostgreSQL tested via describeDatabase

Missing:

• Integration test for the full async PDF import flow
• Test for job timeout/stale job detection (not implemented yet)
• Test for concurrent job creation
• UI component tests with @vitest/browser (mentioned in CLAUDE.md)

🔒 Security Assessment

Low Risk Items:

• Input validation with Zod schemas ✅
• Base64 validation before processing ✅
• User authentication checks ✅

Medium Risk Items:

• Error message exposure (mentioned above)
• No rate limiting mentioned for job creation
• No mention of file size limits for PDF uploads

📊 Performance Considerations

1. Database Impact: The 4 indexes on form_jobs are appropriate, but monitor index usage in production
2. Polling Load: Current implementation could generate significant load with many concurrent users
3. Memory: Fire-and-forget async processing could cause memory issues if many PDFs are processing simultaneously - consider a queue with concurrency limits

✨ Overall Assessment

Score: 8/10

This is a high-quality PR with excellent architecture, comprehensive testing, and thoughtful UX design. The async job system is well-designed and extensible. The main concerns are around production reliability (stuck jobs, polling efficiency) and security (error exposure). These should be addressed before merging to production, though they don't prevent merging to a development environment.

Recommendation: Approve with required changes for production deployment.

Required Before Production:

1. Implement stuck job detection/cleanup
2. Sanitize error messages shown to users
3. Add rate limiting for job creation
4. Consider WebSocket/SSE instead of polling

Nice to Have:

1. Job retry logic
2. Exponential backoff for polling
3. Integration test for full async flow
4. Job history cleanup strategy

---

Great work on this feature! The foundation is solid and the code quality is excellent. 🚀

Generated with Claude Code

[claude]

PR Review: Add async processing of PDF imports

Summary

This PR implements a sophisticated async job processing system for PDF imports, replacing synchronous processing with a background task queue. The implementation is well-structured with good separation of concerns.

✅ Strengths

Architecture & Design

• Excellent job system design: The form_jobs table is well-designed with extensible job types, proper indexing, and comprehensive metadata/result tracking
• Type-safe job handling: The use of TypeScript discriminated unions for JobMetadata and JobResult by job type is excellent (lines 5-37 in form-jobs.ts)
• Proper async pattern: The immediate return with background processing in initializeForm follows best practices for responsive APIs
• Good separation of concerns: Job management logic is cleanly separated into dedicated repository functions
• Conditional exports for zero-build dev: The addition of conditional exports is a significant DX improvement

Code Quality

• Comprehensive test coverage: Both unit and integration tests for job lifecycle and async processing
• Good error handling: Errors are caught and properly recorded in the database with stack traces
• Clean migration: Database migration is well-documented with inline comments explaining field purposes

UX

• Status polling: Frontend automatically polls for processing status and displays appropriate indicators
• Error visibility: Failed imports show inline error messages with expandable details
• Processing indicators: Visual feedback with animated spinners for processing state

🔍 Issues & Concerns

🔴 Critical Issues

1. Race condition in form status check (get-form-status.ts:51)

const hasContent = Object.keys(form.patterns).length > 1;

This check is racy: if the async processing hasn't completed but hasn't failed, the form could show as "draft" when it should show as "processing". The status should be derived from the job state, not pattern count.

Recommendation: Check job status first - if there's an active job, return processing state regardless of pattern count.

2. Unhandled promise rejection (initialize-form.ts:142)

processFormDocumentAsync(ctx, formId, job.id, documentId).catch(err => {
  console.error('Async form processing failed:', err);
  // Error already logged to database by processFormDocumentAsync
});

While the catch prevents unhandled rejection, errors logged only to console won't be visible in production. Consider adding proper logging/monitoring integration.

3. Date handling inconsistencies (form-jobs.ts:82, 119, 148)

created_at: now.toISOString() as any,

Multiple as any casts for dates suggest type system issues. This could cause runtime errors with SQLite vs PostgreSQL date handling differences.

Recommendation: Use the DbDate<T> type helper already defined in the database types, or ensure date serialization is consistent across database engines.

⚠️ Medium Priority Issues

4. No job cleanup/retention policy
Jobs accumulate indefinitely with CASCADE delete only. Failed jobs with error stacks could consume significant storage over time.

Recommendation: Add a retention policy (e.g., delete completed jobs after 30 days, keep failed jobs longer for debugging).

5. No retry mechanism
Failed jobs have no automatic retry. Transient failures (network issues, temporary AWS Bedrock unavailability) require manual intervention.

Recommendation: Add retry logic with exponential backoff, or at minimum document the manual retry process.

6. Polling inefficiency (useFormStatus.ts:93)

pollIntervalRef.current = setInterval(() => {
  fetchStatus();
}, pollInterval);

Every processing form polls every 2-3 seconds. With many users, this could overwhelm the server.

Recommendation: Consider WebSocket/SSE for real-time updates, or implement exponential backoff (2s → 5s → 10s).

7. Missing concurrency control
No mechanism prevents multiple concurrent processing jobs for the same form. Could lead to race conditions if a user uploads multiple PDFs rapidly.

Recommendation: Add a check in createFormJob to prevent creating a new job if one is already processing.

💡 Minor Issues

8. Inconsistent error handling in frontend (FormStatusBadge.tsx:47, AvailableFormList/index.tsx:34)
The form list polling doesn't handle errors from getFormList(), and FormStatusBadge doesn't handle cases where latestJob exists but has unexpected status values.

9. Magic numbers (AvailableFormList/index.tsx:50)

}, 3000); // Poll every 3 seconds

Hardcoded poll interval differs from useFormStatus hook (2000ms). Should be a shared constant.

10. Type cast safety (form-jobs.ts:96, 204)

return success({...}) as any;

Multiple as any casts bypass type checking. These should use proper type assertions or restructure the return type.

11. Unused schema file removed
The packages/database/src/schema.ts file was deleted but may be referenced elsewhere. Verify no broken imports.

12. Conditional exports documentation
Excellent addition to CLAUDE.md, but the package.json exports should include comments explaining the development condition purpose for future maintainers.

🔒 Security Considerations

✅ Good: User authentication check before job creation
✅ Good: Error messages don't leak sensitive data
⚠️ Concern: Error stack traces stored in database could leak file paths/internals. Consider sanitizing or restricting access.

🚀 Performance Considerations

• Database indexing: Excellent indexes on form_jobs table for common query patterns
• Async processing: Properly decouples slow PDF parsing from HTTP request/response cycle
• Polling overhead: As mentioned, polling every 2-3 seconds per active form could be optimized

📝 Test Coverage

✅ Comprehensive tests for:

• Job creation, completion, and failure flows
• Latest job retrieval
• Async processing with mocked PDF parsing
• Database cache backend fixes for SQLite date issues

⚠️ Missing tests for:

• Concurrent job creation (race conditions)
• Job retention/cleanup
• Polling behavior in frontend hooks
• Error cases in processFormDocumentAsync

📊 Code Metrics

• Additions: 3,047 lines (mostly new features + conditional exports)
• Deletions: 363 lines (old experiments, schema cleanup)
• Files changed: 65 files (significant refactor)
• Migration: 58 lines (well-structured)

🎯 Recommendations

Must Fix Before Merge

1. Fix race condition in form status determination (issue Deploy to demo #1)
2. Resolve date type casting issues (issue Changesets publish release #3)
3. Add concurrency control to prevent duplicate jobs (issue Version Packages #7)

Should Address Soon

4. Implement job retention policy (issue Remove tests from precommit hook #4)
5. Add retry mechanism or document manual process (issue Version Packages #5)
6. Optimize polling strategy (issue Demo tweaks #6)

Nice to Have

7. Add proper logging/monitoring integration (issue Flexion forms release #2)
8. Standardize poll intervals across codebase (issue Add Claude Code GitHub Workflow #9)
9. Sanitize error stacks before database storage

✨ Overall Assessment

This is a well-architected feature that significantly improves the user experience for PDF imports. The job system is extensible and follows good patterns. The main concerns are around race conditions, error handling, and operational considerations (monitoring, cleanup, retries).

Recommendation: Address critical issues #1 and #3, add concurrency control (#7), then merge. Issues #4-6 can be follow-up PRs.

Great work on the conditional exports - this will significantly improve the development experience! 🎉