Skip to content

Commit

Permalink
web: fix resource filtering infinite loop (#5249)
Browse files Browse the repository at this point in the history
This is a temporary change to store the resource list options in
session storage instead of local storage. This should prevent odd
behavior when multiple tabs are open to the same Tilt instance and
filtering is done in one which causes an infinite loop where Tilt
phantom types characters into the filter.

Longer-term, because we use local storage elsewhere, we need to
investigate the issues with the React library and/or our usage of
it to avoid these types of problems.

For filtering, having it be synced across tabs/persistent across
Tilt sessions is not critical, so using session storage is a
reasonable mitigation for now.
  • Loading branch information
milas committed Dec 8, 2021
1 parent e3f1eaa commit 70fe4e1
Show file tree
Hide file tree
Showing 12 changed files with 77 additions and 22 deletions.
7 changes: 5 additions & 2 deletions web/src/ApiButton.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,18 @@ import {
makeUIButton,
textField,
} from "./ApiButton.testhelpers"
import { accessorsForTesting, tiltfileKeyContext } from "./BrowserStorage"
import { HudErrorContextProvider } from "./HudErrorContext"
import { InstrumentedButton } from "./instrumentedComponents"
import { accessorsForTesting, tiltfileKeyContext } from "./LocalStorage"
import { flushPromises } from "./promise"

type UIButtonStatus = Proto.v1alpha1UIButtonStatus
type UIButton = Proto.v1alpha1UIButton

const buttonInputsAccessor = accessorsForTesting(`apibutton-TestButton`)
const buttonInputsAccessor = accessorsForTesting(
`apibutton-TestButton`,
localStorage
)

function wrappedMount(e: JSX.Element) {
return mount(
Expand Down
2 changes: 1 addition & 1 deletion web/src/ApiButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ import styled from "styled-components"
import { Tags } from "./analytics"
import { annotations } from "./annotations"
import { ReactComponent as CloseSvg } from "./assets/svg/close.svg"
import { usePersistentState } from "./BrowserStorage"
import FloatDialog from "./FloatDialog"
import { useHudErrorContext } from "./HudErrorContext"
import {
InstrumentedButton,
InstrumentedCheckbox,
InstrumentedTextField,
} from "./instrumentedComponents"
import { usePersistentState } from "./LocalStorage"
import { usePathBuilder } from "./PathBuilder"
import { Color, FontSize, SizeUnit } from "./style-helpers"
import { apiTimeFormat, tiltApiPut } from "./tiltApi"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { mount } from "enzyme"
import React from "react"
import { makeKey, tiltfileKeyContext, usePersistentState } from "./LocalStorage"
import {
makeKey,
tiltfileKeyContext,
usePersistentState,
} from "./BrowserStorage"

describe("localStorageContext", () => {
afterEach(() => {
Expand Down
41 changes: 37 additions & 4 deletions web/src/LocalStorage.tsx → web/src/BrowserStorage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@ export type accessor<S> = {
set: (s: S) => void
}

export function accessorsForTesting<S>(name: string) {
export function accessorsForTesting<S>(name: string, storage: Storage) {
const key = makeKey("test", name)
function get(): S | null {
const v = localStorage.getItem(key)
const v = storage.getItem(key)
if (!v) {
return null
}
return JSON.parse(v) as S
}

function set(s: S): void {
localStorage.setItem(key, JSON.stringify(s))
storage.setItem(key, JSON.stringify(s))
}

return {
Expand All @@ -40,10 +40,43 @@ export function usePersistentState<S>(
name: string,
defaultValue: S,
maybeUpgradeSavedState?: (state: S) => S
): [state: S, setState: Dispatch<SetStateAction<S>>] {
return useBrowserStorageState(
name,
defaultValue,
localStorage,
maybeUpgradeSavedState
)
}

// Like `useState`, but backed by sessionStorage and namespaced by the tiltfileKey
// maybeUpgradeSavedState: transforms any state read from storage - allows, e.g., filling in default values for
// fields added since the state was saved
export function useSessionState<S>(
name: string,
defaultValue: S,
maybeUpgradeSavedState?: (state: S) => S
): [state: S, setState: Dispatch<SetStateAction<S>>] {
return useBrowserStorageState(
name,
defaultValue,
sessionStorage,
maybeUpgradeSavedState
)
}

// Like `useState`, but backed by localStorage and namespaced by the tiltfileKey
// maybeUpgradeSavedState: transforms any state read from storage - allows, e.g., filling in default values for
// fields added since the state was saved
export function useBrowserStorageState<S>(
name: string,
defaultValue: S,
storage: Storage,
maybeUpgradeSavedState?: (state: S) => S
): [state: S, setState: Dispatch<SetStateAction<S>>] {
const tiltfileKey = useContext(tiltfileKeyContext)
let [state, setState] = useStorageState<S>(
localStorage,
storage,
makeKey(tiltfileKey, name),
defaultValue
)
Expand Down
2 changes: 1 addition & 1 deletion web/src/HUD.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { Route, RouteComponentProps, Switch } from "react-router-dom"
import { incr, navigationToTags } from "./analytics"
import AnalyticsNudge from "./AnalyticsNudge"
import AppController from "./AppController"
import { tiltfileKeyContext } from "./BrowserStorage"
import ErrorModal from "./ErrorModal"
import FatalErrorModal from "./FatalErrorModal"
import Features, { FeaturesProvider, Flag } from "./feature"
Expand All @@ -15,7 +16,6 @@ import "./HUD.scss"
import { HudErrorContextProvider } from "./HudErrorContext"
import HudState from "./HudState"
import { InterfaceVersion, useInterfaceVersion } from "./InterfaceVersion"
import { tiltfileKeyContext } from "./LocalStorage"
import LogStore, { LogStoreProvider } from "./LogStore"
import OverviewResourcePane from "./OverviewResourcePane"
import OverviewTablePane from "./OverviewTablePane"
Expand Down
5 changes: 3 additions & 2 deletions web/src/OverviewSidebarOptions.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
cleanupMockAnalyticsCalls,
mockAnalyticsCalls,
} from "./analytics_test_helpers"
import { accessorsForTesting, tiltfileKeyContext } from "./LocalStorage"
import { accessorsForTesting, tiltfileKeyContext } from "./BrowserStorage"
import {
TestsWithErrors,
TwoResourcesTwoTests,
Expand All @@ -26,7 +26,8 @@ import SidebarResources, { SidebarListSection } from "./SidebarResources"
import { StarredResourcesContextProvider } from "./StarredResourcesContext"

const resourceListOptionsAccessor = accessorsForTesting<ResourceListOptions>(
RESOURCE_LIST_OPTIONS_KEY
RESOURCE_LIST_OPTIONS_KEY,
sessionStorage
)

export function assertSidebarItemsAndOptions(
Expand Down
3 changes: 3 additions & 0 deletions web/src/OverviewTable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ const findTableColumnByName = (
// End helpers

afterEach(() => {
sessionStorage.clear()
localStorage.clear()
})

Expand Down Expand Up @@ -353,11 +354,13 @@ describe("overview table with groups", () => {
)

mockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

afterEach(() => {
cleanupMockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

Expand Down
2 changes: 1 addition & 1 deletion web/src/ResourceGroupsContext.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createContext, PropsWithChildren, useContext } from "react"
import { AnalyticsAction, AnalyticsType, incr } from "./analytics"
import { usePersistentState } from "./LocalStorage"
import { usePersistentState } from "./BrowserStorage"

export type GroupState = { expanded: boolean }

Expand Down
4 changes: 2 additions & 2 deletions web/src/ResourceListOptionsContext.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createContext, PropsWithChildren, useContext } from "react"
import { usePersistentState } from "./LocalStorage"
import { useSessionState } from "./BrowserStorage"

/**
* The ResourceListOptions state keeps track of filters and sorting
Expand Down Expand Up @@ -55,7 +55,7 @@ export function ResourceListOptionsProvider(
...DEFAULT_OPTIONS,
}
const [options, setResourceListOptions] =
usePersistentState<ResourceListOptions>(
useSessionState<ResourceListOptions>(
RESOURCE_LIST_OPTIONS_KEY,
defaultPersistentValue,
maybeUpgradeSavedOptions
Expand Down
8 changes: 6 additions & 2 deletions web/src/ResourceNameFilter.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
expectIncrs,
mockAnalyticsCalls,
} from "./analytics_test_helpers"
import { accessorsForTesting, tiltfileKeyContext } from "./LocalStorage"
import { accessorsForTesting, tiltfileKeyContext } from "./BrowserStorage"
import {
DEFAULT_OPTIONS,
ResourceListOptions,
Expand All @@ -21,7 +21,8 @@ import {
} from "./ResourceNameFilter"

const resourceListOptionsAccessor = accessorsForTesting<ResourceListOptions>(
RESOURCE_LIST_OPTIONS_KEY
RESOURCE_LIST_OPTIONS_KEY,
sessionStorage
)

const ResourceNameFilterTestWrapper = () => (
Expand All @@ -37,10 +38,13 @@ const ResourceNameFilterTestWrapper = () => (
describe("ResourceNameFilter", () => {
beforeEach(() => {
mockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

afterEach(() => {
cleanupMockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

Expand Down
17 changes: 12 additions & 5 deletions web/src/SidebarResources.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {
expectIncrs,
mockAnalyticsCalls,
} from "./analytics_test_helpers"
import { accessorsForTesting, tiltfileKeyContext } from "./BrowserStorage"
import Features, { FeaturesProvider, Flag } from "./feature"
import { accessorsForTesting, tiltfileKeyContext } from "./LocalStorage"
import LogStore from "./LogStore"
import { AlertsOnTopToggle } from "./OverviewSidebarOptions"
import { assertSidebarItemsAndOptions } from "./OverviewSidebarOptions.test"
Expand Down Expand Up @@ -43,9 +43,13 @@ import { ResourceStatus, ResourceView } from "./types"
let pathBuilder = PathBuilder.forTesting("localhost", "/")

const resourceListOptionsAccessor = accessorsForTesting<ResourceListOptions>(
RESOURCE_LIST_OPTIONS_KEY
RESOURCE_LIST_OPTIONS_KEY,
sessionStorage
)
const starredItemsAccessor = accessorsForTesting<string[]>(
"pinned-resources",
localStorage
)
const starredItemsAccessor = accessorsForTesting<string[]>("pinned-resources")

const SidebarResourcesTestWrapper = ({
items,
Expand Down Expand Up @@ -95,10 +99,13 @@ function clickStar(
describe("SidebarResources", () => {
beforeEach(() => {
mockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

afterEach(() => {
cleanupMockAnalyticsCalls()
sessionStorage.clear()
localStorage.clear()
})

Expand Down Expand Up @@ -207,7 +214,7 @@ describe("SidebarResources", () => {
],
]
test.each(loadCases)(
"loads %p from localStorage",
"loads %p from browser storage",
(name, options, expectedItems) => {
resourceListOptionsAccessor.set(options)

Expand Down Expand Up @@ -248,7 +255,7 @@ describe("SidebarResources", () => {
["resourceNameFilter", { ...DEFAULT_OPTIONS, resourceNameFilter: "foo" }],
]
test.each(saveCases)(
"saves option %s to localStorage",
"saves option %s to browser storage",
(name, expectedOptions) => {
let ls = new LogStore()
const items = [
Expand Down
2 changes: 1 addition & 1 deletion web/src/StarredResourcesContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import React, {
useState,
} from "react"
import { AnalyticsAction, incr } from "./analytics"
import { usePersistentState } from "./LocalStorage"
import { usePersistentState } from "./BrowserStorage"

export type StarredResourcesContext = {
starredResources: string[]
Expand Down

0 comments on commit 70fe4e1

Please sign in to comment.