Comments#2382
Merged
Merged
Conversation
Introduce CRDT-only comment threads, comments, local read-status tracking, JS interop, and focused tests so the viewer can build comment UI on top of the MiniLcm API. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Use a bottom sheet on mobile, a right-side overlay on desktop, and an inline sidebar on extra-large screens while loading comments only when opened. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
the
💻 FW Lite
Track unread comments directly so local read-state cleanup removes rows instead of growing a seen-comments table forever. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose unread comment lookup by subject so clients can fetch local unread state without first enumerating threads. Co-authored-by: Cursor <cursoragent@cursor.com>
…sense number. Set previous comment id, and use our guid helper instead of crypto random
Allow comment thread queries to optionally include their comments so the editor can load a subject's discussion in one API call. Co-authored-by: Cursor <cursoragent@cursor.com>
Let the comment dialog rely on runed resource state for on-demand thread loading and refresh after comment mutations. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep comments focused on the entry toolbar and align the aside panel beside the combined preview/editor layout. Co-authored-by: Cursor <cursoragent@cursor.com>
…oved readability and scrolling behavior.
… consistent styling, improved structure, and better button integration.
myieye
reviewed
Jun 29, 2026
myieye
reviewed
Jun 29, 2026
myieye
left a comment
myieye
left a comment
Collaborator
There was a problem hiding this comment.
This generally looks fine to me.
Storing "unread" locally has its weaknesses. I'm not totally convinced that it's the right call.
As I commented, I think there are screenshots to delete. Also, one of them shows a comments "dialog". I'm guessing that's out of date, because I saw no such dialog in your demo today. But there's still a CommentDialog component. Perhaps it needs renaming?
I didn't look at the UI code very much.
Co-authored-by: Tim Haasdyk <tim_haasdyk@sil.org>
Collaborator
Author
|
Yeah I agree, keeping the read status local only might be a problem, we can always change it later. The problem is that we run into a huge fan out if we track reads as CRDTs. Say there's 100 comments, and 5 users, that will generate 500 changes to mark comments as read. The approach that discord/slack use is to just track the last seen comment by a user, but that won't work for us since the last seen comment might come after an unread comment. And at the end of the day, even if we come up with a smart way of storing the read status, if it needs to change then we need to submit a change/commit per user per comment (worst case). |
Remove duplicate UserComment→CommentThread EF relationship, dedupe MarkThreadRead, prune stale unread rows on sync, fix comment sort order, and update CommentDialog plus coverage in CommentTests and SyncServiceTests. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve conflicts keeping comment-model APIs alongside develop's CreateEntryOptions, picture support, sync error handling, and i18n entries.
Re-run Lingui extract to reconcile locale files with merged source strings.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Update comment serialization regression data and keep both comment read status and commit metadata dependencies in CrdtMiniLcmApi. Co-authored-by: Cursor <cursoragent@cursor.com>
- Fix comment edit/delete guard to accept either LastUserId or ClientId, so an offline-authored comment isn't locked after the author logs in - Skip the current user's own comments when marking synced comments unread, and treat a null current user as "nothing is ours" - Translate the Open/Closed thread status - void async form-submit handlers, export the new comment types from the dotnet-types barrel, drop a redundant array copy - Reconcile i18n catalogs after the dashboard removal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Compare AuthorId directly against LastUserId and ClientId. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
myieye
approved these changes
Jul 2, 2026
myieye
left a comment
myieye
left a comment
Collaborator
There was a problem hiding this comment.
I think this is good enough for a first go. Pushed a few small changes.
(And removed the dashboard which was presumably unintentionally commited here)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/FwLiteShared/Sync/SyncService.cs (1)
99-118: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWrap
ApplySyncedCommentReadStatusin a try/catch likeSendNotifications.
ApplySyncedCommentReadStatusruns unguarded right afterUpdateSyncStatus(SyncStatus.Success)is broadcast. If it throws (DB error, transient failure, etc.),UpdateSyncDateandSendNotificationsare skipped entirely even though the actual CRDT sync (dataModel.SyncWith) already succeeded and the UI was already told the sync succeeded. This leaves the sync date stale, causing redundant re-processing on the next sync, and produces an inconsistent success/failure signal.SendNotificationsalready follows this exact defensive pattern (catch-and-log) for a similarly best-effort, non-critical side effect.🛡️ Proposed fix
private async Task ApplySyncedCommentReadStatus(SyncResults syncResults, string? currentUserId) { - await commentReadStatusService.MarkCommentsUnread(GetUnreadCommentsFromSyncResults(syncResults, currentUserId)); - var (deletedCommentIds, deletedThreadIds) = GetDeletedCommentsFromSyncResults(syncResults); - await commentReadStatusService.RemoveUnreadComments(deletedCommentIds); - foreach (var threadId in deletedThreadIds) - { - await commentReadStatusService.MarkThreadRead(threadId); - } + try + { + await commentReadStatusService.MarkCommentsUnread(GetUnreadCommentsFromSyncResults(syncResults, currentUserId)); + var (deletedCommentIds, deletedThreadIds) = GetDeletedCommentsFromSyncResults(syncResults); + await commentReadStatusService.RemoveUnreadComments(deletedCommentIds); + foreach (var threadId in deletedThreadIds) + { + await commentReadStatusService.MarkThreadRead(threadId); + } + } + catch (Exception e) + { + logger.LogError(e, "Failed to apply synced comment read status, continuing"); + } }Also applies to: 239-248
🤖 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 `@backend/FwLite/FwLiteShared/Sync/SyncService.cs` around lines 99 - 118, Wrap ApplySyncedCommentReadStatus in SyncService.SyncAsync with the same best-effort try/catch pattern used for SendNotifications, so a failure there does not abort the rest of a successful sync. Catch and log any exception from ApplySyncedCommentReadStatus, then continue to await syncRepository.UpdateSyncDate(syncDate), TryEnsureProjectChangeListener(project), and SendNotifications so the sync date and downstream side effects still complete after dataModel.SyncWith succeeds.
🧹 Nitpick comments (2)
frontend/viewer/src/project/browse/EntryView.svelte (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
SubjectTypefrom thedotnet-typesbarrel.
SubjectTypeis already re-exported from$lib/dotnet-types, so this file can match the existingIEntryimport and avoid depending on the generated path.♻️ Suggested fix
- import {SubjectType} from '$lib/dotnet-types/generated-types/MiniLcm/Models/SubjectType'; + import {SubjectType} from '$lib/dotnet-types';🤖 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 `@frontend/viewer/src/project/browse/EntryView.svelte` at line 29, The EntryView.svelte import for SubjectType should use the dotnet-types barrel instead of the generated MiniLcm path. Update the import alongside the existing IEntry import so this component depends on $lib/dotnet-types consistently and continues to reference SubjectType by name from the shared barrel.frontend/viewer/src/project/demo/in-memory-demo-api.ts (1)
48-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport these comment types from
$lib/dotnet-types. The barrel already exportsICommentThread,IUserComment,SubjectType, andThreadStatus, so the deep generated paths add unnecessary coupling to the generated file layout.🤖 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 `@frontend/viewer/src/project/demo/in-memory-demo-api.ts` around lines 48 - 51, The imports in the in-memory demo API are reaching into deep generated paths instead of using the `$lib/dotnet-types` barrel, which unnecessarily couples the code to the generated folder structure. Update the `in-memory-demo-api` module to import `ICommentThread`, `IUserComment`, `SubjectType`, and `ThreadStatus` from `$lib/dotnet-types` directly, keeping the rest of the file unchanged.
🤖 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
`@backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txt`:
- Around line 3938-3966: The snapshot fixture is internally inconsistent: the
nested CommentThread object in
SnapshotDeserializationRegressionData.latest.verified.txt has a UserComment
whose CommentThreadId does not match the enclosing CommentThread Id, and the
thread timestamps are reversed. Update the CommentThread/Comments entries so the
CommentThreadId on the nested comment matches the surrounding CommentThread Id,
and make the CommentThread CreatedAt and UpdatedAt values chronologically valid
while keeping the MiniLcmCrdtAdapter object self-consistent.
In `@backend/FwLite/LcmCrdt/Changes/Comments/CreateCommentThreadChange.cs`:
- Around line 34-45: `CreateCommentThreadChange.NewEntity` does not populate
`DeletedAt`, so threads created for already-deleted subjects can remain visible.
Update `NewEntity` to mirror the deletion state used in
`CreateUserCommentChange` by setting `CommentThread.DeletedAt` when the
referenced subject (`SubjectId`/`SubjectType`) is already soft-deleted,
otherwise leave it unset; use `CommentThread`, `NewEntity`, and the
subject-deletion check to locate the fix.
In `@backend/FwLite/LcmCrdt/Changes/Comments/EditUserCommentChange.cs`:
- Around line 8-31: The JsonConstructor on EditUserCommentChange is private, so
System.Text.Json will not use it during replay deserialization. Make the
[JsonConstructor] constructor public in EditUserCommentChange, and apply the
same visibility fix to the corresponding constructor in
SetCommentThreadStatusChange so both change types can deserialize correctly.
In `@backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs`:
- Around line 1185-1191: `DeleteCommentThread` currently allows any caller to
delete a thread once it exists, so add an authorization guard before
`AddChange(new DeleteChange<CommentThread>(threadId))`. Reuse the existing
auth/ownership pattern from `EditUserComment` and `DeleteUserComment`, or check
for an admin/moderator role if that is the intended rule, and keep the
`repo.GetCommentThread(threadId)` existence check and
`commentReadStatusService.MarkThreadRead` flow unchanged.
In `@backend/FwLite/LcmCrdt/Data/LocalCommentReadStatusService.cs`:
- Around line 69-93: The current `MarkCommentsUnread` flow in
`LocalCommentReadStatusService` does a read-then-insert on `UnreadComments`,
which can race when the same `CommentId` is processed concurrently. Update the
insert path to be idempotent in the database layer, preferably by using
SQLite-style ignore-on-conflict behavior for `UnreadComment` inserts, or
otherwise wrap `SaveChangesAsync` in handling for the unique-constraint
violation on `CommentId` and treat duplicates as a no-op. Keep the existing
dedupe logic, but make the
`dbContext.UnreadComments.AddRange`/`SaveChangesAsync` sequence safe under
concurrent calls.
In `@backend/FwLite/MiniLcm/Models/Comments.cs`:
- Around line 21-48: `CommentThread` is not tracking its subject reference, so
update `GetReferences()` to return `SubjectId` and make `RemoveReference()`
handle that same identifier. In `CommentThread`, when the removed id matches
`SubjectId`, set `DeletedAt` to the provided time and mark the thread as
deleted/closed as appropriate for this model; otherwise leave it unchanged. Keep
the existing `Copy()` behavior intact.
In
`@frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmFeatures.ts`:
- Line 15: The demo feature flags are missing the new comments capability, so
the UI treats comment support as disabled even though the demo backend
implements comment CRUD. Update the supported feature reporting in
in-memory-demo-api.ts, specifically the supportedFeatures() return object, to
include comments: true alongside the existing write, audio, and customViews
flags. Keep the generated IMiniLcmFeatures contract aligned with this flag so
the frontend can detect comment support correctly.
In `@frontend/viewer/src/lib/entry-editor/CommentDialog.svelte`:
- Around line 51-65: The CommentDialog open flow in threadsResource only fetches
thread data and never marks it as read, so unread state stays stale. Update the
open path in CommentDialog.svelte to call markCommentThreadRead and/or
markCommentRead after successfully loading threads, using the existing api
methods in the same component so the read state is refreshed whenever the dialog
opens.
- Around line 202-217: The Edit action in CommentDialog.svelte is shown to
everyone, but only the original author should be able to edit their comment.
Update the threadView.comments rendering so the Button near
startEditing(comment) is conditionally displayed/disabled based on a check
against projectContext.projectData?.lastUserId and the comment’s author
identity, keeping the edit control hidden for non-authors.
---
Outside diff comments:
In `@backend/FwLite/FwLiteShared/Sync/SyncService.cs`:
- Around line 99-118: Wrap ApplySyncedCommentReadStatus in SyncService.SyncAsync
with the same best-effort try/catch pattern used for SendNotifications, so a
failure there does not abort the rest of a successful sync. Catch and log any
exception from ApplySyncedCommentReadStatus, then continue to await
syncRepository.UpdateSyncDate(syncDate),
TryEnsureProjectChangeListener(project), and SendNotifications so the sync date
and downstream side effects still complete after dataModel.SyncWith succeeds.
---
Nitpick comments:
In `@frontend/viewer/src/project/browse/EntryView.svelte`:
- Line 29: The EntryView.svelte import for SubjectType should use the
dotnet-types barrel instead of the generated MiniLcm path. Update the import
alongside the existing IEntry import so this component depends on
$lib/dotnet-types consistently and continues to reference SubjectType by name
from the shared barrel.
In `@frontend/viewer/src/project/demo/in-memory-demo-api.ts`:
- Around line 48-51: The imports in the in-memory demo API are reaching into
deep generated paths instead of using the `$lib/dotnet-types` barrel, which
unnecessarily couples the code to the generated folder structure. Update the
`in-memory-demo-api` module to import `ICommentThread`, `IUserComment`,
`SubjectType`, and `ThreadStatus` from `$lib/dotnet-types` directly, keeping the
rest of the file unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e7fe140-b273-4e28-83f1-6a7ef605ddde
📒 Files selected for processing (58)
backend/FwLite/FwLiteShared.Tests/Sync/SyncServiceTests.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/FwLiteShared/Sync/SyncService.csbackend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.csbackend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.csbackend/FwLite/LcmCrdt.Tests/ConfigRegistrationTests.csbackend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationRegressionData.latest.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyChangeModels.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txtbackend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyIObjectWithIdModels.verified.txtbackend/FwLite/LcmCrdt.Tests/MiniLcmTests/CommentTests.csbackend/FwLite/LcmCrdt/Changes/Comments/CreateCommentThreadChange.csbackend/FwLite/LcmCrdt/Changes/Comments/CreateUserCommentChange.csbackend/FwLite/LcmCrdt/Changes/Comments/EditUserCommentChange.csbackend/FwLite/LcmCrdt/Changes/Comments/SetCommentThreadStatusChange.csbackend/FwLite/LcmCrdt/CrdtMiniLcmApi.csbackend/FwLite/LcmCrdt/Data/LocalCommentReadStatusService.csbackend/FwLite/LcmCrdt/Data/MiniLcmRepository.csbackend/FwLite/LcmCrdt/Data/UnreadComment.csbackend/FwLite/LcmCrdt/LcmCrdtDbContext.csbackend/FwLite/LcmCrdt/LcmCrdtKernel.csbackend/FwLite/LcmCrdt/Migrations/20260629042525_AddComments.Designer.csbackend/FwLite/LcmCrdt/Migrations/20260629042525_AddComments.csbackend/FwLite/LcmCrdt/Migrations/LcmCrdtDbContextModelSnapshot.csbackend/FwLite/MiniLcm/IMiniLcmReadApi.csbackend/FwLite/MiniLcm/IMiniLcmWriteApi.csbackend/FwLite/MiniLcm/Models/Comments.csbackend/FwLite/MiniLcm/Models/IObjectWithId.csbackend/FwLite/MiniLcm/Normalization/MiniLcmApiWriteNormalizationWrapper.csfrontend/viewer/src/lib/components/ui/input-group/index.tsfrontend/viewer/src/lib/components/ui/input-group/input-group-addon.sveltefrontend/viewer/src/lib/components/ui/input-group/input-group-button.sveltefrontend/viewer/src/lib/components/ui/input-group/input-group-input.sveltefrontend/viewer/src/lib/components/ui/input-group/input-group-text.sveltefrontend/viewer/src/lib/components/ui/input-group/input-group-textarea.sveltefrontend/viewer/src/lib/components/ui/input-group/input-group.sveltefrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmFeatures.tsfrontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMiniLcmJsInvokable.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ICommentThread.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/IUserComment.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/SubjectType.tsfrontend/viewer/src/lib/dotnet-types/generated-types/MiniLcm/Models/ThreadStatus.tsfrontend/viewer/src/lib/dotnet-types/index.tsfrontend/viewer/src/lib/entry-editor/CommentDialog.sveltefrontend/viewer/src/lib/entry-editor/EntityListItemActions.sveltefrontend/viewer/src/lib/entry-editor/object-editors/EntryEditor.sveltefrontend/viewer/src/lib/services/feature-service.tsfrontend/viewer/src/locales/en.pofrontend/viewer/src/locales/es.pofrontend/viewer/src/locales/fr.pofrontend/viewer/src/locales/id.pofrontend/viewer/src/locales/ko.pofrontend/viewer/src/locales/ms.pofrontend/viewer/src/locales/sw.pofrontend/viewer/src/locales/vi.pofrontend/viewer/src/project/browse/EntryView.sveltefrontend/viewer/src/project/demo/in-memory-demo-api.ts
…gainst concurrent duplicates. Co-authored-by: Cursor <cursoragent@cursor.com>
… opening comment author. Co-authored-by: Cursor <cursoragent@cursor.com>
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.
closes #1984
introduces the new commenting model, along with a basic UI for making comments