-
Notifications
You must be signed in to change notification settings - Fork 28
Prompts Usage updates #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Completed Working on "Code Review"✅ Workflow completed successfully. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces user-defined SQL rules and optional memory management to a text2sql application. The backend adds rule specification support to the analysis agent, exposes graph-level user rules via API endpoints, and implements toggles for rule and memory usage. The frontend adds a Settings page with toggles for memory and database rules, UI components for rule management, and passes these flags through the chat service. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant Backend API
participant Graph DB
User->>Frontend: Toggle "Use Database Rules"<br/>(localStorage persisted)
User->>Frontend: Enter text query
Frontend->>Backend API: streamQuery with use_user_rules=true
rect rgb(200, 220, 255)
Backend API->>Graph DB: get_user_rules(graph_id)
Graph DB-->>Backend API: user_rules_spec (SQL rules)
end
Backend API->>Backend API: Analysis Agent: build prompt<br/>with user_rules_spec +<br/>safety rules + memory context
Backend API-->>Frontend: SQL response
Frontend-->>User: Display results
Note over Frontend,Backend API: Optional: User edits rules<br/>in Settings page
User->>Frontend: Update rules in modal
Frontend->>Backend API: PUT /graphs/{id}/user-rules
Backend API->>Graph DB: set_user_rules(graph_id, rules)
Graph DB-->>Backend API: ack
Backend API-->>Frontend: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple architectural layers (backend analysis logic, API endpoints, frontend state management, UI components) with varied patterns: prompt restructuring, conditional feature toggles, database persistence, localStorage state sync, and unmount lifecycle complexity in the Settings page. While individual files follow clear patterns, the heterogeneity of changes and interconnected logic across services, components, and data models requires careful tracing of the feature flow end-to-end. 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. 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 |
|
🚅 Deployed to the QueryWeaver-pr-363 environment in queryweaver
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces prompts usage updates by adding user rules and memory context configuration capabilities to QueryWeaver. Users can now define custom SQL generation rules per database and control memory context usage through a new Settings page.
Key Changes:
- Added support for user-defined rules stored per database with toggle-based control
- Introduced memory context toggle to enable/disable AI memory of previous interactions
- Created a dedicated Settings page for managing user rules and preferences
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/types/api.ts | Added optional use_user_rules and use_memory fields to chat and confirmation requests |
| app/src/services/database.ts | Implemented API methods to fetch and update user rules for databases |
| app/src/services/chat.ts | Extended chat service to pass user rules and memory toggles to backend |
| app/src/pages/Settings.tsx | New Settings page providing UI for managing user rules and memory toggles with auto-save functionality |
| app/src/pages/Index.tsx | Added localStorage-based state management for memory and rules toggles, passing them to ChatInterface |
| app/src/components/ui/switch.tsx | New reusable Switch component for toggle controls |
| app/src/components/modals/SettingsModal.tsx | Created settings modal (appears unused in favor of dedicated Settings page) |
| app/src/components/layout/Sidebar.tsx | Added Settings navigation button to sidebar with route-based active state |
| app/src/components/chat/ChatInterface.tsx | Integrated user rules and memory toggles into chat query workflow |
| app/src/App.tsx | Added /settings route to application routing |
| api/routes/graphs.py | Implemented GET/PUT endpoints for user rules per graph |
| api/graph.py | Added database operations for retrieving and storing user rules |
| api/core/text2sql.py | Integrated user rules and memory toggle into query processing pipeline |
| api/app_factory.py | Fixed unused parameter warning in serve_react_app |
| api/agents/analysis_agent.py | Major prompt refactor with priority hierarchy, safety rules, and user rules integration |
Comments suppressed due to low confidence (3)
app/src/pages/Settings.tsx:1
- Multiple console.log statements should be replaced with proper logging or removed for production. These debug logs will clutter browser console and may expose sensitive information in production environments.
import { useState, useEffect, useRef } from "react";
app/src/pages/Settings.tsx:1
- Multiple console.log statements should be replaced with proper logging or removed for production. These debug logs will clutter browser console and may expose sensitive information in production environments.
import { useState, useEffect, useRef } from "react";
app/src/pages/Settings.tsx:1
- Multiple console.log statements should be replaced with proper logging or removed for production. These debug logs will clutter browser console and may expose sensitive information in production environments.
import { useState, useEffect, useRef } from "react";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/core/text2sql.py (1)
723-725: Memory toggle not respected in destructive operation flow.The
execute_destructive_operationfunction unconditionally creates aMemoryTooland saves queries to memory (lines 842-877), but theConfirmRequestmodel doesn't include theuse_memoryflag. This means destructive operations will always save to memory even when the user has disabled the memory feature.Consider adding
use_memory: bool = TruetoConfirmRequestand guarding the memory operations accordingly.Proposed fix for ConfirmRequest
class ConfirmRequest(BaseModel): """Confirmation request model. Args: BaseModel (_type_): _description_ """ sql_query: str confirmation: str = "" chat: list = [] + use_memory: bool = True
🤖 Fix all issues with AI agents
In @api/agents/analysis_agent.py:
- Around line 375-387: The long Evaluation Guidelines block should be extracted
into a module-level constant (e.g., EVALUATION_GUIDELINES) so the f-string in
analysis_agent.py can reference {EVALUATION_GUIDELINES} and keep all lines under
100 chars; replace the inlined multi-line text in the prompt with that constant
(update the prompt-building f-string used around the triple-quoted string). Also
mark the prompt construction call to litellm.completion() as a false-positive
S608 (e.g., add an inline comment/pragma such as "# nosec" or disable S608
locally) with a short comment noting this is an LLM prompt (actual SQL runs
later in text2sql.py via PostgresLoader/MySQLLoader).
- Around line 206-222: Remove trailing whitespace from the multi-line f-string
blocks and nearby blank lines that are causing pylint failures: trim any spaces
after the closing triple quotes in the instructions_section and
user_rules_section definitions (created when has_instructions and has_user_rules
are true) and remove trailing spaces from the preceding blank lines (related to
memory_instructions and memory_evaluation_guidelines). Also ensure the resulting
lines do not exceed configured line-length limits (wrap or shorten the f-string
indentation/content if needed) so pylint no longer flags trailing-whitespace or
line-length errors.
- Line 343: The long message string that includes "{memory_instructions}" in
analysis_agent.py (the line starting with "Prefer the minimum necessary
tables/joins required...") exceeds the 100-char lint limit; break the string
into multiple shorter parts or move "{memory_instructions}" onto its own
concatenated/templated line (e.g., split the sentence into two string literals
or use an f-string with a separate line for memory_instructions) so no single
source line exceeds 100 characters while preserving the exact message content.
In @api/app_factory.py:
- Line 256: The function serve_react_app currently has an unused parameter
full_path causing pylint failure; fix by either renaming the parameter back to
_full_path in the serve_react_app signature to signal it is intentionally
unused, or keep full_path and add a per-line pylint suppression (e.g., add "#
pylint: disable=unused-argument" on the def line) so the FastAPI catch-all route
signature remains but the linter is satisfied.
In @api/routes/graphs.py:
- Line 246: Remove the sensitive-content log call that prints full user_rules;
specifically delete the logging.info("Retrieved user rules content: %s",
repr(user_rules)) invocation and instead only log non-sensitive metadata (e.g.,
the existing length log or a safe identifier), ensuring any remaining logs
reference user_rules length or ID but never the full content.
- Around line 250-252: The except block that currently catches Exception and
logs via logging.error should be changed to catch specific exception types
relevant to the operation (e.g., ValueError, KeyError, DatabaseError, HTTPError
— list the concrete exceptions your code can raise) and use logging.exception to
record the traceback; replace the generic "except Exception as e:" in the
user-rules retrieval handler with explicit "except <ExceptionType1> as e, except
<ExceptionType2> as e, ..." blocks (or a tuple of types) and call
logging.exception("Error getting user rules") before returning the same
JSONResponse({"error":"Failed to get user rules"}, status_code=500). Ensure you
do not swallow unexpected exceptions so they can surface during linting/tests.
- Around line 270-272: The except Exception block in the update_user_rules route
should be replaced with specific exception handlers and use logging.exception to
include tracebacks; modify the handler (e.g., function update_user_rules) to
catch expected errors like ValueError/ValidationError for input issues and
sqlalchemy.exc.SQLAlchemyError (or your DB error class) for DB failures with
appropriate JSONResponse messages and status codes, and optionally keep a final
broad except Exception as e only as a last-resort fallback that calls
logging.exception("Error updating user rules") before returning the 500
response.
In @app/src/pages/Settings.tsx:
- Around line 283-290: The Sign In Button rendered in the Settings component
currently has no onClick handler; update the JSX for the fallback Button (the
one with className "bg-purple-600 border-purple-500 text-white
hover:bg-purple-700") to call the appropriate sign-in action (e.g., invoke the
existing signIn helper or openSignInModal / router.push('/signin') used
elsewhere in the app). Locate the Button in Settings.tsx (the branch shown when
the user is unauthenticated) and add an onClick prop that calls the app's
sign-in function or navigation method so the button becomes functional.
🧹 Nitpick comments (5)
api/routes/graphs.py (1)
261-261: Consider removing verbose logging of user rules metadata.Line 261 logs the length of user rules content, and line 285 (in the route handler body) also logs this. While less sensitive than the content itself, excessive logging of user rules operations at INFO level may create noise in production logs.
Suggested refinement
Consider downgrading to DEBUG level or removing redundant length logging:
- logging.info("User rules content length: %d", len(data.user_rules)) + logging.debug("User rules content length: %d", len(data.user_rules))app/src/services/database.ts (2)
265-269: Improve error handling to avoid silently swallowing errors.The current implementation throws an error on line 269 which is immediately caught by the catch block on line 274, returning an empty string. This hides legitimate errors (network issues, server errors, etc.) from the caller.
♻️ Recommended approach
Consider differentiating between expected failures (like auth errors) and unexpected failures:
if (!response.ok) { if (response.status === 401 || response.status === 403) { + console.log('Not authenticated - user rules unavailable'); return ''; } - throw new Error('Failed to fetch user rules'); + const errorText = await response.text().catch(() => 'Unknown error'); + console.error('Failed to fetch user rules:', response.status, errorText); + return ''; // Return empty on error, or throw to propagate }Alternatively, if callers should handle errors, let the exception propagate by removing the try-catch or re-throwing after logging.
285-285: Remove console.log statements from production code.Multiple
console.logstatements are present in theupdateUserRulesmethod (lines 285, 287, 298, 307). These are appropriate for debugging but should be removed or replaced with a proper logging mechanism before merging to production.🧹 Cleanup suggestion
Remove debug logging:
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); + await response.json(); // Consume response body } catch (error) { console.error('Error updating user rules:', error); throw error; } }Keep only the
console.errorfor actual errors.Based on coding guidelines: Frontend source changes in app/**/*.{ts,tsx} must be compiled before production runs
Also applies to: 287-287, 298-298, 307-307
app/src/components/modals/SettingsModal.tsx (1)
53-67: Consider adding a character limit for consistency with the Settings page.The Settings page (
app/src/pages/Settings.tsx) enforces a 5000 character limit on the rules textarea withmaxLength={5000}. For consistency, consider adding the same limit here.Proposed fix
<Textarea id="rules" placeholder={`Example rules: - Always use ISO date format (YYYY-MM-DD) - Limit results to 100 rows unless specified - Always include ORDER BY for consistent results - Use LEFT JOIN instead of INNER JOIN for optional relationships - Add comments to complex queries`} value={rules} onChange={(e) => setRules(e.target.value)} + maxLength={5000} className="min-h-[300px] bg-gray-800 border-gray-700 text-white placeholder:text-gray-500 focus:border-purple-500 focus:ring-purple-500" />api/agents/analysis_agent.py (1)
171-176: Consider extracting section builders to reduce local variables.The pipeline reports
too-many-locals (R0914)for_build_prompt. While the current structure is understandable given the complexity, you could extract the section building logic into helper methods (e.g.,_build_instructions_section,_build_memory_section) to reduce the local variable count if you want to satisfy pylint without disabling the rule.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
api/agents/analysis_agent.pyapi/app_factory.pyapi/core/text2sql.pyapi/graph.pyapi/routes/graphs.pyapp/src/App.tsxapp/src/components/chat/ChatInterface.tsxapp/src/components/layout/Sidebar.tsxapp/src/components/modals/SettingsModal.tsxapp/src/components/ui/switch.tsxapp/src/pages/Index.tsxapp/src/pages/Settings.tsxapp/src/services/chat.tsapp/src/services/database.tsapp/src/types/api.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/components/chat/ChatInterface.tsxapp/src/types/api.tsapp/src/pages/Settings.tsxapp/src/components/ui/switch.tsxapp/src/pages/Index.tsxapp/src/components/modals/SettingsModal.tsxapp/src/App.tsxapp/src/components/layout/Sidebar.tsxapp/src/services/chat.tsapp/src/services/database.ts
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All Python code should pass pylint (use make lint)
Files:
api/graph.pyapi/core/text2sql.pyapi/app_factory.pyapi/agents/analysis_agent.pyapi/routes/graphs.py
🧬 Code graph analysis (3)
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/services/database.ts (1)
app/src/config/api.ts (2)
buildApiUrl(40-42)API_CONFIG(2-37)
🪛 GitHub Actions: Pylint
api/app_factory.py
[error] 256-256: Unused argument 'full_path' (unused-argument)
api/agents/analysis_agent.py
[error] 208-208: Trailing whitespace (trailing-whitespace)
[error] 215-215: Trailing whitespace (trailing-whitespace)
[error] 222-222: Trailing whitespace (trailing-whitespace)
[error] 343-343: Line too long (172/100) (line-too-long)
[error] 366-366: Line too long (106/100) (line-too-long)
[error] 367-367: Line too long (107/100) (line-too-long)
[error] 377-377: Line too long (127/100) (line-too-long)
[error] 378-378: Line too long (133/100) (line-too-long)
[error] 379-379: Line too long (192/100) (line-too-long)
[error] 380-380: Line too long (150/100) (line-too-long)
[error] 381-381: Line too long (136/100) (line-too-long)
[error] 382-382: Line too long (110/100) (line-too-long)
[error] 383-383: Line too long (124/100) (line-too-long)
[error] 384-384: Line too long (222/100) (line-too-long)
[error] 384-384: Line too long (222/100) (line-too-long)
[error] 171-171: Too many local variables (too-many-locals) (R0914)
api/routes/graphs.py
[error] 250-250: Catching too general exception (broad-exception-caught) (W0718)
[error] 270-270: Catching too general exception (broad-exception-caught) (W0718)
🪛 GitHub Check: CodeQL
api/routes/graphs.py
[failure] 243-243: Log Injection
This log entry depends on a user-provided value.
[failure] 243-243: Log Injection
This log entry depends on a user-provided value.
[failure] 260-260: Log Injection
This log entry depends on a user-provided value.
[failure] 263-263: Log Injection
This log entry depends on a user-provided value.
[failure] 265-265: Log Injection
This log entry depends on a user-provided value.
[failure] 268-268: Log Injection
This log entry depends on a user-provided value.
🪛 Ruff (0.14.10)
api/app_factory.py
256-256: Unused function argument: full_path
(ARG001)
api/agents/analysis_agent.py
245-387: Possible SQL injection vector through string-based query construction
(S608)
api/routes/graphs.py
250-250: Do not catch blind exception: Exception
(BLE001)
251-251: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
268-268: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
270-270: Do not catch blind exception: Exception
(BLE001)
271-271: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (18)
api/graph.py (2)
57-70: LGTM! Clean implementation of user rules retrieval.The function follows the established pattern in the file (similar to
get_db_description) and handles missing data appropriately by returning an empty string. The defensive check on line 67 properly handles both missing result sets and null values.
73-82: LGTM! Proper upsert implementation.The function uses
MERGEappropriately for the upsert pattern and parameterizes the query to prevent injection. The implementation is consistent with the file's patterns.app/src/components/ui/switch.tsx (1)
1-27: LGTM! Well-structured accessible Switch component.This is a clean implementation wrapping Radix UI Switch primitives. The component properly:
- Forwards refs and props
- Merges classNames for customization
- Includes accessibility features (focus rings, disabled states)
- Uses Radix's data attributes for state-based styling
- Sets displayName for debugging
The implementation follows React best practices and accessibility guidelines.
app/src/App.tsx (1)
8-8: LGTM! Clean route addition for Settings page.The Settings route is properly imported and registered following the existing pattern. The route is correctly placed before the catch-all
"*"route to ensure proper matching.Also applies to: 22-22
app/src/services/chat.ts (1)
40-45: LGTM! Proper handling of optional boolean flags.The conditional spreads correctly include
use_user_rulesanduse_memoryonly when explicitly provided (not undefined). This pattern ensures that:
- Undefined values are omitted from the request body
- Explicit
falsevalues are included (intentional behavior)- The backend can distinguish between "not specified" and "explicitly disabled"
The implementation aligns with the updated
ChatRequestinterface and maintains backward compatibility.app/src/components/chat/ChatInterface.tsx (3)
43-44: LGTM! Well-designed props with sensible defaults.The new props
useMemoryanduseRulesFromDatabaseare properly typed, documented with clear inline comments, and default totrueto preserve existing behavior. This provides backward compatibility while enabling the new toggle functionality.Also applies to: 47-53
179-180: LGTM! Proper mapping of UI flags to backend request.The props are correctly passed to the
streamQuerycall, with clear naming that maps UI concerns (useRulesFromDatabase) to backend API fields (use_user_rules). The inline comment clarifies the backend behavior.
357-357: ConfirmRequest interface does not includeuse_memoryfieldThe
ConfirmRequestinterface inapp/src/types/api.tsonly includessql_query,confirmation,chat, anduse_user_rules— there is nouse_memoryfield. The omission ofuse_memoryin thestreamConfirmOperationcall is not a bug; it reflects the actual interface definition which does not support memory toggling for confirmation operations.Likely an incorrect or invalid review comment.
app/src/types/api.ts (1)
40-41: LGTM! Clean API extension.The optional boolean flags
use_user_rulesanduse_memoryare well-defined and provide clear control over backend behavior. The comments clearly document their purpose.Also applies to: 92-92
app/src/pages/Index.tsx (1)
36-51: LGTM! Solid state management with localStorage persistence.The initialization pattern correctly handles SSR scenarios with
typeof window !== 'undefined'checks and provides sensible defaults. The state hooks are properly integrated with the ChatInterface component.app/src/components/layout/Sidebar.tsx (1)
92-106: LGTM! Clean navigation implementation.The Settings navigation is properly implemented with React Router hooks. The toggle logic correctly handles routing between the home page and settings page, and the active state is accurately derived from
location.pathname.app/src/components/modals/SettingsModal.tsx (1)
21-36: LGTM!The state synchronization pattern is correct. The
useState(initialRules)captures the initial value, and theuseEffectensures the local state stays in sync when theinitialRulesprop changes from the parent.api/core/text2sql.py (2)
239-242: LGTM!Good implementation of the conditional memory tool creation. This properly gates the async task based on the
use_memoryflag, avoiding unnecessary work when memory is disabled.
261-262: LGTM!The conditional fetching of user rules and propagation to the analysis agent is well implemented. The toggle prevents unnecessary database calls when the feature is disabled.
Also applies to: 324-327
app/src/pages/Settings.tsx (3)
124-142: Async save on unmount may not complete.The unmount cleanup calls
databaseService.updateUserRules()asynchronously, but React doesn't wait for promises in cleanup functions. If the user navigates away quickly, the save might not complete. ThehandleBackClickalready handles saving synchronously before navigation (line 184), but direct browser navigation or route changes from other sources might still lose data.Consider using
navigator.sendBeacon()orvisibilitychangeevent for more reliable persistence, though the current approach withhandleBackClickcovering the primary navigation path is reasonable for most cases.
55-66: LGTM!The GitHub stars fetch is non-critical and gracefully falls back to '-' on error. The implementation is acceptable for this use case.
69-112: LGTM!The rules loading effect properly handles the various states: no graph selected, toggle disabled, and successful/failed fetch. The use of refs for change detection is appropriate for the auto-save-on-unmount pattern.
api/agents/analysis_agent.py (1)
257-273: Well-designed safety rules for prompt injection defense.The IMMUTABLE SAFETY RULES (S1-S5) provide good guardrails:
- S4 restricts
user_rules_specto domain-only rules- S5 explicitly handles malicious injection attempts in user rules
This is a thoughtful approach to LLM prompt security, documenting potential attack vectors and instructing the model to ignore them.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Naseem77
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- when switching between the settings and the chat, it reset the chat.
- we need to add save changes or cancel when making changes inside the settings.
- 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.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.