Skip to content

fix: prevent Create New switch reset when URL is entered in ImportFromGit - #1656

Open
olexii4 wants to merge 5 commits into
mainfrom
CRW-12731
Open

fix: prevent Create New switch reset when URL is entered in ImportFromGit#1656
olexii4 wants to merge 5 commits into
mainfrom
CRW-12731

Conversation

@olexii4

@olexii4 olexii4 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the "Create New" switch being silently reset to OFF when a user enters a URL in the Import from Git field, causing a second workspace from the same repository URL to redirect to the existing one instead of creating a new one.

Root Cause

Two cooperating bugs caused the switch to reset every time a valid Git URL was typed.

1. CreateNewIfExistSwitch never wrote its initial state to Navigation.pageState.

CreateNewIfExistSwitch.componentDidMount called props.onChange(true) (propagating the default ON state to the samples list), but it never set Navigation.pageState[CREATE_NEW_IF_EXIST_SWITCH_ID]. The static store stayed at { isChecked: undefined }.

2. CreateNewIfExistingField.componentDidMount unconditionally overwrote Navigation.pageState with false.

When the URL field validated successfully, RepoOptionsAccordion mounted, which in turn mounted the hidden CreateNewIfExistingField. Its componentDidMount ran:

Navigation.pageState[CREATE_NEW_IF_EXIST_SWITCH_ID] = {
  isChecked: this.state.createNewIfExisting, // always false — the field default
};

Because Navigation.pageState was undefined (bug 1), this write was never blocked. The setter broadcast false to all subscribers, including CreateNewIfExistSwitch, which flipped the visible switch to OFF and called handleCreateNewIfExistChange(false). The URL never received the ?new query param, so the factory loader used policies.create=peruser, and CheckExistingWorkspaces silently redirected to the existing workspace.

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

fixes https://redhat.atlassian.net/browse/CRW-12731

