[NAE-2416] AbstractFileDefaultFieldComponent does not push upload event#326
Merged
Conversation
…nt notification to TaskEventService - Integrated `FrontActionService` in `AbstractFileDefaultFieldComponent` to handle front-end actions based on outcomes. - Enhanced file upload logic with better error handling and front action triggers, such as displaying success feedback via snackbar actions. - Updated tests for `AbstractFileListDefaultFieldComponent` to include `FrontActionService`. - Registered a new `snackBar` front action in the `FrontActionModule`. - Refactored the file upload methods to utilize modern RXJS subscription patterns with error handling improvements. - Improved readability and maintainability of file download and preview logic by addressing structuring and formatting issues. - Removed unused imports and simplified test components. These changes aim to improve the user experience by handling outcome-based front actions and offering better feedback on file operations.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFile and file-list upload components receive FrontActionService; upload handlers use object-style RxJS subscriptions (next/error), parse and run frontActions from backend outcomes, centralize success/error snackbar handling, and tests/mocks are updated to provide FrontActionService and simulate upload/download flows. ChangesFile upload front-action and tests
Sequence Diagram(s)sequenceDiagram
participant User
participant FileComponent as File Upload Component
participant Backend as Backend API
participant FrontActions as FrontActionService
participant SnackBar
User->>FileComponent: upload file(s)
FileComponent->>Backend: POST file(s)
Backend-->>FileComponent: progress events / final response (outcome/frontActions/message/error)
alt Progress events
FileComponent->>FileComponent: update progress state
end
alt Success (complete)
FileComponent->>FileComponent: finalize state, update uploadedFiles/value, emit changed fields
alt frontActions present
FileComponent->>FrontActions: runAll(parsed frontActions)
FrontActions->>SnackBar: execute snackBar action (if present)
SnackBar-->>User: display notification
else no frontActions
FileComponent->>SnackBar: show success message
SnackBar-->>User: display notification
end
else Error
FileComponent->>FileComponent: set error state, reset input
FileComponent->>SnackBar: show translated backend error or fallback message
SnackBar-->>User: display error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts (1)
93-102: 🧹 Nitpick | 🔵 TrivialMinor: align local parameter order with the base class.
Local params declare
sanitizerbeforeeventService, but the parent class expects them in the reverse order (and thesuper(...)call correctly passes them in parent order by name). The code works, but mirroring the parent's parameter order in this test subclass avoids confusion and matches theFileDefaultFieldComponentconventions.♻️ Proposed reorder
class TestFileComponent extends AbstractFileDefaultFieldComponent { constructor(taskResourceService: TaskResourceService, log: LoggerService, snackbar: SnackBarService, translate: TranslateService, - sanitizer: DomSanitizer, eventService: EventService, + sanitizer: DomSanitizer, frontActionService: FrontActionService, `@Optional`() `@Inject`(DATA_FIELD_PORTAL_DATA) dataFieldPortalData: DataFieldPortalData<FileField>) { super(taskResourceService, log, snackbar, translate, eventService, sanitizer, frontActionService, dataFieldPortalData); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts` around lines 93 - 102, The constructor parameter order in AbstractFileDefaultFieldComponentSpec is inconsistent with the base class: swap the local parameters so eventService comes before sanitizer to mirror the parent class signature; update the constructor parameter list (constructor(... eventService: EventService, sanitizer: DomSanitizer ...)) while leaving the super(...) call unchanged so the parameter order matches FileDefaultFieldComponent and avoids confusion.projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts (1)
113-125:⚠️ Potential issue | 🟠 MajorBreaking change in public constructor signature requires migration documentation.
AbstractFileDefaultFieldComponentis exported from@netgrif/components-coreas public API. The constructor signature changed by inserting_frontActionServicein the middle of the parameter list (after_sanitizer, beforedataFieldPortalData). External subclasses not recompiled against this version will receive the wrong argument at thedataFieldPortalDataposition and fail at runtime.Internal subclasses (
FileDefaultFieldComponent,TestFileComponent) have been updated correctly. However, the CHANGELOG contains no documentation of this breaking change, and no migration guide has been created. Consider either:
- Appending
_frontActionServiceat the end (before@Optional() dataFieldPortalData)- Marking it
@Optional()to retain backward compatibilityAt minimum, document this breaking change in the next release notes for consumers updating from earlier versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts` around lines 113 - 125, The public constructor of AbstractFileDefaultFieldComponent was changed by inserting _frontActionService before dataFieldPortalData which breaks external subclasses; to fix, restore backward compatibility by making the new parameter optional or moving it to the end: either annotate the _frontActionService parameter with `@Optional`() (so existing callers still pass DATA_FIELD_PORTAL_DATA into dataFieldPortalData) or append _frontActionService after the existing parameters but before dataFieldPortalData, ensuring DATA_FIELD_PORTAL_DATA (dataFieldPortalData) stays last; update the constructor signature in AbstractFileDefaultFieldComponent and keep references to _frontActionService, DATA_FIELD_PORTAL_DATA and dataFieldPortalData consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts`:
- Around line 234-236: The failure log is printing the File object instead of
its name; update the calls to this._log.error in the upload-failure paths (the
one using this.dataField.stringId and
this.fileUploadEl.nativeElement.files.item(0) and the similar block around lines
273-275) to reference the file's name property (use
this.fileUploadEl.nativeElement.files.item(0).name) so the log matches the
success-path format and operators see the actual filename; keep the same
message/context and still pass response.error as the second argument to
this._log.error.
- Around line 232-241: The outer if checks response.error and the inner if
(response.error) is dead code; remove the nested check and ensure the snackbar
fallback is reachable by using a single conditional: inside the existing block
where response.error is truthy call this._log.error(...) with response.error and
then call
this._snackbar.openErrorSnackBar(this._translate.instant(response.error ||
'dataField.snackBar.fileUploadFailed')); update references to response.error,
this._log.error, this._snackbar.openErrorSnackBar and this._translate to
implement this single check (keep fileUploadEl/nativeElement and
this.dataField.stringId unchanged).
- Around line 258-261: FrontActionService.run currently logs an error when an
action ID is not registered but does not return, causing fn to be undefined and
a TypeError when fn.call(...) is invoked; update FrontActionService.run so that
inside the if-block that checks for missing action (the block that logs the
error) you add an early return to exit the method and avoid calling fn.call,
ensuring runAll() and subscribers won’t throw when
parseFrontActionsFromOutcomeTree yields unknown IDs.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`:
- Around line 178-183: The debug message "Files [...] were successfully
uploaded" is logged unconditionally on non-progress responses, causing false
success logs; move the _log.debug(...) call into the success branch so it only
runs when response.error is falsy—specifically place it after the code that sets
this.state.error = false (the success handling block in the poll/response
handler inside abstract-file-list-default-field component), so that only
successful responses trigger the debug log and error responses do not.
---
Outside diff comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts`:
- Around line 93-102: The constructor parameter order in
AbstractFileDefaultFieldComponentSpec is inconsistent with the base class: swap
the local parameters so eventService comes before sanitizer to mirror the parent
class signature; update the constructor parameter list (constructor(...
eventService: EventService, sanitizer: DomSanitizer ...)) while leaving the
super(...) call unchanged so the parameter order matches
FileDefaultFieldComponent and avoids confusion.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts`:
- Around line 113-125: The public constructor of
AbstractFileDefaultFieldComponent was changed by inserting _frontActionService
before dataFieldPortalData which breaks external subclasses; to fix, restore
backward compatibility by making the new parameter optional or moving it to the
end: either annotate the _frontActionService parameter with `@Optional`() (so
existing callers still pass DATA_FIELD_PORTAL_DATA into dataFieldPortalData) or
append _frontActionService after the existing parameters but before
dataFieldPortalData, ensuring DATA_FIELD_PORTAL_DATA (dataFieldPortalData) stays
last; update the constructor signature in AbstractFileDefaultFieldComponent and
keep references to _frontActionService, DATA_FIELD_PORTAL_DATA and
dataFieldPortalData consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 577ae8e8-25c9-4e4c-9839-9902477a95e4
📒 Files selected for processing (7)
projects/netgrif-components-core/src/lib/actions/front-action.module.tsprojects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.tsprojects/netgrif-components/src/lib/data-fields/file-field/file-default-field/file-default-field.component.tsprojects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.ts
Included FrontActionService in the test setup for file-list-default-field and file-default-field components. This ensures proper dependency injection and facilitates testing of features relying on FrontActionService.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.spec.ts (1)
41-45:⚠️ Potential issue | 🟠 MajorImport
FrontActionModuleinstead of providingFrontActionServicedirectly.The
FrontActionModuleconstructor registers thesnackBaraction (and others) intoFrontActionRegistryService. ProvidingFrontActionServicein isolation skips this registration step, leaving the registry empty and causing test failures when the upload handler executes front actions. Import the module to ensure the test environment mirrors runtime action setup:♻️ Proposed change
imports: [ MaterialModule, AngularResizeEventModule, BrowserAnimationsModule, HttpClientTestingModule, TranslateLibModule, - SnackBarModule + SnackBarModule, + FrontActionModule ], providers: [ SideMenuService, - FrontActionService, {provide: AuthenticationMethodService, useClass: MockAuthenticationMethodService},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@projects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.spec.ts` around lines 41 - 45, The test currently provides FrontActionService directly which bypasses FrontActionModule's constructor that registers actions into FrontActionRegistryService; replace the provider entry for FrontActionService with importing FrontActionModule in the TestBed configuration so the module's constructor runs and the snackBar/upload front actions are registered before running file-list-default-field tests (look for FrontActionModule, FrontActionService, and FrontActionRegistryService in the spec to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@projects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.spec.ts`:
- Around line 41-45: The test currently provides FrontActionService directly
which bypasses FrontActionModule's constructor that registers actions into
FrontActionRegistryService; replace the provider entry for FrontActionService
with importing FrontActionModule in the TestBed configuration so the module's
constructor runs and the snackBar/upload front actions are registered before
running file-list-default-field tests (look for FrontActionModule,
FrontActionService, and FrontActionRegistryService in the spec to update).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d62c190e-cd6f-43bb-9707-5a91b12a2a92
📒 Files selected for processing (2)
projects/netgrif-components/src/lib/data-fields/file-field/file-default-field/file-default-field.component.spec.tsprojects/netgrif-components/src/lib/data-fields/file-list-field/file-list-default-field/file-list-default-field.component.spec.ts
Retoocs
requested changes
Apr 27, 2026
Reorganized logging to ensure success messages are logged after error checks and refined error handling logic for file uploads. Added a null-safe access to file names and fixed missing return statement in front-action error handling.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.ts`:
- Around line 255-257: parseFrontActionsFromOutcomeTree currently only inspects
response.outcome.outcomes and therefore misses actions attached to the root;
when building the actions to execute before calling
this._frontActionService.runAll(frontActions), include any
response.outcome.frontActions as well (either by updating
parseFrontActionsFromOutcomeTree to include root frontActions or by merging
response.outcome.frontActions into the frontActions array here) so that
root-level upload actions are not skipped; reference
parseFrontActionsFromOutcomeTree, response.outcome.frontActions, and
_frontActionService.runAll to locate and apply the change.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`:
- Around line 223-225: The error log currently interpolates the File object
causing “[object File]” in logs; update the _log.error call in
abstract-file-list-default-field.component (where it references
this.dataField.stringId and this.fileUploadEl.nativeElement.files.item(0)) to
log the filename instead by using the file's name property (e.g., use
this.fileUploadEl.nativeElement.files.item(0)?.name) and include the error as
the second argument.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c35979e-3714-4756-bcfe-ee716647f5a7
📒 Files selected for processing (3)
projects/netgrif-components-core/src/lib/actions/services/front-action.service.tsprojects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts
Refined file upload error logging and improved event parsing logic to handle frontActions. Added unit tests to verify file upload and download methods, ensuring better component reliability. Updated test dependencies to support new functionality.
Added missing imports for AbstractFileListFieldComponent and invalid data token to the test file. Updated the test component's template to include a file input element with binding to allowed file types.
Implemented tests to verify that the download and upload methods are called successfully. These tests improve the coverage and ensure the methods function as expected.
Implemented tests to verify that the download and upload methods are called successfully. These tests improve the coverage and ensure the methods function as expected.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.ts`:
- Around line 83-93: The tests are tautological because they spyOn
component.download and component.upload and then call those same methods;
instead, remove callThrough and instead stub the underlying dependencies and
assert observable side effects: replace spyOn(component,
'download').and.callThrough() with spying/stubbing the service or utility that
download() delegates to (e.g., fileService.downloadFile or httpClient.get) and
assert that when you call component.download() it triggers that dependency with
correct args and handles success and error paths (mock success
observable/Promise and mock error to verify error handling); do the same for
component.upload() by spying on the upload dependency (e.g.,
fileService.uploadFile or formSubmit) and assert successful response handling
and error handling rather than asserting the method call itself.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.ts`:
- Around line 83-93: The tests currently only spy on component.download and
component.upload and then call them, which doesn't verify behavior; replace
these self-fulfilling assertions by spying on the observable side-effects of
those methods (e.g., the file service methods, HTTP calls, snackbar/dialog
interactions, or component state flags) so download and upload are exercised but
asserted via dependencies: for download, spyOn the file download service or
window.open/anchor trigger that download() should call and assert it was invoked
with the "test" argument (or that a download URL was set); for upload, spyOn the
upload/fileService.upload method or the component's isUploading/isDisabled flags
and assert the service call and the state change or snackbar notification; keep
using component.download and component.upload to invoke behavior but assert on
the external service calls or state changes rather than on the methods
themselves.
In
`@projects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`:
- Around line 183-185: The error log accesses
this.fileUploadEl.nativeElement.files.item(0).name which can throw if item(0) is
null; update the error path in the upload failure handler (the call to
this._log.error) to safely read the filename (e.g. const filename =
this.fileUploadEl?.nativeElement?.files?.item(0)?.name ?? '<unknown>' ) and use
that filename variable and the response.error when calling this._log.error so
null files do not cause an additional exception.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb9b8ad8-1692-496a-9396-b927b947e6ad
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/data-fields/file-field/file-default-field/abstract-file-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.spec.tsprojects/netgrif-components-core/src/lib/data-fields/file-list-field/file-list-default-field/abstract-file-list-default-field.component.tsprojects/netgrif-components-core/src/lib/event/services/event.service.ts
Retoocs
requested changes
Apr 30, 2026
Updated the code to handle cases where the file name might be undefined during error logging. This change prevents runtime errors caused by null references when accessing the file list.
Simplifies the error handling logic by removing an unnecessary null check for `response.error`. This ensures the snackbar displays error messages consistently without redundant conditionals.
Extended `MockTaskResourceService` with additional file methods to simulate file uploads and downloads. Improved test coverage in `AbstractFileListDefaultFieldComponent` and `AbstractFileDefaultFieldComponent` with refined mock implementations and more detailed file operation simulations. Added new dependencies for test files.
Retoocs
previously approved these changes
May 18, 2026
machacjozef
previously approved these changes
May 18, 2026
Reordered the parsing and emission of changed fields after a successful file upload. This ensures consistent execution flow and enhances code clarity.
Retoocs
approved these changes
May 19, 2026
|
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
4 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.
Description
Implements NAE-2416
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
There are no dependencies on other PR.
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests