feat: attach content metadata to content blobs#69
Merged
flyingrobots merged 4 commits intomainfrom Mar 14, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional MIME and byte-size metadata to content attachments, persists metadata as sibling properties, exposes structured reads via getContentMeta/getEdgeContentMeta, and updates Patch/Session/Type declarations, query logic, blob adapter JSDoc, docs, and tests to support metadata-aware flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PatchBuilder
participant BlobStore
participant WarpGraph
rect rgba(200,230,255,0.5)
Client->>PatchBuilder: attachContent(nodeId, content, {mime,size})
PatchBuilder->>BlobStore: store(content, { slug?, mime, size })
BlobStore-->>PatchBuilder: oid
PatchBuilder->>WarpGraph: emit PropSet('_content'=oid)
PatchBuilder->>WarpGraph: emit PropSet('_content.size'=size)
PatchBuilder->>WarpGraph: emit PropSet('_content.mime'=mime)
end
rect rgba(200,255,200,0.5)
Client->>WarpGraph: getContentMeta(nodeId)
WarpGraph->>WarpGraph: read registers (_content, _content.size, _content.mime)
WarpGraph-->>Client: { oid, mime, size } | null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/infrastructure/adapters/CasBlobAdapter.js (1)
99-121: Note:mimeandsizeoptions are accepted but not used in CAS storage.The JSDoc now declares
mimeandsizein the options type, andPatchBuilderV2passes these values (seesrc/domain/services/PatchBuilderV2.jslines 607, 637). However, the implementation constructsstoreOptswithout them — they are silently dropped.This appears intentional since metadata is persisted separately via CRDT properties (
_content.mime,_content.size), but the mismatch between the documented interface and actual behavior may confuse future maintainers. Consider either:
- Adding a brief comment explaining why mime/size are accepted but unused, or
- Removing them from the JSDoc until CAS-level metadata storage is implemented
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infrastructure/adapters/CasBlobAdapter.js` around lines 99 - 121, The JSDoc for async store(content, options) lists mime and size but the implementation in store() (specifically the storeOpts object) doesn't pass them to cas.store; add a short inline comment above the storeOpts construction or update the JSDoc to clarify intent: state that mime and size are accepted for higher-level CRDT metadata (see PatchBuilderV2) and are intentionally not forwarded to CAS because CAS metadata is not used here, or alternatively remove mime/size from the JSDoc; reference the store() method, the storeOpts variable and PatchBuilderV2 usage when making the change.test/type-check/consumer.ts (1)
269-273: Strengthen this type-surface test by importing the new public metadata types directly.Right now the test uses inline object shapes instead of the named API types. Once
ContentMeta/ContentAttachmentOptionsare exported, using them here will catch export regressions earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/type-check/consumer.ts` around lines 269 - 273, Replace the inline object type shapes in the test with the public exported types: import ContentMeta and ContentAttachmentOptions (or the exact exported names) at the top of the test and use them for the variables returned by getContentMeta and getEdgeContentMeta; update contentMeta and edgeContentMeta declarations to use ContentMeta | null, and if any option shapes are used elsewhere switch those to ContentAttachmentOptions to ensure export regressions are caught (reference symbols: ContentMeta, ContentAttachmentOptions, getContentMeta, getEdgeContentMeta).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.d.ts`:
- Around line 1419-1428: The interfaces ContentAttachmentOptions and ContentMeta
are declared but not exported, yet they're referenced in public signatures
(e.g., PatchBuilderV2 and WarpGraph), so export them from the type definitions;
update the declarations for ContentAttachmentOptions and ContentMeta to be
exported (e.g., export interface ContentAttachmentOptions and export interface
ContentMeta) so consumers can import and use these types in their code and IDEs
can resolve the types referenced by PatchBuilderV2 and WarpGraph.
In `@ROADMAP.md`:
- Around line 398-399: The inventory summary is inconsistent: the "Standalone
(done)" list shows 62 items but the summary text still reads "61 standalone
done"; update the inventory totals so they match by changing the summary text
that currently says "61 standalone done" to "62 standalone done" (look for the
"Standalone (done)" label and the overall inventory summary line that mentions
"61 standalone done" and make them consistent).
- Line 506: The document contains conflicting backlog totals: one header states
"32 active items sorted into priority tiers" while a later sentence says "25
standalone items"; update the text so both locations use the same reconciled
total (choose the correct canonical number for the active backlog), and ensure
any adjacent phrasing (e.g., "standalone items", "active items", "priority
tiers", and "execution waves") is consistent with that chosen number; search for
the exact phrases "32 active items sorted into priority tiers" and "25
standalone items" to locate and update all occurrences including the header and
the Wave summary.
---
Nitpick comments:
In `@src/infrastructure/adapters/CasBlobAdapter.js`:
- Around line 99-121: The JSDoc for async store(content, options) lists mime and
size but the implementation in store() (specifically the storeOpts object)
doesn't pass them to cas.store; add a short inline comment above the storeOpts
construction or update the JSDoc to clarify intent: state that mime and size are
accepted for higher-level CRDT metadata (see PatchBuilderV2) and are
intentionally not forwarded to CAS because CAS metadata is not used here, or
alternatively remove mime/size from the JSDoc; reference the store() method, the
storeOpts variable and PatchBuilderV2 usage when making the change.
In `@test/type-check/consumer.ts`:
- Around line 269-273: Replace the inline object type shapes in the test with
the public exported types: import ContentMeta and ContentAttachmentOptions (or
the exact exported names) at the top of the test and use them for the variables
returned by getContentMeta and getEdgeContentMeta; update contentMeta and
edgeContentMeta declarations to use ContentMeta | null, and if any option shapes
are used elsewhere switch those to ContentAttachmentOptions to ensure export
regressions are caught (reference symbols: ContentMeta,
ContentAttachmentOptions, getContentMeta, getEdgeContentMeta).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f1e82df-d1fc-4c2e-aa51-178c8169d5fb
⛔ Files ignored due to path filters (1)
test/unit/domain/__snapshots__/WarpGraph.apiSurface.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
CHANGELOG.mdREADME.mdROADMAP.mddocs/ROADMAP/COMPLETED.mddocs/specs/CONTENT_ATTACHMENT.mdindex.d.tssrc/domain/services/KeyCodec.jssrc/domain/services/PatchBuilderV2.jssrc/domain/warp/PatchSession.jssrc/domain/warp/_wiredMethods.d.tssrc/domain/warp/query.methods.jssrc/infrastructure/adapters/CasBlobAdapter.jssrc/ports/BlobStoragePort.jstest/integration/api/content-attachment.test.jstest/type-check/consumer.tstest/unit/domain/WarpGraph.content.test.jstest/unit/domain/services/PatchBuilderV2.content.test.js
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ROADMAP.md (1)
506-506:⚠️ Potential issue | 🟡 MinorBacklog total still conflicts with earlier summary text.
Line 506 says 25 active standalone items, but Line 176 still says 32 active items. Please reconcile to a single canonical value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 506, The file contains conflicting backlog counts: one sentence reads "The active backlog is **25 standalone items**" while another reads "**32** active items"; pick the correct canonical active-items value and update the other occurrence so both statements match exactly (search for the phrases "The active backlog is **25 standalone items**" and "**32** active items**" and replace the incorrect one). Ensure any related summary or parenthetical counts in the same document are updated too so the total is consistent across the ROADMAP.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/warp/query.methods.js`:
- Around line 368-375: The isSameAttachmentLineage function currently compares
lamport, writerId, and patchSha but omits opIndex, allowing earlier ops in the
same patch to be treated as current; update the function
(isSameAttachmentLineage) to also require contentEventId.opIndex ===
candidateEventId.opIndex (i.e., include an && contentEventId.opIndex ===
candidateEventId.opIndex check alongside the existing lamport/writerId/patchSha
checks) so that operation-order within the same patch is considered when
matching attachment lineage.
---
Duplicate comments:
In `@ROADMAP.md`:
- Line 506: The file contains conflicting backlog counts: one sentence reads
"The active backlog is **25 standalone items**" while another reads "**32**
active items"; pick the correct canonical active-items value and update the
other occurrence so both statements match exactly (search for the phrases "The
active backlog is **25 standalone items**" and "**32** active items**" and
replace the incorrect one). Ensure any related summary or parenthetical counts
in the same document are updated too so the total is consistent across the
ROADMAP.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa4b72f3-6e3c-4ffe-a149-fcaf7bbef5f1
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdROADMAP.mddocs/specs/CONTENT_ATTACHMENT.mdsrc/domain/services/KeyCodec.jssrc/domain/warp/query.methods.jstest/integration/api/content-attachment.test.jstest/unit/domain/WarpGraph.content.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/specs/CONTENT_ATTACHMENT.md
- src/domain/services/KeyCodec.js
- CHANGELOG.md
- test/unit/domain/WarpGraph.content.test.js
Comment on lines
+368
to
+375
| function isSameAttachmentLineage(contentEventId, candidateEventId) { | ||
| return Boolean( | ||
| contentEventId | ||
| && candidateEventId | ||
| && contentEventId.lamport === candidateEventId.lamport | ||
| && contentEventId.writerId === candidateEventId.writerId | ||
| && contentEventId.patchSha === candidateEventId.patchSha | ||
| ); |
There was a problem hiding this comment.
Include opIndex in attachment-lineage matching.
Current lineage matching ignores operation order within the same patch. If _content is rewritten later in that patch, older _content.mime / _content.size from earlier ops in the same patch can be incorrectly treated as current metadata.
Suggested fix
function isSameAttachmentLineage(contentEventId, candidateEventId) {
return Boolean(
contentEventId
&& candidateEventId
&& contentEventId.lamport === candidateEventId.lamport
&& contentEventId.writerId === candidateEventId.writerId
&& contentEventId.patchSha === candidateEventId.patchSha
+ && candidateEventId.opIndex > contentEventId.opIndex
);
}📝 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.
Suggested change
| function isSameAttachmentLineage(contentEventId, candidateEventId) { | |
| return Boolean( | |
| contentEventId | |
| && candidateEventId | |
| && contentEventId.lamport === candidateEventId.lamport | |
| && contentEventId.writerId === candidateEventId.writerId | |
| && contentEventId.patchSha === candidateEventId.patchSha | |
| ); | |
| function isSameAttachmentLineage(contentEventId, candidateEventId) { | |
| return Boolean( | |
| contentEventId | |
| && candidateEventId | |
| && contentEventId.lamport === candidateEventId.lamport | |
| && contentEventId.writerId === candidateEventId.writerId | |
| && contentEventId.patchSha === candidateEventId.patchSha | |
| && candidateEventId.opIndex > contentEventId.opIndex | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/warp/query.methods.js` around lines 368 - 375, The
isSameAttachmentLineage function currently compares lamport, writerId, and
patchSha but omits opIndex, allowing earlier ops in the same patch to be treated
as current; update the function (isSameAttachmentLineage) to also require
contentEventId.opIndex === candidateEventId.opIndex (i.e., include an &&
contentEventId.opIndex === candidateEventId.opIndex check alongside the existing
lamport/writerId/patchSha checks) so that operation-order within the same patch
is considered when matching attachment lineage.
Release Preflight
If you tag this commit as |
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 `@contracts/type-surface.m8.json`:
- Around line 1316-1321: The manifest added types ContentAttachmentOptions and
ContentMeta but left API signatures unchanged; update the exported type surface
so PatchBuilderV2.attachContent and PatchBuilderV2.attachEdgeContent, and
PatchSession.attachContent and PatchSession.attachEdgeContent include the
optional metadata parameter (use ContentAttachmentOptions or ContentMeta as
appropriate), and add WarpGraph.getContentMeta and WarpGraph.getEdgeContentMeta
signatures returning ContentMeta; ensure the exports list and function/method
signatures reference those new types to keep the contract consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91f7eeb8-1980-4983-a886-3c0089ba98c7
📒 Files selected for processing (6)
CHANGELOG.mdROADMAP.mdcontracts/type-surface.m8.jsonindex.d.tssrc/infrastructure/adapters/CasBlobAdapter.jstest/type-check/consumer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- index.d.ts
- test/type-check/consumer.ts
- CHANGELOG.md
- src/infrastructure/adapters/CasBlobAdapter.js
Release Preflight
If you tag this commit as |
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
1 participant
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.
Summary
attachContent()andattachEdgeContent()getContentMeta()andgetEdgeContentMeta()for structured readsCloses #45
Test Plan
Summary by CodeRabbit
New Features
Documentation
Tests