Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBackend now creates/fetches Graphiti entities directly via Cypher, adds an embedding-model name extractor, standardizes embedding_dim=1536, and expands MemoryTool.create. Frontend disables input/refresh by default, re-enables them on graph selection, removes a user alert, and updates addMessage to accept an isLoading flag. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant MemoryTool
participant GraphDB as Graph DB (Cypher)
participant Embedder
rect rgba(200,230,255,0.12)
note over Client,MemoryTool: Updated creation flow
Client->>MemoryTool: create(user_id, graph_id, use_direct_entities=True)
MemoryTool->>GraphDB: MERGE User, Database, HAS_DATABASE (uuid, created_at, summary, name_embedding)
MemoryTool->>Embedder: embed(name/summary)
Embedder-->>MemoryTool: embedding
MemoryTool-->>Client: MemoryTool instance
end
rect rgba(200,255,200,0.12)
note over Client,MemoryTool: Save query flow
Client->>MemoryTool: save_query_memory(query, summary, ...)
MemoryTool->>GraphDB: MATCH Database by graph_id → get database_node_uuid
MemoryTool->>GraphDB: CHECK existing Query node (by database_node_uuid + query)
alt not exists
MemoryTool->>Embedder: embed(query/summary)
Embedder-->>MemoryTool: embedding
MemoryTool->>GraphDB: CREATE Query node + RELATION
else exists
Note right of GraphDB: Skip creation
end
MemoryTool-->>Client: created | skipped
end
sequenceDiagram
autonumber
actor User
participant UI as UI (Buttons/Input)
participant GraphSelect
participant Chat
rect rgba(255,240,200,0.12)
note over UI: Initial disabled state
UI->>UI: Refresh button disabled
UI->>UI: Message input disabled
end
rect rgba(220,255,220,0.12)
User->>GraphSelect: choose graph
GraphSelect->>UI: enable refresh button & submit button (if present)
GraphSelect->>GraphSelect: setSelectedGraph / onSelect / close options
end
rect rgba(255,220,220,0.12)
User->>Chat: sendMessage()
Chat->>Chat: guard: if no graph → return silently
Chat->>UI: addMessage(message, "user", false, currentUser)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes memory saving functionality by improving the user interface behavior when selecting databases and refactoring the Graphiti memory tool implementation. The changes focus on enabling UI controls when a database is selected and streamlining the backend memory management code.
Key changes:
- Enhanced UI state management by enabling input controls when a database is selected
- Simplified frontend validation logic by removing error messages and alerts
- Refactored Graphiti memory tool to use direct Cypher queries instead of search operations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/ts/modules/graph_select.ts | Enables submit button and refresh button when a graph is selected |
| app/ts/modules/chat.ts | Removes error message display and adds parameter to addMessage call |
| app/ts/app.ts | Removes alert dialog for database selection validation |
| app/templates/components/chat_input.j2 | Sets message input field to disabled by default |
| app/templates/components/chat_header.j2 | Sets refresh button to disabled by default |
| api/memory/graphiti_tool.py | Major refactor replacing search-based operations with direct Cypher queries and improved embedding model handling |
| if (state.currentRequestController) { | ||
| state.currentRequestController.abort(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The addition of a false parameter to addMessage changes the function signature without clear documentation of what this boolean parameter represents. Consider using a named parameter or adding a comment to clarify the purpose of this parameter.
![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 (4)
api/memory/graphiti_tool.py (3)
62-75: Honor use_direct_entities and make index creation idempotent
- The use_direct_entities argument is unused; callers can’t opt out.
- CREATE VECTOR INDEX will fail on repeated calls; make it idempotent.
@classmethod -async def create(cls, user_id: str, graph_id: str, use_direct_entities: bool = True) -> "MemoryTool": +async def create(cls, user_id: str, graph_id: str, use_direct_entities: bool = True) -> "MemoryTool": """Async factory to construct and initialize the tool.""" self = cls(user_id, graph_id) - - await self._ensure_entity_nodes_direct(user_id, graph_id) + if use_direct_entities: + await self._ensure_entity_nodes_direct(user_id, graph_id) + else: + # Fallback to legacy episode-based creation + await self._ensure_user_node(user_id) + await self._ensure_database_node(graph_id, user_id) - vector_size = Config.EMBEDDING_MODEL.get_vector_size() - driver = self.graphiti_client.driver - await driver.execute_query(f"CREATE VECTOR INDEX FOR (p:Query) ON (p.embeddings) OPTIONS {{dimension:{vector_size}, similarityFunction:'euclidean'}}") + vector_size = Config.EMBEDDING_MODEL.get_vector_size() + driver = self.graphiti_client.driver + try: + await driver.execute_query( + f"CREATE VECTOR INDEX FOR (p:Query) ON (p.embeddings) " + f"OPTIONS {{dimension:{vector_size}, similarityFunction:'euclidean'}}" + ) + except Exception as e: # keep soft-idempotent; avoid startup failure + msg = str(e).lower() + if "already exists" not in msg: + print(f"Error creating vector index on Query.embeddings: {e}") return selfAlso applies to: 70-73
784-809: Avoid hardcoding embedding_dim; derive from Config to prevent mismatchesHardcoding 1536 risks index/embedding dimension mismatch if the model changes (e.g., text-embedding-3-large = 3072).
- embedder=OpenAIEmbedder( - config=OpenAIEmbedderConfig( - embedding_model=config.embedding_deployment, - embedding_dim=1536 - ), + embedder=OpenAIEmbedder( + config=OpenAIEmbedderConfig( + embedding_model=config.embedding_deployment, + embedding_dim=Config.EMBEDDING_MODEL.get_vector_size(), + ), client=embedding_client_azure, ), ... - graphiti_client = Graphiti( + graphiti_client = Graphiti( graph_driver=falkor_driver, embedder=OpenAIEmbedder( config=OpenAIEmbedderConfig( - embedding_model=embedding_model_name, - embedding_dim=1536 + embedding_model=embedding_model_name, + embedding_dim=Config.EMBEDDING_MODEL.get_vector_size(), ) ), )
336-367: Use Cypher parameters for strings; avoid manual escaping and injection pitfallsString interpolation with escaped quotes is brittle and error-prone. Parameterize user_query, sql_query, and error; keep relationship type derived from a boolean.
- # First check if a Query node with the same user_query and sql_query already exists - check_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MATCH (db)-[r]->(q:Query) - WHERE q.user_query = "{escaped_query}" AND q.sql_query = "{escaped_sql}" - RETURN q.uuid as existing_query_uuid - LIMIT 1 - """ - - graph_driver = self.graphiti_client.driver - check_result = await graph_driver.execute_query(check_query) + # First check if a Query node with the same user_query and sql_query already exists + check_query = """ + MATCH (db:Entity {uuid: $db_uuid})-[]->(q:Query) + WHERE q.user_query = $user_query AND q.sql_query = $sql_query + RETURN q.uuid as existing_query_uuid + LIMIT 1 + """ + graph_driver = self.graphiti_client.driver + check_result = await graph_driver.execute_query( + check_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + ) ... - cypher_query = f""" - MATCH (db:Entity {{uuid: "{database_node_uuid}"}}) - MERGE (q:Query {{ - user_query: "{escaped_query}", - sql_query: "{escaped_sql}", - success: {str(success).lower()}, - error: "{escaped_error}", - timestamp: timestamp(), - embeddings: vecf32($embedding) - }}) - CREATE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) - RETURN q.uuid as query_uuid - """ + cypher_query = f""" + MATCH (db:Entity {{uuid: $db_uuid}}) + MERGE (q:Query {{ + user_query: $user_query, + sql_query: $sql_query, + success: $success, + error: $error, + timestamp: timestamp(), + embeddings: vecf32($embedding) + }}) + CREATE (db)-[:{relationship_type} {{timestamp: timestamp()}}]->(q) + RETURN q.uuid as query_uuid + """ ... - result = await graph_driver.execute_query(cypher_query, embedding=embeddings) + result = await graph_driver.execute_query( + cypher_query, + db_uuid=database_node_uuid, + user_query=query, + sql_query=sql_query, + success=success, + error=error or "", + embedding=embeddings, + )Also applies to: 318-326
app/ts/app.ts (1)
89-106: Loading spinner can get stuck on refresh error.On a non-2xx response you
returnbefore removing theloadingclass and clearingtitle, leaving the button in a perpetual loading state. Use a try/finally around the refresh flow and move the cleanup into finally.DOM.graphSelectRefresh?.addEventListener("click", async () => { const selected = getSelectedGraph(); const refreshButton = DOM.graphSelectRefresh; if (!refreshButton) return; - if (!selected || selected === "Select Database") return + if (!selected || selected === "Select Database") return; refreshButton.classList.add("loading"); + refreshButton.title = ""; - - const result = await fetch( - `/graphs/${encodeURIComponent(selected)}/refresh`, - { - method: "POST", - } - ); - - if (!result.ok) { - console.error( - "Failed to refresh graph:", - result.status, - result.statusText - ); - return; - } - - if (result.body) { - const reader = result.body.getReader(); - const decoder = new TextDecoder(); - let buffer = ""; - - try { - while (true) { - const { done, value } = await reader.read(); - if (done) break; - - buffer += decoder.decode(value, { stream: true }); - - // Process complete messages separated by boundary - const messages = buffer.split("|||FALKORDB_MESSAGE_BOUNDARY|||"); - buffer = messages.pop() || ""; // Keep incomplete message in buffer - - for (const message of messages) { - if (message.trim()) { - try { - const parsed = JSON.parse(message.trim()); - - if (parsed.type === "reasoning_step") { - refreshButton.title += ` ${parsed.message}\n`; - } else if (parsed.type === "final_result") { - refreshButton.title += ` ${parsed.message}`; - } - } catch (e) { - console.warn("Failed to parse message:", message, e); - } - } - } - } - } finally { - reader.releaseLock(); - } - } - - if (DOM.schemaContainer && DOM.schemaContainer.classList.contains("open")) { - await loadAndShowGraph(selected); - setTimeout(resizeGraph, 450); - } - - refreshButton.classList.remove("loading"); - refreshButton.title = ""; + try { + const result = await fetch( + `/graphs/${encodeURIComponent(selected)}/refresh`, + { method: "POST" } + ); + + if (!result.ok) { + console.error( + "Failed to refresh graph:", + result.status, + result.statusText + ); + return; // finally will still run + } + + if (result.body) { + const reader = result.body.getReader(); + const decoder = new TextDecoder(); + let buffer = ""; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + buffer += decoder.decode(value, { stream: true }); + const messages = buffer.split("|||FALKORDB_MESSAGE_BOUNDARY|||"); + buffer = messages.pop() || ""; + for (const message of messages) { + if (!message.trim()) continue; + try { + const parsed = JSON.parse(message.trim()); + if (parsed.type === "reasoning_step") { + refreshButton.title += ` ${parsed.message}\n`; + } else if (parsed.type === "final_result") { + refreshButton.title += ` ${parsed.message}`; + } + } catch (e) { + console.warn("Failed to parse message:", message, e); + } + } + } + } finally { + reader.releaseLock(); + } + } + + if (DOM.schemaContainer && DOM.schemaContainer.classList.contains("open")) { + await loadAndShowGraph(selected); + setTimeout(resizeGraph, 450); + } + } finally { + refreshButton.classList.remove("loading"); + refreshButton.title = ""; + } });Also applies to: 149-151
🧹 Nitpick comments (5)
app/templates/components/chat_header.j2 (1)
6-6: A11y nit: reflect disabled state for screen readersConsider adding aria-disabled="true" and a disabled style class to match the visual/interactive state until it’s re-enabled by JS.
- <button class="action-button" id="graph-select-refresh" disabled> + <button class="action-button is-disabled" id="graph-select-refresh" disabled aria-disabled="true">api/memory/graphiti_tool.py (2)
150-258: Blocking calls inside async and broad excepts
- Config.EMBEDDING_MODEL.embed(...) is synchronous and may block the event loop. Consider running in a thread executor or switching to an async embedder if available.
- Multiple bare except Exception; narrow where feasible or at least log with structured logging.
I can wire an async embedding utility (loop.run_in_executor) and replace prints with logging.getLogger(name).exception(...). Want a patch?
416-429: Use limit variable for vector queryHardcoded 10 for topK fetches more than necessary.
- CALL db.idx.vector.queryNodes('Query', 'embeddings', 10, vecf32($embedding)) + CALL db.idx.vector.queryNodes('Query', 'embeddings', $topK, vecf32($embedding)) ... - records, header, _ = await graph_driver.execute_query(cypher_query, embedding=query_embedding, database_node_uuid=database_node_uuid) + records, header, _ = await graph_driver.execute_query( + cypher_query, + embedding=query_embedding, + database_node_uuid=database_node_uuid, + topK=limit, + )app/ts/app.ts (1)
81-88: Guard is fine; avoid hard-coded placeholder string.
"Select Database"is duplicated across files and is brittle if the label changes. Export a constant (e.g., SELECT_DB_PLACEHOLDER) from graph_select/config and reuse it here.app/ts/modules/chat.ts (1)
14-15: Guard is fine; centralize the placeholder string.Same string literal as in app.ts. Export and reuse a single SELECT_DB_PLACEHOLDER to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
api/memory/graphiti_tool.py(9 hunks)app/templates/components/chat_header.j2(1 hunks)app/templates/components/chat_input.j2(1 hunks)app/ts/app.ts(1 hunks)app/ts/modules/chat.ts(1 hunks)app/ts/modules/graph_select.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/ts/app.tsapp/ts/modules/graph_select.tsapp/ts/modules/chat.ts
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/memory/graphiti_tool.py
🧬 Code graph analysis (3)
app/ts/modules/graph_select.ts (1)
app/ts/modules/config.ts (1)
DOM(39-65)
app/ts/modules/chat.ts (2)
app/ts/modules/config.ts (1)
state(73-77)app/ts/modules/messages.ts (1)
addMessage(8-118)
api/memory/graphiti_tool.py (1)
api/config.py (2)
Config(49-135)embed(20-33)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
63-63: Unused class method argument: use_direct_entities
(ARG003)
249-249: Do not catch blind exception: Exception
(BLE001)
253-253: Consider moving this statement to an else block
(TRY300)
255-255: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (2)
app/templates/components/chat_input.j2 (1)
4-4: Re-enable logic already implementedThe message input is re-enabled in app/ts/modules/chat.ts (line 254) and app/ts/modules/graphs.ts (line 126), so it will be enabled after graph selection. The original concern can be dismissed.
Likely an incorrect or invalid review comment.
api/memory/graphiti_tool.py (1)
380-439: Verify Graphiti.search_ calls return Entity nodes*After switching to direct Entity creation, these
search_/searchcalls may not surface Entity nodes unless Graphiti indexes the Entity label and itsname_embedding. Confirm against a live Graphiti instance; if they don’t return the expected nodes, replace each lookup with a direct Cypher MATCH by label/name or UUID (as done insave_query_memory).Affected locations in
api/memory/graphiti_tool.py:
- lines 85–87 (initial node search)
- lines 122–124 (database node lookup)
- lines 397–400 (
retrieve_similar_queriesDB node search)- lines 459–463 (user node search)
- lines 491–495 (database node search)
- lines 507–511 (reranked results search)
| addMessage(message, "user", false, (window as any).currentUser || null); | ||
| if (DOM.messageInput) DOM.messageInput.value = ''; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure all addMessage call sites use the updated argument order.
This call is correct (3rd arg isQuery=false, 4th user). There are older call sites passing the user object as the 3rd arg, which drops the avatar. Verify and align.
Run to find suspicious 3-arg "user" calls:
If matches exist, update them to addMessage(msg, "user", false, currentUser).
🏁 Script executed:
#!/bin/bash
# 3-arg addMessage with "user" as 2nd arg (likely missing the boolean third arg)
rg -nP --type=ts -C2 '\baddMessage\s*\(\s*[^,]+,\s*"user"\s*,\s*[^,\)]+\s*\)'Length of output: 407
Update the 3-arg addMessage call on line 257 to include the isQuery boolean.
--- a/app/ts/modules/chat.ts
+++ b/app/ts/modules/chat.ts
@@ 255,262
- addMessage(`User choice: ${confirmation}`, "user", (window as any).currentUser || null);
+ addMessage(`User choice: ${confirmation}`, "user", false, (window as any).currentUser || null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addMessage(message, "user", false, (window as any).currentUser || null); | |
| if (DOM.messageInput) DOM.messageInput.value = ''; | |
| ++ b/app/ts/modules/chat.ts | |
| @@ -255,7 +255,7 @@ | |
| // … other logic … | |
| addMessage(`User choice: ${confirmation}`, "user", false, (window as any).currentUser || null); | |
| // … following logic … |
🤖 Prompt for AI Agents
In app/ts/modules/chat.ts around lines 20 to 21, the addMessage invocation must
include the isQuery boolean parameter; modify the call to pass the appropriate
isQuery value (insert the boolean as the third argument) so the remaining
arguments are shifted accordingly (e.g., addMessage(message, "user", /*isQuery*/
false, /*other args*/ (window as any).currentUser || null)); ensure the argument
order matches addMessage's signature.
| if (DOM.graphSelectRefresh && DOM.submitButton) { | ||
| DOM.graphSelectRefresh.disabled = false | ||
| DOM.submitButton.disabled = false | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also enable the message input; enable controls independently
Currently only refresh and submit are enabled, and only if both elements exist. Enable each control independently and re-enable the input so users can type immediately.
- if (DOM.graphSelectRefresh && DOM.submitButton) {
- DOM.graphSelectRefresh.disabled = false
- DOM.submitButton.disabled = false
- };
+ if (DOM.graphSelectRefresh) {
+ DOM.graphSelectRefresh.disabled = false;
+ DOM.graphSelectRefresh.removeAttribute('aria-disabled');
+ }
+ if (DOM.submitButton) {
+ DOM.submitButton.disabled = false;
+ }
+ if (DOM.messageInput) {
+ DOM.messageInput.disabled = false;
+ DOM.messageInput.focus();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (DOM.graphSelectRefresh && DOM.submitButton) { | |
| DOM.graphSelectRefresh.disabled = false | |
| DOM.submitButton.disabled = false | |
| }; | |
| // Re-enable the graph-select refresh button if it exists | |
| if (DOM.graphSelectRefresh) { | |
| DOM.graphSelectRefresh.disabled = false; | |
| DOM.graphSelectRefresh.removeAttribute('aria-disabled'); | |
| } | |
| // Re-enable the submit button if it exists | |
| if (DOM.submitButton) { | |
| DOM.submitButton.disabled = false; | |
| } | |
| // Re-enable and focus the message input so users can type immediately | |
| if (DOM.messageInput) { | |
| DOM.messageInput.disabled = false; | |
| DOM.messageInput.focus(); | |
| } |
🤖 Prompt for AI Agents
In app/ts/modules/graph_select.ts around lines 46 to 49, the code only enables
refresh and submit when both elements exist and neglects the message input;
change it to enable each control independently by checking each DOM element
separately (if DOM.graphSelectRefresh then set disabled = false; if
DOM.submitButton then set disabled = false; if DOM.messageInput then set
disabled = false and call focus() so users can type immediately), ensuring each
check is independent rather than combined.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores