Skip to content

Conversation

@joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Apr 1, 2025

Problem

The storage page shows an empty state (no devices found) after activating/deactivating devices.

Steps to reproduce:

  • Go to iSCSI page and activate devices.
  • Go back to storage.
  • The storage page shows "No devices found" instead of the configured devices and result.

Solution

Source of the problem: the query for getting the available devices requires the result of the query for getting all the devices, but it is not refreshed when there are changes in the list of devices.

For solving it, the useAvailableDevices hook uses useMemo which refreshes the list of available devices if any of the source data changes (either devices or the list of usable Devices).

See https://tkdodo.eu/blog/react-query-data-transformations#2-in-the-render-function: "Especially if you have additional logic in your custom hook to combine with your data transformation, this is a good option."

Testing

  • Tested manually

@joseivanlopez joseivanlopez changed the base branch from master to beta3 April 1, 2025 06:29
@joseivanlopez joseivanlopez marked this pull request as ready for review April 1, 2025 08:52
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care.

Indeed, it is an improvement that remind us we should book time for still revisiting these hooks and queries. Also for using the transformations and relying on select when possible, although you've told me offline that it was not applicable here.

import { StorageDevice } from "~/types/storage";

/**
* @fixme Use a transformation instead of building the devices as part of the fetch function, see
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +162 to +168
const findDevice = (devices: StorageDevice[], sid: number): StorageDevice | undefined => {
const device = devices.find((d) => d.sid === sid);
if (device === undefined) console.warn("Device not found:", sid);

return device;
};
Copy link
Contributor

@dgdavid dgdavid Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: If possible (and when possible) I'd extract that function from here. Looks like it can be defined somewhere and consumed by this (and potentially others) method instead of be (re)defined/hidden here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to add a TODO in order to keep the change as minimal as possible because it goes to beta3 branch.

@dgdavid
Copy link
Contributor

dgdavid commented Apr 1, 2025

for using the transformations and relying on select when possible

For clarifying, not only in storage but in other parts of Agama too since

You can use the select option to select a subset of the data that your component should subscribe to. This is useful for highly optimized data transformations or to avoid unnecessary re-renders.

@joseivanlopez joseivanlopez merged commit 231e2f4 into agama-project:beta3 Apr 1, 2025
1 check passed
@joseivanlopez joseivanlopez mentioned this pull request Apr 1, 2025
joseivanlopez added a commit that referenced this pull request Apr 1, 2025
@imobachgs imobachgs mentioned this pull request Apr 22, 2025
imobachgs added a commit that referenced this pull request Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants