Conversation
|
I tested the functionality of this and it looks good. The pop up window shows accurate connections and the buttons redirect to the appropriate component. I would suggest using different Icons for the context menu options so that the arrows are pointing up and down, to correspond with "Upstream" and "Downstream". Maybe slanted up/down arrows? Just a thought. |
Thanks @Freedom9339. I updated the arrows, as suggested. The upstream context menu item shows an arrow pointing up and to the left, and downstream shows down and to the right. It's slightly more intuitive and looks good. |
0d9d316 to
c56d29a
Compare
|
Will review... |
rfellows
left a comment
There was a problem hiding this comment.
Thanks for restoring View Connections. The effect/filter path and unauthorized/empty handling look directionally right, but this isn’t merge-ready yet.
Verified locally on this branch (npx nx test nifi --runInBand): 6 failed / 2,825 passed, all in component-connections-dialog.component.spec.ts. Lint and a development build passed.
Must-fix before merge:
- The new dialog tests fail (empty mock store + assertions still targeting
goTo()/ a partial row shape). - Remote-port cells navigate with the port id and
ComponentType.RemoteProcessGroup. - Source/Destination Process Group cells navigate to
/process-groups/{groupId}/ProcessGroup/{groupId}when the endpoint is the group currently on the canvas.
The five-column clickable table is a reasonable UX, but it should follow the existing dialog listing-table pattern (bounded scroll, sticky header, striped rows) used by Local Changes and Change Version, and use <a> for in-cell navigation rather than mat-button.
f2021e7 to
5399070
Compare
@rfellows Thanks for taking the time to review and catch a few things that needed attention. I'm made numerous updates.
|
…am connections. View connections > upstream/downstream is available in the context menu from a variety of components: input/output ports, processors, process groups, remote process groups and funnels.
…in the parent group to support port-to-port connection in sibling PGs Update table formatting with fixed header and scrollable
…to match current state of upstream/downstream dialog functionality; original was from a older implementation
…ature of remote port components
a0f0727 to
7670301
Compare
rfellows
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. Tests, current-group non-navigation, the fetched name map, removal of .html.orig, column SCSS, <a>/<span> cells, and the sticky/scroll table look good. npx nx test nifi --runInBand is green (2872).
Still blocking:
- Source/Destination Process Group cells always call
navigateTo(..., processGroupType), so an RPG endpoint becomes/process-groups/{group}/ProcessGroup/{rpgId}. nx run nifi:lintfails (trailing comma inflow.effects.ts, unused import inflow.selectors.ts).
Also please see the notes on reusing component-context for the header, empty-canvas View Connections, table sorting, and whitespace inside the links.
| <div class="tertiary-color font-medium">Selected Component<br> | ||
| <i class="icon component-type-icon" [class]="componentIcon(componentType)"></i> | ||
| {{ componentName }} | ||
| </div> |
There was a problem hiding this comment.
The “Selected Component” header (icon + name) is a good place to reuse the shared component-context widget instead of a one-off icon/name block.
That widget already maps ComponentType to the canvas icon (including icon-group-remote), shows the type label, and offers a copyable id. Cluster Summary uses it the same way at the top of a dialog:
<component-context
[type]="componentType"
[name]="componentName"
[id]="componentId"></component-context>(cluster-summary-dialog.component.html; also the Operation panel.) Import ComponentContext from @nifi/shared and add it to this component’s imports.
ViewComponentConnectionsRequest already has id / name / type. Those just need to be forwarded on ComponentConnectionsDialogRequest so the header can bind [id] as well. The name fallback you already have in requestComponentConnections (readable name, else the component id) is the right [name] — component-context does not itself handle canRead.
| condition: (selection: d3.Selection<any, any, any, any>) => { | ||
| return this.canvasUtils.hasUpstream(selection); | ||
| }, |
There was a problem hiding this comment.
View Connections never appears on empty-canvas right-click. hasUpstream / hasDownstream require selection.size() === 1, so a click on the canvas (no component selected) yields an empty submenu and the parent item is hidden (context-menu.component.ts keeps a submenu only when it has visible children).
Empty canvas is the current process group — the Operation panel already treats selection.size() === 0 that way (getContextType → ProcessGroup, name from breadcrumbs). Several canvas actions do the same, for example Enable/Disable All Controller Services:
condition: (selection) => {
return this.canvasUtils.isProcessGroup(selection) || this.canvasUtils.emptySelection(selection);
}If the intent is to restore 1.x “view connections for the group I’m in,” allow empty selection here and, when selection.empty(), use the current process group id (canvasUtils.getProcessGroupId()) instead of selection.datum(). That matches how those other current-PG actions are wired.
| <a | ||
| class="component-connection-cell neutral-contrast" | ||
| [matTooltip]="resolveGroupName(row.source.groupId)" | ||
| (click)="navigateTo(row.source.groupId, dialogRequestGroupId, processGroupType)"> | ||
| <i class="icon component-type-icon flex-none" [class]="componentIcon(processGroupType)"></i> | ||
| {{ resolveGroupName(row.source.groupId) }} | ||
| </a> |
There was a problem hiding this comment.
Remote ports are correctly non-clickable now, but the Source/Destination Process Group cells still always call navigateTo(..., processGroupType):
(click)="navigateTo(row.source.groupId, dialogRequestGroupId, processGroupType)"For REMOTE_INPUT_PORT / REMOTE_OUTPUT_PORT, groupId is the remote process group id, so this still routes to /process-groups/{current}/ProcessGroup/{rpgId} instead of RemoteProcessGroup/{rpgId} — the same class of bug as navigating the port itself. The icon is also icon-group rather than icon-group-remote.
Go To Source already uses ComponentType.RemoteProcessGroup when the connectable is remote (canvas-context-menu.service.ts). remoteProcessGroupIds is already on the dialog request and never read; wire that (or endpoint.type === RemoteProcessGroup) for the group-cell type and icon, and add a click test for that cell. The destination process-group link (lines 103–109) has the same issue.
| } @else { | ||
| <div class="listing-table component-connections-table flex-1 relative"> | ||
| <div class="absolute inset-0 overflow-y-auto overflow-x-hidden"> | ||
| <table mat-table [dataSource]="rows"> |
There was a problem hiding this comment.
This listing table is not sortable. Other dialog tables (Change Version, Local Changes) use Angular Material sort on the mat-table. A compact version of that pattern:
<table
mat-table
[dataSource]="dataSource"
matSort
matSortDisableClear
(matSortChange)="sortData($event)"
[matSortActive]="initialSortColumn"
[matSortDirection]="initialSortDirection">
...
<th mat-header-cell *matHeaderCellDef mat-sort-header>Connection</th>See change-version-dialog.html (73–80, 99–100) and local-changes-table.html / local-changes-table.ts (sortData + nifiCommon.compareString).
Practical defaults for this dialog:
- Use
MatTableDataSource(or keep the array and re-assign on sort). - Import
MatSortModule. - Default
[matSortActive]="'connection'"and[matSortDirection]="'asc'". - Sort each column by the visible label (
resolveGroupName(...), endpoint name, connection name), withnifiCommon.compareString, so unauthorized / unnamed rows still have a stable key.
| <a | ||
| class="component-connection-cell neutral-contrast" | ||
| [matTooltip]="row.source.name" | ||
| (click)="navigateTo(row.source.id, row.source.groupId, row.source.type)"> | ||
| <i class="icon component-type-icon" [class]="componentIcon(row.source.type)"></i> | ||
| {{ row.source.name }} | ||
| </a> |
There was a problem hiding this comment.
The underline on these links starts a character before the icon/text. That comes from whitespace in the template becoming text nodes inside the <a>. For example:
<a
class="component-connection-cell neutral-contrast"
...>
<i class="icon component-type-icon" [class]="componentIcon(row.source.type)"></i>
{{ row.source.name }}
</a>The newline/indent between > / <i> / {{ ... }} is rendered as spaces, so the underline is longer than the visible content. Icon-to-label spacing is already handled in SCSS (.component-type-icon { margin-right: 0.25rem; }), so the extra DOM spaces are not needed.
Please keep the opening tag, icon, interpolation, and closing tag flush (all five link/span cells):
<a
class="component-connection-cell neutral-contrast"
[matTooltip]="row.source.name"
(click)="navigateTo(row.source.id, row.source.groupId, row.source.type)"
><i class="icon component-type-icon" [class]="componentIcon(row.source.type)"></i>{{ row.source.name }}</a>| direction: request.direction, | ||
| connections: flowEntity.processGroupFlow.flow.connections.filter(attachedTo), | ||
| groupIdToName: this.buildProcessGroupIdToNameMap(flowEntity), | ||
| remoteProcessGroupIds: this.buildRemoteProcessGroupIdSet(flowEntity), |
There was a problem hiding this comment.
npx nx run nifi:lint fails on this change set:
- Here — prettier wants the trailing comma after
remoteProcessGroupIds: this.buildRemoteProcessGroupIdSet(flowEntity)removed. flow.selectors.ts:22— unusedBreadcrumbEntityimport (@typescript-eslint/no-unused-vars), leftover from moving the name map onto the dialog request.
Please fix both so CI lint stays green.
| import { createSelector } from '@ngrx/store'; | ||
| import { CanvasState, selectCanvasState } from '../index'; | ||
| import { ComponentType, selectCurrentRoute } from '@nifi/shared'; | ||
| import { BreadcrumbEntity } from '../../../../state/shared'; |
There was a problem hiding this comment.
Unused BreadcrumbEntity import (@typescript-eslint/no-unused-vars), leftover from moving the name map onto the dialog request. Please drop it (same lint pass as the trailing comma in flow.effects.ts).
|
|
||
| <h2 mat-dialog-title>{{ title }}</h2> | ||
| <mat-dialog-content> | ||
| <!-- <div class="flex flex-col gap-y-4">--> |
There was a problem hiding this comment.
A couple of leftovers from the iteration:
- This commented-out wrapper:
<!-- <div class="flex flex-col gap-y-4">-->. Please delete it rather than leaving it in the shipped template. component-connections-dialog.component.spec.ts674–697 and 699–722 are the same test (navigates to the connection in the group that defines the dialog request). Please keep one.
| it('navigates to the connection in the group that defines the dialog request', () => { | ||
| const connection = readableConnection({ | ||
| id: 'connection-to-navigate-to', | ||
| name: 'Connection To Navigate To' | ||
| }); | ||
|
|
||
| const { fixture, store, dialogRef } = createDialog('upstream', [connection]); | ||
| const dispatch = vi.spyOn(store, 'dispatch'); | ||
|
|
||
| const connectionCell = getCells(fixture, 'mat-column-connection')[0]; | ||
| const link = connectionCell.querySelector('a') as HTMLAnchorElement; | ||
| link.click(); | ||
|
|
||
| expect(dispatch).toHaveBeenCalledWith( | ||
| navigateToComponent({ | ||
| request: { | ||
| id: 'connection-to-navigate-to', | ||
| processGroupId: REQUEST_GROUP_ID, | ||
| type: ComponentType.Connection | ||
| } | ||
| }) | ||
| ); | ||
| expect(dialogRef.close).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
This test is a duplicate of the one immediately above (same name and assertions). Please keep one.
…am connections. View connections > upstream/downstream is available in the context menu from a variety of components: input/output ports, processors, process groups, remote process groups and funnels.
Summary
In NiFi 1.x, there was a context menu option for View Connections > upstream/downstream. This is available from any component, but it is particularly useful for input/output ports. The reason is that the connections to ports are not visible on the graph at the same level as the ports themselves. The user must go to the parent process group, and then there is ambiguity which connections are connected to which ports in cases where there are multiple connections and ports on the process group.
A key feature of the View Connections table is that each item in the table is clickable and will navigate to the selected component or connection.
Restoring this feature makes it far easier to navigate the graph and identify complex routing/connectivity issues in the flow.
NIFI-14777
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000VerifiedstatusPull Request Formatting
mainbranchVerification
Verification was performed by interacting with the graph and testing a variety of scenarios including:
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation