Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Capacitor.App/Views/ChatTabView.axaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,18 @@ protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e)
() => (DataContext as ChatTabViewModel)?.Attachments, TimeProvider.System);
}

/// The window reuses this view for every session it opens, so the reading position belongs to
/// the session it was showing: the next one starts at the tail.
protected override void OnDataContextChanged(EventArgs e) {
DropToggleAnchor();
CancelSmoothScroll();
_thumbDragging = false;
_thumbFraction = null;
_readerGesture = false;
_followTail = true;
base.OnDataContextChanged(e);
}

protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e) {
DropToggleAnchor();
CancelSmoothScroll();
Expand Down
8 changes: 4 additions & 4 deletions src/Capacitor.App/Views/MainWindow.axaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,12 @@
<ContentControl x:Name="WorkspaceHost" Content="{Binding CurrentWorkspace}"
IsVisible="{Binding CurrentWorkspace, Converter={x:Static ObjectConverters.IsNotNull}}">
<ContentControl.DataTemplates>
<DataTemplate x:DataType="vm:WorkspaceViewModel">
<views:RetainedViewTemplate x:DataType="vm:WorkspaceViewModel">
<views:WorkspaceView />
</DataTemplate>
<DataTemplate x:DataType="vm:RemoteSessionViewModel">
</views:RetainedViewTemplate>
Comment on lines +145 to +147

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset per-session UI state before reusing the workspace view

When a user scrolls away from the bottom in session A, closes it, and opens session B, this template now returns the same entire WorkspaceView, including its ChatTabView. That control keeps session-specific fields such as _followTail and the existing scroll viewer offset, and has no DataContextChanged reset; consequently session B can open at an inherited scroll position and remain opted out of following new messages. Reset transient child-view state when the data context changes before reusing this control.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid — ChatTabView now resets follow-tail, the toggle anchor, thumb and smooth-scroll state in OnDataContextChanged, so the next session opens at the tail. A_new_session_in_the_same_view_opens_at_the_bottom_and_follows failed before the change. (A direct local-to-local switch already reused this view before this PR, so it fixes that path too.) 094a975

<views:RetainedViewTemplate x:DataType="vm:RemoteSessionViewModel">
<views:RemoteSessionView />
</DataTemplate>
</views:RetainedViewTemplate>
</ContentControl.DataTemplates>
</ContentControl>
</Panel>
Expand Down
23 changes: 23 additions & 0 deletions src/Capacitor.App/Views/RetainedViewTemplate.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
using Avalonia.Controls;
using Avalonia.Controls.Templates;
using Avalonia.Markup.Xaml.Templates;

namespace Capacitor.App.Views;

/// A data template that builds its view once and hands the same instance back on every later
/// build, including after the presenter has dropped it for null or another template's content.
/// On macOS a detached control stays reachable from any accessibility peer a client created for
/// it, so a template that builds afresh leaks the whole previous view on each rebuild.
public sealed class RetainedViewTemplate : DataTemplate, IRecyclingDataTemplate {
Control? _view;

Control? ITemplate<object?, Control?>.Build(object? param) => Build(param, null);

Control? IRecyclingDataTemplate.Build(object? data, Control? existing) => Build(data, existing);

new Control? Build(object? data, Control? existing) {
// Another presenter matching this template must not steal the view from the one showing it.
if (_view is { Parent: not null } && !ReferenceEquals(_view, existing)) return base.Build(data, null);
return _view ??= base.Build(data, null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

1. Closed sessions remain in memory until reopened 🐞 Bug ☼ Reliability

RetainedViewTemplate keeps the detached workspace control in _view, but neither the template nor
WorkspaceView clears its session DataContext when the control leaves the presenter. After
CloseWorkspace() sets CurrentWorkspace to null, the cached control can keep the closed workspace
model, its subscription, and lazily created panes reachable until another session replaces its
context.
Agent Prompt
## Issue description
The cached workspace view must not keep a closed session model or session-specific pane contents while it is detached.

## Fix Focus Areas
- src/Capacitor.App/Views/RetainedViewTemplate.cs[18-21]
- src/Capacitor.App/Views/WorkspaceView.axaml.cs[29-47]
- src/Capacitor.App/Views/RemoteSessionView.axaml.cs[25-48]

## Recommended Fix
On detachment, clear session-specific DataContext and contents and dispose subscriptions in both retained views. Ensure attaching a new session restores their subscriptions, and test that the cached view no longer references the closed model before reopening.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not reproduced: the view's DataContext is inherited from the presenter, so removing it from the presenter on close clears it, and WorkspaceView's DataContextChanged handler then disposes its subscription and drops the PR reader. Reopening_a_session_after_a_close_reuses_the_workspace_view now asserts the retained view's DataContext is null while closed. 094a975

}
}
5 changes: 3 additions & 2 deletions src/Capacitor.App/Views/TerminalClipboard.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ static bool IsPaste(KeyEventArgs e) =>
Application.Current?.PlatformSettings?.HotkeyConfiguration.Paste.Any(g => g.Matches(e)) == true;

internal static async Task PasteAsync(TerminalControl terminal) {
if (TopLevel.GetTopLevel(terminal)?.Clipboard is not { } clipboard) return;
if (TopLevel.GetTopLevel(terminal)?.Clipboard is not { } clipboard || terminal.Model is not { } model) return;
var text = await clipboard.TryGetTextAsync();
if (string.IsNullOrEmpty(text) || terminal.Model is not { } model) return;
// The control is reused across sessions: a paste outlived by its session goes nowhere.
if (string.IsNullOrEmpty(text) || !ReferenceEquals(terminal.Model, model)) return;
model.Send(TerminalInputEncoder.Paste(text));
}
}
28 changes: 28 additions & 0 deletions test/Capacitor.App.Tests.Unit/ChatTabViewSmokeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,34 @@ await RunOnUiAsync(async () => {
});
}

