fix: render insight text as nodes, not innerHTML, and drop stale requests (#1053)

src/components/AnalyticsInsights.insights.test.jsx

@@ -0,0 +1,156 @@
+import { render, screen, waitFor } from '@testing-library/react';
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+
+/**
+ * Cover for #1053, at the component level.
+ *
+ * `aiInsightsService` is mocked so these exercise the panel's own handling —
+ * escaping and request ordering — without a year of archive data behind them.
+ */
+
+const generateAIInsights = vi.fn();
+vi.mock('../services/aiInsightsService', () => ({
+  generateAIInsights: (...args) => generateAIInsights(...args),
+}));
+
+vi.mock('recharts', async () => {
+  const Stub = ({ children }) => <div>{children}</div>;
+  return {
+    AreaChart: Stub, Area: Stub, XAxis: Stub, YAxis: Stub,
+    Tooltip: Stub, CartesianGrid: Stub, ResponsiveContainer: Stub,
+  };
+});
+
+vi.mock('../utils/chartExport', () => ({
+  exportToSVG: vi.fn(() => true),
+  exportToPNG: vi.fn(() => Promise.resolve()),
+}));
+
+const AnalyticsInsights = (await import('./AnalyticsInsights')).default;
+
+/** @param {Partial<any>} [overrides] */
+function insight(overrides = {}) {
+  return {
+    id: 'seasonal-extremes',
+    icon: '📅',
+    title: 'Seasonal Extremes',
+    description: 'Based on the past year in Delhi, **January 2026** was worst.',
+    confidence: 'High',
+    source: 'Historical Data Aggregation',
+    ...overrides,
+  };
+}
+
+/** A promise plus the functions that settle it, for ordering tests. */
+function deferred() {
+  let resolve;
+  const promise = new Promise((r) => { resolve = r; });
+  return { promise, resolve };
+}
+
+describe('AnalyticsInsights — insight text is escaped (#1053)', () => {
+  beforeEach(() => {
+    generateAIInsights.mockReset();
+  });
+
+  afterEach(() => {
+    vi.clearAllMocks();
+  });
+
+  it('renders the bold marker as an element and everything else as text', async () => {
+    generateAIInsights.mockResolvedValue({ insights: [insight()], error: null });
+
+    render(<AnalyticsInsights lat={28.6} lon={77.2} cityName="Delhi" />);
+
+    await waitFor(() => expect(screen.getByText('January 2026').tagName).toBe('STRONG'));
+  });
+
+  it('does not build elements from a hostile location name', async () => {
+    // `cityName` reaches `description` by interpolation in aiInsightsService,
+    // and comes from the geocoder's answer to text the visitor typed.
+    generateAIInsights.mockResolvedValue({
+      insights: [
+        insight({
+          description:
+            'Based on the past year in <img src=x onerror="window.__xss=1">, **June** was cleanest.',
+        }),
+      ],
+      error: null,
+    });
+
+    const { container } = render(
+      <AnalyticsInsights lat={28.6} lon={77.2} cityName='<img src=x onerror="window.__xss=1">' />
+    );
+
+    await waitFor(() => expect(screen.getByText('June').tagName).toBe('STRONG'));
+
+    expect(container.querySelector('img')).toBeNull();
+    expect(container.querySelector('script')).toBeNull();
+    expect(window.__xss).toBeUndefined();
+  });
+
+  it('does not throw when an insight arrives without a description', async () => {
+    generateAIInsights.mockResolvedValue({
+      insights: [insight({ description: undefined })],
+      error: null,
+    });
+
+    render(<AnalyticsInsights lat={28.6} lon={77.2} cityName="Delhi" />);
+
+    // The title still renders; only the body is empty.
+    await waitFor(() => expect(screen.getByText('Seasonal Extremes')).toBeInTheDocument());
+  });
+});
+
+describe('AnalyticsInsights — stale requests (#1053)', () => {
+  beforeEach(() => {
+    generateAIInsights.mockReset();
+  });
+
+  it('ignores a response for a city that is no longer selected', async () => {
+    const delhi = deferred();
+    const mumbai = deferred();
+    generateAIInsights
+      .mockImplementationOnce(() => delhi.promise)
+      .mockImplementationOnce(() => mumbai.promise);
+
+    const { rerender } = render(
+      <AnalyticsInsights lat={28.6} lon={77.2} cityName="Delhi" />
+    );
+
+    rerender(<AnalyticsInsights lat={19.0} lon={72.8} cityName="Mumbai" />);
+
+    // Mumbai answers first, then Delhi's slower request lands.
+    mumbai.resolve({ insights: [insight({ id: 'm', title: 'Mumbai insight' })], error: null });
+    await waitFor(() => expect(screen.getByText('Mumbai insight')).toBeInTheDocument());
+
+    delhi.resolve({ insights: [insight({ id: 'd', title: 'Delhi insight' })], error: null });
+
+    await waitFor(() => expect(screen.getByText('Mumbai insight')).toBeInTheDocument());
+    expect(screen.queryByText('Delhi insight')).not.toBeInTheDocument();
+  });
+
+  it('does not apply a late error over the current city', async () => {
+    const first = deferred();
+    const second = deferred();
+    generateAIInsights
+      .mockImplementationOnce(() => first.promise)
+      .mockImplementationOnce(() => second.promise);
+
+    const { rerender } = render(<AnalyticsInsights lat={28.6} lon={77.2} cityName="Delhi" />);
+    rerender(<AnalyticsInsights lat={19.0} lon={72.8} cityName="Mumbai" />);
+
+    second.resolve({ insights: [insight({ id: 'm', title: 'Mumbai insight' })], error: null });
+    await waitFor(() => expect(screen.getByText('Mumbai insight')).toBeInTheDocument());
+
+    first.resolve({ insights: [], error: 'No historical data available.' });
+
+    await waitFor(() => expect(screen.getByText('Mumbai insight')).toBeInTheDocument());
+    expect(screen.queryByText('No historical data available.')).not.toBeInTheDocument();
+  });
+
+  it('does not fetch at all without coordinates', () => {
+    render(<AnalyticsInsights cityName="Delhi" />);
+    expect(generateAIInsights).not.toHaveBeenCalled();
+  });
+});

src/components/AnalyticsInsights.jsx

@@ -4,6 +4,7 @@ import { useMemo, useRef, useState, useEffect } from 'react';
 import PropTypes from 'prop-types';
 import { exportToSVG, exportToPNG } from '../utils/chartExport';
 import { generateAIInsights } from '../services/aiInsightsService';
+import { renderBoldMarkup } from '../utils/boldMarkup';
 
 /** Fallback when `timeRange` arrives missing or nonsensical. Matches the dashboard default. */
 const DEFAULT_TIME_RANGE = 24;
@@ -81,24 +82,42 @@ export default function AnalyticsInsights({ analytics = {}, trend = [], timeRang
   const [insightsError, setInsightsError] = useState(null);
 
   useEffect(() => {
-    if (lat != null && lon != null) {
-      setLoadingInsights(true);
-      setInsightsError(null);
-      generateAIInsights(lat, lon, cityName)
-        .then(result => {
-          if (result.error) {
-            setInsightsError(result.error);
-          } else {
-            setInsights(result.insights);
-          }
-        })
-        .catch(err => {
-          setInsightsError(err.message || 'Error fetching insights');
-        })
-        .finally(() => {
-          setLoadingInsights(false);
-        });
-    }
+    if (lat == null || lon == null) return undefined;
+
+    // `generateAIInsights` goes through `fetchHistoricalData` — a year of hourly
+    // archive data, slow on a cold cache. Without this flag, searching Delhi and
+    // then Mumbai leaves two requests in flight and whichever resolves last
+    // wins: Delhi's insights can end up under Mumbai's heading with
+    // `loadingInsights` already false, so nothing signals it. It also stops the
+    // three setState calls below firing after unmount.
+    let current = true;
+
+    setLoadingInsights(true);
+    setInsightsError(null);
+    // The previous city's insights are not an answer about this one, so they go
+    // now rather than lingering behind the spinner.
+    setInsights([]);
+
+    generateAIInsights(lat, lon, cityName)
+      .then(result => {
+        if (!current) return;
+        if (result?.error) {
+          setInsightsError(result.error);
+        } else {
+          setInsights(Array.isArray(result?.insights) ? result.insights : []);
+        }
+      })
+      .catch(err => {
+        if (!current) return;
+        setInsightsError(err?.message || 'Error fetching insights');
+      })
+      .finally(() => {
+        if (current) setLoadingInsights(false);
+      });
+
+    return () => {
+      current = false;
+    };
   }, [lat, lon, cityName]);
 
   const range = Number.isFinite(timeRange) && timeRange > 0 ? timeRange : DEFAULT_TIME_RANGE;
@@ -197,10 +216,13 @@ export default function AnalyticsInsights({ analytics = {}, trend = [], timeRang
                     </span>
                   </div>
                 </div>
-                <p 
+                <p
                   style={{ margin: '0 0 0.75rem 0', fontSize: '0.9rem', color: 'var(--text-secondary, #475569)', lineHeight: 1.5 }}
-                  dangerouslySetInnerHTML={{ __html: insight.description.replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>') }} 
-                />
+                >
+                  {/* Nodes, not innerHTML. `description` interpolates the
+                      location name, which comes from the geocoder — see #1053. */}
+                  {renderBoldMarkup(insight.description)}
+                </p>
                 <div style={{ fontSize: '0.75rem', color: 'var(--muted, #94a3b8)', borderTop: '1px solid var(--line, #f1f5f9)', paddingTop: '0.5rem' }}>
                   Source: {insight.source}
                 </div>

src/utils/boldMarkup.jsx

@@ -0,0 +1,70 @@
+/**
+ * Renders the `**bold**` convention as React nodes.
+ *
+ * `AnalyticsInsights` used to do this by writing HTML:
+ *
+ *   dangerouslySetInnerHTML={{
+ *     __html: insight.description.replace(/\*\*(.*?)\*\*\/g, '<strong>$1</strong>')
+ *   }}
+ *
+ * The `replace` only rewrites the `**` markers, but the *whole* string is then
+ * handed to `innerHTML`, so every other character in it is interpreted as markup
+ * too. `insight.description` is built by interpolating the location name, which
+ * comes from the geocoder's answer to text the visitor typed — third-party data
+ * on a path to `innerHTML`, which is the definition of an XSS sink (#1053).
+ *
+ * Producing nodes instead means React escapes the text between the markers, and
+ * the bold segments, the way it escapes everything else in the component. There
+ * is no string of HTML at any point, so there is nothing to get the escaping of
+ * wrong. `CommunityHub` reached the same conclusion after #497, and its comment
+ * now reads "component never uses dangerouslySetInnerHTML".
+ */
+
+/** Matches a `**...**` span. Lazy, so `**a** and **b**` is two spans, not one. */
+const BOLD_PATTERN = /\*\*([\s\S]+?)\*\*/g;
+
+/**
+ * Splits `text` into plain strings and `<strong>` elements.
+ *
+ * Unmatched markers are left as literal text rather than guessed at: a lone `**`
+ * in a sentence is far more likely to be punctuation than an unclosed tag, and
+ * the alternative is bolding the entire rest of the string.
+ *
+ * @param {unknown} text
+ * @returns {import('react').ReactNode[]} Empty when there is nothing to render.
+ */
+export function renderBoldMarkup(text) {
+  // A non-string is not an error worth throwing over. The previous code called
+  // `.replace` straight on the value, so an insight pushed without a description
+  // took the whole panel down with a TypeError.
+  if (typeof text !== 'string' || text.length === 0) return [];
+
+  /** @type {import('react').ReactNode[]} */
+  const nodes = [];
+  let lastIndex = 0;
+  let key = 0;
+
+  // A fresh regex per call: BOLD_PATTERN is global, and `lastIndex` persists
+  // between calls on a shared instance, so the second insight would start
+  // matching from wherever the first one left off.
+  const pattern = new RegExp(BOLD_PATTERN.source, 'g');
+
+  let match;
+  while ((match = pattern.exec(text)) !== null) {
+    if (match.index > lastIndex) {
+      nodes.push(text.slice(lastIndex, match.index));
+    }
+    nodes.push(<strong key={`bold-${key++}`}>{match[1]}</strong>);
+    lastIndex = match.index + match[0].length;
+
+    // `**` with nothing between it matches zero-width in some engines; step on
+    // so the loop cannot spin.
+    if (match[0].length === 0) pattern.lastIndex++;
+  }
+
+  if (lastIndex < text.length) {
+    nodes.push(text.slice(lastIndex));
+  }
+
+  return nodes;
+}