Skip to content

[dotnet] [bidi] Add OnHistoryUpdated event#15916

Merged
nvborisenko merged 3 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-historyupdated
Jun 19, 2025
Merged

[dotnet] [bidi] Add OnHistoryUpdated event#15916
nvborisenko merged 3 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-historyupdated

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko commented Jun 19, 2025 •
edited by qodo-code-review bot
Loading

User description

https://w3c.github.io/webdriver-bidi/#event-browsingContext-historyUpdated

💥 What does this PR do?

Implement the event.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

• Add OnHistoryUpdated event for BiDi browsing context
• Implement async handlers for history navigation tracking
• Create HistoryUpdatedEventArgs record class

Changes walkthrough 📝

Relevant files
Enhancement
BrowsingContext.cs
Add history updated event handlers                                             

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs

• Add OnHistoryUpdatedAsync method with Func handler
• Add
OnHistoryUpdatedAsync method with Action handler

 +10/-0   
HistoryUpdatedEventArgs.cs
Create HistoryUpdatedEventArgs record class                           

dotnet/src/webdriver/BiDi/BrowsingContext/HistoryUpdatedEventArgs.cs

• Create new record class for history updated events
• Include BiDi,
Context, Timestamp, and Url properties
• Inherit from
BrowsingContextEventArgs base class

 +25/-0   
Bug fix
BrowsingContextModule.cs
Implement history updated subscription methods                     

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs

• Add OnHistoryUpdatedAsync method with async Func handler
• Add
OnHistoryUpdatedAsync method with Action handler
• Both methods
subscribe to browsingContext.fragmentNavigated event

 +10/-0   
Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 19, 2025
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jun 19, 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

    Wrong Event

    The OnHistoryUpdatedAsync methods are subscribing to "browsingContext.fragmentNavigated" event instead of "browsingContext.historyUpdated". This appears to be a copy-paste error from the OnFragmentNavigatedAsync methods above.

     
        return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
}

public async Task<Subscription> OnHistoryUpdatedAsync(Action<HistoryUpdatedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null)
{
    return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
    Missing Documentation

    The HistoryUpdatedEventArgs record class lacks XML documentation comments explaining its purpose and parameters, which is inconsistent with typical .NET documentation standards.

     
    public record HistoryUpdatedEventArgs(BiDi BiDi, BrowsingContext Context, DateTimeOffset Timestamp, string Url)
    : BrowsingContextEventArgs(BiDi, Context);

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jun 19, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    ✅ Fix incorrect event subscription name
    Suggestion Impact:The suggestion was directly implemented - both OnHistoryUpdatedAsync methods were updated to subscribe to the correct "browsingContext.historyUpdated" event instead of the incorrect "browsingContext.fragmentNavigated" event

    code diff:

    -        return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
+        return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false);
     }
 
     public async Task<Subscription> OnHistoryUpdatedAsync(Action<HistoryUpdatedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null)
     {
-        return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
+        return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false);

    The OnHistoryUpdatedAsync methods are subscribing to the wrong event name. They
    should subscribe to "browsingContext.historyUpdated" instead of
    "browsingContext.fragmentNavigated" to match their intended functionality.

    dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs [137-145]

     public async Task<Subscription> OnHistoryUpdatedAsync(Func<HistoryUpdatedEventArgs, Task> handler, BrowsingContextsSubscriptionOptions? options = null)
 {
-    return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
+    return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false);
 }
 
 public async Task<Subscription> OnHistoryUpdatedAsync(Action<HistoryUpdatedEventArgs> handler, BrowsingContextsSubscriptionOptions? options = null)
 {
-    return await Broker.SubscribeAsync("browsingContext.fragmentNavigated", handler, options).ConfigureAwait(false);
+    return await Broker.SubscribeAsync("browsingContext.historyUpdated", handler, options).ConfigureAwait(false);
 }

    [Suggestion processed]

    Suggestion importance[1-10]: 10

    __

    Why: The suggestion correctly identifies a critical bug. The OnHistoryUpdatedAsync methods subscribe to the wrong event name, "browsingContext.fragmentNavigated", which would cause the new feature to not work as intended. The proposed fix to use "browsingContext.historyUpdated" is correct and essential for the functionality.

    		High
    • Update

    @nvborisenko
    Copy link
    Copy Markdown
    Member Author

    nvborisenko commented Jun 19, 2025

    Ci is failing not related to this PR.

    @nvborisenko nvborisenko merged commit 15a7a10 into SeleniumHQ:trunk Jun 19, 2025
    8 of 9 checks passed
    @nvborisenko nvborisenko deleted the bidi-historyupdated branch June 19, 2025 19:11
    @navin772 navin772 mentioned this pull request Aug 22, 2025
    Merged
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    C-dotnet .NET Bindings Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @selenium-ci