/// The window reuses one view for every session it opens: a reader who scrolled up in one
/// session must not open the next one scrolled up and off the tail.
[Test]
[NotInParallel("AvaloniaSession")]
public async Task A_new_session_in_the_same_view_opens_at_the_bottom_and_follows() {
await RunOnUiAsync(async () => {
var host = new Host();
await host.LoadAsync(Tmp.CreateFile("first.jsonl", Enumerable.Repeat(UserLine, 60).ToArray()));
WheelUp(host);
host.Settle();
await Assert.That(host.Scroll.Offset.Y).IsEqualTo(0);

var next = new Host(show: false);
var nextPath = Tmp.CreateFile("second.jsonl", Enumerable.Repeat(UserLine, 60).ToArray());
await next.LoadAsync(nextPath);
host.View.DataContext = next.Chat;
host.Settle();
await Assert.That(host.AtBottom()).IsTrue();

await next.AppendAndTickAsync(nextPath, 20);
host.Settle();
await Assert.That(host.AtBottom()).IsTrue();

await host.CloseAsync();
await next.CloseAsync();
});
}

/// Pins that the initial load lands the reader at the bottom even when it completes before the
/// view's first layout pass — an unshown window, and a tab collapsed from the start — so there
/// is no ScrollViewer to read "was at end" from when the rows arrive.
Expand Down
98 changes: 98 additions & 0 deletions test/Capacitor.App.Tests.Unit/MainWindowSmokeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,104 @@ Control Surface(string name) =>
});
}

