Remove Object URL usage for image previews #6524
Conversation
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 11/01/2025, 11:48:21 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 11/02/2025, 12:01:22 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.27 MB (baseline 3.27 MB) • 🔴 +41 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 724 kB (baseline 724 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 8.18 kB (baseline 8.18 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 295 kB (baseline 295 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 BReusable component library chunks
Status: 1 added / 1 removed Data & Services — 11.4 kB (baseline 11.4 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 1 added / 1 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 2.55 MB (baseline 2.55 MB) • ⚪ 0 BBundles that do not match a named category
|
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (props.task.displayStatus === TaskItemDisplayStatus.Running) { | ||
| if (progressPreviewBlobUrl.value) { | ||
| URL.revokeObjectURL(progressPreviewBlobUrl.value) | ||
| const reader = new FileReader() |
There was a problem hiding this comment.
[quality] high Priority
Issue: FileReader usage creates potential performance issue
Context: FileReader.readAsDataURL is synchronous and blocks the main thread. For large blob files, this could cause UI stuttering during progress preview updates.
Suggestion: Consider using URL.createObjectURL for better performance and only switching to data URLs if memory usage becomes problematic. The original Object URL approach was more performant.
| if (progressPreviewBlobUrl.value) { | ||
| URL.revokeObjectURL(progressPreviewBlobUrl.value) | ||
| const reader = new FileReader() | ||
| reader.onloadend = () => { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Missing error handling for FileReader operations
Context: FileReader operations can fail (e.g., corrupted blob, out of memory), but there's no error handler defined. This could lead to silent failures where progress previews don't show.
Suggestion: Add reader.onerror handler to gracefully handle failures and provide fallback behavior or user feedback.
| const blobUrl = URL.createObjectURL(blob) | ||
| // Preview cleanup is handled in progress_state event to support multiple concurrent previews | ||
| const { setNodePreviewsByExecutionId } = useNodeOutputStore() | ||
| const reader = new FileReader() |
There was a problem hiding this comment.
[performance] high Priority
Issue: Same performance concern as TaskItem.vue - FileReader blocks main thread
Context: FileReader.readAsDataURL is synchronous and will block the UI thread when processing large blobs. This is especially problematic for preview updates which should be fast and non-blocking.
Suggestion: Consider keeping URL.createObjectURL for performance-critical preview operations, or implement the change incrementally with performance monitoring.
| setNodePreviewsByExecutionId(nodeParents.slice(0, i).join(':'), [ | ||
| blobUrl | ||
| ]) | ||
| reader.onloadend = () => { |
There was a problem hiding this comment.
[quality] medium Priority
Issue: Missing error handling for FileReader operations
Context: Same as TaskItem.vue - no error handler defined for FileReader operations. Critical for preview functionality reliability.
Suggestion: Add reader.onerror handler to gracefully handle failures and provide fallback behavior.
| URL.revokeObjectURL(url) | ||
| } | ||
|
|
||
| delete app.nodePreviewImages[nodeLocatorId] |
There was a problem hiding this comment.
[architecture] high Priority
Issue: Removed URL cleanup logic without ensuring consistency
Context: The revokePreviewsByLocatorId function removed URL.revokeObjectURL cleanup, but this function may still receive Object URLs from other code paths that weren't converted to data URLs.
Suggestion: Add conditional cleanup logic that checks if URLs are Object URLs (start with 'blob:') before attempting to revoke, ensuring backward compatibility during the transition period.
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: Remove Object URL usage for image previews (#6524)
Impact: 15 additions, 17 deletions across 3 files
Issue Distribution
- Critical: 0
- High: 3
- Medium: 2
- Low: 0
Category Breakdown
- Architecture: 1 issue
- Performance: 1 issue
- Code Quality: 3 issues
Key Findings
Performance Impact
The switch from Object URLs to Data URLs addresses memory leak concerns but introduces significant performance trade-offs:
- FileReader.readAsDataURL is synchronous and blocks the main thread
- Data URLs are larger in memory (base64 encoding overhead)
- Performance impact will be most noticeable with large image previews
Code Quality Issues
- Missing error handling for all FileReader operations
- Inconsistent cleanup logic (URL.revokeObjectURL still called in TaskItem.vue)
- Pattern deviation from VueUse composables without clear justification
Architecture Concerns
- Removed URL cleanup logic without ensuring consistency across all code paths
- Mixed concerns: memory leak fix bundled with unrelated handleFile method refactor
Positive Observations
- Clear intention to address reported memory leak issues
- Consistent application of FileReader pattern across affected files
- Good isolation of changes to preview-related functionality
Recommendations
Immediate Actions (Before Merge)
- Add error handling for all FileReader operations
- Remove unnecessary URL.revokeObjectURL calls for data URLs
- Consider splitting the handleFile refactor into a separate PR
Performance Monitoring
- Implement performance benchmarks for preview loading
- Monitor memory usage patterns with the new approach
- Consider hybrid approach: Object URLs for small images, data URLs for large ones
Testing Requirements
- Test with various image sizes to validate performance impact
- Test error scenarios (corrupted blobs, OOM conditions)
- Verify no memory leaks remain with the new approach
This is a comprehensive automated review focusing on the Object URL to Data URL conversion and its implications.
github-actions
bot
removed
the
claude-review
If an Object URL is not revoked after being created, it causes a memory leak. Data URLs don't have this issue, but they can clutter the DOM, require more memory due to the base64 encoding, and may result in duplicate copies being kept in memory.
This is a WIP testing branch to determine
┆Issue is synchronized with this Notion page by Unito