api fixes - #12
Merged
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 57 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 (47)
📝 WalkthroughWalkthroughThe PR adds backend service authentication and management APIs, connector admin telemetry and hardening, dashboard analytics/reporting/settings and bulk actions, production configuration templates, and a comprehensive system audit plan. ChangesONEVO platform integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Dashboard
participant Backend
participant Database
User->>Dashboard: Select alerts or clips
Dashboard->>Backend: POST bulk-delete
Backend->>Database: Resolve scoped records and delete eligible entities
Database-->>Backend: Deletion counts
Backend-->>Dashboard: BulkDeleteResponse
Dashboard-->>User: Refresh list and show result
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
PR Summary by QodoHarden API auth/tenant scoping and add analytics/logs + bulk delete UX
AI Description
Diagram
High-Level Assessment
Files changed (47)
|
Code Review by Qodo
1. Cloud-AI key not dedicated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+123
to
+140
| // ---- Production secret validation ---- | ||
| const string DefaultJwtKey = "dev-super-secret-signing-key-change-me-please-32+"; | ||
| if (!builder.Environment.IsDevelopment()) | ||
| { | ||
| if (jwtOpts.SigningKey == DefaultJwtKey || jwtOpts.SigningKey.Length < 32) | ||
| throw new InvalidOperationException( | ||
| "Jwt:SigningKey must be set to a strong value (>= 32 chars) in Production."); | ||
|
|
||
| var bootstrap = ServiceAuth.ConnectorBootstrapKey(cfg); | ||
| if (bootstrap == ServiceAuth.DefaultBootstrapKey) | ||
| throw new InvalidOperationException( | ||
| "Seed:ConnectorBootstrapKey must be changed from the default in Production."); | ||
|
|
||
| var cloudAiKey = ServiceAuth.CloudAiServiceKey(cfg); | ||
| if (cloudAiKey == ServiceAuth.DefaultBootstrapKey) | ||
| throw new InvalidOperationException( | ||
| "CloudAi:ServiceKey must be set to a dedicated value in Production."); | ||
| } |
There was a problem hiding this comment.
1. Cloud-ai key not dedicated 🐞 Bug ⛨ Security
Production startup validation does not enforce CloudAi:ServiceKey being different from the connector bootstrap key, and ServiceAuth.CloudAiServiceKey falls back to the connector key when unset. This defeats the intended trust-boundary separation and allows the same credential to authorize both cloud-ai service calls and connector registration.
Agent Prompt
### Issue description
`ServiceAuth.CloudAiServiceKey()` falls back to `ConnectorBootstrapKey()` when `CloudAi:ServiceKey` is not set, but `Program.cs` only rejects the cloud-ai key when it equals the *default* bootstrap literal. In Production, this allows a configuration where both systems share a single (non-default) key.
### Issue Context
- The code comments and `.env.example` indicate cloud-ai should have a dedicated service key.
- `docker-compose.yml` also defaults `CloudAi__ServiceKey` to `${CONNECTOR_BOOTSTRAP_KEY}`, reinforcing the shared-key path.
### Fix Focus Areas
- backend/Program.cs[123-140]
- backend/Auth/ServiceAuth.cs[9-16]
- docker-compose.yml[82-86]
### Concrete fix
- In non-development environments, fail startup when `CloudAi:ServiceKey` is missing/blank.
- Additionally, fail when `CloudAi:ServiceKey` equals `Seed:ConnectorBootstrapKey` (resolved), not just when it equals `DefaultBootstrapKey`.
- Consider removing the `?? ConnectorBootstrapKey(cfg)` fallback entirely (or keep it only for Development).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+90
| from .network_util import admin_public_host | ||
|
|
||
| admin_host = admin_public_host() | ||
| while not stop.is_set(): | ||
| free = disk_free_pct(cfg.state_dir) | ||
| state.disk_free_pct = free | ||
| degraded = None | ||
| operational = None | ||
| if free < cfg.disk_critical_pct: | ||
| degraded = f"disk_critical:{free:.1f}%" | ||
| operational = f"disk_critical:{free:.1f}%" | ||
| elif free < cfg.disk_warn_pct: | ||
| degraded = f"disk_warning:{free:.1f}%" | ||
| operational = f"disk_warning:{free:.1f}%" | ||
| if store.pending_count() > 50: | ||
| degraded = (degraded + ";" if degraded else "") + "queue_backlog" | ||
| state.degraded_reason = degraded | ||
| operational = (operational + ";" if operational else "") + "queue_backlog" | ||
|
|
||
| with state._lock: | ||
| existing = state.degraded_reason | ||
| if existing and ( | ||
| existing.startswith("Setup") | ||
| or "activation" in existing.lower() | ||
| or "pending" in existing.lower() | ||
| ): | ||
| degraded = f"{existing};{operational}" if operational else existing | ||
| else: | ||
| degraded = operational | ||
| state.degraded_reason = operational | ||
|
|
||
| try: | ||
| client.heartbeat(free, store.pending_count(), degraded, cfg.version) | ||
| client.heartbeat( | ||
| free, store.pending_count(), degraded, cfg.version, | ||
| admin_host=admin_host, admin_port=cfg.admin_port, | ||
| ) |
There was a problem hiding this comment.
3. Heartbeat reports unreachable admin 🐞 Bug ≡ Correctness
The connector heartbeat always reports an inferred LAN IP as AdminHost even when the admin UI binds to 127.0.0.1 by default. The dashboard then uses this AdminHost/AdminPort to build snapshot URLs, which will fail for remote dashboards and stores potentially misleading network metadata in the backend.
Agent Prompt
### Issue description
`run_heartbeat()` reports `admin_host = admin_public_host()` unconditionally, but the admin server commonly binds to `127.0.0.1`. This makes the backend/dash believe the admin is reachable at a LAN IP when it isn't.
### Issue Context
- Admin bind host defaults to loopback.
- Dashboard reads `Connector.adminHost/adminPort` and constructs snapshot URLs.
### Fix Focus Areas
- connector/app/workers.py[58-94]
- connector/app/network_util.py[8-19]
- connector/app/config.py[119-134]
- dashboard/src/app/pages/setup/setup.component.ts[485-495]
### Concrete fix
- Only include `adminHost/adminPort` in heartbeat when `cfg.admin_bind_host` is non-loopback (or when an explicit `CONNECTOR_ADMIN_PUBLIC_HOST` is set).
- Otherwise send `null`/omit those fields so the dashboard doesn’t attempt remote access.
- Optionally, include a separate boolean like `adminReachableLan=true/false` to help UI messaging.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+21
to
+25
| { | ||
| var camera = await AuthorizeCameraAsync(cameraId); | ||
| if (camera is null) return Forbid(); | ||
| return Ok(await _db.CameraZones.Where(z => z.CameraId == cameraId).ToListAsync()); | ||
| } |
There was a problem hiding this comment.
4. Zones 403 on missing 🐞 Bug ≡ Correctness
ZonesController now returns Forbid() when the camera does not exist because AuthorizeCameraAsync returns null for both missing and unauthorized cameras. This changes API semantics and prevents clients from distinguishing invalid camera IDs from cross-tenant access denial.
Agent Prompt
### Issue description
`AuthorizeCameraAsync` returns `null` for missing cameras, and callers map `null` to `Forbid()`. That makes missing resources indistinguishable from authorization failures.
### Issue Context
Other areas in this repo explicitly preserve the NotFound vs Forbid distinction for cameras.
### Fix Focus Areas
- backend/Controllers/ZonesController.cs[19-25]
- backend/Controllers/ZonesController.cs[75-80]
### Concrete fix
- Change `AuthorizeCameraAsync` to return a tri-state result (e.g., `(Camera? camera, bool exists)` or an enum) so callers can return:
- `NotFound()` when the camera does not exist
- `Forbid()` when the camera exists but is not in an accessible store
- Apply consistently to List/Create/Update/Delete.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+11
to
+18
| try | ||
| { | ||
| await db.Database.MigrateAsync(); | ||
| } | ||
| catch | ||
| { | ||
| await db.Database.EnsureCreatedAsync(); | ||
| } |
There was a problem hiding this comment.
5. Seeder masks migration failure 🐞 Bug ☼ Reliability
DbSeeder catches all exceptions from MigrateAsync and silently falls back to EnsureCreatedAsync, hiding real migration/schema failures (especially in production). This can leave the DB in an incompatible state while startup continues, causing harder-to-diagnose runtime errors later.
Agent Prompt
### Issue description
The current seeding path suppresses any `MigrateAsync()` failure and attempts `EnsureCreatedAsync()` without logging. `EnsureCreatedAsync()` is not a safe fallback for a database that should be managed by migrations.
### Issue Context
`Program.cs` already retries `DbSeeder.SeedAsync` and logs readiness failures, but this catch prevents the real migration exception from surfacing.
### Fix Focus Areas
- backend/Data/DbSeeder.cs[11-18]
- backend/Program.cs[144-158]
### Concrete fix
- Catch only the specific legacy case you want to support (e.g., missing migrations history) and log the exception details.
- In Production, do not fall back to `EnsureCreatedAsync()`; log and rethrow so deployment fails fast.
- If supporting legacy EnsureCreated databases, implement an explicit migration bootstrap path (detect history table, then create it / baseline) rather than a blanket catch-all.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+56
| var alertRows = await alerts.AsNoTracking().ToListAsync(); | ||
| var clipCount = await clips.CountAsync(); | ||
| var analyzedClips = await clips.CountAsync(c => c.Status == ClipStatus.Analyzed); | ||
|
|
||
| return Ok(new AnalyticsSummaryResponse( | ||
| TotalAlerts: alertRows.Count, | ||
| PendingAlerts: alertRows.Count(a => a.Status == AlertStatus.PendingReview), | ||
| HighRiskAlerts: alertRows.Count(a => a.RiskLevel == RiskLevel.High), | ||
| MediumRiskAlerts: alertRows.Count(a => a.RiskLevel == RiskLevel.Medium), | ||
| FalsePositives: alertRows.Count(a => a.Status == AlertStatus.FalsePositive), | ||
| TotalClips: clipCount, | ||
| AnalyzedClips: analyzedClips, | ||
| AlertsByType: alertRows | ||
| .GroupBy(a => a.AlertType) | ||
| .ToDictionary(g => g.Key, g => g.Count()) | ||
| )); |
There was a problem hiding this comment.
6. Analytics loads alerts in-memory 🐞 Bug ➹ Performance
AnalyticsController materializes all scoped alerts with ToListAsync and computes counts/grouping in-process. This will become slow and memory-heavy as alert history grows, even though all metrics can be computed via SQL aggregates.
Agent Prompt
### Issue description
The summary endpoint loads all matching `Alert` rows into memory to compute totals and groupings, which scales poorly.
### Issue Context
This endpoint is intended for an ongoing dashboard view and will grow with time.
### Fix Focus Areas
- backend/Controllers/AnalyticsController.cs[26-56]
### Concrete fix
- Replace `ToListAsync` with EF aggregate queries:
- `TotalAlerts = await alerts.CountAsync()`
- `PendingAlerts = await alerts.CountAsync(a => a.Status == ...)`
- `AlertsByType = await alerts.GroupBy(a => a.AlertType).Select(g => new { ... }).ToDictionaryAsync(...)`
- Consider running counts concurrently (multiple tasks) if the DB can handle it.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+276
to
+297
| var deleted = 0; | ||
| var skipped = 0; | ||
| var errors = new List<string>(); | ||
|
|
||
| foreach (var id in targetIds.Distinct()) | ||
| { | ||
| var result = await TryDeleteClipAsync(id); | ||
| switch (result) | ||
| { | ||
| case DeleteClipResult.Deleted: | ||
| deleted++; | ||
| break; | ||
| case DeleteClipResult.SkippedConfirmed: | ||
| skipped++; | ||
| break; | ||
| case DeleteClipResult.NotFound: | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| return Ok(new BulkDeleteResponse(deleted, skipped, errors)); | ||
| } |
There was a problem hiding this comment.
7. Bulk clip delete hides failures 🐞 Bug ☼ Reliability
ClipsController bulk-delete returns an errors array but never populates it and does not isolate per-clip exceptions, so a single S3/DB failure can abort the whole request with a 500. The response also omits any accounting for NotFound clip IDs, making bulk results hard to interpret.
Agent Prompt
### Issue description
Bulk delete currently does not report which clip IDs failed and can fail the entire operation on the first exception.
### Issue Context
`TryDeleteClipAsync` performs multiple external operations (S3 delete + DB changes) that can throw.
### Fix Focus Areas
- backend/Controllers/ClipsController.cs[260-297]
- backend/Controllers/ClipsController.cs[320-354]
### Concrete fix
- Wrap each `TryDeleteClipAsync(id)` call in try/catch; on exception append a human-readable entry to `errors` and continue.
- Track and return an explicit `notFound` count (or include a `missingIds` list) to make results deterministic.
- Optionally use a transaction for DB deletes per clip (keeping S3 deletion best-effort) and/or add a cancellation token to avoid long-running requests.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+261
to
+265
| this.route.queryParamMap.subscribe((params) => { | ||
| const fromQuery = params.get('storeId'); | ||
| if (fromQuery) this.storeId = fromQuery; | ||
| this.load(); | ||
| }); |
There was a problem hiding this comment.
8. Route subscriptions not disposed 🐞 Bug ⚙ Maintainability
AlertsComponent and ClipsComponent subscribe to ActivatedRoute.queryParamMap without disposing the subscription, and ClipsComponent has no OnDestroy hook. Depending on component lifecycle/reuse, this can retain component instances longer than intended and trigger redundant load() calls.
Agent Prompt
### Issue description
Direct `queryParamMap.subscribe(...)` calls are not disposed, which can lead to retained subscriptions and unexpected extra loads.
### Issue Context
AlertsComponent already implements `OnDestroy` but only closes SSE/timers; ClipsComponent does not implement `OnDestroy`.
### Fix Focus Areas
- dashboard/src/app/pages/alerts/alerts.component.ts[259-273]
- dashboard/src/app/pages/clips/clips.component.ts[345-367]
### Concrete fix
- Use `takeUntilDestroyed(inject(DestroyRef))` for both subscriptions, or store the Subscription and `unsubscribe()` in `ngOnDestroy`.
- Consider using the async pipe where practical (less manual lifecycle management).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Merged
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