scriptoriumadmin/zotero
1
0
Fork 0
mirror of https://github.com/zotero/zotero.git synced 2026-10-07 02:58:09 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

drop refreshing of itemTree during embedding

Browse source 
Rerunning itemTree refresh on embedding progress takes
a good amount of work on the main thread, which can
lead to frozen UI if there is an active search when
embedding runs.

Refreshing the itemTree during a search is likely not
necesary because it is a bit jarring to see the row order
change all of a sudden as you scroll down the itemTree, and
lexical search is a decent fallback until indexing
is ready.
This commit is contained in:
Bogdan Abaev 2026-09-09 14:19:07 -07:00
parent fa0b67da2f
commit 484a9a1014
6 changed files with 46 additions and 182 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

37
chrome/content/zotero/collectionViewItemTree.jsx
View file

@ -184,17 +184,12 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
}
/**
* Compute the current index coverage across the selected rows' libraries
* (see Zotero.BestMatch.getIndexState())
* Compute the current index coverage (see Zotero.BestMatch.getIndexState())
*
* @return {Promise<Object|null>}
*/
async _getBestMatchIndexState() {
return Zotero.BestMatch.getIndexState(
this.collectionTreeRows
.map(row => row.ref?.libraryID)
.filter(id => id !== undefined)
);
return Zotero.BestMatch.getIndexState();
}
/**
@ -576,12 +571,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
this.itemTree._refreshPromise = deferred.promise;
try {
// A best-match rerank after an embeddings change reuses the cached
// search results -- only the ranking depends on the embeddings, so
// there's no need to re-run the underlying search
if (!options.reuseSearchResults) {
this.collectionTreeRows.forEach(row => row.clearCache());
}
this.collectionTreeRows.forEach(row => row.clearCache());
this._bestMatchIndexState = null;
// Get the full set of items we want to show, merged across all selected rows
let newSearchItemSet = new Set();
@ -942,7 +932,6 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
var madeChanges = false;
var refresh = false;
var reuseSearchResults = false;
var sort = false;
// Selection strategy
@ -993,23 +982,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
if (items.length == 0) return;
}
if (action == 'refresh' && type == 'item' && extraData && extraData.embeddingsUpdate
&& collectionTreeRows.some(rowIsBestMatchSearch)) {
// The background indexer committed new or changed embeddings, so
// rerun the active best-match search to update the scores and ranks.
// Only the ranking depends on the embeddings, so unless a selected
// row trims membership by score (a top-K cutoff), the underlying
// search results are unchanged and can be reused while just the
// ranking is recomputed.
reuseSearchResults = collectionTreeRows.every((row) => {
return typeof row.hasBestMatchCutoff == 'function'
&& !row.hasBestMatchCutoff();
});
this.itemTree.invalidateRowCache(ids);
refresh = true;
madeChanges = true;
}
else if (action == 'refresh' && type == 'item' && this._bestMatchSession
if (action == 'refresh' && type == 'item' && this._bestMatchSession
&& ids.some(id => this._bestMatchSession.getPreviews(parseInt(id)))) {
// The invalidation above reset these items' previews, and only a
// scoring pass derives previews, so re-run the search
@ -1292,7 +1265,7 @@ class CollectionViewItemTreeRowProvider extends ItemTreeRowProvider {
}
if (refresh) {
await this._refresh({ reuseSearchResults });
await this._refresh();
}
if (sort) {
await this.itemTree._ensureSortContextReady();

32
chrome/content/zotero/xpcom/bestMatch.js
View file

@ -132,37 +132,33 @@ Zotero.BestMatch = new function () {
};
/**
* Embedding-index coverage over the given libraries, for banners
* explaining incomplete best-match results. Null when the semantic
* engine is disabled or every eligible item is indexed. Never throws --
* the state is informational and shouldn't break a search.
* Embedding-index coverage, for banners explaining incomplete best-match
* results. Null when the semantic engine is disabled or everything
* eligible is indexed. Never throws -- the state is informational and
* shouldn't break a search.
*
* @param {Number[]} [libraryIDs] - Limit coverage to these libraries;
* all libraries when empty
* @return {Promise<Object|null>} - { type: 'indexing'|'paused', indexed,
* total }
* total }, where the counts add items to attachment chunks
*/
this.getIndexState = async function (libraryIDs = []) {
this.getIndexState = async function () {
try {
let status = Zotero.Embeddings.Indexing.getStatus();
if (!status.enabled) {
return null;
}
// Counts aren't populated until the indexer runs in this session
if (!status.libraries.length) {
if (!status.items.total && !status.chunks.total) {
status = await Zotero.Embeddings.Indexing.refreshStatus();
}
let ids = new Set(libraryIDs);
let libraries = status.libraries
.filter(lib => !ids.size || ids.has(lib.libraryID));
// Coverage is coverage: attachment fulltext is reported separately
// in the preferences, but an incomplete index is incomplete
// whichever part of it is still filling in
let indexed = libraries.reduce(
(sum, lib) => sum + lib.indexed + lib.indexedAttachments, 0);
let total = libraries.reduce(
(sum, lib) => sum + lib.eligible + lib.eligibleAttachments, 0);
if (indexed >= total) {
// whichever part of it is still filling in. Queued work and
// documents still being extracted aren't in the totals yet.
let indexed = status.items.done + status.chunks.done;
let total = status.items.total + status.chunks.total;
let pending = status.queued.items || status.queued.attachments
|| status.extractionProgress;
if (indexed >= total && !pending) {
return null;
}
// Only an explicit pause reports as paused. Anything else --

42
chrome/content/zotero/xpcom/embeddings.js
View file

@ -1741,12 +1741,6 @@ Zotero.Embeddings.Indexing = new function () {
// clear/prune/re-index steps concurrently.
let _switchChain = Promise.resolve();
// Items whose embeddings were written but not yet announced to views, and
// the coalescing timer for the announcement (see _notifyIndexed())
let _indexedNotifyIDs = new Set();
let _indexedNotifyTimer = null;
const INDEXED_NOTIFY_DELAY = 2000;
/**
* Wire up the background indexer. Guarded so multiple windows don't
* double-initialize.
@ -2208,17 +2202,8 @@ Zotero.Embeddings.Indexing = new function () {
// the computed vectors, not the downloaded model files.
async function _clearEmbeddings() {
await Zotero.Embeddings.initDB();
// Announce the removals, so active semantic views refresh after the
// notification's coalescing delay (e.g. after disabling or a model
// switch)
let cleared = await Zotero.DB.columnQueryAsync(
"SELECT DISTINCT itemID FROM embeddings.itemEmbeddings"
);
await Zotero.DB.queryAsync("DELETE FROM embeddings.itemEmbeddings");
await Zotero.DB.queryAsync("DELETE FROM embeddings.itemChunkCounts");
if (cleared.length) {
_notifyIndexed(cleared);
}
}
// Indexed items, notes and annotations -- the numerator for their
@ -2906,10 +2891,7 @@ Zotero.Embeddings.Indexing = new function () {
}
}
});
if (completed.length) {
_notifyIndexed(completed.map(entry => entry.item.id));
done += completed.length;
}
done += completed.length;
if (onProgress) {
onProgress({ done, total: toEmbed.length });
}
@ -2924,28 +2906,6 @@ Zotero.Embeddings.Indexing = new function () {
.filter(library => ['user', 'group'].includes(library.libraryType));
}
// Announce written or removed embeddings with a 'refresh' item event, so
// an active best-match search reranks as vectors change (e.g. during
// initial indexing, or after a clear). The embeddingsUpdate flag lets the
// item tree rerank only for these events, not for every refresh. Coalesced,
// so a long indexing run produces an update every couple of seconds rather
// than one per committed batch.
function _notifyIndexed(itemIDs) {
for (let id of itemIDs) {
_indexedNotifyIDs.add(id);
}
if (_indexedNotifyTimer) {
return;
}
_indexedNotifyTimer = setTimeout(() => {
_indexedNotifyTimer = null;
let ids = [..._indexedNotifyIDs];
_indexedNotifyIDs.clear();
Zotero.Notifier.trigger('refresh', 'item', ids, { embeddingsUpdate: true })
.catch(e => Zotero.logError(e));
}, INDEXED_NOTIFY_DELAY);
}
/**
* Current runner state, for the preferences UI.
*

2
chrome/content/zotero/xpcom/notifier.js
View file

@ -27,7 +27,7 @@
Zotero.Notifier = new function () {
// Options that apply to an entire event, not a specific object
this.EVENT_LEVEL_OPTIONS = ['autoSyncDelay', 'skipAutoSync', 'embeddingsUpdate'];
this.EVENT_LEVEL_OPTIONS = ['autoSyncDelay', 'skipAutoSync'];
var _observers = {};
var _types = [

105
test/tests/collectionViewItemTreeTest.js
View file

@ -277,13 +277,10 @@ describe("CollectionViewItemTree", function () {
enabled: true,
indexing: false,
paused: false,
libraries: [{
libraryID: Zotero.Libraries.userLibraryID,
indexed: 0,
eligible: 0,
indexedAttachments: 0,
eligibleAttachments: 0
}]
items: { done: 1, total: 1 },
chunks: { done: 0, total: 0 },
queued: { items: 0, attachments: 0 },
extractionProgress: null
}));
Zotero.Prefs.set('search.quicksearch-mode', 'bestMatch');
});
@ -891,13 +888,10 @@ describe("CollectionViewItemTree", function () {
enabled: true,
indexing: true,
paused: false,
libraries: [{
libraryID: Zotero.Libraries.userLibraryID,
indexed: 700,
eligible: 9000,
indexedAttachments: 52,
eligibleAttachments: 553
}]
items: { done: 700, total: 9000 },
chunks: { done: 52, total: 553 },
queued: { items: 0, attachments: 0 },
extractionProgress: null
});
await select(win, col);
@ -919,13 +913,10 @@ describe("CollectionViewItemTree", function () {
enabled: true,
indexing: false,
paused: false,
libraries: [{
libraryID: Zotero.Libraries.userLibraryID,
indexed: 700,
eligible: 9000,
indexedAttachments: 52,
eligibleAttachments: 553
}]
items: { done: 700, total: 9000 },
chunks: { done: 52, total: 553 },
queued: { items: 0, attachments: 0 },
extractionProgress: null
});
await itemsView.setFilter('search', 'between runs query');
banner = win.document.querySelector('.best-match-index-banner');
@ -934,13 +925,10 @@ describe("CollectionViewItemTree", function () {
enabled: true,
indexing: false,
paused: true,
libraries: [{
libraryID: Zotero.Libraries.userLibraryID,
indexed: 700,
eligible: 9000,
indexedAttachments: 52,
eligibleAttachments: 553
}]
items: { done: 700, total: 9000 },
chunks: { done: 52, total: 553 },
queued: { items: 0, attachments: 0 },
extractionProgress: null
});
await itemsView.setFilter('search', 'paused query');
banner = win.document.querySelector('.best-match-index-banner');
@ -951,13 +939,10 @@ describe("CollectionViewItemTree", function () {
enabled: true,
indexing: false,
paused: false,
libraries: [{
libraryID: Zotero.Libraries.userLibraryID,
indexed: 9000,
eligible: 9000,
indexedAttachments: 553,
eligibleAttachments: 553
}]
items: { done: 9000, total: 9000 },
chunks: { done: 553, total: 553 },
queued: { items: 0, attachments: 0 },
extractionProgress: null
});
await itemsView.setFilter('search', 'another query');
assert.notOk(win.document.querySelector('.best-match-index-banner'));
@ -1083,28 +1068,7 @@ describe("CollectionViewItemTree", function () {
await itemsView.setFilter('advanced-search', null);
});
it("should rerank when the indexer announces changed embeddings", async function () {
let col = await createDataObject('collection');
let itemA = await createDataObject('item', { title: "rerank A", collections: [col.id] });
let itemB = await createDataObject('item', { title: "rerank B", collections: [col.id] });
let best = itemA.id;
stubs.push(sinon.stub(Zotero.Embeddings, 'scoreItemIDs').callsFake(scoreEnvelope(
async (query, itemIDs) => new Map(itemIDs.map(id => [id, id == best ? 0.9 : 0.5]))
)));
await select(win, col);
itemsView = zp.itemsView;
await itemsView.setFilter('search', 'some query');
assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]);
// The indexer's coalesced notification after new/removed vectors
best = itemB.id;
await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id], { embeddingsUpdate: true });
await itemsView._refreshPromise;
assert.deepEqual(itemsView._rows.map(row => row.id), [itemB.id, itemA.id]);
});
it("shouldn't rerank on a refresh that isn't an embeddings update", async function () {
it("shouldn't rerank on a refresh", async function () {
let col = await createDataObject('collection');
let itemA = await createDataObject('item', { title: "norerank A", collections: [col.id] });
let itemB = await createDataObject('item', { title: "norerank B", collections: [col.id] });
@ -1118,38 +1082,13 @@ describe("CollectionViewItemTree", function () {
await itemsView.setFilter('search', 'some query');
assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]);
// An unrelated refresh (e.g. a field change) leaves the ranking alone
// A refresh (e.g. a field change) leaves the ranking alone
best = itemB.id;
await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id]);
await itemsView._refreshPromise;
assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]);
});
it("should rerank without re-running the search when embeddings change", async function () {
let col = await createDataObject('collection');
let itemA = await createDataObject('item', { title: "reuse A", collections: [col.id] });
let itemB = await createDataObject('item', { title: "reuse B", collections: [col.id] });
let best = itemA.id;
stubs.push(sinon.stub(Zotero.Embeddings, 'scoreItemIDs').callsFake(scoreEnvelope(
async (query, itemIDs) => new Map(itemIDs.map(id => [id, id == best ? 0.9 : 0.5]))
)));
await select(win, col);
itemsView = zp.itemsView;
await itemsView.setFilter('search', 'some query');
assert.deepEqual(itemsView._rows.map(row => row.id), [itemA.id, itemB.id]);
// The underlying search is dropped and re-run by clearing the row cache
let clearCacheSpy = sinon.spy(Zotero.CollectionTreeRow.prototype, 'clearCache');
stubs.push(clearCacheSpy);
best = itemB.id;
await Zotero.Notifier.trigger('refresh', 'item', [itemA.id, itemB.id], { embeddingsUpdate: true });
await itemsView._refreshPromise;
assert.deepEqual(itemsView._rows.map(row => row.id), [itemB.id, itemA.id]);
assert.isFalse(clearCacheSpy.called);
});
});
it("should expand parent item and attachment for an annotation match", async function () {

10
test/tests/embeddingsTest.js
View file

@ -1285,7 +1285,7 @@ describe("Zotero.Embeddings", function () {
});
describe("Indexing", function () {
it("should announce cleared embeddings when the model changes", async function () {
it("should clear embeddings when the model changes", async function () {
let stubs = [
sinon.stub(Zotero.Embeddings.Indexing, 'startIndexing').resolves(),
sinon.stub(Zotero.Embeddings, 'pruneModels').resolves()
@ -1303,13 +1303,9 @@ describe("Zotero.Embeddings", function () {
+ "VALUES (?, ?, 1)",
[item.id, 'hash']
);
// The model switch clears the old vectors and announces the
// removals (after the coalescing delay), so active semantic
// views refresh
let promise = waitForNotifierEvent('refresh', 'item');
// The model switch clears the old vectors
Zotero.Prefs.set('embeddings.model', 'bge-small-en-v1.5');
let event = await promise;
assert.include(event.ids, item.id);
await Zotero.Embeddings.Indexing.waitForPendingModelSwitch();
assert.equal(
await Zotero.DB.valueQueryAsync(
"SELECT COUNT(*) FROM embeddings.itemEmbeddings"