-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix demo when there's no recorder available #21519
Conversation
WalkthroughWalkthroughThis change enhances the database migration process in the Home Assistant application by adding a conditional check for the presence of the Changes
Sequence Diagram(s)sequenceDiagram
participant App as HomeAssistantAppEl
participant DB as Database
participant Config as Configuration
App->>Config: Check components
alt If `recorder` is present
App->>DB: Proceed with migration
else If `recorder` is absent
App-->>Config: Set _databaseMigration to false
Note over App: Early exit from migration
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
src/layouts/home-assistant.ts (1)
203-203
: Add a comment to explain the conditional check.Consider adding a comment to explain why the presence of the
recorder
component is being checked.+ // Check if the recorder component is present before proceeding with the database migration if (!this.hass?.config?.components.includes("recorder")) {
if (!this.hass?.config?.components.includes("recorder")) { | ||
this._databaseMigration = false; | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be the correct fix since the PR introducing it mentions:
We can't check for recorder in hass.config.components, it won't be added until after the migration has completed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code mentions that people would have been able to run Home Assistant without recorder in earlier versions. Since it checked upon recorder and putted database migration on false if it doesn't exist. That no longer happens, therefore doesn't load the frontend anymore. (But I'm not sure if people run HA without recorder...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From theory to practice: It definitely runs pretty decent if you have no recorder set up. Graphs and everything related is taken away. Broken on current dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 215 should have handled this, but for demo this is a bit different, so we can just skip all of this for __DEMO__
or mock the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've retested this, checked the code and it works with no recorder as well. All good.
The demo is further broken down the line with #18934, since it reloads every 5 seconds as there's no websocket available. |
For demo: we probably need to mock more. But for now, I'm going to do something else. :P |
Proposed change
The demo doesn't have a recorder available, and no longer loads. This is an attempt to fix.
https://demo.home-assistant.io/
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
recorder
component is active, enhancing application stability.