Skip to content
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

Logbook card loading fix #23853

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Conversation

Stormalong
Copy link
Contributor

Breaking change

Proposed change

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

This PR fixes an issue I've been having at least the last 6 months. On one of my dashboards the logbook card consistently fails to load the first time. It just spins forever. Changing to another page then coming back to the page loads the card correctly, which is why I've just been putting up with it until now.

This appears to be a race condition. If I put the logbook card by itself on a view then it loads correctly. I only experience this issue if the logbook card is in a view with many other cards.

I've attached a screen recording of the bug in action. When I click on the "Pool" view you will see the "Pool Activity" card just spinning. When I go back to the "Home" view then back to the "Pool" view you can see the "Pool Activity" loaded. I only stayed on the "Pool" view for a few seconds in this video but you can leave it forever and the card will never load.

Screen.Recording.2025-01-22.172322.mp4

The first commit removes a call to this._unsubscribe();. That function does nothing if this._subscribed is undefined, and the code right before this call returns if this._subscribed is undefined so the call to unsubscribe will never do anything.

The second commit adds an await to the call to subscribeLogbook, which is what actually fixes the bug. This changes the type of this._subscribed which is no longer a Promise.

It does seem that the original code should work fine and this change should result in identical behavior (we just moved the await from _unsubscribe to _subscribeLogbookPeriod) so i can't really explain why this fixes the problem but it does.

I was concerned that adding the await in _subscribeLogbookPeriod might end up blocking the page load or something, so I tested by adding an artificial 5 second delay before the call to subscribeLogbook. This causes the logbook card to spin for 5 seconds before loading (as expected) but the rest of the page loaded and functioned with no discernable issues.

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:

Copy link
Contributor

@MindFreeze MindFreeze left a comment

Choose a reason for hiding this comment

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

The whole promise logic for this was very weird. You have improved it a bit and I can see how it might fix the issue but it is still confusing. Do you mind cleaning it up a bit more? I added some comments.

src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved
src/panels/logbook/ha-logbook.ts Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft January 23, 2025 07:15
@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.

wrapped call to subscribeLogbook in try/catch
remove return values from _subscribeLogbookPeriod
@Stormalong Stormalong marked this pull request as ready for review January 23, 2025 19:30
@home-assistant home-assistant bot requested a review from MindFreeze January 23, 2025 19:30
@MindFreeze MindFreeze merged commit dc8d483 into home-assistant:dev Jan 24, 2025
15 checks passed
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.

2 participants