fix: remove async executor anti-pattern in processScreenshotWithDefaultBackground#3
Open
zbruhnke wants to merge 1 commit intoKartikLabhshetwar:mainfrom
Open
fix: remove async executor anti-pattern in processScreenshotWithDefaultBackground#3zbruhnke wants to merge 1 commit intoKartikLabhshetwar:mainfrom
zbruhnke wants to merge 1 commit intoKartikLabhshetwar:mainfrom
Conversation
…ltBackground
The function was using `new Promise(async (resolve, reject) => { ... })`,
which is a known anti-pattern that causes errors thrown before the first
`await` to become unhandled rejections instead of properly rejecting the
promise.
## The Problem
When an async executor throws synchronously (before any await), the error
escapes the Promise constructor and becomes an unhandled rejection:
```typescript
// Anti-pattern - errors before first await don't reject the promise
return new Promise(async (resolve, reject) => {
validateInput(input); // If this throws, promise never settles!
const result = await asyncOp();
resolve(result);
});
```
This means:
- The promise hangs forever (never resolves or rejects)
- Callers' try/catch blocks can't catch the error
- `.catch()` handlers are never invoked
- The error is silently lost as an unhandled rejection
## The Fix
Refactored to use proper async/await pattern with helper functions that
wrap callback-based APIs (Image.onload, canvas.toBlob, FileReader):
- `loadImage(src)`: Wraps Image loading in a proper Promise
- `canvasToDataUrl(canvas)`: Wraps toBlob + FileReader in a proper Promise
- Main function is now a clean async function using await
## Benefits
- All errors (sync and async) properly propagate through the promise chain
- Code reduced from ~90 lines to ~70 lines
- Flat structure instead of 4+ levels of nested callbacks
- Helper functions are reusable
- Easier to read, test, and maintain
## Testing
Added comprehensive test suite demonstrating the anti-pattern:
- Tests that pass for both old and new implementations (basic functionality)
- Tests that expose the bug (correct pattern passes, anti-pattern fails)
- Real-world scenario showing how errors get lost with the anti-pattern
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@zbruhnke is attempting to deploy a commit to the knox projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the async executor anti-pattern in
processScreenshotWithDefaultBackground()which could cause silent error loss and hanging promises.new Promise(async ...)anti-pattern to use proper async/awaitThe Problem
The original code used
new Promise(async (resolve, reject) => { ... }), which is a well-known anti-pattern. When an async executor throws an error before the firstawait, the error escapes the Promise constructor and becomes an unhandled rejection:Consequences:
try/catchblocks can't catch the error.catch()handlers are never invokedThe Fix
Refactored to use proper async/await with helper functions that wrap callback-based browser APIs:
Benefits
Test Plan
Added
src/lib/promise-anti-pattern.test.tswith comprehensive tests:anti-pattern: sync error causes promise to hang (never settles)anti-pattern: cannot use .catch() to handle sync errorsdemonstrates lost errors - callers think everything is fineRun tests with:
npm testNote: The anti-pattern demonstration tests intentionally cause unhandled rejections (that's the bug being demonstrated). Vitest reports these, which proves the anti-pattern is problematic.
🤖 Generated with Claude Code