CONSOLE-5183: Add persistent pod terminal sessions to Cloud Shell drawer#16269
CONSOLE-5183: Add persistent pod terminal sessions to Cloud Shell drawer#16269BabbarPB08 wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@BabbarPB08: No Jira issue with key OCPBUGS-0000 exists in the tracker at https://redhat.atlassian.net. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BabbarPB08 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 |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @BabbarPB08. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
@BabbarPB08: No Jira issue with key OCPBUGS-0000 exists in the tracker at https://redhat.atlassian.net. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request introduces detached cloud shell sessions, enabling users to open individual pod terminal connections as separate, persistent tabs within the cloud shell drawer. The implementation adds a new ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-reducer.ts (1)
11-53:⚠️ Potential issue | 🟠 MajorDetached-session cap/eviction logic does not match required behavior.
This reducer currently uses cap
8and drops new sessions at limit. Required behavior is max 5 detached sessions with oldest eviction.Proposed fix
-const MAX_DETACHED_SESSIONS = 8; +const MAX_DETACHED_SESSIONS = 5; ... case Actions.AddDetachedSession: { - if (state.detachedSessions.length >= MAX_DETACHED_SESSIONS) { - return state; - } - if (state.detachedSessions.some((s) => s.id === action.payload.id)) { - return state; - } + const withoutDuplicate = state.detachedSessions.filter((s) => s.id !== action.payload.id); + const next = [...withoutDuplicate, action.payload].slice(-MAX_DETACHED_SESSIONS); return { ...state, - detachedSessions: [...state.detachedSessions, action.payload], + detachedSessions: next, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-reducer.ts` around lines 11 - 53, The detached-session cap and eviction are wrong: change MAX_DETACHED_SESSIONS from 8 to 5 and update the AddDetachedSession handling so that when adding a new session (in the reducer case Actions.AddDetachedSession) you still skip duplicates (state.detachedSessions.some(...)) but instead of returning state when length >= MAX_DETACHED_SESSIONS you evict the oldest session and append the new one (e.g., create a new detachedSessions array by slicing off the first element and concatenating the incoming action.payload) so the array never exceeds MAX_DETACHED_SESSIONS and oldest sessions are removed first.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsx`:
- Around line 58-64: When switching focus to a detached tab in
MultiTabbedTerminal (the useEffect that watches detachedSessions and calls
setActiveTabKey with `${DETACHED_PREFIX}${newest.id}`), we must also stop the
activity keepalive by clearing tickNamespace and tickWorkspace so the RAF loop
no longer calls sendActivityTick for the old Cloud Shell; update the same logic
paths (the detached switch at lines referenced and any other tab-switch handlers
around handleTabChange/openTab/closeTab) to explicitly reset tickNamespace and
tickWorkspace (or invoke the existing stop/cleanup method that halts
sendActivityTick) whenever the active tab becomes a detached tab (detect via
DETACHED_PREFIX) or when the last Cloud Shell tab is closed so the RAF loop
stops.
- Around line 26-27: The component currently initializes and updates
terminalTabs and activeTabKey based on counts which breaks when detachedSessions
mount with existing items or when sessions are replaced; update logic to derive
terminalTabs and activeTabKey from detachedSessions identities (e.g., use
detachedSessions.map(s => s.id) or keys) rather than detachedSessions.length,
initialize useState from that identity list, and add a useEffect watching
detachedSessions to: 1) replace terminalTabs with the current detached session
ids, and 2) if activeTabKey is missing or points to a removed id,
setActiveTabKey to the first detached session id (or a sensible fallback).
Target the functions/state variables terminalTabs, setTerminalTabs,
activeTabKey, setActiveTabKey and the detachedSessions reducer-derived value.
In
`@frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-dispatchers.ts`:
- Around line 53-59: The click handler toggles expansion when isExpanded ===
false but detachedSessions.length > 0, causing the drawer to open instead of
invoking the detached-session close flow; update the useCallback in
cloud-shell-dispatchers so that if detachedSessions.length > 0 you call
confirmClose() (or the existing detached-session clear/close routine) regardless
of isExpanded, otherwise fall back to
dispatch(setCloudShellExpanded(!isExpanded)); adjust the conditional logic
around isExpanded, isActive, detachedSessions.length and keep confirmClose,
setCloudShellExpanded, isExpanded, isActive and detachedSessions references
intact.
In `@frontend/public/components/pod-connect.tsx`:
- Around line 160-172: The detached session is missing the shell/command context
so DetachedPodExec falls back to /bin/sh; update the detachToCloudShell caller
to include the current exec command when dispatching addDetachedSession (e.g.,
add a command property populated from the current session state/variable that
holds the shell or exec command), and ensure the DetachedPodExec consumer reads
that command from the DetachedSession object to use instead of defaulting to
/bin/sh; adjust the addDetachedSession payload and any related types/interfaces
accordingly so the command is persisted and restored on reconnect.
---
Outside diff comments:
In
`@frontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-reducer.ts`:
- Around line 11-53: The detached-session cap and eviction are wrong: change
MAX_DETACHED_SESSIONS from 8 to 5 and update the AddDetachedSession handling so
that when adding a new session (in the reducer case Actions.AddDetachedSession)
you still skip duplicates (state.detachedSessions.some(...)) but instead of
returning state when length >= MAX_DETACHED_SESSIONS you evict the oldest
session and append the new one (e.g., create a new detachedSessions array by
slicing off the first element and concatenating the incoming action.payload) so
the array never exceeds MAX_DETACHED_SESSIONS and oldest sessions are removed
first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 73755716-b690-47b9-b8a4-7b67e60e6ca4
📒 Files selected for processing (9)
frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShell.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellDrawer.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/DetachedPodExec.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsxfrontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.tsfrontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-dispatchers.tsfrontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-reducer.tsfrontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-selectors.tsfrontend/public/components/pod-connect.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellDrawer.tsxfrontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-selectors.tsfrontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShell.tsxfrontend/public/components/pod-connect.tsxfrontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.tsfrontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-dispatchers.tsfrontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-reducer.tsfrontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/DetachedPodExec.tsx
🔇 Additional comments (5)
frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShellDrawer.tsx (1)
79-88: Good feature-flag guard for the external terminal link.This keeps the action hidden when DevWorkspace is unavailable and prevents a dead-end UX path.
frontend/packages/webterminal-plugin/src/redux/reducers/cloud-shell-selectors.ts (1)
28-33: Selector + hook addition looks solid.The null-safe fallback to
[]is a good defensive default for downstream tab logic.frontend/packages/webterminal-plugin/src/components/cloud-shell/CloudShell.tsx (1)
23-27: This correctly enables detached-session-only drawer rendering.The
open || hasDetachedSessionsbehavior aligns with the persistent detached tab UX.frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-actions.ts (1)
4-44: Action/type additions are clean and consistent.The detached-session action surface is well-structured and integrates cleanly into the existing typed action union.
frontend/packages/webterminal-plugin/src/components/cloud-shell/DetachedPodExec.tsx (1)
51-134: WebSocket lifecycle and reconnect flow are implemented cleanly.Connection setup, teardown, and error-to-reconnect UX are well structured for detached terminal sessions.
frontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsx
Show resolved
Hide resolved
frontend/packages/webterminal-plugin/src/components/cloud-shell/MultiTabbedTerminal.tsx
Show resolved
Hide resolved
frontend/packages/webterminal-plugin/src/redux/actions/cloud-shell-dispatchers.ts
Show resolved
Hide resolved
BabbarPB08
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@BabbarPB08: This pull request references CONSOLE-5183 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@BabbarPB08: This pull request references CONSOLE-5183 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
BabbarPB08
force-pushed
the
feat/persistent-terminal-sessions
branch
from
cc61cce to
fa69c1d
Compare
Integrates detachable terminal sessions into the existing Cloud Shell drawer, allowing pod and node debug terminals to persist across page navigation. Key changes: - WebSocket handoff: when detaching, the live WebSocket is transferred from PodConnect to DetachedPodExec via a global registry, avoiding the need for a second exec/attach connection. - PodConnect skips sending 'exit' on cleanup when a session is detached, keeping the underlying pod alive. - Node debug pods use stdinOnce:false so the container survives attach disconnection; namespace cleanup is skipped for detached sessions. - Debug terminal pod deletion is skipped when the pod is detached. - Session limit capped at 5 with the detach button disabled at the cap and a counter badge shown in the drawer header. - Cloud Shell keepalive ticks are suppressed for detached pod tabs. - Drawer close handler properly clears detached sessions.
BabbarPB08
force-pushed
the
feat/persistent-terminal-sessions
branch
from
fa69c1d to
6dd5268
Compare
|
@BabbarPB08: This pull request references CONSOLE-5183 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@BabbarPB08: This pull request references CONSOLE-5183 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@BabbarPB08: This pull request references CONSOLE-5183 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
2 participants
CONSOLE Features and Fixes
Jira: https://issues.redhat.com/browse/RFE-6966
Solution description
Adds the ability to "detach" a pod or node debug terminal session into the Cloud Shell drawer so it persists across page navigation. This addresses a long-standing RFE where users lose their terminal session when navigating away from a pod's terminal tab.
Changes:
cloud-shell-actions.ts,cloud-shell-reducer.ts,cloud-shell-selectors.ts):New actions, reducer state, and selectors for managing detached sessions
(X/5 detached)exiton cleanup when detached; disables button at session limit with informative tooltipstdinOnce: falseso debug pods survive attach disconnection; preserves debug namespace when session is detachedKey behaviors:
(X/5 detached)shown in drawer header when detached sessions existstdinOnce:falsekeeps the pod alive after the original attach disconnects; namespace cleanup is skipped for detached sessionsTest cases
Additional info
Screenshots