codeant-ai-for-open-source[bot] commented on code in PR #36743:
URL: https://github.com/apache/superset/pull/36743#discussion_r2631923339
##########
docs/src/theme/Root.js:
##########
@@ -27,10 +46,9 @@ export default function Root({ children }) {
const { matomoUrl, matomoSiteId } = customFields;
if (typeof window !== 'undefined') {
- // Making testing easier, logging debug junk if we're in development
- const devMode = window.location.hostname === 'localhost' ? true : false;
+ const devMode = window.location.hostname === 'localhost';
Review Comment:
**Suggestion:** The `devMode` check only considers `localhost`, so when the
docs are served from other common local hostnames like `127.0.0.1` or `::1`,
full Matomo tracking will still run, leading to polluted analytics from local
development sessions. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
const devMode = ['localhost', '127.0.0.1', '0.0.0.0', '::1'].includes(
window.location.hostname,
);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Valid: many local dev setups use 127.0.0.1, ::1, or 0.0.0.0. Relying only on
'localhost' will allow tracking during local development in those cases,
polluting analytics. Expanding the check to include common local hostnames/IPs
prevents that and is low risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/src/theme/Root.js
**Line:** 49:49
**Comment:**
*Possible Bug: The `devMode` check only considers `localhost`, so when
the docs are served from other common local hostnames like `127.0.0.1` or
`::1`, full Matomo tracking will still run, leading to polluted analytics from
local development sessions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/src/theme/Root.js:
##########
@@ -108,11 +218,19 @@ export default function Root({ children }) {
window.addEventListener('popstate', handleRouteChange);
Review Comment:
**Suggestion:** The Docusaurus route update listeners are added with
anonymous callback functions but removed using `handleRouteChange`, which never
matches the originally registered handlers, so the listeners persist after
unmount and can cause duplicate page view tracking and memory leaks if the
component is mounted/unmounted multiple times. [resource leak]
**Severity Level:** Minor ⚠️
```suggestion
const routeUpdateHandlers = {};
if (devMode) {
console.log('Setting up Matomo tracking with enhanced features');
}
possibleEvents.forEach(eventName => {
const handler = () => {
if (devMode) {
console.log(`Docusaurus route update detected via ${eventName}`);
}
handleRouteChange();
};
routeUpdateHandlers[eventName] = handler;
document.addEventListener(eventName, handler);
});
// Cleanup
return () => {
possibleEvents.forEach(eventName => {
const handler = routeUpdateHandlers[eventName];
if (handler) {
document.removeEventListener(eventName, handler);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Correct: the code registers anonymous handler functions but attempts to
remove them by passing handleRouteChange to removeEventListener, which will not
match the original references. That causes listeners to persist across unmounts
and can lead to duplicate tracking and memory leaks. The improved approach
(store references and remove the same handlers) fixes a real
resource-leak/behavior bug.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/src/theme/Root.js
**Line:** 198:219
**Comment:**
*Resource Leak: The Docusaurus route update listeners are added with
anonymous callback functions but removed using `handleRouteChange`, which never
matches the originally registered handlers, so the listeners persist after
unmount and can cause duplicate page view tracking and memory leaks if the
component is mounted/unmounted multiple times.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
docs/src/theme/Root.js:
##########
@@ -39,27 +57,121 @@ export default function Root({ children }) {
window._paq.push(['setTrackerUrl', `${matomoUrl}/matomo.php`]);
window._paq.push(['setSiteId', matomoSiteId]);
- // Initial page view is handled by handleRouteChange
+ // Track downloads with custom extensions
+ window._paq.push(['setDownloadExtensions',
DOWNLOAD_EXTENSIONS.join('|')]);
// Now load the matomo.js script
const script = document.createElement('script');
script.async = true;
script.src = `${matomoUrl}/matomo.js`;
document.head.appendChild(script);
+ // Helper to track events
+ const trackEvent = (category, action, name, value) => {
+ if (devMode) {
+ console.log('Matomo trackEvent:', { category, action, name, value });
+ }
+ window._paq.push(['trackEvent', category, action, name, value]);
+ };
+
+ // Helper to track site search
+ const trackSiteSearch = (keyword, category, resultsCount) => {
+ if (devMode) {
+ console.log('Matomo trackSiteSearch:', { keyword, category,
resultsCount });
+ }
+ window._paq.push(['trackSiteSearch', keyword, category, resultsCount]);
+ };
+
+ // Track external link clicks with categorization
+ const handleLinkClick = (event) => {
+ const link = event.target.closest('a');
+ if (!link) return;
+
+ const href = link.getAttribute('href');
+ if (!href) return;
+
+ try {
+ const url = new URL(href, window.location.origin);
+
+ // Skip internal links
+ if (url.hostname === window.location.hostname) return;
+
+ // Determine category based on domain
+ let category = 'External Link';
+ for (const [domain, cat] of
Object.entries(EXTERNAL_LINK_CATEGORIES)) {
+ if (url.hostname.includes(domain)) {
+ category = cat;
+ break;
+ }
+ }
+
+ trackEvent('Outbound Link', category, href);
+ } catch {
+ // Invalid URL, skip tracking
+ }
+ };
+
+ // Track Algolia search queries
+ const setupAlgoliaTracking = () => {
+ // Watch for Algolia search modal
+ const observer = new MutationObserver((mutations) => {
+ mutations.forEach((mutation) => {
+ mutation.addedNodes.forEach((node) => {
+ if (node.nodeType === Node.ELEMENT_NODE) {
+ // Check for Algolia search input
+ const searchInput = node.querySelector?.('.DocSearch-Input') ||
+
(node.classList?.contains('DocSearch-Input') ? node : null);
+ if (searchInput) {
+ let debounceTimer;
+ searchInput.addEventListener('input', (e) => {
+ clearTimeout(debounceTimer);
+ debounceTimer = setTimeout(() => {
+ const query = e.target.value.trim();
+ if (query.length >= 3) {
+ // Get result count if available
+ const results =
document.querySelectorAll('.DocSearch-Hit');
+ trackSiteSearch(query, 'Documentation', results.length
|| false);
Review Comment:
**Suggestion:** In the Algolia search tracking, using `results.length ||
false` incorrectly treats searches with zero results as "unknown result count"
instead of 0, which skews the analytics data for Matomo site search reports.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
trackSiteSearch(query, 'Documentation',
results.length);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accurate: results.length || false converts 0 to false which signals
"unknown" rather than the actual 0 count. That skews Matomo analytics for
zero-result queries. Passing results.length (an integer) preserves correct
counts and is a straightforward fix.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/src/theme/Root.js
**Line:** 130:133
**Comment:**
*Logic Error: In the Algolia search tracking, using `results.length ||
false` incorrectly treats searches with zero results as "unknown result count"
instead of 0, which skews the analytics data for Matomo site search reports.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
