Update cluster storage modal

[pnaik1]

https://issues.redhat.com/browse/RHOAIENG-1098

Description

Refer the mock the design

Screen.Recording.2024-11-28.at.5.01.05.PM.mov

How Has This Been Tested?

Refer mocks to cover all the checks

1. Create a cluster storage and attach the existing workbench
2. Check for default mount path
3. Verify the validation path ( We are now allowing numbers)
4. create the cluster storage and check if the mountpath is correctly updated to workbench
5. Edit the existing cluster and check and change the workbench

Test Impact

Added cypress test

Request review criteria:

Self checklist (all need to be checked):

• The developer has manually tested the changes and verified that the changes work
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

• Included any necessary screenshots or gifs if it was a UI change.
• Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

• The developer has tested their solution on a cluster by using the image produced by the PR to main

[pnaik1]

cc @xianli123 , I have updated the description with required screenshot

when custom path is empty, Currently I have displayed this error, let me know your thoughts around here

[codecov]

Codecov Report

Attention: Patch coverage is 88.58447% with 25 lines in your changes missing coverage. Please review.

Project coverage is 85.40%. Comparing base (8484cd3) to head (0d57103).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/src/api/k8s/notebooks.ts	7.69%	12 Missing ⚠️
...cts/screens/detail/storage/ClusterStorageModal.tsx	93.57%	7 Missing ⚠️
.../screens/detail/storage/ClusterStorageTableRow.tsx	92.68%	3 Missing ⚠️
...src/pages/projects/screens/detail/storage/utils.ts	95.74%	2 Missing ⚠️
...rc/pages/projects/screens/spawner/storage/utils.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3529      +/-   ##
==========================================
- Coverage   85.54%   85.40%   -0.14%     
==========================================
  Files        1373     1378       +5     
  Lines       31195    31426     +231     
  Branches     8736     8797      +61     
==========================================
+ Hits        26686    26840     +154     
- Misses       4509     4586      +77     

Files with missing lines	Coverage Δ
frontend/src/components/table/TableBase.tsx	100.00% <ø> (ø)
frontend/src/pages/projects/notebook/utils.ts	33.72% <ø> (-2.23%)	⬇️
...rojects/screens/detail/storage/StorageTableRow.tsx	97.36% <100.00%> (ø)
...ects/screens/detail/storage/clusterTableColumns.ts	100.00% <100.00%> (ø)
.../src/pages/projects/screens/detail/storage/data.ts	44.44% <ø> (ø)
...ts/screens/spawner/storage/ClusterStorageTable.tsx	59.37% <100.00%> (+0.64%)	⬆️
...ages/projects/screens/spawner/storage/constants.ts	100.00% <ø> (ø)
...rc/pages/projects/screens/spawner/storage/types.ts	100.00% <100.00%> (ø)
frontend/src/pages/projects/types.ts	100.00% <ø> (ø)
...rc/pages/projects/screens/spawner/storage/utils.ts	65.71% <66.66%> (ø)
... and 4 more

... and 29 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8484cd3...0d57103. Read the comment docs.

[dpanshug]

There should be label ("Connected") for already connected workbenches, check here

[DaoDaoNoCode]

There is an edge case where the newly added mount path will be reset to default. Here is the step to reproduce:

1. Edit storage A which already has a workbench connection
2. Add workbench connection and edit the mount path
3. Click minus icon to remove the existing connection
4. See the newly added connection mount path is set to default, user will lose the edited value in this case

[xianli123]

Thanks @pnaik1.
I want to confirm how to trigger the popover as you shared (I attached it below). Does the 'View connection paths' trigger the popover?
A small enhancement: The bottom border of the row is missing. It will LGTM, once the border is added.

[pnaik1]

@xianli123 Thanks for the review, the popover will trigger when clicked on View connected paths , I have added the border in the latest changes

Screen.Recording.2024-12-02.at.5.32.13.PM.mov

[xianli123]

@pnaik1 Thanks for doing it! LGTM!

[Gkrumbach07]

wide screens have dropdown show over the modal

[Gkrumbach07]

there should be a slash in front of the custom ones

[Gkrumbach07]

root standard storage path doesn't get a checkmark

[pnaik1]

Updated the pr

[pnaik1]

Updated the PR accordingly

[Gkrumbach07]

This all looks good. just need to still fix the modal overflow issue: #3529 (comment)

[DaoDaoNoCode]

@pnaik1 Can you rebase the PR please? Want to see how it looks like on PF6

[DaoDaoNoCode]

Left some comments based on my testing. I am not a fan of the way to save id in the table column and hide it just in order to auto-generate the name, there should be a better way to save and use this information. 🤔 WDYT? @Gkrumbach07

[Gkrumbach07]

Left some comments based on my testing. I am not a fan of the way to save id in the table column and hide it just in order to auto-generate the name, there should be a better way to save and use this information. 🤔 WDYT? @Gkrumbach07

we should be autogenerating the name when we create it and not in the row

[pnaik1]

@DaoDaoNoCode @Gkrumbach07 Thanks I have updated the pr accordingly
I have a question around this,
Since the rows are dynamic and are editable, there is a required for unique stable column, removing this it aligns the row based on name which disturbs the flow
I see same think done here: https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/screens/spawner/storage/constants.ts#L6-L11
is there any better way handling this??

Left some comments based on my testing. I am not a fan of the way to save id in the table column and hide it just in order to auto-generate the name, there should be a better way to save and use this information. 🤔 WDYT? @Gkrumbach07

we should be autogenerating the name when we create it and not in the row

[DaoDaoNoCode]

hi @pnaik1, if that's the case, could you also update https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/screens/spawner/storage/constants.ts#L10 to use visibility prop and add a hidden Td under https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/screens/spawner/storage/ClusterStorageTable.tsx#L88 as well? I currently don't have a good idea how to refactor this.

[pnaik1]

@DaoDaoNoCode , I have updated the SpawnPage cluster storage table

[Gkrumbach07]

There seems to be a rendering issue. you have some infinite render loop when you add a new row to the table

i added comment with a fix: https://redhat-internal.slack.com/archives/C07C0FNHVPT/p1733851431547269?thread_ts=1733835610.145149&cid=C07C0FNHVPT

[Gkrumbach07]

/lgtm

looks great, thanks for the changes

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07

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

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/OWNERS [Gkrumbach07]

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