fix(core,server): implement two-phase seed population correctly#92
Merged
Conversation
added 3 commits
March 3, 2026 14:44
Three bugs caused the seed response priority path to be dead code: 1. updateSeeds() in server.ts reassigned currentSeeds instead of using .clear()+.set() in-place mutation. Route closures captured the original Map by reference, so reassignment broke the reference chain. Now matches the updateHandlers() pattern. 2. The orchestrator's processSpec() called executeSeeds() to populate the store but never called server.updateSeeds() to sync the route builder's seed map. Added buildSeedMapFromStore() helper that reads materialized data from the store and passes it to updateSeeds(). 3. Hot reload's reloadSpecSeeds() had the same gap — executeSeeds() populated the store but the route builder seed map was never synced. Now calls updateSeeds() after executeSeeds(), which also handles registry hasSeed flags and WebSocket broadcast. Closes vite-20k
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors hot-reload seed handling to use explicit in-place Map mutation and per-spec updateSeeds calls; adds buildSeedMapFromStore(store) to derive seed maps from the in-memory store and replaces broadcast-based seed updates with direct updateSeeds synchronization. Changes
Sequence DiagramsequenceDiagram
participant Client as Hot-Reload Watcher
participant HotReload as hot-reload.ts
participant Orchestrator as orchestrator.ts
participant Store as In-Memory Store
participant Server as server.ts
participant Routes as Route Builder
Client->>HotReload: detects seed file change
HotReload->>Orchestrator: processSpec(spec)
Orchestrator->>Orchestrator: executeSeeds(spec)
Orchestrator->>Store: seed functions populate store
Orchestrator->>Orchestrator: buildSeedMapFromStore(store)
Orchestrator->>Server: server.updateSeeds(seedMap)
Server->>Server: currentSeeds.clear() and .set(...entries)
Server->>Routes: route closures read updated currentSeeds
HotReload->>Client: emit reload notification (seedMap.size)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/server.ts (1)
577-613:⚠️ Potential issue | 🟠 MajorSnapshot entries before mutation and split
updateSeedsinto helpers.At Line 581, clearing
currentSeedsbefore iteratingnewSeedscan wipe data if both references are the same map. Also, CI is failing on Line 577 for cognitive complexity (16 > 15), so this block needs decomposition.Proposed refactor (fix aliasing + reduce complexity)
+function replaceMapInPlace<K, V>(target: Map<K, V>, source: Map<K, V>): Array<[K, V]> { + const entries = Array.from(source.entries()); + target.clear(); + for (const [k, v] of entries) target.set(k, v); + return entries; +} + +function repopulateStoreFromSeedEntries( + store: Store, + entries: ReadonlyArray<[string, unknown[]]>, + logger: Logger, +): void { + store.clearAll(); + for (const [schemaName, items] of entries) { + for (const item of items) { + try { + store.create(schemaName, item); + } catch (error) { + logger.warn(`[vite-plugin-open-api-core] Failed to seed ${schemaName}:`, error); + } + } + } +} + updateSeeds(newSeeds: Map<string, unknown[]>): void { - currentSeeds.clear(); - for (const [key, value] of newSeeds) { - currentSeeds.set(key, value); - } - - store.clearAll(); - for (const [schemaName, items] of newSeeds) { - for (const item of items) { - try { - store.create(schemaName, item); - } catch (error) { - logger.warn(`[vite-plugin-open-api-core] Failed to seed ${schemaName}:`, error); - } - } - } + const seedEntries = replaceMapInPlace(currentSeeds, newSeeds); + repopulateStoreFromSeedEntries(store, seedEntries, logger); - const seedSchemaNames = new Set(newSeeds.keys()); + const seedSchemaNames = new Set(seedEntries.map(([schemaName]) => schemaName)); for (const entry of registry.endpoints.values()) { if (entry.responseSchema) { entry.hasSeed = seedSchemaNames.has(entry.responseSchema); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/server.ts` around lines 577 - 613, The updateSeeds method both risks aliasing bugs (clearing currentSeeds while newSeeds may be the same Map) and exceeds cognitive complexity limits; to fix, first avoid in-place iteration over newSeeds by capturing its entries up-front (e.g., const entries = Array.from(newSeeds.entries()) or a shallow copy) before clearing currentSeeds, then repopulate currentSeeds from that captured list; next split updateSeeds into small helper functions (e.g., applySeedsToCurrentSeeds(entries), repopulateStore(entries) which uses store.create and preserves the existing try/catch behavior, and updateRegistrySeedFlags(seedNames) which updates registry.endpoints' hasSeed flags), then call wsHub.broadcast and logger.info from the top-level updateSeeds to keep it under the complexity threshold; reference updateSeeds, currentSeeds, store.create, registry.endpoints, wsHub.broadcast, and logger.info when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/__tests__/per-spec-reload.test.ts`:
- Around line 385-390: The mocked implementation for mockedExecuteSeeds is
misformatted; reformat the returned object literal and surrounding lines to
match project formatting rules (e.g., spacing, indentation, trailing commas) so
CI formatter passes. Locate the mockedExecuteSeeds.mockImplementation block and
adjust the object returned from the mock (the { schemaCount: 1, totalItems: 1,
itemsPerSchema: { Pet: 1 }, skippedSchemas: [], warnings: [] } literal) and the
adjacent mockStore.getSchemas.mockReturnValue and mockStore.list.mockReturnValue
calls to follow the repository's formatter style.
In `@packages/server/src/hot-reload.ts`:
- Around line 460-478: Extract the duplicated buildSeedMapFromStore
implementation into a shared utility (e.g., export a function from a new or
existing common module) and replace both local implementations with an import;
specifically, move the function body that iterates store.getSchemas(), calls
store.list(schemaName), and sets non-empty arrays into a Map<string, unknown[]>
into the shared helper, export it, then update the callers in hot-reload
(function buildSeedMapFromStore) and the duplicate in orchestrator to import and
call the shared buildSeedMapFromStore; ensure the shared module exports the same
signature (accepts Store and returns Map<string, unknown[]>) and update any
imports/typing references accordingly.
---
Outside diff comments:
In `@packages/core/src/server.ts`:
- Around line 577-613: The updateSeeds method both risks aliasing bugs (clearing
currentSeeds while newSeeds may be the same Map) and exceeds cognitive
complexity limits; to fix, first avoid in-place iteration over newSeeds by
capturing its entries up-front (e.g., const entries =
Array.from(newSeeds.entries()) or a shallow copy) before clearing currentSeeds,
then repopulate currentSeeds from that captured list; next split updateSeeds
into small helper functions (e.g., applySeedsToCurrentSeeds(entries),
repopulateStore(entries) which uses store.create and preserves the existing
try/catch behavior, and updateRegistrySeedFlags(seedNames) which updates
registry.endpoints' hasSeed flags), then call wsHub.broadcast and logger.info
from the top-level updateSeeds to keep it under the complexity threshold;
reference updateSeeds, currentSeeds, store.create, registry.endpoints,
wsHub.broadcast, and logger.info when making these changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
.changesets/fix-vite-20k-seed-population.jsonpackages/core/src/server.tspackages/server/src/__tests__/per-spec-reload.test.tspackages/server/src/hot-reload.tspackages/server/src/orchestrator.ts
Comment on lines
+460
to
+478
| /** | ||
| * Build a seed data Map from the store's current contents. | ||
| * | ||
| * After `executeSeeds()` populates the store, this reads back the | ||
| * materialized data so it can be passed to `server.updateSeeds()`. | ||
| * | ||
| * @param store - Store populated by executeSeeds() | ||
| * @returns Map of schema name to array of items | ||
| */ | ||
| function buildSeedMapFromStore(store: Store): Map<string, unknown[]> { | ||
| const seedMap = new Map<string, unknown[]>(); | ||
| for (const schemaName of store.getSchemas()) { | ||
| const items = store.list(schemaName); | ||
| if (items.length > 0) { | ||
| seedMap.set(schemaName, items); | ||
| } | ||
| } | ||
| return seedMap; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract buildSeedMapFromStore into a shared utility.
This helper is duplicated in packages/server/src/orchestrator.ts (Line 158 onward). Centralizing it avoids drift in seed-map materialization behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/hot-reload.ts` around lines 460 - 478, Extract the
duplicated buildSeedMapFromStore implementation into a shared utility (e.g.,
export a function from a new or existing common module) and replace both local
implementations with an import; specifically, move the function body that
iterates store.getSchemas(), calls store.list(schemaName), and sets non-empty
arrays into a Map<string, unknown[]> into the shared helper, export it, then
update the callers in hot-reload (function buildSeedMapFromStore) and the
duplicate in orchestrator to import and call the shared buildSeedMapFromStore;
ensure the shared module exports the same signature (accepts Store and returns
Map<string, unknown[]>) and update any imports/typing references accordingly.
added 3 commits
March 3, 2026 17:16
Break long single-line return statements into multi-line object literals at lines 389, 501, and 521 to satisfy biome formatting rules.
Move the duplicated buildSeedMapFromStore() utility from both orchestrator.ts and hot-reload.ts into seeds.ts, which already houses seed-related utilities. Both files now import from the shared location, eliminating verbatim code duplication.
The extraction of buildSeedMapFromStore() to seeds.ts requires the test mock to pass through the actual implementation so it can read from the mock store objects during test execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three bugs caused the seed response priority path to be dead code:
updateSeeds() in server.ts reassigned currentSeeds instead of using
.clear()+.set() in-place mutation. Route closures captured the
original Map by reference, so reassignment broke the reference chain.
Now matches the updateHandlers() pattern.
The orchestrator's processSpec() called executeSeeds() to populate
the store but never called server.updateSeeds() to sync the route
builder's seed map. Added buildSeedMapFromStore() helper that reads
materialized data from the store and passes it to updateSeeds().
Hot reload's reloadSpecSeeds() had the same gap — executeSeeds()
populated the store but the route builder seed map was never synced.
Now calls updateSeeds() after executeSeeds(), which also handles
registry hasSeed flags and WebSocket broadcast.
Closes vite-20k
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation