Bugfix/v16/activecampaign/pagination filtering #284

acoumb · 2025-10-22T15:39:17Z

The fix from #283 for the ActiveCampaign integration targeting Umbraco 16, but instead of the AngularJS changes the #handleFilterInput and #onPageChange will trigger a call to the server for retrieving the list of forms.

Copilot

Pull Request Overview

This PR implements pagination and filtering improvements for the ActiveCampaign integration in Umbraco 16, addressing issues from #283. Instead of client-side filtering, the changes enable server-side filtering and pagination by passing search queries to the backend API.

Key changes:

Added server-side search query parameter support throughout the data flow chain
Modified the forms modal to trigger server calls for both filtering and pagination
Implemented proper URI construction with URL encoding for search queries

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Umbraco.Cms.Integrations.Crm.ActiveCampaign.csproj	Version bumped to 5.1.0 for the new release
umbraco-package.json	Version bumped to 5.1.0 to match the project version
activecampaign-forms-modal.element.ts	Refactored filtering to use server-side search instead of client-side array filtering
activecampaign-forms.repository.ts	Added searchQuery parameter support to pass to the API service
activecampaign-forms.context.ts	Added searchQuery parameter to propagate the search term through the data layer
GetFormsPagedController.cs	Implemented URI building logic with search query support and proper URL encoding
form-picker-property-editor.element.ts	Removed extraneous blank line for code cleanup

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

Copilot · 2025-10-22T15:40:15Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+            return queryParamsDictionary.Count == 0
+                ? uri
+                : string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));


The URI building logic concatenates query parameters manually. Consider using UriBuilder or QueryHelpers.AddQueryString for more maintainable and robust query string construction.

Copilot · 2025-10-22T15:40:15Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

+    async #handleFilterInput(event: UUIInputEvent) {
        let query = (event.target.value as string) || '';
        query = query.toLowerCase();
+        this._searchQuery = query;

-        const result = !query
-            ? this._forms
-            : this._forms.filter((form) => form.name.toLowerCase().includes(query));
-
-        this._filteredForms = result;
+        await this.#loadForms(this._currentPageNumber, this._searchQuery);
    }


The filter input handler triggers a server call on every keystroke without debouncing, which could result in excessive API requests. Consider implementing debouncing to reduce server load and improve performance.

Copilot · 2025-10-22T15:40:15Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

        let query = (event.target.value as string) || '';
        query = query.toLowerCase();
+        this._searchQuery = query;



When filtering is applied, the page number should be reset to 1 to avoid displaying empty results if the current page exceeds the filtered result set. Consider resetting this._currentPageNumber to 1 before calling #loadForms.

Suggested change

this._currentPageNumber = 1;

Copilot

Pull Request Overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

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

Copilot · 2025-10-23T11:56:05Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

+        // Clear existing timeout
+        if (this.#filterTimeout) {
+            clearTimeout(this.#filterTimeout);
+        }

-        this._filteredForms = result;
+        this.#filterTimeout = setTimeout(async () => {
+            this._currentPageNumber = 1;
+            await this.#loadForms(this._currentPageNumber, this._searchQuery);
+        }, 500);


The timeout is not cleared when the component is destroyed, which could lead to attempted state updates after the element is removed from the DOM. Add cleanup in a disconnectedCallback or similar lifecycle method to clear this.#filterTimeout.

Copilot · 2025-10-23T11:56:05Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+        {
+            var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
+
+            Dictionary<string, string> queryParamsDictionary = new Dictionary<string, string>();


[nitpick] Use object initializer syntax for better readability: var queryParamsDictionary = new Dictionary<string, string>(); or Dictionary<string, string> queryParamsDictionary = new(); (if using C# 9+).

Suggested change

Dictionary<string, string> queryParamsDictionary = new Dictionary<string, string>();

var queryParamsDictionary = new Dictionary<string, string>();

Copilot · 2025-10-23T11:56:05Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+
+            return queryParamsDictionary.Count == 0
+                ? uri
+                : string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));


[nitpick] Use string interpolation instead of string.Format for better readability: $\"{uri}&{string.Join(\"&\", queryParamsDictionary.Select(kvp => $\"{kvp.Key}={kvp.Value}\"))}\"

Suggested change