/// Reopening after a close shows the view the window built first, bound to the new session:
/// a fresh view per open is what a macOS accessibility peer keeps alive.
[Test]
[NotInParallel("AvaloniaSession")]
public async Task Reopening_a_session_after_a_close_reuses_the_workspace_view() {
await AvaloniaSession.WithImmediateRxScheduler(async () => {
var reuse = await AvaloniaSession.DispatchAsync(() => {
var service = new FakeDaemonClientService();
var (actions, _) = NewActions(service);
var vm = new MainWindowViewModel(
service, CancellationToken.None, TestActivity.New(), TimeProvider.System,
workspaceFactory: agentId => NewWorkspace(service, actions, agentId));
var window = new MainWindow { DataContext = vm };
window.Show();
Dispatcher.UIThread.RunJobs();

vm.OpenSession("0123456789abcdef0123456789abcdef");
Dispatcher.UIThread.RunJobs();
var first = window.GetVisualDescendants().OfType<WorkspaceView>().Single();

vm.CloseWorkspace();
Dispatcher.UIThread.RunJobs();
var heldWhileClosed = first.DataContext;

vm.OpenSession("fedcba9876543210fedcba9876543210");
Dispatcher.UIThread.RunJobs();
var second = window.GetVisualDescendants().OfType<WorkspaceView>().Single();
var boundTo = (second.DataContext as WorkspaceViewModel)?.AgentId;

vm.CloseWorkspace();
window.Close();
Dispatcher.UIThread.RunJobs();
return (Same: ReferenceEquals(first, second), boundTo, heldWhileClosed);
});

await Assert.That(reuse.Same).IsTrue();
await Assert.That(reuse.boundTo).IsEqualTo("fedcba9876543210fedcba9876543210");
await Assert.That(reuse.heldWhileClosed).IsNull(); // the retained view lets the closed session go
});
}

/// A local session in between hands the slot to the other template; the next remote session
/// still gets the remote view the window built first.
[Test]
[NotInParallel("AvaloniaSession")]
public async Task A_remote_session_after_a_local_one_reuses_the_remote_view() {
using var host = new RemoteHost();
await AvaloniaSession.WithImmediateRxScheduler(async () => {
var reuse = await AvaloniaSession.DispatchAsync(() => {
var service = new FakeDaemonClientService();
var (actions, _) = NewActions(service);
var vm = new MainWindowViewModel(
service, CancellationToken.None, TestActivity.New(), TimeProvider.System,
workspaceFactory: agentId => NewWorkspace(service, actions, agentId),
remoteWorkspaceFactory: host.New);
var window = new MainWindow { DataContext = vm };
window.Show();
Dispatcher.UIThread.RunJobs();

vm.OpenSession("remote-1", AgentOrigin.Remote);
Dispatcher.UIThread.RunJobs();
var first = window.GetVisualDescendants().OfType<RemoteSessionView>().Single();

vm.OpenSession("local-1", AgentOrigin.Local);
Dispatcher.UIThread.RunJobs();
var localShown = window.GetVisualDescendants().OfType<WorkspaceView>().Count();

vm.OpenSession("remote-2", AgentOrigin.Remote);
Dispatcher.UIThread.RunJobs();
var second = window.GetVisualDescendants().OfType<RemoteSessionView>().Single();
var boundTo = (second.DataContext as RemoteSessionViewModel)?.AgentId;

vm.CloseWorkspace();
window.Close();
Dispatcher.UIThread.RunJobs();
return (Same: ReferenceEquals(first, second), localShown, boundTo);
});

await Assert.That(reuse.localShown).IsEqualTo(1);
await Assert.That(reuse.Same).IsTrue();
await Assert.That(reuse.boundTo).IsEqualTo("remote-2");
});
}

/// x:DataType on a DataTemplate subclass must still narrow it, or the local template would
/// claim remote sessions too.
[Test]
[NotInParallel("AvaloniaSession")]
public async Task Each_workspace_template_matches_only_its_own_model() {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
var types = await AvaloniaSession.DispatchAsync(() => {
var window = new MainWindow();
var host = window.FindControl<ContentControl>("WorkspaceHost")!;
return host.DataTemplates.OfType<RetainedViewTemplate>().Select(t => t.DataType!).ToList();
});

await Assert.That(types).IsEquivalentTo([typeof(WorkspaceViewModel), typeof(RemoteSessionViewModel)]);
}

/// A shown MainWindow whose rail holds two rows, "Fix the flaky test"
/// and "Leave this one alone", with an optional sibling checkout.
static (MainWindowViewModel Vm, MainWindow Window) RailWindow(
Expand Down
37 changes: 0 additions & 37 deletions test/Capacitor.App.Tests.Unit/MainWindowViewModelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,43 +48,6 @@ static MainWindowViewModel NewVm(
openFeedback: openFeedback, opener: opener);
}

