Conversation
Small fixes
Fix memory user & database saving
Replcae bot before
Small fix
|
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. Caution Review failedThe pull request is closed. 📜 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)
✨ 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 implements comprehensive UI improvements and bug fixes for QueryWeaver, transforming the input field from a basic text input to an auto-resizing textarea with enhanced user experience. The changes focus on improving input handling, visual feedback, and consistency across the interface.
Key changes include:
- Implementation of auto-resizing textarea functionality to replace the basic text input
- Enhanced UI state management with proper button enabling/disabling logic
- Visual consistency improvements including color scheme updates and layout adjustments
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/ts/modules/input.ts | New module implementing textarea auto-resize functionality with height adjustment logic |
| app/ts/modules/chat.ts | Updated message sending logic with textarea support and improved UI state handling |
| app/ts/app.ts | Enhanced event listeners for textarea input with shift+enter support and dynamic submit button state |
| app/templates/components/chat_input.j2 | Converted input element to textarea with auto-resize capabilities |
| app/public/css/*.css | Style updates for button states, color consistency, and layout improvements |
| app/ts/modules/config.ts | Updated DOM element type from HTMLInputElement to HTMLTextAreaElement |
| api/memory/graphiti_tool.py | Backend improvements to memory tool initialization and entity node creation |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/ts/modules/chat.ts (1)
263-264: Bug: addMessage arg order wrong (userInfo passed as isQuery).Third arg is isQuery (boolean). You're passing the user object there, which coerces to true and drops avatar/name.
- addMessage(`User choice: ${confirmation}`, "user", (window as any).currentUser || null); + addMessage(`User choice: ${confirmation}`, "user", false, (window as any).currentUser || null);api/memory/graphiti_tool.py (1)
329-366: Fix Cypher injection risk; parameterize values and embed the original text, not the escaped oneManual string interpolation and DIY escaping are brittle. Use parameters for user_query/sql/error and keep the dynamic relationship type as derived from a boolean.
- # Escape quotes in the query and SQL for Cypher - escaped_query = query.replace("'", "\\'").replace('"', '\\"') - escaped_sql = sql_query.replace("'", "\\'").replace('"', '\\"') - escaped_error = error.replace("'", "\\'").replace('"', '\\"') if error else "" - embeddings = Config.EMBEDDING_MODEL.embed(escaped_query)[0] + # Embed the original user query (escaping not needed for embeddings) + embeddings = Config.EMBEDDING_MODEL.embed(query)[0] @@ - 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 - """ + check_query = """ + MATCH (db:Entity {uuid: $db_uuid}) + MATCH (db)-[r]->(q:Query) + WHERE q.user_query = $user_query AND q.sql_query = $sql_query + RETURN q.uuid as existing_query_uuid + LIMIT 1 + """ @@ - check_result = await graph_driver.execute_query(check_query) + 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}}) + CREATE (q:Query {{ + uuid: randomUUID(), + 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) + 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, + ) return True
♻️ Duplicate comments (6)
app/templates/components/chat_input.j2 (1)
4-4: Remove invalid type attribute on textarea.Textarea doesn’t support type=. This was flagged previously.
- <textarea type="text" id="message-input" placeholder="Describe the SQL query you want..." rows="1"></textarea> + <textarea id="message-input" placeholder="Describe the SQL query you want..." rows="1"></textarea>app/ts/app.ts (1)
228-229: Add missing semicolon (style consistency).Previously noted; keep consistency with the codebase.
- setupTextareaAutoResize() + setupTextareaAutoResize();app/ts/modules/chat.ts (1)
23-27: Correct param order for addMessage (resolves earlier review).Passing isQuery=false, userInfo as 4th arg is correct and addresses the prior comment.
app/ts/modules/input.ts (1)
15-26: NaN/Infinity bug: parseInt(maxHeight) breaks when max-height is 'none' or non-px.This yields NaN and sets height to 'NaNpx'.
- const maxHeight = parseInt(getComputedStyle(textarea).maxHeight); + const computedMaxHeight = getComputedStyle(textarea).maxHeight; + let maxHeight: number; + if (computedMaxHeight === "none") { + maxHeight = Infinity; + } else if (computedMaxHeight.endsWith("px")) { + maxHeight = parseFloat(computedMaxHeight); + } else { + const parsed = parseFloat(computedMaxHeight); + maxHeight = Number.isFinite(parsed) ? parsed : Infinity; + }api/memory/graphiti_tool.py (2)
171-178: Avoid magic string for group_idThe hardcoded '\_' is a magic string; define a module-level constant and reuse.
+# Default group id for memory entities +GROUP_ID_DEFAULT = "_" @@ - 'group_id': '\\_', + 'group_id': GROUP_ID_DEFAULT,
206-215: Repeat of magic string for group_idApply the same constant here for consistency.
- 'group_id': '\\_', + 'group_id': GROUP_ID_DEFAULT,
🧹 Nitpick comments (17)
app/templates/components/database_modal.j2 (2)
9-15: Copy change looks fine; consider i18n key or helper to avoid hardcoded strings.If you’re standardizing “Select Database” across the app, consider centralizing this label (i18n/const) so future tweaks don’t drift between templates.
28-28: Grammar tweak: add article “a”.“Select Database type first...” reads a bit off. Suggest “Select a database type first...”.
Apply this template diff:
-<input type="text" id="db-url-input" class="db-modal-input" placeholder="Select Database type first..." disabled /> +<input type="text" id="db-url-input" class="db-modal-input" placeholder="Select a database type first..." disabled />app/templates/components/chat_header.j2 (1)
25-26: Mark decorative arrow as hidden from screen readers.The “▼” is purely decorative for a file label; hide it from AT.
- <span class="dropdown-arrow">▼</span> + <span class="dropdown-arrow" aria-hidden="true">▼</span>app/ts/modules/graph_select.ts (1)
46-49: Enable controls independently to be more resilient.If one element is missing, the other should still be enabled.
- if (DOM.graphSelectRefresh && DOM.submitButton) { - DOM.graphSelectRefresh.disabled = false - DOM.submitButton.disabled = false - }; + if (DOM.graphSelectRefresh) { + DOM.graphSelectRefresh.disabled = false; + } + if (DOM.submitButton) { + DOM.submitButton.disabled = false; + }app/public/css/chat-components.css (1)
86-102: Avoid conflicting avatar labels: remove user selector from the 'QW' rule.Including .user-message-container::after here sets content:'QW' before being overridden below; it's redundant and can cause confusion. Limit this rule to bot/followup/final only.
-.user-message-container::after, .bot-message-container::before, .followup-message-container::before, .final-result-message-container::before { height: 32px; width: 32px; - content: 'QW'; + content: 'QW'; color: var(--text-secondary); display: flex;app/templates/components/chat_input.j2 (1)
5-5: Add aria-labels for better accessibility.Title helps on hover but screen readers benefit from aria-label on buttons.
- <button class="input-button" title="Submit" id="submit-button" disabled> + <button class="input-button" title="Submit" aria-label="Submit message" id="submit-button" disabled> ... - <button class="input-button" title="Pause" id="pause-button"> + <button class="input-button" title="Pause" aria-label="Pause response" id="pause-button">Also applies to: 15-15
app/ts/app.ts (1)
65-70: Guard against IME composition when sending on Enter.Prevent accidental sends for CJK input; also no need to cast e.
- DOM.messageInput?.addEventListener("keydown", (e: KeyboardEvent) => { - if ((e as KeyboardEvent).key === "Enter" && !e.shiftKey) { + DOM.messageInput?.addEventListener("keydown", (e: KeyboardEvent) => { + if (e.isComposing) return; + if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); sendMessage(); } });app/ts/modules/chat.ts (1)
17-17: Silent UX on no graph selection — consider user feedback.Returning after console.debug gives no UI hint. Suggest prompting the user.
- if (!selectedValue || selectedValue === "Select Database") return console.debug("No selected graph"); + if (!selectedValue || selectedValue === "Select Database") { + addMessage('Please select a database before sending a message.', "followup"); + return; + }app/ts/modules/input.ts (1)
34-45: Auto-resize wiring looks good.Input and paste handlers with initial adjust are correct. Optional: also handle "cut".
// Adjust height on input textarea.addEventListener("input", adjustTextareaHeight); + textarea.addEventListener("cut", () => setTimeout(adjustTextareaHeight, 0));api/memory/graphiti_tool.py (8)
30-44: Helper to strip provider prefix looks goodHandles simple "provider/model" shapes correctly. Consider trimming whitespace to be extra-safe.
def extract_embedding_model_name(full_model_name: str) -> str: @@ - if "/" in full_model_name: + full_model_name = full_model_name.strip() + if "/" in full_model_name: return full_model_name.split("/", 1)[1] # Remove provider prefix else: return full_model_name
63-69: Honor the use_direct_entities flag (currently unused) and keep the legacy pathYou always call the direct path, ignoring the flag. Wire both paths, or drop the flag.
@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: + # Legacy Graphiti-based ensure (kept for compatibility) + 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'}}") + # Create vector index idempotently + 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: + # Best-effort: ignore 'already exists' errors; re-raise others + if "already exists" not in str(e).lower(): + raiseRun make lint to confirm ARG003 (unused arg) is resolved and no new lint issues are introduced.
150-201: Direct node lookup/create: prefer non-blocking embeddings within async and consistent result handling
- Config.EMBEDDING_MODEL.embed likely does network I/O; calling it sync inside asyncio can block. Consider an async embedder or run_in_executor.
- execute_query return handling mixes tuple indexing and destructuring across the file; standardize to destructuring for clarity.
- user_name_embedding = Config.EMBEDDING_MODEL.embed(user_node_name)[0] + # Offload sync embedding call to avoid blocking the event loop + loop = asyncio.get_running_loop() + user_name_embedding = await loop.run_in_executor( + None, lambda: Config.EMBEDDING_MODEL.embed(user_node_name)[0] + ) @@ - user_check_result = await graph_driver.execute_query(check_user_query, name=user_node_name) - - if not user_check_result[0]: # If no records found, create user node + user_records, _, _ = await graph_driver.execute_query(check_user_query, name=user_node_name) + if not user_records: # If no records found, create user node user_uuid = str(uuid.uuid4())
232-252: Narrow exceptions and use try/else for relationship creationCatching bare Exception and returning inside try triggers TRY300; restructure and narrow where possible.
- try: + try: relationship_query = """ @@ - await graph_driver.execute_query( + await graph_driver.execute_query( relationship_query, user_name=user_node_name, database_name=database_node_name ) - print(f"Created HAS_DATABASE relationship between {user_node_name} and {database_node_name}") - - except Exception as rel_error: + except Exception as rel_error: print(f"Error creating HAS_DATABASE relationship: {rel_error}") - # Don't fail the entire function if relationship creation fails - - return True + # Don't fail the entire function if relationship creation fails + else: + print(f"Created HAS_DATABASE relationship between {user_node_name} and {database_node_name}") + return True
308-325: Clarify result structure and avoid brittle double indexingdatabase_result[0][0]['uuid'] assumes a specific tuple-of-list-of-dict shape. Destructure for readability and to guard against API changes.
- database_result = await graph_driver.execute_query(find_database_query, name=database_node_name) - - # Check if database node exists - if not database_result[0]: # If no records found + db_records, _, _ = await graph_driver.execute_query(find_database_query, name=database_node_name) + # Check if database node exists + if not db_records: # If no records found print(f"Database entity node {database_node_name} not found") return False - - database_node_uuid = database_result[0][0]['uuid'] + database_node_uuid = db_records[0]['uuid']If the driver actually returns a different record shape, adjust the key access accordingly and add a small helper to normalize it.
783-787: Avoid hard-coding embedding_dim; derive from Config to prevent mismatchesTie the embedder dimension to the configured model to avoid silent shape errors if the model changes.
- config=OpenAIEmbedderConfig( - embedding_model=config.embedding_deployment, - embedding_dim=1536 - ), + config=OpenAIEmbedderConfig( + embedding_model=config.embedding_deployment, + embedding_dim=Config.EMBEDDING_MODEL.get_vector_size(), + ),
796-808: Apply the same dynamic dimension in the OpenAI fallbackKeep both branches consistent.
- # Extract just the model name without provider prefix for Graphiti - embedding_model_name = extract_embedding_model_name(Config.EMBEDDING_MODEL_NAME) + # Extract just the model name without provider prefix for Graphiti + embedding_model_name = extract_embedding_model_name(Config.EMBEDDING_MODEL_NAME) @@ - config=OpenAIEmbedderConfig( - embedding_model=embedding_model_name, - embedding_dim=1536 - ) + config=OpenAIEmbedderConfig( + embedding_model=embedding_model_name, + embedding_dim=Config.EMBEDDING_MODEL.get_vector_size(), + ) ),
5-5: Do not disable all pylint checks for this fileRepository guideline says all Python should pass pylint. Replace the global disable with targeted disables (e.g., for specific false positives) and fix violations where feasible.
Run make lint and selectively annotate only necessary disables (e.g., invalid-name, too-many-branches) rather than disable=all.
📜 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 ignored due to path filters (1)
app/public/icons/pause.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
api/memory/graphiti_tool.py(9 hunks)app/public/css/buttons.css(3 hunks)app/public/css/chat-components.css(2 hunks)app/public/css/menu.css(1 hunks)app/templates/components/chat_header.j2(2 hunks)app/templates/components/chat_input.j2(1 hunks)app/templates/components/database_modal.j2(2 hunks)app/ts/app.ts(4 hunks)app/ts/modules/chat.ts(2 hunks)app/ts/modules/config.ts(1 hunks)app/ts/modules/graph_select.ts(1 hunks)app/ts/modules/graphs.ts(0 hunks)app/ts/modules/input.ts(1 hunks)app/ts/modules/messages.ts(2 hunks)
💤 Files with no reviewable changes (1)
- app/ts/modules/graphs.ts
🧰 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/modules/messages.tsapp/ts/modules/input.tsapp/ts/modules/graph_select.tsapp/ts/modules/config.tsapp/ts/app.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 (6)
app/ts/modules/messages.ts (2)
app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/input.ts (1)
adjustTextareaHeight(7-27)
app/ts/modules/input.ts (1)
app/ts/modules/config.ts (1)
DOM(39-65)
app/ts/modules/graph_select.ts (1)
app/ts/modules/config.ts (1)
DOM(39-65)
app/ts/app.ts (4)
app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/chat.ts (1)
sendMessage(10-72)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)app/ts/modules/input.ts (1)
setupTextareaAutoResize(29-45)
api/memory/graphiti_tool.py (1)
api/config.py (2)
Config(49-135)embed(20-33)
app/ts/modules/chat.ts (4)
app/ts/modules/config.ts (2)
DOM(39-65)state(73-77)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)app/ts/modules/messages.ts (1)
addMessage(9-119)app/ts/modules/input.ts (1)
adjustTextareaHeight(7-27)
🪛 ast-grep (0.38.6)
app/ts/modules/messages.ts
[warning] 183-183: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: DOM.chatMessages.innerHTML = ""
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🪛 Ruff (0.12.2)
api/memory/graphiti_tool.py
63-63: Unused class method argument: use_direct_entities
(ARG003)
248-248: Do not catch blind exception: Exception
(BLE001)
252-252: Consider moving this statement to an else block
(TRY300)
254-254: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (16)
app/templates/components/chat_header.j2 (2)
4-4: Copy alignment LGTM.Title and “Select Database” capitalization align with the modal.
Also applies to: 14-15
6-6: No other disable flows found; refresh button is enabled on graph selection and never re-disabled.app/public/css/menu.css (1)
206-209: Flex rewrite for #custom-file-upload looks good.This cleanly replaces the background arrow; matches other flex-based headers.
app/ts/modules/config.ts (1)
40-40: No lingering HTMLInputElement usage for messageInput
Search returned no HTMLInputElement references for'message-input';messageInputis correctly typed asHTMLTextAreaElement.app/public/css/chat-components.css (1)
231-234: Pause icon theming LGTM.Explicit color ties the SVG fill to theme variables and matches the currentColor usage.
app/public/css/buttons.css (2)
3-7: Readable label color on primary button: good change.Ensures the icon/text inherit the intended theme color on primary background.
19-22: Consistent disabled states: good additions.Opacity and not-allowed cursor bring parity across inputs and actions.
Also applies to: 55-58
app/templates/components/chat_input.j2 (1)
5-13: SVG theming via currentColor: looks good.Matches CSS variables and ensures consistent theming.
Also applies to: 16-22
app/ts/modules/messages.ts (2)
7-7: Import is correct and scoped minimally.
180-183: Resetting textarea and height on init: good UX polish.app/ts/app.ts (2)
72-82: Submit enable/disable logic tied to content and selection: 👍Aligns with new disabled styles and reduces accidental submits.
103-105: Replacing alert with console.debug: sensible UX choice.app/ts/modules/chat.ts (3)
32-32: Verify CSS expects flex for pause button.Ensure the button’s CSS layout is compatible with display:flex; otherwise use the prior display value.
198-198: Verify CSS expects flex for submit button.Confirm this matches styles; if not, revert to the layout’s expected display (e.g., block/inline-flex).
8-8: Run TypeScript build and typecheck. Ensure the updated addMessage signature is applied consistently across all calls; your build will catch any mismatches.api/memory/graphiti_tool.py (1)
726-729: Using extracted model name for Azure embedder is consistent with Graphiti expectationsGood normalization step.
Summary by CodeRabbit
New Features
UI/UX
Behavior Changes