Fix PATH check, add undo, and popup dialog for env/header vars

PATH check: - Add system_path() that reads /etc/paths + /etc/paths.d/* + well-known package manager dirs (/opt/homebrew/bin, /usr/local/bin, /opt/local/bin) so the ok/warn distinction is consistent whether BCC runs from terminal or as a .app — /opt/homebrew/bin/npx now correctly shows as ok - augmented_path() now uses system_path() as its base and also includes the current process PATH (handles CI / container setups) - diagnostics_text() uses system_path() to mark user-specific dirs with + Undo: - _push_undo() / _undo() snapshot-based undo with 50-entry stack - Undo button in action bar + Ctrl+Z shortcut - Hooked into: add server, duplicate, delete, paste JSON, enable/disable toggle, drag-drop import, and env/header row add/remove (via before_change callback chain through ServerEditor → KeyValueTable) - Undo stack resets on profile load Variable/header entry: - KeyValueTable.+ Add now opens a popup dialog with labeled fields (key name + OK disabled until non-empty) instead of inserting an invisible blank row — applies consistently to both env and headers tables - Pressing Enter in key field moves to value; Enter in value submits Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-29 15:34:18 -04:00
parent dd7557ea62
commit 2e33850fe3
2 changed files with 221 additions and 43 deletions
@@ -103,9 +103,12 @@ class ConnTester(QThread):
 # Small reusable: key/value editor (for env and headers)
 # --------------------------------------------------------------------------- #
 class KeyValueTable(QWidget):
-    def __init__(self, key_label="Key", val_label="Value", on_change=None):
+    def __init__(self, key_label="Key", val_label="Value", on_change=None, before_change=None):
        super().__init__()
        self._on_change = on_change
+        self._before_change = before_change
+        self._key_label = key_label
+        self._val_label = val_label
        lay = QVBoxLayout(self)
        lay.setContentsMargins(0, 0, 0, 0)
        lay.setSpacing(6)
@@ -133,17 +136,69 @@ class KeyValueTable(QWidget):
            self._on_change()

    def _add_row(self):
+        dlg = QDialog(self.window())
+        dlg.setWindowTitle(f"Add {self._key_label}")
+        dlg.setMinimumWidth(340)
+        v = QVBoxLayout(dlg)
+        v.setSpacing(10)
+
+        grid = QGridLayout()
+        grid.setSpacing(8)
+        lbl_k = QLabel(f"{self._key_label}:")
+        lbl_k.setObjectName("muted")
+        grid.addWidget(lbl_k, 0, 0)
+        key_edit = QLineEdit()
+        key_edit.setPlaceholderText(
+            "e.g. API_KEY" if self._key_label == "Variable" else f"{self._key_label.lower()}-name"
+        )
+        grid.addWidget(key_edit, 0, 1)
+
+        lbl_v = QLabel(f"{self._val_label}:")
+        lbl_v.setObjectName("muted")
+        grid.addWidget(lbl_v, 1, 0)
+        val_edit = QLineEdit()
+        val_edit.setPlaceholderText("value")
+        grid.addWidget(val_edit, 1, 1)
+        v.addLayout(grid)
+
+        btns = QDialogButtonBox(
+            QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel
+        )
+        ok_btn = btns.button(QDialogButtonBox.StandardButton.Ok)
+        ok_btn.setObjectName("primary")
+        ok_btn.setEnabled(False)
+        key_edit.textChanged.connect(lambda t: ok_btn.setEnabled(bool(t.strip())))
+        btns.accepted.connect(dlg.accept)
+        btns.rejected.connect(dlg.reject)
+        v.addWidget(btns)
+
+        key_edit.setFocus()
+        # Enter in key field moves to value; Enter in value field submits
+        key_edit.returnPressed.connect(val_edit.setFocus)
+        val_edit.returnPressed.connect(lambda: ok_btn.click() if ok_btn.isEnabled() else None)
+
+        if dlg.exec() != QDialog.DialogCode.Accepted:
+            return
+        k = key_edit.text().strip()
+        val = val_edit.text()
+        if not k:
+            return
+        if self._before_change:
+            self._before_change()
        r = self.table.rowCount()
        self.table.insertRow(r)
-        self.table.setItem(r, 0, QTableWidgetItem(""))
-        self.table.setItem(r, 1, QTableWidgetItem(""))
+        self.table.setItem(r, 0, QTableWidgetItem(k))
+        self.table.setItem(r, 1, QTableWidgetItem(val))
        self._changed()

    def _remove_row(self):
        r = self.table.currentRow()
-        if r >= 0:
-            self.table.removeRow(r)
-            self._changed()
+        if r < 0:
+            return
+        if self._before_change:
+            self._before_change()
+        self.table.removeRow(r)
+        self._changed()

    def load(self, d: dict):
        self.table.blockSignals(True)
@@ -171,10 +226,11 @@ class KeyValueTable(QWidget):
 # The server editor panel (right side)
 # --------------------------------------------------------------------------- #
 class ServerEditor(QFrame):
-    def __init__(self, on_change):
+    def __init__(self, on_change, before_change=None):
        super().__init__()
        self.setObjectName("card")
        self._on_change = on_change
+        self._before_change = before_change   # called before any row add/remove in tables
        self._extra = {}          # preserve unknown server fields verbatim
        self._loading = False

@@ -279,7 +335,8 @@ class ServerEditor(QFrame):
        self.args.textChanged.connect(self._emit)
        v.addWidget(self.args)
        v.addWidget(self._lbl("Environment variables"))
-        self.env = KeyValueTable("Variable", "Value", on_change=self._emit)
+        self.env = KeyValueTable("Variable", "Value", on_change=self._emit,
+                                 before_change=self._before_change)
        v.addWidget(self.env)
        return w

@@ -299,7 +356,8 @@ class ServerEditor(QFrame):
        self.transport.currentIndexChanged.connect(self._emit)
        v.addWidget(self.transport)
        v.addWidget(self._lbl("Headers"))
-        self.headers = KeyValueTable("Header", "Value", on_change=self._emit)
+        self.headers = KeyValueTable("Header", "Value", on_change=self._emit,
+                                     before_change=self._before_change)
        v.addWidget(self.headers)
        return w

@@ -447,10 +505,10 @@ class ServerEditor(QFrame):
        if not url:
            return
        self.test_btn.setEnabled(False)
-        self.test_btn.setText("Testing…")
+        self.test_btn.setText("Testing...")
        self.dep_dot.setText("◆")
        self.dep_dot.setStyleSheet(f"color: {MUTED}; font-size: 14px;")
-        self.dep_label.setText("testing reachability…")
+        self.dep_label.setText("testing reachability...")
        self.dep_label.setStyleSheet(f"color: {MUTED};")
        self._tester = ConnTester(url, timeout=6.0)
        self._tester.done.connect(self._on_test_done)
@@ -478,14 +536,14 @@ class PasteDialog(QDialog):
        v = QVBoxLayout(self)
        info = QLabel(
            "Paste a config snippet from any MCP doc. Accepts a full "
-            "{\"mcpServers\": {…}} block, just the inner {\"name\": {…}} map, "
+            "{\"mcpServers\": {...}} block, just the inner {\"name\": {...}} map, "
            "or a single bare server object."
        )
        info.setObjectName("muted")
        info.setWordWrap(True)
        v.addWidget(info)
        self.box = QPlainTextEdit()
-        self.box.setPlaceholderText('{\n  "mcpServers": {\n    "brave-search": {\n      "command": "npx",\n      "args": ["-y", "@modelcontextprotocol/server-brave-search"],\n      "env": {"BRAVE_API_KEY": "…"}\n    }\n  }\n}')
+        self.box.setPlaceholderText('{\n  "mcpServers": {\n    "brave-search": {\n      "command": "npx",\n      "args": ["-y", "@modelcontextprotocol/server-brave-search"],\n      "env": {"BRAVE_API_KEY": "..."}\n    }\n  }\n}')
        v.addWidget(self.box, 1)
        self.err = QLabel("")
        self.err.setStyleSheet(f"color: {BAD};")
@@ -526,6 +584,7 @@ class MainWindow(QMainWindow):
        self._suppress_sel = False
        self._focused_table = None
        self._row_of_index: dict[int, tuple] = {}
+        self._undo_stack: list[list] = []   # each entry: snapshot of self.servers

        central = QWidget()
        self.setCentralWidget(central)
@@ -538,7 +597,8 @@ class MainWindow(QMainWindow):
        split = QHBoxLayout()
        split.setSpacing(12)
        split.addWidget(self._build_left(), 3)
-        self.editor = ServerEditor(on_change=self._editor_changed)
+        self.editor = ServerEditor(on_change=self._editor_changed,
+                                   before_change=self._push_undo)
        split.addWidget(self.editor, 4)
        root.addLayout(split, 1)

@@ -560,7 +620,7 @@ class MainWindow(QMainWindow):
        self.profile_combo.setMinimumWidth(220)
        self.profile_combo.currentIndexChanged.connect(self._profile_selected)
        bar.addWidget(self.profile_combo)
-        add_cfg = QPushButton("Add config…")
+        add_cfg = QPushButton("Add config...")
        add_cfg.clicked.connect(self.add_custom_config)
        bar.addWidget(add_cfg)
        reload_btn = QPushButton("Reload")
@@ -632,20 +692,51 @@ class MainWindow(QMainWindow):
        self.dup_btn = QPushButton("Duplicate")
        self.del_btn = QPushButton("Delete")
        self.del_btn.setObjectName("danger")
-        self.paste_btn = QPushButton("Paste JSON…")
+        self.paste_btn = QPushButton("Paste JSON...")
        self.copy_btn = QPushButton("Copy to ▸")
+        self.undo_btn = QPushButton("Undo")
+        self.undo_btn.setEnabled(False)
+        self.undo_btn.setToolTip("Undo last change  (Ctrl+Z)")
        self.add_btn.clicked.connect(self.add_server)
        self.dup_btn.clicked.connect(self.duplicate_server)
        self.del_btn.clicked.connect(self.delete_server)
        self.paste_btn.clicked.connect(self.paste_json)
        self.copy_btn.clicked.connect(self.copy_to_menu)
-        for b in (self.add_btn, self.dup_btn, self.del_btn, self.paste_btn, self.copy_btn):
+        self.undo_btn.clicked.connect(self._undo)
+        for b in (self.add_btn, self.dup_btn, self.del_btn, self.paste_btn,
+                  self.copy_btn, self.undo_btn):
            bar.addWidget(b)
+        # Ctrl+Z shortcut
+        undo_action = QAction(self)
+        undo_action.setShortcut("Ctrl+Z")
+        undo_action.triggered.connect(self._undo)
+        self.addAction(undo_action)
        bar.addStretch()
        self.validation_lbl = QLabel("")
        bar.addWidget(self.validation_lbl)
        return bar

+    # --- undo ------------------------------------------------------------ #
+    def _push_undo(self):
+        """Snapshot the current server list onto the undo stack."""
+        self._undo_stack.append(
+            [core.ServerEntry(s.name, dict(s.data), s.enabled) for s in self.servers]
+        )
+        if len(self._undo_stack) > 50:
+            self._undo_stack.pop(0)
+        self.undo_btn.setEnabled(True)
+
+    def _undo(self):
+        if not self._undo_stack:
+            return
+        self.servers = self._undo_stack.pop()
+        self.undo_btn.setEnabled(bool(self._undo_stack))
+        cur = self._current_index()
+        sel = cur if 0 <= cur < len(self.servers) else (0 if self.servers else -1)
+        self._refresh_tables(select_index=sel)
+        self._mark_dirty()
+        self.status.setText("Undone.")
+
    # --- profiles -------------------------------------------------------- #
    def reload_profiles(self):
        discovered = core.discover_profiles()
@@ -664,7 +755,7 @@ class MainWindow(QMainWindow):
            self.profile_combo.setCurrentIndex(0)
            self.load_profile(self.profiles[0])
        else:
-            self.status.setText("No Claude installs found. Use “Add config…” to point at one.")
+            self.status.setText('No Claude installs found. Use "Add config…" to point at one.')

    def add_custom_config(self):
        start = str(core.app_support_base())
@@ -702,6 +793,8 @@ class MainWindow(QMainWindow):
        self.current_profile = profile
        self.servers = core.extract_servers(self.full_config)
        self.dirty = False
+        self._undo_stack.clear()
+        self.undo_btn.setEnabled(False)
        self._refresh_tables(select_index=0 if self.servers else -1)
        self._update_status(saved=False)

@@ -841,6 +934,7 @@ class MainWindow(QMainWindow):
        idx = name_item.data(Qt.ItemDataRole.UserRole)
        if idx is None or not (0 <= idx < len(self.servers)):
            return
+        self._push_undo()
        self.servers[idx].enabled = item.checkState() == Qt.CheckState.Checked
        # Toggling moves the server to the other section; keep it selected there.
        self._refresh_tables(select_index=idx)
@@ -872,6 +966,7 @@ class MainWindow(QMainWindow):

    # --- server actions -------------------------------------------------- #
    def add_server(self):
+        self._push_undo()
        base = "new-server"
        name = base
        n = 2
@@ -889,6 +984,7 @@ class MainWindow(QMainWindow):
        idx = self._current_index()
        if not (0 <= idx < len(self.servers)):
            return
+        self._push_undo()
        src = self.servers[idx]
        existing = {s.name for s in self.servers}
        name = f"{src.name}-copy"
@@ -905,8 +1001,9 @@ class MainWindow(QMainWindow):
        if not (0 <= idx < len(self.servers)):
            return
        name = self.servers[idx].name
-        if QMessageBox.question(self, "Delete server", f"Remove “{name}”?") != QMessageBox.StandardButton.Yes:
+        if QMessageBox.question(self, "Delete server", f'Remove “{name}”?') != QMessageBox.StandardButton.Yes:
            return
+        self._push_undo()
        del self.servers[idx]
        self._refresh_tables(select_index=min(idx, len(self.servers) - 1))
        self._mark_dirty()
@@ -915,13 +1012,14 @@ class MainWindow(QMainWindow):
        dlg = PasteDialog(self)
        if dlg.exec() != QDialog.DialogCode.Accepted or not dlg.result_servers:
            return
+        self._push_undo()
        added, replaced = 0, 0
        existing = {s.name: i for i, s in enumerate(self.servers)}
        for name, data in dlg.result_servers.items():
            if name in existing:
                ans = QMessageBox.question(
                    self, "Server exists",
-                    f"“{name}” already exists. Replace it?\n\nYes = replace · No = keep both (renamed)",
+                    f'“{name}” already exists. Replace it?\n\nYes = replace · No = keep both (renamed)',
                    QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No
                )
                if ans == QMessageBox.StandardButton.Yes:
@@ -969,7 +1067,7 @@ class MainWindow(QMainWindow):
        if src.name in names:
            ans = QMessageBox.question(
                self, "Exists in destination",
-                f"“{src.name}” already exists in {dest.label}. Overwrite it there?"
+                f'“{src.name}” already exists in {dest.label}. Overwrite it there?'
            )
            if ans != QMessageBox.StandardButton.Yes:
                return
@@ -982,7 +1080,7 @@ class MainWindow(QMainWindow):
            QMessageBox.critical(self, "Copy failed", f"Couldn't write {dest.label}:\n{e}")
            return
        bnote = f" (backup: {backup.name})" if backup else ""
-        self.status.setText(f"Copied “{src.name}” → {dest.label}{bnote}. Restart that Claude to apply.")
+        self.status.setText(f'Copied “{src.name}” → {dest.label}{bnote}. Restart that Claude to apply.')

    # --- save ------------------------------------------------------------ #
    def _validate(self) -> bool:
@@ -1052,6 +1150,7 @@ class MainWindow(QMainWindow):
            except Exception as ex:
                QMessageBox.warning(self, "Couldn't import", f"{Path(path).name}:\n{ex}")
                continue
+            self._push_undo()
            for name, data in servers.items():
                names = {s.name for s in self.servers}
                if name in names:
@@ -289,35 +289,109 @@ def validate_servers(servers: list[ServerEntry]) -> list[str]:
 # --------------------------------------------------------------------------- #
 # Dependency / PATH checking
 # --------------------------------------------------------------------------- #
+@functools.lru_cache(maxsize=1)
+def system_path() -> str:
+    """
+    The PATH that a GUI app (e.g., Claude Desktop launched from Finder/dock)
+    reliably inherits, independent of how BCC itself was started.
+
+    This is used to determine the 'ok' vs 'warn' distinction:
+      ok   = found in system_path()      → works however Claude is launched
+      warn = found only in augmented_path() → works from a terminal but may
+             not work when Claude is opened from the desktop
+
+    macOS  : /etc/paths + /etc/paths.d/* (what launchd provides) plus
+             well-known package-manager prefixes (/opt/homebrew, /opt/local).
+    Windows: system + user PATH from the registry.
+    Linux  : /etc/environment + standard FHS dirs + /snap/bin.
+    """
+    dirs: list[str] = []
+
+    if sys.platform == "darwin":
+        for p in ["/etc/paths"] + sorted(glob.glob("/etc/paths.d/*")):
+            try:
+                dirs.extend(ln.strip() for ln in Path(p).read_text().splitlines() if ln.strip())
+            except OSError:
+                pass
+        # Package-manager install prefixes: present on disk once installed,
+        # accessible to all processes regardless of shell configuration.
+        for d in (
+            "/opt/homebrew/bin", "/opt/homebrew/sbin",   # Homebrew (Apple Silicon)
+            "/usr/local/bin", "/usr/local/sbin",          # Homebrew (Intel) / manual installs
+            "/opt/local/bin", "/opt/local/sbin",          # MacPorts
+        ):
+            dirs.append(d)
+
+    elif os.name == "nt":
+        try:
+            import winreg
+            for hive, sub in [
+                (winreg.HKEY_LOCAL_MACHINE,
+                 r"SYSTEM\CurrentControlSet\Control\Session Manager\Environment"),
+                (winreg.HKEY_CURRENT_USER, r"Environment"),
+            ]:
+                try:
+                    with winreg.OpenKey(hive, sub) as k:
+                        val, _ = winreg.QueryValueEx(k, "PATH")
+                        dirs.extend(os.path.expandvars(val).split(os.pathsep))
+                except OSError:
+                    pass
+        except ImportError:
+            pass
+        dirs.extend(["C:\\Windows\\System32", "C:\\Windows",
+                     "C:\\Windows\\System32\\Wbem",
+                     os.path.expandvars(r"%ProgramFiles%\nodejs")])
+
+    else:  # Linux / other
+        try:
+            for line in Path("/etc/environment").read_text().splitlines():
+                if line.upper().startswith("PATH="):
+                    dirs.extend(line[5:].strip('"\'').split(os.pathsep))
+        except OSError:
+            pass
+        for d in ("/usr/local/bin", "/usr/bin", "/bin",
+                  "/usr/sbin", "/sbin", "/snap/bin"):
+            dirs.append(d)
+
+    seen, out = set(), []
+    for d in dirs:
+        d = d.strip()
+        if d and d not in seen:
+            seen.add(d)
+            out.append(d)
+    return os.pathsep.join(out)
+
+
@functools.lru_cache(maxsize=1)
 def augmented_path() -> str:
    """
-    PATH + the usual runtime install locations. GUI apps (especially bundled
-    .app launches) often don't inherit the shell PATH, so npx/uvx can appear
-    'missing' when they're actually fine. This widens the net.
+    The widest PATH BCC searches when looking for a command — system_path()
+    plus user-specific runtime locations (nvm, cargo, volta, bun, etc.) that
+    require shell configuration to be active.

-    Memoized for the session (PATH rarely changes mid-run). Call
-    refresh_path_cache() after the user installs something to re-scan.
+    Memoized for the session. Call refresh_path_cache() to re-scan, e.g.
+    after the user installs a new runtime.
    """
-    parts = os.environ.get("PATH", "").split(os.pathsep)
+    base = system_path().split(os.pathsep)
+    # Also include the current process PATH (catches unusual CI / container setups)
+    env_parts = os.environ.get("PATH", "").split(os.pathsep)
    home = Path.home()
-    extra = [
-        "/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin",
+    user_dirs = [
        str(home / ".local" / "bin"),
        str(home / ".cargo" / "bin"),
        str(home / ".deno" / "bin"),
        str(home / ".bun" / "bin"),
        str(home / ".volta" / "bin"),
    ]
-    extra += glob.glob(str(home / ".nvm" / "versions" / "node" / "*" / "bin"))
-    extra += glob.glob(str(home / ".local" / "share" / "fnm" / "node-versions" / "*" / "installation" / "bin"))
+    user_dirs += glob.glob(str(home / ".nvm" / "versions" / "node" / "*" / "bin"))
+    user_dirs += glob.glob(str(home / ".local" / "share" / "fnm" / "node-versions" / "*" / "installation" / "bin"))
    if os.name == "nt":
        appdata = os.environ.get("APPDATA", "")
        if appdata:
-            extra.append(str(Path(appdata) / "npm"))
+            user_dirs.append(str(Path(appdata) / "npm"))

    seen, ordered = set(), []
-    for p in parts + extra:
+    for p in base + env_parts + user_dirs:
        if p and p not in seen and Path(p).is_dir():
            seen.add(p)
            ordered.append(p)
@@ -325,7 +399,8 @@ def augmented_path() -> str:


 def refresh_path_cache():
-    """Forget the memoized PATH so the next check re-scans (e.g. after an install)."""
+    """Forget memoized PATHs so the next check re-scans (e.g. after an install)."""
+    system_path.cache_clear()
    augmented_path.cache_clear()


@@ -411,7 +486,10 @@ def check_dependency(data: dict, path: str | None = None) -> dict:
            in_base[c] = ok
        else:
            resolved[c] = shutil.which(c, path=aug)
-            in_base[c] = bool(shutil.which(c))  # raw inherited PATH only
+            # 'in_base' means: found in system_path() — the PATH that Claude
+            # Desktop (or any GUI app) reliably inherits regardless of shell config.
+            # This check is consistent whether BCC runs from a terminal or as a .app.
+            in_base[c] = bool(shutil.which(c, path=system_path()))

    missing = [c for c, r in resolved.items() if not r]
    hints: list[str] = []
@@ -428,9 +506,10 @@ def check_dependency(data: dict, path: str | None = None) -> dict:
            label = "found, but only on a non-standard PATH"
            for c in nonbase:
                hints.append(
-                    f"'{c}' was found at {resolved[c]}, but only via a location this app added on "
-                    f"top of the inherited PATH. If you launch Claude Desktop as an app (not from a "
-                    f"terminal), it may not see this. Putting the full path in 'command' is the reliable fix."
+                    f"'{c}' was found at {resolved[c]}, but that location is not in the standard "
+                    f"system PATH. Claude Desktop launched from Finder or the dock may not see it. "
+                    f"Clicking 'Use full path ↳' rewrites the command to its absolute path, which "
+                    f"works regardless of how Claude is launched."
                )
        else:
            status = "ok"
@@ -475,17 +554,17 @@ def diagnostics_text(name: str, data: dict) -> str:
        for h in res["hints"]:
            L.append(f"  - {h}")

-    base_dirs = [p for p in os.environ.get("PATH", "").split(os.pathsep) if p]
+    sys_dirs = set(system_path().split(os.pathsep))
    aug_dirs = res["searched_path"].split(os.pathsep) if res["searched_path"] else []
-    extra = [d for d in aug_dirs if d not in base_dirs]
+    extra = [d for d in aug_dirs if d not in sys_dirs]
    L.append("")
    L.append(f"PATH searched ({len(aug_dirs)} dirs):")
    for d in aug_dirs:
        L.append(("  + " if d in extra else "    ") + d)
    if extra:
        L.append("")
-        L.append("(+ = added by this app on top of the PATH it inherited. Claude Desktop, "
-                 "launched separately, may NOT have these on its PATH.)")
+        L.append("(+ = user-specific dirs not in the standard system PATH. "
+                 "Claude Desktop launched from Finder may NOT have these.)")
    return "\n".join(L)