fix: Crash fix#1287
Conversation
📝 WalkthroughWalkthroughThis PR refactors the file panel model by converting public fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Deploying superfile with
|
| Latest commit: |
c2d8907
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://62530168.superfile.pages.dev |
| Branch Preview URL: | https://crash-fix.superfile.pages.dev |
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 (1)
src/internal/ui/filepanel/get_elements.go (1)
96-109: Consider adding cursor validation after element updates.The mechanical change from
m.Elementtom.element(line 100) is correct. However,UpdateElementsIfNeededis where elements are refreshed, and this is a critical location for cursor validation.As demonstrated in the test file, when elements are updated (especially after external file changes), the cursor can become out of bounds. After line 100, consider adding cursor validation:
🔒 Suggested cursor validation
func (m *Model) UpdateElementsIfNeeded(focusPanelReRender bool, toggleDotFile bool, updatedToggleDotFile bool) { nowTime := time.Now() if !m.shouldSkipPanelUpdate(focusPanelReRender, nowTime, updatedToggleDotFile) { // Load elements for this panel (with/without search filter) m.element = m.getElements(toggleDotFile) + // Validate cursor is within bounds after element update + if m.cursor >= len(m.element) && len(m.element) > 0 { + m.cursor = len(m.element) - 1 + } + if len(m.element) == 0 { + m.cursor = 0 + } // Update file panel list m.LastTimeGetElement = nowTime // For hover to file on first time loading if m.TargetFile != "" { m.applyTargetFileCursor() } } }This validation would prevent the crash scenario demonstrated in
handle_panel_cursor_bug_test.go.
🤖 Fix all issues with AI agents
In @src/internal/handle_file_operations.go:
- Line 47: The call to panel.GetFocusedItem() is dead code because its return
value is discarded; remove this redundant invocation from handle_file_operations
(delete the standalone panel.GetFocusedItem() statement) so no unused call
remains and ensure no other logic depends on its side effects.
In @src/internal/handle_panel_cursor_bug_test.go:
- Around line 14-105: The test shows UpdateElementsIfNeeded() reloads elements
but doesn't clamp/validate the cursor, causing panics; call the existing
ValidateCursorAndRenderIndex() (from src/internal/ui/filepanel/navigation.go)
inside UpdateElementsIfNeeded() immediately after elements are replaced (or
otherwise clamp the cursor there) so the panel cursor is guaranteed <
ElemCount() before any user-facing accesses (validateLayout() must not be the
only place invoking it).
🧹 Nitpick comments (2)
src/internal/handle_panel_movement.go (1)
70-73: Consider adding a defensive Empty() check.While
executeOpenCommand()is currently only called fromenterPanel()after an Empty check, adding a defensive check at the function's entry would make it more robust as a standalone function and prevent potential future crashes if called from other contexts.🛡️ Suggested defensive check
func (m *model) executeOpenCommand() { panel := m.getFocusedFilePanel() + if panel.Empty() { + return + } filePath := panel.GetFocusedItem().LocationBased on learnings, empty panel checks help prevent crashes when accessing panel elements.
src/internal/ui/filepanel/utils.go (1)
114-118: SetCursorPosition marked for test-only use.The emphatic comment (5 exclamation marks) indicates this method should only be used in tests. While it's exported and callable from production code, the intent is clear. Consider whether unexported test helper functions in test files would be more appropriate, though the current approach is acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/internal/handle_file_operation_test.gosrc/internal/handle_file_operations.gosrc/internal/handle_modal.gosrc/internal/handle_panel_cursor_bug_test.gosrc/internal/handle_panel_movement.gosrc/internal/model_file_operations_test.gosrc/internal/model_navigation_test.gosrc/internal/model_test.gosrc/internal/test_utils.gosrc/internal/ui/filepanel/columns.gosrc/internal/ui/filepanel/dimension.gosrc/internal/ui/filepanel/get_elements.gosrc/internal/ui/filepanel/model.gosrc/internal/ui/filepanel/navigation.gosrc/internal/ui/filepanel/navigation_test.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/types.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.go
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
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: 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 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: 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 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: 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/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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
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: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
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: 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
Repo: yorukot/superfile PR: 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
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.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.gosrc/internal/ui/filepanel/columns.gosrc/internal/ui/filepanel/navigation_test.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/handle_file_operations.gosrc/internal/handle_modal.gosrc/internal/ui/filepanel/render.gosrc/internal/handle_panel_movement.gosrc/internal/ui/filepanel/get_elements.go
📚 Learning: 2025-09-20T01:40:50.076Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1081
File: src/internal/ui/sidebar/directory_utils.go:103-112
Timestamp: 2025-09-20T01:40:50.076Z
Learning: lazysegtree identified code duplication between removeNotExistingDirectories and TogglePinnedDirectory functions in src/internal/ui/sidebar/directory_utils.go, specifically 6 lines of JSON marshaling and file writing logic. He prefers to track such duplication fixes in separate GitHub issues and suggests either extracting common util functions or creating a PinnedDir manager for centralized Read/Write operations to PinnedFile.
Applied to files:
src/internal/handle_file_operations.gosrc/internal/ui/filepanel/update.gosrc/internal/test_utils.gosrc/internal/handle_panel_cursor_bug_test.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.gosrc/internal/model_file_operations_test.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
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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/handle_modal.gosrc/internal/model_file_operations_test.gosrc/internal/model_test.go
📚 Learning: 2026-01-10T19:54:40.162Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1277
File: src/internal/common/string_function.go:132-150
Timestamp: 2026-01-10T19:54:40.162Z
Learning: In GetHelpMenuHotkeyString (src/internal/common/string_function.go), the first key in the hotkeys slice is guaranteed to be non-empty due to upstream validation, so the current separator logic using index `i != 0` is correct and defensive checks for leading empty keys are not needed.
Applied to files:
src/internal/handle_modal.go
📚 Learning: 2025-03-29T10:02:04.260Z
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.
Applied to files:
src/internal/handle_modal.gosrc/internal/handle_panel_cursor_bug_test.go
📚 Learning: 2025-09-12T05:16:10.488Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/handle_modal.go:253-253
Timestamp: 2025-09-12T05:16:10.488Z
Learning: lazysegtree identified that the fuzzy search function in src/internal/handle_modal.go for the help menu only searches on item.description but should also include item.key in the search haystack to provide comprehensive search functionality, as users expect to find hotkeys by searching for their key names.
Applied to files:
src/internal/handle_modal.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/model_navigation_test.gosrc/internal/handle_file_operation_test.gosrc/internal/ui/filepanel/utils.gosrc/internal/test_utils.gosrc/internal/handle_panel_cursor_bug_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/model_navigation_test.gosrc/internal/handle_panel_cursor_bug_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_file_operations_test.gosrc/internal/model_test.gosrc/internal/ui/filepanel/navigation_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, 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/model_file_operations_test.go
📚 Learning: 2025-08-03T14:49:03.094Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 973
File: src/internal/ui/processbar/model_test.go:64-64
Timestamp: 2025-08-03T14:49:03.094Z
Learning: In Go, unexported fields (lowercase names) in structs are accessible within the same package, so test files in the same package as the struct definitions can access and test these unexported fields directly without needing them to be exported.
Applied to files:
src/internal/model_test.gosrc/internal/ui/filepanel/navigation_test.go
🧬 Code graph analysis (7)
src/internal/ui/filepanel/render.go (1)
src/internal/common/predefined_variable.go (1)
FilePanelNoneText(51-51)
src/internal/handle_file_operation_test.go (1)
src/internal/test_utils.go (2)
DefaultTestTimeout(21-21)DefaultTestTick(20-20)
src/internal/ui/filepanel/utils.go (1)
src/internal/ui/filepanel/types.go (2)
Model(16-39)Element(63-68)
src/internal/ui/filepanel/columns.go (2)
src/internal/common/string_function.go (2)
FilePanelItemRenderWithIcon(80-99)FilePanelItemRender(100-112)src/config/icon/icon.go (1)
Directory(41-41)
src/internal/test_utils.go (1)
src/internal/ui/filepanel/types.go (1)
Model(16-39)
src/internal/handle_panel_cursor_bug_test.go (2)
src/internal/test_utils_teaprog.go (1)
NewTestTeaProgWithEventLoop(33-39)src/internal/test_utils.go (2)
DefaultTestTimeout(21-21)DefaultTestTick(20-20)
src/internal/ui/filepanel/get_elements.go (1)
src/internal/common/ui_consts.go (1)
ReRenderChunkDivisor(24-24)
⏰ 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). (4)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (32)
src/internal/ui/filepanel/types.go (1)
17-18: Good encapsulation practice.Making
cursor,renderIndex, andelementunexported is a positive step toward controlled access and enables adding validation logic in accessor methods. This aligns with the broader refactoring to use public getters/setters.Note: While this encapsulation is good, the actual cursor validation logic to prevent out-of-bounds access (as demonstrated in the test file) must be implemented in the accessor methods or update functions.
Also applies to: 31-31
src/internal/ui/filepanel/columns.go (1)
15-36: LGTM: Consistent field access updates.All rendering methods have been correctly updated to use the unexported field names (
element,cursor) instead of the previous exported names. Since these methods are internal to thefilepanelpackage, direct field access is appropriate and doesn't bypass encapsulation.Also applies to: 40-48, 51-63, 66-77, 78-87
src/internal/ui/filepanel/dimension.go (1)
21-28: LGTM: Field access updated consistently.The change from
m.Cursortom.cursoris consistent with the encapsulation refactoring. ThescrollToCursormethod appropriately adjusts the view when the panel height changes.src/internal/ui/filepanel/get_elements.go (1)
88-88: LGTM: Field access updated consistently.The change from
m.Elementtom.elementis consistent with the encapsulation refactoring.src/internal/model_test.go (1)
130-134: LGTM! Clean migration to accessor methods.The test correctly uses the new accessor methods (
GetCursor()andGetRenderIndex()) instead of direct field access. This properly validates cursor and render index positions through the public API.src/internal/ui/filepanel/update.go (2)
37-38: LGTM! Consistent migration to private fields.The field accesses have been correctly updated to use the private field names (
cursor,renderIndex) throughout the directory record caching and restoration logic. The existing TODO comment on line 50 already tracks the validation concern for cached values.Also applies to: 56-60, 63-63
78-78: Correct field name update.The iteration correctly uses the private
elementfield instead of the former publicElementfield.src/internal/handle_modal.go (1)
71-76: Excellent crash prevention improvement.The change from direct
Element[Cursor]access to usingEmpty()andGetFocusedItem()is a good defensive pattern that prevents potential crashes when accessing panel elements. This aligns well with the PR's crash fix objective.Based on learnings, this defensive check pattern for empty panels is the preferred approach.
src/internal/ui/filepanel/model.go (1)
49-50: LGTM! Correct initialization of private fields.The struct initialization correctly uses the private field names (
cursor,renderIndex) with the same default values.src/internal/test_utils.go (1)
224-236: Good refactor to delegate lookups to panel methods.The helper functions now correctly use panel methods (
FindElementIndexByLocation,FindElementIndexByName,SetCursorPosition) instead of internal lookup logic. The guards foridx != -1properly prevent setting cursor when element isn't found. The lookup methods correctly return -1 when elements aren't found, andSetCursorPositiondelegates toscrollToCursorwhich validates bounds withif cursor < 0 || cursor >= m.ElemCount() { return }.src/internal/handle_panel_movement.go (2)
32-35: LGTM! Clean encapsulation with proper safety checks.The refactoring correctly replaces direct field access with
panel.Empty()andpanel.GetFocusedItem(), with the Empty check properly guarding against crashes when the panel has no elements.
60-60: LGTM! Consistent use of the new accessor API.The code correctly uses
panel.GetFocusedItem().Locationto access the focused item's location, aligning with the encapsulation pattern introduced in this PR.src/internal/ui/filepanel/render.go (3)
80-97: LGTM! Consistent refactoring of internal field access.The changes correctly update field names from public (Element, Cursor, RenderIndex) to private (element, cursor, renderIndex) while maintaining the same logic. The empty check at line 80 properly guards against crashes, and the rendering loop correctly uses the private fields within the package.
127-133: LGTM! Proper handling of empty panel state.The
getCursorString()method correctly checks for empty panel before incrementing the cursor for 1-based display, ensuring it shows "0/0" for empty panels instead of crashing.
153-158: LGTM! Consistent private field usage.The
NeedsReRender()method correctly uses the privateelementfield for internal state checking.src/internal/ui/filepanel/navigation.go (3)
7-32: LGTM! Clean refactoring to private fields.The navigation methods correctly update field access from public (Cursor, RenderIndex) to private (cursor, renderIndex). The logic for scrolling and cursor movement remains unchanged and correct.
68-76: LGTM! Consistent element access pattern.The
applyTargetFileCursor()method correctly iterates over the privateelementslice, maintaining the same behavior with the new field names.
78-88: LGTM! Validation logic preserved.The
ValidateCursorAndRenderIndex()method correctly validates the internal state using private fields, with error messages properly reflecting the field names.src/internal/model_navigation_test.go (3)
10-10: LGTM! Appropriate test assertion package.Adding
requireimport enables failing fast when test preconditions aren't met, which is good practice.
120-124: LGTM! Excellent test refactoring using public API.The test now properly uses the public API (
ListDown()andGetCursor()) instead of directly manipulating internal fields. This approach:
- Tests through the intended interface
- Is more realistic (simulating actual user navigation)
- Uses
require.Equalto fail fast if the cursor positioning doesn't work as expected
141-143: LGTM! Proper accessor usage in assertions.The test correctly validates state restoration using
GetCursor()andGetRenderIndex()accessors, ensuring the panel state is properly preserved after navigation operations.src/internal/model_file_operations_test.go (3)
40-41: LGTM! Proper accessor usage in test assertion.The test correctly uses
GetFocusedItem().Nameinstead of directElement[0].Nameaccess, properly testing through the public API.
157-160: LGTM! Cleaner error message.The error message is appropriately simplified to remove internal implementation details (Element reference) while still providing useful debugging information.
184-185: LGTM! Simplified assertion message.The assertion message is appropriately generalized, removing internal implementation details while maintaining clarity.
src/internal/ui/filepanel/navigation_test.go (1)
18-20: LGTM! Test code properly updated to use encapsulated fields.The test helper now initializes private fields (cursor, renderIndex, element) and all assertions correctly use the new accessor methods (GetCursor(), GetRenderIndex()) instead of direct field access.
Also applies to: 100-101
src/internal/handle_file_operations.go (2)
79-81: LGTM! Proper use of encapsulated Empty() checks.The refactoring correctly replaces direct Element field checks with the new Empty() method, maintaining safe empty panel handling throughout the file operations.
Also applies to: 110-112, 198-200
84-85: LGTM! Consistent use of GetFocusedItem() accessor.All item access properly uses GetFocusedItem() instead of direct Element[Cursor] indexing, respecting the new encapsulation.
Also applies to: 92-94, 202-203
src/internal/handle_file_operation_test.go (2)
108-112: LGTM! Respects encapsulation while testing navigation.Replacing direct cursor assignment with a ListDown() loop properly tests the public API and respects field encapsulation. This approach has the added benefit of exercising the navigation logic as part of test setup.
400-409: LGTM! Pragmatic fix for test flakiness.The retry-based cleanup helper is a practical solution for Windows file system timing issues. The honest "duct tape" comment appropriately characterizes this workaround.
Based on learnings, lazysegtree prefers keeping test-only code simple without over-engineering synchronization patterns.
src/internal/ui/filepanel/utils.go (3)
5-11: LGTM! Clean accessor methods for encapsulated fields.The new GetCursor(), GetRenderIndex(), ElemCount(), and Empty() methods provide proper read-only access to the private fields, completing the encapsulation refactor.
Also applies to: 98-108
13-31: LGTM! GetFocusedItem methods include proper bounds checking.Both GetFocusedItem() and GetFocusedItemPtr() correctly validate cursor bounds before accessing the element slice, providing defensive programming against invalid cursor states.
120-136: LGTM! Element search helpers added.FindElementIndexByName() and FindElementIndexByLocation() provide convenient O(n) search functionality appropriate for typical file panel element counts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/internal/handle_file_operations.go (3)
76-105: Nice hardening; reduce repeatedGetFocusedItem()calls and parenthesize the condition.This is much safer with
panel.Empty(). Minor: cache the focused item and add parentheses for readability/maintainability (avoids subtle precedence misreads).Proposed minimal change
func (m *model) panelItemRename() { panel := m.getFocusedFilePanel() if panel.Empty() { return } cursorPos := -1 - nameRunes := []rune(panel.GetFocusedItem().Name) + focused := panel.GetFocusedItem() + nameRunes := []rune(focused.Name) nameLen := len(nameRunes) for i := nameLen - 1; i >= 0; i-- { if nameRunes[i] == '.' { cursorPos = i break } } - if cursorPos == -1 || cursorPos == 0 && nameLen > 0 || panel.GetFocusedItem().Directory { + if cursorPos == -1 || (cursorPos == 0 && nameLen > 0) || focused.Directory { cursorPos = nameLen } @@ panel.Rename = common.GenerateRenameTextInput( m.fileModel.SinglePanelWidth-common.InnerPadding, cursorPos, - panel.GetFocusedItem().Name) + focused.Name) }-->
194-203: Good:copySingleItem()is now empty-safe; consider caching location once.Not required, but would avoid calling
GetFocusedItem()twice.-->
442-483: Good:openFileWithEditor()now guards before dereferencing the focused item.Optional: store
loc := panel.GetFocusedItem().Locationonce since it’s used for chooser + args.-->
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/internal/handle_file_operations.gosrc/internal/model_navigation_test.gosrc/internal/ui/filepanel/columns.gosrc/internal/ui/filepanel/dimension.gosrc/internal/ui/filepanel/get_elements.gosrc/internal/ui/filepanel/navigation.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/types.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.go
🚧 Files skipped from review as they are similar to previous changes (4)
- src/internal/ui/filepanel/columns.go
- src/internal/ui/filepanel/dimension.go
- src/internal/ui/filepanel/get_elements.go
- src/internal/ui/filepanel/utils.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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: 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 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: 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.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
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: 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
Repo: yorukot/superfile PR: 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
Repo: yorukot/superfile PR: 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
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: 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
Repo: yorukot/superfile PR: 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
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/model_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/model_navigation_test.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/navigation.gosrc/internal/ui/filepanel/render.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/ui/filepanel/update.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/handle_file_operations.gosrc/internal/ui/filepanel/render.go
📚 Learning: 2025-09-20T01:40:50.076Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1081
File: src/internal/ui/sidebar/directory_utils.go:103-112
Timestamp: 2025-09-20T01:40:50.076Z
Learning: lazysegtree identified code duplication between removeNotExistingDirectories and TogglePinnedDirectory functions in src/internal/ui/sidebar/directory_utils.go, specifically 6 lines of JSON marshaling and file writing logic. He prefers to track such duplication fixes in separate GitHub issues and suggests either extracting common util functions or creating a PinnedDir manager for centralized Read/Write operations to PinnedFile.
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: 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: 2026-01-10T19:54:48.794Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1277
File: src/internal/common/string_function.go:132-150
Timestamp: 2026-01-10T19:54:48.794Z
Learning: In GetHelpMenuHotkeyString (src/internal/common/string_function.go), the first key in the hotkeys slice is guaranteed to be non-empty due to upstream validation, so the current separator logic using index `i != 0` is correct and defensive checks for leading empty keys are not needed.
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 (3)
src/internal/model_navigation_test.go (3)
src/internal/utils/test_utils.go (2)
SetupDirectories(36-45)SetupFiles(55-57)src/internal/test_utils_teaprog.go (1)
NewTestTeaProgWithEventLoop(33-39)src/internal/test_utils.go (2)
DefaultTestTimeout(21-21)DefaultTestTick(20-20)
src/internal/ui/filepanel/navigation.go (1)
src/internal/ui/filepanel/types.go (1)
Model(16-43)
src/internal/ui/filepanel/render.go (1)
src/internal/common/predefined_variable.go (1)
FilePanelNoneText(51-51)
⏰ 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). (3)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Build and Test (windows-latest)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
src/internal/ui/filepanel/types.go (2)
17-22: LGTM: Field privatization strengthens invariant protection.Making cursor, renderIndex, and element private with accessor methods is a solid approach to prevent invalid state. The inline comment clearly documents the intent to minimize direct access and make bugs harder to introduce.
Based on learnings, this aligns well with the preference for maintaining correctness through proper design rather than defensive checks everywhere.
35-35: LGTM: Element field properly encapsulated.Privatizing the element slice is consistent with the broader refactor and will help prevent direct manipulation that could lead to inconsistent state.
src/internal/ui/filepanel/render.go (3)
80-87: LGTM: Accessor-based rendering logic is correct.The rendering logic correctly uses the new accessor methods (Empty(), ElemCount(), GetCursor()) and the private renderIndex field. The emptiness check at line 80 properly guards the loop that follows.
128-132: LGTM: Cursor display logic updated correctly.The cursor string generation properly uses GetCursor() and ElemCount() accessors, with appropriate emptiness checking.
154-156: LGTM: Safe guarded access to first element.The EmptyOrInvalid() check at line 154 ensures GetFirstElement() at line 155 is only called when the panel has valid elements, preventing potential crashes.
src/internal/model_navigation_test.go (2)
121-125: LGTM: Test updated to use accessor methods.The test correctly uses ListDown() for navigation and GetCursor()/GetRenderIndex() for assertions, consistent with the private field refactor.
149-202: Excellent test coverage for the crash fix!This test directly validates the core crash fix by:
- Setting cursor to position 8 in a directory with 10 files
- Navigating away (caching the cursor position)
- Externally deleting files to reduce the count to 4
- Navigating back and verifying the cursor is safely reset to 0
This test scenario precisely covers the edge case mentioned in PR #1261 where cached cursor positions can become invalid after external file system changes. The test confirms that ValidateCursorAndRenderIndex() catches this and scrollToCursor(0) resets the view safely.
src/internal/ui/filepanel/update.go (3)
37-60: LGTM: Cache storage and restoration updated correctly.The persistence logic properly uses the private cursor and renderIndex fields for both saving and restoring directory state.
65-68: Critical crash fix: Validates cached cursor after restoration!This is the core fix for issue #1261. After restoring cursor and renderIndex from the directory cache (lines 56-57), this validation check ensures they're still valid for the current directory state. If external changes (e.g., file deletions) have invalidated the cached position, scrollToCursor(0) safely resets the view.
This directly addresses the TODO comment at lines 50-53 about cached cursor values becoming invalid when files are deleted externally.
83-85: LGTM: Selection logic uses private element field.The iteration correctly uses the private m.element field, consistent with the encapsulation refactor.
src/internal/ui/filepanel/navigation.go (4)
7-23: LGTM: Scroll and cursor logic updated to private fields.The scrollToCursor method correctly manipulates the private cursor and renderIndex fields, maintaining proper bounds checking and view synchronization.
25-32: LGTM: Cursor movement uses private field.The moveCursorBy method properly uses m.cursor for calculations and delegates to scrollToCursor for the actual state update.
68-74: Good refactor: Using FindElementIndexByName for target positioning.Replacing the manual loop with FindElementIndexByName is cleaner and more maintainable. The method correctly handles the case when the target file is not found (idx == -1).
76-86: LGTM: Validation logic uses private fields correctly.The ValidateCursorAndRenderIndex method properly checks m.cursor and m.renderIndex bounds and their relationship, with clear error messages.
src/internal/handle_file_operations.go (6)
107-129: Good:getDeleteCmd()now safely bails on empty panel before dereferencing focus.
This aligns with the crash-fix objective and preventsGetFocusedItem().Locationfrom being called on empty panels.-->
168-173: Good: delete trigger now treats empty browser mode as no-op.-->
353-360: Good:getExtractFileCmd()now guards before reading the focused item location.-->
396-411: Good:getCompressSelectedFilesCmd()now guards before using focused item as fallback.-->
524-534: Good:copyPath()is now empty-safe and won’t crash on empty panel.-->
40-56: No changes needed—the code is already crash-safe.
GetFocusedItem()is guarded byGetElementAtIdx(), which explicitly checks bounds and returns an emptyElement{}struct if the index is invalid. After theElemCount() == 0check on line 43, the cursor is guaranteed to be valid, soGetFocusedItem()will safely return the actual item. No crash is possible.The suggestions about using
Empty()instead ofElemCount() == 0and caching the focused item are stylistic preferences, not crash-safety fixes.Likely an incorrect or invalid review comment.
Issues
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.