mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 17:35:10 +03:00
fix(studio): guard no-op advisor dismissal localStorage updates (#45031)
## 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 -->
This commit is contained in:
1 parent
2eab2d6cdc
commit
2349f76e18
2 files changed
+23
-9
No files matched your search
@@ -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)),
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user