Skip to content

Conversation

@a-gubskiy
Copy link
Member

@a-gubskiy a-gubskiy commented Aug 18, 2025

This pull request focuses on improving code documentation and refining project configuration for the X.PagedList libraries. The main changes include extensive XML documentation for public APIs, updates to project metadata, and minor adjustments to supported frameworks and dependencies.

These changes enhance the maintainability and usability of the codebase for both library users and contributors.

Summary by CodeRabbit

  • New Features

    • Expanded AJAX options: confirmations, loading indicators, lifecycle callbacks, caching, and URL support.
    • Pager: partial-view rendering option and a Go-To-Page form.
    • New TagBuilder helper extensions for richer HTML generation.
  • Refactor

    • Modularized pager rendering; pager now accepts nullable input and can suppress output.
    • HtmlAttribute properties are init-only for immutable initialization.
  • Chores

    • Version bumped to 10.5.8; packaging metadata, icon, signing, and nullable references enabled.
    • Removed .NET 7 target; test/framework package updates; annotation package removed.
  • Documentation

    • Added XML documentation across public APIs.

@a-gubskiy a-gubskiy requested a review from Copilot August 18, 2025 06:36
@coderabbitai
Copy link

coderabbitai bot commented Aug 18, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Caution

Review failed

The pull request is closed.

Walkthrough

Updated project metadata (version, signing, packaging, nullable, icon), removed JetBrains.Annotations usages/[PublicAPI] attributes, narrowed target frameworks, expanded MVC pager internals and AJAX options, added TagBuilder factory/extensions, made HtmlAttribute properties init-only, and applied mainly formatting/test project dependency updates.

Changes

