Skip to content

fix(banner): preserve color when picker is cancelled - #310

Merged
Alan-TheGentleman merged 1 commit into
Gentleman-Programming:mainfrom
barbatdev:fix/issue-301-banner-color-cancel
Aug 15, 2026
Merged

Alan-TheGentleman merged 1 commit into
Gentleman-Programming:mainfrom
barbatdev:fix/issue-301-banner-color-cancel

Conversation

@barbatdev

@barbatdev barbatdev commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Closes #301

Summary

  • Treat banner color picker cancellation as a no-op in both command flows.
  • Preserve the previously saved color without rewriting banner.json or showing a misleading notification.
  • Add runtime coverage for direct, invalid-argument, and nested picker cancellation.

Changes

File Change
extensions/startup-banner.ts Return before persistence and notification when a color picker is cancelled.
tests/runtime-harness.mjs Verify saved colors survive cancellation across both entry points.

Test plan

  • pnpm run test:harness
  • git diff --check
  • Independent verification passed.
  • Native four-lens review completed without severe findings.
  • Pre-commit, pre-push, and pre-PR gates validated the reviewed candidate.

PR type

  • Bug fix
  • New feature
  • Documentation only
  • Code refactoring
  • Maintenance/tooling
  • Breaking change

Contributor checklist

  • Linked an approved issue.
  • Uses one type:* label.
  • Tests are included with the behavior change.
  • No documentation update is required for unchanged command syntax.
  • Commit follows Conventional Commits.
  • No AI attribution or co-author trailer.

Out of scope

Banner defaults, rendering, persistence schema, and command syntax remain unchanged.

Summary by CodeRabbit

  • Bug Fixes

    • Cancelling the banner color selector no longer changes the saved configuration.
    • Invalid or missing color values now open the selector safely.
    • Directly selected valid colors continue to apply as expected.
  • Tests

    • Added coverage for cancellation scenarios, unchanged settings, notifications, and selector behavior.

@barbatdev barbatdev added the type:bug Bug fix label Aug 14, 2026
@coderabbitai

coderabbitai Bot commented Aug 14, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0c64766d-aa5b-45e6-a0d2-e0b6bd1d595d

📥 Commits

Reviewing files that changed from the base of the PR and between 19b0ed7 and 995e7bf.

📒 Files selected for processing (2)
  • extensions/startup-banner.ts
  • tests/runtime-harness.mjs

📝 Walkthrough

Walkthrough

The banner menu and color command now treat cancelled color selection as a no-op. Runtime harness tests cover direct, invalid-argument, and nested picker cancellation.

Changes

Banner color cancellation

Layer / File(s) Summary
Handle cancelled color selection
extensions/startup-banner.ts
The banner menu and color command now apply a color only when selection succeeds. Valid command arguments are applied directly. Invalid or absent arguments open the picker.
Validate cancellation behavior
tests/runtime-harness.mjs
Tests verify unchanged configuration, no notifications, expected picker counts, and semantic configuration round-trips across cancellation flows.

Estimated code review effort: 2 (Simple) | ~10 minutes

Mergeability Score: ⚪ Minimal · up to 995e7

The change makes picker cancellation a no-op so saved banner colors are preserved without unnecessary persistence or notifications; no actionable merge-blocking risk remains after normal checks.

Suggested reviewers: alan-thegentleman

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the fix for preserving the banner color when the picker is cancelled.
Linked Issues check ✅ Passed The changes address issue #301 in both banner color flows and add regression coverage for preserved configuration and suppressed notifications.
Out of Scope Changes check ✅ Passed All changes are limited to the banner cancellation fix and its runtime regression tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Alan-TheGentleman
Alan-TheGentleman merged commit bbddc8d into Gentleman-Programming:main Aug 15, 2026
2 checks passed
@Alan-TheGentleman

Copy link
Copy Markdown
Collaborator

Merged. Verified the bug-catching power of the new harness block rather than trusting it: reverted extensions/startup-banner.ts to main and the block failed with banner-color cancel must not rewrite banner.json — so it genuinely pins the regression, and the picker-open counts (1, 1, 2) mean a 'fix' that just stopped opening the picker wouldn't pass either.

Nice catch on the root cause: await ctx.ui.select(...) as BannerColor swallowing the cancel into undefined, then JSON.stringify dropping the key and the next read normalizing to the default — a silent destroy of a non-default choice. The falsy guard is safe here because every BANNER_COLORS entry is a non-empty string.

Post-merge with current main: harness exit 0, 1113 pass / 0 fail / 1 pre-existing skip. Thanks 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(startup): cancelling the banner color picker resets the saved color

2 participants