feat(repository): support existing blob refs for profile avatar/banner#138
feat(repository): support existing blob refs for profile avatar/banner#138
Conversation
- Add BlobInput = Blob | JsonBlobRef type; profile param types updated accordingly - ProfileOperationsImpl.applyImageField now accepts BlobInput: existing JsonBlobRef is converted to BlobRef via fromJsonRef() and stored without re-uploading; new Blob is uploaded as before - Add blobRefToJsonRef() helper in types.ts (BlobRef.ipld()) - Remove stale BlobUploadResult interface (was @deprecated; actual return type of blobs.upload() is BlobRef from @atproto/lexicon)
🦋 Changeset detectedLatest commit: 196fde2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe changes introduce support for reusing existing JsonBlobRef references in profile avatar and banner fields. A new BlobInput type alias unifies Blob and JsonBlobRef inputs, and a blobRefToJsonRef utility function converts BlobRef to JsonBlobRef. The deprecated BlobUploadResult interface is removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for using existing blob references (JsonBlobRef) in profile avatar and banner fields, eliminating unnecessary re-uploads when updating profiles with already-uploaded images. This is part of a larger effort extracted from PR #123.
Changes:
- Introduces
BlobInputunion type (Blob | JsonBlobRef) for avatar/banner fields across all profile parameter types - Updates
ProfileOperationsImpl.applyImageField()to detect and reuse existing JsonBlobRef objects without re-uploading - Adds
blobRefToJsonRef()helper function and removes deprecatedBlobUploadResultinterface
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-core/src/repository/interfaces.ts | Adds BlobInput union type and updates profile parameter types to accept Blob or JsonBlobRef |
| packages/sdk-core/src/repository/ProfileOperationsImpl.ts | Adds isJsonBlobRef type guard and updates applyImageField to handle existing blob refs |
| packages/sdk-core/src/repository/types.ts | Adds blobRefToJsonRef helper and removes deprecated BlobUploadResult interface |
| packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts | Adds test cases for updateBskyProfile and updateCertifiedProfile with existing JsonBlobRef |
| .changeset/profile-blob-input.md | Documents the feature addition and potential breaking change for BlobUploadResult removal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Untyped/legacy form: { cid: string, mimeType: string } | ||
| if (typeof r.cid === "string" && typeof r.mimeType === "string") { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
The "legacy form" check for JsonBlobRef appears to be incorrect. The JsonBlobRef type from @atproto/lexicon always has the typed form with $type: "blob", ref, mimeType, and size fields. The "untyped/legacy form" with just cid and mimeType is not a valid JsonBlobRef structure according to the AT Protocol specification.
If you need to support legacy blob references with a different structure, you should either:
- Remove this legacy check if it's not actually needed
- Document why this legacy form exists and where it comes from
- Ensure that
BlobRef.fromJsonRef()can actually handle this format (it likely cannot)
The first check (lines 135-137) is sufficient for validating JsonBlobRef objects.
| export function blobRefToJsonRef(blobRef: BlobRef): JsonBlobRef { | ||
| return blobRef.ipld(); | ||
| } |
There was a problem hiding this comment.
The blobRefToJsonRef helper function is added but not exported from the main index.ts file. If this helper is intended to be part of the public API for SDK consumers to convert BlobRef instances to their JSON representation, it should be exported.
If it's only for internal use, consider moving it to a different location (e.g., a utils file) or marking the file/section as internal-only. The changeset description indicates this is a public feature ("Add blobRefToJsonRef(blobRef: BlobRef): JsonBlobRef helper"), which suggests it should be exported.
| /** | ||
| * Input type for blob fields — either a new Blob to upload or an existing JsonBlobRef to reuse. | ||
| */ | ||
| export type BlobInput = Blob | JsonBlobRef; |
There was a problem hiding this comment.
The BlobInput type is added to interfaces.ts and documented as a public API type in the changeset, but it's not exported from the main index.ts file. SDK consumers who want to use this type in their TypeScript code for type-checking would not be able to import it.
Consider adding BlobInput to the exports in src/index.ts if it's intended to be part of the public API. This would allow consumers to write type-safe code like:
const avatar: BlobInput = existingBlobRef;
await repo.profile.updateBskyProfile({ avatar });| // If the input is already a JSON blob ref, convert to BlobRef instance for validation | ||
| // and store without re-uploading | ||
| if (this.isJsonBlobRef(input)) { | ||
| const blobRef = LexiconBlobRef.fromJsonRef(input); |
There was a problem hiding this comment.
The call to LexiconBlobRef.fromJsonRef(input) is not wrapped in error handling. If the input is malformed or doesn't match the expected structure (despite passing the isJsonBlobRef check), this could throw an error that isn't caught and wrapped appropriately.
Consider adding a try-catch block around this conversion to provide clearer error messages if the conversion fails, or ensure that isJsonBlobRef validates all fields that fromJsonRef requires. The current type guard only checks for the presence and types of fields, but doesn't validate the structure of the ref field.
| const blobRef = LexiconBlobRef.fromJsonRef(input); | |
| let blobRef: LexiconBlobRef; | |
| try { | |
| blobRef = LexiconBlobRef.fromJsonRef(input as JsonBlobRef); | |
| } catch (err) { | |
| // Wrap low-level conversion errors in a domain-specific validation error | |
| throw new ValidationError( | |
| `Invalid blob reference for field "${field}" in collection "${collection}": ${JSON.stringify(input)}`, | |
| ); | |
| } |
| * Type guard to check if a value is a JsonBlobRef (already-uploaded blob reference). | ||
| * | ||
| * Supports both typed form (`{ $type: "blob", ref, mimeType, size }`) and | ||
| * untyped/legacy form (`{ cid: string, mimeType: string }`). |
There was a problem hiding this comment.
The JSDoc comment mentions "untyped/legacy form" which is checked in the implementation, but this form is likely not a valid JsonBlobRef according to the AT Protocol specification. This documentation should be updated to either:
- Remove the mention of the legacy form if it's not actually supported
- Clearly document where this legacy form comes from and why it needs to be supported
The inconsistency between the documentation and the actual JsonBlobRef type could lead to confusion for developers.
| * Type guard to check if a value is a JsonBlobRef (already-uploaded blob reference). | |
| * | |
| * Supports both typed form (`{ $type: "blob", ref, mimeType, size }`) and | |
| * untyped/legacy form (`{ cid: string, mimeType: string }`). | |
| * Type guard to check if a value is an AT Protocol `JsonBlobRef` (already-uploaded blob reference). | |
| * | |
| * Primarily supports the spec-compliant typed form (`{ $type: "blob", ref, mimeType, size }`). | |
| * For backward compatibility, it also treats a legacy/untyped form (`{ cid: string, mimeType: string }`) | |
| * as acceptable input. This legacy shape originates from earlier Hypercerts profile image handling | |
| * prior to adopting the formal `JsonBlobRef` schema and is not part of the official AT Protocol spec. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/sdk-core/src/repository/ProfileOperationsImpl.ts (1)
189-201: Theas Blobcast on line 190 is safe but redundant.After the
undefined,null, andisJsonBlobRef()checks, the remaining type in theBlobInputunion is alreadyBlobfrom TypeScript's perspective. The explicit castinput as Blobis harmless and arguably aids readability, but it could mask issues ifBlobInputis widened in the future without updating this method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts` around lines 189 - 201, Remove the redundant type assertion on the Blob upload call: after the null/undefined/isJsonBlobRef() checks the local variable input is already narrowed to Blob, so call this.blobs.upload(input) without the "as Blob" cast; update the usage near blobRef assignment in ProfileOperationsImpl (the block that creates blobRef and assigns result[field] based on BSKY_PROFILE_NSID/field/banner) to use the narrowed input directly.packages/sdk-core/src/repository/interfaces.ts (1)
675-678:BlobInputis wedged between theProfileOperationsJSDoc and the first helper type.The
BlobInputtype is inserted directly after theProfileOperationsJSDoc block (line 674), which could make the JSDoc appear to documentBlobInputrather than theProfileOperationsinterface at line 738. Consider movingBlobInputabove theProfileOperationsJSDoc or into a dedicated "Blob Types" section alongside the blob-related definitions.That said, the pre-existing code already had several types (e.g.,
BskyProfile,CertifiedProfile) between the JSDoc and the interface, so this is consistent with the current layout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/repository/interfaces.ts` around lines 675 - 678, The BlobInput type is declared immediately after the JSDoc for the ProfileOperations interface which can make the JSDoc appear to document BlobInput; move the BlobInput declaration (export type BlobInput = Blob | JsonBlobRef) so it appears before the JSDoc for ProfileOperations or relocate it into a dedicated "Blob Types" section with other blob-related types (e.g., alongside JsonBlobRef), then verify the ProfileOperations interface declaration remains directly after its JSDoc and update any nearby type order to keep documentation and code grouping consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/sdk-core/src/repository/interfaces.ts`:
- Around line 675-678: The BlobInput type is declared immediately after the
JSDoc for the ProfileOperations interface which can make the JSDoc appear to
document BlobInput; move the BlobInput declaration (export type BlobInput = Blob
| JsonBlobRef) so it appears before the JSDoc for ProfileOperations or relocate
it into a dedicated "Blob Types" section with other blob-related types (e.g.,
alongside JsonBlobRef), then verify the ProfileOperations interface declaration
remains directly after its JSDoc and update any nearby type order to keep
documentation and code grouping consistent.
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 189-201: Remove the redundant type assertion on the Blob upload
call: after the null/undefined/isJsonBlobRef() checks the local variable input
is already narrowed to Blob, so call this.blobs.upload(input) without the "as
Blob" cast; update the usage near blobRef assignment in ProfileOperationsImpl
(the block that creates blobRef and assigns result[field] based on
BSKY_PROFILE_NSID/field/banner) to use the narrowed input directly.
1 participant
Summary
Ports the remaining profile-related changes from PR #123 (
bannerbranch) onto a fresh branch offdevelop.BlobInput = Blob | JsonBlobRefunion type; all profile param types (CreateBskyProfileParams,UpdateBskyProfileParams,CreateCertifiedProfileParams,UpdateCertifiedProfileParams) now accept either a newBlobto upload or an existingJsonBlobRefto reuse without re-uploadingProfileOperationsImpl.applyImageFieldupdated to handle theJsonBlobRefpath: converts toBlobRefviaBlobRef.fromJsonRef()(required for AT Protocol lexicon validators) and stores directlyblobRefToJsonRef(blobRef: BlobRef): JsonBlobRefhelper (wrapsblobRef.ipld())BlobUploadResultinterface (was already@deprecated;blobs.upload()returnsBlobReffrom@atproto/lexicon)Test coverage
Two new test cases added to
ProfileOperationsImpl.test.ts:updateBskyProfilewith existingJsonBlobRefavatar → no upload, stored asBlobRefinstanceupdateCertifiedProfilewith existingJsonBlobRefavatar → no upload, stored wrapped insmallImageRelated
Extracted from #123. Also see #136, #137 for other extractions from the same PR.
Summary by CodeRabbit
New Features
Breaking Changes
BlobUploadResultinterface removed; migrate toBlobReffrom@atproto/lexiconinstead.