Item movement test harness + fixes for 8 movement bugs (#10253) by delphiactual · Pull Request #11813 · DestinyItemManager/DIM

delphiactual · 2026-06-18T00:09:40Z

Builds the item-movement test infrastructure asked for in #10253, then uses it to reproduce and fix a batch of open "Feature: Item Movement" bugs. Every fix is TDD'd - the regression test was verified to fail before the fix and pass after.

Test infrastructure (#10253)

src/testing/move-item-test-utils.ts: seeds the real Redux store from the sample profile and drives the actual move logic (executeMoveItem, moveItemTo, and full applyLoadout runs). Includes fixture helpers for setting up edge cases (full buckets, partial/unique stacks, postmaster items, cross-store replacements).
The Bungie.net write APIs are mocked, so moves never hit the network; the in-memory model is driven by the reducer.
31 tests across item-move-service, loadout-apply, auto-loadouts, and move-item, covering transfers (all directions), move-asides, stacking, postmaster pulls, equip/de-equip, the one-exotic rule, error paths, and end-to-end loadout application.

Bugs fixed

Issue	Fix
#8872, #8506	Unique-stack consumables (Hymn of Desecration, Ghost Fragments) failed to move via a filtered search. loadout-apply only marked equips/dequips as "involved", so the consumables space penalty made the destination look full → silent skip / move-aside cascade. Now marks every moved item involved.
#9121	Equipping a vault exotic for a different slot didn't de-equip the existing exotic — the blind-move fast path skipped the one-exotic check. Skip the blind move when equipping an item with an equipping label.
#8418 / #9416 pt2	A de-equip could pick an item the user was actively moving as the replacement. The move session now carries its involved items as exclusions, passed to `getSimilarItem`.
#3573 / #9416 pt1	Bulk search-to-vault could pull a search-matched item out of the vault as a de-equip replacement. The session is now built from all resolved loadout items, not just the ones that need to move.
#9416 pt3	`equipItems` bulk-equipped items assuming they were already in the character's inventory; replacements from the vault silently failed. Now moves each item onto the store first.
#9416 pt4	Exclusions matched by `{id, hash}`, but crafted/shaped items resolve to a new id (by `craftedDate`), so a loadout's own crafted item wasn't excluded. Now passes resolved items as exclusions. (No prior repro existed - this PR adds one.)
#11648	D1 "Max Light" left out the Ghost Shell / Artifact. In D1 these contribute to Light but aren't weapons/armor; they're now included for D1 stores.
#10046	Requesting the same move twice (e.g. clicking again while Bungie.net is slow) failed the second one on a stale item reference. The queued move now re-resolves the live item and no-ops if it's already at the destination.

Investigated, no change needed

Transferring Double Solstice Armor Causes DIM to try and transfer everything #6895 - same-hash unique-stack char-to-char scenario already stops cleanly (a StackFull error, no runaway). The 2021 "transfer everything" cascade relied on a cross-hash game-level constraint DIM doesn't model and isn't reproducible today. Added a regression test to lock in the clean-stop behavior; left the "smart swap" enhancement out as out-of-scope.

Closes #8872
Closes #8506
Closes #9121
Closes #8418
Closes #3573
Closes #9416
Closes #11648
Closes #10046
Closes #7935
Closes #10253

Lays the foundation for testing the item-move-service smart-move logic: - src/testing/move-item-test-utils.ts: builds fresh DimStores from the sample profile, seeds the Redux store with a sample account, and exposes a move() helper that drives executeMoveItem. Includes fixture mutators (setBucketFreeSlots, cloneItem, add/removeItemFromStore) for setting up full-bucket / free-space scenarios, plus an uncached bucket query since findItemsByBucket is weakMemoized on store identity. - src/app/inventory/item-move-service.test.ts: mocks the Bungie.net transfer/equip/lock APIs and covers vault<->character moves, character-to-character (two-hop via vault), equip-in-place, make-space move-aside, and the no-space error case.

Extend the item-move-service suite with stack-handling coverage: - moving part of a stack leaves the remainder on the source - moving a full stack into a store that already has one merges totals Adds findStackable / findSplitStack helpers driven by the sample profile. Postmaster pulls are left as a TODO: the blind-pull path to the current character calls transfer but doesn't relocate the item out of the postmaster in the model, which needs a closer look first.

Resolves the earlier postmaster TODO. The original failure wasn't a bug: the sample profile's only postmaster item is an Exotic Engram, and the Engrams bucket is itself in the Postmaster category, so engrams always report location.inPostmaster (location.hash === bucket.hash). Pulling one is a no-op in the model. A genuine "lost item" lives in the LostItems bucket with its real destination bucket preserved. Adds a placeItemInPostmaster fixture (plus getTestBuckets) that models this, and a test confirming a pull relocates the item out of the postmaster into its destination bucket.

Covers pulling a lost item from one character's postmaster to a different character. executeMoveItem first recurses to pull the item onto its own character, then runs the normal char->vault->char transfer - three transfer calls total. Asserts the item lands on the target out of the postmaster and is gone from the source.

Covers the one-exotic rule: equipping a second exotic weapon (different bucket, same equipping label) forces the currently-equipped exotic off, replacing it with a similar non-exotic. Two equips total. Asserts only one exotic weapon ends up equipped and the old exotic's slot holds a non-exotic.

Four more cases: - equip an item pulled from the vault (transfer + equip) - refuse to equip an item the character can't use (wrong class -> throws, no transfer) - de-equip an equipped item when moving it to the vault (a similar item replaces it in the slot) - refuse to overfill a unique stack (StackFull). The unique stack must be a Consumables item; subclasses and other unique stacks hit a blind-move fast path to the current character that skips the check.

…, #8506) Bulk-moving items via a filtered search goes through loadout-apply, whose move session only marked equipped/dequipped items as "involved". Items merely being moved (e.g. consumables sent to the vault) were treated as incidental, so spaceLeftWithReservations applied its consumables penalty (left -= maxStackSize). For unique-stack consumables - whose available space is capped at one stack - this zeroed out the space, making the destination look full. The result was either the item silently not moving (#8506) or a cascade of move-asides that emptied the vault onto characters (#8872). Drag/drop worked because moveItemTo marks the dragged item involved. Mark every loadout-moved item as involved, not just equips/dequips. Adds an involvedItems option to the test harness move() helper and two regression tests reproducing the mechanism with a large unique-stack consumable (Hymn of Desecration).

) Double-clicking an exotic in the vault for a different slot than an already -equipped exotic failed to equip: the existing exotic wasn't moved aside, so the game rejected equipping a second exotic. The cause is the "blind move from vault/postmaster to the current character" fast path, which calls moveToStore directly and skips ensureValidTransfer - and therefore the one-exotic canEquipExotic check that de-equips the other exotic (pulling a replacement from the vault if needed). Skip the blind move when we're equipping an item with an equippingLabel so exotic equips go through the full validation path. Adds a regression test that reproduces the two-exotics-equipped state.

When an equipped item has to be de-equipped so it can move, getSimilarItem chose its replacement without any exclusions, so it could equip an item the user is actively moving elsewhere - fighting the move it belongs to. Track the move session's explicitly-involved items as an exclusion list and pass it to getSimilarItem in dequipItem. This is point 2 of the issues catalogued in #9416 (single-dequip not converting session items to exclusions); the bulk-dequip paths in loadout-apply remain. Adds a regression test (verified to fail without the fix) and exposes getState from the test harness.

When a filtered-search transfer de-equips an item and has to pull a replacement from the vault, it could pick a search-matched item that's meant to stay in the vault (or move). The bulk-dequip path already excluded applicableLoadoutItems, but the single-dequip path (via executeMoveItem -> dequipItem) didn't, so it was weaker (#9416 point 1). Build the move session from applicableLoadoutItems (all resolved loadout items, including ones already in the vault) instead of only the items that need to move. Combined with dequipItem now consulting the session's exclusions (#8418), the single- and bulk-dequip paths behave consistently. Resolving via getLoadoutItem keeps exclusion ids accurate for shaped items. No automated test: applyLoadout is too heavy to drive in the current harness. The underlying exclusion mechanism is covered by the #8418 service test, and the 49 loadout-reducer tests still pass.

Make applyLoadout drivable in tests: the harness now exposes dispatch, and seeding the manifest (setD2Manifest) plus stubbing getCharacters is enough to run a real loadout application against the sample profile. Two regression tests, each verified to fail when its fix is reverted: - bulk-moving unique-stack consumables (incl. Hymn of Desecration) to the vault completes without a move-aside cascade (#8872 / #8506) - de-equipping during a bulk move doesn't pull a search-matched item out of the vault as the replacement (#3573 / #9416) These give the loadout-apply involvedItems fixes real end-to-end coverage.

D1 Light is computed from weapons, armor, and General-category gear like the Ghost Shell and Artifact (each contributes a Light tier). But maxLightItemSet only gathered weapons and armor, so "Max Light" never moved or equipped the highest-light ghost (or artifact). Include the Ghost and Artifact buckets in the applicable set for D1 stores only - D2 power ignores them. Adds unit tests for maxLightItemSet (verified to fail without the fix).

Moves are queued, but moveItemTo captured the item reference up front and only checked "nothing to do" at request time. If the same move was requested twice (e.g. the user clicked again while Bungie.net was slow), the second queued request ran against a now-stale item whose owner/location still pointed at the original spot, so it tried to move an item that was no longer there and failed. Re-resolve the item to its live copy inside the queued action, and finish as a no-op if it's already at the destination (or gone). Adds a regression test driving moveItemTo twice (verified to fail without the fix).

Pulling a stackable account-wide item (e.g. strange coins / consumables) from another character's postmaster, with the vault full, sent DIM into a runaway of move-asides shuffling items in and out of the vault. ensureCanMoveToStore reserves transient vault space for guardian-to-guardian transfers, because those route through the vault. But account-wide items go straight to the current character without a vault hop, so the reservation was spurious - and with a full vault it forced needless move-asides. Exclude account-wide items from the guardian-to-guardian vault reservation. Adds a regression test (cross-character postmaster pull into a full vault, verified to do an extra move-aside without the fix).

equipItems calls the bulk-equip API assuming every item is already in the target character's inventory. But the items it's handed - e.g. de-equip replacements chosen by getSimilarItem during a bulk loadout dequip - can live in the vault or on another character, and you can't equip an item that isn't there, so the bulk equip silently failed for them. (The exotic move-aside path already moved cross-store items first; the main items didn't.) Move each item onto the store first (items already there are untouched, so the other caller - the main loadout bulk-equip, whose items are already on the store - is unaffected). Adds a service-level regression test with a bulk-equip API mock that only succeeds for items in the store's inventory (verified to fail without the fix).

…nt 4) The bulk-dequip and per-item move paths passed the raw applicableLoadoutItems (unresolved loadout items) as exclusions. Exclusions match by {id, hash}, but crafted/shaped items resolve to a different id than the loadout stores (a new id matched back by craftedDate), so the loadout's own crafted item wasn't excluded and could be picked as a de-equip replacement or moved aside. Pass the resolved items (involvedItems) instead, in the getSimilarItem exclusions, the bulk equipItems exclusions, and the applyLoadoutItem move-aside excludes. robojumper had no reproducer for this; adds an end-to-end one - a crafted loadout item whose loadout-item id is stale - verified to fail without the fix (the crafted dupe gets equipped as the replacement).

A character-to-character move of a unique-stack item routes through the vault. If a same-stack copy already sits in the vault, the item can't transit. DIM already stops cleanly here (a StackFull error, no transfers) - this locks that in so it can't regress into the "transfer everything" runaway from the original report. The cross-hash Solstice cascade in that report relied on a game-level unique constraint DIM doesn't model, and isn't reproducible with current mechanics.

bhollis

This is great. I'd tried to have Claude do this maybe 6 months ago and it couldn't really hack it.

bhollis · 2026-06-18T02:12:08Z

+/**
+ * Build a *fresh* set of DimStores from the sample profile. Unlike
+ * `getTestStores` in test-utils, this is not memoized, so each call returns new
+ * mutable store/item objects that fixture helpers are free to modify.


Same question - why would we mutate? The cache should still be valid if we update items immutably.

We can use immer in the helpers.

Done. With the helpers immutable, setupMoveTestStore no longer needs to refresh store identities before dispatching, so that hack is gone too. I used plain spread updates rather than immer since the helpers are small, but I'm happy to switch to produce if you'd prefer the consistency.

bhollis · 2026-06-18T02:12:10Z

+ * Non-memoized version of findItemsByBucket. `findItemsByBucket` is weakMemoized
+ * on the store object, so reading it after mutating `store.items` in place
+ * returns stale data. Fixture helpers must use this instead.
+ */


Why do we need to mutate store.items instead of changing them immutably through the reducers?

Good call, no reason to. I reworked all the fixture helpers to be pure and immutable. They take the stores array and return a new one (addItemToStore and placeItemInPostmaster return [stores, placedItem], the rest return new stores). Touched stores get a fresh identity, so the findItemsByBucket weak-memo stays valid, which let me delete itemsInBucketUncached entirely.

bhollis · 2026-06-18T02:14:32Z

-                applicableLoadoutItems,
-                moveSession,
-              ),
+              equipItems(getStore(getStores(), owner)!, itemsToEquip, involvedItems, moveSession),


I feel like we missed a prettier invocation here?

comes back clean, for me

bhollis · 2026-06-18T02:17:03Z

+   * equip - they're items the user is actively moving, so equipping one would
+   * fight the move it's part of.
+   */
+  involvedItemExclusions: Exclusion[];


It feels like this doesn't need to be separate from involvedItems and/or involvedItems could just carry both id and hash?

Agreed. I collapsed them into a single involvedItems: Exclusion[] carrying both id and hash. The consumables-penalty check now does a hash .some() over it, and it's passed straight to getSimilarItem as the exclusion list.

bhollis · 2026-06-18T02:21:30Z

+      itemsOnStore.push(
+        i.owner === store.id
+          ? i
+          : await dispatch(executeMoveItem(i, store, { equip: false }, session)),


loadout-apply already moves items into the right place in the common case that this is called - an unstated prerequisite of equipItems is that the items should be in the desired store. I can see where we missed that in the dequip loop but it might be better to either explicitly move the dequip items (from getSimilarItem) onto the right store first.

I documented the precondition (callers should have the items on the store, and loadout-apply does) and kept the loop as a safety net specifically for the getSimilarItem dequip replacements, which can still come from the vault. If you'd rather I move those onto the store at the loadout-apply callsite and drop the guard here, I'm happy to. Let me know which you prefer.

bhollis · 2026-06-18T17:34:06Z

+    expect(amountOfItem(getVault(newStores)!, item)).toBe(startVault + startSource);
+  });
+
+  it('pulls a lost item out of the postmaster onto its character', async () => {


We should repeat the postmaster tests for all of the "bucket full", "vault full", etc. combos. Maybe worth making it a parameterized/table test helper.

bhollis · 2026-06-18T17:34:46Z

+    expect(newOwner.items.some((i) => i.id === item.id && !i.location.inPostmaster)).toBe(true);
+  });
+
+  it('pulls a lost item out of the postmaster onto another character', async () => {


For all of the move scenarios we should also make sure to test stacked items vs. not stacked. Building up a kind of test matrix.

bhollis · 2026-06-18T17:36:19Z

+  // Regression tests for issues #8872 / #8506: bulk-moving consumables via a
+  // filtered search (which goes through loadout-apply) broke on unique-stack
+  // consumables like Hymn of Desecration and Ghost Fragments.
+  it('moves a large unique-stack consumable to the vault in one transfer (#8872)', async () => {


The stack tests should cover moving a stack to somewhere that doesn't have any of that stack, a place that has one stack but room for the incoming stack, a place that has one stack but will overflow to a second item, a place where the destination stack can't overflow because it has to be a single item, etc.

bhollis · 2026-06-18T17:37:16Z

+  // Regression test for issue #9121: equipping an exotic pulled from the vault
+  // must still de-equip the existing exotic, even when the replacement for that
+  // exotic's slot has to come from the vault too.
+  it('de-equips the existing exotic when equipping a vault exotic that needs a vault replacement (#9121)', async () => {


Yeah these test cases are all good but it's making the case for an auto-generated set of "all unique movement possibilities".

This pushed me to add a parameterized matrix in a new file, item-move-matrix.test.ts, using describe.each tables per axis:

source by destination (vault, current, other) for an instanced item

destination fullness: room, bucket-full-but-moveable, full-and-unmovable, asserting success or no-space (262)

stack merge and overflow at both vault and character destinations: dest has none, a partial stack with room, or a stack that overflows into a second, asserting both conservation and the physical-stack count (543), plus the unique-stack single-item refusal

postmaster pulls across destination and fullness states (313)

The stack rows use vault-to-char since a char-to-char consumable move just redirects to the current character. I also left out the "vault full and buckets full but another character has room, so it cascades" combo (262) for now, since it's hard to set up deterministically without the API rejecting move-asides. I took it as a per-axis table rather than a single auto-generated cartesian. Let me know if you'd like it expanded toward the fully-generated version.

bhollis · 2026-06-18T17:39:15Z

+  isVault: false,
+} as unknown as DimStore;
+
+describe('maxLightItemSet', () => {


IMO these tests are too specific and generally low value

Trimmed to just the #11648 ghost-shell regression, and dropped the artifact, highest-light, and D2-ghost cases.

- Collapse MoveSession.involvedItems/involvedItemExclusions into a single Exclusion[] carrying both id and hash. - Document equipItems' precondition (items should be on the store) and keep the move-onto-store loop as a safety net for getSimilarItem dequip replacements. - Rework the move test fixture helpers to be pure/immutable (stores in, stores out), dropping itemsInBucketUncached and the fresh-identity remap in setupMoveTestStore now that the findItemsByBucket memo stays valid. - Make the test find helpers throw instead of returning undefined; hoist buildFreshStores into beforeEach. - Trim auto-loadouts.test.ts to just the #11648 ghost-shell regression. - Add item-move-matrix.test.ts: a parameterized movement matrix covering source x destination, destination fullness (room/move-aside/no-space), stack merge/overflow at vault and character destinations, unique-stack refusal, and postmaster pulls across destination + fullness states.

delphiactual added 17 commits June 16, 2026 15:14

delphiactual added the Feature: Item Movement label Jun 18, 2026

delphiactual temporarily deployed to pr June 18, 2026 00:09 — with GitHub Actions Inactive

bhollis reviewed Jun 18, 2026

View reviewed changes

delphiactual temporarily deployed to pr June 18, 2026 21:19 — with GitHub Actions Inactive

delphiactual temporarily deployed to pr June 18, 2026 21:32 — with GitHub Actions Inactive

delphiactual force-pushed the item-move-test branch from e343a34 to 7356f78 Compare June 18, 2026 21:36

delphiactual had a problem deploying to pr June 18, 2026 21:36 — with GitHub Actions Error

delphiactual deployed to pr June 18, 2026 21:48 — with GitHub Actions View deployment

delphiactual requested a review from bhollis June 18, 2026 21:52

Conversation

delphiactual commented Jun 18, 2026

Test infrastructure (#10253)

Bugs fixed

Investigated, no change needed

Uh oh!

bhollis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delphiactual Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

delphiactual Jun 18, 2026 •

edited

Loading