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 📝 WalkthroughWalkthroughAdds 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.
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
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Issues fixed so far @lazysegtree :
|
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.
|
I attempted to start fixing the code, but there is just too much to fix. I will check it sometime later. |
There was a problem hiding this comment.
(2 files improve in Code Health)
Gates Failed
(2 hotspots with Complex Method, Complex Conditional, Large Method)
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 |
| default_config.go | 1 rule in this hotspot | 8.57 → 8.55 | Suppress |
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| handle_file_operations.go | 8.09 → 8.16 | Bumpy Road Ahead |
| 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.
|
@harryfrzz Meanwhile you should attempt to fix the issues that you can. Many issues are very troublesome for me to fix. I have to go through and understand the entire PR and changes. |
@lazysegtree I’ve fixed the remaining bugs in the code. Could you please review it? |
|
@harryfrzz Thanks for this. Most comments are fixed. I can see many lint issues Details
(4) Don't use init(). That's very unmaintainable. It can cause unwanted side effects, and makes program control flow obscure. It should be used only when necessary. |
|
I am assuming this PR has been abandonded. Its a good feature, just the implementation needs fixing. Its possible that one of the maintainers, or someone else will start from the current state and complete this feature, so I will keep it open. |
|
Hey @lazysegtree I would like to pick this up and finish it |
|
@harryfrzz Is that okay? I will wait one week for you to respond, else @0xjuicebox is allowed to pick it up. |
Yeah sure! My schedule is actually pretty packed right now, so feel free to let @0xjuicebox work on this |
4 participants
Summary by CodeRabbit
New Features
Documentation
Tests