Skip to content

fix: catch errors thrown by onSuccess callback to prevent stuck loading state#143

Draft
behnam-oneschema wants to merge 1 commit into
mainfrom
devin/1773954466-fix-onsuccess-error-handling
Draft

fix: catch errors thrown by onSuccess callback to prevent stuck loading state#143
behnam-oneschema wants to merge 1 commit into
mainfrom
devin/1773954466-fix-onsuccess-error-handling

Conversation

@behnam-oneschema
Copy link
Copy Markdown
Member

@behnam-oneschema behnam-oneschema commented Mar 19, 2026

Summary

Fixes #136.

When an onSuccess callback throws (synchronously or via an async rejection), the importer gets stuck in a loading state because cleanup code never executes. This PR adds error handling at two layers:

  1. packages/importer/src/importer.ts — Wraps this.emit("success", ...) in a try/catch inside the "complete" message handler. This ensures resume-token removal and autoClose always run regardless of listener errors. Caught errors are surfaced via the existing error event with Error severity (not Fatal, since the data import itself succeeded).

  2. packages/importer-react/src/OneSchemaImporter.tsx — Wraps the onSuccess?.(data) call to catch both synchronous throws and asynchronous Promise rejections. Errors are forwarded to onError. onRequestClose() is now always called.

Tasks

#136

Review & Testing Checklist for Human

  • Async error ordering: For async onSuccess handlers, onRequestClose fires synchronously (immediately), while onError fires asynchronously (when the Promise rejects). Verify this ordering is acceptable for consumers — the importer will close before onError is called.
  • onError itself throwing: If the consumer's onError handler throws, that error is not caught. Decide if this is acceptable or if it needs a guard too.
  • Scope limited to importer only: The filefeeds package (filefeeds.ts, OneSchemaFileFeeds.tsx) has similar emit/callback patterns but is not patched here. Confirm this is the desired scope.
  • Manual test: Use a React app with onSuccess that throws synchronously and one that returns a rejected Promise. Confirm the importer closes properly and onError is called in both cases.

Notes

  • The onSuccess prop is typed as (data: any) => void, but at runtime consumers commonly pass async functions. The fix captures the return value and checks instanceof Promise to handle this case.
  • Error severity is Error (not Fatal) since the data import completed successfully on the backend — only the consumer's callback failed.

Link to Devin session: https://app.devin.ai/sessions/5f7e446f2a86441b8b4eb120916536af
Requested by: @behnam-oneschema

…ng state

Closes #136

When an onSuccess callback throws (sync or async), the importer could get
stuck in a loading state because cleanup code (resume-token removal,
autoClose, onRequestClose) never executed.

Changes:
- importer.ts: wrap success event emission in try/catch so cleanup always
  runs; surface caught errors via the error event
- OneSchemaImporter.tsx (React): wrap onSuccess call to catch both sync
  throws and async rejections, forward to onError, and always call
  onRequestClose

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration
Copy link
Copy Markdown
Contributor

Original prompt from Behnam 🧑‍💻

propose a fix for #136

@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

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.

onSuccess does not catching errors

1 participant