Is it tested? How?

  1. Deploy Eclipse Che with the dashboard image from this PR.
  2. Navigate to Create Workspace (Get Started page). Confirm the Create New switch in the toolbar is ON.
  3. Enter a public GitHub or GitLab URL (e.g. https://github.com/che-incubator/quarkus-api-example) and click Create & Open. Workspace A is created.
  4. Return to the same page, confirm the Create New switch is still ON. Enter the same URL and click Create & Open — a second, distinct workspace B must be created (not a redirect to A).
  5. Toggle Create New OFF. Enter the same URL and click Create & Open — the dashboard must navigate to the existing workspace A instead of creating a new one.

Release Notes

Fixed an issue where creating a second workspace from the same Git repository URL would reopen the existing workspace instead of creating a new one.

Docs PR

N/A

@openshift-ci

openshift-ci Bot commented Sep 1, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: olexii4

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@olexii4
olexii4 requested a review from svor September 1, 2026 14:46
@che-bot

che-bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Click here to review and test in web IDE: Contribute

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1656 (linux/amd64, linux/arm64, linux/s390x)

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1656", name: che-dashboard}]}}]"

1 similar comment
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1656 (linux/amd64, linux/arm64, linux/s390x)

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1656", name: che-dashboard}]}}]"

@olexii4

olexii4 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/retest

@svor

svor commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@SkorikSergey please validate

@SkorikSergey

Copy link
Copy Markdown
Contributor

@olexii4 hello, still can reproduce this issue with https://github.com/prabhuk25/PublicRepo repo. But works well with our test githup repos. The only difference I see - metadata name. In prabhuk25 repo it is Devspaces-Public-repo. Works as expected after changing it to devspaces-public-repo (https://github.com/SkorikSergey/PublicRepo/blob/main/devfile.yaml).

@SkorikSergey

Copy link
Copy Markdown
Contributor

@olexii4 also there is e2e tests regression - after setting url to Git repo URL field and clicking on Create & Open button there is only part of url used as factory url(CreateWorkspaceWithExistingNameFromGitUrl test). Cannot reproduce manually.
Screenshot

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1656 (linux/amd64, linux/arm64, linux/s390x)

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1656", name: che-dashboard}]}}]"

…mGit

CreateNewIfExistingField.componentDidMount unconditionally overwrote
Navigation.pageState with false, resetting the global "Create New"
switch even when the user had it ON. Additionally, CreateNewIfExistSwitch
never pushed its default true value to Navigation.pageState, so the
field always saw undefined and replaced it with false.

Fix: CreateNewIfExistSwitch now initialises Navigation.pageState on
mount (when not yet set). CreateNewIfExistingField now adopts the
existing global state on mount instead of overwriting it, so the ?new
param reaches the factory URL and policies.create=perclick is set.

Fixes: https://redhat.atlassian.net/browse/CRW-12731

Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
Rewrite CreateNewIfExistingField/__tests__/index.spec.tsx — the file
was testing TemporaryStorageField instead of CreateNewIfExistingField.
Added four tests covering all componentDidMount branches: adopt global
state, no-op when state matches, write local to global when undefined,
and subscription response. Use act() for out-of-band state triggers.

Simplify componentDidMount to delegate to handleChange() rather than
duplicating its setState + props.onChange logic inline.

Add a comment to the CreateNewIfExistSwitch test assertion explaining
that the switch actively writes its initial value to pageState on mount.

Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
…xistingField

The spec file in this directory was previously testing TemporaryStorageField
and had written two snapshots. Now that the file correctly tests
CreateNewIfExistingField (which has no snapshots), the stale snapshot
file causes CI to fail with exit code 1 due to obsolete snapshots.

Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
componentDidMount called onChange(true) on mount, encoding ?new into
the location URL. buildFactoryLoaderPath then stripped it back out,
leaving the input showing "url?new" while the factory url param had
only "url" — the E2E CreateWorkspaceWithExistingNameFromGitUrl test
saw this as a partial URL mismatch.

Fix: componentDidMount now adopts the global switch state silently
(setState only, no onChange). ImportFromGit.startFactory() reads
Navigation.pageState at click-time to inject ?new when the switch is
ON. Also fix componentDidUpdate to compare raw props instead of
undefined||false, which was overwriting the silently adopted state.

Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
The name-conflict lookup at Apply/Devfile step compared the raw devfile
metadata.name (e.g. "Devspaces-Public-repo") against already-created
workspace names that had gone through sanitizeName() and are lowercase
("devspaces-public-repo"). The case-sensitive mismatch always returned
nameConflict=false, so appendSuffix was never set and prepareDevfile()
produced the same unsuffixed sanitized name, causing Kubernetes to
reject the creation with a 409 Conflict.

Fix: sanitize the candidate name with sanitizeName() before the
allWorkspaces lookup so uppercase devfile names are normalised to the
same form as the existing workspace names.

Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1656 (linux/amd64, linux/arm64, linux/s390x)

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1656", name: che-dashboard}]}}]"

1 similar comment
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1656 (linux/amd64, linux/arm64, linux/s390x)

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1656", name: che-dashboard}]}}]"

@olexii4

olexii4 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@SkorikSergey fixed. The regression had two cooperating causes.

Root cause:

CreateNewIfExistingField.componentDidMount called onChange(true) when it adopted the global switch state. That propagated through AdvancedOptionsRepoOptionsAccordionImportFromGit, which encoded ?new into state.location and then overwrote the input's DOM value with url?new. When the user clicked "Create & Open", buildFactoryLoaderPath correctly moved ?newpolicies.create=perclick, but the factory url param contained only the clean git URL — the E2E test compared the input value (url?new) against the factory url param (url) and reported "only part of url used".

Also, componentDidUpdate compared prevProps.createNewIfExisting against undefined || false instead of the raw prop value, which immediately overwrote the silently-adopted local state.

Fix:

  1. componentDidMount now adopts global switch state via setState only — no onChange, so the URL and input field are not touched on mount.
  2. componentDidUpdate now compares raw prop values (prevProps.createNewIfExisting !== this.props.createNewIfExisting) to avoid the spurious reset.
  3. ImportFromGit.startFactory() reads Navigation.pageState at click-time and injects ?new directly before calling buildFactoryLoaderPath, so policies.create=perclick is still applied when the switch is ON.

Could you check again with https://github.com/prabhuk25/PublicRepo?

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.44%. Comparing base (e9aa836) to head (7a72998).

Files with missing lines Patch % Lines
...rd-frontend/src/components/ImportFromGit/index.tsx 66.66% 3 Missing ⚠️
...AdvancedOptions/CreateNewIfExistingField/index.tsx 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1656      +/-   ##
==========================================
- Coverage   92.45%   92.44%   -0.01%     
==========================================
  Files         609      609              
  Lines       63850    63877      +27     
  Branches     5011     5017       +6     
==========================================
+ Hits        59031    59054      +23     
- Misses       4754     4758       +4     
  Partials       65       65              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants