Handle placement priority

[davidfischer]

Allow multiple placements to have a priority (data-ea-priority). When multiple prioritized placements are on the page, only one will be chosen by the server and returned taking into account the inventory and the priority placed by the client.

[Copilot]

Pull request overview

This pull request adds priority placement support to the EthicalAds client library, allowing multiple ad placements on a page to be grouped and sent to the server together with priority values. The server then selects one placement to return based on the priority and available inventory.

Changes:

• Added data-ea-priority attribute support for ad placements
• Implemented fetchGroup() static method to batch-fetch multiple prioritized placements
• Modified load_placements() to group placements by publisher and priority before fetching
• Enhanced error handling in placement loading

Reviewed changes

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

File	Description
index.js	Added priority field to Placement class, implemented fetchGroup method for batch fetching, updated load_placements to group priority placements, improved error handling
tests/priority-placement.test.html	Added test case for priority placement functionality with two priority placements and one standalone placement
docs/index.rst	Added documentation for the new data-ea-priority attribute

Comments suppressed due to low confidence (3)

index.js:497

• The priority field is stored but the type is not documented in the JSDoc comment for the constructor. When options.priority is null or undefined, it's stored as null, but when it has a value it's stored as a string (from getAttribute). Consider documenting the expected type and whether it should be a string or number.

    this.priority = options.priority || null;
    this.div_id =
      target.id ||
      "ad_" + Date.now() + "_" + Math.floor(Math.random() * 1000000000);
    this.fetchPromise = null;

index.js:1179

• The error handling added here catches errors from placement.load() and converts them to null, which is good. However, errors that occur during load_placements itself (not inside individual placement.load() calls) will still reject the outer Promise and be caught by the wait promise handler (lines 1305-1312) or other callers. This creates two different error handling paths. Consider whether all errors should be handled uniformly. The current approach is acceptable but could be confusing for maintenance.

        return placement.load().catch((err) => {
          if (err instanceof EthicalAdsWarning) {
            logger.warn(err.message);
          } else {
            logger.error(err.message);
          }
          return null;
        });

index.js:496

• The div_id generation uses Date.now() and Math.random() which could theoretically generate the same ID for placements created in rapid succession, especially if multiple placements are created in the same millisecond. While the probability is low due to the random component, consider using a counter or ensuring uniqueness, especially since div_id is used to identify the winning placement in fetchGroup responses.

    this.div_id =
      target.id ||
      "ad_" + Date.now() + "_" + Math.floor(Math.random() * 1000000000);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The test mock for fetchGroup doesn't set placement.response for the winner, only returns the DOM element. However, the real fetchGroup implementation (line 927) sets placement.response = response before returning the element. The test should also set response on the winner placement to more accurately simulate the real behavior, especially since other methods like inViewport() depend on this.response being set.

[Copilot]

The fetchGroup method doesn't have a reject path or error handling in the Promise. If the script fails to load or the callback is never called, the promise will hang indefinitely. The regular fetch() method resolves with undefined on script error (line 845), but fetchGroup's error handler only resolves without a value (line 910), and the per-placement promises in line 916 will then try to check response.html and response.view_url on an undefined response, which could cause issues. Consider ensuring consistent error handling between fetch() and fetchGroup().

Suggested change

	window[callback] = (response) => {
	resolve(response);
	};
	var script = document.createElement("script");
	script.src = url;
	script.type = "text/javascript";
	script.async = true;
	script.addEventListener("error", (err) => {
	resolve();
	});
	document.getElementsByTagName("head")[0].appendChild(script);
	let script = null;
	let timeoutId = null;
	const cleanup = () => {
	if (timeoutId !== null) {
	clearTimeout(timeoutId);
	timeoutId = null;
	}
	if (script && script.parentNode) {
	script.parentNode.removeChild(script);
	}
	try {
	delete window[callback];
	} catch (e) {
	// Older browsers may not allow delete on window properties
	window[callback] = undefined;
	}
	};
	window[callback] = (response) => {
	cleanup();
	resolve(response);
	};
	script = document.createElement("script");
	script.src = url;
	script.type = "text/javascript";
	script.async = true;
	script.addEventListener("error", (err) => {
	cleanup();
	// Resolve with undefined on script error, consistent with single-placement fetch
	resolve();
	});
	document.getElementsByTagName("head")[0].appendChild(script);
	// Fallback timeout in case neither callback nor error is triggered
	timeoutId = window.setTimeout(() => {
	cleanup();
	resolve();
	}, 10000);

