[Feature] Implemented video upload and streaming functionality#18
Conversation
`HTTP load failed with status 404. Load of media resource https://blog-1nmkt8ohs-alph702s-projects.vercel.app/uploads/2606ec983f5a4d08bd2f1bd87589a79c.mp4 failed.`
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request adds comprehensive video upload, processing, and streaming capabilities to the blog platform. It introduces a background worker component for asynchronous video transcoding via APScheduler, extends the Flask app to handle video file uploads and metadata tracking, implements drag-and-drop upload UI, adds a client-side video player with HLS.js support, updates the database schema to track video files, and includes end-to-end tests for video workflows. Environment configuration separates image and video bucket names. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Admin User
participant Frontend as Frontend (Browser)
participant Flask as Flask App
participant Worker as Background Worker
participant Supabase as Supabase Storage
participant FFmpeg as FFmpeg Service
participant Database as Database
rect rgba(200,150,255,0.12)
note over Frontend: Video Upload Flow
User->>Frontend: Select & upload video
Frontend->>Flask: POST /new or /edit (video file)
end
rect rgba(150,200,255,0.12)
note over Flask: Backend Processing
Flask->>Supabase: Upload raw video file
Supabase-->>Flask: File stored, path returned
Flask->>Database: Insert video record (status: pending)
Flask->>Worker: Queue processing job (video_id)
Flask-->>Frontend: Return response with video_id
end
rect rgba(150,255,150,0.12)
note over Worker: Async Transcoding
Worker->>Database: Retrieve video metadata
Worker->>Supabase: Download uploaded video
Supabase-->>Worker: Video file
Worker->>FFmpeg: Send for HLS transcoding
FFmpeg-->>Worker: Return .m3u8 & .ts files
Worker->>Supabase: Upload HLS manifest & segments
Worker->>Database: Update video record (status: processed, filepath)
end
rect rgba(255,200,150,0.12)
note over Frontend: Video Playback
User->>Frontend: Navigate to post with video
Frontend->>Flask: GET /post/{id}
Flask->>Database: Fetch post with video_id
Flask->>Database: Fetch video metadata (m3u8 URL)
Flask-->>Frontend: Render post with video player
Frontend->>Frontend: Initialize Hls.js with .m3u8 URL
Frontend->>Supabase: Stream video segments
Supabase-->>Frontend: HLS segments (.ts files)
Frontend->>User: Play video with adaptive bitrate
end
sequenceDiagram
participant UI as Upload UI
participant Input as File Input
participant Uploader as uploader.js
participant Toast as Toast Notif.
participant FormData as FormData
rect rgba(255,200,200,0.12)
note over UI: Click-to-Upload Flow
UI->>Input: User clicks upload box
Input->>Uploader: Change event triggered
Uploader->>UI: Update placeholder with filename
end
rect rgba(200,255,200,0.12)
note over UI: Drag-and-Drop Flow
UI->>Uploader: dragover event
Uploader->>UI: Add visual drag state (highlight)
UI->>Uploader: drop event with files
Uploader->>Uploader: Validate MIME type
alt Valid file type
Uploader->>Input: Set to file input
Uploader->>UI: Update placeholder
else Invalid file type
Uploader->>Toast: Show warning toast
end
Uploader->>UI: Remove drag state (dragleave)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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)
198-262: Propagate video metadata in the filter route.
home()now returns(id, title, Markup(content), image, formatted_timestamp, videodata)for every post.filter_posts()still emits only five tuple elements and never hydratesvideodata. As soon as templates rely onpost.video(or tuple index 5) after this PR, any filtered view breaks—either the tuple unpacking fails or the player never gets the HLS manifest. We need to selectvideo_idand build the same metadata block here.Apply this diff to align both code paths:
- select_fields = f"id, title, content, image, {TIMESTAMP_FIELD}" if TIMESTAMP_FIELD else "id, title, content, image" + select_fields = ( + f"id, title, content, image, {TIMESTAMP_FIELD}, video_id" + if TIMESTAMP_FIELD + else "id, title, content, image, video_id" + ) ... - posts.append((post.get('id'), post.get('title'), Markup(post.get('content', '')), post.get('image'), formatted_ts)) + videodata = None + video_id = post.get('video_id') + if video_id: + try: + video_resp = ( + supabase_client.table('videos') + .select('filepath', 'filename', 'status') + .eq('id', video_id) + .single() + .execute() + ) + video_record = video_resp.data + if video_record: + videodata = { + 'id': video_id, + 'filename': video_record.get('filename'), + 'filepath': video_record.get('filepath'), + 'status': video_record.get('status'), + 'url': video_record.get('filepath'), + } + except Exception as e: + print(f"Error fetching video info for video_id={video_id}: {e}") + + posts.append( + ( + post.get('id'), + post.get('title'), + Markup(post.get('content', '')), + post.get('image'), + formatted_ts, + videodata, + ) + )
🧹 Nitpick comments (5)
templates/index.html (1)
63-77: Clarify unusual<a>tag wrapping withouthrefattribute.The video player is wrapped in an
<a>tag (line 63) but nohrefis present. This is semantically odd—it should either be a<div>or the<a>should have anhrefto maintain accessibility. If the intent is to prevent the post link click, theonclick="event.stopPropagation()"on the inner div should suffice without wrapping in<a>.- <a> - <div class="player" onclick="event.stopPropagation();"> + <div class="player" onclick="event.stopPropagation();">Remove the wrapper
</a>tag on line 77 as well.templates/edit.html (1)
5-9: Move inline CSS to static stylesheet.The inline
<style>block for.vjs-techpadding should be moved to the main CSS file (static/css/style.css) to keep concerns separated and avoid inline styles. The!importantflag can typically be replaced with proper CSS specificity.-<style> - .vjs-tech { - padding-top: 0px !important; - } -</style>Add to
static/css/style.css:.vjs-tech { padding-top: 0 !important; /* Required to override Video.js defaults for proper aspect ratio */ }.github/workflows/python-app.yml (1)
21-22: Update workflow to use preferred environment variable names.The workflow defines
SUPERBASE_IMAGES_BUCKET_NAMEandSUPERBASE_VIDEOS_BUCKET_NAME, but app.py prefersBLOG_IMAGES_BUCKETandBLOG_VIDEOS_BUCKET(checking these first with fallback to theSUPERBASE_*names). While the current configuration works through fallback handling, updating the workflow to use the preferred variable names improves consistency and clarity. Change lines 21-22 to useBLOG_IMAGES_BUCKETandBLOG_VIDEOS_BUCKET.pyproject.toml (1)
9-9: Consider using a version constraint that prevents unexpected major version bumps.The constraint
>=3.11.0allows APScheduler 4.x (if released), which could introduce breaking changes. APScheduler 3.11.0 has breaking changes including dropped support for Python 3.6/3.7 and migration from pytz to ZoneInfo. Use~=3.11.0(equivalent to>=3.11.0,<4.0.0) to avoid surprises and review the migration notes if you upgrade from earlier versions.templates/post.html (1)
17-31: Consider explicit escaping for video data attributes; Flask's autoescape already protects these, but |tojson is a defensive best practice.Flask enables Jinja2 autoescape by default for
.htmltemplates, so the video data attributes ({{ post.video.url }}, etc.) are already HTML-escaped. However, using the|tojsonfilter would make escaping explicit and is a safer, more portable practice:- data-id="{{ post.video.id }}" - data-filename="{{ post.video.filename }}" - data-filepath="{{ post.video.filepath }}" - data-status="{{ post.video.status }}" - data-url="{{ post.video.url }}" + data-id="{{ post.video.id | tojson }}" + data-filename="{{ post.video.filename | tojson }}" + data-filepath="{{ post.video.filepath | tojson }}" + data-status="{{ post.video.status | tojson }}" + data-url="{{ post.video.url | tojson }}"Verify that autoescape remains enabled in production and no custom Jinja2 configuration disables it globally.
📜 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 (14)
.github/workflows/python-app.yml(1 hunks).gitignore(1 hunks)app.py(14 hunks)pyproject.toml(1 hunks)static/css/style.css(3 hunks)static/js/uploader.js(1 hunks)static/js/video.js(1 hunks)templates/edit.html(2 hunks)templates/index.html(2 hunks)templates/layout.html(1 hunks)templates/new.html(2 hunks)templates/post.html(2 hunks)tests/test_video.py(1 hunks)worker.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
worker.py
9-9: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
19-19: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Do not catch blind exception: Exception
(BLE001)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
67-67: Local variable filepath is assigned to but never used
Remove assignment to unused variable filepath
(F841)
76-76: Probable use of requests call without timeout
(S113)
81-81: Do not catch blind exception: Exception
(BLE001)
83-83: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
83-83: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_video.py
14-14: Avoid specifying long messages outside the exception class
(TRY003)
app.py
129-129: Do not catch blind exception: Exception
(BLE001)
160-160: Do not catch blind exception: Exception
(BLE001)
164-164: Unsafe use of markupsafe.Markup detected
(S704)
392-392: Do not catch blind exception: Exception
(BLE001)
500-500: Do not catch blind exception: Exception
(BLE001)
530-530: Do not use bare except
(E722)
587-587: Do not catch blind exception: Exception
(BLE001)
593-593: Unsafe use of markupsafe.Markup detected
(S704)
🔇 Additional comments (5)
.gitignore (1)
6-10: LGTM!The restructured ignores correctly support the new upload and processing workflow at the repository root level.
templates/layout.html (2)
17-17: Verify local video.js script for security and functionality.The script at
static/js/video.jsis not provided in this review. Ensure it properly initializes Video.js players, handles HLS streams safely (no XSS via data attributes), and implements quality selection securely. This script should be reviewed separately before merge.
14-19: Consider adding Subresource Integrity (SRI) hashes to external CDN resources as a security best practice.While SRI is a W3C specification recommended as a best-practice whenever libraries are loaded from a third-party source, note that the official Video.js documentation itself does not demonstrate SRI usage for vjs.zencdn.net resources. If you choose to implement SRI:
- For the quality-levels plugin from jsDelivr, SRI is well-supported and recommended.
- For Video.js from vjs.zencdn.net, you would need to verify with the maintainers whether SRI hashes are available and supported, since the official documentation does not include them.
- The CDN must support Cross-Origin Resource Sharing (CORS), enforced via the crossorigin attribute.
Generate hashes using tools like the SRI Hash Generator (srihash.org) or curl, and update the integrity attributes when dependencies are upgraded.
templates/new.html (1)
15-24: Verify uploader.js for secure file handling and CSRF protection.The drag-and-drop upload boxes lack client-side file validation (e.g., size limits, format checks beyond accept attribute). Ensure
uploader.jsimplements proper validation and that form submissions include CSRF tokens. Additionally, verify the backend enforces file size and type restrictions.templates/edit.html (1)
30-45: The review comment is incorrect—no XSS vulnerability exists here.Jinja2 enables autoescape by default for
.htmlfiles, which automatically escapes HTML special characters (<,>,&,",') in attribute values. The data attributes inedit.htmlandpost.htmlare already protected. Additionally:
index.htmldoes not contain the same video data attributes (the review claim is inaccurate)video.jsreads these values safely into variables and passes them to the Video.js API, not directly into the DOM- Applying
|tojsonwould be incorrect—it would add extra quotes and break the attribute syntax (e.g.,data-id=""test"")No changes are needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AmanatAliPanhwer
added
enhancement
|
|
The PiP button is not working |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
worker.py (1)
72-72: Missing timeout on requests.post call.Despite a previous review comment flagging this issue, the
requests.postcall still lacks a timeout parameter. This can cause the scheduler thread to hang indefinitely if the external service is slow or unresponsive.Apply this diff:
with open(video_file_path, 'rb') as video: - res = requests.post(f'https://ffmpeg.pythonanywhere.com/upload/{file_id}', files={'file': video}) + res = requests.post( + f'https://ffmpeg.pythonanywhere.com/upload/{file_id}', + files={'file': video}, + timeout=300 # 5 minutes for video processing + )
🧹 Nitpick comments (11)
static/css/video_controls.css (1)
90-103: Add keyboard-visible focus styles and avoid hover-only volume controlButtons and sliders remove outlines and rely on hover (
outline: none;, hover-only volume expansion). This makes focus hard to see and the volume slider hard to reach for keyboard-only or touch users.Consider:
- Adding
:focus-visiblestyles for.controls-bar button,.progress-bar, and.volume-sliderwith clear outlines/glows.- Making the volume slider discoverable without requiring
:hover(e.g., always a minimal width, larger on hover).Also applies to: 173-199
PROJECT_DESCRIPTION.md (1)
23-27: Fix unordered list indentation to satisfy markdownlint (MD007)Static analysis reports MD007 (ul-indent) on several list items in this file. The nested bullet items under “Video Upload and Streaming”, “Supabase”, and “JavaScript” are indented with 4 spaces instead of the expected 2.
To keep CI green and consistent with the rest of the doc, adjust those sub-items to use the same indentation style as other lists (typically 2 spaces under their parent
*).Also applies to: 41-43, 48-53
static/js/video_player.js (2)
107-127: Improve fullscreen compatibility with vendor-prefixed APIsThe current implementation uses only the standard
requestFullscreen()/exitFullscreen()pair, which works on modern browsers but fails on older ones. For cross-browser support, vendor-prefixed fallbacks are recommended for older browsers (webkit, moz, ms).Update
toggleFullscreen()to check for vendor-prefixed methods:function toggleFullscreen() { const request = container.requestFullscreen || container.webkitRequestFullscreen || container.mozRequestFullScreen || container.msRequestFullscreen; const exit = document.exitFullscreen || document.webkitExitFullscreen || document.mozCancelFullScreen || document.msExitFullscreen; if (!document.fullscreenElement && !document.webkitFullscreenElement) { request && request.call(container); } else if (exit) { exit.call(document); } }Also update
updateFullscreenButton()to check the vendor-prefixeddocument.webkitFullscreenElementwhen determining state.
214-275: Adding awindow.Hlsguard is not recommended by hls.js documentationThe current hls.js documentation recommends the pattern
if (Hls.isSupported())without an explicit guard for the global. While your concern about script load failures is valid from a defensive coding standpoint, the official hls.js pattern does not include this check.If script load failures are a concern for your application, adding
window.Hls &&is a reasonable defensive practice, but it's not required per the official documentation.tests/test_edit_post.py (1)
8-9: Extract timeout constant and reconsider excessive 600-second default across all testsPlaywright's recommended approach uses global defaults (30–120 seconds for E2E tests), but the codebase currently hard-codes
timeout=600000(10 minutes) across 123+ test locations, including fixtures inconftest.py. This pattern:
- Masks slow or hung operations (10 min before failure detection)
- Duplicates a magic literal throughout the suite
- Diverges from established best practices
Recommended actions:
- Define a shared constant (e.g.,
E2E_TIMEOUT = 600_000) inconftest.pyor a dedicated config module, and apply it throughout (test_edit_post.py,test_video.py,test_video_player.py,test_viewer.py,test_blog.py, and the existing fixtures).- Evaluate whether a lower timeout (60–120 seconds) is sufficient for non-video flows in
test_edit_post.py,test_viewer.py, andtest_blog.py; reserve the longer timeout only fortest_video.pyandtest_video_player.pywhere video processing genuinely requires it.- Consider using
browser_context.page.set_default_timeout()or Playwright test config to apply global timeouts consistently, then override per-call only for justified slow operations.tests/test_viewer.py (1)
10-107: Consider reducing the 600000ms timeout to a more reasonable value.The 10-minute timeout applied across all navigation and assertion operations is excessive for most test scenarios. Standard Playwright operations typically complete within 30-60 seconds. While video processing may require extended wait times, applying this uniformly could mask performance issues or allow tests to hang unnecessarily.
Consider:
- Using shorter timeouts (30000-60000ms) for basic navigation and UI assertions
- Reserving longer timeouts only for video processing-specific operations
- Using explicit polling loops with progress indicators for long-running operations
tests/conftest.py (1)
64-72: Excessive timeout for login/logout operations.Login and logout are fast operations that should complete within seconds. The 600000ms (10-minute) timeout is unnecessary and may allow tests to hang if authentication services are slow or unresponsive.
Recommend using 30000-60000ms for these operations to ensure tests fail fast.
tests/test_blog.py (1)
10-156: Reconsider the uniform 600000ms timeout across all test operations.While some operations (like video processing) may justify extended timeouts, standard blog operations such as page navigation, post creation, and UI interactions typically complete in seconds. Applying a 10-minute timeout uniformly:
- Masks performance degradation
- Delays test feedback when failures occur
- May hide flakiness or infrastructure issues
Recommend differentiating timeout values based on operation type.
tests/test_video_player.py (1)
7-15: Simplify the error message per static analysis guidance.The static analysis tool flags the long error message string. Consider defining a shorter message or using a custom exception class.
Apply this diff:
if not os.path.exists(source_filepath): - raise FileNotFoundError(f"Test video file not found at {source_filepath}. Please ensure it exists.") + raise FileNotFoundError(f"Test video not found: {source_filepath}")worker.py (1)
9-9: Consider using tempfile.mkdtemp() for temporary directory creation.The hardcoded
/tmppath is flagged as a potential security issue. While it may work for your deployment environment, using Python'stempfilemodule is more secure and cross-platform.+import tempfile + class Worker: def __init__(self, SUPABASE_KEY, SUPABASE_URL, videos_bucket='video'): - self.upload_folder = os.path.join("/tmp", 'uploads') + self.upload_folder = tempfile.mkdtemp(prefix='video_uploads_') os.makedirs(self.upload_folder, exist_ok=True)tests/test_video.py (1)
54-62: Extract duplicate polling logic to a helper function.The video processing polling logic appears twice in this file (lines 54-62 and lines 111-119) with nearly identical code. Extracting this pattern to a reusable helper would improve maintainability and reduce duplication.
Consider adding a helper function like this at the module level:
def poll_video_processing(page: Page, post_url: str, timeout_seconds: int = 120) -> bool: """ Polls the video player status on a post page until processing completes. Returns True if processing completed within timeout, False otherwise. """ for i in range(timeout_seconds): page.goto(post_url, timeout=600000) player = page.locator(".video-player") status = player.get_attribute("data-status") if status == "processed": return True time.sleep(1) print(f"Polling video status... Current: {status} (Attempt {i+1}/{timeout_seconds})") return FalseThen replace both polling blocks:
# In test_create_post_with_video (lines 54-64) processing_complete = poll_video_processing(page, post_url) assert processing_complete, "Video processing did not complete within the timeout period." # In test_edit_post_to_add_video (lines 111-121) processing_complete = poll_video_processing(page, f"{flask_app_url}/post/{post_id}") assert processing_complete, "Video processing did not complete within the timeout period after edit."Also applies to: 111-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
static/js/hls.min.jsis excluded by!**/*.min.jsstatic/svg/fullscreen-exit.svgis excluded by!**/*.svgstatic/svg/fullscreen.svgis excluded by!**/*.svgstatic/svg/gear.svgis excluded by!**/*.svgstatic/svg/pause.svgis excluded by!**/*.svgstatic/svg/pip.svgis excluded by!**/*.svgstatic/svg/play.svgis excluded by!**/*.svgstatic/svg/volume-mute.svgis excluded by!**/*.svgstatic/svg/volume-up.svgis excluded by!**/*.svgtests/assets/test_video.mp4is excluded by!**/*.mp4
📒 Files selected for processing (18)
.github/workflows/python-app.yml(2 hunks).gitignore(1 hunks)PROJECT_DESCRIPTION.md(3 hunks)static/css/style.css(4 hunks)static/css/video_controls.css(1 hunks)static/js/video_player.js(1 hunks)templates/components/video_player.html(1 hunks)templates/edit.html(2 hunks)templates/index.html(2 hunks)templates/layout.html(1 hunks)templates/post.html(3 hunks)tests/conftest.py(1 hunks)tests/test_blog.py(3 hunks)tests/test_edit_post.py(4 hunks)tests/test_video.py(1 hunks)tests/test_video_player.py(1 hunks)tests/test_viewer.py(6 hunks)worker.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- templates/index.html
- templates/edit.html
- .github/workflows/python-app.yml
🧰 Additional context used
🧬 Code graph analysis (5)
tests/test_blog.py (1)
tests/conftest.py (3)
page(54-59)flask_app_url(16-51)admin_logged_in_page(63-72)
tests/test_video.py (1)
tests/conftest.py (3)
admin_logged_in_page(63-72)flask_app_url(16-51)page(54-59)
tests/test_edit_post.py (1)
tests/conftest.py (3)
page(54-59)flask_app_url(16-51)admin_logged_in_page(63-72)
tests/test_video_player.py (1)
tests/conftest.py (3)
admin_logged_in_page(63-72)flask_app_url(16-51)page(54-59)
tests/test_viewer.py (2)
tests/conftest.py (2)
page(54-59)flask_app_url(16-51)static/js/script.js (1)
viewer(90-95)
🪛 markdownlint-cli2 (0.18.1)
PROJECT_DESCRIPTION.md
24-24: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
25-25: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
26-26: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
41-41: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
42-42: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Ruff (0.14.4)
tests/test_video.py
12-12: Avoid specifying long messages outside the exception class
(TRY003)
worker.py
9-9: Probable insecure usage of temporary file or directory: "/tmp"
(S108)
18-18: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Do not catch blind exception: Exception
(BLE001)
61-61: Avoid specifying long messages outside the exception class
(TRY003)
72-72: Probable use of requests call without timeout
(S113)
77-77: Abstract raise to an inner function
(TRY301)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
80-80: Do not catch blind exception: Exception
(BLE001)
82-82: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
82-82: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_video_player.py
13-13: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
templates/layout.html (1)
10-19: LGTM! Video player assets properly integrated.The addition of video-related CSS and JavaScript files to the base layout is correct and follows the existing pattern. The assets are loaded using
url_for, ensuring proper URL generation.static/css/style.css (2)
28-28: Good refactoring with CSS variables and flexbox.The introduction of the
--input-marginvariable and conversion of the form to flexbox layout improves maintainability and consistency.Also applies to: 144-146, 153-153
630-723: Comprehensive video player and uploader styling.The new styles for video player controls, quality menu, and drag-and-drop uploader are well-structured and consistent with the existing design system. Good use of CSS variables for theming.
tests/test_video_player.py (2)
43-56: Good polling pattern for video processing.The 2-minute polling loop with 1-second intervals is appropriate for waiting on asynchronous video processing. Clear progress messages help with debugging.
64-103: Comprehensive volume and mute testing.The test properly validates mute/unmute behavior, volume slider interactions, and icon state changes. Good use of JS property assertions.
worker.py (1)
69-82: Good error handling and status tracking.The try-except block properly:
- Updates status to 'processing' before starting
- Handles failures by setting status to 'failed'
- Cleans up uploaded files
- Re-raises with context
The fallback to original filepath when processing fails (line 76) ensures the video remains accessible even if transcoding fails.
templates/components/video_player.html (1)
1-59: LGTM! Video player component with all dependencies verified.The video player macro is well-designed with proper data attributes, accessibility labels, and comprehensive controls. All required SVG icon dependencies have been verified to exist in
static/svg/.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ffmpeg_microservice.md (1)
37-40: Optional: Address markdown linting issues.The documentation is excellent and comprehensive. For improved consistency with markdown standards, consider:
List indentation (MD007): Nested list items use 4-space indentation; markdownlint expects 2 spaces. Affects lines 37-40, 53, 55, 57-58, 69-70, and 82-83.
Missing code fence language (MD040): The
.envexample at line 102 should specify a language identifier:-``` +```env SUPABASE_URL=https://your-project-ref.supabase.co SUPABASE_SERVICE_ROLE_KEY=your-supabase-service-role-key BLOG_VIDEOS_BUCKET=blog_videosAlso applies to: 53-58, 69-70, 82-83, 102-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)ffmpeg_microservice.md(1 hunks)static/js/video_player.js(1 hunks)templates/post.html(3 hunks)tests/test_video.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_video.py (1)
tests/conftest.py (3)
admin_logged_in_page(63-72)flask_app_url(16-51)page(54-59)
🪛 markdownlint-cli2 (0.18.1)
ffmpeg_microservice.md
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
40-40: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
55-55: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
69-69: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
70-70: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
82-82: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
83-83: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.4)
tests/test_video.py
12-12: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
templates/post.html (1)
17-20: LGTM! Video rendering implementation looks solid.The conditional video player rendering is cleanly implemented. The macro import and invocation pattern follows best practices, and the logic correctly guards against rendering when
post.videois absent.static/js/video_player.js (1)
1-359: Excellent implementation of the video player!The video player implementation is comprehensive and robust:
- HLS support: Properly detects Hls.js availability, handles manifest parsing, quality switching, and gracefully falls back to native HLS (Safari) or direct sources.
- Error handling: Covers network and media errors with appropriate recovery strategies.
- Controls: Full set of playback controls (play/pause, seek, volume, fullscreen, quality, speed, PiP) with proper state management.
- UX: Auto-hide controls, hover interactions, and visual feedback enhance usability.
- PiP: The previously flagged null-dereference issue has been correctly resolved with proper feature detection.
tests/test_video.py (1)
6-134: Comprehensive end-to-end video tests!The test suite provides excellent coverage of video workflows:
- test_create_post_with_video: Validates the full lifecycle from upload through processing to final HLS playback, with appropriate polling and timeout handling.
- test_edit_post_to_add_video: Tests the edit path, ensuring videos can be added to existing posts and processed correctly.
- Polling strategy: The 120-second polling loops (lines 56-64, 113-122) are reasonable for async video processing and include helpful debug logging.
- Assertions: Clear, descriptive messages explain expected behavior at each step.
- Cleanup: Proper cleanup ensures test isolation.
All previously flagged issues (docstring, f-string quoting, assertion messages) have been resolved.
1 participant
📌 Summary
This pull request introduces a comprehensive video upload, processing, and streaming feature to the blog platform. Administrators can now attach video files to posts, which are then processed in the background and made available for streaming with adaptive quality selection.
This new functionality enhances the richness of the blog's content by allowing for multimedia posts. The implementation focuses on maintaining application responsiveness by offloading heavy transcoding tasks to a background worker and an external service.
🔧 Type of Change
🧩 Related Issues
Closes #10
🧠 Changes Introduced
worker.py: A new background worker module usingAPSchedulerto handle asynchronous video processing.app.py:workerto handle video uploads in the/newand/editroutes.videostable to store video metadata, includingfilename,filepath, and processingstatus.video_idforeign key to thepoststable.new.html&edit.html: Added a drag-and-drop UI for video uploads.index.html&post.html: Created a Video player to display the videos. The player is dynamically configured based on the video's processing status.layout.html: Added required dependencies for hls.js and the quality levels plugin.static/js/uploader.js: Enhanced to support the video upload UI.static/js/video.js: New script to initialise the Video player, handle HLS streaming for processed videos, and implement a dynamic quality selection menu.📸 Screenshots
1. New Video Upload Component in New Post & Editor:
Post:
Edit:
2. Video Player on a Blog Post:
✅ Checklist
🧾 Additional Notes
https://ffmpeg.pythonanywhere.com) for video transcoding.blog_videos, is required for this feature to function correctly.Summary by CodeRabbit
New Features
Style
Chores