cleanup and error handler activity#3975
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve refactoring the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (1)
Let me analyze the findings from the script execution to generate the final response.
Update required: Multiple calls to
RunBusinessFlowneed the new parameterSeveral calls to
RunBusinessFlowin test files and implementation code are missing the newdoResetErrorHandlerExecutedFlagparameter:
GingerCoreNET/RosLynLib/GingerScriptGlobals.cs: Method signature needs updateGingerTest/Variable Dependancies/VariableDependancyTest.cs: Missing parameter in two callsGingerCoreNETUnitTest/RunTestslib/GingerRunnerTest.cs: Missing parameterGingerCoreNETUnitTest/RunListeners/ExecutionListenerTest.cs: Missing parameterGingerCoreNETUnitTest/Reports/NewHTMLReportTest.cs: Missing parameterGingerCoreNETUnitTest/RunListeners/RunnerListenerTest.cs: Missing parameterGingerCoreNETUnitTest/GingerRunnerTests/CleanUpActivityTests.cs: Missing parameter in multiple callsGingerCoreNETUnitTest/GingerRunnerTests/ErrorHandlerActivityTest.cs: Missing parameter in multiple callsThe interface
IGingerExecutionEnginecorrectly defines the parameter, but several implementations need to be updated to maintain consistency.🔗 Analysis chain
Line range hint
2104-2104: Verify all calls toRunBusinessFloware updated with the new parameterThe method signature of
RunBusinessFlowhas been updated to include the new parameterbool doResetErrorHandlerExecutedFlag = false. Please ensure that all calls toRunBusinessFlowthroughout the codebase are updated accordingly to include this parameter, and that the default value does not introduce any unintended behavior.Run the following script to identify calls to
RunBusinessFlowand verify the correct usage of the new parameter:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to RunBusinessFlow and check for the new parameter. # Expected result: All calls to RunBusinessFlow include the 'doResetErrorHandlerExecutedFlag' parameter. rg --type cs --multiline --pcre2 'RunBusinessFlow\(\s*[^\)]*' -A 1Length of output: 5871
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs (3 hunks)
- Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Codacy Static Code Analysis
Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs
[warning] 190-190: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L190
Remove this commented out code.
[warning] 195-195: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L195
Remove this commented out code.
🔇 Additional comments (4)
Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs (2)
24-24: LGTM: Well-structured event handling with proper memory management.The implementation follows good practices:
- Uses WeakEventManager to prevent memory leaks
- Comprehensive cleanup in ClearBindings
- Clear event documentation
Also applies to: 31-31, 33-33
209-224: Verify null handling in event handlers.The code should handle potential null references when accessing properties and methods. Consider adding null checks before accessing:
actscollectionContextproperty- Event handlers
Ginger/GingerCoreNET/Run/GingerExecutionEngine.cs (2)
Line range hint
2155-2159: Ensure proper handling ofdoResetErrorHandlerExecutedFlaginRunActivitycallsIn the
RunBusinessFlowmethod, the calls toRunActivitynow pass theresetErrorHandlerExecutedFlagparameter. Please verify that theresetErrorHandlerExecutedFlagis being used correctly inRunActivityand that it aligns with the intended control flow, especially in scenarios involving error handling and activity retries.
4485-4485:⚠️ Potential issueRemove extraneous line
4485~There is an unnecessary line
4485~in the code. It appears to be a leftover from code annotations or line numbering. Please remove it to maintain code cleanliness and readability.Apply this diff to remove the extraneous line:
Likely invalid or redundant comment.
| else if (ItemObject is Activity activity) //.GetType() == typeof(GingerCore.Activity) || ItemObject.GetType() == typeof(CleanUpActivity) || ItemObject.GetType() == typeof(ErrorHandler)) | ||
| { | ||
| GingerCore.GeneralLib.BindingHandler.ObjFieldBinding(xStatus, StatusItem.StatusProperty, ItemObject, nameof(Activity.Status), BindingMode.OneWay); | ||
| PropertyChangedEventManager.AddHandler(source: ((Activity)ItemObject), handler: RunnerItem_ActivityPropertyChanged, propertyName: allProperties); | ||
| PropertyChangedEventManager.AddHandler(source: activity, handler: RunnerItem_ActivityPropertyChanged, propertyName: allProperties); |
There was a problem hiding this comment.
Remove inline comment containing old code.
The pattern matching improvement is good, but the inline comment containing the old code should be removed for better readability.
Apply this diff:
- else if (ItemObject is Activity activity) //.GetType() == typeof(GingerCore.Activity) || ItemObject.GetType() == typeof(CleanUpActivity) || ItemObject.GetType() == typeof(ErrorHandler))
+ else if (ItemObject is Activity activity)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (ItemObject is Activity activity) //.GetType() == typeof(GingerCore.Activity) || ItemObject.GetType() == typeof(CleanUpActivity) || ItemObject.GetType() == typeof(ErrorHandler)) | |
| { | |
| GingerCore.GeneralLib.BindingHandler.ObjFieldBinding(xStatus, StatusItem.StatusProperty, ItemObject, nameof(Activity.Status), BindingMode.OneWay); | |
| PropertyChangedEventManager.AddHandler(source: ((Activity)ItemObject), handler: RunnerItem_ActivityPropertyChanged, propertyName: allProperties); | |
| PropertyChangedEventManager.AddHandler(source: activity, handler: RunnerItem_ActivityPropertyChanged, propertyName: allProperties); | |
| else if (ItemObject is Activity activity) | |
| { | |
| GingerCore.GeneralLib.BindingHandler.ObjFieldBinding(xStatus, StatusItem.StatusProperty, ItemObject, nameof(Activity.Status), BindingMode.OneWay); | |
| PropertyChangedEventManager.AddHandler(source: activity, handler: RunnerItem_ActivityPropertyChanged, propertyName: allProperties); |
| else if (ItemObject is Activity activity) | ||
| { | ||
| IEnumerable<GingerCore.Actions.Act> acts = null; | ||
| //Activity activity = null; | ||
| //if (ItemObject.GetType() == typeof(Activity)) | ||
| //{ | ||
| acts = ((Activity)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | ||
| activity = (Activity)ItemObject; | ||
| //} | ||
|
|
||
| //if (ItemObject.GetType() == typeof(CleanUpActivity)) | ||
| //{ | ||
| // acts = ((CleanUpActivity)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | ||
| // activity = (CleanUpActivity)ItemObject; | ||
| //} | ||
|
|
||
| //if (ItemObject.GetType() == typeof(ErrorHandler)) | ||
| //{ | ||
| // acts = ((ErrorHandler)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | ||
| // activity = (ErrorHandler)ItemObject; | ||
| //} | ||
|
|
There was a problem hiding this comment.
Clean up commented code blocks and simplify type casting.
The code has been improved with pattern matching, but there are a few cleanup items needed:
- Remove the commented code blocks as they're no longer needed and add noise.
- Remove the redundant cast since we've already pattern matched with
is.
Apply this diff to clean up the code:
else if (ItemObject is Activity activity)
{
- IEnumerable<GingerCore.Actions.Act> acts = null;
- //Activity activity = null;
- //if (ItemObject.GetType() == typeof(Activity))
- //{
- acts = ((Activity)ItemObject).Acts.OfType<GingerCore.Actions.Act>();
- activity = (Activity)ItemObject;
- //}
-
- //if (ItemObject.GetType() == typeof(CleanUpActivity))
- //{
- // acts = ((CleanUpActivity)ItemObject).Acts.OfType<GingerCore.Actions.Act>();
- // activity = (CleanUpActivity)ItemObject;
- //}
-
- //if (ItemObject.GetType() == typeof(ErrorHandler))
- //{
- // acts = ((ErrorHandler)ItemObject).Acts.OfType<GingerCore.Actions.Act>();
- // activity = (ErrorHandler)ItemObject;
- //}
-
+ var acts = activity.Acts.OfType<GingerCore.Actions.Act>();
if (acts != null)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else if (ItemObject is Activity activity) | |
| { | |
| IEnumerable<GingerCore.Actions.Act> acts = null; | |
| //Activity activity = null; | |
| //if (ItemObject.GetType() == typeof(Activity)) | |
| //{ | |
| acts = ((Activity)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | |
| activity = (Activity)ItemObject; | |
| //} | |
| //if (ItemObject.GetType() == typeof(CleanUpActivity)) | |
| //{ | |
| // acts = ((CleanUpActivity)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | |
| // activity = (CleanUpActivity)ItemObject; | |
| //} | |
| //if (ItemObject.GetType() == typeof(ErrorHandler)) | |
| //{ | |
| // acts = ((ErrorHandler)ItemObject).Acts.OfType<GingerCore.Actions.Act>(); | |
| // activity = (ErrorHandler)ItemObject; | |
| //} | |
| else if (ItemObject is Activity activity) | |
| { | |
| var acts = activity.Acts.OfType<GingerCore.Actions.Act>(); | |
| if (acts != null) | |
| { |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 190-190: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L190
Remove this commented out code.
[warning] 195-195: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L195
Remove this commented out code.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs (3 hunks)
🧰 Additional context used
🪛 GitHub Check: Codacy Static Code Analysis
Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs
[warning] 190-190: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L190
Remove this commented out code.
[warning] 195-195: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L195
Remove this commented out code.
| IEnumerable<GingerCore.Actions.Act> acts = null; | ||
| if (acts != null) | ||
| { | ||
| RunnerItemPage ri = new RunnerItemPage(Runnerobj: act, runnerItemEventHandler: _runnerItemEventHandler); | ||
| this.Context.Activity = (Activity)ItemObject; | ||
| ri.Context = this.Context; | ||
| act.Context = this.Context; | ||
| ri.xItemSeparator.Visibility = Visibility.Collapsed; | ||
| ri.ItemName = act.Description; | ||
| ri.ItemGuid = act.Guid; | ||
| ri.xViewRunnerItem.Visibility = Visibility.Visible; | ||
| ri.xDetailView.ToolTip = "Expand / Collapse Action"; | ||
| mItemChilds.Add(ri); | ||
|
|
||
| foreach (GingerCore.Actions.Act act in acts) | ||
| { | ||
| RunnerItemPage ri = new RunnerItemPage(Runnerobj: act, runnerItemEventHandler: _runnerItemEventHandler); | ||
| this.Context.Activity = activity; | ||
| ri.Context = this.Context; | ||
| act.Context = this.Context; | ||
| ri.xItemSeparator.Visibility = Visibility.Collapsed; | ||
| ri.ItemName = act.Description; | ||
| ri.ItemGuid = act.Guid; | ||
| ri.xViewRunnerItem.Visibility = Visibility.Visible; | ||
| ri.xDetailView.ToolTip = "Expand / Collapse Action"; | ||
| mItemChilds.Add(ri); | ||
| } |
There was a problem hiding this comment.
Fix logic error: acts is always null, causing actions not to load
In the LoadChildRunnerItems method, the variable acts is initialized to null and is not assigned any value before the null check. This means the condition if (acts != null) will always be false, and the subsequent foreach loop will never execute. As a result, the actions associated with the activity will not be loaded.
Apply this diff to fix the issue:
- IEnumerable<GingerCore.Actions.Act> acts = null;
- if (acts != null)
+ var acts = activity.Acts.OfType<GingerCore.Actions.Act>();
+ if (acts.Any())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| IEnumerable<GingerCore.Actions.Act> acts = null; | |
| if (acts != null) | |
| { | |
| RunnerItemPage ri = new RunnerItemPage(Runnerobj: act, runnerItemEventHandler: _runnerItemEventHandler); | |
| this.Context.Activity = (Activity)ItemObject; | |
| ri.Context = this.Context; | |
| act.Context = this.Context; | |
| ri.xItemSeparator.Visibility = Visibility.Collapsed; | |
| ri.ItemName = act.Description; | |
| ri.ItemGuid = act.Guid; | |
| ri.xViewRunnerItem.Visibility = Visibility.Visible; | |
| ri.xDetailView.ToolTip = "Expand / Collapse Action"; | |
| mItemChilds.Add(ri); | |
| foreach (GingerCore.Actions.Act act in acts) | |
| { | |
| RunnerItemPage ri = new RunnerItemPage(Runnerobj: act, runnerItemEventHandler: _runnerItemEventHandler); | |
| this.Context.Activity = activity; | |
| ri.Context = this.Context; | |
| act.Context = this.Context; | |
| ri.xItemSeparator.Visibility = Visibility.Collapsed; | |
| ri.ItemName = act.Description; | |
| ri.ItemGuid = act.Guid; | |
| ri.xViewRunnerItem.Visibility = Visibility.Visible; | |
| ri.xDetailView.ToolTip = "Expand / Collapse Action"; | |
| mItemChilds.Add(ri); | |
| } | |
| var acts = activity.Acts.OfType<GingerCore.Actions.Act>(); | |
| if (acts.Any()) | |
| { | |
| foreach (GingerCore.Actions.Act act in acts) | |
| { | |
| RunnerItemPage ri = new RunnerItemPage(Runnerobj: act, runnerItemEventHandler: _runnerItemEventHandler); | |
| this.Context.Activity = activity; | |
| ri.Context = this.Context; | |
| act.Context = this.Context; | |
| ri.xItemSeparator.Visibility = Visibility.Collapsed; | |
| ri.ItemName = act.Description; | |
| ri.ItemGuid = act.Guid; | |
| ri.xViewRunnerItem.Visibility = Visibility.Visible; | |
| ri.xDetailView.ToolTip = "Expand / Collapse Action"; | |
| mItemChilds.Add(ri); | |
| } |
🧰 Tools
🪛 GitHub Check: Codacy Static Code Analysis
[warning] 190-190: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L190
Remove this commented out code.
[warning] 195-195: Ginger/Ginger/RunSetPageLib/RunnerItemPage.xaml.cs#L195
Remove this commented out code.
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor