Move and refactor filters component

[JSReds]

!CHANGE DEPENDENCY BEFORE MERGE!

Depends-on: balena-io-modules/ui-shared-components#89

Change-type: major

RENDITION LINES OF CODE:

/Filters
.tsx 1542
.ts 1644

/DataTypes
.ts 198
.tsx 1002

[myarmolinsky]

nit: getRefSchemePrefix? Seeing as you use refSchemePrefix as a parameter name elsewhere

[myarmolinsky]

Is '' possible?

Tiny nit for readability: perhaps trade the ternaries for ifs? Totally fine if you prefer it this way, just mentioning it

[myarmolinsky]

Very nice 😮
What about escape?

[JSReds]

added, thanks for pointing out

[myarmolinsky]

Not to be addressed during this PR, but I'm not quite happy with this property.

AssignDialog uses search and summary, however the user can still click the summary filters to change them into basically the same thing add does. Which means all we are excluding is views. Then DeviceLogs has all 4. Meanwhile, AutoUI has one Filter for add, search, and views, and then another Filter for summary.

So effectively what we amount to is we use all for all instances of Filters except the AssignDialog, for which we want to exclude views and the ability to add non-full-text-search filters (bugged atm), and for AutoUI, the main owner of filters, uses them in pieces instead of as a whole.

We should revisit this later for two things:

• The bug we see in AssignDialog where we hide add but can still access it via summary
• Analyzing whether we can reduce this to a single usecase

It's possible that whatever we choose to replace AutoUI will take care of this 🤞

[JSReds]

I agree, I also would like to remove some of the properties, for sure we can rethink renderMode, AutoUI has a limitation which is, having the summary in the same FIlters components make it really ugly to handle unfortunately. While doing it I also thought that we might soon switch to something different like a sidebar instead of having this filters view. If we rethink the graphics and the way we display them, we can even just remove some of these properties. It would be very nice

[myarmolinsky]

Why not include this among the SearchProps interface? 😅

[JSReds]

TextFieldProps type from Material uses conditional types based on the variant of the text field (filled, standard, outlined).
This force us to define Varinat = 'standard' ... it might be cleaner anyway 🤔
yep, changing it 👍

[myarmolinsky]

nit:

Suggested change

	if (isJSONSchema(schemaOrProperties) && schemaOrProperties.properties) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	properties: constructSchemaProperties(
	schemaOrProperties.properties,
	restRefScheme,
	description,
	) as JSONSchema['properties'],
	};
	}
	if (isJSONSchema(schemaOrProperties) && schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	items: constructSchemaProperties(
	schemaOrProperties.items as JSONSchema,
	restRefScheme,
	description,
	),
	};
	}
	if (isJSONSchema(schemaOrProperties)) {
	if (schemaOrProperties.properties) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	properties: constructSchemaProperties(
	schemaOrProperties.properties,
	restRefScheme,
	description,
	) as JSONSchema['properties'],
	};
	}
	if (schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	items: constructSchemaProperties(
	schemaOrProperties.items as JSONSchema,
	restRefScheme,
	description,
	),
	};
	}
	}

[JSReds]

🤦 don't know why I've repeated it... I just had to move it on top, changed it to so that it is not nested

[myarmolinsky]

nit:

Suggested change

	if (isJSONSchema(f) && f.anyOf) {
	return recursivelyRemoveSeparators(f);
	}
	if (!isJSONSchema(f) || !f.properties) {
	return f;
	}
	if (isJSONSchema(f) {
	if (f.anyOf) {
	return recursivelyRemoveSeparators(f);
	}
	} else {
	if (!f.properties) {
	return f;
	}
	}

[JSReds]

let me think about this, as I'm really against nested if, they make the readability really complex for not reason

[myarmolinsky]

Suggested change

	// If the current level is a JSON schema and has properties, recursively apply the transformation to these properties.
	if (schemaOrProperties.properties) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title, // Use provided title or default to existing title.
	description, // Apply the new description.
	properties: constructSchemaProperties(
	schemaOrProperties.properties,
	restRefScheme,
	description,
	) as JSONSchema['properties'], // Recurse into properties.
	};
	}
	// If the current level is a JSON schema and has an items definition, apply the transformation to the items.
	if (schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title, // Use provided title or default to existing title.
	description, // Apply the new description.
	items: constructSchemaProperties(
	schemaOrProperties.items as JSONSchema,
	restRefScheme,
	description,
	), // Recurse into items.
	};
	}
	if (schemaOrProperties.properties || schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	properties: constructSchemaProperties(
	schemaOrProperties.properties ?? (schemaOrProperties.items as JSONSchema),
	restRefScheme,
	description,
	) as JSONSchema['properties'],
	};
	}

(keep comments of course, I just removed them for the sake of showing purely the suggestion)

[JSReds]

I understand the point but we need to change properties and items in the returned object. Also, the as is different as 1 represent JSONSchema['properties'] and the other JSONSchema['items'].

🤔 we can for sure reduce the code, I just would like to avoid adding complexity. Let me give it a try

[JSReds]

