Move JS filters to Session storage #2016
Conversation
sago2k8
left a comment
sago2k8
left a comment
There was a problem hiding this comment.
left couple of comments, It would be nice to implement a small test also.
| */ | ||
| function getAllFilters(): TopicFiltersMap { | ||
| try { | ||
| const stored = sessionStorage.getItem(SESSION_STORAGE_KEY); |
There was a problem hiding this comment.
Is sessionStorage a global variable ?
There was a problem hiding this comment.
It's a globally accessible object we can rely on
There was a problem hiding this comment.
could we validate the session storage works in secure environments, I assume it does. But I would like to double check
There was a problem hiding this comment.
what do you mean by secure env?
There was a problem hiding this comment.
I meant https, I think the browser might require and extra config. it's possible that the sessionStorage object handles this for you.
| import { uiState } from '../../../../../state/ui-state'; | ||
|
|
||
| export const MessageSearchFilterBar: FC<{ onEdit: (filter: FilterEntry) => void }> = observer(({ onEdit }) => { | ||
| const settings = uiState.topicSettings.searchParams; |
There was a problem hiding this comment.
I understand the change was to replace the global state by sessions storage, could we use zustand to manage this global state and use the persistence layer https://redpandadata.slack.com/archives/C063X3UEWCA/p1762753166216419
There was a problem hiding this comment.
I don't follow, what is the advantage of this? Session storage is persisted per reloads and only gets erased once user close the tab. Having zustand in between does't really make sense. UX doesn't want to have any kind of persistance here.
There was a problem hiding this comment.
Yes my point is if I want to use a global state that happens to be stored in Session Storage, so maybe if there is a known pattern of handling this global state it will be easier to maintain as opposed to a solution only for this page.
https://redpanda.zoom.us/clips/share/Qlro4VR9RWyzLJnb33pLaA