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.
52 tests across item-move-service, item-move-matrix, loadout-apply, auto-loadouts, and move-item, including a parameterized movement matrix (source/destination pairs, destination fullness, stack merge/overflow, postmaster pulls), and covering transfers (all directions), move-asides, stacking, 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; de-equip replacements pulled from the vault silently failed. `loadout-apply` now moves those replacements onto the target store before bulk-equipping, and `equipItems` documents the on-store precondition.
#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

bhollis

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

- 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.

Table-driven coverage addressing the review's "auto-generated set of all unique movement possibilities" comments: - source x destination matrix for an instanced weapon - destination-fullness states (room / move-aside / no-space) - stack merge/overflow at both vault and character destinations (none / partial / overflow-to-second), plus unique-stack refusal - postmaster pulls across destination + fullness states

- 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.

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.

- 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.

…erage equipItems no longer moves off-store items itself; loadout-apply moves the getSimilarItem dequip replacements onto the target store before the bulk equip. equipItems now documents an on-store precondition. Adds a vault-fill test helper and expands the move matrix with vault-full cascade-to-another-character, an everything-full no-space case, more postmaster destination/fullness combos, and stacked-vs-unstacked destination coverage.

The sample profile no longer contains a stuck (cannot-currently-roll) item, so the old data-dependent assertion was commented out. Test buildDefinedPlug's currentlyCanRoll mapping directly instead, which doesn't depend on volatile profile contents.

bhollis

I'm excited about this!

The parameterized matrix in item-move-matrix.test.ts already covers the basic transfer, make-room, stack, and postmaster scenarios, so the duplicate hand-written tests in item-move-service.test.ts are removed. That file now keeps only equip/de-equip semantics and the bug regressions. Folded the assertions the matrix lacked (transfer counts, no-stray-copy) into the matrix first.

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 temporarily deployed to pr June 18, 2026 21:48 — with GitHub Actions Inactive

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

delphiactual added 18 commits June 23, 2026 09:43

delphiactual force-pushed the item-move-test branch from 7356f78 to c7b4dfe Compare June 23, 2026 15:53

delphiactual temporarily deployed to pr June 23, 2026 15:53 — with GitHub Actions Inactive

delphiactual temporarily deployed to pr June 23, 2026 16:24 — with GitHub Actions Inactive

delphiactual temporarily deployed to pr June 23, 2026 16:36 — with GitHub Actions Inactive

Tighten move-matrix test comments

fe7ff62

delphiactual temporarily deployed to pr June 23, 2026 17:04 — with GitHub Actions Inactive

bhollis approved these changes Jun 30, 2026

View reviewed changes

Comment thread src/app/inventory/item-move-matrix.test.ts

delphiactual temporarily deployed to pr June 30, 2026 11:13 — with GitHub Actions Inactive

delphiactual added 2 commits June 30, 2026 07:26

Merge remote-tracking branch 'origin/master' into item-move-test

7d537fe

Reformat with prettier 3.9.4

10b0ef6

delphiactual temporarily deployed to pr June 30, 2026 13:28 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/master' into item-move-test

b6a3427

delphiactual deployed to pr June 30, 2026 14:50 — with GitHub Actions View deployment

delphiactual merged commit 9051e40 into master Jun 30, 2026
4 checks passed

delphiactual deleted the item-move-test branch June 30, 2026 14:55

delphiactual temporarily deployed to pr June 30, 2026 14:55 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Item movement test harness + fixes for 8 movement bugs (#10253)#11813

Item movement test harness + fixes for 8 movement bugs (#10253)#11813
delphiactual merged 25 commits into
masterfrom
item-move-test

delphiactual commented Jun 18, 2026 •

edited

Loading

Uh oh!

bhollis left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhollis left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Test infrastructure (#10253)

Bugs fixed

Investigated, no change needed

Uh oh!

bhollis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhollis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

delphiactual commented Jun 18, 2026 •

edited

Loading