Storage UI: Delete Drive (in proposal; Remove the configuration for this device)#1934
Storage UI: Delete Drive (in proposal; Remove the configuration for this device)#1934mvidner merged 8 commits intostorage-config-uifrom
Conversation
mvidner
force-pushed
the
delete-proposed-drive
branch
from
9dd9190 to
ad15af6
Compare
|
I will need help with this: |
Yes, it is a workflow problem because the combination of re-renders, hooks, and the mentioned undefined. The quick fix could be the one proposed at 8a9477d, but actually Agama should stop re-rendering components that are going to be unmounted. Let's go ahead and commit the right fix later, when all the storage interface pieces are more polished. |
|
When trying to code the unit test, I am most confused by the disconnected names. The test uses the component like: and IIUC the component returns this the test code then goes and this is where I lose my cool:
|
|
Thanks @dgdavid for the pointer:
But 3. still looks wrong. Let's focus on
|
|
Problem 3. resolved:
|
For the record, apart of the things already discussed, I forget mentioning in our IRC talk that this is the name you are giving to the param sent on form submission. As said in IRC, something for the developer, not the name from user POV. |
|
Hi @mvidner, Apologies for the delayed response to the doubts you mentioned in the description of commit 0b15433f0dbb18ae8deb6979d88bfdf3964af138. To be honest, I got a bit distracted by your attempt to use the TL;DR: from a testing perspective, the root of the problem was the missing mock for Of course, we have a lot of room for improvement at the code level, especially regarding the complex storage interface. As mentioned yesterday, that's another reason I prefer not to postpone adding tests: they not only help us ensure that the component behaves as intended and prevent breaking it with future additions, but they also help us spot such complexities and give us the opportunity to simplify them. That said, the error you included in the commit was just the final lines of the failing test output. However, if you scroll to the top, you'll find an interesting error at the beginning. As you can see in the If you take a look at the mentioned internal component, you'll notice it uses two hooks: jest.mock("~/queries/storage", () => ({
...jest.requireActual("~/queries/storage"),
useAvailableDevices: () => [sda],
}));It's a bit messy, but that's where we are for now. We've talked before about the need for a global mocking mechanism, similar to what After mocking Why? I don't know, but it’s worth having a look at https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/Menu/MenuItem.tsx to see if it can be improved—perhaps making the In the meantime, the test can be solved in two ways:
Either option is fine with me at this point, but be careful if you choose the first one and make sure we still provide useful information to the user, rather than just a vague "Do not use". Click to show/hide a proposed diffdiff --git a/web/src/components/storage/DriveEditor.test.tsx b/web/src/components/storage/DriveEditor.test.tsx
index fddf7a1dd..c82d3516f 100644
--- a/web/src/components/storage/DriveEditor.test.tsx
+++ b/web/src/components/storage/DriveEditor.test.tsx
@@ -66,7 +66,11 @@ const mockDrive: ConfigModel.Drive = {
const mockDeleteDrive = jest.fn();
const mockDeletePartition = jest.fn();
-// TODO: why does "~/queries/storage" work elsewhere??
+jest.mock("~/queries/storage", () => ({
+ ...jest.requireActual("~/queries/storage"),
+ useAvailableDevices: () => [sda],
+}));
+
jest.mock("~/queries/storage/config-model", () => ({
...jest.requireActual("~/queries/storage/config-model"),
useConfigModel: () => ({ drives: [mockDrive] }),
@@ -102,7 +106,7 @@ describe("RemoveDriveOption", () => {
await user.click(driveButton);
const drivesMenu = screen.getByRole("menu");
const deleteDriveButton = within(drivesMenu).getByRole("menuitem", {
- name: "Do not use",
+ name: /Do not use/,
});
await user.click(deleteDriveButton);
expect(mockDeleteDrive).toHaveBeenCalled();
diff --git a/web/src/components/storage/DriveEditor.tsx b/web/src/components/storage/DriveEditor.tsx
index 6b5122e4e..ee7b3393a 100644
--- a/web/src/components/storage/DriveEditor.tsx
+++ b/web/src/components/storage/DriveEditor.tsx
@@ -1,5 +1,5 @@
/*
- * Copyright (c) [2024] SUSE LLC
+ * Copyright (c) [2024-2025] SUSE LLC
*
* All Rights Reserved.
*
@@ -20,7 +20,7 @@
* find current contact information at www.suse.com.
*/
-import React, { useId, useRef, useState, startTransition } from "react";
+import React, { useId, useRef, useState } from "react";
import { useNavigate, generatePath } from "react-router-dom";
import { _, formatList } from "~/i18n";
import { sprintf } from "sprintf-js";
@@ -424,12 +424,7 @@ const DriveSelector = ({ drive, selected, toggleAriaLabel }) => {
driveHandler.switch(newDriveName);
setIsOpen(false);
};
- // const onToggle = () => setIsOpen(!isOpen);
- const onToggle = () => {
- console.log("onToggle BEFORE");
- startTransition(() => setIsOpen(!isOpen));
- console.log("onToggle AFTER");
- };
+ const onToggle = () => setIsOpen(!isOpen);
return (
<MenuContainer |
|
I have simply committed David's proposed change. This message...
It is very helpful, I actually wanted to know WHICH component suspended, but I chose to ask an AI and it gave me generic advice about console.log debugging. But I do not see the message with
I will make a card to remove |
(the menu item only appears when I first delete the root partition from it) except it crashes!? and I am pretty sure useDrive has worked before > Unexpected Application Error! > _useDrive4 is undefined > > RemoveDriveOption@https://localhost:10443/index.js:42094:22 > renderWithHooks@https://localhost:10443/index.js:84540:27 > updateFunctionComponent@https://localhost:10443/index.js:88666:20 > beginWork@https://localhost:10443/index.js:90689:16 > beginWork$1@https://localhost:10443/index.js:96514:14 > performUnitOfWork@https://localhost:10443/index.js:95645:12 > ... > 💿 Hey developer 👋 > You can provide a way better UX than this when your app throws errors by providing your own ErrorBoundary or errorElement prop on your route.
The interface was crashing when deleting a drive due to the `RemoveDriveOption` component attempting to destructure an `undefined` value after the delete action was performed. Most likely, after the delete action, a parent component in the tree is re-rendered, which causes `RemoveDriveOption` to re-render as well. As a quick fix, this commit postpones the destructuring until the value returned by the `useDrive` hook is known, returning early if it is `undefined`. A more refined solution is welcome, but will be addressed once the overall storage UI is more polished.
Imitated the partition test, but new fun!
test fails with:
A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator. To fix, updates that suspend should be wrapped with startTransition.
- mock useAvailableDevices - startTransition was a misleading advice given when th test errored with a suspension. - actually the name ends up as "Do not use Remove the configuration for this device" so match partially
mvidner
force-pushed
the
delete-proposed-drive
branch
from
b573dad to
89caba2
Compare
Prepare for releasing Agama 12: * #1858 * #1887 * #1890 * #1892 * #1893 * #1894 * #1896 * #1898 * #1899 * #1900 * #1901 * #1906 * #1908 * #1909 * #1910 * #1911 * #1912 * #1914 * #1915 * #1917 * #1919 * #1920 * #1921 * #1922 * #1923 * #1924 * #1926 * #1927 * #1928 * #1929 * #1930 * #1931 * #1932 * #1933 * #1934 * #1935 * #1936 * #1937 * #1938 * #1939 * #1942 * #1943 * #1945 * #1948 * #1949 * #1952 * #1953 * #1954 * #1955 * #1957 * #1958 * #1959 * #1961 * #1963 * #1964 * #1967 * #1969 * #1970 * #1971 * #1972 * #1973 * #1974 * #1975 * #1976 * #1977 * #1979 * #1980 * #1981 * #1982 * #1984 * #1986 * #1987 * #1988 * #1990 * #1991 * #1992 * #1993 * #1995 * #1996 * #1997 * #1999 * #2000 * #2001 * #2002 * #2003 * #2004 * #2005 * #2006 * #2007 * #2008 * #2009 * #2010 * #2011 * #2012 * #2013 * #2014 * #2015 * #2016 * #2017 * #2019 * #2021 * #2022 * #2025 * #2027 * #2029 * #2030 * #2031 * #2033 * #2034 * #2035 * #2036 * #2037 * #2038 * #2039 * #2040 * #2045 * #2046 * #2050 * #2053 * #2054 * #2055 * #2056 * #2058 * #2060 * #2061 * #2062 * #2063 * #2064 * #2066 * #2067 * #2068 * #2069 * #2070 * #2071 * #2072 * #2073 * #2075 * #2076 * #2080 * #2082 * #2083
3 participants
Problem
https://trello.com/c/JQ3blpRm
Solution
The implementation was easier because the UI model already had a
removeDrivemethod...but testing was fun again, because imitating the tests that worked for deleting a partition was not enough this time 😅
Testing
Screenshots
In action (used Peek for recording, thanks Ancor for the tip)
nvme01but not possible because it contains//from it (Storage UI: Delete Partition (in proposal; trash-can icon) #1915)... (and we have an issue box, that's new but not from me 😄 )nvme01