fix(book-detail): stop the reading log and highlights tabs flashing

Both tabs fetch on mount and were rebuilt on every switch, so each visit replayed blank, skeleton and content inside 120ms. Keep them alive and revalidate silently instead, hold the layout until the first load decides which shape it settles into, and defer skeletons past 220ms. Also fixes the unhandled rejection failing the server suite in CI: the Goodreads bot-challenge test rejected before its assertion was attached.
2026-11-03 00:11:37 -06:00 · 2026-08-26 19:53:36 +00:00
parent 87a3217c5e
commit 8b9442be1f
9 changed files with 241 additions and 51 deletions
@@ -0,0 +1,64 @@
+import { onScopeDispose, ref, watch, type Ref } from 'vue'
+
+const SHOW_AFTER_MS = 220
+const MIN_VISIBLE_MS = 320
+
+/**
+ * Gates a loading placeholder on how long the load actually takes.
+ *
+ * A skeleton that paints and clears inside a couple of frames is not feedback, it is a flash: the
+ * book detail tabs answer in well under 100ms locally, so binding shimmer straight to `loading`
+ * strobes the pane on every visit. This stays down until the wait is long enough to be worth
+ * announcing, and once raised it holds long enough to be read rather than blinking off again.
+ */
+export function useDeferredLoading(loading: Ref<boolean>, options?: { showAfterMs?: number; minVisibleMs?: number }): Ref<boolean> {
+  const showAfterMs = options?.showAfterMs ?? SHOW_AFTER_MS
+  const minVisibleMs = options?.minVisibleMs ?? MIN_VISIBLE_MS
+
+  const visible = ref(false)
+  let showTimer: ReturnType<typeof setTimeout> | null = null
+  let hideTimer: ReturnType<typeof setTimeout> | null = null
+  let shownAt = 0
+
+  function clearTimers() {
+    if (showTimer !== null) {
+      clearTimeout(showTimer)
+      showTimer = null
+    }
+    if (hideTimer !== null) {
+      clearTimeout(hideTimer)
+      hideTimer = null
+    }
+  }
+
+  watch(
+    loading,
+    (busy) => {
+      clearTimers()
+      if (busy) {
+        if (visible.value) return
+        showTimer = setTimeout(() => {
+          showTimer = null
+          shownAt = performance.now()
+          visible.value = true
+        }, showAfterMs)
+        return
+      }
+      if (!visible.value) return
+      const remaining = minVisibleMs - (performance.now() - shownAt)
+      if (remaining <= 0) {
+        visible.value = false
+        return
+      }
+      hideTimer = setTimeout(() => {
+        hideTimer = null
+        visible.value = false
+      }, remaining)
+    },
+    { immediate: true },
+  )
+
+  onScopeDispose(clearTimers)
+
+  return visible
+}
@@ -273,24 +273,38 @@ const formatLine = computed(() =>

 const currentStatusOption = computed(() => STATUS_OPTIONS.find((option) => option.value === (localReadStatus.value ?? 'unread')))

+// A book with no sessions still answers with a stats object of zeros, so a null one means the
+// first load has not landed. Printing its zeros claims the book is unread until the real count
+// arrives; the dash the pace and last-read rows already use says "not known yet" instead.
+const statsPending = computed(() => props.stats === null)
+
 const ledgerRows = computed(() => [
  {
    key: 'total',
    label: t('book.detail.readingLog.vitals.totalTime'),
-    value: formatDuration(props.stats?.totalSeconds ?? 0),
+    value: statsPending.value ? '-' : formatDuration(props.stats?.totalSeconds ?? 0),
    withMomentum: true,
  },
-  { key: 'sessions', label: t('book.detail.readingLog.vitals.sessions'), value: formatNumber(props.stats?.totalSessions ?? 0), withMomentum: false },
+  {
+    key: 'sessions',
+    label: t('book.detail.readingLog.vitals.sessions'),
+    value: statsPending.value ? '-' : formatNumber(props.stats?.totalSessions ?? 0),
+    withMomentum: false,
+  },
  {
    key: 'average',
    label: t('book.detail.readingLog.vitals.avgSession'),
-    value: formatDuration(props.stats?.avgDurationSeconds ?? 0),
+    value: statsPending.value ? '-' : formatDuration(props.stats?.avgDurationSeconds ?? 0),
    withMomentum: false,
  },
  {
    key: 'activeDays',
    label: t('book.detail.readingLog.vitals.activeDays'),
-    value: spanDays.value > 0 ? t('book.detail.readingLog.vitals.activeDaysOf', { active: activeDays.value, total: spanDays.value }) : '0',
+    value: statsPending.value
+      ? '-'
+      : spanDays.value > 0
+        ? t('book.detail.readingLog.vitals.activeDaysOf', { active: activeDays.value, total: spanDays.value })
+        : '0',
    withMomentum: false,
  },
  {
@@ -1,11 +1,12 @@
 <script setup lang="ts">
-import { computed, onBeforeUnmount, onMounted, ref, watch } from 'vue'
+import { computed, onActivated, onBeforeUnmount, onDeactivated, onMounted, ref, watch } from 'vue'
 import { useMediaQuery } from '@vueuse/core'
 import { useRouter } from 'vue-router'
 import { useI18n } from 'vue-i18n'
 import type { AnnotationItem, BookDetail } from '@bookorbit/types'
 import { READER_OPENABLE_FORMATS } from '@bookorbit/types'
 import { useBookHighlights } from '@/features/book/composables/useBookHighlights'
+import { useDeferredLoading } from '@/composables/useDeferredLoading'
 import { useDensity } from '@/features/annotations/composables/useDensity'
 import { HIGHLIGHT_GROUP_MODES, NO_CHAPTER_KEY, type HighlightGroup } from '@/features/book/lib/highlight-groups'
 import HighlightsRail from '@/features/book/components/detail/highlights/HighlightsRail.vue'
@@ -17,6 +18,9 @@ import HighlightsBand from '@/features/book/components/detail/highlights/Highlig
 import HighlightsEmptyStage from '@/features/book/components/detail/highlights/HighlightsEmptyStage.vue'
 import HighlightsShortcutsDialog from '@/features/book/components/detail/highlights/HighlightsShortcutsDialog.vue'

+// Named for the KeepAlive `include` list in BookDetailView.
+defineOptions({ name: 'HighlightsTab' })
+
 const props = defineProps<{ book: BookDetail }>()

 const { t } = useI18n()
@@ -33,6 +37,17 @@ const editingNoteId = ref<number | null>(null)

 const bookTitle = computed(() => props.book.title ?? t('book.detail.highlights.untitled'))
 const blank = computed(() => !hl.loading.value && hl.total.value === 0 && !hl.hasActiveFilters.value)
+
+// Until the first load lands there is nothing to print but zeros, and a rail reading "0 highlights"
+// over an empty stream is a claim, not a loading state. Latched, because a later filter reload
+// must not throw the pane back to the placeholder, which covers the box either settled layout
+// puts there so neither resolution moves anything.
+const resolved = ref(false)
+watch(hl.loading, (busy) => {
+  if (!busy) resolved.value = true
+})
+const firstLoadPending = computed(() => !resolved.value)
+const showFirstLoadSkeleton = useDeferredLoading(firstLoadPending)
 const stackedDetail = computed(() => isStacked.value && hl.activeItem.value != null)

 const readableFile = computed(() => props.book.files.find((file) => file.format != null && READER_OPENABLE_FORMATS.has(file.format as never)) ?? null)
@@ -192,8 +207,29 @@ watch(
  },
 )

-onMounted(() => window.addEventListener('keydown', handleKeydown))
-onBeforeUnmount(() => window.removeEventListener('keydown', handleKeydown))
+// The tab is kept alive, so a cached-but-hidden Highlights tab must stop swallowing j/k/? while
+// another tab is on screen. Mount and unmount stay bound too, for rendering outside a KeepAlive.
+function bindShortcuts() {
+  window.addEventListener('keydown', handleKeydown)
+}
+function unbindShortcuts() {
+  window.removeEventListener('keydown', handleKeydown)
+}
+onMounted(bindShortcuts)
+onBeforeUnmount(unbindShortcuts)
+onDeactivated(unbindShortcuts)
+
+let activatedBefore = false
+onActivated(() => {
+  bindShortcuts()
+  if (!activatedBefore) {
+    activatedBefore = true
+    return
+  }
+  // A sync can add highlights while another tab is open. Silent, because the stream already holds
+  // the right rows and a skeleton over them is the flash this avoids.
+  void hl.fetchHighlights({ silent: true })
+})
 </script>

 <template>
@@ -202,7 +238,13 @@ onBeforeUnmount(() => window.removeEventListener('keydown', handleKeydown))
      {{ hl.error.value }}
    </div>

-    <div v-if="blank" class="flex min-h-0 flex-1 flex-col">
+    <div v-if="!resolved" class="min-h-80 flex-1 rounded-xl border border-border bg-card xl:min-h-0" aria-busy="true">
+      <div v-if="showFirstLoadSkeleton" class="flex flex-col gap-2 px-3 py-3" aria-hidden="true">
+        <div v-for="row in 6" :key="row" class="h-4 rounded bg-muted animate-shimmer" />
+      </div>
+    </div>
+
+    <div v-else-if="blank" class="flex min-h-0 flex-1 flex-col">
      <HighlightsEmptyStage :book-title="bookTitle" :can-read="readableFile != null" @read="handleOpenReader" />
    </div>

@@ -1,9 +1,10 @@
 <script setup lang="ts">
-import { computed, nextTick, ref } from 'vue'
+import { computed, nextTick, onActivated, ref, watch } from 'vue'
 import { useI18n } from 'vue-i18n'
 import { Ellipsis, RotateCcw } from '@lucide/vue'
 import { Permission, type BookDetail, type UserBookStatus } from '@bookorbit/types'
 import { usePermissions } from '@/features/auth/composables/usePermissions'
+import { useDeferredLoading } from '@/composables/useDeferredLoading'
 import { useBookReadingLog, type AddReadingSessionPayload } from '@/features/book/composables/useBookReadingLog'
 import { useReadingAttempts, type ReadingAttemptDraft } from '@/features/book/composables/useReadingAttempts'
 import { useResetReadingState } from '@/features/book/composables/useResetReadingState'
@@ -18,6 +19,9 @@ import ReadingLogEmptyStage from '../reading-log/ReadingLogEmptyStage.vue'
 import ReadingLogExportMenu from './ReadingLogExportMenu.vue'
 import AddSessionDialog from './AddSessionDialog.vue'

+// Named for the KeepAlive `include` list in BookDetailView.
+defineOptions({ name: 'ReadingLogTab' })
+
 const props = defineProps<{ book: BookDetail }>()

 const emit = defineEmits<{
@@ -57,6 +61,19 @@ const {
  remove: removeAttempt,
 } = useReadingAttempts(bookIdRef)

+// The tab is kept alive, so returning to it shows the rows it already had rather than replaying
+// the load. A sync can land a session while another tab is open, so revalidate on the way back -
+// silently, because a skeleton over rows that are already correct is the flash this avoids.
+let activatedBefore = false
+onActivated(() => {
+  if (!activatedBefore) {
+    activatedBefore = true
+    return
+  }
+  void reload({ silent: true })
+  void reloadAttempts({ silent: true })
+})
+
 const { hasPermission } = usePermissions()
 const canManageReadingState = computed(() => hasPermission(Permission.LibraryEditMetadata))
 const {
@@ -93,6 +110,17 @@ const blank = computed(
    !emptyStageDismissed.value && !loading.value && !attemptsLoading.value && (stats.value?.totalSessions ?? 0) === 0 && attempts.value.length === 0,
 )

+// The data decides which of two layouts the tab settles into, so until the first load lands there
+// is no shape to commit to. Latched, because a later filter reload must not throw the pane back
+// to the placeholder. The placeholder covers the same box either settled layout puts there, so
+// whichever way it resolves nothing moves.
+const resolved = ref(false)
+watch([loading, attemptsLoading], ([sessionsBusy, attemptsBusy]) => {
+  if (!sessionsBusy && !attemptsBusy) resolved.value = true
+})
+const firstLoadPending = computed(() => !resolved.value)
+const showFirstLoadSkeleton = useDeferredLoading(firstLoadPending)
+
 function buildDateFrom(quick: QuickFilter): string | undefined {
  const now = new Date()
  if (quick === 'last30') return new Date(now.getTime() - 30 * 24 * 60 * 60 * 1000).toISOString()
@@ -218,22 +246,38 @@ const quickFilters = computed<{ label: string; value: QuickFilter }[]>(() => [
    <div
      class="flex flex-col gap-4 xl:grid xl:min-h-0 xl:flex-1 xl:gap-x-5 xl:gap-y-4"
      :class="
-        blank
+        blank && resolved
          ? 'xl:grid-cols-[17rem_minmax(0,1fr)] xl:grid-rows-[minmax(0,1fr)]'
          : 'xl:grid-cols-[17rem_minmax(0,1fr)_19.25rem] xl:grid-rows-[minmax(0,1fr)_13.5rem]'
      "
    >
      <ReadingLogVitals
        class="xl:col-start-1 xl:row-start-1"
+        :class="{ 'xl:row-span-2': !resolved }"
        :book="book"
        :stats="stats"
        :loading="loading"
-        :hide-add-session="blank"
+        :hide-add-session="blank || !resolved"
        @saved="handleVitalsSaved"
        @add-session="handleOpenAddSession"
      />

-      <ReadingLogEmptyStage v-if="blank" class="xl:col-start-2 xl:row-start-1" @add-session="handleOpenAddSession" @record-past="handleRecordPast" />
+      <div
+        v-if="!resolved"
+        class="min-h-80 rounded-xl border border-border bg-card xl:col-start-2 xl:col-span-2 xl:row-start-1 xl:row-span-2 xl:min-h-0"
+        aria-busy="true"
+      >
+        <div v-if="showFirstLoadSkeleton" class="flex flex-col gap-2 px-3 py-3" aria-hidden="true">
+          <div v-for="row in 6" :key="row" class="h-4 rounded bg-muted animate-shimmer" />
+        </div>
+      </div>
+
+      <ReadingLogEmptyStage
+        v-else-if="blank"
+        class="xl:col-start-2 xl:row-start-1"
+        @add-session="handleOpenAddSession"
+        @record-past="handleRecordPast"
+      />

      <template v-else>
        <ReadingLogLedger
@@ -95,10 +95,14 @@ export function useBookHighlights(bookIdRef: Ref<number>) {
  }

  let fetchSeq = 0
-  async function fetchHighlights() {
+  /**
+   * `silent` refetches without raising `loading`, so a revalidation behind an already-rendered
+   * stream cannot flip the tab through its empty state on the way back.
+   */
+  async function fetchHighlights(opts?: { silent?: boolean }) {
    const bookId = bookIdRef.value
    const seq = ++fetchSeq
-    loading.value = true
+    if (!opts?.silent) loading.value = true
    error.value = null
    try {
      const res = await api(`/api/v1/books/${bookId}/annotations?${buildParams(1).toString()}`)
@@ -42,11 +42,15 @@ export function useBookReadingLog(bookIdRef: Ref<number>) {
    return params
  }

-  async function fetchSessions(opts?: { append?: boolean }) {
+  /**
+   * `silent` refetches without raising `loading`, so a revalidation behind already-rendered rows
+   * cannot dim or skeleton them. Only a load with nothing on screen yet is worth announcing.
+   */
+  async function fetchSessions(opts?: { append?: boolean; silent?: boolean }) {
    const bookId = bookIdRef.value
    const append = opts?.append ?? false
    if (append) loadingMore.value = true
-    else loading.value = true
+    else if (!opts?.silent) loading.value = true
    error.value = null
    try {
      const res = await api(`/api/v1/books/${bookId}/sessions?${buildParams().toString()}`)
@@ -127,9 +131,9 @@ export function useBookReadingLog(bookIdRef: Ref<number>) {
    await fetchSessions()
  }

-  async function reload() {
+  async function reload(opts?: { silent?: boolean }) {
    page.value = 1
-    await fetchSessions()
+    await fetchSessions({ silent: opts?.silent })
  }

  async function exportAll(): Promise<BookReadingSession[]> {
@@ -29,9 +29,10 @@ export function useReadingAttempts(bookIdRef: Ref<number>) {
    return new Map(ordered.map((attempt, index) => [attempt.id, index + 1]))
  })

-  async function load() {
+  /** `silent` keeps `loading` down so a revalidation behind rendered rows cannot skeleton them. */
+  async function load(opts?: { silent?: boolean }) {
    const bookId = bookIdRef.value
-    loading.value = true
+    if (!opts?.silent) loading.value = true
    error.value = null
    try {
      const res = await api(`/api/v1/books/${bookId}/reading-attempts?page=1&pageSize=${PAGE_SIZE}`)
@@ -1,5 +1,5 @@
 <script setup lang="ts">
-import { computed, defineAsyncComponent, provide, watch } from 'vue'
+import { computed, defineAsyncComponent, onMounted, provide, watch } from 'vue'
 import { useRoute } from 'vue-router'
 import { useI18n } from 'vue-i18n'
 import type { BookDetail, BookMetadataLockField } from '@bookorbit/types'
@@ -20,8 +20,12 @@ import { useCoverTint } from '@/features/book/composables/useCoverTint'
 import { useDisplaySettings } from '@/composables/useDisplaySettings'
 import EntityNotFound from '@/components/EntityNotFound.vue'

-const ReadingLogTab = defineAsyncComponent(() => import('@/features/book/components/detail/tabs/ReadingLogTab.vue'))
-const HighlightsTab = defineAsyncComponent(() => import('@/features/book/components/detail/tabs/HighlightsTab.vue'))
+const loadReadingLogTab = () => import('@/features/book/components/detail/tabs/ReadingLogTab.vue')
+const loadHighlightsTab = () => import('@/features/book/components/detail/tabs/HighlightsTab.vue')
+const ReadingLogTab = defineAsyncComponent(loadReadingLogTab)
+const HighlightsTab = defineAsyncComponent(loadHighlightsTab)
+
+const KEPT_ALIVE_TABS = ['ReadingLogTab', 'HighlightsTab']

 const { t } = useI18n()
 const route = useRoute()
@@ -97,6 +101,17 @@ onBookProgressChanged((event) => {

 watch(bookId, (id) => fetch(id), { immediate: true })

+// The two lazy tabs render nothing at all while their chunk arrives, so a cold click on either
+// blanks the pane. Warm both once the page is idle and the first click has a component to mount.
+onMounted(() => {
+  const warm = () => {
+    void loadReadingLogTab()
+    void loadHighlightsTab()
+  }
+  if (typeof requestIdleCallback === 'function') requestIdleCallback(warm, { timeout: 2000 })
+  else setTimeout(warm, 500)
+})
+
 function onMetadataSaved(updated: BookDetail) {
  detail.value = updated
 }
@@ -120,17 +135,24 @@ function onCoverChanged(source: 'extracted' | 'custom' | null) {
  <BookDetailLayout :book-id="bookId" :cover-tint="coverTint">
    <Transition name="content" mode="out-in">
      <div v-if="detail" key="detail" class="h-full">
-        <DetailsTab v-if="tab === 'details'" :book="detail" @saved="onMetadataSaved" @moved="handleMovedToLibrary" />
-        <EditMetadataTab
-          v-else-if="tab === 'edit' && hasPermission('library_edit_metadata')"
-          :book="detail"
-          @saved="onMetadataSaved"
-          @locks-changed="onLocksChanged"
-          @cover-changed="onCoverChanged"
-        />
-        <FilesTab v-else-if="tab === 'files'" :book="detail" @refetch="fetch(detail.id)" />
-        <ReadingLogTab v-else-if="tab === 'reading-log'" :book="detail" @saved="onMetadataSaved" />
-        <HighlightsTab v-else-if="tab === 'highlights'" :book="detail" />
+        <!--
+          Only the two lazy tabs are cached. Both fetch on mount, so without this every return
+          replays blank -> skeleton -> content in under 120ms, which reads as a flash rather than
+          as loading. They revalidate silently on activation instead.
+        -->
+        <KeepAlive :include="KEPT_ALIVE_TABS">
+          <DetailsTab v-if="tab === 'details'" :book="detail" @saved="onMetadataSaved" @moved="handleMovedToLibrary" />
+          <EditMetadataTab
+            v-else-if="tab === 'edit' && hasPermission('library_edit_metadata')"
+            :book="detail"
+            @saved="onMetadataSaved"
+            @locks-changed="onLocksChanged"
+            @cover-changed="onCoverChanged"
+          />
+          <FilesTab v-else-if="tab === 'files'" :book="detail" @refetch="fetch(detail.id)" />
+          <ReadingLogTab v-else-if="tab === 'reading-log'" :book="detail" @saved="onMetadataSaved" />
+          <HighlightsTab v-else-if="tab === 'highlights'" :book="detail" />
+        </KeepAlive>
      </div>

      <div v-else-if="loading" key="loading">
@@ -162,26 +184,19 @@ function onCoverChanged(source: 'extracted' | 'custom' | null) {
        <div v-else-if="tab === 'files'" class="space-y-3">
          <div v-for="i in 3" :key="i" class="h-16 rounded-md bg-muted animate-shimmer" />
        </div>
+        <!--
+          These two match the box the tab itself puts up while its own first load runs, so the
+          handover from "book loading" to "tab loading" changes the fill and never the layout.
+        -->
        <div
          v-else-if="tab === 'reading-log'"
          class="flex h-full min-h-0 flex-col gap-4 xl:grid xl:grid-cols-[17rem_minmax(0,1fr)_19.25rem] xl:grid-rows-[minmax(0,1fr)_13.5rem] xl:gap-x-5 xl:gap-y-4"
        >
-          <div class="h-64 rounded-xl bg-muted animate-shimmer xl:col-start-1 xl:row-start-1 xl:h-auto" />
-          <div class="h-72 rounded-xl bg-muted animate-shimmer xl:col-start-2 xl:row-start-1 xl:h-auto" />
-          <div class="flex flex-col gap-4 xl:col-start-3 xl:row-start-1 xl:min-h-0">
-            <div class="h-40 rounded-xl bg-muted animate-shimmer xl:min-h-0 xl:flex-1" />
-            <div class="h-[9.375rem] shrink-0 rounded-xl bg-muted animate-shimmer" />
-          </div>
-          <div class="h-56 rounded-xl bg-muted animate-shimmer xl:col-span-full xl:row-start-2 xl:h-auto" />
+          <div class="h-64 rounded-xl bg-muted animate-shimmer xl:col-start-1 xl:row-start-1 xl:row-span-2 xl:h-auto" />
+          <div class="min-h-80 rounded-xl bg-muted animate-shimmer xl:col-start-2 xl:col-span-2 xl:row-start-1 xl:row-span-2 xl:min-h-0" />
        </div>
-        <div v-else-if="tab === 'highlights'" class="space-y-4">
-          <div class="flex gap-2">
-            <div class="h-9 flex-1 rounded-md bg-muted animate-shimmer" />
-            <div v-for="i in 3" :key="i" class="h-9 w-9 rounded-full bg-muted animate-shimmer" />
-          </div>
-          <div class="space-y-3">
-            <div v-for="i in 3" :key="i" class="h-24 rounded-lg bg-muted animate-shimmer" />
-          </div>
+        <div v-else-if="tab === 'highlights'" class="flex h-full min-h-0 flex-col">
+          <div class="min-h-80 flex-1 rounded-xl bg-muted animate-shimmer xl:min-h-0" />
        </div>
      </div>

@@ -343,10 +343,12 @@ describe('GoodreadsProvider', () => {
        return Promise.resolve({ ok: true, status: 202, text: () => Promise.resolve('<div id="challenge-container"></div>') });
      }) as never;

-      const searchPromise = provider.search({ title: 'B' });
+      // The rejection lands while the timers run, so the assertion has to be attached before then:
+      // a rejected promise nobody is holding is an unhandled rejection, which fails the whole run.
+      const rejects = expect(provider.search({ title: 'B' })).rejects.toBeInstanceOf(ProviderThrottleError);
      await vi.runAllTimersAsync();

-      await expect(searchPromise).rejects.toBeInstanceOf(ProviderThrottleError);
+      await rejects;
      const detailUrls = vi
        .mocked(global.fetch)
        .mock.calls.map(([url]) => fetchUrl(url))