Add edition marker#161
Open
gkrajniak wants to merge 5 commits into
Open
Conversation
Signed-off-by: Grzegorz Krajniak <grzegorz.krajniak@sap.com>
…tent Signed-off-by: Grzegorz Krajniak <grzegorz.krajniak@sap.com>
Signed-off-by: Grzegorz Krajniak <grzegorz.krajniak@sap.com>
Signed-off-by: Grzegorz Krajniak <grzegorz.krajniak@sap.com>
Signed-off-by: Grzegorz Krajniak <grzegorz.krajniak@sap.com>
📝 WalkthroughWalkthroughThis PR introduces comprehensive unsaved-changes protection to the dashboard editor through three confirmation paths: edit-bar cancellation, in-app navigation interception, and browser unload prevention. Two new dialog components handle discard and navigation scenarios. The dashboard component now tracks mutations via snapshots and grid-change flags, exposing a public ChangesDashboard Unsaved Changes Protection
Edit Cards Dialog Keyboard Navigation
Infrastructure and Unrelated Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/webcomponents-dashboard/main.tsParsing error: /projects/webcomponents-dashboard/main.ts was not found by the project service. Consider either including it in the tsconfig.json or including it in allowDefaultProject. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts (1)
353-359:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDiscard currently restores position but not size.
saveCardsPosition()capturesw/h, but the discard path only reappliesx/y. If a card entered edit mode with grid-derived size that was not already reflected incardsSnapshot, cancelling can reopen the old layout with the wrong dimensions.Suggested fix
this.cards.set( this.cardsSnapshot.map((c) => { const pos = this.cardsPosition.get(c.id); - return { ...c, x: pos?.x, y: pos?.y }; + return { + ...c, + x: pos?.x, + y: pos?.y, + w: pos?.w ?? c.w, + h: pos?.h ?? c.h, + }; }), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts` around lines 353 - 359, The discardEdit function restores x/y from cardsPosition but not size, causing cancelled edits to keep stale dimensions; update the mapping in discardEdit (the cards.set([...].map callback) that uses this.cardsPosition.get(c.id)) to also reapply width/height from the saved position (e.g., pos?.w and pos?.h) when present (falling back to the card’s existing w/h or the value in cardsSnapshot), so sizes saved by saveCardsPosition are restored along with x/y.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts`:
- Around line 636-646: The test is using `as any` when calling
`component.onGridChange(...)` which violates the no-explicit-any lint rule;
replace those casts with `as never` (consistent with other tests) or cast to the
actual `nodesCB` type by importing it and using `as nodesCB` so the calls to
`onGridChange` (referenced as component.onGridChange and the test helper
setup/fixture) use a proper type instead of `any` in both occurrences.
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts`:
- Around line 105-110: The current hasUnsavedChanges() uses JSON.stringify on
the full objects (this.cards(), this.sections()), which breaks on non-JSON
values and cyclic graphs; instead implement a deterministic layout-only snapshot
helper (e.g., buildLayoutSnapshot(cards, sections)) that maps CardConfig objects
to only editable/layout fields (ids, positions/order, layout metadata, and any
simple primitive editable properties — do NOT include componentInputs or other
runtime graphs like functions, Dates, Maps, DOM refs) and use JSON.stringify on
that reduced structure for comparison and snapshot storage (replace uses of
this.sectionsSnapshotJson and this.cardsSnapshotJson and update snapshot
creation/restore sites referenced around hasUnsavedChanges and the other spots
called out). Ensure to reference the helper from hasUnsavedChanges, snapshot
creation, and discard/restore logic so comparisons never serialize full card
graphs or componentInputs.
In
`@projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.html`:
- Line 1: The template currently calls cancelled.emit() directly from the
ui5BeforeClose handler causing duplicate/conflicting close events; replace the
inline (ui5BeforeClose)="cancelled.emit()" with a reference to a new component
method (e.g., onBeforeClose) and implement that method in
DiscardChangesDialogComponent to only call this.cancelled.emit() when the close
was initiated by user-cancel actions (check event.escPressed or a component flag
like cancelInitiated set by your Cancel button click handler) and ignore
programmatic closes/confirm flow; ensure the Cancel button sets
cancelInitiated=true before closing and clear/reset the flag after handling so
confirm/programmatic closes do not emit cancelled.
In
`@projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.ts`:
- Around line 13-19: Update the `@Component` metadata for the DiscardChangesDialog
component (selector 'mfp-discard-changes-dialog') to include standalone: true
and changeDetection: ChangeDetectionStrategy.OnPush; import
ChangeDetectionStrategy from `@angular/core` at the top of the file. Apply the
same change to the UnsavedChangesDialog component in
projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts
so both components declare standalone: true and use
ChangeDetectionStrategy.OnPush.
In
`@projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts`:
- Around line 18-24: The component decorator for UnsavedChangesDialog currently
lists imports but is missing standalone and OnPush change detection; update the
`@Component` on UnsavedChangesDialog (unsaved-changes-dialog.component.ts) to
include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush,
and add the corresponding ChangeDetectionStrategy import from `@angular/core` so
the decorator reads both standalone and changeDetection properties while keeping
the existing imports array (Button, Dialog, Icon, Title).
In
`@projects/ngx/declarative-ui/table/declarative-table/declarative-table.component.html`:
- Line 141: The ui5-option that displays "20" currently has value="10", causing
paginationLimitChanged to emit the wrong numeric limit; update the <ui5-option
value="10">20</ui5-option> to <ui5-option value="20">20</ui5-option> (and audit
other <ui5-option> entries in declarative-table for mismatched value/display
text) so paginationLimitChanged receives the correct numeric limit.
In `@projects/webcomponents-dashboard/main.ts`:
- Around line 15-36: Remove the prototype monkey-patch on
DashboardElement.prototype and instead register a dedicated custom-element
subclass (e.g., DashboardCustomElement or DashboardElementProxy) that defines
requestNavigation(proceed) and delegates to the Angular component instance via a
stable host property; have the Dashboard Angular component set that stable
property (for example assign this to hostElement.__dashboardInstance in the
component's ngAfterViewInit using an injected ElementRef or `@ViewChild` host
reference) so the custom element can read that property
(element.__dashboardInstance) and call instance.requestNavigation(proceed);
update registration to use the new subclass and delete the Object.defineProperty
block and any reads of ngElementStrategy/componentRef.instance.
---
Outside diff comments:
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts`:
- Around line 353-359: The discardEdit function restores x/y from cardsPosition
but not size, causing cancelled edits to keep stale dimensions; update the
mapping in discardEdit (the cards.set([...].map callback) that uses
this.cardsPosition.get(c.id)) to also reapply width/height from the saved
position (e.g., pos?.w and pos?.h) when present (falling back to the card’s
existing w/h or the value in cardsSnapshot), so sizes saved by saveCardsPosition
are restored along with x/y.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0c9bade-651d-4e10-8a29-c03f5994ead2
📒 Files selected for processing (21)
docs/dashboard.mdpackage.jsonprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.htmlprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.scssprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.tsprojects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.tsprojects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.htmlprojects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.scssprojects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.spec.tsprojects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.tsprojects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.htmlprojects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.scssprojects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.spec.tsprojects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.tsprojects/ngx/declarative-ui/dashboard/index.tsprojects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.htmlprojects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.scssprojects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.spec.tsprojects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.tsprojects/ngx/declarative-ui/table/declarative-table/declarative-table.component.htmlprojects/webcomponents-dashboard/main.ts
Comment on lines
+636
to
+646
| component.onGridChange({ nodes: [{ id: 'c1', x: 1, y: 1 }] } as any); | ||
| expect(component.hasUnsavedChanges()).toBe(true); | ||
| }); | ||
|
|
||
| it('ignores grid change events fired outside edit mode', () => { | ||
| const { fixture, component } = setup(); | ||
| fixture.componentRef.setInput('config', { title: 'Operations' }); | ||
| configureFor(component); | ||
| fixture.detectChanges(); | ||
|
|
||
| component.onGridChange({ nodes: [{ id: 'c1', x: 1, y: 1 }] } as any); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts"
# Show the referenced lines with context
nl -ba "$FILE" | sed -n '620,675p'
# Find all occurrences of the cast pattern in the file (any casts)
rg -n " as any|<any>|any\)" "$FILE" || true
# Search for possible event/node types in this spec or nearby code
rg -n "nodesCB|GridChange|onGridChange|grid change|nodes.*CB|CB" "$FILE" || true
# Search globally for nodesCB type definition to suggest accurate casting
rg -n "export (type|interface|class) nodesCB|nodesCB" projects/ngx projects -S || trueRepository: openmfp/webcomponents
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts"
# Print lines 620-675 with line numbers (awk alternative to nl)
awk 'NR>=620 && NR<=675 {printf "%d\t%s\n", NR, $0}' "$FILE"
# Find all occurrences of "as any" or "<any>" in the file (and "any);" patterns)
rg -n "(as any|<any>|any\)" "$FILE" || true
# Search for grid event types/usages nearby
rg -n "(nodesCB|GridChange|onGridChange|grid change|nodes.*CB|nodes.*\\btype\\b|CB\\b)" "$FILE" || true
# Search elsewhere for the nodesCB type definition (to recommend correct casting)
rg -n "export (type|interface|class) nodesCB|nodesCB" projects/ngx projects/webcomponents -S || trueRepository: openmfp/webcomponents
Length of output: 2975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SPEC="projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts"
COMP="projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts"
echo "== nodesCB usage in component.ts =="
rg -n "nodesCB|onGridChange|changeCB" "$COMP"
echo
echo "== component.ts lines around onGridChange =="
awk 'NR>=360 && NR<=430 {printf "%d\t%s\n", NR, $0}' "$COMP"
echo
echo "== component.ts import/type area =="
awk 'NR>=1 && NR<=90 {printf "%d\t%s\n", NR, $0}' "$COMP"
echo
echo "== any casts in spec =="
# escape/parens safely by searching for literal ' as any'
rg -n " as any" "$SPEC" || true
echo
echo "== also search for '<any>' casts =="
rg -n "<any>" "$SPEC" || trueRepository: openmfp/webcomponents
Length of output: 5210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SPEC="projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts"
echo "== Spec import section =="
awk 'NR>=1 && NR<=120 {printf "%d\t%s\n", NR, $0}' "$SPEC"
echo
echo "== Any/typed casts elsewhere in spec =="
rg -n "onGridChange\\(|nodesCB|GridStackNode| as any| as unknown" "$SPEC"
echo
echo "== Context around earlier onGridChange calls =="
awk 'NR>=170 && NR<=260 {printf "%d\t%s\n", NR, $0}' "$SPEC"
echo "----"
awk 'NR>=400 && NR<=470 {printf "%d\t%s\n", NR, $0}' "$SPEC"Repository: openmfp/webcomponents
Length of output: 11492
Remove as any from onGridChange test inputs (lint gate fails)
Dashboard.onGridChangetakesevent: nodesCBand the spec currently usesas anyat lines 636 and 646, triggering@typescript-eslint/no-explicit-any.- Replace both casts with
as never(matching the otheronGridChangetest cases in the same file) or cast tonodesCBby importing it, avoidingany.
🧰 Tools
🪛 ESLint
[error] 636-636: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 646-646: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.spec.ts`
around lines 636 - 646, The test is using `as any` when calling
`component.onGridChange(...)` which violates the no-explicit-any lint rule;
replace those casts with `as never` (consistent with other tests) or cast to the
actual `nodesCB` type by importing it and using `as nodesCB` so the calls to
`onGridChange` (referenced as component.onGridChange and the test helper
setup/fixture) use a proper type instead of `any` in both occurrences.
Comment on lines
+105
to
+110
| hasUnsavedChanges = computed(() => { | ||
| if (!this.editMode()) return false; | ||
| if (this.gridDirty()) return true; | ||
| return ( | ||
| JSON.stringify(this.sections()) !== this.sectionsSnapshotJson || | ||
| JSON.stringify(this.cards()) !== this.cardsSnapshotJson |
There was a problem hiding this comment.
The dirty-check snapshot strategy breaks on non-JSON card inputs.
cards() is part of the public API, and CardConfig.componentInputs is Record<string, unknown>. Serializing the whole graph here means entering edit mode or recomputing hasUnsavedChanges() can throw on cyclic inputs and will silently miscompare non-JSON values like functions, Dates, Maps, or DOM-backed objects. The paired shallow copies also won't restore nested mutations on discard. Please snapshot/compare only the editable layout fields instead of the full card object graph.
Also applies to: 236-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/ngx/declarative-ui/dashboard/dashboard/dashboard.component.ts`
around lines 105 - 110, The current hasUnsavedChanges() uses JSON.stringify on
the full objects (this.cards(), this.sections()), which breaks on non-JSON
values and cyclic graphs; instead implement a deterministic layout-only snapshot
helper (e.g., buildLayoutSnapshot(cards, sections)) that maps CardConfig objects
to only editable/layout fields (ids, positions/order, layout metadata, and any
simple primitive editable properties — do NOT include componentInputs or other
runtime graphs like functions, Dates, Maps, DOM refs) and use JSON.stringify on
that reduced structure for comparison and snapshot storage (replace uses of
this.sectionsSnapshotJson and this.cardsSnapshotJson and update snapshot
creation/restore sites referenced around hasUnsavedChanges and the other spots
called out). Ensure to reference the helper from hasUnsavedChanges, snapshot
creation, and discard/restore logic so comparisons never serialize full card
graphs or componentInputs.
| @@ -0,0 +1,17 @@ | |||
| <ui5-dialog [open]="open()" (ui5BeforeClose)="cancelled.emit()"> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In UI5 Web Components dialog, does the before-close event fire for all close reasons (including programmatic close after confirm actions), and what event detail is recommended to distinguish user cancel from other close paths?
💡 Result:
Yes: UI5 Web Components’ ui5-dialog before-close event is fired for close attempts coming from different close paths, and the event can be cancelled via preventDefault. The component documentation describes before-close as “Fired before the component is closed. This event can be cancelled, which will prevent the popup from closing” and shows an event parameter escPressed to indicate when ESC triggers the close attempt [1]. However, the documentation indicates only one explicit “reason” detail: escPressed (ESC-triggered close attempt). There is no documented dedicated field for “user cancel vs other close reasons” beyond ESC [1]. To distinguish user cancel from other close paths, the recommended approach is to use the specific UI action that represents “cancel” in your confirm/cancel flow, and in your before-close handler rely on escPressed to detect the ESC path (and handle it separately), while treating other close paths as non-ESC and using your own application-level state (e.g., what button triggered the closing). This aligns with the fact that before-close provides only escPressed in its parameter list [1], and the official documentation does not mention a broader “reason/type” field for other close causes. Note: UI5 Web Components maintain that before-close is cancelable via preventDefault and that closing behavior is controllable from that event [2].
Citations:
- 1: https://ui5.github.io/webcomponents/nightly/components/Dialog/
- 2: ui5-dialog, ui5-popover, ui5-responsive-popover: before-close Event Fails with preventDefault and programatic closing of the dilog UI5/webcomponents#10501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the component TS to inspect how confirm/cancel close the dialog
fd -t f "discard-changes-dialog.component.(ts|tsx)$" projects/ngx/declarative-ui/dashboard/discard-changes-dialog -a
# Show the template around the ui5-dialog and any confirm/cancel buttons
TEMPLATE="projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.html"
test -f "$TEMPLATE" && echo "---- $TEMPLATE ----" && cat -n "$TEMPLATE"
# Show the component TS handlers for confirm/cancelled/open and any before-close handling
TSFILE="$(fd -t f "discard-changes-dialog.component.ts$" projects/ngx/declarative-ui/dashboard/discard-changes-dialog -a || true)"
if [ -z "$TSFILE" ]; then
TSFILE="$(fd -t f "discard-changes-dialog.component.ts$" projects/ngx/declarative-ui/dashboard -a | head -n 1 || true)"
fi
test -n "$TSFILE" && echo "---- $TSFILE ----" && cat -n "$TSFILE"Repository: openmfp/webcomponents
Length of output: 2381
Guard cancelled.emit() in ui5BeforeClose (avoid double/conflicting close events).
ui5BeforeClose fires for all close attempts (including programmatic close), so the current (ui5BeforeClose)="cancelled.emit()" can emit cancelled during a “Discard”/confirm flow when the parent closes the dialog, producing confirm + cancelled together. It can also double-emit on the explicit “Cancel” button path.
Update the template to call a handler that emits cancelled only for the intended cancel route (e.g., ESC via the documented event.escPressed, and/or only when the close was initiated by your cancel action via an application flag).
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.html`
at line 1, The template currently calls cancelled.emit() directly from the
ui5BeforeClose handler causing duplicate/conflicting close events; replace the
inline (ui5BeforeClose)="cancelled.emit()" with a reference to a new component
method (e.g., onBeforeClose) and implement that method in
DiscardChangesDialogComponent to only call this.cancelled.emit() when the close
was initiated by user-cancel actions (check event.escPressed or a component flag
like cancelInitiated set by your Cancel button click handler) and ignore
programmatic closes/confirm flow; ensure the Cancel button sets
cancelInitiated=true before closing and clear/reset the flag after handling so
confirm/programmatic closes do not emit cancelled.
Comment on lines
+13
to
+19
| @Component({ | ||
| selector: 'mfp-discard-changes-dialog', | ||
| imports: [Button, Dialog, Icon, Title], | ||
| templateUrl: './discard-changes-dialog.component.html', | ||
| styleUrl: './discard-changes-dialog.component.scss', | ||
| encapsulation: ViewEncapsulation.ShadowDom, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify dashboard dialog components are standalone + OnPush.
fd -i "discard-changes-dialog.component.ts|unsaved-changes-dialog.component.ts|dashboard.component.ts" projects/ngx \
| xargs -I{} sh -c 'echo "== {} =="; rg -n "standalone\\s*:\\s*true|changeDetection\\s*:\\s*ChangeDetectionStrategy\\.OnPush|`@Component`\\(" "{}" -n -C2'Repository: openmfp/webcomponents
Length of output: 814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.ts \
projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts
do
echo "== $f =="
rg -n "standalone\s*:|changeDetection\s*:" "$f" || true
# print the whole `@Component` metadata block area
sed -n '1,90p' "$f"
echo
doneRepository: openmfp/webcomponents
Length of output: 2652
Add standalone: true and OnPush to the dashboard dialog components
DiscardChangesDialog’s @Component metadata lacks standalone: true and changeDetection: ChangeDetectionStrategy.OnPush (despite having imports). The same issue exists in projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts.
Suggested fix
-import { Component, ViewEncapsulation, input, output } from '`@angular/core`';
+import {
+ ChangeDetectionStrategy,
+ Component,
+ ViewEncapsulation,
+ input,
+ output,
+} from '`@angular/core`';
`@Component`({
selector: 'mfp-discard-changes-dialog',
+ standalone: true,
imports: [Button, Dialog, Icon, Title],
templateUrl: './discard-changes-dialog.component.html',
styleUrl: './discard-changes-dialog.component.scss',
encapsulation: ViewEncapsulation.ShadowDom,
+ changeDetection: ChangeDetectionStrategy.OnPush,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @Component({ | |
| selector: 'mfp-discard-changes-dialog', | |
| imports: [Button, Dialog, Icon, Title], | |
| templateUrl: './discard-changes-dialog.component.html', | |
| styleUrl: './discard-changes-dialog.component.scss', | |
| encapsulation: ViewEncapsulation.ShadowDom, | |
| }) | |
| import { | |
| ChangeDetectionStrategy, | |
| Component, | |
| ViewEncapsulation, | |
| input, | |
| output, | |
| } from '`@angular/core`'; | |
| `@Component`({ | |
| selector: 'mfp-discard-changes-dialog', | |
| standalone: true, | |
| imports: [Button, Dialog, Icon, Title], | |
| templateUrl: './discard-changes-dialog.component.html', | |
| styleUrl: './discard-changes-dialog.component.scss', | |
| encapsulation: ViewEncapsulation.ShadowDom, | |
| changeDetection: ChangeDetectionStrategy.OnPush, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/ngx/declarative-ui/dashboard/discard-changes-dialog/discard-changes-dialog.component.ts`
around lines 13 - 19, Update the `@Component` metadata for the
DiscardChangesDialog component (selector 'mfp-discard-changes-dialog') to
include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush;
import ChangeDetectionStrategy from `@angular/core` at the top of the file. Apply
the same change to the UnsavedChangesDialog component in
projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts
so both components declare standalone: true and use
ChangeDetectionStrategy.OnPush.
Comment on lines
+18
to
+24
| @Component({ | ||
| selector: 'mfp-unsaved-changes-dialog', | ||
| imports: [Button, Dialog, Icon, Title], | ||
| templateUrl: './unsaved-changes-dialog.component.html', | ||
| styleUrl: './unsaved-changes-dialog.component.scss', | ||
| encapsulation: ViewEncapsulation.ShadowDom, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts"
echo "== File exists and basic stats =="
ls -l "$FILE"
wc -l "$FILE"
echo "== Component decorator snippet =="
# Print around the `@Component` block
python3 - <<'PY'
import re, itertools
path="projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# find `@Component`(
for i,line in enumerate(lines):
if "`@Component`" in line:
start=i
break
else:
raise SystemExit("No `@Component` found")
# print until closing '})' or '})' with encapsulation etc; crude
out=[]
brace=0
in_block=False
for j in range(start, len(lines)):
l=lines[j]
if "`@Component`" in l:
in_block=True
if in_block:
out.append((j+1,l.rstrip("\n")))
brace += l.count('(') - l.count(')')
# stop when we've likely closed decorator call: line contains '})' and brace <=0
if '})' in l or '})' in l.replace(" ",""):
# still might not be correct; attempt more robust:
pass
# heuristic: stop when line contains "})" AND we already included first "`@Component`("
if out and '})' in l and brace<=0:
end=j
break
else:
end=start+20
for ln,txt in out[:80]:
print(f"{ln}: {txt}")
PY
echo "== Check for standalone & changeDetection =="
rg -n "standalone\s*:" "$FILE" || true
rg -n "changeDetection\s*:" "$FILE" || true
echo "== Check usage in any declarations/imports =="
# Find other references to this component class
CLASS="UnsavedChangesDialogComponent"
rg -n "$CLASS" projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog -S || true
rg -n "$CLASS" projects/ngx -S || true
echo "== Check if this component is declared in a module anywhere (if any) =="
rg -n "declarations:\s*\[.*UnsavedChangesDialogComponent" -S projects/ngx || true
rg -n "UnsavedChangesDialogComponent" -S projects/ngx/declarative-ui || trueRepository: openmfp/webcomponents
Length of output: 829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts"
echo "== Full file =="
cat -n "$FILE"
echo "== Imports from `@angular/core` =="
rg -n "from '`@angular/core`'" "$FILE" || true
echo "== Check specific decorator fields =="
rg -n "standalone\s*:" "$FILE" || true
rg -n "changeDetection\s*:" "$FILE" || true
rg -n "ChangeDetectionStrategy" "$FILE" || true
echo "== Check for signal input/output usage =="
rg -n "\binput\s*\(" "$FILE" || true
rg -n "\boutput\s*\(" "$FILE" || trueRepository: openmfp/webcomponents
Length of output: 1861
Make UnsavedChangesDialog standalone and OnPush.
projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts (lines 18-24): @Component declares imports: [...] but is missing standalone: true, and it doesn’t set changeDetection: ChangeDetectionStrategy.OnPush.
Suggested fix
-import { Component, ViewEncapsulation, input, output } from '`@angular/core`';
+import {
+ ChangeDetectionStrategy,
+ Component,
+ ViewEncapsulation,
+ input,
+ output,
+} from '`@angular/core`';
`@Component`({
selector: 'mfp-unsaved-changes-dialog',
+ standalone: true,
imports: [Button, Dialog, Icon, Title],
templateUrl: './unsaved-changes-dialog.component.html',
styleUrl: './unsaved-changes-dialog.component.scss',
encapsulation: ViewEncapsulation.ShadowDom,
+ changeDetection: ChangeDetectionStrategy.OnPush,
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/ngx/declarative-ui/dashboard/unsaved-changes-dialog/unsaved-changes-dialog.component.ts`
around lines 18 - 24, The component decorator for UnsavedChangesDialog currently
lists imports but is missing standalone and OnPush change detection; update the
`@Component` on UnsavedChangesDialog (unsaved-changes-dialog.component.ts) to
include standalone: true and changeDetection: ChangeDetectionStrategy.OnPush,
and add the corresponding ChangeDetectionStrategy import from `@angular/core` so
the decorator reads both standalone and changeDetection properties while keeping
the existing imports array (Button, Dialog, Icon, Title).
| > | ||
| <ui5-option value="5">5</ui5-option> | ||
| <ui5-option value="10">10</ui5-option> | ||
| <ui5-option value="10">20</ui5-option> |
There was a problem hiding this comment.
Restore the 20-item option’s value.
This option now displays 20 but still emits 10. Because paginationLimitChanged forwards the selected numeric value, picking 20 will silently keep the table at 10 items and push the wrong limit upstream.
Suggested fix
- <ui5-option value="10">20</ui5-option>
+ <ui5-option value="20">20</ui5-option>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ui5-option value="10">20</ui5-option> | |
| <ui5-option value="20">20</ui5-option> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@projects/ngx/declarative-ui/table/declarative-table/declarative-table.component.html`
at line 141, The ui5-option that displays "20" currently has value="10", causing
paginationLimitChanged to emit the wrong numeric limit; update the <ui5-option
value="10">20</ui5-option> to <ui5-option value="20">20</ui5-option> (and audit
other <ui5-option> entries in declarative-table for mismatched value/display
text) so paginationLimitChanged receives the correct numeric limit.
Comment on lines
+15
to
+36
| // `createCustomElement` only proxies @Input()/output() — public methods on the | ||
| // component class are NOT reachable from the DOM. Forward `requestNavigation` | ||
| // explicitly so non-Angular consumers (UI5, plain JS, Luigi, etc.) can route | ||
| // their navigation hooks through the dashboard's unsaved-changes guard. | ||
| Object.defineProperty(DashboardElement.prototype, 'requestNavigation', { | ||
| value(proceed: () => void): boolean { | ||
| const strategy = (this as unknown as { | ||
| ngElementStrategy?: { componentRef?: { instance?: Dashboard } }; | ||
| }).ngElementStrategy; | ||
| const instance = strategy?.componentRef?.instance; | ||
| if (!instance) { | ||
| // Element not yet connected / Angular component not yet created. | ||
| // Falling back to running the navigation immediately preserves the | ||
| // original (pre-guard) behaviour rather than silently blocking the user. | ||
| proceed(); | ||
| return true; | ||
| } | ||
| return instance.requestNavigation(proceed); | ||
| }, | ||
| configurable: true, | ||
| writable: true, | ||
| }); |
There was a problem hiding this comment.
Replace prototype monkey-patching with a custom-element proxy for requestNavigation()
projects/webcomponents-dashboard/main.ts forwards requestNavigation by mutating DashboardElement.prototype and reading ngElementStrategy.componentRef.instance (Angular Elements internal), which violates the required imperative API pattern. Implement requestNavigation via a dedicated custom-element subclass/proxy that invokes the Angular component instance through viewChild, without touching ngElementStrategy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/webcomponents-dashboard/main.ts` around lines 15 - 36, Remove the
prototype monkey-patch on DashboardElement.prototype and instead register a
dedicated custom-element subclass (e.g., DashboardCustomElement or
DashboardElementProxy) that defines requestNavigation(proceed) and delegates to
the Angular component instance via a stable host property; have the Dashboard
Angular component set that stable property (for example assign this to
hostElement.__dashboardInstance in the component's ngAfterViewInit using an
injected ElementRef or `@ViewChild` host reference) so the custom element can read
that property (element.__dashboardInstance) and call
instance.requestNavigation(proceed); update registration to use the new subclass
and delete the Object.defineProperty block and any reads of
ngElementStrategy/componentRef.instance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added edition marker
with the discard changes
and unsaved changes
Summary by CodeRabbit
Release Notes
New Features
Documentation