Merge branch 'feature_first_principles_refactor' into vibes — first-principles large refactor (timer removal, copy simplification, central error formatting, QML/JS boundary progress)

tailscalectl/TailscaleWidget.qml

@@ -14,29 +14,16 @@ PluginComponent {
     property string tailscaleIP: ""
     property string currentExitNode: ""
     property var peers: []
-    property string cachedClipboardTool: ""
     property string _copyText: ""
     property string _copyCurrentTool: ""
-    property int _copyAttempted: 0
+    property bool _pendingToggle: false  // transient one-shot for post-action verification (to be removed in first-principles refactor)
-    property bool _pendingToggle: false  // transient one-shot for poll-then-act (#42) — reset immediately after use, no "desired state" cache
 
     layerNamespacePlugin: "tailscalectl"
     popoutWidth: 360
     popoutHeight: 400
 
-    Timer {
-        id: refreshTimer
-        interval: 5000
-        running: true
-        repeat: true
-        onTriggered: {
-            if (!statusCheck.running) {
-                statusCheck.running = true
-            }
-        }
-    }
-
     Component.onCompleted: {
+        // Initial status fetch on load. Subsequent fetches are on-demand (popout open, explicit refresh, or post-action verification).
         statusCheck.running = true
     }
 
@@ -48,8 +35,8 @@ PluginComponent {
         onExited: (code, status) => {
             if (code !== 0) {
                 var action = root.isConnected ? "disconnect" : "connect"
-                var detail = toggleProcess.stderr.text.trim().slice(0, 120)
+                var detail = toggleProcess.stderr.text.trim()
-                ToastService.showError("tailscalectl", TailscaleLib.errorMessage(action) + (detail ? " — " + detail : ""))
+                ToastService.showError("tailscalectl", TailscaleLib.formatError(action, detail))
             }
             statusCheck.running = true
         }
@@ -62,17 +49,14 @@ PluginComponent {
 
         onExited: (code, status) => {
             if (code === 0) {
-                root.cachedClipboardTool = root._copyCurrentTool
+                ToastService.showInfo(TailscaleLib.getStrings().copied(root._copyText))
-                ToastService.showInfo("Copied " + root._copyText + " to clipboard")
+            } else if (root._copyCurrentTool === "dms") {
-            } else {
+                // One simple fallback attempt: try wl-copy.
-                root._copyAttempted += 1
+                root._copyCurrentTool = "wl-copy"
-                if (root._copyAttempted < TailscaleLib.allClipboardTools().length) {
-                    root._copyCurrentTool = TailscaleLib.nextClipboardTool(root._copyCurrentTool)
                 root._executeCopy()
             } else {
-                    var detail = copyProcess.stderr.text.trim().slice(0, 120)
+                var detail = copyProcess.stderr.text.trim()
-                    ToastService.showError("tailscalectl", "No clipboard tool found" + (detail ? " — " + detail : ""))
+                ToastService.showError("tailscalectl", TailscaleLib.formatError("clipboard", detail))
-                }
             }
         }
     }
@@ -84,8 +68,8 @@ PluginComponent {
 
         onExited: (code, status) => {
             if (code !== 0) {
-                var detail = exitNodeProcess.stderr.text.trim().slice(0, 120)
+                var detail = exitNodeProcess.stderr.text.trim()
-                ToastService.showError("tailscalectl", TailscaleLib.errorMessage("set") + (detail ? " — " + detail : ""))
+                ToastService.showError("tailscalectl", TailscaleLib.formatError("set", detail))
             }
             statusCheck.running = true
         }
@@ -110,12 +94,10 @@ PluginComponent {
                 root.tailscaleIP = ""
                 root.currentExitNode = ""
                 root.peers = []
-                ToastService.showError("tailscalectl", TailscaleLib.errorMessage("status"))
+                ToastService.showError("tailscalectl", TailscaleLib.formatError("status"))
             }
 
-            // #42 poll-then-act: if a toggle was requested, use the *fresh* state we just received
+            // Post-action verification: if a toggle was requested, use the fresh state we just received.
-            // #47: structured logging evaluated and rejected — toasts + 120-char stderr already give
-            // actionable feedback; persistent logs would add state/rotation with no net UX benefit.
             if (root._pendingToggle) {
                 root._pendingToggle = false
                 toggleProcess.command = TailscaleLib.buildToggleCommand(root.isConnected)
@@ -125,8 +107,7 @@ PluginComponent {
     }
 
     function toggleTailscale() {
-        // #42: poll-then-act — never decide up/down from potentially stale root.isConnected.
+        // Post-action verification: force a fresh status check, then act on the real ground truth.
-        // Force a fresh statusCheck; the decision happens in its onExited using the just-received value.
         root._pendingToggle = true
         statusCheck.running = true
     }
@@ -138,19 +119,15 @@ PluginComponent {
 
     function copyToClipboard(text) {
         root._copyText = text
-        root._copyAttempted = 0
+        // Simple two-tool fallback per first-principles plan: dms first, then wl-copy.
-        if (root.cachedClipboardTool) {
+        root._copyCurrentTool = "dms"
-            root._copyCurrentTool = root.cachedClipboardTool
-        } else {
-            root._copyCurrentTool = TailscaleLib.allClipboardTools()[0]
-        }
         root._executeCopy()
     }
 
     function _executeCopy() {
         var cmd = TailscaleLib.buildCopyCommand(root._copyText, root._copyCurrentTool)
         if (!cmd) {
-            ToastService.showError("tailscalectl", "Invalid clipboard command")
+            ToastService.showError("tailscalectl", TailscaleLib.formatError("clipboard"))
             return
         }
         copyProcess.command = cmd
@@ -160,7 +137,7 @@ PluginComponent {
     popoutContent: Component {
         PopoutComponent {
             headerText: "Tailscale"
-            detailsText: root.isConnected ? "Connected" : "Disconnected"
+            detailsText: root.isConnected ? TailscaleLib.getStrings().connected : TailscaleLib.getStrings().disconnected
             showCloseButton: true
 
             Item {
@@ -207,7 +184,7 @@ PluginComponent {
                     Item { width: 1; height: 1; Layout.fillWidth: true }
 
                     StyledText {
-                        text: "Exit node: " + (root.currentExitNode || "None")
+                        text: TailscaleLib.getStrings().exitNodePrefix + (root.currentExitNode || TailscaleLib.getStrings().none)
                         font.pixelSize: Theme.fontSizeSmall
                         color: Theme.surfaceVariantText
                         anchors.verticalCenter: parent.verticalCenter

tailscalectl/lib.js

@@ -33,9 +33,9 @@ function findActiveExitNode(peerMap) {
 
 var safeClipboardTools = ["dms", "wl-copy"]
 
-// Direct argv form — no sh -c, no escaping, no future injection surface (#49)
+    // Direct argv form — no sh -c, no escaping, no future injection surface.
-// clipmanctl intentionally dropped (niche optional history manager, not needed on DMS + Wayland)
+    // clipmanctl intentionally dropped (niche optional history manager, not needed on DMS + Wayland).
-var clipboardTools = {
+    var clipboardTools = {
     "dms":     { argv: ["dms", "cl", "copy"] },
     "wl-copy": { argv: ["wl-copy"] }
 }
@@ -76,8 +76,8 @@ function getStrings() {
     }
 }
 
-// Light UI predicates extracted from QML (advances #43 — keeps view thin)
+    // Light UI predicates — keep the view thin.
-function shouldShowClearExitNode(currentExitNode) {
+    function shouldShowClearExitNode(currentExitNode) {
     return currentExitNode !== ""
 }
 
@@ -103,6 +103,11 @@ function buildToggleCommand(isConnected) {
     return isConnected ? ["tailscale", "down"] : ["tailscale", "up"]
 }
 
+// Single source of truth for the status command used for on-demand and post-action verification.
+function getStatusCommand() {
+    return ["tailscale", "status", "--json"]
+}
+
 function errorMessage(cmd) {
     var messages = {
         "up": "Failed to connect to Tailscale",
@@ -110,12 +115,24 @@ function errorMessage(cmd) {
         "down": "Failed to disconnect from Tailscale",
         "disconnect": "Failed to disconnect from Tailscale",
         "set": "Failed to set exit node",
-        "status": "Failed to read Tailscale status"
+        "status": "Failed to read Tailscale status",
+        "clipboard": "Error copying to clipboard"
+    };
+    return messages[cmd] || "Tailscale command failed";
+}
+
+// Central error formatting for the widget. Used by both success and error paths.
+// detail is optional truncated stderr or extra context.
+function formatError(action, detail) {
+    var base = errorMessage(action);
+    if (detail && detail.length > 0) {
+        var truncated = detail.length > 120 ? detail.slice(0, 120) : detail;
+        return base + " — " + truncated;
     }
-    return messages[cmd] || "Tailscale command failed"
+    return base;
 }
 
 // CommonJS export for Node.js tests (ignored by QML)
 if (typeof module !== "undefined" && module.exports) {
-    module.exports = { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode }
+    module.exports = { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode }
 }

test/lib.test.js

@@ -1,7 +1,7 @@
 import { test } from "node:test"
 import assert from "node:assert"
 import lib from "../tailscalectl/lib.js"
-const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib
+const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib
 
 /*
  * Test coverage note for #48:
@@ -240,3 +240,30 @@ test("parseStatusResult sets isConnected false for non-Running BackendState", ()
     const state = parseStatusResult(json)
     assert.strictEqual(state.isConnected, false)
 })
+
+// --- formatError (central error + detail formatting per first-principles plan) ---
+
+test("formatError returns base message without detail", () => {
+    assert.strictEqual(formatError("status"), "Failed to read Tailscale status")
+    assert.strictEqual(formatError("set"), "Failed to set exit node")
+})
+
+test("formatError appends and truncates detail", () => {
+    const longDetail = "x".repeat(200)
+    const msg = formatError("up", longDetail)
+    assert.ok(msg.includes("Failed to connect to Tailscale"))
+    assert.ok(msg.endsWith("x".repeat(120)))
+    assert.ok(msg.length < 200)
+})
+
+test("formatError handles empty or falsy detail gracefully", () => {
+    assert.strictEqual(formatError("down", ""), "Failed to disconnect from Tailscale")
+    assert.strictEqual(formatError("connect", null), "Failed to connect to Tailscale")
+})
+
+// --- getStatusCommand (new pure helper for low-duty-cycle architecture) ---
+
+test("getStatusCommand returns the canonical tailscale status --json argv", () => {
+    const cmd = getStatusCommand()
+    assert.deepStrictEqual(cmd, ["tailscale", "status", "--json"])
+})