Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented May 28, 2025

User description

🔗 Related Issues

Fixes #15561

💥 What does this PR do?

🔧 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

  • Make DTO constructor parameters nullable where allowed by spec

  • Update BiDi event and data records to accept null values

  • Add missing properties to network request data records

  • Improve type safety for optional BiDi fields


Changes walkthrough 📝

Relevant files
Enhancement
11 files
BrowsingContextInfo.cs
Make Children and OriginalOpener parameters nullable in
BrowsingContextInfo
+1/-2     
NavigateCommand.cs
Allow Navigation parameter to be nullable in NavigateResult
+1/-1     
NavigationInfo.cs
Make Navigation parameter nullable in NavigationInfo         
+1/-1     
LogEntry.cs
Allow Text parameter to be nullable in all LogEntry records
+4/-4     
AuthRequiredEventArgs.cs
Make Context and Navigation parameters nullable in
AuthRequiredEventArgs
+1/-1     
BaseParametersEventArgs.cs
Make Context and Navigation parameters nullable in
BaseParametersEventArgs
+1/-1     
BeforeRequestSentEventArgs.cs
Make Context and Navigation parameters nullable in
BeforeRequestSentEventArgs
+1/-1     
FetchErrorEventArgs.cs
Make Context and Navigation parameters nullable in FetchErrorEventArgs
+1/-1     
RequestData.cs
Add Destination and InitiatorType, make HeadersSize nullable in
RequestData
+1/-1     
ResponseCompletedEventArgs.cs
Make Context and Navigation parameters nullable in
ResponseCompletedEventArgs
+1/-1     
ResponseStartedEventArgs.cs
Make Context and Navigation parameters nullable in
ResponseStartedEventArgs
+1/-1     

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 May 28, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15561 - Fully compliant

    Compliant requirements:

    • Review all entries in the BiDi spec marked with "/ null" and make appropriate types nullable in .NET implementation
    • Update DTO constructor parameters to be nullable where allowed by the spec
    • Ensure proper type safety for optional BiDi fields
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    New Properties

    The PR adds new properties to RequestData record (Destination and InitiatorType) but there's no explanation about their purpose or where they come from in the spec.

    public record RequestData(Request Request, string Url, string Method, IReadOnlyList<Header> Headers, IReadOnlyList<Cookie> Cookies, long? HeadersSize, long? BodySize, string Destination, string? InitiatorType, FetchTimingInfo Timings);

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 28, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: [dotnet] [bidi] Reevaluate nullable parameters in DTO constructors

    2 participants