NIFI-15818: Implementing route-backed component selection so selecting a processor/connection/etc on the canvas updates the URL, and vice versa.#11138
Conversation
…g a processor/connection/etc on the canvas updates the URL, and vice versa.
rfellows
left a comment
There was a problem hiding this comment.
Just a few minor things...
| it('should not navigate when parent process group id is null', async () => { | ||
| const { effects, actions$, mockRouter } = await setup({ | ||
| connectorId: 'conn-x', | ||
| parentProcessGroupId: null, | ||
| processGroupId: 'child-pg' | ||
| }); | ||
| actions$(of(leaveProcessGroup())); | ||
|
|
||
| await firstValueFrom(effects.leaveProcessGroup$.pipe(() => of(undefined))); | ||
| expect(mockRouter.navigate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should not navigate when current process group id is null', async () => { | ||
| const { effects, actions$, mockRouter } = await setup({ | ||
| connectorId: 'conn-x', | ||
| parentProcessGroupId: 'parent-pg', | ||
| processGroupId: null | ||
| }); | ||
| actions$(of(leaveProcessGroup())); | ||
|
|
||
| await firstValueFrom(effects.leaveProcessGroup$.pipe(() => of(undefined))); | ||
| expect(mockRouter.navigate).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
In RxJS, .pipe() expects operator functions -- functions that take a source observable and return a new observable using RxJS operators like map, filter, switchMap, etc. But () => of(undefined) is a plain function that ignores its source argument entirely and just returns of(undefined).
So what actually happens:
effects.leaveProcessGroup$is created (the effect pipeline).pipe(() => of(undefined))wraps it, but the wrapper throws away the source and returnsof(undefined)firstValueFromsubscribes and immediately getsundefinedfromof(undefined)- The test then asserts
mockRouter.navigatewas not called -- which passes, but only because the effect was never actually driven
The filter at line 202-204 of the effects file is the logic being "tested" here:
filter(
([, , parentProcessGroupId, currentProcessGroupId]) =>
parentProcessGroupId != null && currentProcessGroupId != null
),These tests are supposed to prove that when parentProcessGroupId or processGroupId is null, the filter blocks navigation. But the tests pass regardless of whether that filter exists.
Recommendation:
The tricky part is that when the filter blocks, leaveProcessGroup$ simply never emits. Testing "nothing happened" is inherently awkward. Two reasonable approaches:
Option A -- Subscribe, flush, then assert (simplest):
it('should not navigate when parent process group id is null', async () => {
const { effects, actions$, mockRouter } = await setup({
connectorId: 'conn-x',
parentProcessGroupId: null,
processGroupId: 'child-pg'
});
actions$(of(leaveProcessGroup()));
// Subscribe to drive the pipeline, then check nothing happened
const results: any[] = [];
effects.leaveProcessGroup$.subscribe((action) => results.push(action));
expect(results).toEqual([]);
expect(mockRouter.navigate).not.toHaveBeenCalled();
});Option B -- Use firstValueFrom with a timeout that rejects (more explicit):
it('should not navigate when parent process group id is null', async () => {
const { effects, actions$, mockRouter } = await setup({
connectorId: 'conn-x',
parentProcessGroupId: null,
processGroupId: 'child-pg'
});
actions$(of(leaveProcessGroup()));
await expect(
firstValueFrom(effects.leaveProcessGroup$.pipe(timeout(50)))
).rejects.toThrow();
expect(mockRouter.navigate).not.toHaveBeenCalled();
});Option A is simpler because the source observable (of(leaveProcessGroup())) is synchronous, so the effect pipeline runs synchronously through the filter. By the time the next line executes, the filter has already blocked (or not). No async timing issues.
| import * as ConnectorCanvasActions from '../../state/connector-canvas/connector-canvas.actions'; | ||
| import * as ConnectorCanvasSelectors from '../../state/connector-canvas/connector-canvas.selectors'; | ||
| import * as ConnectorCanvasEntityActions from '../../state/connector-canvas-entity/connector-canvas-entity.actions'; | ||
| import { selectRouteParams } from '../../state/connector-canvas/connector-canvas.selectors'; |
There was a problem hiding this comment.
This is already imported and available through ConnectorCanvasSelectors on line 34. However, looking at it a bit more closely... it looks like it can probably be imported from @nifi/shared. The import and re-export in the canvas selectors isn't needed.
| import { ConnectorCanvasState, connectorCanvasFeatureKey } from './index'; | ||
|
|
||
| const { selectRouteParams } = getRouterSelectors(); | ||
| export const { selectRouteParams } = getRouterSelectors(); |
There was a problem hiding this comment.
I don't think we need export this here. In fact, it can be imported from @nifi/shared and not needed from router-store.
Title: NIFI-15818: Route-backed component selection for the connector canvas
Body:
Summary
selectComponents,deselectAllComponents, andnavigateWithoutTransformactions with corresponding effects, reducer handlers, and component wiringselectComponentswith an empty components array would navigate to an invalid route; it now correctly delegates todeselectAllComponentsDetails
Selection routing: Clicking a single component routes to
/connectors/:id/canvas/:processGroupId/:type/:componentId. Selecting multiple components routes to.../bulk/:id1,:id2,.... Clicking the canvas background deselects all and returns to the base canvas route. All selection-initiated navigations usereplaceUrl: trueto avoid polluting browser history.skipTransformmechanism: When the user selects a component on the canvas,navigateWithoutTransformsetsskipTransform: truein the reducer so the resulting route change does not auto-center the viewport. When a deep link is opened directly,skipTransformis false, soonCanvasInitializedcallscenterOnSelection()to center the viewport on the selected component(s).leaveProcessGroup$update: Refactored to use thenavigateWithoutTransformaction (instead of directrouter.navigate+setSkipTransform) after navigating to the parent process group and waiting for the flow to load. This auto-selects the child process group in the parent view, giving the user a visual reference of where they came from.Test cleanup: Converted all effects tests from the
new Promise<void>((resolve) => { setup().then(...) })anti-pattern toasync/awaitwithfirstValueFromfrom RxJS.