Skip to content

Comments

refactor(story): use shared scrollbar module in story overlay#227

Merged
forketyfork merged 2 commits intomainfrom
refactor/story-scrollbar-centralize
Feb 20, 2026
Merged

refactor(story): use shared scrollbar module in story overlay#227
forketyfork merged 2 commits intomainfrom
refactor/story-scrollbar-centralize

Conversation

@forketyfork
Copy link
Owner

The story overlay was the last holdout with its own scrollbar drawing code. FullscreenOverlay.renderScrollbar() did the job with plain SDL FillRect calls — flat rectangles, fixed alpha, no fade, no rounding, no hover state. Every other overlay (reader, diff, terminal) already used scrollbar.zig, which gives you a rounded gradient thumb, fade-in/out animations, and full mouse interaction.

This removes renderScrollbar() and brings the story overlay in line with the rest. story_overlay.zig now holds a scrollbar.State field and calls scrollbar.computeLayout() + scrollbar.render(). Mouse events are wired: wheel triggers activity, left-click on the thumb starts a drag, left-click on the track jumps the position, motion handles drag updates and hover, and button-up ends the drag. computeWrapCols now uses scrollbar.reservedWidth() instead of a hardcoded 10px estimate.

Closes #226

Test plan

  • Open a story file with enough content to scroll (e.g. architect story <some-long-story.md>)
  • Scroll with the mouse wheel — the scrollbar should fade in, then fade out after ~1.5s of inactivity
  • Hover over the scrollbar — it should stay visible and the thumb should brighten
  • Drag the thumb — the content should follow
  • Click on the track above or below the thumb — the view should jump to that position
  • Compare the scrollbar appearance to the reader overlay (Cmd+R) — rounded thumb, same visual style

Issue: story_overlay.zig had its own renderScrollbar() on FullscreenOverlay — plain SDL FillRect calls, no animations, no rounded thumb, no interaction. Every other overlay used scrollbar.zig.
Solution: Deleted renderScrollbar() from FullscreenOverlay and wired story_overlay.zig to scrollbar.computeLayout() + scrollbar.render() with a scrollbar.State field. Added mouse wheel activity signals, drag and track-click on button-down, drag updates and hover detection on motion, and drag-end on button-up. computeWrapCols now uses scrollbar.reservedWidth() instead of a hardcoded pixel value.
@forketyfork forketyfork requested a review from Copilot February 20, 2026 06:25
@forketyfork forketyfork marked this pull request as ready for review February 20, 2026 06:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the story overlay to use the shared scrollbar.zig component (matching reader/diff/terminal overlays) and removes the bespoke SDL FillRect scrollbar implementation.

Changes:

  • Add scrollbar.State to StoryOverlayComponent and render the shared scrollbar via scrollbar.computeLayout() + scrollbar.render().
  • Wire mouse interactions for wheel activity, thumb dragging, track-click jumps, and hover-driven fade behavior.
  • Update wrap column computation to reserve width using scrollbar.reservedWidth() instead of a hardcoded estimate.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/ui/components/story_overlay.zig Integrates shared scrollbar state/rendering and hooks up mouse events + wrap-width reservation.
src/ui/components/fullscreen_overlay.zig Removes the old renderScrollbar() implementation now superseded by scrollbar.zig.
Comments suppressed due to low confidence (1)

src/ui/components/story_overlay.zig:472

  • The cursor state transition logic compares want_pointer against was_pointer derived only from the previous anchor/link hover state. Since want_pointer now includes scrollbar hover/drag, moving off the scrollbar can leave the cursor stuck as a pointer (because was_pointer doesn't include prior scrollbar hit/drag state). Track previous scrollbar-hit/hover/drag state as part of was_pointer, or set the cursor unconditionally based on want_pointer each motion event.
                const want_pointer = self.hovered_anchor != null or self.hovered_link != null or self.scrollbar_state.dragging or scroll_hit != .none;
                const was_pointer = prev_hovered_anchor != null or prev_link != null;
                if (want_pointer != was_pointer) {
                    const cursor = if (want_pointer) self.pointer_cursor else self.arrow_cursor;
                    if (cursor) |cur| _ = c.SDL_SetCursor(cur);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d8fdcd535

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Issue: Four gaps found in PR #227 review: keyboard scroll didn't trigger the scrollbar fade-in, mouse-up could leak through the overlay when not dragging, the overlay didn't reset scrollbar state on hide, and the pointer cursor could get stuck when leaving the scrollbar.
Solution: noteActivity is now called after handleScrollKey returns true. Mouse-up always consumes the event while the overlay is visible and gates endDrag on the left button. hide() calls scrollbar_state.hideNow(). was_pointer now captures the previous scrollbar hover/drag state so the cursor resets correctly when moving off the scrollbar into plain content.
@forketyfork forketyfork merged commit 36ac734 into main Feb 20, 2026
4 checks passed
@forketyfork forketyfork deleted the refactor/story-scrollbar-centralize branch February 20, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Story overlay scrollbar is a custom reimplementation instead of using the centralized scrollbar component

1 participant