: string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));

: $"{uri}&{string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}"))}";

Copilot

Pull Request Overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

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

Copilot · 2025-10-23T12:46:53Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+        {
+            var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
+
+            Dictionary<string, string> queryParamsDictionary = new();


The queryParamsDictionary is declared but never populated or used. The query parameters are being added directly to the URI using QueryHelpers.AddQueryString. This unused variable should be removed along with the unnecessary conditional logic at lines 67-69 that references it.

Copilot · 2025-10-23T12:46:53Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

 using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.WebUtilities;
 using Microsoft.Extensions.Options;
+using System.Web;


The System.Web import at line 6 is unnecessary since QueryHelpers.AddQueryString from Microsoft.AspNetCore.WebUtilities is being used for URL encoding. Remove the unused System.Web import.

Suggested change

using System.Web;

Copilot

Pull Request Overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

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

Copilot · 2025-10-23T12:58:40Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

    }

+    disconnectedCallback() {
+        clearTimeout(this.#filterTimeout);


The clearTimeout call may fail if #filterTimeout is undefined. While clearTimeout handles undefined gracefully in browsers, the explicit check on line 93-95 suggests this code expects defined values. Consider adding a conditional check or calling super.disconnectedCallback() to follow the component lifecycle pattern.

Suggested change

clearTimeout(this.#filterTimeout);

super.disconnectedCallback();

if (this.#filterTimeout !== undefined) {

clearTimeout(this.#filterTimeout);

}

Copilot · 2025-10-23T12:58:41Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

        [ProducesResponseType(StatusCodes.Status403Forbidden)]
        [ProducesResponseType(StatusCodes.Status500InternalServerError)]
-        public async Task<IActionResult> GetForms([FromQuery] int? page = 1)
+        public async Task<IActionResult> GetForms([FromQuery] int? page = 1, string? searchQuery = "")


The searchQuery parameter should be marked with [FromQuery] attribute for consistency with the page parameter and to make the API contract explicit. Without it, model binding may behave unexpectedly in certain scenarios.

Suggested change

public async Task<IActionResult> GetForms([FromQuery] int? page = 1, string? searchQuery = "")

public async Task<IActionResult> GetForms([FromQuery] int? page = 1, [FromQuery] string? searchQuery = "")

Copilot · 2025-10-23T12:58:41Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+
+        private string BuildRequestUri(string baseAddress, int page, string searchQuery)
+        {
+            var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";


The URI construction manually appends '?limit=' without checking if ApiPath already contains query parameters. If ApiPath includes a query string, this will produce malformed URIs. Use QueryHelpers.AddQueryString for the limit parameter as well to handle this correctly.

Suggested change

var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";

var uri = QueryHelpers.AddQueryString($"{baseAddress}{ApiPath}", "limit", Constants.DefaultPageSize.ToString());

Copilot

Pull Request Overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

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

Copilot · 2025-10-23T13:05:05Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

+    disconnectedCallback() {
+        if (this.#filterTimeout) {
+            clearTimeout(this.#filterTimeout);
+        }
+    }


The disconnectedCallback should call super.disconnectedCallback() to ensure proper cleanup of parent class resources.

Copilot

Pull Request Overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

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

Copilot · 2025-10-23T13:09:48Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

    }

-    #handleFilterInput(event: UUIInputEvent) {
+    async #handleFilterInput(event: UUIInputEvent) {


The method is marked as async but doesn't await the setTimeout callback. This can lead to unhandled promise rejections if the loadForms call fails. Consider removing async from the method signature since the actual async work happens in the timeout callback.

Suggested change

async #handleFilterInput(event: UUIInputEvent) {

#handleFilterInput(event: UUIInputEvent) {

Copilot · 2025-10-23T13:09:49Z

...co.Cms.Integrations.Crm.ActiveCampaign/Api/Management/Controllers/GetFormsPagedController.cs

+            var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
+
+            if (page > 1)
+            {
+                uri = QueryHelpers.AddQueryString(uri, "offset", ((page - 1) * Constants.DefaultPageSize).ToString());
+            }
+
+            if (!string.IsNullOrWhiteSpace(searchQuery))
+            {
+                uri = QueryHelpers.AddQueryString(uri, "search", searchQuery);
+            }
+


The URI construction manually adds '?limit=' which could create malformed URIs if ApiPath already contains query parameters. Use QueryHelpers.AddQueryString for all parameters including 'limit' to ensure proper URI construction.

Suggested change

var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";

if (page > 1)

{

uri = QueryHelpers.AddQueryString(uri, "offset", ((page - 1) * Constants.DefaultPageSize).ToString());

}

if (!string.IsNullOrWhiteSpace(searchQuery))

{

uri = QueryHelpers.AddQueryString(uri, "search", searchQuery);

}

var uri = $"{baseAddress}{ApiPath}";

var queryParams = new Dictionary<string, string>

{

{ "limit", Constants.DefaultPageSize.ToString() }

};

if (page > 1)

{

queryParams["offset"] = ((page - 1) * Constants.DefaultPageSize).ToString();

}

if (!string.IsNullOrWhiteSpace(searchQuery))

{

queryParams["search"] = searchQuery;

}

uri = QueryHelpers.AddQueryString(uri, queryParams);

Copilot · 2025-10-23T13:09:49Z

...o.Cms.Integrations.Crm.ActiveCampaign/Client/src/modal/activecampaign-forms-modal.element.ts

+        this.#filterTimeout = setTimeout(async () => {
+            this._currentPageNumber = 1;
+            await this.#loadForms(this._currentPageNumber, this._searchQuery);
+        }, 500);


[nitpick] The 500ms debounce timeout is hardcoded. Consider extracting this to a constant (e.g., SEARCH_DEBOUNCE_MS) to improve maintainability and make it easier to adjust the debounce delay if needed.

acoumb added 2 commits October 22, 2025 14:15

Update pagination and filtering actions

a1a28d3

Update ActiveCampaign backoffice version

b8f07f0

acoumb requested a review from Copilot October 22, 2025 15:39

Copilot AI reviewed Oct 22, 2025

View reviewed changes

acoumb added 2 commits October 23, 2025 12:02

Add debouncing feature

3cb8c08

Modify timeout delay

1798828

acoumb requested a review from Copilot October 23, 2025 11:55

Copilot AI reviewed Oct 23, 2025

View reviewed changes

Use QueryHelpers and clear timeout on disconnected callback.

584d3c1

acoumb requested a review from Copilot October 23, 2025 12:46

Copilot AI reviewed Oct 23, 2025

View reviewed changes

Remove unused declarations

1b730ce

acoumb requested a review from Copilot October 23, 2025 12:57

Copilot AI reviewed Oct 23, 2025

View reviewed changes

Add timeout check on disconnected

6187c87

acoumb requested a review from Copilot October 23, 2025 13:04

Copilot AI reviewed Oct 23, 2025

View reviewed changes

PR updates

710ea33

acoumb requested a review from Copilot October 23, 2025 13:08

Copilot AI reviewed Oct 23, 2025

View reviewed changes

acoumb merged commit c72deb1 into main-v16 Oct 23, 2025
7 checks passed

acoumb deleted the bugfix/v16/activecampaign/pagination-filtering branch October 23, 2025 13:17

	Dictionary<string, string> queryParamsDictionary = new Dictionary<string, string>();
	var queryParamsDictionary = new Dictionary<string, string>();

	: string.Format("{0}&{1}", uri, string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}")));
	: $"{uri}&{string.Join("&", queryParamsDictionary.Select(kvp => $"{kvp.Key}={kvp.Value}"))}";

	public async Task<IActionResult> GetForms([FromQuery] int? page = 1, string? searchQuery = "")
	public async Task<IActionResult> GetForms([FromQuery] int? page = 1, [FromQuery] string? searchQuery = "")

	var uri = $"{baseAddress}{ApiPath}?limit={Constants.DefaultPageSize}";
	var uri = QueryHelpers.AddQueryString($"{baseAddress}{ApiPath}", "limit", Constants.DefaultPageSize.ToString());

	async #handleFilterInput(event: UUIInputEvent) {
	#handleFilterInput(event: UUIInputEvent) {

Bugfix/v16/activecampaign/pagination filtering #284

Bugfix/v16/activecampaign/pagination filtering #284

Uh oh!

Conversation

acoumb commented Oct 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels