Skip to content

LB-1761 : Save Fresh Releases filters in browser storage #3281

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

LB-1761 : Save Fresh Releases filters in browser storage #3281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1832aa
Filter states stored in localStorage
Mshahnawaz1 May 14, 2025
2cb0048
added button to clear
Mshahnawaz1 May 15, 2025
2c15f5c
used localForage
Mshahnawaz1 May 18, 2025
f7eb673
removed showrelease from props
Mshahnawaz1 May 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions frontend/js/src/explore/fresh-releases/components/ReleaseFilters.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
filterRangeOption,
} from "../FreshReleases";
import { PAGE_TYPE_SITEWIDE, filterRangeOptions } from "../FreshReleases";
import useFilterPersistence from "../../../hooks/userPersistanceFilter";

const VARIOUS_ARTISTS_MBID = "89ad4ac3-39f7-470e-963a-56509c546377";

Expand Down Expand Up @@ -148,16 +149,36 @@ export default function ReleaseFilters(props: ReleaseFiltersProps) {
)
: Object.values(filterRangeOptions);

const { clearSavedFilters } = useFilterPersistence({
checkedList,
releaseTagsCheckList,
releaseTagsExcludeCheckList,
includeVariousArtists,
coverartOnly,
setCheckedList,
setReleaseTagsCheckList,
setReleaseTagsExcludeCheckList,
setIncludeVariousArtists,
setCoverartOnly,
});

