diff --git a/vue/src/components/AppNav.vue b/vue/src/components/AppNav.vue index e6c8377..d3f8e89 100644 --- a/vue/src/components/AppNav.vue +++ b/vue/src/components/AppNav.vue @@ -1,20 +1,96 @@ - + RSS Reader ({{ unreadCount }}) diff --git a/vue/src/components/__tests__/AppNav.spec.js b/vue/src/components/__tests__/AppNav.spec.js index 7570444..16d8604 100644 --- a/vue/src/components/__tests__/AppNav.spec.js +++ b/vue/src/components/__tests__/AppNav.spec.js @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { mount, flushPromises } from '@vue/test-utils' import { createRouter, createWebHistory } from 'vue-router' import axios from 'axios' @@ -15,6 +15,12 @@ class FakeIntersectionObserver { disconnect() {} } vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver) +vi.stubGlobal('requestAnimationFrame', cb => cb()) + +function scrollTo(y) { + Object.defineProperty(window, 'scrollY', { value: y, configurable: true, writable: true }) + window.dispatchEvent(new Event('scroll')) +} describe('AppNav', () => { let router @@ -23,6 +29,7 @@ describe('AppNav', () => { localStorage.setItem('user-token', 'abc123') localStorage.setItem('user-id', '7') vi.clearAllMocks() + scrollTo(0) const { feeds, showMessage, message, showModal, viewMode, currentIndex, layout } = useFeeds() feeds.value = [] @@ -44,15 +51,45 @@ describe('AppNav', () => { await router.isReady() }) + // Safety net: if a test using fake timers fails before reaching its own + // vi.useRealTimers(), real timers must still be restored so it doesn't + // silently break the rAF-dependent scroll handling in later tests (fake + // timers by default also fake requestAnimationFrame). + // + // Also unmounts every AppNav mounted via mountNav() this test — without + // this, each test's `window.addEventListener('scroll', ...)` from AppNav's + // onMounted piles up across the whole file (nothing ever unmounts them + // otherwise), so a later scroll-correction test can have its + // consumeScrollCorrection() flag "stolen" by a stale listener from an + // earlier, unrelated test that happens to still be registered. + let mountedWrappers = [] + function mountNav(options = { global: { plugins: [router] } }) { + const wrapper = mount(AppNav, options) + mountedWrappers.push(wrapper) + return wrapper + } + + afterEach(() => { + vi.useRealTimers() + for (const wrapper of mountedWrappers) { + try { + wrapper.unmount() + } catch { + // already unmounted by the test itself — fine + } + } + mountedWrappers = [] + }) + async function mountWithMenuOpen() { - const wrapper = mount(AppNav, { global: { plugins: [router] } }) + const wrapper = mountNav() await wrapper.find('.app-nav__hamburger').trigger('click') await flushPromises() return wrapper } it('toggles the menu open and closed via the hamburger button', async () => { - const wrapper = mount(AppNav, { global: { plugins: [router] } }) + const wrapper = mountNav() expect(wrapper.find('.app-nav__menu').exists()).toBe(false) @@ -164,7 +201,7 @@ describe('AppNav', () => { { id: 2, title: 'Article two', content: '', url: 'https://example.test/2', timestamp: '2026-01-02' }, ] - const wrapper = mount(AppNav, { global: { plugins: [router] } }) + const wrapper = mountNav() await flushPromises() expect(wrapper.find('.app-nav__title').text()).toContain('(2)') @@ -177,14 +214,14 @@ describe('AppNav', () => { { id: 2, title: 'Article two', read: false, content: '', url: 'https://example.test/2', timestamp: '2026-01-02' }, ] - const wrapper = mount(AppNav, { global: { plugins: [router] } }) + const wrapper = mountNav() await flushPromises() expect(wrapper.find('.app-nav__title').text()).toContain('(1)') }) it('hides the unread count when there are no articles', async () => { - const wrapper = mount(AppNav, { global: { plugins: [router] } }) + const wrapper = mountNav() await flushPromises() expect(wrapper.find('.app-nav__unread').exists()).toBe(false) @@ -208,4 +245,152 @@ describe('AppNav', () => { confirmSpy.mockRestore() }) + + it('compacts the header once scrolled past the enter threshold', async () => { + const wrapper = mountNav() + + scrollTo(100) + await wrapper.vm.$nextTick() + + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + }) + + it('stays expanded for scroll offsets inside the hysteresis dead zone', async () => { + const wrapper = mountNav() + + scrollTo(50) + await wrapper.vm.$nextTick() + + expect(wrapper.find('.app-nav').classes()).not.toContain('app-nav--compact') + }) + + it('expands the header again once scrolled back near the top', async () => { + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const wrapper = mountNav() + + scrollTo(100) + await wrapper.vm.$nextTick() + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + scrollTo(10) + await wrapper.vm.$nextTick() + // The expand transition is debounced (see the article-read scroll-dip + // test below), so it shouldn't flip immediately... + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + vi.advanceTimersByTime(150) + await wrapper.vm.$nextTick() + // ...but should once the debounce window elapses with scrollY still low. + expect(wrapper.find('.app-nav').classes()).not.toContain('app-nav--compact') + + vi.useRealTimers() + }) + + it('does not flash expanded on a brief scroll dip below the exit threshold', async () => { + // Regression test: useFeeds.js's handleIntersection corrects the scroll + // position with a window.scrollBy after marking an article read, which can + // transiently dip scrollY below COMPACT_EXIT even while the user keeps + // scrolling down. That one-off dip must not visibly re-expand the header. + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const wrapper = mountNav() + + scrollTo(100) + await wrapper.vm.$nextTick() + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + scrollTo(10) // the corrective dip + await wrapper.vm.$nextTick() + scrollTo(80) // scrolling continues downward right after + await wrapper.vm.$nextTick() + + vi.advanceTimersByTime(200) + await wrapper.vm.$nextTick() + + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + vi.useRealTimers() + }) + + it('stays compact through a real article-read scroll correction, even after a sustained pause', async () => { + // Regression test for the reported glitch: unlike a brief flash, marking + // an article read (list view) can leave scrollY genuinely low for as + // long as the user pauses between scroll gestures — e.g. right when they + // finish one article and are about to start the next — because the list + // view removes read articles from the DOM, collapsing the remainder back + // toward the top of the page. A fixed debounce alone can't distinguish + // that from a real "scrolled back to the top". This drives the actual + // useFeeds.js handleIntersection correction (not a simulated dip) and + // confirms the header stays compact even once the debounce window has + // fully elapsed with no further scrolling. + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + const { feeds: feedsRef, handleIntersection, setInitialLoad } = useFeeds() + feedsRef.value = [{ id: 201, title: 'Only article' }] + setInitialLoad(true) + axios.put.mockResolvedValue({ status: 200 }) + + const wrapper = mountNav() + + scrollTo(100) + await wrapper.vm.$nextTick() + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + // jsdom's getBoundingClientRect().top defaults to 0 for any element, so a + // topbarHeight > 0 exercises the same "article now sits above the header" + // correction branch a real browser would take. + const observeDiv = document.createElement('div') + observeDiv.className = 'observe' + document.body.appendChild(observeDiv) + await handleIntersection([ + { isIntersecting: false, boundingClientRect: { y: -10 }, target: { id: '0' } }, + ], 60) + await flushPromises() + + // The real window.scrollBy correction would land scrollY low, like this. + scrollTo(10) + await wrapper.vm.$nextTick() + + // Long past the debounce window, with no further scrolling — simulating + // the user pausing right at the top of the next article. + vi.advanceTimersByTime(300) + await wrapper.vm.$nextTick() + + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + document.body.removeChild(observeDiv) + setInitialLoad(false) + vi.useRealTimers() + }) + + it('stays compact while scrolling within the hysteresis dead zone', async () => { + const wrapper = mountNav() + + scrollTo(100) + await wrapper.vm.$nextTick() + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + + scrollTo(50) + await wrapper.vm.$nextTick() + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + }) + + it('does not throw on rapid repeated scroll events', async () => { + const wrapper = mountNav() + + for (let y = 0; y <= 200; y += 5) { + scrollTo(y) + } + await wrapper.vm.$nextTick() + + expect(wrapper.find('.app-nav').classes()).toContain('app-nav--compact') + }) + + it('removes the scroll listener on unmount', async () => { + const removeSpy = vi.spyOn(window, 'removeEventListener') + const wrapper = mountNav() + + wrapper.unmount() + + expect(removeSpy).toHaveBeenCalledWith('scroll', expect.any(Function)) + removeSpy.mockRestore() + }) }) diff --git a/vue/src/composables/__tests__/useFeeds.spec.js b/vue/src/composables/__tests__/useFeeds.spec.js index 61fe600..bf6e9c3 100644 --- a/vue/src/composables/__tests__/useFeeds.spec.js +++ b/vue/src/composables/__tests__/useFeeds.spec.js @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' +import { flushPromises } from '@vue/test-utils' import axios from 'axios' import { useFeeds } from '../useFeeds' @@ -12,7 +13,7 @@ class FakeIntersectionObserver { vi.stubGlobal('IntersectionObserver', FakeIntersectionObserver) describe('useFeeds', () => { - const { feeds, showMessage, message, showModal, fetchData, sync, getReadable, setInitialLoad, handleIntersection } = useFeeds() + const { feeds, showMessage, message, showModal, fetchData, sync, getReadable, setInitialLoad, handleIntersection, consumeScrollCorrection } = useFeeds() beforeEach(() => { localStorage.setItem('user-token', 'test-token') @@ -116,6 +117,36 @@ describe('useFeeds', () => { setInitialLoad(false) }) + it('flags a scroll correction when marking an article read repositions the list', async () => { + // Regression coverage for the header-compacting glitch: AppNav's scroll + // handler needs a way to tell "the page moved because content was + // removed" apart from a real user scroll. jsdom's getBoundingClientRect() + // defaults every element's top to 0, so any topbarHeight > 0 exercises + // the same "first article now sits above the header" branch that fires + // in a real browser. + feeds.value = [{ id: 101, title: 'First' }, { id: 102, title: 'Second' }] + setInitialLoad(true) + axios.put.mockResolvedValue({ status: 200 }) + + const observeDiv = document.createElement('div') + observeDiv.className = 'observe' + document.body.appendChild(observeDiv) + + expect(consumeScrollCorrection()).toBe(false) + + await handleIntersection([ + { isIntersecting: false, boundingClientRect: { y: -10 }, target: { id: '0' } }, + ], 60) + await flushPromises() + + expect(consumeScrollCorrection()).toBe(true) + // Consume-once: asking again without another correction reports false. + expect(consumeScrollCorrection()).toBe(false) + + document.body.removeChild(observeDiv) + setInitialLoad(false) + }) + it('strips leftover embedded-video placeholder headings', async () => { feeds.value = [{ id: 1, diff --git a/vue/src/composables/useFeeds.js b/vue/src/composables/useFeeds.js index 5ecf050..9ec5f98 100644 --- a/vue/src/composables/useFeeds.js +++ b/vue/src/composables/useFeeds.js @@ -14,6 +14,13 @@ const layout = ref(localStorage.getItem('layout') || 'list') // 'list' | 'cards' let observer; // Declare observer outside the setup function let initialLoad = false +// Set right before the scroll-position correction below, so AppNav's +// scroll-driven header compacting can tell "the page just moved because +// content was removed" apart from a real user scroll — otherwise every +// article marked read (which resets scrollY toward the top of the +// now-shorter list) would look identical to the user manually scrolling +// back to the top. +let scrollCorrectionPending = false export function authHeaders() { return { @@ -265,6 +272,7 @@ function handleIntersection(entries, topbarHeight = 0) { if (first) { const top = first.getBoundingClientRect().top if (top < topbarHeight) { + scrollCorrectionPending = true window.scrollBy(0, top - topbarHeight) } } @@ -272,6 +280,17 @@ function handleIntersection(entries, topbarHeight = 0) { }) } +// Consume-once: returns whether a scroll-position correction is pending +// (and clears it), so a caller can tell this scroll event apart from a +// real user scroll without the flag lingering into later, unrelated ones. +function consumeScrollCorrection() { + if (scrollCorrectionPending) { + scrollCorrectionPending = false + return true + } + return false +} + function disconnectObserver() { if (observer) { observer.disconnect() @@ -386,6 +405,7 @@ export function useFeeds() { markAllRead, showMessageForXSeconds, setupIntersectionObserver, + consumeScrollCorrection, disconnectObserver, setInitialLoad, handleIntersection,