{{ message }}
fix(search): reindex holders after property delete - #13173
Open
tiensonqin wants to merge 2 commits into
Open
Conversation
Deleting a property put its still-alive holder pages/blocks in the FTS remove set via db-before referrers, but not in the add set once the property eid was gone. Reindex surviving referrers from db-before so holders stay findable. Co-authored-by: Tienson Qin <tiensonqin@gmail.com>
create-conn-with-blocks does not run the worker refs pipeline, so the regression test now adds the same :block/refs links production uses. Co-authored-by: Tienson Qin <tiensonqin@gmail.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrow, aligns with the described failure mode, and includes a focused regression test that would fail under the prior behavior.
Pull request overview
This PR fixes a DB-graph search indexing regression where deleting a property could remove all pages/blocks that held that property from the FTS index (even though those entities still existed in DataScript), requiring a full reindex or manual edits to restore searchability.
Changes:
- Update
frontend.worker.search/get-blocks-from-datoms-implso referrers fromdb-beforeare also included in the “add” side, ensuring surviving holders get reindexed after the referenced entity (property) is deleted. - Add a targeted CLJS unit test covering property deletion via
outliner-page/build-page-retract-txand asserting holders are re-added while the deleted property stays removed from the index.
File summaries
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes logseq/db-test#1153 (transferred from logseq/logseq#13170).
Problem
On a DB graph, deleting a property removed every page/block that held that property from the FTS search index. Datascript still had the entities; only the worker
blocks/blocks_ftsrows were deleted. Rebuild search index (or editing a title) was required to bring them back.Cause
frontend.worker.search/get-blocks-from-datoms-implput holders intoremove-eidsviareferrer-eidsondb-before(they:block/refsthe property). After the property eid is gone,referrer-eidsondb-afteris empty, and the holders' own titles did not change, so they never reachedadd-eids. The listener then deleted their FTS rows and upserted nothing.Change
Include
db-beforereferrers on the add side. Surviving holders are reindexed; entities that no longer exist indb-afterare still dropped byentities-for/block->index, andsync-search-indicekeeps those in the remove set.Tests
frontend.worker.search-test/sync-search-indice-reindexes-holders-when-property-is-deleteddeletes a user property throughoutliner-page/build-page-retract-txand asserts::blocks-to-add(FTS reindex):blocks-to-remove-set(delete-then-upsert):blocks-to-remove-setand is not re-addedThe test seeds
:block/refsfrom holders to the property, matching the worker refs pipeline that production uses when a property value is set.Verified:
LOGSEQ_STABLE_IDENTS=1 node static/tests.js -n frontend.worker.search-test— 59 tests, 195 assertions, 0 failures.