fix: small optimization file panel#1241
fix: small optimization file panel#1241
Conversation
📝 WalkthroughWalkthroughReplaces slice-based selection with a map-based selection model and select-order counter; adds map-backed selection APIs (SetSelected, SetSelectedAll, SetUnSelected, ToggleSelected, CheckSelected, GetSelectedLocations, GetFirstSelectedLocation, SelectedCount, ResetSelected); renames GetSelectedItem* → GetFocusedItem* and updates callers and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-09-12T05:16:10.488ZApplied to files:
📚 Learning: 2025-09-06T13:42:44.590ZApplied to files:
📚 Learning: 2025-09-04T07:24:30.872ZApplied to files:
📚 Learning: 2025-03-29T13:20:46.467ZApplied to files:
🧬 Code graph analysis (1)src/internal/ui/filepanel/update.go (1)
⏰ 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)
🔇 Additional comments (3)
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 |
|
@lazysegtree please help. what's mean "Error: you have exceeded GitHub's API rate limit. " in failed test? |
| selectedFiles := make(map[string]struct{}, len(m.Selected)) | ||
| for _, selectedItem := range m.Selected { | ||
| selectedFiles[selectedItem] = struct{}{} | ||
| } | ||
|
|
||
| for i := m.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 := slices.Contains(m.Selected, m.Element[i].Location) | ||
| _, isSelected := selectedFiles[m.Element[i].Location] |
There was a problem hiding this comment.
@xelavopelk we should initialize the set from slice at render time. That will be expensive too. We should fix it at once.
Render is called very frequently.
Can you complete eliminitate the Selected slice, and use the set everywhere.
There was a problem hiding this comment.
@xelavopelk we should initialize the set from slice at render time. That will be expensive too. We should fix it at once.
Render is called very frequently.
Can you complete eliminitate the
Selectedslice, and use the set everywhere.
ok
There was a problem hiding this comment.
@xelavopelk we should initialize the set from slice at render time. That will be expensive too. We should fix it at once.
Render is called very frequently.
Can you complete eliminitate the
Selectedslice, and use the set everywhere.
i think this need rename to "focused". May be missunderstanding with "Selected" functions
func (m *Model) GetSelectedItem() Element {
if m.Cursor < 0 || len(m.Element) <= m.Cursor {
return Element{}
}
return m.Element[m.Cursor]
}
@lazysegtree FYI
There was a problem hiding this comment.
@xelavopelk Yes. GetSelectedItem should be renamed to GetFocusedItem
There was a problem hiding this comment.
@xelavopelk we should initialize the set from slice at render time. That will be expensive too. We should fix it at once.
Render is called very frequently.
Can you complete eliminitate the
Selectedslice, and use the set everywhere.
- Slice replaced by Set
- GetSelectedItem and others have been renamed to "...FocusedItem..."
|
@xelavopelk For API limit issue |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/internal/ui/filepanel/utils.go (1)
25-51: Well-implemented selection accessor methods.The new methods provide clean encapsulation:
CheckSelected(): O(1) lookup vs O(n) slice search ✓GetSelectedLocations(): Simple extractionGetFirstSelectedLocation(): Correctly finds minimum orderOptional: Consider preserving selection order in
GetSelectedLocations().Currently,
GetSelectedLocations()returns items in arbitrary map iteration order. If operation ordering matters (e.g., for delete/copy operations), consider sorting by the selection order:Optional enhancement to preserve selection order
func (m *Model) GetSelectedLocations() []string { - result := make([]string, 0, len(m.selected)) - for k := range m.selected { - result = append(result, k) + if len(m.selected) == 0 { + return []string{} } + + // Create slice of location-order pairs + type pair struct { + location string + order int + } + pairs := make([]pair, 0, len(m.selected)) + for location, order := range m.selected { + pairs = append(pairs, pair{location, order}) + } + + // Sort by selection order + sort.Slice(pairs, func(i, j int) bool { + return pairs[i].order < pairs[j].order + }) + + // Extract locations + result := make([]string, len(pairs)) + for i, p := range pairs { + result[i] = p.location + } return result }This would require adding
"sort"to imports. However, if ordering isn't critical for current use cases, the existing implementation is simpler and sufficient.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/internal/handle_file_operation_test.gosrc/internal/handle_file_operations.gosrc/internal/handle_panel_movement.gosrc/internal/model.gosrc/internal/model_msg.gosrc/internal/model_test.gosrc/internal/test_utils.gosrc/internal/ui/filemodel/update.gosrc/internal/ui/filepanel/get_elements_test.gosrc/internal/ui/filepanel/navigation_test.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/selection_test.gosrc/internal/ui/filepanel/sort.gosrc/internal/ui/filepanel/types.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.go
💤 Files with no reviewable changes (1)
- src/internal/ui/filepanel/sort.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/internal/ui/filepanel/render.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/model_msg.go:58-62
Timestamp: 2025-09-16T07:17:07.854Z
Learning: lazysegtree correctly identified a race condition bug in DeleteOperationMsg.ApplyToModel() where asynchronous delete operations unconditionally reset the current file selection even if the user has selected different files after starting the delete operation, leading to unexpected selection clearing. This was tracked in GitHub issue #1079.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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).
📚 Learning: 2025-09-09T14:23:14.164Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/ui/rendering/truncate.go:6-6
Timestamp: 2025-09-09T14:23:14.164Z
Learning: The truncate test failure in src/internal/ui/rendering/truncate_test.go during the ANSI package migration from experimental to stable was caused by a truncation bug in the experimental package. The experimental package incorrectly truncated strings even when input length equaled maxWidth (e.g., "1234" with maxWidth=4 became "1..."), while the stable package correctly only truncates when input exceeds maxWidth. The test expectation was based on the buggy behavior and needs to be corrected.
Applied to files:
src/internal/model_test.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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_test.gosrc/internal/ui/filemodel/update.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Applied to files:
src/internal/ui/filemodel/update.go
📚 Learning: 2025-12-25T09:22:10.090Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1227
File: src/internal/ui/filemodel/update.go:66-102
Timestamp: 2025-12-25T09:22:10.090Z
Learning: In yorukot/superfile, the preview panel's UpdatePreviewPanel should NOT validate by comparing msg.GetReqID() against m.ioReqCnt to filter stale responses. During fast scrolling, ioReqCnt increments rapidly, causing legitimate async preview responses to have reqID < ioReqCnt by the time they complete, which would prevent previews from ever displaying. The correct approach is location-based filtering: only apply the preview if selectedItem.Location still matches msg.GetLocation() when the response arrives.
Applied to files:
src/internal/ui/filemodel/update.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/navigation_test.gosrc/internal/handle_panel_movement.go
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/get_elements_test.gosrc/internal/ui/filepanel/utils.gosrc/internal/handle_file_operations.gosrc/internal/handle_panel_movement.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
🧬 Code graph analysis (6)
src/internal/ui/filepanel/selection_test.go (1)
src/internal/ui/filepanel/types.go (2)
Model(14-36)Element(60-65)
src/internal/ui/filepanel/update.go (1)
src/internal/ui/filepanel/types.go (1)
Model(14-36)
src/internal/ui/filepanel/navigation_test.go (1)
src/internal/ui/filepanel/types.go (3)
Model(14-36)Element(60-65)PanelMode(68-68)
src/internal/ui/filepanel/get_elements_test.go (1)
src/internal/ui/filepanel/types.go (2)
Model(14-36)Element(60-65)
src/internal/ui/filepanel/utils.go (1)
src/internal/ui/filepanel/types.go (2)
Model(14-36)Element(60-65)
src/internal/handle_file_operations.go (1)
src/internal/ui/filepanel/types.go (3)
PanelMode(68-68)SelectMode(72-72)Element(60-65)
⏰ 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 (windows-latest)
🔇 Additional comments (19)
src/internal/model_test.go (1)
127-127: LGTM! Terminology updated correctly.The test now correctly uses
GetFocusedItem().Locationinstead ofGetSelectedItem().Location, aligning with the PR's shift from selection-based to focus-based item semantics.Also applies to: 130-130
src/internal/ui/filemodel/update.go (1)
62-62: LGTM! Preview logic correctly updated to focused item.The preview panel now correctly uses
GetFocusedItemPtr()andGetFocusedItem()to determine which item to preview, aligning with the broader refactor from selection-based to focus-based semantics.Also applies to: 93-93
src/internal/model_msg.go (1)
140-140: LGTM! Metadata retrieval updated to focused item.Metadata is now correctly fetched for the focused item using
GetFocusedItemPtr(), consistent with the refactor.src/internal/handle_panel_movement.go (1)
35-35: LGTM! Panel movement logic updated correctly.
- Line 35:
GetFocusedItem()correctly retrieves the focused item for enter/open operations.- Line 130:
SetSelected(item.Location, true)properly uses the new selection API to mark all items as selected.Both changes align with the map-based selection refactor and maintain correct functionality.
Also applies to: 130-130
src/internal/handle_file_operation_test.go (1)
115-115: LGTM! Test updated to use new selection APIs.
- Line 115: Correctly uses
SetSelectedAll(tt.selectedElem, true)to bulk-select items instead of direct field assignment.- Line 129: Correctly uses
GetFocusedItem().Locationto retrieve the focused item location.Both changes properly align with the map-based selection refactor.
Also applies to: 129-129
src/internal/test_utils.go (1)
57-59: LGTM! Test helper updated to use new selection API.The helper now correctly iterates over
selectedItemsand callspanel.SetSelected(item, true)for each item, replacing direct field assignment with the new setter method.src/internal/model.go (1)
150-150: LGTM! Metadata command updated to focused item.Metadata fetching now correctly uses
GetFocusedItem()to determine which item's metadata to load, aligning with the focus-based refactor.src/internal/ui/filepanel/types.go (1)
24-28: LGTM! Core optimization implemented correctly.The refactor from
Selected []stringtoselected map[string]intwithselectOrderCounterdirectly addresses the O(n²) complexity issue. The map provides O(1) selection lookups instead of O(n) slice searches, and the counter maintains selection order.Making these fields private enforces proper encapsulation through accessor methods (SetSelected, SetSelectedAll, GetSelectedLocations, CheckSelected), improving API consistency. All direct field access has been properly migrated to these accessor methods.
src/internal/ui/filepanel/navigation_test.go (1)
197-333: LGTM! Test updates correctly reflect the new selection model.The test changes properly validate the selection order tracking via the map-based representation. The use of
SetSelectedAll()for test setup and assertions againstpanel.selectedaligns well with the new internal API.src/internal/ui/filepanel/selection_test.go (1)
1-59: Excellent test coverage for the new selection lifecycle!This test comprehensively validates the selection state management including order tracking, multi-select operations, and reset behavior. The assertions on both
SelectedCount()and the internalselectedmap provide thorough validation.src/internal/handle_file_operations.go (2)
111-133: LGTM! Consistent use of new selection accessors.The transition from direct
panel.Selectedaccess topanel.GetSelectedLocations()and fromGetSelectedItem()toGetFocusedItem()improves encapsulation and reflects the semantic difference between cursor position and selection state.
172-194: Good refactoring to use the new accessor methods.The consistent adoption of
SelectedCount(),GetSelectedLocations(),GetFirstSelectedLocation(), andGetFocusedItem()across multiple file operations maintains code clarity while benefiting from the improved O(1) selection lookups.Also applies to: 196-219, 392-425
src/internal/ui/filepanel/update.go (2)
11-21: LGTM! Proper transition toResetSelected()for mode switching.The change from
m.Selected = m.Selected[:0]tom.ResetSelected()is cleaner and ensures both the map and counter are properly reset.
23-47: Well-implemented selection mutators.The
SetSelected()andSetSelectedAll()methods properly encapsulate selection state changes with order tracking. The nil map handling viaResetSelected()ensures initialization consistency.src/internal/ui/filepanel/get_elements_test.go (1)
207-278: Test correctly updated for the new selection API.The test properly exercises
SetSelectedAll()for initialization and validates the internal selection state. Edge cases (out-of-bounds, empty panel) are well covered.src/internal/ui/filepanel/utils.go (4)
5-10: Good semantic improvement with method renaming.Renaming
GetSelectedItem()toGetFocusedItem()(and the Ptr variant) better distinguishes between cursor position (focus) and selection state, improving code clarity.Also applies to: 18-23
12-15: LGTM! Proper initialization of map-based selection state.The updated
ResetSelected()correctly initializes both theselectedmap andselectOrderCounter, ensuring clean state for the new selection model.
53-60: Clean transition to the new selection API.
SingleItemSelect()now properly usesCheckSelected()andSetSelected(), eliminating the previous O(n) slice containment check.
66-71: Good defensive programming with the nil check.The nil guard in
SelectedCount()ensures safe operation even if the selection map hasn't been initialized yet.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/internal/ui/filepanel/utils.go (2)
25-28: Consider adding explicit nil check for consistency.While Go's map lookup safely handles
nilmaps (returningfalse),SelectedCount(line 68) includes an explicit nil check. Adding one here would improve consistency:func (m *Model) CheckSelected(location string) bool { if m.selected == nil { return false } _, isSelected := m.selected[location] return isSelected }
31-37: LGTM! Efficient implementation with safe nil handling.The function correctly returns all selected locations. Go's behavior of treating
nilmaps as empty makes this safe without an explicit nil check.Optional: Improve comment grammar
-// Returns not ordered list selected locations +// Returns an unordered list of selected locations func (m *Model) GetSelectedLocations() []string {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/internal/ui/filepanel/utils.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 825
File: src/internal/model_render_test.go:15-25
Timestamp: 2025-05-22T04:53:45.663Z
Learning: In the superfile project, comments in test files documenting additional test cases with "// Additional tests" are intentional TODOs for future work and should not be flagged as missing implementations in PR reviews.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
🧬 Code graph analysis (1)
src/internal/ui/filepanel/utils.go (1)
src/internal/ui/filepanel/types.go (2)
Model(14-36)Element(60-65)
⏰ 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 (windows-latest)
🔇 Additional comments (7)
src/internal/ui/filepanel/utils.go (7)
5-10: LGTM! Clear rename and correct bounds checking.The rename from
GetSelectedItemtoGetFocusedItembetter reflects that this returns the item at the cursor position, not a selected item. The bounds checking correctly handles edge cases.
12-15: LGTM! Clean reset of map-based selection state.Correctly resets both the selection order counter and the selection map.
18-23: LGTM! Consistent rename and safe nil return.Correctly renamed to match
GetFocusedItemand includes appropriate bounds checking with safe nil return.
39-52: LGTM! Correctly finds first selected item by order.The implementation correctly iterates through selected items to find the one with the minimum order value, handling the empty selection case by returning an empty string.
67-72: LGTM! Good defensive programming with explicit nil check.The function correctly returns the selection count with proper nil handling. The
uintreturn type appropriately represents a non-negative count.
1-80: Excellent performance optimization! O(n²) → O(1) selection checks.This refactoring successfully addresses the performance issue mentioned in the PR objectives by replacing slice-based selection with map-based lookups. Key improvements:
- Performance: Selection checks improved from O(n) to O(1), eliminating the O(n²) rendering complexity when checking 100 selected items among 200 files
- Clear naming:
GetSelectedItem→GetFocusedItembetter distinguishes cursor position from multi-selection state- Well-designed APIs: New methods (
CheckSelected,GetSelectedLocations,GetFirstSelectedLocation,SelectedCount) provide clean interfaces for selection operations- Order preservation: The
selectOrderCounterand order tracking in the map enable retrieving the first selected item efficientlyThe implementation is correct and safe. Minor suggestions for consistency are noted in individual comments above.
54-61: LGTM! Clean toggle logic using new selection APIs.The function correctly toggles selection state using the new map-based
CheckSelectedandSetSelectedmethods, which improves performance from O(n) to O(1). The bounds checking and proper initialization of the selection map are in place.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/internal/ui/filepanel/utils.go (2)
34-40: Consider adding nil check for consistency.While the pre-allocated slice is efficient, this method lacks a nil check for
m.selectedunlikeCheckSelectedandSelectedCount. If initialization order isn't guaranteed, this could panic.🔎 Suggested defensive nil check
func (m *Model) GetSelectedLocations() []string { + if m.selected == nil { + return []string{} + } result := make([]string, 0, len(m.selected)) for k := range m.selected { result = append(result, k) } return result }
42-55: LGTM: Correct implementation of first-selected lookup.The algorithm correctly finds the location with the minimum selection order. Note that
len(nil)returns 0 for nil maps in Go, so the empty check works implicitly. However, for consistency withCheckSelectedandSelectedCount, consider an explicit nil check.🔎 Optional: Explicit nil check for consistency
func (m *Model) GetFirstSelectedLocation() string { - if len(m.selected) == 0 { + if m.selected == nil || len(m.selected) == 0 { return "" } result := "" minOrder := math.MaxInt for location, order := range m.selected { if minOrder > order { result = location minOrder = order } } return result }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/internal/ui/filepanel/utils.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When lazysegtree says a review is "inaccurate and pre-existing issues. Not much useful," it means I should focus specifically on bugs or problems introduced by the current PR changes, not architectural concerns or code quality issues that were already present. The refactoring work in superfile PRs is generally well-implemented and should be evaluated on whether the specific changes work correctly, not on broader codebase architecture.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/utils.go
🧬 Code graph analysis (1)
src/internal/ui/filepanel/utils.go (1)
src/internal/ui/filepanel/types.go (2)
Model(14-36)Element(60-65)
⏰ 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 (windows-latest)
🔇 Additional comments (7)
src/internal/ui/filepanel/utils.go (7)
3-3: LGTM: Import added formath.MaxIntusage.The
mathpackage import is correctly added to supportGetFirstSelectedLocation.
5-10: LGTM: Semantic improvement with GetFocusedItem rename.The rename from
GetSelectedItemtoGetFocusedItemmore accurately reflects that this returns the element at the cursor position (focused), not selected items. Bounds checking is correctly preserved.
12-15: LGTM: Correct reset of selection state.Both the selection order counter and the map are properly reset, ensuring clean selection state initialization.
18-23: LGTM: Consistent rename to GetFocusedItemPtr.The rename maintains consistency with
GetFocusedItemand correctly distinguishes focused vs selected elements. Implementation and nil safety are preserved.
25-31: LGTM: Efficient selection check with O(1) lookup.The map-based approach is a significant performance improvement over slice-based selection checking (O(1) vs O(n)). The nil check provides good defensive programming.
70-75: LGTM: Correct selection count with proper nil safety.The nil check and uint return type are appropriate. The implementation efficiently returns the selected item count.
58-64: LGTM: Clean selection toggle logic.The implementation correctly toggles selection state using the new map-based API. The bounds check ensures safety, and
SetSelectedproperly maintains the monotonic ordering ofselectOrderCounterby incrementing only when adding selections, whichGetFirstSelectedLocationdepends on to identify the first-selected item.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/internal/ui/filepanel/utils.go (1)
39-52: GetFirstSelectedLocation is nil-safe; consider performance characteristics.The implementation is nil-safe:
len(m.selected)returns 0 for nil maps, and iterating a nil map performs zero iterations.The O(n) scan over all selected items is acceptable for typical selection counts. If selection sets grow large (hundreds of items), consider maintaining a min-heap or sorted list of selection order for O(1) first-element access.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/internal/ui/filepanel/model.gosrc/internal/ui/filepanel/types.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/model_msg.go:58-62
Timestamp: 2025-09-16T07:17:07.854Z
Learning: lazysegtree correctly identified a race condition bug in DeleteOperationMsg.ApplyToModel() where asynchronous delete operations unconditionally reset the current file selection even if the user has selected different files after starting the delete operation, leading to unexpected selection clearing. This was tracked in GitHub issue #1079.
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/utils.go
🧬 Code graph analysis (1)
src/internal/ui/filepanel/utils.go (1)
src/internal/ui/filepanel/types.go (2)
Model(15-37)Element(61-66)
🪛 GitHub Actions: Go CI
src/internal/ui/filepanel/update.go
[error] 26-26: Nil map assignment panic in SetSelected: assignment to entry in nil map
src/internal/ui/filepanel/utils.go
[error] 59-59: SingleItemSelect caused runtime panic due to nil map; see stack trace
🔇 Additional comments (11)
src/internal/ui/filepanel/types.go (2)
13-14: Warning comment helps but doesn't prevent zero-value misuse.The comment correctly warns developers, but it doesn't enforce the constraint. The pipeline failures confirm that some code paths still instantiate
Model{}directly. Consider whether this struct should be opaque (e.g., exported via interface) or if defensive nil-checks should be added to all map-accessing methods.
27-29: LGTM: Map-based selection improves lookup performance.The refactor from slice to map provides O(1) selection checks versus O(n) slice scanning, addressing the O(n²) rendering concern mentioned in the PR objectives. The
selectOrderCountertracks insertion order without needing to maintain a sorted structure.Note: The counter never decrements (only increments on select), so it could theoretically overflow after billions of select operations, though this is unlikely in practice.
src/internal/ui/filepanel/update.go (1)
14-14: LGTM: Proper delegation to ResetSelected().The change correctly delegates to
ResetSelected()which reinitializes the map, providing defensive safety if the map was somehow nil.src/internal/ui/filepanel/utils.go (7)
3-3: LGTM: Import change reflects new dependencies.The
mathimport is needed formath.MaxIntused inGetFirstSelectedLocation(). Theslicespackage is no longer needed after migrating from slice-based to map-based selection.
5-10: LGTM: Clearer naming distinguishes focused item from selected items.The renames from
GetSelectedItem/GetSelectedItemPtrtoGetFocusedItem/GetFocusedItemPtrclarify that these methods return the cursor-focused item, not items in the selection set. This improves API clarity given the new map-based selection tracking.Also applies to: 18-23
12-15: LGTM: ResetSelected provides defensive map reinitialization.Reinitializing the map with
make()ensures the map is never nil after calling this method, providing defensive safety. Combined with resetting the counter, this properly clears all selection state.
25-28: LGTM: CheckSelected is nil-safe.Reading from a nil map in Go returns the zero value and false for the existence check, so this method is safe even if
m.selectedis nil (returns false).
30-37: LGTM: GetSelectedLocations is nil-safe.Iterating over a nil map in Go is safe (performs zero iterations), and
len(nil map)returns 0, so the pre-allocation is correct.
55-61: Pipeline failure traced to SetSelected nil map issue.The pipeline reports "SingleItemSelect caused runtime panic due to nil map" at line 59. The panic originates from
SetSelected()(called on line 59), which lacks nil-safety checks. The fix should be applied toSetSelected()inupdate.goas noted in that file's review.
67-69: LGTM: SelectedCount is nil-safe.In Go,
len(nil map)returns 0, so this method is safe even ifm.selectedis nil.src/internal/ui/filepanel/model.go (1)
60-60: Fix test Model instantiations to use New() constructor.Test files create
Model{}directly, leaving theselectedfield uninitialized (nil). This causes panics when test methods likeSetSelected()andSelectedCount()try to access or modify the map. The test files inselection_test.go,navigation_test.go, andget_elements_test.gomust be updated to use theNew()constructor so all fields are properly initialized.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/ui/filepanel/get_elements_test.go (1)
1-1: Critical: Fix Go formatting issues before merge.The pipeline detected formatting changes that need to be addressed.
Run the following command to fix the formatting:
go fmt ./...Then commit the formatted changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/internal/ui/filepanel/get_elements_test.gosrc/internal/ui/filepanel/navigation_test.gosrc/internal/ui/filepanel/selection_test.gosrc/internal/ui/filepanel/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- src/internal/ui/filepanel/selection_test.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
📚 Learning: 2025-03-29T13:20:46.467Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/get_elements_test.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/get_elements_test.gosrc/internal/ui/filepanel/navigation_test.go
📚 Learning: 2025-09-09T14:23:14.164Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/ui/rendering/truncate.go:6-6
Timestamp: 2025-09-09T14:23:14.164Z
Learning: The truncate test failure in src/internal/ui/rendering/truncate_test.go during the ANSI package migration from experimental to stable was caused by a truncation bug in the experimental package. The experimental package incorrectly truncated strings even when input length equaled maxWidth (e.g., "1234" with maxWidth=4 became "1..."), while the stable package correctly only truncates when input exceeds maxWidth. The test expectation was based on the buggy behavior and needs to be corrected.
Applied to files:
src/internal/ui/filepanel/navigation_test.go
📚 Learning: 2025-04-12T13:51:24.691Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/ui/filepanel/navigation_test.go
🧬 Code graph analysis (2)
src/internal/ui/filepanel/get_elements_test.go (1)
src/internal/ui/filepanel/types.go (3)
BrowserMode(75-75)SelectMode(74-74)Element(62-67)
src/internal/ui/filepanel/navigation_test.go (1)
src/internal/ui/filepanel/types.go (4)
Model(16-38)Element(62-67)PanelMode(70-70)SelectMode(74-74)
🪛 GitHub Actions: Go CI
src/internal/ui/filepanel/get_elements_test.go
[error] 1-1: Go fmt detected formatting changes. Run 'go fmt ./...' (and commit formatted changes) to fix.
🔇 Additional comments (6)
src/internal/ui/filepanel/types.go (2)
28-30: Excellent optimization from slice to map for selection tracking.Replacing
Selected []stringwithselected map[string]inteliminates O(n) linear scans when checking selection state, directly addressing the O(n²) complexity mentioned in the PR objectives. TheselectOrderCounterpreserves selection order, which is important for user experience.
13-15: The warning comment is appropriate and correctly implemented.The
New()constructor at line 47 ofmodel.goproperly initializes theselectedmap withmake(map[string]int), as does the test helpertestModel(). All Model instantiations in the codebase follow this pattern, preventing panics from uninitialized map access. The warning comment is necessary and the code implements the recommended practice correctly.src/internal/ui/filepanel/navigation_test.go (2)
11-25: Well-designed test helpers with proper map initialization.The
testModelandtestModelWithElemCounthelpers correctly initialize theselectedmap (line 22), which is essential to avoid panics. This pattern should be followed consistently across all Model instantiations in production code as well.
258-267: Test pattern correctly exercises the new selection API.The tests properly use
SetSelectedAllto initialize selection state and verify the internalselectedmap. This white-box testing approach is appropriate for unit tests and validates the map-based selection implementation.src/internal/ui/filepanel/get_elements_test.go (2)
183-187: Thenilelement parameter does not cause issues in this test.The
getDirectoryElements()andgetDirectoryElementsBySearch()functions read fromos.ReadDir(m.Location)and return independently generated slices—they do not access or dereference them.Elementfield. Thenilvalue assigned bytestModelis never used by these functions, so there is no nil pointer dereference risk. Additionally, Go's nil slices are safe when accessed withlen()checks, which are already in place throughout the codebase.Likely an incorrect or invalid review comment.
257-259: No issues found withSetSelectedAllimplementation.The method is robust and properly handles the scenarios mentioned. The
selectedmap is always initialized (in the Model constructor at model.go:60 and in all test setups), empty location slices are correctly handled by the range loop, and the method doesn't need to validate locations since it simply tracks selection state in the map. No changes needed.
lazysegtree
left a comment
There was a problem hiding this comment.
@xelavopelk Looks good now. I have added some improvements. Let me know if you are good to merge this now.
ok.let's merge this. It's my last commit for this year. |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [yorukot/superfile](https://github.com/yorukot/superfile) | minor | `v1.4.0` → `v1.5.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>yorukot/superfile (yorukot/superfile)</summary> ### [`v1.5.0`](https://github.com/yorukot/superfile/releases/tag/v1.5.0) [Compare Source](yorukot/superfile@v1.4.0...v1.5.0) This release brings major new features, including video and PDF preview support, multi-column file panels, and configurable navigation, alongside significant code refactoring and comprehensive bug fixes. #### Install: [**Click me to know how to install**](https://github.com/yorukot/superfile?tab=readme-ov-file#installation) #### Highlights - Added video and PDF preview support via loading the first frame/page as an image. Thanks [@​Yassen-Higazi](https://github.com/Yassen-Higazi) for the implementation <details><summary>Screenshots</summary> <p> <img width="951" height="596" alt="image" src="https://github.com/user-attachments/assets/6edfa9c2-ebcd-4622-a115-f71fa533b3e1" /> <img width="949" height="594" alt="image" src="https://github.com/user-attachments/assets/8d15fa46-5178-422d-8eea-455cac31fdd0" /> </p> </details> - Multi-column file panel view with date/size/permission columns. Thanks [@​xelavopelk](https://github.com/xelavopelk) for the implementation <details><summary>Screenshots</summary> <p> <img width="420" height="264" alt="image" src="https://github.com/user-attachments/assets/e172f1e8-c2a5-42d2-8eeb-62e721f61a4f" /> </p> </details> - Configurable fast navigation in the Filepanel. See this MR for more details: [#​1220](yorukot/superfile#1220) - You can now configure `spf` to open files with specific extensions with your choice of editor application. Thanks [@​litvinov-git](https://github.com/litvinov-git) for the implementation See this MR for more details: [#​1197](yorukot/superfile#1197) - Terminal stdout support for shell commands - Allow launching with the filename. `spf /a/b/c.txt` will launch in `/a/b` with `c.txt` as the selected file. - Various bug fixes, including modal confirmations, layout issues, and race conditions. See 'Detailed Change Summary' ##### Internal Updates - Separated FilePanel and FileModel into a dedicated package for better code organization - Comprehensive end-to-end testing with layout fixes - Enhanced dimension validation and sidebar fixes - Updated multiple dependencies including Astro, Go toolchain, and linters - Added gosec linter and MND linter with magic number cleanup #### Detailed Change Summary <details><summary>Details</summary> <p> ##### Update - allow hover to file [`#1177`](yorukot/superfile#1177) by @​lazysegtree - show count selected items in select mode [`#1187`](yorukot/superfile#1187) by @​xelavopelk - Add icon alias for kts to kt [`#1153`](yorukot/superfile#1153) by @​nicolaic - link icon and metadata [`#1171`](yorukot/superfile#1171) by @​xelavopelk - user configuration of editors by file extension [`#1197`](yorukot/superfile#1197) by @​litvinov-git - add video preview support [`#1178`](yorukot/superfile#1178) by @​Yassen-Higazi - Add pdf preview support [`#1198`](yorukot/superfile#1198) by @​Yassen-Higazi - Add icons in pinned directories [`#1215`](yorukot/superfile#1215) by @​lazysegtree - Enable fast configurable navigation [`#1220`](yorukot/superfile#1220) by @​lazysegtree - add Trash bin to default directories for Linux [`#1236`](yorukot/superfile#1236) by @​lazysegtree - add terminal stdout support for shell commands [`#1250`](yorukot/superfile#1250) by @​majiayu000 - More columns in file panel (MVP) [`#1268`](yorukot/superfile#1268) by @​xelavopelk ##### Bug Fix - only calculate checksum on files [`#1119`](yorukot/superfile#1119) by @​nikero41 - Linter issue with PrintfAndExit [`#1133`](yorukot/superfile#1133) by @​xelavopelk - Remove repeated os.ReadDir calls [`#1155`](yorukot/superfile#1155) by @​lazysegtree - Disable COPYFILE in macOS [`#1194`](yorukot/superfile#1194) by @​lazysegtree - add missing hotkeys to help menu [`#1192`](yorukot/superfile#1192) by @​lazysegtree - Fetch latest version automatically [`#1127`](yorukot/superfile#1127) by @​lazysegtree - Use async methods to prevent test race conditions [`#1201`](yorukot/superfile#1201) by @​lazysegtree - update metadata and process bar sizes when toggling footer [`#1218`](yorukot/superfile#1218) by @​lazysegtree - File panel dimension management [`#1222`](yorukot/superfile#1222) by @​lazysegtree - Layout fixes with full end-to-end tests [`#1227`](yorukot/superfile#1227) by @​lazysegtree - Fix flaky tests [`#1233`](yorukot/superfile#1233) by @​lazysegtree - modal confirmation bug with arrow keys [`#1243`](yorukot/superfile#1243) by @​lazysegtree - small file panel optimization [`#1241`](yorukot/superfile#1241) by @​xelavopelk - use ExtractOperationMsg for extraction [`#1248`](yorukot/superfile#1248) by @​lazysegtree - skip open_with from missing field validation [`#1251`](yorukot/superfile#1251) by @​lazysegtree - border height validation fixes [`#1267`](yorukot/superfile#1267) by @​lazysegtree - fix case with two active panes [`#1271`](yorukot/superfile#1271) by @​xelavopelk - help model formatting [`#1277`](yorukot/superfile#1277) by @​booth-w ##### Optimization - simplify renameIfDuplicate logic [`#1100`](yorukot/superfile#1100) by @​sarff - separate FilePanel into dedicated package [`#1195`](yorukot/superfile#1195) by @​lazysegtree - File model separation [`#1223`](yorukot/superfile#1223) by @​lazysegtree - Dimension validations [`#1224`](yorukot/superfile#1224) by @​lazysegtree - layout validation and sidebar dimension fixes [`#1228`](yorukot/superfile#1228) by @​lazysegtree - user rendering package and removal of unused preview code [`#1245`](yorukot/superfile#1245) by @​lazysegtree - user rendering package for file preview [`#1249`](yorukot/superfile#1249) by @​lazysegtree ##### Documentation - update Fish shell setup docs [`#1142`](yorukot/superfile#1142) by @​wleoncio - fix macOS typo [`#1212`](yorukot/superfile#1212) by @​wcbing - stylistic and linguistic cleanup of config documentation [`#1184`](yorukot/superfile#1184) by @​ninetailedtori ##### Dependencies - update astro monorepo [`#1010`](yorukot/superfile#1010) by @​renovate[bot] - update starlight-giscus [`#1020`](yorukot/superfile#1020) by @​renovate[bot] - bump astro versions [`#1138`](yorukot/superfile#1138), [`#1157`](yorukot/superfile#1157), [`#1158`](yorukot/superfile#1158) by @​dependabot[bot], @​renovate[bot] - bump vite [`#1134`](yorukot/superfile#1134) by @​dependabot[bot] - update setup-go action [`#1038`](yorukot/superfile#1038) by @​renovate[bot] - update expressive-code plugins [`#1189`](yorukot/superfile#1189), [`#1246`](yorukot/superfile#1246) by @​renovate[bot] - update sharp [`#1256`](yorukot/superfile#1256) by @​renovate[bot] - update fontsource monorepo [`#1257`](yorukot/superfile#1257) by @​renovate[bot] - update urfave/cli [`#1136`](yorukot/superfile#1136), [`#1190`](yorukot/superfile#1190) by @​renovate[bot] - update astro / starlight / ansi / toolchain deps [`#1275`](yorukot/superfile#1275), [`#1278`](yorukot/superfile#1278), [`#1280`](yorukot/superfile#1280) by @​renovate[bot] - update python and go versions [`#1276`](yorukot/superfile#1276), [`#1191`](yorukot/superfile#1191) by @​renovate[bot] - update golangci-lint action [`#1286`](yorukot/superfile#1286) by @​renovate[bot] ##### Misc - update CI input names [`#1120`](yorukot/superfile#1120) by @​nikero41 - Everforest Dark Hard theme [`#1114`](yorukot/superfile#1114) by @​fzahner - migrate tutorial demo assets to local [`#1140`](yorukot/superfile#1140) by @​yorukot - new logo asset [`#1145`](yorukot/superfile#1145) by @​nonepork - mirror repository to codeberg [`#1141`](yorukot/superfile#1141) by @​yorukot - sync package lock [`#1143`](yorukot/superfile#1143) by @​yorukot - bump golangci-lint version [`#1135`](yorukot/superfile#1135) by @​lazysegtree - add gosec linter [`#1185`](yorukot/superfile#1185) by @​lazysegtree - enable MND linter and clean magic numbers [`#1180`](yorukot/superfile#1180) by @​lazysegtree - skip permission tests when running as root [`#1186`](yorukot/superfile#1186) by @​lazysegtree - release v1.4.1-rc [`#1203`](yorukot/superfile#1203) by @​lazysegtree - 1.5.0-rc1 housekeeping changes [`#1264`](yorukot/superfile#1264) by @​lazysegtree </p> </details> #### New Contributors * @​fzahner made their first contribution in yorukot/superfile#1114 * @​sarff made their first contribution in yorukot/superfile#1100 * @​nicolaic made their first contribution in yorukot/superfile#1153 * @​Yassen-Higazi made their first contribution in yorukot/superfile#1178 * @​ninetailedtori made their first contribution in yorukot/superfile#1184 * @​litvinov-git made their first contribution in yorukot/superfile#1197 * @​wcbing made their first contribution in yorukot/superfile#1212 * @​majiayu000 made their first contribution in yorukot/superfile#1250 **Full Changelog**: <yorukot/superfile@v1.4.0...v1.5.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0Mi44MC4xIiwidXBkYXRlZEluVmVyIjoiNDIuODAuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
fix: File panel rendering optimization
removed TODO:
PS preparing for multicolumn
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.