PR 1204 review

harvester-ui-extension PR 1204 (abonillabeeche) at head 7342b74, reviewed 2026-11-03. It fixes the two things QA (asc5543) found on harvester 11550 with v1.9.1-rc4: the storage hints showed ", and an explicit Custom profile jumped back to a preset when its values matched one.

Preview: harvesterpr1204r.mf-created-dashboard.ui.rancher.space. Built from 7342b74, backend Harvester .118 (v1.9.0-rc4). Main baseline: the shared harvester-main preview.

Verdict: approve

Both bugs are fixed on the preview, on VM disk rows and on the standalone Volume page. Main shows both bugs the way QA reported them (clips below).
Copilot's point holds: a Custom that the UI only inferred from the disk values still follows a newly selected volume.
The tests catch both bugs. With main's two components put back, 5 of the new tests fail. CI is green, and the branch sits on main's head.
Backport: the 3 commits cherry-pick cleanly onto release-harvester-v1.9 (1.9.1-rc5). Its shell (3.0.12-rc.7) has the same Banner label-key path.
Two nits: the PR also fixes a third hint, the Filesystem cache warning, which the description doesn't mention (card 1). The new VMPerformanceOptions.test.js should be .ts like the test next to it (card 2).
Yu-Jack asked the author for a recording. The two main-vs-PR clips here answer that, if you want to attach them to your review.

Test it yourself

The feature is gated on Harvester v1.9.1, and our box runs v1.9.0-rc4. The gate reads the server-version setting from the store, so I lift it in the browser only. Nothing is written to Harvester. Log in as admin (HARVESTER_PASSWORD). The backend goes through the mf tunnel, so the SUSE VPN tunnels from the Mac need to be up.

Open VM create and run this in DevTools:
document.querySelector('#app').__vue_app__.config.globalProperties.$store.getters['harvester/byId']('harvesterhci.io.setting','server-version').value='v1.9.1'
Go to Volumes → Storage Performance Options → Custom → I/O Mode Native. The hint shows real quotes.
Tick Dedicated I/O Thread. The values now equal High Performance, and the profile stays on Custom. Open the VM-wide High Performance (I/O Threads and Multi-Queue) section: (defaults to "Shared").
For the Volume page, open Volume create and run the same snippet. Choose Custom → Native, then Show Advanced → Volume Mode Filesystem to see the third hint.
To compare, do the same on main.

main vs PR: VM create → Volumes

main: the Native and I/O Threads Policy hints show ". Ticking Dedicated I/O Thread on Custom flips the profile to High Performance, and the Custom fields disappear.

PR 1204: real quotes. Custom stays Custom with High Performance values, and again after every option is set back to Default.

main: Native I/O hint with " — main: Native I/O hint

PR: Native I/O hint with quotes — PR 1204: Native I/O hint

main: I/O Threads Policy hint with " — main: I/O Threads Policy hint

PR: I/O Threads Policy hint with quotes — PR 1204: I/O Threads Policy hint

Copilot's case: existing volume row (PR)

Two browser-only copies of a free volume, one saved with WriteBack and one with the High Performance options (harvesterhci.io/disk-* annotations). Picking the WriteBack one infers Custom. Switching to the other moves the profile to High Performance. After choosing Custom on purpose, switching volumes keeps Custom, and the fields take each volume's saved options.

Findings

The PR also fixes the Filesystem cache warning, but the description doesn't say so

DiskPerformanceOptions.vue:305–309

Nit

What it's about

Four banners in DiskPerformanceOptions.vue switched to label-key, and two in VMPerformanceOptions.vue. The description and the test steps cover only the Native I/O and I/O Threads Policy hints. The cache warning "None" (O_DIRECT) requires storage… had the same bug, and QA didn't hit it because it only shows for cache: none on a Filesystem volume. The other three switched banners have no quotes, so they look the same.

What I found

On the Volume create page with Volume Mode = Filesystem, main shows "None" (O_DIRECT) and the PR shows "None" (O_DIRECT).

main: Volume page, Filesystem warning with " — main: Volume create, Filesystem

