From 2d303d74c6b99ec8ff7f2a7a28bef2c8e44e3fb1 Mon Sep 17 00:00:00 2001 From: Brian Lovin Date: Tue, 27 Jan 2026 18:33:59 -0800 Subject: [PATCH 1/3] Fix sidebar story list scroll-into-view for off-screen selections Use actual item positions from layout instead of hardcoded heights. Story items have variable heights due to text wrapping and optional domain lines, so calculating position as index * (itemHeight + gap) was incorrect. Now uses the rendered item's y position and viewport.height for accurate scroll calculations. Co-Authored-By: Claude Haiku 4.5 --- src/components/StoryList.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/components/StoryList.ts b/src/components/StoryList.ts index 8de14c7..12bfde9 100644 --- a/src/components/StoryList.ts +++ b/src/components/StoryList.ts @@ -164,11 +164,14 @@ export function scrollToStory( state: StoryListState, index: number, ): void { - const itemHeight = 2; // Each story item is ~2 lines - const gap = 1; // Gap between items - const itemTop = index * (itemHeight + gap); - const itemBottom = itemTop + itemHeight; - const viewportHeight = state.scroll.height; + const item = state.items.get(index); + if (!item) return; + + // Get the item's position relative to the scroll content + const contentY = state.scroll.content.y; + const itemTop = item.y - contentY; + const itemBottom = itemTop + item.height; + const viewportHeight = state.scroll.viewport.height; const currentScroll = state.scroll.scrollTop; // Only scroll if the item is outside the visible viewport From 8b63bbca0afe645e4329fc105952236c15546e4d Mon Sep 17 00:00:00 2001 From: Brian Lovin Date: Tue, 27 Jan 2026 18:37:26 -0800 Subject: [PATCH 2/3] Add tests for story list scroll-into-view behavior Tests verify that: - Scrolls down when navigating to off-screen story - Scrolls up when navigating back to off-screen story - Does not scroll when story is already visible Co-Authored-By: Claude Haiku 4.5 --- src/test/app.test.ts | 74 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/src/test/app.test.ts b/src/test/app.test.ts index 58d5b2e..1e5b804 100644 --- a/src/test/app.test.ts +++ b/src/test/app.test.ts @@ -114,6 +114,80 @@ describe("HackerNewsApp", () => { }); }); + describe("Story List Scroll", () => { + it("should scroll down to show off-screen selected story", async () => { + // Create more stories than can fit in viewport + const posts = createMockPosts(20); + ctx.app.setPostsForTesting(posts); + await ctx.renderOnce(); + + const storyListState = (ctx.app as any).storyListState; + expect(storyListState.scroll.scrollTop).toBe(0); + + // Navigate down past the visible viewport (each story is ~3 lines with gap) + for (let i = 0; i < 15; i++) { + ctx.mockInput.pressKey("j"); + await ctx.renderer.idle(); // Wait for async selectStory to complete + await ctx.renderOnce(); + } + + // Scroll should have changed to show the selected story + expect(ctx.app.currentSelectedIndex).toBe(14); + expect(storyListState.scroll.scrollTop).toBeGreaterThan(0); + }); + + it("should scroll up to show off-screen selected story", async () => { + // Create more stories than can fit in viewport + const posts = createMockPosts(20); + ctx.app.setPostsForTesting(posts); + await ctx.renderOnce(); + + const storyListState = (ctx.app as any).storyListState; + + // Navigate down to bottom + for (let i = 0; i < 19; i++) { + ctx.mockInput.pressKey("j"); + await ctx.renderer.idle(); + await ctx.renderOnce(); + } + expect(ctx.app.currentSelectedIndex).toBe(18); + const scrollAtBottom = storyListState.scroll.scrollTop; + expect(scrollAtBottom).toBeGreaterThan(0); + + // Navigate back up past visible viewport + for (let i = 0; i < 15; i++) { + ctx.mockInput.pressKey("k"); + await ctx.renderer.idle(); + await ctx.renderOnce(); + } + + // Scroll should have decreased to show the selected story + expect(ctx.app.currentSelectedIndex).toBe(3); + expect(storyListState.scroll.scrollTop).toBeLessThan(scrollAtBottom); + }); + + it("should not scroll when selected story is already visible", async () => { + const posts = createMockPosts(20); + ctx.app.setPostsForTesting(posts); + await ctx.renderOnce(); + + const storyListState = (ctx.app as any).storyListState; + + // Select first story + ctx.mockInput.pressKey("j"); + await ctx.renderer.idle(); + await ctx.renderOnce(); + expect(storyListState.scroll.scrollTop).toBe(0); + + // Navigate to second story (should still be visible without scrolling) + ctx.mockInput.pressKey("j"); + await ctx.renderer.idle(); + await ctx.renderOnce(); + expect(ctx.app.currentSelectedIndex).toBe(1); + expect(storyListState.scroll.scrollTop).toBe(0); + }); + }); + describe("Comment Navigation", () => { beforeEach(async () => { const mockPost = createMockPostWithComments({}, 5); From 8753ee59b8d2b7fed944ceb1952488df51dd3270 Mon Sep 17 00:00:00 2001 From: Brian Lovin Date: Tue, 27 Jan 2026 18:38:59 -0800 Subject: [PATCH 3/3] Fix flaky scroll test by adding extra render cycle Add extra renderer.idle() and renderOnce() after navigation loops to ensure scroll positions are fully applied before assertions. This fixes timing issues that caused CI failures with older Bun versions. Co-Authored-By: Claude Haiku 4.5 --- src/test/app.test.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/app.test.ts b/src/test/app.test.ts index 1e5b804..ba211ca 100644 --- a/src/test/app.test.ts +++ b/src/test/app.test.ts @@ -127,9 +127,12 @@ describe("HackerNewsApp", () => { // Navigate down past the visible viewport (each story is ~3 lines with gap) for (let i = 0; i < 15; i++) { ctx.mockInput.pressKey("j"); - await ctx.renderer.idle(); // Wait for async selectStory to complete + await ctx.renderer.idle(); await ctx.renderOnce(); } + // Extra render to ensure scroll position is fully applied + await ctx.renderer.idle(); + await ctx.renderOnce(); // Scroll should have changed to show the selected story expect(ctx.app.currentSelectedIndex).toBe(14); @@ -150,6 +153,8 @@ describe("HackerNewsApp", () => { await ctx.renderer.idle(); await ctx.renderOnce(); } + await ctx.renderer.idle(); + await ctx.renderOnce(); expect(ctx.app.currentSelectedIndex).toBe(18); const scrollAtBottom = storyListState.scroll.scrollTop; expect(scrollAtBottom).toBeGreaterThan(0); @@ -160,6 +165,8 @@ describe("HackerNewsApp", () => { await ctx.renderer.idle(); await ctx.renderOnce(); } + await ctx.renderer.idle(); + await ctx.renderOnce(); // Scroll should have decreased to show the selected story expect(ctx.app.currentSelectedIndex).toBe(3);