-
Notifications
You must be signed in to change notification settings - Fork 120
frontend: src: components: video-manager: VideoManager: Hide secondary stream from radcam #3748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
frontend: src: components: video-manager: VideoManager: Hide secondary stream from radcam #3748
Conversation
…y stream from radcam
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts VideoManager device visibility rules so RadCam’s secondary stream is only shown when it has an active stream or the user is in Pirate Mode, mirroring the behavior used for Fake source devices. Sequence diagram for VideoManager device visibility evaluationsequenceDiagram
actor User
participant VideoManager
participant Device
participant Settings
User->>VideoManager: renderDeviceList()
loop for each device
VideoManager->>Device: get name
alt device.name == Fake source
VideoManager->>Device: has_active_stream(device)
VideoManager->>Settings: is_pirate_mode
VideoManager-->>VideoManager: visible = has_active_stream || is_pirate_mode
else device.name == UnderwaterCam_IPCamera_UnderwaterCam and device.source endsWith /stream_1
VideoManager->>Device: has_active_stream(device)
VideoManager->>Settings: is_pirate_mode
VideoManager-->>VideoManager: visible = has_active_stream || is_pirate_mode
else other devices
VideoManager-->>VideoManager: visible = true
end
alt visible
VideoManager-->>User: show device entry
else
VideoManager-->>User: hide device entry
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
- The RadCam detection relies on hard-coded
device.nameand a specificsourcesuffix; consider centralizing these identifiers as constants or using a more robust device identifier to avoid brittle string matching. - The visibility logic for fake sources and RadCam’s secondary stream is now duplicated; you could extract a shared helper (e.g.,
shouldShowConditionalDevice(device, settings)) to keep this behavior consistent and easier to extend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The RadCam detection relies on hard-coded `device.name` and a specific `source` suffix; consider centralizing these identifiers as constants or using a more robust device identifier to avoid brittle string matching.
- The visibility logic for fake sources and RadCam’s secondary stream is now duplicated; you could extract a shared helper (e.g., `shouldShowConditionalDevice(device, settings)`) to keep this behavior consistent and easier to extend.
## Individual Comments
### Comment 1
<location> `core/frontend/src/components/video-manager/VideoManager.vue:140-141` </location>
<code_context>
return has_active_stream(device) || settings.is_pirate_mode
}
+
+ // Do not show RadCam's secondary stream
+ if (device.name === 'UnderwaterCam - IPCamera (UnderwaterCam)' && device.source.endsWith('/stream_1')) {
+ return has_active_stream(device) || settings.is_pirate_mode
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The comment about not showing the secondary stream seems at odds with the visibility logic used here.
The inline comment says "Do not show RadCam's secondary stream", but this condition is identical to the `Fake source` case and will still show the device when it has an active stream or pirate mode is enabled. If the goal is to always hide this secondary stream, this should likely return `false` (or otherwise short-circuit) instead of `has_active_stream(device) || settings.is_pirate_mode`. Please confirm the intended UX and then update either the logic or the comment so they are consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Following the same behavior as for Fake Sources: we only show when there's a configured stream or the user is in Pirate Mode.
Closes #3746
Summary by Sourcery
New Features: