Sidebar code separation #770
Conversation
…s that were needed by sidebar
…to sidebar package
Sidebar separation
WalkthroughThe changes update several import paths, shifting references from the old Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Model
participant S as Sidebar
participant D as Directory Utils
U->>M: Sends key input (rename/search/navigation)
M->>S: Calls UpdateState with the input
S->>D: Retrieves/updates directory information
D-->>S: Returns updated directory data
S-->>M: Provides new sidebar state
M->>U: Renders updated sidebar UI
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (23)
src/internal/ui/sidebar/ReadMe.md (3)
11-12: Remove duplicate heading.There's a duplicate "# Coverage" heading. Remove one of these instances to improve the documentation quality.
- Some methods could be made more pure by reducing side effects # Coverage -# Coverage ```bash🧰 Tools
🪛 LanguageTool
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ...e more pure by reducing side effects # Coverage # Coverage ```bash cd /path/to/ui/prompt go test ...(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
12-12: Multiple headings with the same content
null(MD024, no-duplicate-heading)
15-15: Update path in example command.The example path refers to the
promptdirectory, but since this is in thesidebarpackage, it should reference the correct location.-cd /path/to/ui/prompt +cd /path/to/ui/sidebar
18-19: Fix incomplete sentence at the end of the document.The document ends with an incomplete phrase "Current coverage" after already stating the coverage percentage.
Current coverage is 74.0%. -Current coveragesrc/internal/ui/sidebar/consts.go (2)
6-9: LGTM, but consider using a struct type for better type checking.The variable is correctly marked as a global with
//nolint: gochecknoglobalscomment explaining why it's acceptable. However, consider defining a const struct type instead of using raw string literals for theLocationfield to make it more type-safe and discoverable.
16-17: Standardize naming convention for consistency.The constant
SideBarInitialHeightuses mixed casing (camelCase with a capital 'B'). To maintain consistency with Go naming conventions, consider renaming it to eitherSidebarInitialHeight(PascalCase) orsidebarInitialHeight(camelCase) depending on whether it needs to be exported.-const SideBarInitialHeight = 3 +const SidebarInitialHeight = 3src/internal/ui/sidebar/navigation_test.go (1)
12-14: Fixed potential code duplication in test data preparation.Multiple test cases use similar patterns to prepare test data using
FormDirctorySlice. Consider creating test helper functions to reduce repetition in test data setup.+// Helper functions for test data preparation +func createTestSidebar(homeDirs, pinnedDirs, diskDirs []directory) Model { + return Model{ + directories: FormDirctorySlice(homeDirs, pinnedDirs, diskDirs), + } +}Also applies to: 303-304, 446-447, 523-524
src/internal/common/icon_utils.go (2)
14-19: Validate fallback icon usage for non-nerdfont scenario
Returning an empty icon might be confusing to users or cause UI issues. Consider using a minimal fallback icon or a textual placeholder to maintain visual consistency.
21-28: Check for folder key presence
Potential panic if there is no"folder"key in theicon.Foldersmap. Consider adding a safety check or a fallback condition to avoid runtime errors.src/internal/ui/sidebar/render.go (1)
72-73: Maintain consistency in truncation
Usingcommon.TruncateTextensures text fits within the sidebar. However, consider if you want to also handle full tooltip or hover expansions for longer directory names.src/internal/model.go (4)
86-86: Efficiency concern inUpdateDirectories.
Invokingm.sidebarModel.UpdateDirectories()every update might be performant enough for smaller projects, but if directory fetches become large, consider caching or debouncing directory refreshes.
225-226: Minor grammar improvement.
“// If renaming a object” should read “// If renaming an object.”- // If renaming a object + // If renaming an object
234-235: Comment mismatch with code block.
The comment says “If sort options menu is open,” but the code checksm.sidebarModel.SearchBarFocused(). Consider updating the comment to match the condition for clarity.- // If sort options menu is open + // If sidebar search bar is focused
248-248: Typographical fix.
Use “quitting” rather than “quiting.”- // If quiting input pressed, check if has any running process... + // If quitting input pressed, check if there are any running processes...src/internal/ui/sidebar/sidebar.go (2)
36-65: Finalize rename & persist pinned directories.
ConfirmSidebarRename()updates both the local in-memory state and the pinned file. The repeated directory read from disk ensures correct data but might be optimized if the local model is always authoritative. Consider a possible in-memory approach to reduce disk I/O.
94-109: Directory fetching approach.
UpdateDirectoriescallsGetFilteredDirectoriesorGetDirectoriesrepeatedly, which may be heavy if called in rapid succession. On large directory sets, consider caching.src/internal/ui/sidebar/utils_test.go (3)
22-25: Todo comments contradiction - already using t.Run.Your todo comment mentions using
t.Runfor subtests, but all your tests are already properly usingt.Runwith named test cases (see lines 63-66, 108-111, 151-154).Either remove this TODO item or clarify what specific improvements you want to make to the existing t.Run implementation.
90-91: Fix typo in test case name.There's a typo in the test case name.
- "Curson points to pinned divider", + "Cursor points to pinned divider",
114-156: Ensure variable names are consistent in tests.The
Test_resetCursorfunction usesdatafor the test cases array while other tests usetestcases. Consider standardizing the variable naming across all tests for better consistency.- data := []struct { + testcases := []struct {And update the loop accordingly:
- for _, tt := range data { + for _, tt := range testcases {src/internal/ui/sidebar/utils.go (1)
13-24: Consider optimizing NoActualDir method.The comment suggests a potential optimization by replacing the loop with a simpler condition check.
If performance is a concern, you could implement the optimization mentioned in the comment:
func (s *Model) NoActualDir() bool { - for _, d := range s.directories { - if !d.IsDivider() { - return false - } - } - return true + // Assuming the implementation always includes at most 2 dividers + // when no actual directories are present + return len(s.directories) <= 2 }However, the current implementation is more explicit and less likely to break if the underlying assumptions change.
src/internal/ui/sidebar/navigation.go (1)
93-119: Consider explicit error handling for unreachable branch.Currently, an error is logged if the code flows to the last
slog.Errorcall. Consider returning an error or adding a panic to highlight truly unrecoverable states.func (s *Model) updateRenderIndex(mainPanelHeight int) { ... // Code should never reach here - slog.Error("Unexpected situation in updateRenderIndex", "cursor", s.cursor, - "renderIndex", s.renderIndex, "directory count", len(s.directories)) + panic(fmt.Sprintf("Unexpected state in updateRenderIndex (cursor=%d, renderIndex=%d, dirCount=%d)", + s.cursor, s.renderIndex, len(s.directories))) }src/internal/common/string_function_test.go (1)
112-114: Typographical fix for “Extension” misspelling.The function name “IsExensionExtractable” contains a spelling error. Consider renaming it to “IsExtensionExtractable” for clarity and consistency.
-func IsExensionExtractable(ext string) bool { +func IsExtensionExtractable(ext string) bool { ... } - t.Errorf("IsExensionExtractable (%q) = %v; want %v", tt.ext, result, tt.expected) + t.Errorf("IsExtensionExtractable (%q) = %v; want %v", tt.ext, result, tt.expected)src/internal/common/string_function.go (2)
83-95: Ensure consistency in render method usage.The code is now using exported function names and direct references to styles and colors. However, note that you're setting the background color twice - once in
StringColorRender()and again with.Background(). While this works, it's redundant.- return StringColorRender(lipgloss.Color(style.Color), FooterBGColor). - Background(FooterBGColor). + return StringColorRender(lipgloss.Color(style.Color), FooterBGColor).This same pattern appears in other rendering functions in this file and could be simplified.
194-206: Consider optimizing string building operation.This function processes strings byte-by-byte for handling special characters. While this approach is correct, for longer strings, pre-allocating the string builder capacity could improve performance.
func MakePrintable(line string) string { var sb strings.Builder + sb.Grow(len(line)) // Pre-allocate capacity // This has to be looped byte-wise, looping it rune-wise // or by using strings.Map would cause issues with strings like // "(NBSP)\xa0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
src/cmd/main.go(1 hunks)src/config/fixed_variable.go(1 hunks)src/internal/common/icon_utils.go(1 hunks)src/internal/common/load_config.go(1 hunks)src/internal/common/string_function.go(12 hunks)src/internal/common/string_function_test.go(6 hunks)src/internal/config_function.go(1 hunks)src/internal/default_config.go(2 hunks)src/internal/file_operations.go(1 hunks)src/internal/function.go(4 hunks)src/internal/get_data.go(0 hunks)src/internal/handle_file_operations.go(2 hunks)src/internal/handle_panel_movement.go(4 hunks)src/internal/handle_panel_navigation.go(2 hunks)src/internal/handle_pinned_operations.go(0 hunks)src/internal/internal_consts.go(0 hunks)src/internal/key_function.go(4 hunks)src/internal/model.go(4 hunks)src/internal/model_prompt_test.go(1 hunks)src/internal/model_render.go(12 hunks)src/internal/type.go(2 hunks)src/internal/type_utils.go(1 hunks)src/internal/ui/prompt/model_test.go(1 hunks)src/internal/ui/prompt/tokenize.go(1 hunks)src/internal/ui/sidebar/ReadMe.md(1 hunks)src/internal/ui/sidebar/consts.go(1 hunks)src/internal/ui/sidebar/directory_utils.go(1 hunks)src/internal/ui/sidebar/navigation.go(3 hunks)src/internal/ui/sidebar/navigation_test.go(28 hunks)src/internal/ui/sidebar/render.go(1 hunks)src/internal/ui/sidebar/sidebar.go(1 hunks)src/internal/ui/sidebar/type.go(1 hunks)src/internal/ui/sidebar/utils.go(1 hunks)src/internal/ui/sidebar/utils_test.go(1 hunks)src/internal/wheel_function.go(2 hunks)
💤 Files with no reviewable changes (3)
- src/internal/handle_pinned_operations.go
- src/internal/internal_consts.go
- src/internal/get_data.go
🧰 Additional context used
🧬 Code Graph Analysis (15)
src/internal/type.go (1)
src/internal/ui/prompt/type.go (1)
Model(6-24)
src/internal/ui/sidebar/render.go (6)
src/internal/ui/sidebar/type.go (1)
Model(10-17)src/internal/common/predefined_variable.go (4)
SideBarSuperfileTitle(26-26)SideBarNoneText(29-29)SideBarPinnedDivider(27-27)SideBarDisksDivider(28-28)src/internal/common/style_function.go (1)
SideBarBorderStyle(45-60)src/internal/ui/sidebar/consts.go (3)
SideBarInitialHeight(17-17)PinnedDividerDir(6-9)DiskDividerDir(11-14)src/internal/common/style.go (3)
SidebarStyle(18-18)SidebarSelectedStyle(26-26)FilePanelCursorStyle(30-30)src/internal/common/string_function.go (1)
TruncateText(18-25)
src/internal/ui/sidebar/utils_test.go (2)
src/internal/ui/sidebar/directory_utils.go (1)
FormDirctorySlice(27-38)src/internal/ui/sidebar/type.go (1)
Model(10-17)
src/internal/default_config.go (3)
src/internal/ui/sidebar/sidebar.go (1)
New(111-117)src/internal/ui/sidebar/directory_utils.go (1)
GetDirectories(23-25)src/internal/common/style_function.go (1)
GenerateSearchBar(241-253)
src/internal/ui/sidebar/navigation_test.go (2)
src/internal/ui/sidebar/type.go (1)
Model(10-17)src/internal/ui/sidebar/directory_utils.go (1)
FormDirctorySlice(27-38)
src/internal/handle_file_operations.go (1)
src/internal/common/string_function.go (2)
IsExensionExtractable(156-171)FileNameWithoutExtension(97-106)
src/internal/common/icon_utils.go (2)
src/config/icon/icon.go (4)
Style(4-7)Folders(397-417)Icons(50-165)Aliases(167-395)src/internal/common/default_config.go (2)
Config(11-11)Theme(10-10)
src/internal/model_render.go (1)
src/internal/common/string_function.go (7)
TruncateTextBeginning(27-46)PrettierName(60-73)TruncateText(18-25)TruncateMiddleText(48-58)ClipboardPrettierName(83-95)MakePrintable(194-206)CheckAndTruncateLineLengths(128-142)
src/internal/function.go (2)
src/internal/ui/sidebar/directory_utils.go (1)
FzfSearch(203-209)src/internal/common/string_function.go (1)
FormatFileSize(108-125)
src/internal/ui/sidebar/navigation.go (2)
src/internal/ui/sidebar/type.go (1)
Model(10-17)src/internal/ui/sidebar/consts.go (1)
SideBarInitialHeight(17-17)
src/internal/key_function.go (1)
src/internal/common/default_config.go (1)
Hotkeys(12-12)
src/internal/ui/sidebar/sidebar.go (3)
src/internal/ui/sidebar/type.go (1)
Model(10-17)src/internal/common/style_function.go (1)
GeneratePinnedRenameTextInput(300-315)src/internal/ui/sidebar/directory_utils.go (3)
GetPinnedDirectories(60-85)GetFilteredDirectories(194-200)GetDirectories(23-25)
src/internal/common/string_function_test.go (1)
src/internal/common/string_function.go (7)
TruncateText(18-25)TruncateTextBeginning(27-46)TruncateMiddleText(48-58)FileNameWithoutExtension(97-106)IsBufferPrintable(145-153)IsExensionExtractable(156-171)MakePrintable(194-206)
src/internal/ui/sidebar/directory_utils.go (4)
src/internal/ui/sidebar/consts.go (2)
PinnedDividerDir(6-9)DiskDividerDir(11-14)src/config/icon/icon.go (11)
Home(14-14)Space(10-10)Download(15-15)Documents(16-16)Pictures(17-17)Videos(18-18)Music(19-19)Templates(20-20)PublicShare(21-21)Error(34-34)Search(39-39)src/config/fixed_variable.go (1)
PinnedFile(40-40)src/internal/utils/consts.go (1)
OsWindows(8-8)
src/internal/common/string_function.go (4)
src/internal/common/icon_utils.go (1)
GetElementIcon(10-63)src/internal/common/style_function.go (1)
StringColorRender(210-214)src/internal/common/style.go (3)
FilePanelItemSelectedStyle(38-38)FilePanelStyle(17-17)FooterBGColor(78-78)src/internal/common/default_config.go (1)
Config(11-11)
🪛 LanguageTool
src/internal/ui/sidebar/ReadMe.md
[uncategorized] ~6-~6: Possible missing preposition found.
Context: ...es # To-dos - Add missing unit tests - Separate out implementation of file I/O operatio...
(AI_HYDRA_LEO_MISSING_TO)
[duplication] ~11-~11: Possible typo: you repeated a word.
Context: ...e more pure by reducing side effects # Coverage # Coverage ```bash cd /path/to/ui/prompt go test ...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.17.2)
src/internal/ui/sidebar/ReadMe.md
12-12: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🪛 GitHub Check: build
src/internal/ui/sidebar/directory_utils.go
[failure] 194-194:
unexported-return: exported func GetFilteredDirectories returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 168-168:
unexported-return: exported func FuzzySearch returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 88-88:
unexported-return: exported func GetExternalMediaFolders returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 60-60:
unexported-return: exported func GetPinnedDirectories returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 41-41:
unexported-return: exported func GetWellKnownDirectories returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 27-27:
unexported-return: exported func FormDirctorySlice returns unexported type []sidebar.directory, which can be annoying to use (revive)
[failure] 23-23:
unexported-return: exported func GetDirectories returns unexported type []sidebar.directory, which can be annoying to use (revive)
🪛 GitHub Actions: Go
src/internal/ui/sidebar/directory_utils.go
[error] 23-23: unexported-return: exported func GetDirectories returns unexported type []sidebar.directory, which can be annoying to use (revive)
🔇 Additional comments (88)
src/internal/ui/prompt/tokenize.go (1)
11-11: Import path updated correctly.The import path for the
utilspackage has been updated to reflect its new location directly under theinternaldirectory instead of being nested undercommon. This change is consistent with the architectural changes across the project.src/internal/common/load_config.go (1)
13-13: Import path updated correctly.The import path for the
utilspackage has been updated to match its new location in the project structure. This is consistent with the larger refactoring effort to reorganize the codebase.src/cmd/main.go (1)
16-16: Import path updated correctly.The import path for the
utilspackage has been properly updated to reflect its new location. This change is consistent with the same modifications made in other files.src/internal/ui/sidebar/type.go (1)
1-18: Well-structured type definitions for the sidebar.The new type definitions create a clear and organized structure for the sidebar component, properly encapsulating the directory data and UI state in a dedicated package. The JSON tags on the
directorytype are correctly implemented for serialization purposes.src/internal/config_function.go (1)
10-10: Update Import Path for Utils Package
The import on line 10 has been updated to reference the reorganized location (internal/utils). This change is consistent with the overall refactoring and should ensure that any call toutils.PrintfAndExit(as seen on line 28) functions correctly.src/config/fixed_variable.go (1)
8-8: Update Import Path for Utils Package
The import on line 8 now correctly points togithub.com/yorukot/superfile/src/internal/utilsfollowing the package reorganisation. Verify that all utility function calls (such as those in the CLI error handling around lines 79–81) work as expected after this change.src/internal/model_prompt_test.go (1)
16-16: Update Import Path for Utils Package in Test File
The import on line 16 now correctly reflects the newutilspackage location. This update maintains consistency across test files and meets the refactoring objectives without altering test logic.src/internal/file_operations.go (1)
12-12: Update Import Path for Utils Package in File Operations
The import on line 12 has been updated to the new path. The subsequent usage (e.g., checkingruntime.GOOSagainstutils.OsWindowson line 33) suggests that the refactor has been correctly applied here.src/internal/ui/prompt/model_test.go (1)
12-12: Update Import Path for Utils Package in UI Prompt Tests
The import change on line 12 aligns the test file with the new package structure. This ensures that utility functions (for instance, those setting up the root logger on lines 34–37) are sourced from the correct location.src/internal/default_config.go (2)
22-22: Good refactoring to use the new sidebar package.This change effectively encapsulates the sidebar functionality in its own package, improving modularity. The use of
sidebar.Newwith the appropriate parameters (sidebar.GetDirectories()andcommon.GenerateSearchBar()) maintains the same functionality while adhering to better separation of concerns.
6-6: LGTM! Import added for new sidebar package.The import statement for the new sidebar package is correctly added, enabling the use of the refactored sidebar model.
src/internal/wheel_function.go (2)
28-28: Method name changed from unexported to exported.Similarly to the
ListUpmethod, the change fromlistDowntoListDownrepresents a shift from an unexported to an exported method. This maintains consistency with the naming changes across the codebase.
16-16:✅ Verification successful
Method name changed from unexported to exported.
The method name change from
listUptoListUpindicates a shift from an unexported to an exported method. This is consistent with Go's naming conventions for public APIs but also implies a change in the API visibility.
🏁 Script executed:
#!/bin/bash # Check if ListUp is called from outside the sidebar package, which would justify making it public echo "Checking for usage of ListUp outside the sidebar package..." rg -g "!src/internal/ui/sidebar/*" "\.ListUp\(" src/Length of output: 351
Exporting
ListUpis Justified Based on Its External Usage
The grep results confirm thatListUpis invoked not only insrc/internal/wheel_function.gobut also insrc/internal/key_function.go, which lies outside the sidebar package. This external usage validates the decision to change the method from unexported (listUp) to exported (ListUp), aligning with Go's naming conventions for public APIs.
- File:
src/internal/wheel_function.go(Line 16):action = func() { m.sidebarModel.ListUp(m.mainPanelHeight) }- File:
src/internal/key_function.go: Invocation ofm.sidebarModel.ListUp(m.mainPanelHeight)src/internal/type.go (2)
6-6: LGTM! Import added for new sidebar package.The import statement for the sidebar package is correctly added to replace the previous local sidebar model implementation.
80-80: Good refactoring for better code organization.The type of
sidebarModelhas been changed from a locally defined structure tosidebar.Model, which is imported from the sidebar package. This change improves modularity by encapsulating sidebar functionality in its own package.src/internal/handle_file_operations.go (3)
14-14: Import path updated for utils package.The import path has been updated from the previous location
github.com/yorukot/superfile/src/internal/common/utilsto the new locationgithub.com/yorukot/superfile/src/internal/utils. This change is part of the broader package reorganization in this PR.
505-505: Function call updated to use exported common package function.The function call was changed from
isExensionExtractable(ext)tocommon.IsExensionExtractable(ext), which is consistent with the code separation and package reorganization in this PR.
510-510: Function call updated to use exported common package function.The function call was changed from
fileNameWithoutExtension(panel.element[panel.cursor].location)tocommon.FileNameWithoutExtension(panel.element[panel.cursor].location), which is consistent with the code separation and package reorganization in this PR.src/internal/handle_panel_navigation.go (2)
8-10: Import path added for the new sidebar package.A new import for the sidebar package has been added, which contains the pinned directory functionality that was moved out as part of the sidebar code separation.
18-22: Simplified pinned directory management using the new sidebar package.The implementation has been significantly simplified by delegating the responsibility of managing pinned directories to the dedicated
sidebarpackage. This improves code organization and maintainability.The previous implementation likely contained manual list management, JSON marshaling, and file writing operations that have now been encapsulated in the
TogglePinnedDirectoryfunction.src/internal/type_utils.go (2)
7-7: Import path updated for utils package.The import path has been updated from the previous location to the new location
github.com/yorukot/superfile/src/internal/utils, consistent with other files in this PR.
10-10: Added constant for standardizing invalid type string representation.A new constant
invalidTypeStringwith the value"InvalidType"has been added. This constant is used in theString()methods of various types to return a standardized string when an invalid type is encountered. This improves consistency in error reporting.src/internal/ui/sidebar/navigation_test.go (4)
1-1: Package declaration updated to the new sidebar package.The package declaration has been updated to
sidebaras part of the sidebar code separation.
11-15: Type updated fromsidebarModeltoModel.The type has been updated from
sidebarModeltoModelthroughout the file as part of the sidebar code reorganization. This follows Go's convention of using shorter names for exported types in dedicated packages.Also applies to: 16-20, 90-92, 93-97, 98-102, 103-107, 110-112
12-13: Function name updated to PascalCase for exported function.The function name has been updated from
formDirctorySlicetoFormDirctorySliceto follow Go's naming convention for exported functions (PascalCase).Also applies to: 17-18, 94-95, 99-100, 104-105
463-463: Method names updated to PascalCase for exported methods.The method names have been updated from
listUpandlistDowntoListUpandListDownto follow Go's naming convention for exported methods (PascalCase).Also applies to: 589-589
src/internal/common/icon_utils.go (1)
29-63: Ensure thorough coverage of custom alias resolution
The code resolves both extension-based and full-name aliases, then merges the style from the best match. This looks robust. Ensure you have test cases covering corner cases like empty file names, uppercase file extensions, etc.src/internal/function.go (3)
19-21: Updated imports
The new imports align with the recent sidebar refactor. No immediate issues found.
185-185: Confirm search fallback behavior
Note thatsidebar.FzfSearchdoesn't appear to handle empty or nil input scenarios. IffileAndDirectoriesis empty, the function returns an empty result. Ensure this behavior is acceptable.
361-361: Ensure consistent file size formatting
Usage ofcommon.FormatFileSize(fileInfo.Size())for files is straightforward. Just ensure that for extremely large files, the function retains good performance.src/internal/ui/sidebar/render.go (1)
13-15: Graceful handling of zero-width sidebar
Returning an empty string ifSidebarWidth == 0is a clean approach to disable the sidebar. Good usage.src/internal/model.go (11)
16-16: Updated import path confirmed.
The switch to the internalutilspackage aligns with the reorganized import structure and appears consistent with the rest of the refactoring.
65-65: Seamless sidebar integration.
Callingm.sidebarModel.UpdateState(msg)correctly delegates sidebar-related updates to the new model. This encapsulation should improve code maintainability.
228-229: Renaming overlap check.
The code separately checksm.fileModel.renamingandm.sidebarModel.IsRenaming()in adjacent cases. If both are true simultaneously, the logic might skip one. Consider verifying that renaming states cannot conflict.
231-232: Targeted search focus.
This case-handling ensures the file panel search bar takes priority when focused. Looks coherent with the sidebar approach.
239-240: Key handling for help menu.
The conditioncase m.helpMenu.open:is straightforward. No issues found.
242-243: Graceful quit confirmation.
This block checksm.confirmToQuit. The sequence for confirming or aborting looks logical.
565-565: Ensuring printable lines.
common.MakePrintable(line)is a good safeguard against unprintable ASCII characters.
637-637: Preview naming style.
Usingcommon.PrettierDirectoryPreviewNameensures consistent styling for directory listing. No obvious issues.
642-642: Effective line length restriction.
common.CheckAndTruncateLineLengthshelps avoid horizontal overflow. Looks solid.
672-672: Verifying file type detection.
common.IsTextFile(itemPath)is used to detect text files. Ensure it handles edge cases (e.g., zero-byte files or large binaries).
712-712: Preventing output spillage.
Truncating line widths for code previews maintains a clean layout in the preview panel.src/internal/model_render.go (7)
19-19: Updated import path confirmed.
Switching togithub.com/yorukot/superfile/src/internal/utilsmatches the overall refactoring pattern.
29-29: Clean delegation to sidebar renderer.
return m.sidebarModel.Render(...)centralizes the sidebar drawing logic. This improves code modularity.
46-46: Concise top directory display.
Usingcommon.TruncateTextBeginning(...)helps handle longer path names gracefully. No concerns.
132-132: Cursor & name styling.
common.PrettierNamewithcommon.FilePanelCursorStyleis consistent with existing styling patterns. Confirm that large file sets perform efficiently.
240-240: Process bar display improvement.
Truncating the process name to avoid overflow is an elegant approach for limited footer space.
311-311: Truncating metadata values.
common.TruncateMiddleTexteffectively prevents extra-wide lines in the metadata section.
314-314: Ensuring metadata key fits.
Similar logic for the metadata name keeps columns tidy in the footer panel.src/internal/ui/sidebar/sidebar.go (6)
1-14: Package & imports initialization check.
Definingpackage sidebarand referencingjson,slices,textinput, etc., is straightforward. The new file cleanly encapsulates sidebar logic.
15-28: Pinned item rename flow.
PinnedItemRename()sets up renaming with a text input model. Check that the cursor logic (cursorPos := len(...)) remains valid if the pinned item name is empty.
30-34: User cancellation path.
CancelSidebarRename()effectively resets the renaming state. No issues found.
67-81: Central update logic.
UpdateStateneatly updates renaming or search bar states. Good separation of concerns.
82-93: Handling search bar key events.
HandleSearchBarKeyuses the known hotkeys fromcommon.Hotkeys. Confirm we’ve covered all relevant key combos (e.g., arrow keys if needed for text navigation).
110-118: Sidebar model constructor.
New(...)cleanly initializes the sidebar state. Straightforward design that fosters clarity.src/internal/ui/sidebar/utils_test.go (1)
27-67: Well-structured test for NoActualDir.The table-driven test approach with descriptive names and clear assertions is excellent. It covers various scenarios including empty sidebar, sidebar with only pinned directories, and sidebar with all directories.
src/internal/ui/sidebar/utils.go (1)
3-11: Clean implementation of directory utility methods.The
IsDividerandRequiredHeightmethods are well implemented, withRequiredHeightproperly leveraging theIsDividermethod for determining the appropriate height.src/internal/handle_panel_movement.go (4)
10-10: Updated import path for utils package.The import path has been changed from
github.com/yorukot/superfile/src/internal/common/utilstogithub.com/yorukot/superfile/src/internal/utils, reflecting a reorganization of the package structure. This is consistent with the PR objective of code separation.
122-124: Updated to use encapsulated NoActualDir method.Changed from
m.sidebarModel.noActualDir()tom.sidebarModel.NoActualDir(), following Go's naming convention for exported methods.
134-134: Better encapsulation with GetCurrentDirectoryLocation.Now using
m.sidebarModel.GetCurrentDirectoryLocation()instead of directly accessingm.sidebarModel.directories[m.sidebarModel.cursor].location. This improves encapsulation by hiding the internal structure of the sidebar model.
207-216: Improved sidebar search bar focus handling.The changes simplify the control flow and improve encapsulation:
- Using
SearchBarFocused()method instead of directly accessing the model- Using
SearchBarBlur()method for better encapsulation- Early return when search bar is already focused
This makes the code cleaner and more maintainable.
src/internal/key_function.go (5)
22-22: Updated method call from listUp to ListUp.Changed from
m.sidebarModel.listUp(m.mainPanelHeight)tom.sidebarModel.ListUp(m.mainPanelHeight), following Go's naming convention for exported methods and aligning with the sidebar code refactoring.
39-39: Updated method call from listDown to ListDown.Changed from
m.sidebarModel.listDown(m.mainPanelHeight)tom.sidebarModel.ListDown(m.mainPanelHeight), following Go's naming convention for exported methods and aligning with the sidebar code refactoring.
150-150: Updated method call from pinnedItemRename to PinnedItemRename.Changed from
m.sidebarModel.pinnedItemRename()tom.sidebarModel.PinnedItemRename(), following Go's naming convention for exported methods and aligning with the sidebar code refactoring.
303-303: Updated method call from cancelSidebarRename to CancelSidebarRename.Changed from
m.sidebarModel.cancelSidebarRename()tom.sidebarModel.CancelSidebarRename(), following Go's naming convention for exported methods and aligning with the sidebar code refactoring.
305-305: Updated method call from confirmSidebarRename to ConfirmSidebarRename.Changed from
m.sidebarModel.confirmSidebarRename()tom.sidebarModel.ConfirmSidebarRename(), following Go's naming convention for exported methods and aligning with the sidebar code refactoring.src/internal/ui/sidebar/navigation.go (5)
1-5: Package name refactoring looks good.The package rename to
sidebaraligns with the code separation objective and helps clarify module responsibilities.
7-28: Discrepancy with removednoActualDir().According to the AI-generated summary,
noActualDir()was removed, yet this code callss.NoActualDir(). This is an inconsistency that needs resolution.Please confirm if
NoActualDir()is still defined elsewhere or if it should be removed. You can run this script to locate its definition:#!/bin/bash # Searching for the NoActualDir function definition ast-grep --pattern $'func (s *Model) NoActualDir($_) {\n $$$\n}'Likely an incorrect or invalid review comment.
30-54: Balanced cursor logic.The downward navigation logic mirrors the
ListUpmethod, ensuring consistent navigation. Reviewed and found no issues.
60-71: Index calculation is clear.Summing directory heights until exceeding the panel limit is straightforward and looks correct.
75-91: Reverse index calculation is sound.Iterating from
endIndexupward similarly ensures a correct start index for rendering. Looks good.src/internal/common/string_function_test.go (5)
1-1: Updated package path.Renaming the package to
commonfor these test utilities is consistent with the new code structure.
17-25: Unit tests for text truncation appear correct.All references to the newly exported functions
TruncateText,TruncateTextBeginning, andTruncateMiddleTextalign with the naming conventions, and the expected results match the code logic.
53-53: Filename test is valid.The test call to
FileNameWithoutExtensionis consistent with the refactored function name.
81-81: Buffer printability test is valid.
IsBufferPrintableusage and assertions conform to the intended behavior.
140-140: MakePrintable usage is correct.The
MakePrintablefunction is called properly and validates the test scenario thoroughly.src/internal/ui/sidebar/directory_utils.go (6)
1-20: Initial file structure and imports look well-organized.The new file clearly separates directory-related utilities into a dedicated module.
110-142: Physical disk filtering logic is solid.
ShouldListDisk()properly checks mount points, including a special case for Windows.
144-156: Drive naming approach.
DiskName()appropriately handles the differences between Windows drive letters and Unix directory structures.
158-165: Windows-specific path fix is appropriate.
DiskLocation()ensures the path ends with a backslash for Windows root directories.
202-209: FzfSearch is unexported and well-structured.The concurrency flow with channels and final
End()call is handled properly.
211-242: TogglePinnedDirectory logic is robust.The function gracefully handles adding or removing directories. The JSON marshaling and file write are neatly handled.
src/internal/common/string_function.go (6)
1-1: Good choice changing package name to follow Go visibility conventions.The package has been renamed from
internaltocommon, which better reflects the utility nature of these functions and makes them appropriately accessible to other parts of the codebase.
18-25: Appropriate function name capitalization for exported Go functions.The functions have been properly renamed from camelCase to PascalCase, following Go's convention for exported functions. This change improves consistency across the codebase and makes the API more idiomatic.
Also applies to: 27-46, 48-58
60-73: Approve style function referencing changes.The function is now using
GetElementIcon, direct access to style functions and variables, reflecting the package name change. The code is more concise without the redundant package prefix.
117-117: Direct Config access is appropriate with package change.Now that the functions are in the
commonpackage, the direct reference toConfig.FileSizeUseSIwithout the package prefix is correct.
127-142: Comment matches function purpose.The comment "Truncate line lengths and keep ANSI" accurately describes what the function does. The function preserves ANSI escape sequences while truncating each line to the specified maximum length.
174-188: Function now correctly uses renamed helper.The function now calls
IsBufferPrintable()with its new PascalCase name, maintaining consistency with other naming changes.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
195c931 to
e654b25
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/internal/function.go (1)
400-419: 🛠️ Refactor suggestionConsider optimizing directory size calculation.
The
dirSize()function recursively walks the directory tree, which can be expensive for large directories and cause UI freezes.Consider implementing an asynchronous approach with progress updates:
func dirSize(path string) int64 { + // Check if we have a cached size first + if cachedSize, ok := dirSizeCache[path]; ok { + return cachedSize + } + var size int64 // Its named walkErr to prevent shadowing walkErr := filepath.WalkDir(path, func(_ string, entry os.DirEntry, err error) error { if err != nil { slog.Error("Dir size function error", "error", err) } if !entry.IsDir() { info, infoErr := entry.Info() if infoErr == nil { size += info.Size() } } return err }) if walkErr != nil { slog.Error("errors during WalkDir", "error", walkErr) } + + // Cache the result for future queries + dirSizeCache[path] = size return size }For a fully asynchronous implementation, you'd need to:
- Create a background goroutine for calculation
- Implement a channel for progress updates
- Add timeout handling
- Manage the state in your metadata panel
♻️ Duplicate comments (3)
src/internal/ui/sidebar/directory_utils.go (1)
18-22:⚠️ Potential issueExport "directory" struct or return exported types in exported functions.
All exported functions that return
[]directoryare triggering lint warnings because they're returning an unexported type. This appears in multiple functions throughout the file.Since this issue was flagged in multiple past review comments, you should either:
- Export the
directorystruct by renaming it toDirectory- Make the functions unexported if they're only meant for internal use
This will resolve all the unexported-return lint warnings shown in the pipeline failures.
src/internal/function.go (1)
328-328: Potential performance concern when calculating directory size.Calling
dirSize()could be expensive for large directories, as it recursively walks the entire tree. Consider lazy loading, caching, or an async approach to avoid UI lockups.src/internal/ui/sidebar/utils_test.go (1)
12-14:⚠️ Potential issueFix range syntax in
dirSlicefunction.The
rangekeyword in Go doesn't work directly with integers. When applied to an integer, it attempts to iterate over a collection with that value, not generate a sequence of numbers.Apply this fix:
- for i := range count { + for i := 0; i < count; i++ { res[i] = directory{Name: "Dir" + strconv.Itoa(i), Location: "/a/" + strconv.Itoa(i)} }
🧹 Nitpick comments (7)
src/internal/ui/sidebar/directory_utils.go (1)
119-149: Well-implemented TogglePinnedDirectory function.The new function effectively centralizes the logic for toggling directories in the pinned list:
- It properly handles both adding and removing directories
- It includes comprehensive error handling for JSON operations
- The function returns appropriate errors with context
Consider adding a log statement for successful operations to aid in troubleshooting.
err = os.WriteFile(variable.PinnedFile, updatedData, 0644) if err != nil { return fmt.Errorf("error writing pinned directories file: %w", err) } + // Log the operation for troubleshooting purposes + if unPinned { + slog.Debug("Directory unpinned", "location", dir) + } else { + slog.Debug("Directory pinned", "location", dir) + } return nilsrc/internal/ui/sidebar/navigation_test.go (1)
1-598: Consider using table-driven test helpers for more concise tests.The test file contains repetitive test patterns that could be refactored to use more reusable table-driven test helpers, reducing duplication and improving maintainability.
For example, you could refactor the similar structure between
Test_listUpandTest_listDownby creating a shared test runner function that takes a method to test as a parameter:func runDirectionTest(t *testing.T, testCases []directionalTestCase, methodToTest func(*Model, int)) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { sidebar := tt.sidebar methodToTest(&sidebar, tt.mainPanelHeight) assert.Equal(t, tt.expectedCursor, sidebar.cursor, "cursor position: %s", tt.explanation) assert.Equal(t, tt.expectedRenderIndex, sidebar.renderIndex, "render index: %s", tt.explanation) }) } } // Then in your test functions: func Test_listUp(t *testing.T) { // Define testCases... runDirectionTest(t, testCases, func(m *Model, height int) { m.ListUp(height) }) }src/internal/ui/sidebar/disk_utils.go (2)
24-26: Opportunity to optimize OS checksAs noted in the comment, there are multiple runtime.GOOS checks across shouldListDisk, diskName, and diskLocation functions. Consider refactoring to do this check once and use a strategy pattern or conditional initialization to avoid repeating this check.
36-68: Consider implementing the TODO items for disk mount configurationThe function contains several TODO comments about making disk mount points configurable via a config.yaml file. This would improve the flexibility of the application for users with non-standard mount points.
You could implement this by:
- Reading mount configurations from config.yaml
- Allowing users to specify included/excluded mount points
- Preserving the default behavior for common cases
src/internal/ui/sidebar/utils_test.go (3)
19-19: Possible typo in function nameformDirctorySliceThe function name appears to have a typo - "Dirctory" instead of "Directory". Consider renaming to
formDirectorySlicefor better readability and consistency.
22-25: TODO items for test improvementsConsider implementing the TODO items to improve the test structure:
- Using
t.Runfor each test case (already implemented in the tests below)- Eliminating global variables
- Moving test data into each test function
- Adding descriptive names for test cases
These improvements would make the tests more maintainable and easier to understand.
90-90: Typo in test case nameThere's a typo in the test case name: "Curson" should be "Cursor".
- "Curson points to pinned divider", + "Cursor points to pinned divider",
🛑 Comments failed to post (1)
src/internal/utils/fzf_utils.go (1)
5-12: 🛠️ Refactor suggestion
Implement error handling and documentation for FzfSearch.
The function lacks error handling for potential channel communication issues and detailed documentation explaining why returning MatchResult is more efficient than strings.
-// Returning a string slice causes inefficiency in current usage +// FzfSearch performs fuzzy searching on a source list of strings using the given query. +// It returns MatchResult objects directly instead of converting to string slices to avoid +// unnecessary memory allocations and preserve match positions for highlighting. func FzfSearch(query string, source []string) []fzf.MatchResult { fzfSearcher := fzf.New(source, fzf.DefaultOptions()) fzfSearcher.Search(query) - fzfResults := <-fzfSearcher.GetResultChannel() + // Use select with timeout to prevent blocking indefinitely + var fzfResults fzf.SearchResults + select { + case results := <-fzfSearcher.GetResultChannel(): + fzfResults = results + case <-time.After(2 * time.Second): + // Return empty results after timeout + fzfSearcher.End() + return []fzf.MatchResult{} + } fzfSearcher.End() return fzfResults.Matches }You'll need to add
"time"to your imports to implement the timeout handling.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// FzfSearch performs fuzzy searching on a source list of strings using the given query. // It returns MatchResult objects directly instead of converting to string slices to avoid // unnecessary memory allocations and preserve match positions for highlighting. func FzfSearch(query string, source []string) []fzf.MatchResult { fzfSearcher := fzf.New(source, fzf.DefaultOptions()) fzfSearcher.Search(query) // Use select with timeout to prevent blocking indefinitely var fzfResults fzf.SearchResults select { case results := <-fzfSearcher.GetResultChannel(): fzfResults = results case <-time.After(2 * time.Second): // Return empty results after timeout fzfSearcher.End() return []fzf.MatchResult{} } fzfSearcher.End() return fzfResults.Matches }
|
@coderabbitai correct you generated summary. There is no new feature. Its just a refactoring and code reorganization. |
|
Tip For best results, initiate chat on the files or code changes. @lazysegtree You're absolutely right, and I apologize for the inaccuracy in my summary. Thank you for the correction. The PR is indeed just a refactoring and code reorganization effort with no new features added. It focuses on moving code around, updating import paths, renaming functions to follow consistent PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/internal/ui/sidebar/directory_utils.go (1)
29-32: 🛠️ Refactor suggestionVariable references need to be updated
If the variables in
consts.goare renamed toPinnedDividerDirandDiskDividerDir, the references here need to be updated accordingly.directories = append(directories, homeDirectories...) - directories = append(directories, pinnedDividerDir) + directories = append(directories, PinnedDividerDir) directories = append(directories, pinnedDirectories...) - directories = append(directories, diskDividerDir) + directories = append(directories, DiskDividerDir)
♻️ Duplicate comments (4)
src/internal/function.go (1)
328-328: Potential performance concern when calculating directory sizeCalling
dirSize()could be expensive for large directories, as it recursively walks the entire tree. Consider lazy loading, caching, or an async approach to avoid UI lockups.src/internal/ui/sidebar/render.go (1)
47-49: Potential UI issue with partial list renderingThe code breaks out of the loop if an item doesn't fit within the panel height, which could truncate the displayed list. This was flagged in a previous review. Ensure you have a mechanism for scrolling or pagination if the user has many directories.
This truncation approach works for the scroll mechanism already implemented via the
renderIndex, but consider adding a visual indicator when the list is truncated (such as ellipsis or a "more items below" message).src/internal/ui/sidebar/utils_test.go (1)
12-14: Fix range syntax indirSlicefunction.The
rangekeyword in Go doesn't work directly with integers. When applied to an integer, it attempts to iterate over a collection with that value, not generate a sequence of numbers.Apply this fix:
- for i := range count { + for i := 0; i < count; i++ { res[i] = directory{Name: "Dir" + strconv.Itoa(i), Location: "/a/" + strconv.Itoa(i)} }src/internal/ui/sidebar/utils.go (1)
71-90: 🛠️ Refactor suggestionOptimize pinned index computation as noted in the TODO.
Computing divider indexes on every call is inefficient. Consider caching these values when the directory structure changes.
Update the Model struct to include these indexes as fields:
// Add to Model struct in type.go: type Model struct { directories []directory renderIndex int cursor int rename textinput.Model renaming bool searchBar textinput.Model + pinnedDividerIdx int + diskDividerIdx int } // Then add this method to update the indexes: func (s *Model) updateDividerIndexes() { s.pinnedDividerIdx = -1 s.diskDividerIdx = -1 for i, d := range s.directories { if d == pinnedDividerDir { s.pinnedDividerIdx = i } if d == diskDividerDir { s.diskDividerIdx = i break } } } // Then simplify pinnedIndexRange to use cached values: func (s *Model) pinnedIndexRange() (int, int) { return s.pinnedDividerIdx + 1, s.diskDividerIdx - 1 }Call
updateDividerIndexes()wherevers.directoriesis modified.
🧹 Nitpick comments (12)
src/internal/ui/sidebar/disk_utils.go (2)
24-26: Consider optimizing multiple OS checks.As noted in your comment, there are multiple
runtime.GOOSchecks across the three functions. You could optimize this by performing the check once and storing the result, or by having OS-specific implementations.func getExternalMediaFolders() []directory { // only get physical drives parts, err := disk.Partitions(false) if err != nil { slog.Error("Error while getting external media: ", "error", err) return nil } var disks []directory + isWindows := runtime.GOOS == utils.OsWindows for _, disk := range parts { - // ShouldListDisk, DiskName, and DiskLocation, each has runtime.GOOS checks - // We can ideally reduce it to one check only. - if shouldListDisk(disk.Mountpoint) { + if shouldListDisk(disk.Mountpoint, isWindows) { disks = append(disks, directory{ - Name: diskName(disk.Mountpoint), - Location: diskLocation(disk.Mountpoint), + Name: diskName(disk.Mountpoint, isWindows), + Location: diskLocation(disk.Mountpoint, isWindows), }) } } return disks } func shouldListDisk(mountPoint string, isWindows bool) bool { - if runtime.GOOS == utils.OsWindows { + if isWindows { // Implementation continues...
47-63: Consider implementing configuration for excluded mount points.The TODOs about making excluded mount points and disk mounts configurable via YAML are good suggestions for improving flexibility. Consider implementing this in a future PR to make the sidebar disk display more customizable.
Would you like help with implementing the configuration system for the disk mounts in a subsequent PR?
src/internal/ui/sidebar/directory_utils.go (3)
23-23: Misspelling in function nameThe function name
formDirctorySlicecontains a misspelling ("Dirctory" instead of "Directory"). Consider correcting this for better code readability.-func formDirctorySlice(homeDirectories []directory, pinnedDirectories []directory, diskDirectories []directory) []directory { +func formDirectorySlice(homeDirectories []directory, pinnedDirectories []directory, diskDirectories []directory) []directory {
118-149: Refactor opportunity: Reduce JSON file operation duplicationThe
TogglePinnedDirectoryfunction shares similar JSON file operations withgetPinnedDirectories. Consider extracting the common operations (reading the pinned directories from file, marshaling, and writing back) to reduce code duplication.You could create helper functions such as:
func readPinnedDirectories() ([]directory, error) { directories := []directory{} jsonData, err := os.ReadFile(variable.PinnedFile) if err != nil { return directories, fmt.Errorf("error reading pinned directories file: %w", err) } // Handle the old format check if err := json.Unmarshal(jsonData, &directories); err != nil { // Try old format var paths []string if err := json.Unmarshal(jsonData, &paths); err == nil { for _, path := range paths { directoryName := filepath.Base(path) directories = append(directories, directory{Location: path, Name: directoryName}) } return directories, nil } return directories, fmt.Errorf("error parsing pinned directories: %w", err) } return directories, nil } func writePinnedDirectories(dirs []directory) error { updatedData, err := json.Marshal(dirs) if err != nil { return fmt.Errorf("error marshaling pinned directories: %w", err) } err = os.WriteFile(variable.PinnedFile, updatedData, 0644) if err != nil { return fmt.Errorf("error writing pinned directories file: %w", err) } return nil }Then the functions would be simplified:
func getPinnedDirectories() []directory { directories, err := readPinnedDirectories() if err != nil { slog.Error("Error reading pinned directories", "error", err) } return directories } func TogglePinnedDirectory(dir string) error { dirs, err := readPinnedDirectories() if err != nil { return err } unPinned := false // Toggle logic... return writePinnedDirectories(dirs) }
55-56: Warning: Possible inconsistency in error handlingIn
getPinnedDirectories, errors from JSON unmarshaling are logged but the function continues with potentially incomplete data. Consider adding more robust error handling or being consistent in either returning early or continuing with partial results.Consider adding a comment to clarify the error handling strategy or modifying the approach to be more consistent:
// Check if the data is in the new format if err := json.Unmarshal(jsonData, &directories); err != nil { // If the data is in neither format, log the error slog.Error("Error parsing pinned data", "error", err) + // Continuing with empty directories slice since parsing failed }Also applies to: 75-78
src/internal/ui/sidebar/render.go (1)
11-33: Consider adding width validation or handlingThe
Rendermethod checks for zero width viacommon.Config.SidebarWidth == 0, but doesn't handle cases where the width might be extremely small but non-zero. Consider adding validation or handling for very narrow sidebars.func (s *Model) Render(mainPanelHeight int, isSidebarFocussed bool, currentFilePanelLocation string) string { - if common.Config.SidebarWidth == 0 { + if common.Config.SidebarWidth <= 1 { // Ensure at least 2 characters width for content return "" } // Rest of the method... }src/internal/ui/sidebar/utils_test.go (3)
22-25: Consider implementing the TODO suggestions to improve test maintainability.The TODO comments outline several good practices for test structure:
- Using
t.Run()for subtests (already implemented in some tests)- Eliminating global variables
- Improving test data organization
These changes would enhance test maintainability and isolation.
90-96: Fix typo in test case name.The test case name contains a typo: "Curson" should be "Cursor".
- "Curson points to pinned divider", + "Cursor points to pinned divider",
139-139: Fix typo in comment for cursor position.There's a typo in the comment explaining the expected cursor position:
- expectedCursorPos: 2, // After pinned and dist divider + expectedCursorPos: 2, // After pinned and disk dividersrc/internal/ui/sidebar/navigation.go (3)
7-28: ListUp implementation is well-structured but contains inconsistent logging.The method correctly implements the list navigation behavior, but the log message still refers to "controlListUp" instead of "ListUp".
- slog.Debug("controlListUp called", "cursor", s.cursor, + slog.Debug("ListUp called", "cursor", s.cursor,Otherwise, the navigation logic is sound, handling edge cases appropriately.
30-54: ListDown implementation is well-structured but contains inconsistent logging.Similar to ListUp, the log message refers to "controlListDown" instead of "ListDown".
- slog.Debug("controlListDown called", "cursor", s.cursor, + slog.Debug("ListDown called", "cursor", s.cursor,The navigation logic itself is correct, handling edge cases properly.
7-54: Consider refactoring ListUp and ListDown to reduce code duplication.The ListUp and ListDown methods share similar logic with minor differences. Consider extracting common functionality to reduce duplication:
func (s *Model) navigateList(direction int, mainPanelHeight int) { if s.NoActualDir() { return } if direction < 0 && s.cursor > 0 { s.cursor-- } else if direction > 0 && s.cursor < len(s.directories)-1 { s.cursor++ } else if direction < 0 { // Wrap to bottom s.cursor = len(s.directories) - 1 } else { // Wrap to top s.cursor = 0 } s.updateRenderIndex(mainPanelHeight) if s.directories[s.cursor].IsDivider() { s.navigateList(direction, mainPanelHeight) } } func (s *Model) ListUp(mainPanelHeight int) { slog.Debug("ListUp called", "cursor", s.cursor, "renderIndex", s.renderIndex, "directory count", len(s.directories)) s.navigateList(-1, mainPanelHeight) } func (s *Model) ListDown(mainPanelHeight int) { slog.Debug("ListDown called", "cursor", s.cursor, "renderIndex", s.renderIndex, "directory count", len(s.directories)) s.navigateList(1, mainPanelHeight) }This approach reduces duplication while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/internal/default_config.go(2 hunks)src/internal/function.go(4 hunks)src/internal/ui/sidebar/consts.go(1 hunks)src/internal/ui/sidebar/directory_utils.go(5 hunks)src/internal/ui/sidebar/disk_utils.go(1 hunks)src/internal/ui/sidebar/navigation.go(4 hunks)src/internal/ui/sidebar/navigation_test.go(28 hunks)src/internal/ui/sidebar/render.go(1 hunks)src/internal/ui/sidebar/sidebar.go(1 hunks)src/internal/ui/sidebar/utils.go(1 hunks)src/internal/ui/sidebar/utils_test.go(1 hunks)src/internal/utils/fzf_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/internal/utils/fzf_utils.go
- src/internal/default_config.go
- src/internal/ui/sidebar/sidebar.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.663Z
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.
🧬 Code Graph Analysis (8)
src/internal/ui/sidebar/directory_utils.go (3)
src/config/icon/icon.go (9)
Home(14-14)Space(10-10)Download(15-15)Documents(16-16)Pictures(17-17)Videos(18-18)Music(19-19)Templates(20-20)PublicShare(21-21)src/internal/utils/fzf_utils.go (1)
FzfSearch(6-12)src/config/fixed_variable.go (1)
PinnedFile(40-40)
src/internal/function.go (2)
src/internal/utils/fzf_utils.go (1)
FzfSearch(6-12)src/internal/common/string_function.go (1)
FormatFileSize(108-125)
src/internal/ui/sidebar/utils_test.go (1)
src/internal/ui/sidebar/type.go (1)
Model(10-17)
src/internal/ui/sidebar/render.go (7)
src/internal/ui/sidebar/type.go (1)
Model(10-17)src/internal/common/default_config.go (2)
Config(11-11)Hotkeys(12-12)src/internal/common/predefined_variable.go (4)
SideBarSuperfileTitle(26-26)SideBarNoneText(29-29)SideBarPinnedDivider(27-27)SideBarDisksDivider(28-28)src/internal/common/style_function.go (1)
SideBarBorderStyle(45-60)src/config/icon/icon.go (1)
Cursor(31-31)src/internal/common/style.go (3)
SidebarStyle(18-18)SidebarSelectedStyle(26-26)FilePanelCursorStyle(30-30)src/internal/common/string_function.go (1)
TruncateText(18-25)
src/internal/ui/sidebar/disk_utils.go (2)
src/config/icon/icon.go (1)
Error(34-34)src/internal/utils/consts.go (1)
OsWindows(8-8)
src/internal/ui/sidebar/navigation.go (1)
src/internal/ui/sidebar/type.go (1)
Model(10-17)
src/internal/ui/sidebar/navigation_test.go (1)
src/internal/ui/sidebar/type.go (1)
Model(10-17)
src/internal/ui/sidebar/utils.go (1)
src/internal/ui/sidebar/type.go (1)
Model(10-17)
🔇 Additional comments (16)
src/internal/function.go (3)
19-19: New utils import added for refactored functionality.This import supports the move of the
fzfSearchfunction to the utils package as part of the sidebar code separation.
182-182: Function call updated to use refactored utility.The code now correctly uses
utils.FzfSearchinstead of the localfzfSearchfunction that was previously defined in this file. This change is part of the sidebar code separation effort mentioned in the PR objectives.
359-359: Function call updated to use common utility.The code now correctly uses
common.FormatFileSizeinstead of a local formatting function. This change is part of the standardization effort.src/internal/ui/sidebar/disk_utils.go (4)
13-34: Well-structured function for retrieving external media folders.The function appropriately handles errors and returns formatted disk information. It correctly utilizes the supporting functions for determining which disks to list and how to format their names and locations.
36-68: Well-documented disk listing logic.The function includes detailed comments explaining the filtering logic and thoughtfully includes TODOs for future configurability. The logic correctly handles different operating systems and mount point patterns.
70-82: Good OS-specific handling of disk names.The function correctly handles the different requirements for disk name formatting between Windows and other operating systems, with helpful comments explaining the reasoning.
84-91: Proper disk location path handling for cross-platform compatibility.The function correctly handles the different path format requirements between Windows and other operating systems, ensuring paths are correctly formatted for the respective file systems.
src/internal/ui/sidebar/directory_utils.go (1)
19-21:❌ Incorrect review comment
Function may reference a removed function
The
getDirectoriesfunction callsgetExternalMediaFolders(), but according to the AI-generated summary, this function has been removed from the codebase. Verify that this function still exists or update the code accordingly.
🏁 Script executed:
#!/bin/bash # Check if getExternalMediaFolders function is defined echo "Searching for getExternalMediaFolders function definition..." rg -A 2 -B 2 "func getExternalMediaFolders" src/Length of output: 518
Verification Confirmed:
getExternalMediaFolders()Still ExistsAfter verifying the codebase, we found that the function
getExternalMediaFolders()is defined insrc/internal/ui/sidebar/disk_utils.go. This confirms that the reference ingetDirectories()is valid, and the concern regarding a removed function is not applicable.
- Location Verified:
src/internal/ui/sidebar/disk_utils.gocontains the definition ofgetExternalMediaFolders().Likely an incorrect or invalid review comment.
src/internal/ui/sidebar/navigation_test.go (2)
12-14:✅ Verification successful
Maintain naming consistency
If
formDirctorySlicehas been renamed (or should be renamed) toFormDirectorySlicein the implementation files, the test should use the updated name.
🏁 Script executed:
#!/bin/bash # Check if formDirctorySlice is exported elsewhere echo "Searching for exported FormDirctorySlice function..." rg "func FormDirctorySlice" src/ echo "Searching for unexported formDirctorySlice function..." rg "func formDirctorySlice" src/Length of output: 472
Naming consistency verified: the test is correctly calling "formDirctorySlice".
The implementation in
src/internal/ui/sidebar/directory_utils.godefines the function asformDirctorySlice(unexported), which exactly matches the usage in the test file (src/internal/ui/sidebar/navigation_test.goat lines 12–14). No renaming is needed at this time. If you later decide to export or rename this function (e.g., toFormDirectorySlice), please ensure both the implementation and the test are updated consistently.
13-13:✅ Verification successful
Missing helper function definition
The test uses helper functions
dirSliceandfullDirSlice, but their definitions aren't included in the provided file. Make sure these helper functions are still available and properly defined.Also applies to: 18-18, 95-95, 99-99
🏁 Script executed:
#!/bin/bash # Check for dirSlice and fullDirSlice function definitions echo "Searching for dirSlice and fullDirSlice function definitions..." rg "func (full)?[Dd]irSlice" src/internal/ui/sidebar/Length of output: 350
Helper Functions Verified in Separate File
The helper functions (
dirSliceandfullDirSlice) are defined insrc/internal/ui/sidebar/utils_test.goand are available for use. Please confirm that referencing them from this separate file is intentional. If you prefer inlining or explicitly importing these helpers innavigation_test.gofor clarity, consider adding a comment to indicate their origin.
- Verified definitions:
dirSlicefound atsrc/internal/ui/sidebar/utils_test.gofullDirSlicefound atsrc/internal/ui/sidebar/utils_test.goThis applies to the instances at lines 13, 18, 95, and 99.
src/internal/ui/sidebar/utils.go (5)
13-24: The NoActualDir method is efficient but could be optimized.The method correctly checks if there are only dividers in the directory slice. The comment mentions it could be replaced with
len(s.directories) <= 2for better performance, but that would be less explicit and more prone to errors if the structure changes.As this is a small function that likely isn't in a hot path, the current implementation balances readability and performance well.
26-28: The isCursorInvalid method is concise and efficient.This method handles all possible invalid cursor conditions in a single expression:
- Cursor is negative
- Cursor is out of range
- Cursor points to a divider
Good implementation.
30-41: resetCursor implementation looks good.The method correctly sets the cursor to the first non-divider directory, with appropriate handling of edge cases where no valid directories exist.
43-61: Well-structured search bar and renaming methods.The methods for managing search bar focus and checking renaming status are clear, concise, and follow good encapsulation principles.
63-69: The GetCurrentDirectoryLocation method handles edge cases appropriately.The method returns an empty string for invalid cases and correctly retrieves the location otherwise. Good defensive programming.
src/internal/ui/sidebar/navigation.go (1)
56-120: Navigation helper methods are well-implemented.The methods
lastRenderedIndex,firstRenderedIndex, andupdateRenderIndexprovide appropriate logic for controlling what is visible in the sidebar view. Error handling and edge cases are addressed properly.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
aa2223c to
83b8f94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/internal/common/string_function.go (1)
199-204:⚠️ Potential issueFix the invalid “for i := range len(line)” syntax.
In Go,range len(line)is invalid. Use a traditional index-based loop orrange linefor byte-wise iteration.-for i := range len(line) { +for i := 0; i < len(line); i++ { r := rune(line[i]) if unicode.IsGraphic(r) || r == rune('\t') || r == rune('\n') { sb.WriteByte(line[i]) } }
♻️ Duplicate comments (1)
src/internal/common/string_function.go (1)
155-156:⚠️ Potential issueCorrect the spelling of “IsExensionExtractable”.
The current function and comment both misspell “Extension.” This can cause confusion and inconsistencies.-// IsExensionExtractable checks if a string is a valid compressed archive file extension. -func IsExensionExtractable(ext string) bool { +// IsExtensionExtractable checks if a string is a valid compressed archive file extension. +func IsExtensionExtractable(ext string) bool {
🧹 Nitpick comments (4)
src/internal/common/icon_utils_test.go (1)
9-83: Consider verifyingThemeusage explicitly.
Although the tests correctly validate icon logic, verifying thatTheme.FilePanelFGis set or mocked ensures consistent behavior ifThemechanges in the future. This can help catch edge cases in color handling.src/internal/common/icon_utils.go (1)
10-63: Good implementation of file/directory icon resolution.
The overall logic, including the fallback icon and handling of extension/name aliases, appears correct. However, you may consider ignoring case for directory names similarly to how file extensions are normalized. This can prevent missing an expected directory icon due to uppercase/lowercase discrepancies.if isDir { - resultIcon := icon.Folders["folder"] - betterIcon, hasBetterIcon := icon.Folders[name] + lowerName := strings.ToLower(name) + resultIcon := icon.Folders["folder"] + betterIcon, hasBetterIcon := icon.Folders[lowerName] if hasBetterIcon { resultIcon = betterIcon } return resultIcon }src/internal/common/string_function.go (2)
18-25: Add guard for potential negative maxChars.
IfmaxCharsis smaller thanlen(talis),ansi.Truncatemay behave unexpectedly. Consider adding a check to avoid negative indexing when constructingmaxChars-len(talis).func TruncateText(text string, maxChars int, talis string) string { + if maxChars < len(talis) { + return talis + } truncatedText := ansi.Truncate(text, maxChars-len(talis), "") if text != truncatedText { return truncatedText + talis } return text }
60-73: Reduce repetition in Prettier functions.*
PrettierName,PrettierDirectoryPreviewName, andClipboardPrettierNameshare similar logic (fetching icons, applying styles, rendering text). Consider extracting common code into a helper to follow DRY principles.Also applies to: 75-81, 83-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/common/icon_utils.go(1 hunks)src/internal/common/icon_utils_test.go(1 hunks)src/internal/common/string_function.go(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.312Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
🧬 Code Graph Analysis (3)
src/internal/common/icon_utils_test.go (2)
src/config/icon/icon.go (3)
Style(4-7)Folders(397-417)Icons(50-165)src/internal/common/icon_utils.go (1)
GetElementIcon(10-63)
src/internal/common/icon_utils.go (2)
src/config/icon/icon.go (4)
Style(4-7)Folders(397-417)Icons(50-165)Aliases(167-395)src/internal/common/default_config.go (1)
Theme(10-10)
src/internal/common/string_function.go (4)
src/internal/common/icon_utils.go (1)
GetElementIcon(10-63)src/internal/common/default_config.go (1)
Config(11-11)src/internal/common/style_function.go (1)
StringColorRender(210-214)src/internal/common/style.go (3)
FilePanelItemSelectedStyle(38-38)FilePanelStyle(17-17)FooterBGColor(78-78)
🔇 Additional comments (1)
src/internal/common/icon_utils_test.go (1)
1-8: No issues found in package declaration and imports.
Everything appears standard and consistent with the testing framework.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
83b8f94 to
a6b76cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/internal/function.go (1)
328-330: TODO comment addresses previous review feedback.Appropriate addition of a TODO to address the performance concern with
dirSize(). It's reasonable to defer this optimization for a future PR since this PR is focused on code reorganization only.src/internal/common/string_function.go (1)
155-156: Fixed spelling error in function name.The typo "Exension" has been correctly fixed to "Extension" in the function name, as noted in the previous review.
🧹 Nitpick comments (4)
src/internal/common/string_function.go (4)
18-25: Consider fixing parameter name "talis" to "tails" in truncation functions.The parameter name "talis" appears to be a typo and should probably be "tails" since it represents an ellipsis or other trailing characters.
-func TruncateText(text string, maxChars int, talis string) string { +func TruncateText(text string, maxChars int, tails string) string { - truncatedText := ansi.Truncate(text, maxChars-len(talis), "") + truncatedText := ansi.Truncate(text, maxChars-len(tails), "") if text != truncatedText { - return truncatedText + talis + return truncatedText + tails } return text }Similar changes would apply to the other truncation functions.
Also applies to: 27-46, 48-58
48-58: Consider improving ANSI handling in TruncateMiddleText.The current implementation uses
utf8.RuneCountInStringand direct string slicing, which may not handle ANSI color codes correctly. Consider using theansipackage's functionality for more reliable handling of styled text.func TruncateMiddleText(text string, maxChars int, talis string) string { - if utf8.RuneCountInString(text) <= maxChars { + if ansi.StringWidth(text) <= maxChars { return text } halfEllipsisLength := (maxChars - 3) / 2 - truncatedText := text[:halfEllipsisLength] + talis + text[utf8.RuneCountInString(text)-halfEllipsisLength:] + // Use ansi.Substring to correctly handle ANSI escape codes + prefix := ansi.Substring(text, 0, halfEllipsisLength) + suffix := ansi.Substring(text, ansi.StringWidth(text)-halfEllipsisLength, ansi.StringWidth(text)) + truncatedText := prefix + talis + suffix return truncatedText }
116-125: Address TODO comment about removing duplication.There's a TODO comment about removing duplication in the code. Consider addressing this by refactoring the conditional logic:
-// Todo : Remove duplication here -if Config.FileSizeUseSI { - unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(1000))) - adjustedSize := float64(size) / math.Pow(1000, float64(unitIndex)) - return fmt.Sprintf("%.2f %s", adjustedSize, unitsDec[unitIndex]) -} -unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(1024))) -adjustedSize := float64(size) / math.Pow(1024, float64(unitIndex)) -return fmt.Sprintf("%.2f %s", adjustedSize, unitsBin[unitIndex]) +// Choose base and units based on configuration +base := 1024.0 +units := unitsBin +if Config.FileSizeUseSI { + base = 1000.0 + units = unitsDec +} + +unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(base))) +adjustedSize := float64(size) / math.Pow(base, float64(unitIndex)) +return fmt.Sprintf("%.2f %s", adjustedSize, units[unitIndex])
194-206: Improve byte iteration in MakePrintable function.The current implementation uses a range over
len(line)which is byte-based, but then converts each byte to a rune. This approach is error-prone for multi-byte UTF-8 characters.func MakePrintable(line string) string { var sb strings.Builder - // This has to be looped byte-wise, looping it rune-wise - // or by using strings.Map would cause issues with strings like - // "(NBSP)\xa0" - for i := range len(line) { - r := rune(line[i]) + // Process each byte individually to correctly handle special cases + for i := 0; i < len(line); i++ { + r := rune(line[i]) if unicode.IsGraphic(r) || r == rune('\t') || r == rune('\n') { sb.WriteByte(line[i]) } } return sb.String() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/internal/common/icon_utils.go(1 hunks)src/internal/common/icon_utils_test.go(1 hunks)src/internal/common/string_function.go(12 hunks)src/internal/common/string_function_test.go(6 hunks)src/internal/function.go(4 hunks)src/internal/handle_file_operations.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/internal/common/icon_utils_test.go
- src/internal/common/icon_utils.go
- src/internal/handle_file_operations.go
- src/internal/common/string_function_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.312Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
🧬 Code Graph Analysis (1)
src/internal/function.go (2)
src/internal/utils/fzf_utils.go (1)
FzfSearch(6-12)src/internal/common/string_function.go (1)
FormatFileSize(108-125)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
src/internal/function.go (3)
19-19: Updated import path aligns with the refactoring objective.The addition of the utils package import is consistent with the PR's goal of consolidating sidebar-related functionality into a dedicated package structure.
182-182: LGTM: Replaced local function with utils package function.Good refactoring to use the centralized
utils.FzfSearchfunction rather than maintaining a local implementation. This change aligns with the code reorganization objective of the PR.
361-361: LGTM: Centralized formatting function usage.Good refactoring to use the common
FormatFileSizefunction instead of a local implementation, consistent with the approach applied elsewhere in the file.src/internal/common/string_function.go (5)
1-1: LGTM: Package declaration change aligns with refactoring goals.The package declaration has been changed from
internaltocommon, which is consistent with the PR's objective of reorganizing code.
18-18: LGTM: Function names changed to follow Go's exported function convention.All functions have been renamed to follow PascalCase convention, which is appropriate since they are now exported functions in the
commonpackage. This is consistent with Go's naming conventions and the PR's refactoring objectives.Also applies to: 27-27, 48-48, 60-60, 75-75, 83-83, 97-97, 108-108, 128-128, 145-145, 174-174, 194-194
61-61: Function signature updated to include Config.Nerdfont parameter.The
GetElementIconfunction now includes theConfig.Nerdfontparameter, ensuring the icon rendering is configured consistently.Also applies to: 76-76, 84-84
63-68: LGTM: Style references updated consistently.Style references like
StringColorRender,FilePanelItemSelectedStyle, andFilePanelStyleare now used directly without thecommonprefix, which is appropriate since the code is now in thecommonpackage.Also applies to: 69-73, 77-81, 86-90, 91-95
187-187: LGTM: Updated function call to match new name.The call to
isBufferPrintablehas been updated toIsBufferPrintableto match the new function name.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
a6b76cc to
a3d70ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/internal/function.go (1)
328-330: TODO added to address the performance concern with dirSize.Good addition of the TODO comment acknowledging the performance concern with recursive directory size calculation. This appropriately addresses the feedback from the previous review while keeping the scope of this PR focused on code reorganization.
🧹 Nitpick comments (1)
src/internal/common/icon_utils_test.go (1)
77-80: Add assertions for individual fields.Consider adding separate assertions for each field to provide more specific error messages when failures occur:
- if result.Icon != tt.expected.Icon || result.Color != tt.expected.Color { - t.Errorf("GetElementIcon() = %v, want %v", result, tt.expected) - } + if result.Icon != tt.expected.Icon { + t.Errorf("GetElementIcon() icon = %q, want %q", result.Icon, tt.expected.Icon) + } + if result.Color != tt.expected.Color { + t.Errorf("GetElementIcon() color = %q, want %q", result.Color, tt.expected.Color) + }This would make it clearer which aspect (icon or color) fails in case of test failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/internal/common/icon_utils.go(1 hunks)src/internal/common/icon_utils_test.go(1 hunks)src/internal/common/string_function.go(10 hunks)src/internal/common/string_function_test.go(6 hunks)src/internal/function.go(4 hunks)src/internal/handle_file_operations.go(2 hunks)src/internal/utils/fzf_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/internal/utils/fzf_utils.go
- src/internal/handle_file_operations.go
- src/internal/common/icon_utils.go
- src/internal/common/string_function_test.go
- src/internal/common/string_function.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.312Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
🧬 Code Graph Analysis (2)
src/internal/common/icon_utils_test.go (2)
src/config/icon/icon.go (3)
Style(4-7)Folders(397-417)Icons(50-165)src/internal/common/icon_utils.go (1)
GetElementIcon(10-63)
src/internal/function.go (2)
src/internal/utils/fzf_utils.go (1)
FzfSearch(6-14)src/internal/common/string_function.go (1)
FormatFileSize(108-125)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
src/internal/function.go (3)
19-19: Import path update aligns with the refactoring goal.The addition of the
utilspackage import is consistent with the PR's refactoring objective to consolidate utilities and update import paths.
182-182: Function call updated to use the centralized utility.The change from an internal
fzfSearchfunction toutils.FzfSearchappropriately moves the fuzzy search functionality to a dedicated utils package, improving code organization.
361-361: File size formatting call updated to use common package function.The change from a local formatting function to
common.FormatFileSizealigns with the refactoring goal of consolidating utility functions into appropriate packages.src/internal/common/icon_utils_test.go (3)
1-83: Well-structured test suite for the GetElementIcon function.Good use of table-driven tests with a variety of test cases covering different scenarios, including non-nerd font files, directories, known file extensions, and special files. This provides comprehensive coverage for the GetElementIcon function.
67-72: Confirm behavior for unknown file extensions.For files with unknown extensions, the test expects the function to use the default file icon with Theme.FilePanelFG color. The comment on line 69 suggests Theme might not be defined here, which could lead to inconsistent test results. Consider initializing a mock Theme for testing purposes if needed.
24-24:❓ Verification inconclusive
Verify Theme.FilePanelFG initialization for the tests.
The test uses
Theme.FilePanelFGbut it's not clear whereThemeis defined or initialized. This could cause the tests to fail ifThemeis not properly initialized before the tests run.Also applies to: 70-70
🏁 Script executed:
#!/bin/bash # Check where Theme is defined and initialized echo "Searching for Theme definition in the common package..." rg -A 3 "var Theme" --type go src/internal/common/Length of output: 353
Action: Confirm Theme Initialization for Test Reliability
It appears that the global variable
Themeis declared insrc/internal/common/default_config.go(asvar Theme ThemeType), but its initialization isn’t clearly observable from that file. Please verify thatTheme—and specificallyTheme.FilePanelFG—is properly initialized before running the tests insrc/internal/common/icon_utils_test.go(lines 24 and 70). If the package-level initialization (for example, via aninit()function or explicit test setup) isn’t adequately ensuring default values, consider adding an explicit initialization step in the test setup to avoid potential failures.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
a3d70ff to
91c1880
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/internal/common/string_function.go (1)
194-206:⚠️ Potential issuePotential UTF-8 handling issue in MakePrintable function.
While the function has been properly renamed to PascalCase, there's an issue with UTF-8 character handling:
for i := range len(line) { r := rune(line[i]) // ... }This loop iterates through byte indices, not rune indices, which will cause problems with multi-byte UTF-8 characters. Each byte of a multi-byte character will be treated as a separate rune.
Fix by iterating through runes properly:
- for i := range len(line) { - r := rune(line[i]) - if unicode.IsGraphic(r) || r == rune('\t') || r == rune('\n') { - sb.WriteByte(line[i]) - } - } + for i, r := range line { + if unicode.IsGraphic(r) || r == '\t' || r == '\n' { + if r < 128 { + // ASCII character + sb.WriteByte(byte(r)) + } else { + // Multi-byte UTF-8 character + sb.WriteRune(r) + } + } + }
♻️ Duplicate comments (2)
src/internal/function.go (1)
328-330: Good acknowledgment of performance concernThank you for adding the TODO comment acknowledging the performance concern with
dirSize(). Moving tocommon.FormatFileSizeis consistent with the refactoring objective.src/internal/common/string_function.go (1)
155-171: Fixed typo in function name.The function name has been corrected from
IsExensionExtractabletoIsExtensionExtractable, fixing the typo identified in previous reviews.
🧹 Nitpick comments (8)
src/internal/ui/sidebar/ReadMe.md (3)
1-3: Enhance Introduction Clarity and Detail
The introduction clearly states the sidebar package’s purpose. To further improve usability of the README, consider expanding on what specific aspects of the sidebar UI and directory management this package addresses, and possibly include usage examples or screenshots.
4-10: Improve To-dos Wording for Clarity and Consistency
The to-dos section is well-organized and gives a good overview of pending improvements. However, there are some grammatical tweaks suggested:
- Change “Separate out implementation of file I/O operations. (Disk listing, Reading and Updating pinned.json)” to something like:
“Separate out the implementation of file I/O operations (e.g., disk listing, reading, and updating of pinned.json) so that this package remains focused solely on UI/UX.”
This adjustment also addresses the static analysis note about a possible missing preposition and improves clarity.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Possible missing preposition found.
Context: ...es # To-dos - Add missing unit tests - Separate out implementation of file I/O operatio...(AI_HYDRA_LEO_MISSING_TO)
11-17: Expand the Coverage Section Information
The coverage section provides a clear command for running tests and shows the current coverage percentage. Consider adding additional context—such as goals for increasing coverage or a brief note on which areas need the most attention—to guide future improvements.src/internal/function.go (1)
196-204: Consider replacing with standard library functionAs indicated by your TODO comment, this
arrayContainsfunction could be replaced withslices.Containsfrom the standard library, which would improve maintainability and potentially performance.-// Todo : replace usage of this with slices.contains -func arrayContains(s []string, str string) bool { - for _, v := range s { - if v == str { - return true - } - } - return false -}Note: I see you're already using
slicespackage (line 13), so this should be a straightforward replacement when you're ready to address this TODO.src/internal/common/icon_utils_test.go (2)
67-71: Inconsistent comment on line 70The comment says "Theme is not defined here, so this will be blank", but the test uses
Theme.FilePanelFGwhich implies it should have a value. This comment seems contradictory to the actual code behavior.expected: icon.Style{ Icon: icon.Icons["file"].Icon, - // Theme is not defined here, so this will be blank + // Use the default theme color for files Color: Theme.FilePanelFG, },
10-73: Consider adding test cases for aliased extensionsThe current test suite doesn't cover the alias handling functionality present in the
GetElementIconimplementation (visible in the provided context snippet). Adding test cases for aliased extensions would improve coverage.{ name: "File with unknown extension", file: "test.xyz", isDir: false, nerdFont: true, expected: icon.Style{ Icon: icon.Icons["file"].Icon, // Use the default theme color for files Color: Theme.FilePanelFG, }, }, + { + name: "File with aliased extension", + file: "test.js", + isDir: false, + nerdFont: true, + expected: icon.Icons["js"], + }, + { + name: "File with aliased name", + file: "dockerfile", + isDir: false, + nerdFont: true, + expected: icon.Icons["dockerfile"], + },src/internal/common/string_function.go (2)
18-25: Good PascalCase renaming for exported functions.The string manipulation functions have been properly renamed to follow Go's PascalCase convention for exported functions, improving consistency across the codebase.
There is a TODO comment on line 54 about using
ansi.Substringthat could be addressed:- // Todo : Use ansi.Substring to correctly handle ANSI escape codes - truncatedText := text[:halfEllipsisLength] + tails + text[utf8.RuneCountInString(text)-halfEllipsisLength:] + truncatedText := ansi.Substring(text, 0, halfEllipsisLength) + tails + ansi.Substring(text, utf8.RuneCountInString(text)-halfEllipsisLength, utf8.RuneCountInString(text))Also applies to: 27-46, 48-58
97-125: String utility functions properly renamed.The file naming and size formatting functions have been renamed to follow PascalCase convention.
Consider addressing the TODO on line 116 about removing duplication in the file size formatting logic:
- // Todo : Remove duplication here - if Config.FileSizeUseSI { - unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(1000))) - adjustedSize := float64(size) / math.Pow(1000, float64(unitIndex)) - return fmt.Sprintf("%.2f %s", adjustedSize, unitsDec[unitIndex]) - } - unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(1024))) - adjustedSize := float64(size) / math.Pow(1024, float64(unitIndex)) - return fmt.Sprintf("%.2f %s", adjustedSize, unitsBin[unitIndex]) + base := 1024.0 + units := unitsBin + if Config.FileSizeUseSI { + base = 1000.0 + units = unitsDec + } + unitIndex := int(math.Floor(math.Log(float64(size)) / math.Log(base))) + adjustedSize := float64(size) / math.Pow(base, float64(unitIndex)) + return fmt.Sprintf("%.2f %s", adjustedSize, units[unitIndex])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/internal/common/icon_utils.go(1 hunks)src/internal/common/icon_utils_test.go(1 hunks)src/internal/common/string_function.go(10 hunks)src/internal/common/string_function_test.go(6 hunks)src/internal/function.go(4 hunks)src/internal/handle_file_operations.go(2 hunks)src/internal/ui/sidebar/ReadMe.md(1 hunks)src/internal/utils/fzf_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/internal/utils/fzf_utils.go
- src/internal/common/icon_utils.go
- src/internal/handle_file_operations.go
- src/internal/common/string_function_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.312Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
🧬 Code Graph Analysis (1)
src/internal/common/icon_utils_test.go (2)
src/config/icon/icon.go (3)
Style(4-7)Folders(397-417)Icons(50-165)src/internal/common/icon_utils.go (1)
GetElementIcon(10-63)
🪛 LanguageTool
src/internal/ui/sidebar/ReadMe.md
[uncategorized] ~6-~6: Possible missing preposition found.
Context: ...es # To-dos - Add missing unit tests - Separate out implementation of file I/O operatio...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (9)
src/internal/function.go (3)
19-19: Import path update looks goodThe import path change to use the new utility package location is consistent with the refactoring objective of consolidating functionality into dedicated packages.
182-182: Appropriate move of fuzzy search functionalityThe change from a local
fzfSearchfunction toutils.FzfSearchis a good refactoring step. This centralizes the fuzzy search functionality in the utilities package, making it more maintainable and reusable across the application.
361-361: Consistent use of formatting utilityThe change to use
common.FormatFileSizehere is consistent with the other similar change in the file and aligns with the refactoring objectives.src/internal/common/icon_utils_test.go (2)
9-83: Well-structured comprehensive test for GetElementIconThe test covers multiple scenarios including regular files, directories, and special cases like
.gitdirectories andpackage.jsonfiles. The table-driven test approach makes it easy to extend with additional test cases.
23-25:❓ Verification inconclusive
Verify that Theme is correctly initialized in the test environment
The test expects
Theme.FilePanelFGto be defined, but this variable isn't explicitly initialized in the test file. Ensure it's properly set up before the tests run.
🏁 Script executed:
#!/bin/bash # Check how Theme is initialized in the package grep -r "var Theme" --include="*.go" src/internal/common/ grep -r "Theme.*FilePanelFG" --include="*.go" src/internal/common/Length of output: 533
Ensure Test Environment Loads Proper Theme Initialization
It appears that the variable
Themeis declared insrc/internal/common/default_config.go(asvar Theme ThemeType), and the test file (src/internal/common/icon_utils_test.go) usesTheme.FilePanelFGwithout explicitly initializing it within the test itself. Please verify that the test environment correctly sets up or loads the default configuration so thatTheme.FilePanelFGhas the expected value when the tests run.
- Confirm that either an initialization (e.g., in an init function or a test setup) properly assigns a non-zero value to
Theme.- If the default configuration is meant to initialize
Theme, ensure that this mechanism is invoked before the tests execute.src/internal/common/string_function.go (4)
1-1: Package name changed to align with refactoring goals.This change from
internaltocommonaligns with the PR objectives of consolidating and reorganizing sidebar-related functionality.
60-73: Properly updated function calls to match new naming conventions.The UI-related functions have been updated to:
- Follow PascalCase naming convention
- Call
GetElementIconwith the correct parameters- Remove
common.prefix from references to other functions and variablesThese changes align with the refactoring goals and maintain consistency.
Also applies to: 75-81, 83-95
128-153: Line processing functions properly renamed.The string formatting and buffer processing functions have been renamed following Go's PascalCase convention for exported functions.
174-188: Function name updated with reference to other renamed function.The
IsTextFilefunction has been renamed and its reference toIsBufferPrintablehas been updated to match the new naming convention.
lazysegtree
force-pushed
the
lazysegtree_dev
branch
from
91c1880 to
488cf8d
Compare
|
Addressed most of the actionable nitpicks |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/internal/function.go (1)
328-330: Good addition of performance concern TODOThe added TODO addresses the previously identified performance concern about
dirSize()being expensive for large directories. Including specific optimization strategies (lazy loading, caching, async approach) provides clear direction for future implementation.src/internal/common/string_function.go (1)
155-171: Fixed typo in function name: IsExtensionExtractable.The typo in the function name has been corrected from
IsExensionExtractabletoIsExtensionExtractable, as previously requested in code review.
🧹 Nitpick comments (3)
src/internal/ui/sidebar/ReadMe.md (3)
1-2: Documentation Clarity and File Naming ConventionThe header clearly describes the sidebar package's purpose. Consider verifying that the file naming follows your project conventions—typically, a README file is named
README.md(all uppercase) rather thanReadMe.mdfor consistency.
4-10: To-do List ImprovementThe To-dos section is well-organized and provides a clear roadmap for future improvements. One minor suggestion for clarity: on the bullet point regarding file I/O operations, consider inserting the definite article to improve readability. For example, change:
- Separate out implementation of file I/O operations. (Disk listing, Reading and Updating pinned.json)to:
- Separate out implementation of file I/O operations. (Disk listing, Reading and Updating pinned.json) + Separate out the implementation of file I/O operations (disk listing, reading, and updating pinned.json).This revision addresses the static analysis hint regarding a possible missing preposition.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Possible missing preposition found.
Context: ...es # To-dos - Add missing unit tests - Separate out implementation of file I/O operatio...(AI_HYDRA_LEO_MISSING_TO)
11-17: Coverage Section and Testing ConsiderationsThe coverage section provides useful instructions for running tests. However, the noted 29.3% coverage is quite low. It would be beneficial to prioritize adding unit tests for the sidebar functionality. Please consider either updating the tests or outlining a plan to incrementally improve coverage in future iterations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/internal/common/icon_utils.go(1 hunks)src/internal/common/icon_utils_test.go(1 hunks)src/internal/common/string_function.go(10 hunks)src/internal/common/string_function_test.go(6 hunks)src/internal/function.go(4 hunks)src/internal/handle_file_operations.go(2 hunks)src/internal/ui/sidebar/ReadMe.md(1 hunks)src/internal/utils/fzf_utils.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/internal/utils/fzf_utils.go
- src/internal/common/icon_utils_test.go
- src/internal/common/icon_utils.go
- src/internal/common/string_function_test.go
- src/internal/handle_file_operations.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.312Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
🧬 Code Graph Analysis (1)
src/internal/function.go (2)
src/internal/utils/fzf_utils.go (1)
FzfSearch(6-14)src/internal/common/string_function.go (1)
FormatFileSize(108-125)
🪛 LanguageTool
src/internal/ui/sidebar/ReadMe.md
[uncategorized] ~6-~6: Possible missing preposition found.
Context: ...es # To-dos - Add missing unit tests - Separate out implementation of file I/O operatio...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (16)
src/internal/function.go (3)
19-19: Import addition follows the refactoring patternThe addition of the utils import aligns with the PR objective of consolidating utility functions into dedicated packages and updating import paths throughout the codebase.
182-182: Function call updated to use centralized utilityThe change from using a local
fzfSearchfunction to theutils.FzfSearchutility function follows the refactoring goal of consolidating functionality into dedicated packages. The function signature remains consistent with the original implementation.
361-361: Function call updated to use standardized utilityThe change from using a local
formatFileSizefunction to thecommon.FormatFileSizeutility function follows the refactoring pattern of consolidating functionality and standardizing the codebase.src/internal/common/string_function.go (13)
1-1: Package declaration updated appropriately.The package declaration has been changed to
common, which aligns with the overall refactoring effort of consolidating sidebar-related functionality into dedicated packages.
18-25: Function renamed to PascalCase for improved visibility.The function
truncateTexthas been renamed toTruncateTextfollowing Go's standard convention of using PascalCase for exported functions. This is consistent with the PR's goal of standardizing function names.
27-46: Function renamed to PascalCase with improved tails logic.The function
truncateTextBeginninghas been renamed toTruncateTextBeginning. The logic for adding tails at lines 41-42 has been improved by using a more direct approach with rune slices.
48-58: Function renamed to PascalCase with TODO comment.The function
truncateMiddleTexthas been renamed toTruncateMiddleText. A TODO comment has been added at line 54 to suggest usingansi.Substringfor better handling of ANSI escape codes in the future.As a follow-up to this TODO, please consider implementing the
ansi.Substringapproach in a future PR to correctly handle ANSI escape codes in the truncated text.
60-73: Function renamed and now uses GetElementIcon from icon_utils.go.The function
prettierNamehas been renamed toPrettierNameand now uses theGetElementIconfunction withConfig.Nerdfontparameter. This change centralizes icon selection logic and improves maintainability.
75-81: Function renamed and updated to use GetElementIcon.Similar to
PrettierName, this function has been updated to use the centralizedGetElementIconfunction, improving consistency across the codebase.
83-95: Function renamed and updated for consistent styling.The
clipboardPrettierNamefunction has been renamed toClipboardPrettierNameand updated to useGetElementIcon. The styling has been made consistent with the other similar functions.
97-106: Function renamed to PascalCase for export.The function
fileNameWithoutExtensionhas been renamed toFileNameWithoutExtension, following the PascalCase convention for exported functions.
108-125: Function renamed with direct Config access.The function
formatFileSizehas been renamed toFormatFileSize. Now directly accessesConfig.FileSizeUseSIwithout thecommon.prefix, reflecting the package change.
127-142: Function renamed to PascalCase for export.The function
checkAndTruncateLineLengthshas been renamed toCheckAndTruncateLineLengthsto follow Go's PascalCase convention for exported functions.
144-153: Function renamed to PascalCase for export.The function
isBufferPrintablehas been renamed toIsBufferPrintable, maintaining consistent naming conventions across the codebase.
173-188: Function renamed and simplified function call.The function
isTextFilehas been renamed toIsTextFile. At line 187, the call toIsBufferPrintableno longer needs thecommon.prefix since both functions are in the same package now.
194-206: Function renamed to PascalCase for export.The function
makePrintablehas been renamed toMakePrintableto follow the PascalCase convention for exported functions.
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [yorukot/superfile](https://github.com/yorukot/superfile) | minor | `v1.2.1` -> `v1.3.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.3.0`](https://github.com/yorukot/superfile/releases/tag/v1.3.0) [Compare Source](yorukot/superfile@v1.2.1...v1.3.0) We are excited to release v1.3.0 for superfile, with new features, key bug fixes, and lots of polish under the hood. #### Install: [**Click me to know how to install**](https://github.com/yorukot/superfile?tab=readme-ov-file#installation) #### Highlights - We have revamped the command prompt and added built-in commands for SuperFile-specific actions. Check out yorukot/superfile#745 - Multi-panel startup support. You can now open multiple panels right from startup, making your workflow even more efficient. - Added new configurations : --chooser-file option, show_panel_footer_info config flag and many command prompt specific flags. #### Improvements & Fixes - The sidebar code was refactored and separated for better maintainability and various linter fixes and CI/CD improvements were made to keep the codebase clean and robust. - A new Rendering package is implemented, centralising border, content, and section rendering logic into reusable renderer components, fixing many layout bugs. - Model behaviour, file operations and rendering-related unit tests were added to improve test coverage. #### Detailed Change Summary <details><summary>Details</summary> <p> #### New Features - Added a Command-Prompt for SuperFile specific actions [`#752`](yorukot/superfile#752) by [@​Rocco-Gossmann](https://github.com/Rocco-Gossmann), [@​yorukot](https://github.com/yorukot) and [@​lazysegtree](https://github.com/lazysegtree) - Allow specifying multiple panels at startup [`#759`](yorukot/superfile#759) by [@​lazysegtree](https://github.com/lazysegtree) - Initial draft of rendering package [`#775`](yorukot/superfile#775) by [@​lazysegtree](https://github.com/lazysegtree) - Render unit tests for prompt model [`#809`](yorukot/superfile#809) by [@​lazysegtree](https://github.com/lazysegtree) - Chooser file option, --lastdir-file option, and improvements in quit, and bug fixes [`#812`](yorukot/superfile#812) by [@​lazysegtree](https://github.com/lazysegtree) - Prompt feature leftover items [`#804`](yorukot/superfile#804) by [@​lazysegtree](https://github.com/lazysegtree) - SPF Prompt tutorial and fixes [`#814`](yorukot/superfile#814) by [@​lazysegtree](https://github.com/lazysegtree) - Write prompt tutorial, rename prompt mode to spf mode, add develop branch in GitHub workflow, show_panel_footer_info flag [`#815`](yorukot/superfile#815) by [@​lazysegtree](https://github.com/lazysegtree) - Theme: Add gruvbox-dark-hard [`#828`](yorukot/superfile#828) by [@​Frost-Phoenix](https://github.com/Frost-Phoenix) #### Updates & Improvements - Sidebar separation [`#767`](yorukot/superfile#767) by [@​lazysegtree](https://github.com/lazysegtree) - Sidebar code separation [`#770`](yorukot/superfile#770) by [@​lazysegtree](https://github.com/lazysegtree) - Rendering package and rendering bug fixes [`#781`](yorukot/superfile#781) by [@​lazysegtree](https://github.com/lazysegtree) - Refactor CheckForUpdates [`#797`](yorukot/superfile#797) by [@​JassonCordones](https://github.com/JassonCordones) - Rename metadata strings [`#731`](yorukot/superfile#731) by [@​booth-w](https://github.com/booth-w) #### Bug Fixes - Fix crash with opening file with editor on an empty panel [`#730`](yorukot/superfile#730) by [@​booth-w](https://github.com/booth-w) - Fix: Add some of the remaining linter and fix errors [`#756`](yorukot/superfile#756) by [@​lazysegtree](https://github.com/lazysegtree) - Golangci lint fixes [`#757`](yorukot/superfile#757) by [@​lazysegtree](https://github.com/lazysegtree) - Fix: Remove redundant function containsKey [`#765`](yorukot/superfile#765) by [@​lazysegtree](https://github.com/lazysegtree) - Fix: Correctly resolve path in open and cd prompt actions [`#802`](yorukot/superfile#802) by [@​lazysegtree](https://github.com/lazysegtree) - Prompt dynamic dimensions and unit tests fix [`#805`](yorukot/superfile#805) by [@​lazysegtree](https://github.com/lazysegtree) - Fix: Convert unicode space to normal space, use rendered in file preview to fix layout bugs, Release 1.3.0 [`#825`](yorukot/superfile#825) by [@​lazysegtree](https://github.com/lazysegtree) #### Optimization & Code Quality - Adding linter to CI/CD and fix some lint issues [`#739`](yorukot/superfile#739) by [@​lazysegtree](https://github.com/lazysegtree) - Linter fixes, new feature of allowing multiple directories at startup, other code improvements [`#764`](yorukot/superfile#764) by [@​lazysegtree](https://github.com/lazysegtree) - Model unit tests [`#803`](yorukot/superfile#803) by [@​lazysegtree](https://github.com/lazysegtree) #### Dependency Updates - fix(deps): update dependency astro to v5.7.7 [`#726`](yorukot/superfile#726) by [@​renovate](https://github.com/renovate) - fix(deps): update module github.com/shirou/gopsutil/v4 to v4.25.3 [`#749`](yorukot/superfile#749) by [@​renovate](https://github.com/renovate) - fix(deps): update module github.com/pelletier/go-toml/v2 to v2.2.4 [`#760`](yorukot/superfile#760) by [@​renovate](https://github.com/renovate) - fix(deps): update module github.com/alecthomas/chroma/v2 to v2.16.0 [`#751`](yorukot/superfile#751) by [@​renovate](https://github.com/renovate) - fix(deps): update dependency sharp to ^0.34.0 [`#755`](yorukot/superfile#755) by [@​renovate](https://github.com/renovate) - fix(deps): update dependency [@​astrojs/starlight](https://github.com/astrojs/starlight) to ^0.34.0 [`#761`](yorukot/superfile#761) by [@​renovate](https://github.com/renovate) </p> </details> #### New Contributors * @​Rocco-Gossmann made their first contribution in yorukot/superfile#736 * @​Frost-Phoenix made their first contribution in yorukot/superfile#828 **Full Changelog**: yorukot/superfile@v1.2.1...v1.3.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:eyJjcmVhdGVkSW5WZXIiOiI0MC4yMi4wIiwidXBkYXRlZEluVmVyIjoiNDAuMjMuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
| "time" | ||
|
|
||
| "github.com/yorukot/superfile/src/internal/common" | ||
| "github.com/yorukot/superfile/src/internal/common/utils" |
There was a problem hiding this comment.
@coderabbitai
Please generate a task description for this PR. I will give someone this task. He doesn't know anything about this product. we have a sidebar with implementation scattered across.
move stuff to sidebar package.
Make side functions exported so that they are accessible outside
Move src/internal/common/utils to a separate src/internal/utils package
Ensure nothing breaks
Organize code into correct files.
Any other refactoring that I missed ?
Add Anything else that I missed, which is done in this PR, and should be in the ask description.
Keep it in 6-7 points or less. Format should be markdown. Make sure to highlight what needs to be done, not how or where.
Give basic context about Superfile and what the requirements are.The problem description should be stated such that a reasonably competent software engineer could write the correct PR for it without too much familiarity with the repo. Write it in a way that a LLM could generate a correct PR for it.
Also, generate a TL;DR
There was a problem hiding this comment.
Task Description: Sidebar Code Consolidation and Refactoring
TL;DR
Consolidate scattered sidebar implementation into a dedicated sidebar package, export necessary functions, reorganize utilities, and ensure backward compatibility throughout the codebase.
Context
Superfile is a modern terminal file manager written in Go. It features a sidebar UI component that displays pinned directories, well-known system directories (like Documents, Downloads), and external media/disk partitions. Users can navigate, search, and rename items within the sidebar.
Problem
The sidebar implementation is currently scattered across multiple files in the main internal package. Functions are not properly encapsulated, utilities are mixed with other code, and the architecture makes maintenance and testing difficult.
Requirements
-
Create a dedicated sidebar package at
src/internal/ui/sidebarand consolidate all sidebar-related code (state management, rendering, navigation, directory operations) into this package. -
Export functions that need external access by converting them from lowercase to PascalCase naming convention (e.g.,
listUp→ListUp,sidebarModel→Model). -
Reorganize utility functions by moving
src/internal/common/utilstosrc/internal/utils, making it a standalone package independent of the common package. -
Update all import paths and references throughout the entire codebase to reflect the new package structure without breaking existing functionality.
-
Organize code logically within the sidebar package by separating concerns into appropriate files (e.g., rendering logic, navigation, directory operations, disk utilities, type definitions).
-
Maintain backward compatibility by ensuring all existing features (sidebar navigation, search, rename, pinned directories) continue to work exactly as before.
This refactoring improves code organization, testability, and maintainability without introducing new features or changing user-facing behavior.
🧠 Learnings used
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
PR: yorukot/superfile#1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#0
File: :0-0
Timestamp: 2025-04-12T12:25:06.352Z
Learning: PR #770 "Sidebar code separation" is a refactoring and code reorganization effort with no new features. It involves moving code, updating import paths, renaming functions to follow PascalCase convention, and consolidating sidebar-related functionality into a dedicated package.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
PR: yorukot/superfile#969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
PR: yorukot/superfile#985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
PR: yorukot/superfile#973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
PR: yorukot/superfile#1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
PR: yorukot/superfile#950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
PR: yorukot/superfile#967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
PR: yorukot/superfile#963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
Learnt from: lazysegtree
PR: yorukot/superfile#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.
PRs
Sidebar separation #767
Sidebar code separation, and more unit tests.
Summary by CodeRabbit
New Features
Documentation