You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Staging PR — never merge; promoted to sillsdev when polished (see FORK.md).
Supersedes sillsdev#2490 per hahn-kev's review: instead of scattered UpdateLastUser writes (listing, every sync, project open), login is now the write point. Resolvessillsdev#2491.
New ProjectServerInfoService listens for auth changes and stamps the signed-in user + roles (from the server project list) onto the local projects originating from that server — covers login from anywhere, including the sync dialog.
The listing path and post-upload path route through the same service; project open self-heals the user from the local MSAL cache (crash between login and stamping, project file copied from another machine).
Sync no longer writes user state; comment read-status attribution reads the MSAL cache directly.
GetLexboxProjects returns null on failure instead of caching an empty list, and cache entries are tied to the identity they were fetched under.
A project missing from the server list keeps its stored role (admins download projects they aren't members of; a server-side delete shouldn't strand unpushed edits as readonly) — unless the user changed, then the old user's role isn't trusted.
New ProjectDataChangedEvent pushes fresh ProjectData to the open viewer, so comment ownership/role update live after a mid-session login.
Carries over from sillsdev#2490: origin-scoped stamping (same GUID on two servers can't clobber), OAuthClient.GetCachedUser + the accounts helper, and the review-comment fixes (person-name test usernames).
ProjectData.LastUser* and Role were written from scattered places (project
listing, every sync, PR sillsdev#2490 would have added project open), each covering
a different subset of cases and none covering login from the sync dialog.
Per review feedback on sillsdev#2490, make login the write point: a new
ProjectServerInfoService listens for auth changes and stamps the signed-in
user and their roles onto the local projects originating from that server.
The listing path and post-upload path route through the same service, and
project open self-heals the user from the local MSAL cache (covers a crash
between login and stamping, or a project file copied from another machine).
Also:
- SyncService no longer writes user state; comment read-status attribution
reads the MSAL cache directly, so it's right even when a login raced the
stamping.
- GetLexboxProjects returns null on failure (instead of caching an empty
list for 5 minutes) and ties cache entries to the identity they were
fetched under.
- A project missing from the server list keeps its stored role (admins can
download projects they aren't members of), unless the signed-in user
changed, in which case the previous user's role isn't trusted.
- A new ProjectDataChangedEvent pushes fresh ProjectData to the open
viewer, so comment ownership and role update live after a mid-session
login instead of only at reopen.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98026bcd-8926-4bc8-a356-1ec753c16f65
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
🔍 Trigger review
📝 Walkthrough
Walkthrough
Changes
The PR centralizes cached authentication reads, adds identity-aware project caching and server metadata refresh, updates project role synchronization, introduces project-data-changed events, and applies those events to the viewer’s project context.
Cached MSAL account access is centralized and tested for account identity and empty-account behavior.
Project roles and identity-aware project caching backend/FwLite/FwLiteShared/Projects/*, backend/FwLite/LcmCrdt/CrdtProjectsService.cs, backend/FwLite/LcmCrdt.Tests/Project/*
Role conversions, per-user project-list caching, nullable fetch results, and persisted user/role update behavior are implemented and tested.
Server metadata refresh and role resolution backend/FwLite/FwLiteShared/Projects/ProjectServerInfoService.cs, backend/FwLite/FwLiteShared/Projects/CombinedProjectsService.cs, backend/FwLite/FwLiteShared/FwLiteSharedKernel.cs, backend/FwLite/FwLiteShared.Tests/Projects/*
ProjectServerInfoService refreshes server-owned projects, resolves roles, updates local metadata, publishes events, and is registered as a hosted singleton service.
Project opening and sync integration backend/FwLite/FwLiteShared/Services/ProjectServicesProvider.cs, backend/FwLite/FwLiteShared/Sync/SyncService.cs
Project opening reconciles cached users, while synchronization refreshes server metadata and uses cached user identity for comment read-status updates.
We reviewed changes in e221be6...a3350ad on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable
It is considered a best practice to avoid 'polluting' the global scope with variables that are intended to be local to the script. Global variables created from a script can produce name collisions with global variables created from another script, which will usually lead to runtime errors or unexpected behavior. It is mostly useful for browser scripts.
The reason will be displayed to describe this comment to others. Learn more.
Method with return type `Task` does not follow the naming convention
The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.
The reason will be displayed to describe this comment to others. Learn more.
Method with return type `Task` does not follow the naming convention
The consensus in .NET is to have names of methods dealing with asynchronous operations suffixed with Async. One such example is Stream.ReadAsync from System.IO. Doing so improves readability and provides crucial information at a glance.
The reason will be displayed to describe this comment to others. Learn more.
Nullable boolean expression can be simplified
While the null coalescing operator may come in handy when evaluating the bool? types, it is recommended that you stick to the traditional equality operators and explicit boolean values — an approach that is more readable and easier to comprehend.
Avoids the nullable-bool evaluation DeepSource flagged (CS-R1118) and
reads better than threading null through the model mapping.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Devin caught a hole: ResolveOriginUser swapped LastUserId to the freshly
resolved user but kept the stored role, and once the id is swapped the
login path's trust-neither rule can never fire for that project. A project
file copied from another machine would keep the other user's permissions
indefinitely. Apply the same rule at open time and fetch the real role in
the background, which reaches the open viewer via ProjectDataChangedEvent.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 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/Events/JsEventListener.cs`:
- Around line 17-20: Ensure the JsEventListener path does not lose
ProjectDataChangedEvent updates when the bounded queue reaches
MaxJsEventQueueSize. Make delivery lossless or coalesce pending project updates,
or have the viewer’s onProjectDataChanged flow rehydrate the latest project data
when attaching, while preserving unrelated Sync/EntriesChanged events.
In `@backend/FwLite/FwLiteShared/Projects/LexboxProjectService.cs`:
- Around line 57-75: Update GetLexboxProjects and its consumer flow to preserve
the user identity captured before FetchLexboxProjects across the await. Before
ApplyServerInfo applies roles, verify the returned project list still belongs to
the current cached user; otherwise reject it or propagate the captured UserId so
roles cannot be applied to a different account, while preserving valid cache
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76958881-a2b3-4578-b161-b73871d5483e
📥 Commits
Reviewing files that changed from the base of the PR and between e221be6 and 6c19717.
Avoid dropping ProjectDataChangedEvent updates.TryWrite still drops events when the bounded queue fills, and the viewer’s onProjectDataChanged path consumes only the live stream, so a login burst can leave the open project with stale user/role data. Make this path lossless/coalesced, or rehydrate the latest project data when the listener attaches.
🤖 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/Events/JsEventListener.cs` around lines 17 - 20,
Ensure the JsEventListener path does not lose ProjectDataChangedEvent updates
when the bounded queue reaches MaxJsEventQueueSize. Make delivery lossless or
coalesce pending project updates, or have the viewer’s onProjectDataChanged flow
rehydrate the latest project data when attaching, while preserving unrelated
Sync/EntriesChanged events.
The reason will be displayed to describe this comment to others. Learn more.
[Claude, autonomous]
Fixed in a3350ad: the channel now uses BoundedChannelFullMode.DropOldest, so the newest event always lands — state-typed events (sync status, project data) are superseded by later ones, meaning the final state gets through even under pressure. Full losslessness isn't needed for correctness here: the persisted ProjectData is the source of truth and the open project re-reads it at open.
@myieye, thanks — DropOldest is an improvement because it avoids rejecting the newly written event when the queue is full. However, the finding remains valid for an already-open viewer.
A ProjectDataChangedEvent can still be evicted by any subsequent 64 events before NextEventAsync() reads it. The persisted-data reread only fixes state on a later project open; it does not repair an open viewer whose only update path is the live event stream:
ProjectEventBus.onProjectDataChanged() (Lines 154-160 in frontend/viewer/src/lib/services/event-bus.ts) has no latest-event replay.
DotnetProjectView.svelte applies updates after load only upon receiving that live event (Lines 38-40).
Please retain or reintroduce a per-project rehydration/latest-state mechanism—for example, cache/replay the latest ProjectDataChangedEvent per project when subscribing, or coalesce pending project-data events by project—so unrelated bursts cannot leave a currently open project with stale user/role data.
…overflow
Two CodeRabbit findings:
- GetLexboxProjects could return a list fetched under user A after B logged
in mid-fetch, letting ApplyServerInfo stamp B with A's roles. The result
now carries the identity it was fetched under and ApplyServerInfo treats
a mismatch as no fresh knowledge.
- The bounded JS event channel dropped the newest event when full, so the
last ProjectDataChangedEvent of a burst could be lost and leave the open
project stale. DropOldest guarantees the final state gets through.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
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.
[Claude, autonomous]
Staging PR — never merge; promoted to sillsdev when polished (see FORK.md).
Supersedes sillsdev#2490 per hahn-kev's review: instead of scattered
UpdateLastUserwrites (listing, every sync, project open), login is now the write point. Resolves sillsdev#2491.ProjectServerInfoServicelistens for auth changes and stamps the signed-in user + roles (from the server project list) onto the local projects originating from that server — covers login from anywhere, including the sync dialog.GetLexboxProjectsreturns null on failure instead of caching an empty list, and cache entries are tied to the identity they were fetched under.ProjectDataChangedEventpushes freshProjectDatato the open viewer, so comment ownership/role update live after a mid-session login.Carries over from sillsdev#2490: origin-scoped stamping (same GUID on two servers can't clobber),
OAuthClient.GetCachedUser+ the accounts helper, and the review-comment fixes (person-name test usernames).