Skip to content

Clear a stale pin select when its value is unset#990

Merged
bdraco merged 3 commits into
mainfrom
clear-stale-pin-select
Jun 26, 2026
Merged

Clear a stale pin select when its value is unset#990
bdraco merged 3 commits into
mainfrom
clear-stale-pin-select

Conversation

@bdraco

@bdraco bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

Adding a bus device (e.g. an atm90e32 sensor), clicking "+ Add spi" to add the bus, then returning left the required CS Pin select showing a GPIO it never actually held; the Add button stayed greyed out because the value was never in form state. The same pin select DOM node is reused across the detour (sensor to spi to sensor), so the CS Pin kept displaying the SPI Clk Pin's GPIO that bled in from the detour.

The pin select opts out of the generic value sync (data-no-value-sync) and runs _syncSelectedAttr, which bailed when no option was selected; that left the stale .value in place. It now syncs to the selected option including the empty case, so a pin whose value is unset clears to empty. The unit picker (the other data-no-value-sync user) always has a default unit selected, so it never hits the empty branch.

Related issue or feature (if applicable):

  • n/a (found in testing the bus-dependency add flow)

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed

Checklist

  • The code change is tested and works locally.
  • npm run lint passes.
  • npm run test passes.
  • Tests have been added to verify that the new code works (where applicable).

@github-actions github-actions Bot added the bugfix Bug fix label Jun 26, 2026
@bdraco bdraco mentioned this pull request Jun 26, 2026
Merged
14 tasks
@bdraco bdraco marked this pull request as ready for review June 26, 2026 01:40
Copilot AI review requested due to automatic review settings June 26, 2026 01:40
Copilot AI reviewed Jun 26, 2026
View reviewed changes

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

Fixes a UI state-sync bug in the device config entry form where a reused <wa-select> (notably the pin picker) could keep displaying a stale GPIO after navigating through a dependency “detour” (e.g. sensor → add SPI bus → back), even though the form state had no value set.

Changes:

  • Adjust _syncSelectedAttr so it syncs to the selected option including the empty/unset case, clearing stale .value when no option is selected.
  • Add inline rationale explaining why the empty case must be handled for pin selects that reuse the same DOM node.

Comment thread src/components/device/config-entry-form.ts Outdated
Comment thread src/components/device/config-entry-form.ts
Comment on lines 515 to 519
const selectedOption = select.querySelector<HTMLElement & { value: string }>(
"wa-option[selected]"
);
const desired = selectedOption?.value ?? "";
if (!desired) return;
const current = Array.isArray(select.value)
@esphbot

esphbot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

esphbot
esphbot approved these changes Jun 26, 2026
View reviewed changes

@esphbot esphbot 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.

No blocking issues found.

Copilot AI review requested due to automatic review settings June 26, 2026 02:03
@bdraco

bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Addressed both doc nits in 93ac4c8:

  1. Reworded the config-entry-form.ts comment generically: "the other data-no-value-sync selects always render a ?selected option (the unit pickers keep the in-use unit; exclusive group selects a non-empty placeholder sentinel), so only the pin select reaches the empty branch" — no longer singles out the unit picker.
  2. Updated the now-stale exclusive-group.ts NO_SELECTION comment: dropped the "(which no-ops on "")" clause this PR invalidated, kept the round-trip rationale.

Copilot's regression-test suggestion is already covered by config-entry-form-pin-sync.test.ts (clear-on-unset / sync-to-selected / no-op-when-correct).

Copilot AI reviewed Jun 26, 2026
View reviewed changes

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@bdraco: Addressed both doc nits in 93ac4c8:

Verified the invariant independently. Float-with-unit keeps unit in pickerUnitOptions; time-period selects from TIME_PERIOD_UNITS; exclusive-group selects __none__. Only pin select hits empty branch. Safe.

@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

PR Review — Clear a stale pin select when its value is unset

Correct, well-scoped bugfix with focused test coverage. Merge-ready.

Strengths:

  • Root cause precisely identified: the early if (!desired) return; left a stale .value on a reused pin-select DOM node, so the CS Pin kept the SPI Clk Pin's GPIO across the dep detour. Removing it lets the empty case clear correctly.
  • Inline comment now states the invariant generically (per Copilot/earlier review feedback) instead of singling out the unit picker, so it won't rot as consumers are added.
  • exclusive-group.ts sentinel comment updated to drop the now-false "no-ops on ''" rationale while keeping the round-trip justification.
  • New config-entry-form-pin-sync.test.ts covers all three branches: clear-on-unset, sync-to-selected, and no-write-when-correct.

Verified independently that the change is safe for every other data-no-value-sync select — each always renders a ?selected option (float-with-unit keeps unit in pickerUnitOptions, time-period selects from TIME_PERIOD_UNITS, exclusive-group selects the __none__ sentinel), so only the pin select reaches the empty branch. No regressions.

Checklist

  • Fix targets the actual buggy code path
  • No regression for other data-no-value-sync selects
  • New behavior has test coverage
  • Comments stay consistent with the change

Automated review by Kōan (Claude) HEAD=93ac4c8 1 min 1s

esphbot
esphbot approved these changes Jun 26, 2026
View reviewed changes

@esphbot esphbot 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.

No blocking issues found.

@bdraco bdraco merged commit 422f75f into main Jun 26, 2026
9 checks passed
@bdraco bdraco deleted the clear-stale-pin-select branch June 26, 2026 04:51
@esphome esphome Bot mentioned this pull request Jun 26, 2026
Merged
9 tasks
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

Copilot code review Copilot Copilot left review comments
+1 more reviewer
@esphbot esphbot esphbot approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix Bug fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bdraco @esphbot