feat: Separate file panel to one file #1058
Conversation
WalkthroughExported the internal file panel types and many methods (filePanel → FilePanel), renamed fields to public equivalents, moved navigation/rendering/state helpers to exported APIs, removed some internal render helpers, and updated callers and tests to use the new public surface and accessors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant FocusedPanel as FilePanel
participant FS as Filesystem
participant Renderer
User->>Model: key input / wheel / command
activate Model
Model->>FocusedPanel: delegate action (ListUp/ListDown/ConfirmRename/CopyPath/...)
activate FocusedPanel
alt filesystem operation
FocusedPanel->>FS: read / rename / delete / open
FS-->>FocusedPanel: result / error
end
FocusedPanel-->>Model: update state (Cursor, Selected, Element, Location)
deactivate FocusedPanel
Model->>Renderer: render using exported fields (Location, Element, SortOptions)
Renderer-->>User: updated UI
deactivate Model
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Wide, heterogeneous API surface changes across many files (types, methods, rendering removal, tests). Requires checking exported API correctness, call-site updates, and side effects (filesystem/rename/delete flows) plus test alignment. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/internal/test_utils.go (1)
42-47: Prefer reusing the local panel variable for clarity.Tiny readability nit: call ChangeFilePanelMode() on panel rather than re-fetching the focused panel.
Apply this diff:
- m.getFocusedFilePanel().ChangeFilePanelMode() + panel.ChangeFilePanelMode()src/internal/model_msg.go (1)
63-66: Selection reset moved to ResetSelected() — OK.Optional: consider resetting only on successful delete for better UX, but fine to keep as-is if unchanged behavior is intended.
src/internal/handle_panel_movement.go (1)
33-77: Use selectedItem consistently within enterPanel.Minor consistency nit: after fetching selectedItem, prefer using selectedItem.location in the chooser/write and open-command paths to avoid relying on panel.element[cursor] again.
- chooserErr := m.chooserFileWriteAndQuit(panel.element[panel.cursor].location) + chooserErr := m.chooserFileWriteAndQuit(selectedItem.location)And in executeOpenCommand():
- cmd := exec.Command(dllpath, dllfile, panel.element[panel.cursor].location) + cmd := exec.Command(dllpath, dllfile, selectedItem.location) @@ - cmd := exec.Command(openCommand, panel.element[panel.cursor].location) + cmd := exec.Command(openCommand, selectedItem.location)src/internal/file_panel.go (3)
47-89: Path resolution improvement with noted cache invalidation concern.The addition of absolute path resolution via
utils.ResolveAbsPathis a good improvement. The TODO comment correctly identifies a potential cache invalidation issue wheredirectoryRecordscould become stale if files are modified externally.The TODO comment on lines 70-73 identifies a valid cache invalidation concern. Would you like me to create a GitHub issue to track the implementation of cache validation logic for
directoryRecords?
195-204: Performance concern acknowledged with TODO.The TODO comment correctly identifies the O(n) performance issue with the current slice-based selection implementation. Converting to a set would indeed provide O(1) operations.
The TODO comment on lines 199-200 identifies a performance improvement opportunity. Would you like me to create a GitHub issue to track converting
panel.selectedfrom[]stringtomap[string]struct{}for constant-time operations?
236-243: Consider simplifying the icon spacing logic.The conditional logic for icon spacing could be simplified by setting
icon.Spacebased on theNerdfontconfiguration at initialization.The TODO comment on lines 240-241 suggests a valid simplification. Consider initializing
icon.Spacebased on theNerdfontconfiguration to eliminate this conditional:- if common.Config.Nerdfont { - sortLabel = sortIcon + icon.Space + sortLabel - modeLabel = modeIcon + icon.Space + modeLabel - } else { - // TODO : Figure out if we can set icon.Space to " " if nerdfont is false - // That would simplify code - sortLabel = sortIcon + " " + sortLabel - } + sortLabel = sortIcon + icon.Space + sortLabel + modeLabel = modeIcon + icon.Space + modeLabel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/internal/file_panel.go(3 hunks)src/internal/handle_file_operation_test.go(2 hunks)src/internal/handle_file_operations.go(2 hunks)src/internal/handle_panel_movement.go(2 hunks)src/internal/handle_panel_up_down.go(0 hunks)src/internal/handle_panel_up_down_test.go(1 hunks)src/internal/key_function.go(3 hunks)src/internal/model.go(3 hunks)src/internal/model_msg.go(3 hunks)src/internal/model_render.go(0 hunks)src/internal/test_utils.go(1 hunks)src/internal/wheel_function.go(2 hunks)
💤 Files with no reviewable changes (2)
- src/internal/handle_panel_up_down.go
- src/internal/model_render.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/key_function.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/handle_panel_up_down_test.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/file_panel.go
🧬 Code graph analysis (6)
src/internal/key_function.go (1)
src/internal/handle_panel_up_down.go (5)
listUp(6-23)pgUp(41-65)pgDown(67-87)itemSelectUp(93-96)itemSelectDown(99-102)
src/internal/test_utils.go (3)
src/internal/type.go (1)
filePanels(146-152)src/internal/handle_panel_navigation.go (2)
m(147-156)m(135-144)src/internal/handle_modal.go (2)
m(88-91)m(99-103)
src/internal/handle_panel_up_down_test.go (2)
src/internal/handle_panel_up_down.go (2)
listUp(6-23)listDown(26-39)src/internal/ui/metadata/model_test.go (1)
TestUpDown(9-67)
src/internal/handle_panel_movement.go (1)
src/internal/model_navigation_test.go (1)
TestFilePanelNavigation(15-149)
src/internal/wheel_function.go (1)
src/internal/handle_panel_up_down.go (3)
listUp(6-23)pgUp(41-65)pgDown(67-87)
src/internal/file_panel.go (8)
src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/ui/rendering/renderer.go (1)
Renderer(14-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)src/internal/common/predefined_variable.go (2)
FilePanelTopDirectoryIcon(42-42)FilePanelNoneText(43-43)src/internal/common/style.go (3)
FilePanelTopPathStyle(37-37)FilePanelBGColor(77-77)FilePanelCursorStyle(30-30)src/internal/common/default_config.go (1)
Config(11-11)src/config/icon/icon.go (6)
Space(10-10)Cursor(31-31)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/model_render.go (1)
panel(110-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test (ubuntu-latest)
🔇 Additional comments (21)
src/internal/handle_file_operations.go (2)
102-108: Good switch to exported API.Using panel.GetSelectedItem().location here matches the new public surface and preserves prior behavior guarded by the empty-panel check.
338-346: Consistent use of GetSelectedItem().Refactor aligns with the exported API; the preceding empty-panel check keeps this safe.
src/internal/wheel_function.go (1)
22-23: Approve — ListUp/ListDown verifiedRepo-wide search: no remaining lowercase listUp/listDown usages; exported ListUp/ListDown are defined and used consistently (src/internal/file_panel.go, src/internal/ui/sidebar/navigation.go, src/internal/ui/processbar/model_navigation.go, src/internal/ui/metadata/model.go; invoked from src/internal/key_function.go and src/internal/wheel_function.go).
src/internal/handle_panel_up_down_test.go (1)
118-122: Tests aligned with exported API.Switch to ListDown/ListUp mirrors the production rename without changing semantics.
src/internal/handle_file_operation_test.go (2)
111-113: Test setup uses ChangeFilePanelMode() — consistent with new API.No behavior change; just naming consistency.
133-135: Access via GetSelectedItem() reads well.Asserting on GetSelectedItem().location matches production changes.
src/internal/model.go (3)
141-167: Preview path selection via GetSelectedItem() is correct.Capturing location, setting “Loading…”, and deferring content update remains safe thanks to the location check in FilePreviewUpdateMsg.
174-207: Metadata path selection via GetSelectedItem() is correct.Keeps existing caching/blank logic intact; only the accessor changed.
510-519: Delegation to panel.UpdateCurrentFilePanelDir().Good encapsulation; zoxide tracking on success preserved.
src/internal/model_msg.go (2)
136-141: Pointer accessor for metadata targeting is appropriate.Nil-guard and path match prevent stale updates.
188-196: Pointer accessor for preview targeting is appropriate.Prevents stale preview content on rapid navigation.
src/internal/handle_panel_movement.go (1)
18-23: Parent directory now uses exported method.Matches the API shift; error handling unchanged.
src/internal/key_function.go (4)
32-32: LGTM! Method visibility changes align with public API exposure.The changes from unexported to exported methods (
listUp→ListUp,listDown→ListDown) are consistent with the file panel separation objective, allowing these navigation methods to be called from the refactored file panel module.Also applies to: 45-45
49-49: LGTM! Pagination methods properly exported.The page navigation methods (
pgUp→PgUp,pgDown→PgDown) follow the same pattern of making the API public for external access.Also applies to: 52-52
150-150: LGTM! Select mode operations properly exported.The select mode operations (
singleItemSelect→SingleItemSelect,itemSelectUp→ItemSelectUp,itemSelectDown→ItemSelectDown) are correctly updated to use the public API.Also applies to: 152-152, 154-154
55-55: No action needed — helper returns pointer into slicegetFocusedFilePanel() returns &m.fileModel.filePanels[m.filePanelFocusIndex], so m.getFocusedFilePanel().ChangeFilePanelMode() is equivalent to direct slice-element access; leave as-is.
src/internal/file_panel.go (5)
8-12: Well-organized imports following the separation pattern.The imports correctly reflect the modular architecture with UI components (
icon,common,ui,rendering) properly separated. This aligns with the PR objective of separating the file panel into its own module.Based on the learnings, you prefer maintaining focus on primary refactoring objectives while deferring other improvements.
91-93: Clean delegation pattern for parent directory navigation.The
ParentDirectorymethod elegantly delegates toUpdateCurrentFilePanelDir(".."), maintaining DRY principles.
97-188: Navigation methods correctly migrated from handle_panel_up_down.goThe navigation methods (
ListUp,ListDown,PgUp,PgDown,ItemSelectUp,ItemSelectDown) have been properly moved fromhandle_panel_up_down.goand exported. The logic remains unchanged, ensuring functional consistency.
209-218: Render method properly accepts width parameter.The
Rendermethod now correctly takesfilePanelWidthas a parameter and passes it torenderTopBar, improving the separation of concerns.
332-390: Comprehensive accessor methods for encapsulation.The accessor methods provide a clean public API for the file panel's internal state. This follows good encapsulation practices and allows for future implementation changes without breaking the API.
| // Performance TODO: Remove or cache this if not needed at render time | ||
| // This will unnecessarily slow down rendering. There should be a way to avoid this at render | ||
| _, err := os.ReadDir(panel.element[i].location) | ||
| dirExists := err == nil || panel.element[i].directory | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Performance concern noted for directory existence check.
The TODO comment correctly identifies that checking directory existence during rendering (os.ReadDir) could impact performance. This I/O operation in the render path should be cached or moved to the data update phase.
The performance TODO on lines 278-279 identifies a critical rendering performance issue. The os.ReadDir call on line 280 performs I/O during rendering, which should be avoided. Would you like me to create a GitHub issue to track moving this check to the data update phase or implementing caching?
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 278 to 282, the render path performs
os.ReadDir to check directory existence which causes blocking I/O and slows
rendering; move this check out of the render function into the data update phase
(e.g., when panel.element[i].location is set/changed) or maintain a cached
boolean (dirExists) per element that is updated whenever the element’s location
changes (or on a periodic background refresh), ensuring updates are synchronized
(use a mutex/atomic if render and updater run concurrently) and remove the
os.ReadDir call from the render path.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/internal/model_render.go (1)
101-147: terminalSizeWarn duplication noted previously.*This duplication is already flagged by tooling; fine to defer per scope, but worth consolidating later to a single parametrized helper.
src/internal/file_panel.go (1)
256-293: I/O in render path (dir existence check).Calling
os.ReadDirper row during render is expensive and already called out as a TODO. Move this check to the data update phase or cache the bit per element.Would you like me to draft an issue with options (cache vs. move to updater)?
🧹 Nitpick comments (22)
src/internal/type.go (3)
20-22: Exporting PanelMode inside package internal has limited valueIf the goal is broader reuse, exporting from an internal package won’t help external consumers. Consider deferring any API-facing exposure (and naming) to a non-internal package in a follow-up PR; otherwise keeping these unexported reduces surface area.
Also applies to: 47-51
155-169: Large public struct with many exported, mutable fieldsExposing many fields can weaken invariants (e.g., Cursor/Selected/DirectoryRecords can be mutated freely). If you plan to stabilize this as an API, consider narrowing what’s exported or providing methods instead. For now, a lightweight guard is to ensure zero-value safety (e.g., DirectoryRecords map initialized at construction sites).
type FilePanel struct { - DirectoryRecords map[string]DirectoryRecord + // Initialized at construction to avoid nil-map writes + DirectoryRecords map[string]DirectoryRecord }
193-199: Exported Element with unexported fieldsThis keeps construction/control internal while allowing the type in signatures—reasonable. If any non-internal package needs to read fields later, plan simple getters to avoid future breaking changes.
src/internal/config_function.go (1)
145-151: Minor log key naming nitConsider aligning the key with the new type/name style to avoid confusion. Change only if log consumers aren’t relying on the old key.
- "filePanel_size_bytes", reflect.TypeOf(FilePanel{}).Size(), + "file_panel_size_bytes", reflect.TypeOf(FilePanel{}).Size(),src/internal/model_file_operations_test.go (2)
41-41: Avoid relying on Element[0].name in tests (order-sensitive).Minor: accessing Element[0].name assumes sort/order. If feasible, prefer selecting by name using existing helpers and asserting on the selected item rather than the first index to reduce brittleness.
133-135: Centralize selected item path access in a test helper.Minor: instead of touching unexported .location here, add a small helper in tests to read the selected item’s path. Keeps direct field access in one place.
Example (in test_utils.go):
func getSelectedItemLocation(panel *FilePanel) string { return panel.GetSelectedItem().location }src/internal/model_prompt_test.go (1)
212-219: DRY the HOME/USERPROFILE env selection in tests.You compute the home env var in multiple places. Consider a tiny test helper (in test_utils.go) to reduce duplication.
func userHomeEnvKey() string { if runtime.GOOS == utils.OsWindows { return "USERPROFILE" } return "HOME" }Also applies to: 351-355, 366-374
src/internal/handle_file_operation_test.go (1)
133-135: Test helper for selected-item path (optional).Minor: to avoid sprinkling direct
.locationaccess, add a helper (see earlier suggestion) and use it here.src/internal/handle_panel_up_down_test.go (1)
129-129: Follow-up: add PgUp/PgDown and itemSelect up/down tests.Nice-to-have to round out coverage; can be a separate PR per scope.
Happy to draft the test table if you want.
src/internal/model_render.go (1)
35-38: Harden cursor/render bounds; avoid negative indices.Current guard fixes only “> max.” Add lower-bound checks to prevent negative Cursor/RenderIndex from leaking into Render(). Minimal, localized change below.
- if filePanel.Cursor > len(filePanel.Element)-1 { - filePanel.Cursor = 0 - filePanel.RenderIndex = 0 - } + if filePanel.Cursor < 0 || filePanel.Cursor >= len(filePanel.Element) { + filePanel.Cursor = 0 + } + if filePanel.RenderIndex < 0 || filePanel.RenderIndex >= len(filePanel.Element) { + filePanel.RenderIndex = 0 + }Longer-term: rendering mutating panel state is brittle (see the comment above); consider validating earlier in update-path and keeping render side-effect free in a follow-up.
src/internal/test_utils.go (1)
191-204: Test helpers migrated to FilePanel: LGTMIterating over Element and setting Cursor directly is fine for tests. Optional: also normalize RenderIndex when setting Cursor to ensure visibility in any viewport-dependent tests.
// optionally keep selected visible if panel.RenderIndex > panel.Cursor { panel.RenderIndex = panel.Cursor }Also applies to: 228-233, 235-241
src/internal/key_function.go (2)
32-32: Public API renames look correct; prefer consistent access via getFocusedFilePanel().To reduce repeated indexing and prevent focus-index drift, call methods on m.getFocusedFilePanel() everywhere in this switch.
- m.fileModel.filePanels[m.filePanelFocusIndex].ListUp(m.mainPanelHeight) + m.getFocusedFilePanel().ListUp(m.mainPanelHeight) - m.fileModel.filePanels[m.filePanelFocusIndex].ListDown(m.mainPanelHeight) + m.getFocusedFilePanel().ListDown(m.mainPanelHeight) - m.fileModel.filePanels[m.filePanelFocusIndex].PgUp(m.mainPanelHeight) + m.getFocusedFilePanel().PgUp(m.mainPanelHeight) - m.fileModel.filePanels[m.filePanelFocusIndex].PgDown(m.mainPanelHeight) + m.getFocusedFilePanel().PgDown(m.mainPanelHeight)Also applies to: 45-45, 49-49, 52-52, 55-55
147-147: Select-mode branch: API updates LGTM; minor consistency tweak.Use the focused panel accessor here too.
- if m.getFocusedFilePanel().PanelMode == selectMode { + if m.getFocusedFilePanel().PanelMode == selectMode { switch { case slices.Contains(common.Hotkeys.Confirm, msg): - m.fileModel.filePanels[m.filePanelFocusIndex].SingleItemSelect() + m.getFocusedFilePanel().SingleItemSelect() case slices.Contains(common.Hotkeys.FilePanelSelectModeItemsSelectUp, msg): - m.fileModel.filePanels[m.filePanelFocusIndex].ItemSelectUp(m.mainPanelHeight) + m.getFocusedFilePanel().ItemSelectUp(m.mainPanelHeight) case slices.Contains(common.Hotkeys.FilePanelSelectModeItemsSelectDown, msg): - m.fileModel.filePanels[m.filePanelFocusIndex].ItemSelectDown(m.mainPanelHeight) + m.getFocusedFilePanel().ItemSelectDown(m.mainPanelHeight)Also applies to: 150-150, 152-152, 154-154
src/internal/handle_panel_movement.go (3)
70-71: Use the already-fetched selectedItem to avoid cursor drift between checks and actions.Keeps behavior consistent if future changes mutate Cursor/Element.
- chooserErr := m.chooserFileWriteAndQuit(panel.Element[panel.Cursor].location) + chooserErr := m.chooserFileWriteAndQuit(selectedItem.location)- cmd := exec.Command(dllpath, dllfile, panel.Element[panel.Cursor].location) + cmd := exec.Command(dllpath, dllfile, selectedItem.location)- cmd := exec.Command(openCommand, panel.Element[panel.Cursor].location) + cmd := exec.Command(openCommand, selectedItem.location)Also applies to: 90-90, 99-99
127-129: Avoid duplicate selections on repeated “Select all”.Reset the slice before appending.
func (m *model) selectAllItem() { panel := &m.fileModel.filePanels[m.filePanelFocusIndex] + // Reset to avoid duplicates on repeated triggers + panel.Selected = panel.Selected[:0] for _, item := range panel.Element { panel.Selected = append(panel.Selected, item.location) } }
164-173: Clamp search bar width to non-negative.Prevents accidental negative widths when panel width is very small.
- panel.SearchBar.Width = m.fileModel.width - 4 + w := m.fileModel.width - 4 + if w < 0 { + w = 0 + } + panel.SearchBar.Width = wsrc/internal/type_utils.go (1)
92-101: Improve unknown PanelMode String() for debugging (optional).Return the numeric value to aid logs.
- default: - return invalidTypeString + default: + return fmt.Sprintf("%s(%d)", invalidTypeString, p)src/internal/model.go (1)
691-730: Remove redundant assignment to local copy in panels loop.Inside
for i, filePanel := range m.fileModel.filePanels, assigning tofilePanel.Elementupdates only the loop copy. You already assign tom.fileModel.filePanels[i].Elementbelow, so the first write is redundant and can be dropped.Apply:
- // Update file panel list - filePanel.Element = fileElement m.fileModel.filePanels[i].Element = fileElement m.fileModel.filePanels[i].LastTimeGetElement = nowTimesrc/internal/file_panel.go (4)
97-114: Flatten ListUp to reduce nesting (CodeScene “Bumpy Road Ahead”).Same behavior with fewer branches; improves readability and matches your general style.
Apply:
func (panel *FilePanel) ListUp(mainPanelHeight int) { - if len(panel.Element) == 0 { - return - } - if panel.Cursor > 0 { - panel.Cursor-- - if panel.Cursor < panel.RenderIndex { - panel.RenderIndex-- - } - } else { - if len(panel.Element) > panelElementHeight(mainPanelHeight) { - panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) - panel.Cursor = len(panel.Element) - 1 - } else { - panel.Cursor = len(panel.Element) - 1 - } - } + if len(panel.Element) == 0 { + return + } + if panel.Cursor == 0 { + if len(panel.Element) > panelElementHeight(mainPanelHeight) { + panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) + } + panel.Cursor = len(panel.Element) - 1 + return + } + panel.Cursor-- + if panel.Cursor < panel.RenderIndex { + panel.RenderIndex-- + } }
131-177: PgUp/PgDown: consider clamping RenderIndex to visible range.Small readability refactor (early returns) would also address CodeScene’s nesting warning. Additionally, after computing
RenderIndex = panel.Cursor - panCenter, clamp to[0, max(0, len-Height)]to avoid off-viewport values.
191-205: Selection is O(n) per toggle — keep as future perf task.Your comment notes the O(n^2) path during render. Consider backing
Selectedwith amap[string]struct{}and derive the slice when needed. Given PR scope, fine to defer.I can open a follow-up issue with a minimal migration plan if useful.
296-309: Guard against invalid sort option index.
opts.options[opts.selected]will panic ifselectedis out of range or options is empty. If the invariant guarantees range safety, ignore; otherwise clamp or default to first option.Example:
- selected := opts.options[opts.selected] + if len(opts.options) == 0 || opts.selected < 0 || opts.selected >= len(opts.options) { + return "", icon.SortAsc + } + selected := opts.options[opts.selected]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/internal/config_function.go(1 hunks)src/internal/file_panel.go(2 hunks)src/internal/function.go(4 hunks)src/internal/function_test.go(1 hunks)src/internal/handle_file_operation_test.go(2 hunks)src/internal/handle_file_operations.go(15 hunks)src/internal/handle_modal.go(3 hunks)src/internal/handle_panel_movement.go(7 hunks)src/internal/handle_panel_navigation.go(5 hunks)src/internal/handle_panel_up_down_test.go(9 hunks)src/internal/key_function.go(3 hunks)src/internal/model.go(13 hunks)src/internal/model_file_operations_test.go(3 hunks)src/internal/model_navigation_test.go(2 hunks)src/internal/model_prompt_test.go(12 hunks)src/internal/model_render.go(3 hunks)src/internal/model_test.go(3 hunks)src/internal/test_utils.go(4 hunks)src/internal/type.go(4 hunks)src/internal/type_utils.go(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/handle_panel_up_down_test.gosrc/internal/file_panel.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/handle_panel_up_down_test.gosrc/internal/model_render.gosrc/internal/model.gosrc/internal/file_panel.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: deleteItemWarn() in yorukot/superfile only displays a warning modal via channel messaging and does not perform actual file deletion. The actual deletion happens through getDeleteCmd() which returns a tea.Cmd. The architectural pattern separates UI modal operations (using channels/goroutines) from file I/O operations (using tea.Cmd), which is intentional and appropriate.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/function.go:211-211
Timestamp: 2025-03-29T13:20:46.467Z
Learning: Empty panel.element checks should be added at the beginning of functions that access panel.element to prevent crashes when a panel has no elements, similar to using `if len(panel.element) == 0 { return }`.
Applied to files:
src/internal/handle_file_operations.gosrc/internal/model_render.go
📚 Learning: 2025-03-29T10:02:04.260Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/model_render.go:653-654
Timestamp: 2025-03-29T10:02:04.260Z
Learning: A panel validation function should be implemented to ensure panel.cursor doesn't become negative or go out of bounds, but this should be addressed in a separate PR - not as part of the PR fixing crashes when opening files with the editor on an empty panel.
Applied to files:
src/internal/model_render.go
🧬 Code graph analysis (19)
src/internal/config_function.go (1)
src/internal/type.go (1)
FilePanel(155-169)
src/internal/model_file_operations_test.go (1)
src/internal/type.go (1)
Element(193-198)
src/internal/model_test.go (1)
src/internal/common/type.go (1)
String(36-38)
src/internal/key_function.go (2)
src/internal/type.go (1)
PanelMode(21-21)src/internal/handle_panel_up_down.go (3)
pgUp(41-65)listUp(6-23)pgDown(67-87)
src/internal/function.go (1)
src/internal/type.go (2)
Element(193-198)FilePanel(155-169)
src/internal/model_navigation_test.go (2)
src/internal/test_utils.go (1)
TeaUpdateWithErrCheck(65-68)src/internal/utils/tea_utils.go (1)
TeaRuneKeyMsg(5-10)
src/internal/type_utils.go (2)
src/internal/type.go (4)
FilePanel(155-169)SortOptionsModel(172-178)PanelMode(21-21)DirectoryRecord(187-190)src/internal/common/style_function.go (1)
GenerateSearchBar(235-247)
src/internal/model_prompt_test.go (3)
src/internal/test_utils.go (1)
TeaUpdateWithErrCheck(65-68)src/internal/utils/tea_utils.go (1)
TeaRuneKeyMsg(5-10)src/internal/common/type.go (1)
String(36-38)
src/internal/handle_file_operation_test.go (1)
src/internal/type.go (2)
Element(193-198)PanelMode(21-21)
src/internal/handle_panel_navigation.go (2)
src/internal/ui/sidebar/directory_utils.go (1)
TogglePinnedDirectory(121-151)src/internal/type.go (3)
FilePanel(155-169)PanelMode(21-21)DirectoryRecord(187-190)
src/internal/handle_panel_up_down_test.go (2)
src/internal/type.go (2)
FilePanel(155-169)Element(193-198)src/internal/handle_panel_up_down.go (3)
listUp(6-23)pgDown(67-87)pgUp(41-65)
src/internal/handle_panel_movement.go (1)
src/internal/type.go (1)
Element(193-198)
src/internal/test_utils.go (1)
src/internal/type.go (3)
PanelMode(21-21)FilePanel(155-169)Element(193-198)
src/internal/function_test.go (1)
src/internal/type.go (1)
Element(193-198)
src/internal/handle_file_operations.go (3)
src/internal/type.go (2)
Element(193-198)PanelMode(21-21)src/internal/common/style_function.go (1)
GenerateRenameTextInput(276-292)src/config/fixed_variable.go (1)
ChooserFile(71-71)
src/internal/handle_modal.go (1)
src/internal/type.go (1)
Element(193-198)
src/internal/model_render.go (3)
src/internal/type.go (1)
Element(193-198)src/internal/common/style.go (2)
FilePanelCursorStyle(30-30)ModalStyle(20-20)src/internal/common/style_function.go (2)
GenerateFooterBorder(321-328)SortOptionsModalBorderStyle(159-171)
src/internal/model.go (2)
src/internal/type.go (2)
Element(193-198)PanelMode(21-21)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)
src/internal/file_panel.go (8)
src/internal/type.go (4)
FilePanel(155-169)Element(193-198)PanelMode(21-21)DirectoryRecord(187-190)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)src/internal/common/predefined_variable.go (2)
FilePanelTopDirectoryIcon(42-42)FilePanelNoneText(43-43)src/internal/common/style.go (3)
FilePanelTopPathStyle(37-37)FilePanelBGColor(77-77)FilePanelCursorStyle(30-30)src/internal/common/default_config.go (1)
Config(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test (ubuntu-latest)
🔇 Additional comments (49)
src/internal/type.go (2)
146-152: Slice element type switched to FilePanel: looks goodNice alignment with the new exported type; no functional impact expected.
172-178: AI summary mismatch: SortOptionsModel.data remains unexportedSummary says the field was renamed to Data; code still has
data sortOptionsModelData. Please ignore the summary at this point or confirm intended visibility.src/internal/function_test.go (1)
187-193: Type expectation updated to []Element: LGTMMatches the refactor; test semantics unchanged.
src/internal/config_function.go (1)
145-151: Reflecting FilePanel size instead of filePanel: LGTMAccurately tracks the renamed type; no behavior change.
src/internal/model_test.go (2)
96-96: Public Location field usage: LGTMAssertion updated to the exported Location; consistent with the refactor.
279-306: Zoxide assertions aligned to Location: LGTMAll path assertions now reference the exported Location; matches the public API.
src/internal/model_file_operations_test.go (2)
106-113: Switch to exported FilePanel API: LGTMUsing Element/Cursor/PanelMode and ChangeFilePanelMode/Selected aligns with the new public API.
148-148: Rename/Rename flow and Element logging: LGTMRename.SetValue and Element references updated correctly; behavior unchanged.
Also applies to: 156-156, 182-182
src/internal/model_prompt_test.go (1)
139-139: Prompt tests updated to Location: LGTMAll assertions now use the exported Location field; matches the refactor.
Also applies to: 157-157, 163-163, 167-167, 173-173, 177-177, 183-183, 197-197, 201-201, 209-209, 219-219, 227-227, 285-285, 295-295, 305-305, 315-315, 327-327, 337-337, 355-355, 373-373
src/internal/model_navigation_test.go (1)
125-128: Navigation test migrated to exported fields: LGTMCursor, RenderIndex, SearchBar, and Location references align with the new API.
Also applies to: 132-132, 135-136, 145-147
src/internal/handle_file_operation_test.go (1)
106-113: Use of exported FilePanel fields/methods: LGTMElement/Cursor/PanelMode/Selected and ChangeFilePanelMode usage looks correct.
src/internal/handle_panel_up_down_test.go (1)
12-24: Up/Down movement tests against public FilePanel: LGTMThe scenarios cover wrap, within-range moves, and empty panel; expected Cursor/RenderIndex match the intended semantics.
Also applies to: 20-24, 32-41, 44-53, 56-65, 69-77, 81-89, 92-101, 104-113, 119-125
src/internal/model_render.go (2)
21-24: sidebarRender Location usage: LGTMPassing the exported Location to the sidebar renderer is consistent with the refactor.
304-318: Sort options rendering against exported SortOptions: LGTMCursor/data/options/width/height usage migrated correctly; footer count string is computed as before.
src/internal/test_utils.go (1)
44-47: Select mode switch via ChangeFilePanelMode: LGTMAssertions against PanelMode/Selected align with the new public API.
src/internal/function.go (4)
65-69: LGTM! Proper nil handling added.Good addition of
return nilon error, preventing potential panics from returning an uninitialized slice.
87-91: LGTM! Consistent error handling.The nil return on error is consistent with the pattern in
returnDirElement.
125-138: LGTM! Clean migration to public Element type.The function correctly uses the exported
Elementtype with proper field initialization. The preallocation optimization is preserved.
328-329: LGTM! Proper return type migration.The function signature correctly returns the exported
*FilePaneltype, aligning with the public API refactor.src/internal/handle_panel_navigation.go (3)
19-20: LGTM! Correct field name migration.Using the exported
Locationfield aligns with the public API changes.
43-50: LGTM! Proper FilePanel initialization with exported fields.The initialization correctly uses the exported
FilePaneltype and its public fields (Location,SortOptions,PanelMode,DirectoryRecords,SearchBar).
71-72: LGTM! Consistent SearchBar field access.All search bar width updates correctly use the exported
SearchBarfield.Also applies to: 106-107, 130-131
src/internal/handle_file_operations.go (13)
33-34: LGTM! Correct typingModal field migration.Using the exported
Locationfield intypingModal.
44-50: LGTM! Proper exported field access with safety check.Good defensive programming with the empty check before accessing
Element,Cursor, andLocationfields. This aligns with the learnings about preventing crashes when panels have no elements.
80-94: LGTM! Complete field migration in rename logic.All field accesses correctly use the exported names:
Element,Cursor,Renaming,Rename. The safety check for empty elements is properly maintained.
98-109: LGTM! Proper use of public API methods and fields.Correctly uses
GetSelectedItem()public method and exported fieldsElement,PanelMode,Selected,Location.
160-161: LGTM! Comprehensive mode and selection checks.Properly handles both select mode and browser mode with appropriate empty checks using exported fields.
187-192: LGTM! Safe single item copy with exported fields.Maintains the empty element check and correctly uses exported
ElementandCursorfields.
199-204: LGTM! Safe multiple item copy.Properly checks for empty selection and uses the exported
Selectedfield.
218-218: LGTM! Correct use of public Location field.Uses the exported
Locationfield from the focused file panel.
339-343: LGTM! Safe extraction with public API.Properly checks for empty elements and uses
GetSelectedItem()public method.
380-392: LGTM! Comprehensive compression logic with exported fields.Correctly handles both single and multiple file selection using exported
Element,Cursor,Selected, andLocationfields.Also applies to: 403-403
427-432: LGTM! Safe file opening with exported fields.Maintains empty element check and correctly uses exported
ElementandCursorfields.Also applies to: 459-459
471-471: LGTM! Directory operations with public Location field.Both chooser file write and directory editor opening correctly use the exported
Locationfield.Also applies to: 496-496
509-513: LGTM! Path copying with exported fields.Both path copy operations correctly use exported fields (
Element,Cursor,Location) with appropriate empty checks.Also applies to: 521-521
src/internal/handle_panel_movement.go (1)
19-19: Method rename to ParentDirectory(): LGTM.Matches the exported API.
src/internal/type_utils.go (2)
39-47: Slice factory now returns []FilePanel: LGTM.Allocation and focus initialization look good.
49-73: defaultFilePanel fields updated to exported names: LGTM.Initial PanelMode, SortOptions, and SearchBar construction align with the new API.
src/internal/handle_modal.go (5)
90-97: Sort options: state transitions look consistent after migration.Open/cancel/confirm logic mirrors previous behavior with exported
SortOptionsfields. LGTM.Also applies to: 100-103
106-113: Wrap-around list navigation: behavior preserved.Cursor wrapping and bounds look correct with
data.optionslength. LGTM.Also applies to: 116-123
125-128: Reverse sort toggle migrated cleanly.Boolean flip on
data.reversedis straightforward. LGTM.
133-135: Search bar lifecycle is correct.Blur and clear on cancel, blur on confirm—matches prior UX. LGTM.
Also applies to: 140-141
69-86: Prevent out-of-bounds panic and validate rename target in confirmRename.Validate panel.Cursor before indexing panel.Element and reuse checkFileNameValidity(panel.Rename.Value()) before calling os.Rename; decide desired overwrite semantics (prevent overwrite if newPath exists).
File: src/internal/handle_modal.go (lines 69-86)
func (m *model) confirmRename() { panel := &m.fileModel.filePanels[m.filePanelFocusIndex] // Although we dont expect this to happen based on our current flow // Just adding it here to be safe - if len(panel.Element) == 0 { + if len(panel.Element) == 0 { slog.Error("confirmRename called on empty panel") return } - oldPath := panel.Element[panel.Cursor].location + if panel.Cursor < 0 || panel.Cursor >= len(panel.Element) { + slog.Error("confirmRename called with invalid cursor", + "cursor", panel.Cursor, "len", len(panel.Element)) + return + } + + if err := checkFileNameValidity(panel.Rename.Value()); err != nil { + slog.Error("Invalid rename target", "error", err) + return + } + + oldPath := panel.Element[panel.Cursor].location newPath := filepath.Join(panel.Location, panel.Rename.Value()) // Rename the file err := os.Rename(oldPath, newPath) if err != nil { slog.Error("Error while confirmRename during rename", "error", err) // Dont return. We have to also reset the panel and model information } m.fileModel.renaming = false panel.Rename.Blur() panel.Renaming = false }src/internal/model.go (6)
144-152: Preview/metadata: safe guards for empty selection.Early exits on empty
Elementavoid unnecessary work and UI artifacts. UsingGetSelectedItem()centralizes selection logic. LGTM.Also applies to: 177-183
254-255: SearchBar width propagation is fine.Width calculation aligns with panel width; consistent with surrounding size logic. LGTM.
368-375: Input routing with exported fields looks correct.Focus detection via
SearchBar.Focused()andSortOptions.openguards the right paths. LGTM.
418-421: Rename/search input Update wiring is correct.Stateful updates feed back the
textinput.Modelas required by Bubbles. LGTM.
437-444: Cursor lower-bound clamp only—verify upper bound invariant.We clamp negative cursors to 0, but don’t guard
Cursor >= len(Element). Given wide use ofCursorfor indexing (e.g., rename), confirm the invariant thatCursoris always kept within bounds elsewhere, or add a symmetric upper clamp here.
628-632: Sort options overlay dimensions—verify width/height are kept current.We read
panel.SortOptions.width/heighthere. Ensure they’re updated whenever the terminal size or panel width changes; otherwise the overlay may misplace or mis-size.
| panel.DirectoryRecords[panel.Location] = DirectoryRecord{ | ||
| directoryCursor: panel.Cursor, | ||
| directoryRender: panel.RenderIndex, | ||
| } |
There was a problem hiding this comment.
Possible panic: write to nil map DirectoryRecords.
panel.DirectoryRecords[panel.Location] = ... will panic if DirectoryRecords wasn’t initialized. Initialize on first use.
Apply:
func (panel *FilePanel) UpdateCurrentFilePanelDir(path string) error {
slog.Debug("updateCurrentFilePanelDir", "panel.location", panel.Location, "path", path)
path = utils.ResolveAbsPath(panel.Location, path)
if path == panel.Location {
return nil
}
// NOTE: This could be a configurable feature
// Update the cursor and render status in case we switch back to this.
+ if panel.DirectoryRecords == nil {
+ panel.DirectoryRecords = make(map[string]DirectoryRecord)
+ }
panel.DirectoryRecords[panel.Location] = DirectoryRecord{
directoryCursor: panel.Cursor,
directoryRender: panel.RenderIndex,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| panel.DirectoryRecords[panel.Location] = DirectoryRecord{ | |
| directoryCursor: panel.Cursor, | |
| directoryRender: panel.RenderIndex, | |
| } | |
| if panel.DirectoryRecords == nil { | |
| panel.DirectoryRecords = make(map[string]DirectoryRecord) | |
| } | |
| panel.DirectoryRecords[panel.Location] = DirectoryRecord{ | |
| directoryCursor: panel.Cursor, | |
| directoryRender: panel.RenderIndex, | |
| } |
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 57 to 60, assigning to
panel.DirectoryRecords may panic if DirectoryRecords is nil; guard the write by
initializing the map on first use (e.g., if panel.DirectoryRecords == nil {
panel.DirectoryRecords = make(map[KeyType]DirectoryRecord) }) before assigning
the DirectoryRecord so the map is always non-nil when written to.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/function.go (1)
72-76: Bump go.mod to Go 1.21+ (slices.DeleteFunc used)src/internal/function.go (lines 72–76) uses slices.DeleteFunc (added in Go 1.21); go.mod does not declare Go 1.21+, so the build will fail.
- Update go.mod to declare Go 1.21+ (example: run
go mod edit -go=1.21or setgo 1.22/1.23).- If you must support older Go versions, replace slices.DeleteFunc with a local equivalent or
golang.org/x/exp/slices.DeleteFunc.
🧹 Nitpick comments (7)
src/internal/type.go (1)
155-169: Add GoDoc comments for newly exported types.Exported identifiers should have comments starting with their names to satisfy linters and improve discoverability.
-// Type representing the mode of the panel -type PanelMode uint +// PanelMode represents the interaction mode of a file panel (e.g., SelectMode or BrowserMode). +type PanelMode uint @@ -// Panel representing a file -type FilePanel struct { +// FilePanel represents a single file panel view and its state. +type FilePanel struct { @@ -// Sort options -type SortOptionsModel struct { +// SortOptionsModel encapsulates state for the sort options menu for a panel. +type SortOptionsModel struct { @@ -// Record for directory navigation -type DirectoryRecord struct { +// DirectoryRecord stores per-directory cursor/render positions for navigation history. +type DirectoryRecord struct { @@ -// Element within a file panel -type Element struct { +// Element is a file-system entry (file or directory) displayed in a file panel. +// Note: fields are intentionally unexported to maintain invariants within package internal. +type Element struct {Also applies to: 172-178, 187-190, 193-199, 21-21
src/internal/handle_file_operations.go (1)
84-87: Clarify precedence in rename cursor calculation.Add parentheses to make intent unambiguous for future readers.
- if cursorPos == -1 || cursorPos == 0 && nameLen > 0 || panel.Element[panel.Cursor].directory { + if cursorPos == -1 || (cursorPos == 0 && nameLen > 0) || panel.Element[panel.Cursor].directory {src/internal/function.go (5)
65-83: Return type migration + nil returns: confirm callers handle nil safelyNow returns []Element and nil on error/empty. len(nil) is safe, but please confirm no callers append to or assume non-nil slices.
Apply minor comment fix:
- // No files/directoes to process + // No files/directories to process
85-123: Micro-alloc wins for search pathPreallocate map/slice to avoid re-allocations when scanning large dirs.
- folderElementMap := map[string]os.DirEntry{} - fileAndDirectories := []string{} + folderElementMap := make(map[string]os.DirEntry, len(items)) + fileAndDirectories := make([]string, 0, len(items))
125-139: Preallocation LGTM; tiny naming nitThe function is efficient and clear. Consider a shorter local name.
- directoryElement := make([]Element, 0, len(dirEntries)) + elements := make([]Element, 0, len(dirEntries)) for _, item := range dirEntries { - directoryElement = append(directoryElement, Element{ + elements = append(elements, Element{ name: item.Name(), directory: item.IsDir(), location: filepath.Join(location, item.Name()), }) } - return directoryElement + return elements
158-166: Stabilize Date Modified ordering (tie-break on name)Non-deterministic ordering when ModTime matches; add a name tie-breaker.
- order = func(i, j int) bool { - // No need for err check, we already filtered out dirEntries with err != nil in Info() call - fileInfoI, _ := dirEntries[i].Info() - fileInfoJ, _ := dirEntries[j].Info() - // Note : If ModTime matches, the comparator returns false both ways; order becomes non-deterministic - // TODO: Fix this - return fileInfoI.ModTime().After(fileInfoJ.ModTime()) != reversed - } + order = func(i, j int) bool { + fileInfoI, _ := dirEntries[i].Info() + fileInfoJ, _ := dirEntries[j].Info() + if fileInfoI.ModTime().Equal(fileInfoJ.ModTime()) { + if common.Config.CaseSensitiveSort { + return (dirEntries[i].Name() < dirEntries[j].Name()) != reversed + } + return (strings.ToLower(dirEntries[i].Name()) < strings.ToLower(dirEntries[j].Name())) != reversed + } + return fileInfoI.ModTime().After(fileInfoJ.ModTime()) != reversed + }Happy to open a follow-up issue if you want to keep this PR scoped to type exports.
172-201: Avoid repeated os.ReadDir in comparator (cache child counts)os.ReadDir inside the sort comparator can explode to O(n log n) directory reads. Cache per-directory child counts once per sort.
-func getSizeOrderingFunc(dirEntries []os.DirEntry, reversed bool, location string) sliceOrderFunc { - return func(i, j int) bool { +func getSizeOrderingFunc(dirEntries []os.DirEntry, reversed bool, location string) sliceOrderFunc { + // Cache child counts to avoid repeated disk I/O in comparator + childCounts := make(map[string]int, len(dirEntries)) + getCount := func(name string) int { + if c, ok := childCounts[name]; ok { + return c + } + files, err := os.ReadDir(filepath.Join(location, name)) + if err != nil { + slog.Error("Error when reading directory during sort", "error", err) + childCounts[name] = 0 + return 0 + } + childCounts[name] = len(files) + return childCounts[name] + } + return func(i, j int) bool { // Directories at the top sorted by direct child count (not recursive) // Files sorted by size // One of them is a directory, and other is not if dirEntries[i].IsDir() != dirEntries[j].IsDir() { return dirEntries[i].IsDir() } // This needs to be improved, and we should sort by actual size only // Repeated recursive read would be slow, so we could cache if dirEntries[i].IsDir() && dirEntries[j].IsDir() { - filesI, err := os.ReadDir(filepath.Join(location, dirEntries[i].Name())) - // No need of early return, we only call len() on filesI, so nil would - // just result in 0 - if err != nil { - slog.Error("Error when reading directory during sort", "error", err) - } - filesJ, err := os.ReadDir(filepath.Join(location, dirEntries[j].Name())) - if err != nil { - slog.Error("Error when reading directory during sort", "error", err) - } - return len(filesI) < len(filesJ) != reversed + countI := getCount(dirEntries[i].Name()) + countJ := getCount(dirEntries[j].Name()) + return countI < countJ != reversed } // No need for err check, we already filtered out dirEntries with err != nil in Info() call fileInfoI, _ := dirEntries[i].Info() fileInfoJ, _ := dirEntries[j].Info() return fileInfoI.Size() < fileInfoJ.Size() != reversed } }Can open a small follow-up PR to land this without bloating this refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/internal/file_panel.go(2 hunks)src/internal/function.go(5 hunks)src/internal/handle_file_operation_test.go(2 hunks)src/internal/handle_file_operations.go(15 hunks)src/internal/handle_panel_navigation.go(5 hunks)src/internal/key_function.go(3 hunks)src/internal/test_utils.go(4 hunks)src/internal/type.go(5 hunks)src/internal/type_utils.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/internal/handle_panel_navigation.go
- src/internal/key_function.go
- src/internal/handle_file_operation_test.go
- src/internal/file_panel.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: deleteItemWarn() in yorukot/superfile only displays a warning modal via channel messaging and does not perform actual file deletion. The actual deletion happens through getDeleteCmd() which returns a tea.Cmd. The architectural pattern separates UI modal operations (using channels/goroutines) from file I/O operations (using tea.Cmd), which is intentional and appropriate.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/function.go:211-211
Timestamp: 2025-03-29T13:20:46.467Z
Learning: Empty panel.element checks should be added at the beginning of functions that access panel.element to prevent crashes when a panel has no elements, similar to using `if len(panel.element) == 0 { return }`.
Applied to files:
src/internal/handle_file_operations.go
🧬 Code graph analysis (5)
src/internal/test_utils.go (1)
src/internal/type.go (4)
SelectMode(49-49)PanelMode(21-21)FilePanel(155-169)Element(193-198)
src/internal/handle_file_operations.go (4)
src/internal/type.go (1)
Element(193-198)src/internal/handle_panel_movement.go (3)
panel(133-147)m(125-130)m(107-122)src/internal/model_render.go (1)
panel(110-148)src/internal/handle_modal.go (1)
m(64-86)
src/internal/type.go (2)
src/internal/model_render.go (2)
panel(150-164)panel(110-148)src/internal/handle_modal.go (2)
m(99-103)m(88-91)
src/internal/function.go (2)
src/internal/type.go (3)
SortingKind(31-31)Element(193-198)FilePanel(155-169)src/internal/function_test.go (1)
TestReturnDirElement(15-202)
src/internal/type_utils.go (3)
src/internal/type.go (6)
FilePanel(155-169)SortOptionsModel(172-178)PanelMode(21-21)BrowserMode(50-50)DirectoryRecord(187-190)SelectMode(49-49)src/internal/common/style_function.go (1)
GenerateSearchBar(235-247)src/internal/model_render.go (3)
panel(150-164)panel(110-148)panel(86-108)
🔇 Additional comments (10)
src/internal/type.go (2)
147-152: Renames to use FilePanel look consistent.fileModel now holds []FilePanel; downstream call sites will read cleaner. No functional concerns.
193-199: Exported Element with unexported fields is a good boundary.Keeping fields unexported preserves invariants while enabling typed APIs across the package.
src/internal/handle_file_operations.go (1)
33-37: Field renames to exported API are applied correctly.Location/Cursor/Element/Selected/PanelMode/Rename/Renaming usages look consistent; guards on empty panels remain in place.
Also applies to: 49-51, 80-94, 96-118, 158-180, 338-346, 403-404, 431-466, 470-497, 509-515, 520-523
src/internal/test_utils.go (2)
42-47: Tests aligned to public API (ChangeFilePanelMode, PanelMode, Selected).Reads cleanly and matches the new type surface.
191-208: Helper functions updated to FilePanel/Element fields look good.In-package access to unexported Element fields is fine for tests.
Also applies to: 228-240
src/internal/function.go (2)
25-28: Typed sorting constants look goodAligning constants to SortingKind tightens the API surface and reduces stringly-typed usage.
328-330: Pointer to slice element: verify lifetime assumptionsReturning &m.fileModel.filePanels[idx] is common, but becomes stale if the slice resizes. Ensure callers don’t retain it across operations that append/reallocate filePanels.
src/internal/type_utils.go (3)
39-47: filePanelSlice export migration LGTMClean switch to []FilePanel with first panel focused.
49-73: defaultFilePanel initialization looks correct; validate config boundsGood use of exported fields and constants. Please confirm common.Config.DefaultSortType is within options’ bounds to avoid invalid indices.
92-101: PanelMode String receiver migration LGTMString values preserved; safe for logs/UI.
| if panel.PanelMode == SelectMode { | ||
| items = panel.Selected | ||
| } else { | ||
| items = []string{panel.getSelectedItem().location} | ||
| items = []string{panel.GetSelectedItem().location} | ||
| } | ||
|
|
||
| useTrash := m.hasTrash && !isExternalDiskPath(panel.location) && !permDelete | ||
| useTrash := m.hasTrash && !isExternalDiskPath(panel.Location) && !permDelete | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Snapshot Selected slices to avoid TOCTOU/aliasing in async cmds.
Slices alias backing arrays; if selection changes before the async tea.Cmd runs, operations could act on unintended paths (especially delete/compress). Take a copy at capture time.
@@
- if panel.PanelMode == SelectMode {
- items = panel.Selected
- } else {
- items = []string{panel.GetSelectedItem().location}
- }
+ if panel.PanelMode == SelectMode {
+ // snapshot selection
+ items = append([]string(nil), panel.Selected...)
+ } else {
+ loc := panel.GetSelectedItem().location
+ items = []string{loc}
+ }
@@
- m.copyItems.items = panel.Selected
+ // snapshot selection
+ m.copyItems.items = append([]string(nil), panel.Selected...)
@@
- if len(panel.Selected) == 0 {
- firstFile = panel.Element[panel.Cursor].location
- filesToCompress = append(filesToCompress, firstFile)
- } else {
- firstFile = panel.Selected[0]
- filesToCompress = panel.Selected
- }
+ if len(panel.Selected) == 0 {
+ firstFile = panel.Element[panel.Cursor].location
+ filesToCompress = []string{firstFile}
+ } else {
+ // snapshot selection
+ filesToCompress = append([]string(nil), panel.Selected...)
+ firstFile = filesToCompress[0]
+ }Also applies to: 199-205, 386-392
🤖 Prompt for AI Agents
In src/internal/handle_file_operations.go around lines 103 to 110 (and similarly
at 199-205 and 386-392), the code captures panel.Selected (a slice) directly for
async tea.Cmds which can lead to TOCTOU/aliasing if the selection mutates before
the command runs; fix by making a shallow copy of the selected slice at capture
time (e.g., allocate a new []string of the same length and copy elements) and
use that copy in the async command payload so the command operates on a stable
snapshot of paths.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/internal/handle_file_operations.go (1)
186-197: Paste: snapshot clipboard payload.Avoid capturing m.copyItems.items by reference; freeze a copy for the Cmd.
Apply:
- copyItems := m.copyItems.items + copyItems := append([]string(nil), m.copyItems.items...)src/internal/handle_panel_movement.go (1)
1-178: Fix leftover private-field references: searchBar / sortOptions / rename / renamingrg found remaining usages — align these references with the intended rename (capitalize/export) or revert the broad rename. Affected files include:
- src/superfile_config/vimHotkeys.toml
- src/superfile_config/hotkeys.toml
- src/internal/default_config.go
- src/internal/type.go
- src/internal/model_render.go
- src/internal/model_file_operations_test.go
- src/internal/model.go
- src/internal/handle_modal.go
- src/internal/key_function.go
- src/internal/function.go
- src/internal/handle_file_operations.go
- src/internal/file_panel.go
- src/internal/function_test.go
- src/internal/ui/sidebar/utils.go
- src/internal/ui/rendering/renderer.go
- src/internal/ui/sidebar/README.md
- src/internal/ui/sidebar/type.go
- src/internal/ui/sidebar/render.go
- src/internal/ui/sidebar/sidebar.go
- src/internal/file_operations.go
- src/internal/common/config_type.go
src/internal/key_function.go (1)
20-129: Replace remaining direct m.fileModel.filePanels[...] indexing with getFocusedFilePanel()Findings: direct-index usages remain in these files — src/internal/wheel_function.go, src/internal/handle_file_operations.go, src/internal/model_render.go, src/internal/handle_panel_movement.go, src/internal/key_function.go, src/internal/handle_modal.go, src/internal/handle_panel_navigation.go, src/internal/model.go, src/internal/model_msg.go and multiple tests (model_*.go, model_file_operations_test.go, handle_file_operation_test.go, test_utils.go, etc).
Action: replace occurrences of
m.fileModel.filePanels[m.filePanelFocusIndex]
with
m.getFocusedFilePanel()
and update tests that reference Element/Selected/Cursor accordingly. Preserve or add an explicit index-based accessor for legitimate non‑focused-index uses (neighbor panels, iterations, append/remove operations).
♻️ Duplicate comments (5)
src/internal/handle_file_operations.go (3)
80-85: Snapshot selection before async delete.Avoid aliasing/TOCTOU with panel.Selected when building tea.Cmd payload.
Apply:
- if panel.PanelMode == SelectMode { - items = panel.Selected - } else { - items = []string{panel.GetSelectedItem().location} - } + if panel.PanelMode == SelectMode { + // snapshot selection + items = append([]string(nil), panel.Selected...) + } else { + loc := panel.GetSelectedItem().location + items = []string{loc} + }
177-183: Clipboard copy (multi): snapshot Selected.Assigning the slice directly aliases backing storage; later selection changes will mutate clipboard.
Apply:
- m.copyItems.items = panel.Selected + // snapshot selection + m.copyItems.items = append([]string(nil), panel.Selected...)
359-370: Compress: snapshot selection and destination.
- Snapshot panel.Selected to avoid aliasing.
- Snapshot panel.Location to ensure archive goes to the panel visible at trigger time.
Apply:
- if len(panel.Selected) == 0 { - firstFile = panel.Element[panel.Cursor].location - filesToCompress = append(filesToCompress, firstFile) - } else { - firstFile = panel.Selected[0] - filesToCompress = panel.Selected - } + if len(panel.Selected) == 0 { + firstFile = panel.Element[panel.Cursor].location + filesToCompress = append(filesToCompress, firstFile) + } else { + sel := append([]string(nil), panel.Selected...) + firstFile = sel[0] + filesToCompress = sel + } @@ - return func() tea.Msg { + panelLocation := panel.Location + return func() tea.Msg { @@ - zipPath := filepath.Join(panel.Location, zipName) + zipPath := filepath.Join(panelLocation, zipName)Also applies to: 381-383
src/internal/file_panel.go (2)
282-285: I/O in render path harms performance. (Duplicate of existing feedback)The os.ReadDir call during rendering should be avoided/cached. This was already flagged; acknowledging to avoid rehashing here.
61-64: Potential panic: write to nil map DirectoryRecords.Initialize the map before assignment.
// Update the cursor and render status in case we switch back to this. + if panel.DirectoryRecords == nil { + panel.DirectoryRecords = make(map[string]DirectoryRecord) + } panel.DirectoryRecords[panel.Location] = DirectoryRecord{ directoryCursor: panel.Cursor, directoryRender: panel.RenderIndex, }
🧹 Nitpick comments (8)
src/internal/handle_file_operations.go (3)
58-72: Make the rename-cursor condition explicit.Parenthesize to avoid precedence ambiguity for future readers.
Apply:
- if cursorPos == -1 || cursorPos == 0 && nameLen > 0 || panel.Element[panel.Cursor].directory { + if cursorPos == -1 || (cursorPos == 0 && nameLen > 0) || panel.Element[panel.Cursor].directory {
151-157: Freeze modal semantics before returning tea.Cmd.Compute title/content/action outside the closure to avoid drift if panel.Location or flags change between keypress and command execution.
Apply:
- return func() tea.Msg { - title := common.TrashWarnTitle - content := common.TrashWarnContent - action := notify.DeleteAction - - if !m.hasTrash || isExternalDiskPath(panel.Location) || deletePermanent { - title = common.PermanentDeleteWarnTitle - content = common.PermanentDeleteWarnContent - action = notify.PermanentDeleteAction - } - return NewNotifyModalMsg(notify.New(true, title, content, action), reqID) - } + title := common.TrashWarnTitle + content := common.TrashWarnContent + action := notify.DeleteAction + if !m.hasTrash || isExternalDiskPath(panel.Location) || deletePermanent { + title = common.PermanentDeleteWarnTitle + content = common.PermanentDeleteWarnContent + action = notify.PermanentDeleteAction + } + return func() tea.Msg { + return NewNotifyModalMsg(notify.New(true, title, content, action), reqID) + }
449-476: Open dir with editor: snapshot path before building args.Minor: capture Location into a local to avoid drift if focus changes mid-exec.
Apply:
- err := m.chooserFileWriteAndQuit(m.fileModel.filePanels[m.filePanelFocusIndex].Location) + loc := m.fileModel.filePanels[m.filePanelFocusIndex].Location + err := m.chooserFileWriteAndQuit(loc) @@ - args := append(parts[1:], m.fileModel.filePanels[m.filePanelFocusIndex].Location) + args := append(parts[1:], loc)src/internal/handle_panel_movement.go (1)
69-76: Reuse selectedItem for chooser write.Avoid re-indexing; keeps consistency if Cursor changes within the function.
Apply:
- chooserErr := m.chooserFileWriteAndQuit(panel.Element[panel.Cursor].location) + chooserErr := m.chooserFileWriteAndQuit(selectedItem.location)src/internal/key_function.go (2)
146-146: Prefer public accessors over internal fields.Keeps callers aligned with the new exported API.
- if !m.getFocusedFilePanel().isFocused { + if !m.getFocusedFilePanel().IsFocused() { @@ - if m.getFocusedFilePanel().PanelMode == SelectMode { + if m.getFocusedFilePanel().GetPanelMode() == SelectMode {Also applies to: 151-151
191-197: Polish log messages.Minor grammar fixes in user-facing logs.
- if err := m.getFocusedFilePanel().CopyPath(); err != nil { - slog.Error("Error while copy path", "error", err) + if err := m.getFocusedFilePanel().CopyPath(); err != nil { + slog.Error("Error copying path", "error", err) } @@ - if err := m.getFocusedFilePanel().CopyPWD(); err != nil { - slog.Error("Error while copy present working directory", "error", err) + if err := m.getFocusedFilePanel().CopyPWD(); err != nil { + slog.Error("Error copying current working directory", "error", err) }src/internal/file_panel.go (2)
199-206: Selection toggle is O(n) per action; becomes O(n²) during render.Consider switching Selected to a set (map[string]struct{}) or maintaining a parallel set for membership tests; keep slice only for rendering order if needed. Suggest tracking as a follow-up issue per scope.
135-181: PgUp/PgDown nesting and window math can be simplified.Extract cursor/render window calculation into a helper to reduce branching; improves readability and testability.
If helpful, I can provide a small helper like computeWindowAfterPage(delta int) that returns the new cursor and render index.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/internal/file_panel.go(2 hunks)src/internal/handle_file_operations.go(13 hunks)src/internal/handle_modal.go(1 hunks)src/internal/handle_panel_movement.go(6 hunks)src/internal/key_function.go(9 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-07-27T15:32:06.922Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:32:06.922Z
Learning: When reviewing large refactoring PRs that change async patterns (like moving from goroutines to tea.Cmd), always check for incomplete refactoring where some call sites still use the old pattern while others use the new pattern, as this often leads to compilation errors and architectural inconsistencies.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/handle_file_operations.gosrc/internal/key_function.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: deleteItemWarn() in yorukot/superfile only displays a warning modal via channel messaging and does not perform actual file deletion. The actual deletion happens through getDeleteCmd() which returns a tea.Cmd. The architectural pattern separates UI modal operations (using channels/goroutines) from file I/O operations (using tea.Cmd), which is intentional and appropriate.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/function.go:211-211
Timestamp: 2025-03-29T13:20:46.467Z
Learning: Empty panel.element checks should be added at the beginning of functions that access panel.element to prevent crashes when a panel has no elements, similar to using `if len(panel.element) == 0 { return }`.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
PR: yorukot/superfile#1039
File: src/internal/ui/zoxide/model.go:53-54
Timestamp: 2025-09-06T13:42:44.590Z
Learning: The zoxide modal in src/internal/ui/zoxide/model.go is missing handling for common.Hotkeys.Quit when zClient is nil (lines 52-54), only handling ConfirmTyping and CancelTyping. This creates inconsistency with other modals like sort options menu, help menu, and notify model which all properly handle the Quit hotkey. The prompt modal has the same inconsistency.
Applied to files:
src/internal/key_function.go
🧬 Code graph analysis (5)
src/internal/handle_file_operations.go (3)
src/internal/type.go (5)
Element(193-198)PanelMode(21-21)SelectMode(49-49)BrowserMode(50-50)filePanels(146-152)src/internal/common/style_function.go (1)
GenerateRenameTextInput(276-292)src/config/fixed_variable.go (1)
ChooserFile(71-71)
src/internal/file_panel.go (6)
src/internal/type.go (6)
FilePanel(155-169)Element(193-198)PanelMode(21-21)SelectMode(49-49)BrowserMode(50-50)DirectoryRecord(187-190)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)src/internal/common/predefined_variable.go (2)
FilePanelTopDirectoryIcon(42-42)FilePanelNoneText(43-43)
src/internal/handle_modal.go (3)
src/internal/model_file_operations_test.go (1)
TestFileRename(113-187)src/internal/model_render.go (1)
panel(150-164)src/internal/ui/sidebar/sidebar.go (1)
s(31-34)
src/internal/handle_panel_movement.go (3)
src/internal/type.go (1)
Element(193-198)src/internal/handle_panel_navigation.go (2)
m(77-108)m(147-156)src/internal/model.go (1)
m(688-733)
src/internal/key_function.go (1)
src/internal/type.go (2)
PanelMode(21-21)SelectMode(49-49)
🔇 Additional comments (9)
src/internal/handle_modal.go (2)
58-61: Rename: good migration to exported fields.Using panel.Rename and panel.Renaming aligns with the new public API. Keep m.fileModel.renaming and panel.Renaming in lockstep.
78-79: Search confirm: OK.Blur-only is consistent if filtering is driven by SearchBar.Value().
src/internal/handle_file_operations.go (2)
32-36: New-file modal: location source is correct.Using panel.Location matches the exported surface.
318-323: Extract: safe access via GetSelectedItem.String snapshot into item is correct.
src/internal/handle_panel_movement.go (3)
19-23: ParentDirectory via public API: OK.Migration to panel.ParentDirectory() looks correct.
30-41: Guard on empty Element: good.Early return avoids OOB on Cursor. Using GetSelectedItem() centralizes selection logic.
156-165: Search bar focus toggle: OK.Uses exported SearchBar and sets width relative to panel width.
src/internal/key_function.go (1)
132-142: Nice separation for sidebar-only keys when file panel isn’t focused.Clear, localized handling. LGTM.
src/internal/file_panel.go (1)
300-306: Resolve — SortOptions.options is initialized non-emptydefaultFilePanel populates SortOptions.data.options with four entries (src/internal/type_utils.go) and I found no code that clears/reassigns it; indexing usages are safe.
| func (panel *FilePanel) SelectAllItems() { | ||
| for _, item := range panel.Element { | ||
| panel.Selected = append(panel.Selected, item.location) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
SelectAllItems appends without clearing; duplicates and growth risk.
Clear first to avoid duplicate entries and unbounded growth.
func (panel *FilePanel) SelectAllItems() {
- for _, item := range panel.Element {
- panel.Selected = append(panel.Selected, item.location)
- }
+ panel.Selected = panel.Selected[:0]
+ for _, item := range panel.Element {
+ panel.Selected = append(panel.Selected, item.location)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (panel *FilePanel) SelectAllItems() { | |
| for _, item := range panel.Element { | |
| panel.Selected = append(panel.Selected, item.location) | |
| } | |
| } | |
| func (panel *FilePanel) SelectAllItems() { | |
| panel.Selected = panel.Selected[:0] | |
| for _, item := range panel.Element { | |
| panel.Selected = append(panel.Selected, item.location) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 427 to 431, SelectAllItems currently
appends every item.location to panel.Selected without clearing existing entries,
causing duplicates and unbounded growth; modify the method to reset/clear
panel.Selected before the loop (e.g., assign an empty slice or set length to
zero) and then append each item.location so the selection reflects exactly the
current items.
| func (m *model) toggleReverseSort() { | ||
| panel := &m.fileModel.filePanels[m.filePanelFocusIndex] | ||
| panel.sortOptions.data.reversed = !panel.sortOptions.data.reversed | ||
| panel.SortOptions.data.reversed = !panel.SortOptions.data.reversed |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm list refresh after reversing sort.
Toggling SortOptions.data.reversed alone may not trigger a re-fetch/re-sort until the next tick. Verify call sites that depend on this flag re-read and re-apply sorting immediately as expected.
Run:
🏁 Script executed:
#!/bin/bash
# Ensure sorting reads panel.SortOptions.* (not stale panel.sortOptions) and is used in list building.
rg -nP -C2 '\bSortOptions\b.*reversed|\bSortOptions\b.*selected|\bSortOptions\b.*options' -- src | sed -n '1,120p'Length of output: 2374
Apply the sort/refresh immediately after toggling sort direction
Toggling panel.SortOptions.data.reversed in src/internal/handle_modal.go:65 only flips the flag; there’s no code that re-reads it to rebuild/re-sort the file list. ConfirmSortOptions (src/internal/file_panel.go:468–471) only updates selection/open. Call the file-list refresh/sort routine after toggling (or move the toggle into ConfirmSortOptions) so the UI updates immediately.
🤖 Prompt for AI Agents
In src/internal/handle_modal.go around line 65, toggling
panel.SortOptions.data.reversed only flips the flag but doesn’t refresh or
re-sort the file list; update the code to call the file-list
rebuild/sort/refresh routine immediately after toggling (or move the toggle into
ConfirmSortOptions in src/internal/file_panel.go lines 468–471 so the toggle
happens before the existing refresh logic) so the UI updates the sorted file
list right away.
| panel.SearchBar.Blur() | ||
| panel.SearchBar.SetValue("") | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Search cancel: ensure file list is refreshed.
Clearing SearchBar without triggering a refresh may delay UI update. Confirm that the next render cycle re-queries with empty filter.
Run:
🏁 Script executed:
#!/bin/bash
# getFilePanelItems should branch on panel.SearchBar.Value() and rebuild element.
rg -nP -C2 'getFilePanelItems|SearchBar\.Value\(\)' -- src/internalLength of output: 1537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- head: src/internal/handle_modal.go (1-200) ----"
sed -n '1,200p' src/internal/handle_modal.go || true
echo
echo "---- rg: SearchBar.SetValue occurrences ----"
rg -n --hidden --no-ignore 'SearchBar\.SetValue\(' || true
echo
echo "---- rg: updateModelStateAfterMsg occurrences ----"
rg -n --hidden --no-ignore 'updateModelStateAfterMsg' || true
echo
echo "---- head: src/internal/model.go (1-220) ----"
sed -n '1,220p' src/internal/model.go || true
echo
echo "---- src/internal/model.go (680-740) ----"
sed -n '680,740p' src/internal/model.go || trueLength of output: 16386
Search cancel: force file-panel refresh when clearing search.
Clearing the SearchBar (handle_modal.go: panel.SearchBar.Blur(); panel.SearchBar.SetValue("")) does not guarantee an immediate re-query — getFilePanelItems() (src/internal/model.go:688–724) short‑circuits updates based on LastTimeGetElement/reRenderTime. After SetValue(""), call m.getFilePanelItems() (or set the panel's LastTimeGetElement = time.Time{} / m.updatedToggleDotFile = true) to force an immediate refresh.
🤖 Prompt for AI Agents
In src/internal/handle_modal.go around lines 71 to 73, after calling
panel.SearchBar.Blur() and panel.SearchBar.SetValue(""), force the file-panel to
refresh immediately by either calling m.getFilePanelItems() or resetting the
panel's timing/flags (e.g. set panel.LastTimeGetElement = time.Time{} and/or
m.updatedToggleDotFile = true) so getFilePanelItems() won't short‑circuit; add
the chosen change right after SetValue("") to trigger an immediate re-query and
re-render.
| m.fileMetaData.ListUp() | ||
| case nonePanelFocus: | ||
| m.fileModel.filePanels[m.filePanelFocusIndex].listUp(m.mainPanelHeight) | ||
| m.fileModel.filePanels[m.filePanelFocusIndex].ListUp(m.mainPanelHeight) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify focused-panel access; avoid direct indexing.
Use getFocusedFilePanel() consistently to decouple from fileModel internals and prevent focus-index drift.
- m.fileModel.filePanels[m.filePanelFocusIndex].ListUp(m.mainPanelHeight)
+ m.getFocusedFilePanel().ListUp(m.mainPanelHeight)
@@
- m.fileModel.filePanels[m.filePanelFocusIndex].ListDown(m.mainPanelHeight)
+ m.getFocusedFilePanel().ListDown(m.mainPanelHeight)
@@
- m.fileModel.filePanels[m.filePanelFocusIndex].PgUp(m.mainPanelHeight)
+ m.getFocusedFilePanel().PgUp(m.mainPanelHeight)
@@
- m.fileModel.filePanels[m.filePanelFocusIndex].PgDown(m.mainPanelHeight)
+ m.getFocusedFilePanel().PgDown(m.mainPanelHeight)Also applies to: 45-45, 49-49, 52-52
🤖 Prompt for AI Agents
In src/internal/key_function.go around lines 32 (and similarly at 45, 49, 52),
the code directly indexes into m.fileModel.filePanels using
m.filePanelFocusIndex; replace those direct indexed accesses with the existing
getFocusedFilePanel() call to retrieve the focused panel, then call ListUp (or
the equivalent method) on that returned panel. Ensure you handle a nil return
from getFocusedFilePanel() before calling methods to avoid panics, and remove or
stop using m.filePanelFocusIndex for direct access so all focused-panel
interactions go through the helper.
| switch { | ||
| case slices.Contains(common.Hotkeys.Confirm, msg): | ||
| m.fileModel.filePanels[m.filePanelFocusIndex].singleItemSelect() | ||
| m.fileModel.filePanels[m.filePanelFocusIndex].SingleItemSelect() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Selection-mode calls should also use the focused-panel accessor.
Consistency and future-proofing.
- m.fileModel.filePanels[m.filePanelFocusIndex].SingleItemSelect()
+ m.getFocusedFilePanel().SingleItemSelect()
@@
- m.fileModel.filePanels[m.filePanelFocusIndex].ItemSelectUp(m.mainPanelHeight)
+ m.getFocusedFilePanel().ItemSelectUp(m.mainPanelHeight)
@@
- m.fileModel.filePanels[m.filePanelFocusIndex].ItemSelectDown(m.mainPanelHeight)
+ m.getFocusedFilePanel().ItemSelectDown(m.mainPanelHeight)Also applies to: 156-156, 158-158
🤖 Prompt for AI Agents
In src/internal/key_function.go around lines 154, 156 and 158, the code calls
the file panel via direct indexing
(m.fileModel.filePanels[m.filePanelFocusIndex].SingleItemSelect()) — replace
those direct-index expressions with the focused-panel accessor on fileModel
(e.g., m.fileModel.FocusedPanel().SingleItemSelect()) so selection-mode calls
consistently go through the focused-panel accessor.
| if err := m.getFocusedFilePanel().ConfirmRename(); err != nil { | ||
| slog.Error("Error confirming rename", "error", err) | ||
| } | ||
| m.fileModel.renaming = false | ||
| case notify.QuitAction: |
There was a problem hiding this comment.
Don’t drop rename state on error; avoid duplicate state sources.
ConfirmRename() already resets the panel’s Renaming state on success. Clearing m.fileModel.renaming unconditionally can desync states on error.
if err := m.getFocusedFilePanel().ConfirmRename(); err != nil {
slog.Error("Error confirming rename", "error", err)
}
- m.fileModel.renaming = false if err := m.getFocusedFilePanel().ConfirmRename(); err != nil {
slog.Error("Error confirming rename", "error", err)
}
- m.fileModel.renaming = falseWould you like me to remove all remaining writes to m.fileModel.renaming across the repo?
Also applies to: 288-292
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/internal/file_panel.go (3)
282-285: I/O in render path (os.ReadDir) hurts performanceDoing filesystem I/O while rendering degrades UI responsiveness; move/cached elsewhere.
Want me to open an issue to track moving this check to the data update phase?
61-64: Initialize DirectoryRecords before write to avoid panicWriting to a nil map will panic. Guard initialization before assignment.
- // Update the cursor and render status in case we switch back to this. - panel.DirectoryRecords[panel.Location] = DirectoryRecord{ + // Update the cursor and render status in case we switch back to this. + if panel.DirectoryRecords == nil { + panel.DirectoryRecords = make(map[string]DirectoryRecord) + } + panel.DirectoryRecords[panel.Location] = DirectoryRecord{ directoryCursor: panel.Cursor, directoryRender: panel.RenderIndex, }
431-435: SelectAllItems appends without clearing → duplicates and growthClear selection first so the result reflects the current view exactly.
func (panel *FilePanel) SelectAllItems() { - for _, item := range panel.Element { - panel.Selected = append(panel.Selected, item.location) - } + panel.Selected = panel.Selected[:0] + for _, item := range panel.Element { + panel.Selected = append(panel.Selected, item.location) + } }src/internal/handle_file_operations.go (4)
186-199: Paste: snapshot copyItems and prefer accessor for LocationMake a defensive copy for the closure; use GetLocation() for consistency.
- copyItems := m.copyItems.items + copyItems := append([]string(nil), m.copyItems.items...) @@ - panelLocation := m.getFocusedFilePanel().Location + panelLocation := m.getFocusedFilePanel().GetLocation()
81-89: Snapshot selection before spawning delete CmdSlices alias backing arrays; copy Selected to avoid TOCTOU/aliasing if selection changes before the Cmd runs.
if panel.GetPanelMode() == SelectMode { - items = panel.GetSelected() + items = append([]string(nil), panel.GetSelected()...) } else { items = []string{panel.GetSelectedItem().location} }
176-184: copyMultipleItem should snapshot SelectedAssigning the slice directly aliases future mutations.
- m.copyItems.items = panel.GetSelected() + m.copyItems.items = append([]string(nil), panel.GetSelected()...)
365-371: Compress: snapshot Selected and derive firstFile from the snapshotPrevents races with UI selection changes during compression.
- firstFile = panel.GetSelected()[0] - filesToCompress = panel.GetSelected() + sel := append([]string(nil), panel.GetSelected()...) + firstFile = sel[0] + filesToCompress = sel
🧹 Nitpick comments (5)
src/internal/file_panel.go (1)
265-271: Avoid O(n^2) selection checks during renderPrecompute a set from Selected once, then do O(1) lookups inside the loop.
- end := min(panel.RenderIndex+panelElementHeight(mainPanelHeight), len(panel.Element)) + end := min(panel.RenderIndex+panelElementHeight(mainPanelHeight), len(panel.Element)) + // Build a selection set for O(1) membership checks + selectedSet := make(map[string]struct{}, len(panel.Selected)) + for _, p := range panel.Selected { + selectedSet[p] = struct{}{} + } for i := panel.RenderIndex; i < end; i++ { - // TODO : Fix this, this is O(n^2) complexity. Considered a file panel with 200 files, and 100 selected - // We will be doing a search in 100 item slice for all 200 files. - isSelected := arrayContains(panel.Selected, panel.Element[i].location) + _, isSelected := selectedSet[panel.Element[i].location]src/internal/handle_panel_movement.go (1)
36-41: Use FilePanel.UpdateCurrentFilePanelDir for consistencyPrefer calling panel.UpdateCurrentFilePanelDir(selectedItem.location) here to keep navigation concerns within FilePanel and ensure invariants (like SearchBar clearing) are uniformly applied.
Would you like me to sweep the repo and replace call sites or just open a tracking issue?
src/internal/handle_file_operations.go (3)
65-66: Clarify precedence in rename cursor position conditionAdd parentheses for readability; current logic is correct but hard to parse.
- if cursorPos == -1 || cursorPos == 0 && nameLen > 0 || selectedItem.directory { + if cursorPos == -1 || (cursorPos == 0 && nameLen > 0) || selectedItem.directory {
447-476: Minor consistency nit: use GetLocation()Open-directory path uses the exported field directly; consider GetLocation() for consistency with the rest of the PR.
- err := m.chooserFileWriteAndQuit(m.fileModel.filePanels[m.filePanelFocusIndex].Location) + err := m.chooserFileWriteAndQuit(m.fileModel.filePanels[m.filePanelFocusIndex].GetLocation()) @@ - args := append(parts[1:], m.fileModel.filePanels[m.filePanelFocusIndex].Location) + args := append(parts[1:], m.fileModel.filePanels[m.filePanelFocusIndex].GetLocation())
1-482: No duplicate rename flows remain — update stale commentVerified: FilePanel.ConfirmRename() (src/internal/file_panel.go) is the active rename path and is invoked from src/internal/key_function.go; there is no confirmRename() in handle_modal.go. Remove/update the stale comment in src/internal/handle_file_operations.go (around line 55) that still references confirmRename() in handle_modal.go.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/file_panel.go(2 hunks)src/internal/handle_file_operations.go(13 hunks)src/internal/handle_panel_movement.go(6 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-07-27T15:32:06.922Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:32:06.922Z
Learning: When reviewing large refactoring PRs that change async patterns (like moving from goroutines to tea.Cmd), always check for incomplete refactoring where some call sites still use the old pattern while others use the new pattern, as this often leads to compilation errors and architectural inconsistencies.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: deleteItemWarn() in yorukot/superfile only displays a warning modal via channel messaging and does not perform actual file deletion. The actual deletion happens through getDeleteCmd() which returns a tea.Cmd. The architectural pattern separates UI modal operations (using channels/goroutines) from file I/O operations (using tea.Cmd), which is intentional and appropriate.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/function.go:211-211
Timestamp: 2025-03-29T13:20:46.467Z
Learning: Empty panel.element checks should be added at the beginning of functions that access panel.element to prevent crashes when a panel has no elements, similar to using `if len(panel.element) == 0 { return }`.
Applied to files:
src/internal/handle_file_operations.go
🧬 Code graph analysis (3)
src/internal/file_panel.go (8)
src/internal/type.go (7)
FilePanel(155-169)Element(193-198)PanelMode(21-21)SelectMode(49-49)BrowserMode(50-50)DirectoryRecord(187-190)filePanels(146-152)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/ui/rendering/renderer.go (1)
Renderer(14-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)src/internal/common/predefined_variable.go (2)
FilePanelTopDirectoryIcon(42-42)FilePanelNoneText(43-43)src/internal/model.go (1)
m(688-733)
src/internal/handle_file_operations.go (4)
src/internal/common/style_function.go (1)
GenerateRenameTextInput(276-292)src/internal/type.go (3)
SelectMode(49-49)BrowserMode(50-50)filePanels(146-152)src/config/fixed_variable.go (1)
ChooserFile(71-71)src/internal/handle_modal.go (1)
m(64-86)
src/internal/handle_panel_movement.go (4)
src/internal/handle_panel_navigation.go (1)
m(77-108)src/internal/model_navigation_test.go (1)
TestFilePanelNavigation(15-149)src/internal/model_render.go (1)
panel(110-148)src/internal/model.go (1)
m(688-733)
🔇 Additional comments (3)
src/internal/handle_panel_movement.go (1)
156-165: LGTM: public SearchBar Focus/Blur + width updateThe switch to panel.SearchBar Focus/Blur and width assignment looks correct and aligns with the new public API.
src/internal/handle_file_operations.go (2)
391-399: Chooser-file write: LGTMWriting the path then quitting matches chooser flow expectations.
403-445: Editor open flow: LGTM with sane fallbacksGuards for empty panel, chooser mode, and editor resolution look good.
| if len(panel.Element) > panelElementHeight(mainPanelHeight) { | ||
| panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) | ||
| panel.Cursor = len(panel.Element) - 1 | ||
| } else { | ||
| panel.Cursor = len(panel.Element) - 1 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap-around should reset RenderIndex for short lists
When the list fits in the viewport, wrap-up leaves a stale RenderIndex; reset to 0.
- } else {
- panel.Cursor = len(panel.Element) - 1
- }
+ } else {
+ panel.RenderIndex = 0
+ panel.Cursor = len(panel.Element) - 1
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(panel.Element) > panelElementHeight(mainPanelHeight) { | |
| panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) | |
| panel.Cursor = len(panel.Element) - 1 | |
| } else { | |
| panel.Cursor = len(panel.Element) - 1 | |
| } | |
| if len(panel.Element) > panelElementHeight(mainPanelHeight) { | |
| panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) | |
| panel.Cursor = len(panel.Element) - 1 | |
| } else { | |
| panel.RenderIndex = 0 | |
| panel.Cursor = len(panel.Element) - 1 | |
| } |
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 111 to 116, when wrapping up for
lists that fit in the viewport the code leaves a stale RenderIndex; update the
else branch to reset panel.RenderIndex to 0 (and set panel.Cursor to
len(panel.Element)-1, treating empty lists by setting both RenderIndex and
Cursor to 0) so short lists clear any previous scroll offset and render from the
top.
| panel.Cursor = 0 | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PgUp/PgDown: also reset RenderIndex when all items fit
For panHeight >= panlen, set RenderIndex = 0 for consistent view.
- if panHeight >= panlen {
- panel.Cursor = 0
+ if panHeight >= panlen {
+ panel.Cursor = 0
+ panel.RenderIndex = 0
} else {- if panHeight >= panlen {
- panel.Cursor = panlen - 1
+ if panHeight >= panlen {
+ panel.Cursor = panlen - 1
+ panel.RenderIndex = 0
} else {Also applies to: 171-172
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 145-146 (and also apply same change
at 171-172): when panHeight >= panlen you currently only reset Cursor to 0; also
set RenderIndex = 0 so the rendered window is consistent when all items fit.
Modify the branch that handles the all-items-fit case to assign RenderIndex = 0
(in addition to Cursor = 0) in both locations mentioned.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/internal/handle_file_operation_test.go (2)
107-113: Minor: cache the focused panel locally to cut repeated lookups.Readability and tiny perf win in hot tests.
- m.getFocusedFilePanel().Cursor = tt.cursor + panel := m.getFocusedFilePanel() + panel.Cursor = tt.cursor ... - m.getFocusedFilePanel().ChangeFilePanelMode() - m.getFocusedFilePanel().Selected = tt.selectedElem + panel.ChangeFilePanelMode() + panel.Selected = tt.selectedElem
133-133: Prefer accessor for location (if available).If the Element’s Location got exported or accessor added, use it to decouple tests from struct fields.
Do we have a GetSelectedItemLocation() or selectedItem.Location? If yes, switch to it; otherwise current usage is fine within package internal.
src/internal/model_render.go (2)
35-38: Simplify out-of-bounds check.Using >= reads clearer and handles empty lists naturally.
- if filePanel.Cursor > filePanel.ElementCount()-1 { + if filePanel.Cursor >= filePanel.ElementCount() { filePanel.Cursor = 0 filePanel.RenderIndex = 0 }
306-317: Guard sort-options modal for narrow widths and empty options.Avoid negative width in footer and odd “1/0” display when options are empty.
- for i, option := range panel.SortOptions.data.options { + options := panel.SortOptions.data.options + for i, option := range options { cursor := " " - if i == panel.SortOptions.cursor { + if i == panel.SortOptions.cursor { cursor = common.FilePanelCursorStyle.Render(icon.Cursor) } sortOptionsContent += cursor + common.ModalStyle.Render(" "+option) + "\n" } - bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1), - strconv.Itoa(len(panel.SortOptions.data.options))), panel.SortOptions.width-2) + width := panel.SortOptions.width + if width < 2 { width = 2 } + cursorDisp := 0 + if len(options) > 0 { cursorDisp = panel.SortOptions.cursor + 1 } + bottomBorder := common.GenerateFooterBorder( + fmt.Sprintf("%d/%d", cursorDisp, len(options)), + width-2, + ) - return common.SortOptionsModalBorderStyle(panel.SortOptions.height, panel.SortOptions.width, + return common.SortOptionsModalBorderStyle(panel.SortOptions.height, width, bottomBorder).Render(sortOptionsContent)src/internal/model.go (3)
440-441: Centralize cursor validation (optional).You clamp negative here and clamp upper bound during render. Consider a small panel.Validate() to keep both in one place (per earlier discussion about a validation helper).
Would you like me to sketch a minimal Validate() on the panel that enforces 0 <= Cursor < ElementCount when ElementCount > 0?
629-632: Clamp overlay dimensions for sort options.Avoid negative overlay offsets when width/height are tiny.
- if panel.SortOptions.open { - sortOptions := m.sortOptionsRender() - overlayX := m.fullWidth/2 - panel.SortOptions.width/2 - overlayY := m.fullHeight/2 - panel.SortOptions.height/2 + if panel.SortOptions.open { + sortOptions := m.sortOptionsRender() + w := panel.SortOptions.width + h := panel.SortOptions.height + if w < 1 { w = 1 } + if h < 1 { h = 1 } + overlayX := m.fullWidth/2 - w/2 + overlayY := m.fullHeight/2 - h/2 return stringfunction.PlaceOverlay(overlayX, overlayY, sortOptions, finalRender) }
692-716: Tighten refresh throttling for focused panels.ElementCount/100 often yields 0; clamp to ≥1s to avoid refresh bursts on large directories.
- reRenderTime := int(float64(filePanel.ElementCount()) / 100) + reRenderTime := max(1, filePanel.ElementCount()/100) ... - if filePanel.isFocused && !focusPanelReRender && - nowTime.Sub(filePanel.LastTimeGetElement) < time.Duration(reRenderTime)*time.Second { + if filePanel.isFocused && !focusPanelReRender && + nowTime.Sub(filePanel.LastTimeGetElement) < time.Duration(reRenderTime)*time.Second { continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/handle_file_operation_test.go(2 hunks)src/internal/model.go(13 hunks)src/internal/model_render.go(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/model.gosrc/internal/model_render.go
📚 Learning: 2025-03-29T10:02:04.260Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/model_render.go:653-654
Timestamp: 2025-03-29T10:02:04.260Z
Learning: A panel validation function should be implemented to ensure panel.cursor doesn't become negative or go out of bounds, but this should be addressed in a separate PR - not as part of the PR fixing crashes when opening files with the editor on an empty panel.
Applied to files:
src/internal/model_render.go
🧬 Code graph analysis (3)
src/internal/handle_file_operation_test.go (4)
src/internal/type.go (3)
BrowserMode(50-50)PanelMode(21-21)filePanels(146-152)src/internal/file_panel.go (1)
changeFilePanelMode(31-41)src/internal/test_utils.go (3)
setupPanelModeAndSelection(38-51)setFilePanelSelectedItemByName(235-240)setFilePanelSelectedItemByLocation(228-233)src/internal/handle_panel_movement.go (1)
panel(133-147)
src/internal/model.go (3)
src/internal/type.go (3)
Element(193-198)fileModel(66-110)filePanels(146-152)src/internal/handle_panel_navigation.go (2)
m(77-108)m(147-156)src/internal/handle_panel_movement.go (2)
m(179-190)m(107-122)
src/internal/model_render.go (3)
src/internal/common/style_function.go (2)
GenerateFooterBorder(321-328)SortOptionsModalBorderStyle(159-171)src/internal/type.go (1)
filePanels(146-152)src/internal/handle_panel_navigation.go (1)
m(77-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test (ubuntu-latest)
🔇 Additional comments (16)
src/internal/handle_file_operation_test.go (1)
105-105: Bounds check before cursor assignment: LGTM.Using ElementCount() to assert bounds protects later direct Cursor writes.
src/internal/model_render.go (2)
23-23: Public Location propagation: LGTM.Sidebar now receives panel.Location directly.
58-58: Resolve — FilePanel.Render existsDefined at src/internal/file_panel.go:213; the call at src/internal/model_render.go:58 is valid.
src/internal/model.go (13)
144-151: Preview: short-circuit on empty panel: LGTM.Prevents unnecessary rendering; accessor usage is consistent.
177-182: Metadata: short-circuit on empty panel: LGTM.Matches preview logic and avoids needless work.
254-254: Search bar width migration: LGTM.Width now set via SearchBar; aligns with exported field.
329-341: Debug context additions: LGTM.Good visibility into PanelMode and SearchBar focus during key handling.
369-369: Search-bar focus branch uses exported SearchBar: LGTM.
374-374: Sort options open flag wired via SortOptions: LGTM.
419-421: Rename/searchBar updates via exported fields: LGTM.
489-489: Shell command CWD from panel.Location: LGTM.
502-502: Split panel using current Location: LGTM.
506-507: Open panel relative to current: LGTM.
514-517: CD updates via panel.UpdateCurrentFilePanelDir and zoxide tracking: LGTM.
721-730: Search and element refresh use exported fields: LGTM.Consistent use of SearchBar, SortOptions, Element, and LastTimeGetElement.
744-744: Quit: persist last dir via panel.Location: LGTM.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/internal/handle_panel_movement.go (1)
30-41: Avoid reaching into selectedItem.location; use a local selectedPath via accessor.This keeps movement logic decoupled from element internals and consistent with the new public surface.
- if panel.ElementCount() == 0 { + if panel.ElementCount() == 0 { return } - selectedItem := panel.GetSelectedItem() + selectedItem := panel.GetSelectedItem() + selectedPath := panel.GetSelectedItemLocation() if selectedItem.directory { - // TODO : Propagate error out from this this function. Return here, instead of logging - err := m.updateCurrentFilePanelDir(selectedItem.location) + // TODO : Propagate error out from this this function. Return here, instead of logging + err := m.updateCurrentFilePanelDir(selectedPath) if err != nil { - slog.Error("Error while changing to directory", "error", err, "target", selectedItem.location) + slog.Error("Error while changing to directory", "error", err, "target", selectedPath) } return } - fileInfo, err := os.Lstat(selectedItem.location) + fileInfo, err := os.Lstat(selectedPath) @@ - if fileInfo.Mode()&os.ModeSymlink != 0 { - targetPath, symlinkErr := filepath.EvalSymlinks(selectedItem.location) + if fileInfo.Mode()&os.ModeSymlink != 0 { + targetPath, symlinkErr := filepath.EvalSymlinks(selectedPath)Also applies to: 42-67, 69-77
src/internal/handle_file_operations.go (1)
197-209: Snapshot clipboard slice for paste cmd closure.Protects against clipboard mutation before exec.
- copyItems := m.copyItems.items + copyItems := append([]string(nil), m.copyItems.items...)
♻️ Duplicate comments (9)
src/internal/handle_file_operations.go (3)
82-89: Snapshot selection before async delete cmd to avoid TOCTOU/aliasing.Slices alias backing arrays; selection can change before the cmd runs.
- if panel.GetPanelMode() == SelectMode { - items = panel.GetSelected() - } else { - items = []string{panel.GetSelectedItemLocation()} - } + if panel.GetPanelMode() == SelectMode { + items = append([]string(nil), panel.GetSelected()...) + } else { + loc := panel.GetSelectedItemLocation() + items = []string{loc} + }
178-184: Snapshot Selected when copying multiple items.Prevents clipboard reflecting later selection changes.
- m.copyItems.items = panel.GetSelected() + m.copyItems.items = append([]string(nil), panel.GetSelected()...)
366-371: Snapshot Selected for compress cmd and derive firstFile from the snapshot.Ensures deterministic zip source set and name.
- if len(panel.GetSelected()) == 0 { - firstFile = panel.GetSelectedItemLocation() - filesToCompress = append(filesToCompress, firstFile) - } else { - firstFile = panel.GetSelected()[0] - filesToCompress = panel.GetSelected() - } + if len(panel.GetSelected()) == 0 { + firstFile = panel.GetSelectedItemLocation() + filesToCompress = []string{firstFile} + } else { + filesToCompress = append([]string(nil), panel.GetSelected()...) + firstFile = filesToCompress[0] + }src/internal/file_panel.go (6)
51-65: Nil-map write will panic in DirectoryRecords.
Initialize on first use before assignment.Apply:
func (panel *FilePanel) UpdateCurrentFilePanelDir(path string) error { @@ - // Update the cursor and render status in case we switch back to this. - panel.DirectoryRecords[panel.Location] = DirectoryRecord{ + // Update the cursor and render status in case we switch back to this. + if panel.DirectoryRecords == nil { + panel.DirectoryRecords = make(map[string]DirectoryRecord) + } + panel.DirectoryRecords[panel.Location] = DirectoryRecord{ directoryCursor: panel.Cursor, directoryRender: panel.RenderIndex, }
101-118: Wrap-around on short lists leaves stale RenderIndex → empty render risk.
When len(Element) <= viewport, RenderIndex should be reset to 0 on wrap-up. Otherwise the render loop may start beyond end and show nothing.Apply:
} else { - if len(panel.Element) > panelElementHeight(mainPanelHeight) { + if len(panel.Element) > panelElementHeight(mainPanelHeight) { panel.RenderIndex = len(panel.Element) - panelElementHeight(mainPanelHeight) panel.Cursor = len(panel.Element) - 1 } else { + panel.RenderIndex = 0 panel.Cursor = len(panel.Element) - 1 } }
135-159: PgUp: also reset RenderIndex when all items fit.
Prevents stale scroll offset on small lists.Apply:
if panHeight >= panlen { - panel.Cursor = 0 + panel.Cursor = 0 + panel.RenderIndex = 0 } else {
161-181: PgDown: also reset RenderIndex when all items fit.
Same stale-offset issue as PgUp.Apply:
if panHeight >= panlen { - panel.Cursor = panlen - 1 + panel.Cursor = panlen - 1 + panel.RenderIndex = 0 } else {
259-297: I/O in render hot path: os.ReadDir per row is costly.
Avoid filesystem calls during rendering; rely on cached metadata (element.directory) or precompute existence. Track via a follow-up issue if out of scope.Apply minimal change to use existing metadata:
- _, err := os.ReadDir(panel.Element[i].location) - dirExists := err == nil || panel.Element[i].directory + isDir := panel.Element[i].directory @@ - dirExists, + isDir,
438-442: SelectAllItems appends without clearing; duplicates and growth risk.
Clear first so selection reflects current items exactly.Apply:
func (panel *FilePanel) SelectAllItems() { - for _, item := range panel.Element { - panel.Selected = append(panel.Selected, item.location) - } + panel.Selected = panel.Selected[:0] + for _, item := range panel.Element { + panel.Selected = append(panel.Selected, item.location) + } }
🧹 Nitpick comments (6)
src/internal/handle_file_operation_test.go (2)
107-107: Prefer helper over direct field mutation in tests.Setting Cursor via a helper (e.g., setFilePanelSelectedItemByName/Location) keeps tests agnostic to internal invariants.
109-113: Public API usage looks consistent; minor style nit.If a setter exists, prefer it over direct Selected assignment to avoid bypassing invariants. Otherwise fine.
src/internal/handle_file_operations.go (2)
433-441: Editor command parsing may break with spaces/quotes.strings.Fields() splits poorly for commands like
code --waitor paths with spaces. Optional: use a shellwords parser.Example using mattn/go-shellwords:
- parts := strings.Fields(editor) + parts, _ := shellwords.Parse(editor) cmd := parts[0]If avoiding deps, consider a separate config for args instead.
450-456: Nit: mix of getters and direct field access for Location.Prefer a single style (e.g., GetLocation()) for consistency and easier internal refactors.
Also applies to: 471-477
src/internal/file_panel.go (2)
195-209: Selection uses slice search; acceptable for now.
There’s an acknowledged TODO about set-based membership. Given scope, keep as-is; consider a map[string]struct{} if selection scales.
224-229: Truncation constant -4: document or centralize.
The magic number likely accounts for cursor/spacing/border. Consider deriving from renderer widths to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/internal/file_panel.go(2 hunks)src/internal/handle_file_operation_test.go(2 hunks)src/internal/handle_file_operations.go(13 hunks)src/internal/handle_panel_movement.go(6 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-05-22T04:42:07.369Z
Learnt from: lazysegtree
PR: yorukot/superfile#825
File: src/internal/ui/rendering/renderer_core.go:95-108
Timestamp: 2025-05-22T04:42:07.369Z
Learning: In src/internal/ui/rendering/renderer_core.go, when truncating rendered output that exceeds the maximum height, we should drop all remaining lines after reaching totalHeight without adding an ellipsis or other truncation indicator, as the goal is to have exactly totalHeight lines.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-07-27T15:32:06.922Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:32:06.922Z
Learning: When reviewing large refactoring PRs that change async patterns (like moving from goroutines to tea.Cmd), always check for incomplete refactoring where some call sites still use the old pattern while others use the new pattern, as this often leads to compilation errors and architectural inconsistencies.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: deleteItemWarn() in yorukot/superfile only displays a warning modal via channel messaging and does not perform actual file deletion. The actual deletion happens through getDeleteCmd() which returns a tea.Cmd. The architectural pattern separates UI modal operations (using channels/goroutines) from file I/O operations (using tea.Cmd), which is intentional and appropriate.
Applied to files:
src/internal/handle_file_operations.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/function.go:211-211
Timestamp: 2025-03-29T13:20:46.467Z
Learning: Empty panel.element checks should be added at the beginning of functions that access panel.element to prevent crashes when a panel has no elements, similar to using `if len(panel.element) == 0 { return }`.
Applied to files:
src/internal/handle_file_operations.go
🧬 Code graph analysis (4)
src/internal/file_panel.go (7)
src/internal/type.go (6)
FilePanel(155-169)Element(193-198)PanelMode(21-21)DirectoryRecord(187-190)filePanels(146-152)fileModel(66-110)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/ui/rendering/renderer.go (1)
Renderer(14-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)src/internal/model.go (1)
m(688-733)
src/internal/handle_file_operation_test.go (3)
src/internal/type.go (3)
BrowserMode(50-50)PanelMode(21-21)filePanels(146-152)src/internal/handle_panel_up_down_test.go (1)
Test_filePanelUpDown(9-127)src/internal/test_utils.go (3)
setupPanelModeAndSelection(38-51)setFilePanelSelectedItemByName(235-240)setFilePanelSelectedItemByLocation(228-233)
src/internal/handle_panel_movement.go (2)
src/internal/handle_panel_navigation.go (2)
m(77-108)m(110-132)src/internal/model_navigation_test.go (1)
TestFilePanelNavigation(15-149)
src/internal/handle_file_operations.go (3)
src/internal/common/style_function.go (1)
GenerateRenameTextInput(276-292)src/internal/type.go (2)
SelectMode(49-49)BrowserMode(50-50)src/config/fixed_variable.go (1)
ChooserFile(71-71)
🔇 Additional comments (27)
src/internal/handle_file_operation_test.go (2)
105-105: Good switch to public API guard.ElementCount() precondition keeps the test resilient to empty panels.
133-135: LGTM: assert via GetSelectedItemLocation().Clear intent and decoupled from internals.
src/internal/handle_panel_movement.go (2)
19-23: LGTM: delegate parent navigation to FilePanel.Cleaner responsibility and error surfaced.
80-104: LGTM: use GetSelectedItemLocation() for open commands.Consistent across platforms; avoids internal field access.
src/internal/handle_file_operations.go (5)
62-73: Rename UX looks correct; cursor placement logic is fine.Good use of ElementCount() guard and GenerateRenameTextInput.
166-172: LGTM: single-item copy uses accessor and appends safely.
319-323: LGTM: extract cmd guarded and uses accessor.
382-382: LGTM: zip path built from panel.GetLocation().
406-417: Chooser file flow before editor open is correct.Guard + error handling are consistent with enterPanel().
src/internal/file_panel.go (18)
21-26: LGTM: Safe selected-item accessor.
Bounds check avoids panics; return of zero-value Element is fine.
28-30: LGTM: Efficient selection reset.
Re-slicing to length 0 preserves capacity; good choice.
32-37: Pointer-return caveat: avoid retaining across refresh.
Returning a pointer to a slice element is fine if not stored beyond a refresh that may reallocate. Verify callers don’t persist it.
39-49: Mode toggle clears selection only when leaving Select mode—intentional?
Currently, switching SelectMode -> BrowserMode clears Selected; entering SelectMode does not. Confirm UX.
74-85: Cache restore can be stale; clamp or validate on next update.
You already noted this in the TODO. Ensure Cursor/RenderIndex are validated against current Element length during the next refresh to avoid blank views.
95-97: LGTM: ParentDirectory delegates correctly.
Clear and idiomatic.
120-133: LGTM: ListDown wrap-around resets scroll correctly.
RenderIndex and Cursor reset to top; consistent.
183-191: LGTM: Select-then-move behavior.
Order of selection then navigation is consistent and predictable.
213-223: LGTM: Render orchestration via renderer.
Good separation of concerns.
299-313: LGTM: Sort info formatting.
Date label normalization is a nice touch.
315-325: LGTM: Panel mode labeling.
Simple and clear.
327-333: LGTM: Cursor string off-by-one handled.
Correct 1-based display.
336-398: Accessors look consistent.
No hidden side effects; fine for public surface.
402-417: Rename conflict on case-insensitive filesystems.
String equality may miss conflicts like “foo”→“FOO” on Windows/macOS volumes. Consider a platform-aware check (or attempt os.Stat with a casefolded compare).
419-429: LGTM: Clipboard helpers.
Errors are surfaced; no-op on empty panel is acceptable.
431-437: LGTM: Safe selected-item location accessor.
Returns empty string when no items; OK.
444-466: LGTM: Rename flow and state reset.
Error surfaced, Blur+Renaming=false on success is correct. Consider integrating IsRenamingConflicting() at call sites.
470-498: LGTM: Sort options state transitions.
Open/Cancel/Confirm and cursor wrap look correct.
| func (panel *FilePanel) renderFooter(r *rendering.Renderer) { | ||
| sortLabel, sortIcon := panel.getSortInfo() | ||
| modeLabel, modeIcon := panel.getPanelModeInfo() | ||
| cursorStr := panel.getCursorString() | ||
|
|
||
| if common.Config.Nerdfont { | ||
| sortLabel = sortIcon + icon.Space + sortLabel | ||
| modeLabel = modeIcon + icon.Space + modeLabel | ||
| } else { | ||
| // TODO : Figure out if we can set icon.Space to " " if nerdfont is false | ||
| // That would simplify code | ||
| sortLabel = sortIcon + " " + sortLabel | ||
| } | ||
|
|
||
| if common.Config.ShowPanelFooterInfo { | ||
| r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr) | ||
| if r.AreInfoItemsTruncated() { | ||
| r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) | ||
| } | ||
| } else { | ||
| r.SetBorderInfoItems(cursorStr) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Non-nerdfont path still prepends sort icon; mode label inconsistent.
In the else-branch, sortLabel still includes an icon and modeLabel drops it; terminals without Nerd Font will show tofu. Drop icons when Nerdfont=false and provide a short-text fallback when truncated.
Apply:
if common.Config.Nerdfont {
sortLabel = sortIcon + icon.Space + sortLabel
modeLabel = modeIcon + icon.Space + modeLabel
} else {
- // TODO : Figure out if we can set icon.Space to " " if nerdfont is false
- // That would simplify code
- sortLabel = sortIcon + " " + sortLabel
+ // No Nerd Font: avoid icons entirely
+ // Leave labels as-is
}And handle truncated fallback without icons:
- if common.Config.ShowPanelFooterInfo {
- r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr)
- if r.AreInfoItemsTruncated() {
- r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr)
- }
- } else {
+ if common.Config.ShowPanelFooterInfo {
+ r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr)
+ if r.AreInfoItemsTruncated() {
+ if common.Config.Nerdfont {
+ r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr)
+ } else {
+ r.SetBorderInfoItems("Sort", "Mode", cursorStr)
+ }
+ }
+ } else {
r.SetBorderInfoItems(cursorStr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (panel *FilePanel) renderFooter(r *rendering.Renderer) { | |
| sortLabel, sortIcon := panel.getSortInfo() | |
| modeLabel, modeIcon := panel.getPanelModeInfo() | |
| cursorStr := panel.getCursorString() | |
| if common.Config.Nerdfont { | |
| sortLabel = sortIcon + icon.Space + sortLabel | |
| modeLabel = modeIcon + icon.Space + modeLabel | |
| } else { | |
| // TODO : Figure out if we can set icon.Space to " " if nerdfont is false | |
| // That would simplify code | |
| sortLabel = sortIcon + " " + sortLabel | |
| } | |
| if common.Config.ShowPanelFooterInfo { | |
| r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr) | |
| if r.AreInfoItemsTruncated() { | |
| r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) | |
| } | |
| } else { | |
| r.SetBorderInfoItems(cursorStr) | |
| } | |
| } | |
| func (panel *FilePanel) renderFooter(r *rendering.Renderer) { | |
| sortLabel, sortIcon := panel.getSortInfo() | |
| modeLabel, modeIcon := panel.getPanelModeInfo() | |
| cursorStr := panel.getCursorString() | |
| if common.Config.Nerdfont { | |
| sortLabel = sortIcon + icon.Space + sortLabel | |
| modeLabel = modeIcon + icon.Space + modeLabel | |
| } else { | |
| // No Nerd Font: avoid icons entirely | |
| // Leave labels as-is | |
| } | |
| if common.Config.ShowPanelFooterInfo { | |
| r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr) | |
| if r.AreInfoItemsTruncated() { | |
| if common.Config.Nerdfont { | |
| r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) | |
| } else { | |
| r.SetBorderInfoItems("Sort", "Mode", cursorStr) | |
| } | |
| } | |
| } else { | |
| r.SetBorderInfoItems(cursorStr) | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/internal/file_panel.go around lines 235 to 257, the non-nerdfont branch
still prepends icon glyphs (causing tofu) and modeLabel becomes inconsistent;
change the else branch to omit icons entirely and build labels using plain-text
fallbacks (e.g., short words) with proper spacing, and when handling truncated
info items call SetBorderInfoItems with the plain-text short-labels (no icons)
so the truncated fallback is also icon-free.
| // Focus on search bar | ||
| func (m *model) searchBarFocus() { | ||
| panel := &m.fileModel.filePanels[m.filePanelFocusIndex] | ||
| if panel.searchBar.Focused() { | ||
| panel.searchBar.Blur() | ||
| if panel.SearchBar.Focused() { | ||
| panel.SearchBar.Blur() | ||
| } else { | ||
| panel.searchBar.Focus() | ||
| panel.SearchBar.Focus() | ||
| m.firstTextInput = true | ||
| } | ||
|
|
||
| // config search bar width | ||
| panel.searchBar.Width = m.fileModel.width - 4 | ||
| panel.SearchBar.Width = m.fileModel.width - 4 | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all call sites updated to SearchBar (capital S).
Other files (e.g., handle_panel_navigation.go in snippets) still reference searchBar which will break after this rename.
Run to find stragglers:
🏁 Script executed:
#!/bin/bash
rg -nP --type=go -C2 '\.\s*searchBar\b' || true
rg -nP --type=go -C2 '\bGetSelectedItem\(\)\.(location|name|directory)\b' || trueLength of output: 7237
Update all call sites to SearchBar (capital S)
Multiple remaining .searchBar references found — update them to .SearchBar (will break the build if not fixed).
Files/locations to change:
- src/internal/model_render.go:208 (m.helpMenu.searchBar.View())
- src/internal/key_function.go:319, 322, 324, 340
- src/internal/handle_modal.go:214, 226
- src/internal/model.go:73, 74, 294
- src/internal/ui/sidebar/utils.go:45–56
- src/internal/ui/sidebar/sidebar.go:72–75, 86–89, 97–102, 118–122
- src/internal/ui/sidebar/render.go:26, 64
Also: GetSelectedItem().(location|name|directory) usages found — verify those fields if renamed:
- src/internal/file_panel.go:197, 408, 424, 435, 450
🤖 Prompt for AI Agents
In src/internal/handle_panel_movement.go around lines 153–165 and across the
repo files listed (src/internal/model_render.go:208;
src/internal/key_function.go:319,322,324,340;
src/internal/handle_modal.go:214,226; src/internal/model.go:73,74,294;
src/internal/ui/sidebar/utils.go:45–56;
src/internal/ui/sidebar/sidebar.go:72–75,86–89,97–102,118–122;
src/internal/ui/sidebar/render.go:26,64) replace all usages of .searchBar with
.SearchBar (capital S) so code references the exported field, and update the
GetSelectedItem().(location|name|directory) casts in src/internal/file_panel.go
at lines ~197,408,424,435,450 to use the actual field names currently defined on
the returned type (adjust naming if those fields were renamed); after changes,
run go build and unit tests to catch any remaining references or mismatched
field names and fix accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/internal/file_panel.go (6)
111-117: Wrap-around should reset RenderIndex when list fits viewport.Keeps view consistent for short lists.
} else { - panel.Cursor = len(panel.Element) - 1 + panel.RenderIndex = 0 + panel.Cursor = len(panel.Element) - 1 }
145-147: PgUp/PgDown: also reset RenderIndex when all items fit.Prevents stale scroll offset.
if panHeight >= panlen { - panel.Cursor = 0 + panel.Cursor = 0 + panel.RenderIndex = 0 } else {if panHeight >= panlen { - panel.Cursor = panlen - 1 + panel.Cursor = panlen - 1 + panel.RenderIndex = 0 } else {Also applies to: 171-173
241-248: Non-Nerd Font fallback still prepends icons; use plain-text labels.Prevents tofu in terminals without Nerd Font; also fix truncated fallback.
if common.Config.Nerdfont { sortLabel = sortIcon + icon.Space + sortLabel modeLabel = modeIcon + icon.Space + modeLabel } else { - // TODO : Figure out if we can set icon.Space to " " if nerdfont is false - // That would simplify code - sortLabel = sortIcon + " " + sortLabel + // No Nerd Font: omit icons entirely + // Leave labels as plain text }if common.Config.ShowPanelFooterInfo { r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr) if r.AreInfoItemsTruncated() { - r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) + if common.Config.Nerdfont { + r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) + } else { + r.SetBorderInfoItems("Sort", "Mode", cursorStr) + } } } else {Also applies to: 250-255
283-287: I/O in render path: drop os.ReadDir; rely on cached metadata.This blocks rendering and scales poorly. Use Element.directory (and update during RefreshData).
- // Performance TODO: Remove or cache this if not needed at render time - // This will unnecessarily slow down rendering. There should be a way to avoid this at render - _, err := os.ReadDir(panel.Element[i].location) - dirExists := err == nil || panel.Element[i].directory + // Avoid I/O in render path; rely on element metadata + dirExists := panel.Element[i].directoryI can open an issue to track moving existence checks to RefreshData/cache—want me to file it?
453-457: SelectAllItems appends without clearing; leads to duplicates and growth.Reset before populating to reflect current items exactly.
func (panel *FilePanel) SelectAllItems() { - for _, item := range panel.Element { - panel.Selected = append(panel.Selected, item.location) - } + panel.Selected = panel.Selected[:0] + for _, item := range panel.Element { + panel.Selected = append(panel.Selected, item.location) + } }
60-66: Initialize DirectoryRecords before writing to avoid panic.Writing to a nil map will panic on first call.
Apply:
// Update the cursor and render status in case we switch back to this. + if panel.DirectoryRecords == nil { + panel.DirectoryRecords = make(map[string]DirectoryRecord) + } panel.DirectoryRecords[panel.Location] = DirectoryRecord{ directoryCursor: panel.Cursor, directoryRender: panel.RenderIndex, }
🧹 Nitpick comments (3)
src/internal/file_panel.go (3)
152-158: Clamp RenderIndex to viewport range to keep pages full.Avoids under-filled pages when centering near ends.
- panel.Cursor -= panHeight - panel.RenderIndex = panel.Cursor - panCenter - - if panel.RenderIndex < 0 { - panel.RenderIndex = 0 - } + panel.Cursor -= panHeight + panel.RenderIndex = panel.Cursor - panCenter + if panel.RenderIndex < 0 { + panel.RenderIndex = 0 + } + maxRI := panlen - panHeight + if panel.RenderIndex > maxRI { + panel.RenderIndex = maxRI + }- panel.Cursor = panlen - 1 - panel.RenderIndex = panel.Cursor - panCenter + panel.Cursor = panlen - 1 + panel.RenderIndex = panel.Cursor - panCenter + if panel.RenderIndex < 0 { + panel.RenderIndex = 0 + } + maxRI := panlen - panHeight + if panel.RenderIndex > maxRI { + panel.RenderIndex = maxRI + }- panel.Cursor += panHeight - panel.RenderIndex = panel.Cursor - panCenter + panel.Cursor += panHeight + panel.RenderIndex = panel.Cursor - panCenter + if panel.RenderIndex < 0 { + panel.RenderIndex = 0 + } + maxRI := panlen - panHeight + if panel.RenderIndex > maxRI { + panel.RenderIndex = maxRI + }Also applies to: 174-180
266-272: Avoid O(n·m) lookups by building a set of selected paths once.Reduces render-path cost when many items are selected.
end := min(panel.RenderIndex+panelElementHeight(mainPanelHeight), len(panel.Element)) + // Build a set for O(1) membership checks + selectedSet := make(map[string]struct{}, len(panel.Selected)) + for _, p := range panel.Selected { + selectedSet[p] = struct{}{} + } + for i := panel.RenderIndex; i < end; i++ { - // TODO : Fix this, this is O(n^2) complexity. Considered a file panel with 200 files, and 100 selected - // We will be doing a search in 100 item slice for all 200 files. - isSelected := arrayContains(panel.Selected, panel.Element[i].location) + _, isSelected := selectedSet[panel.Element[i].location]
102-160: Nested conditionals in ListUp/PgUp.Consider small helpers (clamp, allItemsFit) to flatten branches; keep behavior identical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/file_panel.go(2 hunks)src/internal/model.go(13 hunks)src/internal/model_file_operations_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-05-22T04:42:07.369Z
Learnt from: lazysegtree
PR: yorukot/superfile#825
File: src/internal/ui/rendering/renderer_core.go:95-108
Timestamp: 2025-05-22T04:42:07.369Z
Learning: In src/internal/ui/rendering/renderer_core.go, when truncating rendered output that exceeds the maximum height, we should drop all remaining lines after reaching totalHeight without adding an ellipsis or other truncation indicator, as the goal is to have exactly totalHeight lines.
Applied to files:
src/internal/file_panel.go
🧬 Code graph analysis (3)
src/internal/model_file_operations_test.go (2)
src/internal/type.go (1)
Element(193-198)src/internal/handle_file_operations.go (1)
m(78-94)
src/internal/model.go (4)
src/internal/type.go (2)
fileModel(66-110)filePanels(146-152)src/internal/handle_panel_navigation.go (1)
m(77-108)src/internal/handle_panel_movement.go (2)
m(179-190)m(107-122)src/internal/model_render.go (1)
panel(110-148)
src/internal/file_panel.go (5)
src/internal/type.go (6)
FilePanel(155-169)Element(193-198)PanelMode(21-21)SelectMode(49-49)BrowserMode(50-50)DirectoryRecord(187-190)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)
🔇 Additional comments (18)
src/internal/model_file_operations_test.go (4)
41-41: Test correctly uses new public API.The migration to
GetFirstElementName()properly follows the new public API pattern and maintains test functionality.
148-149: Test properly uses public Rename field.The test update to access
Renamedirectly aligns with the public API migration and maintains the expected test behavior.
155-155: Test uses public Element field correctly.The update to reference
Elementinstead of the internalelementfield follows the public API pattern consistently.
181-181: Consistent public API usage in test assertions.The test properly references the public
Elementfield, maintaining consistency with the migration objectives.src/internal/model.go (13)
144-149: Public API method usage follows migration pattern.The updates to use
ElementCount()andGetSelectedItem()properly utilize the new public interface and maintain the existing functionality.
177-181: Metadata command uses public API correctly.The migration to
ElementCount()andGetSelectedItem()maintains the metadata fetching logic while using the new public interface.
254-255: File panel sizing uses public SearchBar field.The update to use
SearchBar.Widthinstead ofsearchBar.Widthfollows the public field migration pattern correctly.
331-336: Logging uses public fields consistently.The debug logging updates to reference
PanelModeandSearchBar.Focused()maintain debugging functionality while using the public API.
368-373: Key handling uses public API appropriately.The updates to use
SearchBar.Focused()andSortOptions.openmaintain the key handling logic while following the public field migration.
418-420: Text input updates use public fields correctly.The migration to
Rename.Update()andSearchBar.Update()maintains the text input functionality using the public API.
428-428: Current working directory tracking uses public Location field.The update to use
Locationinstead of internal location field maintains directory tracking functionality.
439-441: Cursor bounds checking uses public Cursor field.The defensive cursor validation uses the public
Cursorfield appropriately while maintaining safety checks.
488-502: Action handling uses public Location field consistently.The updates to use
Locationin shell command execution and panel creation maintain functionality while following the public API pattern.
513-517: Directory update uses public method correctly.The migration to
UpdateCurrentFilePanelDir()method properly delegates to the file panel's public interface and maintains directory tracking.
628-632: Sort options overlay uses public SortOptions fields.The overlay positioning logic correctly uses the public
SortOptions.open,SortOptions.width, andSortOptions.heightfields.
693-717: File panel refresh logic uses public API effectively.The updates to use
LastTimeGetElement,ElementCount(),GetFirstElementLocation(), andLocationmaintain the refresh logic while utilizing the public interface. TheRefreshData()method call properly delegates to the file panel's public API.
731-731: Directory tracking on quit uses public Location field.The update to use
Locationfor current directory tracking maintains the quit functionality while following the public field migration.src/internal/file_panel.go (1)
1-18: Nice consolidation.The separation of FilePanel logic into a single file reads cleanly and keeps related behaviors together.
lazysegtree
force-pushed
the
filepanel-separation
branch
from
a72a5f7 to
dfea3fc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
(3 files improve in Code Health)
Gates Failed
(2 hotspots with Code Duplication, Overall Code Complexity)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| handle_file_operations.go | 2 rules in this hotspot | 8.09 → 7.16 | Suppress |
| model_render.go | 1 rule in this hotspot | 8.66 → 8.20 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| file_panel.go | 1 critical rule | 10.00 → 9.24 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| handle_panel_movement.go | 9.31 → 9.61 | Complex Conditional |
| handle_panel_up_down.go | 9.84 → nu | Bumpy Road Ahead |
| key_function.go | 8.06 → 8.12 | Complex Method, Overall Code Complexity |
| model.go | no change | Complex Method |
| model_render.go | 8.66 → 8.20 | Bumpy Road Ahead |
| model_navigation_test.go | 9.41 → 9.42 | Large Method |
Quality Gate Profile: The Bare Minimum
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/handle_panel_navigation.go (1)
62-67: Bug: focus shifts to the neighbor panel, not the newly created oneAfter append, focusing index+1 targets an existing panel when focus isn’t at the end. Focus should move to the newly appended panel.
- m.fileModel.filePanels[m.filePanelFocusIndex].isFocused = false - m.fileModel.filePanels[m.filePanelFocusIndex+1].isFocused = returnFocusType(m.focusPanel) + m.fileModel.filePanels[m.filePanelFocusIndex].isFocused = false + newIdx := len(m.fileModel.filePanels) - 1 + m.fileModel.filePanels[newIdx].isFocused = returnFocusType(m.focusPanel) @@ - m.filePanelFocusIndex++ + m.filePanelFocusIndex = newIdx
♻️ Duplicate comments (7)
src/internal/file_panel.go (7)
112-118: Wrap-around up: reset RenderIndex for short lists.Avoids stale scroll offset when everything fits the viewport.
Apply:
} else { - panel.Cursor = len(panel.Element) - 1 + panel.RenderIndex = 0 + panel.Cursor = len(panel.Element) - 1 }
145-151: PgUp: also reset RenderIndex when all items fit.Keeps view consistent in small directories.
Apply:
- if panHeight >= panlen { - panel.Cursor = 0 + if panHeight >= panlen { + panel.Cursor = 0 + panel.RenderIndex = 0 } else {
171-176: PgDown: also reset RenderIndex when all items fit.Match PgUp behavior for symmetry.
Apply:
- if panHeight >= panlen { - panel.Cursor = panlen - 1 + if panHeight >= panlen { + panel.Cursor = panlen - 1 + panel.RenderIndex = 0 } else {
241-257: Non-Nerd Font: drop icon glyphs; add plain-text fallback when truncated.Prevents tofu on terminals without Nerd Font and keeps footer readable.
Apply:
if common.Config.Nerdfont { sortLabel = sortIcon + icon.Space + sortLabel modeLabel = modeIcon + icon.Space + modeLabel } else { - // TODO : Figure out if we can set icon.Space to " " if nerdfont is false - // That would simplify code - sortLabel = sortIcon + " " + sortLabel + // No Nerd Font: avoid icons entirely } if common.Config.ShowPanelFooterInfo { r.SetBorderInfoItems(sortLabel, modeLabel, cursorStr) if r.AreInfoItemsTruncated() { - r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) + if common.Config.Nerdfont { + r.SetBorderInfoItems(sortIcon, modeIcon, cursorStr) + } else { + r.SetBorderInfoItems("Sort", "Mode", cursorStr) + } } } else {
283-287: I/O in render path: remove os.ReadDir from rendering.This blocks rendering; cache dirExists or compute during data refresh.
Would you like me to open a follow-up issue to move this check to RefreshData() and cache a boolean per Element?
453-457: SelectAllItems duplicates selection over time.Clear first to avoid growth and duplicates.
Apply:
func (panel *FilePanel) SelectAllItems() { + panel.Selected = panel.Selected[:0] for _, item := range panel.Element { panel.Selected = append(panel.Selected, item.location) } }
62-66: Nil map write: initialize DirectoryRecords before assign.Writing to a nil map panics.
Apply:
// NOTE: This could be a configurable feature // Update the cursor and render status in case we switch back to this. + if panel.DirectoryRecords == nil { + panel.DirectoryRecords = make(map[string]DirectoryRecord) + } panel.DirectoryRecords[panel.Location] = DirectoryRecord{ directoryCursor: panel.Cursor, directoryRender: panel.RenderIndex, }
🧹 Nitpick comments (7)
src/internal/model_navigation_test.go (2)
43-45: Fix Windows root path for robustnessUse the current drive’s root (e.g., C:) instead of "" to avoid ambiguity on Windows.
- if runtime.GOOS == utils.OsWindows { - rootDir = "\\" - } + if runtime.GOOS == utils.OsWindows { + rootDir = filepath.VolumeName(curTestDir) + "\\" + }
99-105: Potential flake: unquoted cd path with spacesThe test builds cd commands without quoting. If temp paths contain spaces (common on Windows), parsing may fail. Consider guarding for spaces (skip or adjust) until the cd parser supports quotes.
Also applies to: 113-117
src/internal/handle_panel_movement.go (3)
34-41: Prefer panel helper for location to avoid field couplingUse GetSelectedItemLocation() instead of selectedItem.location for consistency.
- selectedItem := panel.GetSelectedItem() - if selectedItem.directory { + selectedItem := panel.GetSelectedItem() + if selectedItem.directory { // TODO : Propagate error out from this this function. Return here, instead of logging - err := m.updateCurrentFilePanelDir(selectedItem.location) + err := m.updateCurrentFilePanelDir(panel.GetSelectedItemLocation()) if err != nil { - slog.Error("Error while changing to directory", "error", err, "target", selectedItem.location) + slog.Error("Error while changing to directory", "error", err, "target", panel.GetSelectedItemLocation()) } return }
42-58: Consistently use accessor for selected item locationKeeps call sites resilient to internal element changes.
- fileInfo, err := os.Lstat(selectedItem.location) + fileInfo, err := os.Lstat(panel.GetSelectedItemLocation()) @@ - if fileInfo.Mode()&os.ModeSymlink != 0 { - targetPath, symlinkErr := filepath.EvalSymlinks(selectedItem.location) + if fileInfo.Mode()&os.ModeSymlink != 0 { + targetPath, symlinkErr := filepath.EvalSymlinks(panel.GetSelectedItemLocation())
90-104: Nit: log message grammarMinor wording fix for clarity.
- if err != nil { - slog.Error("Error while open file with", "error", err) - } + if err != nil { + slog.Error("Error while opening file", "error", err) + } @@ - if err != nil { - slog.Error("Error while open file with", "error", err) - } + if err != nil { + slog.Error("Error while opening file", "error", err) + }src/internal/model.go (1)
709-716: Throttle fix: avoid 0-second window causing constant refresh on small dirs.When ElementCount < 100, reRenderTime=0, so the check never skips refresh; clamp to ≥1s (tune as needed).
Apply:
- reRenderTime := int(float64(filePanel.ElementCount()) / 100) + reRenderSecs := filePanel.ElementCount() / 100 + if reRenderSecs < 1 { + reRenderSecs = 1 + } - if filePanel.isFocused && !focusPanelReRender && - nowTime.Sub(filePanel.LastTimeGetElement) < time.Duration(reRenderTime)*time.Second { + if filePanel.isFocused && !focusPanelReRender && + nowTime.Sub(filePanel.LastTimeGetElement) < time.Duration(reRenderSecs)*time.Second { continue }src/internal/file_panel.go (1)
532-534: Width guard for search bar.If filePanelWidth < 4, Width becomes negative. Clamp at zero to avoid visual glitches.
Apply:
func (panel *FilePanel) SetSearchBarWidth(width int) { - panel.SearchBar.Width = width - 4 + w := width - 4 + if w < 0 { + w = 0 + } + panel.SearchBar.Width = w }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/internal/file_panel.go(2 hunks)src/internal/handle_panel_movement.go(6 hunks)src/internal/handle_panel_navigation.go(5 hunks)src/internal/model.go(13 hunks)src/internal/model_navigation_test.go(2 hunks)src/internal/model_render.go(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
PR: yorukot/superfile#1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
PR: yorukot/superfile#1040
File: src/internal/function.go:58-58
Timestamp: 2025-09-05T08:04:14.324Z
Learning: lazysegtree asked about "dual-panel functionality" and "secondary focus" in the superfile codebase. The system actually supports multiple file panels (not specifically dual-panel), where users can create/close panels and navigate between them. The old `secondFocus` state was used to mark non-active panels when multiple panels were open, not for a specific dual-panel feature. The focus states were: noneFocus (no file panel focused), focus (active panel), and secondFocus (inactive panels).
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
PR: yorukot/superfile#1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/file_panel.gosrc/internal/model_navigation_test.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
PR: yorukot/superfile#770
File: src/internal/ui/sidebar/render.go:0-0
Timestamp: 2025-04-12T13:51:24.691Z
Learning: In the sidebar component of yorukot/superfile, the partial list rendering in render.go is intentional. The sidebar implements scrolling functionality where ListUp/ListDown methods in navigation.go update cursor position and renderIndex to show the appropriate portion of the directory list based on available space.
Applied to files:
src/internal/file_panel.gosrc/internal/model_render.go
📚 Learning: 2025-05-22T04:42:07.369Z
Learnt from: lazysegtree
PR: yorukot/superfile#825
File: src/internal/ui/rendering/renderer_core.go:95-108
Timestamp: 2025-05-22T04:42:07.369Z
Learning: In src/internal/ui/rendering/renderer_core.go, when truncating rendered output that exceeds the maximum height, we should drop all remaining lines after reaching totalHeight without adding an ellipsis or other truncation indicator, as the goal is to have exactly totalHeight lines.
Applied to files:
src/internal/file_panel.go
📚 Learning: 2025-03-29T10:02:04.260Z
Learnt from: lazysegtree
PR: yorukot/superfile#730
File: src/internal/model_render.go:653-654
Timestamp: 2025-03-29T10:02:04.260Z
Learning: A panel validation function should be implemented to ensure panel.cursor doesn't become negative or go out of bounds, but this should be addressed in a separate PR - not as part of the PR fixing crashes when opening files with the editor on an empty panel.
Applied to files:
src/internal/model_render.go
🧬 Code graph analysis (5)
src/internal/file_panel.go (5)
src/internal/type.go (6)
FilePanel(155-169)Element(193-198)PanelMode(21-21)SelectMode(49-49)BrowserMode(50-50)DirectoryRecord(187-190)src/config/icon/icon.go (7)
Cursor(31-31)Error(34-34)Space(10-10)SortAsc(40-40)SortDesc(41-41)Browser(32-32)Select(33-33)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/ui/spf_renderers.go (1)
FilePanelRenderer(34-54)src/internal/common/string_function.go (2)
TruncateTextBeginning(28-47)PrettierName(61-74)
src/internal/model_navigation_test.go (3)
src/internal/test_utils.go (1)
TeaUpdateWithErrCheck(65-68)src/internal/utils/tea_utils.go (1)
TeaRuneKeyMsg(5-10)src/internal/handle_panel_up_down_test.go (1)
Test_filePanelUpDown(9-127)
src/internal/model.go (3)
src/internal/type.go (3)
PanelMode(21-21)filePanels(146-152)fileModel(66-110)src/internal/utils/file_utils.go (1)
ResolveAbsPath(186-199)src/internal/handle_modal.go (1)
m(99-103)
src/internal/model_render.go (4)
src/internal/common/style.go (2)
FilePanelCursorStyle(30-30)ModalStyle(20-20)src/config/icon/icon.go (1)
Cursor(31-31)src/internal/common/style_function.go (2)
GenerateFooterBorder(321-328)SortOptionsModalBorderStyle(159-171)src/internal/ui/sidebar/render.go (1)
s(38-79)
src/internal/handle_panel_navigation.go (3)
src/internal/ui/sidebar/directory_utils.go (1)
TogglePinnedDirectory(121-151)src/internal/type.go (3)
FilePanel(155-169)PanelMode(21-21)DirectoryRecord(187-190)src/internal/type_utils.go (1)
defaultFilePanel(49-73)
🔇 Additional comments (14)
src/internal/model_navigation_test.go (1)
125-126: LGTM: migrated to public panel APIGood move to Cursor/RenderIndex/Location/SearchBar.Value() and verifying state restoration.
Also applies to: 131-131, 134-134, 143-145
src/internal/handle_panel_movement.go (3)
19-23: LGTM: delegate parent navigation to panel APISwitch to ParentDirectory() looks correct and keeps model light.
30-34: LGTM: safe-guard empty panel before selectionElementCount() check prevents OOB; GetSelectedItem() aligns with new API.
153-165: LGTM: SearchBar Focus/Blur via public APIFocus/Blur and SetSearchBarWidth are correctly routed through the panel.
src/internal/handle_panel_navigation.go (3)
19-23: LGTM: pinned directory uses public LocationMatches the exported FilePanel field.
43-50: LGTM: new FilePanel constructionInitializing Location/SortOptions/PanelMode/DirectoryRecords/SearchBar aligns with the new public surface.
70-73: LGTM: width propagation through SetSearchBarWidthUpdating all panels via the public setter avoids direct field pokes and keeps layout consistent.
Also applies to: 105-107, 129-131
src/internal/model_render.go (2)
35-36: Good encapsulation: centralizing index validation.Calling ResetIndexIfInvalid() here is the right place and prevents scattered bounds “duct tape.”
626-630: Overlay sizing reads well with exported SortOptions fields.Positioning via width/height is consistent with SortOptionsModalBorderStyle. No issues.
src/internal/model.go (4)
144-148: Early-return on empty selection for preview is correct.Prevents unnecessary work and avoids nil/OOBe states when no elements exist.
177-184: Metadata blanking on empty panel is correct.Keeps UI consistent without triggering fetches for non-existent items.
254-255: Nice: setter for search bar width.Shifts mutation behind an API and keeps sizing logic centralized.
331-337: Consistent adoption of exported FilePanel API/fields.PanelMode, SearchBar, SortOptions, Location, and ResetIndexIfInvalid() usage is coherent across input/update flows.
Also applies to: 369-375, 419-421, 428-435, 439-441, 487-487, 500-501, 505-515, 730-730
src/internal/file_panel.go (1)
526-530: Bounds reset helper is good.Centralizes recovery from invalid cursor/render after data reloads.
| func (m *model) sortOptionsRender() string { | ||
| panel := m.fileModel.filePanels[m.filePanelFocusIndex] | ||
| sortOptionsContent := common.ModalTitleStyle.Render(" Sort Options") + "\n\n" | ||
| for i, option := range panel.sortOptions.data.options { | ||
| for i, option := range panel.SortOptions.data.options { | ||
| cursor := " " | ||
| if i == panel.sortOptions.cursor { | ||
| if i == panel.SortOptions.cursor { | ||
| cursor = common.FilePanelCursorStyle.Render(icon.Cursor) | ||
| } | ||
| sortOptionsContent += cursor + common.ModalStyle.Render(" "+option) + "\n" | ||
| } | ||
| bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.sortOptions.cursor+1), | ||
| strconv.Itoa(len(panel.sortOptions.data.options))), panel.sortOptions.width-2) | ||
| bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1), | ||
| strconv.Itoa(len(panel.SortOptions.data.options))), panel.SortOptions.width-2) | ||
|
|
||
| return common.SortOptionsModalBorderStyle(panel.sortOptions.height, panel.sortOptions.width, | ||
| return common.SortOptionsModalBorderStyle(panel.SortOptions.height, panel.SortOptions.width, | ||
| bottomBorder).Render(sortOptionsContent) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against empty options and out-of-range cursor.
sortOptionsRender will panic if options is empty; also reuse a local slice to avoid repeated lookups.
Apply:
func (m *model) sortOptionsRender() string {
panel := m.fileModel.filePanels[m.filePanelFocusIndex]
sortOptionsContent := common.ModalTitleStyle.Render(" Sort Options") + "\n\n"
- for i, option := range panel.SortOptions.data.options {
+ opts := panel.SortOptions.data.options
+ if len(opts) == 0 {
+ return ""
+ }
+ if panel.SortOptions.cursor >= len(opts) {
+ panel.SortOptions.cursor = 0
+ }
+ for i, option := range opts {
cursor := " "
- if i == panel.SortOptions.cursor {
+ if i == panel.SortOptions.cursor {
cursor = common.FilePanelCursorStyle.Render(icon.Cursor)
}
sortOptionsContent += cursor + common.ModalStyle.Render(" "+option) + "\n"
}
- bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1),
- strconv.Itoa(len(panel.SortOptions.data.options))), panel.SortOptions.width-2)
+ bottomBorder := common.GenerateFooterBorder(
+ fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1), strconv.Itoa(len(opts))),
+ panel.SortOptions.width-2)
return common.SortOptionsModalBorderStyle(panel.SortOptions.height, panel.SortOptions.width,
bottomBorder).Render(sortOptionsContent)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *model) sortOptionsRender() string { | |
| panel := m.fileModel.filePanels[m.filePanelFocusIndex] | |
| sortOptionsContent := common.ModalTitleStyle.Render(" Sort Options") + "\n\n" | |
| for i, option := range panel.sortOptions.data.options { | |
| for i, option := range panel.SortOptions.data.options { | |
| cursor := " " | |
| if i == panel.sortOptions.cursor { | |
| if i == panel.SortOptions.cursor { | |
| cursor = common.FilePanelCursorStyle.Render(icon.Cursor) | |
| } | |
| sortOptionsContent += cursor + common.ModalStyle.Render(" "+option) + "\n" | |
| } | |
| bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.sortOptions.cursor+1), | |
| strconv.Itoa(len(panel.sortOptions.data.options))), panel.sortOptions.width-2) | |
| bottomBorder := common.GenerateFooterBorder(fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1), | |
| strconv.Itoa(len(panel.SortOptions.data.options))), panel.SortOptions.width-2) | |
| return common.SortOptionsModalBorderStyle(panel.sortOptions.height, panel.sortOptions.width, | |
| return common.SortOptionsModalBorderStyle(panel.SortOptions.height, panel.SortOptions.width, | |
| bottomBorder).Render(sortOptionsContent) | |
| } | |
| func (m *model) sortOptionsRender() string { | |
| panel := m.fileModel.filePanels[m.filePanelFocusIndex] | |
| sortOptionsContent := common.ModalTitleStyle.Render(" Sort Options") + "\n\n" | |
| opts := panel.SortOptions.data.options | |
| if len(opts) == 0 { | |
| return "" | |
| } | |
| if panel.SortOptions.cursor >= len(opts) { | |
| panel.SortOptions.cursor = 0 | |
| } | |
| for i, option := range opts { | |
| cursor := " " | |
| if i == panel.SortOptions.cursor { | |
| cursor = common.FilePanelCursorStyle.Render(icon.Cursor) | |
| } | |
| sortOptionsContent += cursor + common.ModalStyle.Render(" "+option) + "\n" | |
| } | |
| bottomBorder := common.GenerateFooterBorder( | |
| fmt.Sprintf("%s/%s", strconv.Itoa(panel.SortOptions.cursor+1), strconv.Itoa(len(opts))), | |
| panel.SortOptions.width-2) | |
| return common.SortOptionsModalBorderStyle(panel.SortOptions.height, panel.SortOptions.width, | |
| bottomBorder).Render(sortOptionsContent) | |
| } |
🤖 Prompt for AI Agents
In src/internal/model_render.go around lines 299 to 314, sortOptionsRender does
not guard against an empty options slice and can index with an out-of-range
cursor while repeatedly looking up panel.SortOptions; fix by assigning
panel.SortOptions and its data.options to local variables, check if options is
nil or length 0 and return an empty/placeholder modal render (or a modal with
"No options") early, clamp the cursor to the valid range (0..len(options)-1)
before using it, and compute bottomBorder using the safe length; this prevents
panics and reduces repeated field accesses.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests