feat(web): memory management in web ui#127
Open
skulldogged wants to merge 3 commits intospacedriveapp:mainfrom
Open
feat(web): memory management in web ui#127skulldogged wants to merge 3 commits intospacedriveapp:mainfrom
skulldogged wants to merge 3 commits intospacedriveapp:mainfrom
Conversation
WalkthroughThis pull request adds complete CRUD (Create, Update, Delete) capabilities for agent memories across the full stack: frontend API client with typed request/response interfaces, a comprehensive React UI with forms and dialogs for managing memories, backend handlers with validation and embedding management, and updated routing to support the new operations alongside existing list and search endpoints. Changes
Poem
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
🧹 Nitpick comments (2)
interface/src/routes/AgentMemories.tsx (2)
272-274:virtualizerin the dependency array may be unnecessary.
useVirtualizerreturns a stable instance (ref-backed), so including it won't cause an infinite loop, but the effect is really driven bymemories.lengthandexpandedId. Addingvirtualizerjust satisfies the exhaustive-deps lint — which is fine, but worth knowing it's inert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentMemories.tsx` around lines 272 - 274, The effect calls virtualizer.measure() but the dependency array only needs to react to changes in memories.length and expandedId; virtualizer is a stable ref from useVirtualizer and can be removed from the dependencies to avoid a noisy lint-driven inclusion. Update the useEffect in AgentMemories.tsx to depend only on [memories.length, expandedId] (or if you must keep the lint happy, remove virtualizer and add a single-line eslint-disable comment above the useEffect for react-hooks/exhaustive-deps), leaving the call to virtualizer.measure() unchanged.
177-260: Consider passing form data as themutateargument instead of closing over component state.
createMutationandupdateMutationreadformDataandeditingMemoryfrom the component closure rather than receiving them asmutate(…)arguments. This works today becausesaveMemoryvalidates and callsmutate()synchronously in the same render, but it's fragile if the flow ever becomes async (e.g., adding a confirmation step).♻️ Example: passing form data through mutate
const createMutation = useMutation({ - mutationFn: () => { - return api.createMemory({ - agent_id: agentId, - content: formData.content, - memory_type: formData.memory_type, - importance: formData.importance, - }); - }, + mutationFn: (data: MemoryFormData) => { + return api.createMemory({ + agent_id: agentId, + content: data.content, + memory_type: data.memory_type, + importance: data.importance, + }); + }, ... }); const updateMutation = useMutation({ - mutationFn: () => { - if (!editingMemory) throw new Error("No memory selected"); - return api.updateMemory({ - agent_id: agentId, - memory_id: editingMemory.id, - content: formData.content, - memory_type: formData.memory_type, - importance: formData.importance, - }); - }, + mutationFn: ({ memoryId, data }: { memoryId: string; data: MemoryFormData }) => { + return api.updateMemory({ + agent_id: agentId, + memory_id: memoryId, + content: data.content, + memory_type: data.memory_type, + importance: data.importance, + }); + }, ... });Then in
saveMemory:if (editingMemory) { - updateMutation.mutate(); + updateMutation.mutate({ memoryId: editingMemory.id, data: formData }); } else { - createMutation.mutate(); + createMutation.mutate(formData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentMemories.tsx` around lines 177 - 260, createMutation and updateMutation close over component state (formData, editingMemory) which is fragile; modify both mutations so their mutationFn accepts an input argument (e.g., variables) and uses that to call api.createMemory / api.updateMemory instead of reading formData/editingMemory from closure, and then change saveMemory to build a plain payload (trimmed content, memory_type, importance, and for updates include editingMemory.id) and call createMutation.mutate(payload) or updateMutation.mutate(payload); keep the same success/error handlers (refreshMemories, setEditorOpen, setFormError, etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/memories.rs`:
- Around line 195-226: The handler currently calls store.save(&memory) before
creating/storing the embedding, which leaves a memory persisted without an
embedding if embed_one or embedding_table().store fails; either move the
embedding step before persisting (call
embedding_model_arc().embed_one(&memory.content) first, then store.save(&memory)
and embedding_table().store) or, if persisting first is required, add a rollback
on failure (on any error from embed_one or embedding_table().store call
store.delete(&memory.id) or equivalent to remove the partially saved memory),
and keep the existing ensure_fts_index() call after successful storage; update
the code paths around store.save, embedding_model_arc().embed_one,
embedding_table().store, and error handling to implement one of these fixes.
---
Nitpick comments:
In `@interface/src/routes/AgentMemories.tsx`:
- Around line 272-274: The effect calls virtualizer.measure() but the dependency
array only needs to react to changes in memories.length and expandedId;
virtualizer is a stable ref from useVirtualizer and can be removed from the
dependencies to avoid a noisy lint-driven inclusion. Update the useEffect in
AgentMemories.tsx to depend only on [memories.length, expandedId] (or if you
must keep the lint happy, remove virtualizer and add a single-line
eslint-disable comment above the useEffect for react-hooks/exhaustive-deps),
leaving the call to virtualizer.measure() unchanged.
- Around line 177-260: createMutation and updateMutation close over component
state (formData, editingMemory) which is fragile; modify both mutations so their
mutationFn accepts an input argument (e.g., variables) and uses that to call
api.createMemory / api.updateMemory instead of reading formData/editingMemory
from closure, and then change saveMemory to build a plain payload (trimmed
content, memory_type, importance, and for updates include editingMemory.id) and
call createMutation.mutate(payload) or updateMutation.mutate(payload); keep the
same success/error handlers (refreshMemories, setEditorOpen, setFormError, etc.)
unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
interface/src/api/client.tsinterface/src/routes/AgentMemories.tsxsrc/api/memories.rssrc/api/server.rs
Comment on lines
+195
to
+226
| store.save(&memory).await.map_err(|error| { | ||
| tracing::warn!(%error, agent_id = %request.agent_id, "failed to create memory"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
|
|
||
| let embedding = memory_search | ||
| .embedding_model_arc() | ||
| .embed_one(&memory.content) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::warn!(%error, agent_id = %request.agent_id, memory_id = %memory.id, "failed to embed memory content"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
|
|
||
| memory_search | ||
| .embedding_table() | ||
| .store(&memory.id, &memory.content, &embedding) | ||
| .await | ||
| .map_err(|error| { | ||
| tracing::warn!(%error, agent_id = %request.agent_id, memory_id = %memory.id, "failed to store memory embedding"); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; | ||
|
|
||
| if let Err(error) = memory_search.embedding_table().ensure_fts_index().await { | ||
| tracing::warn!(%error, "failed to ensure FTS index after memory create"); | ||
| } | ||
|
|
||
| Ok(Json(MemoryWriteResponse { | ||
| success: true, | ||
| memory, | ||
| })) | ||
| } |
There was a problem hiding this comment.
Memory is persisted before embedding — partial state on embedding failure.
If store.save() succeeds (Line 195) but embedding generation (Line 200) or storage (Line 209) fails, the handler returns a 500 but the memory already exists in the store without an embedding. This memory will appear in list views but be invisible to semantic search.
Consider either: (a) generating the embedding before saving the memory, or (b) cleaning up the saved memory on embedding failure.
🛡️ Option (a): embed first, then persist
+ // Generate embedding before persisting so we don't create orphaned memories
+ let embedding = memory_search
+ .embedding_model_arc()
+ .embed_one(&content)
+ .await
+ .map_err(|error| {
+ tracing::warn!(%error, agent_id = %request.agent_id, "failed to embed memory content");
+ StatusCode::INTERNAL_SERVER_ERROR
+ })?;
+
store.save(&memory).await.map_err(|error| {
tracing::warn!(%error, agent_id = %request.agent_id, "failed to create memory");
StatusCode::INTERNAL_SERVER_ERROR
})?;
- let embedding = memory_search
- .embedding_model_arc()
- .embed_one(&memory.content)
- .await
- .map_err(|error| {
- tracing::warn!(%error, agent_id = %request.agent_id, memory_id = %memory.id, "failed to embed memory content");
- StatusCode::INTERNAL_SERVER_ERROR
- })?;
-
memory_search
.embedding_table()
.store(&memory.id, &memory.content, &embedding)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/memories.rs` around lines 195 - 226, The handler currently calls
store.save(&memory) before creating/storing the embedding, which leaves a memory
persisted without an embedding if embed_one or embedding_table().store fails;
either move the embedding step before persisting (call
embedding_model_arc().embed_one(&memory.content) first, then store.save(&memory)
and embedding_table().store) or, if persisting first is required, add a rollback
on failure (on any error from embed_one or embedding_table().store call
store.delete(&memory.id) or equivalent to remove the partially saved memory),
and keep the existing ensure_fts_index() call after successful storage; update
the code paths around store.save, embedding_model_arc().embed_one,
embedding_table().store, and error handling to implement one of these fixes.
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.
Summary
Memoriesview./api/agents/memories(POST/PUT/DELETE), including embedding sync for create/update/delete so search stays consistent.Details
src/api/memories.rsfor create, update, and soft-delete (forget) flows.src/api/server.rsunder/agents/memories.interface/src/api/client.ts.interface/src/routes/AgentMemories.tsxwith: