NAS-141478 / 27.0.0-BETA.1 / Migrate Sharing/NVMe-oF slide-ins & modal-header to tn-*

[AlexKarpov98]

No description provided.

[bugclerk]

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

[claude]

Claude finished @AlexKarpov98's task in 5m 21s —— View job

---

Review

Nice migration — pulling the namespace form's logic into pure, unit-tested helpers (createNamespaceForm / syncNewFileControls / toNamespaceChanges) and killing the viewChild-based canSubmit/form getter plumbing makes both wrappers dramatically easier to follow, and the "New File leaves the group permanently INVALID" regression fix is a real user-facing win. 🎉 The comments explaining why (projection/ControlContainer, validator ownership, anchored /dev/zvol/ strip) are genuinely useful.

A few things worth a look:

1. open<R> now accepts SidePanelHostCloseable<R | null> — repo-wide loosening for one form — src/app/modules/slide-ins/form-side-panel/form-side-panel.service.ts:129

Every caller of FormSidePanelService.open loses the ability to reject a form whose closed only ever emits null, and the practical consequence for zvol is worth calling out explicitly: null is coerced to undefined by the truthy rule below, so a save that succeeded but returned no record closes the panel and silently skips the opener's onSuccess — no snackbar, no nvmeOfStore/dataset refresh. That's still better than master (which left the panel stuck open), but the doc comment reads as if null is a benign "saved, no record" payload when it's really "saved, reported as a cancel". Consider keeping ZvolFormComponent extends IxFormHostForm<Dataset> and having its two openers use open<Dataset | null>(…) instead, so the widening is local. At minimum, spell out the skipped-onSuccess consequence in the JSDoc. Fix this →

2. Unanchored label regex in the shared toggle helper — src/app/pages/sharing/nvme-of/namespaces/base-namespace-form/testing/namespace-form.testing.ts:23

loader.getHarness(TnButtonToggleHarness.with({ label: new RegExp(escapeRegExp(label)) }))

Escaping is right, but the pattern is unanchored, so 'File' would match 'Existing File' and any future label that's a substring of another silently resolves to the wrong toggle. Since the ✓ marker is a prefix, anchoring the tail (new RegExp(${escapeRegExp(label)}$)) gets you exact matching for free.

3. group() is read once in ngOnInit — base-namespace-form.component.ts:118

The device_type subscription and the initial syncNewFileControls capture the group from ngOnInit only. Both current hosts build the group as a field initializer and never swap it, so this is safe today — but as a required signal input it reads like it tracks changes, and a swapped group would silently leave the branch-sync wiring pointed at the old instance (Save then never re-enables). A one-line comment stating the group is expected to be stable, or moving the wiring into an effect, would remove the footgun.

4. Enter-to-submit is now inconsistent between the two namespace panels — add-subsystem-namespace.component.html:1

NamespaceFormComponent goes through <ix-form>, which renders a real <form> with ngSubmit, so Enter submits there; the wizard variant deliberately renders none, so Enter does nothing. The rationale in the comment (role gate) is sound, but the two panels look identical to a keyboard user and now behave differently. Worth confirming that's intentional given <ix-form>'s own form doesn't skip the role gate either.

5. Host/port specs lost their gating coverage — host-form.component.spec.ts:76, port-form.component.spec.ts

Swapping MatButtonHarness('Save').click() for spectator.component.submit() is correct now that the footer belongs to the host, and submit() is the public host API — no complaint there. But nothing in those two specs asserts the Save gate any more (the namespace spec does, via canSubmit()). A single expect(component.canSubmit()).toBe(false) before filling the form would restore it cheaply.

Minor / FYI

• The CodeQL "incomplete string escaping" alert on base-namespace-form.component.spec.ts looks resolved by the escapeRegExp helper — no .replace('*', …) remains anywhere in nvme-of/. Safe to dismiss.
• ixFormTestingProviders() defaulting the min-feedback hold to 0 is the right call and is well documented, but it does mean the production forkJoin/timer path is now exercised by exactly two suites — worth keeping in mind if that timing ever grows logic.
• Comment density is high in places (multi-line prose inside templates and spec bodies). The "why" content is valuable; some of the "how the harness works" narration in specs will age faster than the code it describes.

I couldn't run yarn test/yarn lint locally (no node_modules in this checkout), so the above is static review only — the WebUI CI run is still queued.
· branch NAS-141478

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.69%. Comparing base (4d3bd4c) to head (06b28ec).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #13883      +/-   ##
==========================================
+ Coverage   87.66%   87.69%   +0.03%     
==========================================
  Files        1898     1900       +2     
  Lines       71978    71963      -15     
  Branches     9171     9168       -3     
==========================================
+ Hits        63096    63108      +12     
+ Misses       8882     8855      -27     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.