fix: addContribution updates hypercert contributors#130
fix: addContribution updates hypercert contributors#130aspiers merged 1 commit intohypercerts-org:developfrom
Conversation
🦋 Changeset detectedLatest commit: a9701cd 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 |
📝 WalkthroughWalkthroughRefactors hypercert contribution and update flows: adds RefUri types, introduces UpdateHypercertParams, changes signatures for Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Ops as HypercertOperationsImpl
participant Store as Repository/Records
participant Resolver as Identity/Details Resolver
Client->>Ops: addContribution(hypercertUri, contributors[], contributionDetails, weight?, onProgress?)
Ops->>Ops: parseAndValidateUri(hypercertUri)
Ops->>Resolver: resolveContributorIdentity(each contributor)
Resolver-->>Ops: RefUri (contributorIdentity)
Ops->>Resolver: resolveContributionDetails(contributionDetails) / createContributionDetailsRecord()
Resolver-->>Ops: RefUri (contributionDetails)
Ops->>Ops: buildContributorEntries(refs..., weight)
Ops->>Store: attachContributorsToHypercert(hypercertUri, newContributorEntries)
Store-->>Ops: UpdateResult
Ops-->>Client: UpdateResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 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)
1377-1380:⚠️ Potential issue | 🟡 MinorTypo in comment: "contribtorInformation".
📝 Fix typo
- // we still store as contribtorInformation since it cant directly be a string + // we still store as contributorInformation since it can't directly be a string
🤖 Fix all issues with AI agents
In `@packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts`:
- Around line 957-980: The test "should update contributors array" uses
hypercertOps.update with an activity URI but the mocked getRecord response
returns a record with collection "org.hypercerts.claim.record"; update the mock
so the returned record's collection is "org.hypercerts.claim.activity" (either
by changing the beforeEach mock response or overriding the mock in this test) so
hypercertOps.update sees a matching activity record when asserting contributors;
change the mocked record's collection field where
mockAgent.com.atproto.repo.getRecord (or the beforeEach mock that supplies the
record) constructs the returned record.
🧹 Nitpick comments (2)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
1293-1328: Use nullish coalescing for$typeandcreatedAtto follow the established pattern.The
createContributionDetailsRecordsets$typeandcreatedAtexplicitly, then spreads...extraPropslast — which accidentally allows callers to override them via the index signature. Other helpers in this file (e.g.,createLocationRecordat line 2683-2684,buildAttachmentRecordat lines 1104-1105) use the nullish coalescing pattern$type ?? DEFAULTper project conventions.♻️ Suggested fix
private async createContributionDetailsRecord(params: { role: string; contributionDescription?: string; startDate?: string; endDate?: string; [key: string]: unknown; }): Promise<CreateResult> { const createdAt = new Date().toISOString(); - const { role, contributionDescription, startDate, endDate, ...extraProps } = params; + const { role, contributionDescription, startDate, endDate, $type, createdAt: callerCreatedAt, ...extraProps } = params; const contributionRecord: HypercertContributionDetails = { - $type: HYPERCERT_COLLECTIONS.CONTRIBUTION_DETAILS, + ...extraProps, + $type: ($type as string) ?? HYPERCERT_COLLECTIONS.CONTRIBUTION_DETAILS, + createdAt: (callerCreatedAt as string) ?? createdAt, role, - createdAt, contributionDescription, startDate, endDate, - ...extraProps, };Based on learnings, record creation helper methods should use nullish coalescing to preserve caller-supplied
$typeandcreatedAtfields when provided, defaulting only when missing.packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
4172-4286: Inconsistent approach for testing private vs. protected methods, and duplicate assertions.Two observations:
Inconsistency:
buildContributorEntriesandattachContributorsToHypercertare tested cleanly via theTestableHypercertOperationssubclass, butresolveToStrongReffalls back to@ts-expect-errorcasts. Consider exposing it via the sameTestableHypercertOperationssubclass for consistency.Duplicate
rejects.toThrowcalls: Several tests (e.g., lines 4220–4225, 4236–4239, 4255–4258, 4264–4272, 4277–4285) invoke the method twice — once to check the error type, once to check the message. Each call creates a new rejected promise and re-exercises the whole mock chain. Use a single assertion with a regex or string match instead:await expect(hypercertOps.resolveToStrongRef(invalidUri)) .rejects.toThrow(expect.objectContaining({ constructor: ValidationError, message: expect.stringContaining("Invalid AT-URI format"), }));Or chain
.rejectsto a variable:const rejection = expect(hypercertOps.resolveToStrongRef(invalidUri)).rejects; await rejection.toThrow(ValidationError); await rejection.toThrow("Invalid AT-URI format");This halves the mock invocations and avoids subtle ordering issues if mock state changes between calls.
Kzoeps
force-pushed
the
add-contribution-update-cert
branch
from
fc7d0f9 to
e957b7d
Compare
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 1377-1379: The inline comment in HypercertOperationsImpl.ts near
the identity handling branch has a misspelling ("contribtorInformation"); update
the comment to read "contributorInformation" (or better: "contributor
information") so it matches the actual symbol and intent around the
addContributorInformation call and stored data; ensure the comment is corrected
adjacent to the identity check and the await this.addContributorInformation({
identifier: identity }) line.
🧹 Nitpick comments (2)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (1)
1293-1310:...extraPropsspread can silently override$typeandcreatedAt.The destructuring at line 1301 only extracts named fields, so if a caller passes
$typeorcreatedAtvia the[key: string]: unknownindex signature, they'll end up inextraPropsand override the explicit defaults at lines 1303-1305 (since the spread is last). Use the nullish coalescing pattern for consistency with other record creation helpers.Suggested fix
- private async createContributionDetailsRecord(params: { - role: string; - contributionDescription?: string; - startDate?: string; - endDate?: string; - [key: string]: unknown; - }): Promise<CreateResult> { - const createdAt = new Date().toISOString(); - const { role, contributionDescription, startDate, endDate, ...extraProps } = params; - const contributionRecord: HypercertContributionDetails = { - $type: HYPERCERT_COLLECTIONS.CONTRIBUTION_DETAILS, - role, - createdAt, - contributionDescription, - startDate, - endDate, - ...extraProps, - }; + private async createContributionDetailsRecord(params: { + role: string; + contributionDescription?: string; + startDate?: string; + endDate?: string; + [key: string]: unknown; + }): Promise<CreateResult> { + const { role, contributionDescription, startDate, endDate, $type, createdAt, ...extraProps } = params; + const contributionRecord: HypercertContributionDetails = { + ...extraProps, + $type: ($type as string) ?? HYPERCERT_COLLECTIONS.CONTRIBUTION_DETAILS, + createdAt: (createdAt as string) ?? new Date().toISOString(), + role, + contributionDescription, + startDate, + endDate, + };Based on learnings: "record creation helper methods should use nullish coalescing to preserve caller-supplied
$typeandcreatedAtfields when provided, defaulting only when missing. Pattern:$type: $type ?? HYPERCERT_COLLECTIONS.X, createdAt: createdAt ?? new Date().toISOString()."packages/sdk-core/tests/repository/HypercertOperationsImpl.test.ts (1)
4172-4286: ExposeresolveToStrongRefviaTestableHypercertOperationsinstead of@ts-expect-error.The rest of this file uses the
TestableHypercertOperationssubclass to expose protected methods cleanly. These tests use repeated@ts-expect-errorcomments (lines 4179, 4201, 4219, 4222, etc.) to access a private method. Exposing it through the subclass would be more consistent and avoid the 12+ suppression comments.Additionally, several tests (e.g., lines 4220–4225, 4235–4240) call the method twice — once per assertion — doubling the mock invocations unnecessarily. Use a variable to capture the rejection instead:
♻️ Example consolidation
- // `@ts-expect-error` - accessing private method for testing - await expect(hypercertOps.resolveToStrongRef(invalidUri)).rejects.toThrow(ValidationError); - await expect( - // `@ts-expect-error` - accessing private method for testing - hypercertOps.resolveToStrongRef(invalidUri), - ).rejects.toThrow("Invalid AT-URI format"); + const promise = hypercertOps.testResolveToStrongRef(invalidUri); + await expect(promise).rejects.toThrow(ValidationError); + await expect(promise).rejects.toThrow("Invalid AT-URI format");
Kzoeps
force-pushed
the
add-contribution-update-cert
branch
from
e957b7d to
a9701cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1293-1310: The createContributionDetailsRecord function currently
always overwrites $type and createdAt; update it to accept optional $type and
createdAt in the CreateContributionDetailsParams type and use nullish coalescing
like buildAttachmentRecord: destructure params to include $type and createdAt,
compute finalCreatedAt = createdAt ?? new Date().toISOString(), and set
contributionRecord.$type = $type ?? HYPERCERT_COLLECTIONS.CONTRIBUTION_DETAILS
and contributionRecord.createdAt = finalCreatedAt while preserving
...extraProps.
In `@packages/sdk-core/src/repository/interfaces.ts`:
- Around line 385-425: UpdateHypercertParams is defined from HypercertClaim
which violates the repo convention; change UpdateHypercertParams to be
Partial<CreateHypercertParams> to follow the UpdateXxxParams =
Partial<CreateXxxParams> rule, and if you still need a type that mirrors the
record schema create a new distinct type (e.g., HypercertClaimUpdate or
UpdateHypercertRecord) defined as Partial<Omit<HypercertClaim, "$type" |
"createdAt" | "rights">> and use that where record-schema updates are required;
also apply the same pattern to the other similar types referenced in the review.
🧹 Nitpick comments (2)
packages/sdk-core/src/repository/HypercertOperationsImpl.ts (2)
1480-1498: Double-fetch of the hypercert record.
attachContributorsToHypercertcallsthis.get(hypercertUri)on line 1488 to read the existing contributors, thenthis.update(...)on line 1492 which internally fetches the same record again (line 649). Under concurrent modifications this also creates a wider TOCTOU window.Consider refactoring
update()to optionally accept pre-fetched record data, or extracting the merge+put logic to avoid the redundant round-trip.
1373-1380: Bare DID strings always create newcontributorInformationrecords—potential duplicates.When
identityis a string (e.g., a DID),resolveContributorIdentityunconditionally callsaddContributorInformation(line 1379), creating a new record each time. IfaddContributionis called multiple times for the same DID, this produces duplicate contributor records. Consider using a deterministicrkey(e.g., hashing the identifier) inaddContributorInformationto achieve idempotent upserts, consistent with howcreateHypercertRecorduses content-hashed rKeys.
2 participants
Summary
addContributionto properly update the hypercert'scontributorsarraycreate()method's contribution handlingBreaking Changes
hypercertUrinow requiredcontributorsnow acceptsContributorIdentityParams[](DIDs, StrongRefs, or inline params)contributionDetailsreplaces separaterole/descriptionparametersUpdateResultinstead ofCreateResultupdate()method now accepts actual record fields (UpdateHypercertParams)See changeset for migration examples.
Summary by CodeRabbit
Breaking Changes
Improvements
Tests