fix(firebase): return content type and plain file name from getFile - #947
Open
marianmoldovan wants to merge 1 commit into
Open
fix(firebase): return content type and plain file name from getFile#947marianmoldovan wants to merge 1 commit into
marianmoldovan wants to merge 1 commit into
Conversation
`getFile` built its result with `new File([blob], path)`, omitting the third `options` argument of the `File` constructor. Without it `File.type` defaults to the empty string, so callers could not tell what kind of file they had received even though the content type was already sitting on the fetched `blob`. The same call also used the full storage path as the `File` name, yielding names like `images/events/abc.png`. A `File` name is a name, not a path, and the slashes leak into consumers: the default filename builder in `useStorageUploadController` composes `randomString() + "_" + file.name`, so re-uploading a fetched file created spurious nested folders in Storage. Pass `blob.type` through as the content type and use `fileRef.name` (the last path component, exposed by the Storage reference) as the file name. `blob.type` is the `Content-Type` that Firebase Storage serves for the object, i.e. its stored `contentType` metadata, so no extra `getMetadata` round-trip is needed. Fixes #556 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes StorageSource.getFile in the Firebase Storage implementation so returned File objects carry the correct MIME type and a plain filename (instead of the full storage path), aligning the behavior with the stored object metadata and preventing path-like File.name values.
Changes:
- Pass
{ type: blob.type }to theFileconstructor soFile.typeis not empty. - Use
fileRef.name(last path component) instead of the full storagepathforFile.name. - Add explanatory inline comments documenting the rationale for both changes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
137
to
138
| const response = await fetch(url); | ||
| const blob = await response.blob(); |
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.
Fixes #556
The bug
getFilebuilt its result withnew File([blob], path), omitting the constructor's third argument. Without itFile.typeis"", even though the content type was already sitting on the fetchedblob.The fix
Two changes, both deliberate:
{ type: blob.type }— Firebase serves the object's storedcontentTypeas the responseContent-Type, soblob.typeis the stored metadata, arriving free with a response we already await. An extragetMetadata()round-trip would return the identical string.fileRef.nameinstead ofpath— the full storage path was used as theFilename, giving names likeimages/events/abc.png. Not cosmetic: the default filename builder composesrandomString() + "_" + file.name, so re-uploading a fetched file created spurious nested pseudo-folders in Storage.fileRef.nameis documented by@firebase/storageas the last path component, which avoids hand-rolling trailing-slash edge cases.Blast radius checked first: only three
getFilereferences exist in source (the interface, this implementation, and a throwing mock) and none readsFile.name.Not verified
packages/firebase_firecmshas no test runner wired up, so no tests were added for a one-line fix.tsc --noEmitis clean for this file. Not exercised against live Firebase Storage — theblob.typebehaviour rests on documented behaviour, not an observed round-trip.🤖 Generated with Claude Code