Skip to content
This repository was archived by the owner on Mar 25, 2026. It is now read-only.

Add Tokenizer Mode Support for Seshat Search Index#2753

Closed
shinaoka wants to merge 8 commits into
element-hq:developfrom
shinaoka:shinaoka/ngram2
Closed

Add Tokenizer Mode Support for Seshat Search Index#2753
shinaoka wants to merge 8 commits into
element-hq:developfrom
shinaoka:shinaoka/ngram2

Conversation

@shinaoka

@shinaoka shinaoka commented Dec 14, 2025

Copy link
Copy Markdown

element-meta Issue #2883: i18n support for full-text search in encrypted rooms

Summary

This PR adds tokenizer mode support to element-desktop, enabling users to choose between standard (language-based) and N-gram tokenization for the local message search index.

Changes

  • Add tokenizerMode option to EventIndex initialization
  • Accept tokenizerMode from element-web via IPC
  • Support both N-gram and language-based tokenization modes
  • Default to language-based tokenization for backward compatibility

Files Changed

  • src/@types/matrix-seshat.d.ts - Add TypeScript types for tokenizer options
  • src/seshat.ts - Pass tokenizerMode to Seshat, handle IPC from element-web

Dependencies

⚠️ This PR depends on:

Seshat PR must be merged and a new npm package version released before this PR can be merged.

After Seshat is released, update package.json to use the new matrix-seshat version (e.g., ^4.1.0).

Merge Order

1. Seshat PR #150  →  npm release (e.g., matrix-seshat@4.1.0)
2. element-desktop (this PR)  →  update matrix-seshat dependency
3. element-web  →  UI for tokenizer mode selection

Checklist

  • Ensure your code works with manual testing.
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

@shinaoka
shinaoka requested a review from a team as a code owner December 14, 2025 23:11
@github-actions github-actions Bot added the Z-Community-PR Issue is solved by a community member's PR label Dec 14, 2025
@shinaoka
shinaoka requested a review from a team as a code owner December 14, 2025 23:21
@dbkr

dbkr commented Dec 15, 2025

Copy link
Copy Markdown
Member

This needs the other two PRs to land first (which will need a look from product).

Comment thread src/seshat-config.ts Outdated
@@ -0,0 +1,29 @@
/*
Copyright 2025 New Vector Ltd.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright should be you

@shinaoka shinaoka Mar 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I interpreted your comment as asking for my name in the copyright header. I kept “New Vector Ltd.” because that seems to be the existing convention in this repo (and CONTRIBUTING.md points to the CLA). If you want individual author names on new files instead, I can update them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's New Vector Ltd on most of the files because most of it was written by New Vector employees, but since this is your work, you own the copyright.

Comment thread src/seshat.ts Outdated
Comment on lines +141 to +151
// Schema mismatch or other errors - delete and recreate the database
console.warn("Failed to open Seshat database, deleting and recreating:", e);
await deleteContents(eventStorePath);
try {
eventIndex = new Seshat(eventStorePath, { passphrase, ...seshatConfig });
// Return that the database was recreated so element-web can re-add checkpoints
ret = { wasRecreated: true };
} catch (e2) {
sendError(payload.id, <Error>e2);
return;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand why this change is relevant to this PR or why it's better

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is needed because changing the n-gram/tokenizer configuration makes the existing Seshat database incompatible. With this fallback, re-indexing will rebuild the index.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay. I was going to suggest putting this in a comment but the code seems to be gone now?

Comment thread test/unit-tests/seshat.test.ts Outdated
@@ -0,0 +1,40 @@
/*
Copyright 2025 New Vector Ltd.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright here too

Comment thread src/seshat-config.ts
Comment thread jest.config.cjs Outdated
},
],
},
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we're really trying to move away from jest so it would be much better to use vitest if we're introducing unit tests to the desktop package.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed this issue.

- Add tokenizerMode option to EventIndex initialization
- Accept tokenizerMode from element-web via IPC
- Support both N-gram and language-based tokenization modes
- Default to language-based tokenization for backward compatibility
@dbkr

dbkr commented Mar 2, 2026

Copy link
Copy Markdown
Member

Also if you could please avoid force pushing: it makes it very hard to review a PR if we can't see what is changing.

@dbkr

dbkr commented Mar 5, 2026

Copy link
Copy Markdown
Member

What's the status of this PR? It looks like you removed the wasRecreated return value that was added but it's still in element-hq/element-web#31540. The tests also look to have been removed completely and the copyright still needs fixing. That said, I'm also not sure what has changed as you force pushed so the history is now gone.

@shinaoka

shinaoka commented Mar 7, 2026

Copy link
Copy Markdown
Author
  • I restored the wasRecreated return path in desktop so it matches the flow expected by element-web PR 31540.
  • I fixed the copyright header in src/seshat-config.ts.
  • Dependency note: desktop uses hakDependencies with matrix-seshat: ^4.0.1, resolved via hak.
  • matrix-org/seshat#157 is still open, and npm matrix-seshat@latest is still 4.0.1.

So this desktop PR is backward-compatible and safe to merge now; tokenizerMode=ngram will become effective once a newer matrix-seshat release is available.

One question on tests, so I can align with your preference:
Would you like me to add a minimal Vitest unit test for createSeshatConfig, or cover this at Playwright level instead?

I’ll add follow-up commits only (no force-push).

@dbkr

dbkr commented Mar 12, 2026

Copy link
Copy Markdown
Member

Thanks for doing that. If it is possible to add a minimal vitest test then that would be ideal - since this previously added a jest test, I assume this shouldn't be too problematic (hopefully?). Much appreciated!

Comment thread test/unit-tests/seshat-index.test.ts Fixed
Comment thread test/unit-tests/seshat-index.test.ts Fixed
Comment thread test/unit-tests/seshat-index.test.ts Fixed
Comment thread test/unit-tests/seshat-index.test.ts Fixed
@shinaoka

Copy link
Copy Markdown
Author

Added minimal Vitest coverage.

src/seshat.ts changed only to extract the initEventIndex logic into a helper so the tokenizer config path can be unit-tested directly; behavior should be unchanged.

The pnpm-lock.yaml diff is just the result of adding vitest to the dependencies, not a manual change.

@dbkr dbkr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this, great to have vitests for desktop too.

Comment thread test/unit-tests/seshat-index.test.ts Outdated
@t3chguy
t3chguy requested a review from dbkr March 20, 2026 15:23
@t3chguy

t3chguy commented Mar 25, 2026

Copy link
Copy Markdown
Member

Sorry this repo has been merged into element-web so you will need to merge your changes into element-hq/element-web#31540 apps/desktop

@t3chguy t3chguy closed this Mar 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

T-Enhancement Z-Community-PR Issue is solved by a community member's PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants