Skip to content

fix(providers): persist account reorder correctly and resync on failure - #4280

Open
StevanusPangau wants to merge 1 commit into
decolua:masterfrom
StevanusPangau:fix/provider-swap-priority
Open

StevanusPangau wants to merge 1 commit into
decolua:masterfrom
StevanusPangau:fix/provider-swap-priority

Conversation

@StevanusPangau

@StevanusPangau StevanusPangau commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes the provider connection reorder (▲/▼) on the provider detail page: the requested order is now persisted correctly on every move, and the UI no longer shows an order that failed to save.

Also fixes the long-standing report in #329 ("can only switch accounts up, not down"), which was auto-closed as not_planned in May 2026 without a fix. This PR supersedes the diagnostic part of that report.

Root cause

handleSwapPriority in src/app/(dashboard)/dashboard/providers/[id]/page.js:

  1. 0-based vs 1-based priority collision — the client wrote raw array indices (priority: index1/index2) for only the swapped pair. Stored priorities are 1-based (server assigns max+1 on create and renumbers 1..N in reorderInTx), so every swap also collided with an untouched row's priority.
  2. Nondeterministic tiebreaker — reorderInTx orders by priority ASC and breaks ties by updatedAt DESC. The two PUTs were sent via Promise.all, so which write ended up "most recent" decided the final order. Consequence: moving a row up one position frequently promoted it all the way to the top (or did nothing), depending on commit order — matching the "sometimes it works" reports.
  3. res.ok never checked — if either PUT failed (expired dashboard session, transient 5xx), the optimistic UI kept showing an order that was never persisted, with no refetch.

Minimal repro (5 accounts, priorities 1–5), move row 3 up one position:

  • old code writes {c2: 1} and {c1: 2}; row c0 still holds priority 1 from before
  • after the server flush, order becomes c2, c0, c1, c3, c4 instead of c0, c2, c1, c3, c4

Fix

  • New pure helper buildPriorityUpdates(next) (same directory pattern as the existing utils.js helpers): emits { id, priority } writes carrying the final 1-based position of every row — not only the moved ones. Writing every row also covers stored duplicate priorities (bulk imports can create ties): with ties present, the server re-breaks them by most-recently-updated on each PUT, so a changed-rows-only strategy lets skipped tied rows drift to positions the user did not ask for (measured divergence: 440/572 single-move cases on tie-heavy shapes).
  • Writes are sent sequentially in ascending position order. This is load-bearing: the server renumbers 1..N after every PUT and breaks ties by most-recently-updated. Ascending order keeps each write's target position valid in every intermediate state. (Parallel writes only have that guarantee for move-to-top; the tests verify all single moves converge.)
  • res.ok is checked per write; local state is synced after the writes; any failure refetches from the server to roll back the optimistic UI, and new moves are gated until that refetch lands.
  • Rapid clicks are serialized through a queue that reads the latest optimistic order via a ref mirror (not a render snapshot), and a row-identity guard protects against a delete landing between queueing and running a queued move.

Testing

  • New tests/unit/provider-swap-priority.test.js (vitest, 7 tests) with an executable model of the server's order/renumber/tiebreak behavior (reorderInTx semantics):
    • every possible single move lands exactly (all from→to pairs, sizes 2–8, pristine priorities)
    • every single move also lands when stored priorities contain duplicates ([1,3,1], alternating [1,3,1,3,1], all-equal, [2,2,2,5], mixed) — including the minimal [1,3,1] middle→last case that diverged under a changed-rows-only strategy
    • consecutive rapid moves land (each applied against the previous result)
    • the Minimal Bugs #329 legacy-parallel behavior is reproduced (red) and fixed (green)
  • npx vitest run before/after this change: identical failure set (pre-existing failures are live-credential smoke tests); no new failures introduced.

Checklist

  • No changes to server-side API or DB schema — dashboard UI + tests only
  • Follows existing patterns (utils.js pure helpers, tests/unit/ vitest)
  • No secrets, no unrelated refactors

Moving a connection up/down wrote raw 0-based array indices for only the
swapped pair, sent as two parallel PUTs, and never checked the responses.
The server orders rows by priority ASC, breaks ties by most-recently-
updated, and renumbers 1..N after every PUT, so three things went wrong:

- 0-based indices collide with the untouched rows' 1-based priorities
- the updatedAt tiebreaker resolves collisions nondeterministically
  depending on which parallel PUT commits first (moves frequently
  promoted the row to the top instead of one position, decolua#329)
- a failed PUT (expired session, transient 500) left the UI showing an
  order that was never persisted, with no resync

The new buildPriorityUpdates helper writes every row's final 1-based
position, and the writes are sent sequentially in ascending position
order. Sequential ascending writes keep each target position valid in
every intermediate state the server's per-PUT renumber produces;
writing every row (not only moved ones) additionally covers stored
duplicate priorities, where the updatedAt tiebreak would otherwise
drift skipped rows. Both properties are verified in tests against a
model of reorderInTx: all single moves converge for sizes 2-8 with
pristine priorities, and for tie-heavy stored shapes ([1,3,1],
alternating duplicates, all-equal) that diverged under a
changed-rows-only strategy.

Also: responses are checked (res.ok), local state is synced after the
writes, any failure refetches from the server to roll back the
optimistic UI, rapid clicks are serialized through a queue that reads
the latest optimistic order (ref mirror, not render snapshot), and a
row-identity guard protects against a delete landing between queueing
and running a queued move.
@StevanusPangau
StevanusPangau force-pushed the fix/provider-swap-priority branch from 644837e to d767458 Compare September 23, 2026 14:28
filipeforattini added a commit to reddb-io/red-router that referenced this pull request Sep 23, 2026
…er saved atomically (#80)

- The request-details view rendered response.content/thinking straight into a
  <pre>, crashing on an array of content blocks (PentatonicDev cb937dd).
  safeText() renders strings as-is and anything else as JSON.
- Reordering accounts wrote two 0-based priorities in parallel (last write
  won; upstream decolua#4280), and indexed the full list with positions from the
  owner-filtered one, so the wrong rows moved when a filter was on. The UI now
  swaps within the list on screen and saves it once through
  PUT /api/providers/reorder, which fills the visible accounts' own slots
  (hidden ones keep theirs), writes 1..N in one transaction
  (setConnectionOrder), and keeps shared accounts admin-managed.
- reorderInTx read a missing/0 priority as first while getProviderConnections
  read it as last; both now read it as last.
Raruu added a commit to Raruu/9router that referenced this pull request Oct 2, 2026
Fork release on top of v0.5.95-1: custom-provider header overrides reachable
from the UI, OpenRouter reasoning efforts mapped into the catalog, the
provider-exact-first resolution order, the Catalog Entry preview dialog and
the minOutput dispatch floor, plus the login INITIAL_PASSWORD hint (decolua#4454),
the CLI --foreground/TRAY_MODE supervisor fix (decolua#4451) and the account-reorder
persistence fix (decolua#4280) extended to the media-providers card. Local
package-lock entries refreshed (gitignored).
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.

1 participant