Conversation
There was a problem hiding this comment.
Pull request overview
Refactors core write operations by renaming “insert/add edge/remove edge” terminology to a more uniform “add/connect/disconnect” API, while also removing several standalone benchmark/demo binaries.
Changes:
- Renamed command variants and core APIs (store/state machine/engine) from
InsertMemory/AddEdge/RemoveEdgetoAdd/Connect/Disconnect. - Updated unit tests across query/index/storage modules to use the new API names.
- Removed multiple
src/bin/*benchmark/demo utilities and made a small docs homepage update.
Reviewed changes
Copilot reviewed 14 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/app/(home)/page.tsx | Updates displayed GitHub stars count on the docs homepage. |
| crates/cortexadb-core/tests/common/mod.rs | Removes an (only-comment) shared test utilities module. |
| crates/cortexadb-core/src/store.rs | Renames store write APIs and internal WriteOp variants to add/connect/disconnect. |
| crates/cortexadb-core/src/storage/wal.rs | Updates WAL tests to use renamed Command variants. |
| crates/cortexadb-core/src/storage/checkpoint.rs | Updates checkpoint tests to use renamed state machine API. |
| crates/cortexadb-core/src/query/hybrid.rs | Updates query tests to renamed state machine edge/memory APIs. |
| crates/cortexadb-core/src/query/executor.rs | Updates query executor tests to renamed state machine APIs. |
| crates/cortexadb-core/src/index/temporal.rs | Updates temporal index tests to renamed state machine API. |
| crates/cortexadb-core/src/index/graph.rs | Updates graph index tests to renamed state machine edge/memory APIs. |
| crates/cortexadb-core/src/index/combined.rs | Updates combined index tests to renamed state machine edge/memory APIs. |
| crates/cortexadb-core/src/facade.rs | Switches facade implementation to call inner.add/add_batch/connect. |
| crates/cortexadb-core/src/engine.rs | Updates engine recovery/execution paths and tests to renamed Command variants. |
| crates/cortexadb-core/src/core/state_machine.rs | Renames memory/edge mutation methods and updates tests accordingly. |
| crates/cortexadb-core/src/core/command.rs | Renames Command variants and constructors; updates tests. |
| crates/cortexadb-core/src/bin/sync_bench.rs | Removes sync benchmark binary. |
| crates/cortexadb-core/src/bin/startup_bench.rs | Removes startup benchmark binary. |
| crates/cortexadb-core/src/bin/monkey_writer.rs | Removes monkey writer utility binary. |
| crates/cortexadb-core/src/bin/monkey_verify.rs | Removes monkey verify utility binary. |
| crates/cortexadb-core/src/bin/manual_store.rs | Removes manual store demo binary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| pub fn insert_memory(&self, entry: MemoryEntry) -> Result<CommandId> { | ||
| pub fn add(&self, entry: MemoryEntry) -> Result<CommandId> { |
There was a problem hiding this comment.
These renames remove/replace previously public-facing APIs (insert_memory, insert_memories_batch, add_edge, remove_edge). If CortexaDBStore is part of the crate’s public API, this is a breaking change for downstream users. Consider keeping backwards-compatible wrapper methods (marked #[deprecated]) that forward to the new names to ease migration.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| pub fn insert_memories_batch(&self, entries: Vec<MemoryEntry>) -> Result<CommandId> { | ||
| pub fn add_batch(&self, entries: Vec<MemoryEntry>) -> Result<CommandId> { |
There was a problem hiding this comment.
These renames remove/replace previously public-facing APIs (insert_memory, insert_memories_batch, add_edge, remove_edge). If CortexaDBStore is part of the crate’s public API, this is a breaking change for downstream users. Consider keeping backwards-compatible wrapper methods (marked #[deprecated]) that forward to the new names to ease migration.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| pub fn add_edge(&self, from: MemoryId, to: MemoryId, relation: String) -> Result<CommandId> { | ||
| pub fn connect(&self, from: MemoryId, to: MemoryId, relation: String) -> Result<CommandId> { |
There was a problem hiding this comment.
These renames remove/replace previously public-facing APIs (insert_memory, insert_memories_batch, add_edge, remove_edge). If CortexaDBStore is part of the crate’s public API, this is a breaking change for downstream users. Consider keeping backwards-compatible wrapper methods (marked #[deprecated]) that forward to the new names to ease migration.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| pub fn remove_edge(&self, from: MemoryId, to: MemoryId) -> Result<CommandId> { | ||
| pub fn disconnect(&self, from: MemoryId, to: MemoryId) -> Result<CommandId> { |
There was a problem hiding this comment.
These renames remove/replace previously public-facing APIs (insert_memory, insert_memories_batch, add_edge, remove_edge). If CortexaDBStore is part of the crate’s public API, this is a breaking change for downstream users. Consider keeping backwards-compatible wrapper methods (marked #[deprecated]) that forward to the new names to ease migration.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub enum Command { | ||
| /// Insert or update a memory entry | ||
| InsertMemory(MemoryEntry), | ||
| Add(MemoryEntry), | ||
| /// Delete a memory entry by ID | ||
| Delete(MemoryId), | ||
| /// Add an edge between two memories with a relation type | ||
| AddEdge { from: MemoryId, to: MemoryId, relation: String }, | ||
| Connect { from: MemoryId, to: MemoryId, relation: String }, | ||
| /// Remove an edge between two memories | ||
| RemoveEdge { from: MemoryId, to: MemoryId }, | ||
| Disconnect { from: MemoryId, to: MemoryId }, | ||
| } |
There was a problem hiding this comment.
Renaming Command variants changes their Serde external representation (e.g., JSON/YAML) from InsertMemory/AddEdge/RemoveEdge to Add/Connect/Disconnect. Even if WAL/checkpoints currently use bincode, this can break any tooling, debugging, or integrations that serialize commands in human-readable formats. To preserve backwards compatibility, add Serde aliases for the old variant names (e.g., alias InsertMemory -> Add, AddEdge -> Connect, RemoveEdge -> Disconnect).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| } | ||
|
|
||
| pub fn insert_memory(&self, entry: MemoryEntry) -> Result<CommandId> { | ||
| pub fn add(&self, entry: MemoryEntry) -> Result<CommandId> { |
There was a problem hiding this comment.
This method is documented/used as an insert-or-update operation (previously insert_memory). The new name add is ambiguous because it commonly implies insert-only. A more specific name (e.g., upsert, put, or insert_or_update) would better reflect behavior and reduce confusion alongside other add_* concepts.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@anaslimem I've opened a new pull request, #21, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@anaslimem I've opened a new pull request, #22, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@anaslimem I've opened a new pull request, #23, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@anaslimem I've opened a new pull request, #24, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@anaslimem I've opened a new pull request, #25, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@anaslimem I've opened a new pull request, #26, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Updated the whole code repo