Improve terminal sizing controls and defaults - #19811
Improve terminal sizing controls and defaults#19811Mitch Denny (mitchdenny) wants to merge 2 commits into
Conversation
Move terminal sizing controls into the footer, preserve the active resolution across terminal peers, and default new terminals to 132x50. Ensure configured dimensions reach both DCP PTYs and HMP consumers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 395e0f6a-f4c1-4db8-a3dd-e0fbf5392773
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19811Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19811" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js — This keyboard-only behavior is not exercised by the updated bUnit tests; those tests mock… |
|
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js — The core resolution-preservation fix is currently untested on the dashboard side. The new… |
What changed in this PR
Improves terminal sizing defaults, preserves producer dimensions, and moves localized sizing/focus controls into the terminal footer.
Changes:
- Changes default terminal dimensions to
132×50. - Initializes HMP/DCP sessions with configured dimensions.
- Adds footer sizing controls, F6 focus navigation, localization, and tests.
| File | Description |
|---|---|
tests/Aspire.TerminalHost.Tests/TerminalHostArgsTests.cs |
Updates default-size assertions. |
tests/Aspire.TerminalHost.Tests/TerminalHostAppTests.cs |
Tests configured handshake and PTY dimensions. |
tests/Aspire.Hosting.Tests/WithTerminalTests.cs |
Updates hosting default assertions. |
tests/Aspire.Dashboard.Components.Tests/Pages/ConsoleLogsTerminalTests.cs |
Updates menu and localization interop tests. |
src/Aspire.TerminalHost/TerminalReplica.cs |
Configures HMP presentation dimensions directly. |
src/Aspire.TerminalHost/TerminalHostArgs.cs |
Changes defaults to 132×50. |
src/Aspire.TerminalHost/Hmp1UdsServerListenerFilter.cs |
Adds the custom HMP consumer listener. |
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs |
Updates documented polyglot defaults. |
src/Aspire.Hosting/ApplicationModel/TerminalAnnotation.cs |
Updates terminal option defaults and docs. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.zh-Hant.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.zh-Hans.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.tr.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.ru.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.pt-BR.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.pl.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.ko.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.ja.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.it.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.fr.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.es.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.de.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.cs.xlf |
Adds the focus hint resource. |
src/Aspire.Dashboard/Resources/ConsoleLogs.resx |
Defines the focus hint text. |
src/Aspire.Dashboard/Resources/ConsoleLogs.Designer.cs |
Exposes the new localized resource. |
src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor.cs |
Removes page-menu terminal controls. |
src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor |
Passes localized footer labels. |
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js |
Implements footer controls, focus navigation, and sizing preservation. |
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.cs |
Adds localized label parameters and JS wiring. |
docs/specs/with-terminal.md |
Documents sizing and focus behavior. |
Files not reviewed (1)
- src/Aspire.Dashboard/Resources/ConsoleLogs.Designer.cs: Generated file
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Cover terminal focus navigation and producer-dimension preservation with Playwright using a lightweight HMP test peer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 395e0f6a-f4c1-4db8-a3dd-e0fbf5392773
Tests selector54 / 99 PR test projects · 3 PR jobs · 2 advisory-only targets, from 32 changed files. Selected PR test projects (54 / 99)
Selected PR jobs (3)
Advisory workflow impact (2)
How these were chosen — grouped by what changed
🔧 show 43
🧪 🔧 📦 affected project 📦 affected project 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
Issues resolved since last review (2)
| Severity | Finding |
|---|---|
|
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js — The core resolution-preservation fix is currently untested on the dashboard side. The new… View resolved comment |
|
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js — This keyboard-only behavior is not exercised by the updated bUnit tests; those tests mock… View resolved comment |
Files not reviewed (1)
- src/Aspire.Dashboard/Resources/ConsoleLogs.Designer.cs: Generated file
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
I found lifecycle and sizing paths that can leave the terminal unavailable or overwrite the configured grid.
| CancellationToken ct = default) | ||
| { | ||
| _listenerCts = CancellationTokenSource.CreateLinkedTokenSource(ct); | ||
| _listenerTask = Task.Run(() => RunListenerAsync(_listenerCts.Token), _listenerCts.Token); |
There was a problem hiding this comment.
OnSessionStartAsync reports success before ListenUnixSocket has bound. A bind/listen exception faults this detached task, but nothing observes it until session teardown, so the producer can stay live while the consumer endpoint is unusable. Could startup await an explicit listener-ready/failure signal and monitor later listener faults during the session?
| { | ||
| await foreach (var stream in Hmp1Transports.ListenUnixSocket(socketPath, ct).ConfigureAwait(false)) | ||
| { | ||
| _ = AddClientAsync(stream, ct); |
There was a problem hiding this comment.
Every accepted client is detached, while session teardown waits only for the listener. Active callbacks and stream cleanup can therefore outlive the presentation and race the next replica cycle; unexpected client faults are also unobserved. Please track these tasks, cancel and await them before OnSessionEndAsync returns, and dispose each stream in finally.
|
|
||
| private Hmp1PresentationAdapter CreateDownstream(DcpUpstreamAdapter upstream) | ||
| { | ||
| var downstream = new Hmp1PresentationAdapter(Columns, Rows); |
There was a problem hiding this comment.
_currentColumns and _currentRows track the negotiated live grid, but every rebuilt terminal seeds both adapters from the immutable configured Columns/Rows. A producer recycle therefore undoes an explicit user resize. Could BuildTerminal() snapshot the current dimensions under _gate for both the terminal and presentation, with a resize-then-reconnect regression test?
| // to Fit or selects another preset. | ||
| function adoptProducerDimensions(state) { | ||
| const client = state.client; | ||
| if (!client || client.isPrimary || client.width <= 0 || client.height <= 0) { |
There was a problem hiding this comment.
An initial Hello can already make this dashboard the primary peer. This guard then leaves fixedDims at the 132x50 default, and the following primary fixed-mode layout sends that size upstream, overwriting an explicit producer size such as 137x41. Initial Hello should seed fixedDims from every valid producer grid before applying role-aware layout; the secondary-only rule can remain for later resize events.
|
|
||
| function attachTerminalFocusNavigation(state, term) { | ||
| term.attachCustomKeyEventHandler((event) => { | ||
| if (event.key !== 'F6') { |
There was a problem hiding this comment.
This also consumes Ctrl+F6, Alt+F6, and Meta+F6 as focus navigation. Could this handle only exact F6 and Shift+F6, returning true whenever Ctrl, Alt, or Meta is present, and add a modified-key regression case?
Description
Terminal sizing in the dashboard currently requires opening the page toolbar, and interacting with a terminal can unexpectedly switch its resolution to Fit. New terminals can also start at
80x24even whenWithTerminalspecifies another size.This change:
F6focus hint on the bottom-left.F6andShift+F6to move focus between the terminal and dashboard controls without consuming Escape.132x50, while continuing to honor explicitWithTerminalcolumn and row overrides.80x24presentation default.User-facing usage
Terminals now start at
132x50by default:Existing overrides remain authoritative:
Screenshots / Recordings
TODO: Capture the terminal footer showing the
F6hint, resolution selector, and font controls.Validation
Aspire.Hosting.TestsWithTerminalTests: 46 passed, 1 platform skip.Aspire.TerminalHost.Tests: 27 passed.Aspire.Dashboard.Components.TestsConsoleLogsTerminalTests: 32 passed.50 132by default and both overridden REPL PTYs report32 120.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?