Use git tags for versioning via setuptools-scm - #47
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe project now uses dynamic SCM-based versioning with a fallback. Docker builds pass the generated version to the runtime. go2rtc starts on demand and stops after stream inactivity. Dashboard polling stops when operations are idle. ChangesDynamic versioning
go2rtc stream lifecycle
Conditional dashboard polling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CameraWebSocket
participant cameras_proxy
participant go2rtc_service
participant go2rtc_process
CameraWebSocket->>cameras_proxy: Open live stream
cameras_proxy->>go2rtc_service: stream_started()
go2rtc_service->>go2rtc_process: Start or reuse process
CameraWebSocket->>cameras_proxy: Close live stream
cameras_proxy->>go2rtc_service: stream_ended()
go2rtc_service->>go2rtc_process: Schedule idle shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 #47 +/- ##
==========================================
+ Coverage 92.97% 93.01% +0.03%
==========================================
Files 81 81
Lines 7191 7239 +48
Branches 718 724 +6
==========================================
+ Hits 6686 6733 +47
- Misses 472 473 +1
Partials 33 33
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: 1
🧹 Nitpick comments (1)
docker/Dockerfile (1)
39-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winScope the pretend-version override to
camera-event-manager.
SETUPTOOLS_SCM_PRETEND_VERSIONis global, and it is set beforepip install ".[prod]". Source-built dependencies that also use setuptools-scm can inherit theAPP_VERSIONvalue as their own version. UseSETUPTOOLS_SCM_PRETEND_VERSION_FOR_CAMERA_EVENT_MANAGERfor this distribution, or unset the global env var before installing dependencies.🤖 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 `@docker/Dockerfile` around lines 39 - 40, Update the Dockerfile environment setup around APP_VERSION so the setuptools-scm pretend-version override is scoped specifically to camera-event-manager, using the distribution-specific variable rather than global SETUPTOOLS_SCM_PRETEND_VERSION before pip install.Source: Coding guidelines
🤖 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 `@docker/Dockerfile`:
- Around line 39-40: Update the APP_VERSION default near the Dockerfile’s
SETUPTOOLS_SCM_PRETEND_VERSION assignment to a valid PEP 440 version, preferably
0.0.0, so the image build does not override pyproject.toml’s fallback with
“unknown”.
---
Nitpick comments:
In `@docker/Dockerfile`:
- Around line 39-40: Update the Dockerfile environment setup around APP_VERSION
so the setuptools-scm pretend-version override is scoped specifically to
camera-event-manager, using the distribution-specific variable rather than
global SETUPTOOLS_SCM_PRETEND_VERSION before pip install.
🪄 Autofix (Beta)
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: eafabe52-69d8-4795-9eb7-11ed1bde903c
📒 Files selected for processing (2)
docker/Dockerfilepyproject.toml
…y-starting go2rtc - Dashboard: dynamic refetchInterval stops polling when no operations running - go2rtc: lazy-start (removed from lifespan) + idle-stop after 60s with no streams - Wire stream_started/stream_ended lifecycle in WS proxy - Add comprehensive tests for all new behavior
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/api/cameras.py (1)
563-608: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCall
go2rtc.stream_started()inside thetryblock to avoid leaking the session and the stream counter.
go2rtc.stream_started()runs at Line 564, before thetryblock starts at Line 565. Ifstream_started()raises, the exception skips thefinallyblock entirely.start()'s call to_write_config()sits outsidestart()'s owntry/except OSError, so a file-write failure there propagates all the way up throughstream_started().Two things leak in that case:
session(created at Line 563) never reachessession.close()at Line 608, leaking theaiohttp.ClientSession.go2rtc.stream_ended()at Line 607 never runs, so_active_streamsstays incremented forever and the idle-stop timer never fires for this stream.Move
go2rtc.stream_started()inside thetryblock sofinallyalways closes the session and always callsstream_ended(), keeping the counter balanced regardless of howstream_started()behaves.🐛 Proposed fix
session = aiohttp.ClientSession() - go2rtc.stream_started() try: + go2rtc.stream_started() async with session.ws_connect(upstream_url) as upstream:🤖 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/api/cameras.py` around lines 563 - 608, Move go2rtc.stream_started() from before the try statement into the beginning of that try block, while keeping session creation before it. Preserve the existing finally block so session.close() and go2rtc.stream_ended() execute whenever stream_started() raises or the proxy flow fails.
🧹 Nitpick comments (2)
tests/unit/test_go2rtc.py (1)
493-515: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct test for
_idle_stop()'s termination path.Current tests cover
stream_started(),stream_ended(), and timer scheduling, but no test calls_idle_stop()directly. Add a test that sets_active_streams = 0, sets_procto a fake process, callsgo2rtc._idle_stop()directly, and assertsfake.terminate()runs andgo2rtc._procbecomesNone. Add a second test for the zombie-reap branch, similar totest_stop_kills_process_that_ignores_terminateat Lines 135-145.🤖 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/unit/test_go2rtc.py` around lines 493 - 515, Add direct coverage for go2rtc._idle_stop(): with _active_streams set to zero and a fake _proc, assert the normal termination path calls terminate() and clears _proc. Add a second test covering the zombie-reap path, mirroring the process behavior and assertions in test_stop_kills_process_that_ignores_terminate.tests/integration/test_cameras_api.py (1)
814-830: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the leak when
stream_started()raises.This test confirms
stream_started()andstream_ended()each run once on a normal connection. Add a test whereapp.services.go2rtc.stream_startedraises, then assertapp.services.go2rtc.stream_endedstill runs and the fake session'sclose()still runs. Against the currentapp/api/cameras.pycode, this test fails becausestream_started()runs outside thetryblock. This test will confirm the fix oncestream_started()moves inside thetryblock.🤖 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 814 - 830, Add a regression test alongside test_live_ws_calls_stream_lifecycle where go2rtc.stream_started raises, then assert the exception path still invokes go2rtc.stream_ended exactly once and the fake aiohttp session’s close() exactly once. Configure the existing _fake_session_cls and websocket setup to observe close(), preserving the normal lifecycle test unchanged.
🤖 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 @.github/workflows/ci.yml:
- Around line 153-156: Update the E2E build’s BUILD_TIME argument in the
workflow build-args block to use a timestamp available during pull_request runs,
such as checkout metadata or github.run_started_at, instead of
github.event.head_commit.timestamp; preserve the existing APP_VERSION and
GIT_SHA arguments.
In `@app/services/go2rtc.py`:
- Around line 154-180: Extract the duplicated terminate/kill/reap sequence into
a shared _terminate_and_reap() helper. Update both stop() and _idle_stop() to
acquire _lock only long enough to swap _proc to None, then perform the blocking
process termination and waits outside the lock; preserve the existing
graceful-terminate, timeout-kill, and unreaped-process warning behavior.
In `@frontend/src/pages/Dashboard.test.tsx`:
- Around line 193-196: Update the initial counter setup in the Dashboard test to
wait until the scanner, download, and purge status request counters have each
reached their expected initial values before assigning initialScan, initialDl,
and initialPurge. Keep the existing counter snapshots unchanged after this
combined wait.
---
Outside diff comments:
In `@app/api/cameras.py`:
- Around line 563-608: Move go2rtc.stream_started() from before the try
statement into the beginning of that try block, while keeping session creation
before it. Preserve the existing finally block so session.close() and
go2rtc.stream_ended() execute whenever stream_started() raises or the proxy flow
fails.
---
Nitpick comments:
In `@tests/integration/test_cameras_api.py`:
- Around line 814-830: Add a regression test alongside
test_live_ws_calls_stream_lifecycle where go2rtc.stream_started raises, then
assert the exception path still invokes go2rtc.stream_ended exactly once and the
fake aiohttp session’s close() exactly once. Configure the existing
_fake_session_cls and websocket setup to observe close(), preserving the normal
lifecycle test unchanged.
In `@tests/unit/test_go2rtc.py`:
- Around line 493-515: Add direct coverage for go2rtc._idle_stop(): with
_active_streams set to zero and a fake _proc, assert the normal termination path
calls terminate() and clears _proc. Add a second test covering the zombie-reap
path, mirroring the process behavior and assertions in
test_stop_kills_process_that_ignores_terminate.
🪄 Autofix (Beta)
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: 88954b38-3fce-4ce5-9095-4e4797a2c977
📒 Files selected for processing (10)
.github/workflows/ci.ymlapp/api/cameras.pyapp/main.pyapp/services/go2rtc.pydocker/Dockerfilefrontend/src/pages/Dashboard.test.tsxfrontend/src/pages/Dashboard.tsxtests/integration/test_cameras_api.pytests/unit/test_go2rtc.pytests/unit/test_main.py
💤 Files with no reviewable changes (1)
- app/main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docker/Dockerfile
camera_streams endpoint now calls go2rtc.start() before checking availability, ensuring go2rtc is launched on-demand when a user visits a camera's live view (E2E test fix).
- ci.yml: use github.run_started_at for BUILD_TIME (available in all event types) - go2rtc.py: extract _terminate_and_reap() helper, release _lock before blocking waits - cameras.py: move stream_started() inside try block for proper cleanup on error - Dashboard.test.tsx: wait for all three counters before snapshotting initial values - Add test for stream_started() error still calling stream_ended/session.close - Add _idle_stop() unit tests (normal terminate + zombie-reap paths)
1 participant
Summary by CodeRabbit