diff --git a/shelfmark/bypass/internal_bypasser.py b/shelfmark/bypass/internal_bypasser.py index 0daba9d5..8d31dd3a 100644 --- a/shelfmark/bypass/internal_bypasser.py +++ b/shelfmark/bypass/internal_bypasser.py @@ -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, diff --git a/shelfmark/core/search_plan.py b/shelfmark/core/search_plan.py index 03406906..3153052e 100644 --- a/shelfmark/core/search_plan.py +++ b/shelfmark/core/search_plan.py @@ -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: diff --git a/shelfmark/main.py b/shelfmark/main.py index b7c54ea4..9638390a 100644 --- a/shelfmark/main.py +++ b/shelfmark/main.py @@ -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: diff --git a/shelfmark/release_sources/__init__.py b/shelfmark/release_sources/__init__.py index deb69e40..1b0d69fa 100644 --- a/shelfmark/release_sources/__init__.py +++ b/shelfmark/release_sources/__init__.py @@ -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, diff --git a/shelfmark/release_sources/direct_download.py b/shelfmark/release_sources/direct_download.py index 83836f7f..0a8687ae 100644 --- a/shelfmark/release_sources/direct_download.py +++ b/shelfmark/release_sources/direct_download.py @@ -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 # - 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. diff --git a/shelfmark/release_sources/irc/source.py b/shelfmark/release_sources/irc/source.py index c5057eba..9e9b51f6 100644 --- a/shelfmark/release_sources/irc/source.py +++ b/shelfmark/release_sources/irc/source.py @@ -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) diff --git a/src/frontend/.oxlintrc.json b/src/frontend/.oxlintrc.json index 68ed4694..818ea6bb 100644 --- a/src/frontend/.oxlintrc.json +++ b/src/frontend/.oxlintrc.json @@ -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", diff --git a/src/frontend/src/App.tsx b/src/frontend/src/App.tsx index 02fc83fa..17c4c729 100644 --- a/src/frontend/src/App.tsx +++ b/src/frontend/src/App.tsx @@ -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(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 => { @@ -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( + () => activeMetadataConfig?.search_fields ?? [], + ); + const incomingSearchFields = activeMetadataConfig?.search_fields; + if (incomingSearchFields && incomingSearchFields !== stableSearchFields) { + setStableSearchFields(incomingSearchFields); } - const stableSearchFields = lastKnownSearchFields.current; const queryTargets = useMemo( () => @@ -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(() => { diff --git a/src/frontend/src/components/ReleaseModal.tsx b/src/frontend/src/components/ReleaseModal.tsx index c99e5cb2..89b17979 100644 --- a/src/frontend/src/components/ReleaseModal.tsx +++ b/src/frontend/src/components/ReleaseModal.tsx @@ -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>({}); @@ -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(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; diff --git a/src/frontend/src/components/SearchBar.tsx b/src/frontend/src/components/SearchBar.tsx index 528d299b..70d901fc 100644 --- a/src/frontend/src/components/SearchBar.tsx +++ b/src/frontend/src/components/SearchBar.tsx @@ -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( const { searchMode } = useSearchMode(); const inputRef = useRef(null); const buttonRef = useRef(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(null); const hasSearchQuery = hasActiveValue(value); const [isSelectorOpen, setIsSelectorOpen] = useState(false); @@ -730,7 +732,7 @@ export const SearchBar = forwardRef( 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( 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)' }} diff --git a/src/frontend/src/components/activity/ActivityCard.tsx b/src/frontend/src/components/activity/ActivityCard.tsx index 7469c845..771b8312 100644 --- a/src/frontend/src/components/activity/ActivityCard.tsx +++ b/src/frontend/src/components/activity/ActivityCard.tsx @@ -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; diff --git a/src/frontend/src/components/settings/SettingsContent.tsx b/src/frontend/src/components/settings/SettingsContent.tsx index fe6d6989..748f1255 100644 --- a/src/frontend/src/components/settings/SettingsContent.tsx +++ b/src/frontend/src/components/settings/SettingsContent.tsx @@ -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(() => { diff --git a/src/frontend/src/components/settings/customFields/UsersManagementField.tsx b/src/frontend/src/components/settings/customFields/UsersManagementField.tsx index 584ee3e0..81874872 100644 --- a/src/frontend/src/components/settings/customFields/UsersManagementField.tsx +++ b/src/frontend/src/components/settings/customFields/UsersManagementField.tsx @@ -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) { diff --git a/src/frontend/src/components/settings/fields/MultiSelectField.tsx b/src/frontend/src/components/settings/fields/MultiSelectField.tsx index 82a33628..099ba539 100644 --- a/src/frontend/src/components/settings/fields/MultiSelectField.tsx +++ b/src/frontend/src/components/settings/fields/MultiSelectField.tsx @@ -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(); + 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(); - 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(); + 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(); - 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 {field.placeholder || 'Select categories...'}; - } - 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 ( -
- {summaryFormatter()} -
- ); } + // 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 {field.placeholder || 'Select categories...'}; + } + 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 ( - +
+ {summaryFormatter()} +
); } + + return ( + + ); +}; + +// 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 = ({ ); }; + +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 ( + + ); + } + + return ( + + ); +}; diff --git a/src/frontend/src/components/shared/Tooltip.tsx b/src/frontend/src/components/shared/Tooltip.tsx index 87c4772c..f13b277e 100644 --- a/src/frontend/src/components/shared/Tooltip.tsx +++ b/src/frontend/src/components/shared/Tooltip.tsx @@ -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; diff --git a/src/frontend/src/hooks/app/useBookTargetDeselectSync.ts b/src/frontend/src/hooks/app/useBookTargetDeselectSync.ts index c0a48cbe..715bce45 100644 --- a/src/frontend/src/hooks/app/useBookTargetDeselectSync.ts +++ b/src/frontend/src/hooks/app/useBookTargetDeselectSync.ts @@ -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))); }; diff --git a/src/frontend/src/hooks/app/useContentTypePreferences.ts b/src/frontend/src/hooks/app/useContentTypePreferences.ts index 97965391..4da4f08a 100644 --- a/src/frontend/src/hooks/app/useContentTypePreferences.ts +++ b/src/frontend/src/hooks/app/useContentTypePreferences.ts @@ -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>; } => { - const initialPreference = readInitialPreference(); - const [contentType, setContentTypeState] = useState( - () => initialPreference.contentType, - ); - const [combinedMode, setCombinedModeState] = useState( - () => 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(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> = useCallback((value) => { + setPreference((current) => ({ + ...current, + contentType: typeof value === 'function' ? value(current.contentType) : value, + })); + }, []); - const setContentType: Dispatch> = useCallback( - (value) => { - setContentTypeState((current) => { - const nextContentType = typeof value === 'function' ? value(current) : value; - persistPreference(nextContentType, combinedModeRef.current); - return nextContentType; - }); - }, - [persistPreference], - ); + const setCombinedMode: Dispatch> = useCallback((value) => { + setPreference((current) => ({ + ...current, + combinedMode: typeof value === 'function' ? value(current.combinedMode) : value, + })); + }, []); - const setCombinedMode: Dispatch> = 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, diff --git a/src/frontend/src/hooks/releaseModal/useDescriptionOverflow.ts b/src/frontend/src/hooks/releaseModal/useDescriptionOverflow.ts index 9a5d33d7..647e45cf 100644 --- a/src/frontend/src/hooks/releaseModal/useDescriptionOverflow.ts +++ b/src/frontend/src/hooks/releaseModal/useDescriptionOverflow.ts @@ -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; diff --git a/src/frontend/src/hooks/releaseModal/useReleaseSearchSession.ts b/src/frontend/src/hooks/releaseModal/useReleaseSearchSession.ts index d12bbc2b..f133ad70 100644 --- a/src/frontend/src/hooks/releaseModal/useReleaseSearchSession.ts +++ b/src/frontend/src/hooks/releaseModal/useReleaseSearchSession.ts @@ -164,7 +164,6 @@ export function useReleaseSearchSession( const lastStatusTimeRef = useRef(0); const pendingStatusRef = useRef(null); const statusTimeoutRef = useRef(null); - activeTabRef.current = activeTab; const allTabs = useMemo(() => { return buildReleaseTabs( @@ -363,7 +362,6 @@ export function useReleaseSearchSession( indexerFilterInitializedRef.current = new Set(); 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) { diff --git a/src/frontend/src/hooks/ui/useTabIndicator.ts b/src/frontend/src/hooks/ui/useTabIndicator.ts index 73eb0434..1632ded9 100644 --- a/src/frontend/src/hooks/ui/useTabIndicator.ts +++ b/src/frontend/src/hooks/ui/useTabIndicator.ts @@ -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>, activeTab: string, tabsDependency: unknown, ): TabIndicatorStyle { - const [tabIndicatorStyle, setTabIndicatorStyle] = useState({ - left: 0, - width: 0, - }); + const [tabIndicatorStyle, setTabIndicatorStyle] = useState(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; diff --git a/src/frontend/src/hooks/useDismiss.ts b/src/frontend/src/hooks/useDismiss.ts index 67a3b17d..067a4bda 100644 --- a/src/frontend/src/hooks/useDismiss.ts +++ b/src/frontend/src/hooks/useDismiss.ts @@ -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[], 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]); }; diff --git a/src/frontend/src/hooks/useLatestCallback.ts b/src/frontend/src/hooks/useLatestCallback.ts new file mode 100644 index 00000000..a4e32c14 --- /dev/null +++ b/src/frontend/src/hooks/useLatestCallback.ts @@ -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( + callback: (...args: Args) => Result, +): (...args: Args) => Result { + const callbackRef = useRef(callback); + + useLayoutEffect(() => { + callbackRef.current = callback; + }); + + return useCallback((...args: Args) => callbackRef.current(...args), []); +} diff --git a/src/frontend/src/hooks/useMountEffect.ts b/src/frontend/src/hooks/useMountEffect.ts index c15b34f5..625ae726 100644 --- a/src/frontend/src/hooks/useMountEffect.ts +++ b/src/frontend/src/hooks/useMountEffect.ts @@ -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); } diff --git a/src/frontend/src/hooks/useSearch.ts b/src/frontend/src/hooks/useSearch.ts index 537531f3..62e68ab9 100644 --- a/src/frontend/src/hooks/useSearch.ts +++ b/src/frontend/src/hooks/useSearch.ts @@ -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); diff --git a/src/frontend/src/hooks/useSettings.ts b/src/frontend/src/hooks/useSettings.ts index f7c96c8c..4717e750 100644 --- a/src/frontend/src/hooks/useSettings.ts +++ b/src/frontend/src/hooks/useSettings.ts @@ -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({}); - const originalValuesRef = useRef({}); - - 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( diff --git a/src/frontend/src/services/api.ts b/src/frontend/src/services/api.ts index 2537da8a..520a7195 100644 --- a/src/frontend/src/services/api.ts +++ b/src/frontend/src/services/api.ts @@ -84,6 +84,9 @@ type ApiResponseErrorShape = Error & { code?: string; requiredMode?: string; payload?: Record; + // 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; + serverMessage?: string; constructor( message: string, @@ -99,6 +103,7 @@ class ApiResponseError extends Error { code?: string; requiredMode?: string; payload?: Record; + 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( @@ -183,12 +220,15 @@ async function fetchJSON( 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( 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 => { const response = await fetchJSON( `${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 => { }; export const getConfig = async (): Promise => { - return fetchJSON(API.config); + const config = await fetchJSON(API.config); + setSearchTimeoutFromConfig(config.release_search_timeout); + return config; }; interface ActivityDismissedItem { diff --git a/src/frontend/src/tests/releaseModalEnterAnimation.test.ts b/src/frontend/src/tests/releaseModalEnterAnimation.test.ts new file mode 100644 index 00000000..0f65f843 --- /dev/null +++ b/src/frontend/src/tests/releaseModalEnterAnimation.test.ts @@ -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); + }); +}); diff --git a/src/frontend/src/tests/searchFailureMessage.test.ts b/src/frontend/src/tests/searchFailureMessage.test.ts new file mode 100644 index 00000000..8e54f9c9 --- /dev/null +++ b/src/frontend/src/tests/searchFailureMessage.test.ts @@ -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): Promise => { + 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); + }); +}); diff --git a/src/frontend/src/tests/searchTimeout.test.ts b/src/frontend/src/tests/searchTimeout.test.ts new file mode 100644 index 00000000..017ea8bb --- /dev/null +++ b/src/frontend/src/tests/searchTimeout.test.ts @@ -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 => ({ + 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 = []; + 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'); + } + }); +}); diff --git a/src/frontend/src/types/index.ts b/src/frontend/src/types/index.ts index 6195afa1..9ee4ea8e 100644 --- a/src/frontend/src/types/index.ts +++ b/src/frontend/src/types/index.ts @@ -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 diff --git a/src/frontend/src/utils/bookTargetEvents.ts b/src/frontend/src/utils/bookTargetEvents.ts index ddc19aec..e3d985d3 100644 --- a/src/frontend/src/utils/bookTargetEvents.ts +++ b/src/frontend/src/utils/bookTargetEvents.ts @@ -1,4 +1,4 @@ -type BookTargetChangeEvent = { +export type BookTargetChangeEvent = { provider: string; bookId: string; target: string; diff --git a/src/frontend/src/utils/releaseModalEnterAnimation.ts b/src/frontend/src/utils/releaseModalEnterAnimation.ts new file mode 100644 index 00000000..c0b350d1 --- /dev/null +++ b/src/frontend/src/utils/releaseModalEnterAnimation.ts @@ -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, + }; +}; diff --git a/src/frontend/src/utils/searchFailureMessage.ts b/src/frontend/src/utils/searchFailureMessage.ts new file mode 100644 index 00000000..a7a4b2f5 --- /dev/null +++ b/src/frontend/src/utils/searchFailureMessage.ts @@ -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; +}; diff --git a/tests/bypass/test_bypass_method_order.py b/tests/bypass/test_bypass_method_order.py new file mode 100644 index 00000000..c29c28fc --- /dev/null +++ b/tests/bypass/test_bypass_method_order.py @@ -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" + ) diff --git a/tests/core/test_releases_api_budget.py b/tests/core/test_releases_api_budget.py index 79d90ce1..526f539c 100644 --- a/tests/core/test_releases_api_budget.py +++ b/tests/core/test_releases_api_budget.py @@ -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 diff --git a/tests/core/test_search_author_narrowing.py b/tests/core/test_search_author_narrowing.py new file mode 100644 index 00000000..d1326bdc --- /dev/null +++ b/tests/core/test_search_author_narrowing.py @@ -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, + +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(",")] diff --git a/tests/direct_download/test_search_page_reuse.py b/tests/direct_download/test_search_page_reuse.py new file mode 100644 index 00000000..8ffe3ae2 --- /dev/null +++ b/tests/direct_download/test_search_page_reuse.py @@ -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 = "
" + + +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 = "This domain is for sale." + 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 = "
No files found. x
" + 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 = ( + "
row
" + ) + 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) diff --git a/tests/e2e/platform/README.md b/tests/e2e/platform/README.md index 010ffdb5..aef08913 100644 --- a/tests/e2e/platform/README.md +++ b/tests/e2e/platform/README.md @@ -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