Conversation
…h> path (CRW-10950) When a workspace is created from a GitHub URL with an explicit branch (e.g. https://github.com/org/repo/tree/my-branch), getSameRepoWorkspaces() compares workspace.source against factoryParams.sourceUrl (both equal — the full URL including /tree/my-branch) AND workspace checkoutFrom.revision ('my-branch') against factoryParams.revision. factoryParams.revision was only populated from the ?revision= query parameter, never from the /tree/<branch> URL path segment. The revision mismatch ('my-branch' !== undefined) caused getSameRepoWorkspaces() to return no matches, so a duplicate workspace was created even when "Create New" was disabled. Fix: extend getRevision() in buildFactoryParams.ts to also extract the branch name from the factory URL's /tree/<branch> path when no explicit ?revision= parameter is present. The explicit query parameter still takes precedence. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1608 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl 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-1608", name: che-dashboard}]}}]" |
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
There was a problem hiding this comment.
Comprehensive PR Review - All Stages Complete
✅ APPROVED - This is an excellent fix with clear root cause analysis, minimal and well-scoped changes, and comprehensive test coverage.
Review Summary
Standard Review: ✅ Approve
Deep Review: ✅ Design is Sound
Impact Review: ✅ No Concerns
Highlights
- Exemplary PR description - Thorough root cause analysis with concrete examples of the workspace matching failure
- Correct abstraction layer - Fix placed in
getRevision()where it belongs, maintaining separation of concerns - Follows existing patterns - Uses the same
split('/tree/')approach asgetProjectFromLocation.tsfor consistency - Strong test coverage - 4 unit tests covering all scenarios (no param, query param, /tree/ path, precedence) plus integration test
- Zero operational impact - Pure correctness improvement with no new dependencies, no config changes, no API surface changes
Optional Suggestions
The review identified a few optional improvements that would strengthen the test suite and documentation:
- Test for branch names with slashes - Consider adding a test for
feature/branch-namepatterns (common Git convention, used in this very project) - Test for trailing-slash edge case - Document the behavior for malformed URLs like
/tree/(correctly returnsundefined) - Document the shared pattern - Add a cross-reference comment noting that both
getRevision()andgetProjectFromLocation.tsusesplit('/tree/'), so both need updating if this pattern changes
These are suggestions for future work, not blockers.
System-Level Analysis
- Observability: No new logging/metrics needed (synchronous parsing function)
- Failure handling: Correct - try/catch returns
undefinedon parse failure, which is the right fallback - Resource cleanup: N/A - no resources opened
- Security: No concerns - read-only URL parsing, no injection vectors
- Performance: Negligible - executes once during factory param construction
Verdict: Ready to merge. The fix correctly solves the duplicate workspace issue with no operational risk.
🤖 Automated review by che-ai-assistant powered by Claude Sonnet 4.5
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4, tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
- Add cross-reference comment in getRevision() pointing to
getProjectFromLocation.ts which uses the same split('/tree/')
pattern, so future changes to support other Git hosts update
both locations.
- Add two tests documenting edge-case behaviour already handled
by the code:
* branch names containing slashes (feature/CRW-10950)
* /tree/ with no branch name (empty string → undefined)
Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
New changes are detected. LGTM label has been removed. |
split('/tree/') cannot distinguish a branch name containing slashes
(feature/my-branch) from a branch + subdirectory path (main/src/file.ts).
GitHub resolves this ambiguity server-side. The test pins the known
behaviour and notes the tradeoff so future maintainers understand why
the full remainder is returned.
Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1608 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl 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-1608", name: che-dashboard}]}}]" |
1 similar comment
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1608 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl 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-1608", name: che-dashboard}]}}]" |
|
@olexii4: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Fixes duplicate workspace creation when a factory URL contains a GitHub
/tree/<branch>path and "Create New" is disabled.Root Cause
getSameRepoWorkspaces()inCheckExistingWorkspacesdetermines whether an existing workspace was created from the same repository by testing two conditions:When a workspace is created from
https://github.com/org/repo/tree/my-branch:workspace.sourceis stored as the full URL including/tree/my-branch.checkoutFrom.revision = 'my-branch'(extracted bygetProjectFromLocation.tswhich splits on/tree/).When the user returns with the same URL and "Create New" unchecked:
factoryParams.sourceUrlcorrectly equalsworkspace.source— the full URL including/tree/my-branch.factoryParams.revisionisundefinedbecausegetRevision()only reads the explicit?revision=query parameter, never the/tree/<branch>path segment.The revision mismatch (
'my-branch' !== undefined) makes every existing workspace fail the filter, sogetSameRepoWorkspaces()returns an empty list and a duplicate is created.URLs without an explicit branch are unaffected because both
workspace.checkoutFrom.revisionandfactoryParams.revisionareundefinedin that case.Fix
Extended
getRevision()inbuildFactoryParams.tsto also extract the branch name from the factory URL's/tree/<branch>path segment when no explicit?revision=query parameter is present:function getRevision(searchParams: URLSearchParams): string | undefined { - return searchParams.get(REVISION_ATTR) || undefined; + const revisionParam = searchParams.get(REVISION_ATTR) || undefined; + if (revisionParam) { + return revisionParam; + } + const factoryUrl = getFactoryUrl(searchParams); + if (factoryUrl) { + try { + const url = new URL(factoryUrl); + const parts = url.pathname.split('/tree/'); + if (parts.length > 1 && parts[1]) { + return parts[1]; + } + } catch { /* not a valid URL */ } + } + return undefined; }The explicit
?revision=query parameter still takes precedence.What issues does this PR fix or reference?
fixes https://redhat.atlassian.net/browse/CRW-10950
Is it tested? How?
https://github.com/<org>/<repo>/tree/<branch>Create the workspace and wait for it to reach Running.
Expected: the existing workspace is opened — no duplicate is created.
Before this fix: a new workspace would be created because the branch embedded in the
/tree/<branch>path was not matched against the existing workspace's revision, sogetSameRepoWorkspaces()always returned an empty list.Release Notes
Docs PR