Cohort / File(s) Summary of changes
Repository metadata
src/Directory.Build.props
Enabled packing (IsPackable); bumped Version/AssemblyVersion/FileVersion/PackageVersion 10.5.3 → 10.5.8; updated copyright year to 2025; added PackageProjectUrl and PackageIcon; enabled SignAssembly and AssemblyOriginatorKeyFile; enabled Nullable.
Target frameworks & package refs
src/X.PagedList.EF/X.PagedList.EF.csproj, src/X.PagedList.Mvc.Core/X.PagedList.Mvc.Core.csproj, src/X.PagedList/X.PagedList.csproj, tests/X.PagedList.Tests/X.PagedList.Tests.csproj
Removed net7.0 (and netstandard2.1 in core lib); adjusted EF Core dependency groups and ranges; removed JetBrains.Annotations package references; test project targets net8.0 only and test SDK/runner bumped.
Removed annotations ([PublicAPI])
multiple files: src/X.PagedList.*/*.cs, src/X.PagedList.Mvc.Core/*.cs, src/X.PagedList.EF/PagedListExtensions.cs
Stripped JetBrains.Annotations using directives and [PublicAPI] attributes across many core and MVC files; no behavioral changes.
Ajax configuration expansion
src/X.PagedList.Mvc.Core/AjaxOptions.cs
Added public properties (UpdateTargetId, Confirm, LoadingElementDuration, LoadingElementId, OnBegin, OnComplete, OnFailure, OnSuccess, Url, AllowCache) and updated ToUnobtrusiveHtmlAttributes to emit corresponding data-ajax-* attributes; added XML docs.
Fluent pager builder & extensions
src/X.PagedList.Mvc.Core/Fluent/HtmlPagerBuilder.cs, src/X.PagedList.Mvc.Core/Fluent/HtmlPagerExtensions.cs, src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs
HtmlPagerBuilder now keeps an IPagedList, supports partial views (WithPartialView), initializes defaults, and has XML docs; extension and interface had [PublicAPI] removed and doc comments added; signatures unchanged.
HtmlHelper refactor & API change
src/X.PagedList.Mvc.Core/HtmlHelper.cs
Major modularization of pager construction, TagBuilder helpers, added Go-To-Page form; PagedListPager signature changed to accept nullable IPagedList? and return string?; added richer rendering options and nullable handling.
TagBuilder utilities & factory
src/X.PagedList.Mvc.Core/TagBuilderExtensions.cs, src/X.PagedList.Mvc.Core/TagBuilderFactory.cs
Added TagBuilder extension methods (AddCssClass, AppendHtml, MergeAttribute, SetInnerText, ToString) and public ITagBuilderFactory with internal TagBuilderFactory implementation; added XML docs and removed [PublicAPI].
HtmlAttribute immutability
src/X.PagedList.Mvc.Core/HtmlAttribute.cs
Changed Key/Value from settable to init-only (required where applicable) and added documentation and example.
Core library style changes
src/X.PagedList/*.cs (BasePagedList, IPagedList, PagedList, OrderedPagedList, PagedListMetaData, StaticPagedList, Extensions/PagedListExtensions.cs)
Removed JetBrains.Annotations/usings and [PublicAPI] attributes; minor formatting/construct changes (e.g., Empty uses target-typed new[]); no signature/behavior changes.
Tests & formatting
tests/X.PagedList.Tests/*.cs
Mostly formatting: reformatted interfaces/classes to block style, whitespace adjustments, removed one test asserting null-dataset constructor behavior, minor code tidy; project test SDK/runner updated.

Sequence Diagram(s)

sequenceDiagram
  participant View
  participant HtmlPagerBuilder
  participant HtmlHelper
  participant TagBuilderFactory
  participant TagBuilderExt as TagBuilderExtensions

  View->>HtmlPagerBuilder: Pager(htmlHelper, pagedList?) / configure (WithPartialView?)
  HtmlPagerBuilder->>HtmlHelper: Build() 
  alt partial view specified
    HtmlHelper->>View: Partial(partialName, pagedList) (uses ViewBag options)
  else default rendering
    HtmlHelper->>TagBuilderFactory: Create("div"/"ul"/"li"/"a"/"form"...)
    loop build elements
      HtmlHelper->>TagBuilderExt: SetInnerText / AppendHtml / MergeAttribute / AddCssClass
    end
    HtmlHelper-->>HtmlPagerBuilder: string? (HTML) or null (suppressed)
    HtmlPagerBuilder-->>View: IHtmlContent
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I nibble code beneath the moon,
New tags and helpers hum a tune.
Init keys snug in carrot bed,
Ajax sparks where links are led.
Factory hops — the pager's new boon. 🥕🐇


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5116a16 and 8d44930.

📒 Files selected for processing (1)
  • src/Directory.Build.props (1 hunks)
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-target-frameworks

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request updates target framework configuration and enhances project documentation for the X.PagedList libraries. The changes focus on modernizing framework targets, updating package dependencies, and adding comprehensive XML documentation for public APIs.

  • Removes .NET 7.0 target framework support and the JetBrains.Annotations dependency
  • Updates package versions and modernizes C# syntax patterns
  • Adds extensive XML documentation for public APIs in the Mvc.Core project

Reviewed Changes

Copilot reviewed 31 out of 33 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
X.PagedList.Tests.csproj Switches from multi-targeting to .NET 8.0 only and updates test package versions
X.PagedList.csproj Removes .NET 7.0 target and JetBrains.Annotations dependency
X.PagedList.Mvc.Core.csproj Removes .NET 7.0 target and JetBrains.Annotations dependency
X.PagedList.EF.csproj Removes .NET 7.0 target and JetBrains.Annotations dependency
Directory.Build.props Updates version to 10.5.5 and copyright year to 2025
Various source files Removes JetBrains.Annotations usage and adds comprehensive XML documentation
Test files Minor formatting improvements and syntax modernization

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape sequences in XML documentation should use proper syntax. Use &lt; and &gt; instead of backslash escapes for HTML elements in documentation.

Suggested change
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
/// <param name="displayMode">Display mode controlling visibility of the <c>First</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToFirstPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the last page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Last</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToLastPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the previous page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Previous</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToPreviousPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Next</c> link.</param>

Copilot uses AI. Check for mistakes.
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape sequences in XML documentation should use proper syntax. Use &lt; and &gt; instead of backslash escapes for HTML elements in documentation.

Suggested change
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
/// <param name="displayMode">Display mode controlling visibility of the <c>First</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToFirstPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the last page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Last</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToLastPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the previous page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Previous</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToPreviousPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Next</c> link.</param>

Copilot uses AI. Check for mistakes.
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape sequences in XML documentation should use proper syntax. Use &lt; and &gt; instead of backslash escapes for HTML elements in documentation.

Suggested change
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
/// <param name="displayMode">Display mode controlling visibility of the <c>First</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToFirstPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the last page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Last</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToLastPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the previous page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Previous</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToPreviousPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Next</c> link.</param>

Copilot uses AI. Check for mistakes.
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escape sequences in XML documentation should use proper syntax. Use &lt; and &gt; instead of backslash escapes for HTML elements in documentation.

Suggested change
/// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
/// <param name="displayMode">Display mode controlling visibility of the <c>First</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToFirstPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the last page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Last</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToLastPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the previous page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Previous</c> link.</param>
/// <returns>The current builder instance.</returns>
IHtmlPagerBuilder DisplayLinkToPreviousPage(PagedListDisplayMode displayMode = PagedListDisplayMode.Always);
/// <summary>
/// Configures whether and when to render the link to the next page.
/// </summary>
/// <param name="displayMode">Display mode controlling visibility of the <c>Next</c> link.</param>

Copilot uses AI. Check for mistakes.
/// <summary>
/// Toggles rendering of the page count and current location text (e.g., \`Page 2 of 10\`).
/// </summary>
/// <param name="displayMode">True to show the page count/current location; false to hide it.</param>
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Documentation should use consistent terminology. The method name suggests 'page count and current location' but this parameter description only mentions 'page count/current location'.

Suggested change
/// <param name="displayMode">True to show the page count/current location; false to hide it.</param>
/// <param name="displayMode">True to show the page count and current location; false to hide it.</param>

Copilot uses AI. Check for mistakes.
/// Renders a pager that shows up to five page numbers at a time.
/// </summary>
/// <returns>Rendered pager as <see cref="IHtmlContent"/>.</returns>
IHtmlContent OnlyShowFivePagesAtATime();
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The method name 'OnlyShowFivePagesAtATime' contains a magic number. Consider documenting why five pages was chosen or making this configurable.

Suggested change
IHtmlContent OnlyShowFivePagesAtATime();
/// Renders a pager that shows up to a specified number of page numbers at a time.
/// </summary>
/// <param name="pageCount">
/// The maximum number of page numbers to display at once.
/// Default is 5, which is a common convention for usability, but can be customized as needed.
/// </param>
/// <returns>Rendered pager as <see cref="IHtmlContent"/>.</returns>
IHtmlContent ShowLimitedPagesAtATime(int pageCount = 5);

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs (1)

56-56: Doc style: use lowercase true/false and consistent wording

.NET XML docs typically use lowercase true/false. Also prefer “page count and current location” for consistency.

-    /// <param name="displayMode">True to show numbered page links; false to hide them.</param>
+    /// <param name="displayMode">true to show numbered page links; false to hide them.</param>
-    /// <param name="displayMode">True to show the page count/current location; false to hide it.</param>
+    /// <param name="displayMode">true to show the page count and current location; false to hide it.</param>
-    /// <param name="displayMode">True to show ellipses; false to omit them.</param>
+    /// <param name="displayMode">true to show ellipses; false to omit them.</param>

Also applies to: 63-63, 70-70

🧹 Nitpick comments (24)
src/X.PagedList/StaticPagedList.cs (1)

61-61: Ensure C# 12 collection expressions compile for all TFMs
Your projects multi-target net6.0, net8.0 and netstandard2.0 with <LangVersion>default</LangVersion>. Default LangVersion on net6.0/netstandard2.0 is C# 10, so

new([], 1, pageSize, 0)

will fail to compile for those targets.

• File: src/X.PagedList/StaticPagedList.cs (line 61)

Recommended change to maintain compatibility:

-        new([], 1, pageSize, 0);
+        new(Array.Empty<T>(), 1, pageSize, 0);

Alternatively, if you prefer using collection expressions everywhere, explicitly set

<LangVersion>12.0</LangVersion>

(or preview) in your csproj/Directory.Build.props and ensure all build agents use the .NET 8 SDK.

src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (2)

24-26: Add a guard against null/whitespace tag names

Optional but useful: validate input early to provide a clearer exception than the underlying TagBuilder (and keep behavior consistent across TFMs).

Apply this diff:

-    public TagBuilder Create(string tagName) => new(tagName);
+    public TagBuilder Create(string tagName)
+    {
+        if (string.IsNullOrWhiteSpace(tagName))
+            throw new System.ArgumentException("Tag name cannot be null or whitespace.", nameof(tagName));
+
+        return new TagBuilder(tagName);
+    }

22-26: Consider DI registration guidance for consumers

Since ITagBuilderFactory is public and TagBuilderFactory is internal, library users can provide their own implementation. Consider adding a brief note in README/docs (or a simple IServiceCollection extension) showing how to register a custom ITagBuilderFactory for consumers who want to alter TagBuilder creation.

src/X.PagedList.Mvc.Core/HtmlAttribute.cs (2)

17-25: Clarify NET6.0 docs vs. actual enforcement for Key (init-only but not required).

On NET6.0, Key is init-only with a default value, so it isn't strictly "required." The remarks currently imply enforcement that isn’t present at runtime.

Apply this doc-only tweak for accuracy:

-    /// Must be specified when constructing the instance.
-    /// Settable only during object initialization.
+    /// Should be specified during object initialization; this property is init-only.
+    /// If not set, it defaults to <see cref="string.Empty"/>.

26-33: Consider default/value semantics for Value on NET6.0.

Value defaults to string.Empty with a non-nullable object type. This works, but two minor notes:

  • For boolean/flag attributes, callers often use null to indicate “presence-only” semantics. With a non-nullable object, that representation isn’t possible for NET6.0.
  • If you keep the current shape, consider documenting that null isn’t expected on NET6.0, while later TFMs enforce initialization via required.

No change strictly necessary; just flagging the nuance for callers migrating from settable to init-only.

src/X.PagedList.Mvc.Core/TagBuilderExtensions.cs (4)

15-23: Pass-through AddCssClass extension is redundant.

Because TagBuilder already has an instance method with the same name/signature, this extension simply forwards to it and will rarely be selected by overload resolution. Consider removing to reduce noise, unless you intentionally keep it for discoverability and unified docs.


25-33: Appending raw HTML: call out XSS considerations or offer a safe overload.

AppendHtml intentionally bypasses encoding. If any inputs could be user-controlled, this is a vector for XSS. At minimum, call it out prominently in the XML docs. Optionally, provide an overload that accepts IHtmlContent.

Would you like me to add an IHtmlContent overload and expand the XML doc warning? I can draft it.


35-44: Consider returning the underlying MergeAttribute result (if available in your TFMs).

TagBuilder.MergeAttribute commonly returns a bool indicating whether the attribute was added/updated. Exposing that can be useful to call sites (e.g., conditional logic). If your ASP.NET Core version supports it, consider mirroring the return type.

If confirmed available, this minimal change preserves information:

-    public static void MergeAttribute(this TagBuilder tagBuilder, string key, string? value)
+    public static bool MergeAttribute(this TagBuilder tagBuilder, string key, string? value)
     {
-        tagBuilder.MergeAttribute(key, value);
+        return tagBuilder.MergeAttribute(key, value);
     }

56-76: renderMode parameter is unused; prefer HtmlEncoder.Default.

  • renderMode isn’t applied in the implementation, which can surprise callers. Either document that it’s currently ignored or remove it from the signature.
  • Prefer HtmlEncoder.Default over constructing a new encoder via TextEncoderSettings.

Apply this small, safe improvement now:

-        encoder ??= HtmlEncoder.Create(new TextEncoderSettings());
+        // Note: renderMode is currently not used by TagBuilder.WriteTo in ASP.NET Core.
+        // If a specific render mode is desired, consider handling it before calling WriteTo.
+        encoder ??= HtmlEncoder.Default;

If you want to align the signature with behavior (optional follow-up):

  • Remove the renderMode parameter and references in XML docs, or
  • Implement renderMode handling explicitly (more involved and easy to get wrong).
tests/X.PagedList.Tests/X.PagedList.Tests.csproj (1)

7-7: Confirm dropping .NET 6 test coverage

Current TFM configurations:

  • Library (src/X.PagedList/X.PagedList.csproj): <TargetFrameworks>net6.0;net8.0;netstandard2.0</TargetFrameworks>
  • Tests (tests/X.PagedList.Tests/X.PagedList.Tests.csproj): <TargetFramework>net8.0</TargetFramework>

If you intend to continue validating .NET 6 in your test suite, consider multi-targeting the tests:

tests/X.PagedList.Tests/X.PagedList.Tests.csproj

-        <TargetFramework>net8.0</TargetFramework>
+        <TargetFrameworks>net6.0;net8.0</TargetFrameworks>
src/Directory.Build.props (1)

13-16: Avoid version duplication by referencing Version.

To reduce maintenance friction, drive AssemblyVersion, FileVersion, and PackageVersion from Version.

-        <Version>10.5.5</Version>
-        <AssemblyVersion>10.5.5</AssemblyVersion>
-        <FileVersion>10.5.5</FileVersion>
-        <PackageVersion>10.5.5</PackageVersion>
+        <Version>10.5.5</Version>
+        <AssemblyVersion>$(Version)</AssemblyVersion>
+        <FileVersion>$(Version)</FileVersion>
+        <PackageVersion>$(Version)</PackageVersion>
tests/X.PagedList.Tests/PagedListFacts.cs (1)

410-410: Nit: simplify lambda to expression-bodied form

Minor readability improvement; no behavior change.

Apply:

-        Func<Blog, int> keySelector = blog => { return blog.BlogID; };
+        Func<Blog, int> keySelector = blog => blog.BlogID;
src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs (3)

28-28: Replace backtick escapes with XML-doc code tags

Use … inside XML documentation rather than backticks with escapes. This renders properly in IntelliSense.

-    /// <param name="displayMode">Display mode controlling visibility of the \`First\` link.</param>
+    /// <param name="displayMode">Display mode controlling visibility of the <c>First</c> link.</param>
-    /// <param name="displayMode">Display mode controlling visibility of the \`Last\` link.</param>
+    /// <param name="displayMode">Display mode controlling visibility of the <c>Last</c> link.</param>
-    /// <param name="displayMode">Display mode controlling visibility of the \`Previous\` link.</param>
+    /// <param name="displayMode">Display mode controlling visibility of the <c>Previous</c> link.</param>
-    /// <param name="displayMode">Display mode controlling visibility of the \`Next\` link.</param>
+    /// <param name="displayMode">Display mode controlling visibility of the <c>Next</c> link.</param>

Also applies to: 35-35, 42-42, 49-49


61-61: Replace backticked example with … code literal

Ensures correct formatting in IntelliSense/tooling.

-    /// Toggles rendering of the page count and current location text (e.g., \`Page 2 of 10\`).
+    /// Toggles rendering of the page count and current location text (e.g., <c>Page 2 of 10</c>).

125-129: Clarify the “five pages” rationale to avoid a magic number smell

Since the API name encodes “five”, consider making the summary more explicit about the choice and that it’s a preset equivalent to the corresponding render option.

-    /// Renders a pager that shows up to five page numbers at a time.
+    /// Renders a pager that shows up to five page numbers at a time.
+    /// This preset uses the same defaults as <see cref="PagedListRenderOptions.OnlyShowFivePagesAtATime"/>,
+    /// where five is a common usability default for compact pagers.
src/X.PagedList/X.PagedList.csproj (1)

14-15: Use modern NuGet metadata for README/license to improve NuGet.org rendering

Optional: prefer PackageReadmeFile/PackageLicenseFile properties over packing raw None items with empty PackagePath. This ensures NuGet.org displays the README and license correctly without extra ItemGroups.

You can remove the None entries and set properties instead:

-        <None Include="../../README.md" Pack="true" PackagePath=""/>
-        <None Include="../../LICENSE.md" Pack="true" PackagePath=""/>
+        <!-- Prefer modern NuGet metadata -->
+        <!-- Ensure these files exist at the specified relative paths -->

Then add to the PropertyGroup (outside the selected lines):

  <PropertyGroup>
    <PackageReadmeFile>README.md</PackageReadmeFile>
    <PackageLicenseFile>LICENSE.md</PackageLicenseFile>
  </PropertyGroup>
  <ItemGroup>
    <None Include="../../README.md" Pack="true" PackagePath="\" />
    <None Include="../../LICENSE.md" Pack="true" PackagePath="\" />
  </ItemGroup>

Note: Using PackagePath="" places files at the package root; alternatively keep only the properties if the files live in the project directory.

src/X.PagedList.EF/X.PagedList.EF.csproj (1)

19-25: Pinning EF versions: consider a patch-range for net8 to get security fixes automatically

You already use a range for net6 ([7.0.20,8.0.0)). For net8, pinning to 9.0.8 means manual bumps for patches. Consider 9.0.* to receive compatible patches automatically.

-        <PackageReference Include="Microsoft.EntityFrameworkCore" Version="9.0.8" />
+        <PackageReference Include="Microsoft.EntityFrameworkCore" Version="9.0.*" />

If you prefer fixed versions for reproducibility, feel free to keep as-is and rely on Renovate/Dependabot.

src/X.PagedList/PagedList.cs (1)

90-93: Improve error message specificity

The generic exception message could be more informative by including the actual collection size.

-            throw new Exception($"{nameof(collection)} size can't be greater than PageSize");
+            throw new InvalidOperationException($"{nameof(collection)} size ({base.Count}) can't be greater than PageSize ({PageSize})");
src/X.PagedList.Mvc.Core/Fluent/HtmlPagerBuilder.cs (1)

183-192: Consider null safety for partial view rendering

When rendering a partial view, the ViewBag properties are set directly. Consider validating that the partial view exists or handling potential rendering errors gracefully.

You might want to add validation to ensure the partial view exists before attempting to render it:

 public IHtmlContent Build()
 {
     if (string.IsNullOrWhiteSpace(_partialViewName))
     {
         return _htmlHelper.PagedListPager(_pagedList, _generatePageUrl, _options);
     }
+    
+    // Optionally validate partial view exists
+    if (_htmlHelper.ViewContext?.ViewEngine?.FindView(_htmlHelper.ViewContext, _partialViewName, false).View == null)
+    {
+        throw new InvalidOperationException($"Partial view '{_partialViewName}' not found.");
+    }

     _htmlHelper.ViewBag.Options = _options;
     _htmlHelper.ViewBag.GeneratePageUrl = _generatePageUrl;

     return _htmlHelper.Partial(_partialViewName, _pagedList);
 }
src/X.PagedList.Mvc.Core/Fluent/HtmlPagerExtensions.cs (1)

26-29: Consider performance impact of empty list creation

The method creates an empty enumerable and converts it to a paged list on every call. Consider caching a static empty paged list instance if this method is called frequently.

 public static class HtmlPagerExtensions
 {
+    private static readonly IPagedList EmptyPagedList = Enumerable.Empty<object?>().ToPagedList();
+    
     public static IHtmlContent Pager(this IHtmlHelper htmlHelper)
     {
-        return new HtmlPagerBuilder(htmlHelper, Enumerable.Empty<object?>().ToPagedList()).Build();
+        return new HtmlPagerBuilder(htmlHelper, EmptyPagedList).Build();
     }
src/X.PagedList.Mvc.Core/HtmlHelper.cs (1)

574-575: Fix inconsistent casing in style attribute

The style attribute uses Pascal case ("Style") instead of the standard lowercase ("style"). While this works in HTML, it's inconsistent with HTML standards.

-        input.Attributes.Add("Style", $"width: {options.InputWidth}px");
+        input.Attributes.Add("style", $"width: {options.InputWidth}px");
-        submit.Attributes.Add("Style", $"width: {options.SubmitButtonWidth}px");
+        submit.Attributes.Add("style", $"width: {options.SubmitButtonWidth}px");
src/X.PagedList.Mvc.Core/AjaxOptions.cs (3)

32-35: Prefix LoadingElementId with '#' to match expected CSS selector.

Unobtrusive Ajax typically expects a selector (e.g., "#spinner"). Emitting the raw ID can break loading indicator behavior.

-            attrs.Add(new HtmlAttribute { Key = "data-ajax-loading", Value = LoadingElementId });
+            attrs.Add(new HtmlAttribute { Key = "data-ajax-loading", Value = "#" + LoadingElementId });

If you prefer to be defensive in case callers pass "#id" already, I can adjust this to only prefix when missing the leading '#'.


37-41: Emit loading-duration only when a loading element is present.

Avoids a dangling configuration with no effect.

-        if (LoadingElementDuration > 0)
+        if (!string.IsNullOrEmpty(LoadingElementId) && LoadingElementDuration > 0)

27-31: Treat whitespace-only values as empty to avoid emitting no-op attributes.

Use IsNullOrWhiteSpace for these checks to avoid emitting attributes that contain only spaces.

-        if (!string.IsNullOrEmpty(Confirm))
+        if (!string.IsNullOrWhiteSpace(Confirm))
@@
-        if (!string.IsNullOrEmpty(LoadingElementId))
+        if (!string.IsNullOrWhiteSpace(LoadingElementId))
@@
-        if (!string.IsNullOrEmpty(OnBegin))
+        if (!string.IsNullOrWhiteSpace(OnBegin))
@@
-        if (!string.IsNullOrEmpty(OnComplete))
+        if (!string.IsNullOrWhiteSpace(OnComplete))
@@
-        if (!string.IsNullOrEmpty(OnFailure))
+        if (!string.IsNullOrWhiteSpace(OnFailure))
@@
-        if (!string.IsNullOrEmpty(OnSuccess))
+        if (!string.IsNullOrWhiteSpace(OnSuccess))
@@
-        if (!string.IsNullOrEmpty(Url))
+        if (!string.IsNullOrWhiteSpace(Url))

Also applies to: 42-66

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d83967b and 5116a16.

📒 Files selected for processing (33)
  • src/Directory.Build.props (1 hunks)
  • src/X.PagedList.EF/PagedListExtensions.cs (0 hunks)
  • src/X.PagedList.EF/X.PagedList.EF.csproj (2 hunks)
  • src/X.PagedList.Mvc.Core/AjaxOptions.cs (2 hunks)
  • src/X.PagedList.Mvc.Core/Fluent/HtmlPagerBuilder.cs (4 hunks)
  • src/X.PagedList.Mvc.Core/Fluent/HtmlPagerExtensions.cs (1 hunks)
  • src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs (1 hunks)
  • src/X.PagedList.Mvc.Core/HtmlAttribute.cs (1 hunks)
  • src/X.PagedList.Mvc.Core/HtmlHelper.cs (14 hunks)
  • src/X.PagedList.Mvc.Core/HtmlHelperExtension.cs (0 hunks)
  • src/X.PagedList.Mvc.Core/TagBuilderExtensions.cs (2 hunks)
  • src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (1 hunks)
  • src/X.PagedList.Mvc.Core/X.PagedList.Mvc.Core.csproj (1 hunks)
  • src/X.PagedList/BasePagedList.cs (1 hunks)
  • src/X.PagedList/Extensions/PagedListExtensions.cs (0 hunks)
  • src/X.PagedList/IPagedList.cs (0 hunks)
  • src/X.PagedList/OrderedPagedList.cs (0 hunks)
  • src/X.PagedList/PagedList.cs (1 hunks)
  • src/X.PagedList/PagedListMetaData.cs (0 hunks)
  • src/X.PagedList/StaticPagedList.cs (2 hunks)
  • src/X.PagedList/X.PagedList.csproj (1 hunks)
  • tests/X.PagedList.Tests/DbSet.cs (1 hunks)
  • tests/X.PagedList.Tests/IDbAsyncEnumerable.cs (1 hunks)
  • tests/X.PagedList.Tests/IDbAsyncEnumerator.cs (1 hunks)
  • tests/X.PagedList.Tests/IDbAsyncQueryProvider.cs (1 hunks)
  • tests/X.PagedList.Tests/PagedListExample.cs (2 hunks)
  • tests/X.PagedList.Tests/PagedListFacts.cs (2 hunks)
  • tests/X.PagedList.Tests/SplitAndPartitionFacts.cs (1 hunks)
  • tests/X.PagedList.Tests/StaticPagedListExample.cs (1 hunks)
  • tests/X.PagedList.Tests/StaticPagedListFacts.cs (1 hunks)
  • tests/X.PagedList.Tests/TestContext.cs (1 hunks)
  • tests/X.PagedList.Tests/TestDbAsyncQueryProvider.cs (3 hunks)
  • tests/X.PagedList.Tests/X.PagedList.Tests.csproj (1 hunks)
💤 Files with no reviewable changes (6)
  • src/X.PagedList/PagedListMetaData.cs
  • src/X.PagedList/Extensions/PagedListExtensions.cs
  • src/X.PagedList.Mvc.Core/HtmlHelperExtension.cs
  • src/X.PagedList/OrderedPagedList.cs
  • src/X.PagedList/IPagedList.cs
  • src/X.PagedList.EF/PagedListExtensions.cs
🧰 Additional context used
🧬 Code Graph Analysis (9)
tests/X.PagedList.Tests/StaticPagedListExample.cs (1)
src/X.PagedList/StaticPagedList.cs (4)
  • StaticPagedList (20-62)
  • StaticPagedList (34-37)
  • StaticPagedList (49-53)
  • StaticPagedList (60-61)
tests/X.PagedList.Tests/PagedListFacts.cs (1)
tests/X.PagedList.Tests/Blog.cs (1)
  • Blog (3-10)
src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs (1)
src/X.PagedList.Mvc.Core/PagedListRenderOptions.cs (5)
  • PagedListRenderOptions (9-438)
  • PagedListRenderOptions (14-46)
  • PagedListRenderOptions (379-404)
  • PagedListRenderOptions (411-427)
  • PagedListRenderOptions (434-437)
tests/X.PagedList.Tests/IDbAsyncEnumerator.cs (2)
tests/X.PagedList.Tests/IDbAsyncEnumerable.cs (1)
  • IDbAsyncEnumerator (5-5)
tests/X.PagedList.Tests/TestDbAsyncQueryProvider.cs (2)
  • IDbAsyncEnumerator (65-68)
  • IDbAsyncEnumerator (70-73)
src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (1)
src/X.PagedList.Mvc.Core/HtmlHelper.cs (11)
  • TagBuilder (73-80)
  • TagBuilder (89-106)
  • TagBuilder (115-136)
  • TagBuilder (145-168)
  • TagBuilder (178-204)
  • TagBuilder (213-235)
  • TagBuilder (244-264)
  • TagBuilder (272-279)
  • TagBuilder (287-294)
  • TagBuilder (304-328)
  • TagBuilder (338-362)
src/X.PagedList.Mvc.Core/TagBuilderExtensions.cs (2)
src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (2)
  • TagBuilder (15-15)
  • TagBuilder (25-25)
src/X.PagedList.Mvc.Core/HtmlHelper.cs (13)
  • TagBuilder (73-80)
  • TagBuilder (89-106)
  • TagBuilder (115-136)
  • TagBuilder (145-168)
  • TagBuilder (178-204)
  • TagBuilder (213-235)
  • TagBuilder (244-264)
  • TagBuilder (272-279)
  • TagBuilder (287-294)
  • TagBuilder (304-328)
  • TagBuilder (338-362)
  • AppendHtml (42-45)
  • SetInnerText (32-35)
src/X.PagedList.Mvc.Core/AjaxOptions.cs (1)
src/X.PagedList.Mvc.Core/HtmlAttribute.cs (1)
  • HtmlAttribute (14-50)
src/X.PagedList.Mvc.Core/Fluent/HtmlPagerExtensions.cs (2)
src/X.PagedList/Extensions/PagedListExtensions.cs (7)
  • IPagedList (86-91)
  • IPagedList (105-116)
  • IPagedList (134-142)
  • IPagedList (160-168)
  • IPagedList (187-195)
  • IPagedList (214-247)
  • IPagedList (268-276)
examples/Example.Website/Controllers/HomeController.cs (1)
  • IPagedList (38-60)
src/X.PagedList.Mvc.Core/HtmlHelper.cs (3)
src/X.PagedList.Mvc.Core/TagBuilderExtensions.cs (3)
  • SetInnerText (51-54)
  • AppendHtml (30-33)
  • ToString (65-75)
src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (2)
  • TagBuilder (15-15)
  • TagBuilder (25-25)
src/X.PagedList.Mvc.Core/PagedListRenderOptions.cs (5)
  • PagedListRenderOptions (9-438)
  • PagedListRenderOptions (14-46)
  • PagedListRenderOptions (379-404)
  • PagedListRenderOptions (411-427)
  • PagedListRenderOptions (434-437)
🔇 Additional comments (32)
tests/X.PagedList.Tests/DbSet.cs (1)

5-7: Formatting-only change; semantics unchanged — LGTM

The class remains an empty wrapper over List. No API or behavior change; consistent with the PR’s formatting refinements.

tests/X.PagedList.Tests/SplitAndPartitionFacts.cs (1)

67-67: No functional changes; formatting only.

Whitespace-only change at end-of-file. Tests remain unaffected.

tests/X.PagedList.Tests/TestContext.cs (1)

6-6: No functional changes in TestContext.

EOF formatting tweak only; no behavioral impact.

tests/X.PagedList.Tests/StaticPagedListFacts.cs (1)

31-31: Type inference change is fine.

Switching int[] data to var data with an obvious initializer improves readability without behavioral change.

tests/X.PagedList.Tests/PagedListExample.cs (1)

15-15: Readability-only spacing; no behavioral impact.

Inserted blank lines after page creations to visually separate arrange/act from assert/logging. Looks good.

Also applies to: 22-22, 31-31

tests/X.PagedList.Tests/IDbAsyncEnumerable.cs (1)

3-10: Formatting-only refactor — LGTM

Interfaces are now in block form with no API changes. This improves consistency/readability across test helpers.

tests/X.PagedList.Tests/IDbAsyncQueryProvider.cs (1)

5-7: Cosmetic change — LGTM

Empty interface moved to block style. Signature unchanged; no behavioral impact.

tests/X.PagedList.Tests/TestDbAsyncQueryProvider.cs (4)

10-12: Class declaration reformatted — LGTM

Switching to multi-line braces for the public test type improves consistency with the rest of the test helpers.


57-58: Constructor style refactor — LGTM

Empty constructor expressed with multi-line braces; no functional changes.


62-63: Constructor style refactor — LGTM

Same as above; formatting-only change confirmed.


99-99: Trailing newline/bracing tidy-up — LGTM

Minor formatting improvement; no semantic impact.

src/X.PagedList.Mvc.Core/TagBuilderFactory.cs (2)

5-15: Good documentation for the factory contract

Clear XML docs on the new public interface and method; this will help downstream consumers.


18-21: Default implementation docs — LGTM

Concise summary and usage look good.

tests/X.PagedList.Tests/IDbAsyncEnumerator.cs (1)

3-10: Formatting-only refactor — LGTM

Interfaces are now block-style; no API changes. Consistent with related test helpers.

src/X.PagedList.Mvc.Core/HtmlAttribute.cs (2)

34-49: Required members on non-NET6 TFMs look good.

The required + init-only properties for Key and Value align with the class remarks and reduce misuse. No issues spotted.


24-24: No post-construction mutations found – init-only setters are safe

A full code-wide scan produced no direct assignments to HtmlAttribute.Key or HtmlAttribute.Value outside of object initializers, nor any parameterless new HtmlAttribute() usages. Switching these properties to init-only will not break existing call sites.

tests/X.PagedList.Tests/X.PagedList.Tests.csproj (1)

11-15: Test SDK and runner versions look good.

The upgrades are current and compatible with net8. No issues spotted.

src/Directory.Build.props (1)

10-10: Copyright year bump: LGTM

Metadata reflects the new year; no concerns.

src/X.PagedList/BasePagedList.cs (1)

1-1: Removal of external annotations: no functional impact.

JetBrains annotations removal doesn’t affect runtime behavior. The file remains solid.

tests/X.PagedList.Tests/StaticPagedListExample.cs (1)

14-14: Whitespace-only touch-up improves readability

The added blank lines and EOF newline are fine and make the sample output blocks easier to visually scan. No functional impact.

Also applies to: 21-21, 28-28, 34-34

src/X.PagedList.Mvc.Core/Fluent/IHtmlPagerBuilder.cs (1)

6-12: Interface-level XML docs: solid addition

Clear, concise, and helpful summary/remarks with correct usage. This will improve discoverability and IntelliSense.

src/X.PagedList.EF/X.PagedList.EF.csproj (1)

7-7: TFM narrowing: removal of net7.0 support
All .csproj, .props, and .targets files have been scanned—there are no remaining references to net7.0. If dropping .NET 7 is intentional, please:

  • Confirm this aligns with consumer expectations for the EF extensions.
  • Add a clear note in your release notes (or CHANGELOG) that support for .NET 7 has been removed.
src/X.PagedList/PagedList.cs (1)

1-1: LGTM!

The removal of the JetBrains.Annotations using directive aligns with the broader cleanup across the repository to remove this dependency.

src/X.PagedList.Mvc.Core/X.PagedList.Mvc.Core.csproj (1)

8-8: Confirm .NET 7 removal is documented

A global search found no occurrences of “net7.0” or “.NET 7” in the codebase or any changelog/release-notes files at the repo root. Please verify that your project’s release documentation (e.g. CHANGELOG.md or release notes) clearly states that support for .NET 7 has been dropped and directs users to upgrade to .NET 8 or downgrade to .NET 6.

src/X.PagedList.Mvc.Core/Fluent/HtmlPagerBuilder.cs (1)

7-13: LGTM! Well-documented class

The XML documentation clearly explains the purpose and usage of the HtmlPagerBuilder class, and the use of <inheritdoc /> on all public methods maintains clean documentation inheritance.

src/X.PagedList.Mvc.Core/Fluent/HtmlPagerExtensions.cs (1)

8-14: LGTM! Excellent documentation

The XML documentation is comprehensive and clearly explains the purpose of this extension class with appropriate use of <remarks> sections.

src/X.PagedList.Mvc.Core/HtmlHelper.cs (2)

10-13: LGTM! Well-structured helper class

The class is well-documented and properly structured with dependency injection for the TagBuilderFactory.


395-408: Review Pagination Off-by-One Handling

I didn’t find any unit tests covering the scenario where the current page is near the end of the list. After you clamp firstPageToDisplay when lastPageToDisplay > list.PageCount, you must also recalculate lastPageToDisplay (or adjust pageNumbersToDisplay) to avoid showing pages beyond the last page.

• File: src/X.PagedList.Mvc.Core/HtmlHelper.cs (around lines 395–408)

 if (lastPageToDisplay > list.PageCount)
 {
     firstPageToDisplay = list.PageCount - maxPageNumbersToDisplay + 1;
+    // Recalculate to prevent pages past the end
+    pageNumbersToDisplay = list.PageCount - firstPageToDisplay + 1;
+    lastPageToDisplay   = list.PageCount;
 }

• Please add unit tests for:

  • list.PageCount < maxPageNumbersToDisplay (e.g. 3 pages with max display = 5)
  • list.PageNumber at or near PageCount, ensuring the displayed range is correctly trimmed.
src/X.PagedList.Mvc.Core/AjaxOptions.cs (4)

5-8: Documentation upgrade looks solid and precise.

Clear, actionable summary and purpose. No concerns.


11-16: Good XML documentation on attribute rendering.

The return type and behavior are well-described. This will help downstream usage.


85-134: Property surface area and XML docs look comprehensive.

The additions cover the common unobtrusive AJAX knobs and are well documented. No issues from an API design standpoint.


62-66: Support for data-ajax-url confirmed

The official jQuery Unobtrusive Ajax plugin reads data-ajax-url on anchors and forms and uses its string value as the request URL (falling back to href or action if the attribute is missing or empty). It accepts any URL you could pass to $.ajax (absolute, root-relative, page-relative, etc.). No changes needed.

Comment on lines 19 to 25
var attrs = new List<HtmlAttribute>
{
new() {Key = "data-ajax-method", Value = HttpMethod},
new() {Key = "data-ajax-mode", Value = InsertionMode},
new() {Key = "data-ajax-update", Value = "#" + UpdateTargetId},
new() {Key = "data-ajax", Value = "true"}
new() { Key = "data-ajax-method", Value = HttpMethod },
new() { Key = "data-ajax-mode", Value = InsertionMode },
new() { Key = "data-ajax-update", Value = "#" + UpdateTargetId },
new() { Key = "data-ajax", Value = "true" }
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Fix two AJAX attribute issues: invalid update selector when UpdateTargetId is null, and incorrect mode value (enum).

  • data-ajax-update is always emitted and becomes "#" when UpdateTargetId is null/empty, which is invalid and can break unobtrusive updates.
  • data-ajax-mode currently emits the enum value (e.g., Replace, InsertBefore, InsertAfter). The jQuery Unobtrusive Ajax plugin expects "replace", "before", or "after". Emitting InsertBefore/InsertAfter will not match and will default to replace or fail.

Apply this diff to map the enum to plugin-supported strings and only emit update when a valid ID is provided:

-        var attrs = new List<HtmlAttribute>
-        {
-            new() { Key = "data-ajax-method", Value = HttpMethod },
-            new() { Key = "data-ajax-mode", Value = InsertionMode },
-            new() { Key = "data-ajax-update", Value = "#" + UpdateTargetId },
-            new() { Key = "data-ajax", Value = "true" }
-        };
+        var attrs = new List<HtmlAttribute>();
+
+        var modeValue = InsertionMode switch
+        {
+            InsertionMode.Replace => "replace",
+            InsertionMode.InsertBefore => "before",
+            InsertionMode.InsertAfter => "after",
+            _ => "replace"
+        };
+
+        attrs.Add(new() { Key = "data-ajax-method", Value = HttpMethod });
+        attrs.Add(new() { Key = "data-ajax-mode", Value = modeValue });
+        if (!string.IsNullOrEmpty(UpdateTargetId))
+        {
+            attrs.Add(new() { Key = "data-ajax-update", Value = "#" + UpdateTargetId });
+        }
+        attrs.Add(new() { Key = "data-ajax", Value = "true" });

Run this to confirm the InsertionMode enum member names so we map correctly:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Locate InsertionMode enum and its members:"
rg -nP -C2 'enum\s+InsertionMode\b'

echo
echo "Where is data-ajax-mode emitted elsewhere (if any):"
rg -nP -C2 '\bdata-ajax-mode\b|ToUnobtrusiveHtmlAttributes\b'

Length of output: 6201


To ensure we map all possible enum values correctly, let’s print out the full InsertionMode definition:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== InsertionMode enum ==="
sed -n '1,50p' src/X.PagedList.Mvc.Core/InsertionMode.cs

Length of output: 312


Prevent emitting an empty update selector and normalize AJAX mode

  • Only include data-ajax-update when UpdateTargetId is non-empty to avoid the invalid selector "#".
  • Currently InsertionMode only has Replace (serialized as "Replace", which the plugin uppercases to "REPLACE" and falls back to content replacement). You can normalize it to lowercase for clarity—and if you ever add InsertBefore/InsertAfter, map those explicitly.

Suggested change in src/X.PagedList.Mvc.Core/AjaxOptions.cs:

--- a/src/X.PagedList.Mvc.Core/AjaxOptions.cs
+++ b/src/X.PagedList.Mvc.Core/AjaxOptions.cs
@@ public virtual IEnumerable<HtmlAttribute> ToUnobtrusiveHtmlAttributes()
-        var attrs = new List<HtmlAttribute>
-        {
-            new() { Key = "data-ajax-method", Value = HttpMethod },
-            new() { Key = "data-ajax-mode", Value = InsertionMode },
-            new() { Key = "data-ajax-update", Value = "#" + UpdateTargetId },
-            new() { Key = "data-ajax", Value = "true" }
-        };
+        var attrs = new List<HtmlAttribute>();
+        attrs.Add(new() { Key = "data-ajax-method", Value = HttpMethod });
+        // normalize mode to lowercase; plugin uppercases internally
+        attrs.Add(new() { Key = "data-ajax-mode", Value = InsertionMode.ToString().ToLowerInvariant() });
+        if (!string.IsNullOrEmpty(UpdateTargetId))
+        {
+            attrs.Add(new() { Key = "data-ajax-update", Value = "#" + UpdateTargetId });
+        }
+        attrs.Add(new() { Key = "data-ajax", Value = "true" });

For future enum values, you might use a switch expression:

var modeValue = InsertionMode switch
{
    InsertionMode.Replace      => "replace",
    InsertionMode.InsertBefore => "before",
    InsertionMode.InsertAfter  => "after",
    _                          => "replace"
};
attrs.Add(new() { Key = "data-ajax-mode", Value = modeValue });
🤖 Prompt for AI Agents
In src/X.PagedList.Mvc.Core/AjaxOptions.cs around lines 19–25, avoid emitting an
empty selector by only adding the data-ajax-update attribute when UpdateTargetId
is non-empty, and normalize data-ajax-mode by mapping the InsertionMode enum to
lowercase string values (e.g. Replace => "replace", InsertBefore => "before",
InsertAfter => "after", default => "replace") before adding the attribute so the
client plugin receives expected values; implement the mapping (preferably with a
switch expression) and conditionally add the update attribute to the attrs list.


<LangVersion>default</LangVersion>
<TargetFrameworks>net6.0;net7.0;net8.0;netstandard2.0;netstandard2.1</TargetFrameworks>
<TargetFrameworks>net6.0;net8.0;netstandard2.0</TargetFrameworks>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Target framework changes: confirm ecosystem impact and alignment across the solution

Dropping net7.0/netstandard2.1 and keeping net6.0/net8.0/netstandard2.0 looks reasonable. Please verify:

  • All other X.PagedList.* projects are consistent with this TFM matrix or intentionally narrower.
  • Release notes mention the TFM changes for consumers.

Run this script to list TFMs used across all .csproj files to spot inconsistencies:


🏁 Script executed:

#!/bin/bash
set -euo pipefail
# Show TargetFramework/TargetFrameworks for every project with context
rg -n -C2 -g '**/*.csproj' -P '(?m)^\s*<(TargetFrameworks?|TargetFramework)\b.*</\1>'

