Add "from-Toolbox" connection flow; auto port-forward; multi-conection#4
Merged
Conversation
Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
azatsarynnyy
changed the title
azatsarynnyy
changed the title
Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntegrates OpenShift client access and CRD models, discovers DevWorkspace environments, adds connection lifecycle hooks to start workspaces and establish SSH port-forwards, provides dynamic port/credential callbacks to environment views, and updates plugin/provider wiring and build artifacts. ChangesOpenShift Dev Spaces Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
azatsarynnyy
changed the title
…mprovments; simplify from-Dashboard flow Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
azatsarynnyy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/src/main/kotlin/com/redhat/devtools/toolbox/EnvironmentRepository.kt (1)
58-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
startPolling()against duplicate poller jobs.Each call currently starts a new infinite polling loop. Re-entry can duplicate refresh traffic and race state updates.
Proposed fix
class EnvironmentRepository( @@ ) { + private var pollingJob: Job? = null @@ fun startPolling() { - coroutineScope.launch(CoroutineName("EnvironmentRepository-Polling")) { + if (pollingJob?.isActive == true) return + pollingJob = coroutineScope.launch(CoroutineName("EnvironmentRepository-Polling")) { // Initial fetch refreshEnvironments() @@ } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/main/kotlin/com/redhat/devtools/toolbox/EnvironmentRepository.kt` around lines 58 - 68, startPolling() currently launches a new infinite coroutine each call; add a nullable Job property (e.g., pollerJob) to EnvironmentRepository and guard startPolling() so it does not start a second poller if pollerJob?.isActive is true: if active, return; otherwise assign pollerJob = coroutineScope.launch(CoroutineName("EnvironmentRepository-Polling")) { ... } and ensure pollerJob is cleared when the coroutine completes or is cancelled. Use the existing coroutineScope, refreshEnvironments(), refreshInterval and isActive checks; optionally cancel any previous pollerJob before launching if you prefer restart behavior.
🧹 Nitpick comments (4)
CLAUDE.md (1)
26-28: 💤 Low valueAdd language specifier to fenced code block.
The architecture diagram code block should specify
textas the language to satisfy the Markdown linter and improve rendering consistency.📝 Proposed fix
-``` +```text DataSource → EnvironmentConfig → Repository → RemoteEnvironment → Provider → Toolbox UI</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@CLAUDE.mdaround lines 26 - 28, The fenced code block containing the
architecture line "DataSource → EnvironmentConfig → Repository →
RemoteEnvironment → Provider → Toolbox UI" lacks a language specifier; update
that code fence to include "text" (i.e., use ```text) so the Markdown linter
passes and rendering is consistent for the architecture diagram snippet.</details> </blockquote></details> <details> <summary>plugin/build.gradle.kts (1)</summary><blockquote> `3-3`: _💤 Low value_ **Remove or explain the commented-out code.** The commented `kotlin-dsl` plugin application has no explanation. If it's not needed, remove it; if it's temporarily disabled, add a comment explaining why. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/build.gradle.kts` at line 3, Remove the stray commented plugin marker "// `kotlin-dsl`" or replace it with a short explanatory comment; locate the top of plugin/build.gradle.kts where the kotlin-dsl mention appears and either delete that commented line if unused, or add a one-line reason (e.g., "temporarily disabled due to X" or "kept for reference - not applied because Y") so future readers understand why `kotlin-dsl` is present but not applied. ``` </details> </blockquote></details> <details> <summary>plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/DevSpacesRemoteEnvironment.kt (1)</summary><blockquote> `237-240`: _💤 Low value_ **Consider logging exec failures for debuggability.** `onFailure` silently counts down without logging the error, making it hard to diagnose issues when reading files from containers. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/DevSpacesRemoteEnvironment.kt` around lines 237 - 240, In DevSpacesRemoteEnvironment.kt update the ExecListener passed to .usingListener so its override fun onFailure(t: Throwable?, response: io.fabric8.kubernetes.client.dsl.ExecListener.Response?) logs the error and response details before calling latch.countDown(); specifically modify the anonymous io.fabric8.kubernetes.client.dsl.ExecListener implementation used in usingListener to call your class logger/processLogger with a descriptive message plus t and response (or their useful fields) so failures reading files from containers are recorded for debugging, then still call latch.countDown(). ``` </details> </blockquote></details> <details> <summary>plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshEnvironmentContentsViewFactory.kt (1)</summary><blockquote> `91-101`: _💤 Low value_ **`equals()` always returning `false` violates the contract and risks unexpected behavior.** While the comment explains this forces Toolbox to refresh cached SSH keys, this breaks the `equals`/`hashCode` contract. If `WorkspaceSshConnectionInfo` is ever used in hash-based collections (HashMap, HashSet), behavior will be incorrect. Consider overriding `hashCode()` to maintain the contract, even if `equals()` has this workaround. Alternatively, document this class must never be used in collections. <details> <summary>📝 Suggested documentation and hashCode override</summary> ```diff +/** + * Note: This class intentionally returns false from equals() to force Toolbox + * to refresh cached SSH keys. Do not use in hash-based collections. + */ private class WorkspaceSshConnectionInfo( private val portProvider: () -> Int, private val credentialsProvider: () -> SshCredentialsStore.Credentials? ) : SshConnectionInfo { // ... existing code ... override fun equals(other: Any?): Boolean { // ... existing comment and return false ... } + + override fun hashCode(): Int = System.identityHashCode(this) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshEnvironmentContentsViewFactory.kt` around lines 91 - 101, The equals() implementation currently always returns false which breaks the equals/hashCode contract; fix by implementing a proper equals and matching hashCode for the class (WorkspaceSshConnectionInfo or the class that defines equals()): compare the concrete identity fields used for SSH identity (e.g., privateKeys path, host, port, user, and any other connection-identifying properties) inside equals(other: Any?) and compute hashCode() from the same fields (use Kotlin's Objects.hash or Kotlin data class style formula) so instances with the same connection identity are equal and have the same hash; if the "always refresh key" behavior is required, instead add clear KDoc on the class that it must not be placed into hash-based collections and choose a consistent alternative (e.g., return System.identityHashCode(this) in hashCode) while keeping equals implemented appropriately. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@CLAUDE.md:
- Line 42: The doc line about SshEnvironmentContentsViewFactory is out of date:
update the sentence under SshEnvironmentContentsViewFactory (environment/) to
remove "supports only one active connection" and instead state that multiple
simultaneous SSH connections are supported (and mention whether the hardcoded
local port 2022 restriction remains or has been removed). Edit the CLAUDE.md
entry for SshEnvironmentContentsViewFactory to reflect the current behavior
(support for multiple connections) and clarify the port behavior accordingly.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/DevSpacesRemoteProvider.kt:
- Around line 69-72: The code reads dwID from queryParams and unconditionally
calls repository.updateConnectionRequest(dwID, true) which can receive an empty
string; validate that dwID is non-empty (and optionally well-formed) before
calling updateConnectionRequest on the repository, e.g., check the dwID variable
and if it's blank skip the call and log or return an error response; update the
logic around the dwID variable and the repository.updateConnectionRequest(...)
invocation to only proceed when dwID is present.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/DevSpacesRemoteEnvironment.kt:
- Around line 55-57: The fields activeClient and activePortForward are accessed
from multiple threads (written in beforeConnection() and read/written in
closePortForward()), causing a race; mark both properties with@Volatileor
guard all accesses with a synchronized block to ensure visibility and atomicity.
Locate the declarations of activeClient and activePortForward in
DevSpacesRemoteEnvironment and either add the Kotlin@Volatileannotation to
both properties or wrap reads/writes inside a single private lock object (e.g.,
synchronized(lock) { ... }) used consistently in beforeConnection(),
closePortForward(), and any other accessors to prevent races.- Around line 230-244: The code currently calls latch.await(10,
TimeUnit.SECONDS) and blindly returns output, risking silent failures; update
the method (in DevSpacesRemoteEnvironment) to capture the boolean result of
latch.await(10, java.util.concurrent.TimeUnit.SECONDS), and if it returns false
throw an appropriate exception (e.g., IOException or IllegalStateException) that
includes contextual details (podName, namespace, filePath) so callers know the
read timed out; also ensure the ExecListener onFailure surfaces the Throwable
(for example by storing it and including it in the thrown exception) so timeouts
and execution errors are not swallowed.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/PortAllocator.kt:
- Around line 26-36: The PortAllocator singleton uses a non-thread-safe
mutableMapOf in the allocatedPorts field, which can corrupt port mappings under
concurrent lifecycle events; change allocatedPorts to a thread-safe map (e.g.,
java.util.concurrent.ConcurrentHashMap or Collections.synchronizedMap) and
ensure the accessor methods get(environmentId), save(environmentId, port) and
release(environmentId) operate on that concurrent map (or wrap their bodies with
appropriate synchronization) so reads/writes are atomic and safe under
concurrent access.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshCredentialsStore.kt:
- Around line 31-41: The cache field in SshCredentialsStore is a plain
mutableMapOf (cache) and its accessors get, save, and release are called from
connection hooks on different threads; replace the non-thread-safe mutableMapOf
with a thread-safe concurrent map (e.g.,
java.util.concurrent.ConcurrentHashMap<String, Credentials> or
Collections.synchronizedMap) and update the cache declaration accordingly,
keeping the existing get(environmentId: String), save(environmentId: String,
credentials: Credentials) and release(environmentId: String) methods unchanged;
add the necessary import for ConcurrentHashMap if used.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/DevWorkspace.kt:
- Around line 42-47: The id field assignment in DevWorkspace.kt must not default
to an empty string; change the fallback so workspace identity is stable: use
status["devworkspaceId"] as? String ?: metadata.uid ?:
"${metadata.namespace}/${metadata.name}" (or similar namespace+name composite)
instead of ""; update the id assignment where id = status["devworkspaceId"] as?
String ?: "" to prefer metadata.uid then a namespace/name composite to avoid
collapsing distinct workspaces.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/DevWorkspaces.kt:
- Around line 148-154: The polling loop in DevWorkspaces.kt is catching
Exception around the call to get(namespace, name), which also swallows
kotlinx.coroutines.CancellationException; update the try/catch so cancellation
is not swallowed by either (a) add a specific catch for CancellationException
that rethrows it, or (b) narrow the caught exception type to a non-cancellation
exception (e.g., IOException/RuntimeException) and keep the existing
delay/continue behavior for those; reference the get(namespace, name) call and
the catch block in DevWorkspaces.kt and ensure CancellationException is rethrown
immediately.- Around line 118-124: The current start(namespace: String, name: String) uses a
JSON Patch with op "replace" for "/spec/started" which fails if spec.started is
absent; change the patch payload to use op "add" (e.g.
[{"op":"add","path":"/spec/started","value":true}]) so the field is created when
missing, keep using
client.genericKubernetesResources(devWorkspaceContext).inNamespace(namespace).withName(name).patch(PatchContext.of(PatchType.JSON),
...) and update the patch string accordingly before the logger.info call.
Outside diff comments:
In@plugin/src/main/kotlin/com/redhat/devtools/toolbox/EnvironmentRepository.kt:
- Around line 58-68: startPolling() currently launches a new infinite coroutine
each call; add a nullable Job property (e.g., pollerJob) to
EnvironmentRepository and guard startPolling() so it does not start a second
poller if pollerJob?.isActive is true: if active, return; otherwise assign
pollerJob =
coroutineScope.launch(CoroutineName("EnvironmentRepository-Polling")) { ... }
and ensure pollerJob is cleared when the coroutine completes or is cancelled.
Use the existing coroutineScope, refreshEnvironments(), refreshInterval and
isActive checks; optionally cancel any previous pollerJob before launching if
you prefer restart behavior.
Nitpick comments:
In@CLAUDE.md:
- Around line 26-28: The fenced code block containing the architecture line
"DataSource → EnvironmentConfig → Repository → RemoteEnvironment → Provider →
Toolbox UI" lacks a language specifier; update that code fence to include "text"
(i.e., use ```text) so the Markdown linter passes and rendering is consistent
for the architecture diagram snippet.In
@plugin/build.gradle.kts:
- Line 3: Remove the stray commented plugin marker "//
kotlin-dsl" or replace
it with a short explanatory comment; locate the top of plugin/build.gradle.kts
where the kotlin-dsl mention appears and either delete that commented line if
unused, or add a one-line reason (e.g., "temporarily disabled due to X" or "kept
for reference - not applied because Y") so future readers understand why
kotlin-dslis present but not applied.In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/DevSpacesRemoteEnvironment.kt:
- Around line 237-240: In DevSpacesRemoteEnvironment.kt update the ExecListener
passed to .usingListener so its override fun onFailure(t: Throwable?, response:
io.fabric8.kubernetes.client.dsl.ExecListener.Response?) logs the error and
response details before calling latch.countDown(); specifically modify the
anonymous io.fabric8.kubernetes.client.dsl.ExecListener implementation used in
usingListener to call your class logger/processLogger with a descriptive message
plus t and response (or their useful fields) so failures reading files from
containers are recorded for debugging, then still call latch.countDown().In
@plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshEnvironmentContentsViewFactory.kt:
- Around line 91-101: The equals() implementation currently always returns false
which breaks the equals/hashCode contract; fix by implementing a proper equals
and matching hashCode for the class (WorkspaceSshConnectionInfo or the class
that defines equals()): compare the concrete identity fields used for SSH
identity (e.g., privateKeys path, host, port, user, and any other
connection-identifying properties) inside equals(other: Any?) and compute
hashCode() from the same fields (use Kotlin's Objects.hash or Kotlin data class
style formula) so instances with the same connection identity are equal and have
the same hash; if the "always refresh key" behavior is required, instead add
clear KDoc on the class that it must not be placed into hash-based collections
and choose a consistent alternative (e.g., return System.identityHashCode(this)
in hashCode) while keeping equals implemented appropriately.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `be74ff6a-c118-41ac-a07b-f470d43f9558` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e65b946554887de304f39b6c7bc30739fa1b2155 and 8576f559a9ae27d27d83424aca183c98402ba9a2. </details> <details> <summary>📒 Files selected for processing (18)</summary> * `CLAUDE.md` * `build-logic/src/main/kotlin/toolbox/buildlogic/InstallToolboxPlugin.kt` * `plugin/build.gradle.kts` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/DevSpacesPlugin.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/DevSpacesRemoteProvider.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/EnvironmentRepository.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/datasource/DevWorkspacesDataSource.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/datasource/EnvironmentDataSource.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/DevSpacesRemoteEnvironment.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/EnvironmentContentsViewFactory.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/PortAllocator.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshCredentialsStore.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/environment/SshEnvironmentContentsViewFactory.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/DevWorkspace.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/DevWorkspaceTemplate.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/DevWorkspaces.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/OpenShiftClientFactory.kt` * `plugin/src/main/kotlin/com/redhat/devtools/toolbox/openshift/Projects.kt` </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * plugin/src/main/kotlin/com/redhat/devtools/toolbox/datasource/EnvironmentDataSource.kt </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
…evWorkspaces.kt Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Artem Zatsarynnyi <azatsary@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://redhat.atlassian.net/browse/CRW-10454 ("from Toolbox" connection flow; connecting to existing CDEs w/o the Dashboard usage)
https://redhat.atlassian.net/browse/CRW-10451 (manage port forwarding automatically, w/o the user's involvement)
https://redhat.atlassian.net/browse/CRW-10452 (multi-connection support)
Changes:
Before this patch, there was a single way to connect - from the Dashboard. This patch introduces the "from Toolbox" flow:
current-contextin the localkubeconfigConnectcommand does the following steps:Port-forwarding is handled automatically, transparently for the user. No need to execute the
oc port-forward ...command manually anymore.UI/UX enhancements:
Empty pagesection at the top of the Toolbox windowDeleteaction from theEnvironment actionsmenu, as we don't need it for now:Short demo of current state:
Screen.Recording.2026-04-30.at.19.09.35.mov