Conversation
✅ Deploy Preview for viseron canceled.
|
There was a problem hiding this comment.
Pull request overview
This PR is a first draft of changes aimed at running Viseron as a Home Assistant add-on/app, with particular focus on Home Assistant Ingress/subpath compatibility across backend redirects/URL generation, frontend API base paths, and the nginx proxy layer.
Changes:
- Add Ingress-aware subpath handling in the Tornado request handler and use it for file redirects.
- Update nginx to forward the ingress header, inject a frontend
window.baseUrl, and tweak MJPEG proxying (disable buffering/gzip). - Add container init logic to symlink media directories into
/datafor simpler volume mapping; update frontend API client base path detection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| viseron/components/webserver/tiered_file_handler.py | Uses computed subpath when redirecting to /files/... to support ingress/subpaths. |
| viseron/components/webserver/request_handler.py | Adds ingress-header-aware get_subpath() and minor formatting changes. |
| rootfs/usr/local/nginx/conf/proxy.conf | Forwards X-Ingress-Path header to backend. |
| rootfs/usr/local/nginx/conf/nginx.conf | Injects window.baseUrl based on ingress header; disables buffering/gzip for MJPEG endpoints. |
| rootfs/etc/cont-init.d/10-adduser | Attempts to symlink media directories into /data for Home Assistant add-on volume usage. |
| frontend/src/lib/api/client.ts | Uses injected window.baseUrl (if present) for API base path selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Create symlink only if it doesn't already exist | ||
| if [ ! -L "/${folder}" ]; then | ||
| ln -s "/data/${folder}" "/${folder}" | ||
| fi |
There was a problem hiding this comment.
Symlink creation only checks -L and will fail for folders that already exist as real directories. /segments is created during image build (see docker/Dockerfile), so ln -s will error and leave /segments pointing at the container filesystem instead of /data/segments, undermining the “single volume mapping” goal. Handle the “directory exists” case (eg, migrate contents to /data/segments and replace it with a symlink, or skip with a clear log + ensure the app writes to /data).
| # Create symlink only if it doesn't already exist | |
| if [ ! -L "/${folder}" ]; then | |
| ln -s "/data/${folder}" "/${folder}" | |
| fi | |
| target="/${folder}" | |
| data_target="/data/${folder}" | |
| # If target is already a symlink, leave it as is | |
| if [ -L "${target}" ]; then | |
| continue | |
| fi | |
| # If target is an existing real directory, migrate contents then replace with a symlink | |
| if [ -d "${target}" ]; then | |
| if [ "$(ls -A "${target}" 2>/dev/null)" ]; then | |
| log_info "Migrating existing contents from ${target} to ${data_target}" | |
| mv "${target}"/* "${data_target}/" 2>/dev/null || : | |
| mv "${target}"/.[!.]* "${data_target}/" 2>/dev/null || : | |
| mv "${target}"/..?* "${data_target}/" 2>/dev/null || : | |
| fi | |
| if ! rmdir "${target}" 2>/dev/null; then | |
| log_warn "Could not remove existing directory ${target}; skipping symlink creation" | |
| continue | |
| fi | |
| elif [ -e "${target}" ]; then | |
| log_warn "Path ${target} exists and is not a directory or symlink; skipping symlink creation" | |
| continue | |
| fi | |
| ln -s "${data_target}" "${target}" |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((window as any).baseUrl) { | ||
| const base = (window as any).baseUrl as string; |
There was a problem hiding this comment.
Using (window as any).baseUrl bypasses type-safety. Consider adding a declare global { interface Window { baseUrl?: string } } in an appropriate .d.ts (eg vite-env.d.ts) and then using window.baseUrl here, so this stays typed and discoverable.
| if ((window as any).baseUrl) { | |
| const base = (window as any).baseUrl as string; | |
| const win = window as Window & { baseUrl?: string }; | |
| if (typeof win.baseUrl === "string" && win.baseUrl.length > 0) { | |
| const base = win.baseUrl; |
c47d3bc to
1b09097
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const prevSrcRef = useRef<string>(src); | ||
|
|
||
| // Reset state synchronously during render when src changes | ||
| if (prevSrcRef.current !== src) { | ||
| prevSrcRef.current = src; | ||
| if (error !== null) setError(null); | ||
| if (!isLoading) setIsLoading(true); | ||
| } |
There was a problem hiding this comment.
This hook calls setError/setIsLoading during render when src changes (lines 27-31). React state updates during render can trigger warnings (“cannot update a component while rendering”) and can cause unexpected re-render loops. Move this reset logic into an effect keyed on src (or derive state from src) instead of mutating state during render.
| const prevSrcRef = useRef<string>(src); | |
| // Reset state synchronously during render when src changes | |
| if (prevSrcRef.current !== src) { | |
| prevSrcRef.current = src; | |
| if (error !== null) setError(null); | |
| if (!isLoading) setIsLoading(true); | |
| } | |
| useEffect(() => { | |
| setError(null); | |
| setIsLoading(true); | |
| }, [src]); |
| firstFrame = false; | ||
| setIsLoading(false); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the stream ends/terminates without yielding any multipart parts, firstFrame stays true and isLoading is never set to false, leaving the UI spinner up indefinitely with no error. After the for await loop completes, ensure you set isLoading false (and preferably set an error) when no frames were received.
| } | |
| } | |
| if (!cancelled && firstFrame) { | |
| setError("MJPEG stream ended before any frames were received."); | |
| setIsLoading(false); | |
| } |
First draft of the needed changes to get Viseron running as a Home Assistant App
MJPEG streaming is not working yet, need to figure that one out still.
Closes #576