[Copilot]

When a prioritized placement rotates (via the rotate() method which calls load() again), it will call this.fetchPromise || this.fetch(). However, the fetchPromise was set during initial grouping, and rotate() clears this.response but not this.fetchPromise. This means a rotated priority placement will reuse the old fetchPromise, which may no longer be valid. Additionally, the rotate() method doesn't re-trigger fetchGroup for priority placements, so they'll fetch individually instead of as a group. Consider either clearing fetchPromise on rotation or handling priority placement rotation differently.

[davidfischer]

This is OK I think. What this means is that only the winning ad position can rotate. It doesn't go into a priority again with all the different ad placements.

[Copilot]

The test only covers the happy path where fetchGroup is called with two prioritized placements. Consider adding tests for edge cases such as: a single prioritized placement (should it use fetchGroup or regular fetch?), mixed publishers with priorities, priority placements with load_manually=true, and rotation behavior of priority placements. These scenarios aren't covered by the current test.

[ericholscher]

I honestly think Copilot did a better job reviewing this than I would have 🤷 It's hard for me to parse this kind of dense JS unfortunately, and it's using some syntax I'm not super familiar with.

[davidfischer]

Overall, happy with how this turned out. I was able to test a number of edge cases successfully. There's still one I'd like to test before pushing this out but it requires a server-side fix (readthedocs/ethical-ad-server#1143).

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

index.js:558

• The validation for priority checks if the value is between 1 and 10 inclusive, but the validation message "Invalid numerical priority" doesn't clarify what the valid range is. Consider updating the warning message to explicitly state the valid range, such as: "EthicalAd: Invalid numerical priority '%s' provided (must be between 1 and 10); ignoring."

        logger.warn(
          "EthicalAd: Invalid numerical priority '%s' provided; ignoring.",
          priorityAttr
        );

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When a single placement has a priority set, it still gets routed through loadGroup instead of the regular load path. This adds unnecessary overhead for the common case of a single prioritized placement on a page. The priority feature is designed for choosing between multiple placements, so consider checking if priority_group.length is greater than 1 before calling loadGroup. For a single placement, it should use the regular load path regardless of whether it has a priority set.

Suggested change

	if (priority_group.length > 0) {
	return Placement.loadGroup(priority_group);
	} else {
	return Promise.resolve([]);
	}
	} else {
	// Create main promise. Iterator `all()` Promise will surround array of
	// placements.
	return Promise.all(
	placements.map((placement) => {
	if (placement && (force_load || !placement.load_manually)) {
	return placement.load().catch((err) => {
	if (err instanceof EthicalAdsWarning) {
	logger.warn(err.message);
	} else {
	logger.error(err.message);
	}
	return null;
	});
	} else {
	return null;
	}
	})
	);
	}
	}
	if (priority_group.length > 1) {
	return Placement.loadGroup(priority_group);
	} else if (priority_group.length === 0) {
	return Promise.resolve([]);
	}
	}
	// Create main promise. Iterator `all()` Promise will surround array of
	// placements.
	return Promise.all(
	placements.map((placement) => {
	if (placement && (force_load || !placement.load_manually)) {
	return placement.load().catch((err) => {
	if (err instanceof EthicalAdsWarning) {
	logger.warn(err.message);
	} else {
	logger.error(err.message);
	}
	return null;
	});
	} else {
	return null;
	}
	})
	);
	}
	}

[davidfischer]

I verified this worked as intended after the server side fix was merged. I added docs. This is ready for review and release.