frontend: video-manager: Add block list support for MCM sources#3846
Open
joaoantoniocardoso wants to merge 1 commit intobluerobotics:masterfrom
Open
frontend: video-manager: Add block list support for MCM sources#3846joaoantoniocardoso wants to merge 1 commit intobluerobotics:masterfrom
joaoantoniocardoso wants to merge 1 commit intobluerobotics:masterfrom
Conversation
Reviewer's GuideAdds block-list support for video sources in the video manager by wiring new backend endpoints into the Vuex video store, exposing a pirate-mode-only block toggle in the VideoDevice UI, and extending the Device type with an optional blocked flag that affects status rendering and stream creation controls. Sequence diagram for blocking and unblocking a video sourcesequenceDiagram
actor User
participant VideoDevice as VideoDevice_component
participant VideoStore as Vuex_VideoStore
participant Backend as Backend_API
User->>VideoDevice: Toggle Block source switch
activate VideoDevice
alt Device currently not blocked
VideoDevice->>VideoStore: blockSource(device.source)
activate VideoStore
VideoStore->>Backend: POST /block_source
Backend-->>VideoStore: 200 OK or error
alt Success
VideoStore->>VideoStore: fetchDevices()
VideoStore->>Backend: GET /devices
Backend-->>VideoStore: Devices_with_blocked_flag
VideoStore->>VideoStore: fetchStreams()
VideoStore->>Backend: GET /streams
Backend-->>VideoStore: Streams
else Failure
VideoStore->>VideoStore: notifier.pushError(VIDEO_SOURCE_BLOCK_FAIL)
end
deactivate VideoStore
else Device currently blocked
VideoDevice->>VideoStore: unblockSource(device.source)
activate VideoStore
VideoStore->>Backend: POST /unblock_source
Backend-->>VideoStore: 200 OK or error
alt Success
VideoStore->>VideoStore: fetchDevices()
VideoStore->>Backend: GET /devices
Backend-->>VideoStore: Devices_with_blocked_flag
VideoStore->>VideoStore: fetchStreams()
VideoStore->>Backend: GET /streams
Backend-->>VideoStore: Streams
else Failure
VideoStore->>VideoStore: notifier.pushError(VIDEO_SOURCE_UNBLOCK_FAIL)
end
deactivate VideoStore
end
VideoStore-->>VideoDevice: Updated devices and streams in store
VideoDevice-->>User: Updated status text, color and Add stream button state
deactivate VideoDevice
Updated class diagram for video store and device block flagclassDiagram
class VideoStore {
+string API_URL
+Promise~void~ blockSource(source)
+Promise~void~ unblockSource(source)
+Promise~void~ fetchDevices()
+Promise~void~ fetchStreams()
}
class Device {
+string source
+Format[] formats
+Control[] controls
+boolean blocked
}
class VideoDeviceComponent {
+Device device
+boolean are_video_streams_available
+boolean updating_streams
+boolean is_redirect_source
+boolean is_pirate_mode
+string status_color
+void openStreamCreationDialog()
+Promise~void~ toggleBlocked()
+Promise~void~ createNewStream(stream)
}
class Settings {
+boolean is_pirate_mode
}
VideoDeviceComponent --> Device : uses
VideoDeviceComponent --> VideoStore : dispatches_actions
VideoDeviceComponent --> Settings : reads
VideoStore --> Device : populates_list_of
class Format
class Control
VideoStore --> Format : uses
VideoStore --> Control : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Integrate the block list API from mavlink-camera-manager PR bluerobotics#561. Pirate-mode users can toggle source blocking via a switch; all users see the blocked state reflected in the status indicator and tooltip.
joaoantoniocardoso
force-pushed
the
master-add-block-list-mcm-sources
branch
from
b7698d0 to
f693aec
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the
blockSourceandunblockSourceactions you're mixingasync/awaitwith.then/.catch; consider rewriting them to use a singletry/await/catchflow for readability and to make it clearer what is awaited. blockSource/unblockSourcecallfetchDevices()andfetchStreams()sequentially after every change; if both are needed every time, you couldawait Promise.all([this.fetchDevices(), this.fetchStreams()])to avoid unnecessary latency and make the intent explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `blockSource` and `unblockSource` actions you're mixing `async/await` with `.then/.catch`; consider rewriting them to use a single `try/await/catch` flow for readability and to make it clearer what is awaited.
- `blockSource`/`unblockSource` call `fetchDevices()` and `fetchStreams()` sequentially after every change; if both are needed every time, you could `await Promise.all([this.fetchDevices(), this.fetchStreams()])` to avoid unnecessary latency and make the intent explicit.
## Individual Comments
### Comment 1
<location path="core/frontend/src/store/video.ts" line_range="248-264" />
<code_context>
}
+ @Action
+ async blockSource(source: string): Promise<void> {
+ await back_axios({
+ method: 'post',
+ url: `${this.API_URL}/block_source`,
+ timeout: 10000,
+ params: { source_string: source },
+ })
+ .then(() => {
+ this.fetchDevices()
+ this.fetchStreams()
+ })
+ .catch((error) => {
+ const message = `Could not block video source: ${error.message}.`
+ notifier.pushError('VIDEO_SOURCE_BLOCK_FAIL', message)
</code_context>
<issue_to_address>
**suggestion:** Avoid mixing async/await with .then/.catch and consider awaiting the subsequent fetches.
Given this is already an async function using await, prefer a try/catch instead of .then/.catch for consistency and readability. That also lets you `await Promise.all([this.fetchDevices(), this.fetchStreams()])` so `blockSource` only resolves once the device and stream state are updated for callers.
```suggestion
@Action
async blockSource(source: string): Promise<void> {
try {
await back_axios({
method: 'post',
url: `${this.API_URL}/block_source`,
timeout: 10000,
params: { source_string: source },
})
await Promise.all([this.fetchDevices(), this.fetchStreams()])
} catch (error: any) {
const message = `Could not block video source: ${error.message}.`
notifier.pushError('VIDEO_SOURCE_BLOCK_FAIL', message)
}
}
```
</issue_to_address>
### Comment 2
<location path="core/frontend/src/store/video.ts" line_range="266-282" />
<code_context>
+ }
+
+ @Action
+ async unblockSource(source: string): Promise<void> {
+ await back_axios({
+ method: 'post',
+ url: `${this.API_URL}/unblock_source`,
+ timeout: 10000,
+ params: { source_string: source },
+ })
+ .then(() => {
+ this.fetchDevices()
+ this.fetchStreams()
+ })
+ .catch((error) => {
+ const message = `Could not unblock video source: ${error.message}.`
+ notifier.pushError('VIDEO_SOURCE_UNBLOCK_FAIL', message)
</code_context>
<issue_to_address>
**suggestion:** Mirror the blockSource refactor here for consistency and clearer control flow.
Please apply the same structure you use in `blockSource` (e.g., try/catch with awaited follow-up fetches) so these two actions stay in sync and avoid subtle behavioral differences between block and unblock over time.
```suggestion
@Action
async unblockSource(source: string): Promise<void> {
try {
await back_axios({
method: 'post',
url: `${this.API_URL}/unblock_source`,
timeout: 10000,
params: { source_string: source },
})
await this.fetchDevices()
await this.fetchStreams()
} catch (error: any) {
const message = `Could not unblock video source: ${error.message}.`
notifier.pushError('VIDEO_SOURCE_UNBLOCK_FAIL', message)
}
}
```
</issue_to_address>
### Comment 3
<location path="core/frontend/src/components/video-manager/VideoDevice.vue" line_range="55-62" />
<code_context>
<v-icon>mdi-plus</v-icon>
Add stream
</v-btn>
+ <v-switch
+ v-if="is_pirate_mode"
+ :input-value="device.blocked"
+ dense
+ hide-details
+ class="mt-2"
+ label="Block source"
+ @change="toggleBlocked"
+ />
</div>
</code_context>
<issue_to_address>
**suggestion:** Consider handling transient toggle state or disabling the switch while the block/unblock request is in flight.
As written, users can toggle the switch multiple times while `toggleBlocked` is still waiting on the backend, which can trigger overlapping block/unblock calls and leave the UI out of sync. A simple `blocking`/`unblocking` flag wired to `:loading`/`:disabled` on the v-switch (or reusing `updating_streams` if it fits) would prevent this and make the interaction more reliable.
Suggested implementation:
```
<v-switch
v-if="is_pirate_mode"
:input-value="device.blocked"
:disabled="updating_streams"
:loading="updating_streams"
dense
hide-details
class="mt-2"
label="Block source"
@change="toggleBlocked"
/>
```
To fully address the race condition, ensure that `toggleBlocked` manages `updating_streams` (or a dedicated flag like `blocking_source`) while the block/unblock request is in flight, for example:
1. Set `this.updating_streams = true` before issuing the backend request.
2. Use a `try/finally` block so that `this.updating_streams` is reset to `false` regardless of success/failure.
3. Optionally, handle failures by reverting the local `device.blocked` state or re-fetching device data so the UI stays in sync with the backend.
If `updating_streams` is not appropriate for this purpose, introduce a new `blocking_source` data property and bind `:disabled` / `:loading` to that instead, updating it in `toggleBlocked` with the same pattern.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+248
to
+264
| @Action | ||
| async blockSource(source: string): Promise<void> { | ||
| await back_axios({ | ||
| method: 'post', | ||
| url: `${this.API_URL}/block_source`, | ||
| timeout: 10000, | ||
| params: { source_string: source }, | ||
| }) | ||
| .then(() => { | ||
| this.fetchDevices() | ||
| this.fetchStreams() | ||
| }) | ||
| .catch((error) => { | ||
| const message = `Could not block video source: ${error.message}.` | ||
| notifier.pushError('VIDEO_SOURCE_BLOCK_FAIL', message) | ||
| }) | ||
| } |
There was a problem hiding this comment.
suggestion: Avoid mixing async/await with .then/.catch and consider awaiting the subsequent fetches.
Given this is already an async function using await, prefer a try/catch instead of .then/.catch for consistency and readability. That also lets you await Promise.all([this.fetchDevices(), this.fetchStreams()]) so blockSource only resolves once the device and stream state are updated for callers.
Suggested change
| @Action | |
| async blockSource(source: string): Promise<void> { | |
| await back_axios({ | |
| method: 'post', | |
| url: `${this.API_URL}/block_source`, | |
| timeout: 10000, | |
| params: { source_string: source }, | |
| }) | |
| .then(() => { | |
| this.fetchDevices() | |
| this.fetchStreams() | |
| }) | |
| .catch((error) => { | |
| const message = `Could not block video source: ${error.message}.` | |
| notifier.pushError('VIDEO_SOURCE_BLOCK_FAIL', message) | |
| }) | |
| } | |
| @Action | |
| async blockSource(source: string): Promise<void> { | |
| try { | |
| await back_axios({ | |
| method: 'post', | |
| url: `${this.API_URL}/block_source`, | |
| timeout: 10000, | |
| params: { source_string: source }, | |
| }) | |
| await Promise.all([this.fetchDevices(), this.fetchStreams()]) | |
| } catch (error: any) { | |
| const message = `Could not block video source: ${error.message}.` | |
| notifier.pushError('VIDEO_SOURCE_BLOCK_FAIL', message) | |
| } | |
| } |
Comment on lines
+266
to
+282
| @Action | ||
| async unblockSource(source: string): Promise<void> { | ||
| await back_axios({ | ||
| method: 'post', | ||
| url: `${this.API_URL}/unblock_source`, | ||
| timeout: 10000, | ||
| params: { source_string: source }, | ||
| }) | ||
| .then(() => { | ||
| this.fetchDevices() | ||
| this.fetchStreams() | ||
| }) | ||
| .catch((error) => { | ||
| const message = `Could not unblock video source: ${error.message}.` | ||
| notifier.pushError('VIDEO_SOURCE_UNBLOCK_FAIL', message) | ||
| }) | ||
| } |
There was a problem hiding this comment.
suggestion: Mirror the blockSource refactor here for consistency and clearer control flow.
Please apply the same structure you use in blockSource (e.g., try/catch with awaited follow-up fetches) so these two actions stay in sync and avoid subtle behavioral differences between block and unblock over time.
Suggested change
| @Action | |
| async unblockSource(source: string): Promise<void> { | |
| await back_axios({ | |
| method: 'post', | |
| url: `${this.API_URL}/unblock_source`, | |
| timeout: 10000, | |
| params: { source_string: source }, | |
| }) | |
| .then(() => { | |
| this.fetchDevices() | |
| this.fetchStreams() | |
| }) | |
| .catch((error) => { | |
| const message = `Could not unblock video source: ${error.message}.` | |
| notifier.pushError('VIDEO_SOURCE_UNBLOCK_FAIL', message) | |
| }) | |
| } | |
| @Action | |
| async unblockSource(source: string): Promise<void> { | |
| try { | |
| await back_axios({ | |
| method: 'post', | |
| url: `${this.API_URL}/unblock_source`, | |
| timeout: 10000, | |
| params: { source_string: source }, | |
| }) | |
| await this.fetchDevices() | |
| await this.fetchStreams() | |
| } catch (error: any) { | |
| const message = `Could not unblock video source: ${error.message}.` | |
| notifier.pushError('VIDEO_SOURCE_UNBLOCK_FAIL', message) | |
| } | |
| } |
Comment on lines
+55
to
+62
| <v-switch | ||
| v-if="is_pirate_mode" | ||
| :input-value="device.blocked" | ||
| dense | ||
| hide-details | ||
| class="mt-2" | ||
| label="Block source" | ||
| @change="toggleBlocked" |
There was a problem hiding this comment.
suggestion: Consider handling transient toggle state or disabling the switch while the block/unblock request is in flight.
As written, users can toggle the switch multiple times while toggleBlocked is still waiting on the backend, which can trigger overlapping block/unblock calls and leave the UI out of sync. A simple blocking/unblocking flag wired to :loading/:disabled on the v-switch (or reusing updating_streams if it fits) would prevent this and make the interaction more reliable.
Suggested implementation:
<v-switch
v-if="is_pirate_mode"
:input-value="device.blocked"
:disabled="updating_streams"
:loading="updating_streams"
dense
hide-details
class="mt-2"
label="Block source"
@change="toggleBlocked"
/>
To fully address the race condition, ensure that toggleBlocked manages updating_streams (or a dedicated flag like blocking_source) while the block/unblock request is in flight, for example:
- Set
this.updating_streams = truebefore issuing the backend request. - Use a
try/finallyblock so thatthis.updating_streamsis reset tofalseregardless of success/failure. - Optionally, handle failures by reverting the local
device.blockedstate or re-fetching device data so the UI stays in sync with the backend.
If updating_streams is not appropriate for this purpose, introduce a new blocking_source data property and bind :disabled / :loading to that instead, updating it in toggleBlocked with the same pattern.
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.
mastercounterpart of the 1.4 PR: #3837This branch includes the commits from the thumbnail rework and MCM Idle
masterPRs so the UI and types stay consistent and CI can pass.Suggested merge order: merge the thumbnail and Idle PRs to
masterfirst, then rebase this branch ontomasterso this PR collapses to a single commit (the block-list change) before final merge.Related
masterPRs:Block-list commit:
33e42773(equivalent to the rebased 1.4 head).Summary by Sourcery
Add video source block list support and enhanced thumbnail controls to the video manager UI and store.
New Features:
Enhancements:
Summary by Sourcery
Add support in the video manager to block and unblock video sources and reflect their blocked status in the UI and device model.
New Features: