RTSP installer changes - #14
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 59 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 (30)
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 |
PR Summary by QodoConnector v1.1.12: safe installer updates, live preview, and camera disable flows
AI Description
Diagram
High-Level Assessment
Files changed (31)
|
Code Review by Qodo
1. Baked backend URL localhost
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -1,3 +1,3 @@ | |||
| # AUTO-GENERATED by installer/build.ps1 - do not edit by hand. | |||
| BAKED_BACKEND_URL = "http://20.193.69.220:8081" | |||
| BAKED_BACKEND_URL = "http://localhost:8081" | |||
There was a problem hiding this comment.
1. Baked backend url localhost 🐞 Bug ≡ Correctness
connector/app/baked_config.py now bakes http://localhost:8081, and connector/app/config.py prefers this baked value when CONNECTOR_BACKEND_URL is unset, so real installs will attempt to pair/heartbeat against the shop PC instead of the API. This breaks connector setup and ongoing operation unless every deployment explicitly overrides the backend URL.
Agent Prompt
## Issue description
The connector’s baked default backend URL was changed to `http://localhost:8081`, but `_default_backend_url()` uses this baked value whenever `CONNECTOR_BACKEND_URL` is not set. For production installers, this causes the connector to call the wrong backend by default.
## Issue Context
- `baked_config.py` is marked auto-generated and is used as the default backend URL.
- If the installer/build pipeline doesn’t overwrite this value during release builds, the shipped connector will fail to register/claim/heartbeat.
## Fix Focus Areas
- connector/app/baked_config.py[1-3]
- connector/app/config.py[50-57]
### Implementation guidance
- Restore the baked URL to the intended deployed API base URL (or ensure the build step always injects the correct value for the target environment).
- Add a build/CI guard (or a runtime startup check in release builds) to fail fast if `BAKED_BACKEND_URL` is `localhost` / `127.0.0.1` unless an explicit dev flag is set.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+101
| [HttpDelete("{id:guid}")] | ||
| [Authorize(Roles = "Admin,Manager,Installer")] | ||
| public async Task<IActionResult> Disable(Guid id) | ||
| { | ||
| var cam = await _db.Cameras.FindAsync(id); | ||
| if (cam is null) return NotFound(); | ||
| if (!TenantAccess.CanAccessStore(User, cam.StoreId)) return Forbid(); | ||
| cam.Status = CameraStatus.Disabled; | ||
| await _db.SaveChangesAsync(); | ||
| return Ok(new { ok = true, cameraId = cam.Id }); | ||
| } |
There was a problem hiding this comment.
2. Disabled cameras can’t re-enable 🐞 Bug ≡ Correctness
DELETE /api/cameras/{id} sets Status=Disabled, but camera provisioning/upsert does not reset Status
on sourceKey conflicts, so re-adding the same source later keeps the camera Disabled. Because
connector camera retrieval excludes Disabled cameras, the connector will never resume monitoring
that camera without a separate manual status fix.
Agent Prompt
## Issue description
A camera can be put into `CameraStatus.Disabled`, and the connector is then prevented from seeing it. However, when the same physical source is provisioned again (same `sourceKey`), the backend upsert path updates name/URLs but does not change `Status`, leaving the camera permanently disabled from the connector’s perspective.
## Issue Context
- Disabling a camera is implemented as a status change, not a delete.
- Connector camera fetch explicitly filters out `Disabled`.
- Provisioning uses `ON CONFLICT (ConnectorId, SourceKey) DO UPDATE` but does not update `Status`.
## Fix Focus Areas
- backend/Controllers/CamerasController.cs[91-115]
- backend/Services/CameraProvisioningService.cs[52-67]
- backend/Controllers/ConnectorsController.cs[213-216]
### Implementation guidance
Pick one consistent behavior:
1) **Re-enable on reprovision**: In `CameraProvisioningService.ProvisionConnectorCameraAsync`, update `Status` in the `DO UPDATE` clause *at least when the existing row is Disabled* (e.g., set to Pending/Active).
2) **Disable = detach**: In the disable endpoint, also clear `ConnectorId` and/or `SourceKey` so provisioning creates a fresh active row (only if that’s acceptable for historical continuity).
3) **Explicit enable**: Add an enable endpoint / UI flow that sets status back to Active/Pending and ensure connector can see it again.
Add a regression test for: disable camera → provision same `sourceKey` again → camera becomes visible to `/api/connectors/cameras`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+542
to
+543
| this.selectedCameraIds.clear(); | ||
| this.api.listCameras(id).subscribe((c) => (this.cameras = c.filter(x => x.status !== 'Disabled'))); |
There was a problem hiding this comment.
3. Disabled cameras still counted 🐞 Bug ≡ Correctness
SetupComponent now hides Disabled cameras client-side, but the backend camera list endpoint still returns Disabled cameras and GetStartedComponent counts cameras using cams.length, so onboarding/metrics can claim cameras are configured even when all are Disabled. This creates inconsistent UI state across pages after using the new disable flow.
Agent Prompt
## Issue description
The dashboard now treats `Disabled` cameras inconsistently:
- Setup page filters them out.
- Other pages (e.g., Get Started) still count them because the backend list API includes them and the UI uses `cams.length`.
This makes workflows and counts disagree after cameras are disabled.
## Issue Context
- The PR introduces a new Disabled status and UI flows that hide disabled cameras in some places.
- The backend `/api/cameras` list does not exclude Disabled.
## Fix Focus Areas
- dashboard/src/app/pages/setup/setup.component.ts[539-545]
- backend/Controllers/CamerasController.cs[25-35]
- dashboard/src/app/pages/get-started/get-started.component.ts[218-236]
- dashboard/src/app/pages/get-started/get-started.component.ts[332-336]
### Implementation guidance
Define a single policy and apply it consistently:
- Option A (recommended): Make `GET /api/cameras` exclude `Status == Disabled` by default (and add an `includeDisabled=true` query param for admin/history views).
- Option B: Keep backend as-is, but update all UI counts and lists (Get Started, overview counts, etc.) to filter out Disabled before computing cameraCount/done flags.
Add an e2e/unit test ensuring that after disabling the only camera in a store, Get Started does not mark the camera setup step as done.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
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.
changes done