Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
- Added Backend, dropdown, cryptographically, SHA, auth, lookups to wordlist - All spellcheck errors in TOKEN_MANAGEMENT.md now resolved Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Bumps [litellm](https://github.com/BerriAI/litellm) from 1.75.9 to 1.76.0. - [Release notes](https://github.com/BerriAI/litellm/releases) - [Commits](https://github.com/BerriAI/litellm/commits) --- updated-dependencies: - dependency-name: litellm dependency-version: 1.76.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/ts/modules/chat.ts (2)
112-116: Avoid double-pushing final responses to result_historyYou push in handleStreamMessage and again inside addMessage("final-result"). Remove one.
- if (step.final_response === true) { - state.result_history.push(step.message); - }Also applies to: 170-172
224-239: Sanitize destructive confirmation UI; remove innerHTML and inline onclick (XSS risk)Build DOM nodes and attach listeners instead of injecting HTML.
- const confirmationHTML = ` - <div class="destructive-confirmation" data-confirmation-id="${confirmationId}"> - <div class="confirmation-text">${(step.message || '').replace(/\n/g, '<br>')}</div> - <div class="confirmation-buttons"> - <button class="confirm-btn danger" onclick="handleDestructiveConfirmation('CONFIRM', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> - CONFIRM - Execute Query - </button> - <button class="cancel-btn" onclick="handleDestructiveConfirmation('CANCEL', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> - CANCEL - Abort Operation - </button> - </div> - </div> - `; - - messageDiv.innerHTML = confirmationHTML; + const wrapper = document.createElement('div'); + wrapper.className = 'destructive-confirmation'; + wrapper.dataset.confirmationId = confirmationId; + + const text = document.createElement('div'); + text.className = 'confirmation-text'; + text.textContent = step.message || ''; + wrapper.appendChild(text); + + const btns = document.createElement('div'); + btns.className = 'confirmation-buttons'; + + const confirmBtn = document.createElement('button'); + confirmBtn.className = 'confirm-btn danger'; + confirmBtn.textContent = 'CONFIRM - Execute Query'; + confirmBtn.addEventListener('click', () => + handleDestructiveConfirmation('CONFIRM', step.sql_query || '', confirmationId)); + + const cancelBtn = document.createElement('button'); + cancelBtn.className = 'cancel-btn'; + cancelBtn.textContent = 'CANCEL - Abort Operation'; + cancelBtn.addEventListener('click', () => + handleDestructiveConfirmation('CANCEL', step.sql_query || '', confirmationId)); + + btns.appendChild(confirmBtn); + btns.appendChild(cancelBtn); + wrapper.appendChild(btns); + + messageDiv.replaceChildren(wrapper);api/routes/graphs.py (2)
387-447: Destructive-op confirmation must not rely on the first token. Use the loader’s analysis.WITH … DELETE/UPDATE and commented queries bypass the current check and execute without confirmation. Gate execution using loader_class.is_schema_modifying_query before running anything.
- if answer_an["is_sql_translatable"]: - # Check if this is a destructive operation that requires confirmation - sql_query = answer_an["sql_query"] - sql_type = sql_query.strip().split()[0].upper() if sql_query else "" - - destructive_ops = ['INSERT', 'UPDATE', 'DELETE', 'DROP', - 'CREATE', 'ALTER', 'TRUNCATE'] - if sql_type in destructive_ops: + if answer_an["is_sql_translatable"]: + sql_query = answer_an["sql_query"] + # Use loader to detect schema-modifying ops (handles WITH … DML, comments, etc.) + is_schema_modifying, operation_type = loader_class.is_schema_modifying_query(sql_query) + if is_schema_modifying: # This is a destructive operation - ask for user confirmation - confirmation_message = f"""⚠️ DESTRUCTIVE OPERATION DETECTED ⚠️ + confirmation_message = f"""⚠️ DESTRUCTIVE OPERATION DETECTED ⚠️ -The generated SQL query will perform a **{sql_type}** operation: +The generated SQL query will perform a **{operation_type}** operation: SQL: {sql_query} @@ - if sql_type == 'INSERT': + if operation_type.upper().startswith('INSERT'): confirmation_message += "• Add new data to the database" - elif sql_type == 'UPDATE': + elif operation_type.upper().startswith('UPDATE'): confirmation_message += ("• Modify existing data in the " "database") - elif sql_type == 'DELETE': + elif operation_type.upper().startswith('DELETE'): confirmation_message += ("• **PERMANENTLY DELETE** data " "from the database") - elif sql_type == 'DROP': + elif operation_type.upper().startswith('DROP'): confirmation_message += ("• **PERMANENTLY DELETE** entire " "tables or database objects") - elif sql_type == 'CREATE': + elif operation_type.upper().startswith('CREATE'): confirmation_message += ("• Create new tables or database " "objects") - elif sql_type == 'ALTER': + elif operation_type.upper().startswith('ALTER'): confirmation_message += ("• Modify the structure of existing " "tables") - elif sql_type == 'TRUNCATE': + elif operation_type.upper().startswith('TRUNCATE'): confirmation_message += ("• **PERMANENTLY DELETE ALL DATA** " "from specified tables") @@ return # Stop here and wait for user confirmation
459-467: Don’t block the event loop when executing SQL.If execute_sql_query is sync, run it in a thread to keep streaming responsive.
- query_results = loader_class.execute_sql_query(answer_an["sql_query"], db_url) + query_results = await asyncio.to_thread( + loader_class.execute_sql_query, answer_an["sql_query"], db_url + )
♻️ Duplicate comments (4)
app/ts/modules/messages.ts (1)
61-87: Wrap case in a block, guard empty results, and fix column alignment order
- Variables declared in this switch clause leak across cases (Biome noSwitchDeclarations).
- Accessing queryResult[0] without validation can throw.
- Rendering rows via Object.values() can misalign cells; derive order from header keys.
- case "query-final-result": - messageDivContainer.className += " final-result-message-container"; - messageDiv.className += " final-result-message"; - messageDiv.style.overflow = "auto"; - const table = document.createElement("table"); - table.id = "query-final-result-table"; - const tableHeader = document.createElement("thead"); - const headerRow = document.createElement("tr"); - Object.keys(queryResult[0]).forEach((column: any) => { - const headerCell = document.createElement("th"); - headerCell.textContent = column; - headerRow.appendChild(headerCell); - }); - tableHeader.appendChild(headerRow); - table.appendChild(tableHeader); - const tableBody = document.createElement("tbody"); - queryResult.forEach((row: any, index: number) => { - const rowRow = document.createElement("tr"); - Object.values(row).forEach((value: any) => { - const cell = document.createElement("td"); - cell.textContent = value; - rowRow.appendChild(cell); - }); - tableBody.appendChild(rowRow); - }); - table.appendChild(tableBody); - messageDiv.appendChild(table); - break; + case "query-final-result": { + messageDivContainer.className += " final-result-message-container"; + messageDiv.className += " final-result-message"; + messageDiv.style.overflow = "auto"; + + if (!Array.isArray(queryResult) || queryResult.length === 0) { + messageDiv.textContent = "No results to display"; + break; + } + + const table = document.createElement("table"); + table.id = "query-final-result-table"; + + const tableHeader = document.createElement("thead"); + const headerRow = document.createElement("tr"); + const headerKeys = Object.keys(queryResult[0]); + headerKeys.forEach((column) => { + const headerCell = document.createElement("th"); + headerCell.textContent = column; + headerRow.appendChild(headerCell); + }); + tableHeader.appendChild(headerRow); + table.appendChild(tableHeader); + + const tableBody = document.createElement("tbody"); + for (const row of queryResult) { + const rowRow = document.createElement("tr"); + for (const key of headerKeys) { + const cell = document.createElement("td"); + const value = (row as Record<string, unknown>)[key]; + cell.textContent = value != null ? String(value) : ""; + rowRow.appendChild(cell); + } + tableBody.appendChild(rowRow); + } + table.appendChild(tableBody); + messageDiv.appendChild(table); + break; + }app/ts/modules/ui.ts (1)
192-201: Add null-guards to avoid runtime crashes when modal not presentCurrent non-null casts can throw.
-export function setupCustomDropdown() { - const dropdown = document.getElementById( - "database-type-dropdown" - ) as HTMLElement; - const selected = dropdown?.querySelector(".dropdown-selected") as HTMLElement; - const options = dropdown?.querySelector(".dropdown-options") as HTMLElement; - const hiddenInput = document.getElementById( - "database-type-select" - ) as HTMLInputElement; +export function setupCustomDropdown() { + const dropdown = document.getElementById("database-type-dropdown"); + if (!dropdown) return; + const selected = dropdown.querySelector(".dropdown-selected") as HTMLElement | null; + const options = dropdown.querySelector(".dropdown-options") as HTMLElement | null; + const hiddenInput = document.getElementById("database-type-select") as HTMLInputElement | null; + if (!selected || !options || !hiddenInput) return;app/ts/modules/chat.ts (1)
185-190: Fix empty-array handling for query results[] is truthy; current code will crash rendering table headers.
-function handleQueryResult(step: any) { - if (step.data) { - addMessage("Query Result", "query-final-result", false, null, step.data); - } else { - addMessage('No results found for the query.'); - } +function handleQueryResult(step: any) { + if (Array.isArray(step.data) && step.data.length > 0) { + addMessage("Query Result", "query-final-result", false, null, step.data); + } else { + addMessage('No results found for the query.', "followup"); + } }api/routes/graphs.py (1)
537-550: Use logging.exception and keep the streamed message generic.Same as earlier feedback; include traceback in logs but not in client output.
- except Exception as e: # pylint: disable=broad-exception-caught - execution_error = str(e) + except Exception as e: # pylint: disable=broad-exception-caught + execution_error = str(e) overall_elapsed = time.perf_counter() - overall_start - logging.error("Error executing SQL query: %s", str(e)) # nosemgrep + logging.exception("Error executing SQL query") @@ - yield json.dumps({ - "type": "error", - "final_response": True, - "message": "Error executing SQL query" - }) + MESSAGE_DELIMITER + yield json.dumps({ + "type": "error", + "final_response": True, + "message": "Error executing SQL query" + }) + MESSAGE_DELIMITER
🧹 Nitpick comments (21)
app/ts/modules/config.ts (1)
40-41: Verify element type: message input appears to be multi-lineCSS sets overflow/resize for #message-input like a textarea. If #message-input is a <textarea>, update the typing for accuracy.
Proposed change:
- messageInput: getElement<HTMLInputElement | null>('message-input'), + messageInput: getElement<HTMLTextAreaElement | null>('message-input'),app/public/css/chat-components.css (1)
206-218: Hidden scrollbars + fixed height: confirm UX and element type
- If #message-input is an , these rules won’t have effect (inputs aren’t multi-line). If it’s a <textarea>, they apply as expected—please verify.
- Hiding scrollbars while allowing overflow can reduce discoverability. Consider showing a thin scrollbar or auto-expanding up to max-height, then scrolling.
Optional adjustments:
#message-input { - resize: none; - min-height: 24px; - height: 24px; - max-height: calc(24px * 5); - overflow-y: auto; + resize: none; + min-height: 24px; + height: auto; + max-height: calc(24px * 5); + overflow-y: auto; line-height: 24px; - scrollbar-width: none; - -ms-overflow-style: none; + scrollbar-width: thin; + -ms-overflow-style: auto; } -#message-input::-webkit-scrollbar { display: none; } +/* Or prefer a thin scrollbar instead of none */ +/* #message-input::-webkit-scrollbar { width: 6px; } */app/public/css/base.css (1)
14-17: Use modern font formats and enable swapTTF is heavy; prefer WOFF2 and add font-display: swap to avoid FOIT.
@font-face { font-family: 'Inter'; - src: url('fonts/inter.ttf'); + src: url('fonts/inter.woff2') format('woff2'), + url('fonts/inter.woff') format('woff'); + font-display: swap; }app/ts/modules/messages.ts (3)
39-47: Improve avatar alt text for accessibilityUse the user’s name (or a descriptive fallback), not just the first initial.
- userAvatar.alt = - (userInfo.name?.charAt(0).toUpperCase() as string) || "User"; + userAvatar.alt = userInfo.name || "User avatar";
179-183: Avoid innerHTML for clearing to quiet XSS lintersSetting empty HTML is safe here but flagged. Prefer replaceChildren() or textContent = "".
- if (DOM.chatMessages) DOM.chatMessages.innerHTML = ""; + if (DOM.chatMessages) DOM.chatMessages.replaceChildren(); [DOM.confValue, DOM.expValue, DOM.missValue].forEach((element) => { - if (element) element.innerHTML = ""; + if (element) element.textContent = ""; });
153-163: Array block detection is too looseAny text with brackets is treated as an array, which can misformat content. Make detection strict or parse JSON.
- if (text.includes("[") && text.includes("]")) { - const parts = text.split("["); - const formattedParts = parts.map((part) => { - const lineDiv = document.createElement("div"); - part = part.replace(/\]/g, ""); - lineDiv.textContent = part; - return lineDiv; - }); - return formattedParts; - } + if (/^\s*\[.*\]\s*$/s.test(text)) { + try { + const arr = JSON.parse(text); + if (Array.isArray(arr)) { + return arr.map((item) => { + const lineDiv = document.createElement("div"); + lineDiv.textContent = String(item); + return lineDiv; + }); + } + } catch { + // fall through to generic handling + } + }app/public/css/menu.css (2)
345-353: Deduplicate .dropdown-option stylesSame selector defined twice; keep one to avoid drift.
-.dropdown-option { - padding: 8px 12px; - cursor: pointer; - display: flex; - align-items: center; - gap: 8px; - transition: background-color 0.2s ease; -}Also applies to: 420-428
95-106: Avoid redefining .header-button globallyTwo blocks target .header-button with different purposes (button vs. dropdown container). Consider scoping the second to .custom-dropdown .header-button (or a new class) to prevent unintended layout on regular buttons.
Also applies to: 243-247
app/ts/app.ts (1)
45-49: Safer/faster clearing of containerAvoid innerHTML assignment; use replaceChildren().
- const container = document.getElementById("schema-graph"); - if (container) container.innerHTML = ""; + const container = document.getElementById("schema-graph"); + if (container) container.replaceChildren();app/ts/modules/ui.ts (2)
189-190: Prefer rAF over timeout for resize after layout changesSchedules after paint reliably.
- setTimeout(resizeGraph, 450); + requestAnimationFrame(() => resizeGraph());
220-226: Avoid innerHTML even when clearing; use textContent=''Removes unnecessary HTML parsing and silences scanners.
- if (dropdownText) dropdownText.innerHTML = ""; + if (dropdownText) dropdownText.textContent = "";app/ts/modules/chat.ts (1)
57-66: Error message should use followup type for consistencyUse the new category to keep styling consistent with other followups.
- addMessage('Sorry, there was an error processing your message: ' + (error.message || String(error))); + addMessage('Sorry, there was an error processing your message: ' + (error.message || String(error)), "followup");api/routes/graphs.py (9)
44-46: Rely on Pydantic validation; drop hasattr guards and tighten types.Since ChatRequest guarantees fields, the hasattr checks are redundant. Simplify and type-narrow.
-queries_history = chat_data.chat if hasattr(chat_data, 'chat') else None -result_history = chat_data.result if hasattr(chat_data, 'result') else None -instructions = chat_data.instructions if hasattr(chat_data, 'instructions') else None +queries_history: list[str] = chat_data.chat +result_history: list[str] | None = chat_data.result +instructions: str | None = chat_data.instructionsAlso applies to: 264-267
121-132: Harden error handling in database_schema: use logging.exception and avoid silent excepts.Improve observability; keep user-facing messages generic. Log skipped-row shape at debug.
- except Exception as e: # pylint: disable=broad-exception-caught - logging.error("Failed to select graph %s: %s", sanitize_log_input(namespaced), e) + except Exception: # pylint: disable=broad-exception-caught + logging.exception("Failed to select graph %s", sanitize_log_input(namespaced)) return JSONResponse(content={"error": "Graph not found or database error"}, status_code=404) @@ - except Exception as e: # pylint: disable=broad-exception-caught - logging.error("Error querying graph data for %s: %s", sanitize_log_input(namespaced), e) + except Exception: # pylint: disable=broad-exception-caught + logging.exception("Error querying graph data for %s", sanitize_log_input(namespaced)) return JSONResponse(content={"error": "Failed to read graph data"}, status_code=500) @@ - except Exception: # pylint: disable=broad-exception-caught - continue + except Exception: # pylint: disable=broad-exception-caught + logging.debug("Skipping malformed table row: %r", row, exc_info=True) + continue @@ - except Exception: # pylint: disable=broad-exception-caught - continue + except Exception: # pylint: disable=broad-exception-caught + logging.debug("Skipping malformed link row: %r", row, exc_info=True) + continueAlso applies to: 136-139, 157-160, 165-195, 205-208
220-247: FastAPI’s File(None) triggers B008; keep it but silence the linter.This is the idiomatic FastAPI pattern. Add a noqa.
-async def load_graph(request: Request, data: GraphData = None, file: UploadFile = File(None)): # pylint: disable=unused-argument +async def load_graph(request: Request, data: GraphData = None, file: UploadFile = File(None)): # pylint: disable=unused-argument # noqa: B008
287-295: Create MemoryTool only when needed (inside the generator and after on-topic check).Avoid unnecessary work if the request is off-topic or fails early.
- memory_tool_task = asyncio.create_task(MemoryTool.create(request.state.user_id, graph_id)) @@ - memory_tool = await memory_tool_task + memory_tool_task = asyncio.create_task(MemoryTool.create(request.state.user_id, graph_id)) + memory_tool = await memory_tool_taskAlso applies to: 356-362
331-352: Return after streaming off-topic response.Prevents extra work and accidental fall-through.
logging.info("Query processing completed (off-topic) - Total time: %.2f seconds", overall_elapsed) + return
373-385: Avoid logging raw SQL; sanitize/truncate before logging.Reduces exposure of sensitive data in logs.
- logging.info("Generated SQL query: %s", answer_an['sql_query']) # nosemgrep + logging.info("Generated SQL query: %s", sanitize_query(answer_an.get('sql_query', ''))) # nosemgrep
602-605: Improve task callbacks: include exc_info to capture tracebacks.Logging error strings alone loses stack traces.
-save_query_task.add_done_callback( - lambda t: logging.error("Query memory save failed: %s", t.exception()) # nosemgrep - if t.exception() else logging.info("Query memory saved successfully") -) +save_query_task.add_done_callback( + lambda t: (logging.error("Query memory save failed", exc_info=t.exception()) # nosemgrep + if t.exception() else logging.info("Query memory saved successfully")) +) @@ -save_task.add_done_callback( - lambda t: logging.error("Memory save failed: %s", t.exception()) # nosemgrep - if t.exception() else logging.info("Conversation saved to memory tool") -) +save_task.add_done_callback( + lambda t: (logging.error("Memory save failed", exc_info=t.exception()) # nosemgrep + if t.exception() else logging.info("Conversation saved to memory tool")) +) @@ -clean_memory_task.add_done_callback( - lambda t: logging.error("Memory cleanup failed: %s", t.exception()) # nosemgrep - if t.exception() else logging.info("Memory cleanup completed successfully") -) +clean_memory_task.add_done_callback( + lambda t: (logging.error("Memory cleanup failed", exc_info=t.exception()) # nosemgrep + if t.exception() else logging.info("Memory cleanup completed successfully")) +)Also applies to: 610-613, 618-620
659-676: Use logging.exception in confirm flow and keep client message generic; also improve callbacks.Preserve traceback in logs; keep streamed text neutral.
- step = {"type": "reasoning_step", - "message": "Step 2: Executing confirmed SQL query"} + step = {"type": "reasoning_step", + "message": "Step 2: Executing confirmed SQL query"} @@ - except Exception as e: # pylint: disable=broad-exception-caught - logging.error("Error executing confirmed SQL query: %s", str(e)) # nosemgrep + except Exception: # pylint: disable=broad-exception-caught + logging.exception("Error executing confirmed SQL query") # nosemgrep @@ - save_query_task.add_done_callback( - lambda t: logging.error( # nosemgrep - "Failed confirmed query memory save failed: %s", t.exception() - ) if t.exception() else logging.info( - "Failed confirmed query memory saved successfully" - ) - ) + save_query_task.add_done_callback( + lambda t: (logging.error("Failed confirmed query memory save failed", + exc_info=t.exception()) # nosemgrep + if t.exception() else logging.info( + "Failed confirmed query memory saved successfully")) + )Also applies to: 756-775
803-820: Refresh endpoint via connect_database: good reuse; tighten error handling.Use logging.exception and raise from to preserve tracebacks.
- except Exception as e: - logging.error("Error in refresh_graph_schema: %s", str(e)) - raise HTTPException(status_code=500, detail="Internal server error while refreshing schema") # pylint: disable=raise-missing-from + except Exception as e: + logging.exception("Error in refresh_graph_schema") + raise HTTPException(status_code=500, + detail="Internal server error while refreshing schema") from e
📜 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/css/fonts/inter.ttfis excluded by!**/*.ttf
📒 Files selected for processing (14)
api/routes/graphs.py(23 hunks)app/public/css/base.css(1 hunks)app/public/css/buttons.css(2 hunks)app/public/css/chat-components.css(1 hunks)app/public/css/menu.css(7 hunks)app/public/css/modals.css(4 hunks)app/public/css/responsive.css(13 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(10 hunks)app/ts/modules/config.ts(1 hunks)app/ts/modules/messages.ts(1 hunks)app/ts/modules/ui.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/templates/components/chat_input.j2
🚧 Files skipped from review as they are similar to previous changes (4)
- app/public/css/modals.css
- app/public/css/responsive.css
- app/templates/components/chat_header.j2
- app/public/css/buttons.css
🧰 Additional context used
📓 Path-based instructions (2)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/ts/app.tsapp/ts/modules/config.tsapp/public/css/chat-components.cssapp/ts/modules/chat.tsapp/public/css/base.cssapp/ts/modules/messages.tsapp/ts/modules/ui.tsapp/public/css/menu.css
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/graphs.py
🧬 Code graph analysis (5)
app/ts/app.ts (6)
app/ts/modules/schema.ts (1)
resizeGraph(228-235)app/ts/modules/messages.ts (1)
initChat(178-194)app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)app/ts/modules/graphs.ts (2)
onGraphChange(225-227)loadGraphs(15-164)app/ts/modules/tokens.ts (1)
setupTokenManagement(15-146)
app/ts/modules/chat.ts (2)
app/ts/modules/messages.ts (2)
addMessage(8-118)moveLoadingMessageToBottom(129-138)app/ts/modules/config.ts (2)
state(73-77)DOM(39-65)
app/ts/modules/messages.ts (2)
app/ts/modules/config.ts (2)
state(73-77)DOM(39-65)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
app/ts/modules/ui.ts (3)
app/ts/modules/config.ts (1)
DOM(39-65)app/ts/modules/messages.ts (1)
initChat(178-194)app/ts/modules/schema.ts (1)
resizeGraph(228-235)
api/routes/graphs.py (2)
api/routes/database.py (3)
connect_database(33-137)DatabaseConnectionRequest(21-29)generate(47-135)api/graph.py (1)
get_db_description(39-54)
🪛 ast-grep (0.38.6)
app/ts/app.ts
[warning] 45-45: 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: container.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)
app/ts/modules/messages.ts
[warning] 179-179: 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)
[warning] 181-181: 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: element.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)
app/ts/modules/ui.ts
[warning] 222-222: 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: dropdownText.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)
🪛 Biome (2.1.2)
app/ts/modules/messages.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 67-67: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 75-75: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
app/public/css/menu.css
[error] 465-465: expected } but instead the file ends
the file ends here
(parse)
🪛 Ruff (0.12.2)
api/routes/graphs.py
136-136: Do not catch blind exception: Exception
(BLE001)
157-157: Do not catch blind exception: Exception
(BLE001)
158-158: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
165-165: Do not catch blind exception: Exception
(BLE001)
193-194: try-except-continue detected, consider logging the exception
(S112)
193-193: Do not catch blind exception: Exception
(BLE001)
207-208: try-except-continue detected, consider logging the exception
(S112)
207-207: Do not catch blind exception: Exception
(BLE001)
220-220: Unused function argument: request
(ARG001)
220-220: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
537-537: Do not catch blind exception: Exception
(BLE001)
540-540: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
756-756: Do not catch blind exception: Exception
(BLE001)
757-757: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
770-772: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
807-807: Abstract raise to an inner function
(TRY301)
817-817: Do not catch blind exception: Exception
(BLE001)
818-818: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
819-819: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ 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). (4)
- GitHub Check: deploy
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (4)
app/ts/modules/config.ts (1)
61-61: LGTM: New DOM accessor matches new header buttongraphSelectRefresh is correctly typed and named; aligns with the new "graph-select-refresh" control.
app/public/css/base.css (1)
23-25: LGTM: Preserve monospace for query text.query-text override correctly restores Fira Code.
api/routes/graphs.py (2)
108-119: LGTM: public API list_databases and namespacing are clear and safe.Filtering by the user_id_ prefix and stripping it server-side is good.
841-843: LGTM: delete handler logs with traceback and returns clear errors.Good use of logging.exception and specific ResponseError handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/public/css/menu.css (1)
130-133: Closing brace fixed — parsing unblockedThe missing brace reported earlier is now present. Good catch.
🧹 Nitpick comments (5)
app/public/css/menu.css (5)
95-118: Unify border token usage for header buttonsYou mix
--border-colorhere and--falkor-border-*elsewhere. Prefer one token family for consistency.- border: 1px solid var(--border-color); + border: 1px solid var(--falkor-border-primary, var(--border-color));Also consider mirroring this change in
.dropdown-selectedfor consistency.
244-249: Avoid duplicate.header-buttondeclarations; scope the second ruleTwo
.header-buttonblocks can confuse cascade. Scope this one to dropdown context.-.header-button { +.custom-dropdown .header-button { position: relative; display: inline-block; min-width: 140px; }
149-150: Use border token instead of text color for bordersBorders tied to
--text-primarycan look heavy in themes.- border-top: 1px solid var(--text-primary); + border-top: 1px solid var(--falkor-border-secondary, var(--text-primary));
320-336: Merge duplicate rules for#database-type-dropdown .dropdown-selectedTwo adjacent blocks target the same selector; combine to reduce cascade complexity.
-#database-type-dropdown .dropdown-selected { - width: 100%; - /* keep flex layout so text and arrow align on one line */ - display: flex; - align-items: center; - justify-content: space-between; -} - -/* Match modal input sizing for a consistent look */ -#database-type-dropdown .dropdown-selected { - padding: 0.6em; - font-size: 1em; - border-radius: 6px; -} +#database-type-dropdown .dropdown-selected { + width: 100%; + display: flex; + align-items: center; + justify-content: space-between; + padding: 0.6em; + font-size: 1em; + border-radius: 6px; +}Also applies to: 338-344
406-419: Reconsider dropdown positioning to avoid horizontal overflow
left: -20%andright: -20%can overflow small containers. Prefer intrinsic width with clamped max.-.graph-options { - top: calc(100% + 20px); - left: -20%; - right: -20%; +.graph-options { + top: calc(100% + 8px); + left: 0; + right: auto; + min-width: 100%; + max-width: min(480px, 90vw); + transform: translateX(0); }If centering is desired:
left: 50%; transform: translate(-50%, 0);
📜 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 (1)
app/public/css/menu.css(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/public/css/menu.css
⏰ 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). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (7)
app/public/css/menu.css (7)
135-147: Table styles look goodCompact cell padding and column dividers are clear and consistent.
164-176: Good pointer-events pattern for overlay controlsContainer blocks hits; buttons re-enable interactions. Solid approach.
Also applies to: 178-187
306-311: Dropdown open-state mechanics look cleanSimple
.opentoggle and arrow rotation — straightforward and maintainable.Also applies to: 316-318
345-349: Ensure options width matches triggerLeft/right 0 is correct for full-width dropdown; keep an eye on parent padding to avoid misalignment.
381-386: Graph header button/trigger styles — LGTMFixed width and spacing create a stable layout; gap on
.graph-selectedis a nice touch.Also applies to: 389-404
421-424:.graph-custom-dropdownis in use
Referenced inapp/templates/components/chat_header.j2(line 12), so the CSS block is still needed.Likely an incorrect or invalid review comment.
125-128: Do not add missing @Keyframes —spinis already defined globally
Keyframes forspinexist in app/public/css/animations.css (lines 42–45); no local redefinition needed.Likely an incorrect or invalid review comment.
Fix #181 replace fonts
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/ts/modules/messages.ts (1)
60-87: Fix scope leak, empty-result crash, column-order bug, and duplicate ID in query-final-result case
- Declarations inside switch must be block-scoped; wrap the case.
- Guard against empty/invalid queryResult to avoid runtime errors.
- Ensure row cell order matches header keys (Object.values can misalign).
- Avoid reusing a fixed table ID; use a class instead.
Apply:
- case "query-final-result": - messageDivContainer.className += " final-result-message-container"; - messageDiv.className += " final-result-message"; - messageDiv.style.overflow = "auto"; - const table = document.createElement("table"); - table.id = "query-final-result-table"; - const tableHeader = document.createElement("thead"); - const headerRow = document.createElement("tr"); - Object.keys(queryResult[0]).forEach((column: any) => { - const headerCell = document.createElement("th"); - headerCell.textContent = column; - headerRow.appendChild(headerCell); - }); - tableHeader.appendChild(headerRow); - table.appendChild(tableHeader); - const tableBody = document.createElement("tbody"); - queryResult.forEach((row: any, index: number) => { - const rowRow = document.createElement("tr"); - Object.values(row).forEach((value: any) => { - const cell = document.createElement("td"); - cell.textContent = value; - rowRow.appendChild(cell); - }); - tableBody.appendChild(rowRow); - }); - table.appendChild(tableBody); - messageDiv.appendChild(table); - break; + case "query-final-result": { + messageDivContainer.classList.add("final-result-message-container"); + messageDiv.classList.add("final-result-message"); + messageDiv.style.overflow = "auto"; + + if ( + !Array.isArray(queryResult) || + queryResult.length === 0 || + typeof queryResult[0] !== "object" || + queryResult[0] === null + ) { + messageDiv.textContent = "No results to display"; + break; + } + + const table = document.createElement("table"); + table.classList.add("query-final-result-table"); + const tableHeader = document.createElement("thead"); + const headerRow = document.createElement("tr"); + const headerKeys = Object.keys(queryResult[0] as Record<string, unknown>); + headerKeys.forEach((column) => { + const headerCell = document.createElement("th"); + headerCell.textContent = column; + headerRow.appendChild(headerCell); + }); + tableHeader.appendChild(headerRow); + table.appendChild(tableHeader); + + const tableBody = document.createElement("tbody"); + (queryResult as Array<Record<string, unknown>>).forEach((row) => { + const rowRow = document.createElement("tr"); + headerKeys.forEach((key) => { + const cell = document.createElement("td"); + const value = (row as any)[key]; + cell.textContent = value != null ? String(value) : ""; + rowRow.appendChild(cell); + }); + tableBody.appendChild(rowRow); + }); + table.appendChild(tableBody); + messageDiv.appendChild(table); + break; + }
🧹 Nitpick comments (4)
app/ts/modules/messages.ts (4)
96-96: Avoid duplicating formatted text for query-final-resultPrevent formatBlock from adding extra lines when a table is rendered.
- const block = formatBlock(message); + const block = type !== "query-final-result" ? formatBlock(message) : null;
39-46: Harden avatar handling and improve alt text
- Validate picture URL scheme (http/https or protocol-relative) to reduce injection/tracking risk.
- Use the user’s name as alt text instead of a single initial.
- if (userInfo && userInfo.picture) { - userAvatar = document.createElement("img"); - userAvatar.src = userInfo.picture; - userAvatar.alt = - (userInfo.name?.charAt(0).toUpperCase() as string) || "User"; - userAvatar.className = "user-message-avatar"; - messageDivContainer.classList.add("has-avatar"); - } + if (userInfo && userInfo.picture) { + const pic = userInfo.picture; + if (typeof pic === "string" && /^(https?:)?\/\//i.test(pic)) { + userAvatar = document.createElement("img"); + userAvatar.src = pic; + userAvatar.alt = userInfo.name ?? "User"; + userAvatar.className = "user-message-avatar"; + messageDivContainer.classList.add("has-avatar"); + } + }
133-136: Don’t remove before re-appending; appendChild moves the nodeMinor perf/DOM churn improvement.
- loadingMessageContainer.remove(); - DOM.chatMessages.appendChild(loadingMessageContainer); + DOM.chatMessages.appendChild(loadingMessageContainer);
179-183: Prefer textContent when clearing containersSilences XSS linters and avoids unnecessary HTML parsing.
- if (DOM.chatMessages) DOM.chatMessages.innerHTML = ""; + if (DOM.chatMessages) DOM.chatMessages.textContent = ""; [DOM.confValue, DOM.expValue, DOM.missValue].forEach((element) => { - if (element) element.innerHTML = ""; + if (element) element.textContent = ""; });
📜 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 (1)
app/ts/modules/messages.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/ts/modules/messages.ts
🧬 Code graph analysis (1)
app/ts/modules/messages.ts (2)
app/ts/modules/config.ts (2)
state(73-77)DOM(39-65)app/ts/modules/graph_select.ts (1)
getSelectedGraph(7-12)
🪛 Biome (2.1.2)
app/ts/modules/messages.ts
[error] 64-64: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 67-67: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 75-75: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 ast-grep (0.38.6)
app/ts/modules/messages.ts
[warning] 179-179: 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)
[warning] 181-181: 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: element.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)
⏰ 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). (5)
- GitHub Check: deploy
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
Fix #180 fix nodes collision
add support for user/password
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (15)
tests/e2e/test_file_upload.py (7)
47-51: Replace no-op assertions with real checks; fail if upload is unavailable for authenticated users.These
assert Truestatements make the test meaningless.- # Test passes if upload was processed (success or appropriate error) - assert True, f"File upload processed: {len(success_messages)} success, {len(error_messages)} errors" + # Expect some upload feedback (success or error) + assert (len(success_messages) + len(error_messages)) > 0, \ + f"Expected upload feedback, got {len(success_messages)} success, {len(error_messages)} errors" else: - assert True, f"File upload interface: visible={is_visible}, enabled={is_enabled}" + pytest.fail(f"File upload should be available for authenticated users: visible={is_visible}, enabled={is_enabled}")
75-76: Assert expected access differences between auth/unauth users.Documenting with
assert Truehides regressions.- # Document the difference - assert True, f"Upload availability - Authenticated: {auth_upload_available}, Unauthenticated: {unauth_upload_available}" + # Authenticated users should have access; unauthenticated should not + assert bool(auth_upload_available), "Authenticated users should have access to file upload" + assert not bool(unauth_upload_available), "Unauthenticated users should not have access to file upload"
99-103: Make unauthenticated interface expectations explicit.The test should fail if unauthenticated users can use upload.
- # The test passes if the UI behaves predictably - assert True, f"File upload interface found: visible={is_visible}, enabled={is_enabled}" + # For unauthenticated users, upload should be hidden or disabled + assert (not is_visible) or (not is_enabled), \ + f"File upload should be restricted for unauthenticated users: visible={is_visible}, enabled={is_enabled}" else: - # No file input found - this might be expected for unauthenticated users - assert True, "File upload interface not available (expected for unauthenticated users)" + # No file input found is expected for unauthenticated users + pass
124-131: Avoid broad exception and assert real outcome after clicking schema button.Catching
Exceptionand assertingTruemasks failures.- # After clicking, check what happens - # Might show login prompt, upload dialog, or error message - assert True, "Schema button clicked successfully" - except Exception as e: - # Expected behavior - button might require authentication - assert True, f"Schema button interaction handled: {e}" + # After clicking, expect an auth prompt for unauthenticated users + login_prompt = page.query_selector("*:text('login'), *:text('authenticate')") + assert login_prompt is not None, "Expected authentication prompt after clicking schema button" + except PlaywrightTimeoutError: + # Timeout likely indicates gated/disabled action; acceptable for unauthenticated users + pass @@ - assert True, f"Found {len(upload_buttons)} upload-related UI elements" + print(f"Found {len(upload_buttons)} upload-related UI elements")
159-167: Validate unauthenticated upload is blocked and don’t swallow unexpected errors.Replace placeholder assertions and broad exception with specific handling.
- # Test passes if appropriate messaging is shown - assert True, f"File upload attempted: {len(error_messages)} errors, {len(login_prompts)} login prompts" + # Expect error messages or authentication prompts + assert len(error_messages) > 0 or len(login_prompts) > 0, \ + "Expected error messages or login prompts for unauthenticated upload attempt" - - except Exception as e: - # Expected - file upload should fail gracefully for unauthenticated users - assert True, f"File upload properly restricted: {e}" + except PlaywrightTimeoutError: + # Expected: action blocked/gated + pass else: - # File input not available - expected for unauthenticated users - assert True, "File upload interface properly hidden from unauthenticated users" + # File input not available is expected for unauthenticated users + pass
198-204: Assert presence of auth prompts and use a single definitive page-load check.Eliminate
assert Trueand overly-permissiveorcondition.- # Test documents the current user experience - assert True, ( - f"Auth prompts: {len(login_buttons)} buttons, {len(auth_messages)} messages" - ) - assert "QueryWeaver" in page_title or current_url.endswith("/"), ( - "Page loaded successfully" - ) + # Verify authentication prompts are present + assert len(login_buttons) > 0 or len(auth_messages) > 0, \ + f"Expected authentication prompts for unauthenticated users, found {len(login_buttons)} buttons, {len(auth_messages)} messages" + # Verify page loaded correctly + assert "QueryWeaver" in page_title, f"Expected 'QueryWeaver' in page title, got: {page_title} (url: {current_url})"
221-233: Replace documentation-only assertion; ensure unauthenticated UI is actually restricted and tighten page-load check.These assertions currently cannot fail.
- # Test passes regardless - this documents the current UI state - assert True, f"Upload UI elements available: {available_elements}" + # Log for debugging + print(f"Upload UI elements available: {available_elements}") + # Verify upload is restricted for unauthenticated users + assert ( + available_elements["file_inputs"] == 0 + or not available_elements["upload_button"] + or not available_elements["schema_button"] + ), f"File upload UI should be restricted for unauthenticated users: {available_elements}" @@ - assert "QueryWeaver" in page.title() or page.url.endswith("/"), "Page loaded successfully" + assert "QueryWeaver" in page.title(), f"Unexpected page title: {page.title()} (url: {page.url})"api/routes/database.py (2)
82-82: Tuple assign: avoid list destructuring (lint/style)Use a tuple (or separate assigns) instead of a list to satisfy linters and readability.
- success, result = [False, ""] + success, result = False, ""
135-135: CodeQL: potential information exposure via streaming errorsEnsure no stack traces leak to clients if the generator fails late. You already guard most paths; confirm app runs with debug disabled and add a global exception handler if not present.
I can draft a FastAPI exception handler to enforce generic JSON errors app-wide if you want.
api/auth/user_management.py (3)
199-226: Prefer Authorization: Bearer, then cookie, then query; remove broad try/exceptHeader-first reduces CSRF risk; parsing is deterministic without exceptions. Also matches prior feedback.
- # Check cookies - api_token = request.cookies.get("api_token") - if api_token: - return api_token - - # Check query parameters - api_token = request.query_params.get("api_token") - if api_token: - return api_token - - # Check Authorization header - auth_header = request.headers.get("authorization") or request.headers.get( - "Authorization" - ) - if auth_header: - try: - parts = auth_header.split(None, 1) - if len(parts) == 2 and parts[0].lower() == "bearer": - return parts[1].strip() - except Exception: # pylint: disable=broad-exception-caught - pass - - return None + # Prefer Authorization: Bearer <token>, then cookie, then query param + auth_header = request.headers.get("authorization") or request.headers.get("Authorization") + if auth_header: + scheme, _, token = auth_header.partition(" ") + if scheme.lower() == "bearer" and token.strip(): + return token.strip() + api_token = request.cookies.get("api_token") + if api_token: + return api_token + return request.query_params.get("api_token")
268-273: Guard missing email and specify encodingAvoid AttributeError if email is None; be explicit on UTF-8.
- email = user_info.get("email") - request.state.user_id = base64.b64encode(email.encode()).decode() - request.state.user_email = email + email = user_info.get("email") + if not email: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Unauthorized - Missing email on user", + ) + request.state.user_id = base64.b64encode(email.encode("utf-8")).decode("utf-8") + request.state.user_email = email
354-360: Prevent token ownership hijacking on MERGEDon’t attach an existing token to a different identity. Add an ownership check before MERGE-ing the relationship. Also enforce uniqueness via migration.
- MERGE (token:Token {id: $api_token}) - ON CREATE SET - token.created_at = timestamp(), - token.expires_at = timestamp() + 86400000 // 24h expiry - MERGE (identity)-[:HAS_TOKEN]->(token) + MERGE (token:Token {id: $api_token}) + ON CREATE SET + token.created_at = timestamp(), + token.expires_at = timestamp() + 86400000 // 24h expiry + // Prevent reassigning an existing token to a different identity + OPTIONAL MATCH (token)<-[:HAS_TOKEN]-(owner:Identity) + WITH token, identity, owner + WHERE owner IS NULL OR owner = identity + MERGE (identity)-[:HAS_TOKEN]->(token)Migration/constraint (outside this file):
CREATE CONSTRAINT token_id_unique IF NOT EXISTS FOR (t:Token) REQUIRE t.id IS UNIQUE;api/routes/auth.py (2)
608-618: Fully clear session on logout.Avoid residual session data after logout.
async def logout(request: Request) -> RedirectResponse: @@ if api_token: resp.delete_cookie("api_token") await delete_user_token(api_token) + # Clear any server-side session state + if hasattr(request, "session"): + request.session.clear() return resp
447-456: Narrow try/except in Google flow; don’t swallow HTTPException; improve logging.Keep token exchange in its own try; use
logging.exception; return generic client message.- try: - google = _get_provider_client(request, "google") - token = await google.authorize_access_token(request) - resp = await google.get("userinfo", token=token) + google = _get_provider_client(request, "google") + try: + token = await google.authorize_access_token(request) + except HTTPException: + raise + except Exception: + logging.exception("Google OAuth token exchange failed") + raise HTTPException(status_code=400, detail="Authentication failed") from None + resp = await google.get("userinfo", token=token) @@ - except Exception as e: - logging.error("Google OAuth authentication failed: %s", str(e)) # nosemgrep - raise HTTPException(status_code=400, detail=f"Authentication failed: {str(e)}") from e + except HTTPException: + raise + except Exception as e: + logging.exception("Google OAuth authentication failed") + raise HTTPException(status_code=400, detail="Authentication failed") from eAlso applies to: 491-494
app/public/css/modals.css (1)
497-506: Duplicate .token-input rules — consolidate to a single definition.Two blocks conflict; merge into one with the superset of properties.
-.token-input { - flex: 1 1 700px; /* allow input to grow, prefer large width */ - min-width: 300px; - padding: 0.6em 0.8em; - border: 1px solid var(--border-color); - border-radius: 6px; - color: var(--text-primary); - background: var(--falkor-quaternary); - overflow-x: auto; -} @@ -.token-input { - flex: 1; - padding: 0.8em; - font-size: 0.9em; - border: 1px solid var(--border-color); - border-radius: 6px; - background: var(--falkor-quaternary); - color: var(--text-primary); - word-break: break-all; -} +.token-input { + flex: 1 1 700px; + min-width: 300px; + padding: 0.75em; + font-size: 0.9em; + border: 1px solid var(--border-color); + border-radius: 6px; + background: var(--falkor-quaternary); + color: var(--text-primary); + overflow-x: auto; + word-break: break-all; +}Also applies to: 575-584
🧹 Nitpick comments (15)
tests/e2e/test_file_upload.py (1)
41-41: Replace arbitrary sleeps with condition waits.
page.wait_for_timeout(...)is brittle. Prefer waiting for concrete UI signals (success/error banners) to reduce flakes.- page.wait_for_timeout(2000) # Wait for any processing + page.wait_for_selector(".alert-success, .success, .alert-error, .error", timeout=10000)Apply similarly where other waits occur.
Also applies to: 123-123, 153-153
api/routes/database.py (2)
117-121: Remove redundant exception argument in logging.exception
logging.exceptionalready includes the traceback; passingeis redundant (TRY401).- except Exception as e: # pylint: disable=broad-exception-caught - logging.exception("Error while loading database schema: %s", str(e)) + except Exception as e: # pylint: disable=broad-exception-caught + logging.exception("Error while loading database schema") @@ - except Exception as e: # pylint: disable=broad-exception-caught - logging.exception("Unexpected error in connect_database stream: %s", str(e)) + except Exception as e: # pylint: disable=broad-exception-caught + logging.exception("Unexpected error in connect_database stream")Also applies to: 123-127
135-135: Content type doesn’t match payload (not valid JSON stream)The body is a delimiter-separated stream of JSON objects, not a single JSON document. Consider using a more appropriate media type to avoid confusion and mis-parsing by intermediaries.
- return StreamingResponse(generate(), media_type="application/json") + return StreamingResponse(generate(), media_type="text/plain; charset=utf-8")api/auth/user_management.py (1)
412-417: Return the declared Pydantic model (typing consistency)Function type says Optional[IdentityInfo] but returns dicts. Instantiate the model to align with the signature.
- return True, { - "identity": identity, - "user": user, - "new_identity": is_new_identity, - } + return True, IdentityInfo(identity=identity, user=user, new_identity=is_new_identity) @@ - return False, { - "identity": identity, - "user": user, - "new_identity": is_new_identity, - } + return False, IdentityInfo(identity=identity, user=user, new_identity=is_new_identity)Also applies to: 420-425
app/ts/modules/modals.ts (5)
353-354: Use the shared delimiter constant to avoid driftImport and use MESSAGE_DELIMITER from config to keep backend/frontend in sync.
- const delimiter = '|||FALKORDB_MESSAGE_BOUNDARY|||'; + const delimiter = MESSAGE_DELIMITER;Add import (outside this hunk):
import { MESSAGE_DELIMITER } from './config';
246-276: Mark previous pending step as error when the next step is an errorCurrently you always mark the previous pending step as success (✓), even if the new step is an error. Adjust to reflect failures.
- // Mark the previous pending step as completed (✓) when adding a new step + // Mark the previous pending step as completed; use ✕ if new step is an error const prevLi = list.lastElementChild as HTMLLIElement | null; if (prevLi) { const prevIcon = prevLi.querySelector('.step-icon') as HTMLElement | null; - if (prevIcon && prevIcon.classList.contains('pending')) { - prevIcon.className = 'step-icon success'; - prevIcon.textContent = '✓'; + if (prevIcon && prevIcon.classList.contains('pending')) { + const isError = status === 'error'; + prevIcon.className = 'step-icon ' + (isError ? 'error' : 'success'); + prevIcon.textContent = isError ? '✕' : '✓'; } }
234-236: Avoid innerHTML for clearing (quiet XSS linters)Use DOM APIs to clear lists; avoids innerHTML usage flagged by scanners.
- if (steps) steps.innerHTML = ''; + if (steps) while (steps.firstChild) steps.removeChild(steps.firstChild); @@ - if (existingList) existingList.innerHTML = ''; + if (existingList) while (existingList.firstChild) existingList.removeChild(existingList.firstChild);Also applies to: 285-287
310-314: Use keydown instead of deprecated keypressSmoother cross-browser behavior; keypress is deprecated.
- dbUrlInput.addEventListener('keypress', function(e) { + dbUrlInput.addEventListener('keydown', function(e) {
346-347: Remove unused ‘type’ from payload or add it to the backend modelBackend currently only models url; extra field is ignored (or could 422 if extra=forbid later). Keep payload minimal or extend the Pydantic model accordingly.
- body: JSON.stringify({ url: dbUrl, type: selectedType }), + body: JSON.stringify({ url: dbUrl }),api/routes/auth.py (3)
74-94: Tighten Pydantic models and fix placeholder docstrings.Use clear docstrings and basic validation hints.
-class EmailLoginRequest(BaseModel): - """_summary_ - - Args: - BaseModel (_type_): _description_ - """ - email: str - password: str +class EmailLoginRequest(BaseModel): + """Email/password login payload.""" + email: str # validated by server-side regex + password: str # min length enforced server-side @@ -class EmailSignupRequest(BaseModel): - """_summary_ - - Args: - BaseModel (_type_): _description_ - """ +class EmailSignupRequest(BaseModel): + """Email/password signup payload.""" firstName: str lastName: str email: str password: str
210-213: Fix typo in error detail.“GEmail” → “Email”.
- detail="GEmail authentication is not enabled" + detail="Email authentication is not enabled"
551-560: Prefer verified primary email.When scanning
user/emails, pickprimary && verifiedfirst.- for email_obj in emails: - if email_obj.get("primary"): - email = email_obj.get("email") - break + for email_obj in emails: + if email_obj.get("primary") and email_obj.get("verified"): + email = email_obj.get("email") + breakapp/templates/components/login_modal.j2 (3)
9-14: Mark error message as an alert for accessibility.- <div class="auth-error-message"> + <div class="auth-error-message" role="alert" aria-live="polite">
19-27: Use url_for for static icons consistently (avoids hardcoded paths).- <a href="{{ url_for('google.login') }}" class="google-login-btn"> - <img src="/static/icons/google.svg" alt="Google logo" class="google-login-logo">Sign in with Google + <a href="{{ url_for('google.login') }}" class="google-login-btn"> + <img src="{{ url_for('static', path='icons/google.svg') }}" alt="Google logo" class="google-login-logo">Sign in with Google </a> @@ - <a href="{{ url_for('github.login') }}" class="github-login-btn"> - <img src="/static/icons/github.svg" alt="GitHub logo" class="github-login-logo">Sign in with GitHub + <a href="{{ url_for('github.login') }}" class="github-login-btn"> + <img src="{{ url_for('static', path='icons/github.svg') }}" alt="GitHub logo" class="github-login-logo">Sign in with GitHub </a>Also applies to: 73-81
39-43: Add autocomplete and names to inputs to improve UX and password manager support.- <form id="email-login-form" class="email-auth-form"> - <input type="email" id="login-email" class="auth-input" placeholder="Email" required> - <input type="password" id="login-password" class="auth-input" placeholder="••••••••••••••••" required> + <form id="email-login-form" class="email-auth-form" autocomplete="on"> + <input type="email" id="login-email" name="email" class="auth-input" placeholder="Email" autocomplete="email" required> + <input type="password" id="login-password" name="password" class="auth-input" placeholder="••••••••••••••••" autocomplete="current-password" required> <button type="submit" class="email-login-btn">Sign in with Email</button> </form> @@ - <form id="email-signup-form" class="email-auth-form"> - <input type="text" id="signup-firstname" class="auth-input" placeholder="First Name" required> - <input type="text" id="signup-lastname" class="auth-input" placeholder="Last Name" required> - <input type="email" id="signup-email" class="auth-input" placeholder="Email" required> - <input type="password" id="signup-password" class="auth-input" placeholder="••••••••••••••••" required> - <input type="password" id="signup-repeat-password" class="auth-input" placeholder="Repeat Password" required> + <form id="email-signup-form" class="email-auth-form" autocomplete="on"> + <input type="text" id="signup-firstname" name="firstName" class="auth-input" placeholder="First Name" autocomplete="given-name" required> + <input type="text" id="signup-lastname" name="lastName" class="auth-input" placeholder="Last Name" autocomplete="family-name" required> + <input type="email" id="signup-email" name="email" class="auth-input" placeholder="Email" autocomplete="email" required> + <input type="password" id="signup-password" name="password" class="auth-input" placeholder="••••••••••••••••" autocomplete="new-password" required> + <input type="password" id="signup-repeat-password" name="passwordConfirm" class="auth-input" placeholder="Repeat Password" autocomplete="new-password" required> <button type="submit" class="email-signup-btn">Sign up</button> </form>Also applies to: 93-100
📜 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 (7)
api/auth/user_management.py(8 hunks)api/routes/auth.py(11 hunks)api/routes/database.py(3 hunks)app/public/css/modals.css(7 hunks)app/templates/components/login_modal.j2(1 hunks)app/ts/modules/modals.ts(5 hunks)tests/e2e/test_file_upload.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/templates/components/login_modal.j2app/ts/modules/modals.tsapp/public/css/modals.css
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/routes/database.pyapi/routes/auth.pytests/e2e/test_file_upload.pyapi/auth/user_management.py
tests/e2e/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place E2E tests under
tests/e2e/with filenamestest_*.py
Files:
tests/e2e/test_file_upload.py
🧬 Code graph analysis (3)
api/routes/database.py (4)
app/ts/modules/config.ts (1)
MESSAGE_DELIMITER(5-5)api/loaders/postgres_loader.py (2)
PostgresLoader(27-544)load(108-154)api/loaders/mysql_loader.py (2)
MySQLLoader(28-581)load(164-211)api/loaders/base_loader.py (1)
load(13-23)
api/routes/auth.py (1)
tests/e2e/examples/dev_auth_bypass.py (1)
validate_user(27-36)
tests/e2e/test_file_upload.py (2)
tests/e2e/pages/home_page.py (2)
HomePage(8-81)navigate_to_home(20-24)tests/conftest.py (2)
authenticated_page(98-114)page_with_base_url(89-94)
🪛 ast-grep (0.38.6)
app/ts/modules/modals.ts
[warning] 234-234: 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: steps.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)
[warning] 285-285: 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: existingList.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/routes/database.py
118-118: Redundant exception object included in logging.exception call
(TRY401)
124-124: Redundant exception object included in logging.exception call
(TRY401)
api/routes/auth.py
153-156: Abstract raise to an inner function
(TRY301)
158-158: Do not catch blind exception: Exception
(BLE001)
159-159: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
160-163: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
197-197: Consider moving this statement to an else block
(TRY300)
199-199: Do not catch blind exception: Exception
(BLE001)
200-200: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
205-205: Unused function argument: request
(ARG001)
210-213: Abstract raise to an inner function
(TRY301)
218-221: Abstract raise to an inner function
(TRY301)
230-233: Abstract raise to an inner function
(TRY301)
237-240: Abstract raise to an inner function
(TRY301)
270-270: Consider moving this statement to an else block
(TRY300)
272-272: Do not catch blind exception: Exception
(BLE001)
273-273: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
274-277: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
350-350: Do not catch blind exception: Exception
(BLE001)
351-351: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
453-453: Abstract raise to an inner function
(TRY301)
492-492: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
493-493: Use explicit conversion flag
Replace with conversion flag
(RUF010)
596-596: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
597-597: Use explicit conversion flag
Replace with conversion flag
(RUF010)
647-647: Possible hardcoded password assigned to argument: "access_token_url"
(S106)
tests/e2e/test_file_upload.py
48-48: Use of assert detected
(S101)
50-50: Use of assert detected
(S101)
76-76: Use of assert detected
(S101)
100-100: Use of assert detected
(S101)
103-103: Use of assert detected
(S101)
126-126: Use of assert detected
(S101)
127-127: Do not catch blind exception: Exception
(BLE001)
129-129: Use of assert detected
(S101)
131-131: Use of assert detected
(S101)
160-160: Use of assert detected
(S101)
162-162: Do not catch blind exception: Exception
(BLE001)
164-164: Use of assert detected
(S101)
167-167: Use of assert detected
(S101)
199-199: Use of assert detected
(S101)
202-202: Use of assert detected
(S101)
230-230: Use of assert detected
(S101)
233-233: Use of assert detected
(S101)
api/auth/user_management.py
70-70: Consider moving this statement to an else block
(TRY300)
71-71: Do not catch blind exception: Exception
(BLE001)
95-95: Do not catch blind exception: Exception
(BLE001)
96-96: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
139-139: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
142-144: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
146-146: Do not catch blind exception: Exception
(BLE001)
184-189: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
190-190: Do not catch blind exception: Exception
(BLE001)
191-196: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
223-224: try-except-pass detected, consider logging the exception
(S110)
223-223: Do not catch blind exception: Exception
(BLE001)
246-246: Do not catch blind exception: Exception
(BLE001)
247-247: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
263-266: Abstract raise to an inner function
(TRY301)
🪛 GitHub Check: CodeQL
api/routes/database.py
[warning] 135-135: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
⏰ 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). (5)
- GitHub Check: deploy
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (2)
api/routes/database.py (1)
59-66: Keep using class methods for loaders
The load methods in both PostgresLoader and MySQLLoader are already decorated with @staticmethod, so calling them on the class won’t raise a TypeError and no instantiation is needed.app/public/css/modals.css (1)
78-92:@keyframes spinis already defined in app/public/css/animations.css (line 42); no changes needed.
Summary by CodeRabbit
New Features
Refactor
Documentation
Style
Tests
Chores