Network Log fixed#4035
Conversation
WalkthroughThe changes in this pull request focus on 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 (3)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (3)
793-803: Optimize network request logging for performanceSerializing the entire
requestobject may include unnecessary data and affect performance. Consider logging only the essential properties needed for debugging or monitoring purposes to enhance efficiency.
Line range hint
830-850: Ensure complete data retrieval in network response loggingIn the
OnNetworkResponseReceivedmethod, to avoid incomplete data, consider awaiting the full response content before serialization. Also, handle potentialNullReferenceExceptionby ensuring thatresponse.Requestis not null.Apply this diff to improve data handling:
await response.FinishedAsync(); if (response != null) { + var responseBody = await response.BodyAsync(); if ((_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl(response.Url)) && response.Request != null) { // Existing code... } }
767-777: Safely detach event handlers to prevent exceptionsEnsure that event handlers are detached safely by checking if they have been attached before attempting to detach them. This can prevent potential exceptions during the cleanup process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(9 hunks)
| if (act.Timeout is not null || act.Timeout != 0) | ||
| { | ||
| timeout = act.Timeout.Value; | ||
| } | ||
| st.Start(); | ||
| while (timeout != st.Elapsed.TotalSeconds) | ||
| { | ||
| if (networkRequestLogList.Count == networkResponseLogList.Count) | ||
| { | ||
| break; | ||
| } | ||
| System.Threading.Thread.Sleep(1000); | ||
| } | ||
| st.Stop(); | ||
| } |
There was a problem hiding this comment.
Fix logical errors in timeout handling and loop condition
There are issues in the timeout handling logic:
-
The condition
if (act.Timeout is not null || act.Timeout != 0)will always evaluate to true. You should useif (act.Timeout.HasValue && act.Timeout.Value != 0)to correctly check ifact.Timeouthas a value other than zero. -
Calling
st.Start()is unnecessary sinceStopwatch.StartNew()starts the stopwatch immediately. -
In the
whileloop, comparingtimeout != st.Elapsed.TotalSecondsis unreliable due to floating-point precision. Usewhile (st.Elapsed.TotalSeconds < timeout)instead to ensure the loop exits accurately when the timeout is reached.
Apply this diff to fix the issues:
Stopwatch st = Stopwatch.StartNew();
- if (act.Timeout is not null || act.Timeout != 0)
+ if (act.Timeout.HasValue && act.Timeout.Value != 0)
{
timeout = act.Timeout.Value;
}
- st.Start();
- while (timeout != st.Elapsed.TotalSeconds)
+ while (st.Elapsed.TotalSeconds < timeout)
{
if (networkRequestLogList.Count == networkResponseLogList.Count)
{
break;
}
System.Threading.Thread.Sleep(1000);
}📝 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.
| if (act.Timeout is not null || act.Timeout != 0) | |
| { | |
| timeout = act.Timeout.Value; | |
| } | |
| st.Start(); | |
| while (timeout != st.Elapsed.TotalSeconds) | |
| { | |
| if (networkRequestLogList.Count == networkResponseLogList.Count) | |
| { | |
| break; | |
| } | |
| System.Threading.Thread.Sleep(1000); | |
| } | |
| st.Stop(); | |
| } | |
| Stopwatch st = Stopwatch.StartNew(); | |
| if (act.Timeout.HasValue && act.Timeout.Value != 0) | |
| { | |
| timeout = act.Timeout.Value; | |
| } | |
| while (st.Elapsed.TotalSeconds < timeout) | |
| { | |
| if (networkRequestLogList.Count == networkResponseLogList.Count) | |
| { | |
| break; | |
| } | |
| System.Threading.Thread.Sleep(1000); | |
| } | |
| st.Stop(); | |
| } |
| networkRequestLogList = []; | ||
| networkResponseLogList = []; |
There was a problem hiding this comment.
Incorrect initialization of lists
Assigning [] to networkRequestLogList and networkResponseLogList is invalid in C#. Lists should be initialized using new List<Tuple<string, object>>().
Apply this diff to fix the issue:
-networkRequestLogList = [];
-networkResponseLogList = [];
+networkRequestLogList = new List<Tuple<string, object>>();
+networkResponseLogList = new List<Tuple<string, object>>();📝 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.
| networkRequestLogList = []; | |
| networkResponseLogList = []; | |
| networkRequestLogList = new List<Tuple<string, object>>(); | |
| networkResponseLogList = new List<Tuple<string, object>>(); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
793-811: Refactor to eliminate code duplication in request loggingThe code within the
ifandelseblocks is almost identical. Consider refactoring to reduce duplication and improve readability.You can refactor the code as follows:
if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl(request.Url)) { bool shouldLog = true; if (_act.GetOrCreateInputParam(nameof(ActBrowserElement.eRequestTypes)).Value == ActBrowserElement.eRequestTypes.FetchOrXHR.ToString()) { shouldLog = request.ResourceType.Equals("XHR", StringComparison.CurrentCultureIgnoreCase) || request.ResourceType.Equals("FETCH", StringComparison.CurrentCultureIgnoreCase); } if (shouldLog) { networkRequestLogList.Add(new Tuple<string, object>( $"RequestUrl:{request.Url}", JsonConvert.SerializeObject(request, Formatting.Indented, new JsonSerializerSettings { ReferenceLoopHandling = ReferenceLoopHandling.Ignore }))); } }
Line range hint
830-851: Refactor to eliminate code duplication in response loggingSimilar to the request logging, the code in the
ifandelseblocks is nearly identical. Refactor to reduce duplication.Here's a suggested refactoring:
await response.FinishedAsync(); if (response != null) { if (_BrowserHelper.ShouldMonitorAllUrls() || _BrowserHelper.ShouldMonitorUrl(response.Url)) { bool shouldLog = true; if (_act.GetOrCreateInputParam(nameof(ActBrowserElement.eRequestTypes)).Value == ActBrowserElement.eRequestTypes.FetchOrXHR.ToString()) { shouldLog = response.Request.ResourceType.Equals("XHR", StringComparison.CurrentCultureIgnoreCase) || response.Request.ResourceType.Equals("FETCH", StringComparison.CurrentCultureIgnoreCase); } if (shouldLog) { networkResponseLogList.Add(new Tuple<string, object>( $"ResponseUrl:{response.Url}", JsonConvert.SerializeObject(response, Formatting.Indented, new JsonSerializerSettings { ReferenceLoopHandling = ReferenceLoopHandling.Ignore }))); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs(9 hunks)
🔇 Additional comments (2)
Ginger/GingerCoreNET/Drivers/CoreDrivers/Web/Playwright/PlaywrightBrowserTab.cs (2)
635-636:
Correct the initialization of lists
Assigning [] to networkRequestLogList and networkResponseLogList is invalid in C#. You should initialize them using new List<Tuple<string, object>>().
Apply this diff to fix the issue:
-networkRequestLogList = [];
-networkResponseLogList = [];
+networkRequestLogList = new List<Tuple<string, object>>();
+networkResponseLogList = new List<Tuple<string, object>>();707-722:
Remove unnecessary Start() call and fix loop condition
- The call to
st.Start()is unnecessary sinceStopwatch.StartNew()starts the stopwatch immediately. - Comparing
timeout != st.Elapsed.TotalSecondsis unreliable due to floating-point precision. Usewhile (st.Elapsed.TotalSeconds < timeout)instead to ensure accurate timeout handling.
Apply this diff to fix the issues:
Stopwatch st = Stopwatch.StartNew();
- st.Start();
- while (timeout != st.Elapsed.TotalSeconds)
+ while (st.Elapsed.TotalSeconds < timeout)
{
if (networkRequestLogList.Count == networkResponseLogList.Count)
{
break;
}
System.Threading.Thread.Sleep(1000);
}
2 participants
Thank you for your contribution.
Before submitting this PR, please make sure:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation