mcgilman commented on code in PR #11248:
URL: https://github.com/apache/nifi/pull/11248#discussion_r3243920922
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.html:
##########
@@ -18,12 +18,15 @@
@if (prop) {
@switch (prop.type) {
@case ('BOOLEAN') {
- <mat-checkbox [formControl]="formControl"
data-qa="property-input-boolean">
- <span class="text-sm">{{ prop.name }}</span>
+ <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>
Review 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:
```html
<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>
```
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.ts:
##########
@@ -339,10 +364,45 @@ export class ConnectorPropertyInput implements
ControlValueAccessor, DoCheck, On
return this.property()?.type === 'STRING_LIST' &&
this.shouldUseSelect();
}
+ /**
+ * Whether the property should be rendered as a slide toggle (BOOLEAN).
+ */
+ shouldUseToggle(): boolean {
+ return this.property()?.type === 'BOOLEAN';
+ }
Review 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.
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.ts:
##########
@@ -402,15 +462,28 @@ export class ConnectorPropertyInput implements
ControlValueAccessor, DoCheck, On
}
/**
- * Compute SearchableSelectOptions from whichever source is available:
- * dynamic values when successfully fetched, otherwise static values.
+ * 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.
Review 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.value` is read here but is **not** tracked by the
`effect()` 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.
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.ts:
##########
@@ -426,6 +499,71 @@ export class ConnectorPropertyInput implements
ControlValueAccessor, DoCheck, On
}));
}
+ private computeSecretSelectOptions(): SearchableSelectOption<string>[] {
+ const options: SearchableSelectOption<string>[] = [];
+ const secrets: Secret[] | null = this.availableSecrets();
+
+ if (secrets && secrets.length > 0) {
+ const providerGroups = new Map<string, Set<string>>();
+ for (const s of secrets) {
+ if (!providerGroups.has(s.providerName)) {
+ providerGroups.set(s.providerName, new Set());
+ }
+ providerGroups.get(s.providerName)!.add(s.groupName);
+ }
+
+ for (const s of secrets) {
+ const groupCount = providerGroups.get(s.providerName)?.size ??
1;
+ const group = groupCount > 1 ? `${s.providerName} -
${s.groupName}` : s.providerName;
+ options.push({
+ value: buildSecretKey(s.providerId, s.providerName,
s.fullyQualifiedName),
+ label: s.name,
+ description: s.description,
+ group
+ });
+ }
+ }
+
+ const currentValue = this.formControl.value;
+ if (currentValue && secrets) {
+ const parsed = parseSecretKey(currentValue);
+ const matching = secrets.find(
+ (s) => s.providerId === parsed.providerId &&
s.fullyQualifiedName === parsed.fullyQualifiedName
+ );
+ if (matching) {
+ const expected = buildSecretKey(
+ matching.providerId,
+ matching.providerName,
+ matching.fullyQualifiedName
+ );
+ 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 }));
+ });
+ }
+ } else {
+ options.push({
+ value: currentValue,
+ label: `${parsed.fullyQualifiedName} (no longer
available)`,
+ disabled: true,
+ group: parsed.providerName
+ });
+ }
+ } else if (currentValue) {
+ const parsed = parseSecretKey(currentValue);
+ options.push({
+ value: currentValue,
+ label: `${parsed.fullyQualifiedName} (no longer available)`,
+ disabled: true,
+ group: parsed.providerName
+ });
Review 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.
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.ts:
##########
@@ -114,6 +130,9 @@ export class ConnectorPropertyInput implements
ControlValueAccessor, DoCheck, On
effect(() => {
const prop = this.property();
this.dynamicAllowableValuesState();
+ this.availableSecrets();
+ this.secretsLoading();
+ this.secretsError();
Review 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.
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/connector-property-input/connector-property-input.component.ts:
##########
@@ -426,6 +499,71 @@ export class ConnectorPropertyInput implements
ControlValueAccessor, DoCheck, On
}));
}
+ private computeSecretSelectOptions(): SearchableSelectOption<string>[] {
+ const options: SearchableSelectOption<string>[] = [];
+ const secrets: Secret[] | null = this.availableSecrets();
+
+ if (secrets && secrets.length > 0) {
+ const providerGroups = new Map<string, Set<string>>();
+ for (const s of secrets) {
+ if (!providerGroups.has(s.providerName)) {
+ providerGroups.set(s.providerName, new Set());
+ }
+ providerGroups.get(s.providerName)!.add(s.groupName);
+ }
+
+ for (const s of secrets) {
+ const groupCount = providerGroups.get(s.providerName)?.size ??
1;
+ const group = groupCount > 1 ? `${s.providerName} -
${s.groupName}` : s.providerName;
+ options.push({
+ value: buildSecretKey(s.providerId, s.providerName,
s.fullyQualifiedName),
+ label: s.name,
+ description: s.description,
+ group
+ });
+ }
+ }
+
+ const currentValue = this.formControl.value;
+ if (currentValue && secrets) {
+ const parsed = parseSecretKey(currentValue);
+ const matching = secrets.find(
+ (s) => s.providerId === parsed.providerId &&
s.fullyQualifiedName === parsed.fullyQualifiedName
+ );
+ if (matching) {
+ const expected = buildSecretKey(
+ matching.providerId,
+ matching.providerName,
+ matching.fullyQualifiedName
+ );
+ 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 }));
+ });
+ }
Review 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:
```ts
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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
