Conversation
🦋 Changeset detectedLatest commit: aaddcb6 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 |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughProfiles split into Bluesky vs Certified flows; ProfileOperations gained get/create/update/upsert for each and accepts a server/pdsUrl. Blob uploads now return BlobRef; Hypercert images use smallImage/largeImage wrappers. Added blob URL utilities and CID extraction; tests, docs, and mocks updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as rgba(66,133,244,0.5)
participant Agent as rgba(52,168,83,0.5)
participant Repo as rgba(255,193,7,0.5)
participant PDS as rgba(234,67,53,0.5)
Client->>Agent: request profile
Agent->>Repo: call getBskyProfile() or getCertifiedProfile()
Repo-->>Agent: profile record (may include smallImage/largeImage with BlobRef)
Agent->>PDS: call getBlobUrl(pdsUrl, did, cid) / resolve blob
PDS-->>Agent: resolved blob URL
Agent-->>Client: profile with avatar/banner as resolved URLs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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
🧪 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 |
![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/package.json`:
- Line 91: Replace the local filesystem dependency for "@hypercerts-org/lexicon"
in package.json with a published npm version; specifically change the value that
currently points to "file:/home/kzoeps/Projects/gainforest/hypercerts-lexicon"
to a semver string (e.g., "^0.10.0-beta.13") so CI and other developers can
install from the registry—if the required changes aren’t yet published, publish
the package first and then update the "@hypercerts-org/lexicon" entry to the
released version.
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 245-247: The catch in ProfileOperationsImpl (around the getRecord
call) is too broad and swallows all errors; change it to only handle the "record
not found" case (e.g., check error.status === 404 or error.name/type/message
that your PDS client uses for not-found) and in that case fall through to return
just the handle, but for any other error rethrow (or log and rethrow) so
network/auth/server errors aren't hidden; update the error check around
getRecord in ProfileOperationsImpl to discriminate and only suppress the
specific not-found error.
🧹 Nitpick comments (1)
packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts (1)
8-8: Consider importingPROFILE_COLLECTIONfrom the implementation.The constant
PROFILE_COLLECTIONis duplicated here. If the value changes in the implementation but not in tests, tests could pass with incorrect assertions. Consider exporting the constant fromProfileOperationsImpl.tsor a shared constants file.
Kzoeps
force-pushed
the
profile
branch
5 times, most recently
from
a244549 to
ae706d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/ProfileOperationsImpl.ts (1)
294-303: Replace ProfileParams with CreateHypercertProfileParams and UpdateHypercertProfileParams.The
ProfileOperationsinterface usesProfileParamsfor bothcreate()andupdate()methods, but this violates SDK-core conventions. The correct types already exist:
create(params: ProfileParams)should becreate(params: CreateHypercertProfileParams)update(params: ProfileParams)should beupdate(params: UpdateHypercertProfileParams)
UpdateHypercertProfileParamscorrectly implements the convention pattern (Omit<Partial<CreateHypercertProfileParams>, ...>with nullable image fields), so update the interface signatures and remove the unusedProfileParamsdefinition.
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/services/hypercerts/types.ts`:
- Around line 163-223: Add a missing HypercertProfileParams union type for the
profile variants (e.g., type HypercertProfileParams =
CreateHypercertProfileParams | UpdateHypercertProfileParams) and refactor
UpdateHypercertProfileParams to use OverrideProperties for consistency: replace
the current Omit<Partial<CreateHypercertProfileParams>, "avatar" | "banner"> & {
avatar?: HypercertImage | null; banner?: HypercertImage | null; } with
OverrideProperties<Partial<CreateHypercertProfileParams>, { avatar?:
HypercertImage | null; banner?: HypercertImage | null; }>, keeping the existing
CreateHypercertProfileParams and HypercertImage symbols intact.
🧹 Nitpick comments (1)
.changeset/profile-certified-lexicon.md (1)
1-25: Add a pre‑1.0 semver caveat for the minor bump.The changeset already lists breaking changes, but a short note that these packages are still 0.x will better set expectations for consumers.
✏️ Suggested wording
**Breaking Changes:** +_Note: These packages are still pre‑1.0; minor bumps may include breaking changes._ - `profile.avatar` and `profile.banner` are now `HypercertImageRecord | undefined` instead of `string | undefined` - Profile data is stored in a different collection (`app.certified.profile` instead of `app.bsky.actor.profile`)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.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1798-1807: Critical: evaluator shape mismatches type/schema (build failure).
HypercertEvaluation.evaluatorsis currently typed asstring[](TS2322 in CI), but the record now writes{ did }[]. This will also fail lexicon validation if the schema still expects strings. Please align the SDK type and lexicon schema, or revert to the string array until the lexicon is updated.If the lexicon still expects strings, a quick fix is:
🔧 Minimal fix if schema expects string[]
- evaluators: params.evaluators.map((evaluator) => ({ did: evaluator })), + evaluators: params.evaluators,#!/bin/bash # Verify the lexicon schema and generated types for evaluators shape. rg -n "evaluators" node_modules/@hypercerts-org/lexicon -g '*.json' -g '*.d.ts' -g '*.ts'
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1803-1809: Add a new CreateEvaluationParams type (and the required
Main/alias/Update/union types for evaluation) to
packages/sdk-core/src/services/hypercerts/types.ts, then update the
HypercertOperationsImpl.addEvaluation method signature to async
addEvaluation(params: CreateEvaluationParams): Promise<CreateResult>; inside
addEvaluation build the HypercertEvaluation using params and preserve
caller-provided $type and createdAt via nullish coalescing (e.g. $type:
params.$type ?? HYPERCERT_COLLECTIONS.EVALUATION and createdAt: params.createdAt
?? new Date().toISOString()), and keep mapping evaluators from params.evaluators
to { did } objects before returning the CreateResult.
🧹 Nitpick comments (1)
packages/sdk-core/src/repository/ProfileOperationsImpl.ts (1)
231-240: Consider runtime validation for record fields.The type assertions (
as string | undefined,as HypercertImageRecord | undefined) are unsafe casts fromRecord<string, unknown>. If the PDS returns unexpected data shapes, these could lead to runtime issues downstream.Per coding guidelines, consider using Zod schemas for runtime validation of the profile record structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/lib/blob-url.ts`:
- Around line 107-116: The code in blob-url.ts extracts a CID by calling
ref.toString(), but ref is an object like { $link: "bafy..." } so to fix: inside
the conditional handling smallImage/largeImage (check imageType, image.image ->
imageData, and ref) replace the use of ref.toString() with ref.$link (or
String(ref.$link) to be safe) so the real CID is returned; ensure you only
access $link when ref is an object and present to avoid undefined.
🧹 Nitpick comments (1)
packages/sdk-core/src/services/hypercerts/types.ts (1)
738-739: Clarify the relationship betweenCreateProfileParamsandCreateHypercertProfileParams.
CreateProfileParamsappears to be a simpler variant (onlySetOptionalwithoutOverridePropertiesfor avatar/banner). Consider whether this is intentionally different fromCreateHypercertProfileParamsor if it should be consolidated. If both are needed, add a doc comment explaining the distinction.
|
working on this will finish it up tmrw |
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 (5)
packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts (1)
117-147:⚠️ Potential issue | 🟡 MinorType signature doesn't reflect SDS response flexibility.
The
upload()method returnsPromise<BlobRef>but the SDS path (line 176) returnsresult.blobdirectly without normalization, which can have differentrefformats (CID instance,{ $link: string }, or plain string). The test at line 131 explicitly validates string ref handling. Consider either:
- Normalizing SDS responses to match the
BlobReftype contract- Using a union return type that reflects the actual flexibility:
Promise<BlobRef | { ref: string | { $link: string }; mimeType: string; size: number }>Note:
HypercertOperationsImpl:321already handles this with runtime type checking.packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
2006-2013:⚠️ Potential issue | 🟡 MinorFix malformed JSDoc
@exampletag.There’s an extra
*that breaks the doc block rendering.📝 Suggested fix
- * * `@example` + * `@example`packages/sdk-core/src/repository/BlobOperationsImpl.ts (2)
160-176:⚠️ Potential issue | 🟡 MinorGuard against missing
blobin SDS upload responses.The method currently returns
result.blobwithout checking if it exists, which could returnundefineddespite thePromise<BlobRef>return type contract. This happens if the SDS endpoint returns anokresponse with an unexpected payload structure.🛡️ Suggested guard
- const result = (await response.json()) - - return result.blob + const result = await response.json(); + if (!result?.blob) { + throw new NetworkError("SDS blob upload failed: missing blob in response"); + } + return result.blob;
113-133:⚠️ Potential issue | 🟡 MinorUpdate upload() documentation to match the BlobRef return type.
The examples incorrectly show destructuring
{ ref, mimeType, size }, butupload()returns aBlobReffrom@atproto/api. The documentation must be updated to reflect the actual return type. Additionally, the suggested use ofblobRef.ref.toString()in the diff may not match the BlobRef API; verify the correct property or method to access the CID (e.g.,blobRef.ipld()as used elsewhere in the codebase).packages/sdk-core/src/repository/interfaces.ts (1)
618-643:⚠️ Potential issue | 🟡 MinorStale doc block references removed methods.
The examples at lines 622–642 still reference
repo.profile.get()andrepo.profile.update(), which no longer exist. These will confuse consumers. Update or remove this block since each method now has its own doc examples.Proposed fix — remove the stale block
-/** - * Profile operations for user profile management. - * - * `@example` - * ```typescript - * // Get profile - * const profile = await repo.profile.get(); - * console.log(profile.displayName); - * - * // Update profile - * await repo.profile.update({ - * displayName: "New Name", - * description: "Updated bio", - * }); - * - * // Update avatar - * await repo.profile.update({ - * avatar: new Blob([avatarData], { type: "image/png" }), - * }); - * - * // Clear a field by passing null - * await repo.profile.update({ - * website: null, // Removes website - * }); - * ``` - */ +/** + * Profile operations for Bluesky and Certified profile management. + * + * Supports dual-profile workflows: Bluesky profiles (app.bsky.actor.profile) + * and Certified profiles (app.certified.actor.profile). + */
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/repository/interfaces.ts`:
- Around line 672-675: UpdateBskyProfileParams is incorrectly derived from
AppBskyActorDefs.ProfileViewDetailed (a read-only view) — change it to be
Partial<CreateBskyProfileParams> so it represents selective updates to the
record; replace the current type alias for UpdateBskyProfileParams with a type
based on Partial<CreateBskyProfileParams> and still allow avatar?: Blob | null
and banner?: Blob | null (i.e., derive from CreateBskyProfileParams which itself
is based on AppBskyActorProfile.Record).
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 94-117: The imageToUrl currently treats only http(s) URIs as
passthrough and converts everything else to a blob URL; update imageToUrl (which
uses extractCidFromImage and getBlobUrl and fields pdsUrl/repoDid) to return any
valid URI scheme unchanged (e.g., detect presence of a scheme like "scheme://",
or more robustly match /^[a-z][a-z0-9+.-]*:\/\//i) instead of only http/https,
and only call getBlobUrl when the result is truly a bare CID (no scheme
present); keep the existing error throw when extractCidFromImage returns falsy.
🧹 Nitpick comments (3)
packages/sdk-core/src/lib/blob-url.ts (1)
49-62: Consider URL-encoding query parameters.While DIDs and CIDs are typically URL-safe in practice,
didandcidare inserted into the query string withoutencodeURIComponent. This could break with unusual inputs.♻️ Suggested fix
const normalizedPdsUrl = pdsUrl.replace(/\/$/, ""); - return `${normalizedPdsUrl}/xrpc/com.atproto.sync.getBlob?did=${did}&cid=${cid}`; + return `${normalizedPdsUrl}/xrpc/com.atproto.sync.getBlob?did=${encodeURIComponent(did)}&cid=${encodeURIComponent(cid)}`;packages/sdk-react/src/hooks/useProfile.ts (1)
105-111: Addpronounsfield toProfileUpdateinterface and include it in theupdateCertifiedProfilecall.The underlying
updateCertifiedProfileAPI fully supports thepronounsfield (viaAppCertifiedActorProfile.Main), but this is not exposed through the React hook'sProfileUpdatetype or the update mutation. To enable users to update pronouns through the hook, addpronouns?: string | nullto theProfileUpdateinterface inpackages/sdk-react/src/types.tsand pass it in the mutation call at lines 105-111.packages/sdk-core/src/repository/interfaces.ts (1)
687-690:UpdateCertifiedProfileParamsshould derive fromCreateCertifiedProfileParamsfor consistency.Per the coding guidelines,
UpdateXxxParamsshould bePartial<CreateXxxParams>. WhileAppCertifiedActorProfile.Mainis the record type (so the practical gap is smaller than the Bsky case), deriving fromCreateCertifiedProfileParamskeeps the two in sync and avoids drift.Proposed fix
-export type UpdateCertifiedProfileParams = OverrideProperties< - Nullable<Except<AppCertifiedActorProfile.Main, "$type" | "createdAt">>, - { avatar?: Blob | null; banner?: Blob | null } ->; +export type UpdateCertifiedProfileParams = Partial<CreateCertifiedProfileParams> & { + avatar?: Blob | null; + banner?: Blob | null; +};As per coding guidelines: "Define UpdateXxxParams as Partial for selective record updates."
Kzoeps
force-pushed
the
profile
branch
9 times, most recently
from
5fa030d to
1e5e282
Compare
This has been done because blobToJsonRef is very flaky. And is an unnecessary step since we can directly use the blobs returned from AtProto instead. This makes the code easier to maintain and also we dont have to manually construct json of blobs which are not only inaccurate but are also wrong in some cases;
…xport Remove deprecated Hypercert*Profile* type aliases in favor of Certified*Profile* types: - Remove HypercertProfile (use CertifiedProfileRecord) - Remove CreateHypercertProfileParams (use CreateCertifiedProfileParams) - Remove UpdateHypercertProfileParams (use UpdateCertifiedProfileParams) - Remove HypercertProfileParams union type - Remove CreateProfileParams alias Remove JsonBlobRef type export: - JsonBlobRef was too "snowflaky" - required unnecessary BlobRef to JSON conversion - BlobRef instances work perfectly fine for all use cases - Removed flaky test patterns with mock JSON objects - Users can import from @atproto/lexicon if needed Additional changes: - Update exports in src/index.ts and src/types.ts - Remove blobToJsonRef() method from HypercertOperationsImpl - Fix typo in ProfileOperationsImpl comment - Update changeset with migration guide and rationale BREAKING CHANGE: Users must update imports to use new type names
|
Rebased |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/sdk-core/src/repository/BlobOperationsImpl.ts (2)
34-50:⚠️ Potential issue | 🟡 MinorDocstring examples are stale — still reference the old
{ ref, mimeType, size }return shape.The
@exampleblocks (lines 34–49, 87–111) show destructuring{ ref, mimeType, size }from the upload result and accessingref.$link. Sinceupload()now returnsBlobRefdirectly, these examples should be updated to reflect the new API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/repository/BlobOperationsImpl.ts` around lines 34 - 50, Update the stale `@example` usage in BlobOperationsImpl to match the new upload() return type (BlobRef) instead of destructuring { ref, mimeType, size }; call const ref = await repo.blobs.upload(imageBlob) (or const blobRef = await repo.blobs.upload(...)) and then use ref.$link (or blobRef.$link) where examples currently reference ref.$link; apply the same change to both example blocks that show destructuring and accessing ref.$link so they reflect the current BlobRef return shape.
157-174:⚠️ Potential issue | 🔴 CriticalSDS upload path returns a plain JSON object without proper BlobRef interface.
response.json()returns a plain object, soresult.blobis{ ref: { $link: "..." }, mimeType, size }without BlobRef methods. Downstream code inHypercertOperationsImpl.ts:330callsimageBlobRef.ref.toString()expecting a CID method, but whenrefis a plain object{ $link: "..." }, calling.toString()returns"[object Object]"instead of the CID string. This will cause bugs in runtime when SDS-uploaded images are used in create/update operations.The function signature promises
Promise<BlobRef>but delivers an incompatible plain object. To fix, construct a properBlobReffrom the SDS JSON response:🔧 Proposed fix: construct a proper BlobRef from SDS response
+ import { BlobRef } from "@atproto/lexicon"; + import { CID } from "multiformats/cid"; // ... private async uploadViaSDS(data: Uint8Array, encoding: string): Promise<BlobRef> { // ... const result = await response.json(); - return result.blob; + const blob = result.blob; + const cid = typeof blob.ref === "string" + ? CID.parse(blob.ref) + : typeof blob.ref?.$link === "string" + ? CID.parse(blob.ref.$link) + : blob.ref; + return new BlobRef(cid, blob.mimeType, blob.size); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/repository/BlobOperationsImpl.ts` around lines 157 - 174, uploadViaSDS currently returns the raw JSON from response.json() which yields plain objects (e.g. result.blob.ref = { $link: "..." }) so downstream code like HypercertOperationsImpl imageBlobRef.ref.toString() gets "[object Object]"; change uploadViaSDS to parse the SDS JSON and construct/return a proper BlobRef instance that conforms to the BlobRef interface used elsewhere (ensure the returned object has a ref with a toString() that returns the CID string, and preserves mimeType/size), e.g. extract result.blob.ref.$link (or equivalent) and wrap it into the BlobRef shape before returning from uploadViaSDS so callers expecting BlobRef methods (like imageBlobRef.ref.toString()) work correctly.packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts (1)
104-147:⚠️ Potential issue | 🟡 MinorSDS test expectations confirm the type mismatch flagged in
BlobOperationsImpl.ts.Lines 119 and 146 assert
result.refis a plain object{ $link: ... }or a bare string — neither is a CID instance as expected by a realBlobRef. These tests will pass today but will break if downstream code later relies onBlobRefmethods (likeref.toString()).Once the SDS upload path is fixed to return a proper
BlobRef(per the comment onBlobOperationsImpl.ts), these tests should be updated to usecreateMockBlobRef()or assert via.ref.toString()as the PDS path test does on line 35.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts` around lines 104 - 147, Update the SDS-path tests to assert/produce a proper BlobRef instead of raw objects/strings: have the mocked SDS response return a BlobRef created with createMockBlobRef() (or convert the expected ref to the same type) and update assertions to compare via ref.toString() (or use createMockBlobRef().toString()) so they match the real BlobRef returned by sdsBlobOps.upload; target the test cases around sdsBlobOps.upload in BlobOperationsImpl.test.ts and ensure mockAgent.fetchHandler json payload and the expect(...) checks use createMockBlobRef()/ref.toString() rather than plain { $link: ... } or bare string.packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
2-12:⚠️ Potential issue | 🟡 MinorUnit tests should use shared mocks from
src/testing/.
This new import continues to rely ontests/utils/mocks.ts. If a shared mock exists, prefer that to keep unit tests consistent; otherwise, consider adding the helper there and re-exporting it.Based on learnings: Applies to packages/sdk-core/tests/**/*.test.ts : Use mocks from src/testing/ for unit tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts` around lines 2 - 12, The test imports createMockAgent, createMockBlobOperations, createMockBlobRef, and TEST_REPO_DID from tests/utils/mocks; update the import to use the shared testing helpers instead (the shared module that re-exports these mocks from src/testing), or if the helpers don't exist in src/testing yet, add and re-export them there and then change the test to import createMockAgent, createMockBlobOperations, createMockBlobRef, and TEST_REPO_DID from the shared testing module so all unit tests use the centralized mocks.
🧹 Nitpick comments (7)
.changeset/profile-certified-lexicon.md (1)
2-3: Add explicit caveat thatminorbumps introduce breaking changes for these 0.x packages.Both packages are pre-1.0, so a
minorbump is technically permissible for breaking changes under 0.x semver conventions. However, per project learnings, the changeset should explicitly note this and provide a rationale for not usingmajorso consumers aren't caught off guard.✏️ Suggested addition after the bump declarations
"@hypercerts-org/sdk-core": minor "@hypercerts-org/sdk-react": minor --- +> ⚠️ **Note:** Both packages are pre-1.0 (0.x). Per 0.x semver conventions, a `minor` bump is +> used here intentionally despite the breaking changes listed below. Consumers should treat +> any minor bump as potentially breaking until these packages reach 1.0.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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/profile-certified-lexicon.md around lines 2 - 3, Add an explicit caveat below the bump declarations for "@hypercerts-org/sdk-core" and "@hypercerts-org/sdk-react" noting that these are pre-1.0 packages and a "minor" bump may introduce breaking changes under 0.x semver; explicitly state any API removals or behavioral changes and give a short rationale why a "major" bump was not used so consumers are warned about compatibility implications.packages/sdk-core/tests/utils/mocks.ts (1)
255-264: Remove unusedcreateMockBlobReffromrepository-fixtures.tsor rename to avoid confusion.Two functions with the same name exist in different files:
packages/sdk-core/tests/utils/repository-fixtures.ts:70returns a plain object ({ $type, ref, mimeType, size }), while the new version inpackages/sdk-core/tests/utils/mocks.ts:255returns aBlobRefinstance with different parameters. The version in repository-fixtures.ts is exported but unused—no imports reference it anywhere in the codebase. Keep the mocks.ts version (which is actively used by ProfileOperationsImpl.test.ts and HypercertOperationsImpl.test.ts) and either delete or rename the unused one in repository-fixtures.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/utils/mocks.ts` around lines 255 - 264, There are two conflicting createMockBlobRef functions; keep the active one that returns a BlobRef (used by ProfileOperationsImpl.test.ts and HypercertOperationsImpl.test.ts) and remove or rename the unused plain-object-returning createMockBlobRef from repository-fixtures.ts to avoid ambiguity; update or delete the export in repository-fixtures.ts so only the BlobRef-returning createMockBlobRef remains (or give the unused one a distinct name) and ensure imports reference the BlobRef version.packages/sdk-react/src/utils/ssr.ts (1)
87-99: Migration togetCertifiedProfile()with proper null handling — approved.The null check on line 90 correctly handles the case where a user hasn't created a certified profile. The
handle ?? ""fallback on line 93 prevents null propagation to the client. The code properly maps all profile fields to the expectedProfileinterface shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-react/src/utils/ssr.ts` around lines 87 - 99, Keep the null check around repo.profile.getCertifiedProfile() and the returned mapping as-is: when profile is null return null, otherwise return an object matching the Profile shape using profile (use repo.profile.getCertifiedProfile() result stored in profile), preserve the handle fallback profile.handle ?? "" and map displayName, description, avatar, banner, website to their respective fields to ensure the client receives the expected shape.packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
1046-1068: Make blob-size assertions explicit.
Several tests assertblob.size === 100whilecreateMockBlobRefis called without a size. If the helper default changes, these tests will start failing. Consider passing the size explicitly in the mock helper for clarity and stability (and apply similarly in the other occurrences).Suggested update (example)
- mockBlobs.upload.mockResolvedValue(createMockBlobRef({ mimeType: "application/geo+json" })); + mockBlobs.upload.mockResolvedValue(createMockBlobRef({ mimeType: "application/geo+json", size: 100 }));Also applies to: 1681-1713, 1749-1775, 3186-3204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts` around lines 1046 - 1068, The tests rely on createMockBlobRef's implicit default size (asserting blob.size === 100) which makes them fragile; update the mock setup to pass an explicit size to createMockBlobRef wherever it's used (e.g., the mockBlobs.upload.mockResolvedValue calls that createBlob with createMockBlobRef) so the assertions like expect(call.record.location.blob.size).toBe(100) are guaranteed; apply the same explicit-size change for the other occurrences referenced in this file (the other test cases using createMockBlobRef and asserting blob.size).packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts (3)
610-706: Upsert error paths are untested.The
upsertCertifiedProfileandupsertBskyProfiledescribe blocks test the happy paths (create-when-missing, update-when-exists), but don't cover the case wheregetRecordthrows a genuine network error during the existence check. This would exercise the outercatchinupsertProfileRecord. Consider adding a test for completeness.Also applies to: 708-766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts` around lines 610 - 706, Add tests that simulate getRecord throwing a network error to exercise the outer catch in upsertProfileRecord: in the upsertCertifiedProfile and upsertBskyProfile describe blocks, add an it(...) where mockAgent.com.atproto.repo.getRecord.mockRejectedValue(new Error("network")) and assert that the upsert method (upsertCertifiedProfile/upsertBskyProfile) rejects with that error (or propagates it) and that neither createRecord nor putRecord (and no blob uploads) were called; reference the upsertCertifiedProfile/upsertBskyProfile call sites and upsertProfileRecord behavior to locate where to insert these tests.
51-58: Calling the method under test twice to check two aspects of the error is wasteful.Each
await expect(...)call invokesgetBskyProfile()a second time, triggering the mock again. Userejects.toThrowonce, or catch the error in a variable and assert on it.- await expect(profileOps.getBskyProfile()).rejects.toThrow(NetworkError); - await expect(profileOps.getBskyProfile()).rejects.toThrow("Failed to get Bluesky profile"); + const promise = profileOps.getBskyProfile(); + await expect(promise).rejects.toThrow(NetworkError); + await expect(promise).rejects.toThrow("Failed to get Bluesky profile");The same pattern repeats at lines 64-65 and 248-249.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts` around lines 51 - 58, The test calls profileOps.getBskyProfile() twice to assert type and message which invokes the mock twice; instead call it once and assert both properties (e.g., capture the returned promise or catch the thrown error) so you only invoke profileOps.getBskyProfile() a single time, and apply both assertions (instance of NetworkError and message contains "Failed to get Bluesky profile"); apply the same fix for the repeated patterns around the other occurrences (the similar double-await assertions at lines noted for the other tests).
8-9: Import profile collection constants from the source of truth.The test hardcodes
BSKY_PROFILE_COLLECTIONandCERTIFIED_PROFILE_COLLECTIONinstead of importing fromHYPERCERT_COLLECTIONS. While current values match the source, this creates maintenance risk: if the source constants change, these tests won't update automatically.Suggested fix
-const BSKY_PROFILE_COLLECTION = "app.bsky.actor.profile"; -const CERTIFIED_PROFILE_COLLECTION = "app.certified.actor.profile"; +import { HYPERCERT_COLLECTIONS } from "../../src/lexicons.js"; + +const BSKY_PROFILE_COLLECTION = HYPERCERT_COLLECTIONS.BSKY_PROFILE; +const CERTIFIED_PROFILE_COLLECTION = HYPERCERT_COLLECTIONS.CERTIFIED_PROFILE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts` around lines 8 - 9, Replace the hardcoded BSKY_PROFILE_COLLECTION and CERTIFIED_PROFILE_COLLECTION test constants with the canonical constants from HYPERCERT_COLLECTIONS: import or reference HYPERCERT_COLLECTIONS and use HYPERCERT_COLLECTIONS.BSKY_PROFILE_COLLECTION and HYPERCERT_COLLECTIONS.CERTIFIED_PROFILE_COLLECTION (or destructure { BSKY_PROFILE_COLLECTION, CERTIFIED_PROFILE_COLLECTION } = HYPERCERT_COLLECTIONS) in ProfileOperationsImpl.test.ts so the tests use the source-of-truth values instead of duplicated literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 54-55: Remove the unused imports ResolvedContributionDetails and
ResolvedContributorIdentity from the import list in HypercertOperationsImpl (or
prefix them with an underscore if intentionally kept for future use);
specifically update the import statement that currently lists
ResolvedContributionDetails and ResolvedContributorIdentity so they are either
deleted or renamed to _ResolvedContributionDetails and
_ResolvedContributorIdentity to satisfy the ESLint rule about unused variables.
In `@packages/sdk-core/src/repository/interfaces.ts`:
- Line 25: Remove the unused imported symbol StrongRef from the import list in
this module: locate the import statement that includes StrongRef (alongside
other repository interfaces) and delete just StrongRef from that named import;
also scan the file for any accidental references to StrongRef and remove or
replace them if present to ensure no unused symbol remains.
In `@packages/sdk-core/src/services/hypercerts/types.ts`:
- Around line 701-703: Add an UpdateCertifiedProfileParams type and a
CertifiedProfileParams union to complete the standard param set: define
UpdateCertifiedProfileParams = Partial<CreateCertifiedProfileParams> and define
CertifiedProfileParams = RefUri | CreateCertifiedProfileParams |
UpdateCertifiedProfileParams so callers can pass an AT-URI (RefUri), a full
create object, or a partial update object; reference the existing
CreateCertifiedProfileParams type when creating the Partial.
In `@packages/sdk-react/src/hooks/useProfile.ts`:
- Around line 101-116: The current saveMutation (mutationFn) always calls
repository.profile.upsertCertifiedProfile and maps null to undefined which
prevents null-based deletions; change the logic to detect existing profile or
any param === null and branch: if a profile exists (fetch via
repository.profile.getCertifiedProfile or similar) or any incoming value is
null, call repository.profile.updateCertifiedProfile and pass the raw values
(including nulls) so fields are deleted; otherwise (no existing profile and no
nulls) use upsertCertifiedProfile and continue mapping absent fields to
undefined for creation. Ensure this branching happens inside saveMutation and
reference the existing repository.profile.get/ getCertifiedProfile,
repository.profile.updateCertifiedProfile and
repository.profile.upsertCertifiedProfile methods.
In `@specs/02-repository-and-lexicons.md`:
- Around line 83-142: Update the ProfileManager spec to match the new SDK
surface: change getCertifiedProfile(): Promise<CertifiedProfile> to
getCertifiedProfile(): Promise<CertifiedProfile | null>, add an upsert method
(e.g., upsertCertifiedProfile(params: UpsertCertifiedProfileParams):
Promise<UpsertResult>) alongside or instead of separate
createCertifiedProfile/updateCertifiedProfile signatures on ProfileManager, and
update the CertifiedProfile, CreateCertifiedProfileParams and
UpdateCertifiedProfileParams interfaces so avatar and banner use the new image
record type (e.g., ImageRecord or ImageRef) instead of string/Blob, preserving
nullable semantics for update fields (displayName?: string | null, avatar?:
ImageRecord | null, etc.); ensure the new upsert params/type names
(UpsertCertifiedProfileParams, UpsertResult) are referenced consistently in the
spec.
In `@specs/04-react-and-clients.md`:
- Around line 157-160: The example incorrectly accesses repo.profile on the
result of useRepository() (a UseRepositoryResult); change usages to go through
the repository property (e.g., use repo.repository.profile.getBskyProfile()) and
add a guard for repository being present (check repository or handle undefined)
before calling profile methods to avoid runtime errors.
- Around line 154-155: The docs reference uses the wrong lexicon namespace:
update the note and any mentions of app.certified.actor.profile to the canonical
app.certified.profile to match the PR and the useProfile hook; search for the
string app.certified.actor.profile in the specs and change it to
app.certified.profile and ensure the useProfile hook documentation and examples
consistently state that it returns Certified profiles under
app.certified.profile.
- Around line 140-144: The onSubmit handler in the form currently calls
update(...) directly and will trigger a full page submit; change the handler to
accept the submit event (e) and call e.preventDefault() before invoking update({
displayName: newName, pronouns: newPronouns, website: newWebsite }) so the form
does not reload the page and the async update can complete (reference the form's
onSubmit handler and the update(...) call).
---
Outside diff comments:
In `@packages/sdk-core/src/repository/BlobOperationsImpl.ts`:
- Around line 34-50: Update the stale `@example` usage in BlobOperationsImpl to
match the new upload() return type (BlobRef) instead of destructuring { ref,
mimeType, size }; call const ref = await repo.blobs.upload(imageBlob) (or const
blobRef = await repo.blobs.upload(...)) and then use ref.$link (or
blobRef.$link) where examples currently reference ref.$link; apply the same
change to both example blocks that show destructuring and accessing ref.$link so
they reflect the current BlobRef return shape.
- Around line 157-174: uploadViaSDS currently returns the raw JSON from
response.json() which yields plain objects (e.g. result.blob.ref = { $link:
"..." }) so downstream code like HypercertOperationsImpl
imageBlobRef.ref.toString() gets "[object Object]"; change uploadViaSDS to parse
the SDS JSON and construct/return a proper BlobRef instance that conforms to the
BlobRef interface used elsewhere (ensure the returned object has a ref with a
toString() that returns the CID string, and preserves mimeType/size), e.g.
extract result.blob.ref.$link (or equivalent) and wrap it into the BlobRef shape
before returning from uploadViaSDS so callers expecting BlobRef methods (like
imageBlobRef.ref.toString()) work correctly.
In `@packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts`:
- Around line 104-147: Update the SDS-path tests to assert/produce a proper
BlobRef instead of raw objects/strings: have the mocked SDS response return a
BlobRef created with createMockBlobRef() (or convert the expected ref to the
same type) and update assertions to compare via ref.toString() (or use
createMockBlobRef().toString()) so they match the real BlobRef returned by
sdsBlobOps.upload; target the test cases around sdsBlobOps.upload in
BlobOperationsImpl.test.ts and ensure mockAgent.fetchHandler json payload and
the expect(...) checks use createMockBlobRef()/ref.toString() rather than plain
{ $link: ... } or bare string.
In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts`:
- Around line 2-12: The test imports createMockAgent, createMockBlobOperations,
createMockBlobRef, and TEST_REPO_DID from tests/utils/mocks; update the import
to use the shared testing helpers instead (the shared module that re-exports
these mocks from src/testing), or if the helpers don't exist in src/testing yet,
add and re-export them there and then change the test to import createMockAgent,
createMockBlobOperations, createMockBlobRef, and TEST_REPO_DID from the shared
testing module so all unit tests use the centralized mocks.
---
Duplicate comments:
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1821-1827: The evaluators mapping in
HypercertOperationsImpl.addEvaluation currently converts params.evaluators
(string[]) to Array<{ did: string }>, but ensure the mapping uses
params.evaluators.map(evaluator => ({ did: evaluator })) and that the
constructed HypercertEvaluation (variable evaluationRecord) sets evaluators to
that mapped array; verify the HYPERCERT_COLLECTIONS.EVALUATION $type and the
subject, summary, and createdAt assignments remain unchanged and that
addEvaluation's signature and CreateXxxParams refactor (tracked separately) will
not be broken by this mapping change.
In `@packages/sdk-core/src/repository/interfaces.ts`:
- Around line 708-711: The previous concern about UpdateBskyProfileParams using
ProfileViewDetailed has been addressed by basing it on CreateBskyProfileParams
and applying Nullable + Except to allow null-for-deletion semantics; no code
change is required—confirm UpdateBskyProfileParams and CreateBskyProfileParams
are the intended types and keep the OverrideProperties<
Nullable<Except<CreateBskyProfileParams, "$type" | "createdAt">>, { avatar?:
Blob | null; banner?: Blob | null } > definition as-is.
In `@packages/sdk-core/src/repository/ProfileOperationsImpl.ts`:
- Around line 308-364: upsertProfileRecord currently lets a RecordNotFound
thrown by this.agent.com.atproto.repo.getRecord bubble up and get wrapped as a
NetworkError instead of treating the record as missing; wrap the getRecord call
(or add a local try/catch) to catch the AT Protocol SDK's RecordNotFound error
and handle it the same way getCertifiedProfile does (treat as "not found" and
call createProfileRecord), while rethrowing other errors; apply this change in
upsertProfileRecord so that getRecord throws RecordNotFound => call
createProfileRecord(collection, params) rather than letting the outer catch
convert it to a NetworkError.
---
Nitpick comments:
In @.changeset/profile-certified-lexicon.md:
- Around line 2-3: Add an explicit caveat below the bump declarations for
"@hypercerts-org/sdk-core" and "@hypercerts-org/sdk-react" noting that these are
pre-1.0 packages and a "minor" bump may introduce breaking changes under 0.x
semver; explicitly state any API removals or behavioral changes and give a short
rationale why a "major" bump was not used so consumers are warned about
compatibility implications.
In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts`:
- Around line 1046-1068: The tests rely on createMockBlobRef's implicit default
size (asserting blob.size === 100) which makes them fragile; update the mock
setup to pass an explicit size to createMockBlobRef wherever it's used (e.g.,
the mockBlobs.upload.mockResolvedValue calls that createBlob with
createMockBlobRef) so the assertions like
expect(call.record.location.blob.size).toBe(100) are guaranteed; apply the same
explicit-size change for the other occurrences referenced in this file (the
other test cases using createMockBlobRef and asserting blob.size).
In `@packages/sdk-core/tests/repository/ProfileOperationsImpl.test.ts`:
- Around line 610-706: Add tests that simulate getRecord throwing a network
error to exercise the outer catch in upsertProfileRecord: in the
upsertCertifiedProfile and upsertBskyProfile describe blocks, add an it(...)
where mockAgent.com.atproto.repo.getRecord.mockRejectedValue(new
Error("network")) and assert that the upsert method
(upsertCertifiedProfile/upsertBskyProfile) rejects with that error (or
propagates it) and that neither createRecord nor putRecord (and no blob uploads)
were called; reference the upsertCertifiedProfile/upsertBskyProfile call sites
and upsertProfileRecord behavior to locate where to insert these tests.
- Around line 51-58: The test calls profileOps.getBskyProfile() twice to assert
type and message which invokes the mock twice; instead call it once and assert
both properties (e.g., capture the returned promise or catch the thrown error)
so you only invoke profileOps.getBskyProfile() a single time, and apply both
assertions (instance of NetworkError and message contains "Failed to get Bluesky
profile"); apply the same fix for the repeated patterns around the other
occurrences (the similar double-await assertions at lines noted for the other
tests).
- Around line 8-9: Replace the hardcoded BSKY_PROFILE_COLLECTION and
CERTIFIED_PROFILE_COLLECTION test constants with the canonical constants from
HYPERCERT_COLLECTIONS: import or reference HYPERCERT_COLLECTIONS and use
HYPERCERT_COLLECTIONS.BSKY_PROFILE_COLLECTION and
HYPERCERT_COLLECTIONS.CERTIFIED_PROFILE_COLLECTION (or destructure {
BSKY_PROFILE_COLLECTION, CERTIFIED_PROFILE_COLLECTION } = HYPERCERT_COLLECTIONS)
in ProfileOperationsImpl.test.ts so the tests use the source-of-truth values
instead of duplicated literals.
In `@packages/sdk-core/tests/utils/mocks.ts`:
- Around line 255-264: There are two conflicting createMockBlobRef functions;
keep the active one that returns a BlobRef (used by
ProfileOperationsImpl.test.ts and HypercertOperationsImpl.test.ts) and remove or
rename the unused plain-object-returning createMockBlobRef from
repository-fixtures.ts to avoid ambiguity; update or delete the export in
repository-fixtures.ts so only the BlobRef-returning createMockBlobRef remains
(or give the unused one a distinct name) and ensure imports reference the
BlobRef version.
In `@packages/sdk-react/src/utils/ssr.ts`:
- Around line 87-99: Keep the null check around
repo.profile.getCertifiedProfile() and the returned mapping as-is: when profile
is null return null, otherwise return an object matching the Profile shape using
profile (use repo.profile.getCertifiedProfile() result stored in profile),
preserve the handle fallback profile.handle ?? "" and map displayName,
description, avatar, banner, website to their respective fields to ensure the
client receives the expected shape.
| export type AttachmentParams = RefUri | CreateAttachmentParams; | ||
|
|
||
| export type CreateCertifiedProfileParams = SetOptional<AppCertifiedActorProfile.Main, "createdAt" | "$type">; |
There was a problem hiding this comment.
Add UpdateCertifiedProfileParams and CertifiedProfileParams union.
Only CreateCertifiedProfileParams was added; the standard type set is incomplete. Please add UpdateCertifiedProfileParams (as Partial<Create...>) and a Params union (RefUri | Create...) to keep the SDK surface consistent.
Suggested update
export type CreateCertifiedProfileParams = SetOptional<AppCertifiedActorProfile.Main, "createdAt" | "$type">;
+
+export type UpdateCertifiedProfileParams = Partial<CreateCertifiedProfileParams>;
+export type CertifiedProfileParams = RefUri | CreateCertifiedProfileParams;As per coding guidelines: For each lexicon entity type, define five types: lexicon Main type, HypercertXxx alias, CreateXxxParams, UpdateXxxParams, and XxxParams union type; Define UpdateXxxParams as Partial for selective record updates; For methods referencing potentially existing records, support three reference types: AT-URI, StrongRef, or CreateXxxParams object, using a union type XxxParams.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type AttachmentParams = RefUri | CreateAttachmentParams; | |
| export type CreateCertifiedProfileParams = SetOptional<AppCertifiedActorProfile.Main, "createdAt" | "$type">; | |
| export type AttachmentParams = RefUri | CreateAttachmentParams; | |
| export type CreateCertifiedProfileParams = SetOptional<AppCertifiedActorProfile.Main, "createdAt" | "$type">; | |
| export type UpdateCertifiedProfileParams = Partial<CreateCertifiedProfileParams>; | |
| export type CertifiedProfileParams = RefUri | CreateCertifiedProfileParams; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-core/src/services/hypercerts/types.ts` around lines 701 - 703,
Add an UpdateCertifiedProfileParams type and a CertifiedProfileParams union to
complete the standard param set: define UpdateCertifiedProfileParams =
Partial<CreateCertifiedProfileParams> and define CertifiedProfileParams = RefUri
| CreateCertifiedProfileParams | UpdateCertifiedProfileParams so callers can
pass an AT-URI (RefUri), a full create object, or a partial update object;
reference the existing CreateCertifiedProfileParams type when creating the
Partial.
| // Save mutation (uses upsert - works for both create and update) | ||
| const saveMutation = useMutation({ | ||
| mutationFn: async (params: ProfileUpdate) => { | ||
| if (!repository) { | ||
| throw new Error("Repository not available"); | ||
| } | ||
|
|
||
| await repository.profile.update({ | ||
| displayName: params.displayName, | ||
| description: params.description, | ||
| avatar: params.avatar, | ||
| banner: params.banner, | ||
| website: params.website, | ||
| // Use upsert instead of update - works for first-time profile creation too | ||
| // Filter out null values for upsert (upsert uses create params, update uses null for deletion) | ||
| await repository.profile.upsertCertifiedProfile({ | ||
| displayName: params.displayName ?? undefined, | ||
| description: params.description ?? undefined, | ||
| avatar: params.avatar ?? undefined, | ||
| banner: params.banner ?? undefined, | ||
| website: params.website ?? undefined, | ||
| }); |
There was a problem hiding this comment.
Save path drops null removals.
ProfileUpdate allows null to clear fields, but converting null → undefined turns deletions into no-ops. Consider using updateCertifiedProfile when a profile exists (or when any param is null) and reserve upsert for create paths.
Possible fix
- await repository.profile.upsertCertifiedProfile({
- displayName: params.displayName ?? undefined,
- description: params.description ?? undefined,
- avatar: params.avatar ?? undefined,
- banner: params.banner ?? undefined,
- website: params.website ?? undefined,
- });
+ if (profileQuery.data) {
+ await repository.profile.updateCertifiedProfile(params);
+ return;
+ }
+ await repository.profile.upsertCertifiedProfile({
+ displayName: params.displayName ?? undefined,
+ description: params.description ?? undefined,
+ avatar: params.avatar ?? undefined,
+ banner: params.banner ?? undefined,
+ website: params.website ?? undefined,
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-react/src/hooks/useProfile.ts` around lines 101 - 116, The
current saveMutation (mutationFn) always calls
repository.profile.upsertCertifiedProfile and maps null to undefined which
prevents null-based deletions; change the logic to detect existing profile or
any param === null and branch: if a profile exists (fetch via
repository.profile.getCertifiedProfile or similar) or any incoming value is
null, call repository.profile.updateCertifiedProfile and pass the raw values
(including nulls) so fields are deleted; otherwise (no existing profile and no
nulls) use upsertCertifiedProfile and continue mapping absent fields to
undefined for creation. Ensure this branching happens inside saveMutation and
reference the existing repository.profile.get/ getCertifiedProfile,
repository.profile.updateCertifiedProfile and
repository.profile.upsertCertifiedProfile methods.
| The ProfileManager supports dual profile types: Bluesky (standard AT Protocol) and Certified (hypercerts-specific with | ||
| additional fields). | ||
|
|
||
| ```typescript | ||
| export class ProfileManager { | ||
| async get(repo: string): Promise<{ | ||
| handle: string; | ||
| displayName?: string; | ||
| description?: string; | ||
| avatar?: string; | ||
| banner?: string; | ||
| website?: string; | ||
| }>; | ||
| // Bluesky profiles (app.bsky.actor.profile) | ||
| async getBskyProfile(): Promise<BskyProfile>; | ||
| async createBskyProfile(params: CreateBskyProfileParams): Promise<CreateResult>; | ||
| async updateBskyProfile(params: UpdateBskyProfileParams): Promise<UpdateResult>; | ||
|
|
||
| // Certified profiles (app.certified.actor.profile) | ||
| async getCertifiedProfile(): Promise<CertifiedProfile>; | ||
| async createCertifiedProfile(params: CreateCertifiedProfileParams): Promise<CreateResult>; | ||
| async updateCertifiedProfile(params: UpdateCertifiedProfileParams): Promise<UpdateResult>; | ||
| } | ||
|
|
||
| async update(params: { | ||
| repo: string; | ||
| displayName?: string | null; | ||
| description?: string | null; | ||
| avatar?: Blob | null; | ||
| banner?: Blob | null; | ||
| website?: string | null; | ||
| }): Promise<{ uri: string; cid: string }>; | ||
| // Types | ||
| export type BskyProfile = AppBskyActorDefs.ProfileViewDetailed; | ||
|
|
||
| export interface CertifiedProfile { | ||
| handle?: string; | ||
| displayName?: string; | ||
| description?: string; | ||
| avatar?: string; // Blob URL | ||
| banner?: string; // Blob URL | ||
| pronouns?: string; // Max 20 graphemes | ||
| website?: string; | ||
| } | ||
|
|
||
| export interface CreateBskyProfileParams { | ||
| displayName?: string; | ||
| description?: string; | ||
| avatar?: Blob; | ||
| banner?: Blob; | ||
| } | ||
|
|
||
| export interface UpdateBskyProfileParams { | ||
| displayName?: string | null; | ||
| description?: string | null; | ||
| avatar?: Blob | null; | ||
| banner?: Blob | null; | ||
| } | ||
|
|
||
| export interface CreateCertifiedProfileParams { | ||
| displayName?: string; | ||
| description?: string; | ||
| avatar?: Blob; | ||
| banner?: Blob; | ||
| pronouns?: string; | ||
| website?: string; | ||
| } | ||
|
|
||
| export interface UpdateCertifiedProfileParams { | ||
| displayName?: string | null; | ||
| description?: string | null; | ||
| avatar?: Blob | null; | ||
| banner?: Blob | null; | ||
| pronouns?: string | null; | ||
| website?: string | null; | ||
| } |
There was a problem hiding this comment.
Spec should reflect nullable Certified profiles, upserts, and image record types.
The ProfileManager section still documents getCertifiedProfile as non-null and omits upsert methods; avatar/banner are still shown as strings. Please update the spec to match the new SDK surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/02-repository-and-lexicons.md` around lines 83 - 142, Update the
ProfileManager spec to match the new SDK surface: change getCertifiedProfile():
Promise<CertifiedProfile> to getCertifiedProfile(): Promise<CertifiedProfile |
null>, add an upsert method (e.g., upsertCertifiedProfile(params:
UpsertCertifiedProfileParams): Promise<UpsertResult>) alongside or instead of
separate createCertifiedProfile/updateCertifiedProfile signatures on
ProfileManager, and update the CertifiedProfile, CreateCertifiedProfileParams
and UpdateCertifiedProfileParams interfaces so avatar and banner use the new
image record type (e.g., ImageRecord or ImageRef) instead of string/Blob,
preserving nullable semantics for update fields (displayName?: string | null,
avatar?: ImageRecord | null, etc.); ensure the new upsert params/type names
(UpsertCertifiedProfileParams, UpsertResult) are referenced consistently in the
spec.
| <form onSubmit={() => update({ | ||
| displayName: newName, | ||
| pronouns: newPronouns, | ||
| website: newWebsite | ||
| })}> |
There was a problem hiding this comment.
Missing e.preventDefault() in onSubmit handler — example would cause a page reload.
Without preventing the default browser form submission, this example will navigate/reload the page before update(...) resolves.
📝 Proposed fix
- <form onSubmit={() => update({
+ <form onSubmit={(e) => { e.preventDefault(); update({
displayName: newName,
pronouns: newPronouns,
website: newWebsite
- })}>
+ }); }}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <form onSubmit={() => update({ | |
| displayName: newName, | |
| pronouns: newPronouns, | |
| website: newWebsite | |
| })}> | |
| <form onSubmit={(e) => { e.preventDefault(); update({ | |
| displayName: newName, | |
| pronouns: newPronouns, | |
| website: newWebsite | |
| }); }}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/04-react-and-clients.md` around lines 140 - 144, The onSubmit handler
in the form currently calls update(...) directly and will trigger a full page
submit; change the handler to accept the submit event (e) and call
e.preventDefault() before invoking update({ displayName: newName, pronouns:
newPronouns, website: newWebsite }) so the form does not reload the page and the
async update can complete (reference the form's onSubmit handler and the
update(...) call).
| **Note**: The `useProfile` hook returns Certified profiles (`app.certified.actor.profile`) which include | ||
| hypercerts-specific fields. For Bluesky profiles, use the core SDK directly: |
There was a problem hiding this comment.
Lexicon namespace mismatch: app.certified.actor.profile vs app.certified.profile.
The PR description states the migration target is app.certified.profile, but the note here uses app.certified.actor.profile. Align with whichever namespace is canonical.
📝 Proposed fix
-**Note**: The `useProfile` hook returns Certified profiles (`app.certified.actor.profile`) which include
+**Note**: The `useProfile` hook returns Certified profiles (`app.certified.profile`) which include📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Note**: The `useProfile` hook returns Certified profiles (`app.certified.actor.profile`) which include | |
| hypercerts-specific fields. For Bluesky profiles, use the core SDK directly: | |
| **Note**: The `useProfile` hook returns Certified profiles (`app.certified.profile`) which include | |
| hypercerts-specific fields. For Bluesky profiles, use the core SDK directly: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/04-react-and-clients.md` around lines 154 - 155, The docs reference
uses the wrong lexicon namespace: update the note and any mentions of
app.certified.actor.profile to the canonical app.certified.profile to match the
PR and the useProfile hook; search for the string app.certified.actor.profile in
the specs and change it to app.certified.profile and ensure the useProfile hook
documentation and examples consistently state that it returns Certified profiles
under app.certified.profile.
| ```typescript | ||
| const repo = useRepository(); | ||
| const bskyProfile = await repo.profile.getBskyProfile(); | ||
| ``` |
There was a problem hiding this comment.
Incorrect property access in the Bluesky example — repo.profile does not exist on UseRepositoryResult.
useRepository() returns a UseRepositoryResult (defined on line 87 of this same file), whose properties are repository, status, error, isSDS, and serverUrl. Accessing repo.profile would be undefined at runtime; the profile operations live under repo.repository.
📝 Proposed fix
-const repo = useRepository();
-const bskyProfile = await repo.profile.getBskyProfile();
+const { repository } = useRepository();
+const bskyProfile = await repository?.profile.getBskyProfile();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/04-react-and-clients.md` around lines 157 - 160, The example
incorrectly accesses repo.profile on the result of useRepository() (a
UseRepositoryResult); change usages to go through the repository property (e.g.,
use repo.repository.profile.getBskyProfile()) and add a guard for repository
being present (check repository or handle undefined) before calling profile
methods to avoid runtime errors.
| - **Removed unused type export:** | ||
| - ❌ `JsonBlobRef` - No longer used in SDK. This type was removed because: | ||
| - It was too "snowflaky" - required converting `BlobRef` instances to JSON format unnecessarily | ||
| - The actual `BlobRef` object works perfectly fine for all use cases |
There was a problem hiding this comment.
That's not true, we had really nasty problems with BlobRef as per https://www.notion.so/try-out-atproto-lex-2c0d7a3bc90d800085faf8eefac34a8e?source=copy_link#2f0d7a3bc90d804fb83ff615dff254c8 and bluesky-social/atproto#4168
Nevertheless I'm not sure JsonBlobRef helped either.
… uploadResultToBlobRef
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
324-342: Good approach: extracting CID string for deterministic hashing.Using
imageBlobRef.ref.toString()instead of embedding the fullBlobRefobject in the hash input ensures JSON-serializability and deterministic rKey generation.Minor nit: Line 324 comment has a typo — "its" should be "it's" (
// if it's a blob ref...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts` around lines 324 - 342, The comment above the imageBlobRef handling has a typo: change the comment "// if its a blob ref guaranteed to have ref and a .toString method" to use the contraction "it's" so it reads "// if it's a blob ref guaranteed to have ref and a .toString method"; update the comment near the imageBlobRef / imageRef block in HypercertOperationsImpl (the lines around the imageBlobRef check and imageRef assignment) to correct the spelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-core/src/index.ts`:
- Around line 103-104: Add and export the missing Hypercert alias and params
union: export the existing CertifiedProfile alias (named CertifiedProfile in
interfaces.ts) alongside the already-exported CreateCertifiedProfileParams,
UpdateCertifiedProfileParams, and CertifiedProfileRecord, and define/export a
new CertifiedProfileParams union type that combines AT-URI, StrongRef, and the
CreateCertifiedProfileParams (e.g., type CertifiedProfileParams = string |
StrongRef | CreateCertifiedProfileParams) so the module follows the same pattern
as other entities like ContributionDetailsParams; update the export list in
packages/sdk-core/src/index.ts to include CertifiedProfile and
CertifiedProfileParams.
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Line 2021: Fix the JSDoc stray asterisk in the comment above the
HypercertOperationsImpl method by changing the `* * `@example`` token to a single
`* `@example``; locate the JSDoc block near the HypercertOperationsImpl
class/constructor or the method surrounding the double asterisk and remove the
extra `*` so the tag reads `@example` correctly.
---
Duplicate comments:
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1818-1824: The evaluators mapping in the HypercertEvaluation
construction must convert string evaluator IDs into the lexicon object shape;
replace the current string array assignment with mapping such that
evaluationRecord (of type HypercertEvaluation) sets evaluators to
params.evaluators.map(evaluator => ({ did: evaluator })); ensure this change is
applied where evaluationRecord is built in HypercertOperationsImpl so the field
type matches HypercertEvaluation and resolves the TS2322 mismatch.
---
Nitpick comments:
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 324-342: The comment above the imageBlobRef handling has a typo:
change the comment "// if its a blob ref guaranteed to have ref and a .toString
method" to use the contraction "it's" so it reads "// if it's a blob ref
guaranteed to have ref and a .toString method"; update the comment near the
imageBlobRef / imageRef block in HypercertOperationsImpl (the lines around the
imageBlobRef check and imageRef assignment) to correct the spelling.
| CreateCertifiedProfileParams, | ||
| UpdateCertifiedProfileParams, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all CertifiedProfile-related types
echo "=== CertifiedProfile type definitions ==="
rg -n "type.*CertifiedProfile|interface.*CertifiedProfile" packages/sdk-core/src/ --type=ts
echo -e "\n=== Check for CertifiedProfileParams union ==="
rg -n "CertifiedProfileParams\s*=" packages/sdk-core/src/ --type=ts
echo -e "\n=== Check interfaces.ts for all profile-related types ==="
grep -n "Profile" packages/sdk-core/src/repository/interfaces.ts | head -20
echo -e "\n=== Check types.ts for CertifiedProfile ==="
grep -n "CertifiedProfile" packages/sdk-core/src/services/hypercerts/types.ts
echo -e "\n=== Check current exports in index.ts for CertifiedProfile ==="
grep -n "CertifiedProfile" packages/sdk-core/src/index.tsRepository: hypercerts-org/hypercerts-sdk
Length of output: 2857
🏁 Script executed:
#!/bin/bash
# Check how other entities define their union types (XxxParams)
echo "=== Look for XxxParams union type patterns in interfaces.ts ==="
rg -n "export type \w+Params = " packages/sdk-core/src/repository/interfaces.ts | head -20
echo -e "\n=== Check for CertifiedProfileParams union specifically ==="
rg -n "export type CertifiedProfileParams" packages/sdk-core/src/ --type=ts
echo -e "\n=== Check BskyProfileParams as comparison (should show pattern) ==="
rg -n "export type BskyProfileParams|export type BskyProfile\b" packages/sdk-core/src/repository/interfaces.ts -A 3
echo -e "\n=== Check what's exported from index.ts for other profile types ==="
rg -n "BskyProfile" packages/sdk-core/src/index.tsRepository: hypercerts-org/hypercerts-sdk
Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Check what's exported from index.ts around the profile sections
echo "=== CertifiedProfile exports in index.ts context ==="
sed -n '100,115p' packages/sdk-core/src/index.ts
echo -e "\n=== BskyProfile exports in index.ts ==="
grep -n "BskyProfile\|Profile" packages/sdk-core/src/index.ts | grep -i profile
echo -e "\n=== Full list of what's exported from interfaces.ts ==="
rg "^export type|^export interface" packages/sdk-core/src/repository/interfaces.ts | grep -i profile
echo -e "\n=== Check if there's a CertifiedProfileParams defined as union elsewhere ==="
rg -n "CertifiedProfileParams" packages/sdk-core/src/ --type=ts -B 2 -A 2Repository: hypercerts-org/hypercerts-sdk
Length of output: 8301
Export missing CertifiedProfile alias and define CertifiedProfileParams union type.
Per coding guidelines, each lexicon entity type should define five types: lexicon Main type, HypercertXxx alias, CreateXxxParams, UpdateXxxParams, and XxxParams union type. The exports include CreateCertifiedProfileParams, UpdateCertifiedProfileParams, and CertifiedProfileRecord, but are missing:
CertifiedProfile(the HypercertXxx alias)—currently defined ininterfaces.tsbut not exportedCertifiedProfileParamsunion type (AT-URI | StrongRef | CreateCertifiedProfileParams)—not yet defined
Both should be added to align with the type system pattern used for other entities like ContributionDetailsParams.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-core/src/index.ts` around lines 103 - 104, Add and export the
missing Hypercert alias and params union: export the existing CertifiedProfile
alias (named CertifiedProfile in interfaces.ts) alongside the already-exported
CreateCertifiedProfileParams, UpdateCertifiedProfileParams, and
CertifiedProfileRecord, and define/export a new CertifiedProfileParams union
type that combines AT-URI, StrongRef, and the CreateCertifiedProfileParams
(e.g., type CertifiedProfileParams = string | StrongRef |
CreateCertifiedProfileParams) so the module follows the same pattern as other
entities like ContributionDetailsParams; update the export list in
packages/sdk-core/src/index.ts to include CertifiedProfile and
CertifiedProfileParams.
| * @throws {@link NetworkError} if the list operation fails | ||
| * | ||
| * @example | ||
| * * @example |
There was a problem hiding this comment.
Minor JSDoc formatting: stray asterisk.
Line 2021 has * * @example (double asterisk). Should be `* `@example.
📝 Proposed fix
- * * `@example`
+ * `@example`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * * @example | |
| * `@example` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts` at line 2021,
Fix the JSDoc stray asterisk in the comment above the HypercertOperationsImpl
method by changing the `* * `@example`` token to a single `* `@example``; locate the
JSDoc block near the HypercertOperationsImpl class/constructor or the method
surrounding the double asterisk and remove the extra `*` so the tag reads
`@example` correctly.
2 participants
Summary
app.certified.profile) instead of Bluesky actor profilepronounsfield support (max 20 graphemes)HypercertImageRecordfor avatar/bannerChanges
Core SDK (
sdk-core)app.certified.profilecollection with proper image wrappingHypercertProfile,CreateHypercertProfileParams,UpdateHypercertProfileParamstypesProfileOperationsandProfileParamsto includepronounsand useHypercertImageRecordReact SDK (
sdk-react)Profileinterface to useHypercertImageRecordfor avatar/bannerBreaking Changes
HypercertImageRecordobjects instead of stringsapp.certified.profilecollection instead ofapp.bsky.actor.profileSummary by CodeRabbit
New Features
Improvements
Tests
Breaking Changes