Skip to content

Conversation

visagang
Copy link
Contributor

@visagang visagang commented Aug 8, 2025

PR Type

Enhancement


Description

  • Add GetCurrentUrl method to web browser interface

  • Implement URL retrieval functionality for Playwright driver


Diagram Walkthrough

flowchart LR
  A["IWebBrowser Interface"] --> B["Add GetCurrentUrl method"]
  B --> C["PlaywrightWebDriver Implementation"]
  C --> D["Return current page URL"]
Loading

File Walkthrough

Relevant files
Enhancement
IWebBrowser.cs
Add GetCurrentUrl method to interface                                       

src/Infrastructure/BotSharp.Abstraction/Browsing/IWebBrowser.cs

  • Add GetCurrentUrl method signature to interface
+1/-0     
PlaywrightWebDriver.ExtractData.cs
Implement GetCurrentUrl for Playwright driver                       

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.ExtractData.cs

  • Implement GetCurrentUrl method for Playwright driver
  • Return current page URL or empty string if page is null
+7/-0     

Copy link

qodo-merge-pro bot commented Aug 8, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Interface Consistency

Ensure all existing IWebBrowser implementations are updated to include and correctly implement the new GetCurrentUrl signature to avoid runtime DI/compile errors.

    Task<string> GetCurrentUrl(MessageInfo message);
}
Async Pattern

Consider making GetCurrentUrl synchronous or truly async; it currently returns Task but performs no awaits. If the interface requires async, use Task.FromResult or ensure awaits if future changes are expected.

public async Task<string> GetCurrentUrl(MessageInfo message)
{
    var page = _instance.GetPage(message.ContextId);
    if (page == null)
        return string.Empty;
    return page.Url;
}
Null Handling

When page is null, returning empty string may hide issues; consider returning null, throwing a specific exception, or logging a warning to aid diagnostics.

public async Task<string> GetCurrentUrl(MessageInfo message)
{
    var page = _instance.GetPage(message.ContextId);
    if (page == null)
        return string.Empty;
    return page.Url;
}

Copy link

qodo-merge-pro bot commented Aug 8, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Remove unnecessary async and add guard
Suggestion Impact:The method signature was changed to remove the async modifier, aligning with the suggestion to avoid unnecessary async overhead. The try-catch and Task.FromResult changes are not reflected in the shown diff.

code diff:

-    public async Task<string> GetCurrentUrl(MessageInfo message)
+    public string GetCurrentUrl(MessageInfo message)

The method is declared async but contains no await, which can introduce
unnecessary state machine overhead and warnings. Also, accessing page.Url may
throw if the page is disposed; wrap in a try-catch and return an empty string on
failure to align with the null-path behavior.

src/Plugins/BotSharp.Plugin.WebDriver/Drivers/PlaywrightDriver/PlaywrightWebDriver.ExtractData.cs [20-26]

-public async Task<string> GetCurrentUrl(MessageInfo message)
+public Task<string> GetCurrentUrl(MessageInfo message)
 {
-    var page = _instance.GetPage(message.ContextId);
-    if (page == null)
-        return string.Empty;
-    return page.Url;
+    try
+    {
+        var page = _instance.GetPage(message.ContextId);
+        if (page == null) return Task.FromResult(string.Empty);
+        return Task.FromResult(page.Url ?? string.Empty);
+    }
+    catch
+    {
+        return Task.FromResult(string.Empty);
+    }
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the async keyword is unnecessary and improves robustness by adding a try-catch block to prevent potential exceptions when accessing page.Url.

Medium
Keep interface-implementation consistency

Ensure the interface contract matches the non-async implementation to avoid
hidden async state machines. If the implementation is synchronous, define the
method to return Task via completed tasks without the async modifier for
consistency and performance.

src/Infrastructure/BotSharp.Abstraction/Browsing/IWebBrowser.cs [33]

+Task<string> GetCurrentUrl(MessageInfo message);
 
-
  • Apply / Chat
Suggestion importance[1-10]: 2

__

Why: The suggestion is not actionable as the improved_code is identical to the existing_code; it only confirms that the current interface definition is correct.

Low
  • Update

@Oceania2018 Oceania2018 merged commit 018df7e into SciSharp:master Aug 8, 2025
4 checks passed
visagang pushed a commit to visagang/BotSharp that referenced this pull request Aug 12, 2025
Add function to get browser url for Playwright
hchen2020 added a commit to Lessen-LLC/BotSharp that referenced this pull request Aug 12, 2025
Merge pull request SciSharp#1122 from visagang/features/vguruparan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants