From da20eb2fdb7fb2a85526f05df0708ea469c82de5 Mon Sep 17 00:00:00 2001 From: the_og Date: Mon, 20 Jul 2026 12:11:49 -0400 Subject: [PATCH 1/2] fix: tolerate non-object server values on load; keep named sets across a merge Two silent-failure bugs found auditing the v1.3.0 features. #72 -- extract_servers called dict() on every server value, so a config that was valid JSON but held a non-object server ("foo": "oops", a number, a list, null) raised on load. The strict parse succeeded, so the repair path never saw it, and the call sat outside the load try/except: an unhandled traceback with the window half-swapped to the new profile. It also made the #54 schema lint unreachable for the most likely hand-edit mistake -- the load died before the linter ran. Malformed values are now preserved verbatim on ServerEntry.raw (behind a NO_RAW sentinel, since a literal JSON null is itself a malformed entry worth keeping) and written back untouched on Save, so nothing is silently deleted. lint_servers names the offending entry instead. #73 -- the stale-file "Merge & save" path reloaded the file from disk and re-applied the user's servers, but apply_servers only writes mcpServers and _disabledMcpServers. Named server sets live under _bccServerSets in the same file, so a set saved that session was dropped from disk and then from memory, with no warning, on the path the user picks because it sounds like the safe one. BCC-owned keys are now declared in BCC_OWNED_KEYS and carried across by carry_owned_keys, which reports genuinely contested keys so the status line can say so. Deliberately one-directional: a key absent locally is left alone on disk, because 'user deleted their last set' and 'another machine just added sets' are indistinguishable and deleting someone else's data is the worse failure. Closes #72 Closes #73 --- bcc.py | 30 ++++++++-- bcc_core.py | 123 +++++++++++++++++++++++++++++++++++++-- tests/test_core.py | 140 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 10 deletions(-) diff --git a/bcc.py b/bcc.py index c71da94..c4836ff 100644 --- a/bcc.py +++ b/bcc.py @@ -1939,9 +1939,21 @@ class MainWindow(QMainWindow): return self.full_config = cfg repaired = True + # extract_servers tolerates malformed entries rather than raising (#72), + # but keep it inside the guard: a load failure must leave the previously + # loaded profile intact instead of half-swapping the window's state. + try: + servers = core.extract_servers(self.full_config) + except Exception as exc: # pragma: no cover - defence in depth + QMessageBox.critical( + self, + "Could not read config", + f"{profile.path}\n\nThe server list couldn't be read: {exc}", + ) + return self._loaded_stat = core.config_fingerprint(profile.path) self.current_profile = profile - self.servers = core.extract_servers(self.full_config) + self.servers = servers self.dirty = False self.restart_btn.hide() self._undo_stack.clear() @@ -2264,7 +2276,7 @@ class MainWindow(QMainWindow): entry = self.servers[idx] old_name = entry.name entry.name = self.editor.current_name() - entry.data = self.editor.dump_data() + entry.set_data(self.editor.dump_data()) # The server stays in its section (enable state unchanged), so update # its existing row in place rather than re-rendering. # An edit invalidates any cached "Test all" result -- the server that @@ -2358,7 +2370,7 @@ class MainWindow(QMainWindow): QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No, ) if ans == QMessageBox.StandardButton.Yes: - self.servers[existing[name]].data = data + self.servers[existing[name]].set_data(data) return False, True name = core.resolve_name_collision(name, {s.name for s in self.servers}) self.servers.append(core.ServerEntry(name, data, True)) @@ -2478,6 +2490,11 @@ class MainWindow(QMainWindow): except Exception as e: QMessageBox.critical(self, "Reload failed", str(e)) return + # The reload above is the on-disk truth for everything the user + # didn't touch -- but it also wipes BCC-authored keys the user + # changed in this session (named sets), which apply_servers + # doesn't write. Carry them over before saving (#73). + contested = core.carry_owned_keys(self.full_config, fresh) core.apply_servers(fresh, self.servers) try: backup = core.write_config(self.current_profile.path, fresh) @@ -2490,8 +2507,13 @@ class MainWindow(QMainWindow): self.dirty = False self.save_btn.setEnabled(False) bnote = f" · backup: {backup.name}" if backup else " · (new file)" + cnote = ( + f" · kept your {', '.join(contested)} (the file on disk had a different copy)" + if contested + else "" + ) self.status.setText( - f"Merged & saved {self.current_profile.path}{bnote}" + f"Merged & saved {self.current_profile.path}{bnote}{cnote}" f" · Restart {self.current_profile.label} to apply." ) self._offer_restart_button() diff --git a/bcc_core.py b/bcc_core.py index c81d5ea..faa6ad5 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -15,6 +15,7 @@ from __future__ import annotations import base64 import contextlib +import copy import difflib import functools import glob @@ -49,6 +50,12 @@ DISABLED_KEY = "_disabledMcpServers" # parks the rest under DISABLED_KEY. SETS_KEY = "_bccServerSets" +# Top-level keys BCC itself authors. They live in the client's config file, but +# BCC is their owner, so on a stale-file merge the in-memory copy wins over the +# on-disk one (see `carry_owned_keys`). Any future BCC-authored key belongs +# here -- forgetting to add one is exactly how #73 happened. +BCC_OWNED_KEYS = (SETS_KEY,) + BACKUP_DIRNAME = ".bcc_backups" MAX_BACKUPS = 15 @@ -178,16 +185,63 @@ class Profile: self.path = Path(self.path) +class _NoRaw: + """Sentinel for ServerEntry.raw. + + `None` can't do this job: `{"mcpServers": {"foo": null}}` is legal JSON and + a real malformed-entry case, so None has to mean "the config said null", + not "there was nothing here". + """ + + __slots__ = () + + def __repr__(self) -> str: # keeps ServerEntry reprs readable in test output + return "" + + +NO_RAW = _NoRaw() + + @dataclass class ServerEntry: + """One server definition. + + `data` is always a dict so every consumer can treat it as one. When the + config held something that wasn't a JSON object for this server (a string, + a number, a list -- all legal JSON, all wrong here), `data` is empty and + the original value is preserved verbatim in `raw` so Save round-trips it + instead of silently deleting the user's line. `lint_servers` surfaces it. + `raw` defaults to the NO_RAW sentinel rather than None, because a config + value of literal `null` is itself a malformed entry worth preserving. + + Assigning `data` means the user replaced the definition through the editor, + which retires `raw` -- use `set_data` so that can't be forgotten. + """ + name: str data: dict enabled: bool = True + raw: object = NO_RAW @property def kind(self) -> str: return "remote" if "url" in self.data and "command" not in self.data else "stdio" + @property + def malformed(self) -> bool: + """True when the config value for this server wasn't a JSON object.""" + return self.raw is not NO_RAW + + def set_data(self, data: dict) -> None: + """Replace the definition from the editor, clearing any malformed original.""" + self.data = data + self.raw = NO_RAW + + def config_value(self): + """What to write back to the config: the edited dict, or the untouched + malformed original when the user never edited it.""" + return self.data if self.raw is NO_RAW else self.raw + # --------------------------------------------------------------------------- # # Discovery @@ -452,13 +506,30 @@ def repair_config_file(path: str | os.PathLike) -> tuple[dict, list[str], str]: return obj, notes, pretty +def _server_entry(name: str, data, enabled: bool) -> ServerEntry: + """Build a ServerEntry, tolerating a value that isn't a JSON object. + + A hand-edited config can legally hold `{"mcpServers": {"foo": "oops"}}` -- + valid JSON, wrong shape. Calling dict() on that raises, which used to take + the whole load down before the linter ever got a look at it (#72). Keep the + original instead and let the linter report it. + """ + if isinstance(data, dict): + return ServerEntry(name=name, data=dict(data), enabled=enabled) + return ServerEntry(name=name, data={}, enabled=enabled, raw=data) + + def extract_servers(cfg: dict) -> list[ServerEntry]: - """Pull enabled (`mcpServers`) and disabled (`_disabledMcpServers`) servers.""" + """Pull enabled (`mcpServers`) and disabled (`_disabledMcpServers`) servers. + + Never raises on a structurally-odd config -- malformed entries come back as + empty-data entries carrying their original value (see `_server_entry`). + """ out: list[ServerEntry] = [] for name, data in (cfg.get("mcpServers") or {}).items(): - out.append(ServerEntry(name=name, data=dict(data), enabled=True)) + out.append(_server_entry(name, data, True)) for name, data in (cfg.get(DISABLED_KEY) or {}).items(): - out.append(ServerEntry(name=name, data=dict(data), enabled=False)) + out.append(_server_entry(name, data, False)) return out @@ -550,8 +621,8 @@ def apply_servers(cfg: dict, servers: list[ServerEntry]) -> dict: Write the server list back into `cfg` in place, preserving every other key and the position of `mcpServers`. Returns the same dict for convenience. """ - enabled = {s.name: s.data for s in servers if s.enabled} - disabled = {s.name: s.data for s in servers if not s.enabled} + enabled = {s.name: s.config_value() for s in servers if s.enabled} + disabled = {s.name: s.config_value() for s in servers if not s.enabled} cfg["mcpServers"] = enabled # replaces value if key existed; appends otherwise if disabled: @@ -564,6 +635,34 @@ def apply_servers(cfg: dict, servers: list[ServerEntry]) -> dict: # --------------------------------------------------------------------------- # # Write (atomic, with rotating backups) # --------------------------------------------------------------------------- # +def carry_owned_keys(local_cfg: dict, fresh_cfg: dict) -> list[str]: + """Carry BCC-authored top-level keys from `local_cfg` onto `fresh_cfg`. + + Used by the stale-file "Merge & save" path, which reloads the file from + disk and re-applies the user's server edits. That reload used to drop + anything BCC owns but `apply_servers` doesn't write -- named server sets + vanished without a word (#73). BCC owns these keys, so the in-memory copy + wins; mutates `fresh_cfg` in place. + + Returns the keys where the on-disk copy differed and was overwritten, so + the caller can tell the user something was actually contested rather than + merely carried across. + + Deliberately one-directional: a key absent locally is left alone on disk. + We can't tell "user deleted their last set" from "user never had sets and + another machine just added some", and silently deleting someone else's + data is the worse of the two failures. + """ + conflicts: list[str] = [] + for key in BCC_OWNED_KEYS: + if key not in local_cfg: + continue + if key in fresh_cfg and fresh_cfg[key] != local_cfg[key]: + conflicts.append(key) + fresh_cfg[key] = copy.deepcopy(local_cfg[key]) + return conflicts + + def _make_backup(path: Path) -> Path: bdir = path.parent / BACKUP_DIRNAME bdir.mkdir(exist_ok=True) @@ -1380,9 +1479,21 @@ def lint_server(name: str, data: dict) -> list[str]: def lint_servers(servers: list[ServerEntry]) -> list[str]: - """Concatenate lint_server warnings across every entry, in order.""" + """Concatenate lint_server warnings across every entry, in order. + + Entries whose config value wasn't a JSON object at all are reported here + rather than in lint_server, which takes an already-dict `data` (#72). + """ out: list[str] = [] for s in servers: + if s.malformed: + nm = s.name.strip() or "(unnamed)" + out.append( + f"'{nm}': server definition is not an object " + f"(found {type(s.raw).__name__}) -- it is preserved as-is; " + f"edit it to replace it with a proper definition" + ) + continue out.extend(lint_server(s.name, s.data)) return out diff --git a/tests/test_core.py b/tests/test_core.py index 725358f..e44dc32 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2370,3 +2370,143 @@ def test_config_has_unfilled_placeholders_false_after_fill(): def test_config_has_unfilled_placeholders_checks_env_too(): cfg = {"command": "uvx", "args": ["mcp-grafana"], "env": {"GRAFANA_URL": ""}} assert c.config_has_unfilled_placeholders(cfg) is True + + +# --------------------------------------------------------------------------- # +# #72 -- a server value that isn't a JSON object must not take the load down +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize("bad", ["not-a-dict", 123, ["a", "b"], None, True, 1.5]) +def test_extract_servers_survives_non_dict_server_value(bad): + entries = c.extract_servers({"mcpServers": {"foo": bad}}) + assert len(entries) == 1 + assert entries[0].name == "foo" + assert entries[0].data == {} + assert entries[0].malformed is True + assert entries[0].raw == bad + + +def test_extract_servers_marks_only_the_bad_entry(): + cfg = {"mcpServers": {"good": {"command": "npx"}, "bad": "oops"}} + by_name = {e.name: e for e in c.extract_servers(cfg)} + assert by_name["good"].malformed is False + assert by_name["good"].data == {"command": "npx"} + assert by_name["bad"].malformed is True + + +def test_extract_servers_handles_malformed_disabled_entry(): + entries = c.extract_servers({c.DISABLED_KEY: {"parked": ["nope"]}}) + assert entries[0].enabled is False + assert entries[0].malformed is True + + +def test_malformed_entry_round_trips_through_save_unchanged(): + """The cardinal rule: never silently delete what the user had on disk.""" + cfg = {"mcpServers": {"good": {"command": "npx"}, "bad": "oops"}} + servers = c.extract_servers(cfg) + out = c.apply_servers(dict(cfg), servers) + assert out["mcpServers"]["bad"] == "oops" + assert out["mcpServers"]["good"] == {"command": "npx"} + + +def test_editing_a_malformed_entry_retires_the_raw_value(): + entry = c.extract_servers({"mcpServers": {"bad": "oops"}})[0] + entry.set_data({"command": "npx"}) + assert entry.malformed is False + assert entry.config_value() == {"command": "npx"} + assert c.apply_servers({}, [entry])["mcpServers"]["bad"] == {"command": "npx"} + + +def test_lint_reports_the_malformed_entry_by_name(): + servers = c.extract_servers({"mcpServers": {"bad": "oops"}}) + warnings = c.lint_servers(servers) + assert len(warnings) == 1 + assert "'bad'" in warnings[0] + assert "not an object" in warnings[0] + assert "str" in warnings[0] + + +def test_lint_still_reports_normal_warnings_alongside_malformed(): + cfg = {"mcpServers": {"bad": "oops", "sloppy": {"command": "npx", "args": "one two"}}} + warnings = c.lint_servers(c.extract_servers(cfg)) + assert any("not an object" in w for w in warnings) + assert any("'args' should be a list" in w for w in warnings) + + +# --------------------------------------------------------------------------- # +# #73 -- the stale-file merge must not discard BCC-authored keys +# --------------------------------------------------------------------------- # +def test_carry_owned_keys_moves_sets_onto_the_reloaded_config(): + local = {"mcpServers": {}, c.SETS_KEY: {"work": ["a", "b"]}} + fresh = {"mcpServers": {"external": {"command": "npx"}}} + contested = c.carry_owned_keys(local, fresh) + assert contested == [] + assert fresh[c.SETS_KEY] == {"work": ["a", "b"]} + assert fresh["mcpServers"] == {"external": {"command": "npx"}} + + +def test_carry_owned_keys_reports_a_genuine_conflict(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {c.SETS_KEY: {"work": ["a", "b"]}} + assert c.carry_owned_keys(local, fresh) == [c.SETS_KEY] + assert fresh[c.SETS_KEY] == {"work": ["a"]} # local wins: BCC owns the key + + +def test_carry_owned_keys_is_quiet_when_both_sides_agree(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {c.SETS_KEY: {"work": ["a"]}} + assert c.carry_owned_keys(local, fresh) == [] + + +def test_carry_owned_keys_leaves_disk_alone_when_absent_locally(): + """Can't distinguish 'deleted my last set' from 'never had sets'; keep theirs.""" + fresh = {c.SETS_KEY: {"remote": ["a"]}} + assert c.carry_owned_keys({}, fresh) == [] + assert fresh[c.SETS_KEY] == {"remote": ["a"]} + + +def test_carry_owned_keys_deep_copies_so_later_edits_do_not_leak(): + local = {c.SETS_KEY: {"work": ["a"]}} + fresh = {} + c.carry_owned_keys(local, fresh) + local[c.SETS_KEY]["work"].append("b") + assert fresh[c.SETS_KEY] == {"work": ["a"]} + + +def test_merge_flow_preserves_sets_and_external_servers(tmp_path): + """End-to-end shape of the Merge & save path that lost sets in #73.""" + path = tmp_path / "claude.json" + path.write_text(json.dumps({"mcpServers": {"old": {"command": "old"}}})) + + # BCC loads, user saves a named set and edits servers in memory. + local = c.load_config(path) + servers = c.extract_servers(local) + c.save_server_set(local, "work", servers) + + # Something else rewrites the file underneath us. + path.write_text(json.dumps({"mcpServers": {"external": {"command": "new"}}, "other": 1})) + + # Merge & save: reload disk, carry BCC keys, re-apply the user's servers. + fresh = c.load_config(path) + c.carry_owned_keys(local, fresh) + c.apply_servers(fresh, servers) + c.write_config(path, fresh) + + saved = c.load_config(path) + assert saved[c.SETS_KEY] == {"work": ["old"]} # the set survived + assert saved["other"] == 1 # unrelated external key preserved + assert "old" in saved["mcpServers"] # user's servers re-applied + + +def test_null_server_value_is_malformed_not_mistaken_for_absent(): + """`{"mcpServers": {"foo": null}}` is legal JSON and a real malformed case, + so None must not double as the 'nothing here' sentinel.""" + entry = c.extract_servers({"mcpServers": {"foo": None}})[0] + assert entry.malformed is True + assert entry.raw is None + assert c.apply_servers({}, [entry])["mcpServers"]["foo"] is None + + +def test_a_normal_entry_is_not_malformed(): + entry = c.extract_servers({"mcpServers": {"foo": {"command": "npx"}}})[0] + assert entry.malformed is False + assert entry.raw is c.NO_RAW From febd617c56a8ec653f8c6e8e0a2dd1c3e4333817 Mon Sep 17 00:00:00 2001 From: the_og Date: Mon, 20 Jul 2026 12:21:45 -0400 Subject: [PATCH 2/2] feat: light theme + system-following, with the dark theme preserved exactly BCC has always been dark-only -- BG #1b1d23, hardcoded at import time, with no light option and no awareness of the desktop's appearance. On a light desktop it matches nothing else on screen and there was no way to change it. Adds a Palette value type in bcc_core with DARK (byte-identical to the colours v1.3.0 shipped) and a new LIGHT, plus resolve_theme(setting, system_is_dark) so the decision is testable without a Qt app. View > Theme offers Match system / Light / Dark, persisted in QSettings under ui/theme, defaulting to following the system. The light palette's semantic colours are deliberately not the dark ones lightened: #4ade80 sits near 1.7:1 against white. They are darkened to clear WCAG AA, and a contrast test enforces >= 4.5:1 for every text colour against its surface in both palettes so nobody harmonises them back later. Three near-black literals were baked into the stylesheet (#1a1205 on-accent text, #202229 disabled table, #16181d diagnostics pane). Fine with one theme, invisible breakage with two -- each now has a palette slot, and a test asserts build_stylesheet contains no hex literals at all. The ~20 inline setStyleSheet(f"color: {MUTED}") call sites are left alone: apply_palette rebinds the module-level colour names, and an f-string resolves its names when it runs, so each call site picks up the new colour on its next render. Switching theme reapplies the global QSS and re-renders the inline-styled widgets, so nothing is left dark-on-light. Refs #75 --- bcc.py | 200 +++++++++++++++++++++++++++++++++------------ bcc_core.py | 132 ++++++++++++++++++++++++++++++ tests/test_core.py | 116 ++++++++++++++++++++++++++ 3 files changed, 397 insertions(+), 51 deletions(-) diff --git a/bcc.py b/bcc.py index c71da94..97dd989 100644 --- a/bcc.py +++ b/bcc.py @@ -18,6 +18,7 @@ from typing import ClassVar from PySide6.QtCore import QRect, QSettings, QSize, Qt, QThread, QTimer, QUrl, Signal from PySide6.QtGui import ( QAction, + QActionGroup, QColor, QCursor, QDesktopServices, @@ -25,6 +26,7 @@ from PySide6.QtGui import ( QIcon, QKeySequence, QPainter, + QPalette, QPixmap, ) from PySide6.QtWidgets import ( @@ -65,85 +67,118 @@ import bcc_core as core # thread during drag-and-drop import, so skip anything larger than this. MAX_DROP_IMPORT_BYTES = 5 * 1024 * 1024 # 5 MB -# --- One-line rebrand: change this to recolor the whole app --------------- # -ACCENT = "#f97316" # warm orange -ACCENT_DIM = "#c2570b" -BG = "#1b1d23" -PANEL = "#23262e" -PANEL_2 = "#2b2f39" -TEXT = "#e7e9ee" -MUTED = "#9aa0ad" -BORDER = "#3a3f4b" -GOOD = "#4ade80" -BAD = "#f87171" -WARN = "#fbbf24" +# --- Theming (issue #75) -------------------------------------------------- # +# The palette lives in bcc_core (testable without a Qt app); these module-level +# names are rebound by `apply_palette()` whenever the theme changes. +# +# Why globals rather than passing a palette around: ~20 inline +# `setStyleSheet(f"color: {MUTED}")` calls are scattered through this file, and +# an f-string resolves its names when it runs, not when it's compiled. Rebinding +# the globals means every one of those call sites picks up the new colour on its +# next render, with no change to the call sites themselves. +PALETTE = core.DARK_PALETTE +ACCENT = ACCENT_DIM = BG = PANEL = PANEL_2 = TEXT = MUTED = BORDER = "" +GOOD = BAD = WARN = REMOTE = ON_ACCENT = DISABLED_BG = MONO_BG = SEL_TEXT = "" +STATUS_COLORS: dict[str, str] = {} +HEALTH_COLORS: dict[str, str] = {} -STATUS_COLORS = {"ok": GOOD, "missing": BAD, "warn": WARN, "remote": "#60a5fa", "unknown": WARN} -STATUS_GLYPH = {"ok": "●", "missing": "●", "warn": "▲", "remote": "◆", "unknown": "○"} +STATUS_GLYPH = { + "ok": "\u25cf", + "missing": "\u25cf", + "warn": "\u25b2", + "remote": "\u25c6", + "unknown": "\u25cb", +} # Health dot (spawn-test outcome, see core.HealthStatus) shown per row in the # server tables' "Health" column -- distinct from the PATH-dependency Status # column above. -HEALTH_COLORS = {"ok": GOOD, "failed": BAD, "untested": MUTED} -HEALTH_GLYPH = {"ok": "●", "failed": "●", "untested": "○"} +HEALTH_GLYPH = {"ok": "\u25cf", "failed": "\u25cf", "untested": "\u25cb"} -STYLESHEET = f""" + +def build_stylesheet(p: core.Palette) -> str: + """Render the global QSS for a palette.""" + return f""" /* No font-family here on purpose: Qt already uses the native system UI font on every platform (San Francisco / Segoe UI / desktop default). Naming web-CSS aliases like -apple-system forces a costly font-alias scan. */ -* {{ font-size: 13px; color: {TEXT}; }} -QMainWindow, QDialog {{ background: {BG}; }} +* {{ font-size: 13px; color: {p.text}; }} +QMainWindow, QDialog {{ background: {p.bg}; }} QLabel#h1 {{ font-size: 15px; font-weight: 600; }} -QLabel#muted {{ color: {MUTED}; }} -QFrame#card {{ background: {PANEL}; border: 1px solid {BORDER}; border-radius: 10px; }} +QLabel#muted {{ color: {p.muted}; }} +QFrame#card {{ background: {p.panel}; border: 1px solid {p.border}; border-radius: 10px; }} QLineEdit, QPlainTextEdit, QComboBox {{ - background: {PANEL_2}; border: 1px solid {BORDER}; border-radius: 7px; - padding: 6px 8px; selection-background-color: {ACCENT}; selection-color: #1a1205; + background: {p.panel_2}; border: 1px solid {p.border}; border-radius: 7px; + padding: 6px 8px; selection-background-color: {p.accent}; selection-color: {p.on_accent}; }} -QLineEdit:focus, QPlainTextEdit:focus, QComboBox:focus {{ border: 1px solid {ACCENT}; }} +QLineEdit:focus, QPlainTextEdit:focus, QComboBox:focus {{ border: 1px solid {p.accent}; }} QComboBox::drop-down {{ border: none; width: 22px; }} -QComboBox QAbstractItemView {{ background: {PANEL_2}; border: 1px solid {BORDER}; - selection-background-color: {ACCENT}; outline: none; }} -QPushButton {{ background: {PANEL_2}; border: 1px solid {BORDER}; border-radius: 7px; +QComboBox QAbstractItemView {{ background: {p.panel_2}; border: 1px solid {p.border}; + selection-background-color: {p.accent}; outline: none; }} +QPushButton {{ background: {p.panel_2}; border: 1px solid {p.border}; border-radius: 7px; padding: 7px 13px; }} -QPushButton:hover {{ border: 1px solid {ACCENT}; }} -QPushButton:disabled {{ color: {MUTED}; background: {PANEL}; }} -QPushButton#primary {{ background: {ACCENT}; border: 1px solid {ACCENT}; color: #1a1205; font-weight: 600; }} -QPushButton#primary:hover {{ background: {ACCENT_DIM}; }} -QPushButton#primary:disabled {{ background: {PANEL}; color: {MUTED}; border: 1px solid {BORDER}; }} -QPushButton#danger:hover {{ border: 1px solid {BAD}; color: {BAD}; }} -QTableWidget {{ background: {PANEL}; border: 1px solid {BORDER}; border-radius: 10px; +QPushButton:hover {{ border: 1px solid {p.accent}; }} +QPushButton:disabled {{ color: {p.muted}; background: {p.panel}; }} +QPushButton#primary {{ background: {p.accent}; border: 1px solid {p.accent}; color: {p.on_accent}; font-weight: 600; }} +QPushButton#primary:hover {{ background: {p.accent_dim}; }} +QPushButton#primary:disabled {{ background: {p.panel}; color: {p.muted}; border: 1px solid {p.border}; }} +QPushButton#danger:hover {{ border: 1px solid {p.bad}; color: {p.bad}; }} +QTableWidget {{ background: {p.panel}; border: 1px solid {p.border}; border-radius: 10px; gridline-color: transparent; outline: none; }} QTableWidget::item {{ padding: 6px 8px; border: none; }} -QTableWidget::item:selected {{ background: {ACCENT}; color: #1a1205; }} +QTableWidget::item:selected {{ background: {p.accent}; color: {p.on_accent}; }} /* Inline cell editors: the global QLineEdit padding/radius clips the text inside a table row, so give editors a compact, flat style instead. */ QTableWidget QLineEdit {{ - background: {PANEL_2}; color: {TEXT}; border: 1px solid {ACCENT}; + background: {p.panel_2}; color: {p.text}; border: 1px solid {p.accent}; border-radius: 3px; padding: 0px 4px; margin: 0px; - selection-background-color: {ACCENT_DIM}; selection-color: #ffffff; + selection-background-color: {p.accent_dim}; selection-color: {p.selection_text}; }} -QHeaderView::section {{ background: {PANEL}; color: {MUTED}; border: none; - border-bottom: 1px solid {BORDER}; padding: 8px; font-weight: 600; }} +QHeaderView::section {{ background: {p.panel}; color: {p.muted}; border: none; + border-bottom: 1px solid {p.border}; padding: 8px; font-weight: 600; }} QScrollBar:vertical {{ background: transparent; width: 10px; margin: 2px; }} -QScrollBar::handle:vertical {{ background: {BORDER}; border-radius: 5px; min-height: 24px; }} +QScrollBar::handle:vertical {{ background: {p.border}; border-radius: 5px; min-height: 24px; }} QScrollBar::add-line, QScrollBar::sub-line {{ height: 0; }} -QLabel#statusbar {{ color: {MUTED}; padding: 4px 2px; }} -QLabel#warnBanner {{ color: #1a1205; background: {WARN}; border-radius: 8px; padding: 8px 10px; font-weight: 600; }} -QLabel#section {{ color: {MUTED}; font-weight: 600; font-size: 12px; padding: 2px 2px; }} -QLabel#sectionDisabled {{ color: {MUTED}; font-weight: 600; font-size: 12px; padding: 2px 2px; }} -QLabel#placeholder {{ color: {MUTED}; padding: 12px; background: {PANEL_2}; border: 1px dashed {BORDER}; border-radius: 8px; }} -QTableWidget#disabledTable {{ background: #202229; }} -QTableWidget#disabledTable::item:selected {{ background: {ACCENT}; color: #1a1205; }} +QLabel#statusbar {{ color: {p.muted}; padding: 4px 2px; }} +QLabel#warnBanner {{ color: {p.on_accent}; background: {p.warn}; border-radius: 8px; padding: 8px 10px; font-weight: 600; }} +QLabel#section {{ color: {p.muted}; font-weight: 600; font-size: 12px; padding: 2px 2px; }} +QLabel#sectionDisabled {{ color: {p.muted}; font-weight: 600; font-size: 12px; padding: 2px 2px; }} +QLabel#placeholder {{ color: {p.muted}; padding: 12px; background: {p.panel_2}; border: 1px dashed {p.border}; border-radius: 8px; }} +QTableWidget#disabledTable {{ background: {p.disabled_bg}; }} +QTableWidget#disabledTable::item:selected {{ background: {p.accent}; color: {p.on_accent}; }} QPlainTextEdit#diag {{ font-family: "Menlo", "Cascadia Code", "Consolas", "DejaVu Sans Mono", monospace; - font-size: 12px; background: #16181d; border: 1px solid {BORDER}; border-radius: 8px; }} + font-size: 12px; background: {p.mono_bg}; border: 1px solid {p.border}; border-radius: 8px; }} QFrame#diagCard {{ background: transparent; border: none; }} QSplitter::handle {{ background: transparent; }} -QSplitter::handle:hover {{ background: {BORDER}; border-radius: 4px; }} -QSplitter::handle:pressed {{ background: {ACCENT}; border-radius: 4px; }} +QSplitter::handle:hover {{ background: {p.border}; border-radius: 4px; }} +QSplitter::handle:pressed {{ background: {p.accent}; border-radius: 4px; }} """ +def apply_palette(p: core.Palette) -> str: + """Rebind the module-level colour names to `p` and return its stylesheet.""" + global PALETTE, ACCENT, ACCENT_DIM, BG, PANEL, PANEL_2, TEXT, MUTED, BORDER + global GOOD, BAD, WARN, REMOTE, ON_ACCENT, DISABLED_BG, MONO_BG, SEL_TEXT + global STATUS_COLORS, HEALTH_COLORS + PALETTE = p + ACCENT, ACCENT_DIM = p.accent, p.accent_dim + BG, PANEL, PANEL_2 = p.bg, p.panel, p.panel_2 + TEXT, MUTED, BORDER = p.text, p.muted, p.border + GOOD, BAD, WARN, REMOTE = p.good, p.bad, p.warn, p.remote + ON_ACCENT, DISABLED_BG, MONO_BG, SEL_TEXT = ( + p.on_accent, + p.disabled_bg, + p.mono_bg, + p.selection_text, + ) + STATUS_COLORS = {"ok": GOOD, "missing": BAD, "warn": WARN, "remote": REMOTE, "unknown": WARN} + HEALTH_COLORS = {"ok": GOOD, "failed": BAD, "untested": MUTED} + return build_stylesheet(p) + + +STYLESHEET = apply_palette(core.DARK_PALETTE) + + # --------------------------------------------------------------------------- # # Background reachability tester (keeps the UI responsive during the request) # --------------------------------------------------------------------------- # @@ -1585,11 +1620,47 @@ class MainWindow(QMainWindow): # --- menu bar ---------------------------------------------------------- # def _build_menu_bar(self): + view_menu = self.menuBar().addMenu("&View") + theme_menu = view_menu.addMenu("Theme") + self._theme_group = QActionGroup(self) + self._theme_group.setExclusive(True) + current = stored_theme_setting() + for setting, label in ( + (core.THEME_SYSTEM, "Match system"), + (core.THEME_LIGHT, "Light"), + (core.THEME_DARK, "Dark"), + ): + act = QAction(label, self, checkable=True) + act.setChecked(setting == current) + act.triggered.connect(lambda _checked=False, s=setting: self._set_theme(s)) + self._theme_group.addAction(act) + theme_menu.addAction(act) + help_menu = self.menuBar().addMenu("&Help") about_action = QAction("About Better Claude Config…", self) about_action.triggered.connect(self._show_about) help_menu.addAction(about_action) + def _set_theme(self, setting: str): + """Persist the theme choice and repaint the running window.""" + QSettings("BCC", "BetterClaudeConfig").setValue("ui/theme", setting) + app = QApplication.instance() + if app is None: # pragma: no cover - only in a headless test harness + return + app.setStyleSheet(theme_stylesheet_for(app, setting)) + # The global stylesheet covers most of the UI, but the inline + # setStyleSheet calls (status dots, warning labels, update banner) only + # pick up the new palette when their widget next renders -- so re-render + # them now rather than leaving dark-on-light text behind. + self._repaint_themed_widgets() + + def _repaint_themed_widgets(self): + """Re-run the inline-styled bits after a palette change.""" + self.status.setStyleSheet(f"color: {MUTED};") + idx = self._current_index() + self._refresh_tables(select_index=idx if idx >= 0 else -1) + self._update_status(saved=False) + def _show_about(self): AboutDialog(self).exec() @@ -2649,6 +2720,33 @@ class MainWindow(QMainWindow): e.accept() +def system_is_dark(app: QApplication) -> bool: + """Whether the desktop is currently using a dark appearance. + + Read from the style's own window colour rather than per-platform APIs -- + Qt has already resolved the OS appearance by the time it builds the + default palette, so this works the same on all three platforms. + """ + try: + return app.palette().color(QPalette.ColorRole.Window).lightness() < 128 + except Exception: # pragma: no cover - defensive; never block startup on theming + return True + + +def stored_theme_setting() -> str: + """The user's theme choice, defaulting to following the system.""" + value = QSettings("BCC", "BetterClaudeConfig").value("ui/theme", core.THEME_SYSTEM) + return value if value in core.THEME_CHOICES else core.THEME_SYSTEM + + +def theme_stylesheet_for(app: QApplication, setting: str | None = None) -> str: + """Resolve setting + OS appearance into a palette, apply it, return the QSS.""" + if setting is None: + setting = stored_theme_setting() + theme = core.resolve_theme(setting, system_is_dark(app)) + return apply_palette(core.palette_for(theme)) + + def main(): if sys.platform == "win32": # Without an explicit AppUserModelID, Windows taskbar groups the app @@ -2667,7 +2765,7 @@ def main(): icon = _app_icon() if not icon.isNull(): app.setWindowIcon(icon) - app.setStyleSheet(STYLESHEET) + app.setStyleSheet(theme_stylesheet_for(app)) win = MainWindow() win.show() sys.exit(app.exec()) diff --git a/bcc_core.py b/bcc_core.py index c81d5ea..a7735d6 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -189,6 +189,138 @@ class ServerEntry: return "remote" if "url" in self.data and "command" not in self.data else "stdio" +# --------------------------------------------------------------------------- # +# Theming (issue #75) +# --------------------------------------------------------------------------- # +THEME_SYSTEM = "system" +THEME_LIGHT = "light" +THEME_DARK = "dark" +THEME_CHOICES = (THEME_SYSTEM, THEME_LIGHT, THEME_DARK) + + +@dataclass(frozen=True) +class Palette: + """Every colour the UI draws with. + + Deliberately exhaustive: the stylesheet used to inline a handful of + near-black literals (`#1a1205` for text on the accent, `#202229` for the + disabled table, `#16181d` for the diagnostics pane), which is fine while + there's one theme and invisible breakage the moment there are two. Each + gets a slot here so a light palette can't silently inherit a dark value. + """ + + name: str + accent: str + accent_dim: str + bg: str + panel: str + panel_2: str + text: str + muted: str + border: str + good: str + bad: str + warn: str + remote: str + on_accent: str # text drawn on top of an accent fill + disabled_bg: str # the parked-servers table + mono_bg: str # diagnostics / log panes + selection_text: str + + +# The shipping theme through v1.3.0. These values are carried over verbatim -- +# adding a light theme must not restyle the dark one. +DARK_PALETTE = Palette( + name="dark", + accent="#f97316", + accent_dim="#c2570b", + bg="#1b1d23", + panel="#23262e", + panel_2="#2b2f39", + text="#e7e9ee", + muted="#9aa0ad", + border="#3a3f4b", + good="#4ade80", + bad="#f87171", + warn="#fbbf24", + remote="#60a5fa", + on_accent="#1a1205", + disabled_bg="#202229", + mono_bg="#16181d", + selection_text="#ffffff", +) + +# The semantic colours are NOT the dark ones lightened. #4ade80 / #fbbf24 sit +# around 1.7:1 against white -- illegible. These are darkened to clear 4.5:1, +# which `test_light_palette_meets_contrast` enforces so nobody "tidies" them +# back toward the dark hues later. +LIGHT_PALETTE = Palette( + name="light", + accent="#c2410c", + accent_dim="#9a3412", + bg="#f6f7f9", + panel="#ffffff", + panel_2="#eef0f4", + text="#1b1d23", + muted="#5c6270", + border="#d3d7de", + good="#15803d", + bad="#b91c1c", + warn="#a16207", + remote="#1d4ed8", + on_accent="#ffffff", + disabled_bg="#e9ebef", + mono_bg="#f0f2f5", + selection_text="#ffffff", +) + +PALETTES = {DARK_PALETTE.name: DARK_PALETTE, LIGHT_PALETTE.name: LIGHT_PALETTE} + + +def resolve_theme(setting: str, system_is_dark: bool) -> str: + """Map a stored theme setting + the OS appearance onto a concrete palette name. + + Anything unrecognised (a hand-edited QSettings value, a setting written by + a future version) falls back to following the system rather than to a + fixed theme -- the user's desktop is the better guess. + """ + if setting == THEME_DARK: + return THEME_DARK + if setting == THEME_LIGHT: + return THEME_LIGHT + return THEME_DARK if system_is_dark else THEME_LIGHT + + +def palette_for(theme: str) -> Palette: + """Concrete palette by name; unknown names fall back to dark (the historical look).""" + return PALETTES.get(theme, DARK_PALETTE) + + +def _hex_to_rgb(value: str) -> tuple[int, int, int]: + v = value.lstrip("#") + if len(v) == 3: + v = "".join(ch * 2 for ch in v) + return int(v[0:2], 16), int(v[2:4], 16), int(v[4:6], 16) + + +def relative_luminance(color: str) -> float: + """WCAG relative luminance for a #rrggbb colour.""" + + def chan(c: int) -> float: + srgb = c / 255.0 + return srgb / 12.92 if srgb <= 0.04045 else ((srgb + 0.055) / 1.055) ** 2.4 + + r, g, b = (chan(c) for c in _hex_to_rgb(color)) + return 0.2126 * r + 0.7152 * g + 0.0722 * b + + +def contrast_ratio(fg: str, bg: str) -> float: + """WCAG contrast ratio between two #rrggbb colours (1.0 to 21.0).""" + a, b = relative_luminance(fg), relative_luminance(bg) + lighter, darker = max(a, b), min(a, b) + return (lighter + 0.05) / (darker + 0.05) + + # --------------------------------------------------------------------------- # # Discovery # --------------------------------------------------------------------------- # diff --git a/tests/test_core.py b/tests/test_core.py index 725358f..2f62d63 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1,6 +1,7 @@ """Pytest port of the original test_core.py script (same 23 behaviours, now proper test functions with tmp_path/monkeypatch fixtures).""" +import dataclasses import json import os import re @@ -2370,3 +2371,118 @@ def test_config_has_unfilled_placeholders_false_after_fill(): def test_config_has_unfilled_placeholders_checks_env_too(): cfg = {"command": "uvx", "args": ["mcp-grafana"], "env": {"GRAFANA_URL": ""}} assert c.config_has_unfilled_placeholders(cfg) is True + + +# --------------------------------------------------------------------------- # +# #75 -- theming +# --------------------------------------------------------------------------- # +@pytest.mark.parametrize( + "setting,system_dark,expected", + [ + (c.THEME_DARK, False, "dark"), + (c.THEME_DARK, True, "dark"), + (c.THEME_LIGHT, False, "light"), + (c.THEME_LIGHT, True, "light"), + (c.THEME_SYSTEM, True, "dark"), + (c.THEME_SYSTEM, False, "light"), + ], +) +def test_resolve_theme_covers_every_setting_and_appearance(setting, system_dark, expected): + assert c.resolve_theme(setting, system_dark) == expected + + +@pytest.mark.parametrize("junk", ["", "solarized", None, "DARK", 3]) +def test_resolve_theme_falls_back_to_following_the_system(junk): + """A hand-edited or future QSettings value should follow the desktop, + not pin a fixed theme.""" + assert c.resolve_theme(junk, True) == "dark" + assert c.resolve_theme(junk, False) == "light" + + +def test_palette_for_known_names(): + assert c.palette_for("dark") is c.DARK_PALETTE + assert c.palette_for("light") is c.LIGHT_PALETTE + + +def test_palette_for_unknown_name_falls_back_to_dark(): + assert c.palette_for("chartreuse") is c.DARK_PALETTE + + +def test_dark_palette_is_unchanged_from_the_shipped_look(): + """v1.3.0 shipped these exact colours; adding a light theme must not + quietly restyle the dark one.""" + p = c.DARK_PALETTE + assert (p.accent, p.bg, p.panel, p.panel_2) == ("#f97316", "#1b1d23", "#23262e", "#2b2f39") + assert (p.text, p.muted, p.border) == ("#e7e9ee", "#9aa0ad", "#3a3f4b") + assert (p.good, p.bad, p.warn, p.remote) == ("#4ade80", "#f87171", "#fbbf24", "#60a5fa") + assert (p.on_accent, p.disabled_bg, p.mono_bg) == ("#1a1205", "#202229", "#16181d") + + +def test_both_palettes_define_every_slot(): + """A missing slot should fail here rather than render a broken window.""" + for pal in (c.DARK_PALETTE, c.LIGHT_PALETTE): + for f in dataclasses.fields(c.Palette): + value = getattr(pal, f.name) + assert value, f"{pal.name}.{f.name} is empty" + if f.name != "name": + assert re.fullmatch(r"#[0-9a-fA-F]{6}", value), f"{pal.name}.{f.name}={value!r}" + + +@pytest.mark.parametrize("pal_name", ["dark", "light"]) +@pytest.mark.parametrize("slot", ["text", "muted", "good", "bad", "warn", "remote", "accent"]) +def test_palette_meets_contrast_on_panel(pal_name, slot): + """Every colour drawn as text/glyph must clear WCAG AA (4.5:1) against the + surface it sits on. The light palette's semantic colours are NOT the dark + ones lightened -- #4ade80 sits near 1.7:1 on white -- so this guards + against someone 'harmonising' them back toward the dark hues.""" + pal = c.palette_for(pal_name) + assert c.contrast_ratio(getattr(pal, slot), pal.panel) >= 4.5 + + +@pytest.mark.parametrize("pal_name", ["dark", "light"]) +def test_on_accent_is_legible_against_the_accent_fill(pal_name): + """Primary buttons and selected rows draw on_accent on top of accent.""" + pal = c.palette_for(pal_name) + assert c.contrast_ratio(pal.on_accent, pal.accent) >= 4.5 + + +def test_contrast_ratio_endpoints(): + assert c.contrast_ratio("#000000", "#ffffff") == pytest.approx(21.0, abs=0.01) + assert c.contrast_ratio("#123456", "#123456") == pytest.approx(1.0, abs=0.001) + assert c.contrast_ratio("#ffffff", "#000000") == pytest.approx(21.0, abs=0.01) + + +def test_relative_luminance_extremes(): + assert c.relative_luminance("#000000") == pytest.approx(0.0) + assert c.relative_luminance("#ffffff") == pytest.approx(1.0) + + +def test_stylesheet_builder_has_no_hardcoded_colours(): + """Every colour in the QSS must come from the palette. + + Three near-black literals used to be inlined here (#1a1205, #202229, + #16181d). Harmless with one theme; with two, they silently render dark + chrome on a light window. Reads the source rather than importing bcc, + which needs PySide6. + """ + src = (Path(__file__).resolve().parent.parent / "bcc.py").read_text(encoding="utf-8") + start = src.index("def build_stylesheet") + body = src[start : src.index("def apply_palette")] + assert re.findall(r"#[0-9a-fA-F]{6}", body) == [] + + +def test_every_palette_slot_is_consumed(): + """A slot added to Palette but never wired up is dead weight. + + Checks for `p.` anywhere in bcc.py, which covers both the QSS and + apply_palette's global bindings -- not every slot belongs in the + stylesheet (`good` and `remote` feed the inline status dots via + STATUS_COLORS/HEALTH_COLORS, never the QSS). This won't catch a slot bound + to a global that nothing then uses; it does catch the common mistake of + extending the dataclass and forgetting to plumb it through. + """ + src = (Path(__file__).resolve().parent.parent / "bcc.py").read_text(encoding="utf-8") + for f in dataclasses.fields(c.Palette): + if f.name == "name": + continue + assert f"p.{f.name}" in src, f"palette slot {f.name!r} is never consumed"