Add tokenizer mode support for message search#33048
Conversation
Add user-facing setting to choose tokenizer mode (language-based vs N-gram) for local message search. N-gram tokenization supports CJK languages and mixed-language content. Includes confirmation dialog for mode changes (requires reindex) and automatic checkpoint recovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add N-gram tokenizer support to Seshat search index initialization. Refactors initEventIndex into a testable module with dependency injection, supporting both language-based and N-gram tokenization modes. Includes automatic database recreation fallback and unit tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, any Seshat open failure that wasn't a ReindexError would silently delete the EventStore and recreate it, losing the user's search index on transient errors like filesystem locks or passphrase issues. Restore the original behavior: only handle ReindexError with recovery logic, and propagate all other errors to the caller (which shows an error to the user via sendError). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Seshat has released v4.2.0. Perhaps now the dependency can be updated, and this pull request can proceed to be reviewed by the team. Great appreciation from a CJK language user. |
…er-mode # Conflicts: # apps/desktop/package.json # pnpm-lock.yaml
|
@Half-Shot @MidhunSureshR How can I help you? |
Sorry, this is entirely blocked on us finding time to review. Thanks for the patience 🙏 |
Half-Shot
left a comment
There was a problem hiding this comment.
Sorry for the lateness on this review. I've asked the crypto team to lend a hand with the more technical aspect of the review, but given some pointers on the web side.
| * Create Seshat configuration based on tokenizer mode. | ||
| * | ||
| * @param tokenizerMode - The tokenizer mode: "ngram" for N-gram tokenization (CJK languages), | ||
| * or "language" for standard language-based tokenization. | ||
| * @returns Configuration object for Seshat initialization. | ||
| */ |
There was a problem hiding this comment.
Your docstring has jumped up a few lines!
There was a problem hiding this comment.
Moved the docstring back onto createSeshatConfig so it no longer documents the enum.
| Language = "language", | ||
| } | ||
|
|
||
| export function createSeshatConfig(tokenizerMode?: string): { |
There was a problem hiding this comment.
Should this throw if an unknown tokenizerMode is given?
There was a problem hiding this comment.
Updated createSeshatConfig to default only when no tokenizerMode is provided, and to throw for unknown values. Added coverage for that case.
| value={this.state.crawlerSleepTime.toString()} | ||
| onChange={this.onCrawlerSleepTimeChange} | ||
| /> | ||
| <Field |
There was a problem hiding this comment.
So I think this bit of the UI is a bit clunky and could do with a refactor, how about using a Compound option component https://compound.element.io/?path=/docs/compound-web_form-controls-radio--docs instead which means:
- You can separate the labels and descriptions (so you don't need the big block of text below)
- Using Compound means it'll fit the rest of our app.
There was a problem hiding this comment.
Refactored the tokenizer mode selector to Compound radio controls, added the tokenizer mode heading, and split the labels/descriptions so the explanatory text is attached to each option.
| "strict_encryption": "Only send messages to verified users", | ||
| "tokenizer_mode": "Search tokenizer mode", | ||
| "tokenizer_mode_change_warning": "Changing the tokenizer mode requires deleting the existing search index. The index will be rebuilt automatically. Do you want to continue?", | ||
| "tokenizer_mode_description": "Language-based: Single language support fixed to UI language. Only works with some languages (English, German, etc.). N-gram: Supports all languages including those without word boundaries. Works with mixed languages. Changing this setting will rebuild your search index.", |
There was a problem hiding this comment.
You probably don't need "Changing this setting will rebuild your search index" if we have a warning too.
There was a problem hiding this comment.
Removed the rebuild note from the option description. The destructive/rebuild behavior is now only in the confirmation warning.
| public async initEventIndex( | ||
| userId: string, | ||
| deviceId: string, | ||
| tokenizerMode?: string, |
There was a problem hiding this comment.
This should probably use the enum, or at the very least:
| tokenizerMode?: string, | |
| tokenizerMode?: "ngram"|"language", |
There was a problem hiding this comment.
Narrowed tokenizerMode through the web indexing manager API using a TokenizerMode enum.
|
|
||
| if (!SettingsStore.getValueAt(SettingLevel.DEVICE, "enableEventIndexing")) { | ||
| const enableEventIndexing = SettingsStore.getValueAt(SettingLevel.DEVICE, "enableEventIndexing"); | ||
| if (!enableEventIndexing) { |
There was a problem hiding this comment.
This seems like an unrelated change?
There was a problem hiding this comment.
This change is needed to pass the selected tokenizer mode from device settings into the platform index manager. I kept it and narrowed the type so the intent is clearer.
| public async initEventIndex( | ||
| userId: string, | ||
| deviceId: string, | ||
| tokenizerMode?: string, |
There was a problem hiding this comment.
Also could do with a more narrow string type?
There was a problem hiding this comment.
Narrowed this to the same TokenizerMode enum used by the base index manager API.
|
|
||
| render(<ConfirmTokenizerChangeDialog onFinished={onFinished} />); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: /ok/i })); |
There was a problem hiding this comment.
Anything wrong with just "ok"? And I think this needs awaiting?
There was a problem hiding this comment.
Updated the test to click the exact "OK" button text and await userEvent.click.
|
|
||
| import BaseEventIndexManager from "../../../src/indexing/BaseEventIndexManager"; | ||
|
|
||
| describe("BaseEventIndexManager", () => { |
There was a problem hiding this comment.
I'm not sure this needs to even be tested to be honest
There was a problem hiding this comment.
Removed this test. It only asserted the base unimplemented method behavior and did not add useful coverage for tokenizer mode.
|
|
||
| it("passes tokenizerMode to initEventIndex IPC call", async () => { | ||
| // IPCManager requires window.electron to exist. | ||
| window.electron = { |
There was a problem hiding this comment.
I agree with SonarCloud :)
| window.electron = { | |
| globalThis.electron = { |
There was a problem hiding this comment.
Updated the electron stub to use globalThis with a typed cast.
|
@shinaoka Are you able to continue with this PR? |
|
Oh, I am sorry. I just missed your messages. I will work on them. |
…nderer crash auto-recovery (session 14-15, element-hq#33048/element-hq#32038, element-hq#31996, element-hq#32222)
|
Addressed the review comments in b3df964:
Verification run locally:
|
|
Any progress? @Half-Shot |



Summary
This PR ports the tokenizer-mode work to the monorepo layout and combines the previous web and desktop-side changes into a single branch.
It adds a user-facing tokenizer mode setting for local message search, passes the selected mode through the desktop layer to Seshat, and asks for confirmation before reindexing when the mode changes.
Related
Merge Status
matrix-org/seshat#150is merged, butmatrix-org/seshat#157is still open andmatrix-seshat@4.1.0has not been released yet.This PR should stay draft for now. Once
matrix-seshat@4.1.0is released, the dependency can be updated and this PR should be ready to merge.Testing
pnpm --dir apps/desktop test:unitpnpm --dir apps/web test -- --runTestsByPath test/unit-tests/async-components/dialogs/eventindex/ManageEventIndexDialog-test.tsx