Fix: Attach location to activity claim #90
Conversation
…-get-linked-to-hypercert
🦋 Changeset detectedLatest commit: 044d202 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe PR refactors evidence and location handling in the SDK core. It introduces new public parameter types ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant SDK as addEvidence()
participant Validation
participant BlobStorage
participant Repository
Client->>SDK: evidence: CreateHypercertEvidenceParams<br/>(subjectUri, content: string | Blob, ...)
alt Content is Blob
SDK->>BlobStorage: uploadBlob(content)
BlobStorage-->>SDK: blobRef
SDK->>SDK: contentUri = blobRef
else Content is String URI
SDK->>SDK: contentUri = content
end
SDK->>SDK: Construct HypercertEvidence<br/>($type, subject, content, ...)
SDK->>Validation: validate(HypercertEvidence)
alt Validation passes
Validation-->>SDK: ✓ valid
SDK->>Repository: createRecord(HypercertEvidence)
Repository-->>SDK: result
SDK->>SDK: emit evidenceAdded event
SDK-->>Client: UpdateResult
else Validation fails
Validation-->>SDK: ✗ ValidationError
SDK-->>Client: throw ValidationError
end
sequenceDiagram
participant Client
participant SDK as attachLocation()
participant BlobStorage
participant Repository
Client->>SDK: location: AttachLocationParams<br/>(location: string | Blob, name, srs, ...)
alt Location is Blob
SDK->>BlobStorage: uploadBlob(location)
BlobStorage-->>SDK: blobRef
SDK->>SDK: Create HypercertLocation<br/>with smallBlob payload
else Location is String URI
SDK->>SDK: Create HypercertLocation<br/>with location URI
end
SDK->>Repository: createRecord(HypercertLocation)
Repository-->>SDK: locationRecord
SDK->>Repository: putRecord(hypercertUri)<br/>+ strongRef to locationRecord
Repository-->>SDK: result
SDK->>SDK: emit locationAttached event
SDK-->>Client: CreateResult
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/HypercertOperationsImpl.ts (1)
813-822:lpVersionfrom params is not used.The
AttachLocationParamsinterface includes anlpVersionfield, but the implementation hard-codeslpVersion: "1.0"at line 815 instead of usinglocation.lpVersionfrom the params. This could lead to unexpected behavior if users provide a different version.🔧 Proposed fix
const locationRecord: HypercertLocation = { $type: HYPERCERT_COLLECTIONS.LOCATION, - lpVersion: "1.0", + lpVersion: location.lpVersion, srs: location.srs, locationType: location.locationType, location: locationData,
🤖 Fix all issues with AI agents
In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts`:
- Around line 507-510: The mock response for
mockAgent.com.atproto.repo.createRecord in HypercertOperationsImpl.test.ts uses
the old "org.hypercerts.claim.location" namespace while the test otherwise
expects the updated "app.certified.location" lexicon; update the mocked data.uri
to use "at://did:plc:test/app.certified.location/xyz" (and adjust the cid string
if needed) so the mock matches the rest of the test expectations and the lexicon
referenced elsewhere in the test suite.
🧹 Nitpick comments (2)
packages/sdk-core/src/repository/interfaces.ts (1)
241-278: Consider consolidating withHypercertEvidenceInputtype.Both
CreateHypercertEvidenceParamsandHypercertEvidenceInput(intypes.ts) serve similar purposes with overlapping fields (subjectUri,content). Consider whether one can extend or replace the other to reduce duplication and maintain consistency.That said, the current separation may be intentional if
HypercertEvidenceInputis meant for internal use whileCreateHypercertEvidenceParamsis the public API parameter type.packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
513-562: Good test coverage for both URI and Blob paths.The tests thoroughly verify the location record structure for both string URI and GeoJSON Blob inputs. The Blob test correctly verifies the
smallBlobwrapper structure.Consider using more specific assertions for the URI validation (e.g.,
toMatch(/app\.certified\.location/)) instead oftoContain("location")for stronger verification.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/fix-add-evidence.md.changeset/modern-planes-grin.mdpackages/sdk-core/src/index.tspackages/sdk-core/src/repository/HypercertOperationsImpl.tspackages/sdk-core/src/repository/interfaces.tspackages/sdk-core/src/repository/types.tspackages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use strict TypeScript with strict: true configuration
Avoid any types (warns in ESLint)
Prefix unused variables with underscore (_)
Use Zod schemas for runtime validation
Use one class/module per file
Classes should use PascalCase naming (e.g., OAuthClient, Repository)
Files should use PascalCase for classes and camelCase for utilities
Types and Interfaces should use PascalCase with descriptive names
Constants should use UPPER_SNAKE_CASE
Use the error hierarchy from sdk-core/src/core/errors.ts with base HypercertsError and specific error classes (ValidationError, AuthenticationError, NetworkError, NotFoundError, SDSRequiredError)
Files:
packages/sdk-core/src/index.tspackages/sdk-core/src/repository/types.tspackages/sdk-core/tests/repository/HypercertOperationsImpl.test.tspackages/sdk-core/src/repository/HypercertOperationsImpl.tspackages/sdk-core/src/repository/interfaces.ts
packages/sdk-*/src/index.ts
📄 CodeRabbit inference engine (AGENTS.md)
Export types from dedicated entrypoints
Files:
packages/sdk-core/src/index.ts
**/tests/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.test.ts: Test files should mirror source structure and be placed in tests/ directory with *.test.ts extension
Minimum test coverage: 70% for lines, functions, and statements; 60% for branches
Use mocks from src/testing/ for unit tests
Use fixtures from tests/utils/fixtures.ts in unit tests
Files:
packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts
🧠 Learnings (4)
📚 Learning: 2026-01-06T17:56:38.894Z
Learnt from: CR
Repo: hypercerts-org/hypercerts-sdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T17:56:38.894Z
Learning: Applies to packages/sdk-*/src/index.ts : Export types from dedicated entrypoints
Applied to files:
packages/sdk-core/src/index.ts
📚 Learning: 2026-01-06T17:56:38.894Z
Learnt from: CR
Repo: hypercerts-org/hypercerts-sdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T17:56:38.894Z
Learning: Applies to packages/sdk-*/src/*/index.ts : Use barrel exports (index.ts) for public APIs
Applied to files:
packages/sdk-core/src/index.ts
📚 Learning: 2026-01-06T17:56:38.894Z
Learnt from: CR
Repo: hypercerts-org/hypercerts-sdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T17:56:38.894Z
Learning: Applies to **/*.{ts,tsx} : Use the error hierarchy from sdk-core/src/core/errors.ts with base HypercertsError and specific error classes (ValidationError, AuthenticationError, NetworkError, NotFoundError, SDSRequiredError)
Applied to files:
packages/sdk-core/src/index.tspackages/sdk-core/src/repository/types.tspackages/sdk-core/src/repository/HypercertOperationsImpl.tspackages/sdk-core/src/repository/interfaces.ts
📚 Learning: 2026-01-06T17:56:38.894Z
Learnt from: CR
Repo: hypercerts-org/hypercerts-sdk PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T17:56:38.894Z
Learning: One record type requires: types in services/<domain>/types.ts, Zod schemas in services/<domain>/schemas.ts, lexicon registration in lexicons/<domain>/index.ts, operations in repository/<Domain>OperationsImpl.ts, tests in tests/repository/, and optional React hook in sdk-react/src/hooks/
Applied to files:
packages/sdk-core/src/index.tspackages/sdk-core/src/repository/interfaces.ts
🧬 Code graph analysis (1)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (5)
packages/sdk-core/src/repository/interfaces.ts (1)
AttachLocationParams(309-322)packages/sdk-core/src/repository/types.ts (2)
CreateResult(26-29)UpdateResult(34-37)packages/sdk-core/src/services/hypercerts/types.ts (3)
HypercertLocation(87-87)HYPERCERT_COLLECTIONS(42-42)HypercertEvidence(83-83)packages/sdk-core/src/core/errors.ts (2)
NetworkError(209-220)ValidationError(170-181)packages/sdk-core/src/lexicons.ts (1)
HYPERCERT_COLLECTIONS(125-190)
🔇 Additional comments (14)
packages/sdk-core/src/repository/types.ts (1)
114-117: LGTM!The new
HypercertEvidenceInputtype is well-designed. It correctly omits the fields that will be constructed by the SDK ($type,createdAt,subject,content) and adds the user-facingsubjectUriand flexiblecontent(string | Blob) fields.packages/sdk-core/src/repository/interfaces.ts (2)
309-322: LGTM!The
AttachLocationParamsinterface is well-structured with:
- Required fields (
lpVersion,srs,locationType,location) for Location Protocol compliance- Flexible
locationtype supporting both URI strings and GeoJSON Blobs- Good JSDoc examples demonstrating both usage patterns
762-762: Verify return type:UpdateResultvsCreateResult.The
addEvidencemethod creates a new evidence record (as per the PR description and implementation). However, it returnsUpdateResultinstead ofCreateResult. While both types have the same shape ({ uri: string; cid: string }), usingCreateResultwould be more semantically accurate for a record creation operation.packages/sdk-core/src/index.ts (1)
28-28: LGTM!The new public type exports are correctly added:
HypercertEvidenceInputfrom./repository/types.jsAttachLocationParams,CreateHypercertEvidenceParams,CreateOrganizationParamsfrom./repository/interfaces.jsThis follows the coding guideline to export types from dedicated entrypoints.
Also applies to: 40-42
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (5)
294-313: LGTM!The
attachLocationWithProgressmethod correctly wraps theattachLocationcall with progress event emissions and proper error handling.
798-811: LGTM!The blob upload path correctly handles the Blob case with appropriate error checking and uses
NetworkErrorfrom the error hierarchy as per coding guidelines.
839-848: LGTM!The location attachment correctly creates a
strongRefand updates the hypercert record to link to the newly created location. This implements the PR objective of attaching location to the activity claim.
866-921: Implementation correctly builds evidence record in SDK.The
addEvidencemethod properly:
- Resolves the subject from URI to get
uriandcid- Handles both string URI and Blob content
- Constructs the full
HypercertEvidencerecord with$type,createdAt,content, andsubject- Validates against the lexicon before creation
- Creates the evidence as a separate record (not appending to hypercert)
This aligns with the PR objectives and changeset description.
896-906: The custom fields support via...restspread is intentional and properly validated.The index signature
[k: string]: unknowninCreateHypercertEvidenceParamsis explicitly documented as "Any additional custom fields supported by the record." The spread at line 897 is a deliberate extensibility pattern. The subsequentvalidate()call at lines 903-906 enforces the lexicon schema before the record is persisted, consistent with how all other record types (rights, location, measurement, etc.) are handled in this class..changeset/fix-add-evidence.md (1)
1-6: LGTM!The changeset correctly documents:
- Minor version bump (appropriate for new features)
- The behavioral change where evidence is now a standalone record
- The SDK now constructs
$type,uri, andsubjectinternallyThe description aligns with the implementation changes.
.changeset/modern-planes-grin.md (1)
1-6: LGTM!The changeset is properly formatted with a minor version bump, which is appropriate for the new features being added (file upload handling, new parameter types, and location attachment updates).
packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (3)
128-128: LGTM!The location URI namespace update from
org.hypercerts.claim.locationtoapp.certified.locationaligns with the updated lexicon mentioned in the PR objectives.
152-170: LGTM!The test correctly sets up the
putRecordmock needed for the location attachment flow that now updates the hypercert record with a reference to the location. The location params structure properly reflects the newAttachLocationParamsinterface.
642-722: LGTM! Comprehensive test coverage for addEvidence.The test suite covers:
- URI string content path with correct
org.hypercerts.defs#uristructure- Blob upload path with correct
org.hypercerts.defs#smallBlobstructure- Event emission verification with expected payload
This aligns well with the new
CreateHypercertEvidenceParamsinterface and covers the key flows.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| mockAgent.com.atproto.repo.createRecord.mockResolvedValue({ | ||
| success: true, | ||
| data: { uri: "at://did:plc:test/org.hypercerts.claim.location/xyz", cid: "location-cid" }, | ||
| }); |
There was a problem hiding this comment.
Inconsistent location URI namespace in mock.
The mock at line 509 uses org.hypercerts.claim.location namespace, but line 128 uses the updated app.certified.location namespace. This inconsistency could cause confusion about which lexicon is actually being used.
Proposed fix
mockAgent.com.atproto.repo.createRecord.mockResolvedValue({
success: true,
- data: { uri: "at://did:plc:test/org.hypercerts.claim.location/xyz", cid: "location-cid" },
+ data: { uri: "at://did:plc:test/app.certified.location/xyz", cid: "location-cid" },
});🤖 Prompt for AI Agents
In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts` around
lines 507 - 510, The mock response for mockAgent.com.atproto.repo.createRecord
in HypercertOperationsImpl.test.ts uses the old "org.hypercerts.claim.location"
namespace while the test otherwise expects the updated "app.certified.location"
lexicon; update the mocked data.uri to use
"at://did:plc:test/app.certified.location/xyz" (and adjust the cid string if
needed) so the mock matches the rest of the test expectations and the lexicon
referenced elsewhere in the test suite.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.