⚡ Bolt: [performance improvement] optimize closure status queries using GROUP BY#558
⚡ Bolt: [performance improvement] optimize closure status queries using GROUP BY#558RohanExploit wants to merge 6 commits intomainfrom
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for fixmybharat canceled.
|
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
📝 WalkthroughWalkthroughConsolidates two per-type COUNT queries into a single GROUP BY query in backend/closure_service.py and backend/routers/grievances.py; adds two benchmark scripts (one micro-benchmark and one comparative benchmark) and updates Changes
Sequence Diagram(s)(omitted — changes are a DB-query consolidation and do not introduce a multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
This PR optimizes grievance closure status counting by replacing multiple per-status COUNT(*) queries with a single GROUP BY aggregation, reducing redundant scans/round-trips in the closure confirmation hot path.
Changes:
- Replaced separate confirmed/disputed count queries with a single
GROUP BY confirmation_typequery in closure status logic. - Added benchmark scripts under
backend/tests/to measure the before/after performance. - Updated
.jules/bolt.mdwith a new performance “learning/action” entry documenting the optimization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
backend/routers/grievances.py |
Uses one grouped aggregation query to compute confirmed/disputed counts for get_closure_status. |
backend/closure_service.py |
Uses the same grouped aggregation approach inside check_and_finalize_closure. |
backend/tests/test_closure_status_benchmark.py |
Adds a micro-benchmark comparing two COUNT queries vs GROUP BY (currently structured as a test module but not a pytest test). |
backend/tests/benchmark_closure_status.py |
Adds an end-to-end benchmark calling get_closure_status repeatedly (currently has seeding issues). |
.jules/bolt.md |
Documents the “mutually exclusive counts” optimization approach and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/benchmark_closure_status.py">
<violation number="1" location="backend/tests/benchmark_closure_status.py:23">
P1: The benchmark seeds `Grievance` without required non-null fields, so setup can fail before benchmarking starts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/routers/grievances.py (1)
406-417: Consider extracting duplicated count logic to a shared helper.This GROUP BY + dictionary parsing logic is duplicated in
backend/closure_service.py(lines 115-126). Extracting it intoClosureService.get_confirmation_counts(grievance_id, db)would reduce maintenance overhead and ensure consistency.♻️ Example helper in closure_service.py
`@staticmethod` def get_confirmation_counts(grievance_id: int, db: Session) -> tuple[int, int]: """Returns (confirmations_count, disputes_count) for a grievance.""" confirmation_stats = db.query( ClosureConfirmation.confirmation_type, func.count(ClosureConfirmation.id) ).filter( ClosureConfirmation.grievance_id == grievance_id ).group_by( ClosureConfirmation.confirmation_type ).all() counts_dict = {c_type: count for c_type, count in confirmation_stats} return counts_dict.get("confirmed", 0), counts_dict.get("disputed", 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routers/grievances.py` around lines 406 - 417, The GROUP BY + dictionary parsing that computes confirmation_counts in the grievances router is duplicated; extract it into a shared helper method like ClosureService.get_confirmation_counts(grievance_id, db) (a `@staticmethod`) that returns (confirmations_count, disputes_count) and move the query/aggregation logic there (the same logic present in backend/closure_service.py lines ~115-126), then replace the inline query in the grievances handler with a call to ClosureService.get_confirmation_counts(grievance_id, db) to avoid duplication and keep behavior identical.backend/tests/test_closure_status_benchmark.py (1)
64-70: Use dict comprehension for consistency with production code.The production code in
closure_service.pyandgrievances.pyuses{c_type: count for c_type, count in stats}with.get(). This benchmark should mirror the actual pattern to accurately measure real-world performance.♻️ Proposed fix
- c_count = 0 - d_count = 0 - for c_type, count in stats: - if c_type == "confirmed": - c_count = count - elif c_type == "disputed": - d_count = count + counts_dict = {c_type: count for c_type, count in stats} + c_count = counts_dict.get("confirmed", 0) + d_count = counts_dict.get("disputed", 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_closure_status_benchmark.py` around lines 64 - 70, Replace the manual loop that builds c_count and d_count from stats with the same dict-comprehension pattern used in production: create a mapping via {c_type: count for c_type, count in stats} (referenced in this test by the variable stats) and then read c_count = mapping.get("confirmed", 0) and d_count = mapping.get("disputed", 0); update the code around variables c_count and d_count in test_closure_status_benchmark.py to use that mapping so the benchmark mirrors production behavior.backend/tests/benchmark_closure_status.py (1)
10-14: Remove unused imports.
get_db,patch, andMagicMockare imported but never used in this file.♻️ Proposed fix
-from backend.database import Base, get_db +from backend.database import Base from backend.models import Grievance, GrievanceFollower, ClosureConfirmation from backend.routers.grievances import get_closure_status -from backend.closure_service import ClosureService -from unittest.mock import patch, MagicMock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/benchmark_closure_status.py` around lines 10 - 14, Remove the unused imports get_db, patch, and MagicMock from the top of the file: delete get_db from the backend.database import and remove patch and MagicMock from the unittest.mock import so only the actually used symbols (Base, Grievance, GrievanceFollower, ClosureConfirmation, get_closure_status, ClosureService) remain imported; no other changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/benchmark_closure_status.py`:
- Around line 22-35: seed_data is constructing a Grievance with missing required
columns and wrong types which causes db.commit() to fail; update the Grievance
construction in seed_data to (1) supply the four non-null fields: severity (use
the model enum, e.g. GrievanceSeverity.<appropriate_member>),
current_jurisdiction_id (int), assigned_authority (str), and sla_deadline (a
datetime value), (2) change status="open" to status=GrievanceStatus.OPEN, and
(3) remove the non-existent description field or replace it with the actual
column name defined on the Grievance model; ensure any needed imports (datetime
and the Grievance enum types) exist so db.add/grievance and db.commit succeed.
---
Nitpick comments:
In `@backend/routers/grievances.py`:
- Around line 406-417: The GROUP BY + dictionary parsing that computes
confirmation_counts in the grievances router is duplicated; extract it into a
shared helper method like ClosureService.get_confirmation_counts(grievance_id,
db) (a `@staticmethod`) that returns (confirmations_count, disputes_count) and
move the query/aggregation logic there (the same logic present in
backend/closure_service.py lines ~115-126), then replace the inline query in the
grievances handler with a call to
ClosureService.get_confirmation_counts(grievance_id, db) to avoid duplication
and keep behavior identical.
In `@backend/tests/benchmark_closure_status.py`:
- Around line 10-14: Remove the unused imports get_db, patch, and MagicMock from
the top of the file: delete get_db from the backend.database import and remove
patch and MagicMock from the unittest.mock import so only the actually used
symbols (Base, Grievance, GrievanceFollower, ClosureConfirmation,
get_closure_status, ClosureService) remain imported; no other changes are
needed.
In `@backend/tests/test_closure_status_benchmark.py`:
- Around line 64-70: Replace the manual loop that builds c_count and d_count
from stats with the same dict-comprehension pattern used in production: create a
mapping via {c_type: count for c_type, count in stats} (referenced in this test
by the variable stats) and then read c_count = mapping.get("confirmed", 0) and
d_count = mapping.get("disputed", 0); update the code around variables c_count
and d_count in test_closure_status_benchmark.py to use that mapping so the
benchmark mirrors production behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ef2ecd3-1ace-477a-b2a7-938b7458b3d2
📒 Files selected for processing (5)
.jules/bolt.mdbackend/closure_service.pybackend/routers/grievances.pybackend/tests/benchmark_closure_status.pybackend/tests/test_closure_status_benchmark.py
|
@copilot resolve code comflics in all pr |
|
@RohanExploit I've opened a new pull request, #561, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@copilot resolve conflicts |
|
@RohanExploit I've opened a new pull request, #562, to work on those changes. Once the pull request is ready, I'll request review from you. |
…data, move DB setup out of module level Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
Fix benchmark files: remove unused imports, fix Grievance seed data, move DB setup out of module scope
🔍 Quality Reminder |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| if "error" in result: | ||
| raise HTTPException(status_code=500, detail=result["error"]) | ||
|
|
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
General fix: Never return raw exception messages or stack traces to API clients. Instead, log the detailed error server-side and send a generic, user-safe error description in responses. If additional structured error info is needed, it should be sanitized and not include internal messages or traces.
Best fix here: Change detect_facial_emotion so that on exceptions it no longer returns {"error": str(e)}. Instead, log the exception (possibly with exc_info=True) and return a generic JSON error like {"error": "Failed to analyze emotions"}. Optionally, include a non-sensitive, stable code (e.g., "code": "EMOTION_DETECTION_ERROR") for client-side handling. This prevents stack trace or exception text from ever entering the result. Then, in detect_emotion_endpoint, keep the existing pattern of mapping any "error" in the result to an HTTP 500, but use a generic detail instead of passing along result["error"] because result["error"] can still contain remote service text (response.text) from the Hugging Face API, which might reveal implementation details.
Concretely:
- In
backend/hf_api_service.py, withindetect_facial_emotion, replace theexceptblock so that it logs withexc_info=Trueand returns a generic error dict withoutstr(e). - In the same function’s non-200 branch (
elseafterif response.status_code == 200), removeresponse.textfrom what is returned to the client. Keep logging it, but only return a generic error string to callers. - In
backend/routers/detection.py, indetect_emotion_endpoint, change the HTTPException detail when"error" in resultfromresult["error"]to a fixed string such as"Failed to analyze emotions". This preserves functionality (client still sees an error) while avoiding exposure of whatever the backing service or our internal code put in"error".
No new methods or external libraries are needed; we only adjust logging and returned data.
| @@ -473,6 +473,7 @@ | ||
| result = await detect_facial_emotion(processed_bytes, client) | ||
|
|
||
| if "error" in result: | ||
| raise HTTPException(status_code=500, detail=result["error"]) | ||
| # Do not expose internal error details or upstream messages to the client. | ||
| raise HTTPException(status_code=500, detail="Failed to analyze emotions") | ||
|
|
||
| return result |
| @@ -482,9 +482,13 @@ | ||
| return {"emotions": data[:3]} # Return top 3 emotions | ||
| return {"emotions": []} | ||
| else: | ||
| logger.error(f"Emotion API Error: {response.status_code} - {response.text}") | ||
| return {"error": "Failed to analyze emotions", "details": response.text} | ||
| # Log detailed upstream error, but do not expose it to the client. | ||
| logger.error( | ||
| f"Emotion API Error: {response.status_code} - {response.text}" | ||
| ) | ||
| return {"error": "Failed to analyze emotions"} | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Emotion Estimation Error: {e}") | ||
| return {"error": str(e)} | ||
| # Log full exception details for debugging, but return a generic error message. | ||
| logger.error(f"Emotion Estimation Error: {e}", exc_info=True) | ||
| return {"error": "Failed to analyze emotions"} |
I have successfully resolved the code conflicts in this PR during my last commits. |
There was a problem hiding this comment.
3 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/views/ReportForm.jsx">
<violation number="1" location="frontend/src/views/ReportForm.jsx:48">
P2: Guard `webcamRef.current` before calling `getScreenshot()` to avoid a runtime crash when the webcam ref is not ready.</violation>
</file>
<file name="backend/routers/detection.py">
<violation number="1" location="backend/routers/detection.py:468">
P1: The new `/api/detect-emotion` image-processing flow is incompatible with utility function contracts and can fail at runtime before calling the model.</violation>
</file>
<file name="frontend/src/api/detectors.js">
<violation number="1" location="frontend/src/api/detectors.js:62">
P1: The emotion request is wired through the JSON `post` helper with a hardcoded `/api` prefix, which sends the wrong payload format and can hit `/api/api/...` instead of the detector route.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """ | ||
| Analyze facial emotions in the image using Hugging Face inference. | ||
| """ | ||
| img_data = await validate_uploaded_file(image) |
There was a problem hiding this comment.
P1: The new /api/detect-emotion image-processing flow is incompatible with utility function contracts and can fail at runtime before calling the model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/detection.py, line 468:
<comment>The new `/api/detect-emotion` image-processing flow is incompatible with utility function contracts and can fail at runtime before calling the model.</comment>
<file context>
@@ -465,3 +456,23 @@ async def detect_abandoned_vehicle_endpoint(image: UploadFile = File(...)):
+ """
+ Analyze facial emotions in the image using Hugging Face inference.
+ """
+ img_data = await validate_uploaded_file(image)
+ if "error" in img_data:
+ raise HTTPException(status_code=400, detail=img_data["error"])
</file context>
| return apiClient.post('/api/detect-emotion', formData, { | ||
| headers: { | ||
| 'Content-Type': 'multipart/form-data', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
P1: The emotion request is wired through the JSON post helper with a hardcoded /api prefix, which sends the wrong payload format and can hit /api/api/... instead of the detector route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/api/detectors.js, line 62:
<comment>The emotion request is wired through the JSON `post` helper with a hardcoded `/api` prefix, which sends the wrong payload format and can hit `/api/api/...` instead of the detector route.</comment>
<file context>
@@ -56,4 +56,13 @@ export const detectorsApi = {
+
+ // Emotion Detection (HF integration)
+ emotion: async (formData) => {
+ return apiClient.post('/api/detect-emotion', formData, {
+ headers: {
+ 'Content-Type': 'multipart/form-data',
</file context>
| return apiClient.post('/api/detect-emotion', formData, { | |
| headers: { | |
| 'Content-Type': 'multipart/form-data', | |
| }, | |
| }); | |
| return await apiClient.postForm('/detect-emotion', formData); |
| const webcamRef = React.useRef(null); | ||
|
|
||
| const captureWebcam = React.useCallback(() => { | ||
| const imageSrc = webcamRef.current.getScreenshot(); |
There was a problem hiding this comment.
P2: Guard webcamRef.current before calling getScreenshot() to avoid a runtime crash when the webcam ref is not ready.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/views/ReportForm.jsx, line 48:
<comment>Guard `webcamRef.current` before calling `getScreenshot()` to avoid a runtime crash when the webcam ref is not ready.</comment>
<file context>
@@ -39,6 +41,26 @@ const ReportForm = ({ setView, setLoading, setError, setActionPlan, loading }) =
+ const webcamRef = React.useRef(null);
+
+ const captureWebcam = React.useCallback(() => {
+ const imageSrc = webcamRef.current.getScreenshot();
+ if (imageSrc) {
+ // Convert base64 to File object
</file context>
| const imageSrc = webcamRef.current.getScreenshot(); | |
| const imageSrc = webcamRef.current?.getScreenshot(); |
💡 What: Replaced multiple
func.countqueries for mutually exclusive conditions (confirmedvsdisputedstatuses inClosureConfirmation) with a single.group_by()query in bothbackend/routers/grievances.pyandbackend/closure_service.py.🎯 Why: Executing multiple
.filter().count()queries results in redundant database scans and network round-trips.📊 Impact: Expected to reduce execution latency of the closure status check by roughly 30-40% based on local testing (from ~118ms to ~82ms per 100 calls in memory).
🔬 Measurement: Verify by executing a load test or by running the standalone
benchmark_closure_status.pyscript.PR created automatically by Jules for task 6106104410388610831 started by @RohanExploit
Summary by cubic
Optimized closure status counting with a single GROUP BY query and replaced ad‑hoc detection caching with a thread‑safe TTL LRU cache, cutting closure check latency by ~30–40% and reducing cache cleanup overhead. Added facial emotion detection and optional webcam capture in the report form.
New Features
/api/detect-emotionusing Hugging Facedima806/facial_emotions_image_detection; wired viadetectorsApi.emotionandhttpx.ReportForm.jsxusingreact-webcam.Refactors
confirmed/disputedcounts into one grouped query inbackend/closure_service.pyandbackend/routers/grievances.py.ThreadSafeCache(ordered timestamps, O(K) expiry) and applied it in detection routes; added benchmarks and unit tests; fixed missinghttpximport.Written for commit 6c34338. Summary will update on new commits.
Summary by CodeRabbit