From 2349f76e18250e766d74cefcdc9e2084a54bed12 Mon Sep 17 00:00:00 2001
From: Danny White <3104761+dnywh@users.noreply.github.com>
Date: Mon, 20 Apr 2026 18:41:32 +1000
Subject: [PATCH] fix(studio): guard no-op advisor dismissal localStorage
 updates (#45031)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## I have read the
[CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md)
file.

YES

## What kind of change does this PR introduce?

Bug fix.

## What is the current behavior?

Advisor dismissals use `useLocalStorageQuery`. When advisor signals
pruning ran, it sometimes invoked `setDismissedKeys` even when nothing
needed to change (no-op updater returning the same array reference).

Separately, `useLocalStorageQuery` would still persist +
`invalidateQueries` even when the computed next value was
reference-equal to the current cached value.

When `useAdvisorSignals` is mounted in two places at once
(`AdvisorSection` + `AdvisorPanel`), those redundant invalidations /
subscriber churn could occasionally cascade into React’s “Maximum update
depth exceeded” error (often surfaced via Radix `composeRefs` in stack
traces). CI saw this as an unhandled error during
`AdvisorSignals.integration.test.tsx`.

## What is the new behavior?

- `useLocalStorageQuery` now **early-returns** when `Object.is(next,
current)` so no-op updates don’t write localStorage or invalidate the
query.
- `useAdvisorSignals` pruning effect now **short-circuits** unless there
is actually a stale banned-IP dismissal to remove.

## Additional context

Follow-up from #44372 (advisor signal items for banned IPs).

Tests run locally:

- `pnpm --filter studio exec vitest run
components/ui/AdvisorPanel/useAdvisorSignals.test.tsx
components/ui/AdvisorPanel/AdvisorSignals.integration.test.tsx
hooks/misc/__tests__/useLocalStorageQuery.test.ts`


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Bug Fixes**
* Enhanced handling of dismissed security alerts by preventing
unnecessary state updates for stale dismissals, significantly reducing
overhead and improving overall application performance.
* Optimized local storage operations to skip redundant writes to storage
and prevent triggering unnecessary cache updates and query invalidations
when stored data values remain unchanged from the previous operation.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---
 .../ui/AdvisorPanel/useAdvisorSignals.ts      | 24 ++++++++++++-------
 apps/studio/hooks/misc/useLocalStorage.ts     |  8 +++++++
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/apps/studio/components/ui/AdvisorPanel/useAdvisorSignals.ts b/apps/studio/components/ui/AdvisorPanel/useAdvisorSignals.ts
index d5ea84f9e42..08592d73b24 100644
--- a/apps/studio/components/ui/AdvisorPanel/useAdvisorSignals.ts
+++ b/apps/studio/components/ui/AdvisorPanel/useAdvisorSignals.ts
@@ -72,19 +72,25 @@ export const useAdvisorSignals = ({ projectRef, enabled = true }: UseAdvisorSign
     [projectRef, data]
   )
 
-  // Prune stale dismissals when the active signal list changes (e.g. an IP was unbanned)
+  // Prune stale dismissals when the active signal list changes (e.g. an IP was unbanned).
+  // Only call the setter when pruning would actually change something — otherwise we
+  // churn subscribers unnecessarily, which can cause feedback loops when this hook is
+  // mounted in more than one place (AdvisorSection + AdvisorPanel).
   useEffect(() => {
     if (!data) return
 
-    const activeKeys = new Set(signalItems.map((item) => item.dismissalKey))
+    const hasStaleBannedIPDismissal = dismissedKeys.some(
+      (key) =>
+        key.startsWith('signal:banned-ip:') &&
+        !signalItems.some((item) => item.dismissalKey === key)
+    )
+    if (!hasStaleBannedIPDismissal) return
 
-    setDismissedKeys((current) => {
-      const next = current.filter((key) =>
-        key.startsWith('signal:banned-ip:') ? activeKeys.has(key) : true
-      )
-      return next.length === current.length ? current : next
-    })
-  }, [data, signalItems, setDismissedKeys])
+    const activeKeys = new Set(signalItems.map((item) => item.dismissalKey))
+    setDismissedKeys((current) =>
+      current.filter((key) => (key.startsWith('signal:banned-ip:') ? activeKeys.has(key) : true))
+    )
+  }, [data, signalItems, dismissedKeys, setDismissedKeys])
 
   const formattedData = useMemo(
     () => signalItems.filter((item) => !dismissedKeySet.has(item.dismissalKey)),
diff --git a/apps/studio/hooks/misc/useLocalStorage.ts b/apps/studio/hooks/misc/useLocalStorage.ts
index f9fa942195a..73bce8d7c58 100644
--- a/apps/studio/hooks/misc/useLocalStorage.ts
+++ b/apps/studio/hooks/misc/useLocalStorage.ts
@@ -86,6 +86,14 @@ export function useLocalStorageQuery<T>(key: string, initialValue: T) {
       const currentValue = queryClient.getQueryData<T>(queryKey) ?? initialValue
       const valueToStore = value instanceof Function ? value(currentValue) : value
 
+      // Bail out when the value is unchanged (matches useState semantics).
+      // Without this, no-op updates from consumers — like a pruning effect
+      // whose updater returns `current` unchanged — still write to
+      // localStorage and invalidate the query, which churns subscribers and
+      // can cascade into "Maximum update depth exceeded" when two consumers
+      // of the same key are mounted together.
+      if (Object.is(valueToStore, currentValue)) return
+
       if (typeof window !== 'undefined') {
         window.localStorage.setItem(key, JSON.stringify(valueToStore))
       }