Skip to content

Device page: sort related automations, scenes, scripts #24742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

ildar170975
Copy link
Contributor

@ildar170975 ildar170975 commented Mar 23, 2025

Breaking change

Proposed change

Before / after:

изображение

Notes:

  1. Now the ha-config-device-page class has same _toEntities() (entity_ids -> objects) method as ha-related-items. Probably it should be made as a common function then.
  2. I am not getting a need for ha-tooltip for related automations. Currently it is not present for scripts but is for automations & scenes.
  3. There is also a difference in defining href for related entities between "automation & scenes" AND scripts. Looks like these blocks were written differently.
  4. This old line causes a eslint error: "This comparison appears to be unintentional because the types 'string' and 'HassEntity' have no overlap":
(e) => e.entity_id === script

Trying to fix it.... Changed the comparison to e.entity_id === script as any.

Suggest to some Dev person (or to myself) to sort out these mentioned differences between blocks for related entities.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@ildar170975 ildar170975 marked this pull request as ready for review March 23, 2025 23:52
@silamon
Copy link
Contributor

silamon commented Mar 24, 2025

#23740 is related.

const entry = this._entityReg.find(
(e) => e.entity_id === script
(e) => e.entity_id === (script as any)
Copy link
Member

Choose a reason for hiding this comment

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

script is an stateObj right? So this should be?

Suggested change
(e) => e.entity_id === (script as any)
(e) => e.entity_id === script.entity_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! Fixed.

entityState.attributes.id
? `/config/automation/edit/${encodeURIComponent(entityState.attributes.id)}`
: undefined
${this._toEntities(this._related.automation).map(
Copy link
Member

Choose a reason for hiding this comment

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

We should wrap this in memoized, so we don't do the sorting and filtering on every render.

Copy link
Member

Choose a reason for hiding this comment

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

I would make this 1 memoized function, that returns all the sorted and filtered entities:

private _getRelated_ = memoizeOne((this._related) => {entity: [], automation: [], ... });

Copy link
Contributor Author

@ildar170975 ildar170975 Apr 13, 2025

Choose a reason for hiding this comment

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

Refactored to memoize. Hope I did it right..
But the entity part is not included in the same memoize, it is in a different one (which was already present).

@ildar170975 ildar170975 marked this pull request as draft April 12, 2025 06:38
@ildar170975 ildar170975 marked this pull request as ready for review April 13, 2025 02:18
entities
.map((entity) => ({
...entity,
stateName: this._computeEntityName(entity),
Copy link
Member

@bramkragten bramkragten Apr 24, 2025

Choose a reason for hiding this comment

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

_computeEntityName takes a EntityRegistryEntry, you are passing a state object to it. Please add proper typing so these things pop up automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code, please review.

@home-assistant home-assistant bot marked this pull request as draft April 24, 2025 14:32
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@ildar170975 ildar170975 marked this pull request as ready for review May 29, 2025 19:55
@home-assistant home-assistant bot requested a review from bramkragten May 29, 2025 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device info page - automations, scripts, scenes aren't sorted alphabetically
3 participants