feat: lazily page broad search buckets#108
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Summary by CodeRabbit
WalkthroughThis PR adds bucketed search pagination for archive search. Bucket cursors now encode versioned bucket and per-bucket keys. Sequence Diagram(s)sequenceDiagram
participant ArchiveView
participant SearchCache
participant SearchIndex
participant SessionDB
ArchiveView->>SearchCache: decodeBucketSearchSessionCursor(cursor)
ArchiveView->>SearchCache: readSearchSessionMetadataForCursor(sessionId)
SearchCache->>SessionDB: touch session and read metadata
SessionDB-->>SearchCache: descriptor
ArchiveView->>SearchIndex: querySearchIndex(textAfter with rank)
SearchIndex-->>ArchiveView: ranked text hits
ArchiveView->>SearchCache: readSearchSessionObjectBucketPage / readSearchSessionChunkBucketPage
SearchCache-->>ArchiveView: bucket page + next cursor
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/archive/query/archive-view.test.ts (1)
1473-1486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssertion relies on multi-page results for
"Source".
expect(secondPage.nextCursor).not.toBe(firstPage.nextCursor)only meaningfully verifies advancement when the query yields more than one page. If the fixture ever produces a single page, both cursors becomenulland this passes for the wrong reason (or fails inconsistently). Consider also assertingfirstPage.nextCursoris non-null to lock in the intended multi-page path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/archive/query/archive-view.test.ts` around lines 1473 - 1486, The pagination assertion in archive-view test for findArchiveObjects should explicitly lock in the multi-page path for "Source". Update the test around firstPage and secondPage so it asserts firstPage.nextCursor is non-null before using it, and keep the secondPage cursor comparison only after that check. This ensures the test is validating cursor advancement in the intended multi-page scenario rather than passing when both cursors are null.src/archive/query/archive-view.ts (1)
1280-1303: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
populateObjectBucketCachesrecomputes the full structured search on every bucket‑1 page.
readObjectBucketPagecallspopulateObjectBucketCachesunconditionally, which re-runsmentions.listBySurfaceTerms, a full-indexquerySearchIndex({ types: null, objectHitLimit: SEARCH_INDEX_FTS_HIT_LIMIT }),findEntities, andfindTriples, then re-populates the cache — for every page request that lands in bucket 1. The keyset cache the population fills is meant to avoid exactly this recomputation, so paging deep into the object bucket repeatedly pays the full search cost.Consider populating once per session (guard on whether the session's object caches are already populated) and skipping repopulation when resuming from an
aftercursor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/archive/query/archive-view.ts` around lines 1280 - 1303, `readObjectBucketPage` is always calling `populateObjectBucketCaches`, which forces the full structured search work to rerun on every bucket-1 page. Update the object-bucket paging flow so `populateObjectBucketCaches` is only invoked once per session when the cache is missing, and skip repopulating when continuing from an `after` cursor. Use the existing `session` state in `readObjectBucketPage`/`populateObjectBucketCaches` to detect whether the object caches are already populated before recomputing and repopulating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/archive/query/search-cache.ts`:
- Around line 372-403: `readSearchSessionObjectBucketPage` is skipping triple
rows whenever `entityRows.length > limit`, which can drop higher-ranked triples
and desync paging. Update the object-bucket pagination flow to fetch both entity
and triple buckets for the page window, or maintain separate keyset state per
bucket so neither side is excluded early. Also align `compareObjectBucketHits`
with the SQL ordering keys used by `readSearchSessionEntityBucketRows` and
`readSearchSessionTripleBucketRows` so the `after` cursor is derived from the
same tie-break order as the database.
In `@src/archive/search-index/search-index.ts`:
- Around line 436-500: `querySearchIndex` is letting duplicate sentence hits
overwrite each other in the result `Map`, which makes the stored text `rank`
depend on tier execution order and breaks pagination in `readTextBucketPage`.
Update the deduping logic in `querySearchIndex` so a sentence keeps a stable
rank regardless of later tiers (for example, preserve the first rank seen or
otherwise merge deterministically instead of blindly using `Map.set(...)`). Make
sure the value consumed by `textAfter` and `isAfterTextKey` stays consistent
across pages.
---
Nitpick comments:
In `@src/archive/query/archive-view.ts`:
- Around line 1280-1303: `readObjectBucketPage` is always calling
`populateObjectBucketCaches`, which forces the full structured search work to
rerun on every bucket-1 page. Update the object-bucket paging flow so
`populateObjectBucketCaches` is only invoked once per session when the cache is
missing, and skip repopulating when continuing from an `after` cursor. Use the
existing `session` state in `readObjectBucketPage`/`populateObjectBucketCaches`
to detect whether the object caches are already populated before recomputing and
repopulating.
In `@test/archive/query/archive-view.test.ts`:
- Around line 1473-1486: The pagination assertion in archive-view test for
findArchiveObjects should explicitly lock in the multi-page path for "Source".
Update the test around firstPage and secondPage so it asserts
firstPage.nextCursor is non-null before using it, and keep the secondPage cursor
comparison only after that check. This ensures the test is validating cursor
advancement in the intended multi-page scenario rather than passing when both
cursors are null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65a92f43-bd89-41aa-aed3-724501872860
📒 Files selected for processing (4)
src/archive/query/archive-view.tssrc/archive/query/search-cache.tssrc/archive/search-index/search-index.tstest/archive/query/archive-view.test.ts
Moskize91
force-pushed
the
codex/lazy-search-bucket-cursors
branch
from
979c93b to
daccee5
Compare
1 participant
Summary
Verification
wg 'wikg:///Users/taozeyu/Downloads/三国演义.wikg' --query '桃园三结义' --jsonreturned in ~2s withsearch_results = 0.Cache note
If local search state behaves oddly after this change, delete the local cache directory and rerun the query:
rm -rf ~/.wikigraph/cacheThe cache is rebuilt automatically.