Conversation
|
Warning Rate limit exceeded@gkorland has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughIntroduces a TypeScript-based frontend under app/, integrates its build into Makefile and CI, updates Flask to use app/templates and app/public for static/templates, replaces legacy JS with TS modules, adds new templates (chat layout, toolbar, sidebars), adjusts CSS and static paths, and updates docs. Removes api/static/js/* legacy entry and modules. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant UI as App (app.ts)
participant G as /graphs API
participant D as /database API
participant C as Chat API (/chat)
U->>UI: Load page (chat.j2)
UI->>UI: initializeApp() ➜ setup UI + listeners
UI->>G: GET /graphs
alt Authenticated and graphs available
G-->>UI: 200 [list of graphs]
UI->>UI: Populate selector, enable inputs
else Unauthenticated or none
G-->>UI: 401/200 []
UI->>UI: Disable inputs, show message
end
U->>UI: Open Schema sidebar / change graph
UI->>G: GET /graphs/{id}/data
G-->>UI: 200 { nodes, links }
UI->>UI: showGraph(data) via ForceGraph
U->>UI: Type message + Send
UI->>C: POST /chat (stream)
C-->>UI: Streamed steps
UI->>UI: Update messages progressively
U->>UI: Connect Database (modal)
UI->>D: POST /database { type, url }
D-->>UI: 200/4xx
UI->>UI: Close modal / show error
sequenceDiagram
autonumber
participant U as User
participant TB as Toolbar/Left Toolbar
participant UI as UI module
participant DOM as DOM
U->>TB: Click Menu / Schema / Theme
TB->>UI: toggleContainer(target)
UI->>DOM: Toggle aria-pressed, classes, focus mgmt
U->>TB: Arrow keys / Enter
TB->>UI: Keyboard navigation handlers
UI->>DOM: Move focus, activate buttons
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF ScorecardScorecard details
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/templates/components/chat_header.j2 (2)
10-13: Enable the schema-upload input
Our search didn’t uncover any logic that removes thedisabledattribute or setsdisabled = falseon the#schema-uploadelement. Because the<input id="schema-upload" disabled/>remains disabled, clicking its<label>will not open the file picker, blocking schema uploads entirely.To resolve, you must add runtime code to enable it (for example, after graphs load):
• In app/ts/modules/graphs.ts, inside the success handler (around line 61), add:
if (DOM.fileUpload) DOM.fileUpload.disabled = false;• Or, if the file input should always be enabled, remove the
disabledattribute from the template in app/templates/components/chat_header.j2.Please update accordingly so users can actually upload a JSON schema.
16-31: Accessibility: Add keyboard support and ARIA roles to the “Connect Database” custom dropdownThe current implementation exposes a custom‐styled dropdown but lacks the required interactive semantics and keyboard handling. To meet basic ARIA authoring practices, please apply the following mandatory refactors:
• app/templates/components/chat_header.j2 (lines 16–31)
– Add role, aria‐attributes and tabindex to the markup:
•.dropdown-selected: role="button", aria-haspopup="listbox", aria-expanded="false", aria-controls="database-dropdown-options", tabindex="0"
•.dropdown-options: role="listbox", tabindex="-1"
•.dropdown-option: role="option", aria-selected="false", tabindex="-1"• app/ts/modules/ui.ts → setupCustomDropdown()
- On the selected element, add a keydown listener for Enter/Space to toggle open/close and update
aria-expanded.- On the options container (or document), add keydown handlers for ArrowUp/ArrowDown (or Left/Right) to move focus between
.dropdown-optionitems and update theiraria-selectedstate.- On focused option, handle Enter/Space to commit selection (update hidden input, close dropdown, reflect choice in
.dropdown-selected, and resetaria-expanded).- Retain the existing Escape handler but ensure it also updates
aria-expandedback to “false” and returns focus to.dropdown-selected.Example diff (HTML only—JS changes in ui.ts similarly required):
@@ app/templates/components/chat_header.j2 - <div class="custom-dropdown" id="database-type-dropdown"> - <div class="dropdown-selected" title="Select Database Type"> + <div class="custom-dropdown" id="database-type-dropdown"> + <div class="dropdown-selected" title="Select Database Type" + role="button" aria-haspopup="listbox" + aria-expanded="false" aria-controls="database-dropdown-options" + tabindex="0"> <span class="dropdown-text">Connect Database</span> <span class="dropdown-arrow">▼</span> </div> - <div class="dropdown-options" id="database-dropdown-options"> + <div class="dropdown-options" id="database-dropdown-options" + role="listbox" tabindex="-1"> <div class="dropdown-option" data-value="postgresql"> + <!-- on JS open: update aria-selected and tabindex --> <img src="{{ url_for('static', filename='icons/postgresql.svg') }}" alt="PostgreSQL" class="db-icon"> <span>PostgreSQL</span> </div> <div class="dropdown-option" data-value="mysql"> + <!-- same here --> <img src="{{ url_for('static', filename='icons/mysql.svg') }}" alt="MySQL" class="db-icon"> <span>MySQL</span> </div> </div>Implement the corresponding keyboard handlers in
setupCustomDropdown()so that all users—mouse, keyboard, and assistive-tech—can operate the dropdown seamlessly.app/ts/modules/chat.ts (2)
240-276: Confirm against the right graph even if the user changes selection later
handleDestructiveConfirmationreadsDOM.graphSelect?.valueat confirm time. If the user changes the graph after the prompt appears, the destructive query could run against a different graph. Use the graph value captured when the confirmation UI was created (stored on the wrapper asdata-graphin the previous diff).Apply this diff:
- try { - const selectedValue = DOM.graphSelect?.value || ''; + try { + // Prefer the graph captured on the confirmation wrapper; fall back to current selection. + const selectedValue = + (confirmationDialog as HTMLElement | null)?.getAttribute('data-graph') || + DOM.graphSelect?.value || + '';
283-285: Remove global exposure; rely on the programmatic listeners aboveNow that we attach event listeners directly, the global
window.handleDestructiveConfirmationis no longer needed and is CSP-hostile. Remove it.Apply this diff:
-// Expose global for inline onclick handlers -(window as any).handleDestructiveConfirmation = handleDestructiveConfirmation;
🧹 Nitpick comments (44)
.gitignore (1)
18-19: Good adds for TS migration; consider scoping .d.ts and ignoring build outputs.
- node_modules/ is right.
- Blanket-ignoring all *.d.ts can hide hand-authored ambient types in-repo. If you plan to author types later, scope the ignore to generated locations only.
- Also ignore build artifacts to prevent churn (tsc outDir and bundler output).
Apply one of the following diffs:
Option A — keep room for custom types, ignore only generated ones and build outputs:
*.tmp tmp_* -*.d.ts node_modules/ +public/js/ +api/static/dist/ +app/public/js/ +# If you ever emit declarations into a folder, ignore there specifically: +# app/types/**/*.d.tsOption B — if you’re certain the repo will never store hand-authored .d.ts:
*.tmp tmp_* *.d.ts node_modules/ +public/js/ +api/static/dist/ +app/public/js/app/public/css/base.css (1)
11-11: Font path likely wrong relative to CSS; add format and swap for better UX.From app/public/css/base.css, the relative path 'fonts/fira_code.ttf' points to app/public/css/fonts/... which probably doesn’t exist. Typical layout is app/public/fonts, so the relative path should be '../fonts/...'. Also add format() and font-display.
Proposed fix:
@font-face { font-family: 'Fira Code'; - src: url('fonts/fira_code.ttf'); + src: url('../fonts/fira_code.woff2') format('woff2'), + url('../fonts/fira_code.ttf') format('truetype'); + font-display: swap; }If fonts actually live under css/fonts/, keep the current path but still add format() and font-display.
app/templates/base.j2 (1)
19-20: Pin CDN versions and defer; consider bundling via esbuild instead.Unpinned CDN URLs can introduce breaking changes and block rendering. At minimum add defer and pin major versions; ideally install via npm and let esbuild bundle them for CSP/SRI and offline repeatability.
Option A — minimally pin + defer:
- <script src="https://unpkg.com/d3"></script> - <script src="https://unpkg.com/force-graph"></script> + <script src="https://unpkg.com/d3@7" defer></script> + <script src="https://unpkg.com/force-graph@1" defer></script>Option B — preferred: add d3 and force-graph to app/package.json and
importthem in TS, letting esbuild include them in your bundle. This removes the need for these tags entirely. I can send a small patch if you want to move them into the bundle.app/templates/components/chat_input.j2 (1)
6-10: Path change looks good; add minor a11y and robustness tweaks.
- Add type="button" to avoid accidental form submission if this moves inside a form.
- Add aria-labels so screen readers don’t rely only on title.
- Optional: decoding="async" on the icons.
- <button class="input-button" title="Submit" id="submit-button"> - <img src="{{ url_for('static', filename='icons/submit.svg') }}" alt="Submit"> + <button class="input-button" type="button" title="Submit" aria-label="Submit" id="submit-button"> + <img src="{{ url_for('static', filename='icons/submit.svg') }}" alt="Submit" decoding="async"> </button> - <button class="input-button" title="Pause" id="pause-button"> - <img src="{{ url_for('static', filename='icons/pause.svg') }}" alt="Pause"> + <button class="input-button" type="button" title="Pause" aria-label="Pause" id="pause-button"> + <img src="{{ url_for('static', filename='icons/pause.svg') }}" alt="Pause" decoding="async"> </button>app/tsconfig.json (1)
1-15: Align tsconfig with esbuild bundling; avoid stray JS outputs and resolution pitfalls.Given esbuild handles bundling/transpilation, prefer type-check only via tsc. Also switch to bundler resolution to match ESM and CDN-less workflow, and exclude build dirs.
Apply:
{ "compilerOptions": { "target": "ES2020", - "module": "ESNext", - "moduleResolution": "node", + "module": "ESNext", + "moduleResolution": "bundler", "strict": true, "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "skipLibCheck": true, - "outDir": "./public/js", - "declaration": false, - "sourceMap": true + "noEmit": true, + "isolatedModules": true, + "lib": ["ES2020", "DOM"], + "sourceMap": true }, - "include": ["./**/*"] + "include": ["ts/**/*"], + "exclude": ["public/**", "static/**", "node_modules/**"] }Notes:
- noEmit avoids stray JS in public/js and keeps esbuild as the single source of truth.
- moduleResolution: "bundler" prevents ESM/extension headaches with esbuild.
- Restrict include to your TS sources (adjust if your entry isn’t under ts/).
app/templates/components/sidebar_menu.j2 (2)
and
2-9: Add semantic structure and ARIA to the sidebar; prefer
for accessibilityUsing semantic elements improves keyboard navigation and screen reader UX. Also, a native
conveys a separator better than a purely decorative div.Apply:
-<div id="menu-container" class="sidebar-container"> +<aside id="menu-container" class="sidebar-container" role="complementary" aria-label="Sidebar menu"> <div id="menu-content"> {% include 'components/menu_instructions.j2' %} - <div class="horizontal-separator"></div> + <hr class="horizontal-separator" aria-hidden="true" /> {% include 'components/menu_analytics.j2' %} </div> - -</div> + +</aside>
2-3: Consider dropping the redundant id if class-based selectors are the new normMost CSS in this PR moved to class selectors (.sidebar-container). Keeping both id and class can increase specificity complexity later. If nothing relies on the id for behavior or styling, consider removing it to avoid selector divergence.
Would you like me to scan the codebase for references to #menu-container and generate a removal diff if unused?
api/app_factory.py (3)
82-95: Reassess the /static/ before_request guard; secure_filename strips directories, weakening checkssecure_filename("js/app.js") → "js_app_js", which bypasses your intent to validate nested paths. Flask’s static route already rejects directories and traversal; if you still want a guard, use werkzeug.utils.safe_join to confirm containment instead of mangling the path.
Minimal hardening:
- if request.path.startswith('/static/'): - # Remove /static/ prefix to get the actual path - filename = secure_filename(request.path[8:]) - # Normalize and ensure the path stays within static_folder - static_folder = os.path.abspath(app.static_folder) - file_path = os.path.normpath(os.path.join(static_folder, filename)) - if not file_path.startswith(static_folder): - abort(400) # Bad request, attempted traversal - if os.path.isdir(file_path): - abort(405) + if request.path.startswith('/static/'): + rel = request.path[len('/static/'):] + try: + # safe_join raises on traversal attempts + from werkzeug.utils import safe_join + file_path = safe_join(app.static_folder, rel) + except Exception: + abort(400) + if file_path and os.path.isdir(file_path): + abort(405)
33-56: Guard OAuth blueprints behind env presence to avoid misconfigured login routesIf client id/secret are missing, the login endpoint will exist and fail at runtime. Only register when fully configured, and log a single warning otherwise.
- google_bp = make_google_blueprint( + google_bp = None + if google_client_id and google_client_secret: + google_bp = make_google_blueprint( client_id=google_client_id, client_secret=google_client_secret, scope=[ "https://www.googleapis.com/auth/userinfo.email", "https://www.googleapis.com/auth/userinfo.profile", "openid" ] - ) - app.register_blueprint(google_bp, url_prefix="/login") + ) + app.register_blueprint(google_bp, url_prefix="/login") + else: + logging.warning("Google OAuth not configured; skipping blueprint registration.")Apply similar conditional registration for GitHub.
25-31: Use absolute paths for templates and static assets, and setstatic_url_pathVerified that the
app/templatesandapp/publicdirectories exist at the project root, so resolving them viaPathlibwill work reliably regardless of the current working directory.
- In api/app_factory.py, add near the top with the other imports:
from pathlib import Path # Resolve project root (two levels up from this file) APP_ROOT = Path(__file__).resolve().parents[1]- Update the Flask application factory to use absolute paths and explicitly set the URL path for static files:
- app = Flask(__name__, template_folder="../app/templates", static_folder="../app/public") + app = Flask( + __name__, + template_folder=str(APP_ROOT / "app" / "templates"), + static_folder=str(APP_ROOT / "app" / "public"), + static_url_path="/static", + )This optional refactor ensures that template and static asset paths remain correct even if the app is invoked from a different directory or via a WSGI server.
app/package.json (2)
5-8: Add type-check and a production build variantType checking catches issues earlier; a minified prod build improves load times.
"scripts": { - "build": "esbuild ts/app.ts --bundle --outfile=./public/js/app.js --sourcemap --platform=browser", - "watch": "esbuild ts/app.ts --bundle --outfile=./public/js/app.js --sourcemap --platform=browser --watch" + "build": "esbuild ts/app.ts --bundle --outfile=./public/js/app.js --sourcemap --platform=browser", + "build:prod": "esbuild ts/app.ts --bundle --outfile=./public/js/app.js --platform=browser --minify --sourcemap=external", + "typecheck": "tsc --noEmit", + "watch": "esbuild ts/app.ts --bundle --outfile=./public/js/app.js --sourcemap --platform=browser --watch" },
9-12: Pin minimal Node version for reproducibilitySpecify engines to prevent subtle esbuild/TS behavior differences across older Node versions.
"devDependencies": { "esbuild": "^0.18.0", "typescript": "^5.0.0" - } + }, + "engines": { + "node": ">=18.18" + }app/README.md (1)
3-10: Mention watch-mode in quickstartGiven package.json exposes a watch script, adding it here improves DX.
npm install -npm run build +npm run build # one-off +# or during development: +npm run watchapp/public/css/responsive.css (2)
15-18: Align selector with class-based sidebar to avoid specificity driftElsewhere you moved to .sidebar-container. Using the class here keeps selectors consistent and reduces future maintenance.
- #menu-container.open~#chat-container { + .sidebar-container.open~#chat-container { margin-left: 0; }
124-130: Double-check mobile GitHub button offset after toolbar changesright: 60px may conflict with the left toolbar or user-profile button overlap on narrow devices. Consider using a flex container in the header for consistent spacing instead of absolute offsets.
If helpful, I can propose a small header layout change using a flex row to eliminate manual pixel offsets.
api/routes/graphs.py (2)
72-91: Fix inconsistency between docstring and implementation.The docstring mentions returning "edges" but the endpoint actually returns "links" as the second key in the JSON response.
- """Return all nodes and edges for the specified graph (namespaced to the user). + """Return all nodes and links for the specified graph (namespaced to the user). - This endpoint returns a JSON object with two keys: `nodes` and `edges`. + This endpoint returns a JSON object with two keys: `nodes` and `links`. Nodes contain a minimal set of properties (id, name, labels, props). - Edges contain source and target node names (or internal ids), type and props. + Links contain source and target node names (or internal ids), type and props. """
114-154: Consider more explicit error handling for edge cases.While the current implementation handles various column data shapes, consider being more explicit about unexpected data formats to aid debugging.
normalized = [] for col in columns: try: # col may be a mapping-like object or a simple value if not col: continue # Some drivers may return a tuple or list for the collected map if isinstance(col, (list, tuple)) and len(col) >= 2: # try to interpret as (name, type) name = col[0] ctype = col[1] if len(col) > 1 else None elif isinstance(col, dict): name = col.get('name') or col.get('columnName') ctype = col.get('type') or col.get('dataType') else: name = str(col) ctype = None + logging.debug("Unexpected column format for graph %s: %s", + sanitize_log_input(namespaced), type(col).__name__) if not name: continue normalized.append({"name": name, "type": ctype}) - except Exception: + except Exception as e: + logging.debug("Error normalizing column for graph %s: %s", + sanitize_log_input(namespaced), str(e)[:100]) continueapp/ts/modules/config.ts (1)
36-66: Consider adding null checks or error handling for missing elements.While the current implementation correctly returns
nullfor missing elements, consider adding development-time warnings when critical elements are not found.function getElement<T extends HTMLElement | null>(id: string): T { - return document.getElementById(id) as T; + const element = document.getElementById(id) as T; + if (!element && process.env.NODE_ENV === 'development') { + console.warn(`Element with id '${id}' not found`); + } + return element; }app/ts/modules/schema.ts (2)
5-10: Add type safety for the ForceGraph global.The use of
@ts-ignoreandanybypasses TypeScript's type checking. Consider adding proper type declarations for better maintainability.Add type declarations at the top of the file:
declare global { interface Window { ForceGraph: any; d3: any; } }Then update the code:
- // ForceGraph might be a global factory function provided by a bundled lib - // @ts-ignore - const Graph = (ForceGraph as any)()(document.getElementById('schema-graph')) + if (!window.ForceGraph) { + console.error('ForceGraph library not loaded'); + return; + } + const Graph = window.ForceGraph()(document.getElementById('schema-graph'))
119-146: Remove duplicate edge color calculation.The edge color is calculated twice - once in
linkCanvasObject(lines 75-96) and again here (lines 119-146). Consider extracting this to a shared function.+function getEdgeColor(): string { + try { + const root = getComputedStyle(document.documentElement); + const cssEdge = root.getPropertyValue('--edge-color'); + if (cssEdge && cssEdge.trim()) return cssEdge.trim(); + + const bg = getComputedStyle(document.body).backgroundColor || ''; + const m = bg.match(/rgba?\((\d+),\s*(\d+),\s*(\d+)/i); + if (m) { + const r = parseInt(m[1], 10) / 255; + const g = parseInt(m[2], 10) / 255; + const b = parseInt(m[3], 10) / 255; + const L = 0.2126 * r + 0.7152 * g + 0.0722 * b; + return L > 0.6 ? '#111' : '#ffffff'; + } + } catch (e) { + // ignore + } + return '#ffffff'; +} export function showGraph(data: any) { // ... existing code ... .linkCanvasObject((link: any, ctx: CanvasRenderingContext2D) => { - const getEdgeColor = () => { - // ... duplicate function ... - }; - const edgeColor = getEdgeColor(); + const edgeColor = getEdgeColor(); // ... rest of the code ... }); // ... later in the code ... - try { - const computedBg = getComputedStyle(document.body).backgroundColor || ''; - const pickColor = (bg: string) => { - // ... duplicate logic ... - }; - const edgeColor = (() => { - const root = getComputedStyle(document.documentElement); - const cssEdge = root.getPropertyValue('--edge-color'); - if (cssEdge && cssEdge.trim()) return cssEdge.trim(); - return pickColor(computedBg); - })(); - - Graph.linkColor(() => edgeColor) - .linkDirectionalArrowLength(6).linkDirectionalArrowRelPos(1); - } catch (e) { - Graph.linkDirectionalArrowLength(6).linkDirectionalArrowRelPos(1); - } + const edgeColor = getEdgeColor(); + Graph.linkColor(() => edgeColor) + .linkDirectionalArrowLength(6).linkDirectionalArrowRelPos(1);app/templates/components/toolbar.j2 (1)
4-12: Make the GitHub link accessible and privacy-safe (add aria-label, hide decorative SVGs, include noreferrer, announce star updates).
- Provide an accessible name for the link (aria-label), since it currently contains only icons + a number.
- Mark the SVGs as decorative (aria-hidden, focusable="false").
- Add rel="noreferrer" alongside noopener to avoid leaking the referrer.
- Make the stars span live so screen readers are notified when it updates.
Apply this diff:
-<a href="/FalkorDB/QueryWeaver" target="_blank" rel="noopener" class="github-link" id="github-link-btn" title="View QueryWeaver on GitHub"> - <svg width="16" height="16" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg"> +<a href="/FalkorDB/QueryWeaver" target="_blank" rel="noopener noreferrer" class="github-link" id="github-link-btn" title="View QueryWeaver on GitHub" aria-label="Open QueryWeaver on GitHub"> + <svg width="16" height="16" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg" aria-hidden="true" focusable="false"> <path d="M12 0C5.374 0 0 5.373 0 12 0 17.302 3.438 21.8 8.207 23.387c.599.111.793-.261.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.089-.745.083-.729.083-.729 1.205.084 1.839 1.237 1.839 1.237 1.07 1.834 2.807 1.304 3.492.997.107-.775.418-1.305.762-1.604-2.665-.305-5.467-1.334-5.467-5.931 0-1.311.469-2.381 1.236-3.221-.124-.303-.535-1.524.117-3.176 0 0 1.008-.322 3.301 1.23A11.509 11.509 0 0112 5.803c1.02.005 2.047.138 3.006.404 2.291-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.235 1.911 1.235 3.221 0 4.609-2.807 5.624-5.479 5.921.43.372.823 1.102.823 2.222v3.293c0 .319.192.694.801.576C20.566 21.797 24 17.3 24 12c0-6.627-5.373-12-12-12z"/> </svg> - <svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg" style="margin-left: 4px;"> + <svg width="12" height="12" viewBox="0 0 24 24" fill="currentColor" xmlns="http://www.w3.org/2000/svg" style="margin-left: 4px;" aria-hidden="true" focusable="false"> <path d="M12 2l3.09 6.26L22 9.27l-5 4.87 1.18 6.88L12 17.77l-6.18 3.25L7 14.14 2 9.27l6.91-1.01L12 2z"/> </svg> - <span id="github-stars" style="margin-left: 2px; font-size: 12px;">-</span> + <span id="github-stars" style="margin-left: 2px; font-size: 12px;" aria-live="polite" aria-atomic="true">★ -</span> </a>app/templates/components/chat_header.j2 (1)
6-9: Label the graph selector for screen readers.Title is not a reliable accessible name. Add aria-label to the select.
- <select title="Select Database" id="graph-select"> + <select title="Select Database" id="graph-select" aria-label="Select database (graph)"> <option value="">Loading...</option> </select>app/templates/chat.j2 (2)
15-15: Mark primary content region for assistive tech.Add role="main" to the chat container to expose a main landmark.
- <div id="chat-container" class="chat-container"> + <div id="chat-container" class="chat-container" role="main" aria-label="Chat main content">
19-22: Announce new messages to screen readers.Use aria-live on the message list so model and user messages are announced as they appear.
- <div class="chat-messages" id="chat-messages"> - </div> + <div class="chat-messages" id="chat-messages" + aria-live="polite" aria-relevant="additions text" aria-atomic="false"></div>app/templates/components/sidebar_schema.j2 (1)
2-4: Expose schema panel and graph to assistive tech.Add a region landmark labeled by the Schema button; give the graph a role and label.
-<div id="schema-container" class="sidebar-container"> +<div id="schema-container" class="sidebar-container" role="region" aria-labelledby="schema-button"> <div id="schema-content"> - <div id="schema-graph"></div> + <div id="schema-graph" role="img" aria-label="Database schema graph"></div> </div> </div>app/templates/components/left_toolbar.j2 (2)
34-36: Menu button: add type="button" to avoid accidental form submit.Minor, but safer in templates reused within forms.
- <button class="action-button toolbar-button" id="menu-button" title="Instructions" aria-label="Instructions" - aria-pressed="false" tabindex="0"> + <button class="action-button toolbar-button" id="menu-button" type="button" title="Instructions" aria-label="Instructions" + aria-pressed="false" tabindex="0">
21-31: Avoid potential ID collisions in clipPath defs.If this component is ever rendered multiple times, ids leftHalf/rightHalf may collide. Prefix them and update references.
- <clipPath id="leftHalf"> + <clipPath id="lt-leftHalf"> <rect x="0" y="0" width="12" height="24"/> </clipPath> - <clipPath id="rightHalf"> + <clipPath id="lt-rightHalf"> <rect x="12" y="0" width="12" height="24"/> </clipPath> ... - <circle class="system" cx="12" cy="12" r="8" fill="currentColor" clip-path="url(#leftHalf)"/> - <circle class="system" cx="12" cy="12" r="8" fill="none" stroke="currentColor" stroke-width="2" clip-path="url(#rightHalf)"/> + <circle class="system" cx="12" cy="12" r="8" fill="currentColor" clip-path="url(#lt-leftHalf)"/> + <circle class="system" cx="12" cy="12" r="8" fill="none" stroke="currentColor" stroke-width="2" clip-path="url(#lt-rightHalf)"/>app/ts/app.ts (1)
39-40: Clear container safely with removeChild instead of innerHTML.The static analysis warning is correct here. While clearing innerHTML with an empty string is generally safe, using DOM methods is more explicit and avoids any potential issues.
- const container = document.getElementById('schema-graph'); - if (container) container.innerHTML = ''; + const container = document.getElementById('schema-graph'); + if (container) { + while (container.firstChild) { + container.removeChild(container.firstChild); + } + }app/ts/modules/ui.ts (1)
175-223: Clear dropdown text safely without innerHTML.While the current implementation clears innerHTML before adding trusted content, using DOM methods is more explicit and safer.
const dropdownText = selected.querySelector('.dropdown-text') as HTMLElement | null; - if (dropdownText) dropdownText.innerHTML = ''; + if (dropdownText) { + while (dropdownText.firstChild) { + dropdownText.removeChild(dropdownText.firstChild); + } + } if (icon && dropdownText) dropdownText.appendChild(icon); if (dropdownText) dropdownText.appendChild(document.createTextNode(text));app/ts/modules/messages.ts (2)
34-34: Simplify type assertion for better readability.The type assertion can be simplified since
charAt(0).toUpperCase()already returns a string.- userAvatar.alt = (userInfo.name?.charAt(0).toUpperCase() as string) || 'User'; + userAvatar.alt = userInfo.name?.charAt(0).toUpperCase() || 'User';
133-148: Clear chat messages safely without innerHTML.For consistency and safety, replace innerHTML usage with DOM methods when clearing content.
export function initChat() { if (DOM.messageInput) DOM.messageInput.value = ''; - if (DOM.chatMessages) DOM.chatMessages.innerHTML = ''; + if (DOM.chatMessages) { + while (DOM.chatMessages.firstChild) { + DOM.chatMessages.removeChild(DOM.chatMessages.firstChild); + } + } [DOM.confValue, DOM.expValue, DOM.missValue].forEach((element) => { - if (element) element.innerHTML = ''; + if (element) { + while (element.firstChild) { + element.removeChild(element.firstChild); + } + } });app/ts/modules/graphs.ts (1)
8-84: Clear graph select options safely without innerHTML.Replace innerHTML usage with DOM methods for consistency and safety when clearing the select element.
if (!isAuthenticated) { - if (DOM.graphSelect) DOM.graphSelect.innerHTML = ''; + if (DOM.graphSelect) { + while (DOM.graphSelect.firstChild) { + DOM.graphSelect.removeChild(DOM.graphSelect.firstChild); + } + } // ... later in the success handler - if (DOM.graphSelect) DOM.graphSelect.innerHTML = ''; + if (DOM.graphSelect) { + while (DOM.graphSelect.firstChild) { + DOM.graphSelect.removeChild(DOM.graphSelect.firstChild); + } + } // ... and in the error handler - if (DOM.graphSelect) DOM.graphSelect.innerHTML = ''; + if (DOM.graphSelect) { + while (DOM.graphSelect.firstChild) { + DOM.graphSelect.removeChild(DOM.graphSelect.firstChild); + } + }app/public/css/buttons.css (5)
148-148: Right offset changed for GitHub button — check collisions with user/profile/logout buttons
#github-link-btn { right: 90px; }may overlap with.logout-btnand.user-profile-btnthat are also fixed atright: 30px. Please validate on 1024px ≥ widths and in locales with long text. Add a mobile override to stack or hide secondary buttons if needed.
298-299: Duplicate comment linesThere are two identical comments back-to-back. Keep one.
Apply this diff:
-/* Left vertical toolbar (activity bar style) */ -/* Left vertical toolbar (activity bar style) */ +/* Left vertical toolbar (activity bar style) */
300-319: Fixed left toolbar: ensure main content accounts for 48px width
#left-toolbaris fixed at 48px wide andleft: 0. Confirm that the main content adds sufficient left margin/padding so it doesn’t sit underneath the toolbar (especially at 768–1024px widths). If not handled elsewhere, consider adding a CSS variable (e.g.,--left-toolbar-width) and consuming it in the main layout.
329-337: SVG icon color may not apply — set fill/stroke to currentColorMany SVGs ignore
color:unless they usecurrentColor. To reliably theme icons, set fill/stroke explicitly.Apply this diff:
-.toolbar-button svg { color: var(--text-primary); opacity: 0.95; width:18px; height:18px } +.toolbar-button svg { + color: var(--text-primary); + fill: currentColor; + stroke: currentColor; + opacity: 0.95; + width: 18px; + height: 18px; +}
341-349: Improve accessibility: add focus-visible styles for keyboard usersHover animations exist, but there’s no explicit focus style. Add a clear
:focus-visibleoutline and a reduced-motion fallback to avoid motion sickness.Apply this diff:
.toolbar-button:hover { transform: translateY(-4px) scale(1.03); background: linear-gradient(180deg, rgba(255,255,255,0.02), rgba(255,255,255,0.01)); } + +.toolbar-button:focus-visible { + outline: 2px solid var(--falkor-accent); + outline-offset: 2px; +}And append a reduced-motion override near the media query below:
@media (max-width: 768px) { #left-toolbar { @@ #left-toolbar-inner { flex-direction: row; gap: 6px; } } + +@media (prefers-reduced-motion: reduce) { + .toolbar-button { + transition: none; + } + .toolbar-button:hover, + .toolbar-button[aria-pressed="true"] { + transform: none; + } +}app/ts/modules/chat.ts (7)
12-16: UX nit: using “follow-up” style for a validation error is confusing
addMessage('Please select a graph...', false, true)displays a follow-up message style for an input validation error. Consider rendering as a bot/system message instead for clarity.Apply this diff:
- addMessage('Please select a graph from the dropdown before sending a message.', false, true); + addMessage('Please select a graph from the dropdown before sending a message.', false, false);
39-49: POST uses both query string and JSON body — confirm server contractYou’re sending
?q=<message>in the URL and also sendingchat/result/instructionsin the body. If the backend uses only the body, dropqfrom the URL to avoid excessively long URLs and duplicative inputs. Ifqis required, consider moving it into the JSON payload for consistency.
58-65: Don’t leak raw error messages to end users
error.message || String(error)may reveal internal details. Log the full error to console (or telemetry), but show a generic message to users.Apply this diff:
- resetUIState(); - addMessage('Sorry, there was an error processing your message: ' + (error.message || String(error)), false); + resetUIState(); + console.error('Error processing message:', error); + addMessage('Sorry, something went wrong while processing your message. Please try again.', false);
135-159: List rendering: redundant DOM lookups and innerHTML clearing
document.getElementById(...-list)is assigned toulbut immediately overwritten by a newdocument.createElement('ul'). The lookup is dead code.- Clearing with
element.innerHTML = ''trips static analysis for XSS, even if content is controlled. Preferelement.replaceChildren().Apply this diff:
- if (!element) return; - element.innerHTML = ''; - let ul = document.getElementById(`${element.id}-list`) as HTMLUListElement | null; - - ul = document.createElement('ul'); + if (!element) return; + element.replaceChildren(); + const ul = document.createElement('ul'); @@ - let li = document.getElementById(`${element.id}-${i}-li`); - - li = document.createElement('li'); + const li = document.createElement('li');
177-183: Query result renderingConsider pretty-printing JSON and collapsing overly large payloads to prevent chat overflow. Optional improvement, not a blocker.
203-205: Escaper function is narrowly scoped to single-quoted JS stringsIf you keep any inline handlers (not recommended), ensure newlines and other edge cases are considered. With the refactor above, this helper can likely be deleted. If retained, rename to clarify purpose or generalize it.
1-7: Module typing improvements (optional)To strengthen type safety, define a discriminated union for stream steps and avoid
anyacross handlers. This helps catch missing fields at compile time and documents the protocol.Add in a types module or atop this file:
type StreamStep = | { type: 'reasoning_step'; message: string } | { type: 'final_result'; message?: string; is_valid: boolean; conf: number; exp?: string; miss?: string; amb?: string; data?: unknown } | { type: 'followup_questions'; message: string } | { type: 'query_result'; data?: unknown } | { type: 'ai_response'; message: string } | { type: 'destructive_confirmation'; message: string; sql_query?: string } | { type: 'operation_cancelled'; message: string } | { type: string; [k: string]: unknown }; function handleStreamMessage(step: StreamStep) { /* ... */ }Also consider declaring the globals to drop
anycasts:declare global { interface Window { currentUser?: { picture?: string; name?: string }; } }
📜 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 (11)
app/package-lock.jsonis excluded by!**/package-lock.jsonapp/public/css/fonts/fira_code.ttfis excluded by!**/*.ttfapp/public/favicon.icois excluded by!**/*.icoapp/public/icons/github.svgis excluded by!**/*.svgapp/public/icons/google.svgis excluded by!**/*.svgapp/public/icons/logo.svgis excluded by!**/*.svgapp/public/icons/mysql.svgis excluded by!**/*.svgapp/public/icons/pause.svgis excluded by!**/*.svgapp/public/icons/postgresql.svgis excluded by!**/*.svgapp/public/icons/submit.svgis excluded by!**/*.svgapp/public/img/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
.gitignore(1 hunks)api/app_factory.py(1 hunks)api/routes/graphs.py(2 hunks)api/static/js/app.js(0 hunks)api/static/js/modules/config.js(0 hunks)api/static/js/modules/graphs.js(0 hunks)api/static/js/modules/ui.js(0 hunks)api/templates/chat.j2(0 hunks)api/templates/components/sidebar_menu.j2(0 hunks)api/templates/components/toolbar.j2(0 hunks)app/README.md(1 hunks)app/package.json(1 hunks)app/public/css/base.css(1 hunks)app/public/css/buttons.css(2 hunks)app/public/css/menu.css(3 hunks)app/public/css/responsive.css(2 hunks)app/templates/base.j2(2 hunks)app/templates/chat.j2(1 hunks)app/templates/components/chat_header.j2(2 hunks)app/templates/components/chat_input.j2(1 hunks)app/templates/components/left_toolbar.j2(1 hunks)app/templates/components/login_modal.j2(1 hunks)app/templates/components/sidebar_menu.j2(1 hunks)app/templates/components/sidebar_schema.j2(1 hunks)app/templates/components/toolbar.j2(1 hunks)app/ts/app.ts(1 hunks)app/ts/modules/chat.ts(6 hunks)app/ts/modules/config.ts(1 hunks)app/ts/modules/graphs.ts(1 hunks)app/ts/modules/messages.ts(4 hunks)app/ts/modules/modals.ts(5 hunks)app/ts/modules/schema.ts(1 hunks)app/ts/modules/ui.ts(1 hunks)app/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (7)
- api/static/js/app.js
- api/static/js/modules/config.js
- api/templates/components/toolbar.j2
- api/static/js/modules/graphs.js
- api/templates/chat.j2
- api/static/js/modules/ui.js
- api/templates/components/sidebar_menu.j2
🧰 Additional context used
📓 Path-based instructions (3)
api/routes/graphs.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep graph/database route handlers in api/routes/graphs.py
Files:
api/routes/graphs.py
api/routes/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/graphs.py
api/app_factory.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement the application factory (including OAuth setup) in api/app_factory.py
Files:
api/app_factory.py
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to api/app_factory.py : Implement the application factory (including OAuth setup) in api/app_factory.py
Applied to files:
api/app_factory.py
🧬 Code graph analysis (6)
app/ts/modules/graphs.ts (2)
app/ts/modules/config.ts (1)
DOM(40-66)app/ts/modules/messages.ts (2)
addMessage(7-74)initChat(133-148)
api/routes/graphs.py (1)
api/auth/user_management.py (1)
token_required(263-285)
app/ts/app.ts (7)
app/ts/modules/schema.ts (1)
showGraph(5-146)app/ts/modules/messages.ts (1)
initChat(133-148)app/ts/modules/config.ts (1)
DOM(40-66)app/ts/modules/chat.ts (2)
sendMessage(8-68)pauseRequest(193-201)app/ts/modules/ui.ts (9)
toggleContainer(7-33)showResetConfirmation(35-40)handleResetConfirmation(46-51)hideResetConfirmation(42-44)handleWindowResize(157-173)setupUserProfileDropdown(53-79)setupThemeToggle(81-122)setupToolbar(124-155)setupCustomDropdown(175-223)app/ts/modules/graphs.ts (3)
onGraphChange(106-108)handleFileUpload(86-104)loadGraphs(8-84)app/ts/modules/modals.ts (2)
setupAuthenticationModal(5-19)setupDatabaseModal(33-137)
app/ts/modules/chat.ts (2)
app/ts/modules/config.ts (2)
DOM(40-66)state(74-78)app/ts/modules/messages.ts (2)
addMessage(7-74)removeLoadingMessage(76-81)
app/ts/modules/ui.ts (2)
app/ts/modules/config.ts (1)
DOM(40-66)app/ts/modules/messages.ts (1)
initChat(133-148)
app/ts/modules/messages.ts (1)
app/ts/modules/config.ts (2)
state(74-78)DOM(40-66)
🪛 ast-grep (0.38.6)
app/ts/modules/graphs.ts
[warning] 11-11: 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.graphSelect.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] 35-35: 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.graphSelect.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] 76-76: 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.graphSelect.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/app.ts
[warning] 39-39: 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/chat.ts
[warning] 32-32: 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)
[warning] 139-139: 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)
[warning] 229-229: 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: messageDiv.innerHTML = confirmationHTML
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] 229-229: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: messageDiv.innerHTML = confirmationHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
app/ts/modules/ui.ts
[warning] 197-197: 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)
app/ts/modules/messages.ts
[warning] 134-134: 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] 136-136: 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)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 90-90: Log Injection
This log entry depends on a user-provided value.
[failure] 111-111: Log Injection
This log entry depends on a user-provided value.
⏰ 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
| This will bundle the frontend and place the result in `api/static/dist/bundle.js` which your Flask templates should load via `/static/dist/bundle.js`. | ||
|
|
||
| Notes: | ||
| - Keep original JS files in `api/static/js/` for backward compatibility until you update templates. | ||
| - After verification you can remove `api/static/js/*` and the `api/static/ts/` copies. |
There was a problem hiding this comment.
Fix outdated bundle path and static URL in docs
Docs still point to api/static/dist/bundle.js, but the build outputs to app/public/js/app.js and is served at /static/js/app.js via the new static_folder. This will mislead anyone wiring templates.
Apply:
-This will bundle the frontend and place the result in `api/static/dist/bundle.js` which your Flask templates should load via `/static/dist/bundle.js`.
+This bundles the frontend to `app/public/js/app.js`. Load it in Flask templates via `/static/js/app.js` (e.g., `<script src="{{ url_for('static', filename='js/app.js') }}" defer></script>`).Consider also adding:
-Notes:
-- Keep original JS files in `api/static/js/` for backward compatibility until you update templates.
-- After verification you can remove `api/static/js/*` and the `api/static/ts/` copies.
+Notes:
+- Use `npm run watch` during development.
+- Legacy assets under `api/static/*` are deprecated; templates now read from `app/public/*`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| This will bundle the frontend and place the result in `api/static/dist/bundle.js` which your Flask templates should load via `/static/dist/bundle.js`. | |
| Notes: | |
| - Keep original JS files in `api/static/js/` for backward compatibility until you update templates. | |
| - After verification you can remove `api/static/js/*` and the `api/static/ts/` copies. | |
| This bundles the frontend to `app/public/js/app.js`. Load it in Flask templates via `/static/js/app.js` (e.g., `<script src="{{ url_for('static', filename='js/app.js') }}" defer></script>`). | |
| Notes: | |
| - Use `npm run watch` during development. | |
| - Legacy assets under `api/static/*` are deprecated; templates now read from `app/public/*`. |
🤖 Prompt for AI Agents
In app/README.md around lines 12 to 16, the documentation references the old
output path api/static/dist/bundle.js and URL /static/dist/bundle.js which are
stale; update the docs to indicate the build now outputs to app/public/js/app.js
and is served at /static/js/app.js (or whatever the Flask app's static_folder
and URL path are). Change the instructions to tell users to keep legacy files in
app/public/js/ (or note the previous location) until templates are updated, and
update the removal note to reference app/public/js/* and any app/public/ts/*
copies so templates will point to /static/js/app.js after verification.
| export function addDestructiveConfirmationMessage(step: any) { | ||
| const messageDiv = document.createElement('div'); | ||
| const messageDivContainer = document.createElement('div'); | ||
|
|
||
| messageDivContainer.className = "message-container bot-message-container destructive-confirmation-container"; | ||
| messageDiv.className = "message bot-message destructive-confirmation-message"; | ||
|
|
||
| // Generate a unique ID for this confirmation dialog | ||
|
|
||
| messageDivContainer.className = 'message-container bot-message-container destructive-confirmation-container'; | ||
| messageDiv.className = 'message bot-message destructive-confirmation-message'; | ||
|
|
||
| const confirmationId = 'confirmation-' + Date.now(); | ||
|
|
||
| // Create the confirmation UI | ||
|
|
||
| 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">${(step.message || '').replace(/\n/g, '<br>')}</div> | ||
| <div class="confirmation-buttons"> | ||
| <button class="confirm-btn danger" onclick="handleDestructiveConfirmation('CONFIRM', '${escapeForSingleQuotedJsString(step.sql_query)}', '${confirmationId}')"> | ||
| <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}')"> | ||
| <button class="cancel-btn" onclick="handleDestructiveConfirmation('CANCEL', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> | ||
| CANCEL - Abort Operation | ||
| </button> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
||
| messageDiv.innerHTML = confirmationHTML; | ||
|
|
||
| messageDivContainer.appendChild(messageDiv); | ||
| DOM.chatMessages.appendChild(messageDivContainer); | ||
| DOM.chatMessages.scrollTop = DOM.chatMessages.scrollHeight; | ||
|
|
||
| // Disable the main input while waiting for confirmation | ||
| DOM.messageInput.disabled = true; | ||
| DOM.submitButton.disabled = true; | ||
| if (DOM.chatMessages) DOM.chatMessages.appendChild(messageDivContainer); | ||
| if (DOM.chatMessages) DOM.chatMessages.scrollTop = DOM.chatMessages.scrollHeight; | ||
|
|
||
| if (DOM.messageInput) DOM.messageInput.disabled = true; | ||
| if (DOM.submitButton) DOM.submitButton.disabled = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
XSS risk: inserting unsanitized HTML and inline event handlers
messageDiv.innerHTML = confirmationHTMLinjectsstep.messagedirectly into the DOM (with only newline-to-
replacement). Ifstep.messagecontains HTML, this is a direct XSS vector. Static analysis flags this.- Inline
onclick="handleDestructiveConfirmation(...)"is brittle (CSP-unfriendly) and requires global exposure of the handler.
Build the confirmation UI with DOM APIs, set textContent for untrusted strings, and attach listeners programmatically. Also capture the graph selection at the time this confirmation is created to avoid executing on a different graph if the user changes the dropdown before confirming (see next comment).
Apply this diff to replace innerHTML + inline handlers with safe DOM construction:
- 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;
+ // Build confirmation UI safely with DOM APIs
+ const wrapper = document.createElement('div');
+ wrapper.className = 'destructive-confirmation';
+ wrapper.setAttribute('data-confirmation-id', confirmationId);
+ // Capture the currently selected graph to avoid cross-graph execution later.
+ const graphAtPrompt = DOM.graphSelect?.value || '';
+ if (graphAtPrompt) wrapper.setAttribute('data-graph', graphAtPrompt);
+
+ const text = document.createElement('div');
+ text.className = 'confirmation-text';
+ const rawMessage = String(step.message || '');
+ // Preserve line breaks without using innerHTML
+ const lines = rawMessage.split('\n');
+ lines.forEach((line, idx) => {
+ if (idx) text.appendChild(document.createElement('br'));
+ text.appendChild(document.createTextNode(line));
+ });
+
+ 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', String(step.sql_query || ''), confirmationId)
+ );
+
+ const cancelBtn = document.createElement('button');
+ cancelBtn.className = 'cancel-btn';
+ cancelBtn.textContent = 'CANCEL - Abort Operation';
+ cancelBtn.addEventListener('click', () =>
+ handleDestructiveConfirmation('CANCEL', String(step.sql_query || ''), confirmationId)
+ );
+
+ btns.appendChild(confirmBtn);
+ btns.appendChild(cancelBtn);
+ wrapper.appendChild(text);
+ wrapper.appendChild(btns);
+ messageDiv.replaceChildren(wrapper);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function addDestructiveConfirmationMessage(step: any) { | |
| const messageDiv = document.createElement('div'); | |
| const messageDivContainer = document.createElement('div'); | |
| messageDivContainer.className = "message-container bot-message-container destructive-confirmation-container"; | |
| messageDiv.className = "message bot-message destructive-confirmation-message"; | |
| // Generate a unique ID for this confirmation dialog | |
| messageDivContainer.className = 'message-container bot-message-container destructive-confirmation-container'; | |
| messageDiv.className = 'message bot-message destructive-confirmation-message'; | |
| const confirmationId = 'confirmation-' + Date.now(); | |
| // Create the confirmation UI | |
| 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">${(step.message || '').replace(/\n/g, '<br>')}</div> | |
| <div class="confirmation-buttons"> | |
| <button class="confirm-btn danger" onclick="handleDestructiveConfirmation('CONFIRM', '${escapeForSingleQuotedJsString(step.sql_query)}', '${confirmationId}')"> | |
| <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}')"> | |
| <button class="cancel-btn" onclick="handleDestructiveConfirmation('CANCEL', '${escapeForSingleQuotedJsString(step.sql_query || '')}', '${confirmationId}')"> | |
| CANCEL - Abort Operation | |
| </button> | |
| </div> | |
| </div> | |
| `; | |
| messageDiv.innerHTML = confirmationHTML; | |
| messageDivContainer.appendChild(messageDiv); | |
| DOM.chatMessages.appendChild(messageDivContainer); | |
| DOM.chatMessages.scrollTop = DOM.chatMessages.scrollHeight; | |
| // Disable the main input while waiting for confirmation | |
| DOM.messageInput.disabled = true; | |
| DOM.submitButton.disabled = true; | |
| if (DOM.chatMessages) DOM.chatMessages.appendChild(messageDivContainer); | |
| if (DOM.chatMessages) DOM.chatMessages.scrollTop = DOM.chatMessages.scrollHeight; | |
| if (DOM.messageInput) DOM.messageInput.disabled = true; | |
| if (DOM.submitButton) DOM.submitButton.disabled = true; | |
| } | |
| export function addDestructiveConfirmationMessage(step: any) { | |
| const messageDiv = document.createElement('div'); | |
| const messageDivContainer = document.createElement('div'); | |
| messageDivContainer.className = 'message-container bot-message-container destructive-confirmation-container'; | |
| messageDiv.className = 'message bot-message destructive-confirmation-message'; | |
| const confirmationId = 'confirmation-' + Date.now(); | |
| // Build confirmation UI safely with DOM APIs | |
| const wrapper = document.createElement('div'); | |
| wrapper.className = 'destructive-confirmation'; | |
| wrapper.setAttribute('data-confirmation-id', confirmationId); | |
| // Capture the currently selected graph to avoid cross-graph execution later. | |
| const graphAtPrompt = DOM.graphSelect?.value || ''; | |
| if (graphAtPrompt) { | |
| wrapper.setAttribute('data-graph', graphAtPrompt); | |
| } | |
| const text = document.createElement('div'); | |
| text.className = 'confirmation-text'; | |
| const rawMessage = String(step.message || ''); | |
| // Preserve line breaks without using innerHTML | |
| const lines = rawMessage.split('\n'); | |
| lines.forEach((line, idx) => { | |
| if (idx) { | |
| text.appendChild(document.createElement('br')); | |
| } | |
| text.appendChild(document.createTextNode(line)); | |
| }); | |
| 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', String(step.sql_query || ''), confirmationId) | |
| ); | |
| const cancelBtn = document.createElement('button'); | |
| cancelBtn.className = 'cancel-btn'; | |
| cancelBtn.textContent = 'CANCEL - Abort Operation'; | |
| cancelBtn.addEventListener('click', () => | |
| handleDestructiveConfirmation('CANCEL', String(step.sql_query || ''), confirmationId) | |
| ); | |
| btns.appendChild(confirmBtn); | |
| btns.appendChild(cancelBtn); | |
| wrapper.appendChild(text); | |
| wrapper.appendChild(btns); | |
| messageDiv.replaceChildren(wrapper); | |
| messageDivContainer.appendChild(messageDiv); | |
| if (DOM.chatMessages) { | |
| DOM.chatMessages.appendChild(messageDivContainer); | |
| DOM.chatMessages.scrollTop = DOM.chatMessages.scrollHeight; | |
| } | |
| if (DOM.messageInput) DOM.messageInput.disabled = true; | |
| if (DOM.submitButton) DOM.submitButton.disabled = true; | |
| } |
🧰 Tools
🪛 ast-grep (0.38.6)
[warning] 229-229: 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: messageDiv.innerHTML = confirmationHTML
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] 229-229: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: messageDiv.innerHTML = confirmationHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
In app/ts/modules/chat.ts around lines 207 to 238, replace the current innerHTML
+ inline onclick handlers with safe DOM construction: create elements via
document.createElement, set untrusted text using textContent (not innerHTML) for
step.message and any other user-provided strings, and attach click event
listeners with addEventListener to call handleDestructiveConfirmation; also
capture and store the current graph selection (or relevant dropdown value) in a
data attribute or closure when building the confirmation so the handler uses
that captured value rather than reading the UI state at click time; ensure any
SQL/query string is escaped or stored as a text node/data attribute and not
interpolated into HTML, and keep the existing class names and DOM
insertion/scroll behavior.
| export function handleFileUpload(event: Event) { | ||
| const target = event.target as HTMLInputElement | null; | ||
| const file = target?.files ? target.files[0] : null; | ||
| if (!file) return; | ||
|
|
||
| const formData = new FormData(); | ||
| formData.append('file', file); | ||
|
|
||
| fetch('/graphs', { | ||
| method: 'POST', | ||
| body: formData | ||
| }).then(response => response.json()) | ||
| .then(data => { | ||
| console.log('File uploaded successfully', data); | ||
| }).catch(error => { | ||
| console.error('Error uploading file:', error); | ||
| addMessage('Sorry, there was an error uploading your file: ' + (error as Error).message, false); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add feedback and refresh after successful file upload.
The file upload currently logs to console but doesn't provide user feedback or refresh the graph list.
}).then(response => response.json())
.then(data => {
console.log('File uploaded successfully', data);
+ addMessage('File uploaded successfully!', false);
+ // Refresh the graph list to show the newly uploaded graph
+ loadGraphs();
}).catch(error => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function handleFileUpload(event: Event) { | |
| const target = event.target as HTMLInputElement | null; | |
| const file = target?.files ? target.files[0] : null; | |
| if (!file) return; | |
| const formData = new FormData(); | |
| formData.append('file', file); | |
| fetch('/graphs', { | |
| method: 'POST', | |
| body: formData | |
| }).then(response => response.json()) | |
| .then(data => { | |
| console.log('File uploaded successfully', data); | |
| }).catch(error => { | |
| console.error('Error uploading file:', error); | |
| addMessage('Sorry, there was an error uploading your file: ' + (error as Error).message, false); | |
| }); | |
| } | |
| export function handleFileUpload(event: Event) { | |
| const target = event.target as HTMLInputElement | null; | |
| const file = target?.files ? target.files[0] : null; | |
| if (!file) return; | |
| const formData = new FormData(); | |
| formData.append('file', file); | |
| fetch('/graphs', { | |
| method: 'POST', | |
| body: formData | |
| }).then(response => response.json()) | |
| .then(data => { | |
| console.log('File uploaded successfully', data); | |
| addMessage('File uploaded successfully!', false); | |
| // Refresh the graph list to show the newly uploaded graph | |
| loadGraphs(); | |
| }).catch(error => { | |
| console.error('Error uploading file:', error); | |
| addMessage('Sorry, there was an error uploading your file: ' + (error as Error).message, false); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In app/ts/modules/graphs.ts around lines 86 to 104, the upload handler only logs
success to the console; update the success path to show a user-facing success
message (use addMessage('File uploaded successfully', true) or similar) and then
refresh the displayed graph list by invoking the module's graph refresh function
(e.g., refreshGraphs() or loadGraphs()) so the new graph appears without a
manual reload; keep the existing error handling as-is and ensure the UI feedback
runs after the response.json() promise resolves.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
37-45: Google OAuth instructions reference the deprecated Google+ API.“Enable the Google+ API” is obsolete and will mislead users. Replace with Google Identity Platform steps.
Proposed edit:
-3. Enable the Google+ API -4. Go to "Credentials" and create an OAuth 2.0 Client ID +3. Configure the OAuth consent screen under “APIs & Services” → “OAuth consent screen” +4. Go to “Credentials” and create an OAuth 2.0 Client ID (Application type: Web application)Makefile (1)
1-72: Add missingbuildtarget & update.PHONYThe
README.md(line 73) instructs users to runmake buildto produceapp/public/js/app.js, but the Makefile declaresbuildas phony without defining it—somake buildcurrently does nothing. We need to:
- Define a
buildtarget that aliases tobuild-prod- Include
build-devandbuild-prodin the.PHONYdeclarationLocations to address in
Makefile:
- After the existing
build-prod:target (around line 23), add:+build: build-prod ## Alias for production build (runs frontend build)- Update the
.PHONYline at the top (line 1) to include bothbuild-devandbuild-prod:-.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build +.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build build-dev build-prodWith these changes,
make buildwill work as documented, and all phony targets are correctly listed.
🧹 Nitpick comments (4)
.gitignore (1)
18-20: Don’t ignore all .d.ts globally; scope ignores to build artifacts and add sourcemaps/.tsbuildinfo.Global
*.d.tswill also exclude any hand-written ambient typings you may want to check in later (e.g.,app/types/global.d.ts). Prefer scoping to build outputs and add common TS artifacts.Apply this diff:
-*.d.ts node_modules/ /app/public/js/* +app/public/js/*.map +app/.tsbuildinfo +# If you later emit declaration files, ignore them only where they are generated, e.g.: +# app/public/types/**/*.d.tsREADME.md (1)
18-19: Specify supported Node.js version to reduce environment drift.Call out a tested LTS (e.g., Node 20.x LTS) to avoid build discrepancies across contributors and CI.
Makefile (2)
51-58: Clean up built frontend artifacts, too.Add the frontend bundle directory to
cleanto avoid stale assets confusing local testing.clean: ## Clean up test artifacts rm -rf test-results/ rm -rf playwright-report/ rm -rf tests/e2e/screenshots/ rm -rf __pycache__/ find . -name "*.pyc" -delete find . -name "*.pyo" -delete + rm -rf app/public/js/*
59-61: Consider wiring dev watch into run-dev.If
app/package.jsonhas awatchscript, you can either run it in parallel (tmux) or document it. Not mandatory, but improves DX.
📜 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 (5)
.github/copilot-instructions.md(2 hunks).gitignore(1 hunks)Makefile(3 hunks)README.md(2 hunks)app/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/copilot-instructions.md
🚧 Files skipped from review as they are similar to previous changes (1)
- app/package.json
🧰 Additional context used
📓 Path-based instructions (1)
Makefile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Expose developer workflows via Makefile targets (install, setup-dev, test, lint, run)
Files:
Makefile
🧠 Learnings (2)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to Makefile : Expose developer workflows via Makefile targets (install, setup-dev, test, lint, run)
Applied to files:
Makefile
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to tests/e2e/test_*.py : Place Playwright E2E tests as tests/e2e/test_*.py
Applied to files:
Makefile
🪛 GitHub Actions: Spellcheck
README.md
[error] 1-1: Spellcheck failed: Spelling errors detected in README.md by spellcheck action (rojopolis/spellcheck-github-actions@0.51.0).
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
⏰ 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (2)
README.md (2)
174-176: Verify LICENSE link filename.README links to
LICENSE.txt. Ensure the repository actually containsLICENSE.txt; many repos useLICENSEwithout an extension.
1-180: It looks like running codespell locally on README.md returned no misspellings—so we’re not reproducing the CI error. Could you please share the exact spellcheck failure message or CI log snippet so we can identify which token is being flagged?
Makefile
Outdated
| @@ -1,4 +1,4 @@ | |||
| .PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev | |||
| .PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
PHONY list and “all” target: add missing ‘all’ and ensure listed targets actually exist.
buildis listed in.PHONYbut there is nobuild:rule.- Static analysis also flags missing
all.
Apply this diff:
-.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build
+.PHONY: help install setup-dev build build-dev build-prod test test-unit test-e2e test-e2e-headed test-e2e-debug lint format clean run-dev run-prod allThen add an “all” aggregator (commonly default) and a real build alias (see other comments).
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
🤖 Prompt for AI Agents
In Makefile lines 1-1, the .PHONY list mentions targets that don't exist and
lacks a default "all" target; add a top-level "all" target (default) that
aggregates the common pipeline (e.g., depends on install build test lint format)
and create a concrete "build" target that performs the project build (or
delegates to an existing setup-dev/compile step). Update .PHONY to include "all"
and "build" and ensure the new build target either runs the real build commands
(compile/package) or is a simple alias that invokes the established setup-dev or
other build recipe so the listed targets actually exist.
| pipenv sync --dev | ||
| npm install ./app | ||
|
|
||
|
|
There was a problem hiding this comment.
Install frontend deps correctly: use npm --prefix with ci for reproducibility.
npm install ./app installs the ./app folder as a package dependency of the root, not what we want. Use --prefix and ci to install into app/ with lockfile fidelity.
install: ## Install dependencies
pipenv sync --dev
- npm install ./app
+ npm --prefix ./app ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| install: ## Install dependencies | |
| pipenv sync --dev | |
| npm install ./app | |
| install: ## Install dependencies | |
| pipenv sync --dev | |
| npm --prefix ./app ci |
🤖 Prompt for AI Agents
In Makefile around lines 9 to 12, the target currently runs "npm install ./app"
which incorrectly installs the app folder as a dependency of the repo root;
replace that invocation with "npm ci --prefix app" (or "npm ci --prefix ./app")
so dependencies are installed into the app/ directory using the lockfile for
reproducible installs; keep the pipenv sync --dev line unchanged.
| npm --prefix ./app run build:dev | ||
|
|
||
| build-prod: | ||
| npm --prefix ./app run build | ||
|
|
||
| test: build-dev ## Run all tests |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a public ‘build’ target as documented in README.
Create a simple alias that maps to the production build. This matches “make build” in the README and keeps workflows discoverable.
build-dev:
npm --prefix ./app run build:dev
build-prod:
npm --prefix ./app run build
+
+# Public build alias (used by README)
+build: build-prod ## Build frontend (prod)🤖 Prompt for AI Agents
In Makefile around lines 20 to 25, the README promises a top-level "make build"
target but the file only defines build-dev and build-prod; add a public "build"
target that simply invokes or depends on the production build (e.g., make build
should run the same command as build-prod), so create a "build:" entry that
aliases to the production build target to keep workflows discoverable.
| test-unit test-e2e | ||
|
|
There was a problem hiding this comment.
Fix broken ‘test’ target: dependencies must be on the rule line, not executed as a shell command.
Right now test-unit test-e2e is executed as a single shell command and will fail. Make them dependencies or explicitly invoke make.
Option A (dependencies on one line):
-test: build-dev ## Run all tests
- test-unit test-e2e
+test: build-dev test-unit test-e2e ## Run all testsOption B (explicit invocations):
-test: build-dev ## Run all tests
- test-unit test-e2e
+test: build-dev ## Run all tests
+ $(MAKE) test-unit
+ $(MAKE) test-e2e🤖 Prompt for AI Agents
In Makefile around lines 26 to 27, the recipe for the "test" target runs
"test-unit test-e2e" as a shell command which fails; change the target so those
are make dependencies or explicitly invoke make. Either add them to the rule
line (e.g., make "test: build-dev test-unit test-e2e" and remove or keep an
empty recipe) or keep "test: build-dev" and change the recipe to call make for
each target ("$(MAKE) test-unit" and "$(MAKE) test-e2e") so they run as Make
targets rather than a single shell command.
| npm --prefix ./app run build | ||
| pipenv run python -m flask --app api.index run | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-building in run-prod.
run-prod depends on build-prod and then runs npm ... build again. Drop the duplicate invocation.
-run-prod: build-prod ## Run production server
- npm --prefix ./app run build
+run-prod: build-prod ## Run production server
pipenv run python -m flask --app api.index run🤖 Prompt for AI Agents
In Makefile around lines 62 to 64, the run-prod target redundantly invokes the
frontend build a second time (it already depends on build-prod); remove the
duplicate "npm --prefix ./app run build" line so run-prod simply depends on
build-prod and then runs the flask server, ensuring the build only runs once.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/ts/modules/chat.ts (1)
241-282: Use captured graph for confirmation; avoid reading mutable UI stateIf users change the graph between prompt and click, confirmation could execute against the wrong graph. Read data-graph captured above.
export async function handleDestructiveConfirmation(confirmation: string, sqlQuery: string, confirmationId: string) { const confirmationDialog = document.querySelector(`[data-confirmation-id="${confirmationId}"]`); if (confirmationDialog) { const confirmBtn = confirmationDialog.querySelector('.confirm-btn') as HTMLButtonElement | null; const cancelBtn = confirmationDialog.querySelector('.cancel-btn') as HTMLButtonElement | null; if (confirmBtn) confirmBtn.disabled = true; if (cancelBtn) cancelBtn.disabled = true; } @@ - if (confirmation === 'CANCEL') { + if (confirmation === 'CANCEL') { addMessage('Operation cancelled. The destructive SQL query was not executed.', false, true); return; } @@ try { - const selectedValue = DOM.graphSelect?.value || ''; + const selectedValue = + (confirmationDialog as HTMLElement | null)?.getAttribute('data-graph') || + DOM.graphSelect?.value || + '';
♻️ Duplicate comments (6)
Makefile (5)
1-1: Add a default “all” target and include it in .PHONY.Tooling flags the absence of an “all” target; also ensure .PHONY lists only real targets. Add “all” and wire it to the common workflow.
-.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build lint-frontend +.PHONY: help install setup-dev build build-dev build-prod test test-unit test-e2e test-e2e-headed test-e2e-debug lint lint-frontend format clean run-dev run-prod docker-falkordb docker-stop all + +# Default pipeline aggregator +all: install build test lint
68-70: Avoid double-building in run-prod.
run-proddepends onbuild-prodand then runsnpm ... buildagain; remove the duplicate.-run-prod: build-prod ## Run production server - npm --prefix ./app run build +run-prod: build-prod ## Run production server pipenv run python -m flask --app api.index run
9-11: Install frontend deps correctly: use npm --prefix with ci for reproducibility.
npm install ./appinstalls the folder as a dependency of the root. Use--prefixandci.install: ## Install dependencies pipenv sync --dev - npm install ./app + npm --prefix ./app ci
20-25: Expose a public build target (Makefile lists “build” in .PHONY but no rule exists).Without a
build:rule,make buildfails. Add an alias to the prod build.build-dev: npm --prefix ./app run build:dev build-prod: npm --prefix ./app run build + +# Public build alias +build: build-prod ## Build frontend (prod)
26-27: Fix the “test” target: make dependencies, not a shell command.Currently
test-unit test-e2eexecutes as a single shell command and fails. Make them dependencies (or call $(MAKE)).Option A (dependencies on the rule line):
-test: build-dev ## Run all tests - test-unit test-e2e +test: build-dev test-unit test-e2e ## Run all testsOption B (explicit invocations):
-test: build-dev ## Run all tests - test-unit test-e2e +test: build-dev ## Run all tests + $(MAKE) test-unit + $(MAKE) test-e2eapp/ts/modules/chat.ts (1)
208-239: Critical: XSS via innerHTML and brittle inline onclick handlers in confirmation UI
- step.message is injected via innerHTML and can execute scripts.
- Inline onclick requires exposing a global and breaks CSP.
Replace with safe DOM construction and programmatic listeners; capture the graph at prompt time:
export function addDestructiveConfirmationMessage(step: any) { const messageDiv = document.createElement('div'); const messageDivContainer = document.createElement('div'); messageDivContainer.className = 'message-container bot-message-container destructive-confirmation-container'; messageDiv.className = 'message bot-message destructive-confirmation-message'; const confirmationId = 'confirmation-' + Date.now(); - 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; + // Build confirmation UI safely + const wrapper = document.createElement('div'); + wrapper.className = 'destructive-confirmation'; + wrapper.setAttribute('data-confirmation-id', confirmationId); + const graphAtPrompt = DOM.graphSelect?.value || ''; + if (graphAtPrompt) wrapper.setAttribute('data-graph', graphAtPrompt); + + const text = document.createElement('div'); + text.className = 'confirmation-text'; + const rawMessage = String(step.message || ''); + rawMessage.split('\n').forEach((line, idx) => { + if (idx) text.appendChild(document.createElement('br')); + text.appendChild(document.createTextNode(line)); + }); + + 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', String(step.sql_query || ''), confirmationId) + ); + + const cancelBtn = document.createElement('button'); + cancelBtn.className = 'cancel-btn'; + cancelBtn.textContent = 'CANCEL - Abort Operation'; + cancelBtn.addEventListener('click', () => + handleDestructiveConfirmation('CANCEL', String(step.sql_query || ''), confirmationId) + ); + + btns.appendChild(confirmBtn); + btns.appendChild(cancelBtn); + wrapper.appendChild(text); + wrapper.appendChild(btns); + messageDiv.replaceChildren(wrapper);Also applies to: 217-231, 231-239
🧹 Nitpick comments (15)
app/.eslintrc.cjs (4)
3-7: Enable type-aware linting and future-proof the parser optionsType-aware rules significantly improve correctness during a migration. Point ESLint to the dedicated project file and bump ecmaVersion to 'latest'.
parserOptions: { - ecmaVersion: 2020, - sourceType: 'module' + ecmaVersion: 'latest', + sourceType: 'module', + // enable type-aware rules + tsconfigRootDir: __dirname, + project: ['./tsconfig.eslint.json'] },
8-9: Adopt typed rule sets for stronger correctnessOnce parserOptions.project is set, consider the typed configs for better insights (e.g., promises, unsafe calls).
- extends: ['eslint:recommended', 'plugin:@typescript-eslint/recommended'], + extends: [ + 'eslint:recommended', + 'plugin:@typescript-eslint/recommended', + // typed + stylistic (requires parserOptions.project) + 'plugin:@typescript-eslint/recommended-type-checked', + 'plugin:@typescript-eslint/stylistic-type-checked' + ],
10-13: Don’t blanket-disableany; downgrade to warn and add consistency ruleTurning it off globally hides useful signals during migration. A warning keeps momentum without losing visibility. Also enforce consistent type-only imports to cut bundle size and improve readability.
rules: { // customize as needed - '@typescript-eslint/no-explicit-any': 'off' + '@typescript-eslint/no-explicit-any': ['warn', { ignoreRestArgs: true }], + '@typescript-eslint/consistent-type-imports': ['error', { prefer: 'type-imports' }] }
1-14: Add env and ignorePatterns to reduce lint noiseExplicit env avoids undefined-global warnings for browser/Node. Ignoring build artifacts keeps lint runs fast and focused.
module.exports = { root: true, + env: { + es2021: true, + browser: true, + node: true + }, + ignorePatterns: ['dist/', 'build/', 'coverage/', 'node_modules/'], parser: '@typescript-eslint/parser',Makefile (2)
45-49: Prefer $(MAKE) for recursive invocation; consider failing on lint.Use
$(MAKE)to propagate make flags. Optional: drop|| trueso CI fails on lint errors.lint: ## Run linting (backend + frontend) @echo "Running backend lint (pylint)" pipenv run pylint $(shell git ls-files '*.py') || true @echo "Running frontend lint (eslint)" - make lint-frontend + $(MAKE) lint-frontend
65-70: Expose a simple “run” alias to meet the documented run workflow.Guidelines call for a top-level
run. Map it to dev by default.run-dev: build-dev ## Run development server pipenv run python -m flask --app api.index run --debug run-prod: build-prod ## Run production server pipenv run python -m flask --app api.index run + +# Public run alias (default to dev) +run: run-devapp/ts/modules/messages.ts (3)
31-37: A11y: Use full name for avatar alt text, avoid unsafe castAlt text should describe the image for screen readers; a single initial is not helpful, and the cast can mask undefined at compile time. Prefer the user’s name with a safe fallback.
Apply:
- userAvatar.alt = (userInfo.name?.charAt(0).toUpperCase() as string) || 'User'; + // Use the full name when available; otherwise a sensible fallback + userAvatar.alt = userInfo.name ?? 'User avatar';
92-131: formatBlock heuristics are brittle; consider stronger delimiters and batching DOM writes
- Array detection via presence of '[' and ']' will trigger on many non-array texts.
- Multiple appends in a loop can cause needless reflows.
Suggestions:
- Gate “array block” only when the text starts and ends with brackets or is JSON-parsable.
- Build into a DocumentFragment and append once to reduce layout thrashing.
133-141: Avoid innerHTML for clearing; use replaceChildren/textContentStatic analysis flags innerHTML (even when empty). It’s safe here but easy to regress. Prefer safer DOM methods.
- if (DOM.chatMessages) DOM.chatMessages.innerHTML = ''; + DOM.chatMessages?.replaceChildren(); @@ - [DOM.confValue, DOM.expValue, DOM.missValue].forEach((element) => { - if (element) element.innerHTML = ''; - }); + [DOM.confValue, DOM.expValue, DOM.missValue].forEach((el) => el?.replaceChildren());Also applies to: 136-138
app/ts/modules/modals.ts (3)
72-79: On cancel, also clear URL input for a clean resetMinor UX nit: clearing the URL field avoids stale values when reopening.
cancelDbModalBtn.addEventListener('click', function() { dbModal.style.display = 'none'; if (dbTypeSelect) dbTypeSelect.selectedIndex = 0; if (connectDbModalBtn) connectDbModalBtn.disabled = true; + if (dbUrlInput) { + dbUrlInput.value = ''; + dbUrlInput.placeholder = 'Select database type first...'; + dbUrlInput.disabled = true; + } });
86-91: Use keydown instead of keypress (deprecated) for Enter handlingThe keypress event is deprecated and may not fire consistently for non-printable keys.
- dbUrlInput.addEventListener('keypress', function(e) { - if (e.key === 'Enter') { + dbUrlInput.addEventListener('keydown', function(e) { + if (e.key === 'Enter') { connectDbModalBtn.click(); } });
116-121: Optional: Include explicit DB type in POST payloadThe current backend route (
api/routes/database.py) accepts a JSON payload and only reads theurlfield, inferring the database type from the URL prefix (postgresql://ormysql://). It ignores any additional properties, so sending atypefield won’t break the contract:
- The handler calls
data = request.get_json()and then only usesdata.get("url")in all checks.- URL prefix logic handles type detection; unknown keys in the payload are silently ignored.
- No changes are required on the server to accept extra fields, but the handler does not yet make use of them.
If you prefer an explicit
typerather than relying on URL parsing, you can optionally refactor both frontend and backend:
Frontend diff (in both occurrences at lines 96–113 and 116–121 of
app/ts/modules/modals.ts):- body: JSON.stringify({ url: dbUrl }) + body: JSON.stringify({ url: dbUrl, type: selectedType })Backend update (example in
api/routes/database.py):- url = data.get("url") if data else None + url = data.get("url") if data else None + db_type = data.get("type") if data else None # then branch on db_type if provided, falling back to URL prefix - if url.startswith("postgres://") or url.startswith("postgresql://"): + if db_type === "postgresql" or url.startswith("postgres://") or url.startswith("postgresql://"): success, result = PostgresLoader.load(g.user_id, url)This is purely an optional refactor for clarity and stronger typing; the existing URL-only approach remains valid.
app/ts/modules/chat.ts (3)
32-34: Avoid innerHTML clears in metrics panesSafer and a tiny perf win to clear via replaceChildren.
- [DOM.confValue, DOM.expValue, DOM.missValue, DOM.ambValue].forEach((element) => { - if (element) element.innerHTML = ''; - }); + [DOM.confValue, DOM.expValue, DOM.missValue, DOM.ambValue].forEach((el) => el?.replaceChildren());
101-106: Don’t echo unparsed payloads back to users on parse failureDisplaying raw chunks can confuse users and leak implementation details. Prefer a generic error and log the details.
- } catch (e) { - addMessage('Failed: ' + message, false); - } + } catch (e) { + console.warn('Stream message parse failed:', { message, error: e }); + addMessage('Received an unreadable message chunk from the server.', false, true); + }
136-160: Clean up list rebuilding and avoid innerHTMLYou clear element.innerHTML earlier; the subsequent getElementById calls are redundant. Also prefer replaceChildren.
- element.innerHTML = ''; - let ul = document.getElementById(`${element.id}-list`) as HTMLUListElement | null; - - ul = document.createElement('ul'); + element.replaceChildren(); + const ul = document.createElement('ul'); ul.className = `final-result-list`; ul.id = `${element.id}-list`; element.appendChild(ul); @@ - let li = document.getElementById(`${element.id}-${i}-li`); - - li = document.createElement('li'); + const li = document.createElement('li'); li.id = `${element.id}-${i}-li`; ul.appendChild(li);Also applies to: 141-147, 149-159
📜 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 (8)
Makefile(3 hunks)app/.eslintrc.cjs(1 hunks)app/package.json(1 hunks)app/ts/modules/chat.ts(5 hunks)app/ts/modules/messages.ts(4 hunks)app/ts/modules/modals.ts(5 hunks)app/ts/modules/schema.ts(1 hunks)app/tsconfig.eslint.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/tsconfig.eslint.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/package.json
- app/ts/modules/schema.ts
🧰 Additional context used
📓 Path-based instructions (1)
Makefile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Expose developer workflows via Makefile targets (install, setup-dev, test, lint, run)
Files:
Makefile
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to Makefile : Expose developer workflows via Makefile targets (install, setup-dev, test, lint, run)
Applied to files:
Makefile
🧬 Code graph analysis (2)
app/ts/modules/chat.ts (2)
app/ts/modules/config.ts (2)
DOM(40-66)state(74-78)app/ts/modules/messages.ts (2)
addMessage(7-74)removeLoadingMessage(76-81)
app/ts/modules/messages.ts (1)
app/ts/modules/config.ts (2)
state(74-78)DOM(40-66)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
🪛 ast-grep (0.38.6)
app/ts/modules/chat.ts
[warning] 32-32: 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)
[warning] 140-140: 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)
[warning] 230-230: 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: messageDiv.innerHTML = confirmationHTML
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] 230-230: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: messageDiv.innerHTML = confirmationHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
app/ts/modules/messages.ts
[warning] 134-134: 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] 136-136: 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). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (13)
app/.eslintrc.cjs (2)
1-14: Solid baseline TS ESLint config for the app/Rooted config, TS parser, recommended presets, and the plugin are correctly wired for the TS migration. Good starting point.
1-14: ESLint configuration verified—please update ignorePatterns with your build output
- ✅ Typed ESLint project file found at
app/tsconfig.eslint.json.- ✅ Required ESLint packages are present in
app/package.json:
@typescript-eslint/parser@typescript-eslint/eslint-plugineslint- ⚙️
tsconfig.jsonspecifiescompilerOptions.outDir: "./public/js"— add"public/js"(relative toapp/) to theignorePatternsarray inapp/.eslintrc.cjs.- 🔍 No
--outdirflags were automatically detected in your npm scripts. Manually review any build or bundler scripts (e.g. esbuild, webpack) inapp/package.jsonfor other output directories you may also want to ignore.Makefile (5)
34-35: E2E dependency on build-dev looks good.
38-39: Headed E2E config is sensible.
42-43: E2E debug target is helpful for local triage.
51-53: Frontend lint wiring via npm --prefix is correct.
65-66: Dev server depends on a dev build — good for DX.app/ts/modules/messages.ts (3)
70-71: Nice: DOM guards prevent null derefsUsing optional chaining for append and scroll is a solid TS/DOM safety improvement. No action needed.
140-144: Double-check greeting gating logicCondition
(DOM.graphSelect.options[0].value || DOM.graphSelect.options.length)is truthy whenever there’s at least one option, even if it’s a placeholder. Verify this matches UX (show generic hello only if a real, non-placeholder option is present/selected).If the goal is “show hello only when a non-empty option exists,” consider:
- if (DOM.graphSelect && DOM.graphSelect.options.length > 0 && (DOM.graphSelect.options[0].value || DOM.graphSelect.options.length)) { + if (DOM.graphSelect && Array.from(DOM.graphSelect.options).some(o => o.value && !o.disabled)) {
76-90: LGTM: loading-message utilities guarded and robustThe presence checks and re-append logic are sound.
app/ts/modules/modals.ts (1)
21-31: LGTM: setLoadingState is typed and null-safeClear separation of concerns and safe guards around sub-elements.
app/ts/modules/chat.ts (2)
284-286: Retain global exposure until inline handlers are migratedInline
onclick="handleDestructiveConfirmation(…)"references still exist in app/ts/modules/chat.ts (at lines 221 and 224), so the global binding is currently required. Remove it only after you have:
- Replaced all inline onclick attributes with addEventListener calls.
- Verified that no calls to
window.handleDestructiveConfirmationremain (e.g., viarg -n 'handleDestructiveConfirmation').Once migration is complete, drop the global export and update the comment as follows:
-// Expose global for inline onclick handlers -(window as any).handleDestructiveConfirmation = handleDestructiveConfirmation; +// No global exposure needed; handlers are bound programmatically.
39-49: PII leakage: Don’t send prompts in the URLTo prevent sensitive user messages from being exposed in logs, caches, or proxy layers, remove the
?q=query-param and include the prompt in your JSON body instead.• Location:
app/ts/modules/chat.ts(lines 39–49)
• Proposed client diff:- const response = await fetch( - '/graphs/' + encodeURIComponent(selectedValue) + '?q=' + encodeURIComponent(message), - { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ - chat: state.questions_history, - result: state.result_history, - instructions: DOM.expInstructions?.value - }), signal: state.currentRequestController.signal } - ); + const response = await fetch( + '/graphs/' + encodeURIComponent(selectedValue), + { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ + q: message, + chat: state.questions_history, + result: state.result_history, + instructions: DOM.expInstructions?.value + }), signal: state.currentRequestController.signal } + );I ran:
rg -n -C3 --glob '!**/dist/**' --glob '!**/node_modules/**' \ -e '/graphs/.*\?q=' -e 'request\.(args|get_json|get_data)' -e 'q\s*[:=]'and found no server-side reads of
q(neither in query nor in JSON bodies). Please verify that your/graphs/:idendpoint is updated to extractqfromrequest.body. If it isn’t, you’ll need to add that support on the server before merging this client change.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/wordlist.txt (1)
39-40: Nit: collapse double blank line.Two consecutive blank lines don’t add structure here and slightly increase churn in future diffs. Prefer a single separator line.
Apply this diff:
- - +
📜 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 (2)
.github/wordlist.txt(1 hunks).github/workflows/tests.yml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/tests.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the main test CI workflow in .github/workflows/tests.yml
Files:
.github/workflows/tests.yml
🧠 Learnings (2)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to .github/workflows/e2e-tests.yml : Use a dedicated E2E CI workflow in .github/workflows/e2e-tests.yml
Applied to files:
.github/workflows/tests.yml
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to .github/workflows/tests.yml : Keep the main test CI workflow in .github/workflows/tests.yml
Applied to files:
.github/workflows/tests.yml
🪛 actionlint (1.7.7)
.github/workflows/tests.yml
45-45: undefined function "exists". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
66-66: undefined function "exists". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
⏰ 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)
.github/wordlist.txt (1)
41-46: LGTM — new TS/frontend tokens make sense for the move to a TypeScript UI.These additions will reduce false positives from the spell checker as you reference the new frontend stack.
| TypeScript | ||
| frontend | ||
| Frontend | ||
| js | ||
| npm | ||
| deps No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm case-sensitivity and related variants covered by your spell-checker.
- If the checker is case-sensitive (common with cspell), keeping both “frontend” and “Frontend” is correct. If not, one entry is enough.
- Consider whether “front-end” (hyphenated) appears in docs; if yes, you may want to whitelist it too to avoid new CI noise.
- Similarly, validate whether “JS” (uppercase acronym) shows up in docs; if so, whitelist “JS” as well.
I can help tune the list once we know which checker and config are in use.
Run this to detect the spell-checker and validate locally:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Detect common spell-checkers
has_cspell=$(fd -a '^cspell\.(json|yml|yaml)$' -HI 2>/dev/null | wc -l | tr -d ' ')
has_codespell=$(fd -a '^\.codespellrc$' -HI 2>/dev/null | wc -l | tr -d ' ')
echo "cspell configs: $has_cspell, codespell configs: $has_codespell"
if [ "$has_cspell" -gt 0 ]; then
echo "Running cspell..."
npx -y cspell@latest --no-progress --no-must-find-files "**/*" || true
fi
if [ "$has_codespell" -gt 0 ]; then
echo "Running codespell..."
python -m pip install --quiet codespell && \
codespell --ignore-words .github/wordlist.txt || true
fiLength of output: 293
Here’s a script to locate and inspect the spell-check workflow configuration:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the spellcheck workflow file
file=$(fd -HI .github/workflows/spellcheck.yml | head -n1 || echo "")
if [ -n "$file" ]; then
echo "Found: $file"
sed -n '1,200p' "$file"
else
echo "spellcheck workflow not found"
fiLength of output: 167
Add and configure project spell-checker
I didn’t find any spell-checker config (no cspell.json/yml, .codespellrc) or a .github/workflows/spellcheck.yml workflow in the repo. To turn on CI-driven spell checking and make use of .github/wordlist.txt, please:
- Create a GitHub Actions workflow at
.github/workflows/spellcheck.ymlthat installs and runs your chosen tool (e.g. CSpell or codespell). - Add the corresponding config file in the repo root (e.g.
cspell.jsonor.codespellrc) and include.github/wordlist.txtin its ignore/dictionary settings. - After setting up the checker, revisit
.github/wordlist.txtfor case-sensitivity:- If your checker is case-sensitive, keep both “frontend” and “Frontend.”
- If it’s case-insensitive, drop one to avoid redundancy.
- Also consider whitelisting any other common variants used in docs:
- “front-end” (hyphenated)
- “JS” (uppercase acronym)
Let me know if you’d like help authoring the workflow or tuning the config!
| - name: Install frontend dependencies | ||
| if: "exists('app/package.json')" | ||
| run: | | ||
| node --version || true | ||
| npm --version || true | ||
| (cd app && npm ci) | ||
|
|
There was a problem hiding this comment.
Fix invalid exists() expression; add setup-node and npm cache for reliability and speed
GitHub Actions doesn’t have an exists() function (actionlint flags this). Use hashFiles() to conditionally run steps when app/package.json is present. Also, don’t rely on runner-provided Node; set it up explicitly and enable npm cache to speed CI. Remove the no-op Node version checks and use working-directory instead of subshells.
Apply:
- - name: Install frontend dependencies
- if: "exists('app/package.json')"
- run: |
- node --version || true
- npm --version || true
- (cd app && npm ci)
+ - name: Set up Node.js (frontend)
+ if: hashFiles('app/package.json') != ''
+ uses: actions/setup-node@v4
+ with:
+ # Prefer the project's engines field; if unset, 20 is a safe default LTS.
+ node-version: 20
+ cache: npm
+ cache-dependency-path: app/package-lock.json
+
+ - name: Install frontend dependencies
+ if: hashFiles('app/package.json') != ''
+ working-directory: app
+ run: npm ci📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install frontend dependencies | |
| if: "exists('app/package.json')" | |
| run: | | |
| node --version || true | |
| npm --version || true | |
| (cd app && npm ci) | |
| - name: Set up Node.js (frontend) | |
| if: hashFiles('app/package.json') != '' | |
| uses: actions/setup-node@v4 | |
| with: | |
| # Prefer the project's engines field; if unset, 20 is a safe default LTS. | |
| node-version: 20 | |
| cache: npm | |
| cache-dependency-path: app/package-lock.json | |
| - name: Install frontend dependencies | |
| if: hashFiles('app/package.json') != '' | |
| working-directory: app | |
| run: npm ci |
🧰 Tools
🪛 actionlint (1.7.7)
45-45: undefined function "exists". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
🤖 Prompt for AI Agents
In .github/workflows/tests.yml around lines 44 to 50, the step uses a
non-existent exists() expression, relies on runner-provided Node via no-op
version checks, and uses a subshell to cd into app; replace the conditional
with: if: "hashFiles('app/package.json') != ''" so the step only runs when
app/package.json exists; add an explicit uses: actions/setup-node@v4 (with
node-version input) before running npm to ensure a known Node is installed;
enable npm caching via the setup-node cache: 'npm' option or add a cache step
for ~/.npm to speed CI; remove the node --version || true and npm --version ||
true no-ops; and run npm ci using working-directory: app instead of a subshell.
| - name: Run frontend lint | ||
| if: "exists('app/package.json')" | ||
| run: | | ||
| (cd app && npm run lint) | ||
|
|
There was a problem hiding this comment.
Use hashFiles() in if and simplify the lint step
Same issue as above: exists() is not a valid expression function. Also, prefer working-directory over subshell.
- - name: Run frontend lint
- if: "exists('app/package.json')"
- run: |
- (cd app && npm run lint)
+ - name: Run frontend lint
+ if: hashFiles('app/package.json') != ''
+ working-directory: app
+ run: npm run lint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run frontend lint | |
| if: "exists('app/package.json')" | |
| run: | | |
| (cd app && npm run lint) | |
| - name: Run frontend lint | |
| if: hashFiles('app/package.json') != '' | |
| working-directory: app | |
| run: npm run lint |
🧰 Tools
🪛 actionlint (1.7.7)
66-66: undefined function "exists". available functions are "always", "cancelled", "contains", "endswith", "failure", "format", "fromjson", "hashfiles", "join", "startswith", "success", "tojson"
(expression)
🤖 Prompt for AI Agents
In .github/workflows/tests.yml around lines 65 to 69, the step uses the invalid
exists() expression and a subshell to change directories; update the if to check
file existence via GitHub Actions' hashFiles (e.g. if: ${{
hashFiles('app/package.json') != '' }}) and remove the subshell by adding
working-directory: app and set run: npm run lint so the step only runs when
app/package.json exists and uses the working-directory instead of (cd ...).
2 participants
Summary by CodeRabbit
New Features
Style
Documentation
Chores