Bulk Edit: Changes are applied very slowly, one by one #3451
Conversation
📝 WalkthroughWalkthroughAdds coordinated bulk-edit support: a new BulkOperationStateService manages bulk lifecycle and progress dialog (now using Angular Signals). Several services and components switch to bulk save flows (saveAll), buffer updates during bulk operations, and present reactive progress messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as Client/UI
participant EntityActions as EntityActionsService
participant BulkState as BulkOperationStateService
participant ProgressDlg as ProgressDialog
participant Backend
User->>UI: Trigger bulk edit (n entities)
UI->>EntityActions: requestBulkAction(entities)
activate EntityActions
EntityActions->>BulkState: startBulkOperation(n)
activate BulkState
BulkState->>ProgressDlg: showProgressDialog(signal(message))
activate ProgressDlg
EntityActions->>Backend: saveAll(entities)
activate Backend
loop per entity update (events)
Backend-->>EntityActions: entitySavedEvent
EntityActions->>BulkState: updateBulkOperationProgress(1, false)
BulkState->>ProgressDlg: update message signal
end
Backend-->>EntityActions: saveAll complete
deactivate Backend
EntityActions->>BulkState: completeBulkOperation()
BulkState->>ProgressDlg: close dialog
deactivate ProgressDlg
deactivate BulkState
deactivate EntityActions
ProgressDlg-->>UI: show completion
UI-->>User: notify success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom Pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Deployed to https://pr-3451.aam-digital.net/ |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
@sleidig I created a separate service file and used Could you check the current bulk edit action functionality and also review the codebase so that If you have any feedback or suggestions, I can work on them |
|
todo:
|
sleidig
left a comment
sleidig
left a comment
There was a problem hiding this comment.
Works amazing 🥇 Great to see the progress and much faster than previously.
(I couldn't think of any edge case or way to trip the counter or anything yet ... if anyone has ideas how to break it, please go for it! 😅 )
Can we already integrate this (progress indicator) for bulk delete also here?
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Sebastian <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/app/core/entity/entity-actions/bulk-operation-state.service.ts:
- Around line 91-98: Remove the debug console.log from updateProgressDialog:
delete the console.log(...) call inside the private updateProgressDialog()
method and, if you need persistent observability, replace it with the app's
structured logger (e.g., use an injected logger instead of console) while
leaving the existing this.progressDialogMessage.set(...) intact.
- Around line 53-72: The method updateBulkOperationProgress currently declares a
boolean return but returns nothing when !this.operationInProgress.value; update
it to always return a boolean by returning false in that early-exit branch
(e.g., replace `if (!this.operationInProgress.value) { return; }` with `if
(!this.operationInProgress.value) { return false; }`) so callers of
updateBulkOperationProgress get a consistent boolean; ensure no other code paths
change and keep the existing behavior for processedUpdateCount,
updateProgressDialog, and completeBulkOperation.
In @src/app/core/entity/entity-actions/entity-edit.service.ts:
- Around line 77-93: The bulk operation is started via
bulkOperationState.startBulkOperation(...) but completeBulkOperation() is only
called in the catch block, leaving the success path incomplete; after awaiting
entityMapper.saveAll(newEntities) and before returning the success object you
must call this.bulkOperationState.completeBulkOperation() (and ensure it runs
even if subsequent lines throw), so add the completeBulkOperation() call on the
success path (or refactor the try/catch into try/finally around saveAll to
always call this.bulkOperationState.completeBulkOperation()), referencing
bulkOperationState.startBulkOperation, entityMapper.saveAll, and
completeBulkOperation.
🧹 Nitpick comments (5)
src/app/core/entity/entity-actions/entity-edit.service.ts (1)
89-93: Suggest adding error logging for bulk operation failures.While the error handling correctly completes the bulk operation and re-throws, adding logging would help diagnose bulk edit failures in production.
🔎 Proposed enhancement with error logging
} catch (error) { // On error, complete bulk operation immediately this.bulkOperationState.completeBulkOperation(); + console.error('Bulk edit operation failed:', error, { entityCount: newEntities.length }); throw error; }As per coding guidelines, Angular services should implement proper error handling and logging.
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts (1)
9-18: Consider adding OnPush change detection strategy.Per coding guidelines, components should use
ChangeDetectionStrategy.OnPushfor performance optimization. Since this component now uses Signals (which work well with OnPush), this would be a good addition.🔎 Proposed fix
+import { Component, inject, Signal, ChangeDetectionStrategy } from "@angular/core"; -import { Component, inject, Signal } from "@angular/core"; import { MAT_DIALOG_DATA, MatDialogModule } from "@angular/material/dialog"; import { MatProgressBarModule } from "@angular/material/progress-bar"; /** * A simple progress indicator dialog * used via the {@link ConfirmationDialogService}. */ @Component({ templateUrl: "./progress-dialog.component.html", imports: [MatProgressBarModule, MatDialogModule], + changeDetection: ChangeDetectionStrategy.OnPush, })src/app/core/entity/entity-actions/entity-actions.service.ts (1)
362-380: Consider usingsaveAllfor consistency with the archive method.The
undoArchivemethod still uses individualsavecalls within aforEach, which doesn't await properly (fire-and-forget). For consistency with the updatedarchivemethod and proper async handling, consider refactoring to usesaveAll.🔎 Proposed fix
async undoArchive<E extends Entity>(entityParam: E | E[]) { let newEntities: E[] = Array.isArray(entityParam) ? entityParam : [entityParam]; const originalEntities: E[] = newEntities.map((e) => e.copy()); - newEntities.forEach(async (e) => { - e.inactive = false; - await this.entityMapper.save(e); - }); + newEntities.forEach((e) => (e.inactive = false)); + + if (newEntities.length > 1) { + this.bulkOperationState.startBulkOperation(newEntities.length); + try { + await this.entityMapper.saveAll(newEntities); + } catch (error) { + this.bulkOperationState.completeBulkOperation(); + throw error; + } + } else { + await this.entityMapper.save(newEntities[0]); + } this.showSnackbarConfirmationWithUndo(src/app/core/entity/entity-actions/bulk-operation-state.service.ts (1)
22-22: Avoidanytype forMatDialogRef.Per coding guidelines, avoid using
any. Consider typing this more specifically.🔎 Proposed fix
+import { ProgressDialogComponent } from "../../common-components/confirmation-dialog/progress-dialog/progress-dialog.component"; ... - private progressDialogRef: MatDialogRef<any> | null = null; + private progressDialogRef: MatDialogRef<ProgressDialogComponent> | null = null;src/app/core/entity/entity-actions/entity-actions.service.spec.ts (1)
48-51: Mock setup is correct; consider adding assertions for bulk operation state calls in archive tests.The mock is properly configured. However, the test for archiving multiple entities (lines 295-310) should verify that
startBulkOperationandcompleteBulkOperationare called appropriately, since the implementation invokes these methods when processing multiple entities.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.htmlsrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/entity-edit.service.ts
🧰 Additional context used
📓 Path-based instructions (18)
**/*.{component,service,directive,guard,interceptor,resolver,pipe}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
inject()function instead of constructor injection for dependencies
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
**/*.service.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.service.ts: UseprovidedIn: 'root'for singleton services in Angular
Implement proper error handling and logging in Angular services
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.ts
**/*.{component,service}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{component,service}.ts: Useupdate()orset()instead ofmutate()when modifying signals in Angular
Create interfaces for configuration objects and let component classes implement them
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Use strict type checking with TypeScript and avoidanytype; useunknownwhen type is uncertain
Prefer type inference in TypeScript when obvious
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js}: Use ESLint for linting (npm run lint)
Use Prettier for code formatting
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
src/app/core/**/*.{service,module}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Core system modules and services belong in
src/app/core/
Files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.ts
**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.component.ts: Use standalone components (default behavior, do NOT setstandalone: true)
Preferinput()andoutput()functions over@Inputand@Outputdecorators in Angular components
Use signals for reactive state management withcomputed()for derived state
SetchangeDetection: ChangeDetectionStrategy.OnPushfor all components
Prefer inline templates for small components in Angular
Prefer Reactive Forms over Template-driven forms in Angular
UseNgOptimizedImagefor static images instead of base64 encoded images
Use Angular Material components for UI consistency following Material Design guidelines
Implement OnPush change detection strategy for performance optimization
Use trackBy functions for lists in Angular templates for performance optimization
Files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
**/*.{component,directive}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{component,directive}.ts: Do NOT use@HostBindingand@HostListenerdecorators; put host bindings inside thehostobject of@Componentor@Directivedecorator
Implement proper permissions checking via CASL integration usingEntityAbilityandDisableEntityOperationDirective
Files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
**/*.component.{ts,html}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.component.{ts,html}: All user-facing strings must be translatable using Angular i18n markers or$localizein Aam Digital
Follow existing translation key patterns in Aam Digital
Use Angular Material accessibility features in components
Files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.htmlsrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
**/*entity*.{service,component}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use proper generics for entity types in TypeScript
Files:
src/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity-list/entity-list/entity-list.component.ts
**/*entity*.service.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use entity services for data access and caching in Aam Digital
Files:
src/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.ts
**/*.component.html
📄 CodeRabbit inference engine (AGENTS.md)
**/*.component.html: Use native control flow (@if,@for,@switch) instead of structural directives (*ngIf,*ngFor,*ngSwitch)
Use class bindings instead ofngClassdirective in Angular templates
Use style bindings instead ofngStyledirective in Angular templates
Keep templates simple and avoid complex logic in Angular templates
Use async pipe for observables in Angular templates instead of manual subscriptions
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.html
**/*.component.{scss,html}
📄 CodeRabbit inference engine (AGENTS.md)
Use global style classes from
src/styles/globals/(e.g.,flex-row,margin-regular) instead of creating new custom styles
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.html
**/*.component.{html,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Follow WCAG guidelines for accessibility in Aam Digital
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.html
src/app/core/common-components/**/*.component.ts
📄 CodeRabbit inference engine (AGENTS.md)
Shared components belong in
src/app/core/common-components/
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
**/*.{component,service}.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests for all new components and services using Jasmine/Karma
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.ts: Mock dependencies properly using Angular testing utilities in unit tests
Use established testing patterns from the Aam Digital project in unit tests
Files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
**/*entity*.spec.ts
📄 CodeRabbit inference engine (AGENTS.md)
Test entity operations with proper data setup/teardown in unit tests
Files:
src/app/core/entity/entity-actions/entity-actions.service.spec.ts
🧠 Learnings (18)
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.{component,service}.ts : Use `update()` or `set()` instead of `mutate()` when modifying signals in Angular
Applied to files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.{component,service,directive,guard,interceptor,resolver,pipe}.ts : Use `inject()` function instead of constructor injection for dependencies
Applied to files:
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.tssrc/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.{ts,html} : All user-facing strings must be translatable using Angular i18n markers or `$localize` in Aam Digital
Applied to files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.htmlsrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.ts : Use signals for reactive state management with `computed()` for derived state
Applied to files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.ts : Use Angular Material components for UI consistency following Material Design guidelines
Applied to files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.htmlsrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.ts : Prefer `input()` and `output()` functions over `Input` and `Output` decorators in Angular components
Applied to files:
src/app/core/admin/admin-overview/admin-overview.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.service.ts : Implement proper error handling and logging in Angular services
Applied to files:
src/app/core/admin/admin-overview/admin-overview.component.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*entity*.service.ts : Use entity services for data access and caching in Aam Digital
Applied to files:
src/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity/entity-actions/bulk-operation-state.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*entity*.spec.ts : Test entity operations with proper data setup/teardown in unit tests
Applied to files:
src/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity/entity-actions/entity-actions.service.tssrc/app/core/entity-list/duplicate-records/duplicate-records.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*entity*.{service,component}.ts : Use proper generics for entity types in TypeScript
Applied to files:
src/app/core/entity/entity-actions/entity-edit.service.tssrc/app/core/entity-list/entity-list/entity-list.component.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.{ts,html} : Use Angular Material accessibility features in components
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.ts : Prefer Reactive Forms over Template-driven forms in Angular
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.html : Use async pipe for observables in Angular templates instead of manual subscriptions
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.tssrc/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.component.ts : Prefer inline templates for small components in Angular
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.spec.ts : Mock dependencies properly using Angular testing utilities in unit tests
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.spec.ts : Use established testing patterns from the Aam Digital project in unit tests
Applied to files:
src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.tssrc/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.{component,service}.spec.ts : Write unit tests for all new components and services using Jasmine/Karma
Applied to files:
src/app/core/entity/entity-actions/entity-actions.service.spec.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to src/app/core/demo-data/**/*.ts : Provide demo data generators for new entities using `faker-js/faker` following existing demo data patterns
Applied to files:
src/app/core/entity/entity-actions/entity-actions.service.spec.ts
🧬 Code graph analysis (1)
src/app/core/entity-list/entity-list/entity-list.component.ts (1)
src/app/core/entity/model/entity-update.ts (1)
UpdatedEntity(7-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Seer Code Review
- GitHub Check: qa / test-unit
- GitHub Check: qa / test-e2e
- GitHub Check: build-webapp
🔇 Additional comments (11)
src/app/core/admin/admin-overview/admin-overview.component.ts (1)
144-146: LGTM! Properly wraps localized message as Signal.The change correctly adapts to the new
showProgressDialogAPI by wrapping the localized string withsignal(). This maintains i18n support while providing reactive message capabilities.src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.html (1)
2-2: LGTM! Template correctly calls signal accessor.The change from
{{ data.message }}to{{ message() }}properly consumes the Signal-based message, enabling reactive updates to the progress dialog title.src/app/core/common-components/confirmation-dialog/progress-dialog/progress-dialog.component.spec.ts (1)
5-5: LGTM! Test properly updated for Signal API.The test correctly provides
message: signal("test title")to match the new component API that expects a Signal. This follows proper Angular testing patterns.Also applies to: 18-18
src/app/core/common-components/confirmation-dialog/confirmation-dialog.service.ts (1)
71-77: LGTM! Signal migration improves reactivity.The change from
message: stringtomessage: Signal<string>enables reactive progress message updates, which is beneficial for bulk operations that need to show dynamic progress. All callers have been properly updated to passSignal<string>arguments (either as wrappingsignal()or passing existingWritableSignalproperties), and signal updates correctly use theset()method rather thanmutate().src/app/core/entity/entity-actions/entity-edit.service.ts (1)
77-81:saveAll()does not automatically update progress; additional instrumentation is needed for tracking.The
EntityMapperService.saveAll()method has no integration withBulkOperationStateService. Progress is only updated when list components receive entity change events and manually callupdateBulkOperationProgress(). The current code starts a bulk operation but relies on external components to track completion—if no listeners are present, the progress dialog will not update or close automatically. Consider either: (1) instrumentingsaveAll()to report progress directly, or (2) documenting that bulk edit operations require active listeners on entity update events for progress tracking to work.src/app/core/entity-list/duplicate-records/duplicate-records.service.ts (1)
27-34: Bulk operation lifecycle properly integrated.The bulk operation state management is correctly implemented with proper error handling. On success, completion is intentionally delegated to the entity-list component which tracks when UI rendering is complete before closing the progress dialog.
src/app/core/entity/entity-actions/entity-actions.service.ts (1)
336-347: Good implementation of bulk save for archive operation.The branching logic correctly uses
saveAllfor multiple entities while preserving the single-entity path for performance. Error handling properly cleans up bulk operation state.src/app/core/entity/entity-actions/bulk-operation-state.service.ts (1)
1-45: Well-structured service for coordinating bulk operations.The service properly:
- Uses
providedIn: 'root'for singleton behavior- Implements
OnDestroyfor cleanup- Exposes both observable (
isBulkOperationInProgress$) and synchronous (isBulkOperationInProgress()) access patterns- Uses signals for reactive progress message updates
src/app/core/entity/entity-actions/entity-actions.service.spec.ts (1)
295-310: Tests properly updated for bulk save behavior.The test assertions correctly verify that
saveAllis called once with all entities instead of multiple individualsavecalls.src/app/core/entity-list/entity-list/entity-list.component.ts (2)
282-298: Bulk operation handling with render-aware completion.The implementation correctly:
- Buffers individual updates during bulk operations to avoid UI performance issues
- Reloads all entities once the expected count is reached
- Uses
setTimeout+requestAnimationFrameto ensure UI rendering is complete before closing the progress dialogThis addresses the timing issue mentioned in past reviews where the progress bar would close before the table finished rendering.
46-46: Good typing improvement.Importing and using
UpdatedEntity<T>instead ofanyfor the callback parameter aligns with strict typing guidelines.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/app/core/entity/entity-actions/bulk-operation-state.service.ts:
- Line 22: Replace the use of the unsafe any type for the dialog reference:
change the declaration of progressDialogRef from MatDialogRef<any> | null to
MatDialogRef<unknown> | null (keeping the | null) so the code uses stricter
typing while still allowing .close() to be called; update any imports/usages
referencing progressDialogRef if necessary to reflect the new generic type.
- Around line 37-45: startBulkOperation currently allows concurrent starts; add
a guard at the top of startBulkOperation to no-op (or return/throw) if an
operation is already in progress by checking operationInProgress (or the
existing progressDialogRef). If you decide to allow restart, first close the
orphaned dialogRef (if progressDialogRef exists/has close) and clear counters
before creating a new dialog; otherwise simply return early to prevent
overwriting expectedUpdateCount, processedUpdateCount, and progressDialogRef.
Ensure you reference startBulkOperation, operationInProgress, progressDialogRef,
confirmationDialog.showProgressDialog, expectedUpdateCount,
processedUpdateCount, and updateProgressDialog in your change so the logic and
dialog lifecycle are handled safely.
🧹 Nitpick comments (1)
src/app/core/entity/entity-actions/bulk-operation-state.service.ts (1)
37-45: Consider adding error handling for dialog operations.If
showProgressDialogthrows an exception, the service state becomes inconsistent (operationInProgressistruebutprogressDialogRefisnull). Wrapping the dialog call in a try-catch block would improve robustness.🔎 Proposed refactor
startBulkOperation(expectedCount?: number) { this.expectedUpdateCount = expectedCount || 0; this.processedUpdateCount = 0; this.operationInProgress.next(true); this.updateProgressDialog(); - this.progressDialogRef = this.confirmationDialog.showProgressDialog( - this.progressDialogMessage, - ); + try { + this.progressDialogRef = this.confirmationDialog.showProgressDialog( + this.progressDialogMessage, + ); + } catch (error) { + console.error('Failed to show progress dialog', error); + this.completeBulkOperation(); + } }Based on coding guidelines, implement proper error handling in Angular services.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{component,service,directive,guard,interceptor,resolver,pipe}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use
inject()function instead of constructor injection for dependencies
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
**/*.service.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.service.ts: UseprovidedIn: 'root'for singleton services in Angular
Implement proper error handling and logging in Angular services
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
**/*.{component,service}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{component,service}.ts: Useupdate()orset()instead ofmutate()when modifying signals in Angular
Create interfaces for configuration objects and let component classes implement them
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Use strict type checking with TypeScript and avoidanytype; useunknownwhen type is uncertain
Prefer type inference in TypeScript when obvious
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
src/**/*.{ts,js}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,js}: Use ESLint for linting (npm run lint)
Use Prettier for code formatting
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
src/app/core/**/*.{service,module}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Core system modules and services belong in
src/app/core/
Files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
🧠 Learnings (2)
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*entity*.service.ts : Use entity services for data access and caching in Aam Digital
Applied to files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
📚 Learning: 2025-11-26T16:20:52.119Z
Learnt from: CR
Repo: Aam-Digital/ndb-core PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T16:20:52.119Z
Learning: Applies to **/*.{component,service}.ts : Use `update()` or `set()` instead of `mutate()` when modifying signals in Angular
Applied to files:
src/app/core/entity/entity-actions/bulk-operation-state.service.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Seer Code Review
- GitHub Check: qa / test-unit
- GitHub Check: build-webapp
- GitHub Check: qa / test-e2e
🔇 Additional comments (5)
src/app/core/entity/entity-actions/bulk-operation-state.service.ts (5)
1-19: LGTM: Service setup follows Angular best practices.The service correctly uses
providedIn: 'root'for singleton behavior and theinject()function for dependency injection, as per the coding guidelines.
77-86: LGTM: Getter methods are straightforward and correct.The getter methods properly expose internal state for external monitoring of bulk operation progress.
91-95: LGTM: Progress dialog update uses correct signal API.The method correctly uses
set()to update the signal, following Angular best practices. The previous console.log statement has been removed.
103-111: LGTM: Cleanup logic is thorough.The method properly resets all state variables and ensures the dialog reference is closed and nulled, preventing resource leaks.
113-119: LGTM: Proper lifecycle cleanup.The
ngOnDestroyhook correctly completes the BehaviorSubject and ensures any in-progress operation is cleaned up, following Angular best practices for service cleanup.
sleidig
left a comment
sleidig
left a comment
There was a problem hiding this comment.
Works super fast now 🤩
Code looks good also. Great job 👍
|
🎉 This PR is included in version 3.68.0-master.11 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
by a batch save and avoiding intermittent reloads of the list closes #2843 --------- Co-authored-by: Sebastian <[email protected]>
|
🎉 This PR is included in version 3.68.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR Checklist
Before you request a manual PR review from someone, please go through all of this list, check the points and mark them checked here.
This helps us reduce the number of review cycles and use the time of our core reviewers more effectively.
ng lint) passesng test) passes⏪ if issues are commented from testing or code review, make changes. Please then re-check every item of the checklist here again.
Remaining TODOs:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.