From cd9eb15cba37d5d6e9b6b413ea79d76d05ffdf91 Mon Sep 17 00:00:00 2001
From: bymyself <cbyrne@comfy.org>
Date: Mon, 4 May 2026 11:16:11 -0700
Subject: [PATCH] test+docs: harden assetsStore deletion+drift composition

Adversarial review (Skeptic, Architect, Minimalist lenses) plus an
oracle review surfaced two improvements without changing the underlying
pagination behavior:

- insertAssetSorted: tiebreak equal created_at timestamps with
  lexicographic id order so jobs that share a millisecond don't reorder
  across repeated incremental merges.
- removeHistoryItems: document the historyOffset decrement invariant.
  After a confirmed delete, the refreshed first page is backfilled with
  rows that previously sat past the old page-1 boundary; those
  backfilled rows count as drift, so the cursor must shrink by
  removedCount or the next loadMoreHistory will compose
  `historyOffset + drift` and silently skip unseen rows.
- Add a regression test that combines deletion with a non-zero drift
  counter and asserts the composed offset
  `(BATCH_SIZE - removed) + drift`. The existing tests covered each
  branch in isolation but not the composition.

Amp-Thread-ID: https://ampcode.com/threads/T-019df415-3078-74f9-9f05-c148c442293c
---
 src/stores/assetsStore.test.ts | 34 +++++++++++++++++++++++++++++++++-
 src/stores/assetsStore.ts      | 25 ++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/src/stores/assetsStore.test.ts b/src/stores/assetsStore.test.ts
index 1f95f45c62..885dc36007 100644
--- a/src/stores/assetsStore.test.ts
+++ b/src/stores/assetsStore.test.ts
@@ -450,7 +450,9 @@ describe('assetsStore - Refactored (Option A)', () => {
       vi.mocked(api.getHistory).mockResolvedValueOnce(mockHistory)
       await store.updateHistory()
 
-      // Delete 3 items — offset should shift from 200 to 197
+      // Delete 3 items — offset shifts from 200 to 197 so the next page
+      // request lines up with the server's post-deletion cursor and we don't
+      // skip the rows that backfilled into the old page-1 boundary.
       store.removeHistoryItems(['prompt_1', 'prompt_3', 'prompt_5'])
       expect(store.historyAssets).toHaveLength(197)
 
@@ -463,6 +465,36 @@ describe('assetsStore - Refactored (Option A)', () => {
       expect(api.getHistory).toHaveBeenLastCalledWith(200, { offset: 197 })
     })
 
+    it('should compose deletion offset with drift on next loadMore', async () => {
+      // Initial page 1 (200 items: prompt_0 .. prompt_199)
+      const page1 = Array.from({ length: 200 }, (_, i) => createMockJobItem(i))
+      vi.mocked(api.getHistory).mockResolvedValueOnce(page1)
+      await store.updateHistory()
+
+      // 3 net-new items merge in via a refresh — drift becomes 3.
+      const newJobs = Array.from({ length: 3 }, (_, i) =>
+        createMockJobItem(7000 + i)
+      )
+      const refreshedPage1 = [...newJobs, ...page1.slice(0, 197)]
+      vi.mocked(api.getHistory).mockResolvedValueOnce(refreshedPage1)
+      await store.updateHistory()
+
+      // Delete 2 loaded items — historyOffset should drop from 200 to 198.
+      store.removeHistoryItems(['prompt_10', 'prompt_20'])
+
+      // Next loadMore offset must compose deletion + drift:
+      //   adjustedOffset = (200 - 2) + 3 = 201
+      // If historyOffset is not decremented the request becomes 203 and the
+      // server rows at the new positions 201–202 are silently skipped.
+      const page2 = Array.from({ length: 200 }, (_, i) =>
+        createMockJobItem(200 + i)
+      )
+      vi.mocked(api.getHistory).mockResolvedValueOnce(page2)
+      await store.loadMoreHistory()
+
+      expect(api.getHistory).toHaveBeenLastCalledWith(200, { offset: 201 })
+    })
+
     it('should allow re-inserting a removed item on next updateHistory', async () => {
       const mockHistory = Array.from({ length: 3 }, (_, i) =>
         createMockJobItem(i)
diff --git a/src/stores/assetsStore.ts b/src/stores/assetsStore.ts
index 8e240d5c56..f44dff5e6c 100644
--- a/src/stores/assetsStore.ts
+++ b/src/stores/assetsStore.ts
@@ -149,9 +149,13 @@ export const useAssetsStore = defineStore('assets', () => {
     loadedIds.add(asset.id)
 
     const assetTime = new Date(asset.created_at ?? 0).getTime()
-    const insertIndex = allHistoryItems.value.findIndex(
-      (item) => new Date(item.created_at ?? 0).getTime() < assetTime
-    )
+    // Sort: newer first; ties broken by lexicographically larger id first
+    // so insertion order is stable across repeated merges.
+    const insertIndex = allHistoryItems.value.findIndex((item) => {
+      const itemTime = new Date(item.created_at ?? 0).getTime()
+      if (itemTime !== assetTime) return itemTime < assetTime
+      return item.id < asset.id
+    })
 
     if (insertIndex === -1) {
       allHistoryItems.value.push(asset)
@@ -161,6 +165,21 @@ export const useAssetsStore = defineStore('assets', () => {
     return true
   }
 
+  /**
+   * Remove items from the local view (used after the server confirmed the
+   * delete). Decrements historyOffset by the count of removed loaded items so
+   * the canonical caller flow — `removeHistoryItems(ids)` then
+   * `await updateHistory()` then `loadMoreHistory()` — produces the correct
+   * server offset:
+   *
+   * After deletion, the refreshed first page that `updateHistory()` fetches
+   * gets backfilled with items that previously sat just past the page-1
+   * boundary. Those backfilled items legitimately count as drift. If we
+   * leave `historyOffset` untouched, `loadMoreHistory()` then computes
+   * `historyOffset + drift` which double-counts the deletion: it adds the
+   * backfilled items on top of the original cursor and skips that many
+   * unseen rows on the next page.
+   */
   function removeHistoryItems(ids: string[]) {
     const idSet = new Set(ids)
     const removedCount = allHistoryItems.value.filter((item) =>