Release #19
Release #19
Conversation
…arkdown support - Added AuditPage component with form for user queries - Integrated ReactMarkdown for displaying AI insights - Implemented data fetching from the API with error handling - Created AuditPage.module.css for styling the new page - Updated router to include protected route for AuditPage - Added new dependencies: react-markdown and recharts
feat: add AuditPage for analyzing audit logs with visualization and m…
|
🎉 This PR is included in version 2.5.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive Audit Log Analysis feature to the application, enabling users to query and visualize write operation history through natural language questions powered by AI.
Changes:
- Added a new Audit Log page with natural language query interface and data visualization capabilities
- Implemented backend services for SQL generation from natural language and result summarization using Gemini AI
- Created a unified authentication hook to support both MSAL and bypass authentication modes
- Added new dependencies including react-markdown for rendering AI summaries and recharts for data visualization
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/router.tsx | Added /audit route with authentication protection |
| frontend/pages/AuditPage.tsx | New page component for audit log analysis with Q&A interface |
| frontend/pages/AuditPage.module.css | Styling for the audit page |
| frontend/package.json | Added react-markdown and recharts dependencies |
| frontend/hooks/useUnifiedAuth.ts | New hook to unify MSAL and bypass authentication |
| frontend/components/NavBar.tsx | New navigation bar with theme toggle and route links |
| frontend/components/ChartDisplay.tsx | Reusable chart component supporting bar, line, and pie charts |
| backend/services/audit_service.py | Service for executing audit queries and orchestrating the analysis workflow |
| backend/services/gemini_service.py | Extended with SQL generation and result summarization functions |
| backend/routes/audit.py | New API endpoint for audit log queries |
| backend/main.py | Registered audit router |
| backend/tests/test_audit_service.py | Unit tests for audit service |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Basic safety check | ||
| if not sql.lower().startswith("select"): | ||
| return "SELECT 'Error: Generated query was not a SELECT statement' as error;" | ||
| return sql |
There was a problem hiding this comment.
The SQL injection protection relies solely on checking if the query starts with "select". This is insufficient as it doesn't prevent malicious SELECT statements (e.g., SELECT queries that use subqueries to modify data or access unauthorized information). Consider using parameterized queries or a SQL parser to validate the query structure more thoroughly.
| conn.close() | ||
| return results | ||
| except Exception as e: | ||
| print(f"Error executing audit query: {e}") |
There was a problem hiding this comment.
The error handling uses print statements instead of proper logging. This is not suitable for production environments where logs need to be structured, searchable, and routable to appropriate monitoring systems. Replace print statements with proper logging using Python's logging module.
| {(config.data_keys || [config.y_key]).map((key, index) => ( | ||
| key && <Bar key={key} dataKey={key} fill={COLORS[index % COLORS.length]} radius={[4, 4, 0, 0]} /> | ||
| ))} |
There was a problem hiding this comment.
The map operation doesn't filter out nullish keys when using optional chaining with (config.data_keys || [config.y_key]). If config.y_key is undefined/null, the array will contain a falsy value that is then checked with key &&, but this pattern could be clearer. Consider explicitly filtering out undefined/null values before mapping.
| {(config.data_keys || [config.y_key]).map((key, index) => ( | |
| key && <Bar key={key} dataKey={key} fill={COLORS[index % COLORS.length]} radius={[4, 4, 0, 0]} /> | |
| ))} | |
| {(config.data_keys || [config.y_key]) | |
| .filter((key): key is string => key != null) | |
| .map((key, index) => ( | |
| <Bar | |
| key={key} | |
| dataKey={key} | |
| fill={COLORS[index % COLORS.length]} | |
| radius={[4, 4, 0, 0]} | |
| /> | |
| ))} |
| <Link | ||
| to="/query-generator" | ||
| className={`flex items-center gap-2 text-sm font-medium px-3 py-2 rounded-md transition-colors ${isActive('/query-generator') ? 'bg-blue-50 text-blue-700 dark:bg-blue-900/20 dark:text-blue-400' : 'text-slate-600 dark:text-slate-300 hover:bg-slate-100 dark:hover:bg-slate-700'}`} | ||
| > | ||
| <DatabaseIcon className="w-4 h-4" /> | ||
| Query Gen | ||
| </Link> | ||
|
|
||
| <Link | ||
| to="/audit" | ||
| className={`flex items-center gap-2 text-sm font-medium px-3 py-2 rounded-md transition-colors ${isActive('/audit') ? 'bg-blue-50 text-blue-700 dark:bg-blue-900/20 dark:text-blue-400' : 'text-slate-600 dark:text-slate-300 hover:bg-slate-100 dark:hover:bg-slate-700'}`} | ||
| > | ||
| <BookmarkIcon className="w-4 h-4" /> | ||
| Audit Log | ||
| </Link> | ||
|
|
||
| <div className="h-6 w-px bg-slate-300 dark:bg-slate-600 mx-1"></div> | ||
|
|
||
| <button | ||
| onClick={toggleTheme} | ||
| className="p-2 rounded-full text-slate-600 dark:text-slate-300 hover:bg-slate-100 dark:hover:bg-slate-700 transition-colors" | ||
| title="Toggle theme" | ||
| > | ||
| {theme === 'light' ? <MoonIcon className="w-5 h-5" /> : <SunIcon className="w-5 h-5" />} | ||
| </button> | ||
|
|
||
| <div className="flex items-center gap-3 pl-2 border-l border-slate-200 dark:border-slate-700"> | ||
| <div className="text-sm text-slate-600 dark:text-slate-300 hidden md:block"> | ||
| {user?.name || user?.email} | ||
| </div> | ||
| <button | ||
| onClick={logout} | ||
| className="p-2 text-red-600 dark:text-red-400 hover:bg-red-50 dark:hover:bg-red-900/20 rounded-full transition-colors" | ||
| title="Sign Out" | ||
| > | ||
| <SignOutIcon className="w-5 h-5" /> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
The NavBar component is missing accessibility attributes. The theme toggle button and logout button should have aria-label attributes for screen readers, and the navigation links should indicate the current page state using aria-current="page" for the active link.
| return ( | ||
| <div className="bg-white dark:bg-slate-800 rounded-xl shadow-lg border border-slate-200 dark:border-slate-700 overflow-hidden p-6 mb-8"> | ||
| <h3 className="text-lg font-semibold text-slate-800 dark:text-slate-100 mb-6 text-center"> | ||
| {config.title || 'Data Visualization'} | ||
| </h3> | ||
| <div className="h-[400px] w-full"> | ||
| <ResponsiveContainer width="100%" height="100%"> | ||
| {renderChart() || <div>Unable to render chart</div>} | ||
| </ResponsiveContainer> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The chart rendering doesn't handle accessibility concerns. Charts should include alternative text descriptions or data tables for screen readers. Consider adding ARIA labels to the chart components and providing a data table view as an accessible alternative.
| # Truncate results if too large to avoid token limits | ||
| results_str = str(results)[:10000] |
There was a problem hiding this comment.
The results are truncated to 10000 characters which may cut off in the middle of a data structure, potentially causing JSON parsing issues or incomplete data representation. Consider implementing a smarter truncation strategy that respects data boundaries (e.g., limiting the number of result rows rather than string length) or using a sampling approach.
| def generate_audit_sql(user_input: str) -> str: | ||
| full_prompt = PROMPT_TEMPLATE_AUDIT_SQL.format(user_input=user_input) | ||
| client = genai.Client() | ||
| response = client.models.generate_content( | ||
| model="gemini-2.5-flash", | ||
| contents=full_prompt, | ||
| config=types.GenerateContentConfig( | ||
| thinking_config=types.ThinkingConfig(thinking_budget=0) | ||
| ), | ||
| ) | ||
| sql = extract_python_code(response.text) | ||
| # Basic safety check | ||
| if not sql.lower().startswith("select"): | ||
| return "SELECT 'Error: Generated query was not a SELECT statement' as error;" | ||
| return sql | ||
|
|
||
|
|
||
| def summarize_audit_results( | ||
| user_input: str, sql_query: str, results: list | ||
| ) -> AuditSummaryResponse: | ||
| # Truncate results if too large to avoid token limits | ||
| results_str = str(results)[:10000] | ||
| full_prompt = PROMPT_TEMPLATE_AUDIT_SUMMARY.format( | ||
| user_input=user_input, sql_query=sql_query, results=results_str | ||
| ) | ||
| client = genai.Client() | ||
| response = client.models.generate_content( | ||
| model="gemini-2.5-flash", | ||
| contents=full_prompt, | ||
| config=types.GenerateContentConfig( | ||
| response_mime_type="application/json", | ||
| response_schema=AuditSummaryResponse, | ||
| thinking_config=types.ThinkingConfig(thinking_budget=0), | ||
| ), | ||
| ) | ||
|
|
||
| if hasattr(response, "parsed") and response.parsed: | ||
| return response.parsed | ||
|
|
||
| import json | ||
|
|
||
| try: | ||
| data = json.loads(response.text) | ||
| return AuditSummaryResponse(**data) | ||
| except Exception as e: | ||
| print(f"Error parsing Gemini response: {e}") | ||
| return AuditSummaryResponse( | ||
| summary="Could not generate summary due to parsing error.", | ||
| visualization=VisualizationConfig(available=False), | ||
| ) |
There was a problem hiding this comment.
Missing test coverage for the newly added audit functionality. While there is a test file for audit_service.py, there are no tests for the new functions in gemini_service.py (generate_audit_sql, summarize_audit_results) or the audit route. Consider adding comprehensive tests for these components to ensure reliability.
| print(f"Error parsing Gemini response: {e}") | ||
| return AuditSummaryResponse( | ||
| summary="Could not generate summary due to parsing error.", | ||
| visualization=VisualizationConfig(available=False), | ||
| ) |
There was a problem hiding this comment.
The error handling prints to stdout but returns a generic fallback response. This makes debugging difficult in production. Consider using proper logging (with appropriate log levels) instead of print statements, and include more context about the error in the returned response or raise an appropriate exception.
| ), | ||
| ) | ||
| suggestion = ( | ||
| response.text.strip() if hasattr(response, "text") else str(response).strip |
There was a problem hiding this comment.
The expression uses response.text.strip() if hasattr(response, "text") else str(response).strip but is missing parentheses around str(response).strip(). This will call strip on the result of str(response) only if hasattr returns False, but the expression will always result in calling the strip attribute access (not the method) on the string, causing an error. This should be str(response).strip().
| response.text.strip() if hasattr(response, "text") else str(response).strip | |
| response.text.strip() if hasattr(response, "text") else str(response).strip() |
| export const useUnifiedAuth = () => { | ||
| // We cannot conditionally call hooks, so we call both and ignore the one we don't need | ||
| // This assumes that calling useAuth when AuthProvider is missing throws, which is the problem. | ||
| // So we CANNOT call useContextAuth if USE_MSAL_AUTH is true. | ||
|
|
||
| // HOWEVER, hooks must be called unconditionally. | ||
| // The only way to solve this without refactoring the Provider structure is to: | ||
| // 1. Create a SafeAuthProvider that always exists? | ||
| // 2. Or, since we know USE_MSAL_AUTH is a build-time constant (or runtime constant), | ||
| // we technically can branch IF the hook implementation allows it, but React forbids it. | ||
|
|
||
| // BUT, since USE_MSAL_AUTH is constant, we can create two different hook implementations | ||
| // and export the correct one. | ||
| return USE_MSAL_AUTH ? useMsalAuth() : useBypassAuth(); | ||
| }; |
There was a problem hiding this comment.
The comment states that "calling useContextAuth if USE_MSAL_AUTH is true" is a problem, but the code violates the React Rules of Hooks by conditionally calling hooks. While the comment acknowledges this, the implementation uses a ternary operator in the return statement to conditionally call useMsalAuth() or useBypassAuth(), which internally call different hooks. This is a violation of the Rules of Hooks - hooks must be called in the same order on every render. Consider refactoring to always call both sets of hooks but only use the relevant values, or use a different architectural approach.
No description provided.