const hasMounted = React.useRef(false);
// Reset filters when range changes
React.useEffect(() => {
if (coverartOnly === true) {
setCoverartOnly(false);
}
if (checkedList?.length > 0) {
setCheckedList([]);
}
if (includeVariousArtists === true) {
setIncludeVariousArtists(false);
if (hasMounted.current) {
// Reset filters when releaseTags or releaseTypes change (but not on first render)
if (coverartOnly === true) {
setCoverartOnly(false);
}
if (checkedList?.length > 0) {
setCheckedList([]);
}
if (includeVariousArtists === true) {
setIncludeVariousArtists(false);
}
clearSavedFilters();
} else {
hasMounted.current = true;
}
}, [releaseTags, releaseTypes]);

Expand Down Expand Up @@ -223,6 +244,14 @@ export default function ReleaseFilters(props: ReleaseFiltersProps) {
</p>
</div>
<div className="sidenav-content-grid">
<button
onClick={clearSavedFilters}
type="button"
className="btn btn-default btn-reset-filters"
style={{ margin: "10px 0" }}
>
Reset All Filters
</button>
<div
onClick={toggleFilters}
onKeyDown={(e) => {
Expand Down
119 changes: 119 additions & 0 deletions frontend/js/src/hooks/userPersistanceFilter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
import { useEffect } from "react";
import localforage from "localforage";

const release_filters_cache = localforage.createInstance({
name: "listenbrainz",
driver: [localforage.INDEXEDDB, localforage.LOCALSTORAGE],
storeName: "fresh-releases",
});
const RELEASE_FILTERS_STORAGE_KEY = "release-filters";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the core of your issue with the filters resetting when you change between "for you" and general releases.

I think the ideal solution is to save the settings separately (more flexibility for users), using this key as a prefix, and adding the pageType as a suffix, something like:

key = `${RELEASE_FILTERS_STORAGE_KEY}-${pageType}`;

Which will result in the keys "release-filters-user" and "release-filters-sitewide" to store the filters separately.


interface StoredFilters {
checkedList: Array<string | undefined>;
releaseTagsCheckList: Array<string | undefined>;
releaseTagsExcludeCheckList: Array<string | undefined>;
includeVariousArtists: boolean;
coverartOnly: boolean;
}

interface UseFilterPersistenceParams {
checkedList: Array<string | undefined>;
releaseTagsCheckList: Array<string | undefined>;
releaseTagsExcludeCheckList: Array<string | undefined>;
includeVariousArtists: boolean;
coverartOnly: boolean;

setCheckedList: (list: Array<string | undefined>) => void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a cumbersome way to go about saving the filters state, and makes it harder to extend in the future.

I think the hook should instead expose a freshReleasesFilters object with all the filters, a setFreshReleasesFilters function that accepts an object of type StoredFilters, as well as your existing clearSavedFilters .

What this means is moving the state management of these filter options inside this hook component instead of in the ReleaseFilters component instead of passing the values and their respective setters to the hook.

In ReleaseFilters you can use the values exposed by the hook, something like :

const { freshReleasesFilters, setFreshReleasesFilters, clearSavedFilters } = useFilterPersistence();
const {
    checkedList,
    releaseTagsCheckList,
    releaseTagsExcludeCheckList,
    includeVariousArtists,
    coverartOnly
  } = freshReleasesFilters

The other approach is to create a hook that only persists one value, and use that hook for each filter value instead of useState.
Something along the lines of

function usePersistentState(key, defaultValue) {
  const [value, setValue] = useState(() => {
    const storedValue = // get value from localForage here
    return storedValue ?? defaultValue;
  });

  useEffect(() => {
     // set value using localForage here
  }, [key, value]);

  return [value, setValue];
}

And then in ReleaseFilters:

const [checkedList, setCheckedList] = usePersistentState(`${RELEASE_FILTERS_STORAGE_KEY}-${pageType}-checkedList`, []);

Which might be easier to use, but does mean as many localForage calls as we have options, when we load the component. I would say test it out and see if there is an impact on performance at all?

setReleaseTagsCheckList: (list: Array<string | undefined>) => void;
setReleaseTagsExcludeCheckList: (list: Array<string | undefined>) => void;
setIncludeVariousArtists: (value: boolean) => void;
setCoverartOnly: (value: boolean) => void;
}

export default function useFilterPersistence(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name here is a bit too generic, you can imagine there are other places in the codebase where we might have filters.
Maybe useFreshReleasesFilterPersistence ?

params: UseFilterPersistenceParams
) {
const {
checkedList,
releaseTagsCheckList,
releaseTagsExcludeCheckList,
includeVariousArtists,
coverartOnly,
setCheckedList,
setReleaseTagsCheckList,
setReleaseTagsExcludeCheckList,
setIncludeVariousArtists,
setCoverartOnly,
} = params;

// Load filters on component mount
useEffect(() => {
const loadFilters = async () => {
try {
const savedFilters = await release_filters_cache.getItem<StoredFilters>(
RELEASE_FILTERS_STORAGE_KEY
);
if (savedFilters) {
setCheckedList(savedFilters.checkedList ?? []);
setReleaseTagsCheckList(savedFilters.releaseTagsCheckList ?? []);
setReleaseTagsExcludeCheckList(
savedFilters.releaseTagsExcludeCheckList ?? []
);
setIncludeVariousArtists(savedFilters.includeVariousArtists ?? false);
setCoverartOnly(savedFilters.coverartOnly ?? false);
}
} catch (error) {
console.error("Failed to load filters:", error);
}
};
loadFilters();
}, []);

// Save filters when they change
useEffect(() => {
const saveFilters = async () => {
try {
const filtersToSave: StoredFilters = {
checkedList: checkedList.filter(
(item): item is string => item !== undefined
),
Comment on lines +77 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This syntax can be made a lot shorter like so to filter out falsy values (will also filter out empty strings but that's good in this case):

Suggested change
checkedList: checkedList.filter(
(item): item is string => item !== undefined
),
checkedList: checkedList.filter(Boolean),

releaseTagsCheckList: releaseTagsCheckList.filter(
(item): item is string => item !== undefined
),
releaseTagsExcludeCheckList: releaseTagsExcludeCheckList.filter(
(item): item is string => item !== undefined
),
includeVariousArtists,
coverartOnly,
};
await release_filters_cache.setItem(
RELEASE_FILTERS_STORAGE_KEY,
filtersToSave
);
} catch (error) {
console.error("Failed to save filters:", error);
}
};
saveFilters();
}, [
checkedList,
releaseTagsCheckList,
releaseTagsExcludeCheckList,
includeVariousArtists,
coverartOnly,
]);

const clearSavedFilters = async () => {
try {
await release_filters_cache.removeItem(RELEASE_FILTERS_STORAGE_KEY);
setCheckedList([]);
setReleaseTagsCheckList([]);
setReleaseTagsExcludeCheckList([]);
setIncludeVariousArtists(false);
setCoverartOnly(false);
} catch (error) {
console.error("Failed to clear filters:", error);
}
};
return { clearSavedFilters };
}
Loading