Keep model architectures list expanded when navigating back by andersendsa · Pull Request #6881 · open-edge-platform/geti

andersendsa · 2026-06-20T02:13:35Z

Summary

Initializes the showMore state in ModelArchitecturesList to true if the currently selected model architecture is not in the collapsed list. This ensures that when a user selects a non-default model and navigates back from the "Advanced settings" screen, their selection is still visible.

Closes:#6823

How to test

Open the Train model dialog.
Click Show more in the model architectures list.
Select an architecture that is not in the initially recommended/collapsed list (e.g., the 5th option).
Click Advanced settings.
Click Back.
Verify that the model architectures list remains expanded, making the selected architecture visible.

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)

Initializes the `showMore` state in `ModelArchitecturesList` to `true` if the currently selected model architecture is not in the collapsed list. This ensures that when a user selects a non-default model and navigates back from the "Advanced settings" screen, their selection is still visible.

Copilot

Pull request overview

Updates the Train Model dialog’s model-architectures selector so that when a user returns from “Advanced settings”, the architectures list starts expanded if the currently selected architecture would otherwise be hidden in the collapsed/recommended subset.

Changes:

Initializes showMore based on whether the selected architecture is present in the collapsed (recommended) list.
Preserves visibility of non-default (non-collapsed) selections when navigating back from advanced settings.

+    const [showMore, setShowMore] = useState<boolean>(() => {
+        if (selectedModelArchitectureId !== null) {
+            const isSelectedInCollapsed = collapsedArchitectures.some((arch) => arch.id === selectedModelArchitectureId);
+            return !isSelectedInCollapsed;
+        }


andersendsa · 2026-06-20T08:06:53Z

Hi @piotrgrubicki pls let me know if this pr needs any changes or if it is good to merge

dwesolow · 2026-06-22T07:16:55Z

Hi @andersendsa, thanks for contribution! Could you please implement test(s) for this change?

andersendsa · 2026-06-22T09:32:21Z

Hi @andersendsa, thanks for contribution! Could you please implement test(s) for this change?

Hi @dwesolow I have added the test for this change the pr is ready for review

Copilot

Pull request overview

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

+    const [showMore, setShowMore] = useState<boolean>(() => {
+        if (selectedModelArchitectureId !== null) {
+            const isSelectedInCollapsed = collapsedArchitectures.some((arch) => arch.id === selectedModelArchitectureId);
+            return !isSelectedInCollapsed;
+        }
+        return false;
+    });


+    it('renders expanded list by default if a non-default architecture is already selected', () => {
+        mockSelectedModelArchitectureId.value = 'arch-5';
+        render(<ModelArchitecturesList />);
+
+        expect(screen.getByRole('button', { name: 'Show less' })).toBeVisible();
+        expect(screen.getByRole('button', { name: /Sort Models by:/i })).toBeVisible();
+    });


        fireEvent.click(screen.getByRole('button', { name: 'Show less' }));
        expect(screen.queryByRole('button', { name: /Sort Models by:/i })).not.toBeInTheDocument();
    });
+


dwesolow · 2026-06-22T09:41:55Z

We should test the change in higher level, to actually test expected behavior, i.e. if user clicks back in advanced settings, the model architectures are expended.

andersendsa · 2026-06-23T01:30:54Z

We should test the change in higher level, to actually test expected behavior, i.e. if user clicks back in advanced settings, the model architectures are expended.

Hi @dwesolow i have added the test pls let me know if the pr needs any changes or if it is good to merge

dwesolow

Great job!

jpggvilaca

@andersendsa while the solution works, i feel like it's a bit hacky. Instead of collapsing/opening based on a condition, we should make this logic selection agnostic. By this i mean, selecting "advanced settings" or selecting a model should not affect the "show more logic". The issue right now is that the state is reset whenever you open Advanced settings because the model list is remounted. So, in order to avoid this, i think we should lift the "show more" state logic to the train model provider. This way the model list will be collapsed/opened regardless of where you are.

TL;DR -> Move showMore/setShowMore to TrainModelProvider to avoid state resets when we navigate to AdvancedSettings

andersendsa · 2026-06-23T09:08:47Z

@andersendsa while the solution works, i feel like it's a bit hacky. Instead of collapsing/opening based on a condition, we should make this logic selection agnostic. By this i mean, selecting "advanced settings" or selecting a model should not affect the "show more logic". The issue right now is that the state is reset whenever you open Advanced settings because the model list is remounted. So, in order to avoid this, i think we should lift the "show more" state logic to the train model provider. This way the model list will be collapsed/opened regardless of where you are.

TL;DR -> Move showMore/setShowMore to TrainModelProvider to avoid state resets when we navigate to AdvancedSettings

Hi @jpggvilaca i have implemented the changes pls let me know if the pr needs anymore changes or if it is good to merge

jpggvilaca · 2026-06-23T09:50:15Z

    const collapsedArchitectures = recommendedArchitectures.slice(0, SHOW_MORE_THRESHOLD);
    const canToggleArchitecturesList = modelArchitectures.length > SHOW_MORE_THRESHOLD;

+    useEffect(() => {


I dont think we need this useEffect anymore. This was what i mentioned previously. We dont need to open/close any show more. Now that the state is on the provider, the list should not collapse

I dont think we need this useEffect anymore. This was what i mentioned previously. We dont need to open/close any show more. Now that the state is on the provider, the list should not collapse

@jpggvilaca I have reverted it

Copilot AI review requested due to automatic review settings June 20, 2026 02:13

andersendsa requested a review from a team as a code owner June 20, 2026 02:13

github-actions Bot added the Geti UI Issues related to Geti application frontend label Jun 20, 2026

Merge branch 'develop' into keep_architecture

81cd321

Copilot started reviewing on behalf of andersendsa June 20, 2026 02:14 View session

Copilot AI reviewed Jun 20, 2026

View reviewed changes

adding tests

2d1bb76

andersendsa requested a review from Copilot June 22, 2026 09:32

Copilot AI reviewed Jun 22, 2026

View reviewed changes

dwesolow reviewed Jun 22, 2026

View reviewed changes

Comment thread ...i/src/features/models/train-model/model-architectures-list/model-architectures-list.test.tsx Outdated

Copilot started reviewing on behalf of andersendsa June 22, 2026 09:41 View session

adding test

6556c63

github-actions Bot added the TEST Any changes in tests label Jun 23, 2026

andersendsa requested a review from dwesolow June 23, 2026 01:03

dwesolow previously approved these changes Jun 23, 2026

View reviewed changes

jpggvilaca reviewed Jun 23, 2026

View reviewed changes

jpggvilaca requested changes Jun 23, 2026

View reviewed changes

avoiding resets when navigate to AdvancedSettings

63fb062

andersendsa dismissed dwesolow’s stale review via 63fb062 June 23, 2026 09:07

andersendsa requested a review from jpggvilaca June 23, 2026 09:07

jpggvilaca reviewed Jun 23, 2026

View reviewed changes

reverting user effect

d27baed

andersendsa requested a review from jpggvilaca June 23, 2026 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep model architectures list expanded when navigating back#6881

Keep model architectures list expanded when navigating back#6881
andersendsa wants to merge 6 commits into
open-edge-platform:developfrom
andersendsa:keep_architecture

andersendsa commented Jun 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

andersendsa commented Jun 20, 2026

Uh oh!

dwesolow commented Jun 22, 2026

Uh oh!

andersendsa commented Jun 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

dwesolow commented Jun 22, 2026

Uh oh!

andersendsa commented Jun 23, 2026

Uh oh!

dwesolow left a comment

Uh oh!

jpggvilaca left a comment

Uh oh!

andersendsa commented Jun 23, 2026

Uh oh!

jpggvilaca Jun 23, 2026

Uh oh!

andersendsa Jun 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

andersendsa commented Jun 20, 2026

Summary

How to test

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

andersendsa commented Jun 20, 2026

Uh oh!

dwesolow commented Jun 22, 2026

Uh oh!

andersendsa commented Jun 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

dwesolow commented Jun 22, 2026

Uh oh!

andersendsa commented Jun 23, 2026

Uh oh!

dwesolow left a comment

Choose a reason for hiding this comment

Uh oh!

jpggvilaca left a comment

Choose a reason for hiding this comment

Uh oh!

andersendsa commented Jun 23, 2026

Uh oh!

jpggvilaca Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

andersendsa Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants