Conversation
- Add BlobInput type (Blob | JsonBlobRef) to interfaces.ts - Update ProfileParams to accept BlobInput for avatar/banner fields - Update applyBlobField() to detect and use existing JsonBlobRef directly - Add isJsonBlobRef() helper method to check for existing blob refs - Add tests for create/update with existing blob refs - Add test for mixed blob upload and existing ref This allows reusing previously uploaded blobs without re-uploading them. Closes hypercerts-sdk-yai
- Add isValidDid() utility function to core/types.ts - Export isValidDid from main index for public use - Add DID validation in BlobOperationsImpl constructor - Add tests for DID validation (valid and invalid formats) This prevents cryptic errors from SDS endpoints when invalid DIDs are passed. Closes hypercerts-sdk-4ni
|
📝 WalkthroughWalkthroughAdds RichText utilities (sync + async mention resolution), introduces and exports DID format validation via Changes
Sequence Diagram(s)(Skipped — changes do not present a multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 PR enhances the SDK's defensive programming and blob handling capabilities by adding DID format validation and support for reusing existing blob references in profile operations.
Changes:
- Added
isValidDid()utility function with regex-based DID format validation - Extended profile avatar/banner fields to accept existing
JsonBlobRefobjects alongside newBlobuploads - Enhanced RichText changeset documentation with 0.x versioning clarification and usage examples
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/sdk-core/src/core/types.ts |
Added isValidDid() function for DID format validation |
packages/sdk-core/src/repository/BlobOperationsImpl.ts |
Added DID validation in constructor using isValidDid() |
packages/sdk-core/src/repository/interfaces.ts |
Added BlobInput type and updated ProfileParams to accept blob refs |
packages/sdk-core/src/repository/ProfileOperationsImpl.ts |
Implemented isJsonBlobRef() type guard and updated applyBlobField() to handle existing refs |
packages/sdk-core/src/index.ts |
Exported isValidDid function |
packages/sdk-core/tests/core/types.test.ts |
Added comprehensive tests for isValidDid() function |
packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts |
Added DID validation tests for constructor |
packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts |
Added tests for existing blob ref handling in create/update |
.changeset/add-richtext-utility.md |
Enhanced with 0.x versioning note, corrected mentions documentation, and usage examples |
.beads/issues.jsonl |
Updated issue tracking including closure of related issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should reject method with numbers", () => { | ||
| expect(isValidDid("did:plc2:abc123")).toBe(false); | ||
| }); |
There was a problem hiding this comment.
The tests should include cases for DID methods that contain digits (e.g., did:key2:abc123, did:btc1:xyz789) to ensure the validation function correctly handles numeric characters in the method name. These are valid according to the W3C DID Core specification but would currently fail with the existing regex.
| * JsonBlobRef has the structure: { $type: "blob", ref: { $link }, mimeType, size } | ||
| * | ||
| * @internal | ||
| */ | ||
| private isJsonBlobRef(value: unknown): value is JsonBlobRef { | ||
| return ( | ||
| typeof value === "object" && | ||
| value !== null && | ||
| "$type" in value && | ||
| (value as Record<string, unknown>).$type === "blob" && | ||
| "ref" in value && | ||
| "mimeType" in value && | ||
| "size" in value | ||
| ); |
There was a problem hiding this comment.
The isJsonBlobRef type guard does not validate the nested structure of the ref field. According to the AT Protocol specification, JsonBlobRef.ref must be an object with a $link property containing the CID string.
The current implementation only checks for the presence of the ref property but doesn't verify that it has the correct structure { $link: string }. This could allow malformed blob references to pass validation, potentially causing runtime errors when the blob ref is used.
Consider adding validation for the nested structure:
"ref" in value &&
typeof (value as Record<string, unknown>).ref === "object" &&
(value as Record<string, unknown>).ref !== null &&
"$link" in (value as Record<string, unknown>).ref| * JsonBlobRef has the structure: { $type: "blob", ref: { $link }, mimeType, size } | |
| * | |
| * @internal | |
| */ | |
| private isJsonBlobRef(value: unknown): value is JsonBlobRef { | |
| return ( | |
| typeof value === "object" && | |
| value !== null && | |
| "$type" in value && | |
| (value as Record<string, unknown>).$type === "blob" && | |
| "ref" in value && | |
| "mimeType" in value && | |
| "size" in value | |
| ); | |
| * JsonBlobRef has the structure: { $type: "blob", ref: { $link: string }, mimeType, size } | |
| * | |
| * @internal | |
| */ | |
| private isJsonBlobRef(value: unknown): value is JsonBlobRef { | |
| if (typeof value !== "object" || value === null) { | |
| return false; | |
| } | |
| const record = value as Record<string, unknown>; | |
| if (record.$type !== "blob" || !("ref" in record) || !("mimeType" in record) || !("size" in record)) { | |
| return false; | |
| } | |
| const ref = record.ref; | |
| if (typeof ref !== "object" || ref === null) { | |
| return false; | |
| } | |
| const refRecord = ref as Record<string, unknown>; | |
| return typeof refRecord.$link === "string"; |
| // Core types and schemas | ||
| export type { DID, Organization, Collaborator, CollaboratorPermissions } from "./core/types.js"; | ||
| export { OrganizationSchema, CollaboratorSchema, CollaboratorPermissionsSchema } from "./core/types.js"; | ||
| export { OrganizationSchema, CollaboratorSchema, CollaboratorPermissionsSchema, isValidDid } from "./core/types.js"; |
There was a problem hiding this comment.
The BlobInput type is not exported from the main index.ts file, even though it's mentioned in the PR description as being exported. This type is useful for consumers who want type-safe usage when passing blob parameters to profile operations.
The type should be exported alongside other repository types in index.ts to ensure it's available to SDK consumers.
| it("should create profile with existing avatar blob ref (no upload)", async () => { | ||
| const existingBlobRef = { | ||
| $type: "blob" as const, | ||
| ref: { $link: "existing-avatar-cid" }, | ||
| mimeType: "image/png", | ||
| size: 2048, | ||
| }; | ||
|
|
||
| await profileOps.create({ | ||
| displayName: "User with Existing Avatar", | ||
| avatar: existingBlobRef, | ||
| }); | ||
|
|
||
| // Should NOT upload - existing blob ref is used directly | ||
| expect(mockBlobs.upload).not.toHaveBeenCalled(); | ||
| expect(mockAgent.com.atproto.repo.createRecord).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| record: expect.objectContaining({ | ||
| displayName: "User with Existing Avatar", | ||
| avatar: existingBlobRef, | ||
| }), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("should create profile with existing banner blob ref (no upload)", async () => { | ||
| const existingBlobRef = { | ||
| $type: "blob" as const, | ||
| ref: { $link: "existing-banner-cid" }, | ||
| mimeType: "image/jpeg", | ||
| size: 4096, | ||
| }; | ||
|
|
||
| await profileOps.create({ | ||
| displayName: "User with Existing Banner", | ||
| banner: existingBlobRef, | ||
| }); | ||
|
|
||
| // Should NOT upload - existing blob ref is used directly | ||
| expect(mockBlobs.upload).not.toHaveBeenCalled(); | ||
| expect(mockAgent.com.atproto.repo.createRecord).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| record: expect.objectContaining({ | ||
| displayName: "User with Existing Banner", | ||
| banner: existingBlobRef, | ||
| }), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The test coverage for existing blob refs should include edge cases for malformed blob references. For example:
- A blob ref missing the
$linkproperty in therefobject - A blob ref with
refas a non-object value (e.g., string or null) - A blob ref missing required fields like
mimeTypeorsize
These tests would ensure that the isJsonBlobRef type guard properly validates the blob structure before using it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/core/types.ts`:
- Around line 25-50: The isValidDid function's method regex only allows
lowercase letters and wrongly rejects methods containing digits; update the
regex in isValidDid to permit lowercase letters and digits (change the method
pattern from `[a-z]+` to `[a-z0-9]+`) so it matches the W3C DID Core spec (e.g.,
`did:example1:abc`), and adjust any inline comment describing the Method rule if
present.
- Around line 25-50: Replace the plain-regex isValidDid implementation with a
Zod-based runtime validator but keep the original DID method restriction
(lowercase letters only); update the exported isValidDid(did: string): boolean
to internally use a Zod schema (e.g., Zod.string().regex(...)) that uses the
existing regex /^did:[a-z]+:[a-zA-Z0-9._:%-]+$/ for validation so the method
portion remains [a-z]+ and reuse the schema elsewhere in the codebase for
consistent runtime validation.
🧹 Nitpick comments (4)
packages/sdk-core/tests/core/types.test.ts (1)
48-54: Remove the duplicatedid:plc:invalid case.The same input is asserted invalid twice, so this is redundant.
🧹 Suggested cleanup
- it("should reject did:method: with empty identifier", () => { - expect(isValidDid("did:plc:")).toBe(false); - });packages/sdk-core/src/repository/interfaces.ts (1)
660-708: Re-exportBlobInputfrom the main entrypoint.This keeps the new public type discoverable without deep imports and aligns with the barrel-export convention.
📦 Suggested export update (packages/sdk-core/src/index.ts)
export type { RecordOperations, BlobOperations, ProfileOperations, HypercertOperations, HypercertEvents, + BlobInput, CreateHypercertParams, CreateHypercertResult, CreateOrganizationParams, LocationParams,As per coding guidelines, Export types from dedicated entrypoints.
packages/sdk-core/src/repository/ProfileOperationsImpl.ts (1)
93-139: Tighten JsonBlobRef validation (prefer Zod schema).
isJsonBlobRefonly checks key presence, so malformed refs (e.g.,ref: nullor non-string$link) can slip through and get persisted. Consider validating field types via Zod for consistent runtime checks. As per coding guidelines, "Use Zod schemas for runtime validation."♻️ Suggested refactor using Zod
+import { z } from "zod"; + +const JsonBlobRefSchema = z.object({ + $type: z.literal("blob"), + ref: z.object({ $link: z.string() }), + mimeType: z.string(), + size: z.number(), +}); + private isJsonBlobRef(value: unknown): value is JsonBlobRef { - return ( - typeof value === "object" && - value !== null && - "$type" in value && - (value as Record<string, unknown>).$type === "blob" && - "ref" in value && - "mimeType" in value && - "size" in value - ); + return JsonBlobRefSchema.safeParse(value).success; }packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts (1)
181-229: Consider fixtures/mocks per test guidelines.
New blob-ref literals are inline and the suite usestests/utils/mocks.ts. To align with test guidelines, consider moving these refs intotests/utils/fixtures.tsand using mocks fromsrc/testing/. As per coding guidelines, "Use mocks from src/testing/ for unit tests" and "Use fixtures from tests/utils/fixtures.ts in test files."
|
@copilot open a new pull request to apply changes based on the comments in this thread |
- Update DID validation regex to allow digits in method names per W3C spec - Add test cases for DIDs with numeric method names (did:key2:, did:btc1:) - Strengthen isJsonBlobRef type guard to validate nested ref.$link structure - Export BlobInput type from main index.ts for consumer usage - Add edge case tests for malformed blob references Co-authored-by: aspiers <100738+aspiers@users.noreply.github.com>
- Update JSDoc comment to reflect digits allowed in DID method names - Remove package-lock.json (monorepo uses pnpm) - Add package-lock.json to .gitignore to prevent future commits Co-authored-by: aspiers <100738+aspiers@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts`:
- Around line 479-565: Update the tests to expect a ValidationError instead of
allowing malformed blob refs to be passed to mockBlobs.upload: change the
assertions in ProfileOperationsImpl.test.ts to call profileOps.update(...)
inside an await expect(...).rejects.toThrow(ValidationError) (or equivalent) for
each malformedRef case, and remove the
expect(mockBlobs.upload).toHaveBeenCalledWith(...) checks; then implement
runtime validation in the ProfileOperationsImpl update flow (the method
profileOps.update / the code path that inspects avatar input and calls
mockBlobs.upload) by adding a Zod schema for BlobInput/JsonBlobRef and throw a
ValidationError when the input does not match the schema so the tests fail fast
and consistently.
🧹 Nitpick comments (1)
packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts (1)
181-229: Consider shared fixtures for blob refs to reduce duplication.
These inline blob refs look like good candidates fortests/utils/fixtures.tsto keep tests consistent and DRY.As per coding guidelines: Use fixtures from tests/utils/fixtures.ts in test files.
Also applies to: 399-478
| it("should treat malformed blob ref (missing $link) as regular Blob and upload", async () => { | ||
| const malformedRef = { | ||
| $type: "blob" as const, | ||
| ref: {}, // Missing $link | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }; | ||
|
|
||
| mockBlobs.upload.mockResolvedValue({ | ||
| ref: { $link: "uploaded-cid" }, | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }); | ||
|
|
||
| await profileOps.update({ | ||
| avatar: malformedRef as unknown as BlobInput, | ||
| }); | ||
|
|
||
| // Should upload because it's not a valid JsonBlobRef | ||
| expect(mockBlobs.upload).toHaveBeenCalledWith(malformedRef); | ||
| }); | ||
|
|
||
| it("should treat malformed blob ref (ref is not an object) as regular Blob and upload", async () => { | ||
| const malformedRef = { | ||
| $type: "blob" as const, | ||
| ref: "string-instead-of-object", | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }; | ||
|
|
||
| mockBlobs.upload.mockResolvedValue({ | ||
| ref: { $link: "uploaded-cid" }, | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }); | ||
|
|
||
| await profileOps.update({ | ||
| avatar: malformedRef as unknown as BlobInput, | ||
| }); | ||
|
|
||
| // Should upload because it's not a valid JsonBlobRef | ||
| expect(mockBlobs.upload).toHaveBeenCalledWith(malformedRef); | ||
| }); | ||
|
|
||
| it("should treat malformed blob ref (ref is null) as regular Blob and upload", async () => { | ||
| const malformedRef = { | ||
| $type: "blob" as const, | ||
| ref: null, | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }; | ||
|
|
||
| mockBlobs.upload.mockResolvedValue({ | ||
| ref: { $link: "uploaded-cid" }, | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }); | ||
|
|
||
| await profileOps.update({ | ||
| avatar: malformedRef as unknown as BlobInput, | ||
| }); | ||
|
|
||
| // Should upload because it's not a valid JsonBlobRef | ||
| expect(mockBlobs.upload).toHaveBeenCalledWith(malformedRef); | ||
| }); | ||
|
|
||
| it("should treat malformed blob ref (missing mimeType) as regular Blob and upload", async () => { | ||
| const malformedRef = { | ||
| $type: "blob" as const, | ||
| ref: { $link: "some-cid" }, | ||
| // Missing mimeType | ||
| size: 1024, | ||
| }; | ||
|
|
||
| mockBlobs.upload.mockResolvedValue({ | ||
| ref: { $link: "uploaded-cid" }, | ||
| mimeType: "image/png", | ||
| size: 1024, | ||
| }); | ||
|
|
||
| await profileOps.update({ | ||
| avatar: malformedRef as unknown as BlobInput, | ||
| }); | ||
|
|
||
| // Should upload because it's not a valid JsonBlobRef | ||
| expect(mockBlobs.upload).toHaveBeenCalledWith(malformedRef); | ||
| }); |
There was a problem hiding this comment.
Malformed blob refs should be rejected, not uploaded.
These tests currently codify passing non-Blob objects into upload, which can surface confusing runtime failures. Prefer validating inputs (e.g., Blob instance or JsonBlobRef) and throwing a ValidationError, then update these tests to expect the error.
As per coding guidelines: Use Zod schemas for runtime validation.
🔧 Suggested test adjustment (apply similarly to the other malformed-ref cases)
- await profileOps.update({
- avatar: malformedRef as unknown as BlobInput,
- });
-
- // Should upload because it's not a valid JsonBlobRef
- expect(mockBlobs.upload).toHaveBeenCalledWith(malformedRef);
+ await expect(
+ profileOps.update({
+ avatar: malformedRef as unknown as BlobInput,
+ }),
+ ).rejects.toThrow("Invalid blob input");🤖 Prompt for AI Agents
In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts` around
lines 479 - 565, Update the tests to expect a ValidationError instead of
allowing malformed blob refs to be passed to mockBlobs.upload: change the
assertions in ProfileOperationsImpl.test.ts to call profileOps.update(...)
inside an await expect(...).rejects.toThrow(ValidationError) (or equivalent) for
each malformedRef case, and remove the
expect(mockBlobs.upload).toHaveBeenCalledWith(...) checks; then implement
runtime validation in the ProfileOperationsImpl update flow (the method
profileOps.update / the code path that inspects avatar input and calls
mockBlobs.upload) by adding a Zod schema for BlobInput/JsonBlobRef and throw a
ValidationError when the input does not match the schema so the tests fail fast
and consistently.
When uploading images for collection avatars/banners, validation failed with:
'ValidationError: Invalid collection record: Record/banner/image should be a blob ref'
The issue: blobs.upload() returns { ref, mimeType, size } but AT Protocol
lexicons define SmallImage.image and LargeImage.image as BlobRef, which
requires the full structure including $type: 'blob'.
Two helper methods were passing raw upload results directly:
- resolveCollectionImageInput() for avatar/banner images
- resolveUriOrBlob() for location/attachment blobs
The SDK already had blobToJsonRef() to add the required $type: 'blob',
but these methods weren't using it. Other code paths (uploadImageBlob,
updateClaim) correctly used blobToJsonRef() and worked fine.
Without $type: 'blob', the server rejects the record because the blob
reference doesn't match the expected BlobRef schema from @atproto/lexicon.
Summary
This PR adds defensive DID format validation in BlobOperationsImpl and extends ProfileOperationsImpl to accept existing blob references (JsonBlobRef) for avatar/banner fields, eliminating unnecessary re-uploads. It also enhances the RichText changeset documentation with usage examples and 0.x versioning clarification.
Changes
DID format validation: Added
isValidDid()utility function tocore/types.tswith comprehensive regex validation for DID format (did:method:identifier). BlobOperationsImpl constructor now validates the repoDid parameter and throws a helpfulValidationErrorfor invalid formats.Existing blob ref support for profiles: Extended
ProfileParamsto acceptBlobInputtype (Blob | JsonBlobRef) for avatar/banner fields. TheapplyBlobField()method now detects existing JsonBlobRef objects and uses them directly without re-uploading, following the pattern used in HypercertOperationsImpl.Enhanced RichText changeset: Updated
add-richtext-utility.mdchangeset with:Testing
isValidDid()covering valid DIDs (did:plc, did:web, various identifiers) and invalid inputs (empty strings, malformed DIDs, URLs)Notes
isValidDid()function is exported from the SDK for consumers who need DID validationBlobInputtype is exported frominterfaces.tsfor type-safe usageSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.