Add Public Facilities and Construction Safety Detectors #368
Conversation
…outing updates 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. |
📝 WalkthroughWalkthroughThe PR removes the hard dependency on python-magic by introducing conditional MIME detection that falls back to mimetypes when libmagic is unavailable. python-magic is removed from Render requirements, environment variable validation is softened to warnings instead of halting startup, and Node.js heap size is increased for builds. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@frontend/src/App.jsx`:
- Around line 301-302: The onBack handlers for the new detectors use
navigate('/') which now routes to the Landing page; update the onBack prop for
PublicFacilitiesDetector and ConstructionSafetyDetector to use navigate('/home')
instead of navigate('/') so the Back button goes to the Home dashboard, and
search for other components that pass onBack={() => navigate('/')} (e.g.,
existing detector routes) and change them similarly to navigate('/home').
In `@frontend/src/ConstructionSafetyDetector.jsx`:
- Around line 13-16: The cleanup in the useEffect captures a stale stopCamera
that reads the initial stream (null), so unmount never stops tracks; update the
component to keep the live MediaStream in a ref (e.g., streamRef) that
startCamera sets and stopCamera reads from, or inline the cleanup in the effect
to close over the latest stream variable; specifically modify startCamera to
assign the obtained stream to streamRef.current (and set state if needed) and
change stopCamera (or the effect cleanup) to call
streamRef.current.getTracks().forEach(t => t.stop()) so the unmount always stops
the actual camera stream.
- Around line 54-69: The toBlob callback can return a null blob and currently
returns early, leaving the analyzing state true; in the
ConstructionSafetyDetector component update the canvas.toBlob callback to handle
the null case by calling setAnalyzing(false) (and optionally setError("Analysis
failed. Please try again.")) before returning so the overlay is dismissed;
ensure this fix references the existing setAnalyzing and setError calls used
around the canvas.toBlob callback and keeps the rest of the try/catch flow that
calls detectorsApi.constructionSafety unchanged.
In `@frontend/src/PublicFacilitiesDetector.jsx`:
- Around line 54-69: The canvas.toBlob callback currently returns early if blob
is null, which skips cleaning up state and leaves analyzing stuck; in the
canvas.toBlob callback for PublicFacilitiesDetector.jsx ensure that when blob is
falsy you call setAnalyzing(false) (and optionally setError with a user-friendly
message) before returning so the loading overlay is cleared—mirror the same
null-blob handling implemented in ConstructionSafetyDetector.jsx and update the
callback around canvas.toBlob(...) to always clear analyzing state even on early
exits.
- Around line 13-16: The cleanup in the useEffect currently captures the initial
stopCamera reference (when stream is null) so tracks never stop; update the
effect to avoid the stale closure by either adding stream to the dependency list
and returning a cleanup that calls stopCamera when stream is present, or
(preferred) use a mutable ref for the media stream (e.g., streamRef) that
startCamera writes to and change the cleanup to directly stop tracks from
streamRef.current (or from videoRef.current.srcObject) so the latest stream is
always used; update references to useEffect, startCamera, stopCamera, stream (or
streamRef/videoRef) accordingly.
In `@frontend/src/views/Home.jsx`:
- Line 88: Replace the hardcoded English labels for the menu/items with i18n
translation keys: instead of "Construction Safety" and "Public Facilities" use
the translation function (t) with new keys matching the existing pattern (e.g.
t('home.issues.constructionSafety') and t('home.issues.publicFacilities'));
update the translation resource files accordingly and ensure the component uses
the same useTranslation/t function already used by adjacent items so the objects
with id 'construction-safety' and 'public-facilities' render localized labels.
In `@verification/verify_new_detectors.py`:
- Around line 40-55: The verification currently only prints failures for
elements like the "Public Facilities" button (pub_btn) and header checks but
never signals failure to the caller; introduce a boolean success flag (e.g.,
success = True) at the start of the script, set success = False wherever a check
fails (e.g., when pub_btn.count() == 0 or when page.get_by_text("Public
Facilities Check").is_visible() is False, and similarly for "Construction
Safety" checks), and after all checks call sys.exit(1) if not success (else
sys.exit(0)) so verify.sh sees a non-zero exit on any failed detection; update
any screenshot branches to also set success = False when capturing failures.
- Around line 80-82: The except block currently calls
page.screenshot(path="verification/error.png") which can itself raise if page
isn't initialized; wrap the screenshot call in a nested try/except inside the
existing except to avoid masking the original exception: inside the outer except
Exception as e, first log or print the original exception, then attempt
page.screenshot(...) inside a try block, and in that inner except catch and log
any screenshot-specific error (including that it failed) without re-raising so
the original error remains visible; reference the existing exception variable e
and the page.screenshot call to locate where to add this nested handling.
In `@verify.sh`:
- Around line 1-14: The script starts backend and frontend into background PIDs
(BACKEND_PID, FRONTEND_PID) but doesn't guarantee cleanup if verification
(verification/verify_new_detectors.py) fails or the script is interrupted; add a
trap that catches EXIT, SIGINT, and SIGTERM and kills the entire process groups
for BACKEND_PID and FRONTEND_PID (use kill -- -$BACKEND_PID and kill --
-$FRONTEND_PID or equivalent) to ensure child processes are terminated, and
ensure the trap runs before launching servers so the cleanup always occurs even
on early failure.
🧹 Nitpick comments (3)
frontend/src/PublicFacilitiesDetector.jsx (1)
1-179: Heavy duplication withConstructionSafetyDetector.jsx.These two components share ~90% of their code (camera init/teardown, canvas capture, toBlob submission, ScoreCard, layout). Consider extracting a shared
DetectorBasecomponent or custom hook (useCameraCapture) that both detectors compose with their specific config (API endpoint, labels, result field mappings, colors).This would reduce the maintenance burden and ensure bug fixes (like the stale closure and null-blob issues above) only need to be applied once.
backend/hf_api_service.py (1)
461-491: Extract the duplicatedget_top_categoryhelper to module level.The
get_top_categoryinner function is now copy-pasted identically in three places:detect_civic_eye_clip(Line 420),detect_public_facilities_clip(Line 477), anddetect_construction_safety_clip(Line 509). Extract it once at module level and reuse it.♻️ Proposed refactor
Add a module-level helper (e.g., near
_detect_clip_generic):def _get_top_category(res_list: list, category_labels: list) -> dict: """Return the highest-ranked result whose label is in category_labels.""" relevant = [r for r in res_list if r.get('label') in category_labels] if relevant: return relevant[0] return {"label": "unknown", "score": 0}Then replace each inline
get_top_categorycall with_get_top_category.- def get_top_category(res_list, category_labels): - relevant = [r for r in res_list if r.get('label') in category_labels] - if relevant: - return relevant[0] - return {"label": "unknown", "score": 0} - - amenity = get_top_category(results, amenity_labels) - cleanliness = get_top_category(results, cleanliness_labels) - safety = get_top_category(results, safety_labels) + amenity = _get_top_category(results, amenity_labels) + cleanliness = _get_top_category(results, cleanliness_labels) + safety = _get_top_category(results, safety_labels)Apply the same to
detect_construction_safety_clipanddetect_civic_eye_clip.backend/routers/detection.py (1)
451-472: Consider chaining the original exception withraise ... from e.The
raise HTTPException(...)inside theexceptblocks (Lines 460, 472) discards the original traceback chain. While the error is already logged withexc_info=True, addingfrom eis a minor improvement for debuggability and satisfies B904.This is a pre-existing pattern across the file, so feel free to defer.
♻️ Suggested diff
except Exception as e: logger.error(f"Public facilities detection error: {e}", exc_info=True) - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from eexcept Exception as e: logger.error(f"Construction safety detection error: {e}", exc_info=True) - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from e
frontend/src/App.jsx
Outdated
| <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} /> | ||
| <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/')} />} /> |
There was a problem hiding this comment.
Back button navigates to Landing page (/) instead of Home (/home).
Since this PR moved the Home route from / to /home, onBack={() => navigate('/')} sends users to the Landing page instead of the dashboard. This applies to both new detectors here.
Proposed fix
- <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} />
- <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/')} />} />
+ <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/home')} />} />
+ <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/home')} />} />Note: The same issue exists for all pre-existing detectors on Lines 255–300 (navigate('/') should also be navigate('/home')). Consider updating those as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} /> | |
| <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/')} />} /> | |
| <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/home')} />} /> | |
| <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/home')} />} /> |
🤖 Prompt for AI Agents
In `@frontend/src/App.jsx` around lines 301 - 302, The onBack handlers for the new
detectors use navigate('/') which now routes to the Landing page; update the
onBack prop for PublicFacilitiesDetector and ConstructionSafetyDetector to use
navigate('/home') instead of navigate('/') so the Back button goes to the Home
dashboard, and search for other components that pass onBack={() =>
navigate('/')} (e.g., existing detector routes) and change them similarly to
navigate('/home').
| useEffect(() => { | ||
| startCamera(); | ||
| return () => stopCamera(); | ||
| }, []); |
There was a problem hiding this comment.
Stale closure: stopCamera won't stop the stream on unmount.
The useEffect on mount captures stopCamera, which reads stream from state. At mount time, stream is null. When the component unmounts, the cleanup calls the stale stopCamera that still sees stream as null, so no tracks are ever stopped — causing a camera resource leak.
Use a ref to track the stream, or move the cleanup logic into the effect itself.
Proposed fix using a ref
+ const streamRef = useRef(null);
+
useEffect(() => {
startCamera();
- return () => stopCamera();
+ return () => {
+ if (streamRef.current) {
+ streamRef.current.getTracks().forEach(track => track.stop());
+ }
+ };
}, []);
const startCamera = async () => {
setError(null);
try {
const mediaStream = await navigator.mediaDevices.getUserMedia({
video: { facingMode: 'environment' }
});
+ streamRef.current = mediaStream;
setStream(mediaStream);
if (videoRef.current) {
videoRef.current.srcObject = mediaStream;
}
} catch (err) {
setError("Camera access failed: " + err.message);
}
};Also applies to: 33-38
🤖 Prompt for AI Agents
In `@frontend/src/ConstructionSafetyDetector.jsx` around lines 13 - 16, The
cleanup in the useEffect captures a stale stopCamera that reads the initial
stream (null), so unmount never stops tracks; update the component to keep the
live MediaStream in a ref (e.g., streamRef) that startCamera sets and stopCamera
reads from, or inline the cleanup in the effect to close over the latest stream
variable; specifically modify startCamera to assign the obtained stream to
streamRef.current (and set state if needed) and change stopCamera (or the effect
cleanup) to call streamRef.current.getTracks().forEach(t => t.stop()) so the
unmount always stops the actual camera stream.
| canvas.toBlob(async (blob) => { | ||
| if (!blob) return; | ||
| const formData = new FormData(); | ||
| formData.append('image', blob, 'construction_safety.jpg'); | ||
|
|
||
| try { | ||
| const data = await detectorsApi.constructionSafety(formData); | ||
| if (data.error) throw new Error(data.error); | ||
| setResult(data); | ||
| } catch (err) { | ||
| console.error(err); | ||
| setError("Analysis failed. Please try again."); | ||
| } finally { | ||
| setAnalyzing(false); | ||
| } | ||
| }, 'image/jpeg', 0.8); |
There was a problem hiding this comment.
analyzing stays true forever if toBlob produces a null blob.
On Line 55, if blob is null, the function returns without ever calling setAnalyzing(false). The user is stuck on the "Checking compliance..." overlay with no way out.
Proposed fix
canvas.toBlob(async (blob) => {
- if (!blob) return;
+ if (!blob) {
+ setError("Failed to capture image. Please try again.");
+ setAnalyzing(false);
+ return;
+ }
const formData = new FormData();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| canvas.toBlob(async (blob) => { | |
| if (!blob) return; | |
| const formData = new FormData(); | |
| formData.append('image', blob, 'construction_safety.jpg'); | |
| try { | |
| const data = await detectorsApi.constructionSafety(formData); | |
| if (data.error) throw new Error(data.error); | |
| setResult(data); | |
| } catch (err) { | |
| console.error(err); | |
| setError("Analysis failed. Please try again."); | |
| } finally { | |
| setAnalyzing(false); | |
| } | |
| }, 'image/jpeg', 0.8); | |
| canvas.toBlob(async (blob) => { | |
| if (!blob) { | |
| setError("Failed to capture image. Please try again."); | |
| setAnalyzing(false); | |
| return; | |
| } | |
| const formData = new FormData(); | |
| formData.append('image', blob, 'construction_safety.jpg'); | |
| try { | |
| const data = await detectorsApi.constructionSafety(formData); | |
| if (data.error) throw new Error(data.error); | |
| setResult(data); | |
| } catch (err) { | |
| console.error(err); | |
| setError("Analysis failed. Please try again."); | |
| } finally { | |
| setAnalyzing(false); | |
| } | |
| }, 'image/jpeg', 0.8); |
🤖 Prompt for AI Agents
In `@frontend/src/ConstructionSafetyDetector.jsx` around lines 54 - 69, The toBlob
callback can return a null blob and currently returns early, leaving the
analyzing state true; in the ConstructionSafetyDetector component update the
canvas.toBlob callback to handle the null case by calling setAnalyzing(false)
(and optionally setError("Analysis failed. Please try again.")) before returning
so the overlay is dismissed; ensure this fix references the existing
setAnalyzing and setError calls used around the canvas.toBlob callback and keeps
the rest of the try/catch flow that calls detectorsApi.constructionSafety
unchanged.
| useEffect(() => { | ||
| startCamera(); | ||
| return () => stopCamera(); | ||
| }, []); |
There was a problem hiding this comment.
Same stale closure bug as ConstructionSafetyDetector: camera stream won't stop on unmount.
See the identical issue and fix described in ConstructionSafetyDetector.jsx. The cleanup function captures stopCamera at mount time when stream is still null, so tracks are never stopped.
Also applies to: 33-38
🤖 Prompt for AI Agents
In `@frontend/src/PublicFacilitiesDetector.jsx` around lines 13 - 16, The cleanup
in the useEffect currently captures the initial stopCamera reference (when
stream is null) so tracks never stop; update the effect to avoid the stale
closure by either adding stream to the dependency list and returning a cleanup
that calls stopCamera when stream is present, or (preferred) use a mutable ref
for the media stream (e.g., streamRef) that startCamera writes to and change the
cleanup to directly stop tracks from streamRef.current (or from
videoRef.current.srcObject) so the latest stream is always used; update
references to useEffect, startCamera, stopCamera, stream (or streamRef/videoRef)
accordingly.
| canvas.toBlob(async (blob) => { | ||
| if (!blob) return; | ||
| const formData = new FormData(); | ||
| formData.append('image', blob, 'public_facilities.jpg'); | ||
|
|
||
| try { | ||
| const data = await detectorsApi.publicFacilities(formData); | ||
| if (data.error) throw new Error(data.error); | ||
| setResult(data); | ||
| } catch (err) { | ||
| console.error(err); | ||
| setError("Analysis failed. Please try again."); | ||
| } finally { | ||
| setAnalyzing(false); | ||
| } | ||
| }, 'image/jpeg', 0.8); |
There was a problem hiding this comment.
Same analyzing stuck-forever bug when toBlob returns null.
If blob is null on Line 55, the early return skips setAnalyzing(false), leaving the user stuck on the loading overlay. See the identical fix proposed in ConstructionSafetyDetector.jsx.
🤖 Prompt for AI Agents
In `@frontend/src/PublicFacilitiesDetector.jsx` around lines 54 - 69, The
canvas.toBlob callback currently returns early if blob is null, which skips
cleaning up state and leaves analyzing stuck; in the canvas.toBlob callback for
PublicFacilitiesDetector.jsx ensure that when blob is falsy you call
setAnalyzing(false) (and optionally setError with a user-friendly message)
before returning so the loading overlay is cleared—mirror the same null-blob
handling implemented in ConstructionSafetyDetector.jsx and update the callback
around canvas.toBlob(...) to always clear analyzing state even on early exits.
frontend/src/views/Home.jsx
Outdated
| { id: 'streetlight', label: t('home.issues.darkStreet'), icon: <Lightbulb size={24} />, color: 'text-slate-600', bg: 'bg-slate-50' }, | ||
| { id: 'traffic-sign', label: t('home.issues.trafficSign'), icon: <Signpost size={24} />, color: 'text-yellow-600', bg: 'bg-yellow-50' }, | ||
| { id: 'abandoned-vehicle', label: t('home.issues.abandonedVehicle'), icon: <Car size={24} />, color: 'text-gray-600', bg: 'bg-gray-50' }, | ||
| { id: 'construction-safety', label: "Construction Safety", icon: <HardHat size={24} />, color: 'text-orange-600', bg: 'bg-orange-50' }, |
There was a problem hiding this comment.
Hardcoded labels bypass i18n.
Both new items use hardcoded English strings ("Construction Safety", "Public Facilities") while adjacent items use t('home.issues.*'). This breaks localization for non-English users. Consider adding translation keys to match the existing pattern.
Also applies to: 105-105
🤖 Prompt for AI Agents
In `@frontend/src/views/Home.jsx` at line 88, Replace the hardcoded English labels
for the menu/items with i18n translation keys: instead of "Construction Safety"
and "Public Facilities" use the translation function (t) with new keys matching
the existing pattern (e.g. t('home.issues.constructionSafety') and
t('home.issues.publicFacilities')); update the translation resource files
accordingly and ensure the component uses the same useTranslation/t function
already used by adjacent items so the objects with id 'construction-safety' and
'public-facilities' render localized labels.
verification/verify_new_detectors.py
Outdated
| # 2. Check for Public Facilities button | ||
| # It has text "Public Facilities" | ||
| pub_btn = page.get_by_text("Public Facilities", exact=True) | ||
| if pub_btn.count() > 0: | ||
| print("Public Facilities button found") | ||
| pub_btn.first.click() | ||
| page.wait_for_timeout(3000) # Wait for lazy load | ||
| # Check header | ||
| if page.get_by_text("Public Facilities Check").is_visible(): | ||
| print("Public Facilities page loaded") | ||
| page.screenshot(path="verification/public_facilities.png") | ||
| else: | ||
| print("Public Facilities header not found") | ||
| else: | ||
| print("Public Facilities button not found") | ||
| page.screenshot(path="verification/missing_pf_button.png", full_page=True) |
There was a problem hiding this comment.
Verification silently passes when elements are missing.
When "Public Facilities" or "Construction Safety" buttons/headers aren't found, the script just prints a message and continues. It never sets a non-zero exit code, so verify.sh has no way to detect failure.
Consider tracking a success flag and calling sys.exit(1) at the end if any check failed.
🤖 Prompt for AI Agents
In `@verification/verify_new_detectors.py` around lines 40 - 55, The verification
currently only prints failures for elements like the "Public Facilities" button
(pub_btn) and header checks but never signals failure to the caller; introduce a
boolean success flag (e.g., success = True) at the start of the script, set
success = False wherever a check fails (e.g., when pub_btn.count() == 0 or when
page.get_by_text("Public Facilities Check").is_visible() is False, and similarly
for "Construction Safety" checks), and after all checks call sys.exit(1) if not
success (else sys.exit(0)) so verify.sh sees a non-zero exit on any failed
detection; update any screenshot branches to also set success = False when
capturing failures.
verification/verify_new_detectors.py
Outdated
| except Exception as e: | ||
| print(f"Error: {e}") | ||
| page.screenshot(path="verification/error.png") |
There was a problem hiding this comment.
Error screenshot may fail if page is in a bad state.
If the exception occurs before page is fully initialized or during navigation, page.screenshot() in the except block could itself raise, masking the original error. Wrap it in a nested try/except.
Proposed fix
except Exception as e:
print(f"Error: {e}")
- page.screenshot(path="verification/error.png")
+ try:
+ page.screenshot(path="verification/error.png")
+ except Exception:
+ print("Could not capture error screenshot")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| print(f"Error: {e}") | |
| page.screenshot(path="verification/error.png") | |
| except Exception as e: | |
| print(f"Error: {e}") | |
| try: | |
| page.screenshot(path="verification/error.png") | |
| except Exception: | |
| print("Could not capture error screenshot") |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 80-80: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@verification/verify_new_detectors.py` around lines 80 - 82, The except block
currently calls page.screenshot(path="verification/error.png") which can itself
raise if page isn't initialized; wrap the screenshot call in a nested try/except
inside the existing except to avoid masking the original exception: inside the
outer except Exception as e, first log or print the original exception, then
attempt page.screenshot(...) inside a try block, and in that inner except catch
and log any screenshot-specific error (including that it failed) without
re-raising so the original error remains visible; reference the existing
exception variable e and the page.screenshot call to locate where to add this
nested handling.
verify.sh
Outdated
| #!/bin/bash | ||
| python start-backend.py > backend.log 2>&1 & | ||
| BACKEND_PID=$! | ||
| cd frontend && npm run dev -- --port 5173 > frontend.log 2>&1 & | ||
| FRONTEND_PID=$! | ||
| echo "Backend PID: $BACKEND_PID" | ||
| echo "Frontend PID: $FRONTEND_PID" | ||
| echo "Waiting for servers to start..." | ||
| sleep 20 | ||
| echo "Running verification..." | ||
| python verification/verify_new_detectors.py | ||
| echo "Verification complete. Stopping servers..." | ||
| kill $BACKEND_PID | ||
| kill $FRONTEND_PID |
There was a problem hiding this comment.
No cleanup on failure; servers may be left running.
If verify_new_detectors.py crashes or the script is interrupted (Ctrl+C), the backend and frontend processes are never killed. Use a trap to ensure cleanup.
Also, kill may send SIGTERM to a wrapper process rather than the actual server process tree. Consider kill -- -$PID or kill 0 to clean up the process group.
Proposed fix
#!/bin/bash
+set -e
+cleanup() {
+ echo "Stopping servers..."
+ kill $BACKEND_PID $FRONTEND_PID 2>/dev/null || true
+}
+trap cleanup EXIT
+
python start-backend.py > backend.log 2>&1 &
BACKEND_PID=$!
cd frontend && npm run dev -- --port 5173 > frontend.log 2>&1 &
FRONTEND_PID=$!
echo "Backend PID: $BACKEND_PID"
echo "Frontend PID: $FRONTEND_PID"
echo "Waiting for servers to start..."
sleep 20
echo "Running verification..."
python verification/verify_new_detectors.py
echo "Verification complete. Stopping servers..."
-kill $BACKEND_PID
-kill $FRONTEND_PID🤖 Prompt for AI Agents
In `@verify.sh` around lines 1 - 14, The script starts backend and frontend into
background PIDs (BACKEND_PID, FRONTEND_PID) but doesn't guarantee cleanup if
verification (verification/verify_new_detectors.py) fails or the script is
interrupted; add a trap that catches EXIT, SIGINT, and SIGTERM and kills the
entire process groups for BACKEND_PID and FRONTEND_PID (use kill --
-$BACKEND_PID and kill -- -$FRONTEND_PID or equivalent) to ensure child
processes are terminated, and ensure the trap runs before launching servers so
the cleanup always occurs even on early failure.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 9 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="frontend/src/PublicFacilitiesDetector.jsx">
<violation number="1" location="frontend/src/PublicFacilitiesDetector.jsx:8">
P1: Camera stream is never stopped on unmount due to stale closure. The `useEffect` cleanup captures the initial `stopCamera` which closes over `stream = null`. Use a ref to track the stream so cleanup always has access to the current value.</violation>
<violation number="2" location="frontend/src/PublicFacilitiesDetector.jsx:55">
P1: If `canvas.toBlob` produces a `null` blob, the early return skips the `finally` block, leaving `analyzing` stuck as `true` forever. The spinner overlay will never dismiss.</violation>
</file>
<file name="frontend/src/App.jsx">
<violation number="1" location="frontend/src/App.jsx:301">
P2: Bug: `onBack` navigates to `'/'` (Landing page) instead of `'/home'` (Dashboard). Since this PR moved the Dashboard from `/` to `/home`, clicking "Back" from these detectors will take users to the Landing page instead of the Dashboard.
Note: the same issue exists on all the pre-existing detector routes (e.g., `PotholeDetector`, `GarbageDetector`, etc.), which should also be updated to `navigate('/home')` as part of this routing change.</violation>
</file>
<file name="frontend/src/ConstructionSafetyDetector.jsx">
<violation number="1" location="frontend/src/ConstructionSafetyDetector.jsx:15">
P1: Camera resource leak on unmount due to stale closure. The `useEffect` cleanup captures `stopCamera` from the initial render when `stream` is `null`, so the camera is never stopped. Use a ref to track the media stream so the cleanup always has access to the current value.
Example fix using a ref:
```jsx
const streamRef = useRef(null);
useEffect(() => {
startCamera();
return () => {
if (streamRef.current) {
streamRef.current.getTracks().forEach(track => track.stop());
}
};
}, []);
And update startCamera to also set streamRef.current = mediaStream.
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
frontend/src/App.jsx
Outdated
| <CivicEyeDetector onBack={() => navigate('/')} /> | ||
| </div> | ||
| } /> | ||
| <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} /> |
There was a problem hiding this comment.
P2: Bug: onBack navigates to '/' (Landing page) instead of '/home' (Dashboard). Since this PR moved the Dashboard from / to /home, clicking "Back" from these detectors will take users to the Landing page instead of the Dashboard.
Note: the same issue exists on all the pre-existing detector routes (e.g., PotholeDetector, GarbageDetector, etc.), which should also be updated to navigate('/home') as part of this routing change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/App.jsx, line 301:
<comment>Bug: `onBack` navigates to `'/'` (Landing page) instead of `'/home'` (Dashboard). Since this PR moved the Dashboard from `/` to `/home`, clicking "Back" from these detectors will take users to the Landing page instead of the Dashboard.
Note: the same issue exists on all the pre-existing detector routes (e.g., `PotholeDetector`, `GarbageDetector`, etc.), which should also be updated to `navigate('/home')` as part of this routing change.</comment>
<file context>
@@ -296,6 +298,8 @@ function AppContent() {
<CivicEyeDetector onBack={() => navigate('/')} />
</div>
} />
+ <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} />
+ <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/')} />} />
<Route path="/my-reports" element={
</file context>
There was a problem hiding this comment.
Pull request overview
Adds two new Hugging Face (CLIP zero-shot) detection experiences—Public Facilities and Construction Safety—and wires them through backend endpoints, frontend API helpers, UI navigation, and a Playwright-based verification script. Also updates routing so Landing is served at / and the Dashboard/Home is served at /home.
Changes:
- Backend: add
/api/detect-public-facilitiesand/api/detect-construction-safetyendpoints and corresponding HF CLIP helpers. - Frontend: add two new detector pages/components, add dashboard buttons, and register new routes/API helpers.
- Tooling: add a
verify.shrunner plus a Playwright script to validate the new buttons/pages.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| verify.sh | Starts backend + Vite dev server and runs Playwright verification script. |
| verification/verify_new_detectors.py | Playwright script that navigates UI and verifies new detector buttons/pages. |
| frontend/src/views/Home.jsx | Adds “Public Facilities” and “Construction Safety” buttons to the dashboard grid. |
| frontend/src/api/detectors.js | Adds new detector API helpers for the two new backend endpoints. |
| frontend/src/PublicFacilitiesDetector.jsx | New detector UI/page that captures camera frame and calls the new endpoint. |
| frontend/src/ConstructionSafetyDetector.jsx | New detector UI/page that captures camera frame and calls the new endpoint. |
| frontend/src/App.jsx | Registers new routes + updates Home routing to /home. |
| backend/routers/detection.py | Adds two new FastAPI endpoints and cached detector wrappers. |
| backend/hf_api_service.py | Implements the two new CLIP-based detection functions and response shapes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
verify.sh
Outdated
| python start-backend.py > backend.log 2>&1 & | ||
| BACKEND_PID=$! | ||
| cd frontend && npm run dev -- --port 5173 > frontend.log 2>&1 & | ||
| FRONTEND_PID=$! |
There was a problem hiding this comment.
This script may not reliably stop the Vite dev server: cd frontend && npm run dev ... & backgrounds a compound command, so $FRONTEND_PID is the subshell PID rather than the underlying Vite process, and kill $FRONTEND_PID may leave the server running. Consider using npm --prefix frontend run dev ... & (or similar) and add a trap to ensure both processes are cleaned up on errors/CTRL-C.
| @@ -0,0 +1,179 @@ | |||
| import React, { useRef, useState, useEffect } from 'react'; | |||
| import { Camera, Eye, Activity, Shield, AlertTriangle, RefreshCw, HardHat, Hammer, Construction } from 'lucide-react'; | |||
There was a problem hiding this comment.
lucide-react imports include unused icons (Camera, Hammer, Construction). This repo enforces no-unused-vars as an error, so this file will fail linting. Remove unused imports or use them.
| import { Camera, Eye, Activity, Shield, AlertTriangle, RefreshCw, HardHat, Hammer, Construction } from 'lucide-react'; | |
| import { Eye, Activity, Shield, AlertTriangle, RefreshCw, HardHat } from 'lucide-react'; |
frontend/src/App.jsx
Outdated
| <Route path="/public-facilities" element={<PublicFacilitiesDetector onBack={() => navigate('/')} />} /> | ||
| <Route path="/construction-safety" element={<ConstructionSafetyDetector onBack={() => navigate('/')} />} /> |
There was a problem hiding this comment.
These new detector routes wire onBack to navigate('/'), but / is treated as the Landing page (see isLandingPage logic) and the dashboard is now at /home. If the intended back destination is the dashboard, update onBack to navigate to /home (or use the existing navigateToView('home')).
frontend/src/api/detectors.js
Outdated
| publicFacilities: createDetectorApi('/api/detect-public-facilities'), | ||
| constructionSafety: createDetectorApi('/api/detect-construction-safety'), |
There was a problem hiding this comment.
New API methods were added here, but the Jest suite in frontend/src/api/__tests__/detectors.test.js enumerates detector endpoints explicitly and doesn’t include these. Add test cases for publicFacilities and constructionSafety to keep endpoint coverage consistent.
backend/routers/detection.py
Outdated
| @router.post("/api/detect-public-facilities") | ||
| async def detect_public_facilities_endpoint(image: UploadFile = File(...)): | ||
| # Optimized Image Processing: Validation + Optimization | ||
| _, image_bytes = await process_uploaded_image(image) | ||
|
|
||
| try: | ||
| return await _cached_detect_public_facilities(image_bytes) | ||
| except Exception as e: |
There was a problem hiding this comment.
New endpoints were added, but there are existing FastAPI endpoint tests under tests/ (e.g., test_smart_scan.py, test_graffiti_endpoint.py) and nothing currently exercises /api/detect-public-facilities. Add a test that posts a dummy image and asserts the expected response shape (and/or mocks detect_public_facilities_clip).
| @@ -0,0 +1,179 @@ | |||
| import React, { useRef, useState, useEffect } from 'react'; | |||
| import { Camera, Eye, Activity, Shield, Sparkles, MapPin, RefreshCw, AlertTriangle, Building, Trash2 } from 'lucide-react'; | |||
There was a problem hiding this comment.
lucide-react imports include several unused icons (e.g., Camera, MapPin, Trash2). With the repo’s ESLint no-unused-vars rule set to error, this will fail npm run lint. Remove unused imports or use them in the component.
| import { Camera, Eye, Activity, Shield, Sparkles, MapPin, RefreshCw, AlertTriangle, Building, Trash2 } from 'lucide-react'; | |
| import { Eye, Activity, Shield, Sparkles, RefreshCw, AlertTriangle, Building } from 'lucide-react'; |
| context.drawImage(video, 0, 0); | ||
|
|
||
| canvas.toBlob(async (blob) => { | ||
| if (!blob) return; |
There was a problem hiding this comment.
If canvas.toBlob yields a null blob (can happen when the video hasn’t produced dimensions yet), the function returns early without clearing analyzing, leaving the UI stuck in the loading state. Handle the null-blob path by setting an error and resetting analyzing.
| if (!blob) return; | |
| if (!blob) { | |
| setError("Analysis failed. Please try again."); | |
| setAnalyzing(false); | |
| return; | |
| } |
| canvas.width = video.videoWidth; | ||
| canvas.height = video.videoHeight; | ||
| context.drawImage(video, 0, 0); | ||
|
|
||
| canvas.toBlob(async (blob) => { | ||
| if (!blob) return; |
There was a problem hiding this comment.
If canvas.toBlob returns null (e.g., video width/height are 0 because the stream isn’t ready), the code returns without clearing analyzing, leaving the spinner overlay stuck. Reset analyzing and surface an actionable error when blob creation fails.
| canvas.width = video.videoWidth; | |
| canvas.height = video.videoHeight; | |
| context.drawImage(video, 0, 0); | |
| canvas.toBlob(async (blob) => { | |
| if (!blob) return; | |
| // Ensure the video stream is ready before capturing a frame | |
| if (!video.videoWidth || !video.videoHeight) { | |
| setAnalyzing(false); | |
| setError("Camera stream is not ready yet. Please wait a moment and try again."); | |
| return; | |
| } | |
| canvas.width = video.videoWidth; | |
| canvas.height = video.videoHeight; | |
| context.drawImage(video, 0, 0); | |
| canvas.toBlob(async (blob) => { | |
| if (!blob) { | |
| setAnalyzing(false); | |
| setError("Unable to capture image from camera. Please ensure the camera is active and try again."); | |
| return; | |
| } |
backend/routers/detection.py
Outdated
| @router.post("/api/detect-construction-safety") | ||
| async def detect_construction_safety_endpoint(image: UploadFile = File(...)): | ||
| # Optimized Image Processing: Validation + Optimization | ||
| _, image_bytes = await process_uploaded_image(image) | ||
|
|
||
| try: | ||
| return await _cached_detect_construction_safety(image_bytes) | ||
| except Exception as e: |
There was a problem hiding this comment.
There’s no test coverage for the new /api/detect-construction-safety endpoint, despite the repo having endpoint-level tests for other detectors. Add a FastAPI TestClient test to validate status code + response contract (and mock detect_construction_safety_clip if needed).
…toml config) Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/utils.py`:
- Around line 85-99: The fallback MIME check in the else branch
(mimetypes.guess_type using file.filename) is spoofable and currently lets
guessed_mime==None pass silently; update the logic so that when HAVE_MAGIC is
false you treat an unknown or missing guessed_mime as invalid (reject with
HTTPException using ALLOWED_MIME_TYPES) rather than allowing it, and do not rely
solely on file.filename—use the file content buffer as the authoritative source
where possible (e.g., mirror the magic.from_buffer behavior or run the content
through the same validation used later such as Image open/validation), fix the
indentation on the raise to match the HAVE_MAGIC branch, and keep references to
HAVE_MAGIC, magic.from_buffer, mimetypes.guess_type, ALLOWED_MIME_TYPES,
HTTPException and file.filename to locate the change.
- Around line 177-186: process_uploaded_image_sync currently skips MIME checking
if HAVE_MAGIC is False, causing inconsistency with _validate_uploaded_file_sync;
update process_uploaded_image_sync to perform the same fallback: when HAVE_MAGIC
is False call mimetypes.guess_type on the filename (or buffer) and verify the
guessed MIME is in ALLOWED_MIME_TYPES, treating unknown/None as disallowed
(deny-unknown), and only proceed to PIL image validation afterwards; keep
references to HAVE_MAGIC, magic.from_buffer, mimetypes.guess_type,
ALLOWED_MIME_TYPES, and process_uploaded_image_sync to locate the change.
🧹 Nitpick comments (1)
backend/utils.py (1)
63-148: Consider extracting duplicated MIME-check + PIL-validation logic into a shared helper.
_validate_uploaded_file_syncandprocess_uploaded_image_syncboth implement the same sequence: seek-to-end for size → MIME check → PIL open → resize. The duplication makes it easy for the two paths to drift (as seen with the inconsistent fallback). Extracting the common pre-validation (size check + MIME check) into a single private function would keep behavior consistent and reduce maintenance surface.Also applies to: 156-228
| if HAVE_MAGIC: | ||
| detected_mime = magic.from_buffer(file_content, mime=True) | ||
|
|
||
| if detected_mime not in ALLOWED_MIME_TYPES: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||
| ) | ||
|
|
||
| # If no magic, rely on PIL below (which is safer anyway for images) |
There was a problem hiding this comment.
Inconsistent fallback: this path skips MIME checking entirely when libmagic is absent.
In _validate_uploaded_file_sync (line 92-99), the no-magic fallback at least attempts mimetypes.guess_type. Here in process_uploaded_image_sync, the fallback is to skip MIME validation altogether and rely only on PIL. This inconsistency means the two processing pipelines offer different security guarantees.
Consider applying the same mimetypes.guess_type fallback (with the deny-unknown fix suggested above) to keep both paths aligned:
Proposed fix
if HAVE_MAGIC:
detected_mime = magic.from_buffer(file_content, mime=True)
if detected_mime not in ALLOWED_MIME_TYPES:
raise HTTPException(
status_code=400,
detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}"
)
- # If no magic, rely on PIL below (which is safer anyway for images)
+ else:
+ guessed_mime, _ = mimetypes.guess_type(file.filename or "unknown")
+ if not guessed_mime or guessed_mime not in ALLOWED_MIME_TYPES:
+ raise HTTPException(
+ status_code=400,
+ detail=f"Invalid file type. Only image files are allowed. Detected: {guessed_mime or 'unknown'}"
+ )🧰 Tools
🪛 Ruff (0.14.14)
[warning] 181-184: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
In `@backend/utils.py` around lines 177 - 186, process_uploaded_image_sync
currently skips MIME checking if HAVE_MAGIC is False, causing inconsistency with
_validate_uploaded_file_sync; update process_uploaded_image_sync to perform the
same fallback: when HAVE_MAGIC is False call mimetypes.guess_type on the
filename (or buffer) and verify the guessed MIME is in ALLOWED_MIME_TYPES,
treating unknown/None as disallowed (deny-unknown), and only proceed to PIL
image validation afterwards; keep references to HAVE_MAGIC, magic.from_buffer,
mimetypes.guess_type, ALLOWED_MIME_TYPES, and process_uploaded_image_sync to
locate the change.
There was a problem hiding this comment.
2 issues 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="backend/utils.py">
<violation number="1" location="backend/utils.py:95">
P1: Security bypass: when `guessed_mime` is `None` (no extension or unrecognized extension), the MIME check is silently skipped. This allows arbitrary file types through when `libmagic` is unavailable. Invert the logic to reject files whose MIME type cannot be determined.</violation>
<violation number="2" location="backend/utils.py:186">
P2: No MIME validation fallback in `process_uploaded_image_sync` when `libmagic` is unavailable. Unlike `_validate_uploaded_file_sync` which has a `mimetypes` fallback, this function skips MIME checking entirely. Add the same `mimetypes.guess_type` fallback for consistency and defense in depth.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…toml config) Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
🔍 Quality Reminder |
There was a problem hiding this comment.
5 issues found across 15 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">
P1: `motion` is still used in the scroll-to-top button but was removed from the framer-motion import, causing a runtime ReferenceError. Re-add `motion` to the import.</violation>
</file>
<file name="backend/utils.py">
<violation number="1">
P1: Removing the `libmagic` fallback is a reliability regression. The previous code gracefully handled the case where the `magic` library (or its system dependency `libmagic`) wasn't available by falling back to `mimetypes.guess_type()`. This bare `import magic` will crash the entire application at startup on hosts without `libmagic` installed. Consider restoring the try/except import pattern with the `mimetypes` fallback.</violation>
</file>
<file name="frontend/src/App.jsx">
<violation number="1">
P1: Routing to `'/'` for the Home view conflicts with the existing `isLandingPage` short-circuit, so the Home dashboard never renders and users are stuck on the landing page.</violation>
</file>
<file name="start-backend.py">
<violation number="1" location="start-backend.py:38">
P2: Injecting dummy credentials (`"dummy_key_for_startup"`, `"dummy_token_for_startup"`) into the environment masks misconfiguration. Downstream code checking `os.getenv("GEMINI_API_KEY")` will believe the key is configured and attempt real API calls, producing confusing auth errors instead of clear 'not configured' messages. Consider using a sentinel pattern (e.g., empty string or a dedicated feature-flag check) so downstream code can distinguish real from placeholder values.</violation>
<violation number="2" location="start-backend.py:42">
P1: Setting a dummy `FRONTEND_URL` of `http://localhost:5173` bypasses the explicit production security guard in `backend/main.py` (line 122-126) that raises `ValueError` when `FRONTEND_URL` is missing in production. Since `ENVIRONMENT` defaults to `"production"` in this same function, the CORS allowed origins will be set to only `["http://localhost:5173"]` — blocking all legitimate frontend requests from the real deployed domain. Either remove this dummy or align it with the downstream security check.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if "TELEGRAM_BOT_TOKEN" in missing_vars: | ||
| os.environ["TELEGRAM_BOT_TOKEN"] = "dummy_token_for_startup" | ||
| if "FRONTEND_URL" in missing_vars: | ||
| os.environ["FRONTEND_URL"] = "http://localhost:5173" # Default fallback |
There was a problem hiding this comment.
P1: Setting a dummy FRONTEND_URL of http://localhost:5173 bypasses the explicit production security guard in backend/main.py (line 122-126) that raises ValueError when FRONTEND_URL is missing in production. Since ENVIRONMENT defaults to "production" in this same function, the CORS allowed origins will be set to only ["http://localhost:5173"] — blocking all legitimate frontend requests from the real deployed domain. Either remove this dummy or align it with the downstream security check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At start-backend.py, line 42:
<comment>Setting a dummy `FRONTEND_URL` of `http://localhost:5173` bypasses the explicit production security guard in `backend/main.py` (line 122-126) that raises `ValueError` when `FRONTEND_URL` is missing in production. Since `ENVIRONMENT` defaults to `"production"` in this same function, the CORS allowed origins will be set to only `["http://localhost:5173"]` — blocking all legitimate frontend requests from the real deployed domain. Either remove this dummy or align it with the downstream security check.</comment>
<file context>
@@ -24,12 +27,22 @@ def validate_environment():
+ if "TELEGRAM_BOT_TOKEN" in missing_vars:
+ os.environ["TELEGRAM_BOT_TOKEN"] = "dummy_token_for_startup"
+ if "FRONTEND_URL" in missing_vars:
+ os.environ["FRONTEND_URL"] = "http://localhost:5173" # Default fallback
+
+ else:
</file context>
|
|
||
| # Set dummy values to prevent immediate crashes during import/init | ||
| if "GEMINI_API_KEY" in missing_vars: | ||
| os.environ["GEMINI_API_KEY"] = "dummy_key_for_startup" |
There was a problem hiding this comment.
P2: Injecting dummy credentials ("dummy_key_for_startup", "dummy_token_for_startup") into the environment masks misconfiguration. Downstream code checking os.getenv("GEMINI_API_KEY") will believe the key is configured and attempt real API calls, producing confusing auth errors instead of clear 'not configured' messages. Consider using a sentinel pattern (e.g., empty string or a dedicated feature-flag check) so downstream code can distinguish real from placeholder values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At start-backend.py, line 38:
<comment>Injecting dummy credentials (`"dummy_key_for_startup"`, `"dummy_token_for_startup"`) into the environment masks misconfiguration. Downstream code checking `os.getenv("GEMINI_API_KEY")` will believe the key is configured and attempt real API calls, producing confusing auth errors instead of clear 'not configured' messages. Consider using a sentinel pattern (e.g., empty string or a dedicated feature-flag check) so downstream code can distinguish real from placeholder values.</comment>
<file context>
@@ -24,12 +27,22 @@ def validate_environment():
+
+ # Set dummy values to prevent immediate crashes during import/init
+ if "GEMINI_API_KEY" in missing_vars:
+ os.environ["GEMINI_API_KEY"] = "dummy_key_for_startup"
+ if "TELEGRAM_BOT_TOKEN" in missing_vars:
+ os.environ["TELEGRAM_BOT_TOKEN"] = "dummy_token_for_startup"
</file context>
…tlify.toml) Co-authored-by: RohanExploit <178623867+RohanExploit@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@start-backend.py`:
- Around line 36-42: The current startup code in start-backend.py sets dummy
environment values for GEMINI_API_KEY, TELEGRAM_BOT_TOKEN, and FRONTEND_URL
which masks misconfiguration; remove the assignments that inject
"dummy_key_for_startup", "dummy_token_for_startup", and the default
"http://localhost:5173" so missing vars remain unset, and instead add runtime
guards where those values are used (e.g., check os.getenv("GEMINI_API_KEY") and
os.getenv("TELEGRAM_BOT_TOKEN") before making Gemini/Telegram calls and return a
clear "feature disabled" or configuration error, and validate FRONTEND_URL at
request-time or startup with a clear error if required).
🧹 Nitpick comments (2)
netlify.toml (1)
6-6: Consider usingnpm ciinstead ofnpm installfor reproducible builds.In CI/CD environments,
npm ciensures a clean install frompackage-lock.json, avoiding unexpected dependency drift. The PR description even mentionsnpm ciadjustments, but this command still usesnpm install.Proposed fix
- command = "cd frontend && npm install && npm run build" + command = "cd frontend && npm ci && npm run build"start-backend.py (1)
17-63:validate_environmentalways returnsTrue— consider returning the list of missing vars.The return value is unused at the call site (Line 75), and it always returns
Trueregardless of outcome. If you want callers to react, return the missing vars list or a boolean indicating full readiness.
| # Set dummy values to prevent immediate crashes during import/init | ||
| if "GEMINI_API_KEY" in missing_vars: | ||
| os.environ["GEMINI_API_KEY"] = "dummy_key_for_startup" | ||
| if "TELEGRAM_BOT_TOKEN" in missing_vars: | ||
| os.environ["TELEGRAM_BOT_TOKEN"] = "dummy_token_for_startup" | ||
| if "FRONTEND_URL" in missing_vars: | ||
| os.environ["FRONTEND_URL"] = "http://localhost:5173" # Default fallback |
There was a problem hiding this comment.
Injecting dummy secrets masks misconfiguration and risks silent failures.
Setting fake values for GEMINI_API_KEY and TELEGRAM_BOT_TOKEN means downstream code will attempt real API calls with invalid credentials, producing confusing 401/403 errors instead of clear "not configured" failures. The FRONTEND_URL fallback to localhost:5173 could also silently break CORS in a deployed environment.
A safer pattern is to leave missing vars unset and guard usage at call sites (e.g., check os.getenv("GEMINI_API_KEY") before making Gemini calls and return a clear "feature disabled" response). This keeps startup alive without masking the root cause.
🧰 Tools
🪛 Ruff (0.14.14)
[error] 40-40: Possible hardcoded password assigned to: "TELEGRAM_BOT_TOKEN"
(S105)
🤖 Prompt for AI Agents
In `@start-backend.py` around lines 36 - 42, The current startup code in
start-backend.py sets dummy environment values for GEMINI_API_KEY,
TELEGRAM_BOT_TOKEN, and FRONTEND_URL which masks misconfiguration; remove the
assignments that inject "dummy_key_for_startup", "dummy_token_for_startup", and
the default "http://localhost:5173" so missing vars remain unset, and instead
add runtime guards where those values are used (e.g., check
os.getenv("GEMINI_API_KEY") and os.getenv("TELEGRAM_BOT_TOKEN") before making
Gemini/Telegram calls and return a clear "feature disabled" or configuration
error, and validate FRONTEND_URL at request-time or startup with a clear error
if required).
There was a problem hiding this comment.
2 issues found across 3 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="backend/utils.py">
<violation number="1" location="backend/utils.py:95">
P1: Security bypass: when `guessed_mime` is `None` (no file extension or unrecognizable extension), the MIME type check is silently skipped, allowing any file through. The condition should reject files when the MIME type cannot be determined, not pass them through.</violation>
<violation number="2" location="backend/utils.py:185">
P2: No MIME type fallback in `process_uploaded_image_sync` when `HAVE_MAGIC` is False. Unlike `_validate_uploaded_file_sync`, this function performs zero MIME type validation without libmagic. Add a `mimetypes`-based fallback here too for consistency.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| else: | ||
| # Fallback if libmagic is missing | ||
| guessed_mime, _ = mimetypes.guess_type(file.filename or "unknown") | ||
| if guessed_mime and guessed_mime not in ALLOWED_MIME_TYPES: |
There was a problem hiding this comment.
P1: Security bypass: when guessed_mime is None (no file extension or unrecognizable extension), the MIME type check is silently skipped, allowing any file through. The condition should reject files when the MIME type cannot be determined, not pass them through.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils.py, line 95:
<comment>Security bypass: when `guessed_mime` is `None` (no file extension or unrecognizable extension), the MIME type check is silently skipped, allowing any file through. The condition should reject files when the MIME type cannot be determined, not pass them through.</comment>
<file context>
@@ -77,13 +82,21 @@ def _validate_uploaded_file_sync(file: UploadFile) -> Optional[Image.Image]:
+ else:
+ # Fallback if libmagic is missing
+ guessed_mime, _ = mimetypes.guess_type(file.filename or "unknown")
+ if guessed_mime and guessed_mime not in ALLOWED_MIME_TYPES:
+ raise HTTPException(
+ status_code=400,
</file context>
| detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}" | ||
| ) | ||
|
|
||
| # If no magic, rely on PIL below (which is safer anyway for images) |
There was a problem hiding this comment.
P2: No MIME type fallback in process_uploaded_image_sync when HAVE_MAGIC is False. Unlike _validate_uploaded_file_sync, this function performs zero MIME type validation without libmagic. Add a mimetypes-based fallback here too for consistency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils.py, line 185:
<comment>No MIME type fallback in `process_uploaded_image_sync` when `HAVE_MAGIC` is False. Unlike `_validate_uploaded_file_sync`, this function performs zero MIME type validation without libmagic. Add a `mimetypes`-based fallback here too for consistency.</comment>
<file context>
@@ -160,13 +173,16 @@ def process_uploaded_image_sync(file: UploadFile) -> tuple[Image.Image, bytes]:
+ detail=f"Invalid file type. Only image files are allowed. Detected: {detected_mime}"
+ )
+
+ # If no magic, rely on PIL below (which is safer anyway for images)
try:
</file context>
| # If no magic, rely on PIL below (which is safer anyway for images) | |
| else: | |
| guessed_mime, _ = mimetypes.guess_type(file.filename or "unknown") | |
| if not guessed_mime or guessed_mime not in ALLOWED_MIME_TYPES: | |
| raise HTTPException( | |
| status_code=400, | |
| detail=f"Invalid file type. Only image files are allowed. Detected: {guessed_mime}" | |
| ) |
1 participant
Added 'Public Facilities' and 'Construction Safety' detection features using Hugging Face API (CLIP zero-shot classification).
Updated backend API with new endpoints.
Updated frontend with new detector components and dashboard buttons.
Refined routing logic to serve the Dashboard at '/home' and Landing page at '/'.
Verified functionality with end-to-end test script.
PR created automatically by Jules for task 12037068576323318443 started by @RohanExploit
Summary by cubic
Adds two image detectors (Public Facilities and Construction Safety) with new UI pages and API endpoints. Improves Netlify/Render stability with optional libmagic, safer MIME checks, non-blocking env var validation, and build tweaks; moves the Dashboard to /home.
New Features
Deployment
Written for commit b1cd6b0. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Improvements
Build & Deployment