fix(ui): spell-bar drag-reorder works — the per-frame rebuild was destroying the dragged cell (#354)

Everything already existed — the drag payloads, the favorite wire pair
(0x1E3 add-at-position / 0x1E4 remove, byte-confirmed against retail's
Event_AddSpellFavorite @0x006A0F70 and ACE), the insert-shift state
ops. The bug: lifting a favorite fires SpellbookChanged, the next
per-frame Tick rebuilt the bar, the rebuild flushed and recreated
every cell, and UiRoot's subtree-removal safety net canceled the
in-flight drag whose source had just been destroyed — one frame after
every lift, before any drop could land.

The rebuild now defers for the duration of the drag gesture, and the
drop ports retail's own -1-if-lifted-before-target index adjustment
(SpellCastSubMenu::AddFavorite @0x004C7060) so final positions are
byte-identical: insert-shift, not swap; drag-out still deletes (the
lift's removal stands on a missed drop, retail's shape). The
real-pointer-pipeline test fails against the pre-fix code with the
exact cancellation and passes after; a discriminator pins that
physical-item drop handlers reject the spell payload.

AP-172 files the one presentation divergence (mid-drag reflow happens
on release, not continuously) — renumbered from the agent's AP-171
draft, which collided with the same-day double-click row. #354 filed
and closed.

Clean-room complete solution: 11,541 passed / 4 skipped / 0 failed.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

docs/ISSUES.md

@@ -24,6 +24,42 @@ What does NOT go here:
 - Every session: scan OPEN issues at start; promote/close anything we touched during the session before ending.
 - Promoting to a Phase: mark as `DONE (promoted to Phase X)` + commit SHA where the Phase entry landed.
 
+## #354 — Spell-bar drag reorder did not work: lifting a favorite canceled the drag before the drop could land
+
+**Status:** CLOSED 2026-08-08 — user gate finding ("I should be able to
+rearrange spells on the spell bar. Like dragging one out and dropping it
+in another position. That does not work today"), diagnosed and fixed same
+session. Root cause: `SpellcastingUiController.BeginFavoriteDrag` performs
+retail's press-time removal (`gmSpellcastingUI::RecvNotice_ItemListBeginDrag`
+@0x004C7360 → `RemoveSpellFromMenu`, matching `PlayerModule::RemoveSpellFavorite`
+@0x005D4910) — correct and pre-existing — but that removal fires
+`SpellbookChanged`, and the very next per-frame `Tick()` (production drives
+this unconditionally via `RetailUiRuntime.Tick`) called `Rebuild()`, which
+flushes and recreates every favorite-bar cell (`UiItemList.Flush` →
+`RemoveChild`). `UiRoot`'s subtree-removal safety net
+(`ClearSubtreeOwnership`) cancels any drag whose source widget is
+destroyed — so the drag was silently canceled one frame after every lift,
+before the user could complete a drop. Empirically confirmed: a real-pointer-
+path test (`DragFavoriteOntoAnotherSlot_ThroughTheRealPointerPipeline_ReordersAndSyncsWire`)
+driving `UiRoot.OnMouseDown`/`OnMouseMove`/a mid-drag `Tick`/`OnMouseUp` fails
+with `screen.DragSource == null` against the pre-fix code, and passes after
+it. Fix (`SpellcastingUiController.cs`): defer the favorite-list rebuild for
+the whole drag gesture (`_favoriteDragActive`), and compensate the drop-time
+target index for the resulting stale sibling numbering by porting retail's
+own `SpellCastSubMenu::AddFavorite` @0x004C7060 index adjustment (decrement
+the target by one when the lifted item's original index was before it) —
+same insert-shift semantics `PlayerModule::AddSpellFavorite` @0x005D43E0's
+`InsertPos` already implements, now reachable through a live drag. Recorded
+as AP-172 in the divergence register (the mid-drag visual reflow now happens
+on release rather than continuously, final positions/wire are retail-exact).
+Tests: `SpellcastingUiControllerTests.cs` (the real-pointer-path reorder
+test + a payload-discriminator sabotage check confirming a spell-favorite
+payload is rejected by a physical `IItemListDragHandler`), `SpellbookTests.cs`
+(`SetFavorite`/`RemoveFavorite` insert-shift unit coverage). Wire golden
+bytes for `AddSpellFavorite`/`RemoveSpellFavorite` (opcodes 0x1E3/0x1E4) and
+`RuntimeCharacterState.TryAddFavorite`/`TryRemoveFavorite` were already
+covered and needed no change.
+
 ## #353 — Toolbar selected-object text: count field ignores authored HJustify; name field does not wrap to its authored two lines
 
 **Status:** CLOSED 2026-08-08 — user-passed ("Ok slider bar looks ok!" + the wrap confirmed); the OneLine routing fix (4cfcc8b3) completed it. (RightAligned on the authored HJustify=2 entry; two stacked centered one-line labels wrapping at the authored 140 px via WrapNameTwoLines).

src/AcDream.App/UI/Layout/SpellcastingUiController.cs

@@ -66,6 +66,7 @@ public sealed class SpellcastingUiController : IRetainedPanelController
     private bool _disposed;
     private bool _favoritesDirty;
     private bool _endowmentDirty;
+    private bool _favoriteDragActive;
 
     private SpellcastingUiController(
         ImportedLayout layout,
@@ -422,16 +423,43 @@ public sealed class SpellcastingUiController : IRetainedPanelController
     }
 
     private void BeginFavoriteDrag(SpellFavoriteDragPayload payload)
-        => _removeFavorite?.Invoke(payload.SourceTab, payload.SpellId);
+    {
+        // gmSpellcastingUI::RecvNotice_ItemListBeginDrag @ 0x004C7360 removes the
+        // lifted item from PlayerModule (+ sends the wire RemoveSpellFavorite)
+        // the instant the drag starts. That removal fires SpellbookChanged, which
+        // would normally set _favoritesDirty and let the next Tick() rebuild the
+        // whole favorite list -- but Rebuild() flushes and recreates every slot
+        // (UiItemList.Flush -> RemoveChild), and UiRoot's subtree-removal safety
+        // net (ClearSubtreeOwnership) cancels any drag rooted in a removed
+        // element. Left alone, a per-frame Tick() would destroy the very cell
+        // driving this gesture and silently cancel the reorder before the user
+        // can complete the drop. _favoriteDragActive defers the rebuild for the
+        // gesture's duration; DropFavorite compensates target indices for the
+        // now-stale numbering (retail's own -1-if-lifted-before-target rule,
+        // ported below).
+        _favoriteDragActive = true;
+        _removeFavorite?.Invoke(payload.SourceTab, payload.SpellId);
+    }
 
-    private static void EndFavoriteDrag(SpellFavoriteDragPayload payload)
+    private void EndFavoriteDrag(SpellFavoriteDragPayload payload)
     {
         // The press-time command already performed retail's PlayerModule
-        // removal on the one Runtime-owned Spellbook.
+        // removal on the one Runtime-owned Spellbook. Release the rebuild
+        // deferral -- the next Tick() resyncs the list to the (possibly
+        // further-updated-by-Drop) Spellbook state.
+        _favoriteDragActive = false;
     }
 
     private void DropFavorite(SpellFavoriteDragPayload payload, int targetTab, int targetPosition)
     {
+        // Rebuild() was deferred for the whole gesture (see BeginFavoriteDrag), so
+        // every sibling slot's captured target index is still numbered against the
+        // PRE-lift list. Retail's own SpellCastSubMenu::AddFavorite @ 0x004C7060
+        // corrects for exactly this staleness: when the lifted item's original
+        // index was before the drop target, the target index shifts down by one
+        // to land where the target visually sits once the gap closes.
+        if (payload.SourceTab == targetTab && payload.SourcePosition < targetPosition)
+            targetPosition -= 1;
         _addFavorite?.Invoke(targetTab, targetPosition, payload.SpellId);
         _selected[targetTab] = payload.SpellId;
     }
@@ -470,7 +498,7 @@ public sealed class SpellcastingUiController : IRetainedPanelController
             UpdateEndowment();
             SelectTab(_activeTab);
         }
-        if (_favoritesDirty)
+        if (_favoritesDirty && !_favoriteDragActive)
         {
             _favoritesDirty = false;
             Rebuild();

tests/AcDream.App.Tests/UI/Layout/SpellcastingUiControllerTests.cs

@@ -170,6 +170,111 @@ public sealed class SpellcastingUiControllerTests
         Assert.Equal([2u], used);
     }
 
+    /// <summary>
+    /// #354 (user gate finding): "I should be able to rearrange spells on the
+    /// spell bar ... drag one out and drop it in another position." Every prior
+    /// test in this file drives <c>Dropped</c>/<c>OnEvent</c> directly, bypassing
+    /// <see cref="UiRoot"/>'s real pointer pipeline -- which is exactly why the
+    /// regression this test guards against was invisible: BeginFavoriteDrag's
+    /// press-time removal fires SpellbookChanged, and a per-frame Tick() (as
+    /// production's RetailUiRuntime.Tick drives unconditionally) used to call
+    /// Rebuild(), which flushes and recreates every slot. UiRoot's subtree-removal
+    /// safety net then canceled the in-flight drag because its source cell had
+    /// just been destroyed out from under it. This drives the WHOLE gesture
+    /// through <see cref="UiRoot.OnMouseDown"/>/<see cref="UiRoot.OnMouseMove"/>/
+    /// a mid-drag <see cref="SpellcastingUiController.Tick"/>/
+    /// <see cref="UiRoot.OnMouseUp"/> and asserts both the retail-faithful final
+    /// order (insert-shift, not swap) and the exact remove-then-add wire pair.
+    /// </summary>
+    [Fact]
+    public void DragFavoriteOntoAnotherSlot_ThroughTheRealPointerPipeline_ReordersAndSyncsWire()
+    {
+        ImportedLayout layout = LayoutImporter.Build(
+            FixtureLoader.LoadCombatInfos(), NoTex, datFont: null);
+        RetailCombatLayout.FitFavoriteSlots(layout);
+        var spellbook = new Spellbook();
+        spellbook.OnSpellLearned(1u, 1f);
+        spellbook.OnSpellLearned(2u, 1f);
+        spellbook.OnSpellLearned(3u, 1f);
+        spellbook.SetFavorite(0, 0, 1u);
+        spellbook.SetFavorite(0, 1, 2u);
+        spellbook.SetFavorite(0, 2, 3u);
+        var objects = new ClientObjectTable();
+        objects.AddOrUpdate(new ClientObject { ObjectId = 1u, Name = "Player" });
+        var selection = new SelectionState();
+        var casting = new RuntimeSpellCastState(spellbook, selection, new NoopSpellCastOperations());
+        var adds = new List<(int Tab, int Position, uint SpellId)>();
+        var removes = new List<(int Tab, uint SpellId)>();
+
+        using SpellcastingUiController? controller = SpellcastingUiController.Bind(
+            layout, spellbook, casting, objects, () => 1u,
+            spellId => spellId,
+            item => item.ObjectId,
+            _ => { },
+            selection,
+            (tab, position, spellId) =>
+            {
+                adds.Add((tab, position, spellId));
+                spellbook.SetFavorite(tab, position, spellId);
+            },
+            (tab, spellId) =>
+            {
+                removes.Add((tab, spellId));
+                spellbook.RemoveFavorite(tab, spellId);
+            });
+        Assert.True(controller is not null, DescribeBinding(layout));
+
+        var screen = new UiRoot { Width = 1280f, Height = 800f };
+        screen.AddChild(layout.Root);
+        controller.Tick();
+
+        UiElement group = layout.FindElement(0x100000AAu)!;
+        UiItemList list = Descendants(group).OfType<UiItemList>().First();
+        UiCatalogSlot slot0 = Assert.IsType<UiCatalogSlot>(list.GetItem(0));
+        UiCatalogSlot slot2 = Assert.IsType<UiCatalogSlot>(list.GetItem(2));
+        Assert.Equal(1u, slot0.EntryId);
+        Assert.Equal(3u, slot2.EntryId);
+
+        System.Numerics.Vector2 p0 = slot0.ScreenPosition;
+        System.Numerics.Vector2 p2 = slot2.ScreenPosition;
+        int x0 = (int)(p0.X + slot0.Width / 2f);
+        int y0 = (int)(p0.Y + slot0.Height / 2f);
+        int x2 = (int)(p2.X + slot2.Width / 2f);
+        int y2 = (int)(p2.Y + slot2.Height / 2f);
+
+        // Press on spell 1's cell and drag past the 3px promotion threshold --
+        // BeginFavoriteDrag fires here and removes spell 1 from the model + wire.
+        screen.OnMouseDown(UiMouseButton.Left, x0, y0);
+        screen.OnMouseMove(x0 + 10, y0);
+        Assert.Same(slot0, screen.DragSource);
+        Assert.Equal([(0, 1u)], removes);
+
+        // A live per-frame Tick, exactly like production's RetailUiRuntime.Tick,
+        // must NOT cancel the drag just because the lift already changed the
+        // Spellbook -- this is the regression this test exists to catch.
+        controller.Tick();
+        Assert.Same(slot0, screen.DragSource);
+
+        // Move onto spell 3's cell and release there.
+        screen.OnMouseMove(x2, y2);
+        screen.OnMouseUp(UiMouseButton.Left, x2, y2);
+
+        Assert.Null(screen.DragSource);
+        // Insert-shift, not swap: dropping spell 1 onto spell 3's (pre-lift index 2)
+        // cell lands it directly before spell 3, matching PlayerModule::AddSpellFavorite
+        // @ 0x005D43E0's InsertPos semantics.
+        Assert.Equal(0, adds[0].Tab);
+        Assert.Equal(1, adds[0].Position);
+        Assert.Equal(1u, adds[0].SpellId);
+        Assert.Equal(new uint[] { 2u, 1u, 3u }, spellbook.GetFavorites(0));
+
+        // The list resyncs to the final state on the next tick.
+        controller.Tick();
+        Assert.Equal(2u, Assert.IsType<UiCatalogSlot>(list.GetItem(0)).EntryId);
+        Assert.Equal(1u, Assert.IsType<UiCatalogSlot>(list.GetItem(1)).EntryId);
+        Assert.Equal(3u, Assert.IsType<UiCatalogSlot>(list.GetItem(2)).EntryId);
+    }
+
     [Fact]
     public void FavoriteDrop_IgnoresForeignInventoryPayload()
     {
@@ -197,6 +302,42 @@ public sealed class SpellcastingUiControllerTests
         Assert.Equal([42u], spellbook.GetFavorites(0));
     }
 
+    /// <summary>
+    /// #354 discriminator sabotage check, symmetric to
+    /// <see cref="FavoriteDrop_IgnoresForeignInventoryPayload"/>: a catalog payload
+    /// minted by the favorite bar must be rejected by every PHYSICAL item slot too
+    /// (toolbar / inventory / vendor / paperdoll all route drops through
+    /// <see cref="IItemListDragHandler"/>, which only accepts <see cref="ItemDragPayload"/>).
+    /// If <see cref="SpellFavoriteDragPayload"/> were ever widened to match, a spell
+    /// icon dragged onto the toolbar would silently misbehave instead of no-op'ing.
+    /// </summary>
+    [Fact]
+    public void SpellFavoritePayload_IsRejectedByAPhysicalItemListDragHandler()
+    {
+        var recordedDrops = new List<uint>();
+        var handler = new RecordingDragHandler(recordedDrops);
+        var list = new UiItemList();
+        list.RegisterDragHandler(handler);
+        var cell = list.Cell;
+        var payload = new SpellFavoriteDragPayload(0, 0, 42u);
+
+        bool handled = cell.OnEvent(new UiEvent(
+            0, cell, UiEventType.DropReleased, Payload: payload));
+
+        Assert.True(handled);
+        Assert.Empty(recordedDrops);
+    }
+
+    private sealed class RecordingDragHandler(List<uint> drops) : IItemListDragHandler
+    {
+        public void OnDragLift(UiItemList sourceList, UiItemSlot sourceCell, ItemDragPayload payload)
+            => drops.Add(payload.ObjId);
+        public ItemDragAcceptance OnDragOver(UiItemList targetList, UiItemSlot targetCell, ItemDragPayload payload)
+            => ItemDragAcceptance.None;
+        public void HandleDropRelease(UiItemList targetList, UiItemSlot targetCell, ItemDragPayload payload)
+            => drops.Add(payload.ObjId);
+    }
+
     [Fact]
     public void SpellbookShortcutDrop_AddsToOpenTabWithoutRemovingLearnedSpell()
     {

tests/AcDream.Core.Tests/Spells/SpellbookTests.cs

@@ -199,6 +199,55 @@ public sealed class SpellbookTests
         Assert.Equal(1, book.ActiveCount);
     }
 
+    /// <summary>
+    /// #354: retail's SpellCastSubMenu::AddFavorite @ 0x004C7060 reorders a favorite
+    /// bar by removing the dragged spell from wherever it currently sits, then
+    /// InsertPos-ing it at the (possibly shifted) target index — never a two-slot
+    /// swap. Dragging slot 0 onto slot 2 must land it directly BEFORE the spell
+    /// that was there, shifting everything between one slot toward the vacated gap.
+    /// </summary>
+    [Fact]
+    public void SetFavorite_MovingWithinTheSameBar_InsertShiftsRatherThanSwaps()
+    {
+        var book = new Spellbook();
+        book.SetFavorite(0, 0, 1u);
+        book.SetFavorite(0, 1, 2u);
+        book.SetFavorite(0, 2, 3u);
+
+        // Drag spell 1 (index 0) onto spell 3's slot (index 2, already adjusted for
+        // the pre-lift removal by the caller, matching PlayerModule::AddSpellFavorite
+        // @ 0x005D43E0's InsertPos(list, position, spellId) contract).
+        book.SetFavorite(0, 1, 1u);
+
+        Assert.Equal(new uint[] { 2u, 1u, 3u }, book.GetFavorites(0));
+    }
+
+    [Fact]
+    public void SetFavorite_MovingToTheEnd_AppendsRatherThanLeavingAGap()
+    {
+        var book = new Spellbook();
+        book.SetFavorite(0, 0, 1u);
+        book.SetFavorite(0, 1, 2u);
+        book.SetFavorite(0, 2, 3u);
+
+        book.SetFavorite(0, 2, 1u);
+
+        Assert.Equal(new uint[] { 2u, 3u, 1u }, book.GetFavorites(0));
+    }
+
+    [Fact]
+    public void RemoveFavorite_LeavesTheRemainingBarContiguous()
+    {
+        var book = new Spellbook();
+        book.SetFavorite(0, 0, 1u);
+        book.SetFavorite(0, 1, 2u);
+        book.SetFavorite(0, 2, 3u);
+
+        book.RemoveFavorite(0, 2u);
+
+        Assert.Equal(new uint[] { 1u, 3u }, book.GetFavorites(0));
+    }
+
     [Fact]
     public void OnEnchantmentRemoved_FiresEvent()
     {