Jira URL: https://ixsystems.atlassian.net/browse/NAS-141049

Claude finished @AlexKarpov98's task in 6m 17s —— View job

Review

Really nice work on the shared-utility extraction here 🎉 — translatedSignal(), tnSelectLabels, and tn-select-option-test-id.constant.ts each replace a pattern that was being re-derived per component, and the TnFormControlHarness + unreadableControl sentinel is a genuinely thoughtful answer to "half-migrated form, one indexed lookup." The a11y deltas are real wins too (div (click) → tn-icon-button [ariaLabel], .link-button as an actual <button>, 1-based row positions so repeated remove buttons stop sharing a name), and the FormArray version-counter → derived-signal rewrite in unlock-sed-disks is a solid cleanup.

🚩 The library bump is buried in a mislabeled commit, and its blast radius is much bigger than this PR

ed260e3 "NAS-141979: Update" moves @truenas/ui-components from ~0.3.26 to ~0.4.8 (package.json:118). That's load-bearing — f7fe11a's own message says "the library change is unreleased — this needs the published version bumped before it can merge" — but three things about it need attention:

• 1267 files in src/ import @truenas/ui-components; this PR touches 100. A minor bump of the design system silently re-skins every already-migrated area (shares-dashboard, audit, apps, dashboard widgets). If something regresses there, git bisect lands on a storage-migration PR titled "Migrate storage / pool-manager wizard to tn-*".
• The commit prefix is NAS-141979, not the branch's NAS-141049, and the message is Update. Per CLAUDE.md the format is NAS-<issue>: <description>.
• Strong suggestion: split the bump into its own PR (or at minimum its own commit with a real message enumerating what 0.4.x changes), and land it first. Then this PR is purely the storage migration.

Fix this →

🚩 The bump wasn't followed by a sweep — several workarounds now self-document as removable

tn-checkbox-change.utils.ts:11 says, verbatim: "Drop this helper — and the guards that call it — once the dependency range moves past 0.4.0." The range is now ~0.4.8, so that condition is met in this very PR. The helper and its single call site (pool-warnings.component.ts:82) should go.

Same staleness in the prose, which now describes a version the repo no longer pins:

• tn-checkbox-change.utils.ts:4 — "the pinned @truenas/ui-components (0.3.26)"
• unlock-sed-disks.component.ts:93 and disk-size-selects.component.ts:70 — "in the pinned 0.3.26 only tn-input, tn-select, tn-autocomplete and tn-chip-input consume TN_FORM_FIELD_CONTEXT"
• SKILL.md — the defects table header reads "package.json currently ~0.3.26", and the component-map row still says "verified in 0.3.26"

The TN_FORM_FIELD_CONTEXT one matters beyond documentation: both call sites fold the hint into the checkbox's [label], i.e. into its accessible name, and each carries a paragraph explaining that this is a deliberate name-vs-description downgrade forced by 0.3.26. If 0.4.8 wires tn-checkbox up to the field context, that trade-off is no longer necessary and the folded name is now just verbose for screen-reader users. Worth re-validating against the installed 0.4.8 rather than inheriting the 0.3.26 conclusion. (I couldn't check the library internals myself — see the note at the bottom.)

Fix this →

⚠️ The tn-radio-group test IDs hand-craft the prefix the playbook says not to hand-craft

All three call sites pass the prefix inside the base:

[testId]="['radio-button', 'encryption-type']"

This only resolves to the legacy radio-button-* because composeTestId's idempotent guard skips a prefix the base already starts with — and the playbook update in this same PR explicitly warns against relying on that ("it survives only because of the idempotent guard", plus "pass the bare semantic base"). It's structurally the same problem as the link-* → button-* shift, for which you prescribe host-level tnTestIdType="link" [tnTestId]="…" pinning. Either apply that remedy here (tnTestIdType="radio-button"), or add a defects-table/component-map row so the next migrator reading the playbook doesn't see this as a contradiction.

🧹 Dead code and a duplicated translated string

Removing ix-modal-header from import-pool.component.html was right (the panel host supplies the title), but it left two loose ends:

• helptextImport.title in helptext/storage/volumes/volume-import-wizard.ts:4 now has no readers — verified by grep across src/app.
• The title it used to supply is hard-coded as a literal in pools-dashboard.component.ts:104: title: this.translate.instant('Import Pool'). Prefer this.translate.instant(helptextImport.title) so the string stays in one place.

Also minor: import-pool.component.ts keeps both protected readonly helptext = helptextImport (template reads helptext.selectPool) and a separate protected readonly poolLabel = helptextImport.poolLabel. Pick one — either promote selectPool to its own field or read helptext.poolLabel in the template.

Fix this →

💭 Small simplification

draid-selection.component.ts:199-206 — the if (!nextOptions.length) branch is just the no-options case of the else if (!nextOptions.some(...)) check below it, and unsetControlIfNoMatchingOption already handles an empty list. Collapsing to a single unsetControlIfNoMatchingOption(...) followed by the default-to-0 guard would read more directly.

The rest holds up well under scrutiny: no leftover mat-* / Mat* / @angular/material anywhere in the changed code, the removed resource-card mixin has no surviving callers, ad-hoc ::ng-deep in storage-health-card was correctly folded into shared tn-card-inline-title / tn-details-item mixins, the panel title survives via the host so dropping ix-modal-header isn't a regression, and availableDiskCountForException's blank-row accounting is correct across the cases I traced. The spectator.component.submit() / canSubmit() spec calls are fine — those are SidePanelForm's host-facing API, not private internals.

Note on verification: node_modules isn't present in this checkout and I don't have permission to run yarn install, so I could not run yarn lint, the specs, or inspect the installed 0.4.8 to confirm which of the documented defects it actually fixes. The WebUI CI run was still queued when I looked — please make sure it goes green on the bumped version before merge, since that's the only signal covering the other ~1160 library consumers.
· Branch: NAS-141049

Codecov Report

❌ Patch coverage is 96.91877% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.72%. Comparing base (a5610f7) to head (ed260e3).
⚠️ Report is 12 commits behind head on master.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##           master   #13868      +/-   ##
==========================================
+ Coverage   87.63%   87.72%   +0.09%     
==========================================
  Files        1895     1904       +9     
  Lines       71860    72058     +198     
  Branches     9142     9206      +64     
==========================================
+ Hits        62972    63214     +242     
+ Misses       8888     8844      -44     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

Shouldnt tn-radio-group be part of the components library?