refactor(sdk-core): extract URI parsing/record helpers, fix blob upload and scope validation#126
Merged
aspiers merged 8 commits intohypercerts-org:developfrom Feb 18, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 5eda6ee 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughCentralizes AT-URI parsing and record fetch/save into exported utilities (AT_URI_REGEX, parseAtUri, fetchRecord, saveRecord), refactors HypercertOperationsImpl to use them (removing redundant fetches), changes BlobOperationsImpl.upload to return an AT Protocol BlobRef, and tightens permission-scope regex; tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HypercertOps as HypercertOperationsImpl
participant ATProto
participant SDS as Storage
Client->>HypercertOps: request using at://... URI (get/update/attach)
HypercertOps->>HypercertOps: parseAtUri / parseUri (AT_URI_REGEX)
HypercertOps->>ATProto: getRecord (fetchRecord)
ATProto-->>HypercertOps: record + cid
HypercertOps->>HypercertOps: apply updates in-memory
HypercertOps->>ATProto: putRecord (saveRecord)
ATProto-->>HypercertOps: result { uri, cid }
HypercertOps-->>Client: return result
alt Blob upload via SDS
Client->>HypercertOps: upload(blob)
HypercertOps->>SDS: upload request
SDS-->>HypercertOps: blob response (ref link, mimeType, size)
HypercertOps->>HypercertOps: CID.parse -> new BlobRef(...)
HypercertOps-->>Client: BlobRef
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Kzoeps
force-pushed
the
location-refactor
branch
from
a4f97a6 to
9358d16
Compare
Kzoeps
force-pushed
the
profile
branch
9 times, most recently
from
5fa030d to
1e5e282
Compare
aspiers
force-pushed
the
location-refactor
branch
from
05c34b8 to
111ca8e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
266-271: 🛠️ Refactor suggestion | 🟠 MajorRemove
parseAndValidateUrimethod — it is dead code.No callers remain after migration to
parseUri. The method performs redundant validation before parsing, whileparseUri(active at lines 167, 852, 1043, 1795, 2428) serves the same purpose more efficiently.🤖 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 266 - 271, Remove the dead method parseAndValidateUri from class HypercertOperationsImpl: delete the private parseAndValidateUri(uri: string): AtUriComponents { ... } implementation and any leftover references or imports solely used by it, and ensure all callers use the existing parseUri (the active function referenced at lines near usages) instead; verify there are no remaining references to parseAndValidateUri and run tests/type-checks to confirm clean removal.
🧹 Nitpick comments (4)
.changeset/refactor-uri-parsing-utilities.md (1)
2-2:patchundersells a public API breaking change — considerminorwith explicit breakage note.The
upload()return type changes from a plain{ ref: { $link: string }, mimeType, size }object to aBlobRefclass instance whererefis a CID object (.toString()) rather than a{ $link: string }plain object. Any consumer that accesseduploadResult.ref.$linkwill silently break at runtime. For a0.xpre-release package, this warrants at least aminorbump with an explicit compatibility note per this project's changeset conventions.♻️ Suggested update
-"@hypercerts-org/sdk-core": patch +"@hypercerts-org/sdk-core": minorAnd add to the bullet list:
+- **BREAKING**: `BlobOperationsImpl.upload()` now returns a `BlobRef` class instance; the `ref` field is a CID object (use `.ref.toString()` instead of `.ref.$link`)Based on learnings, for pre-1.0 libraries (0.x), minor version bumps can introduce breaking changes and should clearly document any API changes, removal or behavioral changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/refactor-uri-parsing-utilities.md at line 2, Update the changeset from "patch" to "minor" and add an explicit breaking-change note describing the upload() return-type change: explain that upload() now returns a BlobRef instance where ref is a CID object (string via .toString()) instead of a plain { $link: string } object, and that any consumer accessing uploadResult.ref.$link will break; include a clear migration hint (e.g., use uploadResult.ref.toString() or unwrap BlobRef) and add this bullet to the list of changes in the changeset file so release tooling and consumers are aware of the API break.packages/sdk-core/src/repository/HypercertOperationsImpl.ts (2)
1042-1050: Variablerecordshadows the concept of "record data" — rename tofetchResultfor clarity.
const record = await this.fetchRecord(uri)holds a{ uri, cid, record, collection, rkey }result object. Accessingrecord.cidon line 1045/1049 is the result object'scid, not the record field's cid. The namerecordcollides with the innerrecordfield of the same object, making the code confusing to reason about.♻️ Proposed rename
- const record = await this.fetchRecord(uri); - if (!record.cid) { - throw new NetworkError(`Record missing CID for repo=${did}, collection=${collection}, rkey=${rkey}`); - } - return { $type: "com.atproto.repo.strongRef" as const, uri, cid: record.cid }; + const fetchResult = await this.fetchRecord(uri); + if (!fetchResult.cid) { + throw new NetworkError(`Record missing CID for repo=${did}, collection=${collection}, rkey=${rkey}`); + } + return { $type: "com.atproto.repo.strongRef" as const, uri, cid: fetchResult.cid };🤖 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 1042 - 1050, In resolveStrongRefFromUri, rename the local variable returned by this.fetchRecord(uri) from record to fetchResult to avoid shadowing the inner record field; update the subsequent references (the existence check and the returned cid) to use fetchResult.cid and return { $type: ..., uri, cid: fetchResult.cid }; keep parseUri(uri) and the NetworkError throw logic unchanged aside from using fetchResult in the checks.
2262-2274:updateProjectfetches the record twice — once to verify the type and once insideupdateCollection.
fetchRecord<HypercertCollection>(uri)at line 2264 incurs a network round-trip solely to checkrecord.type !== "project". ThenupdateCollection(line 2271) callsfetchRecordagain at line 2343 for the actual update. This is two round-trips for what could be one.Consider passing the pre-fetched record into
updateCollection, or performing the type check insideupdateCollectionvia the already-fetched data:♻️ Suggested approach — pass pre-fetched record
Refactor
updateCollectionto accept an optional existing record to skip the second fetch, or extract a private_updateCollectionWithRecord(existingRecord, uri, updates)helper that both paths can call after a single fetch.🤖 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 2262 - 2274, updateProject currently calls fetchRecord(uri) to validate the type and then calls updateCollection(uri, updates) which fetches the same record again; refactor so the pre-fetched record is reused: modify updateCollection to accept an optional existing record (e.g., updateCollection(uri, updates, existingRecord?: HypercertCollection)) or add a private helper like _updateCollectionWithRecord(existingRecord, uri, updates), change updateProject to pass the fetched record to that new parameter/helper, and ensure the type check remains (use record.type === "project") and that only one fetchRecord call occurs per update flow.packages/sdk-core/tests/auth/permissions.test.ts (1)
1087-1102: Test name is misleading — most entries in the array do have colon separators.Items 3–6 in the
queryParamScopesarray ("repo:...","identity:*","identity:*?","include:...") all contain colons, so "without colon separator" is incorrect for the overall test. Additionally,"identity:*?"is a valid prefix match but not a well-defined scope per the ATProto permission spec; it's worth checking whether such a value should actually be considered valid.♻️ Suggested rename
- it("should validate query-param style permissions without colon separator", () => { + it("should validate mixed-format permissions including query-param style", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-core/tests/auth/permissions.test.ts` around lines 1087 - 1102, Rename the test "should validate query-param style permissions without colon separator" and/or update its inputs so the name matches intent: either change the test name to reflect that most entries contain colon separators (e.g., "should validate various query-param and colon-separated permissions") or replace/remove the colon-containing entries in the queryParamScopes array; also review the `"identity:*?"` entry and decide whether `validateScope` should treat that wildcard-with-question-mark as valid per ATProto spec—if not, update the test expectation or the input accordingly; locate the test and the array by the test description string and the call to validateScope to apply the change.
🤖 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/lexicons/utils.ts`:
- Line 43: The AT_URI_REGEX currently uses (.+) for the rkey which allows
slashes and is inconsistent with parseAtUri and isValidAtUri; update the regex
AT_URI_REGEX to use ([^/]+) for the rkey so it disallows slashes and matches the
same validation logic used in parseAtUri/isValidAtUri, ensuring callers get
consistent results between AT_URI_REGEX.test, isValidAtUri, and parseAtUri.
In `@packages/sdk-core/src/repository/BlobOperationsImpl.ts`:
- Around line 171-175: The SDS path in BlobOperationsImpl currently casts
response.json() to a BlobRef but returns a plain object which lacks BlobRef
methods needed by the lexicon serializer; replace the raw return of result.blob
with a proper deserialization using BlobRef.fromJsonRef(result.blob) so callers
like putRecord/createRecord receive a real BlobRef instance with .json()/.ipld()
methods.
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 206-208: The thrown NetworkError is mis-constructed: remove the
stray trailing colon and embed the collection:rkey location in the error
message, and pass an actual Error/unknown as the cause instead of a plain
string; locate the throw inside HypercertOperationsImpl where result.success is
checked and change the throw to include a clear message like "Failed to save
record <collection>:<rkey>" and supply a proper cause (e.g., new Error or the
original error) as the second argument to NetworkError rather than the raw
string `${collection}:${rkey}`.
- Line 181: Replace the silent fallback that sets cid: result.data.cid ?? ""
with an explicit error path: detect when result.data.cid is null/undefined and
throw a descriptive error (e.g., "Missing CID in response") instead of returning
an empty string so callers like resolveStrongRefFromUri and StrongRef
construction cannot produce a cid: "" value; locate the assignment of cid in
HypercertOperationsImpl (the result.data.cid usage) and change it to validate
presence and throw if absent, ensuring downstream callers receive only valid
CIDs.
---
Outside diff comments:
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 266-271: Remove the dead method parseAndValidateUri from class
HypercertOperationsImpl: delete the private parseAndValidateUri(uri: string):
AtUriComponents { ... } implementation and any leftover references or imports
solely used by it, and ensure all callers use the existing parseUri (the active
function referenced at lines near usages) instead; verify there are no remaining
references to parseAndValidateUri and run tests/type-checks to confirm clean
removal.
---
Nitpick comments:
In @.changeset/refactor-uri-parsing-utilities.md:
- Line 2: Update the changeset from "patch" to "minor" and add an explicit
breaking-change note describing the upload() return-type change: explain that
upload() now returns a BlobRef instance where ref is a CID object (string via
.toString()) instead of a plain { $link: string } object, and that any consumer
accessing uploadResult.ref.$link will break; include a clear migration hint
(e.g., use uploadResult.ref.toString() or unwrap BlobRef) and add this bullet to
the list of changes in the changeset file so release tooling and consumers are
aware of the API break.
In `@packages/sdk-core/src/repository/HypercertOperationsImpl.ts`:
- Around line 1042-1050: In resolveStrongRefFromUri, rename the local variable
returned by this.fetchRecord(uri) from record to fetchResult to avoid shadowing
the inner record field; update the subsequent references (the existence check
and the returned cid) to use fetchResult.cid and return { $type: ..., uri, cid:
fetchResult.cid }; keep parseUri(uri) and the NetworkError throw logic unchanged
aside from using fetchResult in the checks.
- Around line 2262-2274: updateProject currently calls fetchRecord(uri) to
validate the type and then calls updateCollection(uri, updates) which fetches
the same record again; refactor so the pre-fetched record is reused: modify
updateCollection to accept an optional existing record (e.g.,
updateCollection(uri, updates, existingRecord?: HypercertCollection)) or add a
private helper like _updateCollectionWithRecord(existingRecord, uri, updates),
change updateProject to pass the fetched record to that new parameter/helper,
and ensure the type check remains (use record.type === "project") and that only
one fetchRecord call occurs per update flow.
In `@packages/sdk-core/tests/auth/permissions.test.ts`:
- Around line 1087-1102: Rename the test "should validate query-param style
permissions without colon separator" and/or update its inputs so the name
matches intent: either change the test name to reflect that most entries contain
colon separators (e.g., "should validate various query-param and colon-separated
permissions") or replace/remove the colon-containing entries in the
queryParamScopes array; also review the `"identity:*?"` entry and decide whether
`validateScope` should treat that wildcard-with-question-mark as valid per
ATProto spec—if not, update the test expectation or the input accordingly;
locate the test and the array by the test description string and the call to
validateScope to apply the change.
- Fix SDS blob upload to return proper BlobRef instance using BlobRef.asBlobRef - Fix AT_URI_REGEX rkey capture group (.+ → [^/]+) to prevent over-matching - Fix fetchRecord to throw NetworkError when CID is absent - Fix saveRecord error message (two-arg → template literal) - Remove dead parseAndValidateUri method and unused imports - Eliminate double-fetch in updateProject via new updateCollectionRecord helper - Rename record → fetchResult in resolveStrongRefFromUri for clarity - Rename misleading test description in permissions.test.ts - Update changeset to minor with breaking change note for BlobRef return type
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/repository/BlobOperationsImpl.test.ts`:
- Around line 119-123: The test is constructing a BlobRef via BlobRef.asBlobRef
but drops the SDS-provided size (result.blob.size), causing size to default to
-1; update the construction to include the actual size from the SDS
response—either pass the typed blob shape { $type: 'blob', ref: result.ref,
mimeType: result.mimeType, size: result.blob.size } to BlobRef.asBlobRef or
instantiate the BlobRef with the size parameter so the resulting BlobRef
(created in the test/path using BlobRef.asBlobRef) has a positive non-zero size
instead of -1.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
AT_URI_REGEXas a constant for reuse, and fix the rkey capture group (.+→[^/]+) to prevent over-matching across path segmentsfetchRecord<T>()andsaveRecord()internal helpers to reduce repeated code;fetchRecordnow throwsNetworkErrorwhen the CID is absent rather than silently defaulting to an empty stringsaveRecorderror construction (previously passed two string args toNetworkErrorinstead of a message + cause)repo.blobs.upload()and return a properBlobRefinstance instead of manually constructing a blob-like object — this is a breaking change: the return type ofupload()is nowBlobRef(from@atproto/lexicon). SDS uploads construct theBlobRefviaCID.parseso thatref,mimeType, andsizeare all correctly populated from the server responseparseAndValidateUriprivate method and its unused imports (isValidAtUri,AtUriComponents)fetchRecordcall inupdateProjectby extracting a privateupdateCollectionRecordhelper that accepts a pre-fetched recordpermissions.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Refactor
Tests