Fix SDS blob uploads; add RichText utilities#122
Conversation
🦋 Changeset detectedLatest commit: 4348925 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 |
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 19. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
📝 WalkthroughWalkthroughAdds RichText facet utilities; introduces a BlobOperations abstraction with SDS routing; refactors ProfileOperationsImpl and HypercertOperationsImpl to use BlobOperations (adds profile create/merge logic); updates Repository wiring, tests, mocks, changesets, docs, and package scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Repository
participant BlobOps as BlobOperations
participant SDS
participant PDS
Client->>Repository: request upload(blob)
Repository->>BlobOps: upload(data)
BlobOps->>BlobOps: determine isSDS & derive Content-Type
alt SDS mode
BlobOps->>SDS: POST com.sds.repo.uploadBlob?repo=... (Content-Type)
SDS-->>BlobOps: { ref: { $link }, mimeType, size }
BlobOps-->>Repository: { ref: { $link }, mimeType, size }
else PDS mode
BlobOps->>PDS: agent.com.atproto.repo.uploadBlob (multipart)
PDS-->>BlobOps: { blob: { ref, mimeType, size } }
BlobOps-->>Repository: { ref: { $link }, mimeType, size }
end
Repository-->>Client: normalized upload result (JsonBlobRef)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 pull request addresses SDS blob upload failures, adds comprehensive RichText utilities for working with AT Protocol's rich text format, and refactors blob operations using dependency injection for better maintainability.
Changes:
- Fixed SDS blob uploads by routing to
com.sds.repo.uploadBlobwith requiredrepoquery parameter - Added RichText utility functions (
createFacetsFromText,createFacetsFromTextSync) for auto-detecting URLs, hashtags, and @mentions - Refactored blob operations to use dependency injection, eliminating duplicate code across multiple operation implementations
- Enhanced JSDoc documentation with comprehensive examples for collection and project methods
- Added
ProfileOperationsImpl.create()method for creating profiles without read-modify-write pattern
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Updated task dependency configuration (removed install task, modified check dependencies) |
| packages/sdk-core/package.json | Added format:check to check command |
| packages/sdk-react/package.json | Added format:check to check command |
| packages/sdk-core/src/repository/BlobOperationsImpl.ts | Added SDS blob upload support via uploadViaSDS() method with direct fetchHandler calls |
| packages/sdk-core/src/repository/ProfileOperationsImpl.ts | Refactored to use BlobOperations dependency injection, added create() method, extracted helper methods |
| packages/sdk-core/src/repository/HypercertOperationsImpl.ts | Refactored to use BlobOperations dependency injection, added blobToJsonRef converter, enhanced JSDoc |
| packages/sdk-core/src/repository/interfaces.ts | Added ProfileParams interface for shared profile operation parameters |
| packages/sdk-core/src/repository/Repository.ts | Updated to pass BlobOperations and isSDS flag to operation implementations |
| packages/sdk-core/src/lib/rich-text.ts | New utility module for creating facets from plain text with async/sync variants |
| packages/sdk-core/src/index.ts | Added exports for RichText utilities |
| packages/sdk-core/tests/**/*.test.ts | Updated tests to mock BlobOperations instead of agent.uploadBlob, added comprehensive RichText tests |
| .changeset/*.md | Added three changesets documenting the changes (patch for bug fixes, minor for RichText feature, patch for docs) |
| AGENTS.md | Added "Landing the Plane" section for session completion workflow |
| .gitattributes | Added custom merge strategy for beads JSONL files |
| .beads/issues.jsonl | Issue tracking data (project management) |
| specs/*.md | Updated specification tracking documents |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dependsOn": ["^build"] | ||
| }, | ||
| "format:check": {}, | ||
| "check": {}, |
There was a problem hiding this comment.
The "check" task now has no dependencies, but it likely should still depend on the tasks it's checking (lint, typecheck, build, test). The empty dependencies array means "check" won't wait for or trigger any other tasks. Consider whether this is intentional or if it should maintain some dependency chain.
| "check": {}, | |
| "check": { | |
| "dependsOn": ["lint", "typecheck", "test", "format:check"] | |
| }, |
| private repoDid: string, | ||
| private _serverUrl: string, | ||
| private isSDS: boolean, | ||
| private repo?: unknown, |
There was a problem hiding this comment.
The repo parameter is declared but never used in the class. Consider removing this unused parameter unless it's intended for future use. If it's reserved for future functionality, add a JSDoc comment explaining its purpose.
| "build": { | ||
| "dependsOn": ["^build"], | ||
| "outputs": ["dist/**"] | ||
| }, |
There was a problem hiding this comment.
The "install" task has been removed from turbo.json, but typical Turborepo setups include this task to handle dependency installation in monorepos. Verify that this removal doesn't break the dependency installation workflow, especially in CI/CD environments or when new developers clone the repository.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (2)
1203-1231: Inconsistent blob upload mock in mixed content test.This test still uses
mockAgent.com.atproto.repo.uploadBlob(line 1205-1210) while other tests in this file have been updated to usemockBlobs.upload. This inconsistency could cause the test to fail or produce unexpected behavior sinceHypercertOperationsImplnow usesBlobOperationsfor uploads.🔧 Proposed fix
it("should add attachment with multiple content items (mixed URIs and Blobs)", async () => { const blob = new Blob(["evidence data"], { type: "application/pdf" }); - mockAgent.com.atproto.repo.uploadBlob.mockResolvedValue({ - success: true, - data: { - blob: { ref: { $link: "blob-cid" }, mimeType: "application/pdf", size: 100 }, - }, + mockBlobs.upload.mockResolvedValue({ + ref: { $link: "blob-cid" }, + mimeType: "application/pdf", + size: 100, }); const result = await hypercertOps.addAttachment({ @@ -1217,7 +1215,7 @@ describe("HypercertOperationsImpl", () => { }); expect(result.uri).toContain("attachment"); - expect(mockAgent.com.atproto.repo.uploadBlob).toHaveBeenCalled(); + expect(mockBlobs.upload).toHaveBeenCalled(); const call = mockAgent.com.atproto.repo.createRecord.mock.calls[0][0];
1976-2032: Another inconsistent blob upload mock in project creation test.The test "should create a project with avatar and banner" at lines 1976-2032 uses
mockAgent.com.atproto.repo.uploadBlobinstead ofmockBlobs.upload. This should be updated for consistency with the BlobOperations refactoring.🔧 Proposed fix
it("should create a project with avatar and banner", async () => { const logoBlob = new Blob(["logo"], { type: "image/png" }); const headerBlob = new Blob(["header"], { type: "image/jpeg" }); - mockAgent.com.atproto.repo.uploadBlob.mockResolvedValueOnce({ - success: true, - data: { - blob: { - $type: "blob", - ref: { $link: "bafyrei-logo" }, - mimeType: "image/png", - size: 150, - }, - }, + mockBlobs.upload.mockResolvedValueOnce({ + ref: { $link: "bafyrei-logo" }, + mimeType: "image/png", + size: 150, }); - mockAgent.com.atproto.repo.uploadBlob.mockResolvedValueOnce({ - success: true, - data: { - blob: { - $type: "blob", - ref: { $link: "bafyrei-header" }, - mimeType: "image/jpeg", - size: 250, - }, - }, + mockBlobs.upload.mockResolvedValueOnce({ + ref: { $link: "bafyrei-header" }, + mimeType: "image/jpeg", + size: 250, });
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 308-329: In update, check the result of the
com.atproto.repo.getRecord call (existing.success) before accessing
existing.data.value; if existing.success is false (record missing) treat
existingProfile as an empty object rather than indexing existing.data.value,
then call mergeParamsIntoProfile with that safe object and proceed to
com.atproto.repo.putRecord; ensure you reference the getRecord result (existing)
and avoid dereferencing existing.data.value when existing.success is falsy so
update and putRecord won't throw on new accounts.
- Around line 92-105: applyBlobField currently assigns only uploadResult.ref
(the { $link } object) to profile blob fields, but AT Protocol requires the full
blob object { $type: "blob", ref: { $link }, mimeType, size }; update
applyBlobField so after uploading (using this.blobs.upload) you convert
uploadResult into the full blob reference structure before assigning to
result[field] (include $type:"blob", ref:uploadResult.ref,
mimeType:uploadResult.mimeType, size:uploadResult.size). Reuse the existing
HypercertOperationsImpl.blobToJsonRef() implementation or extract that logic
into a shared helper and call it from applyBlobField; ensure null deletes the
field and undefined still returns early, and keep blobs.upload as the upload
call.
🧹 Nitpick comments (10)
.gitattributes (1)
1-3: Custom merge driver requires local configuration.The
merge=beadsattribute relies on a custom merge driver that must be configured in each developer's.git/configor global.gitconfig. Without this configuration, Git will use the default merge behavior for the JSONL file, which may cause conflicts on concurrent edits.Consider documenting the merge driver setup in the README or a contributing guide so that team members can configure it correctly.
packages/sdk-core/src/repository/BlobOperationsImpl.ts (2)
163-195: Consider extracting error details from failed SDS responses.The current implementation only uses
response.statusTextfor error messages. Parsing the response body on failure could provide more actionable error details.♻️ Proposed enhancement for better error messages
if (!response.ok) { - throw new NetworkError(`SDS blob upload failed: ${response.statusText}`); + let errorDetail = response.statusText; + try { + const errorBody = await response.json() as { message?: string; error?: string }; + errorDetail = errorBody.message || errorBody.error || errorDetail; + } catch { + // Ignore JSON parse errors, use statusText + } + throw new NetworkError(`SDS blob upload failed: ${errorDetail}`); }
69-71: Remove the unusedrepoparameter from the constructor.The
repoparameter is passed from the Repository class but never referenced within BlobOperationsImpl. Removing it will eliminate dead code.♻️ Remove the unused parameter
constructor( private agent: Agent, private repoDid: string, private _serverUrl: string, private isSDS: boolean, - private repo?: unknown, ) {}Also update the instantiation in Repository.ts line 369:
- this._blobs = new BlobOperationsImpl(this.agent, this.repoDid, this.serverUrl, this._isSDS, this); + this._blobs = new BlobOperationsImpl(this.agent, this.repoDid, this.serverUrl, this._isSDS);specs/lexicon-sync/v0.10.0-beta.4-v0.10.0-beta.11.md (1)
66-112: Avoid hard‑coding test counts in docs.Line 85 includes exact test counts, which tend to drift as suites evolve. Consider phrasing as “tests pass” or linking to CI if you want to keep this accurate.
packages/sdk-core/tests/utils/mocks.ts (1)
200-217: Centralize BlobOperations mocks undersrc/testing.Since this helper is meant for unit tests, consider moving or re-exporting it via
src/testing/so tests can standardize on that location. As per coding guidelines Use mocks fromsrc/testing/for unit tests..changeset/add-richtext-utility.md (1)
1-12: Add a brief compatibility note for the 0.x minor bump.Consider stating that this is additive and no breaking changes are expected, since 0.x minor releases can still be perceived as breaking without explicit context. Based on learnings For pre-1.0 libraries (0.x), minor version bumps can introduce breaking changes. In changeset files, note that breaking changes may occur with minor bumps and clearly document any API changes, removal or behavioral changes, and compatibility caveats.
packages/sdk-core/src/index.ts (2)
253-254: Consider renamingrich-textto camelCase for utilities.The new module is a utility; per convention, a name like
richText.tswould align better with the file-naming guideline. As per coding guidelines Use camelCase for file names containing utilities and helper functions.
254-255: Export RichText types via the dedicated types entrypoint.If
RichTextResultandAppBskyRichtextFacetare meant for public use, consider adding them to the types entrypoint and re-exporting from there to keep type exports centralized. As per coding guidelines Export types from dedicated entrypoints (e.g.,@hypercerts-org/sdk-core/types)..changeset/fix-sds-endpoint-routing.md (1)
1-15: Patch bump may undersell the new public API.This changeset includes a new public method (
ProfileOperationsImpl.create) and a new BlobOperations abstraction; consider a minor bump or explicitly calling out the API additions and any compatibility expectations. Based on learnings For pre-1.0 libraries (0.x), minor version bumps can introduce breaking changes. In changeset files, note that breaking changes may occur with minor bumps and clearly document any API changes, removal or behavioral changes, and compatibility caveats.packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
954-968: Unused parameter_fallbackMimeTypeshould be removed.The
_fallbackMimeTypeparameter is no longer used after the refactoring. Previously it may have been needed for direct blob upload calls, butBlobOperations.upload()handles MIME type detection internally viaBlob.type.♻️ Suggested fix
- private async resolveUriOrBlob(content: string | Blob, _fallbackMimeType: string) { + private async resolveUriOrBlob(content: string | Blob) { if (typeof content === "string") { const uriRef = { $type: "org.hypercerts.defs#uri", uri: content, } satisfies $Typed<OrgHypercertsDefs.Uri>; return uriRef; } const uploadResult = await this.blobs.upload(content); return { $type: "org.hypercerts.defs#smallBlob" as const, blob: uploadResult, }; }Also update call sites (e.g., lines 990 and 1109) to remove the fallback argument.
aspiers
force-pushed
the
richtext-uploadBlob
branch
from
8802b3c to
bbc6f44
Compare
aspiers
force-pushed
the
richtext-uploadBlob
branch
from
bbc6f44 to
363bf0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
954-986: Ensure uploaded blobs are converted toJsonBlobRef.
BlobOperations.uploadreturns{ ref, mimeType, size }without$type. InresolveUriOrBlobandresolveCollectionImageInput, that object is embedded directly, so records may fail lexicon validation and serialize missing$type: "blob". Convert withblobToJsonRefbefore embedding.Suggested fix
- const uploadResult = await this.blobs.upload(content); - return { - $type: "org.hypercerts.defs#smallBlob" as const, - blob: uploadResult, - }; + const uploadResult = await this.blobs.upload(content); + return { + $type: "org.hypercerts.defs#smallBlob" as const, + blob: this.blobToJsonRef(uploadResult), + }; - const uploadResult = await this.blobs.upload(input); - if (isBanner) { - return { $type: "org.hypercerts.defs#largeImage" as const, image: uploadResult }; - } - return { $type: "org.hypercerts.defs#smallImage" as const, image: uploadResult }; + const uploadResult = await this.blobs.upload(input); + const imageRef = this.blobToJsonRef(uploadResult); + if (isBanner) { + return { $type: "org.hypercerts.defs#largeImage" as const, image: imageRef }; + } + return { $type: "org.hypercerts.defs#smallImage" as const, image: imageRef };
🤖 Fix all issues with AI agents
In @.changeset/fix-sds-endpoint-routing.md:
- Around line 1-15: Update the changeset to mark the constructor signature
change as breaking: change the package version bump for
"@hypercerts-org/sdk-core" from patch to minor (per 0.x semantics) and add a
"BREAKING CHANGES" or "breaking" note that the BlobOperations
constructor/signature changed and HypercertOperationsImpl and
ProfileOperationsImpl now require a BlobOperations instance; ensure the
changeset text clearly describes the new required constructor parameter and
recommends consumers update their instantiation code accordingly.
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 188-239: Update the ProfileParams type to include optional $type
and createdAt fields, then modify mergeParamsIntoProfile to apply defaults using
nullish coalescing so callers can override them while ensuring the record always
includes both; specifically, ensure create() continues to call
mergeParamsIntoProfile({} , params) and that mergeParamsIntoProfile sets
profile.$type = params.$type ?? "app.bsky.actor.profile" (or the canonical
default) and profile.createdAt = params.createdAt ?? new Date().toISOString()
(or repository-generated timestamp) so the resulting record always contains
$type and createdAt.
🧹 Nitpick comments (3)
packages/sdk-core/tests/repository/HypercertValidation.test.ts (1)
6-38: Use mocks fromsrc/testingfor unit tests.
createMockBlobOperationsis pulled fromtests/utils/mocks, but the test guidelines require mocks to come fromsrc/testing/. Please move or re-import the mock fromsrc/testingto keep unit-test utilities centralized.As per coding guidelines: Use mocks from
src/testing/for unit tests.packages/sdk-core/src/repository/interfaces.ts (1)
659-727: Add a Zod schema forProfileParamsto enable runtime validation.You’ve introduced a new public params shape; please add a
ProfileParamsSchema(and derive the type from it) or otherwise surface Zod validation for this type to align with the runtime validation guideline.As per coding guidelines: Use Zod schemas for runtime validation in TypeScript files.
packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
5-6: Consider re-exporting mock helpers fromsrc/testing/.The new
createMockBlobOperationsimport is fromtests/utils/mocks. If it isn’t already re-exported fromsrc/testing/, consider moving/re-exporting it there to align the test-helper location convention.As per coding guidelines: Use mocks from
src/testing/for unit tests.
- Add RichText utility (src/lib/rich-text.ts) with createFacetsFromText() and createFacetsFromTextSync() for auto-detecting mentions, URLs, hashtags - Add comprehensive tests for RichText utility (11 tests) - Export RichText utilities from main index - Enhance JSDoc for createCollection, updateCollection, createProject, updateProject with avatar/banner examples - Update spec file to mark Change 2 and Change 3 as complete
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "format:check": {}, | ||
| "check": {}, |
There was a problem hiding this comment.
The turbo.json changes remove the install task entirely and simplify task dependencies. However, this creates inconsistency:
-
The
format:checkandchecktasks no longer havedependsOnarrays (empty objects), while other tasks likelint,test, andtypecheckdepend on^build. -
The
checktask has no dependencies defined here, but in package.json files it's defined as a composite command that runsformat:check,lint,typecheck,build, andtestin sequence.
This could lead to confusion where the task orchestration is split between turbo.json (which handles cross-package dependencies) and package.json (which handles within-package command sequences). Consider documenting this pattern or ensuring consistency.
| "format:check": {}, | |
| "check": {}, | |
| "format:check": { | |
| "dependsOn": ["^build"] | |
| }, | |
| "check": { | |
| "dependsOn": ["^build"] | |
| }, |
| data: Uint8Array, | ||
| encoding: string, | ||
| ): Promise<{ ref: { $link: string }; mimeType: string; size: number }> { | ||
| const url = `/xrpc/com.sds.repo.uploadBlob?repo=${encodeURIComponent(this.repoDid)}`; |
There was a problem hiding this comment.
The SDS blob upload endpoint constructs a URL with the repoDid parameter using encodeURIComponent(), which is good for preventing URL injection. However, there's no validation that this.repoDid is actually a valid DID format before making the request.
While this is likely validated elsewhere during Repository construction, consider adding a comment or assertion to clarify that repoDid is expected to be pre-validated, or add a basic format check (e.g., starts with "did:") to fail fast with a clear error message if the DID is invalid.
Without this patch, blob uploads always use the PDS endpoint (com.atproto.repo.uploadBlob) regardless of server type, and profile operations incorrectly route through non-existent com.sds.repo.* endpoints. This causes blob uploads to fail on SDS servers because the repository DID is never passed. This patch solves the problem by: 1. Using com.sds.repo.uploadBlob with the repo query parameter for SDS servers, and com.atproto.repo.uploadBlob for PDS servers 2. Removing incorrect SDS routing from profile operations — SDS servers override standard ATProto endpoints rather than exposing separate ones 3. Extracting a shared BlobOperations interface with dependency injection, eliminating duplicate blob upload code across HypercertOperationsImpl, ProfileOperationsImpl, and BlobOperationsImpl 4. Adding a ProfileOperationsImpl.create() method for creating new profiles without the read-modify-write pattern 5. Refactoring applyParamsToProfile with helper methods to reduce duplication Co-authored-by: Claude <noreply@anthropic.com>
- Document CreateLocationParams with examples for string, URL, Blob, and structured formats - Add documentation for LocationParams union type - Add JSDoc to resolveLocationValue explaining format handling - Add examples to createLocationRecord method Closes hypercerts-sdk-38n
- Add test for attachLocation with simple text string (beta.13+ format) - Add test for attachLocation with geo: URI string - Add test for createCollection with simple text location string - Verify strings get wrapped in URI ref format Closes hypercerts-sdk-d2e
aspiers
force-pushed
the
richtext-uploadBlob
branch
from
363bf0e to
4348925
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
950-982: Wrap uploaded blobs withblobToJsonRefbefore embedding.
resolveUriOrBlob(line 959) andresolveCollectionImageInput(lines 976–981) embed raw upload results directly without wrapping them. Lexicon blob fields require JsonBlobRef format with{ $type: "blob", ref, mimeType, size }. Other methods in this file (lines 174, 705) correctly usethis.blobToJsonRef()for this purpose.🛠️ Suggested fix
- const uploadResult = await this.blobs.upload(content); - return { - $type: "org.hypercerts.defs#smallBlob" as const, - blob: uploadResult, - }; + const uploadResult = await this.blobs.upload(content); + return { + $type: "org.hypercerts.defs#smallBlob" as const, + blob: this.blobToJsonRef(uploadResult), + };- const uploadResult = await this.blobs.upload(input); + const uploadResult = await this.blobs.upload(input); if (isBanner) { - return { $type: "org.hypercerts.defs#largeImage" as const, image: uploadResult }; + return { $type: "org.hypercerts.defs#largeImage" as const, image: this.blobToJsonRef(uploadResult) }; } - return { $type: "org.hypercerts.defs#smallImage" as const, image: uploadResult }; + return { $type: "org.hypercerts.defs#smallImage" as const, image: this.blobToJsonRef(uploadResult) };
🤖 Fix all issues with AI agents
In @.changeset/add-richtext-utility.md:
- Around line 1-12: Update the changeset to explicitly state the 0.x minor-bump
posture and any compatibility caveats: inside the .changeset entry for
"@hypercerts-org/sdk-core" add a short paragraph whether this minor bump is
intended to be non‑breaking for 0.x or may introduce breaking changes, and list
affected APIs and behavioral notes (mention createFacetsFromText,
createFacetsFromTextSync and the RichText re-export), including any removals,
signature changes, or runtime behavior differences (e.g., async mention
resolution requiring an agent) and recommended upgrade guidance for consumers.
In `@packages/sdk-core/src/lib/rich-text.ts`:
- Around line 40-43: Update the JSDoc for the async function
createFacetsFromText to match the sync variant and tests: replace the incorrect
statement that mentions without an agent will have "empty DIDs" with text
stating that unresolved mentions will use the handle string as the DID (e.g.,
"alice.bsky.social"); ensure the description for the agent parameter and the
return value reflect this behavior consistently with createFacetsFromTextSync
and existing tests.
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1880-1885: The doc examples for repo.hypercerts.createCollection
use the old location shape ({ value: ... }) and must be updated to the new
CreateLocationParams shape; locate the example blocks around the
createCollection usage in HypercertOperationsImpl (the doc comments that show
const collection = await repo.hypercerts.createCollection({...})), and replace
the location object with the required fields from CreateLocationParams (e.g.,
include srs and the appropriate lpVersion or locationType plus the descriptive
name/text fields) so the examples match the runtime shape expected by the
CreateLocationParams type.
🧹 Nitpick comments (9)
AGENTS.md (2)
95-118: Consider reversing the preference order to align with established best practices.The current guidance prefers
cd packages/<package>and treats--filteras an alternative. However, based on learnings, the repository's established best practice is to "Use pnpm workspaces withpnpm --filter <package>for package-specific commands." The--filterapproach is more robust (works from any directory, clearer in CI/scripts) whilecd'ing has trade-offs (requires knowing current directory, uses relative paths).Consider reframing this section to recommend
--filteras the primary approach, with the note that developers may prefercd'ing into packages for interactive work when they want cleaner output or easier argument passing.Based on learnings: "Use pnpm workspaces with
pnpm --filter <package>for package-specific commands"
486-486: Provide guidance for "File issues for remaining work" step.Step 1 of the mandatory workflow instructs to "File issues for remaining work" but provides no guidance on:
- How to create issues (GitHub CLI? Web interface? API?)
- What information to include in issues
- How to link issues to the current work
- Templates or examples to follow
Consider adding a brief example or reference to issue creation tools/process.
packages/sdk-core/src/repository/types.ts (1)
121-143: Add runtime validation forBlobUploadResultbefore conversion.
This is a public helper; a malformed upload result will silently generate invalid blob refs. Consider a small Zod schema and throw aValidationErroron failure before constructingJsonBlobRef.As per coding guidelines, Use Zod schemas for runtime validation in TypeScript files; packages/sdk-core/src/**/*: Use error hierarchy from sdk-core/src/core/errors.ts: HypercertsError, ValidationError, AuthenticationError, NetworkError, NotFoundError, SDSRequiredError.
packages/sdk-core/src/lib/rich-text.ts (2)
1-8: Rename utility file to camelCase for consistency.
Consider renaming torichText.tsand updating exports/imports/tests accordingly.As per coding guidelines, Use camelCase for file names containing utilities and helper functions.
88-104: Validatetextinput at runtime for this public API.
Consider a minimal Zod schema (andValidationErroron failure) to guard non‑string inputs before constructingRichText.As per coding guidelines, Use Zod schemas for runtime validation in TypeScript files; packages/sdk-core/src/**/*: Use error hierarchy from sdk-core/src/core/errors.ts: HypercertsError, ValidationError, AuthenticationError, NetworkError, NotFoundError, SDSRequiredError.
packages/sdk-core/src/services/hypercerts/types.ts (1)
284-376: AddUpdateLocationParamsto complete the location type set.You added
CreateLocationParamsandLocationParams, but the SDK pattern for lexicon entities expects anUpdate*Paramsalias as well. This also keeps updates consistent withPartial<Create*>.♻️ Suggested addition
export type CreateLocationParams = OverrideProperties< SetOptional<HypercertLocation, "$type" | "createdAt">, { location: string | Blob | HypercertLocation["location"]; } >; +export type UpdateLocationParams = Partial<CreateLocationParams>; + export type LocationParams = StrongRef | string | CreateLocationParams;As per coding guidelines: For each lexicon entity, define five types: Main (from lexicon), SDK type, CreateParams, UpdateParams, and union Params type.
packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts (1)
5-13: Align mocks/fixtures with repo test guidelines.These tests import mocks/constants from
tests/utils/mocks.ts. The repo guidelines call for using mocks fromsrc/testing/and fixtures fromtests/utils/fixtures.ts. Consider switching to those sources for consistency.As per coding guidelines: “Use mocks from
src/testing/for unit tests” and “Use fixtures fromtests/utils/fixtures.tsin test files”.packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts (1)
5-16: Use standard test mocks/fixtures.Repo guidelines prefer mocks from
src/testing/and fixtures fromtests/utils/fixtures.ts. Consider swappingcreateMockAgent/createMockBlobOperationsandTEST_REPO_DIDto those sources if available.As per coding guidelines: “Use mocks from
src/testing/for unit tests” and “Use fixtures fromtests/utils/fixtures.tsin test files”.packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
5-6: Use standard test mocks/fixtures.Repo guidelines prefer mocks from
src/testing/and fixtures fromtests/utils/fixtures.ts. Consider switching away fromtests/utils/mocks.tsand reusing fixtures for constants like repo DID.As per coding guidelines: “Use mocks from
src/testing/for unit tests” and “Use fixtures fromtests/utils/fixtures.tsin test files”.Also applies to: 28-38
| * @param text - The plain text to parse for facets | ||
| * @param agent - Optional AT Protocol agent for resolving @mentions to DIDs. | ||
| * Without an agent, mentions will have empty DIDs (invalid for posting). | ||
| * @returns Promise resolving to the text and detected facets |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -150 packages/sdk-core/src/lib/rich-text.ts | cat -nRepository: hypercerts-org/hypercerts-sdk
Length of output: 5580
🏁 Script executed:
find . -type f -name "*rich-text*" | grep -E "\.(test|spec)\.(ts|tsx|js)"Repository: hypercerts-org/hypercerts-sdk
Length of output: 120
🏁 Script executed:
cat -n ./packages/sdk-core/tests/lib/rich-text.test.tsRepository: hypercerts-org/hypercerts-sdk
Length of output: 6311
Fix incorrect async function documentation on unresolved mentions.
The createFacetsFromText documentation (line 42) incorrectly states that mentions without an agent will have "empty DIDs." However, the actual behavior—confirmed by tests—is that mentions receive the handle string as the DID (e.g., alice.bsky.social), not an empty value. Align the async function's documentation with the sync function's more accurate description (lines 110-112) and the test expectations (test line 45).
🤖 Prompt for AI Agents
In `@packages/sdk-core/src/lib/rich-text.ts` around lines 40 - 43, Update the
JSDoc for the async function createFacetsFromText to match the sync variant and
tests: replace the incorrect statement that mentions without an agent will have
"empty DIDs" with text stating that unresolved mentions will use the handle
string as the DID (e.g., "alice.bsky.social"); ensure the description for the
agent parameter and the return value reflect this behavior consistently with
createFacetsFromTextSync and existing tests.
| * ```typescript | ||
| * const collection = await repo.hypercerts.createCollection({ | ||
| * title: "Amazon Basin Projects", | ||
| * items: [...], | ||
| * location: { value: "Amazon Rainforest, Brazil", name: "Amazon Basin" }, | ||
| * }); |
There was a problem hiding this comment.
Location examples still use the old value shape.
The examples show location: { value: ... }, but the new CreateLocationParams expects location plus required fields like srs (and usually lpVersion / locationType). These examples will error at runtime. Please update them to the new shape.
✍️ Example update (apply similarly in all three locations)
- location: { value: "Amazon Rainforest, Brazil", name: "Amazon Basin" },
+ location: {
+ lpVersion: "1.0.0",
+ srs: "EPSG:4326",
+ locationType: "coordinate-decimal",
+ location: "Amazon Rainforest, Brazil",
+ name: "Amazon Basin",
+ },Also applies to: 2088-2094, 2355-2359
🤖 Prompt for AI Agents
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts` around lines
1880 - 1885, The doc examples for repo.hypercerts.createCollection use the old
location shape ({ value: ... }) and must be updated to the new
CreateLocationParams shape; locate the example blocks around the
createCollection usage in HypercertOperationsImpl (the doc comments that show
const collection = await repo.hypercerts.createCollection({...})), and replace
the location object with the required fields from CreateLocationParams (e.g.,
include srs and the appropriate lpVersion or locationType plus the descriptive
name/text fields) so the examples match the runtime shape expected by the
CreateLocationParams type.
This PR addresses SDS blob upload failures and adds RichText utilities for working with AT Protocol's rich text format.
Bug Fixes
com.sds.repo.uploadBlobwith the requiredrepoquery parameter. Previously, all blob uploads used the PDS endpoint regardless of server type, causing failures on SDS servers.New Features
createFacetsFromText,createFacetsFromTextSync): Auto-detect URLs, hashtags, and @mentions from plain text for use in hypercert descriptions. Includes async version with mention-to-DID resolution and sync version for fast detection.Refactoring
BlobOperationsinterface with dependency injection, eliminating duplicate blob upload code acrossHypercertOperationsImpl,ProfileOperationsImpl, andBlobOperationsImpl.ProfileOperationsImpl.create(): New method for creating profiles without the read-modify-write pattern.Technical Details
The SDS fix uses the agent's
fetchHandlerdirectly (same pattern asCollaboratorOperationsImplandOrganizationOperationsImpl) sincecom.sds.repo.uploadBlobis not a registered XRPC namespace on the standard AT Protocol Agent.Files Changed
BlobOperationsImpl.ts,interfaces.tsProfileOperationsImpl.tsHypercertOperationsImpl.tssrc/lib/rich-text.tsBlobOperationsImpl.test.ts,ProfileOperationsImpl.test.ts,HypercertOperationsImpl.test.tsTesting
BlobOperations.uploadinstead ofagent.uploadBlobSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.