Fix lint failures on CI #21986
Fix lint failures on CI #21986
Conversation
WalkthroughWalkthroughThe changes involve refactoring two classes, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MatterAddDevice
participant DialogZWaveJSAddNode
User->>MatterAddDevice: Open Add Device Dialog
MatterAddDevice->>MatterAddDevice: connectedCallback()
MatterAddDevice->>User: Display Dialog
User->>DialogZWaveJSAddNode: Open Add Node Dialog
DialogZWaveJSAddNode->>DialogZWaveJSAddNode: connectedCallback()
DialogZWaveJSAddNode->>User: Display Dialog
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| import { customElement } from "lit/decorators"; | ||
| import { navigate } from "../../../../../common/navigate"; | ||
| import { HomeAssistant } from "../../../../../types"; | ||
| import { showMatterAddDeviceDialog } from "./show-dialog-add-matter-device"; | ||
|
|
||
| @customElement("matter-add-device") | ||
| export class MatterAddDevice extends HTMLElement { | ||
| export class MatterAddDevice extends LitElement { |
There was a problem hiding this comment.
Why should this extend LitElement?
There was a problem hiding this comment.
I'm not sure why it's HTMLElement, but the linting introduced in #21940 forces us to use LitElement. We should revert that one if this is inappropiate.
There was a problem hiding this comment.
It is because the customElement decorator is used, that was added in that release of es-lit, which I don't agree with that but ok...
This is HTMLElement because it is not needed to be a LitElement, because it doesnt use Lit (LitElement extends ReactiveElement) or properties it need to be updated from (ReactiveElement extends HTMLElement)
We can either remove the customElement decorator, and register the element manually:
customElements.define("matter-add-device", MatterAddDevice);Or ignore the linting issue.
| import { customElement } from "lit/decorators"; | ||
| import { navigate } from "../../../../../common/navigate"; | ||
| import { HomeAssistant } from "../../../../../types"; | ||
| import { showZWaveJSAddNodeDialog } from "./show-dialog-zwave_js-add-node"; | ||
|
|
||
| @customElement("zwave_js-add-node") | ||
| export class DialogZWaveJSAddNode extends HTMLElement { | ||
| export class DialogZWaveJSAddNode extends LitElement { |
There was a problem hiding this comment.
Why should this extend LitElement?
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/panels/config/integrations/integration-panels/matter/matter-add-device.ts (1)
Line range hint
1-24: Consider transitioning to LitElement as discussedBased on the past review comments and the PR objectives, there was a discussion about transitioning this component to extend
LitElementinstead ofHTMLElement. This change hasn't been implemented yet, and it might be the root cause of the lint failures mentioned in the PR summary.Consider refactoring the class to extend
LitElement:-import { customElement } from "lit/decorators"; +import { LitElement, html } from "lit"; +import { customElement } from "lit/decorators"; import { navigate } from "../../../../../common/navigate"; import { HomeAssistant } from "../../../../../types"; import { showMatterAddDeviceDialog } from "./show-dialog-add-matter-device"; @customElement("matter-add-device") -export class MatterAddDevice extends HTMLElement { +export class MatterAddDevice extends LitElement { public hass!: HomeAssistant; connectedCallback() { super.connectedCallback(); showMatterAddDeviceDialog(this); navigate(`/config/devices`, { replace: true, }); } + + render() { + return html``; + } } declare global { interface HTMLElementTagNameMap { "matter-add-device": MatterAddDevice; } }This refactoring addresses the discussion in the past comments and might resolve the lint failures mentioned in the PR objectives.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/panels/config/integrations/integration-panels/matter/matter-add-device.ts (1 hunks)
- src/panels/config/integrations/integration-panels/zwave_js/zwave_js-add-node.ts (1 hunks)
Additional comments not posted (1)
src/panels/config/integrations/integration-panels/matter/matter-add-device.ts (1)
11-11: Verify the necessity ofsuper.connectedCallback()The addition of
super.connectedCallback()is correct forHTMLElement. However, given the past comments and the PR objectives, we need to verify if this change addresses the lint failures mentioned in the PR summary.Let's check if there are any lint rules related to
super.connectedCallback():
| @@ -10,6 +10,7 @@ export class DialogZWaveJSAddNode extends HTMLElement { | |||
| public configEntryId!: string; | |||
|
|
|||
| connectedCallback() { | |||
| super.connectedCallback(); | |||
There was a problem hiding this comment.
Inconsistent use of super.connectedCallback()
The addition of super.connectedCallback() is inconsistent with the class extending HTMLElement. HTMLElement doesn't have a connectedCallback method to call.
This change suggests that the class should be extending LitElement instead of HTMLElement, which aligns with the past review comment asking why this should extend LitElement.
Consider updating the class declaration to extend LitElement:
- export class DialogZWaveJSAddNode extends HTMLElement {
+ export class DialogZWaveJSAddNode extends LitElement {If you make this change, ensure you also:
- Import
LitElementfrom the appropriate Lit package. - Update any necessary TypeScript declarations or imports.
- Refactor the class to properly use Lit's reactive properties and rendering if needed.
Committable suggestion was skipped due to low confidence.
4608e59 to
b7cf3e2
Compare
b7cf3e2 to
63ff35d
Compare
Proposed change
Lint failures are appearing on some pull requests since #21940. This only appears when cache is being refreshed for now.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes