Add Console/Terminal view toggle to ConsoleLogs dashboard page#18574
Open
mitchdenny wants to merge 45 commits into
Open
Add Console/Terminal view toggle to ConsoleLogs dashboard page#18574mitchdenny wants to merge 45 commits into
mitchdenny wants to merge 45 commits into
Conversation
When a resource has WithTerminal() applied, the dashboard previously replaced the LogViewer with TerminalView the moment the resource was selected. This hid pre-PTY hosting messages (WaitFor, startup failures) because the terminal frame just showed a blank 'connecting' state and the console log stream was not subscribed to at all for terminal resources. Add a small 'View' FluentSelect to the toolbar (only visible for terminal-enabled resources) that flips between Console and Terminal: - Both LogViewer and TerminalView stay mounted for terminal resources (visibility toggled via display:none on a wrapper div), so neither loses scrollback or has to re-handshake on flip. - Console logs are now subscribed to for terminal resources too, so the Console view actually has content. - The page starts on Console and auto-switches to Terminal the first time the PTY attaches (toolbar status leaves 'connecting'). - On PTY exit (client.onExit) the page auto-switches back to Console so the final log lines and hosting exit message are visible. - The user's manual pick is latched per resource — auto-switch is suppressed once the user has chosen a view. - After display:none → visible the page calls a new refreshLayout JS function to make sure xterm rebinds to the new available space. Toolbar (pause/clear/settings vs. terminal font/size/take-control) and page header / title are now keyed off the active view instead of HasTerminal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18574Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18574" |
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous "terminal resources show only TerminalView" behavior on the dashboard ConsoleLogs page with a dual-mounted Console/Terminal toggle. Both LogViewer and TerminalView stay mounted for WithTerminal() resources and visibility is flipped via CSS, so neither view tears down its subscription/PTY session on toggle. Console logs are now also subscribed for terminal resources, and the page intelligently auto-switches between views around PTY attach/exit while honoring an explicit user choice.
Changes:
- Adds a "View"
FluentSelect(Console | Terminal) shown only for terminal-enabled resources, with default-to-Console, auto-switch on PTY attach/exit, and a manual-pick latch that resets per resource. - Extends
TerminalViewwith anOnExitedcallback +OnTerminalExited[JSInvokable]and aRefreshLayoutAsync/refreshLayoutpath to re-layout xterm after a hidden→visible flip. - Adds the
ConsoleLogsViewenum, three localized strings (resx/Designer/xlf across 13 locales), tests, and a spec update.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor.cs |
Adds view state/latch fields, auto-switch + exit handlers, view-change handler, layout nudge, test accessors, and ConsoleLogsView enum; falls through to subscribe console logs for terminal resources. |
src/Aspire.Dashboard/Components/Pages/ConsoleLogs.razor |
Dual-mounts both views with display toggling, adds the View dropdown, and keys toolbar/title/header off _activeView. |
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.cs |
Adds OnExited callback, OnTerminalExited JSInvokable, RefreshLayoutAsync, and TerminalExitInfo record. |
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js |
Notifies .NET on client.onExit and adds the refreshLayout export. |
src/Aspire.Dashboard/Resources/ConsoleLogs.resx / .Designer.cs |
Adds View/Console/Terminal option strings and generated accessors. |
src/Aspire.Dashboard/Resources/xlf/ConsoleLogs.*.xlf (13 files) |
UpdateXlf-generated entries for the three new strings. |
tests/Aspire.Dashboard.Components.Tests/Pages/ConsoleLogsTerminalTests.cs |
Updates two tests to the dual-mount contract and adds auto-switch / latch / exit coverage. |
docs/specs/with-terminal.md |
Documents the Console/Terminal toggle semantics. |
Review details
Files not reviewed (1)
- src/Aspire.Dashboard/Resources/ConsoleLogs.Designer.cs: Generated file
- Files reviewed: 20/21 changed files
- Comments generated: 0
- Review effort level: Medium
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Two related issues surfaced when stopping and restarting a WithTerminal resource: 1. The page no longer auto-switched back to the Terminal view after the restart. The previous '_terminalAttachedOnce' latch was set on the first PTY attach and never reset, so subsequent attaches were suppressed. Replaced the latch with an edge detector that fires on the 'connecting -> connected' transition, and reset the cached status to 'connecting' inside OnTerminalExitedAsync so the next reconnect's first non-connecting snapshot re-arms the auto-switch. 2. Multiple xterm DOM nodes accumulated in the terminal container. OnAfterRenderAsync can re-enter while the very first initTerminal JS call is still awaiting (Blazor does not serialize lifecycle awaits when re-renders happen during them). The non-firstRender branch then saw '_connectedResourceName == null', treated it as a rebind, and because '_terminalId' was also still 0, fell through to InitializeTerminalAsync again. buildChrome appended a fresh host on top of the existing one, so two xterm instances ended up wired to the same WebSocket. Added an '_initStarted' guard that short-circuits the rebind branch while the initial init is in flight, and made buildChrome defensively clear the Blazor container element before appending its host so we never stack chrome under any future race. Added a regression test that drives attach -> exit -> reconnect and asserts the view ends back on Terminal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Contributor
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
When the user clicks Stop on a WithTerminal resource, DCP kills the producer process directly. The producer never sends an HMP1 Exit frame, so client.onExit on the JS side never fires and the existing OnTerminalExitedAsync path never runs. The page therefore stayed on the Terminal view even after the resource was no longer running. React to the resource snapshot itself: in OnResourceChanged track the selected terminal resource's IsStopped() value and fire the same flip-back-to-Console (plus _lastTerminalStatus re-arm) on the running -> stopped edge. Seed the tracker from the initial snapshot in SubscribeAsync so we don't fire on first observation of a resource that was already stopped when the page loaded. Added a regression test that drives attach -> resource Upsert with KnownResourceState.Exited -> asserts view returns to Console. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
After manually stopping a terminal resource the page would flip back to Console (correct) and then immediately snap back to Terminal as soon as the next toolbar snapshot arrived. Two things conspired: 1. The stop-edge handler synthetically set _lastTerminalStatus to 'connecting' to 'arm' the next attach edge. 2. The JS side often still had an in-flight 'primary' snapshot queued up (the WS close had not propagated yet). The next snapshot to land therefore looked like a clean connecting -> connected transition and triggered the auto-switch. Fix: - Drop the synthetic 'connecting' re-arm. On resource restart the real WS naturally goes through close -> connecting -> connected so the genuine edge will fire anyway. - Gate the auto-switch in OnTerminalToolbarStateChangedAsync on !_selectedTerminalResourceStopped so that any stale snapshot that arrives while the resource is in Exited/Finished/ FailedToStart is ignored. When the resource transitions back out of a stopped state the gate reopens and the next real attach edge wins. Added a regression test that drives attach -> resource Exited -> stale 'primary' snapshot and asserts the view stays on Console. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
In secondary mode (viewing a terminal owned by another primary), the TerminalView locked the xterm grid to producer cols x rows and CSS transform: scale()'d .xterm to fit the pane. xterm.js computes mouse-to-cell coordinates from getBoundingClientRect (which returns transformed dimensions) divided by its internally-measured cell width (which is untransformed), so any scale != 1 offset text selection by roughly the scale factor. In tighter panes the scale was larger and the offset was proportionally worse. Replace lock-and-scale with the same font-driven fixed-grid approach primary fixed-mode already uses: lock the grid to producer dims, pick the largest integer font whose rendered grid fits the pane, then pin the body to the natural rendered size. Letterboxing behavior is the same (whichever axis has spare room), but pointer pixels and cell pixels are now in the same coordinate space so hit testing lines up. Removes the now-dead measureAndScale helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
computeOptimalFont depends on cellWRatio/cellHRatio, which are seeded by calibrateRatios inside pinBodyToNatural — and that only runs one RAF *after* the initial layout pass. On the very first open of a terminal in either secondary or primary fixed mode, the ratios are still zero, computeOptimalFont bails to the default 13px font, and we render at that size until a ResizeObserver tick (window resize, sidebar collapse) re-drives layout with fresh ratios. Add a post-pin refinement step that re-measures once calibration has happened, and if the optimal font moved, adjusts fontSize in place and re-pins the body. Extracted into refineFontAfterCalibration so both fixed and secondary paths share the correction. Uses a direct in-place adjustment rather than a recursive applyRoleAwareLayout call to keep the trigger bounded and avoid stacking under fast resize triggers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
xterm 5.5.0 only reliably re-measures cell metrics when fontFamily changes; setting term.options.fontSize alone can leave stale cell dimensions in the renderer. That caused fitAddon.fit() to divide the available space by the *old* cell size after we switched from a fixed preset (with an auto-computed large font) back to Fit mode, producing a much smaller grid than the toolbar predicted from fitFontPx. Reuse the fontFamily-bounce trick already used by the late font-load handler, and apply it to every code path that writes fontSize (setFontSize, fixed-mode layout, secondary-scale layout, and the post-calibration font refinement). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- src/Aspire.Dashboard/Resources/ConsoleLogs.Designer.cs: Generated file
Comments suppressed due to low confidence (2)
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js:781
- Unconditional
console.log('[TERMDIAG] ...')here will spam the console. Remove it or gate behindwindow.__aspireTerminalDebug.
state.currentFontPx = state.fitFontPx;
}
applyRoleAwareLayout(state);
}
// Computes the current toolbar state snapshot and (when changed) pushes
src/Aspire.Dashboard/Components/Controls/TerminalView.razor.js:788
- Unconditional
console.log('[TERMDIAG] ...')here will spam the console and add overhead. Remove it or gate behindwindow.__aspireTerminalDebug.
// it up to the Blazor host so the page-level toolbar can render the
// status badge, "Take control" button, font controls, size dropdown and
// dims readout. RAF-coalesced because callers include term.onResize,
// applyRoleAwareLayout's RAF callbacks and ResizeObserver — they can
// fire in rapid bursts during window/sidebar resize. Change-detected
// so a no-op call (e.g. layout pass that produced identical dims) does
// not round-trip to .NET.
- Files reviewed: 20/21 changed files
- Comments generated: 5
- Review effort level: Low
This comment has been minimized.
This comment has been minimized.
Not for merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Gate [TERMDIAG] console.log calls in TerminalView.razor.js behind window.__aspireTerminalDebug so end-user consoles are not spammed. - InitializeTerminalAsync now also swallows non-JSDisconnected exceptions (e.g. JSException during module import) so they cannot bubble out of OnAfterRenderAsync and tear down the Blazor circuit. - firstRender in OnAfterRenderAsync only records _connectedResourceName / _connectedReplicaIndex when _terminalId != 0, so a failed JS init does not mask itself as a successful connection. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
xterm's DOM re-render after a fontSize change lags one paint cycle behind the setter, so a calibrateRatios call scheduled via requestAnimationFrame from term.onResize can still measure .xterm-screen at the OLD fontSize's cell metrics. Dividing that stale pixel width by the NEW fontSize yields a cellWRatio ~half of the true value. Once corrupted, that ratio propagates: the Fit menu preview reports roughly double the real grid (172x73 instead of 82x35), and computeOptimalFont in fixed mode picks the wrong font too. Real xterm cell metrics per fontSize are stable across small font deltas — that's the whole reason we cache a ratio. So a 40% jump is diagnostic of a stale-render sample, not a real change. Reject those samples and keep the previous good baseline; the next calibration after xterm has actually re-rendered will match and be accepted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
If a Terminal menu-item click races a resource-selection change to a shell-less resource, the razor markup would hide the toolbar because it only renders search/menu for Console view or terminal- enabled resources — leaving the page in a broken state. Coerce Terminal→Console when the current selection can't host a terminal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Two changes so the Fit menu preview matches what clicking Fit actually produces: - When already in fit mode with a rendered grid, show term.cols / term.rows directly — they ARE what fit() produced, so preview and actual can never disagree. - Cache the (cols, rows, availW, availH) tuple from every real fit() run at fitFontPx. In fixed mode, extrapolate the cached observation by the current avail-space delta instead of using cellWRatio/cellHRatio. Ratio math is 1-2 rows off because xterm's cell measurement includes sub-pixel padding that ratio math can't recover exactly; the cached real observation avoids the drift. Ratio math stays as a bootstrap fallback for the very first render before any fit has been observed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Delete TerminalToolbarFontSize from ConsoleLogs.resx and ConsoleLogs.Designer.cs (no code path references it after the toolbar consolidation) and regenerate the xlf translations. - Replace absolute line-number reference in calibrateRatios's comment with a named reference to term.onResize (line numbers drift as the file evolves). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The projected 'Fit (cols×rows)' label in the size dropdown relied on ratio-based extrapolation and cached observations that couldn't faithfully predict what fitAddon.fit() would actually pick. In practice the preview and the real fit result drifted enough that the number was more misleading than useful. Drop the preview and just show 'Fit' — clicking it produces the true fit dimensions which are then reflected in the toolbar's dimensions readout. Removes FitCols/FitRows from TerminalToolbarState, the JS lastFitObservation cache, the fit-preview computation in buildToolbarSnapshot, and GetTerminalSizePresetsForDisplay. Keeps cellWRatio/cellHRatio calibration and fitFontPx — both are still load-bearing for computeOptimalFont in fixed / secondary layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 7 / 99 test projects · 4 jobs, from 20 changed files. Selected test projects (7 / 99)
Selected jobs (4)
How these were chosen — grouped by what changed🔧 📦 affected project 🧪 Job reasons
Selection computed for commit |
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
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.
Description
For a
WithTerminal()resource the dashboard'sConsoleLogspage previously replacedLogViewerwithTerminalViewthe moment the resource was selected. While the PTY hadn't attached yet (or the resource wasWaitFor-ing something), the terminal frame just showed a blank "connecting" state, and the console log stream wasn't subscribed to at all for terminal resources — so any hosting messages like "Waiting for resource X to become healthy..." or startup failures were invisible.This adds a Console logs / Terminal view selector as two menu items inside the toolbar's existing options (⋯)
AspireMenuButton, shown only when the selected resource has terminal support. The page title and header stay as "Console logs" in both views.Behavior
LogViewerandTerminalViewstay mounted for terminal resources; visibility is toggled withdisplay:noneon a wrapper<div>. Flipping never tears down the LogViewer subscription or the xterm/HMP1 consumer session, so neither view loses scrollback or has to re-handshake.display:none -> visibletransition the page calls a newrefreshLayoutJS function to guarantee xterm rebinds to the new available space (ResizeObserver is not guaranteed to fire for that box-tree change in every browser).FluentSearchand the terminal font/size toolbar controls are keyed off the active view.Terminal view fixes (bundled)
While validating the toggle behavior, three long-standing terminal rendering bugs surfaced and are fixed here as well:
initTerminalnow clears any prior xterm host element from the wrapper before opening a fresh one, so stop/start cycles no longer stack multiple terminals in the DOM.transform: scale()to letterbox the producer's grid into the pane, but xterm's mouse hit-testing dividesgetBoundingClientRect(transformed) by its internally-measured cell width (untransformed), producing a selection offset proportional to the scale factor and additive across viewer tabs. Secondary mode now uses a font-driven fixed-grid approach mirroring primary fixed mode: lock xterm to the producer's cols x rows and pick the largest integer font that fits, so hit-testing works natively with no transform.computeOptimalFontneedscellWRatio/cellHRatiothat are seeded bycalibrateRatiosinsidepinBodyToNatural, which runs one RAF after the initial layout pass. On first render the ratios were still zero, so the layout defaulted to 13px until a browser resize kicked it. A newrefineFontAfterCalibrationhelper re-checks the optimal font after calibration lands and adjusts in place if it changed, so terminals now snap to the best font size on first paint.Toolbar consolidation
The terminal font size and dimensions controls are folded into the same options (⋯) menu as the view selector. The menu groups are: Console logs / Terminal (view), increase / decrease font, and terminal dimensions presets (Fit + fixed grids).
New cross-component plumbing
TerminalViewgainsRefreshLayoutAsync()mapped to a newrefreshLayoutJS export.ConsoleLogs.ConsoleLogsViewenum (Console,Terminal) matching theResourceViewKind/MetricViewKindconvention.ConsoleLogsViewConsoleOption,ConsoleLogsViewTerminalOption) with xlf placeholders generated byUpdateXlf.Tests
ConsoleLogsTerminalTestscovers the toggle behavior:displaystyle on their view wrappers.All
ConsoleLogsTestscases pass.Spec
Updated
docs/specs/with-terminal.mdwith a new "Console / Terminal view toggle" subsection describing the dual-mount, user-controlled toggle, and dual-subscription semantics.Fixes #18082
Fixes #18085
Checklist