Length of output: 2549


TFM consistency confirmed; document changes in release notes

All X.PagedList projects now target net6.0 and net8.0, with the core library additionally targeting netstandard2.0. EF and MVC packages intentionally omit netstandard2.0. Test and example projects remain on net8.0 only:

  • src/X.PagedList/X.PagedList.csproj → net6.0; net8.0; netstandard2.0
  • src/X.PagedList.EF/X.PagedList.EF.csproj → net6.0; net8.0
  • src/X.PagedList.Mvc.Core/X.PagedList.Mvc.Core.csproj → net6.0; net8.0
  • tests/X.PagedList.Tests/X.PagedList.Tests.csproj → net8.0
  • examples/**/*.csproj → net8.0

Action items:

  • Update your release notes (e.g. CHANGELOG.md or RELEASE_NOTES.md) to call out the dropped TFMs (net7.0, netstandard2.1) and the supported matrix (net6.0, net8.0, netstandard2.0).
🤖 Prompt for AI Agents
In src/X.PagedList/X.PagedList.csproj around line 8, the project TFM list was
changed to "net6.0;net8.0;netstandard2.0" and the reviewer requests documenting
TFM consistency and dropped frameworks; update the repository release notes
(e.g., CHANGELOG.md or RELEASE_NOTES.md) to explicitly state the new supported
TFM matrix (core: net6.0, net8.0, netstandard2.0; EF and MVC: net6.0, net8.0;
tests and examples: net8.0), and call out which TFMs were dropped (net7.0 and
netstandard2.1), adding a short migration note for consumers about compatibility
or required changes.

Bumped version numbers from 10.5.5 to 10.5.8 and added the IsPackable property to enable packaging. This ensures the project can be packed and reflects the latest version.
@a-gubskiy a-gubskiy merged commit 005b1a4 into master Aug 18, 2025
3 of 4 checks passed
@a-gubskiy a-gubskiy deleted the update-target-frameworks branch August 18, 2025 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant