Skip to content

feat(dashboard): allow reordering virtual model targets via drag handle - #77

Closed
weselben wants to merge 7 commits into
mainfrom
feat/virtual-model-target-reorder
Closed

feat(dashboard): allow reordering virtual model targets via drag handle#77
weselben wants to merge 7 commits into
mainfrom
feat/virtual-model-target-reorder

Conversation

@weselben

@weselben weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner

TL;DR

The Virtual Model editor shows fallback targets in the order they were added. There is no way to change that order: users must delete and re-add targets to change the failover priority or the round-robin queue. Add a drag handle to each target row. Drag a row onto another row to move it. The full list is reorderable, including the primary target.

Files to review (9, +502 / -5):

File Why
web/dashboard/src/pages/models/VmTargetRow.svelte (start here) The grip handle, drag events, and the drop highlight live here.
web/dashboard/src/pages/models/vmForm.js Pure helpers: flattenFormTargets and moveFormTarget.
web/dashboard/src/pages/models/virtualModelEditor.svelte.js Drag state (vmDragIndex, vmDropIndex) and the drop action.
web/dashboard/src/pages/models/VirtualModelEditor.svelte Passes each row its flattened index and enables the handle.
web/dashboard/tests/models-vm-target-reorder.test.js Contract tests: move semantics, payload order, reopen round-trip, index alignment.
internal/admin/handler_virtualmodels_test.go Go contract test: PUT stores and returns the sent target order.
docs/features/virtual-models.mdx "Reorder targets" section.
web/dashboard/messages/en.json, pl.json models_move_target label for the handle.

Behavior

Target rows with drag handles

  • The grip handle (☰) sits at the left edge of each target row, away from the remove button on the right. It shows only when more than one populated target exists.
  • Drag-and-move, not drag-and-replace. Dropping a row onto another row moves the dragged row to that position. The other rows keep their relative order and shift down (or up). Nothing is swapped away.
  • While dragging, the dragged row fades. The row under the cursor shows the dashed outline: that is where the dragged row will land, in between — not a swap.

Dashed outline marks the drop position

  • Keyboard works the same. Focus the handle and press ArrowUp / ArrowDown to move the row one position. Focus follows the moved row, so holding an arrow walks the same model through the list.
  • Every strategy benefits. For failover, position 1 is the first target the gateway tries. For balancing strategies, position 1 is the first slot in the queue.
  • Weights and provider pins ride along. A moved row keeps its weight and its explicit provider.
  • The summary line under the targets ("…falls back to…") updates live while you reorder, before anything is saved.
  • Saving persists the new order. The payload lists targets in display order; the backend stores and returns them in that order.

How

  • The editor treats the target list as one contiguous list: primary first, extras after. moveFormTarget splices that list and writes the result back into the primary slot plus the extras array.
  • Row indices match the flattened list. An empty primary row (fresh form, or a cleared primary) is not in the list, so extras are indexed from 0 there — otherwise drops on new rows landed out of bounds and were silently refused.
  • The handle uses native HTML5 drag and drop: no dependency. dragenter/dragover preventDefault so drop fires in every browser; text selection is disabled on the handle so a fast grab-release commits the drag. The drop target row is tracked in vmDropIndex for the highlight, written only on change to avoid re-render storms mid-drag.
  • Cross-boundary moves (extra → primary) are the same splice as within the extras.
  • The handle shows only when more than one target exists, and hides for managed virtual models.
  • No backend change. buildVirtualModelSavePayload already serializes targets in array order, and the backend reads that order as the failover priority / queue order.

Contract tests

The behavior above is pinned so refactors cannot silently change it:

  • web/dashboard/tests/models-vm-target-reorder.test.js — insert-between move semantics (drop last onto first lands first, others keep relative order); payload lists targets in the new display order after a drag; reopening a reordered model restores the order; weights and provider pins survive; flattened-index alignment for empty and filled primaries; null/malformed form handling.
  • TestUpsertVirtualModelTargetOrderRoundTrips (internal/admin/handler_virtualmodels_test.go) — a reorder PUT stores the targets in exactly the sent order, and the list view the dashboard renders returns the same order.

Tests

  • npm test in web/dashboard: 596 pass (6 in the reorder contract file).
  • go test ./internal/admin/ ./internal/virtualmodels/: pass.
  • gofmt -l: clean. go vet: clean. go build ./...: clean. ineffassign: clean. errcheck: clean on the touched files.
  • npm run check: svelte-check reports 0 errors, 0 warnings.
  • Manual verification on a seeded instance: fast grab-release drags, unsaved-row reordering, cross-boundary moves, and save round-trips.
  • Not covered: pointer-based drag end-to-end. The drag path is DOM-event wiring; the move logic itself is pure and tested.

Follow-up

  • Browser-level e2e tests (Playwright or similar) for the editor drag-and-keyboard interactions. Native HTML5 drag timing (a fast grab-release can skip the drag session; drop only fires once dragover/dragenter are preventDefaulted) is exactly what node:test cannot pin.

Links


This PR description was generated with AI assistance.

Add a grip handle left of each target row's remove button. Drag a row
onto another row (or focus the handle and use the arrow keys) to change
its position. The full list is reorderable, including the primary
target, so the failover priority and the round-robin queue order are
editable for every strategy. The save payload already serializes
targets in array order, so no backend change is needed.
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author
image Models now House a Drag-and-Drop field with wich the Order can be changed image It is now indicated with a dotted line around the Model that gets replaced/moved-away by the dragged and dropped models - its not a drag and replace - its a drag and move in between feature

Frontend: models-vm-target-reorder.test.js pins drag-and-move (insert-
between, not swap), payload order after a reorder, reopen round-trip, and
weight/provider-pin survival. Backend: TestUpsertVirtualModelTargetOrderRoundTrips
pins that a reorder save stores and returns targets in exactly the order
the editor sent.
@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Thanks for testing — the PR body now explains the behavior in a Behavior section with your screenshots embedded.

One clarification on your second screenshot: the dashed outline is not the model being replaced. It is the drop position: the dragged row will land exactly there, and the outlined row (plus everything between origin and drop row) shifts by one. Nothing is ever swapped away or lost — it is a move-in-between, and the contract tests added in a7d3ff5 pin that (drop last onto first → lands first, others keep relative order) alongside a Go round-trip test that the saved order is what the editor sent.

Also new in this push:

  • test: contract tests for the reorder (dashboard payload order + backend PUT→GET round-trip)
  • docs: "Reorder targets" section in docs/features/virtual-models.mdx

After ArrowUp/ArrowDown the each block reuses the DOM node at the old
index, so focus stayed on whatever model now occupies that slot. Track
the requested focus index and move focus to the moved row's handle, so
repeated arrows walk the same model through the list.

@weselben weselben left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Review — feat(dashboard): allow reordering virtual model targets

Validate: PASS — 522 added lines, no secret patterns, diff sane.
Scope applied: unnecessary comments, test-line coverage, no fork-specific references, match merged-PR norms (baseline: ENTERPILOT#855 playground).

Findings (8)

  • 🔵 web/dashboard/tests/models-vm-target-reorder.test.js:1 — header comment references "(PR #77)", a fork-specific PR number. Meaningless in the upstream repo. Drop the PR reference; keep the behavior description.
  • 🟡 web/dashboard/src/pages/models/VmTargetRow.svelte:63ondragleave fires when the cursor enters a child element (SearchSelect, weight input), so the dashed drop highlight flickers off while still over the row. Guard with a relatedTarget containment check before clearing vmDropIndex.
  • 🔵 web/dashboard/src/pages/models/vmForm.js:81-118 — three multi-line comments above the new exports restate the code. Merged-PR norm in this area is one line. Trim each to a single line.
  • 🔵 web/dashboard/src/pages/models/virtualModelEditor.svelte.js:90-120 — same: the store method comments (enterVmTargetDrop, dropVmTarget, requestVmFocusHandle) explain the caller's wiring, not the method. One line each.
  • 🔵 internal/admin/handler_virtualmodels_test.go:738slicesEqual reimplements stdlib slices.Equal (Go 1.21+, repo is on 1.27). Import slices, delete the helper.
  • 🔵 web/dashboard/src/pages/models/VirtualModelEditor.svelte:84-95vm.vmTargetCount() > 1 && !vm.vmFormManaged is computed inline on every row. Hoist to one reactive const in the component script.
  • 🔵 web/dashboard/tests/models-virtual-models.test.js:1074-1142 — the two moveFormTarget tests duplicate the contract file (models-vm-target-reorder.test.js). Single source of truth: keep the contract file, drop these.
  • 🔵 web/dashboard/src/pages/models/vmForm.js:84 — coverage gap against the 99% target: null form and non-array targets branches of vmFormTargetCount / flattenFormTargets / moveFormTarget are untested. Add a small table case to the contract file.

Notes

  • No fork references in code comments other than the flagged test header.
  • Component DOM wiring (drag handlers, focus effect) is not node-testable; consistent with how the rest of the dashboard ships.
  • Behavior itself verified manually on a seeded demo container; contract tests pin the payload order.

Next step: pr-resolve for the 🟡; the 🔵 batch is a quick cleanup pass.

Comment thread web/dashboard/tests/models-vm-target-reorder.test.js Outdated
Comment thread web/dashboard/src/pages/models/VmTargetRow.svelte Outdated
Comment thread web/dashboard/src/pages/models/vmForm.js Outdated
Comment thread web/dashboard/src/pages/models/virtualModelEditor.svelte.js Outdated
Comment thread internal/admin/handler_virtualmodels_test.go Outdated
Comment thread web/dashboard/src/pages/models/VirtualModelEditor.svelte Outdated
Comment thread web/dashboard/tests/models-virtual-models.test.js Outdated
Comment thread web/dashboard/src/pages/models/vmForm.js
@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Verification pass on PR head (0038423a)

Check Scope Result
gofmt -l whole repo ✅ clean
go vet changed pkgs + go vet ./... ✅ clean (3 pre-existing internal/core json-tag warnings, untouched by this PR)
go test -race internal/admin, internal/virtualmodels ✅ pass
go build ./... whole repo ✅ clean
ineffassign changed pkgs ✅ clean
errcheck changed pkgs ✅ PR files clean (4 hits, all pre-existing in failover_test.go, resolve_bench_test.go, store_mongodb.go)
staticcheck / golangci-lint ⚠️ not runnable: no released build supports the go1.27 export-data version yet (golangci-lint newest = go1.25.4, staticcheck = export data v2 vs v4)
npm test (dashboard) 596 tests ✅ pass
svelte-check dashboard ✅ 0 errors, 0 warnings

Files that may need fixing (aside from the review findings)

None from the verification side. The review findings (1 🟡 + 7 🔵, see review above) cover everything actionable: comment trim, slicesEqual → stdlib, duplicate tests consolidation, dragleave flicker, fork-specific PR reference in the contract-test header, draggable hoist, and the null/non-array coverage cases.

Toolchain note for CI: if upstream CI pins golangci-lint, it will need a build made with go1.27+ before it can run on this repo. Everything else in the matrix is green on PR head.

- drop fork-specific PR reference from contract test header
- guard dragleave with relatedTarget containment (drop highlight no
  longer flickers between child elements)
- trim over-long comments to match merged-PR norms
- slicesEqual -> stdlib slices.Equal in the Go contract test
- hoist draggable expression to one $derived const
- consolidate moveFormTarget tests into the contract file
- cover null/malformed form branches of the target helpers
@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Resolution summary (6e843c2)

Resolved: 8 / 8 inline findings — one commit applied to the PR head branch.

# Severity File Fix
1 🔵 nit tests/models-vm-target-reorder.test.js Fork-specific PR reference removed from header.
2 🟡 risk VmTargetRow.svelte ondragleave now guards with relatedTarget containment; drop highlight no longer flickers between child elements.
3 🔵 nit vmForm.js Three multi-line comments trimmed to one line each.
4 🔵 nit virtualModelEditor.svelte.js Store method comments trimmed to one line each.
5 🔵 nit handler_virtualmodels_test.go slicesEqual deleted; uses stdlib slices.Equal.
6 🔵 nit VirtualModelEditor.svelte draggable expression hoisted to a single canReorder $derived const.
7 🔵 nit models-virtual-models.test.js Duplicate moveFormTarget tests removed; contract file is the single source.
8 🔵 nit vmForm.js Null/malformed form branches covered by a new table case.

Verification on resolved head: gofmt -l clean, go vet clean, go test pass on internal/admin + internal/virtualmodels, npm test 595/595, svelte-check 0/0. Each thread has its own reply with the commit SHA.

PR: #77

The editor always numbered extra rows from 1, but flattenFormTargets
skips an empty primary row: in a fresh/unsaved form (or after clearing
the primary) UI index n mapped to flattened index n-1, so drops on the
new/unsaved rows landed out of bounds and the move was silently
dropped. Extras now start at 0 when the primary has no model, and the
primary handle hides. enterVmTargetDrop also skips redundant writes so
fast drags no longer re-render the list on every dragover event.
Prevent default on dragenter so browsers that require it allow the
drop, and disable text selection on the handle so a fast grab-release
cannot turn into a selection instead of a drag.
@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

Final manual review

Manual verification on a seeded demo container (commit a9604103):

  • Drag: drag a target row onto another — inserts at the drop row's position, others shift. Dashed outline marks the drop position. No swaps. Works on the primary target too. Cross-boundary moves confirmed.
  • Keyboard: focus the grip handle, press ArrowUp / ArrowDown — focus follows the moved row, holding the arrow walks the same model through the list.
  • Strategies: failover priority reorder, round_robin queue reorder — both reflected in the save payload order.
  • Weights + provider pins: survive the move on their row.
  • Reorder before save (newly added rows): now consistent with the flattened list — first-try moves land correctly.
  • Fast grab-release: drag session now commits reliably; previously some flicks dropped silently (fixed with dragenter preventDefault and user-select: none on the handle).

No regressions in the rest of the editor. No backend changes — the backend round-trip test in TestUpsertVirtualModelTargetOrderRoundTrips pins that storage order matches what the editor sends.

Ready for upstream.

@weselben

weselben commented Sep 3, 2026

Copy link
Copy Markdown
Owner Author

moved to upstream repo - ENTERPILOT#879

@weselben weselben closed this Sep 3, 2026
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.

Allow reordering fallback priorities for Virtual Models

1 participant