feat: add save-file flow and ordered chooser output#1412
Open
Mjoyufull wants to merge 6 commits into
Open
Conversation
github-actions
Bot
added
awaiting pr review
documentation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a terminal file-chooser integration with two CLI modes ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Parser
participant Config as Config/Variable
participant TUI as TUI Model
participant Panel as File Panel
participant Writer as Output Writer
CLI->>Config: SetChooserRequest(Mode=Open, OutputFile)
Config->>TUI: Initialize with ChooserRequest
TUI->>Panel: Enable Open mode
User->>Panel: Select file(s) and confirm (hotkey)
Panel->>TUI: openChooserWriteSelectionAndQuit()
TUI->>Writer: Write newline-delimited absolute paths to OutputFile
Writer-->>TUI: Write success
TUI-->>CLI: Exit
sequenceDiagram
participant CLI as CLI Parser
participant Config as Config/Variable
participant TUI as TUI Model
participant Panel as File Panel
participant FS as Filesystem
participant Notify as Notification
participant Writer as Output Writer
CLI->>Config: SetChooserRequest(Mode=Save, OutputFile, SuggestedPath)
Config->>TUI: Initialize with ChooserRequest
TUI->>Panel: Enable SaveMode with suggested name
User->>Panel: Edit save name and confirm (hotkey)
Panel->>TUI: saveChooserConfirmFocusedItem()
TUI->>FS: Stat target (exists / portal placeholder?)
alt target exists and not placeholder
FS->>Notify: Show SaveOverwriteWarn modal
Notify->>User: Ask confirm/cancel
User->>Notify: Confirm
Notify->>TUI: Confirm action
end
TUI->>Writer: Write chosen target path to OutputFile
Writer-->>TUI: Write success
TUI-->>CLI: Exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal/ui/filepanel/render.go (1)
109-121:⚠️ Potential issue | 🔴 CriticalSaveMode label suppresses SelectMode count.
SaveMode and PanelMode are independent; enabling save mode does not reset PanelMode or clear selections. If a user selects items in SelectMode and then enters save flow, the panel can be in both SaveMode and SelectMode simultaneously. In this state,
getPanelModeInfo()returns"Save"immediately, hiding the selected count indicator even though the code atchooser.go:86explicitly handles multi-selection in this scenario.Either enforce this as a mutually-exclusive invariant by resetting PanelMode when enabling SaveMode, or update
getPanelModeInfo()to show the count:if m.SaveMode && m.PanelMode == SelectMode && selectedCount > 0could display"Save (%d)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/ui/filepanel/render.go` around lines 109 - 121, getPanelModeInfo currently returns "Save" whenever SaveMode is true, hiding the SelectMode count; change it to preserve the selection count when the panel is both in SaveMode and SelectMode by returning "Save" plus the selection count when m.SaveMode && m.PanelMode == SelectMode && selectedCount > 0 (e.g., "Save" + icon.Space + fmt.Sprintf("(%d)", selectedCount)), keeping the same icon (icon.Select); update the getPanelModeInfo function to implement this conditional instead of the unconditional Save early-return.
🧹 Nitpick comments (9)
src/internal/handle_modal.go (1)
75-81: Minor: duplicated teardown withcancelRename.Lines 77-79 replicate the exact body of
cancelRename(minus them.fileModel.Renaming = falseorder). Consider extracting a small helper (e.g.m.clearRenamingState()) and reusing it here and at the end ofconfirmRenameto avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/handle_modal.go` around lines 75 - 81, The teardown for ending a rename is duplicated in the m.isSaveChooserMode() branch and in cancelRename; extract those repeated statements into a new helper method (e.g., m.clearRenamingState()) that performs the shared steps (setting panel.Rename.Blur(), panel.Renaming = false, panel.SetSaveEntryName(panel.Rename.Value()) or the correct ordering used elsewhere) and call this helper from the m.isSaveChooserMode() branch and from the end of confirmRename/cancelRename (replace the duplicated lines in the branch with a single call to m.clearRenamingState()) so the rename-cleanup logic is centralized and cannot drift.src/internal/key_function.go (1)
58-106: Consider consolidating save-chooser guard instead of repeating it at each case.The same
if m.isSaveChooserMode() { return nil }block is duplicated across 8 cases. Cleaner alternative: evaluatem.isSaveChooserMode()once at the top ofmainKeyand either short-circuit a denylist of hotkeys, or dispatch through a small helper table. Not blocking — current form is explicit and readable, just repetitive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/key_function.go` around lines 58 - 106, The repeated guard `if m.isSaveChooserMode() { return nil }` across the hotkey case branches should be consolidated: in the `mainKey` handler (or the enclosing function handling `msg`) call `m.isSaveChooserMode()` once and, if true, short-circuit only for the relevant hotkeys (e.g. common.Hotkeys.ChangePanelMode, NextFilePanel, PreviousFilePanel, CloseFilePanel, CreateNewFilePanel, SplitFilePanel) by checking membership (slices.Contains) against a denylist and returning nil; alternatively delegate those hotkeys to a small helper like `denyInSaveChooser(msg)` that performs the membership check, then continue with the remaining switch body unchanged (preserving calls to `m.getFocusedFilePanel().ChangeFilePanelMode()`, `m.fileModel.NextFilePanel()`, `m.fileModel.PreviousFilePanel()`, `m.fileModel.CloseFilePanel()`, `m.fileModel.CreateNewFilePanel(variable.HomeDir)`, and `m.splitPanel()`).testsuite/tests/save_file_test.py (1)
6-6: Unused import.
core.test_constants as tconstis imported but never referenced in this test (only the overwrite variant usestconst.FILE_TEXT1). Safe to drop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/save_file_test.py` at line 6, Remove the unused import "core.test_constants as tconst" from testsuite/tests/save_file_test.py; locate the import statement for the symbol tconst and delete it, leaving other tests intact—if only the overwrite-variant uses tconst.FILE_TEXT1, keep that test's local import instead of a module-level import to avoid unused imports.contrib/superfile-wrapper.sh (1)
25-34: Collapse the three identical--chooser-filebranches.
directory=1,multiple=1, and the fallback all produce the same--chooser-fileinvocation. Either merge them into a singleelsebranch, or, if the intent is to eventually treat these distinctly (e.g., pass a--directoryflag or enforce single-select), add a TODO — as written, the extra branching just adds noise.♻️ Suggested consolidation
if [ "$save" = "1" ]; then # save a file set -- --save-file="$out" "$path" -elif [ "$directory" = "1" ]; then - # upload files from a directory - set -- --chooser-file="$out" "$path" -elif [ "$multiple" = "1" ]; then - # upload multiple files - set -- --chooser-file="$out" "$path" else - # upload only 1 file + # open/upload (single, multiple, or directory) set -- --chooser-file="$out" "$path" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/superfile-wrapper.sh` around lines 25 - 34, The three branches testing directory, multiple, and the fallback all call the identical set command (set -- --chooser-file="$out" "$path"); simplify by collapsing them into a single else branch or a single conditional that handles the shared behavior and remove the redundant elif blocks in contrib/superfile-wrapper.sh; if you expect different future behavior, replace the extra branches with one unified branch and add a TODO comment referencing the variables directory and multiple and the set -- --chooser-file="$out" "$path" invocation so intent is preserved.src/internal/handle_file_operations.go (1)
571-585: Simplify and parenthesizegetRenameCursorPospredicate.On Line 581 the mixed
||/&&chain is readable only if you remember Go’s precedence, and thenameLen > 0guard is unreachable-false: reachingcursorPos == 0means the loop matched a.at index 0, which impliesnameLen >= 1. Consider:♻️ Proposed cleanup
- if cursorPos == -1 || cursorPos == 0 && nameLen > 0 || isDirectory { + // No extension found, dotfile (e.g. ".bashrc"), or directory → place cursor at end. + if cursorPos <= 0 || isDirectory { cursorPos = nameLen }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/handle_file_operations.go` around lines 571 - 585, The conditional in getRenameCursorPos is hard to read and includes a redundant guard; replace the mixed ||/&& expression with a clearer predicate such as checking cursorPos <= 0 or isDirectory. Specifically, update getRenameCursorPos so the if uses a simple, parenthesized test (e.g., if cursorPos <= 0 || isDirectory) and remove the unreachable nameLen > 0 check so the function sets cursorPos = nameLen for both cursorPos == -1 and cursorPos == 0 cases and for directories.src/internal/ui/filepanel/utils.go (2)
178-206: Save-mode API is clean; one timing caveat withscrollToCursor(0).
EnableSaveModecallsscrollToCursor(0), which (persrc/internal/ui/filepanel/navigation.go:7-23) early-returns ifm.ElemCount() == 0. In the current chooser init path (chooser.gocallsUpdateCurrentFilePanelDir(startDir)beforeEnableSaveMode), elements are populated first, so this works. However, any future caller that enables save mode on an empty/unloaded panel would silently skip the cursor reset and could end up with the synthetic save-entry present at render time but the cursor parked elsewhere. Consider either:
- Documenting the "elements must be loaded first" precondition on
EnableSaveMode, or- Setting
m.cursor = 0directly (since the synthetic save entry is always prepended at index 0 whenSaveModeis true) rather than going throughscrollToCursor, which bails out on empty panels.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/ui/filepanel/utils.go` around lines 178 - 206, EnableSaveMode currently calls scrollToCursor(0) which early-returns when the panel is empty (see ElemCount/scrollToCursor), so callers that enable save mode on an unloaded panel can leave the cursor inconsistent; change EnableSaveMode to set m.cursor = 0 directly (and still call any necessary UI refresh) instead of using scrollToCursor, or alternatively add a clear comment documenting the precondition that elements must be loaded before calling EnableSaveMode; update references to EnableSaveMode, scrollToCursor, ElemCount and m.cursor accordingly.
101-103: Prefercmp.Compareover subtraction in the sort comparator.
a.order - b.orderworks here becauseselectOrderCounteris monotonically increasing and realistically bounded, but subtraction-based comparators are a well-known overflow footgun and most Go style guides recommendcmp.Compare(Go 1.21+) for clarity and safety.♻️ Proposed tweak
import ( + "cmp" "math" "path/filepath" "slices" ) ... slices.SortFunc(ordered, func(a, b orderedSelection) int { - return a.order - b.order + return cmp.Compare(a.order, b.order) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/ui/filepanel/utils.go` around lines 101 - 103, The comparator passed to slices.SortFunc currently subtracts integers (return a.order - b.order) which can overflow; update the comparator used on ordered (and type orderedSelection) to call cmp.Compare(a.order, b.order) instead, and add the "cmp" import; keep the same slices.SortFunc(ordered, func(a, b orderedSelection) int { ... }) call but return cmp.Compare(a.order, b.order) so the sort is safe and explicit.testsuite/tests/chooser_file_multiselect_test.py (1)
32-35: Unconditionaltime.sleep(close_wait_time)can flake under load.Relying on a fixed 3-second sleep assumes
spfhas exited and flushed the chooser file by then. On a busy CI runner this may be insufficient, while locally it's needlessly slow. If the harness exposes a "wait for process exit" primitive (thevalidate_spf_closed=Trueflag suggests one exists), prefer waiting on the process termination signal instead of a bare sleep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testsuite/tests/chooser_file_multiselect_test.py` around lines 32 - 35, The end_execution method currently does an unconditional time.sleep(self.close_wait_time) which can flake; replace the blind sleep with the harness/process exit wait primitive (or enable the existing validate_spf_closed behavior) so the method waits for the spf process to actually exit and the chooser file to be flushed before returning; update end_execution to call the harness's wait_for_spf_exit / wait_for_process_termination (or pass validate_spf_closed=True into the code path that closes spf) instead of using time.sleep, and keep the existing debug logs around the wait to retain observability.src/internal/chooser.go (1)
113-150: Silent no-op on directory / unexpected stat error gives no user feedback.In
confirmSaveChooserPath, two branches silently drop the user's action:
- Line 121–123: when the chosen path is an existing directory, the function returns with no notification. From the user's perspective in save mode, pressing confirm on a directory does nothing (no hint to type a filename, no warning).
- Line 147–149: any non-
IsNotExiststat error is logged and swallowed.Consider surfacing at least a brief notification (e.g., via
m.notifyModel) so the user understands why the save didn't proceed. This also helps when the portal pipesspfinto a non-interactive context and an operator checks logs vs. UI state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/chooser.go` around lines 113 - 150, In confirmSaveChooserPath, when the chosen path is an existing directory (the info.IsDir() branch) and in the default/error branch where stat returns a non‑IsNotExist error, surface a user-visible notification instead of silently returning: call m.notifyModel with a short, user-friendly message (e.g., "Please choose a file name; selected path is a directory" for the directory case, and for the stat error include err.Error() like "Cannot access target path: <err>") immediately before returning, while preserving existing slog.Error logging and keeping the existing behavior of not proceeding with chooserWriteAndQuit; update the info.IsDir() branch and the default case (where you currently slog.Error("Save chooser target stat failed", ...)) to add these notifyModel calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cmd/main_test.go`:
- Around line 18-19: The test sets tempDir with t.TempDir() and chdirs to it but
does not resolve symlinks, causing path mismatch on macOS; update the test to
call filepath.EvalSymlinks on the returned tempDir (handle the error with
require.NoError), use the resolved path for os.Chdir and for any expected
SuggestedPath calculations (e.g., where tempDir is used to build expected
SuggestedPath in assertions and in buildChooserRequest-related checks) so
comparisons are done against the canonical path; apply the same change to the
second occurrence referenced at line 39.
In `@src/internal/chooser.go`:
- Around line 131-146: When handling the os.IsNotExist case, avoid leaving the
empty placeholder if m.chooserWriteAndQuit fails: after calling
createSaveChooserPlaceholder(path) and then
m.chooserWriteAndQuit([]string{path}), if writeErr != nil remove the placeholder
(os.Remove(path)) and log a clear error combining the original writeErr and any
removeErr; alternatively, move creation of the actual on-disk placeholder until
after a successful chooserWriteAndQuit so createSaveChooserPlaceholder(path) is
only called on success—update the block around createSaveChooserPlaceholder and
m.chooserWriteAndQuit accordingly and ensure errors from os.Remove are also
logged.
- Line 71: Replace the restrictive config-file permissions with user-file
permissions for user-initiated save operations: in chooserWriteAndQuit() where
you call os.WriteFile(m.chooser.request.OutputFile, ...) and in
createSaveChooserPlaceholder() where the placeholder is written, change the file
mode argument from utils.ConfigFilePerm to utils.UserFilePerm so these
user-requested output files are created with 0644 instead of 0600.
In `@src/internal/ui/filepanel/columns.go`:
- Around line 95-114: The panic happens because renderElementName assumes
elem.Info is non-nil; update Model.renderElementName to check "if elem.Info ==
nil" before accessing elem.Info.Mode(), treating nil Info as not-a-symlink (and
using a safe fallback name/flags) so the function short-circuits safely for
out-of-range or synthetic elements (GetElementAtIdx zero-value and
getSaveEntryElement). Also audit and add the same nil-Info guard to
renderFileSize, renderModifyTime, and renderPermissions so they don't
dereference elem.Info for SaveTarget or out-of-bounds elements, returning
sensible defaults when Info is nil.
In `@src/internal/ui/filepanel/get_elements.go`:
- Around line 117-121: The synthetic SaveTarget element (created by
m.getSaveEntryElement() when m.SaveMode) must be excluded from selection and
destructive operations: update SelectAllItem and SingleItemSelect to skip
elements whose Type or ID equals SaveTarget (or whose Name equals
SaveEntryName), and add a guard in copyMultipleItem/GetSelectedLocations (or the
higher-level clipboard/delete handlers) to filter out any elements flagged as
SaveTarget before performing copy/delete; also ensure applyTargetFileCursor()
checks for SaveEntryName collisions and resolves to the real file if present
(e.g., ignore synthetic entry when mapping cursor by name). This ensures
scrollToCursor(0)/EnableSaveMode can focus the save entry while preventing it
from being treated as a selectable file for copy/delete.
In `@testsuite/tests/chooser_file_multiselect_test.py`:
- Line 42: The assertion message has a grammar typo; update the message in the
assertion that checks existence (assert
self.env.fs_mgr.check_exists(CHOOSER_FILE)) to read "File {CHOOSER_FILE} does
not exist" instead of "does not exists" so the check for CHOOSER_FILE uses the
correct wording.
In `@website/src/content/docs/troubleshooting.md`:
- Around line 24-32: The docs are inconsistent about the rename hotkey for the
ghost between README.md ("Chooser and save mode" documents Ctrl+R) and this
troubleshooting entry; update the troubleshooting.md section about portal
wrapper behavior to mention the rename hotkey (Ctrl+R) for parity with README,
or if Ctrl+R is deprecated, update README instead—ensure references to the
hotkey appear consistently alongside the --chooser-file / --save-file
explanations and mention of the ghost rename behavior so users of
xdg-desktop-portal-termfilechooser and the spf --save-file="$out" "$path" save
flow get the same instruction.
---
Outside diff comments:
In `@src/internal/ui/filepanel/render.go`:
- Around line 109-121: getPanelModeInfo currently returns "Save" whenever
SaveMode is true, hiding the SelectMode count; change it to preserve the
selection count when the panel is both in SaveMode and SelectMode by returning
"Save" plus the selection count when m.SaveMode && m.PanelMode == SelectMode &&
selectedCount > 0 (e.g., "Save" + icon.Space + fmt.Sprintf("(%d)",
selectedCount)), keeping the same icon (icon.Select); update the
getPanelModeInfo function to implement this conditional instead of the
unconditional Save early-return.
---
Nitpick comments:
In `@contrib/superfile-wrapper.sh`:
- Around line 25-34: The three branches testing directory, multiple, and the
fallback all call the identical set command (set -- --chooser-file="$out"
"$path"); simplify by collapsing them into a single else branch or a single
conditional that handles the shared behavior and remove the redundant elif
blocks in contrib/superfile-wrapper.sh; if you expect different future behavior,
replace the extra branches with one unified branch and add a TODO comment
referencing the variables directory and multiple and the set --
--chooser-file="$out" "$path" invocation so intent is preserved.
In `@src/internal/chooser.go`:
- Around line 113-150: In confirmSaveChooserPath, when the chosen path is an
existing directory (the info.IsDir() branch) and in the default/error branch
where stat returns a non‑IsNotExist error, surface a user-visible notification
instead of silently returning: call m.notifyModel with a short, user-friendly
message (e.g., "Please choose a file name; selected path is a directory" for the
directory case, and for the stat error include err.Error() like "Cannot access
target path: <err>") immediately before returning, while preserving existing
slog.Error logging and keeping the existing behavior of not proceeding with
chooserWriteAndQuit; update the info.IsDir() branch and the default case (where
you currently slog.Error("Save chooser target stat failed", ...)) to add these
notifyModel calls.
In `@src/internal/handle_file_operations.go`:
- Around line 571-585: The conditional in getRenameCursorPos is hard to read and
includes a redundant guard; replace the mixed ||/&& expression with a clearer
predicate such as checking cursorPos <= 0 or isDirectory. Specifically, update
getRenameCursorPos so the if uses a simple, parenthesized test (e.g., if
cursorPos <= 0 || isDirectory) and remove the unreachable nameLen > 0 check so
the function sets cursorPos = nameLen for both cursorPos == -1 and cursorPos ==
0 cases and for directories.
In `@src/internal/handle_modal.go`:
- Around line 75-81: The teardown for ending a rename is duplicated in the
m.isSaveChooserMode() branch and in cancelRename; extract those repeated
statements into a new helper method (e.g., m.clearRenamingState()) that performs
the shared steps (setting panel.Rename.Blur(), panel.Renaming = false,
panel.SetSaveEntryName(panel.Rename.Value()) or the correct ordering used
elsewhere) and call this helper from the m.isSaveChooserMode() branch and from
the end of confirmRename/cancelRename (replace the duplicated lines in the
branch with a single call to m.clearRenamingState()) so the rename-cleanup logic
is centralized and cannot drift.
In `@src/internal/key_function.go`:
- Around line 58-106: The repeated guard `if m.isSaveChooserMode() { return nil
}` across the hotkey case branches should be consolidated: in the `mainKey`
handler (or the enclosing function handling `msg`) call `m.isSaveChooserMode()`
once and, if true, short-circuit only for the relevant hotkeys (e.g.
common.Hotkeys.ChangePanelMode, NextFilePanel, PreviousFilePanel,
CloseFilePanel, CreateNewFilePanel, SplitFilePanel) by checking membership
(slices.Contains) against a denylist and returning nil; alternatively delegate
those hotkeys to a small helper like `denyInSaveChooser(msg)` that performs the
membership check, then continue with the remaining switch body unchanged
(preserving calls to `m.getFocusedFilePanel().ChangeFilePanelMode()`,
`m.fileModel.NextFilePanel()`, `m.fileModel.PreviousFilePanel()`,
`m.fileModel.CloseFilePanel()`,
`m.fileModel.CreateNewFilePanel(variable.HomeDir)`, and `m.splitPanel()`).
In `@src/internal/ui/filepanel/utils.go`:
- Around line 178-206: EnableSaveMode currently calls scrollToCursor(0) which
early-returns when the panel is empty (see ElemCount/scrollToCursor), so callers
that enable save mode on an unloaded panel can leave the cursor inconsistent;
change EnableSaveMode to set m.cursor = 0 directly (and still call any necessary
UI refresh) instead of using scrollToCursor, or alternatively add a clear
comment documenting the precondition that elements must be loaded before calling
EnableSaveMode; update references to EnableSaveMode, scrollToCursor, ElemCount
and m.cursor accordingly.
- Around line 101-103: The comparator passed to slices.SortFunc currently
subtracts integers (return a.order - b.order) which can overflow; update the
comparator used on ordered (and type orderedSelection) to call
cmp.Compare(a.order, b.order) instead, and add the "cmp" import; keep the same
slices.SortFunc(ordered, func(a, b orderedSelection) int { ... }) call but
return cmp.Compare(a.order, b.order) so the sort is safe and explicit.
In `@testsuite/tests/chooser_file_multiselect_test.py`:
- Around line 32-35: The end_execution method currently does an unconditional
time.sleep(self.close_wait_time) which can flake; replace the blind sleep with
the harness/process exit wait primitive (or enable the existing
validate_spf_closed behavior) so the method waits for the spf process to
actually exit and the chooser file to be flushed before returning; update
end_execution to call the harness's wait_for_spf_exit /
wait_for_process_termination (or pass validate_spf_closed=True into the code
path that closes spf) instead of using time.sleep, and keep the existing debug
logs around the wait to retain observability.
In `@testsuite/tests/save_file_test.py`:
- Line 6: Remove the unused import "core.test_constants as tconst" from
testsuite/tests/save_file_test.py; locate the import statement for the symbol
tconst and delete it, leaving other tests intact—if only the overwrite-variant
uses tconst.FILE_TEXT1, keep that test's local import instead of a module-level
import to avoid unused imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a86c36f-69f6-4366-a93b-a9d0357f62df
📒 Files selected for processing (28)
README.mdcontrib/superfile-wrapper.shsrc/cmd/main.gosrc/cmd/main_test.gosrc/config/fixed_variable.gosrc/internal/chooser.gosrc/internal/chooser_test.gosrc/internal/common/predefined_variable.gosrc/internal/default_config.gosrc/internal/handle_file_operations.gosrc/internal/handle_modal.gosrc/internal/handle_panel_movement.gosrc/internal/key_function.gosrc/internal/model.gosrc/internal/model_test.gosrc/internal/type.gosrc/internal/ui/filemodel/update.gosrc/internal/ui/filepanel/columns.gosrc/internal/ui/filepanel/get_elements.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/selection_test.gosrc/internal/ui/filepanel/types.gosrc/internal/ui/filepanel/utils.gosrc/internal/ui/notify/type.gotestsuite/tests/chooser_file_multiselect_test.pytestsuite/tests/save_file_overwrite_test.pytestsuite/tests/save_file_test.pywebsite/src/content/docs/troubleshooting.md
Comment on lines
+117
to
+121
| if m.SaveMode { | ||
| elements = append([]Element{m.getSaveEntryElement()}, elements...) | ||
| } | ||
| return elements | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C4 '\b(applyTargetFileCursor|SelectAllItem|SingleItemSelect|ItemSelect(Up|Down))\b'
rg -nP -C3 'HasSaveEntry|IsSaveEntryFocused|GetSaveEntryLocation|EnableSaveMode'Repository: yorukot/superfile
Length of output: 11137
🏁 Script executed:
#!/bin/bash
# Check FindElementIndexByName implementation
rg -nP -A10 'func.*FindElementIndexByName' src/internal/ui/filepanel/
# Check how selected items are processed in copy/delete operations
rg -nP -C5 'copyMultipleItem|getDeleteTriggerCmd' src/internal/
# Check if SaveTarget is filtered in any selection/processing logic
rg -nP 'SaveTarget' src/internal/ui/filepanel/ | head -30
# Check SingleItemSelect to see if it can select the save entry
rg -nP -A8 'func.*SingleItemSelect' src/internal/ui/filepanel/Repository: yorukot/superfile
Length of output: 6833
🏁 Script executed:
#!/bin/bash
# Check GetSelectedLocations implementation
rg -nP -A15 'func.*GetSelectedLocations' src/internal/ui/filepanel/
# Check ToggleSelected and SetSelected
rg -nP -A8 'func.*\(ToggleSelected\|SetSelected\)' src/internal/ui/filepanel/
# Check if there's SaveTarget filtering in any selection methods
rg -nP 'SaveTarget.*selected|selected.*SaveTarget' src/internal/Repository: yorukot/superfile
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Get context around SaveTarget checks
rg -nP -B5 -A10 'SaveTarget' src/internal/model.go | head -40
rg -nP -B5 -A10 'SaveTarget' src/internal/ui/filemodel/update.go | head -40
# Check ToggleSelected and SetSelected implementations
rg -nP -A10 'func.*ToggleSelected\|func.*SetSelected' src/internal/ui/filepanel/Repository: yorukot/superfile
Length of output: 1232
🏁 Script executed:
#!/bin/bash
# Check ToggleSelected and SetSelected implementations
rg -nP -A12 'func \(m \*Model\) (ToggleSelected|SetSelected|SelectedCount)' src/internal/ui/filepanel/
# Check GetFocusedItem to see if it can return SaveTarget
rg -nP -A8 'func.*GetFocusedItem' src/internal/ui/filepanel/Repository: yorukot/superfile
Length of output: 4072
Synthetic save entry can flow into copy/delete operations — add SaveTarget guards.
The prepended save entry creates a selection issue: SelectAllItem() includes all elements (including SaveTarget), and SingleItemSelect() can select it if cursor==0. No downstream filtering prevents these from reaching copyMultipleItem() → GetSelectedLocations() → clipboard, potentially making the synthetic entry a "selected file" in copy/delete/chooser operations.
applyTargetFileCursor()uses name-based lookup, so it should locate the real file correctly, but confirm it handles the case whereSaveEntryNamematches a real filename.SelectAllItem()needs to skip SaveTarget entries, or selection handlers must filter them before clipboard/delete operations.scrollToCursor(0)inEnableSaveMode()lands on the save entry by design, which is correct as initial focus.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/internal/ui/filepanel/get_elements.go` around lines 117 - 121, The
synthetic SaveTarget element (created by m.getSaveEntryElement() when
m.SaveMode) must be excluded from selection and destructive operations: update
SelectAllItem and SingleItemSelect to skip elements whose Type or ID equals
SaveTarget (or whose Name equals SaveEntryName), and add a guard in
copyMultipleItem/GetSelectedLocations (or the higher-level clipboard/delete
handlers) to filter out any elements flagged as SaveTarget before performing
copy/delete; also ensure applyTargetFileCursor() checks for SaveEntryName
collisions and resolves to the real file if present (e.g., ignore synthetic
entry when mapping cursor by name). This ensures
scrollToCursor(0)/EnableSaveMode can focus the save entry while preventing it
from being treated as a selectable file for copy/delete.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/internal/ui/filepanel/navigation.go (1)
68-80: Consider extracting aSaveTarget-aware lookup helper.This hand-rolled loop is effectively
FindElementIndexByNamewith an extra!elem.SaveTargetfilter. Per the relevant snippet,FindElementIndexByNameinsrc/internal/ui/filepanel/utils.go:168-178is still used elsewhere (e.g.test_utils.go:234for cursor positioning) without the filter, so the two paths can diverge if a save target ever shares a name with a real file. Worth extracting once to keep behavior consistent across cursor-positioning call sites.♻️ Proposed helper extraction
In
src/internal/ui/filepanel/utils.go:// FindElementIndexByNameFiltered returns the first index whose Name matches, // optionally skipping SaveTarget entries. Use skipSaveTarget=true for cursor // positioning so the synthetic save entry is never targeted. func (m *Model) FindElementIndexByNameFiltered(name string, skipSaveTarget bool) int { for i, elem := range m.element { if elem.Name != name { continue } if skipSaveTarget && elem.SaveTarget { continue } return i } return -1 }Then in
navigation.go:func (m *Model) applyTargetFileCursor() { - idx := -1 - for i, elem := range m.element { - if elem.Name == m.TargetFile && !elem.SaveTarget { - idx = i - break - } - } - if idx != -1 { + if idx := m.FindElementIndexByNameFiltered(m.TargetFile, true); idx != -1 { m.scrollToCursor(idx) } m.TargetFile = "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/ui/filepanel/navigation.go` around lines 68 - 80, Extract a helper on Model in filepanel utils (e.g., FindElementIndexByNameFiltered(name string, skipSaveTarget bool) int) that searches m.element by Name and optionally skips elements with SaveTarget; replace the manual loop in applyTargetFileCursor to call this helper with skipSaveTarget=true and use the returned index in m.scrollToCursor; keep the original FindElementIndexByName behavior (or forward it to the new helper with skipSaveTarget=false) so other callers (like tests) retain current semantics.src/internal/chooser.go (1)
113-159: Notification titles/messages are inconsistent — extract tocommonalongsideSaveOverwriteWarn*.
warnModalForSaveOverwritecorrectly routes throughcommon.SaveOverwriteWarnTitle/common.SaveOverwriteWarnContent(L163), but the two other user-visible notifications in this file are hardcoded English literals at L122 ("Cannot save to a directory"/"Please choose a file name instead of a directory.") and L156 ("Cannot access target path"). For i18n/localization consistency with the rest of the save-chooser UI strings, hoist these intosrc/internal/commonthe same way.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/chooser.go` around lines 113 - 159, The two hardcoded notification title/body strings in confirmSaveChooserPath should be moved into src/internal/common like the existing SaveOverwriteWarnTitle/SaveOverwriteWarnContent constants and then referenced from this function; replace the literal "Cannot save to a directory" and "Please choose a file name instead of a directory." used in the notify.New call and the literal "Cannot access target path" (used in the error branch where notifyModel is set) with new common constants (e.g., SaveDirErrorTitle, SaveDirErrorContent, SaveAccessErrorTitle) so confirmSaveChooserPath and its notifyModel assignments use the common package strings for i18n consistency while leaving warnModalForSaveOverwrite unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contrib/superfile-wrapper.sh`:
- Around line 32-40: The code builds a single shell string in the variable
command by interpolating escaped args and then runs sh -c "$command", which
allows command-substitution injection via $(...) or backticks; instead, stop
constructing a shell string and invoke the terminal command with arguments
directly to avoid shell interpretation: remove the loop that appends escaped
args to command and the sh -c invocation and replace it with an exec/command
call that passes $termcmd, $cmd and the original "$@" as separate arguments
(refer to the variables command, termcmd, cmd and the use of "$@") so no user
input is eval‑ed by a shell.
In `@src/internal/chooser.go`:
- Around line 19-30: The initializeChooserState function currently calls
panel.EnableSaveMode(saveName) even when
panel.UpdateCurrentFilePanelDir(startDir) returns an error; change it so that on
error you do not blindly enable save mode: attempt to set the panel to the
fallbackDir returned by resolveSaveChooserStartPath (call
panel.UpdateCurrentFilePanelDir(fallbackDir) and only proceed if that succeeds),
and if the fallback also fails surface a user-visible notification/error and
return without calling panel.EnableSaveMode(saveName). Update
initializeChooserState to reference resolveSaveChooserStartPath,
panel.UpdateCurrentFilePanelDir, panel.EnableSaveMode, and the fallbackDir path
so the chooser is only enabled when the panel dir is successfully set.
In `@testsuite/tests/save_file_test.py`:
- Line 42: Fix the grammar in the assertion message in save_file_test.py: update
the f-string used with assert self.env.fs_mgr.check_exists(SAVE_OUT) to read
f"File {SAVE_OUT} does not exist" (replace "does not exists" with "does not
exist") so the message is grammatically correct when the check via
self.env.fs_mgr.check_exists(SAVE_OUT) fails.
---
Nitpick comments:
In `@src/internal/chooser.go`:
- Around line 113-159: The two hardcoded notification title/body strings in
confirmSaveChooserPath should be moved into src/internal/common like the
existing SaveOverwriteWarnTitle/SaveOverwriteWarnContent constants and then
referenced from this function; replace the literal "Cannot save to a directory"
and "Please choose a file name instead of a directory." used in the notify.New
call and the literal "Cannot access target path" (used in the error branch where
notifyModel is set) with new common constants (e.g., SaveDirErrorTitle,
SaveDirErrorContent, SaveAccessErrorTitle) so confirmSaveChooserPath and its
notifyModel assignments use the common package strings for i18n consistency
while leaving warnModalForSaveOverwrite unchanged.
In `@src/internal/ui/filepanel/navigation.go`:
- Around line 68-80: Extract a helper on Model in filepanel utils (e.g.,
FindElementIndexByNameFiltered(name string, skipSaveTarget bool) int) that
searches m.element by Name and optionally skips elements with SaveTarget;
replace the manual loop in applyTargetFileCursor to call this helper with
skipSaveTarget=true and use the returned index in m.scrollToCursor; keep the
original FindElementIndexByName behavior (or forward it to the new helper with
skipSaveTarget=false) so other callers (like tests) retain current semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3ff0b9a-8fb0-4275-a2bd-769f71d77c90
📒 Files selected for processing (13)
contrib/superfile-wrapper.shsrc/cmd/main_test.gosrc/internal/chooser.gosrc/internal/handle_file_operations.gosrc/internal/handle_modal.gosrc/internal/ui/filepanel/columns.gosrc/internal/ui/filepanel/navigation.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/update.gosrc/internal/ui/filepanel/utils.gotestsuite/tests/chooser_file_multiselect_test.pytestsuite/tests/save_file_test.pywebsite/src/content/docs/troubleshooting.md
✅ Files skipped from review due to trivial changes (1)
- src/internal/ui/filepanel/render.go
🚧 Files skipped from review as they are similar to previous changes (6)
- src/internal/ui/filepanel/columns.go
- src/cmd/main_test.go
- src/internal/handle_modal.go
- testsuite/tests/chooser_file_multiselect_test.py
- src/internal/handle_file_operations.go
- src/internal/ui/filepanel/utils.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contrib/superfile-wrapper.sh (1)
6-7: Mark intentionally unused portal arguments as used.
multipleanddirectoryare part of the portal ABI, but ShellCheck flags them because this wrapper intentionally treats all open chooser variants the same today. A tiny no-op use keeps the script clean while preserving the documented parameters.♻️ Suggested cleanup
multiple="$1" directory="$2" save="$3" path="$4" out="$5" debug="$6" + +# The portal passes these flags; superfile currently uses the same open flow for them. +: "$multiple" "$directory"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contrib/superfile-wrapper.sh` around lines 6 - 7, Mark the intentionally unused portal ABI variables by adding a no-op reference so ShellCheck stops warning; specifically, add a line such as : "$multiple" "$directory" (or an equivalent no-op usage) near the top of the script to reference the multiple and directory variables without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/internal/chooser.go`:
- Around line 153-167: The placeholder is created non-atomically and can be
clobbered by a raced existing file; update createSaveChooserPlaceholder to
create the placeholder file using os.OpenFile with flags O_CREATE|O_EXCL (and
appropriate mode) instead of os.WriteFile, detect EEXIST and return a specific
error so the caller (the code path that calls createSaveChooserPlaceholder and
chooserWriteAndQuit) can fall back to overwrite confirmation; make the same
change for the other placeholder creation site referenced around lines 195-196
so both code paths use atomic create-with-excl and consistent EEXIST handling.
---
Nitpick comments:
In `@contrib/superfile-wrapper.sh`:
- Around line 6-7: Mark the intentionally unused portal ABI variables by adding
a no-op reference so ShellCheck stops warning; specifically, add a line such as
: "$multiple" "$directory" (or an equivalent no-op usage) near the top of the
script to reference the multiple and directory variables without changing
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4234ad9-f9c5-4f94-8185-ae9b589dcdf2
📒 Files selected for processing (4)
contrib/superfile-wrapper.shsrc/internal/chooser.gosrc/internal/common/predefined_variable.gotestsuite/tests/save_file_test.py
✅ Files skipped from review due to trivial changes (1)
- testsuite/tests/save_file_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/internal/chooser.go (1)
233-238:⚠️ Potential issue | 🟡 MinorClean up the placeholder if
Closefails.
OpenFilemay have already created the target; ifClosereturns an error, this returns failure while leaving the placeholder behind. This is rare, but it preserves the same cleanup guarantee as the write-failure path.🧹 Suggested cleanup
func createSaveChooserPlaceholder(path string) error { file, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, utils.UserFilePerm) if err != nil { return err } - return file.Close() + if closeErr := file.Close(); closeErr != nil { + if removeErr := os.Remove(path); removeErr != nil { + return errors.Join(closeErr, removeErr) + } + return closeErr + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/internal/chooser.go` around lines 233 - 238, The createSaveChooserPlaceholder function currently returns the Close error but leaves the created placeholder file behind; update createSaveChooserPlaceholder to attempt to close the file and if file.Close() returns an error, remove the placeholder (os.Remove(path)) before returning the error. Ensure you still return the Close error (or wrap it) after the cleanup so callers see the failure; reference the os.OpenFile call and the file.Close() call in createSaveChooserPlaceholder when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/internal/chooser.go`:
- Around line 167-180: The save flow currently only logs filesystem errors and
clears overwrite state too early; update the error handling around
createSaveChooserPlaceholder and parent dir checks to surface failures to the
user via the TUI (e.g., call the existing modal/error display method used
elsewhere) instead of just slog.Error, and do not clear m.overwritePath inside
confirmSaveOverwrite until the actual write completes successfully; also apply
the same behavior in the other save-related blocks referenced (lines roughly
183-204 and 217-226) so any os.Stat, createSaveChooserPlaceholder, or write
errors trigger a visible error modal and preserve overwritePath for retry rather
than dropping it on first failure.
---
Duplicate comments:
In `@src/internal/chooser.go`:
- Around line 233-238: The createSaveChooserPlaceholder function currently
returns the Close error but leaves the created placeholder file behind; update
createSaveChooserPlaceholder to attempt to close the file and if file.Close()
returns an error, remove the placeholder (os.Remove(path)) before returning the
error. Ensure you still return the Close error (or wrap it) after the cleanup so
callers see the failure; reference the os.OpenFile call and the file.Close()
call in createSaveChooserPlaceholder when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb2ef957-cf0d-4e8f-a32a-ff1bf97428da
📒 Files selected for processing (8)
contrib/superfile-wrapper.shsrc/cmd/main.gosrc/cmd/main_test.gosrc/internal/chooser.gosrc/internal/key_function.gosrc/internal/model_test.gosrc/internal/ui/filepanel/render.gosrc/internal/ui/filepanel/render_test.go
✅ Files skipped from review due to trivial changes (1)
- contrib/superfile-wrapper.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cmd/main_test.go
- src/internal/ui/filepanel/render.go
- src/internal/model_test.go
Comment on lines
+167
to
+180
| parentDir := filepath.Dir(path) | ||
| parentInfo, statErr := os.Stat(parentDir) | ||
| if statErr != nil || !parentInfo.IsDir() { | ||
| slog.Error("Save chooser target parent is invalid", "path", path, "error", statErr) | ||
| return | ||
| } | ||
|
|
||
| if createErr := createSaveChooserPlaceholder(path); createErr != nil { | ||
| if errors.Is(createErr, os.ErrExist) { | ||
| m.warnModalForSaveOverwrite(path) | ||
| return | ||
| } | ||
| slog.Error("Error while creating save chooser placeholder", "path", path, "error", createErr) | ||
| return |
Contributor
There was a problem hiding this comment.
Surface save failures in the UI and preserve overwrite retry state.
These save-flow errors only go to logs, so the TUI stays open without telling the user why confirmation failed. Also, confirmSaveOverwrite clears overwritePath before the output write succeeds, so a transient write failure loses the pending target.
🛠️ Suggested direction
func (m *model) confirmNewSaveChooserPath(path string) {
parentDir := filepath.Dir(path)
parentInfo, statErr := os.Stat(parentDir)
if statErr != nil || !parentInfo.IsDir() {
+ message := common.SaveDirErrorContent
+ if statErr != nil {
+ message = statErr.Error()
+ }
+ m.notifyModel = notify.New(true, common.SaveAccessErrorTitle, message, notify.NoAction)
slog.Error("Save chooser target parent is invalid", "path", path, "error", statErr)
return
}
if createErr := createSaveChooserPlaceholder(path); createErr != nil {
if errors.Is(createErr, os.ErrExist) {
m.warnModalForSaveOverwrite(path)
return
}
+ m.notifyModel = notify.New(true, common.SaveAccessErrorTitle, createErr.Error(), notify.NoAction)
slog.Error("Error while creating save chooser placeholder", "path", path, "error", createErr)
return
}
if writeErr := m.chooserWriteAndQuit([]string{path}); writeErr != nil {
+ m.notifyModel = notify.New(true, common.SaveAccessErrorTitle, writeErr.Error(), notify.NoAction)
removeErr := os.Remove(path)
if removeErr != nil {
slog.Error(
@@
func (m *model) confirmSaveOverwrite() {
if m.chooser.overwritePath == "" {
return
}
path := m.chooser.overwritePath
- m.chooser.overwritePath = ""
if err := m.chooserWriteAndQuit([]string{path}); err != nil {
+ m.notifyModel = notify.New(true, common.SaveAccessErrorTitle, err.Error(), notify.NoAction)
slog.Error("Error while confirming save overwrite", "error", err)
+ return
}
+ m.chooser.overwritePath = ""
}Also applies to: 183-204, 217-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/internal/chooser.go` around lines 167 - 180, The save flow currently only
logs filesystem errors and clears overwrite state too early; update the error
handling around createSaveChooserPlaceholder and parent dir checks to surface
failures to the user via the TUI (e.g., call the existing modal/error display
method used elsewhere) instead of just slog.Error, and do not clear
m.overwritePath inside confirmSaveOverwrite until the actual write completes
successfully; also apply the same behavior in the other save-related blocks
referenced (lines roughly 183-204 and 217-226) so any os.Stat,
createSaveChooserPlaceholder, or write errors trigger a visible error modal and
preserve overwritePath for retry rather than dropping it on first failure.
Owner
|
I would take some time to review when I am free, since this is a big PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This adds two related chooser improvements :
--chooser-filenow supports ordered multi-select output--save-fileadds a dedicated save flow for portal/file-chooser usageThe save flow includes a pinned save target entry in the panel, confirmation via the existing open keys, and overwrite confirmation for real existing files.
What I changed
--save-fileCLI support--chooser-fileand--save-filemutually exclusiveeandEcontrib/superfile-wrapper.shexample updated for--save-fileWhy the extra save placeholder logic exists
xdg-desktop-portal-termfilechooservalidates save selections against a real file on disk whencreate_help_file=1is enabled, which is the default.That means returning only a synthetic path is not enough for save mode to work correctly when the target is moved to a different directory.
To handle that correctly, I made sure it creates the real empty placeholder file at the confirmed destination before returning the selected path.
Testing
I Ran:
go test ./src/cmdgo test ./src/internal/ui/filepanelgo test ./src/internal -run 'TestChooserFile|TestChooserFileMultiSelectWritesSelectionOrder|TestSaveChooser|TestResolveSaveChooserStartPath'go test ./... -run '^$'python3 testsuite/main.py -t ChooserFileTest ChooserFileMultiSelectTest SaveFileTest SaveFileOverwriteTestNotes
contrib/superfile-wrapper.sh--chooser-file, save mode will not be used obiouslySummary by CodeRabbit
New Features
Documentation
Tests