PR: Volume page, Filesystem warning with quotes — PR 1204: Volume create, Filesystem

Suggestion

Add it to the summary and as a test step (Volume create → Show Advanced → Volume Mode Filesystem, Custom, Cache None), so QA verifies all three on the backport.

The new test file is JavaScript; the one next to it is TypeScript

VirtualMachineVolume/__tests__/VMPerformanceOptions.test.js

Nit

What it's about

The PR adds VMPerformanceOptions.test.js beside DiskPerformanceOptions.test.ts, from the same feature (PR 1133). The 4 tests in VirtualMachineVolume/type/__tests__/ are .ts as well. This would be the only .js test in the VM volume code.

What I found

I renamed it to .ts with no other change. Its 3 tests pass (18 of 18 in the folder), and the repo's lint script is clean on it. jest runs .ts through ts-jest with isolatedModules, so nothing else is needed.

Suggestion

Rename it to VMPerformanceOptions.test.ts, the same as the test next to it.

An explicit Custom outlives a volume switch, and isn't saved

DiskPerformanceOptions.vue:133–141

FYI

What it's about

userSelectedCustom lives on the component instance. It is set by the selector and cleared only when the user picks Default or High Performance.

What I found

In an existing-volume row, after the user picks Custom, selecting another volume keeps Custom while the fields load that volume's saved options (end of the clip above). I think that's right, since the user asked to tune by hand. Rows are keyed by volume.id, so the flag can't leak to another disk row when one is removed. After saving, the profile is inferred again, so a Custom with preset values reopens as that preset. The author already said so on the issue, so I'm not raising it.

Suggestion

Nothing to change. Leave it out of the review.

Checked

Case	main	PR 1204
Native I/O hint, VM disk row (QA step 1)	"None"	"None"
I/O Threads Policy hint (QA step 3)	"Shared"	"Shared"
Native I/O hint, standalone Volume create (QA step 4)	"None"	"None"
Filesystem cache warning, Volume create (not in the description)	"None"	"None"
Custom, then values = High Performance (VM row and Volume page)	Flips to High Performance	Stays Custom
Custom, then all values back to Default	Flips to Default	Stays Custom
Existing volume row: inferred Custom, then a volume with HP options (Copilot)	High Performance	High Performance
Existing volume row: explicit Custom, then switch volume	Follows the volume	Stays Custom (by design, card 2)
Tooltips with quotes (profile, cache, I/O mode)	Real quotes	Real quotes (untouched)
Unit tests, VirtualMachineVolume folder	5 of the new tests fail on main's components	38 passed, 1 skipped
`VMPerformanceOptions.test.js` renamed to `.ts` (card 2)		Passes as is; lint clean
Full jest suite		155 passed, 1 skipped
CI at 7342b74		lint, unit-tests, DCO, Mergify green; based on main head 77b6435
Backport to release-harvester-v1.9		Cherry-picks cleanly; same Banner `label-key` in shell 3.0.12-rc.7
Saving a VM or volume	Not done: the write path is unchanged, and the box is shared. QA already checked the generated specs on rc4.

Suggested review (Approve)

Preview: https://harvesterpr1204r.mf-created-dashboard.ui.rancher.space/ (built from 7342b74, backend Harvester v1.9.0-rc4 with the v1.9.1 gate lifted in the browser)

I tested this against main on a preview. The Native I/O and I/O Threads Policy hints now show real quotes, on the VM disk rows and on the Volume page. An explicit Custom stays Custom when the values match Default or High Performance, and an inferred Custom still follows the volume in an existing-volume row. The recordings below show main and then this PR, in case they help with the recording Yu-Jack asked for.

One small thing: the PR also fixes the Filesystem cache warning ("None" (O_DIRECT) requires storage…, shown for cache None on a Filesystem volume). Could you add it to the description and test steps so QA checks it on the backport too? And could VMPerformanceOptions.test.js be .ts, like DiskPerformanceOptions.test.ts next to it? It passes as is after the rename. LGTM.

Attach vid/vm-form-main.mp4 and vid/vm-form-pr.mp4 (attach-pr-video). Not posted.