Remove useEffects and fix label selection regression by jpggvilaca · Pull Request #5781 · open-edge-platform/training_extensions

jpggvilaca · 2026-03-16T09:53:35Z

Summary

Remove a few unnecessary useEffects after revisiting https://react.dev/learn/you-might-not-need-an-effect which will cut renders in at least half
Add new hook to handle media transition
Remove annoying react router v7 warning from tests. We still have a warning from --localstorage flag but that comes from node 25 so i didnt bother fixing it
Fix selection label regression. When we selected "No object" we couldnt submit
Requirements checked: Tool persists across media item; Selected label persists across media item; Changing media item resets both annotations and selected annotations

Tested manually but feel free to double check

How to test

Checklist

The PR title and description are clear and descriptive
I have manually tested the changes
All changes are covered by automated tests
All related issues are linked to this PR (if applicable)
Documentation has been updated (if applicable)

Copilot

Pull request overview

This PR aims to reduce unnecessary React renders by removing “sync state from props” useEffects, while also silencing the React Router v7 startTransition warning by enabling the future.v7_startTransition flag on RouterProvider.

Changes:

Enable future.v7_startTransition on RouterProvider in both runtime providers and test render utilities.
Refactor several components/hooks to avoid prop→state syncing via useEffect by using draft state, memoized derived values, or remount keys.
Adjust annotator provider composition and polygon editing mounting behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
application/ui/src/test-utils/render.tsx	Passes React Router `future` flag in test RouterProvider to remove warnings.
application/ui/src/providers.tsx	Passes React Router `future` flag in app RouterProvider.
application/ui/src/shared/annotator/select-annotation-provider.component.tsx	Removes `resetKey` prop and effect-based reset for selected annotations.
application/ui/src/features/dataset/media-preview/annotator-providers.component.tsx	Reorders providers; nests `SelectAnnotationProvider` inside `AnnotationActionsProvider`.
application/ui/src/features/models/train-model/advanced-settings/components/range-parameter-field/range-parameter-field.component.tsx	Replaces effect-driven syncing with draft range state.
application/ui/src/features/models/train-model/advanced-settings/components/number-parameter-field.component.tsx	Replaces effect-driven syncing with draft value state for slider interactions.
application/ui/src/features/annotator/video-player/video-toolbar/video-annotator/video-timeline/video-player-slider/video-player-slider.component.tsx	Uses a drag-only draft frame value instead of syncing local state via effect.
application/ui/src/features/annotator/tools/ssim-tool/use-ssim.hook.ts	Moves preview reset logic from an effect into `updateToolState`.
application/ui/src/features/annotator/tools/hooks/use-polygon-config.hook.tsx	Replaces polygon state + effects with memoized derived polygon.
application/ui/src/features/annotator/tools/edit-polygon/edit-polygon.component.tsx	Removes effect that synced local shape state to prop changes.
application/ui/src/features/annotator/annotations/editable-annotation.component.tsx	Forces polygon editor remount via a key derived from polygon points.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

application/ui/src/features/dataset/media-preview/annotator-providers.component.tsx

application/ui/src/features/annotator/annotations/editable-annotation.component.tsx

github-actions · 2026-03-16T10:01:30Z

📊 Test coverage report

Metric	Coverage
Lines	44.1%
Functions	77.4%
Branches	87.3%
Statements	44.1%

github-actions · 2026-03-16T10:26:13Z

Docker Image Sizes

CPU

Image	Size
geti-cpu:pr-5781	1.01G
geti-cpu:sha-534434d	1.01G

CUDA

Image	Size
geti-cuda:pr-5781	5.54G
geti-cuda:sha-534434d	5.54G

XPU

Image	Size
geti-xpu:pr-5781	3.33G
geti-xpu:sha-534434d	3.33G

application/ui/src/features/annotator/annotations/editable-annotation.component.tsx

application/ui/src/features/annotator/labels/labels.component.test.tsx

application/ui/src/features/dataset/media-preview/annotator-providers.component.tsx

...model/advanced-settings/components/range-parameter-field/range-parameter-field.component.tsx

dwesolow · 2026-03-16T12:19:53Z

