Skip to content

Preserve in-progress form values across the dep detour#989

Merged
bdraco merged 5 commits into
mainfrom
preserve-detour-form-values
Jun 26, 2026
Merged

Preserve in-progress form values across the dep detour#989
bdraco merged 5 commits into
mainfrom
preserve-detour-form-values

Conversation

@bdraco

@bdraco bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

When you fill some fields in the Add-component form (an SPI device's cs_pin, line frequency, etc.) and then click "+ Add " to add a missing dependency, returning to the form wiped everything you had typed; the form re-mounts and re-seeds from scratch, carrying back only the new dependency's id reference.

This snapshots the form's in-progress values when a "+ Add " detour starts (in the dialog, before the form unmounts) and overlays them on return via a new restoredValues step in buildInitialValues. The overlay lands after the seeded defaults and before prefillReference, so what you typed survives while the just-added dependency's id still wins for the reference field. The snapshot is only applied to the original form on return, not to the dependency's own form during the detour, so the dependency keeps its own auto-generated id (no id collision).

This is the input-preservation half; the separate stale-pin-display issue (a pin select showing a value it never held after a detour) is fixed in #990.

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 force-pushed the preserve-detour-form-values branch from dd23d21 to f24d4b8 Compare June 26, 2026 01:38
@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 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

Preserves in-progress <esphome-add-component-form> values when the user detours via “+ Add ” to create a missing dependency, by snapshotting current form values in the dialog and reapplying them during reseed on return.

Changes:

  • Add restoredValues plumbing from dialog → form → buildInitialValues() and overlay restored values before prefillReference.
  • Snapshot the form’s current values in add-component-dialog when starting a dep detour.
  • Add/extend unit tests covering the restore-vs-prefill precedence.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
test/components/device/add-component-form-seed-fns.test.ts Adds coverage for restored values overlay behavior and precedence vs prefillReference.
src/components/device/add-component-form.ts Adds restoredValues input + exposes currentValues for the dialog snapshot.
src/components/device/add-component-form-seed.ts Extends seeding context and overlays restored values before prefillReference.
src/components/device/add-component-dialog.ts Captures and passes restored values during dep detours.

Comment thread src/components/device/add-component-form-seed.ts
Comment thread src/components/device/add-component-dialog.ts
@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

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

Blocking issues found.

  • _onBundleSelected abandons the detour but leaves _returnValues set, leaking stale values onto the bundle's first form
  • Cancelling the detour with Back discards the in-progress values the PR set out to preserve

Copilot AI review requested due to automatic review settings June 26, 2026 01:50
@bdraco

bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Both lifecycle gaps fixed in 49b8054.

  1. Bundle path: added this._returnValues = null; to _onBundleSelected's clear block, alongside the other detour-state resets, so picking a bundle mid-detour no longer overlays the prior component's values onto the bundle's first form.

  2. Back path: made _onBack's _returnTo branch preserve the snapshot across _resetDetourState() (capture, reset, restore), so backing out of a dep returns to the original form with your input intact, consistent with the submit-return path. I went with preserving rather than discarding since, as you noted, both Back and completing-the-dep "return to the form."

Added two tests pinning each: bundle-mid-detour clears the snapshot, and Back preserves it (restores _selected to the original with _returnValues intact). Suite green (3986).

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 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread src/components/device/add-component-form.ts
Comment thread src/components/device/add-component-form-seed.ts
@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

@bdraco: Both lifecycle gaps fixed in 49b8054.

Both fixes confirmed correct and tested. One residual leak remains on a symmetric path. Bundle-advance branch in _submitComponent never clears _returnValues. After a mid-bundle detour, sequence leaks: fill A, detour to add dep, submit dep (submit-return keeps snapshot, correct), submit A, bundle-advance sets _selected=B without clearing snapshot, B mounts with A's values overlaid. Same class as the _onBundleSelected fix. Add this._returnValues = null; before this._selected = nextComponent; in the advance branch. Left inline.

@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

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

Blocking issues found.

  • _returnValues leaks into the next bundle step after a mid-bundle dep detour

@bdraco

bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Ran /simplify and tightened the _returnValues lifecycle (ecda9e3):

  • DRY: extracted _clearDetourFields() (the 5 detour fields, no seq bumps) shared by _resetDetourState and _onBundleSelected, removing the duplicated field list. The seq bumps stay in _resetDetourState only, since bumping them in the bundle path would invalidate the token its own hydrate already validated against.
  • Found + fixed a staleness gap: the bundle-advance branch in _submitComponent left _returnValues set, so a bundle step that detoured would bleed its snapshot onto the next step. Now cleared there (the normal-add branch already resets; submit-return intentionally keeps it for the restore).
  • Coverage: rewrote the test into a full lifecycle suite — capture (with/without a mounted form), the restore gate (mid-detour vs return vs fresh), and every exit (submit-return keeps, back keeps, bundle-pick clears, bundle-advance clears, reset clears).

The _restoredValuesForMount getter, the _onBack snapshot dance, currentValues, and the overlay ordering were all judged correctly minimal and kept. Suite green (3991).

@esphbot

esphbot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

PR Review — Preserve in-progress form values across the dep detour

Solid, well-tested fix. The detour value-preservation lifecycle is now correct across every exit path; only two doc/defensive nits remain.

What's done well:

  • The earlier blocking issues are genuinely resolved in this diff: _clearDetourFields() nulls _returnValues for the _onBundleSelected abandon path, _onBack preserves the snapshot so cancelling no longer discards typed values, and bundle-advance explicitly clears _returnValues so it can't bleed onto the next step.
  • The _restoredValuesForMount gate (restore only when _returnTo is null) is the right shape — it cleanly prevents the dep form from inheriting the original's id and colliding, which I confirmed against the form's reuse-and-reseed behavior.
  • Overlay ordering is correct and verified: it lands after seeded defaults and before prefillReference, so typed fields survive while the just-added dep's id still wins for the reference field. The new seed-fn test pins exactly this precedence.
  • The lifecycle test suite covers capture (with/without a mounted form), the mount gate (mid-detour vs return vs fresh), and all five exit paths — strong coverage for state this fiddly.

What needs attention (both non-blocking):

  • buildInitialValues header step-list docstring is now stale — doesn't mention the restore overlay (suggestion).
  • currentValues returns _values by reference; safe today given reassign-only writes, but a defensive copy would future-proof it (suggestion).

🟢 Suggestions

1. buildInitialValues step-list docstring omits the new restore overlay
src/components/device/add-component-form-seed.ts:140-148

The numbered contract docstring above buildInitialValues still reads:

  1. seed defaults → 2. id-gen → 3. board pins → 4. prefillReference → 5. prefillFields

The diff inserts the restoredValues overlay between step 3 (board pins) and step 4 (prefillReference), but this header list isn't updated, so the function's documented contract now disagrees with what it does.

Why it matters: the ordering here is subtle and user-visible — the whole correctness of the feature hinges on the overlay landing after seeded defaults but before prefillReference (so the just-added dep's id still wins). The inline comment at the overlay site documents this well, but the header list is the first thing a future editor reads, and a stale ordering contract is exactly how someone later moves the overlay to the wrong slot.

Fix: add a step, e.g. 3b. Overlay the user's pre-detour values, before prefillReference so the just-added dep's id still wins for the reference field. Doc-only; not a blocker. (Copilot flagged the same.)

 *  3. Seed pin entries from the board manifest.
 *  4. If we were just brought back from a "+ Add <domain>" detour,
 *     prefill the field that points at that domain with the new id.
 *  5. Overlay constraint-derived prefill fields last.

Checklist

  • State lifecycle consistent across all detour exit paths
  • Overlay/seed ordering correct and tested
  • No snapshot aliasing / mutation hazard
  • Restored values don't re-clobber edits on a still-mounted form
  • Required SeedContext field updated at all call sites (TS strict)
  • Doc comments match implementation — suggestion #1

Automated review by Kōan (Claude) HEAD=ecda9e3 3 min 3s

@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:40
@bdraco

bdraco commented Jun 26, 2026

Copy link
Copy Markdown
Member Author

Addressed both remaining nits in 16009a1:

  1. buildInitialValues doc: added the restore step to the numbered list (step 4, between board-pin seeding and the prefill steps) and renumbered, so the comment matches the actual overlay order.
  2. currentValues: now returns a shallow copy { ...this._values } so the snapshot is stable even if the form keeps editing its values after the dialog captures it.

The earlier Copilot "_returnValues leaks into bundle-advance" note predates ecda9e3 — that path now explicitly clears the snapshot (with a test). Suite green (3991).

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 5 out of 5 changed files in this pull request and generated no new comments.

@bdraco bdraco requested a review from Copilot June 26, 2026 04:57
@bdraco bdraco merged commit 6c97ced into main Jun 26, 2026
10 checks passed
@bdraco bdraco deleted the preserve-detour-form-values branch June 26, 2026 04:57

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 5 out of 5 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants