Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
rebased staging from Main
Add concurrency configuration for GitHub Actions.
Add concurrency settings to GitHub Actions workflow
Add concurrency settings to Playwright workflow
Completed Working on "Code Review"✅ Workflow completed successfully. |
📝 WalkthroughWalkthroughAdds a HealerAgent LLM loop to heal SQL errors, threads database type and user rules through analysis and execution, replaces loader distinct/counts with sampling and batched schema descriptions, and introduces frontend ChatContext, Settings UI, theme tokens, DB rule storage endpoints, expanded error handling, CI concurrency, and E2E sync fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Text2SQL
participant AnalysisAgent
participant HealerAgent
participant LLM
participant DB as Database
Client->>Text2SQL: request SQL for user query (includes db_type, use_user_rules/use_memory)
Text2SQL->>AnalysisAgent: get_analysis(user_query, schema, db_description, database_type, user_rules_spec)
AnalysisAgent->>LLM: generate sql_candidate
LLM-->>AnalysisAgent: sql_candidate
AnalysisAgent-->>Text2SQL: answer_an (includes sql_query)
Text2SQL->>DB: execute(sql_query)
alt DB executes successfully
DB-->>Text2SQL: results
Text2SQL-->>Client: stream results
else execution error
DB--x Text2SQL: error
Text2SQL->>HealerAgent: heal_and_execute(initial_sql, error, execute_func, db_description, question, database_type)
HealerAgent->>LLM: healing prompt (error + hints + DB type)
LLM-->>HealerAgent: healed_sql
HealerAgent->>DB: execute(healed_sql)
alt healed success
DB-->>HealerAgent: results
HealerAgent-->>Text2SQL: healed_sql + results (healing_success)
Text2SQL-->>Client: stream healed results
else healed failure (after attempts)
HealerAgent-->>Text2SQL: healing_failed + diagnostics
Text2SQL-->>Client: emit failure/error
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
|
Static Code Review 📊 🔁 Analyzing this pull request for issues... |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
![typo-app[bot]](https://avatars.githubusercontent.com/in/360763?s=60&v=4){kind=small}
Replaced demo video UI image with new GIF.
Update demo GIF in README
typo-app
bot
added
Manual Review Time 5-10 min
and removed
Manual Review Time 5-10 min
labels
- Fix dialog/modal padding to ensure equal spacing on left and right sides - Reduce suggestion card size by decreasing padding and gaps on mobile - Reorganize mobile header to reduce vertical space: - Move GitHub stars link next to profile icon in top row - Move connected database badge next to tagline text
…acing-and-header-layout fix(mobile): improve mobile UI spacing and header layout
typo-app
bot
added
Manual Review Time 30-60 min
and removed
Manual Review Time 5-10 min
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/pages/Index.tsx (2)
100-112: Consider adding HTTP error handling for robustness.The GitHub API fetch lacks explicit HTTP status checking. While it degrades gracefully to the default
'-', adding response validation would improve observability when the API is rate-limited or returns errors.🔎 Suggested enhancement
useEffect(() => { fetch('https://api.github.com/repos/FalkorDB/QueryWeaver') .then(response => response.json()) + .then(response => { + if (!response.ok) { + throw new Error(`GitHub API responded with status ${response.status}`); + } + return response.json(); + }) .then(data => { if (typeof data.stargazers_count === 'number') { setGithubStars(data.stargazers_count.toLocaleString()); } }) .catch(error => { console.log('Failed to fetch GitHub stars:', error); }); }, []);
366-385: LGTM! Well-implemented GitHub link with proper accessibility.The GitHub stars link includes proper security attributes (
noopener noreferrer), accessibility support (title), and displays live star counts.Note: This implementation is duplicated in the mobile header (lines 447-464). Consider extracting it into a reusable component if further customization is planned.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/components/SuggestionCards.tsxapp/src/components/ui/dialog.tsxapp/src/pages/Index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/src/components/ui/dialog.tsxapp/src/components/SuggestionCards.tsxapp/src/pages/Index.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: FalkorDB/QueryWeaver PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-02T15:24:21.682Z
Learning: Applies to .github/workflows/*.yml : GitHub Actions workflows should set up Python 3.12, use pipenv (sync --dev), create a CI .env using FALKORDB_URL, start FalkorDB for tests, and install Playwright browsers for E2E
🧬 Code graph analysis (1)
app/src/components/SuggestionCards.tsx (1)
app/src/components/ui/card.tsx (1)
CardContent(43-43)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: unit-tests
- GitHub Check: test
- GitHub Check: unit-tests
🔇 Additional comments (4)
app/src/components/SuggestionCards.tsx (1)
11-11: LGTM! Clean responsive spacing adjustments.The reduced gap and padding on mobile devices create a more compact layout while maintaining comfortable spacing on larger screens. No functional or accessibility concerns.
Also applies to: 28-28
app/src/components/ui/dialog.tsx (1)
39-39: LGTM! Improved responsive dialog layout.The updated positioning creates a better mobile experience by using edge insets (
left-4 right-4) on small screens while maintaining the centered layout on desktop. No functional changes.app/src/pages/Index.tsx (2)
446-504: LGTM! Mobile header properly integrates GitHub link.The mobile header successfully incorporates the GitHub stars link alongside the existing authentication UI, with appropriate responsive sizing and layout adjustments.
507-519: LGTM! Good mobile optimization with flex-shrink-0.The
flex-shrink-0on badges ensures they maintain their size and readability, while the shortened text labels improve mobile layout. The tagline and status positioning is clear and functional.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @api/loaders/mysql_loader.py:
- Around line 56-74: The code in _execute_sample_query interpolates table_name
and col_name directly into an f-string which risks SQL injection if those
identifiers ever come from untrusted sources; fix it by validating both
table_name and col_name against a strict identifier regex (e.g. only letters,
digits and underscores, ^[A-Za-z0-9_]+$) before interpolating, then safely
format them into the query using backticks while continuing to pass sample_size
as a parameter to cursor.execute; update _execute_sample_query to raise/handle
an error if validation fails so untrusted names are never used.
🧹 Nitpick comments (3)
api/loaders/base_loader.py (1)
61-66: Consider extending primitive type check to includeboolanddatetime.The current type check filters to
str,int,float, but databases commonly have boolean and datetime columns that would also be useful as sample values.♻️ Suggested enhancement
+import datetime + ... - if isinstance(first_val, (str, int, float)): + if isinstance(first_val, (str, int, float, bool, datetime.date, datetime.datetime)): return [str(v) for v in sample_values]api/loaders/graph_loader.py (1)
34-35: Return value ofcreate_combined_descriptionis discarded.The function modifies
entitiesin-place and returns it, so this works correctly. However, for clarity and consistency with the function's return type, consider either assigning the result or documenting that the function operates in-place.♻️ Option: Explicitly use the return value
- create_combined_description(entities) + entities = create_combined_description(entities)api/utils.py (1)
67-70: Variable shadowing:table_propreassigned in loop.The loop variable
table_propis reassigned to a copy of itself, which can be confusing. Consider using a different variable name for clarity.♻️ Suggested fix
for table_name, table_prop in table_info.items(): # The col_descriptions property is duplicated in the schema (columns has it) - table_prop = table_prop.copy() - table_prop.pop("col_descriptions", None) + table_prop_filtered = table_prop.copy() + table_prop_filtered.pop("col_descriptions", None) messages = [ {"role": "system", "content": system_prompt}, { "role": "user", "content": user_prompt_template.format( - table_name=table_name, table_prop=json.dumps(table_prop) + table_name=table_name, table_prop=json.dumps(table_prop_filtered) ), }, ]
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/loaders/base_loader.pyapi/loaders/graph_loader.pyapi/loaders/mysql_loader.pyapi/loaders/postgres_loader.pyapi/utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/loaders/graph_loader.pyapi/utils.pyapi/loaders/base_loader.pyapi/loaders/postgres_loader.pyapi/loaders/mysql_loader.py
🧬 Code graph analysis (4)
api/loaders/graph_loader.py (1)
api/utils.py (2)
generate_db_description(106-163)create_combined_description(36-104)
api/utils.py (1)
api/config.py (1)
Config(62-148)
api/loaders/base_loader.py (2)
api/loaders/mysql_loader.py (1)
_execute_sample_query(57-74)api/loaders/postgres_loader.py (1)
_execute_sample_query(54-76)
api/loaders/postgres_loader.py (2)
api/loaders/base_loader.py (2)
_execute_sample_query(26-40)extract_sample_values_for_column(43-67)api/loaders/mysql_loader.py (1)
_execute_sample_query(57-74)
🪛 Ruff (0.14.10)
api/utils.py
49-49: Avoid specifying long messages outside the exception class
(TRY003)
api/loaders/mysql_loader.py
64-70: Possible SQL injection vector through string-based query construction
(S608)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: unit-tests
- GitHub Check: unit-tests
- GitHub Check: test
🔇 Additional comments (7)
api/loaders/base_loader.py (1)
26-40: Abstract method signature looks correct.The new
_execute_sample_queryabstract method provides a clean interface for database-specific sampling implementations. The signature aligns with the implementations inmysql_loader.pyandpostgres_loader.py.api/loaders/graph_loader.py (1)
128-134: Sample values integration looks correct.The logic correctly:
- Creates the embedding from the original description (before appending sample values)
- Appends formatted sample values to create
final_descriptionfor storage- Uses
final_descriptionin the graph queryThis ensures embeddings are based on clean descriptions while stored descriptions contain helpful sample data.
api/utils.py (2)
10-34: Well-structured TypedDicts for table metadata.The new type definitions (
ForeignKeyInfo,ColumnInfo,TableInfo) provide good type safety and documentation for the schema metadata structure used across loaders.
84-103: Good batch processing with graceful error handling.The batch processing logic correctly:
- Processes tables in configurable batch sizes
- Handles individual batch failures by falling back to the table name as description
- Properly maps responses back to their corresponding tables
One minor consideration: if
batch_completionitself raises an exception (vs returning Exception objects), it would propagate up. Verify this is the intended behavior.api/loaders/postgres_loader.py (2)
53-76: Secure PostgreSQL sampling implementation.Good use of
psycopg2.sql.Identifierfor parameterized identifiers, preventing SQL injection. The subquery approach ensures distinct values before random sampling, which is the correct behavior.
274-286: Sample values correctly integrated into column metadata.The implementation properly:
- Calls
extract_sample_values_for_column(inherited fromBaseLoader)- Stores
sample_valuesas a separate field incolumns_info- Keeps the description clean without embedded sample values
This aligns well with the downstream usage in
graph_loader.py.api/loaders/mysql_loader.py (1)
318-330: Sample values integration consistent with PostgreSQL loader.The implementation mirrors the PostgreSQL version correctly, maintaining consistency across database loaders.
Prompt Reasoning
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @api/agents/analysis_agent.py:
- Line 170: The pylint disable directive on the _build_prompt function is
malformed because it contains `disable=` twice; update the decorator/string so
the disable list is correct (e.g., "pylint: disable=too-many-arguments,
too-many-positional-arguments, line-too-long") by removing the redundant
`disable=` before line-too-long on the _build_prompt definition so pylint
recognizes all three checks as disabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/agents/analysis_agent.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/agents/analysis_agent.py
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: unit-tests
- GitHub Check: test
- GitHub Check: unit-tests
🔇 Additional comments (3)
api/agents/analysis_agent.py (3)
21-21: LGTM! Database type parameter added correctly.The optional
database_typeparameter is properly typed and positioned, supporting database-aware SQL analysis.
35-37: Database type threading implemented correctly.The
database_typeparameter is properly threaded through_build_prompt, documented in the docstring, and incorporated into both the system message and prompt body. The updated JSON schema description (lines 287-289) correctly references "target database."Note: Database type appears in both the system message (lines 30-31) and prompt body (line 212), which provides redundant context but may be intentional for emphasis.
Also applies to: 170-174, 184-184, 212-212, 287-289
26-33: Remove this comment - the premise is invalid.
BaseAgentdoes not initialize any system message. It only initializesself.messagesas empty or populates it with user/assistant messages from history. The system message insertion logic at lines 26-33 correctly adds the database-type-specific system message on the first call toget_analysis().Likely an incorrect or invalid review comment.
Bump aiohttp from 3.13.2 to 3.13.3 in the pip group across 1 directory
…ht-1.57.0 Bump playwright from 1.56.0 to 1.57.0
- update the sidebar display for moblie inside the settings tab - add the option for the user to open the schema viewer inside settings tab. - update the save option to be displayed only when new changes occurs. - update the sizes for components inside settings for moblie display.
Prompts Usage updates
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @api/graph.py:
- Around line 73-83: Keep MERGE in set_user_rules to preserve defensive writes,
but ensure only one Database node exists per graph: add a uniqueness constraint
or explicit initialization when creating/loading a graph (e.g., in
load_to_graph) that creates a single Database node with a stable key (like
graph_id or a known marker property) or create a schema constraint/index for
:Database on that key; update or document the invariant so get_db_description
and get_user_rules can rely on a single Database node and avoid duplicate nodes.
In @app/src/components/layout/Sidebar.tsx:
- Around line 144-146: The E2E page object is missing a locator and helper
methods for the Settings button referenced by testId "settings-button"; add a
private getter `settingsBtn(): Locator` that returns
`this.page.getByTestId("settings-button")` in `e2e/logic/pom/sidebar.ts`, then
add matching interaction and assertion methods following the pattern of other
sidebar buttons (e.g., `async clickSettings()` that calls
`this.settingsBtn.click()` and a visibility/assertion method like `async
isSettingsVisible()` or `async expectSettingsVisible()` to mirror existing
method naming and behavior).
In @app/src/index.css:
- Around line 240-241: The dark theme block defines --radius: 0.5rem but omits
the --spacing variable, causing inconsistency with :root and
[data-theme="light"]; add an explicit --spacing: 0.25rem to the same dark theme
block (next to --radius) so both themes explicitly declare --spacing and
--radius rather than relying on inheritance.
- Around line 13-40: The OKLch variables (e.g., --background, --foreground,
--primary, --destructive, etc.) are great but need fallbacks for older browsers
and cross-gamut safety: for each OKLch custom property, add a fallback
declaration using sRGB (hex or rgb()) immediately before the oklch() value (so
user agents that don't support OKLch will pick the fallback) and ensure critical
accent/destructive colors have gamut-safe sRGB equivalents tested for Display P3
edge cases; update variables like --background, --primary, --accent,
--destructive, --card, --popover and their -foreground counterparts to include
these fallbacks and verify rendering on sRGB vs Display P3 displays.
In @app/src/pages/Index.tsx:
- Around line 36-51: useMemory and useRulesFromDatabase are only read from
localStorage during initial render (in the useState initializer) so they never
update when Settings.tsx changes localStorage; add synchronization so Index.tsx
reflects changes without a full reload: either subscribe to the window "storage"
event in Index.tsx (listen for keys "queryweaver_use_memory" and
"queryweaver_use_rules_from_database" and call setUseMemory /
setUseRulesFromDatabase accordingly) or lift these values into a shared context
provider and consume the context in Index.tsx, ensuring Settings.tsx updates the
same context; implement one of these approaches and remove reliance on one-time
localStorage reads.
In @app/src/pages/Settings.tsx:
- Around line 369-376: The "Sign In" Button elements in Settings.tsx (the Sign
In button in the desktop header around the Button JSX shown and the duplicate in
the mobile header) are missing onClick handlers; add an onClick that triggers
your auth flow (for example call signIn() from next-auth/react—import signIn at
the top—or use router.push('/signin') if you have a dedicated route) on both
Button instances so they become interactive, and add an aria-label (e.g.,
aria-label="Sign in") for accessibility.
🧹 Nitpick comments (10)
app/src/components/ui/switch.tsx (1)
6-24: Excellent implementation following React best practices.The Switch component correctly uses
React.forwardRefwith proper TypeScript typing, forwards all props and refs appropriately, and includes comprehensive accessibility features (focus-visible rings, disabled states). The use of Radix UI primitives provides a solid, accessible foundation.The long className string on line 12 is functional but could optionally be split across multiple lines for easier scanning.
♻️ Optional: Break long className for readability
<SwitchPrimitives.Root className={cn( - "peer inline-flex h-6 w-11 shrink-0 cursor-pointer items-center rounded-full border-2 border-transparent transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 focus-visible:ring-offset-background disabled:cursor-not-allowed disabled:opacity-50 data-[state=checked]:bg-primary data-[state=unchecked]:bg-input", + "peer inline-flex h-6 w-11 shrink-0 cursor-pointer items-center rounded-full border-2 border-transparent", + "transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring", + "focus-visible:ring-offset-2 focus-visible:ring-offset-background", + "disabled:cursor-not-allowed disabled:opacity-50", + "data-[state=checked]:bg-primary data-[state=unchecked]:bg-input", className )} {...props}api/agents/analysis_agent.py (1)
193-244: Suggest simplifying the prompt construction logic.The nested conditionals and string building for
instructions_section,user_rules_section,memory_section,memory_instructions, andmemory_evaluation_guidelinesare complex and could be simplified for maintainability.♻️ Proposed refactor
Consider extracting each section into a dedicated helper method:
def _build_instructions_section(self, instructions: str) -> str: if not instructions: return "" return f""" <instructions> {instructions} </instructions> """ def _build_user_rules_section(self, user_rules_spec: str) -> str: if not user_rules_spec: return "" return f""" <user_rules_spec> {user_rules_spec} </user_rules_spec> """ def _build_memory_section(self, memory_context: str) -> tuple[str, str, str]: """Returns (memory_section, memory_instructions, memory_evaluation_guidelines)""" if not memory_context: return "", "", "" memory_section = f""" <memory_context> The following information contains relevant context from previous interactions: {memory_context} Use this context to: 1. Better understand the user's preferences and working style 2. Leverage previous learnings about this database 3. Learn from SUCCESSFUL QUERIES patterns and apply similar approaches 4. Avoid FAILED QUERIES patterns and the errors they caused </memory_context> """ memory_instructions = """ - Use <memory_context> only to resolve follow-ups and previously established conventions. - Do not let memory override the schema, <user_rules_spec>, or <instructions>. """ memory_evaluation_guidelines = """ 13. If <memory_context> exists, use it only for resolving follow-ups or established conventions; do not let memory override schema, <user_rules_spec>, or <instructions>. """ return memory_section, memory_instructions, memory_evaluation_guidelinesThen in
_build_prompt:instructions_section = self._build_instructions_section(instructions) user_rules_section = self._build_user_rules_section(user_rules_spec) memory_section, memory_instructions, memory_evaluation_guidelines = self._build_memory_section(memory_context)api/routes/graphs.py (2)
237-250: Consider usinglogging.exceptionfor better diagnostics.The error handling follows existing patterns and correctly returns appropriate status codes. However, using
logging.exceptioninstead oflogging.erroron line 249 would automatically include the stack trace, improving debugging capabilities.♻️ Suggested improvement
except Exception as e: # pylint: disable=broad-exception-caught - logging.error("Error getting user rules: %s", str(e)) + logging.exception("Error getting user rules: %s", str(e)) return JSONResponse(content={"error": "Failed to get user rules"}, status_code=500)Based on static analysis hints.
253-270: Consider usinglogging.exceptionfor better diagnostics.The endpoint correctly updates user rules and handles errors appropriately. Using
logging.exceptionon lines 266 and 269 would provide stack traces for easier debugging.♻️ Suggested improvement
except GraphNotFoundError: - logging.error("Graph not found") + logging.exception("Graph not found") return JSONResponse(content={"error": "Database not found"}, status_code=404) except Exception as e: # pylint: disable=broad-exception-caught - logging.error("Error updating user rules: %s", str(e)) + logging.exception("Error updating user rules: %s", str(e)) return JSONResponse(content={"error": "Failed to update user rules"}, status_code=500)Based on static analysis hints.
app/src/services/database.ts (1)
306-338: Consider reducing console.log statements for production.The method correctly updates user rules with proper error handling. However, there are multiple console.log statements (lines 311, 313, 324, 333) that may be excessive for production code. Consider removing or consolidating these logs.
♻️ Suggested cleanup
static async updateUserRules(graphId: string, userRules: string): Promise<void> { try { - console.log('Updating user rules for graph:', graphId, 'Length:', userRules.length); const url = buildApiUrl(`${API_CONFIG.ENDPOINTS.GRAPHS}/${graphId}/user-rules`); - console.log('PUT request to:', url); const response = await fetch(url, { method: 'PUT', credentials: 'include', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify({ user_rules: userRules }), }); - console.log('Update user rules response status:', response.status); if (!response.ok) { const errorData = await response.json().catch(() => ({})); console.error('Failed to update user rules:', errorData); throw new Error('Failed to update user rules'); } const result = await response.json(); - console.log('User rules updated successfully:', result); + console.log('User rules updated successfully'); } catch (error) { console.error('Error updating user rules:', error); throw error; } }app/src/pages/Settings.tsx (1)
280-286:onSettingsClickis a no-op on the Settings page.Passing an empty function
() => {}foronSettingsClickis intentional since we're already on the Settings page, but it would be clearer to omit the prop entirely or add a comment explaining why.app/src/components/layout/Sidebar.tsx (1)
97-106: Consider whether both callback and navigation are needed.
handleSettingsClickcallsonSettingsClick()before navigating. If the callback is always empty (as in Settings.tsx) or not provided, this is fine. However, ifonSettingsClickperforms async work or has side effects, the navigation will happen immediately after without waiting.If
onSettingsClickis only used for toggling UI state on the same page, consider simplifying by making it optional or removing the navigation whenonSettingsClickis provided.app/src/index.css (1)
12-96: Significant duplication between:rootand[data-theme="light"].The
:rootblock (lines 12-96) and[data-theme="light"]block (lines 98-176) define identical values. Consider defining light mode only in:rootand removing the duplicate[data-theme="light"]block, or vice versa, to improve maintainability.♻️ Proposed approach
Keep
:rootas the default (light mode) and remove the duplicate[data-theme="light"]block entirely. The light theme will be inherited from:rootwhendata-theme="light"is set or when no theme is set.:root { /* Light Mode Colors - OKLch */ ...all light mode variables... } - [data-theme="light"] { - /* Light Mode Colors - OKLch (same as :root) */ - ...duplicate variables... - } [data-theme="dark"] { /* Dark Mode Colors - OKLch */ ...dark mode variables... }Also applies to: 98-176
api/core/text2sql.py (2)
493-496: Use bareraiseto preserve original traceback.Per static analysis hint, when re-raising the same exception, use bare
raiseinstead ofraise exec_errorto preserve the original traceback.♻️ Proposed fix
if not healing_result.get("success"): # Healing failed after all attempts yield json.dumps({ "type": "healing_failed", ... }) + MESSAGE_DELIMITER - raise exec_error + raise
471-471: Consider makingmax_healing_attemptsconfigurable.The hardcoded value of
3formax_healing_attemptsworks, but consider making it a configuration parameter inConfigfor easier tuning without code changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/agents/analysis_agent.pyapi/core/text2sql.pyapi/graph.pyapi/routes/graphs.pyapp/package.jsonapp/src/App.tsxapp/src/components/chat/ChatInterface.tsxapp/src/components/chat/ChatMessage.tsxapp/src/components/layout/Sidebar.tsxapp/src/components/modals/SettingsModal.tsxapp/src/components/ui/switch.tsxapp/src/contexts/ChatContext.tsxapp/src/index.cssapp/src/pages/Index.tsxapp/src/pages/Settings.tsxapp/src/services/chat.tsapp/src/services/database.tsapp/src/types/api.tsapp/tailwind.config.ts
✅ Files skipped from review due to trivial changes (1)
- app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/chat/ChatMessage.tsx
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/src/components/ui/switch.tsxapp/tailwind.config.tsapp/src/components/modals/SettingsModal.tsxapp/src/components/layout/Sidebar.tsxapp/src/contexts/ChatContext.tsxapp/src/types/api.tsapp/src/pages/Settings.tsxapp/src/services/database.tsapp/src/components/chat/ChatInterface.tsxapp/src/pages/Index.tsxapp/src/App.tsxapp/src/services/chat.ts
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/graph.pyapi/routes/graphs.pyapi/core/text2sql.pyapi/agents/analysis_agent.py
🧬 Code graph analysis (8)
app/src/components/ui/switch.tsx (1)
app/src/lib/utils.ts (1)
cn(4-6)
app/src/components/modals/SettingsModal.tsx (4)
app/src/components/ui/dialog.tsx (5)
Dialog(85-85)DialogContent(90-90)DialogHeader(91-91)DialogTitle(93-93)DialogDescription(94-94)app/src/components/ui/label.tsx (1)
Label(17-17)app/src/components/ui/textarea.tsx (1)
Textarea(21-21)app/src/components/ui/button.tsx (1)
Button(47-47)
app/src/components/layout/Sidebar.tsx (3)
e2e/logic/pom/sidebar.ts (1)
Sidebar(14-141)app/hooks/use-mobile.ts (1)
useIsMobile(4-18)app/src/lib/utils.ts (1)
cn(4-6)
app/src/contexts/ChatContext.tsx (2)
app/src/types/api.ts (1)
ConversationMessage(44-47)app/src/contexts/DatabaseContext.tsx (1)
useDatabase(104-110)
app/src/pages/Settings.tsx (10)
app/src/contexts/AuthContext.tsx (1)
useAuth(66-72)app/src/contexts/DatabaseContext.tsx (1)
useDatabase(104-110)app/src/services/database.ts (1)
databaseService(341-341)app/src/components/ui/badge.tsx (1)
Badge(29-29)app/src/components/ui/dropdown-menu.tsx (5)
DropdownMenu(164-164)DropdownMenuTrigger(165-165)DropdownMenuContent(166-166)DropdownMenuItem(167-167)DropdownMenuSeparator(171-171)app/src/components/ui/button.tsx (1)
Button(47-47)app/src/components/ui/avatar.tsx (3)
Avatar(38-38)AvatarImage(38-38)AvatarFallback(38-38)app/src/components/ui/label.tsx (1)
Label(17-17)app/src/components/ui/switch.tsx (1)
Switch(27-27)app/src/components/ui/textarea.tsx (1)
Textarea(21-21)
api/routes/graphs.py (4)
api/core/text2sql.py (1)
_graph_name(100-109)api/graph.py (2)
get_user_rules(57-70)set_user_rules(73-82)api/auth/user_management.py (1)
token_required(251-291)api/core/errors.py (1)
GraphNotFoundError(8-9)
app/src/services/database.ts (1)
app/src/config/api.ts (2)
buildApiUrl(40-42)API_CONFIG(2-37)
app/src/components/chat/ChatInterface.tsx (4)
app/src/contexts/DatabaseContext.tsx (1)
useDatabase(104-110)app/src/contexts/ChatContext.tsx (1)
useChat(93-99)app/src/components/ui/skeleton.tsx (1)
Skeleton(7-7)app/src/lib/utils.ts (1)
cn(4-6)
🪛 Ruff (0.14.10)
api/routes/graphs.py
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
266-266: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
268-268: Do not catch blind exception: Exception
(BLE001)
269-269: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
api/core/text2sql.py
495-495: Use raise without specifying exception name
Remove exception name
(TRY201)
api/agents/analysis_agent.py
246-386: Possible SQL injection vector through string-based query construction
(S608)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: unit-tests
- GitHub Check: test
- GitHub Check: unit-tests
🔇 Additional comments (30)
app/src/components/ui/switch.tsx (3)
1-4: LGTM! Clean imports.The imports are well-organized and include all necessary dependencies for the Switch component.
25-27: LGTM! Proper displayName and export.Setting the
displayNameimproves the debugging experience, and the export follows standard patterns.
1-27: Verify compilation in build process.This frontend TypeScript component must be compiled before production runs. Please ensure it's included in your build pipeline.
As per coding guidelines for files in
app/**/*.{ts,tsx}.app/src/types/api.ts (2)
40-41: LGTM! Clean type additions.The optional flags
use_user_rulesanduse_memoryappropriately extend the request payload to support the new backend features for rules and memory management.
92-92: LGTM! Consistent type addition.The
use_user_rulesflag inConfirmRequestaligns with the same field inChatRequest, maintaining consistency across the API types.app/src/contexts/ChatContext.tsx (3)
57-68: LGTM! Proper graph change detection.The use of
previousGraphIdRefcorrectly prevents resetting the chat on initial mount while ensuring conversation history is cleared when switching between different databases. This prevents data leakage across different database contexts.
70-77: LGTM! Clean reset implementation.The
resetChatcallback properly clears conversation history and reinitializes with a fresh message, usingDate.now().toString()for unique IDs. The use ofuseCallbackwith an empty dependency array is appropriate since the function doesn't depend on any props or state.
93-99: LGTM! Standard context hook pattern.The runtime guard follows React best practices and matches the pattern used in
DatabaseContext.tsx(as shown in relevant code snippets).api/agents/analysis_agent.py (2)
246-386: SQL injection concern is a false positive.The Ruff warning (S608) flags string-based SQL construction, but this prompt is for an LLM that generates SQL—it's not executing SQL directly. The user inputs are embedded in the prompt as instructions, not as SQL values. The actual SQL execution happens elsewhere with proper parameterization.
41-45: The removal oftop_pis correct and follows litellm best practices. Withtemperature=0, the model uses greedy decoding for deterministic output, makingtop_p(nucleus sampling) unnecessary. When using litellm's completion API withtemperature=0,top_pshould be left at its default value or unset—this is the recommended configuration for deterministic LLM behavior.app/src/App.tsx (1)
18-30: LGTM! Proper context provider hierarchy.The
ChatProvideris correctly positioned in the provider hierarchy (afterDatabaseProvidersince it usesuseDatabase), and the Settings route is properly placed before the catch-all route.api/graph.py (1)
57-71: LGTM! Consistent implementation.The
get_user_rulesfunction follows the same pattern asget_db_description(lines 39-54), properly handling the case when the result is empty or None by returning an empty string.app/src/components/modals/SettingsModal.tsx (1)
1-109: LGTM! Well-structured modal component.The implementation follows React best practices with proper state management, prop synchronization, and clear UI structure. The component correctly syncs with
initialRulesvia useEffect and provides intuitive Clear/Cancel/Save actions.app/src/services/database.ts (3)
193-207: LGTM! Improved error handling with specific status code messages.The enhanced error handling provides clearer feedback for authentication, permission, and server errors. The pattern of extracting server error messages for 400 status is well implemented.
280-304: LGTM! Defensive error handling aligns with existing patterns.The method correctly handles authentication failures by returning an empty string, consistent with the
getGraphspattern. This prevents UI disruption when users aren't authenticated.
341-341: LGTM! Export alias enhances code flexibility.Adding the
databaseServicealias allows consumers to import either the class or the instance based on their usage pattern.app/src/services/chat.ts (1)
20-51: LGTM! Correctly separates chat history by role.The updated implementation properly splits conversation history into
chatHistory(user queries) andresultHistory(assistant responses) to match the backend's expected format. The conditional inclusion of optional fields (use_user_rules,use_memory) is well-implemented.app/tailwind.config.ts (2)
4-4: LGTM! Enhanced dark mode support.The selector-based approach with
data-themeattribute support provides more flexible theming control compared to class-only dark mode.
24-82: All CSS variables are properly defined and migration is complete.The tailwind config correctly uses CSS variables for all color tokens (primary, secondary, destructive, success, warning, error, info, muted, accent, sidebar variants, etc.) and each variable is defined across light and dark mode themes in app/src/index.css. The semantic color groups add good maintainability.
app/src/pages/Settings.tsx (2)
198-217: Unmount save may silently fail or execute with stale data.The unmount effect fires an async save without awaiting. While this is a common pattern for cleanup, consider:
- If
isLoadedis false (still loading), we skip saving, which is good.- However, if the component unmounts during loading and then loading completes, the refs may have unexpected values.
The current implementation is reasonable but relies on
initialRulesLoadedRefbeing set correctly. EnsureinitialRulesLoadedRef.currentis set totrueonly after successful load (line 182 confirms this).
1-76: Well-structured state management with proper localStorage persistence.The initialization pattern for
useMemoryanduseRulesFromDatabasewith localStorage fallbacks is clean. The use of refs (loadedRulesRef,currentRulesRef, etc.) to track values for unmount cleanup avoids stale closure issues.app/src/components/layout/Sidebar.tsx (1)
47-47: Design token migration looks correct.The icon styling has been updated from static colors (
text-gray-400 hover:bg-gray-800) to design tokens (text-muted-foreground hover:bg-card hover:text-foreground), consistent with the theming system changes.Also applies to: 62-62, 75-75
app/src/components/chat/ChatInterface.tsx (3)
39-53: Clean prop expansion with sensible defaults.The new
useMemoryanduseRulesFromDatabaseprops have appropriate default values (trueandtrue) that match the expected behavior. The interface extension is backward-compatible.
73-76: Skeleton styling updated to design tokens.The
bg-mutedclass replaces hardcoded colors, consistent with the theming system migration.
327-335: Theuse_memoryflag is intentionally excluded fromstreamConfirmOperation.The backend's
ConfirmRequestmodel does not include ause_memoryfield, andexecute_destructive_operationalways creates a memory tool regardless of any flag. This appears to be by design—destructive operations always have memory context available, while regular queries viastreamQueryallow opt-in/out control. No change needed.app/src/pages/Index.tsx (2)
530-531: Display name change for DEMO_CRM to "CRM".The mobile badge now shows "CRM" instead of "DEMO_CRM" for better UX on smaller screens. This is a cosmetic improvement.
364-403: Header theming and GitHub stars display look correct.The design token migration (
text-muted-foreground,bg-card,border-border) is consistent with other components. The GitHub stars fetch and display provide useful social proof.api/core/text2sql.py (3)
455-523: Healing workflow is well-structured with proper error handling.The iterative healing approach with a maximum of 3 attempts is reasonable. The streaming of healing progress (attempt, success, failed events) provides good visibility to the user. The healer agent is created inside the exception handler, which is appropriate since it's only needed on failure.
639-690: Memory operations properly guarded byuse_memoryflag.The conditional checks for
memory_tool_taskcorrectly ensure memory operations only run whenuse_memoryis enabled. The background task pattern withadd_done_callbackfor error logging is appropriate for non-blocking saves.
260-266: User rules and database type correctly threaded through generation flow.The
use_user_rulesflag now controls whether rules are fetched from the database (line 262), anddb_typeis passed to the analysis agent (line 326). This aligns with the frontend toggle behavior.
Fix demo rules usage
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @api/routes/graphs.py:
- Around line 237-250: In get_graph_user_rules (the route handler), replace the
generic logging.error(...) in the broad except Exception as e block with
logging.exception(...) so the stack trace is recorded automatically; keep the
same JSONResponse content and status_code, only change the logging call to
logging.exception("Error getting user rules: %s", e) (or equivalent) to include
full diagnostics.
- Around line 253-277: In update_graph_user_rules, replace the two logging.error
calls inside the exception handlers with logging.exception so stack traces are
recorded: change the GraphNotFoundError handler's logging.error("Graph not
found") to logging.exception("Graph not found") and change the broad except
handler's logging.error("Error updating user rules: %s", str(e)) to
logging.exception("Error updating user rules", exc_info=True) (or
logging.exception with the same message) so the function GraphNotFoundError and
the generic except block both emit full traceback information.
In @app/src/pages/Settings.tsx:
- Around line 199-217: The Sidebar navigation bypasses your handleBackClick save
logic causing possible data loss; replace the unprotected direct navigate('/')
with a navigation blocker that intercepts all route changes and performs the
same save-and-update-loadedRules flow as handleBackClick. Specifically, add
React Router's useBlocker (or equivalent navigation blocking) in this component,
watch blocker.state === 'blocked', then read currentGraphIdRef, currentRulesRef,
loadedRulesRef and useRulesFromDatabaseRef and call
databaseService.updateUserRules(graphId, currentRules) and on success call
blocker.proceed() and update loadedRulesRef to currentRules, on failure call
blocker.reset() and log the error; remove reliance on the unmount cleanup to
complete async saves since cleanup cannot await navigation.
In @app/src/services/database.ts:
- Around line 193-207: Extract the duplicated error handling into a private
static helper on DatabaseService (e.g.,
DatabaseService.handleDatabaseConnectionError) and replace the duplicated blocks
in connectDatabaseUrl and connectDatabase with a single await
DatabaseService.handleDatabaseConnectionError(response) call that throws the
appropriate Error; the helper should contain the errorMessages map, handle
status === 400 by reading response.json() and throwing errorData.error ||
'Invalid database connection URL.', and otherwise read response.json() and throw
errorData.error || errorMessages[response.status] || `Failed to connect to
database (${response.status})`.
- Around line 280-304: getUserRules currently swallows all exceptions and
returns an empty string, hiding network/server problems; change it so only
authentication/authorization cases (response.status 401/403) return '' while
other HTTP errors and unexpected exceptions are propagated: keep the existing
response.ok check and 401/403 branch in getUserRules, but for other non-ok
statuses throw a descriptive Error (including status/text) instead of falling
through, and in the catch block rethrow the error (or omit the try/catch) so
callers can handle/log failures; reference getUserRules, buildApiUrl and
API_CONFIG.ENDPOINTS.GRAPHS when locating the code to update.
🧹 Nitpick comments (2)
app/src/services/database.ts (1)
306-338: Reduce verbose console logging in production code.The
updateUserRulesmethod contains five console.log statements (lines 311, 313, 324, 333, 335) that are overly verbose for production code. Consider reducing the logging to only error cases or using a proper logging framework with configurable log levels.♻️ Proposed cleanup
static async updateUserRules(graphId: string, userRules: string): Promise<void> { try { - console.log('Updating user rules for graph:', graphId, 'Length:', userRules.length); const url = buildApiUrl(`${API_CONFIG.ENDPOINTS.GRAPHS}/${graphId}/user-rules`); - console.log('PUT request to:', url); const response = await fetch(url, { method: 'PUT', credentials: 'include', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify({ user_rules: userRules }), }); - console.log('Update user rules response status:', response.status); if (!response.ok) { const errorData = await response.json().catch(() => ({})); console.error('Failed to update user rules:', errorData); throw new Error(errorData.error || 'Failed to update user rules'); } - const result = await response.json(); - console.log('User rules updated successfully:', result); + await response.json(); } catch (error) { console.error('Error updating user rules:', error); throw error; } }api/routes/graphs.py (1)
232-234: Consider adding validation to UserRulesRequest.The model accepts any string length for
user_rules. While the frontend limits input to 5000 characters (Settings.tsx line 559), adding backend validation would provide defense-in-depth.♻️ Optional validation enhancement
class UserRulesRequest(BaseModel): """User rules request model.""" - user_rules: str + user_rules: str = Field(..., max_length=5000, description="User-defined rules for SQL generation")Don't forget to import
Field:-from pydantic import BaseModel +from pydantic import BaseModel, Field
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/routes/graphs.pyapp/src/pages/Settings.tsxapp/src/services/database.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Files:
app/src/services/database.tsapp/src/pages/Settings.tsx
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/routes/graphs.py
🧬 Code graph analysis (3)
app/src/services/database.ts (1)
app/src/config/api.ts (2)
buildApiUrl(40-42)API_CONFIG(2-37)
app/src/pages/Settings.tsx (3)
app/src/contexts/AuthContext.tsx (1)
useAuth(66-72)app/src/contexts/DatabaseContext.tsx (1)
useDatabase(104-110)app/src/services/database.ts (1)
databaseService(341-341)
api/routes/graphs.py (4)
api/core/text2sql.py (1)
_graph_name(100-109)api/graph.py (2)
get_user_rules(57-70)set_user_rules(73-82)api/auth/user_management.py (1)
token_required(251-291)api/core/errors.py (1)
GraphNotFoundError(8-9)
🪛 Ruff (0.14.10)
api/routes/graphs.py
248-248: Do not catch blind exception: Exception
(BLE001)
249-249: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
273-273: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
275-275: Do not catch blind exception: Exception
(BLE001)
276-276: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: test
- GitHub Check: test
- GitHub Check: unit-tests
🔇 Additional comments (7)
app/src/services/database.ts (1)
341-341: LGTM!The export alias provides a convenient named export for consumers who prefer
databaseServiceover the class name.api/routes/graphs.py (1)
21-23: LGTM!The new imports are used correctly in the user rules endpoints below.
app/src/pages/Settings.tsx (5)
1-23: LGTM!Imports are well-organized and all appear to be used in the component.
24-75: LGTM!The state initialization is well-structured with proper SSR guards (
typeof window !== 'undefined') and sensible defaults. The ref pattern for unmount cleanup is a reasonable approach to capture current values.
143-187: LGTM!The effect correctly loads rules only when necessary and handles errors appropriately. The dependency array is correct.
Note: Including
toastin the dependency array (line 187) is technically correct, though it's a stable reference from theuseToasthook and won't cause re-runs.
255-275: LGTM!The back navigation handler correctly awaits the save operation before navigating and updates
loadedRulesRefto prevent double-saves. This is a well-implemented pattern.
277-604: LGTM!The JSX structure is well-organized with good responsive design patterns. The character limit (5000) is enforced in the UI and matches the recommended backend validation.
4 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
PR Summary by Typo
Overview
This PR introduces UI/UX enhancements, optimizes CI/CD workflows, and updates documentation. The changes primarily focus on improving the user interface layout and responsiveness, while also streamlining GitHub Actions to prevent redundant runs.
Key Changes
concurrencyin Playwright andtestsGitHub Actions to cancel older, in-progress runs.README.md.SuggestionCardsand adjusted dialog positioning for improved responsiveness.Index.tsx) to reposition the GitHub star display and refine the alignment of user authentication and database status elements.DEMO_CRMgraph to 'CRM' in the status badge.Work Breakdown
To turn off PR summary, please visit Notification settings.