Add synced app-defined metadata for remote resources#75
Merged
Conversation
…teResource<TMetadata>. Apps opt in with ResourceService<TMetadata> and AddCrdtRemoteResources<TMetadata> so metadata participates in CRDT sync. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughRemote resource APIs, CRDT change types, service wiring, sync helpers, and tests are updated to use generic ChangesGeneric metadata remote resource pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SIL.Harmony/ResourceService.cs (1)
126-146:⚠️ Potential issue | 🟠 Major | ⚡ Quick winServer-returned metadata not persisted during pending upload completion.
When
UploadPendingResourcesprocesses pending uploads, theuploadResultmay contain server-authoredMetadata(similar toAddLocalResourceat line 80). However, onlyRemoteResourceUploadedChangeis created, which sets theRemoteIdbut doesn't persist any metadata returned by the server.If the server can return authoritative metadata during upload (e.g., computed hashes, timestamps), this would be lost for resources that were initially created as pending uploads.
Consider adding a
SetRemoteResourceMetadataChangewhenuploadResult.Metadatais not null:Proposed fix
foreach (var localResource in pendingUploads) { var uploadResult = await remoteResourceService.UploadResource(localResource.Id, localResource.LocalPath); changes.Add(new RemoteResourceUploadedChange<TMetadata>(localResource.Id, uploadResult.RemoteId)); + if (uploadResult.Metadata is not null) + { + changes.Add(new SetRemoteResourceMetadataChange<TMetadata>(localResource.Id, uploadResult.Metadata)); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony/ResourceService.cs` around lines 126 - 146, In the UploadPendingResources method, after creating the RemoteResourceUploadedChange for each uploaded resource, check if uploadResult.Metadata is not null. When metadata exists, also create a SetRemoteResourceMetadataChange with the server-returned metadata and add it to the changes list alongside the RemoteResourceUploadedChange. This ensures that any authoritative metadata returned by the server during upload is persisted in the same way as handled in the AddLocalResource method.
🧹 Nitpick comments (1)
src/SIL.Harmony/CrdtConfig.cs (1)
89-96: 💤 Low valueConsider using consistent JSON serialization options.
The
Metadataproperty conversion uses(JsonSerializerOptions?)null(default options), while the rest of the CRDT system uses configuredJsonSerializerOptionsfromCrdtConfig.JsonSerializerOptions. If custom type resolvers or converters are needed for metadata types, this could cause serialization mismatches.If this is intentional to keep metadata serialization simple and independent, consider adding a brief comment explaining the design choice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony/CrdtConfig.cs` around lines 89 - 96, The Metadata property conversion is using default JSON serialization options (null) for both JsonSerializer.Serialize and JsonSerializer.Deserialize calls, while the rest of the CRDT system uses configured options from CrdtConfig.JsonSerializerOptions. Either update both the Serialize and Deserialize conversions to use CrdtConfig.JsonSerializerOptions instead of null to maintain consistency, or if intentional, add a brief comment in the HasConversion block explaining why Metadata serialization is kept simple and independent from the rest of the CRDT configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs`:
- Around line 47-52: The _metadata dictionary is being written with normalized
keys using Path.GetFullPath(localPath) in the SetUploadMetadata method on line
52, but read with raw localPath in the TryGetValue call on line 47. This causes
relative and absolute path variants to have mismatched keys and fail lookups.
Fix this by normalizing the key in the TryGetValue call on line 47 to use
Path.GetFullPath(localPath) instead of raw localPath, ensuring consistency
between how metadata is stored in SetUploadMetadata and retrieved in the upload
result method.
In `@src/SIL.Harmony/Resource/RemoteResource.cs`:
- Around line 47-50: The CloneMetadata method lacks validation to ensure
TMetadata can be safely serialized and deserialized to JSON, which can cause
runtime crashes during copy operations when non-serializable types are used. Add
fail-fast validation in the CloneMetadata method to check whether the metadata
instance can be successfully serialized before attempting the JSON round-trip
serialization, and throw a clear, descriptive exception if serialization fails.
This ensures deterministic error handling and prevents silent failures during
sync flows.
In `@src/SIL.Harmony/ResourceService.cs`:
- Around line 157-164: The UploadPendingResource method does not persist server
metadata from the uploadResult when available. When creating the
RemoteResourceUploadedChange<TMetadata> object, you need to extract and include
the metadata from uploadResult (in addition to the localResource.Id and
uploadResult.RemoteId that are already being passed). Check the
RemoteResourceUploadedChange<TMetadata> constructor to understand what metadata
parameter it expects, then update the method to pass the appropriate metadata
from uploadResult to ensure server-provided metadata is properly persisted in
the change log.
---
Outside diff comments:
In `@src/SIL.Harmony/ResourceService.cs`:
- Around line 126-146: In the UploadPendingResources method, after creating the
RemoteResourceUploadedChange for each uploaded resource, check if
uploadResult.Metadata is not null. When metadata exists, also create a
SetRemoteResourceMetadataChange with the server-returned metadata and add it to
the changes list alongside the RemoteResourceUploadedChange. This ensures that
any authoritative metadata returned by the server during upload is persisted in
the same way as handled in the AddLocalResource method.
---
Nitpick comments:
In `@src/SIL.Harmony/CrdtConfig.cs`:
- Around line 89-96: The Metadata property conversion is using default JSON
serialization options (null) for both JsonSerializer.Serialize and
JsonSerializer.Deserialize calls, while the rest of the CRDT system uses
configured options from CrdtConfig.JsonSerializerOptions. Either update both the
Serialize and Deserialize conversions to use CrdtConfig.JsonSerializerOptions
instead of null to maintain consistency, or if intentional, add a brief comment
in the HasConversion block explaining why Metadata serialization is kept simple
and independent from the rest of the CRDT configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40f5d8c9-4321-4140-a9d9-35674e8f033c
📒 Files selected for processing (20)
src/SIL.Harmony.Core/IRemoteResourceService.cssrc/SIL.Harmony.Sample/CrdtSampleKernel.cssrc/SIL.Harmony.Sample/MediaMetadata.cssrc/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txtsrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesMetadataTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cssrc/SIL.Harmony.Tests/ResourceTests/WordResourceTests.cssrc/SIL.Harmony/CrdtConfig.cssrc/SIL.Harmony/CrdtKernel.cssrc/SIL.Harmony/Resource/CreateRemoteResourceChange.cssrc/SIL.Harmony/Resource/CreateRemoteResourcePendingUpload.cssrc/SIL.Harmony/Resource/DeleteRemoteResourceChange.cssrc/SIL.Harmony/Resource/HarmonyResource.cssrc/SIL.Harmony/Resource/RemoteResource.cssrc/SIL.Harmony/Resource/RemoteResourceNotEnabledException.cssrc/SIL.Harmony/Resource/RemoteResourceUploadedChange.cssrc/SIL.Harmony/Resource/SetRemoteResourceMetadataChange.cssrc/SIL.Harmony/ResourceService.cssrc/SIL.Harmony/SyncHelper.cs
…adata into UploadResource (allows merging etc), ensure metadata is set when uploading a pending resource.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs (1)
47-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize metadata keys consistently to avoid lookup misses.
SetUploadMetadata(Line 52) now stores with the rawlocalPathpassed by tests, whileUploadResourceis invoked byResourceService.AddLocalResource/UploadPendingResourceswithPath.GetFullPath(...)(seeResourceService.cs). If a test seeds metadata via a relative path, the Line 47 lookup will miss and the upload-override behavior will silently be skipped. This works today only because tests happen to pass absolute paths. Normalizing both the write and read withPath.GetFullPathremoves the dependence on caller path form.This is the same root cause as the previous review note, just in the reversed direction.
Proposed fix
- _metadata.TryGetValue(localPath, out var mockMetadata); + _metadata.TryGetValue(Path.GetFullPath(localPath), out var mockMetadata); return new UploadResult<MediaMetadata>(remoteId, mockMetadata ?? metadata); } public void SetUploadMetadata(string localPath, MediaMetadata metadata) => - _metadata[localPath] = metadata; + _metadata[Path.GetFullPath(localPath)] = metadata;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs` around lines 47 - 52, Normalize metadata keys consistently in RemoteServiceMock so seeded upload metadata is found regardless of whether tests pass relative or absolute paths. Update both UploadResource and SetUploadMetadata to use the same normalized key, ideally by calling Path.GetFullPath on the localPath before storing and looking up in the _metadata dictionary. Keep the behavior aligned with ResourceService.AddLocalResource/UploadPendingResources so the mock matches the real caller path form.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/SIL.Harmony/Resource/HarmonyResource.cs`:
- Around line 14-20: The HarmonyResource constructor currently merges
localResource and remoteResource without verifying they belong to the same
entity, so add a validation step before assigning Id, RemoteId, LocalPath, and
Metadata. In HarmonyResource(LocalResource? localResource,
RemoteResource<TMetadata>? remoteResource), when both inputs are present,
compare their IDs and throw if they do not match; otherwise continue
constructing the composite resource. Keep the fix localized to the
HarmonyResource constructor so mismatched local/remote pairs are rejected early.
In `@src/SIL.Harmony/Resource/RemoteResourceUploadedChange.cs`:
- Around line 17-20: The upload change is overwriting existing metadata with
null when the remote service returns no override. Update
RemoteResourceUploadedChange.ApplyChange so it always applies RemoteId, but only
assigns entity.Metadata when the change actually contains server-authored
metadata; otherwise leave the current metadata untouched. Use the existing
RemoteResourceUploadedChange and ApplyChange flow to preserve prior values
coming from UploadPendingResources.
---
Duplicate comments:
In `@src/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cs`:
- Around line 47-52: Normalize metadata keys consistently in RemoteServiceMock
so seeded upload metadata is found regardless of whether tests pass relative or
absolute paths. Update both UploadResource and SetUploadMetadata to use the same
normalized key, ideally by calling Path.GetFullPath on the localPath before
storing and looking up in the _metadata dictionary. Keep the behavior aligned
with ResourceService.AddLocalResource/UploadPendingResources so the mock matches
the real caller path form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cfe7711-8ca2-473b-81ee-f1a51cbebf9b
📒 Files selected for processing (7)
src/SIL.Harmony.Core/IRemoteResourceService.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesMetadataTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteServiceMock.cssrc/SIL.Harmony/Resource/HarmonyResource.cssrc/SIL.Harmony/Resource/RemoteResourceUploadedChange.cssrc/SIL.Harmony/ResourceService.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/SIL.Harmony.Core/IRemoteResourceService.cs
- src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesMetadataTests.cs
- src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs
- src/SIL.Harmony/ResourceService.cs
myieye
reviewed
Jun 29, 2026
myieye
left a comment
myieye
left a comment
Contributor
There was a problem hiding this comment.
I'm having trouble figuring out if Devin AI's feedback is helpful.
So, I'm gonna be lazy and just point you at it 🙈
https://app.devin.ai/review/sillsdev/harmony/pull/75
Add an optional CommitMetadata? parameter to every commit-creating ResourceService method (including SetResourceMetadata) and forward it to DataModel.AddChange/AddChanges, so resource/media commits can carry author info like every other change. Backward-compatible: omitting it behaves as before. Stacked on #75 (cursor/remote-resource-metadata), which reworks ResourceService into the generic ResourceService<TMetadata>. The resource TMetadata (app-defined, synced) and the commit CommitMetadata (author) are orthogonal; #75 still authored resource commits without CommitMetadata. Closes #76 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JH8DKhRUSnxnGsLRaavy6D
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…nto cursor/remote-resource-metadata
Collaborator
Author
|
@myieye that devin review had some helpful stuff. Also I didn't realize it at first but the commit made by claude above isn't actually in this PR, it just referenced it, so I merged it in. |
Contributor
|
@hahn-kev Thanks for adding the Claude commit. I started that commit before this PR existed and just rebased it yesterday. I was planning it for a follow up PR, but folding it in is fine with me. I've noted that this PR will resolve the relevant issue. |
myieye
approved these changes
Jul 1, 2026
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
3 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.
Resolves #76 (though that is not the primary goal of this PR)
Summary
RemoteResource<TMetadata>with syncedMetadataso clients can list media without downloading blobsResourceService<TMetadata>,HarmonyResource<TMetadata>, andAddCrdtRemoteResources<TMetadata>()DI registrationSetRemoteResourceMetadataChange<TMetadata>,DeleteRemoteResourceChange<TMetadata>(stabledelete:RemoteResourcetype name)IRemoteResourceService<TMetadata>generic;UploadResult<TMetadata>can return server-authored metadata that overrides client-passed metadata on uploadNoMetadatamarker type for apps that do not need synced metadata fieldsMediaMetadatarecord in SIL.Harmony.SampleBreaking changes
RemoteResource,ResourceService,HarmonyResource, andIRemoteResourceServiceare now generic-onlyCrdtConfig.AddRemoteResourceEntity()(non-generic) removed — useAddCrdtRemoteResources<TMetadata>()DeleteChange<RemoteResource>replaced byDeleteRemoteResourceChange<TMetadata>SyncWithResourceUploadnow requires<TMetadata>type argumentSummary by CodeRabbit
New Features
Bug Fixes