Skip to content

fix: log removeDevice failures during removeConnection#1283

Open
danditomaso wants to merge 2 commits into
meshtastic:mainfrom
danditomaso:fix/log-remove-connection-errors
Open

fix: log removeDevice failures during removeConnection#1283
danditomaso wants to merge 2 commits into
meshtastic:mainfrom
danditomaso:fix/log-remove-connection-errors

Conversation

@danditomaso

@danditomaso danditomaso commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Closes #1272.

removeConnection in apps/web/src/pages/Connections/useConnections.ts wrapped useDeviceStore.getState().removeDevice(...) in a bare catch {}, hiding failures and leaving the device store partially cleaned up with no diagnostic. Log at warn with the connection id, mesh device id, and the error.

Test plan

  • File builds without new type errors

Summary by CodeRabbit

  • Bug Fixes
    • Added warning logs when removing a connected device fails, improving visibility into connection removal issues.

Bare catch {} on line 89 hid any failure from the device store's
removeDevice call, leaving cleanup partially completed with no signal
in the logs. Log at warn level with the connection + device id and the
error.

Closes meshtastic#1272
Copilot AI review requested due to automatic review settings July 12, 2026 20:26
@vercel

vercel Bot commented Jul 12, 2026

Copy link
Copy Markdown

@danditomaso is attempting to deploy a commit to the Meshtastic Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fbe1919b-8af2-494d-a9fe-36ebb86e472f

📥 Commits

Reviewing files that changed from the base of the PR and between b0bde79 and c1f56b0.

📒 Files selected for processing (1)
  • apps/web/src/pages/Connections/useConnections.ts

📝 Walkthrough

Walkthrough

useConnections now logs failures from device removal during connection cleanup. The live saved-connection lookup in connect is reformatted without changing behavior.

Changes

Connection error handling

Layer / File(s) Summary
Removal logging and connection lookup
apps/web/src/pages/Connections/useConnections.ts
removeConnection warns when removeDevice fails, including connection context and the error. The connect lookup is reformatted with unchanged behavior.

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

Suggested reviewers: copilot

Poem

I’m a bunny with logs in my tray,
Catching lost errors before they hop away.
Connections still find their proper place,
With cleaner lines and a warning trace.
Thump, thump—cleanup now leaves a trace!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description covers the bug, fix, and test plan, but it omits several required template sections. Add the missing template sections, especially Changes Made and Checklist, and include Screenshots only if applicable.
Linked Issues check ⚠️ Warning It logs removeDevice failures for #1272, but does not address the user-initiated toast requirement in the linked issue. If removeConnection is user-initiated, add the requested toast; otherwise note why the toast path is out of scope.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: warning when removeDevice fails during removeConnection.
Out of Scope Changes check ✅ Passed No clear out-of-scope changes are present; the extra line-wrapping in connect is a minor formatting change within the same file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves connection cleanup diagnostics by no longer silently swallowing device-store removal errors during removeConnection, aligning behavior with the intent of issue #1272.

Changes:

  • Log a warn when removeDevice() throws during removeConnection, including connection/device identifiers.
  • Reformat the connect() lookup to read savedConnections from the live store with clearer chaining.

Comment on lines +91 to +97
} catch (err) {
log.warn("removeDevice failed during removeConnection", {
id,
meshDeviceId: conn.meshDeviceId,
err,
});
}
Copilot AI added a commit that referenced this pull request Jul 14, 2026
Apply reviewer feedback from PR #1283 review:
- Log err.name and err.message instead of raw err object to avoid
  {} serialization in JSON-based loggers, consistent with teardown()
  and connect() error handling patterns in the same file
- Reformat long chained call in connect() for readability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: removeConnection silently swallows removeDevice errors

2 participants