-
Notifications
You must be signed in to change notification settings - Fork 836
feat(inspect): Pipeline enable/disable #3097
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
base: feature/geti-inspect
Are you sure you want to change the base?
feat(inspect): Pipeline enable/disable #3097
Conversation
Signed-off-by: Colorado, Camilo <camilo.colorado@intel.com>
Signed-off-by: Colorado, Camilo <camilo.colorado@intel.com>
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.
Pull Request Overview
This PR adds pipeline enable/disable functionality via a switch component, refactors sink configuration handling to use UUIDs consistently, and removes a duplicate MQTT fields component. Additionally, it addresses several pending code review comments from previous PRs including renaming a component, fixing invalidation logic, and implementing WebRTC offer functionality.
- Added a
PipelineSwitchcomponent to enable/disable pipelines - Refactored sink configuration to generate UUIDs at initialization rather than during mutation
- Implemented WebRTC offer functionality in the connection class
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| train-model-dialog.component.tsx | Formatting cleanup - consolidated meta configuration |
| toolbar.tsx | Added PipelineSwitch component to toolbar and reordered variable declarations |
| webhook-fields.component.tsx | Renamed KeyValueBuilder to HeaderKeyValueBuilder |
| utils.ts (webhook-fields) | Updated webhook initial config to use UUID |
| header-key-value-builder.component.tsx | Renamed component and type from KeyValueBuilder to HeaderKeyValueBuilder |
| sink-menu.component.tsx | Removed unnecessary invalidation of sinks query |
| utils.ts (ros-fields) | Updated ROS initial config to use UUID |
| utils.ts (mqtt-fields) | Updated MQTT initial config to use UUID |
| mqtt-fields copy/mqtt-fields.component.tsx | Deleted duplicate MQTT fields component file |
| utils.ts (local-folder-fields) | Updated local folder initial config to use UUID |
| use-sink-mutation.hook.tsx | Removed UUID generation from mutation hook, now uses UUID from config |
| pipeline-switch.component.tsx | New component implementing pipeline enable/disable toggle |
| stream.tsx (inspect/stream) | Added debug console.log statement |
| inference-result.component.tsx | Added early return for StreamContainer when no media item selected |
| inference-provider.component.tsx | Added pipeline update when model is selected |
| web-rtc-connection.ts | Implemented sendOffer method using fetchClient |
| stream.tsx (components/stream) | Deleted unused stream component file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { status, webRTCConnectionRef } = useWebRTCConnection(); | ||
|
|
||
| const connect = useCallback(async () => { | ||
| console.log('feature connect'); |
Copilot
AI
Nov 7, 2025
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.
Debug console.log statement should be removed before merging to production. This appears to be a leftover debugging artifact.
| console.log('feature connect'); |
| const enablePipeline = $api.useMutation('post', '/api/projects/{project_id}/pipeline:enable', { | ||
| onError: (error) => { | ||
| if (error) { | ||
| toast({ type: 'error', message: String(error.detail) }); |
Copilot
AI
Nov 7, 2025
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.
The error message displayed to users may not be helpful if error.detail is undefined or contains technical details. Consider providing a user-friendly fallback message like 'Failed to enable pipeline' when error.detail is not available.
| toast({ type: 'error', message: String(error.detail) }); | |
| const message = | |
| typeof error?.detail === 'string' && error.detail.trim() | |
| ? error.detail | |
| : 'Failed to enable pipeline'; | |
| toast({ type: 'error', message }); |
| const disablePipeline = $api.useMutation('post', '/api/projects/{project_id}/pipeline:disable', { | ||
| meta: { | ||
| invalidates: [ | ||
| ['get', '/api/projects/{project_id}/pipeline', { params: { path: { project_id: projectId } } }], | ||
| ], | ||
| }, | ||
| }); |
Copilot
AI
Nov 7, 2025
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.
The disablePipeline mutation is missing an onError handler similar to enablePipeline. Consider adding error handling to provide user feedback when disabling the pipeline fails.
MarkRedeman
left a comment
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.
Can you check the failing eslint and playwright test?
| const isLoadingInference = useSpinDelay(isPending, { delay: 300 }); | ||
|
|
||
| if (selectedMediaItem === undefined) { | ||
| return <StreamContainer />; |
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.
One thing to note: here we will need to improve the validation a bit more. In the demo code I send you I rendered the StreamContainer here out of convenience.
In a later UX we should:
- Show a help message if no source has been defined yet
- Show the selected media item + live inference when a media item + model is selected
- Show the selected media item + help text if no model is selected/trained yet
- Show the video stream from the active source
| const { selectedMediaItem } = useSelectedMediaItem(); | ||
|
|
||
| const onSetSelectedModelId = (modelId: string | undefined) => { | ||
| setSelectedModelId(modelId); |
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.
I think we could get rid of setSelectedModelId now and instead use the model id from the project's pipeline query.
📝 Description
This PR addresses some pending comments from previous PRs and adds a switch to enable or disable the pipeline. The inference-result.component.tsx file will be updated in an upcoming PR once certain workflow details are clarified.
Screen.Recording.2025-11-07.at.14.19.50.mov
✨ Changes
Select what type of change your PR is:
✅ Checklist
Before you submit your pull request, please make sure you have completed the following steps:
For more information about code review checklists, see the Code Review Checklist.