fix(search): reach the server's deadline, query one author (#1285, #1252) (#1287)

Two independent reasons a working search reported failure to the user.

1. The client gave up before the server did (#1285)

`/api/releases` bounds one release search with RELEASE_SEARCH_TIMEOUT
(default
300s) and answers a spent budget with a sentence naming the real cause -
the
machinery added for #1276. The frontend then aborted the direct_download
search
at a hard-coded 180s, so it always won the race: the user saw "Request
timed
out. Check your network connection or proxy configuration." instead, and
raising RELEASE_SEARCH_TIMEOUT changed nothing they could observe, the
180s
being baked into the hashed bundle inside the image.

- /api/config reports the effective (clamped) budget, and the client
derives its
  abort from it plus a margin, so the server always answers first.
- Direct-mode search shows what the server actually said. Every non-auth
failure
was relabelled "Unable to reach download source. Network may be
restricted or
mirrors blocked.", which discarded the explanation and blamed the user's
network. ApiResponseError now carries `serverMessage`, set only when the
server
  explained itself, so the status-line placeholder still falls back.

Two latency fixes for the cost that made the timeout reachable at all:

- Fetch each distinct AA search URL once per search. The language-filter
retry
re-runs every title variant, and with DIRECT_DOWNLOAD_LANGUAGE_FROM_PATH
on
both passes build a byte-identical URL - behind DDoS-Guard each repeat
is a
  fresh browser solve.
- Drop the solve-only bypass method. `_bypass_method_cdp_gui_click`
opens with
exactly that call and returns the moment it works, so the entry ahead of
it
could only repeat the half that had already failed, plus the backoff
before
the method that does work started. Reported at 0/19 successes and ~5.5s
of
  each ~26s solve against DDoS-Guard.

2. The query carried every contributor, not one author (#1252)

`_pick_search_author` returned `book.search_author` verbatim while the
authors[]
fallback beside it deliberately narrowed to the first name before a
comma. Both
fields routinely arrive holding every contributor joined with ", ": the
frontend
builds `book.author` as `authors.join(', ')` for display
(bookTransformers.ts)
and the release modal sends that display string straight back as the
`author`
parameter, and `browse_record_to_book_metadata` and the manual-search
branch
both split the joined text into `authors` while still passing the
unsplit string
as `search_author`, so the split was never used.

A book whose metadata lists translators was therefore searched for as

    Blindness Jose Saramago, Giovanni Pontiero, <persian translator>

which matches nothing on Anna's Archive. The bypass succeeds, the search
comes
back empty, and the user is told the book has no releases.

Narrowed in one place, `search_plan.first_author`, so the two branches
cannot
drift apart again, and applied to the IRC source, which built its query
with the
same verbatim preference. Hardcover is unaffected: it already sets
`search_author` from `_simplify_author_for_search(authors[0])`, which
resolves
"Last, First" itself and never yields a multi-author string.
This commit is contained in:
CaliBrain
2026-09-01 12:38:57 -04:00
committed by GitHub
parent 633004ecf0
commit 3d7ea40088
38 changed files with 1529 additions and 403 deletions
+6 -13
View File
@@ -525,18 +525,6 @@ async def _bypass_method_humanlike(page: Any) -> bool:
return False
async def _bypass_method_cdp_solve(page: Any) -> bool:
"""CDP Mode with solve_captcha() - auto-detects challenge type."""
try:
logger.debug("Attempting bypass: CDP solve_captcha")
await page.solve_captcha()
await asyncio.sleep(_RNG.uniform(3, 5))
return await _is_bypassed(page)
except _CDP_OPERATION_ERRORS as e:
logger.debug("CDP solve_captcha failed: %s", e)
return False
CDP_CLICK_SELECTORS = [
"#turnstile-widget div", # Cloudflare Turnstile
"#cf-turnstile div", # Alternative CF Turnstile
@@ -616,8 +604,13 @@ async def _bypass_method_cdp_gui_click(page: Any) -> bool:
return False
# Ordered cheapest-first, and deliberately without a bare `solve_captcha()` entry:
# _bypass_method_cdp_gui_click opens by doing exactly that and returns the moment it
# works, so a separate method ahead of it could only ever repeat the half that had
# already failed - one wasted round trip plus the backoff before the next attempt, on
# every solve that gets this far. Measured at ~5.5s of the ~26s each solve cost, and
# 0/19 successes for the standalone method against DDoS-Guard. See issue #1285.
BYPASS_METHODS = [
_bypass_method_cdp_solve,
_bypass_method_cdp_gui_click,
_bypass_method_cdp_click,
_bypass_method_humanlike,
+46 -11
View File
@@ -112,18 +112,53 @@ def _normalize_languages(languages: list[str] | None, user_id: int | None) -> li
return _to_language_codes(languages, source="the search request")
def _pick_search_author(book: BookMetadata) -> str:
author = book.search_author or (book.authors[0] if book.authors else "")
if not author:
return ""
def first_author(value: str) -> str:
"""The first name in a possibly comma-joined author string.
# `search_author` can arrive as the display string for the whole credit list
# ("Author, Translator, Narrator"), which Anna's Archive answers with nothing at
# all. Trim it to the first name, which is what the authors list already gets.
if "," in author:
author = author.split(",")[0].strip()
Both ends of the app hand us every contributor in one string. The frontend joins
`authors` with ", " for display (`bookTransformers.ts`) and that display string comes
straight back as the `author` request parameter, while several providers set
`search_author` from the same joined text. Searching a release source for
"Blindness Jose Saramago, Giovanni Pontiero, ..." - the author plus two translators -
matches nothing, and the user is told the book has no releases at all.
return author
A "Last, First" author collapses to the surname, which is still a usable search term
and is what the authors[] fallback has always done with the same input. See #1252.
"""
first, _, _ = value.partition(",")
return first.strip()
def pick_search_author(book: BookMetadata) -> str:
"""The one author a release query should carry, from whichever field holds one.
Every release source that builds its own query wants exactly this, so it lives here
rather than being re-derived per source - the two branches below drifted apart once
already (#1252) and the IRC source carried a third copy of the same preference.
#1290 fixed the same report by merging the two branches and trimming whichever one
won; this keeps that outcome ("Blindness Jose Saramago" from either field, measured
there at 0 releases before and 49 after) and adds the empty-narrowing fallback, so a
credit list that merely starts with a blank entry does not fall out to title-only.
"""
# Narrowing can come back empty - the joined string starts with a comma because the
# first contributor was blank, and `authors.join(', ')` does not drop the empty entry.
# Falling through to authors[] then still finds a usable name; returning "" would
# search by title alone and lose the author we were holding all along.
if book.search_author:
narrowed = first_author(book.search_author)
if narrowed:
return narrowed
# A bare string here would otherwise be iterated one character at a time; the IRC
# source guarded against exactly that before it shared this helper.
authors = book.authors if isinstance(book.authors, list) else [book.authors or ""]
for author in authors:
narrowed = first_author(author or "")
if narrowed:
return narrowed
return ""
def _pick_search_title(book: BookMetadata) -> str:
@@ -150,7 +185,7 @@ def build_release_search_plan(
if manual_query:
resolved_manual_query = manual_query.strip()[:MANUAL_QUERY_MAX_LEN] or None
author = _pick_search_author(book)
author = pick_search_author(book)
base_title = _pick_search_title(book)
if resolved_manual_query:
+11 -1
View File
@@ -1180,6 +1180,12 @@ def api_config() -> Response | tuple[Response, int]:
[],
user_id=db_user_id,
),
# The client must not give up before this budget does. `/api/releases`
# answers a spent budget with a message naming the real cause (a protection
# challenge nobody could solve); a browser that aborted first replaces it
# with a generic network/proxy error and RELEASE_SEARCH_TIMEOUT becomes a
# setting the user can raise with no visible effect. See issue #1285.
"release_search_timeout": search_deadline.budget_seconds(),
"settings_enabled": _is_config_dir_writable(),
"onboarding_complete": _get_onboarding_complete(),
# Default sort orders
@@ -2947,6 +2953,10 @@ def api_releases() -> Response | tuple[Response, int]:
elif provider == "manual":
resolved_title = title_param or manual_query or "Manual Search"
resolved_author = author_param or ""
# The release modal sends `authors.join(', ')` as `author`, so the commas here
# are joins between contributors, not part of one name. This split is the only
# place that knows that, so `search_author` comes from it rather than from the
# joined text - see issue #1252.
authors = [a.strip() for a in resolved_author.split(",") if a.strip()]
book = BookMetadata(
@@ -2955,7 +2965,7 @@ def api_releases() -> Response | tuple[Response, int]:
provider_display_name="Manual Search",
title=resolved_title,
search_title=resolved_title,
search_author=resolved_author or None,
search_author=authors[0] if authors else None,
authors=authors,
)
else:
+5 -1
View File
@@ -519,6 +519,10 @@ def browse_record_to_book_metadata(
"""Convert a source-native browse record into generic book metadata."""
resolved_title = title_override or str(record.title or "").strip() or "Unknown title"
resolved_author = author_override or str(record.author or "").strip()
# `author_override` is the frontend's display string, `authors.join(', ')` - every
# contributor, translators included. The split below is the only place that knows the
# commas were joins rather than part of a name, so `search_author` is taken from it
# rather than from the joined text. See issue #1252.
authors = [part.strip() for part in resolved_author.split(",") if part.strip()]
publish_year = None
@@ -535,7 +539,7 @@ def browse_record_to_book_metadata(
provider_display_name=get_source_display_name(record.source),
title=resolved_title,
search_title=resolved_title,
search_author=resolved_author or None,
search_author=authors[0] if authors else None,
authors=authors,
cover_url=record.preview,
description=record.description,
+75 -1
View File
@@ -6,6 +6,8 @@ import re
import threading
import time
import unicodedata
from contextlib import contextmanager
from contextvars import ContextVar
from dataclasses import replace
from http import HTTPStatus
from pathlib import Path
@@ -44,7 +46,7 @@ from shelfmark.release_sources import (
)
if TYPE_CHECKING:
from collections.abc import Callable, Iterable
from collections.abc import Callable, Iterable, Iterator
from pathlib import Path
from threading import Event
@@ -579,6 +581,46 @@ def _looks_like_challenge_page(html: str) -> bool:
return challenge_marker(html) is not None
# Pages already fetched during the search in flight, keyed by URL. Scoped to one
# DirectDownload.search() so nothing is carried between requests.
_search_page_cache: ContextVar[dict[str, tuple[str, Tag | None]] | None] = ContextVar(
"aa_search_page_cache", default=None
)
@contextmanager
def _search_page_reuse() -> Iterator[None]:
"""Fetch each distinct AA search URL at most once per search.
One search asks AA for the same URL more than once. The language-filter retry in
`search()` re-runs every title variant, and when DIRECT_DOWNLOAD_LANGUAGE_FROM_PATH
is on the requested language is applied locally instead of as `&lang=`, so both
passes build a byte-identical URL - the retry differs only in the filtering it does
to the response it already had. A repeat is not a cheap round trip either: AA is
behind DDoS-Guard, so each one is a fresh browser solve, tens of seconds that buy
nothing. See issue #1285.
"""
token = _search_page_cache.set({})
try:
yield
finally:
_search_page_cache.reset(token)
def _is_reusable_answer(result: tuple[str, Tag | None]) -> bool:
"""Whether a fetched page is an answer, rather than a giving-up worth retrying.
`_fetch_search_table_uncached` exists to rotate past mirrors that are not actually AA,
and when it runs out of them it *returns* instead of raising: a page with no results
table and no marker. Storing that would hand the language-filter retry - the pass this
cache exists for - a mirror set that may have recovered in between (DNS rotation, a
mirror coming back), turning a transient outage into "this book has no releases". A
real "No files found." is an answer and is worth keeping.
"""
html, tbody = result
return tbody is not None or "No files found." in html or _looks_like_aa_page(html)
# How much of an unreadable search page to quote in the debug log. Enough to carry the
# <head> - title, injected challenge scripts - without pasting a 180 KB page into a log
# file that ships inside the debug bundle.
@@ -624,6 +666,22 @@ def _log_untabled_search_page(url: str, html: str) -> None:
def _fetch_search_table(url: str, selector: network.AAMirrorSelector) -> tuple[str, Tag | None]:
"""Fetch the AA search page, reusing one already fetched during this search."""
cache = _search_page_cache.get()
if cache is not None and url in cache:
logger.debug("Reusing search page already fetched for this search: %s", url)
return cache[url]
result = _fetch_search_table_uncached(url, selector)
if cache is not None and _is_reusable_answer(result):
cache[url] = result
return result
def _fetch_search_table_uncached(
url: str, selector: network.AAMirrorSelector
) -> tuple[str, Tag | None]:
"""Fetch the AA search page, retrying past mirrors that are not actually AA.
A parked or seized domain answers 200 with a page that has no results table and no
@@ -1970,6 +2028,22 @@ class DirectDownloadSource(ReleaseSource):
) -> list[Release]:
"""Search for releases using the book's metadata.
The whole fan-out runs under one page cache, so a URL built twice by different
passes is fetched once. See `_search_page_reuse`.
"""
with _search_page_reuse():
return self._search(book, plan, expand_search=expand_search, content_type=content_type)
def _search(
self,
book: BookMetadata,
plan: ReleaseSearchPlan,
*,
expand_search: bool = False,
content_type: str = "ebook",
) -> list[Release]:
"""Search for releases using the book's metadata.
Priority: ISBN search first (most precise), then title+author fallback.
For non-English languages, uses localized titles from book.titles_by_language.
+8 -5
View File
@@ -15,6 +15,7 @@ if TYPE_CHECKING:
from shelfmark.api.websocket import ws_manager
from shelfmark.core.config import config
from shelfmark.core.logger import setup_logger
from shelfmark.core.search_plan import pick_search_author
from shelfmark.core.utils import is_audiobook
from shelfmark.release_sources import (
ColumnColorHint,
@@ -394,11 +395,13 @@ class IRCReleaseSource(ReleaseSource):
if book.search_title or book.title:
parts.append(book.search_title or book.title)
if book.search_author:
parts.append(book.search_author)
elif book.authors:
# Use first author
author = book.authors[0] if isinstance(book.authors, list) else book.authors
# Only ever the first author: both metadata fields can arrive holding every
# contributor joined with ", ", and an IRC query carrying an author plus two
# translators matches nothing. The choice between them - and the narrowing - is
# `pick_search_author`, shared with the search plan so this cannot drift from it
# again. See issue #1252.
author = pick_search_author(book)
if author:
parts.append(author)
return " ".join(parts)
+9
View File
@@ -35,6 +35,15 @@
"typescript/no-misused-promises": "error",
"typescript/no-non-null-assertion": "error",
"typescript/only-throw-error": "error",
// React Compiler advisories, enforced everywhere with no per-file exemptions.
// The violations inherited from the oxlint 1.70 -> 1.80 bump are all resolved:
// three by widening a dependency to the object the compiler infers, and seven
// by an `oxlint-disable-next-line` that says, at the callsite, why the flagged
// dependency is load-bearing - five are re-run triggers that are never read,
// and two are values the callback genuinely uses.
"react/preserve-manual-memoization": "error",
"react/exhaustive-effect-dependencies": "error",
"react/memo-dependencies": "error",
"react/no-danger": "error",
"react/no-clone-element": "error",
"react/no-react-children": "error",
+85 -80
View File
@@ -32,6 +32,7 @@ import {
import { useActivity } from './hooks/useActivity';
import { useAuth } from './hooks/useAuth';
import { useDownloadTracking } from './hooks/useDownloadTracking';
import { useLatestCallback } from './hooks/useLatestCallback';
import { useMediaQuery } from './hooks/useMediaQuery';
import { useMountEffect } from './hooks/useMountEffect';
import { useRealtimeStatus } from './hooks/useRealtimeStatus';
@@ -73,6 +74,7 @@ import type {
ActingAsUserSelection,
MetadataProviderSummary,
MetadataSearchConfig,
MetadataSearchField,
QueuedDownloadResult,
QueryTargetOption,
SearchMode,
@@ -492,8 +494,6 @@ function App() {
});
// When a book is removed from the Hardcover list currently being browsed, remove it from results
const searchFieldValuesRef = useRef(searchFieldValues);
searchFieldValuesRef.current = searchFieldValues;
useBookTargetDeselectSync({
activeListValue: searchFieldValues.hardcover_list,
setBooks,
@@ -605,24 +605,6 @@ function App() {
};
}, [effectiveActingAsUser, pendingOnBehalfDownload]);
// Wire up logout callback to clear search state
const handleLogoutWithCleanup = useCallback(async () => {
await handleLogout();
resetSearchResultsState();
setActiveQueryTarget('general');
setPendingRequestPayload(null);
setPendingRequestExtraPayloads([]);
setActingAsUser(null);
setAdminUsers([]);
setAdminUsersError(null);
setHasLoadedAdminUsers(false);
setPendingOnBehalfDownload(null);
setFulfillingRequest(null);
resetActivity();
setSettingsOpen(false);
setSelfSettingsOpen(false);
}, [handleLogout, resetActivity, resetSearchResultsState]);
// Combined mode state (ebook + audiobook in one transaction)
const [combinedState, setCombinedState] = useState<CombinedSelectionState | null>(null);
@@ -655,20 +637,6 @@ function App() {
setDownloadsSidebarOpen(true);
prefetchActivityHistory();
}, [downloadsSidebarOpen, prefetchActivityHistory]);
const handleSettingsClick = useCallback(() => {
if (config?.settings_enabled) {
if (authIsAdmin) {
void primeUsersCache();
void primeSettingsCache();
setSettingsOpen(true);
} else {
setSelfSettingsOpen(true);
}
return;
}
setConfigBannerOpen(true);
}, [authIsAdmin, config?.settings_enabled]);
const headerRef = useCallback((el: HTMLDivElement | null) => {
if (headerObserverRef.current) {
headerObserverRef.current.disconnect();
@@ -685,6 +653,39 @@ function App() {
const [settingsOpen, setSettingsOpen] = useState(false);
const [selfSettingsOpen, setSelfSettingsOpen] = useState(false);
const [configBannerOpen, setConfigBannerOpen] = useState(false);
// Wire up logout callback to clear search state
const handleLogoutWithCleanup = useCallback(async () => {
await handleLogout();
resetSearchResultsState();
setActiveQueryTarget('general');
setPendingRequestPayload(null);
setPendingRequestExtraPayloads([]);
setActingAsUser(null);
setAdminUsers([]);
setAdminUsersError(null);
setHasLoadedAdminUsers(false);
setPendingOnBehalfDownload(null);
setFulfillingRequest(null);
resetActivity();
setSettingsOpen(false);
setSelfSettingsOpen(false);
}, [handleLogout, resetActivity, resetSearchResultsState]);
const handleSettingsClick = useCallback(() => {
if (config?.settings_enabled) {
if (authIsAdmin) {
void primeUsersCache();
void primeSettingsCache();
setSettingsOpen(true);
} else {
setSelfSettingsOpen(true);
}
return;
}
setConfigBannerOpen(true);
}, [authIsAdmin, config?.settings_enabled]);
const [onboardingOpen, setOnboardingOpen] = useState(false);
useShowOnboardingDebug({
setOnboardingOpen,
@@ -1075,48 +1076,43 @@ function App() {
// When downloading a book while browsing a Hardcover list the user owns,
// automatically remove it from that list (fire-and-forget).
const searchFieldLabelsRef = useRef(searchFieldLabels);
searchFieldLabelsRef.current = searchFieldLabels;
const metadataConfigRef = useRef(activeMetadataConfig);
metadataConfigRef.current = activeMetadataConfig;
// Stable identity for the download handlers below, while still reading the current
// search field values, labels and metadata config. Not an Effect Event: the callers
// are download handlers, not Effects. See useLatestCallback.
const removeBookFromActiveList = useLatestCallback((book: Book) => {
if (config?.hardcover_auto_remove_on_download === false) return;
if (!bookSupportsTargets(book)) return;
const activeList = searchFieldValues.hardcover_list;
if (!activeList) return;
const target = String(activeList);
const provider = book.provider;
const bookId = book.provider_id;
if (!provider || !bookId) return;
const removeBookFromActiveList = useCallback(
(book: Book) => {
if (config?.hardcover_auto_remove_on_download === false) return;
if (!bookSupportsTargets(book)) return;
const activeList = searchFieldValuesRef.current.hardcover_list;
if (!activeList) return;
const target = String(activeList);
const provider = book.provider;
const bookId = book.provider_id;
if (!provider || !bookId) return;
// Only auto-remove from lists the user owns (Reading Status / My Lists)
const listField = activeMetadataConfig?.search_fields.find(
(f) => f.key === 'hardcover_list' && f.type === 'DynamicSelectSearchField',
);
if (listField && listField.type === 'DynamicSelectSearchField') {
const group = getDynamicOptionGroup(listField.options_endpoint, target);
if (group && group !== 'Reading Status' && group !== 'My Lists') return;
}
// Only auto-remove from lists the user owns (Reading Status / My Lists)
const listField = metadataConfigRef.current?.search_fields.find(
(f) => f.key === 'hardcover_list' && f.type === 'DynamicSelectSearchField',
);
if (listField && listField.type === 'DynamicSelectSearchField') {
const group = getDynamicOptionGroup(listField.options_endpoint, target);
if (group && group !== 'Reading Status' && group !== 'My Lists') return;
}
void setBookTargetState(provider, bookId, target, false)
.then((result) => {
if (result.changed) {
emitBookTargetChange({
provider,
bookId,
target,
selected: false,
});
const listName = searchFieldLabelsRef.current['hardcover_list'];
showToast(`Removed from ${listName || 'list'}`, 'info');
}
})
.catch(() => undefined);
},
[config?.hardcover_auto_remove_on_download, showToast],
);
void setBookTargetState(provider, bookId, target, false)
.then((result) => {
if (result.changed) {
emitBookTargetChange({
provider,
bookId,
target,
selected: false,
});
const listName = searchFieldLabels['hardcover_list'];
showToast(`Removed from ${listName || 'list'}`, 'info');
}
})
.catch(() => undefined);
});
const executeBookDownload = useCallback(
async (book: Book, onBehalfOfUserId?: number): Promise<void> => {
@@ -1911,11 +1907,16 @@ function App() {
// Keep the last known search fields so queryTargets doesn't collapse to
// [general] while the metadata config briefly reloads on content type switch.
const lastKnownSearchFields = useRef(activeMetadataConfig?.search_fields ?? []);
if (activeMetadataConfig?.search_fields) {
lastKnownSearchFields.current = activeMetadataConfig.search_fields;
// Held in state rather than a ref written during render: a ref read back in the same
// pass is what `react/refs` forbids, and this is the adjust-state-during-render shape
// React documents for exactly this - carry the previous value until a new one arrives.
const [stableSearchFields, setStableSearchFields] = useState<MetadataSearchField[]>(
() => activeMetadataConfig?.search_fields ?? [],
);
const incomingSearchFields = activeMetadataConfig?.search_fields;
if (incomingSearchFields && incomingSearchFields !== stableSearchFields) {
setStableSearchFields(incomingSearchFields);
}
const stableSearchFields = lastKnownSearchFields.current;
const queryTargets = useMemo<QueryTargetOption[]>(
() =>
@@ -1953,7 +1954,9 @@ function App() {
? (queryTargets.find((target) => target.field?.key === seriesBrowseCapability.field_key) ??
null)
: null,
[queryTargets, seriesBrowseCapability?.field_key],
// `seriesBrowseCapability` whole: the body reads `.field_key` off it unguarded
// inside the ternary, so that object is the dependency the compiler infers.
[queryTargets, seriesBrowseCapability],
);
const activeQueryValue = useMemo(() => {
@@ -2245,7 +2248,9 @@ function App() {
return book.provider === activeMetadataConfig.provider;
},
[activeMetadataConfig?.provider, seriesBrowseCapability?.sort, seriesBrowseTarget?.field],
// `activeMetadataConfig` whole: the body reads `.provider` off it unguarded on
// the last line, so that object is the dependency the compiler infers.
[activeMetadataConfig, seriesBrowseCapability?.sort, seriesBrowseTarget?.field],
);
const handleManualSearch = useCallback(() => {
+20 -8
View File
@@ -34,6 +34,7 @@ import {
import { getNestedValue, toComparableText, toStringValue } from '../utils/objectHelpers';
import { toBookPlanPayload } from '../utils/packReview';
import { getReleaseFormats } from '../utils/releaseFormats';
import { INITIAL_ENTER_ANIMATION, nextEnterAnimation } from '../utils/releaseModalEnterAnimation';
import { buildReleaseDownloadPayload, type ReleaseDownloadOptions } from '../utils/releasePayload';
import {
getBookTitleCandidates,
@@ -889,6 +890,11 @@ const ReleaseModalSession = ({
} finally {
setIsRequestingBook(false);
}
// Kept against the advisory: the body really does read both. `handleClose` is aliased
// from the `onClose` prop, which is why the compiler names the source instead, and
// dropping `contentType` would let this close over a stale one and request the wrong
// format. Correctness first; the cost is an extra callback identity.
// oxlint-disable-next-line react/memo-dependencies
}, [book, onRequestBook, isRequestingBook, contentType, handleClose]);
const tabRefs = useRef<Record<string, HTMLButtonElement | null>>({});
@@ -1135,7 +1141,9 @@ const ReleaseModalSession = ({
const narratorField = book.display_fields.find((f) => f.icon === 'microphone');
return { starField, ratingsField, usersField, pagesField, lengthField, narratorField };
}, [book?.display_fields]);
// `book`, not `book?.display_fields`: the body reads `book.display_fields`
// unguarded after the early return, which is the dependency the compiler infers.
}, [book]);
const getReleaseActionMode = useCallback(
(release: Release): RequestPolicyMode => {
@@ -1286,6 +1294,9 @@ const ReleaseModalSession = ({
setPackSubmitting(false);
}
},
// Same as handleRequestBook above: the body reads `onDownload`, `contentType` and
// `handleClose`, so they stay in the list whatever the advisory infers.
// oxlint-disable-next-line react/memo-dependencies
[book, packReview, onDownload, contentType, handleClose],
);
@@ -2450,7 +2461,7 @@ const ReleaseModalSession = ({
export const ReleaseModal = ({ book, onClose, ...rest }: ReleaseModalProps) => {
const [isClosing, setIsClosing] = useState(false);
const previousSessionKeyRef = useRef<string | null>(null);
const [enterAnimation, setEnterAnimation] = useState(INITIAL_ENTER_ANIMATION);
const handleClose = useCallback(() => {
setIsClosing(true);
@@ -2474,12 +2485,13 @@ export const ReleaseModal = ({ book, onClose, ...rest }: ReleaseModalProps) => {
].join('|')
: null;
const animateEnter =
!rest.combinedMode ||
previousSessionKeyRef.current === null ||
previousSessionKeyRef.current === sessionKey;
previousSessionKeyRef.current = sessionKey;
// Decided once per session key and held for that session's lifetime, so a
// re-render mid-session cannot restart the enter animation.
const nextAnimation = nextEnterAnimation(enterAnimation, sessionKey, rest.combinedMode != null);
if (nextAnimation !== enterAnimation) {
setEnterAnimation(nextAnimation);
}
const animateEnter = nextAnimation.animate;
if (!book && !isClosing) return null;
if (!book || !sessionKey) return null;
+6 -4
View File
@@ -4,6 +4,7 @@ import { forwardRef, useImperativeHandle, useMemo, useRef, useState } from 'reac
import { useSearchMode } from '../contexts/SearchModeContext';
import { useSearchBarAutocomplete } from '../hooks/searchBar/useSearchBarAutocomplete';
import { useDismiss } from '../hooks/useDismiss';
import { useLatestCallback } from '../hooks/useLatestCallback';
import type { DynamicFieldOption } from '../services/api';
import type { ContentType, MetadataSearchField, QueryTargetOption, SortOption } from '../types';
import { SearchBarAutocompleteSession } from './SearchBarAutocompleteSession';
@@ -196,8 +197,9 @@ export const SearchBar = forwardRef<SearchBarHandle, SearchBarProps>(
const { searchMode } = useSearchMode();
const inputRef = useRef<HTMLInputElement>(null);
const buttonRef = useRef<HTMLButtonElement>(null);
const onSubmitRef = useRef(onSubmit);
onSubmitRef.current = onSubmit;
// Deferred submits below run from a timeout, not an Effect, so this is a latest-value
// callback rather than an Effect Event. See useLatestCallback.
const submitLatest = useLatestCallback(() => onSubmit());
const selectorRef = useRef<HTMLDivElement>(null);
const hasSearchQuery = hasActiveValue(value);
const [isSelectorOpen, setIsSelectorOpen] = useState(false);
@@ -730,7 +732,7 @@ export const SearchBar = forwardRef<SearchBarHandle, SearchBarProps>(
onClick={() => {
onChange(option.value, option.label);
setIsSelectOpen(false);
setTimeout(() => onSubmitRef.current(), 0);
setTimeout(() => submitLatest(), 0);
}}
className={`flex w-full items-center gap-3 px-5 py-2.5 text-left text-sm transition-colors ${
isSelected ? '' : 'hover-surface'
@@ -800,7 +802,7 @@ export const SearchBar = forwardRef<SearchBarHandle, SearchBarProps>(
setAutocompleteSelection(option.value, option.label);
onChange(option.value, option.label);
setIsAutocompleteOpen(false);
setTimeout(() => onSubmitRef.current(), 0);
setTimeout(() => submitLatest(), 0);
}}
className="hover-surface w-full px-5 py-3 text-left text-sm transition-colors"
style={{ color: 'var(--text)' }}
@@ -562,6 +562,10 @@ export const ActivityCard = ({
}
return () => observer.disconnect();
// None of these are read here - they are all re-measure triggers. The title's overflow
// depends on its text and on the width it is laid out in, and opening either panel
// reflows the card. Drop them and the tooltip-on-truncation goes stale.
// oxlint-disable-next-line react/exhaustive-effect-dependencies
}, [item.title, item.author, isRequestDetailsOpen, isRequestRejectOpen]);
const reviewRecord = item.requestRecord;
@@ -347,6 +347,10 @@ function SettingsContentPanel({
if (scrollRef.current) {
scrollRef.current.scrollTop = 0;
}
// `tab.name` is never read here - it is the trigger, and the whole point: the scroll
// position resets *because* the tab changed. Removing it strands the new tab at the
// previous one's offset.
// oxlint-disable-next-line react/exhaustive-effect-dependencies
}, [embedded, tab.name]);
const updateCustomFieldUiState = useCallback((fieldKey: string, key: string, value: unknown) => {
@@ -407,6 +411,9 @@ function SettingsContentPanel({
if (scrollRef.current) {
scrollRef.current.scrollTop = 0;
}
// `activeTakeOverFieldKey` is never read here - it is the trigger. Entering or leaving
// a subpage takeover is exactly when the scroll must reset.
// oxlint-disable-next-line react/exhaustive-effect-dependencies
}, [embedded, activeTakeOverFieldKey]);
const visibleFields = useMemo(() => {
@@ -1,5 +1,6 @@
import { useCallback, useLayoutEffect, useRef } from 'react';
import { useLatestCallback } from '../../../hooks/useLatestCallback';
import { useMountEffect } from '../../../hooks/useMountEffect';
import type { AdminUser } from '../../../services/api';
import { testAdminUserNotificationPreferences } from '../../../services/api';
@@ -169,12 +170,12 @@ export const UsersManagementField = ({
}
}, [backToList, onRefreshOverrideSummary, onSettingsSaved, onUiStateChange, saveEditedUser]);
const handleSaveUserOverridesRef = useRef(handleSaveUserOverrides);
handleSaveUserOverridesRef.current = handleSaveUserOverrides;
const triggerSaveUserOverrides = useCallback(async () => {
await handleSaveUserOverridesRef.current();
}, []);
// Stored in parent UI state, so it must keep a stable identity while still invoking the
// latest handler. Not an Effect Event: those must not be handed to another component.
// See useLatestCallback.
const triggerSaveUserOverrides = useLatestCallback(async () => {
await handleSaveUserOverrides();
});
const handleOpenOverrides = () => {
if (editingUser) {
@@ -44,162 +44,172 @@ const getOptionsIdentity = (options: MultiSelectFieldConfig['options']): string
const getSelectionIdentity = (values: string[]): string =>
values.toSorted((left, right) => left.localeCompare(right)).join('\u0001');
export const MultiSelectField = ({
interface MultiSelectVariantProps {
field: MultiSelectFieldConfig;
selected: string[];
onChange: (value: string[]) => void;
isDisabled: boolean;
}
// Dropdown variant - use DropdownList with checkboxes
const MultiSelectDropdownField = ({
field,
value: fieldValue,
selected,
onChange,
disabled,
}: MultiSelectFieldProps) => {
const selected = fieldValue ?? EMPTY_SELECTION;
// disabled prop is already computed by SettingsContent.getDisabledState()
const isDisabled = disabled ?? false;
isDisabled,
}: MultiSelectVariantProps) => {
const optionValues = field.options.map((opt) => opt.value);
const optionSet = new Set(optionValues);
const hasAllOption = optionSet.has(ALL_OPTION_VALUE);
const orderedOptions = hasAllOption
? [
...field.options.filter((opt) => opt.value === ALL_OPTION_VALUE),
...field.options.filter((opt) => opt.value !== ALL_OPTION_VALUE),
]
: field.options;
const nonAllValues = orderedOptions
.map((opt) => opt.value)
.filter((optValue) => optValue !== ALL_OPTION_VALUE);
// Dropdown variant - use DropdownList with checkboxes
if (field.variant === 'dropdown') {
const optionValues = field.options.map((opt) => opt.value);
const optionSet = new Set(optionValues);
const hasAllOption = optionSet.has(ALL_OPTION_VALUE);
const orderedOptions = hasAllOption
? [
...field.options.filter((opt) => opt.value === ALL_OPTION_VALUE),
...field.options.filter((opt) => opt.value !== ALL_OPTION_VALUE),
]
: field.options;
const nonAllValues = orderedOptions
.map((opt) => opt.value)
.filter((optValue) => optValue !== ALL_OPTION_VALUE);
const normalizeValues = (values: string[]): string[] => {
const deduped = new Set(
values
.map((entry) => entry.trim())
.filter((entry) => entry.length > 0 && optionSet.has(entry)),
);
return orderedOptions.map((opt) => opt.value).filter((optValue) => deduped.has(optValue));
};
const normalizeValues = (values: string[]): string[] => {
const deduped = new Set(
values
.map((entry) => entry.trim())
.filter((entry) => entry.length > 0 && optionSet.has(entry)),
);
return orderedOptions.map((opt) => opt.value).filter((optValue) => deduped.has(optValue));
};
const selectedExplicit = normalizeValues(selected);
const allSelected =
hasAllOption &&
(selectedExplicit.includes(ALL_OPTION_VALUE) ||
(nonAllValues.length > 0 &&
nonAllValues.every((optValue) => selectedExplicit.includes(optValue))));
const selectedExplicit = normalizeValues(selected);
const allSelected =
hasAllOption &&
(selectedExplicit.includes(ALL_OPTION_VALUE) ||
(nonAllValues.length > 0 &&
nonAllValues.every((optValue) => selectedExplicit.includes(optValue))));
// Build parent -> children map for cascading selection
const parentChildMap = new Map<string, string[]>();
orderedOptions.forEach((opt) => {
if (opt.childOf) {
const children = parentChildMap.get(opt.childOf) || [];
children.push(opt.value);
parentChildMap.set(opt.childOf, children);
}
});
// Build parent -> children map for cascading selection
const parentChildMap = new Map<string, string[]>();
orderedOptions.forEach((opt) => {
if (opt.childOf) {
const children = parentChildMap.get(opt.childOf) || [];
children.push(opt.value);
parentChildMap.set(opt.childOf, children);
// Check which children are implicitly selected via parent
const selectedForCascade = allSelected
? selectedExplicit.filter((optValue) => optValue !== ALL_OPTION_VALUE)
: selectedExplicit;
const implicitlySelected = new Set<string>();
selectedForCascade.forEach((val) => {
const children = parentChildMap.get(val);
if (children) {
children.forEach((child) => implicitlySelected.add(child));
}
});
// Build options with disabled state for implicitly selected children
const dropdownOptions = orderedOptions.map((opt) => ({
value: opt.value,
label: opt.label,
disabled: !allSelected && implicitlySelected.has(opt.value),
}));
// For display purposes:
// - if "all" is active, check every option
// - otherwise show explicit + implicit parent/child selections
const displayValue = allSelected
? [ALL_OPTION_VALUE, ...nonAllValues]
: normalizeValues([...selectedExplicit, ...Array.from(implicitlySelected)]);
const handleDropdownChange = (newValue: string | string[]) => {
const nextValues = normalizeValues(Array.isArray(newValue) ? newValue : [newValue]);
if (hasAllOption) {
const includesAll = nextValues.includes(ALL_OPTION_VALUE);
// When currently "all" is active:
// - unticking "all" clears everything
// - unticking a specific option converts to explicit subset
if (allSelected && !includesAll && nextValues.length === nonAllValues.length) {
onChange([]);
return;
}
});
// Check which children are implicitly selected via parent
const selectedForCascade = allSelected
? selectedExplicit.filter((optValue) => optValue !== ALL_OPTION_VALUE)
: selectedExplicit;
const implicitlySelected = new Set<string>();
selectedForCascade.forEach((val) => {
const children = parentChildMap.get(val);
if (children) {
children.forEach((child) => implicitlySelected.add(child));
}
});
// Build options with disabled state for implicitly selected children
const dropdownOptions = orderedOptions.map((opt) => ({
value: opt.value,
label: opt.label,
disabled: !allSelected && implicitlySelected.has(opt.value),
}));
// For display purposes:
// - if "all" is active, check every option
// - otherwise show explicit + implicit parent/child selections
const displayValue = allSelected
? [ALL_OPTION_VALUE, ...nonAllValues]
: normalizeValues([...selectedExplicit, ...Array.from(implicitlySelected)]);
const handleDropdownChange = (newValue: string | string[]) => {
const nextValues = normalizeValues(Array.isArray(newValue) ? newValue : [newValue]);
if (hasAllOption) {
const includesAll = nextValues.includes(ALL_OPTION_VALUE);
// When currently "all" is active:
// - unticking "all" clears everything
// - unticking a specific option converts to explicit subset
if (allSelected && !includesAll && nextValues.length === nonAllValues.length) {
onChange([]);
return;
}
if (allSelected && includesAll && nextValues.length < optionValues.length) {
onChange(nextValues.filter((entry) => entry !== ALL_OPTION_VALUE));
return;
}
if (includesAll) {
onChange([ALL_OPTION_VALUE]);
return;
}
// If user selects every specific option individually, collapse to "all".
if (
nonAllValues.length > 0 &&
nonAllValues.every((optValue) => nextValues.includes(optValue))
) {
onChange([ALL_OPTION_VALUE]);
return;
}
if (allSelected && includesAll && nextValues.length < optionValues.length) {
onChange(nextValues.filter((entry) => entry !== ALL_OPTION_VALUE));
return;
}
// Filter out implicitly selected values - only store explicit selections.
const explicitOnly = nextValues.filter((entry) => !implicitlySelected.has(entry));
onChange(explicitOnly);
};
if (includesAll) {
onChange([ALL_OPTION_VALUE]);
return;
}
// Custom summary formatter - only count explicit selections
const summaryFormatter = () => {
if (allSelected) {
return orderedOptions.find((opt) => opt.value === ALL_OPTION_VALUE)?.label || 'All';
// If user selects every specific option individually, collapse to "all".
if (
nonAllValues.length > 0 &&
nonAllValues.every((optValue) => nextValues.includes(optValue))
) {
onChange([ALL_OPTION_VALUE]);
return;
}
if (selectedExplicit.length === 0) {
return <span className="opacity-60">{field.placeholder || 'Select categories...'}</span>;
}
const selectedLabels = selectedExplicit
.map((v) => orderedOptions.find((o) => o.value === v)?.label)
.filter(Boolean);
if (selectedLabels.length === 1) {
return selectedLabels[0];
}
const [first, second, ...rest] = selectedLabels;
const suffix = rest.length > 0 ? ` +${rest.length}` : '';
return `${first}, ${second ?? ''}${suffix}`.trim();
};
if (isDisabled) {
return (
<div className="w-full cursor-not-allowed rounded-lg border border-(--border-muted) bg-(--bg-soft) px-3 py-2 text-sm opacity-60">
{summaryFormatter()}
</div>
);
}
// Filter out implicitly selected values - only store explicit selections.
const explicitOnly = nextValues.filter((entry) => !implicitlySelected.has(entry));
onChange(explicitOnly);
};
// Custom summary formatter - only count explicit selections
const summaryFormatter = () => {
if (allSelected) {
return orderedOptions.find((opt) => opt.value === ALL_OPTION_VALUE)?.label || 'All';
}
if (selectedExplicit.length === 0) {
return <span className="opacity-60">{field.placeholder || 'Select categories...'}</span>;
}
const selectedLabels = selectedExplicit
.map((v) => orderedOptions.find((o) => o.value === v)?.label)
.filter(Boolean);
if (selectedLabels.length === 1) {
return selectedLabels[0];
}
const [first, second, ...rest] = selectedLabels;
const suffix = rest.length > 0 ? ` +${rest.length}` : '';
return `${first}, ${second ?? ''}${suffix}`.trim();
};
if (isDisabled) {
return (
<DropdownList
options={dropdownOptions}
value={displayValue}
onChange={handleDropdownChange}
multiple
showCheckboxes
keepOpenOnSelect
placeholder={field.placeholder || 'Select categories...'}
widthClassName="w-full"
summaryFormatter={summaryFormatter}
/>
<div className="w-full cursor-not-allowed rounded-lg border border-(--border-muted) bg-(--bg-soft) px-3 py-2 text-sm opacity-60">
{summaryFormatter()}
</div>
);
}
return (
<DropdownList
options={dropdownOptions}
value={displayValue}
onChange={handleDropdownChange}
multiple
showCheckboxes
keepOpenOnSelect
placeholder={field.placeholder || 'Select categories...'}
widthClassName="w-full"
summaryFormatter={summaryFormatter}
/>
);
};
// Pill variant - inline toggle buttons that collapse past a threshold
const MultiSelectPillsField = ({
field,
selected,
onChange,
isDisabled,
}: MultiSelectVariantProps) => {
const [isExpanded, setIsExpanded] = useState(false);
// Initialize based on option count to avoid flash of expanded content
const [needsCollapse, setNeedsCollapse] = useState(
@@ -353,3 +363,35 @@ export const MultiSelectField = ({
</div>
);
};
export const MultiSelectField = ({
field,
value: fieldValue,
onChange,
disabled,
}: MultiSelectFieldProps) => {
const selected = fieldValue ?? EMPTY_SELECTION;
// disabled prop is already computed by SettingsContent.getDisabledState()
const isDisabled = disabled ?? false;
// Each variant is its own component so neither calls hooks conditionally.
if (field.variant === 'dropdown') {
return (
<MultiSelectDropdownField
field={field}
selected={selected}
onChange={onChange}
isDisabled={isDisabled}
/>
);
}
return (
<MultiSelectPillsField
field={field}
selected={selected}
onChange={onChange}
isDisabled={isDisabled}
/>
);
};
@@ -139,6 +139,10 @@ export function Tooltip({
}
if (deltaX !== 0 || deltaY !== 0) {
// Genuine measure-and-adjust: the tooltip must be laid out before we know
// whether it overflows the viewport. The loop converges in one pass because
// the corrected position yields deltaX/deltaY of 0 on the next run.
// oxlint-disable-next-line react/set-state-in-effect
setCoords((current) => {
if (!current) {
return current;
@@ -1,8 +1,8 @@
import { useRef } from 'react';
import { useEffectEvent } from 'react';
import type { Dispatch, SetStateAction } from 'react';
import type { Book } from '../../types';
import { onBookTargetChange } from '../../utils/bookTargetEvents';
import { onBookTargetChange, type BookTargetChangeEvent } from '../../utils/bookTargetEvents';
import { useMountEffect } from '../useMountEffect';
interface UseBookTargetDeselectSyncOptions {
@@ -14,15 +14,14 @@ export const useBookTargetDeselectSync = ({
activeListValue,
setBooks,
}: UseBookTargetDeselectSyncOptions): void => {
const activeListValueRef = useRef(activeListValue);
activeListValueRef.current = activeListValue;
useMountEffect(() => {
return onBookTargetChange((event) => {
if (event.selected) return;
const currentValue = activeListValueRef.current;
if (!currentValue || String(currentValue) !== event.target) return;
setBooks((prev) => prev.filter((book) => book.provider_id !== event.bookId));
});
const handleTargetChange = useEffectEvent((event: BookTargetChangeEvent) => {
if (event.selected) return;
if (!activeListValue || String(activeListValue) !== event.target) return;
setBooks((prev) => prev.filter((book) => book.provider_id !== event.bookId));
});
// Wrapped rather than handed over directly: an Effect Event must not be given to
// something that stores it, and `onBookTargetChange` puts its argument in a
// module-level listener set. Same shape as useDismiss.
useMountEffect(() => onBookTargetChange((event) => handleTargetChange(event)));
};
@@ -1,11 +1,17 @@
import { useCallback, useRef, useState } from 'react';
import { useCallback, useState } from 'react';
import type { Dispatch, SetStateAction } from 'react';
import type { ContentType } from '../../types';
import { useDependencyEffect } from '../useMountEffect';
const CONTENT_TYPE_STORAGE_KEY = 'preferred-content-type';
const readInitialPreference = (): { contentType: ContentType; combinedMode: boolean } => {
interface ContentTypePreference {
contentType: ContentType;
combinedMode: boolean;
}
const readInitialPreference = (): ContentTypePreference => {
try {
const saved = localStorage.getItem(CONTENT_TYPE_STORAGE_KEY);
if (saved === 'combined') {
@@ -26,53 +32,32 @@ export const useContentTypePreferences = (): {
combinedMode: boolean;
setCombinedMode: Dispatch<SetStateAction<boolean>>;
} => {
const initialPreference = readInitialPreference();
const [contentType, setContentTypeState] = useState<ContentType>(
() => initialPreference.contentType,
);
const [combinedMode, setCombinedModeState] = useState<boolean>(
() => initialPreference.combinedMode,
);
const contentTypeRef = useRef(contentType);
const combinedModeRef = useRef(combinedMode);
contentTypeRef.current = contentType;
combinedModeRef.current = combinedMode;
// Both values live in one state object so each setter can derive the other
// from a pure updater instead of mirroring it into a ref during render.
const [preference, setPreference] = useState<ContentTypePreference>(readInitialPreference);
const { contentType, combinedMode } = preference;
const persistPreference = useCallback(
(nextContentType: ContentType, nextCombinedMode: boolean) => {
try {
localStorage.setItem(
CONTENT_TYPE_STORAGE_KEY,
nextCombinedMode ? 'combined' : nextContentType,
);
} catch {
// localStorage may be unavailable in private browsing
}
},
[],
);
const setContentType: Dispatch<SetStateAction<ContentType>> = useCallback((value) => {
setPreference((current) => ({
...current,
contentType: typeof value === 'function' ? value(current.contentType) : value,
}));
}, []);
const setContentType: Dispatch<SetStateAction<ContentType>> = useCallback(
(value) => {
setContentTypeState((current) => {
const nextContentType = typeof value === 'function' ? value(current) : value;
persistPreference(nextContentType, combinedModeRef.current);
return nextContentType;
});
},
[persistPreference],
);
const setCombinedMode: Dispatch<SetStateAction<boolean>> = useCallback((value) => {
setPreference((current) => ({
...current,
combinedMode: typeof value === 'function' ? value(current.combinedMode) : value,
}));
}, []);
const setCombinedMode: Dispatch<SetStateAction<boolean>> = useCallback(
(value) => {
setCombinedModeState((current) => {
const nextCombinedMode = typeof value === 'function' ? value(current) : value;
persistPreference(contentTypeRef.current, nextCombinedMode);
return nextCombinedMode;
});
},
[persistPreference],
);
useDependencyEffect(() => {
try {
localStorage.setItem(CONTENT_TYPE_STORAGE_KEY, combinedMode ? 'combined' : contentType);
} catch {
// localStorage may be unavailable in private browsing
}
}, [contentType, combinedMode]);
return {
contentType,
@@ -44,6 +44,10 @@ export function useDescriptionOverflow({
return () => {
observer.disconnect();
};
// `descriptionKey` is never read here - it is the trigger. When the modal swaps to a
// different release the text changes under the same element, and the overflow has to
// be measured again; drop it and the clamp keeps the previous release's answer.
// oxlint-disable-next-line react/exhaustive-effect-dependencies
}, [descriptionExpanded, descriptionKey, descriptionRef]);
return descriptionOverflows;
@@ -164,7 +164,6 @@ export function useReleaseSearchSession(
const lastStatusTimeRef = useRef(0);
const pendingStatusRef = useRef<SearchStatusData | null>(null);
const statusTimeoutRef = useRef<NodeJS.Timeout | null>(null);
activeTabRef.current = activeTab;
const allTabs = useMemo(() => {
return buildReleaseTabs(
@@ -363,7 +362,6 @@ export function useReleaseSearchSession(
indexerFilterInitializedRef.current = new Set<string>();
const nextInitialActiveTab = preferredDefaultReleaseSource || '';
initialActiveTabRef.current = nextInitialActiveTab;
activeTabRef.current = nextInitialActiveTab;
pendingStatusRef.current = null;
lastStatusTimeRef.current = 0;
if (statusTimeoutRef.current) {
@@ -381,6 +379,7 @@ export function useReleaseSearchSession(
? nextInitialActiveTab
: (tabs[0]?.name ?? '');
activeTabRef.current = nextActiveTab;
setActiveTabState(nextActiveTab);
setReleasesBySource({});
setLoadingBySource({});
@@ -458,6 +457,7 @@ export function useReleaseSearchSession(
const setActiveTab = useCallback(
(tabName: string) => {
activeTabRef.current = tabName;
setActiveTabState(tabName);
if (!tabName) {
+20 -10
View File
@@ -5,24 +5,30 @@ interface TabIndicatorStyle {
width: number;
}
// One shared instance, so the no-active-tab path below can set it repeatedly and React
// bails out on reference equality instead of re-rendering on every resize event.
const HIDDEN_INDICATOR: TabIndicatorStyle = { left: 0, width: 0 };
export function useTabIndicator(
tabRefs: MutableRefObject<Record<string, HTMLButtonElement | null>>,
activeTab: string,
tabsDependency: unknown,
): TabIndicatorStyle {
const [tabIndicatorStyle, setTabIndicatorStyle] = useState({
left: 0,
width: 0,
});
const [tabIndicatorStyle, setTabIndicatorStyle] = useState<TabIndicatorStyle>(HIDDEN_INDICATOR);
useLayoutEffect(() => {
const activeButton = tabRefs.current[activeTab];
if (!activeButton) {
setTabIndicatorStyle({ left: 0, width: 0 });
return undefined;
}
// Single measurement path, so a resize that removes the active tab also
// resets the indicator instead of leaving it stranded.
const updateIndicator = () => {
const activeButton = tabRefs.current[activeTab];
if (!activeButton) {
// The shared constant, not a fresh literal: this path now runs on every resize
// event, and a new object would never be Object.is-equal to the current state,
// so React would re-render on every frame of a window drag for an unchanged value.
setTabIndicatorStyle(HIDDEN_INDICATOR);
return;
}
const containerRect = activeButton.parentElement?.getBoundingClientRect();
const buttonRect = activeButton.getBoundingClientRect();
if (!containerRect) {
@@ -41,6 +47,10 @@ export function useTabIndicator(
return () => {
window.removeEventListener('resize', updateIndicator);
};
// `tabsDependency` is never read here - it exists only to re-run the measurement when
// the tab set changes (callers pass `allTabs` / `showRequestsTab`). The buttons move
// when tabs are added or removed, so without it the indicator sits under the old one.
// oxlint-disable-next-line react/exhaustive-effect-dependencies
}, [activeTab, tabRefs, tabsDependency]);
return tabIndicatorStyle;
+20 -24
View File
@@ -1,47 +1,43 @@
import { useEffect, useEffectEvent, useRef, type RefObject } from 'react';
import { useEffect, useEffectEvent, type RefObject } from 'react';
export const useDismiss = (
isOpen: boolean,
refs: RefObject<HTMLElement | null>[],
onClose: () => void,
) => {
const handleClose = useEffectEvent(() => {
const handlePointerDown = useEffectEvent((event: MouseEvent) => {
const target = event.target;
if (!(target instanceof Node)) {
return;
}
if (refs.some((ref) => ref.current?.contains(target))) {
return;
}
onClose();
});
const refsRef = useRef(refs);
refsRef.current = refs;
const handleEscape = useEffectEvent((event: KeyboardEvent) => {
if (event.key === 'Escape') {
onClose();
}
});
useEffect(() => {
if (!isOpen) {
return undefined;
}
const handleClickOutside = (event: MouseEvent) => {
const target = event.target;
if (!(target instanceof Node)) {
return;
}
if (refsRef.current.some((ref) => ref.current?.contains(target))) {
return;
}
handleClose();
};
const handleEscape = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
handleClose();
}
};
const handleClickOutside = (event: MouseEvent) => handlePointerDown(event);
const handleKeyDown = (event: KeyboardEvent) => handleEscape(event);
document.addEventListener('mousedown', handleClickOutside);
document.addEventListener('keydown', handleEscape);
document.addEventListener('keydown', handleKeyDown);
return () => {
document.removeEventListener('mousedown', handleClickOutside);
document.removeEventListener('keydown', handleEscape);
document.removeEventListener('keydown', handleKeyDown);
};
}, [isOpen]);
};
@@ -0,0 +1,31 @@
import { useCallback, useLayoutEffect, useRef } from 'react';
/**
* A callback with a stable identity that always runs the latest render's implementation.
*
* `useEffectEvent` is React's answer to this shape, but its contract is narrower than it
* looks: an Effect Event may only be called from inside an Effect, and must not be handed
* to another component, stored in state, or registered with something that outlives the
* Effect. Handlers that run from a DOM event, from an async continuation, or from a parent
* holding the function in its own UI state are all outside that contract - React documents
* the behaviour there as undefined, and the React Compiler advisories oxlint reports
* ("existing memoization could not be preserved") are the same fact from the other side.
*
* So this is the supported shape for those callers. The ref is published in a layout
* effect - after commit, before paint - rather than assigned during render, so a render
* React later throws away cannot leak its closure into a handler, and no event can
* observe the gap.
*
* Use `useEffectEvent` when the caller really is an Effect; use this everywhere else.
*/
export function useLatestCallback<Args extends unknown[], Result>(
callback: (...args: Args) => Result,
): (...args: Args) => Result {
const callbackRef = useRef(callback);
useLayoutEffect(() => {
callbackRef.current = callback;
});
return useCallback((...args: Args) => callbackRef.current(...args), []);
}
+5 -7
View File
@@ -1,16 +1,14 @@
import { useEffect, useRef, type DependencyList, type EffectCallback } from 'react';
import { useEffect, useEffectEvent, type DependencyList, type EffectCallback } from 'react';
export function useMountEffect(effect: EffectCallback): void {
const effectRef = useRef(effect);
effectRef.current = effect;
const runEffect = useEffectEvent(effect);
useEffect(() => effectRef.current(), []);
useEffect(() => runEffect(), []);
}
export function useDependencyEffect(effect: EffectCallback, deps: DependencyList): void {
const effectRef = useRef(effect);
effectRef.current = effect;
const runEffect = useEffectEvent(effect);
// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => effectRef.current(), deps);
useEffect(() => runEffect(), deps);
}
+2 -6
View File
@@ -5,6 +5,7 @@ import { DEFAULT_SUPPORTED_FORMATS } from '../data/languages';
import { searchBooks, searchMetadata, AuthenticationError } from '../services/api';
import type { Book, AppConfig, AdvancedFilterState, ContentType, SearchMode } from '../types';
import { LANGUAGE_OPTION_DEFAULT } from '../utils/languageFilters';
import { describeSearchFailure } from '../utils/searchFailureMessage';
const DEFAULT_FORMAT_SELECTION = DEFAULT_SUPPORTED_FORMATS;
@@ -263,12 +264,7 @@ export function useSearch(options: UseSearchOptions): UseSearchReturn {
handleSearchError(error, 'Search failed');
} else {
console.error('Search failed:', error);
const message = error instanceof Error ? error.message : 'Search failed';
const friendly =
message.includes('Network restricted') || message.includes('Unable to reach')
? message
: 'Unable to reach download source. Network may be restricted or mirrors blocked.';
showToast(friendly, 'error');
showToast(describeSearchFailure(error), 'error');
}
} finally {
setIsSearching(false);
+6 -14
View File
@@ -1,4 +1,4 @@
import { useState, useCallback, useRef } from 'react';
import { useState, useCallback } from 'react';
import { getSettings, updateSettings, executeSettingsAction } from '../services/api';
import type {
@@ -23,6 +23,7 @@ import {
setThemePreference,
THEME_FIELD,
} from '../utils/themePreference';
import { useLatestCallback } from './useLatestCallback';
import { useMountEffect } from './useMountEffect';
interface FetchSettingsOptions {
@@ -137,13 +138,9 @@ export function useSettings(): UseSettingsReturn {
() => initialState?.originalValues ?? {},
);
const [isSaving, setIsSaving] = useState(false);
const valuesRef = useRef<SettingsValues>({});
const originalValuesRef = useRef<SettingsValues>({});
valuesRef.current = values;
originalValuesRef.current = originalValues;
const applySettingsResponse = useCallback(
// Stable identity, latest `values`/`originalValues`. Not an Effect Event: it is called
// from async fetch and save continuations, not from an Effect. See useLatestCallback.
const applySettingsResponse = useLatestCallback(
(response: SettingsResponse, options: { preserveDirtyValues?: boolean } = {}) => {
const { preserveDirtyValues = false } = options;
cachedSettingsResponse = response;
@@ -156,11 +153,7 @@ export function useSettings(): UseSettingsReturn {
setError(null);
const nextValues = preserveDirtyValues
? mergeFetchedSettingsWithDirtyValues(
hydratedState.values,
valuesRef.current,
originalValuesRef.current,
)
? mergeFetchedSettingsWithDirtyValues(hydratedState.values, values, originalValues)
: hydratedState.values;
setValues(nextValues);
@@ -170,7 +163,6 @@ export function useSettings(): UseSettingsReturn {
setSelectedTab((current) => current ?? hydratedState.selectedTab);
}
},
[],
);
const fetchSettings = useCallback(
+52 -9
View File
@@ -84,6 +84,9 @@ type ApiResponseErrorShape = Error & {
code?: string;
requiredMode?: string;
payload?: Record<string, unknown>;
// Set only when the server explained itself, so callers can tell a real explanation
// apart from the `503 SERVICE UNAVAILABLE` placeholder built from the status line.
serverMessage?: string;
};
class ApiResponseError extends Error {
@@ -91,6 +94,7 @@ class ApiResponseError extends Error {
code?: string;
requiredMode?: string;
payload?: Record<string, unknown>;
serverMessage?: string;
constructor(
message: string,
@@ -99,6 +103,7 @@ class ApiResponseError extends Error {
code?: string;
requiredMode?: string;
payload?: Record<string, unknown>;
serverMessage?: string;
},
) {
super(message);
@@ -107,6 +112,7 @@ class ApiResponseError extends Error {
this.code = params.code;
this.requiredMode = params.requiredMode;
this.payload = params.payload;
this.serverMessage = params.serverMessage;
}
}
@@ -114,6 +120,13 @@ export const isApiResponseError = (error: unknown): error is ApiResponseErrorSha
return error instanceof ApiResponseError;
};
// The client gave up before the server answered. Distinguishable so callers can report
// the wait rather than guessing at a cause: a search that hits this has told us nothing
// about the network or the mirrors, and saying it did is what issue #1285 was about.
export const isTimeoutError = (error: unknown): error is Error => {
return error instanceof TimeoutError;
};
const mapApiErrorToActionResult = (error: unknown): ActionResult | null => {
if (!isApiResponseError(error) || !error.payload) {
return null;
@@ -149,7 +162,31 @@ const DEFAULT_TIMEOUT_MS = 30000;
// Release searches can be long-running: a source behind Cloudflare/DDoS-Guard has
// to spin up the bypasser and solve the challenge before any results come back,
// which routinely takes well over the default timeout.
const SEARCH_TIMEOUT_MS = 180000;
//
// The server bounds them itself (RELEASE_SEARCH_TIMEOUT, reported by /api/config) and
// answers a spent budget with a message naming the real cause. This client abort is only
// the backstop for a server that never answers at all, so it has to fire *after* the
// server's own deadline - a fixed 180s here beat the 300s default, so the accurate
// message was never reachable and raising the setting did nothing. See issue #1285.
//
// The margin has to cover what the server still has to do *after* its budget trips, not
// just the budget itself. The deadline is cooperative: it is handed to the bypasser as a
// cancel flag, and internal_bypasser._CDP_UNWIND_GRACE_SECONDS allows 15s on its own for a
// cancelled solve to close its browser - before the handler has serialized releases, built
// the column config and put bytes on the wire. A 15s margin is entirely spent by that
// unwind, so give it room for the unwind plus the response.
const SEARCH_TIMEOUT_MARGIN_MS = 45000;
const FALLBACK_SEARCH_TIMEOUT_MS = 300000; // search_deadline.DEFAULT_SEARCH_BUDGET_SECONDS
let searchTimeoutMs = FALLBACK_SEARCH_TIMEOUT_MS + SEARCH_TIMEOUT_MARGIN_MS;
// Exported for tests; callers get this applied automatically via getConfig().
export const setSearchTimeoutFromConfig = (budgetSeconds: unknown): void => {
if (typeof budgetSeconds === 'number' && Number.isFinite(budgetSeconds) && budgetSeconds > 0) {
searchTimeoutMs = budgetSeconds * 1000 + SEARCH_TIMEOUT_MARGIN_MS;
}
};
export const getSearchTimeoutMs = (): number => searchTimeoutMs;
// Utility function for JSON fetch with credentials and timeout
async function fetchJSON<T>(
@@ -183,12 +220,15 @@ async function fetchJSON<T>(
if (isRecord(parsed) && !Array.isArray(parsed)) {
errorData = parsed;
}
// Prefer user-friendly 'message' field, fall back to 'error'
if (typeof errorData?.message === 'string') {
errorMessage = errorData.message;
hasServerMessage = true;
} else if (typeof errorData?.error === 'string') {
errorMessage = errorData.error;
// Prefer user-friendly 'message' field, fall back to 'error'. Both must carry
// actual text: an empty string is not the server explaining itself, and treating
// it as one suppresses the placeholder below and shows the user a blank toast.
const explanation = [errorData?.message, errorData?.error].find(
(candidate): candidate is string =>
typeof candidate === 'string' && candidate.trim() !== '',
);
if (explanation !== undefined) {
errorMessage = explanation;
hasServerMessage = true;
}
} catch (e) {
@@ -213,6 +253,7 @@ async function fetchJSON<T>(
throw new ApiResponseError(errorMessage, {
status: res.status,
serverMessage: hasServerMessage ? errorMessage : undefined,
code: typeof errorData?.code === 'string' ? errorData.code : undefined,
requiredMode:
typeof errorData?.required_mode === 'string' ? errorData.required_mode : undefined,
@@ -243,7 +284,7 @@ export const searchBooks = async (query: string): Promise<Book[]> => {
const response = await fetchJSON<ReleasesResponse>(
`${API_BASE}/releases?source=direct_download&${query}`,
{},
SEARCH_TIMEOUT_MS,
searchTimeoutMs,
);
return response.releases.map(transformReleaseToDirectBook);
};
@@ -586,7 +627,9 @@ export const retryDownload = async (id: string): Promise<void> => {
};
export const getConfig = async (): Promise<AppConfig> => {
return fetchJSON<AppConfig>(API.config);
const config = await fetchJSON<AppConfig>(API.config);
setSearchTimeoutFromConfig(config.release_search_timeout);
return config;
};
interface ActivityDismissedItem {
@@ -0,0 +1,62 @@
import { describe, it, expect } from 'vitest';
import {
INITIAL_ENTER_ANIMATION,
nextEnterAnimation,
type EnterAnimationState,
} from '../utils/releaseModalEnterAnimation';
describe('nextEnterAnimation', () => {
it('animates the first session', () => {
const next = nextEnterAnimation(INITIAL_ENTER_ANIMATION, 'book-1', false);
expect(next).toEqual({ key: 'book-1', animate: true });
});
it('animates the first session in combined mode too', () => {
const next = nextEnterAnimation(INITIAL_ENTER_ANIMATION, 'book-1', true);
expect(next).toEqual({ key: 'book-1', animate: true });
});
it('does not animate a step transition between combined-mode sessions', () => {
const current: EnterAnimationState = { key: 'book-1', animate: true };
expect(nextEnterAnimation(current, 'book-2', true)).toEqual({
key: 'book-2',
animate: false,
});
});
it('animates a session swap outside combined mode', () => {
const current: EnterAnimationState = { key: 'book-1', animate: true };
expect(nextEnterAnimation(current, 'book-2', false)).toEqual({
key: 'book-2',
animate: true,
});
});
it('holds the decision across re-renders of the same session', () => {
// Regression: the decision used to flip back to `true` on the next render,
// replaying the enter animation mid-session.
const stepped = nextEnterAnimation({ key: 'book-1', animate: true }, 'book-2', true);
expect(stepped.animate).toBe(false);
let state = stepped;
for (let i = 0; i < 5; i++) {
state = nextEnterAnimation(state, 'book-2', true);
expect(state.animate).toBe(false);
}
});
it('returns the same reference when the session is unchanged', () => {
const current: EnterAnimationState = { key: 'book-1', animate: false };
expect(nextEnterAnimation(current, 'book-1', true)).toBe(current);
});
it('animates again after the modal closes and reopens', () => {
const open = nextEnterAnimation(INITIAL_ENTER_ANIMATION, 'book-1', true);
const closed = nextEnterAnimation(open, null, true);
expect(closed.key).toBeNull();
const reopened = nextEnterAnimation(closed, 'book-1', true);
expect(reopened.animate).toBe(true);
});
});
@@ -0,0 +1,77 @@
import { describe, it, expect, vi, afterEach } from 'vitest';
import { searchBooks } from '../services/api';
import {
describeSearchFailure,
CLIENT_TIMEOUT_MESSAGE,
UNREACHABLE_SOURCE_MESSAGE,
} from '../utils/searchFailureMessage';
/**
* What a failed direct-mode search tells the user.
*
* Every non-auth failure used to be relabelled "Unable to reach download source. Network
* may be restricted or mirrors blocked.", which threw away the server's explanation and
* blamed the user's network for a protection challenge. See issue #1285.
*/
const jsonResponse = (body: unknown, status: number): Response =>
new Response(JSON.stringify(body), {
status,
statusText: 'SERVICE UNAVAILABLE',
headers: { 'Content-Type': 'application/json' },
});
/** Drive a real searchBooks() failure so the error is the one the hook actually sees. */
const failedSearch = async (respond: () => Promise<Response>): Promise<unknown> => {
vi.stubGlobal('fetch', vi.fn(respond));
return searchBooks('q=dune').catch((error: unknown) => error);
};
describe('describeSearchFailure', () => {
afterEach(() => {
vi.unstubAllGlobals();
});
it('shows the sentence the server sent', async () => {
const sentence = 'The release search ran out of time (300s).';
const error = await failedSearch(() => Promise.resolve(jsonResponse({ error: sentence }, 503)));
expect(describeSearchFailure(error)).toBe(sentence);
});
it('names the wait when the client gave up first', async () => {
// The client's abort is the backstop for a server that never answered. It tells us
// nothing about mirrors or the network, and the old chain reported it as if it did.
const abort = Object.assign(new Error('The operation was aborted.'), { name: 'AbortError' });
const error = await failedSearch(() => Promise.reject(abort));
expect(describeSearchFailure(error)).toBe(CLIENT_TIMEOUT_MESSAGE);
expect(describeSearchFailure(error)).not.toBe(UNREACHABLE_SOURCE_MESSAGE);
expect(describeSearchFailure(error)).not.toContain('mirrors');
});
it('falls back to the mirrors line only when nothing explained itself', async () => {
const error = await failedSearch(() => Promise.resolve(jsonResponse({}, 503)));
expect(describeSearchFailure(error)).toBe(UNREACHABLE_SOURCE_MESSAGE);
});
it('keeps a reachability message that already says the right thing', () => {
const error = new Error('Unable to reach download source. Every mirror was quarantined.');
expect(describeSearchFailure(error)).toBe(error.message);
});
it('never produces an empty sentence from a blank server message', async () => {
// `{"message": ""}` is not the server explaining itself. Treating it as one used to
// reach showToast('') and render an empty error toast.
const error = await failedSearch(() => Promise.resolve(jsonResponse({ message: '' }, 503)));
expect(describeSearchFailure(error)).toBe(UNREACHABLE_SOURCE_MESSAGE);
});
it('handles a non-Error rejection without inventing detail', () => {
expect(describeSearchFailure('something odd')).toBe(UNREACHABLE_SOURCE_MESSAGE);
});
});
@@ -0,0 +1,197 @@
import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest';
import {
getConfig,
getSearchTimeoutMs,
setSearchTimeoutFromConfig,
searchBooks,
isApiResponseError,
} from '../services/api';
/**
* The client's abort must fire *after* the server's own search deadline.
*
* `/api/releases` bounds itself with RELEASE_SEARCH_TIMEOUT and answers a spent budget
* with a message naming the real cause. The client aborted at a fixed 180s against a
* 300s default, so it always won the race and replaced that message with "Request timed
* out. Check your network connection or proxy configuration." Raising the setting had no
* visible effect either, the 180s being baked into the hashed bundle. See issue #1285.
*/
// Must stay ahead of what the server still has to do after its budget trips: the deadline
// is cooperative, and internal_bypasser._CDP_UNWIND_GRACE_SECONDS alone allows 15s for a
// cancelled solve to close its browser before the response is even built.
const MARGIN_MS = 45_000;
const SERVER_UNWIND_GRACE_MS = 15_000;
const jsonResponse = (body: unknown, status = 200): Response =>
new Response(JSON.stringify(body), {
status,
statusText: status === 200 ? 'OK' : 'SERVICE UNAVAILABLE',
headers: { 'Content-Type': 'application/json' },
});
const configBody = (releaseSearchTimeout: number): Record<string, unknown> => ({
release_search_timeout: releaseSearchTimeout,
});
describe('release search timeout', () => {
beforeEach(() => {
setSearchTimeoutFromConfig(300);
});
afterEach(() => {
vi.unstubAllGlobals();
setSearchTimeoutFromConfig(300);
});
it('defaults behind the server default rather than ahead of it', () => {
expect(getSearchTimeoutMs()).toBe(300 * 1000 + MARGIN_MS);
});
it('follows the budget the server reports', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse(configBody(900)))),
);
await getConfig();
expect(getSearchTimeoutMs()).toBe(900 * 1000 + MARGIN_MS);
});
it('still outlasts the server when the budget is lowered', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse(configBody(30)))),
);
await getConfig();
// Exact, not a lower bound: `> 30_000` is also satisfied by the 345_000 left over
// from the previous budget, so a setter that silently stopped applying the config
// would pass it.
expect(getSearchTimeoutMs()).toBe(30 * 1000 + MARGIN_MS);
});
it('leaves the server room to unwind a cancelled solve and answer', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse(configBody(300)))),
);
await getConfig();
// The failure this exists for is a budget spent mid-solve. The server then has to
// close a browser before it can serialize anything, so a margin merely equal to that
// unwind is entirely spent by it and the client aborts first all over again.
expect(getSearchTimeoutMs() - 300 * 1000).toBeGreaterThan(SERVER_UNWIND_GRACE_MS);
});
it('ignores a missing or nonsensical budget instead of disabling the backstop', () => {
const before = getSearchTimeoutMs();
setSearchTimeoutFromConfig(undefined);
setSearchTimeoutFromConfig(0);
setSearchTimeoutFromConfig(-1);
setSearchTimeoutFromConfig('600');
setSearchTimeoutFromConfig(Number.NaN);
expect(getSearchTimeoutMs()).toBe(before);
});
it('applies the derived timeout to the direct_download search', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse(configBody(600)))),
);
await getConfig();
const seen: Array<AbortSignal | undefined> = [];
vi.stubGlobal(
'fetch',
vi.fn((_url: string, init: RequestInit) => {
seen.push(init.signal ?? undefined);
return Promise.resolve(jsonResponse({ releases: [] }));
}),
);
await searchBooks('q=dune');
// The request carries an abort signal, and it is not yet aborted: the point is that
// the clock it runs on is the server's, not a constant.
expect(seen).toHaveLength(1);
expect(seen[0]?.aborted).toBe(false);
expect(getSearchTimeoutMs()).toBe(600 * 1000 + MARGIN_MS);
});
});
describe('server-provided failure messages', () => {
afterEach(() => {
vi.unstubAllGlobals();
});
it('carries the server sentence through instead of a status placeholder', async () => {
const sentence =
'The release search ran out of time (300s). Anna’s Archive is behind a ' +
'protection challenge the bypasser could not solve in that window.';
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse({ error: sentence }, 503))),
);
const error = await searchBooks('q=dune').catch((e: unknown) => e);
expect(isApiResponseError(error)).toBe(true);
if (isApiResponseError(error)) {
expect(error.serverMessage).toBe(sentence);
}
});
it('leaves serverMessage unset when the server explained nothing', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse({}, 503))),
);
const error = await searchBooks('q=dune').catch((e: unknown) => e);
expect(isApiResponseError(error)).toBe(true);
if (isApiResponseError(error)) {
expect(error.serverMessage).toBeUndefined();
// Without this the UI would show a bare "503 SERVICE UNAVAILABLE".
expect(error.message).toContain('Server unavailable');
}
});
it('treats a blank message as no explanation at all', async () => {
// An empty string is not the server explaining itself. Taking it as one suppresses
// the placeholder below *and* survives a `??` fallback, leaving an empty toast.
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse({ message: ' ' }, 503))),
);
const error = await searchBooks('q=dune').catch((e: unknown) => e);
expect(isApiResponseError(error)).toBe(true);
if (isApiResponseError(error)) {
expect(error.serverMessage).toBeUndefined();
expect(error.message).toContain('Server unavailable');
}
});
it('falls back to `error` when `message` is blank', async () => {
vi.stubGlobal(
'fetch',
vi.fn(() => Promise.resolve(jsonResponse({ message: '', error: 'the real reason' }, 503))),
);
const error = await searchBooks('q=dune').catch((e: unknown) => e);
expect(isApiResponseError(error)).toBe(true);
if (isApiResponseError(error)) {
expect(error.serverMessage).toBe('the real reason');
}
});
});
+1
View File
@@ -285,6 +285,7 @@ export interface AppConfig {
auto_open_downloads_sidebar: boolean; // Auto-open sidebar when download is queued
hardcover_auto_remove_on_download: boolean; // Auto-remove from active Hardcover list on download
download_to_browser_content_types: string[]; // Auto-download completed files to browser for selected content types
release_search_timeout: number; // Server-side budget for one release search, in seconds
settings_enabled: boolean; // Whether config directory is mounted and writable
onboarding_complete: boolean; // Whether the user has completed initial setup
default_sort: string; // Default sort for direct mode
+1 -1
View File
@@ -1,4 +1,4 @@
type BookTargetChangeEvent = {
export type BookTargetChangeEvent = {
provider: string;
bookId: string;
target: string;
@@ -0,0 +1,29 @@
export interface EnterAnimationState {
key: string | null;
animate: boolean;
}
export const INITIAL_ENTER_ANIMATION: EnterAnimationState = { key: null, animate: true };
/**
* Decide whether a release modal session should play its enter animation.
*
* The decision is made once, when a session key first appears, and then held for
* that session's lifetime — re-rendering mid-session must not restart the
* animation. Swapping between sessions in combined mode is a step transition
* rather than an entrance, so it does not animate.
*/
export const nextEnterAnimation = (
current: EnterAnimationState,
sessionKey: string | null,
isCombinedMode: boolean,
): EnterAnimationState => {
if (current.key === sessionKey) {
return current;
}
return {
key: sessionKey,
animate: !isCombinedMode || current.key === null,
};
};
@@ -0,0 +1,39 @@
import { isApiResponseError, isTimeoutError } from '../services/api';
// Shown when we genuinely have nothing better: the request failed without the server
// saying why, which really can mean blocked mirrors or a restricted network.
export const UNREACHABLE_SOURCE_MESSAGE =
'Unable to reach download source. Network may be restricted or mirrors blocked.';
// Shown when the client's own abort fired. It is the backstop for a server that never
// answered at all, and it says nothing about mirrors - the budget the client waited out
// is the server's own, which the user can raise. See issue #1285.
export const CLIENT_TIMEOUT_MESSAGE =
'The search took longer than the server said it would. Raise the release search ' +
'timeout if your setup is simply slow.';
/**
* The sentence to show for a failed direct-mode search.
*
* The server knows why a search failed - a spent search budget, an unsolved protection
* challenge - and blanket-replacing that with the mirrors line told users their network
* was broken when it was not. So: the server's own words when it explained itself, the
* timeout line when we gave up before it answered, and the mirrors line only when
* neither applies. See issue #1285.
*/
export const describeSearchFailure = (error: unknown): string => {
if (isTimeoutError(error)) {
return CLIENT_TIMEOUT_MESSAGE;
}
if (isApiResponseError(error) && error.serverMessage) {
return error.serverMessage;
}
const message = error instanceof Error ? error.message : '';
if (message.includes('Network restricted') || message.includes('Unable to reach')) {
return message;
}
return UNREACHABLE_SOURCE_MESSAGE;
};
+88
View File
@@ -0,0 +1,88 @@
"""No method in the list may be a step another method already takes first.
`BYPASS_METHODS` used to open with a solve-only entry that called `page.solve_captcha()`
and checked the result. `_bypass_method_cdp_gui_click`, the entry behind it, opens by
doing exactly that and returns the moment it works - so against a challenge that
`solve_captcha()` cannot clear, the first method could only repeat the half that had
already failed, then charge the loop's backoff before the method that does work started.
Measured on Anna's Archive at 0/19 successes and ~5.5s of the ~26s each solve cost
(issue #1285).
The passive-solve window added in v1.3.13 keeps most solves away from this loop entirely,
so this is about what the loop costs when it does run.
"""
import asyncio
import pytest
import shelfmark.bypass.internal_bypasser as ib
@pytest.fixture
def no_sleep(monkeypatch):
async def _no_sleep(_seconds) -> None:
return None
monkeypatch.setattr(ib.asyncio, "sleep", _no_sleep)
monkeypatch.setattr(ib._RNG, "uniform", lambda _a, _b: 0)
class _Page:
"""Records what a method asked the page to do."""
def __init__(self, *, solve_clears: bool) -> None:
self.solve_clears = solve_clears
self.calls: list[str] = []
async def solve_captcha(self) -> None:
self.calls.append("solve_captcha")
async def is_element_visible(self, selector: str) -> bool:
self.calls.append(f"visible:{selector}")
return False
async def click_with_offset(self, selector: str, _x, _y, center=True) -> None:
self.calls.append(f"click:{selector}")
def _stub_is_bypassed(monkeypatch, page: _Page) -> None:
async def _is_bypassed(*_a, **_kw) -> bool:
return page.solve_clears and "solve_captcha" in page.calls
monkeypatch.setattr(ib, "_is_bypassed", _is_bypassed)
def test_no_solve_only_method_remains_in_the_list():
names = [method.__name__ for method in ib.BYPASS_METHODS]
assert "_bypass_method_cdp_solve" not in names
assert names[0] == "_bypass_method_cdp_gui_click"
def test_the_first_method_still_tries_solve_captcha_first(monkeypatch, no_sleep):
"""Coverage is only preserved because gui_click opens with the same call."""
page = _Page(solve_clears=True)
_stub_is_bypassed(monkeypatch, page)
assert asyncio.run(ib.BYPASS_METHODS[0](page)) is True
assert page.calls == ["solve_captcha"], "it must return before touching any selector"
def test_it_falls_through_to_clicking_when_solve_does_not_clear(monkeypatch, no_sleep):
"""The half that actually works on DDoS-Guard still runs in the same attempt."""
page = _Page(solve_clears=False)
_stub_is_bypassed(monkeypatch, page)
assert asyncio.run(ib.BYPASS_METHODS[0](page)) is False
assert page.calls[0] == "solve_captcha"
assert any(call.startswith("visible:") for call in page.calls), (
"the selector pass should have been reached in the same attempt"
)
def test_the_derived_budgets_follow_the_shortened_list():
"""Both budgets are computed from the list, so removing an entry must not strand them."""
assert ib._BYPASS_METHOD_ATTEMPTS == len(ib.BYPASS_METHODS) + 1
assert ib._BYPASS_METHOD_ATTEMPTS >= len(ib.BYPASS_METHODS), (
"every method must still get a turn"
)
+41
View File
@@ -137,3 +137,44 @@ def test_no_budget_leaks_out_of_the_request(client, main_module):
_request(client, main_module, [{"name": "direct_download", "enabled": True}], lambda *_: [])
assert search_deadline.current() is None
def test_the_client_is_told_what_the_budget_is(client, main_module):
"""The browser has to outlast the server, or the message above never arrives.
The frontend puts its own AbortController on the direct_download search. That abort
was a fixed 180s while this budget defaults to 300s, so the client always gave up
first and replaced the sentence tested above with a generic network/proxy error -
and raising RELEASE_SEARCH_TIMEOUT changed nothing a user could see, because the
hard-coded 180s was in the hashed bundle inside the image. Reporting the budget lets
the client set its backstop behind it. See issue #1285.
"""
_authenticate(client)
with patch.object(main_module, "get_auth_mode", return_value="none"):
resp = client.get("/api/config")
assert resp.status_code == 200
reported = resp.get_json()["release_search_timeout"]
assert reported == search_deadline.budget_seconds()
assert reported > 0
def test_the_reported_budget_is_the_one_actually_enforced(client, main_module):
"""An out-of-range setting is clamped, so the raw config value would mislead."""
_authenticate(client)
with (
patch.object(main_module, "get_auth_mode", return_value="none"),
patch.object(
main_module.app_config,
"get",
side_effect=lambda key, default=None, **_kw: (
99999 if key == "RELEASE_SEARCH_TIMEOUT" else default
),
),
):
resp = client.get("/api/config")
reported = resp.get_json()["release_search_timeout"]
assert reported == search_deadline._MAX_SEARCH_BUDGET_SECONDS
+140
View File
@@ -0,0 +1,140 @@
"""A release search uses one author, not every contributor the book lists.
`bookTransformers.ts` joins a book's authors with ", " for display, and the release modal
sends that display string back as the `author` request parameter; several providers set
`search_author` from equally joined text. `pick_search_author` returned it verbatim while
the authors[] fallback beside it deliberately narrowed to the first name, so a book with
translators was searched for as
Blindness Jose Saramago, Giovanni Pontiero, <persian translator>
which matches nothing on Anna's Archive. The bypass succeeds, the search returns empty,
and the user is told the book has no releases. Reported on issue #1252.
"""
from shelfmark.core.search_plan import build_release_search_plan, first_author, pick_search_author
from shelfmark.metadata_providers import BookMetadata
# The exact string from the report, as the frontend would join it.
JOINED = "José Saramago, Giovanni Pontiero, زهره افتخاری"
def _book(**kwargs) -> BookMetadata:
base = {
"provider": "manual",
"provider_id": "x",
"title": "Blindness",
"search_title": "Blindness",
}
return BookMetadata(**{**base, **kwargs})
def _queries(book: BookMetadata) -> list[str]:
plan = build_release_search_plan(book, languages=["en"])
return [f"{v.title} {plan.author}".strip() for v in plan.grouped_title_variants]
def test_search_author_is_narrowed_to_the_first_author():
book = _book(search_author=JOINED, authors=[a.strip() for a in JOINED.split(",")])
assert _queries(book) == ["Blindness José Saramago"]
def test_the_authors_fallback_still_narrows():
"""Unchanged behaviour, kept under test so the two paths cannot drift apart again."""
book = _book(authors=[JOINED])
assert _queries(book) == ["Blindness José Saramago"]
def test_a_single_author_is_left_alone():
book = _book(search_author="José Saramago")
assert _queries(book) == ["Blindness José Saramago"]
def test_a_book_with_no_author_still_searches_by_title():
book = _book(authors=[])
assert _queries(book) == ["Blindness"]
def test_last_first_collapses_to_the_surname():
"""Still a usable search term, and what the fallback has always done."""
assert first_author("Saramago, José") == "Saramago"
def test_first_author_trims_and_tolerates_odd_input():
assert first_author(" José Saramago ") == "José Saramago"
assert first_author("") == ""
assert first_author(",") == ""
assert first_author("José Saramago,") == "José Saramago"
def test_manual_query_is_untouched():
"""A manual query is the user's own words; narrowing it would rewrite their search."""
book = _book(search_author=JOINED)
plan = build_release_search_plan(book, manual_query=JOINED)
assert plan.manual_query == JOINED
assert plan.author == ""
def test_irc_query_uses_one_author_too():
"""The IRC source builds its own query and had the same verbatim preference."""
from shelfmark.release_sources.irc.source import IRCReleaseSource
book = _book(search_author=JOINED, authors=[a.strip() for a in JOINED.split(",")])
assert IRCReleaseSource()._build_query(book) == "Blindness José Saramago"
def test_a_blank_leading_contributor_falls_back_instead_of_dropping_the_author():
"""`authors.join(', ')` does not drop an empty entry, so the join can start with ",".
Narrowing that to "" and stopping would search by title alone - losing an author the
book was holding all along, in authors[], one line below.
"""
book = _book(search_author=", Giovanni Pontiero", authors=["", "Giovanni Pontiero"])
assert _queries(book) == ["Blindness Giovanni Pontiero"]
def test_a_blank_leading_contributor_does_not_strand_the_irc_query_either():
from shelfmark.release_sources.irc.source import IRCReleaseSource
book = _book(search_author=", Giovanni Pontiero", authors=["", "Giovanni Pontiero"])
assert IRCReleaseSource()._build_query(book) == "Blindness Giovanni Pontiero"
def test_an_all_blank_author_leaves_a_clean_title_only_query():
"""No trailing space, no empty part: the query is just the title."""
from shelfmark.release_sources.irc.source import IRCReleaseSource
book = _book(search_author=", ,", authors=["", " "])
assert _queries(book) == ["Blindness"]
assert IRCReleaseSource()._build_query(book) == "Blindness"
def test_a_bare_string_in_authors_is_not_iterated_character_by_character():
"""The IRC source guarded against this before it shared `pick_search_author`."""
book = _book(authors="José Saramago")
assert pick_search_author(book) == "José Saramago"
def test_the_producers_narrow_before_the_field_is_ever_set():
"""The two places that join also hold the split, so they set search_author from it.
Narrowing downstream cannot tell a comma that joins contributors from one inside a
single name; here the information is still present. See the same-named finding.
"""
from shelfmark.release_sources import BrowseRecord, browse_record_to_book_metadata
record = BrowseRecord(id="abc", title="Blindness", source="direct_download")
book = browse_record_to_book_metadata(record, author_override=JOINED)
assert book.search_author == "José Saramago"
assert book.authors == [a.strip() for a in JOINED.split(",")]
@@ -0,0 +1,193 @@
"""One search fetches each distinct AA URL once, however many passes ask for it.
`DirectDownloadSource.search` fans out: a title variant per grouped variant, then -
when nothing was found - the whole set again without the language filter. With
DIRECT_DOWNLOAD_LANGUAGE_FROM_PATH on, the requested language is applied locally
rather than as `&lang=`, so both passes build a byte-identical URL and the retry
re-fetches a page it already had. Behind DDoS-Guard that repeat is a fresh browser
solve, tens of seconds for nothing. See issue #1285.
"""
import pytest
from bs4 import BeautifulSoup
import shelfmark.release_sources.direct_download as dd
from shelfmark.core import search_deadline
@pytest.fixture(autouse=True)
def _no_ambient_deadline():
token = search_deadline._current.set(None)
yield
search_deadline._current.reset(token)
class _Selector:
current_base = "https://annas-archive.gl"
last_failure = None
def rewrite(self, url: str) -> str:
return url
def next_mirror_or_rotate_dns(self, *, fatal: bool = False, reason: str = ""):
return None, "exhausted"
_PAGE = "<html><body><main><table><tbody></tbody></table></main></body></html>"
def _count_fetches(monkeypatch) -> list[str]:
fetched: list[str] = []
monkeypatch.setattr(
dd.downloader, "html_get_page", lambda url, **_k: fetched.append(url) or _PAGE
)
monkeypatch.setattr(dd.network, "get_available_aa_urls", lambda: ["https://annas-archive.gl"])
return fetched
def test_repeated_url_is_fetched_once_within_one_search(monkeypatch):
fetched = _count_fetches(monkeypatch)
url = "https://annas-archive.gl/search?q=dune"
with dd._search_page_reuse():
first_html, first_table = dd._fetch_search_table(url, _Selector())
second_html, second_table = dd._fetch_search_table(url, _Selector())
assert fetched == [url], "the second ask should have been served from the search's cache"
assert first_html == second_html
assert second_table is first_table
def test_distinct_urls_are_still_fetched_separately(monkeypatch):
fetched = _count_fetches(monkeypatch)
with dd._search_page_reuse():
dd._fetch_search_table("https://annas-archive.gl/search?q=dune", _Selector())
dd._fetch_search_table("https://annas-archive.gl/search?q=dune&lang=en", _Selector())
assert len(fetched) == 2
def test_cache_does_not_leak_between_searches(monkeypatch):
"""A later request must not be answered from an earlier request's pages."""
fetched = _count_fetches(monkeypatch)
url = "https://annas-archive.gl/search?q=dune"
with dd._search_page_reuse():
dd._fetch_search_table(url, _Selector())
with dd._search_page_reuse():
dd._fetch_search_table(url, _Selector())
assert fetched == [url, url]
def test_without_the_context_every_fetch_still_goes_out(monkeypatch):
"""Callers outside a search - `get_book_info`, downloads - are unaffected."""
fetched = _count_fetches(monkeypatch)
url = "https://annas-archive.gl/search?q=dune"
dd._fetch_search_table(url, _Selector())
dd._fetch_search_table(url, _Selector())
assert fetched == [url, url]
def test_a_failure_is_not_cached(monkeypatch):
"""A spent budget raises; the next search must not inherit that as a stored answer."""
fetched = _count_fetches(monkeypatch)
url = "https://annas-archive.gl/search?q=dune"
with dd._search_page_reuse():
with search_deadline.search_deadline(60) as deadline:
deadline.event.set()
with pytest.raises(dd.SearchUnavailableError):
dd._fetch_search_table(url, _Selector())
dd._fetch_search_table(url, _Selector())
assert fetched == [url], "the successful retry should be the only fetch"
def test_a_give_up_page_is_not_cached(monkeypatch):
"""Exhausting the mirrors is not an answer, and the retry must get a fresh attempt.
`_fetch_search_table_uncached` exists to rotate past domains that are not AA, and when
it runs out it *returns* rather than raising: a page with no results table and no
marker. Storing that would hand the language-filter retry - the pass this cache exists
for - a mirror set that may have recovered since, turning a transient outage into
"this book has no releases".
"""
parked = "<html><body>This domain is for sale.</body></html>"
fetched: list[str] = []
monkeypatch.setattr(
dd.downloader, "html_get_page", lambda url, **_k: fetched.append(url) or parked
)
monkeypatch.setattr(dd.network, "get_available_aa_urls", lambda: ["https://annas-archive.gl"])
url = "https://annas-archive.gl/search?q=dune"
with dd._search_page_reuse():
assert dd._fetch_search_table(url, _Selector()) == (parked, None)
assert dd._fetch_search_table(url, _Selector()) == (parked, None)
assert fetched == [url, url], "the second pass must not inherit the first's give-up"
def test_a_genuinely_empty_result_is_still_cached(monkeypatch):
"""A page saying "No files found." is a real answer from a healthy mirror."""
empty = "<html><body><main>No files found. <a href='/md5/x'>x</a></main></body></html>"
fetched: list[str] = []
monkeypatch.setattr(
dd.downloader, "html_get_page", lambda url, **_k: fetched.append(url) or empty
)
monkeypatch.setattr(dd.network, "get_available_aa_urls", lambda: ["https://annas-archive.gl"])
url = "https://annas-archive.gl/search?q=nothing"
with dd._search_page_reuse():
assert dd._fetch_search_table(url, _Selector()) == (empty, None)
assert dd._fetch_search_table(url, _Selector()) == (empty, None)
assert fetched == [url], "an empty answer is an answer; re-solving for it buys nothing"
def test_language_retry_reuses_the_page_it_already_fetched(monkeypatch):
"""The end-to-end shape: language-from-path makes both passes build the same URL."""
fetched = _count_fetches(monkeypatch)
original_get = dd.config.get
def _fake_get(key: str, default=None, user_id=None):
del user_id
if key == "DIRECT_DOWNLOAD_LANGUAGE_FROM_PATH":
return True
return original_get(key, default)
monkeypatch.setattr(dd.config, "get", _fake_get)
monkeypatch.setattr(dd.network, "get_aa_base_url", lambda: "https://annas-archive.gl")
filters_with_lang = dd.SearchFilters(lang=["en"])
filters_without = dd.SearchFilters()
with dd._search_page_reuse():
dd.search_books("dune", filters_with_lang)
dd.search_books("dune", filters_without)
assert len(fetched) == 1, f"both passes build the same URL, got {fetched}"
assert "lang=" not in fetched[0]
def test_soup_reuse_is_safe_for_repeated_parsing(monkeypatch):
"""The cached Tag is read repeatedly, so reuse must not consume it."""
page = (
"<html><body><main><table><tbody><tr><td>row</td></tr></tbody></table></main></body></html>"
)
monkeypatch.setattr(dd.downloader, "html_get_page", lambda _url, **_k: page)
monkeypatch.setattr(dd.network, "get_available_aa_urls", lambda: ["https://annas-archive.gl"])
with dd._search_page_reuse():
_, first = dd._fetch_search_table("https://annas-archive.gl/search?q=dune", _Selector())
_, second = dd._fetch_search_table("https://annas-archive.gl/search?q=dune", _Selector())
assert first is not None
assert second is not None
assert len(first.find_all("tr")) == 1
assert len(second.find_all("tr")) == 1
assert isinstance(BeautifulSoup(str(second), "html.parser"), BeautifulSoup)
+2 -2
View File
@@ -97,7 +97,7 @@ end-to-end (`docker compose up` + suite + teardown) and passes.
> **`full` profile exercises the real end-to-end
> CF solve**: AA search/detail are reachable, but the AA slow-download link points
> at the gate, so downloading Moby-Dick forces the in-image headless Chromium to
> detect the challenge, solve it (`_bypass_method_cdp_solve`), and fetch the file —
> detect the challenge, solve it (`_bypass_method_cdp_gui_click`), and fetch the file —
> verified live (`Challenge detected: cloudflare` → `Bypass successful` → Moby-Dick
> in `/books`).
>
@@ -151,7 +151,7 @@ demand (excluded from the PR matrix). It spins up, with **no** mock bypasser, an
gate (`mock-cf`), whose challenge page runs JS that issues `cf_clearance` and
reloads. Downloading Moby-Dick forces the in-image headless Chromium (seleniumbase
CDP, in the `shelfmark` image via `xvfb`+`chromium`) to load the gate, detect the
challenge (`Challenge detected: cloudflare`), solve it (`_bypass_method_cdp_solve`),
challenge (`Challenge detected: cloudflare`), solve it (`_bypass_method_cdp_gui_click`),
and fetch the cleared "Download now" page → the file lands in `/books`. That
outcome is *only* reachable if Chrome solved the gate — the literal "spin a Chrome
browser" path and the strongest guard for the #1 cluster. Two subtleties this