Skip to content

ui: remove the shared Banner method (no intro banner on any command) - #375

Merged
LukasWodka merged 3 commits into
developfrom
chore/remove-shared-banner
Jul 21, 2026
Merged

ui: remove the shared Banner method (no intro banner on any command)#375
LukasWodka merged 3 commits into
developfrom
chore/remove-shared-banner

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

What

The follow-up to #370: removes the intro banner from every remaining command and deletes the shared ui.Printer.Banner method so it can't come back.

The banner block —

 tracebloc
────────────────────────────────────────
<subtitle>

— was a single shared method called by 7 commands. #370 removed it from the data commands; the doctor redesign dropped it from doctor. This PR removes the last three callers (cluster info, resources, resources set) and deletes the method itself.

Why now

Verified the follow-up is unblocked: doctor already de-bannered on develop; the resources redesign (#359) merged its new view but left the banner; PR #361 (which would have removed it) is closed; and the only open PR touching these areas (#369) is scoped to doctor.go — no collision.

Spacing

Every command still opens with exactly one blank line:

  • cluster info — first line is a Section, which supplies its own leading blank → just remove the banner.
  • resources (show) — first line is a Stat (no leading newline) → replace the banner with Newline().
  • resources set — the no-op and phantom-GPU messages are message-led → add Newline() to each; the Section/dry-run paths supply their own, so I also dropped a now-redundant Newline() in the dry-run path (it was double-blanking once the banner was gone).

Verified live against a k3d env (resources, cluster info, resources set --dry-run for both the no-op and change paths) — all open with a single clean blank line.

Changed

  • internal/ui/ui.go — delete Banner; fix the package doc + Para comment that referenced it.
  • internal/cli/cluster.go, resources.go, resources_set.go — drop the banner calls (+ spacing as above).
  • internal/ui/ui_test.go — drop the Banner call + its "tracebloc" assertion (the method no longer exists).
  • internal/cli/cluster_info_test.go — the test asserted the "cluster diagnostics" subtitle; now asserts the Kubeconfig section (test-ctx) instead.
  • internal/cli/data_delete_json_test.go — stale "banner" comment → "output".

Test plan

  • go build ./..., go vet, gofmt — clean.
  • go test ./... — all pass.
  • Grepped the tree: zero references to the UI Banner remain (the internal/submit "banner" hits are the ingestor's 📊 INGESTION SUMMARY banner — unrelated).

🤖 Generated with Claude Code


Note

Low Risk
CLI presentation and test-only seams only; no auth, cluster mutation, or API behavior changes.

Overview
Removes the shared ui.Printer.Banner helper and the last CLI callers (cluster info, resources show, resources set), completing the de-banner work after earlier commands dropped it.

Spacing:resources show starts with Newline() instead of a banner; no-op / phantom-GPU messages in resources set add Newline() where needed; the confirm path drops an extra Newline() before PromptHint so output does not double-blank; dry-run drops a redundant Newline() before Section. runResourcesShow uses the resolveClusterTargetFn seam for tests.

Tests: Expectations move off banner subtitles (e.g. cluster info checks test-ctx); new tests pin a single leading blank for show and set confirm after #375.

Reviewed by Cursor Bugbot for commit 5c24c7b. Bugbot is set up for automated code reviews on this repo. Configure here.

Completes the banner removal started for the data commands (#370). The
`tracebloc` + rule + subtitle block is gone from the last callers —
`cluster info`, `resources`, `resources set` — and the shared
`ui.Printer.Banner` method is deleted so nothing can reintroduce it.
(`doctor` already dropped its banner in its own redesign.)
Spacing: commands whose first line is a Section (`cluster info`) keep their
leading blank from Section; the Stat/message-led paths (`resources` show,
the `resources set` no-op / phantom-GPU notes) get an explicit Newline so
every command still opens with exactly one blank line. Also dropped a now-
redundant Newline in the `resources set` dry-run path (Section supplies it).
Tests: drop the Banner unit-test call + its assertion; update the
cluster-info test that asserted the "cluster diagnostics" subtitle; fix
stale "banner" comments (incl. the ui package doc + a data-delete test).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Jul 21, 2026
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

👋 Heads-up — Code review queue is at 31 / 30

Above the WIP limit. The team convention is to review existing PRs before opening new work.

Open PRs currently in Code review (oldest first):

Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.)

Comment threadinternal/cli/resources_set.go
…gbot #375)
After removing the intro banner, flag-driven interactive `resources set` (no
--yes/--dry-run) opened with TWO blank lines: a Newline() immediately before
PromptHint, which already self-leads with a newline. The dry-run path dropped
its redundant Newline() (Section supplies one) but the confirm path kept the
stacked pair, so the command no longer opened with a single blank line.
Remove the redundant Newline() — PromptHint's own leading newline gives the one
blank the PR's spacing contract promises. Adds TestSet_ConfirmOpensWithSingleBlank
(drives the flag confirm path via runSet) to pin it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadinternal/cli/resources_set.go
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 6afdd70. Configure here.

Comment threadinternal/cli/resources.go
…#375)
The leading Newline() in runResourcesShow (before resolve, so a resolve-time
redirect line also gets a blank) wasn't covered — every resources-show test
drives renderResources directly, skipping it. Wire runResourcesShow to the
resolveClusterTargetFn seam (matching the data commands) and add
TestShow_OpensWithSingleBlank: it drives the outer function through a canned
target and asserts it opens with exactly one blank — the mirror of
TestSet_ConfirmOpensWithSingleBlank. Confirmed it fails if the Newline() is
removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 5c24c7b. Configure here.

@LukasWodka
LukasWodka merged commit 9257f5b into developJul 21, 2026
20 checks passed
@LukasWodka
LukasWodka deleted the chore/remove-shared-banner branch July 21, 2026 13:26
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass — verified live: no intro banner on data, doctor, resources, or --help.

divyasinghds added a commit that referenced this pull request Aug 7, 2026
Removing the intro Banner (#375) left `resources show` and `resources set`
handling the leading blank differently, so the multi-client resolve-time
redirect line got a blank on show but not on set (Bugbot "Missing lead blank
on set path"). The naive fix — give set the same pre-resolve Newline() — would
re-introduce the #375 double-blank (it stacks with the self-leading confirm
PromptHint / dry-run Section).
Standardize the spacing at the source instead of per-caller:
- discoverRelease's multi-client redirect Infof now self-leads its own single
blank, so every command that hits it gets exactly one leading blank — no
per-command pre-resolve Newline() needed.
- resources show moves its leading blank from before resolveClusterTarget into
renderResources (before the first Stat), so it no longer stacks on the
now-self-leading redirect in the multi-client case.
- resources set keeps no command-level pre-resolve Newline() (preserves the
#375 double-blank fix); its self-leading confirm/dry-run paths are unchanged.
Tests: added a self-lead assertion to TestDiscoverRelease_ScanFindsSingleClient-
Elsewhere; refreshed the stale comment in TestShow_OpensWithSingleBlank (the
blank moved into renderResources). go build/test, gofmt -s, and go vet all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
divyasinghds added a commit that referenced this pull request Aug 7, 2026
…eview)
The self-lead blank in discoverRelease's multi-client redirect fired
wherever discoverRelease ran in each caller's output. For resolve-first
commands (resources show/set, data list, seal) that's a clean leading
blank. But cluster info, data ingest, and data delete all print output
BEFORE resolving, so the self-lead landed mid-output — splitting cluster
info's Kubeconfig section between the server and namespace Fields, and
data ingest between "Connecting…" and its note (saadqbal review).
Thread a leadRedirect bool through discoverRelease / resolveClusterTarget:
true for resolve-first callers (redirect self-leads its one blank), false
for output-first callers (note stays inline, no mid-output blank —
restoring pre-PR spacing). resources set keeps NO pre-resolve Newline(),
so the #375 confirm/dry-run double-blank fix is preserved.
Add real MULTI-CLIENT tests for resources show and set that run through
the actual resolve seam (loadClusterFn/newClientsetFn, not the
resolveClusterTargetFn stub) so the redirect actually fires, asserting
exactly one leading blank. Fix the overclaiming comment on
TestShow_OpensWithSingleBlank (it only covers the single-client open).
Drop the redundant "\n\n" disjunct in the clustertarget_test assertion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
divyasinghds added a commit that referenced this pull request Aug 7, 2026
…#468)
* fix(#380): unify resolve-time leading blank across resources show/set
Removing the intro Banner (#375) left `resources show` and `resources set`
handling the leading blank differently, so the multi-client resolve-time
redirect line got a blank on show but not on set (Bugbot "Missing lead blank
on set path"). The naive fix — give set the same pre-resolve Newline() — would
re-introduce the #375 double-blank (it stacks with the self-leading confirm
PromptHint / dry-run Section).
Standardize the spacing at the source instead of per-caller:
- discoverRelease's multi-client redirect Infof now self-leads its own single
blank, so every command that hits it gets exactly one leading blank — no
per-command pre-resolve Newline() needed.
- resources show moves its leading blank from before resolveClusterTarget into
renderResources (before the first Stat), so it no longer stacks on the
now-self-leading redirect in the multi-client case.
- resources set keeps no command-level pre-resolve Newline() (preserves the
#375 double-blank fix); its self-leading confirm/dry-run paths are unchanged.
Tests: added a self-lead assertion to TestDiscoverRelease_ScanFindsSingleClient-
Elsewhere; refreshed the stale comment in TestShow_OpensWithSingleBlank (the
blank moved into renderResources). go build/test, gofmt -s, and go vet all pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(#380): make redirect leading blank conditional per caller (#468 review)
The self-lead blank in discoverRelease's multi-client redirect fired
wherever discoverRelease ran in each caller's output. For resolve-first
commands (resources show/set, data list, seal) that's a clean leading
blank. But cluster info, data ingest, and data delete all print output
BEFORE resolving, so the self-lead landed mid-output — splitting cluster
info's Kubeconfig section between the server and namespace Fields, and
data ingest between "Connecting…" and its note (saadqbal review).
Thread a leadRedirect bool through discoverRelease / resolveClusterTarget:
true for resolve-first callers (redirect self-leads its one blank), false
for output-first callers (note stays inline, no mid-output blank —
restoring pre-PR spacing). resources set keeps NO pre-resolve Newline(),
so the #375 confirm/dry-run double-blank fix is preserved.
Add real MULTI-CLIENT tests for resources show and set that run through
the actual resolve seam (loadClusterFn/newClientsetFn, not the
resolveClusterTargetFn stub) so the redirect actually fires, asserting
exactly one leading blank. Fix the overclaiming comment on
TestShow_OpensWithSingleBlank (it only covers the single-client open).
Drop the redundant "\n\n" disjunct in the clustertarget_test assertion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal