Enhance SignalR connection handling and listener recovery#2368
Open
hahn-kev wants to merge 43 commits into
Open
Enhance SignalR connection handling and listener recovery#2368hahn-kev wants to merge 43 commits into
hahn-kev wants to merge 43 commits into
Conversation
ListenForProjectChanges previously registered a project in the
_reconnectProjects table only AFTER its state-based early returns. A
project subscribed while the underlying HubConnection was Reconnecting
would silently never be resubscribed when the connection came back.
Hoist the registration above the state checks so the comment ("Reconnected
handler will resubscribe...") is actually true.
Also extract the Reconnecting auth-loss decision into a static internal
method so the auth-stop behavior can be unit-tested without a real
HubConnection or full DI graph. Add two unit tests covering token-present
(no-op) and token-null (evict + stop) branches.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously OnAuthenticationChanged only invalidated the cached project list — the hub connection stayed up with whatever token it was built with. After a logout/login cycle the listener kept running with stale auth until the next reconnect attempt happened to detect it. Track each project we're asked to listen for in a stable dictionary (separate from the per-connection _reconnectProjects table) so we have a list to re-subscribe even after the underlying HubConnection has been torn down. On every auth event, evict the cached connection and walk that list to (re)start listeners; on logout the new connections fail fast at the auth check, on login they come up with the fresh token — auto-recovery without waiting for the user to manually sync or reopen the project. EvictAndStopIfCached is extracted as a static internal helper to match the HandleReconnecting pattern; add two unit tests for its empty-cache and cached-connection branches. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
If the initial StartLexboxProjectChangeListener call fails (e.g. user was offline at project-open), the WithAutomaticReconnect policy never kicks in — the listener stays dead until something re-triggers ListenForProjectChanges. Today the only triggers are project open and the FwLiteWeb hub's OnConnectedAsync; a long-running session can sit indefinitely with no push notifications. Fire-and-forget a ListenForProjectChanges call after every successful SyncService.ExecuteSync. If the listener is already running the cache short-circuits and it's effectively free; if it died (cold-start failure, transient network) this is the recovery trigger. Wrap in try/catch so a listener problem can't masquerade as a sync failure. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When two ListenForProjectChanges callers race past the cache check (both see it empty), both build a HubConnection. The second wins the cache slot; the first is evicted via the IMemoryCache eviction callback and disposed. Disposing fires its Closed event handler, which unconditionally called cache.Remove — evicting the *replacement* connection that just took its place. The next ListenForProjectChanges call finds an empty cache and races again. The race is exposed (not caused) by the post-sync piggyback firing ListenForProjectChanges on every successful sync — observed during two-instance manual testing as a cascade where the cache stayed empty and connections kept cycling. Guard the cache eviction with a ReferenceEquals check so an orphaned Closed event can't take down the live cached connection. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Dispose the HubConnection when StartAsync fails (was leaked on cold-start retry) - Evict a dead cached connection if Disconnected-recovery StartAsync fails - Broaden the subscribe SendAsync catch so a mid-call drop can't fail project-open - Extract RegisterForResubscribe and unit-test the subscription-ordering invariant - HandleReconnecting takes a bool; null token = logged out (OAuthClient owns transient-vs-logout) - MAUI: ConnectivitySyncTrigger re-establishes listeners when connectivity returns Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PushListenerRecoveryService re-runs EnsureListenersForTrackedProjects every 5 minutes so a listener that failed to start (e.g. offline at project-open) recovers without a manual sync, on platforms with no connectivity events (FwLiteWeb). Servers with a live connection are skipped via a pre-loop snapshot so the periodic pass is a no-op once connected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… recovery Two paths could leave sibling projects silently unsubscribed while their server reported Connected: a manual revive (StartAsync does not raise Reconnected) resubscribed only the requesting project, and the recovery pass skipped whole servers, so a connection rebuilt by a single-project path (post-sync, project-open) never picked the siblings back up. Extract the Reconnected handler body into ResubscribeRegisteredProjects, call it after a successful revive, and make EnsureListenersForTrackedProjects skip per project (connected AND registered) instead of per server. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
HandleReconnecting removed the cache entry by key, and the revive-failure path evicted whatever was cached — either could race a rebuild and evict a live replacement connection, orphaning it while callers believe the server has no listener. Apply the same ReferenceEquals guard the Closed handler uses: HandleReconnecting only evicts its own entry (still stops itself — it has no token), and EvictAndStopIfCached takes an optional expected instance for callers holding a specific dead connection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Concurrent ListenForProjectChanges callers that both missed the cache each built and started a connection; the cache replacement then disposed the loser mid-use and silently discarded its resubscribe registrations. Concurrent revives of the same Disconnected connection were worse: the losing StartAsync threw and evicted the winner. A per-server gate around build-and-cache and revive (with re-checks inside) leaves one connection per server by construction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The HubConnection logged only to its own console-only factory, so reconnect attempts and close reasons never reached the app log file -- production logs could not answer why a connection died. Register the app logger factory in the builder''s service collection so SignalR client logs flow through the app''s providers and filters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An OS-frozen app (Android Doze) can return to the foreground with its push listener down and no connectivity transition for the connectivity trigger to react to, leaving recovery to the 5-minute periodic backstop. Resume is exactly when the user is watching, so recover immediately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fixed backoff topped out at 60s, and since SignalR cannot be hurried mid-delay, that was the worst-case window where HTTP already works but pushes are still down (e.g. after a server deploy, where no client-side signal exists to trigger recovery). Retry every 10s while the device reports online -- the failure is server-side and could end any moment -- and back off to 60s while offline, where every attempt is a pointless local failure. Hosts without a connectivity API (FwLiteWeb) report always-online. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A connection sleeping in its reconnect backoff cannot be hurried -- every recovery mechanism found it Reconnecting and correctly stood down, so the committed delay was the floor on recovery latency even when the network was demonstrably back. Callers with strong evidence (connectivity regained, app resume, a successful sync to that server) now stop the mid-backoff connection and rebuild through the normal serialized path, which also resubscribes every tracked project. Weak-signal callers (the periodic backstop) never kick: a wrong kick trades the automatic retry loop for the much slower backstops. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ndler Remove the StartAsync retry TODO (post-sync, connectivity, resume and periodic recovery now cover it), fix comments referencing the renamed retry policy, and catch TriggerSync failures in the OnProjectUpdated handler -- it throws when the background sync service is not running and SignalR would swallow that into its own logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Reconnecting handler decided "logged out" from whether a token could be fetched right then -- but reconnecting happens precisely when the network is flaky, and fetching a token can require a refresh round-trip that fails transiently, which read as a false logout and tore down a listener that should have kept retrying. Switch the signal to IsSignedIn (a local-only account-presence read that flips only on a real logout), now available from the merged OAuth fixes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move auth-change listener recovery out of LexboxProjectService into a dedicated AuthChangeListenerTrigger hosted service, so the subscription lifecycle no longer rides on the service's IDisposable. Report connectivity from the OS network-interface table (NetworkInterfaceNetworkStatus) rather than assuming always-online, and add NetworkChangeSyncTrigger so web hosts re-ensure listeners when connectivity returns (the cross-platform counterpart to MAUI's ConnectivitySyncTrigger). StartLexboxProjectChangeListener now checks the connection cache before any auth call and gates a new build on IsSignedIn (account presence) instead of GetCurrentToken: a transient token-refresh failure during a reconnect could otherwise be mistaken for a logout and tear down a healthy auto-reconnecting connection (the same false-logout trap HandleReconnecting already avoids). Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Each recovery path now announces itself: the auth-change, MAUI-connectivity and web-network-availability triggers log at startup that they're watching, the MAUI resume hook logs when it fires, and the periodic backstop logs a debug heartbeat per check. The connectivity/network triggers already logged their fire events; auth-change fire detail stays in HandleAuthChanged. Makes it easier to tell which of the several recovery paths did (or didn't) run when debugging listener issues. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ne verdict A health check that failed while offline was cached as unhealthy for 30 minutes, so a sync triggered by network recovery (this branch's reconnect-sync) returned the stale "unhealthy" verdict and refused to sync until the cache expired or the app restarted. On a genuine connect transition — where SignalR reaching the server proves it's reachable — drop the cached health verdict (and the per-server projects list, which has the same stale-offline-failure problem) so the catch-up sync re-probes. Also quiet the expected ObjectDisposedException when stopping an evicted connection that's mid-reconnect (it has already disposed itself; logged at Debug), and downgrade the per-attempt reconnect log to Debug. Verified on Android: cold-start-while-offline then online now syncs within seconds, where it previously stayed "Offline" with pending downloads for the full cache window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….user Reframe the connectivity-change log (was a temporary diagnostic) as a permanent event so real-device behaviour stays legible, and gitignore the local-harmony *.props.user artifact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rror When the device was offline the sync indicator/dialog stayed green and a manual sync surfaced a raw UnknownHostException: the server-health cache can still report healthy from an earlier online sync, so ExecuteSync sailed past the ShouldSync gate and the actual transfer threw — which bubbled to the UI and skipped the offline status update. - ExecuteSync now trusts the device's network state: if offline, report Offline and return without attempting the transfer; also catch a mid-sync HttpRequestException and map it to Offline. Either path fires an Offline sync event, so the indicator dot and the sync dialog reflect offline. - SyncDialog handles an un-synced result gracefully (gentle "you're offline" toast, no optimistic zeroing of pending counts) rather than letting it reach the global error handler. Verified on Android: an offline sync now shows "Offline" + cloud-off with a friendly toast; going back online still syncs normally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Widen the ExecuteSync catch to OperationCanceledException so an HttpClient timeout reads as Offline instead of leaking a raw error. ExecuteSync takes no cancellation token, so an OCE there is always a transport timeout, never a caller abort. The CRDT sync dialog showed "You're offline" for every non-synced outcome (also NoServer/NotLoggedIn); use a status-neutral message and drop a dead null-guard. The persistent status indicator still shows the specific status. Re-extract catalogs and add translator context for the new string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ExecuteSync now reports offline/connectivity failures as an unsynced result instead of throwing, so UploadProject's catch no longer fired on a failed upload: the project was left bound to a server it never reached and the /upload route returned Ok(). Honor the unsynced result so the origin is rolled back and the failure surfaces, restoring the pre-offline-handling contract. Found by Devin review of #2317. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… too The PushListenerRecoveryService comment claimed the connectivity trigger was MAUI-only and that FwLiteWeb had none, but this branch added NetworkChangeSyncTrigger for exactly that. Correct it, and state the real reason the periodic backstop still matters: the OS network signal is optimistic (a virtual adapter reads "available"), so a real-uplink return may not fire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…terface and update dependencies across the project.
…nForProjectChanges
github-actions
Bot
added
💻 FW Lite
|
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:
📝 WalkthroughWalkthroughAdds connectivity-aware push listener recovery to FwLite (MAUI and Web). Introduces ChangesBackend: Connectivity-Aware Sync and SignalR Push Listener Recovery
Frontend: Sync Failure UX and i18n
Config and Tooling
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
# Conflicts: # .gitignore # backend/FwLite/FwLiteMaui/App.xaml.cs
Contributor
Contributor
C# FwHeadless Unit Tests48 tests 48 ✅ 16s ⏱️ Results for commit 1cb51ba. ♻️ This comment has been updated with latest results. |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
Contributor
C# Unit Tests165 tests 165 ✅ 20s ⏱️ Results for commit 1cb51ba. ♻️ This comment has been updated with latest results. |
![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 (3)
backend/FwLite/FwLiteShared/Utils/ConcurrentHashSet.cs (1)
1-5: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winScope this utility to a project namespace and internal visibility.
ConcurrentHashSet<T>is currently global andpublic. For this PR usage, keeping it namespaced (e.g.,FwLiteShared.Utils) andinternalavoids accidental API surface growth and type-name collisions.Proposed refactor
+namespace FwLiteShared.Utils; + -public class ConcurrentHashSet<T> : IEnumerable<T> where T : notnull +internal class ConcurrentHashSet<T> : IEnumerable<T> where T : notnull🤖 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/Utils/ConcurrentHashSet.cs` around lines 1 - 5, The ConcurrentHashSet<T> class is currently defined without a namespace and with public visibility, which creates an unnecessarily broad API surface and risks type-name collisions. Add a namespace declaration wrapping the ConcurrentHashSet<T> class in FwLiteShared.Utils, and change the class access modifier from public to internal to restrict its visibility to the assembly and prevent unintended external usage.backend/FwLite/FwLiteShared/Projects/LexboxProjectChangeListener.cs (2)
9-11: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider implementing
IAsyncDisposableto clean up SignalR connections on shutdown.
LexboxHubConnectionimplementsIAsyncDisposable, but this class holds connections in aConcurrentDictionarywithout a disposal mechanism. While the DI container disposes singletons, it cannot reach into the dictionary. For graceful shutdown (avoiding server-side connection leaks), consider implementingIAsyncDisposable:♻️ Example disposal implementation
-public sealed class LexboxProjectChangeListener(IServiceProvider serviceProvider, IOptions<AuthConfig> options) +public sealed class LexboxProjectChangeListener(IServiceProvider serviceProvider, IOptions<AuthConfig> options) : IAsyncDisposable { private readonly ConcurrentDictionary<LexboxServer, LexboxHubConnection> _connections = new(); + public async ValueTask DisposeAsync() + { + foreach (var connection in _connections.Values) + { + await connection.DisposeAsync(); + } + _connections.Clear(); + }🤖 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/Projects/LexboxProjectChangeListener.cs` around lines 9 - 11, The LexboxProjectChangeListener class holds LexboxHubConnection objects in the _connections ConcurrentDictionary but has no disposal mechanism to clean them up on shutdown. Implement the IAsyncDisposable interface on LexboxProjectChangeListener and add a DisposeAsync method that iterates through all LexboxHubConnection instances in the _connections dictionary and calls DisposeAsync on each one to ensure proper cleanup of SignalR connections during graceful shutdown.
29-35: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPotential orphaned connections from
GetOrAddrace condition.
ConcurrentDictionary.GetOrAddcan invoke the factory delegate multiple times when concurrent threads access the same key simultaneously. Only oneLexboxHubConnectionis stored, but others may be created and orphaned without disposal, leaking SignalR connections.Consider using
Lazy<T>or a per-key lock/semaphore to ensure only one connection is ever created per server:♻️ Example fix using Lazy<T> pattern
- private readonly ConcurrentDictionary<LexboxServer, LexboxHubConnection> _connections = new(); + private readonly ConcurrentDictionary<LexboxServer, Lazy<LexboxHubConnection>> _connections = new(); // ... public async Task ListenForProjectChanges(ProjectData projectData, CancellationToken stoppingToken = default, bool kickReconnecting = false) { if (!options.Value.TryGetServer(projectData, out var server)) return; - var connection = _connections.GetOrAdd(server, - (s) => ActivatorUtilities.CreateInstance<LexboxHubConnection>(serviceProvider, s)); + var connection = _connections.GetOrAdd(server, + (s) => new Lazy<LexboxHubConnection>( + () => ActivatorUtilities.CreateInstance<LexboxHubConnection>(serviceProvider, s))).Value; await connection.ListenForProjectChanges(projectData.Id, stoppingToken, kickReconnecting); }Adjust
HandleAuthChangedandEnsureListenersForTrackedProjectssimilarly to access.Value.🤖 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/Projects/LexboxProjectChangeListener.cs` around lines 29 - 35, The _connections ConcurrentDictionary uses GetOrAdd with a factory that can create multiple LexboxHubConnection instances concurrently, leaving orphaned connections. Replace the direct LexboxHubConnection instantiation with a Lazy<LexboxHubConnection> wrapper in the GetOrAdd factory delegate so that only one connection is created per server key. Update the call to connection.ListenForProjectChanges to access the lazy value first (connection.Value), and apply the same Lazy<T> pattern to any other methods that access _connections, such as HandleAuthChanged and EnsureListenersForTrackedProjects, ensuring they also unwrap the lazy value before use.
🤖 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/FwLiteShared.Tests/Projects/LexboxHubConnectionTests.cs`:
- Around line 362-391: The test method
EnsureConnected_WhenAlreadyConnected_DoesNotTriggerCatchupSyncAgain hardcodes
the server health cache key as "ServerHealth|" + Server.Authority.Authority
instead of using the CrdtHttpSyncService.HealthCacheKey helper method. Replace
both occurrences of this hardcoded cache key string (where serverHealthCacheKey
is assigned and set in the test) with calls to
CrdtHttpSyncService.HealthCacheKey(Server.Authority.Authority) to maintain
consistency with the actual cache key generation logic and ensure the test
remains correct if the key format changes.
In `@backend/FwLite/FwLiteShared/Projects/LexboxHubConnection.cs`:
- Around line 72-74: The OnConnected method is being invoked unconditionally
after NewConnection returns, but NewConnection can return null when the user is
signed out (on the signed-out path). Add a null check on the connection variable
to guard the OnConnected invocation, ensuring it is only called when
NewConnection successfully establishes a connection (when connection is not
null). This same guard pattern should be applied to all locations where
OnConnected is invoked after NewConnection, including the additional call sites
at lines 88-95 and 126-130.
- Around line 52-67: The restart logic that checks
HubConnectionState.Disconnected and calls StartAsync, OnConnected, and
CleanupConnection is executing before acquiring the connectionLock, creating a
race condition where concurrent callers can interfere with each other's restart
and cleanup operations. Move the await connectionLock.WaitAsync(stoppingToken)
call to before the if statement that checks the connection state, so that the
entire restart operation is protected by the lock from start to finish.
---
Nitpick comments:
In `@backend/FwLite/FwLiteShared/Projects/LexboxProjectChangeListener.cs`:
- Around line 9-11: The LexboxProjectChangeListener class holds
LexboxHubConnection objects in the _connections ConcurrentDictionary but has no
disposal mechanism to clean them up on shutdown. Implement the IAsyncDisposable
interface on LexboxProjectChangeListener and add a DisposeAsync method that
iterates through all LexboxHubConnection instances in the _connections
dictionary and calls DisposeAsync on each one to ensure proper cleanup of
SignalR connections during graceful shutdown.
- Around line 29-35: The _connections ConcurrentDictionary uses GetOrAdd with a
factory that can create multiple LexboxHubConnection instances concurrently,
leaving orphaned connections. Replace the direct LexboxHubConnection
instantiation with a Lazy<LexboxHubConnection> wrapper in the GetOrAdd factory
delegate so that only one connection is created per server key. Update the call
to connection.ListenForProjectChanges to access the lazy value first
(connection.Value), and apply the same Lazy<T> pattern to any other methods that
access _connections, such as HandleAuthChanged and
EnsureListenersForTrackedProjects, ensuring they also unwrap the lazy value
before use.
In `@backend/FwLite/FwLiteShared/Utils/ConcurrentHashSet.cs`:
- Around line 1-5: The ConcurrentHashSet<T> class is currently defined without a
namespace and with public visibility, which creates an unnecessarily broad API
surface and risks type-name collisions. Add a namespace declaration wrapping the
ConcurrentHashSet<T> class in FwLiteShared.Utils, and change the class access
modifier from public to internal to restrict its visibility to the assembly and
prevent unintended external usage.
🪄 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: 199d8105-b611-4890-a3dc-074473eb0449
📒 Files selected for processing (37)
.gitignorebackend/.editorconfigbackend/FwLite/FwLiteMaui.Tests/ConnectivitySyncTriggerTests.csbackend/FwLite/FwLiteMaui/App.xaml.csbackend/FwLite/FwLiteMaui/FwLiteMauiKernel.csbackend/FwLite/FwLiteMaui/Services/ConnectivityNetworkStatus.csbackend/FwLite/FwLiteMaui/Services/ConnectivitySyncTrigger.csbackend/FwLite/FwLiteShared.Tests/Projects/AdaptiveRetryPolicyTests.csbackend/FwLite/FwLiteShared.Tests/Projects/LexboxHubConnectionTests.csbackend/FwLite/FwLiteShared/FwLiteSharedKernel.csbackend/FwLite/FwLiteShared/Projects/AuthChangeListenerTrigger.csbackend/FwLite/FwLiteShared/Projects/LexboxHubConnection.csbackend/FwLite/FwLiteShared/Projects/LexboxProjectChangeListener.csbackend/FwLite/FwLiteShared/Projects/LexboxProjectService.csbackend/FwLite/FwLiteShared/Projects/PushListenerRecoveryService.csbackend/FwLite/FwLiteShared/Properties/AssemblyInfo.csbackend/FwLite/FwLiteShared/Services/INetworkStatus.csbackend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.csbackend/FwLite/FwLiteShared/Services/NetworkInterfaceNetworkStatus.csbackend/FwLite/FwLiteShared/Sync/BackgroundSyncService.csbackend/FwLite/FwLiteShared/Sync/IBackgroundSyncService.csbackend/FwLite/FwLiteShared/Sync/SyncService.csbackend/FwLite/FwLiteShared/Utils/ConcurrentHashSet.csbackend/FwLite/FwLiteWeb/FwLiteWebKernel.csbackend/FwLite/FwLiteWeb/Hubs/CrdtMiniLcmApiHub.csbackend/FwLite/FwLiteWeb/Services/NetworkChangeSyncTrigger.csbackend/FwLite/LcmCrdt.Tests/CrdtHttpSyncServiceTests.csbackend/FwLite/LcmCrdt/RemoteSync/CrdtHttpSyncService.csfrontend/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/SyncDialog.svelte
…l OnConnected if the connection is null
myieye
requested changes
Jun 22, 2026
myieye
left a comment
myieye
left a comment
Collaborator
There was a problem hiding this comment.
I pushed a commit, because LexboxHubConnection couldn't activate.
It seems to be behaving well and I recognize the concepts in play. 👍
I guess the deadlock needs a fix and the InternalsVisibleTo in the assembly file seems redundant to me.
|
|
||
| namespace FwLiteShared.Projects; | ||
|
|
||
| public sealed class LexboxProjectChangeListener(IServiceProvider serviceProvider, IOptions<AuthConfig> options) : IAsyncDisposable |
Collaborator
There was a problem hiding this comment.
This class is practically a base class or component of LexboxProjectService. That's the only class using it and HandleOnChanged is supposed to go through LexboxProjectService, rather than come directly to this class so that cache management can happen. I just wonder if this class should be less global.
But whatever you prefer. Just having it registered as a global singleton is probably fine.
Collaborator
Author
There was a problem hiding this comment.
That's a good point. I've gone ahead and changed most services to just use LexboxProjectChangeListener, a couple classes which were using LexboxProjectService were using it's other methods, and just ListenForProjectChanges so I've left those in place to keep the changes smaller, but most of the new classes only needed the methods in the change listener.
As for HandleAuthChange (which I think you were referring to above), I would probably prefer to just remove AuthChangeListenerTrigger and make both the project service and the change listener subscribe directly to the global event bus, the whole point is that any class can listen to events that it cares about, so having a central place to distribute notifications kinda goes against that design. We could easily meet in the middle and just modify the Auth listener trigger to call HandleAuthChange on the change listener and no longer make the project service notify the change listener.
Collaborator
There was a problem hiding this comment.
I agree. I think we should drop the AuthChangeListenerTrigger I added.
I guess I forced that into the same pattern as ConnectivitySyncTrigger and NetworkChangeSyncTrigger, which...perhaps also shouldn't be "triggers", but rather expose a simple "CameOnline" event that e.g. LexboxProjectChangeListener can subscribe to.
I'll let you decide what you want to do there.
Collaborator
Author
|
@myieye you said you pushed a commit but I don't see it, I went ahead and fixed the issue I assume you were seeing, I didn't realize that I never actually tested a real app, oops. |
And don't let platform connectivity predictions block syncing
Collaborator
|
@hahn-kev You're right, I never pushed the commit I mentioned. Oops. But, I just pushed a commit now: That button produces a terrible error toast if you're offline, but so does the log in button on the home page. |
myieye
approved these changes
Jun 23, 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
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.
Builds off of #2317
I've introduced 2 new classes.
LexboxHubConnection
Owns the SignlaR connection and recreates/reconnects as needed. Also owns the listening project Ids and dispatching sync notifications.
LexboxProjectChangeListener
Owns the
LexboxHubConnection, creates it based on the server and reuses it for future requests.Note
Right now the connection takes a dependency on the sync service and triggers the sync itself. It might be better have that be handled in the
LexboxProjectChangeListenerby the connection returning an observable and the change listener deciding what to do in response to a project change notification. Something to consider for this PR or the future.