Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughConsolidates FalkorDB env config to a single URL. Updates CSS and templates for a new schema graph layout and controls. Refactors TS modules: introduces a reusable ForceGraph instance with zoom/center controls and resize API, revises chat/message typing via a discriminated union, strengthens graphs loading with auth-aware dropdown and deletion, and pins ForceGraph CDN. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI (Buttons/Sidebar)
participant App as app.ts
participant Schema as schema.ts
participant FG as ForceGraph Instance
User->>UI: Click "Schema" / select DB
UI->>App: loadAndShowGraph(selected)
note over App: Starts async fetch/render
App-->>Schema: showGraph(data)
Schema->>FG: configure graphData, sizes, renderers
App->>Schema: setTimeout(resizeGraph, 450ms)
Schema->>FG: resize + center to fit
UI->>Schema: Zoom In/Out/Center clicks
Schema->>FG: zoom(+/-), zoomToFit()
sequenceDiagram
autonumber
actor User
participant Chat as chat.ts
participant Msg as messages.ts
participant Server as /chat stream
User->>Chat: sendMessage(text)
Chat->>Msg: addMessage("", "loading")
Chat->>Server: POST /chat (stream)
loop stream steps
Server-->>Chat: step JSON/text
alt final_response
Chat->>Msg: addMessage(step.message, "final-result")
else query data
Chat->>Msg: addMessage("Query Result", "query-final-result", null, step.data)
else followups
Chat->>Msg: addMessage(q, "followup")
end
end
Chat->>Msg: removeLoadingMessage()
sequenceDiagram
autonumber
participant UI as UI (Graphs Dropdown)
participant Graphs as graphs.ts
participant API as /graphs
participant Auth as Auth State
Graphs->>Auth: isAuthenticated?
alt not authenticated
Graphs-->>UI: Disable inputs, show login option/placeholder
else authenticated
Graphs->>API: GET /graphs
alt empty list
Graphs-->>UI: "No Databases", disable inputs, message
else has graphs
Graphs-->>UI: Populate custom dropdown
UI->>Graphs: Delete graph
Graphs->>API: DELETE /graphs/{name}
API-->>Graphs: 200/err
Graphs->>Graphs: reload via loadGraphs()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 ReviewThe following issues were found:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.env.example (1)
20-22: Add FASTAPI_DEBUG to.env.example
- Verify
api/index.pyandstart.shreadFASTAPI_DEBUG(e.g.FASTAPI_DEBUG=False)- Ensure
api/extensions.pyconsumesFALKORDB_URL(falling back toFALKORDB_HOST/FALKORDB_PORT), and remove or shim any remainingFALKORDB_HOST/PORTreferences in Dockerfile and CI workflowsapp/ts/modules/chat.ts (1)
223-239: Sanitize server-sent HTML to prevent XSS in destructive confirmation.step.message is injected via innerHTML without escaping. Escape HTML before insertion (minimal change). Consider replacing inline onclick with addEventListener in a follow-up.
- const confirmationHTML = ` + const escapeHtml = (s: string) => + s.replace(/[&<>"']/g, (c) => ({'&':'&','<':'<','>':'>','"':'"',"'":'''}[c] as string)); + const safeMessage = escapeHtml(step.message || '').replace(/\n/g, '<br>'); + const confirmationHTML = ` <div class="destructive-confirmation" data-confirmation-id="${confirmationId}"> - <div class="confirmation-text">${(step.message || '').replace(/\n/g, '<br>')}</div> + <div class="confirmation-text">${safeMessage}</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;Follow-up (optional): replace inline onclick with delegated event listeners bound once on chat container to improve security and CSP compatibility.
Also applies to: 237-237
app/ts/app.ts (1)
25-48: Prevent stale/out-of-order graph renders on rapid selection
Non-awaited fetches can complete out of sequence, briefly showing an old graph. In app/ts/app.ts, introduce a request counter or use an AbortController:// at top of module let loadToken = 0; async function loadAndShowGraph(selected?: string) { if (!selected) return; const token = ++loadToken; try { const resp = await fetch(`/graphs/${encodeURIComponent(selected)}/data`); if (!resp.ok) { console.error('Failed to load graph data:', resp.status, resp.statusText); return; } const data = await resp.json(); if (token !== loadToken) return; // ignore stale response const container = document.getElementById('schema-graph'); if (container) container.innerHTML = ''; showGraph(data); } catch (err) { console.error('Error fetching graph data:', err); } }Alternatively, import and abort the previous
AbortControllerbefore starting a new fetch, passing itssignaltofetchfor true cancellation.
🧹 Nitpick comments (17)
package.json (1)
1-5: Consider workspaces instead of file: dependency.Workspaces simplify installs/hoisting and lifecycle scripts.
{ - "dependencies": { - "queryweaver-app": "file:app" - } + "private": true, + "workspaces": ["app"], + "scripts": { + "build": "npm -w app run build", + "install:all": "npm install --workspaces" + } }app/public/css/menu.css (3)
147-164: Align table borders with theme tokens and guard long content.Use border token instead of text color; add basic overflow safety.
#query-final-result-table { border-collapse: collapse; + max-width: 100%; + table-layout: auto; } #query-final-result-table th, #query-final-result-table td { padding: 4px 8px; + word-break: break-word; } #query-final-result-table th:not(:first-child), #query-final-result-table td:not(:first-child) { - border-left: 1px solid var(--text-primary); + border-left: 1px solid var(--falkor-border-primary); } #query-final-result-table td { - border-top: 1px solid var(--text-primary); + border-top: 1px solid var(--falkor-border-primary); }
186-194: Risk of zero-height graph after removing min-height.If ancestors don’t provide an explicit height, 100% resolves to 0. Consider restoring a min-height and/or using flex.
-#schema-graph { - width: 100%; - height: 100%; -} - -#schema-content { - position: relative; - width: 100%; - height: 100%; -} +#schema-content { + position: relative; + width: 100%; + min-height: 300px; /* ensures visible canvas */ + display: flex; +} +#schema-graph { + width: 100%; + flex: 1 1 auto; +}
209-218: Add keyboard focus styles and hit-area affordances.Improve a11y without changing layout.
#schema-controls button { display: flex; align-items: center; justify-content: center; color: var(--text-primary); background-color: transparent; pointer-events: auto; border: none; cursor: pointer; + width: 32px; + height: 32px; + border-radius: 6px; } +#schema-controls button:hover { + background: var(--falkor-quaternary); +} +#schema-controls button:focus-visible { + outline: 2px solid var(--falkor-border-primary); + outline-offset: 2px; + background: var(--falkor-quaternary); +}app/templates/base.j2 (1)
19-21: Pin d3, add defer, and consider SRI for CDN scripts.Reduces blocking and improves supply-chain safety.
- <script src="https://unpkg.com/d3"></script> - <script src="https://cdn.jsdelivr.net/npm/force-graph@1.50.1"></script> + <script defer src="https://cdn.jsdelivr.net/npm/d3@7"></script> + <script defer src="https://cdn.jsdelivr.net/npm/force-graph@1.50.1"></script> + {# TODO: add integrity="" and crossorigin="anonymous" once hashes are computed #}app/templates/components/sidebar_schema.j2 (1)
6-36: Add accessible names and safe button types.Improves keyboard/screen-reader usability with no visual changes.
- <div id="schema-controls"> - <button id="schema-controls-zoom-in"> + <div id="schema-controls" role="group" aria-label="Schema graph controls"> + <button id="schema-controls-zoom-in" type="button" aria-controls="schema-graph" aria-label="Zoom in" title="Zoom in"> <svg ...>...</svg> </button> - <button id="schema-controls-zoom-out"> + <button id="schema-controls-zoom-out" type="button" aria-controls="schema-graph" aria-label="Zoom out" title="Zoom out"> <svg ...>...</svg> </button> - <button id="schema-controls-center"> + <button id="schema-controls-center" type="button" aria-controls="schema-graph" aria-label="Center graph" title="Center graph"> <svg ...>...</svg> </button> </div>app/ts/modules/graphs.ts (4)
22-26: Remove orphaned option node (never appended).The created is unused and can be removed to avoid confusion.
- const option = document.createElement("option"); - option.value = ""; - option.textContent = "Please log in to access graphs"; - option.disabled = true;
73-80: Dead code: legacy select isn’t populated (options never appended).Either append to a real hidden or drop the block. Since the app uses the custom dropdown, removing is cleaner. - // populate hidden select for legacy code - data.forEach((graph) => { - const option = document.createElement("option"); - option.value = graph; - option.textContent = graph; - option.title = graph; - }); 157-163: Remove unused error/auth node. Same as earlier: created but never appended. - const option = document.createElement("option"); - option.value = ""; - option.textContent = (error as Error).message.includes("Authentication") - ? "Please log in" - : "Error loading graphs"; - option.disabled = true; 208-216: Refresh list and UX after successful upload. Trigger a graphs reload and provide user feedback; also clear the file input to allow re-uploads of the same file. fetch("/graphs", { method: "POST", body: formData, }) .then((response) => response.json()) .then((data) => { console.log("File uploaded successfully", data); + addMessage("Schema uploaded. Reloading databases list…"); + loadGraphs(); + if (target) target.value = ""; }) app/ts/app.ts (2) 66-73: Avoid magic 450ms delay; resize after layout/paint reliably Using a fixed timeout is brittle across devices/transitions. Prefer a paint-synced scheduling approach. Apply this diff: - loadAndShowGraph(selected); - setTimeout(resizeGraph, 450); + loadAndShowGraph(selected); + // Let layout settle before resizing the canvas. + requestAnimationFrame(() => requestAnimationFrame(resizeGraph)); 90-98: Same timing concern on graph re-selection Mirror the resilient scheduling here to avoid racy resizes. - if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { - loadAndShowGraph(selected); - setTimeout(resizeGraph, 450); - } + if (DOM.schemaContainer && DOM.schemaContainer.classList.contains('open')) { + loadAndShowGraph(selected); + requestAnimationFrame(() => requestAnimationFrame(resizeGraph)); + } app/ts/modules/ui.ts (3) 189-190: Throttle resize-triggered graph adjustments Calling setTimeout(resizeGraph, 450) on every window resize can queue many timers and cause jitter. Minimal tweak: debounce inside this module. // add at module scope let resizeTimer: number | undefined; // replace the direct setTimeout call: clearTimeout(resizeTimer); resizeTimer = window.setTimeout(resizeGraph, 200); 56-88: Improve dropdown a11y state Toggle ARIA to reflect open/closed state; helps screen readers. Suggested additions when toggling: Set userProfileBtn.setAttribute("aria-expanded", isOpen ? "true" : "false"). Add aria-controls="user-profile-dropdown" to the button and role="menu" to the dropdown. 21-33: Consider moving padding logic to CSS classes Hardcoded JS styles are harder to maintain; a CSS class on the container can express these states declaratively. app/ts/modules/schema.ts (2) 213-224: Fix element typing in center() schema-graph is the container element (not a canvas). Adjust typing and minor style. -const center = () => { - const canvas = document.getElementById( - "schema-graph" - ) as HTMLCanvasElement; - - if (canvas) { - const rect = canvas.getBoundingClientRect(); +const center = () => { + const containerEl = document.getElementById("schema-graph") as HTMLDivElement | null; + if (containerEl) { + const rect = containerEl.getBoundingClientRect(); const minDimension = Math.min(rect.width, rect.height); const padding = minDimension * 0.1; graphInstance.zoomToFit(500, padding); - } -} + } +}; 166-196: DRY edge-color selection The luminance/CSS fallback appears twice. Consider extracting a single helper to compute the edge color and reuse it for both custom link drawing and linkColor. 📜 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. 📥 Commits Reviewing files that changed from the base of the PR and between c0bbe12 and f24ec68. ⛔ Files ignored due to path filters (1) package-lock.json is excluded by !**/package-lock.json 📒 Files selected for processing (11) .env.example (1 hunks) app/public/css/menu.css (2 hunks) app/templates/base.j2 (1 hunks) app/templates/components/sidebar_schema.j2 (1 hunks) app/ts/app.ts (3 hunks) app/ts/modules/chat.ts (11 hunks) app/ts/modules/graphs.ts (1 hunks) app/ts/modules/messages.ts (1 hunks) app/ts/modules/schema.ts (1 hunks) app/ts/modules/ui.ts (1 hunks) package.json (1 hunks) 🧰 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/templates/base.j2 app/templates/components/sidebar_schema.j2 app/ts/modules/messages.ts app/ts/modules/chat.ts app/ts/modules/graphs.ts app/public/css/menu.css app/ts/app.ts app/ts/modules/schema.ts app/ts/modules/ui.ts .env.example 📄 CodeRabbit inference engine (.github/copilot-instructions.md) Ensure .env.example includes all required environment variables (FastAPI secret/debug, FalkorDB host/port, OAuth client IDs/secrets) with placeholder values Files: .env.example 🧠 Learnings (1) 📚 Learning: 2025-08-24T17:15:21.337Z Learnt from: CR PR: FalkorDB/QueryWeaver#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-08-24T17:15:21.337Z Learning: Applies to .env.example : Ensure `.env.example` includes all required environment variables (FastAPI secret/debug, FalkorDB host/port, OAuth client IDs/secrets) with placeholder values Applied to files: .env.example 🧬 Code graph analysis (5) app/ts/modules/messages.ts (2) app/ts/modules/config.ts (2) state (72-76) DOM (39-64) app/ts/modules/graph_select.ts (1) getSelectedGraph (7-12) app/ts/modules/chat.ts (2) app/ts/modules/messages.ts (1) addMessage (8-116) app/ts/modules/config.ts (2) state (72-76) DOM (39-64) app/ts/modules/graphs.ts (3) app/ts/modules/config.ts (1) DOM (39-64) app/ts/modules/graph_select.ts (5) clearGraphOptions (20-23) addGraphOption (25-57) setSelectedGraph (14-18) toggleOptions (59-62) getSelectedGraph (7-12) app/ts/modules/messages.ts (2) addMessage (8-116) initChat (176-192) app/ts/app.ts (1) app/ts/modules/schema.ts (1) resizeGraph (225-233) app/ts/modules/ui.ts (3) app/ts/modules/config.ts (1) DOM (39-64) app/ts/modules/messages.ts (1) initChat (176-192) app/ts/modules/schema.ts (1) resizeGraph (225-233) 🪛 ast-grep (0.38.6) app/ts/modules/messages.ts [warning] 177-177: 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] 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: 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] 226-226: 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] 62-62: 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] 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] 65-65: 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] 73-73: 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) ⏰ 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 (1) app/ts/modules/schema.ts (1) 140-165: Progressive D3 integration looks good Gracefully enabling forces when available is a solid enhancement.
There was a problem hiding this comment.
Pull Request Overview
This PR implements query result visualization improvements and schema graph controls to enhance the user experience. The changes fix issue #158 by properly rendering query results as formatted tables instead of raw JSON.
Key Changes
- Added table rendering for query results with proper formatting
- Implemented zoom controls and responsive resizing for the schema graph
- Unified chat message types with a more structured approach
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added local dependency for the app module |
| app/ts/modules/ui.ts | Formatting updates and added graph resize integration |
| app/ts/modules/schema.ts | Added graph controls, resizing functionality, and improved rendering |
| app/ts/modules/messages.ts | Refactored message system to use type-based approach with table rendering |
| app/ts/modules/graphs.ts | Code formatting improvements |
| app/ts/modules/chat.ts | Updated to use new message type system |
| app/ts/app.ts | Integrated graph resizing on container toggles |
| app/templates/components/sidebar_schema.j2 | Added zoom and center control buttons |
| app/templates/base.j2 | Pinned Force Graph script to specific version |
| app/public/css/menu.css | Added styles for query result tables and schema controls |
| .env.example | Consolidated FalkorDB configuration |
Summary by CodeRabbit