mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-09-24 13:40:21 +01:00
Keep default filters out of the URL hash (#1314)
Follow up to #1311, per your "no use keeping empty / default values in the url". A plain author search was coming out as ``` #q=herbert&search_by=author&sort=relevance&lang=default&format=epub&format=mobi&format=azw3&format=fb2&format=djvu&format=cbz&format=cbr ``` The builder serialized every advanced filter regardless of whether the user had touched it. Now `sort` drops when it equals the sort the app would apply anyway (the provider default in Universal, the configured one in Direct), `lang` drops while it is still the `default` sentinel, and `format` drops when the selection matches `supported_formats` from the config. Formats are compared as a set, so reselecting everything in a different order still counts as default. Anything the user actually changed stays. That also makes the comment I left in the builder true: a default-state URL now carries no hash at all. The second commit is an ordering bug that omitting defaults made visible. `loadConfig`'s `initial` branch seeds `formats` from `supported_formats` and `sort` from the mode default. The URL bootstrap is gated on config being loaded, so it normally runs after that and wins on its own, but nothing guarantees only one `initial` load happens, and a second one landing after the bootstrap resets `formats` to the full supported list and drops the sort the link asked for. StrictMode double-invokes the mount effect that triggers it, so it reproduces in development: `#q=dune&format=epub&lang=en` intermittently loses its `format=epub`. The seeding is now skipped once the bootstrap has applied, so a link's filters win over the defaults they were meant to override. Five new unit tests on the builder. I also drove both the dev server and a production build in Chromium with `/api` mocked: `#search_by=author&q=herbert` settles at `#q=herbert&search_by=author`, typing a plain query gives `#q=dune`, direct mode at rest carries no hash, and `#q=dune&format=epub&lang=en` keeps both filters in the hash and in the request. That last one was 3 for 5 on the dev server before the second commit and 5 for 5 after.
This commit is contained in:
@@ -708,7 +708,11 @@ function App() {
|
||||
nonce: urlSearchNonce,
|
||||
});
|
||||
const [hasExecutedUrlSearchBootstrap, setHasExecutedUrlSearchBootstrap] = useState(false);
|
||||
// Same fact as the state above, readable from loadConfig's async continuation, which
|
||||
// closes over the render it started in and would otherwise see a stale `false`.
|
||||
const urlSearchBootstrapAppliedRef = useRef(false);
|
||||
useExternalHashChange(() => {
|
||||
urlSearchBootstrapAppliedRef.current = false;
|
||||
setHasExecutedUrlSearchBootstrap(false);
|
||||
setUrlSearchNonce((value) => value + 1);
|
||||
});
|
||||
@@ -847,7 +851,13 @@ function App() {
|
||||
: cfg.default_sort || 'relevance';
|
||||
|
||||
if (cfg?.supported_formats) {
|
||||
if (mode === 'initial') {
|
||||
// Seeding the defaults must not undo filters a shared link already applied.
|
||||
// The URL bootstrap is gated on config being loaded, so normally it runs after
|
||||
// this and wins on its own - but nothing guarantees this is the only 'initial'
|
||||
// load (React's StrictMode double-invokes the mount effect that triggers it in
|
||||
// development), and a late one would reset `formats` to the full supported list
|
||||
// and drop the link's own sort.
|
||||
if (mode === 'initial' && !urlSearchBootstrapAppliedRef.current) {
|
||||
setAdvancedFilters((prev) => ({
|
||||
...prev,
|
||||
formats: cfg.supported_formats,
|
||||
@@ -2012,6 +2022,13 @@ function App() {
|
||||
return searchFieldValues[activeQueryOption.field.key] ?? '';
|
||||
}, [activeQueryOption, searchInput, searchFieldValues]);
|
||||
|
||||
// The sort the app applies with no user choice, mirroring what loadConfig seeds
|
||||
// advancedFilters.sort with - a sort equal to it is a default, not a shared intent.
|
||||
const urlHashDefaultSort =
|
||||
effectiveSearchMode === 'universal'
|
||||
? resolvedMetadataDefaultSort
|
||||
: config?.default_sort || 'relevance';
|
||||
|
||||
// Keep the URL hash fragment live as search state changes. Gated until any URL-driven
|
||||
// bootstrap has applied (or there was nothing to apply), so we don't clobber a shared
|
||||
// link's params with the initial default state before they've been read.
|
||||
@@ -2024,8 +2041,18 @@ function App() {
|
||||
contentType,
|
||||
combinedMode,
|
||||
advancedFilters,
|
||||
defaultSort: urlHashDefaultSort,
|
||||
defaultFormats: supportedFormats,
|
||||
}),
|
||||
[activeQueryValue, effectiveActiveQueryTarget, contentType, combinedMode, advancedFilters],
|
||||
[
|
||||
activeQueryValue,
|
||||
effectiveActiveQueryTarget,
|
||||
contentType,
|
||||
combinedMode,
|
||||
advancedFilters,
|
||||
urlHashDefaultSort,
|
||||
supportedFormats,
|
||||
],
|
||||
);
|
||||
useSyncUrlSearchHash({ enabled: readyToSyncUrlHash, hash: urlSearchHash });
|
||||
|
||||
@@ -2829,6 +2856,7 @@ function App() {
|
||||
setSearchFieldValue={updateSearchFieldValue}
|
||||
runSearchWithPolicyRefresh={runSearchWithPolicyRefresh}
|
||||
onComplete={() => {
|
||||
urlSearchBootstrapAppliedRef.current = true;
|
||||
setHasExecutedUrlSearchBootstrap(true);
|
||||
}}
|
||||
/>
|
||||
|
||||
@@ -127,4 +127,90 @@ describe('buildUrlSearchHash', () => {
|
||||
});
|
||||
expect(new URLSearchParams(unchecked).has('q')).toBe(false);
|
||||
});
|
||||
|
||||
it('omits filters still sitting at their defaults', () => {
|
||||
const hash = buildUrlSearchHash({
|
||||
queryValue: 'dune',
|
||||
searchBy: 'general',
|
||||
contentType: 'ebook',
|
||||
combinedMode: false,
|
||||
defaultSort: 'relevance',
|
||||
defaultFormats: ['epub', 'mobi', 'azw3'],
|
||||
advancedFilters: {
|
||||
sort: 'relevance',
|
||||
lang: ['default'],
|
||||
formats: ['epub', 'mobi', 'azw3'],
|
||||
},
|
||||
});
|
||||
|
||||
expect(hash).toBe('q=dune');
|
||||
});
|
||||
|
||||
it('treats a reordered default selection as the default', () => {
|
||||
const hash = buildUrlSearchHash({
|
||||
queryValue: 'dune',
|
||||
searchBy: 'general',
|
||||
contentType: 'ebook',
|
||||
combinedMode: false,
|
||||
defaultFormats: ['epub', 'mobi', 'azw3'],
|
||||
advancedFilters: { formats: ['azw3', 'epub', 'mobi'] },
|
||||
});
|
||||
|
||||
expect(hash).toBe('q=dune');
|
||||
});
|
||||
|
||||
it('keeps filters the user actually changed', () => {
|
||||
const params = new URLSearchParams(
|
||||
buildUrlSearchHash({
|
||||
queryValue: 'dune',
|
||||
searchBy: 'general',
|
||||
contentType: 'ebook',
|
||||
combinedMode: false,
|
||||
defaultSort: 'relevance',
|
||||
defaultFormats: ['epub', 'mobi', 'azw3'],
|
||||
advancedFilters: {
|
||||
sort: 'newest',
|
||||
lang: ['en', 'de'],
|
||||
formats: ['epub'],
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
expect(params.get('sort')).toBe('newest');
|
||||
expect(params.getAll('lang')).toEqual(['en', 'de']);
|
||||
expect(params.getAll('format')).toEqual(['epub']);
|
||||
});
|
||||
|
||||
it('keeps a narrowed selection that happens to be the same length', () => {
|
||||
const params = new URLSearchParams(
|
||||
buildUrlSearchHash({
|
||||
queryValue: 'dune',
|
||||
searchBy: 'general',
|
||||
contentType: 'ebook',
|
||||
combinedMode: false,
|
||||
defaultFormats: ['epub', 'mobi', 'azw3'],
|
||||
advancedFilters: { formats: ['epub', 'mobi', 'pdf'] },
|
||||
}),
|
||||
);
|
||||
|
||||
expect(params.getAll('format')).toEqual(['epub', 'mobi', 'pdf']);
|
||||
});
|
||||
|
||||
it('leaves a plain default-state search with no hash at all', () => {
|
||||
const hash = buildUrlSearchHash({
|
||||
queryValue: '',
|
||||
searchBy: 'general',
|
||||
contentType: 'ebook',
|
||||
combinedMode: false,
|
||||
defaultSort: 'relevance',
|
||||
defaultFormats: ['epub', 'mobi', 'azw3'],
|
||||
advancedFilters: {
|
||||
sort: 'relevance',
|
||||
lang: ['default'],
|
||||
formats: ['epub', 'mobi', 'azw3'],
|
||||
},
|
||||
});
|
||||
|
||||
expect(hash).toBe('');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { AdvancedFilterState, ContentType } from '../types';
|
||||
import { LANGUAGE_OPTION_DEFAULT } from './languageFilters';
|
||||
|
||||
export interface UrlSearchHashState {
|
||||
/**
|
||||
@@ -11,8 +12,30 @@ export interface UrlSearchHashState {
|
||||
contentType: ContentType;
|
||||
combinedMode: boolean;
|
||||
advancedFilters: Partial<AdvancedFilterState>;
|
||||
/**
|
||||
* Sort the app would apply on its own (the provider's default in Universal mode,
|
||||
* the configured one in Direct). A sort matching it is left out of the hash.
|
||||
*/
|
||||
defaultSort?: string;
|
||||
/**
|
||||
* Format selection the app starts from - `supported_formats` from the server config,
|
||||
* not a fixed list, so an instance with a custom format list still gets a clean URL.
|
||||
*/
|
||||
defaultFormats?: string[];
|
||||
}
|
||||
|
||||
/** Selections are order-independent, so compare them as sets. */
|
||||
const isDefaultSelection = (values: string[] | undefined, defaults: string[]): boolean => {
|
||||
if (!values) {
|
||||
return true;
|
||||
}
|
||||
if (values.length !== defaults.length) {
|
||||
return false;
|
||||
}
|
||||
const defaultSet = new Set(defaults);
|
||||
return values.every((value) => defaultSet.has(value));
|
||||
};
|
||||
|
||||
const serializeQueryValue = (value: string | number | boolean): string => {
|
||||
if (typeof value === 'boolean') {
|
||||
return value ? '1' : '';
|
||||
@@ -26,6 +49,10 @@ const serializeQueryValue = (value: string | number | boolean): string => {
|
||||
*
|
||||
* Kept as a hash fragment (not query params) so it stays browser-side only,
|
||||
* rather than looking like a server-processed query string.
|
||||
*
|
||||
* Filters still sitting at their defaults are left out: they say nothing about what
|
||||
* the user is looking at, and carrying every format and `lang=default` turns a plain
|
||||
* search into a link several times longer than the query it shares.
|
||||
*/
|
||||
export const buildUrlSearchHash = (state: UrlSearchHashState): string => {
|
||||
const params = new URLSearchParams();
|
||||
@@ -49,13 +76,20 @@ export const buildUrlSearchHash = (state: UrlSearchHashState): string => {
|
||||
if (isbn) params.set('isbn', isbn);
|
||||
if (author) params.set('author', author);
|
||||
if (title) params.set('title', title);
|
||||
if (sort) params.set('sort', sort);
|
||||
if (sort && sort !== state.defaultSort) params.set('sort', sort);
|
||||
if (content) params.set('content', content);
|
||||
for (const value of lang ?? []) {
|
||||
if (value) params.append('lang', value);
|
||||
|
||||
// `[LANGUAGE_OPTION_DEFAULT]` is the untouched language selection, and it already means
|
||||
// "whatever the server default is" - spelling it out in the URL adds nothing.
|
||||
if (!isDefaultSelection(lang, [LANGUAGE_OPTION_DEFAULT])) {
|
||||
for (const value of lang ?? []) {
|
||||
if (value) params.append('lang', value);
|
||||
}
|
||||
}
|
||||
for (const value of formats ?? []) {
|
||||
if (value) params.append('format', value);
|
||||
if (!isDefaultSelection(formats, state.defaultFormats ?? [])) {
|
||||
for (const value of formats ?? []) {
|
||||
if (value) params.append('format', value);
|
||||
}
|
||||
}
|
||||
|
||||
return params.toString();
|
||||
|
||||
Reference in New Issue
Block a user