fix: preserve embeddingFormatVersion across store hydration (Fixes #234)#236
Conversation
Fixes #234. The incremental-index button was reindexing from the beginning because normalizePersistedState reconstructed vectorSearchConfig from a fixed allow-list that dropped embeddingFormatVersion (and searchThreshold/searchTopK/enableHyDE/enableReranking). After reload the app treated saved vectors as a legacy format, so indexAllRepos intentionally reindexed every indexable repo. - Centralize defaults in defaultVectorSearchConfig and a single normalizeVectorSearchConfig used by both hydration and migrate. - Persist + clamp embeddingFormatVersion to [1, EMBEDDING_FORMAT_VERSION] so corrupted/future values cannot suppress required reindexing. - Clamp searchThreshold to [0,1] and searchTopK to [5,50] to match the settings UI bounds before reaching the worker. - Add regression tests for store hydration and indexAllRepos incremental filtering with current vs old format version. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCentralizes vector search config normalization in the store, updates incremental indexing and search UI wiring to use the current embedding format version, and adds tests for persisted config normalization and incremental repo selection. ChangesVector search config and indexing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust normalization and migration logic for the vector search configuration in the application store, alongside comprehensive unit tests for both the store and the vector search service. The feedback identifies a critical issue where the default embedding format version is hardcoded to 1 instead of the latest version, which would cause new users to experience unnecessary full reindexing. Additionally, the normalization logic should explicitly fallback to version 1 when the field is missing in an existing configuration to ensure legacy users are correctly migrated.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const defaultVectorSearchConfig: VectorSearchConfig = { | ||
| enabled: false, | ||
| workerUrl: '', | ||
| authToken: '', | ||
| embeddingConfigId: '', | ||
| indexMode: 'readme', | ||
| readmeMaxChars: 6000, | ||
| searchThreshold: 0.35, | ||
| searchTopK: 30, | ||
| enableHyDE: true, | ||
| enableReranking: true, | ||
| embeddingFormatVersion: 1, | ||
| }; |
There was a problem hiding this comment.
In defaultVectorSearchConfig, embeddingFormatVersion is hardcoded to 1. This means a brand new user (or a user resetting their configuration) will start with version 1 instead of the latest EMBEDDING_FORMAT_VERSION (which is 2).
When they perform their first index, they will be treated as having a legacy format version, which triggers a full reindex (though harmless since they have no indexed repos, it is conceptually incorrect). More importantly, if there is no code that automatically updates their configuration to EMBEDDING_FORMAT_VERSION after indexing, they will remain stuck on version 1 forever, causing every subsequent incremental index to reindex all repositories.
To fix this, defaultVectorSearchConfig should default to EMBEDDING_FORMAT_VERSION. For backward compatibility, when normalizing an existing configuration that is missing the version field, we should explicitly fallback to 1 to trigger a reindex for those legacy users.
| const defaultVectorSearchConfig: VectorSearchConfig = { | |
| enabled: false, | |
| workerUrl: '', | |
| authToken: '', | |
| embeddingConfigId: '', | |
| indexMode: 'readme', | |
| readmeMaxChars: 6000, | |
| searchThreshold: 0.35, | |
| searchTopK: 30, | |
| enableHyDE: true, | |
| enableReranking: true, | |
| embeddingFormatVersion: 1, | |
| }; | |
| const defaultVectorSearchConfig: VectorSearchConfig = { | |
| enabled: false, | |
| workerUrl: '', | |
| authToken: '', | |
| embeddingConfigId: '', | |
| indexMode: 'readme', | |
| readmeMaxChars: 6000, | |
| searchThreshold: 0.35, | |
| searchTopK: 30, | |
| enableHyDE: true, | |
| enableReranking: true, | |
| embeddingFormatVersion: EMBEDDING_FORMAT_VERSION, | |
| }; |
There was a problem hiding this comment.
Pushed in cd61f40. defaultVectorSearchConfig.embeddingFormatVersion is now EMBEDDING_FORMAT_VERSION (2), so fresh/reset configs are treated as current — no spurious full reindex on first incremental run.
| const embeddingFormatVersion = typeof config.embeddingFormatVersion === 'number' | ||
| && Number.isInteger(config.embeddingFormatVersion) | ||
| && config.embeddingFormatVersion >= 1 | ||
| && config.embeddingFormatVersion <= EMBEDDING_FORMAT_VERSION | ||
| ? config.embeddingFormatVersion | ||
| : defaultVectorSearchConfig.embeddingFormatVersion; |
There was a problem hiding this comment.
Since defaultVectorSearchConfig.embeddingFormatVersion is updated to EMBEDDING_FORMAT_VERSION (which is 2), we must explicitly fallback to 1 when normalizing an existing configuration that is missing embeddingFormatVersion. This ensures that existing legacy users who do not have this field in their persisted state are correctly identified as having version 1 and get their repositories reindexed.
| const embeddingFormatVersion = typeof config.embeddingFormatVersion === 'number' | |
| && Number.isInteger(config.embeddingFormatVersion) | |
| && config.embeddingFormatVersion >= 1 | |
| && config.embeddingFormatVersion <= EMBEDDING_FORMAT_VERSION | |
| ? config.embeddingFormatVersion | |
| : defaultVectorSearchConfig.embeddingFormatVersion; | |
| const embeddingFormatVersion = typeof config.embeddingFormatVersion === 'number' | |
| && Number.isInteger(config.embeddingFormatVersion) | |
| && config.embeddingFormatVersion >= 1 | |
| && config.embeddingFormatVersion <= EMBEDDING_FORMAT_VERSION | |
| ? config.embeddingFormatVersion | |
| : 1; |
There was a problem hiding this comment.
Pushed in cd61f40. Implemented exactly this split: existing persisted configs whose is missing/out-of-range now fall back to a dedicated (not the fresh-config default), preserving the one-time reindex safety for users migrating from older storage formats. Added a regression test ( → reindexes all indexable repos) to lock the #234 path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/vectorSearchService.test.ts (1)
104-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTraced logic matches production behavior — tests are correct.
Verified against
indexAllRepos's incremental filtering logic (pre-filter onanalyzed_at/analysis_failed, thenformatVersionChanged/vector_indexed_at/ content-time comparisons): both scenarios produce the expectedindexedIds/indexedRepoIdsand call counts.One optional note: the two tests share an identical
reposfixture and structure, differing only informatVersionand expected output. Consider extracting the shared fixture (or usingtest.each) to reduce duplication if more incremental-filtering scenarios are added later (e.g. missingformatVersiondefaulting to legacy v1, which is the actual regression path from#234).♻️ Optional: shared fixture / parameterized test
+const incrementalFilteringRepos = () => [ + makeRepository(1), + makeRepository(2, { vector_indexed_at: undefined }), + makeRepository(3, { + vector_indexed_at: '2026-01-05T00:00:00.000Z', + last_edited: '2026-01-06T00:00:00.000Z', + }), + makeRepository(4, { analyzed_at: undefined, vector_indexed_at: undefined }), + makeRepository(5, { analysis_failed: true, vector_indexed_at: undefined }), +]; + describe('indexAllRepos incremental filtering', () => { - it('skips already indexed unchanged repos when format version is current', async () => { + it.each([ + { formatVersion: EMBEDDING_FORMAT_VERSION, expected: [2, 3] }, + { formatVersion: EMBEDDING_FORMAT_VERSION - 1, expected: [1, 2, 3] }, + ])('filters repos correctly for formatVersion=$formatVersion', async ({ formatVersion, expected }) => { const client = makeIndexClient(); const vectorService = makeVectorService(); - const repos = [ /* ... */ ]; + const repos = incrementalFilteringRepos(); const indexedIds: number[] = []; const result = await indexAllRepos(repos, client, vectorService, { incremental: true, - formatVersion: EMBEDDING_FORMAT_VERSION, + formatVersion, currentFormatVersion: EMBEDDING_FORMAT_VERSION, indexMode: 'description', onRepoIndexed: (repoId) => indexedIds.push(repoId), }); - expect(indexedIds).toEqual([2, 3]); + expect(indexedIds).toEqual(expected); + expect(result.indexedRepoIds).toEqual(expected); + expect(result.indexed).toBe(expected.length); }); });🤖 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/services/vectorSearchService.test.ts` around lines 104 - 165, The incremental filtering tests in `indexAllRepos` are correct, but the duplicated `repos` fixture and test structure should be cleaned up for maintainability. Extract the shared repository fixture (or switch `describe('indexAllRepos incremental filtering')` to `test.each`) so the two cases only vary by `formatVersion` and expected results, keeping the assertions against `indexAllRepos`, `client.embed`, `vectorService.upsert`, and `onRepoIndexed` unchanged.
🤖 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.
Nitpick comments:
In `@src/services/vectorSearchService.test.ts`:
- Around line 104-165: The incremental filtering tests in `indexAllRepos` are
correct, but the duplicated `repos` fixture and test structure should be cleaned
up for maintainability. Extract the shared repository fixture (or switch
`describe('indexAllRepos incremental filtering')` to `test.each`) so the two
cases only vary by `formatVersion` and expected results, keeping the assertions
against `indexAllRepos`, `client.embed`, `vectorService.upsert`, and
`onRepoIndexed` unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 789b40c6-dd6c-4757-b5ba-dc92f8f8a6ba
📒 Files selected for processing (3)
src/services/vectorSearchService.test.tssrc/store/useAppStore.test.tssrc/store/useAppStore.ts
Addresses gemini-code-assist review on #236: - defaultVectorSearchConfig now defaults embeddingFormatVersion to EMBEDDING_FORMAT_VERSION so brand-new/reset configs are not stuck treating themselves as legacy (every incremental index reindexing all). - Existing persisted configs that are missing or out-of-range on embeddingFormatVersion still fall back to version 1 via a separate LEGACY_EMBEDDING_FORMAT_VERSION constant, preserving the one-time reindex safety for users migrating from older storage formats (#234 regression path). Addresses coderabbitai nitpick: - Parameterize the indexAllRepos incremental-filtering tests with test.each, add the missing-formatVersion (undefined) case which is the actual #234 regression path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed both reviewers in cd61f40: gemini-code-assist (high):
coderabbitai (nitpick):
Verification: 105 tests pass, lint clean, build green. |
Keep embeddingFormatVersion monotonic during runtime config merges so stale backend sync cannot downgrade a completed current-format index. Read vector config from fresh store state in the incremental index handler and align the badge count with format-version rebuild work. Also remove ineffective dynamic imports from SearchBar so production builds no longer warn about aiService/vectorSearchService chunking. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/settings/VectorSearchSettings.tsx (1)
233-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing the store's format-version helpers instead of a local hardcoded fallback.
(vectorSearchConfig.embeddingFormatVersion ?? 1)re-implements the "needs reindex" check that already lives inuseAppStore.ts(LEGACY_EMBEDDING_FORMAT_VERSION,isKnownEmbeddingFormatVersion). Duplicating this logic with a magic1is exactly the kind of divergence that caused the original bug (#234, where a separate allow-list droppedembeddingFormatVersion). Exporting and reusing the store's constant/helper here would keep both paths in sync going forward.♻️ Suggested consolidation
- // 嵌入文本格式版本升级时，即使无内容更新也需要触发增量索引来重建所有向量 - const formatVersionNeedsReindex = (vectorSearchConfig.embeddingFormatVersion ?? 1) < EMBEDDING_FORMAT_VERSION; + // 嵌入文本格式版本升级时，即使无内容更新也需要触发增量索引来重建所有向量 + const formatVersionNeedsReindex = (vectorSearchConfig.embeddingFormatVersion ?? LEGACY_EMBEDDING_FORMAT_VERSION) < EMBEDDING_FORMAT_VERSION;(requires exporting
LEGACY_EMBEDDING_FORMAT_VERSIONfromuseAppStore.ts)🤖 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/components/settings/VectorSearchSettings.tsx` around lines 233 - 237, The reindex check in VectorSearchSettings is duplicating the store’s embedding-format version logic by using a hardcoded fallback of 1. Replace the local `(vectorSearchConfig.embeddingFormatVersion ?? 1)` check in the `formatVersionNeedsReindex` computation with the shared helpers/constants from `useAppStore.ts` (such as `isKnownEmbeddingFormatVersion` and the exported legacy version constant) so both paths stay aligned. Keep the `incrementalTargetCount` decision based on that shared reindex check rather than a locally reimplemented version.
🤖 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.
Nitpick comments:
In `@src/components/settings/VectorSearchSettings.tsx`:
- Around line 233-237: The reindex check in VectorSearchSettings is duplicating
the store’s embedding-format version logic by using a hardcoded fallback of 1.
Replace the local `(vectorSearchConfig.embeddingFormatVersion ?? 1)` check in
the `formatVersionNeedsReindex` computation with the shared helpers/constants
from `useAppStore.ts` (such as `isKnownEmbeddingFormatVersion` and the exported
legacy version constant) so both paths stay aligned. Keep the
`incrementalTargetCount` decision based on that shared reindex check rather than
a locally reimplemented version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fd0831e-de05-49ac-94f4-c9f2f2adf1a0
📒 Files selected for processing (4)
src/components/SearchBar.tsxsrc/components/settings/VectorSearchSettings.tsxsrc/store/useAppStore.test.tssrc/store/useAppStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/useAppStore.ts
Export the shared legacy embedding format version and validation helper from the store, then reuse them in the vector search settings UI and incremental-index handler instead of hardcoding the legacy fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes #234. The incremental-index button reindexed from the beginning after every reload.
Root cause
normalizePersistedStateinsrc/store/useAppStore.tsreconstructedvectorSearchConfigfrom a fixed allow-list that droppedembeddingFormatVersion(along withsearchThreshold/searchTopK/enableHyDE/enableReranking). After hydration,embeddingFormatVersionbecameundefined, so:formatVersionNeedsReindex = (undefined ?? 1) < EMBEDDING_FORMAT_VERSION→trueindexAllRepostreated the format as legacy and intentionally reindexed every indexable repo on each incremental runThe button wiring and
indexAllReposbehavior were correct — the bug was in persistence normalization losing the saved format version.Changes
vectorSearchConfigdefaults indefaultVectorSearchConfigand a singlenormalizeVectorSearchConfigused by both hydration andmigrate, eliminating the duplicated in-place backfill that drifted from hydration.embeddingFormatVersion→[1, EMBEDDING_FORMAT_VERSION](future/corrupt versions can't skip required reindexing)searchThreshold→[0, 1]searchTopK→[5, 50](integer, matches the settings UI)embeddingConfigIdis actually present (avoids scanningembeddingConfigson every hydration).indexAllReposmigration logic intact: missing/old format version still triggers one safe reindex.Tests
useAppStore.test.ts: hydration preserves a fullvectorSearchConfig(incl.embeddingFormatVersion: 2andfalsebooleans); legacy persisted state without the newer fields defaults toembeddingFormatVersion: 1.vectorSearchService.test.ts: with current format version, incremental indexing only embeds never-indexed / content-updated repos; with an old format version, it reindexes all analyzed, non-failed repos.Verification
npm run test:run -- src/store/useAppStore.test.ts src/services/vectorSearchService.test.ts→ 24 passednpm run test:run→ 103 passed (11 files)npm run lint→ cleannpm run build→ successNotes
Backend config routes (
server/src/routes/configs.ts,server/src/db/schema.ts) also omitembeddingFormatVersion, but #234's reported path is local/frontend persistence. Kept this fix focused; backend config sync can be a follow-up if needed.🤖 Generated with Claude Code
Summary by CodeRabbit