Improve user and person dialogs #21162
Conversation
WalkthroughWalkthroughThe overall changes focus on importing custom components to enhance the UI, restructuring layouts for better organization, and updating user interaction elements across several dialog components. Specific improvements include integrating settings rows, refining button functionalities, adding user privileges control, and providing better descriptive text in the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DialogAddUser
participant CustomComponents
participant Backend
User->>DialogAddUser: Open Add User Dialog
DialogAddUser->>CustomComponents: Load ha-settings-row
DialogAddUser->>CustomComponents: Load ha-button
DialogAddUser->>CustomComponents: Load ha-icon-button
DialogAddUser->>CustomComponents: Load ha-alert
CustomComponents-->>DialogAddUser: Components Loaded
User->>DialogAddUser: Fill in User Details
User->>DialogAddUser: Click Create User
DialogAddUser->>Backend: Submit User Details
Backend-->>DialogAddUser: User Created
DialogAddUser-->>User: Display Success Message
sequenceDiagram
participant User
participant DialogPersonDetail
participant CustomComponents
User->>DialogPersonDetail: Open Person Detail Dialog
DialogPersonDetail->>CustomComponents: Load ha-button
DialogPersonDetail->>CustomComponents: Load ha-icon-button
DialogPersonDetail->>CustomComponents: Load ha-settings-row
CustomComponents-->>DialogPersonDetail: Components Loaded
User->>DialogPersonDetail: Update Person Details
DialogPersonDetail-->>User: Display Updated Details
sequenceDiagram
participant User
participant DialogUserDetail
participant CustomComponents
participant Backend
User->>DialogUserDetail: Open User Detail Dialog
DialogUserDetail->>CustomComponents: Load ha-settings-row
DialogUserDetail->>CustomComponents: Load ha-button
DialogUserDetail->>CustomComponents: Load ha-icon-button
DialogUserDetail->>CustomComponents: Load ha-alert
CustomComponents-->>DialogUserDetail: Components Loaded
User->>DialogUserDetail: Modify User Settings
DialogUserDetail->>Backend: Submit Changes
Backend-->>DialogUserDetail: Confirm Changes
DialogUserDetail-->>User: Display Updated Settings
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional context usedBiome
Additional comments not posted (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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 1
Outside diff range comments (4)
src/panels/config/users/dialog-add-user.ts (2)
Line range hint
272-272: Type safety issue with the use of 'any'.Using 'any' type can lead to potential bugs and maintenance issues as it bypasses TypeScript's static type checking. Consider specifying more appropriate types to enhance code robustness and maintainability.
- catch (err: any) { + catch (err: Error) {Also applies to: 285-285
Line range hint
293-293: Avoid non-null assertions when not absolutely necessary.Using non-null assertions bypasses TypeScript's nullability checks, which can lead to runtime errors. Consider using optional chaining or proper checks before accessing properties.
- this._params!.userAddedCallback(user); + this._params?.userAddedCallback(user);src/panels/config/users/dialog-user-detail.ts (1)
Line range hint
57-57: Function complexity exceeds recommended limit.The
rendermethod's complexity is too high, which can make the code harder to maintain and understand. Consider refactoring into smaller, more manageable functions.src/panels/config/person/dialog-person-detail.ts (1)
Line range hint
118-118: Multiple instances of unsafe non-null assertions and recommended use of optional chaining.The frequent use of non-null assertions (
!) could lead to runtime errors if the objects turn out to be null. It's recommended to use optional chaining (?.) to safely access properties.- this._params!.entry.name + this._params?.entry?.name - this._params!.removeEntry() + this._params?.removeEntry() - this._params!.entry.username! + this._params?.entry?.username - this._params!.updateEntry(values) + this._params?.updateEntry(values) - this._params!.entry.user_id + this._params?.entry?.user_id - this._params!.refreshUsers() + this._params?.refreshUsers()Also applies to: 128-128, 129-129, 145-145, 150-150, 188-188, 201-201, 211-211, 226-226, 229-229, 239-239, 240-240, 356-356, 357-357, 371-371, 374-374, 378-378, 379-379, 445-445, 457-457
There was a problem hiding this comment.
Great to have some improvements in this area.
Just had a few problems while testing with the change password dialog (dialog-admin-change-password):
- There's a duplicate primaryAction there (so hitting enter doesn't save). Cancel should be secondaryAction.
You can escape the dialog or click in the scrimaction
The escape / scrim action is enabled for the change-username dialog, not for the dialog-admin-change-password.
|
Thank you @silamon, I will address the feedbacks 🙂 |
- add procedure to refelct current software - related to home-assistant/frontend#21162
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range comments (4)
src/panels/config/users/dialog-admin-change-password.ts (4)
Line range hint
75-75: Avoid usinganytype for error handling.Using
anytype can lead to potential bugs and maintainability issues as it disables TypeScript's static type checking. Consider specifying a more explicit type for the error handling.- catch (err: any) { + catch (err: Error) {
Line range hint
80-81: Consider using optional chaining for better safety and readability.Optional chaining (
?.) can be used here to safely access nested properties. This will prevent runtime errors if some properties are not available.- if (!this._userId || !this._data?.new_password) return; + if (!this._userId || !this._data?.new_password?) return;
Line range hint
163-163: Remove non-null assertion to ensure type safety.The non-null assertion operator (
!) bypasses TypeScript's strict null checks, which can lead to unexpected runtime errors. Consider handling potential null or undefined values explicitly.- this._userId!, + this._userId ?? throw new Error("User ID is required");
Line range hint
167-167: Specify a more precise type instead ofany.Using
anyfor the error type in the catch block reduces the effectiveness of TypeScript's type system. Specifying a more detailed type can help catch errors at compile time.- catch (err: any) { + catch (err: Error) {
Proposed change
With the introduction of username edition (#21152), buttons didn't fit the different dialogs.
I used
ha-settings-rowcomponent as form elements to have consistent UI for each dialog.We have room for improvement to merge person/user or at least makes the UX better but that PR only changes the UI, not the UX.
Person dialog (without user access)
Person dialog (with user access)
Add user dialog
Delete user dialog
Advanced user dialog (normal user)
Advanced user dialog (system user)
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
Documentation
Refactor