[NAE-2416] AbstractFileDefaultFieldComponent does not push upload event#326
Open
renczesstefan wants to merge 1 commit intorelease/7.0.0-rev10from
Open
[NAE-2416] AbstractFileDefaultFieldComponent does not push upload event#326renczesstefan wants to merge 1 commit intorelease/7.0.0-rev10from
renczesstefan wants to merge 1 commit intorelease/7.0.0-rev10from
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.
WalkthroughFile upload components now integrate FrontActionService to execute frontend actions parsed from upload responses. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileComponent as File Upload Component
participant FrontActionService
participant Backend API
participant SnackBar
User->>FileComponent: Upload file
FileComponent->>Backend API: POST file
Backend API-->>FileComponent: Return response with frontActions
FileComponent->>FileComponent: Parse frontActions from response
alt Has frontActions
FileComponent->>FrontActionService: Execute parsed frontActions
FrontActionService->>SnackBar: Trigger snackBar action (if applicable)
SnackBar-->>User: Display notification
else No frontActions
FileComponent->>SnackBar: Show success snackbar
SnackBar-->>User: Display success message
end
alt Response has error
FileComponent->>SnackBar: Show error snackbar
SnackBar-->>User: Display error message
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
Comment on lines
232
to
+241
| if (response.error) { | ||
| this._snackbar.openErrorSnackBar(this._translate.instant(response.error)); | ||
| this.state.error = true; | ||
| this._log.error( | ||
| `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)} uploading has failed!`, response.error | ||
| ); | ||
| if (response.error) { | ||
| this._snackbar.openErrorSnackBar(this._translate.instant(response.error)); | ||
| } else { | ||
| this._snackbar.openErrorSnackBar(this._translate.instant('dataField.snackBar.fileUploadFailed')); | ||
| } |
There was a problem hiding this comment.
Dead code: inner if (response.error) is always true.
Line 232 already guards response.error being truthy, so the nested if (response.error) on line 237 cannot be false and the else fallback to dataField.snackBar.fileUploadFailed is unreachable. Either drop the inner branch or replace the outer guard with a different condition (e.g. check for a truthy outcome/response shape) so the fallback is actually reachable.
🛠️ Proposed fix
if (response.error) {
this.state.error = true;
this._log.error(
`File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)} uploading has failed!`, response.error
);
- if (response.error) {
- this._snackbar.openErrorSnackBar(this._translate.instant(response.error));
- } else {
- this._snackbar.openErrorSnackBar(this._translate.instant('dataField.snackBar.fileUploadFailed'));
- }
+ this._snackbar.openErrorSnackBar(this._translate.instant(response.error));
} else {🤖 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 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).
Comment on lines
+234
to
+236
| this._log.error( | ||
| `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)} uploading has failed!`, response.error | ||
| ); |
There was a problem hiding this comment.
Error logs print File object instead of file name.
this.fileUploadEl.nativeElement.files.item(0) returns a File reference that renders as [object File] in most log sinks; the success-path log at line 246 correctly uses .name. Make the failure logs consistent so operators can identify which file failed.
🛠️ Proposed fix
- this._log.error(
- `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)} uploading has failed!`, response.error
- );
+ this._log.error(
+ `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)?.name} uploading has failed!`, response.error
+ );- this._log.error(
- `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)} uploading has failed!`, error
- );
+ this._log.error(
+ `File [${this.dataField.stringId}] ${this.fileUploadEl.nativeElement.files.item(0)?.name} uploading has failed!`, error
+ );Also applies to: 273-275
🤖 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 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.
Comment on lines
+258
to
+261
| const frontActions: Array<FrontAction> = this._eventService.parseFrontActionsFromOutcomeTree(response.outcome); | ||
| if (frontActions?.length > 0) { | ||
| this._frontActionService.runAll(frontActions); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the service still dereferences fn unconditionally.
rg -nP -C3 'public\s+run\s*\(' --type=ts -g '**/front-action.service.ts'
# And check parseFrontActionsFromOutcomeTree return contract.
ast-grep --pattern 'parseFrontActionsFromOutcomeTree($_) { $$$ }'Repository: netgrif/components
Length of output: 891
🏁 Script executed:
# Get the full implementation of the run() method and surrounding context
cat -n projects/netgrif-components-core/src/lib/actions/services/front-action.service.ts | head -30Repository: netgrif/components
Length of output: 1156
🏁 Script executed:
# Also check the runAll() method to understand the call pattern
rg -nP -A 10 'runAll\s*\(' --type=ts -g '**/front-action.service.ts'Repository: netgrif/components
Length of output: 596
🏁 Script executed:
# Check the context around the runAll call in the file-field component
rg -nP -B5 -A5 'runAll\s*\(' --type=ts -g '**/abstract-file-default-field.component.ts'Repository: netgrif/components
Length of output: 2295
Fix unhandled TypeError when unregistered action IDs are in upload outcome.
In FrontActionService.run() (projects/netgrif-components-core/src/lib/actions/services/front-action.service.ts:15-21), the if-guard logs an error when the action ID is missing but lacks a return statement, allowing execution to proceed to fn.call(...) with undefined. When parseFrontActionsFromOutcomeTree() returns an entry with an unknown ID, runAll() iterates through and invokes the faulty run(), throwing TypeError: Cannot read properties of undefined (reading 'call') inside the subscriber after the upload succeeds. Add a return statement in the if-block to prevent this unhandled exception.
🤖 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 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.
Comment on lines
+178
to
183
| this.state.completed = true; | ||
| this.state.uploading = false; | ||
| this.state.progress = 0; | ||
| this._log.debug( | ||
| `Files [${this.dataField.stringId}] were successfully uploaded` | ||
| ); |
There was a problem hiding this comment.
Premature success log: "successfully uploaded" is emitted before response.error is inspected.
_log.debug('Files ... were successfully uploaded') runs unconditionally on every non-progress response, including error outcomes handled just below. This pollutes logs with false success entries and can mislead incident triage. Move the debug log into the success branch (after line 197 where state.error = false).
🛠️ Proposed fix
} else {
this.state.completed = true;
this.state.uploading = false;
this.state.progress = 0;
- this._log.debug(
- `Files [${this.dataField.stringId}] were successfully uploaded`
- );
if (response.error) {
@@
} else {
const changedFieldsMap: ChangedFieldsMap = this._eventService.parseChangedFieldsFromOutcomeTree(response.outcome);
this.dataField.emitChangedFields(changedFieldsMap);
this.state.error = false;
+ this._log.debug(
+ `Files [${this.dataField.stringId}] were successfully uploaded`
+ );
filesToUpload.forEach(fileToUpload => {🤖 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-list-field/file-list-default-field/abstract-file-list-default-field.component.ts`
around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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