Mobile fixes - #53
Open
dk307 wants to merge 3 commits into
Open
Conversation
- Live view fills remaining viewport height (object-contain, no crop) on desktop and mobile instead of a fixed 16:9 box. - Move full-height flex off TabsContent so Radix's hidden attr works; inactive tab panels no longer leave empty space at the top of other tabs. - deploy.sh resolves the venv python (fallback python3, PY override).
Live page: reactive viewport width with adaptive columns (1/2/3/4), manual layouts honored at any width, aspect-video tiles and gridAutoRows auto so overflowing cameras scroll instead of clipping. Live websocket: accept first then cold-boot idle-stopped go2rtc via asyncio.to_thread instead of surfacing an HTTP 403; surface non-1000 closes to the client retry path.
📝 WalkthroughWalkthroughThe pull request improves go2rtc WebSocket startup and failure handling, updates client reconnect behavior, adds responsive live-view layouts, adjusts camera detail sizing, selects the deployment Python interpreter, and expands deployment and integration test coverage. ChangesLive camera connectivity
Responsive live layout
Deployment interpreter selection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant live_ws
participant go2rtc
participant VideoStream
Client->>live_ws: Open WebSocket
live_ws->>go2rtc: Check availability
alt go2rtc unavailable
live_ws->>go2rtc: Start instance asynchronously
go2rtc-->>live_ws: Readiness result
end
alt go2rtc available
live_ws-->>Client: Proxy stream
else startup or availability failure
live_ws-->>Client: Close with code 1013
Client->>VideoStream: Process close event
VideoStream->>VideoStream: Schedule reconnect
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
- Coverage 92.89% 92.88% -0.02%
==========================================
Files 81 81
Lines 7377 7394 +17
Branches 751 753 +2
==========================================
+ Hits 6853 6868 +15
- Misses 490 492 +2
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
frontend/src/pages/Live.test.tsx (1)
159-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the resize listener without remounting.
Lines 167-173 validate a new initial viewport after remount. They do not validate the
resizelistener added inLive.tsx. Change the viewport while the first wall remains mounted. Then dispatchresizeand assert that the existing grid updates.Proposed test update
-import { screen, waitFor } from "`@testing-library/react`"; +import { fireEvent, screen, waitFor } from "`@testing-library/react`"; - const { container, unmount } = renderLive(); + const { container } = renderLive(); await waitFor(() => expect(screen.getAllByTestId("stream")).toHaveLength(3)); const grid = container.querySelector<HTMLElement>("[style*='grid-template-columns']")!; expect(grid.style.gridTemplateColumns).toBe("repeat(1, minmax(0, 1fr))"); - unmount(); setWidth(1100); // tablet - const { container: container2 } = renderLive(); - await waitFor(() => expect(screen.getAllByTestId("stream")).toHaveLength(3)); - const grid2 = container2.querySelector<HTMLElement>("[style*='grid-template-columns']")!; - expect(grid2.style.gridTemplateColumns).toBe("repeat(3, minmax(0, 1fr))"); + fireEvent(window, new Event("resize")); + await waitFor(() => + expect(grid.style.gridTemplateColumns).toBe("repeat(3, minmax(0, 1fr))"), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/Live.test.tsx` around lines 159 - 173, Update the auto-layout test in the existing viewport test case to keep the first render mounted, change its width from phone to tablet, dispatch a resize event, and assert that the same grid updates to three columns. Remove the unmount and second render flow so the test exercises Live’s resize listener rather than only initial layout calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/cameras.py`:
- Around line 570-576: Update the go2rtc startup path, including go2rtc.start
and its subprocess-spawn logic, to re-check _proc while holding _lock
immediately before calling subprocess.Popen(). Return or reuse the existing
process when another caller has already spawned it, ensuring concurrent starts
cannot create duplicate processes.
In `@README.md`:
- Around line 81-83: Update the README deployment description to replace the
incorrect `podman-compose up --build` reference with the actual
`scripts/deploy.sh` behavior: build the image with `podman build`, stop and
remove the existing container, then start it with `podman run`. Preserve the
surrounding local test, rsync, and health-verification details.
- Line 75: Update the deployment prerequisites in README.md to document the
DEPLOY_PASS authentication path, including that scripts/deploy.sh uses sshpass
for SSH and rsync and requires sshpass to be installed. Preserve the existing
SSH-key instructions; only remove the DEPLOY_PASS code path if the alternative
is intentionally not supported.
In `@scripts/deploy.sh`:
- Around line 13-14: Update the deployment script’s PY validation before the
pytest invocation to execute "$PY", verify the interpreter version is Python
3.14 or newer, and exit with a clear error for older or invalid interpreters.
Add the Python >=3.14 requirement to the deployment section of README.md.
In `@tests/integration/test_cameras_api.py`:
- Around line 941-960: The websocket test around client.websocket_connect must
capture the WebSocketDisconnect raised by ws.receive() and assert its code is
1013, rather than suppressing or broadly catching it. Preserve the existing
go2rtc availability/start patches and ensure the assertion validates the
server-side close contract.
---
Nitpick comments:
In `@frontend/src/pages/Live.test.tsx`:
- Around line 159-173: Update the auto-layout test in the existing viewport test
case to keep the first render mounted, change its width from phone to tablet,
dispatch a resize event, and assert that the same grid updates to three columns.
Remove the unmount and second render flow so the test exercises Live’s resize
listener rather than only initial layout calculation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf427b74-ce07-4f9b-959c-f96be52ba49a
📒 Files selected for processing (9)
README.mdapp/api/cameras.pyfrontend/src/components/VideoStream.test.tsxfrontend/src/components/VideoStream.tsxfrontend/src/pages/CameraDetail.tsxfrontend/src/pages/Live.test.tsxfrontend/src/pages/Live.tsxscripts/deploy.shtests/integration/test_cameras_api.py
go2rtc.start(): re-check the process handle under the lock before spawning so concurrent cold boots share one process instead of each spawning a duplicate. Assert the 1013 close contract in the live_ws integration tests. deploy.sh: gate on Python >=3.14 with a clear error. README: correct the podman-compose reference to podman build/stop/rm/run and document the sshpass/DEPLOY_PASS path. Live.test: exercise the resize listener instead of re-mounting.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/services/go2rtc.py`:
- Around line 227-232: Update the failed-start cleanup around
_clear_if_current() so readiness state is cleared only when the shared _proc
still references the failed spawned process. Preserve the replacement process
and its _api_ready state when another caller has installed it, while continuing
to terminate and reap the failed process as needed.
In `@tests/integration/test_cameras_api.py`:
- Around line 949-950: Update both tests in
tests/integration/test_cameras_api.py at lines 949-950 and 1012-1015: bind the
go2rtc.start patch to start_mock while retaining each test’s existing return or
raising behavior, then assert start_mock.assert_called_once_with() after
exercising live_ws. Ensure both cold-start paths verify exactly one start call.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 828f7afa-ea2c-4b96-83ef-b8853c081b16
📒 Files selected for processing (6)
README.mdapp/services/go2rtc.pyfrontend/src/pages/Live.test.tsxscripts/deploy.shtests/integration/test_cameras_api.pytests/unit/test_go2rtc.py
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- frontend/src/pages/Live.test.tsx
Comment on lines
+227
to
+232
| # The API never came up (or the process died while starting). Terminate a | ||
| # still-alive process so it releases its ports, then drop the handle so a | ||
| # later start() can retry instead of waiting on a wedged process. | ||
| if spawned.poll() is None: | ||
| _terminate_and_reap(spawned) | ||
| _clear_if_current(spawned) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep readiness state owned by the current process.
A failed starter can reach Line 232 after another caller has installed and marked a replacement process ready. _clear_if_current() then clears _api_ready even when _proc is not spawned. Later callers wait until timeout although the replacement process is available.
Clear _api_ready only when _proc is proc.
Proposed fix
def _clear_if_current(proc: subprocess.Popen) -> None:
"""Drop the process handle and readiness if *proc* is still the current one."""
global _proc
with _lock:
if _proc is proc:
_proc = None
- _api_ready.clear()
+ _api_ready.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/go2rtc.py` around lines 227 - 232, Update the failed-start
cleanup around _clear_if_current() so readiness state is cleared only when the
shared _proc still references the failed spawned process. Preserve the
replacement process and its _api_ready state when another caller has installed
it, while continuing to terminate and reap the failed process as needed.
Comment on lines
+949
to
+950
| patch("app.services.go2rtc.is_available", return_value=False), | ||
| patch("app.services.go2rtc.start", return_value=False), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert each cold-start call.
Both tests can pass if live_ws skips go2rtc.start() and closes immediately because is_available() remains False. Bind each patch to a mock and assert start_mock.assert_called_once_with().
tests/integration/test_cameras_api.py#L949-L950: retain the failed-start mock and assert one cold-start call.tests/integration/test_cameras_api.py#L1012-L1015: retain the raising-start mock and assert one cold-start call.
📍 Affects 1 file
tests/integration/test_cameras_api.py#L949-L950(this comment)tests/integration/test_cameras_api.py#L1012-L1015
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_cameras_api.py` around lines 949 - 950, Update both
tests in tests/integration/test_cameras_api.py at lines 949-950 and 1012-1015:
bind the go2rtc.start patch to start_mock while retaining each test’s existing
return or raising behavior, then assert start_mock.assert_called_once_with()
after exercising live_ws. Ensure both cold-start paths verify exactly one start
call.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation