Allow Nginx to serve Video Vue static assets (opt-in) #1554
+14
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMakes serving of Video Vue static assets (JS/CSS/map) by Django conditional on a new VIDEO_STATIC_NGINX_SERVE setting, so that when enabled, Nginx is expected to serve those assets while Django continues to serve index.html and handle SPA routing. Sequence diagram for Video Vue asset request when VIDEO_STATIC_NGINX_SERVE is disabledsequenceDiagram
actor Browser
participant Nginx
participant Django
participant VideoAssetView
Browser->>Nginx: GET /video/app.js
Nginx->>Django: Proxy /video/app.js
Django->>VideoAssetView: Dispatch get(path="app.js")
VideoAssetView->>VideoAssetView: VIDEO_STATIC_NGINX_SERVE == False
VideoAssetView-->>Django: Return asset content (200)
Django-->>Nginx: HTTP 200 JS content
Nginx-->>Browser: HTTP 200 JS content
Sequence diagram for Video Vue asset request when VIDEO_STATIC_NGINX_SERVE is enabledsequenceDiagram
actor Browser
participant Nginx
participant Django
participant VideoAssetView
participant NginxStatic
Browser->>Nginx: GET /video/app.js
Nginx->>Django: Proxy /video/app.js
Django->>VideoAssetView: Dispatch get(path="app.js")
VideoAssetView->>VideoAssetView: VIDEO_STATIC_NGINX_SERVE == True
VideoAssetView->>VideoAssetView: ext in VIDEO_STATIC_NGINX_EXTENSIONS
VideoAssetView-->>Django: Raise Http404
Django-->>Nginx: HTTP 404
Nginx->>NginxStatic: Try serve /video/app.js from static
NginxStatic-->>Nginx: JS file content
Nginx-->>Browser: HTTP 200 JS content
Class diagram for VideoAssetView and related settingsclassDiagram
class DjangoSettings {
bool VIDEO_STATIC_NGINX_SERVE
}
class VideoModuleConstants {
Path WEBAPP_DIST_DIR
tuple VIDEO_STATIC_NGINX_EXTENSIONS
}
class VideoAssetView {
+get(request, path, *args, **kwargs)
}
DjangoSettings <.. VideoAssetView : reads
VideoModuleConstants <.. VideoAssetView : uses
VideoModuleConstants : VIDEO_STATIC_NGINX_EXTENSIONS = (.js, .css, .map)
DjangoSettings : VIDEO_STATIC_NGINX_SERVE loaded from config[video.nginx_serve_static] with fallback False
VideoAssetView : get checks VIDEO_STATIC_NGINX_SERVE and path extension to decide 404 or serve from WEBAPP_DIST_DIR
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
VideoAssetViewlogic usesos.path.splitextbutosis not imported in this file in the diff; double‑check thatimport osis present or add it near the other imports to avoid a NameError. - Consider extracting the
('.js', '.css', '.map')extension tuple into a named constant or setting so that it can be shared with the Nginx configuration and easily updated if the set of Video Vue static asset types changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `VideoAssetView` logic uses `os.path.splitext` but `os` is not imported in this file in the diff; double‑check that `import os` is present or add it near the other imports to avoid a NameError.
- Consider extracting the `('.js', '.css', '.map')` extension tuple into a named constant or setting so that it can be shared with the Nginx configuration and easily updated if the set of Video Vue static asset types changes.
## Individual Comments
### Comment 1
<location> `app/eventyay/multidomain/views.py:133-135` </location>
<code_context>
class VideoAssetView(View):
def get(self, request, path='', *args, **kwargs):
+ # When VIDEO_STATIC_NGINX_SERVE is enabled, skip serving static assets
+ if getattr(settings, 'VIDEO_STATIC_NGINX_SERVE', False) and path:
+ _, ext = os.path.splitext(path.lower())
+ if ext in ('.js', '.css', '.map'):
+ raise Http404()
# Accept empty path -> index handling done by SPA view
</code_context>
<issue_to_address>
**suggestion:** Consider centralizing the list of extensions to be Nginx-served in settings or a constant to avoid future drift.
The extensions are currently hard-coded in the view, so any change to the Nginx-served types (e.g., adding fonts or images) would require a code change here. Exposing this tuple via settings or a shared module-level constant would keep it configurable and reduce the risk of this view falling out of sync with Nginx.
Suggested implementation:
```python
# File extensions that are expected to be served directly by Nginx when
# VIDEO_STATIC_NGINX_SERVE is enabled. Keep this in sync with the Nginx config.
VIDEO_STATIC_NGINX_EXTENSIONS = getattr(
settings,
'VIDEO_STATIC_NGINX_EXTENSIONS',
('.js', '.css', '.map'),
)
class VideoAssetView(View):
```
```python
# When VIDEO_STATIC_NGINX_SERVE is enabled, skip serving static assets
if getattr(settings, 'VIDEO_STATIC_NGINX_SERVE', False) and path:
_, ext = os.path.splitext(path.lower())
if ext in VIDEO_STATIC_NGINX_EXTENSIONS:
raise Http404()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MukundC25
force-pushed
the
fix/video-static-nginx
branch
from
b183823 to
1a7cc54
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider using a set instead of a tuple for VIDEO_STATIC_NGINX_EXTENSIONS to make membership checks slightly more efficient and self-descriptive for this lookup use case.
- You might want to centralize the VIDEO_STATIC_NGINX_SERVE flag usage by referencing settings.VIDEO_STATIC_NGINX_SERVE directly (instead of getattr) to make it clearer that the setting is always defined and reduce defensive code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a set instead of a tuple for VIDEO_STATIC_NGINX_EXTENSIONS to make membership checks slightly more efficient and self-descriptive for this lookup use case.
- You might want to centralize the VIDEO_STATIC_NGINX_SERVE flag usage by referencing settings.VIDEO_STATIC_NGINX_SERVE directly (instead of getattr) to make it clearer that the setting is always defined and reduce defensive code.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
MukundC25
force-pushed
the
fix/video-static-nginx
branch
from
1a7cc54 to
2f0e48d
Compare
Author
|
Addressed the latest Sourcery suggestions:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in configuration setting to allow Nginx to serve Video Vue static assets (JS, CSS, and source maps) instead of Django, improving performance in production deployments. The default behavior remains unchanged, ensuring backward compatibility.
Key Changes:
- Added
VIDEO_STATIC_NGINX_SERVEconfiguration setting (default: False) - Modified
VideoAssetViewto return 404 for static assets when the setting is enabled, allowing Nginx to handle them - Django continues to serve
index.htmland handle SPA routing regardless of the setting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
app/eventyay/config/settings.py |
Adds the VIDEO_STATIC_NGINX_SERVE configuration setting from the [video] config section with a safe default of False |
app/eventyay/multidomain/views.py |
Updates VideoAssetView to skip serving .js, .css, and .map files when the new setting is enabled, delegating to Nginx instead |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #1553
What does this PR do?
This PR makes Django stop serving Video Vue app static assets (JS/CSS/maps)
only when explicitly configured, allowing Nginx to serve them instead.
The default behavior remains unchanged.
Why?
Currently, Video Vue static files are served through Django, which is not
ideal in production where Nginx is already in front.
This change allows Nginx to serve those files without breaking:
What does NOT change?
How to enable
Add to your config file (e.g.,
eventyay.cfg):