WEB-894 | Ng-busy lazy loader + no-data null record pipe#3443
Open
anshita-21 wants to merge 1 commit intoopenMF:devfrom
Open
WEB-894 | Ng-busy lazy loader + no-data null record pipe#3443anshita-21 wants to merge 1 commit intoopenMF:devfrom
anshita-21 wants to merge 1 commit intoopenMF:devfrom
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Busy Loading System src/app/shared/directives/ng-busy.directive.ts, src/app/shared/_busy.scss |
New standalone NgBusyDirective that toggles host into a busy state (injects global spinner styles, preserves/restores content and disabled state). New SCSS partial provides spinner styles and animations. |
Create Center src/app/centers/create-center/create-center.component.ts, src/app/centers/create-center/create-center.component.html |
Added public loading: boolean, imported NgBusyDirective, set loading = true on submit and switched subscribe to object form (next/error). Template binds [mifosxNgBusy] and [busyText] on submit button. |
Create Group src/app/groups/create-group/create-group.component.ts, src/app/groups/create-group/create-group.component.html |
Converted component to standalone, added NgBusyDirective import and loading state, tightened client autocomplete to clear results for null/short inputs, set loading lifecycle around createGroup with next/error handlers, and template binds busy directive. |
Groups List Display src/app/groups/groups.component.html |
Updated table cells to render a translated no-data placeholder (labels.text.no-data) when group properties are falsy. |
Translations (multiple locales) src/assets/translations/en-US.json, src/assets/translations/cs-CS.json, src/assets/translations/de-DE.json, src/assets/translations/es-CL.json, src/assets/translations/es-MX.json, src/assets/translations/fr-FR.json, src/assets/translations/it-IT.json, src/assets/translations/ko-KO.json, src/assets/translations/lt-LT.json, src/assets/translations/lv-LV.json, src/assets/translations/ne-NE.json, src/assets/translations/pt-PT.json, src/assets/translations/sw-SW.json |
Added labels.buttons.Creating... and labels.text.no-data (en-US) and added text.no-data → "-" across many locale files for a consistent no-data placeholder and busy label. |
Sequence Diagram
sequenceDiagram
actor User
participant Component as Create Component
participant Directive as NgBusyDirective
participant DOM as Browser DOM
participant Service as Backend Service
participant API as Backend API
User->>Component: Click Submit
Component->>Component: set loading = true
Component->>Directive: [mifosxNgBusy] = true
Directive->>DOM: inject spinner styles (once) and replace content with spinner + busyText
Component->>Service: create request(data)
Service->>API: HTTP POST
API-->>Service: response / error
alt Success
Service-->>Component: next handler
Component->>Component: navigate to list
Component->>Component: set loading = false
Component->>Directive: [mifosxNgBusy] = false
Directive->>DOM: restore original content/state
else Error
Service-->>Component: error handler
Component->>Component: set loading = false
Component->>Directive: [mifosxNgBusy] = false
Directive->>DOM: restore original content/state
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- WEB-521 fix(i18n): add missing translations and apply translate pipes across … #2934 — Overlapping i18n work: adds/uses translation keys (no-data / Creating...) and updates templates to use busy text.
Suggested reviewers
- IOhacker
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title directly reflects the main changes: addition of an ng-busy directive for loading states and no-data handling for null records, matching the core objectives. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
anshita-21
changed the title
anshita-21
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/shared/directives/ng-busy.directive.ts (2)
55-57: Consider using Renderer2 consistently for SSR compatibility.Direct
innerHTMLmanipulation (lines 56, 73) bypasses Angular's platform abstraction, which can cause issues with server-side rendering. While the current usage is safe (no user input), usingRenderer2methods consistently would improve SSR compatibility.The static analysis XSS warnings are false positives here since
originalContentcomes from the component's own template, not user input.Also applies to: 71-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 55 - 57, The directive currently manipulates the host element DOM directly (this.el.nativeElement.innerHTML = '' and appendChild(busyContent)); replace those with Renderer2 calls for SSR safety: use renderer.setProperty(this.el.nativeElement, 'innerHTML', '') to clear or iterate children and call renderer.removeChild for each, and use renderer.appendChild(this.el.nativeElement, busyContent) to add the busy node; update NgBusyDirective where busyContent is created to use renderer.createElement/createText if needed so the same Renderer2 instance is used for all DOM operations.
91-95: Hardcoded white spinner may not be visible on light buttons.The spinner uses hardcoded white colors (
#ffffff), which won't be visible on light-themed or outline-style buttons. Consider usingcurrentColoror CSS custom properties to inherit from the button's text color.💡 Alternative using currentColor
- this.renderer.setStyle(this.spinnerElement, 'border', '2px solid rgba(255, 255, 255, 0.3)'); - this.renderer.setStyle(this.spinnerElement, 'border-top', '2px solid `#ffffff`'); + this.renderer.setStyle(this.spinnerElement, 'border', '2px solid currentColor'); + this.renderer.setStyle(this.spinnerElement, 'border-top-color', 'transparent'); + this.renderer.setStyle(this.spinnerElement, 'opacity', '0.8');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 91 - 95, The spinner currently sets hardcoded white colors via renderer.setStyle on this.spinnerElement ('border' and 'border-top'), which fails on light backgrounds; update the styles in ng-busy.directive to use an inheritable color (e.g., use 'currentColor' for the spinner border or a CSS custom property like '--spinner-color' with a sensible fallback) instead of '#ffffff', and ensure both the general 'border' and the 'border-top' assignments reference the chosen inherited value so the spinner color follows the button's text color or theme variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 71-74: The restoration logic in ng-busy.directive.ts uses a truthy
check on this.originalContent which skips restoring empty strings; change the
conditional in the restore block to explicitly check for null/undefined (e.g.,
this.originalContent !== null && this.originalContent !== undefined) so that
empty string content is correctly restored to this.el.nativeElement.innerHTML;
update the block around the existing comment "// Restore original content" and
the references to this.originalContent and this.el.nativeElement.innerHTML
accordingly.
- Around line 108-130: The addGlobalStyles function currently appends a style
tag per directive instance and never removes it; update the ng-busy.directive to
implement OnDestroy and make global styles a singleton: add a static flag (e.g.,
NgBusyDirective.stylesInjected) and a static reference for the shared style
element so addGlobalStyles only creates/appends the <style> once, and in
ngOnDestroy remove the style element only if this instance is the owner or
decrement a simple instance counter; ensure you still use this.renderer to
create/append/remove the element and keep the unique symbols styleElement,
addGlobalStyles, and ngOnDestroy/OnDestroy in the directive for locating the
changes.
---
Nitpick comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 55-57: The directive currently manipulates the host element DOM
directly (this.el.nativeElement.innerHTML = '' and appendChild(busyContent));
replace those with Renderer2 calls for SSR safety: use
renderer.setProperty(this.el.nativeElement, 'innerHTML', '') to clear or iterate
children and call renderer.removeChild for each, and use
renderer.appendChild(this.el.nativeElement, busyContent) to add the busy node;
update NgBusyDirective where busyContent is created to use
renderer.createElement/createText if needed so the same Renderer2 instance is
used for all DOM operations.
- Around line 91-95: The spinner currently sets hardcoded white colors via
renderer.setStyle on this.spinnerElement ('border' and 'border-top'), which
fails on light backgrounds; update the styles in ng-busy.directive to use an
inheritable color (e.g., use 'currentColor' for the spinner border or a CSS
custom property like '--spinner-color' with a sensible fallback) instead of
'#ffffff', and ensure both the general 'border' and the 'border-top' assignments
reference the chosen inherited value so the spinner color follows the button's
text color or theme variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a7608f7-7ea8-491b-ab24-256b4d8be33e
📒 Files selected for processing (8)
src/app/centers/create-center/create-center.component.htmlsrc/app/centers/create-center/create-center.component.tssrc/app/groups/create-group/create-group.component.htmlsrc/app/groups/create-group/create-group.component.tssrc/app/groups/groups.component.htmlsrc/app/shared/_busy.scsssrc/app/shared/directives/ng-busy.directive.tssrc/assets/translations/en-US.json
👮 Files not reviewed due to content moderation or server errors (6)
- src/assets/translations/en-US.json
- src/app/centers/create-center/create-center.component.html
- src/app/groups/groups.component.html
- src/app/groups/create-group/create-group.component.html
- src/app/shared/_busy.scss
- src/app/centers/create-center/create-center.component.ts
Contributor
|
@anshita-21 please review the error |
IOhacker
requested changes
Mar 26, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Kindly review my comments
Author
Thank you for the review, have updated the PR |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/app/shared/directives/ng-busy.directive.ts (1)
15-17:⚠️ Potential issue | 🟠 MajorDon't remove the shared style tag while other busy directives are still mounted.
globalStylesAddedis static, butstyleElementis instance-local. The first directive instance that injected the shared styles will remove them inngOnDestroy(), even if other[mifosxNgBusy]instances are still alive, so those survivors lose the spinner animation rules. Keep the shared element in a static field and reference-count instances, or leave the global style tag in place for the app lifetime.🛠️ Suggested change
export class NgBusyDirective implements OnInit, OnChanges, OnDestroy { + private static instanceCount = 0; + private static sharedStyleElement: HTMLStyleElement | null = null; `@Input`() mifosxNgBusy = false; @@ - private styleElement: HTMLStyleElement | null = null; private static globalStylesAdded = false; @@ ngOnInit() { + NgBusyDirective.instanceCount++; // Add global styles for animation this.addGlobalStyles(); } @@ private addGlobalStyles() { // Add proper rotation animation only once globally if (!NgBusyDirective.globalStylesAdded) { - this.styleElement = this.renderer.createElement('style'); - this.renderer.setProperty(this.styleElement, 'innerHTML', ` + NgBusyDirective.sharedStyleElement = this.renderer.createElement('style'); + this.renderer.setProperty(NgBusyDirective.sharedStyleElement, 'innerHTML', ` `@keyframes` spin { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } @@ - this.renderer.appendChild(document.head, this.styleElement); + this.renderer.appendChild(document.head, NgBusyDirective.sharedStyleElement); NgBusyDirective.globalStylesAdded = true; } } ngOnDestroy() { - // Clean up style element if this is the last instance - if (this.styleElement && NgBusyDirective.globalStylesAdded) { - this.renderer.removeChild(document.head, this.styleElement); + NgBusyDirective.instanceCount--; + if (NgBusyDirective.instanceCount === 0 && NgBusyDirective.sharedStyleElement) { + this.renderer.removeChild(document.head, NgBusyDirective.sharedStyleElement); + NgBusyDirective.sharedStyleElement = null; NgBusyDirective.globalStylesAdded = false; } }Also applies to: 107-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 15 - 17, The shared style tag is removed by an instance-local styleElement in ngOnDestroy even when other mifosxNgBusy instances still exist; change the implementation so the shared style element is stored in a static field and add a static reference counter (e.g., static sharedStyleElement: HTMLStyleElement | null and static instanceCount = 0) that is incremented when an instance mounts (constructor/ngOnInit) and decremented in ngOnDestroy, and only remove the style element when instanceCount reaches 0 (or alternatively never remove it for app lifetime) while keeping globalStylesAdded logic consistent with the static sharedStyleElement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 21-24: The snapshot of the host's DOM/state is taken once in
ngOnInit (originalContent, originalDisabled) which causes stale restores;
instead capture and store originalContent and originalDisabled immediately
before mutating the element inside showBusyState(), use those per-cycle
snapshots in hideBusyState() to restore, and clear them after restore; update
showBusyState() to set this.originalContent/this.originalDisabled (or per-cycle
vars) right before changing innerHTML/disabled, and update hideBusyState() to
read and restore from those variables rather than the init values.
- Around line 97-98: The fallback "Loading..." must be translated instead of
hardcoded; update the ng-busy directive to use `@ngx-translate/core` (inject
TranslateService into the directive) and replace the fallback assignment in the
busy text code (currently using this.busyText and renderer.createText) with a
translated lookup (e.g., translate.instant('BUSY.LOADING') or translate.get with
subscribe) so that if this.busyText is falsy you call TranslateService for the
i18n key, then pass the resulting string into renderer.createText;
alternatively, make the busyText input required and validate it in the directive
(e.g., throw or console.warn) if missing.
- Line 1: This new source file (ng-busy.directive.ts) is missing the standard
MPL license header; add the repository's canonical MPL header comment banner to
the top of the file (above the import line) so it matches other files, then run
the repo tooling to ensure correctness: execute npm run headers:add (or npm run
headers:check) to apply/verify the header; update the file containing the
Directive class/exports (ng-busy.directive.ts) only—no functional changes to
imports or symbols like Directive, Input, ElementRef, Renderer2, OnInit,
OnChanges, SimpleChanges, OnDestroy, inject.
---
Duplicate comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 15-17: The shared style tag is removed by an instance-local
styleElement in ngOnDestroy even when other mifosxNgBusy instances still exist;
change the implementation so the shared style element is stored in a static
field and add a static reference counter (e.g., static sharedStyleElement:
HTMLStyleElement | null and static instanceCount = 0) that is incremented when
an instance mounts (constructor/ngOnInit) and decremented in ngOnDestroy, and
only remove the style element when instanceCount reaches 0 (or alternatively
never remove it for app lifetime) while keeping globalStylesAdded logic
consistent with the static sharedStyleElement.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 101c7a50-597d-4869-bf69-10484ed64175
📒 Files selected for processing (8)
src/app/centers/create-center/create-center.component.htmlsrc/app/centers/create-center/create-center.component.tssrc/app/groups/create-group/create-group.component.htmlsrc/app/groups/create-group/create-group.component.tssrc/app/groups/groups.component.htmlsrc/app/shared/_busy.scsssrc/app/shared/directives/ng-busy.directive.tssrc/assets/translations/en-US.json
✅ Files skipped from review due to trivial changes (3)
- src/app/groups/groups.component.html
- src/app/shared/_busy.scss
- src/assets/translations/en-US.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/centers/create-center/create-center.component.html
- src/app/groups/create-group/create-group.component.html
- src/app/groups/create-group/create-group.component.ts
IOhacker
requested changes
Mar 30, 2026
Contributor
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Missing translation files for the other Mifos WebApp supported languages, "no-data:-" key/value is very generic
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/app/shared/directives/ng-busy.directive.ts (2)
44-57: 🛠️ Refactor suggestion | 🟠 MajorCapture state before mutation, not at init.
Move the state capture from
ngOnInit()to the start ofshowBusyState()to ensure the most recent content and disabled state are preserved:♻️ Proposed refactor
private showBusyState() { + // Capture current state before mutating + this.originalContent = this.el.nativeElement.innerHTML; + this.originalDisabled = this.el.nativeElement.disabled; + // Add busy class this.renderer.addClass(this.el.nativeElement, this.busyClass);And remove from
ngOnInit():ngOnInit() { - // Store original content and state - this.originalContent = this.el.nativeElement.innerHTML; - this.originalDisabled = this.el.nativeElement.disabled; - // Add global styles for animation this.addGlobalStyles(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 44 - 57, The current implementation captures the element state in ngOnInit; move that capture to the start of showBusyState so you snapshot the latest innerHTML and disabled state right before mutating the element. Update showBusyState to read and store the current content and disabled flag (used by createBusyContent / busyClass logic) before clearing/appending, and remove the redundant capture logic from ngOnInit; ensure references to createBusyContent, busyClass, and any saved state variables remain consistent.
132-138:⚠️ Potential issue | 🟠 MajorngOnDestroy cleanup logic is broken due to instance vs static mismatch.
Only the first directive instance sets
this.styleElement(line 110). Subsequent instances havethis.styleElement = nullbecauseaddGlobalStyles()returns early whenglobalStylesAddedis true. This causes two problems:
- If a non-first instance is destroyed first, cleanup is skipped (harmless but wasteful check).
- If the first instance is destroyed while other instances still exist, the global styles are removed, breaking the spinner animation for remaining busy buttons.
The recommended fix from the prior review (static
styleElement+ instance counter) was not fully applied.🛠️ Proposed fix using static reference and instance counting
private originalContent: string | null = null; private originalDisabled: boolean | null = null; private spinnerElement: HTMLElement | null = null; - private styleElement: HTMLStyleElement | null = null; - private static globalStylesAdded = false; + private static styleElement: HTMLStyleElement | null = null; + private static instanceCount = 0; private el = inject(ElementRef); private renderer = inject(Renderer2); ngOnInit() { // Store original content and state this.originalContent = this.el.nativeElement.innerHTML; this.originalDisabled = this.el.nativeElement.disabled; + NgBusyDirective.instanceCount++; // Add global styles for animation this.addGlobalStyles(); } // ... other methods ... private addGlobalStyles() { // Add proper rotation animation only once globally - if (!NgBusyDirective.globalStylesAdded) { - this.styleElement = this.renderer.createElement('style'); - this.renderer.setProperty(this.styleElement, 'innerHTML', ` + if (!NgBusyDirective.styleElement) { + NgBusyDirective.styleElement = this.renderer.createElement('style'); + this.renderer.setProperty(NgBusyDirective.styleElement, 'innerHTML', ` `@keyframes` spin { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } } .css-spinner { border: 2px solid rgba(255, 255, 255, 0.3) !important; border-top: 2px solid `#ffffff` !important; } button .css-spinner { border: 2px solid rgba(255, 255, 255, 0.3) !important; border-top: 2px solid `#ffffff` !important; } `); - this.renderer.appendChild(document.head, this.styleElement); - NgBusyDirective.globalStylesAdded = true; + this.renderer.appendChild(document.head, NgBusyDirective.styleElement); } } ngOnDestroy() { - // Clean up style element if this is the last instance - if (this.styleElement && NgBusyDirective.globalStylesAdded) { - this.renderer.removeChild(document.head, this.styleElement); - NgBusyDirective.globalStylesAdded = false; + NgBusyDirective.instanceCount--; + // Clean up style element only when last instance is destroyed + if (NgBusyDirective.instanceCount === 0 && NgBusyDirective.styleElement) { + this.renderer.removeChild(document.head, NgBusyDirective.styleElement); + NgBusyDirective.styleElement = null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 132 - 138, NgBusyDirective's cleanup is broken because styleElement is per-instance while globalStylesAdded is static; change to a static shared styleElement and an instance counter so addGlobalStyles() only creates and assigns NgBusyDirective.styleElement once and increments a static instancesCount (in constructor or ngOnInit), and ngOnDestroy decrements instancesCount and only removes the static styleElement from document.head and sets globalStylesAdded = false when instancesCount reaches 0; update references to this.styleElement to NgBusyDirective.styleElement and ensure addGlobalStyles() returns early without nulling instance properties for non-first instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 44-57: The current implementation captures the element state in
ngOnInit; move that capture to the start of showBusyState so you snapshot the
latest innerHTML and disabled state right before mutating the element. Update
showBusyState to read and store the current content and disabled flag (used by
createBusyContent / busyClass logic) before clearing/appending, and remove the
redundant capture logic from ngOnInit; ensure references to createBusyContent,
busyClass, and any saved state variables remain consistent.
- Around line 132-138: NgBusyDirective's cleanup is broken because styleElement
is per-instance while globalStylesAdded is static; change to a static shared
styleElement and an instance counter so addGlobalStyles() only creates and
assigns NgBusyDirective.styleElement once and increments a static instancesCount
(in constructor or ngOnInit), and ngOnDestroy decrements instancesCount and only
removes the static styleElement from document.head and sets globalStylesAdded =
false when instancesCount reaches 0; update references to this.styleElement to
NgBusyDirective.styleElement and ensure addGlobalStyles() returns early without
nulling instance properties for non-first instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1abf1494-d219-4157-983a-522d4479694d
📒 Files selected for processing (20)
src/app/centers/create-center/create-center.component.htmlsrc/app/centers/create-center/create-center.component.tssrc/app/groups/create-group/create-group.component.htmlsrc/app/groups/create-group/create-group.component.tssrc/app/groups/groups.component.htmlsrc/app/shared/_busy.scsssrc/app/shared/directives/ng-busy.directive.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (14)
- src/app/groups/groups.component.html
- src/app/groups/create-group/create-group.component.html
- src/assets/translations/de-DE.json
- src/assets/translations/es-CL.json
- src/assets/translations/fr-FR.json
- src/assets/translations/en-US.json
- src/assets/translations/cs-CS.json
- src/assets/translations/ne-NE.json
- src/assets/translations/lv-LV.json
- src/assets/translations/es-MX.json
- src/assets/translations/it-IT.json
- src/app/shared/_busy.scss
- src/assets/translations/lt-LT.json
- src/assets/translations/sw-SW.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/groups/create-group/create-group.component.ts
Author
Thank you @IOhacker, I've added the key across all lang files. However, ngx-translate falls back to en-US.json by default and the webapp continues to work fine across all languages. |
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/app/shared/directives/ng-busy.directive.ts (4)
107-138:⚠️ Potential issue | 🟠 MajorMake the injected stylesheet truly shared.
globalStylesAddedis static, butstyleElementis owned by the single directive instance that first created it. If another[mifosxNgBusy]instance is still mounted during a route change, destroying the first one removes the shared stylesheet and the surviving spinner loses its animation. Store the style element and an instance count statically, and only remove the node when the last directive is destroyed.🛠️ Suggested change
export class NgBusyDirective implements OnInit, OnChanges, OnDestroy { - private styleElement: HTMLStyleElement | null = null; private static globalStylesAdded = false; + private static styleElement: HTMLStyleElement | null = null; + private static instanceCount = 0; @@ ngOnInit() { + NgBusyDirective.instanceCount++; // Add global styles for animation this.addGlobalStyles(); } @@ private addGlobalStyles() { // Add proper rotation animation only once globally if (!NgBusyDirective.globalStylesAdded) { - this.styleElement = this.renderer.createElement('style'); - this.renderer.setProperty(this.styleElement, 'innerHTML', ` + NgBusyDirective.styleElement = this.renderer.createElement('style'); + this.renderer.setProperty(NgBusyDirective.styleElement, 'innerHTML', ` `@keyframes` spin { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } @@ - this.renderer.appendChild(document.head, this.styleElement); + this.renderer.appendChild(document.head, NgBusyDirective.styleElement); NgBusyDirective.globalStylesAdded = true; } } ngOnDestroy() { - // Clean up style element if this is the last instance - if (this.styleElement && NgBusyDirective.globalStylesAdded) { - this.renderer.removeChild(document.head, this.styleElement); + NgBusyDirective.instanceCount--; + if (NgBusyDirective.instanceCount === 0 && NgBusyDirective.styleElement) { + this.renderer.removeChild(document.head, NgBusyDirective.styleElement); + NgBusyDirective.styleElement = null; NgBusyDirective.globalStylesAdded = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 107 - 138, The stylesheet is treated as global via NgBusyDirective.globalStylesAdded but the style node is instance-owned (styleElement), causing removal while other directives still exist; make the style element and an instance counter static on NgBusyDirective (e.g., NgBusyDirective.styleElement and NgBusyDirective.instanceCount), increment the counter when a directive is initialized (or in addGlobalStyles) and only create/append the static styleElement once (when instanceCount goes from 0 to 1), and in ngOnDestroy decrement the static counter and remove the static styleElement from document.head only when the counter reaches 0; update references in addGlobalStyles() and ngOnDestroy() to use the static symbols.
97-98:⚠️ Potential issue | 🟡 MinorTranslate the default busy label.
Any caller that omits
[busyText]will render hardcoded English viaLoading.... Resolve the fallback through@ngx-translate/coreor makebusyTextrequired.As per coding guidelines "Use proper i18n variables from
@ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 97 - 98, The default hardcoded label "Loading..." must be replaced with a translated fallback; inject TranslateService into the NgBusyDirective constructor and replace the fallback expression so text = this.busyText || this.translate.instant('BUSY.LOADING') (or use this.translate.get(...) async) before calling renderer.createText(text); ensure you add the TranslateService import and constructor parameter, and create a translation key (e.g., BUSY.LOADING) in the i18n files rather than leaving a hardcoded string.
21-24:⚠️ Potential issue | 🟠 MajorOnly restore host state after a real busy cycle.
ngOnChanges()currently runshideBusyState()on the initialfalsebinding, before the directive has ever shown a spinner. On buttons that start disabled until the form is valid, that first pass can clobber the current disabled state; later busy cycles also restore the stale init-time snapshot. CaptureinnerHTML/disabledinsideshowBusyState()and gatehideBusyState()behind a flag that is set only after busy UI was applied.🛠️ Suggested change
export class NgBusyDirective implements OnInit, OnChanges, OnDestroy { + private busyStateApplied = false; private originalContent: string | null = null; private originalDisabled: boolean | null = null; @@ ngOnInit() { - // Store original content and state - this.originalContent = this.el.nativeElement.innerHTML; - this.originalDisabled = this.el.nativeElement.disabled; - // Add global styles for animation this.addGlobalStyles(); } @@ private updateBusyState() { - if (this.mifosxNgBusy) { + if (this.mifosxNgBusy && !this.busyStateApplied) { this.showBusyState(); - } else { + this.busyStateApplied = true; + } else if (!this.mifosxNgBusy && this.busyStateApplied) { this.hideBusyState(); + this.busyStateApplied = false; } } private showBusyState() { + this.originalContent = this.el.nativeElement.innerHTML; + this.originalDisabled = this.el.nativeElement.disabled; + // Add busy class this.renderer.addClass(this.el.nativeElement, this.busyClass); @@ private hideBusyState() { @@ if (this.originalContent !== null && this.originalContent !== undefined) { this.el.nativeElement.innerHTML = this.originalContent; } + this.originalContent = null; + this.originalDisabled = null; }Also applies to: 30-40, 44-57, 59-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` around lines 21 - 24, ngOnChanges is calling hideBusyState on the initial false input and restoring a stale snapshot; fix by capturing the host snapshot (originalContent, originalDisabled) inside showBusyState instead of ngOnInit, add a boolean flag (e.g. busyApplied) set to true when showBusyState has injected the busy UI, and guard hideBusyState to only restore state if busyApplied is true (then reset busyApplied to false after restoring); update references in ngOnChanges, showBusyState and hideBusyState to use this flag and move snapshot logic out of ngOnInit.
1-1:⚠️ Potential issue | 🟠 MajorAdd the standard MPL header before merging.
This new
src/file still lacks the repository banner, soheaders:checkwill keep failing until it is added.Based on learnings: "Applies to src/**/*.{ts,tsx,js,jsx,html,scss,css} : Always run
npm run headers:checkandnpm run headers:addto ensure new files have the correct open source license file headers".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/shared/directives/ng-busy.directive.ts` at line 1, Add the repository's standard MPL file header banner to the top of this new TypeScript file (above the import line in ng-busy.directive.ts) so headers:check passes; run the project's tooling (npm run headers:add) to insert the correct MPL header and verify with npm run headers:check before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/shared/directives/ng-busy.directive.ts`:
- Around line 107-138: The stylesheet is treated as global via
NgBusyDirective.globalStylesAdded but the style node is instance-owned
(styleElement), causing removal while other directives still exist; make the
style element and an instance counter static on NgBusyDirective (e.g.,
NgBusyDirective.styleElement and NgBusyDirective.instanceCount), increment the
counter when a directive is initialized (or in addGlobalStyles) and only
create/append the static styleElement once (when instanceCount goes from 0 to
1), and in ngOnDestroy decrement the static counter and remove the static
styleElement from document.head only when the counter reaches 0; update
references in addGlobalStyles() and ngOnDestroy() to use the static symbols.
- Around line 97-98: The default hardcoded label "Loading..." must be replaced
with a translated fallback; inject TranslateService into the NgBusyDirective
constructor and replace the fallback expression so text = this.busyText ||
this.translate.instant('BUSY.LOADING') (or use this.translate.get(...) async)
before calling renderer.createText(text); ensure you add the TranslateService
import and constructor parameter, and create a translation key (e.g.,
BUSY.LOADING) in the i18n files rather than leaving a hardcoded string.
- Around line 21-24: ngOnChanges is calling hideBusyState on the initial false
input and restoring a stale snapshot; fix by capturing the host snapshot
(originalContent, originalDisabled) inside showBusyState instead of ngOnInit,
add a boolean flag (e.g. busyApplied) set to true when showBusyState has
injected the busy UI, and guard hideBusyState to only restore state if
busyApplied is true (then reset busyApplied to false after restoring); update
references in ngOnChanges, showBusyState and hideBusyState to use this flag and
move snapshot logic out of ngOnInit.
- Line 1: Add the repository's standard MPL file header banner to the top of
this new TypeScript file (above the import line in ng-busy.directive.ts) so
headers:check passes; run the project's tooling (npm run headers:add) to insert
the correct MPL header and verify with npm run headers:check before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69fbaa46-97d5-4c77-8cf2-2ea05c7605af
📒 Files selected for processing (20)
src/app/centers/create-center/create-center.component.htmlsrc/app/centers/create-center/create-center.component.tssrc/app/groups/create-group/create-group.component.htmlsrc/app/groups/create-group/create-group.component.tssrc/app/groups/groups.component.htmlsrc/app/shared/_busy.scsssrc/app/shared/directives/ng-busy.directive.tssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (14)
- src/app/groups/groups.component.html
- src/assets/translations/pt-PT.json
- src/assets/translations/es-CL.json
- src/assets/translations/it-IT.json
- src/assets/translations/de-DE.json
- src/app/shared/_busy.scss
- src/assets/translations/ko-KO.json
- src/assets/translations/ne-NE.json
- src/assets/translations/cs-CS.json
- src/assets/translations/en-US.json
- src/assets/translations/es-MX.json
- src/assets/translations/fr-FR.json
- src/assets/translations/sw-SW.json
- src/assets/translations/lt-LT.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/centers/create-center/create-center.component.html
- src/app/groups/create-group/create-group.component.html
- src/assets/translations/lv-LV.json
- src/app/groups/create-group/create-group.component.ts
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
3 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.
Add ng-busy directive and no-data pipe for improved form UX
#Problem
Two UX gaps were identified across the application:
No loading feedback on form submissions — Submit buttons had no loading indicator. Users received no visual feedback after clicking, making it unclear whether the action was processing or had failed silently.
Blank fields for missing data — Fields with no value were left empty, giving users no indication that data was simply unavailable.
Solution
ng-busy Directive Created a reusable directive that can be dropped onto any submit button to show a spinner and disable the element while a request is in flight — no component refactoring needed.
no-data Pipe Created a transform pipe that replaces empty/null/undefined values with a — placeholder, ensuring users always see a meaningful fallback instead of a blank.
#{https://mifosforge.jira.com/browse/WEB-894}
Screenshots, if any
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes
Style
Translations