we could do something like this:

  if (schemaOrProperties.properties || schemaOrProperties.items) {
    const properties = schemaOrProperties.properties
      ? (constructSchemaProperties(
          schemaOrProperties.properties,
          restRefScheme,
          description
        ) as JSONSchema["properties"])
      : undefined;
    const items: JSONSchema["items"] = schemaOrProperties.items
      ? constructSchemaProperties(
          schemaOrProperties.items as JSONSchema,
          restRefScheme,
          description
        )
      : undefined;
    return {
      ...schemaOrProperties,
      title: title ?? schemaOrProperties.title, // Use provided title or default to existing title.
      description, // Apply the new description.
      properties, // Recurse into properties.
      items, // Recurse into items.
    };
  }

WDYT ? would you find it more readable or nicer ?

[myarmolinsky]

I don't like the redundancy. Also I realized I made a mistake in my earlier suggestion, fixed here, though we need to do the cast:

Suggested change

	// If the current level is a JSON schema and has properties, recursively apply the transformation to these properties.
	if (schemaOrProperties.properties) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title, // Use provided title or default to existing title.
	description, // Apply the new description.
	properties: constructSchemaProperties(
	schemaOrProperties.properties,
	restRefScheme,
	description,
	) as JSONSchema['properties'], // Recurse into properties.
	};
	}
	// If the current level is a JSON schema and has an items definition, apply the transformation to the items.
	if (schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title, // Use provided title or default to existing title.
	description, // Apply the new description.
	items: constructSchemaProperties(
	schemaOrProperties.items as JSONSchema,
	restRefScheme,
	description,
	), // Recurse into items.
	};
	}
	if (schemaOrProperties.properties || schemaOrProperties.items) {
	return {
	...schemaOrProperties,
	title: title ?? schemaOrProperties.title,
	description,
	[schemaOrProperties.properties ? 'properties' : 'items']: constructSchemaProperties(
	schemaOrProperties.properties ?? (schemaOrProperties.items as JSONSchema),
	restRefScheme,
	description,
	),
	};
	}

I just would like to avoid adding complexity

This doesn't really feel complex IMO 😓 What do you find complex about it?

WDYT ? would you find it more readable or nicer ?

If my suggestion doesn't work, this is better than nothing

[myarmolinsky]

the new method?

[JSReds]

left over... there is no new method at the end, I've tried but it was becoming insanely unreadable 😢

[myarmolinsky]

This cast sounds very wrong, especially since string is not a return type of parseDescriptionProperty

Why do we do this?

[JSReds]

just very wrong as you said 😅

[myarmolinsky]

Would this work?

Suggested change

	return refScheme
	? (get(schema, `${refSchemePrefix}${refScheme}`) as JSONSchema) ?? schema
	: schema;
	return (get(schema, `${refSchemePrefix}${refScheme}`) as JSONSchema | null) ?? schema;

(added the | null to the cast too because it has to be able to be nullish for ?? to trigger)

[myarmolinsky]

Would this work?

Because if !refScheme then ${refSchemePrefix}${refScheme} = ${refSchemePrefix}undefined which shouldn't exist right?

[JSReds]

another very wrong left over, thanks for pointing it out

[myarmolinsky]

Would this work?

Suggested change

	return json && ('x-foreign-key-scheme' in json || 'x-ref-scheme' in json)
	? json['x-foreign-key-scheme'] ?? json['x-ref-scheme']
	: null;
	return json?.['x-foreign-key-scheme'] ?? json?.['x-ref-scheme'] ?? null;

[JSReds]

nice, yep now that I've added al checks and types it works 👍

[myarmolinsky]

Suggested change

	{filteredFittingSearchTerms?.map((entity) => (
	{filteredFittingSearchTerms.map((entity) => (

[myarmolinsky]

Unite into 1 find?

[myarmolinsky]

Also lodash has may be more appropriate here given how we're using it

[JSReds]

this cannot be unified in 1 check as this is finding a description in a nested object:

properties: {
						tag_key: {
							title: 'Tag name',
							description: 'key',
							type: 'string',
						},
						value: {
							title: 'Tag value',
							description: 'value',
							type: 'string',
						},
					},

Also note that _.has is not suitable for this purpose as it is intended to verify if a provided path exists within an object.

we need to use _.find to make sure we find the first description field and check the value in a nested case.

[myarmolinsky]

Suggested change

	schema.properties[firstKeyOfValue]
	) {
	const property = schema.properties[firstKeyOfValue];
	// Ensure the property is an object and has a title
	if (typeof property === 'object' && property.title) {
	return property.title;
	}
	}
	typeof schema.properties[firstKeyOfValue] === 'object' && !!schema.properties[firstKeyOfValue].title
	) {
	return schema.properties[firstKeyOfValue].title;
	}

[JSReds]

This is not valid, unfortunately.

in case of schema.properties[firstKeyOfValue], TypeScript evaluates it anew, so TypeScript narrows the type for each specific occurrence.

In the case where we assign it to a const, TypeScript infers the type of property at the time of assignment.

[myarmolinsky]

Did anything change here? Revert if not?

[JSReds]

LGTM