Skip to content

Late review comments for entity widget configuration - #7302

Merged
jpelgrom merged 3 commits into
home-assistant:mainfrom
jpelgrom:late-entitywidget-configure-review
Aug 5, 2026
Merged

Late review comments for entity widget configuration#7302
jpelgrom merged 3 commits into
home-assistant:mainfrom
jpelgrom:late-entitywidget-configure-review

Conversation

@jpelgrom

@jpelgrom jpelgrom commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

Adresses some late review comments on #7007 which were posted on #7288 but apply equally here

Checklist

  • New or updated tests have been added to cover the changes following the testing guidelines.
  • The code follows the project's code style and best_practices.
  • The changes have been thoroughly tested, and edge cases have been considered.
  • Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.
  • I have read the Open Home Foundation AI Policy.

Select exactly one option that describes AI usage in this contribution:

  • I have not used AI for this contribution.
  • AI assistance was used for this contribution.
  • AI fully generated the code for this contribution, but I've reviewed and understood it before submitting and will respond without AI during review.

Screenshots

n/a

Link to pull request in documentation repositories

n/a

Any other notes

Copilot AI lite review requested due to automatic review settings August 4, 2026 20:20
@jpelgrom
jpelgrom requested review from TimoPtr and removed request for Copilot August 4, 2026 20:20
Copilot AI lite review requested due to automatic review settings August 4, 2026 20:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Addresses late review feedback for the Compose-based entity widget configuration flow (#7007), focusing on code style/visibility cleanups and improving screenshot coverage around the server picker behavior.

Changes:

  • Moves constants/helpers to file scope and adjusts visibility/order of ViewModel functions for consistency.
  • Updates entity widget screenshot tests to cover states with and without the server picker.
  • Tweaks server selector behavior in the Compose screen (enabled/visibility handling).

Reviewed changes

Copilot reviewed 5 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
app/src/test/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureViewModelTest.kt Moves test helpers/constants out of a companion object to file-level privates.
app/src/screenshotTest/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureScreenshotTest.kt Renames preview state and adjusts state to exercise server-picker vs single-server UI.
app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureViewModel.kt Reorders and adjusts visibility of ViewModel methods (public/private/internal).
app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureState.kt Moves default text size constant to a private file-level constant.
app/src/main/kotlin/io/homeassistant/companion/android/widgets/entity/EntityWidgetConfigureScreen.kt Removes dropdown enabled gating in server selector (behavior change).

Comment on lines 249 to 258
if (!showServerSelector) return

HADropdownMenu(
items = items,
selectedKey = selectedServerId,
onItemSelected = onServerSelected,
label = stringResource(commonR.string.server_select),
placeholder = stringResource(commonR.string.server_select),
modifier = Modifier.formControlWidth(),
enabled = items.isNotEmpty(),
)

@jpelgrom jpelgrom Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Perfect second-guessing :D

The servers list being empty is quite unlikely, as it's read from the database and uses that list size > 1 OR a widget with invalid configuration exists (no valid server selected at all).

val showServerSelector = serversDropdownItems.size > 1 ||
    serversDropdownItems.none { it.key == selectedServerId }

@jpelgrom
jpelgrom requested a review from TimoPtr August 4, 2026 20:37
@jpelgrom
jpelgrom merged commit 7f238fd into home-assistant:main Aug 5, 2026
20 checks passed
@jpelgrom
jpelgrom deleted the late-entitywidget-configure-review branch August 5, 2026 16:34
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.

3 participants