mcgilman commented on code in PR #11559:
URL: https://github.com/apache/nifi/pull/11559#discussion_r3830795100
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/parameter-contexts/state/parameter-context-listing/parameter-context-listing.effects.ts:
##########
@@ -338,6 +351,78 @@ export class ParameterContextListingEffects {
map((parameterContexts) =>
parameterContexts.filter((pc) => pc.id != parameterContextId))
);
editDialogReference.componentInstance.saving$ =
this.store.select(selectSaving);
+
editDialogReference.componentInstance.hasPendingPostUpdateNavigation$ =
this.store.select(
+ selectHasPendingPostUpdateNavigation
+ );
+
+ const navigateToInheritedParameter = (
Review Comment:
`navigateToInheritedParameter` and the `goTo` closure below it are fairly
large and nested inside this effect. Consider extracting them into named,
independently-testable functions for readability — not blocking, just a
suggestion.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/ui/common/parameter-context/index.ts:
##########
@@ -15,10 +15,18 @@
* limitations under the License.
*/
-import { ParameterContextEntity } from '../../../state/shared';
+import { ParameterContextEntity, PostUpdateNavigationState } from
'../../../state/shared';
export interface EditParameterContextRequest {
Review Comment:
`EditParameterContextRequest` is declared here identically to the one in
`apps/nifi/src/app/pages/parameter-contexts/state/parameter-context-listing/index.ts`.
This copy isn't used by any production code — only
`edit-parameter-context.component.spec.ts` imports it, for test-fixture typing.
Since this PR already touches both files, can we drop this duplicate and have
the spec import the type from the state module instead?
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/parameter-contexts/ui/parameter-context-listing/parameter-context-listing.component.ts:
##########
@@ -52,6 +53,7 @@ import { ParameterContextTable } from
'./parameter-context-table/parameter-conte
})
export class ParameterContextListing implements OnInit {
private store = inject<Store<ParameterContextListingState>>(Store);
+ private router = inject(Router);
Review Comment:
This introduces new `Router`-based logic to read `highlightedParameterName`
off navigation state (see the `lastSuccessfulNavigation()` call below), but
`parameter-context-listing.component.spec.ts` wasn't updated — there's no test
asserting this reads the state and forwards it into
`getEffectiveParameterContextAndOpenDialog` correctly. Could we add coverage
for this?
Separate question (non-blocking): `lastSuccessfulNavigation()` is read after
an async pipeline (`switchMap`/`filter`/`take(1)` gated on the parameter
context appearing in the store). If that resolution is delayed and another
navigation completes first, could the wrong `highlightedParameterName` get
picked up? Worth a second look, even if the impact is only a wrong/missing
highlight.
##########
nifi-frontend/src/main/frontend/apps/nifi/src/app/pages/parameter-contexts/ui/parameter-context-listing/parameter-table/parameter-table.component.ts:
##########
@@ -74,11 +82,15 @@ export class ParameterTable implements AfterViewInit,
ControlValueAccessor {
private store = inject<Store<ParameterContextListingState>>(Store);
Review Comment:
This looks like it only ports the parameter-context-editor half of "navigate
to and highlight a parameter" — I don't see corresponding changes to
`property-table.component.ts` or any parameter-reference-parsing utility that
would let a user click a `#{param}` reference inside a processor's/controller
service's property value and jump to the highlighted parameter here. Is that
intentionally out of scope for this PR, or planned separately? Would help to
note it in the PR description either way.
--
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]
