PR 1227 review
harvester-ui-extension PR 1227 (pohanhuang) at head 26fdb45, reviewed 2026-11-09. It adds the same tooltip to the TPM Persistent State and EFI Persistent State checkboxes and a "Learn how to configure vmstate-persistence" docs link under them. Labels feat and require backport/v1.10. The task, harvester 12181 (v1.10.0), was opened by the author after Webber asked a UX question on the webhook PR.
Preview: harvesterpr1227r.mf-created-dashboard.ui.rancher.space, built from 26fdb45, backend Harvester .105 (v1.10.0-rc1, default install, so vmstate-persistence exists). No main baseline: the PR only adds code, and main shows nothing here. Cody's "before" video covers it.
Verdict: your concern holds. The code is fine, but nobody designed the flow. Comment on top of Cody's Request changes. Grade C
- Who it reaches: the hint never checks whether
vmstate-persistenceexists, although the form already loads every StorageClass. Everyone sees it: every default install, plain TPM or EFI, VMs that already work, and the read-only view. The one cluster it was written for, installed without the Longhorn disk, gets a pixel-identical screen. - Where it sends them: the link lands on "Install the CSI driver", which never mentions
vmstate-persistence, TPM or EFI. No docs page covers it, and the docs note promised on 12096 doesn't exist yet. - What it says: it offers only the CSI route. The webhook's own error says "Add a Longhorn disk or create this storage class".
- Simplest replacement: one condition: a Persistent State box is ticked and the class isn't in the loaded list. That also covers Cody's version-gate and view-mode comments. The look goes to the designer.
- Code level: small and clean, and it follows the repo's own
docLinkpattern. CI is green. The version handling works (v1.10, v1.9 and v1.5 each get their docs; below v1.5 nothing shows).
Status
| PR | Open, mergeable, one commit. CI green: lint, unit tests, DCO |
| Reviews | Cody: Changes requested, 17:17 UTC today, 4 inline comments with screenshots plus before/after videos. ibrokethecloud and houhoucoop are requested and haven't replied. No approvals. The author hasn't answered yet |
| Webhook it pairs with | harvester PR 12096, open. It rejects a VM with persistent vTPM/EFI when the class is missing. Still moving today: votdev noted that CBT also triggers it, and w13915984028 is questioning the Update/runStrategy logic. The condition and message may change |
| The install option it's for | Skip Longhorn default disk: HEP 11875 and installer PR 12122, both open. Until that ships, every cluster has the class unless someone deleted it |
| Docs | None. The author wrote "I'll also add a note to the docs" on 12096 (11-06), but there's no docs PR. Webber asked for the guidance to cover suitable backends, access modes and live-migration limits |
Who sees the hint
| Situation | What the user needs | What the PR shows |
|---|---|---|
| Default install, plain TPM or EFI ticked (every Windows 11 VM needs TPM) | Nothing | Link under the row, and a "Before enabling this option…" tooltip |
| Default install, Persistent State ticked | Nothing, the class exists | Same |
| Editing a VM whose persistent EFI already works | Nothing | Same |
| Read-only Show Configuration | Nothing | Link under a greyed, unticked box |
| v1.5 to v1.9 | Nothing, the chart always creates the class | Same |
| Installed without the Longhorn disk, Persistent State ticked (the case it's for) | "The class is missing: add a Longhorn disk or create it", before Create | Identical to the default install |
Only the last row needs a message. The others are listed to show who sees it, not as separate fixes.
vmstate-persistence (real .105), TPM ticked, Persistent State off
Does it read like AI?
- The code doesn't. It's 31 lines in the repo's own pattern: the same
docLinkand relative imports as ten other files, and a computed like its neighbours. - The copy and the link do. "Ensure that a compatible CSI driver provides…" never says what compatible means. "Learn how to configure vmstate-persistence" promises a page that doesn't exist, so the anchor was picked by its section title. The same two sentences sit on both boxes.
- The real gap is the process. Webber, a backend reviewer, asked for "a hint next to those options with a link to setup guidance". The author opened the task with that solution already written in and built exactly that. No designer or UI owner was in the loop. The form already knows the answer, and the hint ignores it.
Findings (both passes, re-checked)
1
The hint never checks whether the class exists
edit/kubevirt.io.virtualmachine/index.vue:128-131 · our pass · covers Cody's version and view-mode comments
What I found
showPersistentStateStorageHint keys on tpmEnabled/efiEnabled, not the persistent flags, and never looks at StorageClasses. The VM mixin already loads them (mixins/harvester-vm/index.js:96), and the name is already a constant (INTERNAL_STORAGE_CLASS.VMSTATE_PERSISTENCE, config/types.js:31). The webhook in 12096 only rejects persistent vTPM/EFI. Cody's two comments, "shows on v1.5 to v1.9 where the class exists" and "shows in the read-only view", are both symptoms of this.What I'd suggest
Show the message only when a Persistent State box is ticked and
vmstate-persistence isn't in the loaded list. Default installs then see nothing new, and the one user with the problem sees it before Create. Tooltip, inline text or banner is the designer's call.Evidence
Seen live: TPM with and without the class, pixel-identical. EFI the same, except for a few pixels in the left tab list. View mode and an existing VM checked too. Read in the code at
26fdb45.2
The link goes to a page that doesn't cover it
config/doc-links.js:8 · both passes · already posted by Cody
What we add
No page in
harvester/docs explains how to provide vmstate-persistence. The only mention, volume/create-volume.md:100, says not to use it. vm/create-vm.md still says "only non-persistent vTPMs are supported". The author promised a docs note on 12096, but no PR exists.What I'd suggest
A one-line reply in Cody's thread: drop the link until that page exists.
3
The copy offers only the CSI route
l10n/en-us.yaml:781 · our pass
What I found
The tooltip says "ensure that a compatible CSI driver provides a StorageClass named vmstate-persistence". The webhook (12096,
pkg/util/vmstate/vmstate.go) says "Add a Longhorn disk or create this storage class". The HEP says adding a Longhorn disk re-creates the class, which is probably the easy path for someone who skipped the disk. "Before enabling this option" also reads as a to-do on every cluster.What I'd suggest
Give the same two options as the webhook, once its message is final.
4
Should it wait for the backend?
PR body · our pass
The webhook (12096), the install option (11875, 12122) and the docs are all open, and the webhook's condition was still being questioned today. Landing and backporting the copy before they settle risks a second round.
5
Test steps point to the wrong tab
PR description, step 2 · our pass
"Go to the Advanced Options section", but the checkboxes are on Basics (
index.vue:756, seen live). QA would look in the wrong place.6
Unused class
index.vue:859 · our pass
persistent-state-storage-hint has no style and nothing references it. It will probably go anyway if finding 1 changes the block.7
New tab without a cue
index.vue:861 · Cody only
Holds: a plain
<a target="_blank"> with no icon or screen-reader text. The repo is mixed: VirtualMachineVolume, the addon list and the support page use icon-external-link, while UpgradeInfo and the host list don't. Our pass called it the repo convention, which is only half right. Not repeated.The two passes
Pass A here is Cody's own review on the PR: his process, run by him, posted at 17:17 UTC. So I didn't run his skills a second time. Pass B is ours, run by a fresh agent that never saw Cody's comments.
| Pass A (Cody, posted) | Pass B (ours, fresh agent) | |
|---|---|---|
| Findings | 4 inline comments | 1 blocker, 2 suggestions, 2 nits, 1 question |
| Both found | The link's page doesn't cover vmstate (2). The hint shows where the class exists: A through the v1.5 to v1.9 version gate, B on every cluster with no check (1) | |
| Only one found | New-tab cue (7). Link in the read-only view (part of 1; B checked edit mode, not view) | Missing-class case looks identical, and the root cause in the condition (1). Copy vs. the webhook (3). Timing (4). Test-steps tab (5). Unused class (6) |
| Dropped | None. B's "the link cue matches the repo" was corrected: the repo is mixed | |
| Output | Before/after videos, annotated screenshots on GitHub | review, coverage grid, 16 live checks, 8 screenshots |
| Cost | Cody's run, not ours | ~30 min, ~200k tokens, 90 tool calls |
Same pattern as PR 19807 and 19808: his pass catches user-visible symptoms one by one. Ours went to the shared cause and the backend mismatch, which is what answers your UX question.
Checked
| Check | Result |
|---|---|
| Default .105, TPM then EFI ticked, Persistent State off | Link and tooltip shown |
Same, with vmstate-persistence removed from the StorageClass response | Identical (pixel diff: none for TPM; for EFI, only the left tab list) |
| Persistent EFI ticked | Unchanged |
| Edit a VM with persistent EFI on (spec rewritten in the browser) | "Before enabling this option…" on a working VM |
| Read-only Show Configuration, plain EFI | Link under a disabled, unticked box |
| Link target, v1.10 | Anchor resolves. The section never mentions vmstate, TPM or EFI |
| Server version rewritten to v1.9.1 / v1.5.0 / v1.4.3 | /v1.9/, /v1.5/, hidden below v1.5 |
| Keyboard and ARIA on the info icon | Shell tooltip button, focusable, named "More Info" |
| CI | lint, unit tests, DCO pass |
Not checked: what Create does on a cluster without the class (the webhook isn't merged, and all writes were blocked); a real install without the Longhorn disk; whether adding a disk really re-creates the class (the HEP's claim); dark theme. Nothing on .105 was changed: every non-GET after login was aborted. Unrelated, not for this PR: editing default/forklift-vm-designer-3 renders a blank form with TypeError … reading 'memory' in getInitConfig.
What I'd post
Event: Comment. Cody's Request changes already blocks the PR. Screenshots go into the inline comments; there's no preview link and no code.
Body
Thanks for picking this up! Adding to Cody's comments: I think most of them come from the hint not knowing whether
vmstate-persistence is there, so I left that below. Should this wait until harvester/harvester#12096 and the skip-Longhorn-disk option settle, so the text matches what ships? Small one for the test steps: the checkboxes are on the Basics tab, not Advanced Options.index.vue:128
The form already loads all StorageClasses, so could we show this only when a Persistent State box is ticked and
Images: "with the class" / "without it" pair.
vmstate-persistence isn't in the list? Right now it shows as soon as plain TPM or EFI is ticked, on every cluster that has the class (all default installs, and v1.5 to v1.9 as Cody noted), and in the read-only view, while a cluster without the class looks exactly the same. How it should look can go to the designer.Images: "with the class" / "without it" pair.
en-us.yaml:781
The webhook in harvester/harvester#12096 says "Add a Longhorn disk or create this storage class", but this only mentions a CSI driver, and "compatible" isn't explained anywhere. Could it give the same two options as the webhook? "Before enabling this option" also reads like a to-do on clusters where nothing is needed.
Image: the tooltip.
Image: the tooltip.
Reply in Cody's thread on doc-links.js:8
I couldn't find any page that explains how to provide
vmstate-persistence either (the docs note mentioned on harvester/harvester#12096 isn't there yet). I'd drop the link until that page exists.Left out: the unused class (6) and the new-tab cue (7, already Cody's).