feat: add summary section to workspace create/edit by paulovmr · Pull Request #966 · kubeflow/notebooks

paulovmr · 2026-03-12T18:11:45Z

closes #959

demo-summary.webm

Signed-off-by: paulovmr <832830+paulovmr@users.noreply.github.com>

google-oss-prow · 2026-03-12T18:11:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from paulovmr. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

workspaces/frontend/OWNERS

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

caponetto · 2026-03-12T20:45:09Z

/assign

paulovmr · 2026-03-13T10:58:32Z

/ok-to-test

caponetto

Left a few comments inline, please check if they make sense. :)
Here are some things I noticed and wrote down to consider as FUP:

When navigating back to the Workspace Kind selection step after completing subsequent steps, consider showing a confirmation dialog warning that changing the kind will reset all previous selections (image, pod config, properties). This would prevent accidental data loss. Only applicable to create mode though, since workspace kind cannot be changed in edit mode.
The delayed hide on the summary redirect popover is a nice touch. It allows users to copy text and interact with the "Switch to" button. Two suggestions:
1. Consider applying the same delayed-hide behavior to the redirect popovers on the Image and Pod Config selection cards in the grid view, for consistency.
2. Extend the hover-sensitive area to cover the entire popover (including the header and close button). Currently, the popover can be dismissed before the cursor reaches those areas.
Not blocking, but something to consider for a follow-up: the pencil (edit) icon on each summary card suggests inline editing, when it actually navigates to the corresponding wizard step. This could be slightly misleading, especially in edit mode for the Workspace Kind card, where clicking the icon navigates to a step where no change is allowed. A different icon (e.g., an arrow or "go to" indicator) or disabling the button for non-editable sections in edit mode might better communicate the intended action.

caponetto · 2026-03-13T11:31:46Z

.../frontend/src/__tests__/cypress/cypress/tests/mocked/workspaces/summaryRedirectPopover.cy.ts

+
+const DEFAULT_NAMESPACE = 'default';
+
+describe.skip('Summary Redirect Popover - Delayed Hide Behavior', () => {


Can we add a ticket reference associated with the reason for skipping the entire test so we don't lose track of it?

caponetto · 2026-03-13T11:33:18Z

.../frontend/src/__tests__/cypress/cypress/tests/mocked/workspaces/summaryRedirectPopover.cy.ts

+      createWorkspace.clickNext(); // Properties
+
+      // Find redirect icon in summary
+      cy.get('[data-testid="redirect-icon-1-new"]').should('exist');


Please replace all occurrences of cy.get('[data-testid=" with cy.findByTestId(.

caponetto · 2026-03-13T11:34:54Z

.../frontend/src/__tests__/cypress/cypress/tests/mocked/workspaces/summaryRedirectPopover.cy.ts

+
+      // Hover to show popover
+      cy.get('[data-testid="redirect-icon-1-new"]').trigger('mouseenter');
+      cy.get('.pf-v6-c-popover').should('be.visible');


Can we use data-testid instead of cy.get('.pf-v6-c-popover') (this could match with unwanted other things)?

caponetto · 2026-03-13T11:38:38Z

...es/frontend/src/__tests__/cypress/cypress/tests/mocked/workspaces/workspaceFormStyling.cy.ts

+    it('should apply workspace-option-card__header--with-icons class to cards with hidden flag', () => {
+      createWorkspace.checkExtraFilter('showHidden');
+
+      cy.get('#hidden-image .pf-v6-c-card__header').should(


Consider using data-testid (entire file)

caponetto · 2026-03-13T11:42:07Z

.../frontend/src/__tests__/cypress/cypress/tests/mocked/workspaces/summaryRedirectPopover.cy.ts

+    it('should hide popover after 1 second if mouse does not enter popover', () => {
+      createWorkspace.clickNext(); // Pod config
+      createWorkspace.clickNext(); // Properties
+
+      // Hover to show popover
+      cy.get('[data-testid="redirect-icon-1-new"]').trigger('mouseenter');
+      cy.get('.pf-v6-c-popover').should('be.visible');
+
+      // Move mouse away from icon
+      cy.get('[data-testid="redirect-icon-1-new"]').trigger('mouseleave');
+
+      // Popover should be hidden after delay (using clock.tick would require cy.clock() setup)
+      cy.get('.pf-v6-c-popover', { timeout: 2000 }).should('not.be.visible');
+    });


This is maybe a nitpick but I believe tests should tell a story that is simple to read. Complexity should be moved to page objects. It also avoids the need to add comments on every line.

Example:

createWorkspace.hoverPopover... createWorkspace.assertPopoverVisible(true) createWorkspace.moveMouseAway... createWorkspace.assertPopoverVisible(false)

caponetto · 2026-03-13T11:46:10Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

@@ -0,0 +1,654 @@
+import React, { useCallback, useState, useRef, useEffect } from 'react';


Consider extracting the diff rendering, the properties section, and the normalizeLabels helper into separate components or utility functions to improve readability and testability.

caponetto · 2026-03-13T11:49:11Z

workspaces/frontend/src/app/pages/Workspaces/Form/SummaryRedirectIcon.tsx

+    };
+    onClickTarget: () => void;
+  }) => React.ReactNode;
+  hideTimeoutRef: React.MutableRefObject<NodeJS.Timeout | null>;


There are 6 occurrences of // eslint-disable-next-line no-param-reassign to mutate ref .current values. While mutating refs is legitimate in React, the pattern of passing refs as props and mutating them from a child component is unusual. Consider managing the hideTimeoutRef and isHoveringPopoverRef state internally within WorkspaceFormSummaryPanel (where the refs are owned) and passing down only the necessary callbacks, which would eliminate the need for these lint suppressions entirely.

thaorell

Hi Paulo, thank you for the work. The summary section is very helpful to recap information. There are two issues I want to highlight:

The right side panel could contain a lot of information that needs to be displayed (For example, an Image that has a very long description about the image, or PodConfig with a lot of hardware information). We could make Summary an additional step in the WorkspaceForm component (at least for Workspace Create), and the cards should be clickable card that navigates the user back to the Step they click on. That way, there is more real estate on the Summary page (maybe even a YAML view in the future)
The OLD vs NEW section is very valuable on Edit, but comparisons are usually done side by side. We could consider a Split layout for better comparison view

thaorell · 2026-03-13T15:20:43Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

+
+      return (
+        <StackItem key={step}>
+          <Card isCompact>


Consider a clickable card instead of the Pencil Icon button

thaorell · 2026-03-13T15:27:43Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

+                    </StackItem>
+                    <Divider />
+                    {/* OLD section */}
+                    <StackItem>


For a side-by-side comparison, consider using Split instead of Stack, because it is easier to translate and compare the information.

thaorell · 2026-03-13T15:28:12Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

+                    {/* OLD section */}
+                    <StackItem>
+                      <Content component={ContentVariants.p}>
+                        <strong>OLD:</strong>


<strong>Current:</strong>

thaorell · 2026-03-13T15:28:35Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

+                    {/* NEW section */}
+                    <StackItem>
+                      <Content component={ContentVariants.p}>
+                        <strong>NEW:</strong>


<strong>Current:</strong>

thaorell · 2026-03-13T15:33:15Z

workspaces/frontend/src/app/pages/Workspaces/Form/WorkspaceFormSummaryPanel.tsx

+              </StackItem>
+              {redirect.message?.text && <StackItem>{redirect.message.text}</StackItem>}
+              <StackItem>
+                <Button


I would consider not having the Button in the Popover, actions should be performed after the user navigates back to the Step.

feat: add summary section to workspace create/edit

009a9b1

Signed-off-by: paulovmr <832830+paulovmr@users.noreply.github.com>

github-project-automation bot added this to Kubeflow Notebooks Mar 12, 2026

github-project-automation bot moved this to Needs Triage in Kubeflow Notebooks Mar 12, 2026

google-oss-prow bot added the area/frontend area - related to frontend components label Mar 12, 2026

google-oss-prow bot requested review from ederign and thesuperzapper March 12, 2026 18:11

google-oss-prow bot added area/v2 area - version - kubeflow notebooks v2 size/XXL labels Mar 12, 2026

google-oss-prow bot assigned caponetto Mar 12, 2026

google-oss-prow bot added the ok-to-test label Mar 13, 2026

caponetto reviewed Mar 13, 2026

View reviewed changes

thaorell suggested changes Mar 13, 2026

View reviewed changes

google-oss-prow bot assigned thaorell Mar 13, 2026


		const DEFAULT_NAMESPACE = 'default';

		describe.skip('Summary Redirect Popover - Delayed Hide Behavior', () => {

		@@ -0,0 +1,654 @@
		import React, { useCallback, useState, useRef, useEffect } from 'react';

Conversation

paulovmr commented Mar 12, 2026

Uh oh!

google-oss-prow bot commented Mar 12, 2026

Uh oh!

caponetto commented Mar 12, 2026

Uh oh!

paulovmr commented Mar 13, 2026

Uh oh!

caponetto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaorell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caponetto left a comment •

edited

Loading