feat: add bulk rename #1113
feat: add bulk rename #1113
Conversation
… into feature_branch
chore: refactor code
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a bulk-rename feature: UI model, renderer, navigation and tests; editor-driven workflow with tmpfile handling and process-bar execution; hotkey bindings and config fields; key/modal routing refactor and integration into main app flow. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant A as App (superfile)
participant M as BulkRename Modal
participant E as External Editor
participant FS as File System
U->>A: Press bulk_rename hotkey (B)
A->>M: Open modal with selected files
activate M
M->>U: Show options & preview
alt Editor mode
M->>E: Write tmpfile & spawn editor (os/exec)
activate E
E-->>M: Editor closed (tmpfile)
deactivate E
M->>M: Parse tmpfile -> generate RenamePreview
end
U->>M: Confirm
M->>A: Emit BulkRenameAction (Previews)
deactivate M
A->>FS: Execute renames (apply previews) via processbar
FS-->>A: Return results/state
A->>U: Show progress/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/internal/bulk_rename.go(1 hunks)src/internal/handle_file_operations.go(1 hunks)src/internal/key_function.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/internal/handle_file_operations.go
🧰 Additional context used
🧬 Code graph analysis (2)
src/internal/key_function.go (2)
src/internal/common/default_config.go (1)
Hotkeys(12-12)src/internal/model.go (2)
m(420-450)m(333-416)
src/internal/bulk_rename.go (2)
src/internal/common/style_function.go (1)
GenerateBulkRenameTextInput(330-343)src/internal/model_render.go (1)
panel(111-154)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/internal/model_render.go (1)
557-576: Show old → new in preview rowsLine 563 only renders
p.newName, so users can’t verify which original file will map to that name. Please render the full mapping (old → new) and keep the error styling on the combined line whenp.erroris set, per earlier feedback.func (m *model) renderPreviewItem(p renamePreview, width int) string { - centeredLine := lipgloss.NewStyle(). + text := fmt.Sprintf("%s → %s", p.oldName, p.newName) + centeredLine := lipgloss.NewStyle(). Width(width - 4). Align(lipgloss.Center). Background(common.ModalBGColor). Foreground(common.ModalFGColor). - Render(p.newName) + Render(text) if p.error == "" { return centeredLine + "\n" } errorStyle := lipgloss.NewStyle(). Width(width - 4). Align(lipgloss.Center). Background(common.ModalBGColor). Foreground(lipgloss.Color(common.Theme.Error)) - return errorStyle.Render(p.newName) + "\n" + errorStyle.Render(fmt.Sprintf("(%s)", p.error)) + "\n" + return errorStyle.Render(text) + "\n" + errorStyle.Render(fmt.Sprintf("(%s)", p.error)) + "\n" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/internal/key_function.go(2 hunks)src/internal/model.go(4 hunks)src/internal/model_render.go(1 hunks)src/internal/type.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/internal/model.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
PR: yorukot/superfile#1039
File: src/internal/ui/zoxide/model.go:53-54
Timestamp: 2025-09-06T13:42:44.590Z
Learning: The zoxide modal in src/internal/ui/zoxide/model.go is missing handling for common.Hotkeys.Quit when zClient is nil (lines 52-54), only handling ConfirmTyping and CancelTyping. This creates inconsistency with other modals like sort options menu, help menu, and notify model which all properly handle the Quit hotkey. The prompt modal has the same inconsistency.
Applied to files:
src/internal/type.go
🧬 Code graph analysis (3)
src/internal/model_render.go (5)
src/internal/common/predefined_variable.go (1)
ModalHeight(31-31)src/internal/common/style_function.go (1)
ModalBorderStyle(124-126)src/internal/common/style.go (5)
ModalBGColor(81-81)SidebarTitleStyle(25-25)ModalStyle(20-20)ModalFGColor(87-87)ModalErrorStyle(53-53)src/internal/common/default_config.go (1)
Theme(10-10)src/config/icon/icon.go (2)
Cursor(31-31)Error(36-36)
src/internal/key_function.go (1)
src/internal/common/default_config.go (1)
Hotkeys(12-12)
src/internal/type.go (3)
src/internal/ui/zoxide/type.go (1)
Model(9-30)src/internal/ui/notify/model.go (1)
Model(7-12)src/internal/ui/prompt/type.go (1)
Model(6-30)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
M1 - Need to check if the refactoring broke anything
| common.Config.CdOnQuit = true | ||
| return nil | ||
| default: | ||
| // Handles general kinds of inputs in the regular state of the application |
There was a problem hiding this comment.
M2 - Please don't remove explanation comments. Lots of comments including this one, are removed from this file.
src/internal/type.go
Outdated
| type keyInputContext struct { | ||
| msg string | ||
| } // Copied items |
There was a problem hiding this comment.
M1 - Review the usage of this
There was a problem hiding this comment.
M3 - Please update the getHelpMenuData() function in this file for the hotkey 'B' of Bulk Rename
| m.findInput = common.GenerateBulkRenameTextInput("Find text") | ||
| m.replaceInput = common.GenerateBulkRenameTextInput("Replace with") | ||
| m.prefixInput = common.GenerateBulkRenameTextInput("Add prefix") | ||
| m.suffixInput = common.GenerateBulkRenameTextInput("Add suffix") |
There was a problem hiding this comment.
M1 - Maybe better to generate them once, not fot every Open(). This is minor.
| editor := common.Config.Editor | ||
| if editor == "" { | ||
| editor = os.Getenv("EDITOR") | ||
| } | ||
| if editor == "" { | ||
| if runtime.GOOS == utils.OsWindows { | ||
| editor = "notepad" | ||
| } else { | ||
| editor = "nano" | ||
| } | ||
| } |
There was a problem hiding this comment.
M1 - This is duplicated with existing logic. Extract in a common function.
| return converted + ext | ||
| } | ||
|
|
||
| func toTitleCase(text string) string { |
There was a problem hiding this comment.
M2 - https://github.com/yorukot/superfile/pull/1113/files#r2447881280 This comment is valid.
The functions wont work accurately for multi space strings.
| func (m *Model) validateRenameWithoutStat(oldName, newName string) string { | ||
| if newName == "" { | ||
| return "Empty filename" | ||
| } | ||
| if newName == oldName { | ||
| return "No change" | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
M1 - Fix the usage of raw string as a state variable. Use bool or int
There was a problem hiding this comment.
M1 - Review the implementation in more detail
There was a problem hiding this comment.
M1 - Add readme.md - clarifying that it has been vibe coded and not might contain unexpected things. Please flag if you see any issues.
|
@harryfrzz Please check M2,M3 comments. You can ignore M1 if they are too hard or not clear. You may skip a few of M2 comment too, but please fix most of them. Please fix all M3 comments. I will then fix everything, do a thorough review and merge this. |
|
M3 - https://github.com/yorukot/superfile/actions/runs/18993554351/job/54317247938?pr=1113 Fix the gofmt failures 
Also run
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/internal/common/style_function.go (1)
330-343: Fix hardcoded text input width - parameterize like GenerateRenameTextInput.The width is still hardcoded to
ModalWidth - 10(50 chars), but per previous review comments this doesn't match the actual column width needed by the bulk rename modal renderer. This causes cursor visibility issues when typing.Apply this diff to parameterize the width:
-func GenerateBulkRenameTextInput(placeholder string) textinput.Model { +func GenerateBulkRenameTextInput(placeholder string, width int) textinput.Model { ti := textinput.New() ti.Cursor.Style = ModalStyle ti.Cursor.TextStyle = ModalStyle ti.PromptStyle = ModalStyle ti.Prompt = "" ti.TextStyle = ModalStyle ti.Cursor.Blink = true ti.Placeholder = placeholder ti.PlaceholderStyle = ModalStyle ti.CharLimit = 156 - ti.Width = ModalWidth - 10 + ti.Width = width return ti }Then update all call sites in
src/internal/ui/bulk_rename/model.goto pass the actual column width (54).src/internal/model.go (7)
378-415: Restore explanation comment removed during refactor.The refactored modal routing logic improves code organization, but a past review comment (M2) noted that explanation comments were removed from this file during the refactor. Line 400 specifically had context removed.
Please restore any removed explanation comments that clarify the modal routing logic, especially around the early return pattern in
routeModalInputand the state checking inisAnyModalActive.
554-566: Fix editor command parsing - strings.Fields breaks on spaces and quotes.The current use of
strings.Fieldsto parse the editor command will fail when the editor path contains spaces (e.g., Windows "Program Files") or when the editor command includes quoted arguments.Apply this fix to use proper shell-style tokenization:
+import "github.com/google/shlex" + func (m *model) handleEditorModeAction(action bulkrename.EditorModeAction) tea.Cmd { m.pendingEditorAction = &action - parts := strings.Fields(action.Editor) - cmd := parts[0] - args := append(parts[1:], action.TmpfilePath) + parts, err := shlex.Split(action.Editor) + if err != nil || len(parts) == 0 { + slog.Error("Failed to parse editor command", "editor", action.Editor, "error", err) + m.notifyModel = notify.New(true, "Bulk Rename Error", + "Invalid editor command: "+action.Editor, notify.NoAction) + return nil + } + args := append(parts[1:], action.TmpfilePath) - c := exec.Command(cmd, args...) + c := exec.Command(parts[0], args...) return tea.ExecProcess(c, func(err error) tea.Msg { return editorFinishedForBulkRenameMsg{err} }) }Add
"github.com/google/shlex"to the imports at the top of the file.
568-607: Surface bulk rename errors to user - critical silent failures.Three error conditions silently fail without user notification:
- Line 570: No pending editor action
- Line 581: Editor finished with error
- Lines 590-593: Line count mismatch
These should display error modals so users understand why the operation failed.
Apply these fixes:
if m.pendingEditorAction == nil { slog.Error("No pending editor action found") + m.notifyModel = notify.New(true, "Bulk Rename Error", + "Unexpected state: no editor action pending.", notify.NoAction) return nil } // ... defer cleanup ... if err != nil { slog.Error("Editor finished with error", "error", err) + m.notifyModel = notify.New(true, "Bulk Rename Error", + "Editor exited with error: "+err.Error(), notify.NoAction) return nil } // ... read lines ... if len(lines) != len(action.SelectedFiles) { slog.Error("Number of lines in tmpfile doesn't match number of selected files", "expected", len(action.SelectedFiles), "got", len(lines)) + m.notifyModel = notify.New(true, "Bulk Rename Error", + "Line count mismatch: expected "+strconv.Itoa(len(action.SelectedFiles))+ + " names, got "+strconv.Itoa(len(lines))+".", notify.NoAction) return nil }Related to issue #1152 tracking silent failures across file operations.
609-616: Don't use TrimSpace on filenames - preserves intentional spaces incorrectly.The current implementation uses
strings.TrimSpacewhich removes intentional leading/trailing spaces from filenames. This mangles valid filenames like" file.txt "or"test ".Apply this fix to preserve exact filenames while handling CRLF:
+import ( + "bufio" + "bytes" +) + func (m *model) readEditorOutputLines(tmpfilePath string) ([]string, error) { content, err := os.ReadFile(tmpfilePath) if err != nil { slog.Error("Failed to read tmpfile", "error", err) return nil, err } - return strings.Split(strings.TrimSpace(string(content)), "\n"), nil + + sc := bufio.NewScanner(bytes.NewReader(content)) + var lines []string + for sc.Scan() { + line := sc.Text() + // Only strip CRLF, preserve other whitespace + if strings.HasSuffix(line, "\r") { + line = strings.TrimSuffix(line, "\r") + } + lines = append(lines, line) + } + if err := sc.Err(); err != nil { + slog.Error("Failed to scan tmpfile", "error", err) + return nil, err + } + return lines, nil }
618-655: Add missing validations and remove TrimSpace.Two issues in the rename preview generation:
Line 622 uses
strings.TrimSpacewhich has the same problem asreadEditorOutputLines- it removes intentional spaces from filenames.Missing critical validations:
- Path separators (prevents renaming files into subdirectories)
- Duplicate target names within the selection
Apply this fix:
func (m *model) buildRenamePreviews(selectedFiles []string, newNames []string) []bulkrename.RenamePreview { previews := make([]bulkrename.RenamePreview, 0, len(newNames)) + seenTargets := make(map[string]struct{}, len(newNames)) + for i, itemPath := range selectedFiles { oldName := filepath.Base(itemPath) - newName := strings.TrimSpace(newNames[i]) + newName := newNames[i] if newName == "" { slog.Warn("Empty filename in tmpfile, skipping", "line", i+1) continue } preview := bulkrename.RenamePreview{ OldPath: itemPath, OldName: oldName, NewName: newName, } - preview.Error = m.validateRename(itemPath, oldName, newName) + preview.Error = m.validateRename(itemPath, oldName, newName, seenTargets) previews = append(previews, preview) } return previews } -func (m *model) validateRename(itemPath, oldName, newName string) string { +func (m *model) validateRename(itemPath, oldName, newName string, seenTargets map[string]struct{}) string { + // Check for path separators to prevent moving files + if strings.ContainsAny(newName, string(os.PathSeparator)+"/\\") { + return "Invalid name: contains path separator" + } + if newName == oldName { return "No change" } + + // Check for duplicates within selection (case-insensitive) + key := strings.ToLower(newName) + if _, dup := seenTargets[key]; dup { + return "Duplicate target in selection" + } + seenTargets[key] = struct{}{} newPath := filepath.Join(filepath.Dir(itemPath), newName) if strings.EqualFold(itemPath, newPath) { return "" } if _, err := os.Stat(newPath); err == nil { return "File already exists" } return "" }
811-811: Fix sort options overlay centering typo - /29 should be /2.Line 811 uses
/29instead of/2for x-axis centering, which incorrectly positions the sort options modal far off-center. This was flagged as M3 (critical) in previous reviews.Apply this fix:
- overlayX := m.fullWidth/2 - panel.sortOptions.width/29 + overlayX := m.fullWidth/2 - panel.sortOptions.width/2
378-415: Add Quit hotkey handling to bulk rename modal.The bulk rename modal's
HandleUpdatemethod does not handlecommon.Hotkeys.Quit, leaving users unable to quit the application while the modal is open. WhenbulkRenameModel.IsOpen()is true inrouteModalInput, execution returns early and never reacheshandleDefaultKey, which handles the Quit hotkey.Add a case in the switch statement at
src/internal/ui/bulk_rename/model.go(HandleUpdate method, around line 146) to handlecommon.Hotkeys.Quit:case slices.Contains(common.Hotkeys.Quit, msg.String()): m.Close() return common.QuitAction{}, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/internal/common/style_function.go(1 hunks)src/internal/model.go(8 hunks)src/internal/type.go(5 hunks)src/internal/ui/bulk_rename/model.go(1 hunks)src/internal/ui/bulk_rename/model_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/internal/ui/bulk_rename/model_test.go
- src/internal/ui/bulk_rename/model.go
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1113
File: src/superfile_config/vimHotkeys.toml:66-67
Timestamp: 2025-11-03T01:53:26.627Z
Learning: In yorukot/superfile, modal-specific hotkeys (like nav_bulk_rename and rev_nav_bulk_rename using tab/shift+tab in the bulk rename modal) can safely conflict with global hotkey bindings (like next_file_panel and previous_file_panel) because the modal captures input when focused, and the global bindings remain active when the modal is closed. This context-specific key handling is intentional and not considered a conflict.
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1039
File: src/internal/ui/zoxide/model.go:53-54
Timestamp: 2025-09-06T13:42:44.590Z
Learning: The zoxide modal in src/internal/ui/zoxide/model.go is missing handling for common.Hotkeys.Quit when zClient is nil (lines 52-54), only handling ConfirmTyping and CancelTyping. This creates inconsistency with other modals like sort options menu, help menu, and notify model which all properly handle the Quit hotkey. The prompt modal has the same inconsistency.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-07-27T15:32:06.922Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:32:06.922Z
Learning: When reviewing large refactoring PRs that change async patterns (like moving from goroutines to tea.Cmd), always check for incomplete refactoring where some call sites still use the old pattern while others use the new pattern, as this often leads to compilation errors and architectural inconsistencies.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-08-03T09:34:55.721Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: In yorukot/superfile, lazysegtree prefers that individual components like modals should implement interfaces directly and handle their own cancel/confirm logic, rather than using external handler objects. This follows the existing pattern where components like sidebarModel are self-contained and don't depend on the main model object.
Applied to files:
src/internal/type.go
📚 Learning: 2025-04-28T03:48:46.327Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-07-27T08:49:09.687Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-08-02T11:47:07.713Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-07-29T03:42:36.581Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 965
File: src/internal/config_function.go:8-8
Timestamp: 2025-07-29T03:42:36.581Z
Learning: When reviewing large refactoring PRs that introduce new async patterns (like moving from global channels to dedicated processbar channels), always check for incomplete refactoring where critical functionality like navigation depends on data structures that are no longer being populated, as this often leads to completely broken features.
Applied to files:
src/internal/type.go
📚 Learning: 2025-09-20T01:40:50.076Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1081
File: src/internal/ui/sidebar/directory_utils.go:103-112
Timestamp: 2025-09-20T01:40:50.076Z
Learning: lazysegtree identified code duplication between removeNotExistingDirectories and TogglePinnedDirectory functions in src/internal/ui/sidebar/directory_utils.go, specifically 6 lines of JSON marshaling and file writing logic. He prefers to track such duplication fixes in separate GitHub issues and suggests either extracting common util functions or creating a PinnedDir manager for centralized Read/Write operations to PinnedFile.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-04-12T12:00:32.688Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Applied to files:
src/internal/type.go
📚 Learning: 2025-08-24T03:24:50.857Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:18-21
Timestamp: 2025-08-24T03:24:50.857Z
Learning: The superfile project uses github.com/charmbracelet/x/ansi as the standardized ANSI package and deprecates github.com/charmbracelet/x/exp/term/ansi to avoid binary bloat and potential incompatibilities.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-09-04T07:24:30.872Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1041
File: src/internal/default_config.go:38-38
Timestamp: 2025-09-04T07:24:30.872Z
Learning: In the superfile codebase, the main model struct has a zClient field for zoxide directory tracking, and the trackDirectoryWithZoxide function in model.go checks if m.zClient is nil before proceeding. When reviewing model initialization functions like defaultModelConfig, ensure all struct fields are properly assigned, especially external service clients like zClient that enable core functionality.
Applied to files:
src/internal/type.gosrc/internal/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-09T14:23:14.164Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/ui/rendering/truncate.go:6-6
Timestamp: 2025-09-09T14:23:14.164Z
Learning: The truncate test failure in src/internal/ui/rendering/truncate_test.go during the ANSI package migration from experimental to stable was caused by a truncation bug in the experimental package. The experimental package incorrectly truncated strings even when input length equaled maxWidth (e.g., "1234" with maxWidth=4 became "1..."), while the stable package correctly only truncates when input exceeds maxWidth. The test expectation was based on the buggy behavior and needs to be corrected.
Applied to files:
src/internal/model.gosrc/internal/common/style_function.go
📚 Learning: 2025-08-24T03:25:10.117Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Applied to files:
src/internal/model.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-12T05:16:10.488Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/handle_modal.go:253-253
Timestamp: 2025-09-12T05:16:10.488Z
Learning: lazysegtree identified that the fuzzy search function in src/internal/handle_modal.go for the help menu only searches on item.description but should also include item.key in the search haystack to provide comprehensive search functionality, as users expect to find hotkeys by searching for their key names.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-06T12:03:51.042Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1039
File: src/internal/common/config_type.go:156-156
Timestamp: 2025-09-06T12:03:51.042Z
Learning: lazysegtree identified that the in-app help menu in getHelpMenuData function in src/internal/default_config.go is missing many hotkeys defined in HotkeysType, including the new OpenZoxide hotkey, causing inconsistent user experience and reduced feature discoverability.
Applied to files:
src/internal/model.go
📚 Learning: 2025-11-03T01:53:26.627Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1113
File: src/superfile_config/vimHotkeys.toml:66-67
Timestamp: 2025-11-03T01:53:26.627Z
Learning: In yorukot/superfile, modal-specific hotkeys (like nav_bulk_rename and rev_nav_bulk_rename using tab/shift+tab in the bulk rename modal) can safely conflict with global hotkey bindings (like next_file_panel and previous_file_panel) because the modal captures input when focused, and the global bindings remain active when the modal is closed. This context-specific key handling is intentional and not considered a conflict.
Applied to files:
src/internal/model.go
📚 Learning: 2025-04-28T04:27:33.074Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/model_render.go:254-256
Timestamp: 2025-04-28T04:27:33.074Z
Learning: When race conditions or other issues that are unrelated to the current PR's focus are identified during review, lazysegtree prefers creating separate GitHub issues to track them rather than addressing them in the current PR. This helps maintain PR focus and scope.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-29T14:11:21.380Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-06T10:54:31.444Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When lazysegtree says a review is "inaccurate and pre-existing issues. Not much useful," it means I should focus specifically on bugs or problems introduced by the current PR changes, not architectural concerns or code quality issues that were already present. The refactoring work in superfile PRs is generally well-implemented and should be evaluated on whether the specific changes work correctly, not on broader codebase architecture.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-09T13:29:11.771Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-06T10:54:31.444Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When reviewing PRs, especially large refactoring ones, focus specifically on issues introduced by the changes rather than flagging pre-existing architectural problems or code smells that were already present before the PR. Distinguish between new bugs versus existing technical debt.
Applied to files:
src/internal/model.go
📚 Learning: 2025-07-24T03:46:29.516Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-05T11:51:37.645Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:44-44
Timestamp: 2025-08-05T11:51:37.645Z
Learning: In Bubble Tea applications, when functions like warnModalForRenaming() capture state (like m.ioReqCnt) and increment it before returning a tea.Cmd, the increment happens synchronously in the calling context (typically Update()), not inside the returned tea.Cmd function. Since Update() is called synchronously by the Bubble Tea runtime, there are no race conditions in this pattern.
Applied to files:
src/internal/model.go
📚 Learning: 2025-04-27T17:04:47.888Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 775
File: src/internal/common/style_function.go:240-243
Timestamp: 2025-04-27T17:04:47.888Z
Learning: lazysegtree plans to refactor textinput.Model creation in the future to fix the code duplication issue that caused a bug where the sidebar search model was created without setting the Width property.
Applied to files:
src/internal/common/style_function.go
🧬 Code graph analysis (3)
src/internal/type.go (1)
src/internal/ui/bulk_rename/model.go (2)
Model(41-63)EditorModeAction(233-238)
src/internal/model.go (4)
src/internal/common/default_config.go (2)
Config(11-11)Hotkeys(12-12)src/internal/ui/bulk_rename/model.go (4)
EditorModeAction(233-238)BulkRenameAction(244-246)ExecuteBulkRename(505-510)RenamePreview(65-70)src/pkg/string_function/overplace.go (1)
PlaceOverlay(60-115)src/internal/handle_file_operations.go (1)
m(78-101)
src/internal/common/style_function.go (4)
src/internal/ui/bulk_rename/model.go (1)
Model(41-63)src/internal/common/style.go (1)
ModalStyle(20-20)src/internal/common/predefined_variable.go (1)
ModalWidth(30-30)src/internal/ui/prompt/model.go (1)
GenerateModel(22-38)
🔇 Additional comments (4)
src/internal/type.go (1)
8-8: LGTM! Bulk rename type integrations are well-structured.The additions properly integrate the bulk rename feature:
- Import of bulk rename UI package (line 8)
- Model field for bulk rename state (line 78)
- Pending editor action tracking (lines 96-97)
- Editor finished message type (line 210)
All follow existing patterns and align with the bulk rename workflow described in the AI summary.
Also applies to: 76-81, 96-97, 210-210
src/internal/model.go (3)
92-94: LGTM! Editor finish message handling follows established pattern.The integration of
editorFinishedForBulkRenameMsgcorrectly follows the existing async pattern used for other editor finish messages. The error is properly passed to the handler.
474-501: LGTM! Bulk rename input handling follows established patterns.The bulk rename integration in
handleInputUpdatescorrectly:
- Checks if the bulk rename modal is open
- Calls
HandleUpdateto process the message- Applies the resulting action via
applyBulkRenameAction- Properly batches commands when editor actions are returned
This follows the same pattern used for prompt and zoxide modals.
830-835: LGTM! Bulk rename overlay rendering is correctly implemented.The bulk rename modal overlay correctly:
- Checks if the modal is open with
IsOpen()- Renders the modal with
Render()- Centers the overlay using proper
/2divisors- Uses
GetWidth()andGetHeight()to calculate position
src/internal/type.go
Outdated
| type keyInputContext struct { | ||
| msg string | ||
| } // Copied items |
There was a problem hiding this comment.
🧩 Analysis chain
Verify keyInputContext usage - appears to be unused.
The keyInputContext type is defined but not referenced in any of the provided code. The comment // Copied items appearing immediately after suggests this may be misplaced or leftover from an earlier design iteration.
Run the following script to check for usage:
🏁 Script executed:
#!/bin/bash
# Search for keyInputContext usage across the codebase
rg -n "keyInputContext" --type=go -C3Length of output: 283
Remove unused keyInputContext type definition.
The ripgrep search confirms that keyInputContext is defined but never referenced anywhere in the codebase. This is dead code that should be removed from lines 143-145.
🤖 Prompt for AI Agents
In src/internal/type.go around lines 143 to 145, remove the dead/unused type
definition keyInputContext { msg string } because ripgrep shows it is never
referenced; delete these lines and run `go vet`/`go build` to ensure no usages
remain and update any nearby comments or imports if they become unnecessary
after removal.
There was a problem hiding this comment.
@lazysegtree, acknowledged. This is a critical M3 issue that must be fixed before merge.
The keyInputContext type defined at lines 143-145 is confirmed dead code—it's declared but never referenced anywhere in the codebase. Best practices for Go code recommend regularly refactoring to ensure every part serves its intended purpose, so this unused type definition should be removed.
Action required: Delete lines 143-145:
type keyInputContext struct {
msg string
}Also note that the comment // Copied items appears immediately after this type; once the type is removed, verify that comment is positioned correctly above the copyItems type definition.
🧠 Learnings used
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 973
File: src/internal/ui/processbar/model_update.go:7-27
Timestamp: 2025-08-03T14:49:31.221Z
Learning: lazysegtree prefers to keep test-only code simple without adding production-level concerns like goroutine synchronization, cancellation contexts, or complex lifecycle management, even when such patterns might prevent potential issues, since the complexity isn't justified for test utilities.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/handle_modal.go:253-253
Timestamp: 2025-09-12T05:16:10.488Z
Learning: lazysegtree identified that the fuzzy search function in src/internal/handle_modal.go for the help menu only searches on item.description but should also include item.key in the search haystack to provide comprehensive search functionality, as users expect to find hotkeys by searching for their key names.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T07:40:51.938Z
Learning: lazysegtree prefers simpler implementation approaches when the alternatives are significantly more complex, even if the alternatives might be architecturally cleaner, prioritizing maintainability and avoiding over-engineering.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 969
File: src/internal/key_function.go:40-40
Timestamp: 2025-08-03T09:34:55.721Z
Learning: lazysegtree emphasizes proper dependency direction in software architecture, preferring that low-level components (like modal handlers) should not depend on high-level components (like the main model object). He also prioritizes performance considerations, noting that creating objects on every keypress in hot code paths like key handling is inefficient and should be avoided.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 985
File: src/internal/model.go:0-0
Timestamp: 2025-08-11T01:49:30.040Z
Learning: lazysegtree prefers maintaining code correctness through proper design and invariants rather than adding defensive bounds checks at every slice access point, viewing such defensive programming as "duct taping" that can mask actual bugs instead of fixing them at their source.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
|
@lazysegtree Could I work on this PR next month? I’m busy until the end of this month |
|
Okay. I might take up the rest of the work and finish this if I get time this month. |
|
M3 - Typing J and K in the Find & Replace text boxes does't works. It instead does scrolling. |
I'll fix some of the issues ASAP and sorry for the inconvenience made |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/internal/model.go (4)
340-377: Key routing rework looks structurally sound, but relies on each modal to handle Quit explicitly.The split into
handleModalOrDefaultKey,routeModalInput,isAnyModalActive, andhandleDefaultKeyis consistent and keeps global quit/CD‑quit handling out of modal contexts, which is good.However, because
routeModalInputshort‑circuits whenm.promptModal.IsOpen() || m.zoxideModal.IsOpen() || m.bulkRenameModel.IsOpen(), global Quit/CdQuit hotkeys are effectively ignored whenever these modals are open. That makes correct app exit dependent on each modal’s ownHandleUpdatehandlingcommon.Hotkeys.Quit/CdQuit— and the bulk‑rename modal currently does not (see itsHandleUpdate), so the app cannot be quit while it’s open.Either:
- add Quit/CdQuit handling in the bulk‑rename modal (and other affected modals), or
- relax the
isAnyModalActivecheck to still allow Quit/CdQuit through.Given this was already flagged earlier for bulk rename/zoxide, it should be addressed before merge.
Also applies to: 378-423
543-566: Editor command parsing should use shell‑style splitting, notstrings.Fields.
handleEditorModeActionusesstrings.Fields(action.Editor)and assumesparts[0]is the binary. This breaks for common editor commands with spaces/quotes (e.g."C:\Program Files\Editor\editor.exe" --flag), causing failed spawns even though the configuration is valid.Reuse the earlier suggestion to switch to a shell‑style splitter (e.g.
github.com/google/shlex) and handle parse errors:func (m *model) handleEditorModeAction(action bulkrename.EditorModeAction) tea.Cmd { m.pendingEditorAction = &action - parts := strings.Fields(action.Editor) - cmd := parts[0] - args := append(parts[1:], action.TmpfilePath) - - c := exec.Command(cmd, args...) + parts, perr := shlex.Split(action.Editor) + if perr != nil || len(parts) == 0 { + slog.Error("Failed to parse editor command", "editor", action.Editor, "error", perr) + return nil + } + args := append(parts[1:], action.TmpfilePath) + c := exec.Command(parts[0], args...) return tea.ExecProcess(c, func(err error) tea.Msg { return editorFinishedForBulkRenameMsg{err} }) }…and add the
shleximport.Check github.com/google/shlex usage in Go projects: confirm it's still the idiomatic choice for shell-style splitting of command strings.
568-607: Editor‑mode result handling: validation gaps (path separators, duplicates, filesystem checks).The overall flow (
handleEditorFinishedForBulkRename→readEditorOutputLines→buildRenamePreviews→validateRename→filterValidPreviews→ExecuteBulkRename) is clean and matches the lazy‑validation design. A few issues remain:
- Only
strings.TrimSpaceis used when reading lines (readEditorOutputLines) and again inbuildRenamePreviews, which strips intentional leading/trailing spaces from filenames and can drop trailing blank lines.validateRenameonly checks “no change”, case‑only rename, and pre‑existing paths. It doesn’t:
- reject names containing path separators (accidental moves into subdirs),
- detect duplicate targets within the current selection (collisions inside the batch).
These were already raised earlier; they’re still unaddressed here.
Consider:
- Preserving spaces and handling CRLF robustly when parsing the tmpfile (scanner + stripping only
\r), and- Adding path‑separator and duplicate‑target checks before
os.Stat, mirroring the suggested diff in the earlier review (map of planned targets keyed by directory+normalized name).That will avoid surprising renames (moves/collisions) and better protect users from subtle editor mistakes.
609-665: Tmpfile parsing currently trims filenames; this can mangle valid names.
readEditorOutputLinesandbuildRenamePreviewsboth usestrings.TrimSpace, which:
- removes leading/trailing spaces from filenames (valid on POSIX),
- trims trailing blank lines, changing the line count.
Given this is editor‑driven, users may intentionally include spaces. Reuse the scanner‑based approach previously suggested (read lines as‑is, strip only a trailing
\rfor CRLF) and avoid further trimming here — validation can reject truly invalid names separately.This is the same issue called out in earlier comments and should be fixed here for correctness.
src/internal/ui/bulk_rename/model.go (3)
350-358: Title‑case conversion collapses whitespace and doesn’t respect original spacing.
toTitleCaseusesstrings.Fieldsandstrings.Join, which:
- collapses multiple spaces/tabs into a single space, and
- strips leading/trailing whitespace.
Earlier feedback already called this out; the function still behaves that way. If filenames with multiple spaces or specific spacing need to be preserved, please switch to a rune‑wise implementation that:
- walks the string rune by rune,
- tracks whether we are at the start of a “word” (after whitespace),
- uppercases the first rune of each word and lowercases subsequent runes,
- writes all original whitespace characters unchanged.
That will fix the multi‑space issue without regressing case conversion.
360-388: Filename validation is missing path‑separator checks and duplicate‑target detection.
validateRenamein the UI model mirrors the editor‑mode validation in the main model: it only checks for empty/no‑change, allows case‑only renames, and flags pre‑existing targets viaos.Stat. It does not:
- reject names containing path separators (
/,\, oros.PathSeparator), which lets users accidentally move files into subdirectories, or- detect duplicate new names within the same directory in a single batch, leading to intra‑batch collisions at execution time.
These are correctness issues for a bulk‑rename feature and were already highlighted in prior review comments. Please:
- add a path‑separator guard early in
validateRename, and- add duplicate‑target detection (per directory) either here or in preview generation, using a map keyed on directory + normalized
newName(with case normalization for case‑insensitive platforms if desired).That way users see conflicts in the preview before running the operation.
389-395:GetWidth/GetHeightignore configured dimensions; bulk‑rename operation lacks case‑only/partial‑failure handling.Two independent issues here:
Hard‑coded dimensions.
GetWidthandGetHeightalways return80and25, ignoring thewidth/heightfields set inDefaultModel. Either:
- change them to return
m.width/m.height, or- drop the fields if the modal is meant to be fixed‑size.
Right now the fields are dead weight and any future dynamic sizing via those fields would silently not work.
Bulk rename execution: case‑only renames and error handling.
bulkRenameOperation:
- uses a single
os.Renamecall, which fails for case‑only renames on Windows,- breaks the loop on the first error, leaving remaining previews unprocessed,
- only toggles
p.StatetoFailedwhen the first error hits, without tracking multiple failures.Earlier review already suggested:
- handling case‑only renames via a two‑step temp name on Windows, and
- continuing through the whole batch while tracking whether any error occurred, setting the final state to
Failedif so.Please adopt that approach or an equivalent to avoid partial progress and to support case‑only renames reliably.
Also applies to: 396-435
🧹 Nitpick comments (2)
src/internal/ui/bulk_rename/navigation.go (1)
3-23: Avoid magic numbers for rename‑type count; tie to enum instead.
nextType/prevTypeuse raw6and5for cycling, andnavigateDownhard‑codescursor < 1. This works with the current 6 rename types and 2 input fields, but will silently break if types/inputs are added.Consider deriving bounds from the enum/structure, e.g.:
- m.renameType = RenameType((int(m.renameType) + 1) % 6) + const renameTypeCount = int(EditorMode) + 1 + m.renameType = RenameType((int(m.renameType) + 1) % renameTypeCount)and defining a small constant for the max cursor index. This keeps navigation robust against future changes.
Also applies to: 25-44, 46-60
src/internal/ui/bulk_rename/model.go (1)
128-191: Editor selection logic is correct and now matches the desired openFileWithEditor behavior.
handleEditorModecorrectly triescommon.Config.Editor, then$EDITOR, then falls back tonotepadon Windows andnanoelsewhere, as requested. Error handling on tmpfile creation/write is also appropriate (settingerrorMessageand cleaning up the tmpfile). No further changes needed here beyond possibly deduplicating editor selection into a shared helper later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/internal/model.go(7 hunks)src/internal/ui/bulk_rename/model.go(1 hunks)src/internal/ui/bulk_rename/navigation.go(1 hunks)src/internal/ui/bulk_rename/type.go(1 hunks)src/superfile_config/hotkeys.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/superfile_config/hotkeys.toml
🧰 Additional context used
🧠 Learnings (27)
📓 Common learnings
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 0
File: :0-0
Timestamp: 2025-04-12T12:00:32.688Z
Learning: In PR #767 for yorukot/superfile, the focus is on moving code (especially sidebar-related functionality) to a more organized structure without changing functionality. Pre-existing issues should be ignored since the purpose is code reorganization, not fixing existing problems.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1113
File: src/superfile_config/vimHotkeys.toml:66-67
Timestamp: 2025-11-03T01:53:26.636Z
Learning: In yorukot/superfile, modal-specific hotkeys (like nav_bulk_rename and rev_nav_bulk_rename using tab/shift+tab in the bulk rename modal) can safely conflict with global hotkey bindings (like next_file_panel and previous_file_panel) because the modal captures input when focused, and the global bindings remain active when the modal is closed. This context-specific key handling is intentional and not considered a conflict.
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the preview panel refactoring created duplicate ImagePreviewer instances (one in defaultModelConfig and another in preview.New()), which caused image preview crashes. The refactoring also renamed SetContextWithRenderText to SetContentWithRenderText, and moved global batCmd to per-instance batCmd initialization, all contributing to crashes during image preview and test failures.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-07-27T15:32:06.922Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:32:06.922Z
Learning: When reviewing large refactoring PRs that change async patterns (like moving from goroutines to tea.Cmd), always check for incomplete refactoring where some call sites still use the old pattern while others use the new pattern, as this often leads to compilation errors and architectural inconsistencies.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-09-06T13:42:44.590Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1039
File: src/internal/ui/zoxide/model.go:53-54
Timestamp: 2025-09-06T13:42:44.590Z
Learning: The zoxide modal in src/internal/ui/zoxide/model.go is missing handling for common.Hotkeys.Quit when zClient is nil (lines 52-54), only handling ConfirmTyping and CancelTyping. This creates inconsistency with other modals like sort options menu, help menu, and notify model which all properly handle the Quit hotkey. The prompt modal has the same inconsistency.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-09T14:23:14.164Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/ui/rendering/truncate.go:6-6
Timestamp: 2025-09-09T14:23:14.164Z
Learning: The truncate test failure in src/internal/ui/rendering/truncate_test.go during the ANSI package migration from experimental to stable was caused by a truncation bug in the experimental package. The experimental package incorrectly truncated strings even when input length equaled maxWidth (e.g., "1234" with maxWidth=4 became "1..."), while the stable package correctly only truncates when input exceeds maxWidth. The test expectation was based on the buggy behavior and needs to be corrected.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-20T01:40:50.076Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1081
File: src/internal/ui/sidebar/directory_utils.go:103-112
Timestamp: 2025-09-20T01:40:50.076Z
Learning: lazysegtree identified code duplication between removeNotExistingDirectories and TogglePinnedDirectory functions in src/internal/ui/sidebar/directory_utils.go, specifically 6 lines of JSON marshaling and file writing logic. He prefers to track such duplication fixes in separate GitHub issues and suggests either extracting common util functions or creating a PinnedDir manager for centralized Read/Write operations to PinnedFile.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-08-29T13:56:33.955Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1011
File: src/internal/handle_modal.go:145-203
Timestamp: 2025-08-29T13:56:33.955Z
Learning: lazysegtree identified that help menu navigation functions (helpMenuListUp and helpMenuListDown) in src/internal/handle_modal.go are too complicated, can be simplified, are similar to sidebar navigation but inconsistent, and lack unit tests. He prefers to track such technical debt in separate GitHub issues rather than addressing it in the current PR scope.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/navigation.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-08-24T03:25:10.117Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:252-275
Timestamp: 2025-08-24T03:25:10.117Z
Learning: In PR #1013 for yorukot/superfile, when reviewing the ReadFileContent utility function, lazysegtree chose to implement only the parameter renaming fix (filepath → filePath) to avoid shadowing and declined buffer size increases and optional truncation enhancements, preferring to keep the utility function scope focused and avoid over-engineering when the additional features aren't immediately needed.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, lazysegtree reported image preview crashes and rename test failures after refactoring FilePreviewPanel to a separate preview package. The crashes were likely caused by: 1) imagePreviewer initialization differences between old global variable and new per-instance creation, 2) method name changes from SetContextWithRenderText to SetContentWithRenderText, and 3) batCmd initialization moving from global to per-instance without preserving the original configuration.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-09-04T07:24:30.872Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1041
File: src/internal/default_config.go:38-38
Timestamp: 2025-09-04T07:24:30.872Z
Learning: In the superfile codebase, the main model struct has a zClient field for zoxide directory tracking, and the trackDirectoryWithZoxide function in model.go checks if m.zClient is nil before proceeding. When reviewing model initialization functions like defaultModelConfig, ensure all struct fields are properly assigned, especially external service clients like zClient that enable core functionality.
Applied to files:
src/internal/model.go
📚 Learning: 2025-07-27T15:35:25.617Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/default_config.go:16-16
Timestamp: 2025-07-27T15:35:25.617Z
Learning: In yorukot/superfile, deleteItemWarn() only displays a warning modal via the channelMessage system and does not perform actual file deletion. The actual deletion happens when the user confirms the modal, which triggers getDeleteCmd() that returns a tea.Cmd. The architecture intentionally separates UI modal operations (using channelMessage/goroutines) from file I/O operations (using tea.Cmd pattern), creating clean separation of concerns between UI state management and file operations.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-12T05:16:10.488Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/handle_modal.go:253-253
Timestamp: 2025-09-12T05:16:10.488Z
Learning: lazysegtree identified that the fuzzy search function in src/internal/handle_modal.go for the help menu only searches on item.description but should also include item.key in the search haystack to provide comprehensive search functionality, as users expect to find hotkeys by searching for their key names.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-06T12:03:51.042Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1039
File: src/internal/common/config_type.go:156-156
Timestamp: 2025-09-06T12:03:51.042Z
Learning: lazysegtree identified that the in-app help menu in getHelpMenuData function in src/internal/default_config.go is missing many hotkeys defined in HotkeysType, including the new OpenZoxide hotkey, causing inconsistent user experience and reduced feature discoverability.
Applied to files:
src/internal/model.go
📚 Learning: 2025-11-03T01:53:26.636Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1113
File: src/superfile_config/vimHotkeys.toml:66-67
Timestamp: 2025-11-03T01:53:26.636Z
Learning: In yorukot/superfile, modal-specific hotkeys (like nav_bulk_rename and rev_nav_bulk_rename using tab/shift+tab in the bulk rename modal) can safely conflict with global hotkey bindings (like next_file_panel and previous_file_panel) because the modal captures input when focused, and the global bindings remain active when the modal is closed. This context-specific key handling is intentional and not considered a conflict.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-02T11:47:07.713Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 967
File: src/internal/key_function.go:45-47
Timestamp: 2025-08-02T11:47:07.713Z
Learning: lazysegtree prefers to track technical debt and architectural improvements in dedicated GitHub issues when they are identified during PR reviews but are beyond the scope of the current PR, particularly for complex refactoring needs like input handling architecture that would require significant changes.
Applied to files:
src/internal/model.go
📚 Learning: 2025-04-28T04:27:33.074Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/model_render.go:254-256
Timestamp: 2025-04-28T04:27:33.074Z
Learning: When race conditions or other issues that are unrelated to the current PR's focus are identified during review, lazysegtree prefers creating separate GitHub issues to track them rather than addressing them in the current PR. This helps maintain PR focus and scope.
Applied to files:
src/internal/model.go
📚 Learning: 2025-04-28T03:48:46.327Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 781
File: src/internal/ui/sidebar/render.go:46-48
Timestamp: 2025-04-28T03:48:46.327Z
Learning: The user (lazysegtree) prefers to keep PRs focused and manageable in size, sometimes intentionally leaving TODO comments to track minor issues for later PRs rather than addressing everything at once.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the tests were actually passing despite lazysegtree reporting crashes. The real issue was that production image preview crashes occurred during actual application usage due to duplicate ImagePreviewer instances (one in defaultModelConfig and one in preview.New()), while the test environment didn't stress the image preview system enough to expose the conflicts.
Applied to files:
src/internal/model.gosrc/internal/ui/bulk_rename/model.go
📚 Learning: 2025-08-29T14:11:21.380Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/model_render.go:329-341
Timestamp: 2025-08-29T14:11:21.380Z
Learning: lazysegtree prefers to defer help menu rendering optimizations and other technical debt improvements to separate GitHub issues when the current PR scope has grown too large, maintaining focus on the primary refactoring objectives while tracking performance improvements for future work.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-06T10:54:31.444Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When lazysegtree says a review is "inaccurate and pre-existing issues. Not much useful," it means I should focus specifically on bugs or problems introduced by the current PR changes, not architectural concerns or code quality issues that were already present. The refactoring work in superfile PRs is generally well-implemented and should be evaluated on whether the specific changes work correctly, not on broader codebase architecture.
Applied to files:
src/internal/model.go
📚 Learning: 2025-07-27T08:49:09.687Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 963
File: src/internal/handle_file_operations.go:567-570
Timestamp: 2025-07-27T08:49:09.687Z
Learning: lazysegtree prefers to defer technical debt issues like model mutation concerns to later PRs when the current PR has already grown too large, maintaining focus on the primary objectives while acknowledging the need to track such issues for future work.
Applied to files:
src/internal/model.go
📚 Learning: 2025-09-09T13:29:11.771Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1044
File: src/internal/utils/file_utils.go:149-149
Timestamp: 2025-09-09T13:29:11.771Z
Learning: lazysegtree prefers to keep PR scope focused on the primary objectives and considers pre-existing technical debt issues as out of scope for migration/refactoring PRs, preferring to defer such issues to separate GitHub issues rather than expanding the current PR scope.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-06T10:54:31.444Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:47-47
Timestamp: 2025-08-06T10:54:31.444Z
Learning: When reviewing PRs, especially large refactoring ones, focus specifically on issues introduced by the changes rather than flagging pre-existing architectural problems or code smells that were already present before the PR. Distinguish between new bugs versus existing technical debt.
Applied to files:
src/internal/model.go
📚 Learning: 2025-07-24T03:46:29.516Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 950
File: src/internal/ui/metadata/README.md:7-7
Timestamp: 2025-07-24T03:46:29.516Z
Learning: lazysegtree prefers to defer comprehensive unit testing to separate PRs when the current PR has grown too large, maintaining focus on the primary refactoring objectives while tracking testing requirements in dedicated GitHub issues.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-24T03:24:50.857Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1013
File: src/internal/utils/file_utils.go:18-21
Timestamp: 2025-08-24T03:24:50.857Z
Learning: The superfile project uses github.com/charmbracelet/x/ansi as the standardized ANSI package and deprecates github.com/charmbracelet/x/exp/term/ansi to avoid binary bloat and potential incompatibilities.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-05T11:51:37.645Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 979
File: src/internal/common/predefined_variable.go:44-44
Timestamp: 2025-08-05T11:51:37.645Z
Learning: In Bubble Tea applications, when functions like warnModalForRenaming() capture state (like m.ioReqCnt) and increment it before returning a tea.Cmd, the increment happens synchronously in the calling context (typically Update()), not inside the returned tea.Cmd function. Since Update() is called synchronously by the Bubble Tea runtime, there are no race conditions in this pattern.
Applied to files:
src/internal/model.go
📚 Learning: 2025-08-27T11:32:17.326Z
Learnt from: lazysegtree
Repo: yorukot/superfile PR: 1021
File: .gitignore:37-37
Timestamp: 2025-08-27T11:32:17.326Z
Learning: In PR #1021 for yorukot/superfile, the image preview crashes and test failures were caused by duplicate ImagePreviewer initialization - one in defaultModelConfig() and another in preview.New(). This created two separate instances that conflicted in terminal state management, causing crashes during image preview operations and test execution.
Applied to files:
src/internal/ui/bulk_rename/model.go
🧬 Code graph analysis (4)
src/internal/model.go (5)
src/internal/common/default_config.go (2)
Config(11-11)Hotkeys(12-12)src/internal/common/type.go (1)
ModelAction(5-7)src/internal/ui/bulk_rename/type.go (3)
EditorModeAction(49-54)BulkRenameAction(41-43)RenamePreview(34-39)src/internal/ui/bulk_rename/model.go (1)
ExecuteBulkRename(396-401)src/internal/handle_file_operations.go (2)
m(78-101)m(61-73)
src/internal/ui/bulk_rename/type.go (1)
src/internal/ui/processbar/process.go (1)
ProcessState(37-37)
src/internal/ui/bulk_rename/navigation.go (3)
src/internal/ui/bulk_rename/type.go (2)
Model(10-32)RenameType(7-7)src/internal/ui/bulk_rename/model.go (3)
FindReplace(22-22)AddPrefix(23-23)AddSuffix(24-24)src/internal/ui/zoxide/navigation.go (2)
m(3-13)m(15-25)
src/internal/ui/bulk_rename/model.go (6)
src/internal/ui/bulk_rename/type.go (7)
RenameType(7-7)CaseType(8-8)Model(10-32)RenamePreview(34-39)BulkRenameAction(41-43)EditorModeAction(49-54)BulkRenameResultMsg(56-59)src/internal/common/style_function.go (1)
GenerateBulkRenameTextInput(330-343)src/internal/common/type.go (1)
ModelAction(5-7)src/config/icon/icon.go (5)
Error(36-36)Terminal(44-44)Space(10-10)Done(38-38)Cursor(31-31)src/internal/utils/consts.go (1)
OsWindows(8-8)src/internal/ui/processbar/process.go (4)
ProcessState(37-37)Cancelled(42-42)Failed(43-43)Successful(41-41)
🔇 Additional comments (5)
src/internal/model.go (2)
455-501: Bulk‑rename modal integration into the update loop is correct and follows existing patterns.
handleInputUpdatesprioritizesm.bulkRenameModel.IsOpen()and wires actions throughapplyBulkRenameAction, which keeps the main model agnostic of bulk‑rename internals and fits the existing prompt/zoxide pattern. This looks good from an architectural and correctness standpoint.
784-835: Bulk‑rename overlay centering is consistent and correct.The new bulk‑rename overlay branch mirrors the centering logic used for other modals (prompt, zoxide, sort options). Assuming
GetWidth/GetHeightreflect the model’s configured dimensions, this will render the modal properly centered.src/internal/ui/bulk_rename/type.go (1)
7-32: Type definitions for bulk‑rename model are cohesive and match usage.
Model,RenamePreview,BulkRenameAction,EditorModeAction, andBulkRenameResultMsgcleanly separate UI state, preview data, actions, and process results, and align with how they’re consumed inmodel.goand the main model. No issues from a type‑design perspective.Also applies to: 34-59
src/internal/ui/bulk_rename/model.go (2)
240-277: Preview generation and transformations align with lazy validation goals.The
GeneratePreview/GeneratePreviewWithValidationsplit, plusgeneratePreviewdelegating tocreateRenamePreviewwithvalidateflag, achieves the intended “no os.Stat on every render” behavior. Transformation helpers (applyFindReplace,applyPrefix,applySuffix,applyNumbering,applyCaseConversion) all operate only on the filename stem and preserve extensions, which is the expected UX.This structure is solid and matches the performance requirements mentioned in the PR discussion.
Also applies to: 279-348
437-447: Result message and cursor color helpers look good.
NewBulkRenameResultMsgand itsGetState/GetCountaccessors, plusGetCursorColor, are straightforward and integrate cleanly with the process bar and theming systems. No issues here.Also applies to: 449-451
| func (m *Model) HandleUpdate(msg tea.Msg) (common.ModelAction, tea.Cmd) { | ||
| slog.Debug("bulk_rename.Model HandleUpdate()", "msg", msg) | ||
| var action common.ModelAction = common.NoAction{} | ||
| var cmd tea.Cmd | ||
|
|
||
| if !m.IsOpen() { | ||
| slog.Error("HandleUpdate called on closed bulk rename modal") | ||
| return action, cmd | ||
| } | ||
|
|
||
| switch msg := msg.(type) { | ||
| case tea.KeyMsg: | ||
| switch { | ||
| case slices.Contains(common.Hotkeys.CancelTyping, msg.String()): | ||
| m.Close() | ||
| case slices.Contains(common.Hotkeys.ConfirmTyping, msg.String()): | ||
| action = m.handleConfirm() | ||
| case slices.Contains(common.Hotkeys.ListUp, msg.String()): | ||
| m.adjustValue(-1) | ||
| case slices.Contains(common.Hotkeys.ListDown, msg.String()): | ||
| m.adjustValue(1) | ||
| case slices.Contains(common.Hotkeys.NavBulkRename, msg.String()): | ||
| m.nextType() | ||
| case slices.Contains(common.Hotkeys.RevNavBulkRename, msg.String()): | ||
| m.prevType() | ||
| default: | ||
| cmd = m.handleTextInputUpdate(msg) | ||
| } | ||
| default: | ||
| cmd = m.handleTextInputUpdate(msg) | ||
| } |
There was a problem hiding this comment.
Key handling in bulk‑rename modal still eats typing keys (e.g. j/k) for control actions.
In HandleUpdate, ListUp/ListDown are always interpreted as adjustValue, even when the find/replace inputs are active. For Vim-style hotkeys where ListUp/ListDown include k/j, this means typing j/k into the Find/Replace fields never reaches the text inputs — it’s treated as navigation/adjustment instead.
Given the prior report about j/k scrolling instead of inserting characters in text boxes, this looks unresolved. Options:
- Disable
ListUp/ListDownhandling when the active mode uses text inputs (e.g.FindReplace), letting thetextinput.Modelconsume these keys; or - Use dedicated modal‑specific navigation bindings (e.g.
NavBulkRename/RevNavBulkRenameusing Tab/Shift‑Tab) for cursor movement and keepListUp/ListDownfor list navigation elsewhere.
This will make the modal behave more like a normal text field while still supporting keyboard navigation between fields.
Also applies to: 201-239
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Failed
(1 hotspot with Complex Method, Complex Conditional)
(1 file with Bumpy Road Ahead)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| model.go | 2 rules in this hotspot | 8.37 → 7.94 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| model.go | 1 critical rule | 9.03 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| key_function.go | 8.06 → 8.47 | Complex Method, Overall Code Complexity |
Quality Gate Profile: The Bare Minimum
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
Issues fixed so far @lazysegtree :
|
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests