[Feature] Integrated Ruff linter#29
Conversation
… handling - Organized import statements for clarity - Enhanced error messages for missing Supabase credentials - Improved formatting and consistency in string concatenation - Streamlined file upload and processing logic - Added exception handling for upload errors - Updated database status handling during file processing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded Ruff linting (dependency + format config), inserted a CI lint step, and applied project-wide stylistic formatting changes (quote-style normalization and minor reflows); no functional API changes aside from formatting and one constructor default quote normalization. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / Push
participant GH as GitHub Actions
participant PB as "Install Playwright browsers"
participant Lint as "Run linter (uv run lint)"
participant Test as "Run pytest"
Dev->>GH: push / open PR
GH->>PB: install browsers
PB-->>GH: done
GH->>Lint: run `uv run lint`
alt lint passes
Lint-->>GH: success
GH->>Test: run pytest
Test-->>GH: results
else lint fails
Lint-->>GH: fail (stop CI)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app.py (1)
317-378: Day filter window uses the wrong year/month variables (date‑range bug)In the
/filterroute’s day filter, the upper bound is built usingcurrent_year/current_monthinstead ofnext_year_for_day/next_month_for_day. For dates at or near the end of a month (or year), this can produce an upper bound earlier than the lower bound and yield no results.For example, filtering on day
"31"of a 31‑day month will setnext_day = 1andnext_month_for_day = current_month + 1, but thelt(...)boundary still usescurrent_month, so the interval becomes inconsistent.I recommend constructing the range using actual
datetimearithmetic. For instance:- if day != "any": - current_year = year if year != "any" else str(datetime.now().year) - current_month = month if month != "any" else str(datetime.now().month) - next_day = int(day) + 1 - next_month_for_day = int(current_month) - next_year_for_day = int(current_year) - if next_day > 28: - try: - datetime(int(current_year), int(current_month), next_day) - except ValueError: - next_day = 1 - next_month_for_day += 1 - if next_month_for_day > 12: - next_month_for_day = 1 - next_year_for_day += 1 - - query = query.gte( - TIMESTAMP_FIELD, - f"{current_year}-{current_month.zfill(2)}-{day.zfill(2)}T00:00:00Z", - ).lt( - TIMESTAMP_FIELD, - f"{current_year}-{current_month.zfill(2)}-{str(next_day).zfill(2)}T00:00:00Z", - ) + if day != "any": + current_year = int(year) if year != "any" else datetime.now().year + current_month = int(month) if month != "any" else datetime.now().month + current_day = int(day) + + start_dt = datetime(current_year, current_month, current_day) + end_dt = start_dt + timedelta(days=1) + + query = query.gte( + TIMESTAMP_FIELD, + start_dt.strftime("%Y-%m-%dT%H:%M:%SZ"), + ).lt( + TIMESTAMP_FIELD, + end_dt.strftime("%Y-%m-%dT%H:%M:%SZ"), + )This avoids manual calendar edge cases entirely.
🧹 Nitpick comments (12)
.github/workflows/python-app.yml (1)
41-42: Ensureuv run lintis wired to an actual command/scriptThe workflow assumes a
lintentrypoint exists; if there’s no[project.scripts](or equivalent) defininglint, this step will fail at runtime. Please confirm there is alintscript configured (e.g., wrappingruff check .), or consider callinguv run ruff check .directly here.pyproject.toml (1)
20-29: Ruff integration looks good; consider centralizing lint rulesAdding
ruff>=0.14.5todevand the basic[tool.ruff.format]section is fine and consistent with the rest of the PR. As a follow-up (optional), you might add a[tool.ruff.lint]section (e.g.,select,ignore,line-length) so lint configuration is explicit and version-controlled rather than relying on defaults.app.py (7)
28-33: Hard‑coded timezone inget_local_timestamp
get_local_timestampis fixed to"Asia/Karachi". If you ever need to deploy this app in other regions or make it configurable per environment, consider driving the timezone from an environment variable (with"Asia/Karachi"as a default) instead of hard-coding it.
99-127: Dynamic timestamp field resolution is robust but runs at import timeThe
resolve_timestamp_field()helper is a nice way to adapt to schema differences and has a sensible fallback to"timestamp". Just be aware it runs at import time and does a live Supabase query; if environment variables are misconfigured or Supabase is temporarily unavailable, importingapp.pywill log errors and still fall back to"timestamp". That’s fine for this app, but if you later move toward more structured startup/health-check flows, this logic might belong in an explicit initialization phase.
179-207: Home route timestamp/video handling looks correct and defensiveThe updated
home()route wiring — dynamicselect_fields, safe parsing withdateutil.parser, and separate video lookup with.single()— is consistent and has sensible fallbacks for failures. TheMarkup(post.get("content", ""))usage is appropriate given content is admin-authored; just keep in mind this trusts HTML from your DB, so if you ever allow untrusted authors you’ll want explicit sanitization.
256-303: Year/month/day aggregation queries are fine but could be reducedThe logic to build
available_years,available_months, andavailable_daysvia three separate ordered queries works and is easy to follow. If this ever shows up in profiles as a hot path, you could reduce trips to Supabase by deriving these from a single query or by caching the results for some TTL, but that’s an optimization rather than a requirement.
490-517: Persistent login cookie lacks security attributesThe “remember me” cookie is functional, but it’s being set without
httponly,secure, orsamesiteflags, which weakens your auth posture (particularly if you ever serve over HTTPS and/or are concerned about CSRF and XSS).Consider tightening this:
- response.set_cookie("remember_me", token, max_age=30 * 24 * 60 * 60) + response.set_cookie( + "remember_me", + token, + max_age=30 * 24 * 60 * 60, + httponly=True, + secure=True, # if you’re serving over HTTPS + samesite="Lax", + )and mirroring these attributes when deleting the cookie.
901-939: Admin inspect endpoint and key masking look good; consider logging levelThe
/admin/inspectroute’s key masking and summary info (supabase_url, masked key, bucket, max_id, counts) look sensible and gated on"admin" in session. If you ever add structured logging, consider emitting these diagnostics via a logger rather than relying solely on JSON responses or
942-947: Mismatch between upload storage path and Worker’s temp directoryHere you serve from
"tmp/uploads"(relative), whileWorkerwrites video files under"/tmp/uploads"(absolute). If you expect/uploads/<filename>to serve files written byWorker, these two paths won’t line up on most systems.Either align the paths (e.g., both use
/tmp/uploads) or derive from a shared config constant to avoid drift.worker.py (2)
65-83: Broad exception in_upload_folder_to_supabaseCatching a bare
Exceptionand just
86-128: Add timeout and cleanup to video processing HTTP call
_process_filedoes the right thing in terms of status transitions (“processing” → “processed”/“failed”) and cleanup on Supabase, but a couple of operational improvements would help:
requests.post(...)has no timeout, so a hung external service can stall the worker indefinitely.- The local temp file under
/tmp/uploadsis never removed, so repeated processing can slowly fill the filesystem.Consider something like:
- with open(video_file_path, "rb") as video: - res = requests.post( - f"https://ffmpeg.pythonanywhere.com/upload/{file_id}", - files={"file": video}, - ) + with open(video_file_path, "rb") as video: + res = requests.post( + f"https://ffmpeg.pythonanywhere.com/upload/{file_id}", + files={"file": video}, + timeout=60, + ) @@ - except Exception as e: - self.supabase_client.table("videos").update({"status": "failed"}).eq( - "id", file_id - ).execute() - raise RuntimeError(f"Error processing file: {e}") + except Exception as e: + self.supabase_client.table("videos").update({"status": "failed"}).eq( + "id", file_id + ).execute() + raise RuntimeError(f"Error processing file: {e}") + finally: + try: + os.remove(video_file_path) + except OSError: + passThis keeps behavior the same while avoiding indefinite hangs and temp‑file buildup.
tests/conftest.py (1)
21-45: Process startup logic is fine; watchshell=Trueon WindowsThe
flask_app_urlfixture’s process startup logic looks reasonable and isolates test env via a copiedenvdict. Usingshell=Truewith a constant command on Windows is acceptable here, but if you ever start interpolating user or configuration input into that command, it would be safer to switch to a direct argv invocation withoutshell=True.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/python-app.yml(1 hunks)app.py(9 hunks)pyproject.toml(1 hunks)tests/conftest.py(3 hunks)tests/test_blog.py(9 hunks)tests/test_edit_post.py(9 hunks)tests/test_video.py(2 hunks)tests/test_video_player.py(6 hunks)tests/test_viewer.py(4 hunks)worker.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
app.py (2)
worker.py (3)
Worker(11-128)save_file(30-50)queue_file(52-63)static/js/script.js (4)
response(16-20)response(27-31)data(21-21)data(32-32)
tests/test_video.py (2)
tests/test_video_player.py (1)
validate_test_video_file(9-19)tests/conftest.py (3)
flask_app_url(18-67)page(71-76)admin_logged_in_page(81-90)
tests/test_blog.py (1)
tests/conftest.py (2)
page(71-76)flask_app_url(18-67)
tests/test_edit_post.py (1)
tests/conftest.py (3)
page(71-76)flask_app_url(18-67)admin_logged_in_page(81-90)
tests/test_video_player.py (1)
tests/conftest.py (2)
page(71-76)flask_app_url(18-67)
tests/test_viewer.py (1)
tests/conftest.py (3)
page(71-76)flask_app_url(18-67)admin_logged_in_page(81-90)
🪛 Ruff (0.14.4)
app.py
174-174: Do not catch blind exception: Exception
(BLE001)
198-198: Do not catch blind exception: Exception
(BLE001)
240-240: Do not catch blind exception: Exception
(BLE001)
247-247: Unsafe use of markupsafe.Markup detected
(S704)
303-303: Do not catch blind exception: Exception
(BLE001)
383-383: Do not catch blind exception: Exception
(BLE001)
417-417: Unsafe use of markupsafe.Markup detected
(S704)
476-476: Do not catch blind exception: Exception
(BLE001)
517-517: Do not catch blind exception: Exception
(BLE001)
540-540: Do not catch blind exception: Exception
(BLE001)
576-576: Do not catch blind exception: Exception
(BLE001)
592-592: Do not catch blind exception: Exception
(BLE001)
649-649: Do not catch blind exception: Exception
(BLE001)
664-664: Do not catch blind exception: Exception
(BLE001)
686-686: Do not catch blind exception: Exception
(BLE001)
721-721: Do not catch blind exception: Exception
(BLE001)
748-748: Do not catch blind exception: Exception
(BLE001)
761-761: Do not catch blind exception: Exception
(BLE001)
806-806: Do not catch blind exception: Exception
(BLE001)
813-813: Do not catch blind exception: Exception
(BLE001)
857-857: Do not catch blind exception: Exception
(BLE001)
882-882: Do not catch blind exception: Exception
(BLE001)
890-890: Unsafe use of markupsafe.Markup detected
(S704)
930-930: Do not catch blind exception: Exception
(BLE001)
936-936: Do not catch blind exception: Exception
(BLE001)
958-958: Do not catch blind exception: Exception
(BLE001)
tests/test_video.py
13-15: Avoid specifying long messages outside the exception class
(TRY003)
tests/conftest.py
31-31: subprocess call with shell=True identified, security issue
(S602)
32-32: Starting a process with a partial executable path
(S607)
40-40: Starting a process with a partial executable path
(S607)
62-62: subprocess call: check for execution of untrusted input
(S603)
63-63: Starting a process with a partial executable path
(S607)
tests/test_video_player.py
15-17: Avoid specifying long messages outside the exception class
(TRY003)
worker.py
13-13: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
22-24: Avoid specifying long messages outside the exception class
(TRY003)
83-83: Do not catch blind exception: Exception
(BLE001)
94-94: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Probable use of requests call without timeout
(S113)
117-117: Abstract raise to an inner function
(TRY301)
117-117: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (8)
app.py (1)
949-965: Main entrypoint is robust and consistentThe main guard’s use of
HOST/PORTenvironment variables, explicitthreaded=True, and the structured error handling withtraceback.print_exc()andsys.exit(1)is solid. No issues here.worker.py (1)
11-28: Worker initialization and scheduler wiring look goodThe Worker constructor’s upload folder setup, APScheduler background scheduler, and Supabase client creation are coherent. The explicit check for missing
SUPABASE_URL/SUPABASE_KEYwith aValueErrorshould fail fast in misconfigured environments, which is helpful.tests/conftest.py (1)
79-90: Admin login fixture matches app behaviorThe
admin_logged_in_pagefixture correctly drives the login form using the same environment variables as the app (ADMIN_USERNAME/ADMIN_PASSWORD) and asserts redirection to/. This is a good end‑to‑end check of the login flow and will catch regressions around the credentials screen.tests/test_video_player.py (1)
1-131: Formatting improvements align with Ruff integration.The import reordering, quote standardization, and whitespace adjustments throughout this file successfully align with the configured Ruff style. All changes are non-functional and improve consistency.
tests/test_video.py (1)
1-158: Formatting improvements align with Ruff integration.The import reordering, quote standardization, multi-line assertion formatting, and spacing adjustments successfully align with the configured Ruff style. All changes maintain test functionality while improving code consistency.
tests/test_blog.py (1)
1-179: Formatting improvements align with Ruff integration.The import spacing, quote standardization to double quotes, and multi-line formatting adjustments (including the PNG byte literal and URL regex expectation) successfully align with the configured Ruff style. All changes are non-functional and improve consistency across the test suite.
tests/test_viewer.py (1)
1-112: Formatting improvements align with Ruff integration.The import spacing, multi-line locator formatting (lines 84-86), and whitespace adjustments successfully align with the configured Ruff style. All changes are non-functional and improve consistency.
tests/test_edit_post.py (1)
1-152: Formatting improvements align with Ruff integration.The import spacing, PNG byte literal multi-line formatting (lines 87-89, 113-115), function signature wrapping (lines 103-105), and other whitespace adjustments successfully align with the configured Ruff style. All changes are non-functional and improve consistency across the test suite.
AmanatAliPanhwer
changed the title
1 participant
📌 Summary
Integrate Ruff linter into the project to enforce code style and catch common errors. This improves code quality and consistency.
🔧 Type of Change
🧩 Related Issues
Closes #24
🧠 Changes Introduced
pyproject.toml..github/workflows/python-app.yml).pyproject.toml(dependency group format, removed incompatible formatting options).✅ Checklist
🧾 Additional Notes
The
uv run ruff checkcommand should now work as expected.Summary by CodeRabbit
Chores
Style