feat(preview): add configurable scroll hotkeys and bulk scroll size#1506
Open
KristianHolme wants to merge 6 commits into
yorukot:mainyorukot/superfile:mainfrom
KristianHolme:feat/file-preview-scrollKristianHolme/superfile:feat/file-preview-scrollCopy head branch name to clipboard
Open
feat(preview): add configurable scroll hotkeys and bulk scroll size#1506KristianHolme wants to merge 6 commits intoyorukot:mainyorukot/superfile:mainfrom KristianHolme:feat/file-preview-scrollKristianHolme/superfile:feat/file-preview-scrollCopy head branch name to clipboard
KristianHolme wants to merge 6 commits into
yorukot:mainyorukot/superfile:mainfrom
KristianHolme:feat/file-preview-scrollKristianHolme/superfile:feat/file-preview-scrollCopy head branch name to clipboard
Conversation
File preview can scroll by line, bulk steps, and jump to top/bottom via explicit hotkeys while the panel is open in browser mode. Bulk scroll distance is controlled by preview_scroll_bulk in config.toml. Co-authored-by: Cursor <cursoragent@cursor.com>
Bubbletea reports shifted printable keys as their text form (J, K) while hotkey config uses shift+j and shift+k. Preview scroll matching now checks both String() and Keystroke() so configured shift combos work. Co-authored-by: Kristian Holme <KristianHolme@users.noreply.github.com>
Refactor renderTextPreview to satisfy gocognit, wrap long validation message for golines, and apply gofmt alignment in config_type. Co-authored-by: Kristian Holme <KristianHolme@users.noreply.github.com>
Remove HotkeyMatches helper and match preview scroll keys via the standard msg.String() + slices.Contains path. Regular defaults use shift+up/down only; vim defaults add K/J alongside arrow alternates. Co-authored-by: Kristian Holme <KristianHolme@users.noreply.github.com>
|
🎉 Thank you for your first contribution to superfile! We’re really excited to have you here 🙌 A maintainer might ask you to make a few changes before we can merge this PR. 👉 Please also take a moment to review our Contribution Guide If you have any questions, feel free to open a Discussion or just ask in the comments! |
Contributor
📝 WalkthroughWalkthroughThis PR adds configurable file-preview scrolling, including line, bulk, top, and bottom navigation. It updates preview rendering, file slicing, key handling, help text, and config validation to support the new behavior. ChangesPreview scrolling feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/ui/preview/render_utils.go (1)
16-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat EOF as no more content
lineCount >= previewLineis also true whenbatreturns a full page that ends exactly at EOF, so the last page of files whose length is a multiple of the preview height still leaves scrolling enabled. Peek one line past the window, likeReadFileContent, to distinguish “full page” from “full page + more”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/internal/ui/preview/render_utils.go` around lines 16 - 58, The paging logic in getBatSyntaxHighlightedContent currently treats any full page as having more content, even when bat has reached EOF. Update the bat invocation to peek one line past the requested window, mirroring ReadFileContent, so you can distinguish a full page from “full page plus more.” Then compute hasMore from that extra line instead of comparing lineCount to previewLine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/internal/key_function.go`:
- Around line 151-159: `shouldHandlePreviewScrollKeys` is missing the search
overlay focus check, so preview scroll hotkeys can still intercept input while
the search bar is active. Update `model.shouldHandlePreviewScrollKeys` to also
require that `focusPanel.SearchBar.Focused()` is false, so `tryPreviewScrollKey`
won’t consume search keystrokes. This will correctly gate the existing
`updateComponentState` search-bar path and `handleKeyInput` interception without
further changes in `model.go`.
- Around line 228-241: The scroll-to-bottom handler in handlePreviewScrollBottom
is doing a full RefreshPreviewScroll on every ScrollBulkDown iteration, which
causes repeated expensive renders inside a single Update path. Change the loop
so it only advances the preview state through bulk scrolling, then trigger
RefreshPreviewScroll once after the loop finishes, using the final position. If
CanScrollDown must be re-evaluated from a full render, add a cheaper peek/check
in model or fileModel so handlePreviewScrollBottom can preserve correctness
without calling RefreshPreviewScroll repeatedly.
In `@src/internal/ui/preview/model_utils.go`:
- Around line 25-30: Update the scroll-reset behavior in Model so the invariant
enforced by SetLocation is also applied by UpdateRenderedContent/setContent.
Right now SetLocation resets scroll only on location changes, but setContent
assigns m.location directly and can bypass that check, leaving a stale
scrollOffset for future callers. Refactor setContent to reuse SetLocation or
otherwise compare the incoming location before assigning, so Model.resetScroll
is triggered consistently whenever the location changes.
---
Outside diff comments:
In `@src/internal/ui/preview/render_utils.go`:
- Around line 16-58: The paging logic in getBatSyntaxHighlightedContent
currently treats any full page as having more content, even when bat has reached
EOF. Update the bat invocation to peek one line past the requested window,
mirroring ReadFileContent, so you can distinguish a full page from “full page
plus more.” Then compute hasMore from that extra line instead of comparing
lineCount to previewLine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48fb7eb5-3c96-40dd-a083-674e984c0359
📒 Files selected for processing (17)
src/internal/common/config_type.gosrc/internal/common/load_config.gosrc/internal/key_function.gosrc/internal/model.gosrc/internal/ui/filemodel/preview_navigation.gosrc/internal/ui/helpmenu/data.gosrc/internal/ui/preview/model.gosrc/internal/ui/preview/model_utils.gosrc/internal/ui/preview/navigation.gosrc/internal/ui/preview/navigation_test.gosrc/internal/ui/preview/render.gosrc/internal/ui/preview/render_utils.gosrc/pkg/utils/file_utils.gosrc/pkg/utils/file_utils_test.gosrc/superfile_config/config.tomlsrc/superfile_config/hotkeys.tomlsrc/superfile_config/vimHotkeys.toml
Comment on lines
25
to
30
| func (m *Model) SetLocation(location string) { | ||
| if m.location != location { | ||
| m.resetScroll() | ||
| } | ||
| m.location = location | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
UpdateRenderedContent bypasses the new scroll-reset invariant.
SetLocation resets scroll only when the location changes, but UpdateRenderedContent → setContent sets m.location unconditionally without that check. Currently safe because RefreshPreviewScroll always passes the already-focused item's location, but any future caller passing a different location would silently retain a stale scrollOffset, undoing the fix TestPreviewScrollResetsOnNewLocation validates.
♻️ Suggested consolidation
func (m *Model) UpdateRenderedContent(content string, width int, height int, location string) {
- m.setContent(content, width, height, location)
+ if m.location != location {
+ m.resetScroll()
+ }
+ m.setContent(content, width, height, location)
}Also applies to: 46-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/internal/ui/preview/model_utils.go` around lines 25 - 30, Update the
scroll-reset behavior in Model so the invariant enforced by SetLocation is also
applied by UpdateRenderedContent/setContent. Right now SetLocation resets scroll
only on location changes, but setContent assigns m.location directly and can
bypass that check, leaving a stale scrollOffset for future callers. Refactor
setContent to reuse SetLocation or otherwise compare the incoming location
before assigning, so Model.resetScroll is triggered consistently whenever the
location changes.
- Gate preview scroll hotkeys when the search bar is focused - Scroll to bottom with a single render via ScrollBottom sentinel - Reset scroll offset when setContent receives a new location - Peek one extra bat line to detect EOF correctly Co-authored-by: Kristian Holme <KristianHolme@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/internal/ui/preview/render_utils.go (1)
67-84: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded full-file scan with no timeout for line counting.
countFileLinesscans the entire file line-by-line with no size cap orcontexttimeout, unlike the siblingbatinvocation which is bounded bycommon.DefaultPreviewTimeout. For very large files this can block the render call (triggered by scroll-to-bottom) for a noticeable amount of time with no way to bail out.Consider bounding this (e.g. a max-lines cap, or reading file size and estimating/streaming with a timeout) to keep scroll-to-bottom responsive on huge files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/internal/ui/preview/render_utils.go` around lines 67 - 84, countFileLines currently does an unbounded full-file scan, so update this helper in render_utils.go to avoid blocking preview rendering on huge files. Add a bound comparable to the existing preview timeout behavior used elsewhere (for example by threading through a context, enforcing a max-line cap, or short-circuiting based on file size/estimated work) and ensure callers can bail out when the limit is hit. Keep the fix localized around countFileLines and its preview scroll-to-bottom usage so line counting remains responsive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/internal/ui/preview/render_utils.go`:
- Around line 67-84: countFileLines currently does an unbounded full-file scan,
so update this helper in render_utils.go to avoid blocking preview rendering on
huge files. Add a bound comparable to the existing preview timeout behavior used
elsewhere (for example by threading through a context, enforcing a max-line cap,
or short-circuiting based on file size/estimated work) and ensure callers can
bail out when the limit is hit. Keep the fix localized around countFileLines and
its preview scroll-to-bottom usage so line counting remains responsive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14a179fd-d2f9-49be-9e56-e8772ad19423
📒 Files selected for processing (6)
src/internal/key_function.gosrc/internal/ui/preview/model_utils.gosrc/internal/ui/preview/navigation.gosrc/internal/ui/preview/navigation_test.gosrc/internal/ui/preview/render.gosrc/internal/ui/preview/render_utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- src/internal/ui/preview/model_utils.go
- src/internal/ui/preview/navigation.go
- src/internal/ui/preview/render.go
Scroll-to-bottom line counting no longer scans huge files without limit. Incomplete counts clamp to the best-known offset within the same 500ms budget used by bat preview rendering. Co-authored-by: Kristian Holme <KristianHolme@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/ui/preview/render_utils.go (1)
75-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the partial line count when
bufio.ScannerhitsErrTooLong
countFileLinesBeforereturns0onscanner.Err(), so a single oversized line makesnormalizeScrollOffsetForTextreset scroll instead of falling back to the lines already counted. Returning the partial count (and/or raising the scanner buffer) keeps scroll-to-bottom working for minified or single-line files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/internal/ui/preview/render_utils.go` around lines 75 - 95, The issue is in countFileLinesBefore: when bufio.Scanner returns an error such as ErrTooLong, it currently drops the accumulated lineCount and returns 0, which breaks the fallback behavior in normalizeScrollOffsetForText. Update countFileLinesBefore to preserve and return the partial lineCount on scanner errors (especially ErrTooLong), and consider increasing the scanner buffer if needed so oversized or single-line files still allow scroll-to-bottom handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/internal/ui/preview/render_utils.go`:
- Around line 75-95: The issue is in countFileLinesBefore: when bufio.Scanner
returns an error such as ErrTooLong, it currently drops the accumulated
lineCount and returns 0, which breaks the fallback behavior in
normalizeScrollOffsetForText. Update countFileLinesBefore to preserve and return
the partial lineCount on scanner errors (especially ErrTooLong), and consider
increasing the scanner buffer if needed so oversized or single-line files still
allow scroll-to-bottom handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b37e575-8210-47be-af3e-16b61d109e01
📒 Files selected for processing (3)
src/internal/ui/preview/navigation.gosrc/internal/ui/preview/render_utils.gosrc/internal/ui/preview/render_utils_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/internal/ui/preview/navigation.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Written with Cursor, and manually tested. Hopefully this can be useful :)
Note that this does not yet enable scrolling preview with mouse wheel.
Description
Adds file preview scrolling in browser mode when the preview panel is open. Long files, directories, and syntax-highlighted content can be scrolled without moving the file-panel cursor.
Scrolling is active only when:
fby default)Scroll position resets when the previewed file or directory changes.
Preview scroll hotkeys use the same matching as the rest of superfile (
msg.String()+slices.Contains). Shifted letters are configured as uppercase (J/K), notshift+j/shift+k, because Bubbletea reports them as"J"/"K".Config (
config.toml)preview_scroll_bulk21= whole page,2= half page,3= one third. Must be1,2, or3.Example:
Hotkeys (
hotkeys.toml)Six new global hotkeys:
shift+uppreview_scroll_line_upshift+downpreview_scroll_line_downctrl+k,ctrl+uppreview_scroll_bulk_upctrl+j,ctrl+downpreview_scroll_bulk_downalt+k,alt+uppreview_scroll_topalt+j,alt+downpreview_scroll_bottomRegular defaults use arrow keys for line scroll (no
J/K), soj/kcontinue to move the file list.Vim hotkeys (
vimHotkeys.toml)Line scroll uses both letter and arrow bindings:
Bulk and jump bindings stay vim-style:
ctrl+k/ctrl+j,alt+k/alt+j.Upgrading
Existing installs need the new fields. Run:
Or merge the new keys from the embedded defaults into
~/.config/superfile/hotkeys.tomland addpreview_scroll_bulktoconfig.toml.Related Issues
closes #806
Screenshots (Optional)
Pre-Submission Checklist
go fmt ./...to format the codegolangci-lint runand fixed any reported issues in this PR (remaining lint findings are pre-existing onmain)Summary by CodeRabbit
Summary by CodeRabbit
New Features
preview_scroll_bulksetting to control bulk step as a fraction of preview height (validated to values 1–3).Bug Fixes
Tests