Skip to content

[dotnet] [bidi] Add OnHistoryUpdated event #15916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 19, 2025
Merged

[dotnet] [bidi] Add OnHistoryUpdated event #15916

merged 3 commits into from
Jun 19, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jun 19, 2025 •
edited by qodo-merge-pro 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-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    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-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    qodo-merge-pro 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
    Member Author

    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
    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