Conversation
WalkthroughThis PR introduces real-time camera-based pose scoring via a new camera module, significantly refactors feature extraction and estimator logic with confidence gating and form-bonus scoring, adds CORS middleware and a debug endpoint to the API, and includes video chunking tooling. Changes
Sequence DiagramsequenceDiagram
participant User
participant Camera as Camera Device
participant App as analyze_camera()
participant MediaPipe as MediaPipe Pose
participant Estimator as Estimator
participant Display as Screen Overlay
User->>App: Start camera analysis
App->>Camera: Initialize camera stream
Camera-->>App: Frame stream ready
rect rgb(200, 220, 255)
Note over App,Display: Real-time Processing Loop
loop Per Frame
App->>Camera: Read frame
Camera-->>App: Frame data
App->>MediaPipe: Detect pose landmarks
MediaPipe-->>App: Landmarks + visibility
App->>Estimator: Extract features from landmarks
Estimator-->>App: Label, score, confidence, temporal stats
App->>Estimator: Update rolling statistics
Estimator-->>App: Updated internal state
App->>Display: Render overlay (label, score, stats)
end
end
rect rgb(220, 240, 220)
Note over App,Estimator: Final Summary Generation
App->>Estimator: Compute aggregate metrics
Estimator-->>App: Summary (FPS, duration, avg/max score, temporal_stats)
end
App-->>User: Return ScoreSummary (JSON)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 8
🧹 Nitpick comments (2)
score/app.py (1)
244-247: Hardcoded max_width removes client flexibility.The
max_widthis now hardcoded to 960, removing the ability for clients to control video resolution. Client scripts still sendmax_widthvalues that are now ignored.Consider restoring the parameter with a default value:
@app.post("/api/score", response_model=ScoreSummary) async def api_score( file: UploadFile = File(...), mode: str = Form("auto"), + max_width: int = Form(960), model_complexity: int = Form(0), min_det_conf: float = Form(0.5), min_track_conf: float = Form(0.5), ):Then use the parameter value:
summary = analyze_video( tmp_path, mode=mode, - max_width=960, + max_width=max_width, model_complexity=model_complexity,score/features.py (1)
226-436: Comprehensive detection pipeline refactoring.The refactored
detect()method introduces several significant improvements:Early exits for insufficient valid frames, a requirements-based gating mechanism, confidence thresholds, and motion-based score smoothing with rise/fall dynamics.
The logic is complex but well-structured. Consider documenting the rationale for key thresholds in the requirements dictionary for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
score/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
score/.gitignore(1 hunks)score/app.py(4 hunks)score/camera.py(1 hunks)score/features.py(9 hunks)score/input/curl.sh(1 hunks)score/input/score.sh(1 hunks)score/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
score/input/score.sh (2)
frontend/app/solo.tsx (3)
video(54-79)ScoreResponse(18-27)camera(42-96)frontend/utils/score.ts (3)
videoUri(37-94)ScoreResponse(9-18)mode(21-35)
score/camera.py (2)
score/features.py (5)
Estimator(89-553)frame_features_from_xyvis(16-86)push(126-146)detect(226-436)_temporal_stats(148-220)score/app.py (3)
TemporalStats(30-34)ScoreSummary(36-47)_temporal_stats_to_dict(49-55)
score/app.py (1)
score/camera.py (1)
ScoreSummary(20-31)
🪛 Ruff (0.14.8)
score/camera.py
5-5: Redefinition of unused Optional from line 1
Remove definition: Optional
(F811)
5-5: Redefinition of unused List from line 1
Remove definition: List
(F811)
5-5: Redefinition of unused Dict from line 1
Remove definition: Dict
(F811)
43-43: Unpacked variable h is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
72-72: Avoid specifying long messages outside the exception class
(TRY003)
80-80: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
81-81: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
score/features.py
98-98: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
436-436: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
score/app.py
196-196: Do not catch blind exception: Exception
(BLE001)
212-212: Do not catch blind exception: Exception
(BLE001)
225-225: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🪛 Shellcheck (0.11.0)
score/input/score.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🔇 Additional comments (8)
score/.gitignore (1)
216-219: LGTM!The added ignore patterns appropriately exclude secrets (.streamlit/secrets.toml), generated video chunks (input/chunks/*), and external detector artifacts (FitPose-Detector).
score/camera.py (1)
52-174: Core camera analysis logic looks solid.The
analyze_camerafunction correctly:
- Initializes MediaPipe Pose with proper parameters
- Handles camera resource lifecycle in a finally block
- Processes frames with feature extraction and scoring
- Aggregates statistics and returns a structured summary
score/features.py (5)
9-14: LGTM on new constants and helper.The new constants (VISIBLE_MEAN_MIN, MIN_SHOULDER_WIDTH_PX, DEFAULT_MIN_VALID_SECONDS) provide clear thresholds for visibility and quality checks. The
safe_meanhelper properly handles empty arrays.
16-86: Improved robustness with visibility and size checks.The function now validates visibility with VISIBLE_MEAN_MIN and checks shoulder width against MIN_SHOULDER_WIDTH_PX, preventing invalid detections from low-quality frames. The epsilon additions in normalization prevent division-by-zero errors.
88-125: Enhanced estimator state management.The new initialization adds smoothing parameters and confidence thresholds, improving score stability and detection reliability. The stable_threshold default increased to 4, requiring more consistent frames before label changes.
148-220: LGTM on temporal statistics improvements.The FFT calculation now guards against edge cases with a positivity check on
yf[idx], and visible_mean is included in the stats dictionary for downstream quality checks.
438-553: Well-structured form scoring implementation.The
_form_bonusandform_scoremethods provide activity-specific scoring with consistent patterns. Each activity has tailored metrics (depth for squats, torso angle for pushups, etc.) that are normalized and combined with appropriate weights.score/pyproject.toml (1)
13-13: No action required.requests>=2.32.5is the current latest version with all known critical security vulnerabilities (CVE-2024-47081, CVE-2024-35195) already resolved in earlier 2.32.x releases. This version has no unresolved CVEs and is secure.
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=["*"], | ||
| allow_credentials=True, | ||
| allow_methods=["*"], | ||
| allow_headers=["*"], | ||
| ) |
There was a problem hiding this comment.
Restrict CORS origins for production security.
The CORS middleware allows all origins (allow_origins=["*"]), which exposes the API to cross-origin requests from any domain. In production, this creates security risks including CSRF attacks.
Restrict allow_origins to known, trusted domains:
app.add_middleware(
CORSMiddleware,
- allow_origins=["*"],
+ allow_origins=["http://localhost:3000", "https://yourdomain.com"],
allow_credentials=True,
allow_methods=["*"],
allow_headers=["*"],
)Consider using environment variables to configure allowed origins per deployment environment.
📝 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.
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=["*"], | |
| allow_credentials=True, | |
| allow_methods=["*"], | |
| allow_headers=["*"], | |
| ) | |
| app.add_middleware( | |
| CORSMiddleware, | |
| allow_origins=["http://localhost:3000", "https://yourdomain.com"], | |
| allow_credentials=True, | |
| allow_methods=["*"], | |
| allow_headers=["*"], | |
| ) |
🤖 Prompt for AI Agents
In score/app.py around lines 22-28 the CORS middleware is configured with
allow_origins=["*"], which is too permissive for production; replace the
wildcard with a configurable list of trusted origins (read from an environment
variable like ALLOWED_ORIGINS that contains a comma-separated list), parse that
variable into a Python list and pass it to allow_origins, and keep a safe
fallback for local dev (e.g., ["http://localhost:3000"]) when the env var is
unset; also ensure allow_credentials is only True when specific origins are set
(not "*") and document the env var usage in deployment configs.
| @app.post("/api/debug") | ||
| async def debug_request(request: Request): | ||
| info = {} | ||
|
|
||
| info["method"] = request.method | ||
| info["url"] = str(request.url) | ||
| info["headers"] = dict(request.headers) | ||
|
|
||
| info["content_type"] = request.headers.get("content-type") | ||
|
|
||
| info["query_params"] = dict(request.query_params) | ||
|
|
||
| body = await request.body() | ||
| info["body_raw_length"] = len(body) | ||
|
|
||
| try: | ||
| info["body_raw_preview"] = body[:1000].decode("utf-8", errors="replace") | ||
| except Exception: | ||
| info["body_raw_preview"] = str(body[:1000]) | ||
|
|
||
| form_data = {} | ||
| files = {} | ||
|
|
||
| try: | ||
| form = await request.form() | ||
| for key, value in form.items(): | ||
| if hasattr(value, "filename"): | ||
| files[key] = { | ||
| "filename": value.filename, | ||
| "content_type": value.content_type, | ||
| } | ||
| else: | ||
| form_data[key] = value | ||
| except Exception as e: | ||
| info["form_parse_error"] = str(e) | ||
|
|
||
| info["form_fields"] = form_data | ||
| info["files"] = files | ||
|
|
||
| print(info) | ||
|
|
||
| return JSONResponse(content=info) |
There was a problem hiding this comment.
Remove or secure the debug endpoint before production.
The /api/debug endpoint exposes request metadata, headers, and body content without authentication. Headers may contain sensitive data like authorization tokens, and the body preview could leak PII.
Options:
- Remove entirely if only needed for development
- Add authentication and restrict to admin users
- Use conditional registration based on environment:
+import os
+
+if os.getenv("DEBUG_MODE") == "true":
+ @app.post("/api/debug")
+ async def debug_request(request: Request):
+ # ... existing codeNever deploy this endpoint to production without proper access controls.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.8)
196-196: Do not catch blind exception: Exception
(BLE001)
212-212: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In score/app.py around lines 179 to 220 the /api/debug endpoint is exposing
request headers, body previews and other metadata without any authentication
which can leak sensitive data; remove the endpoint for production or secure it:
either delete this handler entirely if only used in development, or
conditionally register it only when an environment flag (e.g., DEBUG=true) is
set, and/or add strict authentication/authorization (require an admin token or
valid user dependency) before returning any data; additionally redact sensitive
headers (Authorization, Cookie, etc.) and limit or remove body previews to avoid
PII leakage when enabling this endpoint.
| min_track_conf: float = Form(0.5), | ||
| save_annotated: bool = Form(False), | ||
| ): | ||
| print(file) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace print statement with structured logging.
The print(file) statement appears to be debug code. Use proper logging for observability in production.
Apply this diff:
+import logging
+
+logger = logging.getLogger(__name__)
+
@app.post("/api/score", response_model=ScoreSummary)
async def api_score(
file: UploadFile = File(...),
mode: str = Form("auto"),
model_complexity: int = Form(0),
min_det_conf: float = Form(0.5),
min_track_conf: float = Form(0.5),
):
- print(file)
+ logger.info(f"Processing upload: {file.filename}, size: {file.size}, mode: {mode}")📝 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.
| print(file) | |
| logger.info(f"Processing upload: {file.filename}, size: {file.size}, mode: {mode}") |
🤖 Prompt for AI Agents
In score/app.py around line 231, replace the debug print(file) with structured
logging: remove the print call, obtain or create the module logger (e.g. logger
= logging.getLogger(__name__) if not present), and emit a log entry with an
appropriate level (debug or info) that includes the filename and any relevant
context as explicit fields (e.g. logger.debug("processing file", extra={"file":
file}) or logger.debug("processing file=%s", file)); ensure imports for logging
exist and avoid logging sensitive contents.
| from typing import Dict, List, Optional | ||
|
|
||
| import numpy as np | ||
| import time | ||
| from typing import Optional, List, Dict |
There was a problem hiding this comment.
Remove duplicate imports.
Lines 1 and 5 both import Optional, List, and Dict from typing, causing redefinition warnings.
Apply this diff:
from typing import Dict, List, Optional
import numpy as np
import time
-from typing import Optional, List, Dict
import cv2📝 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.
| from typing import Dict, List, Optional | |
| import numpy as np | |
| import time | |
| from typing import Optional, List, Dict | |
| from typing import Dict, List, Optional | |
| import numpy as np | |
| import time |
🧰 Tools
🪛 Ruff (0.14.8)
5-5: Redefinition of unused Optional from line 1
Remove definition: Optional
(F811)
5-5: Redefinition of unused List from line 1
Remove definition: List
(F811)
5-5: Redefinition of unused Dict from line 1
Remove definition: Dict
(F811)
🤖 Prompt for AI Agents
In score/camera.py around lines 1 to 5, there are duplicate typing imports
(Optional, List, Dict) on lines 1 and 5; remove the redundant second "from
typing import Optional, List, Dict" line so the file has only a single import
statement for those types (keep the first or consolidate into one import), and
ensure import ordering remains consistent (typing once, then third-party like
numpy, then stdlib like time).
| class TemporalStats(BaseModel): | ||
| dom_freq: Optional[float] = None | ||
| periodic_strength: Optional[float] = None | ||
| ankle_speed_norm: Optional[float] = None | ||
| hip_y_std_norm: Optional[float] = None | ||
|
|
||
| class ScoreSummary(BaseModel): | ||
| frames_processed: int | ||
| input_fps: float | ||
| duration_seconds: float | ||
| mode: str | ||
| avg_score: Optional[float] = None | ||
| max_score: Optional[int] = None | ||
| last_label: Optional[str] = None | ||
| last_score: Optional[int] = None | ||
| last_conf: Optional[float] = None | ||
| temporal_stats: Optional[TemporalStats] = None | ||
| processing_time_seconds: float | ||
|
|
||
| def _temporal_stats_to_dict(med: Dict) -> Dict: | ||
| return { | ||
| "dom_freq": med.get("dom_freq"), | ||
| "periodic_strength": med.get("periodic_strength"), | ||
| "ankle_speed_norm": med.get("ankle_speed_norm"), | ||
| "hip_y_std_norm": med.get("hip_y_std_norm"), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated models to a shared module.
The TemporalStats, ScoreSummary classes and _temporal_stats_to_dict function are duplicated between this file and score/app.py (lines 30-55). This violates DRY principles.
Create a new file score/models.py:
from typing import Optional, Dict
from pydantic import BaseModel
class TemporalStats(BaseModel):
dom_freq: Optional[float] = None
periodic_strength: Optional[float] = None
ankle_speed_norm: Optional[float] = None
hip_y_std_norm: Optional[float] = None
class ScoreSummary(BaseModel):
frames_processed: int
input_fps: float
duration_seconds: float
mode: str
avg_score: Optional[float] = None
max_score: Optional[int] = None
last_label: Optional[str] = None
last_score: Optional[int] = None
last_conf: Optional[float] = None
temporal_stats: Optional[TemporalStats] = None
processing_time_seconds: float
def temporal_stats_to_dict(med: Dict) -> Dict:
return {
"dom_freq": med.get("dom_freq"),
"periodic_strength": med.get("periodic_strength"),
"ankle_speed_norm": med.get("ankle_speed_norm"),
"hip_y_std_norm": med.get("hip_y_std_norm"),
}Then import in both files:
from score.models import TemporalStats, ScoreSummary, temporal_stats_to_dict🤖 Prompt for AI Agents
In score/camera.py around lines 14 to 39 the TemporalStats and ScoreSummary
models and the _temporal_stats_to_dict function are duplicated elsewhere;
extract these into a new shared module score/models.py (define TemporalStats,
ScoreSummary and temporal_stats_to_dict there using the provided signatures and
types), remove the local class/function definitions from score/camera.py, and
replace them with imports from score.models: from score.models import
TemporalStats, ScoreSummary, temporal_stats_to_dict; ensure names match (rename
_temporal_stats_to_dict to temporal_stats_to_dict) and update any local
references.
| curl -X POST "http://localhost:8000/api/score" \ | ||
| -F "file=@input.mp4" \ | ||
| -F "mode=running" \ | ||
| -F "max_width=640" No newline at end of file | ||
| -F "max_width=640" |
There was a problem hiding this comment.
Parameter sent but ignored by the API.
The script sends max_width=640 to /api/score, but the API now hardcodes max_width=960, ignoring this parameter entirely. This creates confusion about which value is actually used.
Consider either removing the max_width parameter from this script or updating it to match the hardcoded value (960) for consistency. Alternatively, restore the API's ability to accept this parameter if client-side control is needed.
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In score/input/curl.sh around lines 1 to 4, the script sends max_width=640 but
the API hardcodes max_width=960 so the passed parameter is ignored; either
remove the max_width form field from this curl script, or change its value to
960 to match the server, or if you prefer client control, update the server to
read and respect the max_width form parameter; pick one approach and apply it so
the script and API are consistent (remove or update the client parameter to 960,
or modify the API to parse and use the submitted max_width).
| mkdir -p chunks | ||
|
|
||
| ffmpeg -i input.mp4 \ | ||
| -c copy \ | ||
| -map 0 \ | ||
| -f segment \ | ||
| -segment_time 2 \ | ||
| -reset_timestamps 1 \ | ||
| chunks/part_%03d.mp4 |
There was a problem hiding this comment.
Add shebang to specify the shell interpreter.
The script lacks a shebang line, making it unclear which shell should execute it.
Apply this diff to add a shebang:
+#!/bin/bash
mkdir -p chunks
ffmpeg -i input.mp4 \📝 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.
| mkdir -p chunks | |
| ffmpeg -i input.mp4 \ | |
| -c copy \ | |
| -map 0 \ | |
| -f segment \ | |
| -segment_time 2 \ | |
| -reset_timestamps 1 \ | |
| chunks/part_%03d.mp4 | |
| #!/bin/bash | |
| mkdir -p chunks | |
| ffmpeg -i input.mp4 \ | |
| -c copy \ | |
| -map 0 \ | |
| -f segment \ | |
| -segment_time 2 \ | |
| -reset_timestamps 1 \ | |
| chunks/part_%03d.mp4 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In score/input/score.sh around lines 1 to 9, the script is missing a shebang so
the shell interpreter is ambiguous; add a shebang as the first line (e.g.,
#!/bin/sh or #!/usr/bin/env bash depending on required features) so the script
will be executed with the intended shell, then keep the existing mkdir and
ffmpeg commands unchanged.
| curl -X POST "http://localhost:8000/api/score" \ | ||
| -F "file=@${file}" \ | ||
| -F "mode=running" \ | ||
| -F "max_width=640" |
There was a problem hiding this comment.
Parameter sent but ignored by the API.
The script sends max_width=640 to /api/score, but the API now hardcodes max_width=960, making this parameter ineffective.
Consider removing the max_width parameter or updating it to 960 for consistency. This duplicates the issue in curl.sh.
🤖 Prompt for AI Agents
In score/input/score.sh around lines 14 to 17, the script sends max_width=640 to
/api/score but the server now hardcodes max_width=960 so the parameter is
ignored; update the client to match the server by either removing the max_width
parameter entirely or changing its value to 960 (and apply the same change to
curl.sh to keep both callers consistent).
1 participant
一部改善をしました。
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
✏️ Tip: You can customize this high-level summary in your review settings.