Ux feedback 2 and some improvments#186
Open
Sobyt483 wants to merge 5 commits into
Open
Conversation
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
…ux-feedback-2 Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
Signed-off-by: Sobyt483 <andrianingomel@gmail.com>
📝 WalkthroughWalkthroughIntroduces a ChangesMount Configuration Type Refactor
Dashboard Edit Mode and Layout Fixes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This was
linked to
issues
Jul 1, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
projects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.scss (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a theme-aware divider color.
rgb(229, 229, 229)hard-codes the separator to one palette and won't track UI5 theme or contrast-mode changes. Please switch this to the supported theme token/selector after verifying the styling hook.🤖 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/edit-cards-dialog/edit-cards-dialog.component.scss` around lines 30 - 32, The separator color in __list-item-container is hard-coded, so update the style to use the supported UI5 theme-aware divider token or selector instead of the fixed rgb value. Verify the existing styling hook in edit-cards-dialog.component.scss and adjust the nested rule for &__list-item-container so it follows theme and contrast-mode changes.projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.ts (1)
49-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEquality via
JSON.stringifyoncomponentInputsis fragile.
componentInputsis typed asRecord<string, unknown>, so it can contain non-JSON-safe values (circular references,Date, functions returning inconsistent stringification,Map/Set). A circular reference will throw inside theequalcallback, which runs during Angular's computed recomputation — this would surface as an uncaught error breaking card rendering rather than a caught, recoverable failure.JSON.stringifyequality is also order-sensitive for object keys, so two logically-equal objects built with keys in different order would be treated as different.Consider a small, safe structural-equality helper (or a well-known deep-equal utility) instead, guarded so it can't throw.
♻️ Proposed safer equality
- { - equal: (a, b) => - a.type === b.type && - a.component === b.component && - JSON.stringify(a.componentInputs) === JSON.stringify(b.componentInputs), - }, + { + equal: (a, b) => { + if (a.type !== b.type || a.component !== b.component) { + return false; + } + try { + return JSON.stringify(a.componentInputs) === JSON.stringify(b.componentInputs); + } catch { + return false; + } + }, + },🤖 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/card/dashboard-card.component.ts` around lines 49 - 64, The `mountCfg` computed in `dashboard-card.component.ts` uses `JSON.stringify` inside the `equal` callback for `componentInputs`, which can throw on circular or non-JSON-safe values and is order-sensitive. Replace that comparison with a safe structural/deep-equality helper that tolerates `Record<string, unknown>` inputs without throwing, and keep the `type` and `component` checks in the same `mountCfg` equality logic.
🤖 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.ts`:
- Around line 226-238: The background-image effect in dashboard.component.ts is
depending on the full config signal, so unrelated config reference changes are
resetting backgroundImageHeight and reloading the image. Update the effect to
react only to backgroundImageUrl changes by reading just that field (or
otherwise guarding against unchanged URLs) in the effect that uses this.config()
and sets backgroundImageHeight, so the image load and height reset only happen
when the URL actually changes.
---
Nitpick comments:
In `@projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.ts`:
- Around line 49-64: The `mountCfg` computed in `dashboard-card.component.ts`
uses `JSON.stringify` inside the `equal` callback for `componentInputs`, which
can throw on circular or non-JSON-safe values and is order-sensitive. Replace
that comparison with a safe structural/deep-equality helper that tolerates
`Record<string, unknown>` inputs without throwing, and keep the `type` and
`component` checks in the same `mountCfg` equality logic.
In
`@projects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.scss`:
- Around line 30-32: The separator color in __list-item-container is hard-coded,
so update the style to use the supported UI5 theme-aware divider token or
selector instead of the fixed rgb value. Verify the existing styling hook in
edit-cards-dialog.component.scss and adjust the nested rule for
&__list-item-container so it follows theme and contrast-mode changes.
🪄 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: 91512bf4-143e-4481-b727-25dcf6367807
📒 Files selected for processing (11)
projects/ngx/declarative-ui/dashboard/card/dashboard-card.component.tsprojects/ngx/declarative-ui/dashboard/card/utils/mount-angular-card.tsprojects/ngx/declarative-ui/dashboard/card/utils/mount-sap-card.tsprojects/ngx/declarative-ui/dashboard/card/utils/mount-wc-card.tsprojects/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/edit-cards-dialog/edit-cards-dialog.component.htmlprojects/ngx/declarative-ui/dashboard/edit-cards-dialog/edit-cards-dialog.component.scssprojects/ngx/declarative-ui/dashboard/models/dashboard.model.ts
Comment on lines
+226
to
+238
| effect((onCleanup) => { | ||
| const url = this.config().backgroundImageUrl; | ||
| this.backgroundImageHeight.set(null); | ||
| if (!url) return; | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| this.backgroundImageHeight.set(img.naturalHeight); | ||
| }; | ||
| img.src = url; | ||
| onCleanup(() => { | ||
| img.onload = null; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Background-image effect re-triggers (and can flicker) on any config reference change, not just backgroundImageUrl changes.
The effect depends on the whole config() signal read, so any new config object reference — even one with the same backgroundImageUrl — resets backgroundImageHeight to null (temporarily falling back to background-size: auto) and reloads the image. This can reintroduce the kind of visual flicker this PR is meant to eliminate, and causes an unnecessary image reload.
♻️ Guard against unrelated config changes
+ private lastBackgroundImageUrl?: string;
+
constructor() {
...
effect((onCleanup) => {
const url = this.config().backgroundImageUrl;
+ if (url === this.lastBackgroundImageUrl) return;
+ this.lastBackgroundImageUrl = url;
this.backgroundImageHeight.set(null);
if (!url) return;
const img = new Image();
img.onload = () => {
this.backgroundImageHeight.set(img.naturalHeight);
};
img.src = url;
onCleanup(() => {
img.onload = null;
});
});
}🤖 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 226 - 238, The background-image effect in dashboard.component.ts is
depending on the full config signal, so unrelated config reference changes are
resetting backgroundImageHeight and reloading the image. Update the effect to
react only to backgroundImageUrl changes by reading just that field (or
otherwise guarding against unchanged URLs) in the effect that uses this.config()
and sets backgroundImageHeight, so the image load and height reset only happen
when the URL actually changes.
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
1 participant
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.
Addressed issue from UX feedback #183
Fix bug with resize saving #175
Fix background issues #177
Additionally fix cards flickering when user save or discard changes
Summary by CodeRabbit
New Features
Bug Fixes
Tests