...eatures/models/train-model/advanced-settings/components/number-parameter-field.component.tsx

    step,
 }: NumberGroupParamsProps) => {
-    const [parameterValue, setParameterValue] = useState<number>(value);
+    const [draftValue, setDraftValue] = useState<number | null>(null);


same here, value is never null

answered above

dwesolow · 2026-03-17T07:39:42Z

...toolbar/video-annotator/video-timeline/video-player-slider/video-player-slider.component.tsx

                }}
                onChangeEnd={(newFrameNumber) => {
                    selectFrame(newFrameNumber);
+                    setDragFrameNumber(null);


why value is updated to null on end?

Or else line 39 const parameterValue = draftRange ?? { start: value[0], end: value[1] }; will always have a draftRange, when we wouldnt be able to reset.

dwesolow · 2026-03-17T07:40:34Z

...toolbar/video-annotator/video-timeline/video-player-slider/video-player-slider.component.tsx

 }: VideoPlayerSliderProps) => {
-    const [sliderValue, setSliderValue] = useState<number>(frameNumber);
-
-    useEffect(() => setSliderValue(frameNumber), [frameNumber]);


Have u checked different scenarios like playing video, selecting next/prev frame etc?

dwesolow · 2026-03-17T07:41:36Z

...model/advanced-settings/components/range-parameter-field/range-parameter-field.component.tsx

        }

-        onChange([parameterValue.start, parameterValue.end]);
+        setDraftRange(null);


same here, why setting that to null?

Answered above. But there was a bug though. handleRangeChangeEnd should use the input Value and not the draft. thanks!

dwesolow · 2026-03-17T07:45:21Z

application/ui/tests/annotator/annotator.spec.ts

+        await test.step('Select annotation on media 1', async () => {
+            await page.getByRole('button', { name: 'selection tool' }).click();
+            await page.getByLabel('annotation rect').nth(1).click();
+        });
+
+        await test.step('Switch to media 2 and back to media 1', async () => {
+            const sidebarItems = page.getByRole('listbox', { name: 'sidebar-items' });
+            await sidebarItems.getByRole('img', { name: 'item-2.jpg' }).click();
+            await expect(annotatorPage.getAnnotationsList()).toBeVisible();
+
+            await sidebarItems.getByRole('img', { name: 'item-1.jpg' }).click();
+            await expect(annotatorPage.getAnnotationsList()).toBeVisible();
+            expect(await annotatorPage.getAnnotationsListItems('annotation rect')).toHaveLength(1);
+        });


this test only checks if annotation is visible, it does not check if annotation is not selected anymore.

Indeed, added aria label to selected annotation and updated test

dwesolow · 2026-03-17T07:45:52Z

application/ui/src/shared/annotator/select-annotation-provider.component.tsx

 };

-export const SelectAnnotationProvider = ({ children, resetKey }: { children: ReactNode; resetKey?: string }) => {
+export const SelectAnnotationProvider = ({ children }: { children: ReactNode }) => {


When do we reset selected annotations?

We do not at the moment

…o reset the annotations, not only selected ones

application/ui/src/features/annotator/tools/hooks/use-polygon-config.hook.tsx

dwesolow · 2026-03-17T11:46:13Z

application/ui/src/features/dataset/media-preview/annotator.component.tsx

        <VideoPlayerProvider
            videoFrame={isVideoFrame(mediaItem) ? mediaItem : undefined}
-            changeSelectedMediaItem={selectMediaItem}
+            changeSelectedMediaItem={onSelectedMediaItem}


why this change? We need to update selected media in SelectedMediaProvider and update URL.

onSelectedMediaItem will call SelectedMediaProvider's setItem. And updating the url is done by useSelectDatasetItem.

camiloHimura · 2026-03-17T11:52:38Z

application/ui/src/features/dataset/media-preview/use-annotator-media-transition.hook.ts

+type UseAnnotatorMediaTransitionProps = {
+    onSelectedMediaItem: (item: Media) => void;
+};
+export const useAnnotatorMediaTransition = ({ onSelectedMediaItem }: UseAnnotatorMediaTransitionProps) => {


This code made me realize we have a useSelectDatasetItem hook that stores the media item in the URL and returns the selected item, while we also have SelectedMediaItemProvider / useSelectedMediaItem, which updates an internal setMediaItem state. Is that second approach really necessary?

This is a valid point. We have useSelectDatasetItem taking care of routing. useSelectedMediaItem taking care of current media info, and now useAnnotatorMediaTransition to orchestrate media states. So the final answer is that we still need all

jpggvilaca added the Geti Tune UI Issues related to Geti Tune UI label Mar 16, 2026

jpggvilaca requested a review from a team as a code owner March 16, 2026 09:53

Copilot AI review requested due to automatic review settings March 16, 2026 09:53

Copilot started reviewing on behalf of jpggvilaca March 16, 2026 09:54 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

application/ui/src/features/dataset/media-preview/annotator-providers.component.tsx Show resolved Hide resolved

application/ui/src/features/annotator/annotations/editable-annotation.component.tsx Outdated Show resolved Hide resolved

jpggvilaca changed the title ~~Use effects~~ Remove useEffects and fix label selection regression Mar 16, 2026