Add MVP voice assist flow #22061
Add MVP voice assist flow #22061
Conversation
WalkthroughWalkthroughThe changes introduce a comprehensive voice assistant setup process within a Home Assistant environment. This includes the addition of interfaces for managing wake words, several functions for interacting with the assistive satellite system, and enhancements to the device configuration page that facilitate direct initiation of the voice assistant setup for devices associated with the "assist_satellite" domain. A new component for managing the update process of the voice assistant is also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DevicePage
participant VoiceAssistantSetupDialog
participant UpdateComponent
User->>DevicePage: Initiate voice assistant setup
DevicePage->>VoiceAssistantSetupDialog: Show setup dialog
VoiceAssistantSetupDialog->>User: Display setup steps
User->>VoiceAssistantSetupDialog: Select wake word
VoiceAssistantSetupDialog->>VoiceAssistantSetupDialog: Update configuration
VoiceAssistantSetupDialog->>User: Confirm setup success
User->>UpdateComponent: Trigger update process
UpdateComponent->>UpdateComponent: Check entity state
UpdateComponent->>User: Display update progress
UpdateComponent->>User: Confirm update success
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
Outside diff range and nitpick comments (22)
src/dialogs/voice-assistant-setup/show-voice-assistant-setup-dialog.ts (3)
3-4: LGTM: Good use of dynamic import.The
loadVoiceAssistantSetupDialogfunction uses dynamic import, which is excellent for code splitting and lazy loading. This approach can improve initial load times by deferring the loading of the dialog component until it's needed.Consider adding a type annotation to the function for improved clarity:
const loadVoiceAssistantSetupDialog = (): Promise<typeof import("./voice-assistant-setup-dialog")> => import("./voice-assistant-setup-dialog");
6-8: LGTM: Clear and concise interface definition.The
VoiceAssistantSetupDialogParamsinterface is well-defined and exported, which promotes type safety when used across the application.Consider adding a JSDoc comment to provide more context about the interface and its usage:
/** * Parameters for initializing the voice assistant setup dialog. */ export interface VoiceAssistantSetupDialogParams { /** The ID of the device for which the voice assistant is being set up. */ deviceId: string; }
10-19: LGTM: Well-structured dialog show function.The
showVoiceAssistantSetupDialogfunction is well-implemented, using a custom event to trigger the dialog display. This approach allows for loose coupling between components and supports lazy loading of the dialog component.Consider adding a JSDoc comment to provide more context about the function and its parameters:
/** * Shows the voice assistant setup dialog. * @param element - The HTML element on which to dispatch the event. * @param dialogParams - The parameters for the dialog. */ export const showVoiceAssistantSetupDialog = ( element: HTMLElement, dialogParams: VoiceAssistantSetupDialogParams ): void => { // ... (existing implementation) };src/dialogs/voice-assistant-setup/styles.ts (1)
6-35: LGTM: Well-structured and responsive CSS styles.The CSS styles are well-organized, use flexbox appropriately for layout, and provide good responsiveness. The component-specific styling reduces the risk of unintended side effects.
Consider adding a media query for larger screens to adjust the
max-widthof the host element. This could improve readability on desktop devices. For example:@media (min-width: 600px) { :host { max-width: 600px; } }src/panels/config/voice-assistants/show-dialog-voice-assistant-pipeline-detail.ts (1)
11-11: LGTM! Consider adding documentation for the new property.The addition of the optional
hideWakeWordproperty is a good approach to control the visibility of the wake word feature in the UI. It aligns well with the MVP voice assist flow objectives.Consider adding a brief comment explaining the purpose and usage of this property to improve code maintainability.
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-cloud.ts (1)
8-9: Consider adding a description for the componentThe class declaration and property look correct. However, it would be beneficial to add a JSDoc comment describing the purpose and usage of this component.
Consider adding a description like this:
/** * A component for setting up Home Assistant Cloud for voice assistants. * This step promotes the benefits of using Home Assistant Cloud and provides a link to start a free trial. */ @customElement("ha-voice-assistant-setup-step-cloud") export class HaVoiceAssistantSetupStepCloud extends LitElement { // ... rest of the code }src/data/assist_satellite.ts (5)
7-11: LGTM: Well-structured interface for wake word options.The
WakeWordOptioninterface clearly represents a wake word option with its identifier, the wake word itself, and the languages it's trained in.Consider using a more specific type for
trained_languagesto enhance type safety:trained_languages: readonly string[];This change would prevent accidental modifications to the array and better represent its intended use as a read-only list of languages.
13-19: LGTM: Comprehensive interface for assist satellite configuration.The
AssistSatelliteConfigurationinterface provides a clear and complete representation of the assist satellite configuration.Consider using readonly properties to enhance type safety and prevent accidental modifications:
export interface AssistSatelliteConfiguration { readonly active_wake_words: readonly string[]; readonly available_wake_words: readonly WakeWordOption[]; readonly max_active_wake_words: number; readonly pipeline_entity_id: string; readonly vad_entity_id: string; }This change would better represent the configuration as an immutable object.
21-29: LGTM: Well-implemented function for intercepting wake words.The
interceptWakeWordfunction correctly sets up a subscription for wake word interception messages.Consider adding a return type to the function for improved type safety:
export const interceptWakeWord = ( hass: HomeAssistant, entity_id: string, callback: (result: WakeWordInterceptMessage) => void ): Promise<() => Promise<void>> => // Add this return type hass.connection.subscribeMessage(callback, { type: "assist_satellite/intercept_wake_word", entity_id, });This change would make it clear that the function returns a Promise that resolves to an unsubscribe function.
42-47: LGTM: Well-implemented function for sending announcements to the assist satellite.The
assistSatelliteAnnouncefunction correctly uses thecallServicemethod to send an announcement to the assist satellite.For consistency with other functions in this file, consider adding a return type:
export const assistSatelliteAnnounce = ( hass: HomeAssistant, entity_id: string, message: string ): Promise<void> => // Add this return type hass.callService("assist_satellite", "announce", { message }, { entity_id });This change would make it clear that the function returns a Promise that resolves when the service call is complete.
58-67: LGTM: Well-implemented function for setting wake words.The
setWakeWordsfunction correctly uses the WebSocket API to update the active wake words for a specified entity.Consider adding a return type and using a readonly array for
wake_word_idsto enhance type safety:export const setWakeWords = ( hass: HomeAssistant, entity_id: string, wake_word_ids: readonly string[] ): Promise<void> => // Add this return type hass.callWS({ type: "assist_satellite/set_wake_words", entity_id, wake_word_ids, });These changes would prevent accidental modifications to the
wake_word_idsarray and make it clear that the function returns a Promise that resolves when the operation is complete.src/dialogs/voice-assistant-setup/voice-assistant-setup-step-change-wake-word.ts (1)
23-46: Render method looks good, but there's a typo in the description.The render method is well-structured, combining static content with dynamic rendering of wake words. However, there's a typo in the description:
- When you voice assistant knows where it is, it can better control the + When your voice assistant knows where it is, it can better control thePlease correct this typo to improve the user experience.
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-check.ts (1)
10-14: LGTM: Properties and state are well-defined. Consider adding JSDoc comments.The properties and state are correctly defined and typed. However, adding JSDoc comments for each property and state would improve code readability and maintainability.
Consider adding JSDoc comments for each property and state. For example:
/** * The Home Assistant instance */ @property({ attribute: false }) public hass!: HomeAssistant; /** * The entity ID of the assist satellite */ @property() public assistEntityId?: string; /** * The current status of the connection test */ @state() private _status?: "success" | "timeout";src/dialogs/voice-assistant-setup/voice-assistant-setup-step-addons.ts (1)
13-35: Consider using constants for timeout values.The use of state properties and the staggered reveal effect in
firstUpdatedis good for user experience. However, the hard-coded timeout values might make it difficult to adjust timing in the future.Consider defining constants for these timeout values at the top of the file:
const FIRST_MESSAGE_DELAY = 200; const SECOND_MESSAGE_DELAY = 600; const THIRD_MESSAGE_DELAY = 3000; const FOURTH_MESSAGE_DELAY = 8000;Then use these constants in the
firstUpdatedmethod:setTimeout(() => { this._showFirst = true; }, FIRST_MESSAGE_DELAY); // ... and so on for the other timeoutsThis will make it easier to adjust timing in the future if needed.
src/panels/config/voice-assistants/dialog-voice-assistant-pipeline-detail.ts (2)
218-225: LGTM! Consider extracting the condition for improved readability.The conditional rendering of the
assist-pipeline-detail-wakewordcomponent is well-implemented. It provides flexibility in the UI by allowing the wake word configuration to be optionally hidden.For improved readability, consider extracting the condition into a separate constant:
const showWakeWord = !this._params.hideWakeWord; ${showWakeWord ? html`<assist-pipeline-detail-wakeword .hass=${this.hass} .data=${this._data} keys="wake_word_entity,wake_word_id" @value-changed=${this._valueChanged} ></assist-pipeline-detail-wakeword>` : nothing}This change would make the template easier to read and the condition's purpose more explicit.
Line range hint
1-369: Consider implementing a consistent error handling strategy.The recent changes have improved error handling in several places, which is great. To further enhance the robustness and user experience of the component, consider implementing a consistent error handling strategy throughout the class.
Here are some suggestions:
- Create a dedicated method for setting errors:
private _setError(error: Error | string) { this._error = typeof error === "string" ? error : error.message || "Unknown error"; this._submitting = false; }
- Use this method consistently in all catch blocks:
} catch (err: any) { this._setError(err); }
Ensure all user actions that can fail (update, create, delete, set preferred) use try-catch blocks with this error handling.
Consider adding a method to clear errors when appropriate, such as when the dialog is opened or closed.
These changes would provide a more consistent and maintainable approach to error handling throughout the component.
src/panels/config/devices/ha-config-device-page.ts (2)
1067-1080: New voice assistant setup action addedA new condition has been added to check if any of the device's entities belong to the "assist_satellite" domain. If true, a new action for setting up the voice assistant is added to the
deviceActionsarray.This implementation aligns with the PR objective and provides a way for users to initiate the voice assistant setup directly from the device page.
However, consider adding a comment explaining the purpose of this check and action for better code readability.
Consider adding a comment like:
// Add voice assistant setup action for devices with assist_satellite entities
1415-1419: New _voiceAssistantSetup methodA new private method
_voiceAssistantSetuphas been added to handle the voice assistant setup action. It calls theshowVoiceAssistantSetupDialogfunction with the current device ID.This implementation is correct and aligns with the PR objective.
Consider adding a JSDoc comment to describe the purpose of this method.
Add a JSDoc comment like:
/** * Initiates the voice assistant setup dialog for the current device. */src/dialogs/voice-assistant-setup/voice-assistant-setup-step-update.ts (2)
58-60: Safeguard division operation to prevent NaN resultsWhen calculating the progress value, dividing by 100 without ensuring
stateObj.attributes.in_progressis a valid number can result inNaNif the value isundefined. It's prudent to provide a default value to avoid this issue.Apply this diff to provide a default value:
.value=${progressIsNumeric - ? stateObj.attributes.in_progress / 100 + ? (stateObj.attributes.in_progress || 0) / 100 : undefined}
77-81: Simplify state check by usingupdateEntity.statedirectlyIn the
_tryUpdatemethod, you can simplify the state check by accessingupdateEntity.statedirectly, sinceupdateEntityalready contains the entity's state. This improves code readability and efficiency.Apply this diff to simplify the code:
if ( updateEntity && - this.hass.states[updateEntity.entity_id].state === "on" + updateEntity.state === "on" ) {src/dialogs/voice-assistant-setup/voice-assistant-setup-step-wake-word.ts (1)
68-68: Localize user-facing strings usingthis.hass.localizeThe strings displayed to the user should be localized to support multiple languages.
Apply this diff to localize the strings:
<!-- In line 68 --> -<img src="/static/icons/casita/sleeping.png" /> +<img + src="/static/icons/casita/sleeping.png" + alt="${this.hass.localize('ui.dialogs.voice_assistant_setup.icon_sleeping_alt')}" +/> <!-- In lines 70-73 --> -<h1> - Say “${this._activeWakeWord(this.assistConfiguration)}” to wake the - device up -</h1> +<h1> + ${this.hass.localize( + "ui.dialogs.voice_assistant_setup.say_to_wake_device", + "wake_word", + this._activeWakeWord(this.assistConfiguration) + )} +</h1> -<p class="secondary">Setup will continue once the device is awake.</p> +<p class="secondary"> + ${this.hass.localize("ui.dialogs.voice_assistant_setup.setup_will_continue")} +</p> <!-- In line 75 --> -<img src="/static/icons/casita/normal.png" /> +<img + src="/static/icons/casita/normal.png" + alt="${this.hass.localize('ui.dialogs.voice_assistant_setup.icon_normal_alt')}" +/> <!-- In lines 77-81 --> -<h1> - Say “${this._activeWakeWord(this.assistConfiguration)}” again -</h1> -<p class="secondary"> - To make sure the wake word works for you. -</p> +<h1> + ${this.hass.localize( + "ui.dialogs.voice_assistant_setup.say_again", + "wake_word", + this._activeWakeWord(this.assistConfiguration) + )} +</h1> +<p class="secondary"> + ${this.hass.localize("ui.dialogs.voice_assistant_setup.wake_word_works_for_you")} +</p> <!-- In line 84 --> -<ha-button @click=${this._changeWakeWord}>Change wake word</ha-button> +<ha-button @click=${this._changeWakeWord}> + ${this.hass.localize("ui.dialogs.voice_assistant_setup.change_wake_word")} +</ha-button>Ensure that the localization keys are added to the translation files.
Also applies to: 70-73, 75-75, 77-81, 84-84
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-success.ts (1)
15-17: Remove unused import 'assistSatelliteAnnounce'The function
assistSatelliteAnnounceis imported but not used in this file. Removing unused imports helps keep the codebase clean and can improve build times.Apply this diff to remove the unused import:
- import { - assistSatelliteAnnounce, - AssistSatelliteConfiguration, - } from "../../data/assist_satellite"; + import { + AssistSatelliteConfiguration, + } from "../../data/assist_satellite";
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (6)
public/static/icons/casita/loading.pngis excluded by!**/*.pngpublic/static/icons/casita/loving.pngis excluded by!**/*.pngpublic/static/icons/casita/normal.pngis excluded by!**/*.pngpublic/static/icons/casita/sad.pngis excluded by!**/*.pngpublic/static/icons/casita/sleeping.pngis excluded by!**/*.pngpublic/static/icons/casita/smiling.pngis excluded by!**/*.png
Files selected for processing (17)
- src/data/assist_satellite.ts (1 hunks)
- src/dialogs/config-flow/step-flow-create-entry.ts (3 hunks)
- src/dialogs/voice-assistant-setup/show-voice-assistant-setup-dialog.ts (1 hunks)
- src/dialogs/voice-assistant-setup/styles.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-dialog.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-addons.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-area.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-change-wake-word.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-check.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-cloud.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-pipeline.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-success.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-update.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-wake-word.ts (1 hunks)
- src/panels/config/devices/ha-config-device-page.ts (4 hunks)
- src/panels/config/voice-assistants/dialog-voice-assistant-pipeline-detail.ts (3 hunks)
- src/panels/config/voice-assistants/show-dialog-voice-assistant-pipeline-detail.ts (1 hunks)
Additional context used
Biome
src/dialogs/voice-assistant-setup/voice-assistant-setup-dialog.ts
[error] 28-39: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
Additional comments not posted (34)
src/dialogs/voice-assistant-setup/show-voice-assistant-setup-dialog.ts (2)
1-1: LGTM: Import statement is appropriate.The import of
fireEventfrom the common module is suitable for the file's purpose of triggering custom events.
1-19: Overall: Excellent implementation of the voice assistant setup dialog launcher.This file provides a clean and efficient implementation for launching the voice assistant setup dialog. It aligns well with the PR objectives of adding an MVP voice assist flow to the Home Assistant frontend. The use of TypeScript, dynamic imports, and custom events demonstrates good modern web development practices.
Key strengths:
- Clear separation of concerns
- Type safety with exported interface
- Lazy loading of dialog component
- Loose coupling through custom events
The suggested minor improvements (adding JSDoc comments and type annotations) would further enhance the code's readability and maintainability, but the current implementation is already of high quality.
src/dialogs/voice-assistant-setup/styles.ts (3)
1-2: LGTM: Imports are correct and appropriate.The imports for
cssfrom "lit" andhaStylefrom the relative path are correctly implemented and necessary for the file's functionality.
4-5: LGTM: Export declaration is well-structured.The export of
AssistantSetupStylesas an array containinghaStyleand custom CSS is a good approach. It allows for easy composition of styles and ensures consistency with the overall Home Assistant styling.
1-36: Overall: Excellent implementation of styles for the voice assistant setup.This new file provides a solid foundation for styling the voice assistant setup component. The code is well-structured, follows best practices, and aligns well with the PR objectives. The use of flexbox for layout ensures good responsiveness, and the styles are appropriately scoped to the component.
The implementation demonstrates attention to detail in creating a user-friendly interface for the MVP voice assist flow. Great job!
src/panels/config/voice-assistants/show-dialog-voice-assistant-pipeline-detail.ts (2)
11-15: Overall, the changes look good and align with the PR objectives.The modifications to the
VoiceAssistantPipelineDetailsDialogParamsinterface enhance its flexibility, which is beneficial for the MVP voice assist flow. The newhideWakeWordproperty and the optionalcreatePipelineanddeletePipelineproperties provide more control over the UI and pipeline management.To ensure a smooth integration:
- Consider adding brief documentation for the
hideWakeWordproperty.- Verify that existing code handles the now-optional
createPipelineanddeletePipelineproperties correctly.These changes lay a good foundation for the voice assistant setup process in the Home Assistant frontend.
15-15: LGTM! Verify existing usage ofdeletePipeline.Making
deletePipelineoptional provides more flexibility in the interface usage, which aligns well with the MVP voice assist flow objectives.Please ensure that existing code handles the case where
deletePipelinemight be undefined. Run the following script to check its usage:#!/bin/bash # Description: Check usage of deletePipeline in the codebase # Search for deletePipeline usage rg --type typescript -A 5 'deletePipeline'src/dialogs/voice-assistant-setup/voice-assistant-setup-step-cloud.ts (4)
1-7: LGTM: Imports and decorators look goodThe imports and decorators are correctly implemented. The use of
customElementdecorator for defining the custom element is a good practice.
27-29: LGTM: Close method is correctly implementedThe
_closemethod is simple and correctly implemented. It fires a "closed" event, which is likely used by the parent component to handle the closing of this setup step.
34-38: LGTM: Global declaration is correctly implementedThe global declaration for the custom element is correctly implemented. This ensures proper TypeScript support when using this custom element in HTML.
1-38: Overall assessment: Good implementation with room for improvementThe
HaVoiceAssistantSetupStepCloudcomponent is well-structured and follows LitElement best practices. It successfully introduces a new step in the voice assistant setup process, promoting Home Assistant Cloud.Key strengths:
- Proper use of LitElement and its decorators.
- Clear separation of concerns between rendering and event handling.
Areas for improvement:
- Enhance accessibility by adding alt text to images and avoiding nested interactive elements.
- Implement internationalization for better multi-language support.
- Consider adding JSDoc comments for better documentation.
- Refine the styling approach for better encapsulation.
These improvements will make the component more robust, accessible, and easier to maintain in the long run.
src/data/assist_satellite.ts (4)
3-5: LGTM: Well-defined interface for wake word interception.The
WakeWordInterceptMessageinterface is concise and clearly represents the structure of a wake word intercept message.
31-40: LGTM: Well-implemented function for testing assist satellite connection.The
testAssistSatelliteConnectionfunction correctly uses the WebSocket API to test the connection status of the assist satellite. The return type is properly specified, ensuring type safety for the response.
49-56: LGTM: Well-implemented function for fetching assist satellite configuration.The
fetchAssistSatelliteConfigurationfunction correctly uses the WebSocket API to retrieve the current configuration of the assist satellite. The return type is properly specified using theAssistSatelliteConfigurationinterface, ensuring type safety for the response.
1-67: Overall: Excellent implementation of assist satellite data management.This file provides a comprehensive set of interfaces and functions for managing wake words and configurations in an assist satellite system. The code is well-structured, follows TypeScript conventions, and demonstrates good use of type safety. The minor suggestions provided in the review comments will further enhance the robustness of the code.
Great job on implementing this new feature!
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-area.ts (5)
1-9: LGTM: Imports and decorators are appropriate.The imports and decorators are well-structured and follow Home Assistant conventions for LitElement components.
10-13: LGTM: Class definition and properties are well-defined.The class extends LitElement correctly, and the properties are appropriately decorated and typed. The
hassproperty is correctly marked as non-attribute.
35-49: LGTM: Utility methods are well-implemented.The
_setAreaand_nextStepmethods handle their responsibilities correctly. Error handling is present, and the use ofupdateDeviceRegistryEntryandfireEventis appropriate.
51-67: LGTM: Styles and global declaration are correct.The styles are appropriately defined, combining imported styles with additional CSS. The global declaration for HTMLElementTagNameMap is correctly implemented, ensuring TypeScript recognition of the custom element.
15-33: LGTM: Render method structure is appropriate.The render method creates a logical UI structure for a setup step, including an image, header, description, area picker, and a button.
Please verify the existence of the static asset:
Verification successful
Static asset
loving.pngexists as expected.The asset
loving.pngwas found atpublic/static/icons/casita/loving.png, confirming that the path/static/icons/casita/loving.pngin the code is valid.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of the loving.png asset fd -t f "loving.png" static/icons/casitaLength of output: 147
Script:
#!/bin/bash # Check if the static/icons directory exists if [ -d "static/icons" ]; then echo "static/icons directory exists." else echo "static/icons directory does not exist." exit 1 fi # Search for loving.png within the static/icons directory fd -t f "loving.png" static/iconsLength of output: 121
Script:
#!/bin/bash # Search for loving.png throughout the entire codebase fd -t f "loving.png"Length of output: 58
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-change-wake-word.ts (3)
1-12: Imports look good and are well-organized.The necessary modules and types are imported correctly, and there are no unused imports. The import order is logical, starting with external libraries followed by internal modules.
14-21: Class definition and properties are well-structured.The class is correctly defined as a custom element extending LitElement. The properties are appropriately decorated and typed, with
hassbeing required and the others optional. This structure provides good flexibility for the component's usage.
63-84: Styles and global declaration are appropriate.The use of
AssistantSetupStylespromotes consistency, and the additional custom CSS for padding and list styling is well-defined. The global declaration correctly maps the custom element name to its class, which is necessary for TypeScript type checking.src/dialogs/voice-assistant-setup/voice-assistant-setup-step-check.ts (2)
1-9: LGTM: Imports and class declaration are well-structured.The imports are appropriate for a LitElement component, and the custom element declaration follows the Home Assistant naming convention.
80-87: LGTM: Styles and global declaration are correctly implemented.The use of shared
AssistantSetupStylespromotes consistency, and the global declaration follows TypeScript best practices for custom elements.src/dialogs/voice-assistant-setup/voice-assistant-setup-step-addons.ts (1)
1-11: LGTM: Imports and class declaration are well-structured.The imports are appropriate for a LitElement component, and the class declaration follows best practices with proper decorators and typing.
src/panels/config/devices/ha-config-device-page.ts (2)
8-8: New import for voice assistant setup dialogA new import statement has been added for the
showVoiceAssistantSetupDialogfunction. This is consistent with the PR objective of adding an MVP voice assist flow.
Line range hint
8-1419: Overall assessment of voice assistant setup changesThe changes introduced for the voice assistant setup functionality are well-implemented and align with the PR objective. The new import, condition check in
_getDeviceActions, and the_voiceAssistantSetupmethod work together to provide a seamless way for users to initiate the voice assistant setup for compatible devices.A few minor suggestions have been made to improve code documentation, but these are not critical issues. The implementation is approved and ready for integration.
src/dialogs/voice-assistant-setup/voice-assistant-setup-step-update.ts (2)
95-98: EnsureupdateConfigis correctly set when no update occursIn the
_nextStepmethod, theupdateConfigproperty is set based on_updated. If_tryUpdatedoesn't result in an update,_updatedremainsfalse. Confirm that this behavior aligns with the desired logic for progressing to the next step.
1-116: Overall implementation aligns with best practicesThe component is well-structured, following LitElement conventions. Properties and state management are handled appropriately, and the user interface provides clear feedback during the update process.
src/dialogs/config-flow/step-flow-create-entry.ts (2)
52-56: Ensure proper sequencing after_flowDone()After calling
this._flowDone(), make sure that the component's state updates as expected before showing the voice assistant setup dialog. If_flowDone()leads to unmounting or state changes, the subsequent call toshowVoiceAssistantSetupDialogmay not behave as intended.
48-50: Confirm the correctness of the domain comparisonWhen checking if an entity belongs to the
assist_satellitedomain usingcomputeDomain(entity.entity_id) === "assist_satellite", verify thatassist_satelliteis the correct domain name and thatcomputeDomainwill return it as expected. Domains are typically things likesensor,light, etc.You can run the following script to list all entities with the
assist_satellitedomain:src/dialogs/voice-assistant-setup/voice-assistant-setup-step-success.ts (2)
178-182: Validate 'preferred_pipeline' before comparisonIn the
_openPipelinemethod,preferred_pipelinemight be undefined, which can cause issues when comparing it topipeline.id.Modify the comparison to handle undefined
preferred_pipeline:preferred: preferred_pipeline ? pipeline.id === preferred_pipeline : false,Likely invalid or redundant comment.
201-203: Confirm event name in '_close' methodEnsure that the event
'closed'fired in the_closemethod is the correct event name expected by the parent component.Run the following script to verify event listeners for
'closed':
src/panels/config/voice-assistants/show-dialog-voice-assistant-pipeline-detail.ts
Show resolved
Hide resolved
| protected override render() { | ||
| return html`<div class="content"> | ||
| <img src="/static/icons/casita/loving.png" /> | ||
| <h1>Home Assistant Cloud</h1> | ||
| <p class="secondary"> | ||
| With Home Assistant Cloud, you get the best results for your voice | ||
| assistant, sign up for a free trial now. | ||
| </p> | ||
| </div> | ||
| <div class="footer"> | ||
| <a href="/config/cloud/register" @click=${this._close} | ||
| ><ha-button>Start your free trial</ha-button></a | ||
| > | ||
| </div>`; | ||
| } |
There was a problem hiding this comment.
Render method: Consider accessibility and internationalization
The render method looks functional, but there are a few areas for improvement:
- The image should have an
altattribute for accessibility. - The text content should be internationalized for multi-language support.
- The
<a>tag wrapping the button might lead to nested interactive elements, which is not ideal for accessibility.
Consider applying these changes:
- Add an
altattribute to the image:
- <img src="/static/icons/casita/loving.png" />
+ <img src="/static/icons/casita/loving.png" alt="Home Assistant Cloud" />- Use localization for text content:
- <h1>Home Assistant Cloud</h1>
+ <h1>${this.hass.localize("ui.panel.config.cloud.register.title")}</h1>
- <p class="secondary">
- With Home Assistant Cloud, you get the best results for your voice
- assistant, sign up for a free trial now.
- </p>
+ <p class="secondary">
+ ${this.hass.localize("ui.dialogs.voice_assistant.cloud_promo")}
+ </p>
- <ha-button>Start your free trial</ha-button>
+ <ha-button>${this.hass.localize("ui.dialogs.voice_assistant.start_free_trial")}</ha-button>- Replace the
<a>tag with an event handler on the button:
- <a href="/config/cloud/register" @click=${this._close}>
- <ha-button>Start your free trial</ha-button>
- </a>
+ <ha-button @click=${this._startFreeTrial}>
+ ${this.hass.localize("ui.dialogs.voice_assistant.start_free_trial")}
+ </ha-button>Then add a new method to handle the button click:
private _startFreeTrial() {
this._close();
this.hass.navigate("/config/cloud/register");
}This approach separates the navigation logic from the UI event, which is a better practice.
| fireEvent(this, "closed"); | ||
| } | ||
|
|
||
| static styles = AssistantSetupStyles; |
There was a problem hiding this comment.
Consider defining styles within the class
While using a shared style is not incorrect, it might be beneficial to define specific styles within the class for better encapsulation and easier maintenance.
Consider defining the styles within the class like this:
static get styles() {
return [
AssistantSetupStyles,
css`
// Add any component-specific styles here
`
];
}This approach allows you to extend the shared styles with component-specific styles if needed in the future.
| When you voice assistant knows where it is, it can better control the | ||
| devices around it. |
There was a problem hiding this comment.
Fix typo in the description text.
There's a small typo in the description text.
Apply this diff to fix the typo:
- When you voice assistant knows where it is, it can better control the
+ When your voice assistant knows where it is, it can better control theCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| When you voice assistant knows where it is, it can better control the | |
| devices around it. | |
| When your voice assistant knows where it is, it can better control the | |
| devices around it. |
| private async _wakeWordPicked(ev) { | ||
| if (!this.assistEntityId) { | ||
| return; | ||
| } | ||
|
|
||
| const wakeWordId = ev.currentTarget.value; | ||
|
|
||
| await setWakeWords(this.hass, this.assistEntityId, [wakeWordId]); | ||
| this._nextStep(); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling in _wakeWordPicked method.
The method correctly checks for the existence of assistEntityId before proceeding. However, it might be beneficial to add error handling for the setWakeWords call:
private async _wakeWordPicked(ev) {
if (!this.assistEntityId) {
return;
}
const wakeWordId = ev.currentTarget.value;
try {
await setWakeWords(this.hass, this.assistEntityId, [wakeWordId]);
this._nextStep();
} catch (error) {
// Handle or log the error
console.error("Failed to set wake word:", error);
// Optionally, notify the user
}
}This change would make the component more robust by handling potential errors when setting the wake word.
| this.hass, | ||
| "/voice_control/voice_remote_local_assistant/" | ||
| )} | ||
| rel="noreferrer noopenner" |
There was a problem hiding this comment.
Correct the typo in the rel attribute value.
At line 93, the rel attribute has a misspelling. The correct value should be noopener instead of noopenner.
Apply this diff to fix the typo:
-rel="noreferrer noopenner"
+rel="noreferrer noopener"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rel="noreferrer noopenner" | |
| rel="noreferrer noopener" |
| for (const entity of Object.values(this.hass.entities)) { | ||
| if (entity.platform === "cloud") { | ||
| const domain = computeDomain(entity.entity_id); | ||
| if (domain === "tts") { | ||
| cloudTtsEntityId = entity.entity_id; | ||
| } else if (domain === "stt") { | ||
| cloudSttEntityId = entity.entity_id; | ||
| } else { | ||
| continue; | ||
| } | ||
| if (cloudTtsEntityId && cloudSttEntityId) { | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix incorrect access to entity properties in _checkCloud() method.
In the _checkCloud() method, the code iterates over this.hass.entities and accesses entity.platform. However:
this.hassdoes not have anentitiesproperty; it should bethis.hass.states.- The
entityobject retrieved fromthis.hass.statesdoes not have aplatformproperty.
To obtain the platform information, you may need to access entity.attributes.platform or adjust your approach based on how the platform data is stored.
Consider modifying the code as follows:
-for (const entity of Object.values(this.hass.entities)) {
+for (const entity of Object.values(this.hass.states)) {And adjust the platform check:
-if (entity.platform === "cloud") {
+if (entity.attributes.platform === "cloud") {Ensure that platform exists within entity.attributes. If not, you may need to revise your method for identifying cloud entities.
Committable suggestion was skipped due to low confidence.
| private _nextStep(ev) { | ||
| if (ev.detail?.updateConfig) { | ||
| this._fetchAssistConfiguration(); | ||
| } | ||
| if (!ev.detail?.noPrevious) { | ||
| this._previousSteps.push(this._step); | ||
| } | ||
| if (ev.detail?.step) { | ||
| this._step = ev.detail.step; | ||
| } else { | ||
| this._step += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid incrementing enum values directly for step transitions
Incrementing the STEP enum using this._step += 1 may cause unexpected behavior if the enum values are changed or reordered in the future. It's safer to define explicit transitions between steps.
Consider modifying the code to use a mapping or a switch statement for step transitions. Here's an example using a switch statement:
private _nextStep(ev) {
if (ev.detail?.updateConfig) {
this._fetchAssistConfiguration();
}
if (!ev.detail?.noPrevious) {
this._previousSteps.push(this._step);
}
if (ev.detail?.step) {
this._step = ev.detail.step;
} else {
switch (this._step) {
case STEP.UPDATE:
this._step = STEP.CHECK;
break;
case STEP.CHECK:
this._step = STEP.WAKEWORD;
break;
case STEP.WAKEWORD:
this._step = STEP.AREA;
break;
// Add other cases as needed
default:
// Handle unexpected cases or set a default step
break;
}
}
}| export const enum STEP { | ||
| INIT, | ||
| UPDATE, | ||
| CHECK, | ||
| WAKEWORD, | ||
| AREA, | ||
| PIPELINE, | ||
| SUCCESS, | ||
| CLOUD, | ||
| ADDONS, | ||
| CHANGE_WAKEWORD, | ||
| } |
There was a problem hiding this comment.
Change 'const enum' to 'enum' to avoid compatibility issues
Using const enum is not supported by bundlers and is incompatible with TypeScript's --isolatedModules mode. This can lead to import issues and unexpected behavior.
Apply this diff to convert it to a regular enum:
-export const enum STEP {
+export enum STEP {
INIT,
UPDATE,
CHECK,
WAKEWORD,
AREA,
PIPELINE,
SUCCESS,
CLOUD,
ADDONS,
CHANGE_WAKEWORD,
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const enum STEP { | |
| INIT, | |
| UPDATE, | |
| CHECK, | |
| WAKEWORD, | |
| AREA, | |
| PIPELINE, | |
| SUCCESS, | |
| CLOUD, | |
| ADDONS, | |
| CHANGE_WAKEWORD, | |
| } | |
| export enum STEP { | |
| INIT, | |
| UPDATE, | |
| CHECK, | |
| WAKEWORD, | |
| AREA, | |
| PIPELINE, | |
| SUCCESS, | |
| CLOUD, | |
| ADDONS, | |
| CHANGE_WAKEWORD, | |
| } |
Tools
Biome
[error] 28-39: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
| .label=${this.hass.localize("ui.dialogs.generic.close") ?? | ||
| "Close"} |
There was a problem hiding this comment.
Update the label for the back navigation button
The label for the back button is currently set to "Close," which may be misleading to users. It should be updated to "Back" to accurately reflect its function.
Apply this diff to correct the label:
- .label=${this.hass.localize("ui.dialogs.generic.close") ??
- "Close"}
+ .label=${this.hass.localize("ui.dialogs.generic.back") ??
+ "Back"}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .label=${this.hass.localize("ui.dialogs.generic.close") ?? | |
| "Close"} | |
| .label=${this.hass.localize("ui.dialogs.generic.back") ?? | |
| "Back"} |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/data/assist_satellite.ts (1 hunks)
- src/dialogs/config-flow/step-flow-create-entry.ts (3 hunks)
- src/panels/config/devices/ha-config-device-page.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/dialogs/config-flow/step-flow-create-entry.ts
- src/panels/config/devices/ha-config-device-page.ts
Additional context used
Biome
src/data/assist_satellite.ts
[error] 5-7: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
Additional comments not posted (7)
src/data/assist_satellite.ts (7)
1-3: LGTM: Imports are appropriate and concise.The imports are relevant to the functionality being implemented in this file. Good job on keeping the imports minimal and focused.
9-25: LGTM: Well-structured interfaces with clear type definitions.The interfaces
WakeWordInterceptMessage,WakeWordOption, andAssistSatelliteConfigurationare well-defined and provide a clear structure for the assistant satellite system. The naming conventions are consistent and descriptive, and the types used are appropriate for their purposes.
27-35: LGTM: Well-implemented subscription function.The
interceptWakeWordfunction is well-implemented. It correctly uses the HomeAssistant's connection to subscribe to wake word interception messages. The function signature is clear, and the callback type matches theWakeWordInterceptMessageinterface.
37-53: LGTM: Well-implemented API interaction functions.Both
testAssistSatelliteConnectionandassistSatelliteAnnouncefunctions are well-implemented:
testAssistSatelliteConnectioncorrectly usescallWSfor a WebSocket call to test the connection.assistSatelliteAnnounceappropriately usescallServiceto send an announcement.The types and parameters are well-defined and suitable for their purposes.
55-73: LGTM: Well-implemented configuration management functions.Both
fetchAssistSatelliteConfigurationandsetWakeWordsfunctions are well-implemented:
fetchAssistSatelliteConfigurationcorrectly specifies the return type asAssistSatelliteConfigurationand uses the appropriate WebSocket call.setWakeWordstakes an array of wake word IDs as expected and makes the correct WebSocket call to update the configuration.The types and parameters are well-defined and suitable for their purposes.
75-79: LGTM: Well-implemented support check function.The
assistSatelliteSupportsSetupFlowfunction is well-implemented:
- It correctly uses the
supportsFeatureutility to check for the ANNOUNCE feature.- It handles the case where
assistSatelliteEntitymight be undefined.- The function is concise and readable.
Good job on creating a clear and efficient support check.
1-79: Overall, well-implemented module for assist satellite functionality.This new file introduces a comprehensive set of interfaces and functions for managing an assistive satellite system. The code is well-structured, with clear type definitions and appropriate use of Home Assistant's API. The only suggestion for improvement is to change the const enum to a regular enum for better compatibility.
Great job on implementing this new feature!
Tools
Biome
[error] 5-7: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/data/assist_satellite.ts (1 hunks)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-update.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/dialogs/voice-assistant-setup/voice-assistant-setup-step-update.ts
Additional context used
Biome
src/data/assist_satellite.ts
[error] 6-8: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
Additional comments not posted (6)
src/data/assist_satellite.ts (6)
10-12: LGTM: WakeWordInterceptMessage interfaceThe
WakeWordInterceptMessageinterface is well-defined and serves its purpose of representing wake word intercept messages.
14-18: LGTM: WakeWordOption interfaceThe
WakeWordOptioninterface is well-structured, providing all necessary properties to represent a wake word option, including its identifier, the wake word itself, and the languages it has been trained in.
20-26: LGTM: AssistSatelliteConfiguration interfaceThe
AssistSatelliteConfigurationinterface is well-defined, providing a comprehensive structure for the assist satellite configuration. It includes all necessary properties such as active and available wake words, maximum active wake words, and relevant entity IDs.
28-36: LGTM: interceptWakeWord functionThe
interceptWakeWordfunction is well-implemented. It correctly uses the Home Assistant WebSocket connection to subscribe to wake word intercept messages for a specific entity. The function signature and implementation are appropriate for its purpose.
38-81: LGTM: Assist satellite interaction functionsThe functions
testAssistSatelliteConnection,assistSatelliteAnnounce,fetchAssistSatelliteConfiguration,setWakeWords, andassistSatelliteSupportsSetupFloware well-implemented and serve their respective purposes effectively. They use appropriate Home Assistant API calls and follow consistent patterns for interacting with the assist satellite system.The
assistSatelliteSupportsSetupFlowfunction, in particular, includes proper checks for entity availability and feature support, which is a good practice for ensuring the reliability of the setup flow.
1-81: Excellent implementation of the assist satellite data layerThis file provides a comprehensive and well-structured foundation for interacting with the assist satellite system. It aligns perfectly with the PR objectives of introducing an MVP for the voice assist setup flow. The interfaces and functions cover all necessary aspects, including configuration management, wake word handling, and system interactions.
Key strengths of this implementation:
- Clear and concise interface definitions
- Consistent use of Home Assistant API calls
- Proper type annotations throughout the file
- Thoughtful function implementations that cover various use cases
The only minor issue (const enum usage) has been addressed in a previous comment. Once that's resolved, this file will serve as an excellent data layer for the voice assist feature.
Great job on laying the groundwork for this new feature!
Tools
Biome
[error] 6-8: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
| export const enum AssistSatelliteEntityFeature { | ||
| ANNOUNCE = 1, | ||
| } |
There was a problem hiding this comment.
Convert const enum to regular enum for better compatibility
The const keyword on the enum declaration can lead to compatibility issues with bundlers and the 'isolatedModules' mode in TypeScript, as pointed out by the static analysis tool.
To address this, please change the enum declaration to a regular enum:
export enum AssistSatelliteEntityFeature {
ANNOUNCE = 1,
}This change ensures better compatibility while maintaining the same functionality.
Tools
Biome
[error] 6-8: The enum declaration should not be const
Const enums are not supported by bundlers and are incompatible with the 'isolatedModules' mode. Their use can lead to import inexistent values.
See TypeScript Docs for more details.
Safe fix: Turn the const enum into a regular enum.(lint/suspicious/noConstEnum)
| @state() private _showFirst = false; | ||
|
|
||
| @state() private _showSecond = false; | ||
|
|
||
| @state() private _showThird = false; | ||
|
|
||
| @state() private _showFourth = false; |
There was a problem hiding this comment.
Can't we just used one counter for raspberry pi and one counter for home assistant cloud?
There was a problem hiding this comment.
Yeah but it gonna be removed anyway I think 🤷🏻♂️
|
@silamon This will not be available for users as no device implement this. It will be at first for internal tests using nightly build. |
Proposed change
Adds an MVP voice assist setup flow.
Copy needs to be updated (and translated if final)
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Documentation