feat(ts-client): support deleting multiple memories#1339
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds bulk-delete support to the TypeScript client’s memory deletion API to align with existing REST API payload shapes.
Changes:
- Updated
MemMachineMemory.delete()to accept a single ID or an array of IDs and emit*_idvs*_idspayloads accordingly. - Added/updated Jest coverage for multi-delete behavior and invalid ID arrays.
- Updated validation/error message for invalid IDs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/ts-client/src/memory/memmachine-memory.ts | Extends delete API to accept `string |
| packages/ts-client/tests/memmachine-memory.spec.ts | Adds regression tests for bulk deletion for episodic/semantic and updates validation error expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private async _deleteMemory(id: string | string[], memoryType: MemoryType): Promise<void> { | ||
| const ids = Array.isArray(id) ? id : [id] | ||
|
|
||
| if (ids.length === 0 || ids.some(memoryId => !memoryId || !memoryId.trim())) { |
There was a problem hiding this comment.
Runtime validation can throw a TypeError for non-string values (e.g., JS consumers passing 1), because memoryId.trim() assumes a string. Consider validating typeof memoryId === 'string' (and trimming) so invalid inputs consistently raise MemMachineAPIError instead of crashing with an unexpected exception.
| if (ids.length === 0 || ids.some(memoryId => !memoryId || !memoryId.trim())) { | |
| if (ids.length === 0 || ids.some(memoryId => typeof memoryId !== 'string' || !memoryId.trim())) { |
| const ids = Array.isArray(id) ? id : [id] | ||
|
|
||
| if (ids.length === 0 || ids.some(memoryId => !memoryId || !memoryId.trim())) { | ||
| throw new MemMachineAPIError('Memory ID must be a non-empty string or non-empty string array') |
There was a problem hiding this comment.
The error text is a bit ambiguous (e.g., it doesn’t explicitly state that every element of the array must be a non-empty string). Consider rewording to something unambiguous like: "Memory ID must be a non-empty string or a non-empty array of non-empty strings".
| throw new MemMachineAPIError('Memory ID must be a non-empty string or non-empty string array') | |
| throw new MemMachineAPIError('Memory ID must be a non-empty string or a non-empty array of non-empty strings') |
| ...(memoryType === 'episodic' ? (ids.length === 1 ? { episodic_id: ids[0] } : { episodic_ids: ids }) : {}), | ||
| ...(memoryType === 'semantic' ? (ids.length === 1 ? { semantic_id: ids[0] } : { semantic_ids: ids }) : {}) |
There was a problem hiding this comment.
These nested ternaries are hard to read and will get harder to extend (e.g., if more memory types are added). Consider constructing the payload key/value with a small helper or a single conditional branch (e.g., compute the key name based on memoryType and ids.length) to reduce duplication and improve readability.
| if (ids.length === 0 || ids.some(memoryId => !memoryId || !memoryId.trim())) { | ||
| throw new MemMachineAPIError('Memory ID must be a non-empty string or non-empty string array') | ||
| } |
There was a problem hiding this comment.
The new validation explicitly rejects empty arrays (ids.length === 0), but the added tests only cover empty strings and arrays containing an empty string. Add a unit test asserting memory.delete([], 'episodic') (and/or semantic) rejects with the same error to cover the empty-array branch.
|
@haosenwang1018 thank you for the pull request submission. Please sign your commits, resolve the unit test failures, and review the CoPilot feedback. You only need to resolve the relevant items. Thanks. |
|
This PR has been obsoleted. main has the string | string[] signature, better normalization (dedup + trim), and equivalent batch tests via #1266 |
Purpose of the change
Add bulk memory deletion support to the TypeScript client so it matches the existing REST API delete payloads.
Description
Fixes #1249
This change updates
MemMachineMemory.delete()in the TypeScript client to accept either a single ID or a list of IDs:memory.delete('1', 'episodic')memory.delete(['1', '2'], 'episodic')Behavior:
episodic_id/semantic_idepisodic_ids/semantic_idsAlso added regression coverage in
packages/ts-client/tests/memmachine-memory.spec.tsfor:Type of change
How Has This Been Tested?
Test command:
cd packages/ts-client && npx jest --runInBand tests/memmachine-memory.spec.tsChecklist
Maintainer Checklist