Skip to content

fix(admin-ui): evict stale priority-group source on remove 404#2784

Merged
tkurki merged 1 commit into
SignalK:masterfrom
dirkwa:priority-stale-ref-prune
Jun 27, 2026
Merged

fix(admin-ui): evict stale priority-group source on remove 404#2784
tkurki merged 1 commit into
SignalK:masterfrom
dirkwa:priority-stale-ref-prune

Conversation

@dirkwa

@dirkwa dirkwa commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Motivation

On the Source Priorities page, removing a source could fail with Could not remove <sourceRef>: server returned 404, and the source would stay stuck in the group.

This happens when a device re-registers under a new transport — a renumbered ttyUSB port, a fresh canName. The device's old sourceRef lingers in the saved priority group, so the reconciled group still lists it and the UI offers a trash icon (the source is isMissingFromStatus). But the removeSource DELETE 404s because the source is genuinely gone, and the handler then reverted the optimistic removal — pinning the stale ref in the group permanently, since the DELETE can never succeed for a source the server no longer has.

Solution

Treat a 404 on a missing-from-status ref as success rather than failure: the server is confirming the source is already gone, which is exactly the desired outcome. The optimistic removal sticks and the ref drops out of the group on Save. Auth (401/403) and other errors still revert and alert as before.

The decision is factored into a pure, unit-tested isRemoveSourceFailure helper.

Tested

  • isRemoveSourceFailure unit tests (success, missing-from-status 404, tracked-ref 404, auth/server errors)
  • vitest run admin-ui suite passes (242 tests)
  • vite build of the admin UI succeeds

Overview

This PR fixes a 404 error that occurs when removing a source from a priority group after the device re-registers under a new transport (such as a renumbered ttyUSB port). The issue arises when a stale source reference remains in the saved priority group but no longer exists on the server.

Problem

When a user attempts to delete a source marked as isMissingFromStatus, the DELETE request returns a 404 error (since the source no longer exists). The existing handler treats this as a failure and reverts the optimistic removal, permanently pinning the stale reference in the group. This creates an impossible scenario where the DELETE can never succeed for a source that is genuinely gone.

Solution

The PR introduces a new isRemoveSourceFailure helper function that distinguishes between different 404 scenarios:

  • 404 on a missing-from-status reference: Treated as success (the server confirming an already-gone source)
  • 404 on a still-tracked reference: Treated as failure (unexpected 404)
  • 2xx responses: Always treated as success
  • Auth/server errors (401, 403, 500): Always treated as failure

Changes

sourceLabels.ts

Adds a pure, unit-tested isRemoveSourceFailure(status: number, ok: boolean, isMissingFromStatus: boolean): boolean utility function that encapsulates the failure-detection logic for source removal operations.

sourceLabels.test.ts

Includes comprehensive test coverage for isRemoveSourceFailure, verifying success cases, missing-from-status 404s, tracked-ref 404s, and authentication/server error handling.

PriorityGroupCard.tsx

Updates the handleRemoveSource function to accept an isMissingFromStatus flag and uses isRemoveSourceFailure to determine whether to revert the optimistic removal and display an error alert. Authentication errors and other unexpected failures continue to trigger alerts as before.

Testing

The admin-ui test suite (242 tests) passes, and the build succeeds. The new logic allows stale references to be successfully dropped from priority groups when their 404 response indicates the source is genuinely gone.

A device that re-registers under a new transport (a renumbered ttyUSB
port, a fresh canName) leaves its old sourceRef in the saved priority
group. The reconciled group still lists that ref, so the UI offers a
trash icon for it (isMissingFromStatus). But the removeSource DELETE
404s — the source is genuinely gone — and the handler reverted the
optimistic removal, pinning the stale ref in the group forever: the
DELETE can never succeed for a source the server no longer has.

Treat a 404 on a missing-from-status ref as success so the ref drops
out of the group on Save.
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 579e4173-6ebf-4539-af3f-2a7ac98e138c

📥 Commits

Reviewing files that changed from the base of the PR and between bc5fd38 and d7c675d.

📒 Files selected for processing (3)
  • packages/server-admin-ui/src/utils/sourceLabels.test.ts
  • packages/server-admin-ui/src/utils/sourceLabels.ts
  • packages/server-admin-ui/src/views/ServerConfig/PriorityGroupCard.tsx

📝 Walkthrough

Walkthrough

Adds a new exported utility isRemoveSourceFailure to sourceLabels.ts that classifies DELETE response outcomes as failures or non-failures, with special handling for ok, 404+isMissingFromStatus, and auth/server error cases. PriorityGroupCard.tsx is updated to use this utility in handleRemoveSource, and unit tests are added to cover all branches.

Changes

Source Removal Failure Classification

Layer / File(s) Summary
isRemoveSourceFailure utility and tests
packages/server-admin-ui/src/utils/sourceLabels.ts, packages/server-admin-ui/src/utils/sourceLabels.test.ts
Exports isRemoveSourceFailure(status, ok, isMissingFromStatus): boolean returning false for ok === true or 404+isMissingFromStatus, and true otherwise. Tests assert correct return values for 2xx, 404 flag combinations, and 401/403/500 codes.
PriorityGroupCard DELETE response handling
packages/server-admin-ui/src/views/ServerConfig/PriorityGroupCard.tsx
Imports isRemoveSourceFailure, adds isMissingFromStatus to handleRemoveSource's parameters, replaces the res.ok guard with the new failure classifier for the revert+alert path, and passes the derived flag from sourceStatusLoaded/statusEntry at the call site.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • SignalK/signalk-server#2685: Modifies the same PriorityGroupCard.tsx trash/delete flow and rollback logic for vanished sources, directly preceding the nuanced failure classification added in this PR.

Suggested labels

fix

🐇 A little helper hops into view,
To check if DELETE was really true!
A 404 with a missing ref?
That's not a failure — just a quiet step.
Auth errors and 500s? Those we'll flag,
And alert the user — no cause to lag! 🗑️✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change: handling a 404 response when removing a stale source from a priority group, which directly addresses the core problem solved by this PR.
Description check ✅ Passed The description comprehensively covers what problem is being solved, the root cause, the solution approach, and testing performed, fully satisfying the template's requirements.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@dirkwa

dirkwa commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Ready for human review

@tkurki tkurki added the fix label Jun 27, 2026
@tkurki tkurki merged commit f46dffc into SignalK:master Jun 27, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants