refactor: Move ui5 serve banner from cli to logger#1445
Open
matz3 wants to merge 16 commits into
Open
Conversation
Replaces the hand-rolled hrtime formatter in the no-listener fallback path of Serve#buildDone with pretty-hrtime, which is already used across @ui5/cli, @ui5/fs, and @ui5/project. Follow-up to #1439.
Make explicit that event handlers, not only setters, mutate banner state and trigger renders. Follow-up to #1439.
Relocate the live status banner previously implemented in packages/cli/lib/serve/ into a new InteractiveConsole writer in @ui5/logger, split into render, format, and per-section state modules. The serve command and logger middleware wire the writer in place of the former Banner integration.
….tool-mode The serve command now emits a `ui5.tool-mode` event before graph resolution and server startup begin. The InteractiveConsole writer uses it to pre-populate dim placeholders for the project, server, and status regions so the full frame is visible from the first paint and later events replace placeholders in place rather than growing the layout. When `--accept-remote-connections` is set, the writer reserves one 'Network:' row per non-internal IPv4 address the CLI announces. Also: - Restore the historical `Server started` / `URL:` stdout format of the Console writer for the non-interactive serve path, so scripts parsing the log lines are not disrupted. - Add a `UI5_CLI_NO_INTERACTIVE` env kill-switch to force the non-interactive logger, useful for CI and debugging.
…me growth log-update v7.2.0's diff-patch path leaves the cursor one or more rows above where it thinks it is when the new frame extends past the previous one — its buildPatch clamps the pre-write cursor move at 0 instead of moving DOWN to the new start row. The next persist()/clear() then calls eraseLines() with a count based on the new frame, erasing rows that overlap the scrollback above the live region (typically the shell prompt line where the user typed 'ui5 serve'). Sidestep the bug by tracking the composed frame's line count and forcing log-update onto its first-frame write path (via clear()) whenever the row count changes. Steady-state renders (spinner ticks, in-place updates with unchanged row count) still take the fast diff path.
The InteractiveConsole writer no longer accepts a networkAddressCount to reserve extra Network: placeholder rows up front. In placeholder mode the server region now paints a single "Network: binding…" row when --accept-remote-connections is set; if the real URL list from ui5.server-listening contains more addresses, the frame simply grows by those additional rows. This is an uncommon case (multiple non-internal IPv4 interfaces) and keeping the hint out of the writer avoids either pushing network-interface enumeration into the serve command handler or exposing the address count from @ui5/server.
The Console writer's ui5.server-listening handler wrote 'Server started' / 'URL:' to stderr and reformatted the remote-connections warning as a single warn-prefixed line. Both diverged from the pre-banner output that scripts and pipes may rely on. Route the URL lines back to stdout, and emit the warning as the original multi-line, fully chalk-formatted block on stderr with surrounding blank lines - reusing the REMOTE_CONNECTIONS_WARNING_LINES constant already shared with the interactive writer.
The event is consumed exclusively by InteractiveConsole's header region.
ConsoleWriter has no listener for it, so emitting on that path was dead
work and the comment ("or scrollback line") described behavior that
does not exist.
Match the interactive console banner: use a middle dot separator in the stale-state description, and note that the TTY check now runs against stderr rather than stdout.
- Drop unused hasContent() exports from all four state modules; region visibility is decided by render*Region returning []. - Un-export resetBuildProgress(); only used inside build.js. - Return line count from #composeLiveRegion instead of re-splitting the joined frame in both #render and logAbove. - Replace nested for-of in #composeLiveRegion with a flat spread. - Drop unreachable #logUpdate null-guard in #render; enable() always sets it before listeners can fire. - Replace three sequential .filter passes in renderServerRegion with a single partitioning loop. - Collapse renderBuildRegion's 3-line array builder into one return.
Give each region's placeholder-enabling function a unique name (enableProjectPlaceholders / enableServerPlaceholders / enableBuildPlaceholders) so InteractiveConsole.js can import them directly without `as` aliases.
Rebuilds the tests lost when the ui5 serve banner moved from packages/cli to packages/logger. Coverage across statements, branches, functions, and lines is back at 100%. - Console.js: project-resolved + server-listening handlers (with and without acceptRemoteConnections, non-array urls, Local fallback). - InteractiveConsole.js: spinner tick, resize, static init/stop, all serve-status variants, log-level filtering, frame-growth path, timer without unref support, and the full process.stdout/stderr interception suite (chunk joining, partial-flush on stop, Buffer/Uint8Array chunks, async callback contract, stopped-writer short-circuit). - interactiveConsole/render.js: region renderers + status-line glyph- and color-per-state matrices. - interactiveConsole/format.js: COLORFGBG dark-mode detection via cache-busting dynamic import. - interactiveConsole/state/*: unit tests for the four state modules. Also marks two defensive #stopped guards in InteractiveConsole as istanbul-ignore — they cover the case where a listener or timer fires after disable(), which cannot happen because disable() detaches all listeners and clears the tick timer synchronously.
Member
Author
|
I pushed a tmp branch to allow including the initial PR #1439 into the diff: main-before-serve-banner...ui5-serve-console-output-followup This should make a review easier as it allows to see what effectively changed, not only what the follow-up PR does. |
…ertions The `Status\s+\S+\s+starting` regex assumed a single non-space glyph between the "Status" label and the state word. On Windows without a Unicode-capable terminal (e.g. GitHub Actions runners), figures.circle falls back to "( )" — a three-character sequence containing a space — so \S+ can no longer span the whole glyph and the assertion fails. Loosen the pattern to `.+?`, matching the convention already used by the ready/stale/building/error assertions in the same file.
RandomByte
approved these changes
Jul 3, 2026
RandomByte
left a comment
RandomByte
left a comment
Member
There was a problem hiding this comment.
Really much better, thanks! Just one optional comment
| const frameworkVersion = framework.version ? ` ${framework.version}` : ""; | ||
| lines.push(`${chalk.dim("Framework")} ${chalk.bold(framework.name + frameworkVersion)}`); | ||
| } else { | ||
| lines.push(`${chalk.dim("Framework")} ${placeholder("(none)")}`); |
Member
There was a problem hiding this comment.
I was wondering whether we should just omit the line if no framework version is configured. Obviously for a UI5 app to work, there is a framework. So it's not really "none" but rather "unknown" - in which case we might just omit the info (and render an empty line) to avoid confusion?
Member
There was a problem hiding this comment.
Due to the use of the fiori-tools-proxy middleware, this might be a very common scenario (Claude found 4169 out of 6505 ui5*.yaml files in my local cluecode repository which do not contain a framework config).
Could a middleware like fiori-tools-proxy in future contribute this information, e.g. via some log point?
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
3 participants
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.
@ui5/cliinto a newInteractiveConsolewriter in@ui5/loggerui5.tool-mode/ui5.tool-infolog events instead of direct CLI callsrender,format, and per-regionstate/*modules@ui5/projectand@ui5/serverto feed the writerThis PR covers several review comments from #1439, but not all of them.
For example, better handling of the log level and framework resolution/download progress will be tackled in another PR.