mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 22:58:08 +00:00
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
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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) =>
|
||||
|
||||
Reference in New Issue
Block a user