fix: emit kv storage events from concrete implementations#481
Merged
Conversation
KvStorage defined put/get/getAll/delete/deleteall events and exposed on/off/emit plumbing, but neither KvViaTabularStorage nor FsFolderKvStorage ever emitted them, so listeners never fired. Wire up the emits in both implementations, make FsFolderKvStorage.deleteAll idempotent so repeated calls don't throw, and add a generic test that verifies events fire across all KV backends.
@workglow/cli
@workglow/ai
@workglow/job-queue
@workglow/knowledge-base
@workglow/storage
@workglow/task-graph
@workglow/tasks
@workglow/util
workglow
commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR ensures IKvStorage event listeners actually fire by emitting KV operation events from concrete storage implementations, and adds a backend-agnostic test to prevent regressions.
Changes:
- Emit
put/get/getAll/delete/deleteallevents fromKvViaTabularStorageoperations (including per-item emission forputBulk). - Emit
put/get/delete/deleteallevents fromFsFolderKvStorage, and makedeleteAll()idempotent viarm(..., { force: true }). - Add generic KV tests asserting events fire (including
putBulkemitting per item).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/test/src/test/storage-kv/genericKvRepositoryTests.ts | Adds generic event-emission assertions for KV backends. |
| packages/storage/src/kv/KvViaTabularStorage.ts | Wires KV event emissions into the tabular-backed KV implementation. |
| packages/storage/src/kv/FsFolderKvStorage.ts | Wires KV event emissions into the filesystem-backed KV implementation; makes deleteAll() idempotent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+94
| it("should emit put, get, delete and deleteall events", async () => { | ||
| const putFn = vi.fn(); | ||
| const getFn = vi.fn(); | ||
| const deleteFn = vi.fn(); | ||
| const deleteAllFn = vi.fn(); | ||
|
|
||
| repository.on("put", putFn); | ||
| repository.on("get", getFn); | ||
| repository.on("delete", deleteFn); | ||
| repository.on("deleteall", deleteAllFn); | ||
|
|
||
| await repository.put("k1", "v1"); | ||
| expect(putFn).toHaveBeenCalledWith("k1", "v1"); | ||
|
|
||
| const value = await repository.get("k1"); | ||
| expect(value).toEqual("v1"); | ||
| expect(getFn).toHaveBeenCalledWith("k1", "v1"); | ||
|
|
||
| await repository.get("missing"); | ||
| expect(getFn).toHaveBeenCalledWith("missing", undefined); | ||
|
|
||
| await repository.delete("k1"); | ||
| expect(deleteFn).toHaveBeenCalledWith("k1"); | ||
|
|
||
| await repository.deleteAll(); | ||
| expect(deleteAllFn).toHaveBeenCalled(); | ||
| }); |
Comment on lines
117
to
121
| : "utf-8"; | ||
| const content = (await readFile(localPath, { encoding })).toString().trim(); | ||
|
|
||
| let value: Value; | ||
| if (encoding === "utf-8") { |
… assertion GitMCP at https://gitmcp.io/docs has been failing the public MCP integration test, taking down both test-vitest-integration and test-bun-integration jobs. Remove it from the rotation. Also add a generic-KV test that exercises the getAll event (empty and populated cases), which was emitted by KvViaTabularStorage but not covered by the new event tests. Probes for getAll support so FsFolderKvStorage (which doesn't implement getAll) is skipped automatically.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
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
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.
KvStorage defined put/get/getAll/delete/deleteall events and exposed
on/off/emit plumbing, but neither KvViaTabularStorage nor FsFolderKvStorage
ever emitted them, so listeners never fired. Wire up the emits in both
implementations, make FsFolderKvStorage.deleteAll idempotent so repeated
calls don't throw, and add a generic test that verifies events fire across
all KV backends.