Skip to content

Commit d34dc76

Browse files
brianlovinclaude
andauthored
Fix sidebar story list scroll-into-view for off-screen selections (#34)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
1 parent c675822 commit d34dc76

2 files changed

Lines changed: 89 additions & 5 deletions

File tree

src/components/StoryList.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,11 +164,14 @@ export function scrollToStory(
164164
state: StoryListState,
165165
index: number,
166166
): void {
167-
const itemHeight = 2; // Each story item is ~2 lines
168-
const gap = 1; // Gap between items
169-
const itemTop = index * (itemHeight + gap);
170-
const itemBottom = itemTop + itemHeight;
171-
const viewportHeight = state.scroll.height;
167+
const item = state.items.get(index);
168+
if (!item) return;
169+
170+
// Get the item's position relative to the scroll content
171+
const contentY = state.scroll.content.y;
172+
const itemTop = item.y - contentY;
173+
const itemBottom = itemTop + item.height;
174+
const viewportHeight = state.scroll.viewport.height;
172175
const currentScroll = state.scroll.scrollTop;
173176

174177
// Only scroll if the item is outside the visible viewport

src/test/app.test.ts

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,87 @@ describe("HackerNewsApp", () => {
114114
});
115115
});
116116

117+
describe("Story List Scroll", () => {
118+
it("should scroll down to show off-screen selected story", async () => {
119+
// Create more stories than can fit in viewport
120+
const posts = createMockPosts(20);
121+
ctx.app.setPostsForTesting(posts);
122+
await ctx.renderOnce();
123+
124+
const storyListState = (ctx.app as any).storyListState;
125+
expect(storyListState.scroll.scrollTop).toBe(0);
126+
127+
// Navigate down past the visible viewport (each story is ~3 lines with gap)
128+
for (let i = 0; i < 15; i++) {
129+
ctx.mockInput.pressKey("j");
130+
await ctx.renderer.idle();
131+
await ctx.renderOnce();
132+
}
133+
// Extra render to ensure scroll position is fully applied
134+
await ctx.renderer.idle();
135+
await ctx.renderOnce();
136+
137+
// Scroll should have changed to show the selected story
138+
expect(ctx.app.currentSelectedIndex).toBe(14);
139+
expect(storyListState.scroll.scrollTop).toBeGreaterThan(0);
140+
});
141+
142+
it("should scroll up to show off-screen selected story", async () => {
143+
// Create more stories than can fit in viewport
144+
const posts = createMockPosts(20);
145+
ctx.app.setPostsForTesting(posts);
146+
await ctx.renderOnce();
147+
148+
const storyListState = (ctx.app as any).storyListState;
149+
150+
// Navigate down to bottom
151+
for (let i = 0; i < 19; i++) {
152+
ctx.mockInput.pressKey("j");
153+
await ctx.renderer.idle();
154+
await ctx.renderOnce();
155+
}
156+
await ctx.renderer.idle();
157+
await ctx.renderOnce();
158+
expect(ctx.app.currentSelectedIndex).toBe(18);
159+
const scrollAtBottom = storyListState.scroll.scrollTop;
160+
expect(scrollAtBottom).toBeGreaterThan(0);
161+
162+
// Navigate back up past visible viewport
163+
for (let i = 0; i < 15; i++) {
164+
ctx.mockInput.pressKey("k");
165+
await ctx.renderer.idle();
166+
await ctx.renderOnce();
167+
}
168+
await ctx.renderer.idle();
169+
await ctx.renderOnce();
170+
171+
// Scroll should have decreased to show the selected story
172+
expect(ctx.app.currentSelectedIndex).toBe(3);
173+
expect(storyListState.scroll.scrollTop).toBeLessThan(scrollAtBottom);
174+
});
175+
176+
it("should not scroll when selected story is already visible", async () => {
177+
const posts = createMockPosts(20);
178+
ctx.app.setPostsForTesting(posts);
179+
await ctx.renderOnce();
180+
181+
const storyListState = (ctx.app as any).storyListState;
182+
183+
// Select first story
184+
ctx.mockInput.pressKey("j");
185+
await ctx.renderer.idle();
186+
await ctx.renderOnce();
187+
expect(storyListState.scroll.scrollTop).toBe(0);
188+
189+
// Navigate to second story (should still be visible without scrolling)
190+
ctx.mockInput.pressKey("j");
191+
await ctx.renderer.idle();
192+
await ctx.renderOnce();
193+
expect(ctx.app.currentSelectedIndex).toBe(1);
194+
expect(storyListState.scroll.scrollTop).toBe(0);
195+
});
196+
});
197+
117198
describe("Comment Navigation", () => {
118199
beforeEach(async () => {
119200
const mockPost = createMockPostWithComments({}, 5);

0 commit comments

Comments
 (0)