Warn when an encrypted search runs before the index has finished building#34001
Conversation
…ding When a search runs in an encrypted room while the local Seshat index is still crawling not-yet-indexed history, results can silently come back partial. SearchWarning now subscribes to the event index's changedCheckpoint progress and shows a polite (role=status) notice while the crawl is in progress, clearing automatically the moment indexing finishes.
|
Please include a screenshot of the warning for design to review |
|
|
||
| export default function SearchWarning({ isRoomEncrypted, kind, showLogo = true }: IProps): JSX.Element { | ||
| const eventIndex = EventIndexPeg.get(); | ||
| const crawlInProgress = useIsCrawlInProgress(eventIndex); |
There was a problem hiding this comment.
Shouldn't this only care if the current room is being indexed/yet to be indexed if the search kind is room rather than all?
There was a problem hiding this comment.
You're right, and it's a live false positive rather than a hypothetical.
useIsCrawlInProgress keyed off index.currentRoom() !== null, which is global: currentRoom() returns null only when currentCheckpoint === null and the whole checkpoint queue is empty. So searching a fully-crawled room showed "results may be incomplete" purely because the crawler happened to be grinding through an unrelated room — for the entire initial crawl. That's cry-wolf on a banner whose only job is to be believed, and it's at odds with #32253 itself, which is framed around "the conversation I am looking at".
One note on the props, since it changes the shape of the fix: the kind this is anchored to is WarningKind { Files, Search } — files-panel vs search-panel, not room-vs-all. The room/all dimension is SearchScope, which wasn't plumbed into this component. So it needed a new prop rather than a re-read of kind; the two are orthogonal and I've kept both.
Now pushed. RoomSearchAuxPanel already computed scope twenty lines above the render and already had searchInfo.roomId, so it's two optional props each side — no changes to RoomView, SearchInfo, or FilePanel. SearchScope.Room → crawlingRooms.has(roomId), otherwise crawlingRooms.size > 0.
Three things worth flagging:
I moved the all-rooms branch onto crawlingRooms() too, rather than leaving it on currentRoom(). That's slightly beyond what you asked. The reason: currentRoom() returns client.getRoom(roomId), which is null if the js-sdk doesn't know the head-of-queue room (e.g. a checkpoint persisted for a room you've since left) — so it can also fail to warn while your room sits queued behind it. crawlingRooms() works on raw room ids and is immune. Happy to revert that half if you'd rather keep the All path untouched.
Seshat can't actually tell us whether a room is fully indexed. isRoomIndexed() looks like the right call but is presence-only — "the index contains events for the given room", i.e. ≥1 event. So "has an outstanding checkpoint" via crawlingRooms() is the closest honest signal, and absence from that set isn't proof of completeness (no back-pagination token, checkpoint dropped on a 403, or pre-seeding all look identical to "done"). That means this narrows false positives without fixing under-reporting. The blind spot exists today too, so I've documented it on the hook rather than papered over it — but it's why I've softened Fixes #32253 to Part of: the issue's other ask (prioritise the open conversation for indexing) is a crawler-ordering change I haven't attempted.
changedCheckpoint only carries currentRoom(), so the payload can't answer a per-room question — the handler ignores the argument and re-reads the set, same as ManageEventIndexDialog. There's a test pinning exactly that (payload names a different room, searched room still queued → still warns).
No string or markup change, so the design sign-off should still hold.
The warning was driven by `currentRoom() !== null`, which is a global signal: it is non-null while the crawler has any outstanding checkpoint for any room. Searching a fully-crawled room while an unrelated room was still being crawled therefore claimed the results may be incomplete when they were not. Drive it from `crawlingRooms()` instead, which reports the rooms with outstanding checkpoints by id, and pass the search scope and room id in from RoomSearchAuxPanel: a room-scoped search now asks only about that room, while an all-rooms search still reacts to any outstanding checkpoint. Using room ids throughout also avoids `currentRoom()` returning null, and so under-reporting a crawl, when the js-sdk does not know the room at the head of the queue. The `changedCheckpoint` payload only carries the globally-current room and cannot answer a per-room question, so the handler re-reads the checkpoint set.
ff36308 to
dc321c5
Compare
| function useIsCrawlInProgress(index: EventIndex | null, scope?: SearchScope, roomId?: string): boolean { | ||
| const readCrawlInProgress = useCallback((): boolean => { | ||
| if (!index) return false; | ||
| const { crawlingRooms } = index.crawlingRooms(); |
There was a problem hiding this comment.
At least based on the jsdoc of this method it doesn't sound like it will be all uncrawled rooms, just rooms which are currently being crawled, no? Maybe combining the check with isRoomIndexed would be better. I.e room is not in crawlingRooms && isRoomIndexed(room)
There was a problem hiding this comment.
though then also worth updating the callback name
There was a problem hiding this comment.
On the first half, though, crawlingRooms() is broader than its jsdoc suggests, and I think the doc is what misled you. It adds every queued checkpoint, not just the one in flight:
this.crawlerCheckpoints.forEach((checkpoint) => {
crawlingRooms.add(checkpoint.roomId);
});
if (this.currentCheckpoint !== null) {
crawlingRooms.add(this.currentCheckpoint.roomId);
}crawlerCheckpoints is the pending queue, so a room sitting fifth in line is already in the set — it does cover "yet to be indexed", just not under that name. The inline doc on the return type says "The rooms that we are currently crawling", which reads as current-only. I've reworded it to "the rooms with an outstanding crawler checkpoint: the one being crawled right now, and those still queued behind it" — one line in EventIndex.ts, since it caused the misread and would cause the next one. Happy to drop it if you'd rather keep this PR to two files.
Your suggestion still closes a real gap, so I've taken it. What the crawl set genuinely can't see is a room with no checkpoint at all — before addInitialCheckpoints() seeds them, such a room is absent from the set and looks identical to "done". That's the issue's own repro ("log into a new desktop session"), which the previous version would have sat silent through. So room-scoped is now has(roomId) || !isRoomIndexed(roomId), i.e. your not in crawlingRooms && isRoomIndexed(room) inverted.
One wrinkle I hit implementing it, which changed the shape slightly. isRoomIndexed has no change notification: changedCheckpoint is the only event EventIndex emits, and it fires solely on checkpoint transitions inside crawlerFunc — adding events to the index emits nothing, and an idle crawler is silent (crawlerCheckpoints.shift() → undefined → idle = true; continue, no emit). So a warning raised purely from !isRoomIndexed once the crawler has drained would never be re-evaluated and would stick for the session.
So I only ask the isRoomIndexed question while the crawler still has work outstanding:
if (relevant || !anyOutstanding || scope !== SearchScope.Room || roomId === undefined) return;
const indexed = await index.isRoomIndexed(roomId);That keeps the case you're pointing at (during the initial crawl, my room isn't seeded yet → warn) and drops the stuck one (crawler idle → nothing more is coming, and claiming the index is "still being built" would be untrue). It also self-clears: the crawler emits when it nulls currentCheckpoint on drain, so we get a final event to re-evaluate on. There's a test pinning it — removing the !anyOutstanding guard turns it red.
Three smaller notes:
isRoomIndexedis presence-only, so it can't be a drop-in&&. Its contract is "the index contains events for the given room" — ≥1 event, not complete — so it's a floor ("definitely not started"), not a completeness check. I've documented it as that rather than implying more.- It returns
boolean | undefined(indexManager?.isRoomIndexed(...));undefinedmeans there's no manager to ask, which isn't evidence either way. So it warns on=== false, not!indexed, with a test for it. - All-rooms stays on the checkpoint set — per-room
isRoomIndexedacross every encrypted room would be an IPC round-trip each, perchangedCheckpoint.
Renamed as you suggested: useIsCrawlInProgress → useIsIndexIncomplete. The old name described the old, narrower question. The checkpoint half stays synchronous and seeds the initial state, so the common "actively crawling" case renders without a flash; only the never-indexed refinement awaits, and the state is re-seeded from the checkpoint set on every scope/room change so a previous room's answer can't sit on screen while the lookup is in flight (also tested). A generation counter drops any response the scope has moved on from.
The crawl set cannot see a room that has no checkpoint: before the initial checkpoints have been seeded, such a room is absent from it and looks identical to one that has been fully crawled. That is the case issue element-hq#32253 describes, so ask isRoomIndexed() as well, and warn when the index holds no events for the room being searched. Only ask it while the crawler still has work outstanding. That is what the warning claims, and the index has no event for its contents changing -- changedCheckpoint fires on checkpoint transitions only, and an idle crawler is silent -- so a warning raised once the crawler had drained would never be re-evaluated and would stick. Rename the hook to useIsIndexIncomplete, as it no longer answers the narrower question of whether a crawl is in progress. Re-seed it from the checkpoint set on each scope or room change so that the previous search's answer is not left on screen while the lookup is in flight, but not on each checkpoint change, which would blink an already-earned warning off and on again. Also reword the crawlingRooms() doc, which described the set as the rooms being crawled when it holds every queued checkpoint too.

Problem
When a search runs in an encrypted room while the local Seshat index is still being
built (crawling not-yet-indexed history), the search can silently return partial
results, with nothing to tell the user that more matches may appear once indexing
finishes (#32253).
Fix
SearchWarningnow observes the event index's crawl progress:useIsIndexIncompletehook subscribes to the index'schangedCheckpointevent andre-reads the index to see whether the room being searched still has history left to crawl,
or has not been indexed at all.
(
WarningKind.Search), a polite live-region notice — "Results may be incompletebecause your search index is still being built." — is rendered with
role="status",so a screen reader announces it if it appears while a search panel is already open.
every checkpoint change), and the listener is removed on unmount.
No behaviour changes for unencrypted rooms, for the no-index / index-error paths, or for
the files warning.
Tests
SearchWarning-test.tsxcovers: no warning when the index is complete; the partial-resultswarning while the searched room is crawling; no warning for a room-scoped search when only an
unrelated room is crawling; the all-rooms search still warning on any outstanding checkpoint;
auto-clear when the searched room's checkpoint drains; the warning surviving a
changedCheckpointthat names a different room while the searched room is still queued; the fallback to the global
check when the room id is not yet known; listener cleanup; and that the notice is scoped to search
rather than the files warning.
Scope
The warning is scoped to the room being searched. A room-scoped search asks whether that room
still has an outstanding crawler checkpoint (
EventIndex.crawlingRooms(), which covers thecheckpoint being crawled and those queued behind it), or whether the index holds no events for it
at all (
isRoomIndexed()) — the latter being how a room looks before its checkpoint has beenseeded, which is the case in this issue's repro and which the checkpoint set alone cannot see. An
all-rooms search reacts to any outstanding checkpoint.
isRoomIndexed()is only consulted while the crawler still has work outstanding: that is what thewarning claims, and the index emits no event when its contents change (
changedCheckpointfireson checkpoint transitions only, and an idle crawler is silent), so a warning raised after the
crawler had drained would never be re-evaluated.
Note that neither signal proves completeness —
isRoomIndexed()reports only that the index holdssome events for a room — so this under-warns rather than over-warns. This is documented on the
hook.
Part of #32253. That issue also asks that the open conversation be prioritised for indexing,
which is a change to the crawler's checkpoint ordering and is not attempted here.
Checklist
public/exportedsymbols have accurate TSDoc documentation.