frontend: video-manager: Add support for MCM Idle state#3840
Conversation
…amStatus Add support for the new stream status state field from MCM with three possible values: 'running', 'idle', and 'stopped'. This enables the frontend to distinguish between healthy idle streams and stopped streams.
…s handling for new state field Replace binary running check with state-based logic: - Thumbnails now fetch when state is running or idle (not stopped) - Status indicator shows green for running, blue for idle, red for stopped - Add computed properties for has_healthy_streams, has_active_streams, has_idle_streams, and status_color
…ay for new state field - Replace Running/Not running text with state-based display (Running, Idle, Stopped) - Only show errors when stream state is stopped (not when idle) - Add stream_state_text computed property for status text mapping
Reviewer's GuideFrontend video manager now supports richer stream state (running/idle/stopped) and extended configuration flags, updating device/stream status UI and thumbnail behavior to use the new state and flags instead of the legacy running boolean alone. Sequence diagram for device status and thumbnail behavior based on stream statesequenceDiagram
actor User
participant VideoDevice
participant StreamStatus
participant VideoThumbnail
participant VideoControlsDialog
User->>VideoDevice: Open device card
VideoDevice->>StreamStatus: Read state and extended_configuration
VideoDevice->>VideoDevice: has_healthy_streams = any stream.state != stopped
VideoDevice->>VideoDevice: thumbnails_disabled = any extended_configuration.disable_thumbnails == true
VideoDevice->>VideoDevice: status_color = grey|success|error
VideoDevice->>VideoThumbnail: setProps(source, register, disabled)
Note over VideoThumbnail: register = are_video_streams_available && has_healthy_streams && !thumbnails_disabled
Note over VideoThumbnail: disabled = thumbnails_disabled
User->>VideoControlsDialog: Open controls dialog
VideoDevice->>VideoControlsDialog: pass thumbnail_register and thumbnail_disabled
Note over VideoControlsDialog: Uses same health and thumbnail flags as device card
Class diagram for updated video stream types and configurationclassDiagram
class ExtendedConfiguration {
+boolean thermal
+boolean disable_lazy
+boolean disable_mavlink
+boolean disable_thumbnails
+boolean disable_zenoh
}
class StreamInformation {
+CaptureConfiguration configuration
+ExtendedConfiguration extended_configuration
+string[] endpoints
}
class VideoAndStreamInformation {
+string name
+StreamInformation stream_information
+VideoSourceLocal~VideoSourceGst~VideoSourceRedirect~VideoSourceOnvif video_source
}
class StreamStatusState {
<<enumeration>>
+running
+idle
+stopped
}
class StreamStatus {
+string id
+boolean running
+StreamStatusState state
+string error
+VideoAndStreamInformation video_and_stream
}
class FrameInterval {
}
class StreamPrototype {
+string name
+string encode
+FrameInterval interval
+string[] endpoints
+boolean thermal
+boolean disable_lazy
+boolean disable_mavlink
+boolean disable_thumbnails
+boolean disable_zenoh
}
StreamInformation "1" --> "1" ExtendedConfiguration : has
VideoAndStreamInformation "1" --> "1" StreamInformation : has
StreamStatus "1" --> "1" VideoAndStreamInformation : wraps
StreamStatus "1" --> "1" StreamStatusState : has
StreamPrototype ..> StreamInformation : derived_from
StreamPrototype --> FrameInterval : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
has_healthy_streams, treating anystream.statethat is not exactly'stopped'as healthy may misclassify unknown or missing states; consider explicitly checking for known healthy states (e.g.,'running' | 'idle') and treating anything else as unhealthy. - The
thumbnails_disabledcomputed property disables thumbnails at the device level if any one stream hasdisable_thumbnailsset; if the intent is per-stream control rather than a global device switch, consider scoping this logic so that one stream’s setting doesn’t affect all thumbnails. - In
VideoStream.vue, the error display now requiresstream.state === 'stopped'; if there is any chance of older backends not sendingstateor sending unexpected values, it may be safer to fall back on therunningflag or a broader condition (e.g.,!stream.running) to ensure errors still render.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `has_healthy_streams`, treating any `stream.state` that is not exactly `'stopped'` as healthy may misclassify unknown or missing states; consider explicitly checking for known healthy states (e.g., `'running' | 'idle'`) and treating anything else as unhealthy.
- The `thumbnails_disabled` computed property disables thumbnails at the device level if any one stream has `disable_thumbnails` set; if the intent is per-stream control rather than a global device switch, consider scoping this logic so that one stream’s setting doesn’t affect all thumbnails.
- In `VideoStream.vue`, the error display now requires `stream.state === 'stopped'`; if there is any chance of older backends not sending `state` or sending unexpected values, it may be safer to fall back on the `running` flag or a broader condition (e.g., `!stream.running`) to ensure errors still render.
## Individual Comments
### Comment 1
<location path="core/frontend/src/components/video-manager/VideoStream.vue" line_range="212-213" />
<code_context>
}
},
computed: {
+ stream_state_text(): string {
+ const stateMap: Record<string, string> = {
+ running: 'Running',
+ idle: 'Idle',
</code_context>
<issue_to_address>
**suggestion:** The `stateMap` typing loses the benefit of the `StreamStatusState` union.
`StreamStatusState` is defined, but `stateMap` uses `Record<string, string>`, so TypeScript won’t enforce that all states are covered. Typing it as `Record<StreamStatusState, string>` (or a mapped type over `StreamStatusState`) will give you compile-time checks when new states are added.
Suggested implementation:
```
computed: {
stream_state_text(): string {
const stateMap: Record<StreamStatusState, string> = {
running: 'Running',
idle: 'Idle',
stopped: 'Stopped',
}
```
1. Ensure that the `StreamStatusState` type is imported or otherwise available in this `<script>` block. For example, if it lives in a shared/types file, add something like:
`import type { StreamStatusState } from '@/types/stream'`
near the top of the `<script>` section.
2. Confirm that `this.stream.state` is typed as `StreamStatusState`. If it is currently just `string`, consider tightening its type in the relevant interface/model so you get full compile-time coverage for all states.
</issue_to_address>
### Comment 2
<location path="core/frontend/src/types/video.ts" line_range="116-121" />
<code_context>
export interface ExtendedConfiguration {
thermal: boolean
+ disable_lazy: boolean
disable_mavlink: boolean
+ disable_thumbnails: boolean
+ disable_zenoh: boolean
}
</code_context>
<issue_to_address>
**suggestion:** New fields in `ExtendedConfiguration` are required booleans, but usage treats them as optional.
These fields are typed as required booleans, but callers use optional chaining and `?? false`, implying they may be absent. To align types with actual usage and avoid defensive access patterns, consider making them optional (e.g. `thermal?: boolean`) and defaulting to `false` at use sites.
Suggested implementation:
```typescript
export interface ExtendedConfiguration {
thermal?: boolean
disable_lazy?: boolean
disable_mavlink?: boolean
disable_thumbnails?: boolean
disable_zenoh?: boolean
}
```
To fully implement the suggestion, callers that currently access these fields defensively (e.g. `config?.extended?.disable_lazy ?? false`) can safely keep that pattern, which now matches the optional typing. If you centralize defaults (for example, when normalizing configuration objects), you can set these fields to `false` there and then simplify call sites to non-optional access (`config.extended.disable_lazy`) in those normalized contexts.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| stream_state_text(): string { | ||
| const stateMap: Record<string, string> = { |
There was a problem hiding this comment.
suggestion: The stateMap typing loses the benefit of the StreamStatusState union.
StreamStatusState is defined, but stateMap uses Record<string, string>, so TypeScript won’t enforce that all states are covered. Typing it as Record<StreamStatusState, string> (or a mapped type over StreamStatusState) will give you compile-time checks when new states are added.
Suggested implementation:
computed: {
stream_state_text(): string {
const stateMap: Record<StreamStatusState, string> = {
running: 'Running',
idle: 'Idle',
stopped: 'Stopped',
}
- Ensure that the
StreamStatusStatetype is imported or otherwise available in this<script>block. For example, if it lives in a shared/types file, add something like:
import type { StreamStatusState } from '@/types/stream'
near the top of the<script>section. - Confirm that
this.stream.stateis typed asStreamStatusState. If it is currently juststring, consider tightening its type in the relevant interface/model so you get full compile-time coverage for all states.
| export interface ExtendedConfiguration { | ||
| thermal: boolean | ||
| disable_lazy: boolean | ||
| disable_mavlink: boolean | ||
| disable_thumbnails: boolean | ||
| disable_zenoh: boolean |
There was a problem hiding this comment.
suggestion: New fields in ExtendedConfiguration are required booleans, but usage treats them as optional.
These fields are typed as required booleans, but callers use optional chaining and ?? false, implying they may be absent. To align types with actual usage and avoid defensive access patterns, consider making them optional (e.g. thermal?: boolean) and defaulting to false at use sites.
Suggested implementation:
export interface ExtendedConfiguration {
thermal?: boolean
disable_lazy?: boolean
disable_mavlink?: boolean
disable_thumbnails?: boolean
disable_zenoh?: boolean
}To fully implement the suggestion, callers that currently access these fields defensively (e.g. config?.extended?.disable_lazy ?? false) can safely keep that pattern, which now matches the optional typing. If you centralize defaults (for example, when normalizing configuration objects), you can set these fields to false there and then simplify call sites to non-optional access (config.extended.disable_lazy) in those normalized contexts.
To test it, one should use MCM next-13 from my fork.
Cherry-picks to 1.4:
Summary by Sourcery
Update video manager frontend to support extended stream states and configuration flags, including idle state handling and thumbnail control.
New Features:
Enhancements:
Summary by Sourcery
Update video manager frontend to use new stream state metadata and extended configuration flags for device status, stream display, and thumbnails.
New Features:
Enhancements: