Sync state dialog/UI for FW Lite#1668
Conversation
|
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 📝 WalkthroughWalkthroughThese changes introduce a new synchronization status API and UI dialog for projects. Backend services and data models are extended to expose sync status information, while the frontend adds a dialog component, supporting services, and type definitions to display and trigger sync actions. TypeScript and .NET types are updated for interop. Changes
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Now every part of the mockup is replicated in the SyncDialog component.
Most common reason is user not logged in to Lexbox server, so display basic error message to that effect. We can improve this later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
frontend/viewer/src/project/ProjectSidebar.svelte (1)
90-105: 🛠️ Refactor suggestionPotential improvement for sync dialog integration
The integration of the SyncDialog component is good, but there are a couple of improvements that could be made:
- The sync indicator (green/red dot) is still using the
isSynchronizingstate variable which doesn't appear to be connected to the actual sync status from the new service.- The optional chaining in
syncDialog?.open()is correct, but should be accompanied by a check that the component is properly initialized.<SyncDialog bind:this={syncDialog} syncLbToLocal={() => console.log('Would sync Lexbox to local')} syncLbToFlex={() => console.log('Would sync Lexbox to FieldWorks')} + on:syncStateChange={(e) => isSynchronizing = e.detail.isSyncing} /> -<Sidebar.MenuButton onclick={() => syncDialog?.open()} class="justify-between"> +<Sidebar.MenuButton + onclick={() => { + if (syncDialog) { + syncDialog.open(); + } else { + console.error('Sync dialog not initialized'); + } + }} + class="justify-between">
🧹 Nitpick comments (8)
frontend/viewer/src/lib/services/service-provider.ts (1)
18-125: Consider adding helper functions for the new servicesI notice you've added the services to the registry, but unlike other services (e.g.,
useAuthService()), there are no corresponding helper functions likeuseHistoryService()oruseSyncService(). Consider adding these if they would be useful in multiple components.Example implementation:
+export function useHistoryService(): IHistoryServiceJsInvokable { + return window.lexbox.ServiceProvider.getService(DotnetService.HistoryService); +} + +export function useSyncService(): ISyncServiceJsInvokable { + return window.lexbox.ServiceProvider.getService(DotnetService.SyncService); +}backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (1)
107-120: Implement consistent error handling with retry mechanism.The error handling is appropriate, but consider implementing a retry mechanism for transient HTTP failures, which are common when making API calls.
public async Task<ProjectSyncStatus?> GetLexboxSyncStatus(LexboxServer server, Guid projectId) { var httpClient = await clientFactory.GetClient(server).CreateHttpClient(); if (httpClient is null) return null; try { - return (await httpClient.GetFromJsonAsync<ProjectSyncStatus?>($"/api/fw-lite/sync/status/{projectId}")); + // Add retry for transient failures + const int maxRetries = 3; + for (int i = 0; i < maxRetries; i++) + { + try + { + return await httpClient.GetFromJsonAsync<ProjectSyncStatus?>($"/api/fw-lite/sync/status/{projectId}"); + } + catch (HttpRequestException ex) when (i < maxRetries - 1 && (ex.StatusCode == System.Net.HttpStatusCode.ServiceUnavailable || + ex.StatusCode == System.Net.HttpStatusCode.GatewayTimeout)) + { + // Only retry on specific status codes indicating transient issues + await Task.Delay((i + 1) * 500); // Exponential backoff + logger.LogWarning(ex, "Retrying lexbox sync status request after transient failure (attempt {Attempt})", i + 1); + } + } + // Last attempt - let any exceptions propagate to the outer catch + return await httpClient.GetFromJsonAsync<ProjectSyncStatus?>($"/api/fw-lite/sync/status/{projectId}"); } catch (Exception e) { logger.LogError(e, "Error getting lexbox sync status"); return null; } }backend/LexCore/Sync/SyncStatus.cs (1)
17-20: Consider adding XML documentation for properties.The code functions correctly, but adding XML documentation comments would improve developer experience when working with this class.
public record ProjectSyncStatus( [property:JsonConverter(typeof(JsonStringEnumConverter))] ProjectSyncStatusEnum status, + /// <summary> + /// Number of pending changes in the CRDT database that need to be synchronized. + /// </summary> int PendingCrdtChanges = 0, + /// <summary> + /// Number of pending Mercurial changes that need to be synchronized. + /// Will be -1 if there is no clone yet, meaning "all the commits, but unknown quantity". + /// </summary> int PendingMercurialChanges = 0, // Will be -1 if there is no clone yet; this means "all the commits, but we don't know how many there will be" + /// <summary> + /// Timestamp of the last CRDT commit that was synchronized. + /// </summary> DateTimeOffset? LastCrdtCommitDate = null, + /// <summary> + /// Timestamp of the last Mercurial commit that was synchronized. + /// </summary> DateTimeOffset? LastMercurialCommitDate = null)backend/FwLite/FwLiteShared/Services/SyncServiceJsInvokable.cs (1)
1-14: Well-structured JavaScript-invokable serviceThe implementation correctly follows the pattern for exposing .NET methods to JavaScript. The class takes a
SyncServicedependency via constructor injection and exposes a clean API for the frontend to consume.Consider adding exception handling to gracefully manage potential errors when calling
syncService.GetSyncStatus(). This would help prevent uncaught exceptions from bubbling up to the JavaScript layer.[JSInvokable] public Task<ProjectSyncStatus?> GetSyncStatus() { - return syncService.GetSyncStatus(); + try + { + return syncService.GetSyncStatus(); + } + catch (Exception ex) + { + // Log the exception + Console.Error.WriteLine($"Error getting sync status: {ex.Message}"); + return Task.FromResult<ProjectSyncStatus?>(null); + } }frontend/viewer/src/lib/services/sync-status-service.ts (1)
10-19: Redundant constructor parameterThe constructor takes a parameter named
projectContextwith aprivatemodifier, but then immediately assigns it to a private field#projectContext. This is redundant and could be simplified.export class SyncStatusService { #projectContext: ProjectContext; get syncStatusApi(): ISyncServiceJsInvokable | undefined { return this.#projectContext.syncService; } - constructor(private projectContext: ProjectContext) { - this.#projectContext = projectContext; + constructor(projectContext: ProjectContext) { + this.#projectContext = projectContext; }frontend/viewer/src/project/SyncDialog.svelte (3)
36-42: Address TODOs for tracking sync countsSeveral placeholder values and TODOs indicate incomplete implementation of sync count tracking.
The code contains hardcoded zero values and TODOs about tracking local-to-LB and LB-to-local counts. Would you like assistance in determining how to properly track these counts based on the available API data?
51-52: Consider enhancing the loading UIThe TODO comment suggests the loading indicator needs improvement.
Consider implementing a loading overlay that maintains the dialog structure while showing the loading state:
{#if loading} - <!-- TODO: Show loading as an overlay? --> - <Loading /> + <div class="absolute inset-0 flex items-center justify-center bg-background/80 z-10"> + <Loading /> + </div> + <div class="opacity-50"> + <!-- Add a placeholder/skeleton UI here --> + <div class="grid grid-rows-5 grid-cols-3 justify-around rounded-lg border-4"> + <!-- Skeleton content --> + </div> + </div> {:else if !status}
22-22: Address commented code and TODOThere's commented code with a TODO about finding the project ID.
Either implement the TODO or remove the commented code to keep the codebase clean:
- // status = await service.getStatus('How do I find the project ID to use in the fallback?'); // TODO
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (19)
backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs(2 hunks)backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs(3 hunks)backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs(6 hunks)backend/FwLite/FwLiteShared/Services/SyncServiceJsInvokable.cs(1 hunks)backend/FwLite/FwLiteShared/Sync/SyncService.cs(3 hunks)backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs(3 hunks)backend/LexBoxApi/Controllers/SyncController.cs(2 hunks)backend/LexCore/Sync/SyncStatus.cs(2 hunks)frontend/viewer/src/DotnetProjectView.svelte(2 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectScope.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/ISyncServiceJsInvokable.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/IProjectSyncStatus.ts(1 hunks)frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/ProjectSyncStatusEnum.ts(1 hunks)frontend/viewer/src/lib/project-context.svelte.ts(5 hunks)frontend/viewer/src/lib/services/service-provider.ts(2 hunks)frontend/viewer/src/lib/services/sync-status-service.ts(1 hunks)frontend/viewer/src/project/ProjectSidebar.svelte(4 hunks)frontend/viewer/src/project/SyncDialog.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
frontend/viewer/src/lib/services/service-provider.ts (2)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IHistoryServiceJsInvokable.ts (1)
IHistoryServiceJsInvokable(11-17)frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/ISyncServiceJsInvokable.ts (1)
ISyncServiceJsInvokable(8-11)
backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (2)
backend/FwLite/FwLiteShared/Sync/SyncService.cs (1)
SyncService(19-184)backend/FwLite/FwLiteShared/Services/SyncServiceJsInvokable.cs (1)
SyncServiceJsInvokable(7-14)
backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (1)
backend/LexCore/Sync/SyncStatus.cs (1)
ProjectSyncStatus(25-33)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/ISyncServiceJsInvokable.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/IProjectSyncStatus.ts (1)
IProjectSyncStatus(8-18)
frontend/viewer/src/lib/project-context.svelte.ts (1)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/ISyncServiceJsInvokable.ts (1)
ISyncServiceJsInvokable(8-11)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Publish FW Lite app for Android
- GitHub Check: Publish FW Lite app for Windows
🔇 Additional comments (38)
frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/DotnetService.ts (1)
14-14: Addition of SyncService looks goodThe
SyncServiceenum value has been properly added to support the new synchronization functionality. This aligns with the PR's objective of creating a sync dialog/UI.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/IProjectScope.ts (1)
14-14: Optional syncService property has been properly addedThe addition of the optional
syncServiceproperty to theIProjectScopeinterface correctly follows the established pattern of other service references likehistoryService.backend/LexBoxApi/Controllers/SyncController.cs (1)
4-4: Good security improvement with scope requirementAdding the
RequireScopeattribute withSendAndReceivescope is an important security enhancement that ensures only authorized users can access the synchronization endpoints.Also applies to: 14-14
frontend/viewer/src/lib/services/service-provider.ts (2)
18-19: Import of service interfaces looks goodThe import of both
IHistoryServiceJsInvokableandISyncServiceJsInvokableinterfaces is appropriate for type safety in the service registry.
29-30: Service registry entries properly addedThe addition of registry entries for
HistoryServiceandSyncServicecorrectly maps to their respective interface types.backend/FwLite/FwLiteShared/TypeGen/ReinforcedFwLiteTypingConfig.cs (3)
7-7: Good addition of the namespace import.The import of
LexCore.Syncnamespace is necessary to access theProjectSyncStatusEnumandProjectSyncStatustypes being added to the TypeScript generation configuration.
108-108: Correctly configured enum export.Adding
ProjectSyncStatusEnumto be exported as a string enum is appropriate. Using.UseString()ensures the TypeScript enum will use string values instead of numeric indices, which matches the JSON serialization approach in the backend and improves readability in network debugging.
121-121: Correctly added type to exported interfaces.Adding
ProjectSyncStatusto the list of types exported as interfaces with public properties ensures the frontend can properly type-check against the synchronization status data structure returned by the backend.backend/FwLite/FwLiteShared/Services/FwLiteProvider.cs (2)
48-48: Proper service type mapping.The mapping from
DotnetService.SyncServicetotypeof(SyncServiceJsInvokable)correctly integrates the new sync service with the existing service resolution infrastructure.
105-105: Good addition to DotnetService enum.Adding
SyncServiceto theDotnetServiceenum properly exposes the new service type to the frontend.frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/ProjectSyncStatusEnum.ts (1)
1-12: Generated enum matches backend implementation.This auto-generated TypeScript enum correctly represents the synchronization states from the backend. The string values match the enum values defined in the C# backend, ensuring consistent type safety across the stack.
frontend/viewer/src/DotnetProjectView.svelte (3)
4-4: Good updates to imports.The imports are correctly updated to include
DotnetServiceandISyncServiceJsInvokable, which are necessary for initializing and typing the sync service.Also applies to: 11-13
49-52: Proper initialization of sync service.The code correctly initializes the
syncServiceby checking if it exists in the project scope and then wraps it in a proxy using the appropriate service type. This follows the same pattern used for other services likehistoryService.
54-54: Correct integration with project context.The project context setup is properly updated to include the new
syncService, making it available to all components that use the project context.frontend/viewer/src/lib/dotnet-types/generated-types/FwLiteShared/Services/ISyncServiceJsInvokable.ts (1)
6-11: Interface looks good with appropriate Promise return type.The
ISyncServiceJsInvokableinterface correctly imports and uses theIProjectSyncStatustype, providing a clean contract for JavaScript interop with the backend sync service.backend/FwLite/FwLiteShared/Sync/SyncService.cs (3)
1-11: Appropriate imports added for the new sync functionality.The new imports for
LexCore.SyncandMicrosoft.Extensions.Optionsare correctly added to support the sync status functionality.
27-27: Constructor parameter added correctly.The
IOptions<AuthConfig> authOptionsparameter is properly added to the constructor to support server configuration retrieval.
81-87: Well-structured sync status retrieval method.The
GetSyncStatusmethod is well-implemented with appropriate error handling by returning null when the status can't be retrieved. The code follows the existing patterns in the class.backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs (1)
1-6: Appropriate imports added for HTTP JSON and sync types.The added imports are necessary for the new sync status functionality and HTTP JSON operations.
backend/LexCore/Sync/SyncStatus.cs (2)
1-5: Added JSON serialization support correctly.The addition of the JSON serialization namespace and converter ensures proper enum serialization in the API responses.
15-15: Proper JSON serialization for enum property.The
JsonConverterattribute ensures the enum values will be serialized as strings in JSON, which is better for cross-platform compatibility and readability.frontend/viewer/src/project/ProjectSidebar.svelte (2)
18-18: Good addition of the SyncDialog importThe import is correctly placed with other component imports.
42-42: Properly defined reactive state variableThe reactive state variable correctly uses Svelte's
$statedecorator, matching the pattern used for other dialog components in this file.frontend/viewer/src/lib/dotnet-types/generated-types/LexCore/Sync/IProjectSyncStatus.ts (1)
1-19: Auto-generated interface looks goodSince this is an auto-generated TypeScript interface, we shouldn't modify it directly. The structure correctly represents the C#
ProjectSyncStatusclass with appropriate property types.Note that the recursive structure (where nested statuses are of the same type as the parent) could cause infinite recursion if not handled carefully when working with this data. Also, date fields are represented as strings and will need parsing to be used as Date objects in the UI.
Be mindful when working with the nested status properties (
neverSynced,syncing,queuedToSync). If you print or iterate through this structure without limiting the depth, you could encounter a stack overflow or excessive memory usage.frontend/viewer/src/lib/services/sync-status-service.ts (1)
1-9: Good service factory implementationThe factory function
useSyncStatusServicefollows the pattern used for other services in the codebase. It correctly retrieves the project context and instantiates the service class.frontend/viewer/src/lib/project-context.svelte.ts (6)
6-8: LGTM: ISyncServiceJsInvokable import additionThe import of the ISyncServiceJsInvokable interface is correctly structured as a named import with proper type annotation.
19-19: LGTM: syncService property added to ProjectContextSetup interfaceThe optional syncService property is correctly added to the ProjectContextSetup interface with the proper type.
39-39: LGTM: Private reactive state field addedThe private #syncService field is properly declared using the $state directive, following the established pattern in this class.
68-70: LGTM: Public getter for syncServiceThe public getter follows the same pattern as other service getters in this class, returning the private field.
75-75: LGTM: Constructor initializationThe constructor correctly initializes #syncService from the optional args parameter.
84-84: LGTM: Setup method updateThe setup method correctly updates the #syncService field from the args parameter.
backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs (6)
64-66: LGTM: OpenCrdtProject signature updatedThe OpenCrdtProject method now correctly creates and passes the SyncServiceJsInvokable instance to the ProjectScope constructor.
87-87: LGTM: OpenFwDataProject updated to pass null syncServiceThe OpenFwDataProject method now passes null for both historyService and syncService parameters, maintaining consistency with the updated ProjectScope constructor.
100-102: LGTM: ProjectScope constructor signature updatedThe constructor signature is correctly extended to accept an optional SyncServiceJsInvokable parameter.
106-106: LGTM: SyncService initializationThe SyncService property is correctly initialized with a DotNetObjectReference if the syncService parameter is not null.
115-118: LGTM: SyncService disposal addedThe cleanup delegate now properly disposes of the SyncService reference if it exists, following the same pattern as HistoryService disposal.
133-133: LGTM: SyncService property addedThe public SyncService property is properly added to the ProjectScope class with the correct type.
frontend/viewer/src/project/SyncDialog.svelte (1)
1-11: LGTM: Component imports and structureThe imports are well-organized and include all necessary UI components, icons, dialog components, and utility modules needed for the sync dialog.
|
I played with the design a bit.
I couldn't really figure out how to test it. |
|
I don't understand why the auth error is complaining about the LexboxApi scope when I clearly put |
Feature flag requirement was requiring Lexbox API scope, which conflicts with the requirement for the SendAndReceive scope that we pass here. Disable the feature flag requirement for now to allow testing the sync dialog without OAuth errors.
|
NOTE: Revert commit c6db730 before merging, we don't want to remove the feature flag requirement when we actually push this. |
|
Got correct sync date working, local sync button tested, everything looks good. Only flaw left is that after synchronizing, the notification popup shows on the page, muted by the dialog's overlay until you click Close. Screenshot demonstrating this:
I thought of auto-closing the dialog, but that might not be what the user expects? ... Actually, no, I think it is. After the sync returns a success, if all the values are 0, I think we should set a timeout of 750 milliseconds to auto-close the dialog, so that the user has just enough time to see that the numbers went to 0 before the dialog auto-closes itself and they see the notification at the bottom of the screen. |
Arbitrarily picked 750 ms timeout, could configure that with prop
|
Commit c866494 auto-closes dialog 750 ms after a successful FW sync. Can easily revert that if it turns out to be a bad idea. I picked 750 ms because that's enough time to register that the numbers have become 0, but not so long that you're starting to move the mouse cursor over to the X button. Hopefully people will be pleasantly surprised that the dialog closes itself for them once they're done with it. |
|
I might have gotten a little carried away 😆 localhost_5137_project_elawa-dev-3-train-flex_browse_entryId.715b5801-0248-4643-aaf4-f57ebeedce6a.syncDialogOpen.true.-.Google.Chrome.2025-05-23.15-02-28.mp4 |
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
This is a whopping PR 💪.
As far as I can tell everything's working well! 🎉
|
This looks great! Nice work Robin. |
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
left some comments, one thing needs to be fixed, the rest are optional. Otherwise great job!
To not have an HTTP connection hanging that gets closed down after 30 seconds, we leave it open for 25 seconds and then retry if we didn't receive a response by that time, with a total configurable timeout defaulting to 15 minutes.
Because local sync might have pushed a change, so now the remote LB-to-FW sync may have different pending numbers.
3 participants
Fixes #1411.
Dialog fully functional, all numbers and dates are correct, sync buttons work.