diff --git a/src/DodoSSH.Client.Shell/ViewModels/TeamsViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TeamsViewModel.cs index 83ae25c..d9088e5 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TeamsViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TeamsViewModel.cs @@ -281,6 +281,20 @@ internal sealed partial class TeamsViewModel( [ObservableProperty] private TeamActionRequest? pendingAction; + /// Set while reselects, so the handler does not read as well. + private bool isReselecting; + + /// + /// Which selection read owns the lists below the team list. + /// + /// + /// Selecting a second team before the first one's read has answered leaves two reads in flight + /// against the same collections, and the one that started first can answer last — so the + /// superseded read drops its answer instead of appending another team's members to the list. UI + /// thread only, which is where every selection change and every continuation on this screen runs. + /// + private int selectionGeneration; + /// Whether there is a server to talk to at all. internal bool IsOnline => connection() is not null; @@ -367,8 +381,22 @@ internal sealed partial class TeamsViewModel( Teams.Add(new TeamRowViewModel(team)); } - SelectedTeam = - Teams.FirstOrDefault(row => row.TeamId == selectedId) ?? Teams.FirstOrDefault(); + // The assignment reselects the same team through a new row object, so the selection handler + // would start its own read of the very lists this method is about to read — two reads + // clearing and then appending into the same collections, which draws every member, invitation + // and vault twice. Suppressed rather than deduplicated, because the read below is awaited and + // the handler's is not: this is the one that has to be the reload's. + isReselecting = true; + + try + { + SelectedTeam = + Teams.FirstOrDefault(row => row.TeamId == selectedId) ?? Teams.FirstOrDefault(); + } + finally + { + isReselecting = false; + } RaiseState(); @@ -892,6 +920,11 @@ internal sealed partial class TeamsViewModel( PendingAction = null; IsEditingTeam = false; + if (isReselecting) + { + return; + } + // Fire-and-forget on purpose, and the only place in this class that is: selection changes come // from a list box, which has no cancellation token and no way to await. Failures land in Status // through RunAsync exactly as a command's would. @@ -947,6 +980,8 @@ internal sealed partial class TeamsViewModel( /// Reads the selected team's members, invitations and vaults. private async Task LoadSelectedAsync(CancellationToken cancellationToken) { + var generation = ++selectionGeneration; + Members.Clear(); Invitations.Clear(); Vaults.Clear(); @@ -964,6 +999,11 @@ internal sealed partial class TeamsViewModel( .ListTeamMembersAsync(team.TeamId, cancellationToken) .ConfigureAwait(true); + if (generation != selectionGeneration) + { + return; + } + foreach (var member in members) { Members.Add(new TeamMemberRowViewModel(member, member.UserId == selfId)); @@ -973,6 +1013,11 @@ internal sealed partial class TeamsViewModel( .ListTeamInvitationsAsync(team.TeamId, cancellationToken) .ConfigureAwait(true); + if (generation != selectionGeneration) + { + return; + } + foreach (var invitation in invitations) { Invitations.Add(new TeamInvitationRowViewModel(invitation)); @@ -982,17 +1027,24 @@ internal sealed partial class TeamsViewModel( OnPropertyChanged(nameof(HasInvitations)); - if (open is null) + if (open is not null) { - return; + ListVaults(open, team.TeamId); } + } - // Read from the session rather than from a team-vaults endpoint, because the interesting fact - // about a team vault here is whether *this* machine can open it — which is a property of the - // keyring and not something the server can answer. + /// Fills the vault list for a team, from what this machine can see. + /// + /// Read from the session rather than from a team-vaults endpoint, because the interesting fact + /// about a team vault here is whether this machine can open it — which is a property of the + /// keyring and not something the server can answer. No await, so it needs no generation guard: it + /// runs to completion inside the read that called it. + /// + private void ListVaults(VaultSession open, Guid teamId) + { var readable = open.ReadableVaults.Select(vault => vault.VaultId).ToHashSet(); - foreach (var vault in open.Vaults.Where(vault => vault.TeamId == team.TeamId)) + foreach (var vault in open.Vaults.Where(vault => vault.TeamId == teamId)) { Vaults.Add(new TeamVaultRowViewModel( vault.VaultId, vault.Name, readable.Contains(vault.VaultId), vault.RekeyRequired)); diff --git a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.Teams.cs b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.Teams.cs index 7feff83..3be6758 100644 --- a/tests/DodoSSH.Client.App.Tests/FakeVaultServer.Teams.cs +++ b/tests/DodoSSH.Client.App.Tests/FakeVaultServer.Teams.cs @@ -212,12 +212,34 @@ internal sealed partial class FakeVaultServer : ITeamApi, IDirectoryApi, IVaultG return Task.CompletedTask; } + /// + /// When set, a member read waits on it before answering. + /// + /// + /// Every other method here answers from memory and therefore completes before its caller's await + /// ever suspends, which hides anything the screen only gets wrong while a read is in flight — the + /// state a real server leaves it in for the length of a round trip. A test that wants that state + /// holds the gate. + /// + internal TaskCompletionSource? MemberReadGate { get; set; } + + /// How many member reads have been asked for, for a test to assert on. + internal int MemberReads { get; private set; } + /// - public Task> ListTeamMembersAsync( + public async Task> ListTeamMembersAsync( Guid teamId, - CancellationToken cancellationToken) => - Task.FromResult>( - members.TryGetValue(teamId, out var list) ? [.. list] : []); + CancellationToken cancellationToken) + { + MemberReads++; + + if (MemberReadGate is { } gate) + { + await gate.Task.WaitAsync(cancellationToken).ConfigureAwait(false); + } + + return members.TryGetValue(teamId, out var list) ? [.. list] : []; + } /// public Task AddTeamMemberAsync( diff --git a/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs b/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs index c2901ce..bf68b83 100644 --- a/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/TeamSharingTests.cs @@ -527,6 +527,50 @@ public sealed class TeamSharingTests : IAsyncLifetime teams.Status.ShouldContain("Withdrew the invitation"); } + /// + /// + /// A reload rebuilds the team list and reselects, so a reload that changed the selection — creating + /// the first team is exactly that — used to leave two reads of the same team in flight: the one the + /// reload awaits, and one the selection handler started on its own. Both clear the member list and + /// then both append to it, so every member was drawn twice. On a team nobody has been added to yet, + /// whose only member is its owner, that read as the owner being in the team twice. + /// + /// + /// Counted rather than inferred from the list, and the gate is why: against a fake that answers from + /// memory each read finishes before the next begins, so the duplicate never appears and the bug + /// survives the test. Holding the read open is what makes this behave like a server. + /// + /// + [Fact] + public async Task CreatingATeam_ReadsItsMembersOnce() + { + await UnlockedAsync(); + + var teams = shell.Teams; + + await teams.LoadAsync(Token); + + teams.NewTeamCommand.Execute(null); + teams.NewTeamName = "Platform"; + teams.NewTeamSlug = "platform"; + + var gate = new TaskCompletionSource(); + + server.MemberReadGate = gate; + + var create = teams.CreateTeamCommand.ExecuteAsync(null); + + // Asserted while the read is still in flight: that is the only moment at which a second read + // started by the selection handler is distinguishable from the reload's own. + server.MemberReads.ShouldBe(1, "a reload reads the selected team's members once"); + + gate.SetResult(); + + await create; + + teams.Members.ShouldHaveSingleItem().Role.ShouldBe("OWNER"); + } + private async Task CreateTeamAsync(TeamsViewModel teams, string name, string slug) { await teams.LoadAsync(Token);