[NAE-2232] Frontend Actions Task events resolver#305
Conversation
…/7.0.0-rev9 # Conflicts: # package.json # projects/netgrif-components-core/package.json # projects/netgrif-components/package.json
- add FrontActionService integration to event task services for handling front actions
WalkthroughAssign, cancel, and finish task services now inject FrontActionService and execute parsed frontActions from operation outcomes via FrontActionService.runAll after changed-fields emission; constructor signatures updated. task-data.service.ts condition for triggering front actions was relaxed using optional chaining to avoid undefined errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant Service as Assign/Cancel/Finish Service
participant Backend
participant FrontAction as FrontActionService
UI->>Service: trigger task operation
Service->>Backend: perform operation (assign/cancel/finish)
Backend-->>Service: outcome (changedFields?, frontActions?)
Service->>Service: emit changedFields (if any)
alt frontActions present
Service->>FrontAction: runAll(frontActions)
FrontAction-->>Service: done
else no frontActions
Note over Service: no front actions to run
end
Service->>UI: continue reload/notification/close flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts(3 hunks)projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts(3 hunks)projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
⏰ 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). (17)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Matrix Test (24)
- GitHub Check: Matrix Test (20)
- GitHub Check: Matrix Test (22)
- GitHub Check: Test with SonarCloud
🔇 Additional comments (9)
projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts (3)
26-30: LGTM! Imports are correct.The imports for
EventService,FrontActionService, andFrontActionare properly added and necessary for the new functionality.
53-55: LGTM! Constructor injection follows Angular patterns.The
FrontActionServiceis correctly injected as a protected member, consistent with other service injections in this constructor.
142-151: No change needed: error path handling consistentError path in cancel-task.service.ts matches assign-task and finish-task services: front actions are only executed on success. No changes required.
projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts (4)
27-28: LGTM!The imports are correctly organized and reference the appropriate modules.
47-47: LGTM!The FrontActionService is properly injected following Angular's dependency injection pattern, consistent with other service dependencies in the constructor.
127-128: LGTM!The parsing logic follows the same pattern as the changed fields parsing above it (lines 125-126), maintaining consistency in how outcome tree data is extracted.
132-135: Wrap FrontActionService.runAll in error handling and confirm error-case behavior
- Surround
runAll(frontActions)with a try–catch (or verify it internally swallows exceptions) to ensure reload/notification logic always runs.- Verify whether error outcomes should execute front actions—CancelTaskService and FinishTaskService omit them on error.
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (2)
27-28: LGTM! Imports are correctly added.The import statements for
FrontActionServiceandFrontActionare properly structured and necessary for the new functionality.
49-50: LGTM! Constructor correctly updated.The
FrontActionServiceinjection follows the established pattern and is properly stored as a protected member for use in the class methods.
| const frontActions: Array<FrontAction> = this._eventService.parseFrontActionsFromOutcomeTree(outcomeResource.outcome); | ||
|
|
||
|
|
||
| if (!!changedFieldsMap) { | ||
| this._changedFieldsService.emitChangedFields(changedFieldsMap); | ||
| } | ||
|
|
||
| if (!!frontActions && frontActions.length > 0) { | ||
| this._frontActionService.runAll(frontActions); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify error handling and consider using optional chaining.
The front actions parsing and execution logic is correct. However, consider the following:
-
Error handling: Verify that
FrontActionService.runAll()handles errors internally. If it can throw exceptions, wrap the call in a try-catch block to prevent disrupting the cancel flow. -
Optional refactor: The null check can be modernized using optional chaining:
- if (!!frontActions && frontActions.length > 0) {
+ if (frontActions?.length > 0) {
this._frontActionService.runAll(frontActions);
}Run the following script to verify error handling in FrontActionService:
🏁 Script executed:
#!/bin/bash
# Verify error handling in FrontActionService.runAll method
ast-grep --pattern $'class FrontActionService {
$$$
runAll($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate FrontActionService definition
rg -n "class FrontActionService" -g "*.ts"
# Inspect runAll implementation for error handling
file=$(rg -l "class FrontActionService" -g "*.ts")
rg -nP 'runAll\s*\(' -C3 "$file"Length of output: 297
Wrap runAll in error handling and modernize null check
- Wrap the call to
this._frontActionService.runAll(frontActions)in atry { … } catch { … }block to ensure any thrown errors don’t interrupt the cancel flow. - Replace
if (!!frontActions && frontActions.length > 0)withif (frontActions?.length > 0).
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts
around lines 129 to 138, replace the double-negation null check with optional
chaining (if (frontActions?.length > 0)) and wrap the call to
this._frontActionService.runAll(frontActions) in a try { ... } catch (err) { ...
} block so any thrown errors do not break the cancel flow; in the catch block
log the error using the service's existing logger (or console.error if no logger
exists) and do not rethrow so processing continues.
- updates from pr agent
- updates from pr agent
- updates from pr agent
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (1)
141-148: Previous review concerns remain unaddressed.The earlier review flagged that
runAll()lacks error handling and may be asynchronous. The current implementation still:
- Missing error handling: If
runAll()throws, it will interrupt the finish flow (reload/close operations won't execute).- Unclear async behavior: If
runAll()performs async operations, the current fire-and-forget approach may cause race conditions with subsequent reload/close.Please verify the
FrontActionService.runAllimplementation to confirm:
- Whether it's synchronous or returns a Promise
- How it handles internal errors
Based on the findings:
- If synchronous: wrap the call in try-catch to prevent exceptions from blocking task finish
- If asynchronous: make this method async and await the call, with try-catch around it
#!/bin/bash # Description: Examine FrontActionService.runAll signature and error handling # Find FrontActionService implementation file=$(fd -e ts FrontActionService | grep -v spec | grep -v test) if [ -z "$file" ]; then echo "FrontActionService file not found" exit 1 fi echo "=== FrontActionService location ===" echo "$file" echo "" # Display the runAll method signature and implementation echo "=== runAll method implementation ===" rg -nP -A 20 '(public|protected|private)?\s*(async\s+)?runAll\s*\(' "$file"projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts (1)
134-136: GuardrunAllto keep the cancel flow resilient.If any front action throws, the subscription will blow up before we resolve the queue, leaving the cancel flow stuck. Wrap the call in a
try/catch, log via_log, and continue so reload/notifications still run.if (frontActions?.length > 0) { - this._frontActionService.runAll(frontActions); + try { + this._frontActionService.runAll(frontActions); + } catch (err) { + this._log.error('Failed to execute front actions for cancel outcome', err); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts(3 hunks)projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts(3 hunks)projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts(3 hunks)projects/netgrif-components-core/src/lib/task/services/task-data.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
projects/netgrif-components-core/src/lib/task/services/cancel-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (1)
projects/netgrif-components-core/src/lib/data-fields/models/changed-fields.ts (1)
FrontAction(48-53)
⏰ 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). (9)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (5)
projects/netgrif-components-core/src/lib/task/services/finish-task.service.ts (2)
27-28: LGTM!The imports are correctly specified and necessary for the front action integration.
49-50: LGTM!The constructor correctly injects
FrontActionServicefollowing Angular's dependency injection pattern. The protected visibility is appropriate for use within class methods.projects/netgrif-components-core/src/lib/task/services/task-data.service.ts (1)
65-65: LGTM! FrontActionService injection added correctly.The FrontActionService dependency is properly injected following Angular's dependency injection pattern and is used appropriately in the
processSuccessfulSetDataRequestmethod to execute front actions from task outcomes.projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts (2)
27-28: LGTM! Imports are correct.The imports for
FrontActionServiceandFrontActionare appropriate and necessary for the new front-action handling functionality.
47-47: LGTM! Constructor injection follows existing patterns.The
FrontActionServiceis correctly injected as a protected dependency, consistent with other service injections in this constructor.
| const frontActions: Array<FrontAction> = this._eventService.parseFrontActionsFromOutcomeTree(outcomeResource.outcome); | ||
|
|
||
| if (!!changedFieldsMap) { | ||
| this._changedFieldsService.emitChangedFields(changedFieldsMap); | ||
| } | ||
| if (frontActions?.length > 0) { | ||
| this._frontActionService.runAll(frontActions); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error handling and consistency with error path.
The front-action execution logic is well-structured with safe optional chaining on line 132. However, two aspects need verification:
-
Error handling: Confirm that
FrontActionService.runAllhandles errors internally and won't throw unhandled exceptions that could disrupt the assign workflow. -
Error path consistency: The error path (lines 145-148) emits changed fields but does not parse or execute front actions. Verify this is intentional—should front actions also be executed when
outcomeResource.erroris true but an outcome is present?
Run the following script to examine the error handling in FrontActionService.runAll:
🏁 Script executed:
#!/bin/bash
# Description: Check FrontActionService.runAll implementation for error handling
# Find and display the runAll method implementation
ast-grep --pattern $'class FrontActionService {
$$$
runAll($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate the FrontActionService class declaration
rg -nP 'class\s+FrontActionService' -C3 -g '*.ts'
# Locate the runAll method within the codebase
rg -nP '\brunAll\s*\(' -C3 -g '*.ts'Length of output: 5238
🏁 Script executed:
#!/bin/bash
# Display FrontActionService implementation
sed -n '1,200p' projects/netgrif-components-core/src/lib/actions/services/front-action.service.ts
# Locate run method definition
rg -nP 'public\s+run\(' -C3 -g projects/netgrif-components-core/src/lib/actions/services/front-action.service.ts
# Display assign-task.service.ts around error path
sed -n '120,200p' projects/netgrif-components-core/src/lib/task/services/assign-task.service.tsLength of output: 5347
Add error handling to front action execution and ensure error path consistency
- FrontActionService.runAll/run does not catch exceptions or guard against undefined handlers—wrap each call in try/catch and return early if
get(...)returns no function to prevent uncaught errors. - In assign-task.service.ts error branch (L145–148), frontActions aren’t parsed or executed—confirm whether front actions should also run when
outcomeResource.erroris true.
🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/task/services/assign-task.service.ts
around lines 127 to 134, update front action execution to be defensive and add
consistent error-path handling: change the runAll invocation so each front
action call is checked for a valid handler (return early or skip if get(...)
yields undefined) and wrap each handler invocation in try/catch to prevent
exceptions from bubbling; additionally, in the error branch around lines 145–148
parse frontActions from outcomeResource.outcome the same way and execute them
(with the same guarded try/catch/handler-existence checks) so front actions are
consistently processed whether outcomeResource.error is true or false.
| if (frontActions?.length > 0) { | ||
| this._frontActionService.runAll(frontActions); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
LGTM! Optional chaining provides safe front action execution.
The optional chaining operator correctly handles cases where parseFrontActionsFromOutcomeTree may return undefined or null, preventing potential runtime errors. The execution order (after changed-fields emission, before cleanup) aligns with the integration pattern described in the PR objectives.
To confirm the optional chaining is necessary, verify whether parseFrontActionsFromOutcomeTree can return undefined or null:
🏁 Script executed:
#!/bin/bash
# Description: Check the return type and implementation of parseFrontActionsFromOutcomeTree
# Search for the method definition and its return type
ast-grep --pattern $'parseFrontActionsFromOutcomeTree($$$) {
$$$
}'
# Also search for the method signature/declaration
rg -nP --type=ts -A 10 'parseFrontActionsFromOutcomeTree\s*\(' | head -50Length of output: 6399
Remove unnecessary optional chaining on frontActions (task-data.service.ts:492–494)
parseFrontActionsFromOutcomeTree always returns an array, so frontActions is never undefined or null. Change
if (frontActions?.length > 0) {
this._frontActionService.runAll(frontActions);
}to
if (frontActions.length > 0) {
this._frontActionService.runAll(frontActions);
}🤖 Prompt for AI Agents
In projects/netgrif-components-core/src/lib/task/services/task-data.service.ts
around lines 492–494, remove the unnecessary optional chaining on frontActions
because parseFrontActionsFromOutcomeTree always returns an array; change the
conditional to check frontActions.length > 0 and call
this._frontActionService.runAll(frontActions) when true so the code uses a
direct length check instead of frontActions?.length.
machacjozef
changed the base branch from
release/7.0.0-rev9
to
release/7.0.0-rev8
|
5 participants
Description
Add FrontActionService integration to event task services for handling front actions
Implements NAE-2232
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?
Manually
Test Configuration
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes