jtmorris/dms_tailscalectl
1
0
Fork 0
Code Issues 2 Pull requests Projects Releases Packages Activity Actions

Lax / fragmented error handling throughout the widget (catch-all masking, missing exit checks, no stderr) #29

New issue
Closed
opened 2026-05-19 04:30:56 +00:00 by vybe · 9 comments
vybe commented 2026-05-19 04:30:56 +00:00
Collaborator
Copy link

Problem

Error handling across the widget is fragmented and often masks root causes:

  • Broad try/catch in statusCheck assumes every exception is a JSON parse error (#21)
  • No onExited / exit-status check on copyProcess (#17)
  • No stderr capture on toggle / exit-node processes (#25)
  • Duplicate error toasts on status failure (#13)
  • Wrong error action reported due to stale state (#14)
  • Missing edge-case guards in helper functions (#22)

A single catch block or missing handler frequently swallows the real failure mode and shows a generic or misleading toast.

Impact

Users (and developers) cannot reliably determine why an operation failed. The same class of problem appears in multiple places, making the widget fragile and hard to debug.

## Problem Error handling across the widget is fragmented and often masks root causes: - Broad `try/catch` in `statusCheck` assumes every exception is a JSON parse error (#21) - No `onExited` / exit-status check on `copyProcess` (#17) - No stderr capture on toggle / exit-node processes (#25) - Duplicate error toasts on status failure (#13) - Wrong error action reported due to stale state (#14) - Missing edge-case guards in helper functions (#22) A single `catch` block or missing handler frequently swallows the real failure mode and shows a generic or misleading toast. ## Impact Users (and developers) cannot reliably determine why an operation failed. The same class of problem appears in multiple places, making the widget fragile and hard to debug.
vybe commented 2026-05-19 04:43:16 +00:00
Author
Collaborator
Copy link

FYI from cross-model audit (gemini-3.1-pro-preview):

The try...catch in statusCheck.onStreamFinished catches all errors and assumes every exception is a JSON parse error. If parsePeers throws a TypeError due to an unexpected API response shape, it gets swallowed and reported to the user as the generic “Failed to parse Tailscale status” toast, hiding the real cause.

FYI from cross-model audit (gemini-3.1-pro-preview): The `try...catch` in `statusCheck.onStreamFinished` catches *all* errors and assumes every exception is a JSON parse error. If `parsePeers` throws a TypeError due to an unexpected API response shape, it gets swallowed and reported to the user as the generic “Failed to parse Tailscale status” toast, hiding the real cause.
vybe commented 2026-05-19 05:09:55 +00:00
Author
Collaborator
Copy link

See also #31 (duplicate state-reset logic in statusCheck handlers).

See also #31 (duplicate state-reset logic in statusCheck handlers).
vybe commented 2026-05-19 05:47:19 +00:00
Author
Collaborator
Copy link

Proposed Path Forward — Error Handling Consolidation

Resolves: #13 (double error toast spam), #14 (stale isConnected in error message), #29 (umbrella: fragmented error handling), #31 (duplicate state-reset logic)

Overview

Issues #13, #14, #29, and #31 all stem from the same root cause: error handling in statusCheck and toggleProcess is split across multiple handlers (onStreamFinished catch vs onExited), with duplicated state-reset logic and no single source of truth for what happened. This plan consolidates all error handling into a unified, single-path flow.

Plan

Step 1 — Extract state-reset to a helper (fixes #31)

Create a resetState() function in TailscaleWidget.qml that performs the four property resets (peers = [], ip = "", exitNode = "", binaryAvailable = false). Replace both the catch block and the onExited block's inline resets with calls to this helper. This eliminates the duplication identified in #31.

Step 2 — Single-path error handling for statusCheck (fixes #13)

Currently, statusCheck has two independent error paths:

  • onStreamFinished catch → "Failed to parse Tailscale status"
  • onExited(code !== 0) → "Failed to read Tailscale status" or "Tailscale binary not found"

Both fire on the same failure event, producing double toasts. Consolidate into a single onExited handler that:

  1. Checks exit code first. If non-zero, handles the error (127 → binary not found, else → status failure), calls resetState(), shows exactly one toast, and returns early.
  2. If exit code is 0, proceeds to parse stdout. If JSON.parse fails, calls resetState() and shows a single "Failed to parse" toast.

Remove the try/catch from onStreamFinished entirely. Move the JSON parsing into onExited (after the exit-code guard). This ensures exactly one toast per failure event.

Step 3 — Capture intended action at click time (fixes #14)

In toggleTailscale(), capture the intended action before starting the process:

var intendedAction = root.isConnected ? "disconnect" : "connect"
toggleProcess.property("intendedAction", intendedAction)  // or use a root-level property

In toggleProcess.onExited, read the captured action instead of the live root.isConnected. This prevents the stale-state bug where a statusCheck poll flips isConnected between the user's click and the process exit.

Use a simple root property like property string lastToggleAction: "" to store the intended action.

Step 4 — Cross-reference #25 and #17

Note that stderr capture (#25) and copyProcess exit checking (#17) are handled in the clipboard security plan. Once both plans are implemented, #29's checklist of sub-issues will all be resolved.

Success Criteria

  1. resetState() is called from exactly two places: the onExited error path and the JSON parse failure path.
  2. A single statusCheck failure produces exactly one toast, never two.
  3. toggleProcess.onExited uses a captured action string, not the live root.isConnected property.
  4. The try/catch in statusCheck.stdout.onStreamFinished is removed.
  5. All existing unit tests pass.

AFK Classification

AFK. This is pure QML logic refactoring. The agent can verify correctness by reading the code, ensuring the control flow is sound, and running node --test test/lib.test.js (which is unaffected since lib.js is not modified). The QML changes cannot be unit-tested without a DMS runtime, but the logic is straightforward enough that the agent can verify it statically.

Files Modified

  • tailscalectl/TailscaleWidget.qml — statusCheck handlers, toggleTailscale function, toggleProcess.onExited

Also resolves: #13, #14, #31

## Proposed Path Forward — Error Handling Consolidation **Resolves:** #13 (double error toast spam), #14 (stale isConnected in error message), #29 (umbrella: fragmented error handling), #31 (duplicate state-reset logic) --- ### Overview Issues #13, #14, #29, and #31 all stem from the same root cause: error handling in `statusCheck` and `toggleProcess` is split across multiple handlers (`onStreamFinished` catch vs `onExited`), with duplicated state-reset logic and no single source of truth for what happened. This plan consolidates all error handling into a unified, single-path flow. ### Plan **Step 1 — Extract state-reset to a helper (fixes #31)** Create a `resetState()` function in `TailscaleWidget.qml` that performs the four property resets (`peers = []`, `ip = ""`, `exitNode = ""`, `binaryAvailable = false`). Replace both the `catch` block and the `onExited` block's inline resets with calls to this helper. This eliminates the duplication identified in #31. **Step 2 — Single-path error handling for statusCheck (fixes #13)** Currently, `statusCheck` has two independent error paths: - `onStreamFinished` catch → "Failed to parse Tailscale status" - `onExited(code !== 0)` → "Failed to read Tailscale status" or "Tailscale binary not found" Both fire on the same failure event, producing double toasts. Consolidate into a single `onExited` handler that: 1. Checks exit code first. If non-zero, handles the error (127 → binary not found, else → status failure), calls `resetState()`, shows exactly one toast, and returns early. 2. If exit code is 0, proceeds to parse stdout. If JSON.parse fails, calls `resetState()` and shows a single "Failed to parse" toast. Remove the `try/catch` from `onStreamFinished` entirely. Move the JSON parsing into `onExited` (after the exit-code guard). This ensures exactly one toast per failure event. **Step 3 — Capture intended action at click time (fixes #14)** In `toggleTailscale()`, capture the intended action before starting the process: ```js var intendedAction = root.isConnected ? "disconnect" : "connect" toggleProcess.property("intendedAction", intendedAction) // or use a root-level property ``` In `toggleProcess.onExited`, read the captured action instead of the live `root.isConnected`. This prevents the stale-state bug where a statusCheck poll flips `isConnected` between the user's click and the process exit. Use a simple `root` property like `property string lastToggleAction: ""` to store the intended action. **Step 4 — Cross-reference #25 and #17** Note that stderr capture (#25) and copyProcess exit checking (#17) are handled in the clipboard security plan. Once both plans are implemented, #29's checklist of sub-issues will all be resolved. ### Success Criteria 1. `resetState()` is called from exactly two places: the `onExited` error path and the JSON parse failure path. 2. A single `statusCheck` failure produces exactly one toast, never two. 3. `toggleProcess.onExited` uses a captured action string, not the live `root.isConnected` property. 4. The `try/catch` in `statusCheck.stdout.onStreamFinished` is removed. 5. All existing unit tests pass. ### AFK Classification **AFK.** This is pure QML logic refactoring. The agent can verify correctness by reading the code, ensuring the control flow is sound, and running `node --test test/lib.test.js` (which is unaffected since `lib.js` is not modified). The QML changes cannot be unit-tested without a DMS runtime, but the logic is straightforward enough that the agent can verify it statically. ### Files Modified - `tailscalectl/TailscaleWidget.qml` — statusCheck handlers, toggleTailscale function, toggleProcess.onExited **Also resolves:** #13, #14, #31
jtmorris commented 2026-05-19 06:43:28 +00:00
Owner
Copy link

Not sold on step 3. This is a simple toggle. Its objective should be a dirt simple toggle. You're storing state all over the place. Why even bother telling the user what state the action failed to set? That is easy enough troubleshooting for an unlikely case. Just have a catch-all error message for the toggleProcess.onExited handler that says something like "Could not toggle Tailscale connection." Easy-peasy. Lemon squeezy.

All the end-user cares about is that the toggle didn't work. All the troubleshooter cares about is that the toggle isn't working. They'll need to test the raw Tailscale commands and make sure the code actually executes those Tailscale commands.

Stored state introduces potential for desynchronization between the stored state and reality. That is fertile ground for edge cases and code maintenance headaches. Sometimes that kind of thing is necessary. Usually not. As a rule-of-thumb, it's better to not seek state knowledge if you don't need it. If you do, then poll state on-demand, rather than store it, unless timing or performance demand otherwise. Now? For an error message for the toggle command? I don't think state is necessary.

Not sold on step 3. This is a simple toggle. Its objective should be a dirt simple toggle. You're storing state all over the place. Why even bother telling the user what state the action failed to set? That is easy enough troubleshooting for an unlikely case. Just have a catch-all error message for the `toggleProcess.onExited` handler that says something like "Could not toggle Tailscale connection." Easy-peasy. Lemon squeezy. All the end-user cares about is that the toggle didn't work. All the troubleshooter cares about is that the toggle isn't working. They'll need to test the raw Tailscale commands and make sure the code actually executes those Tailscale commands. Stored state introduces potential for desynchronization between the stored state and reality. That is fertile ground for edge cases and code maintenance headaches. Sometimes that kind of thing is necessary. Usually not. As a rule-of-thumb, it's better to not seek state knowledge if you don't need it. If you do, then poll state on-demand, rather than store it, unless timing or performance demand otherwise. Now? For an error message for the toggle command? I don't think state is necessary.
vybe commented 2026-05-19 06:49:38 +00:00
Author
Collaborator
Copy link

Proposed Path Forward — Error Handling Consolidation (Revised)

Resolves: #13 (double error toast spam), #14 (stale isConnected in error message), #29 (umbrella: fragmented error handling), #31 (duplicate state-reset logic)

Overview

Per @jtmorris's feedback, Step 3 (storing the intended toggle action) is overengineered. A simple toggle needs a simple error message. No stored state, no desync risk. The revised plan removes that step entirely and uses a generic toggle error.

Plan

Step 1 — Extract state-reset to a helper (fixes #31)

Create a resetState() function in TailscaleWidget.qml that resets the three core properties:

function resetState() {
    root.peers = []
    root.ip = ""
    root.exitNode = ""
}

Note: binaryAvailable is being removed by the process safety plan (#15 / #11), so it's not included here. Replace both the catch block and the onExited block's inline resets with calls to this helper. This eliminates the duplication identified in #31.

Step 2 — Single-path error handling for statusCheck (fixes #13, addresses gemini audit)

Currently, statusCheck has two independent error paths:

  • onStreamFinished catch → "Failed to parse Tailscale status"
  • onExited(code !== 0) → "Failed to read Tailscale status" or "Tailscale binary not found"

Both fire on the same failure event, producing double toasts. Additionally, the try/catch in onStreamFinished catches all errors (including TypeErrors from parsePeers on unexpected API shapes) and masks them as a generic JSON parse error.

Consolidate into a single onExited handler:

  1. Check exit code first. If non-zero, handle the error (127 → binary not found, else → status failure), call resetState(), show exactly one toast, and return early.
  2. If exit code is 0, proceed to parse stdout. If JSON.parse fails or parsePeers throws, call resetState() and show a single "Failed to parse Tailscale status" toast.

Remove the try/catch from onStreamFinished entirely. Move the JSON parsing and parsePeers call into onExited (after the exit-code guard). This ensures exactly one toast per failure event and prevents the catch-all from swallowing TypeErrors.

Step 3 — Generic toggle error (fixes #14, per @jtmorris feedback)

Remove the original Step 3 entirely. No stored state for the intended toggle action.

In toggleProcess.onExited, use a single catch-all error message:

if (code !== 0) {
    resetState()
    ToastService.showError("tailscalectl", "Could not toggle Tailscale connection")
    return
}

The end user cares that the toggle didn't work. A troubleshooter will test raw Tailscale commands. Storing state for an error message introduces desync risk for no meaningful benefit.

Step 4 — Cross-reference #25

Stderr capture (#25) is handled in the clipboard security plan (#12). Once both plans are implemented, #29's checklist of sub-issues will all be resolved.

Success Criteria

  1. resetState() is called from exactly two places: the onExited error path and the JSON parse failure path.
  2. A single statusCheck failure produces exactly one toast, never two.
  3. toggleProcess.onExited shows a generic "Could not toggle Tailscale connection" error — no stored state, no connect/disconnect distinction.
  4. The try/catch in statusCheck.stdout.onStreamFinished is removed.
  5. All existing unit tests pass.

AFK Classification

AFK. Pure QML logic refactoring. The agent can verify correctness by reading the code, ensuring the control flow is sound, and running node --test test/lib.test.js (which is unaffected since lib.js is not modified). The QML changes cannot be unit-tested without a DMS runtime, but the logic is straightforward enough that the agent can verify it statically.

Files Modified

  • tailscalectl/TailscaleWidget.qml — statusCheck handlers, toggleTailscale function, toggleProcess.onExited
## Proposed Path Forward — Error Handling Consolidation (Revised) **Resolves:** #13 (double error toast spam), #14 (stale isConnected in error message), #29 (umbrella: fragmented error handling), #31 (duplicate state-reset logic) --- ### Overview Per @jtmorris's feedback, Step 3 (storing the intended toggle action) is overengineered. A simple toggle needs a simple error message. No stored state, no desync risk. The revised plan removes that step entirely and uses a generic toggle error. ### Plan **Step 1 — Extract state-reset to a helper (fixes #31)** Create a `resetState()` function in `TailscaleWidget.qml` that resets the three core properties: ```js function resetState() { root.peers = [] root.ip = "" root.exitNode = "" } ``` Note: `binaryAvailable` is being removed by the process safety plan (#15 / #11), so it's not included here. Replace both the `catch` block and the `onExited` block's inline resets with calls to this helper. This eliminates the duplication identified in #31. **Step 2 — Single-path error handling for statusCheck (fixes #13, addresses gemini audit)** Currently, `statusCheck` has two independent error paths: - `onStreamFinished` catch → "Failed to parse Tailscale status" - `onExited(code !== 0)` → "Failed to read Tailscale status" or "Tailscale binary not found" Both fire on the same failure event, producing double toasts. Additionally, the `try/catch` in `onStreamFinished` catches *all* errors (including TypeErrors from `parsePeers` on unexpected API shapes) and masks them as a generic JSON parse error. Consolidate into a single `onExited` handler: 1. Check exit code first. If non-zero, handle the error (127 → binary not found, else → status failure), call `resetState()`, show exactly one toast, and return early. 2. If exit code is 0, proceed to parse stdout. If `JSON.parse` fails or `parsePeers` throws, call `resetState()` and show a single "Failed to parse Tailscale status" toast. Remove the `try/catch` from `onStreamFinished` entirely. Move the JSON parsing and `parsePeers` call into `onExited` (after the exit-code guard). This ensures exactly one toast per failure event and prevents the catch-all from swallowing TypeErrors. **Step 3 — Generic toggle error (fixes #14, per @jtmorris feedback)** Remove the original Step 3 entirely. No stored state for the intended toggle action. In `toggleProcess.onExited`, use a single catch-all error message: ```js if (code !== 0) { resetState() ToastService.showError("tailscalectl", "Could not toggle Tailscale connection") return } ``` The end user cares that the toggle didn't work. A troubleshooter will test raw Tailscale commands. Storing state for an error message introduces desync risk for no meaningful benefit. **Step 4 — Cross-reference #25** Stderr capture (#25) is handled in the clipboard security plan (#12). Once both plans are implemented, #29's checklist of sub-issues will all be resolved. ### Success Criteria 1. `resetState()` is called from exactly two places: the `onExited` error path and the JSON parse failure path. 2. A single `statusCheck` failure produces exactly one toast, never two. 3. `toggleProcess.onExited` shows a generic "Could not toggle Tailscale connection" error — no stored state, no connect/disconnect distinction. 4. The `try/catch` in `statusCheck.stdout.onStreamFinished` is removed. 5. All existing unit tests pass. ### AFK Classification **AFK.** Pure QML logic refactoring. The agent can verify correctness by reading the code, ensuring the control flow is sound, and running `node --test test/lib.test.js` (which is unaffected since `lib.js` is not modified). The QML changes cannot be unit-tested without a DMS runtime, but the logic is straightforward enough that the agent can verify it statically. ### Files Modified - `tailscalectl/TailscaleWidget.qml` — statusCheck handlers, toggleTailscale function, toggleProcess.onExited
jtmorris commented 2026-05-19 06:52:58 +00:00
Owner
Copy link

Good enough to give it a shot.

Good enough to give it a shot.
vybe commented 2026-05-21 06:09:53 +00:00
Author
Collaborator
Copy link

@vibe's latest error handling consolidation plan needs update because:

  • parseStatusResult, errorMessage now in lib.js (commits moved JS out of QML)
  • statusCheck now delegates parsing to TailscaleLib.parseStatusResult in onStreamFinished
  • Tests exist for lib.js functions, so resetState / single-path logic must preserve or enhance test coverage

The single-path onExited + resetState helper is still sound, but implementation now involves ensuring lib.js parse functions throw appropriately for error paths, and tests cover the new flows. Generic toggle error is good.

Update plan to edit both lib.js (if needed for parse errors) + QML, plus add test assertions.

Written by AI agent working for @jtmorris. Model: Grok 4.3.

@vibe's latest error handling consolidation plan needs update because: - parseStatusResult, errorMessage now in lib.js (commits moved JS out of QML) - statusCheck now delegates parsing to TailscaleLib.parseStatusResult in onStreamFinished - Tests exist for lib.js functions, so resetState / single-path logic must preserve or enhance test coverage The single-path onExited + resetState helper is still sound, but implementation now involves ensuring lib.js parse functions throw appropriately for error paths, and tests cover the new flows. Generic toggle error is good. Update plan to edit both lib.js (if needed for parse errors) + QML, plus add test assertions. Written by AI agent working for @jtmorris. Model: Grok 4.3.
vybe commented 2026-05-21 07:05:48 +00:00
Author
Collaborator
Copy link

Consolidated fragmented error paths into consistent onExited + stderr pattern. Merged to vibes.

Consolidated fragmented error paths into consistent onExited + stderr pattern. Merged to vibes.
vybe commented 2026-05-21 07:16:56 +00:00
Author
Collaborator
Copy link

Resolved via merge daab237. The plugin is confirmed working by @jtmorris. Human code review required before merging to testing.

Resolved via merge daab237. The plugin is confirmed working by @jtmorris. Human code review required before merging to testing.
vybe closed this issue 2026-05-21 07:16:57 +00:00
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
master
vibes
testing
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: jtmorris/dms_tailscalectl#29
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?