Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors user-result indexing and adds an unused import; switches Redis connection pool to BlockingConnectionPool; OAuth callbacks now redirect to /chat; landing template has DOM-closing tags removed and whitespace changes; Makefile run targets made port-configurable via PORT env var. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant App as Web App
participant OAuth as OAuth Provider
Note over App: OAuth callback now redirects to /chat
User->>Browser: Click "Sign in with Google/GitHub"
Browser->>OAuth: Redirect to provider
OAuth-->>Browser: Redirect back with callback
Browser->>App: GET /auth/<provider>/authorized?code=...
App->>App: Process callback (set cookies/session)
App-->>Browser: 302 Redirect to /chat
Browser->>App: GET /chat
App-->>Browser: Serve chat page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/routes/auth.py (2)
171-179: Harden session cookie and align TTL with DB token expiry.Set
samesite="Lax"to reduce CSRF risk andmax_age=86400(24h) to match the token’sexpires_atin the graph. Also use thestatusenum for readability.Apply:
- redirect = RedirectResponse(url="/chat", status_code=302) + redirect = RedirectResponse(url="/chat", status_code=status.HTTP_302_FOUND) redirect.set_cookie( key="api_token", value=api_token, httponly=True, - secure=True + secure=True, + samesite="Lax", + max_age=86400, + path="/" )Consider extracting cookie-setting into a small helper to avoid duplication between providers.
254-262: Repeat the cookie hardening for GitHub flow.Same rationale as Google: add
samesiteandmax_age, and prefer thestatusenum.Apply:
- redirect = RedirectResponse(url="/chat", status_code=302) + redirect = RedirectResponse(url="/chat", status_code=status.HTTP_302_FOUND) redirect.set_cookie( key="api_token", value=api_token, httponly=True, - secure=True + secure=True, + samesite="Lax", + max_age=86400, + path="/" )
🧹 Nitpick comments (6)
api/extensions.py (2)
6-6: Make import resilient across redis-py versions; alias the pool class.Some environments may not have
redis.asyncio.BlockingConnectionPool(older redis-py). Prefer a compat import that falls back toConnectionPool. This also simplifies future refactors by using a single alias everywhere.Apply:
-from redis.asyncio import BlockingConnectionPool +try: + from redis.asyncio import BlockingConnectionPool as AsyncBlockingConnectionPool +except ImportError: # older redis-py + from redis.asyncio import ConnectionPool as AsyncBlockingConnectionPoolIf you want me to verify exact redis-py versions that ship
asyncio.BlockingConnectionPool.from_url, I can run a quick web check.
19-23: Set sane pool limits/timeouts and use the compat alias.The default unlimited pool can exhaust connections under load. Add
max_connectionsand an acquisitiontimeout. Also switch to the alias from the previous comment.Apply:
- pool = BlockingConnectionPool.from_url( - url, - decode_responses=True - ) + pool = AsyncBlockingConnectionPool.from_url( + url, + decode_responses=True, + max_connections=int(os.getenv("REDIS_MAX_CONNECTIONS", "50")), + timeout=float(os.getenv("REDIS_POOL_TIMEOUT", "10.0")) + )Optionally, unify both branches (URL present/absent) by always creating a pool via
from_url(e.g., default to"redis://localhost:6379"), so behavior (decoding, pooling) is consistent. I can provide a full patch if you want.api/auth/user_management.py (2)
5-5: Remove unused import (ruff F401 / pylint unused-import).
from math import logis unused.Apply:
-from math import log
39-41: Minor tidy: local alias is good; also handle expired tokens proactively.Using
single_resultimproves readability. Consider deleting expired tokens here to keep the graph clean and close the TODO loop.Apply:
- single_result = result.result_set[0] - token_valid = single_result[3] + single_result = result.result_set[0] + email, name, picture, token_valid = single_resultAnd extend the logic below:
- if token_valid: - return { - "email": single_result[0], - "name": single_result[1], - "picture": single_result[2] - } + if token_valid: + return { + "email": email, + "name": name, + "picture": picture, + } + # else: drop expired/invalid token to avoid clutter and accidental reuse + await delete_user_token(api_token)app/templates/landing.j2 (2)
13-13: Optimize hero image for LCP without lazy-loading.Add
decoding="async"andfetchpriority="high"to help rendering; keep it non-lazy since it’s the hero. If you know intrinsic dimensions, adding width/height further reduces CLS.Apply:
- <img src="{{ url_for('static', path='icons/queryweaver.webp') }}" alt="QueryWeaver" /> + <img src="{{ url_for('static', path='icons/queryweaver.webp') }}" + alt="QueryWeaver logo" + decoding="async" + fetchpriority="high" />
86-97: Tiny micro-opt: precompile regexes outside the function (optional).Current code is fine for this page; if you care about micro perf, precompile patterns once to avoid recompilation on every call.
Example (outside function):
<script> const RE_SQL_STRING = /('[^']*')/g; const RE_SQL_KEYWORD = /\b(SELECT|FROM|JOIN|ON|WHERE|AND|OR|GROUP|BY|ORDER|LIMIT|AS|IN|IS|NULL|INNER|LEFT|RIGHT|OUTER)\b/gi; const RE_SQL_FUNC = /\b(COUNT|SUM|AVG|MIN|MAX)\b/gi; const RE_NUMBER = /\b(\d+\.?\d*)\b/g; // then use out.replace(RE_SQL_STRING, ...), etc. </script>
📜 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 (4)
api/auth/user_management.py(2 hunks)api/extensions.py(2 hunks)api/routes/auth.py(3 hunks)app/templates/landing.j2(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to pylint standards across all Python files (repository uses
make lint)
Files:
api/auth/user_management.pyapi/routes/auth.pyapi/extensions.py
app/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep the TypeScript frontend sources in
app/and build them before production runs
Files:
app/templates/landing.j2
🪛 Ruff (0.12.2)
api/auth/user_management.py
5-5: math.log imported but unused
Remove unused import: math.log
(F401)
🪛 GitHub Actions: Pylint
api/auth/user_management.py
[warning] 76-76: pylint: Line too long (137/100) (line-too-long).
[warning] 42-42: pylint: TODO delete invalid token from DB (fixme).
[warning] 51-51: pylint: Catching too general exception (broad-exception-caught).
[warning] 72-72: pylint: Catching too general exception (broad-exception-caught).
[warning] 168-168: pylint: Unnecessary 'elif' after 'return' (no-else-return).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (4)
api/auth/user_management.py (1)
45-47: LGTM: clearer field access.Using
single_result[...]makes the code less noisy without changing behavior.app/templates/landing.j2 (3)
81-83: LGTM: escaping is correct and used before innerHTML.
escapeHtmlis straightforward and called insidehighlightSQL, mitigating XSS when injecting highlighted content.
102-111: LGTM: null checks improve robustness.The added guards for
qEl,sEl, andsuccessElprevent runtime errors if the DOM changes.
129-129: LGTM: safe incremental rendering during typing.You continue to route content through
highlightSQL(which escapes), so usinginnerHTMLhere is safe.
add port settings
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Makefile (4)
65-70: Optional: prefer a Make variable over shell parameter expansion for PORTDefining a Make-level default improves portability (no reliance on shell-specific parameter expansion) and makes PORT visible to help/overrides (e.g., make PORT=8080 run-dev).
Apply this diff within these lines:
- pipenv run uvicorn api.index:app --host 127.0.0.1 --port $${PORT:-5000} --reload + pipenv run uvicorn api.index:app --host 127.0.0.1 --port $(PORT) --reload- pipenv run uvicorn api.index:app --host 0.0.0.0 --port $${PORT:-5000} + pipenv run uvicorn api.index:app --host 0.0.0.0 --port $(PORT)Add this near the top of the file (outside the selected range):
# Default PORT (can be overridden: `make PORT=8080 run-dev`) PORT ?= 5000
1-1: Fix .PHONY inconsistencies and include run/build/docker targets.run-dev/run-prod/build-dev/build-prod/test-e2e-debug/docker-* are missing from .PHONY, and .PHONY lists a non-existent build target. Either remove build from .PHONY or add a build alias. Recommend adding the alias and expanding .PHONY to avoid accidental file/dir collisions.
Apply this diff:
-.PHONY: help install test test-unit test-e2e test-e2e-headed lint format clean setup-dev build lint-frontend +.PHONY: help install setup-dev test test-unit test-e2e test-e2e-headed test-e2e-debug \ + lint lint-frontend format clean \ + build build-dev build-prod run-dev run-prod \ + docker-falkordb docker-stop
20-25: Add help descriptions and a build alias somake helpis completebuild-dev and build-prod are missing
##docs, so they don’t appear inmake help. Also add a build alias to match .PHONY and common workflows.Apply this diff:
-build-dev: +build-dev: ## Build frontend for development npm --prefix ./app run build:dev -build-prod: +build-prod: ## Build frontend for production npm --prefix ./app run build + +# Alias for convenience and to match .PHONY +build: build-prod ## Build frontend for production (alias)
54-56: Replace placeholderformattarget with actual toolsImplementing real formatters reduces churn in reviews and keeps consistency.
Apply this diff:
-format: ## Format code (placeholder - add black/autopep8 if needed) - @echo "Add code formatting tool like black here" +format: ## Format code (backend: black+isort, frontend: prettier) + pipenv run black . + pipenv run isort . + npm --prefix ./app run formatIf the frontend doesn’t have a
formatscript yet, add one in app/package.json using prettier.
📜 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)
Makefile(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Makefile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Provide and maintain Make targets (
install,setup-dev,test-unit,test-e2e,lint,run-dev,run-prod,build-prod,clean) as the canonical workflow
Files:
Makefile
🧠 Learnings (2)
📚 Learning: 2025-08-24T17:15:21.337Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.337Z
Learning: Applies to Makefile : Provide and maintain Make targets (`install`, `setup-dev`, `test-unit`, `test-e2e`, `lint`, `run-dev`, `run-prod`, `build-prod`, `clean`) as the canonical workflow
Applied to files:
Makefile
📚 Learning: 2025-08-24T17:15:21.337Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-24T17:15:21.337Z
Learning: Applies to Dockerfile : Keep Docker configuration consistent with documented run commands (exposing port 5000, running uvicorn with `api.index:app`)
Applied to files:
Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
Makefile (1)
65-70: LGTM: dynamic PORT with sensible defaults and correct bind addressesGood change. Using $${PORT:-5000} gives a safe default while allowing overrides, and keeping --reload only in run-dev with 127.0.0.1 vs 0.0.0.0 in run-prod is the right split. Matches our documented uvicorn entrypoint api.index:app.
1 participant
Summary by CodeRabbit
New Features
Style
Refactor
Chores