Fix storage UI

[joseivanlopez]

Fix the storage UI after the migration to the new API.

Note: the failing tests will be adapted in a separate PR.

[jreidinger]

NP: overused sounds a bit strange. I kind of expect name like ALREADY_USED or IN_USE

[jreidinger]

name is also a bit inconsistent. I kind of expect something like NO_ALIAS

[jreidinger]

especially if you compare it with above NO_ROOT. I think issue should be either identified the part where is issue like ROOT, ALIAS, ENCRYPTION or by problem it describe like missing something, wrong entry, etc.

[jreidinger]

to be honest I am not sure on which level the issue classes acts...as here is generic search issue, but when alias is not found or root device is not found, it has dedicated entry. Overall I found it inconsistent in naming and in usage. For me if I need add new constant there, I will be lost how it should look like.

[ancorgs]

You are right. There should be no generic classes. Each class should correspond to a well defined issue. I'm working on a PR to fix that.

[jreidinger]

this looks strange. I think @imobachgs should probably check it as I kind of expect we should have soon proper type for product and software config here.

[joseivanlopez]

Note that this PR does not define the new types related to sofware and products. It only places the existing types in the correct location. All those types will be revisited when adapting the sofware and product code to the new API.

[teclator]

If it place the existing types in the correct location why product was removed from Config? I would expect some clarification somewhere because it is an storage PR but this is a product change.

[jreidinger]

I see this quite fragile comparison. When any new non config class will be added, then there will be problem. What about having prefixes in classes like config.no_root or config.label?

[joseivanlopez]

Actually @ancorgs initially proposed something like that. @ancorgs , what do you think?

[ancorgs]

Yes. That was my first thought. And I even proposed that to Imo and Iván.

But since using those prefixes is not a general idea used in other scopes and the comparison with "proposal" looked simpler, I decided to keep it simple.

I can bring back the original idea and add a "config" prefix.

[jreidinger]

I really dislike this...There is apiModel which is called model and here it defined another model which is extended version of model. This is extra confusing. I think it should really use different names like extendedModel or modelCalculated. In rust where it is easy to use namespace like proposal::Storage and system::Storage it is easy to have types which identical names, but in different namespace. But in typescript having types with identical name is quite hard to read the code, especially if they collide ( basically you always have to check imports in given file to see what it talks about ).

[joseivanlopez]

The extended version of the model is going to dead. There will be only one model. Note that the changes in this PR are intended to make the code work with the new API, but all the bad designs are kept. Refactoring will come in another iteration.

[dgdavid]

Thanks for the extensive work on adapting the interface to the new API, @joseivanlopez!

I've reviewed the web part, but not in depth since I understand this is more of an adaptation rather than a full refactor, although some improvements have already been made 👌 Also, a big thanks to @jreidinger, who have done a more thorough review!

Overall it LGTM. I’ve added a few comments, some related to the changes here and some not. Feel free to consider them as feedback for thought if you think more changes are not actually not needed here.

[dgdavid]

I'm all for not opening discussions about enforcing strict conventions, specially considering that we don't have a formal style guide in place yet, However, I personally prefer to continue using destructuring where possible when it comes to extract data from hooks. It looks like the new useSystem hook might be returning_ undefined_, but I’m wondering if there's a (clean) way to avoid it and return an empty collection instead.

[dgdavid]

Same applies for similar usage in other files in this PR.

[joseivanlopez]

I am not sure if at some point we will need to distinguish between no data vs not fetched.

[dgdavid]

Don't understand what you mean.

[joseivanlopez]

If we always report data (e.g., {keymaps: []}, locales: []}), then we are not able to know if we have already fetched the data from the api or the data is not available yet. But as said, I don't know if we will need that.

[dgdavid]

This could be the place where return an empty object instead of undefined.

return data || {}

[joseivanlopez]

If we decide to always return a list, then I would move it to the select function.

[dgdavid]

Not to be part of this PR, but we need to address the query keys thingy. Ideally, making it query export its id to be then reused. This would help to avoid typos in the future in a quite cheap way.

[dgdavid]

Not directly related to this PR, but when we tackle the queryKey thingy, we should consider following the recommendation for using query key factories (see https://tkdodo.eu/blog/effective-react-query-keys#use-query-key-factories, and #2447). By doing so, we could easily invalidate queries under the "proposal/" namespace without worrying about potential leftovers in the future.

[joseivanlopez]

Yes, we should use some robust way for defining queries. But I think we will not have nested queries anymore.

[dgdavid]

IMHO we have to take time to think about it. If these queries conceptually belongs to "proposal" I do not see a reason for not following key factory recommendation and put them under "proposal". It will make things better to work with.

[joseivanlopez]

I don't get your point. Which queries do you think can be under proposal?

[dgdavid]

Those you're invalidating here. The fact that you're invalidating them under certain event suggests that these queries could be grouped in some way. "Proposal" was just an example, but the key takeaway is that if you're frequently invalidating a set of queries together, they likely share a common parent or key.

[dgdavid]

Weird queryKey, not sure if we want to follow that path.

[joseivanlopez]

This code is not new. Any suggestion?

[dgdavid]

Not yet, as I do not have the full picture in mind at this time. Let's go ahead and change it when proceed.

[dgdavid]

For a future PR,

Suggested change

	const systemQuery = {
	queryKey: ["system"],
	queryFn: getSystem,
	};
	const KEY = "system";
	const systemQuery = {
	queryKey: [KEY],
	queryFn: getSystem,
	};

and then export KEY.

[teclator]

So, the file was removed here but not added in api/config/product.ts