Copilot commented on code in PR #4271:
URL: https://github.com/apache/streampipes/pull/4271#discussion_r2966258337
##########
ui/src/app/editor/components/pipeline/pipeline.component.ts:
##########
@@ -158,6 +162,16 @@ export class PipelineComponent implements OnInit,
OnDestroy {
this.readonly,
);
if (!this.readonly) {
+ this.shortcutReg = this.shortcutService.register('pipeline', [
+ {
+ key: 'Delete',
+ action: () => this.deleteSelectedElement(),
+ },
+ {
+ key: 'Backspace',
+ action: () => this.deleteSelectedElement(),
+ },
+ ]);
Review Comment:
The shortcut registration id is a constant string ('pipeline'). Since
`KeyboardShortcutService.register` stores actions in a `Map<string, ...>`,
multiple instances of this component (or re-entries without proper teardown)
would overwrite each other’s registrations. Use an instance-unique id (e.g.,
include a generated UUID, route param, or incrementing counter) to avoid
collisions.
##########
ui/src/app/chart-shared/components/charts/table/table-widget.component.ts:
##########
@@ -510,6 +510,32 @@ export class TableWidgetComponent extends
BaseDataExplorerWidgetDirective<TableW
onFilterDropdownClick = (event: MouseEvent): void =>
event.stopPropagation();
+ @HostListener('document:keydown', ['$event'])
+ handleGlobalKeydown(event: KeyboardEvent): void {
+ if (!this.openFilterColumn) {
+ return;
+ }
+
+ const key = event.key.toLowerCase();
+ const ctrl = event.ctrlKey || event.metaKey;
+
+ if (key === 'escape') {
+ this.closeFilter();
+ event.preventDefault();
+ event.stopPropagation();
+ } else if (ctrl && key === 'f') {
+ const input = this.elRef.nativeElement.querySelector(
+ '.column-filter-search',
+ );
+ if (input) {
+ input.focus();
+ input.select();
+ event.preventDefault();
+ event.stopPropagation();
+ }
Review Comment:
`querySelector` returns `Element | null`, so `input.focus()` /
`input.select()` is not type-safe and can fail compilation under typical TS DOM
typings (since `Element` doesn’t guarantee `select()`). Use a typed query
(e.g., `querySelector<HTMLInputElement>(...)`) or cast to `HTMLInputElement`
before calling `focus/select`.
##########
ui/projects/streampipes/shared-ui/src/lib/dialog/base-dialog/base-dialog.service.ts:
##########
@@ -32,11 +32,15 @@ import { PanelDialogConfig } from
'../panel-dialog/panel-dialog.config';
import { StandardDialogConfig } from
'../standard-dialog/standard-dialog.config';
import { CardDialogComponent } from '../card-dialog/card-dialog.component';
import { CardDialogConfig } from '../card-dialog/card-dialog-config';
+import { MatDialog } from '@angular/material/dialog';
@Injectable({
providedIn: 'root',
})
export class DialogService {
+ private openDialogs: DialogRef<any>[] = [];
+ private matDialog = inject(MatDialog);
Review Comment:
This introduces a hard dependency on `MatDialog` inside a shared-ui service
that previously relied on CDK Overlay only. If any consumer uses
`DialogService` without providing Angular Material dialog providers, DI will
fail at runtime. Consider making `MatDialog` injection optional (and guarding
`this.matDialog?.openDialogs`) to keep the service usable in non-Material
contexts.
##########
ui/projects/streampipes/shared-ui/src/lib/services/keyboard-shortcut.service.ts:
##########
@@ -0,0 +1,105 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+import { Injectable, NgZone, OnDestroy, inject } from '@angular/core';
+import { Subject } from 'rxjs';
+import { DialogService } from '../dialog/base-dialog/base-dialog.service';
+
+export interface ShortcutAction {
+ key: string;
+ ctrl?: boolean;
+ shift?: boolean;
+ action: (event: KeyboardEvent) => void;
+ preventDefault?: boolean;
+ allowInDialog?: boolean;
+}
+
+export type ShortcutRegistration = { unregister: () => void };
+
+@Injectable({ providedIn: 'root' })
+export class KeyboardShortcutService implements OnDestroy {
+ private keydown$ = new Subject<KeyboardEvent>();
+ private registrations: Map<string, ShortcutAction[]> = new Map();
+ private listener = (e: KeyboardEvent) => {
+ const isInput = this.isInputFocused(e);
+ const ctrl = e.ctrlKey || e.metaKey;
+
+ if (!isInput || ctrl || e.key === 'Escape') {
+ this.keydown$.next(e);
+ }
+ };
+
+ private dialogService = inject(DialogService);
+
+ constructor(private ngZone: NgZone) {
+ this.ngZone.runOutsideAngular(() =>
+ document.addEventListener('keydown', this.listener, true),
+ );
+ this.keydown$.subscribe(e =>
+ this.ngZone.run(() => this.handleEvent(e)),
+ );
+ }
+
+ ngOnDestroy(): void {
+ document.removeEventListener('keydown', this.listener, true);
+ this.keydown$.complete();
+ }
+
+ register(id: string, actions: ShortcutAction[]): ShortcutRegistration {
+ this.registrations.set(id, actions);
+ return { unregister: () => this.registrations.delete(id) };
+ }
+
+ unregister(id: string): void {
+ this.registrations.delete(id);
+ }
+
+ private handleEvent(event: KeyboardEvent): void {
+ const key = event.key.toLowerCase();
+ const ctrl = event.ctrlKey || event.metaKey;
+ const shift = event.shiftKey;
+
+ const match = Array.from(this.registrations.values())
+ .flat()
+ .find(
+ a =>
+ a.key.toLowerCase() === key &&
+ !!a.ctrl === ctrl &&
+ !!a.shift === shift,
+ );
+
+ if (match) {
+ if (match.preventDefault !== false) {
+ event.preventDefault();
+ event.stopPropagation();
+ }
+
+ if (!this.dialogService.hasOpenDialogs || match.allowInDialog) {
+ match.action(event);
+ }
+ }
+ }
Review Comment:
Shortcut resolution uses `.find(...)` over a flattened list of all
registrations, which means the first registered matching shortcut wins. This
can cause the wrong handler to fire when multiple components register the same
shortcut (e.g., nested/embedded views or overlays). Consider resolving in
reverse registration order (most recently registered wins), or introducing
explicit priority/scoping so the most local context can override global
handlers.
##########
ui/projects/streampipes/shared-ui/src/lib/dialog/base-dialog/base-dialog.service.ts:
##########
@@ -75,16 +79,32 @@ export class DialogService {
dialogRef.componentInstance =
panelDialogContainerRef.instance.attach();
if (config.data) {
- Object.keys(config.data).forEach(key => {
- dialogRef.componentInstance[key] = config.data[key];
- });
+ Object.keys(config.data).forEach(
+ key => (dialogRef.componentInstance[key] = config.data[key]),
+ );
}
this.applyDialogProperties(panelDialogContainerRef, overlay, config);
+ this.openDialogs.push(dialogRef);
+ dialogRef
+ .afterClosed()
+ .subscribe(
+ () =>
+ (this.openDialogs = this.openDialogs.filter(
+ d => d !== dialogRef,
+ )),
+ );
+
return dialogRef;
}
+ get hasOpenDialogs() {
+ return (
+ this.openDialogs.length > 0 || this.matDialog.openDialogs.length > 0
+ );
+ }
Review Comment:
This introduces a hard dependency on `MatDialog` inside a shared-ui service
that previously relied on CDK Overlay only. If any consumer uses
`DialogService` without providing Angular Material dialog providers, DI will
fail at runtime. Consider making `MatDialog` injection optional (and guarding
`this.matDialog?.openDialogs`) to keep the service usable in non-Material
contexts.
##########
ui/src/app/pipeline-details/components/pipeline-details-toolbar/pipeline-details-toolbar.component.html:
##########
@@ -66,5 +66,35 @@
color="accent"
>{{ 'Auto refresh' | translate }}
</mat-slide-toggle>
+ <button
+ mat-icon-button
+ [matMenuTriggerFor]="optMenu"
+ [attr.aria-label]="'Options' | translate"
+ data-cy="pipeline-details-options"
+ >
+ <mat-icon>more_vert</mat-icon>
+ </button>
+ <mat-menu #optMenu="matMenu">
+ @if (hasPipelineWritePrivileges) {
+ <button
+ mat-menu-item
+ (click)="editPipelineEmitter.emit()"
+ data-cy="options-edit-pipeline"
+ >
+ <mat-icon>edit</mat-icon>
+ <span>{{ 'Edit pipeline' | translate }}</span>
+ </button>
+ }
+ @if (hasPipelineWritePrivileges) {
+ <button
+ mat-menu-item
+ (click)="deletePipelineEmitter.emit()"
+ data-cy="options-delete-pipeline"
+ >
+ <mat-icon>clear</mat-icon>
+ <span>{{ 'Delete pipeline' | translate }}</span>
+ </button>
+ }
+ </mat-menu>
Review Comment:
The options (3-dot) menu button is always visible, but all menu items are
gated behind `hasPipelineWritePrivileges`. This can produce an empty menu for
read-only users. Consider hiding/disabled the trigger when there are no
available actions (e.g., wrap the trigger button in the same `@if`, or disable
the button when `!hasPipelineWritePrivileges`).
##########
ui/src/app/chart/components/chart-view/chart-view.component.ts:
##########
@@ -145,6 +149,15 @@ export class ChartViewComponent
@ViewChild('panel', { static: false }) outerPanel: ElementRef;
ngOnInit() {
+ this.shortcutReg = this.shortcutService.register('chart-view', [
+ {
+ key: 's',
+ ctrl: true,
+ action: () => this.onShortcutSave(),
+ allowInDialog: true,
+ },
+ ]);
+
Review Comment:
New shortcut behavior (Ctrl/Cmd+S saving in edit mode) is introduced here
but doesn’t appear to be covered by automated tests. Since there is existing
Cypress coverage in this area (e.g., table filter dropdown keyboard behavior),
consider adding Cypress coverage to verify Ctrl/Cmd+S triggers `saveDataView()`
only when `editMode` is active (and does not trigger when not in edit mode).
##########
ui/src/app/editor/components/pipeline-assembly/pipeline-assembly.component.ts:
##########
@@ -92,14 +101,28 @@ export class PipelineAssemblyComponent implements
AfterViewInit {
private router: Router,
private jsplumbService: JsplumbService,
private translateService: TranslateService,
+ private shortcutService: KeyboardShortcutService,
) {}
ngAfterViewInit() {
+ this.shortcutReg = this.shortcutService.register('pipeline-assembly', [
+ { key: 's', ctrl: true, action: () => this.onShortcutSave() },
+ ]);
this.jsplumbBridge = this.jsPlumbFactoryService.getJsplumbBridge(
this.readonly,
);
}
+ ngOnDestroy(): void {
+ this.shortcutReg?.unregister();
+ }
+
+ private onShortcutSave(): void {
+ if (!this.readonly && this.rawPipelineModel?.length) {
Review Comment:
The Ctrl/Cmd+S handler only triggers when `rawPipelineModel?.length` is
truthy. This prevents saving when the pipeline is intentionally empty (or when
the model exists but has zero elements), which may still be a valid state for
opening the save flow. If the intent is 'pipeline model is present', prefer
checking `this.rawPipelineModel != null` rather than `length`.
```suggestion
if (!this.readonly && this.rawPipelineModel != null) {
```
##########
ui/src/app/editor/components/pipeline-assembly/pipeline-assembly.component.ts:
##########
@@ -137,7 +160,6 @@ export class PipelineAssemblyComponent implements
AfterViewInit {
);
const dialogRef = this.dialogService.open(SavePipelineComponent, {
panelType: PanelType.SLIDE_IN_PANEL,
- disableClose: true,
title: this.translateService.instant('Save pipeline'),
width: '40vw',
Review Comment:
`disableClose: true` was removed from the save pipeline dialog config.
Combined with the new Escape-to-close behavior in `DialogService`, users can
now dismiss the save flow via Escape/backdrop, which risks accidental loss of
work depending on what the save panel contains. If the previous behavior was
intentional, reintroduce `disableClose: true`, or add an explicit
unsaved-changes confirmation before allowing close.
```suggestion
width: '40vw',
disableClose: true,
```
--
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]
