Skip to content

fix(#380): unify resolve-time leading blank across resources show/set - #468

Merged
divyasinghds merged 2 commits into
developfrom
fix/380-resolve-leading-blank
Aug 7, 2026
Merged

fix(#380): unify resolve-time leading blank across resources show/set#468
divyasinghds merged 2 commits into
developfrom
fix/380-resolve-leading-blank

Conversation

@divyasinghds

@divyasinghds divyasinghds commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes #380

Follow-up to #375 (banner removal). Removing the intro Banner left resources show and resources set handling the leading blank differently: in the multi-client case the resolve-time redirect line (discoverRelease's Infof) got a leading blank on show (which printed a pre-resolve Newline()) but not on set (Bugbot "Missing lead blank on set path"). The naive fix — give set the same pre-resolve Newline() — re-introduces the #375 double-blank, since a leading Newline() stacks with the self-leading confirm PromptHint / dry-run Section.

Changes

  1. discoverRelease (internal/cli/clustertarget.go) — the multi-client redirect Infof now self-leads its own single blank (p.Newline() immediately before it). Any command that hits the redirect now gets exactly one leading blank without a per-command pre-resolve Newline().
  2. resources show (internal/cli/resources.go) — moved the leading blank from before resolveClusterTarget in runResourcesShow to inside renderResources (before the first Stat), so it no longer stacks on the now-self-leading redirect in the multi-client case.
  3. resources set (internal/cli/resources_set.go) — unchanged: it already has no command-level pre-resolve Newline(), preserving the ui: remove the shared Banner method (no intro banner on any command) #375 double-blank fix; its self-leading confirm (PromptHint) / dry-run (Section) paths are untouched.

Acceptance

  • Multi-client resources set now opens with exactly one leading blank (self-leading redirect).
  • resources show (single + multi) still opens with exactly one blank — no double. Single-client: no redirect, the renderResources blank leads the view. Multi-client: one blank before the redirect, one before the view — no double-blank anywhere.
  • No regression to the ui: remove the shared Banner method (no intro banner on any command) #375 double-blank fix: set still has no pre-resolve Newline(); confirm/dry-run paths unchanged.
  • Other discoverRelease-resolving commands (data list/delete/ingest, cluster info, seal) still open with one leading blank — none had a pre-resolve Newline() to stack against, and cluster info's opening Section("Kubeconfig") self-leads as before; the now-self-leading redirect just gets its own separator on their multi-client paths.

Tests / validation

  • Added a self-lead assertion to TestDiscoverRelease_ScanFindsSingleClientElsewhere (redirect output must open with exactly one \n).
  • Refreshed the now-stale comment in TestShow_OpensWithSingleBlank (the blank moved into renderResources); the end-to-end single-blank assertion still holds.
  • go build ./..., go test ./... (all packages), gofmt -s -l, and go vet ./internal/cli/ all pass. Copy/string catalog goldens unchanged (no user-facing string changed).

🤖 Generated with Claude Code


Note

Low Risk
CLI presentation-only changes around newlines and discovery messaging; behavior and exit codes unchanged aside from spacing.

Overview
Fixes #380 (follow-up to #375): when cluster discovery retargets to another namespace, terminal output should have exactly one leading blank—not zero on resources set, and not a double when show printed a blank before resolve.

discoverRelease gains a leadRedirect flag. On a successful single-client scan redirect, it optionally prints p.Newline() immediately before the redirect Infof so resolve-first commands get one separator from the shell prompt without each command adding its own pre-resolve blank.

resolveClusterTarget threads leadRedirect through to discoverRelease. Callers pass true when resolve is the first output (resources show/set, data list, seal) and false when something already printed (cluster info kubeconfig section, data ingest “Connecting…”, data delete warning).

resources show drops the pre-resolve Newline() in runResourcesShow and moves the view’s opening blank into renderResources (before the first Stat) so it does not stack with the self-leading redirect on the multi-client path.

resources set keeps no pre-resolve blank; leadRedirect=true supplies the multi-client case while confirm/dry-run paths still self-lead.

Tests add §380 assertions (redirect opens with \n , no \n\n) and end-to-end TestShow_MultiClientRedirectOpensWithSingleBlank / TestSet_MultiClientRedirectOpensWithSingleBlank through real resolve seams.

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

@saadqbal
saadqbal self-requested a review August 7, 2026 10:02
saadqbal
saadqbal previously approved these changes Aug 7, 2026

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean, well-reasoned spacing fix 👍 Centralizing the redirect blank in discoverRelease so every caller self-leads is the right altitude. A few non-blocking nits inline — the one worth a look is that the single-blank tests don't actually exercise the multi-client redirect path they exist to protect.

Comment thread internal/cli/resources_test.go Outdated
Comment thread internal/cli/clustertarget.go Outdated
Comment thread internal/cli/clustertarget_test.go Outdated
divyasinghds and others added 2 commits August 7, 2026 15:56
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>
…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
divyasinghds merged commit 423c80f into develop Aug 7, 2026
25 checks passed
@divyasinghds
divyasinghds deleted the fix/380-resolve-leading-blank branch August 7, 2026 10:57
Sign up for free to 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.

resources: unify resolve-time leading blank across show and set

3 participants