-
-
Notifications
You must be signed in to change notification settings - Fork 32
fix: add error handling for base64 decode operations #1237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
f0ba724 to
6ffe600
Compare
packages/spotlight/src/ui/telemetry/components/insights/envelopes/Attachment.tsx
Outdated
Show resolved
Hide resolved
packages/spotlight/src/ui/telemetry/components/insights/envelopes/Attachment.tsx
Outdated
Show resolved
Hide resolved
Fixes SPOTLIGHT-ELECTRON-4C Fixes SPOTLIGHT-ELECTRON-4G The atob() function throws InvalidCharacterError when decoding invalid base64 strings. This was causing crashes when viewing attachments with corrupted or malformed base64 data. Changes: - Update base64Decode() to return null on decode failure - Add error handling in Attachment.tsx with user-friendly error message - Add error handling in QuerySummary.tsx for URL param decode failures
6ffe600 to
e449aa3
Compare
|
Addressed both review comments:
The new approach uses
Since |
| } else { | ||
| return undefined; // Not a binary type, no blob URL needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The refactoring to useMemo for downloadUrl removed the fallback for text-based attachments, causing the download link to become non-functional for types like .txt, .csv, and .json.
Severity: CRITICAL
🔍 Detailed Analysis
The refactoring of the download URL generation logic within the Attachment component has introduced a regression. Previously, a Blob was created for text-based attachments using the raw attachment string. The new implementation inside useMemo handles image, video, and binary content types but returns undefined for all other types, including common text formats like text/plain, text/csv, and text/json. As a result, the downloadUrl variable becomes undefined for these attachments, rendering the download link inoperable and preventing users from downloading these files.
💡 Suggested Fix
Restore the logic to handle text-based attachments. Inside the useMemo hook, add a final else block that creates a Blob from the raw attachment string for text content types and returns its URL via URL.createObjectURL, similar to the old implementation. This will ensure download links are correctly generated for all supported attachment types.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/spotlight/src/ui/telemetry/components/insights/envelopes/Attachment.tsx#L48-L49
Potential issue: The refactoring of the download URL generation logic within the
`Attachment` component has introduced a regression. Previously, a `Blob` was created for
text-based attachments using the raw attachment string. The new implementation inside
`useMemo` handles image, video, and binary content types but returns `undefined` for all
other types, including common text formats like `text/plain`, `text/csv`, and
`text/json`. As a result, the `downloadUrl` variable becomes `undefined` for these
attachments, rendering the download link inoperable and preventing users from
downloading these files.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8364912
| } | ||
| blobData = decoded; | ||
| } else { | ||
| return undefined; // Not a binary type, no blob URL needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Download broken for text-based attachment content types
High Severity
The refactored downloadUrl logic only creates blob URLs for image, video, and "bin" extension types. For all other content types like text/plain, text/csv, JSON, and code files, downloadUrl is set to undefined. The old code created a blob URL for ALL content types by using attachment directly for text-based types. This breaks the download link (href={downloadUrl ?? undefined}) for non-binary attachments, as the href will be undefined and clicking it won't trigger a download. The PR description states users should "still allowing raw envelope download" but this functionality is now broken for text-based content types.
🔬 Verification Test
Why verification test was not possible: This is a React component that requires a full browser/DOM environment to test the blob URL creation and download functionality. The bug is evident from code analysis - the old code had a fallback branch that used attachment directly for non-binary types to create blob URLs, while the new code returns undefined for these cases, leaving the download <a> tag without a valid href.
The refactoring in #1237 accidentally broke download links for text-based attachments (txt, csv, json, code files) by returning undefined instead of creating a blob URL. This restores the original behavior where all content types get a blob URL for downloading.
The refactoring in #1237 accidentally broke download links for text-based attachments (txt, csv, json, code files) by returning undefined instead of creating a blob URL. This restores the original behavior where all content types get a blob URL for downloading. Also adds comprehensive unit tests for the Attachment component to prevent future regressions: - Download URL generation for various content types - Error handling for invalid base64 data - Content preview rendering - Blob URL cleanup on unmount
## Problem The refactoring in #1237 accidentally broke download links for text-based attachments (txt, csv, json, code files) by returning `undefined` instead of creating a blob URL. ## Solution Restore the original behavior where all content types get a blob URL for downloading. The `else` branch now creates a blob from the raw attachment string instead of returning `undefined`. ## Testing All 172 tests pass.
Fixes SPOTLIGHT-ELECTRON-4C
Fixes SPOTLIGHT-ELECTRON-4G
Problem
The
atob()function throwsInvalidCharacterErrorwhen decoding invalid base64 strings. This was causing crashes in the Electron app when viewing attachments with corrupted or malformed base64 data (10 events, 2+ users affected).Solution
Add error handling to all
atob()/base64Decode()call sites:base64.ts: Updatebase64Decode()to returnnullon decode failure instead of throwingAttachment.tsx: Handle decode failures gracefully and show user-friendly error message while still allowing raw envelope downloadQuerySummary.tsx: Wrap URL param decode in try-catch to handle malformed URLsTesting
All 172 existing tests pass.