Add context to activity changes#2093
Conversation
github-actions
Bot
added
the
💻 FW Lite
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces change context functionality to the activity history feature. Backend adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/FwLite/LcmCrdt/HistoryService.cs (1)
220-229: Consider reverting toStartsWithfor more precise matching.Line 224 changed from
type.Name.StartsWith("JsonPatch")totype.Name.Contains("JsonPatch"). This makes the matching less precise and could incorrectly match type names like"NotJsonPatchChange"or"CustomJsonPatchHandler".Unless there's a specific requirement for partial matching, the original
StartsWithapproach was safer and more precise.Apply this diff to restore precise matching:
- if (type.Name.Contains("JsonPatch")) return $"Edit{change.EntityType.Name}".Humanize(); + if (type.Name.StartsWith("JsonPatch")) return $"Edit{change.EntityType.Name}".Humanize();
🧹 Nitpick comments (3)
frontend/viewer/src/lib/components/ui/format/format-date.svelte (1)
44-44: Good UX enhancement with native tooltip.Adding the full date/time as a tooltip provides helpful additional context while keeping the display concise.
Note: The native
titleattribute has limited accessibility support (e.g., not keyboard-accessible, inconsistent screen reader behavior). For enhanced accessibility, consider using a custom tooltip component in the future, though the current implementation is acceptable as a progressive enhancement.frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte (2)
38-54: Add type guards for snapshot type assertions.The type assertions at lines 44 and 47 assume
context.snapshotmatches thecontext.entityTypewithout validation. If the data is inconsistent, this could cause runtime errors.Consider adding type guards or validation:
let currentEntity = $derived.by(() => { if (!affectedEntry) return undefined; if (context.entityType === 'Entry') { return affectedEntry; } else if (context.entityType === 'Sense') { - const senseId = (context.snapshot as ISense).id; + const snapshot = context.snapshot as ISense; + if (!snapshot?.id) return undefined; + const senseId = snapshot.id; return affectedEntry.senses.find((s) => s.id === senseId); } else if (context.entityType === 'ExampleSentence') { - const exampleId = (context.snapshot as IExampleSentence).id; + const snapshot = context.snapshot as IExampleSentence; + if (!snapshot?.id) return undefined; + const exampleId = snapshot.id; for (const sense of affectedEntry.senses) { const example = sense.exampleSentences.find((ex) => ex.id === exampleId); if (example) return example; } } return undefined; });
120-137: Type assertions lack runtime validation.The type assertions
(showCurrent ? currentEntity : context.snapshot) as IEntryat lines 123, 129, and 135 assume the snapshot type matches the entityType, but there's no runtime check. If the data is inconsistent orcurrentEntityis undefined whenshowCurrentis true, this could cause issues.Consider adding defensive checks or using type guards to ensure type safety at runtime, especially since
currentEntitycan beundefinedbased on the derived logic at lines 38-54.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.cs(1 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(1 hunks)backend/FwLite/FwLiteWeb/FwLiteWebServer.cs(1 hunks)backend/FwLite/FwLiteWeb/HttpHelpers.cs(1 hunks)backend/FwLite/LcmCrdt/CurrentProjectService.cs(1 hunks)backend/FwLite/LcmCrdt/HistoryService.cs(6 hunks)frontend/viewer/src/lib/DictionaryEntry.svelte(2 hunks)frontend/viewer/src/lib/Headwords.svelte(1 hunks)frontend/viewer/src/lib/activity/ActivityItem.svelte(1 hunks)frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte(1 hunks)frontend/viewer/src/lib/activity/ActivityView.svelte(3 hunks)frontend/viewer/src/lib/activity/utils.ts(1 hunks)frontend/viewer/src/lib/components/ui/format/format-date.svelte(1 hunks)frontend/viewer/src/lib/components/ui/format/format-duration.svelte(1 hunks)frontend/viewer/src/lib/components/ui/format/format-duration.ts(1 hunks)frontend/viewer/src/lib/components/ui/format/format-relative-date.svelte(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IChangeContext.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IHistoryLineItem.ts(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/index.ts(1 hunks)frontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.svelte(2 hunks)frontend/viewer/src/lib/services/history-service.ts(3 hunks)frontend/viewer/src/lib/services/multi-window-service.ts(1 hunks)frontend/viewer/src/locales/en.po(16 hunks)frontend/viewer/src/locales/es.po(16 hunks)frontend/viewer/src/locales/fr.po(16 hunks)frontend/viewer/src/locales/id.po(16 hunks)frontend/viewer/src/locales/ko.po(16 hunks)frontend/viewer/src/locales/ms.po(16 hunks)frontend/viewer/src/locales/sw.po(16 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-30T04:53:41.702Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props `index` and `actions` are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.
Applied to files:
frontend/viewer/src/lib/activity/ActivityItem.sveltefrontend/viewer/src/lib/activity/ActivityItemChangePreview.sveltefrontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.svelte
📚 Learning: 2025-07-22T09:19:37.386Z
Learnt from: rmunn
Repo: sillsdev/languageforge-lexbox PR: 1836
File: frontend/viewer/src/lib/components/audio/AudioDialog.svelte:25-25
Timestamp: 2025-07-22T09:19:37.386Z
Learning: In the sillsdev/languageforge-lexbox project, when file size limits or other constants need to be shared between C# backend and TypeScript frontend code, prefer exposing them through Reinforced.Typings type generation rather than hardcoding the values separately. This ensures consistency and prevents discrepancies when values change.
Applied to files:
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs
📚 Learning: 2025-10-06T12:48:36.601Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 2025
File: frontend/viewer/src/locales/sw.po:146-149
Timestamp: 2025-10-06T12:48:36.601Z
Learning: Do not review or comment on `.po` translation files (e.g., files in `frontend/viewer/src/locales/`) in the languageforge-lexbox repository, as translations are managed by Crowdin.
Applied to files:
frontend/viewer/src/locales/en.pofrontend/viewer/src/locales/ko.po
📚 Learning: 2025-07-07T06:02:41.194Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1804
File: backend/FwLite/LcmCrdt/CurrentProjectService.cs:18-19
Timestamp: 2025-07-07T06:02:41.194Z
Learning: In the CurrentProjectService class, the service locator pattern is intentionally used to retrieve IDbContextFactory<LcmCrdtDbContext> and EntrySearchServiceFactory because these services indirectly depend on CurrentProjectService to have the current project set, creating a circular dependency. This is an acceptable use of service locator to break the circular dependency while keeping project context responsibility consolidated.
Applied to files:
backend/FwLite/LcmCrdt/CurrentProjectService.cs
📚 Learning: 2025-06-27T09:24:39.507Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1760
File: backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs:274-277
Timestamp: 2025-06-27T09:24:39.507Z
Learning: In the CrdtMiniLcmApi class, the user prefers to keep the current AddChanges method signature (IEnumerable<IChange>) rather than modifying it to support IAsyncEnumerable for streaming, even when it means materializing collections into memory for bulk operations.
Applied to files:
backend/FwLite/LcmCrdt/HistoryService.cs
📚 Learning: 2025-06-02T14:27:02.745Z
Learnt from: myieye
Repo: sillsdev/languageforge-lexbox PR: 1720
File: frontend/viewer/src/locales/es.json:1786-1790
Timestamp: 2025-06-02T14:27:02.745Z
Learning: Spanish locale file (frontend/viewer/src/locales/es.json) contains generated code that may have null line numbers in origin entries due to limitations in the code generation process.
Applied to files:
frontend/viewer/src/locales/es.po
📚 Learning: 2025-05-27T06:18:33.852Z
Learnt from: hahn-kev
Repo: sillsdev/languageforge-lexbox PR: 1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Applied to files:
frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte
🧬 Code graph analysis (7)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IChangeContext.ts (1)
IChangeContext(9-17)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
backend/FwLite/LcmCrdt/HistoryService.cs (1)
ChangeContext(29-32)
frontend/viewer/src/lib/services/history-service.ts (2)
frontend/viewer/src/lib/project-context.svelte.ts (1)
ProjectContext(36-164)frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IProjectActivity.ts (1)
IProjectActivity(9-16)
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.cs (2)
backend/FwLite/LcmCrdt/HistoryService.cs (1)
ChangeContext(29-32)frontend/viewer/src/lib/project-context.svelte.ts (1)
historyService(93-95)
backend/FwLite/FwLiteWeb/FwLiteWebServer.cs (2)
backend/FwLite/FwLiteWeb/HttpHelpers.cs (1)
GetProjectCode(7-11)backend/FwLite/LcmCrdt/CurrentProjectService.cs (1)
CurrentProjectService(11-163)
backend/FwLite/LcmCrdt/HistoryService.cs (3)
backend/FwLite/FwDataMiniLcmBridge/FieldWorksProjectList.cs (2)
IMiniLcmApi(45-49)IMiniLcmApi(51-54)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (8)
IAsyncEnumerable(121-128)IAsyncEnumerable(162-169)IAsyncEnumerable(215-222)IAsyncEnumerable(259-266)IAsyncEnumerable(352-355)IAsyncEnumerable(388-391)IAsyncEnumerable(393-400)IAsyncEnumerable(613-628)backend/FwLite/FwLiteShared/Sync/SyncService.cs (1)
IAsyncEnumerable(202-222)
frontend/viewer/src/lib/services/multi-window-service.ts (2)
frontend/viewer/src/css-breakpoints.ts (1)
MOBILE_BREAKPOINT(2-2)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IMultiWindowService.ts (1)
IMultiWindowService(6-9)
🔇 Additional comments (36)
frontend/viewer/src/lib/components/ui/format/format-date.svelte (1)
38-41: LGTM! Well-implemented tooltip enhancement.The
fullDatederived value correctly provides a detailed date/time format for the tooltip. Using the existingformatDatefunction ensures consistent localization handling.frontend/viewer/src/lib/components/ui/format/format-duration.ts (1)
60-66: Good addition for observability.The error handling with contextual logging is helpful for debugging duration formatting issues, especially with the new
maxUnitsparameter introduced in this PR.frontend/viewer/src/lib/components/ui/format/format-duration.svelte (2)
21-21: Correct parameter passing.The
maxUnitsparameter is correctly passed as the fourth argument toformatDuration, maintaining the proper parameter order.
4-18: Edge case handling formaxUnitsis adequate.Verification confirms the implementation handles edge cases safely:
- maxUnits = 0: Skipped by falsy check (line 59), returns normalized duration
- maxUnits < 0: Loop breaks immediately, returns minimal valid duration
- maxUnits > 0: Works as designed
The code is defensive and matches the
.tsfunction signature. No validation is needed.frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IHistoryLineItem.ts (1)
6-6: LGTM! Type definition extended correctly.The addition of the
changeproperty with its corresponding import aligns with the PR's goal of adding change context to activity history.Also applies to: 16-16
frontend/viewer/src/lib/components/ui/format/format-relative-date.svelte (1)
74-74: LGTM! Improved popover spacing.The additional padding improves the visual layout of the actual date popover content.
backend/FwLite/LcmCrdt/CurrentProjectService.cs (1)
83-86: LGTM! Consistent parameter naming.The rename from
projectNametoprojectCodeimproves consistency across the codebase and aligns with theGetProjectCodemethod usage.backend/FwLite/FwLiteWeb/FwLiteWebServer.cs (1)
100-103: LGTM! Consistent naming update.The change from
GetProjectNametoGetProjectCodealigns with the parameter rename inCurrentProjectService.SetupProjectContextand improves naming consistency.frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/index.ts (1)
6-6: LGTM! Public API expanded correctly.The export of
IChangeContextexpands the module's public API surface to support the new change context functionality.frontend/viewer/src/lib/DictionaryEntry.svelte (1)
9-9: LGTM! Good refactoring for code reuse.Extracting headword rendering into a dedicated
Headwordscomponent improves maintainability and promotes consistency across the application.Also applies to: 100-100
frontend/viewer/src/lib/activity/ActivityItemChangePreview.svelte (1)
100-103: ****The code is working as designed. Based on the backend implementation in
HistoryService.cs,GetAffectedEntryIdsyields:
- Exactly 1 entry for Entry, Sense, and ExampleSentence types (lines 177, 183, 191)
- Exactly 2 entries for ComplexFormComponent type (lines 197-198)
The component correctly handles this: for non-ComplexFormComponent types,
affectedEntryis derived to be defined only whenaffectedEntries.length === 1, and the outer condition at line 97 enforces this constraint. ComplexFormComponent already has its own separate rendering logic usingfind()calls.The nested check at lines 100-103 is redundant but harmless—it cannot occur in any other scenario. No changes are needed.
Likely an incorrect or invalid review comment.
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
172-172: LGTM! ChangeContext properly exported for TypeScript generation.The addition of
ChangeContextto the exported interfaces list correctly expands the typing surface to support the new change context functionality. This follows the established pattern for exposing backend types to the frontend.frontend/viewer/src/lib/services/history-service.ts (3)
33-38: LGTM! Useful addition ofloaded()getter.The
loaded()getter provides a clean way to check if the history service is available, improving the service's public interface.
40-53: LGTM! Correctly usesprojectCodein API path.The update to use
projectCodein the API path maintains consistency with the renamed parameter throughout the codebase. The timestamp assignment and array reversal logic is correct.
71-73: LGTM! Parameter rename improves consistency.Renaming the parameter from
projectNametoprojectCodealigns with the broader naming improvements across the codebase.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.ts (1)
10-10: LGTM! Generated typings correctly reflect backend API.The generated TypeScript interface properly exposes the new
loadChangeContextmethod with correct type signatures matching the backendHistoryServiceJsInvokableimplementation.Also applies to: 18-18
backend/FwLite/FwLiteShared/Services/HistoryServiceJsInvokable.cs (1)
34-38: LGTM! JSInvokable method follows established patterns.The new
LoadChangeContextmethod correctly implements the cross-boundary API pattern used throughout the class, with proper async/await handling and JSInvokable decoration.frontend/viewer/src/lib/Headwords.svelte (2)
1-29: LGTM! Well-structured component with proper reactive state.The component correctly derives headwords from writing systems with appropriate filtering and formatting. The use of
$derived.by()ensures reactive updates, and the optional placeholder prop provides good UX for empty states.
31-41: LGTM! Clean template with proper separator logic.The template correctly renders headwords with " / " separators between items (not before the first), and the comment on Line 34 helpfully explains the whitespace preservation requirement.
frontend/viewer/src/lib/activity/ActivityView.svelte (2)
3-3: LGTM! Import updates support the refactored UI.The changes to use the
tfunction directly and import the newActivityItemcomponent align with the simplified activity display approach.Also applies to: 9-10
54-62: LGTM! Refactoring improves component structure.Delegating activity detail rendering to the
ActivityItemcomponent improves separation of concerns and maintainability. The new two-column header layout with relative date and author name provides better UX.Also applies to: 71-71
frontend/viewer/src/locales/sw.po (1)
21-21: LGTM! Translation file correctly updated for new components.The localization updates properly reflect the component restructuring, with new keys added for
ActivityItemandActivityItemChangePreview. Empty translation strings (msgstr "") are expected pending translator input.Also applies to: 25-27, 258-275, 310-312, 352-354, 860-863
backend/FwLite/FwLiteWeb/HttpHelpers.cs (1)
7-7: LGTM! Method rename improves naming accuracy.Renaming
GetProjectNametoGetProjectCodebetter reflects the method's purpose of extracting the project code from route values. This aligns with the broader naming improvements across the codebase.frontend/viewer/src/lib/entry-editor/EntryOrSenseItemList.svelte (2)
8-13: LGTM!The imports and service initializations correctly integrate routing and multi-window functionality, enabling context-aware menu actions.
37-42: LGTM!The new "Open in new Window" menu item is correctly implemented with defensive null checking on
multiWindowService, preventing potential runtime errors when the service is unavailable.frontend/viewer/src/lib/activity/ActivityItem.svelte (3)
1-13: LGTM!The exported types
ActivityandChangeWithContextare well-structured and provide a clean public API for the activity item functionality.
29-35: LGTM!The reactive
changesderivation correctly uses theresourcehelper to asynchronously load change contexts, ensuring all contexts are available before rendering.
70-102: LGTM!The template correctly implements defensive rendering with
changes?.currentchecks and usesVListfor performance optimization when displaying many changes. The tabbed interface for Preview and Details provides good UX.frontend/viewer/src/lib/dotnet-types/generated-types/LcmCrdt/IChangeContext.ts (1)
1-18: LGTM!The generated
IChangeContextinterface correctly defines the TypeScript type for the backendChangeContextrecord, providing proper type safety for the change context functionality.frontend/viewer/src/locales/ko.po (1)
1-1346: Skipping review of translation file.Translation files in
frontend/viewer/src/locales/are managed by Crowdin and should not be reviewed here.Based on learnings
backend/FwLite/LcmCrdt/HistoryService.cs (6)
22-34: LGTM!The
ChangeContextrecord is well-structured with proper null handling for theEntityTypeproperty.
36-69: LGTM!The
HistoryLineItemrecord has been correctly updated to include theIChangefield, with proper constructor chaining.
73-91: LGTM!The refactored LINQ query for
ProjectActivity()properly usesNormalizeTimestampto address timezone issues and correctly structures the multi-join query.
159-171: LGTM!The
GetCurrentOrLatestEntrymethod properly handles both existing and deleted entries with appropriate fallback logic and exception handling.
202-207: LGTM!The
NormalizeTimestampmethod correctly addresses the Linq2DB timezone issue with a clear explanation and proper UTC reinterpretation.
139-157: Verify theGetAtCommitmethod signature.Line 150 calls
dataModel.GetAtCommit<IObjectWithId>(commitId, change.EntityId, changeIndex)with three parameters, but theGetObjectmethod at line 99 shows thatGetAtCommitonly accepts two parameters (commitIdandentityId). This would cause a compilation error unless there's an overload that acceptschangeIndex.Run the following script to check if an overload exists:
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
1 participant
And reused the new design in the history dialog:
Still not optimized for mobile, but that's for another day:
