Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds a landing page (template + CSS) with an animated SQL demo, introduces an explicit /chat route and adjusts home() to serve the landing page for unauthenticated users, reformats Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Server
participant Auth as validate_and_cache_user()
Note over Browser,Server: Request root "/"
Browser->>Server: GET /
alt Authenticated
Server->>Auth: validate_and_cache_user()
Auth-->>Server: is_authenticated=True, user_info
Server-->>Browser: 200 chat.j2 (is_authenticated=True, user_info)
else Not authenticated
Server-->>Browser: 200 landing.j2 (is_authenticated=False, user_info=None)
end
Note over Browser,Server: User requests chat
Browser->>Server: GET /chat
Server->>Auth: validate_and_cache_user()
Auth-->>Server: is_authenticated?, user_info?
Server-->>Browser: 200 chat.j2 (is_authenticated, user_info)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
vercel.json (3)
3-6: Confirm global rewrite intent; filesystem is prioritized before rewrites.With vercel.json rewrites, Vercel checks the filesystem first by default. Static assets from the build output will be served if they exist; only unmatched paths will be forwarded to the destination. Please confirm that sending every non-static path to the Python API is desired. If you prefer a more explicit API-only rewrite or want to decouple from the file extension, consider pointing to “/api/index” instead of “/api/index.py”. (vercel.com)
Apply this small tweak to the destination (optional):
- "destination": "/api/index.py" + "destination": "/api/index"
1-1: Add $schema for validation and IDE autocomplete.Adding the schema helps catch mistakes (e.g., misplaced keys) and provides autocompletion in editors.
Apply at the top of the file:
{ + "$schema": "https://openapi.vercel.sh/vercel.json",
8-23: If you choose to keep legacy “builds”, be aware of side effects and limitations.
- When any "builds" entry exists, only build outputs are included in the deployment. That means omitting an explicit Python build could exclude your API from the deployment. If you intentionally keep "builds", remove the misleading maxDuration and rely on default duration limits instead. (vercel.com)
Minimal safe alternative (retain legacy approach, drop ineffective maxDuration):
"builds": [ { "src": "app/package.json", "use": "@vercel/static-build", "config": { "distDir": "public" } }, { "src": "api/index.py", - "use": "@vercel/python", - "config": { - "maxDuration": 300 - } + "use": "@vercel/python" } ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
vercel.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
vercel.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain Vercel deployment configuration in vercel.json
Files:
vercel.json
🧠 Learnings (2)
📓 Common learnings
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
📚 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:
vercel.json
⏰ 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)
vercel.json (1)
3-6: Verified: referenced paths existThe sanity check confirms that all paths used in vercel.json (lines 3–6 and 8–23) are present in the repo:
- app/package.json: OK
- api/index.py: OK
No further action needed.
| "src": "api/index.py", | ||
| "use": "@vercel/python", | ||
| "config": { | ||
| "maxDuration": 300 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
maxDuration inside a builder’s config is ignored; it must be set via the functions property (builders and functions can’t be combined).
- The "config.maxDuration" under the @vercel/python build does not configure function duration. For Python (and other non-inline runtimes), maxDuration must be set under the top-level "functions" key in vercel.json. (vercel.com)
- However, Vercel explicitly disallows using "functions" together with "builds" in the same config. If you keep "builds", the functions property is not permitted, and your maxDuration setting won’t take effect. (vercel.com)
Recommend migrating away from "builds" and using:
- top-level buildCommand + outputDirectory for the frontend,
- top-level functions for Python duration.
Here’s a concrete replacement for the builds block that preserves behavior and enables maxDuration:
- "builds": [
- {
- "src": "app/package.json",
- "use": "@vercel/static-build",
- "config": {
- "distDir": "public"
- }
- },
- {
- "src": "api/index.py",
- "use": "@vercel/python",
- "config": {
- "maxDuration": 300
- }
- }
- ]
+ "buildCommand": "npm --prefix app run build",
+ "outputDirectory": "app/public",
+ "functions": {
+ "api/index.py": {
+ "maxDuration": 300
+ }
+ }Notes:
- buildCommand/outputDirectory are the modern way to declare how your static app is built and where the files end up. (vercel.com)
- Removing "builds" avoids the “Conflicting functions and builds configuration” error and makes maxDuration effective. (vercel.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.
| "src": "api/index.py", | |
| "use": "@vercel/python", | |
| "config": { | |
| "maxDuration": 300 | |
| } | |
| // remove the old "builds" array entirely: | |
| - "builds": [ | |
| - { | |
| - "src": "app/package.json", | |
| - "use": "@vercel/static-build", | |
| - "config": { "distDir": "public" } | |
| - }, | |
| - { | |
| - "src": "api/index.py", | |
| - "use": "@vercel/python", | |
| - "config": { "maxDuration": 300 } | |
| - } | |
| - ], | |
| // add modern build + functions keys: | |
| "buildCommand": "npm --prefix app run build", | |
| "outputDirectory": "app/public", | |
| "functions": { | |
| "api/index.py": { | |
| "maxDuration": 300 | |
| } | |
| } |
🤖 Prompt for AI Agents
In vercel.json around lines 17 to 21, the current "builds" block uses
"config.maxDuration" under the @vercel/python builder which is ignored and also
conflicts with the top-level "functions" property; replace the builds-based
approach by removing the "builds" entry, add top-level "buildCommand" and
"outputDirectory" to declare how the static frontend is built, and add a
top-level "functions" entry for your Python server with the desired
"maxDuration" (and any memory/other settings); this preserves build behavior
while making maxDuration effective and avoiding the builds/functions conflict.
add landing page
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
app/public/css/landing.css (4)
1-9: Use minmax to prevent grid overflow on long titles/content.Long, unbreakable content in the left column can push the grid past its container. Use minmax(0, 1fr) to avoid overflow and improve resilience.
-.landing-container{ +.landing-container{ max-width:1200px; margin:3rem auto; display:grid; - grid-template-columns:1fr 500px; + grid-template-columns:minmax(0,1fr) 500px; gap:2.5rem; align-items:center; padding:0 1rem; }
12-19: Add a generic family fallback to the font stack.Include sans-serif at the end of the stack to avoid falling back to serif on platforms lacking the listed fonts.
-.hero-title{ - font-family: 'Inter', system-ui, -apple-system, 'Segoe UI', Roboto, 'Helvetica Neue', Arial; +.hero-title{ + font-family: 'Inter', system-ui, -apple-system, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, sans-serif; font-weight:800; font-size:4rem; line-height:1.02; margin:0 0 1rem 0; color:var(--text-primary); }
26-29: Provide hover/focus-visible states for buttons to improve a11y.Interactive elements should expose focus states for keyboard users and hover states for pointer users. Also consider a higher-contrast focus ring.
Add near the end of the file:
.btn-pill:hover { filter: brightness(0.95); } .btn-ghost:hover { background: var(--bg-tertiary); } .btn-pill:focus-visible, .btn-ghost:focus-visible { outline: 2px solid var(--falkor-primary); outline-offset: 2px; border-radius: 999px; }
40-41: Avoid hard-coded light-theme colors; fall back to theme token with a default.Hard-coded #e7f1ff may be too bright in dark mode. Prefer a themed token with a safe fallback.
-.demo-cta .btn-full{display:inline-block;width:100%;padding:0.75rem;border-radius:8px;background:#e7f1ff;color:var(--falkor-primary);text-decoration:none} +.demo-cta .btn-full{display:inline-block;width:100%;padding:0.75rem;border-radius:8px;background:var(--falkor-primary-weak, #e7f1ff);color:var(--falkor-primary);text-decoration:none}If --falkor-primary-weak does not exist, consider introducing it where your theme variables are defined.
api/routes/auth.py (1)
32-36: Explicit /chat route is clear; consider centralizing user context.Repeating validate_and_cache_user() across routes can be avoided by a before_request that populates g.user and a context_processor to inject is_authenticated/user_info for all templates.
Example (outside this hunk):
from flask import g @auth_bp.before_app_request def load_user_ctx(): g.user_info, g.is_authenticated = validate_and_cache_user() @app.context_processor def inject_user(): return dict(is_authenticated=getattr(g, "is_authenticated", False), user_info=getattr(g, "user_info", None))app/templates/landing.j2 (3)
14-16: Prefer CSS-driven line breaks over
for headings.Hard line breaks reduce flexibility on different viewports and languages. Let CSS handle line wrapping.
- <h1 class="hero-title">Transform Plain<br/>English into Powerful<br/>SQL.</h1> + <h1 class="hero-title">Transform Plain English into Powerful SQL.</h1>Optional CSS (add to landing.css):
.hero-title { text-wrap: balance; /* graceful wrapping */ }
18-20: CTA label doesn’t match the destination; clarify intent.The “Sign Up for Free” link routes to auth.chat (not a sign-up flow). Rename to reflect that it continues as guest or opens the chat UI.
- <a href="{{ url_for('auth.chat') }}" class="btn-pill">Sign Up for Free</a> + <a href="{{ url_for('auth.chat') }}" class="btn-pill">Try the Chat</a>
30-41: Wrap SQL in atag for optional syntax highlighting.If you use a client-side highlighter (Prism/Highlight.js), adding language-sql improves readability without functional changes.
- <pre class="demo-sql"> -SELECT c.companyName + <pre class="demo-sql"><code class="language-sql"> +SELECT c.companyName FROM customers c JOIN orders o ON c.customerID = o.customerID JOIN order_details od ON o.orderID = od.orderID JOIN products p ON od.productID = p.productID JOIN salesforce_tickets st ON c.customerID = st.customerID WHERE c.country = 'Germany' AND p.productName = 'Chai' AND st.status = 'Open'; - </pre> + </code></pre>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 306595e627e40acb97fa1b2c335085d2e4905f53 and 27f08fc74e0f1896ffb78f0d8c29b727d47652fc.
📒 Files selected for processing (3)
api/routes/auth.py (1 hunks)app/public/css/landing.css (1 hunks)app/templates/landing.j2 (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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
api/routes/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place all additional Flask route handlers under api/routes/
Files:
api/routes/auth.py
🧬 Code graph analysis (1)
api/routes/auth.py (1)
api/auth/user_management.py (1)
validate_and_cache_user (161-260)
⏰ 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 (5)
app/public/css/landing.css (2)
36-38: Good use of theme tokens for dark/light adaptability.
Using var(--falkor-secondary) and var(--falkor-border-tertiary) here ensures correct contrast across themes.
42-45: Responsive breakpoint looks sane.
Single-column collapse at 900px with reduced title size is a sensible default for small screens.
api/routes/auth.py (2)
29-30: Auth’d users drop straight into chat — good.
Returning chat.j2 for authenticated users from "/" aligns with the new explicit /chat route and keeps deep links stable.
25-31: No additional session clearing required in home route. The validate_and_cache_user function already invokes session.clear() on any failed or expired OAuth validation—removing stale tokens and user info before control returns to the home view. By the time you hit the “stale session” check, google.authorized and github.authorized will both be false, and session.pop("user_info", …) suffices to clean up any residual state. Adding another clear here would be redundant.
Likely an incorrect or invalid review comment.
app/templates/landing.j2 (1)
9-49: Overall: clean structure and semantics.
Extending base.j2, separating CSS via extra_css, and using section/aside is tidy. This template should integrate well once the static asset path is confirmed.
| {% block extra_css %} | ||
| <link rel="stylesheet" href="{{ url_for('static', filename='css/landing.css') }}"> | ||
| {% endblock %} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure landing.css is actually served by Flask’s static setup (path likely mismatched).
Template links to static/css/landing.css, but the stylesheet lives at app/public/css/landing.css in this PR. Unless the Flask app’s static_folder points to app/public (or your build copies to app/static/css), this will 404 and the page will be unstyled.
Options:
- Move file to app/static/css/landing.css (preferred), keeping this href unchanged.
- Or configure Flask’s static_folder to app/public.
Quick verification:
If moving the file, run:
🏁 Script executed:
#!/bin/bash
# Does a static/ directory exist, and do we already serve CSS from there?
fd -t d '^static$' app -x echo {}
fd -t f 'landing.css' app -x echo {}
# Does the Flask app set static_folder to "public"?
rg -nP "Flask\s*\(\s*__name__.*static_folder\s*=\s*['\"]public['\"]" -C2Length of output: 182
🏁 Script executed:
#!/bin/bash
git mkdir -p app/static/css 2>/dev/null || true
git mv app/public/css/landing.css app/static/css/landing.cssLength of output: 178
To pinpoint why the file move failed, let’s verify both the exact filesystem path and whether Git is tracking landing.css. Please run:
🏁 Script executed:
#!/bin/bash
# Show detailed listing of the CSS directory
echo "Contents of app/public/css/:"
ls -l app/public/css
# Confirm the exact path for landing.css
echo -e "\nSearching for landing.css under app/:"
fd -t f landing.css app -x echo {}
# Check if Git is tracking that file
echo -e "\nGit tracking status for app/public/css/landing.css:"
git ls-files app/public/css/landing.css || echo "→ Not tracked by Git"Length of output: 1234
Fix static asset path for landing.css
Your template at app/templates/landing.j2 uses
<link rel="stylesheet" href="{{ url_for('static', filename='css/landing.css') }}">but Flask’s default static_folder is app/static, and no override to public was found. As a result, the file under app/public/css/landing.css will 404.
We verified:
- No
app/static/directory exists app/public/css/landing.cssis present and tracked by Git- No
static_folder='public'setting in your Flask app ( confirmed this)
Required fix (preferred): Move the stylesheet into your static directory so it’s served correctly.
• Create the CSS folder and move the file:
mkdir -p app/static/css
git mv app/public/css/landing.css app/static/css/landing.css• Update any other assets in app/public/css/ if they need to be served via url_for('static', …).
Alternative: Change your Flask app initialization (e.g. in app/__init__.py or wsgi.py) to:
app = Flask(__name__, static_folder='public')Locations requiring attention:
- app/public/css/landing.css
- app/templates/landing.j2
- Flask app entrypoint (where
Flask(...)is called)
Move landing.css into Flask’s static folder
Your Jinja template at app/templates/landing.j2 uses:
<link rel="stylesheet" href="{{ url_for('static', filename='css/landing.css') }}">but the stylesheet currently lives at app/public/css/landing.css, and there’s no evidence of static_folder='public' in your Flask setup. This will cause a 404.
Required actions:
- Relocate the asset:
mkdir -p app/static/css git mv app/public/css/landing.css app/static/css/landing.css
- Verify any other CSS files under
app/public/css/are moved or served appropriately. - Confirm that
url_for('static', filename='css/…')links to files inapp/static/.
Alternatively, if you intend to keep assets under app/public, update your Flask app initialization:
app = Flask(__name__, static_folder='public')Files needing changes:
- app/public/css/landing.css
- app/templates/landing.j2
- Flask app entrypoint (where
Flask(...)is instantiated)
🤖 Prompt for AI Agents
In app/templates/landing.j2 around lines 5-7 the template references
static/css/landing.css via url_for('static', filename='css/landing.css') but the
actual file currently lives at app/public/css/landing.css which will 404 unless
Flask is configured to serve public as the static folder; move
app/public/css/landing.css into app/static/css/landing.css (create
app/static/css if needed) and update repository (git mv), or alternatively
change the Flask app initialization to set static_folder='public' where
Flask(...) is instantiated; also scan app/public/css/ for other assets and
either move them to app/static/css or ensure they are served by the chosen
static_folder configuration so url_for('static', ...) resolves correctly.
1 participant
Summary by CodeRabbit
New Features
Style
Chores