From fd8fbb178c58826a14dafc7a7dc1d78fa902dde0 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Fri, 3 Jul 2026 03:24:34 +0900 Subject: [PATCH] fix(reader): apply page margin changes live on all platforms (#4898) (#4900) Adjusting the top, bottom, left, or right page margin had no visible effect until an unrelated setting (e.g. Show Header) was toggled. The BooksGrid perf refactor (#4562) memoized the derived view/content insets on the ViewSettings object identity. saveViewSettings mutates ViewSettings in place (same reference), so the memo never recomputed on a margin edit and the new margin never reached the paginator. Left and right margins were always stale; top and bottom only refreshed when the header/footer visibility (an effect dependency) changed, which is why toggling the header appeared to apply a pending change. Extract the inset derivation into useContentInsets and memoize by the resolved numeric values instead of the object reference: identical numbers across a page turn keep a stable reference (no re-render storm), while a changed margin yields a new one that propagates to the renderer. Co-authored-by: Claude Opus 4.8 (1M context) --- .../app/reader/hooks/useContentInsets.test.ts | 90 +++++++++++++++++++ .../src/app/reader/components/BooksGrid.tsx | 33 ++----- .../src/app/reader/hooks/useContentInsets.ts | 48 ++++++++++ 3 files changed, 143 insertions(+), 28 deletions(-) create mode 100644 apps/readest-app/src/__tests__/app/reader/hooks/useContentInsets.test.ts create mode 100644 apps/readest-app/src/app/reader/hooks/useContentInsets.ts diff --git a/apps/readest-app/src/__tests__/app/reader/hooks/useContentInsets.test.ts b/apps/readest-app/src/__tests__/app/reader/hooks/useContentInsets.test.ts new file mode 100644 index 00000000..b697a0fc --- /dev/null +++ b/apps/readest-app/src/__tests__/app/reader/hooks/useContentInsets.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from 'vitest'; +import { renderHook } from '@testing-library/react'; +import { useContentInsets } from '@/app/reader/hooks/useContentInsets'; +import { ViewSettings } from '@/types/book'; + +const ZERO: { top: number; right: number; bottom: number; left: number } = { + top: 0, + right: 0, + bottom: 0, + left: 0, +}; + +const makeViewSettings = (overrides: Partial = {}): ViewSettings => + ({ + showHeader: true, + showFooter: true, + vertical: false, + writingMode: 'horizontal-tb', + marginTopPx: 44, + marginBottomPx: 44, + marginLeftPx: 16, + marginRightPx: 16, + compactMarginTopPx: 16, + compactMarginBottomPx: 16, + compactMarginLeftPx: 16, + compactMarginRightPx: 16, + ...overrides, + }) as ViewSettings; + +describe('useContentInsets', () => { + // Regression for #4898: saveViewSettings mutates the ViewSettings object in + // place (same reference), so left/right margin edits never reached the + // paginator because the derived insets were memoized on the object identity. + it('reflects an in-place left/right margin edit on the same viewSettings object', () => { + const viewSettings = makeViewSettings(); + const { result, rerender } = renderHook(({ vs, gi }) => useContentInsets(vs, gi), { + initialProps: { vs: viewSettings, gi: ZERO }, + }); + + expect(result.current.contentInsets.left).toBe(16); + expect(result.current.contentInsets.right).toBe(16); + + viewSettings.compactMarginLeftPx = 40; + viewSettings.compactMarginRightPx = 32; + rerender({ vs: viewSettings, gi: ZERO }); + + expect(result.current.contentInsets.left).toBe(40); + expect(result.current.contentInsets.right).toBe(32); + }); + + it('reflects an in-place top/bottom margin edit when header/footer are shown', () => { + const viewSettings = makeViewSettings(); + const { result, rerender } = renderHook(({ vs, gi }) => useContentInsets(vs, gi), { + initialProps: { vs: viewSettings, gi: ZERO }, + }); + + expect(result.current.contentInsets.top).toBe(44); + expect(result.current.contentInsets.bottom).toBe(44); + + viewSettings.marginTopPx = 60; + viewSettings.marginBottomPx = 52; + rerender({ vs: viewSettings, gi: ZERO }); + + expect(result.current.contentInsets.top).toBe(60); + expect(result.current.contentInsets.bottom).toBe(52); + }); + + it('adds the grid insets to the page margins', () => { + const viewSettings = makeViewSettings(); + const { result } = renderHook(() => + useContentInsets(viewSettings, { top: 10, right: 4, bottom: 8, left: 6 }), + ); + + expect(result.current.contentInsets.top).toBe(54); + expect(result.current.contentInsets.left).toBe(22); + }); + + it('keeps a stable contentInsets reference across a page turn (unchanged margins)', () => { + const viewSettings = makeViewSettings(); + const { result, rerender } = renderHook(({ vs, gi }) => useContentInsets(vs, gi), { + initialProps: { vs: viewSettings, gi: ZERO }, + }); + + const first = result.current.contentInsets; + // A page turn re-renders the cell without touching any margin setting. + rerender({ vs: viewSettings, gi: ZERO }); + + expect(result.current.contentInsets).toBe(first); + }); +}); diff --git a/apps/readest-app/src/app/reader/components/BooksGrid.tsx b/apps/readest-app/src/app/reader/components/BooksGrid.tsx index 8313e832..d2f40fa8 100644 --- a/apps/readest-app/src/app/reader/components/BooksGrid.tsx +++ b/apps/readest-app/src/app/reader/components/BooksGrid.tsx @@ -10,7 +10,7 @@ import { useSidebarStore } from '@/store/sidebarStore'; import { useBookDataStore } from '@/store/bookDataStore'; import { useTranslation } from '@/hooks/useTranslation'; import { getGridTemplate, getInsetEdges } from '@/utils/grid'; -import { getViewInsets } from '@/utils/insets'; +import { useContentInsets } from '../hooks/useContentInsets'; import SearchResultsNav from './sidebar/SearchResultsNav'; import BooknotesNav from './sidebar/BooknotesNav'; import FoliateViewer from './FoliateViewer'; @@ -118,33 +118,10 @@ const BookCellInner: React.FC = ({ const config = getConfig(bookKey); const { book, bookDoc } = bookData || {}; - // viewSettings drives both viewInsets and the inset-derived geometry. - // Memoize off its identity so contentInsets stays stable while the - // user is just turning pages — viewSettings only changes when the - // user toggles settings, not on every relocate. Same logic for - // gridInsets which is keyed off the resolved Insets numbers. - const viewInsets = useMemo( - () => (viewSettings ? getViewInsets(viewSettings) : { top: 0, right: 0, bottom: 0, left: 0 }), - [viewSettings], - ); - const contentInsets = useMemo( - () => ({ - top: gridInsets.top + viewInsets.top, - right: gridInsets.right + viewInsets.right, - bottom: gridInsets.bottom + viewInsets.bottom, - left: gridInsets.left + viewInsets.left, - }), - [ - gridInsets.top, - gridInsets.right, - gridInsets.bottom, - gridInsets.left, - viewInsets.top, - viewInsets.right, - viewInsets.bottom, - viewInsets.left, - ], - ); + // viewInsets/contentInsets stay stable while the user is just turning pages + // (margins are unchanged) but update when a margin setting changes — even + // though saveViewSettings mutates viewSettings in place (#4898). + const { viewInsets, contentInsets } = useContentInsets(viewSettings, gridInsets); // Stable callback so HeaderBar doesn't see a new prop reference per // BooksGrid render. diff --git a/apps/readest-app/src/app/reader/hooks/useContentInsets.ts b/apps/readest-app/src/app/reader/hooks/useContentInsets.ts new file mode 100644 index 00000000..8f0b2189 --- /dev/null +++ b/apps/readest-app/src/app/reader/hooks/useContentInsets.ts @@ -0,0 +1,48 @@ +import { useMemo } from 'react'; +import { Insets } from '@/types/misc'; +import { ViewSettings } from '@/types/book'; +import { getViewInsets } from '@/utils/insets'; + +const ZERO_INSETS: Insets = { top: 0, right: 0, bottom: 0, left: 0 }; + +/** + * Resolve the reader's view insets (page margins) and content insets + * (grid insets + page margins) for a book cell. + * + * `saveViewSettings` mutates the `ViewSettings` object in place (keeping the + * same reference), so memoizing on the object identity would freeze the page + * margins: a margin edit changes a field but not the reference, so the memo + * never recomputes and the new margin never reaches the paginator (#4898). + * + * `getViewInsets` is cheap, so resolve it every render and memoize by the + * resolved numeric values instead: identical numbers across a page turn keep + * the same object reference (children bail out of re-rendering), while a + * changed margin yields a new reference that propagates to the paginator. + */ +export const useContentInsets = (viewSettings: ViewSettings | null, gridInsets: Insets) => { + const resolved = viewSettings ? getViewInsets(viewSettings) : ZERO_INSETS; + const viewInsets = useMemo( + () => resolved, + // eslint-disable-next-line react-hooks/exhaustive-deps + [resolved.top, resolved.right, resolved.bottom, resolved.left], + ); + const contentInsets = useMemo( + () => ({ + top: gridInsets.top + viewInsets.top, + right: gridInsets.right + viewInsets.right, + bottom: gridInsets.bottom + viewInsets.bottom, + left: gridInsets.left + viewInsets.left, + }), + [ + gridInsets.top, + gridInsets.right, + gridInsets.bottom, + gridInsets.left, + viewInsets.top, + viewInsets.right, + viewInsets.bottom, + viewInsets.left, + ], + ); + return { viewInsets, contentInsets }; +};