/// The remote host's dependencies, held together so a test disposes them once. The lane is
/// never connected, so even a row that carries a session id dials nothing and the routing
/// assertions stay deterministic.
sealed class RemoteHost : IDisposable {
readonly FakeServerLane _lane = new();
readonly SessionAccessService _access;
readonly FakePermissionService _permissions = new();

public RemoteHost() => _access = new SessionAccessService(_lane, new FakeTimeProvider());

public FakeAgentDirectory Directory { get; } = new();
public FakeWorkContextSource WorkSource { get; } = new();

// A separate overload rather than an optional parameter: the factory parameter takes this
// as a method group, and that conversion needs an exact one-argument signature.
public RemoteSessionViewModel New(string agentId) => New(agentId, null);

/// A session id only where a test needs one: it is what a local row is compared against
/// before the removal counts as an origin change rather than an ended session.
public RemoteSessionViewModel New(string agentId, string? sessionId, IPullRequestSource? pullRequests = null, ComposerDrafts? drafts = null) {
var row = AgentRow.FromRemote(new AgentInstanceDto {
AgentId = agentId, SessionId = sessionId, Status = "Running", DaemonName = "work-mac", OwnerUserId = "u1",
Vendor = "claude", RegisteredAt = DateTime.UtcNow,
});
Directory.Rows.AddOrUpdate(row);
return new RemoteSessionViewModel(row, Directory, _access, _permissions, WorkspaceFixtures.NewActions(), _lane,
(_, _) => Task.FromResult(new SessionDetailFetch(null)), new RecordingOpener(), new FakeTimeProvider(), WorkSource,
() => new FakeTerminalSurface(), pullRequests: pullRequests, drafts: drafts);
}

public void Dispose() {
_access.Dispose();
_permissions.Dispose();
Directory.Dispose();
}
}

/// A real WorkspaceViewModel over the fake service and scripted attach/surface fakes — same
/// pieces WorkspaceNavigationTests.NewNav wires, just without its Nav bookkeeping.
/// A window wired to a launcher pane: the Start/Reconnect buttons live on the pane, so their
Expand Down
45 changes: 45 additions & 0 deletions test/Capacitor.App.Tests.Unit/RemoteHost.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
using Capacitor.App.Services;
using Capacitor.App.ViewModels;
using Capacitor.Cli.Core.PullRequests;
using Capacitor.Remote.Models;
using DynamicData;
using Microsoft.Extensions.Time.Testing;

namespace Capacitor.App.Tests.Unit;

/// The remote host's dependencies, held together so a test disposes them once. The lane is
/// never connected, so even a row that carries a session id dials nothing and the routing
/// assertions stay deterministic.
sealed class RemoteHost : IDisposable {
readonly FakeServerLane _lane = new();
readonly SessionAccessService _access;
readonly FakePermissionService _permissions = new();

public RemoteHost() => _access = new SessionAccessService(_lane, new FakeTimeProvider());

public FakeAgentDirectory Directory { get; } = new();
public FakeWorkContextSource WorkSource { get; } = new();

// A separate overload rather than an optional parameter: the factory parameter takes this
// as a method group, and that conversion needs an exact one-argument signature.
public RemoteSessionViewModel New(string agentId) => New(agentId, null);

/// A session id only where a test needs one: it is what a local row is compared against
/// before the removal counts as an origin change rather than an ended session.
public RemoteSessionViewModel New(string agentId, string? sessionId, IPullRequestSource? pullRequests = null, ComposerDrafts? drafts = null) {
var row = AgentRow.FromRemote(new AgentInstanceDto {
AgentId = agentId, SessionId = sessionId, Status = "Running", DaemonName = "work-mac", OwnerUserId = "u1",
Vendor = "claude", RegisteredAt = DateTime.UtcNow,
});
Directory.Rows.AddOrUpdate(row);
return new RemoteSessionViewModel(row, Directory, _access, _permissions, WorkspaceFixtures.NewActions(), _lane,
(_, _) => Task.FromResult(new SessionDetailFetch(null)), new RecordingOpener(), new FakeTimeProvider(), WorkSource,
() => new FakeTerminalSurface(), pullRequests: pullRequests, drafts: drafts);
}

public void Dispose() {
_access.Dispose();
_permissions.Dispose();
Directory.Dispose();
}
}
Loading