Fix issues, optimize data structures, and expand features #356
Conversation
- Frontend: Added `/verify/:id` route to `frontend/src/App.jsx` to fix broken navigation. - Backend: - `backend/routers/issues.py`: Fixed `verify_issue_endpoint` to use atomic upvote increments and ensure consistent response types. Added caching to `get_nearby_issues` for performance optimization. - `backend/models.py`: Changed `Issue.description` to `Text` type and added `ForeignKey` to `Grievance.issue_id` for data integrity. - `backend/unified_detection_service.py`: Added `detect_fire` method to `UnifiedDetectionService` to expand features within the unified pipeline. - `backend/cache.py`: Added `nearby_issues_cache` instance. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
|
👋 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 failed. Why did it fail? →
|
🙏 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. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a nearby issues cache, enforces a foreign key and expands Issue.description, integrates fire detection into unified detection (new detect_fire + inclusion in detect_all), and adds several frontend components/routes and small API/auth/client changes. Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as detect_all()
participant UDS as UnifiedDetectionService
participant Backend as DetectionBackend
Client->>API: POST /detect (image)
API->>UDS: detect_fire(image)
par other detections
API->>UDS: detect_objects(image)
API->>UDS: detect_segmentation(image)
end
UDS->>Backend: _get_detection_backend()
Backend-->>UDS: backend (HF / local) selected
UDS->>Backend: detect_fire_clip(image)
Backend-->>UDS: raw_results
UDS->>UDS: normalize raw_results -> List[Dict] (or [])
UDS-->>API: fire detections (List[Dict])
API->>API: aggregate results (include "fire")
API-->>Client: detection_results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
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
🧪 Generate unit tests (beta)
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 fixes a missing frontend route, improves backend verification/upvote handling, expands unified detection capabilities, and adds lightweight caching to reduce load on frequently called spatial queries.
Changes:
- Added
/verify/:idroute to the frontend router to fix navigation to verification flow. - Added
detect_firesupport toUnifiedDetectionServiceand included it indetect_all. - Introduced a short-TTL
nearby_issues_cachefor/api/issues/nearby, and updated DB models for longer issue descriptions and a grievance→issue FK constraint.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/App.jsx | Adds the missing verify route to restore navigation from Home. |
| backend/unified_detection_service.py | Adds fire detection and includes it in the unified detect_all pipeline. |
| backend/routers/issues.py | Adds caching for nearby-issues endpoint and switches verify voting to an atomic DB update. |
| backend/models.py | Uses Text for issue descriptions and adds ForeignKey to Grievance.issue_id. |
| backend/cache.py | Adds a new global nearby_issues_cache instance with TTL/max-size settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/routers/issues.py">
<violation number="1" location="backend/routers/issues.py:286">
P2: Cache check incorrectly treats empty list as cache miss. Use `if cached_data is not None:` instead of `if cached_data:` to properly cache empty results when there are no nearby issues.</violation>
</file>
<file name="backend/unified_detection_service.py">
<violation number="1" location="backend/unified_detection_service.py:248">
P1: The condition `backend == "auto"` is dead code since `_get_detection_backend()` never returns `"auto"` - it only returns `"local"`, `"huggingface"`, or `None`. This means when the service is in AUTO mode with local ML available, fire detection will silently fail instead of falling back to HF API. Consider checking for `backend == "local"` and then explicitly checking HF availability for fire detection, or simplifying to just check HF availability directly since fire detection currently only supports HF.</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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/routers/issues.py (2)
254-259:⚠️ Potential issue | 🔴 CriticalBug: NULL upvotes not handled —
Issue.upvotes + 1evaluates to NULL in SQL.The guard on lines 255-256 is ineffective because line 259 reassigns
issue.upvotesto the SQL expressionIssue.upvotes + 1, which overwrites the Python-level= 0. At the SQL level, ifupvotesisNULL,NULL + 1yieldsNULL.The verify endpoint (Line 412) correctly uses
func.coalesce(Issue.upvotes, 0) + 2. Apply the same pattern here for consistency.Proposed fix
- # Increment upvotes atomically - if issue.upvotes is None: - issue.upvotes = 0 - - # Use SQLAlchemy expression for atomic update - issue.upvotes = Issue.upvotes + 1 + # Use SQLAlchemy expression for atomic update (coalesce handles NULL) + issue.upvotes = func.coalesce(Issue.upvotes, 0) + 1
216-220:⚠️ Potential issue | 🟡 Minor
nearby_issues_cacheis not invalidated when a new issue is created.When a new issue is created,
recent_issues_cache.clear()is called butnearby_issues_cacheis not cleared. Newly created issues won't appear in nearby-issues results for up to 60 seconds. If spatial deduplication accuracy is important (e.g., to prevent duplicate reports right after creation), consider also clearingnearby_issues_cachehere.Proposed fix
try: recent_issues_cache.clear() + nearby_issues_cache.clear() except Exception as e: logger.error(f"Error clearing cache: {e}")
🤖 Fix all issues with AI agents
In `@backend/models.py`:
- Line 109: The nullable foreign key Column issue_id uses
ForeignKey("issues.id") without an ondelete rule; update the ForeignKey to
ForeignKey("issues.id", ondelete="SET NULL") so deleting an Issue will null out
issue_id instead of causing integrity errors, and apply the same change to the
other nullable ForeignKey columns referenced in this file (e.g., the columns at
the other reported locations) to ensure consistent ondelete="SET NULL" behavior
across Grievance-related relationships.
In `@backend/unified_detection_service.py`:
- Around line 246-276: The backend-resolution branch is incorrect and contains
stray developer notes: _get_detection_backend() never returns "auto", so the
"auto" check is dead and when AUTO resolves to "local" we silently return []
even if HF is available. Update the detection logic in the method that calls
await self._get_detection_backend() to (1) remove the stray developer comments,
(2) treat "local" as a fallback but, if await self._check_hf_available() is
true, import backend.hf_api_service.detect_fire_clip and call it before giving
up, (3) normalize detect_fire_clip return values the same way you do now (if
list return it, if dict with "detections" return that, if dict wrap in list,
else []), and (4) keep the existing logger calls (logger.warning/logger.error)
but only return [] after trying HF; reference symbols: _get_detection_backend,
_check_hf_available, detect_fire_clip, and logger.
🧹 Nitpick comments (1)
backend/routers/issues.py (1)
340-340:response_model=Union[VoteResponse, Dict[str, Any]]— consider a dedicated response schema.Using
Union[..., Dict[str, Any]]as a response model effectively disables validation for the AI-verification branch (lines 398-403). A dedicated Pydantic model (e.g.,VerificationResultResponse) would provide schema documentation in OpenAPI and runtime validation of the AI response path.
- Created missing components: `LoadingSpinner`, `AppHeader`, `FloatingButtonsManager`. - Updated `frontend/src/App.jsx` to import these components. - Fixed `frontend/src/api/auth.js` to correctly import `apiClient` and return response directly (matching `client.js` behavior). - Verified build with `npm run build`. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/components/FloatingButtonsManager.jsx">
<violation number="1" location="frontend/src/components/FloatingButtonsManager.jsx:7">
P2: Avoid logging raw voice transcripts in production; it can leak sensitive user input. Gate this log to development-only or remove it.</violation>
</file>
<file name="frontend/src/components/AppHeader.jsx">
<violation number="1" location="frontend/src/components/AppHeader.jsx:8">
P2: The `t` function from `useTranslation()` is destructured but never used. All visible text ("My Reports", "Logout", "Login", "VishwaGuru") is hardcoded in English instead of using `t('key')`. Either use the translation function or remove the unused destructuring if i18n isn't needed here.</violation>
<violation number="2" location="frontend/src/components/AppHeader.jsx:45">
P2: Conflicting CSS classes: `block` and `flex` are both applied to this button. These are mutually exclusive display properties - `flex` will override `block`. Remove `block` since you need `flex` for the icon alignment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…encies - Removed unused `ChatWidget` import from `App.jsx` (handled by `FloatingButtonsManager`). - Removed unused imports in `VoiceInput.jsx` and `ChatWidget.jsx`. - Deleted `frontend/package-lock.json` to ensure clean dependency installation on CI. - Verified build passes locally. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
🔍 Quality Reminder |
- `frontend/src/contexts/AuthContext.jsx`: Fixed "use before define" error for `logout` function by moving its definition before `useEffect`. - `frontend/src/components/VoiceInput.jsx`: Fixed "setState in effect" and "SpeechRecognition not supported" error handling by moving check to state initialization/effect logic safely. - `frontend/src/components/AppHeader.jsx`: Removed unused `t` variable. - `frontend/src/views/Home.jsx`: Removed unused `motion` import. Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="frontend/src/views/Home.jsx">
<violation number="1" location="frontend/src/views/Home.jsx:5">
P1: motion is still used below but is no longer imported, so the scroll-to-top button will crash at runtime. Re-import motion from framer-motion.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/src/components/AppHeader.jsx`:
- Around line 44-46: The logout button's className mixes conflicting display
utilities ("block" and "flex") which prevents consistent layout for items-center
and gap-2; update the button (the element that calls handleLogout in
AppHeader.jsx) to remove "block" and keep "flex w-full text-left px-4 py-2
text-sm text-gray-700 hover:bg-gray-100 items-center gap-2" so the icon and text
align correctly.
In `@frontend/src/components/FloatingButtonsManager.jsx`:
- Around line 6-18: Wrap the handleVoiceCommand function in useCallback to
provide a stable function reference so VoiceInput's onTranscript prop doesn't
change every render and restart SpeechRecognition; specifically, convert the
inline const handleVoiceCommand = (transcript) => { ... } to
useCallback((transcript) => { ... }, [setView]) (or include any other state
setters used) and ensure useCallback is imported from React; this keeps the
handleVoiceCommand reference stable for VoiceInput's useEffect dependency and
prevents tearing down the SpeechRecognition instance.
In `@frontend/src/components/LoadingSpinner.jsx`:
- Around line 17-19: The spinner currently mixes a global border color
(border-transparent) with variantClasses (e.g., border-blue-600) which causes
unpredictable results; edit the LoadingSpinner.jsx return className (the
template string that uses sizeClasses[size] and variantClasses[variant]) to use
uniform border width (e.g., border-2) plus the variant color class, and then
explicitly set a side-specific transparent border (e.g., border-l-transparent)
to create the visible gap; update the class string that builds the element (the
JSX div in the LoadingSpinner component) to remove border-transparent and add
the side-specific transparent class so Tailwind's specificity guarantees the
gap.
In `@frontend/src/views/Home.jsx`:
- Line 5: The runtime crash happens because <motion.button> at line ~465 is
still used but `motion` was removed from the import; restore the named import
from framer-motion by changing the import to include motion (e.g., import {
AnimatePresence, motion } from 'framer-motion') so the <motion.button> reference
resolves (locate the import line and the <motion.button> usage in Home.jsx to
update accordingly).
🧹 Nitpick comments (4)
frontend/src/components/AppHeader.jsx (1)
41-48: Dropdown menu doesn't close on outside click.The user menu stays open until the user clicks "My Reports" or "Logout". Clicking anywhere else on the page leaves it open. Consider adding an outside-click handler or a backdrop overlay to dismiss the menu.
frontend/src/components/VoiceInput.jsx (1)
26-65:onTranscriptin the dependency array will re-create the recognition instance on every parent render if not memoized.If the parent passes an inline callback (e.g.
onTranscript={(t) => ...}), this effect re-runs each render, creating and tearing downSpeechRecognitioninstances. Consider documenting thatonTranscriptshould be stable, or use a ref to hold the latest callback.frontend/src/components/FloatingButtonsManager.jsx (2)
7-7: Removeconsole.logor guard it behind a debug flag.Leaving
console.login production code clutters the browser console and can leak user voice input.
11-17: No user feedback when a voice command is unrecognized.If the transcript doesn't match any keyword, the function silently does nothing. Consider providing feedback (e.g., a toast or brief visual indicator) so users know their command wasn't understood.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@RohanExploit I've opened a new pull request, #359, to work on those changes. Once the pull request is ready, I'll request review from you. |
RohanExploit
temporarily deployed
to
fix-app-issues-optimize-features-13587865994661549961 - vishwaguru-backend PR #356
Render
Destroyed
This PR addresses multiple issues and optimizations requested:
App.jsx, causing navigation failures from the Home screen. Added the route.verify_issue_endpointhad potential race conditions and inconsistent return types. Implemented atomic upvote increments using SQLAlchemy expressions and ensured correct response handling.Issuemodel to useTextfor descriptions (supporting longer text) and added a foreign key constraint toGrievancefor better data integrity.detect_fireinto theUnifiedDetectionService, allowing the app to support fire detection through the unified interface, paving the way for future enhancements without slowing down existing flows.get_nearby_issuesusingThreadSafeCacheto reduce database load for frequent spatial queries.PR created automatically by Jules for task 13587865994661549961 started by @RohanExploit
Summary by cubic
Fixes Verify navigation, hardens vote logic, adds fire detection to the unified pipeline, and reduces DB load with cached nearby-issue queries. Stabilizes auth and voice input, adds missing UI, and fixes the frontend build.
Bug Fixes
Performance
Written for commit 1d58df5. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements