Conversation
PR should go to staging
update deps
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMigrates backend from Flask to FastAPI (ASGI), rewrites OAuth/auth flows and route handlers to FastAPI routers/middleware, replaces MySQL driver with PyMySQL, updates templates/static path resolutions, enhances landing UI and typing demo, modernizes frontend TypeScript imports and graph loading, updates dependencies and CI/Makefile, adjusts tests, and sets Dependabot target branch to staging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant A as FastAPI App
participant O as Authlib OAuth
participant S as Session (Cookie)
participant T as Templates
C->>A: GET /
A->>S: Check request.session.user_info
alt Unauthenticated
A->>T: Render landing.j2 (typing demo)
T-->>C: Landing page
else Authenticated
A->>T: Render chat.j2
T-->>C: Chat page
end
C->>A: GET /login/google
A->>O: Build auth redirect (callback URL)
O-->>C: Redirect to provider
C->>A: GET /login/google/authorized?code=...
A->>O: Exchange code for token & userinfo
A->>S: Store token + user_info
A-->>C: Redirect /chat
sequenceDiagram
autonumber
participant C as Client
participant A as FastAPI (graphs_router)
participant L as Loader (DB)
participant AI as LLM/Agents
C->>A: POST /graphs/:id/query (ChatRequest)
A->>AI: Start relevancy & SQL gen (streamed)
par Table discovery
A->>L: Inspect schema (timeout)
L-->>A: Tables/metadata
end
A->>AI: Generate SQL
A-->>C: Stream JSON chunks (steps)
alt Needs confirmation
C->>A: POST /graphs/:id/confirm (ConfirmRequest)
alt CONFIRM
A->>L: Execute SQL
L-->>A: Results
A->>AI: Summarize
A-->>C: Stream results + summary
else Cancel
A-->>C: Stream operation_cancelled
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency ReviewThe following issues were found:
|
fix landing page demo
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
app/public/css/landing.css (1)
36-38: Use a themable success color and add reduced-motion fallback
- Hard-coding
#10B981breaks theme consistency (you’re using CSS vars elsewhere). Prefer a theme var with a fallback.- Consider respecting user reduced-motion preferences for the typing/caret UI.
Apply:
-.demo-success{display:flex;align-items:center;gap:0.5rem;color:#10B981;font-size:0.9rem;font-weight:600} +.demo-success{display:flex;align-items:center;gap:0.5rem;color:var(--success, #10B981);font-size:0.9rem;font-weight:600}Additionally (outside changed lines), consider:
@media (prefers-reduced-motion: reduce){ .demo-sql.typing::after{ animation: none; } }app/templates/landing.j2 (6)
29-40: Use semantics for the success badge and avoid inline styles
- Replace inline
style="display:none"toggling with the semantichiddenattribute or a CSS class.- Expose completion state to assistive tech via
role="status" aria-live="polite".Apply:
- <div class="demo-sql-header"> + <div class="demo-sql-header"> <div class="demo-label">Generated SQL:</div> - <div class="demo-success" style="display:none;"> + <div class="demo-success" role="status" aria-live="polite" hidden> <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> <circle cx="8" cy="8" r="8" fill="#10B981"/> <path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/> </svg> <span>Success</span> </div> </div>Update JS toggles accordingly in the comments below.
66-66: Guard exists; consider caching nullability with a data-attribute for testabilityYou already guard
successEl. If you want easier integration/UI tests, give the element adata-testid="demo-success"; no functional change, just improves E2E selectors.
78-78: Prefer attribute/class toggling to inline styles for success stateUse
hiddento show/hide, which plays nicely with CSS and SSR, and is terser in JS.Apply:
- if(successEl) successEl.style.display = 'flex'; + if(successEl) successEl.hidden = false;
83-85: Stop previous typing before early returns; toggle success viahiddenIf
sElwere ever missing, the early return would skip clearing an existing timer. Move the timer clear before thesElnull-guard and switch tohiddentoggling.Apply:
- function typeSql(text){ - if(!sEl) return; - // hide success while typing - if(successEl) successEl.style.display = 'none'; - // stop any previous typing - if(typingTimer) { - clearInterval(typingTimer); - typingTimer = null; - } + function typeSql(text){ + // stop any previous typing + if(typingTimer) { + clearInterval(typingTimer); + typingTimer = null; + } + if(!sEl) return; + // hide success while typing + if(successEl) successEl.hidden = true;
100-102: Show success usinghidden = false; optionally delay to avoid blink on short textsFor very short SQL, the badge may flicker. A tiny delay can smooth this, but not required.
Apply:
- // show success when typing completes - if(successEl) successEl.style.display = 'flex'; + // show success when typing completes + if(successEl) successEl.hidden = false; + // Optional: small delay + // setTimeout(() => successEl && (successEl.hidden = false), 50);
116-119: Respect prefers-reduced-motion for the initial renderWhen users prefer reduced motion, render instantly instead of typing.
Apply:
- // initial render: use typing animation for first example too - const firstExample = examples[0]; - if(qEl) qEl.textContent = firstExample.q; - typeSql(firstExample.sql); + // initial render: prefer reduced motion + const firstExample = examples[0]; + if(qEl) qEl.textContent = firstExample.q; + if (window.matchMedia && window.matchMedia('(prefers-reduced-motion: reduce)').matches) { + renderFull(0); + } else { + typeSql(firstExample.sql); + }Optional: disable the “Try another question” button while typing to prevent rapid re-queues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
app/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
.github/dependabot.yml(1 hunks)app/package.json(1 hunks)app/public/css/landing.css(1 hunks)app/templates/landing.j2(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
🔇 Additional comments (1)
app/public/css/landing.css (1)
42-42: Minor spacing tweak looks goodThe reduced top margin on
.demo-sqltightens the header/SQL grouping appropriately. No issues.
| target-branch: "staging" | ||
| schedule: | ||
| interval: "weekly" | ||
| - package-ecosystem: "npm" | ||
| directory: "/app" | ||
| target-branch: "staging" | ||
| schedule: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Targeting staging is sensible; verify branch protections and automations
Directing Dependabot PRs to staging is aligned with your flow. Ensure:
- CI required checks are configured for
staging. - Any auto-merge rules or Renovate-equivalent workflows are updated to act on
staging. - Backports/merges from
stagingtomainare scheduled to avoid security updates stalling onmain.
Quick checks to confirm:
🏁 Script executed:
#!/bin/bash
# Ensure branch protection and CI for staging
gh api repos/:owner/:repo/branches/staging/protection || echo "No branch protection on staging"
rg -nP -C2 'on:\s*\n\s*push:.*branches:.*\[[^\]]*staging' -g '.github/workflows/*.yml' || echo "No workflows triggering on staging push"
rg -nP -C2 'on:\s*pull_request:.*branches:.*\[[^\]]*staging' -g '.github/workflows/*.yml' || echo "No PR workflows for staging"Length of output: 982
🏁 Script executed:
#!/bin/bash
set -e
echo "Listing workflow directory contents:"
find .github/workflows -maxdepth 1 -type f
echo -e "\nFinding all YAML workflow files:"
find .github/workflows -type f \( -iname '*.yml' -o -iname '*.yaml' \)
echo -e "\nSearching for ‘staging’ references in workflow files:"
rg -n 'staging' .github/workflows || echo "No references to 'staging' found in workflows"Length of output: 954
CI and Branch Protection for staging Are Missing – Action Required
Our scan shows:
- The GitHub API query for branch protection on
stagingreturned a 403, indicating it’s either unprotected or inaccessible by your integration. - A ripgrep search across all workflows in
.github/workflows/found no references tostaging, so no CI runs or PR checks are configured for that branch.
Please address the following before merging Dependabot PRs into staging:
• Configure branch protection for staging (at minimum, require status checks).
• Update your workflows in .github/workflows/ to include both push and pull_request triggers on staging:
- Add under
on.push.branches:andon.pull_request.branches:the entry"staging".
• Review any auto-merge or Renovate/Dependabot automations to ensure they targetstaging.
• Put in place a clear process or scheduled job to backport security updates fromstagingintomainso hotfixes don’t stall.
Once these are in place, your weekly Dependabot PRs to staging will be properly verified and merged—and any fixes will flow safely to main.
🤖 Prompt for AI Agents
.github/dependabot.yml lines 10-16: branch protection and CI for the staging
branch are missing; add/configure branch protection for "staging" (require
status checks and PR reviews) in the repo settings or via repo protection
config, update all workflows in .github/workflows/ to include "staging" under
both on.push.branches and on.pull_request.branches so CI runs for pushes and PRs
to staging, review Dependabot/auto-merge/Renovate settings to ensure their
target-branch is "staging" (not bypassing protections), and add a
documented/backport process or scheduled job to propagate security fixes from
staging into main.
| "esbuild": "^0.25.9", | ||
| "typescript": "^5.9.2", | ||
| "eslint": "^9.33.0", | ||
| "@typescript-eslint/parser": "^8.40.0", | ||
| "@typescript-eslint/eslint-plugin": "^8.40.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Major tooling upgrades (ESLint v9, TS v5.9, TSESLint v8): check config and engines before merging
- ESLint 9 defaults to the flat config and drops legacy config loaders. If you're still on .eslintrc*, the lint script may no-op or fail silently (script already swallows failures via
|| true). Recommend adding aneslint.config.{js|mjs}at /app and removing|| trueonce green. - @typescript-eslint v8 is aligned to ESLint 9 and TS ≥5.4; verify rule names haven’t changed and that parser/plugin versions match.
- Consider pinning Node engine to avoid CI skew; these versions typically expect Node 18+ (prefer 20+).
Add a minimal flat config and Node engines:
{
"name": "queryweaver-app",
"version": "0.0.1",
"private": true,
+ "engines": {
+ "node": ">=18.18.0"
+ },
"scripts": {
"build": "NODE_ENV=production esbuild ts/app.ts --bundle --outfile=./public/js/app.js --platform=browser --minify",
"build:dev": "NODE_ENV=development 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",
- "lint": "eslint --ext .ts ./ts --max-warnings=0 || true"
+ "lint": "eslint --max-warnings=0"
},Example flat config at app/eslint.config.js:
// app/eslint.config.js
import tseslint from '@typescript-eslint/eslint-plugin'
import tsParser from '@typescript-eslint/parser'
export default [
{
files: ['ts/**/*.ts'],
languageOptions: { parser: tsParser, parserOptions: { project: false } },
plugins: { '@typescript-eslint': tseslint },
rules: {
'@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],
},
},
]Run to confirm config presence and lintability:
🏁 Script executed:
#!/bin/bash
# Check for flat config and legacy configs
fd -H -a 'eslint.config.*' app || true
fd -H -a '.eslintrc*' . || true
# Try a no-file lint to ensure eslint resolves config
npx --yes eslint -v
npx --yes eslint app/ts --ext .ts --print-config app/ts/app.ts >/dev/null && echo "ESLint config OK"Length of output: 257
🏁 Script executed:
#!/bin/bash
# Locate the package.json file
pkg=$(find . -maxdepth 2 -type f -name 'package.json' | head -n 1)
echo "Using package.json: $pkg"
jq '.scripts.lint, .engines' "$pkg"
# Check for flat ESLint config in root or subdirectories
fd -H -a 'eslint.config.*' . || true
# Check for any legacy .eslintrc* files
fd -H -a '.eslintrc*' . || trueLength of output: 371
Ensure ESLint v9 migration & Node engine pinning
It looks like you’re still on a legacy .eslintrc.cjs (found at the repo root) and haven’t yet added a flat config or pinned your Node version. Without these changes, your lint script will silently no-op (due to || true), and CI jobs may run on unexpected Node versions.
• Migrate to ESLint v9 flat config
– Create app/eslint.config.js (or .mjs) with at least a minimal rule set, then remove /home/jailuser/git/app/.eslintrc.cjs once migration is verified.
• Update the lint script in app/package.json
– Remove --ext .ts ./ts (flat config will pick up ts/ by default) and drop the || true so failures fail the CI.
• Pin your Node engine
– Add an engines field to app/package.json to ensure CI uses Node ≥18 (we recommend ≥20).
• Verify parser/plugin alignment
– Confirm that @typescript-eslint/parser and @typescript-eslint/eslint-plugin v8 options/rule names still match your existing config and TS 5.9 setup.
Suggested diff for app/package.json:
"name": "queryweaver-app",
"version": "0.0.1",
"private": true,
+ "engines": {
+ "node": ">=18.18.0"
+ },
"scripts": {
"build": "NODE_ENV=production esbuild ts/app.ts --bundle --outfile=./public/js/app.js --platform=browser --minify",
"build:dev": "NODE_ENV=development 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",
- "lint": "eslint --ext .ts ./ts --max-warnings=0 || true"
+ "lint": "eslint --max-warnings=0"
},And add at app/eslint.config.js:
import tsParser from '@typescript-eslint/parser'
import tsPlugin from '@typescript-eslint/eslint-plugin'
export default [
{
files: ['ts/**/*.ts'],
languageOptions: { parser: tsParser, parserOptions: { project: false } },
plugins: { '@typescript-eslint': tsPlugin },
rules: {
'@typescript-eslint/no-unused-vars': ['warn', { argsIgnorePattern: '^_' }],
},
},
]🤖 Prompt for AI Agents
In app/package.json around lines 12-16, the project still relies on a legacy
.eslintrc.cjs and a lint script that silently succeeds; migrate to ESLint v9 by
adding a flat config file at app/eslint.config.js (or .mjs) with a minimal
ruleset that registers @typescript-eslint parser and plugin for ts/**/*.ts,
remove the repo root .eslintrc.cjs after verifying the flat config works, update
the package.json lint script to remove the explicit --ext .ts ./ts and drop the
trailing "|| true" so lint failures fail CI, add an "engines" field to
package.json to pin Node (e.g., ">=18" or ">=20"), and confirm that
@typescript-eslint/parser and @typescript-eslint/eslint-plugin v8 rule
names/options are compatible with TypeScript 5.9.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/templates/landing.j2 (2)
39-39: Announce SQL typing to screen readersThe SQL output changes dynamically; expose it as a live region so the typed text is announced without requiring focus.
- <pre class="demo-sql"></pre> + <pre class="demo-sql" aria-live="polite" aria-atomic="true"></pre>
116-119: Respect reduced-motion preference on initial renderOffer an instant render path for users with “Reduce motion” enabled and keep typing for others. This also gives
renderFulla clear purpose.- // initial render: use typing animation for first example too - const firstExample = examples[0]; - if(qEl) qEl.textContent = firstExample.q; - typeSql(firstExample.sql); + // initial render: honor reduced-motion + const firstExample = examples[0]; + if(qEl) qEl.textContent = firstExample.q; + const reduceMotion = window.matchMedia && window.matchMedia('(prefers-reduced-motion: reduce)').matches; + if (reduceMotion) { + renderFull(0); + } else { + typeSql(firstExample.sql); + }
📜 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)
app/public/css/landing.css(1 hunks)app/templates/landing.j2(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/public/css/landing.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-tests
| <div class="hero-ctas"> | ||
| <a href="{{ url_for('auth.chat') }}" class="btn-pill">Sign Up for Free</a> | ||
| <a href="{{ url_for('auth.login_google') }}" class="btn-ghost">Sign In</a> | ||
| <a href="{{ url_for('auth.chat') }}" class="btn-ghost">Sign In</a> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm Sign In CTA route (auth.chat) is correct for returning users
Both CTAs now point to auth.chat. If auth.chat requires an authenticated session or leads to onboarding, returning users clicking “Sign In” may hit the wrong flow. Please verify there’s a dedicated login endpoint and wire this CTA to it.
Run this to locate available auth endpoints and usages:
If a dedicated login exists, consider:
- <a href="{{ url_for('auth.chat') }}" class="btn-ghost">Sign In</a>
+ <a href="{{ url_for('auth.login') }}" class="btn-ghost">Sign In</a>🏁 Script executed:
#!/bin/bash
# Inspect blueprint declarations and routes
rg -n -C3 -S "Blueprint\(['\"]auth['\"]"
rg -n -C2 -S "def chat\("
rg -n -C2 -S "url_for\('auth\."
# Look for canonical login routes we might prefer
rg -n -C2 -S "def login\("
rg -n -C2 -S "login_google" Length of output: 1310
Wire “Sign In” CTA to the dedicated login route
Currently both CTAs in app/templates/landing.j2 point at auth.chat (the chat UI, which in turn gates on authentication). We expose a login endpoint via auth.login_google (@auth_bp.route("/login")), so the “Sign In” button should target that directly to initiate the OAuth flow rather than sending unauthenticated users to /chat.
• File: app/templates/landing.j2 (line 19)
- <a href="{{ url_for('auth.chat') }}" class="btn-ghost">Sign In</a>
+ <a href="{{ url_for('auth.login_google') }}" class="btn-ghost">Sign In</a>🤖 Prompt for AI Agents
In app/templates/landing.j2 around line 19 the "Sign In" anchor targets
url_for('auth.chat') (the chat UI) instead of the dedicated login route; change
the href to url_for('auth.login_google') (the @auth_bp.route("/login") endpoint)
so the button initiates the OAuth/login flow directly for unauthenticated users.
| <div class="demo-sql-header"> | ||
| <div class="demo-label">Generated SQL:</div> | ||
| <div class="demo-success" style="display:none;"> | ||
| <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> | ||
| <circle cx="8" cy="8" r="8" fill="#10B981"/> | ||
| <path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/> | ||
| </svg> | ||
| <span>Success</span> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the success badge accessible and avoid inline styles
Use semantic visibility with hidden and announce status changes to assistive tech via role="status" and aria-live. This also removes inline styles.
- <div class="demo-sql-header">
+ <div class="demo-sql-header">
<div class="demo-label">Generated SQL:</div>
- <div class="demo-success" style="display:none;">
+ <div class="demo-success" hidden role="status" aria-live="polite" aria-atomic="true">
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg">
<circle cx="8" cy="8" r="8" fill="#10B981"/>
<path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/>
</svg>
<span>Success</span>
</div>
</div>📝 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.
| <div class="demo-sql-header"> | |
| <div class="demo-label">Generated SQL:</div> | |
| <div class="demo-success" style="display:none;"> | |
| <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> | |
| <circle cx="8" cy="8" r="8" fill="#10B981"/> | |
| <path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/> | |
| </svg> | |
| <span>Success</span> | |
| </div> | |
| <div class="demo-sql-header"> | |
| <div class="demo-label">Generated SQL:</div> | |
| <div class="demo-success" hidden role="status" aria-live="polite" aria-atomic="true"> | |
| <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> | |
| <circle cx="8" cy="8" r="8" fill="#10B981"/> | |
| <path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/> | |
| </svg> | |
| <span>Success</span> | |
| </div> | |
| </div> |
🤖 Prompt for AI Agents
In app/templates/landing.j2 around lines 29 to 37, the success badge uses an
inline style to hide it and lacks ARIA roles; replace the inline
style="display:none;" with the semantic hidden attribute (or remove the
attribute to show), add role="status" and aria-live="polite" on the success
container so screen readers are notified when it becomes visible, and ensure the
SVG has aria-hidden="true" (or an accessible title) to avoid redundant
announcements; update any JS that toggles visibility to set/remove the hidden
attribute rather than manipulating inline styles.
| let idx = 0; | ||
| const qEl = document.querySelector('.demo-question'); | ||
| const sEl = document.querySelector('.demo-sql'); | ||
| const successEl = document.querySelector('.demo-success'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace inline display toggles with the hidden property and set aria-busy during typing
Using hidden is semantically correct and keeps styling in CSS. Toggling aria-busy helps AT know content is updating.
- const successEl = document.querySelector('.demo-success');
+ const successEl = document.querySelector('.demo-success');
@@
- if(successEl) successEl.style.display = 'flex';
+ if(successEl) successEl.hidden = false;
@@
- // hide success while typing
- if(successEl) successEl.style.display = 'none';
+ // hide success while typing
+ if(successEl) successEl.hidden = true;
+ // mark region busy for AT
+ sEl.setAttribute('aria-busy', 'true');
@@
- // show success when typing completes
- if(successEl) successEl.style.display = 'flex';
+ // show success when typing completes
+ if(successEl) successEl.hidden = false;
+ sEl.removeAttribute('aria-busy');Also applies to: 79-79, 83-85, 100-101
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return Response(stream_with_context(generate()), content_type="application/json") | ||
| return StreamingResponse(generate(), media_type="application/json") | ||
|
|
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, we need to ensure that detailed exception messages (including those from database drivers) are not sent to the user in API responses. Instead, we should log the detailed error on the server and return a generic error message to the user. Specifically:
- In
api/loaders/mysql_loader.py, update theloadmethod so that when an exception occurs, the detailed error is logged, but the returned message is a generic one (e.g., "MySQL connection error" or "Error loading MySQL schema"). - Similarly, in the
refresh_graph_schemamethod, ensure that only generic error messages are returned to the user, while the detailed error is logged. - In
api/routes/graphs.py, no changes are needed to the error handling logic, as it will now receive only generic error messages from the loader.
This approach preserves the current functionality (the API still indicates failure and success), but prevents sensitive information from being exposed to the user.
| @@ -162,10 +162,11 @@ | ||
| f"Found {len(entities)} tables.") | ||
|
|
||
| except pymysql.MySQLError as e: | ||
| return False, f"MySQL connection error: {str(e)}" | ||
| logging.error("MySQL connection error: %s", str(e)) | ||
| return False, "MySQL connection error" | ||
| except Exception as e: | ||
| return False, f"Error loading MySQL schema: {str(e)}" | ||
|
|
||
| logging.error("Error loading MySQL schema: %s", str(e)) | ||
| return False, "Error loading MySQL schema" | ||
| @staticmethod | ||
| def extract_tables_info(cursor, db_name: str) -> Dict[str, Any]: | ||
| """ | ||
| @@ -445,10 +445,7 @@ | ||
| except Exception as e: | ||
| # Log the error and return failure | ||
| logging.error("Error refreshing graph schema: %s", str(e)) | ||
| error_msg = "Error refreshing graph schema" | ||
| logging.error(error_msg) | ||
| return False, error_msg | ||
|
|
||
| return False, "Error refreshing graph schema" | ||
| @staticmethod | ||
| def execute_sql_query(sql_query: str, db_url: str) -> List[Dict[str, Any]]: | ||
| """ |
|
|
||
| return Response(stream_with_context(generate_confirmation()), content_type="application/json") | ||
| return StreamingResponse(generate_confirmation(), media_type="application/json") | ||
|
|
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, we need to ensure that any error messages returned from the loader methods (such as MySQLLoader.load and MySQLLoader.refresh_graph_schema) are not directly exposed to the user. Instead, only a generic error message should be sent in the API response, while the detailed error (including the original exception message) should be logged on the server.
Best approach:
- In
api/routes/graphs.py, within theconfirm_destructive_operationroute, when yielding an error message to the user (specifically in the"schema_refresh"block whenrefresh_successisFalse), replace the use ofrefresh_messagewith a generic error message. - Log the detailed
refresh_messageon the server using theloggingmodule. - No changes are needed in the loader itself, as the loader should still return detailed messages for logging purposes.
Files/regions to change:
- In
api/routes/graphs.py, inside thegenerate_confirmationfunction ofconfirm_destructive_operation, update the error response in the"schema_refresh"block to not exposerefresh_messageto the user, and log it instead.
What is needed:
- Use the
loggingmodule to log the detailed error. - Replace the user-facing error message with a generic one.
| @@ -573,11 +573,11 @@ | ||
| } | ||
| ) + MESSAGE_DELIMITER | ||
| else: | ||
| logging.error("Schema was modified but graph refresh failed: %s", refresh_message) | ||
| yield json.dumps( | ||
| { | ||
| "type": "schema_refresh", | ||
| "message": (f"⚠️ Schema was modified but graph refresh failed: " | ||
| f"{refresh_message}"), | ||
| "message": "⚠️ Schema was modified but graph refresh failed due to an internal error.", | ||
| "refresh_status": "failed" | ||
| } | ||
| ) + MESSAGE_DELIMITER |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/loaders/mysql_loader.py (1)
141-156: Ensure connection/cursor are closed on all failure paths (wrap with try/finally).If any exception occurs after connecting (e.g., while extracting entities/relationships), the current code returns early and skips closing the connection/cursor. Wrap connection/cursor usage in try/finally or use context managers to guarantee cleanup.
Here’s a minimal refactor confined to this block:
- conn = pymysql.connect(**conn_params) - cursor = conn.cursor(DictCursor) + conn = pymysql.connect(**conn_params) + try: + cursor = conn.cursor(DictCursor) + try: # Get database name db_name = conn_params['database'] # Get all table information entities = MySQLLoader.extract_tables_info(cursor, db_name) # Get all relationship information relationships = MySQLLoader.extract_relationships(cursor, db_name) - # Close database connection - cursor.close() - conn.close() + finally: + cursor.close() + finally: + conn.close()
♻️ Duplicate comments (3)
app/templates/landing.j2 (3)
19-19: Wire “Sign In” to the login route (not the chat UI).This mirrors the earlier review: the “Sign In” CTA should initiate OAuth, not send unauthenticated users to /chat. Given your current route names, point it to google.login (or to a generic auth.login if you add that name on /login).
Apply:
- <a href="{{ url_for('auth.chat') }}" class="btn-ghost">Sign In</a> + <a href="{{ url_for('google.login') }}" class="btn-ghost">Sign In</a>
29-37: Make the success badge accessible; avoid inline styles.Replace display:none with the semantic hidden attribute and add ARIA. Also mark the SVG decorative.
Apply:
- <div class="demo-success" style="display:none;"> - <svg width="16" height="16" viewBox="0 0 16 16" fill="none" xmlns="http://www.w3.org/2000/svg"> + <div class="demo-success" hidden role="status" aria-live="polite" aria-atomic="true"> + <svg width="16" height="16" viewBox="0 0 16 16" fill="none" aria-hidden="true" xmlns="http://www.w3.org/2000/svg"> <circle cx="8" cy="8" r="8" fill="#10B981"/> <path d="M5.5 8L7 9.5L10.5 6" stroke="white" stroke-width="2" stroke-linecap="round" stroke-linejoin="round"/> </svg> <span>Success</span> </div>
65-65: Use the hidden property and aria-busy for better semantics instead of inline styles.Toggle success visibility with hidden and indicate typing state with aria-busy for assistive tech.
Apply:
- const successEl = document.querySelector('.demo-success'); + const successEl = document.querySelector('.demo-success'); @@ - if(successEl) successEl.style.display = 'flex'; + if(successEl) successEl.hidden = false; @@ - // hide success while typing - if(successEl) successEl.style.display = 'none'; + // hide success while typing + if(successEl) successEl.hidden = true; + sEl.setAttribute('aria-busy', 'true'); @@ - // show success when typing completes - if(successEl) successEl.style.display = 'flex'; + // show success when typing completes + if(successEl) successEl.hidden = false; + sEl.removeAttribute('aria-busy');Also applies to: 78-79, 83-85, 100-101
🧹 Nitpick comments (35)
app/templates/components/chat_header.j2 (1)
3-3: Consistent migration to path= for static assets — good.These changes match the FastAPI/Starlette static routing convention. Keep this consistent repo-wide. If you plan to serve under a non-root prefix (e.g.,
/app), this will continue to work, provided the router mount name remainsstatic.If you want to further harden UX:
- Provide explicit dimensions on icons (reduces CLS).
- Consider
fetchpriority="low"on non-critical decorative icons.No need to block this PR for these.
Also applies to: 23-23, 27-27
app/templates/components/scripts.j2 (1)
17-17: Prefer url_for over hard-coded “/static” path to respect mounts, prefixes, and CDN rewrites.Hard-coding
/static/js/app.jsmay break when the app is:
- served behind a reverse proxy with a path prefix,
- mounted under a sub-application path, or
- using a CDN or hashed asset pipeline.
Use
url_for('static', path='js/app.js')for consistency with other templates.Apply:
-<script type="module" src="/static/js/app.js"></script> +<script type="module" src="{{ url_for('static', path='js/app.js') }}"></script>app/templates/components/login_modal.j2 (1)
7-7: Static icon path migration looks good.This is correct for FastAPI/Starlette. No functional concerns.
If you want micro-optimizations:
- Add
decoding="async"on these small SVGs to avoid any decode-blocking on slower devices, though impact is minimal.Also applies to: 10-10
Makefile (2)
66-66: Dev server switch to Uvicorn is appropriate. Consider parameterizing host/port.Great for local dev with
--reload. To improve flexibility across environments (containers, devboxes), parameterize host/port via variables.Apply this change to use Make variables:
-pipenv run uvicorn api.index:app --host 127.0.0.1 --port 5000 --reload +pipenv run uvicorn api.index:app --host $(HOST) --port $(PORT) --reloadAnd define defaults near the top of the Makefile:
# Defaults (override via: make run-dev HOST=0.0.0.0 PORT=8000) HOST ?= 127.0.0.1 PORT ?= 5000
69-69: Enhance production Uvicorn invocation
I verified that the ASGI app is exposed viaapp = create_app()inapi/index.pyand thatuvicornis declared inPipfile.lock. The existing Makefile target works, but for deployments behind a proxy and to leverage multiple workers, consider the following optional hardening:Proposed diff (in your Makefile):
--- Makefile +++ Makefile @@ –1,3 +1,6 @@ +# Default server settings +HOST ?= 127.0.0.1 +PORT ?= 5000 +WORKERS ?= 1 run-dev: - pipenv run uvicorn api.index:app --host 127.0.0.1 --port 5000 + pipenv run uvicorn api.index:app \ + --host $(HOST) \ + --port $(PORT) \ + --proxy-headers \ + --workers $(WORKERS) + +run: run-dev ## alias for starting the development server
--proxy-headersensures correct client IP and scheme when behind Nginx/Ingress.--workers $(WORKERS)lets you scale to multiple processes for higher concurrency.- Using
HOST,PORT, andWORKERSvariables makes it easy to override in different environments.- Adding a
runalias aligns with common “run” workflow guidelines.These changes are purely optional but recommended for production readiness.
Pipfile.new (1)
10-10: Optional: use uvicorn[standard] for better prod defaults.The “standard” extras enable uvloop, httptools, etc. Consider aligning with the main Pipfile which uses uvicorn without extras.
- uvicorn = {extras = ["standard"], version = "~=0.32.0"} + uvicorn = "~=0.32.0" # or keep extras if you preferapp/templates/base.j2 (1)
19-21: Optional: add SRI for third-party scripts or self-host to reduce supply-chain risk.For https://unpkg.com/d3 and force-graph, either add integrity/crossorigin or serve pinned versions from your static bucket.
api/loaders/mysql_loader.py (1)
164-164: Surface PyMySQL errors while still cleaning up; consider logging the original error.Catching pymysql.MySQLError is right, but pair it with the above cleanup changes. Also include the DB name/host in the log (excluding credentials) to aid debugging.
tests/test_mysql_loader.py (1)
115-135: Consider adding tests for execute_sql_query (SELECT and DML) and rollback on errors.Given the driver switch and the fixed DictCursor usage, add focused tests that validate:
- SELECT returns list[dict] with serialized values
- INSERT/UPDATE/DELETE return the status payload and commit is called
- Rollback and cleanup on MySQLError
I can add a small test module stubbing cursor.description/rowcount paths if you want.
Pipfile (2)
8-8: Optional: consider uvicorn[standard] for production performance.Not required, but adopting extras=[standard] includes uvloop/httptools for a measurable boost in prod.
- uvicorn = "~=0.32.0" + uvicorn = {extras = ["standard"], version = "~=0.32.0"}
7-19: Don’t add a direct starlette pin here; let FastAPI manage it.FastAPI’s own dependency range will ensure a compatible (and, after the update, patched) Starlette gets installed. (fastapi.tiangolo.com)
api/auth/user_management.py (3)
8-11: Remove unused imports (Ruff F401).
requestsandJSONResponseare imported but never used.Apply:
-import requests -from fastapi.responses import JSONResponse from authlib.integrations.starlette_client import OAuth
239-249: GitHub email retrieval: use profile email first, fall back to /user/emails.A direct call to
/user/emailsrequiresuser:emailscope and may fail. If the profile already carries a public email, prefer that before making the extra API call.- # Get primary email - email_resp = await oauth.github.get("/user/emails", token=github_token) - email = None - if email_resp.status_code == 200: - for email_obj in email_resp.json(): - if email_obj.get("primary", False): - email = email_obj.get("email") - break - if not email and email_resp.json(): - email = email_resp.json()[0].get("email") + # Get email: prefer profile email, fall back to /user/emails + email = github_user.get("email") + if not email: + email_resp = await oauth.github.get("/user/emails", token=github_token) + if email_resp.status_code == 200: + for email_obj in email_resp.json(): + if email_obj.get("primary", False): + email = email_obj.get("email") + break + if not email and email_resp.json(): + email = email_resp.json()[0].get("email")Also applies to: 256-265
312-320: Preserve original exception context when converting to HTTP 401 (B904).Raising
HTTPException“from e” keeps cause in logs while FastAPI still returns sanitized JSON to clients.- except Exception as e: + except Exception as e: logging.error("Unexpected error in token_required: %s", e) request.session.pop("user_info", None) - raise HTTPException( + raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, - detail="Unauthorized - Authentication error" - ) + detail="Unauthorized - Authentication error" + ) from eapi/routes/graphs.py (4)
8-12: Trim unused imports to quiet linters and reduce noise.
Dict,Any,status, andFormaren’t used in this module.-from typing import Dict, Any +from typing import Optional, List - -from fastapi import APIRouter, Request, HTTPException, status, UploadFile, File, Form +from fastapi import APIRouter, Request, HTTPException, UploadFile, File from fastapi.responses import JSONResponse, StreamingResponse -from pydantic import BaseModel +from pydantic import BaseModel, Field
34-38: Avoid mutable defaults in Pydantic models; useField(default_factory=...).Mutable defaults can be shared across instances. Safer to use
default_factory.-class ChatRequest(BaseModel): - chat: list - result: list = None - instructions: str = None +class ChatRequest(BaseModel): + chat: List[str] + result: Optional[List[str]] = None + instructions: Optional[str] = None @@ -class ConfirmRequest(BaseModel): - sql_query: str - confirmation: str = "" - chat: list = [] +class ConfirmRequest(BaseModel): + sql_query: str + confirmation: str = "" + chat: List[str] = Field(default_factory=list)Also applies to: 40-44
61-67: Broaden DB URL detection to support common SQLAlchemy-style MySQL URLs.Support
mysql+pymysql://andmysql+mysqlconnector://in addition tomysql://.- elif db_url_lower.startswith('mysql://'): + elif ( + db_url_lower.startswith('mysql://') + or db_url_lower.startswith('mysql+pymysql://') + or db_url_lower.startswith('mysql+mysqlconnector://') + ): return 'mysql', MySQLLoader
504-505: Consider a streaming-friendly media type.You’re delimiting multiple JSON objects with a custom boundary.
application/x-ndjsonortext/event-streamtypically plays nicer with streaming parsers. If the frontend expectsapplication/json, ignore this.Would you like me to switch both
StreamingResponsemedia_type values toapplication/x-ndjsonand update the client accordingly?Also applies to: 620-621
api/index.py (1)
12-18: Make host/port configurable via environment variables.Keeps local default behavior but simplifies containerized and cloud deploys.
- uvicorn.run( - "api.index:app", - host="127.0.0.1", - port=5000, - reload=debug_mode, - log_level="info" if debug_mode else "warning" - ) + host = os.environ.get("HOST", "127.0.0.1") + port = int(os.environ.get("PORT", "5000")) + uvicorn.run( + "api.index:app", + host=host, + port=port, + reload=debug_mode, + log_level="info" if debug_mode else "warning", + )api/routes/__init__.py (1)
1-1: Add a module docstring to satisfy Pylint (C0114).Minor polish; no functional impact.
-# Routes module for text2sql API +"""Expose FastAPI routers for the text2sql API."""api/auth/oauth_handlers.py (3)
4-7: Prune unused imports and fix linter warnings.
timeandrequestsare unused.-import time -from typing import Dict, Any - -import requests +from typing import Dict, Any
14-21: Type return values and split long signature to satisfy Pylint; mark unused args.
- Add return type to
setup_oauth_handlers.- Break long function signatures onto multiple lines.
- Prefix unused parameters with
_to address Pylint’sunused-argument.-def setup_oauth_handlers(app: FastAPI, oauth: OAuth): +def setup_oauth_handlers(app: FastAPI, oauth: OAuth) -> None: @@ - async def handle_google_callback(request: Request, token: Dict[str, Any], user_info: Dict[str, Any]): + async def handle_google_callback( + _request: Request, + _token: Dict[str, Any], + user_info: Dict[str, Any], + ) -> bool: @@ - async def handle_github_callback(request: Request, token: Dict[str, Any], user_info: Dict[str, Any]): + async def handle_github_callback( + _request: Request, + _token: Dict[str, Any], + user_info: Dict[str, Any], + ) -> bool:
38-41: Narrow exception handling where feasible; at least preserve cause.Catching broad
Exceptionis sometimes necessary in auth flows, but prefer preserving context withfrom eso logs keep the root cause.- except Exception as e: - logging.error("Error handling Google OAuth callback: %s", e) - return False + except Exception as e: + logging.error("Error handling Google OAuth callback: %s", e) + return False # consider re-raising or returning details internally @@ - except Exception as e: - logging.error("Error handling GitHub OAuth callback: %s", e) - return False + except Exception as e: + logging.error("Error handling GitHub OAuth callback: %s", e) + return FalseIf desired, we can instead re-raise using custom exceptions consumed by the caller.
Also applies to: 60-63
api/routes/database.py (2)
3-8: Remove unused imports; add run_in_threadpool for blocking calls.Clean up unused types/status and prepare for the next comment’s threadpool usage.
Apply:
-from typing import Dict, Any +from typing import Dict, Any @@ -from fastapi import APIRouter, Request, HTTPException, status +from fastapi import APIRouter, Request, HTTPException from fastapi.responses import JSONResponse from pydantic import BaseModel +from starlette.concurrency import run_in_threadpoolNote: Dict/Any appear unused; if so, also remove them:
-from typing import Dict, Any +# no typing imports needed
47-47: Preserve exception context when re-raising.Chain exceptions to distinguish handler failures from original errors.
Apply:
- raise HTTPException(status_code=500, detail="Failed to connect to PostgreSQL database") + raise HTTPException(status_code=500, detail="Failed to connect to PostgreSQL database") from e @@ - raise HTTPException(status_code=500, detail="Failed to connect to MySQL database") + raise HTTPException(status_code=500, detail="Failed to connect to MySQL database") from e @@ - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from eAlso applies to: 56-56, 73-73
api/app_factory.py (5)
11-11: Remove unused imports to satisfy Ruff and keep things tidy.Jinja2Templates and setup_oauth_handlers aren’t used here.
Apply:
-from fastapi.templating import Jinja2Templates @@ -from api.auth.oauth_handlers import setup_oauth_handlersAlso applies to: 15-15
45-46: Wrap long FastAPI constructor line to satisfy Pylint and readability.Break the long description into a multiline call.
Apply:
- app = FastAPI(title="QueryWeaver", description="Text2SQL with Graph-Powered Schema Understanding") + app = FastAPI( + title="QueryWeaver", + description="Text2SQL with Graph-Powered Schema Understanding", + )
54-61: Make session cookie ‘secure’ configurable.Defaulting https_only=False is fine for dev, but allow enabling via env in staging/prod.
Apply:
- https_only=False, # allow http on localhost in development + https_only=os.getenv("SESSION_COOKIE_SECURE", "false").lower() in ("1", "true", "yes"),Optionally also set same_site from env for stricter deployments.
31-39: Minor: avoid custom 400 in static middleware; let StaticFiles handle it.Raising 400 for paths ending with “/” changes semantics; StaticFiles returns 404. Consider logging and returning the next handler to keep behavior consistent.
79-87: Prefer typed handlers over string matching for OAuth errors.String checks for “token”/“oauth” are brittle. Register a handler for AuthlibBaseError in auth.py (or import here) and keep this catch-all minimal.
Example:
from authlib.common.errors import AuthlibBaseError @app.exception_handler(AuthlibBaseError) async def handle_authlib_error(request: Request, exc: AuthlibBaseError): logging.warning("OAuth error occurred: %s", exc) request.session.clear() return RedirectResponse(url="/", status_code=302)api/routes/auth.py (5)
89-92: Name the generic login endpoint for consistency with template url_for usage.Add a route name (e.g., auth.login) so templates can use url_for('auth.login'). This pairs with the landing “Sign In” CTA change.
Apply:
-@auth_router.get("/login", response_class=RedirectResponse) +@auth_router.get("/login", name="auth.login", response_class=RedirectResponse) async def login_page(_: Request) -> RedirectResponse: return RedirectResponse(url="/login/google", status_code=status.HTTP_302_FOUND)
99-106: Break long warning strings to satisfy Pylint and readability.These warnings exceed the 100-char limit.
Apply:
- logging.warning( - "OAUTH_BASE_URL not set and base URL is 127.0.0.1; " - "if your Google OAuth app uses 'http://localhost:5000', " - "set OAUTH_BASE_URL=http://localhost:5000 to avoid redirect_uri mismatch." - ) + logging.warning( + "OAUTH_BASE_URL not set and base URL is 127.0.0.1; " + "if your Google OAuth app uses 'http://localhost:5000', " + "set OAUTH_BASE_URL=http://localhost:5000 " + "to avoid redirect_uri mismatch." + ) @@ - logging.warning( - "OAUTH_BASE_URL not set and base URL is 127.0.0.1; " - "if your GitHub OAuth app uses 'http://localhost:5000', " - "set OAUTH_BASE_URL=http://localhost:5000 to avoid redirect_uri mismatch." - ) + logging.warning( + "OAUTH_BASE_URL not set and base URL is 127.0.0.1; " + "if your GitHub OAuth app uses 'http://localhost:5000', " + "set OAUTH_BASE_URL=http://localhost:5000 " + "to avoid redirect_uri mismatch." + )Also applies to: 160-165
233-257: Send Google token in form-encoded body, not as query params, for revocation endpoint.Per Google’s spec, token should be in application/x-www-form-urlencoded body. Using params may still work but is non-canonical.
Apply:
- resp = await client.post( - "https://oauth2.googleapis.com/revoke", - params={"token": token}, - headers={"content-type": "application/x-www-form-urlencoded"}, - ) + resp = await client.post( + "https://oauth2.googleapis.com/revoke", + data={"token": token}, + headers={"content-type": "application/x-www-form-urlencoded"}, + )
35-39: Break long list literal across lines to satisfy Pylint limit.The pop-list on Line 37 likely triggers the 100-char rule.
Apply:
- for key in ["user_info", "google_token", "github_token", "token_validated_at", "oauth_google_auth"]: + for key in [ + "user_info", + "google_token", + "github_token", + "token_validated_at", + "oauth_google_auth", + ]: session.pop(key, None)
94-98: Unify route naming under an “auth.” namespace for consistency.You’ve named chat as auth.chat, but login routes are google.login/github.login. Consider auth.login_google/auth.login_github (and auth.login as the generic), to keep template usage consistent.
If you adopt this, also update the landing CTA to url_for('auth.login') or url_for('auth.login_google').
Also applies to: 154-158
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
Makefile(1 hunks)Pipfile(1 hunks)Pipfile.new(1 hunks)api/app_factory.py(1 hunks)api/auth/oauth_handlers.py(1 hunks)api/auth/user_management.py(2 hunks)api/index.py(1 hunks)api/loaders/mysql_loader.py(5 hunks)api/routes/__init__.py(1 hunks)api/routes/auth.py(1 hunks)api/routes/database.py(2 hunks)api/routes/graphs.py(9 hunks)app/templates/base.j2(2 hunks)app/templates/components/chat_header.j2(2 hunks)app/templates/components/chat_input.j2(1 hunks)app/templates/components/login_modal.j2(1 hunks)app/templates/components/scripts.j2(1 hunks)app/templates/landing.j2(7 hunks)app/ts/app.ts(2 hunks)app/ts/modules/chat.ts(1 hunks)app/ts/modules/graphs.ts(1 hunks)app/ts/modules/messages.ts(1 hunks)app/ts/modules/ui.ts(2 hunks)tests/test_mysql_loader.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/ts/modules/ui.ts
🧰 Additional context used
📓 Path-based instructions (11)
api/index.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the Flask application entry point at api/index.py
Files:
api/index.py
api/routes/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/__init__.pyapi/routes/graphs.pyapi/routes/database.pyapi/routes/auth.py
tests/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name Python unit tests as tests/test_*.py
Files:
tests/test_mysql_loader.py
api/loaders/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place data loading utilities under api/loaders/
Files:
api/loaders/mysql_loader.py
api/auth/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place authentication modules under api/auth/
Files:
api/auth/oauth_handlers.pyapi/auth/user_management.py
Makefile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Expose developer workflows via Makefile targets (install, setup-dev, test, lint, run)
Files:
Makefile
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/database.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep database management route handlers in api/routes/database.py
Files:
api/routes/database.py
Pipfile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manage Python dependencies with Pipenv using Pipfile
Files:
Pipfile
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
api/routes/auth.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep authentication route handlers in api/routes/auth.py
Files:
api/routes/auth.py
🧠 Learnings (6)
📚 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 Pipfile : Manage Python dependencies with Pipenv using Pipfile
Applied to files:
Pipfile.new
📚 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/index.py : Keep the Flask application entry point at api/index.py
Applied to files:
api/index.py
📚 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/routes/graphs.py : Keep graph/database route handlers in api/routes/graphs.py
Applied to files:
api/routes/__init__.pyapi/routes/graphs.py
📚 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/routes/database.py : Keep database management route handlers in api/routes/database.py
Applied to files:
api/routes/__init__.pyapi/routes/database.py
📚 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/auth/oauth_handlers.pyapi/app_factory.pyapi/routes/auth.py
📚 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/routes/auth.py : Keep authentication route handlers in api/routes/auth.py
Applied to files:
api/routes/auth.py
🧬 Code graph analysis (5)
api/auth/oauth_handlers.py (1)
api/auth/user_management.py (1)
ensure_user_in_organizations(16-124)
api/routes/graphs.py (7)
api/auth/user_management.py (1)
token_required(280-322)api/loaders/json_loader.py (1)
JSONLoader(23-71)api/loaders/odata_loader.py (1)
ODataLoader(11-135)api/loaders/csv_loader.py (1)
CSVLoader(13-233)api/agents/relevancy_agent.py (1)
RelevancyAgent(57-90)api/agents/analysis_agent.py (2)
AnalysisAgent(9-201)get_analysis(14-49)api/graph.py (2)
find(56-116)get_db_description(38-53)
api/routes/database.py (3)
api/auth/user_management.py (1)
token_required(280-322)api/loaders/postgres_loader.py (2)
PostgresLoader(18-491)load(67-108)api/loaders/mysql_loader.py (2)
MySQLLoader(20-535)load(125-167)
api/app_factory.py (2)
api/auth/oauth_handlers.py (1)
setup_oauth_handlers(14-66)api/routes/auth.py (1)
init_auth(273-307)
api/routes/auth.py (1)
api/auth/user_management.py (1)
validate_and_cache_user(162-278)
🪛 GitHub Actions: Pylint
api/routes/__init__.py
[warning] 1-1: C0114: Missing module docstring (missing-module-docstring)
api/auth/oauth_handlers.py
[warning] 16-16: pylint: Trailing whitespace (trailing-whitespace).
[warning] 19-19: pylint: Trailing whitespace (trailing-whitespace).
[warning] 20-20: pylint: Line too long (105/100) (line-too-long).
[warning] 42-42: pylint: Line too long (105/100) (line-too-long).
[warning] 63-63: pylint: Trailing whitespace (trailing-whitespace).
[warning] 38-38: pylint: Catching too general exception (broad-exception-caught).
[warning] 20-20: pylint: Unused argument 'request' (unused-argument).
[warning] 60-60: pylint: Catching too general exception (broad-exception-caught).
[warning] 42-42: pylint: Unused argument 'request' (unused-argument).
[warning] 42-42: pylint: Unused argument 'token' (unused-argument).
[warning] 4-4: pylint: Unused import time (unused-import).
[warning] 7-7: pylint: Unused import requests (unused-import).
api/app_factory.py
[warning] 38-38: C0303: Trailing whitespace (trailing-whitespace)
[warning] 45-45: C0301: Line too long (102/100) (line-too-long)
api/routes/auth.py
[warning] 37-37: C0301: Line too long (104/100) (line-too-long)
🪛 Ruff (0.12.2)
api/loaders/mysql_loader.py
516-516: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
522-522: try-except block with duplicate exception pymysql.MySQLError
(B025)
api/auth/oauth_handlers.py
7-7: requests imported but unused
Remove unused import: requests
(F401)
api/routes/graphs.py
8-8: typing.Dict imported but unused
Remove unused import
(F401)
8-8: typing.Any imported but unused
Remove unused import
(F401)
10-10: fastapi.status imported but unused
Remove unused import
(F401)
10-10: fastapi.Form imported but unused
Remove unused import
(F401)
194-194: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
api/auth/user_management.py
8-8: requests imported but unused
Remove unused import: requests
(F401)
10-10: fastapi.responses.JSONResponse imported but unused
Remove unused import: fastapi.responses.JSONResponse
(F401)
317-320: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
api/routes/database.py
3-3: typing.Dict imported but unused
Remove unused import
(F401)
3-3: typing.Any imported but unused
Remove unused import
(F401)
5-5: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
47-47: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
56-56: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
73-73: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
api/app_factory.py
11-11: fastapi.templating.Jinja2Templates imported but unused
Remove unused import: fastapi.templating.Jinja2Templates
(F401)
15-15: api.auth.oauth_handlers.setup_oauth_handlers imported but unused
Remove unused import: api.auth.oauth_handlers.setup_oauth_handlers
(F401)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 111-111: Log Injection
This log entry depends on a user-provided value.
[warning] 504-504: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 620-620: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[failure] 661-661: Log Injection
This log entry depends on a user-provided value.
🪛 GitHub Actions: Dependency review
Pipfile
[error] 1-1: Dependency review detected vulnerable packages: Pipfile.lock contains starlette@0.46.2 with a moderate severity denial-of-service vulnerability (GHSA-2c2j-9gv5-cj73).
⏰ 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 (22)
app/ts/modules/messages.ts (1)
5-5: LGTM! Import path modernization is consistent.The import path has been updated from
.jsto extensionless format, aligning with modern TypeScript practices and maintaining consistency with other modules in the PR.app/ts/modules/chat.ts (1)
5-6: LGTM! Import paths updated consistently.Both import statements have been modernized to use extensionless paths, which is consistent with the broader frontend refactoring across the TypeScript modules in this PR.
app/ts/modules/graphs.ts (1)
5-6: LGTM! Import path modernization aligns with codebase standards.The extensionless import paths follow modern TypeScript conventions and are consistent with the changes made across other modules in this PR.
app/ts/app.ts (4)
5-21: LGTM! Import path modernization is comprehensive and consistent.All import statements have been updated to use extensionless paths, which aligns with modern TypeScript practices and maintains consistency across the entire frontend codebase refactor.
23-46: Well-structured graph loading function with proper error handling.The
loadAndShowGraphfunction demonstrates good practices:
- Early return for invalid input
- Proper HTTP status checking
- Data shape validation before processing
- Container cleanup before rendering
- Comprehensive error handling with meaningful logging
The function appropriately handles edge cases and provides clear feedback through console logging.
64-70: Efficient async callback integration.The schema button event listener properly integrates the new
loadAndShowGraphfunction with async/await syntax, ensuring the graph data is loaded when the schema container is toggled open.
86-93: Smart conditional graph loading on selection change.The graph select change handler efficiently loads graph data only when the schema container is currently open, avoiding unnecessary network requests and improving performance.
app/templates/components/chat_input.j2 (1)
6-6: StaticFiles url_for validated – ready to merge
- Verified no legacy
url_for('static', filename=…)patterns inapp/templatesremain.- Confirmed
app.mount("/static", StaticFiles(directory=static_path), name="static")in api/app_factory.py:69.- Confirmed
Jinja2Templatesinstantiation (in api/routes/auth.py:21) and use oftemplates.TemplateResponse(lines 44, 70, 79) guaranteesurl_foris exposed in the Jinja context.Optional enhancement: add explicit
width/heightattributes to the<img>tag to reduce layout shift.app/templates/base.j2 (1)
37-37: LGTM: correct Starlette-style url_for usage with path=.The switch from filename= to path= is correct for Starlette/FastAPI static files.
api/loaders/mysql_loader.py (1)
10-12: Migration to PyMySQL + DictCursor looks good.Imports are correct and reflect the driver switch.
tests/test_mysql_loader.py (1)
104-114: LGTM on switching the patch target to pymysql.connect.This aligns tests with the new driver and keeps behavior intact.
Pipfile (2)
12-12: Good: pymysql is present here (consistent with loader/tests).This aligns the runtime with the driver migration.
27-28: Python 3.12 is fine; verify transitive deps (e.g., playwright) work on CI runners for 3.12.Minor note; nothing to change unless CI shows issues.
app/templates/landing.j2 (2)
6-6: Static asset URL looks correct for FastAPI StaticFiles.Using url_for('static', path='css/landing.css') matches the Starlette/FastAPI StaticFiles mount. No action needed.
116-119: Nice UX touch: typing the first example for consistency.The initial render now uses the same typing path as subsequent examples. Simple and cohesive.
api/routes/database.py (2)
20-23: Auth integration via token_required looks correct.Using token_required to populate request.state.user_id aligns with the new auth flow and keeps the route lean.
44-45: Confirmed: Loader.load methods are static – no instantiation requiredVerified via ripgrep that both PostgresLoader.load and MySQLLoader.load are decorated with
@staticmethod, so calling them directly on the class in api/routes/database.py will work as written. No changes to instantiation or signatures are needed.api/app_factory.py (1)
75-78: Routers are mounted in the app factory per guidelines.Auth, graphs (with /graphs prefix), and database routers are included correctly.
api/routes/auth.py (4)
20-22: Templates wiring for FastAPI is correct.Computing TEMPLATES_DIR and using Jinja2Templates is fine; resolves to app/templates.
40-51: Chat route looks good and returns consistent template context.Renders chat.j2 with is_authenticated and user_info; pairs with home route behavior.
272-308: Auth initialization belongs here and is wired correctly.init_auth(app) registers Google and GitHub providers and exposes oauth via app.state. Good separation from the app factory.
61-87: Home route behavior reads well.Clearing auth session keys when unauthenticated and rendering landing.j2 vs chat.j2 is straightforward.
| if resp.status_code == 200: | ||
| google_user = resp.json() | ||
| # Validate required fields | ||
| if not google_user.get("id") or not google_user.get("email"): | ||
| logging.warning("Invalid Google user data received") | ||
| session.clear() | ||
| request.session.pop("google_token", None) | ||
| request.session.pop("user_info", None) | ||
| return None, False | ||
|
|
||
| # Normalize user info structure | ||
| # Normalize | ||
| user_info = { | ||
| "id": str(google_user.get("id")), # Ensure string type | ||
| "id": str(google_user.get("id")), | ||
| "name": google_user.get("name", ""), | ||
| "email": google_user.get("email"), | ||
| "picture": google_user.get("picture", ""), | ||
| "provider": "google" | ||
| "provider": "google", | ||
| } | ||
| session["user_info"] = user_info | ||
| session["token_validated_at"] = current_time | ||
| request.session["user_info"] = user_info | ||
| request.session["token_validated_at"] = current_time |
There was a problem hiding this comment.
Google userinfo: use sub (OIDC) fallback to avoid false auth failures.
Google’s OIDC userinfo commonly uses sub as the stable subject identifier; relying on id alone will intermittently fail and cause unnecessary 401s. Normalize to prefer sub and fallback to id. Also keep the rest of the logic unchanged.
- if resp.status_code == 200:
- google_user = resp.json()
- if not google_user.get("id") or not google_user.get("email"):
+ if resp.status_code == 200:
+ google_user = resp.json()
+ google_id = google_user.get("sub") or google_user.get("id")
+ if not google_id or not google_user.get("email"):
logging.warning("Invalid Google user data received")
request.session.pop("google_token", None)
request.session.pop("user_info", None)
return None, False
# Normalize
user_info = {
- "id": str(google_user.get("id")),
+ "id": str(google_id),
"name": google_user.get("name", ""),
"email": google_user.get("email"),
"picture": google_user.get("picture", ""),
"provider": "google",
}Also applies to: 212-217
🤖 Prompt for AI Agents
In api/auth/user_management.py around lines 202 to 219 (and also apply same
change to 212-217), the code currently checks google_user.get("id") for a stable
identifier which can be missing; change the validation and normalization to
prefer the OIDC "sub" claim and fall back to "id": compute a stable_id =
google_user.get("sub") or google_user.get("id"), ensure stable_id exists (and
convert to str), keep the existing email check, then set user_info["id"] =
str(stable_id) and preserve the rest of the user_info fields and session
assignments exactly as before.
| # Rollback in case of error | ||
| if 'conn' in locals(): | ||
| conn.rollback() |
There was a problem hiding this comment.
Bug: PyMySQL doesn’t support cursor(dictionary=True); use DictCursor (or pass cursorclass at connect).
This line will raise TypeError. Switch to DictCursor.
- conn = pymysql.connect(**conn_params)
- cursor = conn.cursor(dictionary=True)
+ conn = pymysql.connect(**conn_params)
+ cursor = conn.cursor(DictCursor)Optionally set it at connect-time:
conn = pymysql.connect(**conn_params, cursorclass=DictCursor)
cursor = conn.cursor()For reference on DictCursor usage, see PyMySQL examples. (stackoverflow.com, devdungeon.com)
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 472-472: SQL query built from user-controlled sources
This SQL query depends on a user-provided value.
🤖 Prompt for AI Agents
In api/loaders/mysql_loader.py around lines 470 to 472, the code calls
conn.cursor(dictionary=True) which PyMySQL does not support and will raise a
TypeError; replace this by using pymysql.cursors.DictCursor (either by passing
cursorclass=DictCursor to pymysql.connect or by importing DictCursor and calling
conn.cursor(DictCursor)), and update imports accordingly so cursor.fetchall()
returns dict rows without runtime errors.
api/loaders/mysql_loader.py
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate exception blocks + swallowed error; unify handling and always close safely.
There are two consecutive except pymysql.MySQLError blocks; the first swallows the error and makes the second unreachable, and tools flagged it. Also guard cursor.close() when cursor may not exist. Refactor to a single handler that rolls back, closes resources, then raises a descriptive exception.
Apply this unified handling:
- except pymysql.MySQLError as e:
- # Rollback in case of error
- if 'conn' in locals():
- conn.rollback()
- cursor.close()
- conn.close()
- except pymysql.MySQLError as e:
+ except pymysql.MySQLError as e:
# Rollback in case of error
if 'conn' in locals():
conn.rollback()
- cursor.close()
- conn.close()
+ if 'cursor' in locals():
+ try:
+ cursor.close()
+ except Exception:
+ pass
+ try:
+ conn.close()
+ except Exception:
+ pass
raise Exception(f"MySQL query execution error: {str(e)}") from eThis addresses the duplicate-except warning and the unused variable warning reported by static analysis.
Also applies to: 522-529
🧰 Tools
🪛 Ruff (0.12.2)
516-516: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
522-522: try-except block with duplicate exception pymysql.MySQLError
(B025)
🤖 Prompt for AI Agents
In api/loaders/mysql_loader.py around lines 516-529, there are two consecutive
`except pymysql.MySQLError` blocks that cause the first to swallow the error and
make the second unreachable; replace them with a single `except
pymysql.MySQLError as e` handler that: 1) checks for existence and non-None of
`conn` and `cursor` (e.g., `if 'cursor' in locals() and cursor is not None:`)
before calling `cursor.close()` and similarly for `conn` before calling
`conn.rollback()` and `conn.close()`, 2) ensures resources are closed safely
even if one of them was never created, and 3) raises or re-raises a descriptive
exception that includes the original error (do not silently swallow it).
| success, result = PostgresLoader.load(request.state.user_id, url) | ||
| except (ValueError, ConnectionError) as e: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blocking the event loop: run sync DB loaders in a threadpool.
psycopg2 and PyMySQL are synchronous. Calling them directly in an async route will block the event loop. Offload to a threadpool.
Apply:
- success, result = PostgresLoader.load(request.state.user_id, url)
+ loader = PostgresLoader()
+ success, result = await run_in_threadpool(loader.load, request.state.user_id, url)
@@
- success, result = MySQLLoader.load(request.state.user_id, url)
+ loader = MySQLLoader()
+ success, result = await run_in_threadpool(loader.load, request.state.user_id, url)Also applies to: 53-54
🤖 Prompt for AI Agents
In api/routes/database.py around lines 44-45 (and similarly lines 53-54), the
code calls synchronous DB loaders (psycopg2/PyMySQL) directly inside an async
route which will block the event loop; change these calls to execute the loader
functions in a threadpool (e.g., use asyncio.to_thread or loop.run_in_executor)
and await the result so the synchronous PostgresLoader.load (and the other
loader call at 53-54) run off the event loop, propagate exceptions, and return
their values to the async handler.
| if content_type.startswith("application/json"): | ||
| data = request.get_json() | ||
| if not data or "database" not in data: | ||
| return jsonify({"error": "Invalid JSON data"}), 400 | ||
|
|
||
| graph_id = g.user_id + "_" + data["database"] | ||
| success, result = JSONLoader.load(graph_id, data) | ||
|
|
||
| # # ✅ Handle XML Payload | ||
| # elif content_type.startswith("application/xml") or content_type.startswith("text/xml"): | ||
| # xml_data = request.data | ||
| # graph_id = "" | ||
| # success, result = ODataLoader.load(graph_id, xml_data) | ||
|
|
||
| # # ✅ Handle CSV Payload | ||
| # elif content_type.startswith("text/csv"): | ||
| # csv_data = request.data | ||
| # graph_id = "" | ||
| # success, result = CSVLoader.load(graph_id, csv_data) | ||
|
|
||
| # ✅ Handle File Upload (FormData with JSON/XML) | ||
| elif content_type.startswith("multipart/form-data"): | ||
| if "file" not in request.files: | ||
| return jsonify({"error": "No file uploaded"}), 400 | ||
|
|
||
| file = request.files["file"] | ||
| if file.filename == "": | ||
| return jsonify({"error": "Empty file"}), 400 | ||
| if data: | ||
| if not hasattr(data, 'database') or not data.database: | ||
| raise HTTPException(status_code=400, detail="Invalid JSON data") | ||
|
|
||
| graph_id = request.state.user_id + "_" + data.database | ||
| success, result = JSONLoader.load(graph_id, data.dict()) | ||
|
|
||
| # ✅ Handle File Upload | ||
| elif file: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize graph_id derived from payload/filenames and handle loader exceptions.
- Graph IDs built from
data.databaseor filenames are not sanitized, enabling unexpected characters and potential namespace collisions. - Calls to loaders aren’t guarded; e.g., current
CSVLoaderraisesNotImplementedError, which will bubble up as a 500.
Introduce a small sanitizer and wrap loader calls to consistently return 400s on bad inputs.
Use this helper (add near the top of the module):
# helper placed near other utils
import os
import re
GRAPH_ID_MAX_LEN = 100
def sanitize_graph_name(raw: str) -> str:
if not isinstance(raw, str):
raw = str(raw)
# allow alnum, underscore, dash; replace others with underscore
cleaned = re.sub(r"[^A-Za-z0-9_-]+", "_", raw).strip("_")
return cleaned[:GRAPH_ID_MAX_LEN]Apply:
@@
- if data:
- if not hasattr(data, 'database') or not data.database:
+ if data:
+ if not hasattr(data, 'database') or not data.database:
raise HTTPException(status_code=400, detail="Invalid JSON data")
- graph_id = request.state.user_id + "_" + data.database
- success, result = JSONLoader.load(graph_id, data.dict())
+ graph_id = f"{request.state.user_id}_{sanitize_graph_name(data.database)}"
+ try:
+ success, result = JSONLoader.load(graph_id, data.dict())
+ except Exception as e:
+ logging.error("JSON load failed for %s: %s", sanitize_log_input(graph_id), e)
+ raise HTTPException(status_code=400, detail="Failed to load JSON graph data") from e
@@
- elif file:
+ elif file:
content = await file.read()
filename = file.filename
@@
- if filename.endswith(".json"):
+ if filename.endswith(".json"):
try:
data = json.loads(content.decode("utf-8"))
- graph_id = request.state.user_id + "_" + data.get("database", "")
- success, result = JSONLoader.load(graph_id, data)
+ dbname = data.get("database", "")
+ if not dbname:
+ raise HTTPException(status_code=400, detail="Missing 'database' in JSON")
+ graph_id = f"{request.state.user_id}_{sanitize_graph_name(dbname)}"
+ try:
+ success, result = JSONLoader.load(graph_id, data)
+ except Exception as e:
+ logging.error("JSON file load failed for %s: %s", sanitize_log_input(graph_id), e)
+ raise HTTPException(status_code=400, detail="Failed to load JSON file") from e
except json.JSONDecodeError:
raise HTTPException(status_code=400, detail="Invalid JSON file")
@@
- elif filename.endswith(".xml"):
- xml_data = content.decode("utf-8")
- graph_id = request.state.user_id + "_" + filename.replace(".xml", "")
- success, result = ODataLoader.load(graph_id, xml_data)
+ elif filename.endswith(".xml"):
+ xml_data = content.decode("utf-8")
+ base = os.path.splitext(filename)[0]
+ graph_id = f"{request.state.user_id}_{sanitize_graph_name(base)}"
+ try:
+ success, result = ODataLoader.load(graph_id, xml_data)
+ except Exception as e:
+ logging.error("XML load failed for %s: %s", sanitize_log_input(graph_id), e)
+ raise HTTPException(status_code=400, detail="Failed to load XML file") from e
@@
- elif filename.endswith(".csv"):
- csv_data = content.decode("utf-8")
- graph_id = request.state.user_id + "_" + filename.replace(".csv", "")
- success, result = CSVLoader.load(graph_id, csv_data)
+ elif filename.endswith(".csv"):
+ csv_data = content.decode("utf-8")
+ base = os.path.splitext(filename)[0]
+ graph_id = f"{request.state.user_id}_{sanitize_graph_name(base)}"
+ try:
+ success, result = CSVLoader.load(graph_id, csv_data)
+ except Exception as e:
+ logging.error("CSV load failed for %s: %s", sanitize_log_input(graph_id), e)
+ raise HTTPException(status_code=400, detail="Failed to load CSV file") from eAlso applies to: 218-241
| <link rel="stylesheet" href="/static/css/main.css"> | ||
| <link rel="shortcut icon" href="/static/favicon.ico"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use url_for('static', path=...) instead of absolute “/static” to respect SCRIPT_NAME/subpaths and CDN mounts.
Direct “/static/…” breaks when the app is served under a subpath, behind a proxy with path prefix, or with a different static mount. Starlette’s Jinja integration exposes url_for and expects the “path=” kwarg for static. Suggested change:
- <link rel="stylesheet" href="/static/css/main.css">
- <link rel="shortcut icon" href="/static/favicon.ico">
+ <link rel="stylesheet" href="{{ url_for('static', path='css/main.css') }}">
+ <link rel="shortcut icon" href="{{ url_for('static', path='favicon.ico') }}">📝 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.
| <link rel="stylesheet" href="/static/css/main.css"> | |
| <link rel="shortcut icon" href="/static/favicon.ico"> | |
| <link rel="stylesheet" href="{{ url_for('static', path='css/main.css') }}"> | |
| <link rel="shortcut icon" href="{{ url_for('static', path='favicon.ico') }}"> |
🤖 Prompt for AI Agents
In app/templates/base.j2 around lines 7 to 8, the template uses hardcoded
“/static/…” URLs which break when the app is mounted under a subpath or uses a
CDN; replace those href/src values with Jinja url_for calls using the static
endpoint and the path= keyword (e.g. url_for('static', path='css/main.css') and
url_for('static', path='favicon.ico')) so links resolve correctly under
SCRIPT_NAME, proxies, or alternate static mounts.
Pipfile
Outdated
| fastapi = "~=0.115.0" | ||
| uvicorn = "~=0.32.0" | ||
| litellm = "~=1.74.14" | ||
| falkordb = "~=1.2.0" | ||
| flask = "~=3.1.0" | ||
| psycopg2-binary = "~=2.9.9" | ||
| pymysql = "~=1.1.0" | ||
| authlib = "~=1.4.0" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remediate Starlette DoS advisory by bumping FastAPI so it can resolve a patched Starlette (>=0.47.2).
Your CI flagged starlette@0.46.2 (GHSA-2c2j-9gv5-cj73). The fix is in Starlette 0.47.2. FastAPI 0.115.x caps Starlette below 0.47.0 in earlier patches; FastAPI 0.116.1 broadens support to <0.48.0, allowing 0.47.2+. Update FastAPI and refresh the lock to pull a patched Starlette, and do not pin Starlette directly.
- fastapi = "~=0.115.0"
+ fastapi = "~=0.116.1"Then regenerate the lock (so Starlette resolves to >=0.47.2). References: CVE/GHSA advisory (fixed in 0.47.2) and FastAPI release notes expanding Starlette range. (nvd.nist.gov, osv.dev, fastapi.tiangolo.com)
📝 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.
| fastapi = "~=0.115.0" | |
| uvicorn = "~=0.32.0" | |
| litellm = "~=1.74.14" | |
| falkordb = "~=1.2.0" | |
| flask = "~=3.1.0" | |
| psycopg2-binary = "~=2.9.9" | |
| pymysql = "~=1.1.0" | |
| authlib = "~=1.4.0" | |
| [[packages]] | |
| fastapi = "~=0.116.1" | |
| uvicorn = "~=0.32.0" | |
| litellm = "~=1.74.14" | |
| falkordb = "~=1.2.0" | |
| psycopg2-binary = "~=2.9.9" | |
| pymysql = "~=1.1.0" | |
| authlib = "~=1.4.0" |
🤖 Prompt for AI Agents
In Pipfile around lines 7 to 13, CI flagged Starlette v0.46.2
(GHSA-2c2j-9gv5-cj73) which is pulled by FastAPI; update FastAPI to "~=0.116.1"
so its dependency range allows Starlette >=0.47.2, do not pin Starlette
directly, then run pipenv lock (or your dependency manager) to regenerate the
lockfile so Starlette resolves to >=0.47.2 and the advisory is remediated.
Pipfile.new
Outdated
| [[source]] | ||
| url = "https://pypi.org/simple" | ||
| verify_ssl = true | ||
| name = "pypi" | ||
|
|
||
| [packages] | ||
| litellm = {extras = ["bedrock"], version = "~=1.74.14"} | ||
| falkordb = "~=1.2.0" | ||
| fastapi = "~=0.115.0" | ||
| uvicorn = {extras = ["standard"], version = "~=0.32.0"} | ||
| jsonschema = "~=4.25.0" | ||
| tqdm = "~=4.67.1" | ||
| boto3 = "~=1.40.11" | ||
| psycopg2-binary = "~=2.9.9" | ||
| async-timeout = "~=4.0.3" | ||
| mysql-connector-python = "~=9.4.0" | ||
| authlib = "~=1.4.0" | ||
| itsdangerous = "~=2.2.0" | ||
| python-multipart = "~=0.0.10" | ||
| jinja2 = "~=3.1.4" | ||
| starlette = "~=0.40.0" | ||
|
|
||
| [dev-packages] | ||
| pytest = "~=8.4.1" | ||
| pylint = "~=3.3.4" | ||
| playwright = "~=1.47.0" | ||
| pytest-playwright = "~=0.7.0" | ||
| pytest-asyncio = "~=0.24.0" | ||
|
|
||
| [requires] | ||
| python_version = "3.12" |
There was a problem hiding this comment.
Please remove Pipfile.new; manage dependencies only via Pipfile (to avoid drift/conflicts).
Per the repo’s guidelines, use a single Pipfile. Keeping Pipfile.new alongside Pipfile will cause lockfile drift, CI disagreements, and reviewer confusion. Merge any intended changes into Pipfile and delete this file.
Would you like me to propose a consolidated Pipfile diff and a small script to regenerate Pipfile.lock?
🤖 Prompt for AI Agents
In Pipfile.new (lines 1-31) you added a duplicate dependency file; remove
Pipfile.new, merge any intended dependency changes into the canonical Pipfile
(update its [packages]/[dev-packages] sections to include the new entries and
versions), regenerate the lockfile with the project’s standard command (e.g.,
pipenv lock or the repo’s script) so Pipfile.lock matches Pipfile, and commit
only the updated Pipfile and lockfile; delete Pipfile.new from the branch to
prevent drift and CI confusion.
Pipfile.new
Outdated
| itsdangerous = "~=2.2.0" | ||
| python-multipart = "~=0.0.10" | ||
| jinja2 = "~=3.1.4" | ||
| starlette = "~=0.40.0" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid pinning Starlette directly; let FastAPI control the compatible Starlette range.
FastAPI manages Starlette compatibility; pinning can introduce conflicts. Moreover, your pipeline flagged a Starlette vulnerability (GHSA-2c2j-9gv5-cj73; fixed in Starlette 0.47.2). To remediate properly you’ll need to let FastAPI resolve a patched Starlette version and bump FastAPI accordingly (see Pipfile comment). Do not add an explicit starlette pin here. (fastapi.tiangolo.com, nvd.nist.gov)
- starlette = "~=0.40.0"
+ # Intentionally not pinned; FastAPI controls the supported range.📝 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.
| starlette = "~=0.40.0" | |
| # (in Pipfile.new, under [packages]) | |
| # Intentionally not pinned; FastAPI controls the supported range. |
🤖 Prompt for AI Agents
In Pipfile.new around line 21, remove the explicit starlette = "~=0.40.0" entry
so FastAPI can control the compatible Starlette range; instead, bump the FastAPI
version in the Pipfile to a release that depends on Starlette >=0.47.2 (per the
Pipfile comment) and run pipenv lock / regenerate the lockfile to pick up the
patched Starlette; do not add any other direct Starlette pin or downgrade
FastAPI dependencies.
fix e2e test to work with flask
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/test_api_endpoints.py (1)
26-26: Fix favicon assertion to match comment.Comment says “404 is acceptable,” but assertion only allows 200. Tests will fail if favicon is absent.
- assert response.status_code in [200] # 404 is acceptable if no favicon + assert response.status_code in [200, 404] # 404 is acceptable if no favicontests/conftest.py (1)
25-36: Readiness probe is too strict; accept common ready statuses and avoid redirect chains.The app root might 302 to /chat or return 401/403/404 before auth/static are configured. Current check requires 200 and follows redirects, causing false negatives. Broaden acceptable statuses and disable redirects.
# Wait for the app to start max_retries = 30 for _ in range(max_retries): try: - response = requests.get("http://localhost:5000/", timeout=1) - if response.status_code == 200: + response = requests.get("http://localhost:5000/", timeout=1, allow_redirects=False) + if response.status_code in (200, 302, 401, 403, 404): break except requests.exceptions.RequestException: time.sleep(1) else: process.terminate() - raise RuntimeError("FastAPI app failed to start") + raise RuntimeError("FastAPI app failed to start (health check did not return a ready status code)")
🧹 Nitpick comments (11)
.github/copilot-instructions.md (5)
81-89: Bind to 0.0.0.0 in production examples (containers/cloud) rather than localhost.Using "localhost" limits exposure to the loopback interface and can break Docker/K8s/VM deployments. Recommend binding to 0.0.0.0 in the production command snippet.
# Production mode make run-prod -# OR manually: pipenv run uvicorn api.index:app --host "localhost" --port "5000" +# OR manually: pipenv run uvicorn api.index:app --host "0.0.0.0" --port "5000"
128-131: Clarify env var purpose and remove unused FLASK_DEBUG from docs.The app uses FLASK_SECRET_KEY (FastAPI sessions), but FLASK_DEBUG isn’t referenced in api/app_factory.py or tests. Keep the secret key, drop FLASK_DEBUG from the required list, and clarify the comment to “Required for sessions.”
-# REQUIRED for FastAPI to start -FLASK_SECRET_KEY=your_super_secret_key_here -FLASK_DEBUG=False +# REQUIRED for sessions (FastAPI cookies) +FLASK_SECRET_KEY=your_super_secret_key_here
149-153: Mirror the minimal .env guidance: remove FLASK_DEBUG here too.Keep the secret key and DB settings; FLASK_DEBUG is unused.
FLASK_SECRET_KEY=test-secret-key -FLASK_DEBUG=False FALKORDB_HOST=localhost FALKORDB_PORT=6379
197-215: Add a language to fenced code block to satisfy markdownlint (MD040).Mark tree/listing blocks as text for cleaner linting and editor highlighting.
-``` +```text api/ # Main application package ├── index.py # FastAPI application entry point ...--- `293-295`: **Avoid referencing specific line numbers in docs; it drifts quickly.** Embedding “(lines 1-50 contain core configuration)” will go stale. Remove the parenthetical. ```diff -- `api/app_factory.py`: Application factory with OAuth setup (lines 1-50 contain core configuration) +- `api/app_factory.py`: Application factory with OAuth setuptests/e2e/test_api_endpoints.py (1)
29-31: Make static directory assertion resilient across environments.With SecurityMiddleware, /static/css/ returns 403, but different servers/CDNs may return 404 or 405. Keep the intent (“no directory listing”) but allow common variants.
- assert response.status_code in [403] # Various acceptable responses + assert response.status_code in [403, 404, 405] # No directory listing; status may vary by servertests/e2e/pages/home_page.py (1)
19-23: Optional: Centralize route paths to reduce coupling.Consider a small Routes/Paths helper (or fixture-provided base path) so page objects don’t hard-code “/chat”, easing future route changes.
api/app_factory.py (4)
28-41: Harden static path checks: decode and normalize to catch encoded traversal.Checking for '../' misses percent-encoded traversal (..%2F) and path normalization edge cases. Decode and posix-normalize before checks. Starlette’s StaticFiles already mitigates traversal; this makes your explicit 403 path stricter and less brittle.
async def dispatch(self, request: Request, call_next): - # Block directory access in static files - if request.url.path.startswith(self.STATIC_PREFIX): - # Remove /static/ prefix to get the actual path - filename = request.url.path[len(self.STATIC_PREFIX):] - # Basic security check for directory traversal - if '../' in filename or filename.endswith('/'): - return JSONResponse( - status_code=403, - content={"detail": "Forbidden"} - ) - - response = await call_next(request) - return response + # Block directory access in static files + if request.url.path.startswith(self.STATIC_PREFIX): + # Remove /static/ prefix and decode + raw = request.url.path[len(self.STATIC_PREFIX):] + filename = unquote(raw) + # Normalize to detect traversal reliably + normalized = posixpath.normpath("/" + filename).lstrip("/") + # Forbid directory listing and traversal + if normalized.endswith("/") or normalized.startswith("../") or "/../" in f"/{normalized}/": + return JSONResponse(status_code=403, content={"detail": "Forbidden"}) + response = await call_next(request) + return responseAdd these imports near the other imports:
from urllib.parse import unquote import posixpath
54-62: Make session cookie “secure” configurable via env for production.Hardcoding https_only=False is fine locally but weak in prod. Gate it by SESSION_COOKIE_SECURE=true.
- app.add_middleware( + secure_cookie = os.getenv("SESSION_COOKIE_SECURE", "false").lower() == "true" + app.add_middleware( SessionMiddleware, secret_key=secret_key, session_cookie="qw_session", same_site="lax", # allow top-level OAuth GET redirects to send cookies - https_only=False, # allow http on localhost in development + https_only=secure_cookie, # enable in prod with SESSION_COOKIE_SECURE=true max_age=60 * 60 * 24 * 14, # 14 days - measured by seconds )
49-53: Naming nit: FLASK_SECRET_KEY under FastAPI may confuse new contributors.Not blocking, but consider documenting the rationale (“legacy env var name kept for backward compatibility”) or accept either name (e.g., read SESSION_SECRET if present, else fallback to FLASK_SECRET_KEY).
Example reference change outside this hunk:
secret_key = os.getenv("SESSION_SECRET") or os.getenv("FLASK_SECRET_KEY")
97-101: Minor: Read GTM id once at startup to avoid per-request env lookups.Cache GOOGLE_TAG_MANAGER_ID in app.state on startup and reuse.
Example change outside this hunk:
app.state.gtm_id = os.getenv("GOOGLE_TAG_MANAGER_ID") @app.middleware("http") async def add_template_globals(request: Request, call_next): request.state.google_tag_manager_id = app.state.gtm_id return await call_next(request)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/copilot-instructions.md(7 hunks)api/app_factory.py(1 hunks)tests/conftest.py(3 hunks)tests/e2e/pages/home_page.py(1 hunks)tests/e2e/test_api_endpoints.py(1 hunks)tests/test_simple_integration.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_simple_integration.py
🧰 Additional context used
📓 Path-based instructions (4)
tests/e2e/test_*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place Playwright E2E tests as tests/e2e/test_*.py
Files:
tests/e2e/test_api_endpoints.py
tests/e2e/pages/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Organize E2E Page Object Model classes under tests/e2e/pages/
Files:
tests/e2e/pages/home_page.py
tests/conftest.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep pytest configuration and fixtures in tests/conftest.py
Files:
tests/conftest.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 (6)
📚 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 .env : Create a local .env file (not committed) from .env.example for running the app
Applied to files:
.github/copilot-instructions.md
📚 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 .env.example : Maintain all required environment variables in .env.example as a template
Applied to files:
.github/copilot-instructions.md
📚 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 vercel.json : Maintain Vercel deployment configuration in vercel.json
Applied to files:
.github/copilot-instructions.md
📚 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/index.py : Keep the Flask application entry point at api/index.py
Applied to files:
.github/copilot-instructions.md
📚 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:
.github/copilot-instructions.mdapi/app_factory.py
📚 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/agents/**/*.py : Place AI agent implementations (analysis, relevancy, follow-up) under api/agents/
Applied to files:
.github/copilot-instructions.md
🧬 Code graph analysis (2)
tests/e2e/pages/home_page.py (1)
tests/e2e/pages/base_page.py (1)
navigate_to(13-16)
api/app_factory.py (1)
api/routes/auth.py (1)
init_auth(273-307)
🪛 LanguageTool
.github/copilot-instructions.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...uth authentication. Key Technologies: - Backend: Python 3.12+, FastAPI 0.115.0...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...+, FalkorDB (Redis-based graph database) - AI/ML: LiteLLM with Azure OpenAI/OpenA...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...I integration for text-to-SQL generation - Testing: pytest for unit tests, Playwr...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...r unit tests, Playwright for E2E testing - Dependencies: pipenv for package manag...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...dencies**: pipenv for package management - Authentication: authlib with Google/Gi...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...tion**: authlib with Google/GitHub OAuth - Deployment: Docker support, Vercel con...
(QB_NEW_EN)
[grammar] ~237-~237: There might be a mistake here.
Context: ...PI ecosystem**: FastAPI, Authlib (OAuth) - Database: falkordb, psycopg2-binary (P...
(QB_NEW_EN)
[grammar] ~293-~293: There might be a mistake here.
Context: .../index.py: Main FastAPI app entry point - api/app_factory.py`: Application factory with OAuth setup (...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.github/copilot-instructions.md
197-197: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (2)
.github/copilot-instructions.md (1)
236-242: No action needed: “psycopg2-binary” is indeed a project dependencyI’ve confirmed that
psycopg2-binaryis declared and used throughout the repo:
- Declared in Pipfile (and Pipfile.new) at
psycopg2-binary = "~=2.9.9"- Locked in Pipfile.lock
- Referenced in documentation (docs/postgres_loader.md) and used in code (
api/loaders/postgres_loader.py) and its testsThe Key Dependencies section correctly lists it, so it can remain as-is.
tests/e2e/pages/home_page.py (1)
21-21: LGTM: Home now navigates to /chat.Matches the FastAPI routing surface and reduces root-redirect dependency.
| @app.exception_handler(Exception) | ||
| async def handle_oauth_error(request: Request, exc: Exception): | ||
| """Handle OAuth-related errors gracefully""" | ||
| # Check if it's an OAuth-related error | ||
| if "token" in str(error).lower() or "oauth" in str(error).lower(): | ||
| logging.warning("OAuth error occurred: %s", error) | ||
| session.clear() | ||
| return redirect(url_for("auth.home")) | ||
| if "token" in str(exc).lower() or "oauth" in str(exc).lower(): | ||
| logging.warning("OAuth error occurred: %s", exc) | ||
| request.session.clear() | ||
| return RedirectResponse(url="/", status_code=302) | ||
|
|
||
| # If it's an HTTPException (like abort(403)), re-raise so Flask handles it properly | ||
| if isinstance(error, HTTPException): | ||
| return error | ||
| # If it's an HTTPException, re-raise so FastAPI handles it properly | ||
| if isinstance(exc, HTTPException): | ||
| raise exc | ||
|
|
||
| # For other errors, let them bubble up | ||
| raise error | ||
|
|
||
| @app.before_request | ||
| def block_static_directories(): | ||
| 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) | ||
|
|
||
| @app.context_processor | ||
| def inject_google_tag_manager(): | ||
| """Inject Google Tag Manager ID into template context.""" | ||
| return { | ||
| 'google_tag_manager_id': os.getenv("GOOGLE_TAG_MANAGER_ID") | ||
| } | ||
| raise exc | ||
|
|
There was a problem hiding this comment.
Exception handler can mask errors and risk recursion; delegate HTTPException and target OAuth explicitly.
Catching Exception broadly, then “raise exc” for HTTPException can lead to unclear control flow and potential recursion. Also, substring checks (“oauth”/“token”) are brittle. Use FastAPI’s default HTTPException handler and a targeted OAuthError handler.
- @app.exception_handler(Exception)
- async def handle_oauth_error(request: Request, exc: Exception):
- """Handle OAuth-related errors gracefully"""
- # Check if it's an OAuth-related error
- if "token" in str(exc).lower() or "oauth" in str(exc).lower():
- logging.warning("OAuth error occurred: %s", exc)
- request.session.clear()
- return RedirectResponse(url="/", status_code=302)
-
- # If it's an HTTPException, re-raise so FastAPI handles it properly
- if isinstance(exc, HTTPException):
- raise exc
-
- # For other errors, let them bubble up
- raise exc
+ @app.exception_handler(HTTPException)
+ async def passthrough_http_exception(request: Request, exc: HTTPException):
+ # Delegate to FastAPI's default handler to avoid recursion
+ return await http_exception_handler(request, exc)
+
+ @app.exception_handler(Exception)
+ async def handle_oauth_error(request: Request, exc: Exception):
+ """Handle OAuth-related errors gracefully without masking unrelated errors."""
+ if isinstance(exc, OAuthError) or "oauth" in str(exc).lower() or "token" in str(exc).lower():
+ logging.warning("OAuth error occurred: %s", exc)
+ request.session.clear()
+ return RedirectResponse(url="/", status_code=302)
+ # Bubble up everything else
+ raise excAdd these imports near the top of the file:
from fastapi.exception_handlers import http_exception_handler
from authlib.integrations.starlette_client import OAuthError🤖 Prompt for AI Agents
In api/app_factory.py around lines 80 to 95, the current broad Exception handler
uses brittle substring checks and can mask or recurse on HTTPException; replace
it with a specific handler for OAuthError that logs the exception, clears the
session and returns a RedirectResponse, and remove the global Exception catch so
FastAPI’s default HTTPException handling is preserved by delegating
HTTPException to fastapi.exception_handlers.http_exception_handler; also add the
two imports from fastapi.exception_handlers and
authlib.integrations.starlette_client at the top of the file as suggested.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
api/app_factory.py (1)
80-95: Replace broad Exception handler with targeted handlers to avoid recursion and brittle matching.Catching Exception and re-raising HTTPException here will re-enter the same handler and can recurse; substring checks for "oauth"/"token" are brittle. Delegate HTTPException to FastAPI’s default handler and handle OAuthError explicitly.
Apply this diff:
- @app.exception_handler(Exception) - async def handle_oauth_error(request: Request, exc: Exception): - """Handle OAuth-related errors gracefully""" - # Check if it's an OAuth-related error - if "token" in str(exc).lower() or "oauth" in str(exc).lower(): - logging.warning("OAuth error occurred: %s", exc) - request.session.clear() - return RedirectResponse(url="/", status_code=302) - - # If it's an HTTPException, re-raise so FastAPI handles it properly - if isinstance(exc, HTTPException): - raise exc - - # For other errors, let them bubble up - raise exc + @app.exception_handler(HTTPException) + async def passthrough_http_exception(request: Request, exc: HTTPException): + # Delegate to FastAPI's default handler to avoid recursion + return await http_exception_handler(request, exc) + + @app.exception_handler(OAuthError) + async def handle_oauth_error(request: Request, exc: OAuthError): + """Handle OAuth-related errors gracefully without masking unrelated errors.""" + logging.warning("OAuth error occurred: %s", exc) + request.session.clear() + return RedirectResponse(url="/", status_code=302)Add these imports near the top of the file (see separate comment on Lines 8-13).
+from fastapi.exception_handlers import http_exception_handler +from authlib.integrations.starlette_client import OAuthErrorRun to ensure no catch-all Exception handlers remain:
#!/bin/bash rg -nP "@app\.exception_handler\(\s*Exception\s*\)" -C2 api/app_factory.py
🧹 Nitpick comments (4)
api/app_factory.py (4)
49-53: Use generic env var names for the session secret; improve warning.“FLASK_SECRET_KEY” is legacy. Accept SESSION_SECRET and SECRET_KEY first and keep FLASK_SECRET_KEY as a fallback. Log only when we actually generate a random key.
- secret_key = os.getenv("FLASK_SECRET_KEY") - if not secret_key: - secret_key = secrets.token_hex(32) - logging.warning("FLASK_SECRET_KEY not set, using generated key. Set this in production!") + secret_key = ( + os.getenv("SESSION_SECRET") + or os.getenv("SECRET_KEY") + or os.getenv("FLASK_SECRET_KEY") + ) + if not secret_key: + secret_key = secrets.token_hex(32) + logging.warning( + "No session secret set (SESSION_SECRET/SECRET_KEY/FLASK_SECRET_KEY); " + "generated a random key for development. Configure a persistent secret in production!" + )
55-62: Make cookie security configurable; default to non-secure for localhost only.Allow overriding https_only via env so production can set secure cookies without code changes.
- app.add_middleware( + secure_cookie = os.getenv("SESSION_COOKIE_SECURE", "false").lower() == "true" + app.add_middleware( SessionMiddleware, secret_key=secret_key, session_cookie="qw_session", same_site="lax", # allow top-level OAuth GET redirects to send cookies - https_only=False, # allow http on localhost in development + https_only=secure_cookie, # set SESSION_COOKIE_SECURE=true in prod max_age=60 * 60 * 24 * 14, # 14 days - measured by seconds )
68-71: Prefer pathlib for static path resolution; avoid string joins with '..'.Pathlib improves readability and cross-platform safety.
- static_path = os.path.join(os.path.dirname(__file__), "../app/public") - if os.path.exists(static_path): - app.mount("/static", StaticFiles(directory=static_path), name="static") + static_path = Path(__file__).resolve().parent.parent / "app" / "public" + if static_path.exists(): + app.mount("/static", StaticFiles(directory=str(static_path)), name="static")
96-101: Avoid per-request env lookups; cache GTM ID on app.state.Minor perf/readability tweak.
- # Add template globals + # Add template globals + # Cache once on startup + app.state.google_tag_manager_id = os.getenv("GOOGLE_TAG_MANAGER_ID") @app.middleware("http") async def add_template_globals(request: Request, call_next): - request.state.google_tag_manager_id = os.getenv("GOOGLE_TAG_MANAGER_ID") + request.state.google_tag_manager_id = app.state.google_tag_manager_id response = await call_next(request) return response
📜 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 (3)
Makefile(2 hunks)api/app_factory.py(1 hunks)tests/test_simple_integration.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_simple_integration.py
- Makefile
🧰 Additional context used
📓 Path-based instructions (1)
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 (1)
api/app_factory.py (1)
api/routes/auth.py (1)
init_auth(273-307)
🪛 GitHub Actions: Pylint
api/app_factory.py
[error] 46-46: Pylint: Line too long (C0301).
[error] 23-23: Pylint: Too few public methods (R0903).
⏰ 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)
api/app_factory.py (1)
72-78: Routers and OAuth initialization are wired correctly.init_auth(app) and the include_router calls align with the intended FastAPI migration and meet the app-factory objective.
| from fastapi import FastAPI, Request, HTTPException | ||
| from fastapi.responses import RedirectResponse, JSONResponse | ||
| from fastapi.staticfiles import StaticFiles | ||
| from starlette.middleware.sessions import SessionMiddleware | ||
| from starlette.middleware.base import BaseHTTPMiddleware | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing imports for exception handling helpers.
Required for the targeted handlers suggested below.
from fastapi import FastAPI, Request, HTTPException
from fastapi.responses import RedirectResponse, JSONResponse
from fastapi.staticfiles import StaticFiles
+from fastapi.exception_handlers import http_exception_handler
from starlette.middleware.sessions import SessionMiddleware
from starlette.middleware.base import BaseHTTPMiddleware
+from authlib.integrations.starlette_client import OAuthError
+from pathlib import PathCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In api/app_factory.py around lines 8 to 13, the module is missing exception
helper imports used by the handlers; add the appropriate imports (for example
RequestValidationError from fastapi.exceptions and Starlette's HTTPException
alias if needed) at the top of the file so the exception handlers referenced
later have the correct symbols available.
| """Middleware for security checks including static file access""" | ||
|
|
||
| STATIC_PREFIX = '/static/' | ||
|
|
||
| async def dispatch(self, request: Request, call_next): | ||
| # Block directory access in static files | ||
| if request.url.path.startswith(self.STATIC_PREFIX): | ||
| # Remove /static/ prefix to get the actual path | ||
| filename = request.url.path[len(self.STATIC_PREFIX):] | ||
| # Basic security check for directory traversal | ||
| if not filename or '../' in filename or filename.endswith('/'): | ||
| return JSONResponse( | ||
| status_code=403, | ||
| content={"detail": "Forbidden"} | ||
| ) | ||
|
|
||
| response = await call_next(request) | ||
| return response | ||
|
|
There was a problem hiding this comment.
Address Pylint R0903 and reconsider custom static guard.
- R0903 “Too few public methods” on this lightweight middleware breaks the pipeline. Add an inline disable or convert to function-based middleware.
- StaticFiles already mitigates directory traversal; duplicating checks here risks false positives (e.g., blocking /static/ with 403) and adds maintenance overhead.
Minimal pipeline fix:
-class SecurityMiddleware(BaseHTTPMiddleware):
+class SecurityMiddleware(BaseHTTPMiddleware): # pylint: disable=R0903Optional: remove this class and rely on StaticFiles’ built-in protections, or reimplement as a function middleware colocated near the static mount.
📝 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.
| class SecurityMiddleware(BaseHTTPMiddleware): | |
| """Middleware for security checks including static file access""" | |
| STATIC_PREFIX = '/static/' | |
| async def dispatch(self, request: Request, call_next): | |
| # Block directory access in static files | |
| if request.url.path.startswith(self.STATIC_PREFIX): | |
| # Remove /static/ prefix to get the actual path | |
| filename = request.url.path[len(self.STATIC_PREFIX):] | |
| # Basic security check for directory traversal | |
| if not filename or '../' in filename or filename.endswith('/'): | |
| return JSONResponse( | |
| status_code=403, | |
| content={"detail": "Forbidden"} | |
| ) | |
| response = await call_next(request) | |
| return response | |
| class SecurityMiddleware(BaseHTTPMiddleware): # pylint: disable=R0903 | |
| """Middleware for security checks including static file access""" | |
| STATIC_PREFIX = '/static/' | |
| async def dispatch(self, request: Request, call_next): | |
| # Block directory access in static files | |
| if request.url.path.startswith(self.STATIC_PREFIX): | |
| # Remove /static/ prefix to get the actual path | |
| filename = request.url.path[len(self.STATIC_PREFIX):] | |
| # Basic security check for directory traversal | |
| if not filename or '../' in filename or filename.endswith('/'): | |
| return JSONResponse( | |
| status_code=403, | |
| content={"detail": "Forbidden"} | |
| ) | |
| response = await call_next(request) | |
| return response |
🧰 Tools
🪛 GitHub Actions: Pylint
[error] 23-23: Pylint: Too few public methods (R0903).
| app.secret_key = secrets.token_hex(32) | ||
| """Create and configure the FastAPI application.""" | ||
| app = FastAPI(title="QueryWeaver", description="Text2SQL with Graph-Powered Schema Understanding") | ||
|
|
There was a problem hiding this comment.
Fix Pylint C0301: split long FastAPI constructor line.
This line is flagged and breaks the pipeline. Split into multiple lines.
- app = FastAPI(title="QueryWeaver", description="Text2SQL with Graph-Powered Schema Understanding")
+ app = FastAPI(
+ title="QueryWeaver",
+ description="Text2SQL with Graph-Powered Schema Understanding",
+ )📝 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.
| app = FastAPI(title="QueryWeaver", description="Text2SQL with Graph-Powered Schema Understanding") | |
| app = FastAPI( | |
| title="QueryWeaver", | |
| description="Text2SQL with Graph-Powered Schema Understanding", | |
| ) |
🧰 Tools
🪛 GitHub Actions: Pylint
[error] 46-46: Pylint: Line too long (C0301).
🤖 Prompt for AI Agents
In api/app_factory.py around line 46, the single long FastAPI(...) call exceeds
the line length (Pylint C0301); break the constructor into multiple lines by
putting each keyword argument on its own line (e.g., title=...,
description=...), align the closing parenthesis on its own line, and ensure
indentation matches project style so the call stays within the line-length
limit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/e2e-tests.yml (5)
40-44: Node.js setup: consider pinning strategy and enabling cache
- Using a major-only version ('22') is okay, but CI reproducibility improves if you either pin a minor (e.g., 22.x) or track LTS automatically.
- If the Makefile invokes npm/pnpm/yarn, enable actions/setup-node caching for faster runs.
Apply one of these diffs:
Option A (track LTS automatically; add npm cache):
- - name: Set up Node.js - uses: actions/setup-node@v4 - with: - node-version: '22' + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: 'lts/*' + cache: 'npm'Option B (pin minor; add npm cache):
- node-version: '22' + node-version: '22.x' + cache: 'npm'If you use pnpm or yarn under the hood, switch cache: to 'pnpm' or 'yarn' accordingly.
45-48: Make vs. pipenv: drop unused pipenv setup if Makefile doesn’t use itYou install pipenv earlier (Lines 35–39) but the subsequent steps only call make targets. If make install handles Python deps without pipenv, consider removing the pipenv installation to simplify CI and cut time.
Outside the selected lines, you can remove the pipenv step:
- - name: Install pipenv - run: | - python -m pip install --upgrade pip - pip install pipenvIf make install actually shells out to pipenv, keep it and please ignore this.
49-52: Dev setup via Makefile: good consolidation; consider caching Python depsConsolidating environment setup behind make setup-dev is 👍. For speed, you can enable pip cache in setup-python (if you rely on requirements.txt/constraints).
Example (outside selected lines, augment setup-python step):
- - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + cache: 'pip' + cache-dependency-path: | + requirements*.txt + **/requirements*.txtAdjust cache-dependency-path to whatever your Makefile consumes.
18-25: Avoid 'latest' tag for FalkorDB to keep CI deterministicPin the service image to a specific version or digest to prevent surprise breakages when 'latest' updates.
- image: falkordb/falkordb:latest + image: falkordb/falkordb:3.9.0Replace 3.9.0 with the version you support, or use a sha256 digest for full reproducibility.
61-67: Bound the wait loop to avoid hanging jobsThe wait loop retries indefinitely. Add a max timeout/attempts so failed startups don’t hang CI.
- until docker exec "$(docker ps -q --filter ancestor=falkordb/falkordb:latest)" redis-cli ping; do - echo "Waiting for FalkorDB..." - sleep 2 - done + SECS=0 + until docker exec "$(docker ps -q --filter ancestor=falkordb/falkordb:latest)" redis-cli ping; do + echo "Waiting for FalkorDB..." + sleep 2 + SECS=$((SECS+2)) + if [ "$SECS" -ge 120 ]; then + echo "Timeout waiting for FalkorDB" + exit 1 + fi + doneOptionally, consider docker compose with healthcheck and depends_on: condition: service_healthy for cleaner orchestration.
📜 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/workflows/e2e-tests.yml(2 hunks)Makefile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
📓 Path-based instructions (1)
.github/workflows/e2e-tests.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use a dedicated E2E CI workflow in .github/workflows/e2e-tests.yml
Files:
.github/workflows/e2e-tests.yml
🧠 Learnings (4)
📚 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/e2e-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 tests/e2e/test_*.py : Place Playwright E2E tests as tests/e2e/test_*.py
Applied to files:
.github/workflows/e2e-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 tests/e2e/README.md : Document E2E testing procedures in tests/e2e/README.md
Applied to files:
.github/workflows/e2e-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 setup_e2e_tests.sh : Provide and maintain the E2E setup script at setup_e2e_tests.sh
Applied to files:
.github/workflows/e2e-tests.yml
⏰ 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 (3)
.github/workflows/e2e-tests.yml (3)
1-1: Compliant: Dedicated E2E workflow path used as requestedThis workflow lives at .github/workflows/e2e-tests.yml, matching the guideline to keep E2E CI in a dedicated workflow file.
53-60: Env file still uses FLASK_ after FastAPI migration—confirm or rename*The workflow echoes FLASK_SECRET_KEY and FLASK_DEBUG. Given the PR migrates to FastAPI, verify whether these names are still read by the app/tests or should be replaced (e.g., APP_SECRET_KEY, DEBUG, or FASTAPI_*).
If they’re obsolete, adjust:
- echo "FLASK_SECRET_KEY=test-secret-key-for-ci" >> .env - echo "FLASK_DEBUG=False" >> .env + echo "APP_SECRET_KEY=test-secret-key-for-ci" >> .env + echo "DEBUG=False" >> .envOnly change if the application actually expects the new names.
68-72: LGTM: make test-e2e entry pointGood consolidation—delegating to Makefile keeps CI and local runs aligned. Ensure the target emits artifacts (e.g., JUnit XML) that match the upload steps on failure.
If not already, you can have the target write JUnit XML to a consistent path and upload it similarly to screenshots for easier debugging.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores