NIFI-15855 - Connector wizard - secret and boolean property handling#11248
Conversation
|
Reviewing... |
mcgilman
left a comment
mcgilman
left a comment
There was a problem hiding this comment.
Six findings on the new SECRET and BOOLEAN handling in connector-property-input — one accessibility regression (#1) plus a handful of correctness / clarity items in the SECRET path. Details inline.
Tests, lint, and prettier all came back clean on my end (50/50 tests pass); the structural changes look right and these are mostly tightening passes. The numbered prefixes (#1 … #8) are just to make the comments easy to refer to in follow-up discussion.
| <div class="flex flex-col"> | ||
| <div class="flex gap-x-2 items-center"> | ||
| <span class="text-sm">{{ prop.name }}</span> | ||
| <mat-slide-toggle [formControl]="formControl" data-qa="property-input-boolean"></mat-slide-toggle> | ||
| </div> | ||
| @if (prop.description) { | ||
| <span class="text-xs tertiary-color block">{{ prop.description }}</span> | ||
| } | ||
| </mat-checkbox> | ||
| </div> |
There was a problem hiding this comment.
#1 — Accessibility regression: the new <mat-slide-toggle> is unlabeled.
The previous <mat-checkbox> wrapped the property name as projected content, which Material associates with the inner control via aria-labelledby. The new <mat-slide-toggle> is empty — the property name lives in a sibling <span>, with no aria-label, aria-labelledby, or projected content. A screen reader will announce this toggle as unlabeled. Other slide-toggle usages in this codebase (e.g. bulletin-board.component.html) project the label inside the toggle, which is the established convention.
Could we project the label inside the toggle? That restores the screen-reader association the previous checkbox provided, and as a bonus the whole label becomes a click target:
<div class="flex flex-col">
<mat-slide-toggle
[formControl]="formControl"
labelPosition="before"
data-qa="property-input-boolean">
<span class="text-sm">{{ prop.name }}</span>
</mat-slide-toggle>
@if (prop.description) {
<span class="text-xs tertiary-color block">{{ prop.description }}</span>
}
</div>| /** | ||
| * Whether the property should be rendered as a slide toggle (BOOLEAN). | ||
| */ | ||
| shouldUseToggle(): boolean { | ||
| return this.property()?.type === 'BOOLEAN'; | ||
| } |
There was a problem hiding this comment.
#2 — shouldUseToggle() appears to be dead code.
I can't find a caller for this method — the template uses @switch (prop.type) { @case ('BOOLEAN') } and no test references it. Could we either remove it, or switch the template over to use it for symmetry with the other shouldUseX() predicates in the file? Whichever is preferred, having both an unused predicate and a switch-case is a maintenance trap.
| if (currentValue !== expected) { | ||
| // Provider name changed since saving - update form value to match current secret. | ||
| // Use afterNextRender to safely modify form value after change detection completes. | ||
| runInInjectionContext(this.injector, () => { | ||
| afterNextRender(() => this.formControl.setValue(expected, { emitEvent: true })); | ||
| }); | ||
| } |
There was a problem hiding this comment.
#3 — Form-value mutation as a side effect inside computeSecretSelectOptions().
A method named computeSecretSelectOptions mutating the form control is surprising and makes the function harder to reason about. There's also a subtle risk: every effect re-run that observes currentValue !== expected schedules another afterNextRender callback. If the effect re-runs more than once before the first callback flushes (e.g. parent re-emits availableSecrets twice quickly), multiple identical setValue calls will fire one after another, each with emitEvent: true.
Could we lift this auto-fix into its own effect() (or a small reconcileFormValueWithSecrets() method) so option-compute stays pure, and guard with a single-shot flag that resets on property-name change? Something like:
private autoFixScheduled = false;
// In the constructor, alongside the existing effect:
effect(() => {
const secrets = this.availableSecrets();
if (this.property()?.type !== 'SECRET' || !secrets) return;
if (this.autoFixScheduled) return;
// ...derive expected, compare to formControl.value, schedule once...
});| this.availableSecrets(); | ||
| this.secretsLoading(); | ||
| this.secretsError(); |
There was a problem hiding this comment.
#4 — selectOptions does not recompute when formControl.value changes.
The effect reads property, dynamicAllowableValuesState, availableSecrets, secretsLoading, and secretsError, but formControl.value isn't a signal and isn't tracked here. computeSecretSelectOptions reads formControl.value to decide whether to append a disabled orphan entry.
Consequence: if the user opens the dropdown with an orphan saved value and then selects a real secret, the disabled "<fqn> (no longer available)" entry lingers in selectOptions until the parent next re-emits one of the watched signals. The displayed selection is correct (searchable-select renders it from the value), but the stale orphan in the list is confusing.
Could we subscribe to formControl.valueChanges (with takeUntilDestroyed(this.destroyRef)) and recompute selectOptions on each emission? Or mirror the value into a signal at write time so the existing effect picks it up.
| } else if (currentValue) { | ||
| const parsed = parseSecretKey(currentValue); | ||
| options.push({ | ||
| value: currentValue, | ||
| label: `${parsed.fullyQualifiedName} (no longer available)`, | ||
| disabled: true, | ||
| group: parsed.providerName | ||
| }); |
There was a problem hiding this comment.
#5 — Pre-load orphan label is misleading.
While availableSecrets() === null and secretsLoading() === true, the saved value is rendered as "<fqn> (no longer available)" on a disabled select. The disable state stops the user from acting on it, but the wording is alarming during what is just a transient load — and if the load eventually returns the secret, the user briefly saw it labeled as missing for no reason.
Could we only append the (no longer available) suffix once the load has actually completed (secretsLoading() === false && secretsError() === null)? While loading, render the FQN without the suffix. The pre-load orphan test would need an updated expectation.
| * Compute SearchableSelectOptions from whichever source is available. | ||
| * | ||
| * - SECRET: options come from availableSecrets, valued by a composite key | ||
| * (providerId::providerName::fullyQualifiedName) and grouped by provider | ||
| * (or "Provider - Group" when a provider owns multiple groups). Saved | ||
| * values that no longer match an available secret are surfaced as disabled | ||
| * "(no longer available)" options. When the saved value still matches but | ||
| * the provider name has changed, the form value is rewritten to the new | ||
| * composite key after the next render. | ||
| * - Otherwise: dynamic values when successfully fetched, falling back to | ||
| * the descriptor's static allowableValues. |
There was a problem hiding this comment.
#8 — The expanded doc comment could also note the side-effect channel and the untracked formControl.value dependency.
The rewrite is a real improvement — much clearer than the previous two-liner. While it now explicitly mentions the form-value rewrite, I think it's worth also calling out:
- that this is a side effect scheduled from a method named "compute" (i.e. callers shouldn't assume option-compute is pure — see related comment on lines 539–545), and
- that
formControl.valueis read here but is not tracked by theeffect()that triggers recompute (see related comment on lines 133–135), so options can briefly fall out of sync with the form value until one of the tracked signals re-emits.
Both are non-obvious from the method body and tend to trip up future readers.
2 participants
Summary
NIFI-15855
Completes the connector wizard property-input behavior for
SECRETandBOOLEANproperty descriptors inconnector-property-input. TheavailableSecrets/secretsLoading/secretsErrorinputs were declared but unused in the previous wizard PR; this change wires them through and switchesBOOLEANrendering to a slide-toggle to match the rest of the wizard's input controls.Changes
SECRETSECRETproperties via<searchable-select>instead of a plain text input.providerId::providerName::fullyQualifiedName, built bybuildSecretKey(). The parentconnector-configuration-stepalready unpacks this on save viaparseSecretKey()."Provider - Group"when a provider owns multiple groups."…fqn… (no longer available)"option grouped under the saved provider name. A pre-load placeholder is shown while secrets are still loading.providerId + fullyQualifiedNamebut theproviderNamehas changed, the form value is rewritten to the new composite key after the next render (afterNextRender+runInInjectionContext).secretsErrorflips the<searchable-select>into itsloadErrorstate with the message"Failed to load secrets"and suppresses the description hint."Loading secrets..."whilesecretsLoading()is true; placeholder text follows the fourSECRETstates (Loading secrets...,Failed to load secrets,No secrets available,Select a secret).BOOLEAN<mat-checkbox>with<mat-slide-toggle>: label on the left, toggle on the right, description below.writeValuealready coerces'true'/true/ other to a boolean, so no API change for parent consumers.Reviewer notes
BOOLEANrendering change. This is the first use of<mat-slide-toggle>for a property descriptor innifi-frontend; the existing slide-toggles in the codebase (e.g. bulletin-board, update-attribute rule listing) are ad-hoc UI mode switches. The toggle uses Material's default dash/check thumb icon for visual consistency with those.SECRETform value contract. The form value is the composite key, notsecret.id. The existing parent already depends on this; any future consumer reading the value directly must useparseSecretKey()to unpack it.availableSecrets/secretsLoading/secretsErrorare already populated upstream and passed in viaconnector-configuration-step.Testing
connector-property-input.component.spec.tsextended from 37 to 50 tests.SECRETtests: option shape, single-group vs multi-group label format, post-load orphan, pre-load orphan, provider-rename auto-fix,loadErrorwiring (including empty-string short-circuit), inline spinner show/hide, placeholder state transitions.BOOLEANtests: slide-toggle rendering, parent → child form sync, toggle → parent round-trip via native input click,writeValue('true')truthy coercion,writeValue('false')falsy coercion.nx lint sharedand prettier are clean.